diff mbox

[v3,07/10] rtc: max77686: Use dev_warn() instead of pr_warn()

Message ID 1453836020-29579-8-git-send-email-javier@osg.samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Javier Martinez Canillas Jan. 26, 2016, 7:20 p.m. UTC 
It is better to use dev_*() log functions instead of pr_*() to print
information about the device in the kernel log in a standardized way.

This also allows to remove the local pr_fmt() defined macro.

Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>

---

Changes in v3:
- Add Laxman Dewangan's Acked-by tag to patch #7.

Changes in v2: None

 drivers/rtc/rtc-max77686.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Andi Shyti Jan. 27, 2016, 1:22 a.m. UTC | #1 
Hi Javier,

>  		if (tm->tm_year < 100) {
> -			pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
> -				1900 + tm->tm_year);
> +			dev_warn(info->dev,
> +				 "RTC can't handle year %d. Assume it's 2000\n",
> +				 1900 + tm->tm_year);
>  			return -EINVAL;

Because we are returning an error value, why not use dev_err()?

>  		}
>  	} else {
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Javier Martinez Canillas Jan. 27, 2016, 1:53 a.m. UTC | #2 
Hello Andi,

Thanks a lot for your feedback and review.

On 01/26/2016 10:22 PM, Andi Shyti wrote:
> Hi Javier,
>
>>   		if (tm->tm_year < 100) {
>> -			pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
>> -				1900 + tm->tm_year);
>> +			dev_warn(info->dev,
>> +				 "RTC can't handle year %d. Assume it's 2000\n",
>> +				 1900 + tm->tm_year);
>>   			return -EINVAL;
>
> Because we are returning an error value, why not use dev_err()?
>

You are absolutely right. Since the driver was using pr_warn(), I used
dev_warn() but dev_err() would had been correct.

If you don't mind I plan to do it as a follow up patch to avoid having
to resend the whole series only for this change.
  
Best regards,
Krzysztof Kozlowski Jan. 27, 2016, 2:05 a.m. UTC | #3 
On 27.01.2016 10:53, Javier Martinez Canillas wrote:
> Hello Andi,
> 
> Thanks a lot for your feedback and review.
> 
> On 01/26/2016 10:22 PM, Andi Shyti wrote:
>> Hi Javier,
>>
>>>           if (tm->tm_year < 100) {
>>> -            pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
>>> -                1900 + tm->tm_year);
>>> +            dev_warn(info->dev,
>>> +                 "RTC can't handle year %d. Assume it's 2000\n",
>>> +                 1900 + tm->tm_year);
>>>               return -EINVAL;
>>
>> Because we are returning an error value, why not use dev_err()?
>>
> 
> You are absolutely right. Since the driver was using pr_warn(), I used
> dev_warn() but dev_err() would had been correct.

Wait. The message says that "2000 will be assumed" which is not an
error. The message indicates that driver will proceed, thus the warning.

However the driver won't proceed because the max77686_rtc_set_time()
will abort. This came from max8997 which has the same issue.

This means that either message should be changed (dev_err() without the
"assume" verb) or the function should not abort and set the year to
2000+something (then dev_warn()... look at rtc-ds3234.c and rtc-mcp795.c).

The easiest would be to choose #1 - no changes in the logic.

BR,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andi Shyti Jan. 27, 2016, 2:12 a.m. UTC | #4 
> >>  		if (tm->tm_year < 100) {
> >>-			pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
> >>-				1900 + tm->tm_year);
> >>+			dev_warn(info->dev,
> >>+				 "RTC can't handle year %d. Assume it's 2000\n",
> >>+				 1900 + tm->tm_year);
> >>  			return -EINVAL;
> >
> >Because we are returning an error value, why not use dev_err()?
> >
> 
> You are absolutely right. Since the driver was using pr_warn(), I used
> dev_warn() but dev_err() would had been correct.
> 
> If you don't mind I plan to do it as a follow up patch to avoid having
> to resend the whole series only for this change.

Fine for me!

Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Javier Martinez Canillas Jan. 27, 2016, 2:20 a.m. UTC | #5 
Hello Krzysztof,

On Tue, Jan 26, 2016 at 11:05 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> On 27.01.2016 10:53, Javier Martinez Canillas wrote:
>> Hello Andi,
>>
>> Thanks a lot for your feedback and review.
>>
>> On 01/26/2016 10:22 PM, Andi Shyti wrote:
>>> Hi Javier,
>>>
>>>>           if (tm->tm_year < 100) {
>>>> -            pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
>>>> -                1900 + tm->tm_year);
>>>> +            dev_warn(info->dev,
>>>> +                 "RTC can't handle year %d. Assume it's 2000\n",
>>>> +                 1900 + tm->tm_year);
>>>>               return -EINVAL;
>>>
>>> Because we are returning an error value, why not use dev_err()?
>>>
>>
>> You are absolutely right. Since the driver was using pr_warn(), I used
>> dev_warn() but dev_err() would had been correct.
>
> Wait. The message says that "2000 will be assumed" which is not an
> error. The message indicates that driver will proceed, thus the warning.
>
> However the driver won't proceed because the max77686_rtc_set_time()
> will abort. This came from max8997 which has the same issue.
>
> This means that either message should be changed (dev_err() without the
> "assume" verb) or the function should not abort and set the year to
> 2000+something (then dev_warn()... look at rtc-ds3234.c and rtc-mcp795.c).
>
> The easiest would be to choose #1 - no changes in the logic.
>

Yes, I also prefer option #1 since technically is an error to set a
year that is not supported. Quite likely the user won't want to travel
to the past :)

