Re: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
W dniu 08.06.2015 o 19:20, Anand Moon pisze:
> On 8 June 2015 at 10:58, Vivek Gautam wrote:
>> Hi,
>>
(...)
>>
>> On Monday, June 08, 2015 10:44 AM, "Krzysztof Kozlowski"
>> wrote:
>>
>>> Untested code should not go to the kernel. Additionally you should
>>> mark it as not-tested. Marking such patch as non-tested could help you
>>> finding some independent tests (tests performed by someone else).
>>>
>>> To summarize my point of view:
>>> 1. Unless Vivek's says otherwise, please give him the credits with
>>> proper "from" field.
>>> 2. Issues mentioned in previous mail should be addressed (missing
>>> IS_ERR(), how disabling the regulator during suspend affects waking
>>> up).
>>> 3. The patchset must be tested, even after rebasing.
>>
>>
>> Unfortunately, I got busy with a different project and lost track of the
>> patches posted upstream.
>> If it's not too late I can post a rebased version of the patch with previous
>> review comments addressed.
>>
>>>
>>> Best regards,
>>> Krzysztof
>>
>>
>
> Hi All,
>
> I have learned my lesson not to interfere in others work.
> It will never happen from my side again.
>
> Please accept my apology.
Sure, no problem. Mistakes happen (I make a lot of them too), just learn
from them.
Javier explained what was the proper way to do this - retain original
author of the patch. Your signed-off-by was correct.
In fact the kernel SubmittingPatches is worth reading from time to time:
https://www.kernel.org/doc/Documentation/SubmittingPatches
Under chapter 11th ("Sign your work") in paragraph starting with "If you
are a subsystem or branch maintainer" - it explains how to mention
changes when modifying someone's else patch.
You can also look at example from Bartlomiej, how hge did this when he
took Thomas' patches:
https://lkml.org/lkml/2015/4/3/387
However his case is kind of special because he made such a lot of
changes and made such huge effort that in my humble opinion he could
take the authorship of patch. But in this case the changelog is huge.
And he tested it. Extensively. However he did not change the author of
patches :) and you can just look at this work as an example.
Best regards,
Krzysztof
--
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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
Hi, On Monday, June 08, 2015 3:50 PM, "Anand Moon" On 8 June 2015 at 10:58, Vivek Gautam wrote: Hi, On Monday, June 08, 2015 10:44 AM, "Krzysztof Kozlowski" wrote: my apologies for being late in replying to this thread. 2015-06-08 13:21 GMT+09:00 Anand Moon : Hi Krzysztof , On 8 June 2015 at 07:40, Krzysztof Kozlowski wrote: On 07.06.2015 22:20, Anand Moon wrote: Facilitate getting required 3.3V and 1.0V VDD supply for EHCI controller on Exynos. With the patches for regulators' nodes merged in 3.15: c8c253f ARM: dts: Add regulator entries to smdk5420 275dcd2 ARM: dts: add max77686 pmic node for smdk5250, the exynos systems turn on only minimal number of regulators. Until now, the VDD regulator supplies were either turned on by the bootloader, or the regulators were enabled by default in the kernel, so that the controller drivers did not need to care about turning on these regulators on their own. This was rather bad about these controller drivers. So ensuring now that the controller driver requests the necessary VDD regulators (if available, unless there are direct VDD rails), and enable them so as to make them working. Signed-off-by: Vivek Gautam Signed-off-by: Anand Moon Cc: Jingoo Han Cc: Alan Stern --- Initial version of this patch was part of following series, though they are not dependent on each other, resubmitting after rebasing. http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html So you just took Vivek's patch along with all the credits... That is not how we usually do this. I would expect that rebasing a patch won't change the author unless this is fine with Vivek. Sorry If I have done some mistake on my part. I just looked at below mail chain. Before I send it. http://www.spinics.net/lists/linux-samsung-soc/msg44136.html I don't get it. The patch you are referring to has a proper "From" field. So please use it as an example. I don't want to take any credit out of it. I just re-base on the new kernel. Perhaps, you would have maintained the authorship ! I could not test this patch as it meant for exynos5440 boards. Are you sure? I think the driver is used on almost all of Exynos SoCs (Exynos4, Exynos5250, Exynos542x). That's correct, as pointed by Krzysztof Kozlowski, the driver is same for Exynos4 and Exynos5 series of SoCs. Untested code should not go to the kernel. Additionally you should mark it as not-tested. Marking such patch as non-tested could help you finding some independent tests (tests performed by someone else). To summarize my point of view: 1. Unless Vivek's says otherwise, please give him the credits with proper "from" field. 2. Issues mentioned in previous mail should be addressed (missing IS_ERR(), how disabling the regulator during suspend affects waking up). 3. The patchset must be tested, even after rebasing. Unfortunately, I got busy with a different project and lost track of the patches posted upstream. If it's not too late I can post a rebased version of the patch with previous review comments addressed. Best regards, Krzysztof Hi All, I have learned my lesson not to interfere in others work. It will never happen from my side again. Please accept my apology. Please don't apologise. It's just a part of learning; learning how linux mainlining works. Hope you understand. :-) thanks Vivek -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
On 8 June 2015 at 10:58, Vivek Gautam wrote: > Hi, > > > > On Monday, June 08, 2015 10:44 AM, "Krzysztof Kozlowski" > wrote: > > my apologies for being late in replying to this thread. > >> 2015-06-08 13:21 GMT+09:00 Anand Moon : >>> >>> Hi Krzysztof , >>> >>> On 8 June 2015 at 07:40, Krzysztof Kozlowski >>> wrote: On 07.06.2015 22:20, Anand Moon wrote: > > Facilitate getting required 3.3V and 1.0V VDD supply for > EHCI controller on Exynos. > > With the patches for regulators' nodes merged in 3.15: > c8c253f ARM: dts: Add regulator entries to smdk5420 > 275dcd2 ARM: dts: add max77686 pmic node for smdk5250, > the exynos systems turn on only minimal number of regulators. > > Until now, the VDD regulator supplies were either turned on > by the bootloader, or the regulators were enabled by default > in the kernel, so that the controller drivers did not need to > care about turning on these regulators on their own. > This was rather bad about these controller drivers. > So ensuring now that the controller driver requests the necessary > VDD regulators (if available, unless there are direct VDD rails), > and enable them so as to make them working. > > Signed-off-by: Vivek Gautam > Signed-off-by: Anand Moon > Cc: Jingoo Han > Cc: Alan Stern > --- > Initial version of this patch was part of following series, though > they are not dependent on each other, resubmitting after rebasing. > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html So you just took Vivek's patch along with all the credits... That is not how we usually do this. I would expect that rebasing a patch won't change the author unless this is fine with Vivek. >>> >>> Sorry If I have done some mistake on my part. >>> I just looked at below mail chain. Before I send it. >>> >>> http://www.spinics.net/lists/linux-samsung-soc/msg44136.html >> >> >> I don't get it. The patch you are referring to has a proper "From" >> field. So please use it as an example. >> >>> >>> I don't want to take any credit out of it. I just re-base on the new >>> kernel. > > Perhaps, you would have maintained the authorship ! > >>> I could not test this patch as it meant for exynos5440 boards. >> >> >> Are you sure? I think the driver is used on almost all of Exynos SoCs >> (Exynos4, Exynos5250, Exynos542x). > > > That's correct, as pointed by Krzysztof Kozlowski, the driver is same for > Exynos4 and Exynos5 series > of SoCs. > >> Untested code should not go to the kernel. Additionally you should >> mark it as not-tested. Marking such patch as non-tested could help you >> finding some independent tests (tests performed by someone else). >> >> To summarize my point of view: >> 1. Unless Vivek's says otherwise, please give him the credits with >> proper "from" field. >> 2. Issues mentioned in previous mail should be addressed (missing >> IS_ERR(), how disabling the regulator during suspend affects waking >> up). >> 3. The patchset must be tested, even after rebasing. > > > Unfortunately, I got busy with a different project and lost track of the > patches posted upstream. > If it's not too late I can post a rebased version of the patch with previous > review comments addressed. > >> >> Best regards, >> Krzysztof > > Hi All, I have learned my lesson not to interfere in others work. It will never happen from my side again. Please accept my apology. -Anand Moon -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
--
From: "Krzysztof Kozlowski"
Sent: Monday, June 08, 2015 7:40 AM
To: "Anand Moon" ; "Rob Herring"
; "Pawel Moll" ; "Mark Rutland"
; "Ian Campbell" ;
"Kumar Gala" ; "Kukjin Kim" ; "Alan
Stern" ; "Greg Kroah-Hartman"
; "Vivek Gautam" ;
"Felipe Balbi"
Cc: ; ;
; ;
; "Jingoo Han"
Subject: Re: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd
regulators
On 07.06.2015 22:20, Anand Moon wrote:
Facilitate getting required 3.3V and 1.0V VDD supply for
EHCI controller on Exynos.
With the patches for regulators' nodes merged in 3.15:
c8c253f ARM: dts: Add regulator entries to smdk5420
275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
the exynos systems turn on only minimal number of regulators.
Until now, the VDD regulator supplies were either turned on
by the bootloader, or the regulators were enabled by default
in the kernel, so that the controller drivers did not need to
care about turning on these regulators on their own.
This was rather bad about these controller drivers.
So ensuring now that the controller driver requests the necessary
VDD regulators (if available, unless there are direct VDD rails),
and enable them so as to make them working.
Signed-off-by: Vivek Gautam
Signed-off-by: Anand Moon
Cc: Jingoo Han
Cc: Alan Stern
---
Initial version of this patch was part of following series, though
they are not dependent on each other, resubmitting after rebasing.
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html
So you just took Vivek's patch along with all the credits... That is not
how we usually do this.
I would expect that rebasing a patch won't change the author unless this
is fine with Vivek.
---
.../devicetree/bindings/usb/exynos-usb.txt | 2 +
drivers/usb/host/ehci-exynos.c | 55
+-
2 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
b/Documentation/devicetree/bindings/usb/exynos-usb.txt
index 9b4dbe3..776fa03 100644
--- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
+++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
@@ -23,6 +23,8 @@ Required properties:
Optional properties:
- samsung,vbus-gpio: if present, specifies the GPIO that
needs to be pulled up for the bus to be powered.
+ - vdd33-supply: handle to 3.3V Vdd supply regulator for the controller.
+ - vdd10-supply: handle to 1.0V Vdd supply regulator for the controller.
Example:
diff --git a/drivers/usb/host/ehci-exynos.c
b/drivers/usb/host/ehci-exynos.c
index df538fd..4f8f9d2 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
#include
#include
@@ -45,6 +46,8 @@ static struct hc_driver __read_mostly
exynos_ehci_hc_driver;
struct exynos_ehci_hcd {
struct clk *clk;
struct phy *phy[PHY_NUMBER];
+ struct regulator *vdd33;
+ struct regulator *vdd10;
};
#define to_exynos_ehci(hcd) (struct exynos_ehci_hcd
*)(hcd_to_ehci(hcd)->priv)
@@ -170,7 +173,27 @@ static int exynos_ehci_probe(struct platform_device
*pdev)
err = exynos_ehci_get_phy(>dev, exynos_ehci);
if (err)
- goto fail_clk;
+ goto fail_regulator1;
+
+ exynos_ehci->vdd33 = devm_regulator_get_optional(>dev, "vdd33");
+ if (!IS_ERR(exynos_ehci->vdd33)) {
+ err = regulator_enable(exynos_ehci->vdd33);
+ if (err) {
+ dev_err(>dev,
+ "Failed to enable 3.3V Vdd supply\n");
+ goto fail_regulator1;
+ }
+ }
+
+ exynos_ehci->vdd10 = devm_regulator_get_optional(>dev, "vdd10");
+ if (!IS_ERR(exynos_ehci->vdd10)) {
+ err = regulator_enable(exynos_ehci->vdd10);
+ if (err) {
+ dev_err(>dev,
+ "Failed to enable 1.0V Vdd supply\n");
+ goto fail_regulator2;
+ }
+ }
skip_phy:
@@ -231,6 +254,10 @@ fail_add_hcd:
fail_io:
clk_disable_unprepare(exynos_ehci->clk);
fail_clk:
+ regulator_disable(exynos_ehci->vdd10);
+fail_regulator2:
+ regulator_disable(exynos_ehci->vdd33);
if (!IS_ERR()).
+fail_regulator1:
usb_put_hcd(hcd);
return err;
}
@@ -246,6 +273,11 @@ static int exynos_ehci_remove(struct platform_device
*pdev)
clk_disable_unprepare(exynos_ehci->clk);
+ if (!IS_ERR(exynos_ehci->vdd33))
+ regulator_disable(exynos_ehci->vdd33);
+ if (!IS_ERR(exynos_ehci->vdd10))
+ regulator_disable(exynos_ehci->vdd10);
+
usb_put_hcd(hcd);
return 0;
@@ -268,6 +300,11 @@ static int exynos_ehci_suspend(struct device *dev)
clk_disable_unprepare(exynos_ehci->clk);
+ if (!IS_ERR(exynos_ehci->vdd33))
+ regulator_disable(exynos_ehci->vdd33);
+ if (!IS_ERR(exynos_ehci->vdd10))
+ regulator_disable(exynos_ehci->vdd10);
+
Is EHCI a wakeup source? If yes then how disabling regulators during
suspend affects waking
Re: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
Hi, On Monday, June 08, 2015 10:44 AM, "Krzysztof Kozlowski" wrote: my apologies for being late in replying to this thread. 2015-06-08 13:21 GMT+09:00 Anand Moon : Hi Krzysztof , On 8 June 2015 at 07:40, Krzysztof Kozlowski wrote: On 07.06.2015 22:20, Anand Moon wrote: Facilitate getting required 3.3V and 1.0V VDD supply for EHCI controller on Exynos. With the patches for regulators' nodes merged in 3.15: c8c253f ARM: dts: Add regulator entries to smdk5420 275dcd2 ARM: dts: add max77686 pmic node for smdk5250, the exynos systems turn on only minimal number of regulators. Until now, the VDD regulator supplies were either turned on by the bootloader, or the regulators were enabled by default in the kernel, so that the controller drivers did not need to care about turning on these regulators on their own. This was rather bad about these controller drivers. So ensuring now that the controller driver requests the necessary VDD regulators (if available, unless there are direct VDD rails), and enable them so as to make them working. Signed-off-by: Vivek Gautam Signed-off-by: Anand Moon Cc: Jingoo Han Cc: Alan Stern --- Initial version of this patch was part of following series, though they are not dependent on each other, resubmitting after rebasing. http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html So you just took Vivek's patch along with all the credits... That is not how we usually do this. I would expect that rebasing a patch won't change the author unless this is fine with Vivek. Sorry If I have done some mistake on my part. I just looked at below mail chain. Before I send it. http://www.spinics.net/lists/linux-samsung-soc/msg44136.html I don't get it. The patch you are referring to has a proper "From" field. So please use it as an example. I don't want to take any credit out of it. I just re-base on the new kernel. Perhaps, you would have maintained the authorship ! I could not test this patch as it meant for exynos5440 boards. Are you sure? I think the driver is used on almost all of Exynos SoCs (Exynos4, Exynos5250, Exynos542x). That's correct, as pointed by Krzysztof Kozlowski, the driver is same for Exynos4 and Exynos5 series of SoCs. Untested code should not go to the kernel. Additionally you should mark it as not-tested. Marking such patch as non-tested could help you finding some independent tests (tests performed by someone else). To summarize my point of view: 1. Unless Vivek's says otherwise, please give him the credits with proper "from" field. 2. Issues mentioned in previous mail should be addressed (missing IS_ERR(), how disabling the regulator during suspend affects waking up). 3. The patchset must be tested, even after rebasing. Unfortunately, I got busy with a different project and lost track of the patches posted upstream. If it's not too late I can post a rebased version of the patch with previous review comments addressed. Best regards, Krzysztof -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
Hello Krzysztof, On Mon, Jun 8, 2015 at 8:52 AM, Krzysztof Kozlowski wrote: > 2015-06-08 15:42 GMT+09:00 Javier Martinez Canillas : >> Hello, >> >> On Mon, Jun 8, 2015 at 7:14 AM, Krzysztof Kozlowski >> wrote: >> >> [...] >> >>> >>> To summarize my point of view: >>> 1. Unless Vivek's says otherwise, please give him the credits with >>> proper "from" field. >>> 2. Issues mentioned in previous mail should be addressed (missing >>> IS_ERR(), how disabling the regulator during suspend affects waking >>> up). >>> 3. The patchset must be tested, even after rebasing. >>> >> >> Agreed with all the points. >> >> Anand, >> >> An easy way to preserve authorship when rebasing patches is to use the >> git command author option. As an example you can execute the following >> command: >> >> $ git commit -a -s --author='Vivek Gautam ' > > By default "git am" and "git format-patch" preserve the author of a > patch so usually this step is not necessary. Unless the patch is > applied in a different way... :) > That is correct but if an old patch still applies cleanly on top of current's tree, then there is no need to do a rebase right? ;-) I mean, git am is not as smart as the patch command for example to detect when the line numbers mentioned in the patch are incorrect and does not attempt to find the correct place to apply each hunk of the patch (at least by default, I don't know if there is an option). Which IIUC is what Anand had to do so in that case you need to manually commit again but using the original patch author. > Best regards, > Krzysztof Best regards, Javier -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
2015-06-08 15:42 GMT+09:00 Javier Martinez Canillas : > Hello, > > On Mon, Jun 8, 2015 at 7:14 AM, Krzysztof Kozlowski > wrote: > > [...] > >> >> To summarize my point of view: >> 1. Unless Vivek's says otherwise, please give him the credits with >> proper "from" field. >> 2. Issues mentioned in previous mail should be addressed (missing >> IS_ERR(), how disabling the regulator during suspend affects waking >> up). >> 3. The patchset must be tested, even after rebasing. >> > > Agreed with all the points. > > Anand, > > An easy way to preserve authorship when rebasing patches is to use the > git command author option. As an example you can execute the following > command: > > $ git commit -a -s --author='Vivek Gautam ' By default "git am" and "git format-patch" preserve the author of a patch so usually this step is not necessary. Unless the patch is applied in a different way... :) Best regards, Krzysztof -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
Hello, On Mon, Jun 8, 2015 at 7:14 AM, Krzysztof Kozlowski wrote: [...] > > To summarize my point of view: > 1. Unless Vivek's says otherwise, please give him the credits with > proper "from" field. > 2. Issues mentioned in previous mail should be addressed (missing > IS_ERR(), how disabling the regulator during suspend affects waking > up). > 3. The patchset must be tested, even after rebasing. > Agreed with all the points. Anand, An easy way to preserve authorship when rebasing patches is to use the git command author option. As an example you can execute the following command: $ git commit -a -s --author='Vivek Gautam ' > Best regards, > Krzysztof Best regards, Javier -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
Hello, On Mon, Jun 8, 2015 at 7:14 AM, Krzysztof Kozlowski k.kozlow...@samsung.com wrote: [...] To summarize my point of view: 1. Unless Vivek's says otherwise, please give him the credits with proper from field. 2. Issues mentioned in previous mail should be addressed (missing IS_ERR(), how disabling the regulator during suspend affects waking up). 3. The patchset must be tested, even after rebasing. Agreed with all the points. Anand, An easy way to preserve authorship when rebasing patches is to use the git command author option. As an example you can execute the following command: $ git commit -a -s --author='Vivek Gautam gautam.vi...@samsung.com' Best regards, Krzysztof Best regards, Javier -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
Hello Krzysztof, On Mon, Jun 8, 2015 at 8:52 AM, Krzysztof Kozlowski k.kozlow...@samsung.com wrote: 2015-06-08 15:42 GMT+09:00 Javier Martinez Canillas jav...@dowhile0.org: Hello, On Mon, Jun 8, 2015 at 7:14 AM, Krzysztof Kozlowski k.kozlow...@samsung.com wrote: [...] To summarize my point of view: 1. Unless Vivek's says otherwise, please give him the credits with proper from field. 2. Issues mentioned in previous mail should be addressed (missing IS_ERR(), how disabling the regulator during suspend affects waking up). 3. The patchset must be tested, even after rebasing. Agreed with all the points. Anand, An easy way to preserve authorship when rebasing patches is to use the git command author option. As an example you can execute the following command: $ git commit -a -s --author='Vivek Gautam gautam.vi...@samsung.com' By default git am and git format-patch preserve the author of a patch so usually this step is not necessary. Unless the patch is applied in a different way... :) That is correct but if an old patch still applies cleanly on top of current's tree, then there is no need to do a rebase right? ;-) I mean, git am is not as smart as the patch command for example to detect when the line numbers mentioned in the patch are incorrect and does not attempt to find the correct place to apply each hunk of the patch (at least by default, I don't know if there is an option). Which IIUC is what Anand had to do so in that case you need to manually commit again but using the original patch author. Best regards, Krzysztof Best regards, Javier -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
2015-06-08 15:42 GMT+09:00 Javier Martinez Canillas jav...@dowhile0.org: Hello, On Mon, Jun 8, 2015 at 7:14 AM, Krzysztof Kozlowski k.kozlow...@samsung.com wrote: [...] To summarize my point of view: 1. Unless Vivek's says otherwise, please give him the credits with proper from field. 2. Issues mentioned in previous mail should be addressed (missing IS_ERR(), how disabling the regulator during suspend affects waking up). 3. The patchset must be tested, even after rebasing. Agreed with all the points. Anand, An easy way to preserve authorship when rebasing patches is to use the git command author option. As an example you can execute the following command: $ git commit -a -s --author='Vivek Gautam gautam.vi...@samsung.com' By default git am and git format-patch preserve the author of a patch so usually this step is not necessary. Unless the patch is applied in a different way... :) Best regards, Krzysztof -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
Hi, On Monday, June 08, 2015 3:50 PM, Anand Moon linux.am...@gmail.com On 8 June 2015 at 10:58, Vivek Gautam gautam.vi...@samsung.com wrote: Hi, On Monday, June 08, 2015 10:44 AM, Krzysztof Kozlowski k.kozlow...@samsung.com wrote: my apologies for being late in replying to this thread. 2015-06-08 13:21 GMT+09:00 Anand Moon linux.am...@gmail.com: Hi Krzysztof , On 8 June 2015 at 07:40, Krzysztof Kozlowski k.kozlow...@samsung.com wrote: On 07.06.2015 22:20, Anand Moon wrote: Facilitate getting required 3.3V and 1.0V VDD supply for EHCI controller on Exynos. With the patches for regulators' nodes merged in 3.15: c8c253f ARM: dts: Add regulator entries to smdk5420 275dcd2 ARM: dts: add max77686 pmic node for smdk5250, the exynos systems turn on only minimal number of regulators. Until now, the VDD regulator supplies were either turned on by the bootloader, or the regulators were enabled by default in the kernel, so that the controller drivers did not need to care about turning on these regulators on their own. This was rather bad about these controller drivers. So ensuring now that the controller driver requests the necessary VDD regulators (if available, unless there are direct VDD rails), and enable them so as to make them working. Signed-off-by: Vivek Gautam gautam.vi...@samsung.com Signed-off-by: Anand Moon linux.am...@gmail.com Cc: Jingoo Han jg1@samsung.com Cc: Alan Stern st...@rowland.harvard.edu --- Initial version of this patch was part of following series, though they are not dependent on each other, resubmitting after rebasing. http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html So you just took Vivek's patch along with all the credits... That is not how we usually do this. I would expect that rebasing a patch won't change the author unless this is fine with Vivek. Sorry If I have done some mistake on my part. I just looked at below mail chain. Before I send it. http://www.spinics.net/lists/linux-samsung-soc/msg44136.html I don't get it. The patch you are referring to has a proper From field. So please use it as an example. I don't want to take any credit out of it. I just re-base on the new kernel. Perhaps, you would have maintained the authorship ! I could not test this patch as it meant for exynos5440 boards. Are you sure? I think the driver is used on almost all of Exynos SoCs (Exynos4, Exynos5250, Exynos542x). That's correct, as pointed by Krzysztof Kozlowski, the driver is same for Exynos4 and Exynos5 series of SoCs. Untested code should not go to the kernel. Additionally you should mark it as not-tested. Marking such patch as non-tested could help you finding some independent tests (tests performed by someone else). To summarize my point of view: 1. Unless Vivek's says otherwise, please give him the credits with proper from field. 2. Issues mentioned in previous mail should be addressed (missing IS_ERR(), how disabling the regulator during suspend affects waking up). 3. The patchset must be tested, even after rebasing. Unfortunately, I got busy with a different project and lost track of the patches posted upstream. If it's not too late I can post a rebased version of the patch with previous review comments addressed. Best regards, Krzysztof Hi All, I have learned my lesson not to interfere in others work. It will never happen from my side again. Please accept my apology. Please don't apologise. It's just a part of learning; learning how linux mainlining works. Hope you understand. :-) thanks Vivek -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
Hi, On Monday, June 08, 2015 10:44 AM, Krzysztof Kozlowski k.kozlow...@samsung.com wrote: my apologies for being late in replying to this thread. 2015-06-08 13:21 GMT+09:00 Anand Moon linux.am...@gmail.com: Hi Krzysztof , On 8 June 2015 at 07:40, Krzysztof Kozlowski k.kozlow...@samsung.com wrote: On 07.06.2015 22:20, Anand Moon wrote: Facilitate getting required 3.3V and 1.0V VDD supply for EHCI controller on Exynos. With the patches for regulators' nodes merged in 3.15: c8c253f ARM: dts: Add regulator entries to smdk5420 275dcd2 ARM: dts: add max77686 pmic node for smdk5250, the exynos systems turn on only minimal number of regulators. Until now, the VDD regulator supplies were either turned on by the bootloader, or the regulators were enabled by default in the kernel, so that the controller drivers did not need to care about turning on these regulators on their own. This was rather bad about these controller drivers. So ensuring now that the controller driver requests the necessary VDD regulators (if available, unless there are direct VDD rails), and enable them so as to make them working. Signed-off-by: Vivek Gautam gautam.vi...@samsung.com Signed-off-by: Anand Moon linux.am...@gmail.com Cc: Jingoo Han jg1@samsung.com Cc: Alan Stern st...@rowland.harvard.edu --- Initial version of this patch was part of following series, though they are not dependent on each other, resubmitting after rebasing. http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html So you just took Vivek's patch along with all the credits... That is not how we usually do this. I would expect that rebasing a patch won't change the author unless this is fine with Vivek. Sorry If I have done some mistake on my part. I just looked at below mail chain. Before I send it. http://www.spinics.net/lists/linux-samsung-soc/msg44136.html I don't get it. The patch you are referring to has a proper From field. So please use it as an example. I don't want to take any credit out of it. I just re-base on the new kernel. Perhaps, you would have maintained the authorship ! I could not test this patch as it meant for exynos5440 boards. Are you sure? I think the driver is used on almost all of Exynos SoCs (Exynos4, Exynos5250, Exynos542x). That's correct, as pointed by Krzysztof Kozlowski, the driver is same for Exynos4 and Exynos5 series of SoCs. Untested code should not go to the kernel. Additionally you should mark it as not-tested. Marking such patch as non-tested could help you finding some independent tests (tests performed by someone else). To summarize my point of view: 1. Unless Vivek's says otherwise, please give him the credits with proper from field. 2. Issues mentioned in previous mail should be addressed (missing IS_ERR(), how disabling the regulator during suspend affects waking up). 3. The patchset must be tested, even after rebasing. Unfortunately, I got busy with a different project and lost track of the patches posted upstream. If it's not too late I can post a rebased version of the patch with previous review comments addressed. Best regards, Krzysztof -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
--
From: Krzysztof Kozlowski k.kozlow...@samsung.com
Sent: Monday, June 08, 2015 7:40 AM
To: Anand Moon linux.am...@gmail.com; Rob Herring
robh...@kernel.org; Pawel Moll pawel.m...@arm.com; Mark Rutland
mark.rutl...@arm.com; Ian Campbell ijc+devicet...@hellion.org.uk;
Kumar Gala ga...@codeaurora.org; Kukjin Kim kg...@kernel.org; Alan
Stern st...@rowland.harvard.edu; Greg Kroah-Hartman
gre...@linuxfoundation.org; Vivek Gautam gautam.vi...@samsung.com;
Felipe Balbi ba...@ti.com
Cc: devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
linux-samsung-...@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-...@vger.kernel.org; Jingoo Han jg1@samsung.com
Subject: Re: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd
regulators
On 07.06.2015 22:20, Anand Moon wrote:
Facilitate getting required 3.3V and 1.0V VDD supply for
EHCI controller on Exynos.
With the patches for regulators' nodes merged in 3.15:
c8c253f ARM: dts: Add regulator entries to smdk5420
275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
the exynos systems turn on only minimal number of regulators.
Until now, the VDD regulator supplies were either turned on
by the bootloader, or the regulators were enabled by default
in the kernel, so that the controller drivers did not need to
care about turning on these regulators on their own.
This was rather bad about these controller drivers.
So ensuring now that the controller driver requests the necessary
VDD regulators (if available, unless there are direct VDD rails),
and enable them so as to make them working.
Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
Signed-off-by: Anand Moon linux.am...@gmail.com
Cc: Jingoo Han jg1@samsung.com
Cc: Alan Stern st...@rowland.harvard.edu
---
Initial version of this patch was part of following series, though
they are not dependent on each other, resubmitting after rebasing.
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html
So you just took Vivek's patch along with all the credits... That is not
how we usually do this.
I would expect that rebasing a patch won't change the author unless this
is fine with Vivek.
---
.../devicetree/bindings/usb/exynos-usb.txt | 2 +
drivers/usb/host/ehci-exynos.c | 55
+-
2 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
b/Documentation/devicetree/bindings/usb/exynos-usb.txt
index 9b4dbe3..776fa03 100644
--- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
+++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
@@ -23,6 +23,8 @@ Required properties:
Optional properties:
- samsung,vbus-gpio: if present, specifies the GPIO that
needs to be pulled up for the bus to be powered.
+ - vdd33-supply: handle to 3.3V Vdd supply regulator for the controller.
+ - vdd10-supply: handle to 1.0V Vdd supply regulator for the controller.
Example:
diff --git a/drivers/usb/host/ehci-exynos.c
b/drivers/usb/host/ehci-exynos.c
index df538fd..4f8f9d2 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -21,6 +21,7 @@
#include linux/of_gpio.h
#include linux/phy/phy.h
#include linux/platform_device.h
+#include linux/regulator/consumer.h
#include linux/usb.h
#include linux/usb/hcd.h
@@ -45,6 +46,8 @@ static struct hc_driver __read_mostly
exynos_ehci_hc_driver;
struct exynos_ehci_hcd {
struct clk *clk;
struct phy *phy[PHY_NUMBER];
+ struct regulator *vdd33;
+ struct regulator *vdd10;
};
#define to_exynos_ehci(hcd) (struct exynos_ehci_hcd
*)(hcd_to_ehci(hcd)-priv)
@@ -170,7 +173,27 @@ static int exynos_ehci_probe(struct platform_device
*pdev)
err = exynos_ehci_get_phy(pdev-dev, exynos_ehci);
if (err)
- goto fail_clk;
+ goto fail_regulator1;
+
+ exynos_ehci-vdd33 = devm_regulator_get_optional(pdev-dev, vdd33);
+ if (!IS_ERR(exynos_ehci-vdd33)) {
+ err = regulator_enable(exynos_ehci-vdd33);
+ if (err) {
+ dev_err(pdev-dev,
+ Failed to enable 3.3V Vdd supply\n);
+ goto fail_regulator1;
+ }
+ }
+
+ exynos_ehci-vdd10 = devm_regulator_get_optional(pdev-dev, vdd10);
+ if (!IS_ERR(exynos_ehci-vdd10)) {
+ err = regulator_enable(exynos_ehci-vdd10);
+ if (err) {
+ dev_err(pdev-dev,
+ Failed to enable 1.0V Vdd supply\n);
+ goto fail_regulator2;
+ }
+ }
skip_phy:
@@ -231,6 +254,10 @@ fail_add_hcd:
fail_io:
clk_disable_unprepare(exynos_ehci-clk);
fail_clk:
+ regulator_disable(exynos_ehci-vdd10);
+fail_regulator2:
+ regulator_disable(exynos_ehci-vdd33);
if (!IS_ERR()).
+fail_regulator1:
usb_put_hcd(hcd);
return err;
}
@@ -246,6 +273,11 @@ static int exynos_ehci_remove(struct platform_device
*pdev)
clk_disable_unprepare(exynos_ehci-clk);
+ if (!IS_ERR(exynos_ehci-vdd33))
+ regulator_disable(exynos_ehci-vdd33);
+ if (!IS_ERR(exynos_ehci-vdd10))
+ regulator_disable(exynos_ehci-vdd10);
+
usb_put_hcd(hcd);
return 0;
@@ -268,6 +300,11 @@ static
Re: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
On 8 June 2015 at 10:58, Vivek Gautam gautam.vi...@samsung.com wrote: Hi, On Monday, June 08, 2015 10:44 AM, Krzysztof Kozlowski k.kozlow...@samsung.com wrote: my apologies for being late in replying to this thread. 2015-06-08 13:21 GMT+09:00 Anand Moon linux.am...@gmail.com: Hi Krzysztof , On 8 June 2015 at 07:40, Krzysztof Kozlowski k.kozlow...@samsung.com wrote: On 07.06.2015 22:20, Anand Moon wrote: Facilitate getting required 3.3V and 1.0V VDD supply for EHCI controller on Exynos. With the patches for regulators' nodes merged in 3.15: c8c253f ARM: dts: Add regulator entries to smdk5420 275dcd2 ARM: dts: add max77686 pmic node for smdk5250, the exynos systems turn on only minimal number of regulators. Until now, the VDD regulator supplies were either turned on by the bootloader, or the regulators were enabled by default in the kernel, so that the controller drivers did not need to care about turning on these regulators on their own. This was rather bad about these controller drivers. So ensuring now that the controller driver requests the necessary VDD regulators (if available, unless there are direct VDD rails), and enable them so as to make them working. Signed-off-by: Vivek Gautam gautam.vi...@samsung.com Signed-off-by: Anand Moon linux.am...@gmail.com Cc: Jingoo Han jg1@samsung.com Cc: Alan Stern st...@rowland.harvard.edu --- Initial version of this patch was part of following series, though they are not dependent on each other, resubmitting after rebasing. http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html So you just took Vivek's patch along with all the credits... That is not how we usually do this. I would expect that rebasing a patch won't change the author unless this is fine with Vivek. Sorry If I have done some mistake on my part. I just looked at below mail chain. Before I send it. http://www.spinics.net/lists/linux-samsung-soc/msg44136.html I don't get it. The patch you are referring to has a proper From field. So please use it as an example. I don't want to take any credit out of it. I just re-base on the new kernel. Perhaps, you would have maintained the authorship ! I could not test this patch as it meant for exynos5440 boards. Are you sure? I think the driver is used on almost all of Exynos SoCs (Exynos4, Exynos5250, Exynos542x). That's correct, as pointed by Krzysztof Kozlowski, the driver is same for Exynos4 and Exynos5 series of SoCs. Untested code should not go to the kernel. Additionally you should mark it as not-tested. Marking such patch as non-tested could help you finding some independent tests (tests performed by someone else). To summarize my point of view: 1. Unless Vivek's says otherwise, please give him the credits with proper from field. 2. Issues mentioned in previous mail should be addressed (missing IS_ERR(), how disabling the regulator during suspend affects waking up). 3. The patchset must be tested, even after rebasing. Unfortunately, I got busy with a different project and lost track of the patches posted upstream. If it's not too late I can post a rebased version of the patch with previous review comments addressed. Best regards, Krzysztof Hi All, I have learned my lesson not to interfere in others work. It will never happen from my side again. Please accept my apology. -Anand Moon -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
W dniu 08.06.2015 o 19:20, Anand Moon pisze: On 8 June 2015 at 10:58, Vivek Gautam gautam.vi...@samsung.com wrote: Hi, (...) On Monday, June 08, 2015 10:44 AM, Krzysztof Kozlowski k.kozlow...@samsung.com wrote: Untested code should not go to the kernel. Additionally you should mark it as not-tested. Marking such patch as non-tested could help you finding some independent tests (tests performed by someone else). To summarize my point of view: 1. Unless Vivek's says otherwise, please give him the credits with proper from field. 2. Issues mentioned in previous mail should be addressed (missing IS_ERR(), how disabling the regulator during suspend affects waking up). 3. The patchset must be tested, even after rebasing. Unfortunately, I got busy with a different project and lost track of the patches posted upstream. If it's not too late I can post a rebased version of the patch with previous review comments addressed. Best regards, Krzysztof Hi All, I have learned my lesson not to interfere in others work. It will never happen from my side again. Please accept my apology. Sure, no problem. Mistakes happen (I make a lot of them too), just learn from them. Javier explained what was the proper way to do this - retain original author of the patch. Your signed-off-by was correct. In fact the kernel SubmittingPatches is worth reading from time to time: https://www.kernel.org/doc/Documentation/SubmittingPatches Under chapter 11th (Sign your work) in paragraph starting with If you are a subsystem or branch maintainer - it explains how to mention changes when modifying someone's else patch. You can also look at example from Bartlomiej, how hge did this when he took Thomas' patches: https://lkml.org/lkml/2015/4/3/387 However his case is kind of special because he made such a lot of changes and made such huge effort that in my humble opinion he could take the authorship of patch. But in this case the changelog is huge. And he tested it. Extensively. However he did not change the author of patches :) and you can just look at this work as an example. Best regards, Krzysztof -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
2015-06-08 13:21 GMT+09:00 Anand Moon : > Hi Krzysztof , > > On 8 June 2015 at 07:40, Krzysztof Kozlowski wrote: >> On 07.06.2015 22:20, Anand Moon wrote: >>> Facilitate getting required 3.3V and 1.0V VDD supply for >>> EHCI controller on Exynos. >>> >>> With the patches for regulators' nodes merged in 3.15: >>> c8c253f ARM: dts: Add regulator entries to smdk5420 >>> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250, >>> the exynos systems turn on only minimal number of regulators. >>> >>> Until now, the VDD regulator supplies were either turned on >>> by the bootloader, or the regulators were enabled by default >>> in the kernel, so that the controller drivers did not need to >>> care about turning on these regulators on their own. >>> This was rather bad about these controller drivers. >>> So ensuring now that the controller driver requests the necessary >>> VDD regulators (if available, unless there are direct VDD rails), >>> and enable them so as to make them working. >>> >>> Signed-off-by: Vivek Gautam >>> Signed-off-by: Anand Moon >>> Cc: Jingoo Han >>> Cc: Alan Stern >>> --- >>> Initial version of this patch was part of following series, though >>> they are not dependent on each other, resubmitting after rebasing. >>> >>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html >> >> So you just took Vivek's patch along with all the credits... That is not >> how we usually do this. >> >> I would expect that rebasing a patch won't change the author unless this >> is fine with Vivek. >> > > Sorry If I have done some mistake on my part. > I just looked at below mail chain. Before I send it. > > http://www.spinics.net/lists/linux-samsung-soc/msg44136.html I don't get it. The patch you are referring to has a proper "From" field. So please use it as an example. > > I don't want to take any credit out of it. I just re-base on the new kernel. > I could not test this patch as it meant for exynos5440 boards. Are you sure? I think the driver is used on almost all of Exynos SoCs (Exynos4, Exynos5250, Exynos542x). Untested code should not go to the kernel. Additionally you should mark it as not-tested. Marking such patch as non-tested could help you finding some independent tests (tests performed by someone else). To summarize my point of view: 1. Unless Vivek's says otherwise, please give him the credits with proper "from" field. 2. Issues mentioned in previous mail should be addressed (missing IS_ERR(), how disabling the regulator during suspend affects waking up). 3. The patchset must be tested, even after rebasing. Best regards, Krzysztof -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
Hi Krzysztof ,
On 8 June 2015 at 07:40, Krzysztof Kozlowski wrote:
> On 07.06.2015 22:20, Anand Moon wrote:
>> Facilitate getting required 3.3V and 1.0V VDD supply for
>> EHCI controller on Exynos.
>>
>> With the patches for regulators' nodes merged in 3.15:
>> c8c253f ARM: dts: Add regulator entries to smdk5420
>> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
>> the exynos systems turn on only minimal number of regulators.
>>
>> Until now, the VDD regulator supplies were either turned on
>> by the bootloader, or the regulators were enabled by default
>> in the kernel, so that the controller drivers did not need to
>> care about turning on these regulators on their own.
>> This was rather bad about these controller drivers.
>> So ensuring now that the controller driver requests the necessary
>> VDD regulators (if available, unless there are direct VDD rails),
>> and enable them so as to make them working.
>>
>> Signed-off-by: Vivek Gautam
>> Signed-off-by: Anand Moon
>> Cc: Jingoo Han
>> Cc: Alan Stern
>> ---
>> Initial version of this patch was part of following series, though
>> they are not dependent on each other, resubmitting after rebasing.
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html
>
> So you just took Vivek's patch along with all the credits... That is not
> how we usually do this.
>
> I would expect that rebasing a patch won't change the author unless this
> is fine with Vivek.
>
Sorry If I have done some mistake on my part.
I just looked at below mail chain. Before I send it.
http://www.spinics.net/lists/linux-samsung-soc/msg44136.html
I don't want to take any credit out of it. I just re-base on the new kernel.
I could not test this patch as it meant for exynos5440 boards.
-Anand Moon
>> ---
>> .../devicetree/bindings/usb/exynos-usb.txt | 2 +
>> drivers/usb/host/ehci-exynos.c | 55
>> +-
>> 2 files changed, 56 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> index 9b4dbe3..776fa03 100644
>> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> @@ -23,6 +23,8 @@ Required properties:
>> Optional properties:
>> - samsung,vbus-gpio: if present, specifies the GPIO that
>> needs to be pulled up for the bus to be powered.
>> + - vdd33-supply: handle to 3.3V Vdd supply regulator for the controller.
>> + - vdd10-supply: handle to 1.0V Vdd supply regulator for the controller.
>>
>> Example:
>>
>> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
>> index df538fd..4f8f9d2 100644
>> --- a/drivers/usb/host/ehci-exynos.c
>> +++ b/drivers/usb/host/ehci-exynos.c
>> @@ -21,6 +21,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -45,6 +46,8 @@ static struct hc_driver __read_mostly
>> exynos_ehci_hc_driver;
>> struct exynos_ehci_hcd {
>> struct clk *clk;
>> struct phy *phy[PHY_NUMBER];
>> + struct regulator *vdd33;
>> + struct regulator *vdd10;
>> };
>>
>> #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd
>> *)(hcd_to_ehci(hcd)->priv)
>> @@ -170,7 +173,27 @@ static int exynos_ehci_probe(struct platform_device
>> *pdev)
>>
>> err = exynos_ehci_get_phy(>dev, exynos_ehci);
>> if (err)
>> - goto fail_clk;
>> + goto fail_regulator1;
>> +
>> + exynos_ehci->vdd33 = devm_regulator_get_optional(>dev, "vdd33");
>> + if (!IS_ERR(exynos_ehci->vdd33)) {
>> + err = regulator_enable(exynos_ehci->vdd33);
>> + if (err) {
>> + dev_err(>dev,
>> + "Failed to enable 3.3V Vdd supply\n");
>> + goto fail_regulator1;
>> + }
>> + }
>> +
>> + exynos_ehci->vdd10 = devm_regulator_get_optional(>dev, "vdd10");
>> + if (!IS_ERR(exynos_ehci->vdd10)) {
>> + err = regulator_enable(exynos_ehci->vdd10);
>> + if (err) {
>> + dev_err(>dev,
>> + "Failed to enable 1.0V Vdd supply\n");
>> + goto fail_regulator2;
>> + }
>> + }
>>
>> skip_phy:
>>
>> @@ -231,6 +254,10 @@ fail_add_hcd:
>> fail_io:
>> clk_disable_unprepare(exynos_ehci->clk);
>> fail_clk:
>> + regulator_disable(exynos_ehci->vdd10);
>> +fail_regulator2:
>> + regulator_disable(exynos_ehci->vdd33);
>
> if (!IS_ERR()).
>
>> +fail_regulator1:
>> usb_put_hcd(hcd);
>> return err;
>> }
>> @@ -246,6 +273,11 @@ static int exynos_ehci_remove(struct platform_device
>> *pdev)
>>
>> clk_disable_unprepare(exynos_ehci->clk);
>>
>> + if (!IS_ERR(exynos_ehci->vdd33))
>> + regulator_disable(exynos_ehci->vdd33);
>> + if (!IS_ERR(exynos_ehci->vdd10))
>> +
Re: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
On 07.06.2015 22:20, Anand Moon wrote:
> Facilitate getting required 3.3V and 1.0V VDD supply for
> EHCI controller on Exynos.
>
> With the patches for regulators' nodes merged in 3.15:
> c8c253f ARM: dts: Add regulator entries to smdk5420
> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
> the exynos systems turn on only minimal number of regulators.
>
> Until now, the VDD regulator supplies were either turned on
> by the bootloader, or the regulators were enabled by default
> in the kernel, so that the controller drivers did not need to
> care about turning on these regulators on their own.
> This was rather bad about these controller drivers.
> So ensuring now that the controller driver requests the necessary
> VDD regulators (if available, unless there are direct VDD rails),
> and enable them so as to make them working.
>
> Signed-off-by: Vivek Gautam
> Signed-off-by: Anand Moon
> Cc: Jingoo Han
> Cc: Alan Stern
> ---
> Initial version of this patch was part of following series, though
> they are not dependent on each other, resubmitting after rebasing.
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html
So you just took Vivek's patch along with all the credits... That is not
how we usually do this.
I would expect that rebasing a patch won't change the author unless this
is fine with Vivek.
> ---
> .../devicetree/bindings/usb/exynos-usb.txt | 2 +
> drivers/usb/host/ehci-exynos.c | 55
> +-
> 2 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
> b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> index 9b4dbe3..776fa03 100644
> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> @@ -23,6 +23,8 @@ Required properties:
> Optional properties:
> - samsung,vbus-gpio: if present, specifies the GPIO that
> needs to be pulled up for the bus to be powered.
> + - vdd33-supply: handle to 3.3V Vdd supply regulator for the controller.
> + - vdd10-supply: handle to 1.0V Vdd supply regulator for the controller.
>
> Example:
>
> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
> index df538fd..4f8f9d2 100644
> --- a/drivers/usb/host/ehci-exynos.c
> +++ b/drivers/usb/host/ehci-exynos.c
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -45,6 +46,8 @@ static struct hc_driver __read_mostly exynos_ehci_hc_driver;
> struct exynos_ehci_hcd {
> struct clk *clk;
> struct phy *phy[PHY_NUMBER];
> + struct regulator *vdd33;
> + struct regulator *vdd10;
> };
>
> #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd
> *)(hcd_to_ehci(hcd)->priv)
> @@ -170,7 +173,27 @@ static int exynos_ehci_probe(struct platform_device
> *pdev)
>
> err = exynos_ehci_get_phy(>dev, exynos_ehci);
> if (err)
> - goto fail_clk;
> + goto fail_regulator1;
> +
> + exynos_ehci->vdd33 = devm_regulator_get_optional(>dev, "vdd33");
> + if (!IS_ERR(exynos_ehci->vdd33)) {
> + err = regulator_enable(exynos_ehci->vdd33);
> + if (err) {
> + dev_err(>dev,
> + "Failed to enable 3.3V Vdd supply\n");
> + goto fail_regulator1;
> + }
> + }
> +
> + exynos_ehci->vdd10 = devm_regulator_get_optional(>dev, "vdd10");
> + if (!IS_ERR(exynos_ehci->vdd10)) {
> + err = regulator_enable(exynos_ehci->vdd10);
> + if (err) {
> + dev_err(>dev,
> + "Failed to enable 1.0V Vdd supply\n");
> + goto fail_regulator2;
> + }
> + }
>
> skip_phy:
>
> @@ -231,6 +254,10 @@ fail_add_hcd:
> fail_io:
> clk_disable_unprepare(exynos_ehci->clk);
> fail_clk:
> + regulator_disable(exynos_ehci->vdd10);
> +fail_regulator2:
> + regulator_disable(exynos_ehci->vdd33);
if (!IS_ERR()).
> +fail_regulator1:
> usb_put_hcd(hcd);
> return err;
> }
> @@ -246,6 +273,11 @@ static int exynos_ehci_remove(struct platform_device
> *pdev)
>
> clk_disable_unprepare(exynos_ehci->clk);
>
> + if (!IS_ERR(exynos_ehci->vdd33))
> + regulator_disable(exynos_ehci->vdd33);
> + if (!IS_ERR(exynos_ehci->vdd10))
> + regulator_disable(exynos_ehci->vdd10);
> +
> usb_put_hcd(hcd);
>
> return 0;
> @@ -268,6 +300,11 @@ static int exynos_ehci_suspend(struct device *dev)
>
> clk_disable_unprepare(exynos_ehci->clk);
>
> + if (!IS_ERR(exynos_ehci->vdd33))
> + regulator_disable(exynos_ehci->vdd33);
> + if (!IS_ERR(exynos_ehci->vdd10))
> + regulator_disable(exynos_ehci->vdd10);
> +
Is EHCI a wakeup source? If yes then how disabling regulators during
suspend affects waking up process?
Best
Re: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
2015-06-08 13:21 GMT+09:00 Anand Moon linux.am...@gmail.com: Hi Krzysztof , On 8 June 2015 at 07:40, Krzysztof Kozlowski k.kozlow...@samsung.com wrote: On 07.06.2015 22:20, Anand Moon wrote: Facilitate getting required 3.3V and 1.0V VDD supply for EHCI controller on Exynos. With the patches for regulators' nodes merged in 3.15: c8c253f ARM: dts: Add regulator entries to smdk5420 275dcd2 ARM: dts: add max77686 pmic node for smdk5250, the exynos systems turn on only minimal number of regulators. Until now, the VDD regulator supplies were either turned on by the bootloader, or the regulators were enabled by default in the kernel, so that the controller drivers did not need to care about turning on these regulators on their own. This was rather bad about these controller drivers. So ensuring now that the controller driver requests the necessary VDD regulators (if available, unless there are direct VDD rails), and enable them so as to make them working. Signed-off-by: Vivek Gautam gautam.vi...@samsung.com Signed-off-by: Anand Moon linux.am...@gmail.com Cc: Jingoo Han jg1@samsung.com Cc: Alan Stern st...@rowland.harvard.edu --- Initial version of this patch was part of following series, though they are not dependent on each other, resubmitting after rebasing. http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html So you just took Vivek's patch along with all the credits... That is not how we usually do this. I would expect that rebasing a patch won't change the author unless this is fine with Vivek. Sorry If I have done some mistake on my part. I just looked at below mail chain. Before I send it. http://www.spinics.net/lists/linux-samsung-soc/msg44136.html I don't get it. The patch you are referring to has a proper From field. So please use it as an example. I don't want to take any credit out of it. I just re-base on the new kernel. I could not test this patch as it meant for exynos5440 boards. Are you sure? I think the driver is used on almost all of Exynos SoCs (Exynos4, Exynos5250, Exynos542x). Untested code should not go to the kernel. Additionally you should mark it as not-tested. Marking such patch as non-tested could help you finding some independent tests (tests performed by someone else). To summarize my point of view: 1. Unless Vivek's says otherwise, please give him the credits with proper from field. 2. Issues mentioned in previous mail should be addressed (missing IS_ERR(), how disabling the regulator during suspend affects waking up). 3. The patchset must be tested, even after rebasing. Best regards, Krzysztof -- 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: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
Hi Krzysztof ,
On 8 June 2015 at 07:40, Krzysztof Kozlowski k.kozlow...@samsung.com wrote:
On 07.06.2015 22:20, Anand Moon wrote:
Facilitate getting required 3.3V and 1.0V VDD supply for
EHCI controller on Exynos.
With the patches for regulators' nodes merged in 3.15:
c8c253f ARM: dts: Add regulator entries to smdk5420
275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
the exynos systems turn on only minimal number of regulators.
Until now, the VDD regulator supplies were either turned on
by the bootloader, or the regulators were enabled by default
in the kernel, so that the controller drivers did not need to
care about turning on these regulators on their own.
This was rather bad about these controller drivers.
So ensuring now that the controller driver requests the necessary
VDD regulators (if available, unless there are direct VDD rails),
and enable them so as to make them working.
Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
Signed-off-by: Anand Moon linux.am...@gmail.com
Cc: Jingoo Han jg1@samsung.com
Cc: Alan Stern st...@rowland.harvard.edu
---
Initial version of this patch was part of following series, though
they are not dependent on each other, resubmitting after rebasing.
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html
So you just took Vivek's patch along with all the credits... That is not
how we usually do this.
I would expect that rebasing a patch won't change the author unless this
is fine with Vivek.
Sorry If I have done some mistake on my part.
I just looked at below mail chain. Before I send it.
http://www.spinics.net/lists/linux-samsung-soc/msg44136.html
I don't want to take any credit out of it. I just re-base on the new kernel.
I could not test this patch as it meant for exynos5440 boards.
-Anand Moon
---
.../devicetree/bindings/usb/exynos-usb.txt | 2 +
drivers/usb/host/ehci-exynos.c | 55
+-
2 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
b/Documentation/devicetree/bindings/usb/exynos-usb.txt
index 9b4dbe3..776fa03 100644
--- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
+++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
@@ -23,6 +23,8 @@ Required properties:
Optional properties:
- samsung,vbus-gpio: if present, specifies the GPIO that
needs to be pulled up for the bus to be powered.
+ - vdd33-supply: handle to 3.3V Vdd supply regulator for the controller.
+ - vdd10-supply: handle to 1.0V Vdd supply regulator for the controller.
Example:
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
index df538fd..4f8f9d2 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -21,6 +21,7 @@
#include linux/of_gpio.h
#include linux/phy/phy.h
#include linux/platform_device.h
+#include linux/regulator/consumer.h
#include linux/usb.h
#include linux/usb/hcd.h
@@ -45,6 +46,8 @@ static struct hc_driver __read_mostly
exynos_ehci_hc_driver;
struct exynos_ehci_hcd {
struct clk *clk;
struct phy *phy[PHY_NUMBER];
+ struct regulator *vdd33;
+ struct regulator *vdd10;
};
#define to_exynos_ehci(hcd) (struct exynos_ehci_hcd
*)(hcd_to_ehci(hcd)-priv)
@@ -170,7 +173,27 @@ static int exynos_ehci_probe(struct platform_device
*pdev)
err = exynos_ehci_get_phy(pdev-dev, exynos_ehci);
if (err)
- goto fail_clk;
+ goto fail_regulator1;
+
+ exynos_ehci-vdd33 = devm_regulator_get_optional(pdev-dev, vdd33);
+ if (!IS_ERR(exynos_ehci-vdd33)) {
+ err = regulator_enable(exynos_ehci-vdd33);
+ if (err) {
+ dev_err(pdev-dev,
+ Failed to enable 3.3V Vdd supply\n);
+ goto fail_regulator1;
+ }
+ }
+
+ exynos_ehci-vdd10 = devm_regulator_get_optional(pdev-dev, vdd10);
+ if (!IS_ERR(exynos_ehci-vdd10)) {
+ err = regulator_enable(exynos_ehci-vdd10);
+ if (err) {
+ dev_err(pdev-dev,
+ Failed to enable 1.0V Vdd supply\n);
+ goto fail_regulator2;
+ }
+ }
skip_phy:
@@ -231,6 +254,10 @@ fail_add_hcd:
fail_io:
clk_disable_unprepare(exynos_ehci-clk);
fail_clk:
+ regulator_disable(exynos_ehci-vdd10);
+fail_regulator2:
+ regulator_disable(exynos_ehci-vdd33);
if (!IS_ERR()).
+fail_regulator1:
usb_put_hcd(hcd);
return err;
}
@@ -246,6 +273,11 @@ static int exynos_ehci_remove(struct platform_device
*pdev)
clk_disable_unprepare(exynos_ehci-clk);
+ if (!IS_ERR(exynos_ehci-vdd33))
+ regulator_disable(exynos_ehci-vdd33);
+ if (!IS_ERR(exynos_ehci-vdd10))
+ regulator_disable(exynos_ehci-vdd10);
+
Re: [RESEND 1/2] usb: ehci-exynos: Make provision for vdd regulators
On 07.06.2015 22:20, Anand Moon wrote:
Facilitate getting required 3.3V and 1.0V VDD supply for
EHCI controller on Exynos.
With the patches for regulators' nodes merged in 3.15:
c8c253f ARM: dts: Add regulator entries to smdk5420
275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
the exynos systems turn on only minimal number of regulators.
Until now, the VDD regulator supplies were either turned on
by the bootloader, or the regulators were enabled by default
in the kernel, so that the controller drivers did not need to
care about turning on these regulators on their own.
This was rather bad about these controller drivers.
So ensuring now that the controller driver requests the necessary
VDD regulators (if available, unless there are direct VDD rails),
and enable them so as to make them working.
Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
Signed-off-by: Anand Moon linux.am...@gmail.com
Cc: Jingoo Han jg1@samsung.com
Cc: Alan Stern st...@rowland.harvard.edu
---
Initial version of this patch was part of following series, though
they are not dependent on each other, resubmitting after rebasing.
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266418.html
So you just took Vivek's patch along with all the credits... That is not
how we usually do this.
I would expect that rebasing a patch won't change the author unless this
is fine with Vivek.
---
.../devicetree/bindings/usb/exynos-usb.txt | 2 +
drivers/usb/host/ehci-exynos.c | 55
+-
2 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
b/Documentation/devicetree/bindings/usb/exynos-usb.txt
index 9b4dbe3..776fa03 100644
--- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
+++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
@@ -23,6 +23,8 @@ Required properties:
Optional properties:
- samsung,vbus-gpio: if present, specifies the GPIO that
needs to be pulled up for the bus to be powered.
+ - vdd33-supply: handle to 3.3V Vdd supply regulator for the controller.
+ - vdd10-supply: handle to 1.0V Vdd supply regulator for the controller.
Example:
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
index df538fd..4f8f9d2 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -21,6 +21,7 @@
#include linux/of_gpio.h
#include linux/phy/phy.h
#include linux/platform_device.h
+#include linux/regulator/consumer.h
#include linux/usb.h
#include linux/usb/hcd.h
@@ -45,6 +46,8 @@ static struct hc_driver __read_mostly exynos_ehci_hc_driver;
struct exynos_ehci_hcd {
struct clk *clk;
struct phy *phy[PHY_NUMBER];
+ struct regulator *vdd33;
+ struct regulator *vdd10;
};
#define to_exynos_ehci(hcd) (struct exynos_ehci_hcd
*)(hcd_to_ehci(hcd)-priv)
@@ -170,7 +173,27 @@ static int exynos_ehci_probe(struct platform_device
*pdev)
err = exynos_ehci_get_phy(pdev-dev, exynos_ehci);
if (err)
- goto fail_clk;
+ goto fail_regulator1;
+
+ exynos_ehci-vdd33 = devm_regulator_get_optional(pdev-dev, vdd33);
+ if (!IS_ERR(exynos_ehci-vdd33)) {
+ err = regulator_enable(exynos_ehci-vdd33);
+ if (err) {
+ dev_err(pdev-dev,
+ Failed to enable 3.3V Vdd supply\n);
+ goto fail_regulator1;
+ }
+ }
+
+ exynos_ehci-vdd10 = devm_regulator_get_optional(pdev-dev, vdd10);
+ if (!IS_ERR(exynos_ehci-vdd10)) {
+ err = regulator_enable(exynos_ehci-vdd10);
+ if (err) {
+ dev_err(pdev-dev,
+ Failed to enable 1.0V Vdd supply\n);
+ goto fail_regulator2;
+ }
+ }
skip_phy:
@@ -231,6 +254,10 @@ fail_add_hcd:
fail_io:
clk_disable_unprepare(exynos_ehci-clk);
fail_clk:
+ regulator_disable(exynos_ehci-vdd10);
+fail_regulator2:
+ regulator_disable(exynos_ehci-vdd33);
if (!IS_ERR()).
+fail_regulator1:
usb_put_hcd(hcd);
return err;
}
@@ -246,6 +273,11 @@ static int exynos_ehci_remove(struct platform_device
*pdev)
clk_disable_unprepare(exynos_ehci-clk);
+ if (!IS_ERR(exynos_ehci-vdd33))
+ regulator_disable(exynos_ehci-vdd33);
+ if (!IS_ERR(exynos_ehci-vdd10))
+ regulator_disable(exynos_ehci-vdd10);
+
usb_put_hcd(hcd);
return 0;
@@ -268,6 +300,11 @@ static int exynos_ehci_suspend(struct device *dev)
clk_disable_unprepare(exynos_ehci-clk);
+ if (!IS_ERR(exynos_ehci-vdd33))
+ regulator_disable(exynos_ehci-vdd33);
+ if (!IS_ERR(exynos_ehci-vdd10))
+ regulator_disable(exynos_ehci-vdd10);
+
Is EHCI a wakeup source? If yes then how disabling
