Re: [PATCH 5/9] media: cros-ec-cec: Support multiple ports in MKBP cec_events

[Reka Norman]

Hi Hans,

Thanks for the review.

On Mon, Aug 21, 2023 at 6:51 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> >
> >  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
> >
>