On Thu, 07 Jun 2018, Marek Vasut wrote:
> On 06/07/2018 07:04 AM, Lee Jones wrote:
> > On Wed, 06 Jun 2018, Marek Vasut wrote:
> >> On 06/06/2018 08:16 AM, Lee Jones wrote:
> >>> On Tue, 05 Jun 2018, Marek Vasut wrote:
> >>
> >> [...]
> >>
> >>>>>> -static const struct mfd_cell da9063_devs[] = {
> >>>>>> +static const struct mfd_cell da9063_common_devs[] = {
> >>>>>> {
> >>>>>> .name = DA9063_DRVNAME_REGULATORS,
> >>>>>
> >>>>> Appreciate that these are historical, but these device name defines
> >>>>> make me shudder. They only serve to act as an obfuscation layer when
> >>>>> debugging at platform level. Please consider getting rid of them.
> >>>>
> >>>> The macro can be shared between the core and the drivers, so the names
> >>>> never run out of sync.
> >>>
> >>> Platform driver name changes are vary rare. Even if they are changed,
> >>> even light testing would uncover the fact that child drivers do not
> >>> .probe().
> >>
> >> Sure, while if the macro is used, this problem is avoided altogether.
> >>
> >>> Due to the current obfuscation, I currently have no idea
> >>> what this device's name is.
> >>
> >> I'm sure ctags or git grep can easily help.
> >
> > I'm aware how to get around the 'issue', but it's an additional step
> > which is avoidable. For me personally it comes from doing *a lot* of
> > platform level work and being irritated by the extra grepping. Macros
> > for driver names does not sit right with me at all. There are even
> > worse examples of people defining the MACROs *inside* the driver,
> > which doesn't even benefit from the small redeeming feature you
> > mention above.
>
> If we follow this line of thinking, we could just run cpp and expand all
> macros. Then there's no need for grepping at all. That probably won't be
> the result anyone would like though.
Hmm ... yes, that's the same! :D
> > Anyway, I'm happy with you not wanting to change it. Just leave them
> > as they are for now.
> >>>>>> + {
> >>>>>> + .name = DA9063_DRVNAME_VIBRATION,
> >>>>>> + },
> >>>>>
> >>>>> Place this on a single line please.
> >>>>
> >>>> This would only make the style inconsistent with the ie. LEDs entry.
> >>>>
> >>>>> { .name = DA9063_DRVNAME_VIBRATION },
> >>>
> >>> If that is a one line entry spaced over multiple lines, then that
> >>> should also be changed.
> >>>
> >>> Maybe I will go through and stylise this driver a bit after all (but
> >>> as time is short at the moment, maybe not!) :)
> >>
> >> You'd end up with two entries which look different then the rest, which
> >> triggers my OCD.
> >
> > OCD or not, it's never okay to waste lines. If ordering it not
> > important (which it should not be -- it's fragile to rely on device
> > ordering in MFD cells), the multi-line entries go at the top, followed
> > by the single line entries. If done right, it looks the opposite of
> > bad/out of place.
>
> My point is, the style should at least be consistent. But anyway.
It is consistent.
- Multi-line entries go on multiple lines.
- Single line entries go on single lines.
See drivers/mfd/max77620.c for how it should look.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
