Re: [PATCH v2 01/12] dt-bindings: clock: google,gs101-clock: add PERIC0 clock management unit

[Tudor Ambarus]

On 1/9/24 18:38, Krzysztof Kozlowski wrote:
> On 09/01/2024 17:12, Tudor Ambarus wrote:
>>
>>
>> On 1/9/24 15:01, Krzysztof Kozlowski wrote:
>>> On 09/01/2024 12:58, Tudor Ambarus wrote:
>>>>
>>>>
>>>> On 1/9/24 11:09, Krzysztof Kozlowski wrote:
>>>>> On 09/01/2024 05:03, Rob Herring wrote:
>>>>>> On Thu, Dec 28, 2023 at 12:57:54PM +0000, Tudor Ambarus wrote:
>>>>>>> Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0)
>>>>>>> clock management unit.
>>>>>>>
>>>>>>> Reviewed-by: Sam Protsenko <semen.protsenko@xxxxxxxxxx>
>>>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> - fix comments as per Sam's suggestion and collect his R-b tag
>>>>>>> - Rob's suggestion of renaming the clock-names to just "bus" and "ip"
>>>>>>>   was not implemented as I felt it affects readability in the driver
>>>>>>>   and consistency with other exynos clock drivers. I will happily update
>>>>>>>   the names in the -rc phase if someone else has a stronger opinion than
>>>>>>>   mine. 
>>>>>>
>>>>>> I'll defer to Krzysztof.
>>>>>
>>>>> I miss the point why clock-names cannot be fixed now. This is the name
>>>>> of property, not the input clock name.
>>>>
>>>> They can be fixed now. I've just aired the fixes at:
>>>> https://lore.kernel.org/linux-arm-kernel/20240109114908.3623645-1-tudor.ambarus@xxxxxxxxxx/
>>>>
>>>> Preparing v3 for this patch set to include the updated names here too.
>>>
>>> I think I was not that clear enough. I did not get your current patchset
>>> - so PERIC0 clock controller - cannot use new naming.
>>>
>>
>> Ok, I understand that the fixes from
>> https://lore.kernel.org/linux-arm-kernel/20240109114908.3623645-1-tudor.ambarus@xxxxxxxxxx/
>>
>> are NACK-ed and I shall use the full clock-names in this patch set as
>> well, thus "dout_cmu_peric0_bus", and "dout_cmu_peric0_ip". I don't mind
>> changing them back, will send a v4 using the full clock names.
> 
> They are not rejected, it is just independent thing. At least looks like
> independent.

The datasheet is not so verbose, but as I understand, CMU_MISC and
CMU_PERIC0 are clock domains of the same clock controller, thus I think
they should be treated the same. We should either get rid of the name of
the block in the clock names or keep it, but be consistent across all
the clock domains.
> 
>> Out of curiosity, why can't we change the names? All gs101 patches are
>> for v6.8, thus they haven't made a release yet. We still have the -rc
>> phase where we can fix things.
> 
> We can change. I would not bother that much with doing that, because I
> sent already them to arm-soc. That means I need to consider this as
> fixes and I just did not want to deal with it.
> 
> The question is quite different for a new clock controller - peric0.
> What parts of driver readability is affected by using "bus" name?
> 

As Peter pointed out, if keeping the shorter names, one would have to
cross reference with the device tree in order to determine which clock
is used, its type, whether it's a gate or a divider. Whereas if we keep
the full name, one can see what's the clock about with a glance. The
full name coincides with the clock names that are defined in the clock
driver, thus one can grep for the full name from the device tree and hit
the clock definition from the clock driver.

The cons of keeping the full name is that keeping the name of the block
in the DT's clock name is just redundant. Rob was clear and said that
including the block name in the -names is a pattern we don't want.

In what concerns my personal preference, I like the full name. At the
same time, I see Rob's point, and if that turns out to be a rule, let's
respect it. So I'm fine with both, but let's be consistent across the
driver and have the same clock name scheme for all the clock domains,
otherwise it will just look weird.

Thanks,
ta