Re: [PATCH v3] sound/soc/lapis: add platform driver for ML7213

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1: Type: text/plain, Size: 2669 bytes --]

On Thu, Feb 23, 2012 at 02:46:50PM +0900, Tomoya MORINAGA wrote:
> This driver is for LAPIS Semiconductor ML7213 IOH I2S.

I just merged Lars-Peter's dmaengine library code which has been on the
list for a week or so - this should be updated to use that next time
it's posted.  That should save a lot of code from the driver and make
sure it's following best practices for dmaengine use.

> +static struct ioh_i2s_data *i2s_data;
> +static struct ioh_i2s_dma dmadata[MAX_I2S_CH];

Why are these needed, aren't they dynamically allocated by the driver?

> +	case SNDRV_PCM_STREAM_CAPTURE:
> +		offset =\
> +		    ioh_rtd->dma->rx_cur_period * ioh_rtd->dma->period_bytes;

Drop the continuations, line breaks are just whitespace in C outside of
macros and strings.

> +	case SNDRV_PCM_FORMAT_S32_LE:
> +		byte = 24;
> +		break;

That looks wrong...  are you sure you don't support S24_LE or something?

> +		switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> +		case SND_SOC_DAIFMT_NB_NF:
> +			cmn_reg[i] &= ~ML7213I2S_BCLKPOL;
> +			break;
> +		default:
> +			return -EINVAL;

Looking at that code I suspect at least IB_NF is supported too...

> +static int ml7213i2s_dai_set_clkdiv(struct snd_soc_dai *dai,
> +				  int div_id, int div)
> +{

> +	switch (div_id) {
> +	case ML7213IOH_BCLKFS0:

This all looks like BCLK/sample calculations that the driver should
really be able to figure out for itself rather than forcing every user
to replicate the code to do the calculation, calculation in hw_params
would be more normal.

> +#ifdef CONFIG_PM
> +static void ioh_i2s_save_reg_conf(void)

This stuff has exactly one caller, just inline it.  It's also *very*
suspicious that it's not taking an argument specifying the device...

> +#else
> +#define ml7213i2s_soc_suspend NULL
> +#define spdif_soc_resume NULL

Hrm?

> +#ifdef CONFIG_PM
> +	.suspend = ml7213i2s_soc_suspend,
> +	.resume = ml7213i2s_soc_resume,
> +#endif

The whole point with defining the functions to NULL above is to avoid
the ifdef here.

> +static struct platform_driver ioh_i2s_driver_plat = {

> +static struct platform_driver ioh_dai_driver_plat = {

> +static int ioh_i2s_pci_probe(struct pci_dev *pdev,
> +			     const struct pci_device_id *id)

Why are you creating these platform devices?  I don't understand the
function they serve.  The code handling them looks to have quite a few
problems but I'm not clear they should be there in the first place.

> +	rv = request_irq(pdev->irq, ioh_i2s_irq, IRQF_SHARED, "ml7213_ioh",
> +			 pdev);
> +	if (rv != 0) {
> +		printk(KERN_ERR "Failed to allocate irq\n");
> +		goto out_irq;
> +	}

Are you *sure* you're ready to handle interrupts at this point?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]