diff mbox series

[1/3] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

Message ID 20221124200007.390901-2-hdegoede@redhat.com (mailing list archive)
State Not Applicable
Headers
Series platform/x86: int3472/discrete: Make it work with IPU6 |

Commit Message

Hans de Goede Nov. 24, 2022, 8 p.m. UTC 
  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(-)

Comments

Andy Shevchenko Nov. 24, 2022, 8:09 p.m. UTC | #1 
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";
> +}
Hans de Goede Nov. 24, 2022, 8:20 p.m. UTC | #2 
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
Andy Shevchenko Nov. 24, 2022, 10:19 p.m. UTC | #3 
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.
Dan Scally Nov. 25, 2022, 4 p.m. UTC | #4 
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:
Hans de Goede Nov. 28, 2022, 10:56 a.m. UTC | #5 
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 mbox series

Patch

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: