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@xxxxxxxxxxx>
>> Cc: Joonyoung Shim <jy0922.shim@xxxxxxxxxxx>
>> ---
>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
