gmane.linux.kernel.input

[PATCH 001/002] linux-input: bcm5974-0.31: fixed resource leak, removed work struct, device data struct introduced

From: Henrik Rydberg <rydberg <at> euromail.se>

BCM5974: This driver adds support for the multitouch trackpad on the new
Apple Macbook Air and Macbook Pro Penryn laptops. It replaces the
appletouch driver on those computers, and integrates well with the
synaptics driver of the Xorg system. 

Signed-off-by: Henrik Rydberg <rydberg <at> euromail.se>
---

The touchpad on the new Macbook Air and Macbook Pro Penryn laptops is
based on the Broadcom BCM5974 chip, of which very little is publicly
known. The device is currently not recognized by the kernel, and as a
consequence the synaptics system fails to operate. The fall-back mouse
handling is very poor. The attached driver, bcm5974, remedies this. It
operates similarly to the appletouch driver.

This is version bcm5974-0.31, incorporating changes suggested by Oliver
Neukum. The driver usability is also discussed at
http://ubuntuforums.org/showthread.php?t=840040.

diff --git a/drivers/input/mouse/Kconfig b/drivers/input/mouse/Kconfig
index 7bbea09..89ef7c3 100644
--- a/drivers/input/mouse/Kconfig
+++ b/drivers/input/mouse/Kconfig
@@ -130,6 +130,29 @@ config MOUSE_APPLETOUCH
 	  To compile this driver as a module, choose M here: the
 	  module will be called appletouch.

+config MOUSE_BCM5974

(Continue reading)

headers

Andrew Morton | 2 Jul 00:59

Re: [PATCH 001/002] linux-input: bcm5974-0.31: fixed resource leak, removed work struct, device data struct introduced


> Subject: [PATCH 001/002] linux-input:  bcm5974-0.31: fixed resource leak, removed work struct, device
data struct introduced

hm, I wonder where [002/002] went.

Please copy the USB list on USB patches.

On Sat, 28 Jun 2008 14:54:20 +0200
Henrik Rydberg <rydberg <at> euromail.se> wrote:

> BCM5974: This driver adds support for the multitouch trackpad on the new
> Apple Macbook Air and Macbook Pro Penryn laptops. It replaces the
> appletouch driver on those computers, and integrates well with the
> synaptics driver of the Xorg system. 

- erk, the driver uses two-spaces for indenting all over the place. 
  Mayeb the email client mangled it in imaginative ways, but it _looks_
  to be intentional.

  If so, please don't do that.  Kernel code uses hard tabs at
  8-cols-per-tab for indenting.

> +config MOUSE_BCM5974
> +	tristate "Apple USB BCM5974 Multitouch trackpad support"
> +	depends on USB_ARCH_HAS_HCD
> +	select USB

