diff mbox

[v3,3/7] max8903: cleans up confusing relationship between dc_valid, dok and dcm.

Message ID 1466139626-51434-4-git-send-email-chris@lapa.com.au (mailing list archive)
State Superseded, archived
Headers show

Commit Message

chris@lapa.com.au June 17, 2016, 5 a.m. UTC 
From: Chris Lapa <chris@lapa.com.au>

The max8903_charger.h file indicated that dcm and dok were not optional
when dc_valid is set.

It makes sense to have dok as a compulsory pin when dc_valid is given.
However dcm can be optionally wired to a fixed level especially when the
circuit is configured for dc power exclusively.

The previous implementation already allowed for this somewhat, however no
error was given if dok wasn't given whilst dc_valid was.

The new implementation enforces dok presence when dc_valid is given. Whilst
allowing dcm to be optional.

Signed-off-by: Chris Lapa <chris@lapa.com.au>
---
 drivers/power/max8903_charger.c       | 23 ++++++++++-------------
 include/linux/power/max8903_charger.h |  6 +++---
 2 files changed, 13 insertions(+), 16 deletions(-)

Comments

Krzysztof Kozlowski June 17, 2016, 6:23 a.m. UTC | #1 
On 06/17/2016 07:00 AM, Chris Lapa wrote:
> From: Chris Lapa <chris@lapa.com.au>
> 
> The max8903_charger.h file indicated that dcm and dok were not optional
> when dc_valid is set.
> 
> It makes sense to have dok as a compulsory pin when dc_valid is given.
> However dcm can be optionally wired to a fixed level especially when the
> circuit is configured for dc power exclusively.
> 
> The previous implementation already allowed for this somewhat, however no
> error was given if dok wasn't given whilst dc_valid was.
> 
> The new implementation enforces dok presence when dc_valid is given. Whilst
> allowing dcm to be optional.
> 
> Signed-off-by: Chris Lapa <chris@lapa.com.au>
> ---
>  drivers/power/max8903_charger.c       | 23 ++++++++++-------------
>  include/linux/power/max8903_charger.h |  6 +++---
>  2 files changed, 13 insertions(+), 16 deletions(-)

