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

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

[Sakari Ailus]

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>
---
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])
-- 
2.39.5

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

[Ilpo Järvinen]

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.

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

[Hans de Goede]

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

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

[Sakari Ailus]

Hi Hans,

On Mon, Jan 20, 2025 at 02:39:39PM +0100, Hans de Goede wrote:
> 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:

I beg you to reconsider as I have to disagree with your assessment, for the
following reasons:

The "reset" naming used by the int3472 driver comes from the int3472 driver
/ Windows specific ACPI amendments, not from the ACPI standard nor sensor
datasheets. Also in a proper ACPI implementation we wouldn't be dealing
with such GPIOs at all, instead this would simply work through ACPI power
resources.

There generally isn't a single datasheet name used for such signals across
sensor vendors (or even sensors from a single vendor). Thus the assumption
made in the int3472 driver isn't correct, even if DT bindings were using
the vendor-provided GPIO signal name from datasheets as-is.

We've done quite a bit of work to make the system firmware (including
design differences or outright bugs) invisible to the drivers elsewhere, I
don't see why we should make an exception here. To add to that, mapping the
GPIO name / function to what the driver expects is trivially done in the
int3472 driver, as this patch shows.

The naming used in Devicetree bindings is an ABI, we cannot change that.
For drivers it's an authoritative reference, even if the naming is not
aligned with hardware datasheets. DT binding authors are within their
rights in naming things differently form datasheets, too. I object drivers
having to assume GPIO naming imposed by the int3472 driver when it
conflicts with the naming (and functionality) used in DT bindings. Camera
sensors are a bit special as they require less trivial resources (GPIOs,
regulators and clocks) than most other devices while they are similarly
used in both DT and ACPI based systems.
  
> 
> "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.

FWIW, the int3472 driver uses "power-enable" for a GPIO that powers on a
device. Do you expect drivers to support that as-is? Currently the int3472
driver appears to be the only one using that string in the kernel.

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

-- 
Kind regards,

Sakari Ailus

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

[Hans de Goede]

Hi Sakari,

On 20-Jan-25 3:21 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Jan 20, 2025 at 02:39:39PM +0100, Hans de Goede wrote:
>> 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:
> 
> I beg you to reconsider as I have to disagree with your assessment, for the
> following reasons:
> 
> The "reset" naming used by the int3472 driver comes from the int3472 driver
> / Windows specific ACPI amendments, not from the ACPI standard nor sensor
> datasheets.

Yet looking at the datasheet it is a more correct name then "enable".

> Also in a proper ACPI implementation we wouldn't be dealing
> with such GPIOs at all, instead this would simply work through ACPI power
> resources.

GPIOs being handles as ACPI power-resources is not something which is
typically done.

This was actually done correct for the atomisp devices, clks and regulators
are power-resources there, but the GPIOs are listed as plain ACPI GPIO
resources under the sensor ACPI-fwnode. And ACPI GPIO resources do not
have a name/label only an index. So drivers which need GPIOs and want 
to work on ACPI platforms need an index -> label map, see for example:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/goodix.c#n811

note how this is handled in the driver, and is not expected to be
handled by some platform specific code.

> There generally isn't a single datasheet name used for such signals across
> sensor vendors (or even sensors from a single vendor). Thus the assumption
> made in the int3472 driver isn't correct, even if DT bindings were using
> the vendor-provided GPIO signal name from datasheets as-is.
> 
> We've done quite a bit of work to make the system firmware (including
> design differences or outright bugs) invisible to the drivers elsewhere, I
> don't see why we should make an exception here. To add to that, mapping the
> GPIO name / function to what the driver expects is trivially done in the
> int3472 driver, as this patch shows.

IMHO it is not so trivial done, you are assuming a 1:1 mapping for all
laptop/tablet models this is not necessarily true. E.g. on atomisp tablets
there typically is a standard camera connector with both reset and powerdown
signals and the atomisp _DSM equivalent of the INT3472 GPIO map _DSM
typically contains both signals. But on sensors with only 1 reset pin
it is unclear which of the 2 is actually used. One model camera module
with sensor X may connect the sensor's single xshutdown pin to
the powerdown signal on the standard camera connector, while another
camera module may use the reset signal on the standard connector.

