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;
> + }
--
With Best Regards,
Andy Shevchenko
