Re: [PATCH v4 2/2] gpio: add a reusable generic gpio_chip using regmap

[Michael Walle]

Hi,

Am 2020-05-26 19:29, schrieb Pierre-Louis Bossart:
> > +struct gpio_regmap {
> > +    struct device *parent;
> > +    struct regmap *regmap;
> > +    struct gpio_chip gpio_chip;
> > +
> > +    int reg_stride;
> > +    int ngpio_per_reg;
> > +    unsigned int reg_dat_base;
> > +    unsigned int reg_set_base;
> > +    unsigned int reg_clr_base;
> > +    unsigned int reg_dir_in_base;
> > +    unsigned int reg_dir_out_base;
>
> I wonder if a base is enough, shouldn't there be a 'last' or something
> that constrains the range of regmap addresses to be used for gpios?

This should be covered on the regmap, shouldn't it?

> related question since I couldn't figure out how to convert my PCM512x
> example, where there are 6 GPIOs configured with 3 regmap-visible
> registers [1], to this mapping.
>
> GPIO_EN defines if the GPIO is used or not (each bitfield is tied to a
> GPIO)
>
> GPIO_CONTROL_1 defines the level (each bitfield is tied to a GPIO)
>
> GPIO_OUTPUT_1+offset defines what signal is routed to each GPIO. I am
> really not sure how this part would be handled?
>
> That's 8 registers total to deal with GPIOs.

Looks like you need a custom xlate function:

int pcm512x_gpio_regmap_xlate(struct gpio_regmap *gpio, unsigned int 
base,
                              unsigned int offset, unsigned int *reg,
                              unsigned int *mask)
{
  switch (base)
  case GPIO_EN:
  case GPIO_CONTROL_1:
     *reg = base;
     *mask = (1 << offset);
     break;
  case GPIO_OUTPUT_1:
     *reg = base + offset;
     *mask = ...
     break;
}

base is always one of the xx_base properties in the "struct
gpio_regmap_config".

> > +/**
> > + * struct gpio_regmap_config - Description of a generic regmap
> > gpio_chip.
> > + *
> > + * @parent:        The parent device
> > + * @regmap:        The regmap used to access the registers
> > + *            given, the name of the device is used
> > + * @label:        (Optional) Descriptive name for GPIO controller.
> > + *            If not given, the name of the device is used.
> > + * @ngpio:        Number of GPIOs
> > + * @reg_dat_base:    (Optional) (in) register base address
> > + * @reg_set_base:    (Optional) set register base address
> > + * @reg_clr_base:    (Optional) clear register base address
> > + * @reg_dir_in_base:    (Optional) in setting register base address
> > + * @reg_dir_out_base:    (Optional) out setting register base
> > address
> > + * @reg_stride:        (Optional) May be set if the registers (of
> > the
> > + *            same type, dat, set, etc) are not consecutive.
> > + * @ngpio_per_reg:    Number of GPIOs per register
> > + * @irq_domain:        (Optional) IRQ domain if the controller is
> > + *            interrupt-capable
> > + * @reg_mask_xlate:     (Optional) Translates base address and GPIO
> > + *            offset to a register/bitmask pair. If not
> > + *            given the default gpio_regmap_simple_xlate()
> > + *            is used.
> > + *
> > + * The reg_mask_xlate translates a given base address and GPIO
> > offset to
> > + * register and mask pair. The base address is one of the given
> > reg_*_base.
> > + *
> > + * All base addresses may have the special value
> > GPIO_REGMAP_ADDR_ZERO
> > + * which forces the address to the value 0.
> > + */
> > +struct gpio_regmap_config {
> > +    struct device *parent;
> > +    struct regmap *regmap;
> > +
> > +    const char *label;
> > +    int ngpio;
>
> could we add a .names field for the gpio_chip, I found this useful for
> PCM512x GPIO support, e.g.

Sure, I have the names in the device tree.

But I'd prefer that you'd do a patch on top of this (assuming it is
applied soon), because you can actually test it and there might be
missing more.

[snip]

-michael