Hi Laurent,
On Tue, Dec 31, 2013 at 12:38 AM, Laurent Pinchart
<[email protected]> wrote:
> On Tuesday 24 December 2013 12:40:43 Geert Uytterhoeven wrote:
>> + int irq[MAX_NUM_IRQ];
>
> Should this field be called irqs ?
OK.
>> + unsigned int numirq;
>
> This driver seems to use underscores to separate words in variable names,
> maybe you could use num_irq (or num_irqs) instead.
OK.
>> + int i, ret = 0;
>
> i is an unsigned loop index, could you please make it an unsigned int ?
OK.
>> @@ -948,6 +948,19 @@ static int rspi_probe(struct platform_device *pdev)
>> rspi->ops = ops;
>> rspi->master = master;
>>
>> + for (i = 0; i < MAX_NUM_IRQ; i++) {
>> + irq = platform_get_irq(pdev, i);
>> + if (irq < 0) {
>> + if (rspi->numirq)
>> + break;
>> + dev_err(&pdev->dev, "platform_get_irq error\n");
>> + ret = -ENODEV;
>> + goto error1;
>> + }
>> + rspi->irq[i] = irq;
>> + rspi->numirq++;
>> + }
>> +
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> rspi->addr = devm_ioremap_resource(&pdev->dev, res);
>> if (IS_ERR(rspi->addr)) {
>> @@ -979,14 +992,16 @@ static int rspi_probe(struct platform_device *pdev)
>> master->transfer = rspi_transfer;
>> master->cleanup = rspi_cleanup;
>>
>> - ret = devm_request_irq(&pdev->dev, irq, rspi_irq, 0,
>> - dev_name(&pdev->dev), rspi);
>> - if (ret < 0) {
>> - dev_err(&pdev->dev, "request_irq error\n");
>> - goto error1;
>> + for (i = 0; i < rspi->numirq; i++) {
>> + ret = devm_request_irq(&pdev->dev, rspi->irq[i], rspi_irq, 0,
>> + dev_name(&pdev->dev), rspi);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "request_irq %d error\n",
>> + rspi->irq[i]);
>> + goto error1;
>> + }
>> }
>
> Just an idea, what about combining the two loops ?
I was a bit concerned about dependencies and cleanup, but indeed, it seems
to be possible.
>> --- a/include/linux/spi/rspi.h
>> +++ b/include/linux/spi/rspi.h
>> @@ -1,7 +1,7 @@
>> /*
>> * Renesas SPI driver
>> *
>> - * Copyright (C) 2012 Renesas Solutions Corp.
>> + * Copyright (C) 2012, 2013 Renesas Solutions Corp.
>> *
>> * This program is free software; you can redistribute it and/or modify
>> * it under the terms of the GNU General Public License as published by
>
> This change seems to be unrelated. You might want to squash it into patch 1/8.
I'll move it to the first patch that actually adds code to rspi.h.
Thanks for your comments!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
