RE: [PATCH v6 1/2] ASoC: codecs: add support for ES8389

[Zhang Yi]

> >   	imply SND_SOC_ES8316
> >   	imply SND_SOC_ES8323
> >   	imply SND_SOC_ES8326
> > +	imply SND_SOC_ES8389
> >   	imply SND_SOC_ES8328_SPI
> >   	imply SND_SOC_ES8328_I2C
> >   	imply SND_SOC_ES7134
> 
> Add it in alphabetical order?

I'll fix the issue about the order of this couple of places.

> > diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c new 
> > file mode 100644 index 000000000000..73c1966c30be
> > --- /dev/null
> > +++ b/sound/soc/codecs/es8389.c
> > @@ -0,0 +1,966 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * es8389.c  --  ES8389/ES8390 ALSA SoC Audio Codec
> 
> The only place that mentions ES8390? Is this correct?

Actually ES8389 and ES8390 are the same chip.I'll drop the ES8390 in this place
to avoid any misunderstandings.

> > +	{50, 400000, 8000, 0x00, 0x75, 0x05, 0xC8, 0x01, 0xC1, 0x90, 0x10, 0x00, 0x18, 0xC7, 0xD0, 0xC0, 0x8F, 0xC7, 0x01, 0x12, 0x00, 0x09, 0x19, 0x07},
> > +	{600, 4800000, 8000, 0x05, 0x65, 0x25, 0xF9, 0x00, 0xD1, 0x90, 0x10, 0x00, 0x18, 0xC7, 0xD0, 0xC0, 0x8F, 0xC7, 0x01, 0x12, 0x00, 0x09, 0x19, 0x07},
> > +	{1500, 12000000, 8000, 0x0E, 0x25, 0x25, 0xE8, 0x00, 0xD1, 0x90, 0x40, 0x00, 0x31, 0xC7, 0xC5, 0x00, 0x8F, 0xC7, 0x01, 0x12, 0x00, 0x09, 0x19, 0x07},
> > +	{2400, 19200000, 8000, 0x0B, 0x01, 0x00, 0xD0, 0x00, 0xD1, 0x80, 0x90, 0x00, 0x31, 0xC7, 0xC5, 0x00, 0xC7, 0xC7, 0x00, 0x12, 0x00, 0x09, 0x19, 0x07},
> > +	{3000, 24000000, 8000, 0x0E, 0x24, 0x05, 0xD0, 0x00, 0xC2, 0x80, 0xC0, 0x00, 0x31, 0xC7, 0xC5, 0x00, 0x8F, 0xC7, 0x01, 0x12, 0x00, 0x09, 0x19, 0x07},
> > +	{3250, 26000000, 8000, 0x40, 0x05, 0xA4, 0xC0, 0x00, 0xD1, 0x80, 
> > +0xD0, 0x00, 0x31, 0xC7, 0xC5, 0x00, 0xC7, 0xC7, 0x00, 0x12, 0x00, 
> > +0x09, 0x19, 0x07},
> > +
> > +};
> 
> Missing new line between array above and below function? I think checkpatch warns about this?

I'll fix it

> > +
> > +static int es8389_probe(struct snd_soc_component *codec) {
> > +	int ret = 0;
> > +	struct es8389_private *es8389 = 
> > +snd_soc_component_get_drvdata(codec);
> 
> Use "reverse christmas tree" notation for declaring variables? Also ret gets overwritten below, so it is useless to set it.

I'll fix it

> > +static const struct regmap_config es8389_regmap = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +
> > +	.max_register = ES8389_MAX_REGISTER,
> > +
> > +	.volatile_reg = es8389_volatile_register,
> > +	.cache_type = REGCACHE_MAPLE,
> > +};
> > +
> > +static void es8389_i2c_shutdown(struct i2c_client *i2c) {
> > +	struct snd_soc_component *component;
> > +	struct es8389_private *es8389;
> > +
> > +	es8389 = i2c_get_clientdata(i2c);
> > +	component = es8389->component;
> 
> Unused variable? And while you are removing it, you can also remove it from 'struct es8389_private', as this place seems to be the only user.

Yes, I'll drop them

> > +
> > +	regmap_write(es8389->regmap, ES8389_MASTER_MODE, 0x28);
> > +	regmap_write(es8389->regmap, ES8389_HPSW, 0x00);
> > +	regmap_write(es8389->regmap, ES8389_VMID, 0x00);
> > +	regmap_write(es8389->regmap, ES8389_RESET, 0x00);
> > +	regmap_write(es8389->regmap, ES8389_CSM_JUMP, 0xCC);
> > +	usleep_range(500000, 550000);//500MS
> > +	regmap_write(es8389->regmap, ES8389_CSM_JUMP, 0x00);
> > +	regmap_write(es8389->regmap, ES8389_ANA_CTL1, 0x08);
> > +	regmap_write(es8389->regmap, ES8389_ISO_CTL, 0xC1);
> > +	regmap_write(es8389->regmap, ES8389_PULL_DOWN, 0x00); }
> > +
> > +static int es8389_i2c_probe(struct i2c_client *i2c_client) {
> > +	struct es8389_private *es8389;
> > +	int ret = -1;
> 
> No need to set ret as it will be overwritten anyway, and '-1' is not considered a proper value. Use some error code if you must.

Ok