Re: [PATCH v2 53/58] firewire: don't break long lines

[Stefan Richter]

On Oct 19 Mauro Carvalho Chehab wrote:
> Em Wed, 19 Oct 2016 08:03:01 +0900
> Takashi Sakamoto  escreveu:
> > A structure for firedtv (struct firedtv) has a member for a pointer to
> > struct device. In this case, we can use dev_dbg() for debug printing.
[...]
> Stefan,
> 
> Would the above be OK for you?

ACK.
-- 
Stefan Richter
-==- =-=- =-=--
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 53/58] firewire: don't break long lines

[Mauro Carvalho Chehab]

Em Wed, 19 Oct 2016 08:03:01 +0900
Takashi Sakamoto  escreveu:

> From: Takashi Sakamoto 
> Date: Wed, 19 Oct 2016 07:53:35 +0900
> Subject: [PATCH] [media] firewire: use dev_dbg() instead of printk()
> 
> A structure for firedtv (struct firedtv) has a member for a pointer to
> struct device. In this case, we can use dev_dbg() for debug printing.
> This is more preferrable behaviour in device driver development.
> 
> Signed-off-by: Takashi Sakamoto 
> ---
>  drivers/media/firewire/firedtv-rc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/firewire/firedtv-rc.c
> b/drivers/media/firewire/firedtv-rc.c
> index f82d4a9..04dea2a 100644
> --- a/drivers/media/firewire/firedtv-rc.c
> +++ b/drivers/media/firewire/firedtv-rc.c
> @@ -184,8 +184,9 @@ void fdtv_handle_rc(struct firedtv *fdtv, unsigned int 
> code)
>   else if (code >= 0x4540 && code <= 0x4542)
>   code = oldtable[code - 0x4521];
>   else {
> - printk(KERN_DEBUG "firedtv: invalid key code 0x%04x "
> -"from remote control\n", code);
> + dev_dbg(fdtv->device,
> + "invalid key code 0x%04x from remote control\n",
> + code);
>   return;
>   }

Looks good to me. Applied to my development tree:

https://git.linuxtv.org/mchehab/experimental.git/commit/?h=printk=e0de6d90145753bf40415d670471fcc536b2a26c

https://git.linuxtv.org/mchehab/experimental.git/commit/?h=printk=9532ba4af6a7619bb028ddd3b829e6f163917b79

Stefan,

Would the above be OK for you?

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

Re: [PATCH v2 53/58] firewire: don't break long lines

[Mauro Carvalho Chehab]

Em Wed, 19 Oct 2016 09:56:25 +0200
Stefan Richter  escreveu:

> On Oct 19 Takashi Sakamoto wrote:
> > --- a/drivers/media/firewire/firedtv-rc.c
> > +++ b/drivers/media/firewire/firedtv-rc.c
> > @@ -184,8 +184,9 @@ void fdtv_handle_rc(struct firedtv *fdtv, unsigned
> > int code)
> > else if (code >= 0x4540 && code <= 0x4542)
> > code = oldtable[code - 0x4521];
> > else {
> > -   printk(KERN_DEBUG "firedtv: invalid key code 0x%04x "
> > -  "from remote control\n", code);
> > +   dev_dbg(fdtv->device,
> > +   "invalid key code 0x%04x from remote control\n",
> > +   code);
> > return;
> > }
> >   
> 
> Yes, dev_XYZ(fdtv->device, ...) is better here and is already used this
> way throughout the firedtv driver.  firedtv-rc.c somehow fell through the
> cracks when firedtv was made to use dev_XYZ().
> 
> (On an unrelated note, this reminds me that I still need to take care of
> Mauro's patches "Add a keymap for FireDTV board" and "firedtv: Port it to
> use rc_core" from May 28, 2012.)

Oh! I forgot about those a long time ago ;) Yeah, it would be great if
you could look into those patches when you have some time.

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

Re: [PATCH v2 53/58] firewire: don't break long lines

[Mauro Carvalho Chehab]

Em Wed, 19 Oct 2016 10:01:13 +0200
Stefan Richter  escreveu:

