lkml.org 
[lkml]   [2010]   [Nov]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] input: joystick: Adding Austria Microsystem AS5011 joystick driver
    Hi Anirudh,

    Thanks for your code review.

    >> +static void as5011_update_axes(struct work_struct *work)
    >> +{
    >> + struct as5011_platform_data *plat_dat =
    >> + container_of(work,
    >> + struct as5011_platform_data,
    >> + update_axes_work);
    >> + signed char x, y;
    >> +
    >> + x = as5011_i2c_read(plat_dat->i2c_client, AS5011_X_RES_INT);
    >> + y = as5011_i2c_read(plat_dat->i2c_client, AS5011_Y_RES_INT);
    >
    > What if the read call returns an error ?
    It's a problem yes. But I don't know how to manage this error in this context.

    >> +static int as5011_probe(struct i2c_client *client,
    >> + const struct i2c_device_id *id)
    >> +{
    >
    > __devinit as5011_probe()

    Ok, done.

    >> + if (request_irq(plat_dat->int_irq, as5011_int_interrupt,
    >> + 0, plat_dat->int_irq_name, (void *)plat_dat)) {
    >
    > How about using request_any_context_irq() ?

    Hmm, in fact my platform use a hard IRQ and I used it without question.
    I will see how to improve it.
    >
    >> + dev_err(&client->dev, "as5011: Can't allocate int irq %d\n",
    >> + plat_dat->int_irq);
    >> + retval = -EBUSY;
    >> + goto request_int_irq_error;
    >> + }
    >> +
    >> + retval = input_register_device(plat_dat->input_dev);
    >
    > Its better to register your device at the end, after all the initialization.

    Yes it was a mistake. Done.
    >
    >> + if (retval) {
    >> + dev_err(&client->dev, "as5011: Failed to register device\n");
    >> + retval = -EINVAL;
    >> + goto input_register_device_error;
    >> + }
    >> +
    >> + retval = plat_dat->init_gpio();
    >
    > Please check if the function pointer is NULL.

    Ok, done.

    >> + plat_dat->exit_gpio();
    >
    > Please check if the function pointer is NULL.
    done.

    >> + plat_dat->exit_gpio();
    >
    > Same here.
    done.


    >> + .remove = as5011_remove,
    >
    > __devexit_p (as5011_remove)
    done.

    Thanks.
    Fabien



    \
     
     \ /
      Last update: 2010-11-25 11:35    [W:2.623 / U:0.724 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site