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

[Hans de Goede <hdegoede@redhat.com>]

Hi Sakari,

On 20-Jan-25 11:17 AM, 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.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Sorry but no this won't fly. If we go this route the amount of
quirk code in the int3472 driver is going to get out of control.

Since you are matching this mapping on the sensor-type, this
should be handled in sensor specific code, IOW in the sensor driver.

Also see my reply on the linux-media list here:

https://lore.kernel.org/linux-media/0a0b765e-b342-4433-9c6c-07da78f0f63b@redhat.com/

Sorry but I have to hard nack this. There has to be some line
where the pdx86 glue code stops bending over backwards and
where some of the burden of supporting more then just devicetree
moves to the sensor drivers.

*especially* since this mapping is going to be different per sensor-driver,
with there being *no consistency at all* in the GPIO/pin names used in
the sensor drivers just looking at drivers/media/i2c/ov*.c I see all of:

"enable"
"powerdown"
"pwdn"
"reset"
"resetb"
"Camera power"
"Camera reset"

being used. Given this total lack of consistent pin naming this really
needs to be fixed in the sensor drivers.

Not to mention the fact that the "reset" used by the ACPI tables is
easily more correct for the ov7251 then the "enable" the driver
currently expects given that the datasheet lists the pin as:

XSHUTDOWN input reset (active low with internal pull down resistor)

I don't see any "enable" in there were as the datasheet does say
"reset".

Regards,

Hans






> ---
> 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,
> +				    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])