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

NeilBrown neilb at suse.de
Thu Mar 29 00:28:05 CEST 2012


On Wed, 28 Mar 2012 21:42:32 +0200 Andreas Kemnade <andreas at kemnade.info>
wrote:

> 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).

Yes.
I would probably add a new kernel parameter default_usb_current or similar,
and arrange that allow_usb set that to 600 for  backwards compatibility.


> 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.

:-)  I did a similar thing.  I think there will always be a need for
use-space over-ride but it would be good if we can make the kernel as clever
as possible.

I would generally prefer to set the default in user-space too.. I.e. have
udev rule that triggers whenever a charger is plugged in, and sets the
current to e.g. 500mA if it is less than that.


> 
> > 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. 

Seeing you were so successful at reading the BCI part, maybe you *are* ready
to graduate the the USB section :-)


> 
> 
> > > +/*
> > > + * 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.

Is it?  In what way?

And by "wrong" do you mean "different from the TRM" or "different from what
you measured with a very precise ammeter"??
Because if they came from TI, then maybe they are actually correct.

> 
> > > @@ -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.

Thanks,
NeilBrown

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 828 bytes
Desc: not available
URL: <http://lists.goldelico.com/pipermail/gta04-owner/attachments/20120329/c91a812c/attachment.bin>


More information about the Gta04-owner mailing list