Re: [PATCH v4] extcon: Palmas Extcon Driver
Hi,
On Tuesday 07 May 2013 10:51 AM, Kishon Vijay Abraham I wrote:
Hi,
On Tuesday 07 May 2013 06:13 AM, myungjoo.ham wrote:
From: Graeme Gregory
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
Signed-off-by: Moiz Sonasath
Signed-off-by: Ruchika Kharwar
Signed-off-by: Kishon Vijay Abraham I
[kis...@ti.com: adapted palmas usb driver to use the extcon framework]
Signed-off-by: Sebastien Guiriec
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 000..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
+ * Author: Kishon Vijay Abraham I
+ *
+ * Based on twl6030_usb.c
+ *
+ * Author: Hema HK
+ *
+ * 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)
That was not intentional. Took the previous version from the net. My bad.
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+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 longflags;
+intret = -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) {
hmm.. ok.
I think this is to do with disabling interrupts while reporting the VBUS
state. (linkstat is updated in irq handler). So this should be fine I guess?
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] extcon: Palmas Extcon Driver
On Tuesday 14 May 2013 03:24 PM, Graeme Gregory wrote: On Tue, May 14, 2013 at 02:48:53PM +0530, Kishon Vijay Abraham I wrote: On Tuesday 07 May 2013 04:15 PM, Mark Brown wrote: On Tue, May 07, 2013 at 03:17:08PM +0530, Kishon Vijay Abraham I wrote: On Tuesday 07 May 2013 01:28 PM, Mark Brown wrote: The platform can provide it's own vbus control in which case this is not needed. So why is there no interaction with this external VBUS control and how does the driver distinguish between that and an error getting the regulator? The platform specifies if it provides its own VBUS control by the dt property ti,no_control_vbus. So the driver will give an error only when *ti,no_control_vbus* is not set. Graeme? That doesn't answer the question about why there's no interaction with the external control... Graeme? I already partially answered this, it was written for a SoC where VBUS generation was in hardware on the SoC with no information. I would say if nvidia/Laxman do not need this remove it, if someone ever uses it then they can re-add it easy enough! I think we really do not require this. Just we need the notification through extcon about the cable connection. We should remove it for avoiding complexity for now. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] extcon: Palmas Extcon Driver
On Tue, May 14, 2013 at 02:48:53PM +0530, Kishon Vijay Abraham I wrote: > On Tuesday 07 May 2013 04:15 PM, Mark Brown wrote: > >On Tue, May 07, 2013 at 03:17:08PM +0530, Kishon Vijay Abraham I wrote: > >>On Tuesday 07 May 2013 01:28 PM, Mark Brown wrote: > > > The platform can provide it's own vbus control in which case this is > not needed. > > > >>>So why is there no interaction with this external VBUS control and how > >>>does the driver distinguish between that and an error getting the > >>>regulator? > > > >>The platform specifies if it provides its own VBUS control by the dt > >>property ti,no_control_vbus. So the driver will give an error only > >>when *ti,no_control_vbus* is not set. Graeme? > > > >That doesn't answer the question about why there's no interaction with > >the external control... > > > > Graeme? > I already partially answered this, it was written for a SoC where VBUS generation was in hardware on the SoC with no information. I would say if nvidia/Laxman do not need this remove it, if someone ever uses it then they can re-add it easy enough! Graeme -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] extcon: Palmas Extcon Driver
On Tuesday 07 May 2013 04:15 PM, Mark Brown wrote: On Tue, May 07, 2013 at 03:17:08PM +0530, Kishon Vijay Abraham I wrote: On Tuesday 07 May 2013 01:28 PM, Mark Brown wrote: The platform can provide it's own vbus control in which case this is not needed. So why is there no interaction with this external VBUS control and how does the driver distinguish between that and an error getting the regulator? The platform specifies if it provides its own VBUS control by the dt property ti,no_control_vbus. So the driver will give an error only when *ti,no_control_vbus* is not set. Graeme? That doesn't answer the question about why there's no interaction with the external control... Graeme? -Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] extcon: Palmas Extcon Driver
On Tue, May 07, 2013 at 03:17:08PM +0530, Kishon Vijay Abraham I wrote: > On Tuesday 07 May 2013 01:28 PM, Mark Brown wrote: > >>The platform can provide it's own vbus control in which case this is > >>not needed. > >So why is there no interaction with this external VBUS control and how > >does the driver distinguish between that and an error getting the > >regulator? > The platform specifies if it provides its own VBUS control by the dt > property ti,no_control_vbus. So the driver will give an error only > when *ti,no_control_vbus* is not set. Graeme? That doesn't answer the question about why there's no interaction with the external control... signature.asc Description: Digital signature
Re: [PATCH v4] extcon: Palmas Extcon Driver
On 07/05/13 10:47, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 07 May 2013 01:28 PM, Mark Brown wrote:
>> On Tue, May 07, 2013 at 10:42:53AM +0530, Kishon Vijay Abraham I wrote:
>>> On Monday 06 May 2013 08:10 PM, Mark Brown wrote:
On Mon, May 06, 2013 at 06:47:04PM +0530, Kishon Vijay Abraham I
wrote:
> +if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
> +if (palmas_usb->vbus_reg) {
> +ret = regulator_enable(palmas_usb->vbus_reg);
> +if (ret) {
> +dev_dbg(palmas_usb->dev,
> +"regulator enable failed\n");
> +goto ret0;
>>
This is very bad karma, why is the regulator optional?
>>
>>> The platform can provide it's own vbus control in which case this is
>>> not needed.
>>
>> So why is there no interaction with this external VBUS control and how
>> does the driver distinguish between that and an error getting the
>> regulator?
>
> The platform specifies if it provides its own VBUS control by the dt
> property ti,no_control_vbus. So the driver will give an error only
> when *ti,no_control_vbus* is not set. Graeme?
>
> Thanks
> Kishon
That is correct, but as currently there are only two users I guess this
depends on Laxmans usecase. If he doesn't need this extra complexity I
would remove it for now. It was originally done with another SoC in mind!
Graeme
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] extcon: Palmas Extcon Driver
Hi,
On Tuesday 07 May 2013 01:28 PM, Mark Brown wrote:
On Tue, May 07, 2013 at 10:42:53AM +0530, Kishon Vijay Abraham I wrote:
On Monday 06 May 2013 08:10 PM, Mark Brown wrote:
On Mon, May 06, 2013 at 06:47:04PM +0530, Kishon Vijay Abraham I wrote:
+ if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
+ if (palmas_usb->vbus_reg) {
+ ret = regulator_enable(palmas_usb->vbus_reg);
+ if (ret) {
+ dev_dbg(palmas_usb->dev,
+ "regulator enable failed\n");
+ goto ret0;
This is very bad karma, why is the regulator optional?
The platform can provide it's own vbus control in which case this is
not needed.
So why is there no interaction with this external VBUS control and how
does the driver distinguish between that and an error getting the
regulator?
The platform specifies if it provides its own VBUS control by the dt
property ti,no_control_vbus. So the driver will give an error only when
*ti,no_control_vbus* is not set. Graeme?
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] extcon: Palmas Extcon Driver
On Tuesday 07 May 2013 12:35 PM, Chanwoo Choi wrote: On 05/07/2013 03:57 PM, Chanwoo Choi wrote: diff --git a/include/linux/extcon/extcon_palmas.h b/include/linux/extcon/extcon_palmas.h new file mode 100644 index 000..a5119c9 --- /dev/null +++ b/include/linux/extcon/extcon_palmas.h @@ -0,0 +1,26 @@ +/* + * extcon_palmas.h - palmas extcon driver to detect VBUS or ID events + * + * 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: Kishon Vijay Abraham I + * + * 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. + * + */ + +#ifndef __EXTCON_PALMAS_H__ +#define __EXTCON_PALMAS_H__ + +#definePALMAS_USB_STATE_DISCONNECT0x0 +#definePALMAS_USB_STATE_VBUSBIT(0) +#definePALMAS_USB_STATE_IDBIT(1) + The defined variable in extcon_palmas.h is used only on extcon-palmas.c. So, I would like to move definition from extcon_palmas.h to extcon-palmas.c and remove extcon_palmas.h header file. Actually it has to be used in dwc3-omap.c (that was in a different patch). Should detect the state of USB/USB-HOST on dwc3-omap driver? If yes, dwc3-omap driver can immediately detect the changed state of USB/USB-HOST by using excon_register_interest() function which is defined in extcon-class.c I explain simple usage of extcon_register_interest() to receive newly state of USB cable on dwc3-omap driver. --- struct extcon_specific_cable_nb extcon_notifier struct notifier_block extcon_notifier; /* ... */ extcon_notifier.notifier_call = omap_extcon_notifier; ret = extcon_register_interest(&extcon_dev, "USB", &extcon_notifier); Fix usage of extcon_register_interest() as following: ret = extcon_register_interest(&extcon_dev, NULL, "USB", &extcon_notifier); or ret = extcon_register_interest(&extcon_dev, "palmas-usb", "USB", &extcon_notifier); By this we have to define 2 notifiers (one for USB and the other for USB-HOST). I thought of having a single notifier and handling it using states. It was something like this http://pastebin.com/Nv7q9swz. Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] extcon: Palmas Extcon Driver
On Tue, May 07, 2013 at 10:42:53AM +0530, Kishon Vijay Abraham I wrote:
> On Monday 06 May 2013 08:10 PM, Mark Brown wrote:
> >On Mon, May 06, 2013 at 06:47:04PM +0530, Kishon Vijay Abraham I wrote:
> >
> >>+ if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
> >>+ if (palmas_usb->vbus_reg) {
> >>+ ret = regulator_enable(palmas_usb->vbus_reg);
> >>+ if (ret) {
> >>+ dev_dbg(palmas_usb->dev,
> >>+ "regulator enable failed\n");
> >>+ goto ret0;
> >This is very bad karma, why is the regulator optional?
> The platform can provide it's own vbus control in which case this is
> not needed.
So why is there no interaction with this external VBUS control and how
does the driver distinguish between that and an error getting the
regulator?
signature.asc
Description: Digital signature
Re: [PATCH v4] extcon: Palmas Extcon Driver
On 05/07/2013 03:57 PM, Chanwoo Choi wrote:
> diff --git a/include/linux/extcon/extcon_palmas.h
> b/include/linux/extcon/extcon_palmas.h
> new file mode 100644
> index 000..a5119c9
> --- /dev/null
> +++ b/include/linux/extcon/extcon_palmas.h
> @@ -0,0 +1,26 @@
> +/*
> + * extcon_palmas.h - palmas extcon driver to detect VBUS or ID events
> + *
> + * 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: Kishon Vijay Abraham I
> + *
> + * 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.
> + *
> + */
> +
> +#ifndef __EXTCON_PALMAS_H__
> +#define __EXTCON_PALMAS_H__
> +
> +#definePALMAS_USB_STATE_DISCONNECT0x0
> +#definePALMAS_USB_STATE_VBUSBIT(0)
> +#definePALMAS_USB_STATE_IDBIT(1)
> +
>>> The defined variable in extcon_palmas.h is used only on extcon-palmas.c.
>>> So, I would like to move definition from extcon_palmas.h to extcon-palmas.c
>>> and remove extcon_palmas.h header file.
>> Actually it has to be used in dwc3-omap.c (that was in a different patch).
>>
> Should detect the state of USB/USB-HOST on dwc3-omap driver?
>
> If yes, dwc3-omap driver can immediately detect the changed state of
> USB/USB-HOST
> by using excon_register_interest() function which is defined in extcon-class.c
>
> I explain simple usage of extcon_register_interest()
> to receive newly state of USB cable on dwc3-omap driver.
> ---
> struct extcon_specific_cable_nb extcon_notifier
> struct notifier_block extcon_notifier;
>
> /* ... */
>
> extcon_notifier.notifier_call = omap_extcon_notifier;
> ret = extcon_register_interest(&extcon_dev, "USB", &extcon_notifier);
Fix usage of extcon_register_interest() as following:
ret = extcon_register_interest(&extcon_dev, NULL, "USB", &extcon_notifier);
or
ret = extcon_register_interest(&extcon_dev, "palmas-usb", "USB",
&extcon_notifier);
> /* ... */
>
> int omap_extcon_notifier(struct notifier_block *self,
> unsigned long event, void
> *ptr)
> {
> int usb_state;
>
> usb_state = event;
>
> /* if usb_state is 1, PALMAS_USB_STATE_VBUS */
> /* if usb_state is 0, PALMAS_USB_STATE_DISCONNECT */
>
> /* TODO */
>
> }
> ---
>
> If dwc3-omap driver use extcon_register_interest(), following defined
> variables
> are able to be removed.
> PALMAS_USB_STATE_DISCONNECT
> PALMAS_USB_STATE_VBUS
> PALMAS_USB_STATE_ID
>
> Thanks,
> Chanwoo Choi
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] extcon: Palmas Extcon Driver
On 05/07/2013 03:25 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 07 May 2013 11:40 AM, Chanwoo Choi wrote:
>> Hi Kishon,
>>
>> I add some opinion about this patch.
>>
>> On 05/06/2013 10:17 PM, Kishon Vijay Abraham I wrote:
>>> From: Graeme Gregory
>>>
>>> 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
>>> Signed-off-by: Moiz Sonasath
>>> Signed-off-by: Ruchika Kharwar
>>> Signed-off-by: Kishon Vijay Abraham I
>>> [kis...@ti.com: adapted palmas usb driver to use the extcon framework]
>>> Signed-off-by: Sebastien Guiriec
>>> ---
>>> Changes from v3:
>>> * adapted the driver to extcon framework (so moved to drivers/extcon)
>>> * removed palmas_usb_(write/read) and replaced all calls with
>>>palmas_(read/write).
>>> * ignored a checkpatch warning in the line
>>> static const char *palmas_extcon_cable[] = {
>>>as it seemed to be incorrect?
>>> * removed all references to OMAP in this driver.
>>> * couldn't test this driver with mainline as omap5 panda is not booting
>>>with mainline.
>>> * A comment to change to platform_get_irq from regmap is not done as I felt
>>>the data should come from regmap in this case. Graeme?
>>> Changes from v2:
>>> * Moved the driver to drivers/usb/phy/
>>> * Added a bit more explanation in Kconfig
>>> .../devicetree/bindings/extcon/extcon-twl.txt | 17 +
>>> drivers/extcon/Kconfig |7 +
>>> drivers/extcon/Makefile|1 +
>>> drivers/extcon/extcon-palmas.c | 389
>>>
>>> include/linux/extcon/extcon_palmas.h | 26 ++
>>> include/linux/mfd/palmas.h |8 +-
>>> 6 files changed, 447 insertions(+), 1 deletion(-)
>>> create mode 100644 Documentation/devicetree/bindings/extcon/extcon-twl.txt
>>> create mode 100644 drivers/extcon/extcon-palmas.c
>>> create mode 100644 include/linux/extcon/extcon_palmas.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/extcon/extcon-twl.txt
>>> b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
>>> new file mode 100644
>>> index 000..a7f6527
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
>>> @@ -0,0 +1,17 @@
>>> +EXTCON FOR TWL CHIPS
>>> +
>>> +PALMAS USB COMPARATOR
>>> +Required Properties:
>>> + - compatible : Should be "ti,palmas-usb" or "ti,twl6035-usb"
>>> + - vbus-supply : phandle to the regulator device tree node.
>>> +
>>> +Optional Properties:
>>> + - ti,wakeup : To enable the wakeup comparator in probe
>>> + - ti,no_control_vbus: if the platform wishes its own vbus control
>>> +
>>> +palmas-usb {
>>> + compatible = "ti,twl6035-usb", "ti,palmas-usb";
>>> + vbus-supply = <&smps10_reg>;
>>> + ti,wakeup;
>>> +};
>>> +
>>> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
>>> index 5168a13..c881899 100644
>>> --- a/drivers/extcon/Kconfig
>>> +++ b/drivers/extcon/Kconfig
>>> @@ -53,4 +53,11 @@ config EXTCON_ARIZONA
>>> with Wolfson Arizona devices. These are audio CODECs with
>>> advanced audio accessory detection support.
>>>
>>> +config EXTCON_PALMAS
>>> +tristate "Palmas USB EXTCON support"
>>> +depends on MFD_PALMAS
>>> +help
>>> + Say Y here to enable support for USB peripheral and USB host
>>> + detection by palmas usb.
>>> +
>>> endif # MULTISTATE_SWITCH
>>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>>> index f98a3c4..540e2c3 100644
>>> --- a/drivers/extcon/Makefile
>>> +++ b/drivers/extcon/Makefile
>>> @@ -8,3 +8,4 @@ obj-$(CONFIG_EXTCON_ADC_JACK)+= extcon-adc-jack.o
>>> obj-$(CONFIG_EXTCON_MAX77693)+= extcon-max77693.o
>>> obj-$(CONFIG_EXTCON_MAX8997)+= extcon-max8997.o
>>> obj-$(CONFIG_EXTCON_ARIZONA)+= extcon-arizona.o
>>> +obj-$(CONFIG_EXTCON_PALMAS)+= extcon-palmas.o
>>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
>>> new file mode 100644
>>> index 000..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
>>> + * Author: Kishon Vijay Abraham I
>>> + *
>>> + * Based on twl6030_usb.c
>>> + *
>>> + * Author: Hema HK
>>> + *
>>> + * 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 PARTIC
Re: [PATCH v4] extcon: Palmas Extcon Driver
Hi,
On Tuesday 07 May 2013 11:40 AM, Chanwoo Choi wrote:
Hi Kishon,
I add some opinion about this patch.
On 05/06/2013 10:17 PM, Kishon Vijay Abraham I wrote:
From: Graeme Gregory
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
Signed-off-by: Moiz Sonasath
Signed-off-by: Ruchika Kharwar
Signed-off-by: Kishon Vijay Abraham I
[kis...@ti.com: adapted palmas usb driver to use the extcon framework]
Signed-off-by: Sebastien Guiriec
---
Changes from v3:
* adapted the driver to extcon framework (so moved to drivers/extcon)
* removed palmas_usb_(write/read) and replaced all calls with
palmas_(read/write).
* ignored a checkpatch warning in the line
static const char *palmas_extcon_cable[] = {
as it seemed to be incorrect?
* removed all references to OMAP in this driver.
* couldn't test this driver with mainline as omap5 panda is not booting
with mainline.
* A comment to change to platform_get_irq from regmap is not done as I felt
the data should come from regmap in this case. Graeme?
Changes from v2:
* Moved the driver to drivers/usb/phy/
* Added a bit more explanation in Kconfig
.../devicetree/bindings/extcon/extcon-twl.txt | 17 +
drivers/extcon/Kconfig |7 +
drivers/extcon/Makefile|1 +
drivers/extcon/extcon-palmas.c | 389
include/linux/extcon/extcon_palmas.h | 26 ++
include/linux/mfd/palmas.h |8 +-
6 files changed, 447 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/extcon/extcon-twl.txt
create mode 100644 drivers/extcon/extcon-palmas.c
create mode 100644 include/linux/extcon/extcon_palmas.h
diff --git a/Documentation/devicetree/bindings/extcon/extcon-twl.txt
b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
new file mode 100644
index 000..a7f6527
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
@@ -0,0 +1,17 @@
+EXTCON FOR TWL CHIPS
+
+PALMAS USB COMPARATOR
+Required Properties:
+ - compatible : Should be "ti,palmas-usb" or "ti,twl6035-usb"
+ - vbus-supply : phandle to the regulator device tree node.
+
+Optional Properties:
+ - ti,wakeup : To enable the wakeup comparator in probe
+ - ti,no_control_vbus: if the platform wishes its own vbus control
+
+palmas-usb {
+ compatible = "ti,twl6035-usb", "ti,palmas-usb";
+ vbus-supply = <&smps10_reg>;
+ ti,wakeup;
+};
+
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 5168a13..c881899 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -53,4 +53,11 @@ config EXTCON_ARIZONA
with Wolfson Arizona devices. These are audio CODECs with
advanced audio accessory detection support.
+config EXTCON_PALMAS
+ tristate "Palmas USB EXTCON support"
+ depends on MFD_PALMAS
+ help
+ Say Y here to enable support for USB peripheral and USB host
+ detection by palmas usb.
+
endif # MULTISTATE_SWITCH
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index f98a3c4..540e2c3 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o
obj-$(CONFIG_EXTCON_MAX77693) += extcon-max77693.o
obj-$(CONFIG_EXTCON_MAX8997) += extcon-max8997.o
obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o
+obj-$(CONFIG_EXTCON_PALMAS)+= extcon-palmas.o
diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
new file mode 100644
index 000..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
+ * Author: Kishon Vijay Abraham I
+ *
+ * Based on twl6030_usb.c
+ *
+ * Author: Hema HK
+ *
+ * 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.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
Remove unnecessary header file. When I remove following header file,
I completed kernel build without any problem.
linux/init.h
linux/io.h
linux/regulator/consumer.h
linux/notifier.h
linux/slab.h
linux/delay.h
hmm.. ok.
+static const char *palmas_extcon_ca
Re: [PATCH v4] extcon: Palmas Extcon Driver
Hi Kishon,
I add some opinion about this patch.
On 05/06/2013 10:17 PM, Kishon Vijay Abraham I wrote:
> From: Graeme Gregory
>
> 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
> Signed-off-by: Moiz Sonasath
> Signed-off-by: Ruchika Kharwar
> Signed-off-by: Kishon Vijay Abraham I
> [kis...@ti.com: adapted palmas usb driver to use the extcon framework]
> Signed-off-by: Sebastien Guiriec
> ---
> Changes from v3:
> * adapted the driver to extcon framework (so moved to drivers/extcon)
> * removed palmas_usb_(write/read) and replaced all calls with
> palmas_(read/write).
> * ignored a checkpatch warning in the line
> static const char *palmas_extcon_cable[] = {
> as it seemed to be incorrect?
> * removed all references to OMAP in this driver.
> * couldn't test this driver with mainline as omap5 panda is not booting
> with mainline.
> * A comment to change to platform_get_irq from regmap is not done as I felt
> the data should come from regmap in this case. Graeme?
> Changes from v2:
> * Moved the driver to drivers/usb/phy/
> * Added a bit more explanation in Kconfig
> .../devicetree/bindings/extcon/extcon-twl.txt | 17 +
> drivers/extcon/Kconfig |7 +
> drivers/extcon/Makefile|1 +
> drivers/extcon/extcon-palmas.c | 389
>
> include/linux/extcon/extcon_palmas.h | 26 ++
> include/linux/mfd/palmas.h |8 +-
> 6 files changed, 447 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/extcon/extcon-twl.txt
> create mode 100644 drivers/extcon/extcon-palmas.c
> create mode 100644 include/linux/extcon/extcon_palmas.h
>
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-twl.txt
> b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
> new file mode 100644
> index 000..a7f6527
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
> @@ -0,0 +1,17 @@
> +EXTCON FOR TWL CHIPS
> +
> +PALMAS USB COMPARATOR
> +Required Properties:
> + - compatible : Should be "ti,palmas-usb" or "ti,twl6035-usb"
> + - vbus-supply : phandle to the regulator device tree node.
> +
> +Optional Properties:
> + - ti,wakeup : To enable the wakeup comparator in probe
> + - ti,no_control_vbus: if the platform wishes its own vbus control
> +
> +palmas-usb {
> + compatible = "ti,twl6035-usb", "ti,palmas-usb";
> + vbus-supply = <&smps10_reg>;
> + ti,wakeup;
> +};
> +
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 5168a13..c881899 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -53,4 +53,11 @@ config EXTCON_ARIZONA
> with Wolfson Arizona devices. These are audio CODECs with
> advanced audio accessory detection support.
>
> +config EXTCON_PALMAS
> + tristate "Palmas USB EXTCON support"
> + depends on MFD_PALMAS
> + help
> + Say Y here to enable support for USB peripheral and USB host
> + detection by palmas usb.
> +
> endif # MULTISTATE_SWITCH
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index f98a3c4..540e2c3 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o
> obj-$(CONFIG_EXTCON_MAX77693)+= extcon-max77693.o
> obj-$(CONFIG_EXTCON_MAX8997) += extcon-max8997.o
> obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o
> +obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o
> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
> new file mode 100644
> index 000..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
> + * Author: Kishon Vijay Abraham I
> + *
> + * Based on twl6030_usb.c
> + *
> + * Author: Hema HK
> + *
> + * 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.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
Remove unnecessary header file. When I remove following header file,
I completed kernel b
Re: [PATCH v4] extcon: Palmas Extcon Driver
Hi,
On Tuesday 07 May 2013 06:13 AM, myungjoo.ham wrote:
From: Graeme Gregory
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
Signed-off-by: Moiz Sonasath
Signed-off-by: Ruchika Kharwar
Signed-off-by: Kishon Vijay Abraham I
[kis...@ti.com: adapted palmas usb driver to use the extcon framework]
Signed-off-by: Sebastien Guiriec
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 000..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
+ * Author: Kishon Vijay Abraham I
+ *
+ * Based on twl6030_usb.c
+ *
+ * Author: Hema HK
+ *
+ * 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)
That was not intentional. Took the previous version from the net. My bad.
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+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) {
hmm.. ok.
+
+ 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,
PALMA
Re: [PATCH v4] extcon: Palmas Extcon Driver
Hi,
On Monday 06 May 2013 08:10 PM, Mark Brown wrote:
On Mon, May 06, 2013 at 06:47:04PM +0530, Kishon Vijay Abraham I wrote:
+ if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
+ if (palmas_usb->vbus_reg) {
+ ret = regulator_enable(palmas_usb->vbus_reg);
+ if (ret) {
+ dev_dbg(palmas_usb->dev,
+ "regulator enable failed\n");
+ goto ret0;
This is very bad karma, why is the regulator optional?
The platform can provide it's own vbus control in which case this is not
needed.
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] extcon: Palmas Extcon Driver
Hi,
On Monday 06 May 2013 07:56 PM, Laxman Dewangan wrote:
On Monday 06 May 2013 06:47 PM, Kishon Vijay Abraham I wrote:
+
+static irqreturn_t palmas_vbus_wakeup_irq(int irq, void *_palmas_usb)
Can we name the function to palams_vbus_irq_handler() for better
understanding? Reserve the wakeup word for the suspend-wakeups.
+
+
+static irqreturn_t palmas_id_wakeup_irq(int irq, void *_palmas_usb)
Same here for better name.
+
+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;
+ }
This error will keep coming if the vbus is not require as workqueue get
scheduled always. I think we should remove it.
+
+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");
Can we change the variable names to enable_control_bus and logic
accordingly as it looks more appropriate and easy to understand?
+
+ palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
+ PALMAS_ID_OTG_IRQ);
+ palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
+ PALMAS_ID_IRQ);
+ palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
+ PALMAS_VBUS_OTG_IRQ);
+ palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
+ PALMAS_VBUS_IRQ);
+
Better to name irq1, irq2 in more logical names for easy understanding.
+
+ if (device_create_file(&pdev->dev, &dev_attr_vbus))
+ dev_warn(&pdev->dev, "could not create sysfs file\n");
+
+ palmas_usb->edev.name = "palmas_usb";
+ palmas_usb->edev.supported_cable = palmas_extcon_cable;
+ palmas_usb->edev.mutually_exclusive = mutually_exclusive;
+
+ ret = extcon_dev_register(&palmas_usb->edev, palmas_usb->dev);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register extcon
device\n");
+ return ret;
It need to destroy sysfs also.
+ }
+
+ /* init spinlock for workqueue */
+ spin_lock_init(&palmas_usb->lock);
It is already done above.
+
+ INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work);
Better to create the workqueu when control_vbus is require.
+
+
diff --git a/include/linux/extcon/extcon_palmas.h
b/include/linux/extcon/extcon_palmas.h
new file mode 100644
index 000..a5119c9
--- /dev/null
+++ b/include/linux/extcon/extcon_palmas.h
I think it can be use palama.h only. No need to have one more header for
this.
@@ -0,0 +1,26 @@
- u8 linkstat;
+ int mailboxstat;
Do we really require mailboxstat?
Will fix your comments.
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v4] extcon: Palmas Extcon Driver
> From: Graeme Gregory
>
> 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
> Signed-off-by: Moiz Sonasath
> Signed-off-by: Ruchika Kharwar
> Signed-off-by: Kishon Vijay Abraham I
> [kis...@ti.com: adapted palmas usb driver to use the extcon framework]
> Signed-off-by: Sebastien Guiriec
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 000..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
> + * Author: Kishon Vijay Abraham I
> + *
> + * Based on twl6030_usb.c
> + *
> + * Author: Hema HK
> + *
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +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_S
Re: [PATCH v4] extcon: Palmas Extcon Driver
On Mon, May 06, 2013 at 06:47:04PM +0530, Kishon Vijay Abraham I wrote:
> + if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
> + if (palmas_usb->vbus_reg) {
> + ret = regulator_enable(palmas_usb->vbus_reg);
> + if (ret) {
> + dev_dbg(palmas_usb->dev,
> + "regulator enable failed\n");
> + goto ret0;
This is very bad karma, why is the regulator optional?
signature.asc
Description: Digital signature
Re: [PATCH v4] extcon: Palmas Extcon Driver
On Monday 06 May 2013 06:47 PM, Kishon Vijay Abraham I wrote:
+
+static irqreturn_t palmas_vbus_wakeup_irq(int irq, void *_palmas_usb)
Can we name the function to palams_vbus_irq_handler() for better
understanding? Reserve the wakeup word for the suspend-wakeups.
+
+
+static irqreturn_t palmas_id_wakeup_irq(int irq, void *_palmas_usb)
Same here for better name.
+
+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;
+ }
This error will keep coming if the vbus is not require as workqueue get
scheduled always. I think we should remove it.
+
+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");
Can we change the variable names to enable_control_bus and logic
accordingly as it looks more appropriate and easy to understand?
+
+ palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
+ PALMAS_ID_OTG_IRQ);
+ palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
+ PALMAS_ID_IRQ);
+ palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
+ PALMAS_VBUS_OTG_IRQ);
+ palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
+ PALMAS_VBUS_IRQ);
+
Better to name irq1, irq2 in more logical names for easy understanding.
+
+ if (device_create_file(&pdev->dev, &dev_attr_vbus))
+ dev_warn(&pdev->dev, "could not create sysfs file\n");
+
+ palmas_usb->edev.name = "palmas_usb";
+ palmas_usb->edev.supported_cable = palmas_extcon_cable;
+ palmas_usb->edev.mutually_exclusive = mutually_exclusive;
+
+ ret = extcon_dev_register(&palmas_usb->edev, palmas_usb->dev);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register extcon device\n");
+ return ret;
It need to destroy sysfs also.
+ }
+
+ /* init spinlock for workqueue */
+ spin_lock_init(&palmas_usb->lock);
It is already done above.
+
+ INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work);
Better to create the workqueu when control_vbus is require.
+
+
diff --git a/include/linux/extcon/extcon_palmas.h
b/include/linux/extcon/extcon_palmas.h
new file mode 100644
index 000..a5119c9
--- /dev/null
+++ b/include/linux/extcon/extcon_palmas.h
I think it can be use palama.h only. No need to have one more header for
this.
@@ -0,0 +1,26 @@
- u8 linkstat;
+ int mailboxstat;
Do we really require mailboxstat?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
