* [PATCH v10 06/11] mfd: max77650: new core mfd driver
From: Bartosz Golaszewski @ 2019-04-23 9:04 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski
In-Reply-To: <20190423090451.23711-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Add the core mfd driver for max77650 PMIC. We define five sub-devices
for which the drivers will be added in subsequent patches.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/mfd/Kconfig | 14 +++
drivers/mfd/Makefile | 1 +
drivers/mfd/max77650.c | 232 +++++++++++++++++++++++++++++++++++
include/linux/mfd/max77650.h | 59 +++++++++
4 files changed, 306 insertions(+)
create mode 100644 drivers/mfd/max77650.c
create mode 100644 include/linux/mfd/max77650.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 26ad6468d13a..0f394f08db6f 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -733,6 +733,20 @@ config MFD_MAX77620
provides common support for accessing the device; additional drivers
must be enabled in order to use the functionality of the device.
+config MFD_MAX77650
+ tristate "Maxim MAX77650/77651 PMIC Support"
+ depends on I2C
+ depends on OF || COMPILE_TEST
+ select MFD_CORE
+ select REGMAP_I2C
+ help
+ Say Y here to add support for Maxim Semiconductor MAX77650 and
+ MAX77651 Power Management ICs. This is the core multifunction
+ driver for interacting with the device. The module name is
+ 'max77650'. Additional drivers can be enabled in order to use
+ the following functionalities of the device: GPIO, regulator,
+ charger, LED, onkey.
+
config MFD_MAX77686
tristate "Maxim Semiconductor MAX77686/802 PMIC Support"
depends on I2C
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b4569ed7f3f3..5727d099c16f 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -155,6 +155,7 @@ obj-$(CONFIG_MFD_DA9150) += da9150-core.o
obj-$(CONFIG_MFD_MAX14577) += max14577.o
obj-$(CONFIG_MFD_MAX77620) += max77620.o
+obj-$(CONFIG_MFD_MAX77650) += max77650.o
obj-$(CONFIG_MFD_MAX77686) += max77686.o
obj-$(CONFIG_MFD_MAX77693) += max77693.o
obj-$(CONFIG_MFD_MAX77843) += max77843.o
diff --git a/drivers/mfd/max77650.c b/drivers/mfd/max77650.c
new file mode 100644
index 000000000000..60e07aca6ae5
--- /dev/null
+++ b/drivers/mfd/max77650.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2018 BayLibre SAS
+// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+//
+// Core MFD driver for MAXIM 77650/77651 charger/power-supply.
+// Programming manual: https://pdfserv.maximintegrated.com/en/an/AN6428.pdf
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/max77650.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#define MAX77650_INT_GPI_F_MSK BIT(0)
+#define MAX77650_INT_GPI_R_MSK BIT(1)
+#define MAX77650_INT_GPI_MSK \
+ (MAX77650_INT_GPI_F_MSK | MAX77650_INT_GPI_R_MSK)
+#define MAX77650_INT_nEN_F_MSK BIT(2)
+#define MAX77650_INT_nEN_R_MSK BIT(3)
+#define MAX77650_INT_TJAL1_R_MSK BIT(4)
+#define MAX77650_INT_TJAL2_R_MSK BIT(5)
+#define MAX77650_INT_DOD_R_MSK BIT(6)
+
+#define MAX77650_INT_THM_MSK BIT(0)
+#define MAX77650_INT_CHG_MSK BIT(1)
+#define MAX77650_INT_CHGIN_MSK BIT(2)
+#define MAX77650_INT_TJ_REG_MSK BIT(3)
+#define MAX77650_INT_CHGIN_CTRL_MSK BIT(4)
+#define MAX77650_INT_SYS_CTRL_MSK BIT(5)
+#define MAX77650_INT_SYS_CNFG_MSK BIT(6)
+
+#define MAX77650_INT_GLBL_OFFSET 0
+#define MAX77650_INT_CHG_OFFSET 1
+
+#define MAX77650_SBIA_LPM_MASK BIT(5)
+#define MAX77650_SBIA_LPM_DISABLED 0x00
+
+enum {
+ MAX77650_INT_GPI,
+ MAX77650_INT_nEN_F,
+ MAX77650_INT_nEN_R,
+ MAX77650_INT_TJAL1_R,
+ MAX77650_INT_TJAL2_R,
+ MAX77650_INT_DOD_R,
+ MAX77650_INT_THM,
+ MAX77650_INT_CHG,
+ MAX77650_INT_CHGIN,
+ MAX77650_INT_TJ_REG,
+ MAX77650_INT_CHGIN_CTRL,
+ MAX77650_INT_SYS_CTRL,
+ MAX77650_INT_SYS_CNFG,
+};
+
+static const struct resource max77650_charger_resources[] = {
+ DEFINE_RES_IRQ_NAMED(MAX77650_INT_CHG, "CHG"),
+ DEFINE_RES_IRQ_NAMED(MAX77650_INT_CHGIN, "CHGIN"),
+};
+
+static const struct resource max77650_gpio_resources[] = {
+ DEFINE_RES_IRQ_NAMED(MAX77650_INT_GPI, "GPI"),
+};
+
+static const struct resource max77650_onkey_resources[] = {
+ DEFINE_RES_IRQ_NAMED(MAX77650_INT_nEN_F, "nEN_F"),
+ DEFINE_RES_IRQ_NAMED(MAX77650_INT_nEN_R, "nEN_R"),
+};
+
+static const struct mfd_cell max77650_cells[] = {
+ {
+ .name = "max77650-regulator",
+ .of_compatible = "maxim,max77650-regulator",
+ }, {
+ .name = "max77650-charger",
+ .of_compatible = "maxim,max77650-charger",
+ .resources = max77650_charger_resources,
+ .num_resources = ARRAY_SIZE(max77650_charger_resources),
+ }, {
+ .name = "max77650-gpio",
+ .of_compatible = "maxim,max77650-gpio",
+ .resources = max77650_gpio_resources,
+ .num_resources = ARRAY_SIZE(max77650_gpio_resources),
+ }, {
+ .name = "max77650-led",
+ .of_compatible = "maxim,max77650-led",
+ }, {
+ .name = "max77650-onkey",
+ .of_compatible = "maxim,max77650-onkey",
+ .resources = max77650_onkey_resources,
+ .num_resources = ARRAY_SIZE(max77650_onkey_resources),
+ },
+};
+
+static const struct regmap_irq max77650_irqs[] = {
+ [MAX77650_INT_GPI] = {
+ .reg_offset = MAX77650_INT_GLBL_OFFSET,
+ .mask = MAX77650_INT_GPI_MSK,
+ .type = {
+ .type_falling_val = MAX77650_INT_GPI_F_MSK,
+ .type_rising_val = MAX77650_INT_GPI_R_MSK,
+ .types_supported = IRQ_TYPE_EDGE_BOTH,
+ },
+ },
+ REGMAP_IRQ_REG(MAX77650_INT_nEN_F,
+ MAX77650_INT_GLBL_OFFSET, MAX77650_INT_nEN_F_MSK),
+ REGMAP_IRQ_REG(MAX77650_INT_nEN_R,
+ MAX77650_INT_GLBL_OFFSET, MAX77650_INT_nEN_R_MSK),
+ REGMAP_IRQ_REG(MAX77650_INT_TJAL1_R,
+ MAX77650_INT_GLBL_OFFSET, MAX77650_INT_TJAL1_R_MSK),
+ REGMAP_IRQ_REG(MAX77650_INT_TJAL2_R,
+ MAX77650_INT_GLBL_OFFSET, MAX77650_INT_TJAL2_R_MSK),
+ REGMAP_IRQ_REG(MAX77650_INT_DOD_R,
+ MAX77650_INT_GLBL_OFFSET, MAX77650_INT_DOD_R_MSK),
+ REGMAP_IRQ_REG(MAX77650_INT_THM,
+ MAX77650_INT_CHG_OFFSET, MAX77650_INT_THM_MSK),
+ REGMAP_IRQ_REG(MAX77650_INT_CHG,
+ MAX77650_INT_CHG_OFFSET, MAX77650_INT_CHG_MSK),
+ REGMAP_IRQ_REG(MAX77650_INT_CHGIN,
+ MAX77650_INT_CHG_OFFSET, MAX77650_INT_CHGIN_MSK),
+ REGMAP_IRQ_REG(MAX77650_INT_TJ_REG,
+ MAX77650_INT_CHG_OFFSET, MAX77650_INT_TJ_REG_MSK),
+ REGMAP_IRQ_REG(MAX77650_INT_CHGIN_CTRL,
+ MAX77650_INT_CHG_OFFSET, MAX77650_INT_CHGIN_CTRL_MSK),
+ REGMAP_IRQ_REG(MAX77650_INT_SYS_CTRL,
+ MAX77650_INT_CHG_OFFSET, MAX77650_INT_SYS_CTRL_MSK),
+ REGMAP_IRQ_REG(MAX77650_INT_SYS_CNFG,
+ MAX77650_INT_CHG_OFFSET, MAX77650_INT_SYS_CNFG_MSK),
+};
+
+static const struct regmap_irq_chip max77650_irq_chip = {
+ .name = "max77650-irq",
+ .irqs = max77650_irqs,
+ .num_irqs = ARRAY_SIZE(max77650_irqs),
+ .num_regs = 2,
+ .status_base = MAX77650_REG_INT_GLBL,
+ .mask_base = MAX77650_REG_INTM_GLBL,
+ .type_in_mask = true,
+ .type_invert = true,
+ .init_ack_masked = true,
+ .clear_on_unmask = true,
+};
+
+static const struct regmap_config max77650_regmap_config = {
+ .name = "max77650",
+ .reg_bits = 8,
+ .val_bits = 8,
+};
+
+static int max77650_i2c_probe(struct i2c_client *i2c)
+{
+ struct regmap_irq_chip_data *irq_data;
+ struct device *dev = &i2c->dev;
+ struct irq_domain *domain;
+ struct regmap *map;
+ unsigned int val;
+ int rv, id;
+
+ map = devm_regmap_init_i2c(i2c, &max77650_regmap_config);
+ if (IS_ERR(map)) {
+ dev_err(dev, "Unable to initialise I2C Regmap\n");
+ return PTR_ERR(map);
+ }
+
+ rv = regmap_read(map, MAX77650_REG_CID, &val);
+ if (rv) {
+ dev_err(dev, "Unable to read Chip ID\n");
+ return rv;
+ }
+
+ id = MAX77650_CID_BITS(val);
+ switch (id) {
+ case MAX77650_CID_77650A:
+ case MAX77650_CID_77650C:
+ case MAX77650_CID_77651A:
+ case MAX77650_CID_77651B:
+ break;
+ default:
+ dev_err(dev, "Chip not supported - ID: 0x%02x\n", id);
+ return -ENODEV;
+ }
+
+ /*
+ * This IC has a low-power mode which reduces the quiescent current
+ * consumption to ~5.6uA but is only suitable for systems consuming
+ * less than ~2mA. Since this is not likely the case even on
+ * linux-based wearables - keep the chip in normal power mode.
+ */
+ rv = regmap_update_bits(map,
+ MAX77650_REG_CNFG_GLBL,
+ MAX77650_SBIA_LPM_MASK,
+ MAX77650_SBIA_LPM_DISABLED);
+ if (rv) {
+ dev_err(dev, "Unable to change the power mode\n");
+ return rv;
+ }
+
+ rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77650_irq_chip, &irq_data);
+ if (rv) {
+ dev_err(dev, "Unable to add Regmap IRQ chip\n");
+ return rv;
+ }
+
+ domain = regmap_irq_get_domain(irq_data);
+
+ return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
+ max77650_cells, ARRAY_SIZE(max77650_cells),
+ NULL, 0, domain);
+}
+
+static const struct of_device_id max77650_of_match[] = {
+ { .compatible = "maxim,max77650" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, max77650_of_match);
+
+static struct i2c_driver max77650_i2c_driver = {
+ .driver = {
+ .name = "max77650",
+ .of_match_table = of_match_ptr(max77650_of_match),
+ },
+ .probe_new = max77650_i2c_probe,
+};
+module_i2c_driver(max77650_i2c_driver);
+
+MODULE_DESCRIPTION("MAXIM 77650/77651 multi-function core driver");
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/max77650.h b/include/linux/mfd/max77650.h
new file mode 100644
index 000000000000..c809e211a8cd
--- /dev/null
+++ b/include/linux/mfd/max77650.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 BayLibre SAS
+ * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ *
+ * Common definitions for MAXIM 77650/77651 charger/power-supply.
+ */
+
+#ifndef MAX77650_H
+#define MAX77650_H
+
+#include <linux/bits.h>
+
+#define MAX77650_REG_INT_GLBL 0x00
+#define MAX77650_REG_INT_CHG 0x01
+#define MAX77650_REG_STAT_CHG_A 0x02
+#define MAX77650_REG_STAT_CHG_B 0x03
+#define MAX77650_REG_ERCFLAG 0x04
+#define MAX77650_REG_STAT_GLBL 0x05
+#define MAX77650_REG_INTM_GLBL 0x06
+#define MAX77650_REG_INTM_CHG 0x07
+#define MAX77650_REG_CNFG_GLBL 0x10
+#define MAX77650_REG_CID 0x11
+#define MAX77650_REG_CNFG_GPIO 0x12
+#define MAX77650_REG_CNFG_CHG_A 0x18
+#define MAX77650_REG_CNFG_CHG_B 0x19
+#define MAX77650_REG_CNFG_CHG_C 0x1a
+#define MAX77650_REG_CNFG_CHG_D 0x1b
+#define MAX77650_REG_CNFG_CHG_E 0x1c
+#define MAX77650_REG_CNFG_CHG_F 0x1d
+#define MAX77650_REG_CNFG_CHG_G 0x1e
+#define MAX77650_REG_CNFG_CHG_H 0x1f
+#define MAX77650_REG_CNFG_CHG_I 0x20
+#define MAX77650_REG_CNFG_SBB_TOP 0x28
+#define MAX77650_REG_CNFG_SBB0_A 0x29
+#define MAX77650_REG_CNFG_SBB0_B 0x2a
+#define MAX77650_REG_CNFG_SBB1_A 0x2b
+#define MAX77650_REG_CNFG_SBB1_B 0x2c
+#define MAX77650_REG_CNFG_SBB2_A 0x2d
+#define MAX77650_REG_CNFG_SBB2_B 0x2e
+#define MAX77650_REG_CNFG_LDO_A 0x38
+#define MAX77650_REG_CNFG_LDO_B 0x39
+#define MAX77650_REG_CNFG_LED0_A 0x40
+#define MAX77650_REG_CNFG_LED1_A 0x41
+#define MAX77650_REG_CNFG_LED2_A 0x42
+#define MAX77650_REG_CNFG_LED0_B 0x43
+#define MAX77650_REG_CNFG_LED1_B 0x44
+#define MAX77650_REG_CNFG_LED2_B 0x45
+#define MAX77650_REG_CNFG_LED_TOP 0x46
+
+#define MAX77650_CID_MASK GENMASK(3, 0)
+#define MAX77650_CID_BITS(_reg) (_reg & MAX77650_CID_MASK)
+
+#define MAX77650_CID_77650A 0x03
+#define MAX77650_CID_77650C 0x0a
+#define MAX77650_CID_77651A 0x06
+#define MAX77650_CID_77651B 0x08
+
+#endif /* MAX77650_H */
--
2.21.0
^ permalink raw reply related
* [PATCH v10 05/11] mfd: core: document mfd_add_devices()
From: Bartosz Golaszewski @ 2019-04-23 9:04 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski
In-Reply-To: <20190423090451.23711-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Add a kernel doc for mfd_add_devices().
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
drivers/mfd/mfd-core.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 94e3f32ce935..1ade4c8cc91f 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -269,6 +269,19 @@ static int mfd_add_device(struct device *parent, int id,
return ret;
}
+/**
+ * mfd_add_devices - register child devices
+ *
+ * @parent: Pointer to parent device.
+ * @id: Can be PLATFORM_DEVID_AUTO to let the Platform API take care
+ * of device numbering, or will be added to a device's cell_id.
+ * @cells: Array of (struct mfd_cell)s describing child devices.
+ * @n_devs: Number of child devices to register.
+ * @mem_base: Parent register range resource for child devices.
+ * @irq_base: Base of the range of virtual interrupt numbers allocated for
+ * this MFD device. Unused if @domain is specified.
+ * @domain: Interrupt domain to create mappings for hardware interrupts.
+ */
int mfd_add_devices(struct device *parent, int id,
const struct mfd_cell *cells, int n_devs,
struct resource *mem_base,
--
2.21.0
^ permalink raw reply related
* [PATCH v10 04/11] dt-bindings: input: add DT bindings for max77650
From: Bartosz Golaszewski @ 2019-04-23 9:04 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski, Rob Herring
In-Reply-To: <20190423090451.23711-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Add the DT binding document for the onkey module of max77650.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
.../bindings/input/max77650-onkey.txt | 26 +++++++++++++++++++
1 file changed, 26 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/max77650-onkey.txt
diff --git a/Documentation/devicetree/bindings/input/max77650-onkey.txt b/Documentation/devicetree/bindings/input/max77650-onkey.txt
new file mode 100644
index 000000000000..477dc74f452a
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/max77650-onkey.txt
@@ -0,0 +1,26 @@
+Onkey driver for MAX77650 PMIC from Maxim Integrated.
+
+This module is part of the MAX77650 MFD device. For more details
+see Documentation/devicetree/bindings/mfd/max77650.txt.
+
+The onkey controller is represented as a sub-node of the PMIC node on
+the device tree.
+
+Required properties:
+--------------------
+- compatible: Must be "maxim,max77650-onkey".
+
+Optional properties:
+- linux,code: The key-code to be reported when the key is pressed.
+ Defaults to KEY_POWER.
+- maxim,onkey-slide: The system's button is a slide switch, not the default
+ push button.
+
+Example:
+--------
+
+ onkey {
+ compatible = "maxim,max77650-onkey";
+ linux,code = <KEY_END>;
+ maxim,onkey-slide;
+ };
--
2.21.0
^ permalink raw reply related
* [PATCH v10 03/11] dt-bindings: leds: add DT bindings for max77650
From: Bartosz Golaszewski @ 2019-04-23 9:04 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski, Rob Herring
In-Reply-To: <20190423090451.23711-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Add the DT binding document for the LEDs module of max77650.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
.../bindings/leds/leds-max77650.txt | 57 +++++++++++++++++++
1 file changed, 57 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-max77650.txt
diff --git a/Documentation/devicetree/bindings/leds/leds-max77650.txt b/Documentation/devicetree/bindings/leds/leds-max77650.txt
new file mode 100644
index 000000000000..3a67115cc1da
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-max77650.txt
@@ -0,0 +1,57 @@
+LED driver for MAX77650 PMIC from Maxim Integrated.
+
+This module is part of the MAX77650 MFD device. For more details
+see Documentation/devicetree/bindings/mfd/max77650.txt.
+
+The LED controller is represented as a sub-node of the PMIC node on
+the device tree.
+
+This device has three current sinks.
+
+Required properties:
+--------------------
+- compatible: Must be "maxim,max77650-led"
+- #address-cells: Must be <1>.
+- #size-cells: Must be <0>.
+
+Each LED is represented as a sub-node of the LED-controller node. Up to
+three sub-nodes can be defined.
+
+Required properties of the sub-node:
+------------------------------------
+
+- reg: Must be <0>, <1> or <2>.
+
+Optional properties of the sub-node:
+------------------------------------
+
+- label: See Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger: See Documentation/devicetree/bindings/leds/common.txt
+
+For more details, please refer to the generic GPIO DT binding document
+<devicetree/bindings/gpio/gpio.txt>.
+
+Example:
+--------
+
+ leds {
+ compatible = "maxim,max77650-led";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ label = "blue:usr0";
+ };
+
+ led@1 {
+ reg = <1>;
+ label = "red:usr1";
+ linux,default-trigger = "heartbeat";
+ };
+
+ led@2 {
+ reg = <2>;
+ label = "green:usr2";
+ };
+ };
--
2.21.0
^ permalink raw reply related
* [PATCH v10 02/11] dt-bindings: power: supply: add DT bindings for max77650
From: Bartosz Golaszewski @ 2019-04-23 9:04 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski
In-Reply-To: <20190423090451.23711-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Add the DT binding document for the battery charger module of max77650.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
.../power/supply/max77650-charger.txt | 28 +++++++++++++++++++
1 file changed, 28 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/max77650-charger.txt
diff --git a/Documentation/devicetree/bindings/power/supply/max77650-charger.txt b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
new file mode 100644
index 000000000000..e6d0fb6ff94e
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
@@ -0,0 +1,28 @@
+Battery charger driver for MAX77650 PMIC from Maxim Integrated.
+
+This module is part of the MAX77650 MFD device. For more details
+see Documentation/devicetree/bindings/mfd/max77650.txt.
+
+The charger is represented as a sub-node of the PMIC node on the device tree.
+
+Required properties:
+--------------------
+- compatible: Must be "maxim,max77650-charger"
+
+Optional properties:
+--------------------
+- input-voltage-min-microvolt: Minimum CHGIN regulation voltage. Must be one
+ of: 4000000, 4100000, 4200000, 4300000,
+ 4400000, 4500000, 4600000, 4700000.
+- input-current-limit-microamp: CHGIN input current limit (in microamps). Must
+ be one of: 95000, 190000, 285000, 380000,
+ 475000.
+
+Example:
+--------
+
+ charger {
+ compatible = "maxim,max77650-charger";
+ input-voltage-min-microvolt = <4200000>;
+ input-current-limit-microamp = <285000>;
+ };
--
2.21.0
^ permalink raw reply related
* [PATCH v10 01/11] dt-bindings: mfd: add DT bindings for max77650
From: Bartosz Golaszewski @ 2019-04-23 9:04 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski, Rob Herring
In-Reply-To: <20190423090451.23711-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Add a DT binding document for max77650 ultra-low power PMIC. This
describes the core mfd device and the GPIO module.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
.../devicetree/bindings/mfd/max77650.txt | 46 +++++++++++++++++++
1 file changed, 46 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/max77650.txt
diff --git a/Documentation/devicetree/bindings/mfd/max77650.txt b/Documentation/devicetree/bindings/mfd/max77650.txt
new file mode 100644
index 000000000000..b529d8d19335
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/max77650.txt
@@ -0,0 +1,46 @@
+MAX77650 ultra low-power PMIC from Maxim Integrated.
+
+Required properties:
+-------------------
+- compatible: Must be "maxim,max77650"
+- reg: I2C device address.
+- interrupts: The interrupt on the parent the controller is
+ connected to.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells: Must be <2>.
+
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Must be <2>. The first cell is the pin number and
+ the second cell is used to specify the gpio active
+ state.
+
+Optional properties:
+--------------------
+gpio-line-names: Single string containing the name of the GPIO line.
+
+The GPIO-controller module is represented as part of the top-level PMIC
+node. The device exposes a single GPIO line.
+
+For device-tree bindings of other sub-modules (regulator, power supply,
+LEDs and onkey) refer to the binding documents under the respective
+sub-system directories.
+
+For more details on GPIO bindings, please refer to the generic GPIO DT
+binding document <devicetree/bindings/gpio/gpio.txt>.
+
+Example:
+--------
+
+ pmic@48 {
+ compatible = "maxim,max77650";
+ reg = <0x48>;
+
+ interrupt-controller;
+ interrupt-parent = <&gpio2>;
+ #interrupt-cells = <2>;
+ interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-line-names = "max77650-charger";
+ };
--
2.21.0
^ permalink raw reply related
* [PATCH v10 00/11] mfd: add support for max77650 PMIC
From: Bartosz Golaszewski @ 2019-04-23 9:04 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
This series adds support for max77650 ultra low-power PMIC. It provides
the core mfd driver and a set of five sub-drivers for the regulator,
power supply, gpio, leds and input subsystems.
Patches 1-4 add the DT binding documents. Patch 5 documents mfd_add_devices().
Patches 6-10 add all drivers. Last patch adds a MAINTAINERS entry for this
device.
The regulator part is already upstream.
v1 -> v2:
=========
General:
- use C++ style comments for the SPDX license identifier and the
copyright header
- s/MODULE_LICENSE("GPL")/MODULE_LICENSE("GPL v2")/
- lookup the virtual interrupt numbers in the MFD driver, setup
resources for child devices and use platform_get_irq_byname()
in sub-drivers
- picked up review tags
- use devm_request_any_context_irq() for interrupt requests
LEDs:
- changed the max77650_leds_ prefix to max77650_led_
- drop the max77650_leds structure as the only field it held was the
regmap pointer, move said pointer to struct max77650_led
- change the driver name to "max77650-led"
- drop the last return value check and return the result of
regmap_write() directly
- change the labeling scheme to one consistent with other LED drivers
ONKEY:
- drop the key reporting helper and call the input functions directly
from interrupt handlers
- rename the rv local variable to error
- drop parent device asignment
Regulator:
- drop the unnecessary init_data lookup from the driver code
- drop unnecessary include
Charger:
- disable the charger on driver remove
- change the power supply type to POWER_SUPPLY_TYPE_USB
GPIO:
- drop interrupt support until we have correct implementation of hierarchical
irqs in gpiolib
v2 -> v3:
=========
General:
- dropped regulator patches as they're already in Mark Brown's branch
LED:
- fix the compatible string in the DT binding example
- use the max_brightness property
- use a common prefix ("MAX77650_LED") for all defines in the driver
MFD:
- add the MODULE_DEVICE_TABLE()
- add a sentinel to the of_device_id array
- constify the pointers to irq names
- use an enum instead of defines for interrupt indexes
v3 -> v4:
=========
GPIO:
- as discussed with Linus Walleij: the gpio-controller is now part of
the core mfd module (we don't spawn a sub-node anymore), the binding
document for GPIO has been dropped, the GPIO properties have been
defined in the binding document for the mfd core, the interrupt
functionality has been reintroduced with the irq directly passed from
the mfd part
- due to the above changes the Reviewed-by tag from Linus was dropped
v4 -> v5:
=========
General:
- add a patch documenting mfd_add_devices()
MFD:
- pass the regmap irq_chip irq domain to mfd over mfd_add_devices so that
the hw interrupts from resources can be correctly mapped to virtual irqs
- remove the enum listing cell indexes
- extend Kconfig help
- add a link to the programming manual
- use REGMAP_IRQ_REG() for regmap interrupts (except for GPI which has
is composed of two hw interrupts for rising and falling edge)
- add error messages in probe
- use PLATFORM_DEVID_NONE constant in devm_mfd_add_devices()
- set irq_base to 0 in regmap_add_irq_chip() as other users to, it's only
relevant if it's > 0
Charger:
- use non-maxim specific property names for minimum input voltage and current
limit
- code shrink by using the enable/disable charger helpers everywhere
- use more descriptive names for constants
Onkey:
- use EV_SW event type for slide mode
LED:
- remove stray " from Kconfig help
v5 -> v6:
=========
MFD:
- remove stray spaces in the binding document
- rename the example dt node
- remove unnecessary interrupt-parent property from the bindings
LED:
- add a missing dependency on LEDS_CLASS to Kconfig
Onkey:
- use boolean for the slide button property
Charger:
- fix the property names in DT example
- make constants even more readable
v6 -> v7:
=========
Charger:
- rename the current limit property to current-limit-microamp
v7 -> v8:
=========
General:
- collected acks from Lee
- changed the documentation for mfd_add_devices() as suggested by Lee
- rebased on top of v5.1-rc3
v8 > v9:
========
General:
- collected new tags
- rebased on top of v5.1-rc4
MFD:
- various improvements in error messages
- coding style tweaks
Charger:
- named the two optional properties in a more descriptive way, so that
we can make them generic for charger bindings if more potential users
appear
v9 -> v10:
==========
General:
- added the review tag from Sebastian
- rebased on top of v5.1-rc6
Charger:
- fixed the example in the binding document
Bartosz Golaszewski (11):
dt-bindings: mfd: add DT bindings for max77650
dt-bindings: power: supply: add DT bindings for max77650
dt-bindings: leds: add DT bindings for max77650
dt-bindings: input: add DT bindings for max77650
mfd: core: document mfd_add_devices()
mfd: max77650: new core mfd driver
power: supply: max77650: add support for battery charger
gpio: max77650: add GPIO support
leds: max77650: add LEDs support
input: max77650: add onkey support
MAINTAINERS: add an entry for max77650 mfd driver
.../bindings/input/max77650-onkey.txt | 26 ++
.../bindings/leds/leds-max77650.txt | 57 +++
.../devicetree/bindings/mfd/max77650.txt | 46 +++
.../power/supply/max77650-charger.txt | 28 ++
MAINTAINERS | 14 +
drivers/gpio/Kconfig | 7 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-max77650.c | 190 +++++++++
drivers/input/misc/Kconfig | 9 +
drivers/input/misc/Makefile | 1 +
drivers/input/misc/max77650-onkey.c | 121 ++++++
drivers/leds/Kconfig | 6 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-max77650.c | 147 +++++++
drivers/mfd/Kconfig | 14 +
drivers/mfd/Makefile | 1 +
drivers/mfd/max77650.c | 232 +++++++++++
drivers/mfd/mfd-core.c | 13 +
drivers/power/supply/Kconfig | 7 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/max77650-charger.c | 368 ++++++++++++++++++
include/linux/mfd/max77650.h | 59 +++
22 files changed, 1349 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/max77650-onkey.txt
create mode 100644 Documentation/devicetree/bindings/leds/leds-max77650.txt
create mode 100644 Documentation/devicetree/bindings/mfd/max77650.txt
create mode 100644 Documentation/devicetree/bindings/power/supply/max77650-charger.txt
create mode 100644 drivers/gpio/gpio-max77650.c
create mode 100644 drivers/input/misc/max77650-onkey.c
create mode 100644 drivers/leds/leds-max77650.c
create mode 100644 drivers/mfd/max77650.c
create mode 100644 drivers/power/supply/max77650-charger.c
create mode 100644 include/linux/mfd/max77650.h
--
2.21.0
^ permalink raw reply
* Re: [PATCH 3/3] iio: Add PAT9125 optical tracker sensor
From: Alexandre @ 2019-04-23 8:57 UTC (permalink / raw)
To: Jonathan Cameron
Cc: robh+dt, mark.rutland, knaack.h, lars, pmeerw, linux-kernel,
linux-iio, baylibre-upstreaming, Dmitry Torokhov, linux-input
In-Reply-To: <20190422094236.537e3a01@archlinux>
Hi Jonathan,
On 4/22/19 10:42, Jonathan Cameron wrote:
> On Tue, 16 Apr 2019 14:49:19 +0200
> Alexandre <amergnat@baylibre.com> wrote:
>
>> Hello Jonathan,
>>
>> On 4/7/19 12:20, Jonathan Cameron wrote:
>>> Hi Alexandre,
>>>
>>> So I have no problem with this as an IIO driver, but for devices that
>>> are somewhat 'on the edge' I always like to get a clear answer to the
>>> question: Why not input?
>>>
>>> I would also argue that, to actually be 'useful' we would typically need
>>> some representation of the 'mechanicals' that are providing the motion
>>> being measured. Looking at the datasheet this includes, rotating shafts
>>> (side or end on), disk edges and flat surface tracking (mouse like).
>>>
>>> That's easy enough to do with the iio in kernel consumer interface. These
>>> are similar to when we handle analog electronic front ends.
>>>
>>> I you can, please describe what it is being used for in your application
>>> as that may give us somewhere to start!
>>>
>>> + CC Dmitry and linux-input.
>> I developed this driver to detect the board movement which can't be
>> detected by accelerometer (very slow motion). I admit this use case can
>> be handled by an input, and I'm agree with you, PAT9125 driver could be
>> an input. But, like you said, this chip is able to track different kind
>> of motion, and additionally have an interrupt GPIO, so using it like
>> input limit the driver potential. This chip is designed to work in
>> industrial measurement or embedded systems, and the IIO API match with
>> these environments, so it's the best way to exploit the entire potential
>> of this chip.
>>
>> As I understand (from
>> https://www.kernel.org/doc/html/v4.12/input/event-codes.html#mice ),
>> mouse driver must report values when the device move. This feature
>> souldn't be mandatory for an optical tracker driver, specially for cases
>> where user prefers to use buffer or poll only when he need data.
>>
>>> If 1 or 2, I would suggest that you provide absolute position to
>>> Linux. So add the value to a software counter and provide that.
>>> 32 bits should be plenty of resolution for that.
>> I can't provide absolute position, only relative. Do you mean using
>> input driver to do that ? If not, how is built the position data?
> Sorry, I should have been clearer on this.
> I mean absolute relative to the start point. So on startup you assume
> absolute position is 0 and go from there. What I can't work out is
> if the device does internal tracking, or whether each time you read
> it effectively resets it's internal counters to 0 so the next measurement
> is relative to the previous one.
Each time you read that reset internal counters to 0.
>>> Silly question for you. What happens if you set the delta values to 0?
>>> Do we get an interrupt which is effectively data ready?
>>> If we do, you might want to think about a scheme where that is an option.
>>> As things currently stand we have a confusing interface where changing this
>>> threshold effects the buffered data output. That should only be the
>>> case if this interface is for a trigger, not an event.
>> I'm not sure to understand your question. Is it possible to read delta_x
>> and delta_y = 0 in special/corner case because internal value continue
>> to be updated after toggled motion_detect pin (used for IRQ) until
>> values registers are read and then motion_detect pin is released:
>>
>> * Chip move (i.e. +2 on X axis and 0 on Y axis)
>> * Motion_detect IRQ trigger and internal reg value is updated (i.e.
>> delta_x = 2 and delta_y = 0.
>> * GPIO IRQ handled but read_value isn't executed yet (timing reason)
>> * Chip move back to it origin point (i.e. -2 on X axis and 0 on Y axis)
>> * Motion_detect IRQ still low because it hasn't been reset by read
>> value and internal reg value is updated (i.e. delta_x = 0 and
>> delta_y = 0)
>> * Read_value is executed, we get delta values = 0.
> Again, I was unclear. Is it possible to set the device to interrupt
> every time it evaluates whether motion has occured? Not only when it
> concludes that there has been some motion. That would allow the interrupt
> to be used as a signal that the device has taken a measurement (data
> ready signal in other sensors).
>
I don't know, the datasheet don't describe the role of each bit in
registers and I don't found documentation which provide that. I had to
do research on example code to retrieve some bits, but got nothing on
motion detection pin configuration.
>>> If it is actually not possible to report the two channels separately
>>> then don't report them at all except via the buffered interface and
>>> set the available scan masks so that both are on.
>> I found a way to keep the consistency between delta x and delta y
>> (without losing data). The first part is to reset a value only when user
>> read it (also when it's buffered). The second part is to add the new
>> value to the old value. With these two mechanism, X and Y will always be
>> consistent:
>>
>> * as possible during a move.
>> * perfectly when move is finished.
> Ah. This adding old value to a new value point is what I was getting
> at (I think) with 'absolute' position above.
>
> In industrial control for example you have absolute position by using
> limit switches to set your baseline. Measurement devices are then
> capable of either reporting relative position, which is the movement
> since the last reading was taken, or 'absolute' position which is
> referenced to some known point. It was this form of absolute position
> that I was suggesting you use. If you use such a system without a
> limit switch it is normally called unreference motion. You can do
> it but then the 0 is where ever your device was at power on.
> For some systems it doesn't actually matter (conveyor belts for
> instance where the positions you care about are between things
> on the belt, not the position of the belt itself).
Ok, I decided to return delta between last read/buffering to stay closer
to the hardware mechanism and still coherent with "IIO_CHAN_INFO_RAW".
If user want absolute position, he can make an addition of all received
value in user space, and that allow him to reset/replace the initial
position when he want it.
> Thanks,
>
> Jonathan
>
>>
>> Regards,
>>
>> Alexandre
>>
Thanks for your comments,
Alexandre
^ permalink raw reply
* Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock
From: dmitry.torokhov @ 2019-04-23 8:49 UTC (permalink / raw)
To: Mukesh Ojha, Al Viro
Cc: linux-input, linux-kernel, Gaurav Kohli, Peter Hutterer,
Martin Kepplinger, Paul E. McKenney
In-Reply-To: <17f4a0be-ab04-8537-9197-32fbca807f3f@codeaurora.org>
On Tue, Apr 23, 2019 at 12:51:13PM +0530, Mukesh Ojha wrote:
> On 4/23/2019 8:58 AM, dmitry.torokhov@gmail.com wrote:
> > On Fri, Apr 19, 2019 at 02:13:48PM +0530, Mukesh Ojha wrote:
> > > On 4/19/2019 12:41 PM, dmitry.torokhov@gmail.com wrote:
> > > > On Fri, Apr 19, 2019 at 12:17:44PM +0530, Mukesh Ojha wrote:
> > > > > On 4/18/2019 11:55 AM, Mukesh Ojha wrote:
> > > > > > On 4/18/2019 7:13 AM, dmitry.torokhov@gmail.com wrote:
> > > > > > > > On 4/10/2019 1:29 PM, Mukesh Ojha wrote:
> > > > > > > > > uinput_destroy_device() gets called from two places. In one place,
> > > > > > > > > uinput_ioctl_handler() where it is protected under a lock
> > > > > > > > > udev->mutex but there is no protection on udev device from freeing
> > > > > > > > > inside uinput_release().
> > > > > > > uinput_release() should be called when last file handle to the uinput
> > > > > > > instance is being dropped, so there should be no other users and thus we
> > > > > > > can't be racing with anyone.
> > > > > > Lets say an example where i am creating input device quite frequently
> > > > > >
> > > > > > [ 97.836603] input: syz0 as /devices/virtual/input/input262
> > > > > > [ 97.845589] input: syz0 as /devices/virtual/input/input261
> > > > > > [ 97.849415] input: syz0 as /devices/virtual/input/input263
> > > > > > [ 97.856479] input: syz0 as /devices/virtual/input/input264
> > > > > > [ 97.936128] input: syz0 as /devices/virtual/input/input265
> > > > > >
> > > > > > e.g input265
> > > > > >
> > > > > > while input265 gets created [1] and handlers are getting registered on
> > > > > > that device*fput* gets called on
> > > > > > that device as user space got to know that input265 is created and its
> > > > > > reference is still 1(rare but possible).
> > > > Are you saying that there are 2 threads sharing the same file
> > > > descriptor, one issuing the registration ioctl while the other closing
> > > > the same fd?
> > > Dmitry,
> > >
> > > I don't have a the exact look inside the app here, but this looks like the
> > > same as it is able to do
> > > fput on the uinput device.
> > >
> > > FYI
> > > Syskaller app is running in userspace (which is for syscall fuzzing) on
> > > kernel which is enabled
> > > with various config fault injection, FAULT_INJECTION,FAIL_SLAB,
> > > FAIL_PAGEALLOC, KASAN etc.
> > Mukesh,
> >
> > We need to understand exactly the failure mode. I do not think that
> > introducing another mutex into uinput actually fixes the issue, as we do
> > not order mutex acquisition, so I think it is still possible for the
> > release function to acquire the mutex and run first, and then ioctl
> > would run with freed object.
>
> I have taken care this case from ioctl and release point of view.
>
> Even if the release gets called first it will make the
> file->private_data=NULL.
> and further call to ioctl will not be a problem as the check is already
> there.
Al, do we have any protections in VFS layer from userspace hanging onto
a file descriptor and calling ioctl() on it even as another thread
calls close() on the same fd?
Should the issue be solved by individual drivers, or more careful
accounting for currently running operations is needed at VFS layer?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH next 09/25] input: keyboard: Use dev_get_drvdata()
From: Dmitry Torokhov @ 2019-04-23 8:44 UTC (permalink / raw)
To: Kefeng Wang; +Cc: linux-kernel, Masahiro Yamada, linux-input
In-Reply-To: <20190423075020.173734-10-wangkefeng.wang@huawei.com>
Hi Kefeng,
On Tue, Apr 23, 2019 at 03:50:04PM +0800, Kefeng Wang wrote:
> Using dev_get_drvdata directly.
This assumes that generic device's driver data is exactly the same as platform
device's driver data. This is true today, but does not have to be true
tomorrow.
What is the benefit of violating the layering and accessing "parent"
object's abstraction directly? Performance impact must be negligible...
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH next 12/25] input: touchscreen: Use dev_get_drvdata()
From: Kefeng Wang @ 2019-04-23 7:50 UTC (permalink / raw)
To: linux-kernel
Cc: Masahiro Yamada, Kefeng Wang, Dmitry Torokhov, Shawn Guo,
Sascha Hauer, linux-input
In-Reply-To: <20190423075020.173734-1-wangkefeng.wang@huawei.com>
Using dev_get_drvdata directly.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: linux-input@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/input/touchscreen/imx6ul_tsc.c | 6 ++----
drivers/input/touchscreen/s3c2410_ts.c | 3 +--
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/imx6ul_tsc.c b/drivers/input/touchscreen/imx6ul_tsc.c
index c10fc594f94d..493322ed0e34 100644
--- a/drivers/input/touchscreen/imx6ul_tsc.c
+++ b/drivers/input/touchscreen/imx6ul_tsc.c
@@ -511,8 +511,7 @@ static int imx6ul_tsc_probe(struct platform_device *pdev)
static int __maybe_unused imx6ul_tsc_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct imx6ul_tsc *tsc = platform_get_drvdata(pdev);
+ struct imx6ul_tsc *tsc = dev_get_drvdata(dev);
struct input_dev *input_dev = tsc->input;
mutex_lock(&input_dev->mutex);
@@ -531,8 +530,7 @@ static int __maybe_unused imx6ul_tsc_suspend(struct device *dev)
static int __maybe_unused imx6ul_tsc_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct imx6ul_tsc *tsc = platform_get_drvdata(pdev);
+ struct imx6ul_tsc *tsc = dev_get_drvdata(dev);
struct input_dev *input_dev = tsc->input;
int retval = 0;
diff --git a/drivers/input/touchscreen/s3c2410_ts.c b/drivers/input/touchscreen/s3c2410_ts.c
index 1173890f6719..e5e9f6527ed8 100644
--- a/drivers/input/touchscreen/s3c2410_ts.c
+++ b/drivers/input/touchscreen/s3c2410_ts.c
@@ -396,8 +396,7 @@ static int s3c2410ts_suspend(struct device *dev)
static int s3c2410ts_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct s3c2410_ts_mach_info *info = dev_get_platdata(&pdev->dev);
+ struct s3c2410_ts_mach_info *info = dev_get_platdata(dev);
clk_enable(ts.clock);
enable_irq(ts.irq_tc);
--
2.20.1
^ permalink raw reply related
* [PATCH next 11/25] input: mouse: Use dev_get_drvdata()
From: Kefeng Wang @ 2019-04-23 7:50 UTC (permalink / raw)
To: linux-kernel; +Cc: Masahiro Yamada, Kefeng Wang, Dmitry Torokhov, linux-input
In-Reply-To: <20190423075020.173734-1-wangkefeng.wang@huawei.com>
Using dev_get_drvdata directly.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/input/mouse/navpoint.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/input/mouse/navpoint.c b/drivers/input/mouse/navpoint.c
index d6e8f58a1de3..3d83a79e14d9 100644
--- a/drivers/input/mouse/navpoint.c
+++ b/drivers/input/mouse/navpoint.c
@@ -320,8 +320,7 @@ static int navpoint_remove(struct platform_device *pdev)
static int __maybe_unused navpoint_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct navpoint *navpoint = platform_get_drvdata(pdev);
+ struct navpoint *navpoint = dev_get_drvdata(dev);
struct input_dev *input = navpoint->input;
mutex_lock(&input->mutex);
@@ -334,8 +333,7 @@ static int __maybe_unused navpoint_suspend(struct device *dev)
static int __maybe_unused navpoint_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct navpoint *navpoint = platform_get_drvdata(pdev);
+ struct navpoint *navpoint = dev_get_drvdata(dev);
struct input_dev *input = navpoint->input;
mutex_lock(&input->mutex);
--
2.20.1
^ permalink raw reply related
* [PATCH next 10/25] input: misc: Use dev_get_drvdata()
From: Kefeng Wang @ 2019-04-23 7:50 UTC (permalink / raw)
To: linux-kernel; +Cc: Masahiro Yamada, Kefeng Wang, Dmitry Torokhov, linux-input
In-Reply-To: <20190423075020.173734-1-wangkefeng.wang@huawei.com>
Using dev_get_drvdata directly.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/input/misc/max77693-haptic.c | 6 ++----
drivers/input/misc/max8925_onkey.c | 10 ++++------
drivers/input/misc/max8997_haptic.c | 3 +--
drivers/input/misc/msm-vibrator.c | 6 ++----
drivers/input/misc/palmas-pwrbutton.c | 6 ++----
drivers/input/misc/regulator-haptic.c | 6 ++----
drivers/input/misc/stpmic1_onkey.c | 6 ++----
drivers/input/misc/twl4030-vibra.c | 3 +--
drivers/input/misc/twl6040-vibra.c | 3 +--
9 files changed, 17 insertions(+), 32 deletions(-)
diff --git a/drivers/input/misc/max77693-haptic.c b/drivers/input/misc/max77693-haptic.c
index 46b0f48fbf49..8968fd48e95c 100644
--- a/drivers/input/misc/max77693-haptic.c
+++ b/drivers/input/misc/max77693-haptic.c
@@ -381,8 +381,7 @@ static int max77693_haptic_probe(struct platform_device *pdev)
static int __maybe_unused max77693_haptic_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct max77693_haptic *haptic = platform_get_drvdata(pdev);
+ struct max77693_haptic *haptic = dev_get_drvdata(dev);
if (haptic->enabled) {
max77693_haptic_disable(haptic);
@@ -394,8 +393,7 @@ static int __maybe_unused max77693_haptic_suspend(struct device *dev)
static int __maybe_unused max77693_haptic_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct max77693_haptic *haptic = platform_get_drvdata(pdev);
+ struct max77693_haptic *haptic = dev_get_drvdata(dev);
if (haptic->suspend_state) {
max77693_haptic_enable(haptic);
diff --git a/drivers/input/misc/max8925_onkey.c b/drivers/input/misc/max8925_onkey.c
index 7c49b8d23894..af0ba592a0b3 100644
--- a/drivers/input/misc/max8925_onkey.c
+++ b/drivers/input/misc/max8925_onkey.c
@@ -135,9 +135,8 @@ static int max8925_onkey_probe(struct platform_device *pdev)
static int __maybe_unused max8925_onkey_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct max8925_onkey_info *info = platform_get_drvdata(pdev);
- struct max8925_chip *chip = dev_get_drvdata(pdev->dev.parent);
+ struct max8925_onkey_info *info = dev_get_drvdata(dev);
+ struct max8925_chip *chip = dev_get_drvdata(dev->parent);
if (device_may_wakeup(dev)) {
chip->wakeup_flag |= 1 << info->irq[0];
@@ -149,9 +148,8 @@ static int __maybe_unused max8925_onkey_suspend(struct device *dev)
static int __maybe_unused max8925_onkey_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct max8925_onkey_info *info = platform_get_drvdata(pdev);
- struct max8925_chip *chip = dev_get_drvdata(pdev->dev.parent);
+ struct max8925_onkey_info *info = dev_get_drvdata(dev);
+ struct max8925_chip *chip = dev_get_drvdata(dev->parent);
if (device_may_wakeup(dev)) {
chip->wakeup_flag &= ~(1 << info->irq[0]);
diff --git a/drivers/input/misc/max8997_haptic.c b/drivers/input/misc/max8997_haptic.c
index 99bc762881d5..5ffb0ac68d50 100644
--- a/drivers/input/misc/max8997_haptic.c
+++ b/drivers/input/misc/max8997_haptic.c
@@ -388,8 +388,7 @@ static int max8997_haptic_remove(struct platform_device *pdev)
static int __maybe_unused max8997_haptic_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct max8997_haptic *chip = platform_get_drvdata(pdev);
+ struct max8997_haptic *chip = dev_get_drvdata(dev);
max8997_haptic_disable(chip);
diff --git a/drivers/input/misc/msm-vibrator.c b/drivers/input/misc/msm-vibrator.c
index b60f1aaee705..a28974bfb64e 100644
--- a/drivers/input/misc/msm-vibrator.c
+++ b/drivers/input/misc/msm-vibrator.c
@@ -234,8 +234,7 @@ static int msm_vibrator_probe(struct platform_device *pdev)
static int __maybe_unused msm_vibrator_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct msm_vibrator *vibrator = platform_get_drvdata(pdev);
+ struct msm_vibrator *vibrator = dev_get_drvdata(dev);
cancel_work_sync(&vibrator->worker);
@@ -247,8 +246,7 @@ static int __maybe_unused msm_vibrator_suspend(struct device *dev)
static int __maybe_unused msm_vibrator_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct msm_vibrator *vibrator = platform_get_drvdata(pdev);
+ struct msm_vibrator *vibrator = dev_get_drvdata(dev);
if (vibrator->enabled)
msm_vibrator_start(vibrator);
diff --git a/drivers/input/misc/palmas-pwrbutton.c b/drivers/input/misc/palmas-pwrbutton.c
index 1e1baed63929..27617868b292 100644
--- a/drivers/input/misc/palmas-pwrbutton.c
+++ b/drivers/input/misc/palmas-pwrbutton.c
@@ -270,8 +270,7 @@ static int palmas_pwron_remove(struct platform_device *pdev)
*/
static int __maybe_unused palmas_pwron_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct palmas_pwron *pwron = platform_get_drvdata(pdev);
+ struct palmas_pwron *pwron = dev_get_drvdata(dev);
cancel_delayed_work_sync(&pwron->input_work);
@@ -291,8 +290,7 @@ static int __maybe_unused palmas_pwron_suspend(struct device *dev)
*/
static int __maybe_unused palmas_pwron_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct palmas_pwron *pwron = platform_get_drvdata(pdev);
+ struct palmas_pwron *pwron = dev_get_drvdata(dev);
if (device_may_wakeup(dev))
disable_irq_wake(pwron->irq);
diff --git a/drivers/input/misc/regulator-haptic.c b/drivers/input/misc/regulator-haptic.c
index a1db1e5040dc..0b78a87f3192 100644
--- a/drivers/input/misc/regulator-haptic.c
+++ b/drivers/input/misc/regulator-haptic.c
@@ -206,8 +206,7 @@ static int regulator_haptic_probe(struct platform_device *pdev)
static int __maybe_unused regulator_haptic_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct regulator_haptic *haptic = platform_get_drvdata(pdev);
+ struct regulator_haptic *haptic = dev_get_drvdata(dev);
int error;
error = mutex_lock_interruptible(&haptic->mutex);
@@ -225,8 +224,7 @@ static int __maybe_unused regulator_haptic_suspend(struct device *dev)
static int __maybe_unused regulator_haptic_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct regulator_haptic *haptic = platform_get_drvdata(pdev);
+ struct regulator_haptic *haptic = dev_get_drvdata(dev);
unsigned int magnitude;
mutex_lock(&haptic->mutex);
diff --git a/drivers/input/misc/stpmic1_onkey.c b/drivers/input/misc/stpmic1_onkey.c
index 7b49c9997df7..ff4761540539 100644
--- a/drivers/input/misc/stpmic1_onkey.c
+++ b/drivers/input/misc/stpmic1_onkey.c
@@ -150,8 +150,7 @@ static int stpmic1_onkey_probe(struct platform_device *pdev)
static int __maybe_unused stpmic1_onkey_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct stpmic1_onkey *onkey = platform_get_drvdata(pdev);
+ struct stpmic1_onkey *onkey = dev_get_drvdata(dev);
if (device_may_wakeup(dev)) {
enable_irq_wake(onkey->irq_falling);
@@ -162,8 +161,7 @@ static int __maybe_unused stpmic1_onkey_suspend(struct device *dev)
static int __maybe_unused stpmic1_onkey_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct stpmic1_onkey *onkey = platform_get_drvdata(pdev);
+ struct stpmic1_onkey *onkey = dev_get_drvdata(dev);
if (device_may_wakeup(dev)) {
disable_irq_wake(onkey->irq_falling);
diff --git a/drivers/input/misc/twl4030-vibra.c b/drivers/input/misc/twl4030-vibra.c
index c37aea9ac272..7b9104c058ca 100644
--- a/drivers/input/misc/twl4030-vibra.c
+++ b/drivers/input/misc/twl4030-vibra.c
@@ -159,8 +159,7 @@ static void twl4030_vibra_close(struct input_dev *input)
/*** Module ***/
static int __maybe_unused twl4030_vibra_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct vibra_info *info = platform_get_drvdata(pdev);
+ struct vibra_info *info = dev_get_drvdata(dev);
if (info->enabled)
vibra_disable(info);
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c
index 15e0d352c4cc..c8539a4a98c6 100644
--- a/drivers/input/misc/twl6040-vibra.c
+++ b/drivers/input/misc/twl6040-vibra.c
@@ -226,8 +226,7 @@ static void twl6040_vibra_close(struct input_dev *input)
static int __maybe_unused twl6040_vibra_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct vibra_info *info = platform_get_drvdata(pdev);
+ struct vibra_info *info = dev_get_drvdata(dev);
cancel_work_sync(&info->play_work);
--
2.20.1
^ permalink raw reply related
* [PATCH next 09/25] input: keyboard: Use dev_get_drvdata()
From: Kefeng Wang @ 2019-04-23 7:50 UTC (permalink / raw)
To: linux-kernel; +Cc: Masahiro Yamada, Kefeng Wang, Dmitry Torokhov, linux-input
In-Reply-To: <20190423075020.173734-1-wangkefeng.wang@huawei.com>
Using dev_get_drvdata directly.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/input/keyboard/ep93xx_keypad.c | 10 ++++------
drivers/input/keyboard/gpio_keys.c | 6 ++----
drivers/input/keyboard/imx_keypad.c | 10 ++++------
drivers/input/keyboard/lpc32xx-keys.c | 6 ++----
drivers/input/keyboard/matrix_keypad.c | 10 ++++------
drivers/input/keyboard/omap4-keypad.c | 10 ++++------
drivers/input/keyboard/pmic8xxx-keypad.c | 6 ++----
drivers/input/keyboard/pxa27x_keypad.c | 10 ++++------
drivers/input/keyboard/samsung-keypad.c | 12 ++++--------
drivers/input/keyboard/snvs_pwrkey.c | 10 ++++------
drivers/input/keyboard/spear-keyboard.c | 10 ++++------
drivers/input/keyboard/st-keyscan.c | 6 ++----
drivers/input/keyboard/tegra-kbc.c | 10 ++++------
13 files changed, 44 insertions(+), 72 deletions(-)
diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c
index f77b295e0123..7584a03db4b3 100644
--- a/drivers/input/keyboard/ep93xx_keypad.c
+++ b/drivers/input/keyboard/ep93xx_keypad.c
@@ -185,8 +185,7 @@ static void ep93xx_keypad_close(struct input_dev *pdev)
#ifdef CONFIG_PM_SLEEP
static int ep93xx_keypad_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct ep93xx_keypad *keypad = platform_get_drvdata(pdev);
+ struct ep93xx_keypad *keypad = dev_get_drvdata(dev);
struct input_dev *input_dev = keypad->input_dev;
mutex_lock(&input_dev->mutex);
@@ -198,7 +197,7 @@ static int ep93xx_keypad_suspend(struct device *dev)
mutex_unlock(&input_dev->mutex);
- if (device_may_wakeup(&pdev->dev))
+ if (device_may_wakeup(dev))
enable_irq_wake(keypad->irq);
return 0;
@@ -206,11 +205,10 @@ static int ep93xx_keypad_suspend(struct device *dev)
static int ep93xx_keypad_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct ep93xx_keypad *keypad = platform_get_drvdata(pdev);
+ struct ep93xx_keypad *keypad = dev_get_drvdata(dev);
struct input_dev *input_dev = keypad->input_dev;
- if (device_may_wakeup(&pdev->dev))
+ if (device_may_wakeup(dev))
disable_irq_wake(keypad->irq);
mutex_lock(&input_dev->mutex);
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 6cd199e8a370..0b4cc4834b2f 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -293,8 +293,7 @@ static ssize_t gpio_keys_show_##name(struct device *dev, \
struct device_attribute *attr, \
char *buf) \
{ \
- struct platform_device *pdev = to_platform_device(dev); \
- struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev); \
+ struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev); \
\
return gpio_keys_attr_show_helper(ddata, buf, \
type, only_disabled); \
@@ -320,8 +319,7 @@ static ssize_t gpio_keys_store_##name(struct device *dev, \
const char *buf, \
size_t count) \
{ \
- struct platform_device *pdev = to_platform_device(dev); \
- struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev); \
+ struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev); \
ssize_t error; \
\
error = gpio_keys_attr_store_helper(ddata, buf, type); \
diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
index 539cb670de41..cb6daca57646 100644
--- a/drivers/input/keyboard/imx_keypad.c
+++ b/drivers/input/keyboard/imx_keypad.c
@@ -528,8 +528,7 @@ static int imx_keypad_probe(struct platform_device *pdev)
static int __maybe_unused imx_kbd_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct imx_keypad *kbd = platform_get_drvdata(pdev);
+ struct imx_keypad *kbd = dev_get_drvdata(dev);
struct input_dev *input_dev = kbd->input_dev;
/* imx kbd can wake up system even clock is disabled */
@@ -540,7 +539,7 @@ static int __maybe_unused imx_kbd_suspend(struct device *dev)
mutex_unlock(&input_dev->mutex);
- if (device_may_wakeup(&pdev->dev))
+ if (device_may_wakeup(dev))
enable_irq_wake(kbd->irq);
return 0;
@@ -548,12 +547,11 @@ static int __maybe_unused imx_kbd_suspend(struct device *dev)
static int __maybe_unused imx_kbd_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct imx_keypad *kbd = platform_get_drvdata(pdev);
+ struct imx_keypad *kbd = dev_get_drvdata(dev);
struct input_dev *input_dev = kbd->input_dev;
int ret = 0;
- if (device_may_wakeup(&pdev->dev))
+ if (device_may_wakeup(dev))
disable_irq_wake(kbd->irq);
mutex_lock(&input_dev->mutex);
diff --git a/drivers/input/keyboard/lpc32xx-keys.c b/drivers/input/keyboard/lpc32xx-keys.c
index 1dd57ac0e7a2..0831a6f2a9d4 100644
--- a/drivers/input/keyboard/lpc32xx-keys.c
+++ b/drivers/input/keyboard/lpc32xx-keys.c
@@ -279,8 +279,7 @@ static int lpc32xx_kscan_probe(struct platform_device *pdev)
#ifdef CONFIG_PM_SLEEP
static int lpc32xx_kscan_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev);
+ struct lpc32xx_kscan_drv *kscandat = dev_get_drvdata(dev);
struct input_dev *input = kscandat->input;
mutex_lock(&input->mutex);
@@ -297,8 +296,7 @@ static int lpc32xx_kscan_suspend(struct device *dev)
static int lpc32xx_kscan_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev);
+ struct lpc32xx_kscan_drv *kscandat = dev_get_drvdata(dev);
struct input_dev *input = kscandat->input;
int retval = 0;
diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index 3d1cb7bf5e35..a3477328d02b 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -276,12 +276,11 @@ static void matrix_keypad_disable_wakeup(struct matrix_keypad *keypad)
static int matrix_keypad_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct matrix_keypad *keypad = platform_get_drvdata(pdev);
+ struct matrix_keypad *keypad = dev_get_drvdata(dev);
matrix_keypad_stop(keypad->input_dev);
- if (device_may_wakeup(&pdev->dev))
+ if (device_may_wakeup(dev))
matrix_keypad_enable_wakeup(keypad);
return 0;
@@ -289,10 +288,9 @@ static int matrix_keypad_suspend(struct device *dev)
static int matrix_keypad_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct matrix_keypad *keypad = platform_get_drvdata(pdev);
+ struct matrix_keypad *keypad = dev_get_drvdata(dev);
- if (device_may_wakeup(&pdev->dev))
+ if (device_may_wakeup(dev))
matrix_keypad_disable_wakeup(keypad);
matrix_keypad_start(keypad->input_dev);
diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index 840e53732753..8c73a18daf8b 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -425,11 +425,10 @@ MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
#ifdef CONFIG_PM_SLEEP
static int omap4_keypad_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
+ struct omap4_keypad *keypad_data = dev_get_drvdata(dev);
int error;
- if (device_may_wakeup(&pdev->dev)) {
+ if (device_may_wakeup(dev)) {
error = enable_irq_wake(keypad_data->irq);
if (!error)
keypad_data->irq_wake_enabled = true;
@@ -440,10 +439,9 @@ static int omap4_keypad_suspend(struct device *dev)
static int omap4_keypad_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
+ struct omap4_keypad *keypad_data = dev_get_drvdata(dev);
- if (device_may_wakeup(&pdev->dev) && keypad_data->irq_wake_enabled) {
+ if (device_may_wakeup(dev) && keypad_data->irq_wake_enabled) {
disable_irq_wake(keypad_data->irq);
keypad_data->irq_wake_enabled = false;
}
diff --git a/drivers/input/keyboard/pmic8xxx-keypad.c b/drivers/input/keyboard/pmic8xxx-keypad.c
index 98b24ed18752..048a39321298 100644
--- a/drivers/input/keyboard/pmic8xxx-keypad.c
+++ b/drivers/input/keyboard/pmic8xxx-keypad.c
@@ -636,8 +636,7 @@ static int pmic8xxx_kp_probe(struct platform_device *pdev)
#ifdef CONFIG_PM_SLEEP
static int pmic8xxx_kp_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct pmic8xxx_kp *kp = platform_get_drvdata(pdev);
+ struct pmic8xxx_kp *kp = dev_get_drvdata(dev);
struct input_dev *input_dev = kp->input;
if (device_may_wakeup(dev)) {
@@ -656,8 +655,7 @@ static int pmic8xxx_kp_suspend(struct device *dev)
static int pmic8xxx_kp_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct pmic8xxx_kp *kp = platform_get_drvdata(pdev);
+ struct pmic8xxx_kp *kp = dev_get_drvdata(dev);
struct input_dev *input_dev = kp->input;
if (device_may_wakeup(dev)) {
diff --git a/drivers/input/keyboard/pxa27x_keypad.c b/drivers/input/keyboard/pxa27x_keypad.c
index d0bdaeadf86d..1f54a3162124 100644
--- a/drivers/input/keyboard/pxa27x_keypad.c
+++ b/drivers/input/keyboard/pxa27x_keypad.c
@@ -666,14 +666,13 @@ static void pxa27x_keypad_close(struct input_dev *dev)
#ifdef CONFIG_PM_SLEEP
static int pxa27x_keypad_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct pxa27x_keypad *keypad = platform_get_drvdata(pdev);
+ struct pxa27x_keypad *keypad = dev_get_drvdata(dev);
/*
* If the keypad is used a wake up source, clock can not be disabled.
* Or it can not detect the key pressing.
*/
- if (device_may_wakeup(&pdev->dev))
+ if (device_may_wakeup(dev))
enable_irq_wake(keypad->irq);
else
clk_disable_unprepare(keypad->clk);
@@ -683,8 +682,7 @@ static int pxa27x_keypad_suspend(struct device *dev)
static int pxa27x_keypad_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct pxa27x_keypad *keypad = platform_get_drvdata(pdev);
+ struct pxa27x_keypad *keypad = dev_get_drvdata(dev);
struct input_dev *input_dev = keypad->input_dev;
int ret = 0;
@@ -692,7 +690,7 @@ static int pxa27x_keypad_resume(struct device *dev)
* If the keypad is used as wake up source, the clock is not turned
* off. So do not need configure it again.
*/
- if (device_may_wakeup(&pdev->dev)) {
+ if (device_may_wakeup(dev)) {
disable_irq_wake(keypad->irq);
} else {
mutex_lock(&input_dev->mutex);
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index 1fe1aa2adf85..aadca4261b24 100644
--- a/drivers/input/keyboard/samsung-keypad.c
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -466,8 +466,7 @@ static int samsung_keypad_remove(struct platform_device *pdev)
#ifdef CONFIG_PM
static int samsung_keypad_runtime_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct samsung_keypad *keypad = platform_get_drvdata(pdev);
+ struct samsung_keypad *keypad = dev_get_drvdata(dev);
unsigned int val;
int error;
@@ -490,8 +489,7 @@ static int samsung_keypad_runtime_suspend(struct device *dev)
static int samsung_keypad_runtime_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct samsung_keypad *keypad = platform_get_drvdata(pdev);
+ struct samsung_keypad *keypad = dev_get_drvdata(dev);
unsigned int val;
if (keypad->stopped)
@@ -535,8 +533,7 @@ static void samsung_keypad_toggle_wakeup(struct samsung_keypad *keypad,
static int samsung_keypad_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct samsung_keypad *keypad = platform_get_drvdata(pdev);
+ struct samsung_keypad *keypad = dev_get_drvdata(dev);
struct input_dev *input_dev = keypad->input_dev;
mutex_lock(&input_dev->mutex);
@@ -553,8 +550,7 @@ static int samsung_keypad_suspend(struct device *dev)
static int samsung_keypad_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct samsung_keypad *keypad = platform_get_drvdata(pdev);
+ struct samsung_keypad *keypad = dev_get_drvdata(dev);
struct input_dev *input_dev = keypad->input_dev;
mutex_lock(&input_dev->mutex);
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 4c67cf30a5d9..ba40dc3025ce 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -173,10 +173,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
static int __maybe_unused imx_snvs_pwrkey_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
+ struct pwrkey_drv_data *pdata = dev_get_drvdata(dev);
- if (device_may_wakeup(&pdev->dev))
+ if (device_may_wakeup(dev))
enable_irq_wake(pdata->irq);
return 0;
@@ -184,10 +183,9 @@ static int __maybe_unused imx_snvs_pwrkey_suspend(struct device *dev)
static int __maybe_unused imx_snvs_pwrkey_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
+ struct pwrkey_drv_data *pdata = dev_get_drvdata(dev);
- if (device_may_wakeup(&pdev->dev))
+ if (device_may_wakeup(dev))
disable_irq_wake(pdata->irq);
return 0;
diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 7d25fa338ab4..a0276a3376d2 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c
@@ -288,8 +288,7 @@ static int spear_kbd_remove(struct platform_device *pdev)
static int __maybe_unused spear_kbd_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct spear_kbd *kbd = platform_get_drvdata(pdev);
+ struct spear_kbd *kbd = dev_get_drvdata(dev);
struct input_dev *input_dev = kbd->input;
unsigned int rate = 0, mode_ctl_reg, val;
@@ -300,7 +299,7 @@ static int __maybe_unused spear_kbd_suspend(struct device *dev)
mode_ctl_reg = readl_relaxed(kbd->io_base + MODE_CTL_REG);
- if (device_may_wakeup(&pdev->dev)) {
+ if (device_may_wakeup(dev)) {
if (!enable_irq_wake(kbd->irq))
kbd->irq_wake_enabled = true;
@@ -341,13 +340,12 @@ static int __maybe_unused spear_kbd_suspend(struct device *dev)
static int __maybe_unused spear_kbd_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct spear_kbd *kbd = platform_get_drvdata(pdev);
+ struct spear_kbd *kbd = dev_get_drvdata(dev);
struct input_dev *input_dev = kbd->input;
mutex_lock(&input_dev->mutex);
- if (device_may_wakeup(&pdev->dev)) {
+ if (device_may_wakeup(dev)) {
if (kbd->irq_wake_enabled) {
kbd->irq_wake_enabled = false;
disable_irq_wake(kbd->irq);
diff --git a/drivers/input/keyboard/st-keyscan.c b/drivers/input/keyboard/st-keyscan.c
index 3b85631fde91..454481651ace 100644
--- a/drivers/input/keyboard/st-keyscan.c
+++ b/drivers/input/keyboard/st-keyscan.c
@@ -218,8 +218,7 @@ static int keyscan_probe(struct platform_device *pdev)
#ifdef CONFIG_PM_SLEEP
static int keyscan_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct st_keyscan *keypad = platform_get_drvdata(pdev);
+ struct st_keyscan *keypad = dev_get_drvdata(dev);
struct input_dev *input = keypad->input_dev;
mutex_lock(&input->mutex);
@@ -235,8 +234,7 @@ static int keyscan_suspend(struct device *dev)
static int keyscan_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct st_keyscan *keypad = platform_get_drvdata(pdev);
+ struct st_keyscan *keypad = dev_get_drvdata(dev);
struct input_dev *input = keypad->input_dev;
int retval = 0;
diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index 875205f445b5..861bfcbd817d 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -744,11 +744,10 @@ static void tegra_kbc_set_keypress_interrupt(struct tegra_kbc *kbc, bool enable)
static int tegra_kbc_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct tegra_kbc *kbc = platform_get_drvdata(pdev);
+ struct tegra_kbc *kbc = dev_get_drvdata(dev);
mutex_lock(&kbc->idev->mutex);
- if (device_may_wakeup(&pdev->dev)) {
+ if (device_may_wakeup(dev)) {
disable_irq(kbc->irq);
del_timer_sync(&kbc->timer);
tegra_kbc_set_fifo_interrupt(kbc, false);
@@ -781,12 +780,11 @@ static int tegra_kbc_suspend(struct device *dev)
static int tegra_kbc_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct tegra_kbc *kbc = platform_get_drvdata(pdev);
+ struct tegra_kbc *kbc = dev_get_drvdata(dev);
int err = 0;
mutex_lock(&kbc->idev->mutex);
- if (device_may_wakeup(&pdev->dev)) {
+ if (device_may_wakeup(dev)) {
disable_irq_wake(kbc->irq);
tegra_kbc_setup_wakekeys(kbc, false);
/* We will use fifo interrupts for key detection. */
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock
From: dmitry.torokhov @ 2019-04-23 3:28 UTC (permalink / raw)
To: Mukesh Ojha
Cc: linux-input, linux-kernel, Gaurav Kohli, Peter Hutterer,
Martin Kepplinger, Paul E. McKenney
In-Reply-To: <f3cd46fb-2bb1-0422-b2be-3f7625ec9c4e@codeaurora.org>
On Fri, Apr 19, 2019 at 02:13:48PM +0530, Mukesh Ojha wrote:
>
> On 4/19/2019 12:41 PM, dmitry.torokhov@gmail.com wrote:
> > Hi Mukesh,
> >
> > On Fri, Apr 19, 2019 at 12:17:44PM +0530, Mukesh Ojha wrote:
> > > For some reason my last mail did not get delivered, sending it again.
> > >
> > >
> > > On 4/18/2019 11:55 AM, Mukesh Ojha wrote:
> > > >
> > > > On 4/18/2019 7:13 AM, dmitry.torokhov@gmail.com wrote:
> > > > > Hi Mukesh,
> > > > >
> > > > > On Mon, Apr 15, 2019 at 03:35:51PM +0530, Mukesh Ojha wrote:
> > > > > > Hi Dmitry,
> > > > > >
> > > > > > Can you please have a look at this patch ? as this seems to reproducing
> > > > > > quite frequently
> > > > > >
> > > > > > Thanks,
> > > > > > Mukesh
> > > > > >
> > > > > > On 4/10/2019 1:29 PM, Mukesh Ojha wrote:
> > > > > > > uinput_destroy_device() gets called from two places. In one place,
> > > > > > > uinput_ioctl_handler() where it is protected under a lock
> > > > > > > udev->mutex but there is no protection on udev device from freeing
> > > > > > > inside uinput_release().
> > > > > uinput_release() should be called when last file handle to the uinput
> > > > > instance is being dropped, so there should be no other users and thus we
> > > > > can't be racing with anyone.
> > > > Lets say an example where i am creating input device quite frequently
> > > >
> > > > [ 97.836603] input: syz0 as /devices/virtual/input/input262
> > > > [ 97.845589] input: syz0 as /devices/virtual/input/input261
> > > > [ 97.849415] input: syz0 as /devices/virtual/input/input263
> > > > [ 97.856479] input: syz0 as /devices/virtual/input/input264
> > > > [ 97.936128] input: syz0 as /devices/virtual/input/input265
> > > >
> > > > e.g input265
> > > >
> > > > while input265 gets created [1] and handlers are getting registered on
> > > > that device*fput* gets called on
> > > > that device as user space got to know that input265 is created and its
> > > > reference is still 1(rare but possible).
> > Are you saying that there are 2 threads sharing the same file
> > descriptor, one issuing the registration ioctl while the other closing
> > the same fd?
>
> Dmitry,
>
> I don't have a the exact look inside the app here, but this looks like the
> same as it is able to do
> fput on the uinput device.
>
> FYI
> Syskaller app is running in userspace (which is for syscall fuzzing) on
> kernel which is enabled
> with various config fault injection, FAULT_INJECTION,FAIL_SLAB,
> FAIL_PAGEALLOC, KASAN etc.
Mukesh,
We need to understand exactly the failure mode. I do not think that
introducing another mutex into uinput actually fixes the issue, as we do
not order mutex acquisition, so I think it is still possible for the
release function to acquire the mutex and run first, and then ioctl
would run with freed object.
My guess that this needs to be fixed in VFS layer.
Thanks.
--
Dmitry
^ permalink raw reply
* RE: [PATCH] drivers: hid: Add a module description line
From: Joseph Salisbury @ 2019-04-23 3:25 UTC (permalink / raw)
To: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, jikos@kernel.org,
benjamin.tissoires@redhat.com
Cc: linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <SN6PR2101MB092558880B017881591E41E2D7230@SN6PR2101MB0925.namprd21.prod.outlook.com>
Thanks for the feedback. I'll probably update each patch subject with the module names as well. I'll send a v2 for all three.
Thanks,
Joe
-----Original Message-----
From: Michael Kelley <mikelley@microsoft.com>
Sent: Monday, April 22, 2019 11:16 PM
To: Joseph Salisbury <Joseph.Salisbury@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; sashal@kernel.org; jikos@kernel.org; benjamin.tissoires@redhat.com
Cc: linux-hyperv@vger.kernel.org; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: RE: [PATCH] drivers: hid: Add a module description line
From: Joseph Salisbury <Joseph.Salisbury@microsoft.com> Sent: Monday, April 22, 2019 2:31 PM
>
> Signed-off-by: Joseph Salisbury <joseph.salisbury@microsoft.com>
> ---
> drivers/hid/hid-hyperv.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c index
> 704049e62d58..d3311d714d35 100644
> --- a/drivers/hid/hid-hyperv.c
> +++ b/drivers/hid/hid-hyperv.c
> @@ -614,5 +614,7 @@ static void __exit mousevsc_exit(void) }
>
> MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Microsoft Hyper-V Synthetic HID Driver");
> +
> module_init(mousevsc_init);
> module_exit(mousevsc_exit);
> --
> 2.17.1
Even though it will likely be redundant with the commit title, there
probably needs to be a short commit message. (And also with the
other two similar patches.)
Michael
^ permalink raw reply
* Re: [PATCH 2/4] ARM: ep93xx: keypad: stop using mach/platform.h
From: Dmitry Torokhov @ 2019-04-23 3:22 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Hartley Sweeten, Alexander Sverdlin, Linus Walleij, Stefan Agner,
Enric Balletbo i Serra, Guenter Roeck, linux-input, linux-kernel
In-Reply-To: <20190415192734.935387-2-arnd@arndb.de>
On Mon, Apr 15, 2019 at 09:25:24PM +0200, Arnd Bergmann wrote:
> We can communicate the clock rate using platform data rather than setting
> a flag to use a particular value in the driver, which is cleaner and
> avoids the dependency.
>
> No platform in the kernel currently defines the ep93xx keypad device
> structure, so this is a rather pointless excercise. Any out of tree
> users are probably dead now, but if not, they have to change their
> platform code to match the new platform_data structure.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Please feel free to merge with the rest of the patches.
> ---
> drivers/input/keyboard/Kconfig | 2 +-
> drivers/input/keyboard/ep93xx_keypad.c | 5 +----
> include/linux/platform_data/keypad-ep93xx.h | 4 ++--
> 3 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index a878351f1643..b373f3274542 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -194,7 +194,7 @@ config KEYBOARD_LKKBD
>
> config KEYBOARD_EP93XX
> tristate "EP93xx Matrix Keypad support"
> - depends on ARCH_EP93XX
> + depends on ARCH_EP93XX || COMPILE_TEST
> select INPUT_MATRIXKMAP
> help
> Say Y here to enable the matrix keypad on the Cirrus EP93XX.
> diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c
> index f77b295e0123..71472f6257c0 100644
> --- a/drivers/input/keyboard/ep93xx_keypad.c
> +++ b/drivers/input/keyboard/ep93xx_keypad.c
> @@ -137,10 +137,7 @@ static void ep93xx_keypad_config(struct ep93xx_keypad *keypad)
> struct ep93xx_keypad_platform_data *pdata = keypad->pdata;
> unsigned int val = 0;
>
> - if (pdata->flags & EP93XX_KEYPAD_KDIV)
> - clk_set_rate(keypad->clk, EP93XX_KEYTCHCLK_DIV4);
> - else
> - clk_set_rate(keypad->clk, EP93XX_KEYTCHCLK_DIV16);
> + clk_set_rate(keypad->clk, pdata->clk_rate);
>
> if (pdata->flags & EP93XX_KEYPAD_DISABLE_3_KEY)
> val |= KEY_INIT_DIS3KY;
> diff --git a/include/linux/platform_data/keypad-ep93xx.h b/include/linux/platform_data/keypad-ep93xx.h
> index 0e36818e3680..3054fced8509 100644
> --- a/include/linux/platform_data/keypad-ep93xx.h
> +++ b/include/linux/platform_data/keypad-ep93xx.h
> @@ -9,8 +9,7 @@ struct matrix_keymap_data;
> #define EP93XX_KEYPAD_DIAG_MODE (1<<1) /* diagnostic mode */
> #define EP93XX_KEYPAD_BACK_DRIVE (1<<2) /* back driving mode */
> #define EP93XX_KEYPAD_TEST_MODE (1<<3) /* scan only column 0 */
> -#define EP93XX_KEYPAD_KDIV (1<<4) /* 1/4 clock or 1/16 clock */
> -#define EP93XX_KEYPAD_AUTOREPEAT (1<<5) /* enable key autorepeat */
> +#define EP93XX_KEYPAD_AUTOREPEAT (1<<4) /* enable key autorepeat */
>
> /**
> * struct ep93xx_keypad_platform_data - platform specific device structure
> @@ -24,6 +23,7 @@ struct ep93xx_keypad_platform_data {
> unsigned int debounce;
> unsigned int prescale;
> unsigned int flags;
> + unsigned int clk_rate;
> };
>
> #define EP93XX_MATRIX_ROWS (8)
> --
> 2.20.0
>
--
Dmitry
^ permalink raw reply
* RE: [PATCH] drivers: hid: Add a module description line
From: Michael Kelley @ 2019-04-23 3:15 UTC (permalink / raw)
To: Joseph Salisbury, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, jikos@kernel.org,
benjamin.tissoires@redhat.com
Cc: linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <324016691a62a13ce46c9ccd35c7e492bf609fd6.1555967348.git.joseph.salisbury@microsoft.com>
From: Joseph Salisbury <Joseph.Salisbury@microsoft.com> Sent: Monday, April 22, 2019 2:31 PM
>
> Signed-off-by: Joseph Salisbury <joseph.salisbury@microsoft.com>
> ---
> drivers/hid/hid-hyperv.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
> index 704049e62d58..d3311d714d35 100644
> --- a/drivers/hid/hid-hyperv.c
> +++ b/drivers/hid/hid-hyperv.c
> @@ -614,5 +614,7 @@ static void __exit mousevsc_exit(void)
> }
>
> MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Microsoft Hyper-V Synthetic HID Driver");
> +
> module_init(mousevsc_init);
> module_exit(mousevsc_exit);
> --
> 2.17.1
Even though it will likely be redundant with the commit title, there
probably needs to be a short commit message. (And also with the
other two similar patches.)
Michael
^ permalink raw reply
* [PATCH] drivers: input: serio: Add a module desription
From: Joseph Salisbury @ 2019-04-22 21:38 UTC (permalink / raw)
To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Michael Kelley,
sashal@kernel.org, dmitry.torokhov@gmail.com
Cc: linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
Signed-off-by: Joseph Salisbury <joseph.salisbury@microsoft.com>
---
drivers/input/serio/hyperv-keyboard.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
index a8b9be3e28db..7935e52b5435 100644
--- a/drivers/input/serio/hyperv-keyboard.c
+++ b/drivers/input/serio/hyperv-keyboard.c
@@ -440,5 +440,7 @@ static void __exit hv_kbd_exit(void)
}
MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Microsoft Hyper-V Synthetic Keyboard Driver");
+
module_init(hv_kbd_init);
module_exit(hv_kbd_exit);
--
2.17.1
^ permalink raw reply related
* [PATCH] drivers: hid: Add a module description line
From: Joseph Salisbury @ 2019-04-22 21:31 UTC (permalink / raw)
To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Michael Kelley,
sashal@kernel.org, jikos@kernel.org,
benjamin.tissoires@redhat.com
Cc: linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
Signed-off-by: Joseph Salisbury <joseph.salisbury@microsoft.com>
---
drivers/hid/hid-hyperv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
index 704049e62d58..d3311d714d35 100644
--- a/drivers/hid/hid-hyperv.c
+++ b/drivers/hid/hid-hyperv.c
@@ -614,5 +614,7 @@ static void __exit mousevsc_exit(void)
}
MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Microsoft Hyper-V Synthetic HID Driver");
+
module_init(mousevsc_init);
module_exit(mousevsc_exit);
--
2.17.1
^ permalink raw reply related
* Re: [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface
From: Jonathan Cameron @ 2019-04-22 14:20 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Dmitry Torokhov, Eric Piel, linux-input, letux-kernel, kernel,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-kernel, linux-iio
In-Reply-To: <CD6219BE-61FF-4C38-9532-054C60A77F89@goldelico.com>
On Mon, 15 Apr 2019 23:04:15 +0200
H. Nikolaus Schaller <hns@goldelico.com> wrote:
> Hi Jonathan,
> we seem to have opposite goals and therefore quite different ideas what the right
> solution is.
Yup :) Yikes this email has gotten complex. Hopefully I haven't
lost track of the various points below.
>
> I come from user-space and the input side, find that input can handle abstract "acceleration input",
> and some older chip drivers even provide that as their only interface. Therefore I want
> to provide "acceleration input" in these cases where only iio capable drivers exist by
> using the existing in-kernel-iio infrastructure. Nothing more.
>
> You seem to come from the iio architecture and want to extend it to other APIs as
> general as possible and input is just one of several of them.
Yes, my target is to produce a subsystem that meets many (all would be nice)
requirements, including yours. Whilst I'm happy to debate this for ever, I'm not
sure we are making any substantial progress. As you mention below we
probably need to 'see the code' to drive the discussion forwards.
>
> Different goals usually lead to different solution architectures.
Indeed, but in this case we have your proposal which is a subset of what
I am suggesting. One architecture can fulfil both requirements.
I'll leave it for the other thread, but Bastien has raised the case
(that I'd forgotten) that there already userspace stacks that are
capable of happily taking in both IIO and Input devices. The confusion
here is they will now discover 'both' without the existing userspace
knowing that they are the same device. We need to be very careful not
to break those userspace programs.
So this is an illustration of why the simplistic case doesn't work
'now'.
>
> > Am 14.04.2019 um 13:40 schrieb Jonathan Cameron <jic23@kernel.org>:
> >
> > On Mon, 8 Apr 2019 15:15:56 +0200
> > H. Nikolaus Schaller <hns@goldelico.com> wrote:
> >
> >> Hi Jonathan,
> >>
> >> I still do not fully understand what is worrying you here.
> >
> >>
> >> Do you worry about functionality, flexibility or resources or something else?
> >
> > Two main things:
> > 1) Lack of generality of the approach.
> > This is a single use trick for input devices. Why does it make sense for
> > input devices?
>
> No, it is not a trick...
Bad choice of words. Sorry about that.
Any time we register a device as two types of device it is less than ideal.
If we had the true separation of IIO front end and back end then it
would be perfectly acceptable to 'only' have an input front end for
a given device. That choice would still, in this sort of usecase,
have to be made from userspace. It's policy, not design. If there are reasons
a particular device 'is input' then that mapping should be in DT or similar.
It's no different from knowing an ADC channel is wired to an analog
temperature sensor. For example, you could build a joystick with
an accelerometer in the stick - then the usecase would be obvious!
Hence I would also argue that any dynamic interface should also support
a static path (DT or equivalent) for the cases where is really a
physical characteristic of the system! Perhaps the Sony parts
fall into this category as well.
For a bit of historical background, there was a concerted effort
to produced a userspace stack for IIO for android.
https://01.org/android-iio-sensors-hal
Unfortunately I think it died as a result of other moves in Intel on
one of their periodic shifts in direction.
>
> Why does it make sense for input devices?
These are not just 'input' devices. They are accelerometers. One usecase
is to use them for input. The exact same physical device is used for games
input uses and counting steps for example (actually a lot wider cases than
that, but this one is common in android devices).
Keep that in mind at all times. There are lots of usecases.
So we need a solution that does not result in problems for those
usecases. We are not writing a subsytem targetting android use of
accelerometers. We are writing a subsystem addressing as many usecases
as we can of those devices.
Note that the original reason for IIO was to generalize a whole set of
proposed individual subsystems targeting particular sensor types. So
that is our focus - solutions that work for everyone. This isn't
totally dissimilar from those discussions - at the time I wanted
to do a small focused ALS subsystem and got a resounding "no" from
Linus. Generality matters a lot for the long term.
>
> a) because there is no alternative for accelerometer input devices and there are some
> (older) accelerometer chips which only present themselves as input devices as soon
> as the driver is compiled and the chip is found.
Actually that is not accurate. The vast majority of those older devices
that have had any attempt at mainlining are in IIO. AFAIK no accelerometer
driver has been merged to mainline input for many years. This is because,
amongst others, Dmitry has been of the view they didn't belong there for
a very long time.
>
> b) because input events define ACCEL input devices. But no Temperature or Voltage
> input or generic ADC input.So there is no generalization of input devices beyond
> keyboard, mouse, joystick, accelerometer and some others.
That's not totally inaccurate, but the distinction in the other sensor cases
is that there is a clear 'additional' element that we can map in devicetree
which relates the IIO sensor channel to the input device.
Doesn't matter for the point of view of this discussion though.
>
> > There are lots of other in kernel users and potential
> > ones in the future.
>
> This bridge addition does not exclude any (additional) in-kernel use.
No but it creates several problems:
1. Two ways to do the same thing.
2. Two sets of code to maintain.
3. Confusion over what is the best way of doing it.
4. The known issues with multiple consumers (note my solution has
that problem as well!)
My job here is to maintain the code, which is why I push back on something
that makes that job harder.
When the next usecases comes along and someone says they want to map
all ADC channels to hwmon channels because that is the subystem that
they expect to measure voltages in, then I don't have a good argument
to stop them doing the same thing you have.
As a side note we have in the past had input drivers for gyroscopes and
magnetometers. Why are accelerometers special?
I really don't see why we should treat accelerometers differently
As this discussion runs on, I am increasingly convinced that there *must*
be a userspace policy control over whether an input device is
instantiated for a given accelerometer. Once that is the
case then I cannot see a reason to treat it any differently from
other channel types.
>
> > The ability to register additional IIO consumers like
> > this is useful, lets make it useful to everyone.
>
> But not everyone and every iio device can and should be mappable to the "input" abstraction.
> This does not make any sense to me.
Absolutely. It should not be. I clearly didn't explain this well.
It should be mapped to a consumer.
One type of consumer is iio-input, another is iio-hmwon etc.
>
> For example, does it make sense to map a temperature sensor to accelerometer input? Or an
> accelerometer to hwmon? This seems to be possible of your generalization unless I am missing something here.
> If it is, it ignores that iio sensors are already grouped by what physical property they do measure.
If people want to map crazy channels to crazy sensor outputs, why stop them?
(at this level of the interface).
This is a policy question for a userspace script. Particular consumer drivers
could of course perform sanity checking and refuse to do anything if they
cannot sensibly use the channels.
Yes, the interface has this flexibility. Which is a good thing! Take the example
of the gyroscope as input I used above. If we want to add that support
in future to your driver (I have no idea if it actually makes sense)
then we can - without having to change the interface.
>
> >
> > 2) To much generality of the specific usecase. I don't want to put an Input
> > interface on accelerometers where it makes no sense.
>
> I think you can just ignore the input interfaces in that case, if it was created.
Bastien raised a case where this isn't true.
>
> > The rule of it has
> > 2-3 axis so it must make sense isn't good enough to my mind. How
> > does userspace know which accelerometer to use (more and more devices have
> > multiple?)
>
> In our approach user-space can make it known by udev rules based on /dev/input/event*
> (not on iio but the input created for accelerometers). I think I mentioned that. This
> comes for free for any device registering as input. So it is no additional code.
Sorry, I'm lost. What in there tells you to use 'this' interface rather than one
of the other N that were registered? I'm not sure what information you
have available there.
>
> > You could do something like looking at the location info from
> > DT / ACPI in your driver and pick the 'best' but that's policy. Should be
> > in userspace. Sure you can just use the right input driver, but the moment
> > we do that, we need aware userspace, if that's the case why not make it
> > aware from the start.
> >
> > Believe me I've been round this one a good few times and thought about it
> > a lot.
>
> That is fine and why we should discuss all the different aspects we have collected.
>
> > I'll take a lot of convincing that this isn't a problem that
> > should be pushed into userspace.
> >
> >>
> >> I think having them mapped always does not need much resources (except a handful of bytes
> >> in memory and some µs during probing) unless the event device is opened and really used.
> >> Only then it starts e.g. I2C traffic to read the sensors.
> >
> > The bytes don't really mater.
>
> Ok, good to know.
>
> > The userspace ABI additions do.
>
> There are only new /dev/input/event devices with well defined ABI. This approach does
> not invent anything new here, hence there are no ABI additions which we can break.
But it does - we aren't talking general ABI, but ABI on specific
devices. Sure, Android doesn't care - though you'd be amazed how much
individual android device developers will because we just added another
pile of tests to their CI.
An industrial sensor platform absolutely does. They have to validate
those interfaces. They can't just ignore them because they feel like
it because who knows if some future user will use them?
For another case, see Bastien's reply to the later thread.
Instantiating interfaces has testing costs, even when the are standard
interfaces.
>
> >
> >>
> >> So it is just some unused file sitting around in /dev. Or 2 or could be even 100.
> >> For devices which have no iio accelerometers configured, there will be no /dev/input
> >> file. So we are discussing the rare case of devices with more than one or two accelerometers.
> >
> > Well they aren't exactly rare in IIO using systems ;)
>
> This is another thing where our experiences differ. What specific devices are you thinking
> of? I am focussed on handhelds where the accelerometer (or two) is a way to do GUI input
> depending on device orientation in space.
Again, you are introducing this interface for everyone. Including lots of
'interesting' usecases.
I have worked with sensor platforms with accelerometers of different parts of humans,
We have people do bridge vibration measurement, flying UAVs and tracking the motion
of trucks.
Most are not huge numbers of accelerometers per node but don't rule out the
possibility. It's normally limited by length of cables rather than anything
else so you used multiple nodes after a while each with their own set of sensors.
There are lots of plaforms out there that use multiple accelerometers in more
or less the same place to do very high dynamic range measurement (without losing
precision when things are nearly still).
Anyhow, it's not a particularly important point anyway!
> >
> >>
> >> Now, on every system there are many interfaces and files that are not used because it makes
> >> no sense to look at them. If I check on one of my systems, I find for example a lot of
> >> /dev/tty and only a very small portion is used and generic distros have no issue with it.
> >>
> >> There is even /dev/iio:device0 to /dev/iio:device5 representing the raw iio devices.
> >> Not all of them are actively used, but they are simply there and can be scanned for.
> >
> > Agreed, in the ideal case we wouldn't have had that either, but we are
> > stuck with it. The long term plan is to allow use of IIO backends without the
> > front end being there at all. Lots of SoC ADC users would prefer this. We are
> > stuck with the legacy intertwining fo the front end and back end of IIO so
> > this isn't as easy to do as I would like.
>
> Ah, ok. I think it is a similar discussion of hiding the serdev /dev/tty* if it is
> used for accessing an embedded GPS or Bluetooth chip, for example.
>
> But is this needed? I think it is not a problem if there are multiple consumers for
> the same iio channel. Some in-kernel, some through /dev/iio:device* and maybe some
> through /dev/input (which boils down to in-kernel).
There are quite a few complexities around multiple consumers that we really haven't
solved. Right now the cases that work are very much restricted. I'd love
to tidy some of these up, but never enough time and all that.
It's not that relevant here, but in short a few of the issues are:
1) Interference over control settings. - Two consumers need different filter
settings/ sampling frequency / range. How do we negotiate the choice and
communicate it to the other consumers. More complex questions such
mediating choices of triggers.
2) One driver is doing polled reads, the other is doing interrupt driven.
Most drivers prevent this combination because the polled reads can lead
to unlimited delays on the interrupt driven path and hence break it.
The main driver for this separation was to present only the 'right' interface
to reduced people's validation costs etc. People really do want to have the
option to strip back the userspace inteface. Obviously these are the rare
people who would disable your config option, but the point of this was
that we actually would like to make even the IIO interface optional as
well but have a fair way to go before we can.
>
> >
> >>
> >> So I do not see a resource problem if every accelerometer /dev/iio:device* gets
> >> some companion /dev/input/event* for being used on demand - but only if this bridge
> >> is configured at all.
> >
> > That argument does not apply. If we add a config option, distros will enable it.
> > So the vast majority of systems will ship with this turned on. You cannot
> > use a config variable to control policy and expect it to be change by anyone
> > but a very very small subset of users. So please drop the 'you can just not
> > build it argument'.
>
> This is not my point here. I mention this under the (now known to be wrong) assumption
> that resources do care. I just want to state that kernels built for platforms where every
> byte counts can be stripped down by disabling it. Others where resources are no concern
> simply can map them all, even if not used.
Agreed. A subset of users will just build without this.
>
> > Userspace configuration changing is a lot easier if people actually care.
> > Sure, many distros will ship the same script to everyone.
> >
> >>
> >>> I think we need some deliberate userspace interaction to instantiate
> >>> one of these rather than 'always doing it'.
> >>
> >> My gut feeling is that this additional user-space interaction needs more resources and
> >> adds a lot of complexity, independently of how it is done.
> >
> > Trivial resources and actually fairly trivial complexity. Key thing is
> > it puts the burden on the users of this functionality to configure what they
> > want.
>
> Hm. No. My proposal does not need configuration which accelerometers should go where.
Agreed. I was talking about my proposal here :)
>
> I assumethat input accelerometer users do not want to configure anything, like neither
> a mouse or keyboard is to be configured to be useable (yes there are keymaps but that
> is impossible to automate).
The difference is a mouse is only really useful as a mouse and most of the time a keyboard
is a used only as a keyboard. Here that's not true.
>
> They just want to be able to read device orientation in a device-independent scale.
> Therefore my approach already takes the mount-matrix into account to hide sensor position
> differences.
And how does that work on the common case of a sensor in the lid of a laptop?
how do you know what angle the screen is at?
One oddity to note here is that until very recently we deliberately didn't register
certain ACPI IDs because they confused userspace by reporting two accelerometers
without any info on which was in the lid. Thankfully proper handling of that
is no being sorted. It's still mostly a case of just deliberately ignoring one
of the sensors.
>
> >
> >>
> >> And I think is even less flexible than "always doing it". Let me explain this claim.
> >>
> >> For me, the kernel should present everything the hardware supports to user-space
> >> in better digestable device files or APIs (without making any assumptions about the
> >> user-space code).
> >
> > Agreed, we just have a different view on how this should be done. I want
> > it to be dynamic and extremely flexible, you want the easy way of just
> > putting a fixed set out all the time.
> >
> >>
> >> Then, every user-space that will be installed can find out what the hardware supports
> >> by looking at standard places.
> >>
> >> E.g. it can scan for all mice and keyboards. And for all input accelerometers.
> >
> > Or, you an have the correct 'fairly trivial' userspace setup to scan for all
> > registered accelerometers and 'on demand' create the bindings to bring them up as
> > Input accelerometers if that is what makes sense for your platform.
>
> Why not scan for input accelerometers and leave it as an implementation detail that
> the kernel does serve the physical chips through the iio infrastructure?
If we could separate the IIO front end from the IIO backend I would agree that
would be another valid -userspace- policy.
>
> IMHO some user-spaces may already be scanning all */input/event* and check for
> the device property INPUT_PROP_ACCELEROMETER.
>
> This is a discussion mainly about proper encapsulation of lower level differences.
>
> >
> >>
> >> If the kernel is hiding some chips and needs some initial user-space action before
> >> presenting them all, this requires that the user-space has some a-priori knowledge
> >> about which specific devices it should ask for.
> >
> > No more that it needs to know which accelerometer to use?
>
> >
> >> So it does not really need to scan
> >> for them. Because it must already know. Obviously in some mapping table stored at
> >> a well known location inside the rootfs image.
> >
> > No. Let me give some more details of how this would work. It's really just
> > a more flexible version of what you have.
> >
> > A distro, or individual user decides to put the relevant script in place for the
> > following:
> >
> > 1. Userspace detects a new accelerometer driver, via the standard methods (uevent)
> > 2. Userspace looks to see if it has the required properties. Now this includes things
> > like detecting that it is the accelerometer in the lid of a laptop - if so do not
> > register it as an input device. If it's in the keyboard then do register it.
> > 3. Userspace script then creates the files in configfs
> > /sys/kernel/config/iio/maps/
> > (this interface needs appropriate definition)
> > Maybe...
> > /sys/kernel/config/iio/maps/iio_input/iio_device:X/accel_x, accel_y, etc
> > When done it writes to the bind file
> > /sys/kernel/config/iio/maps/iio_input/iio_device:X/bind
> > which instantiates the input driver.
> >
> > This moves all of the policy decision into userspace, where it belongs. If
> > we want to enable a particular accelerometer on a particular board because it
> > actually works better than the one the default policy says to use, then we can
> > do so.
> >
> > The resulting infrastructure is much more general, because it lets us do the
> > same for any IIO consumer. This input bridge is not a special case. It works
> > equally well for the existing hwmon bridge any would even let us do things
> > like provide the information from userspace that we have an analog accelerometer
> > wired up to an ADC on some hacker board.
>
> Ok, understood.
>
> My approach triggers input uevents:
>
> 1. kernel detects a new iio accelerometer (looks like an analog accelerometer should be
> the DTS child of an iio adc and then iio should create an accelerometer and not a voltage
> channel)
Yes ultimately it would be a child device that would be it's own IIO device. We
already have this for some gyroscopes.
> 2. iio-bridge registers as input event
> 3. this triggers an uevent
> 4 an udev-rule can detect the properties and map it to some "speaking" name like
> /dev/input/main-accelerometer, /dev/input/lid-accelerometer etc. Or if the
> accelerometer is to be ignored, it does not get a "speaking" name at all.
>
> The required udev rules are stored in user space and are of course user-space and application
> specific. But this does not require to invent some new configfs stuff and special scripts
> in user-space. Just install some udev rule at a well established location in file-system.
I'm not sure there is any significant difference between you creating a mapping like
this an udev rule that creates the whole mapping. Bit more to do perhaps but it's
nothing particularly special that I can see. Sure there is new kernel support to be
done.
>
> Yes, this does not cover arbitrary mappings. But what are arbitrary mappings good
> for? Your scheme seems to be able to map a light sensor to accelerometer input.
> Does this "full matrix of everything is possible" really make sense?
From a generic interface point of view - yes it absolutely does.
We define an interface that covers all usecases rather than a whole set of
separate ones that cover individual corner cases. That way we don't have to
keep defining new interfaces.
The individual drivers can easily do validation of what they are provided with.
>
> I can't decide because I have no need for it. Others may have.
>
> But another thought: does it interfere with this input-bridge? Probably no. You can
> still add your configfs approach for general iio devices to e.g. hwmon mappings. Even
> as an alternate method of creating input devices (enabled only if my input-bridge is
> disabled).
Yes see above. Both approaches meet your requirement (I think anyway).
I do not want to see two long term solutions to the same problem.
I'm interested in a long term sustainable solution so I want to see
the generic one.
>
> >
> >
> >>
> >> This seems to make it impossible to develop a generic distro rootfs image - without
> >> asking the user for manual configuration. And that where the kernel already knows
> >> this (which iio accelerometers do exist for a specific piece of hardware).
> >>
> >> This is why I believe a mechanism to instantiate only on demand isn't adding but
> >> removing flexibility because it prevents copying a rootfs from one device to another.
> >
> > I disagree, see above.
> >
> >>
> >>>
> >>> As I mentioned in V1, look at the possibility of a configfs based method
> >>> to build the map. It's easy for userspace to work out what makes sense to
> >>> map in principle. There may be some missing info that we also need to
> >>> look to expose.
> >>
> >> With a "may be missing" it is impossible to write code for it...
> >> Can you please name which information is missing on the input accelerometer
> >> API?
> >
> > See above. It's not the input accelerometer ABI, it's the missing ability
> > to instantiate IIO maps from user space.
> >
> >>
> >>>
> >>> In general, userspace created channel maps would be very useful for
> >>> other things such as maker type boards where they can plug all sorts
> >>> of odd things into ADC channels for example.
> >>
> >> Ok, I understand, but this is a different problem where this iio-input-bridge is not
> >> intended to be a solution. Generic ADCs are not input devices. Like SD cards are not
> >> keyboards.
> >>
> >> So we should not try to mix the idea of general mapping with this input-bridge for
> >> input accelerometers.
> > Yes we should. You are proposing a solution that is a subset of the larger
> > problem set.
>
> Yes, of course. Because I did not see or know about the general problem set.
> And I still don't see a need for user-space controlled mapping for input-accelerometers.
We are clearly going to differ on this. Bastien gave one example for why
this is required. There will be others.
>
> > Why introduce a stop gap like this when we can do it correctly
> > and provide something useful for all those other use cases.
> >
> > The only difference here is the uevent triggered script that creates those maps
> > for your particular usecase.
>
> Well, I am a friend of solving one problem after the other in smaller steps than
> immediately aiming at a very general solution, which has side-effects of inventing
> new stuff for things that would work without.
That works in a world where you can drop the previous approach as part of your
generalization. When you are playing with kernel / userspace ABI then it
doesn't. Ideally you have to figure out the extensible general solution at the
start because you are stuck maintaining the 'small steps' for many years to
come. I don't want to perpetually 'have' to export all 3D accelerometers as
input devices, because we didn't have the ability to chose which should be
exported at some point in the past.
>
> >
> >
> >>
> >> BTW, there is a way to define additional mapping using udev rules which symlink the
> >> /dev/input/event* paths to stable names like /dev/input/accelerometer.
> >>
> >> This comes without additional code and is already provided by udev and the input system.
> >>
> >> So in summary, I have not yet seen a convincing scenario where being able to dynamically
> >> map iio channels to input devices seems beneficial.
> >
> > That is true for the narrow case you are talking about. I don't want to see that
> > narrow case solved in a fashion that effectively breaks solving it properly.
>
> How does it break your approach if added later? The more I think about it they are
> not incompatible. It is just useless to apply both in parallel.
The reality is that if we put one in first that will used for ever because there
will be devices out there using it. Therefore we have to maintain both for
ever.
>
> > If we add this, we have to export all accelerometers for ever under all circumstances
> > to userspace, because to remove it will break existing userspace.
> >
> > If we stand back and work out if we can do the general solution now, we avoid
> > this problem.
>
> We get a different problem that we break existing user-space that simply wants to see
> an /dev/input/accelerometer without doing more than an existing udev rule.
I would love to say such a userspace doesn't exist, but reality is there are
all sorts of hideous things out there. There are cases that deal with this
as an option of course (such as Bastien's sensor-proxy)
The number of devices that are supported under mainline as input accelerometers
is pretty small. It's not a perfect world unfortunately but having to add a
small udev script is at least not a major break if we do cause it.
>
> >
> >>
> >>>
> >>>>
> >>>> This driver simply collects the first 3 accelerometer channels as X, Y and Z.
> >>>> If only 1 or 2 channels are available, they are used for X and Y only. Additional
> >>>> channels are ignored.
> >>>>
> >>>> Scaling is done automatically so that 1g is represented by value 256 and
> >>>> range is assumed to be -511 .. +511 which gives a reasonable precision as an
> >>>> input device.
> >>>
> >>> Why do we do this, rather than letting input deal with it? Input is used
> >>> to widely differing scales IIRC
> >>
> >> Well, it can't be done differently... And what I call scale here is nothing more than
> >> defining ABSMIN_ACC_VAL and ABSMAX_ACC_VAL.
> >>
> >> We need to apply some scale since iio reports in (fractional) units of 1g, i.e. values
> >> of magnitude 1.
> >
> > m/s^2 not g, but doesn't matter for the point of view of this discussion.
>
> My fault. The driver takes care of this in the scaling formula so that "input" reports
> MAX/2 for 1g.
>
> >
> >> These are not adaequate for input events which use integers. So we must
> >> define some factor for iio_convert_raw_to_processed() to scale from raw value range
> >> to int value range. We could report raw values but this would be an improper abstraction
> >> from chip specific differences.
> >
> > Hmm. I can see we perhaps need some mapping, but is there a concept of standard scale
> > for existing input accelerometers? How is this done to give for other input devices
> > such as touch screens? I'd expect to see a separation between scale, and range.
> >
> >
> >>
> >> BTW: the range (and therefore the factor) is reported through the evdev driver to user-space
> >> (evtest reports Min and Max as you can see in the example).
> >>
> >> The most important thing is that this is a hardware independent definition. Every accelerometer
> >> chip will report this range. So you can easily upgrade hardware or switch accelerometers
> >> without touching user-space calibration. Like you can replace ethernet controller chips but
> >> networking works the same with all of them.
> >
> > Agreed, it needs to be hardware independent by the time it hits userspace, but I would
> > have thought that scaling would be done in input, rather than IIO. It's hardly
> > a problem unique to our usecase!
> >
> > Perhaps Dmitry can give some advice on this.
>
> Yes, that would be helpful.
>
> >
> >>
> >>
> >> Hm. Is there an alternative to attach such private data to an struct iio_dev
> >> allocated by someone else? I have not found one yet.
> >>
> >> Or can I add some void *input_mapping; to struct iio_dev? Depending on
> >> #if defined(CONFIG_IIO_INPUT_BRIDGE)?
> >
> > Yes, add a new element.
>
> Ok, works fine.
>
> I already have found one case of iio accelerometer driver where it did make a problem
> not using a special element.
>
> >>>
> >>> iio_input_find_accel_channel(indio_dev, chan, &numchans);
> >>> iio_input_register_device(indio_dev, chan, numchans);
> >>
> >> Well, that looks like it needs some temporary storage of dynamic size
> >> and loop twice over channels for no functional benefit.
> >
> > Use fixed size. The worst that happens is we end up with it being
> > an entry larger that it needs to be.
> >
> >> And handle the
> >> special case of numchans == 0 (the proposed code simply does not call
> >> iio_input_register_accel_channel and does not register anything).
> >>
> >> So I'd prefer to follow the "KISS" principle and register single channels
> >> instead of a set of channels.
> >
> > Well we disagree on this. A singleton approach like used here
> > is to my mind not KISS. I would rather see what is there then
> > act as two simple steps, rather than interleave two different
> > actions with a totally different path for the first channel found.
> > If there is only one channel you just built a load of infrastructure
> > that makes no sense. If you scan first then you can know that
> > before building anything.
>
> Ok, this is more a matter of taste and resource requirements can probably
> be neglected. I'll update the driver.
>
> So in summary, I'll post a v3 that fixes some bugs of v2 (because we need
> them fixed for our production systems as well).
>
> Then it is up to you if you want to take this approach or want to write
> a full version following your concept. Or if it is possible as I assume, we
> can have both.
Thanks. I think we need at some code for what I was proposing to discuss
much further. Unfortunately it may be a little while before I get time to
work on that. Hopefully not too long though!
Jonathan
>
> BR and thanks,
> Nikolaus
>
^ permalink raw reply
* [PATCH v2 59/79] docs: hid: convert to ReST
From: Mauro Carvalho Chehab @ 2019-04-22 13:27 UTC (permalink / raw)
To: Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Jiri Kosina, Jonathan Cameron,
Srinivas Pandruvada, Benjamin Tissoires, Dmitry Torokhov,
linux-input, linux-iio, linux-usb
In-Reply-To: <cover.1555938375.git.mchehab+samsung@kernel.org>
Rename the HID documentation files to ReST, add an
index for them and adjust in order to produce a nice html
output via the Sphinx build system.
While here, fix the sysfs example from hid-sensor.txt, that
has a lot of "?" instead of the proper UTF-8 characters that
are produced by the tree command.
At its new index.rst, let's add a :orphan: while this is not linked to
the main index.rst file, in order to avoid build warnings.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
.../hid/{hid-alps.txt => hid-alps.rst} | 85 ++-
.../hid/{hid-sensor.txt => hid-sensor.rst} | 192 +++----
.../{hid-transport.txt => hid-transport.rst} | 82 ++-
Documentation/hid/{hiddev.txt => hiddev.rst} | 130 +++--
Documentation/hid/{hidraw.txt => hidraw.rst} | 53 +-
Documentation/hid/index.rst | 18 +
Documentation/hid/intel-ish-hid.rst | 485 ++++++++++++++++++
Documentation/hid/intel-ish-hid.txt | 454 ----------------
Documentation/hid/{uhid.txt => uhid.rst} | 46 +-
Documentation/input/input.rst | 2 +-
MAINTAINERS | 2 +-
11 files changed, 885 insertions(+), 664 deletions(-)
rename Documentation/hid/{hid-alps.txt => hid-alps.rst} (64%)
rename Documentation/hid/{hid-sensor.txt => hid-sensor.rst} (61%)
rename Documentation/hid/{hid-transport.txt => hid-transport.rst} (93%)
rename Documentation/hid/{hiddev.txt => hiddev.rst} (80%)
rename Documentation/hid/{hidraw.txt => hidraw.rst} (89%)
create mode 100644 Documentation/hid/index.rst
create mode 100644 Documentation/hid/intel-ish-hid.rst
delete mode 100644 Documentation/hid/intel-ish-hid.txt
rename Documentation/hid/{uhid.txt => uhid.rst} (94%)
diff --git a/Documentation/hid/hid-alps.txt b/Documentation/hid/hid-alps.rst
similarity index 64%
rename from Documentation/hid/hid-alps.txt
rename to Documentation/hid/hid-alps.rst
index 6b02a2447c77..e2f4c4c11e3f 100644
--- a/Documentation/hid/hid-alps.txt
+++ b/Documentation/hid/hid-alps.rst
@@ -1,19 +1,26 @@
+==========================
ALPS HID Touchpad Protocol
-----------------------
+==========================
Introduction
------------
Currently ALPS HID driver supports U1 Touchpad device.
-U1 devuce basic information.
+U1 device basic information.
+
+========== ======
Vender ID 0x044E
Product ID 0x120B
Version ID 0x0121
+========== ======
HID Descriptor
-------------
+--------------
+
+======= ==================== ===== =======================================
Byte Field Value Notes
+======= ==================== ===== =======================================
0 wHIDDescLength 001E Length of HID Descriptor : 30 bytes
2 bcdVersion 0100 Compliant with Version 1.00
4 wReportDescLength 00B2 Report Descriptor is 178 Bytes (0x00B2)
@@ -28,32 +35,42 @@ Byte Field Value Notes
22 wProductID 120B Product ID 0x120B
24 wVersionID 0121 Version 01.21
26 RESERVED 0000 RESERVED
+======= ==================== ===== =======================================
Report ID
-------------
-ReportID-1 (Input Reports) (HIDUsage-Mouse) for TP&SP
-ReportID-2 (Input Reports) (HIDUsage-keyboard) for TP
-ReportID-3 (Input Reports) (Vendor Usage: Max 10 finger data) for TP
-ReportID-4 (Input Reports) (Vendor Usage: ON bit data) for GP
-ReportID-5 (Feature Reports) Feature Reports
-ReportID-6 (Input Reports) (Vendor Usage: StickPointer data) for SP
-ReportID-7 (Feature Reports) Flash update (Bootloader)
+---------
+
+========== ================= =========================================
+ReportID-1 (Input Reports) (HIDUsage-Mouse) for TP&SP
+ReportID-2 (Input Reports) (HIDUsage-keyboard) for TP
+ReportID-3 (Input Reports) (Vendor Usage: Max 10 finger data) for TP
+ReportID-4 (Input Reports) (Vendor Usage: ON bit data) for GP
+ReportID-5 (Feature Reports) Feature Reports
+ReportID-6 (Input Reports) (Vendor Usage: StickPointer data) for SP
+ReportID-7 (Feature Reports) Flash update (Bootloader)
+========== ================= =========================================
Data pattern
------------
+
+===== ========== ===== =================
Case1 ReportID_1 TP/SP Relative/Relative
Case2 ReportID_3 TP Absolute
ReportID_6 SP Absolute
+===== ========== ===== =================
Command Read/Write
------------------
To read/write to RAM, need to send a commands to the device.
+
The command format is as below.
DataByte(SET_REPORT)
+
+===== ======================
Byte1 Command Byte
Byte2 Address - Byte 0 (LSB)
Byte3 Address - Byte 1
@@ -61,13 +78,19 @@ Byte4 Address - Byte 2
Byte5 Address - Byte 3 (MSB)
Byte6 Value Byte
Byte7 Checksum
+===== ======================
Command Byte is read=0xD1/write=0xD2 .
+
Address is read/write RAM address.
+
Value Byte is writing data when you send the write commands.
+
When you read RAM, there is no meaning.
DataByte(GET_REPORT)
+
+===== ======================
Byte1 Response Byte
Byte2 Address - Byte 0 (LSB)
Byte3 Address - Byte 1
@@ -75,6 +98,7 @@ Byte4 Address - Byte 2
Byte5 Address - Byte 3 (MSB)
Byte6 Value Byte
Byte7 Checksum
+===== ======================
Read value is stored in Value Byte.
@@ -82,7 +106,11 @@ Read value is stored in Value Byte.
Packet Format
Touchpad data byte
------------------
- b7 b6 b5 b4 b3 b2 b1 b0
+
+
+======= ======= ======= ======= ======= ======= ======= ======= =====
+- b7 b6 b5 b4 b3 b2 b1 b0
+======= ======= ======= ======= ======= ======= ======= ======= =====
1 0 0 SW6 SW5 SW4 SW3 SW2 SW1
2 0 0 0 Fcv Fn3 Fn2 Fn1 Fn0
3 Xa0_7 Xa0_6 Xa0_5 Xa0_4 Xa0_3 Xa0_2 Xa0_1 Xa0_0
@@ -114,17 +142,25 @@ Touchpad data byte
25 Ya4_7 Ya4_6 Ya4_5 Ya4_4 Ya4_3 Ya4_2 Ya4_1 Ya4_0
26 Ya4_15 Ya4_14 Ya4_13 Ya4_12 Ya4_11 Ya4_10 Ya4_9 Ya4_8
27 LFB4 Zs4_6 Zs4_5 Zs4_4 Zs4_3 Zs4_2 Zs4_1 Zs4_0
+======= ======= ======= ======= ======= ======= ======= ======= =====
-SW1-SW6: SW ON/OFF status
-Xan_15-0(16bit):X Absolute data of the "n"th finger
-Yan_15-0(16bit):Y Absolute data of the "n"th finger
-Zsn_6-0(7bit): Operation area of the "n"th finger
+SW1-SW6:
+ SW ON/OFF status
+Xan_15-0(16bit):
+ X Absolute data of the "n"th finger
+Yan_15-0(16bit):
+ Y Absolute data of the "n"th finger
+Zsn_6-0(7bit):
+ Operation area of the "n"th finger
StickPointer data byte
-------------------
- b7 b6 b5 b4 b3 b2 b1 b0
+----------------------
+
+======= ======= ======= ======= ======= ======= ======= ======= =====
+- b7 b6 b5 b4 b3 b2 b1 b0
+======= ======= ======= ======= ======= ======= ======= ======= =====
Byte1 1 1 1 0 1 SW3 SW2 SW1
Byte2 X7 X6 X5 X4 X3 X2 X1 X0
Byte3 X15 X14 X13 X12 X11 X10 X9 X8
@@ -132,8 +168,13 @@ Byte4 Y7 Y6 Y5 Y4 Y3 Y2 Y1 Y0
Byte5 Y15 Y14 Y13 Y12 Y11 Y10 Y9 Y8
Byte6 Z7 Z6 Z5 Z4 Z3 Z2 Z1 Z0
Byte7 T&P Z14 Z13 Z12 Z11 Z10 Z9 Z8
+======= ======= ======= ======= ======= ======= ======= ======= =====
-SW1-SW3: SW ON/OFF status
-Xn_15-0(16bit):X Absolute data
-Yn_15-0(16bit):Y Absolute data
-Zn_14-0(15bit):Z
+SW1-SW3:
+ SW ON/OFF status
+Xn_15-0(16bit):
+ X Absolute data
+Yn_15-0(16bit):
+ Y Absolute data
+Zn_14-0(15bit):
+ Z
diff --git a/Documentation/hid/hid-sensor.txt b/Documentation/hid/hid-sensor.rst
similarity index 61%
rename from Documentation/hid/hid-sensor.txt
rename to Documentation/hid/hid-sensor.rst
index b287752a31cd..758972e34971 100644
--- a/Documentation/hid/hid-sensor.txt
+++ b/Documentation/hid/hid-sensor.rst
@@ -1,6 +1,6 @@
-
+=====================
HID Sensors Framework
-======================
+=====================
HID sensor framework provides necessary interfaces to implement sensor drivers,
which are connected to a sensor hub. The sensor hub is a HID device and it provides
a report descriptor conforming to HID 1.12 sensor usage tables.
@@ -15,22 +15,22 @@ the drivers themselves."
This specification describes many usage IDs, which describe the type of sensor
and also the individual data fields. Each sensor can have variable number of
data fields. The length and order is specified in the report descriptor. For
-example a part of report descriptor can look like:
+example a part of report descriptor can look like::
- INPUT(1)[INPUT]
- ..
- Field(2)
- Physical(0020.0073)
- Usage(1)
- 0020.045f
- Logical Minimum(-32767)
- Logical Maximum(32767)
- Report Size(8)
- Report Count(1)
- Report Offset(16)
- Flags(Variable Absolute)
-..
-..
+ INPUT(1)[INPUT]
+ ..
+ Field(2)
+ Physical(0020.0073)
+ Usage(1)
+ 0020.045f
+ Logical Minimum(-32767)
+ Logical Maximum(32767)
+ Report Size(8)
+ Report Count(1)
+ Report Offset(16)
+ Flags(Variable Absolute)
+ ..
+ ..
The report is indicating "sensor page (0x20)" contains an accelerometer-3D (0x73).
This accelerometer-3D has some fields. Here for example field 2 is motion intensity
@@ -40,13 +40,14 @@ data will use this format.
Implementation
-=================
+==============
This specification defines many different types of sensors with different sets of
data fields. It is difficult to have a common input event to user space applications,
for different sensors. For example an accelerometer can send X,Y and Z data, whereas
an ambient light sensor can send illumination data.
So the implementation has two parts:
+
- Core hid driver
- Individual sensor processing part (sensor drivers)
@@ -55,8 +56,11 @@ Core driver
The core driver registers (hid-sensor-hub) registers as a HID driver. It parses
report descriptors and identifies all the sensors present. It adds an MFD device
with name HID-SENSOR-xxxx (where xxxx is usage id from the specification).
-For example
+
+For example:
+
HID-SENSOR-200073 is registered for an Accelerometer 3D driver.
+
So if any driver with this name is inserted, then the probe routine for that
function will be called. So an accelerometer processing driver can register
with this name and will be probed if there is an accelerometer-3D detected.
@@ -66,7 +70,8 @@ drivers to register and get events for that usage id. Also it provides parsing
functions, which get and set each input/feature/output report.
Individual sensor processing part (sensor drivers)
------------
+--------------------------------------------------
+
The processing driver will use an interface provided by the core driver to parse
the report and get the indexes of the fields and also can get events. This driver
can use IIO interface to use the standard ABI defined for a type of sensor.
@@ -75,31 +80,34 @@ can use IIO interface to use the standard ABI defined for a type of sensor.
Core driver Interface
=====================
-Callback structure:
-Each processing driver can use this structure to set some callbacks.
+Callback structure::
+
+ Each processing driver can use this structure to set some callbacks.
int (*suspend)(..): Callback when HID suspend is received
int (*resume)(..): Callback when HID resume is received
int (*capture_sample)(..): Capture a sample for one of its data fields
int (*send_event)(..): One complete event is received which can have
multiple data fields.
-Registration functions:
-int sensor_hub_register_callback(struct hid_sensor_hub_device *hsdev,
+Registration functions::
+
+ int sensor_hub_register_callback(struct hid_sensor_hub_device *hsdev,
u32 usage_id,
struct hid_sensor_hub_callbacks *usage_callback):
Registers callbacks for an usage id. The callback functions are not allowed
-to sleep.
+to sleep::
-int sensor_hub_remove_callback(struct hid_sensor_hub_device *hsdev,
+ int sensor_hub_remove_callback(struct hid_sensor_hub_device *hsdev,
u32 usage_id):
Removes callbacks for an usage id.
-Parsing function:
-int sensor_hub_input_get_attribute_info(struct hid_sensor_hub_device *hsdev,
+Parsing function::
+
+ int sensor_hub_input_get_attribute_info(struct hid_sensor_hub_device *hsdev,
u8 type,
u32 usage_id, u32 attr_usage_id,
struct hid_sensor_hub_attribute_info *info);
@@ -110,26 +118,27 @@ so that fields can be set or get individually.
These indexes avoid searching every time and getting field index to get or set.
-Set Feature report
-int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
+Set Feature report::
+
+ int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
u32 field_index, s32 value);
This interface is used to set a value for a field in feature report. For example
if there is a field report_interval, which is parsed by a call to
-sensor_hub_input_get_attribute_info before, then it can directly set that individual
-field.
+sensor_hub_input_get_attribute_info before, then it can directly set that
+individual field::
-int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
+ int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
u32 field_index, s32 *value);
This interface is used to get a value for a field in input report. For example
if there is a field report_interval, which is parsed by a call to
-sensor_hub_input_get_attribute_info before, then it can directly get that individual
-field value.
+sensor_hub_input_get_attribute_info before, then it can directly get that
+individual field value::
-int sensor_hub_input_attr_get_raw_value(struct hid_sensor_hub_device *hsdev,
+ int sensor_hub_input_attr_get_raw_value(struct hid_sensor_hub_device *hsdev,
u32 usage_id,
u32 attr_usage_id, u32 report_id);
@@ -143,6 +152,8 @@ registered callback function to process the sample.
----------
HID Custom and generic Sensors
+------------------------------
+
HID Sensor specification defines two special sensor usage types. Since they
don't represent a standard sensor, it is not possible to define using Linux IIO
@@ -158,66 +169,73 @@ keyboard attached/detached or lid open/close.
To allow application to utilize these sensors, here they are exported uses sysfs
attribute groups, attributes and misc device interface.
-An example of this representation on sysfs:
-/sys/devices/pci0000:00/INT33C2:00/i2c-0/i2c-INT33D1:00/0018:8086:09FA.0001/HID-SENSOR-2000e1.6.auto$ tree -R
-.
-????????? enable_sensor
-????????? feature-0-200316
-??????? ????????? feature-0-200316-maximum
-??????? ????????? feature-0-200316-minimum
-??????? ????????? feature-0-200316-name
-??????? ????????? feature-0-200316-size
-??????? ????????? feature-0-200316-unit-expo
-??????? ????????? feature-0-200316-units
-??????? ????????? feature-0-200316-value
-????????? feature-1-200201
-??????? ????????? feature-1-200201-maximum
-??????? ????????? feature-1-200201-minimum
-??????? ????????? feature-1-200201-name
-??????? ????????? feature-1-200201-size
-??????? ????????? feature-1-200201-unit-expo
-??????? ????????? feature-1-200201-units
-??????? ????????? feature-1-200201-value
-????????? input-0-200201
-??????? ????????? input-0-200201-maximum
-??????? ????????? input-0-200201-minimum
-??????? ????????? input-0-200201-name
-??????? ????????? input-0-200201-size
-??????? ????????? input-0-200201-unit-expo
-??????? ????????? input-0-200201-units
-??????? ????????? input-0-200201-value
-????????? input-1-200202
-??????? ????????? input-1-200202-maximum
-??????? ????????? input-1-200202-minimum
-??????? ????????? input-1-200202-name
-??????? ????????? input-1-200202-size
-??????? ????????? input-1-200202-unit-expo
-??????? ????????? input-1-200202-units
-??????? ????????? input-1-200202-value
+An example of this representation on sysfs::
+
+ /sys/devices/pci0000:00/INT33C2:00/i2c-0/i2c-INT33D1:00/0018:8086:09FA.0001/HID-SENSOR-2000e1.6.auto$ tree -R
+ .
+ │ ├── enable_sensor
+ │ │ ├── feature-0-200316
+ │ │ │ ├── feature-0-200316-maximum
+ │ │ │ ├── feature-0-200316-minimum
+ │ │ │ ├── feature-0-200316-name
+ │ │ │ ├── feature-0-200316-size
+ │ │ │ ├── feature-0-200316-unit-expo
+ │ │ │ ├── feature-0-200316-units
+ │ │ │ ├── feature-0-200316-value
+ │ │ ├── feature-1-200201
+ │ │ │ ├── feature-1-200201-maximum
+ │ │ │ ├── feature-1-200201-minimum
+ │ │ │ ├── feature-1-200201-name
+ │ │ │ ├── feature-1-200201-size
+ │ │ │ ├── feature-1-200201-unit-expo
+ │ │ │ ├── feature-1-200201-units
+ │ │ │ ├── feature-1-200201-value
+ │ │ ├── input-0-200201
+ │ │ │ ├── input-0-200201-maximum
+ │ │ │ ├── input-0-200201-minimum
+ │ │ │ ├── input-0-200201-name
+ │ │ │ ├── input-0-200201-size
+ │ │ │ ├── input-0-200201-unit-expo
+ │ │ │ ├── input-0-200201-units
+ │ │ │ ├── input-0-200201-value
+ │ │ ├── input-1-200202
+ │ │ │ ├── input-1-200202-maximum
+ │ │ │ ├── input-1-200202-minimum
+ │ │ │ ├── input-1-200202-name
+ │ │ │ ├── input-1-200202-size
+ │ │ │ ├── input-1-200202-unit-expo
+ │ │ │ ├── input-1-200202-units
+ │ │ │ ├── input-1-200202-value
Here there is a custom sensors with four fields, two feature and two inputs.
Each field is represented by a set of attributes. All fields except the "value"
are read only. The value field is a RW field.
-Example
-/sys/bus/platform/devices/HID-SENSOR-2000e1.6.auto/feature-0-200316$ grep -r . *
-feature-0-200316-maximum:6
-feature-0-200316-minimum:0
-feature-0-200316-name:property-reporting-state
-feature-0-200316-size:1
-feature-0-200316-unit-expo:0
-feature-0-200316-units:25
-feature-0-200316-value:1
+
+Example::
+
+ /sys/bus/platform/devices/HID-SENSOR-2000e1.6.auto/feature-0-200316$ grep -r . *
+ feature-0-200316-maximum:6
+ feature-0-200316-minimum:0
+ feature-0-200316-name:property-reporting-state
+ feature-0-200316-size:1
+ feature-0-200316-unit-expo:0
+ feature-0-200316-units:25
+ feature-0-200316-value:1
How to enable such sensor?
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
By default sensor can be power gated. To enable sysfs attribute "enable" can be
-used.
-$ echo 1 > enable_sensor
+used::
+
+ $ echo 1 > enable_sensor
Once enabled and powered on, sensor can report value using HID reports.
-These reports are pushed using misc device interface in a FIFO order.
-/dev$ tree | grep HID-SENSOR-2000e1.6.auto
-??????? ????????? 10:53 -> ../HID-SENSOR-2000e1.6.auto
-????????? HID-SENSOR-2000e1.6.auto
+These reports are pushed using misc device interface in a FIFO order::
+
+ /dev$ tree | grep HID-SENSOR-2000e1.6.auto
+ │ │ │ ├── 10:53 -> ../HID-SENSOR-2000e1.6.auto
+ │ ├── HID-SENSOR-2000e1.6.auto
Each reports can be of variable length preceded by a header. This header
consist of a 32 bit usage id, 64 bit time stamp and 32 bit length field of raw
diff --git a/Documentation/hid/hid-transport.txt b/Documentation/hid/hid-transport.rst
similarity index 93%
rename from Documentation/hid/hid-transport.txt
rename to Documentation/hid/hid-transport.rst
index 3dcba9fd4a3a..6f3aaa86ce7b 100644
--- a/Documentation/hid/hid-transport.txt
+++ b/Documentation/hid/hid-transport.rst
@@ -1,5 +1,6 @@
- HID I/O Transport Drivers
- ===========================
+=========================
+HID I/O Transport Drivers
+=========================
The HID subsystem is independent of the underlying transport driver. Initially,
only USB was supported, but other specifications adopted the HID design and
@@ -16,6 +17,8 @@ transport and device setup/management. HID core is responsible of
report-parsing, report interpretation and the user-space API. Device specifics
and quirks are handled by all layers depending on the quirk.
+::
+
+-----------+ +-----------+ +-----------+ +-----------+
| Device #1 | | Device #i | | Device #j | | Device #k |
+-----------+ +-----------+ +-----------+ +-----------+
@@ -42,8 +45,9 @@ and quirks are handled by all layers depending on the quirk.
+----------------+ +-----------+ +------------------+ +------------------+
Example Drivers:
- I/O: USB, I2C, Bluetooth-l2cap
- Transport: USB-HID, I2C-HID, BT-HIDP
+
+ - I/O: USB, I2C, Bluetooth-l2cap
+ - Transport: USB-HID, I2C-HID, BT-HIDP
Everything below "HID Core" is simplified in this graph as it is only of
interest to HID device drivers. Transport drivers do not need to know the
@@ -183,7 +187,7 @@ Other ctrl-channel requests are supported by USB-HID but are not available
-------------------
Transport drivers normally use the following procedure to register a new device
-with HID core:
+with HID core::
struct hid_device *hid;
int ret;
@@ -215,7 +219,7 @@ Once hid_add_device() is entered, HID core might use the callbacks provided in
"custom_ll_driver". Note that fields like "country" can be ignored by underlying
transport-drivers if not supported.
-To unregister a device, use:
+To unregister a device, use::
hid_destroy_device(hid);
@@ -226,73 +230,110 @@ driver callbacks.
-----------------------------
The available HID callbacks are:
- - int (*start) (struct hid_device *hdev)
+
+ ::
+
+ int (*start) (struct hid_device *hdev)
+
Called from HID device drivers once they want to use the device. Transport
drivers can choose to setup their device in this callback. However, normally
devices are already set up before transport drivers register them to HID core
so this is mostly only used by USB-HID.
- - void (*stop) (struct hid_device *hdev)
+ ::
+
+ void (*stop) (struct hid_device *hdev)
+
Called from HID device drivers once they are done with a device. Transport
drivers can free any buffers and deinitialize the device. But note that
->start() might be called again if another HID device driver is loaded on the
device.
+
Transport drivers are free to ignore it and deinitialize devices after they
destroyed them via hid_destroy_device().
- - int (*open) (struct hid_device *hdev)
+ ::
+
+ int (*open) (struct hid_device *hdev)
+
Called from HID device drivers once they are interested in data reports.
Usually, while user-space didn't open any input API/etc., device drivers are
not interested in device data and transport drivers can put devices asleep.
However, once ->open() is called, transport drivers must be ready for I/O.
->open() calls are nested for each client that opens the HID device.
- - void (*close) (struct hid_device *hdev)
+ ::
+
+ void (*close) (struct hid_device *hdev)
+
Called from HID device drivers after ->open() was called but they are no
longer interested in device reports. (Usually if user-space closed any input
devices of the driver).
+
Transport drivers can put devices asleep and terminate any I/O of all
->open() calls have been followed by a ->close() call. However, ->start() may
be called again if the device driver is interested in input reports again.
- - int (*parse) (struct hid_device *hdev)
+ ::
+
+ int (*parse) (struct hid_device *hdev)
+
Called once during device setup after ->start() has been called. Transport
drivers must read the HID report-descriptor from the device and tell HID core
about it via hid_parse_report().
- - int (*power) (struct hid_device *hdev, int level)
+ ::
+
+ int (*power) (struct hid_device *hdev, int level)
+
Called by HID core to give PM hints to transport drivers. Usually this is
analogical to the ->open() and ->close() hints and redundant.
- - void (*request) (struct hid_device *hdev, struct hid_report *report,
- int reqtype)
+ ::
+
+ void (*request) (struct hid_device *hdev, struct hid_report *report,
+ int reqtype)
+
Send an HID request on the ctrl channel. "report" contains the report that
should be sent and "reqtype" the request type. Request-type can be
HID_REQ_SET_REPORT or HID_REQ_GET_REPORT.
+
This callback is optional. If not provided, HID core will assemble a raw
report following the HID specs and send it via the ->raw_request() callback.
The transport driver is free to implement this asynchronously.
- - int (*wait) (struct hid_device *hdev)
+ ::
+
+ int (*wait) (struct hid_device *hdev)
+
Used by HID core before calling ->request() again. A transport driver can use
it to wait for any pending requests to complete if only one request is
allowed at a time.
- - int (*raw_request) (struct hid_device *hdev, unsigned char reportnum,
- __u8 *buf, size_t count, unsigned char rtype,
- int reqtype)
+ ::
+
+ int (*raw_request) (struct hid_device *hdev, unsigned char reportnum,
+ __u8 *buf, size_t count, unsigned char rtype,
+ int reqtype)
+
Same as ->request() but provides the report as raw buffer. This request shall
be synchronous. A transport driver must not use ->wait() to complete such
requests. This request is mandatory and hid core will reject the device if
it is missing.
- - int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len)
+ ::
+
+ int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len)
+
Send raw output report via intr channel. Used by some HID device drivers
which require high throughput for outgoing requests on the intr channel. This
must not cause SET_REPORT calls! This must be implemented as asynchronous
output report on the intr channel!
- - int (*idle) (struct hid_device *hdev, int report, int idle, int reqtype)
+ ::
+
+ int (*idle) (struct hid_device *hdev, int report, int idle, int reqtype)
+
Perform SET/GET_IDLE request. Only used by USB-HID, do not implement!
2.3) Data Path
@@ -314,4 +355,5 @@ transport driver and not passed to hid_input_report().
Acknowledgements to SET_REPORT requests are not of interest to HID core.
----------------------------------------------------
+
Written 2013, David Herrmann <dh.herrmann@gmail.com>
diff --git a/Documentation/hid/hiddev.txt b/Documentation/hid/hiddev.rst
similarity index 80%
rename from Documentation/hid/hiddev.txt
rename to Documentation/hid/hiddev.rst
index 638448707aa2..16c663530db2 100644
--- a/Documentation/hid/hiddev.txt
+++ b/Documentation/hid/hiddev.rst
@@ -1,6 +1,9 @@
+================================================
Care and feeding of your Human Interface Devices
+================================================
-INTRODUCTION
+Introduction
+============
In addition to the normal input type HID devices, USB also uses the
human interface device protocols for things that are not really human
@@ -16,7 +19,7 @@ normalised event interface - see Documentation/input/input.rst
* the hiddev interface, which provides fairly raw HID events
The data flow for a HID event produced by a device is something like
-the following :
+the following::
usb.c ---> hid-core.c ----> hid-input.c ----> [keyboard/mouse/joystick/event]
|
@@ -27,27 +30,29 @@ In addition, other subsystems (apart from USB) can potentially feed
events into the input subsystem, but these have no effect on the hid
device interface.
-USING THE HID DEVICE INTERFACE
+Using the HID Device Interface
+==============================
The hiddev interface is a char interface using the normal USB major,
with the minor numbers starting at 96 and finishing at 111. Therefore,
-you need the following commands:
-mknod /dev/usb/hiddev0 c 180 96
-mknod /dev/usb/hiddev1 c 180 97
-mknod /dev/usb/hiddev2 c 180 98
-mknod /dev/usb/hiddev3 c 180 99
-mknod /dev/usb/hiddev4 c 180 100
-mknod /dev/usb/hiddev5 c 180 101
-mknod /dev/usb/hiddev6 c 180 102
-mknod /dev/usb/hiddev7 c 180 103
-mknod /dev/usb/hiddev8 c 180 104
-mknod /dev/usb/hiddev9 c 180 105
-mknod /dev/usb/hiddev10 c 180 106
-mknod /dev/usb/hiddev11 c 180 107
-mknod /dev/usb/hiddev12 c 180 108
-mknod /dev/usb/hiddev13 c 180 109
-mknod /dev/usb/hiddev14 c 180 110
-mknod /dev/usb/hiddev15 c 180 111
+you need the following commands::
+
+ mknod /dev/usb/hiddev0 c 180 96
+ mknod /dev/usb/hiddev1 c 180 97
+ mknod /dev/usb/hiddev2 c 180 98
+ mknod /dev/usb/hiddev3 c 180 99
+ mknod /dev/usb/hiddev4 c 180 100
+ mknod /dev/usb/hiddev5 c 180 101
+ mknod /dev/usb/hiddev6 c 180 102
+ mknod /dev/usb/hiddev7 c 180 103
+ mknod /dev/usb/hiddev8 c 180 104
+ mknod /dev/usb/hiddev9 c 180 105
+ mknod /dev/usb/hiddev10 c 180 106
+ mknod /dev/usb/hiddev11 c 180 107
+ mknod /dev/usb/hiddev12 c 180 108
+ mknod /dev/usb/hiddev13 c 180 109
+ mknod /dev/usb/hiddev14 c 180 110
+ mknod /dev/usb/hiddev15 c 180 111
So you point your hiddev compliant user-space program at the correct
interface for your device, and it all just works.
@@ -56,7 +61,9 @@ Assuming that you have a hiddev compliant user-space program, of
course. If you need to write one, read on.
-THE HIDDEV API
+The HIDDEV API
+==============
+
This description should be read in conjunction with the HID
specification, freely available from http://www.usb.org, and
conveniently linked of http://www.linux-usb.org.
@@ -69,12 +76,14 @@ each of which can have one or more "usages". In the hid-core,
each one of these usages has a single signed 32 bit value.
read():
+-------
+
This is the event interface. When the HID device's state changes,
it performs an interrupt transfer containing a report which contains
the changed value. The hid-core.c module parses the report, and
returns to hiddev.c the individual usages that have changed within
the report. In its basic mode, the hiddev will make these individual
-usage changes available to the reader using a struct hiddev_event:
+usage changes available to the reader using a struct hiddev_event::
struct hiddev_event {
unsigned hid;
@@ -91,12 +100,18 @@ ioctl() described below.
ioctl():
+--------
+
This is the control interface. There are a number of controls:
-HIDIOCGVERSION - int (read)
-Gets the version code out of the hiddev driver.
+HIDIOCGVERSION
+ - int (read)
+
+ Gets the version code out of the hiddev driver.
+
+HIDIOCAPPLICATION
+ - (none)
-HIDIOCAPPLICATION - (none)
This ioctl call returns the HID application usage associated with the
hid device. The third argument to ioctl() specifies which application
index to get. This is useful when the device has more than one
@@ -106,7 +121,9 @@ returns -1. You can find out beforehand how many application
collections the device has from the num_applications field from the
hiddev_devinfo structure.
-HIDIOCGCOLLECTIONINFO - struct hiddev_collection_info (read/write)
+HIDIOCGCOLLECTIONINFO
+ - struct hiddev_collection_info (read/write)
+
This returns a superset of the information above, providing not only
application collections, but all the collections the device has. It
also returns the level the collection lives in the hierarchy.
@@ -115,14 +132,20 @@ field set to the index that should be returned. The ioctl fills in
the other fields. If the index is larger than the last collection
index, the ioctl returns -1 and sets errno to -EINVAL.
-HIDIOCGDEVINFO - struct hiddev_devinfo (read)
+HIDIOCGDEVINFO
+ - struct hiddev_devinfo (read)
+
Gets a hiddev_devinfo structure which describes the device.
-HIDIOCGSTRING - struct hiddev_string_descriptor (read/write)
+HIDIOCGSTRING
+ - struct hiddev_string_descriptor (read/write)
+
Gets a string descriptor from the device. The caller must fill in the
"index" field to indicate which descriptor should be returned.
-HIDIOCINITREPORT - (none)
+HIDIOCINITREPORT
+ - (none)
+
Instructs the kernel to retrieve all input and feature report values
from the device. At this point, all the usage structures will contain
current values for the device, and will maintain it as the device
@@ -130,21 +153,29 @@ changes. Note that the use of this ioctl is unnecessary in general,
since later kernels automatically initialize the reports from the
device at attach time.
-HIDIOCGNAME - string (variable length)
+HIDIOCGNAME
+ - string (variable length)
+
Gets the device name
-HIDIOCGREPORT - struct hiddev_report_info (write)
+HIDIOCGREPORT
+ - struct hiddev_report_info (write)
+
Instructs the kernel to get a feature or input report from the device,
in order to selectively update the usage structures (in contrast to
INITREPORT).
-HIDIOCSREPORT - struct hiddev_report_info (write)
+HIDIOCSREPORT
+ - struct hiddev_report_info (write)
+
Instructs the kernel to send a report to the device. This report can
be filled in by the user through HIDIOCSUSAGE calls (below) to fill in
individual usage values in the report before sending the report in full
to the device.
-HIDIOCGREPORTINFO - struct hiddev_report_info (read/write)
+HIDIOCGREPORTINFO
+ - struct hiddev_report_info (read/write)
+
Fills in a hiddev_report_info structure for the user. The report is
looked up by type (input, output or feature) and id, so these fields
must be filled in by the user. The ID can be absolute -- the actual
@@ -156,19 +187,25 @@ use the relative IDs above to enumerate the valid IDs. The ioctl
returns non-zero when there is no more next ID. The real report ID is
filled into the returned hiddev_report_info structure.
-HIDIOCGFIELDINFO - struct hiddev_field_info (read/write)
+HIDIOCGFIELDINFO
+ - struct hiddev_field_info (read/write)
+
Returns the field information associated with a report in a
hiddev_field_info structure. The user must fill in report_id and
report_type in this structure, as above. The field_index should also
be filled in, which should be a number from 0 and maxfield-1, as
returned from a previous HIDIOCGREPORTINFO call.
-HIDIOCGUCODE - struct hiddev_usage_ref (read/write)
+HIDIOCGUCODE
+ - struct hiddev_usage_ref (read/write)
+
Returns the usage_code in a hiddev_usage_ref structure, given that
given its report type, report id, field index, and index within the
field have already been filled into the structure.
-HIDIOCGUSAGE - struct hiddev_usage_ref (read/write)
+HIDIOCGUSAGE
+ - struct hiddev_usage_ref (read/write)
+
Returns the value of a usage in a hiddev_usage_ref structure. The
usage to be retrieved can be specified as above, or the user can
choose to fill in the report_type field and specify the report_id as
@@ -176,28 +213,37 @@ HID_REPORT_ID_UNKNOWN. In this case, the hiddev_usage_ref will be
filled in with the report and field information associated with this
usage if it is found.
-HIDIOCSUSAGE - struct hiddev_usage_ref (write)
+HIDIOCSUSAGE
+ - struct hiddev_usage_ref (write)
+
Sets the value of a usage in an output report. The user fills in
the hiddev_usage_ref structure as above, but additionally fills in
the value field.
-HIDIOGCOLLECTIONINDEX - struct hiddev_usage_ref (write)
+HIDIOGCOLLECTIONINDEX
+ - struct hiddev_usage_ref (write)
+
Returns the collection index associated with this usage. This
indicates where in the collection hierarchy this usage sits.
-HIDIOCGFLAG - int (read)
-HIDIOCSFLAG - int (write)
+HIDIOCGFLAG
+ - int (read)
+HIDIOCSFLAG
+ - int (write)
+
These operations respectively inspect and replace the mode flags
that influence the read() call above. The flags are as follows:
- HIDDEV_FLAG_UREF - read() calls will now return
+ HIDDEV_FLAG_UREF
+ - read() calls will now return
struct hiddev_usage_ref instead of struct hiddev_event.
This is a larger structure, but in situations where the
device has more than one usage in its reports with the
same usage code, this mode serves to resolve such
ambiguity.
- HIDDEV_FLAG_REPORT - This flag can only be used in conjunction
+ HIDDEV_FLAG_REPORT
+ - This flag can only be used in conjunction
with HIDDEV_FLAG_UREF. With this flag set, when the device
sends a report, a struct hiddev_usage_ref will be returned
to read() filled in with the report_type and report_id, but
diff --git a/Documentation/hid/hidraw.txt b/Documentation/hid/hidraw.rst
similarity index 89%
rename from Documentation/hid/hidraw.txt
rename to Documentation/hid/hidraw.rst
index c8436e354f44..4a4a0ba1f362 100644
--- a/Documentation/hid/hidraw.txt
+++ b/Documentation/hid/hidraw.rst
@@ -1,5 +1,6 @@
- HIDRAW - Raw Access to USB and Bluetooth Human Interface Devices
- ==================================================================
+================================================================
+HIDRAW - Raw Access to USB and Bluetooth Human Interface Devices
+================================================================
The hidraw driver provides a raw interface to USB and Bluetooth Human
Interface Devices (HIDs). It differs from hiddev in that reports sent and
@@ -31,6 +32,7 @@ directly under /dev (eg: /dev/hidraw0). As this location is distribution-
and udev rule-dependent, applications should use libudev to locate hidraw
devices attached to the system. There is a tutorial on libudev with a
working example at:
+
http://www.signal11.us/oss/udev/
The HIDRAW API
@@ -51,7 +53,7 @@ byte. For devices which do not use numbered reports, the report data
will begin at the first byte.
write()
---------
+-------
The write() function will write a report to the device. For USB devices, if
the device has an INTERRUPT OUT endpoint, the report will be sent on that
endpoint. If it does not, the report will be sent over the control endpoint,
@@ -62,38 +64,52 @@ number. If the device does not use numbered reports, the first byte should
be set to 0. The report data itself should begin at the second byte.
ioctl()
---------
+-------
Hidraw supports the following ioctls:
-HIDIOCGRDESCSIZE: Get Report Descriptor Size
+HIDIOCGRDESCSIZE:
+ Get Report Descriptor Size
+
This ioctl will get the size of the device's report descriptor.
-HIDIOCGRDESC: Get Report Descriptor
+HIDIOCGRDESC:
+ Get Report Descriptor
+
This ioctl returns the device's report descriptor using a
hidraw_report_descriptor struct. Make sure to set the size field of the
hidraw_report_descriptor struct to the size returned from HIDIOCGRDESCSIZE.
-HIDIOCGRAWINFO: Get Raw Info
+HIDIOCGRAWINFO:
+ Get Raw Info
+
This ioctl will return a hidraw_devinfo struct containing the bus type, the
vendor ID (VID), and product ID (PID) of the device. The bus type can be one
-of:
- BUS_USB
- BUS_HIL
- BUS_BLUETOOTH
- BUS_VIRTUAL
+of::
+
+ - BUS_USB
+ - BUS_HIL
+ - BUS_BLUETOOTH
+ - BUS_VIRTUAL
+
which are defined in uapi/linux/input.h.
-HIDIOCGRAWNAME(len): Get Raw Name
+HIDIOCGRAWNAME(len):
+ Get Raw Name
+
This ioctl returns a string containing the vendor and product strings of
the device. The returned string is Unicode, UTF-8 encoded.
-HIDIOCGRAWPHYS(len): Get Physical Address
+HIDIOCGRAWPHYS(len):
+ Get Physical Address
+
This ioctl returns a string representing the physical address of the device.
For USB devices, the string contains the physical path to the device (the
USB controller, hubs, ports, etc). For Bluetooth devices, the string
contains the hardware (MAC) address of the device.
-HIDIOCSFEATURE(len): Send a Feature Report
+HIDIOCSFEATURE(len):
+ Send a Feature Report
+
This ioctl will send a feature report to the device. Per the HID
specification, feature reports are always sent using the control endpoint.
Set the first byte of the supplied buffer to the report number. For devices
@@ -101,7 +117,9 @@ which do not use numbered reports, set the first byte to 0. The report data
begins in the second byte. Make sure to set len accordingly, to one more
than the length of the report (to account for the report number).
-HIDIOCGFEATURE(len): Get a Feature Report
+HIDIOCGFEATURE(len):
+ Get a Feature Report
+
This ioctl will request a feature report from the device using the control
endpoint. The first byte of the supplied buffer should be set to the report
number of the requested report. For devices which do not use numbered
@@ -109,11 +127,12 @@ reports, set the first byte to 0. The report will be returned starting at
the first byte of the buffer (ie: the report number is not returned).
Example
----------
+-------
In samples/, find hid-example.c, which shows examples of read(), write(),
and all the ioctls for hidraw. The code may be used by anyone for any
purpose, and can serve as a starting point for developing applications using
hidraw.
Document by:
+
Alan Ott <alan@signal11.us>, Signal 11 Software
diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
new file mode 100644
index 000000000000..af4324902622
--- /dev/null
+++ b/Documentation/hid/index.rst
@@ -0,0 +1,18 @@
+:orphan:
+
+=============================
+Human Interface Devices (HID)
+=============================
+
+.. toctree::
+ :maxdepth: 1
+
+ hiddev
+ hidraw
+ hid-sensor
+ hid-transport
+
+ uhid
+
+ hid-alps
+ intel-ish-hid
diff --git a/Documentation/hid/intel-ish-hid.rst b/Documentation/hid/intel-ish-hid.rst
new file mode 100644
index 000000000000..cccbf4be17d7
--- /dev/null
+++ b/Documentation/hid/intel-ish-hid.rst
@@ -0,0 +1,485 @@
+=================================
+Intel Integrated Sensor Hub (ISH)
+=================================
+
+A sensor hub enables the ability to offload sensor polling and algorithm
+processing to a dedicated low power co-processor. This allows the core
+processor to go into low power modes more often, resulting in the increased
+battery life.
+
+There are many vendors providing external sensor hubs confirming to HID
+Sensor usage tables, and used in several tablets, 2 in 1 convertible laptops
+and embedded products. Linux had this support since Linux 3.9.
+
+Intel® introduced integrated sensor hubs as a part of the SoC starting from
+Cherry Trail and now supported on multiple generations of CPU packages. There
+are many commercial devices already shipped with Integrated Sensor Hubs (ISH).
+These ISH also comply to HID sensor specification, but the difference is the
+transport protocol used for communication. The current external sensor hubs
+mainly use HID over i2C or USB. But ISH doesn't use either i2c or USB.
+
+1. Overview
+===========
+
+Using a analogy with a usbhid implementation, the ISH follows a similar model
+for a very high speed communication::
+
+ ----------------- ----------------------
+ | USB HID | --> | ISH HID |
+ ----------------- ----------------------
+ ----------------- ----------------------
+ | USB protocol | --> | ISH Transport |
+ ----------------- ----------------------
+ ----------------- ----------------------
+ | EHCI/XHCI | --> | ISH IPC |
+ ----------------- ----------------------
+ PCI PCI
+ ----------------- ----------------------
+ |Host controller| --> | ISH processor |
+ ----------------- ----------------------
+ USB Link
+ ----------------- ----------------------
+ | USB End points| --> | ISH Clients |
+ ----------------- ----------------------
+
+Like USB protocol provides a method for device enumeration, link management
+and user data encapsulation, the ISH also provides similar services. But it is
+very light weight tailored to manage and communicate with ISH client
+applications implemented in the firmware.
+
+The ISH allows multiple sensor management applications executing in the
+firmware. Like USB endpoints the messaging can be to/from a client. As part of
+enumeration process, these clients are identified. These clients can be simple
+HID sensor applications, sensor calibration application or senor firmware
+update application.
+
+The implementation model is similar, like USB bus, ISH transport is also
+implemented as a bus. Each client application executing in the ISH processor
+is registered as a device on this bus. The driver, which binds each device
+(ISH HID driver) identifies the device type and registers with the hid core.
+
+2. ISH Implementation: Block Diagram
+====================================
+
+::
+
+ ---------------------------
+ | User Space Applications |
+ ---------------------------
+
+ ----------------IIO ABI----------------
+ --------------------------
+ | IIO Sensor Drivers |
+ --------------------------
+ --------------------------
+ | IIO core |
+ --------------------------
+ --------------------------
+ | HID Sensor Hub MFD |
+ --------------------------
+ --------------------------
+ | HID Core |
+ --------------------------
+ --------------------------
+ | HID over ISH Client |
+ --------------------------
+ --------------------------
+ | ISH Transport (ISHTP) |
+ --------------------------
+ --------------------------
+ | IPC Drivers |
+ --------------------------
+ OS
+ ---------------- PCI -----------------
+ Hardware + Firmware
+ ----------------------------
+ | ISH Hardware/Firmware(FW) |
+ ----------------------------
+
+3. High level processing in above blocks
+========================================
+
+3.1 Hardware Interface
+----------------------
+
+The ISH is exposed as "Non-VGA unclassified PCI device" to the host. The PCI
+product and vendor IDs are changed from different generations of processors. So
+the source code which enumerate drivers needs to update from generation to
+generation.
+
+3.2 Inter Processor Communication (IPC) driver
+----------------------------------------------
+
+Location: drivers/hid/intel-ish-hid/ipc
+
+The IPC message used memory mapped I/O. The registers are defined in
+hw-ish-regs.h.
+
+3.2.1 IPC/FW message types
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+There are two types of messages, one for management of link and other messages
+are to and from transport layers.
+
+TX and RX of Transport messages
+...............................
+
+A set of memory mapped register offers support of multi byte messages TX and
+RX (E.g.IPC_REG_ISH2HOST_MSG, IPC_REG_HOST2ISH_MSG). The IPC layer maintains
+internal queues to sequence messages and send them in order to the FW.
+Optionally the caller can register handler to get notification of completion.
+A door bell mechanism is used in messaging to trigger processing in host and
+client firmware side. When ISH interrupt handler is called, the ISH2HOST
+doorbell register is used by host drivers to determine that the interrupt
+is for ISH.
+
+Each side has 32 32-bit message registers and a 32-bit doorbell. Doorbell
+register has the following format:
+Bits 0..6: fragment length (7 bits are used)
+Bits 10..13: encapsulated protocol
+Bits 16..19: management command (for IPC management protocol)
+Bit 31: doorbell trigger (signal H/W interrupt to the other side)
+Other bits are reserved, should be 0.
+
+3.2.2 Transport layer interface
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+To abstract HW level IPC communication, a set of callbacks are registered.
+The transport layer uses them to send and receive messages.
+Refer to struct ishtp_hw_ops for callbacks.
+
+3.3 ISH Transport layer
+-----------------------
+
+Location: drivers/hid/intel-ish-hid/ishtp/
+
+3.3.1 A Generic Transport Layer
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The transport layer is a bi-directional protocol, which defines:
+- Set of commands to start, stop, connect, disconnect and flow control
+(ishtp/hbm.h) for details
+- A flow control mechanism to avoid buffer overflows
+
+This protocol resembles bus messages described in the following document:
+http://www.intel.com/content/dam/www/public/us/en/documents/technical-\
+specifications/dcmi-hi-1-0-spec.pdf "Chapter 7: Bus Message Layer"
+
+3.3.2 Connection and Flow Control Mechanism
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Each FW client and a protocol is identified by an UUID. In order to communicate
+to a FW client, a connection must be established using connect request and
+response bus messages. If successful, a pair (host_client_id and fw_client_id)
+will identify the connection.
+
+Once connection is established, peers send each other flow control bus messages
+independently. Every peer may send a message only if it has received a
+flow-control credit before. Once it sent a message, it may not send another one
+before receiving the next flow control credit.
+Either side can send disconnect request bus message to end communication. Also
+the link will be dropped if major FW reset occurs.
+
+3.3.3 Peer to Peer data transfer
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Peer to Peer data transfer can happen with or without using DMA. Depending on
+the sensor bandwidth requirement DMA can be enabled by using module parameter
+ishtp_use_dma under intel_ishtp.
+
+Each side (host and FW) manages its DMA transfer memory independently. When an
+ISHTP client from either host or FW side wants to send something, it decides
+whether to send over IPC or over DMA; for each transfer the decision is
+independent. The sending side sends DMA_XFER message when the message is in
+the respective host buffer (TX when host client sends, RX when FW client
+sends). The recipient of DMA message responds with DMA_XFER_ACK, indicating
+the sender that the memory region for that message may be reused.
+
+DMA initialization is started with host sending DMA_ALLOC_NOTIFY bus message
+(that includes RX buffer) and FW responds with DMA_ALLOC_NOTIFY_ACK.
+Additionally to DMA address communication, this sequence checks capabilities:
+if thw host doesn't support DMA, then it won't send DMA allocation, so FW can't
+send DMA; if FW doesn't support DMA then it won't respond with
+DMA_ALLOC_NOTIFY_ACK, in which case host will not use DMA transfers.
+Here ISH acts as busmaster DMA controller. Hence when host sends DMA_XFER,
+it's request to do host->ISH DMA transfer; when FW sends DMA_XFER, it means
+that it already did DMA and the message resides at host. Thus, DMA_XFER
+and DMA_XFER_ACK act as ownership indicators.
+
+At initial state all outgoing memory belongs to the sender (TX to host, RX to
+FW), DMA_XFER transfers ownership on the region that contains ISHTP message to
+the receiving side, DMA_XFER_ACK returns ownership to the sender. A sender
+needs not wait for previous DMA_XFER to be ack'ed, and may send another message
+as long as remaining continuous memory in its ownership is enough.
+In principle, multiple DMA_XFER and DMA_XFER_ACK messages may be sent at once
+(up to IPC MTU), thus allowing for interrupt throttling.
+Currently, ISH FW decides to send over DMA if ISHTP message is more than 3 IPC
+fragments and via IPC otherwise.
+
+3.3.4 Ring Buffers
+^^^^^^^^^^^^^^^^^^
+
+When a client initiate a connection, a ring or RX and TX buffers are allocated.
+The size of ring can be specified by the client. HID client set 16 and 32 for
+TX and RX buffers respectively. On send request from client, the data to be
+sent is copied to one of the send ring buffer and scheduled to be sent using
+bus message protocol. These buffers are required because the FW may have not
+have processed the last message and may not have enough flow control credits
+to send. Same thing holds true on receive side and flow control is required.
+
+3.3.5 Host Enumeration
+^^^^^^^^^^^^^^^^^^^^^^
+
+The host enumeration bus command allow discovery of clients present in the FW.
+There can be multiple sensor clients and clients for calibration function.
+
+To ease in implantation and allow independent driver handle each client
+this transport layer takes advantage of Linux Bus driver model. Each
+client is registered as device on the the transport bus (ishtp bus).
+
+Enumeration sequence of messages:
+
+- Host sends HOST_START_REQ_CMD, indicating that host ISHTP layer is up.
+- FW responds with HOST_START_RES_CMD
+- Host sends HOST_ENUM_REQ_CMD (enumerate FW clients)
+- FW responds with HOST_ENUM_RES_CMD that includes bitmap of available FW
+ client IDs
+- For each FW ID found in that bitmap host sends
+ HOST_CLIENT_PROPERTIES_REQ_CMD
+- FW responds with HOST_CLIENT_PROPERTIES_RES_CMD. Properties include UUID,
+ max ISHTP message size, etc.
+- Once host received properties for that last discovered client, it considers
+ ISHTP device fully functional (and allocates DMA buffers)
+
+3.4 HID over ISH Client
+-----------------------
+
+Location: drivers/hid/intel-ish-hid
+
+The ISHTP client driver is responsible for:
+
+- enumerate HID devices under FW ISH client
+- Get Report descriptor
+- Register with HID core as a LL driver
+- Process Get/Set feature request
+- Get input reports
+
+3.5 HID Sensor Hub MFD and IIO sensor drivers
+---------------------------------------------
+
+The functionality in these drivers is the same as an external sensor hub.
+Refer to
+Documentation/hid/hid-sensor.rst for HID sensor
+Documentation/ABI/testing/sysfs-bus-iio for IIO ABIs to user space
+
+3.6 End to End HID transport Sequence Diagram
+---------------------------------------------
+
+::
+
+ HID-ISH-CLN ISHTP IPC HW
+ | | | |
+ | | |-----WAKE UP------------------>|
+ | | | |
+ | | |-----HOST READY--------------->|
+ | | | |
+ | | |<----MNG_RESET_NOTIFY_ACK----- |
+ | | | |
+ | |<----ISHTP_START------ | |
+ | | | |
+ | |<-----------------HOST_START_RES_CMD-------------------|
+ | | | |
+ | |------------------QUERY_SUBSCRIBER-------------------->|
+ | | | |
+ | |------------------HOST_ENUM_REQ_CMD------------------->|
+ | | | |
+ | |<-----------------HOST_ENUM_RES_CMD--------------------|
+ | | | |
+ | |------------------HOST_CLIENT_PROPERTIES_REQ_CMD------>|
+ | | | |
+ | |<-----------------HOST_CLIENT_PROPERTIES_RES_CMD-------|
+ | Create new device on in ishtp bus | |
+ | | | |
+ | |------------------HOST_CLIENT_PROPERTIES_REQ_CMD------>|
+ | | | |
+ | |<-----------------HOST_CLIENT_PROPERTIES_RES_CMD-------|
+ | Create new device on in ishtp bus | |
+ | | | |
+ | |--Repeat HOST_CLIENT_PROPERTIES_REQ_CMD-till last one--|
+ | | | |
+ probed()
+ |----ishtp_cl_connect--->|----------------- CLIENT_CONNECT_REQ_CMD-------------->|
+ | | | |
+ | |<----------------CLIENT_CONNECT_RES_CMD----------------|
+ | | | |
+ |register event callback | | |
+ | | | |
+ |ishtp_cl_send(
+ HOSTIF_DM_ENUM_DEVICES) |----------fill ishtp_msg_hdr struct write to HW----- >|
+ | | | |
+ | | |<-----IRQ(IPC_PROTOCOL_ISHTP---|
+ | | | |
+ |<--ENUM_DEVICE RSP------| | |
+ | | | |
+ for each enumerated device
+ |ishtp_cl_send(
+ HOSTIF_GET_HID_DESCRIPTOR|----------fill ishtp_msg_hdr struct write to HW----- >|
+ | | | |
+ ...Response
+ | | | |
+ for each enumerated device
+ |ishtp_cl_send(
+ HOSTIF_GET_REPORT_DESCRIPTOR|--------------fill ishtp_msg_hdr struct write to HW-- >|
+ | | | |
+ | | | |
+ hid_allocate_device
+ | | | |
+ hid_add_device | | |
+ | | | |
+
+
+3.7 ISH Debugging
+-----------------
+
+To debug ISH, event tracing mechanism is used. To enable debug logs
+echo 1 > /sys/kernel/debug/tracing/events/intel_ish/enable
+cat sys/kernel/debug/tracing/trace
+
+3.8 ISH IIO sysfs Example on Lenovo thinkpad Yoga 260
+-----------------------------------------------------
+
+::
+
+ root@otcpl-ThinkPad-Yoga-260:~# tree -l /sys/bus/iio/devices/
+ /sys/bus/iio/devices/
+ ├── iio:device0 -> ../../../devices/0044:8086:22D8.0001/HID-SENSOR-200073.9.auto/iio:device0
+ │ ├── buffer
+ │ │ ├── enable
+ │ │ ├── length
+ │ │ └── watermark
+ ...
+ │ ├── in_accel_hysteresis
+ │ ├── in_accel_offset
+ │ ├── in_accel_sampling_frequency
+ │ ├── in_accel_scale
+ │ ├── in_accel_x_raw
+ │ ├── in_accel_y_raw
+ │ ├── in_accel_z_raw
+ │ ├── name
+ │ ├── scan_elements
+ │ │ ├── in_accel_x_en
+ │ │ ├── in_accel_x_index
+ │ │ ├── in_accel_x_type
+ │ │ ├── in_accel_y_en
+ │ │ ├── in_accel_y_index
+ │ │ ├── in_accel_y_type
+ │ │ ├── in_accel_z_en
+ │ │ ├── in_accel_z_index
+ │ │ └── in_accel_z_type
+ ...
+ │ │ ├── devices
+ │ │ │ │ ├── buffer
+ │ │ │ │ │ ├── enable
+ │ │ │ │ │ ├── length
+ │ │ │ │ │ └── watermark
+ │ │ │ │ ├── dev
+ │ │ │ │ ├── in_intensity_both_raw
+ │ │ │ │ ├── in_intensity_hysteresis
+ │ │ │ │ ├── in_intensity_offset
+ │ │ │ │ ├── in_intensity_sampling_frequency
+ │ │ │ │ ├── in_intensity_scale
+ │ │ │ │ ├── name
+ │ │ │ │ ├── scan_elements
+ │ │ │ │ │ ├── in_intensity_both_en
+ │ │ │ │ │ ├── in_intensity_both_index
+ │ │ │ │ │ └── in_intensity_both_type
+ │ │ │ │ ├── trigger
+ │ │ │ │ │ └── current_trigger
+ ...
+ │ │ │ │ ├── buffer
+ │ │ │ │ │ ├── enable
+ │ │ │ │ │ ├── length
+ │ │ │ │ │ └── watermark
+ │ │ │ │ ├── dev
+ │ │ │ │ ├── in_magn_hysteresis
+ │ │ │ │ ├── in_magn_offset
+ │ │ │ │ ├── in_magn_sampling_frequency
+ │ │ │ │ ├── in_magn_scale
+ │ │ │ │ ├── in_magn_x_raw
+ │ │ │ │ ├── in_magn_y_raw
+ │ │ │ │ ├── in_magn_z_raw
+ │ │ │ │ ├── in_rot_from_north_magnetic_tilt_comp_raw
+ │ │ │ │ ├── in_rot_hysteresis
+ │ │ │ │ ├── in_rot_offset
+ │ │ │ │ ├── in_rot_sampling_frequency
+ │ │ │ │ ├── in_rot_scale
+ │ │ │ │ ├── name
+ ...
+ │ │ │ │ ├── scan_elements
+ │ │ │ │ │ ├── in_magn_x_en
+ │ │ │ │ │ ├── in_magn_x_index
+ │ │ │ │ │ ├── in_magn_x_type
+ │ │ │ │ │ ├── in_magn_y_en
+ │ │ │ │ │ ├── in_magn_y_index
+ │ │ │ │ │ ├── in_magn_y_type
+ │ │ │ │ │ ├── in_magn_z_en
+ │ │ │ │ │ ├── in_magn_z_index
+ │ │ │ │ │ ├── in_magn_z_type
+ │ │ │ │ │ ├── in_rot_from_north_magnetic_tilt_comp_en
+ │ │ │ │ │ ├── in_rot_from_north_magnetic_tilt_comp_index
+ │ │ │ │ │ └── in_rot_from_north_magnetic_tilt_comp_type
+ │ │ │ │ ├── trigger
+ │ │ │ │ │ └── current_trigger
+ ...
+ │ │ │ │ ├── buffer
+ │ │ │ │ │ ├── enable
+ │ │ │ │ │ ├── length
+ │ │ │ │ │ └── watermark
+ │ │ │ │ ├── dev
+ │ │ │ │ ├── in_anglvel_hysteresis
+ │ │ │ │ ├── in_anglvel_offset
+ │ │ │ │ ├── in_anglvel_sampling_frequency
+ │ │ │ │ ├── in_anglvel_scale
+ │ │ │ │ ├── in_anglvel_x_raw
+ │ │ │ │ ├── in_anglvel_y_raw
+ │ │ │ │ ├── in_anglvel_z_raw
+ │ │ │ │ ├── name
+ │ │ │ │ ├── scan_elements
+ │ │ │ │ │ ├── in_anglvel_x_en
+ │ │ │ │ │ ├── in_anglvel_x_index
+ │ │ │ │ │ ├── in_anglvel_x_type
+ │ │ │ │ │ ├── in_anglvel_y_en
+ │ │ │ │ │ ├── in_anglvel_y_index
+ │ │ │ │ │ ├── in_anglvel_y_type
+ │ │ │ │ │ ├── in_anglvel_z_en
+ │ │ │ │ │ ├── in_anglvel_z_index
+ │ │ │ │ │ └── in_anglvel_z_type
+ │ │ │ │ ├── trigger
+ │ │ │ │ │ └── current_trigger
+ ...
+ │ │ │ │ ├── buffer
+ │ │ │ │ │ ├── enable
+ │ │ │ │ │ ├── length
+ │ │ │ │ │ └── watermark
+ │ │ │ │ ├── dev
+ │ │ │ │ ├── in_anglvel_hysteresis
+ │ │ │ │ ├── in_anglvel_offset
+ │ │ │ │ ├── in_anglvel_sampling_frequency
+ │ │ │ │ ├── in_anglvel_scale
+ │ │ │ │ ├── in_anglvel_x_raw
+ │ │ │ │ ├── in_anglvel_y_raw
+ │ │ │ │ ├── in_anglvel_z_raw
+ │ │ │ │ ├── name
+ │ │ │ │ ├── scan_elements
+ │ │ │ │ │ ├── in_anglvel_x_en
+ │ │ │ │ │ ├── in_anglvel_x_index
+ │ │ │ │ │ ├── in_anglvel_x_type
+ │ │ │ │ │ ├── in_anglvel_y_en
+ │ │ │ │ │ ├── in_anglvel_y_index
+ │ │ │ │ │ ├── in_anglvel_y_type
+ │ │ │ │ │ ├── in_anglvel_z_en
+ │ │ │ │ │ ├── in_anglvel_z_index
+ │ │ │ │ │ └── in_anglvel_z_type
+ │ │ │ │ ├── trigger
+ │ │ │ │ │ └── current_trigger
+ ...
diff --git a/Documentation/hid/intel-ish-hid.txt b/Documentation/hid/intel-ish-hid.txt
deleted file mode 100644
index d48b21c71ddd..000000000000
--- a/Documentation/hid/intel-ish-hid.txt
+++ /dev/null
@@ -1,454 +0,0 @@
-Intel Integrated Sensor Hub (ISH)
-===============================
-
-A sensor hub enables the ability to offload sensor polling and algorithm
-processing to a dedicated low power co-processor. This allows the core
-processor to go into low power modes more often, resulting in the increased
-battery life.
-
-There are many vendors providing external sensor hubs confirming to HID
-Sensor usage tables, and used in several tablets, 2 in 1 convertible laptops
-and embedded products. Linux had this support since Linux 3.9.
-
-Intel® introduced integrated sensor hubs as a part of the SoC starting from
-Cherry Trail and now supported on multiple generations of CPU packages. There
-are many commercial devices already shipped with Integrated Sensor Hubs (ISH).
-These ISH also comply to HID sensor specification, but the difference is the
-transport protocol used for communication. The current external sensor hubs
-mainly use HID over i2C or USB. But ISH doesn't use either i2c or USB.
-
-1. Overview
-
-Using a analogy with a usbhid implementation, the ISH follows a similar model
-for a very high speed communication:
-
- ----------------- ----------------------
- | USB HID | --> | ISH HID |
- ----------------- ----------------------
- ----------------- ----------------------
- | USB protocol | --> | ISH Transport |
- ----------------- ----------------------
- ----------------- ----------------------
- | EHCI/XHCI | --> | ISH IPC |
- ----------------- ----------------------
- PCI PCI
- ----------------- ----------------------
- |Host controller| --> | ISH processor |
- ----------------- ----------------------
- USB Link
- ----------------- ----------------------
- | USB End points| --> | ISH Clients |
- ----------------- ----------------------
-
-Like USB protocol provides a method for device enumeration, link management
-and user data encapsulation, the ISH also provides similar services. But it is
-very light weight tailored to manage and communicate with ISH client
-applications implemented in the firmware.
-
-The ISH allows multiple sensor management applications executing in the
-firmware. Like USB endpoints the messaging can be to/from a client. As part of
-enumeration process, these clients are identified. These clients can be simple
-HID sensor applications, sensor calibration application or senor firmware
-update application.
-
-The implementation model is similar, like USB bus, ISH transport is also
-implemented as a bus. Each client application executing in the ISH processor
-is registered as a device on this bus. The driver, which binds each device
-(ISH HID driver) identifies the device type and registers with the hid core.
-
-2. ISH Implementation: Block Diagram
-
- ---------------------------
- | User Space Applications |
- ---------------------------
-
-----------------IIO ABI----------------
- --------------------------
- | IIO Sensor Drivers |
- --------------------------
- --------------------------
- | IIO core |
- --------------------------
- --------------------------
- | HID Sensor Hub MFD |
- --------------------------
- --------------------------
- | HID Core |
- --------------------------
- --------------------------
- | HID over ISH Client |
- --------------------------
- --------------------------
- | ISH Transport (ISHTP) |
- --------------------------
- --------------------------
- | IPC Drivers |
- --------------------------
-OS
----------------- PCI -----------------
-Hardware + Firmware
- ----------------------------
- | ISH Hardware/Firmware(FW) |
- ----------------------------
-
-3. High level processing in above blocks
-
-3.1 Hardware Interface
-
-The ISH is exposed as "Non-VGA unclassified PCI device" to the host. The PCI
-product and vendor IDs are changed from different generations of processors. So
-the source code which enumerate drivers needs to update from generation to
-generation.
-
-3.2 Inter Processor Communication (IPC) driver
-Location: drivers/hid/intel-ish-hid/ipc
-
-The IPC message used memory mapped I/O. The registers are defined in
-hw-ish-regs.h.
-
-3.2.1 IPC/FW message types
-
-There are two types of messages, one for management of link and other messages
-are to and from transport layers.
-
-TX and RX of Transport messages
-
-A set of memory mapped register offers support of multi byte messages TX and
-RX (E.g.IPC_REG_ISH2HOST_MSG, IPC_REG_HOST2ISH_MSG). The IPC layer maintains
-internal queues to sequence messages and send them in order to the FW.
-Optionally the caller can register handler to get notification of completion.
-A door bell mechanism is used in messaging to trigger processing in host and
-client firmware side. When ISH interrupt handler is called, the ISH2HOST
-doorbell register is used by host drivers to determine that the interrupt
-is for ISH.
-
-Each side has 32 32-bit message registers and a 32-bit doorbell. Doorbell
-register has the following format:
-Bits 0..6: fragment length (7 bits are used)
-Bits 10..13: encapsulated protocol
-Bits 16..19: management command (for IPC management protocol)
-Bit 31: doorbell trigger (signal H/W interrupt to the other side)
-Other bits are reserved, should be 0.
-
-3.2.2 Transport layer interface
-
-To abstract HW level IPC communication, a set of callbacks are registered.
-The transport layer uses them to send and receive messages.
-Refer to struct ishtp_hw_ops for callbacks.
-
-3.3 ISH Transport layer
-Location: drivers/hid/intel-ish-hid/ishtp/
-
-3.3.1 A Generic Transport Layer
-
-The transport layer is a bi-directional protocol, which defines:
-- Set of commands to start, stop, connect, disconnect and flow control
-(ishtp/hbm.h) for details
-- A flow control mechanism to avoid buffer overflows
-
-This protocol resembles bus messages described in the following document:
-http://www.intel.com/content/dam/www/public/us/en/documents/technical-\
-specifications/dcmi-hi-1-0-spec.pdf "Chapter 7: Bus Message Layer"
-
-3.3.2 Connection and Flow Control Mechanism
-
-Each FW client and a protocol is identified by an UUID. In order to communicate
-to a FW client, a connection must be established using connect request and
-response bus messages. If successful, a pair (host_client_id and fw_client_id)
-will identify the connection.
-
-Once connection is established, peers send each other flow control bus messages
-independently. Every peer may send a message only if it has received a
-flow-control credit before. Once it sent a message, it may not send another one
-before receiving the next flow control credit.
-Either side can send disconnect request bus message to end communication. Also
-the link will be dropped if major FW reset occurs.
-
-3.3.3 Peer to Peer data transfer
-
-Peer to Peer data transfer can happen with or without using DMA. Depending on
-the sensor bandwidth requirement DMA can be enabled by using module parameter
-ishtp_use_dma under intel_ishtp.
-
-Each side (host and FW) manages its DMA transfer memory independently. When an
-ISHTP client from either host or FW side wants to send something, it decides
-whether to send over IPC or over DMA; for each transfer the decision is
-independent. The sending side sends DMA_XFER message when the message is in
-the respective host buffer (TX when host client sends, RX when FW client
-sends). The recipient of DMA message responds with DMA_XFER_ACK, indicating
-the sender that the memory region for that message may be reused.
-
-DMA initialization is started with host sending DMA_ALLOC_NOTIFY bus message
-(that includes RX buffer) and FW responds with DMA_ALLOC_NOTIFY_ACK.
-Additionally to DMA address communication, this sequence checks capabilities:
-if thw host doesn't support DMA, then it won't send DMA allocation, so FW can't
-send DMA; if FW doesn't support DMA then it won't respond with
-DMA_ALLOC_NOTIFY_ACK, in which case host will not use DMA transfers.
-Here ISH acts as busmaster DMA controller. Hence when host sends DMA_XFER,
-it's request to do host->ISH DMA transfer; when FW sends DMA_XFER, it means
-that it already did DMA and the message resides at host. Thus, DMA_XFER
-and DMA_XFER_ACK act as ownership indicators.
-
-At initial state all outgoing memory belongs to the sender (TX to host, RX to
-FW), DMA_XFER transfers ownership on the region that contains ISHTP message to
-the receiving side, DMA_XFER_ACK returns ownership to the sender. A sender
-needs not wait for previous DMA_XFER to be ack'ed, and may send another message
-as long as remaining continuous memory in its ownership is enough.
-In principle, multiple DMA_XFER and DMA_XFER_ACK messages may be sent at once
-(up to IPC MTU), thus allowing for interrupt throttling.
-Currently, ISH FW decides to send over DMA if ISHTP message is more than 3 IPC
-fragments and via IPC otherwise.
-
-3.3.4 Ring Buffers
-
-When a client initiate a connection, a ring or RX and TX buffers are allocated.
-The size of ring can be specified by the client. HID client set 16 and 32 for
-TX and RX buffers respectively. On send request from client, the data to be
-sent is copied to one of the send ring buffer and scheduled to be sent using
-bus message protocol. These buffers are required because the FW may have not
-have processed the last message and may not have enough flow control credits
-to send. Same thing holds true on receive side and flow control is required.
-
-3.3.5 Host Enumeration
-
-The host enumeration bus command allow discovery of clients present in the FW.
-There can be multiple sensor clients and clients for calibration function.
-
-To ease in implantation and allow independent driver handle each client
-this transport layer takes advantage of Linux Bus driver model. Each
-client is registered as device on the the transport bus (ishtp bus).
-
-Enumeration sequence of messages:
-- Host sends HOST_START_REQ_CMD, indicating that host ISHTP layer is up.
-- FW responds with HOST_START_RES_CMD
-- Host sends HOST_ENUM_REQ_CMD (enumerate FW clients)
-- FW responds with HOST_ENUM_RES_CMD that includes bitmap of available FW
-client IDs
-- For each FW ID found in that bitmap host sends
-HOST_CLIENT_PROPERTIES_REQ_CMD
-- FW responds with HOST_CLIENT_PROPERTIES_RES_CMD. Properties include UUID,
-max ISHTP message size, etc.
-- Once host received properties for that last discovered client, it considers
-ISHTP device fully functional (and allocates DMA buffers)
-
-3.4 HID over ISH Client
-Location: drivers/hid/intel-ish-hid
-
-The ISHTP client driver is responsible for:
-- enumerate HID devices under FW ISH client
-- Get Report descriptor
-- Register with HID core as a LL driver
-- Process Get/Set feature request
-- Get input reports
-
-3.5 HID Sensor Hub MFD and IIO sensor drivers
-
-The functionality in these drivers is the same as an external sensor hub.
-Refer to
-Documentation/hid/hid-sensor.txt for HID sensor
-Documentation/ABI/testing/sysfs-bus-iio for IIO ABIs to user space
-
-3.6 End to End HID transport Sequence Diagram
-
-HID-ISH-CLN ISHTP IPC HW
- | | | |
- | | |-----WAKE UP------------------>|
- | | | |
- | | |-----HOST READY--------------->|
- | | | |
- | | |<----MNG_RESET_NOTIFY_ACK----- |
- | | | |
- | |<----ISHTP_START------ | |
- | | | |
- | |<-----------------HOST_START_RES_CMD-------------------|
- | | | |
- | |------------------QUERY_SUBSCRIBER-------------------->|
- | | | |
- | |------------------HOST_ENUM_REQ_CMD------------------->|
- | | | |
- | |<-----------------HOST_ENUM_RES_CMD--------------------|
- | | | |
- | |------------------HOST_CLIENT_PROPERTIES_REQ_CMD------>|
- | | | |
- | |<-----------------HOST_CLIENT_PROPERTIES_RES_CMD-------|
- | Create new device on in ishtp bus | |
- | | | |
- | |------------------HOST_CLIENT_PROPERTIES_REQ_CMD------>|
- | | | |
- | |<-----------------HOST_CLIENT_PROPERTIES_RES_CMD-------|
- | Create new device on in ishtp bus | |
- | | | |
- | |--Repeat HOST_CLIENT_PROPERTIES_REQ_CMD-till last one--|
- | | | |
- probed()
- |----ishtp_cl_connect-->|----------------- CLIENT_CONNECT_REQ_CMD-------------->|
- | | | |
- | |<----------------CLIENT_CONNECT_RES_CMD----------------|
- | | | |
- |register event callback| | |
- | | | |
- |ishtp_cl_send(
- HOSTIF_DM_ENUM_DEVICES) |----------fill ishtp_msg_hdr struct write to HW----- >|
- | | | |
- | | |<-----IRQ(IPC_PROTOCOL_ISHTP---|
- | | | |
- |<--ENUM_DEVICE RSP-----| | |
- | | | |
-for each enumerated device
- |ishtp_cl_send(
- HOSTIF_GET_HID_DESCRIPTOR |----------fill ishtp_msg_hdr struct write to HW--- >|
- | | | |
- ...Response
- | | | |
-for each enumerated device
- |ishtp_cl_send(
- HOSTIF_GET_REPORT_DESCRIPTOR |----------fill ishtp_msg_hdr struct write to HW- >|
- | | | |
- | | | |
- hid_allocate_device
- | | | |
- hid_add_device | | |
- | | | |
-
-
-3.7 ISH Debugging
-
-To debug ISH, event tracing mechanism is used. To enable debug logs
-echo 1 > /sys/kernel/debug/tracing/events/intel_ish/enable
-cat sys/kernel/debug/tracing/trace
-
-3.8 ISH IIO sysfs Example on Lenovo thinkpad Yoga 260
-
-root@otcpl-ThinkPad-Yoga-260:~# tree -l /sys/bus/iio/devices/
-/sys/bus/iio/devices/
-├── iio:device0 -> ../../../devices/0044:8086:22D8.0001/HID-SENSOR-200073.9.auto/iio:device0
-│ ├── buffer
-│ │ ├── enable
-│ │ ├── length
-│ │ └── watermark
-...
-│ ├── in_accel_hysteresis
-│ ├── in_accel_offset
-│ ├── in_accel_sampling_frequency
-│ ├── in_accel_scale
-│ ├── in_accel_x_raw
-│ ├── in_accel_y_raw
-│ ├── in_accel_z_raw
-│ ├── name
-│ ├── scan_elements
-│ │ ├── in_accel_x_en
-│ │ ├── in_accel_x_index
-│ │ ├── in_accel_x_type
-│ │ ├── in_accel_y_en
-│ │ ├── in_accel_y_index
-│ │ ├── in_accel_y_type
-│ │ ├── in_accel_z_en
-│ │ ├── in_accel_z_index
-│ │ └── in_accel_z_type
-...
-│ │ ├── devices
-│ │ │ │ ├── buffer
-│ │ │ │ │ ├── enable
-│ │ │ │ │ ├── length
-│ │ │ │ │ └── watermark
-│ │ │ │ ├── dev
-│ │ │ │ ├── in_intensity_both_raw
-│ │ │ │ ├── in_intensity_hysteresis
-│ │ │ │ ├── in_intensity_offset
-│ │ │ │ ├── in_intensity_sampling_frequency
-│ │ │ │ ├── in_intensity_scale
-│ │ │ │ ├── name
-│ │ │ │ ├── scan_elements
-│ │ │ │ │ ├── in_intensity_both_en
-│ │ │ │ │ ├── in_intensity_both_index
-│ │ │ │ │ └── in_intensity_both_type
-│ │ │ │ ├── trigger
-│ │ │ │ │ └── current_trigger
-...
-│ │ │ │ ├── buffer
-│ │ │ │ │ ├── enable
-│ │ │ │ │ ├── length
-│ │ │ │ │ └── watermark
-│ │ │ │ ├── dev
-│ │ │ │ ├── in_magn_hysteresis
-│ │ │ │ ├── in_magn_offset
-│ │ │ │ ├── in_magn_sampling_frequency
-│ │ │ │ ├── in_magn_scale
-│ │ │ │ ├── in_magn_x_raw
-│ │ │ │ ├── in_magn_y_raw
-│ │ │ │ ├── in_magn_z_raw
-│ │ │ │ ├── in_rot_from_north_magnetic_tilt_comp_raw
-│ │ │ │ ├── in_rot_hysteresis
-│ │ │ │ ├── in_rot_offset
-│ │ │ │ ├── in_rot_sampling_frequency
-│ │ │ │ ├── in_rot_scale
-│ │ │ │ ├── name
-...
-│ │ │ │ ├── scan_elements
-│ │ │ │ │ ├── in_magn_x_en
-│ │ │ │ │ ├── in_magn_x_index
-│ │ │ │ │ ├── in_magn_x_type
-│ │ │ │ │ ├── in_magn_y_en
-│ │ │ │ │ ├── in_magn_y_index
-│ │ │ │ │ ├── in_magn_y_type
-│ │ │ │ │ ├── in_magn_z_en
-│ │ │ │ │ ├── in_magn_z_index
-│ │ │ │ │ ├── in_magn_z_type
-│ │ │ │ │ ├── in_rot_from_north_magnetic_tilt_comp_en
-│ │ │ │ │ ├── in_rot_from_north_magnetic_tilt_comp_index
-│ │ │ │ │ └── in_rot_from_north_magnetic_tilt_comp_type
-│ │ │ │ ├── trigger
-│ │ │ │ │ └── current_trigger
-...
-│ │ │ │ ├── buffer
-│ │ │ │ │ ├── enable
-│ │ │ │ │ ├── length
-│ │ │ │ │ └── watermark
-│ │ │ │ ├── dev
-│ │ │ │ ├── in_anglvel_hysteresis
-│ │ │ │ ├── in_anglvel_offset
-│ │ │ │ ├── in_anglvel_sampling_frequency
-│ │ │ │ ├── in_anglvel_scale
-│ │ │ │ ├── in_anglvel_x_raw
-│ │ │ │ ├── in_anglvel_y_raw
-│ │ │ │ ├── in_anglvel_z_raw
-│ │ │ │ ├── name
-│ │ │ │ ├── scan_elements
-│ │ │ │ │ ├── in_anglvel_x_en
-│ │ │ │ │ ├── in_anglvel_x_index
-│ │ │ │ │ ├── in_anglvel_x_type
-│ │ │ │ │ ├── in_anglvel_y_en
-│ │ │ │ │ ├── in_anglvel_y_index
-│ │ │ │ │ ├── in_anglvel_y_type
-│ │ │ │ │ ├── in_anglvel_z_en
-│ │ │ │ │ ├── in_anglvel_z_index
-│ │ │ │ │ └── in_anglvel_z_type
-│ │ │ │ ├── trigger
-│ │ │ │ │ └── current_trigger
-...
-│ │ │ │ ├── buffer
-│ │ │ │ │ ├── enable
-│ │ │ │ │ ├── length
-│ │ │ │ │ └── watermark
-│ │ │ │ ├── dev
-│ │ │ │ ├── in_anglvel_hysteresis
-│ │ │ │ ├── in_anglvel_offset
-│ │ │ │ ├── in_anglvel_sampling_frequency
-│ │ │ │ ├── in_anglvel_scale
-│ │ │ │ ├── in_anglvel_x_raw
-│ │ │ │ ├── in_anglvel_y_raw
-│ │ │ │ ├── in_anglvel_z_raw
-│ │ │ │ ├── name
-│ │ │ │ ├── scan_elements
-│ │ │ │ │ ├── in_anglvel_x_en
-│ │ │ │ │ ├── in_anglvel_x_index
-│ │ │ │ │ ├── in_anglvel_x_type
-│ │ │ │ │ ├── in_anglvel_y_en
-│ │ │ │ │ ├── in_anglvel_y_index
-│ │ │ │ │ ├── in_anglvel_y_type
-│ │ │ │ │ ├── in_anglvel_z_en
-│ │ │ │ │ ├── in_anglvel_z_index
-│ │ │ │ │ └── in_anglvel_z_type
-│ │ │ │ ├── trigger
-│ │ │ │ │ └── current_trigger
-...
diff --git a/Documentation/hid/uhid.txt b/Documentation/hid/uhid.rst
similarity index 94%
rename from Documentation/hid/uhid.txt
rename to Documentation/hid/uhid.rst
index 958fff945304..b18cb96c885f 100644
--- a/Documentation/hid/uhid.txt
+++ b/Documentation/hid/uhid.rst
@@ -1,5 +1,6 @@
- UHID - User-space I/O driver support for HID subsystem
- ========================================================
+======================================================
+UHID - User-space I/O driver support for HID subsystem
+======================================================
UHID allows user-space to implement HID transport drivers. Please see
hid-transport.txt for an introduction into HID transport drivers. This document
@@ -22,9 +23,9 @@ If a new device is detected by your HID I/O Driver and you want to register this
device with the HID subsystem, then you need to open /dev/uhid once for each
device you want to register. All further communication is done by read()'ing or
write()'ing "struct uhid_event" objects. Non-blocking operations are supported
-by setting O_NONBLOCK.
+by setting O_NONBLOCK::
-struct uhid_event {
+ struct uhid_event {
__u32 type;
union {
struct uhid_create2_req create2;
@@ -32,7 +33,7 @@ struct uhid_event {
struct uhid_input2_req input2;
...
} u;
-};
+ };
The "type" field contains the ID of the event. Depending on the ID different
payloads are sent. You must not split a single event across multiple read()'s or
@@ -86,31 +87,31 @@ the request was handled successfully. O_NONBLOCK does not affect write() as
writes are always handled immediately in a non-blocking fashion. Future requests
might make use of O_NONBLOCK, though.
- UHID_CREATE2:
+UHID_CREATE2:
This creates the internal HID device. No I/O is possible until you send this
event to the kernel. The payload is of type struct uhid_create2_req and
contains information about your device. You can start I/O now.
- UHID_DESTROY:
+UHID_DESTROY:
This destroys the internal HID device. No further I/O will be accepted. There
may still be pending messages that you can receive with read() but no further
UHID_INPUT events can be sent to the kernel.
You can create a new device by sending UHID_CREATE2 again. There is no need to
reopen the character device.
- UHID_INPUT2:
+UHID_INPUT2:
You must send UHID_CREATE2 before sending input to the kernel! This event
contains a data-payload. This is the raw data that you read from your device
on the interrupt channel. The kernel will parse the HID reports.
- UHID_GET_REPORT_REPLY:
+UHID_GET_REPORT_REPLY:
If you receive a UHID_GET_REPORT request you must answer with this request.
You must copy the "id" field from the request into the answer. Set the "err"
field to 0 if no error occurred or to EIO if an I/O error occurred.
If "err" is 0 then you should fill the buffer of the answer with the results
of the GET_REPORT request and set "size" correspondingly.
- UHID_SET_REPORT_REPLY:
+UHID_SET_REPORT_REPLY:
This is the SET_REPORT equivalent of UHID_GET_REPORT_REPLY. Unlike GET_REPORT,
SET_REPORT never returns a data buffer, therefore, it's sufficient to set the
"id" and "err" fields correctly.
@@ -120,16 +121,18 @@ read()
read() will return a queued output report. No reaction is required to any of
them but you should handle them according to your needs.
- UHID_START:
+UHID_START:
This is sent when the HID device is started. Consider this as an answer to
UHID_CREATE2. This is always the first event that is sent. Note that this
event might not be available immediately after write(UHID_CREATE2) returns.
Device drivers might required delayed setups.
This event contains a payload of type uhid_start_req. The "dev_flags" field
describes special behaviors of a device. The following flags are defined:
- UHID_DEV_NUMBERED_FEATURE_REPORTS:
- UHID_DEV_NUMBERED_OUTPUT_REPORTS:
- UHID_DEV_NUMBERED_INPUT_REPORTS:
+
+ - UHID_DEV_NUMBERED_FEATURE_REPORTS
+ - UHID_DEV_NUMBERED_OUTPUT_REPORTS
+ - UHID_DEV_NUMBERED_INPUT_REPORTS
+
Each of these flags defines whether a given report-type uses numbered
reports. If numbered reports are used for a type, all messages from
the kernel already have the report-number as prefix. Otherwise, no
@@ -137,33 +140,35 @@ them but you should handle them according to your needs.
For messages sent by user-space to the kernel, you must adjust the
prefixes according to these flags.
- UHID_STOP:
+UHID_STOP:
This is sent when the HID device is stopped. Consider this as an answer to
UHID_DESTROY.
+
If you didn't destroy your device via UHID_DESTROY, but the kernel sends an
UHID_STOP event, this should usually be ignored. It means that the kernel
reloaded/changed the device driver loaded on your HID device (or some other
maintenance actions happened).
+
You can usually ignored any UHID_STOP events safely.
- UHID_OPEN:
+UHID_OPEN:
This is sent when the HID device is opened. That is, the data that the HID
device provides is read by some other process. You may ignore this event but
it is useful for power-management. As long as you haven't received this event
there is actually no other process that reads your data so there is no need to
send UHID_INPUT2 events to the kernel.
- UHID_CLOSE:
+UHID_CLOSE:
This is sent when there are no more processes which read the HID data. It is
the counterpart of UHID_OPEN and you may as well ignore this event.
- UHID_OUTPUT:
+UHID_OUTPUT:
This is sent if the HID device driver wants to send raw data to the I/O
device on the interrupt channel. You should read the payload and forward it to
the device. The payload is of type "struct uhid_output_req".
This may be received even though you haven't received UHID_OPEN, yet.
- UHID_GET_REPORT:
+UHID_GET_REPORT:
This event is sent if the kernel driver wants to perform a GET_REPORT request
on the control channeld as described in the HID specs. The report-type and
report-number are available in the payload.
@@ -177,11 +182,12 @@ them but you should handle them according to your needs.
timed out, the kernel will ignore the response silently. The "id" field is
never re-used, so conflicts cannot happen.
- UHID_SET_REPORT:
+UHID_SET_REPORT:
This is the SET_REPORT equivalent of UHID_GET_REPORT. On receipt, you shall
send a SET_REPORT request to your hid device. Once it replies, you must tell
the kernel about it via UHID_SET_REPORT_REPLY.
The same restrictions as for UHID_GET_REPORT apply.
----------------------------------------------------
+
Written 2012, David Herrmann <dh.herrmann@gmail.com>
diff --git a/Documentation/input/input.rst b/Documentation/input/input.rst
index 47f86a4bf16c..0eb61e67a7b7 100644
--- a/Documentation/input/input.rst
+++ b/Documentation/input/input.rst
@@ -188,7 +188,7 @@ LCDs and many other purposes.
The monitor and speaker controls should be easy to add to the hid/input
interface, but for the UPSs and LCDs it doesn't make much sense. For this,
-the hiddev interface was designed. See Documentation/hid/hiddev.txt
+the hiddev interface was designed. See Documentation/hid/hiddev.rst
for more information about it.
The usage of the usbhid module is very simple, it takes no parameters,
diff --git a/MAINTAINERS b/MAINTAINERS
index 66bcec263dbf..4e1e598a32d9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16165,7 +16165,7 @@ M: Benjamin Tissoires <benjamin.tissoires@redhat.com>
L: linux-usb@vger.kernel.org
T: git git://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git
S: Maintained
-F: Documentation/hid/hiddev.txt
+F: Documentation/hid/hiddev.rst
F: drivers/hid/usbhid/
USB INTEL XHCI ROLE MUX DRIVER
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v4 1/3] dt-bindings: input: add GPIO controllable vibrator
From: Rob Herring @ 2019-04-22 13:13 UTC (permalink / raw)
To: Luca Weiss
Cc: Dmitry Torokhov, Mark Rutland, Mauro Carvalho Chehab,
Pascal PAILLET-LME, Coly Li, Lee Jones, Xiaotong Lu, Brian Masney,
Baolin Wang, David Brown, open list:ARM/QUALCOMM SUPPORT,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
In-Reply-To: <20190420122333.23662-1-luca@z3ntu.xyz>
On Sat, Apr 20, 2019 at 7:24 AM Luca Weiss <luca@z3ntu.xyz> wrote:
>
> Provide a simple driver for GPIO controllable vibrators.
> It will be used by the Fairphone 2.
>
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
> Changes from v3:
> - Convert .txt based doc to the new yaml based format
>
> .../bindings/input/gpio-vibrator.yaml | 39 +++++++++++++++++++
> 1 file changed, 39 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/gpio-vibrator.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/gpio-vibrator.yaml b/Documentation/devicetree/bindings/input/gpio-vibrator.yaml
> new file mode 100644
> index 000000000000..bca1b6ea07a4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/gpio-vibrator.yaml
> @@ -0,0 +1,39 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/input/gpio-vibrator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO vibrator
> +
> +maintainers:
> + - Luca Weiss <luca@z3ntu.xyz>
> +
> +description: |+
> + Registers a GPIO device as vibrator, where the on/off capability is controlled by a GPIO.
> +
> +properties:
> + compatible:
> + items:
> + - const: gpio-vibrator
These 2 lines can be simplified to just 'const: gpio-vibrator'.
> +
> + enable-gpios:
> + maxItems: 1
> +
> + vcc-supply:
> + $ref: /schemas/types.yaml#/definitions/phandle
This isn't necessary as we can do this for every occurrence of
'*-supply'. I'll add that to the core schema.
With those 2 changes:
Reviewed-by: Rob Herring <robh@kernel.org>
> + description: Regulator that provides power
> +
> +required:
> + - compatible
> + - enable-gpios
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + vibrator {
> + compatible = "gpio-vibrator";
> + enable-gpios = <&msmgpio 86 GPIO_ACTIVE_HIGH>;
> + vcc-supply = <&pm8941_l18>;
> + };
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH 3/3] iio: light: apple-ib-als: Add driver for ALS on iBridge chip.
From: Jonathan Cameron @ 2019-04-22 12:01 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: Ronald Tschalär, Jiri Kosina, Benjamin Tissoires,
Hartmut Knaack, Lars-Peter Clausen, Lee Jones, linux-input,
linux-iio, linux-kernel
In-Reply-To: <alpine.DEB.2.21.1904221101420.964@vps.pmeerw.net>
On Mon, 22 Apr 2019 11:17:27 +0200 (CEST)
Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:
> On Sun, 21 Apr 2019, Ronald Tschalär wrote:
>
> > On 2016/2017 MacBook Pro's with a Touch Bar the ALS is attached to,
> > and exposed via the iBridge device. This provides the driver for that
> > sensor.
>
> some comments below inline
I'll 'nest' on Peter's review to avoid repetition.
A few additional comments inline.
Thanks,
Jonathan
>
> > Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> > ---
> > drivers/iio/light/Kconfig | 12 +
> > drivers/iio/light/Makefile | 1 +
> > drivers/iio/light/apple-ib-als.c | 694 +++++++++++++++++++++++++++++++
> > 3 files changed, 707 insertions(+)
> > create mode 100644 drivers/iio/light/apple-ib-als.c
> >
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index 36f458433480..49159fab1c0e 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -64,6 +64,18 @@ config APDS9960
> > To compile this driver as a module, choose M here: the
> > module will be called apds9960
> >
> > +config APPLE_IBRIDGE_ALS
> > + tristate "Apple iBridge ambient light sensor"
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > + depends on MFD_APPLE_IBRIDGE
> > + help
> > + Say Y here to build the driver for the Apple iBridge ALS
> > + sensor.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called apple-ib-als.
> > +
> > config BH1750
> > tristate "ROHM BH1750 ambient light sensor"
> > depends on I2C
> > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > index 286bf3975372..144d918917f7 100644
> > --- a/drivers/iio/light/Makefile
> > +++ b/drivers/iio/light/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> > obj-$(CONFIG_AL3320A) += al3320a.o
> > obj-$(CONFIG_APDS9300) += apds9300.o
> > obj-$(CONFIG_APDS9960) += apds9960.o
> > +obj-$(CONFIG_APPLE_IBRIDGE_ALS) += apple-ib-als.o
> > obj-$(CONFIG_BH1750) += bh1750.o
> > obj-$(CONFIG_BH1780) += bh1780.o
> > obj-$(CONFIG_CM32181) += cm32181.o
> > diff --git a/drivers/iio/light/apple-ib-als.c b/drivers/iio/light/apple-ib-als.c
> > new file mode 100644
> > index 000000000000..1718fcbe304f
> > --- /dev/null
> > +++ b/drivers/iio/light/apple-ib-als.c
> > @@ -0,0 +1,694 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Apple Ambient Light Sensor Driver
> > + *
> > + * Copyright (c) 2017-2018 Ronald Tschalär
> > + */
> > +
> > +/*
> > + * MacBookPro models with an iBridge chip (13,[23] and 14,[23]) have an
> > + * ambient light sensor that is exposed via one of the USB interfaces on
> > + * the iBridge as a standard HID light sensor. However, we cannot use the
> > + * existing hid-sensor-als driver, for two reasons:
> > + *
> > + * 1. The hid-sensor-als driver is part of the hid-sensor-hub which in turn
> > + * is a hid driver, but you can't have more than one hid driver per hid
> > + * device, which is a problem because the touch bar also needs to
> > + * register as a driver for this hid device.
> > + *
> > + * 2. While the hid-sensors-als driver stores sensor readings received via
> > + * interrupt in an iio buffer, reads on the sysfs
> > + * .../iio:deviceX/in_illuminance_YYY attribute result in a get of the
> > + * feature report; however, in the case of this sensor here the
> > + * illuminance field of that report is always 0. Instead, the input
> > + * report needs to be requested.
> > + */
> > +
> > +#define dev_fmt(fmt) "als: " fmt
> > +
> > +#include <linux/device.h>
> > +#include <linux/hid.h>
> > +#include <linux/hid-sensor-ids.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/mfd/apple-ibridge.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#define APPLEALS_DYN_SENS 0 /* our dynamic sensitivity */
> > +#define APPLEALS_DEF_CHANGE_SENS APPLEALS_DYN_SENS
> > +
> > +struct appleals_device {
> > + struct appleib_device *ib_dev;
> > + struct device *log_dev;
> > + struct hid_device *hid_dev;
> > + struct hid_report *cfg_report;
> > + struct hid_field *illum_field;
> > + struct iio_dev *iio_dev;
> > + struct iio_trigger *iio_trig;
> > + int cur_sensitivity;
> > + int cur_hysteresis;
> > + bool events_enabled;
> > +};
> > +
> > +static struct hid_driver appleals_hid_driver;
> > +
> > +/*
> > + * This is a primitive way to get a relative sensitivity, one where we get
> > + * notified when the value changes by a certain percentage rather than some
> > + * absolute value. MacOS somehow manages to configure the sensor to work this
> > + * way (with a 15% relative sensitivity), but I haven't been able to figure
> > + * out how so far. So until we do, this provides a less-than-perfect
> > + * simulation.
> > + *
> > + * When the brightness value is within one of the ranges, the sensitivity is
> > + * set to that range's sensitivity. But in order to reduce flapping when the
> > + * brightness is right on the border between two ranges, the ranges overlap
> > + * somewhat (by at least one sensitivity), and sensitivity is only changed if
> > + * the value leaves the current sensitivity's range.
> > + *
> > + * The values chosen for the map are somewhat arbitrary: a compromise of not
> > + * too many ranges (and hence changing the sensitivity) but not too small or
> > + * large of a percentage of the min and max values in the range (currently
> > + * from 7.5% to 30%, i.e. within a factor of 2 of 15%), as well as just plain
> > + * "this feels reasonable to me".
> > + */
> > +struct appleals_sensitivity_map {
> > + int sensitivity;
> > + int illum_low;
> > + int illum_high;
> > +};
> > +
> > +static struct appleals_sensitivity_map appleals_sensitivity_map[] = {
>
> const?
>
> > + { 1, 0, 14 },
> > + { 3, 10, 40 },
> > + { 9, 30, 120 },
> > + { 27, 90, 360 },
> > + { 81, 270, 1080 },
> > + { 243, 810, 3240 },
> > + { 729, 2430, 9720 },
> > +};
> > +
> > +static int appleals_compute_sensitivity(int cur_illum, int cur_sens)
> > +{
> > + struct appleals_sensitivity_map *entry;
> > + int i;
> > +
> > + /* see if we're still in current range */
> > + for (i = 0; i < ARRAY_SIZE(appleals_sensitivity_map); i++) {
> > + entry = &appleals_sensitivity_map[i];
> > +
> > + if (entry->sensitivity == cur_sens &&
> > + entry->illum_low <= cur_illum &&
> > + entry->illum_high >= cur_illum)
> > + return cur_sens;
> > + else if (entry->sensitivity > cur_sens)
> > + break;
> > + }
> > +
> > + /* not in current range, so find new sensitivity */
> > + for (i = 0; i < ARRAY_SIZE(appleals_sensitivity_map); i++) {
> > + entry = &appleals_sensitivity_map[i];
> > +
> > + if (entry->illum_low <= cur_illum &&
> > + entry->illum_high >= cur_illum)
> > + return entry->sensitivity;
> > + }
> > +
> > + /* hmm, not in table, so assume we are above highest range */
> > + i = ARRAY_SIZE(appleals_sensitivity_map) - 1;
> > + return appleals_sensitivity_map[i].sensitivity;
> > +}
> > +
> > +static int appleals_get_field_value_for_usage(struct hid_field *field,
> > + unsigned int usage)
> > +{
> > + int u;
> > +
> > + if (!field)
> > + return -1;
> > +
> > + for (u = 0; u < field->maxusage; u++) {
> > + if (field->usage[u].hid == usage)
> > + return u + field->logical_minimum;
> > + }
> > +
> > + return -1;
> > +}
> > +
> > +static __s32 appleals_get_field_value(struct appleals_device *als_dev,
> > + struct hid_field *field)
> > +{
> > + hid_hw_request(als_dev->hid_dev, field->report, HID_REQ_GET_REPORT);
> > + hid_hw_wait(als_dev->hid_dev);
> > +
> > + return field->value[0];
> > +}
> > +
> > +static void appleals_set_field_value(struct appleals_device *als_dev,
> > + struct hid_field *field, __s32 value)
> > +{
> > + hid_set_field(field, 0, value);
> > + hid_hw_request(als_dev->hid_dev, field->report, HID_REQ_SET_REPORT);
> > +}
> > +
> > +static int appleals_get_config(struct appleals_device *als_dev,
> > + unsigned int field_usage, __s32 *value)
> > +{
> > + struct hid_field *field;
> > +
> > + field = appleib_find_report_field(als_dev->cfg_report, field_usage);
> > + if (!field)
> > + return -EINVAL;
> > +
> > + *value = appleals_get_field_value(als_dev, field);
> > +
> > + return 0;
> > +}
> > +
> > +static int appleals_set_config(struct appleals_device *als_dev,
> > + unsigned int field_usage, __s32 value)
> > +{
> > + struct hid_field *field;
> > +
> > + field = appleib_find_report_field(als_dev->cfg_report, field_usage);
> > + if (!field)
> > + return -EINVAL;
> > +
> > + appleals_set_field_value(als_dev, field, value);
> > +
> > + return 0;
> > +}
> > +
> > +static int appleals_set_enum_config(struct appleals_device *als_dev,
> > + unsigned int field_usage,
> > + unsigned int value_usage)
> > +{
> > + struct hid_field *field;
> > + int value;
> > +
> > + field = appleib_find_report_field(als_dev->cfg_report, field_usage);
> > + if (!field)
> > + return -EINVAL;
> > +
> > + value = appleals_get_field_value_for_usage(field, value_usage);
>
> can return -1, not checked
>
> > +
> > + appleals_set_field_value(als_dev, field, value);
> > +
> > + return 0;
> > +}
> > +
> > +static void appleals_update_dyn_sensitivity(struct appleals_device *als_dev,
> > + __s32 value)
> > +{
> > + int new_sens;
> > + int rc;
> > +
> > + new_sens = appleals_compute_sensitivity(value,
> > + als_dev->cur_sensitivity);
> > + if (new_sens != als_dev->cur_sensitivity) {
> > + rc = appleals_set_config(als_dev,
> > + HID_USAGE_SENSOR_LIGHT_ILLUM |
> > + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS,
> > + new_sens);
> > + if (!rc)
> > + als_dev->cur_sensitivity = new_sens;
> > + }
> > +}
> > +
> > +static void appleals_push_new_value(struct appleals_device *als_dev,
> > + __s32 value)
> > +{
> > + __s32 buf[2] = { value, value };
> > +
> > + iio_push_to_buffers(als_dev->iio_dev, buf);
> > +
> > + if (als_dev->cur_hysteresis == APPLEALS_DYN_SENS)
> > + appleals_update_dyn_sensitivity(als_dev, value);
> > +}
> > +
> > +static int appleals_hid_event(struct hid_device *hdev, struct hid_field *field,
> > + struct hid_usage *usage, __s32 value)
> > +{
> > + struct appleals_device *als_dev =
> > + appleib_get_drvdata(hid_get_drvdata(hdev),
> > + &appleals_hid_driver);
> > + int rc = 0;
> > +
> > + if ((usage->hid & HID_USAGE_PAGE) != HID_UP_SENSOR)
> > + return 0;
> > +
> > + if (usage->hid == HID_USAGE_SENSOR_LIGHT_ILLUM) {
> > + appleals_push_new_value(als_dev, value);
> > + rc = 1;
> > + }
> > +
> > + return rc;
> > +}
> > +
> > +static int appleals_enable_events(struct iio_trigger *trig, bool enable)
> > +{
> > + struct appleals_device *als_dev = iio_trigger_get_drvdata(trig);
> > + int value;
> > +
> > + /* set the sensor's reporting state */
> > + appleals_set_enum_config(als_dev, HID_USAGE_SENSOR_PROP_REPORT_STATE,
> > + enable ? HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM :
> > + HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM);
> > + als_dev->events_enabled = enable;
> > +
> > + /* if the sensor was enabled, push an initial value */
> > + if (enable) {
> > + value = appleals_get_field_value(als_dev, als_dev->illum_field);
> > + appleals_push_new_value(als_dev, value);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int appleals_read_raw(struct iio_dev *iio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct appleals_device *als_dev =
> > + *(struct appleals_device **)iio_priv(iio_dev);
> > + __s32 value;
> > + int rc;
> > +
> > + *val = 0;
> > + *val2 = 0;
>
> no need to set these here
>
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + case IIO_CHAN_INFO_PROCESSED:
> > + *val = appleals_get_field_value(als_dev, als_dev->illum_field);
How can one read by both processed and raw?
> > + return IIO_VAL_INT;
> > +
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + rc = appleals_get_config(als_dev,
> > + HID_USAGE_SENSOR_PROP_REPORT_INTERVAL,
> > + &value);
> > + if (rc)
> > + return rc;
> > +
> > + /* interval is in ms; val is in HZ, val2 in µHZ */
> > + value = 1000000000 / value;
> > + *val = value / 1000000;
> > + *val2 = value - (*val * 1000000);
> > +
> > + return IIO_VAL_INT_PLUS_MICRO;
> > +
> > + case IIO_CHAN_INFO_HYSTERESIS:
> > + if (als_dev->cur_hysteresis == APPLEALS_DYN_SENS) {
> > + *val = als_dev->cur_hysteresis;
> > + return IIO_VAL_INT;
> > + }
> > +
> > + rc = appleals_get_config(als_dev,
> > + HID_USAGE_SENSOR_LIGHT_ILLUM |
> > + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS,
> > + val);
> > + if (!rc) {
> > + als_dev->cur_sensitivity = *val;
> > + als_dev->cur_hysteresis = *val;
> > + }
> > + return rc ? rc : IIO_VAL_INT;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int appleals_write_raw(struct iio_dev *iio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val, int val2, long mask)
> > +{
> > + struct appleals_device *als_dev =
> > + *(struct appleals_device **)iio_priv(iio_dev);
> > + __s32 illum;
> > + int rc;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + rc = appleals_set_config(als_dev,
> > + HID_USAGE_SENSOR_PROP_REPORT_INTERVAL,
> > + 1000000000 / (val * 1000000 + val2));
> > + break;
>
> maybe return directly instead of at the end (matter of taste);
> here and in the other cases below
>
> > +
> > + case IIO_CHAN_INFO_HYSTERESIS:
> > + if (val == APPLEALS_DYN_SENS) {
> > + if (als_dev->cur_hysteresis != APPLEALS_DYN_SENS) {
> > + als_dev->cur_hysteresis = val;
> > + illum = appleals_get_field_value(als_dev,
> > + als_dev->illum_field);
> > + appleals_update_dyn_sensitivity(als_dev, illum);
There is some debate in another thread on whether dynamic sensitivity can be
mapped to hysteresis or not...
> > + }
> > + rc = 0;
> > + break;
> > + }
> > +
> > + rc = appleals_set_config(als_dev,
> > + HID_USAGE_SENSOR_LIGHT_ILLUM |
> > + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS,
> > + val);
> > + if (!rc) {
> > + als_dev->cur_sensitivity = val;
> > + als_dev->cur_hysteresis = val;
> > + }
> > + break;
> > +
> > + default:
> > + rc = -EINVAL;
> > + }
> > +
> > + return rc;
> > +}
> > +
> > +static const struct iio_chan_spec appleals_channels[] = {
> > + {
> > + .type = IIO_INTENSITY,
> > + .modified = 1,
> > + .channel2 = IIO_MOD_LIGHT_BOTH,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > + BIT(IIO_CHAN_INFO_RAW),
Why return both processed and raw? We don't generally allow that in IIO
(there are a few historical exceptions due to us getting it wrong the
first time).
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > + BIT(IIO_CHAN_INFO_HYSTERESIS),
> > + .scan_type = {
> > + .sign = 'u',
> > + .realbits = 32,
> > + .storagebits = 32,
> > + },
> > + .scan_index = 0,
> > + },
> > + {
> > + .type = IIO_LIGHT,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > + BIT(IIO_CHAN_INFO_RAW),
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > + BIT(IIO_CHAN_INFO_HYSTERESIS),
> > + .scan_type = {
> > + .sign = 'u',
> > + .realbits = 32,
> > + .storagebits = 32,
> > + },
> > + .scan_index = 1,
> > + }
> > +};
> > +
> > +static const struct iio_trigger_ops appleals_trigger_ops = {
> > + .set_trigger_state = &appleals_enable_events,
> > +};
> > +
> > +static const struct iio_info appleals_info = {
> > + .read_raw = &appleals_read_raw,
> > + .write_raw = &appleals_write_raw,
> > +};
> > +
> > +static void appleals_config_sensor(struct appleals_device *als_dev,
> > + bool events_enabled, int sensitivity)
> > +{
> > + struct hid_field *field;
> > + __s32 val;
> > +
> > + /*
> > + * We're (often) in a probe here, so need to enable input processing
> > + * in that case, but only in that case.
> > + */
I'd be a little happier if this wasn't termed 'in_hid_probe' but rather something
like _needs_io_start.
That way we are reflecting what needs to happen, not where we are in the code
flow that makes it true.
> > + if (appleib_in_hid_probe(als_dev->ib_dev))
> > + hid_device_io_start(als_dev->hid_dev);
> > +
> > + /* power on the sensor */
> > + field = appleib_find_report_field(als_dev->cfg_report,
> > + HID_USAGE_SENSOR_PROY_POWER_STATE);
> > + val = appleals_get_field_value_for_usage(field,
> > + HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM);
>
> what if -1?
>
> > + hid_set_field(field, 0, val);
> > +
> > + /* configure reporting of change events */
> > + field = appleib_find_report_field(als_dev->cfg_report,
> > + HID_USAGE_SENSOR_PROP_REPORT_STATE);
> > + val = appleals_get_field_value_for_usage(field,
> > + events_enabled ?
> > + HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM :
> > + HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM);
> > + hid_set_field(field, 0, val);
> > +
> > + /* report change events asap */
> > + field = appleib_find_report_field(als_dev->cfg_report,
> > + HID_USAGE_SENSOR_PROP_REPORT_INTERVAL);
> > + hid_set_field(field, 0, field->logical_minimum);
> > +
> > + /*
> > + * Set initial change sensitivity; if dynamic, enabling trigger will set
> > + * it instead.
> > + */
> > + if (sensitivity != APPLEALS_DYN_SENS) {
> > + field = appleib_find_report_field(als_dev->cfg_report,
> > + HID_USAGE_SENSOR_LIGHT_ILLUM |
> > + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS);
> > +
> > + hid_set_field(field, 0, sensitivity);
> > + }
> > +
> > + /* write the new config to the sensor */
> > + hid_hw_request(als_dev->hid_dev, als_dev->cfg_report,
> > + HID_REQ_SET_REPORT);
> > +
> > + if (appleib_in_hid_probe(als_dev->ib_dev))
> > + hid_device_io_stop(als_dev->hid_dev);
> > +};
>
> no semicolon at the end of a function please
>
> > +
> > +static int appleals_config_iio(struct appleals_device *als_dev)
> > +{
> > + struct iio_dev *iio_dev;
> > + struct iio_trigger *iio_trig;
> > + int rc;
> > +
> > + /* create and register iio device */
A very good reason for not adding superficial comments.
This one is wrong or at least misleading. It doesn't register
the iio device. So get rid of all these comments that are
just saying what the code next to them is doing. Keep the ones
that add extra information.
> > + iio_dev = iio_device_alloc(sizeof(als_dev));
>
> how about using the devm_ variants?
>
> > + if (!iio_dev)
> > + return -ENOMEM;
> > +
> > + *(struct appleals_device **)iio_priv(iio_dev) = als_dev;
That is nasty. Add a local variable to make to clear that iio_priv(iio_dev)
is actually some space for this pointer. Complex type casting is not
terribly readable.
> > +
> > + iio_dev->channels = appleals_channels;
> > + iio_dev->num_channels = ARRAY_SIZE(appleals_channels);
> > + iio_dev->dev.parent = &als_dev->hid_dev->dev;
> > + iio_dev->info = &appleals_info;
> > + iio_dev->name = "als";
> > + iio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + rc = iio_triggered_buffer_setup(iio_dev, &iio_pollfunc_store_time, NULL,
> > + NULL);
> > + if (rc) {
> > + dev_err(als_dev->log_dev, "failed to set up iio triggers: %d\n",
>
> just one trigger?
Also wrong as it's not actually setting up the trigger at all ;) It's setting
up the handling for what to do when a trigger event occurs.
>
> > + rc);
> > + goto free_iio_dev;
> > + }
> > +
> > + iio_trig = iio_trigger_alloc("%s-dev%d", iio_dev->name, iio_dev->id);
> > + if (!iio_trig) {
> > + rc = -ENOMEM;
> > + goto clean_trig_buf;
> > + }
> > +
> > + iio_trig->dev.parent = &als_dev->hid_dev->dev;
> > + iio_trig->ops = &appleals_trigger_ops;
> > + iio_trigger_set_drvdata(iio_trig, als_dev);
> > +
> > + rc = iio_trigger_register(iio_trig);
> > + if (rc) {
> > + dev_err(als_dev->log_dev, "failed to register iio trigger: %d\n",
>
> some messages start lowercase, some uppercase (nitpicking)
>
> > + rc);
> > + goto free_iio_trig;
> > + }
> > +
> > + als_dev->iio_trig = iio_trig;
> > +
> > + rc = iio_device_register(iio_dev);
> > + if (rc) {
> > + dev_err(als_dev->log_dev, "failed to register iio device: %d\n",
> > + rc);
> > + goto unreg_iio_trig;
> > + }
> > +
> > + als_dev->iio_dev = iio_dev;
I really don't like nest of pointers going on in here. I haven't dug
down to check if any of them can be remove, but they are definitely
ugly to deal with.
> > +
> > + return 0;
> > +
> > +unreg_iio_trig:
> > + iio_trigger_unregister(iio_trig);
> > +free_iio_trig:
> > + iio_trigger_free(iio_trig);
> > + als_dev->iio_trig = NULL;
> > +clean_trig_buf:
> > + iio_triggered_buffer_cleanup(iio_dev);
> > +free_iio_dev:
> > + iio_device_free(iio_dev);
> > +
> > + return rc;
> > +}
> > +
> > +static int appleals_probe(struct hid_device *hdev,
> > + const struct hid_device_id *id)
> > +{
> > + struct appleals_device *als_dev =
> > + appleib_get_drvdata(hid_get_drvdata(hdev),
> > + &appleals_hid_driver);
> > + struct hid_field *state_field;
> > + struct hid_field *illum_field;
> > + int rc;
> > +
> > + /* find als fields and reports */
> > + state_field = appleib_find_hid_field(hdev, HID_USAGE_SENSOR_ALS,
> > + HID_USAGE_SENSOR_PROP_REPORT_STATE);
> > + illum_field = appleib_find_hid_field(hdev, HID_USAGE_SENSOR_ALS,
> > + HID_USAGE_SENSOR_LIGHT_ILLUM);
> > + if (!state_field || !illum_field)
> > + return -ENODEV;
> > +
> > + if (als_dev->hid_dev) {
> > + dev_warn(als_dev->log_dev,
> > + "Found duplicate ambient light sensor - ignoring\n");
I'll bite. So how can this actually happen? What fundamentally breaks in
your model if there really are two? Can you fix whatever that is so
as to drop this handling?
> > + return -EBUSY;
> > + }
> > +
> > + dev_info(als_dev->log_dev, "Found ambient light sensor\n");
>
> in general avoid logging for the OK case, it just clutters the log
>
> > +
> > + /* initialize device */
> > + als_dev->hid_dev = hdev;
> > + als_dev->cfg_report = state_field->report;
> > + als_dev->illum_field = illum_field;
> > +
> > + als_dev->cur_hysteresis = APPLEALS_DEF_CHANGE_SENS;
> > + als_dev->cur_sensitivity = APPLEALS_DEF_CHANGE_SENS;
> > + appleals_config_sensor(als_dev, false, als_dev->cur_sensitivity);
> > +
> > + rc = appleals_config_iio(als_dev);
> > + if (rc)
> > + return rc;
> > +
> > + return 0;
return appleals_config_iio(als_dev);
> > +}
> > +
> > +static void appleals_remove(struct hid_device *hdev)
> > +{
> > + struct appleals_device *als_dev =
> > + appleib_get_drvdata(hid_get_drvdata(hdev),
> > + &appleals_hid_driver);
> > +
>
> could be a lot less if devm_ were used?
>
> > + if (als_dev->iio_dev) {
> > + iio_device_unregister(als_dev->iio_dev);
> > +
> > + iio_trigger_unregister(als_dev->iio_trig);
> > + iio_trigger_free(als_dev->iio_trig);
> > + als_dev->iio_trig = NULL;
I would be decidedly worried if you actually have any paths
where setting these to NULL do anything useful. By the time
we get here we should absolutely 'know' nothing will touch
these pointers.
It is these that are blocking Peter's suggestion of using
devm to clean all of this up automatically for you.
> > +
> > + iio_triggered_buffer_cleanup(als_dev->iio_dev);
> > + iio_device_free(als_dev->iio_dev);
> > + als_dev->iio_dev = NULL;
> > + }
> > +
> > + als_dev->hid_dev = NULL;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int appleals_reset_resume(struct hid_device *hdev)
> > +{
> > + struct appleals_device *als_dev =
> > + appleib_get_drvdata(hid_get_drvdata(hdev),
> > + &appleals_hid_driver);
> > +
> > + appleals_config_sensor(als_dev, als_dev->events_enabled,
> > + als_dev->cur_sensitivity);
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static struct hid_driver appleals_hid_driver = {
> > + .name = "apple-ib-als",
> > + .probe = appleals_probe,
> > + .remove = appleals_remove,
> > + .event = appleals_hid_event,
> > +#ifdef CONFIG_PM
> > + .reset_resume = appleals_reset_resume,
> > +#endif
> > +};
> > +
> > +static int appleals_platform_probe(struct platform_device *pdev)
> > +{
> > + struct appleib_platform_data *pdata = pdev->dev.platform_data;
> > + struct appleib_device *ib_dev = pdata->ib_dev;
> > + struct appleals_device *als_dev;
> > + int rc;
> > +
> > + als_dev = kzalloc(sizeof(*als_dev), GFP_KERNEL);
> > + if (!als_dev)
> > + return -ENOMEM;
> > +
> > + als_dev->ib_dev = ib_dev;
> > + als_dev->log_dev = pdata->log_dev;
> > +
> > + rc = appleib_register_hid_driver(ib_dev, &appleals_hid_driver, als_dev);
Hmm. This is all a bit odd. We register a platform device, to call it's driver
so that it can then register another driver? I'll let Lee comment on that but
it does seem overly complex.
> > + if (rc) {
> > + dev_err(als_dev->log_dev, "Error registering hid driver: %d\n",
> > + rc);
> > + goto error;
> > + }
> > +
> > + platform_set_drvdata(pdev, als_dev);
> > +
> > + return 0;
> > +
> > +error:
> > + kfree(als_dev);
> > + return rc;
> > +}
> > +
> > +static int appleals_platform_remove(struct platform_device *pdev)
> > +{
> > + struct appleib_platform_data *pdata = pdev->dev.platform_data;
> > + struct appleib_device *ib_dev = pdata->ib_dev;
> > + struct appleals_device *als_dev = platform_get_drvdata(pdev);
> > + int rc;
> > +
> > + rc = appleib_unregister_hid_driver(ib_dev, &appleals_hid_driver);
> > + if (rc) {
> > + dev_err(als_dev->log_dev,
> > + "Error unregistering hid driver: %d\n", rc);
Given you are going to have this error printing in every sub driver and
the text is totally generic, can you just move it into the core?
Same applies for similar cases above.
> > + goto error;
> > + }
> > +
> > + kfree(als_dev);
Use managed allocation to avoid needing to clean this up?
> > +
> > + return 0;
> > +
> > +error:
> > + return rc;
> > +}
> > +
> > +static const struct platform_device_id appleals_platform_ids[] = {
> > + { .name = PLAT_NAME_IB_ALS },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(platform, appleals_platform_ids);
> > +
> > +static struct platform_driver appleals_platform_driver = {
> > + .id_table = appleals_platform_ids,
> > + .driver = {
> > + .name = "apple-ib-als",
> > + },
> > + .probe = appleals_platform_probe,
> > + .remove = appleals_platform_remove,
> > +};
> > +
> > +module_platform_driver(appleals_platform_driver);
> > +
> > +MODULE_AUTHOR("Ronald Tschalär");
> > +MODULE_DESCRIPTION("Apple iBridge ALS driver");
> > +MODULE_LICENSE("GPL v2");
> >
>
^ permalink raw reply
* Re: [PATCH 1/3] mfd: apple-ibridge: Add Apple iBridge MFD driver.
From: Jonathan Cameron @ 2019-04-22 11:34 UTC (permalink / raw)
To: Ronald Tschalär
Cc: Jiri Kosina, Benjamin Tissoires, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Lee Jones,
linux-input, linux-iio, linux-kernel
In-Reply-To: <20190422031251.11968-2-ronald@innovation.ch>
On Sun, 21 Apr 2019 20:12:49 -0700
Ronald Tschalär <ronald@innovation.ch> wrote:
> The iBridge device provides access to several devices, including:
> - the Touch Bar
> - the iSight webcam
> - the light sensor
> - the fingerprint sensor
>
> This driver provides the core support for managing the iBridge device
> and the access to the underlying devices. In particular, since the
> functionality for the touch bar and light sensor is exposed via USB HID
> interfaces, and the same HID device is used for multiple functions, this
> driver provides a multiplexing layer that allows multiple HID drivers to
> be registered for a given HID device. This allows the touch bar and ALS
> driver to be separated out into their own modules.
>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch
Hi Ronald,
I've only taken a fairly superficial look at this. A few global
things to note though.
1. Please either use kernel-doc style for function descriptions, or
do not. Right now you are sort of half way there.
2. There is quite a complex nest of separate structures being allocated,
so think about whether they can be simplified. In particular
use of container_of macros can allow a lot of forwards and backwards
pointers to be dropped if you embed the various structures directly.
This obviously needs hid and mfd review though as neither is my
area of expertise!
Jonathan
>
> ---
> drivers/mfd/Kconfig | 15 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/apple-ibridge.c | 883 ++++++++++++++++++++++++++++++
> include/linux/mfd/apple-ibridge.h | 39 ++
> 4 files changed, 938 insertions(+)
> create mode 100644 drivers/mfd/apple-ibridge.c
> create mode 100644 include/linux/mfd/apple-ibridge.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 76f9909cf396..d55fa77faacf 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1916,5 +1916,20 @@ config RAVE_SP_CORE
> Select this to get support for the Supervisory Processor
> device found on several devices in RAVE line of hardware.
>
> +config MFD_APPLE_IBRIDGE
> + tristate "Apple iBridge chip"
> + depends on ACPI
> + depends on USB_HID
> + depends on X86 || COMPILE_TEST
> + select MFD_CORE
> + help
> + This MFD provides the core support for the Apple iBridge chip
> + found on recent MacBookPro's. The drivers for the Touch Bar
> + (apple-ib-tb) and light sensor (apple-ib-als) need to be
> + enabled separately.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apple-ibridge.
> +
> endmenu
> endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 12980a4ad460..c364e0e9d313 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -241,4 +241,5 @@ obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
> obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
> obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o
> +obj-$(CONFIG_MFD_APPLE_IBRIDGE) += apple-ibridge.o
>
> diff --git a/drivers/mfd/apple-ibridge.c b/drivers/mfd/apple-ibridge.c
> new file mode 100644
> index 000000000000..56d325396961
> --- /dev/null
> +++ b/drivers/mfd/apple-ibridge.c
> @@ -0,0 +1,883 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple iBridge Driver
> + *
> + * Copyright (c) 2018 Ronald Tschalär
> + */
> +
> +/**
> + * MacBookPro models with a Touch Bar (13,[23] and 14,[23]) have an Apple
> + * iBridge chip (also known as T1 chip) which exposes the touch bar,
> + * built-in webcam (iSight), ambient light sensor, and Secure Enclave
> + * Processor (SEP) for TouchID. It shows up in the system as a USB device
> + * with 3 configurations: 'Default iBridge Interfaces', 'Default iBridge
> + * Interfaces(OS X)', and 'Default iBridge Interfaces(Recovery)'. While
> + * the second one is used by MacOS to provide the fancy touch bar
> + * functionality with custom buttons etc, this driver just uses the first.
> + *
> + * In the first (default after boot) configuration, 4 usb interfaces are
> + * exposed: 2 related to the webcam, and 2 USB HID interfaces representing
> + * the touch bar and the ambient light sensor (and possibly the SEP,
> + * though at this point in time nothing is known about that). The webcam
> + * interfaces are already handled by the uvcvideo driver; furthermore, the
> + * handling of the input reports when "keys" on the touch bar are pressed
> + * is already handled properly by the generic USB HID core. This leaves
> + * the management of the touch bar modes (e.g. switching between function
> + * and special keys when the FN key is pressed), the touch bar display
> + * (dimming and turning off), the key-remapping when the FN key is
> + * pressed, and handling of the light sensor.
> + *
> + * This driver is implemented as an MFD driver, with the touch bar and ALS
> + * functions implemented by appropriate subdrivers (mfd cells). Because
> + * both those are basically hid drivers, but the current kernel driver
> + * structure does not allow more than one driver per device, this driver
> + * implements a demuxer for hid drivers: it registers itself as a hid
> + * driver with the core, and in turn it lets the subdrivers register
> + * themselves as hid drivers with this driver; the callbacks from the core
> + * are then forwarded to the subdrivers.
> + *
> + * Lastly, this driver also takes care of the power-management for the
> + * iBridge when suspending and resuming.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/list.h>
> +#include <linux/mfd/apple-ibridge.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/rculist.h>
> +#include <linux/slab.h>
> +#include <linux/srcu.h>
> +#include <linux/usb.h>
> +
> +#include "../hid/usbhid/usbhid.h"
> +
> +#define USB_ID_VENDOR_APPLE 0x05ac
> +#define USB_ID_PRODUCT_IBRIDGE 0x8600
> +
> +#define APPLETB_BASIC_CONFIG 1
> +
> +#define LOG_DEV(ib_dev) (&(ib_dev)->acpi_dev->dev)
> +
> +struct appleib_device {
> + struct acpi_device *acpi_dev;
> + acpi_handle asoc_socw;
> + struct list_head hid_drivers;
> + struct list_head hid_devices;
> + struct mfd_cell *subdevs;
> + struct mutex update_lock; /* protect updates to all lists */
> + struct srcu_struct lists_srcu;
> + bool in_hid_probe;
> +};
> +
> +struct appleib_hid_drv_info {
> + struct list_head entry;
> + struct hid_driver *driver;
> + void *driver_data;
> +};
> +
> +struct appleib_hid_dev_info {
> + struct list_head entry;
> + struct list_head drivers;
> + struct hid_device *device;
> + const struct hid_device_id *device_id;
> + bool started;
> +};
> +
> +static const struct mfd_cell appleib_subdevs[] = {
> + { .name = PLAT_NAME_IB_TB },
> + { .name = PLAT_NAME_IB_ALS },
> +};
> +
> +static struct appleib_device *appleib_dev;
> +
> +#define call_void_driver_func(drv_info, fn, ...) \
This sort of macro may seem like a good idea because it saves a few lines
of code. However, that comes at the cost of readability, so just
put the code inline.
> + do { \
> + if ((drv_info)->driver->fn) \
> + (drv_info)->driver->fn(__VA_ARGS__); \
> + } while (0)
> +
> +#define call_driver_func(drv_info, fn, ret_type, ...) \
> + ({ \
> + ret_type rc = 0; \
> + \
> + if ((drv_info)->driver->fn) \
> + rc = (drv_info)->driver->fn(__VA_ARGS__); \
> + \
> + rc; \
> + })
> +
> +static void appleib_remove_driver(struct appleib_device *ib_dev,
> + struct appleib_hid_drv_info *drv_info,
> + struct appleib_hid_dev_info *dev_info)
> +{
> + list_del_rcu(&drv_info->entry);
> + synchronize_srcu(&ib_dev->lists_srcu);
> +
> + call_void_driver_func(drv_info, remove, dev_info->device);
> +
> + kfree(drv_info);
> +}
> +
> +static int appleib_probe_driver(struct appleib_hid_drv_info *drv_info,
> + struct appleib_hid_dev_info *dev_info)
> +{
> + struct appleib_hid_drv_info *d;
> + int rc = 0;
> +
> + rc = call_driver_func(drv_info, probe, int, dev_info->device,
> + dev_info->device_id);
> + if (rc)
> + return rc;
> +
> + d = kmemdup(drv_info, sizeof(*drv_info), GFP_KERNEL);
> + if (!d) {
> + call_void_driver_func(drv_info, remove, dev_info->device);
> + return -ENOMEM;
> + }
> +
> + list_add_tail_rcu(&d->entry, &dev_info->drivers);
> + return 0;
> +}
> +
> +static void appleib_remove_driver_attachments(struct appleib_device *ib_dev,
> + struct appleib_hid_dev_info *dev_info,
> + struct hid_driver *driver)
> +{
> + struct appleib_hid_drv_info *drv_info;
> + struct appleib_hid_drv_info *tmp;
> +
> + list_for_each_entry_safe(drv_info, tmp, &dev_info->drivers, entry) {
> + if (!driver || drv_info->driver == driver)
> + appleib_remove_driver(ib_dev, drv_info, dev_info);
> + }
> +}
> +
> +/*
> + * Find all devices that are attached to this driver and detach them.
> + *
> + * Note: this must be run with update_lock held.
> + */
> +static void appleib_detach_devices(struct appleib_device *ib_dev,
> + struct hid_driver *driver)
> +{
> + struct appleib_hid_dev_info *dev_info;
> +
> + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry)
> + appleib_remove_driver_attachments(ib_dev, dev_info, driver);
> +}
> +
> +/*
> + * Find all drivers that are attached to this device and detach them.
> + *
> + * Note: this must be run with update_lock held.
> + */
> +static void appleib_detach_drivers(struct appleib_device *ib_dev,
> + struct appleib_hid_dev_info *dev_info)
> +{
> + appleib_remove_driver_attachments(ib_dev, dev_info, NULL);
> +}
> +
> +/**
> + * Unregister a previously registered HID driver from us.
> + * @ib_dev: the appleib_device from which to unregister the driver
> + * @driver: the driver to unregister
> + */
> +int appleib_unregister_hid_driver(struct appleib_device *ib_dev,
> + struct hid_driver *driver)
> +{
> + struct appleib_hid_drv_info *drv_info;
> +
> + mutex_lock(&ib_dev->update_lock);
> +
> + list_for_each_entry(drv_info, &ib_dev->hid_drivers, entry) {
> + if (drv_info->driver == driver) {
This block does look like it perhaps should be in helper function?
Would help with readability.
> + appleib_detach_devices(ib_dev, driver);
> + list_del_rcu(&drv_info->entry);
> + mutex_unlock(&ib_dev->update_lock);
> + synchronize_srcu(&ib_dev->lists_srcu);
> + kfree(drv_info);
> + dev_info(LOG_DEV(ib_dev), "unregistered driver '%s'\n",
> + driver->name);
> + return 0;
> + }
> + }
> +
> + mutex_unlock(&ib_dev->update_lock);
> +
> + return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(appleib_unregister_hid_driver);
> +
> +static int appleib_start_hid_events(struct appleib_hid_dev_info *dev_info)
> +{
> + struct hid_device *hdev = dev_info->device;
> + int rc;
> +
> + rc = hid_connect(hdev, HID_CONNECT_DEFAULT);
> + if (rc) {
> + hid_err(hdev, "ib: hid connect failed (%d)\n", rc);
> + return rc;
> + }
> +
> + rc = hid_hw_open(hdev);
> + if (rc) {
> + hid_err(hdev, "ib: failed to open hid: %d\n", rc);
> + hid_disconnect(hdev);
> + }
> +
> + if (!rc)
> + dev_info->started = true;
> +
> + return rc;
> +}
> +
> +static void appleib_stop_hid_events(struct appleib_hid_dev_info *dev_info)
> +{
> + if (dev_info->started) {
> + hid_hw_close(dev_info->device);
> + hid_disconnect(dev_info->device);
> + dev_info->started = false;
> + }
> +}
> +
> +/**
> + * Register a HID driver with us.
> + * @ib_dev: the appleib_device with which to register the driver
> + * @driver: the driver to register
> + * @data: the driver-data to associate with the driver; this is available
> + * from appleib_get_drvdata(...).
> + */
> +int appleib_register_hid_driver(struct appleib_device *ib_dev,
> + struct hid_driver *driver, void *data)
> +{
> + struct appleib_hid_drv_info *drv_info;
> + struct appleib_hid_dev_info *dev_info;
> + int rc;
> +
> + if (!driver->probe)
> + return -EINVAL;
> +
> + drv_info = kzalloc(sizeof(*drv_info), GFP_KERNEL);
> + if (!drv_info)
> + return -ENOMEM;
> +
> + drv_info->driver = driver;
> + drv_info->driver_data = data;
> +
> + mutex_lock(&ib_dev->update_lock);
> +
> + list_add_tail_rcu(&drv_info->entry, &ib_dev->hid_drivers);
> +
> + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) {
> + appleib_stop_hid_events(dev_info);
> +
> + appleib_probe_driver(drv_info, dev_info);
> +
> + rc = appleib_start_hid_events(dev_info);
> + if (rc)
> + appleib_detach_drivers(ib_dev, dev_info);
> + }
> +
> + mutex_unlock(&ib_dev->update_lock);
> +
> + dev_info(LOG_DEV(ib_dev), "registered driver '%s'\n", driver->name);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(appleib_register_hid_driver);
> +
> +/**
> + * Get the driver-specific data associated with the given, previously
> + * registered HID driver (provided in the appleib_register_hid_driver()
> + * call).
> + * @ib_dev: the appleib_device with which the driver was registered
> + * @driver: the driver for which to get the data
> + */
> +void *appleib_get_drvdata(struct appleib_device *ib_dev,
> + struct hid_driver *driver)
> +{
> + struct appleib_hid_drv_info *drv_info;
> + void *drv_data = NULL;
> + int idx;
> +
> + idx = srcu_read_lock(&ib_dev->lists_srcu);
> +
> + list_for_each_entry_rcu(drv_info, &ib_dev->hid_drivers, entry) {
> + if (drv_info->driver == driver) {
> + drv_data = drv_info->driver_data;
> + break;
> + }
> + }
> +
> + srcu_read_unlock(&ib_dev->lists_srcu, idx);
> +
> + return drv_data;
> +}
> +EXPORT_SYMBOL_GPL(appleib_get_drvdata);
> +
> +/*
> + * Forward a hid-driver callback to all registered sub-drivers. This is for
> + * callbacks that return a status as an int.
> + * @hdev the hid-device
> + * @forward a function that calls the callback on the given driver
> + * @args arguments for the forward function
> + */
> +static int appleib_forward_int_op(struct hid_device *hdev,
> + int (*forward)(struct appleib_hid_drv_info *,
> + struct hid_device *, void *),
> + void *args)
> +{
> + struct appleib_device *ib_dev = hid_get_drvdata(hdev);
> + struct appleib_hid_dev_info *dev_info;
> + struct appleib_hid_drv_info *drv_info;
> + int idx;
> + int rc = 0;
> +
> + idx = srcu_read_lock(&ib_dev->lists_srcu);
> +
> + list_for_each_entry_rcu(dev_info, &ib_dev->hid_devices, entry) {
> + if (dev_info->device != hdev)
> + continue;
> +
> + list_for_each_entry_rcu(drv_info, &dev_info->drivers, entry) {
> + rc = forward(drv_info, hdev, args);
> + if (rc)
> + break;
> + }
> +
> + break;
> + }
> +
> + srcu_read_unlock(&ib_dev->lists_srcu, idx);
> +
> + return rc;
> +}
> +
> +struct appleib_hid_event_args {
> + struct hid_field *field;
> + struct hid_usage *usage;
> + __s32 value;
> +};
> +
> +static int appleib_hid_event_fwd(struct appleib_hid_drv_info *drv_info,
> + struct hid_device *hdev, void *args)
> +{
> + struct appleib_hid_event_args *evt_args = args;
> +
> + return call_driver_func(drv_info, event, int, hdev, evt_args->field,
> + evt_args->usage, evt_args->value);
> +}
> +
> +static int appleib_hid_event(struct hid_device *hdev, struct hid_field *field,
> + struct hid_usage *usage, __s32 value)
> +{
> + struct appleib_hid_event_args args = {
> + .field = field,
> + .usage = usage,
> + .value = value,
> + };
> +
> + return appleib_forward_int_op(hdev, appleib_hid_event_fwd, &args);
> +}
> +
> +static __u8 *appleib_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> + unsigned int *rsize)
> +{
> + /* Some fields have a size of 64 bits, which according to HID 1.11
> + * Section 8.4 is not valid ("An item field cannot span more than 4
> + * bytes in a report"). Furthermore, hid_field_extract() complains
> + * when encountering such a field. So turn them into two 32-bit fields
> + * instead.
> + */
> +
> + if (*rsize == 634 &&
> + /* Usage Page 0xff12 (vendor defined) */
> + rdesc[212] == 0x06 && rdesc[213] == 0x12 && rdesc[214] == 0xff &&
> + /* Usage 0x51 */
> + rdesc[416] == 0x09 && rdesc[417] == 0x51 &&
> + /* report size 64 */
> + rdesc[432] == 0x75 && rdesc[433] == 64 &&
> + /* report count 1 */
> + rdesc[434] == 0x95 && rdesc[435] == 1) {
> + rdesc[433] = 32;
> + rdesc[435] = 2;
> + hid_dbg(hdev, "Fixed up first 64-bit field\n");
> + }
> +
> + if (*rsize == 634 &&
> + /* Usage Page 0xff12 (vendor defined) */
> + rdesc[212] == 0x06 && rdesc[213] == 0x12 && rdesc[214] == 0xff &&
> + /* Usage 0x51 */
> + rdesc[611] == 0x09 && rdesc[612] == 0x51 &&
> + /* report size 64 */
> + rdesc[627] == 0x75 && rdesc[628] == 64 &&
> + /* report count 1 */
> + rdesc[629] == 0x95 && rdesc[630] == 1) {
> + rdesc[628] = 32;
> + rdesc[630] = 2;
> + hid_dbg(hdev, "Fixed up second 64-bit field\n");
> + }
> +
> + return rdesc;
> +}
> +
> +static int appleib_input_configured_fwd(struct appleib_hid_drv_info *drv_info,
> + struct hid_device *hdev, void *args)
> +{
> + return call_driver_func(drv_info, input_configured, int, hdev,
> + (struct hid_input *)args);
> +}
> +
> +static int appleib_input_configured(struct hid_device *hdev,
> + struct hid_input *hidinput)
> +{
> + return appleib_forward_int_op(hdev, appleib_input_configured_fwd,
> + hidinput);
> +}
> +
> +#ifdef CONFIG_PM
> +static int appleib_hid_suspend_fwd(struct appleib_hid_drv_info *drv_info,
> + struct hid_device *hdev, void *args)
> +{
> + return call_driver_func(drv_info, suspend, int, hdev,
> + *(pm_message_t *)args);
> +}
> +
> +static int appleib_hid_suspend(struct hid_device *hdev, pm_message_t message)
> +{
> + return appleib_forward_int_op(hdev, appleib_hid_suspend_fwd, &message);
> +}
> +
> +static int appleib_hid_resume_fwd(struct appleib_hid_drv_info *drv_info,
> + struct hid_device *hdev, void *args)
> +{
> + return call_driver_func(drv_info, resume, int, hdev);
> +}
> +
> +static int appleib_hid_resume(struct hid_device *hdev)
> +{
> + return appleib_forward_int_op(hdev, appleib_hid_resume_fwd, NULL);
> +}
> +
> +static int appleib_hid_reset_resume_fwd(struct appleib_hid_drv_info *drv_info,
> + struct hid_device *hdev, void *args)
> +{
> + return call_driver_func(drv_info, reset_resume, int, hdev);
> +}
> +
> +static int appleib_hid_reset_resume(struct hid_device *hdev)
> +{
> + return appleib_forward_int_op(hdev, appleib_hid_reset_resume_fwd, NULL);
> +}
> +#endif /* CONFIG_PM */
> +
> +/**
> + * Find the field in the report with the given usage.
> + * @report: the report to search
> + * @field_usage: the usage of the field to search for
> + */
> +struct hid_field *appleib_find_report_field(struct hid_report *report,
> + unsigned int field_usage)
> +{
> + int f, u;
> +
> + for (f = 0; f < report->maxfield; f++) {
> + struct hid_field *field = report->field[f];
> +
> + if (field->logical == field_usage)
> + return field;
> +
> + for (u = 0; u < field->maxusage; u++) {
> + if (field->usage[u].hid == field_usage)
> + return field;
> + }
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(appleib_find_report_field);
> +
> +/**
Please use correct kernel-doc style rather than parts of it.
> + * Search all the reports of the device for the field with the given usage.
> + * @hdev: the device whose reports to search
> + * @application: the usage of application collection that the field must
> + * belong to
> + * @field_usage: the usage of the field to search for
> + */
> +struct hid_field *appleib_find_hid_field(struct hid_device *hdev,
> + unsigned int application,
> + unsigned int field_usage)
> +{
> + static const int report_types[] = { HID_INPUT_REPORT, HID_OUTPUT_REPORT,
> + HID_FEATURE_REPORT };
> + struct hid_report *report;
> + struct hid_field *field;
> + int t;
> +
> + for (t = 0; t < ARRAY_SIZE(report_types); t++) {
> + struct list_head *report_list =
> + &hdev->report_enum[report_types[t]].report_list;
> + list_for_each_entry(report, report_list, list) {
> + if (report->application != application)
> + continue;
> +
> + field = appleib_find_report_field(report, field_usage);
> + if (field)
> + return field;
> + }
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(appleib_find_hid_field);
> +
> +/**
> + * Return whether we're currently inside a hid_device_probe or not.
> + * @ib_dev: the appleib_device
> + */
> +bool appleib_in_hid_probe(struct appleib_device *ib_dev)
> +{
> + return ib_dev->in_hid_probe;
> +}
> +EXPORT_SYMBOL_GPL(appleib_in_hid_probe);
> +
> +static struct appleib_hid_dev_info *
> +appleib_add_device(struct appleib_device *ib_dev, struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct appleib_hid_dev_info *dev_info;
> + struct appleib_hid_drv_info *drv_info;
> +
> + /* allocate device-info for this device */
> + dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL);
> + if (!dev_info)
> + return NULL;
> +
> + INIT_LIST_HEAD(&dev_info->drivers);
> + dev_info->device = hdev;
> + dev_info->device_id = id;
> +
> + /* notify all our sub drivers */
> + mutex_lock(&ib_dev->update_lock);
> +
This is interesting. I'd like to see a comment here on what
this flag is going to do.
> + ib_dev->in_hid_probe = true;
> +
> + list_for_each_entry(drv_info, &ib_dev->hid_drivers, entry) {
> + appleib_probe_driver(drv_info, dev_info);
> + }
> +
> + ib_dev->in_hid_probe = false;
> +
> + /* remember this new device */
> + list_add_tail_rcu(&dev_info->entry, &ib_dev->hid_devices);
> +
> + mutex_unlock(&ib_dev->update_lock);
> +
> + return dev_info;
> +}
> +
> +static void appleib_remove_device(struct appleib_device *ib_dev,
> + struct appleib_hid_dev_info *dev_info)
> +{
> + list_del_rcu(&dev_info->entry);
> + synchronize_srcu(&ib_dev->lists_srcu);
> +
> + appleib_detach_drivers(ib_dev, dev_info);
> +
> + kfree(dev_info);
> +}
> +
> +static int appleib_hid_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct appleib_device *ib_dev;
> + struct appleib_hid_dev_info *dev_info;
> + struct usb_device *udev;
> + int rc;
> +
> + /* check usb config first */
> + udev = hid_to_usb_dev(hdev);
> +
> + if (udev->actconfig->desc.bConfigurationValue != APPLETB_BASIC_CONFIG) {
> + rc = usb_driver_set_configuration(udev, APPLETB_BASIC_CONFIG);
> + return rc ? rc : -ENODEV;
> + }
> +
> + /* Assign the driver data */
> + ib_dev = appleib_dev;
> + hid_set_drvdata(hdev, ib_dev);
> +
> + /* initialize the report info */
> + rc = hid_parse(hdev);
> + if (rc) {
> + hid_err(hdev, "ib: hid parse failed (%d)\n", rc);
> + goto error;
> + }
> +
> + /* alloc bufs etc so probe's can send requests; but connect later */
> + rc = hid_hw_start(hdev, 0);
> + if (rc) {
> + hid_err(hdev, "ib: hw start failed (%d)\n", rc);
> + goto error;
> + }
> +
> + /* add this hdev to our device list */
> + dev_info = appleib_add_device(ib_dev, hdev, id);
> + if (!dev_info) {
> + rc = -ENOMEM;
> + goto stop_hw;
> + }
> +
> + /* start the hid */
> + rc = appleib_start_hid_events(dev_info);
> + if (rc)
> + goto remove_dev;
> +
> + return 0;
> +
> +remove_dev:
> + mutex_lock(&ib_dev->update_lock);
> + appleib_remove_device(ib_dev, dev_info);
> + mutex_unlock(&ib_dev->update_lock);
> +stop_hw:
> + hid_hw_stop(hdev);
> +error:
> + return rc;
> +}
> +
> +static void appleib_hid_remove(struct hid_device *hdev)
> +{
> + struct appleib_device *ib_dev = hid_get_drvdata(hdev);
> + struct appleib_hid_dev_info *dev_info;
> +
> + mutex_lock(&ib_dev->update_lock);
> +
> + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) {
> + if (dev_info->device == hdev) {
> + appleib_stop_hid_events(dev_info);
> + appleib_remove_device(ib_dev, dev_info);
> + break;
> + }
> + }
> +
> + mutex_unlock(&ib_dev->update_lock);
> +
> + hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id appleib_hid_devices[] = {
> + { HID_USB_DEVICE(USB_ID_VENDOR_APPLE, USB_ID_PRODUCT_IBRIDGE) },
> + { },
> +};
> +
> +static struct hid_driver appleib_hid_driver = {
> + .name = "apple-ibridge-hid",
> + .id_table = appleib_hid_devices,
> + .probe = appleib_hid_probe,
> + .remove = appleib_hid_remove,
> + .event = appleib_hid_event,
> + .report_fixup = appleib_report_fixup,
> + .input_configured = appleib_input_configured,
> +#ifdef CONFIG_PM
> + .suspend = appleib_hid_suspend,
> + .resume = appleib_hid_resume,
> + .reset_resume = appleib_hid_reset_resume,
> +#endif
> +};
> +
> +static struct appleib_device *appleib_alloc_device(struct acpi_device *acpi_dev)
> +{
> + struct appleib_device *ib_dev;
> + acpi_status sts;
> + int rc;
> +
> + /* allocate */
Drop comments that don't anything a quick glance at the code would tell you.
> + ib_dev = kzalloc(sizeof(*ib_dev), GFP_KERNEL);
> + if (!ib_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + /* init structures */
> + INIT_LIST_HEAD(&ib_dev->hid_drivers);
> + INIT_LIST_HEAD(&ib_dev->hid_devices);
> + mutex_init(&ib_dev->update_lock);
> + init_srcu_struct(&ib_dev->lists_srcu);
> +
> + ib_dev->acpi_dev = acpi_dev;
> +
> + /* get iBridge acpi power control method */
> + sts = acpi_get_handle(acpi_dev->handle, "SOCW", &ib_dev->asoc_socw);
> + if (ACPI_FAILURE(sts)) {
> + dev_err(LOG_DEV(ib_dev),
> + "Error getting handle for ASOC.SOCW method: %s\n",
> + acpi_format_exception(sts));
> + rc = -ENXIO;
> + goto free_mem;
> + }
> +
> + /* ensure iBridge is powered on */
> + sts = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1);
> + if (ACPI_FAILURE(sts))
> + dev_warn(LOG_DEV(ib_dev), "SOCW(1) failed: %s\n",
> + acpi_format_exception(sts));
> +
> + return ib_dev;
> +
> +free_mem:
> + kfree(ib_dev);
> + return ERR_PTR(rc);
> +}
> +
> +static int appleib_probe(struct acpi_device *acpi)
> +{
> + struct appleib_device *ib_dev;
> + struct appleib_platform_data *pdata;
Platform_data has a lot of historical meaning in Linux.
Also you have things in here that are not platform related
at all, such as the dev pointer. Hence I would rename it
as device_data or private or something like that.
> + int i;
> + int ret;
> +
> + if (appleib_dev)
This singleton bothers me a bit. I'm really not sure why it
is necessary. You can just put a pointer to this in
the pdata for the subdevs and I think that covers most of your
usecases. It's generally a bad idea to limit things to one instance
of a device unless that actually major simplifications.
I'm not seeing them here.
> + return -EBUSY;
> +
> + ib_dev = appleib_alloc_device(acpi);
> + if (IS_ERR_OR_NULL(ib_dev))
> + return PTR_ERR(ib_dev);
> +
> + ib_dev->subdevs = kmemdup(appleib_subdevs, sizeof(appleib_subdevs),
> + GFP_KERNEL);
Given this is fixed sized and always referenced via ib_dev->subdevs, just
put the array in there and memcpy into it. That way you have one less
allocation and simpler code.
> + if (!ib_dev->subdevs) {
> + ret = -ENOMEM;
> + goto free_dev;
> + }
> +
> + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
Might as well embed this in ib_dev as well. That would let
you used container_of to avoid having to carry the ib_dev pointer
around in side pdata.
> + if (!pdata) {
> + ret = -ENOMEM;
> + goto free_subdevs;
> + }
> +
> + pdata->ib_dev = ib_dev;
> + pdata->log_dev = LOG_DEV(ib_dev);
> + for (i = 0; i < ARRAY_SIZE(appleib_subdevs); i++) {
> + ib_dev->subdevs[i].platform_data = pdata;
> + ib_dev->subdevs[i].pdata_size = sizeof(*pdata);
> + }
> +
> + ret = mfd_add_devices(&acpi->dev, PLATFORM_DEVID_NONE,
> + ib_dev->subdevs, ARRAY_SIZE(appleib_subdevs),
> + NULL, 0, NULL);
> + if (ret) {
> + dev_err(LOG_DEV(ib_dev), "Error adding MFD devices: %d\n", ret);
> + goto free_pdata;
> + }
> +
> + acpi->driver_data = ib_dev;
> + appleib_dev = ib_dev;
> +
> + ret = hid_register_driver(&appleib_hid_driver);
> + if (ret) {
> + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n",
> + ret);
> + goto rem_mfd_devs;
> + }
> +
> + return 0;
> +
> +rem_mfd_devs:
> + mfd_remove_devices(&acpi->dev);
> +free_pdata:
> + kfree(pdata);
> +free_subdevs:
> + kfree(ib_dev->subdevs);
> +free_dev:
> + appleib_dev = NULL;
> + acpi->driver_data = NULL;
Why at this point? It's not set to anything until much later in the
probe flow. May be worth thinking about devm_ managed allocations
to cleanup some of these allocations automatically and simplify
the error handling.
> + kfree(ib_dev);
> + return ret;
> +}
> +
> +static int appleib_remove(struct acpi_device *acpi)
> +{
> + struct appleib_device *ib_dev = acpi_driver_data(acpi);
> +
> + mfd_remove_devices(&acpi->dev);
> + hid_unregister_driver(&appleib_hid_driver);
> +
> + if (appleib_dev == ib_dev)
From a general reviewability point of view, it's nice to
keep the remove in the same order as the cleanup on
error in probe (and hence reverse of probe). That measn
this should be a little further down.
I'd also like to see a comment on how this condition can be
false.
> + appleib_dev = NULL;
> +
> + kfree(ib_dev->subdevs[0].platform_data);
> + kfree(ib_dev->subdevs);
> + kfree(ib_dev);
Is it worth considering devm in here to avoid the need to
clean all these up by hand?
> +
> + return 0;
> +}
> +
> +static int appleib_suspend(struct device *dev)
> +{
> + struct acpi_device *adev;
> + struct appleib_device *ib_dev;
> + int rc;
> +
> + adev = to_acpi_device(dev);
> + ib_dev = acpi_driver_data(adev);
Given this appears a few times, probably worth the more compact
ib_dev = acpi_driver_data(to_acpi_device(dev));
Allowing you to drop the adev local variable that doesn't add
any info.
> +
> + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> + if (ACPI_FAILURE(rc))
> + dev_warn(LOG_DEV(ib_dev), "SOCW(0) failed: %s\n",
I can sort of see you might want to do the LOG_DEV for consistency
but here I'm fairly sure it's just dev which might be clearer.
> + acpi_format_exception(rc));
> +
> + return 0;
> +}
> +
> +static int appleib_resume(struct device *dev)
> +{
> + struct acpi_device *adev;
> + struct appleib_device *ib_dev;
> + int rc;
> +
> + adev = to_acpi_device(dev);
> + ib_dev = acpi_driver_data(adev);
> +
> + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1);
> + if (ACPI_FAILURE(rc))
> + dev_warn(LOG_DEV(ib_dev), "SOCW(1) failed: %s\n",
> + acpi_format_exception(rc));
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops appleib_pm = {
> + .suspend = appleib_suspend,
> + .resume = appleib_resume,
> + .restore = appleib_resume,
> +};
> +
> +static const struct acpi_device_id appleib_acpi_match[] = {
> + { "APP7777", 0 },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, appleib_acpi_match);
> +
> +static struct acpi_driver appleib_driver = {
> + .name = "apple-ibridge",
> + .class = "topcase", /* ? */
> + .owner = THIS_MODULE,
> + .ids = appleib_acpi_match,
> + .ops = {
> + .add = appleib_probe,
> + .remove = appleib_remove,
> + },
> + .drv = {
> + .pm = &appleib_pm,
> + },
> +};
> +
> +module_acpi_driver(appleib_driver)
> +
> +MODULE_AUTHOR("Ronald Tschalär");
> +MODULE_DESCRIPTION("Apple iBridge driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/apple-ibridge.h b/include/linux/mfd/apple-ibridge.h
> new file mode 100644
> index 000000000000..d321714767f7
> --- /dev/null
> +++ b/include/linux/mfd/apple-ibridge.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Apple iBridge Driver
> + *
> + * Copyright (c) 2018 Ronald Tschalär
> + */
> +
> +#ifndef __LINUX_MFD_APPLE_IBRDIGE_H
> +#define __LINUX_MFD_APPLE_IBRDIGE_H
> +
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +
> +#define PLAT_NAME_IB_TB "apple-ib-tb"
> +#define PLAT_NAME_IB_ALS "apple-ib-als"
> +
> +struct appleib_device;
> +
> +struct appleib_platform_data {
> + struct appleib_device *ib_dev;
> + struct device *log_dev;
> +};
> +
> +int appleib_register_hid_driver(struct appleib_device *ib_dev,
> + struct hid_driver *driver, void *data);
> +int appleib_unregister_hid_driver(struct appleib_device *ib_dev,
> + struct hid_driver *driver);
> +
> +void *appleib_get_drvdata(struct appleib_device *ib_dev,
> + struct hid_driver *driver);
> +bool appleib_in_hid_probe(struct appleib_device *ib_dev);
> +
> +struct hid_field *appleib_find_report_field(struct hid_report *report,
> + unsigned int field_usage);
> +struct hid_field *appleib_find_hid_field(struct hid_device *hdev,
> + unsigned int application,
> + unsigned int field_usage);
> +
> +#endif
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox