Unlike the other AD719Xs, AD7194 has configurable channels. The user can
dynamically configure them in the devicetree.
Add sigma_delta_info member to chip_info structure. Since AD7194 is the
only chip that has no channel sequencer, num_slots should remain
undefined.
Also modify config AD7192 description for better scaling.
Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
drivers/iio/adc/Kconfig | 11 ++-
drivers/iio/adc/ad7192.c | 147 +++++++++++++++++++++++++++++++++++++--
2 files changed, 150 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..74fecc284f1a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -88,12 +88,17 @@ config AD7173
called ad7173.
config AD7192
- tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
+ tristate "Analog Devices AD7192 and similar ADC driver"
depends on SPI
select AD_SIGMA_DELTA
help
- Say yes here to build support for Analog Devices AD7190,
- AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC).
+ Say yes here to build support for Analog Devices SPI analog to digital
+ converters (ADC):
+ - AD7190
+ - AD7192
+ - AD7193
+ - AD7194
+ - AD7195
If unsure, say N (but it's safe to say "Y").
To compile this driver as a module, choose M here: the
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 7160929d32c9..fe2d8d55fa76 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * AD7190 AD7192 AD7193 AD7195 SPI ADC driver
+ * AD7192 and similar SPI ADC driver
*
* Copyright 2011-2015 Analog Devices Inc.
*/
@@ -129,10 +129,22 @@
#define AD7193_CH_AIN8 0x480 /* AIN7 - AINCOM */
#define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */
+#define AD7194_CH_POS(x) (((x) - 1) << 4)
+#define AD7194_CH_NEG(x) ((x) - 1)
+#define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
+ /* 10th bit corresponds to CON18(Pseudo) */
+#define AD7194_DIFF_CH(p, n) (AD7194_CH_POS(p) | AD7194_CH_NEG(n))
+#define AD7194_CH_TEMP 0x100 /* Temp sensor */
+#define AD7194_CH_BASE_NR 2
+#define AD7194_CH_AIN_START 1
+#define AD7194_CH_AIN_NR 16
+#define AD7194_CH_MAX_NR 272
+
/* ID Register Bit Designations (AD7192_REG_ID) */
#define CHIPID_AD7190 0x4
#define CHIPID_AD7192 0x0
#define CHIPID_AD7193 0x2
+#define CHIPID_AD7194 0x3
#define CHIPID_AD7195 0x6
#define AD7192_ID_MASK GENMASK(3, 0)
@@ -170,6 +182,7 @@ enum {
ID_AD7190,
ID_AD7192,
ID_AD7193,
+ ID_AD7194,
ID_AD7195,
};
@@ -178,7 +191,9 @@ struct ad7192_chip_info {
const char *name;
const struct iio_chan_spec *channels;
u8 num_channels;
+ const struct ad_sigma_delta_info *sigma_delta_info;
const struct iio_info *info;
+ int (*parse_channels)(struct iio_dev *indio_dev);
};
struct ad7192_state {
@@ -346,6 +361,18 @@ static const struct ad_sigma_delta_info ad7192_sigma_delta_info = {
.irq_flags = IRQF_TRIGGER_FALLING,
};
+static const struct ad_sigma_delta_info ad7194_sigma_delta_info = {
+ .set_channel = ad7192_set_channel,
+ .append_status = ad7192_append_status,
+ .disable_all = ad7192_disable_all,
+ .set_mode = ad7192_set_mode,
+ .has_registers = true,
+ .addr_shift = 3,
+ .read_mask = BIT(6),
+ .status_ch_mask = GENMASK(3, 0),
+ .irq_flags = IRQF_TRIGGER_FALLING,
+};
+
static const struct ad_sd_calib_data ad7192_calib_arr[8] = {
{AD7192_MODE_CAL_INT_ZERO, AD7192_CH_AIN1},
{AD7192_MODE_CAL_INT_FULL, AD7192_CH_AIN1},
@@ -937,6 +964,14 @@ static const struct iio_info ad7192_info = {
.update_scan_mode = ad7192_update_scan_mode,
};
+static const struct iio_info ad7194_info = {
+ .read_raw = ad7192_read_raw,
+ .write_raw = ad7192_write_raw,
+ .write_raw_get_fmt = ad7192_write_raw_get_fmt,
+ .read_avail = ad7192_read_avail,
+ .validate_trigger = ad_sd_validate_trigger,
+};
+
static const struct iio_info ad7195_info = {
.read_raw = ad7192_read_raw,
.write_raw = ad7192_write_raw,
@@ -1028,12 +1063,96 @@ static const struct iio_chan_spec ad7193_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(14),
};
+static int ad7194_validate_ain_channel(struct device *dev, u32 ain)
+{
+ if (!in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
+ return dev_err_probe(dev, -EINVAL,
+ "Invalid AIN channel: %u\n", ain);
+
+ return 0;
+}
+
+static int ad7194_parse_channels(struct iio_dev *indio_dev)
+{
+ struct device *dev = indio_dev->dev.parent;
+ struct iio_chan_spec *ad7194_channels;
+ const struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0);
+ const struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0);
+ const struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0);
+ const struct iio_chan_spec ad7194_chan_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0);
+ unsigned int num_channels, index = 0;
+ u32 ain[2];
+ int ret;
+
+ num_channels = device_get_child_node_count(dev);
+ if (num_channels > AD7194_CH_MAX_NR)
+ return dev_err_probe(dev, -EINVAL,
+ "Too many channels: %u\n", num_channels);
+
+ num_channels += AD7194_CH_BASE_NR;
+
+ ad7194_channels = devm_kcalloc(dev, num_channels,
+ sizeof(*ad7194_channels), GFP_KERNEL);
+ if (!ad7194_channels)
+ return -ENOMEM;
+
+ indio_dev->channels = ad7194_channels;
+ indio_dev->num_channels = num_channels;
+
+ device_for_each_child_node_scoped(dev, child) {
+ ret = fwnode_property_read_u32_array(child, "diff-channels",
+ ain, ARRAY_SIZE(ain));
+ if (ret == 0) {
+ ret = ad7194_validate_ain_channel(dev, ain[0]);
+ if (ret)
+ return ret;
+
+ ret = ad7194_validate_ain_channel(dev, ain[1]);
+ if (ret)
+ return ret;
+
+ *ad7194_channels = ad7194_chan_diff;
+ ad7194_channels->scan_index = index++;
+ ad7194_channels->channel = ain[0];
+ ad7194_channels->channel2 = ain[1];
+ ad7194_channels->address = AD7194_DIFF_CH(ain[0], ain[1]);
+ } else {
+ ret = fwnode_property_read_u32(child, "single-channel",
+ &ain[0]);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Missing channel property\n");
+
+ ret = ad7194_validate_ain_channel(dev, ain[0]);
+ if (ret)
+ return ret;
+
+ *ad7194_channels = ad7194_chan;
+ ad7194_channels->scan_index = index++;
+ ad7194_channels->channel = ain[0];
+ ad7194_channels->address = AD7194_CH(ain[0]);
+ }
+ ad7194_channels++;
+ }
+
+ *ad7194_channels = ad7194_chan_temp;
+ ad7194_channels->scan_index = index++;
+ ad7194_channels->address = AD7194_CH_TEMP;
+ ad7194_channels++;
+
+ *ad7194_channels = ad7194_chan_timestamp;
+ ad7194_channels->scan_index = index;
+
+ return 0;
+}
+
static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
[ID_AD7190] = {
.chip_id = CHIPID_AD7190,
.name = "ad7190",
.channels = ad7192_channels,
.num_channels = ARRAY_SIZE(ad7192_channels),
+ .sigma_delta_info = &ad7192_sigma_delta_info,
.info = &ad7192_info,
},
[ID_AD7192] = {
@@ -1041,6 +1160,7 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
.name = "ad7192",
.channels = ad7192_channels,
.num_channels = ARRAY_SIZE(ad7192_channels),
+ .sigma_delta_info = &ad7192_sigma_delta_info,
.info = &ad7192_info,
},
[ID_AD7193] = {
@@ -1048,13 +1168,22 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
.name = "ad7193",
.channels = ad7193_channels,
.num_channels = ARRAY_SIZE(ad7193_channels),
+ .sigma_delta_info = &ad7192_sigma_delta_info,
.info = &ad7192_info,
},
+ [ID_AD7194] = {
+ .chip_id = CHIPID_AD7194,
+ .name = "ad7194",
+ .info = &ad7194_info,
+ .sigma_delta_info = &ad7194_sigma_delta_info,
+ .parse_channels = ad7194_parse_channels,
+ },
[ID_AD7195] = {
.chip_id = CHIPID_AD7195,
.name = "ad7195",
.channels = ad7192_channels,
.num_channels = ARRAY_SIZE(ad7192_channels),
+ .sigma_delta_info = &ad7192_sigma_delta_info,
.info = &ad7195_info,
},
};
@@ -1161,11 +1290,17 @@ static int ad7192_probe(struct spi_device *spi)
st->chip_info = spi_get_device_match_data(spi);
indio_dev->name = st->chip_info->name;
indio_dev->modes = INDIO_DIRECT_MODE;
- indio_dev->channels = st->chip_info->channels;
- indio_dev->num_channels = st->chip_info->num_channels;
indio_dev->info = st->chip_info->info;
+ if (st->chip_info->parse_channels) {
+ ret = st->chip_info->parse_channels(indio_dev);
+ if (ret)
+ return ret;
+ } else {
+ indio_dev->channels = st->chip_info->channels;
+ indio_dev->num_channels = st->chip_info->num_channels;
+ }
- ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7192_sigma_delta_info);
+ ret = ad_sd_init(&st->sd, indio_dev, spi, st->chip_info->sigma_delta_info);
if (ret)
return ret;
@@ -1202,6 +1337,7 @@ static const struct of_device_id ad7192_of_match[] = {
{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
+ { .compatible = "adi,ad7194", .data = &ad7192_chip_info_tbl[ID_AD7194] },
{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
{}
};
@@ -1211,6 +1347,7 @@ static const struct spi_device_id ad7192_ids[] = {
{ "ad7190", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7190] },
{ "ad7192", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7192] },
{ "ad7193", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7193] },
+ { "ad7194", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7194] },
{ "ad7195", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7195] },
{}
};
@@ -1227,6 +1364,6 @@ static struct spi_driver ad7192_driver = {
module_spi_driver(ad7192_driver);
MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
-MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7195 ADC");
+MODULE_DESCRIPTION("Analog Devices AD7192 and similar ADC");
MODULE_LICENSE("GPL v2");
MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
--
2.34.1On Tue, May 14, 2024 at 03:02:22PM +0300, Alisa-Dariana Roman wrote:
> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> dynamically configure them in the devicetree.
>
> Add sigma_delta_info member to chip_info structure. Since AD7194 is the
> only chip that has no channel sequencer, num_slots should remain
> undefined.
>
> Also modify config AD7192 description for better scaling.
Some non-critical, mostly style related comments below.
...
This...
> +#define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
> + /* 10th bit corresponds to CON18(Pseudo) */
...should be (you have broken indentation on the comment, btw):
/* 10th bit corresponds to CON18(Pseudo) */
#define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
But no need to resend because of this, let's wait others to comment, and
if everything fine I think Jonathan can massage this when applying.
...
> +#define AD7194_CH_TEMP 0x100 /* Temp sensor */
Not sure that the comment has any value here.
...
> +static int ad7194_validate_ain_channel(struct device *dev, u32 ain)
> +{
> + if (!in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid AIN channel: %u\n", ain);
> +
> + return 0;
While this uses traditional pattern, it might be better looking in a form of
if (in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
return 0;
return dev_err_probe(dev, -EINVAL, "Invalid AIN channel: %u\n", ain);
But at the same time I would rather expect this to be in the caller and here
to have a boolean function
static bool ad7194_is_ain_channel_valid(struct device *dev, u32 ain)
{
return in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR);
}
> +}
...
> + return dev_err_probe(dev, -EINVAL,
> + "Too many channels: %u\n", num_channels);
return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n", num_channels);
?
Or with limit
return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
num_channels);
...
> + ad7194_channels = devm_kcalloc(dev, num_channels,
> + sizeof(*ad7194_channels), GFP_KERNEL);
ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels), GFP_KERNEL);
?
Or
ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels),
GFP_KERNEL);
?
...
> + device_for_each_child_node_scoped(dev, child) {
> + ret = fwnode_property_read_u32_array(child, "diff-channels",
> + ain, ARRAY_SIZE(ain));
> + if (ret == 0) {
And here I would rather go for the traditional pattern, i.e.
if (ret) {
...
} else {
...
}
> + ret = ad7194_validate_ain_channel(dev, ain[0]);
> + if (ret)
> + return ret;
> +
> + ret = ad7194_validate_ain_channel(dev, ain[1]);
> + if (ret)
> + return ret;
> +
> + *ad7194_channels = ad7194_chan_diff;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->channel = ain[0];
> + ad7194_channels->channel2 = ain[1];
> + ad7194_channels->address = AD7194_DIFF_CH(ain[0], ain[1]);
> + } else {
> + ret = fwnode_property_read_u32(child, "single-channel",
> + &ain[0]);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Missing channel property\n");
> +
> + ret = ad7194_validate_ain_channel(dev, ain[0]);
> + if (ret)
> + return ret;
> +
> + *ad7194_channels = ad7194_chan;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->channel = ain[0];
> + ad7194_channels->address = AD7194_CH(ain[0]);
> + }
> + ad7194_channels++;
> + }
--
With Best Regards,
Andy Shevchenko
On Tue, 14 May 2024 16:09:32 +0300
Andy Shevchenko <andy@kernel.org> wrote:
> On Tue, May 14, 2024 at 03:02:22PM +0300, Alisa-Dariana Roman wrote:
> > Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> > dynamically configure them in the devicetree.
> >
> > Add sigma_delta_info member to chip_info structure. Since AD7194 is the
> > only chip that has no channel sequencer, num_slots should remain
> > undefined.
> >
> > Also modify config AD7192 description for better scaling.
>
> Some non-critical, mostly style related comments below.
>
Tweaked a bit. And applied. Please check the result in the testing branch
of iio.git.
> ...
>
> This...
>
> > +#define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
> > + /* 10th bit corresponds to CON18(Pseudo) */
>
> ...should be (you have broken indentation on the comment, btw):
>
> /* 10th bit corresponds to CON18(Pseudo) */
> #define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
>
> But no need to resend because of this, let's wait others to comment, and
> if everything fine I think Jonathan can massage this when applying.
Fixed.
>
> ...
>
> > +#define AD7194_CH_TEMP 0x100 /* Temp sensor */
>
> Not sure that the comment has any value here.
Dropped
>
> ...
>
> > +static int ad7194_validate_ain_channel(struct device *dev, u32 ain)
> > +{
> > + if (!in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
> > + return dev_err_probe(dev, -EINVAL,
> > + "Invalid AIN channel: %u\n", ain);
> > +
> > + return 0;
>
> While this uses traditional pattern, it might be better looking in a form of
>
> if (in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
> return 0;
>
> return dev_err_probe(dev, -EINVAL, "Invalid AIN channel: %u\n", ain);
>
> But at the same time I would rather expect this to be in the caller and here
> to have a boolean function
>
Moved it.
> static bool ad7194_is_ain_channel_valid(struct device *dev, u32 ain)
> {
> return in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR);
> }
>
> > +}
>
> ...
>
> > + return dev_err_probe(dev, -EINVAL,
> > + "Too many channels: %u\n", num_channels);
>
> return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n", num_channels);
>
> ?
>
> Or with limit
>
> return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
> num_channels);
>
>
This one.
> ...
>
> > + ad7194_channels = devm_kcalloc(dev, num_channels,
> > + sizeof(*ad7194_channels), GFP_KERNEL);
>
> ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels), GFP_KERNEL);
>
> ?
>
> Or
>
> ad7194_channels = devm_kcalloc(dev, num_channels, sizeof(*ad7194_channels),
> GFP_KERNEL);
Nope. too long in either case.
>
> ?
>
> ...
>
> > + device_for_each_child_node_scoped(dev, child) {
> > + ret = fwnode_property_read_u32_array(child, "diff-channels",
> > + ain, ARRAY_SIZE(ain));
> > + if (ret == 0) {
>
> And here I would rather go for the traditional pattern, i.e.
>
> if (ret) {
> ...
> } else {
> ...
> }
It's odd, as it's two good paths I've left this one alone.
>
> > + ret = ad7194_validate_ain_channel(dev, ain[0]);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ad7194_validate_ain_channel(dev, ain[1]);
> > + if (ret)
> > + return ret;
> > +
> > + *ad7194_channels = ad7194_chan_diff;
> > + ad7194_channels->scan_index = index++;
> > + ad7194_channels->channel = ain[0];
> > + ad7194_channels->channel2 = ain[1];
> > + ad7194_channels->address = AD7194_DIFF_CH(ain[0], ain[1]);
> > + } else {
> > + ret = fwnode_property_read_u32(child, "single-channel",
> > + &ain[0]);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "Missing channel property\n");
> > +
> > + ret = ad7194_validate_ain_channel(dev, ain[0]);
> > + if (ret)
> > + return ret;
> > +
> > + *ad7194_channels = ad7194_chan;
> > + ad7194_channels->scan_index = index++;
> > + ad7194_channels->channel = ain[0];
> > + ad7194_channels->address = AD7194_CH(ain[0]);
> > + }
> > + ad7194_channels++;
> > + }
>
On 19.05.2024 21:03, Jonathan Cameron wrote:
> On Tue, 14 May 2024 16:09:32 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
>
>> On Tue, May 14, 2024 at 03:02:22PM +0300, Alisa-Dariana Roman wrote:
>>> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
>>> dynamically configure them in the devicetree.
>>>
>>> Add sigma_delta_info member to chip_info structure. Since AD7194 is the
>>> only chip that has no channel sequencer, num_slots should remain
>>> undefined.
>>>
>>> Also modify config AD7192 description for better scaling.
>>
>> Some non-critical, mostly style related comments below.
>>
> Tweaked a bit. And applied. Please check the result in the testing branch
> of iio.git.
Thank you guys for the feedback and for making the adjustments!
+/* 10th bit corresponds to CON18(Pseudo) */
+#define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
+
I noticed this comment got away in the testing branch.
+static bool ad7194_validate_ain_channel(struct device *dev, u32 ain)
+{
+ return in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR);
+}
And the negation got lost here.
With these little changes, tested on board to make sure, running perfectly!
Kind regards,
Alisa-Dariana Roman.
On Sun, 19 May 2024 22:37:58 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:
> On 19.05.2024 21:03, Jonathan Cameron wrote:
> > On Tue, 14 May 2024 16:09:32 +0300
> > Andy Shevchenko <andy@kernel.org> wrote:
> >
> >> On Tue, May 14, 2024 at 03:02:22PM +0300, Alisa-Dariana Roman wrote:
> >>> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> >>> dynamically configure them in the devicetree.
> >>>
> >>> Add sigma_delta_info member to chip_info structure. Since AD7194 is the
> >>> only chip that has no channel sequencer, num_slots should remain
> >>> undefined.
> >>>
> >>> Also modify config AD7192 description for better scaling.
> >>
> >> Some non-critical, mostly style related comments below.
> >>
> > Tweaked a bit. And applied. Please check the result in the testing branch
> > of iio.git.
>
> Thank you guys for the feedback and for making the adjustments!
>
> +/* 10th bit corresponds to CON18(Pseudo) */
> +#define AD7194_CH(p) (BIT(10) | AD7194_CH_POS(p))
> +
> I noticed this comment got away in the testing branch.
>
>
> +static bool ad7194_validate_ain_channel(struct device *dev, u32 ain)
> +{
> + return in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR);
> +}
> And the negation got lost here.
Ouch :(
>
> With these little changes, tested on board to make sure, running perfectly!
>
To make sure I don't mess it up again, could you post the fix and I'll squash
it into the patch on the tree.
I blame caffeine (for no particularly reason ;)
Jonathan
> Kind regards,
> Alisa-Dariana Roman.
>
>
On Mon, May 20, 2024 at 01:20:30PM +0100, Jonathan Cameron wrote:
> On Sun, 19 May 2024 22:37:58 +0300
> Alisa-Dariana Roman <alisadariana@gmail.com> wrote:
> > On 19.05.2024 21:03, Jonathan Cameron wrote:
...
> > +static bool ad7194_validate_ain_channel(struct device *dev, u32 ain)
> > +{
> > + return in_range(ain, AD7194_CH_AIN_START, AD7194_CH_AIN_NR);
> > +}
> > And the negation got lost here.
>
> Ouch :(
And negation most likely should be on the caller's side.
--
With Best Regards,
Andy Shevchenko
---
Would this fix be alright, since writing something like if(!ret) may be
confusing?
And regarding the comment, my bad, there is nothing wrong there.
drivers/iio/adc/ad7192.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 101afce49378..0789121236d6 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -1101,14 +1101,12 @@ static int ad7194_parse_channels(struct iio_dev *indio_dev)
ret = fwnode_property_read_u32_array(child, "diff-channels",
ain, ARRAY_SIZE(ain));
if (ret == 0) {
- ret = ad7194_validate_ain_channel(dev, ain[0]);
- if (ret)
+ if (!ad7194_validate_ain_channel(dev, ain[0]))
return dev_err_probe(dev, -EINVAL,
"Invalid AIN channel: %u\n",
ain[0]);
- ret = ad7194_validate_ain_channel(dev, ain[1]);
- if (ret)
+ if (!ad7194_validate_ain_channel(dev, ain[1]))
return dev_err_probe(dev, -EINVAL,
"Invalid AIN channel: %u\n",
ain[1]);
@@ -1125,8 +1123,7 @@ static int ad7194_parse_channels(struct iio_dev *indio_dev)
return dev_err_probe(dev, ret,
"Missing channel property\n");
- ret = ad7194_validate_ain_channel(dev, ain[0]);
- if (ret)
+ if (!ad7194_validate_ain_channel(dev, ain[0]))
return dev_err_probe(dev, -EINVAL,
"Invalid AIN channel: %u\n",
ain[0]);
--
2.34.1On Wed, 22 May 2024 12:50:23 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:
> ---
>
> Would this fix be alright, since writing something like if(!ret) may be
> confusing?
>
Looks fine to me. Squashed into original commit that I messed up.
Thanks for sorting this out.
Jonathan
> And regarding the comment, my bad, there is nothing wrong there.
>
> drivers/iio/adc/ad7192.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 101afce49378..0789121236d6 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -1101,14 +1101,12 @@ static int ad7194_parse_channels(struct iio_dev *indio_dev)
> ret = fwnode_property_read_u32_array(child, "diff-channels",
> ain, ARRAY_SIZE(ain));
> if (ret == 0) {
> - ret = ad7194_validate_ain_channel(dev, ain[0]);
> - if (ret)
> + if (!ad7194_validate_ain_channel(dev, ain[0]))
> return dev_err_probe(dev, -EINVAL,
> "Invalid AIN channel: %u\n",
> ain[0]);
>
> - ret = ad7194_validate_ain_channel(dev, ain[1]);
> - if (ret)
> + if (!ad7194_validate_ain_channel(dev, ain[1]))
> return dev_err_probe(dev, -EINVAL,
> "Invalid AIN channel: %u\n",
> ain[1]);
> @@ -1125,8 +1123,7 @@ static int ad7194_parse_channels(struct iio_dev *indio_dev)
> return dev_err_probe(dev, ret,
> "Missing channel property\n");
>
> - ret = ad7194_validate_ain_channel(dev, ain[0]);
> - if (ret)
> + if (!ad7194_validate_ain_channel(dev, ain[0]))
> return dev_err_probe(dev, -EINVAL,
> "Invalid AIN channel: %u\n",
> ain[0]);
© 2016 - 2025 Red Hat, Inc.