Re: [PATCH] [media] : Removing warnings caught by checkpatch.pl

[Andrey Utkin]

On Sun, Oct 02, 2016 at 02:30:45AM +0530, Harman Kalra wrote:
>  static int iss_video_queue_setup(struct vb2_queue *vq,
> -  unsigned int *count, unsigned int *num_planes,
> -  unsigned int sizes[], struct device 
> *alloc_devs[])
> + unsigned int *count, unsigned int *num_planes,
> + unsigned int sizes[], struct device *alloc_devs[])

2 spaces + 3 tabs -> 2 spaces + 2 tabs? Am I seeing this correctly?
Both ways are against CodingStyle.

> - /* Try the get selection operation first and fallback to get format if 
> not
> -  * implemented.
> + /* Try the get selection operation first and
> +  * fallback to get format if not implemented.
>*/

This comment style is discouraged so this is at lease not perfect.
Doesn't checkpatch complain with this change? If it doesn't, could you
please also check with --strict, as long as you're working on style.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] drivers: wlan-ng: fixed a coding style issue

[Andrey Utkin]

On Thu, Sep 22, 2016 at 10:52:18PM +0200, becher.jan...@gmail.com wrote:
> I always wondered why I shouldn't make more than one change in a patch,
> but in all talks I watched they said that it's easier for them to merge
> small patches.
> So you think I should make one "big" patch and resend it?
> Thank you for taking your time.

There is "big" and then there is "big".
Walking through entire driver fixing all occurances of _same_ issues
really should be done at once. Trivial style fixes don't usually go in
huge series of commits (but BTW if you send several related patches,
please send them as series at once). And leave granularity to
conceptually different changes.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] drivers: wlan-ng: fixed a coding style issue

[Andrey Utkin]

