| Message ID | a1fa5b612de318213bab4404997d133ed118f5d2.1495863824.git.baolin.wang@spreadtrum.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Sat, May 27, 2017 at 7:56 AM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > This patch adds the binding documentation for Spreadtrum SC9860 pin > controller device. > > Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> (...) > +* Spreadtrum Pin Controller > + > +The Spreadtrum pin controller are organized in 3 blocks (types). > + > +The first block comprises some global control registers, and each > +register contains several feilds with one bit or several bits to feilds -> fields Do you mean "bitfields", i.e a few bits inside a configuration register word? > +configurate for some global common configuration, such as domain configurate -> configure > +pad driving level, system control select Actually I do not understand at all what "domain pad driving level" or "system control select" means, those are very generic terms. Can you describe precisely what it means? What domain? What is a domain pad? What kind of system control? What is it selecting between? > and so on. We recognise > +every feild comprising one bit or several bits in one global control feild -> field > +register as one pin, thus we should record every pin's bit offset, > +bit width and register offset to configurate this feild (pin). feild -> field > +The second block comprises some common registers which have unified > +register definition, and each register described one pin is used > +to configurate pin sleep mode and function select. OK > +The last block comprises some misc registers which also have unified > +register definition, and each register described one pin is used to > +configurate drive strength, pull up/down and so on. configurate -> configure OK that is pin configuration. > +This driver supports the generic pin multiplexing and configuration > +bindings. For details on each properties, you can refer to > +./pinctrl-bindings.txt. Do not talk about the driver in the bindings. Talk about the bindings per se, these bindings are supposed to be OS neutral. > +Required properties for Spreadtrum pin controller: > +- compatible: "sprd,<soc>-pinctrl" > + Please refer to each sprd,<soc>-pinctrl.txt binding doc for supported SoCs. > + > +Required properties for pin configuration node: > +- sprd,pins: each entry consists of 2 integers and represents the pin > + id and config setting for one pin. Do not use the custom property "sprd,pins" for this, if you want to set up pin muxing with a magic value use the generic binding "pinmux", see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt?id=8d5e7c5df0a6c442373628be5221321172b1badf But consider using groups+functions for defining multiplexing instead. But please do NOT combine pin configuration into a magic value. Use the generic pin control bindings, er have bindings for all kinds of pin config, and using them makes the driver much more readable for humans. I understand that you may have a lot of magic number tables around that you are using today, but it is necessary to decompose that into the proper generic pin configuration properties to make it usable. > +++ b/Documentation/devicetree/bindings/pinctrl/sprd,sc9860-pinctrl.txt > @@ -0,0 +1,26 @@ > +* Spreadtrum SC9860 Pin Controller > + > +Please refer to sprd,pinctrl.txt in this directory for common binding part > +and usage. > + > +Required properties: > +- compatible: must be "sprd,sc9860-pinctrl". > +- reg: the register address of pin controller device. > +- sprd,pins: two integers array, represents a group of pins id and config > + setting. The format is sprd,pins = <PIN_ID CONFIG>, PIN_ID can be found > + from pinctrl-sprd-sc9860.c file or spec file, CONFIG is the pad setting > + value like pull-up for this pin. Same comments. > +Example: > +pin_controller: pinctrl@402a0000 { > + compatible = "sprd,sc9860-pinctrl"; > + reg = <0x402a0000 0x10000>; > + > + vio_sd0_ms_0: sd0_ms0 { > + sprd,pins = <8 0x1>; > + }; > + > + vbc_iis0_0: iis0_c { > + sprd,pins = <34 0xc>; > + }; > +}; Magic numbers are very hard to understand. Compare to this from Qualcomm APQ8064 using just standard bindings: pinmux@800000 { i2c4_pins: i2c4_pinmux { pins = "gpio12", "gpio13"; function = "gsbi4"; bias-disable; }; spi_pins: spi_pins { mux { pins = "gpio18", "gpio19", "gpio21"; function = "gsbi5"; drive-strength = <10>; bias-none; }; }; }; Please try go get rid of the magic numbers and get to use pins, function, drive-strength bias etc from the standard bindings. We also have a lot of helper code available to use this. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, May 27, 2017 at 7:56 AM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > This patch adds the pin control driver for Spreadtrum SC9860 platform. > > Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> Overall I see that you want to store functions and groups in the device tree using the current helpers from Tony. That is OK if you want to take that approach, though I prefer the pins/groups/function encoding in plaintext. But when it comes to pin config: > +static int sprd_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin_id, > + unsigned long *config) > +{ > + struct sprd_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev); > + struct sprd_pin *pin = sprd_pinctrl_get_pin_by_id(pctl, pin_id); > + > + if (!pin) > + return -EINVAL; > + > + if (pin->type == GLOBAL_CTRL_PIN) { > + *config = (readl((void __iomem *)pin->reg) >> > + pin->bit_offset) & PINCTRL_BIT_MASK(pin->bit_width); > + } else { > + *config = readl((void __iomem *)pin->reg); > + } > + > + return 0; > +} > + > +static int sprd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin_id, > + unsigned long *configs, unsigned int num_configs) > +{ > + struct sprd_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev); > + struct sprd_pin *pin = sprd_pinctrl_get_pin_by_id(pctl, pin_id); > + unsigned long reg; > + int i; > + > + if (!pin) > + return -EINVAL; > + > + for (i = 0; i < num_configs; i++) { > + if (pin->type == GLOBAL_CTRL_PIN) { > + reg = readl((void __iomem *)pin->reg); > + reg &= ~(PINCTRL_BIT_MASK(pin->bit_width) > + << pin->bit_offset); > + reg |= (configs[i] & PINCTRL_BIT_MASK(pin->bit_width)) > + << pin->bit_offset; > + writel(reg, (void __iomem *)pin->reg); > + } else { > + writel(configs[i], (void __iomem *)pin->reg); > + } > + pin->config = configs[i]; > + } > + > + return 0; > +} This is just hammering the register values, you have effectively defined your register layout as your device tree ABI. Please don't do this, please use genric pin config and use the approach other drivers take with explicit strings encoding the pin configuration. Even pinctrl-single that maintain quite a bit of muxing data in the device tree still use the generic pin config API, see: drivers/pinctrl/pinctrl-single.c pcs_pinconf_get(), pcs_pinconf_set() Same for group config. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On 一, 5月 29, 2017 at 06:18:29下午 +0200, Linus Walleij wrote: > On Sat, May 27, 2017 at 7:56 AM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > > > This patch adds the binding documentation for Spreadtrum SC9860 pin > > controller device. > > > > Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> > (...) > > > +* Spreadtrum Pin Controller > > + > > +The Spreadtrum pin controller are organized in 3 blocks (types). > > + > > +The first block comprises some global control registers, and each > > +register contains several feilds with one bit or several bits to > > feilds -> fields Sorry for typos, will fix in next version. > > Do you mean "bitfields", i.e a few bits inside a configuration register > word? Yes. > > > +configurate for some global common configuration, such as domain > > configurate -> configure OK. > > > +pad driving level, system control select > > Actually I do not understand at all what "domain pad driving level" > or "system control select" means, those are very generic terms. > Can you describe precisely what it means? What domain? What > is a domain pad? What kind of system control? What is it selecting > between? I try to explain what they are on Spreadtrum platform. One pin can output 3.0v or 1.8v, depending on the related domain pad driving selection, if the related domain pad slect 3.0v, then the pin can output 3.0v. "system control" is used to choose this function (like: UART0) for which system, since we have several systems (AP/CP/CM4) on one SoC. > > > and so on. We recognise > > +every feild comprising one bit or several bits in one global control > > feild -> field > > > +register as one pin, thus we should record every pin's bit offset, > > +bit width and register offset to configurate this feild (pin). > > feild -> field > > > +The second block comprises some common registers which have unified > > +register definition, and each register described one pin is used > > +to configurate pin sleep mode and function select. > > OK > > > +The last block comprises some misc registers which also have unified > > +register definition, and each register described one pin is used to > > +configurate drive strength, pull up/down and so on. > > configurate -> configure > > OK that is pin configuration. > > > +This driver supports the generic pin multiplexing and configuration > > +bindings. For details on each properties, you can refer to > > +./pinctrl-bindings.txt. > > Do not talk about the driver in the bindings. Talk about the bindings per > se, these bindings are supposed to be OS neutral. Sure, will remove these. > > > +Required properties for Spreadtrum pin controller: > > +- compatible: "sprd,<soc>-pinctrl" > > + Please refer to each sprd,<soc>-pinctrl.txt binding doc for supported SoCs. > > + > > +Required properties for pin configuration node: > > +- sprd,pins: each entry consists of 2 integers and represents the pin > > + id and config setting for one pin. > > Do not use the custom property "sprd,pins" for this, if you want to set up pin > muxing with a magic value use the generic binding "pinmux", see: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt?id=8d5e7c5df0a6c442373628be5221321172b1badf > > But consider using groups+functions for defining multiplexing instead. > > But please do NOT combine pin configuration into a magic value. Use > the generic pin control bindings, er have bindings for all kinds of pin > config, and using them makes the driver much more readable for > humans. > > I understand that you may have a lot of magic number tables around that > you are using today, but it is necessary to decompose that into the proper > generic pin configuration properties to make it usable. Since we have lots of different pin configuration to set, it will be hard to use the standard pin config describing in binding files. But I will try to remove the magic number and use the common pin config. > > > +++ b/Documentation/devicetree/bindings/pinctrl/sprd,sc9860-pinctrl.txt > > @@ -0,0 +1,26 @@ > > +* Spreadtrum SC9860 Pin Controller > > + > > +Please refer to sprd,pinctrl.txt in this directory for common binding part > > +and usage. > > + > > +Required properties: > > +- compatible: must be "sprd,sc9860-pinctrl". > > +- reg: the register address of pin controller device. > > +- sprd,pins: two integers array, represents a group of pins id and config > > + setting. The format is sprd,pins = <PIN_ID CONFIG>, PIN_ID can be found > > + from pinctrl-sprd-sc9860.c file or spec file, CONFIG is the pad setting > > + value like pull-up for this pin. > > Same comments. > > > +Example: > > +pin_controller: pinctrl@402a0000 { > > + compatible = "sprd,sc9860-pinctrl"; > > + reg = <0x402a0000 0x10000>; > > + > > + vio_sd0_ms_0: sd0_ms0 { > > + sprd,pins = <8 0x1>; > > + }; > > + > > + vbc_iis0_0: iis0_c { > > + sprd,pins = <34 0xc>; > > + }; > > +}; > > Magic numbers are very hard to understand. Compare to this > from Qualcomm APQ8064 using just standard bindings: > > pinmux@800000 { > i2c4_pins: i2c4_pinmux { > pins = "gpio12", "gpio13"; > function = "gsbi4"; > bias-disable; > }; > > spi_pins: spi_pins { > mux { > pins = "gpio18", "gpio19", "gpio21"; > function = "gsbi5"; > drive-strength = <10>; > bias-none; > }; > }; > }; > > Please try go get rid of the magic numbers and get to use pins, function, > drive-strength bias etc from the standard bindings. We also have a lot > of helper code available to use this. Sure. I will try o fix this. Thanks for your helpful comments. > > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On 一, 5月 29, 2017 at 06:28:49下午 +0200, Linus Walleij wrote: > On Sat, May 27, 2017 at 7:56 AM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > > > This patch adds the pin control driver for Spreadtrum SC9860 platform. > > > > Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> > > Overall I see that you want to store functions and groups in the device tree > using the current helpers from Tony. That is OK if you want to take that > approach, though I prefer the pins/groups/function encoding in plaintext. > > But when it comes to pin config: > > > +static int sprd_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin_id, > > + unsigned long *config) > > +{ > > + struct sprd_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev); > > + struct sprd_pin *pin = sprd_pinctrl_get_pin_by_id(pctl, pin_id); > > + > > + if (!pin) > > + return -EINVAL; > > + > > + if (pin->type == GLOBAL_CTRL_PIN) { > > + *config = (readl((void __iomem *)pin->reg) >> > > + pin->bit_offset) & PINCTRL_BIT_MASK(pin->bit_width); > > + } else { > > + *config = readl((void __iomem *)pin->reg); > > + } > > + > > + return 0; > > +} > > + > > +static int sprd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin_id, > > + unsigned long *configs, unsigned int num_configs) > > +{ > > + struct sprd_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev); > > + struct sprd_pin *pin = sprd_pinctrl_get_pin_by_id(pctl, pin_id); > > + unsigned long reg; > > + int i; > > + > > + if (!pin) > > + return -EINVAL; > > + > > + for (i = 0; i < num_configs; i++) { > > + if (pin->type == GLOBAL_CTRL_PIN) { > > + reg = readl((void __iomem *)pin->reg); > > + reg &= ~(PINCTRL_BIT_MASK(pin->bit_width) > > + << pin->bit_offset); > > + reg |= (configs[i] & PINCTRL_BIT_MASK(pin->bit_width)) > > + << pin->bit_offset; > > + writel(reg, (void __iomem *)pin->reg); > > + } else { > > + writel(configs[i], (void __iomem *)pin->reg); > > + } > > + pin->config = configs[i]; > > + } > > + > > + return 0; > > +} > > This is just hammering the register values, you have effectively defined your > register layout as your device tree ABI. > > Please don't do this, please use genric pin config and use the approach > other drivers take with explicit strings encoding the pin configuration. Make sense. I will try to use genric pin config instead of the magic number. Thanks for your comments. > > Even pinctrl-single that maintain quite a bit of muxing data in the device > tree still use the generic pin config API, see: > drivers/pinctrl/pinctrl-single.c > pcs_pinconf_get(), pcs_pinconf_set() > > Same for group config. > > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 31, 2017 at 9:58 AM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > On 一, 5月 29, 2017 at 06:18:29下午 +0200, Linus Walleij wrote: >> > +pad driving level, system control select >> >> Actually I do not understand at all what "domain pad driving level" >> or "system control select" means, those are very generic terms. >> Can you describe precisely what it means? What domain? What >> is a domain pad? What kind of system control? What is it selecting >> between? > > I try to explain what they are on Spreadtrum platform. One pin can output > 3.0v or 1.8v, depending on the related domain pad driving selection, if > the related domain pad slect 3.0v, then the pin can output 3.0v. This can probably use the generic pin control property PIN_CONFIG_POWER_SOURCE (see include/linux/pinctrl/pinconf-generic.h) and the corresponding DT binding "power-source" see drivers/pinctrl/pinconf-generic.c and Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > "system control" is used to choose this function (like: UART0) for which > system, since we have several systems (AP/CP/CM4) on one SoC. Aha, that sounds like a very spreadtrum-specific feature actually. > Since we have lots of different pin configuration to set, it will be hard to > use the standard pin config describing in binding files. But I will try to > remove the magic number and use the common pin config. It is possible to use generic config and add a few custom bindings "on top" of it. See for example: Documentation/devicetree/bindings/pinctrl/qcom,pmic-mpp.txt mixing a few generic and Qualcomm-specific pin config things, and their driver is here: drivers/pinctrl/qcom/pinctrl-spmi-mpp.c Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/pinctrl/sprd,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/sprd,pinctrl.txt new file mode 100644 index 0000000..2edf176 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/sprd,pinctrl.txt @@ -0,0 +1,31 @@ +* Spreadtrum Pin Controller + +The Spreadtrum pin controller are organized in 3 blocks (types). + +The first block comprises some global control registers, and each +register contains several feilds with one bit or several bits to +configurate for some global common configuration, such as domain +pad driving level, system control select and so on. We recognise +every feild comprising one bit or several bits in one global control +register as one pin, thus we should record every pin's bit offset, +bit width and register offset to configurate this feild (pin). + +The second block comprises some common registers which have unified +register definition, and each register described one pin is used +to configurate pin sleep mode and function select. + +The last block comprises some misc registers which also have unified +register definition, and each register described one pin is used to +configurate drive strength, pull up/down and so on. + +This driver supports the generic pin multiplexing and configuration +bindings. For details on each properties, you can refer to +./pinctrl-bindings.txt. + +Required properties for Spreadtrum pin controller: +- compatible: "sprd,<soc>-pinctrl" + Please refer to each sprd,<soc>-pinctrl.txt binding doc for supported SoCs. + +Required properties for pin configuration node: +- sprd,pins: each entry consists of 2 integers and represents the pin + id and config setting for one pin. diff --git a/Documentation/devicetree/bindings/pinctrl/sprd,sc9860-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/sprd,sc9860-pinctrl.txt new file mode 100644 index 0000000..26fba5b --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/sprd,sc9860-pinctrl.txt @@ -0,0 +1,26 @@ +* Spreadtrum SC9860 Pin Controller + +Please refer to sprd,pinctrl.txt in this directory for common binding part +and usage. + +Required properties: +- compatible: must be "sprd,sc9860-pinctrl". +- reg: the register address of pin controller device. +- sprd,pins: two integers array, represents a group of pins id and config + setting. The format is sprd,pins = <PIN_ID CONFIG>, PIN_ID can be found + from pinctrl-sprd-sc9860.c file or spec file, CONFIG is the pad setting + value like pull-up for this pin. + +Example: +pin_controller: pinctrl@402a0000 { + compatible = "sprd,sc9860-pinctrl"; + reg = <0x402a0000 0x10000>; + + vio_sd0_ms_0: sd0_ms0 { + sprd,pins = <8 0x1>; + }; + + vbc_iis0_0: iis0_c { + sprd,pins = <34 0xc>; + }; +};
This patch adds the binding documentation for Spreadtrum SC9860 pin controller device. Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> --- .../devicetree/bindings/pinctrl/sprd,pinctrl.txt | 31 ++++++++++++++++++++ .../bindings/pinctrl/sprd,sc9860-pinctrl.txt | 26 ++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/sprd,pinctrl.txt create mode 100644 Documentation/devicetree/bindings/pinctrl/sprd,sc9860-pinctrl.txt