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