| Message ID | 20240903054435.2659-3-kiseok.jo@irondevice.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add a driver for the Iron Device SMA1307 Amp | expand |
On Tue, Sep 03, 2024 at 02:44:34PM +0900, Kiseok Jo wrote: > Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com> Empty commit? Read submitting-patches. Please run scripts/checkpatch.pl and fix reported warnings. Then please run and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. > --- Where is the changelog? What happened in v2? Why are you sending the same patch? > .../bindings/sound/irondevice,sma1307.yaml | 54 +++++++++++++++++++ > 1 file changed, 54 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > > diff --git a/Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml b/Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > new file mode 100644 > index 000000000..0bb4ee664 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > @@ -0,0 +1,54 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/irondevice,sma1307.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Iron Device SMA1307 Audio Amplifier > + > +maintainers: > + - Kiseok Jo <kiseok.jo@irondevice.com> > + > +description: > + SMA1307 boosted digital speaker amplifier > + with feedback-loop. > + > +allOf: > + - $ref: dai-common.yaml# > + > +properties: > + compatible: > + enum: > + - irondevice,sma1307a > + - irondevice,sma1307aq > + description: > + If a 'q' is added, it indicated the product is AEC-Q100 > + qualified for automotive applications. SMA1307A supports > + both WLCSP and QFN packages. However, SMA1307AQ only > + supports the QFN package. Difference is only in package bin? That does not warrant new compatible. Best regards, Krzysztof
> > On Tue, Sep 03, 2024 at 02:44:34PM +0900, Kiseok Jo wrote: > > Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com> > > Empty commit? Read submitting-patches. > Okay I'll add next patch. > Please run scripts/checkpatch.pl and fix reported warnings. Then please run > and (probably) fix more warnings. > Some warnings can be ignored, especially from --strict run, but the code here > looks like it needs a fix. Feel free to get in touch if the warning is not > clear. > When I checked, I didn't encounter any errors or warnings when using 'checkpatch.pl'. What options might be needed? > > --- > > Where is the changelog? What happened in v2? Why are you sending the same > patch? > I understand that the file has not been applied yet, so it needs to be rewritten again until applied. > > .../bindings/sound/irondevice,sma1307.yaml | 54 +++++++++++++++++++ > > 1 file changed, 54 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > > b/Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > > new file mode 100644 > > index 000000000..0bb4ee664 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > > @@ -0,0 +1,54 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/sound/irondevice,sma1307.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Iron Device SMA1307 Audio Amplifier > > + > > +maintainers: > > + - Kiseok Jo <kiseok.jo@irondevice.com> > > + > > +description: > > + SMA1307 boosted digital speaker amplifier > > + with feedback-loop. > > + > > +allOf: > > + - $ref: dai-common.yaml# > > + > > +properties: > > + compatible: > > + enum: > > + - irondevice,sma1307a > > + - irondevice,sma1307aq > > + description: > > + If a 'q' is added, it indicated the product is AEC-Q100 > > + qualified for automotive applications. SMA1307A supports > > + both WLCSP and QFN packages. However, SMA1307AQ only > > + supports the QFN package. > > Difference is only in package bin? That does not warrant new compatible. > No, they are different chips, so different settings. The settings are automatically configured according to each chip. > Best regards, > Krzysztof Thank you for your detailed review. It has been very helpful!
On 03/09/2024 10:39, Ki-Seok Jo wrote: >> >> On Tue, Sep 03, 2024 at 02:44:34PM +0900, Kiseok Jo wrote: >>> Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com> >> >> Empty commit? Read submitting-patches. >> > > Okay I'll add next patch. > > >> Please run scripts/checkpatch.pl and fix reported warnings. Then please run >> and (probably) fix more warnings. >> Some warnings can be ignored, especially from --strict run, but the code here >> looks like it needs a fix. Feel free to get in touch if the warning is not >> clear. >> > > When I checked, I didn't encounter any errors or warnings when using 'checkpatch.pl'. > What options might be needed? That's not true and I am not happy that I need to prove to you obvious thing. You do not need any options. Look: WARNING: Missing commit description - Add an appropriate one You could at least now double check if reviewer pointed it out instead of immediately disagreeing with review. > > >>> --- >> >> Where is the changelog? What happened in v2? Why are you sending the same >> patch? >> > > I understand that the file has not been applied yet, so it needs to be rewritten again until applied. > Read submitting patches. Best regards, Krzysztof
> > On 03/09/2024 10:39, Ki-Seok Jo wrote: > >> > >> On Tue, Sep 03, 2024 at 02:44:34PM +0900, Kiseok Jo wrote: > >>> Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com> > >> > >> Empty commit? Read submitting-patches. > >> > > > > Okay I'll add next patch. > > > > > >> Please run scripts/checkpatch.pl and fix reported warnings. Then > >> please run and (probably) fix more warnings. > >> Some warnings can be ignored, especially from --strict run, but the > >> code here looks like it needs a fix. Feel free to get in touch if the > >> warning is not clear. > >> > > > > When I checked, I didn't encounter any errors or warnings when using > 'checkpatch.pl'. > > What options might be needed? > > That's not true and I am not happy that I need to prove to you obvious thing. > You do not need any options. Look: > > WARNING: Missing commit description - Add an appropriate one > > You could at least now double check if reviewer pointed it out instead of > immediately disagreeing with review. > I have no intention of opposing the content. I am asking again because I didn't receive any warnings when I did the following, and I suspect I might have done something wrong. ./scripts/checkpatch.pl Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml total: 0 errors, 0 warnings, 54 lines checked Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml has no obvious style problems and is ready for submission. I was under the impression that this only applied to patched files as described above. It turns out it can also be used with patch files. Thank you for the useful information! Thanks for your help!
On 03/09/2024 11:08, Ki-Seok Jo wrote: >> >> On 03/09/2024 10:39, Ki-Seok Jo wrote: >>>> >>>> On Tue, Sep 03, 2024 at 02:44:34PM +0900, Kiseok Jo wrote: >>>>> Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com> >>>> >>>> Empty commit? Read submitting-patches. >>>> >>> >>> Okay I'll add next patch. >>> >>> >>>> Please run scripts/checkpatch.pl and fix reported warnings. Then >>>> please run and (probably) fix more warnings. >>>> Some warnings can be ignored, especially from --strict run, but the >>>> code here looks like it needs a fix. Feel free to get in touch if the >>>> warning is not clear. >>>> >>> >>> When I checked, I didn't encounter any errors or warnings when using >> 'checkpatch.pl'. >>> What options might be needed? >> >> That's not true and I am not happy that I need to prove to you obvious thing. >> You do not need any options. Look: >> >> WARNING: Missing commit description - Add an appropriate one >> >> You could at least now double check if reviewer pointed it out instead of >> immediately disagreeing with review. >> > > I have no intention of opposing the content. I am asking again because I didn't receive any warnings when I did the following, and I suspect I might have done something wrong. > > > ./scripts/checkpatch.pl Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > > total: 0 errors, 0 warnings, 54 lines checked > > Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml has no obvious style problems and is ready for submission. > > I was under the impression that this only applied to patched files as described above. It turns out it can also be used with patch files. Thank you for the useful information! > That's not how you run checkpatch. You run it on the patch. Please read submitting-patches document. It explains everything. Best regards, Krzysztof
On Tue, Sep 03, 2024 at 11:14:56AM +0200, Krzysztof Kozlowski wrote: > On 03/09/2024 11:08, Ki-Seok Jo wrote: > > I have no intention of opposing the content. I am asking again because I didn't receive any warnings when I did the following, and I suspect I might have done something wrong. > > ./scripts/checkpatch.pl Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > > I was under the impression that this only applied to patched files as described above. It turns out it can also be used with patch files. Thank you for the useful information! > That's not how you run checkpatch. You run it on the patch. Please read > submitting-patches document. It explains everything. If you *do* want to run checkpatch on a file for some reason (eg, so you can fix issues in existing code) you can use the --file option for that: ./scripts/checkpatch.pl --file Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml Also I don't know if you're aware of b4 but it's pretty handy for automating a bunch of the procedural stuff around submitting patches, one of the things it can help with is running checkpatch over your series: https://b4.docs.kernel.org/en/latest/
> > > > I have no intention of opposing the content. I am asking again because I > didn't receive any warnings when I did the following, and I suspect I might > have done something wrong. > > > > > > ./scripts/checkpatch.pl > > Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > > > > total: 0 errors, 0 warnings, 54 lines checked > > > > Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml has no > obvious style problems and is ready for submission. > > > > I was under the impression that this only applied to patched files as > described above. It turns out it can also be used with patch files. Thank you > for the useful information! > > > > That's not how you run checkpatch. You run it on the patch. Please read > submitting-patches document. It explains everything. > > Best regards, > Krzysztof Hi, I am in the process of carefully incorporating your feedback and making the necessary revisions. May I kindly ask you a question, if it's not too much trouble? When running checkpatch, what would be the best way to address the following warning? WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?) #21: create mode 100644 Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml In this case, would it be better for me to add a line break in the patch file, or should I leave it as is? WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #65: new file mode 100644 If the warning is appearing because it's a new file, is it something that can be safely ignored, or should I make changes to the MAINTAINERS file? Thank you for your feedback. I am learning a lot of new things! Best regards, Kiseok Jo
On Wed, Sep 11, 2024 at 01:16:01AM +0000, Ki-Seok Jo wrote: > > > > > > I have no intention of opposing the content. I am asking again because I > > didn't receive any warnings when I did the following, and I suspect I might > > have done something wrong. > > > > > > > > > ./scripts/checkpatch.pl > > > Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > > > > > > total: 0 errors, 0 warnings, 54 lines checked > > > > > > Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml has no > > obvious style problems and is ready for submission. > > > > > > I was under the impression that this only applied to patched files as > > described above. It turns out it can also be used with patch files. Thank you > > for the useful information! > > > > > > > That's not how you run checkpatch. You run it on the patch. Please read > > submitting-patches document. It explains everything. > > > > Best regards, > > Krzysztof > > > Hi, > > I am in the process of carefully incorporating your feedback and making the necessary revisions. > > May I kindly ask you a question, if it's not too much trouble? > When running checkpatch, what would be the best way to address the following warning? > > WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?) > #21: > create mode 100644 Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > > In this case, would it be better for me to add a line break in the patch file, or should I leave it as is? Normally I would say you can ignore this, and that checkpatch doesn't usually complain about the actually git output in here - but I think checkpatch "broke" because you did not provide any commit message body at all, so it starting parsing the git output instead. You need to write a body! > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? > #65: > new file mode 100644 > > If the warning is appearing because it's a new file, is it something that can be safely ignored, or should I make changes to the MAINTAINERS file? > > Thank you for your feedback. I am learning a lot of new things! Usually for bindings, which have maintainers listed in them, you can skip adding a MAINTAINERS entry. Cheers, Conor.
> > > > > > Hi, > > > > I am in the process of carefully incorporating your feedback and making the > necessary revisions. > > > > May I kindly ask you a question, if it's not too much trouble? > > When running checkpatch, what would be the best way to address the following > warning? > > > > WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit > > description?) > > #21: > > create mode 100644 > > Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml > > > > In this case, would it be better for me to add a line break in the patch > file, or should I leave it as is? > > Normally I would say you can ignore this, and that checkpatch doesn't usually > complain about the actually git output in here - but I think checkpatch > "broke" because you did not provide any commit message body at all, so it > starting parsing the git output instead. You need to write a body! > > > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? > > #65: > > new file mode 100644 > > > > If the warning is appearing because it's a new file, is it something that > can be safely ignored, or should I make changes to the MAINTAINERS file? > > > > Thank you for your feedback. I am learning a lot of new things! > > Usually for bindings, which have maintainers listed in them, you can skip > adding a MAINTAINERS entry. > > Cheers, > Conor. Thank you for your answer! I'll make sure to add a commit message next time. I'll go over and fix the erros again.
diff --git a/Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml b/Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml new file mode 100644 index 000000000..0bb4ee664 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml @@ -0,0 +1,54 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/irondevice,sma1307.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Iron Device SMA1307 Audio Amplifier + +maintainers: + - Kiseok Jo <kiseok.jo@irondevice.com> + +description: + SMA1307 boosted digital speaker amplifier + with feedback-loop. + +allOf: + - $ref: dai-common.yaml# + +properties: + compatible: + enum: + - irondevice,sma1307a + - irondevice,sma1307aq + description: + If a 'q' is added, it indicated the product is AEC-Q100 + qualified for automotive applications. SMA1307A supports + both WLCSP and QFN packages. However, SMA1307AQ only + supports the QFN package. + + reg: + maxItems: 1 + + '#sound-dai-cells': + const: 1 + +required: + - compatible + - reg + - '#sound-dai-cells' + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + amplifier@1e { + compatible = "irondevice,sma1307a"; + reg = <0x1e>; + #sound-dai-cells = <1>; + }; + };
Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com> --- .../bindings/sound/irondevice,sma1307.yaml | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/irondevice,sma1307.yaml