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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

Best regards,
Krzysztof




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux