Re: [PATCH v2 1/2] dt-binding: clock: cs2600: Add support for the CS2600

[Krzysztof Kozlowski <krzk@kernel.org>]

On 19/12/2024 17:22, Richard Fitzgerald wrote:
> On 19/12/24 15:57, Krzysztof Kozlowski wrote:
>> On 19/12/2024 14:36, Richard Fitzgerald wrote:
>>> On 19/12/24 13:16, Krzysztof Kozlowski wrote:
>>>> On 19/12/2024 13:59, Charles Keepax wrote:
>>>>> On Thu, Dec 19, 2024 at 12:39:38PM +0100, Krzysztof Kozlowski wrote:
>>>>>> On 19/12/2024 12:02, Charles Keepax wrote:
>>>>>>> On Thu, Dec 19, 2024 at 09:51:00AM +0100, Krzysztof Kozlowski wrote:
>>>>>>>> On Wed, Dec 18, 2024 at 08:46:30PM -0600, Paul Handrigan wrote:
>>>>>>>>> +/* CS2600 Auxiliary Output */
>>>>>>>>> +#define CS2600_AUX_OUTPUT_FREQ_UNLOCK	0
>>>>>>>>> +#define CS2600_AUX_OUTPUT_PHASE_UNLOCK	1
>>>>>>>>> +#define CS2600_AUX_OUTPUT_NO_CLKIN	2
>>>>>>>>
>>>>>>>> I still don't see why these three are supposed to be bindings. Drop
>>>>>>>> them.
>>>>>>>
>>>>>>> In a binding one would presumably do:
>>>>>>>
>>>>>>> cirrus,aux-output-source = <CS2600_AUX_OUTPUT_FREQ_UNLOCK>;
>>>>>>>
>>>>>>> Apologies but I don't quite understand what you mean by the values
>>>>>>> are not used in the binding? The driver reads the property and sets
>>>>>>
>>>>>> There is no user of these defines, so not a binding.
>>>>>>
>>>>>>> the pin to have the appropriate function. Admittedly one could drop
>>>>>>
>>>>>> It's not a proof that this is a binding.
>>>>>>
>>>>>>> the defines and then DTS would just have to do:
>>>>>>>
>>>>>>> cirrus,aux-output-source = <0>;
>>>>>>>
>>>>>>> But that feels a bit less helpful when reading the binding.
>>>>>>
>>>>>> Binding and being helpful are two different things. This to be the
>>>>>> binding, it has to be used as a binding, so some translation layer
>>>>>> between driver and DTS. It must have an user in DTS. I keep repeating
>>>>>> this over and over...
>>>>>>
>>>>>
>>>>> Apologies, but I not sure I totally follow this, and apologies if
>>>>> you have already explained this are there some docs I can look
>>>>> at?
>>>>>
>>>>> I think you are saying because these defines merely represent the
>>>>> valid values for a device tree property and are not translated
>>>>> into different values you can't put defines for them in the binding
>>>>> header?
>>>>>
>>>>> So this would not be allowed:
>>>>>
>>>>>     #define CS2600_AUX_OUTPUT_FREQ_UNLOCK 0
>>>>>
>>>>>     cirrus,aux-output-source = <CS2600_AUX_OUTPUT_FREQ_UNLOCK>;
>>>>>
>>>>>     device_property_read_u32(dev, "cirrus,aux-output-source", &val);
>>>>>     regmap_write(regmap, CS2600_OUTPUT_CFG2, val);
>>>>>
>>>>> But this would be fine:
>>>>>
>>>>>     #define CS2600_AUX_OUTPUT_FREQ_UNLOCK 1
>>>>>
>>>>>     cirrus,aux-output-source = <CS2600_AUX_OUTPUT_FREQ_UNLOCK>;
>>>>>
>>>>>     device_property_read_u32(dev, "cirrus,aux-output-source", &val);
>>>>>     switch (val) {
>>>>>     case CS2600_AUX_OUTPUT_FREQ_UNLOCK:
>>>>>       regmap_write(regmap, CS2600_OUTPUT_CFG2, 0);
>>>>>     }
>>>>>
>>>>> And this would also be fine?
>>>>>
>>>>>     cirrus,aux-output-source = <0>;
>>>>>
>>>>>     device_property_read_u32(dev, "cirrus,aux-output-source", &val);
>>>>>     regmap_write(regmap, CS2600_OUTPUT_CFG2, val);
>>>>>
>>>> Yes. If you want to use in DTS user-readable values, then use string.
>>>>
>>>
>>> I don't understand this. Why should we have to use a string value for
>>> something that only needs a simple integer value? Why can't we define
>>> constants with meaningful names?
>>
>> You can and you will find plenty examples of this, but as I explained
>> earlier - this is not a binding. We avoid defining as a binding
>> something which is not a binding.
>>
> 
> What does that mean?
> Perhaps if you clearly explained what the problem is and what you want
> us to change instead of making cryptic statements like "this is not a
> binding" we wouldn't have to waste all this time exchanging emails that
> aren't getting anywhere.
> 
> You didn't explain earlier. You typed some words earlier, but they
> failed to explain, and you are continuing to fail to explain.
Mentioned defines do not meet criteria of a binding, because of reasons
I already stated very clear, so do not try to make them a binding.

You got also pretty clear answer from Charles what to do, so what is
unclear here? Drop these defines and use register values or better
strings. Or fix them to be really a binding (and then come with a reason
why these are bindings).

Best regards,
Krzysztof