From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
To: Michael Walle <michael@walle.cc>
Cc: linux-gpio <linux-gpio@vger.kernel.org>,
linux-devicetree <devicetree@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
linux-hwmon@vger.kernel.org, linux-pwm@vger.kernel.org,
LINUXWATCHDOG <linux-watchdog@vger.kernel.org>,
arm-soc <linux-arm-kernel@lists.infradead.org>,
"Linus Walleij" <linus.walleij@linaro.org>,
"Rob Herring" <robh+dt@kernel.org>,
"Jean Delvare" <jdelvare@suse.com>,
"Guenter Roeck" <linux@roeck-us.net>,
"Lee Jones" <lee.jones@linaro.org>,
"Thierry Reding" <thierry.reding@gmail.com>,
"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
"Wim Van Sebroeck" <wim@linux-watchdog.org>,
"Shawn Guo" <shawnguo@kernel.org>, "Li Yang" <leoyang.li@nxp.com>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Jason Cooper" <jason@lakedaemon.net>,
"Marc Zyngier" <maz@kernel.org>,
"Mark Brown" <broonie@kernel.org>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
Date: Tue, 14 Apr 2020 11:50:11 +0200 [thread overview]
Message-ID: <CAMpxmJVC7e9JnHzBo-h8M1+KmcA32=Rvxo7+znH=-kAbcCr_LQ@mail.gmail.com> (raw)
In-Reply-To: <80bd8661ec8a1f5eda3f09a267846eaa@walle.cc>
pon., 6 kwi 2020 o 12:10 Michael Walle <michael@walle.cc> 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@walle.cc> 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@walle.cc>
> >
> > 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@walle.cc>");
> >> +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?
> >
> >> +#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
next prev parent reply other threads:[~2020-04-14 9:50 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
2020-04-02 20:36 ` [PATCH v2 01/16] include/linux/ioport.h: add helper to define REG resource constructs Michael Walle
2020-04-02 20:36 ` [PATCH v2 02/16] mfd: mfd-core: Don't overwrite the dma_mask of the child device Michael Walle
2020-04-02 20:36 ` [PATCH v2 03/16] mfd: mfd-core: match device tree node against reg property Michael Walle
2020-04-02 20:36 ` [PATCH v2 04/16] regmap-irq: make it possible to add irq_chip do a specific device node Michael Walle
2020-04-14 15:37 ` Applied "regmap-irq: make it possible to add irq_chip do a specific device node" to the regmap tree Mark Brown
2020-04-14 17:12 ` [PATCH v2 04/16] regmap-irq: make it possible to add irq_chip do a specific device node Mark Brown
2020-04-02 20:36 ` [PATCH v2 05/16] dt-bindings: mfd: Add bindings for sl28cpld Michael Walle
2020-04-02 20:36 ` [PATCH v2 06/16] mfd: Add support for Kontron sl28cpld management controller Michael Walle
2020-04-02 20:36 ` [PATCH v2 07/16] irqchip: add sl28cpld interrupt controller support Michael Walle
2020-04-02 20:36 ` [PATCH v2 08/16] watchdog: add support for sl28cpld watchdog Michael Walle
2020-04-03 6:25 ` Guenter Roeck
2020-04-02 20:36 ` [PATCH v2 09/16] pwm: add support for sl28cpld PWM controller Michael Walle
2020-04-02 20:36 ` [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap Michael Walle
2020-04-06 7:47 ` Bartosz Golaszewski
2020-04-06 10:10 ` Michael Walle
2020-04-14 9:50 ` Bartosz Golaszewski [this message]
2020-04-14 10:07 ` Michael Walle
2020-04-14 17:00 ` Bartosz Golaszewski
2020-04-14 18:41 ` Michael Walle
2020-04-14 19:57 ` Michael Walle
2020-04-16 9:20 ` Linus Walleij
2020-04-16 9:34 ` Michael Walle
2020-04-14 17:21 ` Mark Brown
2020-04-14 18:36 ` Michael Walle
2020-04-14 18:39 ` Mark Brown
2020-04-16 9:27 ` Linus Walleij
2020-04-17 6:34 ` Michael Walle
2020-04-21 10:50 ` Michael Walle
2020-04-02 20:36 ` [PATCH v2 11/16] gpio: add support for the sl28cpld GPIO controller Michael Walle
2020-04-16 8:34 ` Linus Walleij
2020-04-16 8:55 ` Michael Walle
2020-04-02 20:36 ` [PATCH v2 12/16] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
2020-04-02 21:30 ` Guenter Roeck
2020-04-02 20:36 ` [PATCH v2 13/16] arm64: dts: freescale: sl28: enable sl28cpld Michael Walle
2020-04-02 20:36 ` [PATCH v2 14/16] arm64: dts: freescale: sl28: map GPIOs to input events Michael Walle
2020-04-02 20:36 ` [PATCH v2 15/16] arm64: dts: freescale: sl28: enable LED support Michael Walle
2020-04-02 20:36 ` [PATCH v2 16/16] arm64: dts: freescale: sl28: enable fan support Michael Walle
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMpxmJVC7e9JnHzBo-h8M1+KmcA32=Rvxo7+znH=-kAbcCr_LQ@mail.gmail.com' \
--to=bgolaszewski@baylibre.com \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jason@lakedaemon.net \
--cc=jdelvare@suse.com \
--cc=lee.jones@linaro.org \
--cc=leoyang.li@nxp.com \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=maz@kernel.org \
--cc=michael@walle.cc \
--cc=robh+dt@kernel.org \
--cc=shawnguo@kernel.org \
--cc=tglx@linutronix.de \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).