| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 10 Oct 2014 14:22:35 +0000
From: Muthu Mani <muth@...ress.com>
To: Lee Jones <lee.jones@...aro.org>
CC: Samuel Ortiz <sameo@...ux.intel.com>,
Wolfram Sang <wsa@...-dreams.de>,
Linus Walleij <linus.walleij@...aro.org>,
Alexandre Courbot <gnurou@...il.com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Rajaram Regupathy <rera@...ress.com>,
"Johan Hovold" <johan@...nel.org>
Subject: RE: [PATCH v3 1/3] mfd: add support for Cypress CYUSBS234 USB Serial
Bridge controller
> -----Original Message-----
> From: Lee Jones [mailto:lee.jones@...aro.org]
> Sent: Thursday, October 09, 2014 1:10 PM
> To: Muthu Mani
> Cc: Samuel Ortiz; Wolfram Sang; Linus Walleij; Alexandre Courbot;
> gregkh@...uxfoundation.org; linux-i2c@...r.kernel.org; linux-
> gpio@...r.kernel.org; linux-usb@...r.kernel.org; linux-
> kernel@...r.kernel.org; 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@...ress.com>
> > Signed-off-by: Rajaram Regupathy <rera@...ress.com>
> > ---
> > 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@...ress.com>
> > + *
> > + * Additional contributors include:
> > + * Rajaram Regupathy <rera@...ress.com>
> > + *
> > + * 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@...ress.com>");
> > +MODULE_AUTHOR("Muthu Mani <muth@...ress.com>");
> > +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@...ress.com>
> > + *
> > + * Additional contributors include:
> > + * Rajaram Regupathy <rera@...ress.com>
> > + *
> > + * 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.
Powered by blists - more mailing lists