misc: adds support the FSA9480 USB Switch
Subsystems (2)
PATCH 1/1 misc: adds support the FSA9480 USB Switch
The FSA9480 is a USB port accessory detector and switch. This patch adds support the FSA9480 USB Switch.
--- drivers/misc/Kconfig | 9 + drivers/misc/Makefile | 1 + drivers/misc/fsa9480.c | 556 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/fsa9480.h | 40 ++++ 4 files changed, 606 insertions(+), 0 deletions(-) create mode 100644 drivers/misc/fsa9480.c create mode 100644 include/linux/fsa9480.h
drivers/misc/Kconfig +9 -0
@@ -390,6 +390,15 @@ config BMP085 To compile this driver as a module, choose M here: the module will be called bmp085. +config USB_SWITCH_FSA9480 + tristate "FSA9480 USB Switch" + depends on I2C + help + The FSA9480 is a USB port accessory detector and switch. + The FSA9480 is fully controlled using I2C and enables USB data, + stereo and mono audio, video, microphone and UART data to use + a common connector port. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig"
drivers/misc/Makefile +1 -0
@@ -35,3 +35,4 @@ obj-y += eeprom/ obj-y += cb710/ obj-$(CONFIG_VMWARE_BALLOON) += vmware_balloon.o obj-$(CONFIG_ARM_CHARLCD) += arm-charlcd.o +obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
drivers/misc/fsa9480.c new file +556 -0
556 lines not on this page · raw message
include/linux/fsa9480.h new file +40 -0
@@ -0,0 +1,40 @@ +/* +* Copyright (C) 2010 Samsung Electronics +* Minkyu Kang <[email protected]> +* Wonguk Jeong <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +* +*/ + +#ifndef _FSA9480_H_ +#define _FSA9480_H_ + +#define FSA9480_ATTACHED 1 +#define FSA9480_DETACHED 0 + +struct fsa9480_platform_data { +void (*cfg_gpio) (void ); +void (*usb_cb) (u8 attached); +void (*uart_cb) (u8 attached); +void (*charger_cb) (u8 attached); +void (*jig_cb) (u8 attached); +void (*reset_cb) (void ); +}; + +void fsa9480_set_switch (const char *buf); +ssize_t fsa9480_get_switch (char *buf); + +#endif /* _FSA9480_H_ */
Signature · 1 line
1.7.0.4
(catching up!)
Quoted 3 lines · On Tue, 31 Aug 2010 19:28:42 +0900 Minkyu Kang…
The FSA9480 is a USB port accessory detector and switch. This patch adds support the FSA9480 USB Switch.
What a strange device.
Is there a data sheet available?
drivers/misc/Kconfig config BMP085 · line 400
Quoted 8 lines
drivers/misc/Kconfig +8
-0+config USB_SWITCH_FSA9480 + tristate "FSA9480 USB Switch" + depends on I2C + help + The FSA9480 is a USB port accessory detector and switch. + The FSA9480 is fully controlled using I2C and enables USB data, + stereo and mono audio, video, microphone and UART data to use + a common connector port.
So if I'm understanding it correctly, it's an i2c-controlled device which turns USB devices on and off, multiplexing them into a single USB port? So if I switch from "USB data" over to "microphone", the USB subsystem will see the "USB data" device get unplugged and will see a "microphone" get plugged in?
Or something else. Am I even vaguely understanding this thing?
It would help if the changelog were to contain a paragraph describing the overall behaviour of this device.
Quoted 27 lines
...
drivers/misc/fsa9480.c new file +24
-0+void fsa9480_set_switch (const char *buf) +{ +struct fsa9480_usbsw *usbsw = chip; +struct i2c_client *client = usbsw->client; +unsigned int value; +unsigned int path =0 ; + + value =fsa9480_read_reg (client,FSA9480_REG_CTRL ); + +if (!strncmp (buf,"VAUDIO" ,6 )) { + path =SW_VAUDIO ; + value &= ~CON_MANUAL_SW ; + }else if (!strncmp (buf,"UART" ,4 )) { + path =SW_UART ; + value &= ~CON_MANUAL_SW ; + }else if (!strncmp (buf,"AUDIO" ,5 )) { + path =SW_AUDIO ; + value &= ~CON_MANUAL_SW ; + }else if (!strncmp (buf,"DHOST" ,5 )) { + path =SW_DHOST ; + value &= ~CON_MANUAL_SW ; + }else if (!strncmp (buf,"AUTO" ,4 )) { + path =SW_AUTO ; + value |=CON_MANUAL_SW ;
drivers/misc/fsa9480.c line 192
Quoted 10 lines
drivers/misc/fsa9480.c new file +10
-0+ }else { +printk (KERN_ERR "Wrong command\n" ); +return ; + } + + usbsw->mansw = path; +fsa9480_write_reg (client,FSA9480_REG_MANSW1 , path); +fsa9480_write_reg (client,FSA9480_REG_CTRL , value); +} +EXPORT_SYMBOL_GPL (fsa9480_set_switch);
Why was this exported?
Quoted 12 lines
drivers/misc/fsa9480.c new file +12
-0+ssize_t fsa9480_get_switch (char *buf) +{ +struct fsa9480_usbsw *usbsw = chip; +struct i2c_client *client = usbsw->client; +unsigned int value; + + value =fsa9480_read_reg (client,FSA9480_REG_MANSW1 ); + +if (value ==SW_VAUDIO ) +return sprintf (buf,"VAUDIO\n" ); +else if (value ==SW_UART ) +return sprintf (buf,"UART\n" );
drivers/misc/fsa9480.c line 215
Quoted 10 lines
drivers/misc/fsa9480.c new file +10
-0+else if (value ==SW_AUDIO ) +return sprintf (buf,"AUDIO\n" ); +else if (value ==SW_DHOST ) +return sprintf (buf,"DHOST\n" ); +else if (value ==SW_AUTO ) +return sprintf (buf,"AUTO\n" ); +else +return sprintf (buf,"%x" , value); +} +EXPORT_SYMBOL_GPL (fsa9480_get_switch);
This export also has no callers?
These functions implement a userspace API. Userspace APIs are important. But the patch provided no documentation for that API. Please always fully, exhaustively document userspace APIs! For they are the one part of the driver which we can never change.
Documenting them in a documentation file is OK. Also there's Documentation/ABI/. And they can be nicely described in the changelog too.
But providing us with no description at all makes review harder and less effective than we'd like it to be, and results in a driver which is harder for our users to use.
OK?
Quoted 19 lines
drivers/misc/fsa9480.c new file +19
-0+static ssize_t fsa9480_show_status (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct fsa9480_usbsw *usbsw =dev_get_drvdata (dev); +struct i2c_client *client = usbsw->client; +int devid, ctrl, adc, dev1, dev2, intr, + intmask1, intmask2, time1, time2, mansw1; + + devid =fsa9480_read_reg (client,FSA9480_REG_DEVID ); + ctrl =fsa9480_read_reg (client,FSA9480_REG_CTRL ); + adc =fsa9480_read_reg (client,FSA9480_REG_ADC ); + intmask1 =fsa9480_read_reg (client,FSA9480_REG_INT1_MASK ); + intmask2 =fsa9480_read_reg (client,FSA9480_REG_INT2_MASK ); + dev1 =fsa9480_read_reg (client,FSA9480_REG_DEV_T1 ); + dev2 =fsa9480_read_reg (client,FSA9480_REG_DEV_T2 ); + time1 =fsa9480_read_reg (client,FSA9480_REG_TIMING1 ); + time2 =fsa9480_read_reg (client,FSA9480_REG_TIMING2 ); + mansw1 =fsa9480_read_reg (client,FSA9480_REG_MANSW1 );
drivers/misc/fsa9480.c line 245
Quoted 10 lines
drivers/misc/fsa9480.c new file +10
-0+ +fsa9480_read_irq (client, &intr); + +return sprintf (buf,"Device ID(%02x), CTRL(%02x)\n" +"ADC(%02x), DEV_T1(%02x), DEV_T2(%02x)\n" +"INT(%04x), INTMASK(%02x, %02x)\n" +"TIMING(%02x, %02x), MANSW1(%02x)\n" , + devid, ctrl, adc, dev1, dev2, intr, + intmask1, intmask2, time1, time2, mansw1); +}
That's will produce odd-looking output I suspect. More conventional would be
Device ID:%02x CTRL:%02x
or something like that.
But that result is basically unparseable by software and a better output would be
Device_ID: %02x
CTRL: %02x
ADC: %02x
etc.
But even that violates the sysfs one-value-per-file guideline.
So this interface is problematic. It should have been discussed up-front in the changelog so we can all take a look at the proposal and have a think about it.
Quoted 4 lines
...
drivers/misc/fsa9480.c new file +1
-0+static irqreturn_t fsa9480_irq_handler (int irq,void *data)
drivers/misc/fsa9480.c line 287
Quoted 10 lines
drivers/misc/fsa9480.c new file +10
-0+{ +struct fsa9480_usbsw *usbsw = data; + +if (!work_pending (&usbsw->work)) { +disable_irq_nosync (irq); +schedule_work (&usbsw->work); + } + +return IRQ_HANDLED ; +}
I expect that this driver can be converted to the new threaded IRQ code (request_threaded_irq) and it will all become simpler.
Quoted 3 lines
...
Quoted 44 lines · Andrew Morton
Quoted 19 lines
drivers/misc/fsa9480.c new file +19
-0+static ssize_t fsa9480_show_status (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct fsa9480_usbsw *usbsw =dev_get_drvdata (dev); +struct i2c_client *client = usbsw->client; +int devid, ctrl, adc, dev1, dev2, intr, + intmask1, intmask2, time1, time2, mansw1; + + devid =fsa9480_read_reg (client,FSA9480_REG_DEVID ); + ctrl =fsa9480_read_reg (client,FSA9480_REG_CTRL ); + adc =fsa9480_read_reg (client,FSA9480_REG_ADC ); + intmask1 =fsa9480_read_reg (client,FSA9480_REG_INT1_MASK ); + intmask2 =fsa9480_read_reg (client,FSA9480_REG_INT2_MASK ); + dev1 =fsa9480_read_reg (client,FSA9480_REG_DEV_T1 ); + dev2 =fsa9480_read_reg (client,FSA9480_REG_DEV_T2 ); + time1 =fsa9480_read_reg (client,FSA9480_REG_TIMING1 ); + time2 =fsa9480_read_reg (client,FSA9480_REG_TIMING2 ); + mansw1 =fsa9480_read_reg (client,FSA9480_REG_MANSW1 );drivers/misc/fsa9480.c line 245
Quoted 10 lines
drivers/misc/fsa9480.c new file +10
-0+ +fsa9480_read_irq (client, &intr); + +return sprintf (buf,"Device ID(%02x), CTRL(%02x)\n" +"ADC(%02x), DEV_T1(%02x), DEV_T2(%02x)\n" +"INT(%04x), INTMASK(%02x, %02x)\n" +"TIMING(%02x, %02x), MANSW1(%02x)\n" , + devid, ctrl, adc, dev1, dev2, intr, + intmask1, intmask2, time1, time2, mansw1); +}That's will produce odd-looking output I suspect. More conventional would be
Device ID:%02x CTRL:%02xor something like that.
But that result is basically unparseable by software and a better output would be
Device_ID: %02x
CTRL: %02x
ADC: %02x
Quoted 7 lines
etc.
But even that violates the sysfs one-value-per-file guideline.
So this interface is problematic. It should have been discussed up-front in the changelog so we can all take a look at the proposal and have a think about it.
Yes, such a sysfs file is not allowed, please just have individual sysfs files for these attribute types (device id, control, adc, etc.)
And are you sure they are even needed? Some of those are already known through the i2c interface, right?
thanks,
greg k-h
Quoted 13 lines · Andrew Morton
(catching up!)
Quoted 3 lines · On Tue, 31 Aug 2010 19:28:42 +0900 Minkyu Kang…
The FSA9480 is a USB port accessory detector and switch. This patch adds support the FSA9480 USB Switch.
What a strange device.
Is there a data sheet available?
Quoted 14 lines
drivers/misc/Kconfig config BMP085 · line 400
Quoted 8 lines
drivers/misc/Kconfig +8
-0+config USB_SWITCH_FSA9480 + tristate "FSA9480 USB Switch" + depends on I2C + help + The FSA9480 is a USB port accessory detector and switch. + The FSA9480 is fully controlled using I2C and enables USB data, + stereo and mono audio, video, microphone and UART data to use + a common connector port.So if I'm understanding it correctly, it's an i2c-controlled device which turns USB devices on and off, multiplexing them into a single USB port? So if I switch from "USB data" over to "microphone", the USB subsystem will see the "USB data" device get unplugged and will see a "microphone" get plugged in?
It is generally for 'smart phones' where one connector has to do many jobs. Basically it's a electrical switch which allows one set of lines on a connector to be electrically connected to either the USB, or one of a number of analogue functions.
Often this is helped by the connector having some form of detect line which tells you what is plugged in, such as a headset, usb cable, etc.
Quoted 163 lines
Or something else. Am I even vaguely understanding this thing?
It would help if the changelog were to contain a paragraph describing the overall behaviour of this device.
Quoted 27 lines
...
drivers/misc/fsa9480.c new file +24
-0+void fsa9480_set_switch (const char *buf) +{ +struct fsa9480_usbsw *usbsw = chip; +struct i2c_client *client = usbsw->client; +unsigned int value; +unsigned int path =0 ; + + value =fsa9480_read_reg (client,FSA9480_REG_CTRL ); + +if (!strncmp (buf,"VAUDIO" ,6 )) { + path =SW_VAUDIO ; + value &= ~CON_MANUAL_SW ; + }else if (!strncmp (buf,"UART" ,4 )) { + path =SW_UART ; + value &= ~CON_MANUAL_SW ; + }else if (!strncmp (buf,"AUDIO" ,5 )) { + path =SW_AUDIO ; + value &= ~CON_MANUAL_SW ; + }else if (!strncmp (buf,"DHOST" ,5 )) { + path =SW_DHOST ; + value &= ~CON_MANUAL_SW ; + }else if (!strncmp (buf,"AUTO" ,4 )) { + path =SW_AUTO ; + value |=CON_MANUAL_SW ;drivers/misc/fsa9480.c line 192
Quoted 10 lines
drivers/misc/fsa9480.c new file +10
-0+ }else { +printk (KERN_ERR "Wrong command\n" ); +return ; + } + + usbsw->mansw = path; +fsa9480_write_reg (client,FSA9480_REG_MANSW1 , path); +fsa9480_write_reg (client,FSA9480_REG_CTRL , value); +} +EXPORT_SYMBOL_GPL (fsa9480_set_switch);Why was this exported?
Quoted 12 lines
drivers/misc/fsa9480.c new file +12
-0+ssize_t fsa9480_get_switch (char *buf) +{ +struct fsa9480_usbsw *usbsw = chip; +struct i2c_client *client = usbsw->client; +unsigned int value; + + value =fsa9480_read_reg (client,FSA9480_REG_MANSW1 ); + +if (value ==SW_VAUDIO ) +return sprintf (buf,"VAUDIO\n" ); +else if (value ==SW_UART ) +return sprintf (buf,"UART\n" );drivers/misc/fsa9480.c line 215
Quoted 10 lines
drivers/misc/fsa9480.c new file +10
-0+else if (value ==SW_AUDIO ) +return sprintf (buf,"AUDIO\n" ); +else if (value ==SW_DHOST ) +return sprintf (buf,"DHOST\n" ); +else if (value ==SW_AUTO ) +return sprintf (buf,"AUTO\n" ); +else +return sprintf (buf,"%x" , value); +} +EXPORT_SYMBOL_GPL (fsa9480_get_switch);This export also has no callers?
These functions implement a userspace API. Userspace APIs are important. But the patch provided no documentation for that API. Please always fully, exhaustively document userspace APIs! For they are the one part of the driver which we can never change.
Documenting them in a documentation file is OK. Also there's
Documentation/ABI/. And they can be nicely described in the changelog too.But providing us with no description at all makes review harder and less effective than we'd like it to be, and results in a driver which is harder for our users to use.
OK?
Quoted 19 lines
drivers/misc/fsa9480.c new file +19
-0+static ssize_t fsa9480_show_status (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct fsa9480_usbsw *usbsw =dev_get_drvdata (dev); +struct i2c_client *client = usbsw->client; +int devid, ctrl, adc, dev1, dev2, intr, + intmask1, intmask2, time1, time2, mansw1; + + devid =fsa9480_read_reg (client,FSA9480_REG_DEVID ); + ctrl =fsa9480_read_reg (client,FSA9480_REG_CTRL ); + adc =fsa9480_read_reg (client,FSA9480_REG_ADC ); + intmask1 =fsa9480_read_reg (client,FSA9480_REG_INT1_MASK ); + intmask2 =fsa9480_read_reg (client,FSA9480_REG_INT2_MASK ); + dev1 =fsa9480_read_reg (client,FSA9480_REG_DEV_T1 ); + dev2 =fsa9480_read_reg (client,FSA9480_REG_DEV_T2 ); + time1 =fsa9480_read_reg (client,FSA9480_REG_TIMING1 ); + time2 =fsa9480_read_reg (client,FSA9480_REG_TIMING2 ); + mansw1 =fsa9480_read_reg (client,FSA9480_REG_MANSW1 );drivers/misc/fsa9480.c line 245
Quoted 10 lines
drivers/misc/fsa9480.c new file +10
-0+ +fsa9480_read_irq (client, &intr); + +return sprintf (buf,"Device ID(%02x), CTRL(%02x)\n" +"ADC(%02x), DEV_T1(%02x), DEV_T2(%02x)\n" +"INT(%04x), INTMASK(%02x, %02x)\n" +"TIMING(%02x, %02x), MANSW1(%02x)\n" , + devid, ctrl, adc, dev1, dev2, intr, + intmask1, intmask2, time1, time2, mansw1); +}That's will produce odd-looking output I suspect. More conventional would be
Device ID:%02x CTRL:%02xor something like that.
But that result is basically unparseable by software and a better output would be
Device_ID: %02x
CTRL: %02x
ADC: %02xetc.
But even that violates the sysfs one-value-per-file guideline.
So this interface is problematic. It should have been discussed up-front in the changelog so we can all take a look at the proposal and have a think about it.
Quoted 4 lines
...
drivers/misc/fsa9480.c new file +1
-0+static irqreturn_t fsa9480_irq_handler (int irq,void *data)drivers/misc/fsa9480.c line 287
Quoted 10 lines
drivers/misc/fsa9480.c new file +10
-0+{ +struct fsa9480_usbsw *usbsw = data; + +if (!work_pending (&usbsw->work)) { +disable_irq_nosync (irq); +schedule_work (&usbsw->work); + } + +return IRQ_HANDLED ; +}I expect that this driver can be converted to the new threaded IRQ code (
request_threaded_irq) and it will all become simpler.Quoted 3 lines
...
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 5 lines
-- Ben Q: What's a light-year? A: One-third less calories than a regular year.
Quoted 11 lines · Andrew Morton
(catching up!)
Quoted 3 lines · On Tue, 31 Aug 2010 19:28:42 +0900 Minkyu Kang <[email protected]> wrote:
The FSA9480 is a USB port accessory detector and switch. This patch adds support the FSA9480 USB Switch.
What a strange device.
As Minkyu Kang is on holiday until 9/27. I reply it instead. he will give more information when returned.
I just explain the how to work and what's the purpose to export functions.
Quoted 1 line
Is there a data sheet available?
It's difficult to find it at web, I'll check it but maybe difficult.
Quoted 10 lines
Quoted 9 lines
+config
USB_SWITCH_FSA9480
+ tristate "FSA9480 USB Switch"
+ depends on I2C
+ help
+ The FSA9480 is a USB port accessory detector and switch.
+ The FSA9480 is fully controlled using I2C and enables USB data,
+ stereo and mono audio, video, microphone and UART data to use
+ a common connector port.
Quoted 5 lines
So if I'm understanding it correctly, it's an i2c-controlled device which turns USB devices on and off, multiplexing them into a single USB port? So if I switch from "USB data" over to "microphone", the USB subsystem will see the "USB data" device get unplugged and will see a "microphone" get plugged in?
When connect the usb cable, then read USB ID(?) or something and switch the mux automatically.
e.g, when you connect the car kit then it switch the path to car kit. Can't turn off the USB devices. only switch the USB path.
Quoted 35 lines
Or something else. Am I even vaguely understanding this thing?
It would help if the changelog were to contain a paragraph describing the overall behaviour of this device.
Quoted 29 lines
...
drivers/misc/fsa9480.c new file +26
-0+void fsa9480_set_switch (const char *buf) +{ + struct fsa9480_usbsw *usbsw = chip; + struct i2c_client *client = usbsw->client; + unsigned int value; + unsigned int path = 0; + + value = fsa9480_read_reg(client, FSA9480_REG_CTRL); + + if (!strncmp(buf, "VAUDIO", 6)) { + path = SW_VAUDIO; + value &= ~CON_MANUAL_SW; + } else if (!strncmp(buf, "UART", 4)) { + path = SW_UART; + value &= ~CON_MANUAL_SW; + } else if (!strncmp(buf, "AUDIO", 5)) { + path = SW_AUDIO; + value &= ~CON_MANUAL_SW; + } else if (!strncmp(buf, "DHOST", 5)) { + path = SW_DHOST; + value &= ~CON_MANUAL_SW; + } else if (!strncmp(buf, "AUTO", 4)) { + path = SW_AUTO; + value |= CON_MANUAL_SW; + } else { + printk(KERN_ERR "Wrong command\n");
Quoted 8 lines
drivers/misc/fsa9480.c line 192
Quoted 8 lines
+ return;
+ }drivers/misc/fsa9480.c new file +6
-0+ + usbsw->mansw = path; + fsa9480_write_reg(client, FSA9480_REG_MANSW1, path); + fsa9480_write_reg(client, FSA9480_REG_CTRL, value); +} +EXPORT_SYMBOL_GPL (fsa9480_set_switch);
To Minkyu, It's should be EXPORT_SYMBOL???
Quoted 2 lines
Why was this exported?
Even though it switch the USB path automatically, we don't use the full path for each purpose.
e.g., we don't use the V(ideo)Audio or Audio. it's connected it for other purpose.
In this case we switch it by manually by MAN(ual) Switch register.
Quoted 17 lines
Quoted 17 lines
drivers/misc/fsa9480.c new file +16
-0+ssize_t fsa9480_get_switch (char *buf) +{ + struct fsa9480_usbsw *usbsw = chip; + struct i2c_client *client = usbsw->client; + unsigned int value; + + value = fsa9480_read_reg(client, FSA9480_REG_MANSW1); + + if (value == SW_VAUDIO) + return sprintf(buf, "VAUDIO\n"); + else if (value == SW_UART) + return sprintf(buf, "UART\n"); + else if (value == SW_AUDIO) + return sprintf(buf, "AUDIO\n"); + else if (value == SW_DHOST) + return sprintf(buf, "DHOST\n");
Quoted 8 lines
Review
Quoted 6 lines
+ else if (value ==
SW_AUTO)
+ returnsprintf(buf, "AUTO\n");
+ else
+ returnsprintf(buf, "%x", value);(quoted diff) +2
-0+} +EXPORT_SYMBOL_GPL(fsa9480_get_switch);This export also has no callers?
It's used check what's the current switch status at other drivers.
Thank you,
Kyungmin Park
Quoted 96 lines
These functions implement a userspace API. Userspace APIs are important. But the patch provided no documentation for that API. Please always fully, exhaustively document userspace APIs! For they are the one part of the driver which we can never change.
Documenting them in a documentation file is OK. Also there's
Documentation/ABI/. And they can be nicely described in the changelog too.But providing us with no description at all makes review harder and less effective than we'd like it to be, and results in a driver which is harder for our users to use.
OK?
Quoted 19 lines
drivers/misc/fsa9480.c new file +19
-0+static ssize_t fsa9480_show_status (struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct fsa9480_usbsw *usbsw = dev_get_drvdata(dev); + struct i2c_client *client = usbsw->client; + int devid, ctrl, adc, dev1, dev2, intr, + intmask1, intmask2, time1, time2, mansw1; + + devid = fsa9480_read_reg(client, FSA9480_REG_DEVID); + ctrl = fsa9480_read_reg(client, FSA9480_REG_CTRL); + adc = fsa9480_read_reg(client, FSA9480_REG_ADC); + intmask1 = fsa9480_read_reg(client, FSA9480_REG_INT1_MASK); + intmask2 = fsa9480_read_reg(client, FSA9480_REG_INT2_MASK); + dev1 = fsa9480_read_reg(client, FSA9480_REG_DEV_T1); + dev2 = fsa9480_read_reg(client, FSA9480_REG_DEV_T2); + time1 = fsa9480_read_reg(client, FSA9480_REG_TIMING1); + time2 = fsa9480_read_reg(client, FSA9480_REG_TIMING2); + mansw1 = fsa9480_read_reg(client, FSA9480_REG_MANSW1);drivers/misc/fsa9480.c line 245
Quoted 10 lines
drivers/misc/fsa9480.c new file +10
-0+ + fsa9480_read_irq(client, &intr); + + return sprintf(buf, "Device ID(%02x), CTRL(%02x)\n" + "ADC(%02x), DEV_T1(%02x), DEV_T2(%02x)\n" + "INT(%04x), INTMASK(%02x, %02x)\n" + "TIMING(%02x, %02x), MANSW1(%02x)\n", + devid, ctrl, adc, dev1, dev2, intr, + intmask1, intmask2, time1, time2, mansw1); +}That's will produce odd-looking output I suspect. More conventional would be
Device ID:%02x CTRL:%02x
or something like that.
But that result is basically unparseable by software and a better output would be
Device_ID: %02x
CTRL: %02x
ADC: %02xetc.
But even that violates the sysfs one-value-per-file guideline.
So this interface is problematic. It should have been discussed up-front in the changelog so we can all take a look at the proposal and have a think about it.
Quoted 4 lines
...
drivers/misc/fsa9480.c new file +1
-0+static irqreturn_t fsa9480_irq_handler (int irq,void *data)drivers/misc/fsa9480.c line 287
Quoted 10 lines
drivers/misc/fsa9480.c new file +10
-0+{ + struct fsa9480_usbsw *usbsw = data; + + if (!work_pending(&usbsw->work)) { + disable_irq_nosync(irq); + schedule_work(&usbsw->work); + } + + return IRQ_HANDLED; +}I expect that this driver can be converted to the new threaded IRQ code (
request_threaded_irq) and it will all become simpler.Quoted 3 lines
...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Dear Andrew Morton,
Quoted 5 lines · Andrew Morton
(catching up!)
On Tue, 31 Aug 2010 19:28:42 +0900
Minkyu Kang <[email protected]> wrote:
Quoted 7 lines
Quoted 3 lines
The FSA9480 is a USB port accessory detector and switch. This patch adds support the FSA9480 USB Switch.
What a strange device.
Is there a data sheet available?
Hello :)
I couldn't find the fsa9480 data sheet on the web. But I found the fsa9280 data sheet instead. It looks very similar.
http://www.fairchildsemi.com/ds/FS/FSA9280A.pdf
Quoted 52 lines
Quoted 9 lines
+config
USB_SWITCH_FSA9480
+ tristate "FSA9480 USB Switch"
+ depends on I2C
+ help
+ The FSA9480 is a USB port accessory detector and switch.
+ The FSA9480 is fully controlled using I2C and enables USB data,
+ stereo and mono audio, video, microphone and UART data to use
+ a common connector port.So if I'm understanding it correctly, it's an i2c-controlled device which turns USB devices on and off, multiplexing them into a single USB port? So if I switch from "USB data" over to "microphone", the USB subsystem will see the "USB data" device get unplugged and will see a "microphone" get plugged in?
Or something else. Am I even vaguely understanding this thing?
It would help if the changelog were to contain a paragraph describing the overall behaviour of this device.
Quoted 31 lines
...
drivers/misc/fsa9480.c new file +28
-0+void fsa9480_set_switch (const char *buf) +{ + struct fsa9480_usbsw *usbsw = chip; + struct i2c_client *client = usbsw->client; + unsigned int value; + unsigned int path = 0; + + value = fsa9480_read_reg(client, FSA9480_REG_CTRL); + + if (!strncmp(buf, "VAUDIO", 6)) { + path = SW_VAUDIO; + value &= ~CON_MANUAL_SW; + } else if (!strncmp(buf, "UART", 4)) { + path = SW_UART; + value &= ~CON_MANUAL_SW; + } else if (!strncmp(buf, "AUDIO", 5)) { + path = SW_AUDIO; + value &= ~CON_MANUAL_SW; + } else if (!strncmp(buf, "DHOST", 5)) { + path = SW_DHOST; + value &= ~CON_MANUAL_SW; + } else if (!strncmp(buf, "AUTO", 4)) { + path = SW_AUTO; + value |= CON_MANUAL_SW; + } else { + printk(KERN_ERR "Wrong command\n"); + return; + }
Quoted 8 lines
drivers/misc/fsa9480.c line 192
Quoted 6 lines
drivers/misc/fsa9480.c new file +6
-0+ + usbsw->mansw = path; + fsa9480_write_reg(client, FSA9480_REG_MANSW1, path); + fsa9480_write_reg(client, FSA9480_REG_CTRL, value); +} +EXPORT_SYMBOL_GPL (fsa9480_set_switch);Why was this exported?
Kyungmin already explained about it.
Quoted 17 lines
Quoted 17 lines
drivers/misc/fsa9480.c new file +16
-0+ssize_t fsa9480_get_switch (char *buf) +{ + struct fsa9480_usbsw *usbsw = chip; + struct i2c_client *client = usbsw->client; + unsigned int value; + + value = fsa9480_read_reg(client, FSA9480_REG_MANSW1); + + if (value == SW_VAUDIO) + return sprintf(buf, "VAUDIO\n"); + else if (value == SW_UART) + return sprintf(buf, "UART\n"); + else if (value == SW_AUDIO) + return sprintf(buf, "AUDIO\n"); + else if (value == SW_DHOST) + return sprintf(buf, "DHOST\n");
Quoted 8 lines
Review
Quoted 6 lines
+ else if (value ==
SW_AUTO)
+ returnsprintf(buf, "AUTO\n");
+ else
+ returnsprintf(buf, "%x", value);(quoted diff) +2
-0+} +EXPORT_SYMBOL_GPL(fsa9480_get_switch);This export also has no callers?
It's for other drivers.
For example.. When USB cable is connected, PMIC driver enable the power for each device.
But if use the manual switch, PMIC driver can't know what device is enabled. So, this function was exported.
Quoted 60 lines
These functions implement a userspace API. Userspace APIs are important. But the patch provided no documentation for that API. Please always fully, exhaustively document userspace APIs! For they are the one part of the driver which we can never change.
Documenting them in a documentation file is OK. Also there's
Documentation/ABI/. And they can be nicely described in the changelog too.But providing us with no description at all makes review harder and less effective than we'd like it to be, and results in a driver which is harder for our users to use.
OK?
Quoted 19 lines
drivers/misc/fsa9480.c new file +19
-0+static ssize_t fsa9480_show_status (struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct fsa9480_usbsw *usbsw = dev_get_drvdata(dev); + struct i2c_client *client = usbsw->client; + int devid, ctrl, adc, dev1, dev2, intr, + intmask1, intmask2, time1, time2, mansw1; + + devid = fsa9480_read_reg(client, FSA9480_REG_DEVID); + ctrl = fsa9480_read_reg(client, FSA9480_REG_CTRL); + adc = fsa9480_read_reg(client, FSA9480_REG_ADC); + intmask1 = fsa9480_read_reg(client, FSA9480_REG_INT1_MASK); + intmask2 = fsa9480_read_reg(client, FSA9480_REG_INT2_MASK); + dev1 = fsa9480_read_reg(client, FSA9480_REG_DEV_T1); + dev2 = fsa9480_read_reg(client, FSA9480_REG_DEV_T2); + time1 = fsa9480_read_reg(client, FSA9480_REG_TIMING1); + time2 = fsa9480_read_reg(client, FSA9480_REG_TIMING2); + mansw1 = fsa9480_read_reg(client, FSA9480_REG_MANSW1);drivers/misc/fsa9480.c line 245
Quoted 10 lines
drivers/misc/fsa9480.c new file +10
-0+ + fsa9480_read_irq(client, &intr); + + return sprintf(buf, "Device ID(%02x), CTRL(%02x)\n" + "ADC(%02x), DEV_T1(%02x), DEV_T2(%02x)\n" + "INT(%04x), INTMASK(%02x, %02x)\n" + "TIMING(%02x, %02x), MANSW1(%02x)\n", + devid, ctrl, adc, dev1, dev2, intr, + intmask1, intmask2, time1, time2, mansw1); +}That's will produce odd-looking output I suspect. More conventional would be
Device ID:%02x CTRL:%02x
or something like that.
But that result is basically unparseable by software and a better output would be
Device_ID: %02x
CTRL: %02x
ADC: %02x
Quoted 7 lines
etc.
But even that violates the sysfs one-value-per-file guideline.
So this interface is problematic. It should have been discussed up-front in the changelog so we can all take a look at the proposal and have a think about it.
Yes. It's for debugging.
I'll remove some features and separate this sysfs file.
Quoted 4 lines
Quoted 4 lines
...
Quoted 14 lines
drivers/misc/fsa9480.c line 287
Quoted 11 lines
drivers/misc/fsa9480.c new file +11
-0+static irqreturn_t fsa9480_irq_handler (int irq,void *data) +{ + struct fsa9480_usbsw *usbsw = data; + + if (!work_pending(&usbsw->work)) { + disable_irq_nosync(irq); + schedule_work(&usbsw->work); + } + + return IRQ_HANDLED; +}I expect that this driver can be converted to the new threaded IRQ code (
request_threaded_irq) and it will all become simpler.
Ok, I'll convert to new IRQ code.
Quoted 10 lines
Quoted 4 lines
...
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks
Minkyu Kang.
Signature · 2 lines
from. prom. www.promsoft.net
PATCH v2 1/1 misc: adds support the FSA9480 USB Switch
The FSA9480 is a USB port accessory detector and switch. This patch adds support the FSA9480 USB Switch.
---
v2:
use the threaded irq
fix the sysfs
drivers/misc/Kconfig | 9 + drivers/misc/Makefile | 1 + drivers/misc/fsa9480.c | 544 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/fsa9480.h | 40 ++++ 4 files changed, 594 insertions(+), 0 deletions(-) create mode 100644 drivers/misc/fsa9480.c create mode 100644 include/linux/fsa9480.h
drivers/misc/Kconfig +9 -0
@@ -390,6 +390,15 @@ config BMP085 To compile this driver as a module, choose M here: the module will be called bmp085. +config USB_SWITCH_FSA9480 + tristate "FSA9480 USB Switch" + depends on I2C + help + The FSA9480 is a USB port accessory detector and switch. + The FSA9480 is fully controlled using I2C and enables USB data, + stereo and mono audio, video, microphone and UART data to use + a common connector port. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig"
drivers/misc/Makefile +1 -0
@@ -35,3 +35,4 @@ obj-y += eeprom/ obj-y += cb710/ obj-$(CONFIG_VMWARE_BALLOON) += vmw_balloon.o obj-$(CONFIG_ARM_CHARLCD) += arm-charlcd.o +obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
drivers/misc/fsa9480.c new file +544 -0
544 lines not on this page · raw message
include/linux/fsa9480.h new file +40 -0
@@ -0,0 +1,40 @@ +/* +* Copyright (C) 2010 Samsung Electronics +* Minkyu Kang <[email protected]> +* Wonguk Jeong <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +* +*/ + +#ifndef _FSA9480_H_ +#define _FSA9480_H_ + +#define FSA9480_ATTACHED 1 +#define FSA9480_DETACHED 0 + +struct fsa9480_platform_data { +void (*cfg_gpio) (void ); +void (*usb_cb) (u8 attached); +void (*uart_cb) (u8 attached); +void (*charger_cb) (u8 attached); +void (*jig_cb) (u8 attached); +void (*reset_cb) (void ); +}; + +void fsa9480_set_switch (const char *buf); +ssize_t fsa9480_get_switch (char *buf); + +#endif /* _FSA9480_H_ */
Signature · 1 line
1.7.0.4
Quoted 10 lines · Minkyu Kang
The FSA9480 is a USB port accessory detector and switch. This patch adds support the FSA9480 USB Switch.
Signed-off-by Minkyu Kang <[email protected]>Signed-off-by Kyungmin Park <[email protected]>---
v2:
use the threaded irq
fix the sysfs
Quoted 4 lines
drivers/misc/Kconfig | 9 + drivers/misc/Makefile | 1 + drivers/misc/fsa9480.c | 544 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/fsa9480.h | 40 ++++
Why do you need a .h file here for a single driver?
You are creating new sysfs files, so you are required to add a new Documentation/ABI file as well, please add that to this patch.
And why are you exporting symbols from this driver, that should not be needed at all.
Quoted 11 lines
+/*
+ *fsa9480.c- FSA9480 micro USB switch device driver
+ *
+ * Copyright (C) 2010 Samsung Electronics
+ * Minkyu Kang <[email protected]>
+ * Wonguk Jeong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
Are you positive about that "any later version" portion of this?
thanks,
greg k-h
Dear Greg KH,
Quoted 11 lines · Greg KH
Quoted 10 lines · Minkyu Kang
The FSA9480 is a USB port accessory detector and switch. This patch adds support the FSA9480 USB Switch.
Signed-off-by Minkyu Kang <[email protected]>Signed-off-by Kyungmin Park <[email protected]>---
v2:
use the threaded irq
fix the sysfs
Quoted 6 lines
Quoted 4 lines
drivers/misc/Kconfig | 9 +
drivers/misc/Makefile | 1 +
drivers/misc/fsa9480.c | 544 +++++++++++++++++++++++++++++++++++++++++++++++include/linux/fsa9480.h| 40 ++++Why do you need a .h file here for a single driver?
Because of platform data.
Quoted 3 lines
You are creating new sysfs files, so you are required to add a new
Documentation/ABIfile as well, please add that to this patch.
ok.
Quoted 3 lines
And why are you exporting symbols from this driver, that should not be needed at all.
It's for other drivers.
For example.. When USB cable is connected, PMIC driver enable the power for each device. (USB, uart, audio and so on..)
But if use the manual switch, PMIC driver can't know what device is enabled. So, this function was exported.
Quoted 14 lines
Quoted 13 lines
+/*
+ *fsa9480.c- FSA9480 micro USB switch device driver
+ *
+ * Copyright (C) 2010 Samsung Electronics
+ * Minkyu Kang <[email protected]>
+ * Wonguk Jeong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
Quoted 1 line
Are you positive about that "any later version" portion of this?
I will update the licence.
Quoted 10 lines
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Thanks
Minkyu Kang
Signature · 2 lines
from. prom. www.promsoft.net
Quoted 10 lines · Minkyu Kang
Dear Greg KH,
Quoted 2 lines
And why are you exporting symbols from this driver, that should not be needed at all.
It's for other drivers.
For example.. When USB cable is connected, PMIC driver enable the power for each device. (USB, uart, audio and so on..)
But if use the manual switch, PMIC driver can't know what device is enabled. So, this function was exported.
This seems like it would require the PMIC driver to have knowledge of the USB switch driver which would be likely to give a system which is rather board specific. For PMIC supply management integration the regulator API would be more normal.
Quoted 14 lines · Minkyu Kang
Dear Greg KH,
Quoted 11 lines · Greg KH
Quoted 10 lines · Minkyu Kang
The FSA9480 is a USB port accessory detector and switch. This patch adds support the FSA9480 USB Switch.
Signed-off-by Minkyu Kang <[email protected]>Signed-off-by Kyungmin Park <[email protected]>---
v2:
?use the threaded irq
?fix the sysfs
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
?drivers/misc/Kconfig ? ?| ? ?9 + ?drivers/misc/Makefile ? | ? ?1 + ?drivers/misc/fsa9480.c ?| ?544 +++++++++++++++++++++++++++++++++++++++++++++++ ?include/linux/fsa9480.h | ? 40 ++++Why do you need a .h file here for a single driver?
Because of platform data.
Then put it in include/linux/platform_data/ instead please.
Quoted 6 lines
Quoted 2 lines
You are creating new sysfs files, so you are required to add a new
Documentation/ABIfile as well, please add that to this patch.ok.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
And why are you exporting symbols from this driver, that should not be needed at all.
It's for other drivers.
For example.. When USB cable is connected, PMIC driver enable the power for each device. (USB, uart, audio and so on..)
But if use the manual switch, PMIC driver can't know what device is enabled. So, this function was exported.
What drivers are using these functions? Where is the code for them? When will they be submitted for inclusion?
thanks,
greg k-h