Messages in this thread | | | Date | Thu, 15 Oct 2020 16:38:18 +0100 | From | Cristian Marussi <> | 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
| |