Messages in this thread | | | Subject | Re: [PATCH v8 3/9] mfd: madera: Add common support for Cirrus Logic Madera codecs | From | Richard Fitzgerald <> | Date | Mon, 26 Feb 2018 17:37:18 +0000 |
| |
On 26/02/18 17:19, Andy Shevchenko wrote: > On Mon, Feb 26, 2018 at 7:06 PM, Richard Fitzgerald > <rf@opensource.cirrus.com> wrote: >> On 26/02/18 14:11, Andy Shevchenko wrote: >>> On Mon, Feb 26, 2018 at 3:05 PM, Richard Fitzgerald >>> <rf@opensource.cirrus.com> 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)
| |