Re: [PATCH 1/2] Input: mcs_touchkey: add device tree support

[Beomho Seo <beomho.seo@samsung.com>]

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