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 >
