| 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: <20211127162345.66f5526f@jic23-huawei>
Date: Sat, 27 Nov 2021 16:23:45 +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 v4 1/2] iio:amplifiers:ad7293: add support for AD7293
On Mon, 22 Nov 2021 15:36:38 +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>
> ---
> changes in v4:
> - move driver to DAC directory, as suggested
> - use local variable `length` where FIELD_GET is used multiple times
> - return `IIO_AVAIL_LIST` directly, instead of break
> - reduce number of lines in the channel definitions section
> drivers/iio/dac/Kconfig | 11 +
> drivers/iio/dac/Makefile | 1 +
> drivers/iio/dac/ad7293.c | 892 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 904 insertions(+)
> create mode 100644 drivers/iio/dac/ad7293.c
>
New day, new things spotted, but they are all very minor so assuming
the dt patch gets a tag from Rob and no one else raises any concerns
I can tidy these up whilst applying.
Obviously if you do happen to do a v5 then handle these as well.
...
> +
> +static int ad7293_set_offset(struct ad7293_state *st, unsigned int ch, u16 offset)
> +{
> + if (ch < AD7293_TSENSE_MIN_OFFSET_CH)
> + return ad7293_spi_write(st, AD7293_REG_VIN0_OFFSET + ch, offset);
> + else if (ch < AD7293_ISENSE_MIN_OFFSET_CH)
> + return ad7293_spi_write(st, AD7293_REG_TSENSE_INT_OFFSET + (ch - AD7293_TSENSE_MIN_OFFSET_CH), offset);
These are 'very' long lines. I'll tweak this whilst applying if you don't end up doing a v5 for any other reason.
> + else if (ch < AD7293_VOUT_MIN_OFFSET_CH)
> + return ad7293_spi_write(st, AD7293_REG_ISENSE0_OFFSET + (ch - AD7293_ISENSE_MIN_OFFSET_CH), offset);
> + else if (ch <= AD7293_VOUT_MAX_OFFSET_CH)
> + return ad7293_spi_update_bits(st, AD7293_REG_UNI_VOUT0_OFFSET + (ch - AD7293_VOUT_MIN_OFFSET_CH),
> + AD7293_REG_VOUT_OFFSET_MSK,
> + FIELD_PREP(AD7293_REG_VOUT_OFFSET_MSK, offset));
> +
> + return -EINVAL;
> +}
> +
...
> +
> +static int ad7293_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long info)
> +{
> + struct ad7293_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + if (!chan->output)
> + return -EINVAL;
> +
> + return ad7293_dac_write_raw(st, chan->channel, val);
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_OFFSET:
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + if (chan->output)
> + return ad7293_set_offset(st, chan->channel + AD7293_VOUT_MIN_OFFSET_CH, val);
More long lines. I'll add some line breaks if I end up tidying this up whilst applying.
Ideally we should still stay under 80 chars where it doesn't hurt readability and under
100 in any case other than possibly a very long string that we don't want to break
because it will make it hard to grep for.
> + else
> + return ad7293_set_offset(st, chan->channel, val);
> + case IIO_CURRENT:
> + return ad7293_set_offset(st, chan->channel + AD7293_ISENSE_MIN_OFFSET_CH, val);
> + case IIO_TEMP:
> + return ad7293_set_offset(st, chan->channel + AD7293_TSENSE_MIN_OFFSET_CH, val);
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + return ad7293_adc_set_scale(st, chan->channel, val);
> + case IIO_CURRENT:
> + return ad7293_isense_set_scale(st, chan->channel, val);
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
...
> +
> +static int ad7293_init(struct ad7293_state *st)
> +{
> + int ret;
> + u16 chip_id;
> + struct spi_device *spi = st->spi;
> + unsigned int supply;
> +
> + ret = ad7293_properties_parse(st);
> + if (ret)
> + return ret;
> +
> + ret = ad7293_reset(st);
> + if (ret)
> + return ret;
> +
> + ret = regulator_enable(st->reg_avdd);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to enable specified AVDD Voltage!\n");
> + return ret;
> + }
> +
> + ret = devm_add_action_or_reset(&spi->dev, ad7293_reg_disable,
> + st->reg_avdd);
> + if (ret)
> + return ret;
> +
> + ret = regulator_enable(st->reg_vdrive);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to enable specified VDRIVE Voltage!\n");
> + return ret;
> + }
> +
> + ret = devm_add_action_or_reset(&spi->dev, ad7293_reg_disable,
> + st->reg_vdrive);
> + if (ret)
> + return ret;
> +
> + supply = regulator_get_voltage(st->reg_avdd);
Missed this before, but regulator_get_voltage() returns a signed int
and it can be an error code. If that happens we should return that error
rather than replacing it with -EINVAL.
> + if (supply > 5500000 || supply < 4500000)
> + return -EINVAL;
> +
> + supply = regulator_get_voltage(st->reg_vdrive);
> + if (supply > 5500000 || supply < 1700000)
> + return -EINVAL;
> +
> + /* Check Chip ID */
> + ret = __ad7293_spi_read(st, AD7293_REG_DEVICE_ID, &chip_id);
> + if (ret)
> + return ret;
> +
> + if (chip_id != AD7293_CHIP_ID) {
> + dev_err(&spi->dev, "Invalid Chip ID.\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
Powered by blists - more mailing lists