RE: [PATCH 2/2] ASoC: sma1307: Add bindings for Iron Device SMA1307 amplifier

[Ki-Seok Jo <kiseok.jo@xxxxxxxxxxxxxx>]

> On 13/08/2024 04:54, Kiseok Jo wrote:
> > Signed-off-by: Kiseok Jo <kiseok.jo@xxxxxxxxxxxxxx>

> Missing commit msg. Please order the patches as asked in submitting bindings - bindings before users.

> A nit, subject: drop second/last, redundant "bindings for". The "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

Thank you for the advise. I'll make the corrections and resend the patch with the changes. Like below:
ASoC: dt-bindings: irondevice,sma1307: Add sma1307 amplifier

> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC (and consider --no-git-fallback argument). It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel.

> Tools like b4 or scripts/get_maintainer.pl provide you proper list of people, so fix your workflow. Tools might also fail if you work on some ancient tree (don't, instead use mainline) or work on fork of kernel (don't, instead use mainline). Just use b4 and everything should be fine, although remember about `b4 prep --auto-to-cc` if you added new patches to the patchset.
> </form letter>

I sent the emails using get_maintainers.pl, but I'll try using b4 as you suggested. Thank you for the helpful information.

> > +  compatible:
> > +    enum:
> > +      - irondevice,sma1307a-w
> > +      - irondevice,sma1307a-f
> > +      - irondevice,sma1307aq-f
> > +    description:
> > +      It is divided according to the package.
> > +      The WLCSP packages are denoted with 'w', and the QFN packages are denoted
> > +      with 'f'. If a 'q' is added, it indicated the product is AEC-Q100
> > +      qualified for automotive applications.

> Package usually does not mean different compatibles. Aren't they all compatible? Or even the same?

They are all different products. For example, there is no sma1307aq-w, so I listed them all to distinguish between them. There are hardware differences between the products.

> > +
> > +  '#sound-dai-cells':
> > +    const: 1
> > +
> > +  use-binary:
> > +    description:
> > +      whether to use binary files for device settings.

> Drop property. You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc.

This is an option for choosing whether to use a binary file or default values when configuring the hardware, so I considered it to be related to hardware and included it in the device tree. If it is not appropriate as a device tree entry, I will consider moving it to sysfs.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#sound-dai-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        amplifier@1e {
> > +            compatible = "irondevice,sma1307a-w";
> > +            reg = <0x1e>;
> > +            #sound-dai-cells = <1>;
> > +            interrupt-parent = <&gpio>;
> > +            interrupts = <4 0>;

> Include proper header and use defines for flags.

I will remove the interrupt as it is not being used. If needed in the future, I will add the header for gpio.h.


Thank you for your kind feedback. I'll make the corrections and distribute the updated version.

Best regards,
Kiseok Jo