| Message ID | 20221216113013.126881-8-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| 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; > + } > +}
Hi, On 12/16/22 14:57, Andy Shevchenko wrote: > 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) Why make it return func and not polarity ? Since there are 2 values to return it makes sense to be consistent and return both by reference. Also this sort of comments really get into the territory of bikeshedding which is not helpful IMHO. Regards, Hans > >> +{ >> + 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(-)