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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 25 Aug 2020 02:23:10 +0300
From:   Cristian Ciocaltea <cristian.ciocaltea@...il.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     Lee Jones <lee.jones@...aro.org>, Rob Herring <robh+dt@...nel.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Sebastian Reichel <sre@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Andreas Färber <afaerber@...e.de>,
        linux-actions@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 3/6] regulator: Add regulator driver for ATC260x PMICs

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