Message-ID: <a19261928f6563ffce6f1d0dadef77a9@gmail.com>
Date: Fri, 15 Nov 2024 13:12:59 +0100
From: Matteo Martelli <matteomartelli3@...il.com>
To: victor.duicu@...rochip.com, jic23@...nel.org, lars@...afoo.de,
	andy.shevchenko@...il.com
Cc: marius.cristea@...rochip.com, victor.duicu@...rochip.com,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v12] iio: adc: pac1921: Add ACPI support to Microchip
 pac1921

Hi Victor,

Below just a few very minor styling points. Sorry I didn't notice them
before. However, I am also fine with this version as it is.

Acked-by: Matteo Martelli <matteomartelli3@...il.com>

On Fri, 15 Nov 2024 12:07:56 +0200, <victor.duicu@...rochip.com> wrote:
> From: Victor Duicu <victor.duicu@...rochip.com>
> 
> This patch implements ACPI support to Microchip pac1921.
> The driver can read the shunt resistor value and label from the ACPI table.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
> Signed-off-by: Victor Duicu <victor.duicu@...rochip.com>
> ---
> 
> The patch was tested on minnowboard and sama5.
> 
> Differences related to previous versions:
> v12:
> - in pac1921_match_acpi_device reformat the instructions that
>   were over 80 characters width.
> - remove PAC1921_MAX_SHUNT_VALUE_uOHMS. Now the maximum acceptable value
>   of shunt resistor is INT_MAX. In pac1921_write_shunt_resistor
>   change the shunt check to respect the new limit and to avoid
>   overflow in val * MICRO.
> 
> v11:
> - credit Andy Shevchenko for code review.
> 
> v10:
> - fix coding style mistakes.
> - add UL to PAC1921_MAX_SHUNT_VALUE_uOHMS.
> - edit comment in pac1921_write_shunt_resistor.
> - in pac1921_probe use is_acpi_device_node instead of
>   ACPI_HANDLE.
> 
> v9:
> - put limits.h in sorted order.
> - remove guid_parse and implement GUID_INIT.
> - remove pac1921_shunt_is_valid.
> - change maximum acceptable value of shunt resistor to 2146.999999.
>   This change was made so the readings for dt and ACPI share
>   the same range.
>   With this value the maximum current that can be read is
>   0.1V / 2146.999999 = 46.576 uA
>   If we use INT_MAX the maximum current is
>   0.1V / 2147.483647 = 46.566 uA
>   The relative error between the two is 0.0214, so it is
>   small enough to allow for code simplicity.
>   A shunt value over a few hundred Ohms is quite unusual.
> 
> v8:
> - fix multiple coding style errors.
> - in pac1921_match_acpi_device change error type to ENOMEM
>   at label is NULL branch.
> - in pac1921_match_acpi_device when reading label,
>   change accesing method of string.
> - change name of PAC1921_ACPI_GET_UOHMS_VALS to
>   PAC1921_ACPI_GET_uOHMS_VALS.
> - add limits.h in include list.
> - change integer constant in PAC1921_MAX_SHUNT_VALUE_OHMS
>   to INT_MAX / MICRO.
> - change pac1921_shunt_is_invalid to pac1921_shunt_is_valid.
> - in pac1921_match_acpi_device change name of variable rez to status.
> 
> v7:
> - in pac1921_shunt_is_invalid remove brackets in return.
> - in pac1921_match_acpi_device and pac1921_parse_of_fw move checking of
>   shunt value and scale calculation to pac1921_probe.
> - in pac1921_match_acpi_device change devm_kmemdup to devm_kstrdup
>   and add label check for NULL.
> - in pac1921_match_acpi_device and pac1921_parse_of_fw remove unnecessary
>   entry arguments. Now indio_dev is the only entry argument.
> - in pac1921_probe, pac1921_match_acpi_device and pac1921_parse_of_fw
>   standardised structure accesing.
> 
> v6:
> - set maximum acceptable value of shunt resistor to INT_MAX UOHMS
>   in devicetree, ACPI table and user input.
> - in pac1921_match_acpi_device remove temp variable.
> 
> v5:
> - set maximum acceptable value of shunt resistor to 2KOHM in devicetree,
>   ACPI table and user input. The chosen value is lesser than INT_MAX,
>   which is about 2.1KOHM.
> - in pac1921_match_acpi_device and pac1921_parse_of_fw change to only
>   read 32b values for resistor shunt.
> 
> v4:
> - change name of pac1921_shunt_is_valid to pac1921_shunt_is_invalid.
> - fix coding style.
> - in pac1921_parse_of_fw change back to device_property_read_u32.
> 
> v3:
> - simplify and make inline function pac1921_shunt_is_valid. Make argument u64.
> - fix link to DSM documentation.
> - in pac1921_match_acpi_device and pac1921_parse_of_fw, the shunt value is
>   read as u64.
> - in pac1921_parse_of_fw remove code for reading label value from
>   devicetree.
> - in pac1921_write_shunt_resistor cast the multiply result to u64 in order
>   to fix overflow.
> 
> v2:
> - remove name variable from priv. Driver reads label attribute with
>   sysfs.
> - define pac1921_shunt_is_valid function.
> - move default assignments in pac1921_probe to original position.
> - roll back coding style changes.
> - add documentation for DSM(the linked document was used as reference).
> - remove acpi_match_device in pac1921_match_acpi_device.
> - remove unnecessary null assignment and comment.
> - change name of function pac1921_match_of_device to
>   pac1921_parse_of_fw.
> 
> v1:
> - initial version for review.
> 
>  drivers/iio/adc/pac1921.c | 100 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 90 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> index b0f6727cfe38..a78ae34057a1 100644
> --- a/drivers/iio/adc/pac1921.c
> +++ b/drivers/iio/adc/pac1921.c
> @@ -12,6 +12,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/limits.h>
>  #include <linux/regmap.h>
>  #include <linux/units.h>
>  
> @@ -67,6 +68,14 @@ enum pac1921_mxsl {
>  #define PAC1921_DEFAULT_DI_GAIN		0 /* 2^(value): 1x gain (HW default) */
>  #define PAC1921_DEFAULT_NUM_SAMPLES	0 /* 2^(value): 1 sample (HW default) */
>  
> +#define PAC1921_ACPI_GET_uOHMS_VALS             0
> +#define PAC1921_ACPI_GET_LABEL			1
> +
> +/* f7bb9932-86ee-4516-a236-7a7a742e55cb */
> +static const guid_t pac1921_guid =
> +			GUID_INIT(0xf7bb9932, 0x86ee, 0x4516, 0xa2,
> +				  0x36, 0x7a, 0x7a, 0x74, 0x2e, 0x55, 0xcb);
> +
>  /*
>   * Pre-computed scale factors for BUS voltage
>   * format: IIO_VAL_INT_PLUS_NANO
> @@ -782,7 +791,7 @@ static ssize_t pac1921_write_shunt_resistor(struct iio_dev *indio_dev,
>  					    const char *buf, size_t len)
>  {
>  	struct pac1921_priv *priv = iio_priv(indio_dev);
> -	u64 rshunt_uohm;
> +	u32 rshunt_uohm;
>  	int val, val_fract;
>  	int ret;
>  
> @@ -793,10 +802,17 @@ static ssize_t pac1921_write_shunt_resistor(struct iio_dev *indio_dev,
>  	if (ret)
>  		return ret;
>  
> -	rshunt_uohm = val * MICRO + val_fract;
> -	if (rshunt_uohm == 0 || rshunt_uohm > INT_MAX)
> +	/*
> +	 * This check validates the shunt is not zero and does not surpass
> +	 * INT_MAX. The check is done before calculating in order to avoid
> +	 * val * MICRO overflowing.
> +	 */
> +	if ((!val && !val_fract) || (val > INT_MAX / MICRO) ||
> +	    ((val == INT_MAX / MICRO) && (val_fract > INT_MAX % MICRO)))
>  		return -EINVAL;

Can the unnecessary parenthesis be removed?

	if ((!val && !val_fract) || (val > INT_MAX / MICRO) ||
                                    ^.....................^
	    ((val == INT_MAX / MICRO) && (val_fract > INT_MAX % MICRO)))
             ^......................^    ^...........................^

>  
> +	rshunt_uohm = val * MICRO + val_fract;
> +
>  	guard(mutex)(&priv->lock);
>  
>  	priv->rshunt_uohm = rshunt_uohm;
> @@ -1151,6 +1167,61 @@ static void pac1921_regulator_disable(void *data)
>  	regulator_disable(regulator);
>  }
>  
> +/*
> + * Documentation related to the ACPI device definition
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ApplicationNotes/ApplicationNotes/PAC193X-Integration-Notes-for-Microsoft-Windows-10-and-Windows-11-Driver-Support-DS00002534.pdf
> + */
> +static int pac1921_match_acpi_device(struct iio_dev *indio_dev)
> +{
> +	acpi_handle handle;
> +	union acpi_object *status;
> +	char *label;
> +	struct pac1921_priv *priv = iio_priv(indio_dev);
> +	struct device *dev = &priv->client->dev;
> +
> +	handle = ACPI_HANDLE(dev);
> +
> +	status = acpi_evaluate_dsm(handle, &pac1921_guid, 1,
> +				   PAC1921_ACPI_GET_uOHMS_VALS, NULL);
> +	if (!status)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Could not read shunt from ACPI table\n");
> +
> +	priv->rshunt_uohm = status->package.elements[0].integer.value;
> +	ACPI_FREE(status);
> +
> +	status = acpi_evaluate_dsm(handle, &pac1921_guid, 1,
> +				   PAC1921_ACPI_GET_LABEL, NULL);
> +	if (!status)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Could not read label from ACPI table\n");
> +
> +	label = devm_kstrdup(dev, status->package.elements[0].string.pointer,
> +			     GFP_KERNEL);
> +	if (!label)
> +		return -ENOMEM;
> +
> +	indio_dev->label = label;
> +	ACPI_FREE(status);
> +
> +	return 0;
> +}
> +
> +static int pac1921_parse_of_fw(struct iio_dev *indio_dev)
> +{
> +	int ret;
> +	struct pac1921_priv *priv = iio_priv(indio_dev);
> +	struct device *dev = &priv->client->dev;
> +
> +	ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> +				       &priv->rshunt_uohm);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Cannot read shunt resistor property\n");
> +
> +	return 0;
> +}
> +
>  static int pac1921_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -1179,14 +1250,16 @@ static int pac1921_probe(struct i2c_client *client)
>  	priv->di_gain = PAC1921_DEFAULT_DI_GAIN;
>  	priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES;
>  
> -	ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> -				       &priv->rshunt_uohm);
> -	if (ret)
> +	if (is_acpi_device_node(dev->fwnode))
> +		ret = pac1921_match_acpi_device(indio_dev);
> +	else
> +		ret = pac1921_parse_of_fw(indio_dev);
> +	if (ret < 0)

