Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Tomasz Figa]

Hi Yong,

On Thu, Jul 13, 2017 at 8:20 AM, Zhi, Yong  wrote:
> Hi, Sakari,
>
> Thanks for the time spent on code review, acks to all the comments, except 
> two places:
>
>> +/* .complete() is called after all subdevices have been located */
>> +static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
>> +{
>> + struct cio2_device *cio2 = container_of(notifier, struct cio2_device,
>> + notifier);
>> + struct sensor_async_subdev *s_asd;
>> + struct fwnode_handle *fwn_remote, *fwn_endpt, *fwn_remote_endpt;
>> + struct cio2_queue *q;
>> + struct fwnode_endpoint remote_endpt;
>> + unsigned int i, pad;
>> + int ret;
>> +
>> + for (i = 0; i < notifier->num_subdevs; i++) {
>> + s_asd = container_of(cio2->notifier.subdevs[i],
>> + struct sensor_async_subdev,
>> + asd);
>> +
>> + fwn_remote = s_asd->asd.match.fwnode.fwnode;
>> + fwn_endpt = (struct fwnode_handle *)
>> + s_asd->vfwn_endpt.base.local_fwnode;
>
> Why do you need a cast?
>
> [YZ] With a cast results in compilation warning:

(I think you mean "without".)

>
> drivers/media/pci/ipu3/ipu3-cio2.c:1298:13: warning: assignment discards 
> ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>fwn_endpt = /*(struct fwnode_handle *)*/

This is a sign that the code is doing something wrong (in this case
probably trying to write to a const pointer), so casting just silences
the unfixed error.

>
>> + ret = v4l2_async_notifier_register(>v4l2_dev, >notifier);
>> + if (ret) {
>> + cio2->notifier.num_subdevs = 0;
>
> No need to assign num_subdevs as 0.
>
> [YZ] _notifier_exit() will call _unregister() if this is not 0.

You shouldn't call _exit() if _init() failed. I noticed that many
error paths in your code does this. Please fix it.

Best regards,
Tomasz

cron job: media_tree daily build: ERRORS

[Hans Verkuil]

This message is generated daily by a cron job that builds media_tree for
the kernels and architectures in the list below.

Results of the daily build of media_tree:

date:   Thu Jul 13 05:00:18 CEST 2017
media-tree git hash:2748e76ddb2967c4030171342ebdd3faa6a5e8e8
media_build git hash:   bc1db0a204a87da86349ea5e64ae0d65e945609d
v4l-utils git hash: 8e68406dae2233e811032dc8e7714c09c818e893
gcc version:i686-linux-gcc (GCC) 7.1.0
sparse version: v0.5.0-3553-g78b2ea6
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.9.0-164

linux-git-arm-at91: WARNINGS
linux-git-arm-davinci: WARNINGS
linux-git-arm-multi: WARNINGS
linux-git-arm-pxa: OK
linux-git-arm-stm32: OK
linux-git-blackfin-bf561: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: WARNINGS
linux-2.6.36.4-i686: WARNINGS
linux-2.6.37.6-i686: WARNINGS
linux-2.6.38.8-i686: WARNINGS
linux-2.6.39.4-i686: OK
linux-3.0.60-i686: OK
linux-3.1.10-i686: OK
linux-3.2.37-i686: OK
linux-3.3.8-i686: OK
linux-3.4.27-i686: ERRORS
linux-3.5.7-i686: OK
linux-3.6.11-i686: OK
linux-3.7.4-i686: OK
linux-3.8-i686: OK
linux-3.9.2-i686: OK
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: OK
linux-3.12.67-i686: OK
linux-3.13.11-i686: WARNINGS
linux-3.14.9-i686: ERRORS
linux-3.15.2-i686: ERRORS
linux-3.16.7-i686: ERRORS
linux-3.17.8-i686: ERRORS
linux-3.18.7-i686: ERRORS
linux-3.19-i686: WARNINGS
linux-4.0.9-i686: WARNINGS
linux-4.1.33-i686: WARNINGS
linux-4.2.8-i686: WARNINGS
linux-4.3.6-i686: WARNINGS
linux-4.4.22-i686: WARNINGS
linux-4.5.7-i686: WARNINGS
linux-4.6.7-i686: WARNINGS
linux-4.7.5-i686: WARNINGS
linux-4.8-i686: OK
linux-4.9.26-i686: OK
linux-4.10.14-i686: OK
linux-4.11-i686: OK
linux-4.12.1-i686: OK
linux-2.6.36.4-x86_64: WARNINGS
linux-2.6.37.6-x86_64: WARNINGS
linux-2.6.38.8-x86_64: WARNINGS
linux-2.6.39.4-x86_64: WARNINGS
linux-3.0.60-x86_64: WARNINGS
linux-3.1.10-x86_64: WARNINGS
linux-3.2.37-x86_64: WARNINGS
linux-3.3.8-x86_64: WARNINGS
linux-3.4.27-x86_64: ERRORS
linux-3.5.7-x86_64: WARNINGS
linux-3.6.11-x86_64: WARNINGS
linux-3.7.4-x86_64: WARNINGS
linux-3.8-x86_64: WARNINGS
linux-3.9.2-x86_64: WARNINGS
linux-3.10.1-x86_64: WARNINGS
linux-3.11.1-x86_64: WARNINGS
linux-3.12.67-x86_64: WARNINGS
linux-3.13.11-x86_64: WARNINGS
linux-3.14.9-x86_64: ERRORS
linux-3.15.2-x86_64: ERRORS
linux-3.16.7-x86_64: ERRORS
linux-3.17.8-x86_64: WARNINGS
linux-3.18.7-x86_64: WARNINGS
linux-3.19-x86_64: WARNINGS
linux-4.0.9-x86_64: WARNINGS
linux-4.1.33-x86_64: WARNINGS
linux-4.2.8-x86_64: WARNINGS
linux-4.3.6-x86_64: WARNINGS
linux-4.4.22-x86_64: WARNINGS
linux-4.5.7-x86_64: WARNINGS
linux-4.6.7-x86_64: WARNINGS
linux-4.7.5-x86_64: WARNINGS
linux-4.8-x86_64: WARNINGS
linux-4.9.26-x86_64: WARNINGS
linux-4.10.14-x86_64: WARNINGS
linux-4.11-x86_64: WARNINGS
linux-4.12.1-x86_64: WARNINGS
apps: WARNINGS
spec-git: OK
sparse: WARNINGS

Detailed results are available here:

http://www.xs4all.nl/~hverkuil/logs/Thursday.log

Full logs are available here:

http://www.xs4all.nl/~hverkuil/logs/Thursday.tar.bz2

The Media Infrastructure API from this daily build is here:

http://www.xs4all.nl/~hverkuil/spec/index.html

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Antti Palosaari]

On 07/13/2017 03:04 AM, Antti Palosaari wrote:

On 07/13/2017 02:45 AM, Jasmin J. wrote:

Hello Antti!


Have you ever looked that coding style doc?

Yes I read it several times already and used it in my daily work in my
previous company.

Beside the Multi-line comment style, which I will fix in a follow up,
you mentioned other issues.
Please can you tell me which one you mean, so that I can check the series
for those things.


eh, OK, here short list from my head:
* you fixed comments, but left //-comments

* many cases where if (ret != 0), which generally should be written as 
if (ret). If you expect it is just error ret value, then prefer if 
(ret), but if ret has some other meaning like it returns number of bytes 
then if you expect 0-bytes returned (ret != 0) is also valid.


* unnecessary looking line split like that:
if (a
   & b)

* logical continuous line split wrong (I think I have seen checkpatch 
reported that kind of mistakes, dunno why not now)

if (a
 && b)
== >
if (a &&
 b)


actually it reports, when run --strict mode:

+   if (a
+   && b) {
+   foo(a);
+   foo(b);
+   }
+

CHECK: Logical continuations should be on the previous line
#11: FILE: drivers/media/usb/dvb-usb-v2/af9035.c:2135:
+   if (a
+   && b) {


Antti
--
http://palosaari.fi/

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Antti Palosaari]

On 07/13/2017 02:45 AM, Jasmin J. wrote:

Hello Antti!


Have you ever looked that coding style doc?

Yes I read it several times already and used it in my daily work in my
previous company.

Beside the Multi-line comment style, which I will fix in a follow up,
you mentioned other issues.
Please can you tell me which one you mean, so that I can check the series
for those things.


eh, OK, here short list from my head:
* you fixed comments, but left //-comments

* many cases where if (ret != 0), which generally should be written as 
if (ret). If you expect it is just error ret value, then prefer if 
(ret), but if ret has some other meaning like it returns number of bytes 
then if you expect 0-bytes returned (ret != 0) is also valid.


* unnecessary looking line split like that:
if (a
  & b)

* logical continuous line split wrong (I think I have seen checkpatch 
reported that kind of mistakes, dunno why not now)

if (a
&& b)
== >
if (a &&
b)


Antti

--
http://palosaari.fi/

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Jasmin J.]

Hello Antti!

> Have you ever looked that coding style doc?
Yes I read it several times already and used it in my daily work in my
previous company.

Beside the Multi-line comment style, which I will fix in a follow up,
you mentioned other issues.
Please can you tell me which one you mean, so that I can check the series
for those things.

BR,
   Jasmin

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Antti Palosaari]

On 07/13/2017 02:23 AM, Jasmin J. wrote:

Hello Antti!


Quickly looking this patch serie I noticed few other coding style mistakes.
You should read kernel coding style documentation first, and then make
changes according to doc.

In fact I used checkpatch.pl to find the issues and fixed them. All the patches
are 100% checkpatch.pl tested and did not have one single error or warning.

So please can you point me to those issues you mean.


Have you ever looked that coding style doc? Maybe better to start 
reading it first. Checkpatch is only a tool, it is nothing which makes 
100% decision which is correct or not.


Multi-line comment style is explained on section 8 on kernel coding 
style doc.


Antti


--
http://palosaari.fi/

Re: [PATCH] Added support for the TerraTec T1 DVB-T USB tuner [IT9135 chipset]

[Antti Palosaari]

On 06/29/2017 08:55 PM, Nuno Henriques wrote:

Signed-off-by: Nuno Henriques 
---
  drivers/media/dvb-core/dvb-usb-ids.h  | 1 +
  drivers/media/usb/dvb-usb-v2/af9035.c | 2 ++
  2 files changed, 3 insertions(+)

diff --git a/drivers/media/dvb-core/dvb-usb-ids.h 
b/drivers/media/dvb-core/dvb-usb-ids.h
index e200aa6f2d2f..5b6041d462bc 100644
--- a/drivers/media/dvb-core/dvb-usb-ids.h
+++ b/drivers/media/dvb-core/dvb-usb-ids.h
@@ -279,6 +279,7 @@
  #define USB_PID_TERRATEC_H7   0x10b4
  #define USB_PID_TERRATEC_H7_2 0x10a3
  #define USB_PID_TERRATEC_H7_3 0x10a5
+#define USB_PID_TERRATEC_T10x10ae
  #define USB_PID_TERRATEC_T3   0x10a0
  #define USB_PID_TERRATEC_T5   0x10a1
  #define USB_PID_NOXON_DAB_STICK   0x00b3
diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c 
b/drivers/media/usb/dvb-usb-v2/af9035.c
index 4df9486e19b9..ccf4a5c68877 100644
--- a/drivers/media/usb/dvb-usb-v2/af9035.c
+++ b/drivers/media/usb/dvb-usb-v2/af9035.c
@@ -2108,6 +2108,8 @@ static const struct usb_device_id af9035_id_table[] = {
{ DVB_USB_DEVICE(USB_VID_KWORLD_2, USB_PID_CTVDIGDUAL_V2,
_props, "Digital Dual TV Receiver CTVDIGDUAL_V2",
RC_MAP_IT913X_V1) },
+   { DVB_USB_DEVICE(USB_VID_TERRATEC, USB_PID_TERRATEC_T1,
+   _props, "TerraTec T1", RC_MAP_IT913X_V1) },
/* XXX: that same ID [0ccd:0099] is used by af9015 driver too */
{ DVB_USB_DEVICE(USB_VID_TERRATEC, 0x0099,
_props, "TerraTec Cinergy T Stick Dual RC (rev. 2)",




Does this stick has a remote? I see always red when I see someone adds 
RC_MAP_IT913X_V1 remote controller as there is now too many simply 
totally wrongly defined remote controllers on that driver.


Commit message is missing, even it is very trivial patch there should be 
something like It is IT9135BX device having USB ID : and remote 
controller model is x..


Use git log to see other commit messages where new usb id is added.

regards
Antti

--
http://palosaari.fi/

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Jasmin J.]

Hello Antti!

> Quickly looking this patch serie I noticed few other coding style mistakes.
> You should read kernel coding style documentation first, and then make
> changes according to doc.
In fact I used checkpatch.pl to find the issues and fixed them. All the patches
are 100% checkpatch.pl tested and did not have one single error or warning.

So please can you point me to those issues you mean.

BR,
   Jasmin

RE: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Zhi, Yong]

Hi, Sakari,

Thanks for the time spent on code review, acks to all the comments, except two 
places:

> +/* .complete() is called after all subdevices have been located */
> +static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct cio2_device *cio2 = container_of(notifier, struct cio2_device,
> + notifier);
> + struct sensor_async_subdev *s_asd;
> + struct fwnode_handle *fwn_remote, *fwn_endpt, *fwn_remote_endpt;
> + struct cio2_queue *q;
> + struct fwnode_endpoint remote_endpt;
> + unsigned int i, pad;
> + int ret;
> +
> + for (i = 0; i < notifier->num_subdevs; i++) {
> + s_asd = container_of(cio2->notifier.subdevs[i],
> + struct sensor_async_subdev,
> + asd);
> +
> + fwn_remote = s_asd->asd.match.fwnode.fwnode;
> + fwn_endpt = (struct fwnode_handle *)
> + s_asd->vfwn_endpt.base.local_fwnode;

Why do you need a cast?

[YZ] With a cast results in compilation warning:

drivers/media/pci/ipu3/ipu3-cio2.c:1298:13: warning: assignment discards 
‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   fwn_endpt = /*(struct fwnode_handle *)*/

> + ret = v4l2_async_notifier_register(>v4l2_dev, >notifier);
> + if (ret) {
> + cio2->notifier.num_subdevs = 0;

No need to assign num_subdevs as 0.

[YZ] _notifier_exit() will call _unregister() if this is not 0.

Regards,

Yong


From: linux-media-ow...@vger.kernel.org [linux-media-ow...@vger.kernel.org] on 
behalf of Sakari Ailus [sakari.ai...@iki.fi]
Sent: Tuesday, July 11, 2017 3:33 AM
To: Zhi, Yong
Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; 
hans.verk...@cisco.com; Zheng, Jian Xu; tf...@chromium.org; Mani, Rajmohan; 
Toivonen, Tuukka; Yang, Hyungwoo; Vijaykumar, Ramya
Subject: Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

Hi Yong,

Thanks for the update! This looks pretty good in general, still some more
comments below.

Do you happen to have a todo list for the changes that are still planned?
AFAIR we should have two items in the list at least ---

1. Extend the format example to include the DMA word boundary and

2. Separate formats on the sub-device and on the video node.

On Mon, Jul 10, 2017 at 06:43:34PM -0500, Yong Zhi wrote:
> This patch adds CIO2 CSI-2 device driver for
> Intel's IPU3 camera sub-system support.
>
> Signed-off-by: Yong Zhi 
> Signed-off-by: Hyungwoo Yang 
> Signed-off-by: Ramya Vijaykumar 
> ---
>  drivers/media/pci/Kconfig|2 +
>  drivers/media/pci/Makefile   |3 +-
>  drivers/media/pci/intel/Makefile |5 +
>  drivers/media/pci/intel/ipu3/Kconfig |   17 +
>  drivers/media/pci/intel/ipu3/Makefile|1 +
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1873 
> ++
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h |  442 +++
>  7 files changed, 2342 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/media/pci/intel/Makefile
>  create mode 100644 drivers/media/pci/intel/ipu3/Kconfig
>  create mode 100644 drivers/media/pci/intel/ipu3/Makefile
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.c
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.h
>
> diff --git a/drivers/media/pci/Kconfig b/drivers/media/pci/Kconfig
> index da28e68..5932e22 100644
> --- a/drivers/media/pci/Kconfig
> +++ b/drivers/media/pci/Kconfig
> @@ -54,5 +54,7 @@ source "drivers/media/pci/smipcie/Kconfig"
>  source "drivers/media/pci/netup_unidvb/Kconfig"
>  endif
>
> +source "drivers/media/pci/intel/ipu3/Kconfig"
> +
>  endif #MEDIA_PCI_SUPPORT
>  endif #PCI
> diff --git a/drivers/media/pci/Makefile b/drivers/media/pci/Makefile
> index a7e8af0..d8f9843 100644
> --- a/drivers/media/pci/Makefile
> +++ b/drivers/media/pci/Makefile
> @@ -13,7 +13,8 @@ obj-y+= ttpci/  \
>   ddbridge/   \
>   saa7146/\
>   smipcie/\
> - netup_unidvb/
> + netup_unidvb/   \
> + intel/
>
>  obj-$(CONFIG_VIDEO_IVTV) += ivtv/
>  obj-$(CONFIG_VIDEO_ZORAN) += zoran/
> diff --git a/drivers/media/pci/intel/Makefile 
> b/drivers/media/pci/intel/Makefile
> new file mode 100644
> index 000..745c8b2
> --- /dev/null
> +++ b/drivers/media/pci/intel/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for the IPU3 cio2 and ImGU drivers
> +#
> +
> +obj-y+= ipu3/
> diff --git a/drivers/media/pci/intel/ipu3/Kconfig 
> b/drivers/media/pci/intel/ipu3/Kconfig
> new file mode 100644
> index 000..2a895d6
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> @@ -0,0 +1,17 @@
> +config VIDEO_IPU3_CIO2
> + tristate "Intel ipu3-cio2 driver"
> + depends on 

Re: [PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Antti Palosaari]

On 07/13/2017 02:00 AM, Jasmin J. wrote:

From: Jasmin Jessich 

Fixed all:
   WARNING: Block comments use * on subsequent lines


Also multiline comments should be written like this:
/*
 * Comment.
 */

Quickly looking this patch serie I noticed few other coding style 
mistakes. You should read kernel coding style documentation first, and 
then make changes according to doc.


regards
Antti

--
http://palosaari.fi/

[PATCH V2 7/9] [media] dvb-core/dvb_ca_en50221.c: Added line breaks

[Jasmin J.]

From: Jasmin Jessich 

Fixed all:
  WARNING: Missing a blank line after declarations

Signed-off-by: Jasmin Jessich 
---
 drivers/media/dvb-core/dvb_ca_en50221.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
b/drivers/media/dvb-core/dvb_ca_en50221.c
index eb7f692..5a0b35d 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -178,7 +178,9 @@ static void dvb_ca_private_free(struct dvb_ca_private *ca)
 
 static void dvb_ca_private_release(struct kref *ref)
 {
-   struct dvb_ca_private *ca = container_of(ref, struct dvb_ca_private, 
refcount);
+   struct dvb_ca_private *ca = container_of(ref, struct dvb_ca_private,
+refcount);
+
dvb_ca_private_free(ca);
 }
 
@@ -298,7 +300,9 @@ static int dvb_ca_en50221_wait_if_status(struct 
dvb_ca_private *ca, int slot,
timeout = jiffies + timeout_hz;
while (1) {
/* read the status and check for error */
-   int res = ca->pub->read_cam_control(ca->pub, slot, 
CTRLIF_STATUS);
+   int res = ca->pub->read_cam_control(ca->pub, slot,
+   CTRLIF_STATUS);
+
if (res < 0)
return -EIO;
 
-- 
2.7.4

[PATCH V2 0/9] Fix coding style in en50221 CAM functions

[Jasmin J.]

From: Jasmin Jessich 

These patch series is the V2 version adapted to the already merged patches
from V1 and other merged patches. It does no longer rename constants, as
stated by Mauro as a no-go. It still fixed nearly all the style issues 
reported by checkpatch.pl in dvb-core/dvb_ca_en50221.c, but keeps more open
than the V1 version for good reasons. I also renamed the patch title,
because "Make checkpatch happy ?" is a bad title and checkpatch complained
about it. I also refactored the stat machine a bit more and added the new
function dvb_ca_en50221_poll_cam_gone in a new patch.
 
Two of them are "WARNING: memory barrier without comment". I have really
no clue why there is a call to "mb()" in that file, so I can't fill in a
good comment.
 
I kept some of the "WARNING: line over 80 characters" findings, which are 
strings for debugging, which shouldn't be split in several lines (will 
give other warning). And some lines, where a change would decrease the
readability. There it would be better to split the functions in new
subroutines, which I currently didn't.
 
And finally one "WARNING: Prefer [subsystem eg: netdev]_dbg", complaining
about the "dprintk" macro. In my opinion it is correctly used and it is
normally disabled anyway.
 
The main problem of the original code was the size of the lines and the
structural complexity of some functions. Beside refactoring of the thread
state machine, I used in nearly every function a helper pointer "sl" (for
"slot" structure) instead the whole structure path. This saved also a lot
of characters in long lines and made the code more readable in my opinion.
 
I split the patch set is small pieces for easier review, compiled each
step and tested the resulting driver on my hardware with the DD DuoFlex CI
(single) card. I took a lot of care in the first two patches to really
keep the code as is without loosing any line during refactoring.

Jasmin Jessich (9):
  [media] dvb-core/dvb_ca_en50221.c: Refactored dvb_ca_en50221_thread
  [media] dvb-core/dvb_ca_en50221.c: New function dvb_ca_en50221_poll_cam_gone
  [media] dvb-core/dvb_ca_en50221.c: use usleep_range
  [media] dvb-core/dvb_ca_en50221.c: Fixed block comments
  [media] dvb-core/dvb_ca_en50221.c: Avoid assignments in ifs
  [media] dvb-core/dvb_ca_en50221.c: Used a helper variable
  [media] dvb-core/dvb_ca_en50221.c: Added line breaks
  [media] dvb-core/dvb_ca_en50221.c: Removed useless braces
  [media] dvb-core/dvb_ca_en50221.c: Fixed 80 char limit

 drivers/media/dvb-core/dvb_ca_en50221.c | 774 ++--
 1 file changed, 441 insertions(+), 333 deletions(-)

-- 
2.7.4

[PATCH V2 4/9] [media] dvb-core/dvb_ca_en50221.c: Fixed block comments

[Jasmin J.]

From: Jasmin Jessich 

Fixed all:
  WARNING: Block comments use * on subsequent lines

Signed-off-by: Jasmin Jessich 
---
 drivers/media/dvb-core/dvb_ca_en50221.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
b/drivers/media/dvb-core/dvb_ca_en50221.c
index c0fd63a..317968b 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -343,7 +343,8 @@ static int dvb_ca_en50221_link_init(struct dvb_ca_private 
*ca, int slot)
ca->slot_info[slot].da_irq_supported = 0;
 
/* set the host link buffer size temporarily. it will be overwritten 
with the
-* real negotiated size later. */
+* real negotiated size later.
+*/
ca->slot_info[slot].link_buf_size = 2;
 
/* read the buffer size from the CAM */
@@ -797,9 +798,10 @@ static int dvb_ca_en50221_write_data(struct dvb_ca_private 
*ca, int slot,
return ca->pub->write_data(ca->pub, slot, buf, bytes_write);
 
/* it is possible we are dealing with a single buffer implementation,
-  thus if there is data available for read or if there is even a read
-  already in progress, we do nothing but awake the kernel thread to
-  process the data if necessary. */
+* thus if there is data available for read or if there is even a read
+* already in progress, we do nothing but awake the kernel thread to
+* process the data if necessary.
+*/
if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) 
< 0)
goto exitnowrite;
if (status & (STATUSREG_DA | STATUSREG_RE)) {
@@ -899,8 +901,9 @@ static int dvb_ca_en50221_slot_shutdown(struct 
dvb_ca_private *ca, int slot)
ca->pub->slot_shutdown(ca->pub, slot);
ca->slot_info[slot].slot_state = DVB_CA_SLOTSTATE_NONE;
 
-   /* need to wake up all processes to check if they're now
-  trying to write to a defunct CAM */
+   /* need to wake up all processes to check if they're now trying to
+* write to a defunct CAM
+*/
wake_up_interruptible(>wait_queue);
 
dprintk("Slot %i shutdown\n", slot);
@@ -1681,8 +1684,10 @@ static int dvb_ca_en50221_io_open(struct inode *inode, 
struct file *file)
 
if (ca->slot_info[i].slot_state == DVB_CA_SLOTSTATE_RUNNING) {
if (ca->slot_info[i].rx_buffer.data != NULL) {
-   /* it is safe to call this here without locks 
because
-* ca->open == 0. Data is not read in this case 
*/
+   /* it is safe to call this here without locks
+* because ca->open == 0. Data is not read in
+* this case
+*/

dvb_ringbuffer_flush(>slot_info[i].rx_buffer);
}
}
-- 
2.7.4

[PATCH V2 3/9] [media] dvb-core/dvb_ca_en50221.c: use usleep_range

[Jasmin J.]

From: Jasmin Jessich 

Fixed all:
  WARNING: msleep < 20ms can sleep for up to 20ms
by using usleep_range.

Signed-off-by: Jasmin Jessich 
---
 drivers/media/dvb-core/dvb_ca_en50221.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
b/drivers/media/dvb-core/dvb_ca_en50221.c
index 66a58ed..c0fd63a 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -313,7 +313,7 @@ static int dvb_ca_en50221_wait_if_status(struct 
dvb_ca_private *ca, int slot,
}
 
/* wait for a bit */
-   msleep(1);
+   usleep_range(1000, 1100);
}
 
dprintk("%s failed timeout:%lu\n", __func__, jiffies - start);
@@ -1484,7 +1484,7 @@ static ssize_t dvb_ca_en50221_io_write(struct file *file,
if (status != -EAGAIN)
goto exit;
 
-   msleep(1);
+   usleep_range(1000, 1100);
}
if (!written) {
status = -EIO;
-- 
2.7.4

[PATCH V2 6/9] [media] dvb-core/dvb_ca_en50221.c: Used a helper variable

[Jasmin J.]

From: Jasmin Jessich 

Used a helper variable "struct dvb_ca_slot *sl" instead of
"ca->slot_info[slot]". This reduces the line length and simplifies
code reading.

Signed-off-by: Jasmin Jessich 
---
 drivers/media/dvb-core/dvb_ca_en50221.c | 187 ++--
 1 file changed, 106 insertions(+), 81 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
b/drivers/media/dvb-core/dvb_ca_en50221.c
index 02b8785..eb7f692 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -234,13 +234,14 @@ static char *findstr(char *haystack, int hlen, char 
*needle, int nlen)
  */
 static int dvb_ca_en50221_check_camstatus(struct dvb_ca_private *ca, int slot)
 {
+   struct dvb_ca_slot *sl = >slot_info[slot];
int slot_status;
int cam_present_now;
int cam_changed;
 
/* IRQ mode */
if (ca->flags & DVB_CA_EN50221_FLAG_IRQ_CAMCHANGE) {
-   return (atomic_read(>slot_info[slot].camchange_count) != 0);
+   return (atomic_read(>camchange_count) != 0);
}
 
/* poll mode */
@@ -249,22 +250,23 @@ static int dvb_ca_en50221_check_camstatus(struct 
dvb_ca_private *ca, int slot)
cam_present_now = (slot_status & DVB_CA_EN50221_POLL_CAM_PRESENT) ? 1 : 
0;
cam_changed = (slot_status & DVB_CA_EN50221_POLL_CAM_CHANGED) ? 1 : 0;
if (!cam_changed) {
-   int cam_present_old = (ca->slot_info[slot].slot_state != 
DVB_CA_SLOTSTATE_NONE);
+   int cam_present_old = (sl->slot_state != DVB_CA_SLOTSTATE_NONE);
+
cam_changed = (cam_present_now != cam_present_old);
}
 
if (cam_changed) {
if (!cam_present_now) {
-   ca->slot_info[slot].camchange_type = 
DVB_CA_EN50221_CAMCHANGE_REMOVED;
+   sl->camchange_type = DVB_CA_EN50221_CAMCHANGE_REMOVED;
} else {
-   ca->slot_info[slot].camchange_type = 
DVB_CA_EN50221_CAMCHANGE_INSERTED;
+   sl->camchange_type = DVB_CA_EN50221_CAMCHANGE_INSERTED;
}
-   atomic_set(>slot_info[slot].camchange_count, 1);
+   atomic_set(>camchange_count, 1);
} else {
-   if ((ca->slot_info[slot].slot_state == 
DVB_CA_SLOTSTATE_WAITREADY) &&
+   if ((sl->slot_state == DVB_CA_SLOTSTATE_WAITREADY) &&
(slot_status & DVB_CA_EN50221_POLL_CAM_READY)) {
// move to validate state if reset is completed
-   ca->slot_info[slot].slot_state = 
DVB_CA_SLOTSTATE_VALIDATE;
+   sl->slot_state = DVB_CA_SLOTSTATE_VALIDATE;
}
}
 
@@ -333,6 +335,7 @@ static int dvb_ca_en50221_wait_if_status(struct 
dvb_ca_private *ca, int slot,
  */
 static int dvb_ca_en50221_link_init(struct dvb_ca_private *ca, int slot)
 {
+   struct dvb_ca_slot *sl = >slot_info[slot];
int ret;
int buf_size;
u8 buf[2];
@@ -340,12 +343,12 @@ static int dvb_ca_en50221_link_init(struct dvb_ca_private 
*ca, int slot)
dprintk("%s\n", __func__);
 
/* we'll be determining these during this function */
-   ca->slot_info[slot].da_irq_supported = 0;
+   sl->da_irq_supported = 0;
 
/* set the host link buffer size temporarily. it will be overwritten 
with the
 * real negotiated size later.
 */
-   ca->slot_info[slot].link_buf_size = 2;
+   sl->link_buf_size = 2;
 
/* read the buffer size from the CAM */
ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND,
@@ -366,7 +369,7 @@ static int dvb_ca_en50221_link_init(struct dvb_ca_private 
*ca, int slot)
buf_size = (buf[0] << 8) | buf[1];
if (buf_size > HOST_LINK_BUF_SIZE)
buf_size = HOST_LINK_BUF_SIZE;
-   ca->slot_info[slot].link_buf_size = buf_size;
+   sl->link_buf_size = buf_size;
buf[0] = buf_size >> 8;
buf[1] = buf_size & 0xff;
dprintk("Chosen link buffer size of %i\n", buf_size);
@@ -457,6 +460,7 @@ static int dvb_ca_en50221_read_tuple(struct dvb_ca_private 
*ca, int slot,
  */
 static int dvb_ca_en50221_parse_attributes(struct dvb_ca_private *ca, int slot)
 {
+   struct dvb_ca_slot *sl;
int address = 0;
int tupleLength;
int tupleType;
@@ -529,10 +533,10 @@ static int dvb_ca_en50221_parse_attributes(struct 
dvb_ca_private *ca, int slot)
rasz = tuple[0] & 3;
if (tupleLength < (3 + rasz + 14))
return -EINVAL;
-   ca->slot_info[slot].config_base = 0;
-   for (i = 0; i < rasz + 1; i++) {
-   ca->slot_info[slot].config_base |= (tuple[2 + i] << (8 * i));
-   }
+   sl = >slot_info[slot];
+   sl->config_base = 0;
+   for (i = 0; i < rasz + 1; i++)
+   sl->config_base |= (tuple[2 + i] << (8 * i));
 
/* check it contains 

[PATCH V2 1/9] [media] dvb-core/dvb_ca_en50221.c: Refactored dvb_ca_en50221_thread

[Jasmin J.]

From: Jasmin Jessich 

Refactored "dvb_ca_en50221_thread" by moving the state machine into the
new function "dvb_ca_en50221_thread_state_machine". This reduces the
thread function size and reduces the structural complexity and of course
gives us more space to meet the line length goal in the new function.

Signed-off-by: Jasmin Jessich 
---
 drivers/media/dvb-core/dvb_ca_en50221.c | 357 +---
 1 file changed, 192 insertions(+), 165 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
b/drivers/media/dvb-core/dvb_ca_en50221.c
index 17970cd..19d0e9a 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -1063,207 +1063,234 @@ static void dvb_ca_en50221_thread_update_delay(struct 
dvb_ca_private *ca)
ca->delay = curdelay;
 }
 
-
-
 /**
- * Kernel thread which monitors CA slots for CAM changes, and performs data 
transfers.
+ * Thread state machine for one CA slot to perform the data transfer.
+ *
+ * @ca: CA instance.
+ * @slot: Slot to process.
  */
-static int dvb_ca_en50221_thread(void *data)
+static void dvb_ca_en50221_thread_state_machine(struct dvb_ca_private *ca,
+   int slot)
 {
-   struct dvb_ca_private *ca = data;
-   int slot;
+   struct dvb_ca_slot *sl = >slot_info[slot];
int flags;
int status;
int pktcount;
void *rxbuf;
 
-   dprintk("%s\n", __func__);
+   mutex_lock(>slot_lock);
 
-   /* choose the correct initial delay */
-   dvb_ca_en50221_thread_update_delay(ca);
+   /* check the cam status + deal with CAMCHANGEs */
+   while (dvb_ca_en50221_check_camstatus(ca, slot)) {
+   /* clear down an old CI slot if necessary */
+   if (sl->slot_state != DVB_CA_SLOTSTATE_NONE)
+   dvb_ca_en50221_slot_shutdown(ca, slot);
 
-   /* main loop */
-   while (!kthread_should_stop()) {
-   /* sleep for a bit */
-   if (!ca->wakeup) {
-   set_current_state(TASK_INTERRUPTIBLE);
-   schedule_timeout(ca->delay);
-   if (kthread_should_stop())
-   return 0;
-   }
-   ca->wakeup = 0;
+   /* if a CAM is NOW present, initialise it */
+   if (sl->camchange_type == DVB_CA_EN50221_CAMCHANGE_INSERTED)
+   sl->slot_state = DVB_CA_SLOTSTATE_UNINITIALISED;
 
-   /* go through all the slots processing them */
-   for (slot = 0; slot < ca->slot_count; slot++) {
-
-   mutex_lock(>slot_info[slot].slot_lock);
-
-   // check the cam status + deal with CAMCHANGEs
-   while (dvb_ca_en50221_check_camstatus(ca, slot)) {
-   /* clear down an old CI slot if necessary */
-   if (ca->slot_info[slot].slot_state != 
DVB_CA_SLOTSTATE_NONE)
-   dvb_ca_en50221_slot_shutdown(ca, slot);
-
-   /* if a CAM is NOW present, initialise it */
-   if (ca->slot_info[slot].camchange_type == 
DVB_CA_EN50221_CAMCHANGE_INSERTED) {
-   ca->slot_info[slot].slot_state = 
DVB_CA_SLOTSTATE_UNINITIALISED;
-   }
+   /* we've handled one CAMCHANGE */
+   dvb_ca_en50221_thread_update_delay(ca);
+   atomic_dec(>camchange_count);
+   }
 
-   /* we've handled one CAMCHANGE */
-   dvb_ca_en50221_thread_update_delay(ca);
-   
atomic_dec(>slot_info[slot].camchange_count);
-   }
+   /* CAM state machine */
+   switch (sl->slot_state) {
+   case DVB_CA_SLOTSTATE_NONE:
+   case DVB_CA_SLOTSTATE_INVALID:
+   /* no action needed */
+   break;
 
-   // CAM state machine
-   switch (ca->slot_info[slot].slot_state) {
-   case DVB_CA_SLOTSTATE_NONE:
-   case DVB_CA_SLOTSTATE_INVALID:
-   // no action needed
-   break;
+   case DVB_CA_SLOTSTATE_UNINITIALISED:
+   sl->slot_state = DVB_CA_SLOTSTATE_WAITREADY;
+   ca->pub->slot_reset(ca->pub, slot);
+   sl->timeout = jiffies + (INIT_TIMEOUT_SECS * HZ);
+   break;
 
-   case DVB_CA_SLOTSTATE_UNINITIALISED:
-   ca->slot_info[slot].slot_state = 
DVB_CA_SLOTSTATE_WAITREADY;
-   ca->pub->slot_reset(ca->pub, slot);
-   ca->slot_info[slot].timeout = jiffies + 
(INIT_TIMEOUT_SECS * HZ);
-   

[PATCH V2 5/9] [media] dvb-core/dvb_ca_en50221.c: Avoid assignments in ifs

[Jasmin J.]

From: Jasmin Jessich 

Fixed all:
  ERROR: do not use assignment in if condition

Signed-off-by: Jasmin Jessich 
---
 drivers/media/dvb-core/dvb_ca_en50221.c | 97 ++---
 1 file changed, 64 insertions(+), 33 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
b/drivers/media/dvb-core/dvb_ca_en50221.c
index 317968b..02b8785 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -348,14 +348,18 @@ static int dvb_ca_en50221_link_init(struct dvb_ca_private 
*ca, int slot)
ca->slot_info[slot].link_buf_size = 2;
 
/* read the buffer size from the CAM */
-   if ((ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND, 
IRQEN | CMDREG_SR)) != 0)
+   ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND,
+IRQEN | CMDREG_SR);
+   if (ret != 0)
return ret;
ret = dvb_ca_en50221_wait_if_status(ca, slot, STATUSREG_DA, HZ);
if (ret != 0)
return ret;
-   if ((ret = dvb_ca_en50221_read_data(ca, slot, buf, 2)) != 2)
+   ret = dvb_ca_en50221_read_data(ca, slot, buf, 2);
+   if (ret != 2)
return -EIO;
-   if ((ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND, 
IRQEN)) != 0)
+   ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND, IRQEN);
+   if (ret != 0)
return ret;
 
/* store it, and choose the minimum of our buffer and the CAM's buffer 
size */
@@ -368,13 +372,18 @@ static int dvb_ca_en50221_link_init(struct dvb_ca_private 
*ca, int slot)
dprintk("Chosen link buffer size of %i\n", buf_size);
 
/* write the buffer size to the CAM */
-   if ((ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND, 
IRQEN | CMDREG_SW)) != 0)
+   ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND,
+IRQEN | CMDREG_SW);
+   if (ret != 0)
return ret;
-   if ((ret = dvb_ca_en50221_wait_if_status(ca, slot, STATUSREG_FR, HZ / 
10)) != 0)
+   ret = dvb_ca_en50221_wait_if_status(ca, slot, STATUSREG_FR, HZ / 10);
+   if (ret != 0)
return ret;
-   if ((ret = dvb_ca_en50221_write_data(ca, slot, buf, 2)) != 2)
+   ret = dvb_ca_en50221_write_data(ca, slot, buf, 2);
+   if (ret != 2)
return -EIO;
-   if ((ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND, 
IRQEN)) != 0)
+   ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND, IRQEN);
+   if (ret != 0)
return ret;
 
/* success */
@@ -403,7 +412,8 @@ static int dvb_ca_en50221_read_tuple(struct dvb_ca_private 
*ca, int slot,
int _address = *address;
 
/* grab the next tuple length and type */
-   if ((_tupleType = ca->pub->read_attribute_mem(ca->pub, slot, _address)) 
< 0)
+   _tupleType = ca->pub->read_attribute_mem(ca->pub, slot, _address);
+   if (_tupleType < 0)
return _tupleType;
if (_tupleType == 0xff) {
dprintk("END OF CHAIN TUPLE type:0x%x\n", _tupleType);
@@ -412,7 +422,8 @@ static int dvb_ca_en50221_read_tuple(struct dvb_ca_private 
*ca, int slot,
*tupleLength = 0;
return 0;
}
-   if ((_tupleLength = ca->pub->read_attribute_mem(ca->pub, slot, _address 
+ 2)) < 0)
+   _tupleLength = ca->pub->read_attribute_mem(ca->pub, slot, _address + 2);
+   if (_tupleLength < 0)
return _tupleLength;
_address += 4;
 
@@ -461,8 +472,9 @@ static int dvb_ca_en50221_parse_attributes(struct 
dvb_ca_private *ca, int slot)
 
 
// CISTPL_DEVICE_0A
-   if ((status =
-dvb_ca_en50221_read_tuple(ca, slot, , , 
, tuple)) < 0)
+   status = dvb_ca_en50221_read_tuple(ca, slot, , ,
+  , tuple);
+   if (status < 0)
return status;
if (tupleType != 0x1D)
return -EINVAL;
@@ -470,8 +482,9 @@ static int dvb_ca_en50221_parse_attributes(struct 
dvb_ca_private *ca, int slot)
 
 
// CISTPL_DEVICE_0C
-   if ((status =
-dvb_ca_en50221_read_tuple(ca, slot, , , 
, tuple)) < 0)
+   status = dvb_ca_en50221_read_tuple(ca, slot, , ,
+  , tuple);
+   if (status < 0)
return status;
if (tupleType != 0x1C)
return -EINVAL;
@@ -479,8 +492,9 @@ static int dvb_ca_en50221_parse_attributes(struct 
dvb_ca_private *ca, int slot)
 
 
// CISTPL_VERS_1
-   if ((status =
-dvb_ca_en50221_read_tuple(ca, slot, , , 
, tuple)) < 0)
+   status = dvb_ca_en50221_read_tuple(ca, slot, , ,
+  , tuple);
+   if (status < 0)
return status;
if (tupleType != 0x15)
   

[PATCH V2 9/9] [media] dvb-core/dvb_ca_en50221.c: Fixed 80 char limit

[Jasmin J.]

From: Jasmin Jessich 

Fixed most of:
  WARNING: line over 80 characters
The remaining lines are printk strings, which should not be split and
lines where I thing they should stay as they are.

Signed-off-by: Jasmin Jessich 
---
 drivers/media/dvb-core/dvb_ca_en50221.c | 57 +
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
b/drivers/media/dvb-core/dvb_ca_en50221.c
index 3e390a4..947c95c 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -76,7 +76,7 @@ MODULE_PARM_DESC(cam_debug, "enable verbose debug messages");
 #define STATUSREG_WE 2 /* write error */
 #define STATUSREG_FR  0x40 /* module free */
 #define STATUSREG_DA  0x80 /* data available */
-#define STATUSREG_TXERR (STATUSREG_RE|STATUSREG_WE)/* general transfer 
error */
+#define STATUSREG_TXERR (STATUSREG_RE|STATUSREG_WE) /* general transfer error 
*/
 
 
 #define DVB_CA_SLOTSTATE_NONE   0
@@ -157,7 +157,9 @@ struct dvb_ca_private {
/* Delay the main thread should use */
unsigned long delay;
 
-   /* Slot to start looking for data to read from in the next user-space 
read operation */
+   /* Slot to start looking for data to read from in the next user-space
+* read operation
+*/
int next_read_slot;
 
/* mutex serializing ioctls */
@@ -227,7 +229,7 @@ static char *findstr(char *haystack, int hlen, char 
*needle, int nlen)
 
 
 
-/* 

 */
+/* ** 
*/
 /* EN50221 physical interface functions */
 
 
@@ -346,8 +348,8 @@ static int dvb_ca_en50221_link_init(struct dvb_ca_private 
*ca, int slot)
/* we'll be determining these during this function */
sl->da_irq_supported = 0;
 
-   /* set the host link buffer size temporarily. it will be overwritten 
with the
-* real negotiated size later.
+   /* set the host link buffer size temporarily. it will be overwritten
+* with the real negotiated size later.
 */
sl->link_buf_size = 2;
 
@@ -366,7 +368,9 @@ static int dvb_ca_en50221_link_init(struct dvb_ca_private 
*ca, int slot)
if (ret != 0)
return ret;
 
-   /* store it, and choose the minimum of our buffer and the CAM's buffer 
size */
+   /* store it, and choose the minimum of our buffer and the CAM's buffer
+* size
+*/
buf_size = (buf[0] << 8) | buf[1];
if (buf_size > HOST_LINK_BUF_SIZE)
buf_size = HOST_LINK_BUF_SIZE;
@@ -435,7 +439,8 @@ static int dvb_ca_en50221_read_tuple(struct dvb_ca_private 
*ca, int slot,
 
/* read in the whole tuple */
for (i = 0; i < _tupleLength; i++) {
-   tuple[i] = ca->pub->read_attribute_mem(ca->pub, slot, _address 
+ (i * 2));
+   tuple[i] = ca->pub->read_attribute_mem(ca->pub, slot,
+  _address + (i * 2));
dprintk("  0x%02x: 0x%02x %c\n",
i, tuple[i] & 0xff,
((tuple[i] > 31) && (tuple[i] < 127)) ? tuple[i] : '.');
@@ -588,7 +593,7 @@ static int dvb_ca_en50221_parse_attributes(struct 
dvb_ca_private *ca, int slot)
end_chain = 1;
break;
 
-   default:/* Unknown tuple type - just skip this tuple 
and move to the next one */
+   default:/* Unknown tuple type - just skip this tuple */
dprintk("dvb_ca: Skipping unknown tuple type:0x%x 
length:0x%x\n",
tupleType, tupleLength);
break;
@@ -764,7 +769,9 @@ static int dvb_ca_en50221_read_data(struct dvb_ca_private 
*ca, int slot,
}
}
 
-   /* OK, add it to the receive buffer, or copy into external buffer if 
supplied */
+   /* OK, add it to the receive buffer, or copy into external buffer if
+* supplied
+*/
if (ebuf == NULL) {
if (sl->rx_buffer.data == NULL) {
status = -EIO;
@@ -915,7 +922,7 @@ static int dvb_ca_en50221_write_data(struct dvb_ca_private 
*ca, int slot,
 
 
 
-/* 

 */
+/* ** 
*/
 /* EN50221 higher level functions */
 
 
@@ -951,7 +958,8 @@ static int dvb_ca_en50221_slot_shutdown(struct 
dvb_ca_private *ca, int slot)
  * @slot: Slot concerned.
  * @change_type: One of the DVB_CA_CAMCHANGE_* values.
  */
-void dvb_ca_en50221_camchange_irq(struct dvb_ca_en50221 *pubca, int slot, int 
change_type)
+void dvb_ca_en50221_camchange_irq(struct dvb_ca_en50221 *pubca, int slot,
+   

[PATCH V2 8/9] [media] dvb-core/dvb_ca_en50221.c: Removed useless braces

[Jasmin J.]

From: Jasmin Jessich 

Fixed all:
  WARNING: braces {} are not necessary for single statement blocks

Signed-off-by: Jasmin Jessich 
---
 drivers/media/dvb-core/dvb_ca_en50221.c | 30 --
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
b/drivers/media/dvb-core/dvb_ca_en50221.c
index 5a0b35d..3e390a4 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -242,9 +242,8 @@ static int dvb_ca_en50221_check_camstatus(struct 
dvb_ca_private *ca, int slot)
int cam_changed;
 
/* IRQ mode */
-   if (ca->flags & DVB_CA_EN50221_FLAG_IRQ_CAMCHANGE) {
+   if (ca->flags & DVB_CA_EN50221_FLAG_IRQ_CAMCHANGE)
return (atomic_read(>camchange_count) != 0);
-   }
 
/* poll mode */
slot_status = ca->pub->poll_slot_status(ca->pub, slot, ca->open);
@@ -258,11 +257,10 @@ static int dvb_ca_en50221_check_camstatus(struct 
dvb_ca_private *ca, int slot)
}
 
if (cam_changed) {
-   if (!cam_present_now) {
+   if (!cam_present_now)
sl->camchange_type = DVB_CA_EN50221_CAMCHANGE_REMOVED;
-   } else {
+   else
sl->camchange_type = DVB_CA_EN50221_CAMCHANGE_INSERTED;
-   }
atomic_set(>camchange_count, 1);
} else {
if ((sl->slot_state == DVB_CA_SLOTSTATE_WAITREADY) &&
@@ -314,9 +312,8 @@ static int dvb_ca_en50221_wait_if_status(struct 
dvb_ca_private *ca, int slot,
}
 
/* check for timeout */
-   if (time_after(jiffies, timeout)) {
+   if (time_after(jiffies, timeout))
break;
-   }
 
/* wait for a bit */
usleep_range(1000, 1100);
@@ -782,9 +779,9 @@ static int dvb_ca_en50221_read_data(struct dvb_ca_private 
*ca, int slot,
buf[0], (buf[1] & 0x80) == 0, bytes_read);
 
/* wake up readers when a last_fragment is received */
-   if ((buf[1] & 0x80) == 0x00) {
+   if ((buf[1] & 0x80) == 0x00)
wake_up_interruptible(>wait_queue);
-   }
+
status = bytes_read;
 
 exit:
@@ -1665,11 +1662,10 @@ static ssize_t dvb_ca_en50221_io_read(struct file 
*file, char __user *buf,
connection_id = hdr[0];
if (hdr[0] == connection_id) {
if (pktlen < count) {
-   if ((pktlen + fraglen - 2) > count) {
+   if ((pktlen + fraglen - 2) > count)
fraglen = count - pktlen;
-   } else {
+   else
fraglen -= 2;
-   }
 
status =
   dvb_ringbuffer_pkt_read_user(>rx_buffer,
@@ -1806,9 +1802,8 @@ static unsigned int dvb_ca_en50221_io_poll(struct file 
*file, poll_table *wait)
 
dprintk("%s\n", __func__);
 
-   if (dvb_ca_en50221_io_read_condition(ca, , ) == 1) {
+   if (dvb_ca_en50221_io_read_condition(ca, , ) == 1)
mask |= POLLIN;
-   }
 
/* if there is something, return now */
if (mask)
@@ -1817,9 +1812,8 @@ static unsigned int dvb_ca_en50221_io_poll(struct file 
*file, poll_table *wait)
/* wait for something to happen */
poll_wait(file, >wait_queue, wait);
 
-   if (dvb_ca_en50221_io_read_condition(ca, , ) == 1) {
+   if (dvb_ca_en50221_io_read_condition(ca, , ) == 1)
mask |= POLLIN;
-   }
 
return mask;
 }
@@ -1961,9 +1955,9 @@ void dvb_ca_en50221_release(struct dvb_ca_en50221 *pubca)
/* shutdown the thread if there was one */
kthread_stop(ca->thread);
 
-   for (i = 0; i < ca->slot_count; i++) {
+   for (i = 0; i < ca->slot_count; i++)
dvb_ca_en50221_slot_shutdown(ca, i);
-   }
+
dvb_remove_device(ca->dvbdev);
dvb_ca_private_put(ca);
pubca->private = NULL;
-- 
2.7.4

[PATCH V2 2/9] [media] dvb-core/dvb_ca_en50221.c: New function dvb_ca_en50221_poll_cam_gone

[Jasmin J.]

From: Jasmin Jessich 

The CAM poll code for the budget-av is exactly the same on several
places. Extracting the code to a new function improves maintainability.

Signed-off-by: Jasmin Jessich 
---
 drivers/media/dvb-core/dvb_ca_en50221.c | 63 ++---
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
b/drivers/media/dvb-core/dvb_ca_en50221.c
index 19d0e9a..66a58ed 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -1064,6 +1064,36 @@ static void dvb_ca_en50221_thread_update_delay(struct 
dvb_ca_private *ca)
 }
 
 /**
+ * Poll if the CAM is gone.
+ *
+ * @ca: CA instance.
+ * @slot: Slot to process.
+ * @return: 0 .. no change
+ *  1 .. CAM state changed
+ */
+
+static int dvb_ca_en50221_poll_cam_gone(struct dvb_ca_private *ca, int slot)
+{
+   int changed = 0;
+   int status;
+
+   /* we need this extra check for annoying interfaces like the
+* budget-av
+*/
+   if ((!(ca->flags & DVB_CA_EN50221_FLAG_IRQ_CAMCHANGE))
+   && (ca->pub->poll_slot_status)) {
+   status = ca->pub->poll_slot_status(ca->pub, slot, 0);
+   if (!(status &
+   DVB_CA_EN50221_POLL_CAM_PRESENT)) {
+   ca->slot_info[slot].slot_state = DVB_CA_SLOTSTATE_NONE;
+   dvb_ca_en50221_thread_update_delay(ca);
+   changed = 1;
+   }
+   }
+   return changed;
+}
+
+/**
  * Thread state machine for one CA slot to perform the data transfer.
  *
  * @ca: CA instance.
@@ -1074,7 +1104,6 @@ static void dvb_ca_en50221_thread_state_machine(struct 
dvb_ca_private *ca,
 {
struct dvb_ca_slot *sl = >slot_info[slot];
int flags;
-   int status;
int pktcount;
void *rxbuf;
 
@@ -1123,20 +1152,8 @@ static void dvb_ca_en50221_thread_state_machine(struct 
dvb_ca_private *ca,
 
case DVB_CA_SLOTSTATE_VALIDATE:
if (dvb_ca_en50221_parse_attributes(ca, slot) != 0) {
-   /* we need this extra check for annoying interfaces like
-* the budget-av
-*/
-   if ((!(ca->flags & DVB_CA_EN50221_FLAG_IRQ_CAMCHANGE))
-   && (ca->pub->poll_slot_status)) {
-   status = ca->pub->poll_slot_status(ca->pub,
-  slot, 0);
-   if (!(status &
- DVB_CA_EN50221_POLL_CAM_PRESENT)) {
-   sl->slot_state = DVB_CA_SLOTSTATE_NONE;
-   dvb_ca_en50221_thread_update_delay(ca);
-   break;
-   }
-   }
+   if (dvb_ca_en50221_poll_cam_gone(ca, slot))
+   break;
 
pr_err("dvb_ca adapter %d: Invalid PC card inserted 
:(\n",
   ca->dvbdev->adapter->num);
@@ -1185,20 +1202,8 @@ static void dvb_ca_en50221_thread_state_machine(struct 
dvb_ca_private *ca,
 
case DVB_CA_SLOTSTATE_LINKINIT:
if (dvb_ca_en50221_link_init(ca, slot) != 0) {
-   /* we need this extra check for annoying interfaces like
-* the budget-av
-*/
-   if ((!(ca->flags & DVB_CA_EN50221_FLAG_IRQ_CAMCHANGE))
-   && (ca->pub->poll_slot_status)) {
-   status = ca->pub->poll_slot_status(ca->pub,
-  slot, 0);
-   if (!(status &
-   DVB_CA_EN50221_POLL_CAM_PRESENT)) {
-   sl->slot_state = DVB_CA_SLOTSTATE_NONE;
-   dvb_ca_en50221_thread_update_delay(ca);
-   break;
-   }
-   }
+   if (dvb_ca_en50221_poll_cam_gone(ca, slot))
+   break;
 
pr_err("dvb_ca adapter %d: DVB CAM link initialisation 
failed :(\n",
   ca->dvbdev->adapter->num);
-- 
2.7.4

omap3isp: is capture mode working? what hardware? was Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Pavel Machek]

Hi!

> What I've done is just rebased the ccp2 branch. In other words, the patches
> in that branch are no more ready than they were.
> 
> To get these merged we should ideally
> 
> 1) Make sure there will be no regressions,

I grepped dts trees a bit... where is omap3isp currently used?
Anything besides N9 and N950?

Does the capture mode currently work for you?

Because as far as I can tell, formatter is disabled, so video is in
wrong format for the userspace.

So something like patch below is needed; (of course after adjusting
the comment etc.)

Thanks,
Pavel

commit eb81524b8b44bbff2518b272cb3de304157bd3ba
Author: Pavel 
Date:   Mon Feb 13 21:26:51 2017 +0100

omap3isp: fix VP2SDR bit so capture (not preview) works

This is neccessary for capture (not preview) to work properly on
N900. Why is unknown.

diff --git a/drivers/media/platform/omap3isp/ispccdc.c 
b/drivers/media/platform/omap3isp/ispccdc.c
index 7207558..2fb755f 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1186,7 +1186,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
/* Use the raw, unprocessed data when writing to memory. The H3A and
 * histogram modules are still fed with lens shading corrected data.
 */
-   syn_mode &= ~ISPCCDC_SYN_MODE_VP2SDR;
+// syn_mode &= ~ISPCCDC_SYN_MODE_VP2SDR;
+   syn_mode |= ISPCCDC_SYN_MODE_VP2SDR;
 
if (ccdc->output & CCDC_OUTPUT_MEMORY)
syn_mode |= ISPCCDC_SYN_MODE_WEN;

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v6] media: platform: Renesas IMR driver

[Sergei Shtylyov]

Hello!

On 07/06/2017 09:16 PM, Sergei Shtylyov wrote:

[...]

+=
+
+This file documents some driver-specific aspects of the IMR driver, such as
+driver-specific ioctls.
+
+The ioctl reference
+~~~
+
+VIDIOC_IMR_MESH - Set mapping data
+^^
+
+Argument: struct imr_map_desc
+
+**Description**:
+
+This ioctl sets up the mesh using which the input frames will be


s/using/through/


+transformed into the output frames. The mesh can be strictly rectangular
+(when IMR_MAP_MESH bit is set in imr_map_desc::type) or arbitrary (when
+that bit is not set).
+
+A rectangular mesh consists of the imr_mesh structure followed by M*N
+vertex objects (where M is imr_mesh::rows and N is imr_mesh::columns).
+In case either IMR_MAP_AUTOSG or IMR_MAP_AUTODG bits were set in
+imr_map_desc::type, imr_mesh::{x|y}0 specify the coordinates of the top
+left corner of the auto-generated mesh and imr_mesh::d{x|y} specify the
+mesh's X/Y steps.


What if any of the other types are used like IMR_MAP_LUCE?


   IMR_MAP_LUCE only affects the vertex object.


Is this documented in a Renesas datasheet?


   Yes.


   Well, not exactly. The different mesh types are a software concept, the 
hardware only understands series of triangles.


[...]


Regards,

Hans


MBR, Sergei

[PATCH for 4.13] cec: cec_transmit_attempt_done: ignore CEC_TX_STATUS_MAX_RETRIES

[Hans Verkuil]

The switch in cec_transmit_attempt_done() should ignore the
CEC_TX_STATUS_MAX_RETRIES status bit.

Calling this function with e.g. CEC_TX_STATUS_NACK | CEC_TX_STATUS_MAX_RETRIES
is perfectly legal and should not trigger the WARN(1).

Signed-off-by: Hans Verkuil 
---
After testing the DisplayPort CEC-Tunneling-over-AUX support with an adapter
that didn't hook up the CEC pin correctly I found a bug in this CEC core 
function
that is corrected with this patch.
---
 drivers/media/cec/cec-adap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index bf45977b2823..d596b601ff42 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -559,7 +559,7 @@ EXPORT_SYMBOL_GPL(cec_transmit_done);

 void cec_transmit_attempt_done(struct cec_adapter *adap, u8 status)
 {
-   switch (status) {
+   switch (status & ~CEC_TX_STATUS_MAX_RETRIES) {
case CEC_TX_STATUS_OK:
cec_transmit_done(adap, status, 0, 0, 0, 0);
return;
-- 
2.11.0

Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Sylwester Nawrocki]

Hi Hugues,

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> This patchset enables OV9655 camera support.
> 
> OV9655 support has been tested using STM32F4DIS-CAM extension board
> plugged on connector P1 of STM32F746G-DISCO board.
> Due to lack of OV9650/52 hardware support, the modified related code
> could not have been checked for non-regression.
> 
> First patches upgrade current support of OV9650/52 to prepare then
> introduction of OV9655 variant patch.
> Because of OV9655 register set slightly different from OV9650/9652,
> not all of the driver features are supported (controls). Supported
> resolutions are limited to VGA, QVGA, QQVGA.
> Supported format is limited to RGB565.
> Controls are limited to color bar test pattern for test purpose.

I appreciate your efforts towards making a common driver but IMO it would be 
better to create a separate driver for the OV9655 sensor.  The original driver 
is 1576 lines of code, your patch set adds half of that (816).  There are
significant differences in the feature set of both sensors, there are 
differences in the register layout.  I would go for a separate driver, we  
would then have code easier to follow and wouldn't need to worry about possible
regressions.  I'm afraid I have lost the camera module and won't be able 
to test the patch set against regressions.

IMHO from maintenance POV it's better to make a separate driver. In the end 
of the day we wouldn't be adding much more code than it is being done now.

>   .../devicetree/bindings/media/i2c/ov965x.txt   |  45 ++
>   drivers/media/i2c/Kconfig  |   6 +-
>   drivers/media/i2c/ov9650.c | 816 
> +
>   3 files changed, 736 insertions(+), 131 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt

--
Thanks,
Sylwester

Re: [PATCH 4/4] drm/vc4: add HDMI CEC support

[Hans Verkuil]

On 12/07/17 21:02, Eric Anholt wrote:
> Hans Verkuil  writes:
> 
>> From: Hans Verkuil 
>>
>> This patch adds support to VC4 for CEC.
>>
>> To prevent the firmware from eating the CEC interrupts you need to add this 
>> to
>> your config.txt:
>>
>> mask_gpu_interrupt1=0x100
>>
>> Signed-off-by: Hans Verkuil 
> 
> This looks pretty great.  Just a couple of little comments.
> 
>> ---
>>  drivers/gpu/drm/vc4/Kconfig|   8 ++
>>  drivers/gpu/drm/vc4/vc4_hdmi.c | 203 
>> -
>>  drivers/gpu/drm/vc4/vc4_regs.h |   5 +
>>  3 files changed, 211 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
>> index 4361bdcfd28a..fdae18aeab4f 100644
>> --- a/drivers/gpu/drm/vc4/Kconfig
>> +++ b/drivers/gpu/drm/vc4/Kconfig
>> @@ -19,3 +19,11 @@ config DRM_VC4
>>This driver requires that "avoid_warnings=2" be present in
>>the config.txt for the firmware, to keep it from smashing
>>our display setup.
>> +
>> +config DRM_VC4_HDMI_CEC
>> +   bool "Broadcom VC4 HDMI CEC Support"
>> +   depends on DRM_VC4
>> +   select CEC_CORE
>> +   help
>> +  Choose this option if you have a Broadcom VC4 GPU
>> +  and want to use CEC.
> 
> Do we need a Kconfig for this?  Couldn't we just #ifdef on CEC_CORE
> instead?

It's been my practice to do so for all drivers where I added CEC support.
The main reason is that it is an optional feature of the HDMI protocol, so
you simply may not want to use it to avoid loading the 55+ kB of the cec module.
It will likely grow in size in the future as well.

Also (esp. true for embedded devices) the CEC pin might not even be hooked up!

Finally, you may prefer to use e.g. a Pulse-Eight USB adapter for whatever
reason and then you don't need this either.

> 
>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> index b0521e6cc281..14e2ece5db94 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> 
>> +static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
>> +{
>> +struct vc4_dev *vc4 = cec_get_drvdata(adap);
>> +u32 hsm_clock = clk_get_rate(vc4->hdmi->hsm_clock);
>> +u32 cntrl1 = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
>> +u32 divclk = cntrl1 & VC4_HDMI_CEC_DIV_CLK_CNT_MASK;
> 
> We should probably be setting the divider to a value of our choice,
> rather than relying on whatever default value is there.

Hardcode the divider to 4091, you mean? I can do that.

> 
> (Bonus points if we were to do this using a common clk divider, so the
> rate shows up in /debug/clk/clk_summary, but I won't require that)
> 
>> +/* clock period in microseconds */
>> +u32 usecs = 100 / (hsm_clock / divclk);
>> +u32 val = HDMI_READ(VC4_HDMI_CEC_CNTRL_5);
>> +
>> +val &= ~(VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET |
>> + VC4_HDMI_CEC_CNT_TO_4700_US_MASK |
>> + VC4_HDMI_CEC_CNT_TO_4500_US_MASK);
>> +val |= ((4700 / usecs) << VC4_HDMI_CEC_CNT_TO_4700_US_SHIFT) |
>> +   ((4500 / usecs) << VC4_HDMI_CEC_CNT_TO_4500_US_SHIFT);
>> +
>> +if (enable) {
>> +cntrl1 &= VC4_HDMI_CEC_DIV_CLK_CNT_MASK |
>> +  VC4_HDMI_CEC_ADDR_MASK;
>> +
>> +HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val |
>> +   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
>> +HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val);
>> +HDMI_WRITE(VC4_HDMI_CEC_CNTRL_2,
>> + ((1500 / usecs) << VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT) |
>> + ((1300 / usecs) << VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT) |
>> + ((800 / usecs) << VC4_HDMI_CEC_CNT_TO_800_US_SHIFT) |
>> + ((600 / usecs) << VC4_HDMI_CEC_CNT_TO_600_US_SHIFT) |
>> + ((400 / usecs) << VC4_HDMI_CEC_CNT_TO_400_US_SHIFT));
>> +HDMI_WRITE(VC4_HDMI_CEC_CNTRL_3,
>> + ((2750 / usecs) << VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT) |
>> + ((2400 / usecs) << VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT) |
>> + ((2050 / usecs) << VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT) |
>> + ((1700 / usecs) << VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT));
>> +HDMI_WRITE(VC4_HDMI_CEC_CNTRL_4,
>> + ((4300 / usecs) << VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT) |
>> + ((3900 / usecs) << VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT) |
>> + ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) |
>> + ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT));
>> +
>> +HDMI_WRITE(VC4_HDMI_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
>> +} else {
>> +HDMI_WRITE(VC4_HDMI_CPU_MASK_SET, VC4_HDMI_CPU_CEC);
>> +HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val |
>> +   

Re: [PATCH v2 3/7] [media] ov9650: add device tree support

[Sylwester Nawrocki]

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> Allows use of device tree configuration data.
> If no device tree data is there, configuration is taken from platform data.
> In order to keep GPIOs configuration compatible between both way of doing,
> GPIOs are switched to descriptor-based interface.
> 
> Signed-off-by: H. Nikolaus Schaller 
> Signed-off-by: Hugues Fruchet 
> ---
>   drivers/media/i2c/Kconfig  |  2 +-
>   drivers/media/i2c/ov9650.c | 77 
> ++
>   2 files changed, 59 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 121b3b5..168115c 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -615,7 +615,7 @@ config VIDEO_OV7670
>   
>   config VIDEO_OV9650
>   tristate "OmniVision OV9650/OV9652 sensor support"
> - depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>   ---help---
> This is a V4L2 sensor-level driver for the Omnivision
> OV9650 and OV9652 camera sensors.
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index 1e4e99e..7e9a902 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -11,12 +11,14 @@
>* it under the terms of the GNU General Public License version 2 as
>* published by the Free Software Foundation.
>*/
> +#include 
>   #include 
>   #include 
>   #include 
>   #include 
>   #include 
>   #include 
> +#include 
>   #include 
>   #include 
>   #include 
> @@ -249,9 +251,10 @@ struct ov965x {
>   struct v4l2_subdev sd;
>   struct media_pad pad;
>   enum v4l2_mbus_type bus_type;
> - int gpios[NUM_GPIOS];
> + struct gpio_desc *gpios[NUM_GPIOS];
>   /* External master clock frequency */
>   unsigned long mclk_frequency;
> + struct clk *clk;
>   
>   /* Protects the struct fields below */
>   struct mutex lock;
> @@ -511,10 +514,10 @@ static int ov965x_set_color_matrix(struct ov965x 
> *ov965x)
>   return 0;
>   }
>   
> -static void ov965x_gpio_set(int gpio, int val)
> +static void ov965x_gpio_set(struct gpio_desc *gpio, int val)
>   {
> - if (gpio_is_valid(gpio))
> - gpio_set_value(gpio, val);
> + if (gpio)
> + gpiod_set_value_cansleep(gpio, val);
>   }
>   
>   static void __ov965x_set_power(struct ov965x *ov965x, int on)
> @@ -1406,24 +1409,28 @@ static int ov965x_configure_gpios(struct ov965x 
> *ov965x,
> const struct ov9650_platform_data *pdata)
>   {
>   int ret, i;
> + int gpios[NUM_GPIOS];
>   
> - ov965x->gpios[GPIO_PWDN] = pdata->gpio_pwdn;
> - ov965x->gpios[GPIO_RST]  = pdata->gpio_reset;
> + gpios[GPIO_PWDN] = pdata->gpio_pwdn;
> + gpios[GPIO_RST]  = pdata->gpio_reset;
>   
> - for (i = 0; i < ARRAY_SIZE(ov965x->gpios); i++) {
> - int gpio = ov965x->gpios[i];
> + for (i = 0; i < ARRAY_SIZE(gpios); i++) {
> + int gpio = gpios[i];
>   
>   if (!gpio_is_valid(gpio))
>   continue;
>   ret = devm_gpio_request_one(>client->dev, gpio,
> - GPIOF_OUT_INIT_HIGH, "OV965X");
> - if (ret < 0)
> + GPIOF_OUT_INIT_HIGH, DRIVER_NAME);
> + if (ret < 0) {
> + dev_err(>client->dev,
> + "Failed to request gpio%d (%d)\n", gpio, ret);
>   return ret;
> + }
>   v4l2_dbg(1, debug, >sd, "set gpio %d to 1\n", gpio);
>   
>   gpio_set_value(gpio, 1);
>   gpio_export(gpio, 0);
> - ov965x->gpios[i] = gpio;
> + ov965x->gpios[i] = gpio_to_desc(gpio);
>   }
>   
>   return 0;
> @@ -1469,14 +1476,10 @@ static int ov965x_probe(struct i2c_client *client,
>   struct v4l2_subdev *sd;
>   struct ov965x *ov965x;
>   int ret;
> + struct device_node *np = client->dev.of_node;
>   
> - if (pdata == NULL) {
> - dev_err(>dev, "platform data not specified\n");
> - return -EINVAL;
> - }
> -
> - if (pdata->mclk_frequency == 0) {
> - dev_err(>dev, "MCLK frequency not specified\n");
> + if (!pdata && !np) {
> + dev_err(>dev, "Platform data or device tree data must 
> be provided\n");
>   return -EINVAL;
>   }
>   
> @@ -1486,7 +1489,35 @@ static int ov965x_probe(struct i2c_client *client,
>   
>   mutex_init(>lock);
>   ov965x->client = client;
> - ov965x->mclk_frequency = pdata->mclk_frequency;
> + mutex_init(>lock);

Are you initializing the mutex twice?

> + if (np) {
> + /* Device tree */
> + ov965x->gpios[GPIO_RST] =
> + devm_gpiod_get_optional(>dev, "resetb",
> +

Re: [PATCH 2/4] drm/vc4: prepare for CEC support

[Hans Verkuil]

On 12/07/17 20:42, Eric Anholt wrote:
> Hans Verkuil  writes:
> 
>> From: Hans Verkuil 
>>
>> In order to support CEC the hsm clock needs to be enabled in
>> vc4_hdmi_bind(), not in vc4_hdmi_encoder_enable(). Otherwise you wouldn't
>> be able to support CEC when there is no hotplug detect signal, which is
>> required by some monitors that turn off the HPD when in standby, but keep
>> the CEC bus alive so they can be woken up.
>>
>> The HDMI core also has to be enabled in vc4_hdmi_bind() for the same
>> reason.
>>
>> Signed-off-by: Hans Verkuil 
> 
> Ccing Boris, I'd love to see what he thinks of this and if we can do any
> better.
> 
> Hans, is it true that CEC needs to be on always, or could it only be
> enabled when somebody is listening to messages?

It depends. If a valid physical address is read from the EDID (i.e. we are
connected to a display) then CEC has to be on always otherwise you can't receive
messages that the display (or others) send to you. Note that even if there is
no application listening to messages, the CEC framework will still listen to
and process CEC core messages and remote control passthrough messages.

If there is no physical address, for example because the hotplug detect is low,
then that is a special case: some displays will turn off the HPD when in 
standby,
but CEC still works. Devices can send an Image View On CEC message to wake up 
such
displays. This is a corner case explicitly allowed by the CEC spec. In my view 
this
is a hack in the specification just to work around broken displays. But such
displays really exist, unfortunately.

So in that case CEC has to be enabled only when we transmit a message.

This is what the CEC framework does: the adap_enabled callback is called with
true when a physical address is set and with false when the PA goes away. If you
transmit a message while there is no PA then adap_enabled is called with true
right before the transmit and with false when the transmit finished.

Regards,

Hans

> 
>>  drivers/gpu/drm/vc4/vc4_hdmi.c | 59 
>> +-
>>  1 file changed, 29 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> index ed63d4e85762..e0104f96011e 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> @@ -463,11 +463,6 @@ static void vc4_hdmi_encoder_disable(struct drm_encoder 
>> *encoder)
>>  HD_WRITE(VC4_HD_VID_CTL,
>>   HD_READ(VC4_HD_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE);
>>  
>> -HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
>> -udelay(1);
>> -HD_WRITE(VC4_HD_M_CTL, 0);
>> -
>> -clk_disable_unprepare(hdmi->hsm_clock);
>>  clk_disable_unprepare(hdmi->pixel_clock);
>>  
>>  ret = pm_runtime_put(>pdev->dev);
>> @@ -509,16 +504,6 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder 
>> *encoder)
>>  return;
>>  }
>>  
>> -/* This is the rate that is set by the firmware.  The number
>> - * needs to be a bit higher than the pixel clock rate
>> - * (generally 148.5Mhz).
>> - */
>> -ret = clk_set_rate(hdmi->hsm_clock, 163682864);
>> -if (ret) {
>> -DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
>> -return;
>> -}
>> -
>>  ret = clk_set_rate(hdmi->pixel_clock,
>> mode->clock * 1000 *
>> ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1));
>> @@ -533,20 +518,6 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder 
>> *encoder)
>>  return;
>>  }
>>  
>> -ret = clk_prepare_enable(hdmi->hsm_clock);
>> -if (ret) {
>> -DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n",
>> -  ret);
>> -clk_disable_unprepare(hdmi->pixel_clock);
>> -return;
>> -}
>> -
>> -HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
>> -udelay(1);
>> -HD_WRITE(VC4_HD_M_CTL, 0);
>> -
>> -HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_ENABLE);
>> -
>>  HDMI_WRITE(VC4_HDMI_SW_RESET_CONTROL,
>> VC4_HDMI_SW_RESET_HDMI |
>> VC4_HDMI_SW_RESET_FORMAT_DETECT);
>> @@ -1205,6 +1176,23 @@ static int vc4_hdmi_bind(struct device *dev, struct 
>> device *master, void *data)
>>  return -EPROBE_DEFER;
>>  }
>>  
>> +/* This is the rate that is set by the firmware.  The number
>> + * needs to be a bit higher than the pixel clock rate
>> + * (generally 148.5Mhz).
>> + */
>> +ret = clk_set_rate(hdmi->hsm_clock, 163682864);
>> +if (ret) {
>> +DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
>> +goto err_put_i2c;
>> +}
>> +
>> +ret = clk_prepare_enable(hdmi->hsm_clock);
>> +if (ret) {
>> +DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n",
>> +  ret);
>> +goto err_put_i2c;
>> +   

Re: [PATCH v2 2/7] [media] ov9650: switch i2c device id to lower case

[Sylwester Nawrocki]

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> Switch i2c device id to lower case as it is

s/i2c/I2C ?

> done for other omnivision cameras.

s/omnivision/Omnivision

This is required for properly matching driver with device on DT platforms,
right? It might be worth to mention that so it is clear why we break any
non-dt platform that could be already using this driver. There seem to be 
none in the mainline kernel tree though.

> Signed-off-by: Hugues Fruchet 

Reviewed-by: Sylwester Nawrocki 

> Signed-off-by: Hugues Fruchet 
> ---
>   drivers/media/i2c/ov9650.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index 2de2fbb..1e4e99e 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -1545,8 +1545,8 @@ static int ov965x_remove(struct i2c_client *client)
>   }
>   
>   static const struct i2c_device_id ov965x_id[] = {
> - { "OV9650", 0 },
> - { "OV9652", 0 },
> + { "ov9650", 0 },
> + { "ov9652", 0 },
>   { /* sentinel */ }
>   };
>   MODULE_DEVICE_TABLE(i2c, ov965x_id);
 

[PATCH v2] [media] lirc_zilog: Clean up lirc zilog error codes

[Yves Lemée]

According the coding style guidelines, the ENOSYS error code must be returned
in case of a non existent system call. This code has been replaced with
the ENOTTY error code indicating a missing functionality.

v2: Improved punctuation
Fixed patch subject

Signed-off-by: Yves Lemée 
---
 drivers/staging/media/lirc/lirc_zilog.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index 015e41bd036e..26dd32d5b5b2 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1249,7 +1249,7 @@ static long ioctl(struct file *filep, unsigned int cmd, 
unsigned long arg)
break;
case LIRC_GET_REC_MODE:
if (!(features & LIRC_CAN_REC_MASK))
-   return -ENOSYS;
+   return -ENOTTY;
 
result = put_user(LIRC_REC2MODE
  (features & LIRC_CAN_REC_MASK),
@@ -1257,21 +1257,21 @@ static long ioctl(struct file *filep, unsigned int cmd, 
unsigned long arg)
break;
case LIRC_SET_REC_MODE:
if (!(features & LIRC_CAN_REC_MASK))
-   return -ENOSYS;
+   return -ENOTTY;
 
result = get_user(mode, uptr);
if (!result && !(LIRC_MODE2REC(mode) & features))
-   result = -EINVAL;
+   result = -ENOTTY;
break;
case LIRC_GET_SEND_MODE:
if (!(features & LIRC_CAN_SEND_MASK))
-   return -ENOSYS;
+   return -ENOTTY;
 
result = put_user(LIRC_MODE_PULSE, uptr);
break;
case LIRC_SET_SEND_MODE:
if (!(features & LIRC_CAN_SEND_MASK))
-   return -ENOSYS;
+   return -ENOTTY;
 
result = get_user(mode, uptr);
if (!result && mode != LIRC_MODE_PULSE)
-- 
2.13.2

Re: [PATCH 4/4] drm/vc4: add HDMI CEC support

[Eric Anholt]

Hans Verkuil  writes:

> From: Hans Verkuil 
>
> This patch adds support to VC4 for CEC.
>
> To prevent the firmware from eating the CEC interrupts you need to add this to
> your config.txt:
>
> mask_gpu_interrupt1=0x100
>
> Signed-off-by: Hans Verkuil 

This looks pretty great.  Just a couple of little comments.

> ---
>  drivers/gpu/drm/vc4/Kconfig|   8 ++
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 203 
> -
>  drivers/gpu/drm/vc4/vc4_regs.h |   5 +
>  3 files changed, 211 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
> index 4361bdcfd28a..fdae18aeab4f 100644
> --- a/drivers/gpu/drm/vc4/Kconfig
> +++ b/drivers/gpu/drm/vc4/Kconfig
> @@ -19,3 +19,11 @@ config DRM_VC4
> This driver requires that "avoid_warnings=2" be present in
> the config.txt for the firmware, to keep it from smashing
> our display setup.
> +
> +config DRM_VC4_HDMI_CEC
> +   bool "Broadcom VC4 HDMI CEC Support"
> +   depends on DRM_VC4
> +   select CEC_CORE
> +   help
> +   Choose this option if you have a Broadcom VC4 GPU
> +   and want to use CEC.

Do we need a Kconfig for this?  Couldn't we just #ifdef on CEC_CORE
instead?

> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index b0521e6cc281..14e2ece5db94 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c

> +static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> + struct vc4_dev *vc4 = cec_get_drvdata(adap);
> + u32 hsm_clock = clk_get_rate(vc4->hdmi->hsm_clock);
> + u32 cntrl1 = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
> + u32 divclk = cntrl1 & VC4_HDMI_CEC_DIV_CLK_CNT_MASK;

We should probably be setting the divider to a value of our choice,
rather than relying on whatever default value is there.

(Bonus points if we were to do this using a common clk divider, so the
rate shows up in /debug/clk/clk_summary, but I won't require that)

> + /* clock period in microseconds */
> + u32 usecs = 100 / (hsm_clock / divclk);
> + u32 val = HDMI_READ(VC4_HDMI_CEC_CNTRL_5);
> +
> + val &= ~(VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET |
> +  VC4_HDMI_CEC_CNT_TO_4700_US_MASK |
> +  VC4_HDMI_CEC_CNT_TO_4500_US_MASK);
> + val |= ((4700 / usecs) << VC4_HDMI_CEC_CNT_TO_4700_US_SHIFT) |
> +((4500 / usecs) << VC4_HDMI_CEC_CNT_TO_4500_US_SHIFT);
> +
> + if (enable) {
> + cntrl1 &= VC4_HDMI_CEC_DIV_CLK_CNT_MASK |
> +   VC4_HDMI_CEC_ADDR_MASK;
> +
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val |
> +VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val);
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_2,
> +  ((1500 / usecs) << VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT) |
> +  ((1300 / usecs) << VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT) |
> +  ((800 / usecs) << VC4_HDMI_CEC_CNT_TO_800_US_SHIFT) |
> +  ((600 / usecs) << VC4_HDMI_CEC_CNT_TO_600_US_SHIFT) |
> +  ((400 / usecs) << VC4_HDMI_CEC_CNT_TO_400_US_SHIFT));
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_3,
> +  ((2750 / usecs) << VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT) |
> +  ((2400 / usecs) << VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT) |
> +  ((2050 / usecs) << VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT) |
> +  ((1700 / usecs) << VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT));
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_4,
> +  ((4300 / usecs) << VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT) |
> +  ((3900 / usecs) << VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT) |
> +  ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) |
> +  ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT));
> +
> + HDMI_WRITE(VC4_HDMI_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
> + } else {
> + HDMI_WRITE(VC4_HDMI_CPU_MASK_SET, VC4_HDMI_CPU_CEC);
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val |
> +VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
> + }
> + return 0;
> +}

> +static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> +   u32 signal_free_time, struct cec_msg *msg)
> +{
> + struct vc4_dev *vc4 = cec_get_drvdata(adap);
> + u32 val;
> + unsigned int i;
> +
> + for (i = 0; i < msg->len; i += 4)
> + HDMI_WRITE(VC4_HDMI_CEC_TX_DATA_1 + i,
> +(msg->msg[i]) |
> +(msg->msg[i + 1] << 8) |
> +(msg->msg[i + 2] << 16) |
> +(msg->msg[i + 3] << 24));
> +
> + val = 

RE: [v2,07/12] intel-ipu3: css: firmware management

[Zhi, Yong]

Thanks Kaehlcke for reviewing the code.

> -Original Message-
> From: Matthias Kaehlcke [mailto:m...@chromium.org]
> Sent: Wednesday, July 12, 2017 11:33 AM
> To: Zhi, Yong 
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian
> Xu ; tf...@chromium.org; Mani, Rajmohan
> ; Toivonen, Tuukka
> 
> Subject: Re: [v2,07/12] intel-ipu3: css: firmware management
> 
> El Wed, Jun 14, 2017 at 05:19:22PM -0500 Yong Zhi ha dit:
> 
> > Functions to load and install imgu FW blobs
> >
> > Signed-off-by: Yong Zhi 
> > ---
> >  drivers/media/pci/intel/ipu3/ipu3-abi.h| 1573
> 
> >  drivers/media/pci/intel/ipu3/ipu3-css-fw.c |  272 +
> >  drivers/media/pci/intel/ipu3/ipu3-css-fw.h |  219 
> >  drivers/media/pci/intel/ipu3/ipu3-css.h|   54 +
> >  4 files changed, 2118 insertions(+)
> >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-abi.h
> >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.c
> >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.h
> >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css.h
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-abi.h
> b/drivers/media/pci/intel/ipu3/ipu3-abi.h
> > new file mode 100644
> > index 000..5107b35
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-abi.h
> > @@ -0,0 +1,1573 @@
> > +/*
> > + * Copyright (c) 2017 Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#ifndef __IPU3_ABI_H
> > +#define __IPU3_ABI_H
> > +
> > +#include 
> > +
> > +/*** IMGU Hardware information
> ***/
> > +
> > +#define IMGU_ISP_VMEM_ALIGN128
> > +#define IMGU_DVS_BLOCK_W   64
> > +#define IMGU_DVS_BLOCK_H   32
> > +#define IMGU_GDC_BUF_X (2 *
> IMGU_DVS_BLOCK_W)
> > +#define IMGU_GDC_BUF_Y IMGU_DVS_BLOCK_H
> > +/* n = 0..1 */
> > +#define IMGU_SP_PMEM_BASE(n)   (0x2 + (n) *
> 0x4000)
> > +#define IMGU_MAX_BQ_GRID_WIDTH 80
> > +#define IMGU_MAX_BQ_GRID_HEIGHT60
> > +#define IMGU_OBGRID_TILE_SIZE  16
> > +#define IMGU_PIXELS_PER_WORD   50
> > +#define IMGU_BYTES_PER_WORD64
> > +#define IMGU_STRIPE_FIXED_HALF_OVERLAP 2
> > +#define IMGU_SHD_SETS  3
> > +#define IMGU_BDS_MIN_CLIP_VAL  0
> > +#define IMGU_BDS_MAX_CLIP_VAL  2
> > +#define IPU3_ABI_AWB_MAX_CELLS_PER_SET 160
> > +#define IPU3_ABI_AF_MAX_CELLS_PER_SET  32
> > +#define IPU3_ABI_AWB_FR_MAX_CELLS_PER_SET  32
> > +
> > +#define IMGU_ABI_ACC_OP_IDLE   0
> > +#define IMGU_ABI_ACC_OP_END_OF_ACK 1
> > +#define IMGU_ABI_ACC_OP_END_OF_OPS 2
> > +#define IMGU_ABI_ACC_OP_NO_OPS 3
> > +
> > +#define IMGU_ABI_ACC_OPTYPE_PROCESS_LINES  0
> > +#define IMGU_ABI_ACC_OPTYPE_TRANSFER_DATA  1
> > +
> > +#define IMGU_MMU_PADDR_SHIFT   12
> > +
> > +/* Register definitions */
> > +
> > +/* PM_CTRL_0_5_0_IMGHMMADR */
> > +#define IMGU_REG_PM_CTRL   0x0
> > +#define IMGU_PM_CTRL_START BIT(0)
> > +#define IMGU_PM_CTRL_CFG_DONE  BIT(1)
> > +#define IMGU_PM_CTRL_RACE_TO_HALT  BIT(2)
> > +#define IMGU_PM_CTRL_NACK_ALL  BIT(3)
> > +#define IMGU_PM_CTRL_CSS_PWRDN BIT(4)
> > +#define IMGU_PM_CTRL_RST_AT_EOFBIT(5)
> > +#define IMGU_PM_CTRL_FORCE_HALTBIT(6)
> > +#define IMGU_PM_CTRL_FORCE_UNHALT  BIT(7)
> > +#define IMGU_PM_CTRL_FORCE_PWRDN   BIT(8)
> > +#define IMGU_PM_CTRL_FORCE_RESET   BIT(9)
> > +#define IMGU_PM_CTRL_RETURN_LICENSE_AT_EOF BIT(10)
> > +#define IMGU_PM_CTRL_POWER_DOWN_AT_EOF
>   (IMGU_PM_CTRL_CSS_PWRDN | \
> > +   IMGU_PM_CTRL_RACE_TO_HALT | \
> > +
>   IMGU_PM_CTRL_RETURN_LICENSE_AT_EOF)
> > +#define IMGU_PM_CTRL_RESET_AT_EOF
>   (IMGU_PM_CTRL_RST_AT_EOF | \
> > +   IMGU_PM_CTRL_RACE_TO_HALT | \
> > +
>   IMGU_PM_CTRL_RETURN_LICENSE_AT_EOF)
> > +/* SYSTEM_REQ_0_5_0_IMGHMMADR */
> > +#define IMGU_REG_SYSTEM_REQ0x18
> > +#define 

Re: [PATCH 2/4] drm/vc4: prepare for CEC support

[Eric Anholt]

Hans Verkuil  writes:

> From: Hans Verkuil 
>
> In order to support CEC the hsm clock needs to be enabled in
> vc4_hdmi_bind(), not in vc4_hdmi_encoder_enable(). Otherwise you wouldn't
> be able to support CEC when there is no hotplug detect signal, which is
> required by some monitors that turn off the HPD when in standby, but keep
> the CEC bus alive so they can be woken up.
>
> The HDMI core also has to be enabled in vc4_hdmi_bind() for the same
> reason.
>
> Signed-off-by: Hans Verkuil 

Ccing Boris, I'd love to see what he thinks of this and if we can do any
better.

Hans, is it true that CEC needs to be on always, or could it only be
enabled when somebody is listening to messages?

>  drivers/gpu/drm/vc4/vc4_hdmi.c | 59 
> +-
>  1 file changed, 29 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index ed63d4e85762..e0104f96011e 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -463,11 +463,6 @@ static void vc4_hdmi_encoder_disable(struct drm_encoder 
> *encoder)
>   HD_WRITE(VC4_HD_VID_CTL,
>HD_READ(VC4_HD_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE);
>  
> - HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
> - udelay(1);
> - HD_WRITE(VC4_HD_M_CTL, 0);
> -
> - clk_disable_unprepare(hdmi->hsm_clock);
>   clk_disable_unprepare(hdmi->pixel_clock);
>  
>   ret = pm_runtime_put(>pdev->dev);
> @@ -509,16 +504,6 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder 
> *encoder)
>   return;
>   }
>  
> - /* This is the rate that is set by the firmware.  The number
> -  * needs to be a bit higher than the pixel clock rate
> -  * (generally 148.5Mhz).
> -  */
> - ret = clk_set_rate(hdmi->hsm_clock, 163682864);
> - if (ret) {
> - DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
> - return;
> - }
> -
>   ret = clk_set_rate(hdmi->pixel_clock,
>  mode->clock * 1000 *
>  ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1));
> @@ -533,20 +518,6 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder 
> *encoder)
>   return;
>   }
>  
> - ret = clk_prepare_enable(hdmi->hsm_clock);
> - if (ret) {
> - DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n",
> -   ret);
> - clk_disable_unprepare(hdmi->pixel_clock);
> - return;
> - }
> -
> - HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
> - udelay(1);
> - HD_WRITE(VC4_HD_M_CTL, 0);
> -
> - HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_ENABLE);
> -
>   HDMI_WRITE(VC4_HDMI_SW_RESET_CONTROL,
>  VC4_HDMI_SW_RESET_HDMI |
>  VC4_HDMI_SW_RESET_FORMAT_DETECT);
> @@ -1205,6 +1176,23 @@ static int vc4_hdmi_bind(struct device *dev, struct 
> device *master, void *data)
>   return -EPROBE_DEFER;
>   }
>  
> + /* This is the rate that is set by the firmware.  The number
> +  * needs to be a bit higher than the pixel clock rate
> +  * (generally 148.5Mhz).
> +  */
> + ret = clk_set_rate(hdmi->hsm_clock, 163682864);
> + if (ret) {
> + DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
> + goto err_put_i2c;
> + }
> +
> + ret = clk_prepare_enable(hdmi->hsm_clock);
> + if (ret) {
> + DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n",
> +   ret);
> + goto err_put_i2c;
> + }
> +
>   /* Only use the GPIO HPD pin if present in the DT, otherwise
>* we'll use the HDMI core's register.
>*/
> @@ -1216,7 +1204,7 @@ static int vc4_hdmi_bind(struct device *dev, struct 
> device *master, void *data)
>_gpio_flags);
>   if (hdmi->hpd_gpio < 0) {
>   ret = hdmi->hpd_gpio;
> - goto err_put_i2c;
> + goto err_unprepare_hsm;
>   }
>  
>   hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW;
> @@ -1224,6 +1212,14 @@ static int vc4_hdmi_bind(struct device *dev, struct 
> device *master, void *data)
>  
>   vc4->hdmi = hdmi;
>  
> + /* HDMI core must be enabled. */
> + if (!(HD_READ(VC4_HD_M_CTL) & VC4_HD_M_ENABLE)) {
> + HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
> + udelay(1);
> + HD_WRITE(VC4_HD_M_CTL, 0);
> +
> + HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_ENABLE);
> + }

I'm wondering if there's any impact from leaving VC4_HD_M_ENABLE on
while the HDMI power domain is off.  I don't quite understand the role
of the power domain, and I've fired off an email internally to check if
there are any experts on this hardware still around.


signature.asc
Description: PGP 

Re: [v2,07/12] intel-ipu3: css: firmware management

[Matthias Kaehlcke]

El Wed, Jun 14, 2017 at 05:19:22PM -0500 Yong Zhi ha dit:

> Functions to load and install imgu FW blobs
> 
> Signed-off-by: Yong Zhi 
> ---
>  drivers/media/pci/intel/ipu3/ipu3-abi.h| 1573 
> 
>  drivers/media/pci/intel/ipu3/ipu3-css-fw.c |  272 +
>  drivers/media/pci/intel/ipu3/ipu3-css-fw.h |  219 
>  drivers/media/pci/intel/ipu3/ipu3-css.h|   54 +
>  4 files changed, 2118 insertions(+)
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-abi.h
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.c
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.h
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css.h
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-abi.h 
> b/drivers/media/pci/intel/ipu3/ipu3-abi.h
> new file mode 100644
> index 000..5107b35
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-abi.h
> @@ -0,0 +1,1573 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __IPU3_ABI_H
> +#define __IPU3_ABI_H
> +
> +#include 
> +
> +/*** IMGU Hardware information ***/
> +
> +#define IMGU_ISP_VMEM_ALIGN  128
> +#define IMGU_DVS_BLOCK_W 64
> +#define IMGU_DVS_BLOCK_H 32
> +#define IMGU_GDC_BUF_X   (2 * IMGU_DVS_BLOCK_W)
> +#define IMGU_GDC_BUF_Y   IMGU_DVS_BLOCK_H
> +/* n = 0..1 */
> +#define IMGU_SP_PMEM_BASE(n) (0x2 + (n) * 0x4000)
> +#define IMGU_MAX_BQ_GRID_WIDTH   80
> +#define IMGU_MAX_BQ_GRID_HEIGHT  60
> +#define IMGU_OBGRID_TILE_SIZE16
> +#define IMGU_PIXELS_PER_WORD 50
> +#define IMGU_BYTES_PER_WORD  64
> +#define IMGU_STRIPE_FIXED_HALF_OVERLAP   2
> +#define IMGU_SHD_SETS3
> +#define IMGU_BDS_MIN_CLIP_VAL0
> +#define IMGU_BDS_MAX_CLIP_VAL2
> +#define IPU3_ABI_AWB_MAX_CELLS_PER_SET   160
> +#define IPU3_ABI_AF_MAX_CELLS_PER_SET32
> +#define IPU3_ABI_AWB_FR_MAX_CELLS_PER_SET32
> +
> +#define IMGU_ABI_ACC_OP_IDLE 0
> +#define IMGU_ABI_ACC_OP_END_OF_ACK   1
> +#define IMGU_ABI_ACC_OP_END_OF_OPS   2
> +#define IMGU_ABI_ACC_OP_NO_OPS   3
> +
> +#define IMGU_ABI_ACC_OPTYPE_PROCESS_LINES0
> +#define IMGU_ABI_ACC_OPTYPE_TRANSFER_DATA1
> +
> +#define IMGU_MMU_PADDR_SHIFT 12
> +
> +/* Register definitions */
> +
> +/* PM_CTRL_0_5_0_IMGHMMADR */
> +#define IMGU_REG_PM_CTRL 0x0
> +#define IMGU_PM_CTRL_START   BIT(0)
> +#define IMGU_PM_CTRL_CFG_DONEBIT(1)
> +#define IMGU_PM_CTRL_RACE_TO_HALTBIT(2)
> +#define IMGU_PM_CTRL_NACK_ALLBIT(3)
> +#define IMGU_PM_CTRL_CSS_PWRDN   BIT(4)
> +#define IMGU_PM_CTRL_RST_AT_EOF  BIT(5)
> +#define IMGU_PM_CTRL_FORCE_HALT  BIT(6)
> +#define IMGU_PM_CTRL_FORCE_UNHALTBIT(7)
> +#define IMGU_PM_CTRL_FORCE_PWRDN BIT(8)
> +#define IMGU_PM_CTRL_FORCE_RESET BIT(9)
> +#define IMGU_PM_CTRL_RETURN_LICENSE_AT_EOF   BIT(10)
> +#define IMGU_PM_CTRL_POWER_DOWN_AT_EOF   (IMGU_PM_CTRL_CSS_PWRDN 
> | \
> + IMGU_PM_CTRL_RACE_TO_HALT | \
> + IMGU_PM_CTRL_RETURN_LICENSE_AT_EOF)
> +#define IMGU_PM_CTRL_RESET_AT_EOF(IMGU_PM_CTRL_RST_AT_EOF | \
> + IMGU_PM_CTRL_RACE_TO_HALT | \
> + IMGU_PM_CTRL_RETURN_LICENSE_AT_EOF)
> +/* SYSTEM_REQ_0_5_0_IMGHMMADR */
> +#define IMGU_REG_SYSTEM_REQ  0x18
> +#define IMGU_SYSTEM_REQ_FREQ_MASK0x3f
> +#define IMGU_SYSTEM_REQ_FREQ_DIVIDER 25
> +#define IMGU_REG_INT_STATUS  0x30
> +#define IMGU_REG_INT_ENABLE  0x34
> +#define IMGU_REG_INT_CSS_IRQ (1 << 31)
> +/* STATE_0_5_0_IMGHMMADR */
> +#define IMGU_REG_STATE   0x130
> +#define IMGU_STATE_HALT_STS  BIT(0)
> +#define IMGU_STATE_IDLE_STS  BIT(1)
> +#define IMGU_STATE_POWER_UP  BIT(2)
> +#define IMGU_STATE_POWER_DOWNBIT(3)
> +#define 

Re: [PATCH 3/4] drm/vc4: Add register defines for CEC.

[Eric Anholt]

Hans Verkuil  writes:

> From: Eric Anholt 
>
> Basic usage:
>
> poweron: HSM clock should be running.  Set the bit clock divider,
> set all the other _US timeouts based on bit clock rate.  Bring RX/TX
> reset up and then down.
>
> powerdown: Set RX/TX reset.
>
> interrupt: read CPU_STATUS, write bits that have been handled to
> CPU_CLEAR.
>
> Bits are added to /debug/dri/0/hdmi_regs so you can check out the
> power-on values.

Let's drop my hints to you from the commit message here :)


signature.asc
Description: PGP signature

Re: [PATCH v2] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage

[Steve Longerbeam]

On 07/12/2017 08:50 AM, Philipp Zabel wrote:

On Tue, 2017-07-11 at 15:18 +0200, Arnd Bergmann wrote:

While looking at a compiler warning, I noticed the use of
IS_ERR_OR_NULL, which is generally a sign of a bad API design
and should be avoided.

In this driver, this is fairly easy, we can simply stop storing
error pointers in persistent structures, and change the two
functions that might return either a NULL pointer or an error
code to consistently return error pointers when failing.

of_parse_subdev() now separates the error code and the pointer
it looks up, to clarify the interface. There are two cases
where this function originally returns 'NULL', and I have
changed that to '0' for success to keep the current behavior,
though returning an error would also make sense there.

Fixes: e130291212df ("[media] media: Add i.MX media core driver")
Signed-off-by: Arnd Bergmann 
---
v2: fix type mismatch
v3: rework of_parse_subdev() as well.


Thanks!

Reviewed-by: Philipp Zabel 
Tested-by: Philipp Zabel 



Looks fine to me. Tested on SabreAuto with affected pipelines.

Reviewed-by: Steve Longerbeam 
Tested-by: Steve Longerbeam 

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Pavel Machek]

Hi!


> > > 1) Make sure there will be no regressions,
> > 
> > Well, all I have running recent kernels is N900. If ccp branch works
> > for you on N9, that's probably as much testing as we can get.
> > 
> > > 2) clean things up in the omap3isp; which resources are needed and when
> > > (e.g. regulators, PHY configuration) isn't clear at the moment and
> > > 
> > > 2) have one driver using the implementation.
> > > 
> > > At least 1) is needed. I think a number of framework patches could be
> > > mergeable before 2) and 3) are done. I can prepare a set later this week.
> > > But even that'd be likely for 4.14, not 4.13.
> > 
> > Yep, it is too late for v4.13 now. But getting stuff ready for v4.14
> > would be good.
...
> > @@ -302,13 +303,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> > if (rval < 0)
> > goto done;
> >  
> > -   rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> > -   if (rval) {
> > -   regulator_disable(phy->vdd);
> > -   goto done;
> > +   if (phy->isp->revision == ISP_REVISION_15_0) {
> 
> Shouldn't you make the related changes to omap3isp_csiphy_release() as
> well?
> 
> Other than that the patch looks good to me.

Ah, yes, that needs to be fixed. Thanks for review.

I'll refresh the series. I believe we now have everything neccessary
to have useful driver for 4.14. Series is still based on 4.12-rc3, I
can rebase it when there's better base.

Best regards,

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v2] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage

[Philipp Zabel]

On Tue, 2017-07-11 at 15:18 +0200, Arnd Bergmann wrote:
> While looking at a compiler warning, I noticed the use of
> IS_ERR_OR_NULL, which is generally a sign of a bad API design
> and should be avoided.
> 
> In this driver, this is fairly easy, we can simply stop storing
> error pointers in persistent structures, and change the two
> functions that might return either a NULL pointer or an error
> code to consistently return error pointers when failing.
> 
> of_parse_subdev() now separates the error code and the pointer
> it looks up, to clarify the interface. There are two cases
> where this function originally returns 'NULL', and I have
> changed that to '0' for success to keep the current behavior,
> though returning an error would also make sense there.
> 
> Fixes: e130291212df ("[media] media: Add i.MX media core driver")
> Signed-off-by: Arnd Bergmann 
> ---
> v2: fix type mismatch
> v3: rework of_parse_subdev() as well.

Thanks!

Reviewed-by: Philipp Zabel 
Tested-by: Philipp Zabel 

regards
Philipp

Re: Lots of new warnings with gcc-7.1.1

[Arnd Bergmann]

On Wed, Jul 12, 2017 at 3:10 PM, Greg Kroah-Hartman
 wrote:
> On Tue, Jul 11, 2017 at 03:35:15PM -0700, Linus Torvalds wrote:
>> [ Very random list of maintainers and mailing lists, at least
>> partially by number of warnings generated by gcc-7.1.1 that is then
>> correlated with the get_maintainers script ]
>>
>> So I upgraded one of my boxes to F26, which upgraded the compiler to 
>> gcc-7.1.1
>>
>> Which in turn means that my nice clean allmodconfig compile is not an
>> unholy mess of annoying new warnings.
>
> I asked Arnd about this the other day on IRC as I've hit this as well on
> the stable releases, and it's really annoying.  He mentioned that he had
> lots of these warnings fixed, but didn't push most of the changes out
> yet.

To clarify: most of the patches I wrote ended up getting merged, but
there were a couple that I did not submit a second time after they
got dropped, but I gave up on trying to fix the new -Wformat warnings
and simply disabled them, hoping someone else would do it before me,
or that the gcc developers would find a way to reduce the false-positive
ones before the release.

>  Arnd, any repo with them in it that we could look at?

I have a private tree on my workstation that has lots of random
crap, and I rebase it all the time but normally don't publish it.

I have uploaded today's snapshot to

git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git randconfig-4.13-next

The way I work with this is helpful to catch build regressions as soon
as they happen, but not so good in finding things that I have either
submitted a patch for before and don't remember if it should be
resubmitted, or stuff that I decided I didn't want to deal with at some
point.

I was already planning to start over from scratch one of these days,
and cherry-pick+resubmit the patches that are actually required
for randconfig builds.

>> Anyway, it would be lovely if some of the more affected developers
>> would take a look at gcc-7.1.1 warnings. Right now I get about three
>> *thousand* lines of warnings from a "make allmodconfig" build, which
>> makes them a bit overwhelming.
>
> I only have 310 when building the 4.12.0 release with 7.1.1, I wonder if
> Fedora turned more warnings on in their compiler release, I'm running
> Arch here:
> $ gcc --version
> gcc (GCC) 7.1.1 20170621

This is what I get in today's linux-next:

$ grep error: 4.13-next-allmod-warning | sed -e 's:^.*\[-W:-W:' | sort
| uniq -c | cut -f 1 -d\] | sort -n
  1 -Werror=parentheses
  2 -Werror=tautological-compare
  2 -Werror=unused-result
 34 -Werror=format-overflow=
 41 -Werror=int-in-bool-context
233 -Werror=format-truncation=

I'll resubmit the patches for -Wparenthese, -Wtautological-compar,
-Wunused-result and -Wint-in-bool-context that I had sent earlier,
plus a new patch to move -Wformat-truncation into W=1.

  Arnd

Re: Lots of new warnings with gcc-7.1.1

[Arnd Bergmann]

On Wed, Jul 12, 2017 at 5:41 AM, Linus Torvalds
 wrote:

>
> We also have about a bazillion
>
> warning: ‘*’ in boolean context, suggest ‘&&’ instead
>
> warnings in drivers/ata/libata-core.c, all due to a single macro that
> uses a pattern that gcc-7.1.1 doesn't like. The warning looks a bit
> debatable, but I suspect the macro could easily be changed too.
>
> Tejun, would you hate just moving the "multiply by 1000" part _into_
> that EZ() macro? Something like the attached (UNTESTED!) patch?

Tejun applied an almost identical patch of mine a while ago, but it seems to
have gotten lost in the meantime in some rebase:

https://patchwork.kernel.org/patch/9721397/
https://patchwork.kernel.org/patch/9721399/

I guess I should have resubmitted the second patch with the suggested
improvement.

 Arnd

Re: Lots of new warnings with gcc-7.1.1

[Greg Kroah-Hartman]

