| 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: <ZvFUR-mZJ4XufcAE@smile.fi.intel.com>
Date: Mon, 23 Sep 2024 14:43:03 +0300
From: Andy Shevchenko <andy@...nel.org>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Nuno Sa <nuno.sa@...log.com>,
Olivier Moysan <olivier.moysan@...s.st.com>,
Uwe Kleine-König <ukleinek@...nel.org>,
David Lechner <dlechner@...libre.com>,
Marcelo Schmitt <marcelo.schmitt@...log.com>,
João Paulo Gonçalves <joao.goncalves@...adex.com>,
Mike Looijmans <mike.looijmans@...ic.nl>,
Dumitru Ceclan <mitrutzceclan@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Alisa-Dariana Roman <alisadariana@...il.com>,
Sergiu Cuciurean <sergiu.cuciurean@...log.com>,
Dragos Bogdan <dragos.bogdan@...log.com>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-pwm@...r.kernel.org
Subject: Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver
On Mon, Sep 23, 2024 at 01:10:23PM +0300, Antoniu Miclaus wrote:
> Add support for the AD485X DAS familiy.
...
> +#include <asm/unaligned.h>
It's better to move this after linux/*.h
> +#include <linux/delay.h>
> +#include <linux/iio/backend.h>
> +#include <linux/iio/iio.h>
This can be split to a group after the other linux/*.h
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
Seems missing headers:
+ array_size.h
+ bits.h
+ device.h
+ err.h
+ mod_devicetable.h
+ module.h
+ mutex.h
+ types.h
...
> +static int ad485x_set_sampling_freq(struct ad485x_state *st, unsigned int freq)
> +{
> + struct pwm_state cnv_state = {
> + .duty_cycle = AD485X_T_CNVH_NS,
> + .enabled = true,
> + };
> + int ret;
> +
> + if (freq > st->info->throughput)
> + freq = st->info->throughput;
clamp() ? (will need minmax.h)
> + cnv_state.period = DIV_ROUND_CLOSEST_ULL(1000000000, freq);
We have time / frequency constants, like HZ_PER_..., NSEC_PER_....
> + ret = pwm_apply_might_sleep(st->cnv, &cnv_state);
> + if (ret)
> + return ret;
> +
> + st->sampling_freq = freq;
> +
> + return 0;
> +}
> +
> +static int ad485x_setup(struct ad485x_state *st)
> +{
> + unsigned int product_id;
> + int ret;
> +
> + ret = ad485x_set_sampling_freq(st, 1000000);
Ditto.
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> + AD485X_SW_RESET);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_B,
> + AD485X_SINGLE_INSTRUCTION);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> + AD485X_SDO_ENABLE);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_PRODUCT_ID_L, &product_id);
> + if (ret)
> + return ret;
> +
> + if (product_id != st->info->product_id)
> + dev_warn(&st->spi->dev, "Unknown product ID: 0x%02X\n",
> + product_id);
> +
> + ret = regmap_write(st->regmap, AD485X_REG_DEVICE_CTRL,
> + AD485X_ECHO_CLOCK_MODE);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_PACKET, 0);
> +}
...
> +static int ad485x_find_opt(bool *field, u32 size, u32 *ret_start)
> +{
> + int i, cnt = 0, max_cnt = 0, start, max_start = 0;
Why are they signed?
> + for (i = 0, start = -1; i < size; i++) {
> + if (field[i] == 0) {
> + if (start == -1)
> + start = i;
> + cnt++;
> + } else {
> + if (cnt > max_cnt) {
> + max_cnt = cnt;
> + max_start = start;
> + }
> + start = -1;
> + cnt = 0;
> + }
> + }
> +
> + if (cnt > max_cnt) {
> + max_cnt = cnt;
> + max_start = start;
> + }
> +
> + if (!max_cnt)
> + return -EIO;
> +
> + *ret_start = max_start;
> +
> + return max_cnt;
> +}
...
> +static int ad485x_calibrate(struct ad485x_state *st)
> +{
> + int opt_delay, lane_num, delay, i, s, c;
Why e.g. i is signed?
> + enum iio_backend_interface_type interface_type;
> + bool pn_status[AD485X_MAX_LANES][AD485X_MAX_IODELAY];
Why not a bitmap?
> + int ret;
> +}
...
> +static const char *const ad4858_packet_fmts[] = {
> + "20-bit", "24-bit", "32-bit"
Leave trailing comma, it may help extending the list in case it becomes a
multi-line.
> +};
> +
> +static const char *const ad4857_packet_fmts[] = {
> + "16-bit", "24-bit"
Ditto.
> +};
...
> +static int ad485x_get_packet_format(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct ad485x_state *st = iio_priv(indio_dev);
> + unsigned int format;
> + int ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_PACKET, &format);
> + if (ret)
> + return ret;
> + format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
> +
> + return format;
return FIELD_GET(...);
> +}
> +static int ad485x_get_calibscale(struct ad485x_state *st, int ch, int *val,
> + int *val2)
One line?
...
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> + ®_val);
> + if (ret)
> + return ret;
> +
> + gain = (reg_val & 0xFF) << 8;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> + ®_val);
> + if (ret)
> + return ret;
> +
> + gain |= reg_val & 0xFF;
Why not bulk read and proper __le16/__be16 type?
...
> +static int ad485x_set_calibscale(struct ad485x_state *st, int ch, int val,
> + int val2)
> +{
> + unsigned long long gain;
> + unsigned int reg_val;
> + int ret;
> +
> + gain = val * 1000000 + val2;
MICRO?
> + gain = gain * 32768;
> + gain = DIV_U64_ROUND_CLOSEST(gain, 1000000);
Can be combined into one lien.
> + reg_val = gain;
...
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> + reg_val >> 8);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> + reg_val & 0xFF);
Bulk write?
> +}
...
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
> + &msb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
> + &mid);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch),
> + &lsb);
> + if (ret)
> + return ret;
Bulk read?
> + if (st->info->resolution == 16) {
> + *val = msb << 8;
> + *val |= mid;
> + *val = sign_extend32(*val, 15);
So, please use respective byteorder conversions...
> + } else {
> + *val = msb << 12;
> + *val |= mid << 4;
> + *val |= lsb >> 4;
> + *val = sign_extend32(*val, 19);
...here as well (we have _be14()/_le24 ones).
> + }
...
> +static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val,
> + int val2)
> +{
> + unsigned int lsb, mid, msb;
> + int ret;
> +
> + if (st->info->resolution == 16) {
> + lsb = 0;
> + mid = val & 0xFF;
> + msb = (val >> 8) & 0xFF;
> + } else {
> + lsb = (val << 4) & 0xFF;
> + mid = (val >> 4) & 0xFF;
> + msb = (val >> 12) & 0xFF;
> + }
Ditto.
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch), lsb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch), mid);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb);
Bulk write?
> +}
...
> +static const unsigned int ad485x_scale_table[][2] = {
> + {2500, 0x0}, {5000, 0x1}, {5000, 0x2}, {10000, 0x3}, {6250, 0x04},
> + {12500, 0x5}, {10000, 0x6}, {20000, 0x7}, {12500, 0x8}, {25000, 0x9},
> + {20000, 0xA}, {40000, 0xB}, {25000, 0xC}, {50000, 0xD}, {40000, 0xE},
> + {80000, 0xF}
It's more natural to list them in pow-of-two per line manner.
Moreover the last item is redundant. It's equal to the index. Why do you need it?
> +};
...
> +static const int ad4857_offset_table[] = {
> + 0, -32768
Here, and above and everywhere else in such cases leave a trailing comma.
> +};
...
> + for (i = 0; i < info->num_scales; i++) {
> + __ad485x_get_scale(st, info->scale_table[i][0],
> + &scale_val[0], &scale_val[1]);
> + if (scale_val[0] != val || scale_val[1] != val2)
> + continue;
> +
> + /*
> + * Adjust the softspan value (differential or single ended)
> + * based on the scale value selected and current offset of
> + * the channel.
> + *
> + * If the offset is 0 then continue iterations until finding
> + * the next matching scale value which always corresponds to
> + * the single ended mode.
> + */
> + if (!st->offsets[chan->channel] && !j) {
> + j++;
So, j may be named properly and be boolean for the sake of the semantic usage.
> + continue;
> + }
> +
> + return regmap_write(st->regmap,
> + AD485X_REG_CHX_SOFTSPAN(chan->channel),
> + info->scale_table[i][1]);
> + }
...
> +{
> + guard(mutex)(&st->lock);
> +
> + if (st->offsets[chan->channel] != val) {
Invert and drop indentation for the next lines.
> + st->offsets[chan->channel] = val;
> + /* Restore to the default range if offset changes */
> + if (st->offsets[chan->channel])
> + return regmap_write(st->regmap,
> + AD485X_REG_CHX_SOFTSPAN(chan->channel),
> + AD485X_SOFTSPAN_N40V_40V);
> + return regmap_write(st->regmap,
> + AD485X_REG_CHX_SOFTSPAN(chan->channel),
> + AD485X_SOFTSPAN_0V_40V);
> + }
> +
> + return 0;
> +}
...
> +static int ad485x_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long info)
> +{
> + struct ad485x_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = st->sampling_freq;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + return ad485x_get_calibscale(st, chan->channel, val, val2);
> + case IIO_CHAN_INFO_SCALE:
> + return ad485x_get_scale(st, chan, val, val2);
> + case IIO_CHAN_INFO_CALIBBIAS:
> + return ad485x_get_calibbias(st, chan->channel, val, val2);
> + case IIO_CHAN_INFO_OFFSET:
> + scoped_guard(mutex, &st->lock)
> + * val = st->offsets[chan->channel];
This is interesting white space location...
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
...
> + struct ad485x_state *st = iio_priv(indio_dev);
> + unsigned int c;
> + int ret;
> +
> + for (c = 0; c < st->info->num_channels; c++) {
> + if (test_bit(c, scan_mask))
Do we have a helper now for this iterator?
> + ret = iio_backend_chan_enable(st->back, c);
> + else
> + ret = iio_backend_chan_disable(st->back, c);
> + if (ret)
> + return ret;
> + }
...
> +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> + IIO_ENUM_AVAILABLE("packet_format",
> + IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> + {},
Please, drop comma in the terminator lines.
> +};
...
> +static const struct ad485x_chip_info ad4858i_info = {
> + .name = "ad4858i",
> + .product_id = AD4858I_PROD_ID,
> + .scale_table = ad485x_scale_table,
> + .num_scales = ARRAY_SIZE(ad485x_scale_table),
> + .offset_table = ad4858_offset_table,
> + .num_offset = ARRAY_SIZE(ad4858_offset_table),
> + .channels = ad4858_channels,
> + .num_channels = ARRAY_SIZE(ad4858_channels),
> + .throughput = 1000000,
How many people wrote this patch? It has inconsistency at least in multipliers
like this all over the code. Who knows what's else also being inconsistent...
> + .resolution = 20,
> +};
...
> +static const struct regmap_config regmap_config = {
> + .reg_bits = 16,
> + .val_bits = 8,
> + .read_flag_mask = BIT(7),
> +};
Why do you need regmap lock?
...
> +static const char * const ad485x_power_supplies[] = {
> + "vcc", "vdd", "vddh", "vio"
Leave trailing comma.
> +};
...
> +static int ad485x_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct ad485x_state *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> + mutex_init(&st->lock);
Why not devm?
> + ret = devm_regulator_bulk_get_enable(&spi->dev,
> + ARRAY_SIZE(ad485x_power_supplies),
> + ad485x_power_supplies);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "failed to get and enable supplies\n");
> +
> + st->cnv = devm_pwm_get(&spi->dev, NULL);
> + if (IS_ERR(st->cnv))
> + return PTR_ERR(st->cnv);
> +
> + st->info = spi_get_device_match_data(spi);
> + if (!st->info)
> + return -ENODEV;
> +
> + st->regmap = devm_regmap_init_spi(spi, ®map_config);
> + if (IS_ERR(st->regmap))
> + return PTR_ERR(st->regmap);
> +
> + ret = ad485x_scale_offset_fill(st);
> + if (ret)
> + return ret;
> +
> + ret = ad485x_setup(st);
> + if (ret)
> + return ret;
> +
> + indio_dev->name = st->info->name;
> + indio_dev->channels = st->info->channels;
> + indio_dev->num_channels = st->info->num_channels;
> + indio_dev->info = &ad485x_info;
> +
> + st->back = devm_iio_backend_get(&spi->dev, NULL);
> + if (IS_ERR(st->back))
> + return PTR_ERR(st->back);
> +
> + ret = devm_iio_backend_request_buffer(&spi->dev, st->back, indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_backend_enable(&spi->dev, st->back);
> + if (ret)
> + return ret;
> +
> + ret = ad485x_calibrate(st);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists