Re: linux-next: manual merge of the watchdog tree with the mfd tree

Re: linux-next: manual merge of the watchdog tree with the mfd tree

[Wim Van Sebroeck]

Hi Stephen,

> Today's linux-next merge of the watchdog tree got a conflict in
> drivers/watchdog/iTCO_wdt.c between commit 887c8ec7219f ("watchdog:
> Convert iTCO_wdt driver to mfd model") from the  tree and commit
> c3614aa19d3e ("watchdog: iTCO_wdt.c: fix printk format warnings") from
> the watchdog tree.
> 
> I fixed it up (see below) and can carry the fix as necessary.

Thanks for fixing this up.

But I'm a bit surprised: I wasn't Cc'ed about this patch to Convert
the iTCO_wdt driver to the mfd model...

Kind regards,
Wim.

Re: linux-next: manual merge of the watchdog tree with the mfd tree

[Guenter Roeck]

At 01:32 AM 5/18/2012, Wim Van Sebroeck wrote:
>Hi Stephen,
>
> > Today's linux-next merge of the watchdog tree got a conflict in
> > drivers/watchdog/iTCO_wdt.c between commit 887c8ec7219f ("watchdog:
> > Convert iTCO_wdt driver to mfd model") from the  tree and commit
> > c3614aa19d3e ("watchdog: iTCO_wdt.c: fix printk format warnings") from
> > the watchdog tree.
> >
> > I fixed it up (see below) and can carry the fix as necessary.
>
>Thanks for fixing this up.
>
>But I'm a bit surprised: I wasn't Cc'ed about this patch to Convert
>the iTCO_wdt driver to the mfd model...
Oh my. Kind of embarrassing to forget one of the maintainers in all 
the discussions which tree this patch set should go through. Even 
though I didn't much of the code, my sign-off is there, and I should 
have made sure that you are on the Cc: list. Sorry for that, and I 
owe you a beer or two if we ever meet in person.

Hope you are ok with the changes - the patch set already missed the 
last commit window because of the which-tree-to-use issue, and it 
would be sad to miss another one. And Jean is really waiting for it 
to go in to be able to push some related patches which need the gpio driver.

Thanks,
Guenter

Re: linux-next: manual merge of the watchdog tree with the mfd tree

[Wim Van Sebroeck]

Hi Guenter,

> >> Today's linux-next merge of the watchdog tree got a conflict in
> >> drivers/watchdog/iTCO_wdt.c between commit 887c8ec7219f ("watchdog:
> >> Convert iTCO_wdt driver to mfd model") from the  tree and commit
> >> c3614aa19d3e ("watchdog: iTCO_wdt.c: fix printk format warnings") from
> >> the watchdog tree.
> >>
> >> I fixed it up (see below) and can carry the fix as necessary.
> >
> >Thanks for fixing this up.
> >
> >But I'm a bit surprised: I wasn't Cc'ed about this patch to Convert
> >the iTCO_wdt driver to the mfd model...
> Oh my. Kind of embarrassing to forget one of the maintainers in all 
> the discussions which tree this patch set should go through. Even 
> though I didn't much of the code, my sign-off is there, and I should 
> have made sure that you are on the Cc: list. Sorry for that, and I 
> owe you a beer or two if we ever meet in person.

The maintainer and the writer of the driver: that's at least two beers indeed :-).

> Hope you are ok with the changes - the patch set already missed the 
> last commit window because of the which-tree-to-use issue, and it 
> would be sad to miss another one. And Jean is really waiting for it 
> to go in to be able to push some related patches which need the gpio driver.

I'm OK with it (although this means I can't go ahead yet with a watchdog core
conversion of the iTCO_wdt driver), except for the comile warning that the patch
introduces (as reported by Randy).

The original driver printed addresses as %04lx, this new driver uses %04llx but
doesn't typecast the values correctly. See message from Randy below.
Wether or not %04lx or %04llx is the best to use is off-course another question.
I don't think we have a clear standard yet, but I'm fine with either one of them.

Kind regards,
Wim.
---
Date: Mon, 14 May 2012 13:15:20 -0700
From: Randy Dunlap <rdunlap@xenotime.net>
To: Stephen Rothwell <sfr@canb.auug.org.au>
CC: linux-next@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>, 
 Wim Van Sebroeck <wim@iguana.be>,
 linux-watchdog@vger.kernel.org
Subject: [PATCH -next] wdt: fix iTCO printk format warnings

From: Randy Dunlap <rdunlap@xenotime.net>

Fix printk format warnings:

drivers/watchdog/iTCO_wdt.c:577:3: warning: format '%04llx' expects type 'long long unsigned int', but argument 2 has type 'resource_size_t'
drivers/watchdog/iTCO_wdt.c:594:3: warning: format '%04llx' expects type 'long long unsigned int', but argument 2 has type 'resource_size_t'
drivers/watchdog/iTCO_wdt.c:600:2: warning: format '%04llx' expects type 'long long unsigned int', but argument 4 has type 'resource_size_t'

Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
Cc:	Wim Van Sebroeck <wim@iguana.be>
---
 drivers/watchdog/iTCO_wdt.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- linux-next-20120514.orig/drivers/watchdog/iTCO_wdt.c
+++ linux-next-20120514/drivers/watchdog/iTCO_wdt.c
@@ -575,7 +575,7 @@ static int __devinit iTCO_wdt_probe(stru
 	if (!request_region(iTCO_wdt_private.smi_res->start,
 			resource_size(iTCO_wdt_private.smi_res), dev->name)) {
 		pr_err("I/O address 0x%04llx already in use, device disabled\n",
-		       SMI_EN);
+		       (u64)SMI_EN);
 		ret = -EBUSY;
 		goto unmap_gcs;
 	}
@@ -592,13 +592,13 @@ static int __devinit iTCO_wdt_probe(stru
 	if (!request_region(iTCO_wdt_private.tco_res->start,
 			resource_size(iTCO_wdt_private.tco_res), dev->name)) {
 		pr_err("I/O address 0x%04llx already in use, device disabled\n",
-		       TCOBASE);
+		       (u64)TCOBASE);
 		ret = -EBUSY;
 		goto unreg_smi;
 	}
 
 	pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
-		ich_info->name, ich_info->iTCO_version, TCOBASE);
+		ich_info->name, ich_info->iTCO_version, (u64)TCOBASE);
 
 	/* Clear out the (probably old) status */
 	outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */

Re: linux-next: manual merge of the watchdog tree with the mfd tree

[Guenter Roeck]

At 09:35 AM 5/22/2012, Wim Van Sebroeck wrote:
>Hi Guenter,
>
> > >> Today's linux-next merge of the watchdog tree got a conflict in
> > >> drivers/watchdog/iTCO_wdt.c between commit 887c8ec7219f ("watchdog:
> > >> Convert iTCO_wdt driver to mfd model") from the  tree and commit
> > >> c3614aa19d3e ("watchdog: iTCO_wdt.c: fix printk format warnings") from
> > >> the watchdog tree.
> > >>
> > >> I fixed it up (see below) and can carry the fix as necessary.
> > >
> > >Thanks for fixing this up.
> > >
> > >But I'm a bit surprised: I wasn't Cc'ed about this patch to Convert
> > >the iTCO_wdt driver to the mfd model...
> > Oh my. Kind of embarrassing to forget one of the maintainers in all
> > the discussions which tree this patch set should go through. Even
> > though I didn't much of the code, my sign-off is there, and I should
> > have made sure that you are on the Cc: list. Sorry for that, and I
> > owe you a beer or two if we ever meet in person.
>
>The maintainer and the writer of the driver: that's at least two 
>beers indeed :-).

Make it three. Going to be a happy night.

> > Hope you are ok with the changes - the patch set already missed the
> > last commit window because of the which-tree-to-use issue, and it
> > would be sad to miss another one. And Jean is really waiting for it
> > to go in to be able to push some related patches which need the 
> gpio driver.
>
>I'm OK with it (although this means I can't go ahead yet with a watchdog core
>conversion of the iTCO_wdt driver), except for the comile warning 
>that the patch
>introduces (as reported by Randy).
>
>The original driver printed addresses as %04lx, this new driver uses 
>%04llx but
>doesn't typecast the values correctly. See message from Randy below.
>Wether or not %04lx or %04llx is the best to use is off-course 
>another question.
>I don't think we have a clear standard yet, but I'm fine with either 
>one of them.

Me too, though %04lx and typecast to long should really be sufficient.

I'd guess the change was made because resource_size_t is sometimes a long
and sometimes a long long, depending on the platform. I hit that 
problem a couple of times myself.

How do we make sure that Randy's patch makes it upstream ? The iTCO 
patch is currently in Sam's tree.

Thanks,
Guenter

