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:

quoted

On Tuesday, June 30, 2020 6:33:19 PM PDT Drew Fustini wrote:

quoted

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.

quoted

-		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]);

quoted

+			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().

quoted

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.

quoted

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/20200913231557.2063071-1-drew@beagleboard.org/ (local)
[2] https://lore.kernel.org/linux-omap/20200913230306.2061645-1-drew@beagleboard.org/ (local)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel