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

[Krzysztof Kozlowski <krzk@kernel.org>]

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.

Best regards,
Krzysztof