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

[wsa@kernel.org]

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

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


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