From: Heikki Krogerus <[email protected]>
NXP ISP1704 is Battery Charging Specification 1.0 compliant USB
transceiver. This adds a power supply driver for ISP1704 and
ISP1707 USB transceivers.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/power/Kconfig | 7 +
drivers/power/Makefile | 1 +
drivers/power/isp1704_charger.c | 371 +++++++++++++++++++++++++++++++++++++++
3 files changed, 379 insertions(+), 0 deletions(-)
create mode 100644 drivers/power/isp1704_charger.c
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 0734356..d55fc29 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -166,4 +166,11 @@ config BATTERY_INTEL_MID
Say Y here to enable the battery driver on Intel MID
platforms.
+config CHARGER_ISP1704
+ tristate "ISP1704 USB Charger Detection"
+ select NOP_USB_XCEIV if ! MACH_NOKIA_RX51
+ help
+ Say Y to enable support for USB Charger Detection with
+ ISP1707/ISP1704 USB transceivers.
+
endif # POWER_SUPPLY
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 10143aa..c73d381 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -37,3 +37,4 @@ obj-$(CONFIG_BATTERY_S3C_ADC) += s3c_adc_battery.o
obj-$(CONFIG_CHARGER_PCF50633) += pcf50633-charger.o
obj-$(CONFIG_BATTERY_JZ4740) += jz4740-battery.o
obj-$(CONFIG_BATTERY_INTEL_MID) += intel_mid_battery.o
+obj-$(CONFIG_CHARGER_ISP1704) += isp1704_charger.o
diff --git a/drivers/power/isp1704_charger.c b/drivers/power/isp1704_charger.c
new file mode 100644
index 0000000..821ad29
--- /dev/null
+++ b/drivers/power/isp1704_charger.c
@@ -0,0 +1,371 @@
+/*
+ * isp1704_charger.c - ISP1704 USB Charger Detection driver
+ *
+ * Copyright (C) 2010 Nokia Corporation
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/sysfs.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/delay.h>
+
+#include <linux/usb/otg.h>
+#include <linux/usb/ulpi.h>
+#include <linux/usb/ch9.h>
+#include <linux/usb/gadget.h>
+
+/* Vendor specific Power Control register */
+#define ISP1704_PWR_CTRL 0x3d
+#define ISP1704_PWR_CTRL_SWCTRL (1 << 0)
+#define ISP1704_PWR_CTRL_DET_COMP (1 << 1)
+#define ISP1704_PWR_CTRL_BVALID_RISE (1 << 2)
+#define ISP1704_PWR_CTRL_BVALID_FALL (1 << 3)
+#define ISP1704_PWR_CTRL_DP_WKPU_EN (1 << 4)
+#define ISP1704_PWR_CTRL_VDAT_DET (1 << 5)
+#define ISP1704_PWR_CTRL_DPVSRC_EN (1 << 6)
+#define ISP1704_PWR_CTRL_HWDETECT (1 << 7)
+
+#define NXP_VENDOR_ID 0x04cc
+
+static u16 isp170x_id[] = {
+ 0x1704,
+ 0x1707,
+};
+
+struct isp1704_charger {
+ struct device *dev;
+ struct power_supply psy;
+ struct otg_transceiver *otg;
+ struct notifier_block nb;
+ struct work_struct work;
+
+ char model[7];
+ unsigned present:1;
+};
+
+/*
+ * ISP1704 detects PS/2 adapters as charger. To make sure the detected charger
+ * is actually a dedicated charger, the following steps need to be taken.
+ */
+static inline int isp1704_charger_verify(struct isp1704_charger *isp)
+{
+ u8 r, ret = 0;
+
+ /* Reset the transceiver */
+ r = otg_io_read(isp->otg, ULPI_FUNC_CTRL);
+ r |= ULPI_FUNC_CTRL_RESET;
+ otg_io_write(isp->otg, ULPI_FUNC_CTRL, r);
+ usleep_range(1000, 2000);
+
+ /* Set normal mode */
+ r &= ~(ULPI_FUNC_CTRL_RESET | ULPI_FUNC_CTRL_OPMODE_MASK);
+ otg_io_write(isp->otg, ULPI_FUNC_CTRL, r);
+
+ /* Clear the DP and DM pull-down bits */
+ r = ULPI_OTG_CTRL_DP_PULLDOWN | ULPI_OTG_CTRL_DM_PULLDOWN;
+ otg_io_write(isp->otg, ULPI_CLR(ULPI_OTG_CTRL), r);
+
+ /* Enable strong pull-up on DP (1.5K) and reset */
+ r = ULPI_FUNC_CTRL_TERMSELECT | ULPI_FUNC_CTRL_RESET;
+ otg_io_write(isp->otg, ULPI_SET(ULPI_FUNC_CTRL), r);
+ usleep_range(1000, 2000);
+
+ /* Read the line state */
+ if (otg_io_read(isp->otg, ULPI_DEBUG)) {
+ /* Is it a charger or PS/2 connection */
+
+ /* Enable weak pull-up resistor on DP */
+ otg_io_write(isp->otg, ULPI_SET(ISP1704_PWR_CTRL),
+ ISP1704_PWR_CTRL_DP_WKPU_EN);
+
+ /* Disable strong pull-up on DP (1.5K) */
+ otg_io_write(isp->otg, ULPI_CLR(ULPI_FUNC_CTRL),
+ ULPI_FUNC_CTRL_TERMSELECT);
+
+ /* Enable weak pull-down resistor on DM */
+ otg_io_write(isp->otg, ULPI_SET(ULPI_OTG_CTRL),
+ ULPI_OTG_CTRL_DM_PULLDOWN);
+
+ /* It's a charger if the line states are clear */
+ if (!(otg_io_read(isp->otg, ULPI_DEBUG)))
+ ret = 1;
+
+ /* Disable weak pull-up resistor on DP */
+ otg_io_write(isp->otg, ULPI_CLR(ISP1704_PWR_CTRL),
+ ISP1704_PWR_CTRL_DP_WKPU_EN);
+ } else {
+ ret = 1;
+
+ /* Disable strong pull-up on DP (1.5K) */
+ otg_io_write(isp->otg, ULPI_CLR(ULPI_FUNC_CTRL),
+ ULPI_FUNC_CTRL_TERMSELECT);
+ }
+
+ return ret;
+}
+
+static inline int isp1704_charger_detect(struct isp1704_charger *isp)
+{
+ unsigned long timeout;
+ u8 r;
+ int vdat;
+
+ /* set SW control bit in PWR_CTRL register */
+ otg_io_write(isp->otg, ISP1704_PWR_CTRL,
+ ISP1704_PWR_CTRL_SWCTRL);
+
+ /* enable manual charger detection */
+ r = (ISP1704_PWR_CTRL_SWCTRL | ISP1704_PWR_CTRL_DPVSRC_EN);
+ otg_io_write(isp->otg, ULPI_SET(ISP1704_PWR_CTRL), r);
+ usleep_range(1000, 2000);
+
+ timeout = jiffies + msecs_to_jiffies(300);
+ while (!time_after(jiffies, timeout)) {
+ /* Check if there is a charger */
+ vdat = !!(otg_io_read(isp->otg, ISP1704_PWR_CTRL)
+ & ISP1704_PWR_CTRL_VDAT_DET);
+ if (vdat)
+ break;
+ }
+ return vdat;
+}
+
+static void isp1704_charger_work(struct work_struct *data)
+{
+ struct isp1704_charger *isp =
+ container_of(data, struct isp1704_charger, work);
+
+ /* FIXME Only supporting dedicated chargers even though isp1704 can
+ * detect HUB and HOST chargers. If the device has already been
+ * enumerated, the detection will break the connection.
+ */
+ if (isp->otg->state != OTG_STATE_B_IDLE)
+ return;
+
+ /* disable data pullups */
+ if (isp->otg->gadget)
+ usb_gadget_disconnect(isp->otg->gadget);
+
+ /* detect charger */
+ if (isp1704_charger_detect(isp))
+ isp->present = isp1704_charger_verify(isp);
+
+ /* enable data pullups */
+ if (isp->otg->gadget)
+ usb_gadget_connect(isp->otg->gadget);
+
+ if (isp->present)
+ power_supply_changed(&isp->psy);
+}
+
+static int isp1707_notifier_call(struct notifier_block *nb,
+ unsigned long event, void *unused)
+{
+ struct isp1704_charger *isp =
+ container_of(nb, struct isp1704_charger, nb);
+
+ switch (event) {
+ case USB_EVENT_VBUS:
+ schedule_work(&isp->work);
+ break;
+ case USB_EVENT_NONE:
+ isp->present = 0;
+ power_supply_changed(&isp->psy);
+ break;
+ default:
+ return NOTIFY_DONE;
+ }
+
+ return NOTIFY_OK;
+}
+
+static int isp1704_charger_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct isp1704_charger *isp =
+ container_of(psy, struct isp1704_charger, psy);
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_PRESENT:
+ val->intval = isp->present;
+ break;
+ case POWER_SUPPLY_PROP_MODEL_NAME:
+ val->strval = isp->model;
+ break;
+ case POWER_SUPPLY_PROP_MANUFACTURER:
+ val->strval = "NXP";
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static enum power_supply_property power_props[] = {
+ POWER_SUPPLY_PROP_PRESENT,
+ POWER_SUPPLY_PROP_MODEL_NAME,
+ POWER_SUPPLY_PROP_MANUFACTURER,
+};
+
+static inline int isp1704_test_ulpi(struct isp1704_charger *isp)
+{
+ int vendor, product, i;
+ int ret = -ENODEV;
+
+ /* Test ULPI interface */
+ ret = otg_io_write(isp->otg, ULPI_SCRATCH, 0xaa);
+ if (ret < 0)
+ return ret;
+ ret = otg_io_read(isp->otg, ULPI_SCRATCH);
+ if (ret < 0)
+ return ret;
+ if (ret != 0xaa)
+ return -ENODEV;
+ /* Verify the product and vendor id matches */
+ vendor = otg_io_read(isp->otg, ULPI_VENDOR_ID_LOW);
+ vendor |= otg_io_read(isp->otg, ULPI_VENDOR_ID_HIGH) << 8;
+ if (vendor != NXP_VENDOR_ID)
+ return -ENODEV;
+ for (i = 0; i < ARRAY_SIZE(isp170x_id); i++) {
+ product = otg_io_read(isp->otg, ULPI_PRODUCT_ID_LOW);
+ product |= otg_io_read(isp->otg, ULPI_PRODUCT_ID_HIGH) << 8;
+ if (product == isp170x_id[i]) {
+ sprintf(isp->model, "isp%x", product);
+ return product;
+ }
+ }
+
+ dev_err(isp->dev, "product id %x not matching known ids", product);
+
+ return -ENODEV;
+}
+
+static int __devinit isp1704_charger_probe(struct platform_device *pdev)
+{
+ struct isp1704_charger *isp;
+ int ret = -ENODEV;
+
+ isp = kzalloc(sizeof *isp, GFP_KERNEL);
+ if (!isp)
+ return -ENOMEM;
+
+ isp->otg = otg_get_transceiver();
+ if (!isp->otg) {
+ kfree(isp);
+ return ret;
+ }
+
+ ret = isp1704_test_ulpi(isp);
+ if (ret < 0)
+ goto fail;
+
+ isp->dev = &pdev->dev;
+ platform_set_drvdata(pdev, isp);
+
+ isp->psy.name = "isp1704";
+ isp->psy.type = POWER_SUPPLY_TYPE_USB;
+ isp->psy.properties = power_props;
+ isp->psy.num_properties = ARRAY_SIZE(power_props);
+ isp->psy.get_property = isp1704_charger_get_property;
+
+ ret = power_supply_register(isp->dev, &isp->psy);
+ if (ret)
+ goto fail;
+
+ /* REVISIT: using work in order to allow the otg notifications to be
+ * made atomically in the future.
+ */
+ INIT_WORK(&isp->work, isp1704_charger_work);
+
+ isp->nb.notifier_call = isp1707_notifier_call;
+ ret = otg_register_notifier(isp->otg, &isp->nb);
+ if (ret)
+ goto fail2;
+
+ dev_info(isp->dev, "registered with product id %s\n", isp->model);
+
+ return 0;
+fail2:
+ power_supply_unregister(&isp->psy);
+fail:
+ otg_put_transceiver(isp->otg);
+ kfree(isp);
+
+ dev_err(&pdev->dev, "failed to register isp1704 with error %d\n", ret);
+
+ return ret;
+}
+
+static int __devexit isp1704_charger_remove(struct platform_device *pdev)
+{
+ struct isp1704_charger *isp = platform_get_drvdata(pdev);
+
+ otg_unregister_notifier(isp->otg, &isp->nb);
+ power_supply_unregister(&isp->psy);
+ otg_put_transceiver(isp->otg);
+ kfree(isp);
+
+ return 0;
+}
+
+static struct platform_driver isp1704_charger_driver = {
+ .driver = {
+ .name = "isp1704_charger",
+ },
+ .probe = isp1704_charger_probe,
+ .remove = __devexit_p(isp1704_charger_remove),
+};
+
+static struct platform_device *isp1704_device;
+
+static int __init isp1704_charger_init(void)
+{
+ int ret = 0;
+
+ ret = platform_driver_register(&isp1704_charger_driver);
+ if (ret)
+ return ret;
+
+ isp1704_device = platform_device_register_simple("isp1704_charger",
+ 0, NULL, 0);
+ if (IS_ERR(isp1704_device)) {
+ platform_driver_unregister(&isp1704_charger_driver);
+ ret = PTR_ERR(isp1704_device);
+ }
+
+ return ret;
+}
+module_init(isp1704_charger_init);
+
+static void __exit isp1704_charger_exit(void)
+{
+ platform_device_unregister(isp1704_device);
+ platform_driver_unregister(&isp1704_charger_driver);
+}
+module_exit(isp1704_charger_exit);
+
+MODULE_ALIAS("platform:isp1704-charger");
+MODULE_AUTHOR("Nokia Corporation");
+MODULE_DESCRIPTION("ISP170x USB Charger driver");
+MODULE_LICENSE("GPL");
--
1.7.0.4
On Wed, Aug 18, 2010 at 04:01:05PM +0300, Krogerus Heikki (EXT-Teleca/Helsinki) wrote:
> From: Heikki Krogerus <[email protected]>
>
> NXP ISP1704 is Battery Charging Specification 1.0 compliant USB
> transceiver. This adds a power supply driver for ISP1704 and
> ISP1707 USB transceivers.
>
> Signed-off-by: Heikki Krogerus <[email protected]>
> ---
I like this driver (very). A few, mostly cosmetic comments down
below.
[...]
> +config CHARGER_ISP1704
> + tristate "ISP1704 USB Charger Detection"
> + select NOP_USB_XCEIV if ! MACH_NOKIA_RX51
> + help
"! MACH_NOKIA_RX51" looks wrong here. You probably don't actually
need this if you fix platform device registration issue (see the
very bottom of this email).
[...]
> +/*
> + * ISP1704 detects PS/2 adapters as charger. To make sure the detected charger
> + * is actually a dedicated charger, the following steps need to be taken.
> + */
> +static inline int isp1704_charger_verify(struct isp1704_charger *isp)
> +{
> + u8 r, ret = 0;
Please put variable declarations on separate lines, plus,
'ret' should probably be int, i.e.
int ret;
u8 r;
> +
> + /* Reset the transceiver */
> + r = otg_io_read(isp->otg, ULPI_FUNC_CTRL);
> + r |= ULPI_FUNC_CTRL_RESET;
> + otg_io_write(isp->otg, ULPI_FUNC_CTRL, r);
> + usleep_range(1000, 2000);
> +
> + /* Set normal mode */
> + r &= ~(ULPI_FUNC_CTRL_RESET | ULPI_FUNC_CTRL_OPMODE_MASK);
> + otg_io_write(isp->otg, ULPI_FUNC_CTRL, r);
> +
> + /* Clear the DP and DM pull-down bits */
> + r = ULPI_OTG_CTRL_DP_PULLDOWN | ULPI_OTG_CTRL_DM_PULLDOWN;
> + otg_io_write(isp->otg, ULPI_CLR(ULPI_OTG_CTRL), r);
> +
> + /* Enable strong pull-up on DP (1.5K) and reset */
> + r = ULPI_FUNC_CTRL_TERMSELECT | ULPI_FUNC_CTRL_RESET;
> + otg_io_write(isp->otg, ULPI_SET(ULPI_FUNC_CTRL), r);
> + usleep_range(1000, 2000);
> +
> + /* Read the line state */
> + if (otg_io_read(isp->otg, ULPI_DEBUG)) {
> + /* Is it a charger or PS/2 connection */
> +
> + /* Enable weak pull-up resistor on DP */
> + otg_io_write(isp->otg, ULPI_SET(ISP1704_PWR_CTRL),
> + ISP1704_PWR_CTRL_DP_WKPU_EN);
> +
> + /* Disable strong pull-up on DP (1.5K) */
> + otg_io_write(isp->otg, ULPI_CLR(ULPI_FUNC_CTRL),
> + ULPI_FUNC_CTRL_TERMSELECT);
> +
> + /* Enable weak pull-down resistor on DM */
> + otg_io_write(isp->otg, ULPI_SET(ULPI_OTG_CTRL),
> + ULPI_OTG_CTRL_DM_PULLDOWN);
> +
> + /* It's a charger if the line states are clear */
> + if (!(otg_io_read(isp->otg, ULPI_DEBUG)))
> + ret = 1;
> +
> + /* Disable weak pull-up resistor on DP */
> + otg_io_write(isp->otg, ULPI_CLR(ISP1704_PWR_CTRL),
> + ISP1704_PWR_CTRL_DP_WKPU_EN);
> + } else {
> + ret = 1;
> +
> + /* Disable strong pull-up on DP (1.5K) */
> + otg_io_write(isp->otg, ULPI_CLR(ULPI_FUNC_CTRL),
> + ULPI_FUNC_CTRL_TERMSELECT);
> + }
How about
if (!otg_io_read(isp->otg, ULPI_DEBUG)) {
/* Disable strong pull-up on DP (1.5K) */
otg_io_write(isp->otg, ULPI_CLR(ULPI_FUNC_CTRL),
ULPI_FUNC_CTRL_TERMSELECT);
return 1;
}
<the rest>
Saves indentation level.
> + return ret;
> +}
> +
> +static inline int isp1704_charger_detect(struct isp1704_charger *isp)
> +{
> + unsigned long timeout;
> + u8 r;
> + int vdat;
> +
> + /* set SW control bit in PWR_CTRL register */
> + otg_io_write(isp->otg, ISP1704_PWR_CTRL,
> + ISP1704_PWR_CTRL_SWCTRL);
> +
> + /* enable manual charger detection */
> + r = (ISP1704_PWR_CTRL_SWCTRL | ISP1704_PWR_CTRL_DPVSRC_EN);
> + otg_io_write(isp->otg, ULPI_SET(ISP1704_PWR_CTRL), r);
> + usleep_range(1000, 2000);
> +
> + timeout = jiffies + msecs_to_jiffies(300);
> + while (!time_after(jiffies, timeout)) {
I guess it is possible that vdat might become uninitialized
if the code never hits the loop. The time between
timeout = jiffies + msecs_to_jiffies(300);
and
while (!time_after(jiffies, timeout)) {
is undefined.
> + /* Check if there is a charger */
> + vdat = !!(otg_io_read(isp->otg, ISP1704_PWR_CTRL)
> + & ISP1704_PWR_CTRL_VDAT_DET);
Technically, there is no need for "!!".
> + if (vdat)
> + break;
> + }
> + return vdat;
> +}
> +
> +static void isp1704_charger_work(struct work_struct *data)
> +{
> + struct isp1704_charger *isp =
> + container_of(data, struct isp1704_charger, work);
> +
> + /* FIXME Only supporting dedicated chargers even though isp1704 can
> + * detect HUB and HOST chargers. If the device has already been
> + * enumerated, the detection will break the connection.
> + */
> + if (isp->otg->state != OTG_STATE_B_IDLE)
> + return;
> +
> + /* disable data pullups */
> + if (isp->otg->gadget)
> + usb_gadget_disconnect(isp->otg->gadget);
> +
> + /* detect charger */
shouldn't be there 'isp->present = 0;'?..
> + if (isp1704_charger_detect(isp))
> + isp->present = isp1704_charger_verify(isp);
i.e. it's kinda weird to see conditional isp->present update...
> +
> + /* enable data pullups */
> + if (isp->otg->gadget)
> + usb_gadget_connect(isp->otg->gadget);
> +
> + if (isp->present)
...and then unconditional checking of isp->present.
I guess the code is OK, and somewhere else the logic handles
this... but still an odd-looking function.
> + power_supply_changed(&isp->psy);
> +}
> +
> +static int isp1707_notifier_call(struct notifier_block *nb,
> + unsigned long event, void *unused)
> +{
> + struct isp1704_charger *isp =
> + container_of(nb, struct isp1704_charger, nb);
> +
> + switch (event) {
> + case USB_EVENT_VBUS:
> + schedule_work(&isp->work);
> + break;
> + case USB_EVENT_NONE:
> + isp->present = 0;
Probably this makes the code above work. But why don't we
call schedule_work(&isp->work)?
[...]
> +static inline int isp1704_test_ulpi(struct isp1704_charger *isp)
> +{
> + int vendor, product, i;
int vendor;
int product;
int i;
> + int ret = -ENODEV;
> +
> + /* Test ULPI interface */
> + ret = otg_io_write(isp->otg, ULPI_SCRATCH, 0xaa);
> + if (ret < 0)
> + return ret;
By adding empty line here you'll make the code prettier.
> + ret = otg_io_read(isp->otg, ULPI_SCRATCH);
> + if (ret < 0)
> + return ret;
Ditto.
> + if (ret != 0xaa)
> + return -ENODEV;
Ditto.
> + /* Verify the product and vendor id matches */
> + vendor = otg_io_read(isp->otg, ULPI_VENDOR_ID_LOW);
> + vendor |= otg_io_read(isp->otg, ULPI_VENDOR_ID_HIGH) << 8;
> + if (vendor != NXP_VENDOR_ID)
> + return -ENODEV;
Ditto.
> + for (i = 0; i < ARRAY_SIZE(isp170x_id); i++) {
> + product = otg_io_read(isp->otg, ULPI_PRODUCT_ID_LOW);
> + product |= otg_io_read(isp->otg, ULPI_PRODUCT_ID_HIGH) << 8;
> + if (product == isp170x_id[i]) {
> + sprintf(isp->model, "isp%x", product);
> + return product;
> + }
> + }
> +
> + dev_err(isp->dev, "product id %x not matching known ids", product);
> +
> + return -ENODEV;
> +}
> +
> +static int __devinit isp1704_charger_probe(struct platform_device *pdev)
> +{
> + struct isp1704_charger *isp;
> + int ret = -ENODEV;
> +
> + isp = kzalloc(sizeof *isp, GFP_KERNEL);
> + if (!isp)
> + return -ENOMEM;
> +
> + isp->otg = otg_get_transceiver();
> + if (!isp->otg) {
> + kfree(isp);
> + return ret;
goto failX instead?
> + }
> +
> + ret = isp1704_test_ulpi(isp);
> + if (ret < 0)
> + goto fail;
> +
> + isp->dev = &pdev->dev;
> + platform_set_drvdata(pdev, isp);
> +
> + isp->psy.name = "isp1704";
> + isp->psy.type = POWER_SUPPLY_TYPE_USB;
> + isp->psy.properties = power_props;
> + isp->psy.num_properties = ARRAY_SIZE(power_props);
> + isp->psy.get_property = isp1704_charger_get_property;
> +
> + ret = power_supply_register(isp->dev, &isp->psy);
> + if (ret)
> + goto fail;
> +
> + /* REVISIT: using work in order to allow the otg notifications to be
> + * made atomically in the future.
> + */
> + INIT_WORK(&isp->work, isp1704_charger_work);
> +
> + isp->nb.notifier_call = isp1707_notifier_call;
Add an empty line here?
> + ret = otg_register_notifier(isp->otg, &isp->nb);
> + if (ret)
> + goto fail2;
> +
> + dev_info(isp->dev, "registered with product id %s\n", isp->model);
> +
> + return 0;
> +fail2:
> + power_supply_unregister(&isp->psy);
> +fail:
> + otg_put_transceiver(isp->otg);
> + kfree(isp);
> +
> + dev_err(&pdev->dev, "failed to register isp1704 with error %d\n", ret);
> +
> + return ret;
> +}
[...]
> +static int __init isp1704_charger_init(void)
> +{
> + int ret = 0;
> +
> + ret = platform_driver_register(&isp1704_charger_driver);
> + if (ret)
> + return ret;
> +
> + isp1704_device = platform_device_register_simple("isp1704_charger",
> + 0, NULL, 0);
This is wrong. Please move this into either isp1704 generic
or usb driver (if any), or platform (board) code.
That way you also won't need "! MACH_NOKIA_RX51" in the Kconfig.
Thanks!
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
Hi,
On 08/18/2010 04:01 PM, Krogerus Heikki (EXT-Teleca/Helsinki) wrote:
> From: Heikki Krogerus<[email protected]>
>
> NXP ISP1704 is Battery Charging Specification 1.0 compliant USB
> transceiver. This adds a power supply driver for ISP1704 and
> ISP1707 USB transceivers.
>
> Signed-off-by: Heikki Krogerus<[email protected]>
> ---
> drivers/power/Kconfig | 7 +
> drivers/power/Makefile | 1 +
> drivers/power/isp1704_charger.c | 371 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 379 insertions(+), 0 deletions(-)
> create mode 100644 drivers/power/isp1704_charger.c
>
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 0734356..d55fc29 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -166,4 +166,11 @@ config BATTERY_INTEL_MID
> Say Y here to enable the battery driver on Intel MID
> platforms.
>
> +config CHARGER_ISP1704
> + tristate "ISP1704 USB Charger Detection"
> + select NOP_USB_XCEIV if ! MACH_NOKIA_RX51
As Anton mentioned, this is wrong.
It is possible for platforms other than RX51 to use ISP1704 and this driver
should be usable on them without modifications to kconfig or driver code.
> + help
> + Say Y to enable support for USB Charger Detection with
> + ISP1707/ISP1704 USB transceivers.
> +
> endif # POWER_SUPPLY
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index 10143aa..c73d381 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -37,3 +37,4 @@ obj-$(CONFIG_BATTERY_S3C_ADC) += s3c_adc_battery.o
> obj-$(CONFIG_CHARGER_PCF50633) += pcf50633-charger.o
> obj-$(CONFIG_BATTERY_JZ4740) += jz4740-battery.o
> obj-$(CONFIG_BATTERY_INTEL_MID) += intel_mid_battery.o
> +obj-$(CONFIG_CHARGER_ISP1704) += isp1704_charger.o
> diff --git a/drivers/power/isp1704_charger.c b/drivers/power/isp1704_charger.c
> new file mode 100644
> index 0000000..821ad29
> --- /dev/null
> +++ b/drivers/power/isp1704_charger.c
> @@ -0,0 +1,371 @@
> +/*
> + * isp1704_charger.c - ISP1704 USB Charger Detection driver
> + *
> + * Copyright (C) 2010 Nokia Corporation
> + *
> + * 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.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
<snip>
> +
> +static inline int isp1704_test_ulpi(struct isp1704_charger *isp)
> +{
> + int vendor, product, i;
> + int ret = -ENODEV;
> +
> + /* Test ULPI interface */
> + ret = otg_io_write(isp->otg, ULPI_SCRATCH, 0xaa);
> + if (ret< 0)
> + return ret;
> + ret = otg_io_read(isp->otg, ULPI_SCRATCH);
> + if (ret< 0)
> + return ret;
> + if (ret != 0xaa)
> + return -ENODEV;
> + /* Verify the product and vendor id matches */
> + vendor = otg_io_read(isp->otg, ULPI_VENDOR_ID_LOW);
> + vendor |= otg_io_read(isp->otg, ULPI_VENDOR_ID_HIGH)<< 8;
> + if (vendor != NXP_VENDOR_ID)
> + return -ENODEV;
> + for (i = 0; i< ARRAY_SIZE(isp170x_id); i++) {
> + product = otg_io_read(isp->otg, ULPI_PRODUCT_ID_LOW);
> + product |= otg_io_read(isp->otg, ULPI_PRODUCT_ID_HIGH)<< 8;
product id should be read outside the for loop. This way you will save
unnecessary otg_io_reads. product id won't change anyways.
> + if (product == isp170x_id[i]) {
> + sprintf(isp->model, "isp%x", product);
> + return product;
> + }
> + }
> +
> + dev_err(isp->dev, "product id %x not matching known ids", product);
> +
> + return -ENODEV;
> +}
<snip>
> +static struct platform_driver isp1704_charger_driver = {
> + .driver = {
> + .name = "isp1704_charger",
> + },
> + .probe = isp1704_charger_probe,
> + .remove = __devexit_p(isp1704_charger_remove),
> +};
> +
> +static struct platform_device *isp1704_device;
> +
> +static int __init isp1704_charger_init(void)
> +{
> + int ret = 0;
> +
> + ret = platform_driver_register(&isp1704_charger_driver);
> + if (ret)
> + return ret;
> +
> + isp1704_device = platform_device_register_simple("isp1704_charger",
> + 0, NULL, 0);
This platform_device_register should be done in the rx51 board file. i.e.
board-rx51-peripherals.c
> + if (IS_ERR(isp1704_device)) {
> + platform_driver_unregister(&isp1704_charger_driver);
> + ret = PTR_ERR(isp1704_device);
> + }
> +
> + return ret;
> +}
> +module_init(isp1704_charger_init);
> +
> +static void __exit isp1704_charger_exit(void)
> +{
> + platform_device_unregister(isp1704_device);
don't do platform_device_unregister here.
> + platform_driver_unregister(&isp1704_charger_driver);
> +}
> +module_exit(isp1704_charger_exit);
> +
> +MODULE_ALIAS("platform:isp1704-charger");
> +MODULE_AUTHOR("Nokia Corporation");
> +MODULE_DESCRIPTION("ISP170x USB Charger driver");
> +MODULE_LICENSE("GPL");
--
regards,
-roger
Hi,
On Wed, Aug 18, 2010 at 03:42:37PM +0200, ext Anton Vorontsov wrote:
>On Wed, Aug 18, 2010 at 04:01:05PM +0300, Krogerus Heikki (EXT-Teleca/Helsinki) wrote:
>> NXP ISP1704 is Battery Charging Specification 1.0 compliant USB
>> transceiver. This adds a power supply driver for ISP1704 and
>> ISP1707 USB transceivers.
>>
>> Signed-off-by: Heikki Krogerus <[email protected]>
>> ---
>
> I like this driver (very). A few, mostly cosmetic comments down
> below.
>
> [...]
>> +config CHARGER_ISP1704
>> + tristate "ISP1704 USB Charger Detection"
>> + select NOP_USB_XCEIV if ! MACH_NOKIA_RX51
>> + help
>
> "! MACH_NOKIA_RX51" looks wrong here. You probably don't actually
> need this if you fix platform device registration issue (see the
> very bottom of this email).
>
> [...]
>> +/*
>> + * ISP1704 detects PS/2 adapters as charger. To make sure the detected charger
>> + * is actually a dedicated charger, the following steps need to be taken.
>> + */
>> +static inline int isp1704_charger_verify(struct isp1704_charger *isp)
>> +{
>> + u8 r, ret = 0;
>
> Please put variable declarations on separate lines, plus,
> 'ret' should probably be int, i.e.
>
> int ret;
> u8 r;
OK
>> +
>> + /* Reset the transceiver */
>> + r = otg_io_read(isp->otg, ULPI_FUNC_CTRL);
>> + r |= ULPI_FUNC_CTRL_RESET;
>> + otg_io_write(isp->otg, ULPI_FUNC_CTRL, r);
>> + usleep_range(1000, 2000);
>> +
>> + /* Set normal mode */
>> + r &= ~(ULPI_FUNC_CTRL_RESET | ULPI_FUNC_CTRL_OPMODE_MASK);
>> + otg_io_write(isp->otg, ULPI_FUNC_CTRL, r);
>> +
>> + /* Clear the DP and DM pull-down bits */
>> + r = ULPI_OTG_CTRL_DP_PULLDOWN | ULPI_OTG_CTRL_DM_PULLDOWN;
>> + otg_io_write(isp->otg, ULPI_CLR(ULPI_OTG_CTRL), r);
>> +
>> + /* Enable strong pull-up on DP (1.5K) and reset */
>> + r = ULPI_FUNC_CTRL_TERMSELECT | ULPI_FUNC_CTRL_RESET;
>> + otg_io_write(isp->otg, ULPI_SET(ULPI_FUNC_CTRL), r);
>> + usleep_range(1000, 2000);
>> +
>> + /* Read the line state */
>> + if (otg_io_read(isp->otg, ULPI_DEBUG)) {
>> + /* Is it a charger or PS/2 connection */
>> +
>> + /* Enable weak pull-up resistor on DP */
>> + otg_io_write(isp->otg, ULPI_SET(ISP1704_PWR_CTRL),
>> + ISP1704_PWR_CTRL_DP_WKPU_EN);
>> +
>> + /* Disable strong pull-up on DP (1.5K) */
>> + otg_io_write(isp->otg, ULPI_CLR(ULPI_FUNC_CTRL),
>> + ULPI_FUNC_CTRL_TERMSELECT);
>> +
>> + /* Enable weak pull-down resistor on DM */
>> + otg_io_write(isp->otg, ULPI_SET(ULPI_OTG_CTRL),
>> + ULPI_OTG_CTRL_DM_PULLDOWN);
>> +
>> + /* It's a charger if the line states are clear */
>> + if (!(otg_io_read(isp->otg, ULPI_DEBUG)))
>> + ret = 1;
>> +
>> + /* Disable weak pull-up resistor on DP */
>> + otg_io_write(isp->otg, ULPI_CLR(ISP1704_PWR_CTRL),
>> + ISP1704_PWR_CTRL_DP_WKPU_EN);
>> + } else {
>> + ret = 1;
>> +
>> + /* Disable strong pull-up on DP (1.5K) */
>> + otg_io_write(isp->otg, ULPI_CLR(ULPI_FUNC_CTRL),
>> + ULPI_FUNC_CTRL_TERMSELECT);
>> + }
>
> How about
>
> if (!otg_io_read(isp->otg, ULPI_DEBUG)) {
> /* Disable strong pull-up on DP (1.5K) */
> otg_io_write(isp->otg, ULPI_CLR(ULPI_FUNC_CTRL),
> ULPI_FUNC_CTRL_TERMSELECT);
> return 1;
> }
>
> <the rest>
>
> Saves indentation level.
Yes, makes sense.
>> + return ret;
>> +}
>> +
>> +static inline int isp1704_charger_detect(struct isp1704_charger *isp)
>> +{
>> + unsigned long timeout;
>> + u8 r;
>> + int vdat;
>> +
>> + /* set SW control bit in PWR_CTRL register */
>> + otg_io_write(isp->otg, ISP1704_PWR_CTRL,
>> + ISP1704_PWR_CTRL_SWCTRL);
>> +
>> + /* enable manual charger detection */
>> + r = (ISP1704_PWR_CTRL_SWCTRL | ISP1704_PWR_CTRL_DPVSRC_EN);
>> + otg_io_write(isp->otg, ULPI_SET(ISP1704_PWR_CTRL), r);
>> + usleep_range(1000, 2000);
>> +
>> + timeout = jiffies + msecs_to_jiffies(300);
>> + while (!time_after(jiffies, timeout)) {
>
> I guess it is possible that vdat might become uninitialized
> if the code never hits the loop. The time between
> timeout = jiffies + msecs_to_jiffies(300);
> and
> while (!time_after(jiffies, timeout)) {
> is undefined.
So do while loop it is.
>> + /* Check if there is a charger */
>> + vdat = !!(otg_io_read(isp->otg, ISP1704_PWR_CTRL)
>> + & ISP1704_PWR_CTRL_VDAT_DET);
>
> Technically, there is no need for "!!".
VDAT_DET is the fifth bit, and since vdat is returned, it would end up
being the value for isp->present. However..
>> + if (vdat)
>> + break;
>> + }
>> + return vdat;
>> +}
>> +
>> +static void isp1704_charger_work(struct work_struct *data)
>> +{
>> + struct isp1704_charger *isp =
>> + container_of(data, struct isp1704_charger, work);
>> +
>> + /* FIXME Only supporting dedicated chargers even though isp1704 can
>> + * detect HUB and HOST chargers. If the device has already been
>> + * enumerated, the detection will break the connection.
>> + */
>> + if (isp->otg->state != OTG_STATE_B_IDLE)
>> + return;
>> +
>> + /* disable data pullups */
>> + if (isp->otg->gadget)
>> + usb_gadget_disconnect(isp->otg->gadget);
>> +
>> + /* detect charger */
>
> shouldn't be there 'isp->present = 0;'?..
>
>> + if (isp1704_charger_detect(isp))
>> + isp->present = isp1704_charger_verify(isp);
>
> i.e. it's kinda weird to see conditional isp->present update...
>
>> +
>> + /* enable data pullups */
>> + if (isp->otg->gadget)
>> + usb_gadget_connect(isp->otg->gadget);
>> +
>> + if (isp->present)
>
> ...and then unconditional checking of isp->present.
>
> I guess the code is OK, and somewhere else the logic handles
> this... but still an odd-looking function.
..I'll call isp1704_charger_verify() from inside
isp1704_charger_detect(), based on the vdat.
>> + power_supply_changed(&isp->psy);
>> +}
>> +
>> +static int isp1707_notifier_call(struct notifier_block *nb,
>> + unsigned long event, void *unused)
>> +{
>> + struct isp1704_charger *isp =
>> + container_of(nb, struct isp1704_charger, nb);
>> +
>> + switch (event) {
>> + case USB_EVENT_VBUS:
>> + schedule_work(&isp->work);
>> + break;
>> + case USB_EVENT_NONE:
>> + isp->present = 0;
>
> Probably this makes the code above work. But why don't we
> call schedule_work(&isp->work)?
USB_EVENT_NONE comes when we loose VBUS (the cable is unplugged). No
need to detect charger in this case.
> [...]
>> +static inline int isp1704_test_ulpi(struct isp1704_charger *isp)
>> +{
>> + int vendor, product, i;
>
> int vendor;
> int product;
> int i;
OK
>> + int ret = -ENODEV;
>> +
>> + /* Test ULPI interface */
>> + ret = otg_io_write(isp->otg, ULPI_SCRATCH, 0xaa);
>> + if (ret < 0)
>> + return ret;
>
> By adding empty line here you'll make the code prettier.
>
>> + ret = otg_io_read(isp->otg, ULPI_SCRATCH);
>> + if (ret < 0)
>> + return ret;
>
> Ditto.
>
>> + if (ret != 0xaa)
>> + return -ENODEV;
>
> Ditto.
>
>> + /* Verify the product and vendor id matches */
>> + vendor = otg_io_read(isp->otg, ULPI_VENDOR_ID_LOW);
>> + vendor |= otg_io_read(isp->otg, ULPI_VENDOR_ID_HIGH) << 8;
>> + if (vendor != NXP_VENDOR_ID)
>> + return -ENODEV;
>
> Ditto.
>
>> + for (i = 0; i < ARRAY_SIZE(isp170x_id); i++) {
>> + product = otg_io_read(isp->otg, ULPI_PRODUCT_ID_LOW);
>> + product |= otg_io_read(isp->otg, ULPI_PRODUCT_ID_HIGH) << 8;
>> + if (product == isp170x_id[i]) {
>> + sprintf(isp->model, "isp%x", product);
>> + return product;
>> + }
>> + }
>> +
>> + dev_err(isp->dev, "product id %x not matching known ids", product);
>> +
>> + return -ENODEV;
>> +}
>> +
>> +static int __devinit isp1704_charger_probe(struct platform_device *pdev)
>> +{
>> + struct isp1704_charger *isp;
>> + int ret = -ENODEV;
>> +
>> + isp = kzalloc(sizeof *isp, GFP_KERNEL);
>> + if (!isp)
>> + return -ENOMEM;
>> +
>> + isp->otg = otg_get_transceiver();
>> + if (!isp->otg) {
>> + kfree(isp);
>> + return ret;
>
> goto failX instead?
OK for this, and the above empty lines.
>> + }
>> +
>> + ret = isp1704_test_ulpi(isp);
>> + if (ret < 0)
>> + goto fail;
>> +
>> + isp->dev = &pdev->dev;
>> + platform_set_drvdata(pdev, isp);
>> +
>> + isp->psy.name = "isp1704";
>> + isp->psy.type = POWER_SUPPLY_TYPE_USB;
>> + isp->psy.properties = power_props;
>> + isp->psy.num_properties = ARRAY_SIZE(power_props);
>> + isp->psy.get_property = isp1704_charger_get_property;
>> +
>> + ret = power_supply_register(isp->dev, &isp->psy);
>> + if (ret)
>> + goto fail;
>> +
>> + /* REVISIT: using work in order to allow the otg notifications to be
>> + * made atomically in the future.
>> + */
>> + INIT_WORK(&isp->work, isp1704_charger_work);
>> +
>> + isp->nb.notifier_call = isp1707_notifier_call;
>
> Add an empty line here?
OK
>> + ret = otg_register_notifier(isp->otg, &isp->nb);
>> + if (ret)
>> + goto fail2;
>> +
>> + dev_info(isp->dev, "registered with product id %s\n", isp->model);
>> +
>> + return 0;
>> +fail2:
>> + power_supply_unregister(&isp->psy);
>> +fail:
>> + otg_put_transceiver(isp->otg);
>> + kfree(isp);
>> +
>> + dev_err(&pdev->dev, "failed to register isp1704 with error %d\n", ret);
>> +
>> + return ret;
>> +}
>[...]
>> +static int __init isp1704_charger_init(void)
>> +{
>> + int ret = 0;
>> +
>> + ret = platform_driver_register(&isp1704_charger_driver);
>> + if (ret)
>> + return ret;
>> +
>> + isp1704_device = platform_device_register_simple("isp1704_charger",
>> + 0, NULL, 0);
>
> This is wrong. Please move this into either isp1704 generic
> or usb driver (if any), or platform (board) code.
>
> That way you also won't need "! MACH_NOKIA_RX51" in the Kconfig.
I'll do this. Thanks for the review. v2 coming up.
--
heikki
Hi,
On Wed, Aug 18, 2010 at 04:55:28PM +0200, Quadros Roger (Nokia-MS/Helsinki) wrote:
> On 08/18/2010 04:01 PM, Krogerus Heikki (EXT-Teleca/Helsinki) wrote:
> > NXP ISP1704 is Battery Charging Specification 1.0 compliant USB
> > transceiver. This adds a power supply driver for ISP1704 and
> > ISP1707 USB transceivers.
<snip>
> > +static inline int isp1704_test_ulpi(struct isp1704_charger *isp)
> > +{
> > + int vendor, product, i;
> > + int ret = -ENODEV;
> > +
> > + /* Test ULPI interface */
> > + ret = otg_io_write(isp->otg, ULPI_SCRATCH, 0xaa);
> > + if (ret< 0)
> > + return ret;
> > + ret = otg_io_read(isp->otg, ULPI_SCRATCH);
> > + if (ret< 0)
> > + return ret;
> > + if (ret != 0xaa)
> > + return -ENODEV;
> > + /* Verify the product and vendor id matches */
> > + vendor = otg_io_read(isp->otg, ULPI_VENDOR_ID_LOW);
> > + vendor |= otg_io_read(isp->otg, ULPI_VENDOR_ID_HIGH)<< 8;
> > + if (vendor != NXP_VENDOR_ID)
> > + return -ENODEV;
> > + for (i = 0; i< ARRAY_SIZE(isp170x_id); i++) {
> > + product = otg_io_read(isp->otg, ULPI_PRODUCT_ID_LOW);
> > + product |= otg_io_read(isp->otg, ULPI_PRODUCT_ID_HIGH)<< 8;
>
> product id should be read outside the for loop. This way you will save
> unnecessary otg_io_reads. product id won't change anyways.
Good point. Fixing.
> > + if (product == isp170x_id[i]) {
> > + sprintf(isp->model, "isp%x", product);
> > + return product;
> > + }
> > + }
> > +
> > + dev_err(isp->dev, "product id %x not matching known ids", product);
> > +
> > + return -ENODEV;
> > +}
>
> <snip>
>
> > +static struct platform_driver isp1704_charger_driver = {
> > + .driver = {
> > + .name = "isp1704_charger",
> > + },
> > + .probe = isp1704_charger_probe,
> > + .remove = __devexit_p(isp1704_charger_remove),
> > +};
> > +
> > +static struct platform_device *isp1704_device;
> > +
> > +static int __init isp1704_charger_init(void)
> > +{
> > + int ret = 0;
> > +
> > + ret = platform_driver_register(&isp1704_charger_driver);
> > + if (ret)
> > + return ret;
> > +
> > + isp1704_device = platform_device_register_simple("isp1704_charger",
> > + 0, NULL, 0);
>
> This platform_device_register should be done in the rx51 board file. i.e.
> board-rx51-peripherals.c
OK. I'll fix this.
Thanks,
--
heikki
> +static void isp1704_charger_work(struct work_struct *data)
> +{
> + struct isp1704_charger *isp =
> + container_of(data, struct isp1704_charger, work);
> +
> + /* FIXME Only supporting dedicated chargers even though isp1704 can
> + * detect HUB and HOST chargers. If the device has already been
> + * enumerated, the detection will break the connection.
> + */
Minor CodingStyle comment (since you're reworking the patch anyway).
Preferred style for multi-line comments is:
/*
* FIXME Only supporting ...
* detect HUB ...
* enumerated ...
*/