| Message ID | 20241108145024.1490536-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] media: ov7251: Remap "reset" to "enable" for OV7251 | expand |
Hi, On 8-Nov-24 3:50 PM, Andy Shevchenko wrote: > The driver of OmniVision OV7251 expects "enable" pin instead of "reset". > Remap "reset" to "enable" and update polarity. > > In particular, the Linux kernel can't load the camera sensor > driver on Microsoft Surface Book 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 > > Suggested-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/media/i2c/ov7251.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c > index 30f61e04ecaf..7b35add1e0ed 100644 > --- a/drivers/media/i2c/ov7251.c > +++ b/drivers/media/i2c/ov7251.c > @@ -1696,7 +1696,21 @@ static int ov7251_probe(struct i2c_client *client) > return PTR_ERR(ov7251->analog_regulator); > } > > + /* > + * 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) && > + PTR_ERR(ov7251->enable_gpio) != -EPROBE_DEFER) { > + 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);
Hi Andy, Thanks for the patch. On Fri, Nov 08, 2024 at 04:50:24PM +0200, Andy Shevchenko wrote: > The driver of OmniVision OV7251 expects "enable" pin instead of "reset". > Remap "reset" to "enable" and update polarity. > > In particular, the Linux kernel can't load the camera sensor > driver on Microsoft Surface Book 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 > > Suggested-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Should this be cc'd to stable? I guess it's not exactly a fix in the driver but a BIOS bug, but it can be worked around in the driver. :-) > --- > drivers/media/i2c/ov7251.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c > index 30f61e04ecaf..7b35add1e0ed 100644 > --- a/drivers/media/i2c/ov7251.c > +++ b/drivers/media/i2c/ov7251.c > @@ -1696,7 +1696,21 @@ static int ov7251_probe(struct i2c_client *client) > return PTR_ERR(ov7251->analog_regulator); > } > > + /* > + * 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 It's the functionality that matters albeit I guess this is somewhat a matter of taste: a similar pin was named "reset" for MIPI CCS. > + * 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) && > + PTR_ERR(ov7251->enable_gpio) != -EPROBE_DEFER) { > + ov7251->enable_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); This looks like it'd benefit from a line wrap. I can do that if there's no need for v2 otherwise. > + 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);
On Fri, Nov 08, 2024 at 04:06:39PM +0000, Sakari Ailus wrote: > On Fri, Nov 08, 2024 at 04:50:24PM +0200, Andy Shevchenko wrote: > > The driver of OmniVision OV7251 expects "enable" pin instead of "reset". > > Remap "reset" to "enable" and update polarity. > > > > In particular, the Linux kernel can't load the camera sensor > > driver on Microsoft Surface Book 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 ... > Should this be cc'd to stable? I guess it's not exactly a fix in the driver > but a BIOS bug, but it can be worked around in the driver. :-) It's everything, but a BIOS bug, it's DT bug and whoever first introduced that GPIO in the driver. Even in the DT present in kernel the pin was referred as CAM_RST_N, which is exactly how this patch names it. OTOH it's a fix to the driver that never worked for ACPI case, so there never was a regression to fix.
Hi Andy, On Fri, Nov 08, 2024 at 06:28:05PM +0200, Andy Shevchenko wrote: > On Fri, Nov 08, 2024 at 04:06:39PM +0000, Sakari Ailus wrote: > > On Fri, Nov 08, 2024 at 04:50:24PM +0200, Andy Shevchenko wrote: > > > The driver of OmniVision OV7251 expects "enable" pin instead of "reset". > > > Remap "reset" to "enable" and update polarity. > > > > > > In particular, the Linux kernel can't load the camera sensor > > > driver on Microsoft Surface Book 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 > > ... > > > Should this be cc'd to stable? I guess it's not exactly a fix in the driver > > but a BIOS bug, but it can be worked around in the driver. :-) > > It's everything, but a BIOS bug, it's DT bug and whoever first introduced that > GPIO in the driver. Even in the DT present in kernel the pin was referred as How is that a DT (binding?) bug? > CAM_RST_N, which is exactly how this patch names it. > > OTOH it's a fix to the driver that never worked for ACPI case, so there never > was a regression to fix. It probably worked just fine, just not with that Surface Book. The polarity of the enable gpio appears to be set wrong in devm_gpiod_get() call. I can post a patch but cannot test it. Similarly, you should actually set the flags to GPIOD_OUT_HIGH as reset should be enabled here -- it's disabled only in power_on() as part of the power-on sequence.
diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c index 30f61e04ecaf..7b35add1e0ed 100644 --- a/drivers/media/i2c/ov7251.c +++ b/drivers/media/i2c/ov7251.c @@ -1696,7 +1696,21 @@ static int ov7251_probe(struct i2c_client *client) return PTR_ERR(ov7251->analog_regulator); } + /* + * 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) && + PTR_ERR(ov7251->enable_gpio) != -EPROBE_DEFER) { + 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);
The driver of OmniVision OV7251 expects "enable" pin instead of "reset". Remap "reset" to "enable" and update polarity. In particular, the Linux kernel can't load the camera sensor driver on Microsoft Surface Book 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 Suggested-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/media/i2c/ov7251.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)