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

[Felipe Balbi]

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.

> Besides that it now uses threaded interrupts and makes use of charge
> state change interrupt to notify power supply core.
>
> For this driver to work, twl-core needs to be patched to register
> correct platform device, and twl4030-irq needs to set COR bit
> correctly (will send those patches to mfd).

> +static int __devinit twl4030_bci_probe(struct platform_device *pdev)
> +{
> +       struct twl4030_bci *bci;
> +       int ret;
> +       int reg;
> +
> +       bci = kzalloc(sizeof(*bci), GFP_KERNEL);
> +       if (bci == NULL)
> +               return -ENOMEM;
> +
> +       bci->dev = &pdev->dev;
> +       bci->irq_chg = platform_get_irq(pdev, 0);
> +       bci->irq_bci = platform_get_irq(pdev, 1);
> +
> +       ret = request_threaded_irq(bci->irq_chg, NULL,
> +                       twl4030_charger_interrupt, 0, pdev->name, bci);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "could not request irq %d, status %d\n",
> +                       bci->irq_chg, ret);
> +               goto fail_chg_irq;
> +       }
> +
> +       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.

> +       platform_set_drvdata(pdev, bci);
> +
> +       bci->ac.name = "twl4030_ac";
> +       bci->ac.type = POWER_SUPPLY_TYPE_MAINS;
> +       bci->ac.properties = twl4030_charger_props;
> +       bci->ac.num_properties = ARRAY_SIZE(twl4030_charger_props);
> +       bci->ac.get_property = twl4030_bci_get_property;
> +
> +       ret = power_supply_register(&pdev->dev, &bci->ac);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to register ac: %d\n", ret);
> +               goto fail_register_ac;
> +       }
> +
> +       bci->usb.name = "twl4030_usb";
> +       bci->usb.type = POWER_SUPPLY_TYPE_USB;
> +       bci->usb.properties = twl4030_charger_props;
> +       bci->usb.num_properties = ARRAY_SIZE(twl4030_charger_props);
> +       bci->usb.get_property = twl4030_bci_get_property;
> +
> +       ret = power_supply_register(&pdev->dev, &bci->usb);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to register usb: %d\n", ret);
> +               goto fail_register_usb;
> +       }
> +
> +#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. 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

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();

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