[PATCH] Input: rotary_encoder cleanup

[PATCH] Input: rotary_encoder cleanup

[H Hartley Sweeten]

There is no need to carry the two irq numbers in the allocated memory.
They are only needed for the free_irq() calls during the remove.  Since
the driver is already carrying the platform_data we can just call
gpio_to_irq() to get the irq number.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Daniel Mack <daniel@caiaq.de>

---

diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index c806fbf..910d2db 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -32,9 +32,6 @@ struct rotary_encoder {
 	unsigned int axis;
 	unsigned int pos;
 
-	unsigned int irq_a;
-	unsigned int irq_b;
-
 	bool armed;
 	unsigned char dir;	/* 0 - clockwise, 1 - CCW */
 };
@@ -121,8 +118,6 @@ static int __devinit rotary_encoder_probe(struct platform_device *pdev)
 
 	encoder->input = input;
 	encoder->pdata = pdata;
-	encoder->irq_a = gpio_to_irq(pdata->gpio_a);
-	encoder->irq_b = gpio_to_irq(pdata->gpio_b);
 
 	/* create and register the input driver */
 	input->name = pdev->name;
@@ -160,21 +155,21 @@ static int __devinit rotary_encoder_probe(struct platform_device *pdev)
 	}
 
 	/* request the IRQs */
-	err = request_irq(encoder->irq_a, &rotary_encoder_irq,
+	err = request_irq(gpio_to_irq(pdata->gpio_a), &rotary_encoder_irq,
 			  IORESOURCE_IRQ_HIGHEDGE | IORESOURCE_IRQ_LOWEDGE,
 			  DRV_NAME, encoder);
 	if (err) {
 		dev_err(&pdev->dev, "unable to request IRQ %d\n",
-			encoder->irq_a);
+			gpio_to_irq(pdata->gpio_a));
 		goto exit_free_gpio_b;
 	}
 
-	err = request_irq(encoder->irq_b, &rotary_encoder_irq,
+	err = request_irq(gpio_to_irq(pdata->gpio_b), &rotary_encoder_irq,
 			  IORESOURCE_IRQ_HIGHEDGE | IORESOURCE_IRQ_LOWEDGE,
 			  DRV_NAME, encoder);
 	if (err) {
 		dev_err(&pdev->dev, "unable to request IRQ %d\n",
-			encoder->irq_b);
+			gpio_to_irq(pdata->gpio_b));
 		goto exit_free_irq_a;
 	}
 
@@ -183,7 +178,7 @@ static int __devinit rotary_encoder_probe(struct platform_device *pdev)
 	return 0;
 
 exit_free_irq_a:
-	free_irq(encoder->irq_a, encoder);
+	free_irq(gpio_to_irq(pdata->gpio_a), encoder);
 exit_free_gpio_b:
 	gpio_free(pdata->gpio_b);
 exit_free_gpio_a:
@@ -202,8 +197,8 @@ static int __devexit rotary_encoder_remove(struct platform_device *pdev)
 	struct rotary_encoder *encoder = platform_get_drvdata(pdev);
 	struct rotary_encoder_platform_data *pdata = pdev->dev.platform_data;
 
-	free_irq(encoder->irq_a, encoder);
-	free_irq(encoder->irq_b, encoder);
+	free_irq(gpio_to_irq(pdata->gpio_a), encoder);
+	free_irq(gpio_to_irq(pdata->gpio_b), encoder);
 	gpio_free(pdata->gpio_a);
 	gpio_free(pdata->gpio_b);
 	input_unregister_device(encoder->input); 

Re: [PATCH] Input: rotary_encoder cleanup

[Daniel Mack]

On Sat, Jun 20, 2009 at 06:15:51PM -0400, H Hartley Sweeten wrote:
> There is no need to carry the two irq numbers in the allocated memory.
> They are only needed for the free_irq() calls during the remove.  Since
> the driver is already carrying the platform_data we can just call
> gpio_to_irq() to get the irq number.
> 
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Acked-by: Daniel Mack <daniel@caiaq.de>

Thanks!


> diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
> index c806fbf..910d2db 100644
> --- a/drivers/input/misc/rotary_encoder.c
> +++ b/drivers/input/misc/rotary_encoder.c
> @@ -32,9 +32,6 @@ struct rotary_encoder {
>  	unsigned int axis;
>  	unsigned int pos;
>  
> -	unsigned int irq_a;
> -	unsigned int irq_b;
> -
>  	bool armed;
>  	unsigned char dir;	/* 0 - clockwise, 1 - CCW */
>  };
> @@ -121,8 +118,6 @@ static int __devinit rotary_encoder_probe(struct platform_device *pdev)
>  
>  	encoder->input = input;
>  	encoder->pdata = pdata;
> -	encoder->irq_a = gpio_to_irq(pdata->gpio_a);
> -	encoder->irq_b = gpio_to_irq(pdata->gpio_b);
>  
>  	/* create and register the input driver */
>  	input->name = pdev->name;
> @@ -160,21 +155,21 @@ static int __devinit rotary_encoder_probe(struct platform_device *pdev)
>  	}
>  
>  	/* request the IRQs */
> -	err = request_irq(encoder->irq_a, &rotary_encoder_irq,
> +	err = request_irq(gpio_to_irq(pdata->gpio_a), &rotary_encoder_irq,
>  			  IORESOURCE_IRQ_HIGHEDGE | IORESOURCE_IRQ_LOWEDGE,
>  			  DRV_NAME, encoder);
>  	if (err) {
>  		dev_err(&pdev->dev, "unable to request IRQ %d\n",
> -			encoder->irq_a);
> +			gpio_to_irq(pdata->gpio_a));
>  		goto exit_free_gpio_b;
>  	}
>  
> -	err = request_irq(encoder->irq_b, &rotary_encoder_irq,
> +	err = request_irq(gpio_to_irq(pdata->gpio_b), &rotary_encoder_irq,
>  			  IORESOURCE_IRQ_HIGHEDGE | IORESOURCE_IRQ_LOWEDGE,
>  			  DRV_NAME, encoder);
>  	if (err) {
>  		dev_err(&pdev->dev, "unable to request IRQ %d\n",
> -			encoder->irq_b);
> +			gpio_to_irq(pdata->gpio_b));
>  		goto exit_free_irq_a;
>  	}
>  
> @@ -183,7 +178,7 @@ static int __devinit rotary_encoder_probe(struct platform_device *pdev)
>  	return 0;
>  
>  exit_free_irq_a:
> -	free_irq(encoder->irq_a, encoder);
> +	free_irq(gpio_to_irq(pdata->gpio_a), encoder);
>  exit_free_gpio_b:
>  	gpio_free(pdata->gpio_b);
>  exit_free_gpio_a:
> @@ -202,8 +197,8 @@ static int __devexit rotary_encoder_remove(struct platform_device *pdev)
>  	struct rotary_encoder *encoder = platform_get_drvdata(pdev);
>  	struct rotary_encoder_platform_data *pdata = pdev->dev.platform_data;
>  
> -	free_irq(encoder->irq_a, encoder);
> -	free_irq(encoder->irq_b, encoder);
> +	free_irq(gpio_to_irq(pdata->gpio_a), encoder);
> +	free_irq(gpio_to_irq(pdata->gpio_b), encoder);
>  	gpio_free(pdata->gpio_a);
>  	gpio_free(pdata->gpio_b);
>  	input_unregister_device(encoder->input); 
> --
> 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

Re: [PATCH] Input: rotary_encoder cleanup

[Dmitry Torokhov]

On Tue, Jun 23, 2009 at 06:21:26PM +0200, Daniel Mack wrote:
> On Sat, Jun 20, 2009 at 06:15:51PM -0400, H Hartley Sweeten wrote:
> > There is no need to carry the two irq numbers in the allocated memory.
> > They are only needed for the free_irq() calls during the remove.  Since
> > the driver is already carrying the platform_data we can just call
> > gpio_to_irq() to get the irq number.
> > 
> > Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> 
> Acked-by: Daniel Mack <daniel@caiaq.de>
> 

Do we know how many encoders we need to have in the system to start
seeing the benefits (given that all these conversions increase text
size)?

-- 
Dmitry

RE: [PATCH] Input: rotary_encoder cleanup

[H Hartley Sweeten]

On Monday, June 29, 2009 7:32 PM, Dmitry Torokhov wrote:
> On Tue, Jun 23, 2009 at 06:21:26PM +0200, Daniel Mack wrote:
> > On Sat, Jun 20, 2009 at 06:15:51PM -0400, H Hartley Sweeten wrote:
> > > There is no need to carry the two irq numbers in the allocated memory.
> > > They are only needed for the free_irq() calls during the remove.  Since
> > > the driver is already carrying the platform_data we can just call
> > > gpio_to_irq() to get the irq number.
> > > 
> > > Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> > 
> > Acked-by: Daniel Mack <daniel@caiaq.de>
> > 
> 
> Do we know how many encoders we need to have in the system to start
> seeing the benefits (given that all these conversions increase text
> size)?

