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

[Krzysztof Kozlowski <krzk@kernel.org>]

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.

Best regards,
Krzysztof