Message ID | 20230926145943.42814-3-brgl@bgdev.pl |
---|---|
State | New |
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 r8csp2045199vqu; Tue, 26 Sep 2023 09:32:52 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH0aY/pGev9PoglK0neFEI4xPVjeMzMO2g84z91/FIEInf/Sr3oP8/wnhTASYglZyYHCypI X-Received: by 2002:a17:903:234d:b0:1c6:1fc3:6857 with SMTP id c13-20020a170903234d00b001c61fc36857mr6316799plh.27.1695745971533; Tue, 26 Sep 2023 09:32:51 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695745971; cv=none; d=google.com; s=arc-20160816; b=cxvh5S1xCk6FcN7vaJoEtP2aPn+V9AYmEpGPBtivFguo0pPzNlJm0dKr8MapDYALyq 24I4pnk9QyQFXSSsItFnVKc9g0pNRZkCD00jTCTFmBKXpbNUxNgpHwqDKMn4oTCh96sJ NCoFofiE+PVrZ12Nm9hbFk7XTow5dgXEGMUkqZpJVrsMYlkNvNqPW2MI/jnZPZLKfTlc zBBSb6kKw16q0h8r0R1CXUtGYYWIFMlRCWHjbVRAlnIUDK5dD7GVOb9krZ4uf3ertOb5 lABN4Hh7GS8LVaiTTf1UxdYXL7O8bkzYgxiRBbPOXnYJiIboqEudWI68MIdj4QBEPiye RbXA== 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 :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=LOR2X/AoUnTx42nPhZDbyArkpmdX3bsMYj2lZRsFUms=; fh=Tss7K8NVbsMYNZ1fG9jf8bECM8zIrRfQ37piI3ZudAE=; b=sjoQYZdvXI5uY4ETaYgLvGyNZtoTPzeW9PxboMHvkPKvvhP+0NspnXhTXW9ZNhM1W0 7HBWaWsPf7Xp6UXMJrPambvA75A12s382g3ishyupgPe4SM3EmYhd1VP5VnnOxHEXz2a l8CCqP5zx2NPfYvfeYUoJmNMwcOL6VGWYGEF4DWshF/If76gdxTK+HVqH8Yc7YBAMPiU fV6iRNPGGolBiYtGy50XSKy09AamS7ZoQ3o8AxXF0o1JXQADF8RRnI5crQPyrAsPjI8i 1RMsWY3jVDkVsNRLrE8PQoM3n9bHLofUw9ONasQB/nhNn6gfHUe+eGnsK4bu4AFYKBrc owDA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=zOZwMSAX; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from lipwig.vger.email (lipwig.vger.email. [23.128.96.33]) by mx.google.com with ESMTPS id ky4-20020a170902f98400b001c20db2510asi10808978plb.53.2023.09.26.09.32.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 09:32:51 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33; Authentication-Results: mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=zOZwMSAX; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 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 lipwig.vger.email (Postfix) with ESMTP id B941082554A6; Tue, 26 Sep 2023 08:00:24 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at lipwig.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235017AbjIZPAG (ORCPT <rfc822;pwkd43@gmail.com> + 28 others); Tue, 26 Sep 2023 11:00:06 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55040 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234996AbjIZPAB (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 26 Sep 2023 11:00:01 -0400 Received: from mail-wr1-x429.google.com (mail-wr1-x429.google.com [IPv6:2a00:1450:4864:20::429]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AF8C7127 for <linux-kernel@vger.kernel.org>; Tue, 26 Sep 2023 07:59:54 -0700 (PDT) Received: by mail-wr1-x429.google.com with SMTP id ffacd0b85a97d-32336a30d18so1508785f8f.2 for <linux-kernel@vger.kernel.org>; Tue, 26 Sep 2023 07:59:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20230601.gappssmtp.com; s=20230601; t=1695740393; x=1696345193; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=LOR2X/AoUnTx42nPhZDbyArkpmdX3bsMYj2lZRsFUms=; b=zOZwMSAXHjeLQGWcL2Uhltnz2hlI7XNV9ewI0AYTJIyufx2fw5+odUKZX3SF3OdVo2 UlQEE0zam+rMuUY+dmj78myXlrMmWSkfn37NQb+yQn2yRzuAYt1mHQeX69fRpj3LWgoq DnRkDh1CFa3VtNEjAJi12cVCeQPSzMIuLFEBnrWlbPdaoXkN0akjsISOdCiNgMPoHRLF r7kNO0MescDjiY5P31lLEnDKROy/OtDt6ZI8ox9V10HkOMJPo00rSnEjLYDdN9ij1f9G l36r3oZGDy/tqplSiP6d6VejCSKVhHopDn8QC2mOJlzpGtJlbgtq+H3NJEoLoy5x/ooi jU0Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695740393; x=1696345193; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=LOR2X/AoUnTx42nPhZDbyArkpmdX3bsMYj2lZRsFUms=; b=GDKlnZjSkY7mE15XLtRf76hlWCNPpmAe9M1h415Lfhxhd4S0hf1hoZ7j0H+uG+6/k7 vryjlOfuK8aKJI/hZ5+8b0NLJunyIXRIISC7IHbose6ZVsULC+nmpZGphhIshAVyf2Gf TLTu+vcTDqntwsxwJUpbQGLE+9fnu5/u0dYCkDYltE2hjmVtcJkCuekX8YdAiHtKB0CF AVRjZmJFXGYxgDnr7s5KlQcdz4xbmxXbM0sztWiE4Ip9SSWPbZKP+Mtf5jdrUf56A6iL wY3c1LZbzjdghlsaJwF8NrP5Tq00jJup2YPcS2XGc8R4/KE3zItUjanW24OaeiQ2KPyI 1UqA== X-Gm-Message-State: AOJu0YwRP19T/N0Oo22/IwgJknmpmpfspHwc79OwW0H26baDUhNy7J+/ 875+Zv5e46xm6n4j3Oa3uKRfLw== X-Received: by 2002:adf:f044:0:b0:319:7134:a3cf with SMTP id t4-20020adff044000000b003197134a3cfmr9483453wro.31.1695740393042; Tue, 26 Sep 2023 07:59:53 -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.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 07:59:52 -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 2/4] platform/x86: int3472: led: don't use gpiod_toggle_active_low() Date: Tue, 26 Sep 2023 16:59:41 +0200 Message-Id: <20230926145943.42814-3-brgl@bgdev.pl> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230926145943.42814-1-brgl@bgdev.pl> References: <20230926145943.42814-1-brgl@bgdev.pl> 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 lipwig.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 (lipwig.vger.email [0.0.0.0]); Tue, 26 Sep 2023 08:00:24 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778118527965066373 X-GMAIL-MSGID: 1778118527965066373 |
Series |
platform/x86: int3472: don't use gpiod_toggle_active_low()
|
|
Commit Message
Bartosz Golaszewski
Sept. 26, 2023, 2:59 p.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use temporary lookup tables with appropriate lookup flags. Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> --- drivers/platform/x86/intel/int3472/led.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
Comments
On Tue, Sep 26, 2023 at 04:59:41PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use > temporary lookup tables with appropriate lookup flags. ... > + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( > + int3472->dev, path, agpio->pin_table[0], > + "int3472,privacy-led", polarity, > + GPIOD_OUT_LOW); Personally I found this style weird. I prefer to have longer line over the split on the parentheses.
On Tue, Sep 26, 2023 at 5:27 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Sep 26, 2023 at 04:59:41PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use > > temporary lookup tables with appropriate lookup flags. > > ... > > > + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( > > + int3472->dev, path, agpio->pin_table[0], > > + "int3472,privacy-led", polarity, > > + GPIOD_OUT_LOW); > > Personally I found this style weird. I prefer to have longer line over > the split on the parentheses. > I in turn prefer this one. Checkpatch doesn't complain either way so I'll leave it to the maintainers of this driver to decide. Bart
HI, On 9/27/23 09:02, Bartosz Golaszewski wrote: > On Tue, Sep 26, 2023 at 5:27 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: >> >> On Tue, Sep 26, 2023 at 04:59:41PM +0200, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>> >>> Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use >>> temporary lookup tables with appropriate lookup flags. >> >> ... >> >>> + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( >>> + int3472->dev, path, agpio->pin_table[0], >>> + "int3472,privacy-led", polarity, >>> + GPIOD_OUT_LOW); >> >> Personally I found this style weird. I prefer to have longer line over >> the split on the parentheses. >> > > I in turn prefer this one. Checkpatch doesn't complain either way so > I'll leave it to the maintainers of this driver to decide. I'm fine with keeping this as is, using longer lines does not seem to make things better here. Regards, Hans
Hi, On 9/26/23 16:59, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use > temporary lookup tables with appropriate lookup flags. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/platform/x86/intel/int3472/led.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c > index bca1ce7d0d0c..62e0cd5207a7 100644 > --- a/drivers/platform/x86/intel/int3472/led.c > +++ b/drivers/platform/x86/intel/int3472/led.c > @@ -25,18 +25,14 @@ int skl_int3472_register_pled(struct int3472_discrete_device *int3472, > if (int3472->pled.classdev.dev) > return -EBUSY; > > - int3472->pled.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0], > - "int3472,privacy-led"); > + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( > + int3472->dev, path, agpio->pin_table[0], > + "int3472,privacy-led", polarity, > + GPIOD_OUT_LOW); Yeah so this is not going to work, path here is an ACPI device path, e.g. on my laptop (which actually uses the INT3472 glue code) the path-s of the 2 GPIO controllers are: `\_SB_.GPI0` resp `\_SB_.PC00.XHCI.RHUB.HS08.VGPO` Where as skl_int3472_gpiod_get_from_temp_lookup() stores the passed in path in gpiod_lookup_table.table[0].key, which is the dev_name() of the GPIO controller's parent dev which are `INTC1055:00` resp. `INTC1096:00` . So we are going to need to add some code to INT3472 to go from path to a correct value for gpiod_lookup_table.table[0].key which means partly reproducing most of acpi_get_gpiod: struct gpio_chip *chip; acpi_handle handle; acpi_status status; status = acpi_get_handle(NULL, path, &handle); if (ACPI_FAILURE(status)) return ERR_PTR(-ENODEV); chip = gpiochip_find(handle, acpi_gpiochip_find); if (!chip) return ERR_PTR(-EPROBE_DEFER); And then get the key from the chip. Which means using gpiochip_find in the int3472 code now, which does not sound like an improvement. I think that was is needed instead is adding an active_low flag to acpi_get_and_request_gpiod() and then have that directly set the active-low flag on the returned desc. Regards, Hans > if (IS_ERR(int3472->pled.gpio)) > return dev_err_probe(int3472->dev, PTR_ERR(int3472->pled.gpio), > "getting privacy LED GPIO\n"); > > - if (polarity == GPIO_ACTIVE_LOW) > - gpiod_toggle_active_low(int3472->pled.gpio); > - > - /* Ensure the pin is in output mode and non-active state */ > - gpiod_direction_output(int3472->pled.gpio, 0); > - > /* Generate the name, replacing the ':' in the ACPI devname with '_' */ > snprintf(int3472->pled.name, sizeof(int3472->pled.name), > "%s::privacy_led", acpi_dev_name(int3472->sensor));
On Wed, Sep 27, 2023 at 11:40 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 9/26/23 16:59, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use > > temporary lookup tables with appropriate lookup flags. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > drivers/platform/x86/intel/int3472/led.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c > > index bca1ce7d0d0c..62e0cd5207a7 100644 > > --- a/drivers/platform/x86/intel/int3472/led.c > > +++ b/drivers/platform/x86/intel/int3472/led.c > > @@ -25,18 +25,14 @@ int skl_int3472_register_pled(struct int3472_discrete_device *int3472, > > if (int3472->pled.classdev.dev) > > return -EBUSY; > > > > - int3472->pled.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0], > > - "int3472,privacy-led"); > > + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( > > + int3472->dev, path, agpio->pin_table[0], > > + "int3472,privacy-led", polarity, > > + GPIOD_OUT_LOW); > > Yeah so this is not going to work, path here is an ACPI device path, e.g. > on my laptop (which actually uses the INT3472 glue code) the path-s of > the 2 GPIO controllers are: `\_SB_.GPI0` resp `\_SB_.PC00.XHCI.RHUB.HS08.VGPO` > > Where as skl_int3472_gpiod_get_from_temp_lookup() stores the passed in path > in gpiod_lookup_table.table[0].key, which is the dev_name() of the GPIO > controller's parent dev which are `INTC1055:00` resp. `INTC1096:00` . > > So we are going to need to add some code to INT3472 to go from path to > a correct value for gpiod_lookup_table.table[0].key which means partly > reproducing most of acpi_get_gpiod: > > struct gpio_chip *chip; > acpi_handle handle; > acpi_status status; > > status = acpi_get_handle(NULL, path, &handle); > if (ACPI_FAILURE(status)) > return ERR_PTR(-ENODEV); > > chip = gpiochip_find(handle, acpi_gpiochip_find); > if (!chip) > return ERR_PTR(-EPROBE_DEFER); > > And then get the key from the chip. Which means using gpiochip_find > in the int3472 code now, which does not sound like an improvement. > > I think that was is needed instead is adding an active_low flag > to acpi_get_and_request_gpiod() and then have that directly > set the active-low flag on the returned desc. > Ultimately I'd like everyone to use gpiod_get() for getting descriptors but for now I get it's enough. Are you find with this being done in a single patch across GPIO and this driver? Bart > Regards, > > Hans > > > > > > > > > > if (IS_ERR(int3472->pled.gpio)) > > return dev_err_probe(int3472->dev, PTR_ERR(int3472->pled.gpio), > > "getting privacy LED GPIO\n"); > > > > - if (polarity == GPIO_ACTIVE_LOW) > > - gpiod_toggle_active_low(int3472->pled.gpio); > > - > > - /* Ensure the pin is in output mode and non-active state */ > > - gpiod_direction_output(int3472->pled.gpio, 0); > > - > > /* Generate the name, replacing the ':' in the ACPI devname with '_' */ > > snprintf(int3472->pled.name, sizeof(int3472->pled.name), > > "%s::privacy_led", acpi_dev_name(int3472->sensor)); >
Hi Bart, On 9/27/23 12:44, Bartosz Golaszewski wrote: > On Wed, Sep 27, 2023 at 11:40 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 9/26/23 16:59, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>> >>> Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use >>> temporary lookup tables with appropriate lookup flags. >>> >>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>> --- >>> drivers/platform/x86/intel/int3472/led.c | 12 ++++-------- >>> 1 file changed, 4 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c >>> index bca1ce7d0d0c..62e0cd5207a7 100644 >>> --- a/drivers/platform/x86/intel/int3472/led.c >>> +++ b/drivers/platform/x86/intel/int3472/led.c >>> @@ -25,18 +25,14 @@ int skl_int3472_register_pled(struct int3472_discrete_device *int3472, >>> if (int3472->pled.classdev.dev) >>> return -EBUSY; >>> >>> - int3472->pled.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0], >>> - "int3472,privacy-led"); >>> + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( >>> + int3472->dev, path, agpio->pin_table[0], >>> + "int3472,privacy-led", polarity, >>> + GPIOD_OUT_LOW); >> >> Yeah so this is not going to work, path here is an ACPI device path, e.g. >> on my laptop (which actually uses the INT3472 glue code) the path-s of >> the 2 GPIO controllers are: `\_SB_.GPI0` resp `\_SB_.PC00.XHCI.RHUB.HS08.VGPO` >> >> Where as skl_int3472_gpiod_get_from_temp_lookup() stores the passed in path >> in gpiod_lookup_table.table[0].key, which is the dev_name() of the GPIO >> controller's parent dev which are `INTC1055:00` resp. `INTC1096:00` . >> >> So we are going to need to add some code to INT3472 to go from path to >> a correct value for gpiod_lookup_table.table[0].key which means partly >> reproducing most of acpi_get_gpiod: >> >> struct gpio_chip *chip; >> acpi_handle handle; >> acpi_status status; >> >> status = acpi_get_handle(NULL, path, &handle); >> if (ACPI_FAILURE(status)) >> return ERR_PTR(-ENODEV); >> >> chip = gpiochip_find(handle, acpi_gpiochip_find); >> if (!chip) >> return ERR_PTR(-EPROBE_DEFER); >> >> And then get the key from the chip. Which means using gpiochip_find >> in the int3472 code now, which does not sound like an improvement. >> >> I think that was is needed instead is adding an active_low flag >> to acpi_get_and_request_gpiod() and then have that directly >> set the active-low flag on the returned desc. >> > > Ultimately I'd like everyone to use gpiod_get() for getting > descriptors but for now I get it's enough. Are you find with this > being done in a single patch across GPIO and this driver? Yes doing this in a single patch is fine. Also I'm fine with merging such a patch through the gpio tree . Regards, Hans >>> if (IS_ERR(int3472->pled.gpio)) >>> return dev_err_probe(int3472->dev, PTR_ERR(int3472->pled.gpio), >>> "getting privacy LED GPIO\n"); >>> >>> - if (polarity == GPIO_ACTIVE_LOW) >>> - gpiod_toggle_active_low(int3472->pled.gpio); >>> - >>> - /* Ensure the pin is in output mode and non-active state */ >>> - gpiod_direction_output(int3472->pled.gpio, 0); >>> - >>> /* Generate the name, replacing the ':' in the ACPI devname with '_' */ >>> snprintf(int3472->pled.name, sizeof(int3472->pled.name), >>> "%s::privacy_led", acpi_dev_name(int3472->sensor)); >> >
Hi Again, On 9/27/23 15:08, Hans de Goede wrote: > Hi Bart, > > On 9/27/23 12:44, Bartosz Golaszewski wrote: >> On Wed, Sep 27, 2023 at 11:40 AM Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> Hi, >>> >>> On 9/26/23 16:59, Bartosz Golaszewski wrote: >>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>>> >>>> Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use >>>> temporary lookup tables with appropriate lookup flags. >>>> >>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>>> --- >>>> drivers/platform/x86/intel/int3472/led.c | 12 ++++-------- >>>> 1 file changed, 4 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c >>>> index bca1ce7d0d0c..62e0cd5207a7 100644 >>>> --- a/drivers/platform/x86/intel/int3472/led.c >>>> +++ b/drivers/platform/x86/intel/int3472/led.c >>>> @@ -25,18 +25,14 @@ int skl_int3472_register_pled(struct int3472_discrete_device *int3472, >>>> if (int3472->pled.classdev.dev) >>>> return -EBUSY; >>>> >>>> - int3472->pled.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0], >>>> - "int3472,privacy-led"); >>>> + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( >>>> + int3472->dev, path, agpio->pin_table[0], >>>> + "int3472,privacy-led", polarity, >>>> + GPIOD_OUT_LOW); >>> >>> Yeah so this is not going to work, path here is an ACPI device path, e.g. >>> on my laptop (which actually uses the INT3472 glue code) the path-s of >>> the 2 GPIO controllers are: `\_SB_.GPI0` resp `\_SB_.PC00.XHCI.RHUB.HS08.VGPO` >>> >>> Where as skl_int3472_gpiod_get_from_temp_lookup() stores the passed in path >>> in gpiod_lookup_table.table[0].key, which is the dev_name() of the GPIO >>> controller's parent dev which are `INTC1055:00` resp. `INTC1096:00` . >>> >>> So we are going to need to add some code to INT3472 to go from path to >>> a correct value for gpiod_lookup_table.table[0].key which means partly >>> reproducing most of acpi_get_gpiod: >>> >>> struct gpio_chip *chip; >>> acpi_handle handle; >>> acpi_status status; >>> >>> status = acpi_get_handle(NULL, path, &handle); >>> if (ACPI_FAILURE(status)) >>> return ERR_PTR(-ENODEV); >>> >>> chip = gpiochip_find(handle, acpi_gpiochip_find); >>> if (!chip) >>> return ERR_PTR(-EPROBE_DEFER); >>> >>> And then get the key from the chip. Which means using gpiochip_find >>> in the int3472 code now, which does not sound like an improvement. >>> >>> I think that was is needed instead is adding an active_low flag >>> to acpi_get_and_request_gpiod() and then have that directly >>> set the active-low flag on the returned desc. >>> >> >> Ultimately I'd like everyone to use gpiod_get() for getting >> descriptors but for now I get it's enough. Are you find with this >> being done in a single patch across GPIO and this driver? > > Yes doing this in a single patch is fine. > > Also I'm fine with merging such a patch through the gpio tree . So thinking about this more I realized that the int3472 code already generates GPIO lookups for the sensor device for some (powerdown, reset) GPIOs, it only needs the gpio_desc for the case where the GPIO is turned into a regulator, clock or led. Since the int3472 code is already generating lookups it already has a way to go from path to a lookup "key": status = acpi_get_handle(NULL, path, &handle); if (ACPI_FAILURE(status)) return -EINVAL; adev = acpi_fetch_acpi_dev(handle); if (!adev) return -ENODEV; table_entry->key = acpi_dev_name(adev); So we can get the key without needing to call gpio_find_chip() So I do believe that the temp lookup approach should actually work. I'm currently traveling, so no promises but I should be able to rework your series in something which actually works and which will: 1. Stop using gpiod_toggle_active_low() 2. Allow dropping acpi_get_and_request_gpiod() So no need for a patch to add an active-low parameter to acpi_get_and_request_gpiod(), sorry about the noise. Regards, Hans >>>> if (IS_ERR(int3472->pled.gpio)) >>>> return dev_err_probe(int3472->dev, PTR_ERR(int3472->pled.gpio), >>>> "getting privacy LED GPIO\n"); >>>> >>>> - if (polarity == GPIO_ACTIVE_LOW) >>>> - gpiod_toggle_active_low(int3472->pled.gpio); >>>> - >>>> - /* Ensure the pin is in output mode and non-active state */ >>>> - gpiod_direction_output(int3472->pled.gpio, 0); >>>> - >>>> /* Generate the name, replacing the ':' in the ACPI devname with '_' */ >>>> snprintf(int3472->pled.name, sizeof(int3472->pled.name), >>>> "%s::privacy_led", acpi_dev_name(int3472->sensor)); >>> >>
diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c index bca1ce7d0d0c..62e0cd5207a7 100644 --- a/drivers/platform/x86/intel/int3472/led.c +++ b/drivers/platform/x86/intel/int3472/led.c @@ -25,18 +25,14 @@ int skl_int3472_register_pled(struct int3472_discrete_device *int3472, if (int3472->pled.classdev.dev) return -EBUSY; - int3472->pled.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0], - "int3472,privacy-led"); + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( + int3472->dev, path, agpio->pin_table[0], + "int3472,privacy-led", polarity, + GPIOD_OUT_LOW); if (IS_ERR(int3472->pled.gpio)) return dev_err_probe(int3472->dev, PTR_ERR(int3472->pled.gpio), "getting privacy LED GPIO\n"); - if (polarity == GPIO_ACTIVE_LOW) - gpiod_toggle_active_low(int3472->pled.gpio); - - /* Ensure the pin is in output mode and non-active state */ - gpiod_direction_output(int3472->pled.gpio, 0); - /* Generate the name, replacing the ':' in the ACPI devname with '_' */ snprintf(int3472->pled.name, sizeof(int3472->pled.name), "%s::privacy_led", acpi_dev_name(int3472->sensor));