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

[Krzysztof Kozlowski]

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:
enum max77686_types {
	TYPE_MAX77686_UNKNOWN
	...
}
if (max77686->type == TYPE_MAX77686_UNKNOWN)
	return -EINVAL;

Best regards,
Krzysztof

> -
> -	max77686->type = (unsigned long)match->data;
> -
>  	i2c_set_clientdata(i2c, max77686);
> +	max77686->type = (unsigned long)of_device_get_match_data(&i2c->dev);
>  	max77686->dev = &i2c->dev;
>  	max77686->i2c = i2c;
>  
> -- 
> 2.7.4
>