Message-ID: <9f3a8777-9bd4-5044-7d3b-5ccf25a9b97e@collabora.com>
Date:   Fri, 7 Feb 2020 18:00:54 +0100
From:   Enric Balletbo i Serra <enric.balletbo@...labora.com>
To:     Prashant Malani <pmalani@...omium.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        heikki.krogerus@...el.com, Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>
Subject: Re: [PATCH 1/3] platform/chrome: Add Type C connector class driver

Hi Prashant,

On 7/2/20 0:30, Prashant Malani wrote:
> Hi Enric,
> 
> Thanks for taking a look at the patch. Please see my responses inline
> (I will defer sending the next version till the one pending question I
> had is resolved):
> 
> On Thu, Feb 6, 2020 at 8:19 AM Enric Balletbo i Serra
> <enric.balletbo@...labora.corp-partner.google.com> wrote:
>>
>> Hi Prashant,
>>
>> On 5/2/20 21:59, Prashant Malani wrote:
>>> Add a driver to implement the Type C connector class for Chrome OS
>>> devices with ECs (Embedded Controllers).
>>>
>>> The driver relies on firmware device specifications for various port
>>> attributes. On ACPI platforms, this is specified using the logical
>>> device with HID GOOG0014. On DT platforms, this is specified using the
>>> DT node with compatible string "google,cros-ec-typec".
>>>
>>
>> If that's the case you should document this in a binding file.
> 
> Done. I will add this in the next version of the series.
> 
>> driver a replacement of the cros-ec-extcon driver or is a different thing?
> 
> Currently it is distinct. We're hoping to plug in to type C connector
> class work which Heikki is working on (relating to Type C Mux agent).
> Hopefully, we will gradually transition the extcon functionality to
> the Type C port driver.
> 
>>
>> There is a device where I can test this?
> 
> I think you can try it on kevin if you add the DT node for it (haven't
> tried it myself on kevin). An example will be present in the DT
> Documentation I provide in the next version.
> 
>>
>>> This patch reads the device FW node and uses the port attributes to
>>> register the typec ports with the Type C connector class framework, but
>>> doesn't do much else.
>>>
>>> Subsequent patches will add more functionality to the driver, including
>>> obtaining current port information (polarity, vconn role, current power
>>> role etc.) after querying the EC.
>>>
>>> Signed-off-by: Prashant Malani <pmalani@...omium.org>
>>> ---
>>>  drivers/platform/chrome/Kconfig         |  11 ++
>>>  drivers/platform/chrome/Makefile        |   1 +
>>>  drivers/platform/chrome/cros_ec_typec.c | 228 ++++++++++++++++++++++++
>>>  3 files changed, 240 insertions(+)
>>>  create mode 100644 drivers/platform/chrome/cros_ec_typec.c
>>>
>>> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
>>> index 5f57282a28da00..1370dfd1ca1565 100644
>>> --- a/drivers/platform/chrome/Kconfig
>>> +++ b/drivers/platform/chrome/Kconfig
>>> @@ -214,6 +214,17 @@ config CROS_EC_SYSFS
>>>         To compile this driver as a module, choose M here: the
>>>         module will be called cros_ec_sysfs.
>>>
>>> +config CROS_EC_TYPEC
>>> +     tristate "ChromeOS EC Type-C Connector Control"
>>> +     depends on MFD_CROS_EC_DEV && TYPEC
>>> +     default n
>>
>> Default value is already n, so you don't need to put it here. But I'd say that
>> we might be interested on have default MFD_CROS_EC_DEV like the other drivers.
> 
> Done.
> 
>>
>>> +     help
>>> +       If you say Y here, you get support for accessing Type C connector
>>> +       information from the Chrome OS EC.
>>> +
>>> +       To compile this driver as a module, choose M here: the module will be
>>> +       called cros_ec_typec.
>>> +
>>>  config CROS_USBPD_LOGGER
>>>       tristate "Logging driver for USB PD charger"
>>>       depends on CHARGER_CROS_USBPD
>>> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
>>> index aacd5920d8a180..caf2a9cdb5e6d1 100644
>>> --- a/drivers/platform/chrome/Makefile
>>> +++ b/drivers/platform/chrome/Makefile
>>> @@ -12,6 +12,7 @@ obj-$(CONFIG_CROS_EC_ISHTP)         += cros_ec_ishtp.o
>>>  obj-$(CONFIG_CROS_EC_RPMSG)          += cros_ec_rpmsg.o
>>>  obj-$(CONFIG_CROS_EC_SPI)            += cros_ec_spi.o
>>>  cros_ec_lpcs-objs                    := cros_ec_lpc.o cros_ec_lpc_mec.o
>>> +obj-$(CONFIG_CROS_EC_TYPEC)          += cros_ec_typec.o
>>>  obj-$(CONFIG_CROS_EC_LPC)            += cros_ec_lpcs.o
>>>  obj-$(CONFIG_CROS_EC_PROTO)          += cros_ec_proto.o cros_ec_trace.o
>>>  obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
>>> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
>>> new file mode 100644
>>> index 00000000000000..fe5659171c2a85
>>> --- /dev/null
>>> +++ b/drivers/platform/chrome/cros_ec_typec.c
>>> @@ -0,0 +1,228 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright 2020 Google LLC
>>> + *
>>> + * This driver provides the ability to view and manage Type C ports through the
>>> + * Chrome OS EC.
>>> + */
>>> +
>>> +#include <linux/acpi.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_data/cros_ec_commands.h>
>>> +#include <linux/platform_data/cros_ec_proto.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/usb/typec.h>
>>> +
>>> +#define DRV_NAME "cros-ec-typec"
>>> +
>>> +/* Platform-specific data for the Chrome OS EC Type C controller. */
>>> +struct cros_typec_data {
>>> +     struct device *dev;
>>> +     struct cros_ec_device *ec;
>>> +     int num_ports;
>>> +     /* Array of ports, indexed by port number. */
>>> +     struct typec_port *ports[EC_USB_PD_MAX_PORTS];
>>> +};
>>> +
>>> +int cros_typec_parse_port_props(struct typec_capability *cap,
>>
>> static int
> 
> Sorry, done.
> 
>>
>>> +                             struct fwnode_handle *fwnode,
>>> +                             struct device *dev)
>>> +{
>>> +     const char *buf;
>>> +     int ret;
>>> +
>>> +     memset(cap, 0, sizeof(*cap));
>>> +     ret = fwnode_property_read_string(fwnode, "power-role", &buf);
>>> +     if (ret) {
>>> +             dev_err(dev, "power-role not found: %d\n", ret);
>>> +             return ret;
>>> +     }
>>> +
>>> +     ret = typec_find_port_power_role(buf);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +     cap->type = ret;
>>> +
>>> +     ret = fwnode_property_read_string(fwnode, "data-role", &buf);
>>> +     if (ret) {
>>> +             dev_err(dev, "data-role not found: %d\n", ret);
>>> +             return ret;
>>> +     }
>>> +
>>> +     ret = typec_find_port_data_role(buf);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +     cap->data = ret;
>>> +
>>> +     ret = fwnode_property_read_string(fwnode, "try-power-role", &buf);
>>> +     if (ret) {
>>> +             dev_err(dev, "try-power-role not found: %d\n", ret);
>>> +             return ret;
>>> +     }
>>> +
>>> +     ret = typec_find_power_role(buf);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +     cap->prefer_role = ret;
>>> +
>>> +     cap->fwnode = fwnode;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int cros_typec_init_ports(struct cros_typec_data *typec)
>>> +{
>>> +     struct device *dev = typec->dev;
>>> +     struct typec_capability cap;
>>> +     struct fwnode_handle *fwnode;
>>> +     int ret;
>>> +     int i;
>>> +     int nports;
>>> +     u32 port_num;
>>> +
>>> +     nports = device_get_child_node_count(dev);
>>> +     if (nports == 0) {
>>> +             dev_err(dev, "No port entries found.\n");
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     device_for_each_child_node(dev, fwnode) {
>>> +             if (fwnode_property_read_u32(fwnode, "port-number",
>>> +                                          &port_num)) {
>>> +                     dev_err(dev, "No port-number for port, skipping.\n");
>>> +                     ret = -EINVAL;
>>> +                     goto unregister_ports;
>>> +             }
>>> +
>>> +             if (port_num >= typec->num_ports) {
>>> +                     dev_err(dev, "Invalid port number.\n");
>>> +                     ret = -EINVAL;
>>> +                     goto unregister_ports;
>>> +             }
>>> +
>>> +             dev_dbg(dev, "Registering port %d\n", port_num);
>>> +             ret = cros_typec_parse_port_props(&cap, fwnode, dev);
>>> +             if (ret < 0)
>>> +                     goto unregister_ports;
>>> +             typec->ports[port_num] = typec_register_port(dev, &cap);
>>> +             if (IS_ERR(typec->ports[port_num])) {
>>> +                     dev_err(dev, "Failed to register port %d\n", port_num);
>>> +                     ret = PTR_ERR(typec->ports[port_num]);
>>> +                     goto unregister_ports;
>>> +             }
>>> +     }
>>> +
>>> +     return 0;
>>> +
>>> +unregister_ports:
>>> +     for (i = 0; i < typec->num_ports; i++)
>>> +             typec_unregister_port(typec->ports[i]);
>>> +     return ret;
>>> +}
>>> +
>>> +static int cros_typec_ec_command(struct cros_typec_data *typec,
>>> +                              unsigned int version,
>>> +                              unsigned int command,
>>> +                              void *outdata,
>>> +                              unsigned int outsize,
>>> +                              void *indata,
>>> +                              unsigned int insize)
>>> +{
>>> +     struct cros_ec_command *msg;
>>> +     int ret;
>>> +
>>> +     msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
>>> +     if (!msg)
>>> +             return -ENOMEM;
>>> +
>>> +     msg->version = version;
>>> +     msg->command = command;
>>> +     msg->outsize = outsize;
>>> +     msg->insize = insize;
>>> +
>>> +     if (outsize)
>>> +             memcpy(msg->data, outdata, outsize);
>>> +
>>> +     ret = cros_ec_cmd_xfer_status(typec->ec, msg);
>>> +     if (ret >= 0 && insize)
>>> +             memcpy(indata, msg->data, insize);
>>> +
>>> +     kfree(msg);
>>> +     return ret;
>>> +}
>>> +
>>> +
>>> +static int cros_typec_get_num_ports(struct cros_typec_data *typec)
>>> +{
>>
>> This functions is only called once at probe, you can just put the code there. I
>> had some readibility problems trying to follow de code.
> 
> Done.
> 
>>
>>> +     struct ec_response_usb_pd_ports resp;
>>> +     int ret;
>>> +
>>> +     ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
>>> +                                 &resp, sizeof(resp));
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     typec->num_ports = resp.num_ports;
>>> +     if (typec->num_ports > EC_USB_PD_MAX_PORTS) {
>>> +             dev_warn(typec->dev,
>>> +                      "Too many ports reported: %d, limiting to max: %d\n",
>>> +                      typec->num_ports, EC_USB_PD_MAX_PORTS);
>>
>> You say that you are limiting the number of ports to max but typec->num_ports is
>> still resp.num_ports, is that correct?
>>
> 
> Sorry, thanks for catching this, it should be set to
> EC_USB_PD_MAX_PORTS. I will fix this in the next version.
> 
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +#ifdef CONFIG_ACPI
>>> +static const struct acpi_device_id cros_typec_acpi_id[] = {
>>> +     { "GOOG0014", 0 },
>>> +     { /* sentinel */ }
>>
>> No need to add /* sentinel */ here, is obvious.
> 
> Done.
> 
>>
>>> +};
>>> +MODULE_DEVICE_TABLE(acpi, cros_typec_acpi_id);
>>> +#endif
>>> +
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id cros_typec_of_match[] = {
>>> +     { .compatible = "google,cros-ec-typec", },
>>> +     { /* sentinel */ },
>>
>> No need to add /* sentinel */ here, is obvious. And no need for the colon at the
>> end.
> 
> Done.
>>
>>> +};
>>> +MODULE_DEVICE_TABLE(of, cros_typec_of_match);
>>> +#endif
>>> +
>>> +static int cros_typec_probe(struct platform_device *pdev)
>>> +{
>>> +     struct device *dev = &pdev->dev;
>>> +     struct cros_typec_data *typec;
>>> +     int ret;
>>> +
>>> +     typec = devm_kzalloc(dev, sizeof(*typec), GFP_KERNEL);
>>> +     if (!typec)
>>> +             return -ENOMEM;
>>> +     typec->dev = dev;
>>> +     typec->ec = dev_get_drvdata(pdev->dev.parent);
>>> +     platform_set_drvdata(pdev, typec);
>>> +
>>> +     ret = cros_typec_get_num_ports(typec);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     ret = cros_typec_init_ports(typec);
>>
>> both, cros_typec_get_num_ports and cros_typec_init_ports are only called once,
>> as I said I had some problems of readibility because typec->num_ports is set in
>> one function and unregister in another function when fails.
> 
> Can we keep cros_typec_init_ports() as a separate function? I was
> hoping to avoid cluttering the _probe() with FW node parsing logic.
> cros_typec_init_ports() registers the ports (and unregisters on
> failure), so seems fairly self-contained.
> Of course, happy to place everything in probe if you still prefer that.
> 

I don't mind clutter probe if is more readable but try it, and lets see how
looks like. You are also registering and unregistering the typec ports there,
not only parsing. Usually what we have is parsing the firmware or dt and then
register and unregister the typec ports, all mixed. I'm more in favour of have
it separated, IMHO.

>>
>> I'd just remove those two functions and put the code directly here.
>>
>>> +     if (!ret)
>>> +             return ret;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static struct platform_driver cros_typec_driver = {
>>> +     .driver = {
>>> +             .name   = DRV_NAME,
>>
>> no tab, just space after .-name
> 
> Done.
>>
>>> +             .acpi_match_table = ACPI_PTR(cros_typec_acpi_id),
>>> +             .of_match_table = of_match_ptr(cros_typec_of_match),
>>> +     },
>>> +     .probe          = cros_typec_probe,
>>
>> no tab, just space after .probe
> 
> Done.
> 
>>
>>> +};
>>> +
>>> +module_platform_driver(cros_typec_driver);
>>> +
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_DESCRIPTION("Chrome OS EC Type C control");
>>>
>>
>> You probably want to add the MODULE_AUTHOR here
> 
> Done.
> 
> Best regards,
> 

Powered by blists - more mailing lists