From mboxrd@z Thu Jan 1 00:00:00 1970 From: Janusz Krzysztofik Subject: Re: [PATCH 5/6] mtd: rawnand: ams-delta: use GPIO lookup table Date: Sat, 19 May 2018 01:15:30 +0200 Message-ID: <3427199.r4OBoDP6Xz@z50> References: <20180518210954.29044-1-jmkrzyszt@gmail.com> <20180518210954.29044-5-jmkrzyszt@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-lf0-f66.google.com (mail-lf0-f66.google.com [209.85.215.66]) by alsa0.perex.cz (Postfix) with ESMTP id 351472672AF for ; Sat, 19 May 2018 01:15:23 +0200 (CEST) Received: by mail-lf0-f66.google.com with SMTP id w202-v6so16164677lff.12 for ; Fri, 18 May 2018 16:15:23 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Andy Shevchenko Cc: linux-fbdev@vger.kernel.org, ALSA Development Mailing List , Aaro Koskinen , Tony Lindgren , Richard Weinberger , Mark Brown , Dmitry Torokhov , Liam Girdwood , Linux Kernel Mailing List , Peter Ujfalusi , Boris Brezillon , Tomi Valkeinen , "open list:MEMORY TECHNOLOGY..." , linux-arm Mailing List , linux-input , Linux OMAP Mailing List , Jarkko Nikula List-Id: alsa-devel@alsa-project.org On Friday, May 18, 2018 11:21:14 PM CEST Andy Shevchenko wrote: > On Sat, May 19, 2018 at 12:09 AM, Janusz Krzysztofik > > wrote: > > + gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > > + if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > So, is it optional or not at the end? > If it is, why do we check for NULL? As far as I can understand, nand_chip->dev_ready() callback is optional. That's why I decided to use the _optional variant of devm_gpiod_get(). In case of ams-delta, the dev_ready() callback depends on availability of the 'rdy' GPIO pin. As a consequence, I'm checking for both NULL and ERR in order to decide if dev_ready() will be supported. I can pretty well replace it with the standard form and check for ERR only if the purpose of the _optional form is different. > > this->dev_ready = ams_delta_nand_ready; > > > > } else { > > > > this->dev_ready = NULL; > > pr_notice("Couldn't request gpio for Delta NAND > > ready.\n"); > > dev_notice() ? Sure, but maybe in a separate patch? That's not a new code just being added but an existing one, not the merit of the change. > > } > > > > +err_gpiod: > > + if (err == -ENODEV || err == -ENOENT) > > + err = -EPROBE_DEFER; > > Hmm... Amstrad Delta uses gpio-mmio driver. Unfortunatelty that driver is not availble before device init phase, unlike other crucial GPIO drivers which are initialized earlier, e.g. during the postcore or at latetst the subsys phase. Hence, devices which depend on GPIO pins provided by gpio-mmio must either be declared late or fail softly so they get another chance of being probed succesfully. I thought of replacing the gpio-mmio platform driver with bgpio functions it exports but for now I haven't implemented it, not even shared the idea. Does it really hurt to return -EPROBE_DEFER if a GPIO pin can't be obtained? Thanks, Janusz From mboxrd@z Thu Jan 1 00:00:00 1970 From: Janusz Krzysztofik Date: Fri, 18 May 2018 23:15:30 +0000 Subject: Re: [PATCH 5/6] mtd: rawnand: ams-delta: use GPIO lookup table Message-Id: <3427199.r4OBoDP6Xz@z50> List-Id: References: <20180518210954.29044-1-jmkrzyszt@gmail.com> <20180518210954.29044-5-jmkrzyszt@gmail.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Andy Shevchenko Cc: linux-fbdev@vger.kernel.org, ALSA Development Mailing List , Aaro Koskinen , Tony Lindgren , Richard Weinberger , Mark Brown , Dmitry Torokhov , Liam Girdwood , Linux Kernel Mailing List , Peter Ujfalusi , Boris Brezillon , Tomi Valkeinen , "open list:MEMORY TECHNOLOGY..." , linux-arm Mailing List , linux-input , Linux OMAP Mailing List , Jarkko Nikula On Friday, May 18, 2018 11:21:14 PM CEST Andy Shevchenko wrote: > On Sat, May 19, 2018 at 12:09 AM, Janusz Krzysztofik > > wrote: > > + gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > > + if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > So, is it optional or not at the end? > If it is, why do we check for NULL? As far as I can understand, nand_chip->dev_ready() callback is optional. That's why I decided to use the _optional variant of devm_gpiod_get(). In case of ams-delta, the dev_ready() callback depends on availability of the 'rdy' GPIO pin. As a consequence, I'm checking for both NULL and ERR in order to decide if dev_ready() will be supported. I can pretty well replace it with the standard form and check for ERR only if the purpose of the _optional form is different. > > this->dev_ready = ams_delta_nand_ready; > > > > } else { > > > > this->dev_ready = NULL; > > pr_notice("Couldn't request gpio for Delta NAND > > ready.\n"); > > dev_notice() ? Sure, but maybe in a separate patch? That's not a new code just being added but an existing one, not the merit of the change. > > } > > > > +err_gpiod: > > + if (err = -ENODEV || err = -ENOENT) > > + err = -EPROBE_DEFER; > > Hmm... Amstrad Delta uses gpio-mmio driver. Unfortunatelty that driver is not availble before device init phase, unlike other crucial GPIO drivers which are initialized earlier, e.g. during the postcore or at latetst the subsys phase. Hence, devices which depend on GPIO pins provided by gpio-mmio must either be declared late or fail softly so they get another chance of being probed succesfully. I thought of replacing the gpio-mmio platform driver with bgpio functions it exports but for now I haven't implemented it, not even shared the idea. Does it really hurt to return -EPROBE_DEFER if a GPIO pin can't be obtained? Thanks, Janusz From mboxrd@z Thu Jan 1 00:00:00 1970 From: Janusz Krzysztofik To: Andy Shevchenko Cc: Tony Lindgren , Dmitry Torokhov , Boris Brezillon , Tomi Valkeinen , Mark Brown , Aaro Koskinen , Richard Weinberger , Peter Ujfalusi , Jarkko Nikula , Liam Girdwood , linux-arm Mailing List , Linux OMAP Mailing List , Linux Kernel Mailing List , linux-input , "open list:MEMORY TECHNOLOGY..." , linux-fbdev@vger.kernel.org, ALSA Development Mailing List Subject: Re: [PATCH 5/6] mtd: rawnand: ams-delta: use GPIO lookup table Date: Sat, 19 May 2018 01:15:30 +0200 Message-ID: <3427199.r4OBoDP6Xz@z50> In-Reply-To: References: <20180518210954.29044-1-jmkrzyszt@gmail.com> <20180518210954.29044-5-jmkrzyszt@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Friday, May 18, 2018 11:21:14 PM CEST Andy Shevchenko wrote: > On Sat, May 19, 2018 at 12:09 AM, Janusz Krzysztofik > > wrote: > > + gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > > + if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > So, is it optional or not at the end? > If it is, why do we check for NULL? As far as I can understand, nand_chip->dev_ready() callback is optional. That's why I decided to use the _optional variant of devm_gpiod_get(). In case of ams-delta, the dev_ready() callback depends on availability of the 'rdy' GPIO pin. As a consequence, I'm checking for both NULL and ERR in order to decide if dev_ready() will be supported. I can pretty well replace it with the standard form and check for ERR only if the purpose of the _optional form is different. > > this->dev_ready = ams_delta_nand_ready; > > > > } else { > > > > this->dev_ready = NULL; > > pr_notice("Couldn't request gpio for Delta NAND > > ready.\n"); > > dev_notice() ? Sure, but maybe in a separate patch? That's not a new code just being added but an existing one, not the merit of the change. > > } > > > > +err_gpiod: > > + if (err == -ENODEV || err == -ENOENT) > > + err = -EPROBE_DEFER; > > Hmm... Amstrad Delta uses gpio-mmio driver. Unfortunatelty that driver is not availble before device init phase, unlike other crucial GPIO drivers which are initialized earlier, e.g. during the postcore or at latetst the subsys phase. Hence, devices which depend on GPIO pins provided by gpio-mmio must either be declared late or fail softly so they get another chance of being probed succesfully. I thought of replacing the gpio-mmio platform driver with bgpio functions it exports but for now I haven't implemented it, not even shared the idea. Does it really hurt to return -EPROBE_DEFER if a GPIO pin can't be obtained? Thanks, Janusz From mboxrd@z Thu Jan 1 00:00:00 1970 From: jmkrzyszt@gmail.com (Janusz Krzysztofik) Date: Sat, 19 May 2018 01:15:30 +0200 Subject: [PATCH 5/6] mtd: rawnand: ams-delta: use GPIO lookup table In-Reply-To: References: <20180518210954.29044-1-jmkrzyszt@gmail.com> <20180518210954.29044-5-jmkrzyszt@gmail.com> Message-ID: <3427199.r4OBoDP6Xz@z50> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Friday, May 18, 2018 11:21:14 PM CEST Andy Shevchenko wrote: > On Sat, May 19, 2018 at 12:09 AM, Janusz Krzysztofik > > wrote: > > + gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > > + if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > So, is it optional or not at the end? > If it is, why do we check for NULL? As far as I can understand, nand_chip->dev_ready() callback is optional. That's why I decided to use the _optional variant of devm_gpiod_get(). In case of ams-delta, the dev_ready() callback depends on availability of the 'rdy' GPIO pin. As a consequence, I'm checking for both NULL and ERR in order to decide if dev_ready() will be supported. I can pretty well replace it with the standard form and check for ERR only if the purpose of the _optional form is different. > > this->dev_ready = ams_delta_nand_ready; > > > > } else { > > > > this->dev_ready = NULL; > > pr_notice("Couldn't request gpio for Delta NAND > > ready.\n"); > > dev_notice() ? Sure, but maybe in a separate patch? That's not a new code just being added but an existing one, not the merit of the change. > > } > > > > +err_gpiod: > > + if (err == -ENODEV || err == -ENOENT) > > + err = -EPROBE_DEFER; > > Hmm... Amstrad Delta uses gpio-mmio driver. Unfortunatelty that driver is not availble before device init phase, unlike other crucial GPIO drivers which are initialized earlier, e.g. during the postcore or at latetst the subsys phase. Hence, devices which depend on GPIO pins provided by gpio-mmio must either be declared late or fail softly so they get another chance of being probed succesfully. I thought of replacing the gpio-mmio platform driver with bgpio functions it exports but for now I haven't implemented it, not even shared the idea. Does it really hurt to return -EPROBE_DEFER if a GPIO pin can't be obtained? Thanks, Janusz