On Tue, Jul 11, 2017 at 03:35:15PM -0700, Linus Torvalds wrote:
> [ Very random list of maintainers and mailing lists, at least
> partially by number of warnings generated by gcc-7.1.1 that is then
> correlated with the get_maintainers script ]
> 
> So I upgraded one of my boxes to F26, which upgraded the compiler to gcc-7.1.1
> 
> Which in turn means that my nice clean allmodconfig compile is not an
> unholy mess of annoying new warnings.

I asked Arnd about this the other day on IRC as I've hit this as well on
the stable releases, and it's really annoying.  He mentioned that he had
lots of these warnings fixed, but didn't push most of the changes out
yet.  Arnd, any repo with them in it that we could look at?

> Normally I hate the stupid new warnings, but this time around they are
> actually exactly the kinds of warnings you'd want to see and that are
> hard for humans to pick out errors: lots of format errors wrt limited
> buffer sizes.
> 
> At the same time, many of them *are* annoying. We have various limited
> buffers that are limited for a good reason, and some of the format
> truncation warnings are about numbers in the range {0-MAX_INT], where
> we definitely know that we don't need to worry about the really big
> ones.
> 
> After all, we're using "snprintf()" for a reason - we *want* to
> truncate if the buffer is too small.

Yeah, that's the warnings in the USB core code, we "know" this will not
happen, and we are using snprintf() for that reason as well, I don't
know how to fool gcc into the fact that it's all ok here.

