| Message ID | cover.3ff63fdf302c6bda02ea7d160ad2aa5afee0899d.1512135804.git-series.quentin.schulz@free-electrons.com |
|---|---|
| Headers | show |
| Series | add pinmuxing support for pins in AXP209 and AXP813 PMICs | expand |
On Fri, Dec 01, 2017 at 02:44:43PM +0100, Quentin Schulz wrote: > +static void axp20x_gpio_set(struct gpio_chip *chip, unsigned offset, > + int value) > +{ checkpatch output: WARNING: Prefer 'unsigned int' to bare use of 'unsigned' > +static int axp20x_pmx_set_mux(struct pinctrl_dev *pctldev, > + unsigned int function, unsigned int group) > +{ > + struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev); > + unsigned int mask; > + > + /* Every pin supports GPIO_OUT and GPIO_IN functions */ > + if (function <= AXP20X_FUNC_GPIO_IN) > + return axp20x_pmx_set(pctldev, group, > + gpio->funcs[function].muxval); > + > + if (function == AXP20X_FUNC_LDO) > + mask = gpio->desc->ldo_mask; > + else > + mask = gpio->desc->adc_mask; What is the point of this test... > + if (!(BIT(group) & mask)) > + return -EINVAL; > + > + /* > + * We let the regulator framework handle the LDO muxing as muxing bits > + * are basically also regulators on/off bits. It's better not to enforce > + * any state of the regulator when selecting LDO mux so that we don't > + * interfere with the regulator driver. > + */ > + if (function == AXP20X_FUNC_LDO) > + return 0; ... if you know that you're not going to do anything with one of the outcomes. It would be better to just move that part above, instead of doing the same test twice. It looks good otherwise, thanks! Maxime
On Fri, Dec 01, 2017 at 02:44:47PM +0100, Quentin Schulz wrote: > To prepare for patches that will add support for a new PMIC that has a > different GPIO adc muxing value, add an adc_mux within axp20x_pctl > structure and use it. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Maxime
On Fri, Dec 01, 2017 at 02:44:51PM +0100, Quentin Schulz wrote: > On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively) > ldo_io0 and ldo_io1. > > Let's add the pinctrl properties to the said regulators. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Maxime
On Fri, Dec 1, 2017 at 2:44 PM, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > As GPIO/pinctrl driver now supports AXP813, add a cell for it. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Acked-by: Linus Walleij <linus.walleij@linaro.org> It doesn't seem to have any dependencies so I guess Lee can simply apply this separately. Yours, Linus Walleij -- 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 Fri, Dec 1, 2017 at 2:44 PM, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > This adds DT node for the GPIO/pinctrl part present in AXP813/AXP818. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> Acked-by: Linus Walleij <linus.walleij@linaro.org> Please apply this through ARM SoC. Yours, Linus Walleij -- 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 Fri, Dec 1, 2017 at 2:44 PM, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > The AXP209 and AXP813 PMICs have several pins (respectively 3 and 2) that can > be used either as GPIOs or for other purposes (ADC or LDO here). > > We already have a GPIO driver for the GPIO use of those pins on the AXP209. > Let's "upgrade" this driver to support all the functions these pins can have. > > Then we add support to this driver for the AXP813 which is slighlty different > (basically a different offset in two registers and one less pin). > > I suggest patches 1 to 8 go through Linus's tree and 9 and 10 via Maxime or > Chen-Yu's tree. > > v4: Looks overall good. As soon as Maxime is happy with everything I will happily apply 1-8 to the pinctrl tree and then pull it to GPIO as well to avoid clashes. I think there were some minor comments but it seems almost finished. Yours, Linus Walleij -- 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
Hi Maxime, On 01/12/2017 16:57, Maxime Ripard wrote: > On Fri, Dec 01, 2017 at 02:44:43PM +0100, Quentin Schulz wrote: >> +static void axp20x_gpio_set(struct gpio_chip *chip, unsigned offset, >> + int value) >> +{ > > checkpatch output: > WARNING: Prefer 'unsigned int' to bare use of 'unsigned' > >> +static int axp20x_pmx_set_mux(struct pinctrl_dev *pctldev, >> + unsigned int function, unsigned int group) >> +{ >> + struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev); >> + unsigned int mask; >> + >> + /* Every pin supports GPIO_OUT and GPIO_IN functions */ >> + if (function <= AXP20X_FUNC_GPIO_IN) >> + return axp20x_pmx_set(pctldev, group, >> + gpio->funcs[function].muxval); >> + >> + if (function == AXP20X_FUNC_LDO) >> + mask = gpio->desc->ldo_mask; >> + else >> + mask = gpio->desc->adc_mask; > > What is the point of this test... > >> + if (!(BIT(group) & mask)) >> + return -EINVAL; >> + >> + /* >> + * We let the regulator framework handle the LDO muxing as muxing bits >> + * are basically also regulators on/off bits. It's better not to enforce >> + * any state of the regulator when selecting LDO mux so that we don't >> + * interfere with the regulator driver. >> + */ >> + if (function == AXP20X_FUNC_LDO) >> + return 0; > > ... if you know that you're not going to do anything with one of the > outcomes. It would be better to just move that part above, instead of > doing the same test twice. > Return value is different. In one case, it is an error to request "ldo" for a pin that does not support it. In the other case, the ldo request is valid but nothing's done on driver side. Both cases are handled differently by the core: http://elixir.free-electrons.com/linux/latest/source/drivers/pinctrl/pinmux.c#L439 I think that's the behavior we're expecting from this driver. Or maybe you're asking to do: + if (function == AXP20X_FUNC_LDO) { + if (!(BIT(group) & gpio->desc->ldo_mask)) + return -EINVAL; + return 0; + } else if (!(BIT(group) & gpio->desc->adc_mask)) { + return -EINVAL; + } ? Thanks, Quentin
On Sat, 02 Dec 2017, Linus Walleij wrote: > On Fri, Dec 1, 2017 at 2:44 PM, Quentin Schulz > <quentin.schulz@free-electrons.com> wrote: > > > As GPIO/pinctrl driver now supports AXP813, add a cell for it. > > > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > > Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > It doesn't seem to have any dependencies so I guess Lee can simply > apply this separately. Yup! Although, I'd prefer to wait for the other patches in the set to be applied. In the mean time: For my own reference: Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
Hi, On Mon, Dec 04, 2017 at 09:07:52AM +0100, Quentin Schulz wrote: > >> +static int axp20x_pmx_set_mux(struct pinctrl_dev *pctldev, > >> + unsigned int function, unsigned int group) > >> +{ > >> + struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev); > >> + unsigned int mask; > >> + > >> + /* Every pin supports GPIO_OUT and GPIO_IN functions */ > >> + if (function <= AXP20X_FUNC_GPIO_IN) > >> + return axp20x_pmx_set(pctldev, group, > >> + gpio->funcs[function].muxval); > >> + > >> + if (function == AXP20X_FUNC_LDO) > >> + mask = gpio->desc->ldo_mask; > >> + else > >> + mask = gpio->desc->adc_mask; > > > > What is the point of this test... > > > >> + if (!(BIT(group) & mask)) > >> + return -EINVAL; > >> + > >> + /* > >> + * We let the regulator framework handle the LDO muxing as muxing bits > >> + * are basically also regulators on/off bits. It's better not to enforce > >> + * any state of the regulator when selecting LDO mux so that we don't > >> + * interfere with the regulator driver. > >> + */ > >> + if (function == AXP20X_FUNC_LDO) > >> + return 0; > > > > ... if you know that you're not going to do anything with one of the > > outcomes. It would be better to just move that part above, instead of > > doing the same test twice. > > > > Return value is different. In one case, it is an error to request "ldo" > for a pin that does not support it. In the other case, the ldo request > is valid but nothing's done on driver side. > > Both cases are handled differently by the core: > http://elixir.free-electrons.com/linux/latest/source/drivers/pinctrl/pinmux.c#L439 > > I think that's the behavior we're expecting from this driver. Ah, right. > Or maybe you're asking to do: > > + if (function == AXP20X_FUNC_LDO) { > + if (!(BIT(group) & gpio->desc->ldo_mask)) > + return -EINVAL; > + return 0; > + } else if (!(BIT(group) & gpio->desc->adc_mask)) { > + return -EINVAL; > + } > > ? No, it's definitely better the way you did it. Maxime
On Sat, Dec 02, 2017 at 05:00:03PM +0100, Linus Walleij wrote: > On Fri, Dec 1, 2017 at 2:44 PM, Quentin Schulz > <quentin.schulz@free-electrons.com> wrote: > > > The AXP209 and AXP813 PMICs have several pins (respectively 3 and 2) that can > > be used either as GPIOs or for other purposes (ADC or LDO here). > > > > We already have a GPIO driver for the GPIO use of those pins on the AXP209. > > Let's "upgrade" this driver to support all the functions these pins can have. > > > > Then we add support to this driver for the AXP813 which is slighlty different > > (basically a different offset in two registers and one less pin). > > > > I suggest patches 1 to 8 go through Linus's tree and 9 and 10 via Maxime or > > Chen-Yu's tree. > > > > v4: > > Looks overall good. As soon as Maxime is happy with everything I will > happily apply 1-8 to the pinctrl tree and then pull it to GPIO as well to > avoid clashes. > > I think there were some minor comments but it seems almost finished. You can apply everything with my Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> The only comment left is the checkpatch warning, but there's multiple occurences of that issue in the driver, so it can definitely be done in a separate patch. (But please do it Quentin ;)) Maxime
On Fri, Dec 1, 2017 at 9:44 PM, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > This adds DT node for the GPIO/pinctrl part present in AXP813/AXP818. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > --- > arch/arm/boot/dts/axp81x.dtsi | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi > index 73b761f..0ef959d 100644 > --- a/arch/arm/boot/dts/axp81x.dtsi > +++ b/arch/arm/boot/dts/axp81x.dtsi > @@ -48,6 +48,12 @@ > interrupt-controller; > #interrupt-cells = <1>; > > + axp_gpio: axp-gpio { > + compatible = "x-powers,axp813-gpio"; > + gpio-controller; > + #gpio-cells = <2>; What about interrupt-controller for directly referenced interrupts from the GPIO pins? Otherwise, Acked-by: Chen-Yu Tsai <wens@csie.org> > + }; > + > regulators { > /* Default work frequency for buck regulators */ > x-powers,dcdc-freq = <3000>; > -- > git-series 0.9.1 > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. -- 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 Fri, Dec 1, 2017 at 11:58 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Fri, Dec 01, 2017 at 02:44:51PM +0100, Quentin Schulz wrote: >> On AXP813/818, GPIO0 and GPIO1 can be used as LDO as (respectively) >> ldo_io0 and ldo_io1. >> >> Let's add the pinctrl properties to the said regulators. >> >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > > Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Acked-by: Chen-Yu Tsai <wens@csie.org> -- 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
Hi, On Tue, Dec 05, 2017 at 05:24:47PM +0800, Chen-Yu Tsai wrote: > On Fri, Dec 1, 2017 at 9:44 PM, Quentin Schulz > <quentin.schulz@free-electrons.com> wrote: > > This adds DT node for the GPIO/pinctrl part present in AXP813/AXP818. > > > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > > --- > > arch/arm/boot/dts/axp81x.dtsi | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi > > index 73b761f..0ef959d 100644 > > --- a/arch/arm/boot/dts/axp81x.dtsi > > +++ b/arch/arm/boot/dts/axp81x.dtsi > > @@ -48,6 +48,12 @@ > > interrupt-controller; > > #interrupt-cells = <1>; > > > > + axp_gpio: axp-gpio { > > + compatible = "x-powers,axp813-gpio"; > > + gpio-controller; > > + #gpio-cells = <2>; > > What about interrupt-controller for directly referenced interrupts from > the GPIO pins? There's a bit more to it to enable interrupts. You would probably need to set up a chained interrupt controller in the GPIO driver, and in the DTS with a interrupt-parent and interrupts properties pointing to the AXP device itself. > Otherwise, > > Acked-by: Chen-Yu Tsai <wens@csie.org> Applied 9 and 10, thanks! Maxime