mbox series

[v4,00/15] Add support for AD7091R-2/-4/-8

Message ID cover.1702746240.git.marcelo.schmitt1@gmail.com
Headers show
Series Add support for AD7091R-2/-4/-8 | expand

Message

Marcelo Schmitt Dec. 16, 2023, 5:44 p.m. UTC 
From: Marcelo Schmitt <marcelo.schmitt1@gmail.com>

----------------- 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

[1]: https://lore.kernel.org/linux-iio/CAMknhBHCYicEL_xhumBQMUm=HBVb=7dLrYsK8Zj2o7RodvMarw@mail.gmail.com/

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.
Link: https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad7091r8

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

Comments

Jonathan Cameron Dec. 17, 2023, 2:51 p.m. UTC | #1 
On Sat, 16 Dec 2023 10:07:24 -0800
Joe Perches <joe@perches.com> wrote:

> On Sat, 2023-12-16 at 14:45 -0300, Marcelo Schmitt wrote:
> > 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.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > ---
> > 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).  
> 
> 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.
> 
> > 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));   
> 
> 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:
> 
> Acked-by: Joe Perches <joe@perches.com>
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

> 
> > 
> > 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(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 25fdb7fda112..d56c98146da3 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -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 Dec. 17, 2023, 2:55 p.m. UTC | #2 
On Sat, 16 Dec 2023 14:46:37 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> 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 <marcelo.schmitt@analog.com>
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

> ---
>  drivers/iio/adc/ad7091r-base.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
> index 0e5d3d2e9c98..8aaa854f816f 100644
> --- a/drivers/iio/adc/ad7091r-base.c
> +++ b/drivers/iio/adc/ad7091r-base.c
> @@ -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);
Jonathan Cameron Dec. 17, 2023, 2:59 p.m. UTC | #3 
On Sat, 16 Dec 2023 14:47:25 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> 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 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.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
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

> ---
>  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(-)
> 
> diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
> index d3d287d3b953..0d1f544de07a 100644
> --- a/drivers/iio/adc/ad7091r-base.c
> +++ b/drivers/iio/adc/ad7091r-base.c
> @@ -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,
> diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h
> index 509748aef9b1..1d30eeb46bcc 100644
> --- a/drivers/iio/adc/ad7091r-base.h
> +++ b/drivers/iio/adc/ad7091r-base.h
> @@ -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__ */
> diff --git a/drivers/iio/adc/ad7091r5.c b/drivers/iio/adc/ad7091r5.c
> index 2f048527b7b7..9d3ccfca94ec 100644
> --- a/drivers/iio/adc/ad7091r5.c
> +++ b/drivers/iio/adc/ad7091r5.c
> @@ -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 Dec. 17, 2023, 3:41 p.m. UTC | #4 
On Sat, 16 Dec 2023 14:49:07 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> 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 <marcelo.schmitt@analog.com>
This one sounds to me like it should have a fixes tag and be
much earlier in the set to perhaps simplify backports.

Jonathan

> ---
>  drivers/iio/adc/ad7091r-base.c | 7 +++++++
>  drivers/iio/adc/ad7091r-base.h | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
> index aead72ef55b6..9d0b489966f5 100644
> --- a/drivers/iio/adc/ad7091r-base.c
> +++ b/drivers/iio/adc/ad7091r-base.c
> @@ -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)
> diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h
> index 81b8a4bbb929..9cfb362a00a4 100644
> --- a/drivers/iio/adc/ad7091r-base.h
> +++ b/drivers/iio/adc/ad7091r-base.h
> @@ -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)
David Lechner Dec. 17, 2023, 11:58 p.m. UTC | #5 
On Sat, Dec 16, 2023 at 11:52 AM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> Implement event configuration callbacks allowing users to read/write
> event thresholds and enable/disable event generation.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
> This is from a review suggestion David made on v3 [1].
>
> Is this the case for a Suggested-by tag?
>
> [1]: https://lore.kernel.org/linux-iio/CAMknhBFPbAqp4-AQdmbp+VRW-Ksk1PxaLCG+3n=Zk4gyStqhgw@mail.gmail.com/#t
>

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 Dec. 18, 2023, 12:10 a.m. UTC | #6 
On Sun, Dec 17, 2023 at 5:58 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On Sat, Dec 16, 2023 at 11:52 AM Marcelo Schmitt
> <marcelo.schmitt@analog.com> wrote:
> >
> > Implement event configuration callbacks allowing users to read/write
> > event thresholds and enable/disable event generation.
> >
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > ---
> > This is from a review suggestion David made on v3 [1].
> >
> > Is this the case for a Suggested-by tag?
> >
> > [1]: https://lore.kernel.org/linux-iio/CAMknhBFPbAqp4-AQdmbp+VRW-Ksk1PxaLCG+3n=Zk4gyStqhgw@mail.gmail.com/#t
> >
>
> 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".

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.
Marcelo Schmitt Dec. 18, 2023, 5:22 p.m. UTC | #7 
On 12/17, Jonathan Cameron wrote:
> On Sat, 16 Dec 2023 14:46:37 -0300
> Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:
> 
> > 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 <marcelo.schmitt@analog.com>
> 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

> 
> Applied to the togreg branch of iio.git and pushed out as testing for 0-day
> to take a look at it.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/ad7091r-base.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
> > index 0e5d3d2e9c98..8aaa854f816f 100644
> > --- a/drivers/iio/adc/ad7091r-base.c
> > +++ b/drivers/iio/adc/ad7091r-base.c
> > @@ -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);
>
Marcelo Schmitt Dec. 18, 2023, 5:35 p.m. UTC | #8 
On 12/17, Jonathan Cameron wrote:
> On Sat, 16 Dec 2023 14:49:07 -0300
> Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:
> 
> > 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 <marcelo.schmitt@analog.com>
> 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.

> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/ad7091r-base.c | 7 +++++++
> >  drivers/iio/adc/ad7091r-base.h | 1 +
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
> > index aead72ef55b6..9d0b489966f5 100644
> > --- a/drivers/iio/adc/ad7091r-base.c
> > +++ b/drivers/iio/adc/ad7091r-base.c
> > @@ -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)
> > diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h
> > index 81b8a4bbb929..9cfb362a00a4 100644
> > --- a/drivers/iio/adc/ad7091r-base.h
> > +++ b/drivers/iio/adc/ad7091r-base.h
> > @@ -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)
> 
>