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=-11.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,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 331E5C4741F for ; Wed, 4 Nov 2020 12:07:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CEB4C21734 for ; Wed, 4 Nov 2020 12:07:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="RNcnFReA" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729658AbgKDMHw (ORCPT ); Wed, 4 Nov 2020 07:07:52 -0500 Received: from us-smtp-delivery-124.mimecast.com ([63.128.21.124]:36254 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729520AbgKDMHv (ORCPT ); Wed, 4 Nov 2020 07:07:51 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1604491668; 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=MYIqumr/TUod3fxLpneCnXJl7Cwbmb61D8YOXZY/RJM=; b=RNcnFReAwhv1qa69CwYIRtv1rDSyc1TZgIpz2BTO4BeIKrzG1raIXbYhyaixwZ4kkSCGxM ocXizzoQsVDFrnsDx5FdPIvKfbVY7AsPuskv3kt45FpwIHG9GSwWKszbujA8A96XdcaO2X lFbeQLqGqqB/iT+HMVtHKvdMalDje9c= Received: from mail-ej1-f70.google.com (mail-ej1-f70.google.com [209.85.218.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-548-KjoXZaHtPKOzVX0JXJoRsg-1; Wed, 04 Nov 2020 07:07:46 -0500 X-MC-Unique: KjoXZaHtPKOzVX0JXJoRsg-1 Received: by mail-ej1-f70.google.com with SMTP id z25so6691806ejd.2 for ; Wed, 04 Nov 2020 04:07:46 -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=MYIqumr/TUod3fxLpneCnXJl7Cwbmb61D8YOXZY/RJM=; b=T8RM9sinAse8/DlDnZ3U6qyjpDn3OwaGpWsxvSqDZMWgNokrHqVR9pyPafcFLllvaN 40GhBDltP/Nb10+xYuLNZfJDOaBYQASPMtfeA60qZUGjnN0M4tF/vUudcxGyU3zgvmeV /D8/2R0ORc47NYzqYQrTAIzhAhEsqOlNUB+k7SRkxF9p/bp5RcAMRdCH78oZRL/Z7mYH yMzrn+nVj46Gw/7Q1n93IYngCocrrNYCvpTaVU1JJUO8fIgSW7v5gDjKoIkGYbeQL1kP VntuCYhF/zZjCXNPQ706dR0197N/+UfFgHyr4abCcPWQ7MG1SDSlZSTD9ddU6xan19YM 9w8Q== X-Gm-Message-State: AOAM530tBP21cl9GplWIx2fFZL1PRWtIEn0ev0o4vO1Raw1du7ifl1O5 lRwZjukQg69SAMHs/WU0XslakPxgP6M5GElzL+AiwH83sMVoaDZDZWx34pEQUYjfT/LA/nAOaM+ 20fZnxJ5b0S17AbL/gtuuU+lE X-Received: by 2002:aa7:db48:: with SMTP id n8mr18916843edt.123.1604491664651; Wed, 04 Nov 2020 04:07:44 -0800 (PST) X-Google-Smtp-Source: ABdhPJxDSqmWCrNhXIjT7YreEx+UuN0J+Iy2gOhwLgXQczJahJB+s3hPG94XQqz2qtavzf5PwCbkRQ== X-Received: by 2002:aa7:db48:: with SMTP id n8mr18916814edt.123.1604491664335; Wed, 04 Nov 2020 04:07:44 -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 bx24sm894687ejb.51.2020.11.04.04.07.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 04 Nov 2020 04:07:43 -0800 (PST) Subject: Re: [PATCH v4 1/4] HID: i2c-hid: Reorganize so ACPI and OF are subclasses To: Douglas Anderson , jkosina@suse.cz, benjamin.tissoires@redhat.com, gregkh@linuxfoundation.org, Dmitry Torokhov Cc: linux-input@vger.kernel.org, swboyd@chromium.org, andrea@borgia.bo.it, kai.heng.feng@canonical.com, robh+dt@kernel.org, Aaron Ma , Jiri Kosina , Masahiro Yamada , Pavel Balan , Xiaofei Tan , You-Sheng Yang , linux-kernel@vger.kernel.org References: <20201104012929.3850691-1-dianders@chromium.org> <20201103172824.v4.1.Ied4ce10d229cd7c69abf13a0361ba0b8d82eb9c4@changeid> From: Hans de Goede Message-ID: Date: Wed, 4 Nov 2020 13:07:42 +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: <20201103172824.v4.1.Ied4ce10d229cd7c69abf13a0361ba0b8d82eb9c4@changeid> 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/4/20 2:29 AM, Douglas Anderson wrote: > This patch rejiggers the i2c-hid code so that the OF (Device Tree) and > ACPI support is separated out a bit. The OF and ACPI drivers are now > effectively "subclasses" of the generic code. > > Essentially, what we're doing here: > * Make "power up" and "power down" a virtual function that can be > (optionally) implemented by subclasses. > * Each subclass is a drive on its own, so it implements probe / remove > / suspend / resume / shutdown. The core code provides > implementations that the subclasses can call into, or fully use for > their implementation if they don't have special needs. > > 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 Thank you for doing this, overall this looks pretty good to me. Some small comments inline, not a full review. > diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c > new file mode 100644 > index 000000000000..d4c29dc62297 > --- /dev/null > +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c > @@ -0,0 +1,167 @@ > +/* > + * HID over I2C ACPI Subclass > + * > + * Copyright (c) 2012 Benjamin Tissoires > + * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France > + * Copyright (c) 2012 Red Hat, Inc > + * > + * This code was forked out of the core code, which was partly based on > + * "USB HID support for Linux": > + * > + * Copyright (c) 1999 Andreas Gal > + * Copyright (c) 2000-2005 Vojtech Pavlik > + * Copyright (c) 2005 Michael Haboustak for Concept2, Inc > + * Copyright (c) 2007-2008 Oliver Neukum > + * Copyright (c) 2006-2010 Jiri Kosina > + * > + * This file is subject to the terms and conditions of the GNU General Public > + * License. See the file COPYING in the main directory of this archive for > + * more details. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "i2c-hid.h" > + > +struct i2c_hid_acpi { > + struct i2chid_subclass_data subclass; This feels a bit weird, we are the subclass so typically we would be embedding a base_class data struct here ... (more remarks below, note just my 2 cents you may want to wait for feedback from others). > + struct i2c_client *client; You pass this to i2c_hid_core_probe which then stores it own copy, why not just store it in the subclass (or even better baseclass) data struct ? > + bool power_fixed; > +}; > + > +static const struct acpi_device_id i2c_hid_acpi_blacklist[] = { > + /* > + * The CHPN0001 ACPI device, which is used to describe the Chipone > + * ICN8505 controller, has a _CID of PNP0C50 but is not HID compatible. > + */ > + {"CHPN0001", 0 }, > + { }, > +}; > + > +static int i2c_hid_acpi_get_descriptor(struct i2c_client *client) > +{ > + static guid_t i2c_hid_guid = > + GUID_INIT(0x3CDFF6F7, 0x4267, 0x4555, > + 0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE); > + union acpi_object *obj; > + struct acpi_device *adev; > + acpi_handle handle; > + u16 hid_descriptor_address; > + > + handle = ACPI_HANDLE(&client->dev); > + if (!handle || acpi_bus_get_device(handle, &adev)) { > + dev_err(&client->dev, "Error could not get ACPI device\n"); > + return -ENODEV; > + } > + > + if (acpi_match_device_ids(adev, i2c_hid_acpi_blacklist) == 0) > + return -ENODEV; > + > + obj = acpi_evaluate_dsm_typed(handle, &i2c_hid_guid, 1, 1, NULL, > + ACPI_TYPE_INTEGER); > + if (!obj) { > + dev_err(&client->dev, "Error _DSM call to get HID descriptor address failed\n"); > + return -ENODEV; > + } > + > + hid_descriptor_address = obj->integer.value; > + ACPI_FREE(obj); > + > + return hid_descriptor_address; > +} > + > +static int i2c_hid_acpi_power_up_device(struct i2chid_subclass_data *subclass) > +{ > + struct i2c_hid_acpi *ihid_of = > + container_of(subclass, struct i2c_hid_acpi, subclass); > + 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; > +} > + > +int i2c_hid_acpi_probe(struct i2c_client *client, > + const struct i2c_device_id *dev_id) > +{ > + struct device *dev = &client->dev; > + struct i2c_hid_acpi *ihid_acpi; > + u16 hid_descriptor_address; > + int ret; > + > + ihid_acpi = devm_kzalloc(&client->dev, sizeof(*ihid_acpi), GFP_KERNEL); > + if (!ihid_acpi) > + return -ENOMEM; > + > + ihid_acpi->subclass.power_up_device = i2c_hid_acpi_power_up_device; > + > + ret = i2c_hid_acpi_get_descriptor(client); > + if (ret < 0) > + return ret; > + hid_descriptor_address = ret; > + > + if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) { > + device_set_wakeup_capable(dev, true); > + device_set_wakeup_enable(dev, false); > + } > + > + return i2c_hid_core_probe(client, &ihid_acpi->subclass, > + hid_descriptor_address); > +} > + > +static void i2c_hid_acpi_shutdown(struct i2c_client *client) > +{ > + i2c_hid_core_shutdown(client); > + acpi_device_set_power(ACPI_COMPANION(&client->dev), ACPI_STATE_D3_COLD); > +} > + > +static const struct dev_pm_ops i2c_hid_acpi_pm = { > + SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume) > +}; > + > +static const struct acpi_device_id i2c_hid_acpi_match[] = { > + {"ACPI0C50", 0 }, > + {"PNP0C50", 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, i2c_hid_acpi_match); > + > +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); > + > +static struct i2c_driver i2c_hid_acpi_driver = { > + .driver = { > + .name = "i2c_hid_acpi", > + .pm = &i2c_hid_acpi_pm, > + .probe_type = PROBE_PREFER_ASYNCHRONOUS, > + .acpi_match_table = ACPI_PTR(i2c_hid_acpi_match), > + }, > + > + .probe = i2c_hid_acpi_probe, > + .remove = i2c_hid_core_remove, > + .shutdown = i2c_hid_acpi_shutdown, > + .id_table = i2c_hid_acpi_id_table, > +}; > + > +module_i2c_driver(i2c_hid_acpi_driver); > + > +MODULE_DESCRIPTION("HID over I2C ACPI driver"); > +MODULE_AUTHOR("Benjamin Tissoires "); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > index aeff1ffb0c8b..7e7303c2a45e 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > @@ -35,12 +35,8 @@ > #include > #include > #include > -#include > -#include > #include > > -#include > - > #include "../hid-ids.h" > #include "i2c-hid.h" > > @@ -156,10 +152,10 @@ struct i2c_hid { > > wait_queue_head_t wait; /* For waiting the interrupt */ > > - struct i2c_hid_platform_data pdata; > - > bool irq_wake_enabled; > struct mutex reset_lock; > + > + struct i2chid_subclass_data *subclass; > }; Personally, I would do things a bit differently here: 1. Just add the int (*power_up_device)(struct i2chid_subclass_data *subclass); void (*power_down_device)(struct i2chid_subclass_data *subclass); members which you put in the subclass struct here. 2. Move the declaration of this complete struct to drivers/hid/i2c-hid/i2c-hid.h and use this as the base-class which I described before (and store the client pointer here). 3. And then kzalloc both this baseclass struct + the subclass-data (only the bool "power_fixed" in the ACPI case) in one go in the subclass code replacing 2 kzallocs (+ error checking with one, simplifying the code and reducing memory fragmentation (by a tiny sliver). ### About the power_*_device callbacks, I wonder if it would not be more consistent to also have a shutdown callback and make i2c_driver.shutdown point to a (modified) i2c_hid_core_shutdown() function. You may also want to consider pointing that shutdown callback to the power_off function in the of case (in a separate commit as that is a behavioral change). > diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c > new file mode 100644 > index 000000000000..e1838cdef0aa > --- /dev/null > +++ b/drivers/hid/i2c-hid/i2c-hid-of.c > @@ -0,0 +1,149 @@ > +/* > + * HID over I2C Open Firmware Subclass > + * > + * Copyright (c) 2012 Benjamin Tissoires > + * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France > + * Copyright (c) 2012 Red Hat, Inc > +MODULE_DESCRIPTION("HID over I2C OF driver"); > +MODULE_AUTHOR("Benjamin Tissoires "); In case Benjamin misses this during his own review: I'm not sure if he will want to be set as AUTHOR of this, given that part of the plan is for someone else to be the primary point of contact for the of bits. > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/platform_data/i2c-hid.h b/include/linux/platform_data/i2c-hid.h > deleted file mode 100644 > index c628bb5e1061..000000000000 > --- a/include/linux/platform_data/i2c-hid.h > +++ /dev/null > @@ -1,41 +0,0 @@ > -/* > - * HID over I2C protocol implementation > - * > - * Copyright (c) 2012 Benjamin Tissoires > - * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France > - * > - * This file is subject to the terms and conditions of the GNU General Public > - * License. See the file COPYING in the main directory of this archive for > - * more details. > - */ > - > -#ifndef __LINUX_I2C_HID_H > -#define __LINUX_I2C_HID_H > - > -#include > -#include > - > -/** > - * struct i2chid_platform_data - used by hid over i2c implementation. > - * @hid_descriptor_address: i2c register where the HID descriptor is stored. > - * @supplies: regulators for powering on the device. > - * @post_power_delay_ms: delay after powering on before device is usable. > - * > - * Note that it is the responsibility of the platform driver (or the acpi 5.0 > - * driver, or the flattened device tree) to setup the irq related to the gpio in > - * the struct i2c_board_info. > - * The platform driver should also setup the gpio according to the device: > - * > - * A typical example is the following: > - * irq = gpio_to_irq(intr_gpio); > - * hkdk4412_i2c_devs5[0].irq = irq; // store the irq in i2c_board_info > - * gpio_request(intr_gpio, "elan-irq"); > - * s3c_gpio_setpull(intr_gpio, S3C_GPIO_PULL_UP); > - */ > -struct i2c_hid_platform_data { > - u16 hid_descriptor_address; > - struct regulator_bulk_data supplies[2]; > - int post_power_delay_ms; > -}; > - > -#endif /* __LINUX_I2C_HID_H */ Nice, deleting platform_data files is always good :) Regards, Hans