> On Oct 18 Mauro Carvalho Chehab wrote:
> [...]
> > The patch was generated via the script below, and manually
> > adjusted if needed.  
> [...]
> > Reviewed-by: Takashi Sakamoto 
> > Acked-by: Stefan Richter 
> > Signed-off-by: Mauro Carvalho Chehab 
> > ---
> >  drivers/media/firewire/firedtv-rc.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)  
> [...]
> 
> Patch v1 also had a hunk in drivers/media/firewire/firedtv-avc.c.
> What happened to it?

There was some mistake when I manipulated it manually, after re-run
the script.

Btw, I liked more the Takashi's dev_dbg() patch for firedtv-rc.c.

So, I guess the better would be to apply his patch and the one
enclosed here, with just the firedtv-avc.c change.

Thanks,
Mauro

[PATCH] firewire: don't break long lines

Due to the 80-cols restrictions, and latter due to checkpatch
warnings, several strings were broken into multiple lines. This
is not considered a good practice anymore, as it makes harder
to grep for strings at the source code.

As we're right now fixing other drivers due to KERN_CONT, we need
to be able to identify what printk strings don't end with a "\n".
It is a way easier to detect those if we don't break long lines.

So, join those continuation lines.

The patch was generated via the script below, and manually
adjusted if needed.


use Text::Tabs;
while (<>) {
if ($next ne "") {
$c=$_;
if ($c =~ /^\s+\"(.*)/) {
$c2=$1;
$next =~ s/\"\n$//;
$n = expand($next);
$funpos = index($n, '(');
$pos = index($c2, '",');
if ($funpos && $pos > 0) {
$s1 = substr $c2, 0, $pos + 2;
$s2 = ' ' x ($funpos + 1) . substr $c2, $pos + 
2;
$s2 =~ s/^\s+//;

$s2 = ' ' x ($funpos + 1) . $s2 if ($s2 ne "");

print unexpand("$next$s1\n");
print unexpand("$s2\n") if ($s2 ne "");
} else {
print "$next$c2\n";
}
$next="";
next;
} else {
print $next;
}
$next="";
} else {
if (m/\"$/) {
if (!m/\\n\"$/) {
$next=$_;
next;
}
}
}
print $_;
}


Reviewed-by: Takashi Sakamoto 
Acked-by: Stefan Richter 
Signed-off-by: Mauro Carvalho Chehab 

