Message-ID: <CAA+hA=Qa_3vXvgNMRuDjw93Wb3fGNOM1EqmvYVQO5CN9w2FA3A@mail.gmail.com>
Date:	Fri, 10 Feb 2012 16:33:38 -0800
From:	Dong Aisheng <dongas86@...il.com>
To:	Tony Lindgren <tony@...mide.com>
Cc:	linux-omap@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	Stephen Warren <swarren@...dia.com>,
	Linus Walleij <linus.walleij@...ricsson.com>,
	Barry Song <21cnbao@...il.com>,
	Haojian Zhuang <haojian.zhuang@...vell.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Thomas Abraham <thomas.abraham@...aro.org>,
	Rajendra Nayak <rajendra.nayak@...aro.org>,
	Dong Aisheng <dong.aisheng@...aro.org>,
	Shawn Guo <shawn.guo@...escale.com>
Subject: Re: [PATCH 2/2] pinctrl: Add simple pinmux driver using device tree data

Hi Tony,

On Fri, Feb 10, 2012 at 12:05:03PM -0800, Tony Lindgren wrote:
> Hi Dong,
>
> * Dong Aisheng <dongas86@...il.com> [120207 17:22]:
> > On 2/4/12, Tony Lindgren <tony@...mide.com> wrote:
> >
> > The most difference may be the function enable due to hw difference.
> > But i see that for DT case, it seems function and group creation may
> > also be a problem.
>
> What all do you see as a problem with group creation? Just the
> naming?
I said from different SoC's pointer of view.
Not only naming, but also if group and function created in driver or dt file
and their map parsing.

> > > + sprintf(name, "%lx",
> > > +         (unsigned long)smux->res->start + offset);
> > > + pin->name = name;
> > I'm wondering how about other people do not want the reg address to be PIN name?
> > It's less meaningful.
>
> How about try adding optional pinctrl-simple,pin-name entry that you
> can add to each mux entry?
>
Put it in pinctrl device node?
Then how do we name each pin?
And for IMX, currently we name all pins in driver.
I still can not find a good reason that i should name all pins in dt file.
Yes, we indeed have such a case.
For IMX, some special pins mux still need a setting called 'select input' which
is controlled in other registers.
But a rough idea in my mind that may choose different way to fix this issue.
It's a little like:
pinctrl_usdhc4: pinconfig-usdhc4 {
       mux =
               <MX6Q_SD4_CMD  0 SELECT_INPUT>
               <MX6Q_SD4_CLK  0 0>
               <MX6Q_SD4_DAT0 1 0>
               <MX6Q_SD4_DAT1 1 0>
               <MX6Q_SD4_DAT2 1 SELECT_INPUT>
               <MX6Q_SD4_DAT3 1 0>
               <MX6Q_SD4_DAT4 1 0>
               <MX6Q_SD4_DAT5 1 0>
               <MX6Q_SD4_DAT6 1 0>
               <MX6Q_SD4_DAT7 1 0>;
}
This format would not make the dts writer to take too much care of
register address
and value. For this case, the #pinmux-cells would be <3>;
It is a little different from OMAP.

For your proposal, I'm afraid it is a little too much depend on the SoC register
layout. We need to find a flexible enough way to cover all possible
register layout
difference for all SoCs.
(Considering one register controls multi muxs?)

> Note that for more complex mapping you may want to add a hardware
> specific .data entry that corresponds to the compatible flag, let's
> say for conf range offset to mux range offset.
>
> > > +         res = smux_rename_function(function, np->full_name);
> > A little unclear for rename here.
> > Can we find a better way?
>
> You might want to play with parsing optional names from .dts file.
> I don't need the names and they slow down the parsing. For the names,
> we can show them with userspace tools using debugfs.
>
> For reference, this is what I get under debugfs function entry
> after the rename:
>
> function: /ocp/serial@...8020000, groups = [ pinconfig-uart3 ]
>
The name looks reasonable to me.

> > > +static int __init smux_parse_one_pinctrl_entry(struct smux_device *smux,
> > > +                                           struct device_node *np)
> > > +{
> > > +   int count = 0;
> > > +
> > > +   do {
>
> ...
>
> > > + } while (++count);
> > This 'while' is for what? Define multi pinctrl properties?
>
> Yes each client device might request multiple muxes, let's say
> two pingroups from two different pinctrl driver instances.
>
You mean like this?
serial@...20000 {
      pinctrl = <&pmx_uart3_x>;
      pinctrl = <&pmx_uart3_y>;
};

Did i misunderstand?
I still can not understand why need this.
The pinctrl properly in device node can support multi pinmuxs .
serial@...20000 {
      pinctrl = <&pmx_uart3_x &pmx_uart3_y>;
It's good to me that the consensus we reached at Linaro Connect is much like
my proposal in above URL. :)

> > > + val = of_get_property(pdev->dev.of_node,
> > > +                         "pinctrl-simple,function-off", &len);
> > > + if (!val || len != 4) {
> > > +         dev_err(smux->dev, "function off mode not specified\n");
> > > +         ret = -EINVAL;
> > How about other SoCs not support function off mode?
>
> Maybe disable should not do anything if function-off is not specified?
>
IIRC currently pinctrl must need a disable function, if that, yes.
However i think we may change it in the future that make .disable function
optinal.

> > > +free:
> > > + devm_kfree(smux->dev, smux);
> > > +
> > For devm_* routines, do you still need this error checking?
> > IIRC, the resource will be automatically released if probe failed.
>
> I guess, are there some recommendations somewhere for that?
I don't know where it is.
I just checked the code before.
You can see really_probe in drivers/base/dd.c.

Regards
Dong Aisheng
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists