> -----Original Message----- > From: Lee Jones [mailto:lee.jones@xxxxxxxxxx] > Sent: Thursday, October 09, 2014 1:10 PM > To: Muthu Mani > Cc: Samuel Ortiz; Wolfram Sang; Linus Walleij; Alexandre Courbot; > gregkh@xxxxxxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux- > gpio@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; Rajaram Regupathy; Johan Hovold > Subject: Re: [PATCH v3 1/3] mfd: add support for Cypress CYUSBS234 USB > Serial Bridge controller > > 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. Thanks for reviewing! Sure, will fix other files as well. > > > + 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_*)? Ok, will use managed resources. > > > + 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. mfd_add_devices would utlimately invoke the cell drivers' probe before returning and cell drivers use usb_dev in their probe. So, leaving it as such. > > > + 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/ Is there a typo? > > > +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 > This message and any attachments may contain Cypress (or its subsidiaries) > confidential information. If it has been received in error, please advise the > sender and immediately delete this message. ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
