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@xxxxxxxxx>
> + */
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:
signature.asc
Description: PGP signature
