From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751650AbdAMNNo (ORCPT ); Fri, 13 Jan 2017 08:13:44 -0500 Received: from ec2-52-27-115-49.us-west-2.compute.amazonaws.com ([52.27.115.49]:50432 "EHLO osg.samsung.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751490AbdAMNNn (ORCPT ); Fri, 13 Jan 2017 08:13:43 -0500 Subject: Re: [PATCH 2/4] mfd: max77686: Use of_device_get_match_data() helper To: Krzysztof Kozlowski References: <1484228857-20182-1-git-send-email-javier@osg.samsung.com> <1484228857-20182-3-git-send-email-javier@osg.samsung.com> <20170113130421.xm76aik6fq6cc4vb@kozik-lap> Cc: linux-kernel@vger.kernel.org, Laxman Dewangan , Chanwoo Choi , Bartlomiej Zolnierkiewicz , Lee Jones From: Javier Martinez Canillas Message-ID: <60ac85aa-e2d3-14ba-4029-10aea0d6fd2f@osg.samsung.com> Date: Fri, 13 Jan 2017 10:12:46 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20170113130421.xm76aik6fq6cc4vb@kozik-lap> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> >> 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 >> #include >> #include >> +#include >> >> 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