Re: [PATCH v3 1/2] iio: adc: Add driver for Texas Instruments ADS131E0x ADC family

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jan 12, 2021 at 2:37 PM <tomislav.denis@xxxxxxx> wrote:
>
> From: Tomislav Denis <tomislav.denis@xxxxxxx>
>
> The ADS131E0x are a family of multichannel, simultaneous sampling,
> 24-bit, delta-sigma, analog-to-digital converters (ADCs) with a
> built-in programmable gain amplifier (PGA), internal reference
> and an onboard oscillator.

...

> +config TI_ADS131E08
> +       tristate "Texas Instruments ADS131E08"
> +       depends on SPI && OF

Why OF?!

> +       select IIO_BUFFER
> +       select IIO_TRIGGERED_BUFFER
> +       help
> +         Say yes here to get support for Texas Instruments ADS131E04, ADS131E06
> +         and ADS131E08 chips.
> +
> +         This driver can also be built as a module. If so, the module will be
> +         called ti-ads131e08.

...

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Texas Instruments ADS131E0x 4-, 6- and 8-Channel ADCs
> + *
> + * Copyright (c) 2020 AVL DiTEST GmbH
> + *   Tomislav Denis <tomislav.denis@xxxxxxx>
> + *
> + * Datasheet: https://www.ti.com/lit/ds/symlink/ads131e08.pdf

Please, put this as a tag in the commit message as well

> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>

> +#include <linux/of_device.h>

Why?!

> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <asm/unaligned.h>

Can we group these to something like (wit blank lines between groups)

linux/*.h

linux/iio*.h

asm/*.h

?

...

> +#define ADS131E08_CMD_RREG(r)          (0x20 | (r & GENMASK(4, 0)))
> +#define ADS131E08_CMD_WREG(r)          (0x40 | (r & GENMASK(4, 0)))

BIT(5)
BIT(6)

...

> +#define ADS131E08_VREF_2V4_MV          2400
> +#define ADS131E08_VREF_4V_MV           4000

I prefer mV despite this being defined constant.

...

> +struct ads131e08_state {
> +       const struct ads131e08_info *info;
> +       struct spi_device *spi;
> +       struct iio_trigger *trig;
> +       struct clk *adc_clk;
> +       struct regulator *vref_reg;
> +       struct ads131e08_channel_config *channel_config;
> +       unsigned int data_rate;
> +       unsigned int vref_mv;
> +       unsigned int sdecode_delay_us;
> +       unsigned int reset_delay_us;
> +       unsigned int readback_len;
> +       struct completion completion;

> +

Redundant blank line. But it's up to you.

> +       struct {
> +               u8 data[ADS131E08_NUM_DATA_BYTES_MAX];
> +               s64 ts __aligned(8);
> +       } tmp_buf;
> +
> +       u8 tx_buf[3] ____cacheline_aligned;
> +       /*
> +        * Add extra one padding byte to be able to access the last channel
> +        * value using u32 pointer
> +        */
> +       u8 rx_buf[ADS131E08_NUM_STATUS_BYTES +
> +               ADS131E08_NUM_DATA_BYTES_MAX + 1];
> +};

...

> +static const struct ads131e08_data_rate_desc ads131e08_data_rate_tbl[] = {
> +       { .rate = 64,   .reg = 0x00 },
> +       { .rate = 32,   .reg = 0x01 },
> +       { .rate = 16,   .reg = 0x02 },
> +       { .rate = 8,    .reg = 0x03 },
> +       { .rate = 4,    .reg = 0x04 },
> +       { .rate = 2,    .reg = 0x05 },
> +       { .rate = 1,    .reg = 0x06 },
> +};

Can't you use simple bit operations on this?

rate = BIT(6 - index)
reg = index

...

> +static const struct ads131e08_pga_gain_desc ads131e08_pga_gain_tbl[] = {
> +       { .gain = 1,   .reg = 0x01 },
> +       { .gain = 2,   .reg = 0x02 },

reg == 3 valid?

> +       { .gain = 4,   .reg = 0x04 },
> +       { .gain = 8,   .reg = 0x05 },
> +       { .gain = 12,  .reg = 0x06 },
> +};

Also can be changed by formula, but I remember that in some cases
tables are preferable.

...

