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
