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

[Michael Walle]

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