Re: [PATCH v2] power: supply: PCHG: Use MKBP for device event handling

[Stephen Boyd]

Quoting Daisuke Nojiri (2022-01-23 17:11:40)
> This patch makes the PCHG driver receive device events through

$ git grep "This patch" -- Documentation/process

i.e. don't use "This patch"

> MKBP protocol since CrOS EC switched to deliver all peripheral
> charge events to the MKBP protocol. This will unify PCHG event
> handling on X86 and ARM.
>
> Signed-off-by: Daisuke Nojiri <dnojiri@xxxxxxxxxxxx>
> ---

Just some nitpicks

> diff --git a/drivers/power/supply/cros_peripheral_charger.c b/drivers/power/supply/cros_peripheral_charger.c
> index 305f10dfc06d1b..cb402f48087ddf 100644
> --- a/drivers/power/supply/cros_peripheral_charger.c
> +++ b/drivers/power/supply/cros_peripheral_charger.c
> @@ -237,46 +238,22 @@ static int cros_pchg_event(const struct charger_data *charger,
>         return NOTIFY_OK;
>  }
>
> -static u32 cros_get_device_event(const struct charger_data *charger)
> -{
> -       struct ec_params_device_event req;
> -       struct ec_response_device_event rsp;
> -       struct device *dev = charger->dev;
> -       int ret;
> -
> -       req.param = EC_DEVICE_EVENT_PARAM_GET_CURRENT_EVENTS;
> -       ret = cros_pchg_ec_command(charger, 0, EC_CMD_DEVICE_EVENT,
> -                                  &req, sizeof(req), &rsp, sizeof(rsp));
> -       if (ret < 0) {
> -               dev_warn(dev, "Unable to get device events (err:%d)\n", ret);
> -               return 0;
> -       }
> -
> -       return rsp.event_mask;
> -}
> -
>  static int cros_ec_notify(struct notifier_block *nb,
>                           unsigned long queued_during_suspend,
>                           void *data)
>  {
>         struct cros_ec_device *ec_dev = (struct cros_ec_device *)data;

Not a problem in this patch but the cast can be dropped.

> -       u32 host_event = cros_ec_get_host_event(ec_dev);
>         struct charger_data *charger =
>                         container_of(nb, struct charger_data, notifier);
> -       u32 device_event_mask;
> +       u32 host_event;
>
> -       if (!host_event)
> +       if (ec_dev->event_data.event_type != EC_MKBP_EVENT_PCHG
> +                       || ec_dev->event_size != sizeof(host_event))

Does checkpatch complain here? Preferably it's written as

       if (ec_dev->event_data.event_type != EC_MKBP_EVENT_PCHG ||
           ec_dev->event_size != sizeof(host_event))

>                 return NOTIFY_DONE;
>
> -       if (!(host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_DEVICE)))
> -               return NOTIFY_DONE;
> +       host_event = get_unaligned_le32(&ec_dev->event_data.data.host_event);
>
> -       /*
> -        * todo: Retrieve device event mask in common place
> -        * (e.g. cros_ec_proto.c).
> -        */
> -       device_event_mask = cros_get_device_event(charger);
> -       if (!(device_event_mask & EC_DEVICE_EVENT_MASK(EC_DEVICE_EVENT_WLC)))
> +       if (!(host_event & EC_MKBP_PCHG_DEVICE_EVENT))
>                 return NOTIFY_DONE;
>
>         return cros_pchg_event(charger, host_event);
> diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> index 271bd87bff0a25..c784bed3388865 100644
> --- a/include/linux/platform_data/cros_ec_commands.h
> +++ b/include/linux/platform_data/cros_ec_commands.h
> @@ -3386,6 +3386,9 @@ enum ec_mkbp_event {
>         /* Send an incoming CEC message to the AP */
>         EC_MKBP_EVENT_CEC_MESSAGE = 9,
>
> +       /* Peripheral device charger event */
> +       EC_MKBP_EVENT_PCHG = 12,
> +
>         /* Number of MKBP events */
>         EC_MKBP_EVENT_COUNT,
>  };
> @@ -5527,6 +5530,67 @@ enum pchg_state {
>         [PCHG_STATE_CONNECTED] = "CONNECTED", \
>         }
>
> +/**

Please use only one '*', i.e. '/*' so that this doesn't trip up
kernel-doc generation that looks for two stars.

> + * Update firmware of peripheral chip
> + */
> +#define EC_CMD_PCHG_UPDATE 0x0136