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

[Muthu Mani <muth@xxxxxxxxxxx>]

> -----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�����{��
b���ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f