There are 2 ways to handle this:

1. Make the driver deal with the fact that there may be 2 GPIOs,
treating both as optional and driving both low/high at the same time
since only 1 will actually be used. This is somewhat ugly on the driver
side, but then fixes things for all tablets/laptops out there using
this sensor model.

2. Make this the platform glue's problem and require the platform code
to have per laptop/tablet model quirks using DMI matching meaning that
instead of things just working OOTB for models not added to the quirk
table, we now need users to report an issue and then manually fix
that for every model under the sun. Which is very much sub optimal.

See e.g.:

https://github.com/jwrdegoede/linux-sunxi/commit/e43be8f19b5913a2e4bd715e7eef88fd909a2d1d

(which I have not posted upstream yet since I don't have the mt9m114
driver working on atomisp models yet)

I foresee similar problems with the INT3472 stuff. On Andy's device
we need to map reset to enable, but maybe next time it is powerdown ?

Multiply these kinda per laptop/tablet model issues by the amount
of different sensors which there are and this becomes a big 
mess of per-sensor + per-laptop-model quirks in the int3472 code,
where as these things can typically be handled reasonable well
inside the sensor driver.

> The naming used in Devicetree bindings is an ABI, we cannot change that.

Right and Andy's patch does not try to change, nor break the ABI,
Andy's patch merely adds a fallback to look for a "reset" pin if
there is no "enable" pin which is nice and simple and clean and
really not much of a burden to carry inside the sensor driver.

Drivers outside of drivers/media/i2c have much bigger kludges to
deal with interfacing with ACPI tables.

> For drivers it's an authoritative reference, even if the naming is not
> aligned with hardware datasheets. DT binding authors are within their
> rights in naming things differently form datasheets, too. I object drivers
> having to assume GPIO naming imposed by the int3472 driver when it
> conflicts with the naming (and functionality) used in DT bindings. Camera
> sensors are a bit special as they require less trivial resources (GPIOs,
> regulators and clocks) than most other devices while they are similarly
> used in both DT and ACPI based systems.
>   
>>
>> "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.
> 
> FWIW, the int3472 driver uses "power-enable" for a GPIO that powers on a
> device. Do you expect drivers to support that as-is? Currently the int3472
> driver appears to be the only one using that string in the kernel.

Finding that pin actually results in the INT3472 code registering
an avdd regulator, not a GPIO lookup, in this case the "power-enable"
label is something purely internal to the INT3472 code.

This is all part of the INT3472 code already bending itself over
backwards to make things "invisible to the drivers" but IMHO we
need to draw the line somewhere.

For me the main reason for saying no to this is that it is
a per sensor thing and we already have a place to handle
per sensor things and that is in the sensor driver.

I've not seen any convincing arguments from you why this
sensor specific handling does not belong in the sensor
specific driver code.

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

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

[Sakari Ailus]

Hi Hans,

On Mon, Jan 20, 2025 at 04:08:45PM +0100, Hans de Goede wrote:
> Hi Sakari,
> 
> On 20-Jan-25 3:21 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Jan 20, 2025 at 02:39:39PM +0100, Hans de Goede wrote:
> >> 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:
> > 
> > I beg you to reconsider as I have to disagree with your assessment, for the
> > following reasons:
> > 
> > The "reset" naming used by the int3472 driver comes from the int3472 driver
> > / Windows specific ACPI amendments, not from the ACPI standard nor sensor
> > datasheets.
> 
> Yet looking at the datasheet it is a more correct name then "enable".

Possibly in this instance, but whether it is or not is still besides the
point: this may not be the name on the datasheet.

> 
> > Also in a proper ACPI implementation we wouldn't be dealing
> > with such GPIOs at all, instead this would simply work through ACPI power
> > resources.
> 
> GPIOs being handles as ACPI power-resources is not something which is
> typically done.
> 
> This was actually done correct for the atomisp devices, clks and regulators
> are power-resources there, but the GPIOs are listed as plain ACPI GPIO
> resources under the sensor ACPI-fwnode. And ACPI GPIO resources do not
> have a name/label only an index. So drivers which need GPIOs and want 
> to work on ACPI platforms need an index -> label map, see for example:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/goodix.c#n811
> 
> note how this is handled in the driver, and is not expected to be
> handled by some platform specific code.

