Re: [PATCH 2/5] iio: adc: Add ti-ads1262 driver

[Krzysztof Kozlowski]

On Fri, Jun 12, 2026 at 05:46:20PM -0500, Kurt Borja wrote:
> +
> +	val = FIELD_PREP(ADS1262_IDACMAG_MAG1_MASK, idac_mags[0]);
> +	val |= FIELD_PREP(ADS1262_IDACMAG_MAG2_MASK, idac_mags[1]);
> +
> +	return regmap_update_bits(st->regmap, ADS1262_IDACMAG_REG,
> +				  ADS1262_IDACMAG_MAG1_MASK |
> +				  ADS1262_IDACMAG_MAG2_MASK, val);
> +}
> +
> +static int ads1262_gpio_setup(struct ads1262 *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	struct gpio_desc *gpiod;
> +	const char *con_id;
> +
> +	con_id = "start";

Nope, see further

> +	gpiod = devm_gpiod_get_optional(dev, con_id, GPIOD_OUT_LOW);
> +	if (IS_ERR(gpiod))
> +		return dev_err_probe(dev, PTR_ERR(gpiod),
> +				     "Failed to get %s GPIO\n", con_id);
> +	st->start_gpiod = gpiod;
> +
> +	con_id = "reset";

Nope

> +	gpiod = devm_gpiod_get_optional(dev, con_id, GPIOD_OUT_HIGH);
> +	if (IS_ERR(gpiod))
> +		return dev_err_probe(dev, PTR_ERR(gpiod),
> +				     "Failed to get %s GPIO\n", con_id);
> +	st->reset_gpiod = gpiod;
> +
> +	return 0;
> +}
> +
> +static int ads1262_clk_setup(struct ads1262 *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	struct clk *clk;
> +	int ret;
> +
> +	clk = devm_clk_get_optional_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk),
> +				     "Failed to get external clock\n");
> +
> +	/*
> +	 * The nominal clock frequency as indicated by the datasheet is
> +	 * 7372800.
> +	 */
> +	ret = clk_set_rate(clk, 7372800);
> +	if (ret)
> +		return dev_err_probe(dev, PTR_ERR(clk),
> +				     "Failed to set the nominal clock frequency.\n");
> +
> +	return 0;
> +}
> +
> +static int ads1262_regulator_setup(struct ads1262 *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	const char *reg_id, *prop;
> +	u32 mux[2] = {};
> +	int val, ret;
> +
> +	reg_id = "dvdd";
> +	ret = devm_regulator_get_enable(dev, reg_id);
> +	if (ret)
> +		goto err_regulator_get;
> +
> +	reg_id = "avdd";

Nope.

> +	ret = devm_regulator_get_enable(dev, reg_id);
> +	if (ret)
> +		goto err_regulator_get;
> +
> +	prop = "ti,neg-refmux";
> +	device_property_read_u32(dev, prop, &mux[0]);
> +	if (mux[0] >= ADS1262_RMUX_COUNT)
> +		return dev_err_probe(dev, -ENXIO, " %s out of range\n", prop);
> +
> +	prop = "ti,pos-refmux";

Don't do such syntax. You make git grep unnecesssary difficult.

> +	device_property_read_u32(dev, prop, &mux[1]);

And this shows in `git grep` as completely pointless code.

> +	if (mux[1] >= ADS1262_RMUX_COUNT)
> +		return dev_err_probe(dev, -ENXIO, " %s out of range\n", prop);
> +
> +	if (mux[0] == ADS1262_RMUX_INTER && mux[1] == ADS1262_RMUX_INTER) {
> +		/* The internal voltage reference is 2.5 V */
> +		st->vref_uV = 2500000;
> +		return 0;
> +	}
> +
> +	val = FIELD_PREP(ADS1262_REFMUX_RMUXN_MASK, mux[0]);
> +	val |= FIELD_PREP(ADS1262_REFMUX_RMUXP_MASK, mux[1]);
> +	ret = regmap_update_bits(st->regmap, ADS1262_REFMUX_REG,
> +				 ADS1262_REFMUX_RMUXN_MASK |
> +				 ADS1262_REFMUX_RMUXP_MASK, val);
> +	if (ret)
> +		return ret;
> +
> +	reg_id = "vref";

Nope.

> +	st->vref_uV = devm_regulator_get_enable_read_voltage(dev, reg_id);
> +	if (st->vref_uV < 0)
> +		goto err_regulator_get;
> +
> +	return 0;
> +
> +err_regulator_get:
> +	return dev_err_probe(dev, ret, "Failed to get regulator %s\n", reg_id);
> +}
> +

Functions used by probe() should be before probe(), not somewhere in the
middle of the code. IOW, entire probe is together.
...

> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static int ads1262_runtime_suspend(struct device *dev)
> +{
> +	struct ads1262 *st = dev_get_drvdata(dev);
> +
> +	if (!st->reset_gpiod)
> +		return 0;
> +
> +	regcache_cache_only(st->regmap, true);
> +
> +	return ads1262_dev_power_off(st);
> +}
> +
> +static int ads1262_runtime_resume(struct device *dev)
> +{
> +	struct ads1262 *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!st->reset_gpiod)
> +		return 0;
> +
> +	ret = ads1262_dev_power_on(st);
> +	if (ret)
> +		return ret;
> +
> +	regcache_cache_only(st->regmap, false);
> +	regcache_mark_dirty(st->regmap);
> +
> +	return regcache_sync(st->regmap);
> +}
> +
> +DEFINE_RUNTIME_DEV_PM_OPS(ads1262_runtime_pm, ads1262_runtime_suspend,
> +			  ads1262_runtime_resume, NULL);
> +
> +static const struct of_device_id ads1262_of_match[] = {
> +	{ .compatible = "ti,ads1262" },
> +	{ .compatible = "ti,ads1263" },

So devices are fully compatible? Then it should be expressed in the
binding and drop one entry here.

Best regards,
Krzysztof