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:

quoted

+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.

quoted

+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