On Friday 17 of May 2013 14:26:25 Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 18:22 Wed 15 May , Stephen Warren wrote:
> > On 05/15/2013 06:13 PM, Tomasz Figa wrote:
> > > On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote:
> > >> Tomasz / Linus,
> > >>
> > >> On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa
> > >> <tomasz.figa@xxxxxxxxx>
> > >
> > > wrote:
> > >>> Yes. I don't like the current way too much either, duplication
> > >>> being
> > >>> one of the reasons.
> > >>
> > >> Do you have any other ideas? It sounds like Linus didn't like my
> > >> suggestion and makes some good points...
> > >
> > > I don't have anything interesting at the moment. It's a bit late now
> > > here (2 AM), so I'm going to get some sleep first.
> > >
> > > Also after reading Stephen's reply, I'm wondering if hogging
> > > wouldn't
> > > solve the problem indeed. (It might have to be fixed on
> > > pinctrl-samsung
> > > first, as last time I tried to use it, it caused some errors from
> > > pinctrl core, but haven't time to track them down, as it wasn't
> > > anything important at that time).
> >
> > One issue I noticed with the DT fragments earlier in this thread. It
> > looks like hogs in the Samsung pinctrl bingings end up looking like:
> >
> > pinctrl {
> >
> > pina {
> >
> > samsung,pins = <PIN_A PIN_B PIN_C>;
> > samsung,pin-function = <0xf>;
> > samsung,pin-pud = <0>;
> > ...
>
> I have a huge issue here that we had on at91 too
>
> we are going to have a huge numbet of node
>
> and on at91 we handle the pin the same way as samsung
> and ST have also a similiar IP
>
> so I'll prefer to reuse the AT91 DT bindings
>
> as said by Linus I just push a cleanup of the magic by using Macro
> which make it really readable now
>
> some extract of the sama5 pinctrl
>
> mmc0 {
> pinctrl_mmc0_clk_cmd_dat0: mmc0_clk_cmd_dat0 {
> atmel,pins =
> <AT91_PIOD 9 AT91_PERIPH_A
AT91_PINCTRL_NONE /* PD9 periph A MCI0_CK
> */ AT91_PIOD 0 AT91_PERIPH_A AT91_PINCTRL_PULL_UP /* PD0 periph A
> MCI0_CDA with pullup */ AT91_PIOD 1 AT91_PERIPH_A
> AT91_PINCTRL_PULL_UP>; /* PD1 periph A MCI0_DA0 with pullup */ };
> pinctrl_mmc0_dat1_3: mmc0_dat1_3 {
> atmel,pins =
> <AT91_PIOD 2 AT91_PERIPH_A
AT91_PINCTRL_PULL_UP /* PD2 periph A
> MCI0_DA1 with pullup */ AT91_PIOD 3 AT91_PERIPH_A
> AT91_PINCTRL_PULL_UP /* PD3 periph A MCI0_DA2 with pullup */ AT91_PIOD
> 4 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>; /* PD4 periph A MCI0_DA3
with
> pullup */ };
> pinctrl_mmc0_dat4_7: mmc0_dat4_7 {
> atmel,pins =
> <AT91_PIOD 5 AT91_PERIPH_A
AT91_PINCTRL_PULL_UP /* PD5 periph A
> MCI0_DA4 with pullup, conflicts with TIOA0, PWMH2 */ AT91_PIOD 6
> AT91_PERIPH_A AT91_PINCTRL_PULL_UP /* PD6 periph A MCI0_DA5 with
> pullup, conflicts with TIOB0, PWML2 */ AT91_PIOD 7 AT91_PERIPH_A
> AT91_PINCTRL_PULL_UP /* PD7 periph A MCI0_DA6 with pullup, conlicts
> with TCLK0, PWMH3 */ AT91_PIOD 8 AT91_PERIPH_A
> AT91_PINCTRL_PULL_UP>; /* PD8 periph A MCI0_DA7 with pullup,
conflicts
> with PWML3 */ };
> };
>
> of sam9g45
>
> i2c_gpio2 {
> pinctrl_i2c_gpio2: i2c_gpio2-0 {
> atmel,pins =
> <AT91_PIOB 4 AT91_PERIPH_GPIO
AT91_PINCTRL_MULTI_DRIVE /* PB4 gpio
> multidrive I2C2 data */ AT91_PIOB 5 AT91_PERIPH_GPIO
> AT91_PINCTRL_MULTI_DRIVE>; /* PB5 gpio multidrive I2C2 clock */ };
> };
>
> so we could share the c code too
OK. After thinking about it a bit more recently, I think your solution
might be fine.
However there is one thing I'm worried about. As far as I remember, when
setting a function (mux selector), pinctrl core calls pin_request() to
request all pins of the group for the device which requested the
configuration.
Now if we use hogs to set up default configuration of pins, all of them
would get requested for the pin controller and then further pin control
configuration done by device drivers would fail. This is why I wanted to
allow setting pinmux and pinconf separately, without one requiring
another.
Best regards,
Tomasz
> Best Regards,
> J,
>
> > };
> > pinp {
> >
> > samsung,pins = <PIN_P PIN_Q>;
> > samsung,pin-function = <0xe>;
> > samsung,pin-pud = <1>;
> > ...
> >
> > };
> > pinx {
> >
> > samsung,pins = <PIN_X PIN_Y PIN_Z>;
> > samsung,pin-function = <0xd>;
> > samsung,pin-pud = <2>;
> > ...
> >
> > };
> >
> > pinctrl-names = "default";
> > pinctrl-0 = <&pina &pinp &pinx>;
> >
> > };
> >
> > That pinctrl-0 property could get rather large (hard to
> > write/maintain,
> > unwieldy) if it needs to set up lots of different configurations.
> > That's why I made the equivalent Tegra bindings be:
> >
> > pinctrl {
> >
> > pins_default {
> >
> > pina {
> >
> > samsung,pins = <PIN_A PIN_B PIN_C>;
> > samsung,pin-function = <0xf>;
> > samsung,pin-pud = <0>;
> > ...
> >
> > };
> > pinp {
> >
> > samsung,pins = <PIN_P PIN_Q>;
> > samsung,pin-function = <0xe>;
> > samsung,pin-pud = <1>;
> > ...
> >
> > };
> > pinx {
> >
> > samsung,pins = <PIN_X PIN_Y PIN_Z>;
> > samsung,pin-function = <0xd>;
> > samsung,pin-pud = <2>;
> > ...
> >
> > };
> >
> > };
> >
> > pinctrl-names = "default";
> > pinctrl-0 = <&pins_default>;
> >
> > };
> >
> > The extra level within the "pinctrl configuration node"
> > ("pins_default"
> > here) makes the pinctrl-0 property a lot easier to write, and the
> > advantage happens at every use-site that needs to configure different
> > subsets of the relevant pins in different ways.
> >
> > If you're changing all the bindings anyway, introducing this extra
> > level might be something to think about.
> >
> > I did try to explain my philosophy here when we all got together to
> > design the pinctrl bindings, but I obviously didn't explain it well
> > enough, or people didn't like it anyway.
> > _______________________________________________
> > devicetree-discuss mailing list
> > devicetree-discuss@xxxxxxxxxxxxxxxx
> > https://lists.ozlabs.org/listinfo/devicetree-discuss
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