diff --git a/drivers/media/firewire/firedtv-avc.c 
b/drivers/media/firewire/firedtv-avc.c
index 251a556112a9..5bde6c209cd7 100644
--- a/drivers/media/firewire/firedtv-avc.c
+++ b/drivers/media/firewire/firedtv-avc.c
@@ -1181,8 +1181,8 @@ int avc_ca_pmt(struct firedtv *fdtv, char *msg, int 
length)
if (es_info_length > 0) {
pmt_cmd_id = msg[read_pos++];
if (pmt_cmd_id != 1 && pmt_cmd_id != 4)
-   dev_err(fdtv->device, "invalid pmt_cmd_id %d "
-   "at stream level\n", pmt_cmd_id);
+   dev_err(fdtv->device, "invalid pmt_cmd_id %d at 
stream level\n",
+   pmt_cmd_id);
 
if (es_info_length > sizeof(c->operand) - 4 -
 write_pos) {
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 53/58] firewire: don't break long lines

[Stefan Richter]

On Oct 19 Takashi Sakamoto wrote:
> --- a/drivers/media/firewire/firedtv-rc.c
> +++ b/drivers/media/firewire/firedtv-rc.c
> @@ -184,8 +184,9 @@ void fdtv_handle_rc(struct firedtv *fdtv, unsigned
> int code)
>   else if (code >= 0x4540 && code <= 0x4542)
>   code = oldtable[code - 0x4521];
>   else {
> - printk(KERN_DEBUG "firedtv: invalid key code 0x%04x "
> -"from remote control\n", code);
> + dev_dbg(fdtv->device,
> + "invalid key code 0x%04x from remote control\n",
> + code);
>   return;
>   }
> 

Yes, dev_XYZ(fdtv->device, ...) is better here and is already used this
way throughout the firedtv driver.  firedtv-rc.c somehow fell through the
cracks when firedtv was made to use dev_XYZ().

(On an unrelated note, this reminds me that I still need to take care of
Mauro's patches "Add a keymap for FireDTV board" and "firedtv: Port it to
use rc_core" from May 28, 2012.)
-- 
Stefan Richter
-==- =-=- =--==
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 53/58] firewire: don't break long lines

[Takashi Sakamoto]

Hi,

On Oct 19 2016 05:46, Mauro Carvalho Chehab wrote:
> Due to the 80-cols restrictions, and latter due to checkpatch
> warnings, several strings were broken into multiple lines. This
> is not considered a good practice anymore, as it makes harder
> to grep for strings at the source code.
> 
> As we're right now fixing other drivers due to KERN_CONT, we need
> to be able to identify what printk strings don't end with a "\n".
> It is a way easier to detect those if we don't break long lines.
> 
> So, join those continuation lines.
> 
> The patch was generated via the script below, and manually
> adjusted if needed.
> 
> 
> use Text::Tabs;
> while (<>) {
>   if ($next ne "") {
>   $c=$_;
>   if ($c =~ /^\s+\"(.*)/) {
>   $c2=$1;
>   $next =~ s/\"\n$//;
>   $n = expand($next);
>   $funpos = index($n, '(');
>   $pos = index($c2, '",');
>   if ($funpos && $pos > 0) {
>   $s1 = substr $c2, 0, $pos + 2;
>   $s2 = ' ' x ($funpos + 1) . substr $c2, $pos + 
> 2;
>   $s2 =~ s/^\s+//;
> 
>   $s2 = ' ' x ($funpos + 1) . $s2 if ($s2 ne "");
> 
>   print unexpand("$next$s1\n");
>   print unexpand("$s2\n") if ($s2 ne "");
>   } else {
>   print "$next$c2\n";
>   }
>   $next="";
>   next;
>   } else {
>   print $next;
>   }
>   $next="";
>   } else {
>   if (m/\"$/) {
>   if (!m/\\n\"$/) {
>   $next=$_;
>   next;
>   }
>   }
>   }
>   print $_;
> }
> 
> 
> Reviewed-by: Takashi Sakamoto 
> Acked-by: Stefan Richter 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/firewire/firedtv-rc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/firewire/firedtv-rc.c 
> b/drivers/media/firewire/firedtv-rc.c
> index f82d4a93feb3..46dde73944df 100644
> --- a/drivers/media/firewire/firedtv-rc.c
> +++ b/drivers/media/firewire/firedtv-rc.c
> @@ -184,8 +184,8 @@ void fdtv_handle_rc(struct firedtv *fdtv, unsigned int 
> code)
>   else if (code >= 0x4540 && code <= 0x4542)
>   code = oldtable[code - 0x4521];
>   else {
> - printk(KERN_DEBUG "firedtv: invalid key code 0x%04x "
> -"from remote control\n", code);
> + printk(KERN_DEBUG "firedtv: invalid key code 0x%04x from remote 
> control\n",
> +code);
>   return;
>   }

I realized that we can use dev_dbg() instead of the printk(). What do
you think about this patch? Anyway, the line is within 80 characters.

 8< 

>From da3289a04226450d6dbabb5c81155ac17c11374d Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto 
Date: Wed, 19 Oct 2016 07:53:35 +0900
Subject: [PATCH] [media] firewire: use dev_dbg() instead of printk()

A structure for firedtv (struct firedtv) has a member for a pointer to
struct device. In this case, we can use dev_dbg() for debug printing.
This is more preferrable behaviour in device driver development.

Signed-off-by: Takashi Sakamoto 
---
 drivers/media/firewire/firedtv-rc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/firewire/firedtv-rc.c
b/drivers/media/firewire/firedtv-rc.c
index f82d4a9..04dea2a 100644
--- a/drivers/media/firewire/firedtv-rc.c
+++ b/drivers/media/firewire/firedtv-rc.c
@@ -184,8 +184,9 @@ void fdtv_handle_rc(struct firedtv *fdtv, unsigned
int code)
else if (code >= 0x4540 && code <= 0x4542)
code = oldtable[code - 0x4521];
else {
-   printk(KERN_DEBUG "firedtv: invalid key code 0x%04x "
-  "from remote control\n", code);
+   dev_dbg(fdtv->device,
+   "invalid key code 0x%04x from remote control\n",
+   code);
return;
}

-- 
2.7.4


Regards

Takashi Sakamoto
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html