Re: [PATCH 1/2] iio: adc: driver for texas instruments ads7142
From: József Horváth
Date: Sun May 02 2021 - 16:49:30 EST
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.
> 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.
> >
> > Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
> >
> > Signed-off-by: Jozsef Horvath <info@xxxxxxxxxxx>
> > ---
> Should only be one ---
>
> comments inline.
> > ---
> > MAINTAINERS | 6 +
> > drivers/iio/adc/Kconfig | 10 +
> > drivers/iio/adc/Makefile | 1 +
> >
> > +config TI_ADS7142
> > + tristate "Texas Instruments ADS7142 ADC driver"
> > + depends on I2C
> > + help
> > + This driver is for Texas Instruments ADS7142 Nanopower, Dual-Channel, Programmable Sensor Monitor.
>
> Please keep to shorter lines in Kconfig files < 80 chars preferred.
Ok, you are right.
>
> > + Say 'Y' here if you wish to use it.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called ti-ads7142.
> > + * Copyright (C) 2020 Jozsef Horvath <info@xxxxxxxxxxx>
> > + *
> blank line not needed, plus maybe 2021 given when you are posting it?
>
> > +}
> > +
> > +static int ti_ads7142_reg_read(const struct i2c_client *client, u8 reg,
> > + u8 *data)
> > +{
> > + struct i2c_msg msg[2];
> Use c99 assignment to do this as something like.
>
> struct i2c_msg msg[2] = {
> {
> .addr = client->addr,
> .len = 2,
> .buf = buf,
> }, {
> .addr = client->addr,
> .flags = I2C_M_RD,
> .len = 1,
> .buf = data,
> }
> } ;
> > + u8 buf[2];
>
> u8 buf[2] = { TI_..., reg };
>
Ok, you are right
> > +static int ti_ads7142_data_buffer_read(const struct i2c_client *client,
> > + int length, void *data)
> > +{
> > + struct i2c_msg msg;
> > + int ret;
> > +
> > + msg.addr = client->addr;
> > + msg.flags = I2C_M_RD;
> > + msg.len = length;
> > + msg.buf = data;
> > +
> > + ret = i2c_transfer(client->adapter, &msg, 1);
>
> Looks very similar to an i2c_smbus_read_block_data call though I suppose it
> is a little odd to use mixture of smbus and non in one driver.
>
I would use i2c_transfer, or i2c_master_recv could better.
> > +static int ti_ads7142_soft_reset(const struct i2c_client *client)
> > +{
> > + struct i2c_msg msg;
> > + u8 buf[2];
> u8 buf[2] = { TI_ADS7142_OC_GENERAL, 0x06 }; is more compact for no loss
> of readability.
>
Ok, I'll do that
> > + int ret;
> > +
> > + buf[0] = TI_ADS7142_OC_GENERAL;
> > + buf[1] = 0x06;
> > +
> > + msg.addr = client->addr;
> > + msg.flags = 0;
> > + msg.len = 2;
> > + msg.buf = buf;
> > +
> > + ret = i2c_transfer(client->adapter, &msg, 1);
>
> i2c_master_send() or even better isn't this just an open coded
> i2c_smbus_write_byte_data()
You are right, I'll change to i2c_master_send
>
>
> > +
> > + 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
> > +static int ti_ads7142_read_channel_manual(struct iio_dev *indio_dev,
> > + int address, int *val)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> > + u16 data_buffer;
> > + int ret;
> > +
> > + if (address < 0 || address > 1)
>
> I'm assuming there is no way we could get here with this not being true?
> If so drop it. If it is possible, then add a comment as it seems like
> an odd thing to need to check.
>
I'ts get called by iio_info.read_raw, so if it's not require to check, I'll remove it.
> > +static int ti_ads7142_read_channel_monitor(struct iio_dev *indio_dev,
> > + int address, int *val)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + struct ti_ads7142_channel *channel;
> > + int ret;
> > +
> > + if (address < 0 || address > 1)
> > + return -EINVAL;
> > +
> > + ret = ti_ads7142_address2channel(indio_dev, address, &channel);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&priv->lock);
> > + if (!channel->data.status) {
> > + ret = -EAGAIN;
> > + } else {
> > + *val = channel->data.value;
> > + channel->data.status = 0;
> > + ret = 0;
>
> ret already is 0 so no need to set it.
You are right.
> > +static irqreturn_t ti_ads7142_ist(int irq, void *dev_id)
> > +{
> > + struct iio_dev *indio_dev = dev_id;
> > + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + struct ti_ads7142_channel *channel;
> > + u8 low_flags;
> > + u8 high_flags;
> > + u8 seq_st;
> > + int i;
> > + int ret;
> > + int channel_collected;
> > + s64 timestamp = iio_get_time_ns(indio_dev);
> > +
> > + mutex_lock(&priv->lock);
> > + if (!priv->config.monitoring_mode || !priv->monitor_pending) {
> > + mutex_unlock(&priv->lock);
> > + return IRQ_NONE;
> > + }
> > +
> > + ret = ti_ads7142_reg_read(client, TI_ADS7142_SEQUENCE_STATUS, &seq_st);
> > + if (ret) {
> > + dev_err(indio_dev->dev.parent,
> > + "%s: SEQUENCE_STATUS reg read error(%i)",
> > + __func__, ret);
> > + goto final;
> > + }
> > +
> > + if ((seq_st & TI_ADS7142_SEQUENCE_STATUS_SEQ_ERR_ST_MSK)
> > + != TI_ADS7142_SEQUENCE_STATUS_SEQ_ENABLED) {
> > + dev_err(indio_dev->dev.parent,
> > + "%s: SEQUENCE_STATUS error(%i)",
> > + __func__, seq_st);
> > + goto final;
> > + }
> > +
> > + ret = ti_ads7142_reg_read(client, TI_ADS7142_ALT_LOW_FLAGS,
> > + &low_flags);
> > + if (ret) {
> > + dev_err(indio_dev->dev.parent,
> > + "%s: ALT_LOW_FLAGS reg read error(%i)",
> > + __func__, ret);
> > + goto final;
> > + }
> > +
> > + ret = ti_ads7142_reg_read(client, TI_ADS7142_ALT_HIGH_FLAGS,
> > + &high_flags);
> > + if (ret) {
> > + dev_err(indio_dev->dev.parent,
> > + "%s: ALT_HIGH_FLAGS reg read error(%i)",
> > + __func__, ret);
> > + goto final;
> > + }
> > +
> > + channel_collected = 0;
> > + ret = ti_ads7142_collect_channel_data(indio_dev, &channel_collected);
> > + if (ret)
> > + goto final;
> > +
> > + if (!channel_collected)
> > + goto final;
> > +
> > + for (i = 0; i < priv->channel_count; i++) {
> > + channel = &priv->channels[i];
> > + if (!(channel_collected & (1 << channel->channel)))
> > + continue;
>
> Perhaps use a for_each_bit_set(i, channel_collected) to simplify this.
>
I'll check it. Thank you.
> > +static int ti_ads7142_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long info)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = ti_ads7142_read_channel(indio_dev, chan->address, val);
> > + if (!ret)
>
> if (ret)
> return ret;
>
> return IIO_VAL_INT;
>
> Always have error cases out of line. That consistency makes
> it easier to review.
>
I dont like 'return' from 'case', but I can live with this. I'll change it.
> > +static int ti_ads7142_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val, int val2, long mask)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + priv->config.n_clk = val;
> > + if (priv->config.monitoring_mode)
> > + ret = ti_ads7142_do_work(indio_dev);
> return ti_...
>
> Early returns almost always easier to read.
> Note applies to lots of stuff above.
>
Ok, I'll fix it all
> > +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.
> > +static int ti_ads7142_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + int state)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + struct ti_ads7142_channel *channel;
> > + bool have_to_do = false;
> > + 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;
> > +
> > + mutex_lock(&priv->lock);
> > + if (dir == IIO_EV_DIR_RISING) {
> > + if (channel->config.alert_high != state) {
> > + channel->config.alert_high = state;
> > + have_to_do = true;
> > + }
> > + } else {
> > + if (channel->config.alert_low != state) {
> > + channel->config.alert_low = state;
> > + have_to_do = true;
> > + }
> > + }
> > + mutex_unlock(&priv->lock);
> > +
> > + if (have_to_do)
> > + ret = ti_ads7142_do_work(indio_dev);
> that's going to need a better name as I have no idea what _do_work implies.
>
> > +
> > + return ret;
> > +}
> > +
> > +static const struct iio_info ti_ads7142_iio_info = {
>
> If interrupt is going to be optional, pick between two versions of struct iio_info
> so that we don't provide callbacks for the case where we have no interrupts.
>
Ok, I'll do that.
> > + .read_raw = ti_ads7142_read_raw,
> > + .write_raw = ti_ads7142_write_raw,
> > + .read_event_value = ti_ads7142_read_event_value,
> > + .write_event_value = ti_ads7142_write_event_value,
> > + .read_event_config = ti_ads7142_read_event_config,
> > + .write_event_config = ti_ads7142_write_event_config,
> > +};
> > +
> > +static int ti_ads7142_parse_channel_config_of(struct device *dev,
> > + struct iio_dev *indio_dev)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + struct device_node *channel_node;
> > + struct iio_chan_spec *iio_channels;
> > + struct iio_chan_spec *iio_channel;
> > + struct ti_ads7142_channel *ads_channel;
> > + int channel_index = 0;
> > + int ret;
> > +
> > + priv->channel_count = of_get_available_child_count(dev->of_node);
> > + if (!priv->channel_count) {
> > + dev_err(dev, "dt: there is no channel definition");
> > + return -ENODEV;
> > + }
> > +
> > + priv->channels = devm_kcalloc(dev, priv->channel_count,
> > + sizeof(*priv->channels),
> > + GFP_KERNEL);
> > + if (!priv->channels)
> > + return -ENOMEM;
> > +
> > + indio_dev->num_channels = priv->channel_count;
>
> Why do you need that in two places?
>
You are right this is redundancy.
> > + iio_channels = devm_kcalloc(dev, priv->channel_count,
> > + sizeof(*iio_channels),
> > + GFP_KERNEL);
> > + if (!iio_channels)
> > + return -ENOMEM;
> > +
> > + indio_dev->channels = iio_channels;
> > +
> > + for_each_available_child_of_node(dev->of_node, channel_node) {
> > + ads_channel = &priv->channels[channel_index];
> > +
> > + ret = of_property_read_u32(channel_node, "reg",
> > + &ads_channel->channel);
> > + if (ret)
> > + goto err;
> > +
> > + iio_channel = &iio_channels[channel_index];
> > + iio_channel->type = IIO_VOLTAGE;
> > + iio_channel->indexed = 1;
> > + iio_channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> > + | BIT(IIO_CHAN_INFO_SAMP_FREQ);
> > + if (!IS_ERR(priv->vref))
> Ah. This somewhat explains the optional regulator.
> We have done this in a few old drivers because they initially were missing vref support
> and we couldn't break existing device tree bindings. I'm not keen to see it
> done for a new driver. Just make the vref-supply required.
> If it's a fixed voltage that is always on, then the device tree additions are
> trivial anyway.
>
Ok, I'll do that.
> > + iio_channel->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> > + iio_channel->scan_type.sign = 'u';
> > + iio_channel->scan_type.realbits = 12;
> > + iio_channel->scan_type.storagebits = 16;
> > + iio_channel->scan_type.shift = 0;
>
> No need to specify obvious default shift of 0. Rely on the zeroed allocation.
>
> > + iio_channel->scan_type.endianness = IIO_CPU;
> > + iio_channel->address = ads_channel->channel;
> > + iio_channel->scan_index = ads_channel->channel;
> > + iio_channel->channel = ads_channel->channel;
> > + if (priv->config.monitoring_mode) {
> > + iio_channel->event_spec = ti_ads7142_events;
> > + iio_channel->num_event_specs = ARRAY_SIZE(ti_ads7142_events);
> > + }
> > +
> > + ads_channel->config.high_threshold = TI_ADS7142_THRESHOLD_MSK;
> > + ret = of_property_read_u32(channel_node, "ti,threshold-rising",
>
> These are usually considered a matter of userspace policy only. Will need a strong
> argument for them to be in DT.
Ok, I'll remove this from dt.
>
> > + &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.
>
> > + 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);
> > +}
> > +
> > +static int ti_ads7142_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct ti_ads7142_priv *priv;
> > + int ret;
> > +
> > + ret = ti_ads7142_soft_reset(client);
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*priv));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + priv = iio_priv(indio_dev);
> > + i2c_set_clientdata(client, indio_dev);
> > +
> > + indio_dev->dev.parent = &client->dev;
> > + indio_dev->dev.of_node = client->dev.of_node;
>
> Both of the above are set by the iio core, so no need to set her.
Ok, thank you, I'll remove it.
>
> > + indio_dev->name = TI_ADS7142_NAME;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &ti_ads7142_iio_info;
> > +
> > + mutex_init(&priv->lock);
> > +
> > + priv->vref = devm_regulator_get_optional(&client->dev, "vref");
>
> There isn't a vref pin on the device. Vref is used in the datasheet
> but the pinout makes it clear it is actually avdd which is definitely
> not optional.
You are right, avdd is a better name.
>
> > + if (!IS_ERR(priv->vref)) {
> > + ret = regulator_enable(priv->vref);
> > + if (ret)
> > + goto err;
> As you have simple handling of power in here, I would definitely look
> to use managed calls to disable the regulators.
>
> devm_add_action_or_reset() is usual way of doing this.
>
> If nothing else it will ensure that the unwind on removal is the mirror
> image of the setup on probe() and hence it is much easier to avoid any
> subtle race conditions.
Very good suggestion, thank you.
>
> > + }
> > +
> > + priv->power = devm_regulator_get_optional(&client->dev, "power");
>
> dvdd? Not sure what else power could be. Note that you should only
> use _get_optional() for regulators that really optional and may not be
> connected. For cases where you simply can't control them and they aren't
> specified in the dts then devm_regulator_get() will supply a stub regulator
> that is always on.
>
Ok, I'll change it to dvdd and required property
>
> > + if (!IS_ERR(priv->power)) {
> > + ret = regulator_enable(priv->power);
> > + if (ret)
> > + goto err_regulator;
> > + }
> > +
> > + ret = ti_ads7142_parse_config_of(&client->dev, indio_dev);
> > + if (ret)
> > + goto err_regulator;
> > +
> > + if (!client->irq && priv->config.monitoring_mode) {
>
> Given you can check this before you have to do any unwinding, move it
> earlier.
Ok, I'll do this.
>
> > + ret = -EINVAL;
> > + dev_err(&client->dev, "Interrupt not specified\n");
> > + goto err_regulator;
> > + }
> > + if (client->irq && priv->config.monitoring_mode) {
> > + ret = devm_request_threaded_irq(&client->dev, client->irq,
> > + NULL, ti_ads7142_ist,
> > + IRQF_ONESHOT | IRQF_SHARED,
> > + dev_name(&client->dev),
> > + indio_dev);
> > + if (ret) {
> > + dev_err(&client->dev, "Unable to request IRQ %i",
> > + client->irq);
> > + goto err_regulator;
> > + }
> > + }
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret) {
> > + dev_err(&client->dev, "Failed to register iio device");
> > + goto err_regulator;
> > + }
> > +
> > + ret = ti_ads7142_do_work(indio_dev);
>
> As iio_device_register() is responsible for exposing userspace interfaces it
> is very rarely a good idea to do anything after it in probe()
> (some exceptions for runtime pm autosuspend setup as that can kick in later)
>
Ok, I'll move it earlier.
> > + if (!ret) {
> > + dev_info(&client->dev, "%s is a %s device at address 0x%X",
> > + dev_name(&indio_dev->dev), indio_dev->name,
> > + client->addr);
> > + return ret;
> Always have the error path out of line as it is much more consistent
> with what reviewers are expecting
>
> if (ret)
> goto error_unregister()
>
> return 0;
>
> > + }
> > +
> > + iio_device_unregister(indio_dev);
>
>
>
> > +
> > +err_regulator:
> > + if (!IS_ERR(priv->vref))
> > + regulator_disable(priv->vref);
> > + if (!IS_ERR(priv->power))
> > + regulator_disable(priv->power);
> > +err:
> > + mutex_destroy(&priv->lock);
> We rarely bother with mutex_destroy() in paths where the driver is being
> removed as it is only useful for debugging use of mutexes after they have
> been removed. That doesn't happen in these paths so it tends to just be
> noise to have a mutex_destroy(). This function is useful if you have a
> mutex inside something with a different lifespan from the device and
> like to enable mutex debugging.
>
Ok, I'll remove it, thank you for the explanation.
> > +
> > + return ret;
> > +}
> > +
> > +static int ti_ads7142_remove(struct i2c_client *client)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > +
> > + if (!IS_ERR(priv->vref))
> > + regulator_disable(priv->vref);
> If you'd not used _optional() above you could do these without worrying
> about whether the regulator was there or not.
>
> > + if (!IS_ERR(priv->power))
> > + regulator_disable(priv->power);
> > + mutex_destroy(&priv->lock);
> > + iio_device_unregister(indio_dev);
>
> _remove should always look like the unwinding of probe - so
> I should be able to match items in reverse order.
Ok, I check it.
Thank you for the review and suggestions.
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");
>
>