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().