Re: [PATCH v3 2/3] media: ov8856: Add devicetree support

[Robert Foss]

On Mon, 6 Apr 2020 at 17:06, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>
> On Mon, Apr 06, 2020 at 03:37:24PM +0200, Robert Foss wrote:
> > On Tue, 31 Mar 2020 at 16:01, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > > On Tue, Mar 31, 2020 at 4:36 PM Robert Foss <robert.foss@xxxxxxxxxx> wrote:
>
> ...
>
> > > > +       if (ret < 0) {
> > >
> > > Do you need all ' < 0' parts all over the series?
> >
> > Some checks are needed due to ACPI and DT support co-existing.
> > Maybe it would be better to just split the probing into an ACPI path
> > and a DT path.
> >
> > I'll have a look through the series for redundant retval checks.
>
> Drop where it is redundant.
>
> ...
>
> > > > -       ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > > -       if (ret)
> > > > -               return ret;
> > >
> > > Where is it gone? Why?
> >
> > It was replaced by a clk_get_rate call, which as Sakari pointed out,
> > isn't correct.
> > I'll rework the clock handling for v4.
>
> If it was in the driver it should stay -- properties is an ABI (between firmware and kernel).

Ack.

>
> > > > +       ov8856->xvclk = devm_clk_get_optional(dev, "xvclk");
> > > > +       if (IS_ERR(ov8856->xvclk)) {
> > >
> > > > +               dev_err(dev, "could not get xvclk clock (%ld)\n",
> > > > +                       PTR_ERR(ov8856->xvclk));
> > >
> > > Also you may use %pe here and in similar cases.
> >
> > Weirdly checkpatch complains about this.
> > But it builds and runs cleanly, so I'll add it in v4.
>
> %pe requires pointer, PTR_ERR converts pointer to integer.

Ack.

>
> ...
>
> > > > +       ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names),
> > > > +                                     ov8856->supplies);
> > > > +       if (ret) {
> > >
> > > > +               dev_warn(dev, "failed to get regulators\n");
> > >
> > > If it's a warning, why we return from here?
> > > Same question to all other places with same issue.
> >
> > The issue I was seeing was the driver having to return a EDEFER here,
> > so this warning sheds some light on which exact component is returning
> > an EDEFER.
> >
> > [   15.962623] ov8856 16-0010: Dropping the link to regulator.29
> > [   15.968464] ov8856 16-0010: failed to get regulators
> > [   15.973493] ov8856 16-0010: failed to get HW configuration: -517
> > [   15.979591] ov8856 16-0010: removing from PM domain titan_top_gdsc
> > [   15.985855] ov8856 16-0010: genpd_remove_device()
> > [   15.990672] i2c 16-0010: Driver ov8856 requests probe deferral
> >
> > Personally I found it helpful to speed up debugging, but I'll happily
> > remove it if you prefer no warning.
>
> My point is that you have it in align:
>  - if it is an error, print as an error and bail out, otherwise
>  - if it is a warning, print it and continue.

I see what you're saying now, let's remove it then :)
I guess in the specific case of EDEFER, it doesn't fit neatly into
either of those categories, in the sense that the way you continue is
to return and then try to probe again later.

There are some other locations where this is handled wrong, I'll align
them properly for v4.

>
> > > > +               return ret;
> > > >         }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>