| 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: <aeb5e95a-f108-4b23-b14b-c9fe1463775e@wanadoo.fr>
Date: Mon, 29 Jul 2024 23:34:30 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: eblanc@...libre.com
Cc: Michael.Hennerich@...log.com, baylibre-upstreaming@...ups.io,
conor+dt@...nel.org, devicetree@...r.kernel.org, dlechner@...libre.com,
jic23@...nel.org, krzk+dt@...nel.org, lars@...afoo.de,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, nuno.sa@...log.com,
robh@...nel.org
Subject: Re: [PATCH RFC 2/5] iio: adc: ad4030: add driver for ad4030-24
Le 27/06/2024 à 13:59, Esteban Blanc a écrit :
> This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
>
> The driver implements basic support for the AD4030-24 1 channel
> differential ADC with hardware gain and offset control.
>
> Signed-off-by: Esteban Blanc <eblanc-rdvid1DuHRBWk0Htik3J/w@...lic.gmane.org>
> ---
Hi,
a few nitpick below, should it help.
...
> +static int ad4030_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel,
> + const int **vals, int *type,
> + int *length, long mask)
> +{
> + struct ad4030_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_CALIBBIAS:
> + *vals = ad4030_get_offset_avail(st);
> + *type = IIO_VAL_INT;
> + return IIO_AVAIL_RANGE;
> +
> + case IIO_CHAN_INFO_CALIBSCALE:
> + *vals = (void *)ad4030_gain_avail;
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + return IIO_AVAIL_RANGE;
Nitpick: in other switch below, there is a blank line before default:
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int ad4030_regulators_get(struct ad4030_state *st)
> +{
> + struct device *dev = &st->spi->dev;
> + static const char * const ids[] = {"vdd-5v", "vdd-1v8"};
> + int ret;
> +
> + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ids), ids);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> +
> + st->vio_uv = devm_regulator_get_enable_read_voltage(dev, "vio");
> + if (st->vio_uv < 0)
> + return dev_err_probe(dev, st->vio_uv,
> + "Failed to enable and read vio voltage");
Nitpick: missing \n
> +
> + st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> + if (st->vref_uv < 0) {
> + if (st->vref_uv != -ENODEV)
> + return dev_err_probe(dev, st->vref_uv,
> + "Failed to read vref voltage");
Nitpick: missing \n
> +
> + /* if not using optional REF, the internal REFIN must be used */
> + st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "refin");
> + if (st->vref_uv < 0)
> + return dev_err_probe(dev, st->vref_uv,
> + "Failed to read vrefin voltage");
Nitpick: missing \n
> + }
> +
> + if (st->vref_uv < AD4030_VREF_MIN_UV || st->vref_uv > AD4030_VREF_MAX_UV)
> + return dev_err_probe(dev, -EINVAL,
> + "vref(%d) must be in the range [%lu %lu]\n",
> + st->vref_uv, AD4030_VREF_MIN_UV,
> + AD4030_VREF_MAX_UV);
> +
> + return 0;
> +}
...
> +static int ad4030_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct ad4030_state *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
Nitpick: dev could be used. It is the same &spi->dev
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> +
> + /* Make sure the SPI clock is within range to read register */
> + st->conversion_speed_hz = spi->max_speed_hz;
> + if (spi->max_speed_hz > AD4030_SPI_MAX_REG_XFER_SPEED)
> + spi->max_speed_hz = AD4030_SPI_MAX_REG_XFER_SPEED;
> +
> + st->regmap = devm_regmap_init(&spi->dev, &ad4030_regmap_bus, st,
Nitpick: dev could be used. It is the same &spi->dev
> + &ad4030_regmap_config);
> + if (IS_ERR(st->regmap))
> + dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
Nitpick: dev could be used. It is the same &spi->dev
> + "Failed to initialize regmap\n");
> +
> + st->chip = spi_get_device_match_data(spi);
> + if (!st->chip)
> + return -EINVAL;
> +
> + ret = ad4030_regulators_get(st);
> + if (ret)
> + return ret;
> +
> + ret = ad4030_reset(st);
> + if (ret)
> + return ret;
> +
> + ret = ad4030_detect_chip_info(st);
> + if (ret)
> + return ret;
> +
> + ret = ad4030_config(st);
> + if (ret)
> + return ret;
> +
> + st->cnv_gpio = devm_gpiod_get(dev, "cnv", GPIOD_OUT_LOW);
> + if (IS_ERR(st->cnv_gpio))
> + return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
> + "Failed to get cnv gpio");
Nitpick: missing \n
> +
> + indio_dev->name = st->chip->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &ad4030_iio_info;
> + indio_dev->channels = st->chip->channels;
> + indio_dev->num_channels = 2 * st->chip->num_channels + 1;
Nitpick : 2 spaces after =
> + indio_dev->available_scan_masks = st->chip->available_masks;
> + indio_dev->masklength = st->chip->available_masks_len;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> + iio_pollfunc_store_time,
> + ad4030_trigger_handler,
> + &ad4030_buffer_setup_ops);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
...
CJ
Powered by blists - more mailing lists