Re: [PATCH v8 08/12] regulator: bd718x7: Split driver to common and bd718x7 specific parts

[Vaittinen, Matti]

Hello Lee,

On Tue, 2020-01-07 at 12:41 +0000, Lee Jones wrote:
> On Mon, 30 Dec 2019, Matti Vaittinen wrote:
> 
> > Few ROHM PMICs allow setting the voltage states for different
> > system states
> > like RUN, IDLE, SUSPEND and LPSR. States are then changed via SoC
> > specific
> > mechanisms. bd718x7 driver implemented device-tree parsing
> > functions for
> > these state specific voltages. The parsing functions can be re-used 
> > by
> > other ROHM chip drivers like bd71828. Split the generic functions
> > from
> > bd718x7-regulator.c to rohm-regulator.c and export them for other
> > modules
> > to use.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> > Acked-by: Mark Brown <broonie@xxxxxxxxxx>
> > ---
> > 
> > +
> > +struct rohm_dvs_config {
> > +	uint64_t level_map;
> > +	unsigned int run_reg;
> > +	unsigned int run_mask;
> > +	unsigned int run_on_mask;
> > +	unsigned int idle_reg;
> > +	unsigned int idle_mask;
> > +	unsigned int idle_on_mask;
> > +	unsigned int suspend_reg;
> > +	unsigned int suspend_mask;
> > +	unsigned int suspend_on_mask;
> > +	unsigned int lpsr_reg;
> > +	unsigned int lpsr_mask;
> > +	unsigned int lpsr_on_mask;
> > +};
> 
> I think this deserves a kernel-doc header.
Oh, the struct here? Hmm. You might be correct. I can add some.

> 
> > +#if IS_ENABLED(CONFIG_REGULATOR_ROHM)
> > +int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config
> > *dvs,
> > +				  struct device_node *np,
> > +				  const struct regulator_desc *desc,
> > +				  struct regmap *regmap);
> 
> Does these really need to live in the parent's header file?

I don't know what would be a better place?

> What other call-sites are there?

After this series the bd718x7-regulator.c and bd71828-regulator.c are
the in-tree drivers using these. rohm-regulator.c is implementing them.
And I hope we see yet another driver landing in later this year. 

Anyways, I will investigate if I can switch this to some common (not
rohm specific) DT bindings at some point (I've scheduled this study to
March) - If I can then they should live in regulator core headers.

But changing the existing properties should again be own set of patches
and I'd prefer doing that work independently of this series and not
delaying the BD71828 due to not-yet-evaluated bd718x7 property changes.

> 
> > +#else
> > +static inline int rohm_regulator_set_dvs_levels(const struct
> > rohm_dvs_config *dvs,
> > +						struct device_node *np,
> > +						const struct
> > regulator_desc *desc,
> > +						struct regmap *regmap)
> > +{
> > +	return 0;
> > +}
> > +#endif //IS_ENABLED(CONFIG_REGULATOR_ROHM)
> 
> a) This comment is not really required
> b) You shouldn't be using C++ comments

Thanks, I'll remove it.

Best Regards
	Matti