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

From: Mark Brown <broonie@kernel.org>
To: Cristian Marussi <cristian.marussi@arm.com>
Cc: linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, sudeep.holla@arm.com,
	lukasz.luba@arm.com, james.quinlan@broadcom.com,
	Jonathan.Cameron@Huawei.com, robh@kernel.org,
	satyakim@qti.qualcomm.com, etienne.carriere@linaro.org
Subject: Re: [PATCH 3/4] regulator: add SCMI driver
Date: Tue, 6 Oct 2020 11:56:37 +0100	[thread overview]
Message-ID: <20201006105637.GB5259@sirena.org.uk> (raw)
In-Reply-To: <20201005222623.1123-4-cristian.marussi@arm.com>

[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]

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.

> +	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?

> +	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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]