* [PATCH 1/2] Input: mcs_touchkey: add device tree support
@ 2014-05-21 5:51 Beomho Seo
2014-05-21 13:48 ` Mark Rutland
0 siblings, 1 reply; 3+ messages in thread
From: Beomho Seo @ 2014-05-21 5:51 UTC (permalink / raw)
To: linux-input, devicetree, dmitry.torokhov, Joonyoung Shim,
mark.rutland
Cc: Myungjoo Ham, Jaehoon Chung, Chanwoo Choi
Add device tree support for mcs touchkey driver.
Tested on exynos 4412 board.
Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
Cc: Joonyoung Shim <jy0922.shim@samsung.com>
---
drivers/input/keyboard/mcs_touchkey.c | 73 +++++++++++++++++++++++++++++++--
1 file changed, 69 insertions(+), 4 deletions(-)
diff --git a/drivers/input/keyboard/mcs_touchkey.c b/drivers/input/keyboard/mcs_touchkey.c
index 1da8e0b..9bff47b 100644
--- a/drivers/input/keyboard/mcs_touchkey.c
+++ b/drivers/input/keyboard/mcs_touchkey.c
@@ -19,6 +19,7 @@
#include <linux/irq.h>
#include <linux/slab.h>
#include <linux/pm.h>
+#include <linux/of_gpio.h>
/* MCS5000 Touchkey */
#define MCS5000_TOUCHKEY_STATUS 0x04
@@ -96,6 +97,60 @@ static irqreturn_t mcs_touchkey_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}
+#ifdef CONFIG_OF
+static struct mcs_platform_data *mcs_touchkey_parse_dt(struct device *dev)
+{
+ struct mcs_platform_data *pdata;
+ struct device_node *np = dev->of_node;
+ unsigned int keymap[2];
+ unsigned int len;
+ int i = 0;
+ const __be32 *prop;
+
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_err(dev, "Failed to allocate platform data\n");
+ return ERR_PTR(-ENOMEM);
+ }
+
+ prop = of_get_property(np, "linux,code", &len);
+ if (!prop) {
+ dev_err(dev, "Failed to get code\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ if (len % sizeof(u32)) {
+ dev_err(dev, "Malformed keycode property\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ pdata->keymap_size = len / sizeof(u32);
+
+ if (of_property_read_u32(np, "key_maxval", &pdata->key_maxval)) {
+ dev_err(dev, "Failed to get key max value data\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ if (pdata->keymap_size > pdata->key_maxval) {
+ dev_err(dev, "Key map size overflow\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ for (i = 0; i < pdata->keymap_size; i++) {
+ u32 code = be32_to_cpup(prop + i);
+ keymap[i] = MCS_KEY_MAP(i, code);
+ }
+ pdata->keymap = keymap;
+ return pdata;
+}
+#else
+static inline struct mcs_platform_data *mcs_touchkey_parse_dt
+ (struct device *dev)
+{
+ return NULL;
+}
+#endif
+
static int mcs_touchkey_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -107,10 +162,14 @@ static int mcs_touchkey_probe(struct i2c_client *client,
int error;
int i;
- pdata = dev_get_platdata(&client->dev);
- if (!pdata) {
- dev_err(&client->dev, "no platform data defined\n");
- return -EINVAL;
+ if (&client->dev.of_node)
+ pdata = mcs_touchkey_parse_dt(&client->dev);
+ else
+ pdata = dev_get_platdata(&client->dev);
+
+ if (IS_ERR(pdata)) {
+ dev_err(&client->dev, "Failed to get platform data\n");
+ return PTR_ERR(pdata);
}
data = kzalloc(sizeof(struct mcs_touchkey_data) +
@@ -262,11 +321,17 @@ static const struct i2c_device_id mcs_touchkey_id[] = {
};
MODULE_DEVICE_TABLE(i2c, mcs_touchkey_id);
+static struct of_device_id mcs_touchkey_dt_match[] = {
+ { .compatible = "mcs5000_touchkey", },
+ { .compatible = "mcs5080_touchkey", },
+};
+
static struct i2c_driver mcs_touchkey_driver = {
.driver = {
.name = "mcs_touchkey",
.owner = THIS_MODULE,
.pm = &mcs_touchkey_pm_ops,
+ .of_match_table = of_match_ptr(mcs_touchkey_dt_match),
},
.probe = mcs_touchkey_probe,
.remove = mcs_touchkey_remove,
--
1.7.9.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] Input: mcs_touchkey: add device tree support
2014-05-21 5:51 [PATCH 1/2] Input: mcs_touchkey: add device tree support Beomho Seo
@ 2014-05-21 13:48 ` Mark Rutland
2014-05-22 2:27 ` Beomho Seo
0 siblings, 1 reply; 3+ messages in thread
From: Mark Rutland @ 2014-05-21 13:48 UTC (permalink / raw)
To: Beomho Seo
Cc: linux-input@vger.kernel.org, devicetree@vger.kernel.org,
dmitry.torokhov@gmail.com, Joonyoung Shim, Myungjoo Ham,
Jaehoon Chung, Chanwoo Choi
On Wed, May 21, 2014 at 06:51:49AM +0100, Beomho Seo wrote:
> Add device tree support for mcs touchkey driver.
> Tested on exynos 4412 board.
>
> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> ---
> drivers/input/keyboard/mcs_touchkey.c | 73 +++++++++++++++++++++++++++++++--
> 1 file changed, 69 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/keyboard/mcs_touchkey.c b/drivers/input/keyboard/mcs_touchkey.c
> index 1da8e0b..9bff47b 100644
> --- a/drivers/input/keyboard/mcs_touchkey.c
> +++ b/drivers/input/keyboard/mcs_touchkey.c
> @@ -19,6 +19,7 @@
> #include <linux/irq.h>
> #include <linux/slab.h>
> #include <linux/pm.h>
> +#include <linux/of_gpio.h>
>
> /* MCS5000 Touchkey */
> #define MCS5000_TOUCHKEY_STATUS 0x04
> @@ -96,6 +97,60 @@ static irqreturn_t mcs_touchkey_interrupt(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +#ifdef CONFIG_OF
> +static struct mcs_platform_data *mcs_touchkey_parse_dt(struct device *dev)
> +{
> + struct mcs_platform_data *pdata;
> + struct device_node *np = dev->of_node;
> + unsigned int keymap[2];
> + unsigned int len;
> + int i = 0;
> + const __be32 *prop;
Hmm. Almost every use of __be32 *prop values is indicative of something
that can be done with existing accessors. I suspect that may be true
here...
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata) {
> + dev_err(dev, "Failed to allocate platform data\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + prop = of_get_property(np, "linux,code", &len);
> + if (!prop) {
> + dev_err(dev, "Failed to get code\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (len % sizeof(u32)) {
> + dev_err(dev, "Malformed keycode property\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + pdata->keymap_size = len / sizeof(u32);
Use of_property_count_u32_elems. It does all of this and returns a
negative error code if anything is wrong.
> +
> + if (of_property_read_u32(np, "key_maxval", &pdata->key_maxval)) {
> + dev_err(dev, "Failed to get key max value data\n");
> + return ERR_PTR(-EINVAL);
> + }
What is this? This sounds like an implementation detail. Why is it in
the DT?
Why doesn't it follow dt conventions ('-' rather than '_')?
> +
> + if (pdata->keymap_size > pdata->key_maxval) {
> + dev_err(dev, "Key map size overflow\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + for (i = 0; i < pdata->keymap_size; i++) {
> + u32 code = be32_to_cpup(prop + i);
Use the DT accessors (e.g. of_property_read_u32_index). There is
absolutely no reason to touch the raw DTB data here.
> + keymap[i] = MCS_KEY_MAP(i, code);
> + }
> + pdata->keymap = keymap;
> + return pdata;
> +}
> +#else
> +static inline struct mcs_platform_data *mcs_touchkey_parse_dt
> + (struct device *dev)
> +{
> + return NULL;
> +}
> +#endif
> +
> static int mcs_touchkey_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -107,10 +162,14 @@ static int mcs_touchkey_probe(struct i2c_client *client,
> int error;
> int i;
>
> - pdata = dev_get_platdata(&client->dev);
> - if (!pdata) {
> - dev_err(&client->dev, "no platform data defined\n");
> - return -EINVAL;
> + if (&client->dev.of_node)
> + pdata = mcs_touchkey_parse_dt(&client->dev);
> + else
> + pdata = dev_get_platdata(&client->dev);
> +
> + if (IS_ERR(pdata)) {
> + dev_err(&client->dev, "Failed to get platform data\n");
> + return PTR_ERR(pdata);
> }
>
> data = kzalloc(sizeof(struct mcs_touchkey_data) +
> @@ -262,11 +321,17 @@ static const struct i2c_device_id mcs_touchkey_id[] = {
> };
> MODULE_DEVICE_TABLE(i2c, mcs_touchkey_id);
>
> +static struct of_device_id mcs_touchkey_dt_match[] = {
> + { .compatible = "mcs5000_touchkey", },
> + { .compatible = "mcs5080_touchkey", },
NAK. These do not follow the "$VENDOR,$DEVICE" pattern.
Mark.
> +};
> +
> static struct i2c_driver mcs_touchkey_driver = {
> .driver = {
> .name = "mcs_touchkey",
> .owner = THIS_MODULE,
> .pm = &mcs_touchkey_pm_ops,
> + .of_match_table = of_match_ptr(mcs_touchkey_dt_match),
> },
> .probe = mcs_touchkey_probe,
> .remove = mcs_touchkey_remove,
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] Input: mcs_touchkey: add device tree support
2014-05-21 13:48 ` Mark Rutland
@ 2014-05-22 2:27 ` Beomho Seo
0 siblings, 0 replies; 3+ messages in thread
From: Beomho Seo @ 2014-05-22 2:27 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-input@vger.kernel.org, devicetree@vger.kernel.org,
dmitry.torokhov@gmail.com, Joonyoung Shim, Myungjoo Ham,
Jaehoon Chung, Chanwoo Choi
Hello,
Thank you for your advice.
On 05/21/2014 10:48 PM, Mark Rutland wrote:
> On Wed, May 21, 2014 at 06:51:49AM +0100, Beomho Seo wrote:
>> Add device tree support for mcs touchkey driver.
>> Tested on exynos 4412 board.
>>
>> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
>> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
>> ---
>> drivers/input/keyboard/mcs_touchkey.c | 73 +++++++++++++++++++++++++++++++--
>> 1 file changed, 69 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/mcs_touchkey.c b/drivers/input/keyboard/mcs_touchkey.c
>> index 1da8e0b..9bff47b 100644
>> --- a/drivers/input/keyboard/mcs_touchkey.c
>> +++ b/drivers/input/keyboard/mcs_touchkey.c
>> @@ -19,6 +19,7 @@
>> #include <linux/irq.h>
>> #include <linux/slab.h>
>> #include <linux/pm.h>
>> +#include <linux/of_gpio.h>
>>
>> /* MCS5000 Touchkey */
>> #define MCS5000_TOUCHKEY_STATUS 0x04
>> @@ -96,6 +97,60 @@ static irqreturn_t mcs_touchkey_interrupt(int irq, void *dev_id)
>> return IRQ_HANDLED;
>> }
>>
>> +#ifdef CONFIG_OF
>> +static struct mcs_platform_data *mcs_touchkey_parse_dt(struct device *dev)
>> +{
>> + struct mcs_platform_data *pdata;
>> + struct device_node *np = dev->of_node;
>> + unsigned int keymap[2];
>> + unsigned int len;
>> + int i = 0;
>> + const __be32 *prop;
>
> Hmm. Almost every use of __be32 *prop values is indicative of something
> that can be done with existing accessors. I suspect that may be true
> here...
>
>> +
>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata) {
>> + dev_err(dev, "Failed to allocate platform data\n");
>> + return ERR_PTR(-ENOMEM);
>> + }
>> +
>> + prop = of_get_property(np, "linux,code", &len);
>> + if (!prop) {
>> + dev_err(dev, "Failed to get code\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + if (len % sizeof(u32)) {
>> + dev_err(dev, "Malformed keycode property\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + pdata->keymap_size = len / sizeof(u32);
>
> Use of_property_count_u32_elems. It does all of this and returns a
> negative error code if anything is wrong.
>
>> +
>> + if (of_property_read_u32(np, "key_maxval", &pdata->key_maxval)) {
>> + dev_err(dev, "Failed to get key max value data\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>
> What is this? This sounds like an implementation detail. Why is it in
> the DT?
>
> Why doesn't it follow dt conventions ('-' rather than '_')?
>
OK. I will revise *_parse_dt function on your advice.
>> +
>> + if (pdata->keymap_size > pdata->key_maxval) {
>> + dev_err(dev, "Key map size overflow\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + for (i = 0; i < pdata->keymap_size; i++) {
>> + u32 code = be32_to_cpup(prop + i);
>
> Use the DT accessors (e.g. of_property_read_u32_index). There is
> absolutely no reason to touch the raw DTB data here.
>
>> + keymap[i] = MCS_KEY_MAP(i, code);
>> + }
>> + pdata->keymap = keymap;
>> + return pdata;
>> +}
>> +#else
>> +static inline struct mcs_platform_data *mcs_touchkey_parse_dt
>> + (struct device *dev)
>> +{
>> + return NULL;
>> +}
>> +#endif
>> +
>> static int mcs_touchkey_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> @@ -107,10 +162,14 @@ static int mcs_touchkey_probe(struct i2c_client *client,
>> int error;
>> int i;
>>
>> - pdata = dev_get_platdata(&client->dev);
>> - if (!pdata) {
>> - dev_err(&client->dev, "no platform data defined\n");
>> - return -EINVAL;
>> + if (&client->dev.of_node)
>> + pdata = mcs_touchkey_parse_dt(&client->dev);
>> + else
>> + pdata = dev_get_platdata(&client->dev);
>> +
>> + if (IS_ERR(pdata)) {
>> + dev_err(&client->dev, "Failed to get platform data\n");
>> + return PTR_ERR(pdata);
>> }
>>
>> data = kzalloc(sizeof(struct mcs_touchkey_data) +
>> @@ -262,11 +321,17 @@ static const struct i2c_device_id mcs_touchkey_id[] = {
>> };
>> MODULE_DEVICE_TABLE(i2c, mcs_touchkey_id);
>>
>> +static struct of_device_id mcs_touchkey_dt_match[] = {
>> + { .compatible = "mcs5000_touchkey", },
>> + { .compatible = "mcs5080_touchkey", },
>
> NAK. These do not follow the "$VENDOR,$DEVICE" pattern.
>
> Mark.
>
OK, I will changes to above pattern.
Additionally, Fix bindings documentation and vendor-prefixes.txt.
>> +};
>> +
>> static struct i2c_driver mcs_touchkey_driver = {
>> .driver = {
>> .name = "mcs_touchkey",
>> .owner = THIS_MODULE,
>> .pm = &mcs_touchkey_pm_ops,
>> + .of_match_table = of_match_ptr(mcs_touchkey_dt_match),
>> },
>> .probe = mcs_touchkey_probe,
>> .remove = mcs_touchkey_remove,
>> --
>> 1.7.9.5
>>
>
Again, thank you for your advice.
I'll fix them and send v2 patch soon.
Best regards,
Beomho Seo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-05-22 2:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-21 5:51 [PATCH 1/2] Input: mcs_touchkey: add device tree support Beomho Seo
2014-05-21 13:48 ` Mark Rutland
2014-05-22 2:27 ` Beomho Seo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).