linux-kernel - Re: [PATCH v2 4/4] media: i2c: gc0308: new 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231026021449.xrslmujpmp3upbhv@mercury.elektranox.org>
Date:   Thu, 26 Oct 2023 04:14:49 +0200
From:   Sebastian Reichel <sre@...nel.org>
To:     Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc:     Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] media: i2c: gc0308: new driver

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@...nel.org>
> > ---

...

> 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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)