Message-ID: <57598A99.9010109@ti.com>
Date:	Thu, 9 Jun 2016 10:26:17 -0500
From:	"Andrew F. Davis" <afd@...com>
To:	Lee Jones <lee.jones@...aro.org>
CC:	Wolfram Sang <wsa@...-dreams.de>, <linux-i2c@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] mfd: sm-usb-dig: Add support for the TI SM-USB-DIG

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@...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@...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.
> 

Powered by blists - more mailing lists