Hmm.. Didn't think about that. How do you determine the text size?

I am trying to work out a clean way to pass an array of encoders from
the platform init file so that multiple devices can be handled by the
driver.  That was what prompted the change.

Regards,
Hartley

Re: [PATCH] Input: rotary_encoder cleanup

[Daniel Mack]

On Tue, Jun 30, 2009 at 11:56:36AM -0400, H Hartley Sweeten wrote:
> On Monday, June 29, 2009 7:32 PM, Dmitry Torokhov wrote:
> > Do we know how many encoders we need to have in the system to start
> > seeing the benefits (given that all these conversions increase text
> > size)?
> 
> Hmm.. Didn't think about that. How do you determine the text size?

'objdump -h'?

> I am trying to work out a clean way to pass an array of encoders from
> the platform init file so that multiple devices can be handled by the
> driver.  That was what prompted the change.

Hmm, and then report them all via the very same input device? Or
register one for each encoder? The latter could easily be done by
registering multiple platform_devices with different platform_data,
right?

Daniel

RE: [PATCH] Input: rotary_encoder cleanup

[H Hartley Sweeten]

On Wednesday, July 01, 2009 11:34 AM, Daniel Mack wrote:
> On Tue, Jun 30, 2009 at 11:56:36AM -0400, H Hartley Sweeten wrote:
> > On Monday, June 29, 2009 7:32 PM, Dmitry Torokhov wrote:
> > > Do we know how many encoders we need to have in the system to start
> > > seeing the benefits (given that all these conversions increase text
> > > size)?
> > 
> > Hmm.. Didn't think about that. How do you determine the text size?
> 
> 'objdump -h'?

Thanks.

> > I am trying to work out a clean way to pass an array of encoders from
> > the platform init file so that multiple devices can be handled by the
> > driver.  That was what prompted the change.
> 
> Hmm, and then report them all via the very same input device? Or
> register one for each encoder? The latter could easily be done by
> registering multiple platform_devices with different platform_data,
> right?

I suppose each encoder could be registered individually.  Then each would
report as a unique input device.  This should work with the driver as it
is now.  Then drawback is if there are a number of encoders and a
userspace app is opening all of them and doing a EVIOCGRAB it makes the
app a bit messy.

I was thinking more or passing an array of encoders to the driver and then
having it report all of them as one input device.  That ends up being a
lot cleaner.

Regards,
Hartley

Re: [PATCH] Input: rotary_encoder cleanup

[Daniel Mack]

On Wed, Jul 01, 2009 at 02:45:58PM -0400, H Hartley Sweeten wrote:
> > Hmm, and then report them all via the very same input device? Or
> > register one for each encoder? The latter could easily be done by
> > registering multiple platform_devices with different platform_data,
> > right?
> 
> I suppose each encoder could be registered individually.  Then each would
> report as a unique input device.  This should work with the driver as it
> is now.  Then drawback is if there are a number of encoders and a
> userspace app is opening all of them and doing a EVIOCGRAB it makes the
> app a bit messy.
>
> I was thinking more or passing an array of encoders to the driver and then
> having it report all of them as one input device.  That ends up being a
> lot cleaner.

I'd be fine with such a change. Just curious: how many encoders do you
have on your board?

Daniel

RE: [PATCH] Input: rotary_encoder cleanup

[H Hartley Sweeten]

On Wednesday, July 01, 2009 11:50 AM, Daniel Mack wrote:
> On Wed, Jul 01, 2009 at 02:45:58PM -0400, H Hartley Sweeten wrote:
> > > Hmm, and then report them all via the very same input device? Or
> > > register one for each encoder? The latter could easily be done by
> > > registering multiple platform_devices with different platform_data,
> > > right?
> > 
> > I suppose each encoder could be registered individually.  Then each would
> > report as a unique input device.  This should work with the driver as it
> > is now.  Then drawback is if there are a number of encoders and a
> > userspace app is opening all of them and doing a EVIOCGRAB it makes the
> > app a bit messy.
> > 
> > I was thinking more or passing an array of encoders to the driver and then
> > having it report all of them as one input device.  That ends up being a
> > lot cleaner.
> 
> I'd be fine with such a change. Just curious: how many encoders do you
> have on your board?

