Re: [PATCH v3] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Grazvydas Ignotas]

hello,

On Mon, Sep 27, 2010 at 9:18 AM, Felipe Balbi <balbi@xxxxxx> wrote:
> On Sun, Sep 26, 2010 at 02:35:40PM -0500, Grazvydas Ignotas wrote:
>>
>> TWL4030/TPS65950 is a multi-function device with integrated charger,
>> which allows charging from AC or USB. This driver enables the
>> charger and provides several monitoring functions.
>>
>> Tested on OMAP3 Pandora board.
>>
>> Signed-off-by: Grazvydas Ignotas <notasas@xxxxxxxxx>
>> ---
>> This is v3 of BCI charger driver I first sent nearly a year ago [1].
>> I've updated it to use the new OTG notifiers for VBUS notifications,
>> however it still is not able to determine charge current to use.
>> For that reason there is now a temporary module param to enable USB
>> charging, until I can figure out how to get that info from the gadget
>> stack (hopefully Felipe can help me here).
>
> You need to send USB_EVENT_ENUMERATED from usb_gadget_vbus_draw(), but
> wait until the UDC class is finished, then we will have a common
> location to do those stuff.

ok, I hope I can get CCed or notified in some way as I might miss that.

<snip>

>> +
>> +       ret = request_threaded_irq(bci->irq_bci, NULL,
>> +               twl4030_bci_interrupt, 0, pdev->name, bci);
>> +       if (ret < 0) {
>> +               dev_err(&pdev->dev, "could not request irq %d, status
>> %d\n",
>> +                       bci->irq_bci, ret);
>> +               goto fail_bci_irq;
>> +       }
>
> you should register the power_supply before enabling the IRQ lines,
> otherwise you might call power_supply_changed() to a non-existent
> power_supply.

good catch, will do.

<snip>

>> +
>> +#ifdef CONFIG_USB_OTG_UTILS
>> +       bci->transceiver = otg_get_transceiver();
>> +#endif
>
> this driver should actually depend on that. It won't work without access
> to the transceiver anyway.

Well it can still do AC charging fine without OTG stuff.

> Or you add something like:
>
> #ifdef CONFIG_USB_OTG_UTILS
> extern struct otg_transceiver *otg_get_transceiver(void);
> extern void otg_put_transceiver(struct otg_transceiver *);
> #else
> static inline struct otg_transceiver *otg_get_transceiver(void)
> { return NULL; }
> static inline void otg_put_transceiver(struct otg_transceiver *x)
> { }
> #endif

I much prefer that, will send a patch.

>
> to the otg.h and avoid having to use that ifdef here and on any other
> driver. (should be a separate patch though).
>
>> +static struct platform_driver twl4030_bci_driver = {
>> +       .probe          = twl4030_bci_probe,
>
> drivers should not reference __init sections anymore. If you make this
> statically linked to the kernel, you'll get section mismatches. It's
> better to make probe __init remove it from here and call
> platform_driver_probe() from module_init();

Tried that and did not get any section mismatches, but will switch
anyway to better match trends.

>
>> +       .remove         = __devexit_p(twl4030_bci_remove),
>> +       .driver         = {
>> +               .name   = "twl4030_bci",
>> +               .owner  = THIS_MODULE,
>> +       },
>> +};
>
> --
> balbi
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/