Re: [PATCH v6 10/19] i2c: octeon: Add support for cn78xx chips

[David Daney]

On 04/20/2016 02:52 PM, Wolfram Sang wrote:
> On Mon, Apr 11, 2016 at 05:28:41PM +0200, Jan Glauber wrote:
> > From: David Daney <david.daney@xxxxxxxxxx>
> >
> > cn78xx has a different interrupt architecture, so we have to manage
> > the interrupts differently.
>
> I'd appreciate if you could explain here why interrupts use NOAUTOEN and
> have to be manually en-/disabled? This is surely unusual.

It is quite nice, isn't it?

Since there were enable bits in both the interrupt controller *and* the 
I2C bus hardware, we had redundancy that could profitably be eliminated.

Now with the advent of the cn78xx, the system is much better.  This 
redundancy has been eliminated and interrupts are enabled only in the 
interrupt controller.

Fortunately, the kernel supplies us with convenience functions for 
manipulating these interrupt enable bits (enable_irq() and 
disable_irq()).  Since we don't want to irqs enabled when the driver is 
probed we have to set NOAUTOEN before calling request_irq().

Personally, I preferred the old way where each device had interrupt 
enable bits that could be controlled independently of the interrupt 
controller.  I have been told that I should abandon my preference for 
that type of antiquated architecture and welcome the brave new world of 
the unified interrupt enable bit.  Never the less, I still feel some 
nostalgia for the old way.

The good news is that it is not very much work to add software support 
for the cn78xx style interrupt enabling/masking.  The hooks we add can 
also be used to support the MSI-X interrupts in the Thunder support 
patch that follows.

David.


> > Signed-off-by: David Daney <ddaney@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Jan Glauber <jglauber@xxxxxxxxxx>
> > ---
> >   drivers/i2c/busses/i2c-octeon.c | 131 ++++++++++++++++++++++++++++++++++++----
> >   1 file changed, 120 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
> > index 0bb9f0b..fff1ac0 100644
> > --- a/drivers/i2c/busses/i2c-octeon.c
> > +++ b/drivers/i2c/busses/i2c-octeon.c
> > @@ -11,6 +11,7 @@
> >    * warranty of any kind, whether express or implied.
> >    */
> >
> > +#include <linux/atomic.h>
> >   #include <linux/platform_device.h>
> >   #include <linux/interrupt.h>
> >   #include <linux/kernel.h>
> > @@ -112,11 +113,18 @@ struct octeon_i2c {
> >   	wait_queue_head_t queue;
> >   	struct i2c_adapter adap;
> >   	int irq;
> > +	int hlc_irq;		/* For cn7890 only */
> >   	u32 twsi_freq;
> >   	int sys_freq;
> >   	void __iomem *twsi_base;
> >   	struct device *dev;
> >   	bool hlc_enabled;
> > +	void (*int_en)(struct octeon_i2c *);
> > +	void (*int_dis)(struct octeon_i2c *);
> > +	void (*hlc_int_en)(struct octeon_i2c *);
> > +	void (*hlc_int_dis)(struct octeon_i2c *);
>
> I'd prefer having 'enable' and 'disable' written out, but I leave that
> to you.
>
> > +	atomic_t int_en_cnt;
> > +	atomic_t hlc_int_en_cnt;
> >   };