Re: [PATCH 2/2] clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

[Matti Vaittinen]

Hello Stephen

On Sat, Sep 01, 2018 at 12:13:26PM -0700, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-08-31 03:21:23)
> > Hello All,
> > 
> > Just wanted to point out for the reviewers that this patch depends on
> > not yet accepted MFD/regulator patch. (struct/defines in
> > include/linux/mfd/rohm-bd718x7.h were changed)
> > https://lore.kernel.org/lkml/cover.1535545377.git.matti.vaittinen@xxxxxxxxxxxxxxxxx/
> > 
> > On Fri, Aug 31, 2018 at 09:18:19AM +0800, kbuild test robot wrote:
> > > Hi Matti,
> > > 
> > > Thank you for the patch! Yet something to improve:
> > > 
> > > [auto build test ERROR on clk/clk-next]
> > > [also build test ERROR on v4.19-rc1 next-20180830]
> > > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> > 
> > Thus this error was expected. What is the generally best way to work
> > when there is changes to more than one subsystem? With this approach
> > the patch set here won't compile until MFD part gets applied. But if
> > I use old definitions/structs here, then clk tree gets broken when
> > MFD/regulator part changes defines. I see only bad and worse options =)
> > Anyways, I guess sending this patch with new defines (and applying it
> > only after MFD) is still better than applying this with old defines and
> > breaking it when MFD changes. (Assuming all of the patches get applied
> > at some point).
> > 
> 
> Does anything besides the clk driver need the defines that are in the
> header which are used in this file?

The register address for clk enable and bit mask are not used by any
other driver. But I'd like to keep all the register addresses in one
enum. That eases checking changes when new chips come.

The private data for MFD (which contains for example the regmap and chip
type) are required by other drivers too.

> If not, then it's better to put
> those defines in the C file for the clk driver so we don't have to hop
> between files and have merge dependencies. Also, the regmap could
> possibly by grabbed from the dev->parent pointer instead of passing it
> down through an mfd structure, allowing this driver to be more generic
> assuming it is always a child of some mfd device that has a regmap.

Currently clk only uses regmap directly from the struct bd718xx. But I
would like to have the access to chip_type as well. And even if we forget
the chip_type. Well, I only see two bad ways of omitting the struct
bd718xx:

1. Always keep the regmap as first member in parent device's driver_data.
Then we can just get the driver data and do cast to regmap. I am not fan
of this as it breaks as fast as someone changes the struct bd718xx - and
as this is part of MFD - well, there is no guarantees the clk people are
even reviewing such change. Also if we need the chip type we are back
using the struct.

2. Use some regmnap wrapper functions which would only take the parent
dev pointer and dig out the regmap details. This would be doable but
then we still have dependency to these wrappers (so dependency is not
solved). Additionally these wrappers are something we decided to get rid
of few patch versions ago.

I don't expect much renamings to take place after the latest patch set
to MFD/regulator tree has been applied. So maybe the clk patch set can
just wait untill MFD/regulator stuff gets integrated. I would be
gratefull if we could review the clk patch set already now though so I
could do improvements/changes to clk side while waiting for
MFD/regulator to get applied. Also, the devm_ functions (patch 1) are
not depending on MFD - so maybe we can get them reviewed/applied? If I
have the spare time I can look if I can clean up some of those not freed
clkdev lookups.

Br,
	Matti Vaittinen
>