Received-SPF: pass (google.com: domain of linux-kernel+bounces-81398-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240221160215.484151-1-panikiel@google.com> <20240221160215.484151-9-panikiel@google.com>
 <13aeb2ff-72f4-49d9-b65e-ddc31569a936@linaro.org> <CAM5zL5q0oKoTMR0jSwYVAChCOJ9iKYPRFiU1vH4qDqhHALKz4w@mail.gmail.com>
 <e1fd8cbd-060b-4d15-8256-6d8dbba545da@linaro.org>
In-Reply-To: <e1fd8cbd-060b-4d15-8256-6d8dbba545da@linaro.org>
From: =?UTF-8?Q?Pawe=C5=82_Anikiel?= <panikiel@google.com>
Date: Mon, 26 Feb 2024 13:43:55 +0100
Message-ID: <CAM5zL5qxBM1xQ__t86gxUKMy8O3BzoCe_vTFxxsqFq7mw4-8EQ@mail.gmail.com>
Subject: Re: [PATCH v2 8/9] media: dt-bindings: Add Intel Displayport RX IP
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: airlied@gmail.com, akpm@linux-foundation.org, conor+dt@kernel.org, 
	daniel@ffwll.ch, dinguyen@kernel.org, hverkuil-cisco@xs4all.nl, 
	krzysztof.kozlowski+dt@linaro.org, maarten.lankhorst@linux.intel.com, 
	mchehab@kernel.org, mripard@kernel.org, robh+dt@kernel.org, 
	tzimmermann@suse.de, devicetree@vger.kernel.org, 
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, 
	linux-media@vger.kernel.org, chromeos-krk-upstreaming@google.com, 
	ribalda@chromium.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Feb 26, 2024 at 1:06=E2=80=AFPM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 26/02/2024 11:59, Pawe=C5=82 Anikiel wrote:
> >>> +properties:
> >>> +  compatible:
> >>> +    const: intel,dprx-20.0.1
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
> >>> +
> >>> +  intel,max-link-rate:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    description: Max link rate configuration parameter
> >>
> >> Please do not duplicate property name in description. It's useless.
> >> Instead explain what is this responsible for.
> >>
> >> Why max-link-rate would differ for the same dprx-20.0.1? And why
> >> standard properties cannot be used?
> >>
> >> Same for all questions below.
> >
> > These four properties are the IP configuration parameters mentioned in
> > the device description. When generating the IP core you can set these
> > parameters, which could make them differ for the same dprx-20.0.1.
> > They are documented in the user guide, for which I also put a link in
> > the description. Is that enough? Or should I also document these
> > parameters here?
>
> user-guide is something for users, like user-space programmers or
> end-users. I would never open it to look for any information related to
> hardware.
>
> Anyway, external resources are a no-go. We have it clearly in submitting
> patches:
>
> https://elixir.bootlin.com/linux/v6.8-rc6/source/Documentation/process/su=
bmitting-patches.rst#L130

Okay, I will describe these properties in the bindings as well.

>
> >
> >>
> >>> +
> >>> +  intel,max-lane-count:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    description: Max lane count configuration parameter
> >>> +
> >>> +  intel,multi-stream-support:
> >>> +    type: boolean
> >>> +    description: Multi-Stream Transport support configuration parame=
ter
> >>> +
> >>> +  intel,max-stream-count:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    description: Max stream count configuration parameter
> >>> +
> >>> +  port:
> >>> +    $ref: /schemas/graph.yaml#/properties/port
> >>> +    description: SST main link
> >>
> >> I don't understand why you have both port and ports. Shouldn't this be
> >> under ports?
> >
> > I put both so that you can use the shorter port property when the
> > device only has one port (i.e. no MST support). It would work fine
> > without it. If you think that's unnecessary, I can remove it (and use
> > the ports property even if there is only one).
>
> No, it is fine, but then you need allOf: which will restrict to only one
> of them: either port or ports.

There already is an allOf below that says that ports is required for
MST support and port is required otherwise. Isn't this enough?