I haven't had a chance to figure out a way to pass the array of encoders yet.

But, to answer your question, I have a couple different control panels with
2, 3, or 4 rotary encoders on them.  To go beyond 4 I think I will need to
work out a patch that only requires 1 interrupt line for each encoder.  My
platform has 24 gpio's that can generate interrupts but I am using a number
of them for other purposes.

Regards,
Hartley

Re: [PATCH] Input: rotary_encoder cleanup

[Dmitry Torokhov]

On Wed, Jul 01, 2009 at 02:45:58PM -0400, H Hartley Sweeten wrote:
> On Wednesday, July 01, 2009 11:34 AM, Daniel Mack wrote:
> > On Tue, Jun 30, 2009 at 11:56:36AM -0400, H Hartley Sweeten wrote:
> > > On Monday, June 29, 2009 7:32 PM, Dmitry Torokhov wrote:
> > > > Do we know how many encoders we need to have in the system to start
> > > > seeing the benefits (given that all these conversions increase text
> > > > size)?
> > > 
> > > Hmm.. Didn't think about that. How do you determine the text size?
> > 
> > 'objdump -h'?
> 
> Thanks.
> 
> > > I am trying to work out a clean way to pass an array of encoders from
> > > the platform init file so that multiple devices can be handled by the
> > > driver.  That was what prompted the change.
> > 
> > Hmm, and then report them all via the very same input device? Or
> > register one for each encoder? The latter could easily be done by
> > registering multiple platform_devices with different platform_data,
> > right?
> 
> I suppose each encoder could be registered individually.  Then each would
> report as a unique input device.  This should work with the driver as it
> is now.  Then drawback is if there are a number of encoders and a
> userspace app is opening all of them and doing a EVIOCGRAB it makes the
> app a bit messy.
> 
> I was thinking more or passing an array of encoders to the driver and then
> having it report all of them as one input device.  That ends up being a
> lot cleaner.
>

Hmm, what axis will they be reporting though? Seems like very
specialized [ab]use if they all share the same input device...
 
-- 
Dmitry

RE: [PATCH] Input: rotary_encoder cleanup

[H Hartley Sweeten]

On Tuesday, July 07, 2009 11:57 PM, Dmitry Torokhov wrote:
> On Wed, Jul 01, 2009 at 02:45:58PM -0400, H Hartley Sweeten wrote:
>> On Wednesday, July 01, 2009 11:34 AM, Daniel Mack wrote:
>>> On Tue, Jun 30, 2009 at 11:56:36AM -0400, H Hartley Sweeten wrote:
>>>> On Monday, June 29, 2009 7:32 PM, Dmitry Torokhov wrote:
>>>>> Do we know how many encoders we need to have in the system to start
>>>>> seeing the benefits (given that all these conversions increase text
>>>>> size)?
>>>> 
>>>> Hmm.. Didn't think about that. How do you determine the text size?
>>> 
>>> 'objdump -h'?
>> 
>> Thanks.
>> 
>>>> I am trying to work out a clean way to pass an array of encoders
>>>> from the platform init file so that multiple devices can be handled
>>>> by the driver.  That was what prompted the change.
>>> 
>>> Hmm, and then report them all via the very same input device? Or
>>> register one for each encoder? The latter could easily be done by
>>> registering multiple platform_devices with different platform_data,
>>> right?
>> 
>> I suppose each encoder could be registered individually.  Then each
>> would report as a unique input device.  This should work with the
>> driver as it is now.  Then drawback is if there are a number of
>> encoders and a userspace app is opening all of them and doing a
>> EVIOCGRAB it makes the app a bit messy.
>> 
>> I was thinking more or passing an array of encoders to the driver and
>> then having it report all of them as one input device.  That ends up
>> being a lot cleaner.
>>
>
> Hmm, what axis will they be reporting though? Seems like very
> specialized [ab]use if they all share the same input device...

Each encoder reports as a unique axis.  They just all share one input
device.

If two encoders are reporting on the same axis they would have to be
on different input devices.

I was hoping to pass something like this to the rotary_encoder driver
from my platform init:

