linux-kernel - Re: [PATCH v2 2/2] iio: adc: ad7173: add AD7173 driver

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: <cc64b68545b4e178e1f5d90109f72e9a@walle.cc>
Date:   Wed, 04 Oct 2023 09:07:39 +0200
From:   Michael Walle <michael@...le.cc>
To:     Dumitru Ceclan <mitrutzceclan@...il.com>
Cc:     mitrutzceclan@...il.com, linus.walleij@...aro.org, brgl@...ev.pl,
        andy@...nel.org, linux-gpio@...r.kernel.org,
        Lars-Peter Clausen <lars@...afoo.de>,
        Jonathan Cameron <jic23@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        ChiaEn Wu <chiaen_wu@...htek.com>,
        Niklas Schnelle <schnelle@...ux.ibm.com>,
        Leonard Göhrs <l.goehrs@...gutronix.de>,
        Mike Looijmans <mike.looijmans@...ic.nl>,
        Haibo Chen <haibo.chen@....com>,
        Hugo Villeneuve <hvilleneuve@...onoff.com>,
        Ceclan Dumitru <dumitru.ceclan@...log.com>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: adc: ad7173: add AD7173 driver

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