drivers/iio/adc/pac1921.c | 116 +++++++++++++++++++++++++++++++------- 1 file changed, 97 insertions(+), 19 deletions(-)
From: Victor Duicu <victor.duicu@microchip.com>
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@microchip.com>
---
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;
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.");
+
+ 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';
+ 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";
+ }
+
+ 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;
-
- 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);
if (ret)
return dev_err_probe(dev, ret,
- "Cannot add action for vdd regulator disposal\n");
+ "Cannot add action for vdd regulator disposal\n");
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;
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;
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.0On Wed, 25 Sep 2024 18:08:56 +0300
<victor.duicu@microchip.com> wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> 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@microchip.com>
Hi Victor.
Comments inline on top of what Matteo already provided.
Thanks,
Jonathan
If there is documentation for the DSM etc here available, then please
add a comment with a link to it.
> +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);
check the fw type rather than doing this.
The ACPI_HANDLE() check you have below should be sufficient already.
> + 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.");
Say why it is an error. "Shunt value cannot be 0\n");
Note also the new line.
> +
> + pac1921_calc_current_scales(priv);
> +
> + rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_NAME, NULL);
> + if (!rez) {
> + priv->name = "";
Not a huge point in clearing this if you are going to error out anyway.
I woudn't bother doing so.
> + 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';
> + ACPI_FREE(rez);
> +
> + return 0;
> +}
> +
> +static int pac1921_match_of_device(struct i2c_client *client, struct pac1921_priv *priv)
This isn't matching the device, it's reading parameters from fw.
pac1921_parse_of_fw() or something like that for the function name.
> +{
> + int ret;
> + struct device *dev = &client->dev;
> +
> + /* Read shunt resistor value */
Don't put comments in that are obvious from the code.
Comments have a nasty habit of bit rotting (people fail to keep them up to
date) so to reduce chances of that we only use them when they add information
not obvious from the code.
> + ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms", &priv->rshunt_uohm);
When it doesn't hurt readability, please try to stay under 80 chars.
> + 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);
This is a new thing for the of binding so should have been in a precursor patch and included
DT binding documentation update. + as per other discussion use label which
will just work out of the box.
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Invalid rail-name value\n");
> + } else {
> + priv->name = "pac1921";
> + }
> +
> + 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;
> -
> - 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);
As Matteo stated, formatting changes don't belong in a patch making
more substantial changes such as this. If you think they
should be done, then separate patch so we can assess them without
them adding noise to the real work here.
> if (ret)
> return dev_err_probe(dev, ret,
> - "Cannot add action for vdd regulator disposal\n");
> + "Cannot add action for vdd regulator disposal\n");
>
> 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;
>
> 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 name should remain the part number. As Matteo commented we have
label for more board specific information such as distinguishing between
instances by easy to understand naming.
> 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" },
That's a valid ID for once which is good. I'll assume Microchip
has a process for tracking these and avoiding clashes.
> + { }
> +};
> +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
On Sat, 2024-09-28 at 17:42 +0100, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Wed, 25 Sep 2024 18:08:56 +0300
> <victor.duicu@microchip.com> wrote:
>
> > From: Victor Duicu <victor.duicu@microchip.com>
> >
> >
> > MODULE_DEVICE_TABLE(of, pac1921_of_match);
> >
> > +static const struct acpi_device_id pac1921_acpi_match[] = {
> > + { "MCHP1921" },
>
> That's a valid ID for once which is good. I'll assume Microchip
> has a process for tracking these and avoiding clashes.
>
MCHP id is registered with UEFI and MCHP1921 is tracked internally.
> > + { }
> > +};
> > +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
>
Quoting victor.duicu@microchip.com (2024-09-25 17:08:56)
> From: Victor Duicu <victor.duicu@microchip.com>
>
> 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@microchip.com>
>
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
© 2016 - 2024 Red Hat, Inc.