Hi Dmitry,

On 8 September 2011 02:20, Dmitry Torokhov [off-list ref] wrote:

Hi Thomas,

On Tue, Sep 06, 2011 at 07:25:17PM +0530, Thomas Abraham wrote:

quoted

 static int samsung_keypad_is_s5pv210(struct device *dev)
 {
      struct platform_device *pdev = to_platform_device(dev);
-     enum samsung_keypad_type type =
-             platform_get_device_id(pdev)->driver_data;
+     enum samsung_keypad_type type;

+#ifdef CONFIG_OF
+     if (dev->of_node)
+             return of_device_is_compatible(dev->of_node,
+                             "samsung,s5pv210-keypad");
+#endif
+     type = platform_get_device_id(pdev)->driver_data;
      return type == KEYPAD_TYPE_S5PV210;

This function is called every time we scan keypad matrix, I do not think
you want to scan DT bindings here. You need to cache the device type at
probe time and use it.

Ok. As you suggested, this will be changed to cache the type in driver
private data during probe and use the cached copy when required.

quoted

 }

@@ -235,6 +246,130 @@ static void samsung_keypad_close(struct input_dev *input_dev)

      samsung_keypad_stop(keypad);
 }

+#ifdef CONFIG_OF
+static
+struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
+{
+     struct samsung_keypad_platdata *pdata;
+     struct matrix_keymap_data *keymap_data;
+     uint32_t *keymap;
+     struct device_node *np = dev->of_node, *key_np;
+     unsigned int key_count = 0;
+
+     pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+     if (!pdata) {
+             dev_err(dev, "could not allocate memory for platform data\n");
+             return NULL;
+     }

pdata is not used once probe() completes so it would be better to free
it and not rely on devm_* facilities to free it for you once device
unbinds from the driver.

Ok. That would be better. pdata will be freed after probe completes.

quoted

+
+     of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
+     of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);
+     if (!pdata->rows || !pdata->cols) {
+             dev_err(dev, "number of keypad rows/columns not specified\n");
+             return NULL;
+     }
+
+     keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
+     if (!keymap_data) {
+             dev_err(dev, "could not allocate memory for keymap data\n");
+             return NULL;
+     }
+     pdata->keymap_data = keymap_data;
+
+     for_each_child_of_node(np, key_np)
+             key_count++;
+
+     keymap_data->keymap_size = key_count;
+     keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
+     if (!keymap) {
+             dev_err(dev, "could not allocate memory for keymap\n");
+             return NULL;
+     }
+     keymap_data->keymap = keymap;
+
+     for_each_child_of_node(np, key_np) {
+             unsigned int row, col, key_code;
+             of_property_read_u32(key_np, "keypad,row", &row);
+             of_property_read_u32(key_np, "keypad,column", &col);
+             of_property_read_u32(key_np, "keypad,key-code", &key_code);
+             *keymap++ = KEY(row, col, key_code);
+     }

THis seems like generic mechanism that could be used by other drivers...
Maybe move into matrix-keypad.c? You would also not need to allocate
temporary buffer for intermediate keymap data.

Yes, this could be reused in other drivers as well. But, moving this
into matrix-keypad.c file means that KEYBOARD_MATRIX config option
would have to be selected for all platforms reusing this code. So, I
am not sure of the right place for this.

quoted

+
+     if (of_get_property(np, "linux,input-no-autorepeat", NULL))
+             pdata->no_autorepeat = true;
+     if (of_get_property(np, "linux,input-wakeup", NULL))
+             pdata->wakeup = true;
+
+     return pdata;
+}
+
+static void samsung_keypad_parse_dt_gpio(struct device *dev,
+                             struct samsung_keypad *keypad)
+{
+     struct device_node *np = dev->of_node;
+     int gpio, ret, row, col;
+
+     for (row = 0; row < keypad->rows; row++) {
+             gpio = of_get_named_gpio(np, "row-gpios", row);
+             keypad->row_gpios[row] = gpio;
+             if (!gpio_is_valid(gpio)) {
+                     dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
+                                     row, gpio);
+                     continue;
+             }
+
+             ret = gpio_request(gpio, "keypad-row");
+             if (ret)
+                     dev_err(dev, "keypad row[%d] gpio request failed\n",
+                                     row);
+     }
+
+     for (col = 0; col < keypad->cols; col++) {
+             gpio = of_get_named_gpio(np, "col-gpios", col);
+             keypad->col_gpios[col] = gpio;
+             if (!gpio_is_valid(gpio)) {
+                     dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
+                                     col, gpio);
+                     continue;
+             }
+
+             ret = gpio_request(col, "keypad-col");
+             if (ret)
+                     dev_err(dev, "keypad column[%d] gpio request failed\n",
+                                     col);

