Re: [V8, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver

[Tomasz Figa <tfiga@chromium.org>]

On Fri, Jun 12, 2020 at 11:33 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Tomasz,
>
> On Wed, 2020-06-10 at 18:36 +0000, Tomasz Figa wrote:
> > On Sat, May 23, 2020 at 12:50:15PM +0800, Dongchun Zhu wrote:
> > > Hi Tomasz,
> > >
> > > Thanks for the review. My replies are as below.
> > >
> > > On Thu, 2020-05-21 at 19:32 +0000, Tomasz Figa wrote:
> > > > Hi Dongchun,
> > > >
> > > > On Sat, May 09, 2020 at 04:06:27PM +0800, Dongchun Zhu wrote:
> > [snip]
> > > > > +{
> > > > > +       struct i2c_client *client = to_i2c_client(dev);
> > > > > +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > > +       struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > > > > +       int ret;
> > > > > +
> > > > > +       gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
> > > > > +       gpiod_set_value_cansleep(ov02a10->pd_gpio, 0);
> > > > > +
> > > > > +       ret = clk_prepare_enable(ov02a10->eclk);
> > > > > +       if (ret < 0) {
> > > > > +               dev_err(dev, "failed to enable eclk\n");
> > > > > +               return ret;
> > > > > +       }
> > > > > +
> > > > > +       ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> > > > > +       if (ret < 0) {
> > > > > +               dev_err(dev, "failed to enable regulators\n");
> > > > > +               goto disable_clk;
> > > > > +       }
> > > > > +       usleep_range(5000, 6000);
> > > > > +
> > > > > +       gpiod_set_value_cansleep(ov02a10->pd_gpio, 1);
> > > >
> > > > This is a "powerdown" GPIO. It must be set to 0 if the sensor is to be
> > > > powered on.
> > > >
> > >
> > > The value set by gpiod_set_value_cansleep() API actually depends upon
> > > GPIO polarity defined in DT.
> > > Since I set GPIO_ACTIVE_LOW to powerdown,
> > > gpiod_set_value_cansleep(gpio_desc, value) would set !value to
> > > gpio_desc.
> > > Thus here powerdown would be low-state when sensor is powered on.
> > > For GPIO polarity, I also post a comment to the binding patch.
> > >
> >
> > That's true. However, this makes the driver really confusing. If someone
> > reads this code and compares with the datasheet, it looks incorrect,
> > because in the datasheet the powerdown GPIO needs to be configured low
> > for the sensor to operate.
> >
> > I'd recommend defining the binding in a way that makes it clear in the driver code
> > that it implementes the power sequencing as per the datasheet.
> >
>
> Uh-huh...
> But it all depends on how we look at the powerdown GPIO.
> Or where should we define the active low or active high, the driver or
> DT?
>
> My initial idea is using DT GPIO polarity to describe sensor active
> polarity according to the datasheet.
> As an active low shutdown signal is equivalent to an active high enable
> signal.
>

Okay, I discussed this offline with Laurent and Sakari and we also
found the guidelines of the Linux GPIO subsystem on this [1].

The conclusion is that the pin names in the driver or DT must not
contain any negation prefixes and the driver needs to care only about
the logical function of the pin, such as "powerdown" or "reset". In
case of this driver, we should call the pins "rst" and "pd" and
setting them to 1 would trigger the reset and power down respectively.
The physical signal polarity must be configured in DT using the
polarity flags.

[1] https://www.kernel.org/doc/html/latest/driver-api/gpio/consumer.html#the-active-low-and-open-drain-semantics

Best regards,
Tomasz