[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-i2c
Subject:    Re: [RFC] i2c-parport: Add SMBus alert support for ADM1032EB
From:       David Brownell <david-b () pacbell ! net>
Date:       2008-11-25 10:45:09
Message-ID: 200811250245.09781.david-b () pacbell ! net
[Download RAW message or body]

On Tuesday 25 November 2008, Jean Delvare wrote:
> > It'd be simpler to use the parport-light code, and delegate all
> > that handling to i2c-core.  After all, you're not likely to
> > have a collection of such i2c adapter dongles sharing a single
> > parallel port ...
> 
> Even the i2c-parport driver only supports one device at the moment,
> because we request the parallel port with flag PARPORT_FLAG_EXCL.
> Honestly I don't really recall why I did that, maybe just paranoia
> while I was developing the driver and then I forgot to change it. OTOH
> I'm not sure if it is really possible to grab the parallel port
> non-exclusively especially now that we can receive an interrupt at
> about any time.
> 
> The daisy chaining of parallel port devices always looked like a hack
> to me anyway.

Yes, a hack.  I'm happy to see it wither away.


> > Alternatively, maybe *both* parport adapter drivers should be
> > updated.
> 
> Yes, that would make sense. However, I always have a hard time testing
> i2c-parport-light, as apparently loading the parport_pc driver is
> enough for the parallel port hardware to lose its pristine state and
> then i2c-parport-light no longer works. And of course parport_pc tends
> to load automatically on my systems.

Oh.  I never used it enough to notice such stuff, I guess.
Somehow I expect parport-light bugs are low priority.  ;)


> > > @@ -189,12 +197,18 @@ static void i2c_parport_attach (struct p
> > >  	if (adapter_parm[type].init.val)
> > >  		line_set(port, 1, &adapter_parm[type].init);
> > >  
> > > -	parport_release(adapter->pdev);
> > > +	/* Setup SMBus alert if supported */
> > > +	if (adapter_parm[type].smbus_alert) {
> > > +		adapter->adapter.do_setup_alert = 1;
> > > +		adapter->adapter.alert_edge_triggered = 1;
> > 
> > The fact that you need to set this trigger flag reflects a bug
> > in the SMBALERT# patch.  Since "you" are handling the IRQ, it
> > has no business caring about that.  It should suffice to set
> > the do_setup_alert flag.
> 
> I'm unsure about this. For edge-triggered interrupts, you indeed
> shouldn't do anything in i2c-core if the adapter driver takes care of
> the interrupt.

If the adapter driver is taking care of the interrupt, then i2c-core
should *never* try touching it.  That part is easy to fix.


> For level-triggered interrupts though, the interrupt 
> needs to be disabled and re-enabled. smbus_alert() appears to be the
> right place to re-enable the interrupt. Else how would the adapter
> driver code know when to re-enable it?

That's another aspect of the bug.  You'll recall I mentioned needing
a hook for that, maybe a callback ... 


> > > --- linux-2.6.28-rc6.orig/drivers/hwmon/lm90.c	2008-10-27 08:46:47.000000000 +0100
> > > +++ linux-2.6.28-rc6/drivers/hwmon/lm90.c	2008-11-21 17:34:36.000000000 +0100
> > 
> > Separate patch?
> 
> I kept it in the same patch to help review and testing. When I submit
> it upstream, I will split appropriately.
> 
> Note that this is one more reason too make sure that the i2c-core code
> is robust against misbehaving devices. At some point in time,
> i2c-parport might support SMBus alert and the lm90 driver might not, or
> vice-versa.

Right; I certainly wasn't arguing aginst robustness.

(And testing against ... quirkier ... hardware than I
used is of course a great way to shake loose issues
in new code.)

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]