Re: [PATCH v2 3/6] regulator: Add regulator driver for ATC260x PMICs

[Cristian Ciocaltea]

Hi Mark,

Thanks for reviewing!

On Mon, Aug 24, 2020 at 12:00:45PM +0100, Mark Brown wrote:
> On Sat, Aug 22, 2020 at 01:19:49AM +0300, Cristian Ciocaltea wrote:
> 
> > +static int atc260x_set_voltage_time_sel(struct regulator_dev *rdev,
> > +					unsigned int old_selector,
> > +					unsigned int new_selector)
> > +{
> > +	struct atc260x_regulator_data *data = rdev_get_drvdata(rdev);
> > +	int id = rdev_get_id(rdev);
> > +
> > +	if (new_selector > old_selector)
> > +		return id > data->last_dcdc_reg_id ? data->voltage_time_ldo
> > +						   : data->voltage_time_dcdc;
> 
> Please write normal conditional statements to make things easier to
> read.  It also looks like this would be more robustly written by just
> having separate ops for DCDCs and LDOs, this could easily break if
> another device is supported in the driver.

Sure, I can provide separate ops, but in this case we duplicate almost
all of them. If this is not acceptable, then I will just rewrite the
conditional statement.

> > +static const struct of_device_id atc260x_regulator_of_match[] = {
> > +	{ .compatible = "actions,atc2603c-regulator" },
> > +	{ .compatible = "actions,atc2609a-regulator" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, atc260x_regulator_of_match);
> 
> We don't need compatibles here, this is just reflecting the current
> Linux device model into the OS neutral DT bindings.  Another OS may
> choose to split regulators up differently.  We should just instantiate
> the regulator device from the MFD based on identifying the chip overall.

I have actually seen this in some MFD drivers I had been studying before
starting this work. I wasn't sure what is the rationale behind, I
assumed they have just an informative purpose.

So, if I understand correctly, this approach is deprecated now and I
should remove the compatibles from both the function driver and the
corresponding mfd_cell in the core implementation. And not only for
regulators, but for all the other functions of the MFD device.

Regards,
Cristi