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 C2D5FCD11C2 for ; Fri, 5 Apr 2024 08:16:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=D6aw1808Tsvx6ZaY6VhHuud7GuUWuMKX0V3ImrkOckI=; b=UYC3mVAtqBV+kq VhnNDGP+hQOtp7a2rqmJ/FGC189TAcOxEZPcpMPOON6qucgXZBOjTviKhYMGe3yigMKkok6GnLqvf UnmPQML3Vc7y2X+UL/+7oz4EhSSYg0s+fm9GFtVBJWQDiApw2G3/tq+OXhJIYSpbrA+Wegk7Ljp87 1h+IquLz0PFcxj+KverwkYZ9xMoX+XqD3YNC5Mge7g7LbtWmXL3G8u4HDK9Sm1WhGeO6M8dyhW147 Xb7AQOVmgphC/g2PWuN+iiAOLzEHtShNbhhTW6ODfB10XlUU5+4Q826OKX/QbpVwciG/moIbOzBlA LJQyhRIH17q5e5ht82hw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rsejy-00000005tGL-1zNZ; Fri, 05 Apr 2024 08:16:02 +0000 Received: from mail-wm1-x334.google.com ([2a00:1450:4864:20::334]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rsejv-00000005tFB-3Bna for linux-arm-kernel@lists.infradead.org; Fri, 05 Apr 2024 08:16:01 +0000 Received: by mail-wm1-x334.google.com with SMTP id 5b1f17b1804b1-4163181a7ceso1186815e9.0 for ; Fri, 05 Apr 2024 01:15:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1712304958; x=1712909758; 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=l0EQOfaQpw4diSaNUla+Yt9ZI58t4pKoAeHS1ES6kHY=; b=ebZNIjflLT+u+S60/f55FhrbE77okMM9eNYf+ampQixfYIqE2yBVsZXykIyZsUMH6Z hoNMb4uRcEXu8k+vG1bLMvsP6sJaXzqTM8uQ6oRplxs4lYFo3gCwUjHX3h9/f4i2mhmV x0eZCdwAhwKQV7WkT4IV19jFZdswkeaxiHLwC1reCnvurN2ZrJfEnGJPfh7AEUHFpL6d KORpAY5BDjcVA4K55OOQ8YKbFh8R+5BTAcUkmZvgKzseaX67DVoDBv1dqST7Iyx36xpH EsRt7Mu8A59v6bUbwQtpX1Xt0KuB4vM9U8qv3ex35deC+Fg6ap1SBg7uF3tfYu/JMC5n GGrA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712304958; x=1712909758; 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=l0EQOfaQpw4diSaNUla+Yt9ZI58t4pKoAeHS1ES6kHY=; b=NhFYdUuzH/qwLCISeO5OZkInZ/PAAU5RpbCgTszpF0d8AMzPzefrbrOtm6kdGB5+u7 223QaG9W0PrUru5CqUZc8KBhkWX1U5yvYtlnagIUkeOzZm2zKq4bYNsCAgxeBlL2nC5E RFM0cbFuBuVuFIFD5FaojYT4Pn/He9kGWFyFtWnkr/bN7H/sM/VdiStkUIOrgNe2FRCZ T9iqWVkSSFmm0rFqkfXCnZJ5Gs5j6CuswP3zNnJV1UamiFbpkemjnRLK2A5GAlw42UaC J+LeY98uiqlbKFge1T2zHyD7gwShpYnf8p5HbAu3XC1oBQsWB12lumB+Do4iDM9+imkC oe2A== X-Forwarded-Encrypted: i=1; AJvYcCVg738nSRohgemgf922qwD6s2Q5gCYR8eb0qtZmWTOd7jmrbN0JI/ZZTqJp7tUoCQyaSOG851gIQ5EPkO7FJZwCJ1zv8Y/sFJ5JAM/jOd225Co9IAA= X-Gm-Message-State: AOJu0YxfhH8u4H9VFfBe9ANOgYfQx9G1coLAvO1bGfkYjZdg+m3StLBG 50EBwQlqiwFXUjdootRGnEtjcYUSFM+G6I78QYwveA2jWhdCOIPM X-Google-Smtp-Source: AGHT+IFJF07eybUNJc74Q+2ul5zRE6d3CbQ2mzRAhDZAWzlZk3TA66G2QcsHDG0GgoZ4BiX6wiGX1Q== X-Received: by 2002:a05:600c:5112:b0:413:28ba:1b17 with SMTP id o18-20020a05600c511200b0041328ba1b17mr619272wms.6.1712304957561; Fri, 05 Apr 2024 01:15:57 -0700 (PDT) Received: from eichest-laptop ([2a02:168:af72:0:727e:ef62:9d19:ed24]) by smtp.gmail.com with ESMTPSA id y10-20020a1c4b0a000000b004162061c704sm5377133wma.29.2024.04.05.01.15.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 05 Apr 2024 01:15:56 -0700 (PDT) Date: Fri, 5 Apr 2024 10:15:55 +0200 From: Stefan Eichenberger To: Dmitry Torokhov Subject: Re: [PATCH RESEND v3 2/2] Input: atmel_mxt_ts - support poweroff in suspend Message-ID: References: <20240209105012.22470-1-eichest@gmail.com> <20240209105012.22470-3-eichest@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240405_011559_847190_32C231DD X-CRM114-Status: GOOD ( 36.29 ) 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: devicetree@vger.kernel.org, alexandre.belloni@bootlin.com, nick@shmanahar.org, linux-kernel@vger.kernel.org, linus.walleij@linaro.org, linux-input@vger.kernel.org, conor+dt@kernel.org, Stefan Eichenberger , robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, claudiu.beznea@tuxon.dev, francesco.dolcini@toradex.com, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Dmitry, Thanks for the feedback, I had a first look at the changes and I'm not sure if we would break some use cases. Therfore, here some questions. On Fri, Mar 15, 2024 at 11:51:19AM -0700, Dmitry Torokhov wrote: > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > > index 542a31448c8f..2d5655385702 100644 > > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > > @@ -317,6 +317,7 @@ struct mxt_data { > > struct gpio_desc *reset_gpio; > > struct gpio_desc *wake_gpio; > > bool use_retrigen_workaround; > > + bool poweroff_sleep; > > > > /* Cached parameters from object table */ > > u16 T5_address; > > @@ -2799,15 +2800,18 @@ static int mxt_configure_objects(struct mxt_data *data, > > dev_warn(dev, "Error %d updating config\n", error); > > } > > > > - if (data->multitouch) { > > - error = mxt_initialize_input_device(data); > > - if (error) > > - return error; > > - } else { > > - dev_warn(dev, "No touch object detected\n"); > > - } > > + /* If input device is not already registered */ > > + if (!data->input_dev) { > > + if (data->multitouch) { > > + error = mxt_initialize_input_device(data); > > + if (error) > > + return error; > > + } else { > > + dev_warn(dev, "No touch object detected\n"); > > + } > > > > - mxt_debug_init(data); > > + mxt_debug_init(data); > > + } > > > > return 0; > > } > > @@ -3325,6 +3329,8 @@ static int mxt_probe(struct i2c_client *client) > > msleep(MXT_RESET_INVALID_CHG); > > } > > > > + data->poweroff_sleep = device_property_read_bool(&client->dev, > > + "atmel,poweroff-sleep"); > > /* > > * Controllers like mXT1386 have a dedicated WAKE line that could be > > * connected to a GPIO or to I2C SCL pin, or permanently asserted low. > > @@ -3387,12 +3393,21 @@ static int mxt_suspend(struct device *dev) > > if (!input_dev) > > return 0; > > > > - mutex_lock(&input_dev->mutex); > > + if (!device_may_wakeup(dev) && data->poweroff_sleep) { > > + if (data->reset_gpio) > > + gpiod_set_value(data->reset_gpio, 1); > > > > - if (input_device_enabled(input_dev)) > > - mxt_stop(data); > > + regulator_bulk_disable(ARRAY_SIZE(data->regulators), > > + data->regulators); > > + data->T44_address = 0; > > + } else { > > + mutex_lock(&input_dev->mutex); > > + > > + if (input_device_enabled(input_dev)) > > + mxt_stop(data); > > > > - mutex_unlock(&input_dev->mutex); > > + mutex_unlock(&input_dev->mutex); > > + } > > This all should go into mxt_stop(), so that if device is closed, or > inhibited, you power it down as well (if you can). We would then have to power it up during probe to see if the device is threre, read the configuration and power it down again afterwards until the device is opened. If the device is in bootloader mode we would most likely have to keep the power on all the time and never turn it off, right? > > > > > disable_irq(data->irq); > > > > @@ -3408,14 +3423,37 @@ static int mxt_resume(struct device *dev) > > if (!input_dev) > > return 0; > > > > - enable_irq(data->irq); > > + if (!device_may_wakeup(dev) && data->poweroff_sleep) { > > + int ret; > > > > - mutex_lock(&input_dev->mutex); > > + ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators), > > + data->regulators); > > + if (ret) { > > + dev_err(dev, "failed to enable regulators: %d\n", > > + ret); > > + return ret; > > + } > > + msleep(MXT_BACKUP_TIME); > > > > - if (input_device_enabled(input_dev)) > > - mxt_start(data); > > + 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); > > + } > > > > - mutex_unlock(&input_dev->mutex); > > + /* This also enables the irq again */ > > + mxt_initialize(data); > > And this needs to go into mxt_start(). Also, we should make sure that > once resume operation completes the device is fully operational. That > means you should not request the firmware asynchronously in > mxt_initialize() in case you are in the resume path. I think you should > also unwind mxt_initialize() and mxt_configure_objects() to make it > clear what is the part of initial initialization and what is part of > re-initializing during resume. The configuration that is exposed to > userspace (resolution, number of objects, other properties) should stay > the same, the configuration of the chip itself (power mode, etc) should > be restored. Here we would most likely have to load the firmware (configuration) synchronously all the time if the poweroff_sleep flag is set. Ths makes sure that the device is ready when we open the device. Would this delay be acceptable when opening the input device? Normally the configuration is not that big and should load quite fast. Regards, Stefan _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel