Re: [PATCH] ASoC: add driver for max9768 amplifier

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1.1: Type: text/plain, Size: 1633 bytes --]


> > +static int max9768_write(struct snd_soc_codec *codec, unsigned int reg,
> > +		unsigned int value)
> 
> Use regmap for register I/O please.

OK, currently negotiating this with Lars-Peter.

> > +	if (max9768->flags & MAX9768_FLAG_CLASSIC_PWM) {
> > +		ret = snd_soc_write(codec, MAX9768_CTRL, MAX9768_CTRL_PWM);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> Some documentation on what this does would be nice.  Is this something
> that might usefully be changed at runtime?

It only depends on the hardware design. Will a pointer to the section of the
datasheet be enough for documentation?

> > +	ret = snd_soc_add_controls(codec, max9768_volume,
> > +			ARRAY_SIZE(max9768_volume));
> > +	if (ret)
> > +		return ret;
> 
> Use the controls field in the driver.  I'd also expect to see some sort

Understood.

> of DAPM support, even if it's just mapping out the pins.  Since you're
> manually controlling the mute it'd be nice to make the unmute be (mute
> && power) to save on pops and clicks from the CODEC being amplified.

Sorry, not understood. Is there an example of what you mean here somewhere in
the tree?

> > +		err = gpio_request_one(pdata->mute_gpio, GPIOF_INIT_HIGH, "MAX9768 Mute");
> > +		max9768->mute_gpio = err ?: pdata->mute_gpio;
> 
> I really don't like the ternery operator at the best of times...

Can we agree to disagree? It is so convenient here and saves a few lines.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]