[5/9] HID: i2c-hid: Rearrange probe() to power things up later
Commit Message
In a future patch, we want to change i2c-hid not to necessarily power
up the touchscreen during probe. In preparation for that, rearrange
the probe function so that we put as much stuff _before_ powering up
the device as possible.
This change is expected to have no functional effect.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 124 ++++++++++++++++++-----------
1 file changed, 77 insertions(+), 47 deletions(-)
Comments
Hi Douglas,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on hid/for-next drm-misc/drm-misc-next linus/master v6.4-rc3 next-20230523]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Douglas-Anderson/dt-bindings-HID-i2c-hid-Add-panel-property-to-i2c-hid-backed-panels/20230524-034323
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230523122802.5.Ifcc9b0a44895d164788966f9b9511fe094ca8cf9%40changeid
patch subject: [PATCH 5/9] HID: i2c-hid: Rearrange probe() to power things up later
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d744f5cbecdd7a7ee141282a5879e9112f22fb22
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Douglas-Anderson/dt-bindings-HID-i2c-hid-Add-panel-property-to-i2c-hid-backed-panels/20230524-034323
git checkout d744f5cbecdd7a7ee141282a5879e9112f22fb22
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/hid/i2c-hid/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305240732.R1ZAvBTN-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/hid/i2c-hid/i2c-hid-core.c:956:5: warning: no previous prototype for 'i2c_hid_core_initial_power_up' [-Wmissing-prototypes]
956 | int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/i2c_hid_core_initial_power_up +956 drivers/hid/i2c-hid/i2c-hid-core.c
943
944 /**
945 * i2c_hid_core_initial_power_up() - First time power up of the i2c-hid device.
946 * @ihid: The ihid object created during probe.
947 *
948 * This function is called at probe time.
949 *
950 * The initial power on is where we do some basic validation that the device
951 * exists, where we fetch the HID descriptor, and where we create the actual
952 * HID devices.
953 *
954 * Return: 0 or error code.
955 */
> 956 int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
957 {
958 struct i2c_client *client = ihid->client;
959 struct hid_device *hid = ihid->hid;
960 int ret;
961
962 ret = i2c_hid_core_power_up(ihid);
963 if (ret)
964 return ret;
965
966 /* Make sure there is something at this address */
967 ret = i2c_smbus_read_byte(client);
968 if (ret < 0) {
969 i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
970 ret = -ENXIO;
971 goto err;
972 }
973
974 ret = i2c_hid_fetch_hid_descriptor(ihid);
975 if (ret < 0) {
976 dev_err(&client->dev,
977 "Failed to fetch the HID Descriptor\n");
978 goto err;
979 }
980
981 enable_irq(client->irq);
982
983 hid->version = le16_to_cpu(ihid->hdesc.bcdVersion);
984 hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
985 hid->product = le16_to_cpu(ihid->hdesc.wProductID);
986
987 hid->initial_quirks |= i2c_hid_get_dmi_quirks(hid->vendor,
988 hid->product);
989
990 snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
991 client->name, (u16)hid->vendor, (u16)hid->product);
992 strscpy(hid->phys, dev_name(&client->dev), sizeof(hid->phys));
993
994 ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
995
996 ret = hid_add_device(hid);
997 if (ret) {
998 if (ret != -ENODEV)
999 hid_err(client, "can't add hid device: %d\n", ret);
1000 goto err;
1001 }
1002
1003 return 0;
1004
1005 err:
1006 i2c_hid_core_power_down(ihid);
1007 return ret;
1008 }
1009
Hi Douglas,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on hid/for-next drm-misc/drm-misc-next linus/master v6.4-rc3 next-20230524]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Douglas-Anderson/dt-bindings-HID-i2c-hid-Add-panel-property-to-i2c-hid-backed-panels/20230524-034323
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230523122802.5.Ifcc9b0a44895d164788966f9b9511fe094ca8cf9%40changeid
patch subject: [PATCH 5/9] HID: i2c-hid: Rearrange probe() to power things up later
config: i386-randconfig-s001
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/d744f5cbecdd7a7ee141282a5879e9112f22fb22
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Douglas-Anderson/dt-bindings-HID-i2c-hid-Add-panel-property-to-i2c-hid-backed-panels/20230524-034323
git checkout d744f5cbecdd7a7ee141282a5879e9112f22fb22
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 olddefconfig
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hid/i2c-hid/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305242047.249gSHeV-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/hid/i2c-hid/i2c-hid-core.c:956:5: sparse: sparse: symbol 'i2c_hid_core_initial_power_up' was not declared. Should it be static?
@@ -855,7 +855,8 @@ static int i2c_hid_init_irq(struct i2c_client *client)
irqflags = IRQF_TRIGGER_LOW;
ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
- irqflags | IRQF_ONESHOT, client->name, ihid);
+ irqflags | IRQF_ONESHOT | IRQF_NO_AUTOEN,
+ client->name, ihid);
if (ret < 0) {
dev_warn(&client->dev,
"Could not register for %s interrupt, irq = %d,"
@@ -940,6 +941,72 @@ static void i2c_hid_core_shutdown_tail(struct i2c_hid *ihid)
ihid->ops->shutdown_tail(ihid->ops);
}
+/**
+ * i2c_hid_core_initial_power_up() - First time power up of the i2c-hid device.
+ * @ihid: The ihid object created during probe.
+ *
+ * This function is called at probe time.
+ *
+ * The initial power on is where we do some basic validation that the device
+ * exists, where we fetch the HID descriptor, and where we create the actual
+ * HID devices.
+ *
+ * Return: 0 or error code.
+ */
+int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
+{
+ struct i2c_client *client = ihid->client;
+ struct hid_device *hid = ihid->hid;
+ int ret;
+
+ ret = i2c_hid_core_power_up(ihid);
+ if (ret)
+ return ret;
+
+ /* Make sure there is something at this address */
+ ret = i2c_smbus_read_byte(client);
+ if (ret < 0) {
+ i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
+ ret = -ENXIO;
+ goto err;
+ }
+
+ ret = i2c_hid_fetch_hid_descriptor(ihid);
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "Failed to fetch the HID Descriptor\n");
+ goto err;
+ }
+
+ enable_irq(client->irq);
+
+ hid->version = le16_to_cpu(ihid->hdesc.bcdVersion);
+ hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
+ hid->product = le16_to_cpu(ihid->hdesc.wProductID);
+
+ hid->initial_quirks |= i2c_hid_get_dmi_quirks(hid->vendor,
+ hid->product);
+
+ snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
+ client->name, (u16)hid->vendor, (u16)hid->product);
+ strscpy(hid->phys, dev_name(&client->dev), sizeof(hid->phys));
+
+ ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
+
+ ret = hid_add_device(hid);
+ if (ret) {
+ if (ret != -ENODEV)
+ hid_err(client, "can't add hid device: %d\n", ret);
+ goto err;
+ }
+
+ return 0;
+
+err:
+ i2c_hid_core_power_down(ihid);
+ return ret;
+}
+
int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
u16 hid_descriptor_address, u32 quirks)
{
@@ -966,16 +1033,10 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
if (!ihid)
return -ENOMEM;
- ihid->ops = ops;
-
- ret = i2c_hid_core_power_up(ihid);
- if (ret)
- return ret;
-
i2c_set_clientdata(client, ihid);
+ ihid->ops = ops;
ihid->client = client;
-
ihid->wHIDDescRegister = cpu_to_le16(hid_descriptor_address);
init_waitqueue_head(&ihid->wait);
@@ -986,28 +1047,12 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
* real computation later. */
ret = i2c_hid_alloc_buffers(ihid, HID_MIN_BUFFER_SIZE);
if (ret < 0)
- goto err_powered;
-
+ return ret;
device_enable_async_suspend(&client->dev);
- /* Make sure there is something at this address */
- ret = i2c_smbus_read_byte(client);
- if (ret < 0) {
- i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
- ret = -ENXIO;
- goto err_powered;
- }
-
- ret = i2c_hid_fetch_hid_descriptor(ihid);
- if (ret < 0) {
- dev_err(&client->dev,
- "Failed to fetch the HID Descriptor\n");
- goto err_powered;
- }
-
ret = i2c_hid_init_irq(client);
if (ret < 0)
- goto err_powered;
+ goto err_buffers_allocated;
hid = hid_allocate_device();
if (IS_ERR(hid)) {
@@ -1021,26 +1066,11 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
hid->ll_driver = &i2c_hid_ll_driver;
hid->dev.parent = &client->dev;
hid->bus = BUS_I2C;
- hid->version = le16_to_cpu(ihid->hdesc.bcdVersion);
- hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
- hid->product = le16_to_cpu(ihid->hdesc.wProductID);
-
hid->initial_quirks = quirks;
- hid->initial_quirks |= i2c_hid_get_dmi_quirks(hid->vendor,
- hid->product);
-
- snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
- client->name, (u16)hid->vendor, (u16)hid->product);
- strscpy(hid->phys, dev_name(&client->dev), sizeof(hid->phys));
-
- ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
- ret = hid_add_device(hid);
- if (ret) {
- if (ret != -ENODEV)
- hid_err(client, "can't add hid device: %d\n", ret);
+ ret = i2c_hid_core_initial_power_up(ihid);
+ if (ret)
goto err_mem_free;
- }
return 0;
@@ -1050,9 +1080,9 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
err_irq:
free_irq(client->irq, ihid);
-err_powered:
- i2c_hid_core_power_down(ihid);
+err_buffers_allocated:
i2c_hid_free_buffers(ihid);
+
return ret;
}
EXPORT_SYMBOL_GPL(i2c_hid_core_probe);
@@ -1062,6 +1092,8 @@ void i2c_hid_core_remove(struct i2c_client *client)
struct i2c_hid *ihid = i2c_get_clientdata(client);
struct hid_device *hid;
+ i2c_hid_core_power_down(ihid);
+
hid = ihid->hid;
hid_destroy_device(hid);
@@ -1069,8 +1101,6 @@ void i2c_hid_core_remove(struct i2c_client *client)
if (ihid->bufsize)
i2c_hid_free_buffers(ihid);
-
- i2c_hid_core_power_down(ihid);
}
EXPORT_SYMBOL_GPL(i2c_hid_core_remove);