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

Message-ID: <20201006105637.GB5259@sirena.org.uk>
Date:   Tue, 6 Oct 2020 11:56:37 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Cristian Marussi <cristian.marussi@....com>
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

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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)