Message-ID: <gvigk6helnl3yeouy636dgvay7tqux7lnxns3256fivzz4l3er@7ts7fz7vitff>
Date: Fri, 13 Jun 2025 12:02:44 +0200
From: Jorge Marques <gastmaier@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: Jorge Marques <jorge.marques@...log.com>, 
	Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, 
	Michael Hennerich <Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jonathan Corbet <corbet@....net>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, Uwe Kleine-König <ukleinek@...nel.org>, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v3 8/8] iio: adc: Add events support to ad4052

Hi David,
On Thu, Jun 12, 2025 at 02:38:45PM -0500, David Lechner wrote:
> On 6/10/25 2:34 AM, Jorge Marques wrote:
> > The AD4052 family supports autonomous monitoring readings for threshold
> > crossings. Add support for catching the GPIO interrupt and expose as an IIO
> > event. The device allows to set either, rising and falling directions. Only
> > either threshold crossing is implemented.
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@...log.com>
> > ---
> 
> ...
> 
> > +
> > +static ssize_t ad4052_events_frequency_store(struct device *dev,
> > +					     struct device_attribute *attr,
> > +					     const char *buf,
> > +					     size_t len)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +	if (st->wait_event) {
> > +		ret = -EBUSY;
> > +		goto out_release;
> > +	}
> 
> I'm wondering if we should instead have some kind of iio_device_claim_monitor_mode()
> so that we don't have to implement this manually everywhere. If monitor mode was
> claimed, then iio_device_claim_direct() and iio_device_claim_buffer_mode() would
> both return -EBUSY. If buffer mode was claimed, iio_device_claim_monitor_mode()
> would fail. If direct mode was claimed, iio_device_claim_monitor_mode() would wait.
> 
I don't think this would scale with other vendors and devices, it is a
limitation of ADI:ADC:SPI requiring to enter configuration mode to read
registers. A deep dive into the other drivers that use IIO Events is
needed.
> > +
> > +	ret = __sysfs_match_string(AD4052_FS(st->chip->grade),
> > +				   AD4052_FS_LEN(st->chip->grade), buf);
> > +	if (ret < 0)
> > +		goto out_release;
> > +
> > +	st->events_frequency = ret;
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(sampling_frequency, 0644,
> > +		       ad4052_events_frequency_show,
> > +		       ad4052_events_frequency_store, 0);
> > +
> > +static ssize_t sampling_frequency_available_show(struct device *dev,
> > +						 struct device_attribute *attr,
> > +						 char *buf)
> > +{
> > +	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
> > +	int ret = 0;
> > +
> > +	for (u8 i = AD4052_FS_OFFSET(st->chip->grade);
> > +	     i < AD4052_FS_LEN(st->chip->grade); i++)
> > +		ret += sysfs_emit_at(buf, ret, "%s ", ad4052_conversion_freqs[i]);
> > +
> > +	ret += sysfs_emit_at(buf, ret, "\n");
> > +	return ret;
> > +}
> > +
> > +static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> > +
> > +static struct attribute *ad4052_event_attributes[] = {
> > +	&iio_dev_attr_sampling_frequency.dev_attr.attr,
> > +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group ad4052_event_attribute_group = {
> > +	.attrs = ad4052_event_attributes,
> > +};
> > +
> >  static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
> >  				   struct iio_chan_spec const *chan)
> >  {
> > @@ -602,6 +699,19 @@ static int ad4052_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *c
> >  				  val);
> >  }
> >  
> > +static irqreturn_t ad4052_irq_handler_thresh(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +
> 
> Can we not read the status register here to find out what the exact
> event was? I guess that would require taking it out of monitor mode.
> 
It requires entering configuration mode and results in a monitoring
downtime. Earlier versions of this driver would do that, but the
conclusion was that it was better to have the user disabling events and
reading registers, so he is explicitly aware of the monitoring downtime.
> > +	iio_push_event(indio_dev,
> > +		       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
> > +					    IIO_EV_TYPE_THRESH,
> > +					    IIO_EV_DIR_EITHER),
> > +		       iio_get_time_ns(indio_dev));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static irqreturn_t ad4052_irq_handler_drdy(int irq, void *private)
> >  {
> >  	struct ad4052_state *st = private;
> > @@ -616,6 +726,18 @@ static int ad4052_request_irq(struct iio_dev *indio_dev)
> >  	struct device *dev = &st->spi->dev;
> >  	int ret;
> >  
> > +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
> > +	if (ret > 0) {
> > +		ret = devm_request_threaded_irq(dev, ret, NULL,
> > +						ad4052_irq_handler_thresh,
> > +						IRQF_ONESHOT, indio_dev->name,
> > +						indio_dev);
> > +		if (ret)
> > +			return ret;
> > +	} else if (ret == -EPROBE_DEFER) {
> > +		return ret;
> > +	}
> 
> By swapping the order, we can avoid the else. Also, do we really want to
> ignore all other errors? It seems like there would just be ENODEV or ENOENT
> that means the interrupt is not there and we would want to pass on other
> errors.
> 
Ack on the swap order.

If not set on the devicetree, including improper devicetree cases, it
should continue without. If the driver that manages the irq is not
probed, defer probe.

I tested different devicetrees and got:

* any property is missing: -EINVAL
* wrong interrupt-names: -ENODATA
* inconsistent array length between properties: -EOVERFLOW

EPROTO and ENXIO errors are also expected according the method comment,
the latter seems to be when the system doesn't support dts at all? And
EPROTO just another user-set dts issue.
I'm okay with ignoring them silently, or logging if gp0/1 found or not,
but not micromanage every error.

> > +
> >  	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
> >  	if (ret > 0) {
> >  		ret = devm_request_threaded_irq(dev, ret, NULL,
> 
> 
> ...
> 
> > +
> > +static int ad4052_monitor_mode_enable(struct ad4052_state *st)
> > +{
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(&st->spi->dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4052_conversion_frequency_set(st, st->events_frequency);
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	ret = ad4052_set_operation_mode(st, AD4052_MONITOR_MODE);
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	return ret;
> > +out_error:
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->dev);
> > +	return ret;
> > +}
> > +
> > +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
> > +{
> > +	int ret;
> > +
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->dev);
> > +
> > +	ret = ad4052_exit_command(st);
> > +	if (ret)
> > +		return ret;
> > +	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> > +			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
> > +			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
> > +}
> > +
> 
> It seems like we need to make sure monitor mode is disabled when the
> driver is removed. Otherwise we could end up with unbalanced calls to
> the pm_runtime stuff and leave the chip running.
> 
> 
When monitor mode is enabled, pm is already disabled (won't enter low
power). I expect the pm to handle the clean-up properly since devm is
used.
The .remove() I suggest is reg access to:

* Put in configuration mode, if not.
* Put on low power mode, if not.

> > +static int ad4052_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 ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		ret = -EBUSY;
> > +		goto out_release;
> > +	}
> > +
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		ret = __ad4052_read_event_info_value(st, dir, val);
> > +		break;
> > +	case IIO_EV_INFO_HYSTERESIS:
> > +		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
> > +		break;
> 
> These functions don't need __ prefix. There is no name clash.
> 
Ack.

Best regards,
Jorge
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : IIO_VAL_INT;
> > +}
> > +

Powered by blists - more mailing lists