Messages in this thread | | | Date | Fri, 2 Mar 2012 16:39:15 +0000 | From | Mark Brown <> | Subject | Re: [PATCH v3] sound/soc/lapis: add platform driver for ML7213 |
| |
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? [unhandled content-type:application/pgp-signature] | |