input: misc: Add IBM Operation Panel driver

PATCH Superseded Eddie James 6 participants 19 messages Aug 20, 2020 – Sep 8, 2020

Subsystems (7)

Device tree bindings HID I2C subsystem host drivers Input drivers +3 more subsystems

lore

Share & tools

lore

Eddie James

author maintainer

2020-08-20 16:11 UTC #0 lore raw

cover input: misc: Add IBM Operation Panel driver

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

Signature · 1 line

2.26.2

Eddie James

author maintainer

2020-08-20 16:11 UTC #1 lore raw

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

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 <[email protected]>

---
 .../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

Documentation/devicetree/bindings/input/ibm,op-panel.yaml new file +38 -0

new file mode 100644
index 000000000000..86a32e8f3ef0
@@ -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 <[email protected]>
+
+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 at 62 {
+            compatible = "ibm,op-panel";
+            reg = <0x40000062>; /* I2C_OWN_SLAVE_ADDRESS */
+        };
+    };

MAINTAINERS +6 -0

index ac79fdbdf8d0..a9fd08e9cd54 100644
@@ -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 <[email protected]>
+L:	linux-input at vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/input/ibm,op-panel.yaml
+
 IBM Power 842 compression accelerator
 M:	Haren Myneni <[email protected]>
 S:	Supported

Signature · 1 line

2.26.2

Joel Stanley

maintainer Acked-by on v1 1/5

2020-09-01 06:01 UTC #2 lore raw

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

Quoted 7 lines · 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 <[email protected]>

Acked-by Joel Stanley <[email protected]>

Quoted 70 lines

---
 .../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

Documentation/devicetree/bindings/input/ibm,op-panel.yaml new file +38 -0

new file mode 100644
index 000000000000..86a32e8f3ef0
@@ -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 <[email protected]>
+
+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 at 62 {
+            compatible = "ibm,op-panel";
+            reg = <0x40000062>; /* I2C_OWN_SLAVE_ADDRESS */
+        };
+    };

MAINTAINERS +6 -1

index ac79fdbdf8d0..a9fd08e9cd54 100644
@@ -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 <[email protected]>
+L:     linux-input at vger.kernel.org
+S:     Maintained
+F:     Documentation/devicetree/bindings/input/ibm,op-panel.yaml
+
 IBM Power 842 compression accelerator
 M:     Haren Myneni <[email protected]>
 S:     Supported
--

2.26.2

Rob Herring

maintainer Reviewed-by on v1 1/5

2020-09-08 20:37 UTC #3 lore raw

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

In reply to Eddie James

Quoted 34 lines · 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 <[email protected]>

---
 .../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

Documentation/devicetree/bindings/input/ibm,op-panel.yaml new file +16 -0

new file mode 100644
index 000000000000..86a32e8f3ef0
@@ -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 <[email protected]>
+
+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.
+

Documentation/devicetree/bindings/input/ibm,op-panel.yaml line 26

Quoted 10 lines

Documentation/devicetree/bindings/input/ibm,op-panel.yaml new file +10 -0
+properties:
+  compatible:
+    const: ibm,op-panel
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg

additionalProperties: false

With that added,

Reviewed-by Rob Herring <[email protected]>

Eddie James

author maintainer

2020-08-20 16:11 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 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

MAINTAINERS +1 -0

index a9fd08e9cd54..077cc79ad7fd 100644
@@ -8283,6 +8283,7 @@ M:	Eddie James <[email protected]>
 L:	linux-input at 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 <[email protected]>

drivers/input/misc/Kconfig +10 -0

index 362e8a01980c..88fb465a18b8 100644
@@ -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

drivers/input/misc/Makefile +1 -0

index a48e5f2d859d..7e9edf0a142b 100644
@@ -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

drivers/input/misc/ibm-panel.c new file +186 -0

186 lines not on this page · raw message

Signature · 1 line

2.26.2

Joel Stanley

maintainer Reviewed-by on v1 2/5

2020-09-01 05:58 UTC #5 lore raw

Quoted 34 lines · Eddie James

Signed-off-by Eddie James <[email protected]>

---
 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

MAINTAINERS +1 -0

index a9fd08e9cd54..077cc79ad7fd 100644
@@ -8283,6 +8283,7 @@ M:        Eddie James <[email protected]>
 L:     linux-input at 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 <[email protected]>

drivers/input/misc/Kconfig +0 -0

index 362e8a01980c..88fb465a18b8 100644
@@ -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.

drivers/input/misc/Kconfig config INPUT_ADXL34X_SPI · line 719

