Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756136AbdGKRUz (ORCPT ); Tue, 11 Jul 2017 13:20:55 -0400 Received: from mail-pf0-f194.google.com ([209.85.192.194]:34008 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755370AbdGKRUx (ORCPT ); Tue, 11 Jul 2017 13:20:53 -0400 Subject: Re: [PATCH v1 1/1] gpio: gpio-crystalcove: Skip IRQ CTRL register update for virtual GPIOs To: Hans de Goede , sathyanarayanan.kuppuswamy@linux.intel.com, Andy Shevchenko Cc: Linus Walleij , "linux-gpio@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <5fabde8f9366fce42b5f361f3472bc91754ca7db.1497482431.git.sathyanarayanan.kuppuswamy@linux.intel.com> <167cd7bd-1e70-d44a-180c-da890a017110@linux.intel.com> <9fd783e9-3c53-582f-24f9-97901d63501b@linux.intel.com> <909b020e-75eb-b3ab-7123-f3e3f41143c9@redhat.com> From: "Kuppuswamy, Sathyanarayanan" Message-ID: Date: Tue, 11 Jul 2017 10:20:50 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <909b020e-75eb-b3ab-7123-f3e3f41143c9@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2507 Lines: 79 Hi Hans, On 7/11/2017 2:47 AM, Hans de Goede wrote: > Hi, > > On 11-07-17 01:35, sathyanarayanan kuppuswamy wrote: >> Hi Hans, >> >> Do you have any comments on this patch ? It kind of fixes your patch, >> so would prefer to get your comments. > > Sorry I did not notice this patch before, did you Cc me ? > > As for the patch, I deliberately did not add the check > to crystalcove_update_irq_ctrl, crystalcove_update_irq_ctrl > only gets called from crystalcove_bus_sync_unlock if > UPDATE_IRQ_TYPE > > UPDATE_IRQ_TYPE only gets set from crystalcove_irq_type > which at the top contains: > > if (data->hwirq >= CRYSTALCOVE_GPIO_NUM) > return 0; > > So crystalcove_update_irq_ctrl will never get called for > virtual GPIOs. > > TL;DR: your patch is not necessary. Thanks for the clarification. > > Regards, > > Hans > > >> >> >> On 06/15/2017 02:45 PM, sathyanarayanan kuppuswamy wrote: >>> Hi Andy, >>> >>> >>> On 06/15/2017 02:19 AM, Andy Shevchenko wrote: >>>> On Thu, Jun 15, 2017 at 2:21 AM, >>>> wrote: >>>>> From: Kuppuswamy Sathyanarayanan >>>>> >>>>> >>>>> Commit 9a752b4c9ab9 ("gpio: crystalcove: Do not write regular gpio >>>>> registers for virtual GPIOs") added support to skip GPIO register >>>>> update for virtual GPIOs, but it missed to add skip logic in >>>>> crystalcove_update_irq_ctrl() function. This patch fixes it. >>>>> @@ -134,6 +134,9 @@ static void crystalcove_update_irq_ctrl(struct >>>>> crystalcove_gpio *cg, int gpio) >>>>> { >>>>> int reg = to_reg(gpio, CTRL_IN); >>>>> >>>>> + if (reg < 0) >>>>> + return; >>>>> + >>>>> regmap_update_bits(cg->regmap, reg, CTLI_INTCNT_BE, >>>>> cg->intcnt_value); >>>>> } >>>> Shouldn't it have been done using irq_valid_mask flag in the first >>>> place? >>> Agree. Setting irq_valid_mask would be the proper approach to skip >>> IRQ for some GPIO pins. But commit 9a752b4c9ab9 added the GPIO index >>> based checks in other IRQ set/unset functions in this driver and >>> missed to add it only in this update_irq_ctrl() function. May be I >>> can submit a separate patch to clean it up and use logic based on >>> setting irq_valid_mask. >>> >>> Even if we do the cleanup patch, I would still prefer to have this >>> check. Since to_reg() can return value < 0, its good to check it >>> before passing it to regmap_* functions. Let me know your comments. >>> >>>> >>> >>