Re: [PATCH 2/2] ASoC: codecs: Add Awinic AW8738 audio amplifier driver

[Stephan Gerhold <stephan@gerhold.net>]

On Tue, Mar 01, 2022 at 01:45:01PM +0000, Mark Brown wrote:
> On Tue, Mar 01, 2022 at 01:37:42PM +0100, Stephan Gerhold wrote:
> 
> > The Awinic AW8738 is a simple audio amplifier using an enable GPIO.
> > The main difference to simple-amplifier is that there is a "one-wire
> > pulse control" that allows configuring the amplifier to one of a few
> > pre-defined modes.
> 
> What exactly are the modes here?  Looking at the web site for the part
> it seems like it's selecting a power limit for the speaker so it makes
> sense that the mode would be fixed in DT but it's not clear from the
> driver.
> 

It seems to be mostly a power limit but not only. E.g. on AW8738 mode
3/4 and 5/6 seem to have the same power limit but select between a
"NCN function" or "Multi-Level AGC function", which seems to control
how the amplifier behaves if the power limit is reached.

The exact effect of the modes varies greatly between different Awinic
parts, but since I don't really see a use case for changing those
options dynamically I think it's best to just load it from DT.

> > +	aw->gpiod_enable = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> > +	if (IS_ERR(aw->gpiod_enable))
> > +		return dev_err_probe(dev, PTR_ERR(aw->gpiod_enable),
> > +				     "Failed to get 'enable' gpio");
> 
> Are we sure that enable is the best name for this pin?  It's more
> complex than just an enable since it's the 1 wire data - according to
> what's on the awinic web site it looks like the actual label is /SHDN
> which is similarly misleading though :/

Yeah, I was considering to call it "shdn" instead but given the negation
that seemed even more confusing. I ended up using "enable" since this is
the name used in the mode table of the datasheet (which will probably be
the main reference when setting up the amplifier in the DT).

Thanks,
Stephan