[PATCH 0/5] input: misc: Add IBM Operation Panel driver

[PATCH 0/5] input: misc: Add IBM Operation Panel driver

[Eddie James]

This series adds support for input from the IBM Operation Panel, which is
a simple controller with three buttons and an LCD display meant for
interacting with a server. It's connected over I2C. This series only
supports the input from the panel, in which the panel masters the I2C bus
and sends data to the host system when someone presses a button on the
controller.

Eddie James (5):
  dt-bindings: input: Add documentation for IBM Operation Panel
  input: misc: Add IBM Operation Panel driver
  i2c: aspeed: Mask IRQ status to relevant bits
  ARM: dts: Aspeed: Tacoma: Add IBM Operation Panel I2C device
  ARM: dts: Aspeed: Rainier: Add IBM Operation Panel I2C device

 .../bindings/input/ibm,op-panel.yaml          |  38 ++++
 MAINTAINERS                                   |   7 +
 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts  |   6 +
 arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts   |   6 +
 drivers/i2c/busses/i2c-aspeed.c               |   1 +
 drivers/input/misc/Kconfig                    |  10 +
 drivers/input/misc/Makefile                   |   1 +
 drivers/input/misc/ibm-panel.c                | 186 ++++++++++++++++++
 8 files changed, 255 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/ibm,op-panel.yaml
 create mode 100644 drivers/input/misc/ibm-panel.c

-- 
2.26.2

[PATCH 1/5] dt-bindings: input: Add documentation for IBM Operation Panel

[Eddie James]

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>
---
 .../bindings/input/ibm,op-panel.yaml          | 38 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 44 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..86a32e8f3ef0
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/ibm,op-panel.yaml
@@ -0,0 +1,38 @@
+# 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
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ibm-op-panel@62 {
+            compatible = "ibm,op-panel";
+            reg = <0x40000062>; /* I2C_OWN_SLAVE_ADDRESS */
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index ac79fdbdf8d0..a9fd08e9cd54 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8278,6 +8278,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

[PATCH 2/5] input: misc: Add IBM Operation Panel driver

[Eddie James]

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>
---
 MAINTAINERS                    |   1 +
 drivers/input/misc/Kconfig     |  10 ++
 drivers/input/misc/Makefile    |   1 +
 drivers/input/misc/ibm-panel.c | 186 +++++++++++++++++++++++++++++++++
 4 files changed, 198 insertions(+)
 create mode 100644 drivers/input/misc/ibm-panel.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a9fd08e9cd54..077cc79ad7fd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8283,6 +8283,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..88fb465a18b8 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -708,6 +708,16 @@ 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
+	  Supports the IBM Operation Panel as an input device. The panel is a
+	  controller attached to the system 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.
+
 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..607e7dd5a0fb
--- /dev/null
+++ b/drivers/input/misc/ibm-panel.c
@@ -0,0 +1,186 @@
+// 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/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 command_size)
+{
+	u8 i;
+	u8 chksum;
+	u16 sum = 0;
+	int pressed;
+	int released;
+
+	if (command_size != sizeof(panel->command)) {
+		dev_dbg(&panel->input->dev, "command too short\n");
+		return;
+	}
+
+	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)
+{
+	u8 command_size = 0;
+	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:
+		command_size = 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
+			dev_dbg(&panel->input->dev, "command truncated\n");
+		break;
+	default:
+		break;
+	}
+
+	spin_unlock_irqrestore(&panel->lock, flags);
+
+	if (command_size)
+		ibm_panel_process_command(panel, command_size);
+
+	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);
+		dev_err(&client->dev,
+			"Failed to register I2C slave device: %d\n", rc);
+		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

[PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits

[Eddie James]

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>
---
 drivers/i2c/busses/i2c-aspeed.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 31268074c422..abf40f2af8b4 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -604,6 +604,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 &= 0xf000ffff;
 	irq_remaining = irq_received;
 
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
-- 
2.26.2

[PATCH 4/5] ARM: dts: Aspeed: Tacoma: Add IBM Operation Panel I2C device

[Eddie James]

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>
---
 arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts | 6 ++++++
 1 file changed, 6 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..9cf2e02ae9e2 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
@@ -438,7 +438,13 @@ aliases {
 };
 
 &i2c0 {
+	multi-master;
 	status = "okay";
+
+	ibm-panel@62 {
+		compatible = "ibm,op-panel";
+		reg = <0x40000062>; /* I2C_OWN_SLAVE_ADDRESS */
+	};
 };
 
 &i2c1 {
-- 
2.26.2

[PATCH 5/5] ARM: dts: Aspeed: Rainier: Add IBM Operation Panel I2C device

[Eddie James]

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>
---
 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 6 ++++++
 1 file changed, 6 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..f121f3c26a3a 100644
--- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
@@ -698,6 +698,7 @@ eeprom@53 {
 };
 
 &i2c7 {
+	multi-master;
 	status = "okay";
 
 	si7021-a20@20 {
@@ -831,6 +832,11 @@ gpio@15 {
 		};
 	};
 
+	ibm-panel@62 {
+		compatible = "ibm,op-panel";
+		reg = <0x40000062>;	/* I2C_OWN_SLAVE_ADDRESS */
+	};
+
 	dps: dps310@76 {
 		compatible = "infineon,dps310";
 		reg = <0x76>;
-- 
2.26.2

Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 114 bytes --]


> +	irq_received &= 0xf000ffff;

Can we have a define for this? Like ASPEED_I2CD_INTR_MASTER_IRQS or
something?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits

[Joel Stanley]

On Thu, 20 Aug 2020 at 16:12, 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>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 31268074c422..abf40f2af8b4 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -604,6 +604,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 &= 0xf000ffff;
>         irq_remaining = irq_received;

This would defeat the check for irq_remaining. I don't think we want to do this.

Can you explain why these bits are being set in slave mode?

Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits

[Eddie James]

On 8/25/20 1:38 AM, Joel Stanley wrote:
> On Thu, 20 Aug 2020 at 16:12, 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>
>> ---
>>   drivers/i2c/busses/i2c-aspeed.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 31268074c422..abf40f2af8b4 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -604,6 +604,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 &= 0xf000ffff;
>>          irq_remaining = irq_received;
> This would defeat the check for irq_remaining. I don't think we want to do this.
>
> Can you explain why these bits are being set in slave mode?


No, I don't have any documentation for the bits that are masked off 
here, so I don't know why they would get set.

The check for irq_remaining is still useful for detecting that the 
driver state machine might be out of sync with what the master is doing.


Thanks,

Eddie

Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits

[Tao Ren]

On Tue, Aug 25, 2020 at 02:47:51PM -0500, Eddie James wrote:
> 
> On 8/25/20 1:38 AM, Joel Stanley wrote:
> > On Thu, 20 Aug 2020 at 16:12, 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>
> > > ---
> > >   drivers/i2c/busses/i2c-aspeed.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> > > index 31268074c422..abf40f2af8b4 100644
> > > --- a/drivers/i2c/busses/i2c-aspeed.c
> > > +++ b/drivers/i2c/busses/i2c-aspeed.c
> > > @@ -604,6 +604,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 &= 0xf000ffff;
> > >          irq_remaining = irq_received;
> > This would defeat the check for irq_remaining. I don't think we want to do this.
> > 
> > Can you explain why these bits are being set in slave mode?
> 
> 
> No, I don't have any documentation for the bits that are masked off here, so
> I don't know why they would get set.
> 
> The check for irq_remaining is still useful for detecting that the driver
> state machine might be out of sync with what the master is doing.

I have a similar patch in my local tree, and the reason being: AST2600
I2C Controller may set I2CD10[25:24] to report Current Slave Parking
Status (defined in new register I2CS24) even though the new register
mode is off. The 2 bits can be ignored in legacy mode, and Ryan from
ASPEED could confirm it.


Cheers,

Tao

RE: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits

[Ryan Chen]

> -----Original Message-----
> From: Tao Ren [mailto:rentao.bupt@gmail.com]
> Sent: Wednesday, August 26, 2020 4:05 AM
> To: Eddie James <eajames@linux.ibm.com>
> Cc: Joel Stanley <joel@jms.id.au>; devicetree <devicetree@vger.kernel.org>;
> linux-aspeed <linux-aspeed@lists.ozlabs.org>; dmitry.torokhov@gmail.com;
> Brendan Higgins <brendanhiggins@google.com>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; Rob Herring <robh+dt@kernel.org>;
> linux-i2c@vger.kernel.org; linux-input@vger.kernel.org; Ryan Chen
> <ryan_chen@aspeedtech.com>
> Subject: Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits
> 
> On Tue, Aug 25, 2020 at 02:47:51PM -0500, Eddie James wrote:
> >
> > On 8/25/20 1:38 AM, Joel Stanley wrote:
> > > On Thu, 20 Aug 2020 at 16:12, 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>
> > > > ---
> > > >   drivers/i2c/busses/i2c-aspeed.c | 1 +
> > > >   1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-aspeed.c
> > > > b/drivers/i2c/busses/i2c-aspeed.c index 31268074c422..abf40f2af8b4
> > > > 100644
> > > > --- a/drivers/i2c/busses/i2c-aspeed.c
> > > > +++ b/drivers/i2c/busses/i2c-aspeed.c
> > > > @@ -604,6 +604,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 &= 0xf000ffff;
> > > >          irq_remaining = irq_received;
> > > This would defeat the check for irq_remaining. I don't think we want to do
> this.
> > >
> > > Can you explain why these bits are being set in slave mode?
> >
> >
> > No, I don't have any documentation for the bits that are masked off
> > here, so I don't know why they would get set.
> >
> > The check for irq_remaining is still useful for detecting that the
> > driver state machine might be out of sync with what the master is doing.
> 
> I have a similar patch in my local tree, and the reason being: AST2600 I2C
> Controller may set I2CD10[25:24] to report Current Slave Parking Status
> (defined in new register I2CS24) even though the new register mode is off. The
> 2 bits can be ignored in legacy mode, and Ryan from ASPEED could confirm it.
Yes, in AST2600 i2cd10[25:24] will be the same with new mode register i2cs24[25:24]
Thanks Tao.
> 
> 
> Cheers,
> 
> Tao

Re: [PATCH 2/5] input: misc: Add IBM Operation Panel driver

[Joel Stanley]

On Thu, 20 Aug 2020 at 16:12, Eddie James <eajames@linux.ibm.com> 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>
> ---
>  MAINTAINERS                    |   1 +
>  drivers/input/misc/Kconfig     |  10 ++
>  drivers/input/misc/Makefile    |   1 +
>  drivers/input/misc/ibm-panel.c | 186 +++++++++++++++++++++++++++++++++
>  4 files changed, 198 insertions(+)
>  create mode 100644 drivers/input/misc/ibm-panel.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a9fd08e9cd54..077cc79ad7fd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8283,6 +8283,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..88fb465a18b8 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -708,6 +708,16 @@ 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
> +         Supports the IBM Operation Panel as an input device. The panel is a
> +         controller attached to the system 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.

Is this always attached via a service processor/bmc? If so, mention it
here so people know there's no point enabling it on a host/distro
kernel.

I assume you're implementing the protocol correctly.  If you have a
link to a specification then include that in the file.

The code looks okay to me.

Reviewed-by: Joel Stanley <joel@jms.id.au>

Re: [PATCH 4/5] ARM: dts: Aspeed: Tacoma: Add IBM Operation Panel I2C device

[Joel Stanley]

On Thu, 20 Aug 2020 at 16:12, 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>

> ---
>  arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts | 6 ++++++
>  1 file changed, 6 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..9cf2e02ae9e2 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> @@ -438,7 +438,13 @@ aliases {
>  };
>
>  &i2c0 {
> +       multi-master;
>         status = "okay";
> +
> +       ibm-panel@62 {
> +               compatible = "ibm,op-panel";
> +               reg = <0x40000062>; /* I2C_OWN_SLAVE_ADDRESS */

Other users of SLAVE_ADDRESS have included <dt-bindings/i2c/i2c.h> and
written the reg as follows:

reg = <(I2C_OWN_SLAVE_ADDRESS | 0x62)>

Which obviously has the same result. I'll leave it up to you.

Cheers,

Joel

> +       };
>  };
>
>  &i2c1 {
> --
> 2.26.2
>

Re: [PATCH 5/5] ARM: dts: Aspeed: Rainier: Add IBM Operation Panel I2C device

[Joel Stanley]

On Thu, 20 Aug 2020 at 16:12, 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>

Same comments as for Tacoma.

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 6 ++++++
>  1 file changed, 6 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..f121f3c26a3a 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> @@ -698,6 +698,7 @@ eeprom@53 {
>  };
>
>  &i2c7 {
> +       multi-master;
>         status = "okay";
>
>         si7021-a20@20 {
> @@ -831,6 +832,11 @@ gpio@15 {
>                 };
>         };
>
> +       ibm-panel@62 {
> +               compatible = "ibm,op-panel";
> +               reg = <0x40000062>;     /* I2C_OWN_SLAVE_ADDRESS */
> +       };
> +
>         dps: dps310@76 {
>                 compatible = "infineon,dps310";
>                 reg = <0x76>;
> --
> 2.26.2
>

Re: [PATCH 1/5] dt-bindings: input: Add documentation for IBM Operation Panel

[Joel Stanley]

On Thu, 20 Aug 2020 at 16:12, Eddie James <eajames@linux.ibm.com> wrote:
>
> 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>

Acked-by: Joel Stanley <joel@jms.id.au>

> ---
>  .../bindings/input/ibm,op-panel.yaml          | 38 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 44 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..86a32e8f3ef0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/ibm,op-panel.yaml
> @@ -0,0 +1,38 @@
> +# 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
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ibm-op-panel@62 {
> +            compatible = "ibm,op-panel";
> +            reg = <0x40000062>; /* I2C_OWN_SLAVE_ADDRESS */
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ac79fdbdf8d0..a9fd08e9cd54 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8278,6 +8278,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
>

Re: [PATCH 2/5] input: misc: Add IBM Operation Panel driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]


> +	switch (event) {
> +	case I2C_SLAVE_STOP:
> +		command_size = 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
> +			dev_dbg(&panel->input->dev, "command truncated\n");

Just double checking: Do you really want to process truncated commands?
Since you detect the state here, you could also choose to reject such
commands?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/5] ARM: dts: Aspeed: Tacoma: Add IBM Operation Panel I2C device

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 398 bytes --]


> > +       ibm-panel@62 {
> > +               compatible = "ibm,op-panel";
> > +               reg = <0x40000062>; /* I2C_OWN_SLAVE_ADDRESS */
> 
> Other users of SLAVE_ADDRESS have included <dt-bindings/i2c/i2c.h> and
> written the reg as follows:
> 
> reg = <(I2C_OWN_SLAVE_ADDRESS | 0x62)>
> 
> Which obviously has the same result. I'll leave it up to you.

The latter, please.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/5] input: misc: Add IBM Operation Panel driver

[Eddie James]

On 9/1/20 1:11 AM, Wolfram Sang wrote:
>> +	switch (event) {
>> +	case I2C_SLAVE_STOP:
>> +		command_size = 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
>> +			dev_dbg(&panel->input->dev, "command truncated\n");
> Just double checking: Do you really want to process truncated commands?
> Since you detect the state here, you could also choose to reject such
> commands?


Yes I suppose not. It could still be a valid command with extra bytes, 
but unlikely, so probably better not to handle it.


Thanks,

Eddie


>

Re: [PATCH 1/5] dt-bindings: input: Add documentation for IBM Operation Panel

[Rob Herring]

On Thu, Aug 20, 2020 at 11:11:48AM -0500, Eddie James wrote:
> 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>
> ---
>  .../bindings/input/ibm,op-panel.yaml          | 38 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 44 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..86a32e8f3ef0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/ibm,op-panel.yaml
> @@ -0,0 +1,38 @@
> +# 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

With that added,

Reviewed-by: Rob Herring <robh@kernel.org>