Since you asked me to do other changes, I'll fix it in this patch for v4.

> BR,
> Krzysztof
>
>

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andi Shyti Jan. 27, 2016, 2:35 a.m. UTC | #6 
> > On 01/26/2016 10:22 PM, Andi Shyti wrote:
> >> Hi Javier,
> >>
> >>>           if (tm->tm_year < 100) {
> >>> -            pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
> >>> -                1900 + tm->tm_year);
> >>> +            dev_warn(info->dev,
> >>> +                 "RTC can't handle year %d. Assume it's 2000\n",
> >>> +                 1900 + tm->tm_year);
> >>>               return -EINVAL;
> >>
> >> Because we are returning an error value, why not use dev_err()?
> >>
> > 
> > You are absolutely right. Since the driver was using pr_warn(), I used
> > dev_warn() but dev_err() would had been correct.
> 
> Wait. The message says that "2000 will be assumed" which is not an
> error. The message indicates that driver will proceed, thus the warning.
> 
> However the driver won't proceed because the max77686_rtc_set_time()
> will abort. This came from max8997 which has the same issue.
> 
> This means that either message should be changed (dev_err() without the
> "assume" verb) or the function should not abort and set the year to
> 2000+something (then dev_warn()... look at rtc-ds3234.c and rtc-mcp795.c).
> 
> The easiest would be to choose #1 - no changes in the logic.

Nevertheless, the function fails, and those who call
max77686_rtc_tm_to_data() fail as well, so, we are printing
warning, but behaving as error.

Either we print error and return error, or we print warning, we
set:

tm->tm_year = 100; /* don't know if I got the logic right */

and return 0

Right?

Thanks,
Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Javier Martinez Canillas Jan. 27, 2016, 2:42 a.m. UTC | #7 
Hello Andi,

On Tue, Jan 26, 2016 at 11:35 PM, Andi Shyti <andi.shyti@samsung.com> wrote:
>> > On 01/26/2016 10:22 PM, Andi Shyti wrote:
>> >> Hi Javier,
>> >>
>> >>>           if (tm->tm_year < 100) {
>> >>> -            pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
>> >>> -                1900 + tm->tm_year);
>> >>> +            dev_warn(info->dev,
>> >>> +                 "RTC can't handle year %d. Assume it's 2000\n",
>> >>> +                 1900 + tm->tm_year);
>> >>>               return -EINVAL;
>> >>
>> >> Because we are returning an error value, why not use dev_err()?
>> >>
>> >
>> > You are absolutely right. Since the driver was using pr_warn(), I used
>> > dev_warn() but dev_err() would had been correct.
>>
>> Wait. The message says that "2000 will be assumed" which is not an
>> error. The message indicates that driver will proceed, thus the warning.
>>
>> However the driver won't proceed because the max77686_rtc_set_time()
>> will abort. This came from max8997 which has the same issue.
>>
>> This means that either message should be changed (dev_err() without the
>> "assume" verb) or the function should not abort and set the year to
>> 2000+something (then dev_warn()... look at rtc-ds3234.c and rtc-mcp795.c).
>>
>> The easiest would be to choose #1 - no changes in the logic.
>
> Nevertheless, the function fails, and those who call
> max77686_rtc_tm_to_data() fail as well, so, we are printing
> warning, but behaving as error.
>
> Either we print error and return error, or we print warning, we
> set:
>
> tm->tm_year = 100; /* don't know if I got the logic right */
>
> and return 0
>
> Right?
>

