| Message ID | 20221216113013.126881-8-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: lookup-table support + int3472/media privacy LED support | expand |
On Fri, Dec 16, 2022 at 12:30:09PM +0100, Hans de Goede wrote: > Add a helper function to map the type returned by the _DSM > method to a function name + the default polarity for that function. > > And fold the INT3472_GPIO_TYPE_RESET and INT3472_GPIO_TYPE_POWERDOWN > cases into a single generic case. > > This is a preparation patch for further GPIO mapping changes. ... > +static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity) I find a bit strange not making this a function that returns func: static const char *int3472_get_func_and_polarity(u8 type, u32 *polarity) > +{ > + switch (type) { > + case INT3472_GPIO_TYPE_RESET: > + *func = "reset"; > + *polarity = GPIO_ACTIVE_LOW; return "reset"; etc. > + break; > + case INT3472_GPIO_TYPE_POWERDOWN: > + *func = "powerdown"; > + *polarity = GPIO_ACTIVE_LOW; > + break; > + case INT3472_GPIO_TYPE_CLK_ENABLE: > + *func = "clk-enable"; > + *polarity = GPIO_ACTIVE_HIGH; > + break; > + case INT3472_GPIO_TYPE_PRIVACY_LED: > + *func = "privacy-led"; > + *polarity = GPIO_ACTIVE_HIGH; > + break; > + case INT3472_GPIO_TYPE_POWER_ENABLE: > + *func = "power-enable"; > + *polarity = GPIO_ACTIVE_HIGH; > + break; > + default: > + *func = "unknown"; > + *polarity = GPIO_ACTIVE_HIGH; > + break; > + } > +}
On Fri, Dec 16, 2022 at 05:15:14PM +0100, Hans de Goede wrote: > On 12/16/22 14:57, Andy Shevchenko wrote: > > On Fri, Dec 16, 2022 at 12:30:09PM +0100, Hans de Goede wrote: ... > >> +static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity) > > > > I find a bit strange not making this a function that returns func: > > > > static const char *int3472_get_func_and_polarity(u8 type, u32 *polarity) > > Why make it return func and not polarity ? Because of name "func" and "polarity". And as you read a prototype from left to right it exactly follows that. > Since there are 2 values to return it makes sense to be > consistent and return both by reference. But this is unneeded complication, no? > Also this sort of comments really get into the territory > of bikeshedding which is not helpful IMHO. I find helpful to have a prototype shorter and the switch-case shorter due to return vs break. Of course you can think it's unhelpful, but I have another opinion. All by all you are the maintainer there, your call.
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index 974a132db651..bd3797ce64bf 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -184,6 +184,36 @@ static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472, return 0; } +static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity) +{ + switch (type) { + case INT3472_GPIO_TYPE_RESET: + *func = "reset"; + *polarity = GPIO_ACTIVE_LOW; + break; + case INT3472_GPIO_TYPE_POWERDOWN: + *func = "powerdown"; + *polarity = GPIO_ACTIVE_LOW; + break; + case INT3472_GPIO_TYPE_CLK_ENABLE: + *func = "clk-enable"; + *polarity = GPIO_ACTIVE_HIGH; + break; + case INT3472_GPIO_TYPE_PRIVACY_LED: + *func = "privacy-led"; + *polarity = GPIO_ACTIVE_HIGH; + break; + case INT3472_GPIO_TYPE_POWER_ENABLE: + *func = "power-enable"; + *polarity = GPIO_ACTIVE_HIGH; + break; + default: + *func = "unknown"; + *polarity = GPIO_ACTIVE_HIGH; + break; + } +} + /** * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor * @ares: A pointer to a &struct acpi_resource @@ -223,6 +253,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, struct acpi_resource_gpio *agpio; union acpi_object *obj; const char *err_msg; + const char *func; + u32 polarity; int ret; u8 type; @@ -246,19 +278,14 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, type = obj->integer.value & 0xff; + int3472_get_func_and_polarity(type, &func, &polarity); + switch (type) { case INT3472_GPIO_TYPE_RESET: - ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, "reset", - GPIO_ACTIVE_LOW); - if (ret) - err_msg = "Failed to map reset pin to sensor\n"; - - break; case INT3472_GPIO_TYPE_POWERDOWN: - ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, "powerdown", - GPIO_ACTIVE_LOW); + ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity); if (ret) - err_msg = "Failed to map powerdown pin to sensor\n"; + err_msg = "Failed to map GPIO pin to sensor\n"; break; case INT3472_GPIO_TYPE_CLK_ENABLE:
Add a helper function to map the type returned by the _DSM method to a function name + the default polarity for that function. And fold the INT3472_GPIO_TYPE_RESET and INT3472_GPIO_TYPE_POWERDOWN cases into a single generic case. This is a preparation patch for further GPIO mapping changes. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v3: - Add break to default case Changes in v2: - Make the helper function doing the type -> function mapping, also return a default polarity for the function. --- drivers/platform/x86/intel/int3472/discrete.c | 45 +++++++++++++++---- 1 file changed, 36 insertions(+), 9 deletions(-)