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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zy4_hR9AsDhmK5MK@kekkonen.localdomain>
Date: Fri, 8 Nov 2024 16:42:45 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Hans de Goede <hdegoede@...hat.com>
Subject: Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for
 OV7251

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