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);
Thanks,
Charles