> +static int ads131e08_exec_cmd(struct ads131e08_state *st, u8 cmd)
> +{
> +       int ret;
> +
> +       ret = spi_write_then_read(st->spi, &cmd, 1, NULL, 0);
> +       if (ret) {
> +               dev_err(&st->spi->dev,
> +                       "%s: SPI write failed for cmd: %02x\n", __func__, cmd);

__func__ is not needed.

> +               return ret;
> +       }
> +
> +       return 0;

The last is redundant, simply
  return ret;
instead of both of them.

> +}
> +
> +static int ads131e08_read_reg(struct ads131e08_state *st, u8 reg)
> +{
> +       int ret;

> +

Redundant blank line.

> +       struct spi_transfer transfer[] = {
> +               {
> +                       .tx_buf = &st->tx_buf,
> +                       .len = 2,
> +                       .delay_usecs = st->sdecode_delay_us,
> +               }, {
> +                       .rx_buf = &st->rx_buf,
> +                       .len = 1,
> +               },
> +       };
> +
> +       st->tx_buf[0] = ADS131E08_CMD_RREG(reg);
> +       st->tx_buf[1] = 0;
> +
> +       ret = spi_sync_transfer(st->spi, transfer, ARRAY_SIZE(transfer));
> +       if (ret) {
> +               dev_err(&st->spi->dev, "%s: SPI transfer failed\n", __func__);

__func__ is almost useless. Better to think about the uniqueness of
the messages across the driver.

> +               return ret;
> +       }
> +
> +       return st->rx_buf[0];
> +}

> +static int ads131e08_write_reg(struct ads131e08_state *st, u8 reg, u8 value)
> +{

Similar comments as per previous function.

> +       ret = spi_sync_transfer(st->spi, transfer, ARRAY_SIZE(transfer));
> +       if (ret) {
> +               dev_err(&st->spi->dev, "%s: SPI transfer failed\n", __func__);

> +               return ret;
> +       }
> +
> +       return 0;

return ret;

> +}
> +
> +static int ads131e08_read_data(struct ads131e08_state *st, int rx_len)
> +{

As above.

> +}

...

> +       for (i = 0; i < ARRAY_SIZE(ads131e08_data_rate_tbl); ++i) {

i++ works the same and more regular pattern.
Ditto for other occurrences.

> +               if (ads131e08_data_rate_tbl[i].rate == data_rate)
> +                       break;
> +       }

...

> +       struct ads131e08_state *st = iio_priv(indio_dev);
> +       const struct iio_chan_spec *channel = indio_dev->channels;
> +       int ret, i;
> +       u8 active_channels = 0;

Reversed xmas tree order?
Also for the rest of the driver.

...

> +       /* Power down unused channels */
> +       for (i = 0; i < st->info->max_channels; ++i) {
> +               if (!(active_channels & BIT(i))) {

NIH of for_each_clear_bit()

> +                       ret = ads131e08_power_down_channel(st, i, true);
> +                       if (ret)
> +                               return ret;
> +               }
> +       }

...

> +       /*
> +        * Channel offset calibration is triggered with first START command.

the first

> +        * Since calibration take more time than settling operation,

takes

> +        * this causes timeout error when command START is sent first
> +        * time (e.g. first call of the ads131e08_read_direct method).
> +        * To avoid this problem offset calibration is triggered here.
> +        */

...

> +       ret = ads131e08_exec_cmd(st, ADS131E08_CMD_STOP);
> +       if (ret)
> +               return ret;
> +
> +       return 0;

return _exec_cmd(...);

...

> +       ret = wait_for_completion_timeout(&st->completion,
> +               msecs_to_jiffies(ADS131E08_MAX_SETTLING_TIME_MS));

With a temporary variable is easier to read.

... timeout = msecs_to_jiffies(...);

       ret = wait_for_completion_timeout(&st->completion, timeout);

> +       if (!ret)
> +               return -ETIMEDOUT;

...

> +       *value = sign_extend32(
> +               get_unaligned_be32(src) >> (32 - num_bits), num_bits - 1);

One line, please.

> +       return ret;

Expected other than 0?

...

> +       case IIO_CHAN_INFO_SCALE:
> +               if (!IS_ERR(st->vref_reg)) {

Why not positive conditional?

> +                       ret = regulator_get_voltage(st->vref_reg);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       *value = ret / 1000;
> +               } else {
> +                       *value = st->vref_mv;
> +               }

...

> +       return -EINVAL;

Can you use rather
  default:
   return -EINVAL;
instead?

...

> +       switch (mask) {
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               ret = iio_device_claim_direct_mode(indio_dev);
> +               if (ret)
> +                       return ret;
> +
> +               ret = ads131e08_set_data_rate(st, value);
> +               iio_device_release_direct_mode(indio_dev);
> +               return ret;
> +       }
> +
> +       return -EINVAL;

Ditto.

...

> +static int ads131e08_debugfs_reg_access(struct iio_dev *indio_dev,
> +       unsigned int reg, unsigned int writeval, unsigned int *readval)
> +{
> +       struct ads131e08_state *st = iio_priv(indio_dev);
> +
> +       if (readval) {
> +               int ret = ads131e08_read_reg(st, (u8)reg);

Is casting needed?

> +               *readval = ret;
> +               return ret;
> +       }

...

> +       /*
> +        * The number of data bits per channel depends on the data rate.
> +        * For 32 and 64 ksps data rates, number of data bits per channel
> +        * is 16. This case is not compliant with used (fixed) scan element
> +        * type (be:s24/32>>8). So we use a litle tweek to pack properly

little tweak

> +        * 16 bits of data into the buffer.
> +        */

...

> +       for_each_set_bit(chn, indio_dev->active_scan_mask,
> +               indio_dev->masklength) {

One line.

...

> +               /*
> +                * Data conversion from 16 bits of data to 24 bits of data
> +                * is done by sign etension (properly filling padding byte).

extension

> +                */
> +               if (tweek_offset)
> +                       memset(dest, *src & BIT(7) ? 0xff : 0x00, 1);

One byte with memset()?!

...

> +       ret = of_property_read_u32(node, "ti,vref-internal", &tmp);

device_property_read_u32()

> +       if (ret)
> +               tmp = 0;

...

> +       num_channels = of_get_available_child_count(node);

fwnode API has something OF agnostic.

> +       if (num_channels == 0) {
> +               dev_err(&st->spi->dev, "no channel children\n");
> +               return -ENODEV;
> +       }

...

> +       for_each_available_child_of_node(node, child) {
> +               ret = of_property_read_u32(child, "reg", &channel);

> +               ret = of_property_read_u32(child, "ti,gain", &tmp);

> +               ret = of_property_read_u32(child, "ti,mux", &tmp);

> +       }

Easy to convert to fwnode API.

...

> +       info = of_device_get_match_data(&spi->dev);

device_get_match_data()

> +       if (!info) {
> +               dev_err(&spi->dev, "failed to get match data\n");
> +               return -ENODEV;
> +       }

...

> +       indio_dev->dev.of_node = spi->dev.of_node;

Do you need to copy fwnode as well?

...

> +       st->adc_clk = devm_clk_get(&spi->dev, "adc-clk");
> +       if (IS_ERR(st->adc_clk)) {
> +               dev_err(&spi->dev, "failed to get the ADC clock\n");

Spam when deferred.

> +               return PTR_ERR(st->adc_clk);
> +       }

...

> +       ret = clk_prepare_enable(st->adc_clk);
> +       if (ret) {
> +               dev_err(&spi->dev, "failed to prepare/enable the ADC clock\n");
> +               return ret;
> +       }
> +
> +       ret = devm_add_action_or_reset(&spi->dev, ads131e08_clk_disable, st);
> +       if (ret) {

> +               clk_disable_unprepare(st->adc_clk);

"_or_reset" does call this, no?

> +               return ret;
> +       }

Actually both calls can be moved to a separate helper.

...

> +       adc_clk_ns = NSEC_PER_SEC / adc_clk_hz;
> +       st->sdecode_delay_us = DIV_ROUND_UP(
> +               ADS131E08_WAIT_SDECODE_CYCLES * adc_clk_ns, 1000);

NSEC_PER_USEC

> +       st->reset_delay_us = DIV_ROUND_UP(
> +               ADS131E08_WAIT_RESET_CYCLES * adc_clk_ns, 1000);

Ditto.


> +       ret = devm_iio_device_register(&spi->dev, indio_dev);
> +       if (ret) {
> +               dev_err(&spi->dev, "failed to register IIO device\n");
> +               return ret;
> +       }
> +
> +       return 0;

return devm_iio_device_register(...);

-- 
With Best Regards,
Andy Shevchenko



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux