[Gta04-owner] [PATCH 3.2] twl4030_charger: set usb max current through sysfs

[Andreas Kemnade]

Hi,

On Tue, 2012-03-27 at 08:59 +1100, NeilBrown wrote:
> Hi,
>  thanks so much for reformatting your patch like this.  It helps in a number
> of ways, including making it easier to reply to it detail.
> I see you have also run it through 'checkpatch.pl' which is great.
> 
> 
> On Mon, 26 Mar 2012 23:00:15 +0200 Andreas Kemnade <andreas at kemnade.info>
> wrote:
> 
> > It provides a sysfs file here:
> > /sys/bus/platform/devices/twl4030_bci/max_current
> > /sys/class/power_supply/twl4030_usb/device/max_current
> > (symlinked stuff)
> > 
> > Into this sysfs file the maximum usb current in uA can be written or read.
> > The setting stays the same when the usb plug is disconnected and
> > reconnected.
> 
> This is obviously a good interface for development and testing, but I think
> we need to also think about a good interface to present for upstream
> inclusion.
> 
> I think that every time you plug in a cable it should be treated as a new
> power source and assessed on its merits.
> 
> So the max current should be set to '100mA' (is that right for USB?) unless
> there is reason to believe more can be provided.

I guess the kernel parameter allow_usb should be modified so that it
does not accept a bool but an int specifing the default current on new
connections. The default should be that 100mA (if the commandline
parameter is not given).
On the other hand setting that to a higher value (e.g.) reduces chances
that the GTA04 cannot be charged. I have no devices that would be
damaged by such currents. Replacing the 100mA default charge rate
with 500mA was one of the first things I did with the GTA02 kernel
just because I did not know how to access the id pin mechanically and I
did not know sources for mini-b plugs.

> The kernel might make some assessment and possibly increase the max current.
> Also user-space must have the option of increasing (or deceasing) the max
> current.

> So I think that we should probably define that the max_current field gets
> reset every time a device is plugged in.
> We also need to look at ways to find out the resistance-to-ground of the ID
> pin, and whether the D+ and D- pins are shorted.  If those details could be
> exported to user-space, that would be great.
> 
I already stumbled upon that resistance-to-ground in the datasheet.
So making a patch for that should not be too hard. 
Based on that information, userspace could decide and/or present
a menu. And anyway that ID stuff is not in the bci interfaces
and you suggested me to "find something easier to play with" after I
have read the bci part of the manual multiple times. 


> > +/*
> > + * sysfs max_current show
> > + */
> > +static ssize_t twl4030_bci_max_current_show(struct device *dev,
> > +	struct device_attribute *attr, char *buf)
> > +{
> > +	int status = 0;
> > +	unsigned int cur;
> > +	u8 c1, c2;
> > +	u8 bcictl1;
> > +
> > +	status = twl4030_bci_read(TWL4030_BCIIREF1, &c1);
> > +	if (status < 0)
> > +		return status;
> > +	status = twl4030_bci_read(TWL4030_BCIIREF2, &c2);
> > +	if (status < 0)
> > +		return status;
> > +	cur = (c2 & 1) ? 256 : 0;
> > +	cur += c1;
> > +	cur *= 1665;
> 
> It would be good to have a comment explaining this magic number.
> Maybe just a reference to where in the TRM it is mentioned would do, but if
> there is more that can be said, it wouldn't hurt.
> 
> I note there is a comment in the file "TI provided formulas...".
> Maybe we should be using the same formula?  Maybe we should put them
> in a 'static inline' ??
> 

Yes, that is the same formula as in twl4030_charger_get_current() but
the second pair of formulas in the comment is wrong.

> > @@ -567,6 +657,11 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
> >  			       TWL4030_INTERRUPTS_BCIIMR2A);
> >  	if (ret < 0)
> >  		dev_warn(&pdev->dev, "failed to unmask interrupts: %d\n", ret);
> > +#ifdef CONFIG_SYSFS
> > +	if (device_create_file(&pdev->dev, &dev_attr_max_current))
> > +		dev_warn(&pdev->dev, "could not create sysfs file\n");
> > +#endif
> > +
> 
> Do we need a matching "device_remove_file" in "twl4030_bci_remove" ??
> 
probably yes...

Greetings
Andreas Kemnade
PS: for my original problem, clearing the VBUSUNPLGEN/BCIMFSTS4 bit
helps. But of course that has side effects.