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

[Dongchun Zhu <dongchun.zhu@mediatek.com>]

Hi Tomasz,

On Fri, 2020-06-12 at 20:49 +0200, Tomasz Figa wrote:
> 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
> 

Thank you for the sharing.
If driver only focus on the logical function of GPIO pins,
'n_rst_gpio' may need to be renamed back to the 1st version 'rst_gpio'.

> Best regards,
> Tomasz