From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751638AbdAPXQw (ORCPT ); Mon, 16 Jan 2017 18:16:52 -0500 Received: from mleia.com ([178.79.152.223]:47330 "EHLO mail.mleia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750858AbdAPXQs (ORCPT ); Mon, 16 Jan 2017 18:16:48 -0500 Subject: Re: [PATCH] i2c: core: helper function to detect slave mode To: Luis Oliveira , Andy Shevchenko , Andy Shevchenko References: <73246c4a-504c-52d7-dde4-970a45dca0bd@mleia.com> <3748130b-5321-12eb-ec75-e2637dd9fc54@mleia.com> <1484240482.2133.92.camel@linux.intel.com> <7fdf5d3c-d0ea-ec45-6b18-4573fff6dd11@synopsys.com> Cc: Wolfram Sang , Rob Herring , Mark Rutland , Jarkko Nikula , Mika Westerberg , linux-i2c@vger.kernel.org, devicetree , "linux-kernel@vger.kernel.org" , Ramiro.Oliveira@synopsys.com, Joao Pinto , CARLOS.PALMINHA@synopsys.com From: Vladimir Zapolskiy Message-ID: Date: Tue, 17 Jan 2017 01:14:17 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.4.0 MIME-Version: 1.0 In-Reply-To: <7fdf5d3c-d0ea-ec45-6b18-4573fff6dd11@synopsys.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-49551924 X-CRM114-CacheID: sfid-20170116_231418_975155_10919EA7 X-CRM114-Status: GOOD ( 17.19 ) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Luis, On 01/16/2017 12:32 PM, Luis Oliveira wrote: > On 12-Jan-17 17:01, Andy Shevchenko wrote: >> On Sat, 2017-01-07 at 03:24 +0200, Vladimir Zapolskiy wrote: >>> On 01/07/2017 02:19 AM, Andy Shevchenko wrote: >>>> On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy >>>> wrote: >>>>> On 01/07/2017 12:45 AM, Andy Shevchenko wrote: >> >>>>>> + } >>>>>>>> + } else if (IS_BUILTIN(CONFIG_ACPI) && >>>>>>>> ACPI_HANDLE(dev)) { >>>>>>>> + dev_dbg(dev, "ACPI slave is not supported >>>>>>>> yet\n"); >>>>>>>> + } >>>>>>> >>>>>>> If so, then it might be better to drop else-if stub for now. >>>>>> >>>>>> Please, don't. >>>>>> >>>>> >>>>> Why do you ask for this stub to be added? >>>> >>>> 1. Exactly the reason you asked above. Here is the code which has >>>> built differently on different platforms. x86 usually is not using >>>> CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for >>>> existing examples. >>> >>> From the context by the stub I mean dev_dbg() in >>> i2c_slave_mode_detect() >>> function, I don't see a connection to GPIO library, please clarify. >> >> I agree that is not good proof for using IS_ENABLED/IS_BUILTIN macro. > > I can prepare a V3 and remove it if that's the decision. > from my review comments you may find that your v2 change contains a bug plus it misses a minor but desirable code optimization. You may get more review comments then, for example it is not obvious that on a platform with both CONFIG_ACPI and CONFIG_OF enabled there should be an exclusive selection of only one of two possible branches as in your code etc. The discussed IS_BUILTIN() and dev_dbg() code chunks are other ones, for both of them you may find my review comments and Andy's responses, it's up to you as the author to make any updates or keep the code as is, taking into account any expressed concerns and concerns about concerns the maintainer will make a decision. -- With best wishes, Vladimir From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vladimir Zapolskiy Subject: Re: [PATCH] i2c: core: helper function to detect slave mode Date: Tue, 17 Jan 2017 01:14:17 +0200 Message-ID: References: <73246c4a-504c-52d7-dde4-970a45dca0bd@mleia.com> <3748130b-5321-12eb-ec75-e2637dd9fc54@mleia.com> <1484240482.2133.92.camel@linux.intel.com> <7fdf5d3c-d0ea-ec45-6b18-4573fff6dd11@synopsys.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <7fdf5d3c-d0ea-ec45-6b18-4573fff6dd11-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Luis Oliveira , Andy Shevchenko , Andy Shevchenko Cc: Wolfram Sang , Rob Herring , Mark Rutland , Jarkko Nikula , Mika Westerberg , linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w@public.gmane.org, Joao Pinto , CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w@public.gmane.org List-Id: devicetree@vger.kernel.org Hello Luis, On 01/16/2017 12:32 PM, Luis Oliveira wrote: > On 12-Jan-17 17:01, Andy Shevchenko wrote: >> On Sat, 2017-01-07 at 03:24 +0200, Vladimir Zapolskiy wrote: >>> On 01/07/2017 02:19 AM, Andy Shevchenko wrote: >>>> On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy >>>> wrote: >>>>> On 01/07/2017 12:45 AM, Andy Shevchenko wrote: >> >>>>>> + } >>>>>>>> + } else if (IS_BUILTIN(CONFIG_ACPI) && >>>>>>>> ACPI_HANDLE(dev)) { >>>>>>>> + dev_dbg(dev, "ACPI slave is not supported >>>>>>>> yet\n"); >>>>>>>> + } >>>>>>> >>>>>>> If so, then it might be better to drop else-if stub for now. >>>>>> >>>>>> Please, don't. >>>>>> >>>>> >>>>> Why do you ask for this stub to be added? >>>> >>>> 1. Exactly the reason you asked above. Here is the code which has >>>> built differently on different platforms. x86 usually is not using >>>> CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for >>>> existing examples. >>> >>> From the context by the stub I mean dev_dbg() in >>> i2c_slave_mode_detect() >>> function, I don't see a connection to GPIO library, please clarify. >> >> I agree that is not good proof for using IS_ENABLED/IS_BUILTIN macro. > > I can prepare a V3 and remove it if that's the decision. > from my review comments you may find that your v2 change contains a bug plus it misses a minor but desirable code optimization. You may get more review comments then, for example it is not obvious that on a platform with both CONFIG_ACPI and CONFIG_OF enabled there should be an exclusive selection of only one of two possible branches as in your code etc. The discussed IS_BUILTIN() and dev_dbg() code chunks are other ones, for both of them you may find my review comments and Andy's responses, it's up to you as the author to make any updates or keep the code as is, taking into account any expressed concerns and concerns about concerns the maintainer will make a decision. -- With best wishes, Vladimir -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html