[alsa-devel] [PATCH] ASoC: nau8810: Add driver for Nuvoton codec chip NAU88C10

[Mark Brown]

On Fri, Jun 17, 2016 at 04:40:34PM +0800, John Hsu wrote:

> +static int nau8810_reg_write(void *context, unsigned int reg,
> +			      unsigned int value)
> +{
> +	struct i2c_client *client = context;
> +	uint8_t buf[2];
> +	__be16 *out = (void *)buf;
> +	int ret;
> +
> +	*out = cpu_to_be16((reg << 9) | value);
> +	ret = i2c_master_send(client, buf, sizeof(buf));

...

> +	reg_buf = (uint8_t)(reg << 1);
> +	xfer[0].addr = client->addr;
> +	xfer[0].len = sizeof(reg_buf);
> +	xfer[0].buf = &reg_buf;
> +	xfer[0].flags = 0;
> +
> +	xfer[1].addr = client->addr;
> +	xfer[1].len = sizeof(val_buf);
> +	xfer[1].buf = (uint8_t *)&val_buf;
> +	xfer[1].flags = I2C_M_RD;

This looks like a regmap with 7 bit registers and 9 bit value.  Why
aren't we just using the standard regmap support for this?

> +static const struct soc_enum nau8810_enum[] = {
> +	SOC_ENUM_SINGLE(NAU8810_REG_COMP, NAU8810_ADCCM_SFT,
> +		ARRAY_SIZE(nau8810_companding), nau8810_companding),

Don't define a big array of enums, this makes the code both hard to read
and error prone when we index into the array by hard coded raw numbers.
Just define each enum as a variable and then reference it by address
like we do for enums in other drivers.

> +	SOC_SINGLE("Digital Loopback Switch", NAU8810_REG_COMP,
> +		NAU8810_ADDAP_SFT, 1, 0),

This looks like it should be a DAPM control.

> +	SOC_SINGLE_TLV("Playback Gain", NAU8810_REG_DACGAIN,
> +		NAU8810_DACGAIN_SFT, 0xff, 0, digital_tlv),

All volume controls should have names ending in Volume so that userspace
tools can know how to presen them.

> +static int nau8810_set_clkdiv(struct snd_soc_dai *dai, int div_id, int div)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +	struct nau8810 *nau8810 = snd_soc_codec_get_drvdata(codec);
> +	int ret = 0;
> +
> +	nau8810->div_id = div_id;
> +	if (div_id != NAU8810_MCLK_DIV_MCLK)
> +		/* Defer the master clock prescaler configuration to DAI
> +		 * hardware parameter if master clock from MCLK because
> +		 * it needs runtime fs information to get the proper div.
> +		 */
> +		ret = nau8810_config_clkdiv(nau8810, div, 0);
> +
> +	return ret;
> +}

You shouldn't be implementing new set_clkdiv() operations, there's no
point in having each machine driver figure out the internal clocking of
the device.  Just specify the clocks coming into the device and have
the driver figure out what to do with them.

> +static int nau8810_probe(struct snd_soc_codec *codec)
> +{
> +	struct nau8810 *nau8810 = snd_soc_codec_get_drvdata(codec);
> +
> +	regmap_write(nau8810->regmap, NAU8810_REG_RESET, 0x00);

This will break the regmap cache if the driver is ever rebound to a
card, do this once on I2C probe.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20160627/b7341945/attachment.sig>