On Thu, Sep 12, 2024 at 2:58 PM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> On Tue, Sep 10, 2024 at 10:15:22AM GMT, Łukasz Bartosik wrote:
> > From: Pavan Holla <pholla@xxxxxxxxxxxx>
> >
> > Implementation of a UCSI transport driver for ChromeOS.
> > This driver will be loaded if the ChromeOS EC implements a PPM.
> >
> > Signed-off-by: Pavan Holla <pholla@xxxxxxxxxxxx>
> > Co-developed-by: Łukasz Bartosik <ukaszb@xxxxxxxxxxxx>
> > Signed-off-by: Łukasz Bartosik <ukaszb@xxxxxxxxxxxx>
> > ---
> > MAINTAINERS | 7 +
> > drivers/usb/typec/ucsi/Kconfig | 13 ++
> > drivers/usb/typec/ucsi/Makefile | 1 +
> > drivers/usb/typec/ucsi/cros_ec_ucsi.c | 273 ++++++++++++++++++++++++++
> > 4 files changed, 294 insertions(+)
> > create mode 100644 drivers/usb/typec/ucsi/cros_ec_ucsi.c
> >
>
> [...]
>
> > +
> > +static int cros_ucsi_sync_control(struct ucsi *ucsi, u64 cmd)
> > +{
> > + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
> > + bool ack = UCSI_COMMAND(cmd) == UCSI_ACK_CC_CI;
> > + int ret;
> > +
> > + if (ack)
> > + set_bit(ACK_PENDING, &udata->flags);
> > + else
> > + set_bit(COMMAND_PENDING, &udata->flags);
> > +
> > + ret = cros_ucsi_async_control(ucsi, cmd);
> > + if (ret)
> > + goto out;
> > +
> > + if (!wait_for_completion_timeout(&udata->complete, WRITE_TMO_MS))
> > + ret = -ETIMEDOUT;
> > +
> > +out:
> > + if (ack)
> > + clear_bit(ACK_PENDING, &udata->flags);
> > + else
> > + clear_bit(COMMAND_PENDING, &udata->flags);
> > + return ret;
> > +}
> > +
> > +struct ucsi_operations cros_ucsi_ops = {
> > + .read_version = cros_ucsi_read_version,
> > + .read_cci = cros_ucsi_read_cci,
> > + .read_message_in = cros_ucsi_read_message_in,
> > + .async_control = cros_ucsi_async_control,
> > + .sync_control = cros_ucsi_sync_control,
>
> Please use ucsi_sync_control_common instead.
>
I will use ucsi_sync_control_common. Thanks for pointing it out.
> > +};
> > +
> > +static void cros_ucsi_work(struct work_struct *work)
> > +{
> > + struct cros_ucsi_data *udata = container_of(work, struct cros_ucsi_data, work);
> > + u32 cci;
> > +
> > + if (cros_ucsi_read_cci(udata->ucsi, &cci))
> > + return;
> > +
> > + if (UCSI_CCI_CONNECTOR(cci))
> > + ucsi_connector_change(udata->ucsi, UCSI_CCI_CONNECTOR(cci));
> > +
> > + if (cci & UCSI_CCI_ACK_COMPLETE &&
> > + test_and_clear_bit(ACK_PENDING, &udata->flags))
> > + complete(&udata->complete);
> > + if (cci & UCSI_CCI_COMMAND_COMPLETE &&
> > + test_and_clear_bit(COMMAND_PENDING, &udata->flags))
> > + complete(&udata->complete);
>
> ucsi_notify_common(). Why are you ignoring these functions?
>
I have missed these common functions mainly because our baseline is on
the v6.6 kernel. I will use ucsi_notify_common() however I noticed one
major difference. The ucsi_notify_common() uses test_bit while above
we use test_and_clear_bit. I will send a separate commit to change
test_bit->test_and_clear_bit in the ucsi_notify_common() because usage
of test_and_clear_bit fixes possible race condition.
Thanks,
Lukasz
> --
> With best wishes
> Dmitry