> Anyway, it would be lovely if some of the more affected developers
> would take a look at gcc-7.1.1 warnings. Right now I get about three
> *thousand* lines of warnings from a "make allmodconfig" build, which
> makes them a bit overwhelming.

I only have 310 when building the 4.12.0 release with 7.1.1, I wonder if
Fedora turned more warnings on in their compiler release, I'm running
Arch here:
$ gcc --version
gcc (GCC) 7.1.1 20170621

thanks,

greg k-h

Re: Lots of new warnings with gcc-7.1.1

[Mauro Carvalho Chehab]

Em Tue, 11 Jul 2017 15:35:15 -0700
Linus Torvalds  escreveu:

> [ Very random list of maintainers and mailing lists, at least
> partially by number of warnings generated by gcc-7.1.1 that is then
> correlated with the get_maintainers script ]

Under drivers/media, I fixed a bunch of gcc 7.1 warnings before the
merge window. While most were just noise, some actually pointed to
human errors.

Now, gcc-7.1.1 produces only 6 warnings with W=1 on x86_64 (allyesconfig), 
either due to unused-but-set-variable or unused-const-variable. I guess
both warning options are disabled by default. Anyway, I have patches
to fix them already. I'll send you later.

The atomisp staging driver is a completely different beast, with would
produce itself a huge amount of warnings. I ended by adding some
logic on drivers/staging/media/atomisp/ Makefiles to disable them:

ccflags-y += $(call cc-disable-warning, missing-declarations)
ccflags-y += $(call cc-disable-warning, missing-prototypes)
ccflags-y += $(call cc-disable-warning, unused-but-set-variable)
ccflags-y += $(call cc-disable-warning, unused-const-variable)
ccflags-y += $(call cc-disable-warning, suggest-attribute=format)
ccflags-y += $(call cc-disable-warning, implicit-fallthrough)

(there's actually one patch pending related to atomisp, that I'll also
be sending you soon - meant to avoid warnings if compiled with an older
gcc version)

Thanks,
Mauro

[PATCH] media: staging: atomisp: disable warnings with cc-disable-warning

[Mauro Carvalho Chehab]

Instead of directly using -Wno-foo, use cc-disable-warning, as it
checks if the compiler has the warnings we want to disable.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/staging/media/atomisp/pci/atomisp2/Makefile | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/Makefile 
b/drivers/staging/media/atomisp/pci/atomisp2/Makefile
index 726eaa293c55..2bd98f0667ec 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/Makefile
+++ b/drivers/staging/media/atomisp/pci/atomisp2/Makefile
@@ -354,7 +354,9 @@ ccflags-y += $(INCLUDES) $(DEFINES) -fno-common
 
 # HACK! While this driver is in bad shape, don't enable several warnings
 #   that would be otherwise enabled with W=1
