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.