Re: [RFC PATCH 01/16] ASoC: pcm512x: expose 6 GPIOs - Pierre-Louis Bossart

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

>> +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;
> 
>> +}

ok

> 
> ...
> 
>> +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.

did you mean use  if (ret) or drop the test altogether?

There's no standard style for regmap functions so I used what was used 
in the rest of this driver.

Mark?

> 
>> +		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.

good suggestion, thanks.

> 
>> +	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).

ok

> 
>> +}
> 
> ...
> 
>> +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));

There's a point where this becomes less readable IMHO, but fine.
The !! gives me a headache...

>> +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

ok

> 
>> +	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.

Not sure we do, will look into this.

>> +static const struct gpio_chip template_chip = {
> 
> Give better name, please. E.g. pcm512x_gpio_chip.

ok

>> +	/* 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)

ok