[PATCH]2.5.30 i2c updates 3/4

Mark D. Studebaker mds at paradyne.com
Fri Aug 9 03:23:24 CEST 2002


Specifically it was sti/cli removal.
I guess some of the issues raised in the mail below were related
and some were not.

Forgive me, I am irq/locking stupid.
Albert Cranford, who is on the sensors mailing list,
is creating and submitting the patches;
his patches are here
http://personal.atl.bellsouth.net/mia/a/c/ac9410/albert/albert.html
and his email is at the bottom of that page.

I think the 8xx driver also needs mods; Albert can fill you in.
Any help you can give Albert directly, and (especially)
pointers to anybody else doing the work or if you can
verify that the 405/8xx drivers we have are the latest would be
most helpful.

thanks
mds



akuster wrote:
> 
> Mark D. Studebaker wrote:
> > Armin, Tom,
> > As you know we checked in the 405 drivers you submitted to us in late
> > March
> > and they are now in our 2.6.4 release.
> >
> > We are attempting to fix up the locking in the 405 drivers for
> > submission
> > to Linus. Can you help?
> >
> > thanks
> > mds
> >
> >
> >
> > Albert Cranford wrote:
> >
> >>Linus, please hold off incorporating all 4 patches until we
> >>can get corrections.  Thanks.
> >>
> >>Rusty,
> >>Couple of quick comments while I forward to I2C authors.
> >>First thanks for your effort to review our code.  It is
> >>very much appreciated.  Next time I'll post to kernel list
> >>before submitting to Linus.
> >>
> >>I meant to remove the compatibility stuff from 2.5.
> >>Something I forgot to check in the new drivers.
> >>
> >>The cli&sti replacement with driver_lock should be at the
> >>module level and not proc level for SMP safety.  I'll
> >>change this in all three drivers.
> >>
> >>The requirement to support I2C and Sensors for releases
> >>2.2 through current is very challenging.  I took your
> >>advice from the last time, but haven't completed the work
> >>yet.
> >>
> >>The important clues you provided will be reviewed and incorporated in the next patch attempt.
> >>Thanks again,
> >>Albert
> >>
> >>Russell King wrote:
> >>
> >>>The following are _some_ comments from a quick read through; they're not
> >>>a thorough review, so there's probably lots of stuff I've missed.  But I
> >>>felt I couldn't carry on reading anything else on lkml... 8)
> >>>
> >>>On Wed, Aug 07, 2002 at 01:29:03AM -0400, Albert Cranford wrote:
> >>>
> >>>>+static void iic_ibmocp_waitforpin(void *data) {
> >>>>+
> >>>>+   int timeout = 2;
> >>>>+   struct iic_ibm *priv_data = data;
> >>>>+   spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;
> >>>>
> >>>What's the point of a spinlock on the stack?  It doesn't gain you any
> >>>protection against other threads on a SMP system.
> >>>
> >>>
> >>>>+   if (priv_data->iic_irq > 0) {
> >>>>+      spin_lock_irq(&driver_lock);
> >>>>+      if (iic_pending == 0) {
> >>>>+        interruptible_sleep_on_timeout(&(iic_wait[priv_data->index]), timeout*HZ );
> >>>>
> >>>You shouldn't be scheduling with interrupts disabled, spinlock locked.
> >>>Using a derivative of sleep_on().  Please look at wait_event() and
> >>>interruptible_wait_event().  Also, interruptible_sleep_on() returns a
> >>>value to tell you if it was interrupted...
> >>>
> >>>
> >>>>+   for(i=0; i<IIC_NUMS; i++) {
> >>>>+      struct iic_ibm *priv_data = (struct iic_ibm *)iic_ibmocp_data[i]->data;
> >>>>+      if (priv_data->iic_irq > 0) {
> >>>>+         disable_irq(priv_data->iic_irq);
> >>>>+         free_irq(priv_data->iic_irq, 0);
> >>>>
> >>>You shouldn't be calling disable_irq() just before free_irq().  Firstly, if
> >>>the interrupt is being shared, then you just killed the other devices using
> >>>that IRQ.  Secondly, free_irq will disable the interrupt source for you, so
> >>>its redundant anyway.
> >>>
> >>>
> >>>>+       if (cpm->reloc == 0) {
> >>>>+               volatile cpm8xx_t *cp = cpm->cp;
> >>>>+
> >>>>+               if (cpm_debug) printk("force_close()\n");
> >>>>+               cp->cp_cpcr =
> >>>>+                       mk_cr_cmd(CPM_CR_CH_I2C, CPM_CR_CLOSE_RXBD) |
> >>>>+                       CPM_CR_FLG;
> >>>>+
> >>>>+               while (cp->cp_cpcr & CPM_CR_FLG);
> >>>>
> >>>Ouch.  There should be a call to cpu_relax() in this (and any other such)
> >>>while loops.  (IIRC Athlons tend to self-destruct without this.)
> >>>
> >>>
> >>>>+       invalidate_dcache_range((unsigned long) buf, (unsigned long) (buf+count));
> >>>>
> >>>This is only defined on ARM and PPC; on ARM its not intended to be called
> >>>by any driver directly (it should be using the pci_* DMA consistency stuff).
> >>>On PPC, it looks like it should be a call to dma_cache_inv() which is the
> >>>more accepted interface.  You can find them in include/asm-ppc/io.h
> >>>
> >>>
> >>>>+
> >>>>+       /* Chip bug, set enable here */
> >>>>+       local_irq_save(flags);
> >>>>+       i2c->i2c_i2cmr = 0x13;  /* Enable some interupts */
> >>>>+       i2c->i2c_i2cer = 0xff;
> >>>>+       i2c->i2c_i2mod = 1;     /* Enable */
> >>>>+       i2c->i2c_i2com = 0x81;  /* Start master */
> >>>>+
> >>>>+       /* Wait for IIC transfer */
> >>>>+       interruptible_sleep_on(&iic_wait);
> >>>>
> >>>Again, sleeping with interrupts disabled...
> >>>
> >>>
> >>>>+       flush_dcache_range((unsigned long) tb, (unsigned long) (tb+1));
> >>>>+       flush_dcache_range((unsigned long) buf, (unsigned long) (buf+count));
> >>>>
> >>>Same comments as invalidate_dcache_range(); dma_cache_wback_inv().
> >>>
> >>>
> >>>>+static void pcf_epp_waitforpin(void) {
> >>>>+  int timeout = 10;
> >>>>+  spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;
> >>>>+
> >>>>+  if (gpe.pe_irq > 0) {
> >>>>+    spin_lock_irq(&driver_lock);
> >>>>+    if (pcf_pending == 0) {
> >>>>+      interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ);
> >>>>+      //udelay(100);
> >>>>+    } else {
> >>>>+      pcf_pending = 0;
> >>>>+    }
> >>>>+    spin_unlock_irq(&driver_lock);
> >>>>
> >>>See above.
> >>>
> >>>
> >>>>+  if (gpe.pe_irq > 0) {
> >>>>
> >>>An IRQ number of zero is completely valid on some systems.
> >>>
> >>>
> >>>>+    if (request_irq(gpe.pe_irq, pcf_epp_handler, 0, "PCF8584", 0) < 0) {
> >>>>+      printk(KERN_NOTICE "i2c-pcf-epp.o: Request irq%d failed\n", gpe.pe_irq);
> >>>>+      gpe.pe_irq = 0;
> >>>>+    } else
> >>>>+      disable_irq(gpe.pe_irq);
> >>>>+      enable_irq(gpe.pe_irq);
> >>>>
> >>>The indentation here makes the code look broken.  However, I suspect the
> >>>bug is balanced out because of the following bit of code:
> >>>
> >>>
> >>>>+static void pcf_epp_exit(void)
> >>>>+{
> >>>>+  if (gpe.pe_irq > 0) {
> >>>>+    disable_irq(gpe.pe_irq);
> >>>>+    free_irq(gpe.pe_irq, 0);
> >>>>
> >>>See comments about disable_irq/free_irq above.
> >>>
> >>>
> >>>>+static int bit_pport_init(void)
> >>>>+{
> >>>>+       //release_region( (base+2) ,1);
> >>>>
> >>>Eww.  Please don't give people ideas about releasing someone elses
> >>>resources.
> >>>
> >>>
> >>>>+       if (check_region((base+2),1) < 0 ) {
> >>>>
> >>>You should request the region first, then probe.  If the probe fails,
> >>>release the region.  Using check_region is not safe on any 2.2 or later
> >>>kernel (hint: you can sleep in request_region).
> >>>
> >>>And finally, I think keeping all the ifdef crap for "I want i2c to build
> >>>across any kernel what so ever" is really bad.  For an example how to
> >>>handle this, please see the jffs2/mtd code which has more or less the
> >>>same requirement, but without the pain of having to put lots of ifdefs
> >>>in the code.  The code is written to support the latest kernel, and the
> >>>compatibility stuff is tucked away inside a header file.
> >>>
> >>>--
> >>>Russell King (rmk at arm.linux.org.uk)                The developer of ARM Linux
> >>>             http://www.arm.linux.org.uk/personal/aboutme.html
> >>>
> >>--
> >>Albert Cranford Deerfield Beach FL USA
> >>ac9410 at bellsouth.net
> >>
> 
> Mark & others,
> 
> By fix up, you mean whats described in this mail thread? or is there
> something else.
> 
> Armin



More information about the lm-sensors mailing list