On Thu, Sep 22, 2016 at 09:11:46PM +0200, Jannik Becher wrote:
> removed a space after a cast to obtain the coding style.
> 
> Signed-off-by: Jannik Becher 
> ---
>  drivers/staging/wlan-ng/p80211req.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/wlan-ng/p80211req.c 
> b/drivers/staging/wlan-ng/p80211req.c
> index 40627d5..ecd2fff 100644
> --- a/drivers/staging/wlan-ng/p80211req.c
> +++ b/drivers/staging/wlan-ng/p80211req.c
> @@ -110,7 +110,7 @@ static void p80211req_handle_action(struct wlandevice 
> *wlandev, u32 *data,
>  */
>  int p80211req_dorequest(struct wlandevice *wlandev, u8 *msgbuf)
>  {
> - struct p80211msg *msg = (struct p80211msg *) msgbuf;
> + struct p80211msg *msg = (struct p80211msg *)msgbuf;
>  
>   /* Check to make sure the MSD is running */
>   if (!((wlandev->msdstate == WLAN_MSD_HWPRESENT &&

checkpatch.pl shows that in this file there are other occurances of
style deviations - of same or other kinds. Could you please present work
on wider scope, e.g. on whole wlan-ng/, or at last making this one file
to pass checkpatch.pl --strict cleanly?

Not that I don't appreciate this small achievement, but I believe
maintainers won't merge this one, exactly because there is more work of
same kind in the same file.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: dgnc: Fix multi line comment alignment

[Andrey Utkin]

On Tue, Sep 06, 2016 at 03:19:03PM +0200, Fernando Apesteguia wrote:
> Sorry, but I fail to see how I can align all the lines by changing just the 
> first and last ones.

Ah now I see that original version doesn't have spaces before asterisk.
Sorry for noise.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: dgnc: Fix multi line comment alignment

[Andrey Utkin]

On Mon, Sep 05, 2016 at 08:28:32PM +0200, Fernando Apesteguia wrote:
> Fix alignment in multi line comment block.
> 
> Remove extra '*' to use the preferred comment style as in 
> Documentation/CodingStyle
> 
> Signed-off-by: Fernando Apesteguia 
> 
> ---
>  drivers/staging/dgnc/dgnc_driver.c | 50 
> +++---
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/staging/dgnc/dgnc_driver.c 
> b/drivers/staging/dgnc/dgnc_driver.c
> index cc6105a..01e948c 100644
> --- a/drivers/staging/dgnc/dgnc_driver.c
> +++ b/drivers/staging/dgnc/dgnc_driver.c
> @@ -602,31 +602,31 @@ static void dgnc_do_remap(struct dgnc_board *brd)
>   brd->re_map_membase = ioremap(brd->membase, 0x1000);
>  }
>  
> -/*
> -*
> -* Function:
> -*
> -*dgnc_poll_handler
> -*
> -* Author:
> -*
> -*Scott H Kilau
> -*
> -* Parameters:
> -*
> -*dummy -- ignored
> -*
> -* Return Values:
> -*
> -*none
> -*
> -* Description:
> -*
> -*As each timer expires, it determines (a) whether the "transmit"
> -*waiter needs to be woken up, and (b) whether the poller needs to
> -*be rescheduled.
> -*
> -**/
> +/*
> + *
> + * Function:
> + *
> + *dgnc_poll_handler
> + *
> + * Author:
> + *
> + *Scott H Kilau
> + *
> + * Parameters:
> + *
> + *dummy -- ignored
> + *
> + * Return Values:
> + *
> + *none
> + *
> + * Description:
> + *
> + *As each timer expires, it determines (a) whether the "transmit"
> + *waiter needs to be woken up, and (b) whether the poller needs to
> + *be rescheduled.
> + *
> + */
>  
>  static void dgnc_poll_handler(ulong dummy)
>  {
> -- 
> 2.7.4

Changing first and last lines of that comment should be enough, but all
lines got changed. I'd (re)submit it with just first and last lines
changed.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/7] staging: comedi: jr3_pci.h: Fix checkpatch warning

[Andrey Utkin]

On Fri, Aug 26, 2016 at 06:35:51PM -0400, Anson Jacob wrote:
> I only found one trailing whitespace issue in the patchset.

Yes, there's just one.

> Are my commits are too long. Am I supposed to submit shorter ones.

In my opinion your commits are generally fine.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/7] staging: comedi: jr3_pci.h: Fix checkpatch warning

[Andrey Utkin]

On Fri, Aug 26, 2016 at 02:33:08PM -0400, Anson Jacob wrote:
>  
> - /* Default_FS contains the full scale that is used if the user does */
> - /* not set a full scale. */
> + /* 

Applying: staging: comedi: jr3_pci.h: Fix checkpatch warning
.git/rebase-apply/patch:226: trailing whitespace.
/* 
warning: 1 line adds whitespace errors.

This means you have a space after asterisk. You would catch this if you run
checkpatch.pl after making your commits.

Actually, some files you have amended, still have issues of same difficulty.
You are encouraged to sort that out, too. See

for x in drivers/staging/comedi/drivers/cb_pcidas64.c 
drivers/staging/comedi/drivers/jr3_pci.c 
drivers/staging/comedi/drivers/jr3_pci.h 
drivers/staging/comedi/drivers/ni_at_a2150.c 
drivers/staging/comedi/drivers/ni_atmio.c 
drivers/staging/comedi/drivers/s626.h; do ./scripts/checkpatch.pl --strict -f 
$x; done
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: comedi: ni_at_a2150.c: Fix checkpatch warning

[Andrey Utkin]

On Thu, Aug 25, 2016 at 12:09:23PM -0400, Anson Jacob wrote:
> On Thu, Aug 25, 2016 at 06:47:08PM +0300, Andrey Utkin wrote:
> > 
> > > +#define CHANNEL_BITS(x)  ((x) & 0x7)
> > > +#define CHANNEL_MASK 0x7
> > 
> > No uniform alignment. Please get everything in a row.
> > If it's hard or this part of driver is expected to have a lot of changes
> > in near future, then I'd remove any whitespace over single space between
> > name and value.
> >
> 
> I didn't get your point in this case. Could you explain it.

Reviewing actual source code with the patch applied, it looks well.  But
if you look at the patch email itself, or in "git show , the
alignment is broken. This is an illustration why using tab characters in
the middle of lines is bad idea. However this is what I see in lots of
kernel code.
So feel free to ignore this point.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: comedi: ni_at_a2150.c: Fix checkpatch warning

[Andrey Utkin]

On Thu, Aug 25, 2016 at 11:16:13AM -0400, Anson Jacob wrote:
> Fix checkpatch.pl warning 'line over 80 characters'
> 
> Signed-off-by: Anson Jacob 
> ---
>  drivers/staging/comedi/drivers/ni_at_a2150.c | 82 
> 
>  1 file changed, 46 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/ni_at_a2150.c 
> b/drivers/staging/comedi/drivers/ni_at_a2150.c
> index 957fb9f..3c00de1 100644
> --- a/drivers/staging/comedi/drivers/ni_at_a2150.c
> +++ b/drivers/staging/comedi/drivers/ni_at_a2150.c
> @@ -58,41 +58,49 @@
>  
>  /* Registers and bits */
>  #define CONFIG_REG   0x0
> -#define   CHANNEL_BITS(x)((x) & 0x7)
> -#define   CHANNEL_MASK   0x7
> -#define   CLOCK_SELECT_BITS(x)   (((x) & 0x3) << 3)

You have lost this treeish look which carried information about thing
being a register or a field.

> +#define CHANNEL_BITS(x)  ((x) & 0x7)
> +#define CHANNEL_MASK 0x7

No uniform alignment. Please get everything in a row.
If it's hard or this part of driver is expected to have a lot of changes
in near future, then I'd remove any whitespace over single space between
name and value.

> +#define ENABLE0_BIT  0x80/* enable (don't internally ground)
> +  * channels 0 and 1
> +  */
> +#define ENABLE1_BIT  0x100   /* enable (don't internally ground)
> +  * channels 2 and 3
> +  */

This commenting style is discouraged (Linus has even stated explicitly
that he dislikes it, even for pieces of code which historically had this
style everywhere). Opening "/*" should not be followed by text.

Also, In my personal experience, it is more stable to have these
comments on previous line, not at end of the line. This way you always
have enough space for both a comment and a macro.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: lowmemorykiller.c: Fix checkpatch warning

[Andrey Utkin]

On Thu, Aug 25, 2016 at 11:10:25AM -0400, Anson Jacob wrote:
> Fix checkpatch.pl 'line over 80 characters' warning
> 
> Signed-off-by: Anson Jacob 
> ---
>  drivers/staging/android/lowmemorykiller.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/android/lowmemorykiller.c 
> b/drivers/staging/android/lowmemorykiller.c
> index 45a1b4e..80d7adf 100644
> --- a/drivers/staging/android/lowmemorykiller.c
> +++ b/drivers/staging/android/lowmemorykiller.c
> @@ -92,8 +92,8 @@ static unsigned long lowmem_scan(struct shrinker *s, struct 
> shrink_control *sc)
>   int array_size = ARRAY_SIZE(lowmem_adj);
>   int other_free = global_page_state(NR_FREE_PAGES) - totalreserve_pages;
>   int other_file = global_node_page_state(NR_FILE_PAGES) -
> - 
> global_node_page_state(NR_SHMEM) -
> - total_swapcache_pages();
> + global_node_page_state(NR_SHMEM) -
> + total_swapcache_pages();
>  
>   if (lowmem_adj_size < array_size)
>   array_size = lowmem_adj_size;

Indeed original alignment on opening brace here doesn't make sense.
I don't mind if your patch gets accepted, but out of curiosity I have opened
this file in vim with kernel coding style plugin loaded
(https://github.com/vivien/vim-linux-coding-style), selected the affected
lines, and reindented the selection by pressing 'gq'. It ended up with this,
being both shorter and simpler:

diff --git a/drivers/staging/android/lowmemorykiller.c 
b/drivers/staging/android/lowmemorykiller.c
index 45a1b4e..3aef467 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -92,8 +92,7 @@ static unsigned long lowmem_scan(struct shrinker *s, struct 
shrink_control *sc)
int array_size = ARRAY_SIZE(lowmem_adj);
int other_free = global_page_state(NR_FREE_PAGES) - totalreserve_pages;
int other_file = global_node_page_state(NR_FILE_PAGES) -
-   
global_node_page_state(NR_SHMEM) -
-   total_swapcache_pages();
+   global_node_page_state(NR_SHMEM) - total_swapcache_pages();
 
if (lowmem_adj_size < array_size)
array_size = lowmem_adj_size;

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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(&pci_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 :)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] Add tw5864 driver

[Andrey Utkin]

Thanks for review Hans!

On Mon, Jul 11, 2016 at 07:58:38AM +0200, Hans Verkuil wrote:
> > +" v4l2-ctl --device $dev --set-ctrl=video_gop_size=1; done\n"
> 
> Replace $dev by /dev/videoX
> 
> Wouldn't it make more sense to default to this? And show the warning only if
> P-frames are enabled?

I believe it's better to leave P-frames on by default. All-I-frames
stream has huge bitrate. And the pixels artifacts is not very strong,
it's 0 - 10 bad pixels on picture at same time in our dev environment,
and probably up to 50 bad pixels max in other environments I know of.

> > +   dma_sync_single_for_cpu(&dev->pci->dev, cur_frame->vlc.dma_addr,
> > +   H264_VLC_BUF_SIZE, DMA_FROM_DEVICE);
> > +   dma_sync_single_for_cpu(&dev->pci->dev, cur_frame->mv.dma_addr,
> > +   H264_MV_BUF_SIZE, DMA_FROM_DEVICE);
> 
> This is almost certainly the wrong place. This should probably happen in the
> tasklet. The tasklet runs after the isr, so by the time the tasklet runs
> you've already called dma_sync_single_for_device.

Thanks, moved to tasklet subroutine tw5864_handle_frame().

I didn't seem to me like dma_sync_single_for_* can take long time or be
otherwise bad to be done from interrupt context.

> > +static int tw5864_querycap(struct file *file, void *priv,
> > +  struct v4l2_capability *cap)
> > +{
> > +   struct tw5864_input *input = video_drvdata(file);
> > +
> > +   strcpy(cap->driver, "tw5864");
> > +   snprintf(cap->card, sizeof(cap->card), "TW5864 Encoder %d",
> > +input->nr);
> > +   sprintf(cap->bus_info, "PCI:%s", pci_name(input->root->pci));
> > +   cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_READWRITE |
> > +   V4L2_CAP_STREAMING;
> > +   cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> 
> This line can be dropped, the core will fill in the capabilities field for 
> you.

No, removing this line causes v4l2-compliance failures and also ffmpeg fails to
play the device.

Required ioctls:
fail: v4l2-compliance.cpp(550): dcaps & ~caps
test VIDIOC_QUERYCAP: FAIL

Allow for multiple opens:
test second video open: OK
fail: v4l2-compliance.cpp(550): dcaps & ~caps
test VIDIOC_QUERYCAP: FAIL
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] Add tw5864 driver

[Andrey Utkin]

Hi Hans,

Thanks for great help.
I believe the issues highlighted by your are rectified by now.

One chunk of your proposed changes seems to be wrong.

Also I have one non-technical change I want to introduce to this driver, see it
in the bottom of this letter ("Also, I decided to document known video quality
issues in a printed warning...").

On Fri, Jul 01, 2016 at 03:35:40PM +0200, Hans Verkuil wrote:
> On 06/10/2016 12:11 AM, Andrey Utkin wrote:
> > +   cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> 
> This line can be dropped: the v4l2 core will do this automatically.

This seems not so: dropping it resulted in new compliance fails:

Required ioctls:
fail: v4l2-compliance.cpp(550): dcaps & ~caps
test VIDIOC_QUERYCAP: FAIL

Allow for multiple opens:
test second video open: OK
fail: v4l2-compliance.cpp(550): dcaps & ~caps
test VIDIOC_QUERYCAP: FAIL


I am running latest v4l-utils from git.
This particular fail happens on kernels built from next-20160707 and
next-20160609.

BTW next-20160707 makes my dev machine to hang after few minutes of uptime,
regardless of my module being loaded, so for now I am testing driver on
next-20160609.
This (running old linux-next) causes such new fail with latest v4l-utils:

fail: v4l2-test-buffers.cpp(293): g_flags() & V4L2_BUF_FLAG_DONE

which is understandable because of recent commit to v4l-utils flipping expected
behaviour in this regard:

commit 7d784c6894b10cdf5ec025c2cd7c320320f5f658
Author: Hans Verkuil 
Date:   Fri Jul 8 23:10:34 2016 +0200

v4l2-compliance: fix a check for the DONE flag

This was always set by vb2 drivers due to a bug. It is now cleared
again after that bug was fixed, but the test should now be inverted.

Signed-off-by: Hans Verkuil 

diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp 
b/utils/v4l2-compliance/v4l2-test-buffers.cpp
index fb14170..dc82918 100644
--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -290,7 +290,7 @@ int buffer::check(unsigned type, unsigned memory, unsigned 
index,
fail_on_test(g_bytesused(p) > g_length(p));
}
fail_on_test(!g_timestamp().tv_sec && !g_timestamp().tv_usec);
-   fail_on_test(!(g_flags() & V4L2_BUF_FLAG_DONE));
+   fail_on_test(g_flags() & V4L2_BUF_FLAG_DONE);
fail_on_test((int)g_sequence() < seq.last_seq + 1);
if (v4l_type_is_video(g_type())) {
fail_on_test(g_field() == V4L2_FIELD_ALTERNATE);

So please expect this fail in v4l2-compliance logs of my new submission.



Also, I decided to document known video quality issues in a printed warning; I
like how it looks now both in code and in dmesg, but checkpatch.pl doesn't like
it. See commit at
https://github.com/bluecherrydvr/linux/commit/83395b6c5e1e5ceb642c9a04a28db5fc22566c87

The message is split in pieces because otherwise it gets truncated.

I'd like some approval or suggestion for rework on this.

It looks like this in dmesg:

[ 5101.182151] tw5864 :06:07.0: 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 such command on device files created by this 
driver:

   for dev in /dev/video*; do v4l2-ctl --device $dev 
--set-ctrl=video_gop_size=1; done

[ 5101.357312] systemd-journald[219]: Compressed data object 850 -> 636 using XZ
[ 5101.471071] tw5864 :06:07.0: 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 su

Re: [Kernel] Mainlining of Pyra nub joystick driver

[Andrey Utkin]

On Tue, Jun 14, 2016 at 07:09:50PM +0200, H. Nikolaus Schaller wrote:
> 
> > Am 14.06.2016 um 19:02 schrieb Andrey Utkin :
> > Update: found drivers/input/joystick/as5011.c in mainline kernel,
> > will look how it works with as5013 hardware.
> 
> Before you spend too much time on it, they have not much in
> common except the manufacturer and the as501x designation.
> 
> Register models (data sheet) are very different.

Thanks for this hint, Nikolaus!

But still, apart from register models I guess as5011 driver is a good
starting point for a fork. I am considering this because
 - as5011 driver has support for button push event, and as5013 doesn't;
 - as5011 driver conforms to mainline standards much better than
   existing out-of-tree as5013 driver. It also conforms to what
   reviewers expect to see: anyway as5013 won't pass review unless all
   tricky modes are dropped. This is according to Arnd reply; Pyra dev
   team member aTc also votes for userspace input management daemon.

I am considering forking instead of adding conditional logic to as5011
because I don't have a chance to test as5011 hardware, so I could break
it unintentionally and never know about that. This is serious because
I'm going to add support of all power modes and other parameters
eventually.

There is one thing which both as5011 and as5013 existing drivers do:
expose a header file in include/linux/input/, with some "struct
blah_blah_platform_data" declaration. This header file is not included
anywhere except for corresponding driver's .c file. So I think this
header file is not needed, and all its contents should be inlined into
single driver's source file. From asking on #kernelnewbies IRC chat, I
gained some confidence that there's no subtleties regarding this.

I guess "struct drivername_platform_data" is something deprecated by
now, as long as there are devicetree files and of_...() API, so I should
port vsense_dt_init() logic from existing as5013 driver?

Also info about GPIO pin for button push interrupt should be added to
devicetree files. Or please indicate that it's not going to have its
GPIO pin for button interrupt - then driver should check button status
at once with periodic position check.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Kernel] Mainlining of Pyra nub joystick driver

[Andrey Utkin]

On Tue, Jun 14, 2016 at 02:45:23PM +0300, Andrey Utkin wrote:
> There's a pair of "nub" devices on Pyra handheld PC
> ...

Update: found drivers/input/joystick/as5011.c in mainline kernel,
will look how it works with as5013 hardware.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Mainlining of Pyra nub joystick driver

[Andrey Utkin]

There's a pair of "nub" devices on Pyra handheld PC
(https://pyra-handheld.com/), and there's driver for nub, which is going
to be reworked for upstreaming. While the device itself fits most to
"joystick" category, the computer itself lacks touchpad and mouse
buttons, and the existing driver is capable of switching between modes, in
which it shows up like one of the following:
 - scrolling wheel,
 - mouse buttons set,
 - pointer updating its absolute position (graphic pad alike AFAIU),
 - pointer device behaving like actual joystick / pointing stick.

Currently modes switching happens through r/w file in /proc, which is of
couse going to be changed.

I wonder if such mode switching mechanism is tolerable for inclusion to
upstream kernel in this case. I'd like some advice how to rearrange the
driver to save most of flexibility while matching upstream kernel
conventions. I am especially interested in comments from subsystem
maintainers.

Existing driver:
http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=drivers/input/misc/as5013.c;h=1bfb5243f692c0c0a9c93881968849cac947c92d;hb=refs/heads/work/hns/input/as5013
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] Add tw5864 driver

[Andrey Utkin]

Update: added local change
 - to require fewer DMA buffers;
 - to require fewer vb2_queue buffers;
 - don't require vb2_queue buffers to be DMA-capable.
https://github.com/bluecherrydvr/linux/commit/410a86b08d230ff2a401ac9f5be3b30f8b29f30d
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

How should I use kernel-defined i2c structs in this driver

[Andrey Utkin]

Could anybody please give a hint - which kernel-defined i2c objects, and how
many of them, I need to define and use to substitute these driver-defined
functions i2c_read(), i2c_write() ?
https://github.com/bluecherrydvr/linux/blob/release/tw5864/1.16/drivers/media/pci/tw5864/tw5864-config.c
In a word, there's 4 chips with different addresses, to which this code
communicates via main chip's dedicated registers.
Do i need a single i2c_adapter or several?
Do i need i2c_client entities?
where should I put what is named "devid" here?

Thanks in advance.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] Add tw5864 driver - cover letter

[Andrey Utkin]

This is a driver for multimedia devices based on Techwell/Intersil TW5864 chip.

It is basically written from scratch. There was an awful reference driver for
2.6 kernel, which is nearly million lines of code and requires half a dozen
special userspace libraries, and still doesn't quite work. So currently
submitted driver is a product of reverse-engineering and heuristics. tw68
driver was used as code skeleton.

The device advertises many capabilities, but this version of driver only
supports H.264 encoding of captured video channels.

There is one known issue, which reproduces on two of five setups of which I
know: P-frames are distorted, but I-frames are fine. Changing quality and
framerate settings does not affect this. Currently such workaround is used:

v4l2-ctl -d /dev/video$n -c video_gop_size=1

GOP size is set to 1, so that every output frame is I-frame. 
We are regularly contacting manufacturer regarding such issues, but
unfortunately they can do little to help us.


Andrey Utkin (1):
  Add tw5864 driver

 MAINTAINERS  |7 +
 drivers/staging/media/Kconfig|2 +
 drivers/staging/media/Makefile   |1 +
 drivers/staging/media/tw5864/Kconfig |   11 +
 drivers/staging/media/tw5864/Makefile|3 +
 drivers/staging/media/tw5864/tw5864-bs.h |  154 ++
 drivers/staging/media/tw5864/tw5864-config.c |  359 +
 drivers/staging/media/tw5864/tw5864-core.c   |  453 ++
 drivers/staging/media/tw5864/tw5864-h264.c   |  183 +++
 drivers/staging/media/tw5864/tw5864-reg.h| 2200 ++
 drivers/staging/media/tw5864/tw5864-tables.h |  237 +++
 drivers/staging/media/tw5864/tw5864-video.c  | 1364 
 drivers/staging/media/tw5864/tw5864.h|  280 
 include/linux/pci_ids.h  |1 +
 14 files changed, 5255 insertions(+)
 create mode 100644 drivers/staging/media/tw5864/Kconfig
 create mode 100644 drivers/staging/media/tw5864/Makefile
 create mode 100644 drivers/staging/media/tw5864/tw5864-bs.h
 create mode 100644 drivers/staging/media/tw5864/tw5864-config.c
 create mode 100644 drivers/staging/media/tw5864/tw5864-core.c
 create mode 100644 drivers/staging/media/tw5864/tw5864-h264.c
 create mode 100644 drivers/staging/media/tw5864/tw5864-reg.h
 create mode 100644 drivers/staging/media/tw5864/tw5864-tables.h
 create mode 100644 drivers/staging/media/tw5864/tw5864-video.c
 create mode 100644 drivers/staging/media/tw5864/tw5864.h

-- 
2.7.1.380.g0fea050.dirty

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] Add tw5864 driver - cover letter

[Andrey Utkin]

From: Andrey Utkin 

This is a driver for multimedia devices based on Techwell/Intersil TW5864 chip.

It is basically written from scratch. There was an awful reference driver for
2.6 kernel, which is nearly million lines of code and requires half a dozen
special userspace libraries, and still doesn't quite work. So currently
submitted driver is a product of reverse-engineering and heuristics. tw68
driver was used as code skeleton.

The device advertises many capabilities, but this version of driver only
supports H.264 encoding of captured video channels.

There is one known issue, which reproduces on two of five setups of which I
know: P-frames are distorted, but I-frames are fine. Changing quality and
framerate settings does not affect this. Currently such workaround is used:

v4l2-ctl -d /dev/video$n -c video_gop_size=1

GOP size is set to 1, so that every output frame is I-frame. 
We are regularly contacting manufacturer regarding such issues, but
unfortunately they can do little to help us.


Andrey Utkin (1):
  Add tw5864 driver

 MAINTAINERS  |7 +
 drivers/staging/media/Kconfig|2 +
 drivers/staging/media/Makefile   |1 +
 drivers/staging/media/tw5864/Kconfig |   11 +
 drivers/staging/media/tw5864/Makefile|3 +
 drivers/staging/media/tw5864/tw5864-bs.h |  154 ++
 drivers/staging/media/tw5864/tw5864-config.c |  359 +
 drivers/staging/media/tw5864/tw5864-core.c   |  453 ++
 drivers/staging/media/tw5864/tw5864-h264.c   |  183 +++
 drivers/staging/media/tw5864/tw5864-reg.h| 2200 ++
 drivers/staging/media/tw5864/tw5864-tables.h |  237 +++
 drivers/staging/media/tw5864/tw5864-video.c  | 1364 
 drivers/staging/media/tw5864/tw5864.h|  280 
 include/linux/pci_ids.h  |1 +
 14 files changed, 5255 insertions(+)
 create mode 100644 drivers/staging/media/tw5864/Kconfig
 create mode 100644 drivers/staging/media/tw5864/Makefile
 create mode 100644 drivers/staging/media/tw5864/tw5864-bs.h
 create mode 100644 drivers/staging/media/tw5864/tw5864-config.c
 create mode 100644 drivers/staging/media/tw5864/tw5864-core.c
 create mode 100644 drivers/staging/media/tw5864/tw5864-h264.c
 create mode 100644 drivers/staging/media/tw5864/tw5864-reg.h
 create mode 100644 drivers/staging/media/tw5864/tw5864-tables.h
 create mode 100644 drivers/staging/media/tw5864/tw5864-video.c
 create mode 100644 drivers/staging/media/tw5864/tw5864.h

-- 
2.7.1.380.g0fea050.dirty

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH v0] Add tw5864 driver

[Andrey Utkin]

On Fri, 11 Mar 2016 10:59:02 +0100
Hans Verkuil  wrote:
> While userspace may specify FIELD_ANY when setting a format, the
> driver should always map that to a specific field setting and should
> never return FIELD_ANY back to userspace.
> 
> In this case, the 'field' field of the v4l2_buffer struct has
> FIELD_ANY which means it is not set correctly (or at all) in the
> driver.
> 
> It's a common mistake, which is why v4l2-compliance tests for it :-)

Thanks for great guidance Hans, finally I have solved all issues.

You can review latest state at tw5864 branch, also you can review
changelog of v4l2-compliance fixing at tags tw5864_pre_1.11,
tw5864_pre_1.10 of https://github.com/bluecherrydvr/linux .

I will make a final internal review before submission, and try
to submit the driver for inclusion.

Everybody is appreciated to make any comments even before submission,
the actual code to review is at tw5864 branch.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH v0] Add tw5864 driver

