Hi Linus, Sorry for my late reply. ---- On Mon, 23 Jan 2023 20:32:28 +0800 Linus Walleij wrote --- > Hi Li, > > thanks for your patch! > > It's nice to see Ambarella working with the kernel community. > > On Mon, Jan 23, 2023 at 8:41 AM Li Chen lchen@xxxxxxxxxxxxx> wrote: > > > +properties: > > + compatible: > > + items: > > + - const: ambarella,pinctrl > > I bet there will be more instances of pin controllers from Ambarella, so I would > use this only as a fallback, so the for should likely be: > > compatible = "ambarella,-pinctrl", "ambarella,pinctrl"; > > we need to establish this already otherwise "ambarella,pinctrl" just becomes > the "weird name of the first ambarella SoC supported by standard DT bindings". There is only single "ambarella,pinctrl" in Ambarella downstream kernels, and we use soc_device_attribute->data and soc_device_attribute->soc_id/family to get correct SoC-specific information like reg offset and etc. Krzysztof has taught me that this way is wrong and SoC is required in compatible: https://www.spinics.net/lists/arm-kernel/msg1043145.html So I will update this property to "ambarella,s6lm-pinctrl" in the new version. > > + amb,pull-regmap: > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > + items: > > + maxItems: 1 > > + > > + amb,ds-regmap: > > + items: > > + maxItems: 1 > > Interesting that these registers are elsewhere, but I bet there is an > engineering > explanation for this :) > > > + properties: > > + amb,pinmux-ids: > > + description: | > > + an integer array. Each integer in the array specifies a pin > > + with given mux function, with pin id and mux packed as: > > + mux << 12 | pin id > > + Here mux means function of this pin, and pin id is identical to gpio id. For > > + the SoC supporting IOMUX, like S2L, the maximal value of mux is 5. However, > > + for the SoC not supporting IOMUX, like A5S, S2, the third or fourth function > > + is selected by other "virtual pins" setting. Here the "virtual pins" means > > + there is no real hardware pins mapping to the corresponding register address. > > + So the registers for the "virtual pins" can be used for the selection of 3rd > > + or 4th function for other real pins. > > I think you can use the standard bindings for this if you insist on > using the "magic > numbers" scheme. > > (I prefer function names and group names as strings, but I gave up on trying > to convince the world to use this because people have so strong opions about > it.) > > From Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml: > > pinmux: > description: > The list of numeric pin ids and their mux settings that properties in the > node apply to (either this, "pins" or "groups" have to be specified) > $ref: /schemas/types.yaml#/definitions/uint32-array > Well noted, I will switch to pinmux in v2. > > + amb,pinconf-ids: > > + description: | > > + an integer array. Each integer in the array specifies a pin > > + with given configuration, with pin id and config packed as: > > + config << 16 | pin id > > + Here config is used to configure pull up/down and drive strength of the pin, > > + and it's orgnization is: > > + bit1~0: 00: pull down, 01: pull up, 1x: clear pull up/down > > + bit2: reserved > > + bit3: 0: leave pull up/down as default value, 1: config pull up/down > > + bit5~4: drive strength value, 0: 2mA, 1: 4mA, 2: 8mA, 3: 12mA > > + bit6: reserved > > + bit7: 0: leave drive strength as default value, 1: config drive strength > > I would be very happy if I could convince you to use the standard (string) > bindings for this. > And from Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml > > For each config node this means using strings such as > bias-high-impedance; etc in the device tree pin config node. > > Following that scheme just makes life so much easier for maintainers and > reviewers: very few people reviewing or debugging the system will think > it is easy to read a magic number and then (in their head) mask out the > bits to see that "OK this is drive strength 8mA" and then have energy and > memory enough in their head to remember that "wait a minute, that is supposed > to be 12mA in this design", leading to long review and development > cycles. > > By using: > > drive-push-pull; > drive-strength = ; > > you make the cognitive load on the people reading the device tree much > lower and easier to write, maintain and debug for humans. > > The tendency to encode this info in terse bitfields appear to be done for either > of these reasons: > > - Footprint / memory usage > - Adopt the users to the way the machine thinks instead of the other way around > - "We always did it this way" > > Neither is a very good argument on a new 64bit platform. Thanks for your detailed explanation. I totally agree with you and I also really hate magic number haha. I will convert it to standard binding after convincing my manager. Regards, Li
