[alsa-devel] [PATCH 1/4] ASoC sst: Add msic codec driver

[Koul, Vinod]

> 
> > This patch adds the msic asoc codec driver. This driver currently supports
> only playback.
> > Capture and jack detection to be added later
> 
> This looks mostly OK, but you probably want to run it through
> checkpatch.pl.  For quite a few of the debug prints you've got here
> you're replicating stuff that's in the core as standard - if you turn on
> debug logs from the core you should get equivalent logging.
Yes we have run checkpatch. Currently yes we have quite a few logs, we will
remove them later when we add the capture, jack detection etc. It would be
nice to have these logs on.

> Some more specific issues below:
> 
> > +	switch (params_format(params)) {
> > +	case SNDRV_PCM_FORMAT_S16_LE:
> > +		format = 3;
> > +		break;
> > +
> > +	case SNDRV_PCM_FORMAT_S24_LE:
> > +		format = 0;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	snd_soc_update_bits(dai->codec, MSIC_PCM2C2, BIT(4)|BIT(5), format);
> 
> This won't work - both the mask and value for snd_soc_update_bits() are
> bitmasks so I'd expect the format for S16 to be 0x18 with that bitmask.
> 
> > +	snd_soc_update_bits(dai->codec, MSIC_PCM1C1, BIT(7), rate);
Will fix

> Similarly here.
> 
> > +	/*PCM interface */
> > +	msic_write(codec, MSIC_PCM2RXSLOT01, 0x10);
> > +	msic_write(codec, MSIC_PCM2RXSLOT23, 0x32);
> > +	msic_write(codec, MSIC_PCM2RXSLOT45, 0x54);
> 
> For these it'd be better if you could provide some comments explaining
> what the configuration that's being set is.  It's hard to tell if this
> is stuff that should be managed by the driver or not.
Sure


> 
> > +static struct platform_driver intelmid_codec_driver = {
> > +	.driver		= {
> > +		.name		= "mid-msic-codec",
> 
> > +MODULE_ALIAS("msic-codec");
> 
> This should be platform:mid-msic-codec.
Okay

~Vinod