Re: [PATCH v6 06/11] cec: add HDMI CEC framework

[Hans Verkuil <hverkuil@xs4all.nl>]

Hi Kamil,

Just two tiny issues, and after that you can add my:

Reviewed-by: Hans Verkuil <hans.verkuil@ciso.com>

to this.

On 05/04/2015 07:32 PM, Kamil Debski wrote:
> diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h
> new file mode 100644
> index 0000000..67b0049
> --- /dev/null
> +++ b/include/uapi/linux/cec.h
> @@ -0,0 +1,332 @@
> +#ifndef _CEC_H
> +#define _CEC_H
> +
> +#include <linux/types.h>
> +
> +struct cec_msg {
> +	__u64 ts;
> +	__u32 len;
> +	__u32 status;
> +	__u32 timeout;
> +	/* timeout (in ms) is used to timeout CEC_RECEIVE.
> +	   Set to 0 if you want to wait forever. */
> +	__u8  msg[16];
> +	__u8  reply;
> +	/* If non-zero, then wait for a reply with this opcode.
> +	   If there was an error when sending the msg or FeatureAbort
> +	   was returned, then reply is set to 0.
> +	   If reply is non-zero upon return, then len/msg are set to
> +	   the received message.
> +	   If reply is zero upon return and status has the
> +	   CEC_TX_STATUS_FEATURE_ABORT bit set, then len/msg are set to the
> +	   received feature abort message.
> +	   If reply is zero upon return and status has the
> +	   CEC_TX_STATUS_REPLY_TIMEOUT
> +	   bit set, then no reply was seen at all.
> +	   This field is ignored with CEC_RECEIVE.
> +	   If reply is non-zero for CEC_TRANSMIT and the message is a broadcast,
> +	   then -EINVAL is returned.
> +	   if reply is non-zero, then timeout is set to 1000 (the required
> +	   maximum response time).
> +	 */
> +	__u32 sequence;
> +	/* The framework assigns a sequence number to messages that are sent.
> +	 * This can be used to track replies to previously sent messages.
> +	 */
> +	__u8 reserved[35];
> +};

It is confusing in struct cec_msg that the comments come *after* the field
they belong to instead of just before. Can you change this?

> +
> +#define CEC_G_EVENT		_IOWR('a', 9, struct cec_event)

This can be __IOR since we never write anything.

> +/*
> +   Read and set the vendor ID of the CEC adapter.
> + */
> +#define CEC_G_VENDOR_ID		_IOR('a', 10, __u32)
> +#define CEC_S_VENDOR_ID		_IOW('a', 11, __u32)
> +/*
> +   Enable/disable the passthrough mode
> + */
> +#define CEC_G_PASSTHROUGH	_IOR('a', 12, __u32)
> +#define CEC_S_PASSTHROUGH	_IOW('a', 13, __u32)
> +
> +#endif
> 

Regards,

	Hans