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

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

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

> > +struct max9768 {
> > +	struct i2c_client *client;
> > +	int mute_gpio;
> > +	int shdn_gpio;
> 
> The shutdown gpio is requested, but never used.

It is initialized to HIGH, otherwise the chip won't do a thing. The pin can be
later used for PM, but I don't need that now.

> > +	ret = snd_soc_cache_write(codec, reg, value);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	data = (reg << 6) | value;
> 
> 
> Adding support for this register type to regmap and use regmap then is the
> way to go. regmap should also be used for the cache instead of the legacy
> ASoC cache.

Okay, are you already at it? Otherwise I'll try adding this tomorrow.

> > +	ret = snd_soc_add_controls(codec, max9768_volume,
> > +			ARRAY_SIZE(max9768_volume));
> > +	if (ret)
> > +		return ret;
> 
> Use the controls/num_controls fields of your codec_driver to register these
> controls.

OK.

> > +static struct snd_soc_codec_driver soc_codec_dev_max9768 = {
> 
> max9768_codec_driver would be a better name.

OK.

> > +	return snd_soc_register_codec(&client->dev, &soc_codec_dev_max9768, NULL, 0);
> 
> GPIOs are not freed in case of an error.
> 
> > +}
> > +
> > +static int __devexit max9768_i2c_remove(struct i2c_client *client)
> > +{
> > +	snd_soc_unregister_codec(&client->dev);
> 
> 
> GPIOs are never freed.

Eeeks, right, damn. Thanks!

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 --]