Re: [PATCH v3 1/3] mfd: add support for Cypress CYUSBS234 USB Serial Bridge controller

[Lee Jones <lee.jones@xxxxxxxxxx>]

On Mon, 06 Oct 2014, Muthu Mani wrote:

> Adds support for USB-I2C/GPIO interfaces of Cypress Semiconductor
> CYUSBS234 USB-Serial Bridge controller.
> 
> Details about the device can be found at:
> http://www.cypress.com/?rID=84126
> 
> Signed-off-by: Muthu Mani <muth@xxxxxxxxxxx>
> Signed-off-by: Rajaram Regupathy <rera@xxxxxxxxxxx>
> ---
> Changes since v2:
> * Used auto mfd id to support multiple devices
> * Cleaned up the code
> 
> Changes since v1:
> * Identified different serial interface and loaded correct cell driver
> * Formed a mfd id to support multiple devices
> * Removed unused platform device
> 
>  drivers/mfd/Kconfig           |  12 +++
>  drivers/mfd/Makefile          |   1 +
>  drivers/mfd/cyusbs23x.c       | 167 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/cyusbs23x.h |  62 ++++++++++++++++
>  4 files changed, 242 insertions(+)
>  create mode 100644 drivers/mfd/cyusbs23x.c
>  create mode 100644 include/linux/mfd/cyusbs23x.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index de5abf2..a31e9e3 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -116,6 +116,18 @@ config MFD_ASIC3
>  	  This driver supports the ASIC3 multifunction chip found on many
>  	  PDAs (mainly iPAQ and HTC based ones)
>  
> +config MFD_CYUSBS23X
> +        tristate "Cypress CYUSBS23x USB Serial Bridge controller"

White space issue here.

> +	select MFD_CORE
> +	depends on USB
> +	default n
> +	help
> +	  Say yes here if you want support for Cypress Semiconductor
> +	  CYUSBS23x USB-Serial Bridge controller.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called cyusbs23x.
> +
>  config PMIC_DA903X
>  	bool "Dialog Semiconductor DA9030/DA9034 PMIC Support"
>  	depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f001487..fc5bcd1 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -151,6 +151,7 @@ si476x-core-y := si476x-cmd.o si476x-prop.o si476x-i2c.o
>  obj-$(CONFIG_MFD_SI476X_CORE)	+= si476x-core.o
>  
>  obj-$(CONFIG_MFD_CS5535)	+= cs5535-mfd.o
> +obj-$(CONFIG_MFD_CYUSBS23X)     += cyusbs23x.o
>  obj-$(CONFIG_MFD_OMAP_USB_HOST)	+= omap-usb-host.o omap-usb-tll.o
>  obj-$(CONFIG_MFD_PM8921_CORE) 	+= pm8921-core.o ssbi.o
>  obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
> diff --git a/drivers/mfd/cyusbs23x.c b/drivers/mfd/cyusbs23x.c
> new file mode 100644
> index 0000000..c70ea40
> --- /dev/null
> +++ b/drivers/mfd/cyusbs23x.c
> @@ -0,0 +1,167 @@
> +/*
> + * Cypress USB-Serial Bridge Controller USB adapter driver
> + *
> + * Copyright (c) 2014 Cypress Semiconductor Corporation.
> + *
> + * Author:
> + *   Muthu Mani <muth@xxxxxxxxxxx>
> + *
> + * Additional contributors include:
> + *   Rajaram Regupathy <rera@xxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +/*
> + * This is core MFD driver for Cypress Semiconductor CYUSBS234 USB-Serial
> + * Bridge controller. CYUSBS234 offers a single channel serial interface
> + * (I2C/SPI/UART). It can be configured to enable either of I2C, SPI, UART
> + * interfaces. The GPIOs are also available to access.
> + * Details about the device can be found at:
> + *    http://www.cypress.com/?rID=84126
> + *
> + * Separate cell drivers are available for I2C and GPIO. SPI and UART are not
> + * supported yet. All GPIOs are exposed for get operation. However, only
> + * unused GPIOs can be set.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/mutex.h>
> +

This '\n' is superfluous, please remove it.

> +#include <linux/mfd/core.h>
> +#include <linux/mfd/cyusbs23x.h>
> +
> +#include <linux/usb.h>
> +
> +static const struct usb_device_id cyusbs23x_usb_table[] = {
> +	{ USB_DEVICE(0x04b4, 0x0004) },   /* Cypress Semiconductor */
> +	{ }                               /* Terminating entry */

