Messages in this thread | | | Date | Tue, 26 May 2020 23:27:57 +0200 | From | Michael Walle <> | Subject | Re: [PATCH v4 2/2] gpio: add a reusable generic gpio_chip using regmap |
| |
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
| |