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 --]
next prev parent reply other threads:[~2020-10-06 10:57 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-05 22:26 [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator Cristian Marussi
2020-10-05 22:26 ` [PATCH 1/4] firmware: arm_scmi: Add Voltage Domain Support Cristian Marussi
2020-10-08 10:10 ` Etienne Carriere
2020-10-09 12:37 ` Cristian Marussi
2020-10-05 22:26 ` [PATCH 2/4] firmware: arm_scmi: add SCMI Voltage Domain devname Cristian Marussi
2020-10-05 22:26 ` [PATCH 3/4] regulator: add SCMI driver Cristian Marussi
2020-10-06 10:56 ` Mark Brown [this message]
2020-10-15 15:38 ` Cristian Marussi
2020-10-05 22:26 ` [PATCH 4/4] dt-bindings: arm: add support for SCMI Regulators Cristian Marussi
2020-10-06 10:59 ` Mark Brown
2020-10-07 8:06 ` Cristian Marussi
2020-10-06 0:38 ` [PATCH 0/4] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator Florian Fainelli
2020-10-09 12:42 ` Cristian Marussi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201006105637.GB5259@sirena.org.uk \
--to=broonie@kernel.org \
--cc=Jonathan.Cameron@Huawei.com \
--cc=cristian.marussi@arm.com \
--cc=etienne.carriere@linaro.org \
--cc=james.quinlan@broadcom.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lukasz.luba@arm.com \
--cc=robh@kernel.org \
--cc=satyakim@qti.qualcomm.com \
--cc=sudeep.holla@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).