Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 25 Aug 2020 02:23:10 +0300
From:   Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
To:     Mark Brown <broonie@kernel.org>
Cc:     Lee Jones <lee.jones@linaro.org>, Rob Herring <robh+dt@kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Sebastian Reichel <sre@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        Andreas =?iso-8859-1?Q?F=E4rber?= <afaerber@suse.de>,
        linux-actions@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        linux-pm@vger.kernel.org
Subject: Re: [PATCH v2 3/6] regulator: Add regulator driver for ATC260x PMICs
Message-ID: <20200824232310.GA2301286@BV030612LT>
References: <cover.1598043782.git.cristian.ciocaltea@gmail.com>
 <8da70f0b19de17fb8edead7ff06461ae2451b0e9.1598043782.git.cristian.ciocaltea@gmail.com>
 <20200824110045.GA4676@sirena.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200824110045.GA4676@sirena.org.uk>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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