Re: [PATCH v1 8/9] regulator: keembay: Add regulator for Keem Bay SoC

[Mark Brown <broonie@kernel.org>]

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

On Thu, Jan 14, 2021 at 11:26:59PM +0800, Muhammad Husaini Zulkifli wrote:

> Keem Bay SD regulator driver module is added to encapsulate ARM
> Secure Monitor Call Calling Convention (SMCCC) during set voltage
> operations to control I/O Rail supplied voltage levels which communicate
> with Trusted Firmware.

Adding in Sudeep again for the SMCCC bits.  I just checked and am I
right in thinking this might already be shipping in production?

> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Keem Bay SD Regulator
> + *
> + * Copyright (C) 2020, Intel Corporation
> + * Author: Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
> + */

Please make the entire comment a C++ comment to improve legibility.

> +static int keembay_regulator_set_voltage(struct regulator_dev *dev,
> +					int min_uV, int max_uV,
> +					unsigned *selector)
> +{
> +	int tmp_volt;
> +
> +	if (min_uV == KEEMBAY_IOV_1_8V_uV && max_uV == KEEMBAY_IOV_1_8V_uV)
> +		tmp_volt = KEEMBAY_SET_1V8_IO_RAIL;
> +	else
> +		tmp_volt = KEEMBAY_SET_3V3_IO_RAIL;
> +
> +	return keembay_set_io_rail_supplied_voltage(tmp_volt);
> +}

This has serious problems with input validation and is broken for most
valid input.  A set_voltage() function should set the voltage to the
lowest valid voltage within the range specified in the arguments and
return an error if it is not possible to set a voltage within the range
specified by the arguments.  This function will set 3.3V for any input
range other than exactly 1.8V so for example if the caller validly sets
a range of 1.7V-1.9V then 3.3V will be selected instead of 1.8V, or if
the user sets 1.0-1.1V then it will set 3.3V instead of returning an
error.

> +static int keembay_regulator_get_voltage(struct regulator_dev *dev)
> +{
> +	int ret;
> +
> +	ret = keembay_get_io_rail_supplied_voltage();
> +
> +	return ret ? KEEMBAY_IOV_1_8V_uV : KEEMBAY_IOV_3_3V_uV;
> +}

This ignores any errors or out of bounds values returned by the called
function, and please write normal conditional statements rather than
using the ternery operator to improve legibility.

> +/*
> + * Using subsys_initcall to ensure that Keem Bay regulator platform driver
> + * is initialized before device driver try to utilize it.
> + */
> +subsys_initcall(keembay_regulator_init);

There is no need to do this, probe deferral will ensure that
dependencies will be resolved.

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