[RFC] pinctrl driver for Zynq

[RFC] pinctrl driver for Zynq

[Steffen Trumtrar]

Hi!

On Wed, Sep 24, 2014 at 02:09:14PM -0700, S?ren Brinkmann wrote:
> Hi,
> 
> I think I have pinctrl driver that is covering the pinmux options of
> Zynq and I also figured out how the DT bindings work.
> 
> But there are a couple of things that probably could be done better.
> 
> One thing making the DT bindings explode, seems to be all those single
> pin functions that can be muxed to every pin.
> Next to GPIO, this applies to SD card and write protect - which are even
> present twice since Zynq has two SDIO cores. Just these functions
> account for a couple of hundred nodes in the DT and a bunch of lines in
> the driver. Is there a better way to do this?
> 
> In particular for GPIO there seemed to be a better solution with
> implementing gpio_request_enable(), but that seemed to allow GPIO in
> parallel to request and mux the pin which does not work on Zynq. IOW: I
> expected the core would reject a call of gpio_request_enable for a pin
> that is already muxed to some other function, but that was not the case
> in my testing. Am I missing something here?
> 
> And finally, for SD card detect and write protect, we actually have to
> disable the muxing. The problem with those functions is, that they have
> a dedicated mux for that function which is in parallel to the "normal"
> pinmuxes. So, muxing a "normal" function to a pin would not void the
> muxing of the SD signals. I thought this would be easily resolved by
> implementing the 'disable' op, but after I did that, I noticed that
> there is only a stale documentation comment of this member of struct
> pinmux_ops left, the actual function pointer is gone.
> 
> 	Thanks,
> 	S?ren
> 
> ------------8<-----------------8<-------------------8<--------------8<----------
> Date: Mon, 15 Sep 2014 17:24:35 -0700
> Subject: [PATCH RFC] pinctrl: Add driver for Zynq
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
>  arch/arm/boot/dts/zynq-7000.dtsi | 3039 +++++++++++++++++++++++++++++++++++++-
>  arch/arm/boot/dts/zynq-zc706.dts |   13 +
>  arch/arm/mach-zynq/Kconfig       |    1 +
>  drivers/pinctrl/Kconfig          |    8 +
>  drivers/pinctrl/Makefile         |    1 +
>  drivers/pinctrl/pinctrl-zynq.c   |  927 ++++++++++++
>  6 files changed, 3988 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pinctrl/pinctrl-zynq.c
> 
> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> index 6cc83d4c6c76..814873da0392 100644
> --- a/arch/arm/boot/dts/zynq-7000.dtsi
> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> @@ -229,7 +229,7 @@
>  		slcr: slcr at f8000000 {
>  			#address-cells = <1>;
>  			#size-cells = <1>;
> -			compatible = "xlnx,zynq-slcr", "syscon";
> +			compatible = "xlnx,zynq-slcr", "syscon", "simple-bus";
>  			reg = <0xF8000000 0x1000>;
>  			ranges;
>  			clkc: clkc at 100 {
> @@ -250,6 +250,3043 @@
>  						"dbg_trc", "dbg_apb";
>  				reg = <0x100 0x100>;
>  			};
> +
> +			pinctrl0: pinctrl at 700 {
> +				compatible = "xlnx,pinctrl-zynq";
> +				reg = <0x700 0x200>;
> +
> +				pinctrl_i2c0_0: pinctrl-i2c0 at 0 {
> +					i2c0-mux {
> +						function = "i2c0";
> +						pins = "i2c0_0_grp";
> +					};
> +				};
> +

(...)

> +				pinctrl_sdio1_cd_54: pinctrl-sdio1_cd at 54 {
> +					sdio1_cd-mux {
> +						function = "sdio1_cd";
> +						pins = "sdio1_emio_cd_grp";
> +					};
> +				};
> +			};
>  		};
>  

Wouldn't this reaaally bloat the dtb?
I know that imx decided to remove all the pinctrls from the dtsis, because
the dtbs got to big.

Regards,
Steffen

-- 
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 |

[RFC] pinctrl driver for Zynq

[Sören Brinkmann]

On Thu, 2014-09-25 at 10:17AM +0200, Steffen Trumtrar wrote:
> Hi!
> 
> On Wed, Sep 24, 2014 at 02:09:14PM -0700, S?ren Brinkmann wrote:
> > Hi,
> > 
> > I think I have pinctrl driver that is covering the pinmux options of
> > Zynq and I also figured out how the DT bindings work.
> > 
> > But there are a couple of things that probably could be done better.
> > 
> > One thing making the DT bindings explode, seems to be all those single
> > pin functions that can be muxed to every pin.
> > Next to GPIO, this applies to SD card and write protect - which are even
> > present twice since Zynq has two SDIO cores. Just these functions
> > account for a couple of hundred nodes in the DT and a bunch of lines in
> > the driver. Is there a better way to do this?
> > 
> > In particular for GPIO there seemed to be a better solution with
> > implementing gpio_request_enable(), but that seemed to allow GPIO in
> > parallel to request and mux the pin which does not work on Zynq. IOW: I
> > expected the core would reject a call of gpio_request_enable for a pin
> > that is already muxed to some other function, but that was not the case
> > in my testing. Am I missing something here?
> > 
> > And finally, for SD card detect and write protect, we actually have to
> > disable the muxing. The problem with those functions is, that they have
> > a dedicated mux for that function which is in parallel to the "normal"
> > pinmuxes. So, muxing a "normal" function to a pin would not void the
> > muxing of the SD signals. I thought this would be easily resolved by
> > implementing the 'disable' op, but after I did that, I noticed that
> > there is only a stale documentation comment of this member of struct
> > pinmux_ops left, the actual function pointer is gone.
> > 
> > 	Thanks,
> > 	S?ren
> > 
> > ------------8<-----------------8<-------------------8<--------------8<----------
> > Date: Mon, 15 Sep 2014 17:24:35 -0700
> > Subject: [PATCH RFC] pinctrl: Add driver for Zynq
> > 
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > ---
> >  arch/arm/boot/dts/zynq-7000.dtsi | 3039 +++++++++++++++++++++++++++++++++++++-
> >  arch/arm/boot/dts/zynq-zc706.dts |   13 +
> >  arch/arm/mach-zynq/Kconfig       |    1 +
> >  drivers/pinctrl/Kconfig          |    8 +
> >  drivers/pinctrl/Makefile         |    1 +
> >  drivers/pinctrl/pinctrl-zynq.c   |  927 ++++++++++++
> >  6 files changed, 3988 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/pinctrl/pinctrl-zynq.c
> > 
> > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> > index 6cc83d4c6c76..814873da0392 100644
> > --- a/arch/arm/boot/dts/zynq-7000.dtsi
> > +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> > @@ -229,7 +229,7 @@
> >  		slcr: slcr at f8000000 {
> >  			#address-cells = <1>;
> >  			#size-cells = <1>;
> > -			compatible = "xlnx,zynq-slcr", "syscon";
> > +			compatible = "xlnx,zynq-slcr", "syscon", "simple-bus";
> >  			reg = <0xF8000000 0x1000>;
> >  			ranges;
> >  			clkc: clkc at 100 {
> > @@ -250,6 +250,3043 @@
> >  						"dbg_trc", "dbg_apb";
> >  				reg = <0x100 0x100>;
> >  			};
> > +
> > +			pinctrl0: pinctrl at 700 {
> > +				compatible = "xlnx,pinctrl-zynq";
> > +				reg = <0x700 0x200>;
> > +
> > +				pinctrl_i2c0_0: pinctrl-i2c0 at 0 {
> > +					i2c0-mux {
> > +						function = "i2c0";
> > +						pins = "i2c0_0_grp";
> > +					};
> > +				};
> > +
> 
> (...)
> 
> > +				pinctrl_sdio1_cd_54: pinctrl-sdio1_cd at 54 {
> > +					sdio1_cd-mux {
> > +						function = "sdio1_cd";
> > +						pins = "sdio1_emio_cd_grp";
> > +					};
> > +				};
> > +			};
> >  		};
> >  
> 
> Wouldn't this reaaally bloat the dtb?
> I know that imx decided to remove all the pinctrls from the dtsis, because
> the dtbs got to big.