-ccflags-y += -Wno-unused-const-variable -Wno-missing-prototypes \
--Wno-unused-but-set-variable -Wno-missing-declarations \
--Wno-suggest-attribute=format -Wno-missing-prototypes \
--Wno-implicit-fallthrough
+ccflags-y += $(call cc-disable-warning, implicit-fallthrough)
+ccflags-y += $(call cc-disable-warning, missing-prototypes)
+ccflags-y += $(call cc-disable-warning, missing-declarations)
+ccflags-y += $(call cc-disable-warning, suggest-attribute=format)
+ccflags-y += $(call cc-disable-warning, unused-const-variable)
+ccflags-y += $(call cc-disable-warning, unused-but-set-variable)
-- 
2.13.0

Re: [PATCH v2 2/3] drm: rcar-du: Fix planes to CRTC assignment when using the VSP

[Laurent Pinchart]

Hi Kieran,

On Wednesday 12 Jul 2017 11:30:19 Kieran Bingham wrote:
> On 11/07/17 23:29, Laurent Pinchart wrote:
> > The DU can compose the output of a VSP with other planes on Gen2
> > hardware, and of two VSPs on Gen3 hardware. Neither of these features
> > are supported by the driver, and the current implementation always
> > assigns planes to CRTCs the same way.
> > 
> > Simplify the implementation by configuring plane assignment when setting
> > up DU groups, instead of recomputing it for every atomic plane update.
> > This allows skipping the wait for vertical blanking when stopping a
> > CRTC, as there's no need to reconfigure plane assignment at that point.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> Reviewed-by: Kieran Bingham 
> 
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 31 ++-
> >  drivers/gpu/drm/rcar-du/rcar_du_group.c | 12 
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 28 +---
> >  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 10 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  9 -
> >  5 files changed, 46 insertions(+), 44 deletions(-)

[snip]

> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_group.c index
> > 00d5f470d377..d26b647207b8 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > @@ -126,6 +126,18 @@ static void rcar_du_group_setup(struct rcar_du_group
> > *rgrp)> 
> > if (rcdu->info->gen >= 3)
> > rcar_du_group_write(rgrp, DEFR10, DEFR10_CODE | 
DEFR10_DEFE10);
> > 
> > +   if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> > +   /*
> > +* The CRTCs can compose the output of a VSP with other planes
> > +* on Gen2 hardware, and of two VSPs on Gen3 hardware. Neither
> > +* of these features are supported by the driver, so we 
hardcode
> > +* plane assignment to CRTCs when setting the group up to 
avoid
> > +* the need to restart then group when setting planes up.
> 
> Minor nits in comment:
> 
>   /restart then group/restart the group/
> 
> I would also possibly swap the final 'planes up' as 'up planes' if you
> update here anyway:
> 
> * so we hardcode plane assignment to CRTCs when setting the group up to
> avoid
> * the need to restart the group when setting up planes.
> 
> Up to you of course :)

Thanks, I've fixed both, and also replaced "setting the group up" with 
"setting up the group".

> > +*/
> > +   rcar_du_group_write(rgrp, DS1PR, 1);
> > +   rcar_du_group_write(rgrp, DS2PR, rcdu->info->gen >= 3 ? 3 : 
2);
> 
> whew ... that DS2PR indexing change from g2 to g3 looks annoying ... I had
> to write out the logic tables on paper to verify the change here from the
> previous code.

That's also how I wrote the code :-)

> > +   }
> > +
> > 
> > /*
> > 
> >  * Use DS1PR and DS2PR to configure planes priorities and connects the
> >  * superposition 0 to DU0 pins. DU1 pins will be configured 
dynamically.

-- 
Regards,

Laurent Pinchart

[PATCH] media: vimc: cleanup a few warnings

[Mauro Carvalho Chehab]

The const structs uded by MODULE_DEVICE_TABLE()
may never be used with COMPILE_TEST:

drivers/media/platform/vimc/vimc-capture.c:528:40: warning: 
'vimc_cap_driver_ids' defined but not used [-Wunused-const-variable=]
 static const struct platform_device_id vimc_cap_driver_ids[] = {
^~~
drivers/media/platform/vimc/vimc-debayer.c:588:40: warning: 
'vimc_deb_driver_ids' defined but not used [-Wunused-const-variable=]
 static const struct platform_device_id vimc_deb_driver_ids[] = {
^~~
drivers/media/platform/vimc/vimc-scaler.c:442:40: warning: 
'vimc_sca_driver_ids' defined but not used [-Wunused-const-variable=]
 static const struct platform_device_id vimc_sca_driver_ids[] = {
^~~
drivers/media/platform/vimc/vimc-sensor.c:376:40: warning: 
'vimc_sen_driver_ids' defined but not used [-Wunused-const-variable=]
 static const struct platform_device_id vimc_sen_driver_ids[] = {
^~~

So, add the proper notation to avoid warnings.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/platform/vimc/vimc-capture.c | 3 ++-
 drivers/media/platform/vimc/vimc-debayer.c | 3 ++-
 drivers/media/platform/vimc/vimc-scaler.c  | 3 ++-
 drivers/media/platform/vimc/vimc-sensor.c  | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c 
b/drivers/media/platform/vimc/vimc-capture.c
index 14cb32e21130..c6f4a407e019 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -525,7 +525,8 @@ static struct platform_driver vimc_cap_pdrv = {
},
 };
 
-static const struct platform_device_id vimc_cap_driver_ids[] = {
+static const __maybe_unused
+struct platform_device_id vimc_cap_driver_ids[] = {
{
.name   = VIMC_CAP_DRV_NAME,
},
diff --git a/drivers/media/platform/vimc/vimc-debayer.c 
b/drivers/media/platform/vimc/vimc-debayer.c
index 35b15bd4d61d..428454e33b75 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -585,7 +585,8 @@ static struct platform_driver vimc_deb_pdrv = {
},
 };
 
-static const struct platform_device_id vimc_deb_driver_ids[] = {
+static const __maybe_unused
+struct platform_device_id vimc_deb_driver_ids[] = {
{
.name   = VIMC_DEB_DRV_NAME,
},
diff --git a/drivers/media/platform/vimc/vimc-scaler.c 
b/drivers/media/platform/vimc/vimc-scaler.c
index fe77505d2679..35bf3b32108f 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -439,7 +439,8 @@ static struct platform_driver vimc_sca_pdrv = {
},
 };
 
-static const struct platform_device_id vimc_sca_driver_ids[] = {
+static const __maybe_unused
+struct platform_device_id vimc_sca_driver_ids[] = {
{
.name   = VIMC_SCA_DRV_NAME,
},
diff --git a/drivers/media/platform/vimc/vimc-sensor.c 
b/drivers/media/platform/vimc/vimc-sensor.c
index ebdbbe8c05ed..9ad2be111a14 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -373,7 +373,8 @@ static struct platform_driver vimc_sen_pdrv = {
},
 };
 
-static const struct platform_device_id vimc_sen_driver_ids[] = {
+static const __maybe_unused
+struct platform_device_id vimc_sen_driver_ids[] = {
{
.name   = VIMC_SEN_DRV_NAME,
},
-- 
2.13.0

Re: [PATCH v2 1/3] drm: rcar-du: Use the VBK interrupt for vblank events

[Kieran Bingham]

Hi Laurent,

On 11/07/17 23:29, Laurent Pinchart wrote:
> When implementing support for interlaced modes, the driver switched from
> reporting vblank events on the vertical blanking (VBK) interrupt to the
> frame end interrupt (FRM). This incorrectly divided the reported refresh
> rate by two. Fix it by moving back to the VBK interrupt.
> 
> Fixes: 906eff7fcada ("drm: rcar-du: Implement support for interlaced modes")
> Signed-off-by: Laurent Pinchart 

Of course, this looks much more correct than the patch I submitted :-)

Reviewed-by: Kieran Bingham 

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 98cf446391dc..17fd1cd5212c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -698,7 +698,7 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
>   status = rcar_du_crtc_read(rcrtc, DSSR);
>   rcar_du_crtc_write(rcrtc, DSRCR, status & DSRCR_MASK);
>  
> - if (status & DSSR_FRM) {
> + if (status & DSSR_VBK) {
>   drm_crtc_handle_vblank(>crtc);
>  
>   if (rcdu->info->gen < 3)
> 

Re: [PATCH v2 2/3] drm: rcar-du: Fix planes to CRTC assignment when using the VSP

[Kieran Bingham]

Hi Laurent,

Thanks for the patch

Only a minor nit on one comment, but aside from that,

On 11/07/17 23:29, Laurent Pinchart wrote:
> The DU can compose the output of a VSP with other planes on Gen2
> hardware, and of two VSPs on Gen3 hardware. Neither of these features
> are supported by the driver, and the current implementation always
> assigns planes to CRTCs the same way.
> 
> Simplify the implementation by configuring plane assignment when setting
> up DU groups, instead of recomputing it for every atomic plane update.
> This allows skipping the wait for vertical blanking when stopping a
> CRTC, as there's no need to reconfigure plane assignment at that point.
> 
> Signed-off-by: Laurent Pinchart 

Reviewed-by: Kieran Bingham 

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 31 ---
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 12 
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 28 +---
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 10 +-
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  9 -
>  5 files changed, 46 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 17fd1cd5212c..413ab032afed 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -315,6 +315,10 @@ static void rcar_du_crtc_update_planes(struct 
> rcar_du_crtc *rcrtc)
>   unsigned int i;
>   u32 dspr = 0;
>  
> + /* Plane assignment is fixed when using the VSP. */
> + if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
> + return;
> +
>   for (i = 0; i < rcrtc->group->num_planes; ++i) {
>   struct rcar_du_plane *plane = >group->planes[i];
>   unsigned int j;
> @@ -351,17 +355,6 @@ static void rcar_du_crtc_update_planes(struct 
> rcar_du_crtc *rcrtc)
>   }
>   }
>  
> - /* If VSP+DU integration is enabled the plane assignment is fixed. */
> - if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> - if (rcdu->info->gen < 3) {
> - dspr = (rcrtc->index % 2) + 1;
> - hwplanes = 1 << (rcrtc->index % 2);
> - } else {
> - dspr = (rcrtc->index % 2) ? 3 : 1;
> - hwplanes = 1 << ((rcrtc->index % 2) ? 2 : 0);
> - }
> - }
> -
>   /*
>* Update the planes to display timing and dot clock generator
>* associations.
> @@ -462,8 +455,13 @@ static void rcar_du_crtc_setup(struct rcar_du_crtc 
> *rcrtc)
>   rcar_du_crtc_set_display_timing(rcrtc);
>   rcar_du_group_set_routing(rcrtc->group);
>  
> - /* Start with all planes disabled. */
> - rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
> + /*
> +  * Start with all planes disabled, except when using the VSP in which
> +  * case the fixed plane assignment must not be modified.
> +  */
> + if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> + rcar_du_group_write(rcrtc->group,
> + rcrtc->index % 2 ? DS2PR : DS1PR, 0);
>  
>   /* Enable the VSP compositor. */
>   if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> @@ -505,8 +503,11 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>* are stopped in one operation as we now wait for one vblank per CRTC.
>* Whether this can be improved needs to be researched.
>*/
> - rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
> - drm_crtc_wait_one_vblank(crtc);
> + if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> + rcar_du_group_write(rcrtc->group,
> + rcrtc->index % 2 ? DS2PR : DS1PR, 0);
> + drm_crtc_wait_one_vblank(crtc);
> + }
>  
>   /*
>* Disable vertical blanking interrupt reporting. We first need to wait
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 00d5f470d377..d26b647207b8 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -126,6 +126,18 @@ static void rcar_du_group_setup(struct rcar_du_group 
> *rgrp)
>   if (rcdu->info->gen >= 3)
>   rcar_du_group_write(rgrp, DEFR10, DEFR10_CODE | DEFR10_DEFE10);
>  
> + if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> + /*
> +  * The CRTCs can compose the output of a VSP with other planes
> +  * on Gen2 hardware, and of two VSPs on Gen3 hardware. Neither
> +  * of these features are supported by the driver, so we hardcode
> +  * plane assignment to CRTCs when setting the group up to avoid
> +  * the need to 

Re: Query: IR remote over android

[Sean Young]

On Wed, Jul 12, 2017 at 02:40:20PM +0530, Sharma, Jitendra wrote:
> Hi,
> 
> I am working on a android project. Here, I want to enable Remote control
> support on one of our custom msm chipset based board.
> 
> The idea is, once board boot up, then via HDMI over HDMI monitor we will see
> android UI, and we want to browse through that UI using any standard
> protocol(like RC6 or nec) based remote
> 
> For enabling remote control support, I followed below steps:
> 
> 1) Enabled RC support for driver compilation in our defconfig file like:
> 
> +CONFIG_MEDIA_RC_SUPPORT=y
> +CONFIG_RC_DEVICES=y
> +CONFIG_IR_GPIO_CIR=y
> 
> 2) We have one RC6 philips remote. So, we created keycode file using
> scancodes and used that keycode file for device node mentioned below.
> 
> 3) As IR receiver is connected via gpio over our custom board, so we add a
> device tree entry like:
> 
> +   ir: ir-receiver {
> +   compatible = "gpio-ir-receiver";
> +   gpios = < 120 1>;
> +   linux,rc-map-name = "rc-rc6-philips"; /*rc-rc6-philips is
> the keycode file for one RC6 protocol based file*/
> +   };
> 4) Finally create boot.img and flash it onto board
> 
> Now our observation with above created boot.img is as follows:
> 
> 1) We boot up without HDMI connected (For our case till we not connect HDMI,
> android userspace won't come up).
> 
> 2) Via getevent tool, we could see remote events coming up proper . This is
> Good case
> 
> 3) Now we connect HDMI (after connecting HDMI, android userspace gets up).
> We observed that via getevent tool, no event is coming up even after
> multiple remote key presses. This is bad case
> 
> I enabled IR_dprintk and for this bad case, I observed that for each key
> press, below logs appear continously:
> 
> [  128.208417] sample: (0us space)
> [  128.211341] sample: (0us space)
> [  128.211683] sample: (0us space)
> [  128.212180] sample: (0us space)
> 
> And then eventually RC6 decoder function fails in very early stage.

Looking at that, I would guess that the edge trigger on the gpio pin
is firing at the right time, but gpio_get_value() is not returning
the correct value, so a space get passed every time rather than pulse
and space.

> 4) After more debugging, I observed that, if I apply below change in
> rc-main.c file and create and flash new boot.img in our board and boot it
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 3f0f71a..1acdd09 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1347,7 +1349,6 @@ int rc_register_device(struct rc_dev *dev)
> return -EINVAL;
> 
> set_bit(EV_KEY, dev->input_dev->evbit);
> -   set_bit(EV_REP, dev->input_dev->evbit);
> set_bit(EV_MSC, dev->input_dev->evbit);
> set_bit(MSC_SCAN, dev->input_dev->mscbit);
> if (dev->open)
> 
> then, after connecting HDMI, I could see remote working over android .
> 
> 
> So, my query is, does EV_REP in rc-main.c causing remote decoder function to
> fail. Is it some kind of bug. Or am i missing something.

This just tells the input layer to handle autorepeat and is entirely
unrelated; if the rc driver is not reporting pulse/space information
correctly then the input layer never gets any key presses anyway.

I would suspect that the connecting hdmi somehow does a few tricks with
your gpio ports or there is a heisenbug. 


Sean

Query: IR remote over android

[Sharma, Jitendra]

Hi,

I am working on a android project. Here, I want to enable Remote control 
support on one of our custom msm chipset based board.


The idea is, once board boot up, then via HDMI over HDMI monitor we will 
see android UI, and we want to browse through that UI using any standard 
protocol(like RC6 or nec) based remote


For enabling remote control support, I followed below steps:

1) Enabled RC support for driver compilation in our defconfig file like:

+CONFIG_MEDIA_RC_SUPPORT=y
+CONFIG_RC_DEVICES=y
+CONFIG_IR_GPIO_CIR=y

2) We have one RC6 philips remote. So, we created keycode file using 
scancodes and used that keycode file for device node mentioned below.


3) As IR receiver is connected via gpio over our custom board, so we add 
a device tree entry like:


+   ir: ir-receiver {
+   compatible = "gpio-ir-receiver";
+   gpios = < 120 1>;
+   linux,rc-map-name = "rc-rc6-philips"; /*rc-rc6-philips 
is the keycode file for one RC6 protocol based file*/

+   };
4) Finally create boot.img and flash it onto board

Now our observation with above created boot.img is as follows:

1) We boot up without HDMI connected (For our case till we not connect 
HDMI, android userspace won't come up).


2) Via getevent tool, we could see remote events coming up proper . This 
is Good case


3) Now we connect HDMI (after connecting HDMI, android userspace gets 
up). We observed that via getevent tool, no event is coming up even 
after multiple remote key presses. This is bad case


I enabled IR_dprintk and for this bad case, I observed that for each key 
press, below logs appear continously:


[  128.208417] sample: (0us space)
[  128.211341] sample: (0us space)
[  128.211683] sample: (0us space)
[  128.212180] sample: (0us space)

And then eventually RC6 decoder function fails in very early stage.

4) After more debugging, I observed that, if I apply below change in 
rc-main.c file and create and flash new boot.img in our board and boot it


diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 3f0f71a..1acdd09 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1347,7 +1349,6 @@ int rc_register_device(struct rc_dev *dev)
return -EINVAL;

set_bit(EV_KEY, dev->input_dev->evbit);
-   set_bit(EV_REP, dev->input_dev->evbit);
set_bit(EV_MSC, dev->input_dev->evbit);
set_bit(MSC_SCAN, dev->input_dev->mscbit);
if (dev->open)

then, after connecting HDMI, I could see remote working over android .


So, my query is, does EV_REP in rc-main.c causing remote decoder 
function to fail. Is it some kind of bug. Or am i missing something.



Thanks,

Jitendra

Re: Trying to use IR driver for my SoC

[Sean Young]

On Tue, Jul 11, 2017 at 11:51:02PM +0200, Mason wrote:
> On 11/07/2017 20:35, Sean Young wrote:
> 
> > Mason wrote:
> >
> >> Repeating the test (pressing '1' for one second) with ir-keytable:
> >>
> >> # ir-keytable -p all -t -v
> >> Found device /sys/class/rc/rc0/
> >> Input sysfs node is /sys/class/rc/rc0/input0/
> >> Event sysfs node is /sys/class/rc/rc0/input0/event0/
> >> Parsing uevent /sys/class/rc/rc0/input0/event0/uevent
> >> /sys/class/rc/rc0/input0/event0/uevent uevent MAJOR=13
> >> /sys/class/rc/rc0/input0/event0/uevent uevent MINOR=64
> >> /sys/class/rc/rc0/input0/event0/uevent uevent DEVNAME=input/event0
> >> Parsing uevent /sys/class/rc/rc0/uevent
> >> /sys/class/rc/rc0/uevent uevent NAME=rc-empty
> >> input device is /dev/input/event0
> >> /sys/class/rc/rc0/protocols protocol rc-5 (disabled)
> >> /sys/class/rc/rc0/protocols protocol nec (disabled)
> >> /sys/class/rc/rc0/protocols protocol rc-6 (disabled)
> >> Opening /dev/input/event0
> >> Input Protocol version: 0x00010001
> >> /sys/class/rc/rc0//protocols: Invalid argument
> >> Couldn't change the IR protocols
> >> Testing events. Please, press CTRL-C to abort.
> >> 1296.124872: event type EV_MSC(0x04): scancode = 0x4cb41
> >> 1296.124872: event type EV_SYN(0x00).
> >> 1296.178753: event type EV_MSC(0x04): scancode = 0x00
> >> 1296.178753: event type EV_SYN(0x00).
> >> 1296.286526: event type EV_MSC(0x04): scancode = 0x00
> >> 1296.286526: event type EV_SYN(0x00).
> >> 1296.394303: event type EV_MSC(0x04): scancode = 0x00
> >> 1296.394303: event type EV_SYN(0x00).
> >> 1296.502081: event type EV_MSC(0x04): scancode = 0x00
> >> 1296.502081: event type EV_SYN(0x00).
> >> 1296.609857: event type EV_MSC(0x04): scancode = 0x00
> >> 1296.609857: event type EV_SYN(0x00).
> >> 1296.717635: event type EV_MSC(0x04): scancode = 0x00
> >> 1296.717635: event type EV_SYN(0x00).
> >> 1296.825412: event type EV_MSC(0x04): scancode = 0x00
> >> 1296.825412: event type EV_SYN(0x00).
> >> 1296.933189: event type EV_MSC(0x04): scancode = 0x00
> >> 1296.933189: event type EV_SYN(0x00).
> >> 1297.040967: event type EV_MSC(0x04): scancode = 0x00
> >> 1297.040967: event type EV_SYN(0x00).
> >> 1297.148745: event type EV_MSC(0x04): scancode = 0x00
> >> 1297.148745: event type EV_SYN(0x00).
> >> 1297.256522: event type EV_MSC(0x04): scancode = 0x00
> >> 1297.256522: event type EV_SYN(0x00).
> >>
> >> It looks like scancode 0x00 means "REPEAT" ?
> > 
> > This looks like nec repeat to me; nec repeats are sent every 110ms;
> > however when a repeat occurs, the driver should call rc_repeat(),
> > sending a scancode of 0 won't work.
> 
> IIUC, the driver requires some fixup, to behave as user-space
> would expect; is that correct?

Yes, it does.

> Are you the reviewer for rc drivers?

Yes, I am. However there is plenty of knowledge on rc on this list. :)

> >> And 0x4cb41 would be '1' then?
> >>
> >> If I compile the legacy driver (which is much more cryptic)
> >> it outputs 04 cb 41 be
> > 
> > ~0xbe = 0x41. The code in tangox_ir_handle_nec() has decoded this
> > into extended nec (so the driver should send RC_TYPE_NECX), see
> > https://github.com/mansr/linux-tangox/blob/master/drivers/media/rc/tangox-ir.c#L68
> 
> Another required fixup IIUC, right?

Yes, there is.

> Thank you so much for all the insight you've provided.

np. It's nice to see this driver mainlined.

Thanks,
Sean