Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <204e1e61-3182-4dc5-a795-0565803b832e@linaro.org>
Date:   Thu, 28 Jul 2022 12:07:31 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock
 controller bindings
Content-Language: en-US
To:     Jerome Brunet <jbrunet@baylibre.com>, Yu Tu <yu.tu@amlogic.com>,
        linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Kevin Hilman <khilman@baylibre.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>
References: <20220728054202.6981-1-yu.tu@amlogic.com>
 <20220728054202.6981-2-yu.tu@amlogic.com>
 <82e3fd36-df96-a555-4cea-47fabd26502b@linaro.org>
 <1jv8rhfw8h.fsf@starbuckisacylon.baylibre.com>
 <367cf98b-ef06-8f44-76c8-9099a1ec13dc@linaro.org>
 <1jmtctfuli.fsf@starbuckisacylon.baylibre.com>
 <c088e01c-0714-82be-8347-6140daf56640@linaro.org>
 <1jfsilftab.fsf@starbuckisacylon.baylibre.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <1jfsilftab.fsf@starbuckisacylon.baylibre.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 28/07/2022 11:54, Jerome Brunet wrote:
> 
> On Thu 28 Jul 2022 at 11:48, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 28/07/2022 11:09, Jerome Brunet wrote:
>>>
>>> On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>>> On 28/07/2022 10:50, Jerome Brunet wrote:
>>>>>
>>>>> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>>>
>>>>>> On 28/07/2022 07:42, Yu Tu wrote:
>>> [...]
>>>>>>> +/*
>>>>>>> + * CLKID index values
>>>>>>> + */
>>>>>>> +
>>>>>>> +#define CLKID_FIXED_PLL			1
>>>>>>> +#define CLKID_FCLK_DIV2			3
>>>>>>> +#define CLKID_FCLK_DIV3			5
>>>>>>> +#define CLKID_FCLK_DIV4			7
>>>>>>> +#define CLKID_FCLK_DIV5			9
>>>>>>> +#define CLKID_FCLK_DIV7			11
>>>>>>
>>>>>> Why these aren't continuous? IDs are expected to be incremented by 1.
>>>>>>
>>>>>
>>>>> All clocks have IDs, it is one big table in the driver, but we are not exposing them all.
>>>>> For example, with composite 'mux / div / gate' assembly, we usually need
>>>>> only the leaf.
>>>>
>>>> I understand you do not expose them all, but that is not the reason to
>>>> increment ID by 2 or 3... Otherwise these are not IDs and you are not
>>>> expected to put register offsets into the bindings (you do not bindings
>>>> in such case).
>>>
>>> Why is it not an IDs if it not continuous in the bindings ?
>>>
>>> If there is technical reason, we'll probably end up exposing everything. It
>>> would not be a dramatic change. I asked for this over v1 because we have
>>> done that is the past and I think it makes sense.
>>>
>>> I'm happy to be convinced to do things differently. Just looking for the
>>> technical reason that require contiuous exposed IDs.
>>>
>>> The other IDs exists, but we do not expose them as bindings.
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n125
>>
>> https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/
>>
>> https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/
>>
>> https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/
>>
>> The IDs are abstract numbers, where the number does not matter because
>> it is not tied to driver implementation or device programming model. The
>> driver maps ID to respective clock.
>>
>> Using some meaningful numbers as these IDs, means you tied bindings to
>> your implementation and any change in implementation requires change in
>> the bindings. This contradicts the idea of bindings.
>>
> 
> I totally agree. Bindings ID are abstract numbers.
> We do follow that. We even document it:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n118
> 
> It is just a choice to not expose some IDs.
> It is not tied to the implementation at all.
> I think we actually follow the rules and the idea behind it.
> 
> We can expose then all If you still think what we are doing is not appropriate.

No, no need. You are right and I took your not-by-one-increment-ID by
other approaches I saw.

The IDs do not have to be incremental, they should not be tied to
programming model.

You have it done and documented, so thanks for explanation:

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof