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

["József Horváth" <info@ministro.hu>]

On Mon, May 03, 2021 at 11:26:00AM +0100, Jonathan Cameron wrote:
> 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.

I will try to bring it into sysfs.

> 
> > 
> > > 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.
> 

Ok, I'll come back a possible solution soon.

> > 
> > > > 
> > > > 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()
> 

'..._setup' is ok for me.

> 
> ...
> 
> >  
> > > > +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. 
> 

I'ts clear now, include/linux/property.h was a good starting point. Thank you.

> 
> > 
> > >   
> > > > +	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");  
> > > 
> > >   
> > 
> 

Best regards
Jozsef