Yep, it absolutely does and I don't really know what to do about it.
Listing them all is a lot. Not listing them all would force board DTs to
potentially duplicate such nodes. This is definitely one of the things
I'm seeking input on.
Is there any good solution or best practice?

	S?ren

[RFC] pinctrl driver for Zynq

[Linus Walleij]

On Thu, Sep 25, 2014 at 6:02 PM, S?ren Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> On Thu, 2014-09-25 at 10:17AM +0200, Steffen Trumtrar wrote:
>> On Wed, Sep 24, 2014 at 02:09:14PM -0700, S?ren Brinkmann wrote:

>> (...)
>>
>> > +                           pinctrl_sdio1_cd_54: pinctrl-sdio1_cd at 54 {
>> > +                                   sdio1_cd-mux {
>> > +                                           function = "sdio1_cd";
>> > +                                           pins = "sdio1_emio_cd_grp";
>> > +                                   };
>> > +                           };
>> > +                   };
>> >             };
>> >
>>
>> Wouldn't this reaaally bloat the dtb?
>> I know that imx decided to remove all the pinctrls from the dtsis, because
>> the dtbs got to big.
>
> Yep, it absolutely does and I don't really know what to do about it.
> Listing them all is a lot. Not listing them all would force board DTs to
> potentially duplicate such nodes. This is definitely one of the things
> I'm seeking input on.
> Is there any good solution or best practice?

What about the middle road just putting those that are actually
used in some DTS file into the DTSI?

Else I vote for duplicating them in each DTS.

Yours,
Linus Walleij

[RFC] pinctrl driver for Zynq

[Linus Walleij]

On Wed, Sep 24, 2014 at 11:09 PM, S?ren Brinkmann
<soren.brinkmann@xilinx.com> wrote:

> I think I have pinctrl driver that is covering the pinmux options of
> Zynq and I also figured out how the DT bindings work.

OK let's look....

> One thing making the DT bindings explode, seems to be all those single
> pin functions that can be muxed to every pin.

The purpose of the device tree is to describe configuration of the
hardware, not to describe all possibilities the hardware has, just those
deployed on a specific system. I think this is the problem, will
look more into detail below...

> Next to GPIO, this applies to SD card and write protect - which are even
> present twice since Zynq has two SDIO cores. Just these functions
> account for a couple of hundred nodes in the DT and a bunch of lines in
> the driver. Is there a better way to do this?

Probably :-)

> In particular for GPIO there seemed to be a better solution with
> implementing gpio_request_enable(), but that seemed to allow GPIO in
> parallel to request and mux the pin which does not work on Zynq.

Just that it becomes possible to use GPIO and another function in
parallel doesn't mean you have to use this possibility. I think this
is the case with some drivers as of now.

I know it has this unfulfilledness about it...

> IOW: I
> expected the core would reject a call of gpio_request_enable for a pin
> that is already muxed to some other function, but that was not the case
> in my testing. Am I missing something here?

There are systems where a certain function and GPIO can be used
in parallel, so GPIO can "spy" on a pin used by another function.
I think there was something like a flag line from some hardware
that was used by the HW block, but sometimes other parts of
the system needed to know the state of that line and it was not
in the registers for that hardware, but it could be read by using this
dual-mode property of the GPIO.

> And finally, for SD card detect and write protect, we actually have to
> disable the muxing. The problem with those functions is, that they have
> a dedicated mux for that function which is in parallel to the "normal"
> pinmuxes. So, muxing a "normal" function to a pin would not void the
> muxing of the SD signals.

This should be solved internally in the pin control driver, as it should
handle hardware pecularities and abstract it to the framework.

Do not rely on the framework to provide convenient quirk hooks
for hardware oddities.

> I thought this would be easily resolved by
> implementing the 'disable' op, but after I did that, I noticed that
> there is only a stale documentation comment of this member of struct
> pinmux_ops left, the actual function pointer is gone.

This has been removed since it was bogus and is being cleaned
up and the vtable entry .enable() has been renamed .set_mux().

> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> index 6cc83d4c6c76..814873da0392 100644
> --- a/arch/arm/boot/dts/zynq-7000.dtsi
> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> @@ -229,7 +229,7 @@
>                 slcr: slcr at f8000000 {
>                         #address-cells = <1>;
>                         #size-cells = <1>;
> -                       compatible = "xlnx,zynq-slcr", "syscon";
> +                       compatible = "xlnx,zynq-slcr", "syscon", "simple-bus";
>                         reg = <0xF8000000 0x1000>;
>                         ranges;
>                         clkc: clkc at 100 {
> @@ -250,6 +250,3043 @@
>                                                 "dbg_trc", "dbg_apb";
>                                 reg = <0x100 0x100>;
>                         };
> +
> +                       pinctrl0: pinctrl at 700 {
> +                               compatible = "xlnx,pinctrl-zynq";
> +                               reg = <0x700 0x200>;
> +
> +                               pinctrl_i2c0_0: pinctrl-i2c0 at 0 {

So is this a state?

> +                                       i2c0-mux {
> +                                               function = "i2c0";
> +                                               pins = "i2c0_0_grp";

This I know we discussed in person. I have renamed the latter
string from "pins" to "groups" which is as you can see way more
appropriate. See this patch which is queued for v3.18:

http://marc.info/?l=devicetree&m=141223584006648&w=2

I'm also working on converting the Nomadik driver to this and
provide central DT parsing in the pin control core.

> +                               pinctrl_i2c0_1: pinctrl-i2c0 at 1 {
> +                                       i2c0-mux {
> +                                               function = "i2c0";
> +                                               pins = "i2c0_1_grp";
> +                                       };
> +                               };
> +
> +                               pinctrl_i2c0_2: pinctrl-i2c0 at 2 {
> +                                       i2c0-mux {
> +                                               function = "i2c0";
> +                                               pins = "i2c0_2_grp";
> +                                       };
> +                               };

My main concern here is whether all these states are really
in use, or if you're busy outlining everything the controller
can do. Just include the bits you actually need for the
specific system this device tree is for.

> +                               pinctrl_i2c1_10: pinctrl-i2c1 at 10 {
> +                                       i2c1-mux {
> +                                               function = "i2c1";
> +                                               pins = "i2c1_10_grp";
> +                                       };
> +                               };
> +
> +                               pinctrl_gem0_0: pinctrl-gem0 at 0 {
> +                                       gem0-mux {
> +                                               function = "ethernet0";
> +                                               pins = "ethernet0_0_grp";
> +                                       };
> +                               };

This doesn't add up. You opened up
pinctrl_i2c0_0: pinctrl-i2c0 at 0 { ... a bit up.

Now you're putting gem0 and what not into a i2c0's node?

Something is seriously wrong here and I don't know where
it's gone wrong.

> +                               pinctrl_gpio0_0: pinctrl-gpio0 at 0 {
> +                                       gpio0-mux {
> +                                               function = "gpio0";
> +                                               pins = "gpio0_0_grp";
> +                                       };
> +                               };

Yeah that's sort of hairy to maintain isn't it?
Go for the quick helper function .gpio_request_enable() and
skip this. Live with the fact that pins may be used in
parallel or figure out a way to patch the framework to prevent
it.

I once considered add a bool no_dual_mode_gpio; to
pinctrl_desc and enforce isolation between the two things.

> diff --git a/arch/arm/boot/dts/zynq-zc706.dts b/arch/arm/boot/dts/zynq-zc706.dts
> index 4cc9913078cd..1ae9bcaee252 100644
> --- a/arch/arm/boot/dts/zynq-zc706.dts
> +++ b/arch/arm/boot/dts/zynq-zc706.dts
> @@ -33,11 +33,20 @@
>  &gem0 {

I'm not familiar with this syntax of putting an ampersand in front
of a node like that. What does that mean? To me ampersands
are phandles :-/

>         status = "okay";
>         phy-mode = "rgmii";
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_gem0_0>, <&pinctrl_mdio0_0>;

This looks right, and maybe you should put in only these
nodes that you actually use below the pin controller?

>  &i2c0 {
>         status = "okay";
>         clock-frequency = <400000>;
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_i2c0_10>;

So why do we need to defined pinctrl_i2c0_0 ...
pinctrl_i2c0_N?

> +++ b/arch/arm/mach-zynq/Kconfig
> @@ -9,6 +9,7 @@ config ARCH_ZYNQ
>         select HAVE_ARM_TWD if SMP
>         select ICST
>         select MFD_SYSCON
> +       select PINCTRL

select PINCTRL_ZYNQ?

Usually these things seem to be hard to live without.

> +++ b/drivers/pinctrl/Kconfig
> @@ -305,6 +305,14 @@ config PINCTRL_PALMAS
>           open drain configuration for the Palmas series devices like
>           TPS65913, TPS80036 etc.
>
> +config PINCTRL_ZYNQ
> +       bool "Pinctrl driver for Xilinx Zynq"
> +       depends on ARCH_ZYNQ
> +       select PINMUX
> +       select GENERIC_PINCONF

Thanks for using generic pinconf. But you haven't implemented
it yet?!

> +++ b/drivers/pinctrl/pinctrl-zynq.c
(...)
> +#define ZYNQ_PINMUX_MUX_SHIFT  1
> +#define ZYNQ_PINMUX_MUX_MASK   (0x7f << ZYNQ_PINMUX_MUX_SHIFT)

I would just like ...
#define ZYNQ_PINMUX_MUX_MASK 0xfe

> +/**
> + * struct zynq_pinmux_function - a pinmux function
> + * @name:    Name of the pinmux function.
> + * @groups:  List of pingroups for this function.
> + * @ngroups: Number of entries in @groups.
> + */
> +struct zynq_pinmux_function {
> +       const char *name;
> +       const char * const *groups;
> +       unsigned int ngroups;
> +       unsigned int mux_val;
> +       u32 mux;
> +       u32 mux_mask;
> +       u8 mux_shift;
> +};

I agree: it is a good thing to document. But kerneldoc all of
it!

> +enum zynq_pinmux_functions {
> +       ZYNQ_PMUX_ethernet0,
> +       ZYNQ_PMUX_ethernet1,
> +       ZYNQ_PMUX_mdio0,
> +       ZYNQ_PMUX_mdio1,
> +       ZYNQ_PMUX_qspi0,
> +       ZYNQ_PMUX_qspi1,
> +       ZYNQ_PMUX_qspi_fbclk,
> +       ZYNQ_PMUX_qspi_cs1,
> +       ZYNQ_PMUX_spi0,
> +       ZYNQ_PMUX_spi1,
> +       ZYNQ_PMUX_sdio0,
> +       ZYNQ_PMUX_sdio0_pc,
> +       ZYNQ_PMUX_sdio0_cd,
> +       ZYNQ_PMUX_sdio0_wp,
> +       ZYNQ_PMUX_sdio1,
> +       ZYNQ_PMUX_sdio1_pc,
> +       ZYNQ_PMUX_sdio1_cd,
> +       ZYNQ_PMUX_sdio1_wp,
> +       ZYNQ_PMUX_smc0_nor,
> +       ZYNQ_PMUX_smc0_nor_cs1,
> +       ZYNQ_PMUX_smc0_nor_addr25,
> +       ZYNQ_PMUX_smc0_nand,
> +       ZYNQ_PMUX_can0,
> +       ZYNQ_PMUX_can1,
> +       ZYNQ_PMUX_uart0,
> +       ZYNQ_PMUX_uart1,
> +       ZYNQ_PMUX_i2c0,
> +       ZYNQ_PMUX_i2c1,
> +       ZYNQ_PMUX_ttc0,
> +       ZYNQ_PMUX_ttc1,
> +       ZYNQ_PMUX_swdt0,
> +       ZYNQ_PMUX_gpio0,
> +       ZYNQ_PMUX_MAX_FUNC
> +};

This looks entirely reasonable. Those are the functions we
really need to control on this system.

> +const struct pinctrl_pin_desc zynq_pins[] = {
> +      PINCTRL_PIN(0,  "MIO0"),
(...)
> +      PINCTRL_PIN(53, "MIO53"),
> +      PINCTRL_PIN(54, "EMIO_SD0_WP"),
> +      PINCTRL_PIN(55, "EMIO_SD0_CD"),
> +      PINCTRL_PIN(56, "EMIO_SD1_WP"),
> +      PINCTRL_PIN(57, "EMIO_SD0_CD"),
> +};

Looks good.

> +/* pin groups */
> +static const unsigned int ethernet0_0_pins[] = {16, 17, 18, 19, 20, 21, 22, 23, 24,
> +                                           25, 26, 27};
> +static const unsigned int ethernet1_0_pins[] = {28, 29, 30, 31, 32, 33, 34, 35, 36,
> +                                               37, 38, 39};
(...)
> +static const unsigned int sdio1_3_pins[] = {46, 47, 48, 49, 40, 51};

Looks good.

> +static const unsigned int sdio0_emio_wp_pins[] = {54};
> +static const unsigned int sdio0_emio_cd_pins[] = {55};
> +static const unsigned int sdio1_emio_wp_pins[] = {56};
> +static const unsigned int sdio1_emio_cd_pins[] = {57};

So these are unique functions, not just a way to use GPIO?

(...)
> +static const unsigned int swdt0_4_pins[] = {52, 53};

Looks good until here.

> +static const unsigned int gpio0_0_pins[] = {0};
(...)
> +static const unsigned int gpio0_53_pins[] = {53};

Looks very tiresome to maintain. What about using the
shortcut?

(...)
> +struct zynq_pctrl_group zynq_pctrl_groups[] = {
> +       DEFINE_ZYNQ_PINCTRL_GRP(ethernet0_0),
> +       DEFINE_ZYNQ_PINCTRL_GRP(ethernet1_0),
(...)
> +       DEFINE_ZYNQ_PINCTRL_GRP(swdt0_4),

Generally looks good.

> +/* function groups */
> +static const char * const ethernet0_groups[] = {"ethernet0_0_grp"};
> +static const char * const ethernet1_groups[] = {"ethernet1_0_grp"};
> +static const char * const mdio0_groups[] = {"mdio0_0_grp"};
> +static const char * const mdio1_groups[] = {"mdio1_0_grp"};
> +static const char * const qspi0_groups[] = {"qspi0_0_grp"};
> +static const char * const qspi1_groups[] = {"qspi0_1_grp"};
> +static const char * const qspi_fbclk_groups[] = {"qspi_fbclk_grp"};
> +static const char * const qspi_cs1_groups[] = {"qspi_cs1_grp"};
> +static const char * const spi0_groups[] =
> +                               {"spi0_0_grp", "spi0_1_grp", "spi0_2_grp"};
> +static const char * const spi1_groups[] =
> +               {"spi1_0_grp", "spi1_1_grp", "spi1_2_grp", "spi1_3_grp"};

Looks good too, OK these functions can be muxed on these groups...

> +static const char * const sdio0_pc_groups[] = {"gpio0_0_grp",
> +               "gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
> +               "gpio0_8_grp", "gpio0_10_grp", "gpio0_12_grp",
> +               "gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
> +               "gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
> +               "gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
> +               "gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
> +               "gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
> +               "gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
> +               "gpio0_50_grp", "gpio0_52_grp"};
> +static const char * const sdio1_pc_groups[] = {"gpio0_1_grp",
> +               "gpio0_3_grp", "gpio0_5_grp", "gpio0_7_grp",
> +               "gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
> +               "gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
> +               "gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
> +               "gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
> +               "gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
> +               "gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
> +               "gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
> +               "gpio0_51_grp", "gpio0_53_grp"};
> +static const char * const sdio0_cd_groups[] = {"gpio0_0_grp",
> +               "gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
> +               "gpio0_10_grp", "gpio0_12_grp",
> +               "gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
> +               "gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
> +               "gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
> +               "gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
> +               "gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
> +               "gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
> +               "gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
> +               "gpio0_3_grp", "gpio0_5_grp",
> +               "gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
> +               "gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
> +               "gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
> +               "gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
> +               "gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
> +               "gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
> +               "gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
> +               "gpio0_51_grp", "gpio0_53_grp", "sdio0_emio_cd_grp"};
> +static const char * const sdio0_wp_groups[] = {"gpio0_0_grp",
> +               "gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
> +               "gpio0_10_grp", "gpio0_12_grp",
> +               "gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
> +               "gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
> +               "gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
> +               "gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
> +               "gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
> +               "gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
> +               "gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
> +               "gpio0_3_grp", "gpio0_5_grp",
> +               "gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
> +               "gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
> +               "gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
> +               "gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
> +               "gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
> +               "gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
> +               "gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
> +               "gpio0_51_grp", "gpio0_53_grp", "sdio0_emio_wp_grp"};
> +static const char * const sdio1_cd_groups[] = {"gpio0_0_grp",
> +               "gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
> +               "gpio0_10_grp", "gpio0_12_grp",
> +               "gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
> +               "gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
> +               "gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
> +               "gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
> +               "gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
> +               "gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
> +               "gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
> +               "gpio0_3_grp", "gpio0_5_grp",
> +               "gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
> +               "gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
> +               "gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
> +               "gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
> +               "gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
> +               "gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
> +               "gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
> +               "gpio0_51_grp", "gpio0_53_grp", "sdio1_emio_cd_grp"};
> +static const char * const sdio1_wp_groups[] = {"gpio0_0_grp",
> +               "gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
> +               "gpio0_10_grp", "gpio0_12_grp",
> +               "gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
> +               "gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
> +               "gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
> +               "gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
> +               "gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
> +               "gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
> +               "gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
> +               "gpio0_3_grp", "gpio0_5_grp",
> +               "gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
> +               "gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
> +               "gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
> +               "gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
> +               "gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
> +               "gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
> +               "gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
> +               "gpio0_51_grp", "gpio0_53_grp", "sdio1_emio_wp_grp"};

That's like, making your life hard. Use the shortcut.

> +#define DEFINE_ZYNQ_PINMUX_FUNCTION(fname, mval)       \
> +       [ZYNQ_PMUX_##fname] = {                         \
> +               .name = #fname,                         \
> +               .groups = fname##_groups,               \
> +               .ngroups = ARRAY_SIZE(fname##_groups),  \
> +               .mux_val = mval                         \
> +       }
> +
> +#define DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(fname, mval, mux, mask, shift) \
> +       [ZYNQ_PMUX_##fname] = {                         \
> +               .name = #fname,                         \
> +               .groups = fname##_groups,               \
> +               .ngroups = ARRAY_SIZE(fname##_groups),  \
> +               .mux_val = mval,                        \
> +               .mux_mask = mask,                       \
> +               .mux_shift = shift                      \
> +       }
> +
> +#define ZYNQ_SDIO_WP_SHIFT     0
> +#define ZYNQ_SDIO_WP_MASK      (0x3f << ZYNQ_SDIO_WP_SHIFT)

Defining something and shifting it 0 positions seem a bit surplus.

> +#define ZYNQ_SDIO_CD_SHIFT     16
> +#define ZYNQ_SDIO_CD_MASK      (0x3f << ZYNQ_SDIO_CD_SHIFT)

This conveys something about the hardware though.

> +static const struct zynq_pinmux_function zynq_pmux_functions[] = {
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(ethernet0, 1),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(ethernet1, 1),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(mdio0, 0x40),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(mdio1, 0x50),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(qspi0, 1),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(qspi1, 1),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(qspi_fbclk, 1),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(qspi_cs1, 1),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(spi0, 0x50),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(spi1, 0x50),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(sdio0, 0x40),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(sdio0_pc, 0xc),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(sdio0_wp, 0, 130, ZYNQ_SDIO_WP_MASK,
> +                                       ZYNQ_SDIO_WP_SHIFT),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(sdio0_cd, 0, 130, ZYNQ_SDIO_CD_MASK,
> +                                       ZYNQ_SDIO_CD_SHIFT),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(sdio1, 0x40),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(sdio1_pc, 0xc),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(sdio1_wp, 0, 134, ZYNQ_SDIO_WP_MASK,
> +                                       ZYNQ_SDIO_WP_SHIFT),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(sdio1_cd, 0, 134, ZYNQ_SDIO_CD_MASK,
> +                                       ZYNQ_SDIO_CD_SHIFT),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(smc0_nor, 4),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(smc0_nor_cs1, 8),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(smc0_nor_addr25, 4),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(smc0_nand, 8),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(can0, 0x10),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(can1, 0x10),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(uart0, 0x70),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(uart1, 0x70),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(i2c0, 0x20),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(i2c1, 0x20),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(ttc0, 0x60),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(ttc1, 0x60),
> +       DEFINE_ZYNQ_PINMUX_FUNCTION(swdt0, 0x30),

Looks good.

> +static int zynq_pinmux_enable(struct pinctrl_dev *pctldev,
> +                             unsigned function,
> +                             unsigned group)

Rename zynq_pinmux_set() when rebasing to v3.18-rc1 (when
it is tagged, or use the pinctrl tree "devel" branch).

> +static const struct pinmux_ops zynq_pinmux_ops = {
> +       .get_functions_count = zynq_pmux_get_functions_count,
> +       .get_function_name = zynq_pmux_get_function_name,
> +       .get_function_groups = zynq_pmux_get_function_groups,
> +       .enable = zynq_pinmux_enable,

Renamed .set_mux() in the current codebase.

> +static struct pinctrl_desc zynq_desc = {
> +       .name = "zynq_pinctrl",
> +       .pins = zynq_pins,
> +       .npins = ARRAY_SIZE(zynq_pins),
> +       .pctlops = &zynq_pctrl_ops,
> +       .pmxops = &zynq_pinmux_ops,
> +       //const struct pinconf_ops *confops;

Delete it instead of commenting out. Patch it in later when
you patch in the pinconf support. It's fine to begin with only
muxing...

> +static int zynq_pinctrl_probe(struct platform_device *pdev)
> +
> +{
> +       struct resource *res;
> +       struct device_node *slcr;

Usually we just call this *np (node pointer) please rename the variable.

> +       struct zynq_pinctrl *pctrl;

Call it zpctrl or something more unique.

> +       pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
> +       if (!pctrl)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +       slcr = of_get_parent(pdev->dev.of_node);
> +       if (slcr->data) {
> +               pctrl->regs = (__force void __iomem *)slcr->data + res->start;

This looks weird. Use DT parsing functions and accessors, no funny
business like this. The res-start to the device should be the real
physical address, not a relative base with offset, dunno what happened
here but it is wrong.

Yours,
Linus Walleij

[RFC] pinctrl driver for Zynq

[Michal Simek]

Hi Linus,

Soren will reply it I believe just some explanation from me.

>> diff --git a/arch/arm/boot/dts/zynq-zc706.dts b/arch/arm/boot/dts/zynq-zc706.dts
>> index 4cc9913078cd..1ae9bcaee252 100644
>> --- a/arch/arm/boot/dts/zynq-zc706.dts
>> +++ b/arch/arm/boot/dts/zynq-zc706.dts
>> @@ -33,11 +33,20 @@
>>  &gem0 {
> 
> I'm not familiar with this syntax of putting an ampersand in front
> of a node like that. What does that mean? To me ampersands
> are phandles :-/

It just uses node label as reference and add some properties to it.
It means gem0 node is in dtsi and in this dts we want to extend it
by board specific information.

...

>> +       pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
>> +       if (!pctrl)
>> +               return -ENOMEM;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> +       slcr = of_get_parent(pdev->dev.of_node);
>> +       if (slcr->data) {
>> +               pctrl->regs = (__force void __iomem *)slcr->data + res->start;
> 
> This looks weird. Use DT parsing functions and accessors, no funny
> business like this. The res-start to the device should be the real
> physical address, not a relative base with offset, dunno what happened
> here but it is wrong.

Soren already has follow up version with is using regmap interface because slcr
is parent and it is syscon. That's why this code will go away.

Thanks,
Michal

[RFC] pinctrl driver for Zynq

[Sören Brinkmann]

Hi Linus,

thanks for the review.

On Tue, 2014-10-07 at 01:05PM +0200, Linus Walleij wrote:
> On Wed, Sep 24, 2014 at 11:09 PM, S?ren Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
> 
> > I think I have pinctrl driver that is covering the pinmux options of
> > Zynq and I also figured out how the DT bindings work.
> 
> OK let's look....
> 
> > One thing making the DT bindings explode, seems to be all those single
> > pin functions that can be muxed to every pin.
> 
> The purpose of the device tree is to describe configuration of the
> hardware, not to describe all possibilities the hardware has, just those
> deployed on a specific system. I think this is the problem, will
> look more into detail below...

Yep, it is. I listed all possible muxing options.

> 
> > Next to GPIO, this applies to SD card and write protect - which are even
> > present twice since Zynq has two SDIO cores. Just these functions
> > account for a couple of hundred nodes in the DT and a bunch of lines in
> > the driver. Is there a better way to do this?
> 
> Probably :-)
> 
> > In particular for GPIO there seemed to be a better solution with
> > implementing gpio_request_enable(), but that seemed to allow GPIO in
> > parallel to request and mux the pin which does not work on Zynq.
> 
> Just that it becomes possible to use GPIO and another function in
> parallel doesn't mean you have to use this possibility. I think this
> is the case with some drivers as of now.

For Zynq, that is not possible.

> 
> I know it has this unfulfilledness about it...
> 
> > IOW: I
> > expected the core would reject a call of gpio_request_enable for a pin
> > that is already muxed to some other function, but that was not the case
> > in my testing. Am I missing something here?
> 
> There are systems where a certain function and GPIO can be used
> in parallel, so GPIO can "spy" on a pin used by another function.
> I think there was something like a flag line from some hardware
> that was used by the HW block, but sometimes other parts of
> the system needed to know the state of that line and it was not
> in the registers for that hardware, but it could be read by using this
> dual-mode property of the GPIO.
> 
> > And finally, for SD card detect and write protect, we actually have to
> > disable the muxing. The problem with those functions is, that they have
> > a dedicated mux for that function which is in parallel to the "normal"
> > pinmuxes. So, muxing a "normal" function to a pin would not void the
> > muxing of the SD signals.
> 
> This should be solved internally in the pin control driver, as it should
> handle hardware pecularities and abstract it to the framework.

Yeah, I think there should not be a problem after all. If the SDHCI mux
config changes it should change for all pins. And even if it's using
micro-sd which does not feature all pins, there are the EMIO mux options
for that, which just tie the signal off internally.

> 
> Do not rely on the framework to provide convenient quirk hooks
> for hardware oddities.
> 
> > I thought this would be easily resolved by
> > implementing the 'disable' op, but after I did that, I noticed that
> > there is only a stale documentation comment of this member of struct
> > pinmux_ops left, the actual function pointer is gone.
> 
> This has been removed since it was bogus and is being cleaned
> up and the vtable entry .enable() has been renamed .set_mux().
> 
> > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> > index 6cc83d4c6c76..814873da0392 100644
> > --- a/arch/arm/boot/dts/zynq-7000.dtsi
> > +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> > @@ -229,7 +229,7 @@
> >                 slcr: slcr at f8000000 {
> >                         #address-cells = <1>;
> >                         #size-cells = <1>;
> > -                       compatible = "xlnx,zynq-slcr", "syscon";
> > +                       compatible = "xlnx,zynq-slcr", "syscon", "simple-bus";
> >                         reg = <0xF8000000 0x1000>;
> >                         ranges;
> >                         clkc: clkc at 100 {
> > @@ -250,6 +250,3043 @@
> >                                                 "dbg_trc", "dbg_apb";
> >                                 reg = <0x100 0x100>;
> >                         };
> > +
> > +                       pinctrl0: pinctrl at 700 {
> > +                               compatible = "xlnx,pinctrl-zynq";
> > +                               reg = <0x700 0x200>;
> > +
> > +                               pinctrl_i2c0_0: pinctrl-i2c0 at 0 {
> 
> So is this a state?

I think that was my original idea. The state for the first I2C core at
its first muxing option.

> 
> > +                                       i2c0-mux {
> > +                                               function = "i2c0";
> > +                                               pins = "i2c0_0_grp";
> 
> This I know we discussed in person. I have renamed the latter
> string from "pins" to "groups" which is as you can see way more
> appropriate. See this patch which is queued for v3.18:
> 
> http://marc.info/?l=devicetree&m=141223584006648&w=2

That is great. If we then also fix that we can use pins for addressing
actual pins and groups for the groups in the generic framework, it would
solve my problem with MMC SD/WP below.

> 
> I'm also working on converting the Nomadik driver to this and
> provide central DT parsing in the pin control core.
> 
> > +                               pinctrl_i2c0_1: pinctrl-i2c0 at 1 {
> > +                                       i2c0-mux {
> > +                                               function = "i2c0";
> > +                                               pins = "i2c0_1_grp";
> > +                                       };
> > +                               };
> > +
> > +                               pinctrl_i2c0_2: pinctrl-i2c0 at 2 {
> > +                                       i2c0-mux {
> > +                                               function = "i2c0";
> > +                                               pins = "i2c0_2_grp";
> > +                                       };
> > +                               };
> 
> My main concern here is whether all these states are really
> in use, or if you're busy outlining everything the controller
> can do. Just include the bits you actually need for the
> specific system this device tree is for.

This lists all options. I will remove all this and move actually used
states to board DTs.

> 
> > +                               pinctrl_i2c1_10: pinctrl-i2c1 at 10 {
> > +                                       i2c1-mux {
> > +                                               function = "i2c1";
> > +                                               pins = "i2c1_10_grp";
> > +                                       };
> > +                               };
> > +
> > +                               pinctrl_gem0_0: pinctrl-gem0 at 0 {
> > +                                       gem0-mux {
> > +                                               function = "ethernet0";
> > +                                               pins = "ethernet0_0_grp";
> > +                                       };
> > +                               };
> 
> This doesn't add up. You opened up
> pinctrl_i2c0_0: pinctrl-i2c0 at 0 { ... a bit up.

But I also closed the I2C node above.

> 
> Now you're putting gem0 and what not into a i2c0's node?

I don't think so. They should all have their own nodes.
The supposed structure is:
 pinctrl at 0xdeadbee0 {
 	device_foo at 0 {
		pinmux-settings {
			...
		};
	};
 	device_foo at 1 {
		pinmux-settings {
			...
		};
	};
 	device_bar at 0 {
		pinmux-settings {
			...
		};
	};
 	device_bar at 1 {
		pinmux-settings {
			...
		};
	};
 };

I admit, this is probably not fully thought through yet.

> 
> Something is seriously wrong here and I don't know where
> it's gone wrong.
> 
> > +                               pinctrl_gpio0_0: pinctrl-gpio0 at 0 {
> > +                                       gpio0-mux {
> > +                                               function = "gpio0";
> > +                                               pins = "gpio0_0_grp";
> > +                                       };
> > +                               };
> 
> Yeah that's sort of hairy to maintain isn't it?
> Go for the quick helper function .gpio_request_enable() and
> skip this. Live with the fact that pins may be used in
> parallel or figure out a way to patch the framework to prevent
> it.
> 
> I once considered add a bool no_dual_mode_gpio; to
> pinctrl_desc and enforce isolation between the two things.

That is probably what Zynq needs since parallel use is not possible in
Zynq. Either a pin is muxed to the GPIO core or to somewhere else, but
never both (exceptions are the MMC SD and WP pins that always receive
the muxed in signal).

> 
> > diff --git a/arch/arm/boot/dts/zynq-zc706.dts b/arch/arm/boot/dts/zynq-zc706.dts
> > index 4cc9913078cd..1ae9bcaee252 100644
> > --- a/arch/arm/boot/dts/zynq-zc706.dts
> > +++ b/arch/arm/boot/dts/zynq-zc706.dts
> > @@ -33,11 +33,20 @@
> >  &gem0 {
> 
> I'm not familiar with this syntax of putting an ampersand in front
> of a node like that. What does that mean? To me ampersands
> are phandles :-/

They are phandles. I reference the gem0 node from the common dtsi file
to extend its content with the pinctrl properties.

> 
> >         status = "okay";
> >         phy-mode = "rgmii";
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_gem0_0>, <&pinctrl_mdio0_0>;
> 
> This looks right, and maybe you should put in only these
> nodes that you actually use below the pin controller?

Yeah, I tend to agree.

> 
> >  &i2c0 {
> >         status = "okay";
> >         clock-frequency = <400000>;
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_i2c0_10>;
> 
> So why do we need to defined pinctrl_i2c0_0 ...
> pinctrl_i2c0_N?

It's bluntly listing all possible options for muxing.

> 
> > +++ b/arch/arm/mach-zynq/Kconfig
> > @@ -9,6 +9,7 @@ config ARCH_ZYNQ
> >         select HAVE_ARM_TWD if SMP
> >         select ICST
> >         select MFD_SYSCON
> > +       select PINCTRL
> 
> select PINCTRL_ZYNQ?

I don't know, do we want to force building this driver? Michal?

> Usually these things seem to be hard to live without.

Well, we could live without it until now :)

> 
> > +++ b/drivers/pinctrl/Kconfig
> > @@ -305,6 +305,14 @@ config PINCTRL_PALMAS
> >           open drain configuration for the Palmas series devices like
> >           TPS65913, TPS80036 etc.
> >
> > +config PINCTRL_ZYNQ
> > +       bool "Pinctrl driver for Xilinx Zynq"
> > +       depends on ARCH_ZYNQ
> > +       select PINMUX
> > +       select GENERIC_PINCONF
> 
> Thanks for using generic pinconf. But you haven't implemented
> it yet?!

Working on it :) Though, I'm not sure I get away with the generic
implementation.

> 
> > +++ b/drivers/pinctrl/pinctrl-zynq.c
> (...)
> > +#define ZYNQ_PINMUX_MUX_SHIFT  1
> > +#define ZYNQ_PINMUX_MUX_MASK   (0x7f << ZYNQ_PINMUX_MUX_SHIFT)
> 
> I would just like ...
> #define ZYNQ_PINMUX_MUX_MASK 0xfe

What's the difference? I usually find it easier to figure out the width
and then shift it in position. But after all, it doesn't really matter.

> 
> > +/**
> > + * struct zynq_pinmux_function - a pinmux function
> > + * @name:    Name of the pinmux function.
> > + * @groups:  List of pingroups for this function.
> > + * @ngroups: Number of entries in @groups.
> > + */
> > +struct zynq_pinmux_function {
> > +       const char *name;
> > +       const char * const *groups;
> > +       unsigned int ngroups;
> > +       unsigned int mux_val;
> > +       u32 mux;
> > +       u32 mux_mask;
> > +       u8 mux_shift;
> > +};
> 
> I agree: it is a good thing to document. But kerneldoc all of
> it!

Yeah, too much churn and updating the kernel-doc got lost. I will update
that.

> 
> > +enum zynq_pinmux_functions {
> > +       ZYNQ_PMUX_ethernet0,
> > +       ZYNQ_PMUX_ethernet1,
> > +       ZYNQ_PMUX_mdio0,
> > +       ZYNQ_PMUX_mdio1,
> > +       ZYNQ_PMUX_qspi0,
> > +       ZYNQ_PMUX_qspi1,
> > +       ZYNQ_PMUX_qspi_fbclk,
> > +       ZYNQ_PMUX_qspi_cs1,
> > +       ZYNQ_PMUX_spi0,
> > +       ZYNQ_PMUX_spi1,
> > +       ZYNQ_PMUX_sdio0,
> > +       ZYNQ_PMUX_sdio0_pc,
> > +       ZYNQ_PMUX_sdio0_cd,
> > +       ZYNQ_PMUX_sdio0_wp,
> > +       ZYNQ_PMUX_sdio1,
> > +       ZYNQ_PMUX_sdio1_pc,
> > +       ZYNQ_PMUX_sdio1_cd,
> > +       ZYNQ_PMUX_sdio1_wp,
> > +       ZYNQ_PMUX_smc0_nor,
> > +       ZYNQ_PMUX_smc0_nor_cs1,
> > +       ZYNQ_PMUX_smc0_nor_addr25,
> > +       ZYNQ_PMUX_smc0_nand,
> > +       ZYNQ_PMUX_can0,
> > +       ZYNQ_PMUX_can1,
> > +       ZYNQ_PMUX_uart0,
> > +       ZYNQ_PMUX_uart1,
> > +       ZYNQ_PMUX_i2c0,
> > +       ZYNQ_PMUX_i2c1,
> > +       ZYNQ_PMUX_ttc0,
> > +       ZYNQ_PMUX_ttc1,
> > +       ZYNQ_PMUX_swdt0,
> > +       ZYNQ_PMUX_gpio0,
> > +       ZYNQ_PMUX_MAX_FUNC
> > +};
> 
> This looks entirely reasonable. Those are the functions we
> really need to control on this system.
> 
> > +const struct pinctrl_pin_desc zynq_pins[] = {
> > +      PINCTRL_PIN(0,  "MIO0"),
> (...)
> > +      PINCTRL_PIN(53, "MIO53"),
> > +      PINCTRL_PIN(54, "EMIO_SD0_WP"),
> > +      PINCTRL_PIN(55, "EMIO_SD0_CD"),
> > +      PINCTRL_PIN(56, "EMIO_SD1_WP"),
> > +      PINCTRL_PIN(57, "EMIO_SD0_CD"),
> > +};
> 
> Looks good.
> 
> > +/* pin groups */
> > +static const unsigned int ethernet0_0_pins[] = {16, 17, 18, 19, 20, 21, 22, 23, 24,
> > +                                           25, 26, 27};
> > +static const unsigned int ethernet1_0_pins[] = {28, 29, 30, 31, 32, 33, 34, 35, 36,
> > +                                               37, 38, 39};
> (...)
> > +static const unsigned int sdio1_3_pins[] = {46, 47, 48, 49, 40, 51};
> 
> Looks good.
> 
> > +static const unsigned int sdio0_emio_wp_pins[] = {54};
> > +static const unsigned int sdio0_emio_cd_pins[] = {55};
> > +static const unsigned int sdio1_emio_wp_pins[] = {56};
> > +static const unsigned int sdio1_emio_cd_pins[] = {57};
> 
> So these are unique functions, not just a way to use GPIO?

Yep, we have dedicated muxes for the SD and WP pins that can mux any pin
to the corresponding pins of the SDHCI

> 
> (...)
> > +static const unsigned int swdt0_4_pins[] = {52, 53};
> 
> Looks good until here.
> 
> > +static const unsigned int gpio0_0_pins[] = {0};
> (...)
> > +static const unsigned int gpio0_53_pins[] = {53};
> 
> Looks very tiresome to maintain. What about using the
> shortcut?

Well, the shortcut might work for GPIO, but then I still have the SD and
WP pins that also are muxable on every pin.
Also, implementing pinconf I encountered, that I have to use groups when
defining my pinconf in DT (since I used
pinconf_generic_dt_node_to_map_group). Which again makes me use this
single-pin groups. Defining pinconf on a functional group usually
doesn't make much sense, IMHO, since it includes inputs as well as
output. I need a finer granular mechanism for pin-selection. So, since I
already have these gpio groups, I just fell back to using those.
It's horrifically ugly, but I couldn't find a better way - apart from
ditching all the generic helpers and implementing something custom for
Zynq.
(I hope this can be fixed with the introduction of 'groups' for the DT
property, as mentioned above)

> 
> (...)
> > +struct zynq_pctrl_group zynq_pctrl_groups[] = {
> > +       DEFINE_ZYNQ_PINCTRL_GRP(ethernet0_0),
> > +       DEFINE_ZYNQ_PINCTRL_GRP(ethernet1_0),
> (...)
> > +       DEFINE_ZYNQ_PINCTRL_GRP(swdt0_4),
> 
> Generally looks good.
> 
> > +/* function groups */
> > +static const char * const ethernet0_groups[] = {"ethernet0_0_grp"};
> > +static const char * const ethernet1_groups[] = {"ethernet1_0_grp"};
> > +static const char * const mdio0_groups[] = {"mdio0_0_grp"};
> > +static const char * const mdio1_groups[] = {"mdio1_0_grp"};
> > +static const char * const qspi0_groups[] = {"qspi0_0_grp"};
> > +static const char * const qspi1_groups[] = {"qspi0_1_grp"};
> > +static const char * const qspi_fbclk_groups[] = {"qspi_fbclk_grp"};
> > +static const char * const qspi_cs1_groups[] = {"qspi_cs1_grp"};
> > +static const char * const spi0_groups[] =
> > +                               {"spi0_0_grp", "spi0_1_grp", "spi0_2_grp"};
> > +static const char * const spi1_groups[] =
> > +               {"spi1_0_grp", "spi1_1_grp", "spi1_2_grp", "spi1_3_grp"};
> 
> Looks good too, OK these functions can be muxed on these groups...
> 
> > +static const char * const sdio0_pc_groups[] = {"gpio0_0_grp",
> > +               "gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
> > +               "gpio0_8_grp", "gpio0_10_grp", "gpio0_12_grp",
> > +               "gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
> > +               "gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
> > +               "gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
> > +               "gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
> > +               "gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
> > +               "gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
> > +               "gpio0_50_grp", "gpio0_52_grp"};
> > +static const char * const sdio1_pc_groups[] = {"gpio0_1_grp",
> > +               "gpio0_3_grp", "gpio0_5_grp", "gpio0_7_grp",
> > +               "gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
> > +               "gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
> > +               "gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
> > +               "gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
> > +               "gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
> > +               "gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
> > +               "gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
> > +               "gpio0_51_grp", "gpio0_53_grp"};
> > +static const char * const sdio0_cd_groups[] = {"gpio0_0_grp",
> > +               "gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
> > +               "gpio0_10_grp", "gpio0_12_grp",
> > +               "gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
> > +               "gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
> > +               "gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
> > +               "gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
> > +               "gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
> > +               "gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
> > +               "gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
> > +               "gpio0_3_grp", "gpio0_5_grp",
> > +               "gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
> > +               "gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
> > +               "gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
> > +               "gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
> > +               "gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
> > +               "gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
> > +               "gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
> > +               "gpio0_51_grp", "gpio0_53_grp", "sdio0_emio_cd_grp"};
> > +static const char * const sdio0_wp_groups[] = {"gpio0_0_grp",
> > +               "gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
> > +               "gpio0_10_grp", "gpio0_12_grp",
> > +               "gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
> > +               "gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
> > +               "gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
> > +               "gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
> > +               "gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
> > +               "gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
> > +               "gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
> > +               "gpio0_3_grp", "gpio0_5_grp",
> > +               "gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
> > +               "gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
> > +               "gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
> > +               "gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
> > +               "gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
> > +               "gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
> > +               "gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
> > +               "gpio0_51_grp", "gpio0_53_grp", "sdio0_emio_wp_grp"};
> > +static const char * const sdio1_cd_groups[] = {"gpio0_0_grp",
> > +               "gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
> > +               "gpio0_10_grp", "gpio0_12_grp",
> > +               "gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
> > +               "gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
> > +               "gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
> > +               "gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
> > +               "gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
> > +               "gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
> > +               "gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
> > +               "gpio0_3_grp", "gpio0_5_grp",
> > +               "gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
> > +               "gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
> > +               "gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
> > +               "gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
> > +               "gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
> > +               "gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
> > +               "gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
> > +               "gpio0_51_grp", "gpio0_53_grp", "sdio1_emio_cd_grp"};
> > +static const char * const sdio1_wp_groups[] = {"gpio0_0_grp",
> > +               "gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
> > +               "gpio0_10_grp", "gpio0_12_grp",
> > +               "gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
> > +               "gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
> > +               "gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
> > +               "gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
> > +               "gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
> > +               "gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
> > +               "gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
> > +               "gpio0_3_grp", "gpio0_5_grp",
> > +               "gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
> > +               "gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
> > +               "gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
> > +               "gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
> > +               "gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
> > +               "gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
> > +               "gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
> > +               "gpio0_51_grp", "gpio0_53_grp", "sdio1_emio_wp_grp"};
> 
> That's like, making your life hard. Use the shortcut.

There is only a - currently, for Zynq, not really working - shortcut for
GPIO, right? This is MMC SD/WP, there is no shortcut for those, AFAICT.

> 
> > +#define DEFINE_ZYNQ_PINMUX_FUNCTION(fname, mval)       \
> > +       [ZYNQ_PMUX_##fname] = {                         \
> > +               .name = #fname,                         \
> > +               .groups = fname##_groups,               \
> > +               .ngroups = ARRAY_SIZE(fname##_groups),  \
> > +               .mux_val = mval                         \
> > +       }
> > +
> > +#define DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(fname, mval, mux, mask, shift) \
> > +       [ZYNQ_PMUX_##fname] = {                         \
> > +               .name = #fname,                         \
> > +               .groups = fname##_groups,               \
> > +               .ngroups = ARRAY_SIZE(fname##_groups),  \
> > +               .mux_val = mval,                        \
> > +               .mux_mask = mask,                       \
> > +               .mux_shift = shift                      \
> > +       }
> > +
> > +#define ZYNQ_SDIO_WP_SHIFT     0
> > +#define ZYNQ_SDIO_WP_MASK      (0x3f << ZYNQ_SDIO_WP_SHIFT)
> 
> Defining something and shifting it 0 positions seem a bit surplus.

Keeping the pattern homogeneous. The CPP should resolve this, so there
shouldn't be any run-time overhead.

> 
> > +#define ZYNQ_SDIO_CD_SHIFT     16
> > +#define ZYNQ_SDIO_CD_MASK      (0x3f << ZYNQ_SDIO_CD_SHIFT)
> 
> This conveys something about the hardware though.
> 
> > +static const struct zynq_pinmux_function zynq_pmux_functions[] = {
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(ethernet0, 1),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(ethernet1, 1),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(mdio0, 0x40),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(mdio1, 0x50),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(qspi0, 1),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(qspi1, 1),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(qspi_fbclk, 1),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(qspi_cs1, 1),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(spi0, 0x50),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(spi1, 0x50),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(sdio0, 0x40),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(sdio0_pc, 0xc),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(sdio0_wp, 0, 130, ZYNQ_SDIO_WP_MASK,
> > +                                       ZYNQ_SDIO_WP_SHIFT),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(sdio0_cd, 0, 130, ZYNQ_SDIO_CD_MASK,
> > +                                       ZYNQ_SDIO_CD_SHIFT),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(sdio1, 0x40),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(sdio1_pc, 0xc),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(sdio1_wp, 0, 134, ZYNQ_SDIO_WP_MASK,
> > +                                       ZYNQ_SDIO_WP_SHIFT),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(sdio1_cd, 0, 134, ZYNQ_SDIO_CD_MASK,
> > +                                       ZYNQ_SDIO_CD_SHIFT),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(smc0_nor, 4),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(smc0_nor_cs1, 8),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(smc0_nor_addr25, 4),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(smc0_nand, 8),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(can0, 0x10),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(can1, 0x10),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(uart0, 0x70),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(uart1, 0x70),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(i2c0, 0x20),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(i2c1, 0x20),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(ttc0, 0x60),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(ttc1, 0x60),
> > +       DEFINE_ZYNQ_PINMUX_FUNCTION(swdt0, 0x30),
> 
> Looks good.
> 
> > +static int zynq_pinmux_enable(struct pinctrl_dev *pctldev,
> > +                             unsigned function,
> > +                             unsigned group)
> 
> Rename zynq_pinmux_set() when rebasing to v3.18-rc1 (when
> it is tagged, or use the pinctrl tree "devel" branch).

Will do. I think things will take a while and I will take the 3.18
branches to base my work on.

> 
> > +static const struct pinmux_ops zynq_pinmux_ops = {
> > +       .get_functions_count = zynq_pmux_get_functions_count,
> > +       .get_function_name = zynq_pmux_get_function_name,
> > +       .get_function_groups = zynq_pmux_get_function_groups,
> > +       .enable = zynq_pinmux_enable,
> 
> Renamed .set_mux() in the current codebase.
> 
> > +static struct pinctrl_desc zynq_desc = {
> > +       .name = "zynq_pinctrl",
> > +       .pins = zynq_pins,
> > +       .npins = ARRAY_SIZE(zynq_pins),
> > +       .pctlops = &zynq_pctrl_ops,
> > +       .pmxops = &zynq_pinmux_ops,
> > +       //const struct pinconf_ops *confops;
> 
> Delete it instead of commenting out. Patch it in later when
> you patch in the pinconf support. It's fine to begin with only
> muxing...

Yeah, I wasn't planning to keep it this way for an actual submission. I
just didn't clean it up for the RFC.

> 
> > +static int zynq_pinctrl_probe(struct platform_device *pdev)
> > +
> > +{
> > +       struct resource *res;
> > +       struct device_node *slcr;
> 
> Usually we just call this *np (node pointer) please rename the variable.
> 
> > +       struct zynq_pinctrl *pctrl;
> 
> Call it zpctrl or something more unique.
> 
> > +       pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
> > +       if (!pctrl)
> > +               return -ENOMEM;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +
> > +       slcr = of_get_parent(pdev->dev.of_node);
> > +       if (slcr->data) {
> > +               pctrl->regs = (__force void __iomem *)slcr->data + res->start;
> 
> This looks weird. Use DT parsing functions and accessors, no funny
> business like this. The res-start to the device should be the real
> physical address, not a relative base with offset, dunno what happened
> here but it is wrong.

I reworked this and it is using syscon and regmap now.

        pctrl->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
                                                        "syscon");
        if (IS_ERR(pctrl->syscon)) {
                dev_err(&pdev->dev, "unable to get syscon\n");
                return PTR_ERR(pctrl->syscon);
        }

        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
        if (!res) {
                dev_err(&pdev->dev, "missing IO resource\n");
                return -ENODEV;
        }
        pctrl->pctrl_offset = res->start;


	Thanks,
	S?ren

[RFC] pinctrl driver for Zynq

[Sören Brinkmann]

On Tue, 2014-10-07 at 01:05PM +0200, Linus Walleij wrote:
> On Wed, Sep 24, 2014 at 11:09 PM, S?ren Brinkmann
[...]
> > +++ b/drivers/pinctrl/Kconfig
> > @@ -305,6 +305,14 @@ config PINCTRL_PALMAS
> >           open drain configuration for the Palmas series devices like
> >           TPS65913, TPS80036 etc.
> >
> > +config PINCTRL_ZYNQ
> > +       bool "Pinctrl driver for Xilinx Zynq"
> > +       depends on ARCH_ZYNQ
> > +       select PINMUX
> > +       select GENERIC_PINCONF
> 
> Thanks for using generic pinconf. But you haven't implemented
> it yet?!

Muxing uses the generic pinconf stuff to parse the DT. Even for muxing
only I need this, I think. That codes needs to change with your
proposed change to introduce the 'groups' property.

	S?ren