On Thu, Apr 09, 2020 at 02:58:26PM -0500, Pierre-Louis Bossart wrote:
The GPIOs are used e.g. on HifiBerry DAC+ HATs to control the LED (GPIO3) and the choice of the 44.1 (GPIO6) or 48 (GPIO3) kHz oscillator (when present).
Enable basic gpio_chip to get/set values and get/set directions. Tested with GPIO_LIB from sys/class/gpio, the LED turns on/off as desired.
One question, can this use existing GPIO infrastructure, like bgpio_init()? Ah, I see, that one operates over MMIO, while we would need something based on regmap API.
Bartosz, do we have plans to have bgpio_regmap_init() or alike?
...
+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;
+}
...
+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.
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.
- 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).
+}
...
+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));
+}
...
+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
- 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.
+}
...
+static const struct gpio_chip template_chip = {
Give better name, please. E.g. pcm512x_gpio_chip.
- .label = "pcm512x-gpio",
- .names = pcm512x_gpio_names,
- .owner = THIS_MODULE,
- .get_direction = pcm512x_gpio_get_direction,
- .direction_input = pcm512x_gpio_direction_input,
- .direction_output = pcm512x_gpio_direction_output,
- .get = pcm512x_gpio_get,
- .set = pcm512x_gpio_set,
- .base = -1, /* let gpiolib select the base */
- .ngpio = ARRAY_SIZE(pcm512x_gpio_names),
+};
...
- /* 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)
dev_err(dev, "Failed to register gpio chip: %d\n", ret);goto err;- }