[PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Andy Shevchenko]

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(+)

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);
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Hans de Goede]

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

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Sakari Ailus]

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

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Sakari Ailus]

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.

-- 
Regards,

Sakari Ailus

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Hans de Goede]

Hi,

On 8-Nov-24 5:42 PM, Sakari Ailus wrote:
> 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?

Since it is not following the datasheet name for the pin,
it arguably is a DT binding bug

But whatever, the whole discussion about if it is a bug and whose
bug it is is not useful. Since we cannot go back in time and change
the DT binding DT and ACPI are simply going to disagree on the name
and we will need something like this patch.

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

That is on purpose, at least the polarity if the devm_gpiod_get(..., "reset",
...) is inverted from the existing one for "enable" because reset needs
to be inactive/disabled to enable the sensor.

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

This seems to be a pre-existing bug in this driver, which currently
starts driving enable high, enabling the sensor at gpiod_get() time.

Note that fixing this is tricky-ish, if the pin was already high at
gpiod_get() time then changing the gpiod_get() to drive it low
will result in it only being driven low for a very short time since
ov7251_set_power_on() will get called almost immediately after this
and it will drive the pin high again without any delays.

So if the pin was already high then making it low at gpiod_get()
time will result in a very short spike to low, immediately followed
by the pin going high again. This short spike may very well leave
the sensor in a confused state rather then properly resetting it...

OTOH if the pin was already high with the old code where
gpiod_get("enable") requests the pin as high (so it is left high),
then the existing state of the sensor is simply preserved (no reset)
which should be fine for the initial probe which just checks
the id register.

And if the pin was low then it is driven high once and kept high,
so again no glitch / spike. So arguably the old code is fine.

If this is changed then a delay needs to be added to ensure that
the pin is guaranteed to be driven low for some minimum amount
of time.

Regards,

Hans

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Sakari Ailus]

Hi Hans,

On Fri, Nov 08, 2024 at 07:19:05PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 8-Nov-24 5:42 PM, Sakari Ailus wrote:
> > 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?
> 
> Since it is not following the datasheet name for the pin,
> it arguably is a DT binding bug
> 
> But whatever, the whole discussion about if it is a bug and whose
> bug it is is not useful. Since we cannot go back in time and change
> the DT binding DT and ACPI are simply going to disagree on the name
> and we will need something like this patch.
> 
> >> 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.
> 
> That is on purpose, at least the polarity if the devm_gpiod_get(..., "reset",
> ...) is inverted from the existing one for "enable" because reset needs
> to be inactive/disabled to enable the sensor.
> 
> > 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.
> 
> This seems to be a pre-existing bug in this driver, which currently
> starts driving enable high, enabling the sensor at gpiod_get() time.
> 
> Note that fixing this is tricky-ish, if the pin was already high at
> gpiod_get() time then changing the gpiod_get() to drive it low
> will result in it only being driven low for a very short time since
> ov7251_set_power_on() will get called almost immediately after this
> and it will drive the pin high again without any delays.

The question here is not about how long the hard reset is applied, but
whether or not the sensor's power-on sequence is followed. Currently it is
not.

> 
> So if the pin was already high then making it low at gpiod_get()
> time will result in a very short spike to low, immediately followed
> by the pin going high again. This short spike may very well leave
> the sensor in a confused state rather then properly resetting it...
> 
> OTOH if the pin was already high with the old code where
> gpiod_get("enable") requests the pin as high (so it is left high),
> then the existing state of the sensor is simply preserved (no reset)
> which should be fine for the initial probe which just checks
> the id register.
> 
> And if the pin was low then it is driven high once and kept high,
> so again no glitch / spike. So arguably the old code is fine.

I'm not sure how do you reach that conclusion. Because it happens to work,
presumably at least somewhere? The code appears to be part of the 

> 
> If this is changed then a delay needs to be added to ensure that
> the pin is guaranteed to be driven low for some minimum amount
> of time.

-- 
Regards,

Sakari Ailus

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Hans de Goede]

Hi,

