Message ID | 20230926145943.42814-1-brgl@bgdev.pl |
---|---|
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp2140540vqu; Tue, 26 Sep 2023 12:20:02 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFkYYvcivE2tqpy8IigyTYB1qj1GpX66h3tQ2/Z/ty7d4XSU6NQf7tFBuUX6/GAPedFp3dj X-Received: by 2002:a05:6a21:4982:b0:154:b1f7:b396 with SMTP id ax2-20020a056a21498200b00154b1f7b396mr10503233pzc.35.1695756001912; Tue, 26 Sep 2023 12:20:01 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695756001; cv=none; d=google.com; s=arc-20160816; b=eqq7fObp2DjkufRa8ErcETdR3WIf8WzHNvdppZBK+135HL9mKQPaLfOaOtOP/1mHtL 3hgaP/nv+OkJKEFxSUtHediiP1xsm0ytEeu62ets87TL0eQxpAc0y1zZGSErG66wBrs8 UYlynRAxsdK/qYRYpEa6pNys6h0CnM2D/JVz5coXm9X+1eymrjRpi4WTW+wmbkOXFDwU TIvaf7HFY7/YE4vpj4p5e83jTGXPo0HwzaFw1Pwfjyj402sN7wboE3g1bl5fKINsNGzq 9OfvWS0UozQcoJgQCjtQQubMZF9r2RBE0Sh/Q3ZYQ9rlYYoC8dfrzLQV7xLyOglC7ihz teHw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=vfLYtgRnxIhfGIuZ3AuQPnmd6mF7Zk5St0BXq4vGsIQ=; fh=Tss7K8NVbsMYNZ1fG9jf8bECM8zIrRfQ37piI3ZudAE=; b=OvJK/M4pExqAliqOTiyf2TyTrbNiWNHd74Hk6mllju1pI+mYGF/E39XJKnMVDdxVm8 PCxyW4LF0rwSJj+CJlkrHNLz2KjDEuIz60iCMhE5aDm7pcjK/+TnOGs+4AICP0abPWz+ 6Pi2phVbNuVEvO87j+YHd8i458Mb3u89euo/3Lsw2yGKM/sM3wsUsGdYU2rptzu97u+Q HesT6jT8heCWad/Hw3H7OG7YpdU4VhsxxVIisoF+rLSiONjC69IrWqE0rBGS1ir3DwfG Wcycir1ZEycuFyvRcKYA12shGEPxynCHgpgLYhYx1bL0cy/3pUL4Ar/26uq13nmuqR8D HBow== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=TLzw53xl; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from pete.vger.email (pete.vger.email. [2620:137:e000::3:6]) by mx.google.com with ESMTPS id s19-20020a170903201300b001c353153012si12633602pla.415.2023.09.26.12.20.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 12:20:01 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6; Authentication-Results: mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=TLzw53xl; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id 90F6F81C396B; Tue, 26 Sep 2023 08:00:04 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234994AbjIZPAB (ORCPT <rfc822;pwkd43@gmail.com> + 28 others); Tue, 26 Sep 2023 11:00:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45330 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229841AbjIZO77 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 26 Sep 2023 10:59:59 -0400 Received: from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com [IPv6:2a00:1450:4864:20::42c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B9B178E for <linux-kernel@vger.kernel.org>; Tue, 26 Sep 2023 07:59:52 -0700 (PDT) Received: by mail-wr1-x42c.google.com with SMTP id ffacd0b85a97d-32167a4adaaso8732733f8f.1 for <linux-kernel@vger.kernel.org>; Tue, 26 Sep 2023 07:59:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20230601.gappssmtp.com; s=20230601; t=1695740391; x=1696345191; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=vfLYtgRnxIhfGIuZ3AuQPnmd6mF7Zk5St0BXq4vGsIQ=; b=TLzw53xlfCA4B2yyekGcmN2Gow3tYsMQZDMag7APmsRKZLKRwPcuPoc7Xfm9pZ1N8A i8XZ6fbmgckGNwnl/tB/4I5ppkDl4KZpStL4Xiv9ChBKbHtPW+I1ipHDcBULsm4XLdE3 aBu2drv72zmlMLQSx51xCIobXP2ySCWI7bFE7fqUuQCm41h1B3nTdBqVBLZk+3Wqr9TQ UQcxODnyP08mnU+n1WxUthfrUHGcjCBdV14jAzYfqloN4c3Y2vLkGRyoEGR/WlHc5Kh+ TuNVFJ7KMX7DuFMdygi7a54VJGyS0URP4mSdyyDhwbZ4H+ipRDjnQ5o8xw/GHYglb4jt R8xw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695740391; x=1696345191; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=vfLYtgRnxIhfGIuZ3AuQPnmd6mF7Zk5St0BXq4vGsIQ=; b=BeRVa8BDIoswwIVnDMbybcD9DV8CEyaUMrj+lzgTTjc/eTS73kc1TK+9Ni+cfmm1po cnJRTI9AxVlExDoC5ky+P0XRJ3qNDY3v7oRU0YTniiaKOe9bBAXDQl+wu52j85a3+vP8 SqbC5n+8lQJYBbWFacT2JX+dvRpyNk+gdph6Ta6JeY6oKfsVq8lSCRv5N0VGQ3HXj237 aD5UY0ZnT7e85mJADhwY5hRiDRcoIOv0htKkxf4wEKMEuHrSgFt7xM+L0PgPJQ9jSURM m/ICP7YH1FgsACkWFmoDFNyg8V7sGqsevf+girsiky1DqlOgm4/KP+Pkm/EAjZMe1ucW XptQ== X-Gm-Message-State: AOJu0YynAZ8mcmeoTX0lUk2X3vJxlx1tRLWAlsINEo17YhHozHrqI07/ 4LKeudrJxKmP+tVK4JJF0YM+eQ== X-Received: by 2002:a5d:44ca:0:b0:31f:d52a:82af with SMTP id z10-20020a5d44ca000000b0031fd52a82afmr8678818wrr.49.1695740391015; Tue, 26 Sep 2023 07:59:51 -0700 (PDT) Received: from brgl-uxlite.home ([2a01:cb1d:334:ac00:686:c497:30b1:d2b9]) by smtp.gmail.com with ESMTPSA id s2-20020a5d5102000000b0031f82743e25sm14871634wrt.67.2023.09.26.07.59.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 07:59:50 -0700 (PDT) From: Bartosz Golaszewski <brgl@bgdev.pl> To: Mika Westerberg <mika.westerberg@linux.intel.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Linus Walleij <linus.walleij@linaro.org>, Daniel Scally <djrscally@gmail.com>, Hans de Goede <hdegoede@redhat.com>, Mark Gross <markgross@kernel.org> Cc: linux-gpio@vger.kernel.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org, Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Subject: [RFT PATCH 0/4] platform/x86: int3472: don't use gpiod_toggle_active_low() Date: Tue, 26 Sep 2023 16:59:39 +0200 Message-Id: <20230926145943.42814-1-brgl@bgdev.pl> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Tue, 26 Sep 2023 08:00:04 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778129045664521669 X-GMAIL-MSGID: 1778129045664521669 |
Series |
platform/x86: int3472: don't use gpiod_toggle_active_low()
|
|
Message
Bartosz Golaszewski
Sept. 26, 2023, 2:59 p.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
gpiod_toggle_active_low() is a badly designed API that should have never
been used elsewhere then in the MMC code. And even there we should find
a better solution.
Replace the uses of it in the int3472 driver with the good old temporary
lookup table trick. This is not very pretty either but it's the lesser
evil.
Bartosz Golaszewski (4):
platform/x86: int3472: provide a helper for getting GPIOs from lookups
platform/x86: int3472: led: don't use gpiod_toggle_active_low()
platform/x86: int3472: clk_and_regulator: use GPIO lookup tables
gpio: acpi: remove acpi_get_and_request_gpiod()
drivers/gpio/gpiolib-acpi.c | 28 ------------------
.../x86/intel/int3472/clk_and_regulator.c | 22 ++++++--------
drivers/platform/x86/intel/int3472/common.c | 29 +++++++++++++++++++
drivers/platform/x86/intel/int3472/common.h | 9 ++++++
drivers/platform/x86/intel/int3472/led.c | 12 +++-----
include/linux/gpio/consumer.h | 8 -----
6 files changed, 51 insertions(+), 57 deletions(-)
Comments
On Tue, Sep 26, 2023 at 04:59:43PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > With no more users, we can remove acpi_get_and_request_gpiod(). The best patch in the series! Reviewed-by: From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Tue, Sep 26, 2023 at 04:59:39PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > gpiod_toggle_active_low() is a badly designed API that should have never > been used elsewhere then in the MMC code. And even there we should find > a better solution. > > Replace the uses of it in the int3472 driver with the good old temporary > lookup table trick. This is not very pretty either but it's the lesser > evil. Good jon! I have only style issues, otherwise Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Hi Bartosz, On 9/26/23 16:59, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > gpiod_toggle_active_low() is a badly designed API that should have never > been used elsewhere then in the MMC code. And even there we should find > a better solution. > > Replace the uses of it in the int3472 driver with the good old temporary > lookup table trick. This is not very pretty either but it's the lesser > evil. I saw your previous proposal which added a new api to directly set the active_low flag, rather then toggle it. I intended to reply to that thread to say that I liked that approach, but I don't remember if I actually did reply. I wonder what made you abandon the new function to directly set the active-low flag on a gpio_desc? For the int3472 code that would work pretty well and it would be much cleaner then the temp gpio-lookup approach. Regards, Hans > > Bartosz Golaszewski (4): > platform/x86: int3472: provide a helper for getting GPIOs from lookups > platform/x86: int3472: led: don't use gpiod_toggle_active_low() > platform/x86: int3472: clk_and_regulator: use GPIO lookup tables > gpio: acpi: remove acpi_get_and_request_gpiod() > > drivers/gpio/gpiolib-acpi.c | 28 ------------------ > .../x86/intel/int3472/clk_and_regulator.c | 22 ++++++-------- > drivers/platform/x86/intel/int3472/common.c | 29 +++++++++++++++++++ > drivers/platform/x86/intel/int3472/common.h | 9 ++++++ > drivers/platform/x86/intel/int3472/led.c | 12 +++----- > include/linux/gpio/consumer.h | 8 ----- > 6 files changed, 51 insertions(+), 57 deletions(-) >
Hi, On 9/27/23 10:38, Hans de Goede wrote: > Hi Bartosz, > > On 9/26/23 16:59, Bartosz Golaszewski wrote: >> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >> >> gpiod_toggle_active_low() is a badly designed API that should have never >> been used elsewhere then in the MMC code. And even there we should find >> a better solution. >> >> Replace the uses of it in the int3472 driver with the good old temporary >> lookup table trick. This is not very pretty either but it's the lesser >> evil. > > I saw your previous proposal which added a new api to directly set > the active_low flag, rather then toggle it. > > I intended to reply to that thread to say that I liked that approach, > but I don't remember if I actually did reply. > > I wonder what made you abandon the new function to directly set > the active-low flag on a gpio_desc? > > For the int3472 code that would work pretty well and it would > be much cleaner then the temp gpio-lookup approach. I missed that 4/4 removes acpi_get_and_request_gpiod(), so I guess that this is not just only about removing gpiod_toggle_active_low() but also about removing gpiod_toggle_active_low() ? Regards, Hans >> >> Bartosz Golaszewski (4): >> platform/x86: int3472: provide a helper for getting GPIOs from lookups >> platform/x86: int3472: led: don't use gpiod_toggle_active_low() >> platform/x86: int3472: clk_and_regulator: use GPIO lookup tables >> gpio: acpi: remove acpi_get_and_request_gpiod() >> >> drivers/gpio/gpiolib-acpi.c | 28 ------------------ >> .../x86/intel/int3472/clk_and_regulator.c | 22 ++++++-------- >> drivers/platform/x86/intel/int3472/common.c | 29 +++++++++++++++++++ >> drivers/platform/x86/intel/int3472/common.h | 9 ++++++ >> drivers/platform/x86/intel/int3472/led.c | 12 +++----- >> include/linux/gpio/consumer.h | 8 ----- >> 6 files changed, 51 insertions(+), 57 deletions(-) >>
On Wed, Sep 27, 2023 at 10:38 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Bartosz, > > On 9/26/23 16:59, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > gpiod_toggle_active_low() is a badly designed API that should have never > > been used elsewhere then in the MMC code. And even there we should find > > a better solution. > > > > Replace the uses of it in the int3472 driver with the good old temporary > > lookup table trick. This is not very pretty either but it's the lesser > > evil. > > I saw your previous proposal which added a new api to directly set > the active_low flag, rather then toggle it. > > I intended to reply to that thread to say that I liked that approach, > but I don't remember if I actually did reply. > > I wonder what made you abandon the new function to directly set > the active-low flag on a gpio_desc? > > For the int3472 code that would work pretty well and it would > be much cleaner then the temp gpio-lookup approach. > You did reply, yes. Under one of the other patches Linus W stated that first: adding the ability for consumers to toggle the polarity was added to handle the MMC slot quirk, then it was used unknowingly to GPIO maintainers in other places (including this driver). I then acknowledged the fact that it should have never existed in the first place as this is HW description and should be defined in ACPI, DT or lookup flags. I'm not sure why this information needs to be hard-coded in the driver in int3472_get_func_and_polarity() but maybe it could be pulled into gpiolib-acpi.c with other quirks? Bart
Hi Bartosz, On 9/27/23 10:48, Bartosz Golaszewski wrote: > On Wed, Sep 27, 2023 at 10:38 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Bartosz, >> >> On 9/26/23 16:59, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>> >>> gpiod_toggle_active_low() is a badly designed API that should have never >>> been used elsewhere then in the MMC code. And even there we should find >>> a better solution. >>> >>> Replace the uses of it in the int3472 driver with the good old temporary >>> lookup table trick. This is not very pretty either but it's the lesser >>> evil. >> >> I saw your previous proposal which added a new api to directly set >> the active_low flag, rather then toggle it. >> >> I intended to reply to that thread to say that I liked that approach, >> but I don't remember if I actually did reply. >> >> I wonder what made you abandon the new function to directly set >> the active-low flag on a gpio_desc? >> >> For the int3472 code that would work pretty well and it would >> be much cleaner then the temp gpio-lookup approach. >> > > You did reply, yes. Under one of the other patches Linus W stated that > first: adding the ability for consumers to toggle the polarity was > added to handle the MMC slot quirk, then it was used unknowingly to > GPIO maintainers in other places (including this driver). I then > acknowledged the fact that it should have never existed in the first > place as this is HW description and should be defined in ACPI, DT or > lookup flags. I see and I understand. > I'm not sure why this information needs to be hard-coded in the driver > in int3472_get_func_and_polarity() but maybe it could be pulled into > gpiolib-acpi.c with other quirks? The problem is that for camera sensors Intel uses this special INT3472 ACPI device with a custom _DSM to list GPIOs, with the _DSM returning an u32 and one of the bits in the u32 is the polarity. We really do not want to deal with this Intel camera team hack inside gpiolib-acpi and I can understand why you and Linus W want to get rid of functions which allow drivers to meddle with a gpio_desc's active-low flag. So using a temporary gpio-lookup in the int3472 code as you are proposing is the best (least bad) thing to do here then. I'll try to make some time to test this sometime the coming days. Other then the discussion we just had is there any specific reason why this should be considered a RFC / why this would not be ready for merging? (I still need to review these, but lets assume that goes well) Regards, Hans
On Wed, Sep 27, 2023 at 11:02 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Bartosz, > > On 9/27/23 10:48, Bartosz Golaszewski wrote: > > On Wed, Sep 27, 2023 at 10:38 AM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi Bartosz, > >> > >> On 9/26/23 16:59, Bartosz Golaszewski wrote: > >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > >>> > >>> gpiod_toggle_active_low() is a badly designed API that should have never > >>> been used elsewhere then in the MMC code. And even there we should find > >>> a better solution. > >>> > >>> Replace the uses of it in the int3472 driver with the good old temporary > >>> lookup table trick. This is not very pretty either but it's the lesser > >>> evil. > >> > >> I saw your previous proposal which added a new api to directly set > >> the active_low flag, rather then toggle it. > >> > >> I intended to reply to that thread to say that I liked that approach, > >> but I don't remember if I actually did reply. > >> > >> I wonder what made you abandon the new function to directly set > >> the active-low flag on a gpio_desc? > >> > >> For the int3472 code that would work pretty well and it would > >> be much cleaner then the temp gpio-lookup approach. > >> > > > > You did reply, yes. Under one of the other patches Linus W stated that > > first: adding the ability for consumers to toggle the polarity was > > added to handle the MMC slot quirk, then it was used unknowingly to > > GPIO maintainers in other places (including this driver). I then > > acknowledged the fact that it should have never existed in the first > > place as this is HW description and should be defined in ACPI, DT or > > lookup flags. > > I see and I understand. > > > I'm not sure why this information needs to be hard-coded in the driver > > in int3472_get_func_and_polarity() but maybe it could be pulled into > > gpiolib-acpi.c with other quirks? > > The problem is that for camera sensors Intel uses this special > INT3472 ACPI device with a custom _DSM to list GPIOs, with the _DSM > returning an u32 and one of the bits in the u32 is the polarity. > > We really do not want to deal with this Intel camera team hack > inside gpiolib-acpi and I can understand why you and Linus W > want to get rid of functions which allow drivers to meddle > with a gpio_desc's active-low flag. > > So using a temporary gpio-lookup in the int3472 code as > you are proposing is the best (least bad) thing to do > here then. > > I'll try to make some time to test this sometime > the coming days. > > Other then the discussion we just had is there any specific > reason why this should be considered a RFC / why this would > not be ready for merging? (I still need to review these, > but lets assume that goes well) > This is not an RFC but rather RFT - Request For Testing. I don't have any HW to test those with so I only built it. Bart