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

[Cristian Marussi]

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