For consistency: if (ret) instead of if (ret < 0)

>  		return dev_err_probe(dev, ret,
> -				     "Cannot read shunt resistor property\n");
> -	if (priv->rshunt_uohm == 0 || priv->rshunt_uohm > INT_MAX)
> -		return dev_err_probe(dev, -EINVAL,
> -				     "Invalid shunt resistor: %u\n",
> +				     "Parameter parsing error\n");
> +
> +	if (!priv->rshunt_uohm || priv->rshunt_uohm > INT_MAX)
> +		return dev_err_probe(dev, -EINVAL, "Invalid shunt resistor: %u\n",

The error string in a new line to keep it 80 char wide.

>  				     priv->rshunt_uohm);
>  
>  	pac1921_calc_current_scales(priv);
> @@ -1246,11 +1319,18 @@ static const struct of_device_id pac1921_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, pac1921_of_match);
>  
> +static const struct acpi_device_id pac1921_acpi_match[] = {
> +	{ "MCHP1921" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, pac1921_acpi_match);
> +
>  static struct i2c_driver pac1921_driver = {
>  	.driver	 = {
>  		.name = "pac1921",
>  		.pm = pm_sleep_ptr(&pac1921_pm_ops),
>  		.of_match_table = pac1921_of_match,
> +		.acpi_match_table = pac1921_acpi_match,
>  	},
>  	.probe = pac1921_probe,
>  	.id_table = pac1921_id,
> 
> base-commit: 20fd1383cd616d61b2a79967da1221dc6cfb8430
> -- 
> 2.43.0
> 

Thanks,
Matteo Martelli

Powered by blists - more mailing lists