From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932967AbdEKNxR (ORCPT ); Thu, 11 May 2017 09:53:17 -0400 Received: from mga05.intel.com ([192.55.52.43]:19163 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932330AbdEKNxO (ORCPT ); Thu, 11 May 2017 09:53:14 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,324,1491289200"; d="scan'208";a="1167824210" Message-ID: <1494510791.6967.9.camel@linux.intel.com> Subject: Re: [PATCH] i2c-designware: add i2c gpio recovery option From: Andy Shevchenko To: Phil Reid , Tim Sander Cc: Jarkko Nikula , Mika Westerberg , Wolfram Sang , linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org Date: Thu, 11 May 2017 16:53:11 +0300 In-Reply-To: <0aafcff1-6970-e99e-5b93-b0877ebf8579@electromag.com.au> References: <2259005.m0altzP21Z@dabox> <1966782.aCEMFj2YWU@virgo> <4a84d6b1-2bba-6f01-8286-49661ef45576@electromag.com.au> <32490844.DoY2McpBxU@dabox> <1494421981.16411.7.camel@linux.intel.com> <0aafcff1-6970-e99e-5b93-b0877ebf8579@electromag.com.au> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2017-05-11 at 09:24 +0800, Phil Reid wrote: > G'day Andy, > > Thanks for the review. You're welcome, just don't forget to remove the parts that are out of scope and/or you agree with. > On 10/05/2017 21:13, Andy Shevchenko wrote: > > On Wed, 2017-05-10 at 13:57 +0200, Tim Sander wrote: > > > +static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev, > > > +                                    struct i2c_adapter *adap) > > > +{ > > > +       struct i2c_bus_recovery_info *rinfo = &dev->rinfo; > > > + > > > +       dev->gpio_scl = devm_gpiod_get_optional(dev->dev, > > > +                                               "scl", > > > +                                               GPIOD_OUT_HIGH); > > > +       if (IS_ERR_OR_NULL(dev->gpio_scl)) > > > > This is wrong. You should not use this macro in most cases. And > > especially it breaks the logic behind _optional(). > > My logic here was that if the gpio is optional return null we return > 0. Why?! _optional() *implies* that all rest calls will go fine and do nothing. > which is an okay status. > But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've > never > quite wrapped my head around why that's the case. > > But the probe function only bails out if this returns EPROBE_DEFER. > Not sure that's the best approach You need something like desc = devm_gpiod_get_optional(...); if (IS_ERR(desc)) return PTR_ERR(desc); > > > +               return PTR_ERR(dev->gpio_sda); > > > +       rinfo->scl_gpio = desc_to_gpio(dev->gpio_scl); > > > +       rinfo->sda_gpio = desc_to_gpio(dev->gpio_sda); > > > > Why?! > >  From my first attempt, didn't remove it from the example I sent. > > We could change i2c_init_recovery to something like the following > then the gpio set / getter could use the default functions. > Not sure the code is completely correct but hopefully you get the > concept. > > static void i2c_init_recovery(struct i2c_adapter *adap) > { > struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; > char *err_str; > > if (!bri) > return; > > if (!bri->recover_bus) { > err_str = "no recover_bus() found"; > goto err; > } > > /* bail out if either no gpio or no set/get callback. */ > if (!gpio_is_valid(bri->scl_gpio) && (!bri->set_scl || !bri- > >get_scl)) { > if (!gpio_is_valid(bri->scl_gpio)) > err_str = "invalid SCL gpio"; > else > err_str = "no {get|set}_scl() found"; > goto err; > } > > if (gpio_is_valid(bri->sda_gpio)) > bri->get_sda = get_sda_gpio_value; > > if (gpio_is_valid(bri->scl_gpio)) { > bri->get_scl = get_scl_gpio_value; > bri->set_scl = set_scl_gpio_value; > } > > return; >   err: > dev_err(&adap->dev, "Not using recovery: %s\n", err_str); > adap->bus_recovery_info = NULL; > } I have briefly looked at the current code. So, my suggestion is to switch to gpio descriptors in current code and then rebase your stuff on top. I wouldn't encourage people to continue using legacy GPIO API. -- Andy Shevchenko Intel Finland Oy