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 <[email protected]> >> --- >> >> 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
