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
