Re: [V1, 2/2] media: i2c: Add more sensor mode for ov8856 camera sensor

[Tomasz Figa <tfiga@chromium.org>]

Hi Dongchun,

On Mon, Sep 9, 2019 at 6:27 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Tomasz,
>
> On Fri, 2019-08-23 at 19:01 +0900, Tomasz Figa wrote:
> > Hi Dongchun,
> >
> > On Thu, Aug 08, 2019 at 05:22:15PM +0800, dongchun.zhu@mediatek.com wrote:
[snip]
> > > +
> > >  /* vertical-timings from sensor */
> > >  #define OV8856_REG_VTS                     0x380e
> > >  #define OV8856_VTS_MAX                     0x7fff
> > > @@ -64,6 +80,14 @@
> > >
> > >  #define to_ov8856(_sd)                     container_of(_sd, struct ov8856, sd)
> > >
> > > +static const char * const ov8856_supply_names[] = {
> > > +   "dovdd",        /* Digital I/O power */
> > > +   "avdd",         /* Analog power */
> > > +   "dvdd",         /* Digital core power */
> > > +};
> > > +
> > > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)
> > > +
> > >  enum {
> > >     OV8856_LINK_FREQ_720MBPS,
> > >     OV8856_LINK_FREQ_360MBPS,
> > > @@ -316,6 +340,208 @@ static const struct ov8856_reg mode_3280x2464_regs[] = {
> > >     {0x5e00, 0x00}
> > >  };
> > >
> > > +static const struct ov8856_reg mode_3264x2448_regs[] = {
[snip]
> > > +};
> > > +
> >
> > It would be better if we could find the differences between the two arrays
> > and handle them incrementally.
> >
>
> This approach is not recommended.
>

Not recommended by whom? :) I myself recommend that approach.

I'm sorry, but I'm going to NACK this patch (including the
chromeos-4.19 tree), unless there is a very good technical reason not
to do it the way I'm suggesting. The other drivers do it that way and
I see no reason why this one should be an exception.

> For these two arrays, sensor input clock frequencies (19.2MHz, 24MHz)
> are different, corresponding to different PLL register setting.
>
> Besides, there are also some differences in image resolution and
> hts/vts, including 0x3614 register that reflecting sensor revision.
>

What would be the reason preventing us from handling that in driver code?

Note that I do _not_ mean just taking addresses and values that are
different and putting them to a separate array. What I'm asking for is
to handle the differences in a programmatic way - with dedicated code
in the driver setting appropriate registers.

[snip]

> > > +           fmt->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> > > +   else
> > > +           fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> > > +
> > >     fmt->field = V4L2_FIELD_NONE;
> > >  }
> > >
> > > @@ -850,6 +1333,17 @@ static int ov8856_start_streaming(struct ov8856 *ov8856)
> > >             return ret;
> > >     }
> > >
> > > +   /* update R3614 for 1B module */
> >
> > What's R3614?
> >
>
> R3614 is the register 0x3614, which reflects the sensor revision.
> For instance, it would be 0x20 for 1B module, while 0x60 for 2A module.
>

My point is - this comment doesn't mean anything for a person reading
it. The code below is actually more meaningful - you can see that the
clock settings register is written with a value for 1B.

> > > +   if (ov8856->is_1B_module) {
> > > +           ret = ov8856_write_reg(ov8856, OV8856_CLK_REG,
> > > +                                  OV8856_REG_VALUE_08BIT,
> > > +                                  OV8856_CLK_REG_1B_VAL);

Please define this value according to what it means, not a fixed
constant for 1B sensor revision.

> > > +           if (ret) {
> > > +                   dev_err(&client->dev, "failed to set R3614");
> > > +                   return ret;
> > > +           }
> > > +   }
> > > +
> > >     ret = __v4l2_ctrl_handler_setup(ov8856->sd.ctrl_handler);
> > >     if (ret)
> > >             return ret;
> > > @@ -882,6 +1376,8 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> > >     if (ov8856->streaming == enable)
> > >             return 0;
> > >
> > > +   dev_dbg(&client->dev, "hardware version: (%d)\n", ov8856->is_1B_module);
> > > +
> > >     mutex_lock(&ov8856->mutex);
> > >     if (enable) {
> > >             ret = pm_runtime_get_sync(&client->dev);
> > > @@ -908,6 +1404,54 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> > >     return ret;
> > >  }
> > >
> > > +/* Calculate the delay in us by clock rate and clock cycles */
> > > +static inline u32 ov8856_cal_delay(u32 cycles)
> > > +{
> > > +   return DIV_ROUND_UP(cycles, OV8856_XVCLK_FREQ / 1000 / 1000);
> > > +}
> > > +
> > > +static int __ov8856_power_on(struct ov8856 *ov8856)
> > > +{
> > > +   int ret;
> > > +   u32 delay_us;
> > > +   struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > > +
> > > +   ret = clk_prepare_enable(ov8856->xvclk);
> > > +   if (ret < 0) {
> > > +           dev_err(&client->dev, "Failed to enable xvclk\n");
> > > +           return ret;
> > > +   }
> > > +
> > > +   gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> >
> > According to my datasheet, this sensor doesn't have a reset pin. The one I
> > can see there is XSHUTDN, which I would call "n_shutdn" here.
> >
>
> I would rename this pin in next release.
> BTW, how do you define "n_shutdn" or "shuutdn"?
> If GPIO is actively high, then "n_shutdn"?
>

If the GPIO is active-high (means shutdown on high) then it's just
"shutdn_gpio". However, the datasheet says it's active-low (means
shutdown on low), so that should be "n_shutdn_gpio".

> > > +
> > > +   ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > > +   if (ret < 0) {
> > > +           dev_err(&client->dev, "Failed to enable regulators\n");
> > > +           goto disable_clk;
> > > +   }
> > > +
> > > +   gpiod_set_value_cansleep(ov8856->reset_gpio, 0);
> >
> > According to the datasheet, XSHUTDN should be 0 for shutdown and 1 for
> > running. Why is it the other way around here?
> >
>
> For GPIO, the definition of bit field of flags defined in DT seems
> reversed.
> This would be fixed in next release.
>
> > > +
> > > +   /* 8192 cycles prior to first SCCB transaction */
> > > +   delay_us = ov8856_cal_delay(8192);
> >
> > If we pass a constant to the function and the function itself only uses
> > constants inside, could we just define a constant delay instead?
> >
>
> This calculation refers to powering up sequence in datasheet.
> Did you mean using usleep_range() directly?

My point is, we can just

#define OV8856_SCCB_INIT_DELAY_US    (8192 * [...])

[...]

usleep_range(OV8856_SCCB_INIT_DELAY_US, OV8856_SCCB_INIT_DELAY_US + 200);

>
> > > +   usleep_range(delay_us  * 2, delay_us * 4);
> >
> > Normally one one just give some small delta here, like +/- 100 us.
> >
>
> Fixed in next release.
>
> > > +
> > > +   return 0;
> > > +
> > > +disable_clk:
> > > +   clk_disable_unprepare(ov8856->xvclk);
> > > +
> > > +   return ret;
> > > +}
> > > +
> > > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > > +{
> > > +   clk_disable_unprepare(ov8856->xvclk);
> > > +   gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > +
> > > +   regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > > +}
> > > +
> > >  static int __maybe_unused ov8856_suspend(struct device *dev)
> > >  {
> > >     struct i2c_client *client = to_i2c_client(dev);
> > > @@ -915,8 +1459,8 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
> > >     struct ov8856 *ov8856 = to_ov8856(sd);
> > >
> > >     mutex_lock(&ov8856->mutex);
> > > -   if (ov8856->streaming)
> > > -           ov8856_stop_streaming(ov8856);
> > > +
> > > +   __ov8856_power_off(ov8856);
> >
> > This change is incorrect because it will power off even if the device was
> > already powered off, causing reference count mismatch. The original code
> > was okay.
> >
>
> Then do we need to power off sensor per power off sequence?
> I thought this function would be called by pm_runtime_put when power
> count is 0.
>

This is the system suspend callback, not runtime suspend callback.
It's only called when the system goes to sleep.

> > >
> > >     mutex_unlock(&ov8856->mutex);
> > >
> > > @@ -1089,6 +1633,20 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> > >             return -ENXIO;
> > >     }
> > >
> > > +   /* set R3614 to distinguish harward versions */
> >
> > hardware
> >
>
> Sorry for the typo.
> Fixed in next release.
>

Also a similar comment for R3614, as above. It doesn't have any
meaning to someone without the datasheet in front of them. Please just
remove it.

Best regards,
Tomasz