Re: [RFC, PATCH 1/2] gspca: add input support for interrupt endpoints

[Jean-Francois Moine <moinejf@free.fr>]

Hi,

On Fri, 20 Nov 2009 08:14:10 +0100
Németh Márton <nm127@freemail.hu> wrote:
> Hans de Goede wrote:
> > On 11/19/2009 08:46 AM, Németh Márton wrote:  
> >> Add helper functions for interrupt endpoint based input handling.  
> > First of all many many thanks for doing this!  
> 
> You are welcome :-) . My goal is to just make my webcam working
> properly...

Many thanks from me too. This job will be useful for other webcams.

	[snip]
> > I'm personally not a big fan of adding more configuration options,
> > what should be done instead is make the compilation dependent on the
> > CONFIG_INPUT kernel config option, I see no reason not to enable
> > this when CONFIG_INPUT is enabled.  
> 
> I added dependency on CONFIG_INPUT.

The option USB_GSPCA_SN9C20X_EVDEV should be removed too.

> > Some other remarks, you are using:
> > printk(KERN_DEBUG
> > In various places, please use
> > PDEBUG(D_FOO
> > instead so that the output can be controlled using the gspca
> > module's debug parameter.  
> 
> I created a PDEBUG_INPUT() for this otherwise there is a circular
> dependency between gspca_main and gspca_input because of the variable
> gspca_debug.

That is because you created a separate module.

> > And in gspca_input_connect() you are setting name to "pac7302", this
> > needs to be generalized somehow,  
> 
> I use now gspca_dev->sd_desc->name.

OK for me.

> > and also you are not setting the
> > input device's parent there, I think we need to fix that too
> > (although I'm not sure what it should be set to).  
> 
> I don't know what to use there, maybe somebody on the linux-input
> mailing list could tell.

sn9c20x sets it to &gspca_dev->dev->dev.

> Also, I am not sure about setting of input_dev->id.version.

It seems it can be EV_VERSION only.

> Unfortunately I still get the following error when I start streaming,
> stop streaming or unplug the device:
> 
> [ 6876.780726] uhci_hcd 0000:00:10.1: dma_pool_free buffer-32,
> de0ad168/1e0ad168 (bad dma)

As there is no 'break' in gspca_input_create_urb(), many URBs are
created.

> Please find the new version of this patch later in this mail.

Here are some other remarks:

- As the input functions are called from the gspca main only, and as
  they cannot be used by other drivers, there is no need to have a
  separate module.

- Almost all other webcams who have buttons ask for polling. So, the
  'int_urb' should be pac7302 dependent (in 'struct sd' and not in
  'struct gspca_dev').

Cheers.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html