On Sun, 2 May 2021 20:48:47 +0000 József Horváth <info@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> > > > --- ... > > > > > > > > + > > > + 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@xxxxxxxxxxx>"); > > > +MODULE_DESCRIPTION("Texas Instruments TI_ADS7142 ADC driver"); > > > > >
