| 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
| ||
|
Date: Sat, 18 Sep 2010 02:37:54 +0100
From: Ben Dooks <ben-i2c@...ff.org>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: Minkyu Kang <mk7.kang@...sung.com>, linux-kernel@...r.kernel.org,
linux-i2c@...r.kernel.org, kyungmin.park@...sung.com,
Greg KH <greg@...ah.com>
Subject: Re: [PATCH] misc: adds support the FSA9480 USB Switch
On Fri, Sep 17, 2010 at 03:07:31PM -0700, Andrew Morton wrote:
> (catching up!)
>
> On Tue, 31 Aug 2010 19:28:42 +0900
> Minkyu Kang <mk7.kang@...sung.com> wrote:
>
> > 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?
>
> > +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.
> 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.
>
> >
> > ...
> >
> > +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;
> > + }
> > +
> > + 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?
>
> > +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");
> > + 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?
>
> > +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);
> > +
> > + 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.
>
> >
> > ...
> >
> > +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.
>
> >
> > ...
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists