Re: [PATCH] iio: adc: ti-ads112c14: add gpio support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Sep 25, 2026 at 04:50:47PM -0500, David Lechner (TI) wrote:
> Add support for using the AIN4/GPIO0 to AIN7/GPIO3 pins as GPIOs when
> the gpio-controller property is present.
> 
> Pins that are already used for something else according to the
> devicetree are excluded from the valid GPIO mask. This includes analog
> inputs and excitation current outputs used by channels, REFP/REFN when
> an external reference is used, the /FAULT and /DRDY interrupts and the
> external clock input.
> 
> The per-pin register field macros are replaced with parameterized ones
> so that they can be used with the GPIO offset.

...

> +	DECLARE_BITMAP(gpio_reserved_mask, ADS112C14_NUM_GPIO);

Why not use valid_mask in GPIO chip directly?

...

> +static void ads112c14_reserve_gpio_for_ain(unsigned long *gpio_reserved_mask,
> +					   u32 ain)
> +{
> +	if (ain >= 4 && ain <= 7)
> +		set_bit(ain - 4, gpio_reserved_mask);

Why atomic op is needed?

> +}

...

> +static int ads112c14_gpio_init_valid_mask(struct gpio_chip *gc,
> +					  unsigned long *valid_mask,
> +					  unsigned int ngpios)
> +{
> +	struct iio_dev *indio_dev = gpiochip_get_data(gc);
> +	struct ads112c14_data *data = iio_priv(indio_dev);

> +	bitmap_fill(valid_mask, ngpios);

It's already filled by GPIOLIB, isn't it?

> +	bitmap_andnot(valid_mask, valid_mask, data->gpio_reserved_mask, ngpios);
> +
> +	return 0;
> +}

> +static int ads112c14_gpio_init(struct iio_dev *indio_dev)
> +{
> +	struct ads112c14_data *data = iio_priv(indio_dev);
> +	struct device *dev = indio_dev->dev.parent;
> +
> +	for (unsigned int i = 0; i < ADS112C14_NUM_GPIO; i++) {
> +		data->gpio_names[i] = devm_kasprintf(dev, GFP_KERNEL, "%s:GPIO%u",
> +						     dev_name(&indio_dev->dev), i);
> +		if (!data->gpio_names[i])
> +			return -ENOMEM;

Wondering if you can utilise devm_kasprintf_strarray().

> +	}
> +
> +	data->gc = (struct gpio_chip) {
> +		.owner = THIS_MODULE,
> +		.label = dev_name(dev),
> +		.parent = dev,
> +		.base = -1,
> +		.ngpio = ADS112C14_NUM_GPIO,
> +		.names = data->gpio_names,
> +		.can_sleep = true,
> +		.init_valid_mask = ads112c14_gpio_init_valid_mask,
> +		.get_direction = ads112c14_gpio_get_direction,
> +		.direction_input = ads112c14_gpio_direction_input,
> +		.direction_output = ads112c14_gpio_direction_output,
> +		.get = ads112c14_gpio_get,
> +		.set = ads112c14_gpio_set,
> +	};
> +
> +	return devm_gpiochip_add_data(dev, &data->gc, indio_dev);
> +}

...

>  static int ads112c14_parse_channels(struct iio_dev *indio_dev,
> -				    bool *need_avdd_ref, bool *need_ext_ref)
> +				    bool *need_avdd_ref, bool *need_ext_ref,
> +				    unsigned long *gpio_reserved_mask)
>  {

> +			ads112c14_reserve_gpio_for_ain(gpio_reserved_mask, pair[0]);

> +			ads112c14_reserve_gpio_for_ain(gpio_reserved_mask, pair[0]);
> +			ads112c14_reserve_gpio_for_ain(gpio_reserved_mask, pair[1]);
>  		} else {

> +			ads112c14_reserve_gpio_for_ain(gpio_reserved_mask, pair[0]);
> +			if (measurement->iadc_count > 1)
> +				ads112c14_reserve_gpio_for_ain(gpio_reserved_mask, pair[1]);

Can't this be done afterwards at init_valid_mask() stage?

...

>  static int ads112c14_probe(struct i2c_client *client)
>  {
> +	DECLARE_BITMAP(gpio_reserved_mask, ADS112C14_NUM_GPIO);

Hmm... But you have one already in the struct, why another one here?


...

> +	/* FAULT shares a pin with GPIO2. */
> +	if (fwnode_property_match_string(dev_fwnode(dev), "interrupt-names", "fault") >= 0)
> +		set_bit(2, gpio_reserved_mask);

Non-atomic?

...

>  	if (fwnode_property_match_string(dev_fwnode(dev), "interrupt-names", "drdy") >= 0) {
>  		data->drdy_irq = fwnode_irq_get_byname(dev_fwnode(dev), "drdy");
>  		if (data->drdy_irq < 0)
>  			return dev_err_probe(dev, data->drdy_irq,
>  					     "failed to get drdy interrupt\n");
>  
> +		/* DRDY shares a pin with GPIO3. */
> +		set_bit(3, gpio_reserved_mask);

Ditto.


>  		if (clk)
>  			return dev_err_probe(dev, -EINVAL,
>  					     "cannot use both DRDY and CLK - they share the same pin\n");

...

>  	if (clk) {
> +		/* CLK shares a pin with GPIO3. */
> +		set_bit(3, gpio_reserved_mask);

Ditto.

>  		ret = regmap_update_bits(data->regmap, ADS112C14_REG_GPIO_CFG,
> -					 ADS112C14_GPIO_CFG_GPIO3_CFG,
> -					 FIELD_PREP(ADS112C14_GPIO_CFG_GPIO3_CFG,
> +					 ADS112C14_GPIO_CFG_GPIO_CFG(3),
> +					 FIELD_PREP(ADS112C14_GPIO_CFG_GPIO_CFG(3),
>  						    ADS112C14_GPIO_CFG_GPIO_CFG_INPUT));
>  		if (ret)
>  			return ret;

...

> +	if (device_property_read_bool(dev, "gpio-controller")) {
> +		bitmap_copy(data->gpio_reserved_mask, gpio_reserved_mask,
> +			    ADS112C14_NUM_GPIO);
> +
> +		ret = ads112c14_gpio_init(indio_dev);
> +		if (ret)
> +			return ret;
> +	}

This all smells like it should be refactored to nicely have it in one place in init_valid_mask().

-- 
With Best Regards,
Andy Shevchenko