Re: [PATCH 1/2] iio: adc: driver for texas instruments ads7142

Jonathan Cameron
2021-05-03T10:26:52Z

On Sun, 2 May 2021 20:48:47 +0000
József Horváth <info@ministro.hu> wrote:

> On Sun, May 02, 2021 at 06:14:23PM +0100, Jonathan Cameron wrote:
> > On Sat, 1 May 2021 18:24:28 +0000
> > Jozsef Horvath <info@ministro.hu> wrote:
> >
> > > This is an iio driver for
> > > Texas Instruments ADS7142 dual-channel, programmable sensor monitor.
> > >
> > > Operation modes supportedby the driver:
> > > When the 'ti,monitoring-mode' property is not present
> > > in the devicetree node definition, the driver initiates a single
> > > conversion in the device for each read request
> > > (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> > > This is a one-shot conversion, and it is called
> > > "Manual Mode" in the datasheet.
> > >
> > > When the 'ti,monitoring-mode' property is present
> > > in the devicetree node definition, the driver configures
> > > the device's digital window comparator and sets the device's
> > > data buffer operation mode to pre alert data mode.
> > > The driver reads the conversion result when the BUSY/RDY interrupt
> > > fires, and keeps the value until the next BUSY/RDY interrupt
> > > or the first read request
> > > (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> >
> > Hi Jozsef.
> >
> > Interesting device - somewhat like an impact sensor, but on a general
> > purpose ADC.
>
> Yes, but now I'm using as an ADC in my project.
> In my point of view this is a general purpose ADC with monitoring features.
>
> >
> > Hmm. This sounds rather unintuitive and also very much like a policy
> > decision rather than anything to do with the hardware. Hence it
> > should almost certainly be in control of userspace and no via
> > dt parameters.
> >
>
> I think that, this operation modes are not generic enough to bring it to sysfs.

That just means you need to figure out how to do it, not make it a
boot time, or board flash time based control.

>
> > The interrupt driven nature of the device implies that a polled interface
> > such as sysfs is not appropriate to support this mode.
> >
> > Based on the description you have given here and a quick look
> > at the flow charts in the datasheet I would suggest.
> > 1) Enable sysfs reads as manual mode only.
> > 2) Implement the buffered part of an IIO driver. This is what we use
> > for data where autonomous clocking is going on.
>
> I'll check the buffered api.
>
> > 3) Add triggers to represent the different autonomous modes. In some
> > sense all the modes present can be considered be a series of
> > 'capture now' signals that are being generated by the hardware in
> > response to some event'.
> >
> > So you'd have a pre_alert_tigger, post_alert_trigger
> > Stop_burst and start_burst are more interesting to handle because you
> > will need something to actually start/stop them. These could be done
> > via a sysfs attribute for the trigger, or more complex schemes exist
> > such as triggering them off another trigger... one or two of the SoC
> > ADCs do that sort of thing.
> >
> >
> > > The digital window comparator and hysteresis parameters
> > > can be controlled by:
> > > - the devicetree definition of channel node
> > > - iio sysfs interfaces
> > > This is event driven conversion, and is called
> > > "Autonomous Mode with Pre Alert Data" in the datasheet.
> > > This mode can be used to wake up the system with the ALERT pin,
> > > in case when the monitored voltage level is out of the configured range.
> >
> > Whilst it's fine to only enable the modes you want, we should think about how
> > to ensure other modes can be supported.
> >
>
> As I described above, I would keep the operation modes in dt, and
> 'ti,monitoring-mode' can be an enum.

Sorry but no. Unless you can make a 'very' strong argument of the
fact that this a characteristic of the hardware setup (wiring etc) then
it needs to be userspace controlled.

>
> > >
> > > Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
> > >
> > > Signed-off-by: Jozsef Horvath <info@ministro.hu>
> > > ---

...

>
> >
> >
> > > +
> > > + return ret >= 0 ? 0 : ret;
> >
> > if ret == 0 then something went wrong and we should report that.
>
> You are right
>
> > > + channel->data.value = value;
> > > + *channel_collected |= 1 << channel_address;
> > > + }
> > > + }
> > > + } while (--data_buffer_status);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int ti_ads7142_do_work(struct iio_dev *indio_dev)
> >
> > As mentioned below, these function needs a more informative name.
>
> I'll change it to ..._do_monitoring_work, and create something like
> start_pre_alert_monitoring, start_post_alert_monitoring, etc

Maybe, but I'd expect 'work' to imply it was the function called on
each cycle of monitoring. This is more monitoring_setup()

...

>
> > > +static int ti_ads7142_read_event_config(struct iio_dev *indio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir)
> > > +{
> > > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > > + struct ti_ads7142_channel *channel;
> > > + int ret;
> > > +
> > > + if (!priv->config.monitoring_mode)
> > > + return -EINVAL;
> > > +
> > > + if (type != IIO_EV_TYPE_THRESH)
> > > + return -EINVAL;
> > > +
> > > + ret = ti_ads7142_address2channel(indio_dev, chan->address,
> > > + &channel);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (dir == IIO_EV_DIR_RISING)
> > > + ret = channel->config.alert_high ? 1 : 0;
> >
> > Not fine using ret = channel->config.alert_high; directly?
> >
>
> alert_high is bool, ret is int.
> I know, the 'true' value is 1, and its autmatically casted,
> but who knows the future...I would keep this, if possible.

Ok. Assuming the c spec will change to make this invalid is a bit implausible,
but it's not too bad I guess.

>
...

>
> >
> > > + &ads_channel->config.high_threshold);
> > > + ads_channel->config.alert_high = !ret;
> > > + ret = of_property_read_u32(channel_node, "ti,threshold-falling",
> > > + &ads_channel->config.low_threshold);
> > > + ads_channel->config.alert_low = !ret;
> > > + ret = of_property_read_u32(channel_node, "ti,hysteresis",
> > > + &ads_channel->config.hysteresis);
> > > + channel_index++;
> > > + }
> > > +
> > > + return 0;
> > > +err:
> > > + of_node_put(channel_node);
> > > + return ret;
> > > +}
> > > +
> > > +static int ti_ads7142_parse_config_of(struct device *dev,
> > > + struct iio_dev *indio_dev)
> > > +{
> > > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > > +
> > > + priv->config.osc_sel = of_property_read_bool(dev->of_node,
> > > + "ti,osc-sel");
> >
> > Please use generic device property access functions where possible.
> > That basically gives us support on non OF based platforms for free.
>
> Could you please explain this, I dont understand.

See include/linux/property.h

That provides firmware type agnostic property accessors - so they will work
whether the property is provided via ACPI or via DT (or a number of other
options though those are less common).

ACPI DSDT tables can use a special device type PRP0001 which basically is a
wrapper for device tree properties. So the properties are all the same
(including compatible) but you need to use the generic accessors to be able to
read them.

>
> >
> > > + of_property_read_u32(dev->of_node, "ti,n-clk", &priv->config.n_clk);
> > > + priv->config.monitoring_mode = of_property_read_bool(dev->of_node,
> > > + "ti,monitoring-mode");
> > > +
> > > + return ti_ads7142_parse_channel_config_of(dev, indio_dev);
> > > +}
>
...
> Thank you for the review and suggestions.

You are welcome.

Thanks,

Jonathan

>
> Best regards
> Jozsef

>
> >
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +module_i2c_driver(ti_ads7142_driver);
> > > +
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("Jozsef Horvath <info@ministro.hu>");
> > > +MODULE_DESCRIPTION("Texas Instruments TI_ADS7142 ADC driver");
> >
> >
>