Date:   Thu, 11 May 2017 08:09:37 +0200
From:   Ludovic Desroches <ludovic.desroches@...rochip.com>
To:     Eugen Hristev <eugen.hristev@...rochip.com>
CC:     Jonathan Cameron <jic23@...nel.org>, <nicolas.ferre@...rochip.com>,
        <alexandre.belloni@...e-electrons.com>,
        <linux-iio@...r.kernel.org>, <lars@...afoo.de>,
        <linux-arm-kernel@...ts.infradead.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <ludovic.desroches@...rochip.com>
Subject: Re: [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and
 buffer support

On Wed, May 10, 2017 at 11:49:07AM +0300, Eugen Hristev wrote:
> Hello,
> 
> Thanks for the suggestions and reply.
> 
> Please see my answers inline
> 
> Eugen
> 
> On 07.05.2017 18:01, Jonathan Cameron wrote:
> > On 04/05/17 13:13, Eugen Hristev wrote:
> > > Added support for the external hardware trigger on pin ADTRG,
> > > integrated the three possible edge triggers into the subsystem
> > > and created buffer management for data retrieval
> > > 
> > > Signed-off-by: Eugen Hristev <eugen.hristev@...rochip.com>
> > > ---
> > >   Changes in v2:
> > >  - Moved buffer allocation and freeing into the preenable and postdisable
> > >    callbacks.
> > >    We have a total of scan bytes that can vary a lot depending on each channel
> > >    enabled at a certain point.
> > How much?  Having dynamic allocations are likely to prove just as costly as you'll
> > almost always end up allocating a lot more than you ask for.
> > 
> > I'm counting worst case of 18x 2byte channels + timestamp = 48 bytes.
> > A quick google suggests you'll always allocate 32 bytes whatever with slab
> > (lower for slob). So not a huge saving...
> > 
> > Worth the hassle?
> 
> 
> I will modify the allocation of scan_bytes to make it static.
> 
> > 
> > >  - made the at91 trigger list part of state structure
> > >  - made the iio trigger list preallocated in state structure
> > >  - moved irq enabling/disabling into the try_reenable callback
> > I'd have liked a little explanation on why we need to explicitly disable
> > the irq.  It will have little effect it if triggers too often as the
> > trigger consumers are all oneshot anyway.
> 
> Regarding the irq, the discussion is a bit longer.
> 
> As it is now, the interrupt is not a oneshot threaded one, because only
> the top half handler is installed, and the threaded one is NULL.
> Calling trigger_poll without disabling the interrupt will make the
> handler loop, so that is the reason for disabling and reenabling
> the interrupt.
> 
> One solution is to change it to oneshot threaded interrupt and call
> trigger_poll_chained to make it a nested handling. This will make a
> longer response time for conversions though.
> 
> One other option is to do irq_save and irq_restore before issuing the
> trigger poll, but that limits the usability of the trigger by any
> other device I guess.
> 
> I did the behavior with disabling and enabling the interrupt considering
> the old at91 driver and the stm32-adc driver which looks to be fairly
> similar with this implementation.
> 
> Can you please advise on which approach you think it's best for this
> driver ?
> So I can try that, and resend the patch.
> 
> > >  - on trigger disable must write disable registries as well
> > Basically looks good. A few questions inline.
> > 
> > Jonathan
> > > 
> > >  drivers/iio/adc/at91-sama5d2_adc.c | 231 ++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 228 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> > > index e10dca3..11f5570 100644
> > > --- a/drivers/iio/adc/at91-sama5d2_adc.c
> > > +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> > > @@ -23,8 +23,15 @@
> > >  #include <linux/platform_device.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/wait.h>
> > > +#include <linux/slab.h>
> > > +
> > >  #include <linux/iio/iio.h>
> > >  #include <linux/iio/sysfs.h>
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/trigger.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > > +
> > >  #include <linux/regulator/consumer.h>
> > > 
> > >  /* Control Register */
> > > @@ -132,6 +139,17 @@
> > >  #define AT91_SAMA5D2_PRESSR	0xbc
> > >  /* Trigger Register */
> > >  #define AT91_SAMA5D2_TRGR	0xc0
> > > +/* Mask for TRGMOD field of TRGR register */
> > > +#define AT91_SAMA5D2_TRGR_TRGMOD_MASK GENMASK(2, 0)
> > > +/* No trigger, only software trigger can start conversions */
> > > +#define AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER 0
> > > +/* Trigger Mode external trigger rising edge */
> > > +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE 1
> > > +/* Trigger Mode external trigger falling edge */
> > > +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
> > > +/* Trigger Mode external trigger any edge */
> > > +#define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
> > > +
> > >  /* Correction Select Register */
> > >  #define AT91_SAMA5D2_COSR	0xd0
> > >  /* Correction Value Register */
> > > @@ -145,14 +163,20 @@
> > >  /* Version Register */
> > >  #define AT91_SAMA5D2_VERSION	0xfc
> > > 
> > > +#define AT91_SAMA5D2_HW_TRIG_CNT 3
> > > +#define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
> > > +#define AT91_SAMA5D2_DIFF_CHAN_CNT 6
> > > +
> > >  #define AT91_SAMA5D2_CHAN_SINGLE(num, addr)				\
> > >  	{								\
> > >  		.type = IIO_VOLTAGE,					\
> > >  		.channel = num,						\
> > >  		.address = addr,					\
> > > +		.scan_index = num,					\
> > >  		.scan_type = {						\
> > >  			.sign = 'u',					\
> > >  			.realbits = 12,					\
> > > +			.storagebits = 16,				\
> > >  		},							\
> > >  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > >  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > > @@ -168,9 +192,11 @@
> > >  		.channel = num,						\
> > >  		.channel2 = num2,					\
> > >  		.address = addr,					\
> > > +		.scan_index = num + AT91_SAMA5D2_SINGLE_CHAN_CNT,	\
> > >  		.scan_type = {						\
> > >  			.sign = 's',					\
> > >  			.realbits = 12,					\
> > > +			.storagebits = 16,				\
> > >  		},							\
> > >  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > >  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > > @@ -188,18 +214,25 @@ struct at91_adc_soc_info {
> > >  	unsigned			max_sample_rate;
> > >  };
> > > 
> > > +struct at91_adc_trigger {
> > > +	char				*name;
> > > +	unsigned int			trgmod_value;
> > > +};
> > > +
> > >  struct at91_adc_state {
> > >  	void __iomem			*base;
> > >  	int				irq;
> > >  	struct clk			*per_clk;
> > >  	struct regulator		*reg;
> > >  	struct regulator		*vref;
> > > +	u16				*buffer;
> > >  	int				vref_uv;
> > >  	const struct iio_chan_spec	*chan;
> > >  	bool				conversion_done;
> > >  	u32				conversion_value;
> > >  	struct at91_adc_soc_info	soc_info;
> > >  	wait_queue_head_t		wq_data_available;
> > > +	struct iio_trigger		*trig[AT91_SAMA5D2_HW_TRIG_CNT];
> > >  	/*
> > >  	 * lock to prevent concurrent 'single conversion' requests through
> > >  	 * sysfs.
> > > @@ -207,6 +240,21 @@ struct at91_adc_state {
> > >  	struct mutex			lock;
> > >  };
> > > 
> > > +static const struct at91_adc_trigger at91_adc_trigger_list[] = {
> > > +	{
> > > +		.name = "external-rising",
> > > +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE,
> > > +	},
> > > +	{
> > > +		.name = "external-falling",
> > > +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL,
> > > +	},
> > > +	{
> > > +		.name = "external-any",
> > > +		.trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY,
> > > +	},
> > Hmm. Should this be a userspace configurable option?  Feels rather like
> > it is an element of the hardware - reflecting the characteristics of some
> > hardware device sat on the pin.
> The user can choose from sysfs which trigger
> is best suited for the use case, since all
> three triggers are provided and can be connected to the buffer.
> It reflects more the triggering capability of the ADC rather than
> any different hardware device sitting on the pin

I am also in favour of a userspace configurable option. For sure it's
hardware related but on our board we only provide a trigger pin, we
don't know which hardware the customer will put on this pin.

Regards

Ludovic

> 
> > > +};
> > > +
> > >  static const struct iio_chan_spec at91_adc_channels[] = {
> > >  	AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
> > >  	AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
> > > @@ -226,8 +274,168 @@ static const struct iio_chan_spec at91_adc_channels[] = {
> > >  	AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
> > >  	AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
> > >  	AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
> > > +	IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_SINGLE_CHAN_CNT
> > > +				+ AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
> > >  };
> > > 
> > > +static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> > > +{
> > > +	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> > > +	struct at91_adc_state *st = iio_priv(indio);
> > > +	u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
> > > +	u8 bit;
> > > +	int i;
> > > +
> > > +	/* clear TRGMOD */
> > > +	status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
> > > +
> > > +	if (state)
> > > +		for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
> > > +			if (!strstr(trig->name,
> > > +				    at91_adc_trigger_list[i].name)) {
> > > +				status |= at91_adc_trigger_list[i].trgmod_value;
> > > +				break;
> > > +			}
> > > +		}
> > > +
> > > +	/* set/unset hw trigger */
> > > +	at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
> > > +
> > > +	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> > > +		struct iio_chan_spec const *chan = indio->channels + bit;
> > > +
> > > +		if (state) {
> > > +			at91_adc_writel(st, AT91_SAMA5D2_CHER,
> > > +					BIT(chan->channel));
> > > +			at91_adc_writel(st, AT91_SAMA5D2_IER,
> > > +					BIT(chan->channel));
> > > +		} else {
> > > +			at91_adc_writel(st, AT91_SAMA5D2_IDR,
> > > +					BIT(chan->channel));
> > > +			at91_adc_writel(st, AT91_SAMA5D2_CHDR,
> > > +					BIT(chan->channel));
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int at91_adc_reenable_trigger(struct iio_trigger *trig)
> > > +{
> > > +	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> > > +	struct at91_adc_state *st = iio_priv(indio);
> > > +
> > > +	enable_irq(st->irq);
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct iio_trigger_ops at91_adc_trigger_ops = {
> > > +	.owner = THIS_MODULE,
> > > +	.set_trigger_state = &at91_adc_configure_trigger,
> > > +	.try_reenable = &at91_adc_reenable_trigger,
> > > +};
> > > +
> > > +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev)
> > > +{
> > > +	struct at91_adc_state *st = iio_priv(indio_dev);
> > > +
> > > +	st->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);Why allocate it here?  Presumably the biggest it can get is fairly
> > small?  Unless very large, just make sure you allocate enough
> > directly in st in the first place.
> 
> Yes , as discussed above will change to static allocation.
> 
> > 
> > > +
> > > +	if (!st->buffer)
> > > +		return -ENOMEM;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
> > > +{
> > > +	struct at91_adc_state *st = iio_priv(indio_dev);
> > > +
> > > +	kfree(st->buffer);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
> > > +	.preenable = &at91_adc_buffer_preenable,
> > > +	.postenable = &iio_triggered_buffer_postenable,
> > > +	.predisable = &iio_triggered_buffer_predisable,
> > > +	.postdisable = &at91_adc_buffer_postdisable,
> > > +};
> > > +
> > > +static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
> > > +						     char *trigger_name)
> > > +{
> > > +	struct iio_trigger *trig;
> > > +	int ret;
> > > +
> > > +	trig = devm_iio_trigger_alloc(&indio->dev, "%s-dev%d-%s", indio->name,
> > > +				      indio->id, trigger_name);
> > > +	if (!trig)
> > > +		return NULL;
> > > +
> > > +	trig->dev.parent = indio->dev.parent;
> > > +	iio_trigger_set_drvdata(trig, indio);
> > > +	trig->ops = &at91_adc_trigger_ops;
> > > +
> > > +	ret = devm_iio_trigger_register(&indio->dev, trig);
> > > +
> > > +	if (ret)
> > > +		return NULL;
> > > +
> > > +	return trig;
> > > +}
> > > +
> > > +static int at91_adc_trigger_init(struct iio_dev *indio)
> > > +{
> > > +	struct at91_adc_state *st = iio_priv(indio);
> > > +	int i;
> > > +
> > > +	for (i = 0; i < AT91_SAMA5D2_HW_TRIG_CNT; i++) {
> > > +		st->trig[i] = at91_adc_allocate_trigger(indio,
> > > +						at91_adc_trigger_list[i].name);
> > > +		if (!st->trig[i]) {
> > > +			dev_err(&indio->dev,
> > > +				"could not allocate trigger %d\n", i);
> > > +			return -ENOMEM;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> > > +{
> > > +	struct iio_poll_func *pf = p;
> > > +	struct iio_dev *indio = pf->indio_dev;
> > > +	struct at91_adc_state *st = iio_priv(indio);
> > > +	int i = 0;
> > > +	u8 bit;
> > > +
> > > +	for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> > > +		struct iio_chan_spec const *chan = indio->channels + bit;
> > > +
> > > +		st->buffer[i] = at91_adc_readl(st, chan->address);
> > > +		i++;
> > > +	}
> > > +
> > > +	iio_push_to_buffers_with_timestamp(indio, st->buffer, pf->timestamp);
> > > +
> > > +	iio_trigger_notify_done(indio->trig);
> > > +
> > > +	/* Needed to ACK the DRDY interruption */
> > > +	at91_adc_readl(st, AT91_SAMA5D2_LCDR);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int at91_adc_buffer_init(struct iio_dev *indio)
> > > +{
> > > +	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
> > > +			&iio_pollfunc_store_time,
> > > +			&at91_adc_trigger_handler, &at91_buffer_setup_ops);
> > > +}
> > > +
> > >  static unsigned at91_adc_startup_time(unsigned startup_time_min,
> > >  				      unsigned adc_clk_khz)
> > >  {
> > > @@ -293,14 +501,19 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
> > >  	u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
> > >  	u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
> > > 
> > > -	if (status & imr) {
> > > +	if (!(status & imr))
> > > +		return IRQ_NONE;
> > > +
> > > +	if (iio_buffer_enabled(indio)) {
> > > +		disable_irq_nosync(irq);
> > > +		iio_trigger_poll(indio->trig);
> > > +	} else {
> > >  		st->conversion_value = at91_adc_readl(st, st->chan->address);
> > >  		st->conversion_done = true;
> > >  		wake_up_interruptible(&st->wq_data_available);
> > > -		return IRQ_HANDLED;
> > >  	}
> > > 
> > > -	return IRQ_NONE;
> > > +	return IRQ_HANDLED;
> > >  }
> > > 
> > >  static int at91_adc_read_raw(struct iio_dev *indio_dev,
> > > @@ -499,6 +712,18 @@ static int at91_adc_probe(struct platform_device *pdev)
> > > 
> > >  	platform_set_drvdata(pdev, indio_dev);
> > > 
> > > +	ret = at91_adc_buffer_init(indio_dev);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
> > > +		goto per_clk_disable_unprepare;
> > > +	}
> > > +
> > > +	ret = at91_adc_trigger_init(indio_dev);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "couldn't setup the triggers.\n");
> > > +		goto per_clk_disable_unprepare;
> > > +	}
> > > +
> > >  	ret = iio_device_register(indio_dev);
> > >  	if (ret < 0)
> > >  		goto per_clk_disable_unprepare;
> > > 
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists