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 <[email protected]> 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,<soc-name>-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".
> + 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
> + 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 = <8>;
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.
Yours,
Linus Walleij
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 [email protected]> 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