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@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxx/ > > 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
