| Message ID | 20221129231149.697154-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ov5693/int3472: Privacy LED handling changes + IPU6 compatibility | expand |
On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> 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. ... > + 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; A nit-pick: In long term maintenance it's always good to have a break statement even in the default case. > + }
Hi, On 11/30/22 10:49, Andy Shevchenko wrote: > On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> 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. > > ... > >> + 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; > > A nit-pick: In long term maintenance it's always good to have a break > statement even in the default case. Ack, I'll add this when merging this (unless there are other / bigger reasons for a v2). Regards, Hans
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index 974a132db651..1eb053d13353 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -184,6 +184,35 @@ 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; + } +} + /** * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor * @ares: A pointer to a &struct acpi_resource @@ -223,6 +252,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 +277,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 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 | 44 +++++++++++++++---- 1 file changed, 35 insertions(+), 9 deletions(-)