On Thu, 24 Oct 2024 13:00:50 +0300
<victor.duicu@xxxxxxxxxxxxx> wrote:
> From: Victor Duicu <victor.duicu@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
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);
