From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 34D5FC2BD09 for ; Tue, 9 Jul 2024 04:56:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Cc:List-Subscribe: List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Type:MIME-Version:References:Message-ID:Subject:To:From:Date:Reply-To :Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=SqKaJocUioa9zVhmFj29t92/q5RUZD9GpRYJvaszsh0=; b=CPJlfwMpzP52+6uAyd0N/ZxQN2 gLa2VSnJ0tpRaakWpV+OV+lhOwekzoR8F+WVWGgVI0moazTqi2fPRDsEKC8azXW4NMMpIk+Kq14Bz s+JQDp6UAzZusQanS+LEo0pWD0SHXk/iMdsHBwG5bcyepLlMCd70k883NTVDXnsghFmdrVKVaTXze GFBdiahO+Nla74Ch+SlbhNyN2Dcd5iD4rnheWj2T/hpzOYUnwO/eMsuL1bV9W09DNLe506EFchLIi NewqJAqUpPj6JstizdmxrpdiUDcr1nz+6ma7huo4KcwmdTnio6J/iTyZwBSIctbMpEYZXSMUNiYNE MdcnsDJg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sR2tS-00000005tW8-44Bf; Tue, 09 Jul 2024 04:55:58 +0000 Received: from mail-pg1-x530.google.com ([2607:f8b0:4864:20::530]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sR2tD-00000005tSQ-0sqr for linux-arm-kernel@lists.infradead.org; Tue, 09 Jul 2024 04:55:44 +0000 Received: by mail-pg1-x530.google.com with SMTP id 41be03b00d2f7-77d3f00778cso460203a12.3 for ; Mon, 08 Jul 2024 21:55:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1720500942; x=1721105742; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=SqKaJocUioa9zVhmFj29t92/q5RUZD9GpRYJvaszsh0=; b=ewz4GytDVhLlWAbNKJp2+L/FZ+gKheqF8OWXsIp5NP3XDbRIEFk7Bs7eiF0IAq3BZ9 QspAAuTIdU82lDx3zzxpkB7vXfcuwKU+uyjCm/6GavXHN6DMD9Gu0T5RsKzQUZGM1qf6 kSvj4ga3Zr3YOqLP6aSxEJivnHldP5jJrMdVGPcX+iT0R1JF3BdtdQgm07CEY9DYpPb+ KrMm/R+8lu+pvNFyolKyQxiHWcFRPs+iMEdgN4Y3izBfi0ZOq3UMsG0yx2iBSGZp4RCE 0BqbU2GIHyIJae8/hRsXQyFmA6eqm+kRs4kMywG0EYYl2kH2a9Ge+aiQOKR45mWpRJ5w Khsg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1720500942; x=1721105742; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=SqKaJocUioa9zVhmFj29t92/q5RUZD9GpRYJvaszsh0=; b=Jj9bM90NvtGAejuIIk+OrdYX6a3ackMCJzr9ngMiz3WWgBBeU3gpx2QOS2HD9YDVA5 9u3xf6hBLCG+6s9L/W4zFd3uox7eTCtlekAGhol13di8gwW1kbgTPSO5e+bot+Ect0Cr 8z8s9oVIa34iPYPnGHA0KRbvhiJR3edX+sgL5QFMm16fAZKyzTvmjG0rATjjjlhbvo3w iiNlZwdE6UAgD64Cm2tEDKRGoGQmRKgOzsjkAiYAUVOuw/k7usBqookyZF51jO8Cv83P WfCW55bMusN9VZ7jOEb8iq4XpPUISEUhk9bitkY7o/PLoP1wpQDLjYLMd8vL6XYaEvQA GMgw== X-Forwarded-Encrypted: i=1; AJvYcCUmkMP6ZBG8ikbRwJhJXTNEB87a8UG/vmzgzvE434YnuTnJQP20JS97k4ZKqnM113mE06bCKT2v8R/uAd/Emcds8M4Xpez9ltaqnc0842QeEwxUmI0= X-Gm-Message-State: AOJu0YyW7an6SBqunMd0jF/jBrnBJuUUH4A31XG7Y3xNoLgoY9fn1Pfm PwNZNdW8a6pWqge6r5WmLA69cEgwon9NOHkWJA8ZLdZXL9ZNnFmj X-Google-Smtp-Source: AGHT+IHbadqVJQre10gCHyswTEmwsTSHIFV6clkSkz4h7Tp9Flx8bMAH8aAoU2KjYDqs5mnA6wfg4Q== X-Received: by 2002:a05:6a20:4f12:b0:1be:e265:81fa with SMTP id adf61e73a8af0-1c29824d472mr1014200637.35.1720500941419; Mon, 08 Jul 2024 21:55:41 -0700 (PDT) Received: from google.com ([2620:15c:9d:2:922a:af36:b3d9:2eac]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2ca34e89d46sm889698a91.27.2024.07.08.21.55.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Jul 2024 21:55:41 -0700 (PDT) Date: Mon, 8 Jul 2024 21:55:37 -0700 From: Dmitry Torokhov To: Stefan Eichenberger Subject: Re: [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions Message-ID: References: <20240417090527.15357-1-eichest@gmail.com> <20240417090527.15357-2-eichest@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240708_215543_281719_48A0E336 X-CRM114-Status: GOOD ( 31.01 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: robh@kernel.org, conor+dt@kernel.org, nick@shmanahar.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linus.walleij@linaro.org, linux-input@vger.kernel.org, alexandre.belloni@bootlin.com, Stefan Eichenberger , krzysztof.kozlowski+dt@linaro.org, claudiu.beznea@tuxon.dev, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Stefan, On Fri, Jun 21, 2024 at 04:38:25PM +0200, Stefan Eichenberger wrote: > Hi Dmitry > > On Thu, Jun 20, 2024 at 08:37:40AM -0700, Dmitry Torokhov wrote: > > Hi Stefan, > > > > On Wed, Apr 17, 2024 at 11:05:24AM +0200, Stefan Eichenberger wrote: > > > @@ -3374,8 +3390,7 @@ static void mxt_remove(struct i2c_client *client) > > > sysfs_remove_group(&client->dev.kobj, &mxt_attr_group); > > > mxt_free_input_device(data); > > > mxt_free_object_table(data); > > > - regulator_bulk_disable(ARRAY_SIZE(data->regulators), > > > - data->regulators); > > > + mxt_power_off(data); > > > > This change means that on unbind we will leave with GPIO line asserted. > > Won't this potentially cause some current leakage? Why do we need to > > have reset asserted here? > > This is correct, but I checked the datasheet of three different maxTouch > models and all of them have the reset line low active. This means we had > a current leakage before this patch. No, the reset line would be either set or pulled high, which is the normal state for the device. > Now it is fixed because we assert > the reset line, which sets the pin to 0. I also think it makes sense if > we look at the power on sequence. There we first power on the controller > before we release the reset line. Without asserting it on unbind this > would never trigger a reset after a power on. Please see that in mxt_probe() we do: /* Request the RESET line as asserted so we go into reset */ data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH); If the reset GPIO is annotated as "active low" (as it should unless there are inverters on the line) this will cause the line be driven to 0, putting the chip into the reset state. Then we enable regulators and deassert the reset GPIO with this bit of code: if (data->reset_gpio) { /* Wait a while and then de-assert the RESET GPIO line */ msleep(MXT_RESET_GPIO_TIME); gpiod_set_value(data->reset_gpio, 0); msleep(MXT_RESET_INVALID_CHG); } So the line here will be left at "1" state (logical off). It should stay this way until we need to go through the reset sequence again. I can see that you need the power on sequence to be executed again after probing is done. I recommend you make it something like this: static int mxt_power_on() { int error; if (data->reset_gpio) gpiod_set_value_cansleep(data->reset_gpio, 1); error = regulator_bulk_enable(ARRAY_SIZE(data->regulators), data->regulators); if (error) { dev_err(&client->dev, "failed to enable regulators: %d\n", error); return error; } /* * The device takes 40ms to come up after power-on according * to the mXT224 datasheet, page 13. */ msleep(MXT_BACKUP_TIME); if (data->reset_gpio) { /* Wait a while and then de-assert the RESET GPIO line */ msleep(MXT_RESET_GPIO_TIME); gpiod_set_value(data->reset_gpio, 0); msleep(MXT_RESET_INVALID_CHG); } return 0; } And then mxt_power_off() should only disable regulators, and leave the reset line alone. This way first time around the first "piod_set_value_cansleep(data->reset_gpio, 1)" will be effectively a noop, but on subsequent calls it will ensure that you have the transition inactive->active->inactive for the reset line. I wonder if we need both MXT_BACKUP_TIME and MXT_RESET_GPIO_TIME though... Thanks. -- Dmitry