[Andrey Utkin]

On Fri, 11 Mar 2016 09:00:18 +0100
Hans Verkuil  wrote:
> The reason is likely to be the tw5864_queue_setup function which has
> not been updated to handle CREATE_BUFS support correctly. It should
> look like this:
> 
> static int tw5864_queue_setup(struct vb2_queue *q,
> unsigned int *num_buffers,
> unsigned int *num_planes, unsigned int
> sizes[], void *alloc_ctxs[])
> {
>   struct tw5864_input *dev = vb2_get_drv_priv(q);
> 
>   if (q->num_buffers + *num_buffers < 12)
>   *num_buffers = 12 - q->num_buffers;
> 
>   alloc_ctxs[0] = dev->alloc_ctx;
>   if (*num_planes)
>   return sizes[0] < H264_VLC_BUF_SIZE ? -EINVAL : 0;
> 
>   sizes[0] = H264_VLC_BUF_SIZE;
>   *num_planes = 1;
> 
>   return 0;
> }

Thanks for suggestion, but now the failure looks this way:

Streaming ioctls:
test read/write: OK
fail: v4l2-test-buffers.cpp(297): g_field() == V4L2_FIELD_ANY
fail: v4l2-test-buffers.cpp(703): buf.check(q, last_seq)
fail: v4l2-test-buffers.cpp(976): captureBufs(node, q, m2m_q, 
frame_count, false)
test MMAP: FAIL

Will check that later. If you have any suggestions, I would be very
grateful.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH v0] Add tw5864 driver

[Andrey Utkin]

Hi Hans!

Some improvements took place on the driver, including cleaner
v4l2-compliance tests passing. But there's a single test failure I
don't understand.

In the code of v4l2-compliance, it seems like an API
call CREATE_BUFS is supposed to fail with EINVAL. But in case of my
driver, which simply uses vb2_ioctl_create_bufs(), the call returns 0.

Please review the below report.

The report is produced by fresh v4l-utils from git:
v4l-utils-1.10.0-59-g1388c0a

The actual code which was tested is at tag release/tw5864/1.10 of
https://github.com/bluecherrydvr/linux.git , see
drivers/staging/media/tw5864 . If we sort this out, I am considering
resubmit the driver for merging upstream.

There's also another issue with v4l2-compliance. At some moment the
driver receives S_PARM command with timeperframe 0/0, by which reason I
added special handling, but I guess there's something out of my
knowledge which caused this and which should be fixed.


 # v4l2-compliance -d /dev/video6 -s
Driver Info:
Driver name   : tw5864
Card type : TW5864 Encoder 2
Bus info  : PCI::06:05.0
Driver version: 4.5.0
Capabilities  : 0x8521
Video Capture
Read/Write
Streaming
Extended Pix Format
Device Capabilities
Device Caps   : 0x0521
Video Capture
Read/Write
Streaming
Extended Pix Format

Compliance test for device /dev/video6 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 11 Private Controls: 0

Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK (Not Supported)

Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK

Test input 0:

Streaming ioctls:
test read/write: OK
fail: v4l2-test-buffers.cpp(959): ret != EINVAL
test MMAP: FAIL
test USERPTR: OK (Not Supported)
test DMABUF: Cannot test, specify --expbuf-device


Total: 45, Succeeded: 44, Failed: 1, Warnings: 0
[ERR]
14:14:15root@tw ~
 # gdb v4l2-compliance 
GNU gdb (Ubuntu 7.10-1ubuntu3) 7.10
Copyright (C) 2015 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
 This is free software: you are free
to change and redistribute it. There is NO WARRANTY, to the extent
permitted by law.  Type "show copying" and "show warranty" for details.
This GDB was configured as "i686-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:

Re: [RFC PATCH v0] Add tw5864 driver

[Andrey Utkin]

On Mon, Feb 8, 2016 at 11:58 AM, Hans Verkuil  wrote:
> Hi Andrey,
>
> Hmm, it looks like I forgot to reply. Sorry about that.

Thank you very much anyway.

> I wouldn't change the memcpy: in my experience it is very useful to get a
> well-formed compressed stream out of the hardware. And the overhead of
> having to do a memcpy is a small price to pay and on modern CPUs should
> be barely noticeable for SDTV inputs.

So there's no usecase for scatter-gather approach, right?

> I don't believe that the lockups you see are related to the memcpy as
> such. The trace says that a cpu is stuck for 22s, no way that is related
> to something like that. It looks more like a deadlock somewhere.

There was a locking issue (lack of _irqsave) and was fixed since then.

> Regarding the compliance tests: don't pass VB2_USERPTR (doesn't work well
> with videobuf2-dma-contig). Also add vidioc_expbuf = vb2_ioctl_expbuf for
> the DMABUF support. That should clear up some of the errors you see.

Thank you!

-- 
Bluecherry developer.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH v0] Add tw5864 driver

[Andrey Utkin]

On Mon, Jan 11, 2016 at 12:52 PM, Hans Verkuil  wrote:
> Did you also test with v4l2-compliance? Before I accept the driver I want to 
> see the
> output of 'v4l2-compliance' and 'v4l2-compliance -s'. Basically there 
> shouldn't be
> any failures.
>
> I did a quick scan over the source and I saw nothing big. When you post the 
> new
> version I will go over it more carefully and do a proper review.

Thank you for review.
You can find output of v4l2-compliance and v4l2-compliance -s tests
below (v4l-utils-1.8.0-42-gf1348b4).
There are some issues, I will work on them.
Could you please advise how could I optimize high-volume data passing
to userspace apps?
In one installation with 16 boards, when all streams are set to be 30
FPS all-I-frames (workaround for another P-frames quality issue), the
host struggles from regular lockups, see message immediately below
(4.2.0 kernel though, DKMS package with slightly modified code to
match older API).
I am not fully sure it is related to my driver, but I guess memcpy-ish
approach to preparing video frames may be very CPU-expensive. 30 FPS
all-I-frames video stream is like 3 megabytes per second.
Of course this is not very good situation anyway, and I should either
fix original issue or find some workarounds, at last to lower
framerate. But still, optimization of data passing using some
appropriate API like scatter-gather should be done I believe. I need
hints how to do that right.
Thanks in advance.

Jan 13 19:35:20 cctv kernel: [34202.715069] NMI watchdog: BUG: soft
lockup - CPU#2 stuck for 22s! [khugepaged:69]
Jan 13 19:35:20 cctv kernel: [34202.715071] Modules linked in: ib_iser
rdma_cm iw_cm ib_cm ib_sa ib_mad ib_core ib_addr iscsi_tcp
libiscsi_tcp libiscsi scsi_transport_iscsi ast ttm drm_kms_helper drm
syscopyarea sysfillrect sysimgblt xfs libcrc32c ipmi_ssif joydev
input_leds intel_rapl iosf_mbi x86_pkg_temp_thermal intel_powerclamp
coretemp kvm_intel kvm crct10dif_pclmul crc32_pclmul aesni_intel
aes_x86_64 lrw gf128mul glue_helper ablk_helper cryptd sb_edac
edac_core lpc_ich mei_me mei shpchp wmi tw5864(OE) solo6x10
videobuf2_dma_contig videobuf2_dma_sg videobuf2_memops videobuf2_core
v4l2_common videodev media ipmi_si 8250_fintek ipmi_msghandler snd_pcm
snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq snd_seq_device
snd_timer snd acpi_power_meter acpi_pad soundcore mac_hid parport_pc
ppdev lp parport hid_generic usbhid hid igb i2c_algo_bit ahci dca ptp
libahci megaraid_sas pps_core
Jan 13 19:35:20 cctv kernel: [34202.715105] CPU: 2 PID: 69 Comm:
khugepaged Tainted: GW  OEL  4.2.0-23-generic
#28~14.04.1-Ubuntu
Jan 13 19:35:20 cctv kernel: [34202.715106] Hardware name: Supermicro
Super Server/X10SRi-F, BIOS 2.0 12/17/2015
Jan 13 19:35:20 cctv kernel: [34202.715107] task: 88046d7a6040 ti:
88046d0f4000 task.ti: 88046d0f4000
Jan 13 19:35:20 cctv kernel: [34202.715108] RIP:
0010:[]  []
smp_call_function_many+0x1f9/0x250
Jan 13 19:35:20 cctv kernel: [34202.715112] RSP: 0018:88046d0f7b58
 EFLAGS: 0202
Jan 13 19:35:20 cctv kernel: [34202.715112] RAX: 0003 RBX:
 RCX: 88047fd1a870
Jan 13 19:35:20 cctv kernel: [34202.715113] RDX: 0004 RSI:
0100 RDI: 88047fc97788
Jan 13 19:35:20 cctv kernel: [34202.715114] RBP: 88046d0f7b98 R08:
00fb R09: 
Jan 13 19:35:20 cctv kernel: [34202.715114] R10: 0004 R11:
 R12: 0297
Jan 13 19:35:20 cctv kernel: [34202.715115] R13: 88046d0f7b08 R14:
0202 R15: 0202
Jan 13 19:35:20 cctv kernel: [34202.715116] FS:
() GS:88047fc8()
knlGS:
Jan 13 19:35:20 cctv kernel: [34202.715117] CS:  0010 DS:  ES:
 CR0: 80050033
Jan 13 19:35:20 cctv kernel: [34202.715118] CR2: 7fae31dc CR3:
01c0d000 CR4: 001406e0
Jan 13 19:35:20 cctv kernel: [34202.715118] Stack:
Jan 13 19:35:20 cctv kernel: [34202.715119]  00017740
01ff88040001 0009 81fb4ec0
Jan 13 19:35:20 cctv kernel: [34202.715120]  8117d4c0
 0002 0100
Jan 13 19:35:20 cctv kernel: [34202.715122]  88046d0f7bc8
810f4e49 0100 
Jan 13 19:35:20 cctv kernel: [34202.715123] Call Trace:
Jan 13 19:35:20 cctv kernel: [34202.715127]  [] ?
page_alloc_cpu_notify+0x50/0x50
Jan 13 19:35:20 cctv kernel: [34202.715128]  []
on_each_cpu_mask+0x29/0x70
Jan 13 19:35:20 cctv kernel: [34202.715129]  []
drain_all_pages+0xd3/0xf0
Jan 13 19:35:20 cctv kernel: [34202.715131]  []
__alloc_pages_nodemask+0x631/0x990
Jan 13 19:35:20 cctv kernel: [34202.715134]  []
khugepaged_scan_mm_slot+0x540/0xf50
Jan 13 19:35:20 cctv kernel: [34202.715136]  [] ?
schedule_timeout+0x165/0x2a0
Jan 13 19:35:20 cctv kernel: [34202.715137]  []
khugepaged+0x11d/0x440
Jan 13 19:35:20 cctv kernel: [34202.715139]  [] ?
prepare_to_wait_event+0xf0/0xf0
Jan 13 19:35:20 cctv k

Re: [RFC PATCH v0] Add tw5864 driver

[Andrey Utkin]

On Sun, Jan 3, 2016 at 5:47 AM, Joe Perches  wrote:
> several of these have unnecessary parentheses

Thanks, fixed.

> Maybe use bool a bit more

Thanks, fixed.

> or maybe just use fls

Thanks, fls() fit greatly, rewritten the function with compatibility testing.

>> +static inline int bs_size_ue(unsigned int val)
>> +{
>> + int i_size = 0;
>> + static const int i_size0_254[255] = {
>
> Same sort of thing

Dropped this procedure because it is not used.
Thanks.

>> diff --git a/drivers/staging/media/tw5864/tw5864-config.c 
>> b/drivers/staging/media/tw5864/tw5864-config.c
> []
>> +u8 tw_indir_readb(struct tw5864_dev *dev, u16 addr)
>> +{
>> + int timeout = 3;
>
> misleading name, retries would be more proper,
> or maybe use real timed loops.

Thanks, renamed to "retries".

> This seems a little repetitive.

Thanks, reworked.

> u16?

Thanks, fixed.

> odd indentation

Indeed. For some mysterious reason, vim + syntastic insists on this way. Fixed.

>> +#ifdef DEBUG
>> + dev_dbg(&input->root->pci->dev,
>> + "input %d, frame md stats: min %u, max %u, avg %u, cells above 
>> threshold: %u\n",
>> + input->input_number, min, max, sum / md_cells,
>> + cnt_above_thresh);
>> +#endif
>
> unnecessary #ifdef

Not quite. This debug printout works with variables which are declared
in another "#ifdef DEBUG" clause. And it turns out that dev_dbg is
compiled not only if DEBUG is declared, so when I remove this ifdef, I
get "undefined variable" errors. It seems it is compiled if this
condition is met:

#if (defined DEBUG) || (defined CONFIG_DYNAMIC_DEBUG)

so I can wrap my stats variables into this statement instead. But such
change is not equivalent - I guess CONFIG_DYNAMIC_DEBUG is common to
be enabled, so debug stats will be always calculated, even when module
is not under debug. Except if I use DEFINE_DYNAMIC_DEBUG_METADATA etc.
in my code. Please let me know if this can be sorted out in cleaner
way.



On Sun, Jan 3, 2016 at 7:38 AM, Leon Romanovsky  wrote:
> On Sun, Jan 03, 2016 at 03:41:42AM +0200, Andrey Utkin wrote:
> 
>> +/*
>> + *  TW5864 driver - Exp-Golomb code functions
>> + *
>> + *  Copyright (C) 2015 Bluecherry, LLC 
>> + *  Copyright (C) 2015 Andrey Utkin 
>
> I doubt that you have contract with your employer which permits you to
> claim copyright on the work/product.

Thank you for commenting.
I have previously asked my employer to review copyright statment, and
he told this is fine.
Now I have requrested him again with reference to your comment.

-- 
Bluecherry developer.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: On Lindent shortcomings and massive style fixing

[Andrey Utkin]

On Tue, Dec 29, 2015 at 9:32 AM, Mauro Carvalho Chehab
 wrote:
> IMHO, there are two problems by letting indent breaking long
> lines:
>
> 1) indent would break strings on printks. This is something that we don't
> want to break strings on multiple lines in the Kernel;

Yeah, GNU indent does its work badly (although I believe it could be
taught to respect long literals), this makes me want to have a better
tool for clean "relayouting" C code.
I believe that'd require at last a proper syntax parser. With such
tool, it would be straightforward to rewrite source code automatically
to please any demanding reviewer of code style, except for issues of
higher level of thought (like naming or nesting limitations).

> 2) It doesn't actually solve the problem of having too complex loops,
> with is why the 80 columns warning is meant to warn. Worse than that,
> if a piece of code is inside more than 4 or 5 indentation levels, the
> resulting code of using indent for 80-cols line break is a total crap.

Then I'd propose to enforce nesting limitation explicitly, because
Documentation/CodingStyle appreciates low nesting, just doesn't give
exact numbers.
It's better this way, because the programmer could pay attention to N
places of excessive nesting and fix it manually, and then carelessly
reformat NNN places of "80 chars" issues automatically, comparing to
reviewing all NNN places, to figure out if there's excessive nesting,
or not.
(CCed checkpatch.pl maintainers.)

> That's said, on a quick look at the driver, it seems that the 80-cols
> violations are mostly (if not all) on the comments, like:
>
> int i_poc_lsb = (frame_seqno_in_gop << 1); /* why multiplied by two? 
> TODO try without multiplication */
>
> and
>
> #define TW5864_UNDEF_REG_0x0224 0x0224  /* Undeclared in spec (or not yet 
> added to tw5864-reg.h) but used */
> #define TW5864_UNDEF_REG_0x4014 0x4014  /* Undeclared in spec (or not yet 
> added to tw5864-reg.h) but used */
> #define TW5864_UNDEF_REG_0xA800 0xA800  /* Undeclared in spec (or not yet 
> added to tw5864-reg.h) but used */
>
> Btw, the content of tw5864-reg-undefined.h is weird... Why not just
> add the stuff there at tw5864-reg.h and remove the comments for all
> defines there?

tw5864-reg-undefined.h will be edited for sure (maybe dropped), of
course it won't stay as it is now.
It was generated by script during reverse-engineering that bastard
chip from hell.

> Also, Lindent already did some crappy 80-cols like breaks, like:
>
> static int pci_i2c_multi_read(struct tw5864_dev *dev, u8 devid, u8 devfn, u8 
> *buf,
>u32 count)
>
> (count is misaligned with the open parenthesis)

I just added "static " after indenting.
Actually, Documentation/CodingStyle says nothing about alignment of
function declaration tail on open parenthesis.
Also I'd like to mention that I hate such alignment, because it
requires intermixing of tabs and spaces. I am not aware if this is K&R
thing or not. If it is, then please don't kill me.

Thanks for kind replies, gentlemen.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

On Lindent shortcomings and massive style fixing

[Andrey Utkin]

After some iterations of checkpatch.pl, on a new developed driver
(tw5864), now I have the following:

 $ grep 'WARNING\|ERROR' /src/checkpatch.tw5864 | sort | uniq -c
 31 ERROR: do not use C99 // comments
147 WARNING: Block comments use a trailing */ on a separate line
144 WARNING: Block comments use * on subsequent lines
435 WARNING: line over 80 characters

At this point, Lindent was already used, and checkpatch.pl warnings
introduced by Lindent itself were fixed. Usage of "indent
--linux-style" (which behaves differently BTW) doesn't help anymore,
too.

Could anybody please advise how to sort out these issues
automatically, because they look like perfectly solvable in automated
fashion. Of course manual work would result in more niceness, but I am
not eager to go through hundreds of place of code just to fix "over 80
characters" issues now.

First one ("do not use C99 // comments") looks easy with regexps, but
the other are not.

Is there any known improvements or successors for Lindent? Or could we
get indent/Lindent improved if we collect some money for this task?

If anybody wants to look at actual code, here it is:
https://github.com/bluecherrydvr/linux.git , branch tw5864_stable,
drivers/staging/media/tw5864

Current output of "checkpatch.pl -f" for all source files in the
driver is here:
https://gist.github.com/andrey-utkin/12295148475e34ef948b

Thanks in advance.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [x264-devel] Help to debug h264 headers (or video) generation in kernel driver

[Andrey Utkin]

On Sat, Sep 26, 2015 at 10:44 AM, Max Lapshin  wrote:
> Do you get some packets from this device or it is a bytestream?

Oh hi a linux.org.ru buddy :)
I get headerless binary data portions of known length, one portion for
each encoded video frame. NAL headers generation is done on driver
level in reference software, and I have no choice but to follow same
approach.
You can check most of the info about this project from my earlier
request for help: https://lkml.org/lkml/2015/6/2/761

-- 
Bluecherry developer.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Help to debug h264 headers (or video) generation in kernel driver

[Andrey Utkin]

I'm working on Linux kernel driver for multimedia grabber and H264
encoder based on Techwell/Intersil 5864 chip. Of course it will be
open and pushed into upstream kernel.

The device produces H264 encoded data, but it lacks any headers. The
reference driver generates headers and glues frames together in code,
but I failed both reverse-engineering and porting that code. The code
of reference driver is overwhelmingly complicated, and I have no
knowledge how H264 bitstream is formed, that's why my attempts failed.

I have reached a limited success with both setting up the hardware
encoder and forming the headers. You can see the samples of produced
video here:
http://lizard.bluecherry.net/~autkin/tw5864_tiled_video/ntsc_cif.h264
http://lizard.bluecherry.net/~autkin/tw5864_tiled_video/ntsc_d1.h264
http://lizard.bluecherry.net/~autkin/tw5864_tiled_video/pal_cif.h264
http://lizard.bluecherry.net/~autkin/tw5864_tiled_video/pal_d1.h264

Currently it produces "tiled" picture. The width of tiles on these
sample videos is 256 pixels.
I am not sure whether the issue is in hardware settings, or incorrect
headers which lead to partial failure of decoding.
Here is ffplay log for playing back such video:
https://gist.github.com/krieger-od/5269c762a36bcb52b93a . Obviously
some enhancements to headers generation are needed.

Playing with hardware mirroring flags proves that the raw frame gets
caught fully and correctly. But for h264 encoding, it gets garbled.
Also please notice that frame order is incorrect, the motion goes back
and forth.

You can see our latest code for this development at
https://github.com/bluecherrydvr/linux/tree/tw5864/drivers/staging/media/tw5864
(branch tw5864, subdirectory drivers/staging/media/tw5864).

Any input is extremely appreciated.

-- 
Bluecherry developer.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] [RFC] drivers/staging/fbtft: fix sparse warnings

[Andrey Utkin]

