Add support for AD7091R-2/-4/-8
Subsystems (5)
Marcelo Schmitt
maintainerv4 cover Add support for AD7091R-2/-4/-8
From: Marcelo Schmitt <[email protected]>
----------------- Updates -----------------
Applied changes suggested to the previous set.
Change log v3 -> v4:
- Patch 1: checkpatch patch
* Changed __aligned regex string.
- Patch 2: alert handling fix
* Applied David's suggestion [1] to pass iio_dev on to IRQ thread handler.
- Patches 6, 7, 9
* Removed ad7091r prefix from callback function names.
- New Patch 7: Remove uneeded probe parameters
* Removed id->name and regmap from probe paramenters.
- Patch 8 (now Patch 9): Enable internal vref
* Not expecting NULL return from regulator_get_optional() anymore;
* Reverted to previous probe defer handling.
- Patch 10 (now Patch 11): dt doc
* Extending existing ad7091r5 dt doc instead of creating a new one;
* Added VDD and VDRIVE supplies to dt doc;
* Removed channel property from dt doc;
* Interrupt description, interrupt constraint check, example indentation improvements.
- Patch 12 (now Patch 13): add ad7091r8 patch
* Neats to macros, gpio setups, and probe parameters.
- Patch 13 (now Patch 14):
* Made use of wild cards in MAINTAINERS file.
- New Patch (Patch 15): event configuration callbacks
Thank you all for the help with this set,
Marcelo
----------------- Context -----------------
This series adds support for AD7091R-2/-4/-8 ADCs which can do single shot or sequenced readings. Threshold events are also supported.
Overall, AD7091R-2/-4/-8 are very similar to AD7091R-5 except they use SPI interface.
Changes have been tested with raspberrypi and eval board on raspberrypi kernel 6.7-rc3 from raspberrypi fork.
Marcelo Schmitt (15):
scripts: checkpatch: Add __aligned to the list of attribute notes
iio: adc: ad7091r: Pass iio_dev to event handler
iio: adc: ad7091r: Set alert bit in config register
iio: adc: ad7091r: Align arguments to function call parenthesis
iio: adc: ad7091r: Move generic AD7091R code to base driver and header
file
iio: adc: ad7091r: Move chip init data to container struct
iio: adc: ad7091r: Remove unneeded probe parameters
iio: adc: ad7091r: Set device mode through chip_info callback
iio: adc: ad7091r: Enable internal vref if external vref is not
supplied
iio: adc: ad7091r: Add chip_info callback to get conversion result
channel
iio: adc: Split AD7091R-5 config symbol
dt-bindings: iio: Add AD7091R-8
iio: adc: Add support for AD7091R-8
MAINTAINERS: Add MAINTAINERS entry for AD7091R
iio: adc: ad7091r: Allow users to configure device events
.../bindings/iio/adc/adi,ad7091r5.yaml | 82 +++++- MAINTAINERS | 8 + drivers/iio/adc/Kconfig | 16 ++ drivers/iio/adc/Makefile | 4 +- drivers/iio/adc/ad7091r-base.c | 255 +++++++++++------ drivers/iio/adc/ad7091r-base.h | 81 +++++- drivers/iio/adc/ad7091r5.c | 120 ++++---- drivers/iio/adc/ad7091r8.c | 257 ++++++++++++++++++ scripts/checkpatch.pl | 1 + 9 files changed, 682 insertions(+), 142 deletions(-) create mode 100644 drivers/iio/adc/ad7091r8.c
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 1/15 scripts: checkpatch: Add __aligned to the list of attribute notes
Checkpatch presumes attributes marked with __aligned(alignment) are part of a function declaration and throws a warning stating that those compiler attributes should have an identifier name which is not correct. Add __aligned compiler attributes to the list of attribute notes so they don't cause warnings anymore.
---
Any expression that evaluates to an integer that is a power of 2 can be within __aligned parenthesis.
I can't see how we could use a regex to check code meets such constraint (if possible).
Some additional exotic uses of __aligned are:drivers/net/wireless/quantenna/qtnfmac/bus.h:72: char bus_priv[] __aligned(sizeof(void *));include/linux/netdevice.h:225:} __aligned(4 * sizeof(unsigned long));
The regex
__aligned\s*\(.*\)
seems to match all use cases.
We might not catch invalid arguments to __aligned, but it looks like making checkpath confidently report those wouldn't be feasible anyway.
The patch that would trigger the mentioned warning in now patch number 13 (iio: adc: Add support for AD7091R-8).
scripts/checkpatch.pl | 1 + 1 file changed, 1 insertion(+)
scripts/checkpatch.pl +1 -0
@@ -512,6 +512,7 @@ our $Attribute = qr{ __ro_after_init| __kprobes| $InitAttribute| + __aligned\s*\(.*\)| ____cacheline_aligned| ____cacheline_aligned_in_smp| ____cacheline_internodealigned_in_smp|
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 2/15 iio: adc: ad7091r: Pass iio_dev to event handler
In reply to Marcelo SchmittPrevious version of ad7091r event handler received the ADC state pointer and retrieved the iio device from driver data field with dev_get_drvdata(). However, no driver data have ever been set, which led to null pointer dereference when running the event handler.
Pass the iio device to the event handler and retrieve the ADC state struct from it so we avoid the null pointer dereference and save the driver from filling the driver data field.
--- drivers/iio/adc/ad7091r-base.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/iio/adc/ad7091r-base.c +3 -3
@@ -174,8 +174,8 @@ static const struct iio_info ad7091r_info = {static irqreturn_t ad7091r_event_handler (int irq,void *private) { -struct ad7091r_state *st = (struct ad7091r_state *) private; -struct iio_dev *iio_dev =dev_get_drvdata (st->dev); +struct iio_dev *iio_dev = private; +struct ad7091r_state *st =iio_priv (iio_dev);unsigned int i, read_val;int ret;s64 timestamp =iio_get_time_ns (iio_dev);
@@ -234,7 +234,7 @@ int ad7091r_probe(struct device *dev, const char *name,if (irq) { ret =devm_request_threaded_irq (dev, irq,NULL , ad7091r_event_handler, -IRQF_TRIGGER_FALLING |IRQF_ONESHOT , name, st); +IRQF_TRIGGER_FALLING |IRQF_ONESHOT , name, iio_dev);if (ret)return ret; }
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 3/15 iio: adc: ad7091r: Set alert bit in config register
In reply to Marcelo SchmittThe ad7091r-base driver sets up an interrupt handler for firing events when inputs are either above or below a certain threshold. However, for the interrupt signal to come from the device it must be configured to enable the ALERT/BUSY/GPO pin to be used as ALERT, which was not being done until now.
Enable interrupt signals on the ALERT/BUSY/GPO pin by setting the proper bit in the configuration register.
--- drivers/iio/adc/ad7091r-base.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/iio/adc/ad7091r-base.c +6 -0
@@ -28,6 +28,7 @@#define AD7091R_REG_RESULT_CONV_RESULT (x) ((x) & 0xfff)/* AD7091R_REG_CONF */ +#define AD7091R_REG_CONF_ALERT_EN BIT(4)#define AD7091R_REG_CONF_AUTO BIT(8)#define AD7091R_REG_CONF_CMD BIT(10)
@@ -232,6 +233,11 @@ int ad7091r_probe(struct device *dev, const char *name, iio_dev->channels = chip_info->channels;if (irq) { + ret =regmap_update_bits (st->map,AD7091R_REG_CONF , +AD7091R_REG_CONF_ALERT_EN ,BIT (4 )); +if (ret) +return ret; + ret =devm_request_threaded_irq (dev, irq,NULL , ad7091r_event_handler,IRQF_TRIGGER_FALLING |IRQF_ONESHOT , name, iio_dev);
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 4/15 iio: adc: ad7091r: Align arguments to function call parenthesis
In reply to Marcelo SchmittAlign arguments to function call open parenthesis to comply with the Linux kernel coding style.
--- drivers/iio/adc/ad7091r-base.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/iio/adc/ad7091r-base.c +3 -2
@@ -239,8 +239,9 @@ int ad7091r_probe(struct device *dev, const char *name,return ret; ret =devm_request_threaded_irq (dev, irq,NULL , - ad7091r_event_handler, -IRQF_TRIGGER_FALLING |IRQF_ONESHOT , name, iio_dev); + ad7091r_event_handler, +IRQF_TRIGGER_FALLING | +IRQF_ONESHOT , name, iio_dev);if (ret)return ret; }
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 5/15 iio: adc: ad7091r: Move generic AD7091R code to base driver and header file
In reply to Marcelo SchmittSome code generic to AD7091R devices such as channel definitions and event spec structs were in the AD7091R-5 driver.
There was also some generic register definitions declared in the base driver which would make more sense to be in the header file. The device state struct will be needed for the ad7091r8 driver in a follow up patch so that ought to be moved to the header file as well. Lastly, a couple of regmap callback functions are also capable of abstracting characteristics of different AD7091R devices and those are now being exported to IIO_AD7091R name space.
Move AD7091R generic code either to the base driver or to the header file so both the ad7091r5 and the ad7091r8 driver can use those declaration in follow up patches.
--- drivers/iio/adc/ad7091r-base.c | 46 +++++++++++++++++----------------- drivers/iio/adc/ad7091r-base.h | 42 ++++++++++++++++++++++++++++++- drivers/iio/adc/ad7091r5.c | 39 +++------------------------- 3 files changed, 68 insertions(+), 59 deletions(-)
drivers/iio/adc/ad7091r-base.c +23 -23
@@ -15,14 +15,6 @@#include "ad7091r-base.h" -#define AD7091R_REG_RESULT 0 -#define AD7091R_REG_CHANNEL 1 -#define AD7091R_REG_CONF 2 -#define AD7091R_REG_ALERT 3 -#define AD7091R_REG_CH_LOW_LIMIT (ch) ((ch) * 3 + 4) -#define AD7091R_REG_CH_HIGH_LIMIT (ch) ((ch) * 3 + 5) -#define AD7091R_REG_CH_HYSTERESIS (ch) ((ch) * 3 + 6) -/* AD7091R_REG_RESULT */ #define AD7091R_REG_RESULT_CH_ID (x) (((x) >> 13) & 0x3)#define AD7091R_REG_RESULT_CONV_RESULT (x) ((x) & 0xfff)
@@ -35,20 +27,26 @@#define AD7091R_REG_CONF_MODE_MASK \ (AD7091R_REG_CONF_AUTO | AD7091R_REG_CONF_CMD) -enum ad7091r_mode { -AD7091R_MODE_SAMPLE , -AD7091R_MODE_COMMAND , -AD7091R_MODE_AUTOCYCLE , -}; - -struct ad7091r_state { -struct device *dev; -struct regmap *map; -struct regulator *vref; -const struct ad7091r_chip_info *chip_info; -enum ad7091r_mode mode; -struct mutex lock;/*lock to prevent concurent reads */ +const struct iio_event_spec ad7091r_events[] = { + { + .type =IIO_EV_TYPE_THRESH , + .dir =IIO_EV_DIR_RISING , + .mask_separate =BIT (IIO_EV_INFO_VALUE ) | +BIT (IIO_EV_INFO_ENABLE ), + }, + { + .type =IIO_EV_TYPE_THRESH , + .dir =IIO_EV_DIR_FALLING , + .mask_separate =BIT (IIO_EV_INFO_VALUE ) | +BIT (IIO_EV_INFO_ENABLE ), + }, + { + .type =IIO_EV_TYPE_THRESH , + .dir =IIO_EV_DIR_EITHER , + .mask_separate =BIT (IIO_EV_INFO_HYSTERESIS ), + }, }; +EXPORT_SYMBOL_NS_GPL (ad7091r_events,IIO_AD7091R );static int ad7091r_set_mode (struct ad7091r_state *st,enum ad7091r_mode mode) {
@@ -269,7 +267,7 @@ int ad7091r_probe(struct device *dev, const char *name, }EXPORT_SYMBOL_NS_GPL (ad7091r_probe,IIO_AD7091R ); -static bool ad7091r_writeable_reg (struct device *dev,unsigned int reg) +bool ad7091r_writeable_reg (struct device *dev,unsigned int reg) {switch (reg) {case AD7091R_REG_RESULT :
@@ -279,8 +277,9 @@ static bool ad7091r_writeable_reg(struct device *dev, unsigned int reg)return true; } } +EXPORT_SYMBOL_NS_GPL (ad7091r_writeable_reg,IIO_AD7091R ); -static bool ad7091r_volatile_reg (struct device *dev,unsigned int reg) +bool ad7091r_volatile_reg (struct device *dev,unsigned int reg) {switch (reg) {case AD7091R_REG_RESULT :
@@ -290,6 +289,7 @@ static bool ad7091r_volatile_reg(struct device *dev, unsigned int reg)return false; } } +EXPORT_SYMBOL_NS_GPL (ad7091r_volatile_reg,IIO_AD7091R );const struct regmap_config ad7091r_regmap_config = { .reg_bits =8 ,
drivers/iio/adc/ad7091r-base.h +41 -1
59 lines not on this page · raw message
drivers/iio/adc/ad7091r5.c +4 -35
@@ -12,42 +12,11 @@#include "ad7091r-base.h" -static const struct iio_event_spec ad7091r5_events[] = { - { - .type =IIO_EV_TYPE_THRESH , - .dir =IIO_EV_DIR_RISING , - .mask_separate =BIT (IIO_EV_INFO_VALUE ) | -BIT (IIO_EV_INFO_ENABLE ), - }, - { - .type =IIO_EV_TYPE_THRESH , - .dir =IIO_EV_DIR_FALLING , - .mask_separate =BIT (IIO_EV_INFO_VALUE ) | -BIT (IIO_EV_INFO_ENABLE ), - }, - { - .type =IIO_EV_TYPE_THRESH , - .dir =IIO_EV_DIR_EITHER , - .mask_separate =BIT (IIO_EV_INFO_HYSTERESIS ), - }, -}; - -#define AD7091R_CHANNEL (idx, bits, ev, num_ev) { \ - .type = IIO_VOLTAGE, \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ - .indexed = 1, \ - .channel = idx, \ - .event_spec = ev, \ - .num_event_specs = num_ev, \ - .scan_type.storagebits = 16, \ - .scan_type.realbits = bits, \ -}static const struct iio_chan_spec ad7091r5_channels_irq[] = { -AD7091R_CHANNEL (0 ,12 , ad7091r5_events,ARRAY_SIZE (ad7091r5_events)), -AD7091R_CHANNEL (1 ,12 , ad7091r5_events,ARRAY_SIZE (ad7091r5_events)), -AD7091R_CHANNEL (2 ,12 , ad7091r5_events,ARRAY_SIZE (ad7091r5_events)), -AD7091R_CHANNEL (3 ,12 , ad7091r5_events,ARRAY_SIZE (ad7091r5_events)), +AD7091R_CHANNEL (0 ,12 , ad7091r_events,ARRAY_SIZE (ad7091r_events)), +AD7091R_CHANNEL (1 ,12 , ad7091r_events,ARRAY_SIZE (ad7091r_events)), +AD7091R_CHANNEL (2 ,12 , ad7091r_events,ARRAY_SIZE (ad7091r_events)), +AD7091R_CHANNEL (3 ,12 , ad7091r_events,ARRAY_SIZE (ad7091r_events)), };static const struct iio_chan_spec ad7091r5_channels_noirq[] = {
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 6/15 iio: adc: ad7091r: Move chip init data to container struct
In reply to Marcelo SchmittAD7091R designs may differ on their communication protocol and resources required for proper setup. Extract what is design specific into a init_info struct so the base driver can use data and callback functions from that struct rather than checking which specific chip is connected during device initialization.
--- drivers/iio/adc/ad7091r-base.c | 27 ++++++++++----------- drivers/iio/adc/ad7091r-base.h | 15 +++++++++--- drivers/iio/adc/ad7091r5.c | 43 ++++++++++++++++++++++++---------- 3 files changed, 55 insertions(+), 30 deletions(-)
drivers/iio/adc/ad7091r-base.c +12 -15
@@ -207,7 +207,7 @@ static void ad7091r_remove(void *data) }int ad7091r_probe (struct device *dev,const char *name, -const struct ad7091r_chip_info *chip_info, +const struct ad7091r_init_info *init_info,struct regmap *map,int irq) {struct iio_dev *iio_dev;
@@ -220,17 +220,16 @@ int ad7091r_probe(struct device *dev, const char *name, st =iio_priv (iio_dev); st->dev = dev; - st->chip_info = chip_info; - st->map = map; + init_info->init_adc_regmap (st, init_info->regmap_config); +if (IS_ERR (st->map)) +return dev_err_probe (st->dev,PTR_ERR (st->map), +"Error initializing regmap\n" ); - iio_dev->name = name; iio_dev->info = &ad7091r_info; iio_dev->modes =INDIO_DIRECT_MODE ; - iio_dev->num_channels = chip_info->num_channels; - iio_dev->channels = chip_info->channels; -if (irq) { + st->chip_info = &init_info->irq_info; ret =regmap_update_bits (st->map,AD7091R_REG_CONF ,AD7091R_REG_CONF_ALERT_EN ,BIT (4 ));if (ret)
@@ -242,8 +241,14 @@ int ad7091r_probe(struct device *dev, const char *name, IRQF_ONESHOT, name, iio_dev);if (ret)return ret; + }else { + st->chip_info = &init_info->info_no_irq; } + iio_dev->name = st->chip_info->name; + iio_dev->num_channels = st->chip_info->num_channels; + iio_dev->channels = st->chip_info->channels; + st->vref =devm_regulator_get_optional (dev,"vref" );if (IS_ERR (st->vref)) {if (PTR_ERR (st->vref) == -EPROBE_DEFER )
@@ -291,14 +296,6 @@ bool ad7091r_volatile_reg(struct device *dev, unsigned int reg) }EXPORT_SYMBOL_NS_GPL (ad7091r_volatile_reg,IIO_AD7091R ); -const struct regmap_config ad7091r_regmap_config = { - .reg_bits =8 , - .val_bits =16 , - .writeable_reg = ad7091r_writeable_reg, - .volatile_reg = ad7091r_volatile_reg, -}; -EXPORT_SYMBOL_NS_GPL (ad7091r_regmap_config,IIO_AD7091R ); -MODULE_AUTHOR ("Beniamin Bia <[email protected]>" );MODULE_DESCRIPTION ("Analog Devices AD7091Rx multi-channel converters" );MODULE_LICENSE ("GPL v2" );
drivers/iio/adc/ad7091r-base.h +12 -3
@@ -8,6 +8,8 @@#ifndef __DRIVERS_IIO_ADC_AD7091R_BASE_H__#define __DRIVERS_IIO_ADC_AD7091R_BASE_H__ +#include <linux/regmap.h> +#define AD7091R_REG_RESULT 0#define AD7091R_REG_CHANNEL 1#define AD7091R_REG_CONF 2
@@ -47,17 +49,24 @@ struct ad7091r_state { };struct ad7091r_chip_info { +const char *name;unsigned int num_channels;const struct iio_chan_spec *channels;unsigned int vref_mV; }; -extern const struct iio_event_spec ad7091r_events[3 ]; +struct ad7091r_init_info { +struct ad7091r_chip_info irq_info; +struct ad7091r_chip_info info_no_irq; +const struct regmap_config *regmap_config; +void (*init_adc_regmap)(struct ad7091r_state *st, +const struct regmap_config *regmap_conf); +}; -extern const struct regmap_config ad7091r_regmap_config; +extern const struct iio_event_spec ad7091r_events[3 ];int ad7091r_probe (struct device *dev,const char *name, -const struct ad7091r_chip_info *chip_info, +const struct ad7091r_init_info *init_info,struct regmap *map,int irq);bool ad7091r_volatile_reg (struct device *dev,unsigned int reg);
drivers/iio/adc/ad7091r5.c +31 -12
73 lines not on this page · raw message
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 7/15 iio: adc: ad7091r: Remove unneeded probe parameters
In reply to Marcelo SchmittWith the grouping of ad7091r initialization data and callbacks into the init_info struct, there is no more need to pass the device name and register map through probe function parameters as those will be available in the init_info object.
Remove probe parameters that are not needed anymore.
---
Maybe squash this patch with the previous one.
Wasn't sure about it and squashing is easier than to split so I left it separate.
drivers/iio/adc/ad7091r-base.c | 8 ++++---- drivers/iio/adc/ad7091r-base.h | 5 ++--- drivers/iio/adc/ad7091r5.c | 3 +-- 3 files changed, 7 insertions(+), 9 deletions(-)
drivers/iio/adc/ad7091r-base.c +4 -4
@@ -206,9 +206,8 @@ static void ad7091r_remove(void *data)regulator_disable (st->vref); } -int ad7091r_probe (struct device *dev,const char *name, -const struct ad7091r_init_info *init_info, -struct regmap *map,int irq) +int ad7091r_probe (struct device *dev,const struct ad7091r_init_info *init_info, +int irq) {struct iio_dev *iio_dev;struct ad7091r_state *st;
@@ -238,7 +237,8 @@ int ad7091r_probe(struct device *dev, const char *name, ret =devm_request_threaded_irq (dev, irq,NULL , ad7091r_event_handler,IRQF_TRIGGER_FALLING | -IRQF_ONESHOT , name, iio_dev); +IRQF_ONESHOT , + st->chip_info->name, iio_dev);if (ret)return ret; }else {
drivers/iio/adc/ad7091r-base.h +2 -3
@@ -65,9 +65,8 @@ struct ad7091r_init_info {extern const struct iio_event_spec ad7091r_events[3 ]; -int ad7091r_probe (struct device *dev,const char *name, -const struct ad7091r_init_info *init_info, -struct regmap *map,int irq); +int ad7091r_probe (struct device *dev,const struct ad7091r_init_info *init_info, +int irq);bool ad7091r_volatile_reg (struct device *dev,unsigned int reg);bool ad7091r_writeable_reg (struct device *dev,unsigned int reg);
drivers/iio/adc/ad7091r5.c +1 -2
@@ -64,14 +64,13 @@ static struct ad7091r_init_info ad7091r5_init_info = {static int ad7091r5_i2c_probe (struct i2c_client *i2c) { -const struct i2c_device_id *id =i2c_client_get_device_id (i2c);const struct ad7091r_init_info *init_info; init_info =i2c_get_match_data (i2c);if (!init_info)return -EINVAL ; -return ad7091r_probe (&i2c->dev, id->name, init_info,NULL , i2c->irq); +return ad7091r_probe (&i2c->dev, init_info, i2c->irq); }static const struct of_device_id ad7091r5_dt_ids[] = {
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 8/15 iio: adc: ad7091r: Set device mode through chip_info callback
In reply to Marcelo SchmittAD7091R-5 devices have a few modes of operation (sample, command, autocycle) which are set by writing to configuration register fields. Follow up patches will add support for AD7091R-2/-4/-8 which don't have those operation modes nor the register fields for setting them. Make ad7091r_set_mode() a callback function of AD7091R chip_info struct so the base driver can appropriately handle each design without having to check which actual chip is connected.
--- drivers/iio/adc/ad7091r-base.c | 38 +--------------------------------- drivers/iio/adc/ad7091r-base.h | 9 ++++++++ drivers/iio/adc/ad7091r5.c | 30 +++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 37 deletions(-)
drivers/iio/adc/ad7091r-base.c +1 -37
@@ -19,14 +19,6 @@#define AD7091R_REG_RESULT_CH_ID (x) (((x) >> 13) & 0x3)#define AD7091R_REG_RESULT_CONV_RESULT (x) ((x) & 0xfff) -/* AD7091R_REG_CONF */ -#define AD7091R_REG_CONF_ALERT_EN BIT(4) -#define AD7091R_REG_CONF_AUTO BIT(8) -#define AD7091R_REG_CONF_CMD BIT(10) - -#define AD7091R_REG_CONF_MODE_MASK \ - (AD7091R_REG_CONF_AUTO | AD7091R_REG_CONF_CMD) -const struct iio_event_spec ad7091r_events[] = { { .type =IIO_EV_TYPE_THRESH ,
@@ -48,34 +40,6 @@ const struct iio_event_spec ad7091r_events[] = { };EXPORT_SYMBOL_NS_GPL (ad7091r_events,IIO_AD7091R ); -static int ad7091r_set_mode (struct ad7091r_state *st,enum ad7091r_mode mode) -{ -int ret, conf; - -switch (mode) { -case AD7091R_MODE_SAMPLE : - conf =0 ; -break ; -case AD7091R_MODE_COMMAND : - conf =AD7091R_REG_CONF_CMD ; -break ; -case AD7091R_MODE_AUTOCYCLE : - conf =AD7091R_REG_CONF_AUTO ; -break ; -default : -return -EINVAL ; - } - - ret =regmap_update_bits (st->map,AD7091R_REG_CONF , -AD7091R_REG_CONF_MODE_MASK , conf); -if (ret) -return ret; - - st->mode = mode; - -return 0 ; -} -static int ad7091r_set_channel (struct ad7091r_state *st,unsigned int channel) {unsigned int dummy;
@@ -264,7 +228,7 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info, }/* Use command mode by default to convert only desired channels*/ - ret =ad7091r_set_mode (st,AD7091R_MODE_COMMAND ); + ret = st->chip_info->set_mode (st,AD7091R_MODE_COMMAND );if (ret)return ret;
drivers/iio/adc/ad7091r-base.h +9 -0
@@ -19,6 +19,14 @@#define AD7091R_REG_CH_HIGH_LIMIT (ch) ((ch) * 3 + 5)#define AD7091R_REG_CH_HYSTERESIS (ch) ((ch) * 3 + 6) +/* AD7091R_REG_CONF */ +#define AD7091R_REG_CONF_ALERT_EN BIT(4) +#define AD7091R_REG_CONF_AUTO BIT(8) +#define AD7091R_REG_CONF_CMD BIT(10) + +#define AD7091R_REG_CONF_MODE_MASK \ + (AD7091R_REG_CONF_AUTO | AD7091R_REG_CONF_CMD) +#define AD7091R_CHANNEL (idx, bits, ev, num_ev) { \ .type = IIO_VOLTAGE, \ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
@@ -53,6 +61,7 @@ struct ad7091r_chip_info {unsigned int num_channels;const struct iio_chan_spec *channels;unsigned int vref_mV; +int (*set_mode)(struct ad7091r_state *st,enum ad7091r_mode mode); };struct ad7091r_init_info {
drivers/iio/adc/ad7091r5.c +30 -0
@@ -26,11 +26,40 @@ static const struct iio_chan_spec ad7091r5_channels_noirq[] = { AD7091R_CHANNEL(3, 12, NULL, 0), }; +static int ad7091r5_set_mode (struct ad7091r_state *st,enum ad7091r_mode mode) +{ +int ret, conf; + +switch (mode) { +case AD7091R_MODE_SAMPLE : + conf =0 ; +break ; +case AD7091R_MODE_COMMAND : + conf =AD7091R_REG_CONF_CMD ; +break ; +case AD7091R_MODE_AUTOCYCLE : + conf =AD7091R_REG_CONF_AUTO ; +break ; +default : +return -EINVAL ; + } + + ret =regmap_update_bits (st->map,AD7091R_REG_CONF , +AD7091R_REG_CONF_MODE_MASK , conf); +if (ret) +return ret; + + st->mode = mode; + +return 0 ; +} +static const struct ad7091r_chip_info ad7091r5_chip_info_irq = { .name ="ad7091r-5" , .channels = ad7091r5_channels_irq, .num_channels =ARRAY_SIZE (ad7091r5_channels_irq), .vref_mV =2500 , + .set_mode = &ad7091r5_set_mode, };static const struct ad7091r_chip_info ad7091r5_chip_info_noirq = {
@@ -38,6 +67,7 @@ static const struct ad7091r_chip_info ad7091r5_chip_info_noirq = { .channels = ad7091r5_channels_noirq, .num_channels = ARRAY_SIZE(ad7091r5_channels_noirq), .vref_mV = 2500, + .set_mode = &ad7091r5_set_mode, };static const struct regmap_config ad7091r_regmap_config = {
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 9/15 iio: adc: ad7091r: Enable internal vref if external vref is not supplied
In reply to Marcelo SchmittThe ADC needs a voltage reference to work correctly.
Enable AD7091R internal voltage reference if no external vref is supplied.
--- drivers/iio/adc/ad7091r-base.c | 7 +++++++ drivers/iio/adc/ad7091r-base.h | 1 + 2 files changed, 8 insertions(+)
drivers/iio/adc/ad7091r-base.c +7 -0
@@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info,if (IS_ERR (st->vref)) {if (PTR_ERR (st->vref) == -EPROBE_DEFER )return -EPROBE_DEFER ; + st->vref =NULL ; +/* Enable internal vref */ + ret =regmap_update_bits (st->map,AD7091R_REG_CONF , +AD7091R_REG_CONF_INT_VREF ,BIT (0 )); +if (ret) +return dev_err_probe (st->dev, ret, +"Error on enable internal reference\n" ); }else { ret =regulator_enable (st->vref);if (ret)
drivers/iio/adc/ad7091r-base.h +1 -0
@@ -20,6 +20,7 @@#define AD7091R_REG_CH_HYSTERESIS (ch) ((ch) * 3 + 6)/* AD7091R_REG_CONF */ +#define AD7091R_REG_CONF_INT_VREF BIT(0)#define AD7091R_REG_CONF_ALERT_EN BIT(4)#define AD7091R_REG_CONF_AUTO BIT(8)#define AD7091R_REG_CONF_CMD BIT(10)
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 10/15 iio: adc: ad7091r: Add chip_info callback to get conversion result channel
In reply to Marcelo SchmittAD7091R-5 and AD7091R-2/-4/-8 have slightly different register field layout and due to that require different masks for getting the index of the channel associated with each read.
Add a callback function so the base driver can get correct channel ID for each chip variant.
--- drivers/iio/adc/ad7091r-base.c | 6 +----- drivers/iio/adc/ad7091r-base.h | 6 ++++++ drivers/iio/adc/ad7091r5.c | 7 +++++++ 3 files changed, 14 insertions(+), 5 deletions(-)
drivers/iio/adc/ad7091r-base.c +1 -5
@@ -15,10 +15,6 @@#include "ad7091r-base.h" -/* AD7091R_REG_RESULT */ -#define AD7091R_REG_RESULT_CH_ID (x) (((x) >> 13) & 0x3) -#define AD7091R_REG_RESULT_CONV_RESULT (x) ((x) & 0xfff) -const struct iio_event_spec ad7091r_events[] = { { .type =IIO_EV_TYPE_THRESH ,
@@ -73,7 +69,7 @@ static int ad7091r_read_one(struct iio_dev *iio_dev,if (ret)return ret; -if (AD7091R_REG_RESULT_CH_ID (val) != channel) +if (st->chip_info->reg_result_chan_id (val) != channel)return -EIO ; *read_val =AD7091R_REG_RESULT_CONV_RESULT (val);
drivers/iio/adc/ad7091r-base.h +6 -0
@@ -19,6 +19,11 @@#define AD7091R_REG_CH_HIGH_LIMIT (ch) ((ch) * 3 + 5)#define AD7091R_REG_CH_HYSTERESIS (ch) ((ch) * 3 + 6) +/* AD7091R_REG_RESULT */ +#define AD7091R5_REG_RESULT_CH_ID (x) (((x) >> 13) & 0x3) +#define AD7091R8_REG_RESULT_CH_ID (x) (((x) >> 13) & 0x7) +#define AD7091R_REG_RESULT_CONV_RESULT (x) ((x) & 0xfff) +/* AD7091R_REG_CONF */ #define AD7091R_REG_CONF_INT_VREF BIT(0)#define AD7091R_REG_CONF_ALERT_EN BIT(4)
@@ -62,6 +67,7 @@ struct ad7091r_chip_info {unsigned int num_channels;const struct iio_chan_spec *channels;unsigned int vref_mV; +unsigned int (*reg_result_chan_id)(unsigned int val);int (*set_mode)(struct ad7091r_state *st,enum ad7091r_mode mode); };
drivers/iio/adc/ad7091r5.c +7 -0
@@ -54,11 +54,17 @@ static int ad7091r5_set_mode(struct ad7091r_state *st, enum ad7091r_mode mode)return 0 ; } +static unsigned int ad7091r5_reg_result_chan_id (unsigned int val) +{ +return AD7091R5_REG_RESULT_CH_ID (val); +} +static const struct ad7091r_chip_info ad7091r5_chip_info_irq = { .name ="ad7091r-5" , .channels = ad7091r5_channels_irq, .num_channels =ARRAY_SIZE (ad7091r5_channels_irq), .vref_mV =2500 , + .reg_result_chan_id = &ad7091r5_reg_result_chan_id, .set_mode = &ad7091r5_set_mode, };
@@ -67,6 +73,7 @@ static const struct ad7091r_chip_info ad7091r5_chip_info_noirq = { .channels = ad7091r5_channels_noirq, .num_channels = ARRAY_SIZE(ad7091r5_channels_noirq), .vref_mV = 2500, + .reg_result_chan_id = &ad7091r5_reg_result_chan_id, .set_mode = &ad7091r5_set_mode, };
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 11/15 iio: adc: Split AD7091R-5 config symbol
In reply to Marcelo SchmittSplit AD7091R-5 kconfig symbol into one symbol for the base AD7091R driver and another one for the I2C interface AD7091R-5 driver.
--- drivers/iio/adc/Kconfig | 4 ++++ drivers/iio/adc/Makefile | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-)
drivers/iio/adc/Kconfig +4 -0
@@ -36,9 +36,13 @@ config AD4130 To compile this driver as a module, choose M here: the module will be called ad4130. +config AD7091R + tristate + config AD7091R5 tristate "Analog Devices AD7091R5 ADC Driver" depends on I2C + select AD7091R select REGMAP_I2C help Say yes here to build support for Analog Devices AD7091R-5 ADC.
drivers/iio/adc/Makefile +2 -1
@@ -7,7 +7,8 @@ obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o obj-$(CONFIG_AD4130) += ad4130.o -obj-$(CONFIG_AD7091R5) += ad7091r5.o ad7091r-base.o +obj-$(CONFIG_AD7091R) += ad7091r-base.o +obj-$(CONFIG_AD7091R5) += ad7091r5.o obj-$(CONFIG_AD7124) += ad7124.o obj-$(CONFIG_AD7192) += ad7192.o obj-$(CONFIG_AD7266) += ad7266.o
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 12/15 dt-bindings: iio: Add AD7091R-8
In reply to Marcelo SchmittAdd device tree documentation for AD7091R-8.
--- .../bindings/iio/adc/adi,ad7091r5.yaml | 82 ++++++++++++++++++- 1 file changed, 78 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/iio/adc/adi,ad7091r5.yaml +78 -4
@@ -4,36 +4,92 @@ $id: http://devicetree.org/schemas/iio/adc/adi,ad7091r5.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# -title: Analog Devices AD7091R5 4-Channel 12-Bit ADC +title: Analog Devices AD7091R-2/-4/-5/-8 Multi-Channel 12-Bit ADCs maintainers: - Michael Hennerich <[email protected]> + - Marcelo Schmitt <[email protected]> description: | - Analog Devices AD7091R5 4-Channel 12-Bit ADC + Analog Devices AD7091R5 4-Channel 12-Bit ADC supporting I2C interface https://www.analog.com/media/en/technical-documentation/data-sheets/ad7091r-5.pdf + Analog Devices AD7091R-2/AD7091R-4/AD7091R-8 2-/4-/8-Channel 12-Bit ADCs + supporting SPI interface + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7091R-2_7091R-4_7091R-8.pdf properties: compatible: enum: + - adi,ad7091r2 + - adi,ad7091r4 - adi,ad7091r5 + - adi,ad7091r8 reg: maxItems: 1 + vdd-supply: + description: + Provide VDD power to the sensor (VDD range is from 2.7V to 5.25V). + + vdrive-supply: + description: + Determines the voltage level at which the interface logic will operate. + The V_drive voltage range is from 1.8V to 5.25V and must not exceed VDD by + more than 0.3V. + vref-supply: description: Phandle to the vref power supply - interrupts: + convst-gpios: + description: + GPIO connected to the CONVST pin. + This logic input is used to initiate conversions on the analog + input channels. maxItems: 1 + reset-gpios: + maxItems: 1 + + interrupts: + description: + Interrupt for signaling when conversion results exceed the high limit for + ADC readings or fall below the low limit for them. Interrupt source must + be attached to ALERT/BUSY/GPO0 pin. + maxItems: 1 required: - compatible - reg -additionalProperties: false +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + + # AD7091R-2 does not have ALERT/BUSY/GPO pin + - if: + properties: + compatible: + contains: + enum: + - adi,ad7091r2 + then: + properties: + interrupts: false + + - if: + properties: + compatible: + contains: + enum: + - adi,ad7091r2 + - adi,ad7091r4 + - adi,ad7091r8 + then: + required: + - convst-gpios + +unevaluatedProperties: false examples: - |
@@ -51,4 +107,22 @@ examples: interrupt-parent = <&gpio>; }; }; + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad7091r8"; + reg = <0x0>; + spi-max-frequency = <1000000>; + vref-supply = <&adc_vref>; + convst-gpios = <&gpio 25 GPIO_ACTIVE_LOW>; + reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>; + interrupts = <22 IRQ_TYPE_EDGE_FALLING>; + interrupt-parent = <&gpio>; + }; + }; ...
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 13/15 iio: adc: Add support for AD7091R-8
In reply to Marcelo SchmittAdd support for Analog Devices AD7091R-2, AD7091R-4, and AD7091R-8 low power 12-Bit SAR ADCs with SPI interface.
Extend ad7091r-base driver so it can be used by the AD7091R-8 driver.
--- drivers/iio/adc/Kconfig | 12 ++ drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ad7091r-base.c | 7 + drivers/iio/adc/ad7091r-base.h | 7 + drivers/iio/adc/ad7091r8.c | 257 +++++++++++++++++++++++++++++++++ 5 files changed, 284 insertions(+) create mode 100644 drivers/iio/adc/ad7091r8.c
drivers/iio/adc/Kconfig +12 -0
@@ -47,6 +47,18 @@ config AD7091R5 help Say yes here to build support for Analog Devices AD7091R-5 ADC. +config AD7091R8 + tristate "Analog Devices AD7091R8 ADC Driver" + depends on SPI + select AD7091R + select REGMAP_SPI + help + Say yes here to build support for Analog Devices AD7091R-2, AD7091R-4, + and AD7091R-8 ADC. + + To compile this driver as a module, choose M here: the module will be + called ad7091r8. + config AD7124 tristate "Analog Devices AD7124 and similar sigma-delta ADCs driver" depends on SPI_MASTER
drivers/iio/adc/Makefile +1 -0
@@ -9,6 +9,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o obj-$(CONFIG_AD4130) += ad4130.o obj-$(CONFIG_AD7091R) += ad7091r-base.o obj-$(CONFIG_AD7091R5) += ad7091r5.o +obj-$(CONFIG_AD7091R8) += ad7091r8.o obj-$(CONFIG_AD7124) += ad7124.o obj-$(CONFIG_AD7192) += ad7192.o obj-$(CONFIG_AD7266) += ad7266.o
drivers/iio/adc/ad7091r-base.c +7 -0
@@ -6,6 +6,7 @@*/ #include <linux/bitops.h> +#include <linux/bitfield.h> #include <linux/iio/events.h> #include <linux/iio/iio.h> #include <linux/interrupt.h>
@@ -187,6 +188,12 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info, iio_dev->info = &ad7091r_info; iio_dev->modes =INDIO_DIRECT_MODE ; +if (init_info->setup) { + ret = init_info->setup (st); +if (ret <0 ) +return ret; + } +if (irq) { st->chip_info = &init_info->irq_info; ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
drivers/iio/adc/ad7091r-base.h +7 -0
@@ -45,6 +45,8 @@ .scan_type.realbits = bits, \ } +#include <linux/gpio/consumer.h> +struct device ;enum ad7091r_mode {
@@ -56,10 +58,14 @@ enum ad7091r_mode {struct ad7091r_state {struct device *dev;struct regmap *map; +struct gpio_desc *convst_gpio; +struct gpio_desc *reset_gpio;struct regulator *vref;const struct ad7091r_chip_info *chip_info;enum ad7091r_mode mode;struct mutex lock;/*lock to prevent concurent reads */ +__be16 tx_buf__aligned(IIO_DMA_MINALIGN) ; +__be16 rx_buf; };struct ad7091r_chip_info {
@@ -77,6 +83,7 @@ struct ad7091r_init_info {const struct regmap_config *regmap_config;void (*init_adc_regmap)(struct ad7091r_state *st,const struct regmap_config *regmap_conf); +int (*setup)(struct ad7091r_state *st); };extern const struct iio_event_spec ad7091r_events[3 ];
drivers/iio/adc/ad7091r8.c new file +257 -0
257 lines not on this page · raw message
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 14/15 MAINTAINERS: Add MAINTAINERS entry for AD7091R
In reply to Marcelo SchmittThe driver for AD7091R was added inca693001: iio: adc: Add support for AD7091R5 ADC but no MAINTAINERS file entry was added for it since then. Add a proper MAINTAINERS file entry for the AD7091R driver.
--- MAINTAINERS | 8 ++++++++ 1 file changed, 8 insertions(+)
MAINTAINERS +8 -0
@@ -1126,6 +1126,14 @@ F: Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 F: Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml F: drivers/iio/adc/ad4130.c +ANALOG DEVICES INC AD7091R DRIVER +M: Marcelo Schmitt <[email protected]> +L: [email protected] +S: Supported +W: http://ez.analog.com/community/linux-device-drivers +F: Documentation/devicetree/bindings/iio/adc/adi,ad7091r* +F: drivers/iio/adc/drivers/iio/adc/ad7091r* + ANALOG DEVICES INC AD7192 DRIVER M: Alexandru Tachici <[email protected]> L: [email protected]
Signature · 1 line
2.42.0
Marcelo Schmitt
maintainerPATCH v4 15/15 iio: adc: ad7091r: Allow users to configure device events
In reply to Marcelo SchmittImplement event configuration callbacks allowing users to read/write event thresholds and enable/disable event generation.
---
This is from a review suggestion David made on v3 [1].
Is this the case for a Suggested-by tag?
drivers/iio/adc/ad7091r-base.c | 117 +++++++++++++++++++++++++++++++-- 1 file changed, 113 insertions(+), 4 deletions(-)
drivers/iio/adc/ad7091r-base.c +113 -4
@@ -20,19 +20,18 @@ const struct iio_event_spec ad7091r_events[] = { { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_RISING, - .mask_separate = BIT(IIO_EV_INFO_VALUE) | - BIT(IIO_EV_INFO_ENABLE), + .mask_separate = BIT(IIO_EV_INFO_VALUE), }, { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_FALLING, - .mask_separate = BIT(IIO_EV_INFO_VALUE) | - BIT(IIO_EV_INFO_ENABLE), + .mask_separate = BIT(IIO_EV_INFO_VALUE), }, { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_EITHER, .mask_separate = BIT(IIO_EV_INFO_HYSTERESIS), + .mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE), }, }; EXPORT_SYMBOL_NS_GPL(ad7091r_events, IIO_AD7091R);
@@ -128,8 +127,118 @@ static int ad7091r_read_raw(struct iio_dev *iio_dev,return ret; } +static int ad7091r_read_event_config (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir) +{ +struct ad7091r_state *st =iio_priv (indio_dev); +unsigned int alert; +int ret; + + ret =regmap_read (st->map,AD7091R_REG_CONF , &alert); +if (ret) +return ret; + +return !!(FIELD_GET (AD7091R_REG_CONF_ALERT_EN , alert)); +} + +static int ad7091r_write_event_config (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir,int state) +{ +struct ad7091r_state *st =iio_priv (indio_dev); + +/* +* Whenever alerts are enabled, every ADC conversion will generate +* an alert if the conversion result for a particular channel falls +* bellow the respective low limit register or above the respective +* high limit register. +* We can enable or disable all alerts by writing to the config reg. +*/ +return regmap_update_bits (st->map,AD7091R_REG_CONF , +AD7091R_REG_CONF_ALERT_EN , state <<4 ); +} + +static int ad7091r_read_event_value (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +enum iio_event_info info,int *val,int *val2) +{ +struct ad7091r_state *st =iio_priv (indio_dev); +int ret; + +switch (info) { +case IIO_EV_INFO_VALUE : +switch (dir) { +case IIO_EV_DIR_RISING : + ret =regmap_read (st->map, +AD7091R_REG_CH_HIGH_LIMIT (chan->channel), + val); +if (ret) +return ret; +return IIO_VAL_INT ; +case IIO_EV_DIR_FALLING : + ret =regmap_read (st->map, +AD7091R_REG_CH_LOW_LIMIT (chan->channel), + val); +if (ret) +return ret; +return IIO_VAL_INT ; +default : +return -EINVAL ; + } +case IIO_EV_INFO_HYSTERESIS : + ret =regmap_read (st->map, +AD7091R_REG_CH_HYSTERESIS (chan->channel), + val); +if (ret) +return ret; +return IIO_VAL_INT ; +default : +return -EINVAL ; + } +} + +static int ad7091r_write_event_value (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +enum iio_event_info info,int val,int val2) +{ +struct ad7091r_state *st =iio_priv (indio_dev); + +switch (info) { +case IIO_EV_INFO_VALUE : +switch (dir) { +case IIO_EV_DIR_RISING : +return regmap_write (st->map, +AD7091R_REG_CH_HIGH_LIMIT (chan->channel), + val); +case IIO_EV_DIR_FALLING : +return regmap_write (st->map, +AD7091R_REG_CH_LOW_LIMIT (chan->channel), + val); +default : +return -EINVAL ; + } +case IIO_EV_INFO_HYSTERESIS : +return regmap_write (st->map, +AD7091R_REG_CH_HYSTERESIS (chan->channel), + val); +default : +return -EINVAL ; + } +} +static const struct iio_info ad7091r_info = { .read_raw = ad7091r_read_raw, + .read_event_config = &ad7091r_read_event_config, + .write_event_config = &ad7091r_write_event_config, + .read_event_value = &ad7091r_read_event_value, + .write_event_value = &ad7091r_write_event_value, };static irqreturn_t ad7091r_event_handler (int irq,void *private)
Signature · 1 line
2.42.0
Re: [PATCH v4 01/15] scripts: checkpatch: Add __aligned to the list of attribute notes
In reply to Marcelo SchmittQuoted 11 lines · Marcelo Schmitt
Checkpatch presumes attributes marked with
__aligned(alignment)are part of a function declaration and throws a warning stating that those compiler attributes should have an identifier name which is not correct. Add__alignedcompiler attributes to the list of attribute notes so they don't cause warnings anymore.Signed-off-by Marcelo Schmitt <[email protected]>---
Any expression that evaluates to an integer that is a power of 2 can be within__alignedparenthesis.
I can't see how we could use a regex to check code meets such constraint (if possible).
You can't because if a #define is used for the alignment value it is not necessarily available to a patch fragment or even a file if the #define is in an #include.
Quoted 3 lines
Some additional exotic uses of
__alignedare:drivers/net/wireless/quantenna/qtnfmac/bus.h:72: char bus_priv[]__aligned(sizeof(void *));include/linux/netdevice.h:225:}__aligned(4 * sizeof(unsigned long));
Right, then there are random uses like:
drivers/firmware/qcom/qcom_qseecom_uefisecapp.c: size_t __aligned; \drivers/firmware/qcom/qcom_qseecom_uefisecapp.c: *__offset = __aligned; \
so there's always some false positive/negative issue with checkpatch.
Anyway:
Quoted 26 lines
The regex
__aligned\s*\(.*\)seems to match all use cases.
We might not catch invalid arguments to
__aligned, but it looks like making checkpath confidently report those wouldn't be feasible anyway.The patch that would trigger the mentioned warning in now patch number 13 (iio: adc: Add support for AD7091R-8).
scripts/checkpatch.pl | 1 + 1 file changed, 1 insertion(+)scripts/checkpatch.pl +1
-0@@ -512,6 +512,7 @@ our $Attribute = qr{ __ro_after_init| __kprobes| $InitAttribute| + __aligned\s*\(.*\)| ____cacheline_aligned| ____cacheline_aligned_in_smp| ____cacheline_internodealigned_in_smp|
Re: [PATCH v4 12/15] dt-bindings: iio: Add AD7091R-8
In reply to Marcelo SchmittQuoted 3 lines · Marcelo Schmitt
Add device tree documentation for AD7091R-8.
Signed-off-by Marcelo Schmitt <[email protected]>
Cheers,
Conor.
Quoted 131 lines
--- .../bindings/iio/adc/adi,ad7091r5.yaml | 82 ++++++++++++++++++- 1 file changed, 78 insertions(+), 4 deletions(-)Documentation/devicetree/bindings/iio/adc/adi,ad7091r5.yaml +78
-4@@ -4,36 +4,92 @@ $id: http://devicetree.org/schemas/iio/adc/adi,ad7091r5.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# -title: Analog Devices AD7091R5 4-Channel 12-Bit ADC +title: Analog Devices AD7091R-2/-4/-5/-8 Multi-Channel 12-Bit ADCs maintainers: - Michael Hennerich <[email protected]> + - Marcelo Schmitt <[email protected]> description: | - Analog Devices AD7091R5 4-Channel 12-Bit ADC + Analog Devices AD7091R5 4-Channel 12-Bit ADC supporting I2C interface https://www.analog.com/media/en/technical-documentation/data-sheets/ad7091r-5.pdf + Analog Devices AD7091R-2/AD7091R-4/AD7091R-8 2-/4-/8-Channel 12-Bit ADCs + supporting SPI interface + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7091R-2_7091R-4_7091R-8.pdf properties: compatible: enum: + - adi,ad7091r2 + - adi,ad7091r4 - adi,ad7091r5 + - adi,ad7091r8 reg: maxItems: 1 + vdd-supply: + description: + Provide VDD power to the sensor (VDD range is from 2.7V to 5.25V). + + vdrive-supply: + description: + Determines the voltage level at which the interface logic will operate. + The V_drive voltage range is from 1.8V to 5.25V and must not exceed VDD by + more than 0.3V. + vref-supply: description: Phandle to the vref power supply - interrupts: + convst-gpios: + description: + GPIO connected to the CONVST pin. + This logic input is used to initiate conversions on the analog + input channels. maxItems: 1 + reset-gpios: + maxItems: 1 + + interrupts: + description: + Interrupt for signaling when conversion results exceed the high limit for + ADC readings or fall below the low limit for them. Interrupt source must + be attached to ALERT/BUSY/GPO0 pin. + maxItems: 1 required: - compatible - reg -additionalProperties: false +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + + # AD7091R-2 does not have ALERT/BUSY/GPO pin + - if: + properties: + compatible: + contains: + enum: + - adi,ad7091r2 + then: + properties: + interrupts: false + + - if: + properties: + compatible: + contains: + enum: + - adi,ad7091r2 + - adi,ad7091r4 + - adi,ad7091r8 + then: + required: + - convst-gpios + +unevaluatedProperties: false examples: - |@@ -51,4 +107,22 @@ examples: interrupt-parent = <&gpio>; }; }; + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad7091r8"; + reg = <0x0>; + spi-max-frequency = <1000000>; + vref-supply = <&adc_vref>; + convst-gpios = <&gpio 25 GPIO_ACTIVE_LOW>; + reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>; + interrupts = <22 IRQ_TYPE_EDGE_FALLING>; + interrupt-parent = <&gpio>; + }; + };Signature · 1 line
2.42.0
kernel test robot
build botRe: [PATCH v4 6/15] iio: adc: ad7091r: Move chip init data to container struct
In reply to Marcelo Schmittkernel test robot noticed the following build errors:
Hi Marcelo,
kernel test robot noticed the following build errors:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.7-rc5 next-20231215] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Marcelo-Schmitt/scripts-checkpatch-Add-__aligned-to-the-list-of-attribute-notes/20231217-055420 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/9d1248860193b55e79640b2e64c21c66bd6645f9.1702746240.git.marcelo.schmitt1%40gmail.com patch subject: [PATCH v4 6/15] iio: adc: ad7091r: Move chip init data to container struct config: um-randconfig-r111-20231217 (https://download.01.org/0day-ci/archive/20231217/[email protected]/config) compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231217/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> drivers/iio/adc/ad7091r5.c:59:14: error: initializer element is not constant
.irq_info = ad7091r5_chip_info_irq,
^~~~~~~~~~~~~~~~~~~~~~
drivers/iio/adc/ad7091r5.c:59:14: note: (near initialization for 'ad7091r5_init_info.irq_info')
drivers/iio/adc/ad7091r5.c:60:17: error: initializer element is not constant
.info_no_irq = ad7091r5_chip_info_noirq,
^~~~~~~~~~~~~~~~~~~~~~~~ drivers/iio/adc/ad7091r5.c:60:17: note: (near initialization for 'ad7091r5_init_info.info_no_irq')
vim +59 drivers/iio/adc/ad7091r5.c
57
58 static struct ad7091r_init_info ad7091r5_init_info = {
> 59 .irq_info = ad7091r5_chip_info_irq,
60 .info_no_irq = ad7091r5_chip_info_noirq,
61 .regmap_config = &ad7091r_regmap_config,
62 .init_adc_regmap = &ad7091r5_regmap_init
63 };
64 Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Jonathan Cameron
maintainerRe: [PATCH v4 6/15] iio: adc: ad7091r: Move chip init data to container struct
Quoted 25 lines · On Sun, 17 Dec 2023 21:04:50 +0800 kernel test robot <[email protected]> wrote:
Hi Marcelo,
kernel test robot noticed the following build errors:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.7-rc5 next-20231215] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]url: https://github.com/intel-lab-lkp/linux/commits/Marcelo-Schmitt/scripts-checkpatch-Add-__aligned-to-the-list-of-attribute-notes/20231217-055420 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/9d1248860193b55e79640b2e64c21c66bd6645f9.1702746240.git.marcelo.schmitt1%40gmail.com patch subject: [PATCH v4 6/15] iio: adc: ad7091r: Move chip init data to container struct config: um-randconfig-r111-20231217 (https://download.01.org/0day-ci/archive/20231217/[email protected]/config) compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231217/[email protected]/reproduce)If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/All errors (new ones prefixed by >>):
Quoted 7 lines
Quoted 1 line
Quoted 1 line
drivers/iio/adc/ad7091r5.c:59:14: error: initializer element is not constant.irq_info = ad7091r5_chip_info_irq, ^~~~~~~~~~~~~~~~~~~~~~ drivers/iio/adc/ad7091r5.c:59:14: note: (near initialization for 'ad7091r5_init_info.irq_info') drivers/iio/adc/ad7091r5.c:60:17: error: initializer element is not constant .info_no_irq = ad7091r5_chip_info_noirq, ^~~~~~~~~~~~~~~~~~~~~~~~
You should use a pointer rather than assigning the whole structure.
Quoted 14 lines
drivers/iio/adc/ad7091r5.c:60:17: note: (near initialization for 'ad7091r5_init_info.info_no_irq')vim +59
drivers/iio/adc/ad7091r5.c57 58 static struct ad7091r_init_info ad7091r5_init_info = {Quoted 1 line
59 .irq_info =
ad7091r5_chip_info_irq,60 .info_no_irq = ad7091r5_chip_info_noirq, 61 .regmap_config = &ad7091r_regmap_config, 62 .init_adc_regmap = &ad7091r5_regmap_init 63 }; 64
Jonathan Cameron
maintainerRe: [PATCH v4 01/15] scripts: checkpatch: Add __aligned to the list of attribute notes
In reply to Joe PerchesQuoted 26 lines · On Sat, 16 Dec 2023 10:07:24 -0800 Joe Perches <[email protected]> wrote:
Quoted 11 lines · Marcelo Schmitt
Checkpatch presumes attributes marked with
__aligned(alignment)are part of a function declaration and throws a warning stating that those compiler attributes should have an identifier name which is not correct. Add__alignedcompiler attributes to the list of attribute notes so they don't cause warnings anymore.Signed-off-by Marcelo Schmitt <[email protected]>---
Any expression that evaluates to an integer that is a power of 2 can be within__alignedparenthesis.
I can't see how we could use a regex to check code meets such constraint (if possible).You can't because if a #define is used for the alignment value it is not necessarily available to a patch fragment or even a file if the #define is in an #include.
Quoted 3 lines
Some additional exotic uses of
__alignedare:drivers/net/wireless/quantenna/qtnfmac/bus.h:72: char bus_priv[]__aligned(sizeof(void *));include/linux/netdevice.h:225:}__aligned(4 * sizeof(unsigned long));Right, then there are random uses like:
drivers/firmware/qcom/qcom_qseecom_uefisecapp.c:size_t__aligned; \drivers/firmware/qcom/qcom_qseecom_uefisecapp.c:*__offset=__aligned; \
Quoted 6 lines
so there's always some false positive/negative issue with checkpatch.
Anyway:
Acked-by Joe Perches <[email protected]>
Given this should cut down on false postives I've picked this one up now.
Applied to the togreg branch of iio.git and pushed out briefly as testing to see what blows up.
I'll see if I can pick up some more parts of this series to avoid us going round again with quite so many patches.
Jonathan
Quoted 29 lines
Quoted 27 lines
The regex
__aligned\s*\(.*\)seems to match all use cases.
We might not catch invalid arguments to
__aligned, but it looks like making checkpath confidently report those wouldn't be feasible anyway.The patch that would trigger the mentioned warning in now patch number 13 (iio: adc: Add support for AD7091R-8).
scripts/checkpatch.pl | 1 + 1 file changed, 1 insertion(+)scripts/checkpatch.pl +1
-0@@ -512,6 +512,7 @@ our $Attribute = qr{ __ro_after_init| __kprobes| $InitAttribute| + __aligned\s*\(.*\)| ____cacheline_aligned| ____cacheline_aligned_in_smp| ____cacheline_internodealigned_in_smp|
Jonathan Cameron
maintainerRe: [PATCH v4 02/15] iio: adc: ad7091r: Pass iio_dev to event handler
In reply to Marcelo SchmittQuoted 11 lines · On Sat, 16 Dec 2023 14:46:11 -0300 Marcelo Schmitt…
Previous version of ad7091r event handler received the ADC state pointer and retrieved the iio device from driver data field with
dev_get_drvdata(). However, no driver data have ever been set, which led to null pointer dereference when running the event handler.Pass the iio device to the event handler and retrieve the ADC state struct from it so we avoid the null pointer dereference and save the driver from filling the driver data field.
Fixes ca69300173b6 ("iio: adc: Add support for AD7091R5 ADC")Signed-off-by Marcelo Schmitt <[email protected]>
Given we are late in the cycle, I've applied this to the togreg branch of iio.git and it will hopefully go in during the merge window rather than before. Marked it for stable though so should get backported appropriately.
Thanks,
Jonathan
Quoted 28 lines
--- drivers/iio/adc/ad7091r-base.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/iio/adc/ad7091r-base.c +3
-3@@ -174,8 +174,8 @@ static const struct iio_info ad7091r_info = {static irqreturn_t ad7091r_event_handler (int irq,void *private) { -struct ad7091r_state *st = (struct ad7091r_state *) private; -struct iio_dev *iio_dev =dev_get_drvdata (st->dev); +struct iio_dev *iio_dev = private; +struct ad7091r_state *st =iio_priv (iio_dev);unsigned int i, read_val;int ret;s64 timestamp =iio_get_time_ns (iio_dev);@@ -234,7 +234,7 @@ int ad7091r_probe(struct device *dev, const char *name,if (irq) { ret =devm_request_threaded_irq (dev, irq,NULL , ad7091r_event_handler, -IRQF_TRIGGER_FALLING |IRQF_ONESHOT , name, st); +IRQF_TRIGGER_FALLING |IRQF_ONESHOT , name, iio_dev);if (ret)return ret; }
Jonathan Cameron
maintainerRe: [PATCH v4 03/15] iio: adc: ad7091r: Set alert bit in config register
In reply to Marcelo SchmittQuoted 9 lines · On Sat, 16 Dec 2023 14:46:37 -0300 Marcelo Schmitt…
The ad7091r-base driver sets up an interrupt handler for firing events when inputs are either above or below a certain threshold. However, for the interrupt signal to come from the device it must be configured to enable the ALERT/BUSY/GPO pin to be used as ALERT, which was not being done until now.
Enable interrupt signals on the ALERT/BUSY/GPO pin by setting the proper bit in the configuration register.Signed-off-by Marcelo Schmitt <[email protected]>
Hi Marcelo,
In V3 review I asked if this should have a fixes tag. I've assumed for now the answer is no and applied it without. If you let me know fast enough I can probably slip on in, but if not you may want to consider requesting a backport after this is upstream.
Applied to the togreg branch of iio.git and pushed out as testing for 0-day to take a look at it.
Thanks,
Jonathan
Quoted 28 lines
--- drivers/iio/adc/ad7091r-base.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/iio/adc/ad7091r-base.c +6
-0@@ -28,6 +28,7 @@#define AD7091R_REG_RESULT_CONV_RESULT (x) ((x) & 0xfff)/* AD7091R_REG_CONF */ +#define AD7091R_REG_CONF_ALERT_EN BIT(4)#define AD7091R_REG_CONF_AUTO BIT(8)#define AD7091R_REG_CONF_CMD BIT(10)@@ -232,6 +233,11 @@ int ad7091r_probe(struct device *dev, const char *name, iio_dev->channels = chip_info->channels;if (irq) { + ret =regmap_update_bits (st->map,AD7091R_REG_CONF , +AD7091R_REG_CONF_ALERT_EN ,BIT (4 )); +if (ret) +return ret; + ret =devm_request_threaded_irq (dev, irq,NULL , ad7091r_event_handler,IRQF_TRIGGER_FALLING |IRQF_ONESHOT , name, iio_dev);
Re: [PATCH v4 04/15] iio: adc: ad7091r: Align arguments to function call parenthesis
In reply to Marcelo SchmittQuoted 4 lines · On Sat, 16 Dec 2023 14:47:01 -0300 Marcelo Schmitt…
Align arguments to function call open parenthesis to comply with the Linux kernel coding style.
Signed-off-by Marcelo Schmitt <[email protected]>
Applied
Quoted 20 lines
--- drivers/iio/adc/ad7091r-base.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/iio/adc/ad7091r-base.c +3
-2@@ -239,8 +239,9 @@ int ad7091r_probe(struct device *dev, const char *name,return ret; ret =devm_request_threaded_irq (dev, irq,NULL , - ad7091r_event_handler, -IRQF_TRIGGER_FALLING |IRQF_ONESHOT , name, iio_dev); + ad7091r_event_handler, +IRQF_TRIGGER_FALLING | +IRQF_ONESHOT , name, iio_dev);if (ret)return ret; }
Jonathan Cameron
maintainerRe: [PATCH v4 05/15] iio: adc: ad7091r: Move generic AD7091R code to base driver and header file
In reply to Marcelo SchmittQuoted 10 lines · On Sat, 16 Dec 2023 14:47:25 -0300 Marcelo Schmitt…
Some code generic to AD7091R devices such as channel definitions and event spec structs were in the AD7091R-5 driver.
There was also some generic register definitions declared in the base driver which would make more sense to be in the header file. The device state struct will be needed for the ad7091r8 driver in a follow up patch so that ought to be moved to the header file as well. Lastly, a couple of regmap callback functions are also capable of abstracting characteristics of different AD7091R devices and those are now being exported toIIO_AD7091Rname space.
Quoted 5 lines
Move AD7091R generic code either to the base driver or to the header file so both the ad7091r5 and the ad7091r8 driver can use those declaration in follow up patches.
Signed-off-by Marcelo Schmitt <[email protected]>
Hi Marcelo
I'm going to stop here as can't apply next patch because of build warnings and this one is hard to justify without the rest of the series.
So please just include this patch onwards in v5.
This looks fine to me btw. I'll just comment on patches where I have anything to add. If not they look fine.
Jonathan
Quoted 209 lines
--- drivers/iio/adc/ad7091r-base.c | 46 +++++++++++++++++----------------- drivers/iio/adc/ad7091r-base.h | 42 ++++++++++++++++++++++++++++++- drivers/iio/adc/ad7091r5.c | 39 +++------------------------- 3 files changed, 68 insertions(+), 59 deletions(-)drivers/iio/adc/ad7091r-base.c +23
-23@@ -15,14 +15,6 @@#include "ad7091r-base.h" -#define AD7091R_REG_RESULT 0 -#define AD7091R_REG_CHANNEL 1 -#define AD7091R_REG_CONF 2 -#define AD7091R_REG_ALERT 3 -#define AD7091R_REG_CH_LOW_LIMIT (ch) ((ch) * 3 + 4) -#define AD7091R_REG_CH_HIGH_LIMIT (ch) ((ch) * 3 + 5) -#define AD7091R_REG_CH_HYSTERESIS (ch) ((ch) * 3 + 6) -/* AD7091R_REG_RESULT */ #define AD7091R_REG_RESULT_CH_ID (x) (((x) >> 13) & 0x3)#define AD7091R_REG_RESULT_CONV_RESULT (x) ((x) & 0xfff)@@ -35,20 +27,26 @@#define AD7091R_REG_CONF_MODE_MASK \ (AD7091R_REG_CONF_AUTO | AD7091R_REG_CONF_CMD) -enum ad7091r_mode { -AD7091R_MODE_SAMPLE , -AD7091R_MODE_COMMAND , -AD7091R_MODE_AUTOCYCLE , -}; - -struct ad7091r_state { -struct device *dev; -struct regmap *map; -struct regulator *vref; -const struct ad7091r_chip_info *chip_info; -enum ad7091r_mode mode; -struct mutex lock;/*lock to prevent concurent reads */ +const struct iio_event_spec ad7091r_events[] = { + { + .type =IIO_EV_TYPE_THRESH , + .dir =IIO_EV_DIR_RISING , + .mask_separate =BIT (IIO_EV_INFO_VALUE ) | +BIT (IIO_EV_INFO_ENABLE ), + }, + { + .type =IIO_EV_TYPE_THRESH , + .dir =IIO_EV_DIR_FALLING , + .mask_separate =BIT (IIO_EV_INFO_VALUE ) | +BIT (IIO_EV_INFO_ENABLE ), + }, + { + .type =IIO_EV_TYPE_THRESH , + .dir =IIO_EV_DIR_EITHER , + .mask_separate =BIT (IIO_EV_INFO_HYSTERESIS ), + }, }; +EXPORT_SYMBOL_NS_GPL (ad7091r_events,IIO_AD7091R );static int ad7091r_set_mode (struct ad7091r_state *st,enum ad7091r_mode mode) {@@ -269,7 +267,7 @@ int ad7091r_probe(struct device *dev, const char *name, }EXPORT_SYMBOL_NS_GPL (ad7091r_probe,IIO_AD7091R ); -static bool ad7091r_writeable_reg (struct device *dev,unsigned int reg) +bool ad7091r_writeable_reg (struct device *dev,unsigned int reg) {switch (reg) {case AD7091R_REG_RESULT :@@ -279,8 +277,9 @@ static bool ad7091r_writeable_reg(struct device *dev, unsigned int reg)return true; } } +EXPORT_SYMBOL_NS_GPL (ad7091r_writeable_reg,IIO_AD7091R ); -static bool ad7091r_volatile_reg (struct device *dev,unsigned int reg) +bool ad7091r_volatile_reg (struct device *dev,unsigned int reg) {switch (reg) {case AD7091R_REG_RESULT :@@ -290,6 +289,7 @@ static bool ad7091r_volatile_reg(struct device *dev, unsigned int reg)return false; } } +EXPORT_SYMBOL_NS_GPL (ad7091r_volatile_reg,IIO_AD7091R );const struct regmap_config ad7091r_regmap_config = { .reg_bits =8 ,drivers/iio/adc/ad7091r-base.h +41
-1@@ -8,8 +8,43 @@ #ifndef __DRIVERS_IIO_ADC_AD7091R_BASE_H__ #define __DRIVERS_IIO_ADC_AD7091R_BASE_H__ +#define AD7091R_REG_RESULT 0 +#define AD7091R_REG_CHANNEL 1 +#define AD7091R_REG_CONF 2 +#define AD7091R_REG_ALERT 3 + +#define AD7091R_REG_CH_LOW_LIMIT(ch) ((ch) * 3 + 4) +#define AD7091R_REG_CH_HIGH_LIMIT(ch) ((ch) * 3 + 5) +#define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6) + +#define AD7091R_CHANNEL(idx, bits, ev, num_ev) { \ + .type = IIO_VOLTAGE, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .indexed = 1, \ + .channel = idx, \ + .event_spec = ev, \ + .num_event_specs = num_ev, \ + .scan_type.storagebits = 16, \ + .scan_type.realbits = bits, \ +} + struct device; -struct ad7091r_state; + +enum ad7091r_mode { + AD7091R_MODE_SAMPLE, + AD7091R_MODE_COMMAND, + AD7091R_MODE_AUTOCYCLE, +}; + +struct ad7091r_state { + struct device *dev; + struct regmap *map; + struct regulator *vref; + const struct ad7091r_chip_info *chip_info; + enum ad7091r_mode mode; + struct mutex lock; /*lock to prevent concurent reads */ +}; struct ad7091r_chip_info { unsigned int num_channels;@@ -17,10 +52,15 @@ struct ad7091r_chip_info { unsigned int vref_mV; }; +extern const struct iio_event_spec ad7091r_events[3 ]; +extern const struct regmap_config ad7091r_regmap_config;int ad7091r_probe (struct device *dev,const char *name,const struct ad7091r_chip_info *chip_info,struct regmap *map,int irq); +bool ad7091r_volatile_reg (struct device *dev,unsigned int reg); +bool ad7091r_writeable_reg (struct device *dev,unsigned int reg); +#endif /* __DRIVERS_IIO_ADC_AD7091R_BASE_H__ */ drivers/iio/adc/ad7091r5.c +4
-35@@ -12,42 +12,11 @@#include "ad7091r-base.h" -static const struct iio_event_spec ad7091r5_events[] = { - { - .type =IIO_EV_TYPE_THRESH , - .dir =IIO_EV_DIR_RISING , - .mask_separate =BIT (IIO_EV_INFO_VALUE ) | -BIT (IIO_EV_INFO_ENABLE ), - }, - { - .type =IIO_EV_TYPE_THRESH , - .dir =IIO_EV_DIR_FALLING , - .mask_separate =BIT (IIO_EV_INFO_VALUE ) | -BIT (IIO_EV_INFO_ENABLE ), - }, - { - .type =IIO_EV_TYPE_THRESH , - .dir =IIO_EV_DIR_EITHER , - .mask_separate =BIT (IIO_EV_INFO_HYSTERESIS ), - }, -}; - -#define AD7091R_CHANNEL (idx, bits, ev, num_ev) { \ - .type = IIO_VOLTAGE, \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ - .indexed = 1, \ - .channel = idx, \ - .event_spec = ev, \ - .num_event_specs = num_ev, \ - .scan_type.storagebits = 16, \ - .scan_type.realbits = bits, \ -}static const struct iio_chan_spec ad7091r5_channels_irq[] = { -AD7091R_CHANNEL (0 ,12 , ad7091r5_events,ARRAY_SIZE (ad7091r5_events)), -AD7091R_CHANNEL (1 ,12 , ad7091r5_events,ARRAY_SIZE (ad7091r5_events)), -AD7091R_CHANNEL (2 ,12 , ad7091r5_events,ARRAY_SIZE (ad7091r5_events)), -AD7091R_CHANNEL (3 ,12 , ad7091r5_events,ARRAY_SIZE (ad7091r5_events)), +AD7091R_CHANNEL (0 ,12 , ad7091r_events,ARRAY_SIZE (ad7091r_events)), +AD7091R_CHANNEL (1 ,12 , ad7091r_events,ARRAY_SIZE (ad7091r_events)), +AD7091R_CHANNEL (2 ,12 , ad7091r_events,ARRAY_SIZE (ad7091r_events)), +AD7091R_CHANNEL (3 ,12 , ad7091r_events,ARRAY_SIZE (ad7091r_events)), };static const struct iio_chan_spec ad7091r5_channels_noirq[] = {
Jonathan Cameron
maintainerRe: [PATCH v4 09/15] iio: adc: ad7091r: Enable internal vref if external vref is not supplied
In reply to Marcelo SchmittQuoted 4 lines · On Sat, 16 Dec 2023 14:49:07 -0300 Marcelo Schmitt…
The ADC needs a voltage reference to work correctly.
Enable AD7091R internal voltage reference if no external vref is supplied.Signed-off-by Marcelo Schmitt <[email protected]>
This one sounds to me like it should have a fixes tag and be much earlier in the set to perhaps simplify backports.
Jonathan
Quoted 36 lines
--- drivers/iio/adc/ad7091r-base.c | 7 +++++++ drivers/iio/adc/ad7091r-base.h | 1 + 2 files changed, 8 insertions(+)drivers/iio/adc/ad7091r-base.c +7
-0@@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info,if (IS_ERR (st->vref)) {if (PTR_ERR (st->vref) == -EPROBE_DEFER )return -EPROBE_DEFER ; + st->vref =NULL ; +/* Enable internal vref */ + ret =regmap_update_bits (st->map,AD7091R_REG_CONF , +AD7091R_REG_CONF_INT_VREF ,BIT (0 )); +if (ret) +return dev_err_probe (st->dev, ret, +"Error on enable internal reference\n" ); }else { ret =regulator_enable (st->vref);if (ret)drivers/iio/adc/ad7091r-base.h +1
-0@@ -20,6 +20,7 @@#define AD7091R_REG_CH_HYSTERESIS (ch) ((ch) * 3 + 6)/* AD7091R_REG_CONF */ +#define AD7091R_REG_CONF_INT_VREF BIT(0)#define AD7091R_REG_CONF_ALERT_EN BIT(4)#define AD7091R_REG_CONF_AUTO BIT(8)#define AD7091R_REG_CONF_CMD BIT(10)
Jonathan Cameron
maintainerRe: [PATCH v4 13/15] iio: adc: Add support for AD7091R-8
In reply to Marcelo SchmittQuoted 5 lines · On Sat, 16 Dec 2023 14:50:44 -0300 Marcelo Schmitt…
Add support for Analog Devices AD7091R-2, AD7091R-4, and AD7091R-8 low power 12-Bit SAR ADCs with SPI interface.
Extend ad7091r-base driver so it can be used by the AD7091R-8 driver.Signed-off-by Marcelo Schmitt <[email protected]>
Hi Marcelo
A few trivial things from taking another look.
Jonathan
drivers/iio/adc/ad7091r-base.c line 9
Quoted 9 lines
drivers/iio/adc/ad7091r-base.c +1
-0@@ -6,6 +6,7 @@*/ #include <linux/bitops.h> +#include <linux/bitfield.h>
Not obvious what connection of this header to the code below is... Wrong patch perhaps?
Quoted 21 lines
drivers/iio/adc/ad7091r-base.c +6
-0#include <linux/iio/events.h> #include <linux/iio/iio.h> #include <linux/interrupt.h> @@ -187,6 +188,12 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info, iio_dev->info = &ad7091r_info; iio_dev->modes =INDIO_DIRECT_MODE ; +if (init_info->setup) { + ret = init_info->setup (st); +if (ret <0 ) +return ret; + } +if (irq) { st->chip_info = &init_info->irq_info; ret = regmap_update_bits(st->map, AD7091R_REG_CONF,drivers/iio/adc/ad7091r-base.h +0
-0
drivers/iio/adc/ad7091r-base.h line 49
Quoted 5 lines
drivers/iio/adc/ad7091r-base.h +2
-0@@ -45,6 +45,8 @@ .scan_type.realbits = bits, \ } +#include <linux/gpio/consumer.h> +
struct gpio_desc;
and drop the include which only provides an opaque definition anyway.
Quoted 31 lines
drivers/iio/adc/ad7091r-base.h +5
-0struct device ;enum ad7091r_mode {@@ -56,10 +58,14 @@ enum ad7091r_mode {struct ad7091r_state {struct device *dev;struct regmap *map; +struct gpio_desc *convst_gpio; +struct gpio_desc *reset_gpio;struct regulator *vref;const struct ad7091r_chip_info *chip_info;enum ad7091r_mode mode;struct mutex lock;/*lock to prevent concurent reads */ +__be16 tx_buf__aligned(IIO_DMA_MINALIGN) ; +__be16 rx_buf; };struct ad7091r_chip_info {@@ -77,6 +83,7 @@ struct ad7091r_init_info {const struct regmap_config *regmap_config;void (*init_adc_regmap)(struct ad7091r_state *st,const struct regmap_config *regmap_conf); +int (*setup)(struct ad7091r_state *st); };extern const struct iio_event_spec ad7091r_events[3 ];drivers/iio/adc/ad7091r8.c new file +0
-0
drivers/iio/adc/ad7091r8.c line 168
Quoted 10 lines
drivers/iio/adc/ad7091r8.c new file +9
-0+static int ad7091r8_gpio_setup (struct ad7091r_state *st) +{ + st->convst_gpio =devm_gpiod_get (st->dev,"adi,conversion-start" , +GPIOD_OUT_LOW ); +if (IS_ERR (st->convst_gpio)) +return dev_err_probe (st->dev,PTR_ERR (st->convst_gpio), +"Error getting convst GPIO\n" ); + + st->reset_gpio = devm_gpiod_get_optional(st->dev, "reset",
Trivial but looks like a bonus space after the =
Quoted 12 lines
drivers/iio/adc/ad7091r8.c new file +12
-0+ GPIOD_OUT_HIGH); +if (IS_ERR (st->reset_gpio)) +return dev_err_probe (st->dev,PTR_ERR (st->convst_gpio), +"Error on requesting reset GPIO\n" ); + +if (st->reset_gpio) { +fsleep (20 ); +gpiod_set_value_cansleep (st->reset_gpio,0 ); + } + +return 0 ; +}
Jonathan Cameron
maintainerRe: [PATCH v4 15/15] iio: adc: ad7091r: Allow users to configure device events
In reply to Marcelo SchmittQuoted 19 lines · On Sat, 16 Dec 2023 14:51:50 -0300 Marcelo Schmitt…
Implement event configuration callbacks allowing users to read/write event thresholds and enable/disable event generation.
Signed-off-by Marcelo Schmitt <[email protected]>---
This is from a review suggestion David made on v3 [1].Is this the case for a Suggested-by tag?
drivers/iio/adc/ad7091r-base.c | 117 +++++++++++++++++++++++++++++++-- 1 file changed, 113 insertions(+), 4 deletions(-)drivers/iio/adc/ad7091r-base.c +0
-0
drivers/iio/adc/ad7091r-base.c const struct iio_event_spec ad7091r_events[] = { · line 23
Quoted 6 lines
drivers/iio/adc/ad7091r-base.c +1
-2@@ -20,19 +20,18 @@ const struct iio_event_spec ad7091r_events[] = { { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_RISING, - .mask_separate = BIT(IIO_EV_INFO_VALUE) | - BIT(IIO_EV_INFO_ENABLE), + .mask_separate = BIT(IIO_EV_INFO_VALUE),
This is an ABI change. So would need a really strong reason to make it... mind you - it seems like this has been broken until now anyway so this change may be fine.
drivers/iio/adc/ad7091r-base.c const struct iio_event_spec ad7091r_events[] = { · line 34
Quoted 13 lines
drivers/iio/adc/ad7091r-base.c +2
-2}, { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_FALLING, - .mask_separate = BIT(IIO_EV_INFO_VALUE) | - BIT(IIO_EV_INFO_ENABLE), + .mask_separate = BIT(IIO_EV_INFO_VALUE), }, { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_EITHER, .mask_separate = BIT(IIO_EV_INFO_HYSTERESIS), + .mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE),
It's relatively unusual that you can't separate the two event directions with careful control of the thresholds. So I think you can implement the existing ABI by just setting the thresholds to the either 0 or 2^12 - 1 as appropriate. The docs seem to say it must exceed the value, or fall below the min so these values should ensure it can't do either.
You can then enable the event generate if one of them is set.
David Lechner
reviewerRe: [PATCH v4 15/15] iio: adc: ad7091r: Allow users to configure device events
In reply to Marcelo SchmittQuoted 4 lines · Marcelo Schmitt
Implement event configuration callbacks allowing users to read/write event thresholds and enable/disable event generation.
Quoted 8 lines
Signed-off-by Marcelo Schmitt <[email protected]>---
This is from a review suggestion David made on v3 [1].Is this the case for a Suggested-by tag?
I suppose it fits the definition of Suggested-by well enough and would be appreciated. Even more so on [PATCH v4 02/15] "iio: adc: ad7091r:
Pass iio_dev to event handler".
David Lechner
reviewerRe: [PATCH v4 15/15] iio: adc: ad7091r: Allow users to configure device events
Quoted 11 lines · David Lechner
Quoted 8 lines · Marcelo Schmitt
Implement event configuration callbacks allowing users to read/write event thresholds and enable/disable event generation.
Signed-off-by Marcelo Schmitt <[email protected]>---
This is from a review suggestion David made on v3 [1].
Quoted 8 lines
Quoted 4 lines
Is this the case for a Suggested-by tag?
I suppose it fits the definition of Suggested-by well enough and would be appreciated. Even more so on [PATCH v4 02/15] "iio: adc: ad7091r:
Passiio_devto event handler".
And it seems like this should have the Fixes tag since this is fixing a null pointer dereference. And the commit message should describe the problem and that this as a fix, otherwise it sounds like we are just adding a new feature here.
David Lechner
reviewerRe: [PATCH v4 15/15] iio: adc: ad7091r: Allow users to configure device events
In reply to Marcelo SchmittQuoted 51 lines · Marcelo Schmitt
Implement event configuration callbacks allowing users to read/write event thresholds and enable/disable event generation.
Signed-off-by Marcelo Schmitt <[email protected]>---
This is from a review suggestion David made on v3 [1].Is this the case for a Suggested-by tag?
drivers/iio/adc/ad7091r-base.c | 117 +++++++++++++++++++++++++++++++-- 1 file changed, 113 insertions(+), 4 deletions(-)drivers/iio/adc/ad7091r-base.c +8
-4@@ -20,19 +20,18 @@ const struct iio_event_spec ad7091r_events[] = { { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_RISING, - .mask_separate = BIT(IIO_EV_INFO_VALUE) | - BIT(IIO_EV_INFO_ENABLE), + .mask_separate = BIT(IIO_EV_INFO_VALUE), }, { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_FALLING, - .mask_separate = BIT(IIO_EV_INFO_VALUE) | - BIT(IIO_EV_INFO_ENABLE), + .mask_separate = BIT(IIO_EV_INFO_VALUE), }, { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_EITHER, .mask_separate = BIT(IIO_EV_INFO_HYSTERESIS), + .mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE), }, }; EXPORT_SYMBOL_NS_GPL(ad7091r_events, IIO_AD7091R);@@ -128,8 +127,118 @@ static int ad7091r_read_raw(struct iio_dev *iio_dev,return ret; } +static int ad7091r_read_event_config (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir) +{
drivers/iio/adc/ad7091r-base.c static int ad7091r_read_raw(struct iio_dev *iio_dev, · line 144
Quoted 10 lines
drivers/iio/adc/ad7091r-base.c +10
-0+struct ad7091r_state *st =iio_priv (indio_dev); +unsigned int alert; +int ret; + + ret =regmap_read (st->map,AD7091R_REG_CONF , &alert); +if (ret) +return ret; + +return !!(FIELD_GET (AD7091R_REG_CONF_ALERT_EN , alert)); +}
According to the datasheet, bit 4 of the config register is for selecting the function of the ALERT/BUSY/GPO0 pin as either ALERT/BUSY or GPIO, so this sounds like a pinmux function rather than an event enable function.
context: #define AD7091R_REG_CONF_ALERT_EN BIT(4) in a previous patch
Quoted 69 lines
drivers/iio/adc/ad7091r-base.c +69
-0+ +static int ad7091r_write_event_config (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir,int state) +{ +struct ad7091r_state *st =iio_priv (indio_dev); + +/* +* Whenever alerts are enabled, every ADC conversion will generate +* an alert if the conversion result for a particular channel falls +* bellow the respective low limit register or above the respective +* high limit register. +* We can enable or disable all alerts by writing to the config reg. +*/ +return regmap_update_bits (st->map,AD7091R_REG_CONF , +AD7091R_REG_CONF_ALERT_EN , state <<4 ); +} + +static int ad7091r_read_event_value (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +enum iio_event_info info,int *val,int *val2) +{ +struct ad7091r_state *st =iio_priv (indio_dev); +int ret; + +switch (info) { +case IIO_EV_INFO_VALUE : +switch (dir) { +case IIO_EV_DIR_RISING : + ret =regmap_read (st->map, +AD7091R_REG_CH_HIGH_LIMIT (chan->channel), + val); +if (ret) +return ret; +return IIO_VAL_INT ; +case IIO_EV_DIR_FALLING : + ret =regmap_read (st->map, +AD7091R_REG_CH_LOW_LIMIT (chan->channel), + val); +if (ret) +return ret; +return IIO_VAL_INT ; +default : +return -EINVAL ; + } +case IIO_EV_INFO_HYSTERESIS : + ret =regmap_read (st->map, +AD7091R_REG_CH_HYSTERESIS (chan->channel), + val); +if (ret) +return ret; +return IIO_VAL_INT ; +default : +return -EINVAL ; + } +} + +static int ad7091r_write_event_value (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +enum iio_event_info info,int val,int val2) +{ +struct ad7091r_state *st =iio_priv (indio_dev); + +switch (info) {
drivers/iio/adc/ad7091r-base.c static int ad7091r_read_raw(struct iio_dev *iio_dev, · line 223
Quoted 10 lines
drivers/iio/adc/ad7091r-base.c +10
-0+case IIO_EV_INFO_VALUE : +switch (dir) { +case IIO_EV_DIR_RISING : +return regmap_write (st->map, +AD7091R_REG_CH_HIGH_LIMIT (chan->channel), + val); +case IIO_EV_DIR_FALLING : +return regmap_write (st->map, +AD7091R_REG_CH_LOW_LIMIT (chan->channel), + val);
These registers are limited to 12-bit values. Do we need to check val first and return -EINVAL if out of range?
Quoted 24 lines
drivers/iio/adc/ad7091r-base.c +16
-1+default : +return -EINVAL ; + } +case IIO_EV_INFO_HYSTERESIS : +return regmap_write (st->map, +AD7091R_REG_CH_HYSTERESIS (chan->channel), + val); +default : +return -EINVAL ; + } +} +static const struct iio_info ad7091r_info = { .read_raw = ad7091r_read_raw, + .read_event_config = &ad7091r_read_event_config, + .write_event_config = &ad7091r_write_event_config, + .read_event_value = &ad7091r_read_event_value, + .write_event_value = &ad7091r_write_event_value, };static irqreturn_t ad7091r_event_handler (int irq,void *private) --2.42.0
David Lechner
reviewerRe: [PATCH v4 09/15] iio: adc: ad7091r: Enable internal vref if external vref is not supplied
In reply to Marcelo SchmittQuoted 15 lines · Marcelo Schmitt
The ADC needs a voltage reference to work correctly.
Enable AD7091R internal voltage reference if no external vref is supplied.Signed-off-by Marcelo Schmitt <[email protected]>--- drivers/iio/adc/ad7091r-base.c | 7 +++++++ drivers/iio/adc/ad7091r-base.h | 1 + 2 files changed, 8 insertions(+)drivers/iio/adc/ad7091r-base.c +0
-0
drivers/iio/adc/ad7091r-base.c int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info, · line 224
Quoted 8 lines
drivers/iio/adc/ad7091r-base.c +4
-0@@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info,if (IS_ERR (st->vref)) {if (PTR_ERR (st->vref) == -EPROBE_DEFER )return -EPROBE_DEFER ; + st->vref =NULL ; +/* Enable internal vref */ + ret =regmap_update_bits (st->map,AD7091R_REG_CONF , +AD7091R_REG_CONF_INT_VREF ,BIT (0 ));
Can we use regmap_set_bits() here to avoid the BIT(0)?
The same comment applies to other patches in this series.
Quoted 21 lines
drivers/iio/adc/ad7091r-base.c +3
-0+if (ret) +return dev_err_probe (st->dev, ret, +"Error on enable internal reference\n" ); }else { ret =regulator_enable (st->vref);if (ret)drivers/iio/adc/ad7091r-base.h +1
-1@@ -20,6 +20,7 @@#define AD7091R_REG_CH_HYSTERESIS (ch) ((ch) * 3 + 6)/* AD7091R_REG_CONF */ +#define AD7091R_REG_CONF_INT_VREF BIT(0)#define AD7091R_REG_CONF_ALERT_EN BIT(4)#define AD7091R_REG_CONF_AUTO BIT(8)#define AD7091R_REG_CONF_CMD BIT(10) --2.42.0
Re: [PATCH v4 03/15] iio: adc: ad7091r: Set alert bit in config register
In reply to Jonathan CameronQuoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Sat, 16 Dec 2023 14:46:37 -0300 Marcelo Schmitt…
The ad7091r-base driver sets up an interrupt handler for firing events when inputs are either above or below a certain threshold. However, for the interrupt signal to come from the device it must be configured to enable the ALERT/BUSY/GPO pin to be used as ALERT, which was not being done until now.
Enable interrupt signals on the ALERT/BUSY/GPO pin by setting the proper bit in the configuration register.
Quoted 7 lines
Quoted 1 line
Signed-off-by Marcelo Schmitt <[email protected]>Hi Marcelo,
In V3 review I asked if this should have a fixes tag. I've assumed for now the answer is no and applied it without. If you let me know fast enough I can probably slip on in, but if not you may want to consider requesting a backport after this is upstream.
The events for these devices would not work both because of the broken dereference fixed in patch 2 and the alert signal was not being enabled. Patch 2 fixed a null pointer dereference that would lead to an error in the kernel. This patch (on top of the previous one) makes the event generation actually work although it's not fixing any errors.
I was hesitant in marking this one with a fixes tag too worrying I might be adding too many fixes tags for a feature that never worked.
Anyway, looks like I should have added that so at least now I have learned something. Will see how to ask backports for the fixing patches once they get in mainline.
Thanks
Quoted 37 lines
Applied to the togreg branch of iio.git and pushed out as testing for 0-day to take a look at it.
Thanks,
Jonathan
Quoted 28 lines
--- drivers/iio/adc/ad7091r-base.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/iio/adc/ad7091r-base.c +6
-0@@ -28,6 +28,7 @@#define AD7091R_REG_RESULT_CONV_RESULT (x) ((x) & 0xfff)/* AD7091R_REG_CONF */ +#define AD7091R_REG_CONF_ALERT_EN BIT(4)#define AD7091R_REG_CONF_AUTO BIT(8)#define AD7091R_REG_CONF_CMD BIT(10)@@ -232,6 +233,11 @@ int ad7091r_probe(struct device *dev, const char *name, iio_dev->channels = chip_info->channels;if (irq) { + ret =regmap_update_bits (st->map,AD7091R_REG_CONF , +AD7091R_REG_CONF_ALERT_EN ,BIT (4 )); +if (ret) +return ret; + ret =devm_request_threaded_irq (dev, irq,NULL , ad7091r_event_handler,IRQF_TRIGGER_FALLING |IRQF_ONESHOT , name, iio_dev);
Re: [PATCH v4 09/15] iio: adc: ad7091r: Enable internal vref if external vref is not supplied
In reply to Jonathan CameronQuoted 9 lines · Jonathan Cameron
Quoted 4 lines · On Sat, 16 Dec 2023 14:49:07 -0300 Marcelo Schmitt…
The ADC needs a voltage reference to work correctly.
Enable AD7091R internal voltage reference if no external vref is supplied.Signed-off-by Marcelo Schmitt <[email protected]>This one sounds to me like it should have a fixes tag and be much earlier in the set to perhaps simplify backports.
Could be. If we stick to the fact that the dt-binding does not require a voltage regulator then this can be seen as a fix.
Though, if users can provide an external reference this patch makes no difference them.
I am using the internal reference for testing so having this one makes a difference for me.
Quoted 41 lines
Jonathan
Quoted 36 lines
--- drivers/iio/adc/ad7091r-base.c | 7 +++++++ drivers/iio/adc/ad7091r-base.h | 1 + 2 files changed, 8 insertions(+)drivers/iio/adc/ad7091r-base.c +7
-0@@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info,if (IS_ERR (st->vref)) {if (PTR_ERR (st->vref) == -EPROBE_DEFER )return -EPROBE_DEFER ; + st->vref =NULL ; +/* Enable internal vref */ + ret =regmap_update_bits (st->map,AD7091R_REG_CONF , +AD7091R_REG_CONF_INT_VREF ,BIT (0 )); +if (ret) +return dev_err_probe (st->dev, ret, +"Error on enable internal reference\n" ); }else { ret =regulator_enable (st->vref);if (ret)drivers/iio/adc/ad7091r-base.h +1
-0@@ -20,6 +20,7 @@#define AD7091R_REG_CH_HYSTERESIS (ch) ((ch) * 3 + 6)/* AD7091R_REG_CONF */ +#define AD7091R_REG_CONF_INT_VREF BIT(0)#define AD7091R_REG_CONF_ALERT_EN BIT(4)#define AD7091R_REG_CONF_AUTO BIT(8)#define AD7091R_REG_CONF_CMD BIT(10)
Re: [PATCH v4 09/15] iio: adc: ad7091r: Enable internal vref if external vref is not supplied
In reply to David LechnerQuoted 28 lines · David Lechner
Quoted 15 lines · Marcelo Schmitt
The ADC needs a voltage reference to work correctly.
Enable AD7091R internal voltage reference if no external vref is supplied.Signed-off-by Marcelo Schmitt <[email protected]>--- drivers/iio/adc/ad7091r-base.c | 7 +++++++ drivers/iio/adc/ad7091r-base.h | 1 + 2 files changed, 8 insertions(+)drivers/iio/adc/ad7091r-base.c +0
-0drivers/iio/adc/ad7091r-base.c int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info, · line 224
Quoted 8 lines
drivers/iio/adc/ad7091r-base.c +4
-0@@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info,if (IS_ERR (st->vref)) {if (PTR_ERR (st->vref) == -EPROBE_DEFER )return -EPROBE_DEFER ; + st->vref =NULL ; +/* Enable internal vref */ + ret =regmap_update_bits (st->map,AD7091R_REG_CONF , +AD7091R_REG_CONF_INT_VREF ,BIT (0 ));Can we use
regmap_set_bits()here to avoid the BIT(0)?
Quoted 1 line
The same comment applies to other patches in this series.
Looks good, will do.
Thanks
Quoted 22 lines
Quoted 22 lines
drivers/iio/adc/ad7091r-base.c +3
-0+if (ret) +return dev_err_probe (st->dev, ret, +"Error on enable internal reference\n" ); }else { ret =regulator_enable (st->vref);if (ret)drivers/iio/adc/ad7091r-base.h +1
-1@@ -20,6 +20,7 @@#define AD7091R_REG_CH_HYSTERESIS (ch) ((ch) * 3 + 6)/* AD7091R_REG_CONF */ +#define AD7091R_REG_CONF_INT_VREF BIT(0)#define AD7091R_REG_CONF_ALERT_EN BIT(4)#define AD7091R_REG_CONF_AUTO BIT(8)#define AD7091R_REG_CONF_CMD BIT(10) --2.42.0
Jonathan Cameron
maintainerRe: [PATCH v4 09/15] iio: adc: ad7091r: Enable internal vref if external vref is not supplied
In reply to Marcelo SchmittQuoted 11 lines · On Mon, 18 Dec 2023 14:35:27 -0300 Marcelo Schmitt…
Quoted 9 lines · Jonathan Cameron
Quoted 4 lines · On Sat, 16 Dec 2023 14:49:07 -0300 Marcelo Schmitt…
The ADC needs a voltage reference to work correctly.
Enable AD7091R internal voltage reference if no external vref is supplied.Signed-off-by Marcelo Schmitt <[email protected]>This one sounds to me like it should have a fixes tag and be much earlier in the set to perhaps simplify backports.
Quoted 6 lines
Could be. If we stick to the fact that the dt-binding does not require a voltage regulator then this can be seen as a fix.
Though, if users can provide an external reference this patch makes no difference them.
I am using the internal reference for testing so having this one makes a difference for me.
The binding has it as optional, though usually when not having an external reference leads to use of an internal one, we call it out in the description.
Meh, can backport it as a fix if anyone asks for it.
Jonathan
Quoted 42 lines
Quoted 42 lines
Jonathan
Quoted 36 lines
--- drivers/iio/adc/ad7091r-base.c | 7 +++++++ drivers/iio/adc/ad7091r-base.h | 1 + 2 files changed, 8 insertions(+)drivers/iio/adc/ad7091r-base.c +7
-0@@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info,if (IS_ERR (st->vref)) {if (PTR_ERR (st->vref) == -EPROBE_DEFER )return -EPROBE_DEFER ; + st->vref =NULL ; +/* Enable internal vref */ + ret =regmap_update_bits (st->map,AD7091R_REG_CONF , +AD7091R_REG_CONF_INT_VREF ,BIT (0 )); +if (ret) +return dev_err_probe (st->dev, ret, +"Error on enable internal reference\n" ); }else { ret =regulator_enable (st->vref);if (ret)drivers/iio/adc/ad7091r-base.h +1
-0@@ -20,6 +20,7 @@#define AD7091R_REG_CH_HYSTERESIS (ch) ((ch) * 3 + 6)/* AD7091R_REG_CONF */ +#define AD7091R_REG_CONF_INT_VREF BIT(0)#define AD7091R_REG_CONF_ALERT_EN BIT(4)#define AD7091R_REG_CONF_AUTO BIT(8)#define AD7091R_REG_CONF_CMD BIT(10)