> From: Graeme Gregory <gg@xxxxxxxxxxxxxxx> > > This is the driver for the USB comparator built into the palmas chip. It > handles the various USB OTG events that can be generated by cable > insertion/removal. > > Signed-off-by: Graeme Gregory <gg@xxxxxxxxxxxxxxx> > Signed-off-by: Moiz Sonasath <m-sonasath@xxxxxx> > Signed-off-by: Ruchika Kharwar <ruchika@xxxxxx> > Signed-off-by: Kishon Vijay Abraham I <kishon@xxxxxx> > [kishon@xxxxxx: adapted palmas usb driver to use the extcon framework] > Signed-off-by: Sebastien Guiriec <s-guiriec@xxxxxx> Here goes some comments in the code: [] > diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c > new file mode 100644 > index 0000000..3ef042f > --- /dev/null > +++ b/drivers/extcon/extcon-palmas.c > @@ -0,0 +1,389 @@ > +/* > + * Palmas USB transceiver driver > + * > + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * Author: Graeme Gregory <gg@xxxxxxxxxxxxxxx> > + * Author: Kishon Vijay Abraham I <kishon@xxxxxx> > + * > + * Based on twl6030_usb.c > + * > + * Author: Hema HK <hemahk@xxxxxx> > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ Why the email addresses are masked in the source code? (I'm just curious as this is the first time to see such in kernel source) > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/platform_device.h> > +#include <linux/io.h> > +#include <linux/usb/phy_companion.h> > +#include <linux/regulator/consumer.h> > +#include <linux/err.h> > +#include <linux/notifier.h> > +#include <linux/slab.h> > +#include <linux/delay.h> > +#include <linux/mfd/palmas.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/extcon/extcon_palmas.h> > + > +static const char *palmas_extcon_cable[] = { > + [0] = "USB", > + [1] = "USB-HOST", [1] = "USB-Host", > + NULL, > +}; > + > +static const int mutually_exclusive[] = {0x3, 0x0}; > + > +static void palmas_usb_wakeup(struct palmas *palmas, int enable) > +{ > + if (enable) > + palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP, > + PALMAS_USB_WAKEUP_ID_WK_UP_COMP); > + else > + palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP, 0); > +} > + > +static ssize_t palmas_usb_vbus_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + unsigned long flags; > + int ret = -EINVAL; > + struct palmas_usb *palmas_usb = dev_get_drvdata(dev); > + > + spin_lock_irqsave(&palmas_usb->lock, flags); This spinlock is used in this sysfs-show function only. Is this really needed? The only protected value here is "linkstat", which is "readonly" in this protected context. Thus, removing the spin_lock and updating like the following should be the same with less overhead: int linkstat = palmas_usb->linkstat; switch(linkstat) { > + > + switch (palmas_usb->linkstat) { > + case PALMAS_USB_STATE_VBUS: > + ret = snprintf(buf, PAGE_SIZE, "vbus\n"); > + break; > + case PALMAS_USB_STATE_ID: > + ret = snprintf(buf, PAGE_SIZE, "id\n"); > + break; > + case PALMAS_USB_STATE_DISCONNECT: > + ret = snprintf(buf, PAGE_SIZE, "none\n"); > + break; > + default: > + ret = snprintf(buf, PAGE_SIZE, "UNKNOWN\n"); > + } > + spin_unlock_irqrestore(&palmas_usb->lock, flags); > + > + return ret; > +} > +static DEVICE_ATTR(vbus, 0444, palmas_usb_vbus_show, NULL); > + [] > + > +static void palmas_set_vbus_work(struct work_struct *data) > +{ > + int ret; > + struct palmas_usb *palmas_usb = container_of(data, struct palmas_usb, > + set_vbus_work); > + > + if (IS_ERR_OR_NULL(palmas_usb->vbus_reg)) { > + dev_err(palmas_usb->dev, "invalid regulator\n"); > + return; > + } > + > + /* > + * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1 > + * register. This enables boost mode. > + */ > + > + if (palmas_usb->vbus_enable) { > + ret = regulator_enable(palmas_usb->vbus_reg); > + if (ret) > + dev_dbg(palmas_usb->dev, "regulator enable failed\n"); > + } else { > + regulator_disable(palmas_usb->vbus_reg); > + } > +} > + > +static int palmas_set_vbus(struct phy_companion *comparator, bool enabled) > +{ > + struct palmas_usb *palmas_usb = comparator_to_palmas(comparator); > + > + palmas_usb->vbus_enable = enabled; > + schedule_work(&palmas_usb->set_vbus_work); > + > + return 0; > +} > + > +static int palmas_start_srp(struct phy_companion *comparator) > +{ > + struct palmas_usb *palmas_usb = comparator_to_palmas(comparator); > + > + palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE, > + PALMAS_USB_VBUS_CTRL_SET, PALMAS_USB_VBUS_CTRL_SET_VBUS_DISCHRG > + | PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK); > + palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE, > + PALMAS_USB_VBUS_CTRL_SET, > + PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS | > + PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK); > + > + mdelay(100); Why not msleep()? It's long enough to consider sleep. Is this in an atomic context? (if so, 100msec seems even more awful) > + > + palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE, > + PALMAS_USB_VBUS_CTRL_CLR, > + PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS | > + PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS); > + > + return 0; > +} > + > +static void palmas_dt_to_pdata(struct device_node *node, > + struct palmas_usb_platform_data *pdata) > +{ > + pdata->no_control_vbus = of_property_read_bool(node, > + "ti,no_control_vbus"); > + pdata->wakeup = of_property_read_bool(node, "ti,wakeup"); > +} > + > +static int palmas_usb_probe(struct platform_device *pdev) > +{ [] > + /* init spinlock for workqueue */ > + spin_lock_init(&palmas_usb->lock); [] > + /* init spinlock for workqueue */ > + spin_lock_init(&palmas_usb->lock); Why this lock is initialized again? > + > + INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work); > + [] > + > +module_platform_driver(palmas_usb_driver); > + > +MODULE_ALIAS("platform:palmas-usb"); > +MODULE_AUTHOR("Graeme Gregory <gg@xxxxxxxxxxxxxxx>"); Is this intentional? > +MODULE_DESCRIPTION("Palmas USB transceiver driver"); > +MODULE_LICENSE("GPL"); > +MODULE_DEVICE_TABLE(of, of_palmas_match_tbl); > Cheers, MyungJoo -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
