+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
