On 14.11.2023 22:29, Krzysztof Kozlowski wrote:
> On 14/11/2023 21:05, Alisa-Dariana Roman wrote:
>> Document properties used for clock configuration.
>
> Some background here is missing - otherwise it looks like you are adding
> new properties...
>
>>
>> Signed-off-by: Alisa-Dariana Roman <[email protected]>
>> ---
>> .../devicetree/bindings/iio/adc/adi,ad7192.yaml | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>> index 16def2985ab4..9b59d6eea368 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>> @@ -80,6 +80,16 @@ properties:
>> and when chop is disabled.
>> type: boolean
>>
>> + adi,clock-xtal:
>> + description: |
>> + External crystal connected from MCLK1 to MCLK2.
>
> And this should be input clock.
>
>> + type: boolean
>> +
>> + adi,int-clock-output-enable:
>> + description: |
>> + Internal 4.92 MHz clock available on MCLK2 pin.
>> + type: boolean
>
> This should be clock-cells and clock provider.
>
> Unless you are just documenting already used interface which you do not
> want to break...
>
>> +
>> bipolar:
>> description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
>> type: boolean
>
> Best regards,
> Krzysztof
>
Thank you very much for the feedback!
If I understand correctly, there is already an input clock in the bindings:
```
clocks:
maxItems: 1
description: phandle to the master clock (mclk)
clock-names:
items:
- const: mclk
```
What I wanted to accomplish with this patch is to document these boolean
properties (from the ad7192 driver code):
```
/* use internal clock */
if (!st->mclk) {
if (device_property_read_bool(dev, "adi,int-clock-output-enable"))
clock_sel = AD7192_CLK_INT_CO;
} else {
if (device_property_read_bool(dev, "adi,clock-xtal"))
clock_sel = AD7192_CLK_EXT_MCLK1_2;
else
clock_sel = AD7192_CLK_EXT_MCLK2;
}
```
Please let me know how to proceed further!
Kind regards,
Alisa-Dariana Roman
On 02/02/2024 15:14, Alisa-Dariana Roman wrote:
> On 14.11.2023 22:29, Krzysztof Kozlowski wrote:
>> On 14/11/2023 21:05, Alisa-Dariana Roman wrote:
>>> Document properties used for clock configuration.
>>
>> Some background here is missing - otherwise it looks like you are adding
>> new properties...
>>
>>>
>>> Signed-off-by: Alisa-Dariana Roman <[email protected]>
>>> ---
>>> .../devicetree/bindings/iio/adc/adi,ad7192.yaml | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> index 16def2985ab4..9b59d6eea368 100644
>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> @@ -80,6 +80,16 @@ properties:
>>> and when chop is disabled.
>>> type: boolean
>>>
>>> + adi,clock-xtal:
>>> + description: |
>>> + External crystal connected from MCLK1 to MCLK2.
>>
>> And this should be input clock.
>>
>>> + type: boolean
>>> +
>>> + adi,int-clock-output-enable:
>>> + description: |
>>> + Internal 4.92 MHz clock available on MCLK2 pin.
>>> + type: boolean
>>
>> This should be clock-cells and clock provider.
>>
>> Unless you are just documenting already used interface which you do not
>> want to break...
>>
>>> +
>>> bipolar:
>>> description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
>>> type: boolean
>>
>> Best regards,
>> Krzysztof
>>
>
> Thank you very much for the feedback!
>
> If I understand correctly, there is already an input clock in the bindings:
You tell me...
>
> What I wanted to accomplish with this patch is to document these boolean
> properties (from the ad7192 driver code):
Please explain with clear words: do you mean that existing upstream
Linux driver has it? I don't care about some out of tree drivers...
> ```
> /* use internal clock */
> if (!st->mclk) {
> if (device_property_read_bool(dev, "adi,int-clock-output-enable"))
> clock_sel = AD7192_CLK_INT_CO;
> } else {
> if (device_property_read_bool(dev, "adi,clock-xtal"))
> clock_sel = AD7192_CLK_EXT_MCLK1_2;
> else
> clock_sel = AD7192_CLK_EXT_MCLK2;
> }
> ```
>
> Please let me know how to proceed further!
Please open the datasheet of your product and add properties matching
the hardware, not driver.
I don't know what to say more except the same - you want to enable clock
provided from this device on some pin to some other devices? If yes,
then this is clock-provider, so you clock-provider bindings I mentioned
in my first reply.
Best regards,
Krzysztof
On Fri, Feb 2, 2024 at 8:14 AM Alisa-Dariana Roman
<[email protected]> wrote:
>
> On 14.11.2023 22:29, Krzysztof Kozlowski wrote:
> > On 14/11/2023 21:05, Alisa-Dariana Roman wrote:
> >> Document properties used for clock configuration.
> >
> > Some background here is missing - otherwise it looks like you are adding
> > new properties...
> >
> >>
> >> Signed-off-by: Alisa-Dariana Roman <[email protected]>
> >> ---
> >> .../devicetree/bindings/iio/adc/adi,ad7192.yaml | 10 ++++++++++
> >> 1 file changed, 10 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> >> index 16def2985ab4..9b59d6eea368 100644
> >> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> >> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> >> @@ -80,6 +80,16 @@ properties:
> >> and when chop is disabled.
> >> type: boolean
> >>
> >> + adi,clock-xtal:
> >> + description: |
> >> + External crystal connected from MCLK1 to MCLK2.
A better description could be:
When this flag is present, it indicates that the clock from the clocks
property is a crystal oscillator connected to MCLK1 and MCLK2. When
omitted the clock is a CMOS-compatible clock connected to MCLK2.
> >
> > And this should be input clock.
> >
> >> + type: boolean
> >> +
> >> + adi,int-clock-output-enable:
> >> + description: |
> >> + Internal 4.92 MHz clock available on MCLK2 pin.
> >> + type: boolean
> >
> > This should be clock-cells and clock provider.
> >
> > Unless you are just documenting already used interface which you do not
> > want to break...
This property is already used in the mainline Linux driver, so sounds
like the "don't want to break it" case. But it would make sense to
deprecate this property and use standard clock provider bindings
instead.
> >
> >> +
> >> bipolar:
> >> description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
> >> type: boolean
> >
> > Best regards,
> > Krzysztof
> >
>
> Thank you very much for the feedback!
>
> If I understand correctly, there is already an input clock in the bindings:
> ```
> clocks:
> maxItems: 1
> description: phandle to the master clock (mclk)
>
> clock-names:
> items:
> - const: mclk
> ```
>
> What I wanted to accomplish with this patch is to document these boolean
> properties (from the ad7192 driver code):
> ```
> /* use internal clock */
> if (!st->mclk) {
> if (device_property_read_bool(dev, "adi,int-clock-output-enable"))
> clock_sel = AD7192_CLK_INT_CO;
> } else {
> if (device_property_read_bool(dev, "adi,clock-xtal"))
> clock_sel = AD7192_CLK_EXT_MCLK1_2;
> else
> clock_sel = AD7192_CLK_EXT_MCLK2;
> }
> ```
>
> Please let me know how to proceed further!
>
> Kind regards,
> Alisa-Dariana Roman
>
There was another recent discussion about this exact same clock
input/output on another chip recently [1]. So it would be nice if we
could end up with the same bindings in both cases (cc Ceclan). In the
other thread, it was proposed to have the clocks property to be an
array of two phandles, one for the crystal oscillator and one for the
external clock rather than a single clock and the adi,clock-xtal
property. But that would be a breaking change to these bindings.
[1]: https://lore.kernel.org/linux-iio/20240122-bloating-dyslexic-cbc0258c898a@spud/t/#m4e375aa36dae6da0c319518137f03e2f63e72af9
On 02/02/2024 23:20, David Lechner wrote:
>>>
>>> And this should be input clock.
>>>
>>>> + type: boolean
>>>> +
>>>> + adi,int-clock-output-enable:
>>>> + description: |
>>>> + Internal 4.92 MHz clock available on MCLK2 pin.
>>>> + type: boolean
>>>
>>> This should be clock-cells and clock provider.
>>>
>>> Unless you are just documenting already used interface which you do not
>>> want to break...
>
> This property is already used in the mainline Linux driver, so sounds
> like the "don't want to break it" case. But it would make sense to
> deprecate this property and use standard clock provider bindings
> instead.
One could think like that, but what type of process would it create?
Send driver changes WITHOUT binding, then document whatever crap you
have saying "Linux already supports it!".
Whenever such argument is used, I am repeating the same.
Let's be more clear: NAK if this is clock provider and the only argument
is that someone sneaked undocumented interface bypassing review.
Best regards,
Krzysztof
On Mon, 2024-02-05 at 10:28 +0100, Krzysztof Kozlowski wrote:
> On 02/02/2024 23:20, David Lechner wrote:
> > > >
> > > > And this should be input clock.
> > > >
> > > > > + type: boolean
> > > > > +
> > > > > + adi,int-clock-output-enable:
> > > > > + description: |
> > > > > + Internal 4.92 MHz clock available on MCLK2 pin.
> > > > > + type: boolean
> > > >
> > > > This should be clock-cells and clock provider.
> > > >
> > > > Unless you are just documenting already used interface which you do not
> > > > want to break...
> >
> > This property is already used in the mainline Linux driver, so sounds
> > like the "don't want to break it" case. But it would make sense to
> > deprecate this property and use standard clock provider bindings
> > instead.
>
> One could think like that, but what type of process would it create?
> Send driver changes WITHOUT binding, then document whatever crap you
> have saying "Linux already supports it!".
>
> Whenever such argument is used, I am repeating the same.
>
> Let's be more clear: NAK if this is clock provider and the only argument
> is that someone sneaked undocumented interface bypassing review.
>
Fair enough...
Alisa,
I guess you have two alternatives then:
1) Drop this patch;
2) Add proper clock provider properties.
I would obviously go with 2). You can then take care of backward compatibility in the
driver. Like, if clock-cells is present, ignore the legacy properties and properly
export the clocks we have in the device.
- Nuno Sá