| 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: <20211121132828.7a266dbc@jic23-huawei>
Date: Sun, 21 Nov 2021 13:39:03 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>
Cc: <robh+dt@...nel.org>, <linux-iio@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] iio:amplifiers:ad7293: add support for AD7293
On Mon, 15 Nov 2021 12:23:39 +0200
Antoniu Miclaus <antoniu.miclaus@...log.com> wrote:
> The AD7293 is a Power Amplifier drain current controller
> containing functionality for general-purpose monitoring
> and control of current, voltage, and temperature, integrated
> into a single chip solution with an SPI-compatible interface.
>
> Datasheet:
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7293.pdf
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
Hi Antoniu
A few minor / trivial things noticed whilst having a read through.
They are all things I'd have either ignored or fixed up if you weren't likely to be
doing a v4 anyway because of the dt-bindings... Seeing as you probably will be, please
tidy these up as well.
This is looking like a nice driver to me.
I'm a bit unsure if we should have this in a directory called amplifiers though.
The description is (I think) referring to closed loop control of current as
shown in figure 46. The circuit with external transistor + sense resistor operates
as a current DAC. As such I'd move this to the DAC directory as I think that's
the primary purpose intended for this device.
I guess a follow up set would supply explicit support for closed loop operation?
That can be enabled when someone has a use case for it.
Thanks,
Jonathan
> ---
...
> +
> +static int __ad7293_spi_read(struct ad7293_state *st, unsigned int reg,
> + u16 *val)
> +{
> + int ret;
> + struct spi_transfer t = {0};
> +
> + ret = ad7293_page_select(st, reg);
> + if (ret)
> + return ret;
> +
> + st->data[0] = AD7293_READ | FIELD_GET(AD7293_REG_ADDR_MSK, reg);
> + st->data[1] = 0x0;
> + st->data[2] = 0x0;
> +
> + t.tx_buf = &st->data[0];
> + t.rx_buf = &st->data[0];
> + t.len = 1 + FIELD_GET(AD7293_TRANSF_LEN_MSK, reg);
Given you are going to use this field multiple times, I would use a local variable.
> +
> + ret = spi_sync_transfer(st->spi, &t, 1);
> + if (ret)
> + return ret;
> +
> + if (FIELD_GET(AD7293_TRANSF_LEN_MSK, reg) == 1)
> + *val = st->data[1];
> + else
> + *val = get_unaligned_be16(&st->data[1]);
> +
> + return 0;
> +}
...
> +
> +static int __ad7293_spi_write(struct ad7293_state *st, unsigned int reg,
> + u16 val)
> +{
> + int ret;
> + unsigned int length = 1 + FIELD_GET(AD7293_TRANSF_LEN_MSK, reg);
I suggest not having the 1 + here
> +
> + ret = ad7293_page_select(st, reg);
> + if (ret)
> + return ret;
> +
> + st->data[0] = FIELD_GET(AD7293_REG_ADDR_MSK, reg);
> +
> + if (FIELD_GET(AD7293_TRANSF_LEN_MSK, reg) == 1)
then this becomes if (length == 1)
> + st->data[1] = val;
> + else
> + put_unaligned_be16(val, &st->data[1]);
> +
> + return spi_write(st->spi, &st->data[0], length);
and you can use length + 1 here. I think that ends up a little bit clearer.
> +}
> +
...
> +
> +static int ad7293_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long info)
> +{
> + switch (info) {
> + case IIO_CHAN_INFO_OFFSET:
> + *vals = dac_offset_table;
> + *type = IIO_VAL_INT;
> + *length = ARRAY_SIZE(dac_offset_table);
> +
> + return IIO_AVAIL_LIST;
> + case IIO_CHAN_INFO_SCALE:
> + *type = IIO_VAL_INT;
> +
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + *vals = adc_range_table;
> + *length = ARRAY_SIZE(adc_range_table);
> + break;
> + case IIO_CURRENT:
> + *vals = isense_gain_table;
> + *length = ARRAY_SIZE(isense_gain_table);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return IIO_AVAIL_LIST;
Trivial: I'd prefer to see this return where the breaks are above.
Nice to pair the values and type in a couple of lines rather than having
to look down here.
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +#define AD7293_CHAN_ADC(_channel) { \
> + .type = IIO_VOLTAGE, \
> + .output = 0, \
> + .indexed = 1, \
> + .channel = _channel, \
> + .info_mask_separate = \
> + BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> + .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_SCALE) \
> +}
Trivial, but I think you could sensibly reduce how many lines these are
spread over without loss of readability.
#define AD7293_CHAN_ADC(_channel) { \
.type = IIO_VOLTAGE, \
.output = 0, \
.indexed = 1, \
.channel = _channel, \
.info_mask_separate = \
BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) | \
BIT(IIO_CHAN_INFO_OFFSET), \
.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE) \
}
etc
> +
> +#define AD7293_CHAN_DAC(_channel) { \
> + .type = IIO_VOLTAGE, \
> + .output = 1, \
> + .indexed = 1, \
> + .channel = _channel, \
> + .info_mask_separate = \
> + BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> + .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> +}
> +
> +#define AD7293_CHAN_ISENSE(_channel) { \
> + .type = IIO_CURRENT, \
> + .output = 0, \
> + .indexed = 1, \
> + .channel = _channel, \
> + .info_mask_separate = \
> + BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_OFFSET) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_SCALE) \
> +}
> +
> +#define AD7293_CHAN_TEMP(_channel) { \
> + .type = IIO_TEMP, \
> + .output = 0, \
> + .indexed = 1, \
> + .channel = _channel, \
> + .info_mask_separate = \
> + BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
> +}
> +
Powered by blists - more mailing lists