Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 14 Sep 2020 01:27:11 +0200
From:   Drew Fustini <drew@beagleboard.org>
To:     Trent Piepho <tpiepho@gmail.com>
Cc:     linux-arm-kernel@lists.infradead.org,
        Tony Lindgren <tony@atomide.com>,
        Rob Herring <robh+dt@kernel.org>, linux-omap@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
        Linus Walleij <linus.walleij@linaro.org>,
        Haojian Zhuang <haojian.zhuang@linaro.org>,
        devicetree@vger.kernel.org, bcousson@baylibre.com,
        Jason Kridner <jkridner@beagleboard.org>,
        Robert Nelson <robertcnelson@gmail.com>
Subject: Re: [PATCH v4 1/2] pinctrl: single: parse #pinctrl-cells = 2
Message-ID: <20200913232711.GA2065644@x1>
References: <20200701013320.130441-1-drew@beagleboard.org>
 <20200701013320.130441-2-drew@beagleboard.org>
 <3139716.CMS8C0sQ7x@zen.local>
 <20200913194233.GA1955808@x1>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200913194233.GA1955808@x1>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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