On Wed, May 22, 2024 at 02:59:53PM +0300, ranechita wrote:
> Added support for ad7770,ad7771,ad7779 ADCs. The
> data is streamed only on the spi-mode, without
> using the data lines.
> ---
Please, explain here, in the comment area, why any existing driver can not be
reused (extended) for these ADCs.
...
> +#include <linux/gpio.h>
This header must not be in the new code.
...
> +#define AD777X_SINC3_MAXFREQ 16000
> +#define AD777X_SINC5_MAXFREQ 128000
HZ_PER_KHZ ? You will need units.h.
...
> +#define DEC3 1000
> +#define DEC6 1000000
NIH some of units.h constants. Use them.
...
> + /*
> + * DMA (thus cache coherency maintenance) requires the
> + * transfer buffers to live in their own cache lines.
> + */
> + u8 reg_rx_buf[3] ____cacheline_aligned;
> + u8 reg_tx_buf[3];
> + u8 spidata_rx[32];
> + u8 spidata_tx[32];
These will not be cache aligned. Is it a problem?
> + u8 reset_buf[8];
> +};
...
> +static bool ad777x_has_axi_adc(struct device *dev)
> +{
> + return device_property_present(dev, "spibus-connected");
> +}
Seems like useless wrapper to me. Why can't be used in-line?
...
> + st->reg_tx_buf[0] = AD777X_SPI_READ_CMD | (reg & 0x7F);
GENMASK()
> + st->reg_tx_buf[1] = 0;
> + st->reg_tx_buf[2] = crc8(ad777x_crc8_table, st->reg_tx_buf, 2, 0);
...
> + ret = spi_sync_transfer(st->spi, reg_read_tr,
> + ARRAY_SIZE(reg_read_tr));
One line.
Where is the ret check?
> + crc_buf[0] = AD777X_SPI_READ_CMD | (reg & 0x7F);
GENMASK()
> + crc_buf[1] = st->reg_rx_buf[1];
> + exp_crc = crc8(ad777x_crc8_table, crc_buf, 2, 0);
> + if (st->crc_enabled && exp_crc != st->reg_rx_buf[2]) {
> + dev_err(&st->spi->dev, "Bad CRC %x, expected %x",
> + st->reg_rx_buf[2], exp_crc);
> + return -EINVAL;
> + }
> + *rbuf = st->reg_rx_buf[1];
> +
> + return ret;
...
> + return spi_sync_transfer(st->spi, reg_write_tr,
> + ARRAY_SIZE(reg_write_tr));
One line. Haven't you forgot to include array_size.h?
...
> +static int ad777x_spi_write_mask(struct ad777x_state *st,
> + u8 reg,
> + u8 mask,
> + u8 val)
Make it less LoCs.
> +{
> + int ret;
> + u8 regval, data;
> +
> + ret = ad777x_spi_read(st, reg, &data);
> + if (ret)
> + return ret;
> +
> + regval = data;
> + regval &= ~mask;
> + regval |= val;
> +
> + if (regval != data) {
> + ret = ad777x_spi_write(st, reg, regval);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
This all can be written as
if (regval != data)
return ad777x_spi_write(st, reg, regval);
return 0;
...or...
if (regval == data)
return 0;
return ad777x_spi_write(st, reg, regval);
(I prefer the latter as it shows better the flow)
> +}
No mutex no nothing for RMW op like this?
Btw, can't you use regmap for IO?
...
> + if (st->filter_enabled == AD777X_SINC3 &&
> + sampling_freq > AD777X_SINC3_MAXFREQ) {
> + return -EINVAL;
> + } else if (st->filter_enabled == AD777X_SINC5 &&
Redundant 'else'
> + sampling_freq > AD777X_SINC5_MAXFREQ) {
Broken indentation.
> + return -EINVAL;
> + }
Unneeded {}.
...
> + ret = ad777x_spi_write(st, AD777X_REG_SRC_N_LSB, lsb);
> + if (ret)
> + return ret;
> + ret = ad777x_spi_write(st, AD777X_REG_SRC_N_MSB, msb);
> + if (ret)
> + return ret;
Can you use 16-bit writes?
Same Q to all similar LSB/MSB write groups.
...
> + if (div % kfreq != 0) {
' != 0' is redundant
> + }
...
> + ret |= ad777x_spi_write(st, AD777X_REG_SRC_UPDATE, 0x1);
|= ???
> + if (ret)
> + return ret;
> + usleep_range(10, 15);
fsleep()
...
> + ret |= ad777x_spi_write(st, AD777X_REG_SRC_UPDATE, 0x0);
> + if (ret)
> + return ret;
> + usleep_range(10, 15);
The same two comments as per above.
...
> + ret = ad777x_spi_write_mask(st, AD777X_REG_DOUT_FORMAT,
> + AD777X_DOUT_FORMAT_MSK,
> + FIELD_PREP(AD777X_DOUT_FORMAT_MSK,
> + mode));
Broken indentation.
Where is the ret check?
> + switch (mode) {
> + case AD777x_4LINES:
> + ret = ad777x_set_sampling_frequency(st,
> + AD777X_DEFAULT_SAMPLING_FREQ);
There is no point to have this line being wrapped.
> + if (ret)
> + return ret;
> + axiadc_write(axi_adc_st, ADI_REG_CNTRL, AXI_CTRL_4_LINES);
> + break;
> + case AD777x_2LINES:
> + ret = ad777x_set_sampling_frequency(st,
> + AD777X_DEFAULT_SAMPLING_2LINE);
Ditto.
> + if (ret)
> + return ret;
> + axiadc_write(axi_adc_st, ADI_REG_CNTRL, AXI_CTRL_2_LINES);
> + break;
> + case AD777x_1LINE:
> + ret = ad777x_set_sampling_frequency(st,
> + AD777X_DEFAULT_SAMPLING_1LINE);
Ditto.
> + if (ret)
> + return ret;
> + axiadc_write(axi_adc_st, ADI_REG_CNTRL, AXI_CTRL_1_LINE);
> + break;
> + default:
> + return -EINVAL;
> + }
...
> +static int ad777x_set_filter(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + unsigned int mode)
> +{
> + struct ad777x_state *st = ad777x_get_data(indio_dev);
> + int ret = 0;
What is the purpose of the assignment?
> + ret = ad777x_spi_write_mask(st,
> + AD777X_REG_GENERAL_USER_CONFIG_2,
> + AD777X_FILTER_MSK,
> + FIELD_PREP(AD777X_FILTER_MSK, mode));
> + if (ret < 0)
> + return ret;
> +
> + ret = ad777x_set_sampling_frequency(st, st->sampling_freq);
> + if (ret < 0)
> + return ret;
> +
> + st->filter_enabled = mode;
> +
> + return 0;
> +}
...
> + switch (mask) {
> + case IIO_CHAN_INFO_CALIBSCALE:
> + return ad777x_set_calibscale(st, chan->channel, val2);
> + case IIO_CHAN_INFO_CALIBBIAS:
> + return ad777x_set_calibbias(st, chan->channel, val);
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return ad777x_set_sampling_frequency(st, val);
> + }
> +
> + return -EINVAL;
Use 'default' case.
...
> + for (i = 0; i < AD777X_NUM_CHANNELS; i++) {
> + bit = test_bit(i, scan_mask);
> + if (bit)
> + st->active_ch |= BIT(i);
> + else
> + st->active_ch &= ~BIT(i);
> + }
How is this differ to bitmap_copy()?
...
> + for (i = 0; i < AD777X_NUM_CHANNELS; i++) {
> + if (st->active_ch & BIT(i)) {
for_each_set_bit();
> + tmp[k] = st->spidata_rx[4 * i];
> + tmp[k + 1] = st->spidata_rx[4 * i + 1];
> + tmp[k + 2] = st->spidata_rx[4 * i + 2];
> + tmp[k + 3] = st->spidata_rx[4 * i + 3];
Shouldn't be __le32 used for the Rx buffer?
With that it it as simple as copy __le32 to a CPU u32.
> + k += 4;
> + }
> + }
...
> + for (i = 0; i < AD777X_RESET_BUF_SIZE; i++)
> + st->reset_buf[i] = 0xFF;
memset().
...
> + if (reset_gpio) {
> + gpiod_set_value(reset_gpio, 1);
> + usleep_range(225, 230);
fsleep()
> + return 0;
> + }
> +
> + ret = spi_sync_transfer(st->spi, reg_read_tr,
> + ARRAY_SIZE(reg_read_tr));
> + if (ret)
> + return ret;
> + usleep_range(225, 230);
fsleep()
...
> +static const struct iio_chan_spec_ext_info ad777x_ext_info[] = {
> + IIO_ENUM("data_lines", IIO_SHARED_BY_ALL, &ad777x_data_lines_enum),
> + IIO_ENUM_AVAILABLE("data_lines", IIO_SHARED_BY_ALL,
> + &ad777x_data_lines_enum),
> + { },
No comma for the terminator entry. Same for the other similar cases.
> +};
...
> + .max_rate = 4096000UL,
HZ_PER_KHZ ?
...
> + .max_rate = 4096000UL,
Ditto.
...
> +static void ad777x_clk_disable(void *data)
> +{
> + clk_disable_unprepare(data);
> +}
See below.
...
> + if (strcmp(st->chip_info->name, "ad7771") == 0)
> + conv->chip_info = &conv_chip_info_filter;
> + else
> + conv->chip_info = &conv_chip_info;
No, just make it driver_data directly.
...
> + if (strcmp(st->chip_info->name, "ad7771") == 0)
> + indio_dev->channels = ad777x_channels_filter;
> + else
> + indio_dev->channels = ad777x_channels;
Ditto.
...
> + ret = devm_request_threaded_irq(&st->spi->dev, st->spi->irq, NULL,
With
struct device *dev = &st->spi->dev;
entire function become easier to read.
> + ad777x_irq_handler, IRQF_ONESHOT,
> + indio_dev->name, indio_dev);
> + if (ret)
> + return dev_err_probe(&st->spi->dev, ret,
> + "request irq %d failed\n",
> + st->spi->irq);
...
> + gpiod_set_value(start_gpio, 0);
> + usleep_range(10, 15);
> + gpiod_set_value(start_gpio, 1);
> + usleep_range(10, 15);
> + gpiod_set_value(start_gpio, 0);
> + usleep_range(10, 15);
fsleep() in all cases.
...
> + ret = devm_add_action_or_reset(&spi->dev,
> + ad777x_reg_disable,
> + st->vref);
Make it occupy less LoCs.
> + if (ret)
> + return ret;
...
> + st->mclk = devm_clk_get(&spi->dev, "mclk");
> + if (IS_ERR(st->mclk))
> + return PTR_ERR(st->mclk);
> +
> + ret = clk_prepare_enable(st->mclk);
> + if (ret < 0)
> + return ret;
> + ret = devm_add_action_or_reset(&spi->dev,
> + ad777x_clk_disable,
> + st->mclk);
> + if (ret)
> + return ret;
So, what's wrong with the _enabled() API?
...
> + st->chip_info = device_get_match_data(&spi->dev);
> + if (!st->chip_info) {
> + const struct spi_device_id *id = spi_get_device_id(spi);
> +
> + if (id) {
> + st->chip_info =
> + (struct ad777x_chip_info *)id->driver_data;
> + }
> + if (!st->chip_info)
> + return -EINVAL;
> + }
We have an API for all this.
spi_get_device_match_data().
...
> +static SIMPLE_DEV_PM_OPS(ad777x_pm_ops, ad777x_suspend, ad777x_resume);
Use new PM macros that starts with DEFINE_.
...
> +static struct spi_driver ad777x_driver = {
> + .driver = {
> + .name = "ad777x",
> + .pm = &ad777x_pm_ops,
You will need a pm_sleep_ptr() or alike.
> + .of_match_table = ad777x_of_table,
> + },
> + .probe = ad777x_probe,
> + .id_table = ad777x_id,
> +};
> +module_spi_driver(ad777x_driver);
--
With Best Regards,
Andy Shevchenko
