Re: [PATCH 1/4] dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello,

I've seen all your comments, thanks for that. I'll answer to some.

On Tue Dec 19, 2023 at 8:34 AM CET, Krzysztof Kozlowski wrote:
> On 18/12/2023 18:19, Théo Lebrun wrote:
> > Add dt-schema type bindings for the Mobileye EyeQ5 pin controller.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  .../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml   | 125 +++++++++++++++++++++
> >  MAINTAINERS                                        |   1 +
> >  2 files changed, 126 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml
> > new file mode 100644
> > index 000000000000..5faddebe2413
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml
> > @@ -0,0 +1,125 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Mobileye EyeQ5 pinctrl (pinmux & pinconf) controller
>
> pinctrl means pin controller, so you basically wrote:
> pin controller pinmux and pin configuration controller
>
> Just "pin controller"
>
>
> > +
> > +description:
> > +  The EyeQ5 pin controller handles a pin bank. It is custom to this platform,
>
> Can part of SoC be not custom to given platform? I mean... describe the
> hardware, not write essay.
>
> > +  its registers live in a shared region called OLB.
> > +  There are two pin banks on the platform, each having a specific compatible.
>
> Instead of repeating something obvious - visible from the binding -
> explain why. Say something different than the binding is saying.
>
>
> > +  Pins and groups are bijective.
> > +
> > +maintainers:
> > +  - Grégory Clement <gregory.clement@bootlin.com>
> > +  - Théo Lebrun <theo.lebrun@bootlin.com>
> > +  - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^pinctrl([0-9]+)?$"
> > +    description:
> > +      We have no unique address, we rely on OLB; we therefore can't keep the
> > +      standard pattern and cannot inherit from pinctrl.yaml.
>
> No, instead fix pinctrl.yaml

I've tried some things, but I'm unsure how to proceed. Options I see:

 - Modify pinctrl.yaml so that if reg/ranges is required, $nodename must
   be the current value ("^(pinctrl|pinmux)(@[0-9a-f]+)?$"). Else,
   $nodename should be "^(pinctrl|pinmux)(-[0-9a-f]+)?$".

   I've tried some things but nothing conclusive for the moment.

 - Leave pinctrl.yaml alone and override $nodename from our binding.
   I've not found a way to do that though.

 - Use the current $nodename, ie with a unit address. With that approach
   I get the "node has a unit name, but no reg or ranges property"
   warning which, reading the code, I don't see a way of avoiding.

Were you thinking about option 1? Any advice on how to proceed would be
helpful, I've not been able to get a working patch to use option 1.

>
> > +
> > +  compatible:
> > +    enum:
> > +      - mobileye,eyeq5-a-pinctrl
> > +      - mobileye,eyeq5-b-pinctrl
>
> Why two compatibles? Description provided no rationale for this.

I'll add that info. The gist of it is to have one node per bank. Each
pin has two function: GPIO or pin-dependent. So we must know which bank
we are to know what each pin function can be.

Both nodes are child to the same OLB. The compatible also tells us which
registers to use.

>
> > +
> > +  "#pinctrl-cells":
> > +    const: 1
> > +
> > +  mobileye,olb:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      A phandle to the OLB syscon. This is a fallback to using the parent as
> > +      syscon node.
>
> So here is the explanation for missing unit address. If all registers,
> as you claim in description, belong to OLB, then this should be part of
> OLB. Drop the phandle.

The reason I provided both options was that I see four drivers that do
this kind of fallback. I guess it was for legacy reasons. I'm dropping
the phandle and keeping only the child option.

	drivers/gpio/gpio-syscon.c
	drivers/phy/rockchip/phy-rockchip-usb.c
	drivers/phy/samsung/phy-exynos-dp-video.c
	drivers/soc/rockchip/io-domain.c

>
> > +
> > +required:
> > +  - compatible
> > +  - "#pinctrl-cells"
>
> So now please test your code without olb phandle...

That is the main way I am running my code.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com