selecting USB here seems like a bad idea, although I note that several
other drivers had bad ideas.  `depends on' would be better, IMO.

(Continue reading)

headers

David Brownell | 2 Jul 05:04

Re: [PATCH 001/002] linux-input: bcm5974-0.31: fixed resource leak, removed work struct, device data struct introduced

On Tuesday 01 July 2008, Andrew Morton wrote:
> I dunno what USB driver normally use for their dma memory.  Perhaps
> dma_alloc_coherent()?

kmalloc() as a rule ... dma_alloc_coherent() only makes sense if
the buffer gets reused enough that the dma mapping ops (or as I
think of them, cache maintainence ops) really hurt.

- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Andrew Morton | 2 Jul 05:38

Re: [PATCH 001/002] linux-input: bcm5974-0.31: fixed resource leak, removed work struct, device data struct introduced

On Tue, 1 Jul 2008 20:04:16 -0700 David Brownell <david-b <at> pacbell.net> wrote:

> On Tuesday 01 July 2008, Andrew Morton wrote:
> > I dunno what USB driver normally use for their dma memory. __Perhaps
> > dma_alloc_coherent()?
> 
> kmalloc() as a rule ...

kmalloc(GFP_DMA)?  Hopefully not...

> dma_alloc_coherent() only makes sense if
> the buffer gets reused enough that the dma mapping ops (or as I
> think of them, cache maintainence ops) really hurt.
> 
> - Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

David Brownell | 2 Jul 05:53

Re: [PATCH 001/002] linux-input: bcm5974-0.31: fixed resource leak, removed work struct, device data struct introduced

On Tuesday 01 July 2008, Andrew Morton wrote:
> > > I dunno what USB driver normally use for their dma memory. __Perhaps
> > > dma_alloc_coherent()?
> > 
> > kmalloc() as a rule ...
> 
> kmalloc(GFP_DMA)?  Hopefully not...

Right.  Does anything except parport need that any more?
Precious little ISA stuff does DMA.

USB should never use GFP_DMA.

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

headers

Henrik Rydberg | 2 Jul 09:46

Re: [PATCH 001/002] linux-input: bcm5974-0.31: fixed resource leak, removed work struct, device data struct introduced

On Tue, 2008-07-01 at 15:59 -0700, Andrew Morton wrote:
> > Subject: [PATCH 001/002] linux-input:  bcm5974-0.31: fixed resource leak, removed work struct,
device data struct introduced
> 
> hm, I wonder where [002/002] went.

Thank you very much for your comments, I will return shortly with a new
version. I did not fully grasp, from reading
Documentation/SubmitPatches, how the numbering in the subject should
work; when I submit my new version in reply to your mail, should I
submit a complete patch against the vanilla kernel, with number 001/003?

Best Regards,
Henrik Rydberg

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Andrew Morton | 2 Jul 09:53

Re: [PATCH 001/002] linux-input: bcm5974-0.31: fixed resource leak, removed work struct, device data struct introduced

On Wed, 02 Jul 2008 09:46:39 +0200 Henrik Rydberg <rydberg@...> wrote:

> On Tue, 2008-07-01 at 15:59 -0700, Andrew Morton wrote:
> > > Subject: [PATCH 001/002] linux-input:  bcm5974-0.31: fixed resource leak, removed work struct,
device data struct introduced
> > 
> > hm, I wonder where [002/002] went.
> 
> Thank you very much for your comments, I will return shortly with a new
> version. I did not fully grasp, from reading
> Documentation/SubmitPatches, how the numbering in the subject should
> work; when I submit my new version in reply to your mail, should I
> submit a complete patch against the vanilla kernel, with number 001/003?
> 

no....

The sequence numbering is only used when you're sending two or more
patches at the same time.  It is used so that the recipient can work
out what order they are to be applied in (they can get reordered in
flight) and so that the recipient can verify that none were missed.

So for a single patch, don't include it at all.

Also, it is better to prepare 2.6.27 patches against linux-next, as
that is the candidate 2.6.27 tree.  But in the case of a brand new
driver it doesn't matter a lot.  Unless you're using some interface
which we've gone and changed, but that's fairly uncommon.

--

(Continue reading)

headers

Randy Dunlap | 3 Jul 17:09

Re: [PATCH 001/002] linux-input: bcm5974-0.31: fixed resource leak, removed work struct, device data struct introduced

On Wed, 2 Jul 2008 00:53:55 -0700 Andrew Morton wrote:

> On Wed, 02 Jul 2008 09:46:39 +0200 Henrik Rydberg
<rydberg@...> wrote:
> 
> > On Tue, 2008-07-01 at 15:59 -0700, Andrew Morton wrote:
> > > > Subject: [PATCH 001/002] linux-input:  bcm5974-0.31: fixed resource leak, removed work struct,
device data struct introduced
> > > 
> > > hm, I wonder where [002/002] went.
> > 
> > Thank you very much for your comments, I will return shortly with a new
> > version. I did not fully grasp, from reading
> > Documentation/SubmitPatches, how the numbering in the subject should
> > work; when I submit my new version in reply to your mail, should I
> > submit a complete patch against the vanilla kernel, with number 001/003?
> > 
> 
> no....
> 
> The sequence numbering is only used when you're sending two or more

related, dependent,

> patches at the same time.  It is used so that the recipient can work
> out what order they are to be applied in (they can get reordered in
> flight) and so that the recipient can verify that none were missed.
> 
> So for a single patch, don't include it at all.

(Continue reading)

headers

Henrik Rydberg | 3 Jul 18:33

Re: [PATCH 001/002] linux-input: bcm5974-0.31: fixed resource leak, removed work struct, device data struct introduced

On Tue, 2008-07-01 at 15:59 -0700, Andrew Morton wrote:
> > +  }
> > +
> > +  button = data[1];
> > +
> > +  /* only report button state changes */
> > +  if (button != dev->bt_state) {
> > +    input_report_key(dev->input, BTN_LEFT, button);
> > +    input_sync(dev->input);
> > +  }
> > +
> > +  dev->bt_state = button;
> > +
> > + exit:
> > +  retval = usb_submit_urb(dev->bt_urb, GFP_ATOMIC);
> 
> GFP_ATOMIC is a red flag.  Is this quite unrelaible allocation mode
> really needed here?

Being new to kernel work, I rely a lot on how other drivers work.
However, doing some reading, these are my observations:

* The URB works in interrupt mode.

* The call to usb_submit_urb above is within a completion handler.

* From what I read on kerneltrap (2.6.22), such URBs should be
resubmitted using the ATOMIC method. Maybe this changed, I could not
tell.

(Continue reading)