From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Walleij Date: Fri, 24 May 2013 08:52:42 +0000 Subject: Re: [PATCH v4 02/21] sh-pfc: Add DT support Message-Id: List-Id: References: <1369138482-5871-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <1369138482-5871-3-git-send-email-laurent.pinchart+renesas@ideasonboard.com> In-Reply-To: <1369138482-5871-3-git-send-email-laurent.pinchart+renesas@ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-arm-kernel@lists.infradead.org On Tue, May 21, 2013 at 2:14 PM, Laurent Pinchart wrote: > Support device instantiation through the device tree. The compatible > property is used to select the SoC pinmux information. > > Set the gpio_chip device field to the PFC device to enable automatic > GPIO OF support. > > Cc: devicetree-discuss@lists.ozlabs.org > Signed-off-by: Laurent Pinchart (...) (Again I'm rather nervous about all the device tree binding reviewing being dropped in the lap of Linux subsystem maintainers and then expecting them to be good and OS-neutral... Not your fault though.) > +Pin Configuration Node Properties: > + > +- renesas,pins : An array of strings, each string containing the name of a pin. > +- renesas,groups : An array of strings, each string containing the name of a pin > + group. > + > +- renesas,function: A string containing the name of the function to mux to the > + pin group(s) specified by the renesas,groups property > + > + Valid values for pin, group and function names can be found in the group and > + function arrays of the PFC data file corresponding to the SoC > + (drivers/pinctrl/sh-pfc/pfc-*.c) > + > +- renesas,pull-up: An integer representing the pull-up strength to be applied > + to all pins specified by the renesas,pins and renesas-groups properties. > + 0 disables the pull-up, 1 enables it. Other values should not be used. Then just use a boolean. > +- renesas,pull-down: An integer representing the pull-down strength to be > + applied to all pins specified by the renesas,pins and renesas-groups > + properties. 0 disables the pull-down, 1 enables it. Other values should not > + be used. Just use a boolean oneliner then. But I prefer that you define something really generic in Documentation/devicetree/bindings/pinconf.txt for anyone using generic pin config, then reference that. This way we can have a more general config binding for any system using generic pin config, mapping to the configs we have in The upside is that we could move the pinconf generic parsing code to drivers/pinctrl/pinconf-generic.c and thus avoid code duplication. We have so far not tried much to standardize pinctrl bindings, but this would be a good opportunity. > +On SH7372, SH73A0, R8A73A4 and R8A7740 the PFC node is also a GPIO controller > +node. > + > +Required Properties: > + > + - gpio-controller: Marks the device node as a gpio controller. > + > + - #gpio-cells: Should be 2. The first cell is the pin number and the second > + cell is used to specify optional parameters as bit flags. Only the GPIO > + active low flag (bit 0) is currently supported. I suggest these properties shall be defined in an include file using that new include hierarchy, since you're using pinconf generic. include/dt-bindings/gpio/gpio.h Referenced by in the .dts[i] files. In Linux-next you find how tegra machines and the imx6sl DTs have started to use this facility for symbolic names. And for that you should reference Use the symbolic names GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW here. (...) > +Example 2: A GPIO LED node that references a GPIO > + > + leds { > + compatible = "gpio-leds"; > + led1 { > + gpios = <&pfc 20 1>; /* Active low */ #include gpios = <&pfc 20 GPIO_ACTIVE_LOW>; (...) > +Example 3: KZM-A9-GT (SH-Mobile AG5) default pin state hog and pin control maps > + for the MMCIF and SCIFA4 devices > + > + &pfc { > + pinctrl-0 = <&scifa4_pins>; > + pinctrl-names = "default"; > + > + mmcif_pins: mmcif { > + mux { > + renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0"; > + renesas,function = "mmc0"; > + }; > + cfg { > + renesas,groups = "mmc0_data8_0"; > + renesas,pins = "PORT279"; > + renesas,pull-up = <1>; Just renesas,pull-up; Or go for the generic pinconf binding I'm suggesting. Then I guess it'd be something like: pinconf-bias-pull-up; (Quite readable.) (...) > +++ b/drivers/pinctrl/sh-pfc/pinctrl.c > +static const struct sh_pfc_config_param sh_pfc_config_params[] = { > + { "renesas,pull-up", PIN_CONFIG_BIAS_PULL_UP }, > + { "renesas,pull-down", PIN_CONFIG_BIAS_PULL_DOWN }, > +}; So these should be checked as booleans instead: > + for (i = 0; i < ARRAY_SIZE(sh_pfc_config_params); ++i) { > + const struct sh_pfc_config_param *param > + &sh_pfc_config_params[i]; > + unsigned long config; > + u32 val; > + > + ret = of_property_read_u32(np, param->name, &val); of_property_read_bool() Though I much rather like this code added as helper lib in pinconf-generic.c. Use drivers/pinctrl/pinconf.h for API. Yours, Linus Walleij From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Walleij Subject: Re: [PATCH v4 02/21] sh-pfc: Add DT support Date: Fri, 24 May 2013 10:52:42 +0200 Message-ID: References: <1369138482-5871-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <1369138482-5871-3-git-send-email-laurent.pinchart+renesas@ideasonboard.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Return-path: In-Reply-To: <1369138482-5871-3-git-send-email-laurent.pinchart+renesas@ideasonboard.com> Sender: linux-sh-owner@vger.kernel.org To: Laurent Pinchart , Haojian Zhuang Cc: "linux-sh@vger.kernel.org" , "devicetree-discuss@lists.ozlabs.org" , "linux-arm-kernel@lists.infradead.org" , Magnus Damm , Guennadi Liakhovetski List-Id: devicetree@vger.kernel.org On Tue, May 21, 2013 at 2:14 PM, Laurent Pinchart wrote: > Support device instantiation through the device tree. The compatible > property is used to select the SoC pinmux information. > > Set the gpio_chip device field to the PFC device to enable automatic > GPIO OF support. > > Cc: devicetree-discuss@lists.ozlabs.org > Signed-off-by: Laurent Pinchart (...) (Again I'm rather nervous about all the device tree binding reviewing being dropped in the lap of Linux subsystem maintainers and then expecting them to be good and OS-neutral... Not your fault though.) > +Pin Configuration Node Properties: > + > +- renesas,pins : An array of strings, each string containing the name of a pin. > +- renesas,groups : An array of strings, each string containing the name of a pin > + group. > + > +- renesas,function: A string containing the name of the function to mux to the > + pin group(s) specified by the renesas,groups property > + > + Valid values for pin, group and function names can be found in the group and > + function arrays of the PFC data file corresponding to the SoC > + (drivers/pinctrl/sh-pfc/pfc-*.c) > + > +- renesas,pull-up: An integer representing the pull-up strength to be applied > + to all pins specified by the renesas,pins and renesas-groups properties. > + 0 disables the pull-up, 1 enables it. Other values should not be used. Then just use a boolean. > +- renesas,pull-down: An integer representing the pull-down strength to be > + applied to all pins specified by the renesas,pins and renesas-groups > + properties. 0 disables the pull-down, 1 enables it. Other values should not > + be used. Just use a boolean oneliner then. But I prefer that you define something really generic in Documentation/devicetree/bindings/pinconf.txt for anyone using generic pin config, then reference that. This way we can have a more general config binding for any system using generic pin config, mapping to the configs we have in The upside is that we could move the pinconf generic parsing code to drivers/pinctrl/pinconf-generic.c and thus avoid code duplication. We have so far not tried much to standardize pinctrl bindings, but this would be a good opportunity. > +On SH7372, SH73A0, R8A73A4 and R8A7740 the PFC node is also a GPIO controller > +node. > + > +Required Properties: > + > + - gpio-controller: Marks the device node as a gpio controller. > + > + - #gpio-cells: Should be 2. The first cell is the pin number and the second > + cell is used to specify optional parameters as bit flags. Only the GPIO > + active low flag (bit 0) is currently supported. I suggest these properties shall be defined in an include file using that new include hierarchy, since you're using pinconf generic. include/dt-bindings/gpio/gpio.h Referenced by in the .dts[i] files. In Linux-next you find how tegra machines and the imx6sl DTs have started to use this facility for symbolic names. And for that you should reference Use the symbolic names GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW here. (...) > +Example 2: A GPIO LED node that references a GPIO > + > + leds { > + compatible = "gpio-leds"; > + led1 { > + gpios = <&pfc 20 1>; /* Active low */ #include gpios = <&pfc 20 GPIO_ACTIVE_LOW>; (...) > +Example 3: KZM-A9-GT (SH-Mobile AG5) default pin state hog and pin control maps > + for the MMCIF and SCIFA4 devices > + > + &pfc { > + pinctrl-0 = <&scifa4_pins>; > + pinctrl-names = "default"; > + > + mmcif_pins: mmcif { > + mux { > + renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0"; > + renesas,function = "mmc0"; > + }; > + cfg { > + renesas,groups = "mmc0_data8_0"; > + renesas,pins = "PORT279"; > + renesas,pull-up = <1>; Just renesas,pull-up; Or go for the generic pinconf binding I'm suggesting. Then I guess it'd be something like: pinconf-bias-pull-up; (Quite readable.) (...) > +++ b/drivers/pinctrl/sh-pfc/pinctrl.c > +static const struct sh_pfc_config_param sh_pfc_config_params[] = { > + { "renesas,pull-up", PIN_CONFIG_BIAS_PULL_UP }, > + { "renesas,pull-down", PIN_CONFIG_BIAS_PULL_DOWN }, > +}; So these should be checked as booleans instead: > + for (i = 0; i < ARRAY_SIZE(sh_pfc_config_params); ++i) { > + const struct sh_pfc_config_param *param = > + &sh_pfc_config_params[i]; > + unsigned long config; > + u32 val; > + > + ret = of_property_read_u32(np, param->name, &val); of_property_read_bool() Though I much rather like this code added as helper lib in pinconf-generic.c. Use drivers/pinctrl/pinconf.h for API. Yours, Linus Walleij From mboxrd@z Thu Jan 1 00:00:00 1970 From: linus.walleij@linaro.org (Linus Walleij) Date: Fri, 24 May 2013 10:52:42 +0200 Subject: [PATCH v4 02/21] sh-pfc: Add DT support In-Reply-To: <1369138482-5871-3-git-send-email-laurent.pinchart+renesas@ideasonboard.com> References: <1369138482-5871-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <1369138482-5871-3-git-send-email-laurent.pinchart+renesas@ideasonboard.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, May 21, 2013 at 2:14 PM, Laurent Pinchart wrote: > Support device instantiation through the device tree. The compatible > property is used to select the SoC pinmux information. > > Set the gpio_chip device field to the PFC device to enable automatic > GPIO OF support. > > Cc: devicetree-discuss at lists.ozlabs.org > Signed-off-by: Laurent Pinchart (...) (Again I'm rather nervous about all the device tree binding reviewing being dropped in the lap of Linux subsystem maintainers and then expecting them to be good and OS-neutral... Not your fault though.) > +Pin Configuration Node Properties: > + > +- renesas,pins : An array of strings, each string containing the name of a pin. > +- renesas,groups : An array of strings, each string containing the name of a pin > + group. > + > +- renesas,function: A string containing the name of the function to mux to the > + pin group(s) specified by the renesas,groups property > + > + Valid values for pin, group and function names can be found in the group and > + function arrays of the PFC data file corresponding to the SoC > + (drivers/pinctrl/sh-pfc/pfc-*.c) > + > +- renesas,pull-up: An integer representing the pull-up strength to be applied > + to all pins specified by the renesas,pins and renesas-groups properties. > + 0 disables the pull-up, 1 enables it. Other values should not be used. Then just use a boolean. > +- renesas,pull-down: An integer representing the pull-down strength to be > + applied to all pins specified by the renesas,pins and renesas-groups > + properties. 0 disables the pull-down, 1 enables it. Other values should not > + be used. Just use a boolean oneliner then. But I prefer that you define something really generic in Documentation/devicetree/bindings/pinconf.txt for anyone using generic pin config, then reference that. This way we can have a more general config binding for any system using generic pin config, mapping to the configs we have in The upside is that we could move the pinconf generic parsing code to drivers/pinctrl/pinconf-generic.c and thus avoid code duplication. We have so far not tried much to standardize pinctrl bindings, but this would be a good opportunity. > +On SH7372, SH73A0, R8A73A4 and R8A7740 the PFC node is also a GPIO controller > +node. > + > +Required Properties: > + > + - gpio-controller: Marks the device node as a gpio controller. > + > + - #gpio-cells: Should be 2. The first cell is the pin number and the second > + cell is used to specify optional parameters as bit flags. Only the GPIO > + active low flag (bit 0) is currently supported. I suggest these properties shall be defined in an include file using that new include hierarchy, since you're using pinconf generic. include/dt-bindings/gpio/gpio.h Referenced by in the .dts[i] files. In Linux-next you find how tegra machines and the imx6sl DTs have started to use this facility for symbolic names. And for that you should reference Use the symbolic names GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW here. (...) > +Example 2: A GPIO LED node that references a GPIO > + > + leds { > + compatible = "gpio-leds"; > + led1 { > + gpios = <&pfc 20 1>; /* Active low */ #include gpios = <&pfc 20 GPIO_ACTIVE_LOW>; (...) > +Example 3: KZM-A9-GT (SH-Mobile AG5) default pin state hog and pin control maps > + for the MMCIF and SCIFA4 devices > + > + &pfc { > + pinctrl-0 = <&scifa4_pins>; > + pinctrl-names = "default"; > + > + mmcif_pins: mmcif { > + mux { > + renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0"; > + renesas,function = "mmc0"; > + }; > + cfg { > + renesas,groups = "mmc0_data8_0"; > + renesas,pins = "PORT279"; > + renesas,pull-up = <1>; Just renesas,pull-up; Or go for the generic pinconf binding I'm suggesting. Then I guess it'd be something like: pinconf-bias-pull-up; (Quite readable.) (...) > +++ b/drivers/pinctrl/sh-pfc/pinctrl.c > +static const struct sh_pfc_config_param sh_pfc_config_params[] = { > + { "renesas,pull-up", PIN_CONFIG_BIAS_PULL_UP }, > + { "renesas,pull-down", PIN_CONFIG_BIAS_PULL_DOWN }, > +}; So these should be checked as booleans instead: > + for (i = 0; i < ARRAY_SIZE(sh_pfc_config_params); ++i) { > + const struct sh_pfc_config_param *param = > + &sh_pfc_config_params[i]; > + unsigned long config; > + u32 val; > + > + ret = of_property_read_u32(np, param->name, &val); of_property_read_bool() Though I much rather like this code added as helper lib in pinconf-generic.c. Use drivers/pinctrl/pinconf.h for API. Yours, Linus Walleij