Quoting victor.duicu@xxxxxxxxxxxxx (2024-09-25 17:08:56)
> From: Victor Duicu <victor.duicu@xxxxxxxxxxxxx>
>
> This patch implements ACPI support for pac1921.
> Driver can read shunt resistor value and device name from ACPI table.
>
> Signed-off-by: Victor Duicu <victor.duicu@xxxxxxxxxxxxx>
>
Hello Victor, thanks for the patch.
I am not going to address the specific ACPI parsing as I don't have much
experience with it yet, but I have some other comments (see below).
> ---
> drivers/iio/adc/pac1921.c | 116 +++++++++++++++++++++++++++++++-------
> 1 file changed, 97 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> index 4c2a1c07bc39..9bb61b88aaef 100644
> --- a/drivers/iio/adc/pac1921.c
> +++ b/drivers/iio/adc/pac1921.c
> @@ -67,6 +67,10 @@ 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_NAME 1
> +#define PAC1921_DSM_UUID "f7bb9932-86ee-4516-a236-7a7a742e55cb"
> +
> /*
> * Pre-computed scale factors for BUS voltage
> * format: IIO_VAL_INT_PLUS_NANO
> @@ -190,6 +194,7 @@ struct pac1921_priv {
> u8 n_samples;
> u8 prev_ovf_flags;
> u8 ovf_enabled_events;
> + char *name;
I think what you are assigning to "name" later in the patch would better
fit in a "label" sysfs attribute. iio core already checks if a "label"
DT property [1] is present and exposes it via sysfs. When not using a DT I
think you can just set the parsed name from ACPI into indio_dev->label.
Also, if you pass indio_dev to the ACPI match function you won't even
need a variable in pac1921_priv for that purpose. See for example
drivers/iio/accel/kxcjk-1013.c:kxcjk1013_match_acpi_device or
drivers/iio/accel/bmc150-accel-core.c:bmc150_apply_bosc0200_acpi_orientation.
>
> bool first_integr_started;
> bool first_integr_done;
> @@ -1151,6 +1156,79 @@ static void pac1921_regulator_disable(void *data)
> regulator_disable(regulator);
> }
>
> +static int pac1921_match_acpi_device(struct i2c_client *client, struct pac1921_priv *priv)
> +{
> + const struct acpi_device_id *device_pointer;
> + acpi_handle handle;
> + union acpi_object *rez;
> + guid_t guid;
> + struct device *dev = &client->dev;
> +
> + guid_parse(PAC1921_DSM_UUID, &guid);
> + handle = ACPI_HANDLE(&client->dev);
> +
> + device_pointer = acpi_match_device(dev->driver->acpi_match_table, dev);
> + if (!device_pointer)
> + return -EINVAL;
> +
> + 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 (priv->rshunt_uohm == 0)
> + return dev_err_probe(&client->dev, -EINVAL, "Shunt value is 0.");
Calculation of current scales assumes rshunt_uohm to be inside signed
int boundaries, so it should be checked rshunt_uohm doesn't exceed
INT_MAX. If there is any reason for the shunt resistor to be bigger than
INT_MAX (2.1KOhm) than the scales calculation must be reconsidered as
well.
Also, given that the check would be around three different points it
could also be put in an inline function or macro, something like
pac1921_shunt_is_valid(), or even into a common wrapper that validates,
set the shunt and update the current scales. But that could also be a
refactoring for a future patch.
> +
> + pac1921_calc_current_scales(priv);
> +
> + rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_NAME, NULL);
> + if (!rez) {
> + priv->name = "";
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Could not read name from ACPI table\n");
> + }
> +
> + priv->name = devm_kmemdup(&client->dev, rez->package.elements->string.pointer,
> + (size_t)rez->package.elements->string.length + 1,
> + GFP_KERNEL);
> + priv->name[rez->package.elements->string.length] = '\0';
As mentioned above, such name should likely be set to into indio_dev->label.
> + ACPI_FREE(rez);
> +
> + return 0;
> +}
> +
> +static int pac1921_match_of_device(struct i2c_client *client, struct pac1921_priv *priv)
> +{
> + int ret;
> + struct device *dev = &client->dev;
> +
> + /* Read shunt resistor value */
> + 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 (device_property_present(dev, "name")) {
> + ret = device_property_read_string(dev, "name", (const char **)&priv->name);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Invalid rail-name value\n");
> + } else {
> + priv->name = "pac1921";
> + }
> +
Parsing of the "name" property should likely be removed, the use case
should already be covered by the DT label property [1].
> + return 0;
> +}
> +
> static int pac1921_probe(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
> @@ -1172,22 +1250,14 @@ static int pac1921_probe(struct i2c_client *client)
> "Cannot initialize register map\n");
>
> devm_mutex_init(dev, &priv->lock);
> + if (ACPI_HANDLE(&client->dev))
> + ret = pac1921_match_acpi_device(client, priv);
> + else
> + ret = pac1921_match_of_device(client, priv);
>
> - priv->dv_gain = PAC1921_DEFAULT_DV_GAIN;
> - priv->di_gain = PAC1921_DEFAULT_DI_GAIN;
> - priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES;
Is there any reason to move these default assignments later in the
function?
> -
> - 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 (ret < 0)
> + return dev_err_probe(&client->dev, ret,
> + "parameter parsing error\n");
>
> priv->vdd = devm_regulator_get(dev, "vdd");
> if (IS_ERR(priv->vdd))
> @@ -1198,13 +1268,15 @@ static int pac1921_probe(struct i2c_client *client)
> if (ret)
> return dev_err_probe(dev, ret, "Cannot enable vdd regulator\n");
>
> - ret = devm_add_action_or_reset(dev, pac1921_regulator_disable,
> - priv->vdd);
> + ret = devm_add_action_or_reset(dev, pac1921_regulator_disable, priv->vdd);
I would leave coding style changes out from this patch.
> if (ret)
> return dev_err_probe(dev, ret,
> - "Cannot add action for vdd regulator disposal\n");
> + "Cannot add action for vdd regulator disposal\n");
Also here, I would leave coding style changes out from this patch.
>
> msleep(PAC1921_POWERUP_TIME_MS);
> + priv->dv_gain = PAC1921_DEFAULT_DV_GAIN;
> + priv->di_gain = PAC1921_DEFAULT_DI_GAIN;
> + priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES;
Can these be moved back at the top of the function?
>
> ret = pac1921_init(priv);
> if (ret)
> @@ -1212,7 +1284,7 @@ static int pac1921_probe(struct i2c_client *client)
>
> priv->iio_info = pac1921_iio;
>
> - indio_dev->name = "pac1921";
> + indio_dev->name = priv->name;
This should likely be removed for the reasons mentioned above.
> indio_dev->info = &priv->iio_info;
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->channels = pac1921_channels;
> @@ -1244,11 +1316,17 @@ 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: fec496684388685647652ab4213454fbabdab099
> --
> 2.43.0
>
[1]: https://github.com/devicetree-org/dt-schema/blob/c51125d571cac9596048e888a856d70650e400e0/dtschema/schemas/iio/iio.yaml#L39
Best regards,
Matteo Martelli
