Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
Date:   Wed, 04 Oct 2023 09:07:39 +0200
From:   Michael Walle <michael@walle.cc>
To:     Dumitru Ceclan <mitrutzceclan@gmail.com>
Cc:     mitrutzceclan@gmail.com, linus.walleij@linaro.org, brgl@bgdev.pl,
        andy@kernel.org, linux-gpio@vger.kernel.org,
        Lars-Peter Clausen <lars@metafoo.de>,
        Jonathan Cameron <jic23@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        Arnd Bergmann <arnd@arndb.de>,
        ChiaEn Wu <chiaen_wu@richtek.com>,
        Niklas Schnelle <schnelle@linux.ibm.com>,
        =?UTF-8?Q?Leonard_G=C3=B6hrs?= <l.goehrs@pengutronix.de>,
        Mike Looijmans <mike.looijmans@topic.nl>,
        Haibo Chen <haibo.chen@nxp.com>,
        Hugo Villeneuve <hvilleneuve@dimonoff.com>,
        Ceclan Dumitru <dumitru.ceclan@analog.com>,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] iio: adc: ad7173: add AD7173 driver
In-Reply-To: <20230928125443.615006-2-mitrutzceclan@gmail.com>
References: <20230928125443.615006-1-mitrutzceclan@gmail.com>
 <20230928125443.615006-2-mitrutzceclan@gmail.com>
Message-ID: <cc64b68545b4e178e1f5d90109f72e9a@walle.cc>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

I've just had a look at the gpio-regmap part.

> +static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int 
> base,
> +			     unsigned int offset, unsigned int *reg,
> +			     unsigned int *mask)
> +{
> +	*mask = AD7173_GPO_DATA(offset);
> +	*reg = AD7173_REG_GPIO;

*reg = base;

See also below.

> +	return 0;
> +}
> +
> +static int ad7173_gpio_init(struct iio_dev *indio_dev)
> +{
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	struct gpio_regmap_config gpio_regmap = {};
> +	struct device *dev = &st->sd.spi->dev;
> +	unsigned int mask;
> +
> +	st->regmap = devm_regmap_init_spi(st->sd.spi, &ad7173_regmap_config);
> +	if (IS_ERR(st->regmap)) {
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "Unable to init regmap\n");
> +	}
> +
> +	mask = AD7173_GPIO_OP_EN0 | AD7173_GPIO_OP_EN1 | 
> AD7173_GPIO_OP_EN2_3;
> +	regmap_update_bits(st->regmap, AD7173_REG_GPIO, mask, mask);
> +
> +	gpio_regmap.parent = dev;
> +	gpio_regmap.regmap = st->regmap;
> +	gpio_regmap.ngpio = st->info->num_gpios;
> +	gpio_regmap.reg_set_base = GPIO_REGMAP_ADDR_ZERO;

Why don't you set it to AD7173_REG_GPIO? Register 0 seems wrong, it
looks like you're using that to say this is a output-only I/O.


> +	gpio_regmap.reg_mask_xlate = ad7173_mask_xlate;
> +
> +	st->gpio_regmap = devm_gpio_regmap_register(dev, &gpio_regmap);
> +	if (IS_ERR(st->gpio_regmap)) {
> +		return dev_err_probe(dev, PTR_ERR(st->gpio_regmap),
> +				     "Unable to init gpio-regmap\n");
> +	}
> +
> +	return 0;
> +}
> +
> +#endif /* CONFIG_GPIOLIB */

Otherwise looks good. But I've noticed that the chip can actually
also do input on gpio0 and gpio1. If you ever want to support that,
it seems like you need two gpio-regmaps.

-michael