On 11-Nov-24 8:55 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Nov 08, 2024 at 07:19:05PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 8-Nov-24 5:42 PM, Sakari Ailus wrote:
>>> 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?
>>
>> Since it is not following the datasheet name for the pin,
>> it arguably is a DT binding bug
>>
>> But whatever, the whole discussion about if it is a bug and whose
>> bug it is is not useful. Since we cannot go back in time and change
>> the DT binding DT and ACPI are simply going to disagree on the name
>> and we will need something like this patch.
>>
>>>> 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.
>>
>> That is on purpose, at least the polarity if the devm_gpiod_get(..., "reset",
>> ...) is inverted from the existing one for "enable" because reset needs
>> to be inactive/disabled to enable the sensor.
>>
>>> 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.
>>
>> This seems to be a pre-existing bug in this driver, which currently
>> starts driving enable high, enabling the sensor at gpiod_get() time.
>>
>> Note that fixing this is tricky-ish, if the pin was already high at
>> gpiod_get() time then changing the gpiod_get() to drive it low
>> will result in it only being driven low for a very short time since
>> ov7251_set_power_on() will get called almost immediately after this
>> and it will drive the pin high again without any delays.
> 
> The question here is not about how long the hard reset is applied, but
> whether or not the sensor's power-on sequence is followed. Currently it is
> not.

Right / agreed. The 2 points which I am trying to make are:

1. This is a pre-existing problem unrelated to this patch.

So this should be fixed in a separate patch.

2. That separate patch should put a delay after requesting the GPIO
to enforce that it is (logically) low (for "enable") for a minimum
amount of time.

Regards,

Hans

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Andy Shevchenko]

On Mon, Nov 11, 2024 at 10:46:32AM +0100, Hans de Goede wrote:
> On 11-Nov-24 8:55 AM, Sakari Ailus wrote:
> > On Fri, Nov 08, 2024 at 07:19:05PM +0100, Hans de Goede wrote:
> >> On 8-Nov-24 5:42 PM, Sakari Ailus wrote:
> >>> 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?
> >>
> >> Since it is not following the datasheet name for the pin,
> >> it arguably is a DT binding bug
> >>
> >> But whatever, the whole discussion about if it is a bug and whose
> >> bug it is is not useful. Since we cannot go back in time and change
> >> the DT binding DT and ACPI are simply going to disagree on the name
> >> and we will need something like this patch.
> >>
> >>>> 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.
> >>
> >> That is on purpose, at least the polarity if the devm_gpiod_get(..., "reset",
> >> ...) is inverted from the existing one for "enable" because reset needs
> >> to be inactive/disabled to enable the sensor.
> >>
> >>> 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.
> >>
> >> This seems to be a pre-existing bug in this driver, which currently
> >> starts driving enable high, enabling the sensor at gpiod_get() time.
> >>
> >> Note that fixing this is tricky-ish, if the pin was already high at
> >> gpiod_get() time then changing the gpiod_get() to drive it low
> >> will result in it only being driven low for a very short time since
> >> ov7251_set_power_on() will get called almost immediately after this
> >> and it will drive the pin high again without any delays.
> > 
> > The question here is not about how long the hard reset is applied, but
> > whether or not the sensor's power-on sequence is followed. Currently it is
> > not.
> 
> Right / agreed. The 2 points which I am trying to make are:
> 
> 1. This is a pre-existing problem unrelated to this patch.
> 
> So this should be fixed in a separate patch.
> 
> 2. That separate patch should put a delay after requesting the GPIO
> to enforce that it is (logically) low (for "enable") for a minimum
> amount of time.

Sakari, what's the status on this, please?
We have non-working camera just because of this small patch is absent.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Sakari Ailus]

Hi Andy,

