diff mbox series

[2/6] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

Message ID 20221129231149.697154-3-hdegoede@redhat.com (mailing list archive)
State Superseded
Delegated to: Sakari Ailus
Headers
Series ov5693/int3472: Privacy LED handling changes + IPU6 compatibility |

Commit Message

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

Comments

Andy Shevchenko Nov. 30, 2022, 9:49 a.m. UTC | #1 
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.

> +       }
Hans de Goede Nov. 30, 2022, 10:37 a.m. UTC | #2 
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 mbox series

Patch

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: