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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9c115086bd574b6c778a093143ebf54e14d7202b.camel@gmail.com>
Date: Thu, 10 Apr 2025 07:31:59 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
  devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: Ana-Maria Cusco <ana-maria.cusco@...log.com>, jic23@...nel.org, 
	lars@...afoo.de, Michael.Hennerich@...log.com, dlechner@...libre.com, 
	nuno.sa@...log.com, andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, marcelo.schmitt1@...il.com
Subject: Re: [PATCH v1 2/7] iio: adc: Add basic support for AD4170

Hi Marecelo,

First, superficial look...

On Wed, 2025-04-09 at 09:24 -0300, Marcelo Schmitt wrote:
> From: Ana-Maria Cusco <ana-maria.cusco@...log.com>
> 
> Add support for the AD4170 ADC with the following features:
> - Single-shot read.
> - Analog front end PGA configuration.
> - Digital filter and sampling frequency configuration.
> - Calibration gain and offset configuration.
> - Differential and pseudo-differential input configuration.
> 
> Signed-off-by: Ana-Maria Cusco <ana-maria.cusco@...log.com>
> Co-developed-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> ---
>  MAINTAINERS              |    1 +
>  drivers/iio/adc/Kconfig  |   16 +
>  drivers/iio/adc/Makefile |    1 +
>  drivers/iio/adc/ad4170.c | 1950 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1968 insertions(+)
>  create mode 100644 drivers/iio/adc/ad4170.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 991b6e2e373a..56cd87028dfd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1343,6 +1343,7 @@ L:	linux-iio@...r.kernel.org
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
> +F:	drivers/iio/adc/ad4170.c
>  
>  ANALOG DEVICES INC AD4695 DRIVER
>  M:	Michael Hennerich <michael.hennerich@...log.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 636469392945..de7139fc2a1f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -70,6 +70,22 @@ config AD4130
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad4130.
>  
> +
> +config AD4170
> +	tristate "Analog Device AD4170 ADC Driver"
> +	depends on SPI
> +	depends on GPIOLIB
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	select REGMAP_SPI
> +	depends on COMMON_CLK
> +	help
> +	  Say yes here to build support for Analog Devices AD4170 SPI analog
> +	  to digital converters (ADC).
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ad4170.
> +
>  config AD4695
>  	tristate "Analog Device AD4695 ADC Driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 07d4b832c42e..d3a1376d1f96 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
>  obj-$(CONFIG_AD4000) += ad4000.o
>  obj-$(CONFIG_AD4030) += ad4030.o
>  obj-$(CONFIG_AD4130) += ad4130.o
> +obj-$(CONFIG_AD4170) += ad4170.o
>  obj-$(CONFIG_AD4695) += ad4695.o
>  obj-$(CONFIG_AD4851) += ad4851.o
>  obj-$(CONFIG_AD7091R) += ad7091r-base.o
> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> new file mode 100644
> index 000000000000..0d24286ac2ab
> --- /dev/null
> +++ b/drivers/iio/adc/ad4170.c
> @@ -0,0 +1,1950 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2024 Analog Devices, Inc.
> + * Author: Ana-Maria Cusco <ana-maria.cusco@...log.com>
> + * Author: Marcelo Schmitt <marcelo.schmitt@...log.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +#include <linux/util_macros.h>
> +

...

> +
> +static int ad4170_read_sample(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int *val)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	int settling_time_ms, ret;
> +
> +	guard(mutex)(&st->lock);
> +	/*
> +	 * The ADC sequences through all enabled channels. That can lead to
> +	 * incorrect channel being sampled if a previous read would have left a
> +	 * different channel enabled. Thus, always enable and disable the
> +	 * channel on single-shot read.
> +	 */
> +	ret = ad4170_set_channel_enable(st, chan->address, true);
> +	if (ret)
> +		return ret;
> +
> +	reinit_completion(&st->completion);

I would do the above right before wait_for_completion_timeout()...

> +
> +	ret = ad4170_set_mode(st, AD4170_ADC_CTRL_MODE_SINGLE);
> +	if (ret)
> +		goto err_disable;
> 

...

> +
> +static int ad4170_set_pga(struct ad4170_state *st,
> +			  struct iio_chan_spec const *chan, int val, int val2)
> +{
> +	struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address];
> +	struct ad4170_setup *setup = &chan_info->setup;
> +	unsigned int old_pga = FIELD_GET(AD4170_AFE_PGA_GAIN_MSK, setup->afe);
> +	unsigned int pga;
> +	int ret = 0;
> +
> +	for (pga = 0; pga < AD4170_NUM_PGA_OPTIONS; pga++) {
> +		if (val == chan_info->scale_tbl[pga][0] &&
> +		    val2 == chan_info->scale_tbl[pga][1])
> +			break;
> +	}
> +
> +	if (pga == AD4170_NUM_PGA_OPTIONS)
> +		return -EINVAL;
> +
> +	if (pga == old_pga)
> +		return 0;
> +
> +	setup->afe &= ~AD4170_AFE_PGA_GAIN_MSK;
> +	setup->afe |= FIELD_PREP(AD4170_AFE_PGA_GAIN_MSK, pga);
> 

ditto...

> +
> +	guard(mutex)(&st->lock);
> +	ret = ad4170_write_channel_setup(st, chan->address, false);
> +	if (ret) {
> +		setup->afe &= ~AD4170_AFE_PGA_GAIN_MSK;
> +		setup->afe |= FIELD_PREP(AD4170_AFE_PGA_GAIN_MSK, old_pga);
> +	}
> +
> +	return ret;
> +}
> +
> +static int ad4170_set_channel_freq(struct ad4170_state *st,
> +				   struct iio_chan_spec const *chan, int val,
> +				   int val2)
> +{
> +	struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address];
> +	struct ad4170_setup *setup = &chan_info->setup;
> +	enum ad4170_filter_type f_type = __ad4170_get_filter_type(setup->filter);
> +	int filt_fs_tbl_size, i, ret = 0;
> +	unsigned int old_filter_fs;
> +
> +	switch (f_type) {
> +	case AD4170_SINC5_AVG:
> +		fallthrough;
> +	case AD4170_SINC3:
> +		filt_fs_tbl_size = ARRAY_SIZE(ad4170_sinc3_filt_fs_tbl);
> +		break;
> +	case AD4170_SINC5:
> +		filt_fs_tbl_size = ARRAY_SIZE(ad4170_sinc5_filt_fs_tbl);
> +		break;
> +	}
> +
> +	for (i = 0; i < filt_fs_tbl_size; i++) {
> +		if (st->sps_tbl[f_type][i][0] == val &&
> +		    st->sps_tbl[f_type][i][1] == val2)
> +			break;
> +	}
> +	if (i >= filt_fs_tbl_size)
> +		return -EINVAL;
> +
> +	old_filter_fs = setup->filter_fs;
> +	if (f_type == AD4170_SINC5)
> +		setup->filter_fs = ad4170_sinc5_filt_fs_tbl[i];
> +	else
> +		setup->filter_fs = ad4170_sinc3_filt_fs_tbl[i];
> +
> +	guard(mutex)(&st->lock);

Shouldn't the lock also protect the 'setup' struct?

> +	ret = ad4170_write_channel_setup(st, chan->address, false);
> +	if (ret)
> +		setup->filter_fs = old_filter_fs;
> +
> +	return ret;
> +}
> +
> +static int ad4170_set_calib_offset(struct ad4170_state *st,
> +				   struct iio_chan_spec const *chan, int val)
> +{
> +	struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address];
> +	struct ad4170_setup *setup = &chan_info->setup;
> +	u32 old_offset;
> +	int ret;
> +
> +	old_offset = setup->offset;
> +	setup->offset = val;
> +
> +	guard(mutex)(&st->lock);
> +	ret = ad4170_write_channel_setup(st, chan->address, false);
> +	if (ret)
> +		setup->offset = old_offset;
> +
> +	return ret;
> +}
> +
> +static int ad4170_set_calib_gain(struct ad4170_state *st,
> +				 struct iio_chan_spec const *chan, int val)
> +{
> +	struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address];
> +	struct ad4170_setup *setup = &chan_info->setup;
> +	u32 old_gain;
> +	int ret;
> +
> +	old_gain = setup->gain;
> +	setup->gain = val;
> +
> +	guard(mutex)(&st->lock);
> +	ret = ad4170_write_channel_setup(st, chan->address, false);
> +	if (ret)
> +		setup->gain = old_gain;
> +
> +	return ret;
> +}
> +
> +static int __ad4170_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int val,
> +			      int val2, long info)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return ad4170_set_pga(st, chan, val, val2);
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return ad4170_set_channel_freq(st, chan, val, val2);
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		return ad4170_set_calib_offset(st, chan, val);
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		return ad4170_set_calib_gain(st, chan, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4170_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val,
> +			    int val2, long info)
> +{
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	ret = __ad4170_write_raw(indio_dev, chan, val, val2, info);
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}
> +
> +static int ad4170_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				    struct iio_chan_spec const *chan,
> +				    long info)
> +{
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return IIO_VAL_INT_PLUS_NANO;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info ad4170_info = {
> +	.read_raw = ad4170_read_raw,
> +	.read_avail = ad4170_read_avail,
> +	.write_raw = ad4170_write_raw,
> +	.write_raw_get_fmt = ad4170_write_raw_get_fmt,
> +	.debugfs_reg_access = ad4170_debugfs_reg_access,
> +};
> +
> +static int ad4170_soft_reset(struct ad4170_state *st)
> +{
> +	int ret;
> +
> +	ret = regmap_write(st->regmap8, AD4170_CONFIG_A_REG,
> +			   AD4170_SW_RESET_MSK);
> +	if (ret)
> +		return ret;
> +
> +	/* AD4170-4 requires 1 ms between reset and any register access. */
> +	fsleep(MILLI);
> +
> +	return 0;
> +}
> +
> +static int ad4170_parse_reference(struct ad4170_state *st,
> +				  struct fwnode_handle *child,
> +				  struct ad4170_setup *setup)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +	u8 aux;
> +
> +	/* Positive reference buffer setup */
> +	aux = AD4170_REF_BUF_PRE; /* Default to have precharge buffer enabled. */
> +	ret = fwnode_property_read_u8(child, "adi,buffered-positive", &aux);
> +	if (ret) {

Shouldn't this be if (!ret)?

> +		if (aux < AD4170_REF_BUF_PRE || aux > AD4170_REF_BUF_BYPASS)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Invalid adi,buffered-positive:
> %u\n",
> +					     aux);
> +	}
> +	setup->afe |= FIELD_PREP(AD4170_AFE_REF_BUF_P_MSK, aux);
> +
> +	/* Negative reference buffer setup */
> +	aux = AD4170_REF_BUF_PRE; /* Default to have precharge buffer enabled. */
> +	ret = fwnode_property_read_u8(child, "adi,buffered-negative", &aux);
> +	if (ret) {

ditto

> +		if (aux < AD4170_REF_BUF_PRE || aux > AD4170_REF_BUF_BYPASS)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Invalid adi,buffered-negative:
> %u\n",
> +					     aux);
> +	}
> +	setup->afe |= FIELD_PREP(AD4170_AFE_REF_BUF_M_MSK, aux);
> +
> +	/* Voltage reference selection */
> +	aux = AD4170_REF_REFOUT; /* Default reference selection. */
> +	fwnode_property_read_u8(child, "adi,reference-select", &aux);
> +	if (aux > AD4170_REF_AVDD)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid reference selected %u\n", aux);
> +	setup->afe |= FIELD_PREP(AD4170_AFE_REF_SELECT_MSK, aux);
> +
> +	return 0;
> +}
> +
> +static int ad4170_parse_adc_channel_type(struct device *dev,
> +					 struct fwnode_handle *child,
> +					 struct iio_chan_spec *chan)
> +{
> +	u32 pins[2];
> +	int ret;
> +
> +	ret = fwnode_property_read_u32_array(child, "diff-channels", pins,
> +					     ARRAY_SIZE(pins));
> +	if (!ret) {
> +		chan->differential = true;
> +		chan->channel = pins[0];
> +		chan->channel2 = pins[1];
> +		return 0;
> +	}
> +	ret = fwnode_property_read_u32(child, "single-channel", &pins[0]);
> +	if (!ret) {
> +		chan->differential = false;
> +		chan->channel = pins[0];
> +
> +		ret = fwnode_property_read_u32(child, "common-mode-channel",
> +					       &pins[1]);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +				"single-ended channels must define common-mode-
> channel\n");
> +
> +		chan->channel2 = pins[1];
> +		return 0;
> +	}

Kind of a nitpick but for the above I would flip the logic. First check for errors in
the single-channel case and then you can have one less of level of indentation...

> +	return dev_err_probe(dev, ret,
> +		"Channel must define one of diff-channels or single-channel.\n");
> +}
> +
> +static int ad4170_parse_channel_node(struct iio_dev *indio_dev,
> +				     struct fwnode_handle *child,
> +				     unsigned int chan_num)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->spi->dev;
> +	struct ad4170_chan_info *chan_info;
> +	struct ad4170_setup *setup;
> +	struct iio_chan_spec *chan;
> +	unsigned int ch_reg;
> +	u8 ref_select;
> +	bool bipolar;
> +	int ret;
> +
> +	ret = fwnode_property_read_u32(child, "reg", &ch_reg);
> +	if (ret)
> +		return ret;
> +

Could also deserve a log message?

> +	if (ch_reg >= AD4170_MAX_CHANNELS)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Channel idx greater than no of channels\n");
> 

...

> 
> +
> +static int ad4170_trigger_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	st->trig = devm_iio_trigger_alloc(indio_dev->dev.parent, "%s-trig%d",
> +					  indio_dev->name,
> +					  iio_device_id(indio_dev));
> +	if (!st->trig)
> +		return -ENOMEM;
> +
> +	st->trig->ops = &ad4170_trigger_ops;
> +	st->trig->dev.parent = indio_dev->dev.parent;
> +
> +	iio_trigger_set_drvdata(st->trig, indio_dev);
> +	ret = devm_iio_trigger_register(indio_dev->dev.parent, st->trig);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->trig = iio_trigger_get(st->trig);
> +
> +	return request_irq(st->spi->irq, &ad4170_irq_handler, IRQF_ONESHOT,
> +			   indio_dev->name, indio_dev);

devm_request_irq()...

> +}
> +
> +static int ad4170_regulator_setup(struct ad4170_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +
> +	/* Required regulators */
> +	ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to get AVDD voltage.\n");
> +
> +	st->vrefs_uv[AD4170_AVDD_SUP] = ret;
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "iovdd");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to get IOVDD voltage.\n");
> +
> +	st->vrefs_uv[AD4170_IOVDD_SUP] = ret;
> +
> +	/* Optional regulators */
> +	ret = devm_regulator_get_enable_read_voltage(dev, "avss");
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "Failed to get AVSS voltage.\n");
> +
> +	/* Assume AVSS at GND (0V) if not provided */
> +	st->vrefs_uv[AD4170_AVSS_SUP] = ret == -ENODEV ? 0 : -ret;
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "refin1p");
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "Failed to get REFIN+ voltage.\n");
> +
> +	st->vrefs_uv[AD4170_REFIN1P_SUP] = ret;
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "refin1n");
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "Failed to get REFIN- voltage.\n");
> +
> +	/* Negative supplies are assumed to provide negative voltage */
> +	st->vrefs_uv[AD4170_REFIN1N_SUP] = ret == -ENODEV ? -ENODEV : -ret;

Maybe to early for me but the comment does not make it clear to me why the negation?
Won't the regulator return a negative voltage?

> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "refin2p");
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "Failed to get REFIN2+
> voltage.\n");
> +
> +	st->vrefs_uv[AD4170_REFIN2P_SUP] = ret;
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "refin2n");
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "Failed to get REFIN2-
> voltage.\n");
> +
> +	/* Negative supplies are assumed to provide negative voltage */
> +	st->vrefs_uv[AD4170_REFIN2N_SUP] = ret == -ENODEV ? -ENODEV : -ret;
> +
> +	return 0;
> +}
> +
> +static int ad4170_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad4170_state *st;
> +	const char *dev_name;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	devm_mutex_init(dev, &st->lock);

check for errors...

> +
> +	dev_name = spi_get_device_match_data(spi);
> +	if (!dev_name)
> +		return -EINVAL;
> +
> +	indio_dev->name = dev_name;
> +	indio_dev->info = &ad4170_info;
> +
> +	st->spi = spi;
> +	st->regmap8 = devm_regmap_init_spi(spi, &ad4170_regmap8_config);
> +	if (IS_ERR(st->regmap8))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap8),
> +				     "Failed to initialize regmap8\n");
> +
> +	st->regmap16 = devm_regmap_init_spi(spi, &ad4170_regmap16_config);
> +	if (IS_ERR(st->regmap16))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap16),
> +				     "Failed to initialize regmap16\n");
> +
> +	st->regmap24 = devm_regmap_init_spi(spi, &ad4170_regmap24_config);
> +	if (IS_ERR(st->regmap24))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap24),
> +				     "Failed to initialize regmap24\n");
> +

Hmm, interesting idea... but I would expect an explanation on why can't we have bulk
reads for the 16 and 24 bit cases? Without it, I have to ask why not?
 
> +	ret = ad4170_regulator_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4170_soft_reset(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4170_parse_firmware(indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to parse firmware\n");
> +
> +	ret = ad4170_initial_config(indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to setup device\n");
> +
> +	init_completion(&st->completion);
> +
> +	if (spi->irq) {
> +		ret = ad4170_trigger_setup(indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to setup
> trigger\n");

Typically it's better to log the errors inside ad4170_trigger_setup() unless you use
it outside probe.

- Nuno Sá
> 
> +MODULE_AUTHOR("Marcelo Schmitt <marcelo.schmitt@...log.com>");
> +MODULE_DESCRIPTION("Analog Devices AD4170 SPI driver");
> +MODULE_LICENSE("GPL");


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