From: Sascha Hauer <s.hauer@pengutronix.de>
To: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
swarren@wwwdotorg.org, linus.walleij@linaro.org,
nicolas.ferre@atmel.com
Subject: Re: [RESEND PATCH 2/2] pinctrl: introduce complex pin description
Date: Wed, 15 Jul 2015 12:05:25 +0200 [thread overview]
Message-ID: <20150715100525.GE5161@pengutronix.de> (raw)
In-Reply-To: <20150715084542.GC14886@odux.rfo.atmel.com>
On Wed, Jul 15, 2015 at 10:45:42AM +0200, Ludovic Desroches wrote:
> On Tue, Jul 14, 2015 at 08:13:59AM +0200, Sascha Hauer wrote:
> > On Wed, Jun 10, 2015 at 05:04:57PM +0200, Ludovic Desroches wrote:
> > > Using a string to describe a pin in the device tree can be not enough.
> > > Some controllers may need extra information to fully describe a pin. It
> > > concerns mainly controllers which have a per pin muxing approach which
> > > don't fit well the notions of groups and functions.
> > > Instead of using a pin name, a 32 bit value is used. The 16 least
> > > significant bits are used for the pin number. Other 16 bits can be used to
> > > store extra parameters.
> >
> > In the Mediatek driver we use 'pinmux' as name for the property
> > containing the combined pin number / mux value defines. 'pinmux' better
> > describes what it is...
> >
>
> At the moment, I don't mix pin number and pin mux. I mix pin number and
> ioset. It allows to check that all the pins belong to the same ioset.
>
> As said previously, I didn't want to mix pin mux and pin conf in the
> same node (but it is something I can do, it's not a problem on my side).
> If I do it I will have to mux three values: pin number, pin mux value
> and pin ioset.
>
> So assuming I do this change, your advice is to add a 'pinmux' property in
> addition of 'pins' instead of trying to use it?
My advise is to not enslave your code to this ioset concept. The only
effect of introducing this concept is one single warning in the log:
dev_warn(&pdev->dev,
"/!\\ pins from group %s are not using the same ioset /!\\\n",
group->name);
There are *no* decisions made in the driver upon the ioset, only the
above warning is printed.
I can easily find examples in which a device needs some functional pins
and some additional GPIOs, be it for card detection or something else,
and this *will* work, regardless of which ioset the pins are in. Why
should a ioset concept limit me in this way that everything works except
I have to suppress or ignore the warning in the log or split the pinmux
node up to two different nodes?
The only thing that won't work (or at least you don't want to guarantee
that it works), is to mix functional pins from the upper left corner of
the SoC with pins from the lower right corner to form a single MMC/SD
controller. Yes, you shouldn't do that. I may or may not work, but that
is nothing this particular SoC introduces, it's like this on many other
SoCs, perhaps even including earlier Atmel SoCs. Still, me as the guy
writing board support have to support the way the hardware is designed,
and if a board designer chooses to use pins from different iosets for a
single device I'll support it that way, no matter if a warning is shown
or not. If the users of this board are annoyed by the above warning
you'll probably receive a patch dropping it soon. Then the ioset concept
has completely vanished, but still the device trees are written like
that.
So, yes, my advice is to drop the ioset concept completely. If you still
insist on it then you can encode the ioset number in the pinmux define.
Only the lower 16bit are defined as the pin number, you can use the
upper 16bit like you want. You can encode the muxing in bits 16-23 and the
ioset in bits 24-31.
> > > + if (pctldesc->complex_pin_desc && pins_prop) {
> > > + of_property_for_each_u32(np, subnode_target_type, prop, cur, val) {
> > > + pin_id = val & PINCTRL_PIN_MASK;
> > > + items_name[i++] = pctldesc->pins[pin_id].name;
> > > + }
> >
> > I don't like that even though pins have numbers here they are converted
> > to strings which the driver later has to search in a list just to
> > convert it back into the number. This is quite inefficient.
> >
> > I guess this could be optimized later, but it would be nice to have the
> > pin number directly in the driver.
>
> I know that is something you don't like but, at the moment, I need a string for
> pinctrl_utils_add_map_mux and pinctrl_utils_add_map_configs.
Yeah, I am fine with it. This can be fixed later. I am more concerned
about the device tree bindings than about the actual implementation. The
implementation can far more easy be changed than the bindings.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
WARNING: multiple messages have this Message-ID (diff)
From: s.hauer@pengutronix.de (Sascha Hauer)
To: linux-arm-kernel@lists.infradead.org
Subject: [RESEND PATCH 2/2] pinctrl: introduce complex pin description
Date: Wed, 15 Jul 2015 12:05:25 +0200 [thread overview]
Message-ID: <20150715100525.GE5161@pengutronix.de> (raw)
In-Reply-To: <20150715084542.GC14886@odux.rfo.atmel.com>
On Wed, Jul 15, 2015 at 10:45:42AM +0200, Ludovic Desroches wrote:
> On Tue, Jul 14, 2015 at 08:13:59AM +0200, Sascha Hauer wrote:
> > On Wed, Jun 10, 2015 at 05:04:57PM +0200, Ludovic Desroches wrote:
> > > Using a string to describe a pin in the device tree can be not enough.
> > > Some controllers may need extra information to fully describe a pin. It
> > > concerns mainly controllers which have a per pin muxing approach which
> > > don't fit well the notions of groups and functions.
> > > Instead of using a pin name, a 32 bit value is used. The 16 least
> > > significant bits are used for the pin number. Other 16 bits can be used to
> > > store extra parameters.
> >
> > In the Mediatek driver we use 'pinmux' as name for the property
> > containing the combined pin number / mux value defines. 'pinmux' better
> > describes what it is...
> >
>
> At the moment, I don't mix pin number and pin mux. I mix pin number and
> ioset. It allows to check that all the pins belong to the same ioset.
>
> As said previously, I didn't want to mix pin mux and pin conf in the
> same node (but it is something I can do, it's not a problem on my side).
> If I do it I will have to mux three values: pin number, pin mux value
> and pin ioset.
>
> So assuming I do this change, your advice is to add a 'pinmux' property in
> addition of 'pins' instead of trying to use it?
My advise is to not enslave your code to this ioset concept. The only
effect of introducing this concept is one single warning in the log:
dev_warn(&pdev->dev,
"/!\\ pins from group %s are not using the same ioset /!\\\n",
group->name);
There are *no* decisions made in the driver upon the ioset, only the
above warning is printed.
I can easily find examples in which a device needs some functional pins
and some additional GPIOs, be it for card detection or something else,
and this *will* work, regardless of which ioset the pins are in. Why
should a ioset concept limit me in this way that everything works except
I have to suppress or ignore the warning in the log or split the pinmux
node up to two different nodes?
The only thing that won't work (or at least you don't want to guarantee
that it works), is to mix functional pins from the upper left corner of
the SoC with pins from the lower right corner to form a single MMC/SD
controller. Yes, you shouldn't do that. I may or may not work, but that
is nothing this particular SoC introduces, it's like this on many other
SoCs, perhaps even including earlier Atmel SoCs. Still, me as the guy
writing board support have to support the way the hardware is designed,
and if a board designer chooses to use pins from different iosets for a
single device I'll support it that way, no matter if a warning is shown
or not. If the users of this board are annoyed by the above warning
you'll probably receive a patch dropping it soon. Then the ioset concept
has completely vanished, but still the device trees are written like
that.
So, yes, my advice is to drop the ioset concept completely. If you still
insist on it then you can encode the ioset number in the pinmux define.
Only the lower 16bit are defined as the pin number, you can use the
upper 16bit like you want. You can encode the muxing in bits 16-23 and the
ioset in bits 24-31.
> > > + if (pctldesc->complex_pin_desc && pins_prop) {
> > > + of_property_for_each_u32(np, subnode_target_type, prop, cur, val) {
> > > + pin_id = val & PINCTRL_PIN_MASK;
> > > + items_name[i++] = pctldesc->pins[pin_id].name;
> > > + }
> >
> > I don't like that even though pins have numbers here they are converted
> > to strings which the driver later has to search in a list just to
> > convert it back into the number. This is quite inefficient.
> >
> > I guess this could be optimized later, but it would be nice to have the
> > pin number directly in the driver.
>
> I know that is something you don't like but, at the moment, I need a string for
> pinctrl_utils_add_map_mux and pinctrl_utils_add_map_configs.
Yeah, I am fine with it. This can be fixed later. I am more concerned
about the device tree bindings than about the actual implementation. The
implementation can far more easy be changed than the bindings.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
next prev parent reply other threads:[~2015-07-15 10:05 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-10 15:04 [RESEND PATCH 0/2] get pinctrl more flexible for per pin muxing controllers Ludovic Desroches
2015-06-10 15:04 ` Ludovic Desroches
2015-06-10 15:04 ` Ludovic Desroches
2015-06-10 15:04 ` [RESEND PATCH 1/2] pinctrl: change function behavior " Ludovic Desroches
2015-06-10 15:04 ` Ludovic Desroches
2015-06-10 15:04 ` Ludovic Desroches
2015-06-15 15:58 ` Stephen Warren
2015-06-15 15:58 ` Stephen Warren
2015-06-15 15:58 ` Stephen Warren
2015-06-17 12:38 ` Ludovic Desroches
2015-06-17 12:38 ` Ludovic Desroches
2015-06-17 12:38 ` Ludovic Desroches
2015-06-17 15:55 ` Stephen Warren
2015-06-17 15:55 ` Stephen Warren
2015-06-18 12:33 ` Ludovic Desroches
2015-06-18 12:33 ` Ludovic Desroches
2015-06-18 12:33 ` Ludovic Desroches
2015-07-14 5:57 ` Sascha Hauer
2015-07-14 5:57 ` Sascha Hauer
2015-07-15 7:46 ` Ludovic Desroches
2015-07-15 7:46 ` Ludovic Desroches
2015-07-15 7:46 ` Ludovic Desroches
2015-07-15 8:29 ` Ludovic Desroches
2015-07-15 8:29 ` Ludovic Desroches
2015-07-15 8:29 ` Ludovic Desroches
2015-07-27 9:43 ` Linus Walleij
2015-07-27 9:43 ` Linus Walleij
2015-07-27 12:12 ` Ludovic Desroches
2015-07-27 12:12 ` Ludovic Desroches
2015-06-30 9:17 ` Nicolas Ferre
2015-06-30 9:17 ` Nicolas Ferre
2015-06-30 9:17 ` Nicolas Ferre
2015-07-13 12:07 ` Linus Walleij
2015-07-13 12:07 ` Linus Walleij
2015-07-13 12:07 ` Linus Walleij
2015-07-14 6:54 ` Sascha Hauer
2015-07-14 6:54 ` Sascha Hauer
2015-07-13 12:13 ` Linus Walleij
2015-07-13 12:13 ` Linus Walleij
2015-06-10 15:04 ` [RESEND PATCH 2/2] pinctrl: introduce complex pin description Ludovic Desroches
2015-06-10 15:04 ` Ludovic Desroches
2015-06-10 15:04 ` Ludovic Desroches
2015-06-15 16:01 ` Stephen Warren
2015-06-15 16:01 ` Stephen Warren
2015-06-15 16:01 ` Stephen Warren
2015-06-17 12:42 ` Ludovic Desroches
2015-06-17 12:42 ` Ludovic Desroches
2015-06-17 12:42 ` Ludovic Desroches
2015-07-14 6:13 ` Sascha Hauer
2015-07-14 6:13 ` Sascha Hauer
2015-07-15 8:45 ` Ludovic Desroches
2015-07-15 8:45 ` Ludovic Desroches
2015-07-15 8:45 ` Ludovic Desroches
2015-07-15 10:05 ` Sascha Hauer [this message]
2015-07-15 10:05 ` Sascha Hauer
2015-07-15 13:52 ` Ludovic Desroches
2015-07-15 13:52 ` Ludovic Desroches
2015-07-15 13:52 ` Ludovic Desroches
2015-06-10 15:04 ` [RESEND PROTO] pinctrl: rough draft for a future controller Ludovic Desroches
2015-06-10 15:04 ` Ludovic Desroches
2015-06-10 15:04 ` Ludovic Desroches
2015-06-10 15:04 ` [RESEND PROTO] ARM: at91/dt: proto dt Ludovic Desroches
2015-06-10 15:04 ` Ludovic Desroches
2015-06-10 15:04 ` Ludovic Desroches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150715100525.GE5161@pengutronix.de \
--to=s.hauer@pengutronix.de \
--cc=devicetree@vger.kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nicolas.ferre@atmel.com \
--cc=swarren@wwwdotorg.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.