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

[Stephen Boyd]

Quoting Matti Vaittinen (2018-11-11 23:45:53)
> Hello Stephen & All,
> 
> On Mon, Sep 03, 2018 at 09:38:43AM +0300, Matti Vaittinen wrote:
> > 
> > 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/
> > > 
> > > 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.
> > 
> The MFD renaming patch has now been merged to clk-next. So dependencies
> to MFD should now be in-tree. I will rebase this to clk-next and do
> resend. This patch still requires the devm variants for clkdev lookups
> an devm variant of "parent of provider"-registration. I did submit those
> as part of the cleanup series:
> https://lore.kernel.org/linux-clk/cover.1541161503.git.matti.vaittinen@xxxxxxxxxxxxxxxxx/
> 
> My question is if you prefer me to add this driver in that series and
> resend the series as v4? Or do you want me to send this in own
> individual patch with just a note about the dependency?
> 

Either way is fine. Just be explicit and we'll figure it out. Of course,
be aware that kbuild robot may complain if it can't figure out the
dependency and something fails to compile so it may be easier to resend
the whole combined series as something that compiles.