Re: [PATCH v2] iio: adc: Add support for DLN2 ADC

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 25 Jun 2017 07:06:17 -1000
Jack Andersen <jackoalan@gmail.com> wrote:

> Ok, the temporary enables for raw reads are fine with me; my
> applications all use buffered access anyway.
> 
> It would be better to allow the dynamic release of unused ADC
> channels for GPIO pins, which lazy enabling prevents.
Why does it prevent it?  It moves the failure point later, but that's
all.  So if you end up with a pin being assigned to be ADC after
it has been grabbed as a gpio, fail at the update_scan_mask call
-EBUSY would make sense I think.

I guess it might take some effort to make that info available though...

J
> 
> On 25 June 2017 at 05:58, Jonathan Cameron <jic23@kernel.org> wrote:
> > On Sat, 24 Jun 2017 12:18:58 -1000
> > Jack Andersen <jackoalan@gmail.com> wrote:
> >  
> >> You're right about the fiddly device part. The important thing
> >> to note about the DLN2 is it's a multifunction device with a
> >> unified command message interface. All four functions (GPIO, ADC,
> >> I2C, SPI) use this interface for every possible action. Commands
> >> are performed synchronously and return at least an error code.
> >>
> >> The dln2.c module in MFD implements this command interface and
> >> manages 'handles' for all four functions as well as a single
> >> event handle for handling USB transfers of events like GPIO
> >> low/high and ADC thresholds / periodic timers. Existing modules
> >> like dln2_gpio and dln2_i2c all share the symbols exported by
> >> dln2.
> >>
> >> I'll revaluate the code when I'm back in the office on Monday.
> >>
> >> More explanations inline.
> >>
> >> On 24 June 2017 at 09:38, Jonathan Cameron <jic23@kernel.org> wrote:  
> >> > On Fri, 23 Jun 2017 09:25:58 +0200 (CEST)
> >> > Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:
> >> >  
> >> >> > This patch adds support for Diolan DLN2 ADC via IIO's ADC interface.
> >> >> > ADC is the fourth and final component of the DLN2 for the kernel.  
> >> >>
> >> >> I am missing a changelog or information if and how review comments have
> >> >> been addressed
> >> >>
> >> >> thanks for the super-fast turnaround, anyway
> >> >>
> >> >> p.  
> >> > A few bits and pieces inline.  Fiddly device.  Out of curiosity
> >> > are then any relevant docs or is this a reverse engineering job?
> >> >
> >> > There were a few unusual elements so would be nice to verify they
> >> > line up with docs if available!
> >> >
> >> > Jonathan  
> >> Reverse engineering wasn't really necessary; Diolan has macros
> >> of all the command enums and structures in their userspace
> >> headers (for some reason). The calls implemented in their binary
> >> blob are simple libusb transfer wrappers; I essentially stuck to
> >> the transfer style used by dln2_gpio instead.
> >>
> >> For reference:
> >> http://dlnware.com/downloads/linux-setup
> >> Unzip and refer to examples/common/dln_adc.h  
> >> >>  
> >> >> > Signed-off-by: Jack Andersen <jackoalan@gmail.com>
> >> >> > ---
> >> >> >  drivers/iio/adc/Kconfig    |   9 +
> >> >> >  drivers/iio/adc/Makefile   |   1 +
> >> >> >  drivers/iio/adc/dln2-adc.c | 644 +++++++++++++++++++++++++++++++++++++++++++++
> >> >> >  drivers/mfd/dln2.c         |  12 +  
> >> > touching an mfd file so needs an ack from Lee. Cc'd.  
> >> >> >  4 files changed, 666 insertions(+)
> >> >> >  create mode 100644 drivers/iio/adc/dln2-adc.c
> >> >> >
> >> >> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> >> >> > index 401f47b..78d7455 100644
> >> >> > --- a/drivers/iio/adc/Kconfig
> >> >> > +++ b/drivers/iio/adc/Kconfig
> >> >> > @@ -239,6 +239,15 @@ config DA9150_GPADC
> >> >> >       To compile this driver as a module, choose M here: the module will be
> >> >> >       called berlin2-adc.
> >> >> >
> >> >> > +config DLN2_ADC
> >> >> > +   tristate "Diolan DLN-2 ADC driver support"
> >> >> > +   depends on MFD_DLN2
> >> >> > +   help
> >> >> > +     Say yes here to build support for Diolan DLN-2 ADC.
> >> >> > +
> >> >> > +     This driver can also be built as a module. If so, the module will be
> >> >> > +     called adc_dln2.
> >> >> > +
> >> >> >  config ENVELOPE_DETECTOR
> >> >> >     tristate "Envelope detector using a DAC and a comparator"
> >> >> >     depends on OF
> >> >> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> >> >> > index 9339bec..378bc65 100644
> >> >> > --- a/drivers/iio/adc/Makefile
> >> >> > +++ b/drivers/iio/adc/Makefile
> >> >> > @@ -24,6 +24,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
> >> >> >  obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
> >> >> >  obj-$(CONFIG_CPCAP_ADC) += cpcap-adc.o
> >> >> >  obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
> >> >> > +obj-$(CONFIG_DLN2_ADC) += dln2-adc.o
> >> >> >  obj-$(CONFIG_ENVELOPE_DETECTOR) += envelope-detector.o
> >> >> >  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> >> >> >  obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
> >> >> > diff --git a/drivers/iio/adc/dln2-adc.c b/drivers/iio/adc/dln2-adc.c
> >> >> > new file mode 100644
> >> >> > index 0000000..6c58b76
> >> >> > --- /dev/null
> >> >> > +++ b/drivers/iio/adc/dln2-adc.c
> >> >> > @@ -0,0 +1,644 @@
> >> >> > +/*
> >> >> > + * Driver for the Diolan DLN-2 USB-ADC adapter
> >> >> > + *
> >> >> > + * Copyright (c) 2017 Jack Andersen
> >> >> > + *
> >> >> > + * This program is free software; you can redistribute it and/or
> >> >> > + * modify it under the terms of the GNU General Public License as
> >> >> > + * published by the Free Software Foundation, version 2.
> >> >> > + */
> >> >> > +
> >> >> > +#include <linux/kernel.h>
> >> >> > +#include <linux/module.h>
> >> >> > +#include <linux/types.h>
> >> >> > +#include <linux/platform_device.h>
> >> >> > +#include <linux/mfd/dln2.h>
> >> >> > +
> >> >> > +#include <linux/iio/iio.h>
> >> >> > +#include <linux/iio/sysfs.h>
> >> >> > +#include <linux/iio/trigger.h>
> >> >> > +#include <linux/iio/trigger_consumer.h>
> >> >> > +#include <linux/iio/buffer.h>
> >> >> > +#include <linux/iio/kfifo_buf.h>
> >> >> > +
> >> >> > +#define DLN2_ADC_MOD_NAME "dln2-adc"
> >> >> > +
> >> >> > +#define DLN2_ADC_ID             0x06
> >> >> > +
> >> >> > +#define DLN2_ADC_GET_CHANNEL_COUNT DLN2_CMD(0x01, DLN2_ADC_ID)
> >> >> > +#define DLN2_ADC_ENABLE                    DLN2_CMD(0x02, DLN2_ADC_ID)
> >> >> > +#define DLN2_ADC_DISABLE           DLN2_CMD(0x03, DLN2_ADC_ID)
> >> >> > +#define DLN2_ADC_CHANNEL_ENABLE            DLN2_CMD(0x05, DLN2_ADC_ID)
> >> >> > +#define DLN2_ADC_CHANNEL_DISABLE   DLN2_CMD(0x06, DLN2_ADC_ID)
> >> >> > +#define DLN2_ADC_SET_RESOLUTION            DLN2_CMD(0x08, DLN2_ADC_ID)
> >> >> > +#define DLN2_ADC_CHANNEL_GET_VAL   DLN2_CMD(0x0A, DLN2_ADC_ID)
> >> >> > +#define DLN2_ADC_CHANNEL_GET_ALL_VAL       DLN2_CMD(0x0B, DLN2_ADC_ID)
> >> >> > +#define DLN2_ADC_CHANNEL_SET_CFG   DLN2_CMD(0x0C, DLN2_ADC_ID)
> >> >> > +#define DLN2_ADC_CHANNEL_GET_CFG   DLN2_CMD(0x0D, DLN2_ADC_ID)
> >> >> > +#define DLN2_ADC_CONDITION_MET_EV  DLN2_CMD(0x10, DLN2_ADC_ID)
> >> >> > +
> >> >> > +#define DLN2_ADC_EVENT_NONE                0
> >> >> > +#define DLN2_ADC_EVENT_BELOW               1
> >> >> > +#define DLN2_ADC_EVENT_LEVEL_ABOVE 2
> >> >> > +#define DLN2_ADC_EVENT_OUTSIDE             3
> >> >> > +#define DLN2_ADC_EVENT_INSIDE              4
> >> >> > +#define DLN2_ADC_EVENT_ALWAYS              5
> >> >> > +
> >> >> > +#define DLN2_ADC_MAX_CHANNELS 8
> >> >> > +#define DLN2_ADC_DATA_BITS 10
> >> >> > +
> >> >> > +struct dln2_adc {
> >> >> > +   struct platform_device *pdev;
> >> >> > +   int port;
> >> >> > +   struct iio_trigger *trig;
> >> >> > +   struct mutex mutex;
> >> >> > +   /* Set once initialized */
> >> >> > +   bool port_enabled;
> >> >> > +   /* Set once resolution request made to HW */
> >> >> > +   bool resolution_set;
> >> >> > +   /* Bitmask requesting enabled channels */
> >> >> > +   unsigned long chans_requested;
> >> >> > +   /* Bitmask indicating enabled channels on HW */
> >> >> > +   unsigned long chans_enabled;
> >> >> > +   /* Channel that is arbitrated for event trigger */
> >> >> > +   int trigger_chan;
> >> >> > +};
> >> >> > +
> >> >> > +struct dln2_adc_port_chan {
> >> >> > +   u8 port;
> >> >> > +   u8 chan;
> >> >> > +};
> >> >> > +
> >> >> > +struct dln2_adc_get_all_vals {
> >> >> > +   __le16 channel_mask;
> >> >> > +   __le16 values[DLN2_ADC_MAX_CHANNELS];
> >> >> > +};
> >> >> > +
> >> >> > +static int dln2_adc_get_chan_count(struct dln2_adc *dln2)
> >> >> > +{
> >> >> > +   int ret;
> >> >> > +   u8 port = dln2->port;
> >> >> > +   u8 count;
> >> >> > +   int olen = sizeof(count);
> >> >> > +
> >> >> > +   ret = dln2_transfer(dln2->pdev, DLN2_ADC_GET_CHANNEL_COUNT,
> >> >> > +                       &port, sizeof(port), &count, &olen);
> >> >> > +   if (ret < 0) {
> >> >> > +           dev_dbg(&dln2->pdev->dev, "Problem in %s\n", __func__);
> >> >> > +           return ret;
> >> >> > +   }
> >> >> > +   if (olen < sizeof(count))
> >> >> > +           return -EPROTO;
> >> >> > +
> >> >> > +   return count;
> >> >> > +}
> >> >> > +
> >> >> > +static int dln2_adc_set_port_resolution(struct dln2_adc *dln2)
> >> >> > +{
> >> >> > +   int ret;
> >> >> > +   struct dln2_adc_port_chan port_chan = {
> >> >> > +           .port = dln2->port,
> >> >> > +           .chan = DLN2_ADC_DATA_BITS,
> >> >> > +   };
> >> >> > +
> >> >> > +   ret = dln2_transfer_tx(dln2->pdev, DLN2_ADC_SET_RESOLUTION,
> >> >> > +                          &port_chan, sizeof(port_chan));
> >> >> > +   if (ret < 0) {
> >> >> > +           dev_dbg(&dln2->pdev->dev, "Problem in %s\n", __func__);
> >> >> > +           return ret;
> >> >> > +   }
> >> >> > +
> >> >> > +   return 0;
> >> >> > +}
> >> >> > +
> >> >> > +static int dln2_adc_set_chan_enabled(struct dln2_adc *dln2,
> >> >> > +                                int channel, bool enable)
> >> >> > +{
> >> >> > +   int ret;
> >> >> > +   struct dln2_adc_port_chan port_chan = {
> >> >> > +           .port = dln2->port,
> >> >> > +           .chan = channel,
> >> >> > +   };
> >> >> > +
> >> >> > +   u16 cmd = enable ? DLN2_ADC_CHANNEL_ENABLE : DLN2_ADC_CHANNEL_DISABLE;
> >> >> > +
> >> >> > +   ret = dln2_transfer_tx(dln2->pdev, cmd, &port_chan, sizeof(port_chan));
> >> >> > +   if (ret < 0) {
> >> >> > +           dev_dbg(&dln2->pdev->dev, "Problem in %s\n", __func__);
> >> >> > +           return ret;
> >> >> > +   }
> >> >> > +
> >> >> > +   return 0;
> >> >> > +}
> >> >> > +
> >> >> > +static int dln2_adc_set_port_enabled(struct dln2_adc *dln2, bool enable)
> >> >> > +{
> >> >> > +   int ret;
> >> >> > +   u8 port = dln2->port;
> >> >> > +   __le16 conflict;
> >> >> > +   int olen = sizeof(conflict);
> >> >> > +
> >> >> > +   u16 cmd = enable ? DLN2_ADC_ENABLE : DLN2_ADC_DISABLE;
> >> >> > +
> >> >> > +   ret = dln2_transfer(dln2->pdev, cmd, &port, sizeof(port),
> >> >> > +                       &conflict, &olen);
> >> >> > +   if (ret < 0) {
> >> >> > +           dev_dbg(&dln2->pdev->dev, "Problem in %s(%d)\n",
> >> >> > +                   __func__, (int)enable);
> >> >> > +           return ret;
> >> >> > +   }
> >> >> > +   if (enable && olen < sizeof(conflict))
> >> >> > +           return -EPROTO;
> >> >> > +
> >> >> > +   return 0;
> >> >> > +}
> >> >> > +
> >> >> > +/*
> >> >> > + * ADC channels are lazily enabled due to the pins being shared with GPIO
> >> >> > + * channels. Enabling channels requires taking the ADC port offline, specifying
> >> >> > + * the resolution, individually enabling channels, then putting the port back
> >> >> > + * online. If GPIO pins have already been exported by gpio_dln2, EINVAL is
> >> >> > + * reported.
> >> >> > + */  
> >> > Ah, now it's all starting to make sense.  Let me check I have this right.
> >> >
> >> > 1) You have an 8 channel adc.
> >> > 2) You can only (or it's more efficient) to read all 8 channels at once.
> >> > 3) As the pins are shared, not all 8 channels are actually wired to anything.
> >> > 4) This prevents you just using an available_scan_mask entry to enable them all.
> >> >    Hence you have rolled your own demux.  If you need to do this quick then
> >> >    take a look at the precomputed mux tables in the core.  If not I guess
> >> >    what you have works fine.  
> >> Yes, that code was borrowed from the simple dummy example module.
> >> I'm new to IIO, so I didn't realize a more efficient remuxing
> >> mechanism was available. I can look into integrating that on
> >> Monday.  
> > I'm thinking it's a non starter because of the lazy enabling, but maybe
> > there is some way to make it work.  
> >> >> > +static int dln2_adc_update_enabled_chans(struct dln2_adc *dln2)
> >> >> > +{
> >> >> > +   int ret, i, chan_count;
> >> >> > +   struct iio_dev *indio_dev;
> >> >> > +
> >> >> > +   if (dln2->chans_enabled == dln2->chans_requested)
> >> >> > +           return 0;
> >> >> > +
> >> >> > +   indio_dev = platform_get_drvdata(dln2->pdev);
> >> >> > +   chan_count = indio_dev->num_channels;
> >> >> > +
> >> >> > +   if (dln2->port_enabled) {
> >> >> > +           ret = dln2_adc_set_port_enabled(dln2, false);
> >> >> > +           if (ret < 0)
> >> >> > +                   return ret;
> >> >> > +           dln2->port_enabled = false;
> >> >> > +   }
> >> >> > +
> >> >> > +   if (!dln2->resolution_set) {
> >> >> > +           ret = dln2_adc_set_port_resolution(dln2);
> >> >> > +           if (ret < 0)
> >> >> > +                   return ret;
> >> >> > +           dln2->resolution_set = true;
> >> >> > +   }
> >> >> > +
> >> >> > +   for (i = 0; i < chan_count; ++i) {
> >> >> > +           bool requested = dln2->chans_requested & (1 << i);
> >> >> > +           bool enabled = dln2->chans_enabled & (1 << i);
> >> >> > +
> >> >> > +           if (requested == enabled)
> >> >> > +                   continue;
> >> >> > +           ret = dln2_adc_set_chan_enabled(dln2, i, requested);
> >> >> > +           if (ret < 0)
> >> >> > +                   return ret;
> >> >> > +   }
> >> >> > +
> >> >> > +   dln2->chans_enabled = dln2->chans_requested;
> >> >> > +
> >> >> > +   ret = dln2_adc_set_port_enabled(dln2, true);
> >> >> > +   if (ret < 0)
> >> >> > +           return ret;
> >> >> > +   dln2->port_enabled = true;
> >> >> > +
> >> >> > +   return 0;
> >> >> > +}
> >> >> > +
> >> >> > +static int dln2_adc_get_chan_freq(struct dln2_adc *dln2, unsigned int channel)
> >> >> > +{
> >> >> > +   int ret;
> >> >> > +   struct dln2_adc_port_chan port_chan = {
> >> >> > +           .port = dln2->port,
> >> >> > +           .chan = channel,
> >> >> > +   };
> >> >> > +   struct {
> >> >> > +           __u8 type;
> >> >> > +           __le16 period;
> >> >> > +           __le16 low;
> >> >> > +           __le16 high;
> >> >> > +   } __packed get_cfg;
> >> >> > +   int olen = sizeof(get_cfg);
> >> >> > +
> >> >> > +   ret = dln2_transfer(dln2->pdev, DLN2_ADC_CHANNEL_GET_CFG,
> >> >> > +                       &port_chan, sizeof(port_chan), &get_cfg, &olen);
> >> >> > +   if (ret < 0) {
> >> >> > +           dev_dbg(&dln2->pdev->dev, "Problem in %s\n", __func__);
> >> >> > +           return ret;
> >> >> > +   }
> >> >> > +   if (olen < sizeof(get_cfg))
> >> >> > +           return -EPROTO;
> >> >> > +
> >> >> > +   return get_cfg.period;  
> >> > Returning a value called period for a function claiming to be reading the
> >> > frequency is unusual. It's got to be converted to the frequency.  
> >> Yea, I screwed up there. Diolan's command interface uses periods
> >> in milliseconds so I'll make sure the necessary reciprocal gets
> >> put in and the function is clearly named.  
> >> >> > +}
> >> >> > +
> >> >> > +static int dln2_adc_set_chan_freq(struct dln2_adc *dln2, unsigned int channel,
> >> >> > +                             unsigned int freq)
> >> >> > +{
> >> >> > +   int ret;
> >> >> > +   struct {
> >> >> > +           struct dln2_adc_port_chan port_chan;
> >> >> > +           __u8 type;
> >> >> > +           __le16 period;
> >> >> > +           __le16 low;
> >> >> > +           __le16 high;
> >> >> > +   } __packed set_cfg = {
> >> >> > +           .port_chan.port = dln2->port,
> >> >> > +           .port_chan.chan = channel,
> >> >> > +           .type = freq ? DLN2_ADC_EVENT_ALWAYS : DLN2_ADC_EVENT_NONE,
> >> >> > +           .period = cpu_to_le16(freq)
> >> >> > +   };
> >> >> > +
> >> >> > +   ret = dln2_transfer_tx(dln2->pdev, DLN2_ADC_CHANNEL_SET_CFG,
> >> >> > +                          &set_cfg, sizeof(set_cfg));
> >> >> > +   if (ret < 0) {
> >> >> > +           dev_dbg(&dln2->pdev->dev, "Problem in %s\n", __func__);
> >> >> > +           return ret;
> >> >> > +   }
> >> >> > +   return 0;
> >> >> > +}
> >> >> > +
> >> >> > +static int dln2_adc_read(struct dln2_adc *dln2, unsigned int channel)
> >> >> > +{
> >> >> > +   int ret;
> >> >> > +   int old_chans_requested = dln2->chans_requested;
> >> >> > +
> >> >> > +   dln2->chans_requested |= (1 << channel);  
> >> > Ah, so this changes the enabled channels and can lead to the
> >> > changes in what is enabled when buffered mode is going on.
> >> >
> >> > I'd just use the iio_claim_direct_mode set of functions
> >> > to prevent direct reads when the buffer is operating.
> >> > They add a lot of complexity for what is usually a fairly
> >> > unusual requirement.  
> >> Will do.  
> >> >> > +   ret = dln2_adc_update_enabled_chans(dln2);
> >> >> > +   if (ret < 0) {
> >> >> > +           dln2->chans_requested = old_chans_requested;
> >> >> > +           return ret;
> >> >> > +   }
> >> >> > +   dln2->port_enabled = true;
> >> >> > +
> >> >> > +   struct dln2_adc_port_chan port_chan = {
> >> >> > +           .port = dln2->port,
> >> >> > +           .chan = channel,
> >> >> > +   };
> >> >> > +   __le16 value;
> >> >> > +   int olen = sizeof(value);
> >> >> > +
> >> >> > +   ret = dln2_transfer(dln2->pdev, DLN2_ADC_CHANNEL_GET_VAL,
> >> >> > +                       &port_chan, sizeof(port_chan), &value, &olen);
> >> >> > +   if (ret < 0) {
> >> >> > +           dev_dbg(&dln2->pdev->dev, "Problem in %s\n", __func__);
> >> >> > +           return ret;
> >> >> > +   }
> >> >> > +   if (olen < sizeof(value))
> >> >> > +           return -EPROTO;
> >> >> > +
> >> >> > +   return le16_to_cpu(value);
> >> >> > +}
> >> >> > +
> >> >> > +static int dln2_adc_read_all(struct dln2_adc *dln2,
> >> >> > +                        struct dln2_adc_get_all_vals *get_all_vals)
> >> >> > +{
> >> >> > +   int ret;
> >> >> > +   __u8 port = dln2->port;
> >> >> > +   int olen = sizeof(*get_all_vals);
> >> >> > +
> >> >> > +   ret = dln2_transfer(dln2->pdev, DLN2_ADC_CHANNEL_GET_ALL_VAL,
> >> >> > +                       &port, sizeof(port), get_all_vals, &olen);  
> >> > Getting channels that aren't wired to anything?  Fair enough if true,
> >> > but unusual.  
> >> I had the same thought myself; the adc example in Diolan's
> >> package reads in a fixed 8 channels every time, apparently
> >> zeroing the disabled ones.  
> > It would make their firmware simpler probably.  Push the complexity to
> > the more powerful main processor.  
> >> >> > +   if (ret < 0) {
> >> >> > +           dev_dbg(&dln2->pdev->dev, "Problem in %s\n", __func__);
> >> >> > +           return ret;
> >> >> > +   }
> >> >> > +   if (olen < sizeof(*get_all_vals))
> >> >> > +           return -EPROTO;
> >> >> > +
> >> >> > +   return 0;
> >> >> > +}
> >> >> > +
> >> >> > +static int dln2_adc_read_raw(struct iio_dev *indio_dev,
> >> >> > +                        struct iio_chan_spec const *chan,
> >> >> > +                        int *val,
> >> >> > +                        int *val2,
> >> >> > +                        long mask)
> >> >> > +{
> >> >> > +   int ret;
> >> >> > +   struct dln2_adc *dln2 = iio_priv(indio_dev);
> >> >> > +
> >> >> > +   switch (mask) {
> >> >> > +   case IIO_CHAN_INFO_RAW:
> >> >> > +           mutex_lock(&dln2->mutex);
> >> >> > +           ret = dln2_adc_read(dln2, chan->channel);
> >> >> > +           mutex_unlock(&dln2->mutex);
> >> >> > +
> >> >> > +           if (ret < 0)
> >> >> > +                   return ret;
> >> >> > +
> >> >> > +           *val = ret;
> >> >> > +           return IIO_VAL_INT;
> >> >> > +
> >> >> > +   case IIO_CHAN_INFO_SCALE:
> >> >> > +           *val = 0;
> >> >> > +           /* 3.3 / (1 << 10) * 1000000000 */
> >> >> > +           *val2 = 3222656;
> >> >> > +           return IIO_VAL_INT_PLUS_NANO;
> >> >> > +
> >> >> > +   case IIO_CHAN_INFO_SAMP_FREQ:
> >> >> > +           mutex_lock(&dln2->mutex);
> >> >> > +           if (dln2->trigger_chan == -1)
> >> >> > +                   ret = 0;
> >> >> > +           else
> >> >> > +                   ret = dln2_adc_get_chan_freq(dln2, dln2->trigger_chan);
> >> >> > +           mutex_unlock(&dln2->mutex);
> >> >> > +
> >> >> > +           if (ret < 0)
> >> >> > +                   return ret;
> >> >> > +
> >> >> > +           *val = ret / 1000;
> >> >> > +           *val2 = (ret % 1000) * 1000;
> >> >> > +           return IIO_VAL_INT_PLUS_MICRO;
> >> >> > +
> >> >> > +   default:
> >> >> > +           return -EINVAL;
> >> >> > +   }
> >> >> > +}
> >> >> > +
> >> >> > +static int dln2_adc_write_raw(struct iio_dev *indio_dev,
> >> >> > +                         struct iio_chan_spec const *chan,
> >> >> > +                         int val,
> >> >> > +                         int val2,
> >> >> > +                         long mask)
> >> >> > +{
> >> >> > +   int ret;
> >> >> > +   unsigned int freq;
> >> >> > +   struct dln2_adc *dln2 = iio_priv(indio_dev);
> >> >> > +
> >> >> > +   switch (mask) {
> >> >> > +   case IIO_CHAN_INFO_SAMP_FREQ:
> >> >> > +           freq = val * 1000 + val2 / 1000;
> >> >> > +           if (freq > 65535) {
> >> >> > +                   freq = 65535;
> >> >> > +                   dev_warn(&dln2->pdev->dev,
> >> >> > +                            "clamping freq to 65535ms\n");  
> >> > frequency measured in a unit of time?  
> >> Will fix Monday  
> >> >> > +           }
> >> >> > +           mutex_lock(&dln2->mutex);
> >> >> > +
> >> >> > +           /*
> >> >> > +            * The first requested channel is arbitrated as a shared
> >> >> > +            * trigger source, so only one event is registered with the DLN.
> >> >> > +            * The event handler will then read all enabled channel values
> >> >> > +            * using DLN2_ADC_CHANNEL_GET_ALL_VAL to maintain
> >> >> > +            * synchronization between ADC readings.
> >> >> > +            */
> >> >> > +           if (dln2->trigger_chan == -1)
> >> >> > +                   dln2->trigger_chan = chan->channel;  
> >> > This looks dangerous. either it is always fine to use a random channel or it's
> >> > not...
> >> >
> >> > I'd cache the frequency and only actually set it when bringing up the buffer
> >> > - that way if the setup is sane you will have a channel.  
> >> Fair point, I'll refactor it that way.  
> >> >> > +           ret = dln2_adc_set_chan_freq(dln2, dln2->trigger_chan, freq);
> >> >> > +
> >> >> > +           mutex_unlock(&dln2->mutex);
> >> >> > +
> >> >> > +           if (ret < 0)
> >> >> > +                   return ret;
> >> >> > +
> >> >> > +           return 0;  
> >> > Just return ret and loose the other path.  I chased through the code
> >> > and we get 0 for success.  
> >> Ok  
> >> >> > +
> >> >> > +   default:
> >> >> > +           return -EINVAL;
> >> >> > +   }
> >> >> > +}
> >> >> > +
> >> >> > +#define DLN2_ADC_CHAN(idx) {                                       \
> >> >> > +   .type = IIO_VOLTAGE,                                    \
> >> >> > +   .channel = idx,                                         \
> >> >> > +   .indexed = 1,                                           \
> >> >> > +   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
> >> >> > +   .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE) |   \
> >> >> > +                              BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> >> >> > +   .scan_index = idx,                                      \  
> >> > .scan_type = {
> >> >         .sign = 'u', etc is easier to read.  
> >> Ok  
> >> >> > +   .scan_type.sign = 'u',                                  \
> >> >> > +   .scan_type.realbits = DLN2_ADC_DATA_BITS,               \
> >> >> > +   .scan_type.storagebits = 16,                            \
> >> >> > +   .scan_type.endianness = IIO_LE,                         \
> >> >> > +}
> >> >> > +
> >> >> > +static const struct iio_chan_spec dln2_adc_iio_channels[] = {
> >> >> > +   DLN2_ADC_CHAN(0),
> >> >> > +   DLN2_ADC_CHAN(1),
> >> >> > +   DLN2_ADC_CHAN(2),
> >> >> > +   DLN2_ADC_CHAN(3),
> >> >> > +   DLN2_ADC_CHAN(4),
> >> >> > +   DLN2_ADC_CHAN(5),
> >> >> > +   DLN2_ADC_CHAN(6),
> >> >> > +   DLN2_ADC_CHAN(7),
> >> >> > +   IIO_CHAN_SOFT_TIMESTAMP(8),
> >> >> > +};
> >> >> > +
> >> >> > +static const struct iio_info dln2_adc_info = {
> >> >> > +   .read_raw = dln2_adc_read_raw,
> >> >> > +   .write_raw = dln2_adc_write_raw,
> >> >> > +   .driver_module = THIS_MODULE,
> >> >> > +};
> >> >> > +
> >> >> > +static irqreturn_t dln2_adc_trigger_h(int irq, void *p)
> >> >> > +{
> >> >> > +   struct iio_poll_func *pf = p;
> >> >> > +   struct iio_dev *indio_dev = pf->indio_dev;
> >> >> > +   struct {
> >> >> > +           __le16 values[DLN2_ADC_MAX_CHANNELS];
> >> >> > +           int64_t timestamp_space;
> >> >> > +   } data;
> >> >> > +   struct dln2_adc_get_all_vals dev_data;
> >> >> > +   struct dln2_adc *dln2 = iio_priv(indio_dev);
> >> >> > +   int old_chans_requested;
> >> >> > +   int i, j;
> >> >> > +
> >> >> > +   mutex_lock(&dln2->mutex);
> >> >> > +
> >> >> > +   old_chans_requested = dln2->chans_requested;
> >> >> > +   dln2->chans_requested |= *indio_dev->active_scan_mask;
> >> >> > +   if (dln2_adc_update_enabled_chans(dln2) < 0) {
> >> >> > +           dln2->chans_requested = old_chans_requested;  
> >> > I'm confused. Why would this change here?  Shouldn't change
> >> > whilst data is being captured unless something very odd is going on.
> >> > Ah, found it - you want to allow sysfs reads at the same time.
> >> > Given the complexity required to do that safely, I'd just not
> >> > do it for now - maybe add it as a follow up patch later.  
> >> >> > +           mutex_unlock(&dln2->mutex);
> >> >> > +           goto done;
> >> >> > +   }
> >> >> > +
> >> >> > +   if (dln2_adc_read_all(dln2, &dev_data) < 0) {
> >> >> > +           mutex_unlock(&dln2->mutex);
> >> >> > +           goto done;
> >> >> > +   }
> >> >> > +
> >> >> > +   mutex_unlock(&dln2->mutex);
> >> >> > +
> >> >> > +   for (i = 0, j = 0;  
> >> >
> >> > Having found the note about lazy enabling above I think I now
> >> > understand what you are doing here.  You can't use the
> >> > core infrastructure and set available_scan_masks to 0x1FF because
> >> > that would result in enabling all the pins, some of which are wanted
> >> > for gpio usage perhaps.
> >> >
> >> > Hence you end up rolling your own demux.  You are reading all
> >> > the ADC channels, but some aren't necessarily connected to any actual
> >> > pins.
> >> >  
> >> Precisely. The lazy enabling adapts to the demand of the user.
> >> Since IIO doesn't really have an export mechanism like the GPIO
> >> subsystem, I can't preemptively enable channels unless I make the
> >> driver buffer-only (which I'd rather not do cause direct reads
> >> come in handy).  
> > one of those fiddly 'boundary' cases for defining connectivity.
> > If we were talking hard wired pins on a board this would all go
> > in device tree, but in reality most of the time we have
> > no idea what is connected to a given DLN2.  
> >> >> > +        i < bitmap_weight(indio_dev->active_scan_mask,
> >> >> > +                          indio_dev->masklength); i++, j++) {
> >> >> > +           j = find_next_bit(indio_dev->active_scan_mask,
> >> >> > +                             indio_dev->masklength, j);
> >> >> > +           data.values[i] = dev_data.values[j];
> >> >> > +   }
> >> >> > +
> >> >> > +   iio_push_to_buffers_with_timestamp(indio_dev, &data,
> >> >> > +                                      iio_get_time_ns(indio_dev));
> >> >> > +
> >> >> > +done:
> >> >> > +   iio_trigger_notify_done(indio_dev->trig);
> >> >> > +   return IRQ_HANDLED;
> >> >> > +}
> >> >> > +
> >> >> > +static int dln2_adc_triggered_buffer_postenable(struct iio_dev *indio_dev)
> >> >> > +{
> >> >> > +   int ret;
> >> >> > +   struct dln2_adc *dln2 = iio_priv(indio_dev);
> >> >> > +
> >> >> > +   mutex_lock(&dln2->mutex);
> >> >> > +   dln2->chans_requested |= *indio_dev->active_scan_mask;  
> >> > Perhaps should be in the update_scan_mask callback rather than here?
> >> > You could have multiple consumers of this data which would cause
> >> > this path to be 'interesting'.
> >> >
> >> > I'm also a little curious to know why it isn't just an =
> >> > Also why don't you want to disable them when then buffer is disabled.  
> >> This allows the enabled channels to remain enabled if the user
> >> opts for direct raw reads. If I were to enable a channel
> >> temporarily for a raw read, that's 6 or 7 synchronous commands:
> >> - Set resolution (if not already done)
> >> - Enable channel
> >> - Enable port
> >> - Get value
> >> - Get value (a second one is needed because the first read after
> >>   enabling always seems to return 0, must be a firmware bug)
> >> - Disable port
> >> - Disable channel
> >>
> >> This just strikes me as inefficient, but it would be more
> >> consistent state-wise. If you think this is an ok trade-off,
> >> I'll go for the enable/disable approach. I suppose it's also
> >> possible to reset the lazy channel mask when the user exits
> >> buffered mode. The initial zero-return after enabling channels is
> >> a real kicker, and makes me want to keep disables/enables to a
> >> minimum.  
> > That is a pain.  Still more consistent to clear the channel mask
> > again after a buffer disable + prevent single channel reads
> > whilst it is running.  At least, do this for the first merge
> > of the driver then we can revisit if you have a usecase
> > where the juggling is really necessary.
> >  
> >> >> > +   ret = dln2_adc_update_enabled_chans(dln2);
> >> >> > +   mutex_unlock(&dln2->mutex);
> >> >> > +   if (ret < 0) {
> >> >> > +           dev_dbg(&dln2->pdev->dev, "Problem in %s\n", __func__);
> >> >> > +           return ret;
> >> >> > +   }
> >> >> > +
> >> >> > +   return iio_triggered_buffer_postenable(indio_dev);
> >> >> > +}
> >> >> > +
> >> >> > +static const struct iio_buffer_setup_ops dln2_adc_buffer_setup_ops = {
> >> >> > +   .postenable = dln2_adc_triggered_buffer_postenable,
> >> >> > +   .predisable = iio_triggered_buffer_predisable,
> >> >> > +};
> >> >> > +
> >> >> > +static void dln2_adc_event(struct platform_device *pdev, u16 echo,
> >> >> > +                      const void *data, int len)
> >> >> > +{
> >> >> > +   struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >> >> > +   struct dln2_adc *dln2 = iio_priv(indio_dev);
> >> >> > +
> >> >> > +   iio_trigger_poll(dln2->trig);  
> >> > Is this in interrupt context?
> >> > if not should be iio_trigger_poll_chained(dln2->trig);  
> >> I'll double check that on Monday, the MFD code would determine
> >> that.  
> >> >> > +}
> >> >> > +
> >> >> > +static const struct iio_trigger_ops dln2_adc_trigger_ops = {
> >> >> > +   .owner = THIS_MODULE,  
> >> > Hohum.  Patch set to remove the need for this is waiting for me
> >> > to get around to revising it.  Interestingly checking why you
> >> > had this at all lead me to a check in the trigger register that
> >> > I wasn't getting rid of in that patch set. oops.
> >> >
> >> > Upshot is that the equivalent is done via macro magic rather
> >> > than an explicit entry in this structure.  It's possible this
> >> > driver will cross with that series, in which case we'll get
> >> > some build issues that will need fixing up.  I'll hopefully
> >> > remember to deal with them when applying though.  
> >> >> > +};
> >> >> > +
> >> >> > +static int dln2_adc_probe(struct platform_device *pdev)
> >> >> > +{
> >> >> > +   struct device *dev = pdev->dev;
> >> >> > +   struct dln2_adc *dln2;
> >> >> > +   struct dln2_platform_data *pdata = dev_get_platdata(&pdev->dev);
> >> >> > +   struct iio_dev *indio_dev;
> >> >> > +   struct iio_buffer *buffer;
> >> >> > +   int ret;
> >> >> > +   int chans;
> >> >> > +
> >> >> > +   indio_dev = devm_iio_device_alloc(dev, sizeof(struct dln2_adc));  
> >> > Slight preference for sizeof(*dln2)  
> >> Agreed  
> >> >> > +   if (!indio_dev) {
> >> >> > +           dev_err(dev, "failed allocating iio device\n");
> >> >> > +           return -ENOMEM;
> >> >> > +   }
> >> >> > +
> >> >> > +   dln2 = iio_priv(indio_dev);
> >> >> > +   dln2->pdev = pdev;
> >> >> > +   dln2->port = pdata->port;
> >> >> > +   mutex_init(&dln2->mutex);
> >> >> > +   dln2->port_enabled = false;
> >> >> > +   dln2->resolution_set = false;
> >> >> > +   dln2->chans_requested = 0;
> >> >> > +   dln2->chans_enabled = 0;
> >> >> > +   dln2->trigger_chan = -1;  
> >> > This is fine at setup, but isn't reset to -1 if the channels are all disabled.  
> >> Ok  
> >> >> > +
> >> >> > +   platform_set_drvdata(pdev, indio_dev);
> >> >> > +
> >> >> > +   chans = dln2_adc_get_chan_count(dln2);
> >> >> > +   if (chans < 0) {
> >> >> > +           dev_err(dev, "failed to get channel count: %d\n", chans);
> >> >> > +           ret = chans;
> >> >> > +           goto dealloc_dev;
> >> >> > +   }
> >> >> > +   if (chans > DLN2_ADC_MAX_CHANNELS) {
> >> >> > +           chans = DLN2_ADC_MAX_CHANNELS;
> >> >> > +           dev_warn(dev, "clamping channels to %d\n",
> >> >> > +                    DLN2_ADC_MAX_CHANNELS);
> >> >> > +   }
> >> >> > +
> >> >> > +   indio_dev->name = DLN2_ADC_MOD_NAME;
> >> >> > +   indio_dev->dev.parent = dev;
> >> >> > +   indio_dev->info = &dln2_adc_info;
> >> >> > +   indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
> >> >> > +   indio_dev->channels = dln2_adc_iio_channels;
> >> >> > +   indio_dev->num_channels = chans + 1;  
> >> > Why +1?  Presumably to include the timestamp.  I guess this
> >> > means that the code that gets the channel count always gets 8.
> >> > You need to check that if you are going to use a const channel array.  
> >> Yea, I see in the docs that's supposed to be monotonic.
> >> I'll assemble that array on the stack instead at probe time to be
> >> safe.  
> >> >> > +   indio_dev->setup_ops = &dln2_adc_buffer_setup_ops;
> >> >> > +
> >> >> > +   dln2->trig = devm_iio_trigger_alloc(dev, "samplerate");  
> >> > Interesting naming for a trigger... rather too generic + doesn't
> >> > cope with multiple dln2's connect to same machine. Admittedly
> >> > a number of historical drivers have that problem but we can't
> >> > fix them now without ABI breakage.
> >> >  
> >> Good point, is there a preferred unique identifier to suffix that
> >> name?  
> > we've never restricted this particularly as it's just a magic string
> > to match things up.
> >
> > Quite a few drivers stick with "magicname-dev%d",  ..., indio_dev->id  
> >> >> > +   if (!dln2->trig) {
> >> >> > +           dev_err(dev, "failed to allocate trigger\n");
> >> >> > +           ret = -ENOMEM;
> >> >> > +           goto dealloc_dev;
> >> >> > +   }
> >> >> > +   dln2->trig->ops = &dln2_adc_trigger_ops;
> >> >> > +   iio_trigger_set_drvdata(dln2->trig, dln2);
> >> >> > +   iio_trigger_register(dln2->trig);
> >> >> > +   iio_trigger_set_immutable(indio_dev, dln2->trig);
> >> >> > +
> >> >> > +   buffer = devm_iio_kfifo_allocate(dev);
> >> >> > +   if (!buffer) {
> >> >> > +           dev_err(dev, "failed to allocate kfifo\n");
> >> >> > +           ret = -ENOMEM;
> >> >> > +           goto dealloc_trigger;
> >> >> > +   }
> >> >> > +
> >> >> > +   iio_device_attach_buffer(indio_dev, buffer);
> >> >> > +
> >> >> > +   indio_dev->pollfunc = iio_alloc_pollfunc(NULL,
> >> >> > +                                            &dln2_adc_trigger_h,
> >> >> > +                                            IRQF_ONESHOT,
> >> >> > +                                            indio_dev,
> >> >> > +                                            "samplerate");  
> >> > Again very generic naming + shouldn't be the same as the trigger name.
> >> > Almost no drivers do this manually any more.  Why does the
> >> > iio_triggered_buffer_setup boilerplate function not work here?  
> >> I read about this in the IIO presentation from back when the
> >> subsystem was first introduced. Didn't realize it was
> >> depreciated.  
> > It's not deprecated as such, just that there are utility functions
> > to make the job easier if you happen to fit with the standard forms.
> > Just code repetition reduction really.  This approach may still
> > be necessary if something more 'interesting' is going on.  
> >> >> > +
> >> >> > +   if (!indio_dev->pollfunc) {
> >> >> > +           ret = -ENOMEM;
> >> >> > +           goto dealloc_kfifo;
> >> >> > +   }
> >> >> > +
> >> >> > +   ret = dln2_register_event_cb(pdev, DLN2_ADC_CONDITION_MET_EV,
> >> >> > +                                dln2_adc_event);
> >> >> > +   if (ret) {
> >> >> > +           dev_err(dev, "failed to register event cb: %d\n", ret);
> >> >> > +           goto dealloc_pollfunc;
> >> >> > +   }
> >> >> > +
> >> >> > +   ret = iio_device_register(indio_dev);
> >> >> > +   if (ret) {
> >> >> > +           dev_err(dev, "failed to register iio device: %d\n", ret);
> >> >> > +           goto dealloc_pollfunc;
> >> >> > +   }
> >> >> > +
> >> >> > +   return 0;
> >> >> > +
> >> >> > +dealloc_pollfunc:
> >> >> > +   iio_dealloc_pollfunc(indio_dev->pollfunc);
> >> >> > +dealloc_kfifo:
> >> >> > +dealloc_trigger:
> >> >> > +   iio_trigger_unregister(dln2->trig);
> >> >> > +dealloc_dev:
> >> >> > +
> >> >> > +   return ret;
> >> >> > +}
> >> >> > +
> >> >> > +static int dln2_adc_remove(struct platform_device *pdev)
> >> >> > +{
> >> >> > +   struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >> >> > +   struct dln2_adc *dln2 = iio_priv(indio_dev);
> >> >> > +
> >> >> > +   iio_device_unregister(indio_dev);
> >> >> > +   dln2_unregister_event_cb(pdev, DLN2_ADC_CONDITION_MET_EV);
> >> >> > +   iio_trigger_unregister(dln2->trig);
> >> >> > +   iio_dealloc_pollfunc(indio_dev->pollfunc);
> >> >> > +
> >> >> > +   return 0;
> >> >> > +}
> >> >> > +
> >> >> > +static struct platform_driver dln2_adc_driver = {
> >> >> > +   .driver.name    = DLN2_ADC_MOD_NAME,
> >> >> > +   .probe          = dln2_adc_probe,
> >> >> > +   .remove         = dln2_adc_remove,
> >> >> > +};
> >> >> > +
> >> >> > +module_platform_driver(dln2_adc_driver);
> >> >> > +
> >> >> > +MODULE_AUTHOR("Jack Andersen <jackoalan@gmail.com");
> >> >> > +MODULE_DESCRIPTION("Driver for the Diolan DLN2 ADC interface");
> >> >> > +MODULE_LICENSE("GPL v2");
> >> >> > +MODULE_ALIAS("platform:dln2-adc");
> >> >> > diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> >> >> > index 704e189..a22ab8c 100644
> >> >> > --- a/drivers/mfd/dln2.c
> >> >> > +++ b/drivers/mfd/dln2.c
> >> >> > @@ -53,6 +53,7 @@ enum dln2_handle {
> >> >> >     DLN2_HANDLE_GPIO,
> >> >> >     DLN2_HANDLE_I2C,
> >> >> >     DLN2_HANDLE_SPI,
> >> >> > +   DLN2_HANDLE_ADC,
> >> >> >     DLN2_HANDLES
> >> >> >  };
> >> >> >
> >> >> > @@ -663,6 +664,12 @@ static int dln2_start_rx_urbs(struct dln2_dev *dln2, gfp_t gfp)
> >> >> >     .port = 0,
> >> >> >  };
> >> >> >
> >> >> > +/* Only one ADC port supported */
> >> >> > +static struct dln2_platform_data dln2_pdata_adc = {
> >> >> > +   .handle = DLN2_HANDLE_ADC,
> >> >> > +   .port = 0,
> >> >> > +};
> >> >> > +
> >> >> >  static const struct mfd_cell dln2_devs[] = {
> >> >> >     {
> >> >> >             .name = "dln2-gpio",
> >> >> > @@ -679,6 +686,11 @@ static int dln2_start_rx_urbs(struct dln2_dev *dln2, gfp_t gfp)
> >> >> >             .platform_data = &dln2_pdata_spi,
> >> >> >             .pdata_size = sizeof(struct dln2_platform_data),
> >> >> >     },
> >> >> > +   {
> >> >> > +           .name = "dln2-adc",
> >> >> > +           .platform_data = &dln2_pdata_adc,
> >> >> > +           .pdata_size = sizeof(struct dln2_platform_data),
> >> >> > +   },
> >> >> >  };
> >> >> >
> >> >> >  static void dln2_stop(struct dln2_dev *dln2)
> >> >> >  
> >> >>  
> >> >  
> >  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>