On Thu, Jan 16, 2025 at 04:48:12PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 11, 2024 at 10:46:32AM +0100, Hans de Goede wrote:
> > On 11-Nov-24 8:55 AM, Sakari Ailus wrote:
> > > On Fri, Nov 08, 2024 at 07:19:05PM +0100, Hans de Goede wrote:
> > >> On 8-Nov-24 5:42 PM, Sakari Ailus wrote:
> > >>> 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?
> > >>
> > >> Since it is not following the datasheet name for the pin,
> > >> it arguably is a DT binding bug
> > >>
> > >> But whatever, the whole discussion about if it is a bug and whose
> > >> bug it is is not useful. Since we cannot go back in time and change
> > >> the DT binding DT and ACPI are simply going to disagree on the name
> > >> and we will need something like this patch.
> > >>
> > >>>> 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.
> > >>
> > >> That is on purpose, at least the polarity if the devm_gpiod_get(..., "reset",
> > >> ...) is inverted from the existing one for "enable" because reset needs
> > >> to be inactive/disabled to enable the sensor.
> > >>
> > >>> 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.
> > >>
> > >> This seems to be a pre-existing bug in this driver, which currently
> > >> starts driving enable high, enabling the sensor at gpiod_get() time.
> > >>
> > >> Note that fixing this is tricky-ish, if the pin was already high at
> > >> gpiod_get() time then changing the gpiod_get() to drive it low
> > >> will result in it only being driven low for a very short time since
> > >> ov7251_set_power_on() will get called almost immediately after this
> > >> and it will drive the pin high again without any delays.
> > > 
> > > The question here is not about how long the hard reset is applied, but
> > > whether or not the sensor's power-on sequence is followed. Currently it is
> > > not.
> > 
> > Right / agreed. The 2 points which I am trying to make are:
> > 
> > 1. This is a pre-existing problem unrelated to this patch.
> > 
> > So this should be fixed in a separate patch.
> > 
> > 2. That separate patch should put a delay after requesting the GPIO
> > to enforce that it is (logically) low (for "enable") for a minimum
> > amount of time.
> 
> Sakari, what's the status on this, please?
> We have non-working camera just because of this small patch is absent.

Thanks for the ping.

I took a closer look at the problem, indeed the GPIO name comes from the
int3472 driver and it's much better to work around this there than to ram
Windows ACPI table specifics to sensor drivers. I'll post a patch for that
shortly. Testing would be appreciated as the Surface Go 2 doesn't have a
GPIO connected to this sensor.

Maybe backporting would be reasonable even though this isn't a fix as such?

-- 
Regards,

Sakari Ailus

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Hans de Goede]

+Cc platform-driver-x86@vger.kernel.org

Hi Sakari,

On 20-Jan-25 10:39 AM, Sakari Ailus wrote:
> Hi Andy,
> 
> On Thu, Jan 16, 2025 at 04:48:12PM +0200, Andy Shevchenko wrote:
>> On Mon, Nov 11, 2024 at 10:46:32AM +0100, Hans de Goede wrote:
>>> On 11-Nov-24 8:55 AM, Sakari Ailus wrote:
>>>> On Fri, Nov 08, 2024 at 07:19:05PM +0100, Hans de Goede wrote:
>>>>> On 8-Nov-24 5:42 PM, Sakari Ailus wrote:
>>>>>> 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?
>>>>>
>>>>> Since it is not following the datasheet name for the pin,
>>>>> it arguably is a DT binding bug
>>>>>
>>>>> But whatever, the whole discussion about if it is a bug and whose
>>>>> bug it is is not useful. Since we cannot go back in time and change
>>>>> the DT binding DT and ACPI are simply going to disagree on the name
>>>>> and we will need something like this patch.
>>>>>
>>>>>>> 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.
>>>>>
>>>>> That is on purpose, at least the polarity if the devm_gpiod_get(..., "reset",
>>>>> ...) is inverted from the existing one for "enable" because reset needs
>>>>> to be inactive/disabled to enable the sensor.
>>>>>
>>>>>> 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.
>>>>>
>>>>> This seems to be a pre-existing bug in this driver, which currently
>>>>> starts driving enable high, enabling the sensor at gpiod_get() time.
>>>>>
>>>>> Note that fixing this is tricky-ish, if the pin was already high at
>>>>> gpiod_get() time then changing the gpiod_get() to drive it low
>>>>> will result in it only being driven low for a very short time since
>>>>> ov7251_set_power_on() will get called almost immediately after this
>>>>> and it will drive the pin high again without any delays.
>>>>
>>>> The question here is not about how long the hard reset is applied, but
>>>> whether or not the sensor's power-on sequence is followed. Currently it is
>>>> not.
>>>
>>> Right / agreed. The 2 points which I am trying to make are:
>>>
>>> 1. This is a pre-existing problem unrelated to this patch.
>>>
>>> So this should be fixed in a separate patch.
>>>
>>> 2. That separate patch should put a delay after requesting the GPIO
>>> to enforce that it is (logically) low (for "enable") for a minimum
>>> amount of time.
>>
>> Sakari, what's the status on this, please?
>> We have non-working camera just because of this small patch is absent.
> 
> Thanks for the ping.
> 
> I took a closer look at the problem, indeed the GPIO name comes from the
> int3472 driver and it's much better to work around this there than to ram
> Windows ACPI table specifics to sensor drivers. I'll post a patch for that
> shortly. Testing would be appreciated as the Surface Go 2 doesn't have a
> GPIO connected to this sensor.

Actually Andy wrote this patch after first writing a similar quirk patch
as yours from:

https://lore.kernel.org/platform-driver-x86/1cf93f61-9285-f2fe-fb92-8fb8a3f44201@linux.intel.com/T/#t

because I asked him to fix this in the sensor driver instead.

The problem is that we cannot fix this in the INT3472 driver without
that becoming one big mess of driver specific quirks.

The Windows code seems to have separate power-ctrl/sequence drivers
for the INT3472 device vs the rest of the sensor driver and this
power-sequence driver happily consumes whatever GPIOs the INT3472
device provides independent of the sensor.

So e.g. on some designs we are going to see a reset pin and on others
a powerdown pin.

But in this specific case things are actually more simple,
the datasheet describes the pin as:

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

So the devicetree binding calling it "enable" is just wrong and in
this case it is actually the DT binding which is buggy (there is no
"enable" pin anywhere in the datasheet) and not the ACPI tables.

Also since you match on the sensor-type for the remapping this clearly
is a per sensor thing and per sensor handling should be in the sensor
drivers, which gives us the per sensor mapping for free.

Yes needing to handle this in the sensor driver makes the sensor
driver slightly less pretty. But dealing with platform specifics
in drivers is done all over the kernel and I don't see why camera
sensor drivers are so special that they get to be excluded from
sometimes needing to deal with platform specifics.

Regards,

Hans

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Laurent Pinchart]

On Mon, Jan 20, 2025 at 02:30:46PM +0100, Hans de Goede wrote:
> +Cc platform-driver-x86@vger.kernel.org
> 
> Hi Sakari,
> 
> On 20-Jan-25 10:39 AM, Sakari Ailus wrote:
> > Hi Andy,
> > 
> > On Thu, Jan 16, 2025 at 04:48:12PM +0200, Andy Shevchenko wrote:
> >> On Mon, Nov 11, 2024 at 10:46:32AM +0100, Hans de Goede wrote:
> >>> On 11-Nov-24 8:55 AM, Sakari Ailus wrote:
> >>>> On Fri, Nov 08, 2024 at 07:19:05PM +0100, Hans de Goede wrote:
> >>>>> On 8-Nov-24 5:42 PM, Sakari Ailus wrote:
> >>>>>> 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?
> >>>>>
> >>>>> Since it is not following the datasheet name for the pin,
> >>>>> it arguably is a DT binding bug
> >>>>>
> >>>>> But whatever, the whole discussion about if it is a bug and whose
> >>>>> bug it is is not useful. Since we cannot go back in time and change
> >>>>> the DT binding DT and ACPI are simply going to disagree on the name
> >>>>> and we will need something like this patch.
> >>>>>
> >>>>>>> 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.
> >>>>>
> >>>>> That is on purpose, at least the polarity if the devm_gpiod_get(..., "reset",
> >>>>> ...) is inverted from the existing one for "enable" because reset needs
> >>>>> to be inactive/disabled to enable the sensor.
> >>>>>
> >>>>>> 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.
> >>>>>
> >>>>> This seems to be a pre-existing bug in this driver, which currently
> >>>>> starts driving enable high, enabling the sensor at gpiod_get() time.
> >>>>>
> >>>>> Note that fixing this is tricky-ish, if the pin was already high at
> >>>>> gpiod_get() time then changing the gpiod_get() to drive it low
> >>>>> will result in it only being driven low for a very short time since
> >>>>> ov7251_set_power_on() will get called almost immediately after this
> >>>>> and it will drive the pin high again without any delays.
> >>>>
> >>>> The question here is not about how long the hard reset is applied, but
> >>>> whether or not the sensor's power-on sequence is followed. Currently it is
> >>>> not.
> >>>
> >>> Right / agreed. The 2 points which I am trying to make are:
> >>>
> >>> 1. This is a pre-existing problem unrelated to this patch.
> >>>
> >>> So this should be fixed in a separate patch.
> >>>
> >>> 2. That separate patch should put a delay after requesting the GPIO
> >>> to enforce that it is (logically) low (for "enable") for a minimum
> >>> amount of time.
> >>
> >> Sakari, what's the status on this, please?
> >> We have non-working camera just because of this small patch is absent.
> > 
> > Thanks for the ping.
> > 
> > I took a closer look at the problem, indeed the GPIO name comes from the
> > int3472 driver and it's much better to work around this there than to ram
> > Windows ACPI table specifics to sensor drivers. I'll post a patch for that
> > shortly. Testing would be appreciated as the Surface Go 2 doesn't have a
> > GPIO connected to this sensor.
> 
> Actually Andy wrote this patch after first writing a similar quirk patch
> as yours from:
> 
> https://lore.kernel.org/platform-driver-x86/1cf93f61-9285-f2fe-fb92-8fb8a3f44201@linux.intel.com/T/#t
> 
> because I asked him to fix this in the sensor driver instead.
> 
> The problem is that we cannot fix this in the INT3472 driver without
> that becoming one big mess of driver specific quirks.
> 
> The Windows code seems to have separate power-ctrl/sequence drivers
> for the INT3472 device vs the rest of the sensor driver and this
> power-sequence driver happily consumes whatever GPIOs the INT3472
> device provides independent of the sensor.
> 
> So e.g. on some designs we are going to see a reset pin and on others
> a powerdown pin.
> 
> But in this specific case things are actually more simple,
> the datasheet describes the pin as:
> 
> XSHUTDOWN input reset (active low with internal pull down resistor)
> 
> So the devicetree binding calling it "enable" is just wrong and in
> this case it is actually the DT binding which is buggy (there is no
> "enable" pin anywhere in the datasheet) and not the ACPI tables.

There has never been a requirement for the GPIO in DT to match the pin
name from the datasheet. It's actually quite the contrary, DT
maintainers request GPIO names to be standardized. Using
"xshutdown-gpios" wouldn't be appreciated, instead DT bindings are
encouraged to use standard names such as "reset", "enable" and
"powerdown". As the pin is called "XSHUTDOWN", "enable" is not
necessarily a bad match. "reset" could have been picked too, but that
wasn't the case and we can't change that.

The core of the issue here is that we have two firmware representations,
DT and ACPI, and no coordination between the two. I don't expect that to
change, and until Windows OEMs collaborate to standardize ACPI bindings,
I'll consider this as a Windows-specific hack that I don't want to see
being pushed to indidividual sensor drivers. This would be better
handled in my opinion in the INT3472 driver.

> Also since you match on the sensor-type for the remapping this clearly
> is a per sensor thing and per sensor handling should be in the sensor
> drivers, which gives us the per sensor mapping for free.
> 
> Yes needing to handle this in the sensor driver makes the sensor
> driver slightly less pretty. But dealing with platform specifics
> in drivers is done all over the kernel and I don't see why camera
> sensor drivers are so special that they get to be excluded from
> sometimes needing to deal with platform specifics.

The whole point of having standardized firmware descriptions is to avoid
board-specific code in drivers. INT3472 is meant to handle board
specificities.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Hans de Goede]

Hi Laurent,

