Re: [PATCH v3 2/3] media: i2c: add os02g10 image sensor driver

[Tarang Raval]

Hi Sakari, Elgin

> > +static int os02g10_power_on(struct device *dev)
> > +{
> > +     struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +     struct os02g10 *os02g10 = to_os02g10(sd);
> > +     int ret;
> > +
> > +     ret = regulator_bulk_enable(ARRAY_SIZE(os02g10_supply_name),
> > +                                 os02g10->supplies);
> > +     if (ret) {
> > +             dev_err(os02g10->dev, "failed to enable regulators\n");
> > +             return ret;
> > +     }
> > +
> > +     /* T4: delay from DOVDD stable to MCLK on */
> > +     fsleep(5 * USEC_PER_MSEC);
>
> Does the sensor really require this? Typically no delays are required
> before lifting xshutdown.

The 5 ms delay for T4 (DOVDD stable to ECLK on) is required by the
datasheet.

However, T3 and T4 are independent timing requirements, not sequential
ones. The current implementation introduces an unnecessary extra 5 ms
delay before deasserting XSHUTDN.

The comment could also be updated to:
/* Wait for T3/T4 timing requirements after supplies become stable */


Power-up Sequence:

DOVDD    ________/─────────────────────────────────────────────────────
                 |
                 |<------ T4 = 5 ms ------>|
                 |                         |
                 |                         +------ ECLK ON

AVDD     ________________/─────────────────────────────────────────────
                      ^
                      |
                    T1 >= 0 ms

DVDD     ________________________/─────────────────────────────────────
                               |
                               |<------ T3 = 5 ms ------>|
                               |                         |
                               |                         +------ XSHUTDN Release

XSHUTDN  ____________________________________/─────────────────────────

ECLK     __________________________________________/\/\/\/\/\/\/\/\/───

SCCB     ____________________________________________________XXXXXXXXXX
                                                 ^
                                                 |
                                 T5 = 5 ms ------+

> > +
> > +     ret = clk_prepare_enable(os02g10->xclk);
> > +     if (ret) {
> > +             dev_err(os02g10->dev, "failed to enable clock\n");
> > +             goto err_regulator_off;
> > +     }
> > +
> > +     /* T3: delay from DVDD stable to sensor power up stable */
> > +     fsleep(5 * USEC_PER_MSEC);
>
> The supplies were enabled before the clock so right now there's already 5
> ms delay here. Consider with the above comment.
>
> > +
> > +     gpiod_set_value_cansleep(os02g10->reset_gpio, 0);
> > +
> > +     /* T5: delay from sensor power up stable to SCCB initialization */
> > +     fsleep(5 * USEC_PER_MSEC);
> > +
> > +     return 0;
> > +
> > +err_regulator_off:
> > +     regulator_bulk_disable(ARRAY_SIZE(os02g10_supply_name), os02g10->supplies);
>
> A newline here would be nice.
>
> > +     return ret;
> > +}

Best Regards,
Tarang