Re: [PATCH v1 1/1] platform/x86: int3472: discrete: Remap "reset" to "enable" for OV7251

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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