Re: [PATCH v2 1/3] dt-bindings: arm: google: add gs101-raven and generic gs101-pixel

[Krzysztof Kozlowski <krzk@kernel.org>]

On 23/12/2024 16:31, André Draszik wrote:
> On Mon, 2024-12-23 at 15:14 +0100, Krzysztof Kozlowski wrote:
>> On 23/12/2024 08:45, André Draszik wrote:
>>> Hi Krzysztof,
>>>
>>> On Sun, 2024-12-22 at 12:38 +0100, Krzysztof Kozlowski wrote:
>>>> On 20/12/2024 12:27, André Draszik wrote:
>>>>> Raven is Google's code name for Pixel 6 Pro. Since there are
>>>>> differences compared to Pixel 6 (Oriole), we need to add a separate
>>>>> compatible for it.
>>>>>
>>>>> We also want to support a generic DT, which can work on any type of
>>>>
>>>> There are no such generic DT devices upstream, so we cannot add bindings
>>>> for them.
>>>
>>> Do you have a better suggestion for the wording?
>>> How about 'gs101-based Pixel base board'?
>>
>> It's not exactly about the wording but the concept. We don't have
>> generic devices, thus no generic DT (DTS). Period. Thus you cannot have
>> such schema.
> 
> There is a Pixel base board, with different additions to it, e.g.
> different displays. The boot loader can pick the right one.
> 
> Let's discuss that in the other thread to have things in one place :-)
>>
> 
>>>>> gs101-based Pixel device, e.g. Pixel 6, or Pixel 6 Pro, or Pixel 6a (as
>>>>> a future addition). Such a DT will have certain nodes disabled / not
>>>>> added. To facilitate such a generic gs101-based Pixel device, also add
>>>>> a more generic gs101-pixel compatible. We can not just use the existing
>>>>> google,gs101 for that, as it refers to the SoC, not a board.
>>>>>
>>>>> Signed-off-by: André Draszik <andre.draszik@linaro.org>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/arm/google.yaml | 18 ++++++++++++++----
>>>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/arm/google.yaml b/Documentation/devicetree/bindings/arm/google.yaml
>>>>> index e20b5c9b16bc..a8faf2256242 100644
>>>>> --- a/Documentation/devicetree/bindings/arm/google.yaml
>>>>> +++ b/Documentation/devicetree/bindings/arm/google.yaml
>>>>> @@ -34,11 +34,21 @@ properties:
>>>>>      const: '/'
>>>>>    compatible:
>>>>>      oneOf:
>>>>> -      - description: Google Pixel 6 / Oriole
>>>>> +      - description: Google GS101 Pixel devices, as generic Pixel, or Pixel 6
>>>>> +          (Oriole), or 6 Pro (Raven)
>>>>> +        minItems: 2
>>>>> +        maxItems: 3
>>>>>          items:
>>>>> -          - enum:
>>>>> -              - google,gs101-oriole
>>>>> -          - const: google,gs101
>>>>> +          enum:
>>>>> +            - google,gs101-oriole
>>>>> +            - google,gs101-raven
>>>>> +            - google,gs101-pixel
>>>>> +            - google,gs101
>>>>
>>>> SoC cannot be a board in the same time.
>>>
>>> Can you please expand? google,gs101 is the SoC, the other ones are boards.
>>> Is the commit message unclear?
>>
>> You now say that these are valid boards:
>>
>> compatible = "google,gs101", "google,gs101";
> 
> Sorry, I don't see how (apart from the fact that dtbs_check flags
> non-unique elements anyway). The result of the patch is:
> 
>         minItems: 2
>         maxItems: 3
>         items:
>           enum:
>             - google,gs101-oriole
>             - google,gs101-raven
>             - google,gs101-pixel
>             - google,gs101

The problem is this line. Although entire concept of flexible list is
neither need nor ever recommended.

>         allOf:
>           - contains:
>               const: google,gs101-pixel
>           - contains:
>               const: google,gs101
> 
> So one can not have 'google,gs101' twice. And if I only add

Still can be, but indeed not with my example but:

"google,gs101", "google,gs101", "google,gs101-pixel";

>     compatible = "google,gs101", "google,gs101";
> to the .dts, then dtbs_check complains indeed.
> 
>> (although compatibles
>>
>> compatible = "google,gs101", "google,gs101-pixel";
> 
> OK, the schema doesn't flag incorrect ordering indeed.
> 
>> Both are wrong. SoC should not be before the board and SoC cannot be
>> used alone. Your schema allows that and that's not good.
>>
>> I did not get what you want to achieve here, so tricky to advice.
> 
> The intention is to enforce either of the following three:
> 
>     compatible = "google,gs101-raven", "google,gs101-pixel", "google,gs101";
>     compatible = "google,gs101-oriole", "google,gs101-pixel", "google,gs101";

These two are standard - list of three: enum + const + const

>     compatible = "google,gs101-pixel", "google,gs101";

And that's a separate entry.

> 
> I think this works (but I was aiming for something shorter,
> possibly involving minItems):
> 
>   compatible:
>     oneOf:
>       - description: Google GS101 Pixel base board

What is a base board in terms of phone? This can be only final product,
you cannot use/have a baseboard. This is not an evalkit.

>         items:
>           - const: google,gs101-pixel
>           - const: google,gs101



Best regards,
Krzysztof