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

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

On Tue, Apr 14, 2020 at 12:52:07PM -0500, Pierre-Louis Bossart wrote:

...

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

Do you see 'ret' part in my phrase above?

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

I see. May be than to drop it from the rest and do not add more?

> > > +		return ret;

...

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

You can check assembly if it gives better result in code generation.
But at least new drivers in GPIO are using it.

...

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

I didn't get you in the first part. Are you talking about struct device?
It's there, just needs to be de-referenced from GPIO chip.

-- 
With Best Regards,
Andy Shevchenko