Re: [PATCH v2 4/4] media: i2c: gc0308: new driver

[Sebastian Reichel]

Hi Jacopo,

Thanks for the quick review, much appreciated!

On Tue, Oct 24, 2023 at 10:38:45AM +0200, Jacopo Mondi wrote:
> On Tue, Oct 24, 2023 at 02:59:53AM +0200, Sebastian Reichel wrote:
> > Introduce new driver for GalaxyCore GC0308, which is a cheap
> > 640x480 with an on-chip ISP sensor sold since 2010. Data is
> > provided via parallel bus.
> >
> > Signed-off-by: Sebastian Reichel <sre@xxxxxxxxxx>
> > ---

...

> Annoying questions, is this driver compatible with the newly
> introduced CCI helpers? Not that pressing as you're on regmap
> so you haven't your own read/write routines.

I cannot use devm_cci_regmap_init_i2c(), because I use regmap's
pagination feature. So instead of doing something like this:

{REG_PAGE_SELECT, 0x00},
{CCI_REG8(0x00), 0x23},
{CCI_REG8(0x01), 0x42},
{REG_PAGE_SELECT, 0x01},
{CCI_REG8(0x00), 0x13},
{CCI_REG8(0x01), 0x37},

I can do

{CCI_REG8(0x000), 0x23},
{CCI_REG8(0x001), 0x42},
{CCI_REG8(0x100), 0x13},
{CCI_REG8(0x101), 0x37},

That said, I updated the driver to use the CCI helpers instead of
directly using regmap with the exception of the initialization.

[...]

> > +	gpiod_set_value_cansleep(gc0308->reset_gpio, 1);
> > +	msleep(100);
> > +	gpiod_set_value_cansleep(gc0308->reset_gpio, 0);
> > +	msleep(100);
> 
> Wow! that's long :)

I shortened them a bit. These were just the initial values I put
there. Unfortunately the timings are not described in the datasheet.
I now use values I found in some downstream drivers (10-20ms and
30ms).

[...]

> > +static int gc0308_set_exposure(struct gc0308 *gc0308, enum gc0308_exp_val exp)
> > +{
> > +	const struct gc0308_exposure *regs = &gc0308_exposure_values[exp];
> > +	struct reg_sequence exposure_reg_seq[] = {
> > +		REG_SEQ0(GC0308_LUMA_OFFSET, regs->luma_offset),
> > +		REG_SEQ0(GC0308_AEC_TARGET_Y, regs->aec_target_y),
> > +	};
> > +
> > +	dev_err(gc0308->dev, "exposure: %i\n", exp);
> 
> maybe dev_dbg ?

oops. That should just go away. I had some issues when switching
from V4L2_CID_EXPOSURE to V4L2_CID_AUTO_EXPOSURE_BIAS after the
feedback from Sakari and forgot to remove this "debug" print.

[...]

> > +static int gc0308_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct gc0308 *gc0308 = container_of(ctrl->handler, struct gc0308, hdl);
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(gc0308->dev);
> 
> Sensor drivers are usually not resumed in the s_ctrl call path, but
> usually the current power state is checked and the function returns
> early if the device is not powered
> 
>         if (!pm_runtime_get_if_in_use(&client->dev))
>                 return 0;
> 
> Then in the the s_stream() call path, after the sensor has been
> resumed, the controls are applied by calling
> 
>         __v4l2_ctrl_handler_setup()
> 
> as you aready do!

Right and the value is trashed by the following poweroff anyways.

[...]

> > +	if (gc0308->clk) {
> > +		clkrate = clk_get_rate(gc0308->clk);
> > +		if (clkrate != 24000000)
> > +			dev_warn(dev, "unexpected clock rate: %lu\n", clkrate);
> 
> Should the driver continue to operate if the frequency is not
> supported ?

That's what Sakari suggested. I guess a clock rate of 23.9 MHz
would be fine, but different from 24MHz. Unfortunately the
datasheet does not describe the allowed clock rates. It just says,
that the max framerate is 30FPS at 24MHz. So I think a non-fatal
warning is the right thing to do.

[...]

I fixed the other things and plan to send a new series soon. Just
need to do some more testing.

Greetings,

-- Sebastian

Attachment: signature.asc
Description: PGP signature