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

[Aliaksandr Smirnou <support@pinefeat.co.uk>]

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.

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

> > +	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?

> > +#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

> > +/**
> > + * 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