Received-SPF: pass (google.com: domain of linux-kernel+bounces-152323-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <b86f1058-da53-4a9c-bc12-e7297351b482@salutedevices.com>
Date: Sat, 20 Apr 2024 19:15:14 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v3 4/6] dt-bindings: clock: meson: document A1 SoC
 audio clock controller driver
Content-Language: en-US
To: Rob Herring <robh@kernel.org>
CC: Neil Armstrong <neil.armstrong@linaro.org>, Jerome Brunet
	<jbrunet@baylibre.com>, Michael Turquette <mturquette@baylibre.com>, Stephen
 Boyd <sboyd@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor
 Dooley <conor+dt@kernel.org>, Kevin Hilman <khilman@baylibre.com>, Martin
 Blumenstingl <martin.blumenstingl@googlemail.com>, Philipp Zabel
	<p.zabel@pengutronix.de>, Jiucheng Xu <jiucheng.xu@amlogic.com>,
	<linux-amlogic@lists.infradead.org>, <linux-clk@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
References: <20240419125812.983409-1-jan.dakinevich@salutedevices.com>
 <20240419125812.983409-5-jan.dakinevich@salutedevices.com>
 <20240419210949.GA3979121-robh@kernel.org>
From: Jan Dakinevich <jan.dakinevich@salutedevices.com>
In-Reply-To: <20240419210949.GA3979121-robh@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit


On 4/20/24 00:09, Rob Herring wrote:
> On Fri, Apr 19, 2024 at 03:58:10PM +0300, Jan Dakinevich wrote:
>> Add device tree bindings for A1 SoC audio clock and reset controllers.
>>
>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
>> ---
>>
>> This controller has 6 mandatory and up to 20 optional clocks. To describe
>> this, I use 'additionalItems'. It produces correct processed-schema.json:
>>
>>   "clock-names": {
>>       "maxItems": 26,
>>       "items": [
>>           {
>>               "const": "pclk"
>>           },
>>           {
>>               "const": "dds_in"
>>           },
>>           {
>>               "const": "fclk_div2"
>>           },
>>           {
>>               "const": "fclk_div3"
>>           },
>>           {
>>               "const": "hifi_pll"
>>           },
>>           {
>>               "const": "xtal"
>>           }
>>       ],
>>       "additionalItems": {
>>           "oneOf": [
>>               {
>>                   "pattern": "^slv_sclk[0-9]$"
>>               },
>>               {
>>                   "pattern": "^slv_lrclk[0-9]$"
>>               }
>>           ]
>>       },
>>       "type": "array",
>>       "minItems": 6
>>   },
>>
>> and it behaves as expected. However, the checking is followed by
>> complaints like this:
>>
>>   Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean'
>>
>> And indeed, 'additionalItems' has boolean type in meta-schema. So, how to
>> do it right?
> 
> The meta-schemas are written both to prevent nonsense that json-schema 
> allows by default (e.g additionalitems (wrong case)) and constraints to 
> follow the patterns we expect. I'm happy to loosen the latter case if 
> there's really a need. 
> 
> Generally, most bindings shouldn't be using 'additionalItems' at all as 
> all entries should be defined, but there's a few exceptions. Here, the 
> only reasoning I see is 26 entries is a lot to write out, but that 
> wouldn't really justify it. 

Writing a lot of entries don't scary me too much, but the reason is that
the existence of optional clock sources depends on schematics. Also, we
unable to declare dt-nodes for 'clocks' array in any generic way,
because their declaration would depends on that what is actually
connected to the SoC (dt-node could be "fixed-clock" with specific rate
or something else).

By the way, I don't know any example (neither for A1 SoC nor for other
Amlogic's SoCs) where these optional clocks are used, but they are
allowed by hw.

This is my understanding of this controller. I hope, Jerome Brunet will
clarify how it actually works.

> As Krzysztof pointed out, you either have 
> the clocks in the h/w or you don't, so saying they are variable is 
> suspect.
> 
> Rob

-- 
Best regards
Jan Dakinevich