Re: [RESEND PATCH v7 2/2] media: dw9807: Add dw9807 vcm driver

[Tomasz Figa]

On Thu, Apr 12, 2018 at 6:57 PM Sakari Ailus 
wrote:

> Hi Jacopo,

> On Thu, Apr 12, 2018 at 10:57:01AM +0200, jacopo mondi wrote:
> ...
> > > +   if (MAX_RETRY == ++retry) {
> > > +   dev_err(>dev,
> > > +   "Cannot do the write operation because
VCM is busy\n");
> >
> > Nit: this is over 80 cols, it's fine, but I think you can really
> > shorten the error messag without losing context.

> dev_warn() or dev_info() might be more appropriate actually. Or even
> dev_dbg(). This isn't a grave problem; just a sign the user space is
trying
> to move the lens before it has reached its previous target position.

On the other hand, we print this only if we reach MAX_RETRY, which probably
means that the lens is stuck or some other unexpected failure.


> >
> > > +   return -EIO;
> > > +   }
> > > +   usleep_range(DW9807_CTRL_DELAY_US, DW9807_CTRL_DELAY_US +
10);
> >
> > mmm, I wonder if a sleep range of 10usecs is really a strict
> > requirement. Have a look at Documentation/timers/timers-howto.txt.
> > With such a small range you're likely fire some unrequired interrupt.

> If the user is trying to tell where to move the lens next, no time should
> be wasted on waiting. It'd perhaps rather make sense to return an error
> (-EBUSY): the user application (as well as the application developer)
would
> know about the attempt to move the lens too fast and could take an
informed
> decision on what to do next. This could include changing the target
> position, waiting more or changing the program to adjust the 3A loop
> behaviour.

Actually, shouldn't we wait for the lens to finish moving after we set the
position? If we don't do it, we risk the userspace requesting a capture
with the lens still moving.

If "time wasted on waiting" is a concern here, userspace could as well just
have a separate thread for controlling the lens (as something that is
expected to take time due to physical limitations).

Best regards,
Tomasz

Re: Donation

[M. M. Fridman]

--
I Mikhail Fridman. has selected you specially as one of my beneficiaries
for my Charitable Donation, Just as I have declared on May 23, 2016 to 
give

my fortune as charity.

Check the link below for confirmation:

http://www.ibtimes.co.uk/russias-second-wealthiest-man-mikhail-fridman-plans-leaving-14-2bn-fortune-charity-1561604

Reply as soon as possible with further directives.

Best Regards,
Mikhail Fridman.

cron job: media_tree daily build: OK

[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:   Mon Apr 16 05:00:13 CEST 2018
media-tree git hash:b284d4d5a6785f8cd07eda2646a95782373cd01e
media_build git hash:   78d6ded165a133942a9615a80ca8e0d48749c592
v4l-utils git hash: 47d43b130dc6e9e0edc900759fb37649208371e4
gcc version:i686-linux-gcc (GCC) 7.3.0
sparse version: v0.5.2-rc1
smatch version: v0.5.0-4419-g3b5bf5c9
host hardware:  x86_64
host os:4.14.0-3-amd64

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: OK
linux-git-arm-stm32: OK
linux-git-arm64: OK
linux-git-i686: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
linux-2.6.36.4-i686: OK
linux-2.6.36.4-x86_64: OK
linux-2.6.37.6-i686: OK
linux-2.6.37.6-x86_64: OK
linux-2.6.38.8-i686: OK
linux-2.6.38.8-x86_64: OK
linux-2.6.39.4-i686: OK
linux-2.6.39.4-x86_64: OK
linux-3.0.101-i686: OK
linux-3.0.101-x86_64: OK
linux-3.1.10-i686: OK
linux-3.1.10-x86_64: OK
linux-3.2.101-i686: OK
linux-3.2.101-x86_64: OK
linux-3.3.8-i686: OK
linux-3.3.8-x86_64: OK
linux-3.4.113-i686: OK
linux-3.4.113-x86_64: OK
linux-3.5.7-i686: OK
linux-3.5.7-x86_64: OK
linux-3.6.11-i686: OK
linux-3.6.11-x86_64: OK
linux-3.7.10-i686: OK
linux-3.7.10-x86_64: OK
linux-3.8.13-i686: OK
linux-3.8.13-x86_64: OK
linux-3.9.11-i686: OK
linux-3.9.11-x86_64: OK
linux-3.10.108-i686: OK
linux-3.10.108-x86_64: OK
linux-3.11.10-i686: OK
linux-3.11.10-x86_64: OK
linux-3.12.74-i686: OK
linux-3.12.74-x86_64: OK
linux-3.13.11-i686: OK
linux-3.13.11-x86_64: OK
linux-3.14.79-i686: OK
linux-3.14.79-x86_64: OK
linux-3.15.10-i686: OK
linux-3.15.10-x86_64: OK
linux-3.16.56-i686: OK
linux-3.16.56-x86_64: OK
linux-3.17.8-i686: OK
linux-3.17.8-x86_64: OK
linux-3.18.102-i686: OK
linux-3.18.102-x86_64: OK
linux-3.19.8-i686: OK
linux-3.19.8-x86_64: OK
linux-4.0.9-i686: OK
linux-4.0.9-x86_64: OK
linux-4.1.51-i686: OK
linux-4.1.51-x86_64: OK
linux-4.2.8-i686: OK
linux-4.2.8-x86_64: OK
linux-4.3.6-i686: OK
linux-4.3.6-x86_64: OK
linux-4.4.109-i686: OK
linux-4.4.109-x86_64: OK
linux-4.5.7-i686: OK
linux-4.5.7-x86_64: OK
linux-4.6.7-i686: OK
linux-4.6.7-x86_64: OK
linux-4.7.10-i686: OK
linux-4.7.10-x86_64: OK
linux-4.8.17-i686: OK
linux-4.8.17-x86_64: OK
linux-4.9.91-i686: OK
linux-4.9.91-x86_64: OK
linux-4.14.31-i686: OK
linux-4.14.31-x86_64: OK
linux-4.15.14-i686: OK
linux-4.15.14-x86_64: OK
linux-4.16-i686: OK
linux-4.16-x86_64: OK
apps: OK
spec-git: OK
sparse: WARNINGS

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

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

[PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode

[Akinobu Mita]

The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler,
and the s_frame_interval() in subdev video ops could be called when the
device is under power saving mode.  These callbacks for ov772x driver
cause updating H/W registers that will fail under power saving mode.

This avoids it by not apply any changes to H/W if the device is not powered
up.  Instead the changes will be restored right after power-up.

Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Signed-off-by: Akinobu Mita 
---
* v2
- New patch

 drivers/media/i2c/ov772x.c | 77 +-
 1 file changed, 62 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 1297a21..c44728f 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -741,19 +741,29 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
struct ov772x_priv *priv = to_ov772x(sd);
struct v4l2_fract *tpf = >interval;
unsigned int fps;
-   int ret;
+   int ret = 0;
 
fps = ov772x_select_fps(priv, tpf);
 
-   ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
-   if (ret)
-   return ret;
+   mutex_lock(>power_lock);
+   /*
+* If the device is not powered up by the host driver do
+* not apply any changes to H/W at this time. Instead
+* the frame rate will be restored right after power-up.
+*/
+   if (priv->power_count > 0) {
+   ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
+   if (ret)
+   goto error;
+   }
 
tpf->numerator = 1;
tpf->denominator = fps;
priv->fps = fps;
+error:
+   mutex_unlock(>power_lock);
 
-   return 0;
+   return ret;
 }
 
 static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -765,6 +775,16 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
int ret = 0;
u8 val;
 
+   /* v4l2_ctrl_lock() locks our own mutex */
+
+   /*
+* If the device is not powered up by the host driver do
+* not apply any controls to H/W at this time. Instead
+* the controls will be restored right after power-up.
+*/
+   if (priv->power_count == 0)
+   return 0;
+
switch (ctrl->id) {
case V4L2_CID_VFLIP:
val = ctrl->val ? VFLIP_IMG : 0x00;
@@ -888,6 +908,10 @@ static int ov772x_power_off(struct ov772x_priv *priv)
return 0;
 }
 
+static int ov772x_set_params(struct ov772x_priv *priv,
+const struct ov772x_color_format *cfmt,
+const struct ov772x_win_size *win);
+
 static int ov772x_s_power(struct v4l2_subdev *sd, int on)
 {
struct ov772x_priv *priv = to_ov772x(sd);
@@ -898,8 +922,20 @@ static int ov772x_s_power(struct v4l2_subdev *sd, int on)
/* If the power count is modified from 0 to != 0 or from != 0 to 0,
 * update the power state.
 */
-   if (priv->power_count == !on)
-   ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
+   if (priv->power_count == !on) {
+   if (on) {
+   ret = ov772x_power_on(priv);
+   /* Restore the controls */
+   if (!ret)
+   ret = ov772x_set_params(priv, priv->cfmt,
+   priv->win);
+   /* Restore the format and the frame rate */
+   if (!ret)
+   ret = __v4l2_ctrl_handler_setup(>hdl);
+   } else {
+   ret = ov772x_power_off(priv);
+   }
+   }
 
if (!ret) {
/* Update the power count. */
@@ -1163,7 +1199,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
struct v4l2_mbus_framefmt *mf = >format;
const struct ov772x_color_format *cfmt;
const struct ov772x_win_size *win;
-   int ret;
+   int ret = 0;
 
if (format->pad)
return -EINVAL;
@@ -1184,14 +1220,23 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
return 0;
}
 
-   ret = ov772x_set_params(priv, cfmt, win);
-   if (ret < 0)
-   return ret;
-
+   mutex_lock(>power_lock);
+   /*
+* If the device is not powered up by the host driver do
+* not apply any changes to H/W at this time. Instead
+* the format will be restored right after power-up.
+*/
+   if (priv->power_count > 0) {
+   ret = ov772x_set_params(priv, cfmt, win);
+   if (ret < 0)
+   goto error;
+ 

[PATCH v2 08/10] media: ov772x: handle nested s_power() calls

[Akinobu Mita]

Depending on the v4l2 driver, calling s_power() could be nested.  So the
actual transitions between power saving mode and normal operation mode
should only happen at the first power on and the last power off.

This adds an s_power() nesting counter and updates the power state if the
counter is modified from 0 to != 0 or from != 0 to 0.

Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Signed-off-by: Akinobu Mita 
---
* v2
- New patch

 drivers/media/i2c/ov772x.c | 33 +
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 4245a46..2cd6e85 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -424,6 +424,9 @@ struct ov772x_priv {
/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
unsigned shortband_filter;
unsigned int  fps;
+   /* lock to protect power_count */
+   struct mutex  power_lock;
+   int   power_count;
 #ifdef CONFIG_MEDIA_CONTROLLER
struct media_pad pad;
 #endif
@@ -871,9 +874,25 @@ static int ov772x_power_off(struct ov772x_priv *priv)
 static int ov772x_s_power(struct v4l2_subdev *sd, int on)
 {
struct ov772x_priv *priv = to_ov772x(sd);
+   int ret = 0;
+
+   mutex_lock(>power_lock);
 
-   return on ? ov772x_power_on(priv) :
-   ov772x_power_off(priv);
+   /* If the power count is modified from 0 to != 0 or from != 0 to 0,
+* update the power state.
+*/
+   if (priv->power_count == !on)
+   ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
+
+   if (!ret) {
+   /* Update the power count. */
+   priv->power_count += on ? 1 : -1;
+   WARN_ON(priv->power_count < 0);
+   }
+
+   mutex_unlock(>power_lock);
+
+   return ret;
 }
 
 static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
@@ -1303,6 +1322,7 @@ static int ov772x_probe(struct i2c_client *client,
return -ENOMEM;
 
priv->info = client->dev.platform_data;
+   mutex_init(>power_lock);
 
v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -1314,8 +1334,10 @@ static int ov772x_probe(struct i2c_client *client,
v4l2_ctrl_new_std(>hdl, _ctrl_ops,
  V4L2_CID_BAND_STOP_FILTER, 0, 256, 1, 0);
priv->subdev.ctrl_handler = >hdl;
-   if (priv->hdl.error)
-   return priv->hdl.error;
+   if (priv->hdl.error) {
+   ret = priv->hdl.error;
+   goto error_mutex_destroy;
+   }
 
priv->clk = clk_get(>dev, "xclk");
if (IS_ERR(priv->clk)) {
@@ -1363,6 +1385,8 @@ static int ov772x_probe(struct i2c_client *client,
clk_put(priv->clk);
 error_ctrl_free:
v4l2_ctrl_handler_free(>hdl);
+error_mutex_destroy:
+   mutex_destroy(>power_lock);
 
return ret;
 }
@@ -1377,6 +1401,7 @@ static int ov772x_remove(struct i2c_client *client)
gpiod_put(priv->pwdn_gpio);
v4l2_async_unregister_subdev(>subdev);
v4l2_ctrl_handler_free(>hdl);
+   mutex_destroy(>power_lock);
 
return 0;
 }
-- 
2.7.4

[PATCH v2 04/10] media: ov772x: add media controller support

[Akinobu Mita]

Create a source pad and set the media controller type to the sensor.

Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Signed-off-by: Akinobu Mita 
---
* v2
- Move video_probe() before the entity initialization and remove the #ifdef
  around the media_entity_cleanup()

 drivers/media/i2c/ov772x.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 188f2f1..0ae2a4f 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -424,6 +424,9 @@ struct ov772x_priv {
/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
unsigned shortband_filter;
unsigned int  fps;
+#ifdef CONFIG_MEDIA_CONTROLLER
+   struct media_pad pad;
+#endif
 };
 
 /*
@@ -1318,16 +1321,26 @@ static int ov772x_probe(struct i2c_client *client,
if (ret < 0)
goto error_gpio_put;
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+   priv->pad.flags = MEDIA_PAD_FL_SOURCE;
+   priv->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+   ret = media_entity_pads_init(>subdev.entity, 1, >pad);
+   if (ret < 0)
+   goto error_gpio_put;
+#endif
+
priv->cfmt = _cfmts[0];
priv->win = _win_sizes[0];
priv->fps = 15;
 
ret = v4l2_async_register_subdev(>subdev);
if (ret)
-   goto error_gpio_put;
+   goto error_entity_cleanup;
 
return 0;
 
+error_entity_cleanup:
+   media_entity_cleanup(>subdev.entity);
 error_gpio_put:
if (priv->pwdn_gpio)
gpiod_put(priv->pwdn_gpio);
@@ -1343,6 +1356,7 @@ static int ov772x_remove(struct i2c_client *client)
 {
struct ov772x_priv *priv = to_ov772x(i2c_get_clientdata(client));
 
+   media_entity_cleanup(>subdev.entity);
clk_put(priv->clk);
if (priv->pwdn_gpio)
gpiod_put(priv->pwdn_gpio);
-- 
2.7.4

[PATCH v2 06/10] media: dt-bindings: ov772x: add device tree binding

[Akinobu Mita]

This adds a device tree binding documentation for OV7720/OV7725 sensor.

Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Cc: Rob Herring 
Signed-off-by: Akinobu Mita 
---
* v2
- Add "dt-bindings:" in the subject
- Add a brief description of the sensor
- Update the GPIO names
- Indicate the GPIO active level

 .../devicetree/bindings/media/i2c/ov772x.txt   | 42 ++
 MAINTAINERS|  1 +
 2 files changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt 
b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
new file mode 100644
index 000..b045503
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
@@ -0,0 +1,42 @@
+* Omnivision OV7720/OV7725 CMOS sensor
+
+The Omnivision OV7720/OV7725 sensor supports multiple resolutions output,
+such as VGA, QVGA, and any size scaling down from CIF to 40x30. It also can
+support the YUV422, RGB565/555/444, GRB422 or raw RGB output formats.
+
+Required Properties:
+- compatible: shall be one of
+   "ovti,ov7720"
+   "ovti,ov7725"
+- clocks: reference to the xclk input clock.
+- clock-names: shall be "xclk".
+
+Optional Properties:
+- reset-gpios: reference to the GPIO connected to the RSTB pin which is
+  active low, if any.
+- powerdown-gpios: reference to the GPIO connected to the PWDN pin which is
+  active high, if any.
+
+The device node shall contain one 'port' child node with one child 'endpoint'
+subnode for its digital output video port, in accordance with the video
+interface bindings defined in Documentation/devicetree/bindings/media/
+video-interfaces.txt.
+
+Example:
+
+ {
+   ov772x: camera@21 {
+   compatible = "ovti,ov7725";
+   reg = <0x21>;
+   reset-gpios = <_gpio_0 0 GPIO_ACTIVE_LOW>;
+   powerdown-gpios = <_gpio_0 1 GPIO_ACTIVE_LOW>;
+   clocks = <>;
+   clock-names = "xclk";
+
+   port {
+   ov772x_0: endpoint {
+   remote-endpoint = <_in0>;
+   };
+   };
+   };
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 0a1410d..f500953 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10344,6 +10344,7 @@ T:  git git://linuxtv.org/media_tree.git
 S: Odd fixes
 F: drivers/media/i2c/ov772x.c
 F: include/media/i2c/ov772x.h
+F: Documentation/devicetree/bindings/media/i2c/ov772x.txt
 
 OMNIVISION OV7740 SENSOR DRIVER
 M: Wenyou Yang 
-- 
2.7.4

[PATCH v2 09/10] media: ov772x: reconstruct s_frame_interval()

[Akinobu Mita]

This splits the s_frame_interval() in subdev video ops into selecting the
frame interval and setting up the registers.

This is a preparatory change to avoid accessing registers under power
saving mode.

Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Signed-off-by: Akinobu Mita 
---
* v2
- New patch

 drivers/media/i2c/ov772x.c | 56 +-
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 2cd6e85..1297a21 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -617,25 +617,16 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int 
enable)
return 0;
 }
 
-static int ov772x_set_frame_rate(struct ov772x_priv *priv,
-struct v4l2_fract *tpf,
-const struct ov772x_color_format *cfmt,
-const struct ov772x_win_size *win)
+static unsigned int ov772x_select_fps(struct ov772x_priv *priv,
+struct v4l2_fract *tpf)
 {
-   struct i2c_client *client = v4l2_get_subdevdata(>subdev);
-   unsigned long fin = clk_get_rate(priv->clk);
unsigned int fps = tpf->numerator ?
   tpf->denominator / tpf->numerator :
   tpf->denominator;
unsigned int best_diff;
-   unsigned int fsize;
-   unsigned int pclk;
unsigned int diff;
unsigned int idx;
unsigned int i;
-   u8 clkrc = 0;
-   u8 com4 = 0;
-   int ret;
 
/* Approximate to the closest supported frame interval. */
best_diff = ~0L;
@@ -646,7 +637,25 @@ static int ov772x_set_frame_rate(struct ov772x_priv *priv,
best_diff = diff;
}
}
-   fps = ov772x_frame_intervals[idx];
+
+   return ov772x_frame_intervals[idx];
+}
+
+static int ov772x_set_frame_rate(struct ov772x_priv *priv,
+unsigned int fps,
+const struct ov772x_color_format *cfmt,
+const struct ov772x_win_size *win)
+{
+   struct i2c_client *client = v4l2_get_subdevdata(>subdev);
+   unsigned long fin = clk_get_rate(priv->clk);
+   unsigned int fsize;
+   unsigned int pclk;
+   unsigned int best_diff;
+   unsigned int diff;
+   unsigned int i;
+   u8 clkrc = 0;
+   u8 com4 = 0;
+   int ret;
 
/* Use image size (with blankings) to calculate desired pixel clock. */
switch (cfmt->com7 & OFMT_MASK) {
@@ -711,10 +720,6 @@ static int ov772x_set_frame_rate(struct ov772x_priv *priv,
if (ret < 0)
return ret;
 
-   tpf->numerator = 1;
-   tpf->denominator = fps;
-   priv->fps = tpf->denominator;
-
return 0;
 }
 
@@ -735,8 +740,20 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
 {
struct ov772x_priv *priv = to_ov772x(sd);
struct v4l2_fract *tpf = >interval;
+   unsigned int fps;
+   int ret;
+
+   fps = ov772x_select_fps(priv, tpf);
+
+   ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
+   if (ret)
+   return ret;
 
-   return ov772x_set_frame_rate(priv, tpf, priv->cfmt, priv->win);
+   tpf->numerator = 1;
+   tpf->denominator = fps;
+   priv->fps = fps;
+
+   return 0;
 }
 
 static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -992,7 +1009,6 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 const struct ov772x_win_size *win)
 {
struct i2c_client *client = v4l2_get_subdevdata(>subdev);
-   struct v4l2_fract tpf;
int ret;
u8  val;
 
@@ -1074,9 +1090,7 @@ static int ov772x_set_params(struct ov772x_priv *priv,
goto ov772x_set_fmt_error;
 
/* COM4, CLKRC: Set pixel clock and framerate. */
-   tpf.numerator = 1;
-   tpf.denominator = priv->fps;
-   ret = ov772x_set_frame_rate(priv, , cfmt, win);
+   ret = ov772x_set_frame_rate(priv, priv->fps, cfmt, win);
if (ret < 0)
goto ov772x_set_fmt_error;
 
-- 
2.7.4

[PATCH v2 03/10] media: ov772x: create subdevice device node

[Akinobu Mita]

Set the V4L2_SUBDEV_FL_HAS_DEVNODE flag for the subdevice so that the
subdevice device node is created.

Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Signed-off-by: Akinobu Mita 
---
* v2
- No changes

 drivers/media/i2c/ov772x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 8badd6f..188f2f1 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1287,6 +1287,7 @@ static int ov772x_probe(struct i2c_client *client,
priv->info = client->dev.platform_data;
 
v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
+   priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
v4l2_ctrl_handler_init(>hdl, 3);
v4l2_ctrl_new_std(>hdl, _ctrl_ops,
  V4L2_CID_VFLIP, 0, 1, 1, 0);
-- 
2.7.4

[PATCH v2 07/10] media: ov772x: support device tree probing

[Akinobu Mita]

The ov772x driver currently only supports legacy platform data probe.
This change enables device tree probing.

Note that the platform data probe can select auto or manual edge control
mode, but the device tree probling can only select auto edge control mode
for now.

Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Signed-off-by: Akinobu Mita 
---
* v2
- Add missing NULL checks for priv->info
- Leave the check for the missing platform data if legacy platform data
  probe is used.

 drivers/media/i2c/ov772x.c | 61 --
 1 file changed, 43 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 88d1418a..4245a46 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -749,13 +749,13 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_VFLIP:
val = ctrl->val ? VFLIP_IMG : 0x00;
priv->flag_vflip = ctrl->val;
-   if (priv->info->flags & OV772X_FLAG_VFLIP)
+   if (priv->info && (priv->info->flags & OV772X_FLAG_VFLIP))
val ^= VFLIP_IMG;
return ov772x_mask_set(client, COM3, VFLIP_IMG, val);
case V4L2_CID_HFLIP:
val = ctrl->val ? HFLIP_IMG : 0x00;
priv->flag_hflip = ctrl->val;
-   if (priv->info->flags & OV772X_FLAG_HFLIP)
+   if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
val ^= HFLIP_IMG;
return ov772x_mask_set(client, COM3, HFLIP_IMG, val);
case V4L2_CID_BAND_STOP_FILTER:
@@ -914,19 +914,14 @@ static void ov772x_select_params(const struct 
v4l2_mbus_framefmt *mf,
*win = ov772x_select_win(mf->width, mf->height);
 }
 
-static int ov772x_set_params(struct ov772x_priv *priv,
-const struct ov772x_color_format *cfmt,
-const struct ov772x_win_size *win)
+static int ov772x_edgectrl(struct ov772x_priv *priv)
 {
struct i2c_client *client = v4l2_get_subdevdata(>subdev);
-   struct v4l2_fract tpf;
int ret;
-   u8  val;
 
-   /* Reset hardware. */
-   ov772x_reset(client);
+   if (!priv->info)
+   return 0;
 
-   /* Edge Ctrl. */
if (priv->info->edgectrl.strength & OV772X_MANUAL_EDGE_CTRL) {
/*
 * Manual Edge Control Mode.
@@ -937,19 +932,19 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 
ret = ov772x_mask_set(client, DSPAUTO, EDGE_ACTRL, 0x00);
if (ret < 0)
-   goto ov772x_set_fmt_error;
+   return ret;
 
ret = ov772x_mask_set(client,
  EDGE_TRSHLD, OV772X_EDGE_THRESHOLD_MASK,
  priv->info->edgectrl.threshold);
if (ret < 0)
-   goto ov772x_set_fmt_error;
+   return ret;
 
ret = ov772x_mask_set(client,
  EDGE_STRNGT, OV772X_EDGE_STRENGTH_MASK,
  priv->info->edgectrl.strength);
if (ret < 0)
-   goto ov772x_set_fmt_error;
+   return ret;
 
} else if (priv->info->edgectrl.upper > priv->info->edgectrl.lower) {
/*
@@ -961,15 +956,35 @@ static int ov772x_set_params(struct ov772x_priv *priv,
  EDGE_UPPER, OV772X_EDGE_UPPER_MASK,
  priv->info->edgectrl.upper);
if (ret < 0)
-   goto ov772x_set_fmt_error;
+   return ret;
 
ret = ov772x_mask_set(client,
  EDGE_LOWER, OV772X_EDGE_LOWER_MASK,
  priv->info->edgectrl.lower);
if (ret < 0)
-   goto ov772x_set_fmt_error;
+   return ret;
}
 
+   return 0;
+}
+
+static int ov772x_set_params(struct ov772x_priv *priv,
+const struct ov772x_color_format *cfmt,
+const struct ov772x_win_size *win)
+{
+   struct i2c_client *client = v4l2_get_subdevdata(>subdev);
+   struct v4l2_fract tpf;
+   int ret;
+   u8  val;
+
+   /* Reset hardware. */
+   ov772x_reset(client);
+
+   /* Edge Ctrl. */
+   ret =  ov772x_edgectrl(priv);
+   if (ret < 0)
+   goto ov772x_set_fmt_error;
+
/* Format and window size. */
ret = ov772x_write(client, HSTART, win->rect.left >> 2);
if (ret < 0)
@@ -1020,9 +1035,9 @@ 

[PATCH v2 02/10] media: ov772x: add checks for register read errors

[Akinobu Mita]

This change adds checks for register read errors and returns correct
error code.

Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Signed-off-by: Akinobu Mita 
---
* v2
- Assign the ov772x_read() return value to pid and ver directly
- Do the same for MIDH and MIDL

 drivers/media/i2c/ov772x.c | 20 ++--
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 7e79da0..8badd6f 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1146,7 +1146,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
 static int ov772x_video_probe(struct ov772x_priv *priv)
 {
struct i2c_client  *client = v4l2_get_subdevdata(>subdev);
-   u8  pid, ver;
+   int pid, ver, midh, midl;
const char *devname;
int ret;
 
@@ -1156,7 +1156,11 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
 
/* Check and show product ID and manufacturer ID. */
pid = ov772x_read(client, PID);
+   if (pid < 0)
+   return pid;
ver = ov772x_read(client, VER);
+   if (ver < 0)
+   return ver;
 
switch (VERSION(pid, ver)) {
case OV7720:
@@ -1172,13 +1176,17 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
goto done;
}
 
+   midh = ov772x_read(client, MIDH);
+   if (midh < 0)
+   return midh;
+   midl = ov772x_read(client, MIDL);
+   if (midl < 0)
+   return midl;
+
dev_info(>dev,
 "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
-devname,
-pid,
-ver,
-ov772x_read(client, MIDH),
-ov772x_read(client, MIDL));
+devname, pid, ver, midh, midl);
+
ret = v4l2_ctrl_handler_setup(>hdl);
 
 done:
-- 
2.7.4

[PATCH v2 01/10] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Akinobu Mita]

The ov772x driver only works when the i2c controller have
I2C_FUNC_PROTOCOL_MANGLING.  However, many i2c controller drivers don't
support it.

The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
it doesn't support repeated starts.

This changes the reading ov772x register method so that it doesn't
require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages.

Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Signed-off-by: Akinobu Mita 
---
* v2
- Replace the implementation of ov772x_read() instead of adding an
  alternative method

 drivers/media/i2c/ov772x.c | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index b62860c..7e79da0 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -542,9 +542,19 @@ static struct ov772x_priv *to_ov772x(struct v4l2_subdev 
*sd)
return container_of(sd, struct ov772x_priv, subdev);
 }
 
-static inline int ov772x_read(struct i2c_client *client, u8 addr)
+static int ov772x_read(struct i2c_client *client, u8 addr)
 {
-   return i2c_smbus_read_byte_data(client, addr);
+   int ret;
+   u8 val;
+
+   ret = i2c_master_send(client, , 1);
+   if (ret < 0)
+   return ret;
+   ret = i2c_master_recv(client, , 1);
+   if (ret < 0)
+   return ret;
+
+   return val;
 }
 
 static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
@@ -1255,10 +1265,9 @@ static int ov772x_probe(struct i2c_client *client,
return -EINVAL;
}
 
-   if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
- I2C_FUNC_PROTOCOL_MANGLING)) {
+   if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
dev_err(>dev,
-   "I2C-Adapter doesn't support SMBUS_BYTE_DATA or 
PROTOCOL_MANGLING\n");
+   "I2C-Adapter doesn't support SMBUS_BYTE_DATA\n");
return -EIO;
}
client->flags |= I2C_CLIENT_SCCB;
-- 
2.7.4

[PATCH v2 00/10] media: ov772x: support media controller, device tree probing, etc.

[Akinobu Mita]

This patchset includes support media controller, device tree probing and
other miscellanuous changes for ov772x driver.

* v2 (thanks to Jacopo Mondi)
- Replace the implementation of ov772x_read() instead of adding an
  alternative method
- Assign the ov772x_read() return value to pid and ver directly
- Do the same for MIDH and MIDL
- Move video_probe() before the entity initialization and remove the #ifdef
  around the media_entity_cleanup()
- Use generic names for reset and powerdown gpios (New)
- Add "dt-bindings:" in the subject
- Add a brief description of the sensor
- Update the GPIO names
- Indicate the GPIO active level
- Add missing NULL checks for priv->info
- Leave the check for the missing platform data if legacy platform data
  probe is used.
- Handle nested s_power() calls (New)
- Reconstruct s_frame_interval() (New)
- Avoid accessing registers under power saving mode (New)

Akinobu Mita (10):
  media: ov772x: allow i2c controllers without
I2C_FUNC_PROTOCOL_MANGLING
  media: ov772x: add checks for register read errors
  media: ov772x: create subdevice device node
  media: ov772x: add media controller support
  media: ov772x: use generic names for reset and powerdown gpios
  media: dt-bindings: ov772x: add device tree binding
  media: ov772x: support device tree probing
  media: ov772x: handle nested s_power() calls
  media: ov772x: reconstruct s_frame_interval()
  media: ov772x: avoid accessing registers under power saving mode

 .../devicetree/bindings/media/i2c/ov772x.txt   |  42 
 MAINTAINERS|   1 +
 arch/sh/boards/mach-migor/setup.c  |   5 +-
 drivers/media/i2c/ov772x.c | 275 -
 4 files changed, 255 insertions(+), 68 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt

Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Cc: Rob Herring 
-- 
2.7.4

[PATCH v2 05/10] media: ov772x: use generic names for reset and powerdown gpios

[Akinobu Mita]

The ov772x driver uses "rstb-gpios" and "pwdn-gpios" for reset and
powerdown pins.  However, using generic names for thse gpios is preferred.
("reset-gpios" and "powerdown-gpios" respectively)

There is only one mainline user for these gpios, so rename to generic
names.

Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Signed-off-by: Akinobu Mita 
---
* v2
- New patch

 arch/sh/boards/mach-migor/setup.c | 5 +++--
 drivers/media/i2c/ov772x.c| 8 
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/sh/boards/mach-migor/setup.c 
b/arch/sh/boards/mach-migor/setup.c
index 271dfc2..73b9ee4 100644
--- a/arch/sh/boards/mach-migor/setup.c
+++ b/arch/sh/boards/mach-migor/setup.c
@@ -351,8 +351,9 @@ static struct platform_device migor_ceu_device = {
 static struct gpiod_lookup_table ov7725_gpios = {
.dev_id = "0-0021",
.table  = {
-   GPIO_LOOKUP("sh7722_pfc", GPIO_PTT0, "pwdn", GPIO_ACTIVE_HIGH),
-   GPIO_LOOKUP("sh7722_pfc", GPIO_PTT3, "rstb", GPIO_ACTIVE_LOW),
+   GPIO_LOOKUP("sh7722_pfc", GPIO_PTT0, "powerdown",
+   GPIO_ACTIVE_HIGH),
+   GPIO_LOOKUP("sh7722_pfc", GPIO_PTT3, "reset", GPIO_ACTIVE_LOW),
},
 };
 
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 0ae2a4f..88d1418a 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -837,10 +837,10 @@ static int ov772x_power_on(struct ov772x_priv *priv)
 * available to handle this cleanly, request the GPIO temporarily
 * to avoid conflicts.
 */
-   priv->rstb_gpio = gpiod_get_optional(>dev, "rstb",
+   priv->rstb_gpio = gpiod_get_optional(>dev, "reset",
 GPIOD_OUT_LOW);
if (IS_ERR(priv->rstb_gpio)) {
-   dev_info(>dev, "Unable to get GPIO \"rstb\"");
+   dev_info(>dev, "Unable to get GPIO \"reset\"");
return PTR_ERR(priv->rstb_gpio);
}
 
@@ -1309,10 +1309,10 @@ static int ov772x_probe(struct i2c_client *client,
goto error_ctrl_free;
}
 
-   priv->pwdn_gpio = gpiod_get_optional(>dev, "pwdn",
+   priv->pwdn_gpio = gpiod_get_optional(>dev, "powerdown",
 GPIOD_OUT_LOW);
if (IS_ERR(priv->pwdn_gpio)) {
-   dev_info(>dev, "Unable to get GPIO \"pwdn\"");
+   dev_info(>dev, "Unable to get GPIO \"powerdown\"");
ret = PTR_ERR(priv->pwdn_gpio);
goto error_clk_put;
}
-- 
2.7.4

[PATCH] media: rc: mtk-cir: use of_device_get_match_data()

[Ryder Lee]

The usage of of_device_get_match_data() reduce the code size a bit.

Signed-off-by: Ryder Lee 
---
 drivers/media/rc/mtk-cir.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/rc/mtk-cir.c b/drivers/media/rc/mtk-cir.c
index e88eb64..e42efd9 100644
--- a/drivers/media/rc/mtk-cir.c
+++ b/drivers/media/rc/mtk-cir.c
@@ -299,8 +299,6 @@ static int mtk_ir_probe(struct platform_device *pdev)
 {
struct device *dev = >dev;
struct device_node *dn = dev->of_node;
-   const struct of_device_id *of_id =
-   of_match_device(mtk_ir_match, >dev);
struct resource *res;
struct mtk_ir *ir;
u32 val;
@@ -312,7 +310,7 @@ static int mtk_ir_probe(struct platform_device *pdev)
return -ENOMEM;
 
ir->dev = dev;
-   ir->data = of_id->data;
+   ir->data = of_device_get_match_data(dev);
 
ir->clk = devm_clk_get(dev, "clk");
if (IS_ERR(ir->clk)) {
-- 
1.9.1

OV5640 with 12MHz xclk

[Samuel Bobrowicz]

Can anyone verify if the OV5640 driver works with input clocks other
than the typical 24MHz? The driver suggests anything from 6MHz-24MHz
is acceptable, but I am running into issues while bringing up a module
that uses a 12MHz oscillator. I'd expect that different xclk's would
necessitate different register settings for the various resolutions
(PLL settings, PCLK width, etc.), however the driver does not seem to
modify nearly enough based on the frequency of xclk.

Sam

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

Comments I have snipped out from this reply are addressed, thanks for 
bringing them to my attention!

On 2018-04-05 11:10:01 +0200, Jacopo Mondi wrote:

[snip]

> > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> > +{
> > +   int timeout;
> > +
> > +   /* Wait for the clock and data lanes to enter LP-11 state. */
> > +   for (timeout = 100; timeout > 0; timeout--) {
> > +   const u32 lane_mask = (1 << priv->lanes) - 1;
> > +
> > +   if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 &&
> 
> Nitpicking:
>   if ((rcar_csi2_read(priv, PHCLM_REG) & 0x01) &&
> 
> Don't you prefer to provide defines also for bit fields instead of
> using magic values? In this case something like
> PHCLM_REG_STOPSTATE_CLK would do.

Thanks addressed per your and Kieran's suggestion.

> 
> Also, from tables 25.[17-20] it seems to me that for H3 and V3 you
> have to set INSTATE to an hardcoded value after having validated PHDLM.
> Maybe it is not necessary, just pointing it out.

I assume you mean Figures 25.[17-20] and not Tables as the last table in 
chapter 25 is Table 25.15 and the register in question is INTSTATE :-) 
And to clarify this is documented for H3 which this driver supports and 
V3H and M3-N which this driver dose not yet support. And the constant 
you are to set it to is ULPS_START | UPLS_END.

This is a good catch as this was introduced in a later version of the 
datasheet and the current code where the ULPS_START | UPLS_END is set 
before confirming LP-11 have kept on working. Check the 
priv->info->clear_ulps usage in rcar_csi2_start(). I do think it's 
better to follow the flow-chart in the new datasheet so I will move this 
to the end of rcar_csi2_start() to reflect that (provided that the end 
result still works :-) Thanks for pointing this out!

[snip]

> > +static int rcar_csi2_start(struct rcar_csi2 *priv)
> > +{
> > +   const struct rcar_csi2_format *format;
> > +   u32 phycnt, phypll, vcdt = 0, vcdt2 = 0;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> > +   priv->mf.width, priv->mf.height,
> > +   priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> > +
> > +   /* Code is validated in set_fmt */
> > +   format = rcar_csi2_code_to_fmt(priv->mf.code);
> > +
> > +   /*
> > +* Enable all Virtual Channels
> > +*
> > +* NOTE: It's not possible to get individual datatype for each
> > +*   source virtual channel. Once this is possible in V4L2
> > +*   it should be used here.
> > +*/
> > +   for (i = 0; i < 4; i++) {
> > +   u32 vcdt_part;
> > +
> > +   vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
> > +   VCDT_SEL_DT(format->datatype);
> > +
> > +   /* Store in correct reg and offset */
> > +   if (i < 2)
> > +   vcdt |= vcdt_part << ((i % 2) * 16);
> > +   else
> > +   vcdt2 |= vcdt_part << ((i % 2) * 16);
> > +   }
> > +
> > +   switch (priv->lanes) {
> > +   case 1:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> > +   break;
> > +   case 2:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +   break;
> > +   case 4:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
> > +   PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +   break;
> 
> Even simpler this could be written as
> 
> phycnt = PHYCNT_ENABLECLK | (1 << priv->lanes) - 1;

Fixed per your and Geert's suggestion.

> 
> > +   default:
> > +   return -EINVAL;
> 
> Can this happen? You have validated priv->lanes already when parsing
> DT

This can't happen but I like to have a catch all in any case, but since 
I took yours and Geert's suggestion above this issue goes away :-)

> 
> > +   }
> > +
> > +   ret = rcar_csi2_calc_phypll(priv, format->bpp, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   /* Clear Ultra Low Power interrupt */
> > +   if (priv->info->clear_ulps)
> > +   rcar_csi2_write(priv, INTSTATE_REG,
> > +   INTSTATE_INT_ULPS_START |
> > +   INTSTATE_INT_ULPS_END);
> > +
> > +   /* Init */
> > +   rcar_csi2_write(priv, TREF_REG, TREF_TREF);
> > +   rcar_csi2_reset(priv);
> > +   rcar_csi2_write(priv, PHTC_REG, 0);
> > +
> > +   /* Configure */
> > +   rcar_csi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> > +   FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> 
> On the FLD_FLD_NUM(2) mask. Why 2?
> I read on the datasheet "the register must not be changed from default
> value" and I read defaul to be 0x

This is based on feedback from Renesas. The register is not properly 
documented. I'm working on improving it but for now I would like to keep 
it as FLD_FLD_NUM(2) and make a neater and documented fix in a follow up 
commit. In short it 

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

I have addressed all your comment's but one for the next version. Please 
indicate if you are fine with this and I can still keep your review tag 
:-)

On 2018-04-04 18:15:16 +0300, Laurent Pinchart wrote:

[snip]

> > +static int rcar_csi2_start(struct rcar_csi2 *priv)
> > +{
> > +   const struct rcar_csi2_format *format;
> > +   u32 phycnt, phypll, vcdt = 0, vcdt2 = 0;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> > +   priv->mf.width, priv->mf.height,
> > +   priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> > +
> > +   /* Code is validated in set_fmt */
> > +   format = rcar_csi2_code_to_fmt(priv->mf.code);
> 
> You could store the format pointer iin the rcar_csi2 structure to avoid 
> looking it up here.

I could do that, but then I would duplicate the storage of the code 
between the two cached values priv->mf and priv->. I could find that acceptable but if you don't strongly 
disagree I would prefer to keep the current way of looking it up here 
:-)

[snip]

> > +static int rcar_csi2_probe_resources(struct rcar_csi2 *priv,
> > +struct platform_device *pdev)
> > +{
> > +   struct resource *res;
> > +   int irq;
> > +
> > +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +   priv->base = devm_ioremap_resource(>dev, res);
> > +   if (IS_ERR(priv->base))
> > +   return PTR_ERR(priv->base);
> > +
> > +   irq = platform_get_irq(pdev, 0);
> > +   if (irq < 0)
> > +   return irq;
> 
> You don't seem to use the IRQ. Is this meant to catch invalid DT that don't 
> specify an IRQ, to make sure we'll always have one available when we'll need 
> to later ?

Yes, as you deducted this is currently only to catch invalid DT. In the 
DT documentation I list it as a mandatory property. I think there might 
be potential use-case to at some point add interrupt support of for the 
some of the error interrupts which can be enabled, specially now when we 
seen similar patches for VIN floating around.

> > +
> > +   return 0;
> > +
> > +error:
> > +   v4l2_async_notifier_unregister(>notifier);
> > +   v4l2_async_notifier_cleanup(>notifier);
> > +
> > +   return ret;
> > +}
> 
> [snip]
> 
> With these small issues fixed and Kieran's and Maxime's comments addressed as 
> you see fit,
> 
> Reviewed-by: Laurent Pinchart 

Thanks, I will hold of adding it until you indicate if you are OK with 
the one comment I'm not fully agreeing with you on.

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Sakari,

Thanks for your feedback.

On 2018-04-04 23:13:57 +0300, Sakari Ailus wrote:

[snip]

> > > + pm_runtime_enable(>dev);
> > 
> > Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the
> > device uninitialised at probe, and pm_runtime_get_sync will not
> > initialise it either if CONFIG_PM is not enabled. I guess you could
> > call your runtime_resume function unconditionally, and mark the device
> > as active in runtime_pm using pm_runtime_set_active.
> 
> There doesn't seem to be any runtime_resume function. Was there supposed
> to be one?

No there is not suppose to be one.

> 
> Assuming runtime PM would actually do something here, you might add
> pm_runtime_idle() to power the device off after probing.
> 
> I guess pm_runtime_set_active() should precede pm_runtime_enable().

The CSI-2 is in the always on power domain so the calls to 
pm_runtime_get_sync() and pm_runtime_put() are there in the s_stream() 
callback to enable and disable the module clock. I'm no expert on PM but 
in my testing the pm_ calls in this driver seems to be correct.

1. In probe I call pm_runtime_enable(). And rudimentary tests shows the 
   clock is off (but I might miss something) as I wish it to be until 
   stream on time.
2. In s_stream() I call pm_runtime_get_sync() before writing any 
   register when starting a stream. And likewise I call pm_runtime_put() 
   when stopping and I no longer need to write to a register.
3. In remove() I call pm_runtime_disable().

Am I missing something obvious here?

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Geert and Laurent,

Thanks for the feedback.

On 2018-04-05 11:26:45 +0300, Laurent Pinchart wrote:

[snip]

> > Alternatively, you could check for a valid number of lanes, and use
> > knowledge about the internal lane bits:
> > 
> > phycnt = PHYCNT_ENABLECLK;
> > phycnt |= (1 << priv->lanes) - 1;
> 
> If Niklas is fine with that, I like it too.

I'm fine what that thanks for the suggestion Geert!

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Maxime,

Thanks for your feedback.

On 2018-03-29 13:30:39 +0200, Maxime Ripard wrote:
> Hi Niklas,
> 
> On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
> > +   switch (priv->lanes) {
> > +   case 1:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> > +   break;
> > +   case 2:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +   break;
> > +   case 4:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
> > +   PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +   break;
> > +   default:
> > +   return -EINVAL;
> > +   }
> 
> I guess you could have a simpler construct here using this:
> 
> phycnt = PHYCNT_ENABLECLK;
> 
> switch (priv->lanes) {
> case 4:
>   phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2;
> case 2:
>   phycnt |= PHYCNT_ENABLE_1;
> case 1:
>   phycnt |= PHYCNT_ENABLE_0;
>   break;
> 
> default:
>   return -EINVAL;
> }
> 
> But that's really up to you.

Thanks for the suggestion and sparking of the discussion, I think I will 
go with Geert at.al approach of:

phycnt = PHYCNT_ENABLECLK;
phycnt |= (1 << priv->lanes) - 1;

> 
> > +static int rcar_csi2_probe(struct platform_device *pdev)
> > +{
> > +   const struct soc_device_attribute *attr;
> > +   struct rcar_csi2 *priv;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
> > +   if (!priv)
> > +   return -ENOMEM;
> > +
> > +   priv->info = of_device_get_match_data(>dev);
> > +
> > +   /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */
> > +   attr = soc_device_match(r8a7795es1);
> > +   if (attr)
> > +   priv->info = attr->data;
> > +
> > +   priv->dev = >dev;
> > +
> > +   mutex_init(>lock);
> > +   priv->stream_count = 0;
> > +
> > +   ret = rcar_csi2_probe_resources(priv, pdev);
> > +   if (ret) {
> > +   dev_err(priv->dev, "Failed to get resources\n");
> > +   return ret;
> > +   }
> > +
> > +   platform_set_drvdata(pdev, priv);
> > +
> > +   ret = rcar_csi2_parse_dt(priv);
> > +   if (ret)
> > +   return ret;
> > +
> > +   priv->subdev.owner = THIS_MODULE;
> > +   priv->subdev.dev = >dev;
> > +   v4l2_subdev_init(>subdev, _csi2_subdev_ops);
> > +   v4l2_set_subdevdata(>subdev, >dev);
> > +   snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> > +KBUILD_MODNAME, dev_name(>dev));
> > +   priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +
> > +   priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > +   priv->subdev.entity.ops = _csi2_entity_ops;
> > +
> > +   priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
> > +   for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++)
> > +   priv->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > +
> > +   ret = media_entity_pads_init(>subdev.entity, NR_OF_RCAR_CSI2_PAD,
> > +priv->pads);
> > +   if (ret)
> > +   goto error;
> > +
> > +   pm_runtime_enable(>dev);
> 
> Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the
> device uninitialised at probe, and pm_runtime_get_sync will not
> initialise it either if CONFIG_PM is not enabled. I guess you could
> call your runtime_resume function unconditionally, and mark the device
> as active in runtime_pm using pm_runtime_set_active.

Yes CONFIG_PM is selected by ARCH_RENESAS. Thanks for letting me know 
about this in any case I was not aware of this behavior in the case 
CONFIG_PM where not enabled.

> 
> Looks good otherwise, once fixed (and if relevant):
> Reviewed-by: Maxime Ripard 

Thanks!

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-04-04 18:25:23 +0300, Laurent Pinchart wrote:

[snip]

> > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644
> > > index ..c0c2a763151bc928
> > > --- /dev/null
> > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > @@ -0,0 +1,884 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> 
> Do you intend making it GPL-2.0+ or did you mean GPL-2.0 ?

Wops I intended to make it GPL-2.0 thanks for catching this.

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Kieran,

Thanks for your feedback.

On 2018-03-13 23:23:49 +0100, Kieran Bingham wrote:
> Hi Niklas
> 
> Thank you for this patch ...
> I know it has been a lot of work getting this and the VIN together!
> 
> On 13/02/18 00:01, Niklas Söderlund wrote:
> > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> > connected between the video sources and the video grabbers (VIN).
> > 
> > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> > 
> > Signed-off-by: Niklas Söderlund 
> > Reviewed-by: Hans Verkuil 
> 
> I don't think there's actually anything major in the below - so with any
> comments addressed, or specifically ignored you can add my:
> 
> Reviewed-by: Kieran Bingham 

Thanks, see bellow for answers to your questions. I have removed the 
questions Laurent already have provided a reply for, thanks for doing 
that!

> 
> tag if you like.
> 
> 
> > ---
> >  drivers/media/platform/rcar-vin/Kconfig |  12 +
> >  drivers/media/platform/rcar-vin/Makefile|   1 +
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 884 
> > 
> >  3 files changed, 897 insertions(+)
> >  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
> > 
> > diff --git a/drivers/media/platform/rcar-vin/Kconfig 
> > b/drivers/media/platform/rcar-vin/Kconfig
> > index af4c98b44d2e22cb..6875f30c1ae42631 100644
> > --- a/drivers/media/platform/rcar-vin/Kconfig
> > +++ b/drivers/media/platform/rcar-vin/Kconfig
> > @@ -1,3 +1,15 @@
> > +config VIDEO_RCAR_CSI2
> > +   tristate "R-Car MIPI CSI-2 Receiver"
> > +   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF
> 
> I think I recall recently seeing that depending upon OF is redundant and not
> necessary (though I think that's minor in this instance)

I can't seem to find anything to indicate this, but as you state if 
there is such a transition ongoing we can delete this later IMHO.

$ find . -name Kconfig -exec grep OF {} \; | grep "depends on" | wc -l
622

But I'm happy to be proven wrong and remove it now as well.

[snip]

> > +static const struct rcar_csi2_format 
> > *rcar_csi2_code_to_fmt(unsigned int code)
> > +{
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> > +   if (rcar_csi2_formats[i].code == code)
> > +   return rcar_csi2_formats + i;
> 
> I would have written this as:
>   return _csi2_formats[i];  but I think your way is valid too :)

This seems to be the popular opinion among reviewers so I will adopt 
this style for the next version :-)


> 
> I have a nice 'for_each_entity_in_array' macro I keep meaning to post which
> would be nice here too - but hey - for another day.

Looking forward to it.

[snip]

> > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> > +{
> > +   int timeout;
> > +
> > +   /* Wait for the clock and data lanes to enter LP-11 state. */
> > +   for (timeout = 100; timeout > 0; timeout--) {
> > +   const u32 lane_mask = (1 << priv->lanes) - 1;
> > +
> > +   if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 &&
> 
> The '1' is not very clear here.. Could this be:
> 
>   if ((rcar_csi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATE) &&

Great catch I have clearly missed this define. I will call it 
PHCLM_STOPSTATECLK since that it's what the latest datasheet I have 
calls it. Thanks for pointing this out.

[snip]

> > +static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > +   struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +   struct v4l2_subdev *nextsd;
> > +   int ret = 0;
> > +
> > +   mutex_lock(>lock);
> > +
> > +   if (!priv->remote) {
> > +   ret = -ENODEV;
> > +   goto out;
> > +   }
> > +
> > +   nextsd = priv->remote;
> > +
> > +   if (enable && priv->stream_count == 0) {
> > +   pm_runtime_get_sync(priv->dev);
> > +
> > +   ret = rcar_csi2_start(priv);
> > +   if (ret) {
> > +   pm_runtime_put(priv->dev);
> > +   goto out;
> > +   }
> > +
> > +   ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
> 
> Would it be nicer to pass 'enable' through instead of the '1'? (of course
> enable==1 here)
> 
> > +   if (ret) {
> > +   rcar_csi2_stop(priv);
> > +   pm_runtime_put(priv->dev);
> > +   goto out;
> > +   }
> > +   } else if (!enable && priv->stream_count == 1) {
> > +   rcar_csi2_stop(priv);
> > +   v4l2_subdev_call(nextsd, video, s_stream, 0);
> 
> likewise here...
> 

I agree with Laurent's comment here that it's much clearer to use 1/0 
instead of 'enable' when it comes to read the code and will keep it as 
such :-)

> > +   pm_runtime_put(priv->dev);
> > +   }
> 
> What's the 'nextsd' in this 

cx88 invalid video opcodes when VBI enabled

[Devin Heitmueller]

Hello,

I received a report of a case where cx88 video capture was failing on
start and the dmesg was reporting an invalid opcode on the video IRQ
handler.  I did a bisect, and it looks like it's a result of the
videobuf2 conversion done back in late 2014.  A few notes:

1.  It only seems to occur if both video and VBI are being used.  I
cannot reproduce the issue when doing video only.

2.  It seems like it's some sort of order-of-operations issue when
interacting with the video/vbi device nodes, since it only happens
with the stock VLC and not tvtime.  It could also be that VLC uses
libzvbi, which access the VBI device in mmap mode, whereas tvtime
still uses read() for VBI access.

3.  The problem is readily reproducible on a stock Ubuntu 14.04
system, as well as with 16.04, using the stock VLC that ships with the
distro.  I'm testing with the following command line:

vlc v4l:///dev/video0 :v4l2-vbi-dev=/dev/vbi0

Sometimes it doesn't happen on the first hit and you have to run it a
few times, but I've never seen it take more than 5 executions for the
failure to occur.

4.  The problem occurs even when I blacklist the cx88-alsa driver.
This is worth noting since cx88-alsa has a different issue that
results in dumping out the RISC engine state, and I wanted to both
ensure the two issues weren't confused for each other, nor that the
ALSA interaction could be impacting the order of operations for
interacting with the driver.

Any suggestions on the best way to debug this without having to learn
the intimate details of the RISC engine on the cx88?  From the state
of the RISC engine it looks like there is some issue with queuing the
opcodes/arguments (where in some cases arguments are interpreted as
opcodes), but this is certainly not my area of expertise.

Thanks,

Devin

[   54.427224] cx88[0]: irq vid [0x10088] vbi_risc1* vbi_risc2* opc_err*
[   54.427232] cx88[0]/0: video risc op code error
[   54.427238] cx88[0]: video y / packed - dma channel status dump
[   54.427241] cx88[0]:   cmds: initial risc: 0x87cdb000
[   54.427244] cx88[0]:   cmds: cdt base: 0x00180440
[   54.427247] cx88[0]:   cmds: cdt size: 0x000c
[   54.427249] cx88[0]:   cmds: iq base : 0x00180400
[   54.427251] cx88[0]:   cmds: iq size : 0x0010
[   54.427253] cx88[0]:   cmds: risc pc : 0x87cdb03c
[   54.427256] cx88[0]:   cmds: iq wr ptr   : 0x010e
[   54.427258] cx88[0]:   cmds: iq rd ptr   : 0x0102
[   54.427260] cx88[0]:   cmds: cdt current : 0x0458
[   54.427263] cx88[0]:   cmds: pci target  : 0x
[   54.427265] cx88[0]:   cmds: line / byte : 0x
[   54.427267] cx88[0]:   risc0: 0x80008000 [ sync resync count=0 ]
[   54.427271] cx88[0]:   risc1: 0x1c000280 [ write sol eol count=640 ]
[   54.427276] cx88[0]:   risc2: 0x87dc [ arg #1 ]
[   54.427279] cx88[0]:   risc3: 0x1c000280 [ write sol eol count=640 ]
[   54.427283] cx88[0]:   iq 0: 0x7000 [ jump count=0 ]
[   54.427286] cx88[0]:   iq 1: 0x80008000 [ arg #1 ]
[   54.427289] cx88[0]:   iq 2: 0x1c000280 [ write sol eol count=640 ]
[   54.427293] cx88[0]:   iq 3: 0x87dc [ arg #1 ]
[   54.427295] cx88[0]:   iq 4: 0x1c000280 [ write sol eol count=640 ]
[   54.427300] cx88[0]:   iq 5: 0x87dc0500 [ arg #1 ]
[   54.427302] cx88[0]:   iq 6: 0x1c000280 [ write sol eol count=640 ]
[   54.427306] cx88[0]:   iq 7: 0x87dc0a00 [ arg #1 ]
[   54.427309] cx88[0]:   iq 8: 0x1c000280 [ write sol eol count=640 ]
[   54.427313] cx88[0]:   iq 9: 0x87dc0f00 [ arg #1 ]
[   54.427315] cx88[0]:   iq a: 0x1c000280 [ write sol eol count=640 ]
[   54.427319] cx88[0]:   iq b: 0x87dc1400 [ arg #1 ]
[   54.427321] cx88[0]:   iq c: 0x1c000280 [ write sol eol count=640 ]
[   54.427326] cx88[0]:   iq d: 0x87dc1900 [ arg #1 ]
[   54.427328] cx88[0]:   iq e: 0xd201 [ writecr irq2 count=1 ]
[   54.427332] cx88[0]:   iq f: 0x0031c040 [ arg #1 ]
[   54.427334] cx88[0]:   iq 10: 0x00180c00 [ arg #2 ]
[   54.427337] cx88[0]:   iq 11: 0x6f60580c [ arg #3 ]
[   54.427339] cx88[0]: fifo: 0x00180c00 -> 0x183400
[   54.427340] cx88[0]: ctrl: 0x00180400 -> 0x180460
[   54.427342] cx88[0]:   ptr1_reg: 0x00180eb8
[   54.427344] cx88[0]:   ptr2_reg: 0x00180458
[   54.427347] cx88[0]:   cnt1_reg: 0x0007
[   54.427349] cx88[0]:   cnt2_reg: 0x


-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

[PATCH stable v4.15 0/3] lirc_zilog bugs

[Sean Young]

This driver has a few problems, however the driver has been removed from
staging in v4.16 (replaced by a new driver). Please can these patches
be included in the 4.15.* stable tree.

Thanks

Sean Young (3):
  media: staging: lirc_zilog: broken reference counting
  Revert "media: lirc_zilog: driver only sends LIRCCODE"
  media: staging: lirc_zilog: incorrect reference counting

 drivers/staging/media/lirc/lirc_zilog.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
2.14.3

[PATCH stable v4.15 1/3] media: staging: lirc_zilog: broken reference counting

[Sean Young]

commit 615cd3fe6ccc ("[media] media: lirc_dev: make better use of
file->private_data") removed the reference get from open, so on the first
close the reference count hits zero and the lirc device is freed.

BUG: unable to handle kernel NULL pointer dereference at 0040
IP: lirc_thread+0x94/0x520 [lirc_zilog]
PGD 22d69c067 P4D 22d69c067 PUD 22d69d067 PMD 0
Oops:  [#1] SMP NOPTI
CPU: 2 PID: 701 Comm: zilog-rx-i2c-7 Tainted: P C OE
4.15.14-300.fc27.x86_64 #1
Hardware name: Gigabyte Technology Co., Ltd. GA-MA790FXT-UD5P/GA-MA790FXT-UD5P, 
BIOS F6 08/06/2009
RIP: 0010:lirc_thread+0x94/0x520 [lirc_zilog]
RSP: 0018:b482c131be98 EFLAGS: 00010246
RAX:  RBX: 8fdabf056000 RCX: 
RDX:  RSI: 0246 RDI: 0246
RBP: 8fdab740af00 R08: 8fdacfd214a0 R09: 
R10:  R11: 0040 R12: b482c10dba48
R13: 8fdabea89e00 R14: 8fdab740af00 R15: c0b5e500
FS:  () GS:8fdacfd0() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 0040 CR3: 0002124c CR4: 06e0
Call Trace:
 ? __schedule+0x247/0x880
 ? get_ir_tx+0x40/0x40 [lirc_zilog]
 kthread+0x113/0x130
 ? kthread_create_worker_on_cpu+0x70/0x70
 ? do_syscall_64+0x74/0x180
 ? SyS_exit_group+0x10/0x10
 ret_from_fork+0x22/0x40
Code: 20 8b 85 80 00 00 00 85 c0 0f 84 a6 00 00 00 bf 04 01 00 00 e8 ee 34 d4 
d7 e8 69 88 56 d7 84 c0 75 69 48 8b 45 18 c6 44 24 37 00 <48> 8b 58 40 4c 8d 6b 
18 4c 89 ef e8 fc 4d d4 d7 4c 89 ef 48 89
RIP: lirc_thread+0x94/0x520 [lirc_zilog] RSP: b482c131be98
CR2: 0040
This code has been replaced completely in kernel v4.16 by a new driver,
see commit acaa34bf06e9 ("media: rc: implement zilog transmitter"), and
commit f95367a7b758 ("media: staging: remove lirc_zilog driver").

Fixes: 615cd3fe6ccc ("[media] media: lirc_dev: make better use of 
file->private_data")

Cc: sta...@vger.kernel.org # v4.15
Reported-by: Warren Sturm 
Tested-by: Warren Sturm 
Signed-off-by: Sean Young 
---
 drivers/staging/media/lirc/lirc_zilog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index 6bd0717bf76e..bf6869e48a0f 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1291,6 +1291,7 @@ static int open(struct inode *node, struct file *filep)
 
lirc_init_pdata(node, filep);
ir = lirc_get_pdata(filep);
+   get_ir_device(ir, false);
 
atomic_inc(>open_count);
 
-- 
2.14.3

[PATCH stable v4.15 2/3] Revert "media: lirc_zilog: driver only sends LIRCCODE"

[Sean Young]

The lirc config documented here
https://www.blushingpenguin.com/mark/blog/?p=24 uses raw_codes for sending
IR. Each key only has one pulse, which in fact is an index into the
haup-ir-blaster.bin file. Changing the driver to LIRCCODE (although more
accurate) breaks this configuration.

This code has been replaced completely in kernel v4.16 by a new driver,
see commit acaa34bf06e9 ("media: rc: implement zilog transmitter"), and
commit f95367a7b758 ("media: staging: remove lirc_zilog driver").

This reverts commit 89d8a2cc51d1f29ea24a0b44dde13253141190a0.

Fixes: 615cd3fe6ccc ("[media] media: lirc_dev: make better use of 
file->private_data")

Cc: sta...@vger.kernel.org # v4.14-v4.15
Reported-by: Warren Sturm 
Tested-by: Warren Sturm 
Signed-off-by: Sean Young 
---
 drivers/staging/media/lirc/lirc_zilog.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index bf6869e48a0f..e8d6c1abc6d8 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -287,7 +287,7 @@ static void release_ir_tx(struct kref *ref)
struct IR_tx *tx = container_of(ref, struct IR_tx, ref);
struct IR *ir = tx->ir;
 
-   ir->l->features &= ~LIRC_CAN_SEND_LIRCCODE;
+   ir->l->features &= ~LIRC_CAN_SEND_PULSE;
/* Don't put_ir_device(tx->ir) here, so our lock doesn't get freed */
ir->tx = NULL;
kfree(tx);
@@ -1266,14 +1266,14 @@ static long ioctl(struct file *filep, unsigned int cmd, 
unsigned long arg)
if (!(features & LIRC_CAN_SEND_MASK))
return -ENOTTY;
 
-   result = put_user(LIRC_MODE_LIRCCODE, uptr);
+   result = put_user(LIRC_MODE_PULSE, uptr);
break;
case LIRC_SET_SEND_MODE:
if (!(features & LIRC_CAN_SEND_MASK))
return -ENOTTY;
 
result = get_user(mode, uptr);
-   if (!result && mode != LIRC_MODE_LIRCCODE)
+   if (!result && mode != LIRC_MODE_PULSE)
return -EINVAL;
break;
default:
@@ -1482,7 +1482,7 @@ static int ir_probe(struct i2c_client *client, const 
struct i2c_device_id *id)
kref_init(>ref);
ir->tx = tx;
 
-   ir->l->features |= LIRC_CAN_SEND_LIRCCODE;
+   ir->l->features |= LIRC_CAN_SEND_PULSE;
mutex_init(>client_lock);
tx->c = client;
tx->need_boot = 1;
-- 
2.14.3

[PATCH stable v4.15 3/3] media: staging: lirc_zilog: incorrect reference counting

[Sean Young]

Whenever poll is called, the reference count is increased but never
decreased. This means that on rmmod, the lirc_thread is not stopped,
and will trample over freed memory.

Zilog/Hauppauge IR driver unloaded
BUG: unable to handle kernel paging request at c17ba640
Oops: 0010 [#1] SMP
CPU: 1 PID: 667 Comm: zilog-rx-i2c-1 Tainted: P C OE   
4.13.16-302.fc27.x86_64 #1
Hardware name: Gigabyte Technology Co., Ltd. GA-MA790FXT-UD5P/GA-MA790FXT-UD5P, 
BIOS F6 08/06/2009
task: 964eb452ca00 task.stack: b254414dc000
RIP: 0010:0xc17ba640
RSP: 0018:b254414dfe78 EFLAGS: 00010286
RAX:  RBX: 964ec1b35890 RCX: 
RDX:  RSI: 0246 RDI: 0246
RBP: b254414dff00 R08: 036e R09: 964ecfc8dfd0
R10: b254414dfe78 R11: 000f4240 R12: 964ec2bf28a0
R13: 964ec1b358a8 R14: 964ec1b358d0 R15: 964ec1b35800
FS:  () GS:964ecfc8() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: c17ba640 CR3: 00023058c000 CR4: 06e0
Call Trace:
 kthread+0x125/0x140
 ? kthread_park+0x60/0x60
 ? do_syscall_64+0x67/0x140
 ret_from_fork+0x25/0x30
Code:  Bad RIP value.
RIP: 0xc17ba640 RSP: b254414dfe78
CR2: c17ba640

Note that zilog-rx-i2c-1 should have exited by now, but hasn't due to
the missing put in poll().

This code has been replaced completely in kernel v4.16 by a new driver,
see commit acaa34bf06e9 ("media: rc: implement zilog transmitter"), and
commit f95367a7b758 ("media: staging: remove lirc_zilog driver").

Cc: sta...@vger.kernel.org # v4.15- (all up to and including v4.15)
Reported-by: Warren Sturm 
Tested-by: Warren Sturm 
Signed-off-by: Sean Young 
---
 drivers/staging/media/lirc/lirc_zilog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index e8d6c1abc6d8..022720210f70 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1227,6 +1227,7 @@ static unsigned int poll(struct file *filep, poll_table 
*wait)
 
dev_dbg(ir->dev, "%s result = %s\n", __func__,
ret ? "POLLIN|POLLRDNORM" : "none");
+   put_ir_rx(rx, false);
return ret;
 }
 
-- 
2.14.3

[PATCH stable v4.14 0/2] lirc_zilog bugs

[Sean Young]

This driver has a few problems, however the driver has been removed from
staging in v4.16 (replaced by a new driver). Please can these patches
be included in the 4.14.* stable tree.

Sean Young (2):
  Revert "media: lirc_zilog: driver only sends LIRCCODE"
  media: staging: lirc_zilog: incorrect reference counting

 drivers/staging/media/lirc/lirc_zilog.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.14.3

[PATCH stable v4.14 1/2] Revert "media: lirc_zilog: driver only sends LIRCCODE"

[Sean Young]

The lirc config documented here
https://www.blushingpenguin.com/mark/blog/?p=24 uses raw_codes for sending
IR. Each key only has one pulse, which in fact is an index into the
haup-ir-blaster.bin file. Changing the driver to LIRCCODE (although more
accurate) breaks this configuration.

This code has been replaced completely in kernel v4.16 by a new driver,
see commit acaa34bf06e9 ("media: rc: implement zilog transmitter"), and
commit f95367a7b758 ("media: staging: remove lirc_zilog driver").

This reverts commit 89d8a2cc51d1f29ea24a0b44dde13253141190a0.

Fixes: 615cd3fe6ccc ("[media] media: lirc_dev: make better use of 
file->private_data")

Cc: sta...@vger.kernel.org # v4.14-v4.15
Reported-by: Warren Sturm 
Tested-by: Warren Sturm 
Signed-off-by: Sean Young 
---
 drivers/staging/media/lirc/lirc_zilog.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index 71af13bd0ebd..26dd32d5b5b2 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -288,7 +288,7 @@ static void release_ir_tx(struct kref *ref)
struct IR_tx *tx = container_of(ref, struct IR_tx, ref);
struct IR *ir = tx->ir;
 
-   ir->l.features &= ~LIRC_CAN_SEND_LIRCCODE;
+   ir->l.features &= ~LIRC_CAN_SEND_PULSE;
/* Don't put_ir_device(tx->ir) here, so our lock doesn't get freed */
ir->tx = NULL;
kfree(tx);
@@ -1267,14 +1267,14 @@ static long ioctl(struct file *filep, unsigned int cmd, 
unsigned long arg)
if (!(features & LIRC_CAN_SEND_MASK))
return -ENOTTY;
 
-   result = put_user(LIRC_MODE_LIRCCODE, uptr);
+   result = put_user(LIRC_MODE_PULSE, uptr);
break;
case LIRC_SET_SEND_MODE:
if (!(features & LIRC_CAN_SEND_MASK))
return -ENOTTY;
 
result = get_user(mode, uptr);
-   if (!result && mode != LIRC_MODE_LIRCCODE)
+   if (!result && mode != LIRC_MODE_PULSE)
return -EINVAL;
break;
default:
@@ -1512,7 +1512,7 @@ static int ir_probe(struct i2c_client *client, const 
struct i2c_device_id *id)
kref_init(>ref);
ir->tx = tx;
 
-   ir->l.features |= LIRC_CAN_SEND_LIRCCODE;
+   ir->l.features |= LIRC_CAN_SEND_PULSE;
mutex_init(>client_lock);
tx->c = client;
tx->need_boot = 1;
-- 
2.14.3

[PATCH stable v4.14 2/2] media: staging: lirc_zilog: incorrect reference counting

[Sean Young]

Whenever poll is called, the reference count is increased but never
decreased. This means that on rmmod, the lirc_thread is not stopped,
and will trample over freed memory.

Zilog/Hauppauge IR driver unloaded
BUG: unable to handle kernel paging request at c17ba640
Oops: 0010 [#1] SMP
CPU: 1 PID: 667 Comm: zilog-rx-i2c-1 Tainted: P C OE   
4.13.16-302.fc27.x86_64 #1
Hardware name: Gigabyte Technology Co., Ltd. GA-MA790FXT-UD5P/GA-MA790FXT-UD5P, 
BIOS F6 08/06/2009
task: 964eb452ca00 task.stack: b254414dc000
RIP: 0010:0xc17ba640
RSP: 0018:b254414dfe78 EFLAGS: 00010286
RAX:  RBX: 964ec1b35890 RCX: 
RDX:  RSI: 0246 RDI: 0246
RBP: b254414dff00 R08: 036e R09: 964ecfc8dfd0
R10: b254414dfe78 R11: 000f4240 R12: 964ec2bf28a0
R13: 964ec1b358a8 R14: 964ec1b358d0 R15: 964ec1b35800
FS:  () GS:964ecfc8() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: c17ba640 CR3: 00023058c000 CR4: 06e0
Call Trace:
 kthread+0x125/0x140
 ? kthread_park+0x60/0x60
 ? do_syscall_64+0x67/0x140
 ret_from_fork+0x25/0x30
Code:  Bad RIP value.
RIP: 0xc17ba640 RSP: b254414dfe78
CR2: c17ba640

Note that zilog-rx-i2c-1 should have exited by now, but hasn't due to
the missing put in poll().

This code has been replaced completely in kernel v4.16 by a new driver,
see commit acaa34bf06e9 ("media: rc: implement zilog transmitter"), and
commit f95367a7b758 ("media: staging: remove lirc_zilog driver").

Cc: sta...@vger.kernel.org # v4.15- (all up to and including v4.15)
Reported-by: Warren Sturm 
Tested-by: Warren Sturm 
Signed-off-by: Sean Young 
---
 drivers/staging/media/lirc/lirc_zilog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index 26dd32d5b5b2..e35e1b2160e3 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1228,6 +1228,7 @@ static unsigned int poll(struct file *filep, poll_table 
*wait)
 
dev_dbg(ir->l.dev, "%s result = %s\n", __func__,
ret ? "POLLIN|POLLRDNORM" : "none");
+   put_ir_rx(rx, false);
return ret;
 }
 
-- 
2.14.3