Re: [PATCH 13/15] dt-bindings: pinctrl: add support for Ambarella

Li Chen
2023-01-28T10:06:14Z

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@ambarella.com> 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