Quoted 10 lines

drivers/input/misc/Kconfig +9 -0

 
+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 <[email protected]>

Wolfram Sang

2020-09-01 06:11 UTC #6 lore raw

In reply to Eddie James

drivers/input/misc/ibm-panel.c line 102

Quoted 13 lines

drivers/input/misc/ibm-panel.c new file +12 -0

+	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?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20200901/2d5dae3a/attachment.sig>

Eddie James

author maintainer

2020-09-02 15:47 UTC #7 lore raw

Quoted 7 lines · Wolfram Sang

Quoted 7 lines

drivers/input/misc/ibm-panel.c new file +7 -0

+	switch (event) {
+	case I2C_SLAVE_STOP:
+		command_size = panel->idx;
+		fallthrough;
+	case I2C_SLAVE_WRITE_REQUESTED:
+		panel->idx = 0;
+		break;

Quoted 8 lines

drivers/input/misc/ibm-panel.c line 102
Quoted 5 lines
drivers/input/misc/ibm-panel.c new file +5 -0
+	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

Quoted 1 line

Eddie James

author maintainer

2020-08-20 16:11 UTC #8 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/i2c/busses/i2c-aspeed.c | 1 +
 1 file changed, 1 insertion(+)

drivers/i2c/busses/i2c-aspeed.c +1 -0

index 31268074c422..abf40f2af8b4 100644
@@ -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)

Signature · 1 line

2.26.2

Wolfram Sang

2020-08-21 05:54 UTC #9 lore raw

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

drivers/i2c/busses/i2c-aspeed.c static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) · line 607

Quoted 2 lines

drivers/i2c/busses/i2c-aspeed.c +1 -0
+	irq_received &= 0xf000ffff;

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

Joel Stanley

maintainer

2020-08-25 06:38 UTC #10 lore raw

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

In reply to Eddie James

Quoted 15 lines · 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 <[email protected]>
---
 drivers/i2c/busses/i2c-aspeed.c | 1 +
 1 file changed, 1 insertion(+)
drivers/i2c/busses/i2c-aspeed.c +0 -0
index 31268074c422..abf40f2af8b4 100644

drivers/i2c/busses/i2c-aspeed.c static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) · line 608

Quoted 5 lines

drivers/i2c/busses/i2c-aspeed.c +1 -0

@@ -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?

Eddie James

author maintainer

2020-08-25 19:47 UTC #11 lore raw

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

Quoted 22 lines · Joel Stanley

Quoted 14 lines · 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 <[email protected]>
---
  drivers/i2c/busses/i2c-aspeed.c | 1 +
  1 file changed, 1 insertion(+)
drivers/i2c/busses/i2c-aspeed.c +0 -0
index 31268074c422..abf40f2af8b4 100644
drivers/i2c/busses/i2c-aspeed.c static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) · line 608
Quoted 5 lines
drivers/i2c/busses/i2c-aspeed.c +1 -0
@@ -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.

Quoted 1 line

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

Tao Ren

2020-08-25 20:05 UTC #12 lore raw

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

Quoted 24 lines · Eddie James

Quoted 22 lines · Joel Stanley
Quoted 14 lines · 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 <[email protected]>
---
  drivers/i2c/busses/i2c-aspeed.c | 1 +
  1 file changed, 1 insertion(+)
drivers/i2c/busses/i2c-aspeed.c +0 -0
index 31268074c422..abf40f2af8b4 100644
drivers/i2c/busses/i2c-aspeed.c static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) · line 608
Quoted 5 lines
drivers/i2c/busses/i2c-aspeed.c +1 -0
@@ -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.

Quoted 8 lines

Quoted 1 line
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

Ryan Chen

maintainer

2020-08-26 01:59 UTC #13 lore raw

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

Quoted 47 lines

-----Original Message-----
From: Tao Ren [mailto:rentao.bupt at gmail.com]
Sent: Wednesday, August 26, 2020 4:05 AM
To: Eddie James <[email protected]>
Cc: Joel Stanley <[email protected]>; devicetree <[email protected]>; linux-aspeed <[email protected]>; dmitry.torokhov at gmail.com; Brendan Higgins <[email protected]>; Linux Kernel Mailing List <[email protected]>; Rob Herring <[email protected]>; linux-i2c at vger.kernel.org; linux-input at vger.kernel.org; Ryan Chen <[email protected]>
Subject: Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits
Quoted 3 lines · Eddie James
Quoted 1 line · Joel Stanley
On Thu, 20 Aug 2020 at 16:12, Eddie James <[email protected]>
Quoted 9 lines · wrote:
Quoted 9 lines
Quoted 9 lines
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 <[email protected]>
---
  drivers/i2c/busses/i2c-aspeed.c | 1 +
  1 file changed, 1 insertion(+)
Quoted 6 lines
Quoted 6 lines
drivers/i2c/busses/i2c-aspeed.c static irqreturn_t aspeed_i2c_bus_irq(int irq,
Quoted 6 lines
(quoted diff) +0 -0
b/drivers/i2c/busses/i2c-aspeed.c index 31268074c422..abf40f2af8b4 100644
drivers/i2c/busses/i2c-aspeed.c +0 -0
@@ -604,6 +604,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq,
void *dev_id)
Quoted 6 lines
Quoted 6 lines
drivers/i2c/busses/i2c-aspeed.c static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) · line 608
Quoted 5 lines
drivers/i2c/busses/i2c-aspeed.c +1 -0
         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.
Quoted 7 lines
Quoted 2 lines
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.

Quoted 7 lines

Quoted 2 lines
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.

Quoted 5 lines

Cheers,
Tao

Eddie James

author maintainer

2020-08-20 16:11 UTC #14 lore raw

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

In reply to 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 <[email protected]>

---
 arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts +6 -0

index 5f4ee67ac787..9cf2e02ae9e2 100644
@@ -438,7 +438,13 @@ aliases {
 };
 
 &i2c0 {
+	multi-master;
 	status = "okay";
+
+	ibm-panel at 62 {
+		compatible = "ibm,op-panel";
+		reg = <0x40000062>; /* I2C_OWN_SLAVE_ADDRESS */
+	};
 };
 
 &i2c1 {

Signature · 1 line

2.26.2

Joel Stanley

maintainer Reviewed-by on v1 4/5

2020-09-01 06:00 UTC #15 lore raw

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

Quoted 5 lines · 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 <[email protected]>

Reviewed-by Joel Stanley <[email protected]>

Quoted 9 lines

---
 arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts +0 -0

index 5f4ee67ac787..9cf2e02ae9e2 100644

arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts aliases { · line 446

Quoted 9 lines

arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts +5 -0

@@ -438,7 +438,13 @@ aliases {
 };
 
 &i2c0 {
+       multi-master;
        status = "okay";
+
+       ibm-panel at 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

Quoted 7 lines

arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts +1 -1
+       };
 };
 
 &i2c1 {
--
2.26.2

Wolfram Sang

2020-09-01 06:11 UTC #16 lore raw

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

Quoted 11 lines

arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts aliases { · line 446
Quoted 4 lines
arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts +3 -0
+       ibm-panel at 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.

Eddie James

author maintainer

2020-08-20 16:11 UTC #17 lore raw

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

In reply to 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 <[email protected]>

---
 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts +6 -0

index b94421f6cbd5..f121f3c26a3a 100644
@@ -698,6 +698,7 @@ eeprom at 53 {
 };
 
 &i2c7 {
+	multi-master;
 	status = "okay";
 
 	si7021-a20 at 20 {

@@ -831,6 +832,11 @@ gpio at 15 {
 		};
 	};
 
+	ibm-panel at 62 {
+		compatible = "ibm,op-panel";
+		reg = <0x40000062>;	/* I2C_OWN_SLAVE_ADDRESS */
+	};
+
 	dps: dps310 at 76 {
 		compatible = "infineon,dps310";
 		reg = <0x76>;

Signature · 1 line

2.26.2

Joel Stanley

maintainer Reviewed-by on v1 5/5

2020-09-01 06:01 UTC #18 lore raw

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

Quoted 5 lines · 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 <[email protected]>

Same comments as for Tacoma.

Reviewed-by Joel Stanley <[email protected]>

Quoted 31 lines

---
 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts +6 -1

index b94421f6cbd5..f121f3c26a3a 100644
@@ -698,6 +698,7 @@ eeprom at 53 {
 };
 
 &i2c7 {
+       multi-master;
        status = "okay";
 
        si7021-a20 at 20 {

@@ -831,6 +832,11 @@ gpio at 15 {
                };
        };
 
+       ibm-panel at 62 {
+               compatible = "ibm,op-panel";
+               reg = <0x40000062>;     /* I2C_OWN_SLAVE_ADDRESS */
+       };
+
        dps: dps310 at 76 {
                compatible = "infineon,dps310";
                reg = <0x76>;
--

2.26.2