| Message ID | 20240314232201.2102178-14-jan.dakinevich@salutedevices.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce support of audio for Amlogic A1 SoC family | expand |
On 15/03/2024 00:21, Jan Dakinevich wrote: > This option allow to redefine the rate of DSP system clock. And why is it suitable for bindings? Describe the hardware, not what you want to do in the driver. > > Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> > --- > Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml > index df21dd72fc65..d2f23a59a6b6 100644 > --- a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml > +++ b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml > @@ -40,6 +40,10 @@ properties: > resets: > maxItems: 1 > > + sysrate: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: redefine rate of DSP system clock No vendor prefix, so is it a generic property? Also, missing unit suffix, but more importantly I don't understand why this is a property of hardware. Best regards, Krzysztof
On 3/15/24 13:00, Krzysztof Kozlowski wrote: > On 15/03/2024 00:21, Jan Dakinevich wrote: >> This option allow to redefine the rate of DSP system clock. > > And why is it suitable for bindings? Describe the hardware, not what you > want to do in the driver. > What do you mean? I am adding some new property and should describe it in dt-bindinds. Isn't it? >> >> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >> --- >> Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >> index df21dd72fc65..d2f23a59a6b6 100644 >> --- a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >> +++ b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >> @@ -40,6 +40,10 @@ properties: >> resets: >> maxItems: 1 >> >> + sysrate: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: redefine rate of DSP system clock > > No vendor prefix, so is it a generic property? Also, missing unit > suffix, but more importantly I don't understand why this is a property > of hardware. > Answered in next message. > Best regards, > Krzysztof >
On 17/03/2024 16:55, Jan Dakinevich wrote: > > > On 3/15/24 13:00, Krzysztof Kozlowski wrote: >> On 15/03/2024 00:21, Jan Dakinevich wrote: >>> This option allow to redefine the rate of DSP system clock. >> >> And why is it suitable for bindings? Describe the hardware, not what you >> want to do in the driver. >> > > What do you mean? I am adding some new property and should describe it > in dt-bindinds. Isn't it? No, if the property is not suitable for bindings, you should not add it in the first place. So again: explain what sort of hardware, not driver, problem you are solving here, so we can understand why do you need new property. Otherwise use existing properties or no properties, because we do not define all possible clocks in the bindings. Let's be clear: with such commit msg explanation as you have, my answer is: no, driver should set clock frequency and you do not need this property at all. Best regards, Krzysztof
On 3/17/24 19:27, Krzysztof Kozlowski wrote: > On 17/03/2024 16:55, Jan Dakinevich wrote: >> >> >> On 3/15/24 13:00, Krzysztof Kozlowski wrote: >>> On 15/03/2024 00:21, Jan Dakinevich wrote: >>>> This option allow to redefine the rate of DSP system clock. >>> >>> And why is it suitable for bindings? Describe the hardware, not what you >>> want to do in the driver. >>> >> >> What do you mean? I am adding some new property and should describe it >> in dt-bindinds. Isn't it? > > No, if the property is not suitable for bindings, you should not add it > in the first place. So again: explain what sort of hardware, not driver, > problem you are solving here, so we can understand why do you need new > property. Otherwise use existing properties or no properties, because we > do not define all possible clocks in the bindings. > > Let's be clear: with such commit msg explanation as you have, my answer > is: no, driver should set clock frequency and you do not need this > property at all. > Could you please take a look on answer to "Jerome Brunet <jbrunet@baylibre.com>"'s message on the same thread. There, I am trying to explain what I am solving by this commit. I would be happy to avoid this w/a, but currently this is the best I came up with. > Best regards, > Krzysztof >
On 17/03/2024 17:35, Jan Dakinevich wrote: > > > On 3/17/24 19:27, Krzysztof Kozlowski wrote: >> On 17/03/2024 16:55, Jan Dakinevich wrote: >>> >>> >>> On 3/15/24 13:00, Krzysztof Kozlowski wrote: >>>> On 15/03/2024 00:21, Jan Dakinevich wrote: >>>>> This option allow to redefine the rate of DSP system clock. >>>> >>>> And why is it suitable for bindings? Describe the hardware, not what you >>>> want to do in the driver. >>>> >>> >>> What do you mean? I am adding some new property and should describe it >>> in dt-bindinds. Isn't it? >> >> No, if the property is not suitable for bindings, you should not add it >> in the first place. So again: explain what sort of hardware, not driver, >> problem you are solving here, so we can understand why do you need new >> property. Otherwise use existing properties or no properties, because we >> do not define all possible clocks in the bindings. >> >> Let's be clear: with such commit msg explanation as you have, my answer >> is: no, driver should set clock frequency and you do not need this >> property at all. >> > > Could you please take a look on answer to "Jerome Brunet > <jbrunet@baylibre.com>"'s message on the same thread. There, I am trying > to explain what I am solving by this commit. How is this answer here? You asked "What do you mean", so apparently you did not understand why I am responding and why you cannot just document whatever you wish, because that "whatever you wish" is not correct. I explained that but now you respond that I should read other part of emails. Really? So again, do you understand that commit msg should provide rationale why you think this describes hardware and why this is suitable for bindings? Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml index df21dd72fc65..d2f23a59a6b6 100644 --- a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml +++ b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml @@ -40,6 +40,10 @@ properties: resets: maxItems: 1 + sysrate: + $ref: /schemas/types.yaml#/definitions/uint32 + description: redefine rate of DSP system clock + required: - compatible - reg
This option allow to redefine the rate of DSP system clock. Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> --- Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml | 4 ++++ 1 file changed, 4 insertions(+)