* [PATCH v3 1/5] dt-bindings: input: Add documentation for IBM Operation Panel
2020-09-09 20:30 [PATCH v3 0/5] input: misc: Add IBM Operation Panel driver Eddie James
@ 2020-09-09 20:30 ` Eddie James
2020-09-09 20:30 ` [PATCH v3 2/5] input: misc: Add IBM Operation Panel driver Eddie James
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Eddie James @ 2020-09-09 20:30 UTC (permalink / raw)
To: linux-input
Cc: devicetree, linux-aspeed, linux-i2c, linux-kernel, robh+dt,
dmitry.torokhov, joel, andrew, benh, brendanhiggins, wsa,
rentao.bupt, ryan_chen
Document the bindings for the IBM Operation Panel, which provides
a simple interface to control a server. It has a display and three
buttons.
Also update MAINTAINERS for the new file.
Signed-off-by: Eddie James <eajames@linux.ibm.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Joel Stanley <joel@jms.id.au>
---
.../bindings/input/ibm,op-panel.yaml | 41 +++++++++++++++++++
MAINTAINERS | 6 +++
2 files changed, 47 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/ibm,op-panel.yaml
diff --git a/Documentation/devicetree/bindings/input/ibm,op-panel.yaml b/Documentation/devicetree/bindings/input/ibm,op-panel.yaml
new file mode 100644
index 000000000000..52c4a6275a77
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/ibm,op-panel.yaml
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/ibm,op-panel.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: IBM Operation Panel
+
+maintainers:
+ - Eddie James <eajames@linux.ibm.com>
+
+description: |
+ The IBM Operation Panel provides a simple interface to control the connected
+ server. It has a display and three buttons: two directional arrows and one
+ 'Enter' button.
+
+properties:
+ compatible:
+ const: ibm,op-panel
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/i2c/i2c.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ibm-op-panel@62 {
+ compatible = "ibm,op-panel";
+ reg = <(0x62 | I2C_OWN_SLAVE_ADDRESS)>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 3c0692404907..28408d29d873 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8346,6 +8346,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/aegl/linux.git
F: Documentation/ia64/
F: arch/ia64/
+IBM Operation Panel Input Driver
+M: Eddie James <eajames@linux.ibm.com>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/input/ibm,op-panel.yaml
+
IBM Power 842 compression accelerator
M: Haren Myneni <haren@us.ibm.com>
S: Supported
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 2/5] input: misc: Add IBM Operation Panel driver
2020-09-09 20:30 [PATCH v3 0/5] input: misc: Add IBM Operation Panel driver Eddie James
2020-09-09 20:30 ` [PATCH v3 1/5] dt-bindings: input: Add documentation for IBM Operation Panel Eddie James
@ 2020-09-09 20:30 ` Eddie James
2020-09-10 6:13 ` Wolfram Sang
2020-09-13 17:17 ` Dmitry Torokhov
2020-09-09 20:30 ` [PATCH v3 3/5] i2c: aspeed: Mask IRQ status to relevant bits Eddie James
` (2 subsequent siblings)
4 siblings, 2 replies; 14+ messages in thread
From: Eddie James @ 2020-09-09 20:30 UTC (permalink / raw)
To: linux-input
Cc: devicetree, linux-aspeed, linux-i2c, linux-kernel, robh+dt,
dmitry.torokhov, joel, andrew, benh, brendanhiggins, wsa,
rentao.bupt, ryan_chen
Add a driver to get the button events from the panel and provide
them to userspace with the input subsystem. The panel is
connected with I2C and controls the bus, so the driver registers
as an I2C slave device.
Signed-off-by: Eddie James <eajames@linux.ibm.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
MAINTAINERS | 1 +
drivers/input/misc/Kconfig | 18 ++++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/ibm-panel.c | 189 +++++++++++++++++++++++++++++++++
4 files changed, 209 insertions(+)
create mode 100644 drivers/input/misc/ibm-panel.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 28408d29d873..5429da957a1a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8351,6 +8351,7 @@ M: Eddie James <eajames@linux.ibm.com>
L: linux-input@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/input/ibm,op-panel.yaml
+F: drivers/input/misc/ibm-panel.c
IBM Power 842 compression accelerator
M: Haren Myneni <haren@us.ibm.com>
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 362e8a01980c..65ab1ce7b259 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -708,6 +708,24 @@ config INPUT_ADXL34X_SPI
To compile this driver as a module, choose M here: the
module will be called adxl34x-spi.
+config INPUT_IBM_PANEL
+ tristate "IBM Operation Panel driver"
+ depends on I2C_SLAVE || COMPILE_TEST
+ help
+ Say Y here if you have an IBM Operation Panel connected to your system
+ over I2C. The panel is typically connected only to a system's service
+ processor (BMC).
+
+ If unsure, say N.
+
+ The Operation Panel is a controller with some buttons and an LCD
+ display that allows someone with physical access to the system to
+ perform various administrative tasks. This driver only supports the part
+ of the controller that sends commands to the system.
+
+ To compile this driver as a module, choose M here: the module will be
+ called ibm-panel.
+
config INPUT_IMS_PCU
tristate "IMS Passenger Control Unit driver"
depends on USB
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index a48e5f2d859d..7e9edf0a142b 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_INPUT_GPIO_DECODER) += gpio_decoder.o
obj-$(CONFIG_INPUT_GPIO_VIBRA) += gpio-vibra.o
obj-$(CONFIG_INPUT_HISI_POWERKEY) += hisi_powerkey.o
obj-$(CONFIG_HP_SDC_RTC) += hp_sdc_rtc.o
+obj-$(CONFIG_INPUT_IBM_PANEL) += ibm-panel.o
obj-$(CONFIG_INPUT_IMS_PCU) += ims-pcu.o
obj-$(CONFIG_INPUT_IQS269A) += iqs269a.o
obj-$(CONFIG_INPUT_IXP4XX_BEEPER) += ixp4xx-beeper.o
diff --git a/drivers/input/misc/ibm-panel.c b/drivers/input/misc/ibm-panel.c
new file mode 100644
index 000000000000..7329f4641636
--- /dev/null
+++ b/drivers/input/misc/ibm-panel.c
@@ -0,0 +1,189 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) IBM Corporation 2020
+ */
+
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/kernel.h>
+#include <linux/limits.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/spinlock.h>
+
+#define DEVICE_NAME "ibm-panel"
+
+struct ibm_panel {
+ u8 idx;
+ u8 command[11];
+ spinlock_t lock; /* protects writes to idx and command */
+ struct input_dev *input;
+};
+
+static void ibm_panel_process_command(struct ibm_panel *panel)
+{
+ u8 i;
+ u8 chksum;
+ u16 sum = 0;
+ int pressed;
+ int released;
+
+ if (panel->command[0] != 0xff && panel->command[1] != 0xf0) {
+ dev_dbg(&panel->input->dev, "command invalid\n");
+ return;
+ }
+
+ for (i = 0; i < sizeof(panel->command) - 1; ++i) {
+ sum += panel->command[i];
+ if (sum & 0xff00) {
+ sum &= 0xff;
+ sum++;
+ }
+ }
+
+ chksum = sum & 0xff;
+ chksum = ~chksum;
+ chksum++;
+
+ if (chksum != panel->command[sizeof(panel->command) - 1]) {
+ dev_dbg(&panel->input->dev, "command failed checksum\n");
+ return;
+ }
+
+ released = panel->command[2] & 0x80;
+ pressed = released ? 0 : 1;
+
+ switch (panel->command[2] & 0xf) {
+ case 0:
+ input_report_key(panel->input, BTN_NORTH, pressed);
+ break;
+ case 1:
+ input_report_key(panel->input, BTN_SOUTH, pressed);
+ break;
+ case 2:
+ input_report_key(panel->input, BTN_SELECT, pressed);
+ break;
+ default:
+ dev_dbg(&panel->input->dev, "unknown command %u\n",
+ panel->command[2] & 0xf);
+ return;
+ }
+
+ input_sync(panel->input);
+}
+
+static int ibm_panel_i2c_slave_cb(struct i2c_client *client,
+ enum i2c_slave_event event, u8 *val)
+{
+ unsigned long flags;
+ struct ibm_panel *panel = i2c_get_clientdata(client);
+
+ dev_dbg(&panel->input->dev, "event: %u data: %02x\n", event, *val);
+
+ spin_lock_irqsave(&panel->lock, flags);
+
+ switch (event) {
+ case I2C_SLAVE_STOP:
+ if (panel->idx == sizeof(panel->command))
+ ibm_panel_process_command(panel);
+ else
+ dev_dbg(&panel->input->dev,
+ "command incorrect size %u\n", panel->idx);
+ fallthrough;
+ case I2C_SLAVE_WRITE_REQUESTED:
+ panel->idx = 0;
+ break;
+ case I2C_SLAVE_WRITE_RECEIVED:
+ if (panel->idx < sizeof(panel->command))
+ panel->command[panel->idx++] = *val;
+ else
+ /*
+ * The command is too long and therefore invalid, so set the index
+ * to it's largest possible value. When a STOP is finally received,
+ * the command will be rejected upon processing.
+ */
+ panel->idx = U8_MAX;
+ break;
+ case I2C_SLAVE_READ_REQUESTED:
+ case I2C_SLAVE_READ_PROCESSED:
+ *val = 0xff;
+ break;
+ default:
+ break;
+ }
+
+ spin_unlock_irqrestore(&panel->lock, flags);
+
+ return 0;
+}
+
+static int ibm_panel_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ int rc;
+ struct ibm_panel *panel = devm_kzalloc(&client->dev, sizeof(*panel),
+ GFP_KERNEL);
+
+ if (!panel)
+ return -ENOMEM;
+
+ panel->input = devm_input_allocate_device(&client->dev);
+ if (!panel->input)
+ return -ENOMEM;
+
+ panel->input->name = client->name;
+ panel->input->id.bustype = BUS_I2C;
+ input_set_capability(panel->input, EV_KEY, BTN_NORTH);
+ input_set_capability(panel->input, EV_KEY, BTN_SOUTH);
+ input_set_capability(panel->input, EV_KEY, BTN_SELECT);
+
+ rc = input_register_device(panel->input);
+ if (rc) {
+ dev_err(&client->dev, "Failed to register input device: %d\n",
+ rc);
+ return rc;
+ }
+
+ spin_lock_init(&panel->lock);
+
+ i2c_set_clientdata(client, panel);
+ rc = i2c_slave_register(client, ibm_panel_i2c_slave_cb);
+ if (rc) {
+ input_unregister_device(panel->input);
+ return rc;
+ }
+
+ return 0;
+}
+
+static int ibm_panel_remove(struct i2c_client *client)
+{
+ int rc;
+ struct ibm_panel *panel = i2c_get_clientdata(client);
+
+ rc = i2c_slave_unregister(client);
+
+ input_unregister_device(panel->input);
+
+ return rc;
+}
+
+static const struct of_device_id ibm_panel_match[] = {
+ { .compatible = "ibm,op-panel" },
+ { }
+};
+
+static struct i2c_driver ibm_panel_driver = {
+ .driver = {
+ .name = DEVICE_NAME,
+ .of_match_table = ibm_panel_match,
+ },
+ .probe = ibm_panel_probe,
+ .remove = ibm_panel_remove,
+};
+module_i2c_driver(ibm_panel_driver);
+
+MODULE_AUTHOR("Eddie James <eajames@linux.ibm.com>");
+MODULE_DESCRIPTION("IBM Operation Panel Driver");
+MODULE_LICENSE("GPL");
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 2/5] input: misc: Add IBM Operation Panel driver
2020-09-09 20:30 ` [PATCH v3 2/5] input: misc: Add IBM Operation Panel driver Eddie James
@ 2020-09-10 6:13 ` Wolfram Sang
2020-09-13 17:17 ` Dmitry Torokhov
1 sibling, 0 replies; 14+ messages in thread
From: Wolfram Sang @ 2020-09-10 6:13 UTC (permalink / raw)
To: Eddie James
Cc: linux-input, devicetree, linux-aspeed, linux-i2c, linux-kernel,
robh+dt, dmitry.torokhov, joel, andrew, benh, brendanhiggins,
rentao.bupt, ryan_chen
[-- Attachment #1: Type: text/plain, Size: 473 bytes --]
On Wed, Sep 09, 2020 at 03:30:56PM -0500, Eddie James wrote:
> Add a driver to get the button events from the panel and provide
> them to userspace with the input subsystem. The panel is
> connected with I2C and controls the bus, so the driver registers
> as an I2C slave device.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> # I2C slave parts
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/5] input: misc: Add IBM Operation Panel driver
2020-09-09 20:30 ` [PATCH v3 2/5] input: misc: Add IBM Operation Panel driver Eddie James
2020-09-10 6:13 ` Wolfram Sang
@ 2020-09-13 17:17 ` Dmitry Torokhov
1 sibling, 0 replies; 14+ messages in thread
From: Dmitry Torokhov @ 2020-09-13 17:17 UTC (permalink / raw)
To: Eddie James
Cc: linux-input, devicetree, linux-aspeed, linux-i2c, linux-kernel,
robh+dt, joel, andrew, benh, brendanhiggins, wsa, rentao.bupt,
ryan_chen
Hi Eddie,
On Wed, Sep 09, 2020 at 03:30:56PM -0500, Eddie James wrote:
> Add a driver to get the button events from the panel and provide
> them to userspace with the input subsystem. The panel is
> connected with I2C and controls the bus, so the driver registers
> as an I2C slave device.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> ---
> MAINTAINERS | 1 +
> drivers/input/misc/Kconfig | 18 ++++
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/ibm-panel.c | 189 +++++++++++++++++++++++++++++++++
> 4 files changed, 209 insertions(+)
> create mode 100644 drivers/input/misc/ibm-panel.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 28408d29d873..5429da957a1a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8351,6 +8351,7 @@ M: Eddie James <eajames@linux.ibm.com>
> L: linux-input@vger.kernel.org
> S: Maintained
> F: Documentation/devicetree/bindings/input/ibm,op-panel.yaml
> +F: drivers/input/misc/ibm-panel.c
>
> IBM Power 842 compression accelerator
> M: Haren Myneni <haren@us.ibm.com>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 362e8a01980c..65ab1ce7b259 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -708,6 +708,24 @@ config INPUT_ADXL34X_SPI
> To compile this driver as a module, choose M here: the
> module will be called adxl34x-spi.
>
> +config INPUT_IBM_PANEL
> + tristate "IBM Operation Panel driver"
> + depends on I2C_SLAVE || COMPILE_TEST
> + help
> + Say Y here if you have an IBM Operation Panel connected to your system
> + over I2C. The panel is typically connected only to a system's service
> + processor (BMC).
> +
> + If unsure, say N.
> +
> + The Operation Panel is a controller with some buttons and an LCD
> + display that allows someone with physical access to the system to
> + perform various administrative tasks. This driver only supports the part
> + of the controller that sends commands to the system.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called ibm-panel.
> +
> config INPUT_IMS_PCU
> tristate "IMS Passenger Control Unit driver"
> depends on USB
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index a48e5f2d859d..7e9edf0a142b 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_INPUT_GPIO_DECODER) += gpio_decoder.o
> obj-$(CONFIG_INPUT_GPIO_VIBRA) += gpio-vibra.o
> obj-$(CONFIG_INPUT_HISI_POWERKEY) += hisi_powerkey.o
> obj-$(CONFIG_HP_SDC_RTC) += hp_sdc_rtc.o
> +obj-$(CONFIG_INPUT_IBM_PANEL) += ibm-panel.o
> obj-$(CONFIG_INPUT_IMS_PCU) += ims-pcu.o
> obj-$(CONFIG_INPUT_IQS269A) += iqs269a.o
> obj-$(CONFIG_INPUT_IXP4XX_BEEPER) += ixp4xx-beeper.o
> diff --git a/drivers/input/misc/ibm-panel.c b/drivers/input/misc/ibm-panel.c
> new file mode 100644
> index 000000000000..7329f4641636
> --- /dev/null
> +++ b/drivers/input/misc/ibm-panel.c
> @@ -0,0 +1,189 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) IBM Corporation 2020
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/kernel.h>
> +#include <linux/limits.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/spinlock.h>
> +
> +#define DEVICE_NAME "ibm-panel"
> +
> +struct ibm_panel {
> + u8 idx;
> + u8 command[11];
> + spinlock_t lock; /* protects writes to idx and command */
> + struct input_dev *input;
> +};
> +
> +static void ibm_panel_process_command(struct ibm_panel *panel)
> +{
> + u8 i;
> + u8 chksum;
> + u16 sum = 0;
> + int pressed;
> + int released;
> +
> + if (panel->command[0] != 0xff && panel->command[1] != 0xf0) {
> + dev_dbg(&panel->input->dev, "command invalid\n");
> + return;
> + }
> +
> + for (i = 0; i < sizeof(panel->command) - 1; ++i) {
> + sum += panel->command[i];
> + if (sum & 0xff00) {
> + sum &= 0xff;
> + sum++;
> + }
> + }
> +
> + chksum = sum & 0xff;
> + chksum = ~chksum;
> + chksum++;
Maybe move checksum calculation into a separate function?
> +
> + if (chksum != panel->command[sizeof(panel->command) - 1]) {
> + dev_dbg(&panel->input->dev, "command failed checksum\n");
> + return;
> + }
> +
> + released = panel->command[2] & 0x80;
> + pressed = released ? 0 : 1;
pressed = !(panel->command[2] & BIT(7));
or "pressed = !released;" if you want to keep both.
> +
> + switch (panel->command[2] & 0xf) {
> + case 0:
> + input_report_key(panel->input, BTN_NORTH, pressed);
> + break;
> + case 1:
> + input_report_key(panel->input, BTN_SOUTH, pressed);
> + break;
> + case 2:
> + input_report_key(panel->input, BTN_SELECT, pressed);
> + break;
> + default:
> + dev_dbg(&panel->input->dev, "unknown command %u\n",
> + panel->command[2] & 0xf);
> + return;
> + }
> +
> + input_sync(panel->input);
> +}
> +
> +static int ibm_panel_i2c_slave_cb(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val)
> +{
> + unsigned long flags;
> + struct ibm_panel *panel = i2c_get_clientdata(client);
> +
> + dev_dbg(&panel->input->dev, "event: %u data: %02x\n", event, *val);
> +
> + spin_lock_irqsave(&panel->lock, flags);
> +
> + switch (event) {
> + case I2C_SLAVE_STOP:
> + if (panel->idx == sizeof(panel->command))
> + ibm_panel_process_command(panel);
> + else
> + dev_dbg(&panel->input->dev,
> + "command incorrect size %u\n", panel->idx);
> + fallthrough;
> + case I2C_SLAVE_WRITE_REQUESTED:
> + panel->idx = 0;
> + break;
> + case I2C_SLAVE_WRITE_RECEIVED:
> + if (panel->idx < sizeof(panel->command))
> + panel->command[panel->idx++] = *val;
> + else
> + /*
> + * The command is too long and therefore invalid, so set the index
> + * to it's largest possible value. When a STOP is finally received,
> + * the command will be rejected upon processing.
> + */
> + panel->idx = U8_MAX;
> + break;
> + case I2C_SLAVE_READ_REQUESTED:
> + case I2C_SLAVE_READ_PROCESSED:
> + *val = 0xff;
> + break;
> + default:
> + break;
> + }
> +
> + spin_unlock_irqrestore(&panel->lock, flags);
> +
> + return 0;
> +}
> +
> +static int ibm_panel_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + int rc;
> + struct ibm_panel *panel = devm_kzalloc(&client->dev, sizeof(*panel),
> + GFP_KERNEL);
> +
> + if (!panel)
> + return -ENOMEM;
> +
> + panel->input = devm_input_allocate_device(&client->dev);
> + if (!panel->input)
> + return -ENOMEM;
> +
> + panel->input->name = client->name;
> + panel->input->id.bustype = BUS_I2C;
> + input_set_capability(panel->input, EV_KEY, BTN_NORTH);
> + input_set_capability(panel->input, EV_KEY, BTN_SOUTH);
> + input_set_capability(panel->input, EV_KEY, BTN_SELECT);
North/South/Select are gamepad buttons, not general purpose ones. I
think you should not hard-code keymap in the driver, but rather use
device property to specify keymap that makes sense for the particular
board's application.
> +
> + rc = input_register_device(panel->input);
> + if (rc) {
> + dev_err(&client->dev, "Failed to register input device: %d\n",
> + rc);
> + return rc;
> + }
> +
> + spin_lock_init(&panel->lock);
> +
> + i2c_set_clientdata(client, panel);
> + rc = i2c_slave_register(client, ibm_panel_i2c_slave_cb);
> + if (rc) {
> + input_unregister_device(panel->input);
You are using devm, there is no need to manually unregister input
device.
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +static int ibm_panel_remove(struct i2c_client *client)
> +{
> + int rc;
> + struct ibm_panel *panel = i2c_get_clientdata(client);
> +
> + rc = i2c_slave_unregister(client);
> +
> + input_unregister_device(panel->input);
This is not needed.
> +
> + return rc;
The remove operation is not reversible, so there is no need to return
error here. Just log en error if i2c_slave_unregister() fails if you
want, and return 0.
> +}
> +
> +static const struct of_device_id ibm_panel_match[] = {
> + { .compatible = "ibm,op-panel" },
> + { }
> +};
> +
> +static struct i2c_driver ibm_panel_driver = {
> + .driver = {
> + .name = DEVICE_NAME,
> + .of_match_table = ibm_panel_match,
> + },
> + .probe = ibm_panel_probe,
> + .remove = ibm_panel_remove,
> +};
> +module_i2c_driver(ibm_panel_driver);
> +
> +MODULE_AUTHOR("Eddie James <eajames@linux.ibm.com>");
> +MODULE_DESCRIPTION("IBM Operation Panel Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.26.2
>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 3/5] i2c: aspeed: Mask IRQ status to relevant bits
2020-09-09 20:30 [PATCH v3 0/5] input: misc: Add IBM Operation Panel driver Eddie James
2020-09-09 20:30 ` [PATCH v3 1/5] dt-bindings: input: Add documentation for IBM Operation Panel Eddie James
2020-09-09 20:30 ` [PATCH v3 2/5] input: misc: Add IBM Operation Panel driver Eddie James
@ 2020-09-09 20:30 ` Eddie James
2020-09-10 6:18 ` Wolfram Sang
2020-09-10 9:00 ` Brendan Higgins
2020-09-09 20:30 ` [PATCH v3 4/5] ARM: dts: Aspeed: Tacoma: Add IBM Operation Panel I2C device Eddie James
2020-09-09 20:30 ` [PATCH v3 5/5] ARM: dts: Aspeed: Rainier: " Eddie James
4 siblings, 2 replies; 14+ messages in thread
From: Eddie James @ 2020-09-09 20:30 UTC (permalink / raw)
To: linux-input
Cc: devicetree, linux-aspeed, linux-i2c, linux-kernel, robh+dt,
dmitry.torokhov, joel, andrew, benh, brendanhiggins, wsa,
rentao.bupt, ryan_chen
Mask the IRQ status to only the bits that the driver checks. This
prevents excessive driver warnings when operating in slave mode
when additional bits are set that the driver doesn't handle.
Signed-off-by: Eddie James <eajames@linux.ibm.com>
Reviewed-by: Tao Ren <rentao.bupt@gmail.com>
---
drivers/i2c/busses/i2c-aspeed.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 31268074c422..724bf30600d6 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -69,6 +69,7 @@
* These share bit definitions, so use the same values for the enable &
* status bits.
*/
+#define ASPEED_I2CD_INTR_RECV_MASK 0xf000ffff
#define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT BIT(14)
#define ASPEED_I2CD_INTR_BUS_RECOVER_DONE BIT(13)
#define ASPEED_I2CD_INTR_SLAVE_MATCH BIT(7)
@@ -604,6 +605,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
bus->base + ASPEED_I2C_INTR_STS_REG);
readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+ irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
irq_remaining = irq_received;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/5] i2c: aspeed: Mask IRQ status to relevant bits
2020-09-09 20:30 ` [PATCH v3 3/5] i2c: aspeed: Mask IRQ status to relevant bits Eddie James
@ 2020-09-10 6:18 ` Wolfram Sang
2020-09-10 6:20 ` Wolfram Sang
2020-09-10 9:00 ` Brendan Higgins
1 sibling, 1 reply; 14+ messages in thread
From: Wolfram Sang @ 2020-09-10 6:18 UTC (permalink / raw)
To: Eddie James
Cc: linux-input, devicetree, linux-aspeed, linux-i2c, linux-kernel,
robh+dt, dmitry.torokhov, joel, andrew, benh, brendanhiggins,
rentao.bupt, ryan_chen
[-- Attachment #1: Type: text/plain, Size: 492 bytes --]
On Wed, Sep 09, 2020 at 03:30:57PM -0500, Eddie James wrote:
> Mask the IRQ status to only the bits that the driver checks. This
> prevents excessive driver warnings when operating in slave mode
> when additional bits are set that the driver doesn't handle.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> Reviewed-by: Tao Ren <rentao.bupt@gmail.com>
I reconsidered and applied it now because this helps whenever slave mode
is used. So, applied to for-current, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/5] i2c: aspeed: Mask IRQ status to relevant bits
2020-09-10 6:18 ` Wolfram Sang
@ 2020-09-10 6:20 ` Wolfram Sang
0 siblings, 0 replies; 14+ messages in thread
From: Wolfram Sang @ 2020-09-10 6:20 UTC (permalink / raw)
To: Eddie James
Cc: linux-input, devicetree, linux-aspeed, linux-i2c, linux-kernel,
robh+dt, dmitry.torokhov, joel, andrew, benh, brendanhiggins,
rentao.bupt, ryan_chen
[-- Attachment #1: Type: text/plain, Size: 705 bytes --]
On Thu, Sep 10, 2020 at 08:18:13AM +0200, Wolfram Sang wrote:
> On Wed, Sep 09, 2020 at 03:30:57PM -0500, Eddie James wrote:
> > Mask the IRQ status to only the bits that the driver checks. This
> > prevents excessive driver warnings when operating in slave mode
> > when additional bits are set that the driver doesn't handle.
> >
> > Signed-off-by: Eddie James <eajames@linux.ibm.com>
> > Reviewed-by: Tao Ren <rentao.bupt@gmail.com>
>
> I reconsidered and applied it now because this helps whenever slave mode
> is used. So, applied to for-current, thanks!
If someone could provide a Fixes tag, that would be welcome. For me, not
knowing the HW it doesn't look trivial to determine.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/5] i2c: aspeed: Mask IRQ status to relevant bits
2020-09-09 20:30 ` [PATCH v3 3/5] i2c: aspeed: Mask IRQ status to relevant bits Eddie James
2020-09-10 6:18 ` Wolfram Sang
@ 2020-09-10 9:00 ` Brendan Higgins
2020-09-10 13:55 ` Eddie James
1 sibling, 1 reply; 14+ messages in thread
From: Brendan Higgins @ 2020-09-10 9:00 UTC (permalink / raw)
To: Eddie James
Cc: linux-input, devicetree, linux-aspeed, linux-i2c,
Linux Kernel Mailing List, Rob Herring, Dmitry Torokhov,
Joel Stanley, Andrew Jeffery, Benjamin Herrenschmidt, wsa,
rentao.bupt, Ryan Chen
On Wed, Sep 9, 2020 at 1:31 PM Eddie James <eajames@linux.ibm.com> wrote:
>
> Mask the IRQ status to only the bits that the driver checks. This
> prevents excessive driver warnings when operating in slave mode
> when additional bits are set that the driver doesn't handle.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> Reviewed-by: Tao Ren <rentao.bupt@gmail.com>
Sorry, looks like I didn't get my comment in in time.
Looks good in principle. One minor comment below:
> ---
> drivers/i2c/busses/i2c-aspeed.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 31268074c422..724bf30600d6 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -69,6 +69,7 @@
> * These share bit definitions, so use the same values for the enable &
> * status bits.
> */
> +#define ASPEED_I2CD_INTR_RECV_MASK 0xf000ffff
Could we define ASPEED_I2CD_INTR_RECV_MASK to be ASPEED_I2CD_INTR_ALL ?
> #define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT BIT(14)
> #define ASPEED_I2CD_INTR_BUS_RECOVER_DONE BIT(13)
> #define ASPEED_I2CD_INTR_SLAVE_MATCH BIT(7)
> @@ -604,6 +605,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> bus->base + ASPEED_I2C_INTR_STS_REG);
> readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
> irq_remaining = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/5] i2c: aspeed: Mask IRQ status to relevant bits
2020-09-10 9:00 ` Brendan Higgins
@ 2020-09-10 13:55 ` Eddie James
0 siblings, 0 replies; 14+ messages in thread
From: Eddie James @ 2020-09-10 13:55 UTC (permalink / raw)
To: Brendan Higgins
Cc: linux-input, devicetree, linux-aspeed, linux-i2c,
Linux Kernel Mailing List, Rob Herring, Dmitry Torokhov,
Joel Stanley, Andrew Jeffery, Benjamin Herrenschmidt, wsa,
rentao.bupt, Ryan Chen
On 9/10/20 4:00 AM, Brendan Higgins wrote:
> On Wed, Sep 9, 2020 at 1:31 PM Eddie James <eajames@linux.ibm.com> wrote:
>> Mask the IRQ status to only the bits that the driver checks. This
>> prevents excessive driver warnings when operating in slave mode
>> when additional bits are set that the driver doesn't handle.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> Reviewed-by: Tao Ren <rentao.bupt@gmail.com>
> Sorry, looks like I didn't get my comment in in time.
>
> Looks good in principle. One minor comment below:
>
>> ---
>> drivers/i2c/busses/i2c-aspeed.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 31268074c422..724bf30600d6 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -69,6 +69,7 @@
>> * These share bit definitions, so use the same values for the enable &
>> * status bits.
>> */
>> +#define ASPEED_I2CD_INTR_RECV_MASK 0xf000ffff
> Could we define ASPEED_I2CD_INTR_RECV_MASK to be ASPEED_I2CD_INTR_ALL ?
That was my original thought... there is another define for that already
a few lines down though.
Thanks,
Eddie
>
>> #define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT BIT(14)
>> #define ASPEED_I2CD_INTR_BUS_RECOVER_DONE BIT(13)
>> #define ASPEED_I2CD_INTR_SLAVE_MATCH BIT(7)
>> @@ -604,6 +605,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>> writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
>> bus->base + ASPEED_I2C_INTR_STS_REG);
>> readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> + irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
>> irq_remaining = irq_received;
>>
>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> --
>> 2.26.2
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 4/5] ARM: dts: Aspeed: Tacoma: Add IBM Operation Panel I2C device
2020-09-09 20:30 [PATCH v3 0/5] input: misc: Add IBM Operation Panel driver Eddie James
` (2 preceding siblings ...)
2020-09-09 20:30 ` [PATCH v3 3/5] i2c: aspeed: Mask IRQ status to relevant bits Eddie James
@ 2020-09-09 20:30 ` Eddie James
2020-09-10 2:41 ` Joel Stanley
2020-09-09 20:30 ` [PATCH v3 5/5] ARM: dts: Aspeed: Rainier: " Eddie James
4 siblings, 1 reply; 14+ messages in thread
From: Eddie James @ 2020-09-09 20:30 UTC (permalink / raw)
To: linux-input
Cc: devicetree, linux-aspeed, linux-i2c, linux-kernel, robh+dt,
dmitry.torokhov, joel, andrew, benh, brendanhiggins, wsa,
rentao.bupt, ryan_chen
Set I2C bus 0 to multi-master mode and add the panel device that will
register as a slave.
Signed-off-by: Eddie James <eajames@linux.ibm.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
index 5f4ee67ac787..4d070d6ba09f 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
@@ -4,6 +4,7 @@
#include "aspeed-g6.dtsi"
#include <dt-bindings/gpio/aspeed-gpio.h>
+#include <dt-bindings/i2c/i2c.h>
#include <dt-bindings/leds/leds-pca955x.h>
/ {
@@ -438,7 +439,13 @@ aliases {
};
&i2c0 {
+ multi-master;
status = "okay";
+
+ ibm-panel@62 {
+ compatible = "ibm,op-panel";
+ reg = <(0x62 | I2C_OWN_SLAVE_ADDRESS)>;
+ };
};
&i2c1 {
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 4/5] ARM: dts: Aspeed: Tacoma: Add IBM Operation Panel I2C device
2020-09-09 20:30 ` [PATCH v3 4/5] ARM: dts: Aspeed: Tacoma: Add IBM Operation Panel I2C device Eddie James
@ 2020-09-10 2:41 ` Joel Stanley
0 siblings, 0 replies; 14+ messages in thread
From: Joel Stanley @ 2020-09-10 2:41 UTC (permalink / raw)
To: Eddie James
Cc: linux-input, devicetree, linux-aspeed, linux-i2c,
Linux Kernel Mailing List, Rob Herring, dmitry.torokhov,
Andrew Jeffery, Benjamin Herrenschmidt, Brendan Higgins, wsa,
Tao Ren, Ryan Chen
On Wed, 9 Sep 2020 at 20:31, Eddie James <eajames@linux.ibm.com> wrote:
>
> Set I2C bus 0 to multi-master mode and add the panel device that will
> register as a slave.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
Applied to the aspeed tree for v5.10.
Cheers,
Joel
> ---
> arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> index 5f4ee67ac787..4d070d6ba09f 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> @@ -4,6 +4,7 @@
>
> #include "aspeed-g6.dtsi"
> #include <dt-bindings/gpio/aspeed-gpio.h>
> +#include <dt-bindings/i2c/i2c.h>
> #include <dt-bindings/leds/leds-pca955x.h>
>
> / {
> @@ -438,7 +439,13 @@ aliases {
> };
>
> &i2c0 {
> + multi-master;
> status = "okay";
> +
> + ibm-panel@62 {
> + compatible = "ibm,op-panel";
> + reg = <(0x62 | I2C_OWN_SLAVE_ADDRESS)>;
> + };
> };
>
> &i2c1 {
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 5/5] ARM: dts: Aspeed: Rainier: Add IBM Operation Panel I2C device
2020-09-09 20:30 [PATCH v3 0/5] input: misc: Add IBM Operation Panel driver Eddie James
` (3 preceding siblings ...)
2020-09-09 20:30 ` [PATCH v3 4/5] ARM: dts: Aspeed: Tacoma: Add IBM Operation Panel I2C device Eddie James
@ 2020-09-09 20:30 ` Eddie James
2020-09-10 2:42 ` Joel Stanley
4 siblings, 1 reply; 14+ messages in thread
From: Eddie James @ 2020-09-09 20:30 UTC (permalink / raw)
To: linux-input
Cc: devicetree, linux-aspeed, linux-i2c, linux-kernel, robh+dt,
dmitry.torokhov, joel, andrew, benh, brendanhiggins, wsa,
rentao.bupt, ryan_chen
Set I2C bus 7 to multi-master mode and add the panel device that will
register as a slave.
Signed-off-by: Eddie James <eajames@linux.ibm.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
index b94421f6cbd5..50d528444f5d 100644
--- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
@@ -4,6 +4,7 @@
#include "aspeed-g6.dtsi"
#include <dt-bindings/gpio/aspeed-gpio.h>
+#include <dt-bindings/i2c/i2c.h>
#include <dt-bindings/leds/leds-pca955x.h>
/ {
@@ -698,6 +699,7 @@ eeprom@53 {
};
&i2c7 {
+ multi-master;
status = "okay";
si7021-a20@20 {
@@ -831,6 +833,11 @@ gpio@15 {
};
};
+ ibm-panel@62 {
+ compatible = "ibm,op-panel";
+ reg = <(0x62 | I2C_OWN_SLAVE_ADDRESS)>;
+ };
+
dps: dps310@76 {
compatible = "infineon,dps310";
reg = <0x76>;
--
2.26.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 5/5] ARM: dts: Aspeed: Rainier: Add IBM Operation Panel I2C device
2020-09-09 20:30 ` [PATCH v3 5/5] ARM: dts: Aspeed: Rainier: " Eddie James
@ 2020-09-10 2:42 ` Joel Stanley
0 siblings, 0 replies; 14+ messages in thread
From: Joel Stanley @ 2020-09-10 2:42 UTC (permalink / raw)
To: Eddie James
Cc: linux-input, devicetree, linux-aspeed, linux-i2c,
Linux Kernel Mailing List, Rob Herring, dmitry.torokhov,
Andrew Jeffery, Benjamin Herrenschmidt, Brendan Higgins, wsa,
Tao Ren, Ryan Chen
On Wed, 9 Sep 2020 at 20:31, Eddie James <eajames@linux.ibm.com> wrote:
>
> Set I2C bus 7 to multi-master mode and add the panel device that will
> register as a slave.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
Applied to the aspeed tree for v5.10.
Cheers,
Joel
> ---
> arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> index b94421f6cbd5..50d528444f5d 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> @@ -4,6 +4,7 @@
>
> #include "aspeed-g6.dtsi"
> #include <dt-bindings/gpio/aspeed-gpio.h>
> +#include <dt-bindings/i2c/i2c.h>
> #include <dt-bindings/leds/leds-pca955x.h>
>
> / {
> @@ -698,6 +699,7 @@ eeprom@53 {
> };
>
> &i2c7 {
> + multi-master;
> status = "okay";
>
> si7021-a20@20 {
> @@ -831,6 +833,11 @@ gpio@15 {
> };
> };
>
> + ibm-panel@62 {
> + compatible = "ibm,op-panel";
> + reg = <(0x62 | I2C_OWN_SLAVE_ADDRESS)>;
> + };
> +
> dps: dps310@76 {
> compatible = "infineon,dps310";
> reg = <0x76>;
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 14+ messages in thread