[alsa-devel] [PATCH 4/8] sound:asoc:spdif_in: Add spdif IN support

[Rajeev kumar]

Hello Mark,

On 3/20/2012 9:25 PM, Mark Brown wrote:
> On Tue, Mar 20, 2012 at 05:03:48PM +0530, Rajeev Kumar wrote:
>
> This looks good, a few minor things but almost good to go.
>
>> This patch implements the spdif IN driver for ST peripheral
>
> S/PDIF.
>

Ok,


>> +	if (irq_status&  SPDIF_IRQ_FIFOWRITE)
>> +		pr_err("spdif in: fifo write error\n");
>> +	if (irq_status&  SPDIF_IRQ_EMPTYFIFOREAD)
>> +		pr_err("spdif in: empty fifo read error\n");
>> +	if (irq_status&  SPDIF_IRQ_FIFOFULL)
>> +		pr_err("spdif in: fifo full error\n");
>> +	if (irq_status&  SPDIF_IRQ_OUTOFRANGE)
>> +		pr_err("spdif in: out of range error\n");
>
> dev_err().
>

Ok

>> +	if (!devm_request_mem_region(&pdev->dev, res->start,
>> +				resource_size(res), pdev->name)) {
>> +		dev_warn(&pdev->dev, "Failed to get memory resourse\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	host = devm_kzalloc(&pdev->dev, sizeof(*host), GFP_KERNEL);
>> +	if (!host) {
>> +		dev_warn(&pdev->dev, "kzalloc fail\n");
>> +		return -ENOMEM;
>> +	}
>
> Good to see this - this is the sort of stuff I was looking for in the
> I2S driver.
>

Will follow this in I2S driver.

>> +	host->clk = clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(host->clk))
>> +		return PTR_ERR(host->clk);
>
>> +	pdata = dev_get_platdata(&pdev->dev);
>
> Should really be error checking in case you didn't get your platform
> data.
>

Agreed,

>> +	ret = devm_request_irq(&pdev->dev, host->irq, spdif_in_irq, 0,
>> +			"spdif-in", host);
>> +	if (ret) {
>
> I'm really not enthused about the idea of using devm_request_irq() here
> - what steps are you taking to make sure that the IRQ can't possibly
> fire after you've started tearing down the device.  In general it's
> relatively hard to use devm_request_irq() safely.
>

Disabling interrupt in remove will insure that interrupt will not fire 
after tearing down the device.


>> +#define SPDIF_IN_DEV_PM_OPS NULL
>
> Just remove this if it's unconditionally empty.
>

OK,

Best Regards
Rajeev

>> +static int __init spdif_in_init(void)
>
> module_platform_driver().