* [PATCH v4] iio: sx9310: Support ACPI property
@ 2021-02-17 6:18 Gwendal Grignou
2021-02-25 19:57 ` Stephen Boyd
0 siblings, 1 reply; 2+ messages in thread
From: Gwendal Grignou @ 2021-02-17 6:18 UTC (permalink / raw)
To: jic23, lars, swboyd, campello; +Cc: linux-iio, Gwendal Grignou
Use device_property_read_... to support both device tree and ACPI
bindings.
Given |semtech,combined-sensors| is a variable array, use
device_property_count to get the length of the array first before
reading it to avoid underflow errors.
Add support for variable array per documentation
("iio/proximity/semtech,sx9310.yaml").
Fixes: 5b19ca2c78a0 ("iio: sx9310: Set various settings from DT")
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
Changes since v3:
Add "Fixes" comment in commit message
Fix the logic set COMBMODE register: when we know the DT property is
missing or incorrect, exit as soon as possible.
Changes since v2:
Add comment how the default array is used.
Add comment in commit message to indicate this CL fix an issue with
existing use of of_property_read_u32_array() when reading a variale
length array.
Changes since v1:
Use device_property_count_u32(...) instead of device_property_read_u32_array(..., NULL, 0)
drivers/iio/proximity/sx9310.c | 54 +++++++++++++++++++++-------------
1 file changed, 34 insertions(+), 20 deletions(-)
diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 6a04959df35e5..77d2c9e102842 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -20,6 +20,7 @@
#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/pm.h>
+#include <linux/property.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
#include <linux/slab.h>
@@ -1215,36 +1216,49 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev)
}
static const struct sx9310_reg_default *
-sx9310_get_default_reg(struct sx9310_data *data, int i,
+sx9310_get_default_reg(struct device *dev, int i,
struct sx9310_reg_default *reg_def)
{
- int ret;
- const struct device_node *np = data->client->dev.of_node;
- u32 combined[SX9310_NUM_CHANNELS] = { 4, 4, 4, 4 };
+ int ret, count;
+ u32 combined[SX9310_NUM_CHANNELS];
unsigned long comb_mask = 0;
const char *res;
u32 start = 0, raw = 0, pos = 0;
memcpy(reg_def, &sx9310_default_regs[i], sizeof(*reg_def));
- if (!np)
- return reg_def;
-
switch (reg_def->reg) {
case SX9310_REG_PROX_CTRL2:
- if (of_property_read_bool(np, "semtech,cs0-ground")) {
+ if (device_property_read_bool(dev, "semtech,cs0-ground")) {
reg_def->def &= ~SX9310_REG_PROX_CTRL2_SHIELDEN_MASK;
reg_def->def |= SX9310_REG_PROX_CTRL2_SHIELDEN_GROUND;
}
- reg_def->def &= ~SX9310_REG_PROX_CTRL2_COMBMODE_MASK;
- of_property_read_u32_array(np, "semtech,combined-sensors",
- combined, ARRAY_SIZE(combined));
- for (i = 0; i < ARRAY_SIZE(combined); i++) {
- if (combined[i] <= SX9310_NUM_CHANNELS)
- comb_mask |= BIT(combined[i]);
+ count = device_property_count_u32(dev, "semtech,combined-sensors");
+ if (count > 0 && count <= ARRAY_SIZE(combined)) {
+ ret = device_property_read_u32_array(dev,
+ "semtech,combined-sensors", combined,
+ count);
+ } else {
+ /*
+ * Either the property does not exist in the DT, the
+ * number of entries is incorrect.
+ */
+ break;
+ }
+ if (ret) {
+ /* We could not read the array (invalid DT). */
+ break;
}
- comb_mask &= 0xf;
+ for (i = 0; i < count; i++) {
+ if (combined[i] >= SX9310_NUM_CHANNELS) {
+ /* Invalid sensor (invalid DT). */
+ break;
+ }
+ comb_mask |= BIT(combined[i]);
+ }
+
+ reg_def->def &= ~SX9310_REG_PROX_CTRL2_COMBMODE_MASK;
if (comb_mask == (BIT(3) | BIT(2) | BIT(1) | BIT(0)))
reg_def->def |= SX9310_REG_PROX_CTRL2_COMBMODE_CS0_CS1_CS2_CS3;
else if (comb_mask == (BIT(1) | BIT(2)))
@@ -1256,7 +1270,7 @@ sx9310_get_default_reg(struct sx9310_data *data, int i,
break;
case SX9310_REG_PROX_CTRL4:
- ret = of_property_read_string(np, "semtech,resolution", &res);
+ ret = device_property_read_string(dev, "semtech,resolution", &res);
if (ret)
break;
@@ -1280,7 +1294,7 @@ sx9310_get_default_reg(struct sx9310_data *data, int i,
break;
case SX9310_REG_PROX_CTRL5:
- ret = of_property_read_u32(np, "semtech,startup-sensor", &start);
+ ret = device_property_read_u32(dev, "semtech,startup-sensor", &start);
if (ret) {
start = FIELD_GET(SX9310_REG_PROX_CTRL5_STARTUPSENS_MASK,
reg_def->def);
@@ -1290,7 +1304,7 @@ sx9310_get_default_reg(struct sx9310_data *data, int i,
reg_def->def |= FIELD_PREP(SX9310_REG_PROX_CTRL5_STARTUPSENS_MASK,
start);
- ret = of_property_read_u32(np, "semtech,proxraw-strength", &raw);
+ ret = device_property_read_u32(dev, "semtech,proxraw-strength", &raw);
if (ret) {
raw = FIELD_GET(SX9310_REG_PROX_CTRL5_RAWFILT_MASK,
reg_def->def);
@@ -1303,7 +1317,7 @@ sx9310_get_default_reg(struct sx9310_data *data, int i,
raw);
break;
case SX9310_REG_PROX_CTRL7:
- ret = of_property_read_u32(np, "semtech,avg-pos-strength", &pos);
+ ret = device_property_read_u32(dev, "semtech,avg-pos-strength", &pos);
if (ret)
break;
@@ -1339,7 +1353,7 @@ static int sx9310_init_device(struct iio_dev *indio_dev)
/* Program some sane defaults. */
for (i = 0; i < ARRAY_SIZE(sx9310_default_regs); i++) {
- initval = sx9310_get_default_reg(data, i, &tmp);
+ initval = sx9310_get_default_reg(&indio_dev->dev, i, &tmp);
ret = regmap_write(data->regmap, initval->reg, initval->def);
if (ret)
return ret;
--
2.30.0.478.g8a0d178c01-goog
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH v4] iio: sx9310: Support ACPI property
2021-02-17 6:18 [PATCH v4] iio: sx9310: Support ACPI property Gwendal Grignou
@ 2021-02-25 19:57 ` Stephen Boyd
0 siblings, 0 replies; 2+ messages in thread
From: Stephen Boyd @ 2021-02-25 19:57 UTC (permalink / raw)
To: Gwendal Grignou, campello, jic23, lars; +Cc: linux-iio, Gwendal Grignou
Quoting Gwendal Grignou (2021-02-16 22:18:29)
> Use device_property_read_... to support both device tree and ACPI
> bindings.
> Given |semtech,combined-sensors| is a variable array, use
> device_property_count to get the length of the array first before
> reading it to avoid underflow errors.
>
> Add support for variable array per documentation
> ("iio/proximity/semtech,sx9310.yaml").
>
> Fixes: 5b19ca2c78a0 ("iio: sx9310: Set various settings from DT")
>
Usually there isn't a newline here and Fixes is attached to the SoB.
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
> Changes since v3:
> Add "Fixes" comment in commit message
> Fix the logic set COMBMODE register: when we know the DT property is
> missing or incorrect, exit as soon as possible.
I'd still prefer it to be two patches, one for the fix of array property
parsing to send back to stable trees and one to add the ACPI parsing
support, but I'm not the maintainer here so this isn't really up to me.
> Changes since v2:
> Add comment how the default array is used.
> Add comment in commit message to indicate this CL fix an issue with
> existing use of of_property_read_u32_array() when reading a variale
> length array.
> Changes since v1:
> Use device_property_count_u32(...) instead of device_property_read_u32_array(..., NULL, 0)
>
> drivers/iio/proximity/sx9310.c | 54 +++++++++++++++++++++-------------
> 1 file changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> index 6a04959df35e5..77d2c9e102842 100644
> --- a/drivers/iio/proximity/sx9310.c
> +++ b/drivers/iio/proximity/sx9310.c
> @@ -20,6 +20,7 @@
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
> #include <linux/pm.h>
> +#include <linux/property.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> @@ -1215,36 +1216,49 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev)
> }
>
> static const struct sx9310_reg_default *
> -sx9310_get_default_reg(struct sx9310_data *data, int i,
> +sx9310_get_default_reg(struct device *dev, int i,
> struct sx9310_reg_default *reg_def)
> {
> - int ret;
> - const struct device_node *np = data->client->dev.of_node;
> - u32 combined[SX9310_NUM_CHANNELS] = { 4, 4, 4, 4 };
> + int ret, count;
> + u32 combined[SX9310_NUM_CHANNELS];
> unsigned long comb_mask = 0;
> const char *res;
> u32 start = 0, raw = 0, pos = 0;
>
> memcpy(reg_def, &sx9310_default_regs[i], sizeof(*reg_def));
> - if (!np)
> - return reg_def;
> -
> switch (reg_def->reg) {
> case SX9310_REG_PROX_CTRL2:
> - if (of_property_read_bool(np, "semtech,cs0-ground")) {
> + if (device_property_read_bool(dev, "semtech,cs0-ground")) {
> reg_def->def &= ~SX9310_REG_PROX_CTRL2_SHIELDEN_MASK;
> reg_def->def |= SX9310_REG_PROX_CTRL2_SHIELDEN_GROUND;
> }
>
> - reg_def->def &= ~SX9310_REG_PROX_CTRL2_COMBMODE_MASK;
> - of_property_read_u32_array(np, "semtech,combined-sensors",
> - combined, ARRAY_SIZE(combined));
> - for (i = 0; i < ARRAY_SIZE(combined); i++) {
> - if (combined[i] <= SX9310_NUM_CHANNELS)
> - comb_mask |= BIT(combined[i]);
> + count = device_property_count_u32(dev, "semtech,combined-sensors");
> + if (count > 0 && count <= ARRAY_SIZE(combined)) {
> + ret = device_property_read_u32_array(dev,
> + "semtech,combined-sensors", combined,
> + count);
> + } else {
> + /*
> + * Either the property does not exist in the DT, the
> + * number of entries is incorrect.
> + */
> + break;
> + }
> + if (ret) {
> + /* We could not read the array (invalid DT). */
> + break;
> }
Wouldn't this be shorter?
count = device_property_count_u32(dev, "semtech,combined-sensors");
if (count < 0 || count > ARRAY_SIZE(combined))
break;
ret = device_property_read_u32_array(dev,
"semtech,combined-sensors",
combined, count);
if (ret)
break;
If the 'break' logic is off-putting then I suggest moving it to another
function and using 'return' to indicate early returns from the code
flow.
>
> - comb_mask &= 0xf;
> + for (i = 0; i < count; i++) {
> + if (combined[i] >= SX9310_NUM_CHANNELS) {
> + /* Invalid sensor (invalid DT). */
> + break;
Don't think we need to do this. We have DT validation for this instead
so that we don't have to carry code in the kernel to validate something
that can be checked at build time. Hopefully ACPI has something similar?
> + }
> + comb_mask |= BIT(combined[i]);
> + }
> +
> + reg_def->def &= ~SX9310_REG_PROX_CTRL2_COMBMODE_MASK;
> if (comb_mask == (BIT(3) | BIT(2) | BIT(1) | BIT(0)))
> reg_def->def |= SX9310_REG_PROX_CTRL2_COMBMODE_CS0_CS1_CS2_CS3;
> else if (comb_mask == (BIT(1) | BIT(2)))
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-02-25 19:59 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-17 6:18 [PATCH v4] iio: sx9310: Support ACPI property Gwendal Grignou
2021-02-25 19:57 ` Stephen Boyd
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).