[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a78c5ce7-bc89-44ad-8c8a-7c17ed8a7995@linaro.org>
Date: Wed, 20 Dec 2023 11:26:21 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Théo Lebrun <theo.lebrun@...tlin.com>,
Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
Gregory CLEMENT <gregory.clement@...tlin.com>,
Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Cc: linux-mips@...r.kernel.org, linux-gpio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
Tawfik Bayouk <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH 1/4] dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add
bindings
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@...tlin.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@...tlin.com>
>>> + - Théo Lebrun <theo.lebrun@...tlin.com>
>>> + - Vladimir Kondratiev <vladimir.kondratiev@...ileye.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
Powered by blists - more mailing lists