2015-02-23 21:27 GMT+02:00 Noralf Trønnes :
> Yes, it's best to leave this alone for now.
> I'm working on a proposal to provide better layering and minimal coupling
> to fbdev. This will hopefully lead to screen_base eventually being used
> only twice in the fbtft module and nowhere else.

Ok, staying away and looking for sparse warnings for other staging drivers.
Thanks to all for comments.

-- 
Andrey Utkin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] [RFC] drivers/staging/fbtft: fix sparse warnings

[Andrey Utkin]

See below how sparse output changed with these changes.
In few words:
- fixed printf specifiers for size_t;
- trying to fix address space specifiers issues, not sure what's correct 
approach, ASKING FOR COMMENTS AND HELP;
- didn't touch "was not declared. Should it be static?" yet.

-drivers/staging/fbtft/fbtft-core.c: In function ‘fbtft_register_framebuffer’:
-drivers/staging/fbtft/fbtft-core.c:1004:4: warning: format ‘%d’ expects 
argument of type ‘int’, but argument 3 has type ‘size_t’ [-Wformat=]
-drivers/staging/fbtft/fbtft-io.c: In function ‘fbtft_write_spi_emulate_9’:
-drivers/staging/fbtft/fbtft-io.c:63:4: warning: format ‘%d’ expects argument 
of type ‘int’, but argument 4 has type ‘size_t’ [-Wformat=]
-drivers/staging/fbtft/fbtft-io.c: In function ‘fbtft_read_spi’:
-drivers/staging/fbtft/fbtft-io.c:110:5: warning: format ‘%d’ expects argument 
of type ‘int’, but argument 4 has type ‘size_t’ [-Wformat=]
-drivers/staging/fbtft/fbtft-core.c:928:19: warning: incorrect type in argument 
1 (different address spaces)
-drivers/staging/fbtft/fbtft-core.c:928:19:expected void const *addr
-drivers/staging/fbtft/fbtft-core.c:928:19:got char [noderef] 
*screen_base
 drivers/staging/fbtft/fbtft-sysfs.c:24:5: warning: symbol 
'fbtft_gamma_parse_str' was not declared. Should it be static?
 drivers/staging/fbtft/fbtft-sysfs.c:154:6: warning: symbol 
'fbtft_expand_debug_value' was not declared. Should it be static?
 drivers/staging/fbtft/fbtft-sysfs.c:210:6: warning: symbol 'fbtft_sysfs_init' 
was not declared. Should it be static?
 drivers/staging/fbtft/fbtft-sysfs.c:217:6: warning: symbol 'fbtft_sysfs_exit' 
was not declared. Should it be static?
-drivers/staging/fbtft/fbtft-bus.c:145:19: warning: cast removes address space 
of expression
-drivers/staging/fbtft/fbtft-bus.c:204:15: warning: incorrect type in 
assignment (different address spaces)
-drivers/staging/fbtft/fbtft-bus.c:204:15:expected unsigned char [usertype] 
*vmem8
-drivers/staging/fbtft/fbtft-bus.c:204:15:got char [noderef] *
-drivers/staging/fbtft/fbtft-bus.c:248:19: warning: cast removes address space 
of expression
-drivers/staging/fbtft/fb_agm1264k-fl.c:276:24: warning: cast removes address 
space of expression
+drivers/staging/fbtft/fbtft-bus.c:152:49: warning: incorrect type in argument 
2 (different address spaces)
+drivers/staging/fbtft/fbtft-bus.c:152:49:expected void *buf
+drivers/staging/fbtft/fbtft-bus.c:152:49:got unsigned short [noderef] 
[usertype] *[assigned] vmem16
+drivers/staging/fbtft/fbtft-bus.c:254:41: warning: incorrect type in argument 
2 (different address spaces)
+drivers/staging/fbtft/fbtft-bus.c:254:41:expected void *buf
+drivers/staging/fbtft/fbtft-bus.c:254:41:got unsigned short [noderef] 
[usertype] *[assigned] vmem16
+drivers/staging/fbtft/fbtft-core.c:928:16: warning: cast removes address space 
of expression
 drivers/staging/fbtft/fb_hx8340bn.c:111:6: warning: symbol 'set_addr_win' was 
not declared. Should it be static?
-drivers/staging/fbtft/fb_pcd8544.c:113:24: warning: cast removes address space 
of expression
-drivers/staging/fbtft/fb_ra8875.c:286:19: warning: cast removes address space 
of expression
-drivers/staging/fbtft/fb_ssd1306.c:175:24: warning: cast removes address space 
of expression
-drivers/staging/fbtft/fb_tls8204.c:102:24: warning: cast removes address space 
of expression
-drivers/staging/fbtft/fb_uc1701.c:153:24: warning: cast removes address space 
of expression
-drivers/staging/fbtft/fb_watterott.c:76:24: warning: cast removes address 
space of expression
+drivers/staging/fbtft/fb_uc1701.c:153:32: warning: cast removes address space 
of expression
+drivers/staging/fbtft/fb_uc1701.c:153:32: warning: incorrect type in 
initializer (different address spaces)
+drivers/staging/fbtft/fb_uc1701.c:153:32:expected unsigned short [noderef] 
[usertype] *vmem16
+drivers/staging/fbtft/fb_uc1701.c:153:32:got unsigned short [usertype] 
*
+drivers/staging/fbtft/fb_watterott.c:76:32: warning: cast removes address 
space of expression
+drivers/staging/fbtft/fb_watterott.c:76:32: warning: incorrect type in 
initializer (different address spaces)
+drivers/staging/fbtft/fb_watterott.c:76:32:expected unsigned short 
[noderef] [usertype] *vmem16
+drivers/staging/fbtft/fb_watterott.c:76:32:got unsigned short [usertype] 
*
 drivers/staging/fbtft/fb_watterott.c:115:24: warning: cast removes address 
space of expression
 drivers/staging/fbtft/fbtft_device.c:32:19: warning: symbol 'spi_device' was 
not declared. Should it be static?
 drivers/staging/fbtft/fbtft_device.c:33:24: warning: symbol 'p_device' was not 
declared. Should it be static?

This is for Eudyptulla challenge. If you want me to help with any other staging 
driver, I am open.

Signed-off-by: Andrey Utkin 
---
 drivers/staging/fbtft/fb_agm1264k-fl.c |  4 ++--
 drivers/staging/fbtft/fb_pcd8544.c |  4 ++--
 drive

Re: solo6x10: all interrupts for all cards handled by CPU0, no balancing - why?

[Andrey Utkin]

The host was rebooted and got back online.
Without irqbalance daemon, all solo6x10 interrupts are still on CPU0.
See https://gist.github.com/krieger-od/d1686243c67fbe3e14a5
Any ideas are strongly appreciated.


-- 
Andrey Utkin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

solo6x10: all interrupts for all cards handled by CPU0, no balancing - why?

[Andrey Utkin]

Hi, having another "card freeze" issue with linux-next (tag
next-20150128) on a server running 3 solo6110 cards. The freeze
happens after 3 days or so. Much better than 30 minutes, which was the
case before the recent enhancement by Krzysztof Halasa.
This is Ubuntu Trusty. There's /usr/sbin/irqbalance process running.
See interrupt stats here:
https://gist.github.com/krieger-od/f8d99080d6fc30dad3d2
I wonder why all solo6x10 interrupts happen on CPU0, while there are 3
more cores.

However, I have got an idea reading this:
Irqbalance is a daemon to help balance the cpu load generated by interrupts
across all of a systems cpus.  Irqbalance identifies the highest volume
interrupt sources, and isolates them to a single unique cpu, so that load is
spread as much as possible over an entire processor set, while minimizing cache
hit rates for irq handlers.

Disabled irqbalance launch on boot. Rebooted, and the host got down up to now :(
Any comments, except laugh? :)

-- 
Andrey Utkin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: lustre: Fix the warning messages about casting without __user macro

[Andrey Utkin]

2014-12-10 16:49 GMT+02:00 Greg KH :
> On Wed, Dec 10, 2014 at 07:09:59AM +, Al Viro wrote:
>> On Tue, Dec 09, 2014 at 10:56:12PM -0800, Shalin Mehta wrote:
>> > From: Shalin Mehta 
>> >
>> > This issue is showed up while compiling with sparse. The iov_base in 
>> > struct iovec struct explicitly declares that the assigned value should be 
>> > user space pointer with __user macro. Where as here, the __user macro 
>> > isn't used while casting.
>>
>> ... and pointers are not user space ones at all.  Which is to say, quit
>> messing with casts; it's not struct iovec.  Proper fix is to replace
>> it here (and in almost all places throughout drivers/staging/lustre) with
>> struct kvec.  And yes, such a patch had been sent.  Still not applied,
>> AFAICS...
>
> Yeah, it's in the merge window and I'll pick new staging patches back up
> when 3.19-rc1 is released.

Hi Greg,
Still no commits for
drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c since Sun Nov
23 (staging: lustre: Coalesce string fragments) in linux-next. Maybe
the aforementioned commit got missed for merging?

-- 
Andrey Utkin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: lustre/lnet/lnet/lib-move.c: memcpy with (struct iovec*)->iov_base, which is __user

[Andrey Utkin]

2015-01-07 13:32 GMT+02:00 Dilger, Andreas :
> On 2015/01/07, 1:36 AM, "Dan Carpenter"  wrote:
>> Didn't Al change these to kvec instead of iovec?  You have to look at
>> the callers to figure out if it's actually a user space pointer or a
>> kernel pointer.
>
> A patch was sent by Al on Dec 2 to replace iovec with kvec, in a thread
> titled "[PATCH] staging:lustre:lnet: Incorrect type in assignment".  Greg
> replied on Dec 10 in another thread (also fixing this same warning) titled
> "[PATCH] staging: lustre: Fix the warning messages about casting without
> __user macro" that Al's patch was in the staging tree for 3.19-rc1.

In current linux-next, I see none of the mentioned patches. Also I
don't see them mentioned in GKH's merge commit "Merge tag
'staging-3.19-rc1'..."
Is it ok?

-- 
Andrey Utkin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: lustre/lnet/lnet/lib-move.c: memcpy with (struct iovec*)->iov_base, which is __user

[Andrey Utkin]

2015-01-07 10:36 GMT+02:00 Dan Carpenter :
> Didn't Al change these to kvec instead of iovec?  You have to look at
> the callers to figure out if it's actually a user space pointer or a
> kernel pointer.

I am looking at current linux-next checkout.

-- 
Andrey Utkin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

lustre/lnet/lnet/lib-move.c: memcpy with (struct iovec*)->iov_base, which is __user

[Andrey Utkin]

Dear maintainers of LustreFS and other experienced kernel developers!

I am working on fixing some sparse warnings as a task of Eudyptula Challenge.
There's a thing that look suspiciously to me (or I just don't
understand it). This looks same both in upstream kernel code in
drivers/staging/ and in https://github.com/Xyratex/lustre-stable.git
(it doesn't fetch from http://git.whamcloud.com/fs/lustre-release.git,
but I guess it is similar with Xyratex repo).

In lnet/lnet/lib-move.c, in lnet_copy_iov2iov(), at line 209, there is

memcpy((char *)diov->iov_base + doffset,
(char *)siov->iov_base + soffset, this_nob);

diov and siov are struct iovec *, and iov_base in it is void __user *.
Are diov and siov really used semantically correctly here, holding
userspace addresses in iov_base?
Is it semantically correct to use memcpy to copy bytes from one
userspace address to another?

How to treat the comment "/* NB diov, siov are READ-ONLY */" at line
177 in the same procedure? Is it adequate?

This above-mentioned memcpy operation causes sparse warnings "warning:
cast removes address space of expression" for both first and second
line. When i change it to

memcpy((void __user *)((char __user *)diov->iov_base + doffset),
(void __user *)((char __user *)siov->iov_base
+ soffset), this_nob);

It gives other sparse warnings:
drivers/staging/lustre/lnet/lnet/lib-move.c:208:25: warning: incorrect
type in argument 1 (different address spaces)
drivers/staging/lustre/lnet/lnet/lib-move.c:208:25:expected void *to
drivers/staging/lustre/lnet/lnet/lib-move.c:208:25:got void
[noderef] *
drivers/staging/lustre/lnet/lnet/lib-move.c:209:26: warning: incorrect
type in argument 2 (different address spaces)
drivers/staging/lustre/lnet/lnet/lib-move.c:209:26:expected void const *from
drivers/staging/lustre/lnet/lnet/lib-move.c:209:26:got void
[noderef] *