This comment is not required, please remove it.

> +};
> +
> +MODULE_DEVICE_TABLE(usb, cyusbs23x_usb_table);
> +
> +static const struct mfd_cell cyusbs23x_i2c_devs[] = {
> +	{
> +		.name = "cyusbs23x-i2c",
> +	},
> +	{
> +		.name = "cyusbs23x-gpio",
> +	},
> +};
> +
> +static int update_ep_details(struct usb_interface *interface,
> +				struct cyusbs23x *cyusbs)
> +{
> +	struct usb_host_interface *iface_desc;
> +	struct usb_endpoint_descriptor *ep;
> +	int i;
> +
> +	iface_desc = interface->cur_altsetting;
> +
> +	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> +
> +		ep = &iface_desc->endpoint[i].desc;
> +
> +		if (!cyusbs->bulk_in_ep_num && usb_endpoint_is_bulk_in(ep))
> +			cyusbs->bulk_in_ep_num = ep->bEndpointAddress;
> +		if (!cyusbs->bulk_out_ep_num && usb_endpoint_is_bulk_out(ep))
> +			cyusbs->bulk_out_ep_num = ep->bEndpointAddress;
> +		if (!cyusbs->intr_in_ep_num && usb_endpoint_is_int_in(ep))
> +			cyusbs->intr_in_ep_num = ep->bEndpointAddress;
> +	}

All of the USB specific code in this driver will require a USB Ack.

> +	dev_dbg(&interface->dev, "%s intr_in=%d, bulk_in=%d, bulk_out=%d\n",
> +		__func__, cyusbs->intr_in_ep_num ,
> +		cyusbs->bulk_in_ep_num, cyusbs->bulk_out_ep_num);
> +
> +	if (!cyusbs->bulk_in_ep_num || !cyusbs->bulk_out_ep_num ||
> +		!cyusbs->intr_in_ep_num)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int cyusbs23x_probe(struct usb_interface *interface,
> +			   const struct usb_device_id *id)
> +{
> +	struct cyusbs23x *cyusbs;
> +	const struct mfd_cell *cyusbs23x_devs;
> +	int ret, ndevs = 0;
> +	u8 sub_class;
> +
> +	cyusbs = kzalloc(sizeof(*cyusbs), GFP_KERNEL);

Any reason for not using managed resources (devm_*)?

> +	if (cyusbs == NULL)

if (!cyusbs)

> +		return -ENOMEM;
> +
> +	cyusbs->usb_dev = usb_get_dev(interface_to_usbdev(interface));

Can you do this last?  Then you can remove the 'error' error path.

> +	cyusbs->usb_intf = interface;
> +	cyusbs->intf_num = interface->cur_altsetting->desc.bInterfaceNumber;

If you're already saving 'interface' there is no need to save this
also, just extract it from 'cyusbs->usb_intf' when you need it.

> +	ret = update_ep_details(interface, cyusbs);
> +	if (ret < 0) {

Can ret be > 0?  If not, just do 'if (ret)'

> +		dev_err(&interface->dev, "invalid interface\n");

If you put the error message in update_ep_details() can you print out
a more specific error message and remove this line.

> +		goto error;
> +	}
> +
> +	usb_set_intfdata(interface, cyusbs);
> +
> +	/* Serial interfaces (I2C, SPI, UART) differ in interface subclass */
> +	sub_class = interface->cur_altsetting->desc.bInterfaceSubClass;
> +	switch (sub_class) {

This is a waste of a variable and adds nothing to the code.  Just
switch() for 'interface->cur_altsetting->desc.bInterfaceSubClass'.

> +	case CY_USBS_SCB_I2C:
> +		dev_info(&interface->dev, "I2C interface found\n");

Was it even lost?

Would "using I2C interface" be better?

> +		cyusbs23x_devs = cyusbs23x_i2c_devs;
> +		ndevs = ARRAY_SIZE(cyusbs23x_i2c_devs);
> +		break;

I assume there will be other interfaces supported at a later date?  If
not, this switch() is pretty over-the-top.

> +	default:
> +		dev_err(&interface->dev, "unsupported subclass\n");
> +		ret = -ENODEV;
> +		goto error;
> +	}
> +
> +	ret = mfd_add_devices(&interface->dev, PLATFORM_DEVID_AUTO,
> +				cyusbs23x_devs, ndevs, NULL, 0, NULL);
> +	if (ret != 0) {

if (ret)

> +		dev_err(&interface->dev, "Failed to add mfd devices to core\n");

"Failed to register devices"

> +		goto error;
> +	}
> +
> +	return 0;
> +
> +error:
> +	usb_put_dev(cyusbs->usb_dev);
> +	kfree(cyusbs);
> +
> +	return ret;
> +}
> +
> +static void cyusbs23x_disconnect(struct usb_interface *interface)
> +{
> +	struct cyusbs23x *cyusbs = usb_get_intfdata(interface);
> +
> +	mfd_remove_devices(&interface->dev);
> +	usb_put_dev(cyusbs->usb_dev);
> +	kfree(cyusbs);

If you use managed resources, you can remove this line.

> +	dev_dbg(&interface->dev, "disconnected\n");

Please remove this line.

> +}
> +
> +static struct usb_driver cyusbs23x_usb_driver = {
> +	.name           = "cyusbs23x",
> +	.probe          = cyusbs23x_probe,
> +	.disconnect     = cyusbs23x_disconnect,
> +	.id_table       = cyusbs23x_usb_table,
> +};
> +
> +module_usb_driver(cyusbs23x_usb_driver);
> +
> +MODULE_AUTHOR("Rajaram Regupathy <rera@xxxxxxxxxxx>");
> +MODULE_AUTHOR("Muthu Mani <muth@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Cypress CYUSBS23x mfd core driver");

