Re: [PATCH] misc: adds support the FSA9480 USB Switch

[Greg KH <greg@kroah.com>]

On Fri, Sep 17, 2010 at 03:07:31PM -0700, Andrew Morton wrote:
> > +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.

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