From: Brendan Higgins <brendanhiggins@google.com> To: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: "Marc Zyngier" <marc.zyngier@arm.com>, "Wolfram Sang" <wsa@the-dreams.de>, robh+dt@kernel.org, mark.rutland@arm.com, tglx@linutronix.de, jason@lakedaemon.net, "Joel Stanley" <joel@jms.id.au>, "Vladimir Zapolskiy" <vz@mleia.com>, "Kachalov Anton" <mouse@mayc.ru>, "Cédric Le Goater" <clg@kaod.org>, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, "OpenBMC Maillist" <openbmc@lists.ozlabs.org> Subject: Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed Date: Wed, 29 Mar 2017 02:59:53 -0700 [thread overview] Message-ID: <CAFd5g46TAGHWtH7p1o=AJqy8_SygpJZ4RJkJpUwDD3nKxpB5YQ@mail.gmail.com> (raw) In-Reply-To: <1490734216.3177.140.camel@kernel.crashing.org> The main reason I took this approach is just because I thought it was cleaner from the perspective of the busses which are totally independent (except for the fact that they share a single hardware interrupt). I did not make any measurements, so I doubt that I have anything to add that you don't already know. I saw other usages of chained interrupts that do the same thing (scan a "status" register and use them to make software interrupts) and I thought that is basically what the dummy irq chip code is for. The only thing I thought I was doing that was novel was actually breaking out the dummy irqchip into its own driver; it is not my idea, but I do think makes it a lot cleaner. Nevertheless, it should be cheap in terms of number of instructions; the most expensive part looks like looking up the mapping. In any case, I think the low hanging fruit here is supporting buffering or DMA, like Ben suggested. To address the comment on being over engineered: outside of the init function (which would exist regardless of how we do this, if not here then in the I2C driver); the code is actually pretty small and generic. All that being said, it would not be very hard to do this without using the dummy irqchip code and it would definitely be smaller in terms of indirection and space used, but I think the code would actually be more complicated to read. We would be going back to having an I2C controller along with the I2C busses; where all the I2C controller does is read the IRQ register and then call the appropriate bus irq handler, which looks a lot like a dummy irqchip. Cheers
WARNING: multiple messages have this Message-ID (diff)
From: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> To: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> Cc: "Marc Zyngier" <marc.zyngier-5wv7dgnIgG8@public.gmane.org>, "Wolfram Sang" <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org, jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org, "Joel Stanley" <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>, "Vladimir Zapolskiy" <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>, "Kachalov Anton" <mouse-Pma6HLj0uuo@public.gmane.org>, "Cédric Le Goater" <clg-Bxea+6Xhats@public.gmane.org>, linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "OpenBMC Maillist" <openbmc-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org> Subject: Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed Date: Wed, 29 Mar 2017 02:59:53 -0700 [thread overview] Message-ID: <CAFd5g46TAGHWtH7p1o=AJqy8_SygpJZ4RJkJpUwDD3nKxpB5YQ@mail.gmail.com> (raw) In-Reply-To: <1490734216.3177.140.camel-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> The main reason I took this approach is just because I thought it was cleaner from the perspective of the busses which are totally independent (except for the fact that they share a single hardware interrupt). I did not make any measurements, so I doubt that I have anything to add that you don't already know. I saw other usages of chained interrupts that do the same thing (scan a "status" register and use them to make software interrupts) and I thought that is basically what the dummy irq chip code is for. The only thing I thought I was doing that was novel was actually breaking out the dummy irqchip into its own driver; it is not my idea, but I do think makes it a lot cleaner. Nevertheless, it should be cheap in terms of number of instructions; the most expensive part looks like looking up the mapping. In any case, I think the low hanging fruit here is supporting buffering or DMA, like Ben suggested. To address the comment on being over engineered: outside of the init function (which would exist regardless of how we do this, if not here then in the I2C driver); the code is actually pretty small and generic. All that being said, it would not be very hard to do this without using the dummy irqchip code and it would definitely be smaller in terms of indirection and space used, but I think the code would actually be more complicated to read. We would be going back to having an I2C controller along with the I2C busses; where all the I2C controller does is read the IRQ register and then call the appropriate bus irq handler, which looks a lot like a dummy irqchip. Cheers -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-03-29 9:59 UTC|newest] Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-03-28 5:12 [PATCH v6 0/5] i2c: aspeed: added driver for Aspeed I2C Brendan Higgins 2017-03-28 5:12 ` Brendan Higgins 2017-03-28 5:12 ` [PATCH v6 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller Brendan Higgins 2017-03-28 8:49 ` Benjamin Herrenschmidt 2017-03-29 10:34 ` Brendan Higgins 2017-03-29 12:11 ` Benjamin Herrenschmidt 2017-03-29 20:51 ` Brendan Higgins 2017-03-29 21:17 ` Benjamin Herrenschmidt 2017-04-03 14:16 ` Rob Herring 2017-04-03 14:16 ` Rob Herring 2017-03-28 5:12 ` [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed Brendan Higgins 2017-03-28 5:12 ` Brendan Higgins 2017-03-28 8:32 ` Marc Zyngier 2017-03-28 8:32 ` Marc Zyngier 2017-03-28 9:12 ` Benjamin Herrenschmidt 2017-03-28 9:40 ` Marc Zyngier 2017-03-28 9:40 ` Marc Zyngier 2017-03-28 20:50 ` Benjamin Herrenschmidt 2017-03-28 20:50 ` Benjamin Herrenschmidt 2017-03-29 9:59 ` Brendan Higgins [this message] 2017-03-29 9:59 ` Brendan Higgins 2017-03-29 10:55 ` Marc Zyngier 2017-03-28 8:52 ` Benjamin Herrenschmidt 2017-03-28 8:52 ` Benjamin Herrenschmidt 2017-03-29 10:58 ` Joel Stanley 2017-03-29 20:16 ` Brendan Higgins 2017-03-28 5:12 ` [PATCH v6 3/5] i2c: aspeed: added documentation for Aspeed I2C driver Brendan Higgins 2017-03-28 8:54 ` Benjamin Herrenschmidt 2017-03-28 8:54 ` Benjamin Herrenschmidt 2017-03-29 10:25 ` Brendan Higgins 2017-03-29 10:25 ` Brendan Higgins 2017-04-03 14:22 ` Rob Herring 2017-04-03 14:24 ` Rob Herring 2017-03-28 5:12 ` [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C Brendan Higgins 2017-03-28 5:12 ` Brendan Higgins 2017-03-28 8:57 ` Benjamin Herrenschmidt 2017-03-28 9:09 ` Benjamin Herrenschmidt 2017-03-29 10:23 ` Brendan Higgins 2017-03-31 0:33 ` Joel Stanley 2017-03-31 7:33 ` Benjamin Herrenschmidt 2017-03-31 7:33 ` Benjamin Herrenschmidt 2017-04-24 18:56 ` Brendan Higgins 2017-04-24 18:56 ` Brendan Higgins 2017-04-25 2:19 ` Benjamin Herrenschmidt 2017-04-25 8:32 ` Brendan Higgins 2017-04-25 8:32 ` Brendan Higgins 2017-04-25 8:50 ` Ryan Chen 2017-04-25 9:34 ` Benjamin Herrenschmidt 2017-04-25 9:47 ` Ryan Chen 2017-04-25 9:47 ` Ryan Chen 2017-04-25 19:50 ` Brendan Higgins 2017-04-26 0:52 ` Ryan Chen 2017-04-26 0:52 ` Ryan Chen 2017-03-28 5:12 ` [PATCH v6 5/5] i2c: aspeed: added slave support for Aspeed I2C driver Brendan Higgins 2017-03-31 0:01 ` [PATCH v6 0/5] i2c: aspeed: added driver for Aspeed I2C Andrew Jeffery 2017-03-31 0:01 ` Andrew Jeffery 2017-03-31 0:01 ` Andrew Jeffery
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAFd5g46TAGHWtH7p1o=AJqy8_SygpJZ4RJkJpUwDD3nKxpB5YQ@mail.gmail.com' \ --to=brendanhiggins@google.com \ --cc=benh@kernel.crashing.org \ --cc=clg@kaod.org \ --cc=devicetree@vger.kernel.org \ --cc=jason@lakedaemon.net \ --cc=joel@jms.id.au \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=marc.zyngier@arm.com \ --cc=mark.rutland@arm.com \ --cc=mouse@mayc.ru \ --cc=openbmc@lists.ozlabs.org \ --cc=robh+dt@kernel.org \ --cc=tglx@linutronix.de \ --cc=vz@mleia.com \ --cc=wsa@the-dreams.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.