| Message ID | 20200820161152.22751-1-eajames@linux.ibm.com |
|---|---|
| Headers | show |
| Series | input: misc: Add IBM Operation Panel driver | expand |
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>
> + 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?
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 >