Re: [PATCH v1 5/6] platform/chrome: Register USB PD charger device

[Lee Jones <lee.jones@linaro.org>]

On Fri, 05 Feb 2016, Tomeu Vizoso wrote:

> Check if a EC considers EC_CMD_USB_PD_PORTS a valid command and register
> a USB PD charger device if so. This check is needed for older versions
> of the ChromeOS EC firmware that don't support the EC_CMD_GET_FEATURES
> command.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> 
>  drivers/mfd/cros_ec.c                 | 15 +++++++++++++++
>  drivers/platform/chrome/cros_ec_dev.c | 34 ++++++++++++++++++++++++++++++++++
>  include/linux/mfd/cros_ec.h           |  2 ++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index fbe78b819fdd..20ca9794d2f3 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -202,5 +202,20 @@ EXPORT_SYMBOL(cros_ec_resume);
>  
>  #endif
>  
> +static const struct mfd_cell cros_usb_pd_charger_devs[] = {
> +	{
> +		.name = "cros-usb-pd-charger",
> +		.id   = -1,
> +	},
> +};
> +
> +int cros_ec_usb_pd_charger_register(struct device *dev)
> +{
> +	return mfd_add_devices(dev, 0, cros_usb_pd_charger_devs,
> +			       ARRAY_SIZE(cros_usb_pd_charger_devs),
> +			       NULL, 0, NULL);
> +}
> +EXPORT_SYMBOL(cros_ec_usb_pd_charger_register);

I'm not keen on this idea at all.

You're calling cros_ec_usb_pd_charger_register() from a device you
registered from this same source file.  Seems very incestuous and
hacky.

It's bad enough that we're calling into the platform driver from here
already, but seeing as we are, just extend the call to
cros_ec_query_all() to suit your purposes.

>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("ChromeOS EC core driver");
> diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
> index d45cd254ed1c..7a97cd313c68 100644
> --- a/drivers/platform/chrome/cros_ec_dev.c
> +++ b/drivers/platform/chrome/cros_ec_dev.c
> @@ -87,6 +87,29 @@ exit:
>  	return ret;
>  }
>  
> +static int cros_ec_has_cmd_usb_pd_ports(struct cros_ec_dev *ec)
> +{
> +	struct cros_ec_command *msg;
> +	int ret;
> +
> +	msg = kmalloc(sizeof(*msg) + sizeof(struct ec_response_usb_pd_ports),
> +		      GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	msg->version = 0;
> +	msg->command = EC_CMD_USB_PD_PORTS + ec->cmd_offset;
> +	msg->insize = sizeof(struct ec_response_usb_pd_ports);
> +	msg->outsize = 0;
> +
> +	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
> +	ret = ret >= 0 && msg->result == EC_RES_SUCCESS;
> +
> +	kfree(msg);
> +
> +	return ret;
> +}
> +
>  /* Device file ops */
>  static int ec_device_open(struct inode *inode, struct file *filp)
>  {
> @@ -269,8 +292,19 @@ static int ec_device_probe(struct platform_device *pdev)
>  		goto dev_reg_failed;
>  	}
>  
> +	/* check whether this EC instance has the PD charge manager */
> +	if (cros_ec_has_cmd_usb_pd_ports(ec)) {
> +		retval = cros_ec_usb_pd_charger_register(dev);
> +		if (retval) {
> +			dev_err(dev, "failed to add usb-pd-charger device\n");
> +			goto pd_reg_failed;
> +		}
> +	}
> +
>  	return 0;
>  
> +pd_reg_failed:
> +	put_device(&ec->class_dev);
>  dev_reg_failed:
>  set_named_failed:
>  	dev_set_drvdata(dev, NULL);
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index c21583411ba0..543191f493a9 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -329,4 +329,6 @@ extern struct attribute_group cros_ec_vbc_attr_group;
>   */
>  uint32_t cros_ec_get_host_event(struct cros_ec_device *ec_dev);
>  
> +int cros_ec_usb_pd_charger_register(struct device *dev);
> +
>  #endif /* __LINUX_MFD_CROS_EC_H */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog