Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 14 Jan 2021 17:14:34 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Cc:     ulf.hansson@linaro.org, lgirdwood@gmail.com, robh+dt@kernel.org,
        devicetree@vger.kernel.org, adrian.hunter@intel.com,
        michal.simek@xilinx.com, linux-mmc@vger.kernel.org,
        linux-kernel@vger.kernel.org, andriy.shevchenko@intel.com,
        Rashmi.A@intel.com, mahesh.r.vaidya@intel.com,
        Sudeep Holla <sudeep.holla@arm.com>
Subject: Re: [PATCH v1 8/9] regulator: keembay: Add regulator for Keem Bay SoC
Message-ID: <20210114171434.GI4854@sirena.org.uk>
References: <20210114152700.21916-1-muhammad.husaini.zulkifli@intel.com>
 <20210114152700.21916-9-muhammad.husaini.zulkifli@intel.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="1E1Oui4vdubnXi3o"
Content-Disposition: inline
In-Reply-To: <20210114152700.21916-9-muhammad.husaini.zulkifli@intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


--1E1Oui4vdubnXi3o
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

--1E1Oui4vdubnXi3o
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmAAe/kACgkQJNaLcl1U
h9Do4Af/RWczu73+RbTFDo9aNENM8BeoKxbtXH9eMaV1Ld1loIafX0ASjSYsJzW8
npIlmVMhfa0rdR3443Nd2AcCP8bu8y5QIkryte5XEac3nHy+BoLcxJLa8iUIdHqS
9REvzq4D7mwHyXQtangWa9gKnkJKZitOrOOxddj31DTQe4P1BZrje4qXcgfxKTsS
AB0EhRznmGJQxnssc+ANWs94IF7ixAKKPWBwhfrifKP4Z0GbMR5SjMbTGTEVsiMD
35k0h/EcBhl8/9mnHgO3Z8h7VpQ8hf4yIAFJwZNjEv1z52flc+v2o6HABWMts4VT
fNi1mU1wb/64EwoyDST2H6Ttk9cQQw==
=9PuY
-----END PGP SIGNATURE-----

--1E1Oui4vdubnXi3o--