[PATCH 1/2] mfd: sm-usb-dig: Add support for the TI SM-USB-DIG

[PATCH 1/2] mfd: sm-usb-dig: Add support for the TI SM-USB-DIG

[Andrew F. Davis]

The TI SM-USB-DIG is a USB to SPI/I2C/1Wire/GPIO adapter.
Add MFD core support.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
The SPI, GPIO, and 1Wire drivers are WIP.

 drivers/mfd/Kconfig            |   8 +++
 drivers/mfd/Makefile           |   2 +
 drivers/mfd/sm-usb-dig.c       | 138 +++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/sm-usb-dig.h |  73 ++++++++++++++++++++++
 4 files changed, 221 insertions(+)
 create mode 100644 drivers/mfd/sm-usb-dig.c
 create mode 100644 include/linux/mfd/sm-usb-dig.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 1bcf601..455219a 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1373,6 +1373,14 @@ config MFD_LM3533
 	  additional drivers must be enabled in order to use the LED,
 	  backlight or ambient-light-sensor functionality of the device.
 
+config MFD_SM_USB_DIG
+	tristate "Texas Instruments SM-USB-DIG interface adapter"
+	select MFD_CORE
+	help
+	  Support for the TI SM-USB-DIG USB to SPI/I2C/1Wire/GPIO adapter.
+	  Additional drivers such as SPI_SM_USB_DIG, I2C_SM_USB_DIG, etc. must
+	  be enabled in order to use the functionality of the device.
+
 config MFD_TIMBERDALE
 	tristate "Timberdale FPGA"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 42a66e1..376013e 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -68,6 +68,8 @@ obj-$(CONFIG_MFD_WM8350_I2C)	+= wm8350-i2c.o
 wm8994-objs			:= wm8994-core.o wm8994-irq.o wm8994-regmap.o
 obj-$(CONFIG_MFD_WM8994)	+= wm8994.o
 
+obj-$(CONFIG_MFD_SM_USB_DIG)    += sm-usb-dig.o
+
 obj-$(CONFIG_TPS6105X)		+= tps6105x.o
 obj-$(CONFIG_TPS65010)		+= tps65010.o
 obj-$(CONFIG_TPS6507X)		+= tps6507x.o
