On Mon, 17 Jun 2024 20:39:05 +0200 Francesco Dolcini <francesco@xxxxxxxxxx> wrote: > From: João Paulo Gonçalves <joao.goncalves@xxxxxxxxxxx> > > The ADS1119 is a precision, 16-bit, analog-to-digital converter (ADC) > that features two differential or four single-ended inputs through a > flexible input multiplexer (MUX), rail-to-rail input > buffers, a programmable gain stage, a voltage reference, and an > oscillator. > > Apart from normal single conversion, the driver also supports > continuous conversion mode using a triggered buffer. However, in this > mode only one channel can be scanned at a time. > > Datasheet: https://www.ti.com/lit/gpn/ads1119 > Signed-off-by: João Paulo Gonçalves <joao.goncalves@xxxxxxxxxxx> > Signed-off-by: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx> Hi. My suggested changes are minor, so I just made them whilst applying. Please check the result! Applied with these tweaks a few more whitespace things checkpatch --strict pointed out to the togreg branch of iio.git and pushed out as testing for 0-day to see what we missed. Thanks, Jonathan > diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c > new file mode 100644 > index 000000000000..ccf259ebae08 > --- /dev/null > +++ b/drivers/iio/adc/ti-ads1119.c > @@ -0,0 +1,839 @@ > + > +#define ADS1119_V_CHAN(_chan, _chan2, _addr) { \ Why pass in the parameters as you use this in one place and they are all 0? Just put this definition inline where you use it as const struct iio_chan_spec ads1119_channel = (const struct iio_chan_spec) { .type = IIO_VOLTAGE, etc. (or something along these lines. }; > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .address = _addr, \ > + .channel = _chan, \ > + .channel2 = _chan2, \ > + .info_mask_separate = \ > + BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_OFFSET) | \ > + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .info_mask_shared_by_all_available = \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .scan_index = _addr, \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_CPU, \ > + }, \ > +} > + > +struct ads1119_channel_config { > + int gain; > + int datarate; > + int mux; > +}; > + > +struct ads1119_state { > + struct completion completion; > + struct i2c_client *client; > + struct gpio_desc *reset_gpio; > + struct iio_trigger *trig; > + struct ads1119_channel_config *channels_cfg; > + unsigned int num_channels_cfg; > + unsigned int cached_config; > + int vref_uV; > +}; > + > +static const char * const ads1119_power_supplies[] = { > + "avdd", "dvdd" > +}; > + > +static const int ads1119_available_datarates[] = { > + 20, 90, 330, 1000, > +}; > + > +static const int ads1119_available_gains[] = { > + 1, 1, > + 1, 4, > +}; > + > +static int ads1119_upd_cfg_reg(struct ads1119_state *st, unsigned int fields, > + unsigned int val) > +{ > + unsigned int config = st->cached_config; > + int ret; > + > + config &= ~fields; > + config |= val; > + > + ret = i2c_smbus_write_byte_data(st->client, ADS1119_CMD_WREG, config); > + if (ret) > + return ret; > + > + st->cached_config = config; > + > + return 0; > +} > + > +static bool ads1119_data_ready(struct ads1119_state *st) > +{ > + int status; > + > + status = i2c_smbus_read_byte_data(st->client, ADS1119_CMD_RREG_STATUS); > + if (status < 0) > + return false; > + > + return !!FIELD_GET(ADS1119_STATUS_DRDY_FIELD, status); The !! doesn't add anything as the cast to bool effectively does the same thing > +} > + > +static int ads1119_single_conversion(struct ads1119_state *st, > + struct iio_chan_spec const *chan, > + int *val, > + bool calib_offset) > +{ > + struct device *dev = &st->client->dev; > + int mux, gain, datarate; > + unsigned int sample; > + int ret; > + > + mux = st->channels_cfg[chan->address].mux; > + gain = st->channels_cfg[chan->address].gain; > + datarate = st->channels_cfg[chan->address].datarate; Slightly neater to do this at declaration fo variables. int mux = st->... etc. > + > + if (calib_offset) > + mux = ADS1119_MUX_SHORTED; > + > + ret = pm_runtime_resume_and_get(dev); > + if (ret) > + goto pdown; > + > + ret = ads1119_configure_channel(st, mux, gain, datarate); > + if (ret) > + goto pdown; > + > + ret = i2c_smbus_write_byte(st->client, ADS1119_CMD_START_SYNC); > + if (ret) > + goto pdown; > + > + ret = ads1119_read_data(st, chan, &sample); > + if (ret) > + goto pdown; > + > + *val = sign_extend32(sample, chan->scan_type.realbits - 1); > + ret = IIO_VAL_INT; > +pdown: > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > + return ret; > +} > +static int ads1119_poll_data_ready(struct ads1119_state *st, > + struct iio_chan_spec const *chan) > +{ > + unsigned int datarate = st->channels_cfg[chan->address].datarate; > + unsigned long wait_time; > + bool data_ready; > + > + /* Poll 5 times more than the data rate */ > + wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate); > + > + return read_poll_timeout(ads1119_data_ready, data_ready, > + data_ready == true, wait_time, checkpatch --strict pointed out that you might as well just use dataready as the conditional, rather than dataready == true. > + ADS1119_MAX_DRDY_TIMEOUT, false, st); > +} > + > +static int ads1119_alloc_and_config_channels(struct iio_dev *indio_dev) > +{ > + const struct iio_chan_spec ads1119_channel = ADS1119_V_CHAN(0, 0, 0); > + const struct iio_chan_spec ads1119_ts = IIO_CHAN_SOFT_TIMESTAMP(0); > + struct ads1119_state *st = iio_priv(indio_dev); > + struct iio_chan_spec *iio_channels, *chan; > + struct device *dev = &st->client->dev; > + unsigned int num_channels, i; > + bool differential; > + u32 ain[2]; > + int ret; > + > + st->num_channels_cfg = device_get_child_node_count(dev); > + if (st->num_channels_cfg > ADS1119_MAX_CHANNELS) > + return dev_err_probe(dev, -EINVAL, > + "Too many channels %d, max is %d\n", > + st->num_channels_cfg, > + ADS1119_MAX_CHANNELS); > + > + st->channels_cfg = devm_kcalloc(dev, st->num_channels_cfg, > + sizeof(*st->channels_cfg), GFP_KERNEL); > + if (!st->channels_cfg) > + return -ENOMEM; > + > + /* Allocate one more iio channel for the timestamp */ > + num_channels = st->num_channels_cfg + 1; > + iio_channels = devm_kcalloc(dev, num_channels, > + sizeof(*iio_channels), > + GFP_KERNEL); Can wrap that on one fewer line. > + if (!iio_channels) > + return -ENOMEM; > + > + i = 0; > + > > > +static int ads1119_probe(struct i2c_client *client) > +{ ... > + if (client->irq > 0) { > + ret = devm_request_threaded_irq(dev, > + client->irq, Can combine some of these lines under 80 chars. > + ads1119_irq_handler, > + NULL, > + IRQF_TRIGGER_FALLING, > + "ads1119", > + indio_dev); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to allocate irq\n"); > + > + st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", > + indio_dev->name, > + iio_device_id(indio_dev)); > + if (!st->trig) > + return dev_err_probe(dev, -ENOMEM, > + "Failed to allocate IIO trigger\n"); > + > + st->trig->ops = &ads1119_trigger_ops; > + iio_trigger_set_drvdata(st->trig, indio_dev); > + > + ret = devm_iio_trigger_register(dev, st->trig); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to register IIO trigger\n"); > + } > + > + ret = ads1119_init(st, vref_external); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to initialize device\n"); > + > + pm_runtime_set_autosuspend_delay(dev, ADS1119_SUSPEND_DELAY); > + pm_runtime_use_autosuspend(dev); > + pm_runtime_mark_last_busy(dev); > + pm_runtime_set_active(dev); > + > + ret = devm_pm_runtime_enable(dev); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to enable pm runtime\n"); > + > + ret = devm_add_action_or_reset(dev, ads1119_powerdown, st); Hmm. This naturally (I think anyway) pairs with the reset in ads1119_init() so it's a little odd to find it down here. I assume the reasoning was that the runtime PM might result in it being on? However I think the worst that can happen is that runtime pm powers down the powered down device just before it is stopped by the devm_pm_runtime enable cleanup. So this ordering doesn't matter. Hence I'll leave it alone, but it did make me think for a few minutes. > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to add powerdown action\n"); > + > + return devm_iio_device_register(dev, indio_dev); > +} > +/* > + * The ADS1119 does not require a resume function because it automatically > + * powers on after a reset. > + * After a power down command, the ADS1119 can still communicate but turns off > + * its analog parts. To resume from power down, the device will power up again > + * upon receiving a start/sync command. > + */ > +static DEFINE_RUNTIME_DEV_PM_OPS(ads1119_pm_ops, > + ads1119_runtime_suspend, > + NULL, > + NULL); Trivial but can rewrap those lines to save 2 lines without significant loss of readability.
