Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On 10/12/07, Ahmed S. Darwish <[EMAIL PROTECTED]> wrote: > On Fri, Oct 12, 2007 at 01:29:31PM -0400, Dmitry Torokhov wrote: > > > > > Isn't disabling device interrupts from the begining of the ISR > > > "ad7142_interrupt" > > > till the kthread "ad7142_thread" got waked-up and scheduled a long time, > > > espicially if there's a high load on the userspace side ? > > > > > > > It is OK - you disable a specific interrupt line preventing it from > > raising any more IRQs until current one is serviced. > > Won't this affect system responsiveness if the IRQ line was shared ? You are right, this would not work in case of shared IRQs. Hovewer this driver is for an embedded arch and the line is not shared. > > > > > This is different from disabling interrupts on CPU. > > > > mm, Why disabling interrupts in general. Doesn't IRQ hanlers of the same kind > got > executed in a serialized fashion even on SMPs ?. If so, why not just wakeup > our > custom-thread or use workqueues and let them do their business ? Because you don't want CPU to be continually interrupted while untill right thread gets scheduled. I think using work[queue] is the best solution for this driver, but you still want to mask that particular IRQ off until you do the read. -- Dmitry - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On Fri, Oct 12, 2007 at 01:29:31PM -0400, Dmitry Torokhov wrote: > Hi Ahmed, > Hi :), > On 10/12/07, Ahmed S. Darwish <[EMAIL PROTECTED]> wrote: > > On Fri, Oct 12, 2007 at 03:38:47PM +0800, Bryan Wu wrote: > > > > > > Signed-off-by: Bryan Wu <[EMAIL PROTECTED]> > > > --- > > > > Hi Bryan, > > > > Why creating module's own kthread to call ad7142_decode and process keycodes > > instead of using a tasklet ? > > > > Yo can't access i2c from a tasklet context. > > > Isn't disabling device interrupts from the begining of the ISR > > "ad7142_interrupt" > > till the kthread "ad7142_thread" got waked-up and scheduled a long time, > > espicially if there's a high load on the userspace side ? > > > > It is OK - you disable a specific interrupt line preventing it from > raising any more IRQs until current one is serviced. Won't this affect system responsiveness if the IRQ line was shared ? > > This is different from disabling interrupts on CPU. > mm, Why disabling interrupts in general. Doesn't IRQ hanlers of the same kind got executed in a serialized fashion even on SMPs ?. If so, why not just wakeup our custom-thread or use workqueues and let them do their business ? It's the first time for me to read others' patches carefully and kindly ask about some explanations. I hope I'm not bothering people with my misunderstandings! (till I get more experienced). Thanks, -- Ahmed S. Darwish HomePage: http://darwish.07.googlepages.com Blog: http://darwish-07.blogspot.com - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
Hi Ahmed, On 10/12/07, Ahmed S. Darwish <[EMAIL PROTECTED]> wrote: > On Fri, Oct 12, 2007 at 03:38:47PM +0800, Bryan Wu wrote: > > > > Signed-off-by: Bryan Wu <[EMAIL PROTECTED]> > > --- > > Hi Bryan, > > Why creating module's own kthread to call ad7142_decode and process keycodes > instead of using a tasklet ? > Yo can't access i2c from a tasklet context. > Isn't disabling device interrupts from the begining of the ISR > "ad7142_interrupt" > till the kthread "ad7142_thread" got waked-up and scheduled a long time, > espicially if there's a high load on the userspace side ? > It is OK - you disable a specific interrupt line preventing it from raising any more IRQs until current one is serviced. This is different from disabling interrupts on CPU. > Minor issues below. > > > + > > +/* RADC stage 0 - 11 result (uncompensated) actually located in SRAM */ > > +#define ADCRESULT_S0 0x0B > > +#define ADCRESULT_S1 0x0C > > +#define ADCRESULT_S2 0x0D > > +#define ADCRESULT_S3 0x0E > > +#define ADCRESULT_S4 0x0F > > +#define ADCRESULT_S5 0x10 > > +#define ADCRESULT_S6 0x11 > > +#define ADCRESULT_S7 0x12 > > +#define ADCRESULT_S8 0x13 > > +#define ADCRESULT_S9 0x14 > > +#define ADCRESULT_S100x15 > > +#define ADCRESULT_S110x16 > > + > > Keeping last two lines aligned with their above counterparts ? I believe they are aligned if you aplly the patch. -- Dmitry - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On Fri, Oct 12, 2007 at 03:38:47PM +0800, Bryan Wu wrote:
>
> Signed-off-by: Bryan Wu <[EMAIL PROTECTED]>
> ---
Hi Bryan,
Why creating module's own kthread to call ad7142_decode and process keycodes
instead of using a tasklet ?
Isn't disabling device interrupts from the begining of the ISR
"ad7142_interrupt"
till the kthread "ad7142_thread" got waked-up and scheduled a long time,
espicially if there's a high load on the userspace side ?
Minor issues below.
> +
> +/* RADC stage 0 - 11 result (uncompensated) actually located in SRAM */
> +#define ADCRESULT_S0 0x0B
> +#define ADCRESULT_S1 0x0C
> +#define ADCRESULT_S2 0x0D
> +#define ADCRESULT_S3 0x0E
> +#define ADCRESULT_S4 0x0F
> +#define ADCRESULT_S5 0x10
> +#define ADCRESULT_S6 0x11
> +#define ADCRESULT_S7 0x12
> +#define ADCRESULT_S8 0x13
> +#define ADCRESULT_S9 0x14
> +#define ADCRESULT_S100x15
> +#define ADCRESULT_S110x16
> +
Keeping last two lines aligned with their above counterparts ?
> +static int
> +ad7142_probe(struct i2c_adapter *adap, int addr, int kind)
> +{
> + struct i2c_client *client;
> + int rc;
> +
> + client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL);
> + if (!client)
> + return -ENOMEM;
> + memset(client, 0, sizeof(struct i2c_client));
kzalloc.
Regards,
--
Ahmed S. Darwish
HomePage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On 10/12/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
> On Fri, 2007-10-12 at 11:50 -0400, Dmitry Torokhov wrote:
> > On 10/12/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
> > > On Fri, 2007-10-12 at 10:49 -0400, Dmitry Torokhov wrote:
> > > > Hi Bryan,
> > > >
> > > > On 10/12/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
> > > > > +
> > > [snip]
> > > > > +
> > > > > +static void ad7142_close(struct input_dev *dev)
> > > > > +{
> > > > > + free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
> > > > > + kthread_stop(ad7142_task);
> > > >
> > > > Don't you need to write something over i2c to shut the devoce off?
> > > > What stops it from continuing to generate interrupts?
> > > >
> > > Actually, I am going to use "completion" to replace the whole
> > > wait_interrupt_xxx and intr_flag things which original from Aubrey. How
> > > do you think of that?
> > >
> >
> > I don't think it is a very good idea - for me completion is one-time
> > deal. You use it and then you are done. How about firing a work from
> > interrupt and either rely on the default workqueue (keventd) or create
> > your own to execute it?
> >
>
> completion is a wrapper of workqueue and simpler to use.
> my method:
> 1. In kthread:
> do {
> |___|___wait_for_completion(_completion);
> |___|___ad7142_decode();
> |___|___enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
> |___} while (!kthread_should_stop());
>
> 2. In irq handler will fire "complete(_completion);"
>
> This is simpler and understand easier
>
You also need to re-initialize completion every time you done
processing in kthread otherwise you will be constantly doing
ad7142_decode(). Plus, how are you going to stop kthread (completion
may not be signalled for a long time if nobody touches the joystick).
--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On Fri, 2007-10-12 at 11:50 -0400, Dmitry Torokhov wrote:
> On 10/12/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
> > On Fri, 2007-10-12 at 10:49 -0400, Dmitry Torokhov wrote:
> > > Hi Bryan,
> > >
> > > On 10/12/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
> > > > +
> > [snip]
> > > > +
> > > > +static void ad7142_close(struct input_dev *dev)
> > > > +{
> > > > + free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
> > > > + kthread_stop(ad7142_task);
> > >
> > > Don't you need to write something over i2c to shut the devoce off?
> > > What stops it from continuing to generate interrupts?
> > >
> > Actually, I am going to use "completion" to replace the whole
> > wait_interrupt_xxx and intr_flag things which original from Aubrey. How
> > do you think of that?
> >
>
> I don't think it is a very good idea - for me completion is one-time
> deal. You use it and then you are done. How about firing a work from
> interrupt and either rely on the default workqueue (keventd) or create
> your own to execute it?
>
completion is a wrapper of workqueue and simpler to use.
my method:
1. In kthread:
do {
|___|___wait_for_completion(_completion);
|___|___ad7142_decode();
|___|___enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
|___} while (!kthread_should_stop());
2. In irq handler will fire "complete(_completion);"
This is simpler and understand easier
Thanks
-Bryan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On 10/12/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
> On Fri, 2007-10-12 at 10:49 -0400, Dmitry Torokhov wrote:
> > Hi Bryan,
> >
> > On 10/12/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
> > > +
> [snip]
> > > +
> > > +static void ad7142_close(struct input_dev *dev)
> > > +{
> > > + free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
> > > + kthread_stop(ad7142_task);
> >
> > Don't you need to write something over i2c to shut the devoce off?
> > What stops it from continuing to generate interrupts?
> >
> Actually, I am going to use "completion" to replace the whole
> wait_interrupt_xxx and intr_flag things which original from Aubrey. How
> do you think of that?
>
I don't think it is a very good idea - for me completion is one-time
deal. You use it and then you are done. How about firing a work from
interrupt and either rely on the default workqueue (keventd) or create
your own to execute it?
Thank you.
--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On Fri, 2007-10-12 at 10:49 -0400, Dmitry Torokhov wrote:
> Hi Bryan,
>
> On 10/12/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
> > +
[snip]
> > +
> > +static void ad7142_close(struct input_dev *dev)
> > +{
> > + free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
> > + kthread_stop(ad7142_task);
>
> Don't you need to write something over i2c to shut the devoce off?
> What stops it from continuing to generate interrupts?
>
Actually, I am going to use "completion" to replace the whole
wait_interrupt_xxx and intr_flag things which original from Aubrey. How
do you think of that?
Thanks
-Bryan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
Hi Bryan,
On 10/12/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
> +
> +static int
> +ad7142_probe(struct i2c_adapter *adap, int addr, int kind)
> +{
> + struct i2c_client *client;
> + int rc;
> +
> + client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL);
> + if (!client)
> + return -ENOMEM;
> + memset(client, 0, sizeof(struct i2c_client));
kzalloc?
> + strncpy(client->name, AD7142_DRV_NAME, I2C_NAME_SIZE);
strlcpy?
> + client->addr = addr;
> + client->adapter = adap;
> + client->driver = _driver;
> +
> + rc = i2c_attach_client(client);
> + if (rc) {
> + printk(KERN_ERR "i2c_attach_client fail: %d\n", rc);
> + goto fail_attach;
> + }
> +
> + /*
> +* The ADV7142 has an autoincrement function,
> +* use it if the adapter understands raw I2C
> +*/
> + rc = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
> + if (!rc) {
> + printk(KERN_ERR
> + "AD7142: i2c bus doesn't support raw I2C
> operation\n");
> + rc = -EINVAL;
I wonder if -ENOSYS is any better here...
> + goto fail_check;
> + }
> +
> + rc = request_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt,
> + IRQF_TRIGGER_LOW, "ad7142_joy", ad7142_interrupt);
> + if (rc) {
> + printk(KERN_ERR "AD7142: Can't allocate irq %d\n",
> + CONFIG_BFIN_JOYSTICK_IRQ_PFX);
> + rc = -EBUSY;
Just use rc as is.
...
> +
> +static int ad7142_i2c_read(struct i2c_client *client, unsigned short offset,
> + unsigned short *data, unsigned int len)
> +{
> + int ret = -1;
> + int i;
> + u8 block_data[32];
> +
> + if (len < 1 && len > 16) {
What Roel said ;)
> +
> +static int ad7142_thread(void *nothing)
> +{
> + do {
> + wait_event_interruptible(ad7142_wait,
> + kthread_should_stop() || (intr_flag != 0));
should we add "if (intr_flag) { " here? How would the kernel react if
you enable_irq() on irq that is already enabled?
> + ad7142_decode();
> + intr_flag = 0;
> + enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
> + } while (!kthread_should_stop());
> +
> + pr_debug("ad7142: kthread exiting\n");
> +
> + return 0;
> +}
> +
> +static int ad7142_open(struct input_dev *dev)
> +{
> + unsigned short id, value;
> +
> + ad7142_i2c_read(ad7142_client, DEVID, , 1);
> + if (id != AD7142_I2C_ID) {
> + printk(KERN_ERR "Open AD7142 error\n");
> + return -ENODEV;
> + }
> +
> + ad7142_i2c_write(ad7142_client, STAGE0_CONNECTION, stage[0], 8);
> + ad7142_i2c_write(ad7142_client, STAGE1_CONNECTION, stage[1], 8);
> + ad7142_i2c_write(ad7142_client, STAGE2_CONNECTION, stage[2], 8);
> + ad7142_i2c_write(ad7142_client, STAGE3_CONNECTION, stage[3], 8);
> + ad7142_i2c_write(ad7142_client, STAGE4_CONNECTION, stage[4], 8);
> + ad7142_i2c_write(ad7142_client, STAGE5_CONNECTION, stage[4], 8);
> + ad7142_i2c_write(ad7142_client, STAGE6_CONNECTION, stage[4], 8);
> + ad7142_i2c_write(ad7142_client, STAGE7_CONNECTION, stage[4], 8);
> + ad7142_i2c_write(ad7142_client, STAGE8_CONNECTION, stage[4], 8);
> + ad7142_i2c_write(ad7142_client, STAGE9_CONNECTION, stage[4], 8);
> + ad7142_i2c_write(ad7142_client, STAGE10_CONNECTION, stage[4], 8);
> + ad7142_i2c_write(ad7142_client, STAGE11_CONNECTION, stage[4], 8);
> +
> + value = 0x00B0;
> + ad7142_i2c_write(ad7142_client, PWRCONVCTL, , 1);
> +
> + value = 0x0690;
> + ad7142_i2c_write(ad7142_client, AMBCOMPCTL_REG1, , 1);
> +
> + value = 0x0664;
> + ad7142_i2c_write(ad7142_client, AMBCOMPCTL_REG2, , 1);
> +
> + value = 0x290F;
> + ad7142_i2c_write(ad7142_client, AMBCOMPCTL_REG3, , 1);
> +
> + value = 0x000F;
> + ad7142_i2c_write(ad7142_client, INTEN_REG0, , 1);
> + ad7142_i2c_write(ad7142_client, INTEN_REG1, , 1);
> +
> + value = 0x;
> + ad7142_i2c_write(ad7142_client, INTEN_REG2, , 1);
> +
> + ad7142_i2c_read(ad7142_client, AMBCOMPCTL_REG1, , 1);
> +
> + value = 0x000F;
> + ad7142_i2c_write(ad7142_client, AMBCOMPCTL_REG0, , 1);
> +
> + ad7142_task = kthread_run(ad7142_thread, NULL, "ad7142_task");
> + if (IS_ERR(ad7142_task)) {
> + printk(KERN_ERR "serio: Failed to start kseriod\n");
> + return PTR_ERR(ad7142_task);
> + }
> + return 0;
> +}
> +
> +static void ad7142_close(struct input_dev *dev)
> +{
> + free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
> + kthread_stop(ad7142_task);
Don't you need to write something over i2c to shut the devoce off?
What stops it from continuing to generate interrupts?
> +}
> +
> +static int
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
Bryan Wu wrote:
> +static int ad7142_i2c_read(struct i2c_client *client, unsigned short offset,
> + unsigned short *data, unsigned int len)
> +{
> + int ret = -1;
> + int i;
> + u8 block_data[32];
> +
> + if (len < 1 && len > 16) {
you want || here
> + printk(KERN_ERR "AD7142: read data length error\n");
> + return ret;
> + }
> +
> + /* Do raw I2C, not smbus compatible */
> + block_data[0] = (offset & 0xFF00) >> 8;
> + block_data[1] = (offset & 0x00FF);
> +
> + ret = i2c_master_send(client, block_data, 2);
> + if (ret < 0) {
> + printk(KERN_ERR "AD7142: I2C read error\n");
> + return ret;
> + }
> +
> + ret = i2c_master_recv(client, block_data, len * 2);
> + if (ret < 0) {
> + printk(KERN_ERR "AD7142: I2C transfer error\n");
> + return ret;
> + }
> +
> + for (i = 0; i < len; i++) {
> + unsigned short temp;
> + temp = block_data[2 * i];
> + temp = (temp << 8) & 0xFF00;
> + *data++ = temp | block_data[2 * i + 1];
> + }
> +
> + return ret;
> +}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On 285, 10 12, 2007 at 03:38:47PM +0800, Bryan Wu wrote:
> Subject: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick
> driver
>
> [try #2] Changelog:
> - Coding style issues fixed, passed checkpatch.pl
> - Kill uselss "ad7142_used"
> - Move request_irq to probe
> - Move i2c_check_functionality to probe
> - Error handling added
>
> Signed-off-by: Bryan Wu <[EMAIL PROTECTED]>
> ---
> drivers/input/joystick/Kconfig | 16 ++
> drivers/input/joystick/Makefile |1 +
> drivers/input/joystick/ad7142.c | 485
> +++
> 3 files changed, 502 insertions(+), 0 deletions(-)
> create mode 100644 drivers/input/joystick/ad7142.c
>
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index 7c662ee..aeb7cc9 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -282,4 +282,20 @@ config JOYSTICK_XPAD_LEDS
> This option enables support for the LED which surrounds the Big X on
> XBox 360 controller.
>
> +config JOYSTICK_AD7142
> + tristate "Analog Devices AD7142 Joystick support"
> + depends on BFIN && I2C
> + help
> + Say Y here if you want to support an AD7142 joystick
> +
> +
> + config BFIN_JOYSTICK_IRQ_PFX
> + int "GPIO for Interrupt"
> + depends on (BFIN && JOYSTICK_AD7142)
> + range 33 120
> + default "55" if BFIN537_STAMP
> + default "39" if BFIN533_STAMP
> + help
> + Choose an GPIO as Keypad interrupt.[0..15]
> +
> endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index e855abb..8df388f 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -5,6 +5,7 @@
> # Each configuration option enables a list of files.
>
> obj-$(CONFIG_JOYSTICK_A3D) += a3d.o
> +obj-$(CONFIG_JOYSTICK_AD7142)+= ad7142.o
> obj-$(CONFIG_JOYSTICK_ADI) += adi.o
> obj-$(CONFIG_JOYSTICK_AMIGA) += amijoy.o
> obj-$(CONFIG_JOYSTICK_ANALOG)+= analog.o
> diff --git a/drivers/input/joystick/ad7142.c b/drivers/input/joystick/ad7142.c
> new file mode 100644
> index 000..c31e639
> --- /dev/null
> +++ b/drivers/input/joystick/ad7142.c
> @@ -0,0 +1,485 @@
> +/*
> + * File: drivers/input/joystick/ad7142.c
> + * Based on: drivers/input/joystick/amijoy.c
> + * Original Author: Aubrey Li
> + * Maintained by: Bryan Wu <[EMAIL PROTECTED]>
> + *
> + * Created: Apr 7th, 2006
> + * Description:
> + *
> + * Modified:
> + * Copyright 2005-2007 Analog Devices Inc.
> + *
> + * Bugs: Enter bugs at http://blackfin.uclinux.org/
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2, or (at your option)
> + * any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; see the file COPYING.
> + * If not, write to the Free Software Foundation,
> + * 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +MODULE_AUTHOR("Aubrey Li, Bryan Wu <[EMAIL PROTECTED]>");
> +MODULE_DESCRIPTION("Driver for AD7142 joysticks");
> +MODULE_LICENSE("GPL");
> +
> +/*
> + * Feeding the output queue to the device is handled by way of a
> + * workqueue.
> + */
> +static struct task_struct *ad7142_task;
> +static DECLARE_WAIT_QUEUE_HEAD(ad7142_wait);
> +
> +static struct input_dev *ad7142_dev;
> +
> +#define AD7142_DRV_NAME "ad7142_js"
> +#define AD7142_I2C_ID0xE622
> +#define AD7142_I2C_ADDR 0x2C
> +/*
> + * Ram map - these registers are defined as we go along
> + */
> +/* RW Power & conversion control */
> +#define PWRCONVCTL 0x00
> +
> +/* RW Ambient compensation control register 0 - 3 */
> +#define AMBCOMPCTL_REG0
[PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
Subject: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick
driver
[try #2] Changelog:
- Coding style issues fixed, passed checkpatch.pl
- Kill uselss "ad7142_used"
- Move request_irq to probe
- Move i2c_check_functionality to probe
- Error handling added
Signed-off-by: Bryan Wu <[EMAIL PROTECTED]>
---
drivers/input/joystick/Kconfig | 16 ++
drivers/input/joystick/Makefile |1 +
drivers/input/joystick/ad7142.c | 485 +++
3 files changed, 502 insertions(+), 0 deletions(-)
create mode 100644 drivers/input/joystick/ad7142.c
diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index 7c662ee..aeb7cc9 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -282,4 +282,20 @@ config JOYSTICK_XPAD_LEDS
This option enables support for the LED which surrounds the Big X on
XBox 360 controller.
+config JOYSTICK_AD7142
+ tristate "Analog Devices AD7142 Joystick support"
+ depends on BFIN && I2C
+ help
+ Say Y here if you want to support an AD7142 joystick
+
+
+ config BFIN_JOYSTICK_IRQ_PFX
+ int "GPIO for Interrupt"
+ depends on (BFIN && JOYSTICK_AD7142)
+ range 33 120
+ default "55" if BFIN537_STAMP
+ default "39" if BFIN533_STAMP
+ help
+ Choose an GPIO as Keypad interrupt.[0..15]
+
endif
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index e855abb..8df388f 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -5,6 +5,7 @@
# Each configuration option enables a list of files.
obj-$(CONFIG_JOYSTICK_A3D) += a3d.o
+obj-$(CONFIG_JOYSTICK_AD7142) += ad7142.o
obj-$(CONFIG_JOYSTICK_ADI) += adi.o
obj-$(CONFIG_JOYSTICK_AMIGA) += amijoy.o
obj-$(CONFIG_JOYSTICK_ANALOG) += analog.o
diff --git a/drivers/input/joystick/ad7142.c b/drivers/input/joystick/ad7142.c
new file mode 100644
index 000..c31e639
--- /dev/null
+++ b/drivers/input/joystick/ad7142.c
@@ -0,0 +1,485 @@
+/*
+ * File: drivers/input/joystick/ad7142.c
+ * Based on: drivers/input/joystick/amijoy.c
+ * Original Author: Aubrey Li
+ * Maintained by: Bryan Wu <[EMAIL PROTECTED]>
+ *
+ * Created: Apr 7th, 2006
+ * Description:
+ *
+ * Modified:
+ * Copyright 2005-2007 Analog Devices Inc.
+ *
+ * Bugs: Enter bugs at http://blackfin.uclinux.org/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; see the file COPYING.
+ * If not, write to the Free Software Foundation,
+ * 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+MODULE_AUTHOR("Aubrey Li, Bryan Wu <[EMAIL PROTECTED]>");
+MODULE_DESCRIPTION("Driver for AD7142 joysticks");
+MODULE_LICENSE("GPL");
+
+/*
+ * Feeding the output queue to the device is handled by way of a
+ * workqueue.
+ */
+static struct task_struct *ad7142_task;
+static DECLARE_WAIT_QUEUE_HEAD(ad7142_wait);
+
+static struct input_dev *ad7142_dev;
+
+#define AD7142_DRV_NAME"ad7142_js"
+#define AD7142_I2C_ID 0xE622
+#define AD7142_I2C_ADDR0x2C
+/*
+ * Ram map - these registers are defined as we go along
+ */
+/* RW Power & conversion control */
+#define PWRCONVCTL 0x00
+
+/* RW Ambient compensation control register 0 - 3 */
+#define AMBCOMPCTL_REG00x01
+#define AMBCOMPCTL_REG10x02
+#define AMBCOMPCTL_REG20x03
+#define AMBCOMPCTL_REG30x04
+
+/* RW Interrupt enable register 0 - 2 */
+#define INTEN_REG0 0x05
+#define INTEN_REG1 0x06
+#define INTEN_REG2 0x07
+
+/* RLow limit interrupt status register 0 */
+#define INTSTAT_REG0 0x08
+/* RHigh limit interrupt status register 1 */
+#define INTSTAT_REG1 0x09
+/* RInterrupt status register 2 */
+#define INTSTAT_REG2 0x0A
+
+/* RADC stage 0 - 11 result (uncompensated) actually located in SRAM */
+#define ADCRESULT_S0 0x0B
+#define ADCRESULT_S1 0x0C
+#define ADCRESULT_
[PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
Subject: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver [try #2] Changelog: - Coding style issues fixed, passed checkpatch.pl - Kill uselss ad7142_used - Move request_irq to probe - Move i2c_check_functionality to probe - Error handling added Signed-off-by: Bryan Wu [EMAIL PROTECTED] --- drivers/input/joystick/Kconfig | 16 ++ drivers/input/joystick/Makefile |1 + drivers/input/joystick/ad7142.c | 485 +++ 3 files changed, 502 insertions(+), 0 deletions(-) create mode 100644 drivers/input/joystick/ad7142.c diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig index 7c662ee..aeb7cc9 100644 --- a/drivers/input/joystick/Kconfig +++ b/drivers/input/joystick/Kconfig @@ -282,4 +282,20 @@ config JOYSTICK_XPAD_LEDS This option enables support for the LED which surrounds the Big X on XBox 360 controller. +config JOYSTICK_AD7142 + tristate Analog Devices AD7142 Joystick support + depends on BFIN I2C + help + Say Y here if you want to support an AD7142 joystick + + + config BFIN_JOYSTICK_IRQ_PFX + int GPIO for Interrupt + depends on (BFIN JOYSTICK_AD7142) + range 33 120 + default 55 if BFIN537_STAMP + default 39 if BFIN533_STAMP + help + Choose an GPIO as Keypad interrupt.[0..15] + endif diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile index e855abb..8df388f 100644 --- a/drivers/input/joystick/Makefile +++ b/drivers/input/joystick/Makefile @@ -5,6 +5,7 @@ # Each configuration option enables a list of files. obj-$(CONFIG_JOYSTICK_A3D) += a3d.o +obj-$(CONFIG_JOYSTICK_AD7142) += ad7142.o obj-$(CONFIG_JOYSTICK_ADI) += adi.o obj-$(CONFIG_JOYSTICK_AMIGA) += amijoy.o obj-$(CONFIG_JOYSTICK_ANALOG) += analog.o diff --git a/drivers/input/joystick/ad7142.c b/drivers/input/joystick/ad7142.c new file mode 100644 index 000..c31e639 --- /dev/null +++ b/drivers/input/joystick/ad7142.c @@ -0,0 +1,485 @@ +/* + * File: drivers/input/joystick/ad7142.c + * Based on: drivers/input/joystick/amijoy.c + * Original Author: Aubrey Li + * Maintained by: Bryan Wu [EMAIL PROTECTED] + * + * Created: Apr 7th, 2006 + * Description: + * + * Modified: + * Copyright 2005-2007 Analog Devices Inc. + * + * Bugs: Enter bugs at http://blackfin.uclinux.org/ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2, or (at your option) + * any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; see the file COPYING. + * If not, write to the Free Software Foundation, + * 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. + */ + +#include linux/types.h +#include linux/errno.h +#include linux/kernel.h +#include linux/module.h +#include linux/moduleparam.h +#include linux/init.h +#include linux/input.h +#include linux/interrupt.h +#include linux/i2c.h +#include linux/delay.h +#include linux/kthread.h +#include linux/uaccess.h +#include linux/irq.h + +#include asm/blackfin.h + +MODULE_AUTHOR(Aubrey Li, Bryan Wu [EMAIL PROTECTED]); +MODULE_DESCRIPTION(Driver for AD7142 joysticks); +MODULE_LICENSE(GPL); + +/* + * Feeding the output queue to the device is handled by way of a + * workqueue. + */ +static struct task_struct *ad7142_task; +static DECLARE_WAIT_QUEUE_HEAD(ad7142_wait); + +static struct input_dev *ad7142_dev; + +#define AD7142_DRV_NAMEad7142_js +#define AD7142_I2C_ID 0xE622 +#define AD7142_I2C_ADDR0x2C +/* + * Ram map - these registers are defined as we go along + */ +/* RW Power conversion control */ +#define PWRCONVCTL 0x00 + +/* RW Ambient compensation control register 0 - 3 */ +#define AMBCOMPCTL_REG00x01 +#define AMBCOMPCTL_REG10x02 +#define AMBCOMPCTL_REG20x03 +#define AMBCOMPCTL_REG30x04 + +/* RW Interrupt enable register 0 - 2 */ +#define INTEN_REG0 0x05 +#define INTEN_REG1 0x06 +#define INTEN_REG2 0x07 + +/* RLow limit interrupt status register 0 */ +#define INTSTAT_REG0 0x08 +/* RHigh limit interrupt status register 1 */ +#define INTSTAT_REG1 0x09 +/* RInterrupt status register 2 */ +#define INTSTAT_REG2 0x0A + +/* RADC stage 0 - 11 result (uncompensated) actually located in SRAM */ +#define ADCRESULT_S0 0x0B +#define
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On 285, 10 12, 2007 at 03:38:47PM +0800, Bryan Wu wrote: Subject: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver [try #2] Changelog: - Coding style issues fixed, passed checkpatch.pl - Kill uselss ad7142_used - Move request_irq to probe - Move i2c_check_functionality to probe - Error handling added Signed-off-by: Bryan Wu [EMAIL PROTECTED] --- drivers/input/joystick/Kconfig | 16 ++ drivers/input/joystick/Makefile |1 + drivers/input/joystick/ad7142.c | 485 +++ 3 files changed, 502 insertions(+), 0 deletions(-) create mode 100644 drivers/input/joystick/ad7142.c diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig index 7c662ee..aeb7cc9 100644 --- a/drivers/input/joystick/Kconfig +++ b/drivers/input/joystick/Kconfig @@ -282,4 +282,20 @@ config JOYSTICK_XPAD_LEDS This option enables support for the LED which surrounds the Big X on XBox 360 controller. +config JOYSTICK_AD7142 + tristate Analog Devices AD7142 Joystick support + depends on BFIN I2C + help + Say Y here if you want to support an AD7142 joystick + + + config BFIN_JOYSTICK_IRQ_PFX + int GPIO for Interrupt + depends on (BFIN JOYSTICK_AD7142) + range 33 120 + default 55 if BFIN537_STAMP + default 39 if BFIN533_STAMP + help + Choose an GPIO as Keypad interrupt.[0..15] + endif diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile index e855abb..8df388f 100644 --- a/drivers/input/joystick/Makefile +++ b/drivers/input/joystick/Makefile @@ -5,6 +5,7 @@ # Each configuration option enables a list of files. obj-$(CONFIG_JOYSTICK_A3D) += a3d.o +obj-$(CONFIG_JOYSTICK_AD7142)+= ad7142.o obj-$(CONFIG_JOYSTICK_ADI) += adi.o obj-$(CONFIG_JOYSTICK_AMIGA) += amijoy.o obj-$(CONFIG_JOYSTICK_ANALOG)+= analog.o diff --git a/drivers/input/joystick/ad7142.c b/drivers/input/joystick/ad7142.c new file mode 100644 index 000..c31e639 --- /dev/null +++ b/drivers/input/joystick/ad7142.c @@ -0,0 +1,485 @@ +/* + * File: drivers/input/joystick/ad7142.c + * Based on: drivers/input/joystick/amijoy.c + * Original Author: Aubrey Li + * Maintained by: Bryan Wu [EMAIL PROTECTED] + * + * Created: Apr 7th, 2006 + * Description: + * + * Modified: + * Copyright 2005-2007 Analog Devices Inc. + * + * Bugs: Enter bugs at http://blackfin.uclinux.org/ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2, or (at your option) + * any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; see the file COPYING. + * If not, write to the Free Software Foundation, + * 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. + */ + +#include linux/types.h +#include linux/errno.h +#include linux/kernel.h +#include linux/module.h +#include linux/moduleparam.h +#include linux/init.h +#include linux/input.h +#include linux/interrupt.h +#include linux/i2c.h +#include linux/delay.h +#include linux/kthread.h +#include linux/uaccess.h +#include linux/irq.h + +#include asm/blackfin.h + +MODULE_AUTHOR(Aubrey Li, Bryan Wu [EMAIL PROTECTED]); +MODULE_DESCRIPTION(Driver for AD7142 joysticks); +MODULE_LICENSE(GPL); + +/* + * Feeding the output queue to the device is handled by way of a + * workqueue. + */ +static struct task_struct *ad7142_task; +static DECLARE_WAIT_QUEUE_HEAD(ad7142_wait); + +static struct input_dev *ad7142_dev; + +#define AD7142_DRV_NAME ad7142_js +#define AD7142_I2C_ID0xE622 +#define AD7142_I2C_ADDR 0x2C +/* + * Ram map - these registers are defined as we go along + */ +/* RW Power conversion control */ +#define PWRCONVCTL 0x00 + +/* RW Ambient compensation control register 0 - 3 */ +#define AMBCOMPCTL_REG0 0x01 +#define AMBCOMPCTL_REG1 0x02 +#define AMBCOMPCTL_REG2 0x03 +#define AMBCOMPCTL_REG3 0x04 + +/* RW Interrupt enable register 0 - 2 */ +#define INTEN_REG0 0x05 +#define INTEN_REG1 0x06 +#define INTEN_REG2 0x07 + +/* RLow limit interrupt status register 0 */ +#define INTSTAT_REG0 0x08 +/* RHigh limit interrupt status register 1 */ +#define INTSTAT_REG1 0x09 +/* RInterrupt
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
Bryan Wu wrote:
+static int ad7142_i2c_read(struct i2c_client *client, unsigned short offset,
+ unsigned short *data, unsigned int len)
+{
+ int ret = -1;
+ int i;
+ u8 block_data[32];
+
+ if (len 1 len 16) {
you want || here
+ printk(KERN_ERR AD7142: read data length error\n);
+ return ret;
+ }
+
+ /* Do raw I2C, not smbus compatible */
+ block_data[0] = (offset 0xFF00) 8;
+ block_data[1] = (offset 0x00FF);
+
+ ret = i2c_master_send(client, block_data, 2);
+ if (ret 0) {
+ printk(KERN_ERR AD7142: I2C read error\n);
+ return ret;
+ }
+
+ ret = i2c_master_recv(client, block_data, len * 2);
+ if (ret 0) {
+ printk(KERN_ERR AD7142: I2C transfer error\n);
+ return ret;
+ }
+
+ for (i = 0; i len; i++) {
+ unsigned short temp;
+ temp = block_data[2 * i];
+ temp = (temp 8) 0xFF00;
+ *data++ = temp | block_data[2 * i + 1];
+ }
+
+ return ret;
+}
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
On Fri, 2007-10-12 at 10:49 -0400, Dmitry Torokhov wrote:
Hi Bryan,
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
+
[snip]
+
+static void ad7142_close(struct input_dev *dev)
+{
+ free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
+ kthread_stop(ad7142_task);
Don't you need to write something over i2c to shut the devoce off?
What stops it from continuing to generate interrupts?
Actually, I am going to use completion to replace the whole
wait_interrupt_xxx and intr_flag things which original from Aubrey. How
do you think of that?
I don't think it is a very good idea - for me completion is one-time
deal. You use it and then you are done. How about firing a work from
interrupt and either rely on the default workqueue (keventd) or create
your own to execute it?
Thank you.
--
Dmitry
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On Fri, 2007-10-12 at 10:49 -0400, Dmitry Torokhov wrote:
Hi Bryan,
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
+
[snip]
+
+static void ad7142_close(struct input_dev *dev)
+{
+ free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
+ kthread_stop(ad7142_task);
Don't you need to write something over i2c to shut the devoce off?
What stops it from continuing to generate interrupts?
Actually, I am going to use completion to replace the whole
wait_interrupt_xxx and intr_flag things which original from Aubrey. How
do you think of that?
Thanks
-Bryan
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
Hi Bryan,
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
+
+static int
+ad7142_probe(struct i2c_adapter *adap, int addr, int kind)
+{
+ struct i2c_client *client;
+ int rc;
+
+ client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL);
+ if (!client)
+ return -ENOMEM;
+ memset(client, 0, sizeof(struct i2c_client));
kzalloc?
+ strncpy(client-name, AD7142_DRV_NAME, I2C_NAME_SIZE);
strlcpy?
+ client-addr = addr;
+ client-adapter = adap;
+ client-driver = ad7142_driver;
+
+ rc = i2c_attach_client(client);
+ if (rc) {
+ printk(KERN_ERR i2c_attach_client fail: %d\n, rc);
+ goto fail_attach;
+ }
+
+ /*
+* The ADV7142 has an autoincrement function,
+* use it if the adapter understands raw I2C
+*/
+ rc = i2c_check_functionality(client-adapter, I2C_FUNC_I2C);
+ if (!rc) {
+ printk(KERN_ERR
+ AD7142: i2c bus doesn't support raw I2C
operation\n);
+ rc = -EINVAL;
I wonder if -ENOSYS is any better here...
+ goto fail_check;
+ }
+
+ rc = request_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt,
+ IRQF_TRIGGER_LOW, ad7142_joy, ad7142_interrupt);
+ if (rc) {
+ printk(KERN_ERR AD7142: Can't allocate irq %d\n,
+ CONFIG_BFIN_JOYSTICK_IRQ_PFX);
+ rc = -EBUSY;
Just use rc as is.
...
+
+static int ad7142_i2c_read(struct i2c_client *client, unsigned short offset,
+ unsigned short *data, unsigned int len)
+{
+ int ret = -1;
+ int i;
+ u8 block_data[32];
+
+ if (len 1 len 16) {
What Roel said ;)
+
+static int ad7142_thread(void *nothing)
+{
+ do {
+ wait_event_interruptible(ad7142_wait,
+ kthread_should_stop() || (intr_flag != 0));
should we add if (intr_flag) { here? How would the kernel react if
you enable_irq() on irq that is already enabled?
+ ad7142_decode();
+ intr_flag = 0;
+ enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
+ } while (!kthread_should_stop());
+
+ pr_debug(ad7142: kthread exiting\n);
+
+ return 0;
+}
+
+static int ad7142_open(struct input_dev *dev)
+{
+ unsigned short id, value;
+
+ ad7142_i2c_read(ad7142_client, DEVID, id, 1);
+ if (id != AD7142_I2C_ID) {
+ printk(KERN_ERR Open AD7142 error\n);
+ return -ENODEV;
+ }
+
+ ad7142_i2c_write(ad7142_client, STAGE0_CONNECTION, stage[0], 8);
+ ad7142_i2c_write(ad7142_client, STAGE1_CONNECTION, stage[1], 8);
+ ad7142_i2c_write(ad7142_client, STAGE2_CONNECTION, stage[2], 8);
+ ad7142_i2c_write(ad7142_client, STAGE3_CONNECTION, stage[3], 8);
+ ad7142_i2c_write(ad7142_client, STAGE4_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE5_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE6_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE7_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE8_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE9_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE10_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE11_CONNECTION, stage[4], 8);
+
+ value = 0x00B0;
+ ad7142_i2c_write(ad7142_client, PWRCONVCTL, value, 1);
+
+ value = 0x0690;
+ ad7142_i2c_write(ad7142_client, AMBCOMPCTL_REG1, value, 1);
+
+ value = 0x0664;
+ ad7142_i2c_write(ad7142_client, AMBCOMPCTL_REG2, value, 1);
+
+ value = 0x290F;
+ ad7142_i2c_write(ad7142_client, AMBCOMPCTL_REG3, value, 1);
+
+ value = 0x000F;
+ ad7142_i2c_write(ad7142_client, INTEN_REG0, value, 1);
+ ad7142_i2c_write(ad7142_client, INTEN_REG1, value, 1);
+
+ value = 0x;
+ ad7142_i2c_write(ad7142_client, INTEN_REG2, value, 1);
+
+ ad7142_i2c_read(ad7142_client, AMBCOMPCTL_REG1, value, 1);
+
+ value = 0x000F;
+ ad7142_i2c_write(ad7142_client, AMBCOMPCTL_REG0, value, 1);
+
+ ad7142_task = kthread_run(ad7142_thread, NULL, ad7142_task);
+ if (IS_ERR(ad7142_task)) {
+ printk(KERN_ERR serio: Failed to start kseriod\n);
+ return PTR_ERR(ad7142_task);
+ }
+ return 0;
+}
+
+static void ad7142_close(struct input_dev *dev)
+{
+ free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
+ kthread_stop(ad7142_task);
Don't you need to write something over i2c to shut the devoce off?
What stops it from continuing to generate interrupts?
+}
+
+static int __init ad7142_init(void)
+{
+ int ret;
+
+ ad7142_dev = input_allocate_device();
+ if
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
Hi Ahmed, On 10/12/07, Ahmed S. Darwish [EMAIL PROTECTED] wrote: On Fri, Oct 12, 2007 at 03:38:47PM +0800, Bryan Wu wrote: Signed-off-by: Bryan Wu [EMAIL PROTECTED] --- Hi Bryan, Why creating module's own kthread to call ad7142_decode and process keycodes instead of using a tasklet ? Yo can't access i2c from a tasklet context. Isn't disabling device interrupts from the begining of the ISR ad7142_interrupt till the kthread ad7142_thread got waked-up and scheduled a long time, espicially if there's a high load on the userspace side ? It is OK - you disable a specific interrupt line preventing it from raising any more IRQs until current one is serviced. This is different from disabling interrupts on CPU. Minor issues below. + +/* RADC stage 0 - 11 result (uncompensated) actually located in SRAM */ +#define ADCRESULT_S0 0x0B +#define ADCRESULT_S1 0x0C +#define ADCRESULT_S2 0x0D +#define ADCRESULT_S3 0x0E +#define ADCRESULT_S4 0x0F +#define ADCRESULT_S5 0x10 +#define ADCRESULT_S6 0x11 +#define ADCRESULT_S7 0x12 +#define ADCRESULT_S8 0x13 +#define ADCRESULT_S9 0x14 +#define ADCRESULT_S100x15 +#define ADCRESULT_S110x16 + Keeping last two lines aligned with their above counterparts ? I believe they are aligned if you aplly the patch. -- Dmitry - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
On Fri, 2007-10-12 at 11:50 -0400, Dmitry Torokhov wrote:
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
On Fri, 2007-10-12 at 10:49 -0400, Dmitry Torokhov wrote:
Hi Bryan,
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
+
[snip]
+
+static void ad7142_close(struct input_dev *dev)
+{
+ free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
+ kthread_stop(ad7142_task);
Don't you need to write something over i2c to shut the devoce off?
What stops it from continuing to generate interrupts?
Actually, I am going to use completion to replace the whole
wait_interrupt_xxx and intr_flag things which original from Aubrey. How
do you think of that?
I don't think it is a very good idea - for me completion is one-time
deal. You use it and then you are done. How about firing a work from
interrupt and either rely on the default workqueue (keventd) or create
your own to execute it?
completion is a wrapper of workqueue and simpler to use.
my method:
1. In kthread:
do {
|___|___wait_for_completion(ad7142_completion);
|___|___ad7142_decode();
|___|___enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
|___} while (!kthread_should_stop());
2. In irq handler will fire complete(ad7142_completion);
This is simpler and understand easier
You also need to re-initialize completion every time you done
processing in kthread otherwise you will be constantly doing
ad7142_decode(). Plus, how are you going to stop kthread (completion
may not be signalled for a long time if nobody touches the joystick).
--
Dmitry
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On Fri, 2007-10-12 at 11:50 -0400, Dmitry Torokhov wrote:
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
On Fri, 2007-10-12 at 10:49 -0400, Dmitry Torokhov wrote:
Hi Bryan,
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
+
[snip]
+
+static void ad7142_close(struct input_dev *dev)
+{
+ free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
+ kthread_stop(ad7142_task);
Don't you need to write something over i2c to shut the devoce off?
What stops it from continuing to generate interrupts?
Actually, I am going to use completion to replace the whole
wait_interrupt_xxx and intr_flag things which original from Aubrey. How
do you think of that?
I don't think it is a very good idea - for me completion is one-time
deal. You use it and then you are done. How about firing a work from
interrupt and either rely on the default workqueue (keventd) or create
your own to execute it?
completion is a wrapper of workqueue and simpler to use.
my method:
1. In kthread:
do {
|___|___wait_for_completion(ad7142_completion);
|___|___ad7142_decode();
|___|___enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
|___} while (!kthread_should_stop());
2. In irq handler will fire complete(ad7142_completion);
This is simpler and understand easier
Thanks
-Bryan
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On Fri, Oct 12, 2007 at 01:29:31PM -0400, Dmitry Torokhov wrote: Hi Ahmed, Hi :), On 10/12/07, Ahmed S. Darwish [EMAIL PROTECTED] wrote: On Fri, Oct 12, 2007 at 03:38:47PM +0800, Bryan Wu wrote: Signed-off-by: Bryan Wu [EMAIL PROTECTED] --- Hi Bryan, Why creating module's own kthread to call ad7142_decode and process keycodes instead of using a tasklet ? Yo can't access i2c from a tasklet context. Isn't disabling device interrupts from the begining of the ISR ad7142_interrupt till the kthread ad7142_thread got waked-up and scheduled a long time, espicially if there's a high load on the userspace side ? It is OK - you disable a specific interrupt line preventing it from raising any more IRQs until current one is serviced. Won't this affect system responsiveness if the IRQ line was shared ? This is different from disabling interrupts on CPU. mm, Why disabling interrupts in general. Doesn't IRQ hanlers of the same kind got executed in a serialized fashion even on SMPs ?. If so, why not just wakeup our custom-thread or use workqueues and let them do their business ? It's the first time for me to read others' patches carefully and kindly ask about some explanations. I hope I'm not bothering people with my misunderstandings! (till I get more experienced). Thanks, -- Ahmed S. Darwish HomePage: http://darwish.07.googlepages.com Blog: http://darwish-07.blogspot.com - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On Fri, Oct 12, 2007 at 03:38:47PM +0800, Bryan Wu wrote:
Signed-off-by: Bryan Wu [EMAIL PROTECTED]
---
Hi Bryan,
Why creating module's own kthread to call ad7142_decode and process keycodes
instead of using a tasklet ?
Isn't disabling device interrupts from the begining of the ISR
ad7142_interrupt
till the kthread ad7142_thread got waked-up and scheduled a long time,
espicially if there's a high load on the userspace side ?
Minor issues below.
+
+/* RADC stage 0 - 11 result (uncompensated) actually located in SRAM */
+#define ADCRESULT_S0 0x0B
+#define ADCRESULT_S1 0x0C
+#define ADCRESULT_S2 0x0D
+#define ADCRESULT_S3 0x0E
+#define ADCRESULT_S4 0x0F
+#define ADCRESULT_S5 0x10
+#define ADCRESULT_S6 0x11
+#define ADCRESULT_S7 0x12
+#define ADCRESULT_S8 0x13
+#define ADCRESULT_S9 0x14
+#define ADCRESULT_S100x15
+#define ADCRESULT_S110x16
+
Keeping last two lines aligned with their above counterparts ?
+static int
+ad7142_probe(struct i2c_adapter *adap, int addr, int kind)
+{
+ struct i2c_client *client;
+ int rc;
+
+ client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL);
+ if (!client)
+ return -ENOMEM;
+ memset(client, 0, sizeof(struct i2c_client));
kzalloc.
Regards,
--
Ahmed S. Darwish
HomePage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On 10/12/07, Ahmed S. Darwish [EMAIL PROTECTED] wrote: On Fri, Oct 12, 2007 at 01:29:31PM -0400, Dmitry Torokhov wrote: Isn't disabling device interrupts from the begining of the ISR ad7142_interrupt till the kthread ad7142_thread got waked-up and scheduled a long time, espicially if there's a high load on the userspace side ? It is OK - you disable a specific interrupt line preventing it from raising any more IRQs until current one is serviced. Won't this affect system responsiveness if the IRQ line was shared ? You are right, this would not work in case of shared IRQs. Hovewer this driver is for an embedded arch and the line is not shared. This is different from disabling interrupts on CPU. mm, Why disabling interrupts in general. Doesn't IRQ hanlers of the same kind got executed in a serialized fashion even on SMPs ?. If so, why not just wakeup our custom-thread or use workqueues and let them do their business ? Because you don't want CPU to be continually interrupted while untill right thread gets scheduled. I think using work[queue] is the best solution for this driver, but you still want to mask that particular IRQ off until you do the read. -- Dmitry - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