static struct rotary_encoder_platform_data my_rotary_encoder_info[] = {
	{
		.gpio_a		= GPIO_ROTARY1_A,
		.gpio_b		= GPIO_ROTARY1_B,
		.axis			= REL_X,
		.relative_axis	= true,
	}, {
		.gpio_a		= GPIO_ROTARY2_A,
		.gpio_b		= GPIO_ROTARY2_B,
		.axis			= REL_Y,
		.relative_axis	= true,	
	}, {
		.gpio_a		= GPIO_ROTARY3_A,
		.gpio_b		= GPIO_ROTARY3_B,
		.axis			= REL_Z,
		.relative_axis	= true,	
	}, {
		.gpio_a		= GPIO_ROTARY4_A,
		.gpio_b		= GPIO_ROTARY4_B,
		.axis			= ABS_THROTTLE,
		.steps		= 1024,
		.rollover		= false,
		.relative_axis	= false,
	},
	( 0 }
};

static struct platform_device rotary_encoder_device = {
	.name		= "rotary-encoder",
	.id		= 0,
	.dev		= {
		.platform_data = my_rotary_encoder_info,
	},
};

The rotary_encoder_probe() function would then just loop thru the
plaform_data and OR in either BIT_MASK(EV_REL) or BIT_MASK(EV_ABS)
as appropriate and either set the input->relbit[0] or call
input_set_abs_params() for the axis.

I just need to figure out a way to pass the correct cookie to the
irq function so that the right axis is processed.

Does this sound resonable?

Regards,
Hartley

Re: [PATCH] Input: rotary_encoder cleanup

[Dmitry Torokhov]

On Wed, Jul 08, 2009 at 05:47:07PM -0400, H Hartley Sweeten wrote:
> On Tuesday, July 07, 2009 11:57 PM, Dmitry Torokhov wrote:
> > On Wed, Jul 01, 2009 at 02:45:58PM -0400, H Hartley Sweeten wrote:
> >> On Wednesday, July 01, 2009 11:34 AM, Daniel Mack wrote:
> >>> On Tue, Jun 30, 2009 at 11:56:36AM -0400, H Hartley Sweeten wrote:
> >>>> On Monday, June 29, 2009 7:32 PM, Dmitry Torokhov wrote:
> >>>>> Do we know how many encoders we need to have in the system to start
> >>>>> seeing the benefits (given that all these conversions increase text
> >>>>> size)?
> >>>> 
> >>>> Hmm.. Didn't think about that. How do you determine the text size?
> >>> 
> >>> 'objdump -h'?
> >> 
> >> Thanks.
> >> 
> >>>> I am trying to work out a clean way to pass an array of encoders
> >>>> from the platform init file so that multiple devices can be handled
> >>>> by the driver.  That was what prompted the change.
> >>> 
> >>> Hmm, and then report them all via the very same input device? Or
> >>> register one for each encoder? The latter could easily be done by
> >>> registering multiple platform_devices with different platform_data,
> >>> right?
> >> 
> >> I suppose each encoder could be registered individually.  Then each
> >> would report as a unique input device.  This should work with the
> >> driver as it is now.  Then drawback is if there are a number of
> >> encoders and a userspace app is opening all of them and doing a
> >> EVIOCGRAB it makes the app a bit messy.
> >> 
> >> I was thinking more or passing an array of encoders to the driver and
> >> then having it report all of them as one input device.  That ends up
> >> being a lot cleaner.
> >>
> >
> > Hmm, what axis will they be reporting though? Seems like very
> > specialized [ab]use if they all share the same input device...
> 
> Each encoder reports as a unique axis.  They just all share one input
> device.
> 

Well, if they truly represent X and Y axes then sharing the same input
device is proper. However if you are using single input just to simplify
userspace application and events reported do not really represent
movement for the axis specified - that would be bad.

-- 
Dmitry

RE: [PATCH] Input: rotary_encoder cleanup

[H Hartley Sweeten]

On Wednesday, July 08, 2009 9:18 PM, Dmitry Torokhov wrote:
>>> Hmm, what axis will they be reporting though? Seems like very
>>> specialized [ab]use if they all share the same input device...
>> 
>> Each encoder reports as a unique axis.  They just all share one
>> input device.
>> 
>
> Well, if they truly represent X and Y axes then sharing the same
> input device is proper. However if you are using single input
> just to simplify userspace application and events reported do
> not really represent movement for the axis specified - that would
> be bad.

I agree.  Trying to report two of the same axis over a single input
would be a major abuse of the api.

Reporting multiple axis' over a single input should be doable.  I
just need to work out how to modify the rotary_encoder driver to
support it...

Thanks,
Hartley