diff --git a/drivers/mfd/sm-usb-dig.c b/drivers/mfd/sm-usb-dig.c
new file mode 100644
index 0000000..cf7ccab
--- /dev/null
+++ b/drivers/mfd/sm-usb-dig.c
@@ -0,0 +1,138 @@
+/*
+ * MFD Core driver for TI SM-USB-DIG
+ *
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
+ *	Andrew F. Davis <afd@ti.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 program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether expressed or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License version 2 for more details.
+ */
+
+#include <linux/usb.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+
+#include <linux/mfd/sm-usb-dig.h>
+
+#define USB_VENDOR_ID_TI                0x0451
+#define USB_DEVICE_ID_TI_SM_USB_DIG	0x2f90
+
+#define SMUSBDIG_USB_TIMEOUT		1000	/* in ms */
+
+struct smusbdig_device {
+	struct usb_device *usb_dev;
+	struct usb_interface *interface;
+};
+
+int smusbdig_xfer(struct smusbdig_device *smusbdig, u8 *buffer, int size)
+{
+	struct device *dev = &smusbdig->interface->dev;
+	int actual_length, ret;
+
+	if (!smusbdig || !buffer || size <= 0)
+		return -EINVAL;
+
+	ret = usb_interrupt_msg(smusbdig->usb_dev,
+				usb_sndctrlpipe(smusbdig->usb_dev, 1),
+				buffer, size, &actual_length,
+				SMUSBDIG_USB_TIMEOUT);
+	if (ret) {
+		dev_err(dev, "USB transaction failed\n");
+		return ret;
+	}
+
+	ret = usb_interrupt_msg(smusbdig->usb_dev,
+				usb_rcvctrlpipe(smusbdig->usb_dev, 1),
+				buffer, SMUSBDIG_PACKET_SIZE, &actual_length,
+				SMUSBDIG_USB_TIMEOUT);
+	if (ret) {
+		dev_err(dev, "USB transaction failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(smusbdig_xfer);
+
+static const struct mfd_cell smusbdig_mfd_cells[] = {
+	{ .name = "sm-usb-dig-gpio", },
+	{ .name = "sm-usb-dig-i2c", },
+	{ .name = "sm-usb-dig-spi", },
+	{ .name = "sm-usb-dig-w1", },
+};
+
+static int smusbdig_probe(struct usb_interface *interface,
+			  const struct usb_device_id *usb_id)
+{
+	struct usb_host_interface *hostif = interface->cur_altsetting;
+	struct device *dev = &interface->dev;
+	struct smusbdig_device *smusbdig;
+	u8 buffer[SMUSBDIG_PACKET_SIZE];
+	int ret;
+
+	if (hostif->desc.bInterfaceNumber != 0 ||
+	    hostif->desc.bNumEndpoints < 2)
+		return -ENODEV;
+
+	smusbdig = devm_kzalloc(dev, sizeof(*smusbdig), GFP_KERNEL);
+	if (!smusbdig)
+		return -ENOMEM;
+
+	smusbdig->usb_dev = usb_get_dev(interface_to_usbdev(interface));
+	smusbdig->interface = interface;
+	usb_set_intfdata(interface, smusbdig);
+
+	buffer[0] = SMUSBDIG_VERSION;
+	ret = smusbdig_xfer(smusbdig, buffer, 1);
+	if (ret)
+		return ret;
+
+	dev_info(dev, "TI SM-USB-DIG Version: %d.%02d Found\n",
+		 buffer[0], buffer[1]);
+
+	/* Turn on power supply output */
+	buffer[0] = SMUSBDIG_COMMAND;
+	buffer[1] = SMUSBDIG_COMMAND_DUTPOWERON;
+	ret = smusbdig_xfer(smusbdig, buffer, 2);
+	if (ret)
+		return ret;
+
+	dev_set_drvdata(dev, smusbdig);
+	ret = mfd_add_hotplug_devices(dev, smusbdig_mfd_cells,
+				      ARRAY_SIZE(smusbdig_mfd_cells));
+	if (ret) {
+		dev_err(dev, "unable to add MFD devices\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+void smusbdig_disconnect(struct usb_interface *interface)
+{
+	mfd_remove_devices(&interface->dev);
+}
+
+static const struct usb_device_id smusbdig_id_table[] = {
+	{ USB_DEVICE(USB_VENDOR_ID_TI, USB_DEVICE_ID_TI_SM_USB_DIG) },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(usb, smusbdig_id_table);
+
+static struct usb_driver smusbdig_driver = {
+	.name = "sm-usb-dig",
+	.probe = smusbdig_probe,
+	.disconnect = smusbdig_disconnect,
+	.id_table = smusbdig_id_table,
+};
+module_usb_driver(smusbdig_driver);
+
+MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
+MODULE_DESCRIPTION("Core driver for TI SM-USB-DIG interface adapter");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/sm-usb-dig.h b/include/linux/mfd/sm-usb-dig.h
new file mode 100644
index 0000000..1558ff2
--- /dev/null
+++ b/include/linux/mfd/sm-usb-dig.h
@@ -0,0 +1,73 @@
+/*
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
+ *	Andrew F. Davis <afd@ti.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 program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether expressed or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License version 2 for more details.
+ */
+
+#ifndef __LINUX_MFD_SM_USB_DIG_H
+#define __LINUX_MFD_SM_USB_DIG_H
+
+#define SMUSBDIG_PACKET_SIZE    32
+/* (packet size - packet header */
+#define SMUSBDIG_DATA_SIZE (SMUSBDIG_PACKET_SIZE - 7)
+
+enum smusbdig_function {
+	SMUSBDIG_SPI            = 0x01,
+	SMUSBDIG_I2C            = 0x02,
+	SMUSBDIG_1W             = 0x03,
+	SMUSBDIG_COMMAND        = 0x04,
+	SMUSBDIG_VERSION        = 0x07,
+};
+
+enum smusbdig_sub_command {
+	SMUSBDIG_COMMAND_DUTPOWERON	= 0x01,
+	SMUSBDIG_COMMAND_DUTPOWEROFF	= 0x02,
+};
+
+struct smusbdig_packet {
+	u8 function;
+	u8 channel;
+	u8 edge_polarity;
+	u8 num_commands;
+	u8 is_command_mask[3];
+	u8 data[SMUSBDIG_DATA_SIZE];
+} __packed;
+
+static void smusbdig_packet_add_command(struct smusbdig_packet *packet,
+					int command)
+{
+	int num_commands = packet->num_commands;
+	int mask_number = num_commands / 8;
+	int mask_bit = num_commands % 8;
+
+	if (num_commands >= SMUSBDIG_DATA_SIZE)
+		return;
+
+	packet->is_command_mask[mask_number] |= BIT(7 - mask_bit);
+	packet->data[num_commands] = command;
+	packet->num_commands++;
+}
+
+static void smusbdig_packet_add_data(struct smusbdig_packet *packet, u8 data)
+{
+	int num_commands = packet->num_commands;
+
+	if (num_commands >= SMUSBDIG_DATA_SIZE)
+		return;
+
+	packet->data[num_commands] = data;
+	packet->num_commands++;
+}
+
+struct smusbdig_device;
+int smusbdig_xfer(struct smusbdig_device *smusbdig, u8 *buffer, int size);
+
+#endif /* __LINUX_MFD_SM_USB_DIG_H */
-- 
2.8.3

[PATCH 2/2] i2c: sm-usb-dig: add TI SM-USB-DIG I2C bus driver

[Andrew F. Davis]

Add support for the I2C bus functionality of the TI SM-USB-DIG.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/i2c/busses/Kconfig          |  10 ++
 drivers/i2c/busses/Makefile         |   1 +
 drivers/i2c/busses/i2c-sm-usb-dig.c | 190 ++++++++++++++++++++++++++++++++++++
 3 files changed, 201 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-sm-usb-dig.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index f167021..5c74d09 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1076,6 +1076,16 @@ config I2C_ROBOTFUZZ_OSIF
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-osif.
 
+config I2C_SM_USB_DIG
+	tristate "Texas Instruments SM-USB-DIG I2C interface"
+	depends on MFD_SM_USB_DIG
+	help
+	  This adds support for the I2C bus functionality of the
+	  TI SM-USB-DIG USB interface adapter.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-sm-usb-dig.
+
 config I2C_TAOS_EVM
 	tristate "TAOS evaluation module"
 	depends on TTY
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 37f2819..2ee25ef 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -103,6 +103,7 @@ obj-$(CONFIG_I2C_DLN2)		+= i2c-dln2.o
 obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
 obj-$(CONFIG_I2C_PARPORT_LIGHT)	+= i2c-parport-light.o
 obj-$(CONFIG_I2C_ROBOTFUZZ_OSIF)	+= i2c-robotfuzz-osif.o
+obj-$(CONFIG_I2C_SM_USB_DIG)	+= i2c-sm-usb-dig.o
 obj-$(CONFIG_I2C_TAOS_EVM)	+= i2c-taos-evm.o
 obj-$(CONFIG_I2C_TINY_USB)	+= i2c-tiny-usb.o
 obj-$(CONFIG_I2C_VIPERBOARD)	+= i2c-viperboard.o
diff --git a/drivers/i2c/busses/i2c-sm-usb-dig.c b/drivers/i2c/busses/i2c-sm-usb-dig.c
new file mode 100644
index 0000000..719a8c1
--- /dev/null
+++ b/drivers/i2c/busses/i2c-sm-usb-dig.c
@@ -0,0 +1,190 @@
+/*
+ * I2C bus driver for TI SM-USB-DIG
+ *
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
+ *	Andrew F. Davis <afd@ti.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 program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether expressed or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License version 2 for more details.
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/sm-usb-dig.h>
+
+/* (data size - start condition - address - ACK) / ACK after data byte */
+#define SMUSBDIG_I2C_MAX_MSG ((SMUSBDIG_DATA_SIZE - 3) / 2)
+
+struct smusbdig_i2c {
+	struct device *dev;
+	struct smusbdig_device *smusbdig;
+	struct i2c_adapter adapter;
+};
+
+enum smusbdig_i2c_command {
+	SMUSBDIG_I2C_START = 0x3,
+	SMUSBDIG_I2C_STOP = 0x4,
+	SMUSBDIG_I2C_ACKM = 0x5,
+	SMUSBDIG_I2C_ACKS = 0x6,
+};
+
+static void smusbdig_i2c_packet_init(struct smusbdig_packet *packet)
+{
+	memset(packet, 0, sizeof(*packet));
+	packet->function = SMUSBDIG_I2C;
+	packet->channel = 0x1;
+}
+
+static int smusbdig_i2c_xfer(struct i2c_adapter *adapter,
+			     struct i2c_msg *msgs, int num)
+{
+	struct smusbdig_i2c *smusbdig_i2c = i2c_get_adapdata(adapter);
+	struct smusbdig_packet packet;
+	int i, j, k, ret;
+
+	for (i = 0; i < num; i++) {
+		smusbdig_i2c_packet_init(&packet);
+		smusbdig_packet_add_command(&packet, SMUSBDIG_I2C_START);
+		/* add read bit to address if needed */
+		msgs[i].addr <<= 1;
+		if (msgs[i].flags & I2C_M_RD)
+			msgs[i].addr |= BIT(0);
+		smusbdig_packet_add_data(&packet, msgs[i].addr);
+		smusbdig_packet_add_command(&packet, SMUSBDIG_I2C_ACKS);
+		if (msgs[i].flags & I2C_M_RD) {
+			for (j = 0; j < msgs[i].len; j++) {
+				smusbdig_packet_add_data(&packet, 0xff);
+				smusbdig_packet_add_command(&packet, SMUSBDIG_I2C_ACKM);
+			}
+		} else {
+			for (j = 0; j < msgs[i].len; j++) {
+				smusbdig_packet_add_data(&packet, msgs[i].buf[j]);
+				smusbdig_packet_add_command(&packet, SMUSBDIG_I2C_ACKS);
+			}
+		}
+
+		ret = smusbdig_xfer(smusbdig_i2c->smusbdig,
+				    (u8 *)&packet, sizeof(packet));
+		if (ret)
+			return ret;
+
+		/*
+		 * now we read in any data we got during read MSGs
+		 * and check ACKS
+		 */
+		if (((u8 *)&packet)[2]) {
+			num = -EPROTO;
+			goto stop;
+		}
+		for (j = 0, k = 3; j < msgs[i].len; j++, k += 2) {
+			if (msgs[i].flags & I2C_M_RD) {
+				msgs[i].buf[j] = ((u8 *)&packet)[k];
+			} else if (((u8 *)&packet)[k + 1]) {
+				num = -EPROTO;
+				goto stop;
+			}
+		}
+	}
+
+stop:
+	/* send stop condition */
+	smusbdig_i2c_packet_init(&packet);
+	smusbdig_packet_add_command(&packet, SMUSBDIG_I2C_STOP);
+	ret = smusbdig_xfer(smusbdig_i2c->smusbdig,
+			    (u8 *)&packet, sizeof(packet));
+	if (ret)
+		return ret;
+
+	return num;
+}
+
+static u32 smusbdig_i2c_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm smusbdig_i2c_algo = {
+	.master_xfer = smusbdig_i2c_xfer,
+	.functionality = smusbdig_i2c_func,
+};
+
+static struct i2c_adapter smusbdig_i2c_adapter = {
+	.owner = THIS_MODULE,
+	.class = I2C_CLASS_HWMON,
+	.algo = &smusbdig_i2c_algo,
+};
+
+static struct i2c_adapter_quirks dln2_i2c_quirks = {
+	.max_read_len = SMUSBDIG_I2C_MAX_MSG,
+	.max_write_len = SMUSBDIG_I2C_MAX_MSG,
+};
+
+static int smusbdig_i2c_probe(struct platform_device *pdev)
+{
+	struct smusbdig_i2c *smusbdig_i2c;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	smusbdig_i2c = devm_kzalloc(dev, sizeof(*smusbdig_i2c), GFP_KERNEL);
+	if (!smusbdig_i2c)
+		return -ENOMEM;
+
+	smusbdig_i2c->dev = dev;
+	smusbdig_i2c->smusbdig = dev_get_drvdata(dev->parent);
+	smusbdig_i2c->adapter = smusbdig_i2c_adapter;
+	strlcpy(smusbdig_i2c->adapter.name, dev_name(dev),
+		sizeof(smusbdig_i2c->adapter.name));
+	smusbdig_i2c->adapter.quirks = &dln2_i2c_quirks;
+	smusbdig_i2c->adapter.dev.parent = dev;
+	smusbdig_i2c->adapter.dev.of_node = dev->of_node;
+
+	i2c_set_adapdata(&smusbdig_i2c->adapter, smusbdig_i2c);
+	platform_set_drvdata(pdev, smusbdig_i2c);
+
+	ret = i2c_add_adapter(&smusbdig_i2c->adapter);
+	if (ret) {
+		dev_err(dev, "unable to add I2C adapter\n");
+		return ret;
+	}
+
+	dev_info(dev, "TI SM-USB-DIG Added: I2C Bus\n");
+
+	return 0;
+}
+
+static int smusbdig_i2c_remove(struct platform_device *pdev)
+{
+	struct smusbdig_i2c *smusbdig_i2c = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&smusbdig_i2c->adapter);
+
+	return 0;
+}
+
+static const struct platform_device_id smusbdig_i2c_id_table[] = {
+	{ "sm-usb-dig-i2c", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, smusbdig_i2c_id_table);
+
+static struct platform_driver smusbdig_i2c_driver = {
+	.driver = {
+		.name = "sm-usb-dig-i2c",
+	},
+	.probe = smusbdig_i2c_probe,
+	.remove = smusbdig_i2c_remove,
+	.id_table = smusbdig_i2c_id_table,
+};
+module_platform_driver(smusbdig_i2c_driver);
+
+MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
+MODULE_DESCRIPTION("I2C bus driver for TI SM-USB-DIG interface adapter");
+MODULE_LICENSE("GPL v2");
-- 
2.8.3

Re: [PATCH 1/2] mfd: sm-usb-dig: Add support for the TI SM-USB-DIG

[Lee Jones]

On Tue, 31 May 2016, Andrew F. Davis wrote:

> The TI SM-USB-DIG is a USB to SPI/I2C/1Wire/GPIO adapter.
> Add MFD core support.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
> The SPI, GPIO, and 1Wire drivers are WIP.
> 
>  drivers/mfd/Kconfig            |   8 +++
>  drivers/mfd/Makefile           |   2 +
>  drivers/mfd/sm-usb-dig.c       | 138 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/sm-usb-dig.h |  73 ++++++++++++++++++++++
>  4 files changed, 221 insertions(+)
>  create mode 100644 drivers/mfd/sm-usb-dig.c
>  create mode 100644 include/linux/mfd/sm-usb-dig.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1bcf601..455219a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1373,6 +1373,14 @@ config MFD_LM3533
>  	  additional drivers must be enabled in order to use the LED,
>  	  backlight or ambient-light-sensor functionality of the device.
>  
> +config MFD_SM_USB_DIG
> +	tristate "Texas Instruments SM-USB-DIG interface adapter"

If it is decided that MFD is truly the best place for this driver, you
are still going to need a USB Ack for it.

> +	select MFD_CORE
> +	help
> +	  Support for the TI SM-USB-DIG USB to SPI/I2C/1Wire/GPIO adapter.
> +	  Additional drivers such as SPI_SM_USB_DIG, I2C_SM_USB_DIG, etc. must
> +	  be enabled in order to use the functionality of the device.
> +
>  config MFD_TIMBERDALE
>  	tristate "Timberdale FPGA"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 42a66e1..376013e 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -68,6 +68,8 @@ obj-$(CONFIG_MFD_WM8350_I2C)	+= wm8350-i2c.o
>  wm8994-objs			:= wm8994-core.o wm8994-irq.o wm8994-regmap.o
>  obj-$(CONFIG_MFD_WM8994)	+= wm8994.o
>  
> +obj-$(CONFIG_MFD_SM_USB_DIG)    += sm-usb-dig.o
> +
>  obj-$(CONFIG_TPS6105X)		+= tps6105x.o
>  obj-$(CONFIG_TPS65010)		+= tps65010.o
>  obj-$(CONFIG_TPS6507X)		+= tps6507x.o
> diff --git a/drivers/mfd/sm-usb-dig.c b/drivers/mfd/sm-usb-dig.c
> new file mode 100644
> index 0000000..cf7ccab
> --- /dev/null
> +++ b/drivers/mfd/sm-usb-dig.c

This should probably be ti-sm-usb-dig.c

> @@ -0,0 +1,138 @@
> +/*
> + * MFD Core driver for TI SM-USB-DIG
> + *
> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
> + *	Andrew F. Davis <afd@ti.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 program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether expressed or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License version 2 for more details.
> + */
> +
> +#include <linux/usb.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +
> +#include <linux/mfd/sm-usb-dig.h>

All alphabetical.

> +#define USB_VENDOR_ID_TI                0x0451
> +#define USB_DEVICE_ID_TI_SM_USB_DIG	0x2f90

TI at the beginning.

> +#define SMUSBDIG_USB_TIMEOUT		1000	/* in ms */

Rename to SMUSBDIG_USB_TIMEOUT_MS

> +struct smusbdig_device {
> +	struct usb_device *usb_dev;
> +	struct usb_interface *interface;
> +};

s/smusbdig/ti_smusbdig/

... throughout.

> +int smusbdig_xfer(struct smusbdig_device *smusbdig, u8 *buffer, int size)
> +{
> +	struct device *dev = &smusbdig->interface->dev;
> +	int actual_length, ret;
> +
> +	if (!smusbdig || !buffer || size <= 0)
> +		return -EINVAL;
> +
> +	ret = usb_interrupt_msg(smusbdig->usb_dev,
> +				usb_sndctrlpipe(smusbdig->usb_dev, 1),
> +				buffer, size, &actual_length,
> +				SMUSBDIG_USB_TIMEOUT);
> +	if (ret) {
> +		dev_err(dev, "USB transaction failed\n");
> +		return ret;
> +	}
> +
> +	ret = usb_interrupt_msg(smusbdig->usb_dev,
> +				usb_rcvctrlpipe(smusbdig->usb_dev, 1),
> +				buffer, SMUSBDIG_PACKET_SIZE, &actual_length,
> +				SMUSBDIG_USB_TIMEOUT);
> +	if (ret) {
> +		dev_err(dev, "USB transaction failed\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(smusbdig_xfer);
> +
> +static const struct mfd_cell smusbdig_mfd_cells[] = {
> +	{ .name = "sm-usb-dig-gpio", },
> +	{ .name = "sm-usb-dig-i2c", },
> +	{ .name = "sm-usb-dig-spi", },
> +	{ .name = "sm-usb-dig-w1", },
> +};
> +
> +static int smusbdig_probe(struct usb_interface *interface,
> +			  const struct usb_device_id *usb_id)
> +{
> +	struct usb_host_interface *hostif = interface->cur_altsetting;
> +	struct device *dev = &interface->dev;
> +	struct smusbdig_device *smusbdig;
> +	u8 buffer[SMUSBDIG_PACKET_SIZE];
> +	int ret;
> +
> +	if (hostif->desc.bInterfaceNumber != 0 ||
> +	    hostif->desc.bNumEndpoints < 2)
> +		return -ENODEV;
> +
> +	smusbdig = devm_kzalloc(dev, sizeof(*smusbdig), GFP_KERNEL);
> +	if (!smusbdig)
> +		return -ENOMEM;
> +
> +	smusbdig->usb_dev = usb_get_dev(interface_to_usbdev(interface));
> +	smusbdig->interface = interface;
> +	usb_set_intfdata(interface, smusbdig);
> +
> +	buffer[0] = SMUSBDIG_VERSION;
> +	ret = smusbdig_xfer(smusbdig, buffer, 1);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(dev, "TI SM-USB-DIG Version: %d.%02d Found\n",
> +		 buffer[0], buffer[1]);
> +
> +	/* Turn on power supply output */
> +	buffer[0] = SMUSBDIG_COMMAND;
> +	buffer[1] = SMUSBDIG_COMMAND_DUTPOWERON;
> +	ret = smusbdig_xfer(smusbdig, buffer, 2);
> +	if (ret)
> +		return ret;
> +
> +	dev_set_drvdata(dev, smusbdig);
> +	ret = mfd_add_hotplug_devices(dev, smusbdig_mfd_cells,
> +				      ARRAY_SIZE(smusbdig_mfd_cells));
> +	if (ret) {
> +		dev_err(dev, "unable to add MFD devices\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void smusbdig_disconnect(struct usb_interface *interface)
> +{
> +	mfd_remove_devices(&interface->dev);
> +}
> +
> +static const struct usb_device_id smusbdig_id_table[] = {
> +	{ USB_DEVICE(USB_VENDOR_ID_TI, USB_DEVICE_ID_TI_SM_USB_DIG) },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(usb, smusbdig_id_table);
> +
> +static struct usb_driver smusbdig_driver = {
> +	.name = "sm-usb-dig",
> +	.probe = smusbdig_probe,
> +	.disconnect = smusbdig_disconnect,
> +	.id_table = smusbdig_id_table,
> +};
> +module_usb_driver(smusbdig_driver);

This doesn't look like an MFD driver.

Why aren't you putting this in the USB subsystem?

> +MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
> +MODULE_DESCRIPTION("Core driver for TI SM-USB-DIG interface adapter");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/sm-usb-dig.h b/include/linux/mfd/sm-usb-dig.h
> new file mode 100644
> index 0000000..1558ff2
> --- /dev/null
> +++ b/include/linux/mfd/sm-usb-dig.h
> @@ -0,0 +1,73 @@
> +/*
> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
> + *	Andrew F. Davis <afd@ti.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 program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether expressed or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License version 2 for more details.
> + */
> +
> +#ifndef __LINUX_MFD_SM_USB_DIG_H
> +#define __LINUX_MFD_SM_USB_DIG_H
> +
> +#define SMUSBDIG_PACKET_SIZE    32
> +/* (packet size - packet header */
> +#define SMUSBDIG_DATA_SIZE (SMUSBDIG_PACKET_SIZE - 7)
> +
> +enum smusbdig_function {
> +	SMUSBDIG_SPI            = 0x01,
> +	SMUSBDIG_I2C            = 0x02,
> +	SMUSBDIG_1W             = 0x03,
> +	SMUSBDIG_COMMAND        = 0x04,
> +	SMUSBDIG_VERSION        = 0x07,
> +};
> +
> +enum smusbdig_sub_command {
> +	SMUSBDIG_COMMAND_DUTPOWERON	= 0x01,
> +	SMUSBDIG_COMMAND_DUTPOWEROFF	= 0x02,
> +};
> +
> +struct smusbdig_packet {
> +	u8 function;
> +	u8 channel;
> +	u8 edge_polarity;
> +	u8 num_commands;
> +	u8 is_command_mask[3];
> +	u8 data[SMUSBDIG_DATA_SIZE];
> +} __packed;
> +
> +static void smusbdig_packet_add_command(struct smusbdig_packet *packet,
> +					int command)
> +{
> +	int num_commands = packet->num_commands;
> +	int mask_number = num_commands / 8;
> +	int mask_bit = num_commands % 8;
> +
> +	if (num_commands >= SMUSBDIG_DATA_SIZE)
> +		return;
> +
> +	packet->is_command_mask[mask_number] |= BIT(7 - mask_bit);
> +	packet->data[num_commands] = command;
> +	packet->num_commands++;
> +}

Inline?

> +static void smusbdig_packet_add_data(struct smusbdig_packet *packet, u8 data)
> +{
> +	int num_commands = packet->num_commands;
> +
> +	if (num_commands >= SMUSBDIG_DATA_SIZE)
> +		return;
> +
> +	packet->data[num_commands] = data;
> +	packet->num_commands++;
> +}

Inline?

> +struct smusbdig_device;
> +int smusbdig_xfer(struct smusbdig_device *smusbdig, u8 *buffer, int size);
> +
> +#endif /* __LINUX_MFD_SM_USB_DIG_H */

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

Re: [PATCH 1/2] mfd: sm-usb-dig: Add support for the TI SM-USB-DIG

[Andrew F. Davis]

On 06/08/2016 08:06 AM, Lee Jones wrote:
> On Tue, 31 May 2016, Andrew F. Davis wrote:
> 
>> The TI SM-USB-DIG is a USB to SPI/I2C/1Wire/GPIO adapter.
>> Add MFD core support.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> ---
>> The SPI, GPIO, and 1Wire drivers are WIP.
>>
>>  drivers/mfd/Kconfig            |   8 +++
>>  drivers/mfd/Makefile           |   2 +
>>  drivers/mfd/sm-usb-dig.c       | 138 +++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/sm-usb-dig.h |  73 ++++++++++++++++++++++
>>  4 files changed, 221 insertions(+)
>>  create mode 100644 drivers/mfd/sm-usb-dig.c
>>  create mode 100644 include/linux/mfd/sm-usb-dig.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 1bcf601..455219a 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1373,6 +1373,14 @@ config MFD_LM3533
>>  	  additional drivers must be enabled in order to use the LED,
>>  	  backlight or ambient-light-sensor functionality of the device.
>>  
>> +config MFD_SM_USB_DIG
>> +	tristate "Texas Instruments SM-USB-DIG interface adapter"
> 
> If it is decided that MFD is truly the best place for this driver, you
> are still going to need a USB Ack for it.
> 

Okay, will CC for next version.

>> +	select MFD_CORE
>> +	help
>> +	  Support for the TI SM-USB-DIG USB to SPI/I2C/1Wire/GPIO adapter.
>> +	  Additional drivers such as SPI_SM_USB_DIG, I2C_SM_USB_DIG, etc. must
>> +	  be enabled in order to use the functionality of the device.
>> +
>>  config MFD_TIMBERDALE
>>  	tristate "Timberdale FPGA"
>>  	select MFD_CORE
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 42a66e1..376013e 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -68,6 +68,8 @@ obj-$(CONFIG_MFD_WM8350_I2C)	+= wm8350-i2c.o
>>  wm8994-objs			:= wm8994-core.o wm8994-irq.o wm8994-regmap.o
>>  obj-$(CONFIG_MFD_WM8994)	+= wm8994.o
>>  
>> +obj-$(CONFIG_MFD_SM_USB_DIG)    += sm-usb-dig.o
>> +
>>  obj-$(CONFIG_TPS6105X)		+= tps6105x.o
>>  obj-$(CONFIG_TPS65010)		+= tps65010.o
>>  obj-$(CONFIG_TPS6507X)		+= tps6507x.o
>> diff --git a/drivers/mfd/sm-usb-dig.c b/drivers/mfd/sm-usb-dig.c
>> new file mode 100644
>> index 0000000..cf7ccab
>> --- /dev/null
>> +++ b/drivers/mfd/sm-usb-dig.c
> 
> This should probably be ti-sm-usb-dig.c
> 

There doesn't seem to be a standard of prefixing devices with their
manufacturers name, why would here be any different?

>> @@ -0,0 +1,138 @@
>> +/*
>> + * MFD Core driver for TI SM-USB-DIG
>> + *
>> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
>> + *	Andrew F. Davis <afd@ti.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 program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether expressed or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License version 2 for more details.
>> + */
>> +
>> +#include <linux/usb.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/module.h>
>> +
>> +#include <linux/mfd/sm-usb-dig.h>
> 
> All alphabetical.
> 

ACK

>> +#define USB_VENDOR_ID_TI                0x0451
>> +#define USB_DEVICE_ID_TI_SM_USB_DIG	0x2f90
> 
> TI at the beginning.
> 

ACK

>> +#define SMUSBDIG_USB_TIMEOUT		1000	/* in ms */
> 
> Rename to SMUSBDIG_USB_TIMEOUT_MS
> 

ACK

>> +struct smusbdig_device {
>> +	struct usb_device *usb_dev;
>> +	struct usb_interface *interface;
>> +};
> 
> s/smusbdig/ti_smusbdig/
> 
> ... throughout.
> 

I'm not sure about this, I don't think anyone else will be making one of
these and this only adds a lot of extra characters to a lot of lines.

>> +int smusbdig_xfer(struct smusbdig_device *smusbdig, u8 *buffer, int size)
>> +{
>> +	struct device *dev = &smusbdig->interface->dev;
>> +	int actual_length, ret;
>> +
>> +	if (!smusbdig || !buffer || size <= 0)
>> +		return -EINVAL;
>> +
>> +	ret = usb_interrupt_msg(smusbdig->usb_dev,
>> +				usb_sndctrlpipe(smusbdig->usb_dev, 1),
>> +				buffer, size, &actual_length,
>> +				SMUSBDIG_USB_TIMEOUT);
>> +	if (ret) {
>> +		dev_err(dev, "USB transaction failed\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = usb_interrupt_msg(smusbdig->usb_dev,
>> +				usb_rcvctrlpipe(smusbdig->usb_dev, 1),
>> +				buffer, SMUSBDIG_PACKET_SIZE, &actual_length,
>> +				SMUSBDIG_USB_TIMEOUT);
>> +	if (ret) {
>> +		dev_err(dev, "USB transaction failed\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(smusbdig_xfer);
>> +
>> +static const struct mfd_cell smusbdig_mfd_cells[] = {
>> +	{ .name = "sm-usb-dig-gpio", },
>> +	{ .name = "sm-usb-dig-i2c", },
>> +	{ .name = "sm-usb-dig-spi", },
>> +	{ .name = "sm-usb-dig-w1", },
>> +};
>> +
>> +static int smusbdig_probe(struct usb_interface *interface,
>> +			  const struct usb_device_id *usb_id)
>> +{
>> +	struct usb_host_interface *hostif = interface->cur_altsetting;
>> +	struct device *dev = &interface->dev;
>> +	struct smusbdig_device *smusbdig;
>> +	u8 buffer[SMUSBDIG_PACKET_SIZE];
>> +	int ret;
>> +
>> +	if (hostif->desc.bInterfaceNumber != 0 ||
>> +	    hostif->desc.bNumEndpoints < 2)
>> +		return -ENODEV;
>> +
>> +	smusbdig = devm_kzalloc(dev, sizeof(*smusbdig), GFP_KERNEL);
>> +	if (!smusbdig)
>> +		return -ENOMEM;
>> +
>> +	smusbdig->usb_dev = usb_get_dev(interface_to_usbdev(interface));
>> +	smusbdig->interface = interface;
>> +	usb_set_intfdata(interface, smusbdig);
>> +
>> +	buffer[0] = SMUSBDIG_VERSION;
>> +	ret = smusbdig_xfer(smusbdig, buffer, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_info(dev, "TI SM-USB-DIG Version: %d.%02d Found\n",
>> +		 buffer[0], buffer[1]);
>> +
>> +	/* Turn on power supply output */
>> +	buffer[0] = SMUSBDIG_COMMAND;
>> +	buffer[1] = SMUSBDIG_COMMAND_DUTPOWERON;
>> +	ret = smusbdig_xfer(smusbdig, buffer, 2);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_set_drvdata(dev, smusbdig);
>> +	ret = mfd_add_hotplug_devices(dev, smusbdig_mfd_cells,
>> +				      ARRAY_SIZE(smusbdig_mfd_cells));
>> +	if (ret) {
>> +		dev_err(dev, "unable to add MFD devices\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +void smusbdig_disconnect(struct usb_interface *interface)
>> +{
>> +	mfd_remove_devices(&interface->dev);
>> +}
>> +
>> +static const struct usb_device_id smusbdig_id_table[] = {
>> +	{ USB_DEVICE(USB_VENDOR_ID_TI, USB_DEVICE_ID_TI_SM_USB_DIG) },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(usb, smusbdig_id_table);
>> +
>> +static struct usb_driver smusbdig_driver = {
>> +	.name = "sm-usb-dig",
>> +	.probe = smusbdig_probe,
>> +	.disconnect = smusbdig_disconnect,
>> +	.id_table = smusbdig_id_table,
>> +};
>> +module_usb_driver(smusbdig_driver);
> 
> This doesn't look like an MFD driver.
> 
> Why aren't you putting this in the USB subsystem?
> 

This is not a USB driver, it just attaches to the USB bus like other
drivers in this subsystem that attach to SPI/I2C/Platform buses, drivers
tend to go into folders based on the functionality they expose, and this
exposes multiple functions, not USB functionality, so MFD makes the most
sense to me.

This device/driver is just like the dln2 and viperboard drivers
currently in the MFD subsystem.

>> +MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
>> +MODULE_DESCRIPTION("Core driver for TI SM-USB-DIG interface adapter");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/sm-usb-dig.h b/include/linux/mfd/sm-usb-dig.h
>> new file mode 100644
>> index 0000000..1558ff2
>> --- /dev/null
>> +++ b/include/linux/mfd/sm-usb-dig.h
>> @@ -0,0 +1,73 @@
>> +/*
>> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
>> + *	Andrew F. Davis <afd@ti.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 program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether expressed or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License version 2 for more details.
>> + */
>> +
>> +#ifndef __LINUX_MFD_SM_USB_DIG_H
>> +#define __LINUX_MFD_SM_USB_DIG_H
>> +
>> +#define SMUSBDIG_PACKET_SIZE    32
>> +/* (packet size - packet header */
>> +#define SMUSBDIG_DATA_SIZE (SMUSBDIG_PACKET_SIZE - 7)
>> +
>> +enum smusbdig_function {
>> +	SMUSBDIG_SPI            = 0x01,
>> +	SMUSBDIG_I2C            = 0x02,
>> +	SMUSBDIG_1W             = 0x03,
>> +	SMUSBDIG_COMMAND        = 0x04,
>> +	SMUSBDIG_VERSION        = 0x07,
>> +};
>> +
>> +enum smusbdig_sub_command {
>> +	SMUSBDIG_COMMAND_DUTPOWERON	= 0x01,
>> +	SMUSBDIG_COMMAND_DUTPOWEROFF	= 0x02,
>> +};
>> +
>> +struct smusbdig_packet {
>> +	u8 function;
>> +	u8 channel;
>> +	u8 edge_polarity;
>> +	u8 num_commands;
>> +	u8 is_command_mask[3];
>> +	u8 data[SMUSBDIG_DATA_SIZE];
>> +} __packed;
>> +
>> +static void smusbdig_packet_add_command(struct smusbdig_packet *packet,
>> +					int command)
>> +{
>> +	int num_commands = packet->num_commands;
>> +	int mask_number = num_commands / 8;
>> +	int mask_bit = num_commands % 8;
>> +
>> +	if (num_commands >= SMUSBDIG_DATA_SIZE)
>> +		return;
>> +
>> +	packet->is_command_mask[mask_number] |= BIT(7 - mask_bit);
>> +	packet->data[num_commands] = command;
>> +	packet->num_commands++;
>> +}
> 
> Inline?
> 

ACK

>> +static void smusbdig_packet_add_data(struct smusbdig_packet *packet, u8 data)
>> +{
>> +	int num_commands = packet->num_commands;
>> +
>> +	if (num_commands >= SMUSBDIG_DATA_SIZE)
>> +		return;
>> +
>> +	packet->data[num_commands] = data;
>> +	packet->num_commands++;
>> +}
> 
> Inline?
> 

ACK

>> +struct smusbdig_device;
>> +int smusbdig_xfer(struct smusbdig_device *smusbdig, u8 *buffer, int size);
>> +
>> +#endif /* __LINUX_MFD_SM_USB_DIG_H */
> 

Re: [PATCH 1/2] mfd: sm-usb-dig: Add support for the TI SM-USB-DIG

[Lee Jones]

On Wed, 08 Jun 2016, Andrew F. Davis wrote:

> On 06/08/2016 08:06 AM, Lee Jones wrote:
> > On Tue, 31 May 2016, Andrew F. Davis wrote:
> > 
> >> The TI SM-USB-DIG is a USB to SPI/I2C/1Wire/GPIO adapter.
> >> Add MFD core support.
> >>
> >> Signed-off-by: Andrew F. Davis <afd@ti.com>
> >> ---
> >> The SPI, GPIO, and 1Wire drivers are WIP.
> >>
> >>  drivers/mfd/Kconfig            |   8 +++
> >>  drivers/mfd/Makefile           |   2 +
> >>  drivers/mfd/sm-usb-dig.c       | 138 +++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/mfd/sm-usb-dig.h |  73 ++++++++++++++++++++++
> >>  4 files changed, 221 insertions(+)
> >>  create mode 100644 drivers/mfd/sm-usb-dig.c
> >>  create mode 100644 include/linux/mfd/sm-usb-dig.h
> >>
> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >> index 1bcf601..455219a 100644
> >> --- a/drivers/mfd/Kconfig
> >> +++ b/drivers/mfd/Kconfig
> >> @@ -1373,6 +1373,14 @@ config MFD_LM3533
> >>  	  additional drivers must be enabled in order to use the LED,
> >>  	  backlight or ambient-light-sensor functionality of the device.
> >>  
> >> +config MFD_SM_USB_DIG
> >> +	tristate "Texas Instruments SM-USB-DIG interface adapter"
> > 
> > If it is decided that MFD is truly the best place for this driver, you
> > are still going to need a USB Ack for it.
> > 
> 
> Okay, will CC for next version.
> 
> >> +	select MFD_CORE
> >> +	help
> >> +	  Support for the TI SM-USB-DIG USB to SPI/I2C/1Wire/GPIO adapter.
> >> +	  Additional drivers such as SPI_SM_USB_DIG, I2C_SM_USB_DIG, etc. must
> >> +	  be enabled in order to use the functionality of the device.
> >> +
> >>  config MFD_TIMBERDALE
> >>  	tristate "Timberdale FPGA"
> >>  	select MFD_CORE
> >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> >> index 42a66e1..376013e 100644
> >> --- a/drivers/mfd/Makefile
> >> +++ b/drivers/mfd/Makefile
> >> @@ -68,6 +68,8 @@ obj-$(CONFIG_MFD_WM8350_I2C)	+= wm8350-i2c.o
> >>  wm8994-objs			:= wm8994-core.o wm8994-irq.o wm8994-regmap.o
> >>  obj-$(CONFIG_MFD_WM8994)	+= wm8994.o
> >>  
> >> +obj-$(CONFIG_MFD_SM_USB_DIG)    += sm-usb-dig.o
> >> +
> >>  obj-$(CONFIG_TPS6105X)		+= tps6105x.o
> >>  obj-$(CONFIG_TPS65010)		+= tps65010.o
> >>  obj-$(CONFIG_TPS6507X)		+= tps6507x.o
> >> diff --git a/drivers/mfd/sm-usb-dig.c b/drivers/mfd/sm-usb-dig.c
> >> new file mode 100644
> >> index 0000000..cf7ccab
> >> --- /dev/null
> >> +++ b/drivers/mfd/sm-usb-dig.c
> > 
> > This should probably be ti-sm-usb-dig.c
> > 
> 
> There doesn't seem to be a standard of prefixing devices with their
> manufacturers name, why would here be any different?

Because most drivers have a standard naming convention; maxim, da, lp,
tps, wm, etc.  So they are easy to group and categorise.  Others use
their company or family name; qcom, st, omap, etc, which has the
same effect.  Where as "sm" doesn't really tell me much.

What does the SM stand for anyway?

> >> @@ -0,0 +1,138 @@
> >> +/*
> >> + * MFD Core driver for TI SM-USB-DIG
> >> + *
> >> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
> >> + *	Andrew F. Davis <afd@ti.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 program is distributed "as is" WITHOUT ANY WARRANTY of any
> >> + * kind, whether expressed or implied; without even the implied warranty
> >> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License version 2 for more details.
> >> + */
> >> +
> >> +#include <linux/usb.h>
> >> +#include <linux/mfd/core.h>
> >> +#include <linux/module.h>
> >> +
> >> +#include <linux/mfd/sm-usb-dig.h>
> > 
> > All alphabetical.
> > 
> 
> ACK
> 
> >> +#define USB_VENDOR_ID_TI                0x0451
> >> +#define USB_DEVICE_ID_TI_SM_USB_DIG	0x2f90
> > 
> > TI at the beginning.
> > 
> 
> ACK
> 
> >> +#define SMUSBDIG_USB_TIMEOUT		1000	/* in ms */
> > 
> > Rename to SMUSBDIG_USB_TIMEOUT_MS
> > 
> 
> ACK
> 
> >> +struct smusbdig_device {
> >> +	struct usb_device *usb_dev;
> >> +	struct usb_interface *interface;
> >> +};
> > 
> > s/smusbdig/ti_smusbdig/
> > 
> > ... throughout.
> > 
> 
> I'm not sure about this, I don't think anyone else will be making one of
> these and this only adds a lot of extra characters to a lot of lines.

We usually prefix functions by vendor or platform name by convention.

Examples:

  git grep "static struct.*(" -- drivers/mfd/ drivers/spi

> >> +int smusbdig_xfer(struct smusbdig_device *smusbdig, u8 *buffer, int size)
> >> +{
> >> +	struct device *dev = &smusbdig->interface->dev;
> >> +	int actual_length, ret;
> >> +
> >> +	if (!smusbdig || !buffer || size <= 0)
> >> +		return -EINVAL;
> >> +
> >> +	ret = usb_interrupt_msg(smusbdig->usb_dev,
> >> +				usb_sndctrlpipe(smusbdig->usb_dev, 1),
> >> +				buffer, size, &actual_length,
> >> +				SMUSBDIG_USB_TIMEOUT);
> >> +	if (ret) {
> >> +		dev_err(dev, "USB transaction failed\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = usb_interrupt_msg(smusbdig->usb_dev,
> >> +				usb_rcvctrlpipe(smusbdig->usb_dev, 1),
> >> +				buffer, SMUSBDIG_PACKET_SIZE, &actual_length,
> >> +				SMUSBDIG_USB_TIMEOUT);
> >> +	if (ret) {
> >> +		dev_err(dev, "USB transaction failed\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(smusbdig_xfer);
> >> +
> >> +static const struct mfd_cell smusbdig_mfd_cells[] = {
> >> +	{ .name = "sm-usb-dig-gpio", },
> >> +	{ .name = "sm-usb-dig-i2c", },
> >> +	{ .name = "sm-usb-dig-spi", },
> >> +	{ .name = "sm-usb-dig-w1", },
> >> +};
> >> +
> >> +static int smusbdig_probe(struct usb_interface *interface,
> >> +			  const struct usb_device_id *usb_id)
> >> +{
> >> +	struct usb_host_interface *hostif = interface->cur_altsetting;
> >> +	struct device *dev = &interface->dev;
> >> +	struct smusbdig_device *smusbdig;
> >> +	u8 buffer[SMUSBDIG_PACKET_SIZE];
> >> +	int ret;
> >> +
> >> +	if (hostif->desc.bInterfaceNumber != 0 ||
> >> +	    hostif->desc.bNumEndpoints < 2)
> >> +		return -ENODEV;
> >> +
> >> +	smusbdig = devm_kzalloc(dev, sizeof(*smusbdig), GFP_KERNEL);
> >> +	if (!smusbdig)
> >> +		return -ENOMEM;
> >> +
> >> +	smusbdig->usb_dev = usb_get_dev(interface_to_usbdev(interface));
> >> +	smusbdig->interface = interface;
> >> +	usb_set_intfdata(interface, smusbdig);
> >> +
> >> +	buffer[0] = SMUSBDIG_VERSION;
> >> +	ret = smusbdig_xfer(smusbdig, buffer, 1);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	dev_info(dev, "TI SM-USB-DIG Version: %d.%02d Found\n",
> >> +		 buffer[0], buffer[1]);
> >> +
> >> +	/* Turn on power supply output */
> >> +	buffer[0] = SMUSBDIG_COMMAND;
> >> +	buffer[1] = SMUSBDIG_COMMAND_DUTPOWERON;
> >> +	ret = smusbdig_xfer(smusbdig, buffer, 2);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	dev_set_drvdata(dev, smusbdig);
> >> +	ret = mfd_add_hotplug_devices(dev, smusbdig_mfd_cells,
> >> +				      ARRAY_SIZE(smusbdig_mfd_cells));
> >> +	if (ret) {
> >> +		dev_err(dev, "unable to add MFD devices\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +void smusbdig_disconnect(struct usb_interface *interface)
> >> +{
> >> +	mfd_remove_devices(&interface->dev);
> >> +}
> >> +
> >> +static const struct usb_device_id smusbdig_id_table[] = {
> >> +	{ USB_DEVICE(USB_VENDOR_ID_TI, USB_DEVICE_ID_TI_SM_USB_DIG) },
> >> +	{ /* sentinel */ }
> >> +};
> >> +MODULE_DEVICE_TABLE(usb, smusbdig_id_table);
> >> +
> >> +static struct usb_driver smusbdig_driver = {
> >> +	.name = "sm-usb-dig",
> >> +	.probe = smusbdig_probe,
> >> +	.disconnect = smusbdig_disconnect,
> >> +	.id_table = smusbdig_id_table,
> >> +};
> >> +module_usb_driver(smusbdig_driver);
> > 
> > This doesn't look like an MFD driver.
> > 
> > Why aren't you putting this in the USB subsystem?
> > 
> 
> This is not a USB driver, it just attaches to the USB bus like other
> drivers in this subsystem that attach to SPI/I2C/Platform buses, drivers
> tend to go into folders based on the functionality they expose, and this
> exposes multiple functions, not USB functionality, so MFD makes the most
> sense to me.
> 
> This device/driver is just like the dln2 and viperboard drivers
> currently in the MFD subsystem.

Okay.

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

Re: [PATCH 1/2] mfd: sm-usb-dig: Add support for the TI SM-USB-DIG

[Andrew F. Davis]

On 06/09/2016 09:23 AM, Lee Jones wrote:
> On Wed, 08 Jun 2016, Andrew F. Davis wrote:
> 
>> On 06/08/2016 08:06 AM, Lee Jones wrote:
>>> On Tue, 31 May 2016, Andrew F. Davis wrote:
>>>
>>>> The TI SM-USB-DIG is a USB to SPI/I2C/1Wire/GPIO adapter.
>>>> Add MFD core support.
>>>>
>>>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>>>> ---
>>>> The SPI, GPIO, and 1Wire drivers are WIP.
>>>>
>>>>  drivers/mfd/Kconfig            |   8 +++
>>>>  drivers/mfd/Makefile           |   2 +
>>>>  drivers/mfd/sm-usb-dig.c       | 138 +++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/mfd/sm-usb-dig.h |  73 ++++++++++++++++++++++
>>>>  4 files changed, 221 insertions(+)
>>>>  create mode 100644 drivers/mfd/sm-usb-dig.c
>>>>  create mode 100644 include/linux/mfd/sm-usb-dig.h
>>>>
>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>> index 1bcf601..455219a 100644
>>>> --- a/drivers/mfd/Kconfig
>>>> +++ b/drivers/mfd/Kconfig
>>>> @@ -1373,6 +1373,14 @@ config MFD_LM3533
>>>>  	  additional drivers must be enabled in order to use the LED,
>>>>  	  backlight or ambient-light-sensor functionality of the device.
>>>>  
>>>> +config MFD_SM_USB_DIG
>>>> +	tristate "Texas Instruments SM-USB-DIG interface adapter"
>>>
>>> If it is decided that MFD is truly the best place for this driver, you
>>> are still going to need a USB Ack for it.
>>>
>>
>> Okay, will CC for next version.
>>
>>>> +	select MFD_CORE
>>>> +	help
>>>> +	  Support for the TI SM-USB-DIG USB to SPI/I2C/1Wire/GPIO adapter.
>>>> +	  Additional drivers such as SPI_SM_USB_DIG, I2C_SM_USB_DIG, etc. must
>>>> +	  be enabled in order to use the functionality of the device.
>>>> +
>>>>  config MFD_TIMBERDALE
>>>>  	tristate "Timberdale FPGA"
>>>>  	select MFD_CORE
>>>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>>>> index 42a66e1..376013e 100644
>>>> --- a/drivers/mfd/Makefile
>>>> +++ b/drivers/mfd/Makefile
>>>> @@ -68,6 +68,8 @@ obj-$(CONFIG_MFD_WM8350_I2C)	+= wm8350-i2c.o
>>>>  wm8994-objs			:= wm8994-core.o wm8994-irq.o wm8994-regmap.o
>>>>  obj-$(CONFIG_MFD_WM8994)	+= wm8994.o
>>>>  
>>>> +obj-$(CONFIG_MFD_SM_USB_DIG)    += sm-usb-dig.o
>>>> +
>>>>  obj-$(CONFIG_TPS6105X)		+= tps6105x.o
>>>>  obj-$(CONFIG_TPS65010)		+= tps65010.o
>>>>  obj-$(CONFIG_TPS6507X)		+= tps6507x.o
>>>> diff --git a/drivers/mfd/sm-usb-dig.c b/drivers/mfd/sm-usb-dig.c
>>>> new file mode 100644
>>>> index 0000000..cf7ccab
>>>> --- /dev/null
>>>> +++ b/drivers/mfd/sm-usb-dig.c
>>>
>>> This should probably be ti-sm-usb-dig.c
>>>
>>
>> There doesn't seem to be a standard of prefixing devices with their
>> manufacturers name, why would here be any different?
> 
> Because most drivers have a standard naming convention; maxim, da, lp,
> tps, wm, etc.  So they are easy to group and categorise.  Others use
> their company or family name; qcom, st, omap, etc, which has the
> same effect.  Where as "sm" doesn't really tell me much.
> 
> What does the SM stand for anyway?
> 

I have no idea :), I think the original version may have only supported
SMbus.

>>>> @@ -0,0 +1,138 @@
>>>> +/*
>>>> + * MFD Core driver for TI SM-USB-DIG
>>>> + *
>>>> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
>>>> + *	Andrew F. Davis <afd@ti.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 program is distributed "as is" WITHOUT ANY WARRANTY of any
>>>> + * kind, whether expressed or implied; without even the implied warranty
>>>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License version 2 for more details.
>>>> + */
>>>> +
>>>> +#include <linux/usb.h>
>>>> +#include <linux/mfd/core.h>
>>>> +#include <linux/module.h>
>>>> +
>>>> +#include <linux/mfd/sm-usb-dig.h>
>>>
>>> All alphabetical.
>>>
>>
>> ACK
>>
>>>> +#define USB_VENDOR_ID_TI                0x0451
>>>> +#define USB_DEVICE_ID_TI_SM_USB_DIG	0x2f90
>>>
>>> TI at the beginning.
>>>
>>
>> ACK
>>
>>>> +#define SMUSBDIG_USB_TIMEOUT		1000	/* in ms */
>>>
>>> Rename to SMUSBDIG_USB_TIMEOUT_MS
>>>
>>
>> ACK
>>
>>>> +struct smusbdig_device {
>>>> +	struct usb_device *usb_dev;
>>>> +	struct usb_interface *interface;
>>>> +};
>>>
>>> s/smusbdig/ti_smusbdig/
>>>
>>> ... throughout.
>>>
>>
>> I'm not sure about this, I don't think anyone else will be making one of
>> these and this only adds a lot of extra characters to a lot of lines.
> 
> We usually prefix functions by vendor or platform name by convention.
> 
> Examples:
> 
>   git grep "static struct.*(" -- drivers/mfd/ drivers/spi
> 

What was this supposed to return? It didn't give me any examples of
vendor prefixed functions. They are all prefixed by the driver name,
like I'm doing here (smusbdig).

Not really a big deal, if you think it's better this way, I'll make the
change for v2.

Thanks,
Andrew

>>>> +int smusbdig_xfer(struct smusbdig_device *smusbdig, u8 *buffer, int size)
>>>> +{
>>>> +	struct device *dev = &smusbdig->interface->dev;
>>>> +	int actual_length, ret;
>>>> +
>>>> +	if (!smusbdig || !buffer || size <= 0)
>>>> +		return -EINVAL;
>>>> +
>>>> +	ret = usb_interrupt_msg(smusbdig->usb_dev,
>>>> +				usb_sndctrlpipe(smusbdig->usb_dev, 1),
>>>> +				buffer, size, &actual_length,
>>>> +				SMUSBDIG_USB_TIMEOUT);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "USB transaction failed\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	ret = usb_interrupt_msg(smusbdig->usb_dev,
>>>> +				usb_rcvctrlpipe(smusbdig->usb_dev, 1),
>>>> +				buffer, SMUSBDIG_PACKET_SIZE, &actual_length,
>>>> +				SMUSBDIG_USB_TIMEOUT);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "USB transaction failed\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(smusbdig_xfer);
>>>> +
>>>> +static const struct mfd_cell smusbdig_mfd_cells[] = {
>>>> +	{ .name = "sm-usb-dig-gpio", },
>>>> +	{ .name = "sm-usb-dig-i2c", },
>>>> +	{ .name = "sm-usb-dig-spi", },
>>>> +	{ .name = "sm-usb-dig-w1", },
>>>> +};
>>>> +
>>>> +static int smusbdig_probe(struct usb_interface *interface,
>>>> +			  const struct usb_device_id *usb_id)
>>>> +{
>>>> +	struct usb_host_interface *hostif = interface->cur_altsetting;
>>>> +	struct device *dev = &interface->dev;
>>>> +	struct smusbdig_device *smusbdig;
>>>> +	u8 buffer[SMUSBDIG_PACKET_SIZE];
>>>> +	int ret;
>>>> +
>>>> +	if (hostif->desc.bInterfaceNumber != 0 ||
>>>> +	    hostif->desc.bNumEndpoints < 2)
>>>> +		return -ENODEV;
>>>> +
>>>> +	smusbdig = devm_kzalloc(dev, sizeof(*smusbdig), GFP_KERNEL);
>>>> +	if (!smusbdig)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	smusbdig->usb_dev = usb_get_dev(interface_to_usbdev(interface));
>>>> +	smusbdig->interface = interface;
>>>> +	usb_set_intfdata(interface, smusbdig);
>>>> +
>>>> +	buffer[0] = SMUSBDIG_VERSION;
>>>> +	ret = smusbdig_xfer(smusbdig, buffer, 1);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	dev_info(dev, "TI SM-USB-DIG Version: %d.%02d Found\n",
>>>> +		 buffer[0], buffer[1]);
>>>> +
>>>> +	/* Turn on power supply output */
>>>> +	buffer[0] = SMUSBDIG_COMMAND;
>>>> +	buffer[1] = SMUSBDIG_COMMAND_DUTPOWERON;
>>>> +	ret = smusbdig_xfer(smusbdig, buffer, 2);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	dev_set_drvdata(dev, smusbdig);
>>>> +	ret = mfd_add_hotplug_devices(dev, smusbdig_mfd_cells,
>>>> +				      ARRAY_SIZE(smusbdig_mfd_cells));
>>>> +	if (ret) {
>>>> +		dev_err(dev, "unable to add MFD devices\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +void smusbdig_disconnect(struct usb_interface *interface)
>>>> +{
>>>> +	mfd_remove_devices(&interface->dev);
>>>> +}
>>>> +
>>>> +static const struct usb_device_id smusbdig_id_table[] = {
>>>> +	{ USB_DEVICE(USB_VENDOR_ID_TI, USB_DEVICE_ID_TI_SM_USB_DIG) },
>>>> +	{ /* sentinel */ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(usb, smusbdig_id_table);
>>>> +
>>>> +static struct usb_driver smusbdig_driver = {
>>>> +	.name = "sm-usb-dig",
>>>> +	.probe = smusbdig_probe,
>>>> +	.disconnect = smusbdig_disconnect,
>>>> +	.id_table = smusbdig_id_table,
>>>> +};
>>>> +module_usb_driver(smusbdig_driver);
>>>
>>> This doesn't look like an MFD driver.
>>>
>>> Why aren't you putting this in the USB subsystem?
>>>
>>
>> This is not a USB driver, it just attaches to the USB bus like other
>> drivers in this subsystem that attach to SPI/I2C/Platform buses, drivers
>> tend to go into folders based on the functionality they expose, and this
>> exposes multiple functions, not USB functionality, so MFD makes the most
>> sense to me.
>>
>> This device/driver is just like the dln2 and viperboard drivers
>> currently in the MFD subsystem.
> 
> Okay.
> 

Re: [PATCH 1/2] mfd: sm-usb-dig: Add support for the TI SM-USB-DIG

[Lee Jones]

On Thu, 09 Jun 2016, Andrew F. Davis wrote:

> On 06/09/2016 09:23 AM, Lee Jones wrote:
> > On Wed, 08 Jun 2016, Andrew F. Davis wrote:
> > 
> >> On 06/08/2016 08:06 AM, Lee Jones wrote:
> >>> On Tue, 31 May 2016, Andrew F. Davis wrote:
> >>>
> >>>> The TI SM-USB-DIG is a USB to SPI/I2C/1Wire/GPIO adapter.
> >>>> Add MFD core support.
> >>>>
> >>>> Signed-off-by: Andrew F. Davis <afd@ti.com>
> >>>> ---
> >>>> The SPI, GPIO, and 1Wire drivers are WIP.
> >>>>
> >>>>  drivers/mfd/Kconfig            |   8 +++
> >>>>  drivers/mfd/Makefile           |   2 +
> >>>>  drivers/mfd/sm-usb-dig.c       | 138 +++++++++++++++++++++++++++++++++++++++++
> >>>>  include/linux/mfd/sm-usb-dig.h |  73 ++++++++++++++++++++++
> >>>>  4 files changed, 221 insertions(+)
> >>>>  create mode 100644 drivers/mfd/sm-usb-dig.c
> >>>>  create mode 100644 include/linux/mfd/sm-usb-dig.h
> >>>>
> >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >>>> index 1bcf601..455219a 100644
> >>>> --- a/drivers/mfd/Kconfig
> >>>> +++ b/drivers/mfd/Kconfig
> >>>> @@ -1373,6 +1373,14 @@ config MFD_LM3533
> >>>>  	  additional drivers must be enabled in order to use the LED,
> >>>>  	  backlight or ambient-light-sensor functionality of the device.
> >>>>  
> >>>> +config MFD_SM_USB_DIG
> >>>> +	tristate "Texas Instruments SM-USB-DIG interface adapter"
> >>>
> >>> If it is decided that MFD is truly the best place for this driver, you
> >>> are still going to need a USB Ack for it.
> >>>
> >>
> >> Okay, will CC for next version.
> >>
> >>>> +	select MFD_CORE
> >>>> +	help
> >>>> +	  Support for the TI SM-USB-DIG USB to SPI/I2C/1Wire/GPIO adapter.
> >>>> +	  Additional drivers such as SPI_SM_USB_DIG, I2C_SM_USB_DIG, etc. must
> >>>> +	  be enabled in order to use the functionality of the device.
> >>>> +
> >>>>  config MFD_TIMBERDALE
> >>>>  	tristate "Timberdale FPGA"
> >>>>  	select MFD_CORE
> >>>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> >>>> index 42a66e1..376013e 100644
> >>>> --- a/drivers/mfd/Makefile
> >>>> +++ b/drivers/mfd/Makefile
> >>>> @@ -68,6 +68,8 @@ obj-$(CONFIG_MFD_WM8350_I2C)	+= wm8350-i2c.o
> >>>>  wm8994-objs			:= wm8994-core.o wm8994-irq.o wm8994-regmap.o
> >>>>  obj-$(CONFIG_MFD_WM8994)	+= wm8994.o
> >>>>  
> >>>> +obj-$(CONFIG_MFD_SM_USB_DIG)    += sm-usb-dig.o
> >>>> +
> >>>>  obj-$(CONFIG_TPS6105X)		+= tps6105x.o
> >>>>  obj-$(CONFIG_TPS65010)		+= tps65010.o
> >>>>  obj-$(CONFIG_TPS6507X)		+= tps6507x.o
> >>>> diff --git a/drivers/mfd/sm-usb-dig.c b/drivers/mfd/sm-usb-dig.c
> >>>> new file mode 100644
> >>>> index 0000000..cf7ccab
> >>>> --- /dev/null
> >>>> +++ b/drivers/mfd/sm-usb-dig.c
> >>>
> >>> This should probably be ti-sm-usb-dig.c
> >>>
> >>
> >> There doesn't seem to be a standard of prefixing devices with their
> >> manufacturers name, why would here be any different?
> > 
> > Because most drivers have a standard naming convention; maxim, da, lp,
> > tps, wm, etc.  So they are easy to group and categorise.  Others use
> > their company or family name; qcom, st, omap, etc, which has the
> > same effect.  Where as "sm" doesn't really tell me much.
> > 
> > What does the SM stand for anyway?
> > 
> 
> I have no idea :), I think the original version may have only supported
> SMbus.
> 
> >>>> @@ -0,0 +1,138 @@
> >>>> +/*
> >>>> + * MFD Core driver for TI SM-USB-DIG
> >>>> + *
> >>>> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
> >>>> + *	Andrew F. Davis <afd@ti.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 program is distributed "as is" WITHOUT ANY WARRANTY of any
> >>>> + * kind, whether expressed or implied; without even the implied warranty
> >>>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>>> + * GNU General Public License version 2 for more details.
> >>>> + */
> >>>> +
> >>>> +#include <linux/usb.h>
> >>>> +#include <linux/mfd/core.h>
> >>>> +#include <linux/module.h>
> >>>> +
> >>>> +#include <linux/mfd/sm-usb-dig.h>
> >>>
> >>> All alphabetical.
> >>>
> >>
> >> ACK
> >>
> >>>> +#define USB_VENDOR_ID_TI                0x0451
> >>>> +#define USB_DEVICE_ID_TI_SM_USB_DIG	0x2f90
> >>>
> >>> TI at the beginning.
> >>>
> >>
> >> ACK
> >>
> >>>> +#define SMUSBDIG_USB_TIMEOUT		1000	/* in ms */
> >>>
> >>> Rename to SMUSBDIG_USB_TIMEOUT_MS
> >>>
> >>
> >> ACK
> >>
> >>>> +struct smusbdig_device {
> >>>> +	struct usb_device *usb_dev;
> >>>> +	struct usb_interface *interface;
> >>>> +};
> >>>
> >>> s/smusbdig/ti_smusbdig/
> >>>
> >>> ... throughout.
> >>>
> >>
> >> I'm not sure about this, I don't think anyone else will be making one of
> >> these and this only adds a lot of extra characters to a lot of lines.
> > 
> > We usually prefix functions by vendor or platform name by convention.
> > 
> > Examples:
> > 
> >   git grep "static struct.*(" -- drivers/mfd/ drivers/spi
> > 
> 
> What was this supposed to return? It didn't give me any examples of
> vendor prefixed functions. They are all prefixed by the driver name,
> like I'm doing here (smusbdig).

Which is why I am also suggesting you change the driver name. ;)

It would be good to group TI's drivers together, and 3 chars really
isn't an issue.

  ti_smusbdig.c

> Not really a big deal, if you think it's better this way, I'll make the
> change for v2.
> 
> Thanks,
> Andrew
> 
> >>>> +int smusbdig_xfer(struct smusbdig_device *smusbdig, u8 *buffer, int size)
> >>>> +{
> >>>> +	struct device *dev = &smusbdig->interface->dev;
> >>>> +	int actual_length, ret;
> >>>> +
> >>>> +	if (!smusbdig || !buffer || size <= 0)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	ret = usb_interrupt_msg(smusbdig->usb_dev,
> >>>> +				usb_sndctrlpipe(smusbdig->usb_dev, 1),
> >>>> +				buffer, size, &actual_length,
> >>>> +				SMUSBDIG_USB_TIMEOUT);
> >>>> +	if (ret) {
> >>>> +		dev_err(dev, "USB transaction failed\n");
> >>>> +		return ret;
> >>>> +	}
> >>>> +
> >>>> +	ret = usb_interrupt_msg(smusbdig->usb_dev,
> >>>> +				usb_rcvctrlpipe(smusbdig->usb_dev, 1),
> >>>> +				buffer, SMUSBDIG_PACKET_SIZE, &actual_length,
> >>>> +				SMUSBDIG_USB_TIMEOUT);
> >>>> +	if (ret) {
> >>>> +		dev_err(dev, "USB transaction failed\n");
> >>>> +		return ret;
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(smusbdig_xfer);
> >>>> +
> >>>> +static const struct mfd_cell smusbdig_mfd_cells[] = {
> >>>> +	{ .name = "sm-usb-dig-gpio", },
> >>>> +	{ .name = "sm-usb-dig-i2c", },
> >>>> +	{ .name = "sm-usb-dig-spi", },
> >>>> +	{ .name = "sm-usb-dig-w1", },
> >>>> +};
> >>>> +
> >>>> +static int smusbdig_probe(struct usb_interface *interface,
> >>>> +			  const struct usb_device_id *usb_id)
> >>>> +{
> >>>> +	struct usb_host_interface *hostif = interface->cur_altsetting;
> >>>> +	struct device *dev = &interface->dev;
> >>>> +	struct smusbdig_device *smusbdig;
> >>>> +	u8 buffer[SMUSBDIG_PACKET_SIZE];
> >>>> +	int ret;
> >>>> +
> >>>> +	if (hostif->desc.bInterfaceNumber != 0 ||
> >>>> +	    hostif->desc.bNumEndpoints < 2)
> >>>> +		return -ENODEV;
> >>>> +
> >>>> +	smusbdig = devm_kzalloc(dev, sizeof(*smusbdig), GFP_KERNEL);
> >>>> +	if (!smusbdig)
> >>>> +		return -ENOMEM;
> >>>> +
> >>>> +	smusbdig->usb_dev = usb_get_dev(interface_to_usbdev(interface));
> >>>> +	smusbdig->interface = interface;
> >>>> +	usb_set_intfdata(interface, smusbdig);
> >>>> +
> >>>> +	buffer[0] = SMUSBDIG_VERSION;
> >>>> +	ret = smusbdig_xfer(smusbdig, buffer, 1);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	dev_info(dev, "TI SM-USB-DIG Version: %d.%02d Found\n",
> >>>> +		 buffer[0], buffer[1]);
> >>>> +
> >>>> +	/* Turn on power supply output */
> >>>> +	buffer[0] = SMUSBDIG_COMMAND;
> >>>> +	buffer[1] = SMUSBDIG_COMMAND_DUTPOWERON;
> >>>> +	ret = smusbdig_xfer(smusbdig, buffer, 2);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	dev_set_drvdata(dev, smusbdig);
> >>>> +	ret = mfd_add_hotplug_devices(dev, smusbdig_mfd_cells,
> >>>> +				      ARRAY_SIZE(smusbdig_mfd_cells));
> >>>> +	if (ret) {
> >>>> +		dev_err(dev, "unable to add MFD devices\n");
> >>>> +		return ret;
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +void smusbdig_disconnect(struct usb_interface *interface)
> >>>> +{
> >>>> +	mfd_remove_devices(&interface->dev);
> >>>> +}
> >>>> +
> >>>> +static const struct usb_device_id smusbdig_id_table[] = {
> >>>> +	{ USB_DEVICE(USB_VENDOR_ID_TI, USB_DEVICE_ID_TI_SM_USB_DIG) },
> >>>> +	{ /* sentinel */ }
> >>>> +};
> >>>> +MODULE_DEVICE_TABLE(usb, smusbdig_id_table);
> >>>> +
> >>>> +static struct usb_driver smusbdig_driver = {
> >>>> +	.name = "sm-usb-dig",
> >>>> +	.probe = smusbdig_probe,
> >>>> +	.disconnect = smusbdig_disconnect,
> >>>> +	.id_table = smusbdig_id_table,
> >>>> +};
> >>>> +module_usb_driver(smusbdig_driver);
> >>>
> >>> This doesn't look like an MFD driver.
> >>>
> >>> Why aren't you putting this in the USB subsystem?
> >>>
> >>
> >> This is not a USB driver, it just attaches to the USB bus like other
> >> drivers in this subsystem that attach to SPI/I2C/Platform buses, drivers
> >> tend to go into folders based on the functionality they expose, and this
> >> exposes multiple functions, not USB functionality, so MFD makes the most
> >> sense to me.
> >>
> >> This device/driver is just like the dln2 and viperboard drivers
> >> currently in the MFD subsystem.
> > 
> > Okay.
> > 

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