Re: [PATCH 2/2] rainshadow-cec: new RainShadow Tech HDMI CEC driver

[Hans Verkuil <hverkuil@xs4all.nl>]

Hi Dmitry,

Thanks for the feedback. See my comments below:

On 03/01/17 07:51, Dmitry Torokhov wrote:
> Hi Hans,
>
> On Thu, Dec 15, 2016 at 02:02:07PM +0100, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> This driver supports the RainShadow Tech USB HDMI CEC adapter.
>>
>> See: http://rainshadowtech.com/HdmiCecUsb.html
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  MAINTAINERS                                       |   7 +
>>  drivers/media/usb/Kconfig                         |   1 +
>>  drivers/media/usb/Makefile                        |   1 +
>>  drivers/media/usb/rainshadow-cec/Kconfig          |  10 +
>>  drivers/media/usb/rainshadow-cec/Makefile         |   1 +
>>  drivers/media/usb/rainshadow-cec/rainshadow-cec.c | 344 ++++++++++++++++++++++
>>  6 files changed, 364 insertions(+)
>>  create mode 100644 drivers/media/usb/rainshadow-cec/Kconfig
>>  create mode 100644 drivers/media/usb/rainshadow-cec/Makefile
>>  create mode 100644 drivers/media/usb/rainshadow-cec/rainshadow-cec.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 52cc077..78ebc5d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -10069,6 +10069,13 @@ L:	linux-fbdev@vger.kernel.org
>>  S:	Maintained
>>  F:	drivers/video/fbdev/aty/aty128fb.c
>>
>> +RAINSHADOW-CEC DRIVER
>> +M:	Hans Verkuil <hverkuil@xs4all.nl>
>> +L:	linux-media@vger.kernel.org
>> +T:	git git://linuxtv.org/media_tree.git
>> +S:	Maintained
>> +F:	drivers/media/usb/rainshadow-cec/*
>> +
>>  RALINK MIPS ARCHITECTURE
>>  M:	John Crispin <john@phrozen.org>
>>  L:	linux-mips@linux-mips.org
>> diff --git a/drivers/media/usb/Kconfig b/drivers/media/usb/Kconfig
>> index c9644b6..b24e753 100644
>> --- a/drivers/media/usb/Kconfig
>> +++ b/drivers/media/usb/Kconfig
>> @@ -63,6 +63,7 @@ endif
>>  if MEDIA_CEC_SUPPORT
>>  	comment "USB HDMI CEC adapters"
>>  source "drivers/media/usb/pulse8-cec/Kconfig"
>> +source "drivers/media/usb/rainshadow-cec/Kconfig"
>>  endif
>>
>>  endif #MEDIA_USB_SUPPORT
>> diff --git a/drivers/media/usb/Makefile b/drivers/media/usb/Makefile
>> index 0f15e33..738b993 100644
>> --- a/drivers/media/usb/Makefile
>> +++ b/drivers/media/usb/Makefile
>> @@ -25,3 +25,4 @@ obj-$(CONFIG_VIDEO_USBTV) += usbtv/
>>  obj-$(CONFIG_VIDEO_GO7007) += go7007/
>>  obj-$(CONFIG_DVB_AS102) += as102/
>>  obj-$(CONFIG_USB_PULSE8_CEC) += pulse8-cec/
>> +obj-$(CONFIG_USB_RAINSHADOW_CEC) += rainshadow-cec/
>> diff --git a/drivers/media/usb/rainshadow-cec/Kconfig b/drivers/media/usb/rainshadow-cec/Kconfig
>> new file mode 100644
>> index 0000000..447291b
>> --- /dev/null
>> +++ b/drivers/media/usb/rainshadow-cec/Kconfig
>> @@ -0,0 +1,10 @@
>> +config USB_RAINSHADOW_CEC
>> +	tristate "RainShadow Tech HDMI CEC"
>> +	depends on USB_ACM && MEDIA_CEC_SUPPORT
>> +	select SERIO
>> +	select SERIO_SERPORT
>> +	---help---
>> +	  This is a cec driver for the RainShadow Tech HDMI CEC device.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called rainshadow-cec.
>> diff --git a/drivers/media/usb/rainshadow-cec/Makefile b/drivers/media/usb/rainshadow-cec/Makefile
>> new file mode 100644
>> index 0000000..a79fbc7
>> --- /dev/null
>> +++ b/drivers/media/usb/rainshadow-cec/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_USB_RAINSHADOW_CEC) += rainshadow-cec.o
>> diff --git a/drivers/media/usb/rainshadow-cec/rainshadow-cec.c b/drivers/media/usb/rainshadow-cec/rainshadow-cec.c
>> new file mode 100644
>> index 0000000..dc7f287
>> --- /dev/null
>> +++ b/drivers/media/usb/rainshadow-cec/rainshadow-cec.c
>> @@ -0,0 +1,344 @@
>> +/*
>> + * RainShadow Tech HDMI CEC driver
>> + *
>> + * Copyright 2016 Hans Verkuil <hverkuil@xs4all.nl
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version of 2 of the License, or (at your
>> + * option) any later version. See the file COPYING in the main directory of
>> + * this archive for more details.
>> + */
>> +
>> +/*
>> + * Notes:
>> + *
>> + * The higher level protocols are currently disabled. This can be added
>> + * later, similar to how this is done for the Pulse Eight CEC driver.
>> + *
>> + * Documentation of the protocol is available here:
>> + *
>> + * http://rainshadowtech.com/doc/HDMICECtoUSBandRS232v2.0.pdf
>> + */
>> +
>> +#include <linux/completion.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/serio.h>
>> +#include <linux/slab.h>
>> +#include <linux/time.h>
>> +#include <linux/delay.h>
>> +#include <linux/ctype.h>
>> +
>> +#include <media/cec.h>
>> +
>> +MODULE_AUTHOR("Hans Verkuil <hverkuil@xs4all.nl>");
>> +MODULE_DESCRIPTION("RainShadow Tech HDMI CEC driver");
>> +MODULE_LICENSE("GPL");
>> +
>> +static int debug;
>> +module_param(debug, int, 0644);
>> +MODULE_PARM_DESC(debug, "debug level (0-1)");
>> +
>> +#define DATA_SIZE 256
>> +
>> +struct rain {
>> +	struct device *dev;
>> +	struct serio *serio;
>> +	struct cec_adapter *adap;
>> +	struct completion cmd_done;
>> +	struct work_struct work;
>> +	struct cec_msg rx_msg;
>> +	char data[DATA_SIZE];
>> +	char buf[DATA_SIZE];
>> +	unsigned int idx;
>> +	bool started;
>> +	struct mutex write_lock;
>> +};
>> +
>> +static void rain_irq_work_handler(struct work_struct *work)
>> +{
>> +	struct rain *rain =
>> +		container_of(work, struct rain, work);
>> +	struct cec_msg *msg = &rain->rx_msg;
>> +	const char *data = rain->data + 3;
>> +	int stat = -1;
>> +
>> +	msg->len = 0;
>> +	for (; *data; data++) {
>> +		if (!isxdigit(*data))
>> +			continue;
>> +		if (isxdigit(data[0]) && isxdigit(data[1])) {
>> +			if (msg->len == CEC_MAX_MSG_SIZE)
>> +				break;
>> +			hex2bin(msg->msg + msg->len, data, 2);
>> +			msg->len++;
>> +			data++;
>> +			continue;
>> +		}
>> +		if (!data[1])
>> +			stat = hex_to_bin(data[0]);
>> +		break;
>> +	}
>> +
>> +	if (rain->data[0] == 'R') {
>> +		if (stat == 1 || stat == 2)
>> +			cec_received_msg(rain->adap, msg);
>> +		return;
>> +	}
>> +
>> +	switch (stat) {
>> +	case 1:
>> +		cec_transmit_done(rain->adap, CEC_TX_STATUS_OK,
>> +				  0, 0, 0, 0);
>> +		break;
>> +	case 2:
>> +		cec_transmit_done(rain->adap, CEC_TX_STATUS_NACK,
>> +				  0, 1, 0, 0);
>> +		break;
>> +	default:
>> +		cec_transmit_done(rain->adap, CEC_TX_STATUS_LOW_DRIVE,
>> +				  0, 0, 0, 1);
>> +		break;
>> +	}
>> +}
>> +
>> +static irqreturn_t rain_interrupt(struct serio *serio, unsigned char data,
>> +				    unsigned int flags)
>> +{
>> +	struct rain *rain = serio_get_drvdata(serio);
>> +
>> +	if (!rain->started && data != '?')
>> +		return IRQ_HANDLED;
>> +	if (data == '\r') {
>> +		rain->buf[rain->idx] = '\0';
>> +		if (debug)
>> +			dev_info(rain->dev, "received: %s\n", rain->buf);
>
> Just dev_dbg() please. I think everyone is using dynamic debug.

OK.

>
>> +		strcpy(rain->data, rain->buf);
>> +		if (!memcmp(rain->data, "REC", 3) ||
>> +		    !memcmp(rain->data, "STA", 3))
>> +			schedule_work(&rain->work);
>> +		else
>> +			complete(&rain->cmd_done);
>> +		rain->idx = 0;
>> +		rain->started = false;
>> +		return IRQ_HANDLED;
>
> This relies on work to complete before you receive the next message,
> which is not always the case as far as I can understand.

I reworked this. This was in fact buggy, mainly because this adapter has the bad
behavior of echoing the CEC messages it transmits. So I would get a REC and a STA
message in quick succession.

>
>> +	} else if (data == '?') {
>> +		rain->idx = 0;
>> +		rain->started = true;
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	if (data == '\n') {
>
> Should it be a "switch (data) {" ?

Done. That's indeed better.

>
>> +		rain->idx = 0;
>> +		rain->started = false;
>> +		return IRQ_HANDLED;
>> +	}
>> +	if (rain->idx >= DATA_SIZE - 1) {
>> +		dev_dbg(rain->dev,
>> +			"throwing away %d bytes of garbage\n", rain->idx);
>> +		rain->idx = 0;
>> +	}
>> +	rain->buf[rain->idx++] = data;
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static void rain_disconnect(struct serio *serio)
>> +{
>> +	struct rain *rain = serio_get_drvdata(serio);
>> +
>> +	cec_unregister_adapter(rain->adap);
>> +	dev_info(&serio->dev, "disconnected\n");
>> +	serio_close(serio);
>> +	serio_set_drvdata(serio, NULL);
>> +	kfree(rain);
>> +}
>> +
>> +static int rain_send(struct rain *rain, const char *command)
>> +{
>> +	int err = serio_write(rain->serio, '!');
>> +
>> +	if (debug)
>> +		dev_info(rain->dev, "send: %s\n", command);
>> +	while (!err && *command)
>> +		err = serio_write(rain->serio, *command++);
>> +	if (!err)
>> +		err = serio_write(rain->serio, '~');
>> +
>> +	return err;
>> +}
>> +
>> +static int rain_send_and_wait(struct rain *rain,
>> +			      const char *cmd, const char *reply)
>> +{
>> +	int err;
>> +
>> +	init_completion(&rain->cmd_done);
>> +
>> +	mutex_lock(&rain->write_lock);
>> +	err = rain_send(rain, cmd);
>> +	if (err)
>> +		goto err;
>> +
>> +	if (!wait_for_completion_timeout(&rain->cmd_done, HZ)) {
>> +		err = -ETIMEDOUT;
>> +		goto err;
>> +	}
>> +	if (reply && strncmp(rain->data, reply, strlen(reply))) {
>> +		if (debug)
>> +			dev_info(rain->dev,
>> +				 "transmit of '%s': received '%s' instead of '%s'\n",
>> +				 cmd, rain->data, reply);
>> +		err = -EIO;
>> +	}
>> +err:
>> +	mutex_unlock(&rain->write_lock);
>> +	return err;
>> +}
>> +
>> +static int rain_setup(struct rain *rain, struct serio *serio,
>> +			struct cec_log_addrs *log_addrs, u16 *pa)
>> +{
>> +	u16 res;
>> +	int err;
>> +
>> +	err = rain_send_and_wait(rain, "R", "REV");
>> +	if (err)
>> +		return err;
>> +	dev_info(rain->dev, "Firmware version %s\n", rain->data + 4);
>> +
>> +	err = rain_send_and_wait(rain, "Q 1", "QTY");
>> +	if (err)
>> +		return err;
>> +	err = rain_send_and_wait(rain, "c0000", "CFG");
>> +	if (err)
>> +		return err;
>> +	return rain_send_and_wait(rain, "A F 0000", "ADR");
>
> Given that this is instantiated via serport + inputattach the setup
> sequence should be done in inputattach, not kernel.

Certainly not. inputattach knows nothing about CEC, it just sets the right
line discipline.

You don't want to have part of the adapter setup in inputattach and another
part in the CEC driver, that would be impossible to maintain and keep in sync.

Ideally inputattach wouldn't be needed at all, but to my knowledge it is still
not possible to do without this userspace component. Some work was done for
TTY slave devices, but I am not aware of anything being merged. Please let me
know if I am wrong.

Regards,

	Hans