linux-kernel - Re: [PATCH 3/4] regulator: add SCMI driver

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] [day] [month] [year] [list]

Date:   Thu, 15 Oct 2020 16:38:18 +0100
From:   Cristian Marussi <cristian.marussi@....com>
To:     Mark Brown <broonie@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        sudeep.holla@....com, lukasz.luba@....com,
        james.quinlan@...adcom.com, Jonathan.Cameron@...wei.com,
        robh@...nel.org, satyakim@....qualcomm.com,
        etienne.carriere@...aro.org
Subject: Re: [PATCH 3/4] regulator: add SCMI driver

Hi

sorry for the late reply.

On Tue, Oct 06, 2020 at 11:56:37AM +0100, Mark Brown wrote:
> On Mon, Oct 05, 2020 at 11:26:22PM +0100, Cristian Marussi wrote:
> 
> > - .get_voltage / .set_voltage: routed via SCMI Voltage Domain Protocol
> > - .get_voltage_sel/.set_voltage_sel: using regulator framework helpers
> 
> You should not be implementing both of these interfaces, pick one.  It
> looks like the direct voltage operations are the redundant ones here,
> while the protocol uses actual voltages to communicate with the firmware
> which makes the direct voltage operations a better fit it seems like the
> expectation is that only a limited set of voltages is supported (as is
> normal for the underlying physical regulators) so you want selectors.
> 

I'm dropping non _sel methods in V2.

> > +	sreg->name = devm_kasprintf(dev, GFP_KERNEL, "%s", vinfo->name);
> > +	sreg->desc.name = devm_kasprintf(dev, GFP_KERNEL,
> > +					 "Vscmi.%s", sreg->name);
> > +	if (!sreg->name || !sreg->desc.name)
> > +		return -ENOMEM;
> 
> Why are we using different names here?
> 

Not really a good reason...dropping internal name and "Vscmi" prefix
in V2.

> > +	num_doms = handle->voltage_ops->num_domains_get(handle);
> > +	if (num_doms <= 0) {
> > +		dev_err(&sdev->dev, "number of voltage domains invalid\n");
> > +		return num_doms ?: -EINVAL;
> 
> Please write normal conditional statements to improve legibility.

Ok.

Thanks

Cristian