Re: [PATCH v6] iio: adc: pac1921: Add ACPI support to Microchip pac1921

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 24 Oct 2024 13:00:50 +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.
> 
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
Hi Victor,

Been a while since I looked at a version of this. Some comments inline
on stuff that I probably missed before now.

Jonathan

>  };
>  
> +static inline bool pac1921_shunt_is_invalid(u32 shunt_val)
> +{
> +	return (shunt_val == 0 || shunt_val > INT_MAX);

Drop the brackets as they don't add anything much.

> +}
> +

> +/*
> + * 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 i2c_client *client, struct pac1921_priv *priv,
> +				     struct iio_dev *indio_dev)
> +{
> +	acpi_handle handle;
> +	union acpi_object *rez;
> +	guid_t guid;
> +	char *label;
> +
> +	guid_parse(PAC1921_DSM_UUID, &guid);
> +	handle = ACPI_HANDLE(&client->dev);
> +
> +	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_UOHMS_VALS, NULL);
> +	if (!rez)
> +		return dev_err_probe(&client->dev, -EINVAL,
> +				     "Could not read shunt from ACPI table\n");
> +
> +	priv->rshunt_uohm = rez->package.elements[0].integer.value;
> +	ACPI_FREE(rez);
> +
> +	if (pac1921_shunt_is_invalid(priv->rshunt_uohm))
> +		return dev_err_probe(&client->dev, -EINVAL, "Invalid shunt resistor\n");
> +
> +	pac1921_calc_current_scales(priv);

As below - I'd do this and the validity check outside of this function to avoid
duplication for the two firmware types.

> +
> +	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_LABEL, NULL);
> +	if (!rez)
> +		return dev_err_probe(&client->dev, -EINVAL,
> +				     "Could not read label from ACPI table\n");
> +
> +	label = devm_kmemdup(&client->dev, rez->package.elements->string.pointer,
> +			     (size_t)rez->package.elements->string.length + 1,
Are you duplicating one off the of the string?
I'm curious at the need for a null.

Consider devm_kstrdup() which only copies the actual string + adds the terminator.

+ check for allocation failure label may be NULL.

> +			     GFP_KERNEL);
> +	label[rez->package.elements->string.length] = '\0';
> +	indio_dev->label = label;
> +	ACPI_FREE(rez);
> +
> +	return 0;
> +}
> +
> +static int pac1921_parse_of_fw(struct i2c_client *client, struct pac1921_priv *priv)
> +{
> +	int ret;
> +	struct device *dev = &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");
> +
> +	if (pac1921_shunt_is_invalid(priv->rshunt_uohm))
> +		return dev_err_probe(dev, -EINVAL, "Invalid shunt resistor: %u\n",
> +				     priv->rshunt_uohm);
> +
> +	pac1921_calc_current_scales(priv);

Why not do the sanity checks and scale calc outside of the firmware specific code
given it is duplicated and not related to the firmware parsing itself.


> +
> +	return 0;
> +}
> +
>  static int pac1921_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -1176,17 +1254,13 @@ 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)
> -		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",
> -				     priv->rshunt_uohm);
> -
> -	pac1921_calc_current_scales(priv);
> +	if (ACPI_HANDLE(&client->dev))
> +		ret = pac1921_match_acpi_device(client, priv, indio_dev);

priv is trivial to get from the indio_dev, so don't pass them both in.
Client also trivial to get from there, so just pass in the indio_dev to both
calls.


> +	else
> +		ret = pac1921_parse_of_fw(client, priv);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
Use dev as it is the same device.

> +				     "parameter parsing error\n");
>  
>  	priv->vdd = devm_regulator_get(dev, "vdd");
>  	if (IS_ERR(priv->vdd))
> @@ -1243,11 +1317,17 @@ static const struct of_device_id pac1921_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, pac1921_of_match);