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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 2 Mar 2012 16:39:15 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Tomoya MORINAGA <tomoya.rohm@...il.com>
Cc:	Liam Girdwood <lrg@...com>, Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.de>, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org, qi.wang@...el.com,
	yong.y.wang@...el.com, joel.clark@...el.com, kok.howg.ewe@...el.com
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?

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)