Re: [PATCH v2 1/1] platform/x86: int3472: Call "reset" GPIO "enable" for INT347E

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 20 Jan 2025, Sakari Ailus wrote:

> The DT bindings for ov7251 specify "enable" GPIO (xshutdown in
> documentation) but the int3472 indiscriminately provides this as a "reset"
> GPIO to sensor drivers. Take this into account by assigning it as "enable"
> with active high polarity for INT347E devices, i.e. ov7251.
> 

I suggest to mention here that "reset" remains the default.

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> since v1:
> 
> - Fixed device name string.
> 
>  drivers/platform/x86/intel/int3472/discrete.c | 45 ++++++++++++++++---
>  1 file changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index d881b2cfcdfc..6404ef1eb4a7 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -122,13 +122,47 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
>  	return desc;
>  }
>  
> -static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
> +/**
> + * struct int3472_reset_gpio_map - Map "reset" GPIO to whatever is expected by
> + * the sensor driver (as in DT bindings)
> + * @devname: The name of the device without the instance number e.g. i2c-INT347E
> + * @func: The function, e.g. "enable"
> + * @polarity: GPIO_ACTIVE_{HIGH,LOW}
> + */
> +static const struct int3472_reset_gpio_map {
> +	const char *devname;
> +	const char *func;
> +	unsigned int polarity;
> +} int3472_reset_gpio_map[] = {
> +	{ "i2c-INT347E", "enable", GPIO_ACTIVE_HIGH },
> +};
> +
> +static void int3472_get_func_and_polarity(const char *sensor_name, u8 type,
> +					  const char **func, u32 *polarity)
>  {
>  	switch (type) {
> -	case INT3472_GPIO_TYPE_RESET:
> -		*func = "reset";
> -		*polarity = GPIO_ACTIVE_LOW;
> +	case INT3472_GPIO_TYPE_RESET: {
> +		const struct int3472_reset_gpio_map *map = NULL;
> +		unsigned int i;
> +
> +		for (i = 0; i < ARRAY_SIZE(int3472_reset_gpio_map); i++) {
> +			if (strncmp(sensor_name, int3472_reset_gpio_map[i].devname,


Please add #include for ARRAY_SIZE() and strncmp().

> +				    strlen(int3472_reset_gpio_map[i].devname)))
> +				continue;
> +
> +			map = &int3472_reset_gpio_map[i];
> +			break;
> +		}
> +
> +		if (map) {
> +			*func = map->func;
> +			*polarity = map->polarity;
> +		} else {
> +			*func = "reset";
> +			*polarity = GPIO_ACTIVE_LOW;
> +		}
>  		break;
> +	}
>  	case INT3472_GPIO_TYPE_POWERDOWN:
>  		*func = "powerdown";
>  		*polarity = GPIO_ACTIVE_LOW;
> @@ -217,7 +251,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  
>  	type = FIELD_GET(INT3472_GPIO_DSM_TYPE, obj->integer.value);
>  
> -	int3472_get_func_and_polarity(type, &func, &polarity);
> +	int3472_get_func_and_polarity(int3472->sensor_name, type, &func,
> +				      &polarity);
>  
>  	pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
>  	if (pin != agpio->pin_table[0])
> 

-- 
 i.