On 20-Jan-25 5:05 PM, Laurent Pinchart wrote:
> On Mon, Jan 20, 2025 at 02:30:46PM +0100, Hans de Goede wrote:
>> +Cc platform-driver-x86@vger.kernel.org
>>
>> Hi Sakari,
>>
>> On 20-Jan-25 10:39 AM, Sakari Ailus wrote:
>>> Hi Andy,
>>>
>>> On Thu, Jan 16, 2025 at 04:48:12PM +0200, Andy Shevchenko wrote:
>>>> On Mon, Nov 11, 2024 at 10:46:32AM +0100, Hans de Goede wrote:
>>>>> On 11-Nov-24 8:55 AM, Sakari Ailus wrote:
>>>>>> On Fri, Nov 08, 2024 at 07:19:05PM +0100, Hans de Goede wrote:
>>>>>>> On 8-Nov-24 5:42 PM, Sakari Ailus wrote:
>>>>>>>> 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?
>>>>>>>
>>>>>>> Since it is not following the datasheet name for the pin,
>>>>>>> it arguably is a DT binding bug
>>>>>>>
>>>>>>> But whatever, the whole discussion about if it is a bug and whose
>>>>>>> bug it is is not useful. Since we cannot go back in time and change
>>>>>>> the DT binding DT and ACPI are simply going to disagree on the name
>>>>>>> and we will need something like this patch.
>>>>>>>
>>>>>>>>> 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.
>>>>>>>
>>>>>>> That is on purpose, at least the polarity if the devm_gpiod_get(..., "reset",
>>>>>>> ...) is inverted from the existing one for "enable" because reset needs
>>>>>>> to be inactive/disabled to enable the sensor.
>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> This seems to be a pre-existing bug in this driver, which currently
>>>>>>> starts driving enable high, enabling the sensor at gpiod_get() time.
>>>>>>>
>>>>>>> Note that fixing this is tricky-ish, if the pin was already high at
>>>>>>> gpiod_get() time then changing the gpiod_get() to drive it low
>>>>>>> will result in it only being driven low for a very short time since
>>>>>>> ov7251_set_power_on() will get called almost immediately after this
>>>>>>> and it will drive the pin high again without any delays.
>>>>>>
>>>>>> The question here is not about how long the hard reset is applied, but
>>>>>> whether or not the sensor's power-on sequence is followed. Currently it is
>>>>>> not.
>>>>>
>>>>> Right / agreed. The 2 points which I am trying to make are:
>>>>>
>>>>> 1. This is a pre-existing problem unrelated to this patch.
>>>>>
>>>>> So this should be fixed in a separate patch.
>>>>>
>>>>> 2. That separate patch should put a delay after requesting the GPIO
>>>>> to enforce that it is (logically) low (for "enable") for a minimum
>>>>> amount of time.
>>>>
>>>> Sakari, what's the status on this, please?
>>>> We have non-working camera just because of this small patch is absent.
>>>
>>> Thanks for the ping.
>>>
>>> I took a closer look at the problem, indeed the GPIO name comes from the
>>> int3472 driver and it's much better to work around this there than to ram
>>> Windows ACPI table specifics to sensor drivers. I'll post a patch for that
>>> shortly. Testing would be appreciated as the Surface Go 2 doesn't have a
>>> GPIO connected to this sensor.
>>
>> Actually Andy wrote this patch after first writing a similar quirk patch
>> as yours from:
>>
>> https://lore.kernel.org/platform-driver-x86/1cf93f61-9285-f2fe-fb92-8fb8a3f44201@linux.intel.com/T/#t
>>
>> because I asked him to fix this in the sensor driver instead.
>>
>> The problem is that we cannot fix this in the INT3472 driver without
>> that becoming one big mess of driver specific quirks.
>>
>> The Windows code seems to have separate power-ctrl/sequence drivers
>> for the INT3472 device vs the rest of the sensor driver and this
>> power-sequence driver happily consumes whatever GPIOs the INT3472
>> device provides independent of the sensor.
>>
>> So e.g. on some designs we are going to see a reset pin and on others
>> a powerdown pin.
>>
>> But in this specific case things are actually more simple,
>> the datasheet describes the pin as:
>>
>> XSHUTDOWN input reset (active low with internal pull down resistor)
>>
>> So the devicetree binding calling it "enable" is just wrong and in
>> this case it is actually the DT binding which is buggy (there is no
>> "enable" pin anywhere in the datasheet) and not the ACPI tables.
> 
> There has never been a requirement for the GPIO in DT to match the pin
> name from the datasheet. It's actually quite the contrary, DT
> maintainers request GPIO names to be standardized. Using
> "xshutdown-gpios" wouldn't be appreciated, instead DT bindings are
> encouraged to use standard names such as "reset", "enable" and
> "powerdown". As the pin is called "XSHUTDOWN", "enable" is not
> necessarily a bad match. "reset" could have been picked too, but that
> wasn't the case and we can't change that.
> 
> The core of the issue here is that we have two firmware representations,
> DT and ACPI, and no coordination between the two. I don't expect that to
> change, and until Windows OEMs collaborate to standardize ACPI bindings,
> I'll consider this as a Windows-specific hack that I don't want to see
> being pushed to indidividual sensor drivers. This would be better
> handled in my opinion in the INT3472 driver.
> 
>> Also since you match on the sensor-type for the remapping this clearly
>> is a per sensor thing and per sensor handling should be in the sensor
>> drivers, which gives us the per sensor mapping for free.
>>
>> Yes needing to handle this in the sensor driver makes the sensor
>> driver slightly less pretty. But dealing with platform specifics
>> in drivers is done all over the kernel and I don't see why camera
>> sensor drivers are so special that they get to be excluded from
>> sometimes needing to deal with platform specifics.
> 
> The whole point of having standardized firmware descriptions is to avoid
> board-specific code in drivers. INT3472 is meant to handle board
> specificities.

The problem is that some issues are relatively easy to fix in a generic
manner in the driver's while they are quite hard to fix, requiring
per board/laptop-model enablement using DMI quirks in the platform
glue code.

A good example of this is this case from further down the thread:

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

Dealing with this specific case at the glue code level requires per
laptop-model knowledge to map the right one of the 2 pins to "reset"
which means needing to manually add a quirk every time we learn
about a new model and perhaps more importantly this means things
not workin OOTB until we add such a quirk.

When working on the UVC driver you sometimes ask / wonder what
does Windows do here?   In this case it is pretty clear that
Window simply sets all GPIOs listed in the INT3472 ACPI resources/
the DSM call describing the GPIOs to their "on" state when
the sensor needs to be turned on.

Since we don't have the sensor-power-sequencing part split of
from the rest of the sensor driver code as Windows appears to
have, we need to at least be able to mimick Windows to get
things to "just work" which in some cases means driving 2 GPIOs
even if the sensor really only has one enable/reset input.

I'm most interested in how we can solve the mentioned mt9m114
example here, since that is a case where per model DMI
quirks will be necessary for a glue level solution, where as
a sensor-driver level solution will solve this once for all
models with that sensor.

Also drivers/media/i2c so far is quite unique in trying to
completely avoid any ACPI platform specific handling. Almost
all other driver subsystems I have worked in have various
platform specific code-paths, sometimes for device-tree
specific things and sometimes for ACPI specific things, e.g. :

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

but that is just one example out of many. Although I understand
that it is desirable to keep platform specific code out of
the sensor drivers we also need to be pragmatic here IMHO.

Regards,

Hans

Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Andy Shevchenko]