What is supposed way to avoid warnings in this case?

Thanks.

-- 
Andrey Utkin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 1/4] [media] solo6x10: clean up properly in stop_streaming

[Andrey Utkin]

This fixes warning from drivers/media/v4l2-core/videobuf2-core.c,
WARN_ON(atomic_read(&q->owned_by_drv_count)).

Signed-off-by: Andrey Utkin 
---
 drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
index 28023f9..6cd6a25 100644
--- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
@@ -781,9 +781,19 @@ static int solo_enc_start_streaming(struct vb2_queue *q, 
unsigned int count)
 static void solo_enc_stop_streaming(struct vb2_queue *q)
 {
struct solo_enc_dev *solo_enc = vb2_get_drv_priv(q);
+   unsigned long flags;
 
+   spin_lock_irqsave(&solo_enc->av_lock, flags);
solo_enc_off(solo_enc);
-   INIT_LIST_HEAD(&solo_enc->vidq_active);
+   while (!list_empty(&solo_enc->vidq_active)) {
+   struct solo_vb2_buf *buf = list_entry(
+   solo_enc->vidq_active.next,
+   struct solo_vb2_buf, list);
+
+   list_del(&buf->list);
+   vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
+   }
+   spin_unlock_irqrestore(&solo_enc->av_lock, flags);
solo_ring_stop(solo_enc->solo_dev);
 }
 
-- 
1.8.5.5


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] [media] solo6x10: don't turn off/on encoder interrupt in processing loop

[Andrey Utkin]

2014-11-03 19:15 GMT+04:00 Hans Verkuil :
> Hi Andrey,
>
> On 10/29/2014 05:03 PM, Andrey Utkin wrote:
>> The used approach actually cannot prevent new encoder interrupt to
>> appear, because interrupt handler can execute in different thread, and
>> in current implementation there is still race condition regarding this.
>
> I don't understand what you mean with 'interrupt handler can execute in
> different thread'. Can you elaborate?
>
> Note that I do think that this change makes sense, but I do like to have a
> better explanation.
>

Hi Hans, thanks for response.

I'm not proficient in linux kernel, so it's hard to make sure and
strict statements regarding this.

In the commit justification I mean that solo_ring_thread(), which is
edited, runs in a thread started with kthread_run().
Interrupt hander is executed on random kernel thread (whichever is
currently running, is it correct?). So temporarily disabling
interrupts from video encoders by writing to special register cannot
be used for "processing serialization", for "fixation of state" or
anything useful at all, thus it should be removed from code.

Is it clear now?

Please feel free to push the patch with edited description, even
without resubmission from me.

-- 
Andrey Utkin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/4] [media] solo6x10: don't turn off/on encoder interrupt in processing loop

[Andrey Utkin]

The used approach actually cannot prevent new encoder interrupt to
appear, because interrupt handler can execute in different thread, and
in current implementation there is still race condition regarding this.
Also from practice the code with this change seems to work as stable as
before.

Signed-off-by: Andrey Utkin 
---
 drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
index b9b61b9..30e09d9 100644
--- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
@@ -703,9 +703,7 @@ static int solo_ring_thread(void *data)
 
if (timeout == -ERESTARTSYS || kthread_should_stop())
break;
-   solo_irq_off(solo_dev, SOLO_IRQ_ENCODER);
solo_handle_ring(solo_dev);
-   solo_irq_on(solo_dev, SOLO_IRQ_ENCODER);
try_to_freeze();
}
 
-- 
1.8.5.5

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/4] [media] solo6x10: free DMA allocation when releasing encoder

[Andrey Utkin]

Fixes this warning:

[  956.730136] [ cut here ]
[  956.730143] WARNING: CPU: 1 PID: 10134 at lib/dma-debug.c:963 
dma_debug_device_change+0x191/0x1f0()
[  956.730146] pci :07:05.0: DMA-API: device driver has pending DMA 
allocations while released from device [count=8]
One of leaked entries details: [device address=0xd3d57000] [size=512 
bytes] [mapped with DMA_BIDIRECTIONAL] [mapped as coherent]
[  956.730147] Modules linked in: solo6x10(-) videobuf2_dma_contig 
videobuf2_dma_sg videobuf2_memops videobuf2_core ipt_MASQUERADE 
nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat videobuf_vmalloc 
videobuf_core v4l2_common videodev rt2800usb rt2800lib rt2x00usb rt2x00lib 
mac80211 cfg80211 crc_ccitt usbkbd hid_a4tech hid_generic usbhid 
snd_hda_codec_hdmi snd_hda_codec_via snd_hda_codec_generic snd_hda_intel 
snd_hda_controller snd_hda_codec snd_hwdep snd_pcm x86_pkg_temp_thermal 
snd_timer snd soundcore
[  956.730172] CPU: 1 PID: 10134 Comm: rmmod Not tainted 
3.18.0-rc1-next-20141023-zver-dirty #24
[  956.730173] Hardware name: System manufacturer System Product Name/P8H77-V, 
BIOS 0501 02/28/2012
[  956.730175]  0009 8801df9e3c58 817ffe6b 
0001
[  956.730177]  8801df9e3ca8 8801df9e3c98 81091ec7 
0046
[  956.730180]  880215457e90 0008 81cbb10f 
880215570098
[  956.730183] Call Trace:
[  956.730188]  [] dump_stack+0x4f/0x7c
[  956.730192]  [] warn_slowpath_common+0x87/0xb0
[  956.730194]  [] warn_slowpath_fmt+0x41/0x50
[  956.730197]  [] ? dma_debug_device_change+0xb8/0x1f0
[  956.730199]  [] dma_debug_device_change+0x191/0x1f0
[  956.730203]  [] notifier_call_chain+0x4d/0x70
[  956.730205]  [] __blocking_notifier_call_chain+0x59/0x80
[  956.730207]  [] blocking_notifier_call_chain+0x11/0x20
[  956.730211]  [] __device_release_driver+0xcf/0xf0
[  956.730213]  [] driver_detach+0xc8/0xd0
[  956.730215]  [] bus_remove_driver+0x57/0xd0
[  956.730218]  [] driver_unregister+0x29/0x60
[  956.730221]  [] pci_unregister_driver+0x21/0x90
[  956.730225]  [] solo_pci_driver_exit+0x10/0x12 [solo6x10]
[  956.730228]  [] SyS_delete_module+0x170/0x1f0
[  956.730232]  [] ? trace_hardirqs_on_thunk+0x3a/0x3f
[  956.730234]  [] system_call_fastpath+0x12/0x17
[  956.730235] ---[ end trace e730af02713a6c53 ]---
[  956.730237] Mapped at:
[  956.730238]  [] debug_dma_alloc_coherent+0x3c/0xb0
[  956.730240]  [] solo_enc_v4l2_init+0x706/0xba0 [solo6x10]
[  956.730243]  [] solo_pci_probe+0x503/0x700 [solo6x10]
[  956.730245]  [] local_pci_probe+0x49/0xa0
[  956.730248]  [] pci_device_probe+0xd1/0x120

Signed-off-by: Andrey Utkin 
---
 drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
index 6cd6a25..9afeb69 100644
--- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
@@ -1385,6 +1385,9 @@ static void solo_enc_free(struct solo_enc_dev *solo_enc)
if (solo_enc == NULL)
return;
 
+   pci_free_consistent(solo_enc->solo_dev->pdev,
+   sizeof(struct solo_p2m_desc) * solo_enc->desc_nelts,
+   solo_enc->desc_items, solo_enc->desc_dma);
video_unregister_device(solo_enc->vfd);
v4l2_ctrl_handler_free(&solo_enc->hdl);
kfree(solo_enc);
-- 
1.8.5.5

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/4] [media] solo6x10: bind start & stop of encoded frames processing thread to device (de)init

[Andrey Utkin]

Before, it was called from individual encoder (de)init procedures, which
lead to spare threads running (which were actually lost, leaked).
The current fix uses trivial approach, and the downside is that the
processing thread is working always, even when there's no consumer.

Signed-off-by: Andrey Utkin 
---
 drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
index 9afeb69..b9b61b9 100644
--- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
@@ -770,12 +770,8 @@ static void solo_ring_stop(struct solo_dev *solo_dev)
 static int solo_enc_start_streaming(struct vb2_queue *q, unsigned int count)
 {
struct solo_enc_dev *solo_enc = vb2_get_drv_priv(q);
-   int ret;
 
-   ret = solo_enc_on(solo_enc);
-   if (ret)
-   return ret;
-   return solo_ring_start(solo_enc->solo_dev);
+   return solo_enc_on(solo_enc);
 }
 
 static void solo_enc_stop_streaming(struct vb2_queue *q)
@@ -794,7 +790,6 @@ static void solo_enc_stop_streaming(struct vb2_queue *q)
vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
}
spin_unlock_irqrestore(&solo_enc->av_lock, flags);
-   solo_ring_stop(solo_enc->solo_dev);
 }
 
 static struct vb2_ops solo_enc_video_qops = {
@@ -1432,13 +1427,15 @@ int solo_enc_v4l2_init(struct solo_dev *solo_dev, 
unsigned nr)
 solo_dev->v4l2_enc[0]->vfd->num,
 solo_dev->v4l2_enc[solo_dev->nr_chans - 1]->vfd->num);
 
-   return 0;
+   return solo_ring_start(solo_dev);
 }
 
 void solo_enc_v4l2_exit(struct solo_dev *solo_dev)
 {
int i;
 
+   solo_ring_stop(solo_dev);
+
for (i = 0; i < solo_dev->nr_chans; i++)
solo_enc_free(solo_dev->v4l2_enc[i]);
 
-- 
1.8.5.5

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/4] [media] solo6x10: free vb2 buffers on stop_streaming

[Andrey Utkin]

This fixes warning from drivers/media/v4l2-core/videobuf2-core.c:2144

Signed-off-by: Andrey Utkin 
---
 drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
index 28023f9..6cd6a25 100644
--- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
@@ -781,9 +781,19 @@ static int solo_enc_start_streaming(struct vb2_queue *q, 
unsigned int count)
 static void solo_enc_stop_streaming(struct vb2_queue *q)
 {
struct solo_enc_dev *solo_enc = vb2_get_drv_priv(q);
+   unsigned long flags;
 
+   spin_lock_irqsave(&solo_enc->av_lock, flags);
solo_enc_off(solo_enc);
-   INIT_LIST_HEAD(&solo_enc->vidq_active);
+   while (!list_empty(&solo_enc->vidq_active)) {
+   struct solo_vb2_buf *buf = list_entry(
+   solo_enc->vidq_active.next,
+   struct solo_vb2_buf, list);
+
+   list_del(&buf->list);
+   vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
+   }
+   spin_unlock_irqrestore(&solo_enc->av_lock, flags);
solo_ring_stop(solo_enc->solo_dev);
 }
 
-- 
1.8.5.5

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] drivers/staging/bcm/Misc.c: fix a check

[Andrey Utkin]

The issue was reported by static analysis.
The value holding flags was &-ed with 0x02, being compared to 0x01
(TRUE) after that. Such comparsion is always false.

Resolution: drop the comparsion to TRUE in the condition.

&-ing with 0x02 is right, according to result variable name
(reporting_mode) and description in drivers/staging/bcm/target_params.h
("bit 1 = 1: CINR reporting in Idlemode Msg").

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=80801
Reported-by: David Binderman 
Signed-off-by: Andrey Utkin 
---
 drivers/staging/bcm/Misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/bcm/Misc.c b/drivers/staging/bcm/Misc.c
index a1c833c..883f739 100644
--- a/drivers/staging/bcm/Misc.c
+++ b/drivers/staging/bcm/Misc.c
@@ -1154,7 +1154,7 @@ static void doPowerAutoCorrection(struct bcm_mini_adapter 
*psAdapter)
reporting_mode = 
ntohl(psAdapter->pstargetparams->m_u32PowerSavingModeOptions) & 0x02;
psAdapter->bIsAutoCorrectEnabled = !((char)(psAdapter->ulPowerSaveMode 
>> 3) & 0x1);
 
-   if (reporting_mode == TRUE) {
+   if (reporting_mode) {
BCM_DEBUG_PRINT(psAdapter, DBG_TYPE_INITEXIT, MP_INIT, 
DBG_LVL_ALL, "can't do suspen/resume as reporting mode is enable");
psAdapter->bDoSuspend = false;
}
-- 
1.8.5.5

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/4] drivers/staging/comedi/drivers/addi-data/hwdrv_apci3200.c: drop incorrect checks

