Hi Wolfram,
Thank you for the review.
On Mon, Dec 20, 2021 at 10:16 AM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> > + if (!np) {
>
> Very minor nit: Maybe 'if (np)' and switch the blocks? Positive logic is
> a tad easier to read.
>
OK will update it for v2.
> > + struct resource *res;
> > + resource_size_t n;
> > +
> > + while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> > + for (n = res->start; n <= res->end; n++) {
> > + ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> > + 0, dev_name(&dev->dev), pd);
> > + if (ret) {
> > + dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> > + return ret;
> > + }
> > + }
> > + k++;
> > + }
>
> Yeah, it is good to keep the legacy block as is.
>
> > + do {
> > + irq = platform_get_irq_optional(dev, k);
> > + if (irq <= 0 && irq != -ENXIO)
> > + return irq ? irq : -ENXIO;
> > + if (irq == -ENXIO)
> > + break;
> > + ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
> > + 0, dev_name(&dev->dev), pd);
> > if (ret) {
> > - dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> > + dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
> > return ret;
> > }
> > - }
> > - k++;
> > + k++;
> > + } while (irq);
>
> In addition to the 'irq == 0' case from patch 1, I tried to shorten the
> block for the np-case. I only came up with this. The assigntment and
> comparison of the while-argument is not exactly pretty, but the block
> itself is easier to read. I'll let you decide.
>
> while (irq = platform_get_irq_optional(dev, k) != -ENXIO) {
> if (irq < 0)
> return irq;
>
> ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
> 0, dev_name(&dev->dev), pd);
> if (ret) {
> dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
> return ret;
> }
> k++;
> }
>
> Only brainstorming, not even build tested.
>
LGTM, I'll give that a shot.
Cheers,
Prabhakar
