Hi Dmitry,
On 8 September 2011 02:20, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
> Hi Thomas,
>
> On Tue, Sep 06, 2011 at 07:25:17PM +0530, Thomas Abraham wrote:
>> 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.
>
>> }
>>
>> @@ -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.
>
>> +
>> + 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.
>
>> +
>> + 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.
>
>> + }
>> +}
>> +
>> +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-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
