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?