linux-kernel - Re: [PATCH v3 2/2] media: i2c: Pinefeat cef168 lens control board driver

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] [day] [month] [year] [list]

Message-ID: <d2ka3glpjiisjs7ydx7knzzfb2dzi2lyc2r7d4hppqor33xate@2evtuolz6wah>
Date: Wed, 20 Aug 2025 14:56:38 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Aliaksandr Smirnou <support@...efeat.co.uk>, 
	Hans Verkuil <hverkuil@...all.nl>
Cc: jacopo.mondi@...asonboard.com, conor+dt@...nel.org, 
	devicetree@...r.kernel.org, krzk+dt@...nel.org, linux-kernel@...r.kernel.org, 
	linux-media@...r.kernel.org, mchehab@...nel.org, robh@...nel.org
Subject: Re: [PATCH v3 2/2] media: i2c: Pinefeat cef168 lens control board
 driver

Hello
  +cc Hans

On Tue, Aug 19, 2025 at 10:32:34PM +0100, Aliaksandr Smirnou wrote:
> Hi Jacopo,
>
> Thank you for the review. Your remarks are very helpful. While I'll apply
> most of them, could you clarify the one regarding pm_runtime_ for me?
>
> On Tue, 19 Aug 2025 15:47:54 +0200, Jacopo Mondi wrote:
> > > +#include <linux/crc8.h>
> >
> > Do you need to "select CRC8" in Kconfig then ?
>
> Yes, I'll include it.
>
> > > +#include "cef168.h"
> >
> > Why an header file ?
>
> Ok, I'll remove the header moving everying in the .c file.
>
> > > +	for (retry = 0; retry < 3; retry++) {
> >
> > This seems a bit random, why do you need to retry three times ?
>
> The driver retries writes to work around an issue in the Raspberry
> Pi's I2C hardware, where the BCM2835 mishandles clock stretching.
> When the slave stretches the clock, the Pi can misread the SCL line
> or sample data too early, making it think the write failed. To
> improve reliability, the kernel driver automatically retries the
> write, effectively compensating for the hardware's timing bug.
>

mmm, usually a mainline driver is not the right place for introducing in
workarounds for a specific host/soc, as the assumption is that the
driver should work on all platforms equally.

Your case is certainly different as this product is pi-specific...

With a comment I wouldn't mind too much. Maybe others do :)

> > > +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(data) &&
> > > +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(focus_range) &&
> > > +	    ctrl->id != CEF168_V4L2_CID_CUSTOM(lens_id))
> > > +		return -EINVAL;
> >
> > If you mark them WRITE_ONLY wouldn't the core take care of this ?
>
> These controls are read-only. The data they return depens on the lens.
>

Sorry, I wasn't clear.

If you mark as WO the controls you don't accept here, will the core
handle this for you ?

> > > +	struct cef168_data data;
> >
> > I thought the compiler would complain for variables declared not at
> > the beginning of the function
>
> Ok, I'll move the variable at the beginning.
>
> > > +	pm_runtime_set_active(&client->dev);
> >
> > Is the device powered up at this point ?
> > If you depend on the pm_runtime_resume_and_get() call in open() to
> > power the device up, then you need to depend on PM in KConfig ?
>
> Yes, the device powers from 3v3 rail of a SBC, which makes it powered
> up as soon as the SBC is up. Given that, should I remove all code
> around Power Management Runtime (pm_runtime_*) as redundant?
>

Yes, I overlooked the fact you don't regiser any resume/suspend
routine, so your calls to pm_runtime_set_active/pm_runtime_put are
nops (they do at least provide usage counting, but I would anyway
remove them).

> > > +#define CEF168_V4L2_CID_CUSTOM(ctrl) \
> > > +	((V4L2_CID_USER_BASE | 168) + custom_##ctrl)
> >
> > I think you need to reserve space for your controls in
> > include/uapi/linux/v4l2-controls.h
> >
> > otherwise this will never be visible to applications ?
>
> I found there is no need for that. Custom control become available
> automatically by name via the v4l2-ctl utility. For example, the focus
> range can be read directly in the terminal as follows:
>
> v4l2-ctl -d $DEV_LENS -C focus_range
>

Yes the driver enuemrates them, but you need to add them to the main
header, otherwise USER_BASE | 168 will be take by someone else.

See in example 7c8c957ef12c41968adb66d785ce1dd5fb2f96e7 which is the
latest commit that adds a custom control space.

Hans, do we need a uapi header file with the definition of the custom
controls registered by this driver ?

Thanks
  j

> > > +/**
> > > + * cef168 data structure
> >
> > No need to kerneldoc unless you properly document all fields and
> > include the file in some of the Documentation/
>
> OK, I'll remove the comment above the structure.
>
> Kind regards,
>   Aliaksandr