| Message ID | cover.1c314f4154a6d27354625f03d0a5269eee55a9c5.1506428208.git-series.quentin.schulz@free-electrons.com |
|---|---|
| Headers | show |
| Series | add pinmuxing support for pins in AXP209 and AXP813 PMICs | expand |
On Tue, Sep 26, 2017 at 12:17:13PM +0000, Quentin Schulz wrote: > Instead of using a function to retrieve each pin's correct control > register, use drv_data within pinctrl_pin_desc to store the ctrl reg. > > Remove axp20x_gpio_get_reg and replace every occurrence by a get from > drv_data. Why do you need to do that? This should be explained. Maxime
On Tue, Sep 26, 2017 at 12:17:17PM +0000, Quentin Schulz wrote: > As pinctrl and GPIO driver now supports AXP813, add a cell for it. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > --- > drivers/mfd/axp20x.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c > index 336de66..a457528 100644 > --- a/drivers/mfd/axp20x.c > +++ b/drivers/mfd/axp20x.c > @@ -876,6 +876,9 @@ static struct mfd_cell axp813_cells[] = { > .name = "axp221-pek", > .num_resources = ARRAY_SIZE(axp803_pek_resources), > .resources = axp803_pek_resources, > + }, { > + .name = "axp20x-gpio", > + .of_compatible = "x-powers,axp813-pctl", This was probably introduced in the previous driver, but why are you using the pctl suffix? Can't we just use the GPIO one to remain consistent with the previous users and the datasheet? Thanks! Maxime
On Tue, Sep 26, 2017 at 12:17:18PM +0000, Quentin Schulz wrote: > From: Maxime Ripard <maxime.ripard@free-electrons.com> > > The AXP813 PMIC is used with some Allwinner SoCs. Create a dtsi to > include in each board embedding it. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> There must be my Signed-off-by here. Maxime
Hi Maxime, On 26/09/2017 15:01, Maxime Ripard wrote: > On Tue, Sep 26, 2017 at 12:17:13PM +0000, Quentin Schulz wrote: >> Instead of using a function to retrieve each pin's correct control >> register, use drv_data within pinctrl_pin_desc to store the ctrl reg. >> >> Remove axp20x_gpio_get_reg and replace every occurrence by a get from >> drv_data. > > Why do you need to do that? This should be explained. > Agreed that it misses an explanation. Today, to get a register addr of one of the GPIOs in the PMIC, we basically get the GPIO number and returns the register via this info. There are 3 GPIOs in AXP209, 2 in AXP813. I didn't want to have a switch case for the GPIO number and then an if/else inside one of the case to check if the device is AXP209 or AXP813 in which case we return -EINVAL instead of the GPIO2 reg. With support for new PMIC, we would have a bunch of if conditions and complexify the process for something really simple. IMHO, this also allows easier integration of future PMICs which might have different regs for the GPIOs. I don't *need* it but I find this solution nicer. Thanks, Quentin
On Tue, Sep 26, 2017 at 01:17:05PM +0000, Quentin Schulz wrote: > Hi Maxime, > > On 26/09/2017 15:01, Maxime Ripard wrote: > > On Tue, Sep 26, 2017 at 12:17:13PM +0000, Quentin Schulz wrote: > >> Instead of using a function to retrieve each pin's correct control > >> register, use drv_data within pinctrl_pin_desc to store the ctrl reg. > >> > >> Remove axp20x_gpio_get_reg and replace every occurrence by a get from > >> drv_data. > > > > Why do you need to do that? This should be explained. > > > > Agreed that it misses an explanation. > > Today, to get a register addr of one of the GPIOs in the PMIC, we > basically get the GPIO number and returns the register via this info. > > There are 3 GPIOs in AXP209, 2 in AXP813. I didn't want to have a switch > case for the GPIO number and then an if/else inside one of the case to > check if the device is AXP209 or AXP813 in which case we return -EINVAL > instead of the GPIO2 reg. With support for new PMIC, we would have a > bunch of if conditions and complexify the process for something really > simple. I'm not sure how that relates to your code actually. The only thing that patch is doing is to move the register offset from a function to the structure associated to the pin. However, even in the AXP813 case, you're using exactly the same values, so that's not really needed. Now, you also mentionned the pin number. While this patch doesn't really address it, it's also no really needed. The number of pins is already known and registered in the GPIO framework. If the framework doesn't already do it (which would be surprising), you can just check that the pin number passed is not going to be higher than the one you registered. > IMHO, this also allows easier integration of future PMICs which might > have different regs for the GPIOs. Let's worry about future PMICs in the future. Maxime
On Tue, Sep 26, 2017 at 8:17 PM, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > From: Maxime Ripard <maxime.ripard@free-electrons.com> > > The AXP813 PMIC is used with some Allwinner SoCs. Create a dtsi to > include in each board embedding it. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > --- > arch/arm/boot/dts/axp813.dtsi | 58 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 58 insertions(+) > create mode 100644 arch/arm/boot/dts/axp813.dtsi > > diff --git a/arch/arm/boot/dts/axp813.dtsi b/arch/arm/boot/dts/axp813.dtsi > new file mode 100644 > index 0000000..e7f95e8 > --- /dev/null > +++ b/arch/arm/boot/dts/axp813.dtsi > @@ -0,0 +1,58 @@ > +/* > + * Copyright 2017 Free Electrons > + * > + * Quentin Schulz <quentin.schulz@free-electrons.com> > + * > + * This file is dual-licensed: you can use it either under the terms > + * of the GPL or the X11 license, at your option. Note that this dual > + * licensing only applies to this file, and not this project as a > + * whole. > + * > + * a) This file is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of the > + * License, or (at your option) any later version. > + * > + * This file is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * Or, alternatively, > + * > + * b) Permission is hereby granted, free of charge, to any person > + * obtaining a copy of this software and associated documentation > + * files (the "Software"), to deal in the Software without > + * restriction, including without limitation the rights to use, > + * copy, modify, merge, publish, distribute, sublicense, and/or > + * sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following > + * conditions: > + * > + * The above copyright notice and this permission notice shall be > + * included in all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES > + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT > + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, > + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > + * OTHER DEALINGS IN THE SOFTWARE. > + */ > + > +/* > + * AXP813 Integrated Power Management Chip > + */ > + > +&axp813 { I'd like to name the label axp81x instead. And possibly the filename as well. See https://github.com/wens/linux/commit/05b9ca82c795816f7f2569ce96dc35b62487f89c ChenYu > + interrupt-controller; > + #interrupt-cells = <1>; > + > + axp_pctl: axp_pctl { > + compatible = "x-powers,axp813-pctl"; > + gpio-controller; > + #gpio-cells = <2>; > + }; > +}; > -- > git-series 0.9.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 26 Sep 2017, Maxime Ripard wrote: > On Tue, Sep 26, 2017 at 12:17:17PM +0000, Quentin Schulz wrote: > > As pinctrl and GPIO driver now supports AXP813, add a cell for it. > > > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > > --- > > drivers/mfd/axp20x.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c > > index 336de66..a457528 100644 > > --- a/drivers/mfd/axp20x.c > > +++ b/drivers/mfd/axp20x.c > > @@ -876,6 +876,9 @@ static struct mfd_cell axp813_cells[] = { > > .name = "axp221-pek", > > .num_resources = ARRAY_SIZE(axp803_pek_resources), > > .resources = axp803_pek_resources, > > + }, { > > + .name = "axp20x-gpio", > > + .of_compatible = "x-powers,axp813-pctl", > > This was probably introduced in the previous driver, but why are you > using the pctl suffix? Can't we just use the GPIO one to remain > consistent with the previous users and the datasheet? Right. Pinctrl is a Linuxisum. GPIO sounds more appropriate.