Re: [PATCH v4 2/2] media: i2c: Add GC08A3 image sensor driver

[sakari.ailus@xxxxxxxxxxxxxxx]

Hi Zhi,

On Tue, Feb 20, 2024 at 05:45:54AM +0000, Zhi Mao (毛智) wrote:
> Hi Laurent,
> 
> Thanks for you reply.
> I'd like to ask for advice about how to contrl "reset-pin", please
> check the below comments.
> 
> On Tue, 2024-02-20 at 05:01 +0200, Laurent Pinchart wrote:
> >  	 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >  Hi Zhi,
> > 
> > On Tue, Feb 20, 2024 at 02:12:26AM +0000, Zhi Mao (毛智) wrote:
> > > On Tue, 2024-02-06 at 20:45 +0200, Laurent Pinchart wrote:
> > > > On Sun, Feb 04, 2024 at 02:15:38PM +0800, Zhi Mao wrote:
> > > > > Add a V4L2 sub-device driver for Galaxycore GC08A3 image
> > sensor.
> > > > >
> > > > > Signed-off-by: Zhi Mao <zhi.mao@xxxxxxxxxxxx>
> > > > > ---
> > > > >  drivers/media/i2c/Kconfig  |   10 +
> > > > >  drivers/media/i2c/Makefile |    1 +
> > > > >  drivers/media/i2c/gc08a3.c | 1448
> > ++++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 1459 insertions(+)
> > > > >  create mode 100644 drivers/media/i2c/gc08a3.c
> > 
> > [snip]
> > 
> > > > > diff --git a/drivers/media/i2c/gc08a3.c
> > b/drivers/media/i2c/gc08a3.c
> > > > > new file mode 100644
> > > > > index 000000000000..3fc7fffb815c
> > > > > --- /dev/null
> > > > > +++ b/drivers/media/i2c/gc08a3.c
> > > > > @@ -0,0 +1,1448 @@
> > 
> > [snip]
> > 
> > > > > +static int gc08a3_power_on(struct device *dev)
> > > > > +{
> > > > > +struct i2c_client *client = to_i2c_client(dev);
> > > > > +struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > > +struct gc08a3 *gc08a3 = to_gc08a3(sd);
> > > > > +int ret;
> > > > > +
> > > > > +ret = regulator_bulk_enable(ARRAY_SIZE(gc08a3_supply_name),
> > > > > +    gc08a3->supplies);
> > > > > +if (ret < 0) {
> > > > > +dev_err(gc08a3->dev, "failed to enable regulators: %d\n",
> > ret);
> > > > > +return ret;
> > > > > +}
> > > > > +
> > > > > +ret = clk_prepare_enable(gc08a3->xclk);
> > > > > +if (ret < 0) {
> > > > > +regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name),
> > > > > +       gc08a3->supplies);
> > > > > +dev_err(gc08a3->dev, "clk prepare enable failed\n");
> > > > > +return ret;
> > > > > +}
> > > > > +
> > > > > +usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US);
> > > > > +
> > > > > +gpiod_set_value_cansleep(gc08a3->reset_gpio, 1);
> > > >
> > > > Are you asserting reset when powering on ? That sounds wrong, you
> > should
> > > > de-assert reset here (and acquire the reset gpio in probe() with
> > > > GPIOD_OUT_HIGH). Drivers should use logical levels for GPIOs,
> > setting a
> > > > GPIO named "reset" to 1 should assert the reset signal, even if
> > the
> > > > physical signal is active low. You may have the wrong polarity in
> > the
> > > > device tree.
> > >
> > > According to the sensor power sequence sepc, "reset" pin should be
> > pull
> > > from low to high after "dovdd/dvdd/avdd" power on, so I follow this
> > > power sequece to pull "reset" pin high in software flow.
> > 
> > From a hardware point of view that's right, but the Linux kernel
> > handles
> > logical level of GPIOs. If a GPIO is named "reset", it is expected
> > that
> > calling
> > 
> > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1);
> > 
> > will "assert" the reset signal, setting it to a logical "reset =
> > true"
> > level. This maps to the hardware 0V output level, as the signal is
> > active-low. To achieve this, define the reset GPIO as active low in
> > DT,
> > and the GPIO framework will invert the signal for you. You should
> > then
> > call
> > 
> > gpiod_set_value_cansleep(gc08a3->reset_gpio, 1);
> > 
> > in the driver when you want to assert reset (set it to 0V), and
> > 
> > gpiod_set_value_cansleep(gc08a3->reset_gpio, 0);
> > 
> > when you want to deassert it (set it to 3.3V, or whatever the I/O
> > voltage for the signal is).
> > 
> > This way all driver use logical states, and the inversion is handled
> > in
> > DT.
> > 
> 
> Sensor power sequence as below:
>                       ------------------
>                      | | |
>                      | | | 
> dvdd/avdd/dovdd  --------
>                                ---------
>                               |
>                               |
> reset-pin        -------------
> 
> In order to match this power sequece, "reset-pin" contrl flow is below:
> 1. config the "reset-pin" is "active-high" in DTS:
>     reset-gpios = <&pio 19 GPIO_ACTIVE_HIGH>;
> 
> 2. image sensor driver probe function:
> gc08a3->reset_gpio = devm_gpiod_get(dev, "reset",
> GPIOD_OUT_LOW);  //init "reset-pin" is low
> 
> 3. image sensor driver power_on function:
> gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); //pull "reset-pin"
> high 
> 
> so, the expect state of "reset-pin" is from low to high.
> If I am wrong, please correct me.


>From Documentation/driver-api/gpio/consumer.rst:

	As a consumer should not have to care about the physical line
	level, all of the gpiod_set_value_xxx() or
	gpiod_set_array_value_xxx() functions operate with the *logical*
	value. With this they take the active low property into account.
	This means that they check whether the GPIO is configured to be
	active low, and if so, they manipulate the passed value before the
	physical line level is driven.

I.e. when you want to enable reset, you set the value to 1 in the driver. I
think you're now setting the value to 0 in that case. The opposite for
disabling it of course.

-- 
Regards,

Sakari Ailus