| Message ID | 20221124200007.390901-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | platform/x86: int3472/discrete: Make it work with IPU6 | expand |
On Thu, Nov 24, 2022 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Make the GPIO to sensor mapping more generic 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 const char *int3472_dsm_type_to_func(u8 type) > +{ > + switch (type) { > + case INT3472_GPIO_TYPE_RESET: > + return "reset"; > + case INT3472_GPIO_TYPE_POWERDOWN: > + return "powerdown"; > + case INT3472_GPIO_TYPE_CLK_ENABLE: > + return "clken"; > + case INT3472_GPIO_TYPE_PRIVACY_LED: > + return "pled"; > + case INT3472_GPIO_TYPE_POWER_ENABLE: > + return "power-enable"; default: return "unknown"; ? > + } > + > + return "unknown"; > +}
Hi, Thank you for the reviews! On 11/24/22 21:09, Andy Shevchenko wrote: > On Thu, Nov 24, 2022 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Make the GPIO to sensor mapping more generic 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 const char *int3472_dsm_type_to_func(u8 type) >> +{ >> + switch (type) { >> + case INT3472_GPIO_TYPE_RESET: >> + return "reset"; >> + case INT3472_GPIO_TYPE_POWERDOWN: >> + return "powerdown"; >> + case INT3472_GPIO_TYPE_CLK_ENABLE: >> + return "clken"; >> + case INT3472_GPIO_TYPE_PRIVACY_LED: >> + return "pled"; >> + case INT3472_GPIO_TYPE_POWER_ENABLE: >> + return "power-enable"; > > default: > return "unknown"; > > ? > >> + } >> + >> + return "unknown"; >> +} > In the passed some compiler versions complained about the non-void function not ending with a return statement. I guess I can give your variant a try (I agree it is more readable) and of we get compiler warnings we can switch back. I'll fix this up in the next version or when merging this, depending on further feedback on the series. Regards, Hans
On Thu, Nov 24, 2022 at 10:20 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/24/22 21:09, Andy Shevchenko wrote: > > On Thu, Nov 24, 2022 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > >> +static const char *int3472_dsm_type_to_func(u8 type) > >> +{ > >> + switch (type) { > >> + case INT3472_GPIO_TYPE_RESET: > >> + return "reset"; > >> + case INT3472_GPIO_TYPE_POWERDOWN: > >> + return "powerdown"; > >> + case INT3472_GPIO_TYPE_CLK_ENABLE: > >> + return "clken"; > >> + case INT3472_GPIO_TYPE_PRIVACY_LED: > >> + return "pled"; > >> + case INT3472_GPIO_TYPE_POWER_ENABLE: > >> + return "power-enable"; > > > > default: > > return "unknown"; > > > > ? > > > >> + } > >> + > >> + return "unknown"; > >> +} > > > > In the passed some compiler versions complained about the non-void > function not ending with a return statement. > > I guess I can give your variant a try (I agree it is more readable) > and of we get compiler warnings we can switch back. > > I'll fix this up in the next version or when merging this, > depending on further feedback on the series. I believe it's not the case for a long time. (Esp. when the kernel requires GCC 5.1 as bare minimum). We have a lot of (relatively) new code that uses switch-cases like I suggested and I have heard none of the complains from anybody, including all CIs crawling through the kernel code.
Hi Hans On 24/11/2022 20:00, Hans de Goede wrote: > Make the GPIO to sensor mapping more generic 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> > --- Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> Tested-by: Daniel Scally <dan.scally@ideasonboard.com> > drivers/platform/x86/intel/int3472/discrete.c | 31 ++++++++++++++----- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c > index 974a132db651..bc6c62f3f3bf 100644 > --- a/drivers/platform/x86/intel/int3472/discrete.c > +++ b/drivers/platform/x86/intel/int3472/discrete.c > @@ -184,6 +184,24 @@ static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472, > return 0; > } > > +static const char *int3472_dsm_type_to_func(u8 type) > +{ > + switch (type) { > + case INT3472_GPIO_TYPE_RESET: > + return "reset"; > + case INT3472_GPIO_TYPE_POWERDOWN: > + return "powerdown"; > + case INT3472_GPIO_TYPE_CLK_ENABLE: > + return "clken"; > + case INT3472_GPIO_TYPE_PRIVACY_LED: > + return "pled"; > + case INT3472_GPIO_TYPE_POWER_ENABLE: > + return "power-enable"; > + } > + > + return "unknown"; > +} > + > /** > * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor > * @ares: A pointer to a &struct acpi_resource > @@ -223,6 +241,7 @@ 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; > int ret; > u8 type; > > @@ -246,19 +265,15 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, > > type = obj->integer.value & 0xff; > > + func = int3472_dsm_type_to_func(type); > + > 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", > + ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, > GPIO_ACTIVE_LOW); > 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:
Hi, On 11/25/22 17:00, Dan Scally wrote: > Hi Hans > > On 24/11/2022 20:00, Hans de Goede wrote: >> Make the GPIO to sensor mapping more generic 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> >> --- > > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > > Tested-by: Daniel Scally <dan.scally@ideasonboard.com> Thank you. Note I have made some (not insignificant) changes to this patch for the v2 series which I'm working on, so I have decided to not add these tags because of the changes. Regards, Hans > >> drivers/platform/x86/intel/int3472/discrete.c | 31 ++++++++++++++----- >> 1 file changed, 23 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c >> index 974a132db651..bc6c62f3f3bf 100644 >> --- a/drivers/platform/x86/intel/int3472/discrete.c >> +++ b/drivers/platform/x86/intel/int3472/discrete.c >> @@ -184,6 +184,24 @@ static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472, >> return 0; >> } >> +static const char *int3472_dsm_type_to_func(u8 type) >> +{ >> + switch (type) { >> + case INT3472_GPIO_TYPE_RESET: >> + return "reset"; >> + case INT3472_GPIO_TYPE_POWERDOWN: >> + return "powerdown"; >> + case INT3472_GPIO_TYPE_CLK_ENABLE: >> + return "clken"; >> + case INT3472_GPIO_TYPE_PRIVACY_LED: >> + return "pled"; >> + case INT3472_GPIO_TYPE_POWER_ENABLE: >> + return "power-enable"; >> + } >> + >> + return "unknown"; >> +} >> + >> /** >> * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor >> * @ares: A pointer to a &struct acpi_resource >> @@ -223,6 +241,7 @@ 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; >> int ret; >> u8 type; >> @@ -246,19 +265,15 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >> type = obj->integer.value & 0xff; >> + func = int3472_dsm_type_to_func(type); >> + >> 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", >> + ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, >> GPIO_ACTIVE_LOW); >> 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: >
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index 974a132db651..bc6c62f3f3bf 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -184,6 +184,24 @@ static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472, return 0; } +static const char *int3472_dsm_type_to_func(u8 type) +{ + switch (type) { + case INT3472_GPIO_TYPE_RESET: + return "reset"; + case INT3472_GPIO_TYPE_POWERDOWN: + return "powerdown"; + case INT3472_GPIO_TYPE_CLK_ENABLE: + return "clken"; + case INT3472_GPIO_TYPE_PRIVACY_LED: + return "pled"; + case INT3472_GPIO_TYPE_POWER_ENABLE: + return "power-enable"; + } + + return "unknown"; +} + /** * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor * @ares: A pointer to a &struct acpi_resource @@ -223,6 +241,7 @@ 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; int ret; u8 type; @@ -246,19 +265,15 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, type = obj->integer.value & 0xff; + func = int3472_dsm_type_to_func(type); + 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", + ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, GPIO_ACTIVE_LOW); 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:
Make the GPIO to sensor mapping more generic 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> --- drivers/platform/x86/intel/int3472/discrete.c | 31 ++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-)