| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+U=Dso9sWGZnu1re9Mf4GqrhO6sJk+aTfsA0qeEyyM0kTwMiA@mail.gmail.com>
Date: Mon, 3 Jan 2022 12:44:22 +0200
From: Alexandru Ardelean <ardeleanalex@...il.com>
To: Iain Hunter <drhunter95@...il.com>
Cc: iain@...terembedded.co.uk, Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Alexandru Ardelean <aardelean@...iqon.com>,
Randy Dunlap <rdunlap@...radead.org>,
Andy Shevchenko <andy.shevchenko@...il.com>,
Cai Huoqing <caihuoqing@...du.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
Tomislav Denis <tomislav.denis@....com>,
Oleksij Rempel <linux@...pel-privat.de>,
Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
Fabio Estevam <festevam@...il.com>,
Biju Das <biju.das.jz@...renesas.com>,
LKML <linux-kernel@...r.kernel.org>,
linux-iio <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] add TI ads1018 driver to iio
On Fri, Dec 31, 2021 at 6:23 PM Iain Hunter <drhunter95@...il.com> wrote:
>
Hey,
And Happy New Year :)
This commit requires a description.
Maybe run the checkpatch.pl script for some stylistics.
And a few other comments inline :)
> Signed-off-by: Iain Hunter <drhunter95@...il.com>
> ---
> drivers/iio/adc/Kconfig | 12 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-ads1018.c | 840 +++++++++++++++++++++++++++++++++++
> 3 files changed, 853 insertions(+)
> create mode 100644 drivers/iio/adc/ti-ads1018.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 8bf5b62a73f4..129194755c03 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1131,6 +1131,18 @@ config TI_ADS1015
> This driver can also be built as a module. If so, the module will be
> called ti-ads1015.
>
> +config TI_ADS1018
> + tristate "Texas Instruments ADS1018 ADC"
> + depends on SPI
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + If you say yes here you get support for Texas Instruments ADS1018
> + ADC chip.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ti-ads1018.
> +
> config TI_ADS7950
> tristate "Texas Instruments ADS7950 ADC driver"
> depends on SPI && GPIOLIB
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d3f53549720c..da462538ec17 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_TI_ADC108S102) += ti-adc108s102.o
> obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
> obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
> +obj-$(CONFIG_TI_ADS1018) += ti-ads1018.o
> obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
> obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o
> obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
> diff --git a/drivers/iio/adc/ti-ads1018.c b/drivers/iio/adc/ti-ads1018.c
> new file mode 100644
> index 000000000000..b22cd0a4d0bb
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1018.c
> @@ -0,0 +1,840 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ADS1018 - Texas Instruments Analog-to-Digital Converter
> + *
> + * Copyright 2021 Iain Hunter <iain@...terembedded.co.uk>
> + *
> + * Based on ti-ads1015.c
> + * Copyright (c) 2016, Intel Corporation.
> + *
> + * IIO driver for 12 bit SPI ADC ADS1018 from TI
> + *
> + */
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +#include <linux/bitfield.h>
> +#include <asm/unaligned.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#define ADS1018_DRV_NAME "ads1018"
> +
> +#define ADS1018_CHANNELS 8
> +
> +#define ADS1018_CFG_NOP_MASK GENMASK(2, 1)
> +#define ADS1018_CFG_PULL_UP_MASK BIT(3)
> +#define ADS1018_CFG_TS_MODE_MASK BIT(4)
> +#define ADS1018_CFG_DR_MASK GENMASK(7, 5)
> +#define ADS1018_CFG_MODE_MASK BIT(8)
> +#define ADS1018_CFG_PGA_MASK GENMASK(11, 9)
> +#define ADS1018_CFG_MUX_MASK GENMASK(14, 12)
> +
> +#define ADS1018_PGA_2_048V 1
> +
> +#define ADS1018_MODE_SINGLE_SHOT 1
> +#define ADS1018_MODE_CONTINUOUS 0
> +
> +#define ADS1018_STATE_RATE_128 0
> +#define ADS1018_STATE_RATE_250 1
> +#define ADS1018_STATE_RATE_490 2
> +#define ADS1018_STATE_RATE_920 3
> +#define ADS1018_STATE_RATE_1600 4
> +#define ADS1018_STATE_RATE_2400 5
> +#define ADS1018_STATE_RATE_3300 6
> +
> +#define ADS1018_TS_MODE_ADC 0
> +#define ADS1018_TS_MODE_TEMP 1
> +
> +#define ADS1018_PULLUP_DOUT_DISABLE 0
> +#define ADS1018_PULLUP_DOUT_ENABLE 1
> +
> +#define ADS1018_NOP_UPDATE_CONFIG 1
> +#define ADS1018_RESERVED 1
> +
> +#define ADS1018_CONFIGURE \
> + (FIELD_PREP(ADS1018_CFG_PGA_MASK, ADS1018_PGA_2_048V) | \
> + FIELD_PREP(ADS1018_CFG_MODE_MASK, ADS1018_MODE_CONTINUOUS) | \
> + FIELD_PREP(ADS1018_CFG_DR_MASK, ADS1018_STATE_RATE_2400) | \
> + FIELD_PREP(ADS1018_CFG_TS_MODE_MASK, ADS1018_TS_MODE_ADC) | \
> + FIELD_PREP(ADS1018_CFG_PULL_UP_MASK, ADS1018_PULLUP_DOUT_ENABLE) | \
> + FIELD_PREP(ADS1018_CFG_NOP_MASK, ADS1018_NOP_UPDATE_CONFIG)| \
> + ADS1018_RESERVED)
> +
> +#define ADS1018_SLEEP_DELAY_MS 2000
> +#define ADS1018_DEFAULT_PGA 2
> +#define ADS1018_DEFAULT_DATA_RATE 4
> +#define ADS1018_DEFAULT_CHAN 0
> +
> +enum chip_ids {
> + ADS1018};
//stylistic
enum closing bracket on separate line;
> +
> +enum ads1018_channels {
> + ADS1018_AIN0_AIN1 = 0,
> + ADS1018_AIN0_AIN3,
> + ADS1018_AIN1_AIN3,
> + ADS1018_AIN2_AIN3,
> + ADS1018_AIN0,
> + ADS1018_AIN1,
> + ADS1018_AIN2,
> + ADS1018_AIN3,
> + ADS1018_TIMESTAMP,
> +};
> +
> +struct ti_ads1018_chip_info {
> + const struct iio_chan_spec *channels;
> + unsigned int num_channels;
> +};
> +
> +#define NUM_DIFF_CHAN 2
> +unsigned int ti_ads1018_diff_channels[NUM_DIFF_CHAN];
Using this global here will mean that only 1 device can use
diff-channels in the entire system.
So, if you have more than device in a system, the configuration of one
device will get used by both/all devices probed here.
This should be moved on the ti_ads1018_state struct.
That way, each device can live with it's own separate configuration.
> +
> +static const unsigned int ads1018_data_rate[] = {
> + 128, 250, 490, 920, 1600, 2400, 3300
> +};
> +
> +/*
> + * Translation from PGA bits to full-scale positive and negative input voltage
> + * range in mV
> + */
> +static int ads1018_fullscale_range[] = {
> + 6144, 4096, 2048, 1024, 512, 256
> +};
> +
> +#define ADS1018_V_CHAN(_chan, _addr) { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .address = _addr, \
> + .channel = _chan, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .info_mask_separate_available = \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .scan_index = _addr, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 12, \
> + .storagebits = 16, \
> + .shift = 4, \
> + .endianness = IIO_CPU, \
> + }, \
> + .datasheet_name = "AIN"#_chan, \
> +}
> +
> +#define ADS1018_V_DIFF_CHAN(_chan, _chan2, _addr) { \
> + .type = IIO_VOLTAGE, \
> + .differential = 1, \
> + .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_SAMP_FREQ), \
> + .info_mask_separate_available = \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .scan_index = _addr, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 12, \
> + .storagebits = 16, \
> + .shift = 4, \
> + .endianness = IIO_CPU, \
> + }, \
> + .datasheet_name = "AIN"#_chan"-AIN"#_chan2, \
> +}
> +
> +struct ads1018_channel_data {
> + bool enabled;
> + unsigned int pga;
> + unsigned int data_rate;
> +};
> +
> +struct ti_ads1018_state {
> + struct spi_device *spi;
> + struct spi_transfer scan_single_xfer[1];
> + struct spi_message scan_single_msg;
> +
> + struct iio_trigger *trig;
> + /*
> + * Protects ADC ops, e.g: concurrent sysfs/buffered
> + * data reads, configuration updates
> + */
> + struct mutex lock;
> + struct ads1018_channel_data channel_data[ADS1018_CHANNELS];
> + unsigned int current_config;
> + unsigned int settings;
> + const unsigned int *data_rate;
> + /*
> + * Set to true when the ADC is switched to the continuous-conversion
> + * mode and exits from a power-down state. This flag is used to avoid
> + * getting the stale result from the conversion register.
> + */
> + bool conv_invalid;
> + /*
> + * DMA (thus cache coherency maintenance) requires the
> + * transfer buffers to live in their own cache lines.
> + *
> + */
> + unsigned char spi_rx_buf[sizeof(u32)]
> + ____cacheline_aligned;
> + unsigned char spi_tx_buf[sizeof(u32)]
> + ____cacheline_aligned;
> +};
> +
> +static const struct iio_chan_spec ti_ads1018_channels[] = {
> + ADS1018_V_DIFF_CHAN(0, 1, ADS1018_AIN0_AIN1),
> + ADS1018_V_DIFF_CHAN(0, 3, ADS1018_AIN0_AIN3),
> + ADS1018_V_DIFF_CHAN(1, 3, ADS1018_AIN1_AIN3),
> + ADS1018_V_DIFF_CHAN(2, 3, ADS1018_AIN2_AIN3),
> + ADS1018_V_CHAN(0, ADS1018_AIN0),
> + ADS1018_V_CHAN(1, ADS1018_AIN1),
> + ADS1018_V_CHAN(2, ADS1018_AIN2),
> + ADS1018_V_CHAN(3, ADS1018_AIN3),
> + IIO_CHAN_SOFT_TIMESTAMP(ADS1018_TIMESTAMP),
> +};
> +
> +static const struct ti_ads1018_chip_info ti_ads1018_chip = {
> + .channels = ti_ads1018_channels,
> + .num_channels = ARRAY_SIZE(ti_ads1018_channels),
> +};
> +
> +static int ti_ads1018_write_config_register(struct ti_ads1018_state *st, int value)
> +{
> + int ret;
> +
> + /* Write new value to the first 2 bytes of the 4 byte SPI transfer */
> + put_unaligned_be16(value, (void *)&st->spi_tx_buf[0]);
> + put_unaligned_be16(0x0000, (void *)&st->spi_tx_buf[2]);
> +
> + ret = spi_sync(st->spi, &st->scan_single_msg);
> + if (ret)
> + return ret;
> +
> + /* Update the state current config based on result of this write
> + * the updated config register value is in second 16 bit word
> + * which is third and fourth byte
> + */
> + st->current_config = get_unaligned_be16((void *)&st->spi_rx_buf[2]);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused
> +ads1018_set_power_state(struct ti_ads1018_state *st, bool on)
> +{
> + int ret;
> + struct spi_device *spi = st->spi;
> + struct device *dev = &(spi->dev);
> +
> + if (pm_runtime_enabled(dev)) {
> + if (on) {
> + ret = pm_runtime_resume_and_get(dev);
> + } else {
> + pm_runtime_mark_last_busy(dev);
> + ret = pm_runtime_put_autosuspend(dev);
> + }
> +
> + return ret < 0 ? ret : 0;
> + } else {
> + return 0;
> + }
this if() block has too many levels of indentation;
to simplify, you can do early exits:
if (!pm_runtime_enabled(dev))
return 0;
if (on)
return pm_runtime_resume_and_get(dev);
pm_runtime_mark_last_busy(dev);
return pm_runtime_put_autosuspend(dev);
> +}
> +
> +static
> +int ads1018_get_adc_result(struct ti_ads1018_state *st, int chan, int *val)
> +{
> + int ret, pga, dr, dr_old, conv_time;
> + unsigned int old, mask, cfg;
> + unsigned int cmd;
> +
> + if (chan < 0 || chan >= ADS1018_CHANNELS)
> + return -EINVAL;
> +
> + old = st->current_config;
> + pga = st->channel_data[chan].pga;
> + dr = st->channel_data[chan].data_rate;
> + mask = ADS1018_CFG_MUX_MASK | ADS1018_CFG_PGA_MASK |
> + ADS1018_CFG_DR_MASK;
> + cfg = FIELD_PREP(ADS1018_CFG_MUX_MASK, chan) |
> + FIELD_PREP(ADS1018_CFG_PGA_MASK, pga) |
> + FIELD_PREP(ADS1018_CFG_DR_MASK, dr);
> +
> + cfg = (old & ~mask) | (cfg & mask);
> + if (old != cfg) {
> + /* Update the configuration to the new one */
> + ret = ti_ads1018_write_config_register(st, cfg);
> + if (ret)
> + return ret;
> + st->conv_invalid = true;
> + }
> +
> + if (st->conv_invalid) {
> + dr_old = FIELD_GET(ADS1018_CFG_DR_MASK, old);
> +
> + conv_time = DIV_ROUND_UP(USEC_PER_SEC, st->data_rate[dr_old]);
> + conv_time += DIV_ROUND_UP(USEC_PER_SEC, st->data_rate[dr]);
> + conv_time += conv_time / 10; /* 10% internal clock inaccuracy */
> + usleep_range(conv_time, conv_time + 1);
> + st->conv_invalid = false;
> + }
> +
> + /* For just a data read cmd is 0x0000 */
> + cmd = 0x0000;
> + put_unaligned_be16(cmd, (void *)&st->spi_tx_buf[0]);
> + put_unaligned_be16(0x0000, (void *)&st->spi_tx_buf[2]);
> +
> + ret = spi_sync(st->spi, &st->scan_single_msg);
> + if (ret)
> + return ret;
> +
> + *val = get_unaligned_be16((void *)&st->spi_rx_buf[0]);
> + return 0;
> +}
> +
> +static irqreturn_t ti_ads1018_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct ti_ads1018_state *st = iio_priv(indio_dev);
> + /* Ensure natural alignment of timestamp */
> + struct {
> + s16 chan;
> + s64 timestamp __aligned(8);
> +
> + } scan;
> +
> + int chan, ret, res;
> +
> + memset(&scan, 0, sizeof(scan));
> +
> + mutex_lock(&st->lock);
> + chan = find_first_bit(indio_dev->active_scan_mask,
> + indio_dev->masklength);
> + ret = ads1018_get_adc_result(st, chan, &res);
> + if (ret < 0) {
> + mutex_unlock(&st->lock);
> + goto err;
> + }
> +
> + scan.chan = res;
> + mutex_unlock(&st->lock);
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &scan,
> + iio_get_time_ns(indio_dev));
> +
> +err:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int ads1018_set_scale(struct ti_ads1018_state *st,
> + struct iio_chan_spec const *chan,
> + int scale, int uscale)
> +{
> + int fullscale = div_s64((scale * 1000000LL + uscale) <<
> + (chan->scan_type.realbits - 1), 1000000);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ads1018_fullscale_range); i++) {
> + if (ads1018_fullscale_range[i] == fullscale) {
> + st->channel_data[chan->address].pga = i;
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ads1018_set_data_rate(struct ti_ads1018_state *st, int chan, int rate)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ads1018_data_rate); i++) {
This looks quirky.
It assumes that the number of elements in st->data_rate[i] is the same
as in ads1018_data_rate.
Which raises a few questions: why should we assign "
st->channel_data=ads1018_data_rate" if it's the same/constant array?
will there be another device added?
Another idea is to add a 0 / -1 terminator value to the array.
Something like: for (i = 0;st->data_rate[i] /* != -1 */ ; i++) {
> + if (st->data_rate[i] == rate) {
> + st->channel_data[chan].data_rate = i;
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ads1018_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct ti_ads1018_state *st = iio_priv(indio_dev);
> + int ret, idx;
> +
> + mutex_lock(&st->lock);
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW: {
> + int shift = chan->scan_type.shift;
> + int sign_bit = chan->scan_type.realbits - 1;
> +
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + break;
> +
> + ret = ads1018_set_power_state(st, true);
> + if (ret < 0)
> + goto release_direct;
> +
> + ret = ads1018_get_adc_result(st, chan->address, val);
> + if (ret < 0) {
> + ads1018_set_power_state(st, false);
> + goto release_direct;
> + }
> +
> + *val = sign_extend32(*val >> shift, sign_bit);
> +
> + ret = ads1018_set_power_state(st, false);
> + if (ret < 0)
> + goto release_direct;
> +
> + ret = IIO_VAL_INT;
> +release_direct:
> + iio_device_release_direct_mode(indio_dev);
> + break;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + idx = st->channel_data[chan->address].pga;
> + *val = ads1018_fullscale_range[idx];
> + *val2 = chan->scan_type.realbits - 1;
> + ret = IIO_VAL_FRACTIONAL_LOG2;
> + break;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + idx = st->channel_data[chan->address].data_rate;
> + *val = st->data_rate[idx];
> + ret = IIO_VAL_INT;
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> +static int ads1018_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long mask)
> +{
> + struct ti_ads1018_state *st = iio_priv(indio_dev);
> + int idx;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + idx = st->channel_data[chan->address].pga;
> + *length = sizeof(int) / sizeof(int);
> + *vals = &(ads1018_fullscale_range[idx]);
> + *type = IIO_VAL_INT;
> + return IIO_AVAIL_LIST;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + idx = st->channel_data[chan->address].data_rate;
> + *length = sizeof(int) / sizeof(int);
> + *vals = &(st->data_rate[idx]);
> + *type = IIO_VAL_INT;
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ads1018_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct ti_ads1018_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + ret = ads1018_set_scale(st, chan, val, val2);
> + break;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = ads1018_set_data_rate(st, chan->address, val);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> +static int ads1018_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + return ads1018_set_power_state(iio_priv(indio_dev), true);
> +}
> +
> +static int ads1018_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + return ads1018_set_power_state(iio_priv(indio_dev), false);
> +}
> +
> +static const struct iio_buffer_setup_ops ti_ads1018_buffer_setup_ops = {
> + .preenable = ads1018_buffer_preenable,
> + .postdisable = ads1018_buffer_postdisable,
> + .validate_scan_mask = &iio_validate_scan_mask_onehot,
> +};
> +
> +static IIO_CONST_ATTR_NAMED(ads1018_scale_available, scale_available,
> + "3 2 1 0.5 0.25 0.125");
> +
> +static IIO_CONST_ATTR_NAMED(ads1018_sampling_frequency_available,
> + sampling_frequency_available, "128 250 490 920 1600 2400 3300");
> +
> +static struct attribute *ads1018_attributes[] = {
> + &iio_const_attr_ads1018_scale_available.dev_attr.attr,
> + &iio_const_attr_ads1018_sampling_frequency_available.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group ads1018_attribute_group = {
> + .attrs = ads1018_attributes,
> +};
> +
> +
> +static const struct iio_info ti_ads1018_info = {
> + .read_raw = ads1018_read_raw,
> + .read_avail = ads1018_read_avail,
> + .write_raw = ads1018_write_raw,
> + .attrs = &ads1018_attribute_group,
> +};
> +
> +static int ads1018_validate_diff_channels(
> + struct device *dev, struct fwnode_handle *node,
> + unsigned int *diff_channels, unsigned int *channel)
> +{
> + switch (diff_channels[0]) {
> +
> + case 0:{
stylistically, the opening bracket of the case-block goes on the next line
though, checkpatch may complain
> + if (diff_channels[1] == 1)
> + *channel = ADS1018_AIN0_AIN1;
> + else if (diff_channels[1] == 3)
> + *channel = ADS1018_AIN0_AIN3;
> + else {
> + dev_err(dev, "invalid diff channel <%u %u> on %pfw\n",
> + diff_channels[0], diff_channels[1],
> + node);
> + fwnode_handle_put(node);
all these fwnode_handle_put() calls should be moved to the caller;
and maybe use the error condition to call them;
doing this here looks a bit too decoupled/spread across the file;
> + return -EINVAL;
> + }
> + break;
> + }
> + case 1:{
> + if (diff_channels[1] == 3)
> + *channel = ADS1018_AIN1_AIN3;
> + else {
> + dev_err(dev, "invalid diff channel <%u %u> on %pfw\n",
> + diff_channels[0], diff_channels[1],
> + node);
> + fwnode_handle_put(node);
> + return -EINVAL;
these if() else () blocks also can be reduced in indentation
something like:
if (diff_channels[1] == 3) {
*channel = ADS1018_AIN1_AIN3;;
return 0;
}
dev_err(dev, "invalid diff channel <%u %u> on %pfw\n",
diff_channels[0], diff_channels[1],
node);
return -EINVAL;
> + }
> + break;
> + }
> + case 2:{
> + if (diff_channels[1] == 3)
> + *channel = ADS1018_AIN2_AIN3;
> + else {
> + dev_err(dev, "invalid diff channel <%u %u> on %pfw\n",
> + diff_channels[0], diff_channels[1],
> + node);
> + fwnode_handle_put(node);
> + return -EINVAL;
> + }
> + break;
> + }
> + }
> + return 0;
> +}
> +
> +
nitpick: remove extra line here
> +static int ads1018_device_get_channels_config(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> + struct ti_ads1018_state *data = iio_priv(indio_dev);
> + struct device *dev = &spi->dev;
> + struct fwnode_handle *node;
> + int i = -1;
while not init this to -EINVAL directly?
> +
> + if (!spi->dev.of_node ||
> + !of_get_next_child(spi->dev.of_node, NULL))
> + return -EINVAL;
> +
> + device_for_each_child_node(dev, node) {
> + u32 pval;
> + unsigned int channel;
> + size_t number_diff_channels;
> +
> + if (!fwnode_property_read_u32(node, "ti,adc-channels", &pval)) {
> + channel = pval+ADS1018_AIN0;
> + if (channel > ADS1018_AIN3) {
> + dev_err(dev,
> + "invalid single ended channel %u on %pfw\n",
> + channel, node);
> + fwnode_handle_put(node);
> + return -EINVAL;
> + }
> + }
> +
> + number_diff_channels = fwnode_property_read_u32_array(node,
> + "ti,adc-diff-channels", NULL,
> + NUM_DIFF_CHAN);
> +
> + if ((number_diff_channels == NUM_DIFF_CHAN) &&
> + (!fwnode_property_read_u32_array(node,
> + "ti,adc-diff-channels",
> + ti_ads1018_diff_channels,
> + NUM_DIFF_CHAN))) {
> +
> + if (!ads1018_validate_diff_channels(dev, node,
> + ti_ads1018_diff_channels,
> + &channel))
> + return -EINVAL;
> + }
> +
> + data->channel_data[channel].enabled = 1;
> + data->channel_data[channel].pga = ADS1018_DEFAULT_PGA;
> + data->channel_data[channel].data_rate = ADS1018_DEFAULT_DATA_RATE;
> +
> + i++;
> + }
> +
> + return i < 0 ? -EINVAL : 0;
> +}
> +
> +static void ads1018_get_channels_config(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> + struct ti_ads1018_state *data = iio_priv(indio_dev);
> + unsigned int k;
> +
> + if (!ads1018_device_get_channels_config(spi))
> + return;
this looks a bit quirky;
the ads1018_device_get_channels_config() returns -EINVAL or 0
maybe i missed it: but could it return something else than these 2 values?
if yes, i would return this to the probe() function and fail the probe
also, the fallback here is silent in some cases;
maybe add a log-message about it?
otherwise people would be puzzled as to what went wrong in their DT
though, to be honest, I would fail the entire probe on all -EINVAL
errors and [probably] allow a default configuration *only* if there is
no OF-node/fwnode
i'm not sure that drivers need to fallback on bad DT configuration,
and doing so may sometimes be worse than just failing the probe;
but that's another debathe;
> + /* fallback on default configuration */
> + for (k = 0; k < ADS1018_CHANNELS; ++k) {
> + data->channel_data[k].enabled = 0;
> + data->channel_data[k].pga = ADS1018_DEFAULT_PGA;
> + data->channel_data[k].data_rate = ADS1018_DEFAULT_DATA_RATE;
> + }
> +}
> +
> +static int ads1018_set_conv_mode(struct ti_ads1018_state *st, int mode)
> +{
> + int updatedConfig;
> +
> + updatedConfig = st->current_config & ~(ADS1018_CFG_MODE_MASK);
> + updatedConfig |= FIELD_PREP(ADS1018_CFG_MODE_MASK, mode);
> +
> + return ti_ads1018_write_config_register(st, updatedConfig);
> +}
> +
> +static int ti_ads1018_set_trigger_state(struct iio_trigger *trig, bool enable)
> +{
> + return 0;
> +}
> +
> +static const struct iio_trigger_ops ti_ads1018_trigger_ops = {
> + .set_trigger_state = ti_ads1018_set_trigger_state,
> + .validate_device = iio_trigger_validate_own_device,
> +};
> +
> +static void ads1018_disable(void *data)
> +{
> + struct ti_ads1018_state *st = (struct ti_ads1018_state *) data;
> + struct spi_device *spi = st->spi;
> +
> + pm_runtime_disable(&spi->dev);
> + pm_runtime_set_suspended(&spi->dev);
> + pm_runtime_put_noidle(&spi->dev);
> +
> + /* power down single shot mode */
> + ads1018_set_conv_mode(st, ADS1018_MODE_SINGLE_SHOT);
> +}
> +
> +static int ti_ads1018_probe(struct spi_device *spi)
> +{
> + const struct ti_ads1018_chip_info *info;
> + struct ti_ads1018_state *st;
> + struct iio_dev *indio_dev;
> + int ret;
> + int i;
> + int config;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> +
> + st->spi = spi;
> + config = ADS1018_CONFIGURE;
> + spi_set_drvdata(spi, indio_dev);
> +
> + mutex_init(&st->lock);
> +
> + info = &ti_ads1018_chip;
> + indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = info->channels;
> + indio_dev->num_channels = info->num_channels;
> + indio_dev->info = &ti_ads1018_info;
> + st->data_rate = &ads1018_data_rate[0];
> +
> + ret = devm_add_action_or_reset(&spi->dev, ads1018_disable,
> + st);
> +
> + if (ret)
> + return ret;
> +
> + /* allocate and then register trigger with IIO core */
> + st->trig = devm_iio_trigger_alloc(indio_dev->dev.parent, "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (st->trig == NULL)
> + return -ENOMEM;
> +
> + st->trig->ops = &ti_ads1018_trigger_ops;
> +
> + devm_iio_trigger_register(indio_dev->dev.parent, st->trig);
This requires an error check.
ret = devm_iio_trigger_register(indio_dev->dev.parent, st->trig);
if (ret)
return ret;
> +
> + /* set the default trigger */
> + indio_dev->trig = iio_trigger_get(st->trig);
> +
> + ads1018_get_channels_config(spi);
> +
> + /* Find out if DT has enabled any of the channels, only one allowed so
> + * just take the first one
> + */
> + for (i = 0; i < info->num_channels; i++)
> + if (st->channel_data[i].enabled == 1)
> + break;
> +
> + /* If a channel was enabled set it by default in the device */
> + if (i < info->num_channels) {
> + /* mask out the mux bits */
> + config &= ~(ADS1018_CFG_MUX_MASK);
> + config |= FIELD_PREP(ADS1018_CFG_MUX_MASK, i);
> + }
> +
> + /*
> + * Always use 4 * 8 bit transfer mode so that the status is read back
> + * in the second 16 bit word. This allows us to track when updated
> + * configuration needs to be applied.
> + */
> + st->scan_single_xfer[0].tx_buf = &st->spi_tx_buf[0];
> + st->scan_single_xfer[0].rx_buf = &st->spi_rx_buf[0];
> + st->scan_single_xfer[0].len = 4;
> + st->scan_single_xfer[0].cs_change = 0;
> + st->scan_single_xfer[0].bits_per_word = 8;
> +
> + spi_message_init_with_transfers(&st->scan_single_msg,
> + st->scan_single_xfer, 1);
> +
> + ret = ti_ads1018_write_config_register(st, config);
> + if (ret < 0) {
> + dev_err(&spi->dev, "ads1018 configuration failed\n");
> + return ret;
> + }
> +
> + ret = devm_iio_triggered_buffer_setup(indio_dev->dev.parent,
> + indio_dev, NULL,
> + ti_ads1018_trigger_handler,
> + &ti_ads1018_buffer_setup_ops);
> + if (ret < 0) {
> + dev_err(&spi->dev, "iio triggered buffer setup failed\n");
> + return ret;
> + }
> +
> + ret = ads1018_set_conv_mode(st, ADS1018_MODE_CONTINUOUS);
> + if (ret)
> + return ret;
> +
> + st->conv_invalid = true;
> +
> + ret = pm_runtime_set_active(&spi->dev);
> + if (ret)
> + return ret;
// stylistic
add an empty line here
> + pm_runtime_set_autosuspend_delay(&spi->dev, ADS1018_SLEEP_DELAY_MS);
> + pm_runtime_use_autosuspend(&spi->dev);
> + pm_runtime_enable(&spi->dev);
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +#ifdef CONFIG_PM
There's a recent trend/switch to not use this define.
See:
https://patchwork.kernel.org/project/linux-iio/list/?series=602256
> +static int ads1018_runtime_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct ti_ads1018_state *st = iio_priv(indio_dev);
> +
> + return ads1018_set_conv_mode(st, ADS1018_MODE_SINGLE_SHOT);
> +}
> +
> +static int ads1018_runtime_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct ti_ads1018_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = ads1018_set_conv_mode(st, ADS1018_MODE_CONTINUOUS);
> + if (!ret)
> + st->conv_invalid = true;
> +
> + return ret;
> +}
> +#endif
> +
> +static const struct dev_pm_ops ads1018_pm_ops = {
> + SET_RUNTIME_PM_OPS(ads1018_runtime_suspend,
> + ads1018_runtime_resume, NULL)
> +};
> +
> +static const struct spi_device_id ti_ads1018_id[] = {
> + { "ads1018", ADS1018 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, ti_ads1018_id);
> +
> +static const struct of_device_id ads1018_of_table[] = {
> + { .compatible = "ti,ads1018", .data = &ti_ads1018_chip },
> + { },
// mostly stylistic
Comma can be removed: it's a null terminator.
> +};
> +MODULE_DEVICE_TABLE(of, ads1018_of_table);
> +
> +static struct spi_driver ti_ads1018_driver = {
> + .driver = {
> + .name = ADS1018_DRV_NAME,
maybe just use the value directly here; the ADS1018_DRV_NAME macros
isn't used anywhere else;
> + .of_match_table = ads1018_of_table,
> + .pm = &ads1018_pm_ops,
> + },
> + .probe = ti_ads1018_probe,
> + .id_table = ti_ads1018_id,
> +};
> +module_spi_driver(ti_ads1018_driver);
> +
> +MODULE_AUTHOR("Iain Hunter <iain@...terembedded.co.uk>");
> +MODULE_DESCRIPTION("TI TI_ADS1018 ADC");
> +MODULE_LICENSE("GPL v2");
> --
> 2.25.1
>
Powered by blists - more mailing lists