mbox series

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

Message ID 20200908200101.64974-1-eajames@linux.ibm.com
Headers show
Series input: misc: Add IBM Operation Panel driver | expand

Message

Eddie James Sept. 8, 2020, 8 p.m. UTC 
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, typically to a service
processor. 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.

Changes since v1:
 - Redo DTS documentation example to use I2C_OWN_SLAVE_ADDRESS
 - Reject commands received in the input driver that are too long
 - Add a definition for the interrupt status mask in the Aspeed I2C driver
 - Use I2C_OWN_SLAVE_ADDRESS for both dts additions

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          |  39 ++++
 MAINTAINERS                                   |   7 +
 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts  |   7 +
 arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts   |   7 +
 drivers/i2c/busses/i2c-aspeed.c               |   2 +
 drivers/input/misc/Kconfig                    |  18 ++
 drivers/input/misc/Makefile                   |   1 +
 drivers/input/misc/ibm-panel.c                | 192 ++++++++++++++++++
 8 files changed, 273 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/ibm,op-panel.yaml
 create mode 100644 drivers/input/misc/ibm-panel.c

Comments

Wolfram Sang Sept. 8, 2020, 8:40 p.m. UTC | #1 
Hi Eddie,

> +	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
> +			/*
> +			 * 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;
> +	default:
> +		break;
> +	}

Sorry, I missed this in my last review. READ states are mandatory, so
you will need something like this:

+	case I2C_SLAVE_READ_REQUESTED:
+	case I2C_SLAVE_READ_PROCESSED:
+		*val = 0xff;
+		break;


> +	if (command_size)
> +		ibm_panel_process_command(panel, command_size);

I wondered if you could check for the correct command_size here, so no
need to call into the function when the size doesn't match?

> +       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);

This dev_err can go. The core will print messages if something goes
wrong.

The rest looks good from an I2C point of view.

I'd think this all should go via the input tree?

Kind regards,

   Wolfram