I think we should bail out if one of the calls fails.

I intended to continue even if some request fails because there could
a partially usable keyboard. If there is a failure, there is a error
message to indicate that keyboard might not be fully functional. If
you are not too strict on this one, I would like to retain it this
way.

quoted

+     }
+}
+
+static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
+{
+     int cnt;
+
+     for (cnt = 0; cnt < keypad->rows; cnt++)
+             if (gpio_is_valid(keypad->row_gpios[cnt]))
+                     gpio_free(keypad->row_gpios[cnt]);
+
+     for (cnt = 0; cnt < keypad->cols; cnt++)
+             if (gpio_is_valid(keypad->col_gpios[cnt]))
+                     gpio_free(keypad->col_gpios[cnt]);
+}
+#else
+static
+struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
+{
+     return NULL;
+}
+
+static void samsung_keypad_parse_dt_gpio(struct device_node *np,
+             struct samsung_keypad *keypad, unsigned int rows,
+             unsigned int cols)
+{
+}
+
+static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
+{
+}
+#endif
+
 static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 {
      const struct samsung_keypad_platdata *pdata;

@@ -246,7 +381,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)

      unsigned int keymap_size;
      int error;

-     pdata = pdev->dev.platform_data;
+     if (pdev->dev.of_node)
+             pdata = samsung_keypad_parse_dt(&pdev->dev);
+     else
+             pdata = pdev->dev.platform_data;
      if (!pdata) {
              dev_err(&pdev->dev, "no platform data defined\n");
              return -EINVAL;

@@ -303,6 +441,9 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)

      keypad->cols = pdata->cols;
      init_waitqueue_head(&keypad->wait);

+     if (pdev->dev.of_node)
+             samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
+
      input_dev->name = pdev->name;
      input_dev->id.bustype = BUS_HOST;
      input_dev->dev.parent = &pdev->dev;

@@ -349,6 +490,7 @@ err_free_irq:

      free_irq(keypad->irq, keypad);
 err_put_clk:
      clk_put(keypad->clk);
+     samsung_keypad_dt_gpio_free(keypad);
 err_unmap_base:
      iounmap(keypad->base);
 err_free_mem:

@@ -374,6 +516,7 @@ static int __devexit samsung_keypad_remove(struct platform_device *pdev)

      free_irq(keypad->irq, keypad);

      clk_put(keypad->clk);
+     samsung_keypad_dt_gpio_free(keypad);

      iounmap(keypad->base);
      kfree(keypad);

@@ -447,6 +590,17 @@ static const struct dev_pm_ops samsung_keypad_pm_ops = {

 };
 #endif

+#ifdef CONFIG_OF
+static const struct of_device_id samsung_keypad_dt_match[] = {
+     { .compatible = "samsung,s3c6410-keypad" },
+     { .compatible = "samsung,s5pv210-keypad" },
+     {},
+};
+MODULE_DEVICE_TABLE(of, samsung_keypad_dt_match);
+#else
+#define samsung_keypad_dt_match NULL
+#endif
+
 static struct platform_device_id samsung_keypad_driver_ids[] = {
      {
              .name           = "samsung-keypad",

@@ -465,6 +619,7 @@ static struct platform_driver samsung_keypad_driver = {

      .driver         = {
              .name   = "samsung-keypad",
              .owner  = THIS_MODULE,
+             .of_match_table = samsung_keypad_dt_match,
 #ifdef CONFIG_PM
              .pm     = &samsung_keypad_pm_ops,
 #endif
--
1.6.6.rc2

Thanks.

--
Dmitry

Thanks for your review and comments.

Regards,
Thomas.
--
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