Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap

From: Michael Walle
Date: Tue Apr 14 2020 - 06:07:43 EST

Am 2020-04-14 11:50, schrieb Bartosz Golaszewski:
pon., 6 kwi 2020 o 12:10 Michael Walle <michael@xxxxxxxx> napisaÅ(a):


Hi Bartosz, Hi Mark Brown,

Am 2020-04-06 09:47, schrieb Bartosz Golaszewski:
> czw., 2 kwi 2020 o 22:37 Michael Walle <michael@xxxxxxxx> napisaÅ(a):
>>
>> There are quite a lot simple GPIO controller which are using regmap to
>> access the hardware. This driver tries to be a base to unify existing
>> code into one place. This won't cover everything but it should be a
>> good
>> starting point.
>>
>> It does not implement its own irq_chip because there is already a
>> generic one for regmap based devices. Instead, the irq_chip will be
>> instanciated in the parent driver and its irq domain will be associate
>> to this driver.
>>
>> For now it consists of the usual registers, like set (and an optional
>> clear) data register, an input register and direction registers.
>> Out-of-the-box, it supports consecutive register mappings and mappings
>> where the registers have gaps between them with a linear mapping
>> between
>> GPIO offset and bit position. For weirder mappings the user can
>> register
>> its own .xlate().
>>
>> Signed-off-by: Michael Walle <michael@xxxxxxxx>
>
> Hi Michael,
>
> Thanks for doing this! When looking at other generic drivers:
> gpio-mmio and gpio-reg I can see there are some corner-cases and more
> specific configuration options we could add

I didn't want to copy every bit without being able to test it.


Sure, I didn't mean we need to do it now - just set it as the future goal.

> but it's not a blocker,
> we'll probably be extending this one as we convert more drivers to
> using it.

correct, that was also my plan.

> Personally I'd love to see gpio-mmio and gpio-reg removed
> and replaced by a single, generic regmap interface eventually.

agreed.



[snip!]

>> +
>> +/**
>> + * gpio_regmap_simple_xlate() - translate base/offset to reg/mask
>> + *
>> + * Use a simple linear mapping to translate the offset to the
>> bitmask.
>> + */
>> +int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int
>> base,
>> + unsigned int offset,
>> + unsigned int *reg, unsigned int *mask)
>> +{
>> + unsigned int line = offset % gpio->ngpio_per_reg;
>> + unsigned int stride = offset / gpio->ngpio_per_reg;
>> +
>> + *reg = base + stride * gpio->reg_stride;
>> + *mask = BIT(line);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(gpio_regmap_simple_xlate);
>
> Why does this need to be exported?

Mh, the idea was that a user could also set this xlate() by himself (for
whatever reason). But since it is the default, it is not really
necessary.
That being said, I don't care if its only local to this module.


Let's only export symbols that have external users then.

[snip!]

>> +
>> +MODULE_AUTHOR("Michael Walle <michael@xxxxxxxx>");
>> +MODULE_DESCRIPTION("GPIO generic regmap driver core");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/gpio-regmap.h b/include/linux/gpio-regmap.h
>> new file mode 100644
>> index 000000000000..ad63955e0e43
>> --- /dev/null
>> +++ b/include/linux/gpio-regmap.h
>> @@ -0,0 +1,88 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef _LINUX_GPIO_REGMAP_H
>> +#define _LINUX_GPIO_REGMAP_H
>> +
>> +struct gpio_regmap_addr {
>> + unsigned int addr;
>> + bool valid;
>> +};
>
> I'm not quite sure what the meaning behind the valid field here is.
> When would we potentially set it to false?

Some base addresses are optional, but on the other hand, a base address
of 0 could also be valid. So I cannot use 0 as an indicator whether a
base address is set or not. The generic mmio driver has some special
case for the ack base, where there is a use_ack flag which forces to
use the ack register even if its zero. So I've had a look at the kernel
if there is a better idiom for that, but I haven't found anything.

So the best from a user perspective I've could come up with was:

->base_reg = GPIO_REGMAP_ADDR(addr);

I'm open for suggestions.


Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in
IS_ERR() returning true?

Unfortunatly, its not a pointer, but only a regular unsigned int (ie
the type the regmap API has for its "reg" property). It could be a
pointer of course but then the user would have to allocate additional
memory.

-michael


>
>> +#define GPIO_REGMAP_ADDR(_addr) \
>> + ((struct gpio_regmap_addr) { .addr = _addr, .valid = true })
>> +
>> +/**
>> + * struct gpio_regmap - Description of a generic regmap gpio_chip.
>> + *
>> + * @parent: The parent device
>> + * @regmap: The regmap use to access the registers
>
> s/use/used/
>
>> + * 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) out setting register base address
>> + * @reg_dir_out_base: (Optional) in 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.
>> + * @to_irq: (Optional) Maps GPIO offset to a irq number.
>> + * By default assumes a linear mapping of the
>> + * given irq_domain.
>> + * @driver_data: Pointer to the drivers private data. Not used
>> by
>> + * gpio-regmap.
>> + *
>> + * 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.
>> + */
>> +struct gpio_regmap {
>
> I'd prefer to follow a pattern seen in other such APIs of calling this
> structure gpio_regmap_config and creating another private structure
> called gpio_regmap used in callbacks that would only contain necessary
> fields.

something like the following?

struct gpio_regmap *gpio_regmap_register(struct gpio_regmap_config *)

but if that structure is private, how can a callback access individual
elements? Or do you mean private in "local to the gpio drivers"?


Either making the structure local to drivers/gpio or making it
entirely opaque and providing accessor functions. Depending on how
much of the structure one may want to access.

Also I was unsure about the naming, eg. some use
stuff_register()/stuff_unregister() and some stuff_add()/stuff_remove().


register/unregister is fine with me.

Bart