| Message ID | 20230814043140.1108917-6-rekanorman@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: cros-ec-cec: Add support for multiple ports | expand |
Hi Reka, On 14/08/2023 06:29, Reka Norman wrote: > Use the top four bits of the cec_events MKBP event to store the port > number. > > Signed-off-by: Reka Norman <rekanorman@chromium.org> > --- > > drivers/media/cec/platform/cros-ec/cros-ec-cec.c | 13 +++++++++++-- > include/linux/platform_data/cros_ec_commands.h | 10 ++++++++++ > 2 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/cec/platform/cros-ec/cros-ec-cec.c b/drivers/media/cec/platform/cros-ec/cros-ec-cec.c > index d674a432dfdd..eb4b778f83e9 100644 > --- a/drivers/media/cec/platform/cros-ec/cros-ec-cec.c > +++ b/drivers/media/cec/platform/cros-ec/cros-ec-cec.c > @@ -77,8 +77,17 @@ static void handle_cec_message(struct cros_ec_cec *cros_ec_cec) > static void handle_cec_event(struct cros_ec_cec *cros_ec_cec) > { > struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec; > - uint32_t events = cros_ec->event_data.data.cec_events; > - struct cros_ec_cec_port *port = cros_ec_cec->ports[CEC_PORT]; > + uint32_t cec_events = cros_ec->event_data.data.cec_events; > + int port_num = EC_MKBP_EVENT_CEC_GET_PORT(cec_events); > + uint32_t events = EC_MKBP_EVENT_CEC_GET_EVENTS(cec_events); > + struct cros_ec_cec_port *port; > + > + if (port_num < 0 || port_num >= cros_ec_cec->num_ports) { Since cec_events is unsigned, then I would also keep port_num unsigned. Mixing signed and unsigned for bit shifting is a bit dangerous. That will also mean that you can skip the 'port_num < 0' check. Regards, Hans > + dev_err(cros_ec->dev, > + "received CEC event for invalid port %d\n", port_num); > + return; > + } > + port = cros_ec_cec->ports[port_num]; > > if (events & EC_MKBP_CEC_SEND_OK) > cec_transmit_attempt_done(port->adap, CEC_TX_STATUS_OK); > diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h > index 9a0c6e28f370..b7e8573a8a49 100644 > --- a/include/linux/platform_data/cros_ec_commands.h > +++ b/include/linux/platform_data/cros_ec_commands.h > @@ -4440,6 +4440,16 @@ struct ec_response_i2c_passthru_protect { > > #define MAX_CEC_MSG_LEN 16 > > +/* > + * Helper macros for packing/unpacking cec_events. > + * bits[27:0] : bitmask of events from enum mkbp_cec_event > + * bits[31:28]: port number > + */ > +#define EC_MKBP_EVENT_CEC_PACK(events, port) \ > + (((events) & GENMASK(27, 0)) | (((port) & 0xf) << 28)) > +#define EC_MKBP_EVENT_CEC_GET_EVENTS(event) ((event) & GENMASK(27, 0)) > +#define EC_MKBP_EVENT_CEC_GET_PORT(event) (((event) >> 28) & 0xf) > + > /* CEC message from the AP to be written on the CEC bus */ > #define EC_CMD_CEC_WRITE_MSG 0x00B8 >
Hi Hans, Thanks for the review. On Mon, Aug 21, 2023 at 6:51 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > Hi Reka, > > On 14/08/2023 06:29, Reka Norman wrote: > > Use the top four bits of the cec_events MKBP event to store the port > > number. > > > > Signed-off-by: Reka Norman <rekanorman@chromium.org> > > --- > > > > drivers/media/cec/platform/cros-ec/cros-ec-cec.c | 13 +++++++++++-- > > include/linux/platform_data/cros_ec_commands.h | 10 ++++++++++ > > 2 files changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/cec/platform/cros-ec/cros-ec-cec.c b/drivers/media/cec/platform/cros-ec/cros-ec-cec.c > > index d674a432dfdd..eb4b778f83e9 100644 > > --- a/drivers/media/cec/platform/cros-ec/cros-ec-cec.c > > +++ b/drivers/media/cec/platform/cros-ec/cros-ec-cec.c > > @@ -77,8 +77,17 @@ static void handle_cec_message(struct cros_ec_cec *cros_ec_cec) > > static void handle_cec_event(struct cros_ec_cec *cros_ec_cec) > > { > > struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec; > > - uint32_t events = cros_ec->event_data.data.cec_events; > > - struct cros_ec_cec_port *port = cros_ec_cec->ports[CEC_PORT]; > > + uint32_t cec_events = cros_ec->event_data.data.cec_events; > > + int port_num = EC_MKBP_EVENT_CEC_GET_PORT(cec_events); > > + uint32_t events = EC_MKBP_EVENT_CEC_GET_EVENTS(cec_events); > > + struct cros_ec_cec_port *port; > > + > > + if (port_num < 0 || port_num >= cros_ec_cec->num_ports) { > > Since cec_events is unsigned, then I would also keep port_num unsigned. > Mixing signed and unsigned for bit shifting is a bit dangerous. > > That will also mean that you can skip the 'port_num < 0' check. Done in v2. > > Regards, > > Hans > > > + dev_err(cros_ec->dev, > > + "received CEC event for invalid port %d\n", port_num); > > + return; > > + } > > + port = cros_ec_cec->ports[port_num]; > > > > if (events & EC_MKBP_CEC_SEND_OK) > > cec_transmit_attempt_done(port->adap, CEC_TX_STATUS_OK); > > diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h > > index 9a0c6e28f370..b7e8573a8a49 100644 > > --- a/include/linux/platform_data/cros_ec_commands.h > > +++ b/include/linux/platform_data/cros_ec_commands.h > > @@ -4440,6 +4440,16 @@ struct ec_response_i2c_passthru_protect { > > > > #define MAX_CEC_MSG_LEN 16 > > > > +/* > > + * Helper macros for packing/unpacking cec_events. > > + * bits[27:0] : bitmask of events from enum mkbp_cec_event > > + * bits[31:28]: port number > > + */ > > +#define EC_MKBP_EVENT_CEC_PACK(events, port) \ > > + (((events) & GENMASK(27, 0)) | (((port) & 0xf) << 28)) > > +#define EC_MKBP_EVENT_CEC_GET_EVENTS(event) ((event) & GENMASK(27, 0)) > > +#define EC_MKBP_EVENT_CEC_GET_PORT(event) (((event) >> 28) & 0xf) > > + > > /* CEC message from the AP to be written on the CEC bus */ > > #define EC_CMD_CEC_WRITE_MSG 0x00B8 > > >
diff --git a/drivers/media/cec/platform/cros-ec/cros-ec-cec.c b/drivers/media/cec/platform/cros-ec/cros-ec-cec.c index d674a432dfdd..eb4b778f83e9 100644 --- a/drivers/media/cec/platform/cros-ec/cros-ec-cec.c +++ b/drivers/media/cec/platform/cros-ec/cros-ec-cec.c @@ -77,8 +77,17 @@ static void handle_cec_message(struct cros_ec_cec *cros_ec_cec) static void handle_cec_event(struct cros_ec_cec *cros_ec_cec) { struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec; - uint32_t events = cros_ec->event_data.data.cec_events; - struct cros_ec_cec_port *port = cros_ec_cec->ports[CEC_PORT]; + uint32_t cec_events = cros_ec->event_data.data.cec_events; + int port_num = EC_MKBP_EVENT_CEC_GET_PORT(cec_events); + uint32_t events = EC_MKBP_EVENT_CEC_GET_EVENTS(cec_events); + struct cros_ec_cec_port *port; + + if (port_num < 0 || port_num >= cros_ec_cec->num_ports) { + dev_err(cros_ec->dev, + "received CEC event for invalid port %d\n", port_num); + return; + } + port = cros_ec_cec->ports[port_num]; if (events & EC_MKBP_CEC_SEND_OK) cec_transmit_attempt_done(port->adap, CEC_TX_STATUS_OK); diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h index 9a0c6e28f370..b7e8573a8a49 100644 --- a/include/linux/platform_data/cros_ec_commands.h +++ b/include/linux/platform_data/cros_ec_commands.h @@ -4440,6 +4440,16 @@ struct ec_response_i2c_passthru_protect { #define MAX_CEC_MSG_LEN 16 +/* + * Helper macros for packing/unpacking cec_events. + * bits[27:0] : bitmask of events from enum mkbp_cec_event + * bits[31:28]: port number + */ +#define EC_MKBP_EVENT_CEC_PACK(events, port) \ + (((events) & GENMASK(27, 0)) | (((port) & 0xf) << 28)) +#define EC_MKBP_EVENT_CEC_GET_EVENTS(event) ((event) & GENMASK(27, 0)) +#define EC_MKBP_EVENT_CEC_GET_PORT(event) (((event) >> 28) & 0xf) + /* CEC message from the AP to be written on the CEC bus */ #define EC_CMD_CEC_WRITE_MSG 0x00B8
Use the top four bits of the cec_events MKBP event to store the port number. Signed-off-by: Reka Norman <rekanorman@chromium.org> --- drivers/media/cec/platform/cros-ec/cros-ec-cec.c | 13 +++++++++++-- include/linux/platform_data/cros_ec_commands.h | 10 ++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-)