| 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
| ||
|
Message-ID: <20160609161409.GD5438@dell>
Date: Thu, 9 Jun 2016 17:14:09 +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 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@...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).
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
Powered by blists - more mailing lists