Re: [PATCH v16 3/3] iio: adc: ad7173: add AD7173 driver

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 28 Feb 2024 13:06:20 +0200
Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel
> applications or higher speed multiplexed applications. The Sigma-Delta
> ADC is intended primarily for measurement of signals close to DC but also
> delivers outstanding performance with input bandwidths out to ~10kHz.
> 
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Reviewed-by: Michael Walle <michael@walle.cc> # for gpio-regmap
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> Link: https://lore.kernel.org/r/20240223133758.9787-3-mitrutzceclan@gmail.com
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Err. I guess you pulled this down from the tree where we got build errors.
If you do that make sure to drop the Link and sign off from next version
as those reflect the path upstream so need to be done afresh for v16.

I fixed it up.

Anyhow, I'm think there are a few cleanups that would be ideally done here
but lets do those after it's merged.

Applied to the togreg-normal branch of iio.git.

Note that I'm not yet sure if I'll be able to squeeze in another pull
request this cycle, so this may be waiting for a while.

Jonathan


> +
> +static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
> +{
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	struct device *dev = indio_dev->dev.parent;
> +	unsigned int num_channels;
> +	int ret;
> +
> +	st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
> +	st->regulators[1].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF2];
> +	st->regulators[2].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_AVDD1_AVSS];
> +
> +	/*
> +	 * If a regulator is not available, it will be set to a dummy regulator.
> +	 * Each channel reference is checked with regulator_get_voltage() before
> +	 * setting attributes so if any channel uses a dummy supply the driver
> +	 * probe will fail.
> +	 */
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
> +				      st->regulators);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get regulators\n");
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> +
> +	ret = devm_add_action_or_reset(dev, ad7173_disable_regulators, st);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to add regulators disable action\n");
> +
> +	ret = device_property_match_property_string(dev, "clock-names",
> +						    ad7173_clk_sel,
> +						    ARRAY_SIZE(ad7173_clk_sel));
> +	if (ret < 0) {
> +		st->adc_mode |= FIELD_PREP(AD7173_ADC_MODE_CLOCKSEL_MASK,
> +					   AD7173_ADC_MODE_CLOCKSEL_INT);
> +		ad7173_register_clk_provider(indio_dev);
> +	} else {
> +		st->adc_mode |= FIELD_PREP(AD7173_ADC_MODE_CLOCKSEL_MASK,
> +					   AD7173_ADC_MODE_CLOCKSEL_EXT + ret);
> +		st->ext_clk = devm_clk_get(dev, ad7173_clk_sel[ret]);
> +		if (IS_ERR(st->ext_clk))
> +			return dev_err_probe(dev, PTR_ERR(st->ext_clk),
> +					     "Failed to get external clock\n");
> +
> +		ret = clk_prepare_enable(st->ext_clk);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to enable external clock\n");
> +
> +		ret = devm_add_action_or_reset(dev, ad7173_clk_disable_unprepare,
> +					       st->ext_clk);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(dev), "rdy");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Interrupt 'rdy' is required\n");
> +
> +	ad7173_sigma_delta_info.irq_line = ret;
> +
> +	num_channels = device_get_child_node_count(dev);
> +
> +	if (st->info->has_temp)
> +		num_channels++;
> +
> +	if (num_channels == 0)
> +		return dev_err_probe(dev, -ENODATA, "No channels specified\n");
> +	indio_dev->num_channels = num_channels;
> +	st->num_channels = num_channels;

I'm not seeing benefit of duplication here really and logically it feels like
a lot of this last chunk would sit better in ad7173_fw_parse_channel_config()

Perhaps that's a job for a future tidying up patch.

> +
> +	return ad7173_fw_parse_channel_config(indio_dev);
> +}

...