Re: [PATCH 1/2] ASoC: dt-bindings: Add adi,ssm3515 amp schema

[Martin Povišer]

Hello Conor,

> On 9. 5. 2023, at 20:25, Conor Dooley <conor@xxxxxxxxxx> wrote:
> 
> Hey Martin,
> 
> On Tue, May 09, 2023 at 06:38:27PM +0200, Martin Povišer wrote:
>> Add a DT schema for the SSM3515 amp by Analog Devices. It's a simple
>> mono amplifier with digital input.
>> 
>> Signed-off-by: Martin Povišer <povik+lin@xxxxxxxxxxx>
>> ---
>> .../bindings/sound/adi,ssm3515.yaml           | 66 +++++++++++++++++++
>> 1 file changed, 66 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/sound/adi,ssm3515.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/sound/adi,ssm3515.yaml b/Documentation/devicetree/bindings/sound/adi,ssm3515.yaml
>> new file mode 100644
>> index 000000000000..19b7185ae8e2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/adi,ssm3515.yaml
>> @@ -0,0 +1,66 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2019-20 Texas Instruments Incorporated
> 
> Copyright here looks a little odd, copy & paste from the TI bindings you
> send patches for earlier, or intended?

Yeah, that must have been a copypaste from the other schemas. Not intended
in any case. Thanks for the catch.

> 
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/sound/adi,ssm3515.yaml#";
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
> 
> Drop the quotes on these please.

OK

>> +
>> +  adi,ana-gain:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [0, 1, 2, 3]
>> +    description: |
>> +      The value to be set in the ANA_GAIN register field on the codec. This determines
>> +      the full voltage span of the codec's analog output.
>> +
>> +      To quote the datasheet on the available options:
>> +
>> +        00: 8.4 V Full-Scale Gain Mapping
>> +        01: 12.6 V Full-Scale Gain Mapping
>> +        10: 14 V Full-Scale Gain Mapping
>> +        11: 15 V Full-Scale Gain Mapping
> 
> Putting register values into the DT does not seem correct, although
> I know nothing about sound and will have to defer to Krzysztof & Co. on
> that front.

It’s an analog gain selection, it just seemed most honest and convenient
to defer to the register field in describing of what this property does.

I thought this should be here, and not with the other gain controls to be
exported to userspace, since it relates to the supply voltage. You probably
can’t select a gain value close or over the amp’s supply and expect proper
functioning, so the value here should be the system’s integrator choice.

On second thought there doesn’t seem to be any risk of damage to the amp
itself, so the knowledge of what’s the optimal value could be left up to
ALSA use-case configuration, and there’s no real downside in treating it
like other gain controls.

I think I will remove it in the next iteration.

> Cheers,
> Conor.

Cheers, Martin