Currently, pinctrl-single only allows #pinctrl-cells = 1.
This series will allow pinctrl-single to also support #pinctrl-cells = 2
If "pinctrl-single,pins" has 3 arguments (offset, conf, mux) then
pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
get the value to store in the register.
To take advantage of #pinctrl-cells = 2, the AM33XX_PADCONF macro in
omap.h is modified to keep pin conf and pin mux values separate.
change log:
- v4: squash patches 2 and 3 together so that git biesct will not result
in a boot failure
- v3: change order of patches to make sure the pinctrl-single.c patch
does not break anything without the dts patches
- v2: remove outer parentheses from AM33XX_PADCONF macro as it causes a
compile error in dtc. I had added it per suggestion from checkpatch
about having parentheses around complex values.
Drew Fustini (2):
pinctrl: single: parse #pinctrl-cells = 2
ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
arch/arm/boot/dts/am33xx-l4.dtsi | 2 +-
drivers/pinctrl/pinctrl-single.c | 11 +++++++++--
include/dt-bindings/pinctrl/omap.h | 2 +-
3 files changed, 11 insertions(+), 4 deletions(-)
--
2.25.1
If "pinctrl-single,pins" has 3 arguments (offset, conf, mux), then
pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
get the value to store in the register.
Signed-off-by: Drew Fustini <[email protected]>
---
drivers/pinctrl/pinctrl-single.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index f3a8a465d27e..a436ed7762cc 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1017,10 +1017,17 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
break;
}
- /* Index plus one value cell */
offset = pinctrl_spec.args[0];
vals[found].reg = pcs->base + offset;
- vals[found].val = pinctrl_spec.args[1];
+
+ switch (pinctrl_spec.args_count) {
+ case 2:
+ vals[found].val = pinctrl_spec.args[1];
+ break;
+ case 3:
+ vals[found].val = (pinctrl_spec.args[1] | pinctrl_spec.args[2]);
+ break;
+ }
dev_dbg(pcs->dev, "%pOFn index: 0x%x value: 0x%x\n",
pinctrl_spec.np, offset, pinctrl_spec.args[1]);
--
2.25.1
* Drew Fustini <[email protected]> [200701 01:34]:
> Currently, pinctrl-single only allows #pinctrl-cells = 1.
>
> This series will allow pinctrl-single to also support #pinctrl-cells = 2
>
> If "pinctrl-single,pins" has 3 arguments (offset, conf, mux) then
> pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> get the value to store in the register.
>
> To take advantage of #pinctrl-cells = 2, the AM33XX_PADCONF macro in
> omap.h is modified to keep pin conf and pin mux values separate.
>
> change log:
> - v4: squash patches 2 and 3 together so that git biesct will not result
> in a boot failure
Thanks for updating it. Best that Linus queues both patches via
the pinctrl tree when no more comments:
Acked-by: Tony Lindgren <[email protected]>
On Wed, 1 Jul 2020 at 09:33, Drew Fustini <[email protected]> wrote:
>
> Currently, pinctrl-single only allows #pinctrl-cells = 1.
>
> This series will allow pinctrl-single to also support #pinctrl-cells = 2
>
> If "pinctrl-single,pins" has 3 arguments (offset, conf, mux) then
> pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> get the value to store in the register.
>
> To take advantage of #pinctrl-cells = 2, the AM33XX_PADCONF macro in
> omap.h is modified to keep pin conf and pin mux values separate.
>
> change log:
> - v4: squash patches 2 and 3 together so that git biesct will not result
> in a boot failure
>
> - v3: change order of patches to make sure the pinctrl-single.c patch
> does not break anything without the dts patches
>
> - v2: remove outer parentheses from AM33XX_PADCONF macro as it causes a
> compile error in dtc. I had added it per suggestion from checkpatch
> about having parentheses around complex values.
>
> Drew Fustini (2):
> pinctrl: single: parse #pinctrl-cells = 2
> ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
>
> arch/arm/boot/dts/am33xx-l4.dtsi | 2 +-
> drivers/pinctrl/pinctrl-single.c | 11 +++++++++--
> include/dt-bindings/pinctrl/omap.h | 2 +-
> 3 files changed, 11 insertions(+), 4 deletions(-)
>
> --
> 2.25.1
>
Acked-by: Haojian Zhuang <[email protected]>
On Wed, Jul 1, 2020 at 3:33 AM Drew Fustini <[email protected]> wrote:
> Currently, pinctrl-single only allows #pinctrl-cells = 1.
>
> This series will allow pinctrl-single to also support #pinctrl-cells = 2
>
> If "pinctrl-single,pins" has 3 arguments (offset, conf, mux) then
> pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> get the value to store in the register.
>
> To take advantage of #pinctrl-cells = 2, the AM33XX_PADCONF macro in
> omap.h is modified to keep pin conf and pin mux values separate.
>
> change log:
> - v4: squash patches 2 and 3 together so that git biesct will not result
> in a boot failure
>
> - v3: change order of patches to make sure the pinctrl-single.c patch
> does not break anything without the dts patches
>
> - v2: remove outer parentheses from AM33XX_PADCONF macro as it causes a
> compile error in dtc. I had added it per suggestion from checkpatch
> about having parentheses around complex values.
>
> Drew Fustini (2):
> pinctrl: single: parse #pinctrl-cells = 2
> ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
Both patches applied to the pinctrl devel branch for v5.9!
Please make sure not to create colliding patches in the DTS files merged
through ARM SoC this merge window.
Yours,
Linus Walleij
On Tue, Jul 7, 2020 at 1:01 PM Linus Walleij <[email protected]> wrote:
>
> On Wed, Jul 1, 2020 at 3:33 AM Drew Fustini <[email protected]> wrote:
>
> > Currently, pinctrl-single only allows #pinctrl-cells = 1.
> >
> > This series will allow pinctrl-single to also support #pinctrl-cells = 2
> >
> > If "pinctrl-single,pins" has 3 arguments (offset, conf, mux) then
> > pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> > get the value to store in the register.
> >
> > To take advantage of #pinctrl-cells = 2, the AM33XX_PADCONF macro in
> > omap.h is modified to keep pin conf and pin mux values separate.
> >
> > change log:
> > - v4: squash patches 2 and 3 together so that git biesct will not result
> > in a boot failure
> >
> > - v3: change order of patches to make sure the pinctrl-single.c patch
> > does not break anything without the dts patches
> >
> > - v2: remove outer parentheses from AM33XX_PADCONF macro as it causes a
> > compile error in dtc. I had added it per suggestion from checkpatch
> > about having parentheses around complex values.
> >
> > Drew Fustini (2):
> > pinctrl: single: parse #pinctrl-cells = 2
> > ARM: dts: am33xx-l4: change #pinctrl-cells from 1 to 2
>
> Both patches applied to the pinctrl devel branch for v5.9!
>
> Please make sure not to create colliding patches in the DTS files merged
> through ARM SoC this merge window.
>
> Yours,
> Linus Walleij
Thanks.
Which repo/branch is the best for me to use if I am going to be
posting any further dts patches?
-drew
On Tue, Jul 7, 2020 at 1:02 PM Drew Fustini <[email protected]> wrote:
> Which repo/branch is the best for me to use if I am going to be
> posting any further dts patches?
Mine, pinctrl devel branch during this (v5.9) cycle I suppose.
Yours,
Linus Walleij
On Tuesday, June 30, 2020 6:33:19 PM PDT Drew Fustini wrote:
> If "pinctrl-single,pins" has 3 arguments (offset, conf, mux), then
> pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> get the value to store in the register.
> - vals[found].val = pinctrl_spec.args[1];
> +
> + switch (pinctrl_spec.args_count) {
> + case 2:
> + vals[found].val = pinctrl_spec.args[1];
> + break;
> + case 3:
> + vals[found].val = (pinctrl_spec.args[1] |
pinctrl_spec.args[2]);
> + break;
> + }
>
> dev_dbg(pcs->dev, "%pOFn index: 0x%x value: 0x%x\n",
> pinctrl_spec.np, offset,
pinctrl_spec.args[1]);
If #pinctrl-cells value is greater than 2, nothing will set vals[found].val to
anything other than zero (from when it's calloc'ed) and the pinctrl will
silently be programmed to zero.
The debug printout was not change to print vals[found].val, so it will
continue to print the value of the 2nd cell.
The result is that a #pinctrl-cells of 3 will produce no warning or error,
program the pinctrl to zero, whilst at the same time emit debug log messages
that it is programming the expected values.
The device tree documentation still states that #pinctrl-cells must be 1 when
using pinctrl-single,pins. This new special case of ORing two values is not
documented.
On Tue, Sep 08, 2020 at 04:52:58PM -0700, Trent Piepho wrote:
> On Tuesday, June 30, 2020 6:33:19 PM PDT Drew Fustini wrote:
> > If "pinctrl-single,pins" has 3 arguments (offset, conf, mux), then
> > pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> > get the value to store in the register.
>
>
> > - vals[found].val = pinctrl_spec.args[1];
> > +
> > + switch (pinctrl_spec.args_count) {
> > + case 2:
> > + vals[found].val = pinctrl_spec.args[1];
> > + break;
> > + case 3:
> > + vals[found].val = (pinctrl_spec.args[1] |
> pinctrl_spec.args[2]);
> > + break;
> > + }
> >
> > dev_dbg(pcs->dev, "%pOFn index: 0x%x value: 0x%x\n",
> > pinctrl_spec.np, offset,
> pinctrl_spec.args[1]);
>
> If #pinctrl-cells value is greater than 2, nothing will set vals[found].val to
> anything other than zero (from when it's calloc'ed) and the pinctrl will
> silently be programmed to zero.
If #pinctrl-cells is 3, then it will be:
vals[found].val = (pinctrl_spec.args[1] | pinctrl_spec.args[2]);
Do you mean if #pinctrl-cells is great than 3 then it will just have a
default value of zero?
That does appear to be the case and is probably not the behavior we
want. Thank you for pointing this out. Earlier, there is a check to
make sure there are at least 2 arguments:
if (pinctrl_spec.args_count < 2) {
dev_err(pcs->dev, "invalid args_count for spec: %i\n",
pinctrl_spec.args_count);
break;
}
I'll submit a patch where the upper bound is also checked:
if (pinctrl_spec.args_count < 2 || pinctrl_spec.args_count > 3) {
dev_err(pcs->dev, "invalid args_count for spec: %i\n",
pinctrl_spec.args_count);
break;
}
> The debug printout was not change to print vals[found].val, so it will
> continue to print the value of the 2nd cell.
Thank you for pointing this out. Yes, this is an oversight and I will
submit a patch.
> The result is that a #pinctrl-cells of 3 will produce no warning or error,
> program the pinctrl to zero, whilst at the same time emit debug log messages
> that it is programming the expected values.
>
> The device tree documentation still states that #pinctrl-cells must be 1 when
> using pinctrl-single,pins. This new special case of ORing two values is not
> documented.
This is a good point, too. I will make a patch to update the
documentation.
-Drew
On Sun, Sep 13, 2020 at 09:42:33PM +0200, Drew Fustini wrote:
> On Tue, Sep 08, 2020 at 04:52:58PM -0700, Trent Piepho wrote:
> > On Tuesday, June 30, 2020 6:33:19 PM PDT Drew Fustini wrote:
> > > If "pinctrl-single,pins" has 3 arguments (offset, conf, mux), then
> > > pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> > > get the value to store in the register.
> >
> >
> > > - vals[found].val = pinctrl_spec.args[1];
> > > +
> > > + switch (pinctrl_spec.args_count) {
> > > + case 2:
> > > + vals[found].val = pinctrl_spec.args[1];
> > > + break;
> > > + case 3:
> > > + vals[found].val = (pinctrl_spec.args[1] |
> > pinctrl_spec.args[2]);
> > > + break;
> > > + }
> > >
> > > dev_dbg(pcs->dev, "%pOFn index: 0x%x value: 0x%x\n",
> > > pinctrl_spec.np, offset,
> > pinctrl_spec.args[1]);
> >
> > If #pinctrl-cells value is greater than 2, nothing will set vals[found].val to
> > anything other than zero (from when it's calloc'ed) and the pinctrl will
> > silently be programmed to zero.
>
> If #pinctrl-cells is 3, then it will be:
>
> vals[found].val = (pinctrl_spec.args[1] | pinctrl_spec.args[2]);
>
> Do you mean if #pinctrl-cells is great than 3 then it will just have a
> default value of zero?
>
> That does appear to be the case and is probably not the behavior we
> want. Thank you for pointing this out. Earlier, there is a check to
> make sure there are at least 2 arguments:
>
> if (pinctrl_spec.args_count < 2) {
> dev_err(pcs->dev, "invalid args_count for spec: %i\n",
> pinctrl_spec.args_count);
> break;
> }
>
> I'll submit a patch where the upper bound is also checked:
>
> if (pinctrl_spec.args_count < 2 || pinctrl_spec.args_count > 3) {
> dev_err(pcs->dev, "invalid args_count for spec: %i\n",
> pinctrl_spec.args_count);
> break;
> }
>
I was mistaken when I wrote the above. I was using the term
#pinctrl-cells when I should have been writing pinctrl_spec.args_count.
pinctrl_spec.args_count is 2 when #pictrl-cells is 1.
pinctrl_spec.args_count is 3 when #pictrl-cells is 2.
I have submitted patches [1][2] with fixes for the bounds check and the
dev_dbg().
> > The debug printout was not change to print vals[found].val, so it will
> > continue to print the value of the 2nd cell.
>
> Thank you for pointing this out. Yes, this is an oversight and I will
> submit a patch.
>
> > The result is that a #pinctrl-cells of 3 will produce no warning or error,
> > program the pinctrl to zero, whilst at the same time emit debug log messages
> > that it is programming the expected values.
> >
> > The device tree documentation still states that #pinctrl-cells must be 1 when
> > using pinctrl-single,pins. This new special case of ORing two values is not
> > documented.
>
> This is a good point, too. I will make a patch to update the
> documentation.
>
>
> -Drew
[1] https://lore.kernel.org/linux-omap/[email protected]/
[2] https://lore.kernel.org/linux-omap/[email protected]/