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 <[email protected]> >> Cc: Joonyoung Shim <[email protected]> >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
