From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>, linux-kernel@vger.kernel.org,
        CARLOS.PALMINHA@synopsys.com
Subject: Re: [PATCH] reset: Make optional functions really optional.
Date: Fri, 23 Dec 2016 19:57:20 +0200
Message-ID: <49419831.97eIjS4gdS@avalon>
User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; )
In-Reply-To: <36d931b7-3bf4-c3b9-a8e8-24c9b080c39f@synopsys.com>
References: <58b07c24e1bc263db0d43274b46022c8d8506302.1481822669.git.Ramiro.Oliveira@synopsys.com> <1482494934.2394.53.camel@pengutronix.de> <36d931b7-3bf4-c3b9-a8e8-24c9b080c39f@synopsys.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2247
Lines: 67

Hi Ramiro,

On Friday 23 Dec 2016 17:19:43 Ramiro Oliveira wrote:
> On 12/23/2016 12:08 PM, Philipp Zabel wrote:
> > Am Freitag, den 23.12.2016, 13:23 +0200 schrieb Laurent Pinchart:
> >> On Friday 23 Dec 2016 11:58:57 Philipp Zabel wrote:
> >>> Am Donnerstag, den 15.12.2016, 18:05 +0000 schrieb Ramiro Oliveira:
> >>>> Up until now optional functions in the reset API were similar to the
> >>>> non
> >>>> optional.
> >>>> 
> >>>> This patch corrects that, while maintaining compatibility with existing
> >>>> drivers.
> >>>> 
> >>>> As suggested here:
> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_201
> >>>> 6_12_14_502&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=BHEb-RADEOm-lgrwdN4zqtr
> >>>> 2BWZMjeocyTkjphE6PrA&m=_0T0di-X6zgDw8ZRLDNk2ExL2EieBiCmAmuxc8OGAg4&s=H5
> >>>> BfD4P5MB85jtyUjDrn6yKu-6ws5srNWNNiFpPL0pQ&e=
> >>>> 
> >>>> Signed-off-by: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>
> >>>> ---
> >>>> 
> >>>>  drivers/reset/core.c  | 21 +++++++++++++++++++--
> >>>>  include/linux/reset.h | 46 +++++++++++++++++++++++++++++++++++++++----
> >>>>  2 files changed, 61 insertions(+), 6 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> >>>> index 395dc9c..6150e7c 100644
> >>>> --- a/drivers/reset/core.c
> >>>> +++ b/drivers/reset/core.c

[snip]

> >>>>  static inline struct reset_control *reset_control_get_optional_shared(
> >>>>  					struct device *dev, const char
> >>>>  					*id)
> >>>>  {
> >>>> -	return __of_reset_control_get(dev ? dev->of_node : NULL, id,
> >>>> 0, 1);
> >>>> +
> >>>> +	struct reset_control *desc;
> >>>> +
> >>>> +	desc = __of_reset_control_get(dev ? dev->of_node : NULL, id,
> >>>> 0, 1);
> >>>> +
> >>>> +	if (IS_ERR(desc)) {
> >>>> +		if (PTR_ERR(desc) == -ENOENT)
> >>>> +			return NULL;
> >>>> +	}
> >>> 
> >>> With this duplication, I think it might be better to add an int optional
> >>> parameter
> >> 
> >> What's wrong with bool by the way ? :-)
> > 
> > Nothing wrong, it's just that the "exclusive" parameter is already int.
> > I'd be perfectly fine with using bool for both.
> 
> Do you prefer me to keep them both int, or change them to bool?

I'd prefer bool myself, it's slightly more explicit.

-- 
Regards,

Laurent Pinchart