[Andrey Utkin]

Dropped some "< 0" and ">= 0" checks on unsigned int values.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=80501
Reported-by: David Binderman 
Signed-off-by: Andrey Utkin 
---
 .../comedi/drivers/addi-data/hwdrv_apci3200.c| 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3200.c 
b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3200.c
index a3026a2..3a215ac 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3200.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3200.c
@@ -1750,34 +1750,34 @@ static int apci3200_ai_config(struct comedi_device *dev,
/* END JK 06.07.04: Management of sevrals boards */
 
if (data[5] == 0) {
-   if (ui_ChannelNo < 0 || ui_ChannelNo > 15) {
+   if (ui_ChannelNo > 15) {
printk("\nThe Selection of the channel is in error\n");
i_err++;
-   }   /*  if(ui_ChannelNo<0 || ui_ChannelNo>15) */
+   }   /*  if(ui_ChannelNo>15) */
}   /* if(data[5]==0) */
else {
if (data[14] == 2) {
-   if (ui_ChannelNo < 0 || ui_ChannelNo > 3) {
+   if (ui_ChannelNo > 3) {
printk("\nThe Selection of the channel is in 
error\n");
i_err++;
-   }   /*  if(ui_ChannelNo<0 || ui_ChannelNo>3) */
+   }   /*  if(ui_ChannelNo>3) */
}   /* if(data[14]==2) */
else {
-   if (ui_ChannelNo < 0 || ui_ChannelNo > 7) {
+   if (ui_ChannelNo > 7) {
printk("\nThe Selection of the channel is in 
error\n");
i_err++;
-   }   /*  if(ui_ChannelNo<0 || ui_ChannelNo>7) */
+   }   /*  if(ui_ChannelNo>7) */
}   /* elseif(data[14]==2) */
}   /* elseif(data[5]==0) */
if (data[12] == 0 || data[12] == 1) {
switch (data[5]) {
case 0:
-   if (ui_ChannelNo >= 0 && ui_ChannelNo <= 3) {
+   if (ui_ChannelNo <= 3) {
/* BEGIN JK 06.07.04: Management of sevrals 
boards */
/* i_Offset=0; */
s_BoardInfos[dev->minor].i_Offset = 0;
/* END JK 06.07.04: Management of sevrals 
boards */
-   }   /* if(ui_ChannelNo >=0 && ui_ChannelNo <=3) */
+   }   /* if(ui_ChannelNo <=3) */
if (ui_ChannelNo >= 4 && ui_ChannelNo <= 7) {
/* BEGIN JK 06.07.04: Management of sevrals 
boards */
/* i_Offset=64; */
@@ -1831,12 +1831,12 @@ static int apci3200_ai_config(struct comedi_device *dev,
ui_ChannelNo = 0;
break;
}   /* if(data[14]==2) */
-   if (ui_ChannelNo >= 0 && ui_ChannelNo <= 1) {
+   if (ui_ChannelNo <= 1) {
/* BEGIN JK 06.07.04: Management of sevrals 
boards */
/* i_Offset=0; */
s_BoardInfos[dev->minor].i_Offset = 0;
/* END JK 06.07.04: Management of sevrals 
boards */
-   }   /* if(ui_ChannelNo >=0 && ui_ChannelNo <=1) */
+   }   /* if(ui_ChannelNo <=1) */
if (ui_ChannelNo >= 2 && ui_ChannelNo <= 3) {
/* BEGIN JK 06.07.04: Management of sevrals 
boards */
/* i_ChannelNo=i_ChannelNo-2; */
-- 
1.8.5.5

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/4] drivers/staging/media/davinci_vpfe/dm365_ipipeif.c: fix negativity check

[Andrey Utkin]

[linux-3.16-rc5/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c:210]:
(style) Checking if unsigned variable 'val' is less than zero.

val = get_oneshot_mode(ipipeif->input);
if (val < 0) {
pr_err("ipipeif: links setup required");
return -EINVAL;
}

but

static int get_oneshot_mode(enum ipipeif_input_entity input)

Introduced temporary variable for negativity check.
"val" is afterwards used in a lot of bitwise operations, so changing its type
to signed is not safe, according to CERT C Secure Coding Standards chapter
INT13-C: "Use bitwise operators only on unsigned operands"
https://www.securecoding.cert.org/confluence/display/seccode/INT13-C.+Use+bitwise+operators+only+on+unsigned+operands

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=80521
Reported-by: David Binderman 
Signed-off-by: Andrey Utkin 
---
 drivers/staging/media/davinci_vpfe/dm365_ipipeif.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c
index 59540cd..6d4893b 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c
@@ -196,6 +196,7 @@ static int ipipeif_hw_setup(struct v4l2_subdev *sd)
int data_shift;
int pack_mode;
int source1;
+   int tmp;
 
ipipeif_base_addr = ipipeif->ipipeif_base_addr;
 
@@ -206,8 +207,8 @@ static int ipipeif_hw_setup(struct v4l2_subdev *sd)
outformat = &ipipeif->formats[IPIPEIF_PAD_SOURCE];
 
/* Combine all the fields to make CFG1 register of IPIPEIF */
-   val = get_oneshot_mode(ipipeif->input);
-   if (val < 0) {
+   tmp = val = get_oneshot_mode(ipipeif->input);
+   if (tmp < 0) {
pr_err("ipipeif: links setup required");
return -EINVAL;
}
-- 
1.8.5.5

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Andrey Utkin]

2014-07-11 17:38 GMT+03:00 Ian Abbott :
> Signed-off-by: Andrey Utkin 

I think it's incorrect that you have instantly placed my signoff
statement on this new patch.
Anyway, thanks for your work on the issue.

-- 
Andrey Utkin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Andrey Utkin]

2014-07-11 15:01 GMT+03:00 Ian Abbott :
> On 2014-07-11 11:13, Andrey Utkin wrote:
>>
>> The issue was discovered with static analysis and has two instances in
>> this file. The code looks like this
>> if (x < 65536000) {
>> ...
>> } else if (x < 65536) {
>> ...
>> } else if (x <= 0x /* 655360 */) {
>> ...
>> } else if (x <= 0x /* 6553600 */) {
>> ...
>> }
>>
>> The meaning of this block is to select appropriate clock frequency for
>> interval timer basing on "x", which is amount of time.
>>
>> Notes:
>> 1. That last condition matches previous one - that's the issue.
>> 2. Decimal numbers in comments don't match hex numbers in expressions.
>> But in first case the numbers have same order, while in the second case
>> the hex number is the same, and the decimal one is 10 times bigger.
>> 3. Actually type of "x" is "unsigned int", so its exact upper limit is
>> not obviously known.
>> 4. There's no "else" block.
>>
>> So it makes sense to make an "else" block from last "else if" case. The
>> code inside the block seems correct for such usage.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79871
>> Reported-by: David Binderman 
>> Signed-off-by: Andrey Utkin 
>> ---
>>   drivers/staging/comedi/drivers/ni_atmio16d.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/ni_atmio16d.c
>> b/drivers/staging/comedi/drivers/ni_atmio16d.c
>> index 6ad27f5..895b56d 100644
>> --- a/drivers/staging/comedi/drivers/ni_atmio16d.c
>> +++ b/drivers/staging/comedi/drivers/ni_atmio16d.c
>> @@ -338,7 +338,7 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
>> } else if (cmd->convert_arg <= 0x /* 655360 */) {
>> base_clock = CLOCK_10_KHZ;
>> timer = cmd->convert_arg / 10;
>> -   } else if (cmd->convert_arg <= 0x /* 6553600 */) {
>> +   } else {
>> base_clock = CLOCK_1_KHZ;
>> timer = cmd->convert_arg / 100;
>> }
>
>
> Since 0x is the maximum value 'cmd->convert_arg' can be,

Could you please substantiate this? I see that convert_arg has type
"unsigned int" which may be 8 bytes on 64-bit platform. I haven't
tracked where from actual values come, if the values are limited to 4
bytes, maybe we need to set the type to u32.

> the final
> else can be moved to the 'base_clock = CLOCK_10_KHZ' block and the
> 'base_clock = CLOCK_1_KHZ' block can be removed altogether.

Feel free to prepare such patch, i'm not really keen on the subject.

-- 
Andrey Utkin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Andrey Utkin]

The issue was discovered with static analysis and has two instances in
this file. The code looks like this
if (x < 65536000) {
...
} else if (x < 65536) {
...
} else if (x <= 0x /* 655360 */) {
...
} else if (x <= 0x /* 6553600 */) {
...
}

The meaning of this block is to select appropriate clock frequency for
interval timer basing on "x", which is amount of time.

Notes:
1. That last condition matches previous one - that's the issue.
2. Decimal numbers in comments don't match hex numbers in expressions.
But in first case the numbers have same order, while in the second case
the hex number is the same, and the decimal one is 10 times bigger.
3. Actually type of "x" is "unsigned int", so its exact upper limit is
not obviously known.
4. There's no "else" block.

So it makes sense to make an "else" block from last "else if" case. The
code inside the block seems correct for such usage.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79871
Reported-by: David Binderman 
Signed-off-by: Andrey Utkin 
---
 drivers/staging/comedi/drivers/ni_atmio16d.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_atmio16d.c 
b/drivers/staging/comedi/drivers/ni_atmio16d.c
index 6ad27f5..895b56d 100644
--- a/drivers/staging/comedi/drivers/ni_atmio16d.c
+++ b/drivers/staging/comedi/drivers/ni_atmio16d.c
@@ -338,7 +338,7 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
} else if (cmd->convert_arg <= 0x /* 655360 */) {
base_clock = CLOCK_10_KHZ;
timer = cmd->convert_arg / 10;
-   } else if (cmd->convert_arg <= 0x /* 6553600 */) {
+   } else {
base_clock = CLOCK_1_KHZ;
timer = cmd->convert_arg / 100;
}
@@ -406,7 +406,7 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
} else if (cmd->scan_begin_arg < 0x /* 655360 */) {
base_clock = CLOCK_10_KHZ;
timer = cmd->scan_begin_arg / 10;
-   } else if (cmd->scan_begin_arg < 0x /* 6553600 */) {
+   } else {
base_clock = CLOCK_1_KHZ;
timer = cmd->scan_begin_arg / 100;
}
-- 
1.8.3.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] solo6x10: update GOP size, QP immediately

[Andrey Utkin]

Previously, it was needed to reopen device to update GOP size and
quantization parameter. Now we update device registers with new values
immediately.

Signed-off-by: Andrey Utkin 
---
 drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
index bf6eb06..14f933f 100644
--- a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
@@ -1110,9 +1110,13 @@ static int solo_s_ctrl(struct v4l2_ctrl *ctrl)
 ctrl->val);
case V4L2_CID_MPEG_VIDEO_GOP_SIZE:
solo_enc->gop = ctrl->val;
+   solo_reg_write(solo_dev, SOLO_VE_CH_GOP(solo_enc->ch), 
solo_enc->gop);
+   solo_reg_write(solo_dev, SOLO_VE_CH_GOP_E(solo_enc->ch), 
solo_enc->gop);
return 0;
case V4L2_CID_MPEG_VIDEO_H264_MIN_QP:
solo_enc->qp = ctrl->val;
+   solo_reg_write(solo_dev, SOLO_VE_CH_QP(solo_enc->ch), 
solo_enc->qp);
+   solo_reg_write(solo_dev, SOLO_VE_CH_QP_E(solo_enc->ch), 
solo_enc->qp);
return 0;
case V4L2_CID_MOTION_THRESHOLD:
solo_enc->motion_thresh = ctrl->val;
-- 
1.8.3.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] solo6x10: expose encoder quantization setting as V4L2 control

[Andrey Utkin]

solo6*10 boards have configurable quantization parameter which takes
values from 0 to 31, inclusively.

This change enables setting it with ioctl VIDIOC_S_CTRL with id
V4L2_CID_MPEG_VIDEO_H264_MIN_QP.

Signed-off-by: Andrey Utkin 
---
 drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
