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

[Michael Walle <michael@walle.cc>]

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