[PATCH v2 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and buffer support
Jonathan Cameron
jic23 at kernel.org
Sun May 14 08:12:49 PDT 2017
On 11/05/17 07:09, Ludovic Desroches wrote:
> 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 at microchip.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.
Leave it as is. Thanks for the explanation.
>>
>>>> - 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.
hmm. OK I'm persuaded I think.
Could do this with devicetree overlays or similar.
So follow up question is whether this is the right interface.
Can all 3 run at once sensibly? If not are we not looking at a control
parameter for a single trigger?
>
> 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 at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the linux-arm-kernel
mailing list