Re: [PATCH v4] [media] pci: Add tw5864 driver - fixed few style nits, going to resubmit soon

[Hans Verkuil]

On 07/13/2016 04:05 AM, Andrey Utkin wrote:
> Found and fixed few very minor coding style nits, will resubmit in few days,
> now still waiting for comments to v4.

Can you resubmit now? I plan to review it on Friday or Monday, and I'd rather
review the latest version.

Regards,

Hans

> 
> https://github.com/bluecherrydvr/linux/commits/tw5864
> 
> commit 31f7c98a144cb3fb8a94662f002d9b6142d1f390
> Author: Andrey Utkin 
> Date:   Wed Jul 13 05:00:28 2016 +0300
> 
> Fix checkpatch --strict issue
> 
>  CHECK: Alignment should match open parenthesis
>  #3599: FILE: drivers/media/pci/tw5864/tw5864-video.c:539:
>  +static int tw5864_fmt_vid_cap(struct file *file, void *priv,
>  +   struct v4l2_format *f)
> 
> commit 11a09a1048af597ecf374507b08c809eed91b86d
> Author: Andrey Utkin 
> Date:   Wed Jul 13 04:59:34 2016 +0300
> 
> Fix checkpatch --strict issue
> 
>  CHECK: Please don't use multiple blank lines
>  #3244: FILE: drivers/media/pci/tw5864/tw5864-video.c:184:
> 
> commit 861b2ba8593db7abe89291a4ba85976519783f4a
> Author: Andrey Utkin 
> Date:   Wed Jul 13 04:58:37 2016 +0300
> 
> Fix checkpatch --strict issue
> 
>  CHECK: No space is necessary after a cast
>  #3053: FILE: drivers/media/pci/tw5864/tw5864-util.c:36:
>  +   return (u8) tw_readl(TW5864_IND_DATA);
> --
> 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 v4] [media] pci: Add tw5864 driver - fixed few style nits, going to resubmit soon

[Hans Verkuil]

On 07/13/2016 04:05 AM, Andrey Utkin wrote:
> Found and fixed few very minor coding style nits, will resubmit in few days,
> now still waiting for comments to v4.

Can you resubmit now? I plan to review it on Friday or Monday, and I'd rather
review the latest version.

Regards,

Hans

> 
> https://github.com/bluecherrydvr/linux/commits/tw5864
> 
> commit 31f7c98a144cb3fb8a94662f002d9b6142d1f390
> Author: Andrey Utkin 
> Date:   Wed Jul 13 05:00:28 2016 +0300
> 
> Fix checkpatch --strict issue
> 
>  CHECK: Alignment should match open parenthesis
>  #3599: FILE: drivers/media/pci/tw5864/tw5864-video.c:539:
>  +static int tw5864_fmt_vid_cap(struct file *file, void *priv,
>  +   struct v4l2_format *f)
> 
> commit 11a09a1048af597ecf374507b08c809eed91b86d
> Author: Andrey Utkin 
> Date:   Wed Jul 13 04:59:34 2016 +0300
> 
> Fix checkpatch --strict issue
> 
>  CHECK: Please don't use multiple blank lines
>  #3244: FILE: drivers/media/pci/tw5864/tw5864-video.c:184:
> 
> commit 861b2ba8593db7abe89291a4ba85976519783f4a
> Author: Andrey Utkin 
> Date:   Wed Jul 13 04:58:37 2016 +0300
> 
> Fix checkpatch --strict issue
> 
>  CHECK: No space is necessary after a cast
>  #3053: FILE: drivers/media/pci/tw5864/tw5864-util.c:36:
>  +   return (u8) tw_readl(TW5864_IND_DATA);
> --
> 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 v4] [media] pci: Add tw5864 driver - fixed few style nits, going to resubmit soon

[Andrey Utkin]

Found and fixed few very minor coding style nits, will resubmit in few days,
now still waiting for comments to v4.

https://github.com/bluecherrydvr/linux/commits/tw5864

commit 31f7c98a144cb3fb8a94662f002d9b6142d1f390
Author: Andrey Utkin 
Date:   Wed Jul 13 05:00:28 2016 +0300

Fix checkpatch --strict issue

 CHECK: Alignment should match open parenthesis
 #3599: FILE: drivers/media/pci/tw5864/tw5864-video.c:539:
 +static int tw5864_fmt_vid_cap(struct file *file, void *priv,
 +   struct v4l2_format *f)

commit 11a09a1048af597ecf374507b08c809eed91b86d
Author: Andrey Utkin 
Date:   Wed Jul 13 04:59:34 2016 +0300

Fix checkpatch --strict issue

 CHECK: Please don't use multiple blank lines
 #3244: FILE: drivers/media/pci/tw5864/tw5864-video.c:184:

commit 861b2ba8593db7abe89291a4ba85976519783f4a
Author: Andrey Utkin 
Date:   Wed Jul 13 04:58:37 2016 +0300

Fix checkpatch --strict issue

 CHECK: No space is necessary after a cast
 #3053: FILE: drivers/media/pci/tw5864/tw5864-util.c:36:
 +   return (u8) tw_readl(TW5864_IND_DATA);

Re: [PATCH v4] [media] pci: Add tw5864 driver - fixed few style nits, going to resubmit soon

[Andrey Utkin]

Found and fixed few very minor coding style nits, will resubmit in few days,
now still waiting for comments to v4.

https://github.com/bluecherrydvr/linux/commits/tw5864

commit 31f7c98a144cb3fb8a94662f002d9b6142d1f390
Author: Andrey Utkin 
Date:   Wed Jul 13 05:00:28 2016 +0300

Fix checkpatch --strict issue

 CHECK: Alignment should match open parenthesis
 #3599: FILE: drivers/media/pci/tw5864/tw5864-video.c:539:
 +static int tw5864_fmt_vid_cap(struct file *file, void *priv,
 +   struct v4l2_format *f)

commit 11a09a1048af597ecf374507b08c809eed91b86d
Author: Andrey Utkin 
Date:   Wed Jul 13 04:59:34 2016 +0300

Fix checkpatch --strict issue

 CHECK: Please don't use multiple blank lines
 #3244: FILE: drivers/media/pci/tw5864/tw5864-video.c:184:

commit 861b2ba8593db7abe89291a4ba85976519783f4a
Author: Andrey Utkin 
Date:   Wed Jul 13 04:58:37 2016 +0300

Fix checkpatch --strict issue

 CHECK: No space is necessary after a cast
 #3053: FILE: drivers/media/pci/tw5864/tw5864-util.c:36:
 +   return (u8) tw_readl(TW5864_IND_DATA);

Re: [PATCH v4] [media] pci: Add tw5864 driver

[Andrey Utkin]

On Mon, Jul 11, 2016 at 09:40:53AM -0700, Joe Perches wrote:
> Each of these blocks will start with the dev_ prefix
> and the subsequent lines will not have the same prefix

Yes. I have checked how it looks before submitting, but I didn't see
this as a problem. I don't mind changing that (anyway I have found few
micro-issues with checkpatch --strict and would like to resubmit), but
would like to hear some second opinion.

> It also might be better to issue something like a single
> line dev_warn referring to the driver code and just leave
> this comment in the driver sources.
> 
> Something like:
> 
>   dev_warn(_dev->dev,
>   "This driver has known defects in video quality\n");

Things get complicated if you consider mainstream distros and their
years-behind kernels. The simplest way to preserve correspondence
between state of driver and such notice is to contain the notice in the
compiled driver. I hope the state of affairs will change to better
someday :)

Re: [PATCH v4] [media] pci: Add tw5864 driver

[Andrey Utkin]

On Mon, Jul 11, 2016 at 09:40:53AM -0700, Joe Perches wrote:
> Each of these blocks will start with the dev_ prefix
> and the subsequent lines will not have the same prefix

Yes. I have checked how it looks before submitting, but I didn't see
this as a problem. I don't mind changing that (anyway I have found few
micro-issues with checkpatch --strict and would like to resubmit), but
would like to hear some second opinion.

> It also might be better to issue something like a single
> line dev_warn referring to the driver code and just leave
> this comment in the driver sources.
> 
> Something like:
> 
>   dev_warn(_dev->dev,
>   "This driver has known defects in video quality\n");

Things get complicated if you consider mainstream distros and their
years-behind kernels. The simplest way to preserve correspondence
between state of driver and such notice is to contain the notice in the
compiled driver. I hope the state of affairs will change to better
someday :)

Re: [PATCH v4] [media] pci: Add tw5864 driver

[Joe Perches]

On Mon, 2016-07-11 at 18:17 +0300, Andrey Utkin wrote:
[]
> diff --git a/drivers/media/pci/tw5864/tw5864-core.c 
> b/drivers/media/pci/tw5864/tw5864-core.c
[]
> +static const char * const artifacts_warning =
> +"BEWARE OF KNOWN ISSUES WITH VIDEO QUALITY\n"
> +"\n"
> +"This driver was developed by Bluecherry LLC by deducing behaviour of\n"
> +"original manufacturer's driver, from both source code and execution 
> traces.\n"
> +"It is known that there are some artifacts on output video with this 
> driver:\n"
> +" - on all known hardware samples: random pixels of wrong color (mostly\n"
> +"   white, red or blue) appearing and disappearing on sequences of 
> P-frames;\n"
> +" - on some hardware samples (known with H.264 core version e006:2800):\n"
> +"   total madness on P-frames: blocks of wrong luminance; blocks of wrong\n"
> +"   colors \"creeping\" across the picture.\n"
> +"There is a workaround for both issues: avoid P-frames by setting GOP size\n"
> +"to 1. To do that, run this command on device files created by this 
> driver:\n"
> +"\n"
> +"v4l2-ctl --device /dev/videoX --set-ctrl=video_gop_size=1\n"
> +"\n";
> +
> +static char *artifacts_warning_continued =
> +"These issues are not decoding errors; all produced H.264 streams are 
> decoded\n"
> +"properly. Streams without P-frames don't have these artifacts so it's not\n"
> +"analog-to-digital conversion issues nor internal memory errors; we 
> conclude\n"
> +"it's internal H.264 encoder issues.\n"
> +"We cannot even check the original driver's behaviour because it has never\n"
> +"worked properly at all in our development environment. So these issues 
> may\n"
> +"be actually related to firmware or hardware. However it may be that 
> there's\n"
> +"just some more register settings missing in the driver which would please\n"
> +"the hardware.\n"
> +"Manufacturer didn't help much on our inquiries, but feel free to disturb\n"
> +"again the support of Intersil (owner of former Techwell).\n"
> +"\n";
[]
> +static int tw5864_initdev(struct pci_dev *pci_dev,
> +   const struct pci_device_id *pci_id)
> +{
[]
> + dev_warn(_dev->dev, "%s", artifacts_warning);
> + dev_warn(_dev->dev, "%s", artifacts_warning_continued);

Is all that verbosity useful?

And trivially:

Each of these blocks will start with the dev_ prefix
and the subsequent lines will not have the same prefix

Perhaps it'd be better to write this something like:

static const char * const artifacts_warning[] = {
"BEWARE OF KNOWN ISSUES WITH VIDEO QUALITY",
"",
"This driver was developed by Bluecherry LLC by deducing behaviour of",
"original manufacturer's driver, from both source code and execution 
traces.",
"It is known that there are some artifacts on output video with this 
driver:",
" - on all known hardware samples: random pixels of wrong color 
(mostly",
"   white, red or blue) appearing and disappearing on sequences of 
P-frames;",
" - on some hardware samples (known with H.264 core version 
e006:2800):",
"   total madness on P-frames: blocks of wrong luminance; blocks of 
wrong",
"   colors \"creeping\" across the picture.",
"There is a workaround for both issues: avoid P-frames by setting GOP 
size",
"to 1. To do that, run this command on device files created by this 
driver:",
"",
"v4l2-ctl --device /dev/videoX --set-ctrl=video_gop_size=1",
"",
"These issues are not decoding errors; all produced H.264 streams are 
decoded",
"properly. Streams without P-frames don't have these artifacts so it's 
not",
"analog-to-digital conversion issues nor internal memory errors; we 
conclude",
"it's internal H.264 encoder issues.",
"We cannot even check the original driver's behaviour because it has 
never",
"worked properly at all in our development environment. So these issues 
may",
"be actually related to firmware or hardware. However it may be that 
there's",
"just some more register settings missing in the driver which would 
please",
"the hardware.",
"Manufacturer didn't help much on our inquiries, but feel free to 
disturb",
"again the support of Intersil (owner of former Techwell).\n"
};

and use

for (i = 0; i < ARRAY_SIZE(artifacts_warning), i++)
dev_warn(_dev->dev, %s\n", artifacts_warning[i]);

so that each line is prefixed.

It also might be better to issue something like a single
line dev_warn referring to the driver code and just leave
this comment in the driver sources.

Something like:

dev_warn(_dev->dev,
"This driver has known defects in video quality\n");

Re: [PATCH v4] [media] pci: Add tw5864 driver

[Joe Perches]

On Mon, 2016-07-11 at 18:17 +0300, Andrey Utkin wrote:
[]
> diff --git a/drivers/media/pci/tw5864/tw5864-core.c 
> b/drivers/media/pci/tw5864/tw5864-core.c
[]
> +static const char * const artifacts_warning =
> +"BEWARE OF KNOWN ISSUES WITH VIDEO QUALITY\n"
> +"\n"
> +"This driver was developed by Bluecherry LLC by deducing behaviour of\n"
> +"original manufacturer's driver, from both source code and execution 
> traces.\n"
> +"It is known that there are some artifacts on output video with this 
> driver:\n"
> +" - on all known hardware samples: random pixels of wrong color (mostly\n"
> +"   white, red or blue) appearing and disappearing on sequences of 
> P-frames;\n"
> +" - on some hardware samples (known with H.264 core version e006:2800):\n"
> +"   total madness on P-frames: blocks of wrong luminance; blocks of wrong\n"
> +"   colors \"creeping\" across the picture.\n"
> +"There is a workaround for both issues: avoid P-frames by setting GOP size\n"
> +"to 1. To do that, run this command on device files created by this 
> driver:\n"
> +"\n"
> +"v4l2-ctl --device /dev/videoX --set-ctrl=video_gop_size=1\n"
> +"\n";
> +
> +static char *artifacts_warning_continued =
> +"These issues are not decoding errors; all produced H.264 streams are 
> decoded\n"
> +"properly. Streams without P-frames don't have these artifacts so it's not\n"
> +"analog-to-digital conversion issues nor internal memory errors; we 
> conclude\n"
> +"it's internal H.264 encoder issues.\n"
> +"We cannot even check the original driver's behaviour because it has never\n"
> +"worked properly at all in our development environment. So these issues 
> may\n"
> +"be actually related to firmware or hardware. However it may be that 
> there's\n"
> +"just some more register settings missing in the driver which would please\n"
> +"the hardware.\n"
> +"Manufacturer didn't help much on our inquiries, but feel free to disturb\n"
> +"again the support of Intersil (owner of former Techwell).\n"
> +"\n";
[]
> +static int tw5864_initdev(struct pci_dev *pci_dev,
> +   const struct pci_device_id *pci_id)
> +{
[]
> + dev_warn(_dev->dev, "%s", artifacts_warning);
> + dev_warn(_dev->dev, "%s", artifacts_warning_continued);

Is all that verbosity useful?

And trivially:

Each of these blocks will start with the dev_ prefix
and the subsequent lines will not have the same prefix

Perhaps it'd be better to write this something like:

static const char * const artifacts_warning[] = {
"BEWARE OF KNOWN ISSUES WITH VIDEO QUALITY",
"",
"This driver was developed by Bluecherry LLC by deducing behaviour of",
"original manufacturer's driver, from both source code and execution 
traces.",
"It is known that there are some artifacts on output video with this 
driver:",
" - on all known hardware samples: random pixels of wrong color 
(mostly",
"   white, red or blue) appearing and disappearing on sequences of 
P-frames;",
" - on some hardware samples (known with H.264 core version 
e006:2800):",
"   total madness on P-frames: blocks of wrong luminance; blocks of 
wrong",
"   colors \"creeping\" across the picture.",
"There is a workaround for both issues: avoid P-frames by setting GOP 
size",
"to 1. To do that, run this command on device files created by this 
driver:",
"",
"v4l2-ctl --device /dev/videoX --set-ctrl=video_gop_size=1",
"",
"These issues are not decoding errors; all produced H.264 streams are 
decoded",
"properly. Streams without P-frames don't have these artifacts so it's 
not",
"analog-to-digital conversion issues nor internal memory errors; we 
conclude",
"it's internal H.264 encoder issues.",
"We cannot even check the original driver's behaviour because it has 
never",
"worked properly at all in our development environment. So these issues 
may",
"be actually related to firmware or hardware. However it may be that 
there's",
"just some more register settings missing in the driver which would 
please",
"the hardware.",
"Manufacturer didn't help much on our inquiries, but feel free to 
disturb",
"again the support of Intersil (owner of former Techwell).\n"
};

and use

for (i = 0; i < ARRAY_SIZE(artifacts_warning), i++)
dev_warn(_dev->dev, %s\n", artifacts_warning[i]);

so that each line is prefixed.

It also might be better to issue something like a single
line dev_warn referring to the driver code and just leave
this comment in the driver sources.

Something like:

dev_warn(_dev->dev,
"This driver has known defects in video quality\n");