Re: [PATCH 2/4] mfd: max77686: Use of_device_get_match_data() helper

[Javier Martinez Canillas]

Hello Krzysztof,

Thanks for the feedback.

On 01/13/2017 10:04 AM, Krzysztof Kozlowski wrote:
> On Thu, Jan 12, 2017 at 10:47:35AM -0300, Javier Martinez Canillas wrote:
>> Use the generic helper to get the matched of_device_id .data,
>> instead of open coding it.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>> ---
>>
>>  drivers/mfd/max77686.c | 9 ++-------
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
>> index ddae3bf3e46c..33dd09493605 100644
>> --- a/drivers/mfd/max77686.c
>> +++ b/drivers/mfd/max77686.c
>> @@ -34,6 +34,7 @@
>>  #include <linux/mfd/max77686-private.h>
>>  #include <linux/err.h>
>>  #include <linux/of.h>
>> +#include <linux/of_device.h>
>>  
>>  static const struct mfd_cell max77686_devs[] = {
>>  	{ .name = "max77686-pmic", },
>> @@ -175,7 +176,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>>  			      const struct i2c_device_id *id)
>>  {
>>  	struct max77686_dev *max77686 = NULL;
>> -	const struct of_device_id *match;
>>  	unsigned int data;
>>  	int ret = 0;
>>  	const struct regmap_config *config;
>> @@ -188,13 +188,8 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>>  	if (!max77686)
>>  		return -ENOMEM;
>>  
>> -	match = of_match_node(max77686_pmic_dt_match, i2c->dev.of_node);
>> -	if (!match)
>> -		return -EINVAL;
> 
> The commit message would suggest that the code is equivalent (except usage
> of helper) but it is not the same entirely. You are not checking for
> matched data. Returned NULL will be cast back to type to valid
> TYPE_MAX77686. This should not happen but either mention the removal of
> check in commit msg or make it:

Yes, the check didn't make too much sense since as you said this can't happen.
IOW, the probe being called means that OF registered a platform device with a
valid compatible from the OF match table so the match will always succeed.

But you are right that I should had mentioned in the commit, will do in v2.

> enum max77686_types {
> 	TYPE_MAX77686_UNKNOWN
> 	...
> }
> if (max77686->type == TYPE_MAX77686_UNKNOWN)
> 	return -EINVAL;
>

I prefer the former, this will just add code that will never be used.
 
> Best regards,
> Krzysztof
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America