Re: [RFC PATCH 01/16] ASoC: pcm512x: expose 6 GPIOs

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: alsa-devel@alsa-project.org, tiwai@suse.de, broonie@kernel.org,
	Daniel Matuschek <daniel@hifiberry.com>,
	Matthias Reichl <hias@horus.com>,
	Hui Wang <hui.wang@canonical.com>,
	linux-gpio@vger.kernel.org,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	linux-clk@vger.kernel.org,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>
Subject: Re: [RFC PATCH 01/16] ASoC: pcm512x: expose 6 GPIOs
Date: Tue, 14 Apr 2020 20:09:34 +0300	[thread overview]
Message-ID: <20200414170934.GA34613@smile.fi.intel.com> (raw)
In-Reply-To: <20200409195841.18901-2-pierre-louis.bossart@linux.intel.com>

On Thu, Apr 09, 2020 at 02:58:26PM -0500, Pierre-Louis Bossart wrote:
> The GPIOs are used e.g. on HifiBerry DAC+ HATs to control the LED
> (GPIO3) and the choice of the 44.1 (GPIO6) or 48 (GPIO3) kHz
> oscillator (when present).
> 
> Enable basic gpio_chip to get/set values and get/set
> directions. Tested with GPIO_LIB from sys/class/gpio, the LED turns
> on/off as desired.

One question, can this use existing GPIO infrastructure, like bgpio_init()?
Ah, I see, that one operates over MMIO, while we would need something based on
regmap API.

Bartosz, do we have plans to have bgpio_regmap_init() or alike?

...

> +static int pcm512x_gpio_get_direction(struct gpio_chip *chip,
> +				      unsigned int offset)
> +{
> +	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(pcm512x->regmap, PCM512x_GPIO_EN, &val);
> +	if (ret < 0)
> +		return ret;

> +	val = (val >> offset) & 1;
> +
> +	/* val is 0 for input, 1 for output, return inverted */
> +	return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;

This better to read as simple conditional, like

	if (val & BIT(offset))
		return ..._OUT;
	return ..._IN;

> +}

...

> +static int pcm512x_gpio_direction_output(struct gpio_chip *chip,
> +					 unsigned int offset,
> +					 int value)
> +{
> +	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
> +	unsigned int reg;
> +	int ret;
> +
> +	/* select Register GPIOx output for OUTPUT_x (1..6) */
> +	reg = PCM512x_GPIO_OUTPUT_1 + offset;

> +	ret = regmap_update_bits(pcm512x->regmap, reg, 0x0f, 0x02);

Magic numbers detected.

> +	if (ret < 0)

Drop unnecessary ' < 0' parts where it makes sense, like here.

> +		return ret;
> +

> +	/* enable output x */

(1)

> +	ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN,
> +				 BIT(offset), BIT(offset));
> +	if (ret < 0)
> +		return ret;
> +
> +	/* set value */

(2)

With this (1)->(2) ordering it may be a glitch. So, first set value (if
hardware allows you, otherwise it seems like a broken one), and then switch
it to output.

> +	return regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_CONTROL_1,
> +				  BIT(offset), value << offset);

You are using many times BIT(offset) mask above, perhaps
	int mask = BIT(offset);

Also, more robust is to use ternary here: 'value ? BIT(offset) : 0'.
Rationale: think what happen with value != 1 (theoretical possibility in the
future).

> +}

...

> +static int pcm512x_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{

> +	return (val >> offset) & 1;

Don't forget to use BIT() macro.

	return !!(val & BIT(offset));

> +}

...

> +static void pcm512x_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +			     int value)
> +{
> +	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
> +	int ret;
> +
> +	ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_CONTROL_1,
> +				 BIT(offset), value << offset);

value ? BIT(offset) : 0

> +	if (ret < 0)

> +		pr_debug("%s: regmap_update_bits failed: %d\n", __func__, ret);

No __func__ in debug messages.
Use dev_dbg() when we have struct device available.

> +}

...

> +static const struct gpio_chip template_chip = {

Give better name, please. E.g. pcm512x_gpio_chip.

> +	.label			= "pcm512x-gpio",
> +	.names			= pcm512x_gpio_names,
> +	.owner			= THIS_MODULE,
> +	.get_direction		= pcm512x_gpio_get_direction,
> +	.direction_input	= pcm512x_gpio_direction_input,
> +	.direction_output	= pcm512x_gpio_direction_output,
> +	.get			= pcm512x_gpio_get,
> +	.set			= pcm512x_gpio_set,
> +	.base			= -1, /* let gpiolib select the base */
> +	.ngpio			= ARRAY_SIZE(pcm512x_gpio_names),
> +};

...

> +	/* expose 6 GPIO pins, numbered from 1 to 6 */
> +	pcm512x->chip = template_chip;
> +	pcm512x->chip.parent = dev;
> +
> +	ret = devm_gpiochip_add_data(dev, &pcm512x->chip, pcm512x);

> +	if (ret != 0) {

if (ret)

> +		dev_err(dev, "Failed to register gpio chip: %d\n", ret);
> +		goto err;
> +	}

-- 
With Best Regards,
Andy Shevchenko