>Kind regards,
>Wim.
>---
>Date: Mon, 14 May 2012 13:15:20 -0700
>From: Randy Dunlap <rdunlap@xenotime.net>
>To: Stephen Rothwell <sfr@canb.auug.org.au>
>CC: linux-next@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
>  Wim Van Sebroeck <wim@iguana.be>,
>  linux-watchdog@vger.kernel.org
>Subject: [PATCH -next] wdt: fix iTCO printk format warnings
>
>From: Randy Dunlap <rdunlap@xenotime.net>
>
>Fix printk format warnings:
>
>drivers/watchdog/iTCO_wdt.c:577:3: warning: format '%04llx' expects 
>type 'long long unsigned int', but argument 2 has type 'resource_size_t'
>drivers/watchdog/iTCO_wdt.c:594:3: warning: format '%04llx' expects 
>type 'long long unsigned int', but argument 2 has type 'resource_size_t'
>drivers/watchdog/iTCO_wdt.c:600:2: warning: format '%04llx' expects 
>type 'long long unsigned int', but argument 4 has type 'resource_size_t'
>
>Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
>Cc:     Wim Van Sebroeck <wim@iguana.be>
>---
>  drivers/watchdog/iTCO_wdt.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
>--- linux-next-20120514.orig/drivers/watchdog/iTCO_wdt.c
>+++ linux-next-20120514/drivers/watchdog/iTCO_wdt.c
>@@ -575,7 +575,7 @@ static int __devinit iTCO_wdt_probe(stru
>         if (!request_region(iTCO_wdt_private.smi_res->start,
>                         resource_size(iTCO_wdt_private.smi_res), 
> dev->name)) {
>                 pr_err("I/O address 0x%04llx already in use, device 
> disabled\n",
>-                      SMI_EN);
>+                      (u64)SMI_EN);
>                 ret = -EBUSY;
>                 goto unmap_gcs;
>         }
>@@ -592,13 +592,13 @@ static int __devinit iTCO_wdt_probe(stru
>         if (!request_region(iTCO_wdt_private.tco_res->start,
>                         resource_size(iTCO_wdt_private.tco_res), 
> dev->name)) {
>                 pr_err("I/O address 0x%04llx already in use, device 
> disabled\n",
>-                      TCOBASE);
>+                      (u64)TCOBASE);
>                 ret = -EBUSY;
>                 goto unreg_smi;
>         }
>
>         pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
>-               ich_info->name, ich_info->iTCO_version, TCOBASE);
>+               ich_info->name, ich_info->iTCO_version, (u64)TCOBASE);
>
>         /* Clear out the (probably old) status */
>         outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */

Re: linux-next: manual merge of the watchdog tree with the mfd tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

Hi guys,

On Tue, 22 May 2012 15:37:04 -0700 Guenter Roeck <guenter@roeck-us.net> wrote:
>
> >I don't think we have a clear standard yet, but I'm fine with either 
> >one of them.
> 
> Me too, though %04lx and typecast to long should really be sufficient.
> 
> I'd guess the change was made because resource_size_t is sometimes a long
> and sometimes a long long, depending on the platform. I hit that 
> problem a couple of times myself.

Exactly.  Since resource_size_t can be either 32 bit or 64 bit on 32 bit
platforms, you must print it as %llx and cast it to u64 always to
prevent these warnings.  If you cast it to (unsigned) long you could
possibly truncate the value.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: linux-next: manual merge of the watchdog tree with the mfd tree

[Wim Van Sebroeck]

Hi Sam,

Since %04llx with a typecast to u64 is the way to go (Thanks Stephen and Randy)...

> How do we make sure that Randy's patch makes it upstream ? The iTCO 
> patch is currently in Sam's tree.

Can you carry Randy's patch/fix (since it fixes the change introduced by the iTCO_wdt mfd patch)?
If not then I will apply the patch later on once the iTCO_wdt mfd patch is mainstream.

Kind regards,
Wim.

Re: linux-next: manual merge of the watchdog tree with the mfd tree

[Samuel Ortiz]

Hi Wim,

On Wed, May 23, 2012 at 07:44:30AM +0200, Wim Van Sebroeck wrote:
> Hi Sam,
> 
> Since %04llx with a typecast to u64 is the way to go (Thanks Stephen and Randy)...
> 
> > How do we make sure that Randy's patch makes it upstream ? The iTCO 
> > patch is currently in Sam's tree.
> 
> Can you carry Randy's patch/fix (since it fixes the change introduced by the iTCO_wdt mfd patch)?
> If not then I will apply the patch later on once the iTCO_wdt mfd patch is mainstream.
> 
I'd appreciate if you could take it. I'm trying to avoid taking more patches
at this moment.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: linux-next: manual merge of the watchdog tree with the mfd tree

[Wim Van Sebroeck]

Hi Sam,

> > Since %04llx with a typecast to u64 is the way to go (Thanks Stephen and Randy)...
> > 
> > > How do we make sure that Randy's patch makes it upstream ? The iTCO 
> > > patch is currently in Sam's tree.
> > 
> > Can you carry Randy's patch/fix (since it fixes the change introduced by the iTCO_wdt mfd patch)?
> > If not then I will apply the patch later on once the iTCO_wdt mfd patch is mainstream.
> > 
> I'd appreciate if you could take it. I'm trying to avoid taking more patches
> at this moment.

No problem. I'll take it then.

Kind regards,
Wim.