s/mfd/MFD/

> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/cyusbs23x.h b/include/linux/mfd/cyusbs23x.h
> new file mode 100644
> index 0000000..1580d98
> --- /dev/null
> +++ b/include/linux/mfd/cyusbs23x.h
> @@ -0,0 +1,62 @@
> +/*
> + * Cypress USB-Serial Bridge Controller definitions
> + *
> + * Copyright (c) 2014 Cypress Semiconductor Corporation.
> + *
> + * Author:
> + *   Muthu Mani <muth@xxxxxxxxxxx>
> + *
> + * Additional contributors include:
> + *   Rajaram Regupathy <rera@xxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#ifndef __MFD_CYUSBS23X_H__
> +#define __MFD_CYUSBS23X_H__
> +
> +#include <linux/types.h>
> +#include <linux/usb.h>
> +
> +/* Structure to hold all device specific stuff */
> +struct cyusbs23x {
> +	struct usb_device *usb_dev;
> +	struct usb_interface *usb_intf;
> +
> +	u8 intf_num;
> +	u8 bulk_in_ep_num;
> +	u8 bulk_out_ep_num;
> +	u8 intr_in_ep_num;
> +};
> +
> +enum cy_usbs_vendor_cmds {
> +	CY_I2C_GET_CONFIG_CMD  = 0xC4,
> +	CY_I2C_SET_CONFIG_CMD  = 0xC5,
> +	CY_I2C_WRITE_CMD       = 0xC6,
> +	CY_I2C_READ_CMD        = 0xC7,
> +	CY_I2C_GET_STATUS_CMD  = 0xC8,
> +	CY_I2C_RESET_CMD       = 0xC9,
> +	CY_GPIO_GET_CONFIG_CMD = 0xD8,
> +	CY_GPIO_SET_CONFIG_CMD = 0xD9,
> +	CY_GPIO_GET_VALUE_CMD  = 0xDA,
> +	CY_GPIO_SET_VALUE_CMD  = 0xDB,
> +};
> +
> +/* Serial interfaces (I2C, SPI, UART) differ in interface subclass */
> +enum cy_scb_modes {
> +	CY_USBS_SCB_DISABLED = 0,
> +	CY_USBS_SCB_UART = 1,
> +	CY_USBS_SCB_SPI = 2,
> +	CY_USBS_SCB_I2C = 3

No need to number these.

> +};
> +
> +/* SCB index shift */
> +#define CY_SCB_INDEX_SHIFT      15
> +
> +#define CY_USBS_CTRL_XFER_TIMEOUT	2000
> +#define CY_USBS_BULK_XFER_TIMEOUT	5000
> +#define CY_USBS_INTR_XFER_TIMEOUT	5000
> +
> +#endif /* __MFD_CYUSBS23X_H__ */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html