I was thinking in moving the check at the start of the function so a
on error, the RTC sec/min/hour/etc registers are untouched.

> Thanks,
> Andi
> --

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Belloni Jan. 27, 2016, 2:46 a.m. UTC | #8 
Hi,

On 27/01/2016 at 11:05:36 +0900, Krzysztof Kozlowski wrote :
> On 27.01.2016 10:53, Javier Martinez Canillas wrote:
> > Hello Andi,
> > 
> > Thanks a lot for your feedback and review.
> > 
> > On 01/26/2016 10:22 PM, Andi Shyti wrote:
> >> Hi Javier,
> >>
> >>>           if (tm->tm_year < 100) {
> >>> -            pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
> >>> -                1900 + tm->tm_year);
> >>> +            dev_warn(info->dev,
> >>> +                 "RTC can't handle year %d. Assume it's 2000\n",
> >>> +                 1900 + tm->tm_year);
> >>>               return -EINVAL;
> >>
> >> Because we are returning an error value, why not use dev_err()?
> >>
> > 
> > You are absolutely right. Since the driver was using pr_warn(), I used
> > dev_warn() but dev_err() would had been correct.
> 
> Wait. The message says that "2000 will be assumed" which is not an
> error. The message indicates that driver will proceed, thus the warning.
> 
> However the driver won't proceed because the max77686_rtc_set_time()
> will abort. This came from max8997 which has the same issue.
> 
> This means that either message should be changed (dev_err() without the
> "assume" verb) or the function should not abort and set the year to
> 2000+something (then dev_warn()... look at rtc-ds3234.c and rtc-mcp795.c).
> 
> The easiest would be to choose #1 - no changes in the logic.
> 

My stance on that is to never set a date that differs from the requested
date. Else, userspace has no way of knowing whether this is an erroneous
date or the real date when reading back.

I think I had a look and the driver is already doing the right thing but
the message is wrong.
Javier Martinez Canillas Jan. 27, 2016, 2:50 a.m. UTC | #9 
Hello Alexandre,

On Tue, Jan 26, 2016 at 11:46 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> Hi,
>
> On 27/01/2016 at 11:05:36 +0900, Krzysztof Kozlowski wrote :
>> On 27.01.2016 10:53, Javier Martinez Canillas wrote:
>> > Hello Andi,
>> >
>> > Thanks a lot for your feedback and review.
>> >
>> > On 01/26/2016 10:22 PM, Andi Shyti wrote:
>> >> Hi Javier,
>> >>
>> >>>           if (tm->tm_year < 100) {
>> >>> -            pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
>> >>> -                1900 + tm->tm_year);
>> >>> +            dev_warn(info->dev,
>> >>> +                 "RTC can't handle year %d. Assume it's 2000\n",
>> >>> +                 1900 + tm->tm_year);
>> >>>               return -EINVAL;
>> >>
>> >> Because we are returning an error value, why not use dev_err()?
>> >>
>> >
>> > You are absolutely right. Since the driver was using pr_warn(), I used
>> > dev_warn() but dev_err() would had been correct.
>>
>> Wait. The message says that "2000 will be assumed" which is not an
>> error. The message indicates that driver will proceed, thus the warning.
>>
>> However the driver won't proceed because the max77686_rtc_set_time()
>> will abort. This came from max8997 which has the same issue.
>>
>> This means that either message should be changed (dev_err() without the
>> "assume" verb) or the function should not abort and set the year to
>> 2000+something (then dev_warn()... look at rtc-ds3234.c and rtc-mcp795.c).
>>
>> The easiest would be to choose #1 - no changes in the logic.
>>
>
> My stance on that is to never set a date that differs from the requested
> date. Else, userspace has no way of knowing whether this is an erroneous
> date or the real date when reading back.
>
> I think I had a look and the driver is already doing the right thing but
> the message is wrong.
>

That's correct.

> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 09fc73016d3a..da47b1d85793 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -12,8 +12,6 @@ 
  *
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/slab.h>
 #include <linux/rtc.h>
 #include <linux/delay.h>
@@ -245,8 +243,9 @@  static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data,
 		data[RTC_YEAR] = tm->tm_year > 100 ? (tm->tm_year - 100) : 0;
 
 		if (tm->tm_year < 100) {
-			pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
-				1900 + tm->tm_year);
+			dev_warn(info->dev,
+				 "RTC can't handle year %d. Assume it's 2000\n",
+				 1900 + tm->tm_year);
 			return -EINVAL;
 		}
 	} else {