Re: [PATCH v8 3/9] mfd: madera: Add common support for Cirrus Logic Madera codecs

[Richard Fitzgerald]

On 26/02/18 17:19, Andy Shevchenko wrote:
> On Mon, Feb 26, 2018 at 7:06 PM, Richard Fitzgerald
> <rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > On 26/02/18 14:11, Andy Shevchenko wrote:
> > > On Mon, Feb 26, 2018 at 3:05 PM, Richard Fitzgerald
> > > <rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> > > > +static void madera_enable_hard_reset(struct madera *madera)
> > > > +{
> > > > +       if (madera->reset_gpio)
> > >
> > >
> > > if (!...)
> > >    return
> >
> > Could do but why bother? For such a trivial function, in my opinion
> >
> > static void madera_enable_hard_reset(struct madera *madera)
> > {
> >          if (madera->reset_gpio)
> >                  gpiod_set_value_cansleep(madera->reset_gpio, 0);
> > }
> >
> > is simpler and more readable than
> >
> > static void madera_enable_hard_reset(struct madera *madera)
> > {
> >          if (!madera->reset_gpio)
> >                  return;
> >
> >          gpiod_set_value_cansleep(madera->reset_gpio, 0);
> > }
>
> The rationale is that if someone wants to add more code you will not
> need to take care of deeper indentation and potentially split lines.

Yes, true. It's probably unlikely here and I'm inclined to leave it
as it is because Lee already acked it.


> > > > +               gpiod_set_value_cansleep(madera->reset_gpio, 0);
> > > > +}
> > > > +
> > > > +static void madera_disable_hard_reset(struct madera *madera)
> > > > +{
> > > > +       if (madera->reset_gpio) {
> > >
> > >
> > > Ditto.
> >
> > As above, yes it would work the other way but I think for such a simple
> > implementation the way I have written it is more readable.
>
> I have different opinion, but yes. It's more matter of taste with
> rationale above (perhaps never happen to this code).
>
> > > > +               gpiod_set_value_cansleep(madera->reset_gpio, 1);
> > > > +               usleep_range(1000, 2000);
> > > > +       }
> > > > +}
>
> > > > +const struct of_device_id madera_of_match[] = {
> > > > +       { .compatible = "cirrus,cs47l35", .data = (void *)CS47L35 },
> > > > +       { .compatible = "cirrus,cs47l85", .data = (void *)CS47L85 },
> > > > +       { .compatible = "cirrus,cs47l90", .data = (void *)CS47L90 },
> > > > +       { .compatible = "cirrus,cs47l91", .data = (void *)CS47L91 },
> > > > +       { .compatible = "cirrus,wm1840", .data = (void *)WM1840 },
> > >
> > >
> > > > +       {},
> > >
> > >
> > > No comma.
> >
> > Seems to be personal preference. Both ways are used in the kernel and
> > we've always used this style so I'll leave it to Lee to decide.
>
> This is not.
> The rationale is pretty obvious, terminator must terminate. With cheap
> price (no comma), we just prevent some potential weird cases (bad
> patch application for example, or not very careful contributor) where
> entry goes after. Compiler will fail.

Yes, ok. I see a lot of people don't do that (I searched). But if I
do a new version of this patch I'll change it.

> > > > +};
>
> > > > +               ret = devm_gpio_request_one(madera->dev,
> > > > +                                           madera->pdata.reset,
> > > > +                                           GPIOF_DIR_OUT |
> > > > GPIOF_INIT_LOW,
> > > > +                                           "madera reset");
> > > > +               if (!ret)
> > > > +                       madera->reset_gpio =
> > > > gpio_to_desc(madera->pdata.reset);
> > >
> > >
> > > Why? What's wrong with descriptors?
>
> > This is what I mean by code going stale when it's acked but then never
> > gets merged. Some time ago there was a reason (which I forget).
>
> So, can we switch to descriptors?

Yes, however as this patch has been in review for nearly 1 year now, and
acked for several months, I'd really hoped we could get it merged now
and update it later.

> > > > +       dev_set_drvdata(madera->dev, madera);
> > > > +       if (dev_get_platdata(madera->dev))
> > >
> > >
> > > What this dance for?
> >
> > Are you perhaps thinking the second line is dev_get_drvdata()?
> > dev_get_platdata() gets a pointer to any pdata, so not related
> > to dev_set_drvdata().
>
> Indeed.
>
> > > > +       ret = mfd_add_devices(madera->dev, PLATFORM_DEVID_NONE,
> > > > +                             mfd_devs, n_devs,
> > > > +                             NULL, 0, NULL);
> > >
> > >
> > > devm_?
> >
> > I can try it and see. It's scary because we can depend on our
> > children but maybe devm_mfd_add_devices() is safe.
>
> It will fail in the same way. It does nothing more, than keeping a
> pointer to release function and its data.
>
> > > > +struct madera_irqchip_pdata;
> > > > +struct madera_codec_pdata;
>
> > > Why do you need platform data in new code?
>
> > Answered in a comment in another patch. We care about allowing people
> > to use our chips with systems that don't use devicetree/acpi. There
> > are also many out-of-tree systems.
>
> a) we don't care about out of tree much;

You might not, but as a commercial company we have to.

> b) there are other means to provide date w/o using platform data:
>   - unified device property API (including built-in device properties)
>   - bunch of lookup tables GPIO, regulator, PWM, etc
>   - fwnode graph for more complex cases with device dependencies

Basically same answer as (a)