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@xxxxxxxxxxx> >>> --- >>> .../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@xxxxxxxxxxx> >>> + - Théo Lebrun <theo.lebrun@xxxxxxxxxxx> >>> + - Vladimir Kondratiev <vladimir.kondratiev@xxxxxxxxxxxx> >>> + >>> +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
