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

2020-09-02 Thread 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(>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 2/5] input: misc: Add IBM Operation Panel driver

2020-09-01 Thread Wolfram Sang

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



signature.asc
Description: PGP signature


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

2020-08-31 Thread Joel Stanley
On Thu, 20 Aug 2020 at 16:12, 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 
> ---
>  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 
>  L: linux-in...@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 
> 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 


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

2020-08-20 Thread 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 
---
 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 
 L: linux-in...@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 
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 ..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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#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(>input->dev, "command too short\n");
+   return;
+   }
+
+   if (panel->command[0] != 0xff && panel->command[1] != 0xf0) {
+   dev_dbg(>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(>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(>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(>input->dev, "event: