Re: [PATCH] Input: add MAX7359 key switch controller driver, v2

[Kim Kyuwon]

On Sun, May 10, 2009 at 5:01 AM, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
> P.S. I made a bunch of random changes, mostly to streamline the probe
> code (below), hopfully I did not break anything.
>
> diff --git a/drivers/input/keyboard/max7359_keypad.c b/drivers/input/keyboard/max7359_keypad.c
> index 918386c..280edc3 100644
> --- a/drivers/input/keyboard/max7359_keypad.c
> +++ b/drivers/input/keyboard/max7359_keypad.c
> @@ -204,71 +198,59 @@ static int __devinit max7359_probe(struct i2c_client *client,
>                                        const struct i2c_device_id *id)
>  {
>        struct max7359_keypad *keypad;
> -       struct max7359_keypad_platform_data *pdata;
> +       struct max7359_keypad_platform_data *pdata = client->dev.platform_data;
>        struct input_dev *input_dev;
>        int ret;
> +       int error;
>
> -       keypad = kzalloc(sizeof(struct max7359_keypad), GFP_KERNEL);
> -       if (keypad == NULL) {
> -               dev_err(&client->dev, "failed to allocate driver data\n");
> -               return -ENOMEM;
> +       if (!client->irq) {
> +               dev_err(&client->dev, "The irq number should not be zero\n");
> +               return -EINVAL;
>        }
>
> -       keypad->client = client;
> -       pdata = keypad->pdata = client->dev.platform_data;
> -       i2c_set_clientdata(client, keypad);
> -
>        /* Detect MAX7359: The initial Keys FIFO value is '0x3F' */
>        ret = max7359_read_reg(client, MAX7359_REG_KEYFIFO);
>        if (ret < 0) {
>                dev_err(&client->dev, "failed to detect device\n");
> -               goto failed_free;
> -       } else {
> -               dev_info(&client->dev, "keys FIFO is 0x%02x"
> -                               ", succeeded in detecting device\n", ret);
> +               return -ENODEV;
>        }
>
> -       /* Initialize MAX7359 */
> -       max7359_initialize(client);
> +       dev_dbg(&client->dev, "keys FIFO is 0x%02x\n", ret);
>
> -       /* Create and register the input driver. */
> +       keypad = kzalloc(sizeof(struct max7359_keypad), GFP_KERNEL);
>        input_dev = input_allocate_device();
> -       if (!input_dev) {
> -               dev_err(&client->dev, "failed to allocate input device\n");
> -               ret = -ENOMEM;
> -               goto failed_free;
> +       if (!keypad || !input_dev) {
> +               dev_err(&client->dev, "failed to allocate memory\n");
> +               error = -ENOMEM;
> +               goto failed_free_mem;
>        }

Hi Dmitry,

Thank you for streamlining this driver. I confirmed this streamlined driver
is nicely working on my S3C6410 board and of course I mostly like your
changes. But I'm afraid this 'error' variable is not used at the final
return statement in the probe function.

Thanks & Regards,
Kyuwon

P.S. You may need this kind of patch.
--
diff --git a/drivers/input/keyboard/max7359_keypad.c b/drivers/input/keyboard/max7359_keypad.c
index 280edc3..6e09e0d 100644
--- a/drivers/input/keyboard/max7359_keypad.c
+++ b/drivers/input/keyboard/max7359_keypad.c
@@ -200,7 +200,6 @@ static int __devinit max7359_probe(struct i2c_client *client,
 	struct max7359_keypad *keypad;
 	struct max7359_keypad_platform_data *pdata = client->dev.platform_data;
 	struct input_dev *input_dev;
-	int ret;
 	int error;
 
 	if (!client->irq) {
@@ -209,13 +208,13 @@ static int __devinit max7359_probe(struct i2c_client *client,
 	}
 
 	/* Detect MAX7359: The initial Keys FIFO value is '0x3F' */
-	ret = max7359_read_reg(client, MAX7359_REG_KEYFIFO);
-	if (ret < 0) {
+	error = max7359_read_reg(client, MAX7359_REG_KEYFIFO);
+	if (error < 0) {
 		dev_err(&client->dev, "failed to detect device\n");
 		return -ENODEV;
 	}
 
-	dev_dbg(&client->dev, "keys FIFO is 0x%02x\n", ret);
+	dev_dbg(&client->dev, "keys FIFO is 0x%02x\n", error);
 
 	keypad = kzalloc(sizeof(struct max7359_keypad), GFP_KERNEL);
 	input_dev = input_allocate_device();
@@ -254,8 +253,8 @@ static int __devinit max7359_probe(struct i2c_client *client,
 	}
 
 	/* Register the input device */
-	ret = input_register_device(input_dev);
-	if (ret) {
+	error = input_register_device(input_dev);
+	if (error) {
 		dev_err(&client->dev, "failed to register input device\n");
 		goto failed_free_irq;
 	}
@@ -273,7 +272,7 @@ failed_free_irq:
 failed_free_mem:
 	input_free_device(input_dev);
 	kfree(keypad);
-	return ret;
+	return error;
 }
 
 static int __devexit max7359_remove(struct i2c_client *client)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/