On Sat, May 27, 2017 at 7:56 AM, Baolin Wang [off-list ref] wrote:

This patch adds the binding documentation for Spreadtrum SC9860 pin
controller device.

Signed-off-by: Baolin Wang <redacted>

(...)

+* 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.

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