* [PATCH v1 1/1] platform/x86: int3472: discrete: Remap "reset" to "enable" for OV7251 @ 2024-08-21 18:40 Andy Shevchenko 2024-08-26 15:12 ` Hans de Goede 0 siblings, 1 reply; 3+ messages in thread From: Andy Shevchenko @ 2024-08-21 18:40 UTC (permalink / raw) To: Ilpo Järvinen, Andy Shevchenko, Hans de Goede, platform-driver-x86, linux-kernel Cc: Daniel Scally The driver of OV7251 expects "enable" pin instead of "reset". Remap "reset" to "enable" and update polarity. In particular, the Microsoft Surface Book can't load the camera sensor driver without this change: ov7251 i2c-INT347E:00: supply vdddo not found, using dummy regulator ov7251 i2c-INT347E:00: supply vddd not found, using dummy regulator ov7251 i2c-INT347E:00: supply vdda not found, using dummy regulator ov7251 i2c-INT347E:00: cannot get enable gpio ov7251 i2c-INT347E:00: probe with driver ov7251 failed with error -2 Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- Hmm... I have spent some time to achieve this, and then I realised that linux-surface GitHub project already has something similar [1]. The advantage of [1] is that it applies the quirk to all OV7251 sensors on the platform (don't know how useful it IRL). However, it seems the [1] has two issues: 1) it missed terminator entry in the ACPI ID table; 2) it forces polarity to be active high, while I think the XOR approach is better as it's possible (but quite unlikely I believe) that reset pin might be inverted on the PCB level. All in all, I'm fine with any of these patches to be applied with the above mentioned improvements / caveats. Link: https://github.com/linux-surface/kernel/commit/d0f2c2d5a449c2bf69432f90d164183143d8af8d [1] drivers/platform/x86/intel/int3472/discrete.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index b5f6f71bb1dd..0559295dfb27 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -86,6 +86,16 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347 return -EINVAL; } + /* + * The driver of OV7251 expects "enable" pin instead of "reset". + * Remap "reset" to "enable" and update polarity. + */ + if (!strcmp(int3472->sensor_name, "i2c-INT347E:00") && + !strcmp(func, "reset")) { + func = "enable"; + polarity ^= GPIO_ACTIVE_LOW; + } + ret = skl_int3472_fill_gpiod_lookup(&int3472->gpios.table[int3472->n_sensor_gpios], agpio, func, polarity); if (ret) -- 2.43.0.rc1.1336.g36b5255a03ac ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1 1/1] platform/x86: int3472: discrete: Remap "reset" to "enable" for OV7251 2024-08-21 18:40 [PATCH v1 1/1] platform/x86: int3472: discrete: Remap "reset" to "enable" for OV7251 Andy Shevchenko @ 2024-08-26 15:12 ` Hans de Goede 2024-08-26 17:21 ` Andy Shevchenko 0 siblings, 1 reply; 3+ messages in thread From: Hans de Goede @ 2024-08-26 15:12 UTC (permalink / raw) To: Andy Shevchenko, Ilpo Järvinen, platform-driver-x86, linux-kernel Cc: Daniel Scally Hi Andy, On 8/21/24 8:40 PM, Andy Shevchenko wrote: > The driver of OV7251 expects "enable" pin instead of "reset". > Remap "reset" to "enable" and update polarity. > > In particular, the Microsoft Surface Book can't load the camera sensor > driver without this change: > > ov7251 i2c-INT347E:00: supply vdddo not found, using dummy regulator > ov7251 i2c-INT347E:00: supply vddd not found, using dummy regulator > ov7251 i2c-INT347E:00: supply vdda not found, using dummy regulator > ov7251 i2c-INT347E:00: cannot get enable gpio > ov7251 i2c-INT347E:00: probe with driver ov7251 failed with error -2 > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > > Hmm... I have spent some time to achieve this, and then I realised that > linux-surface GitHub project already has something similar [1]. > > The advantage of [1] is that it applies the quirk to all OV7251 sensors > on the platform (don't know how useful it IRL). > > However, it seems the [1] has two issues: > 1) it missed terminator entry in the ACPI ID table; > 2) it forces polarity to be active high, while I think the XOR approach > is better as it's possible (but quite unlikely I believe) that reset pin > might be inverted on the PCB level. > > All in all, I'm fine with any of these patches to be applied with the > above mentioned improvements / caveats. > > Link: https://github.com/linux-surface/kernel/commit/d0f2c2d5a449c2bf69432f90d164183143d8af8d [1] Looking at the datasheet the sensor actually has a reset pin and not an enable pin and the current GPIO mapping in the ov7251 driver / device-tree bindings is wrong. The datasheet describes the single reset control pin as: D6 XSHUTDOWN input "reset (active low with internal pull down resistor)" So as we have done before I would greatly prefer for the sensor driver to get fixed instead of hacking around this in the int3472 code. You could do something similar to what is done in the ov2680.c driver for this, here is the ov2680 gpiod-get code adjusted for the ov7251 case: /* * The device-tree bindings call this pin "enable", but the * datasheet describes the pin as "reset (active low with internal * pull down resistor)". The ACPI tables describing this sensor * on e.g. the Microsoft Surface Book use the ACPI equivalent of * "reset" as pin name, which ACPI glue code then maps to "reset". * Check for a "reset" pin if there is no "enable" pin. */ ov7251->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH); if (IS_ERR(ov7251->enable_gpio)) { ov7251->enable_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); if (!IS_ERR(ov7251->enable_gpio)) gpiod_toggle_active_low(ov7251->enable_gpio); } if (IS_ERR(ov7251->enable_gpio)) { dev_err(dev, "cannot get enable gpio\n"); return PTR_ERR(ov7251->enable_gpio); } (untested) Regards, Hans > > drivers/platform/x86/intel/int3472/discrete.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c > index b5f6f71bb1dd..0559295dfb27 100644 > --- a/drivers/platform/x86/intel/int3472/discrete.c > +++ b/drivers/platform/x86/intel/int3472/discrete.c > @@ -86,6 +86,16 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347 > return -EINVAL; > } > > + /* > + * The driver of OV7251 expects "enable" pin instead of "reset". > + * Remap "reset" to "enable" and update polarity. > + */ > + if (!strcmp(int3472->sensor_name, "i2c-INT347E:00") && > + !strcmp(func, "reset")) { > + func = "enable"; > + polarity ^= GPIO_ACTIVE_LOW; > + } > + > ret = skl_int3472_fill_gpiod_lookup(&int3472->gpios.table[int3472->n_sensor_gpios], > agpio, func, polarity); > if (ret) ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v1 1/1] platform/x86: int3472: discrete: Remap "reset" to "enable" for OV7251 2024-08-26 15:12 ` Hans de Goede @ 2024-08-26 17:21 ` Andy Shevchenko 0 siblings, 0 replies; 3+ messages in thread From: Andy Shevchenko @ 2024-08-26 17:21 UTC (permalink / raw) To: Hans de Goede Cc: Ilpo Järvinen, platform-driver-x86, linux-kernel, Daniel Scally On Mon, Aug 26, 2024 at 05:12:44PM +0200, Hans de Goede wrote: > On 8/21/24 8:40 PM, Andy Shevchenko wrote: > > The driver of OV7251 expects "enable" pin instead of "reset". > > Remap "reset" to "enable" and update polarity. > > > > In particular, the Microsoft Surface Book can't load the camera sensor > > driver without this change: > > > > ov7251 i2c-INT347E:00: supply vdddo not found, using dummy regulator > > ov7251 i2c-INT347E:00: supply vddd not found, using dummy regulator > > ov7251 i2c-INT347E:00: supply vdda not found, using dummy regulator > > ov7251 i2c-INT347E:00: cannot get enable gpio > > ov7251 i2c-INT347E:00: probe with driver ov7251 failed with error -2 > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > > > Hmm... I have spent some time to achieve this, and then I realised that > > linux-surface GitHub project already has something similar [1]. > > > > The advantage of [1] is that it applies the quirk to all OV7251 sensors > > on the platform (don't know how useful it IRL). > > > > However, it seems the [1] has two issues: > > 1) it missed terminator entry in the ACPI ID table; > > 2) it forces polarity to be active high, while I think the XOR approach > > is better as it's possible (but quite unlikely I believe) that reset pin > > might be inverted on the PCB level. > > > > All in all, I'm fine with any of these patches to be applied with the > > above mentioned improvements / caveats. > > > > Link: https://github.com/linux-surface/kernel/commit/d0f2c2d5a449c2bf69432f90d164183143d8af8d [1] > > Looking at the datasheet the sensor actually has a reset pin and not > an enable pin and the current GPIO mapping in the ov7251 driver / > device-tree bindings is wrong. > > The datasheet describes the single reset control pin as: > > D6 XSHUTDOWN input "reset (active low with internal pull down resistor)" > > So as we have done before I would greatly prefer for the sensor driver > to get fixed instead of hacking around this in the int3472 code. > > You could do something similar to what is done in the ov2680.c driver > for this, here is the ov2680 gpiod-get code adjusted for the ov7251 case: > > /* > * The device-tree bindings call this pin "enable", but the > * datasheet describes the pin as "reset (active low with internal > * pull down resistor)". The ACPI tables describing this sensor > * on e.g. the Microsoft Surface Book use the ACPI equivalent of > * "reset" as pin name, which ACPI glue code then maps to "reset". > * Check for a "reset" pin if there is no "enable" pin. > */ > ov7251->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH); > if (IS_ERR(ov7251->enable_gpio)) { > ov7251->enable_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > if (!IS_ERR(ov7251->enable_gpio)) > gpiod_toggle_active_low(ov7251->enable_gpio); > } > if (IS_ERR(ov7251->enable_gpio)) { > dev_err(dev, "cannot get enable gpio\n"); > return PTR_ERR(ov7251->enable_gpio); > } That's a good idea! We can also fix in-kernel DT with that, because now it's quite confusing to have a comment for CAMx_RST_N (note N, sic!). I will test this at some point in the future unless Dan or somebody else beats me up to it. > > drivers/platform/x86/intel/int3472/discrete.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c > > index b5f6f71bb1dd..0559295dfb27 100644 > > --- a/drivers/platform/x86/intel/int3472/discrete.c > > +++ b/drivers/platform/x86/intel/int3472/discrete.c > > @@ -86,6 +86,16 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347 > > return -EINVAL; > > } > > > > + /* > > + * The driver of OV7251 expects "enable" pin instead of "reset". > > + * Remap "reset" to "enable" and update polarity. > > + */ > > + if (!strcmp(int3472->sensor_name, "i2c-INT347E:00") && > > + !strcmp(func, "reset")) { > > + func = "enable"; > > + polarity ^= GPIO_ACTIVE_LOW; > > + } > > + > > ret = skl_int3472_fill_gpiod_lookup(&int3472->gpios.table[int3472->n_sensor_gpios], > > agpio, func, polarity); > > if (ret) -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-08-26 17:21 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-21 18:40 [PATCH v1 1/1] platform/x86: int3472: discrete: Remap "reset" to "enable" for OV7251 Andy Shevchenko 2024-08-26 15:12 ` Hans de Goede 2024-08-26 17:21 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox