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 X-Spam-Level: X-Spam-Status: No, score=-10.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D85A4C433E0 for ; Fri, 15 Jan 2021 15:01:08 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0C2AD23603 for ; Fri, 15 Jan 2021 15:01:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0C2AD23603 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:References: To:Subject:From:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=zRum+DEy+l00omCr65ge3fPEaoBxKO/OpWFH/DG8Qps=; b=WMtG4+7QwBabGd6Yc7/JqxBxn St0r12nqI84CcFFWnEVwZ8kXgPqshZ99fOiTGLs3vRhZCqFShJgNhA4u20geiuVdVm+y7RNHmze+k F+uisn/kXyrERtMBiNfz+cQWdsO8c5r1JM8ZRa/JuLXZkeoou5Uje6pYIDz8LITKRckAvpvSnj3/r HLP7hQWrQEIfT0OeWsmzkk455dyarsdmm18yRz0Txya5FVzZ1YxSG8vDCB7cf3rJLZZqHA/RHAmEB jmNSKdwGtnJtptH3I3P9n346ek6u/FMHBgsiis0YS/7X6+Cd0xAqExmLMLs/OLymLmJmyHpCfe3no d4FiNbFqw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l0QYa-0000fU-Oo; Fri, 15 Jan 2021 14:58:32 +0000 Received: from us-smtp-delivery-124.mimecast.com ([63.128.21.124]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1l0QYW-0000ei-9P for linux-arm-kernel@lists.infradead.org; Fri, 15 Jan 2021 14:58:30 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610722707; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=dmVQjQTpqOnD4r9o3Q6+/fS9ZXoOCu6z+q20+s/vvWQ=; b=QH75kkNUjnFtmHIIAnk1g5yTdMJo0KL0iIzcAuux4a77OpfG8gGALAvTJeogWJxk77+1LD ZtQ5oSEyrbE9XSsjxVZP5rNEPuyibBv/BMPGdZjmGkKk6Wxgj0sXfo5FLVC5bh+ft/lqMw qhwKXS+fpj342daF/MzD0fZLwA3MkM8= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-396-xUb4Tnn0Ms6MG3tPaPc6xQ-1; Fri, 15 Jan 2021 09:58:25 -0500 X-MC-Unique: xUb4Tnn0Ms6MG3tPaPc6xQ-1 Received: by mail-wr1-f71.google.com with SMTP id z8so4280763wrh.5 for ; Fri, 15 Jan 2021 06:58:25 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=dmVQjQTpqOnD4r9o3Q6+/fS9ZXoOCu6z+q20+s/vvWQ=; b=Yl0iTVJ5ZOUMi0vcoo9nctii1MbjpJ63FGBTzQZbUqeJGBVYYAw79/oCvVWslWH10Q j7fSK/VXj0AX6KYYZ/hwoolghG6ic6+qypBu0pF5lSVoJgBHscaHHxyMODBMXm2/qgsb evhwvcGWHFcFDq6l5wo9oLQQNoyHnDv49bCPrSdUt1Iyxrk4dv5rtjl0NNFYEJGoSs6J YZ4TRZW+59v6ZxlA1dlFJ0/w3cmy0o/f/E4YXZhkIku1P49+7hlLDwm2G1CNrpeccz4Y lvpIVddQr0LLJ+vSJBnkb6WByf4CAGnJSOYeyP72kk3wA6li5n2PZdbDWJmQJ2nTje5n uSNQ== X-Gm-Message-State: AOAM5337jsUKnATyTnmXiVkZprndeQ2Eg0p6JkMttUfMrZn8c/RiYUKm 9/m1E35n8jxm70g94ae4qkYGuQvxhlWLw2ZY/GCXY1/vmfSVe29/MJseXBgXj9ALB7yWhzzINte 8MHH+yRzJn6vB14AEMm0xQYbbB77Xr7ANaWc= X-Received: by 2002:a1c:e255:: with SMTP id z82mr1118477wmg.60.1610722704149; Fri, 15 Jan 2021 06:58:24 -0800 (PST) X-Google-Smtp-Source: ABdhPJwW+XSWfmPhbHhzTUqZ64FJxtNe1z+L1rkyzhGjXUijdF1PlSioAAyUWeDkY39cnqvjrYChFw== X-Received: by 2002:a1c:e255:: with SMTP id z82mr1118429wmg.60.1610722703896; Fri, 15 Jan 2021 06:58:23 -0800 (PST) Received: from ?IPv6:2a01:e34:eea6:7961::e71? ([2a01:e34:eea6:7961::e71]) by smtp.gmail.com with ESMTPSA id f14sm15410402wre.69.2021.01.15.06.58.21 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 15 Jan 2021 06:58:23 -0800 (PST) From: Benjamin Tissoires Subject: Re: [PATCH v8 0/4] HID: i2c-hid: Reorganize to allow supporting goodix,gt7375p To: Doug Anderson References: <20201211222448.2115188-1-dianders@chromium.org> Message-ID: Date: Fri, 15 Jan 2021 15:58:20 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.1 MIME-Version: 1.0 In-Reply-To: Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=benjamin.tissoires@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210115_095828_377471_65455655 X-CRM114-Status: GOOD ( 47.71 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Geert Uytterhoeven , Catalin Marinas , =?UTF-8?Q?Guido_G=c3=bcnther?= , Bjorn Andersson , Kai-Heng Feng , Will Deacon , Anson Huang , Masahiro Yamada , Vinod Koul , Linux ARM , "open list:HID CORE LAYER" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Jiri Kosina , Stephen Boyd , Hans de Goede , Rob Herring , Daniel Playfair Cal , Andrea Borgia , Max Krummenacher , Greg Kroah-Hartman , Dmitry Torokhov , LKML , Li Yang , Michael Walle , Xiaofei Tan , Jiri Kosina , Shawn Guo , Pavel Balan Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, On Wed, Jan 13, 2021 at 8:35 PM Benjamin Tissoires wrote: > > On Wed, Jan 13, 2021 at 5:05 PM Doug Anderson wrote: > > > > Hi, > > > > On Wed, Jan 13, 2021 at 7:09 AM Benjamin Tissoires > > wrote: > > > > > > > I wanted to apply the series yesterday, but for these kinds of changes > > > > I like giving it a spin on actual hardware. Turns out that my XPS-13 > > > > can not boot to v5.11-rc2, which makes testing the new branch slightly > > > > more difficult. > > > > > > > > I'll give it a spin next week, but I think I should be able to land it for 5.12. > > > > > > > > Regarding the defconfig conflict, no worries, we can handle it with > > > > Stephen and Linus. > > > > > > > > > > After 2 full kernel bisects (I messed up the first because I am an > > > idiot and inverted good and bad after the first reboot), I found my > > > culprit, and I was able to test the series today. > > > > > > The series works fine regarding enumeration and removing of devices, > > > but it prevents my system from being suspended. If I rmmod > > > i2c-hid-acpi, suspend works fine, but if it is present, it immediately > > > comes back, which makes me think that something must be wrong. > > > > > > I also just reverted the series and confirmed that suspend/resume now > > > works, meaning that patch 1/4 needs to be checked. > > > > Can you give me any hints about what type of failure you're seeing? > > Any logs? I don't have an ACPI system to test with... > > I don't have any logs, just that the system comes back up. There is a > chance we are not powering the device down correctly, which triggers > an IRQ and which puts the system back on. > > > > > Is there any chance that some type of userspace / udev rule is getting > > tripped up by the driver being renamed? We ran into something like > > this recently on Chrome OS where we had a tool that was hardcoded to > > look for "i2c-hid" and needed to be adapted to account for the new > > driver name. Often userspace tweaks with wakeup rules based on driver > > name... > > I don't think there is anything like that on a regular desktop. > > > > > I'll go stare at the code now and see if anything jumps out. > > > > Thanks, but don't spend too much time on it, unless something really > jumps out. I'll debug that tomorrow. It's much easier with an actual > device than by just looking at the code. > Well, that's weird. Now suspend resume works reliably even with your series. It could just have been that the lid sensor was too close to a magnet or something like that. Though while testing the old version of i2c-hid, it was working... Such a mystery :) Anyway, while trying to dig up that now-non-issue, I got the following patch that you likely want to squash into 1/4: --- diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c index 0f86060f01b4..dd6d9f74e7e7 100644 --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c @@ -31,7 +31,6 @@ struct i2c_hid_acpi { struct i2chid_ops ops; struct i2c_client *client; - bool power_fixed; }; static const struct acpi_device_id i2c_hid_acpi_blacklist[] = { @@ -75,25 +74,6 @@ static int i2c_hid_acpi_get_descriptor(struct i2c_client *client) return hid_descriptor_address; } -static int i2c_hid_acpi_power_up(struct i2chid_ops *ops) -{ - struct i2c_hid_acpi *ihid_of = - container_of(ops, struct i2c_hid_acpi, ops); - struct device *dev = &ihid_of->client->dev; - struct acpi_device *adev; - - /* Only need to call acpi_device_fix_up_power() the first time */ - if (ihid_of->power_fixed) - return 0; - ihid_of->power_fixed = true; - - adev = ACPI_COMPANION(dev); - if (adev) - acpi_device_fix_up_power(adev); - - return 0; -} - static void i2c_hid_acpi_shutdown_tail(struct i2chid_ops *ops) { struct i2c_hid_acpi *ihid_of = @@ -107,6 +87,7 @@ static int i2c_hid_acpi_probe(struct i2c_client *client, { struct device *dev = &client->dev; struct i2c_hid_acpi *ihid_acpi; + struct acpi_device *adev; u16 hid_descriptor_address; int ret; @@ -115,7 +96,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client, return -ENOMEM; ihid_acpi->client = client; - ihid_acpi->ops.power_up = i2c_hid_acpi_power_up; ihid_acpi->ops.shutdown_tail = i2c_hid_acpi_shutdown_tail; ret = i2c_hid_acpi_get_descriptor(client); @@ -123,6 +103,10 @@ static int i2c_hid_acpi_probe(struct i2c_client *client, return ret; hid_descriptor_address = ret; + adev = ACPI_COMPANION(dev); + if (adev) + acpi_device_fix_up_power(adev); + if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) { device_set_wakeup_capable(dev, true); device_set_wakeup_enable(dev, false); --- This allows to keep the powering ordering of the old i2c-hid module (power up before setting device wakeup capable), and simplify the not so obvious power_fixed field of struct i2c_hid_acpi. (I can also send it as a followup on the series if you prefer). Cheers, Benjamin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel