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

Hans Verkuil
2015-05-08T11:30:31Z

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