On Wed, Sep 11, 2024 at 4:09 PM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Tue, Sep 10, 2024 at 10:15:25AM +0000, Łukasz Bartosik wrote:
> > Add netlink to ChromeOS UCSI driver to allow forwarding
> > of UCSI messages to userspace for debugging and testing
> > purposes.
>
> Why does this need to be cros_ec specific?
>
You're right. Netlink does not have to be cros_ec_ucsi specific.
Would you like to have netlink in typec_ucsi ?
> > Signed-off-by: Łukasz Bartosik <ukaszb@xxxxxxxxxxxx>
> > ---
> > MAINTAINERS | 4 +-
> > drivers/usb/typec/ucsi/Makefile | 4 +-
> > .../{cros_ec_ucsi.c => cros_ec_ucsi_main.c} | 66 +++++++++++++-
> > drivers/usb/typec/ucsi/cros_ec_ucsi_nl.c | 87 +++++++++++++++++++
> > drivers/usb/typec/ucsi/cros_ec_ucsi_nl.h | 52 +++++++++++
> > 5 files changed, 209 insertions(+), 4 deletions(-)
> > rename drivers/usb/typec/ucsi/{cros_ec_ucsi.c => cros_ec_ucsi_main.c} (79%)
> > create mode 100644 drivers/usb/typec/ucsi/cros_ec_ucsi_nl.c
> > create mode 100644 drivers/usb/typec/ucsi/cros_ec_ucsi_nl.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index d084f32208f0..2afb406a24ce 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5305,7 +5305,9 @@ M: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
> > M: Łukasz Bartosik <ukaszb@xxxxxxxxxxxx>
> > L: chrome-platform@xxxxxxxxxxxxxxx
> > S: Maintained
> > -F: drivers/usb/typec/ucsi/cros_ec_ucsi.c
> > +F: drivers/usb/typec/ucsi/cros_ec_ucsi_main.c
> > +F: drivers/usb/typec/ucsi/cros_ec_ucsi_nl.c
> > +F: drivers/usb/typec/ucsi/cros_ec_ucsi_nl.h
> > F: drivers/usb/typec/ucsi/cros_ec_ucsi_trace.h
> >
> > CHRONTEL CH7322 CEC DRIVER
> > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> > index be98a879104d..82d960394c39 100644
> > --- a/drivers/usb/typec/ucsi/Makefile
> > +++ b/drivers/usb/typec/ucsi/Makefile
> > @@ -21,5 +21,7 @@ obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o
> > obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
> > obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o
> > obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o
> > -obj-$(CONFIG_CROS_EC_UCSI) += cros_ec_ucsi.o
> > obj-$(CONFIG_UCSI_LENOVO_YOGA_C630) += ucsi_yoga_c630.o
> > +
> > +obj-$(CONFIG_CROS_EC_UCSI) += cros_ec_ucsi.o
> > +cros_ec_ucsi-y := cros_ec_ucsi_main.o cros_ec_ucsi_nl.o
> > diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi_main.c
> > similarity index 79%
> > rename from drivers/usb/typec/ucsi/cros_ec_ucsi.c
> > rename to drivers/usb/typec/ucsi/cros_ec_ucsi_main.c
> > index 70185616ec86..008b61921278 100644
> > --- a/drivers/usb/typec/ucsi/cros_ec_ucsi.c
> > +++ b/drivers/usb/typec/ucsi/cros_ec_ucsi_main.c
> > @@ -19,6 +19,7 @@
> > #define CREATE_TRACE_POINTS
> > #include "ucsi.h"
> > #include "cros_ec_ucsi_trace.h"
> > +#include "cros_ec_ucsi_nl.h"
> >
> > /*
> > * Maximum size in bytes of a UCSI message between AP and EC
> > @@ -43,6 +44,43 @@ struct cros_ucsi_data {
> > unsigned long flags;
> > };
> >
> > +/*
> > + * When set to true the cros_ec_ucsi driver will forward all UCSI messages
> > + * exchanged between OPM <-> PPM to userspace through netlink
> > + */
> > +static bool is_ap_sniffer_en;
> > +
> > +static ssize_t enable_ap_sniffer_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + return sprintf(buf, "%d\n", is_ap_sniffer_en);
> > +}
> > +
> > +static ssize_t enable_ap_sniffer_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + u8 value;
> > +
> > + if (kstrtou8(buf, 0, &value))
> > + return -EINVAL;
> > +
> > + is_ap_sniffer_en = value ? 1 : 0;
> > + return count;
> > +}
> > +
> > +static DEVICE_ATTR_RW(enable_ap_sniffer);
> > +
> > +static struct attribute *cros_ec_ucsi_attrs[] = {
> > + &dev_attr_enable_ap_sniffer.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group cros_ec_ucsi_attrs_grp = {
> > + .attrs = cros_ec_ucsi_attrs,
> > +};
>
> Undocumented sysfs entry.
>
I will document the new sysfs entry.
> > static int cros_ucsi_read(struct ucsi *ucsi, unsigned int offset, void *val,
> > size_t val_len)
> > {
> > @@ -65,6 +103,9 @@ static int cros_ucsi_read(struct ucsi *ucsi, unsigned int offset, void *val,
> > return ret;
> > }
> >
> > + if (is_ap_sniffer_en)
> > + nl_cros_ec_bcast_msg(NL_CROS_EC_TO_PPM, NL_CROS_EC_RD, offset,
> > + val, val_len);
> > trace_cros_ec_opm_to_ppm_rd(offset, val, val_len);
> > return 0;
> > }
> > @@ -101,6 +142,9 @@ static int cros_ucsi_async_control(struct ucsi *ucsi, u64 cmd)
> > return ret;
> > }
> >
> > + if (is_ap_sniffer_en)
> > + nl_cros_ec_bcast_msg(NL_CROS_EC_TO_PPM, NL_CROS_EC_WR,
> > + req->offset, (u8 *) &cmd, sizeof(cmd));
> > trace_cros_ec_opm_to_ppm_wr(req->offset, &cmd, sizeof(cmd));
> > return 0;
> > }
> > @@ -144,6 +188,8 @@ 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 (is_ap_sniffer_en)
> > + nl_cros_ec_bcast_msg(NL_CROS_EC_TO_OPM, 0, 0, NULL, 0);
> > trace_cros_ec_ppm_to_opm(0);
> >
> > if (cros_ucsi_read_cci(udata->ucsi, &cci))
> > @@ -229,13 +275,29 @@ static int cros_ucsi_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > + ret = nl_cros_ec_register();
> > + if (ret) {
> > + dev_err(dev, "failed to register netlink: error=%d", ret);
> > + cros_ucsi_destroy(udata);
> > + return ret;
> > + }
> > +
> > + ret = sysfs_create_group(&dev->kobj, &cros_ec_ucsi_attrs_grp);
> > + if (ret) {
> > + dev_err(dev, "failed to register sysfs group: error=%d", ret);
> > + cros_ucsi_destroy(udata);
> > + return ret;
> > + }
> > +
> > return 0;
> > }
> >
> > -static void cros_ucsi_remove(struct platform_device *dev)
> > +static void cros_ucsi_remove(struct platform_device *pdev)
> > {
> > - struct cros_ucsi_data *udata = platform_get_drvdata(dev);
> > + struct cros_ucsi_data *udata = platform_get_drvdata(pdev);
>
> Please merge that change into the patch 3/8.
>
I will merge this change to the"usb: typec: ucsi: Implement ChromeOS
UCSI driver".
Thanks,
Lukasz
> > + sysfs_remove_group(&pdev->dev.kobj, &cros_ec_ucsi_attrs_grp);
> > + nl_cros_ec_unregister();
> > ucsi_unregister(udata->ucsi);
> > cros_ucsi_destroy(udata);
> > }
>
> thanks,
>
> --
> heikki