On Mon, Jan 20, 2025 at 06:05:05PM +0200, Laurent Pinchart wrote:
> On Mon, Jan 20, 2025 at 02:30:46PM +0100, Hans de Goede wrote:
> > On 20-Jan-25 10:39 AM, Sakari Ailus wrote:
> > > On Thu, Jan 16, 2025 at 04:48:12PM +0200, Andy Shevchenko wrote:
> > >> On Mon, Nov 11, 2024 at 10:46:32AM +0100, Hans de Goede wrote:
> > >>> On 11-Nov-24 8:55 AM, Sakari Ailus wrote:
> > >>>> On Fri, Nov 08, 2024 at 07:19:05PM +0100, Hans de Goede wrote:
> > >>>>> On 8-Nov-24 5:42 PM, Sakari Ailus wrote:
> > >>>>>> 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?
> > >>>>>
> > >>>>> Since it is not following the datasheet name for the pin,
> > >>>>> it arguably is a DT binding bug
> > >>>>>
> > >>>>> But whatever, the whole discussion about if it is a bug and whose
> > >>>>> bug it is is not useful. Since we cannot go back in time and change
> > >>>>> the DT binding DT and ACPI are simply going to disagree on the name
> > >>>>> and we will need something like this patch.
> > >>>>>
> > >>>>>>> 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.
> > >>>>>
> > >>>>> That is on purpose, at least the polarity if the devm_gpiod_get(..., "reset",
> > >>>>> ...) is inverted from the existing one for "enable" because reset needs
> > >>>>> to be inactive/disabled to enable the sensor.
> > >>>>>
> > >>>>>> 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.
> > >>>>>
> > >>>>> This seems to be a pre-existing bug in this driver, which currently
> > >>>>> starts driving enable high, enabling the sensor at gpiod_get() time.
> > >>>>>
> > >>>>> Note that fixing this is tricky-ish, if the pin was already high at
> > >>>>> gpiod_get() time then changing the gpiod_get() to drive it low
> > >>>>> will result in it only being driven low for a very short time since
> > >>>>> ov7251_set_power_on() will get called almost immediately after this
> > >>>>> and it will drive the pin high again without any delays.
> > >>>>
> > >>>> The question here is not about how long the hard reset is applied, but
> > >>>> whether or not the sensor's power-on sequence is followed. Currently it is
> > >>>> not.
> > >>>
> > >>> Right / agreed. The 2 points which I am trying to make are:
> > >>>
> > >>> 1. This is a pre-existing problem unrelated to this patch.
> > >>>
> > >>> So this should be fixed in a separate patch.
> > >>>
> > >>> 2. That separate patch should put a delay after requesting the GPIO
> > >>> to enforce that it is (logically) low (for "enable") for a minimum
> > >>> amount of time.
> > >>
> > >> Sakari, what's the status on this, please?
> > >> We have non-working camera just because of this small patch is absent.
> > > 
> > > Thanks for the ping.
> > > 
> > > I took a closer look at the problem, indeed the GPIO name comes from the
> > > int3472 driver and it's much better to work around this there than to ram
> > > Windows ACPI table specifics to sensor drivers. I'll post a patch for that
> > > shortly. Testing would be appreciated as the Surface Go 2 doesn't have a
> > > GPIO connected to this sensor.
> > 
> > Actually Andy wrote this patch after first writing a similar quirk patch
> > as yours from:
> > 
> > https://lore.kernel.org/platform-driver-x86/1cf93f61-9285-f2fe-fb92-8fb8a3f44201@linux.intel.com/T/#t
> > 
> > because I asked him to fix this in the sensor driver instead.
> > 
> > The problem is that we cannot fix this in the INT3472 driver without
> > that becoming one big mess of driver specific quirks.
> > 
> > The Windows code seems to have separate power-ctrl/sequence drivers
> > for the INT3472 device vs the rest of the sensor driver and this
> > power-sequence driver happily consumes whatever GPIOs the INT3472
> > device provides independent of the sensor.
> > 
> > So e.g. on some designs we are going to see a reset pin and on others
> > a powerdown pin.
> > 
> > But in this specific case things are actually more simple,
> > the datasheet describes the pin as:
> > 
> > XSHUTDOWN input reset (active low with internal pull down resistor)
> > 
> > So the devicetree binding calling it "enable" is just wrong and in
> > this case it is actually the DT binding which is buggy (there is no
> > "enable" pin anywhere in the datasheet) and not the ACPI tables.
> 
> There has never been a requirement for the GPIO in DT to match the pin
> name from the datasheet. It's actually quite the contrary, DT
> maintainers request GPIO names to be standardized. Using
> "xshutdown-gpios" wouldn't be appreciated, instead DT bindings are
> encouraged to use standard names such as "reset", "enable" and
> "powerdown". As the pin is called "XSHUTDOWN", "enable" is not
> necessarily a bad match. "reset" could have been picked too, but that
> wasn't the case and we can't change that.

This is very strange. I always thought that DT is about describing _hardware_
and not some abstractions like Linux ones. The HW is pretty much clear that
the pin is "reset" by semantics (if you want standardization), and not
"enable".

> The core of the issue here is that we have two firmware representations,
> DT and ACPI, and no coordination between the two. I don't expect that to
> change, and until Windows OEMs collaborate to standardize ACPI bindings,
> I'll consider this as a Windows-specific hack that I don't want to see
> being pushed to indidividual sensor drivers. This would be better
> handled in my opinion in the INT3472 driver.

Yeah and to me it's clearly the bug in DT schema for this pin. It had to be
"reset" from day 1 in accordance with datasheet.

> > Also since you match on the sensor-type for the remapping this clearly
> > is a per sensor thing and per sensor handling should be in the sensor
> > drivers, which gives us the per sensor mapping for free.
> > 
> > Yes needing to handle this in the sensor driver makes the sensor
> > driver slightly less pretty. But dealing with platform specifics
> > in drivers is done all over the kernel and I don't see why camera
> > sensor drivers are so special that they get to be excluded from
> > sometimes needing to deal with platform specifics.
> 
> The whole point of having standardized firmware descriptions is to avoid
> board-specific code in drivers. INT3472 is meant to handle board
> specificities.

Yes, but world is not ideal and people made mistakes...

-- 
With Best Regards,
Andy Shevchenko