The power resources do contain GPIO handling when it is required for
implementing the device's power on / off sequences. FWIW, on Chromebooks
this "just works" without the IPU bridge and without .

> 
> > There generally isn't a single datasheet name used for such signals across
> > sensor vendors (or even sensors from a single vendor). Thus the assumption
> > made in the int3472 driver isn't correct, even if DT bindings were using
> > the vendor-provided GPIO signal name from datasheets as-is.
> > 
> > We've done quite a bit of work to make the system firmware (including
> > design differences or outright bugs) invisible to the drivers elsewhere, I
> > don't see why we should make an exception here. To add to that, mapping the
> > GPIO name / function to what the driver expects is trivially done in the
> > int3472 driver, as this patch shows.
> 
> IMHO it is not so trivial done, you are assuming a 1:1 mapping for all
> laptop/tablet models this is not necessarily true. E.g. on atomisp tablets
> there typically is a standard camera connector with both reset and powerdown
> signals and the atomisp _DSM equivalent of the INT3472 GPIO map _DSM
> typically contains both signals. But on sensors with only 1 reset pin
> it is unclear which of the 2 is actually used. One model camera module
> with sensor X may connect the sensor's single xshutdown pin to
> the powerdown signal on the standard camera connector, while another
> camera module may use the reset signal on the standard connector.
> 
> There are 2 ways to handle this:
> 
> 1. Make the driver deal with the fact that there may be 2 GPIOs,
> treating both as optional and driving both low/high at the same time
> since only 1 will actually be used. This is somewhat ugly on the driver
> side, but then fixes things for all tablets/laptops out there using
> this sensor model.
> 
> 2. Make this the platform glue's problem and require the platform code
> to have per laptop/tablet model quirks using DMI matching meaning that
> instead of things just working OOTB for models not added to the quirk
> table, we now need users to report an issue and then manually fix
> that for every model under the sun. Which is very much sub optimal.

In our case here there's really no difference DMI-wise, is there? In both
cases the GPIO handling is determined based on the device. Besides, my
patch is technically better in the sense that it is explicitly using an
existing firmware interface instead of adding driver support for a firmware
interface that doesn't exist (neither as a firmware interface definition
nor an implementation of one).

> 
> See e.g.:
> 
> https://github.com/jwrdegoede/linux-sunxi/commit/e43be8f19b5913a2e4bd715e7eef88fd909a2d1d

I guess we don't have DT bindings for a (virtual) device that would allow
controlling one or more actual GPIOs using a single virtual one?

> 
> (which I have not posted upstream yet since I don't have the mt9m114
> driver working on atomisp models yet)
> 
> I foresee similar problems with the INT3472 stuff. On Andy's device
> we need to map reset to enable, but maybe next time it is powerdown ?
> 
> Multiply these kinda per laptop/tablet model issues by the amount
> of different sensors which there are and this becomes a big 
> mess of per-sensor + per-laptop-model quirks in the int3472 code,
> where as these things can typically be handled reasonable well
> inside the sensor driver.

Again, I don't want to add DMI checks in client drivers if that can
be reasonably handled elsewhere.

> 
> > The naming used in Devicetree bindings is an ABI, we cannot change that.
> 
> Right and Andy's patch does not try to change, nor break the ABI,
> Andy's patch merely adds a fallback to look for a "reset" pin if
> there is no "enable" pin which is nice and simple and clean and
> really not much of a burden to carry inside the sensor driver.
> 
> Drivers outside of drivers/media/i2c have much bigger kludges to
> deal with interfacing with ACPI tables.

That may well be. One of the reasons we have tried to avoid this in drivers
is that a large portion of any driver's code would consist of handling
firmware interface differences if we didn't. And most of that code would be
quite buggy.

Proving the point: the patch inverting GPIO polarity in a driver based on
the GPIO's name is much more complicated (a practice I'd expect to spread
across some number of drivers) than centrally connecting what int3472 digs
from Windows specific ACPI tables to what a sensor driver expects. Now, I'm
not claiming this will be always an easier, cleaner or even possible way to
do things but in this case it is IMHO.

> 
> > For drivers it's an authoritative reference, even if the naming is not
> > aligned with hardware datasheets. DT binding authors are within their
> > rights in naming things differently form datasheets, too. I object drivers
> > having to assume GPIO naming imposed by the int3472 driver when it
> > conflicts with the naming (and functionality) used in DT bindings. Camera
> > sensors are a bit special as they require less trivial resources (GPIOs,
> > regulators and clocks) than most other devices while they are similarly
> > used in both DT and ACPI based systems.
> >   
> >>
> >> "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.
> > 
> > FWIW, the int3472 driver uses "power-enable" for a GPIO that powers on a
> > device. Do you expect drivers to support that as-is? Currently the int3472
> > driver appears to be the only one using that string in the kernel.
> 
> Finding that pin actually results in the INT3472 code registering
> an avdd regulator, not a GPIO lookup, in this case the "power-enable"
> label is something purely internal to the INT3472 code.

Many sensors also have a power-down GPIO control besides reset.

> 
> This is all part of the INT3472 code already bending itself over
> backwards to make things "invisible to the drivers" but IMHO we
> need to draw the line somewhere.
> 
> For me the main reason for saying no to this is that it is
> a per sensor thing and we already have a place to handle
> per sensor things and that is in the sensor driver.
> 
> I've not seen any convincing arguments from you why this
> sensor specific handling does not belong in the sensor
> specific driver code.

Let's try to discuss today where that code lands on #linux-media, and
hopefully each some conclusion.

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

-- 
Kind regards,

Sakari Ailus

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

[Hans de Goede]

Hi Saikari,

On 21-Jan-25 9:08 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Jan 20, 2025 at 04:08:45PM +0100, Hans de Goede wrote:
>> Hi Sakari,
>>
>> On 20-Jan-25 3:21 PM, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Mon, Jan 20, 2025 at 02:39:39PM +0100, Hans de Goede wrote:
>>>> 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:
>>>
>>> I beg you to reconsider as I have to disagree with your assessment, for the
>>> following reasons:
>>>
>>> The "reset" naming used by the int3472 driver comes from the int3472 driver
>>> / Windows specific ACPI amendments, not from the ACPI standard nor sensor
>>> datasheets.
>>
>> Yet looking at the datasheet it is a more correct name then "enable".
> 
> Possibly in this instance, but whether it is or not is still besides the
> point: this may not be the name on the datasheet.
> 
>>
>>> Also in a proper ACPI implementation we wouldn't be dealing
>>> with such GPIOs at all, instead this would simply work through ACPI power
>>> resources.
>>
>> GPIOs being handles as ACPI power-resources is not something which is
>> typically done.
>>
>> This was actually done correct for the atomisp devices, clks and regulators
>> are power-resources there, but the GPIOs are listed as plain ACPI GPIO
>> resources under the sensor ACPI-fwnode. And ACPI GPIO resources do not
>> have a name/label only an index. So drivers which need GPIOs and want 
>> to work on ACPI platforms need an index -> label map, see for example:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/goodix.c#n811
>>
>> note how this is handled in the driver, and is not expected to be
>> handled by some platform specific code.
> 
> The power resources do contain GPIO handling when it is required for
> implementing the device's power on / off sequences. FWIW, on Chromebooks
> this "just works" without the IPU bridge and without .
> 
>>
>>> There generally isn't a single datasheet name used for such signals across
>>> sensor vendors (or even sensors from a single vendor). Thus the assumption
>>> made in the int3472 driver isn't correct, even if DT bindings were using
>>> the vendor-provided GPIO signal name from datasheets as-is.
>>>
>>> We've done quite a bit of work to make the system firmware (including
>>> design differences or outright bugs) invisible to the drivers elsewhere, I
>>> don't see why we should make an exception here. To add to that, mapping the
>>> GPIO name / function to what the driver expects is trivially done in the
>>> int3472 driver, as this patch shows.
>>
>> IMHO it is not so trivial done, you are assuming a 1:1 mapping for all
>> laptop/tablet models this is not necessarily true. E.g. on atomisp tablets
>> there typically is a standard camera connector with both reset and powerdown
>> signals and the atomisp _DSM equivalent of the INT3472 GPIO map _DSM
>> typically contains both signals. But on sensors with only 1 reset pin
>> it is unclear which of the 2 is actually used. One model camera module
>> with sensor X may connect the sensor's single xshutdown pin to
>> the powerdown signal on the standard camera connector, while another
>> camera module may use the reset signal on the standard connector.
>>
>> There are 2 ways to handle this:
>>
>> 1. Make the driver deal with the fact that there may be 2 GPIOs,
>> treating both as optional and driving both low/high at the same time
>> since only 1 will actually be used. This is somewhat ugly on the driver
>> side, but then fixes things for all tablets/laptops out there using
>> this sensor model.
>>
>> 2. Make this the platform glue's problem and require the platform code
>> to have per laptop/tablet model quirks using DMI matching meaning that
>> instead of things just working OOTB for models not added to the quirk
>> table, we now need users to report an issue and then manually fix
>> that for every model under the sun. Which is very much sub optimal.
> 
> In our case here there's really no difference DMI-wise, is there? In both
> cases the GPIO handling is determined based on the device. Besides, my
> patch is technically better in the sense that it is explicitly using an
> existing firmware interface instead of adding driver support for a firmware
> interface that doesn't exist (neither as a firmware interface definition
> nor an implementation of one).
> 
>>
>> See e.g.:
>>
>> https://github.com/jwrdegoede/linux-sunxi/commit/e43be8f19b5913a2e4bd715e7eef88fd909a2d1d
> 
> I guess we don't have DT bindings for a (virtual) device that would allow
> controlling one or more actual GPIOs using a single virtual one?
> 
>>
>> (which I have not posted upstream yet since I don't have the mt9m114
>> driver working on atomisp models yet)
>>
>> I foresee similar problems with the INT3472 stuff. On Andy's device
>> we need to map reset to enable, but maybe next time it is powerdown ?
>>
>> Multiply these kinda per laptop/tablet model issues by the amount
>> of different sensors which there are and this becomes a big 
>> mess of per-sensor + per-laptop-model quirks in the int3472 code,
>> where as these things can typically be handled reasonable well
>> inside the sensor driver.
> 
> Again, I don't want to add DMI checks in client drivers if that can
> be reasonably handled elsewhere.

Ok, good that was my main concern.

<snip>

As discussed on IRC lets move forward with doing the mapping in the INT3472
as the patch from this thread does.

But I do want to see the mapping code be a bit more generic, I'll reply
to the original patch with a suggestion on how to do that.

Regards,

Hans

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

[Hans de Goede]

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>
> ---
> 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;

Instead of using a devname match this should be changed to using
proper ACPI HID matching.

Instead of passing int3472->sensor_name as extra argument to
int3472_get_func_and_polarity(), pass int3472->sensor which
is a "struct acpi_device *" and then use e.g. :

	acpi_dev_hid_uid_match(int3472->sensor, "INT347E", NULL)

but then with the new sensor function argument

> +	const char *func;
> +	unsigned int polarity;
> +} int3472_reset_gpio_map[] = {
> +	{ "i2c-INT347E", "enable", GPIO_ACTIVE_HIGH },
> +};

Rather then having a mechanism for just the reset pin, I would prefer
to be able to remap any type to any type.

So I would like to see this struct changed to e.g. :

static struct int3472_gpio_map {
	const char *hid;
	u8 type_from;
	u8 type_to;
	const char *func;
	unsigned int polarity;
};

static const struct int3472_gpio_map[] = {
	{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, "enable", GPIO_ACTIVE_HIGH },
};

> +
> +static void int3472_get_func_and_polarity(const char *sensor_name, u8 type,

And change type to a * here ("u8 *type) so that its contents can be
overwritten by the mapping code

> +					  const char **func, u32 *polarity)

So the new function prototype would become:

static void int3472_get_func_and_polarity(struct acpi_device *sensor, u8 *type,
					  const char **func, u32 *polarity)


>  {

and do the for loop here before the (now) "switch (*type)":

	for (i = 0; i < ARRAY_SIZE(int3472_gpio_map); i++) {
		if (*type != int3472_reset_gpio_map[i].type_from ||
		    !acpi_dev_hid_uid_match(sensor, int3472_reset_gpio_map[i].hid, NULL))
			continue;

		*type = int3472_reset_gpio_map[i].type_to;
		*func = int3472_reset_gpio_map[i].func;
		*polarity = int3472_reset_gpio_map[i].polarity;
		return;
	}

This should give us a lot more flexibility for future mappings.

Regards,

Hans




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

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

[Sakari Ailus]

Hi Hans,

Thanks for the review.

On Tue, Jan 21, 2025 at 10:42:41AM +0100, Hans de Goede wrote:
> 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>
> > ---
> > 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;
> 
> Instead of using a devname match this should be changed to using
> proper ACPI HID matching.
> 
> Instead of passing int3472->sensor_name as extra argument to
> int3472_get_func_and_polarity(), pass int3472->sensor which
> is a "struct acpi_device *" and then use e.g. :
> 
> 	acpi_dev_hid_uid_match(int3472->sensor, "INT347E", NULL)
> 
> but then with the new sensor function argument
> 
> > +	const char *func;
> > +	unsigned int polarity;
> > +} int3472_reset_gpio_map[] = {
> > +	{ "i2c-INT347E", "enable", GPIO_ACTIVE_HIGH },
> > +};
> 
> Rather then having a mechanism for just the reset pin, I would prefer
> to be able to remap any type to any type.
> 
> So I would like to see this struct changed to e.g. :
> 
> static struct int3472_gpio_map {
> 	const char *hid;
> 	u8 type_from;
> 	u8 type_to;
> 	const char *func;

I'll place func before the u8 fields for fewer holes.

> 	unsigned int polarity;
> };
> 
> static const struct int3472_gpio_map[] = {
> 	{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, "enable", GPIO_ACTIVE_HIGH },
> };
> 
> > +
> > +static void int3472_get_func_and_polarity(const char *sensor_name, u8 type,
> 
> And change type to a * here ("u8 *type) so that its contents can be
> overwritten by the mapping code
> 
> > +					  const char **func, u32 *polarity)
> 
> So the new function prototype would become:
> 
> static void int3472_get_func_and_polarity(struct acpi_device *sensor, u8 *type,
> 					  const char **func, u32 *polarity)
> 
> 
> >  {
> 
> and do the for loop here before the (now) "switch (*type)":
> 
> 	for (i = 0; i < ARRAY_SIZE(int3472_gpio_map); i++) {
> 		if (*type != int3472_reset_gpio_map[i].type_from ||
> 		    !acpi_dev_hid_uid_match(sensor, int3472_reset_gpio_map[i].hid, NULL))
> 			continue;
> 
> 		*type = int3472_reset_gpio_map[i].type_to;
> 		*func = int3472_reset_gpio_map[i].func;
> 		*polarity = int3472_reset_gpio_map[i].polarity;
> 		return;
> 	}
> 
> This should give us a lot more flexibility for future mappings.

I'll address these in v3 along with Ilpo's comments.

-- 
Kind regards,

Sakari Ailus

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

[Hans de Goede]

Hi,

On 21-Jan-25 12:11 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the review.
> 
> On Tue, Jan 21, 2025 at 10:42:41AM +0100, Hans de Goede wrote:
>> 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>
>>> ---
>>> 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;
>>
>> Instead of using a devname match this should be changed to using
>> proper ACPI HID matching.
>>
>> Instead of passing int3472->sensor_name as extra argument to
>> int3472_get_func_and_polarity(), pass int3472->sensor which
>> is a "struct acpi_device *" and then use e.g. :
>>
>> 	acpi_dev_hid_uid_match(int3472->sensor, "INT347E", NULL)
>>
>> but then with the new sensor function argument
>>
>>> +	const char *func;
>>> +	unsigned int polarity;
>>> +} int3472_reset_gpio_map[] = {
>>> +	{ "i2c-INT347E", "enable", GPIO_ACTIVE_HIGH },
>>> +};
>>
>> Rather then having a mechanism for just the reset pin, I would prefer
>> to be able to remap any type to any type.
>>
>> So I would like to see this struct changed to e.g. :
>>
>> static struct int3472_gpio_map {
>> 	const char *hid;
>> 	u8 type_from;
>> 	u8 type_to;
>> 	const char *func;
> 
> I'll place func before the u8 fields for fewer holes.

Hmm, by itself that is a good idea, but ...
 
>> 	unsigned int polarity;
>> };
>>
>> static const struct int3472_gpio_map[] = {
>> 	{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, "enable", GPIO_ACTIVE_HIGH },

... that changes this line from the above to:

 	{ "INT347E", "enable", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, GPIO_ACTIVE_HIGH },

which IMHO is slightly less logical/readable. I would like to keep
the ACPI-hid and the type_from next to each other. So I have a slight
preference for keeping things as I suggested.

>> };
>>
>>> +
>>> +static void int3472_get_func_and_polarity(const char *sensor_name, u8 type,
>>
>> And change type to a * here ("u8 *type) so that its contents can be
>> overwritten by the mapping code
>>
>>> +					  const char **func, u32 *polarity)
>>
>> So the new function prototype would become:
>>
>> static void int3472_get_func_and_polarity(struct acpi_device *sensor, u8 *type,
>> 					  const char **func, u32 *polarity)
>>
>>
>>>  {
>>
>> and do the for loop here before the (now) "switch (*type)":
>>
>> 	for (i = 0; i < ARRAY_SIZE(int3472_gpio_map); i++) {
>> 		if (*type != int3472_reset_gpio_map[i].type_from ||
>> 		    !acpi_dev_hid_uid_match(sensor, int3472_reset_gpio_map[i].hid, NULL))
>> 			continue;
>>
>> 		*type = int3472_reset_gpio_map[i].type_to;
>> 		*func = int3472_reset_gpio_map[i].func;
>> 		*polarity = int3472_reset_gpio_map[i].polarity;
>> 		return;
>> 	}
>>
>> This should give us a lot more flexibility for future mappings.
> 
> I'll address these in v3 along with Ilpo's comments.

Great, thank you.

Regards,

Hans

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

[Sakari Ailus]

Hi Hans,

On Tue, Jan 21, 2025 at 01:35:49PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 21-Jan-25 12:11 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > Thanks for the review.
> > 
> > On Tue, Jan 21, 2025 at 10:42:41AM +0100, Hans de Goede wrote:
> >> 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>
> >>> ---
> >>> 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;
> >>
> >> Instead of using a devname match this should be changed to using
> >> proper ACPI HID matching.
> >>
> >> Instead of passing int3472->sensor_name as extra argument to
> >> int3472_get_func_and_polarity(), pass int3472->sensor which
> >> is a "struct acpi_device *" and then use e.g. :
> >>
> >> 	acpi_dev_hid_uid_match(int3472->sensor, "INT347E", NULL)
> >>
> >> but then with the new sensor function argument
> >>
> >>> +	const char *func;
> >>> +	unsigned int polarity;
> >>> +} int3472_reset_gpio_map[] = {
> >>> +	{ "i2c-INT347E", "enable", GPIO_ACTIVE_HIGH },
> >>> +};
> >>
> >> Rather then having a mechanism for just the reset pin, I would prefer
> >> to be able to remap any type to any type.
> >>
> >> So I would like to see this struct changed to e.g. :
> >>
> >> static struct int3472_gpio_map {
> >> 	const char *hid;
> >> 	u8 type_from;
> >> 	u8 type_to;
> >> 	const char *func;
> > 
> > I'll place func before the u8 fields for fewer holes.
> 
> Hmm, by itself that is a good idea, but ...
>  
> >> 	unsigned int polarity;
> >> };
> >>
> >> static const struct int3472_gpio_map[] = {
> >> 	{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, "enable", GPIO_ACTIVE_HIGH },
> 
> ... that changes this line from the above to:
> 
>  	{ "INT347E", "enable", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, GPIO_ACTIVE_HIGH },
> 
> which IMHO is slightly less logical/readable. I would like to keep
> the ACPI-hid and the type_from next to each other. So I have a slight
> preference for keeping things as I suggested.

Works for me.

-- 
Sakari Ailus