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
