[Gta04-owner] [PATCH 3.2] twl4030_charger: set usb max current through sysfs
NeilBrown
neilb at suse.de
Mon Mar 26 23:59:52 CEST 2012
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.
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'm happy to take the patch in its current form, but we should be upfront
about the fact that the semantics will probably change).
>
> Signed-off-by: Andreas Kemnade <andreas at kemnade.info>
> ---
> drivers/power/twl4030_charger.c | 95 +++++++++++++++++++++++++++++++++++++++
> 1 files changed, 95 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
> index c6dee6d..1c26387 100644
> --- a/drivers/power/twl4030_charger.c
> +++ b/drivers/power/twl4030_charger.c
> @@ -30,6 +30,11 @@
> #define TWL4030_BCIMFSTS4 0x10
> #define TWL4030_BCICTL1 0x23
> #define TWL4030_BB_CFG 0x12
> +#define TWL4030_BCIIREF1 0x27
> +#define TWL4030_BCIIREF2 0x28
> +#define TWL4030_BCIMFKEY 0x11
probably should have TABs in here rather than spaces.
> +
> +
>
> #define TWL4030_BCIAUTOWEN BIT(5)
> #define TWL4030_CONFIG_DONE BIT(4)
> @@ -353,6 +358,91 @@ static int twl4030_bci_usb_ncb(struct notifier_block *nb, unsigned long val,
> return NOTIFY_OK;
> }
>
> +
> +#ifdef CONFIG_SYSFS
Having #ifdef inside .c files is discouraged - it hurts readability.
In this case they should be unnecessary. The code will still compile fine
without CONFIG_SYSFS.
> +/*
> + * 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' ??
> + status = twl4030_bci_read(TWL4030_BCICTL1, &bcictl1);
> + if (status < 0)
> + return status;
> + if (bcictl1 & TWL4030_CGAIN)
> + cur *= 2;
> + if (status == 0)
> + return scnprintf(buf, PAGE_SIZE, "%u\n", cur);
> + return status;
> +}
> +
> +static int twl4030_charger_set_max_current(int cur)
> +{
> + u8 bcictl1;
> + int status;
> + /* get setting of CGAIN bit */
> + status = twl4030_bci_read(TWL4030_BCICTL1, &bcictl1);
> + if (status < 0)
> + return status;
> + if (bcictl1 & TWL4030_CGAIN)
> + cur /= 2;
> + cur = cur / 1665;
> + if (cur > 511)
> + return -EINVAL;
> + if (cur < 0)
> + return -EINVAL;
I think this should be
if (cur < 256)
should it not? The TRM said that the high bit is forced to '1'.
Also, I wonder if we should consider setting the CGAIN flag for particularly
high currents. Do you know if it is used for anything else?
> + /* disable write protection for one write access for BCIIREF */
> + status = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE, 0xE7,
> + TWL4030_BCIMFKEY);
> + if (status < 0)
> + return status;
> + status = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
> + (cur & 0x100) ? 3 : 2, TWL4030_BCIIREF2);
> + if (status < 0)
> + return status;
> + /* disable write protection for one write access for BCIIREF */
> + status = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE, 0xE7,
> + TWL4030_BCIMFKEY);
> + if (status < 0)
> + return status;
> + status = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE, cur & 0xff,
> + TWL4030_BCIIREF1);
> + return status;
> +}
> +
> +/*
> + * sysfs max_current store
> + */
> +static ssize_t
> +twl4030_bci_max_current_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t n)
> +{
> + unsigned int cur = 0;
> + int status = 0;
> + if (1 != sscanf(buf, "%u", &cur))
> + return -EINVAL;
'kstrtouint' is currently the preferred way to do this.
status = kstrtoint(buf, 10, &cur);
if (status)
return status;
This ensures consistent error codes.
> + status = twl4030_charger_set_max_current(cur);
> + return (status == 0) ? n : status;
> +}
> +
> +static DEVICE_ATTR(max_current, 0644, twl4030_bci_max_current_show,
> + twl4030_bci_max_current_store);
> +#endif /* CONFIG_SYSFS */
> +
> /*
> * TI provided formulas:
> * CGAIN == 0: ICHG = (BCIICHG * 1.7) / (2^10 - 1) - 0.85
> @@ -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" ??
>
> twl4030_charger_enable_ac(true);
> twl4030_charger_enable_usb(bci, true);
Thanks a lot,
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/20120327/57a70897/attachment-0001.bin>
More information about the Gta04-owner
mailing list