| Message ID | 20230202090715.18384-1-kiseok.jo@irondevice.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixed the schema binding according to test | expand |
Thank you for your patch. There is something to discuss/improve. On 02/02/2023 10:07, Kiseok Jo wrote: > Modified according to the writing-schema.rst file and tested. Use imperative, not past tense (Fixed->Fix, Modified->Modify). > > Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com> Use subject prefixes matching the subsystem (which you can get for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching). Therefore it should be: "ASoC: dt-bindings: irondevice,sma1303: Rework binding and add missing properties" > --- > .../bindings/sound/irondevice,sma1303.yaml | 46 +++++++++++++++++-- > 1 file changed, 43 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml b/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml > index 162c52606635..35d9a046ef75 100644 > --- a/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml > +++ b/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml > @@ -10,22 +10,62 @@ maintainers: > - Kiseok Jo <kiseok.jo@irondevice.com> > > description: > - SMA1303 digital class-D audio amplifier with an integrated boost converter. > + SMA1303 digital class-D audio amplifier > + with an integrated boost converter. > > allOf: > - - $ref: name-prefix.yaml# > + - $ref: dai-common.yaml# > + > +properties: > + compatible: > + enum: > + - irondevice,sma1303 > + > + reg: > + maxItems: 1 > + > + '#sound-dai-cells': > + const: 1 > + > + i2c-retry: > + description: number of retries for I2C regmap. Why do you need this? Why this fits the purpose of DT (or IOW why this differs between boards)? > + maximum: 49 > + default: 3 > + > + tdm-slot-rx: > + description: set the tdm rx start slot. Aren't you now re-writing dai-tdm-slot-rx-mask property? Same for tx below. > + maximum: 7 > + default: 0 > + > + tdm-slot-tx: > + description: set the tdm tx start slot. > + maximum: 7 > + default: 0 > + > + sys-clk-id: > + description: select the using system clock. What does it mean? Why do you need such property instead of clocks? > + default: 3 > > required: > - compatible > - reg > + - '#sound-dai-cells' > Best regards, Krzysztof
> Thank you for your patch. There is something to discuss/improve. > > On 02/02/2023 10:07, Kiseok Jo wrote: > > Modified according to the writing-schema.rst file and tested. > > Use imperative, not past tense (Fixed->Fix, Modified->Modify). Okay. I got it. I'll do that when I rewrite it. Thanks. > > > > Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com> > > Use subject prefixes matching the subsystem (which you can get for example > with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch > is touching). Therefore it should be: > "ASoC: dt-bindings: irondevice,sma1303: Rework binding and add missing > properties" > Oh, thank you for good information. I feel like I still lack a lot. I'll apply it. Thanks! > > > --- > > .../bindings/sound/irondevice,sma1303.yaml | 46 +++++++++++++++++-- > > 1 file changed, 43 insertions(+), 3 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml > > b/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml > > index 162c52606635..35d9a046ef75 100644 > > --- a/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml > > +++ b/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml > > @@ -10,22 +10,62 @@ maintainers: > > - Kiseok Jo <kiseok.jo@irondevice.com> > > > > description: > > - SMA1303 digital class-D audio amplifier with an integrated boost > converter. > > + SMA1303 digital class-D audio amplifier with an integrated boost > > + converter. > > > > allOf: > > - - $ref: name-prefix.yaml# > > + - $ref: dai-common.yaml# > > + > > +properties: > > + compatible: > > + enum: > > + - irondevice,sma1303 > > + > > + reg: > > + maxItems: 1 > > + > > + '#sound-dai-cells': > > + const: 1 > > + > > + i2c-retry: > > + description: number of retries for I2C regmap. > > Why do you need this? Why this fits the purpose of DT (or IOW why this > differs between boards)? When working with drivers on mulitiple platforms, there were cases where I2C did not work properly dpending on the AP or setting. So I made it possible to set a few retry settings, and then check or do other actions. Retry is performed only when I2C fails. And each device may have a different pull-up resisor or strength, so there may be differences between boards. Could that property be a problem? > > + maximum: 49 > > + default: 3 > > + > > + tdm-slot-rx: > > + description: set the tdm rx start slot. > > Aren't you now re-writing dai-tdm-slot-rx-mask property? Same for tx below. > It can be the same as audio DAI's tdm slot, I think but there are cases where it is set differently, so I omitted it separately. > > + maximum: 7 > > + default: 0 > > + > > + tdm-slot-tx: > > + description: set the tdm tx start slot. > > + maximum: 7 > > + default: 0 > > + > > + sys-clk-id: > > + description: select the using system clock. > > What does it mean? Why do you need such property instead of clocks? This can receive an external clock, but it can use internal clock. Should I write all the clock descriptions in case? > > + default: 3 > > > > required: > > - compatible > > - reg > > + - '#sound-dai-cells' > > > > Best regards, > Krzysztof Thank you for quick response! I'll take a look to see if there are any other issues. Thanks. Best regards, Kiseok Jo
On 02/02/2023 10:55, Ki-Seok Jo wrote: >> Thank you for your patch. There is something to discuss/improve. >> >> On 02/02/2023 10:07, Kiseok Jo wrote: >>> Modified according to the writing-schema.rst file and tested. >> >> Use imperative, not past tense (Fixed->Fix, Modified->Modify). > > Okay. I got it. I'll do that when I rewrite it. Thanks. > >>> >>> Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com> >> >> Use subject prefixes matching the subsystem (which you can get for example >> with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch >> is touching). Therefore it should be: >> "ASoC: dt-bindings: irondevice,sma1303: Rework binding and add missing >> properties" >> > > Oh, thank you for good information. I feel like I still lack a lot. > I'll apply it. Thanks! > >> >>> --- >>> .../bindings/sound/irondevice,sma1303.yaml | 46 +++++++++++++++++-- >>> 1 file changed, 43 insertions(+), 3 deletions(-) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml >>> b/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml >>> index 162c52606635..35d9a046ef75 100644 >>> --- a/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml >>> +++ b/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml >>> @@ -10,22 +10,62 @@ maintainers: >>> - Kiseok Jo <kiseok.jo@irondevice.com> >>> >>> description: >>> - SMA1303 digital class-D audio amplifier with an integrated boost >> converter. >>> + SMA1303 digital class-D audio amplifier with an integrated boost >>> + converter. >>> >>> allOf: >>> - - $ref: name-prefix.yaml# >>> + - $ref: dai-common.yaml# >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - irondevice,sma1303 >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + '#sound-dai-cells': >>> + const: 1 >>> + >>> + i2c-retry: >>> + description: number of retries for I2C regmap. >> >> Why do you need this? Why this fits the purpose of DT (or IOW why this >> differs between boards)? > > When working with drivers on mulitiple platforms, there were cases where > I2C did not work properly dpending on the AP or setting. > So I made it possible to set a few retry settings, and then check or do > other actions. Retry is performed only when I2C fails. > > And each device may have a different pull-up resisor or strength, > so there may be differences between boards. None of I2C drivers need it (except SBS battery), so it should not be needed here. If you have wrong pin setup, this one should be corrected instead. > > Could that property be a problem? > >>> + maximum: 49 >>> + default: 3 >>> + >>> + tdm-slot-rx: >>> + description: set the tdm rx start slot. >> >> Aren't you now re-writing dai-tdm-slot-rx-mask property? Same for tx below. >> > > It can be the same as audio DAI's tdm slot, I think but there are cases > where it is set differently, so I omitted it separately. Unfortunately I still do not understand why do you need it. Use generic DAI/TDM properties. > >>> + maximum: 7 >>> + default: 0 >>> + >>> + tdm-slot-tx: >>> + description: set the tdm tx start slot. >>> + maximum: 7 >>> + default: 0 >>> + >>> + sys-clk-id: >>> + description: select the using system clock. >> >> What does it mean? Why do you need such property instead of clocks? > > This can receive an external clock, but it can use internal clock. > Should I write all the clock descriptions in case? How do you configure and enable external clock with this property? I don't see it. If the device has clock input, this should be "clocks". If it is omitted, then internal clock is used. Best regards, Krzysztof
> > > > + > > > > + i2c-retry: > > > > + description: number of retries for I2C regmap. > > > > > > Why do you need this? Why this fits the purpose of DT (or IOW why > > > this differs between boards)? > > > > When working with drivers on mulitiple platforms, there were cases > > where I2C did not work properly dpending on the AP or setting. > > So I made it possible to set a few retry settings, and then check or > > do other actions. Retry is performed only when I2C fails. > > > > And each device may have a different pull-up resisor or strength, so > > there may be differences between boards. > > None of I2C drivers need it (except SBS battery), so it should not be > needed here. If you have wrong pin setup, this one should be corrected > instead. Okay, I agree. It doesn't seem necessary. I'll remove this property. Thanks! > > > > + tdm-slot-rx: > > > > + description: set the tdm rx start slot. > > > > > > Aren't you now re-writing dai-tdm-slot-rx-mask property? Same for tx > below. > > > > > > > It can be the same as audio DAI's tdm slot, I think but there are > > cases where it is set differently, so I omitted it separately. > Unfortunately I still do not understand why do you need it. Use generic > DAI/TDM properties. Looking back, it seems to be the same as the dai-tdm-slot-rx-mask. It seems to be correct to map the amp function in the driver. However, I looked for the ones used in other amplifier drivers, Most of them were added to the user control using mixer. Come to think of it, there are many cases where the TDM slot is changed after the driver is connected. > > > > + sys-clk-id: > > > > + description: select the using system clock. > > > > > > What does it mean? Why do you need such property instead of clocks? > > > > This can receive an external clock, but it can use internal clock. > > Should I write all the clock descriptions in case? > > How do you configure and enable external clock with this property? I don't > see it. If the device has clock input, this should be "clocks". If it is > omitted, then internal clock is used. > Basically, this value is set with set_sysclk in the dai operations. So, I also get the clk_id from this function and set it. From the point of view of the codec driver, there are case where the machine driver does not give this value(clk_id). So this is a property that can set an initial value. It's not requied value. All three properties mentioned above are optional. So, if it need to be removed, it's okay. But I think it seems correct to register the second item(tdm slot) as a mixer. Thank you for your good feeback! Best regards, Kiseok Jo
On 03/02/2023 06:06, Ki-Seok Jo wrote: >>>>> + sys-clk-id: >>>>> + description: select the using system clock. >>>> >>>> What does it mean? Why do you need such property instead of clocks? >>> >>> This can receive an external clock, but it can use internal clock. >>> Should I write all the clock descriptions in case? >> >> How do you configure and enable external clock with this property? I don't >> see it. If the device has clock input, this should be "clocks". If it is >> omitted, then internal clock is used. >> > > Basically, this value is set with set_sysclk in the dai operations. > So, I also get the clk_id from this function and set it. > From the point of view of the codec driver, there are case where the machine > driver does not give this value(clk_id). It's entirely different discussion. You did not document the clocks/values for it and just wrote "select the using", so like a "bool" property. You need bindings documenting the clocks. Use the same name as here: https://lore.kernel.org/all/20221022162742.21671-2-aidanmacdonald.0x0@gmail.com/ Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml b/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml index 162c52606635..35d9a046ef75 100644 --- a/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml +++ b/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml @@ -10,22 +10,62 @@ maintainers: - Kiseok Jo <kiseok.jo@irondevice.com> description: - SMA1303 digital class-D audio amplifier with an integrated boost converter. + SMA1303 digital class-D audio amplifier + with an integrated boost converter. allOf: - - $ref: name-prefix.yaml# + - $ref: dai-common.yaml# + +properties: + compatible: + enum: + - irondevice,sma1303 + + reg: + maxItems: 1 + + '#sound-dai-cells': + const: 1 + + i2c-retry: + description: number of retries for I2C regmap. + maximum: 49 + default: 3 + + tdm-slot-rx: + description: set the tdm rx start slot. + maximum: 7 + default: 0 + + tdm-slot-tx: + description: set the tdm tx start slot. + maximum: 7 + default: 0 + + sys-clk-id: + description: select the using system clock. + default: 3 required: - compatible - reg + - '#sound-dai-cells' additionalProperties: false examples: - | - i2c_bus { + i2c { + #address-cells = <1>; + #size-cells = <0>; + amplifier@1e { compatible = "irondevice,sma1303"; reg = <0x1e>; + #sound-dai-cells = <1>; + i2c-retry = <5>; + tdm-slot-rx = <0>; + tdm-slot-tx = <2>; + sys-clk-id = <3>; }; };
Modified according to the writing-schema.rst file and tested. Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com> --- .../bindings/sound/irondevice,sma1303.yaml | 46 +++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) base-commit: e33d4c4f1e2de74cfea556d75eef0886d5b7d472