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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 20/12/2023 10:21, Théo Lebrun wrote:
> 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]+)?$".

Yes, but: "-[0-9]", these are not hex.

I don't understand what is the problem here. It's just a regex and there
are plenty of examples how this should look like.

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

Why?

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

OK

> 
> 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
> 


Best regards,
Krzysztof