Re: [PATCH v3] iio: adc: pac1921: add ACPI support to Microchip pac1921.

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 11 Oct 2024 16:44:54 +0300
<victor.duicu@microchip.com> wrote:

> From: Victor Duicu <victor.duicu@microchip.com>
> 
> This patch implements ACPI support to Microchip pac1921.
> The driver can read shunt resistor value and label from ACPI table.
> 
> The patch was tested on a minnowboard(64b) and sama5(32b).
> In order to avoid overflow when reading 64b values from ACPi table or
> devicetree it is necessary:
> - the revision of .dsl file must be 2 or greater to enable 64b arithmetic.
> - the shunt resistor variable in devicetree must have the prefix "/bits/ 64".
> 

Change log should be below the --- as it doesn't want to end up in the git history.


> Differences related to previous versions:
> 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.
> 
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
A few comments in line.

J

 int pac1921_parse_of_fw(struct i2c_client *client, struct pac1921_priv *priv,
> +			       struct iio_dev *indio_dev)
> +{
> +	int ret;
> +	struct device *dev = &client->dev;
> +	u64 temp;
> +
> +	ret = device_property_read_u64(dev, "shunt-resistor-micro-ohms", &temp);
> +
No line break here.  Want to associate that error check with the line above
as clearly as possible.

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Cannot read shunt resistor property\n");
> +
> +	if (pac1921_shunt_is_valid(temp))
> +		return dev_err_probe(dev, -EINVAL, "Invalid shunt resistor: %u\n",
> +				     priv->rshunt_uohm);
> +
> +	priv->rshunt_uohm = (u32)temp;
> +	pac1921_calc_current_scales(priv);
> +
> +	return 0;
> +}