Message-ID: <20160609142325.GD2385@dell>
Date:	Thu, 9 Jun 2016 15:23:25 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	"Andrew F. Davis" <afd@...com>
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 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?

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

> >> +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

Powered by blists - more mailing lists