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=-8.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,NICE_REPLY_A,SIGNED_OFF_BY,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 0F8DEC5517A for ; Wed, 11 Nov 2020 11:10:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A640A2074B for ; Wed, 11 Nov 2020 11:10:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="V7uZ4hL1" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726041AbgKKLKY (ORCPT ); Wed, 11 Nov 2020 06:10:24 -0500 Received: from us-smtp-delivery-124.mimecast.com ([63.128.21.124]:27089 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726176AbgKKLKQ (ORCPT ); Wed, 11 Nov 2020 06:10:16 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1605093015; 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=nB/8PtxCFjq/6L3Buun52o3IdFGex7Y7MyTLoEB5RKc=; b=V7uZ4hL17tBMbGlPGK5m9swIT5RwlMwGbR+CW04AwvySE1kM7+j4wPalOq7crgrK9u5MsL V7n73d10YLFUC0D+FjTQCDoDSFZ1lkWgLhGeL8L5+N7S3e6nNwV7XnhzvzWGnKz9i9oi3B kaVw2WZv9SOYovLIbAAYq7vmvVj47Dg= Received: from mail-ed1-f72.google.com (mail-ed1-f72.google.com [209.85.208.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-494-Wn8fVjEdOd2jb9F6jsVNVQ-1; Wed, 11 Nov 2020 06:10:13 -0500 X-MC-Unique: Wn8fVjEdOd2jb9F6jsVNVQ-1 Received: by mail-ed1-f72.google.com with SMTP id h6so712299edt.12 for ; Wed, 11 Nov 2020 03:10:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=nB/8PtxCFjq/6L3Buun52o3IdFGex7Y7MyTLoEB5RKc=; b=hInShNmhqSu/vcOrTE3pHLio3zTCFsezMHzN+2sd3ORzdtbbHlz1jO2GtuXwJR9Yr1 PgGUtpNOwJC4/ylccs33WZoOsDFG7f5rJXBJIjCfrzVbDDQaJ1bRKrT8hMJwTTFIsQvf by1yUliS4fJU5JevoDfKoOyG4vE1vr3wW6A7MvVnx12EIs/cXx7zJXiW73jdvK13k9mj tIhiON4XV9FP9VvqwZuygmQIghSCOLrNyZsc+8wlsvNCe4MeDpunGFwxuHgRaN96Wkf5 0XorlaIc/kwYaFK1hItMlJa+Zpt4dsCILRUX7whj9EKbl81gIDcVGXwiZThFY+9D/mGO Ag5Q== X-Gm-Message-State: AOAM5329LUixpIqrrT0j59ZvWQQW5Kd66ffsBkV7Q7IIusF8RP7+qL6t B1mnpCjOvdOS6YYDU4vSogcoUygRZKhDvIKmqIhtWIrUhnM/9m654pBSxoJD/KAdxBBZScPXABV 3kxXqWIGA3GlbZVgtaJq4i2vzpANFhkyGa6+UfR+h5BIuyZPwxP6FjdODSZZl3N+WXQbLj6KIQy nM X-Received: by 2002:a17:906:c18c:: with SMTP id g12mr24338206ejz.334.1605093011953; Wed, 11 Nov 2020 03:10:11 -0800 (PST) X-Google-Smtp-Source: ABdhPJwBdq5GZK41HrhsHP4TdbrW/eQUWPlUxyNamO2Sw75ZgKxmUhBCLFdiLJa4T5gVT6eTucPACg== X-Received: by 2002:a17:906:c18c:: with SMTP id g12mr24338138ejz.334.1605093011368; Wed, 11 Nov 2020 03:10:11 -0800 (PST) Received: from x1.localdomain (2001-1c00-0c0c-fe00-6c10-fbf3-14c4-884c.cable.dynamic.v6.ziggo.nl. [2001:1c00:c0c:fe00:6c10:fbf3:14c4:884c]) by smtp.gmail.com with ESMTPSA id d23sm744805edy.57.2020.11.11.03.10.10 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 11 Nov 2020 03:10:10 -0800 (PST) Subject: Re: [PATCH v5 1/4] HID: i2c-hid: Reorganize so ACPI and OF are separate modules To: Dmitry Torokhov , Doug Anderson Cc: Jiri Kosina , Benjamin Tissoires , Greg Kroah-Hartman , Kai-Heng Feng , Rob Herring , "open list:HID CORE LAYER" , Stephen Boyd , Andrea Borgia , Jiri Kosina , Masahiro Yamada , Pavel Balan , Xiaofei Tan , You-Sheng Yang , LKML References: <20201109213636.1267536-1-dianders@chromium.org> <20201109133526.v5.1.Ied4ce10d229cd7c69abf13a0361ba0b8d82eb9c4@changeid> <20201111000458.GW1003057@dtor-ws> From: Hans de Goede Message-ID: <52296c00-07ae-29ad-6e93-0e80430da3e9@redhat.com> Date: Wed, 11 Nov 2020 12:10:09 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 MIME-Version: 1.0 In-Reply-To: <20201111000458.GW1003057@dtor-ws> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 11/11/20 1:04 AM, Dmitry Torokhov wrote: > On Tue, Nov 10, 2020 at 02:17:27PM -0800, Doug Anderson wrote: >> Hi, >> >> On Tue, Nov 10, 2020 at 1:01 AM Hans de Goede wrote: >>> >>> Hi, >>> >>> On 11/9/20 10:36 PM, Douglas Anderson wrote: >>>> This patch rejiggers the i2c-hid code so that the OF (Open Firmware >>>> aka Device Tree) and ACPI support is separated out a bit. The OF and >>>> ACPI drivers are now separate modules that wrap the core module. >>>> >>>> Essentially, what we're doing here: >>>> * Make "power up" and "power down" a function that can be (optionally) >>>> implemented by a given user of the i2c-hid core. >>>> * The OF and ACPI modules are drivers on their own, so they implement >>>> probe / remove / suspend / resume / shutdown. The core code >>>> provides implementations that OF and ACPI can call into. >>>> >>>> We'll organize this so that we now have 3 modules: the old i2c-hid >>>> module becomes the "core" module and two new modules will depend on >>>> it, handling probing the specific device. >>>> >>>> As part of this work, we'll remove the i2c-hid "platform data" >>>> concept since it's not needed. >>>> >>>> Signed-off-by: Douglas Anderson >>>> --- >>>> >>>> Changes in v5: >>>> - Add shutdown_tail op and use it in ACPI. >>>> - i2chid_subclass_data => i2chid_ops. >>>> - power_up_device => power_up (same with power_down). >>>> - subclass => ops. >>>> >>> >>> Thanks this looks good to now, 2 small remarks below (since you are >>> going to do a v6 anyways). Feel free to ignore these remarks if >>> you prefer to keep things as is. >>> >>> And feel free to add my reviewed-by to v6 of this patch: >>> >>> Reviewed-by: Hans de Goede >> >> Thanks! >> >> >>>> +static const struct i2c_device_id i2c_hid_acpi_id_table[] = { >>>> + { "hid", 0 }, >>>> + { "hid-over-i2c", 0 }, >>>> + { }, >>>> +}; >>>> +MODULE_DEVICE_TABLE(i2c, i2c_hid_acpi_id_table); >>> >>> Hmm, I do not think these old-style i2c-ids are necessarry at >>> all in this driver. I expect all use-cases to use either >>> of or acpi matches. >>> >>> This was already present in the code before though, so >>> please ignore this remark. This is just something which >>> I noticed and thought was worth while pointing out as >>> a future cleanup. >> >> Yeah, I wasn't sure if there was anyone using them. >> >> Hrm. Thinking about it, though, is it really OK for two drivers to >> both have the same table listed? I'm not sure how that would work. >> Do you know? >> >> I don't know a ton about ACPI, but for device tree I know i2c has a >> fallback mode. Specifically having this table means that we'll match >> compatible strings such as: >> >> "zipzapzing,hid" >> "kapowzers,hid-over-i2c" >> >> In other words it'll ignore the vendor part and just match on the >> second half. Just to make sure I wasn't remembering that from a dream >> I tried it and it worked. I don't see any mainline device trees that >> look like that, though. I could delete it, though it doesn't really >> take up much space and it seems nice to keep it working in case anyone >> was relying on it? >> >> For ACPI is there a similar fallback? If not then it seems like it'd >> be easy to remove it from there... > > Just a random thought - will all this still be working with ACPI PRP0001 > and DT-style compatible string and properties in _DSD? That should keep working. Unless someone mixes a DT-style compatible string with the PNP0C50 ACPI HID specific DSM for getting the hid_descriptor_address (instead of a DT style property). But that would be a really weird mix to use and obviously would go against both the PNP0C50 and the PRP0001 specs. Regards, Hans