LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Dmitry Torokhov" <dmitry.torokhov@gmail.com>
To: bryan.wu@analog.com
Cc: linux-input@atrey.karlin.mff.cuni.cz,
	linux-joystick@atrey.karlin.mff.cuni.cz,
	linux-kernel@vger.kernel.org, akpm@linux-foundation.org
Subject: Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
Date: Fri, 12 Oct 2007 10:49:13 -0400	[thread overview]
Message-ID: <d120d5000710120749o2505dd7aq42559c35298f05d0@mail.gmail.com> (raw)
In-Reply-To: <1192174727.6247.20.camel@roc-laptop>

Hi Bryan,

On 10/12/07, Bryan Wu <bryan.wu@analog.com> 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 = 0x0000;
> +       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 (!ad7142_dev)
> +               return -ENOMEM;
> +
> +       ad7142_dev->open = ad7142_open;
> +       ad7142_dev->close = ad7142_close;
> +       ad7142_dev->evbit[0] = BIT(EV_KEY);
> +       ad7142_dev->keybit[LONG(BTN_BASE)] = BIT(BTN_BASE) | BIT(BTN_BASE2) |
> +                                               BIT(BTN_BASE3) | BIT(BTN_BASE4);
> +       ad7142_dev->keybit[LONG(KEY_UP)] |= BIT(KEY_UP) | BIT(KEY_DOWN) |
> +                                               BIT(KEY_LEFT) | BIT(KEY_RIGHT);
> +
> +       ad7142_dev->name = "ad7142 joystick";
> +       ad7142_dev->phys = "ad7142/input0";
> +       ad7142_dev->id.bustype = BUS_I2C;
> +       ad7142_dev->id.vendor = 0x0001;
> +       ad7142_dev->id.product = 0x0001;
> +       ad7142_dev->id.version = 0x0100;
> +
> +       ret = input_register_device(ad7142_dev);
> +       if (ret) {
> +               printk(KERN_ERR "Failed to register AD7142 input device!\n");
> +               goto fail_register;
> +       }
> +

This all should go into ad7142_probe(). There is no need to create an
input device if we not going to attach to i2c adapter.

> +       ret = i2c_add_driver(&ad7142_driver);
> +       if (ret) {
> +               printk(KERN_ERR "Failed to add AD7142 I2C driver!\n");
> +               goto fail_add;
> +       }
> +       return 0;
> +
> +fail_add:
> +       input_unregister_device(ad7142_dev);

You need to stick "ad7142_dev" here - you are not allowed to call
input_free_device() after calling input_unregister_device().

> +fail_register:
> +       input_free_device(ad7142_dev);
> +       return ret;
> +}
> +
> +static void __exit ad7142_exit(void)
> +{
> +       i2c_del_driver(&ad7142_driver);
> +       input_unregister_device(ad7142_dev);
> +}
> +
> +module_init(ad7142_init);
> +module_exit(ad7142_exit);
> --
> 1.5.3.4
>
>

-- 
Dmitry

  parent reply	other threads:[~2007-10-12 14:49 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-10-12  7:38 Bryan Wu
2007-10-12  7:51 ` Andrey Panin
2007-10-12  9:14 ` Roel Kluin
2007-10-12 14:49 ` Dmitry Torokhov [this message]
2007-10-12 15:39   ` Bryan Wu
2007-10-12 15:50     ` Dmitry Torokhov
2007-10-12 15:59       ` Bryan Wu
2007-10-12 16:14         ` Dmitry Torokhov
2007-10-12 16:41 ` Ahmed S. Darwish
2007-10-12 17:29   ` Dmitry Torokhov
2007-10-12 19:21     ` Ahmed S. Darwish
2007-10-12 20:23       ` Dmitry Torokhov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d120d5000710120749o2505dd7aq42559c35298f05d0@mail.gmail.com \
    --to=dmitry.torokhov@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=bryan.wu@analog.com \
    --cc=linux-input@atrey.karlin.mff.cuni.cz \
    --cc=linux-joystick@atrey.karlin.mff.cuni.cz \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox