Am 2020-05-25 11:05, schrieb Bartosz Golaszewski:
wt., 12 maj 2020 o 16:41 Michael Walle <michael@xxxxxxxx> napisał(a):
>> +
>> +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..a868cbcde6e9
>> --- /dev/null
>> +++ b/include/linux/gpio-regmap.h
>> @@ -0,0 +1,69 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef _LINUX_GPIO_REGMAP_H
>> +#define _LINUX_GPIO_REGMAP_H
>> +
>> +struct gpio_regmap;
>> +
>> +#define GPIO_REGMAP_ADDR_ZERO ((unsigned long)(-1))
>> +#define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO)
>> +
>
> What if the addr is actually 0?
Then the driver has to set GPIO_REGMAP_ADDR_ZERO or use the
convenience
macro GPIO_REGMAP_ADDR.
So you can have
struct gpio_regmap_config config = { 0 };
config.reg_dat_base = 0x10;
config.reg_dir_out_base = 0x20;
or
config.reg_dat_base = GPIO_REGMAP_ADDR_ZERO;
or if you can't be sure if the RHS value might be zero:
config.reg_dat_base = GPIO_REGMAP_ADDR(reg);
> Maybe drop GPIO_REGMAP_ADDR and require users to set unused registers
> to GPIO_REGMAP_ADDR_ZERO?
Thats bad because:
* you'd have to set plenty of unused base registers for a simple
driver
* if there will be additional properties in the future, you have to
touch
all other drivers, because they are initialized as 0 (ie. valid
reg
0).
>> +/**
>> + * 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) out setting register base address
>> + * @reg_dir_out_base: (Optional) in setting register base address
>
> The two above are inverted I think?
good catch.
> Also: why the limitation of only supporting one at a time?
they should be exclusive, either you have a register where you set the
output bits to one, or the input bits. Maybe this need a bit more
context
above. in gpio-mmio.c you can set both and both are used in
set_direction(), but only one is read in get_direction().
That being said, I have no strong opinion wether they should be
exclusive
or not, besides the symmetry of set_/get_direction().
-michael
Sorry for the late response, your comments make sense to me. Are you
going to submit a v4 before the v5.8 merge window?
I'm currently stuck with how to handle the MFD part. Ie. Rob doesn't
seem to like the logicial device numbering - or at least there wasn't
an answer to that one anymore, see patch 5/16.
If you like I could submit this patch on its own. But then there
wouldn't be a user for it.
-michael
