Message-ID: <20180705125124.GQ496@dell>
Date:   Thu, 5 Jul 2018 13:51:24 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc:     mturquette@...libre.com, robh+dt@...nel.org, mark.rutland@....com,
        lgirdwood@...il.com, broonie@...nel.org, mazziesaccount@...il.com,
        arnd@...db.de, dmitry.torokhov@...il.com, sre@...nel.org,
        chenjh@...k-chips.com, andrew.smirnov@...il.com,
        linus.walleij@...aro.org, kstewart@...uxfoundation.org,
        heiko@...ech.de, gregkh@...uxfoundation.org, eballetbo@...il.com,
        sboyd@...nel.org, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-input@...r.kernel.org, mikko.mutanen@...rohmeurope.com,
        heikki.haikola@...rohmeurope.com
Subject: Re: [PATCH v9 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

> > > > > +++ b/include/linux/mfd/bd71837.h
> > > > > @@ -0,0 +1,391 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > 
> > > > I thought these were meant to use C++ (//) comments?
> > > 
> > > The checkpatch.pl did complain if I used // comment on SPDX line for
> > > header file. OTOH for c-file it required // comments and complained
> > > about /* */ - version. So I did as it suggested and used 
> > 
> > Well that's just dandy -- who comes up with this stuff?
> 
> Engineers I bet =)

Ones who do not suffer from OCD, clearly!

> > > > > +/* Copyright (C) 2018 ROHM Semiconductors */
> > > > > +
> > > > > +/*
> > > > > + * ROHM BD71837MWV header file
> > > > > + */
> > > > 
> > > > If you prefix the mentions of (BD,bd)71837 with (ROHM,rohm) then this
> > > > comment becomes redundant and you can remove it.
> > > 
> > > I am sorry but I am not sure what you suggest here. Do you mean I should
> > > add ROHM or rohm to all definitions here? I think that would make
> > > definitions pretty long so I would certinly need to split more lines.
> > > Such cange would also impact already applied patches. So maybe I
> > > misinterpreted your comment? And in case I did not - can this prefixing of
> > > types - be done as a separate patch set as it impacts to regulators and
> > > clk too? (clk is not yet applied so that is easy to change though).
> > 
> > Any lines which are already long, you can justify as not having to do
> > this, however I think for the filenames and function names it would
> > be nice if they were prefixed.
> 
> Right. For file names this should be easily doable. And when the regmap
> wrappers are removed there are no public functions left. And I think the
> name of file containing the functions is sufficient for grouping
> functions under "Rohm", right? 

That's fine.

> > Filenames are particularly important.  That way all of the Rohm
> > drivers will be grouped.  Unless you can be assured that all Rohm
> > devices will be prefixed by 'bd', then the point is slightly mooted,
> > however since 'bd' doesn't really correlate with 'rohm' it's still
> > difficult to assume that bd* drivers are associated with Rohm -- if
> > you catch my drift.
> 
> I guess I do catch it. And no, all ROHM stuff will definitely not be
> prefixed with bd - which is the name of the power chip

> I mean power IC series.

Now you're getting it! ;)

> > > > > +struct bd71837_board {
> > > > > +	struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
> > > > > +	int	gpio_intr;
> > > > > +};
> > > > 
> > > > Where is this populated?
> > > > 
> > > Currently nowhere as I use device-tree for getting the regulator/irq
> > > config. This is for architectures which do not use DT - but as I don't
> > > have one for testing I did leave the depends_on OF. If it was populated
> > > I would expect it to be done in some setup code.
> > 
> > Please don't add code for 'what ifs'.
> > 
> > Please remove it and add it back when you need it.
> 
> Allright. Although this will also break the regulator part then...

Well, it's broken anyway ...

> > > > > +/*
> > > > > + * bd71837 sub-driver chip access routines
> > > > > + */
> > > > > +
> > > > 
> > > > Please don't abstract APIs for no reason.
> > > > 
> > > > Use the regmap_* API directly instead.
> > > > 
> > > 
> > > Yes. This was already pointed out by Stephen Boyd. But again, as part of
> > > the patches (reguators) were already applied using the wrappers - I asked
> > > if I can remove these in separate patch set after getting this initial
> > > version out.
> > 
> > That is one of the issues with applying related patches without each
> > of them being reviewed first.  Applying unsuitable code is not the
> > correct thing to do, sorry.
> 
> So I assume you are not Ok with adding the wrappers and removing them
> with later set of patches? I'll do following workaround then:

No, I'm not okay with that at all. :|

> 1. Change MFD Kconfig option name => current regulator code will not be
> compiled even if the MFD would be applied.
> 2. Change MFD according to this discussion (and break the compatibility
> with applied regulator code)
> 3. Fix the regulator code with further patches to Mark
> 4. Fix the depends_on Kconfig option in regulator tree to match the new
> one suggested here.
> 
> Does this sound reasonable?

That's how I would do it.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Powered by blists - more mailing lists