Code looks correct:
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski June 17, 2016, 6:26 a.m. UTC | #2 
On 06/17/2016 07:00 AM, Chris Lapa wrote:
> From: Chris Lapa <chris@lapa.com.au>
> 
> The max8903_charger.h file indicated that dcm and dok were not optional
> when dc_valid is set.
> 
> It makes sense to have dok as a compulsory pin when dc_valid is given.
> However dcm can be optionally wired to a fixed level especially when the
> circuit is configured for dc power exclusively.
> 
> The previous implementation already allowed for this somewhat, however no
> error was given if dok wasn't given whilst dc_valid was.
> 
> The new implementation enforces dok presence when dc_valid is given. Whilst
> allowing dcm to be optional.
> 
> Signed-off-by: Chris Lapa <chris@lapa.com.au>
> ---
>  drivers/power/max8903_charger.c       | 23 ++++++++++-------------
>  include/linux/power/max8903_charger.h |  6 +++---
>  2 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/power/max8903_charger.c b/drivers/power/max8903_charger.c
> index 0a5b0e1..dbd911c4 100644
> --- a/drivers/power/max8903_charger.c
> +++ b/drivers/power/max8903_charger.c
> @@ -211,27 +211,24 @@ static int max8903_probe(struct platform_device *pdev)
>  	}
>  
>  	if (pdata->dc_valid) {
> -		if (pdata->dok && gpio_is_valid(pdata->dok) &&
> -				pdata->dcm && gpio_is_valid(pdata->dcm)) {
> +		if (pdata->dok && gpio_is_valid(pdata->dok)) {
>  			gpio = pdata->dok; /* PULL_UPed Interrupt */
>  			ta_in = gpio_get_value(gpio) ? 0 : 1;
> +		} else {
> +			dev_err(dev, "When DC is wired, DOK should"
> +					" be wired as well.\n");

Just found one nit. Don't split the strings.
        dev_err(dev,
                "When DC is wired, DOK should be wired as well.\n");

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
chris@lapa.com.au June 17, 2016, 6:28 a.m. UTC | #3 
On 17/06/2016 4:26 PM, Krzysztof Kozlowski wrote:
> On 06/17/2016 07:00 AM, Chris Lapa wrote:
>> From: Chris Lapa <chris@lapa.com.au>
>>
>> The max8903_charger.h file indicated that dcm and dok were not optional
>> when dc_valid is set.
>>
>> It makes sense to have dok as a compulsory pin when dc_valid is given.
>> However dcm can be optionally wired to a fixed level especially when the
>> circuit is configured for dc power exclusively.
>>
>> The previous implementation already allowed for this somewhat, however no
>> error was given if dok wasn't given whilst dc_valid was.
>>
>> The new implementation enforces dok presence when dc_valid is given. Whilst
>> allowing dcm to be optional.
>>
>> Signed-off-by: Chris Lapa <chris@lapa.com.au>
>> ---
>>  drivers/power/max8903_charger.c       | 23 ++++++++++-------------
>>  include/linux/power/max8903_charger.h |  6 +++---
>>  2 files changed, 13 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/power/max8903_charger.c b/drivers/power/max8903_charger.c
>> index 0a5b0e1..dbd911c4 100644
>> --- a/drivers/power/max8903_charger.c
>> +++ b/drivers/power/max8903_charger.c
>> @@ -211,27 +211,24 @@ static int max8903_probe(struct platform_device *pdev)
>>  	}
>>
>>  	if (pdata->dc_valid) {
>> -		if (pdata->dok && gpio_is_valid(pdata->dok) &&
>> -				pdata->dcm && gpio_is_valid(pdata->dcm)) {
>> +		if (pdata->dok && gpio_is_valid(pdata->dok)) {
>>  			gpio = pdata->dok; /* PULL_UPed Interrupt */
>>  			ta_in = gpio_get_value(gpio) ? 0 : 1;
>> +		} else {
>> +			dev_err(dev, "When DC is wired, DOK should"
>> +					" be wired as well.\n");
>
> Just found one nit. Don't split the strings.
>         dev_err(dev,
>                 "When DC is wired, DOK should be wired as well.\n");
I saw that one as well when I ran checkpatch, however I thought if I 
changed it then I would get a warning about the line being > 80 chars. 
So wasn't sure which direction to go.

Thanks
Chris
>
> Best regards,
> Krzysztof
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski June 17, 2016, 6:42 a.m. UTC | #4 
On 06/17/2016 08:28 AM, Chris Lapa wrote:
> On 17/06/2016 4:26 PM, Krzysztof Kozlowski wrote:
>> On 06/17/2016 07:00 AM, Chris Lapa wrote:
>>> From: Chris Lapa <chris@lapa.com.au>
>>>
>>> The max8903_charger.h file indicated that dcm and dok were not optional
>>> when dc_valid is set.
>>>
>>> It makes sense to have dok as a compulsory pin when dc_valid is given.
>>> However dcm can be optionally wired to a fixed level especially when the
>>> circuit is configured for dc power exclusively.
>>>
>>> The previous implementation already allowed for this somewhat,
>>> however no
>>> error was given if dok wasn't given whilst dc_valid was.
>>>
>>> The new implementation enforces dok presence when dc_valid is given.
>>> Whilst
>>> allowing dcm to be optional.
>>>
>>> Signed-off-by: Chris Lapa <chris@lapa.com.au>
>>> ---
>>>  drivers/power/max8903_charger.c       | 23 ++++++++++-------------
>>>  include/linux/power/max8903_charger.h |  6 +++---
>>>  2 files changed, 13 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/power/max8903_charger.c
>>> b/drivers/power/max8903_charger.c
>>> index 0a5b0e1..dbd911c4 100644
>>> --- a/drivers/power/max8903_charger.c
>>> +++ b/drivers/power/max8903_charger.c
>>> @@ -211,27 +211,24 @@ static int max8903_probe(struct platform_device
>>> *pdev)
>>>      }
>>>
>>>      if (pdata->dc_valid) {
>>> -        if (pdata->dok && gpio_is_valid(pdata->dok) &&
>>> -                pdata->dcm && gpio_is_valid(pdata->dcm)) {
>>> +        if (pdata->dok && gpio_is_valid(pdata->dok)) {
>>>              gpio = pdata->dok; /* PULL_UPed Interrupt */
>>>              ta_in = gpio_get_value(gpio) ? 0 : 1;
>>> +        } else {
>>> +            dev_err(dev, "When DC is wired, DOK should"
>>> +                    " be wired as well.\n");
>>
>> Just found one nit. Don't split the strings.
>>         dev_err(dev,
>>                 "When DC is wired, DOK should be wired as well.\n");
> I saw that one as well when I ran checkpatch, however I thought if I
> changed it then I would get a warning about the line being > 80 chars.
> So wasn't sure which direction to go.

Checkpatch shouldn't complain on strings so if you move the string to
next line, it should be ok.

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
chris@lapa.com.au June 19, 2016, 1:48 p.m. UTC | #5 
On 17/06/2016 4:26 PM, Krzysztof Kozlowski wrote:
> On 06/17/2016 07:00 AM, Chris Lapa wrote:
>> From: Chris Lapa <chris@lapa.com.au>
>>
>> The max8903_charger.h file indicated that dcm and dok were not optional
>> when dc_valid is set.
>>
>> It makes sense to have dok as a compulsory pin when dc_valid is given.
>> However dcm can be optionally wired to a fixed level especially when the
>> circuit is configured for dc power exclusively.
>>
>> The previous implementation already allowed for this somewhat, however no
>> error was given if dok wasn't given whilst dc_valid was.
>>
>> The new implementation enforces dok presence when dc_valid is given. Whilst
>> allowing dcm to be optional.
>>
>> Signed-off-by: Chris Lapa <chris@lapa.com.au>
>> ---
>>  drivers/power/max8903_charger.c       | 23 ++++++++++-------------
>>  include/linux/power/max8903_charger.h |  6 +++---
>>  2 files changed, 13 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/power/max8903_charger.c b/drivers/power/max8903_charger.c
>> index 0a5b0e1..dbd911c4 100644
>> --- a/drivers/power/max8903_charger.c
>> +++ b/drivers/power/max8903_charger.c
>> @@ -211,27 +211,24 @@ static int max8903_probe(struct platform_device *pdev)
>>  	}
>>
>>  	if (pdata->dc_valid) {
>> -		if (pdata->dok && gpio_is_valid(pdata->dok) &&
>> -				pdata->dcm && gpio_is_valid(pdata->dcm)) {
>> +		if (pdata->dok && gpio_is_valid(pdata->dok)) {
>>  			gpio = pdata->dok; /* PULL_UPed Interrupt */
>>  			ta_in = gpio_get_value(gpio) ? 0 : 1;
>> +		} else {
>> +			dev_err(dev, "When DC is wired, DOK should"
>> +					" be wired as well.\n");
>
> Just found one nit. Don't split the strings.
>         dev_err(dev,
>                 "When DC is wired, DOK should be wired as well.\n");
>
> Best regards,
> Krzysztof
>

Fixed in v4

Thanks,
Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/power/max8903_charger.c b/drivers/power/max8903_charger.c
index 0a5b0e1..dbd911c4 100644
--- a/drivers/power/max8903_charger.c
+++ b/drivers/power/max8903_charger.c
@@ -211,27 +211,24 @@  static int max8903_probe(struct platform_device *pdev)
 	}
 
 	if (pdata->dc_valid) {
-		if (pdata->dok && gpio_is_valid(pdata->dok) &&
-				pdata->dcm && gpio_is_valid(pdata->dcm)) {
+		if (pdata->dok && gpio_is_valid(pdata->dok)) {
 			gpio = pdata->dok; /* PULL_UPed Interrupt */
 			ta_in = gpio_get_value(gpio) ? 0 : 1;
+		} else {
+			dev_err(dev, "When DC is wired, DOK should"
+					" be wired as well.\n");
+			return -EINVAL;
+		}
+	}
 
+	if (pdata->dcm) {
+		if (gpio_is_valid(pdata->dcm)) {
 			gpio = pdata->dcm; /* Output */
 			gpio_set_value(gpio, ta_in);
 		} else {
-			dev_err(dev, "When DC is wired, DOK and DCM should"
-					" be wired as well.\n");
+			dev_err(dev, "Invalid pin: dcm.\n");
 			return -EINVAL;
 		}
-	} else {
-		if (pdata->dcm) {
-			if (gpio_is_valid(pdata->dcm))
-				gpio_set_value(pdata->dcm, 0);
-			else {
-				dev_err(dev, "Invalid pin: dcm.\n");
-				return -EINVAL;
-			}
-		}
 	}
 
 	if (pdata->usb_valid) {
diff --git a/include/linux/power/max8903_charger.h b/include/linux/power/max8903_charger.h
index 24f51db..89d3f1c 100644
--- a/include/linux/power/max8903_charger.h
+++ b/include/linux/power/max8903_charger.h
@@ -26,8 +26,8 @@ 
 struct max8903_pdata {
 	/*
 	 * GPIOs
-	 * cen, chg, flt, and usus are optional.
-	 * dok, dcm, and uok are not optional depending on the status of
+	 * cen, chg, flt, dcm and usus are optional.
+	 * dok and uok are not optional depending on the status of
 	 * dc_valid and usb_valid.
 	 */
 	int cen;	/* Charger Enable input */
@@ -41,7 +41,7 @@  struct max8903_pdata {
 	/*
 	 * DC(Adapter/TA) is wired
 	 * When dc_valid is true,
-	 *	dok and dcm should be valid.
+	 *	dok should be valid.
 	 *
 	 * At least one of dc_valid or usb_valid should be true.
 	 */