index b8ff113..bf6eb06 100644
--- a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
@@ -,6 +,9 @@ static int solo_s_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_MPEG_VIDEO_GOP_SIZE:
solo_enc->gop = ctrl->val;
return 0;
+   case V4L2_CID_MPEG_VIDEO_H264_MIN_QP:
+   solo_enc->qp = ctrl->val;
+   return 0;
case V4L2_CID_MOTION_THRESHOLD:
solo_enc->motion_thresh = ctrl->val;
if (!solo_enc->motion_global || !solo_enc->motion_enabled)
@@ -1260,6 +1263,8 @@ static struct solo_enc_dev *solo_enc_alloc(struct 
solo_dev *solo_dev,
V4L2_CID_SHARPNESS, 0, 15, 1, 0);
v4l2_ctrl_new_std(hdl, &solo_ctrl_ops,
V4L2_CID_MPEG_VIDEO_GOP_SIZE, 1, 255, 1, solo_dev->fps);
+   v4l2_ctrl_new_std(hdl, &solo_ctrl_ops,
+   V4L2_CID_MPEG_VIDEO_H264_MIN_QP, 0, 31, 1, 
SOLO_DEFAULT_QP);
v4l2_ctrl_new_custom(hdl, &solo_motion_threshold_ctrl, NULL);
v4l2_ctrl_new_custom(hdl, &solo_motion_enable_ctrl, NULL);
v4l2_ctrl_new_custom(hdl, &solo_osd_text_ctrl, NULL);
-- 
1.8.3.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] [media] davinci-vpfe: Fix retcode check

[Andrey Utkin]

2014-07-08 17:32 GMT+03:00 Levente Kurusa :
> Hmm, while it is true that get_ipipe_mode returns an int, but
> the consequent call to regw_ip takes an u32 as its second
> argument. Did it cause a build warning for you? (Can't really
> check since I don't have ARM cross compilers close-by)
> If not, then:

Cannot say for sure would compiler complain.
I also haven't really checked it, and unfortunately even haven't
succeeded to make a config that would build that code. But i believe
that warning is still better than misbehaviour.

-- 
Andrey Utkin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] solo6x10: update GOP size, QP immediately

[Andrey Utkin]

Previously, it was needed to reopen device to update GOP size and
quantization parameter. Now we update device registers with new values
immediately.
---
 drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
index bf6eb06..14f933f 100644
--- a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
@@ -1110,9 +1110,13 @@ static int solo_s_ctrl(struct v4l2_ctrl *ctrl)
 ctrl->val);
case V4L2_CID_MPEG_VIDEO_GOP_SIZE:
solo_enc->gop = ctrl->val;
+   solo_reg_write(solo_dev, SOLO_VE_CH_GOP(solo_enc->ch), 
solo_enc->gop);
+   solo_reg_write(solo_dev, SOLO_VE_CH_GOP_E(solo_enc->ch), 
solo_enc->gop);
return 0;
case V4L2_CID_MPEG_VIDEO_H264_MIN_QP:
solo_enc->qp = ctrl->val;
+   solo_reg_write(solo_dev, SOLO_VE_CH_QP(solo_enc->ch), 
solo_enc->qp);
+   solo_reg_write(solo_dev, SOLO_VE_CH_QP_E(solo_enc->ch), 
solo_enc->qp);
return 0;
case V4L2_CID_MOTION_THRESHOLD:
solo_enc->motion_thresh = ctrl->val;
-- 
1.8.3.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] solo6x10: expose encoder quantization setting as V4L2 control

[Andrey Utkin]

solo6*10 boards have configurable quantization parameter which takes
values from 0 to 31, inclusively.

This change enables setting it with ioctl VIDIOC_S_CTRL with id
V4L2_CID_MPEG_VIDEO_H264_MIN_QP.
---
 drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
index b8ff113..bf6eb06 100644
--- a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
@@ -,6 +,9 @@ static int solo_s_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_MPEG_VIDEO_GOP_SIZE:
solo_enc->gop = ctrl->val;
return 0;
+   case V4L2_CID_MPEG_VIDEO_H264_MIN_QP:
+   solo_enc->qp = ctrl->val;
+   return 0;
case V4L2_CID_MOTION_THRESHOLD:
solo_enc->motion_thresh = ctrl->val;
if (!solo_enc->motion_global || !solo_enc->motion_enabled)
@@ -1260,6 +1263,8 @@ static struct solo_enc_dev *solo_enc_alloc(struct 
solo_dev *solo_dev,
V4L2_CID_SHARPNESS, 0, 15, 1, 0);
v4l2_ctrl_new_std(hdl, &solo_ctrl_ops,
V4L2_CID_MPEG_VIDEO_GOP_SIZE, 1, 255, 1, solo_dev->fps);
+   v4l2_ctrl_new_std(hdl, &solo_ctrl_ops,
+   V4L2_CID_MPEG_VIDEO_H264_MIN_QP, 0, 31, 1, 
SOLO_DEFAULT_QP);
v4l2_ctrl_new_custom(hdl, &solo_motion_threshold_ctrl, NULL);
v4l2_ctrl_new_custom(hdl, &solo_motion_enable_ctrl, NULL);
v4l2_ctrl_new_custom(hdl, &solo_osd_text_ctrl, NULL);
-- 
1.8.3.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] [media] davinci-vpfe: Fix retcode check

[Andrey Utkin]

Use signed type to check correctly for negative error code. The issue
was reported with static analyser:

[linux-3.13/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c:270]:
(style) A pointer can not be negative so it is either pointless or an
error to check if it is.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=69071
Reported-by: David Binderman 
Signed-off-by: Andrey Utkin 
---
 drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c
index b2daf5e..e326032 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c
@@ -254,7 +254,7 @@ int config_ipipe_hw(struct vpfe_ipipe_device *ipipe)
void __iomem *ipipe_base = ipipe->base_addr;
struct v4l2_mbus_framefmt *outformat;
u32 color_pat;
-   u32 ipipe_mode;
+   int ipipe_mode;
u32 data_path;
 
/* enable clock to IPIPE */
-- 
1.8.3.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: ft1000-usb: check for errors in card_send_command

[Andrey Utkin]

kmalloc() result check was lacking. Fixing that required also
changing card_send_command() return type from void to int, and
checking its return code everywhere.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=78561
Reported-by: Maksymilian Arciemowicz 
Signed-off-by: Andrey Utkin 
---
 drivers/staging/ft1000/ft1000-usb/ft1000_debug.c |  6 +++---
 drivers/staging/ft1000/ft1000-usb/ft1000_hw.c| 25 +---
 drivers/staging/ft1000/ft1000-usb/ft1000_usb.h   |  2 +-
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_debug.c 
b/drivers/staging/ft1000/ft1000-usb/ft1000_debug.c
index a8945b7..9f4c785 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_debug.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_debug.c
@@ -482,14 +482,14 @@ static long ft1000_ioctl(struct file *file, unsigned int 
command,
 /* Connect Message */
 DEBUG("FT1000:ft1000_ioctl: IOCTL_FT1000_CONNECT\n");
 ConnectionMsg[79] = 0xfc;
-  card_send_command(ft1000dev, (unsigned short 
*)ConnectionMsg, 0x4c);
+  result = card_send_command(ft1000dev, (unsigned 
short *)ConnectionMsg, 0x4c);
 
 break;
 case IOCTL_DISCONNECT:
 /* Disconnect Message */
 DEBUG("FT1000:ft1000_ioctl: IOCTL_FT1000_DISCONNECT\n");
 ConnectionMsg[79] = 0xfd;
-  card_send_command(ft1000dev, (unsigned short 
*)ConnectionMsg, 0x4c);
+  result = card_send_command(ft1000dev, (unsigned 
short *)ConnectionMsg, 0x4c);
 break;
 case IOCTL_GET_DSP_STAT_CMD:
 /* DEBUG("FT1000:ft1000_ioctl: IOCTL_FT1000_GET_DSP_STAT called\n"); */
@@ -652,7 +652,7 @@ static long ft1000_ioctl(struct file *file, unsigned int 
command,
 }
 pmsg++;
ppseudo_hdr = (struct pseudo_hdr *)pmsg;
-   card_send_command(ft1000dev,(unsigned 
short*)dpram_data,total_len+2);
+   result = card_send_command(ft1000dev,(unsigned 
short*)dpram_data,total_len+2);
 
 
 ft1000dev->app_info[app_index].nTxMsg++;
diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c 
b/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
index b6a7708..7012e09 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
@@ -322,18 +322,23 @@ static void card_reset_dsp(struct ft1000_usb *ft1000dev, 
bool value)
 *   ptempbuffer - command buffer
 *   size - command buffer size
 */
-void card_send_command(struct ft1000_usb *ft1000dev, void *ptempbuffer,
+int card_send_command(struct ft1000_usb *ft1000dev, void *ptempbuffer,
   int size)
 {
+   int ret;
unsigned short temp;
unsigned char *commandbuf;
 
DEBUG("card_send_command: enter card_send_command... size=%d\n", size);
 
commandbuf = kmalloc(size + 2, GFP_KERNEL);
+   if (!commandbuf)
+   return -ENOMEM;
memcpy((void *)commandbuf + 2, (void *)ptempbuffer, size);
 
-   ft1000_read_register(ft1000dev, &temp, FT1000_REG_DOORBELL);
+   ret = ft1000_read_register(ft1000dev, &temp, FT1000_REG_DOORBELL);
+   if (ret)
+   return ret;
 
if (temp & 0x0100)
usleep_range(900, 1100);
@@ -345,19 +350,23 @@ void card_send_command(struct ft1000_usb *ft1000dev, void 
*ptempbuffer,
if (size % 4)
size += 4 - (size % 4);
 
-   ft1000_write_dpram32(ft1000dev, 0, commandbuf, size);
+   ret = ft1000_write_dpram32(ft1000dev, 0, commandbuf, size);
+   if (ret)
+   return ret;
usleep_range(900, 1100);
-   ft1000_write_register(ft1000dev, FT1000_DB_DPRAM_TX,
+   ret = ft1000_write_register(ft1000dev, FT1000_DB_DPRAM_TX,
  FT1000_REG_DOORBELL);
+   if (ret)
+   return ret;
usleep_range(900, 1100);
 
-   ft1000_read_register(ft1000dev, &temp, FT1000_REG_DOORBELL);
+   ret = ft1000_read_register(ft1000dev, &temp, FT1000_REG_DOORBELL);
 
 #if 0
if ((temp & 0x0100) == 0)
DEBUG("card_send_command: Message sent\n");
 #endif
-
+   return ret;
 }
 
 /* load or reload the DSP */
@@ -1375,8 +1384,10 @@ static int ft1000_proc_drvmsg(struct ft1000_usb *dev, 
u16 size)
*pmsg++ = convert.wrd;
*pmsg++ = htons(info->DrvErrNum);
 
-   card_send_command(dev, (unsigned char *)&tempbuffer[0],
+   status = card_send_command(dev, (unsigned char 
*)&tempbuffer[0],
(u16)(0x0012 + PSEUDOSZ));
+   if (status)
+   goto

[PATCH] staging: rtl8192ee: Correct bitmask in comparsion

[Andrey Utkin]

The issue is discovered by static checker. The proposed change (0x000c0
-> 0x000c) is likely correct because:
1. 16-bit `map` holds value coming from struct
ieee80211_vht_mcs_info.tx_mcs_map, which is described so: "TX MCS map 2
bits for each stream, total 8 streams". The changed code refers to case
of 2 TX streams, and 0x000c mask filters two bits related to the second
stream. Some codelines below 0x0003 mask is used to test first stream.
2. Mask 0x000c is used 3 more times in that place.
3. Specifying 5 digits of hex value is uncommon, especially while working
with `u16` variable. So likely the trailing zero is a typo.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=78041
Reported-by: David Binderman 
Signed-off-by: Andrey Utkin 
---
 drivers/staging/rtl8192ee/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8192ee/base.c b/drivers/staging/rtl8192ee/base.c
index a7c69f7..71ed12e 100644
--- a/drivers/staging/rtl8192ee/base.c
+++ b/drivers/staging/rtl8192ee/base.c
@@ -827,7 +827,7 @@ static u8 _rtl_get_vht_highest_n_rate(struct ieee80211_hw 
*hw,
u16 map = le16_to_cpu(sta->vht_cap.vht_mcs.tx_mcs_map);
 
if ((get_rf_type(rtlphy) == RF_2T2R) &&
-   (map & 0x000c) != 0x000c0) {
+   (map & 0x000c) != 0x000c) {
if ((map & 0x000c) >> 2 == IEEE80211_VHT_MCS_SUPPORT_0_7)
hw_rate =
rtlpriv->cfg->maps[RTL_RC_VHT_RATE_2SS_MCS7];
-- 
1.8.3.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel