[PATCH v5] [DVB][FRONTEND] Modified the code related to FE_SET_PROPERTY ioctl

[Satendra Singh Thakur]

-Since all the properties in the func dtv_property_process_set
 are 4 bytes, We have added 2 new arguments u32 cmd and u32 data;
 in place of older argument struct dtv_property *tvp.
-We have removed property validation in function
 dtv_property_process_set because most of the frontend drivers in
 linux source are not using the method ops.set_property.
-In place of dtv_property_dump, we have added own logic
 to dump cmd and data of the incoming property
-We have also re-named dtv_property_dump to dtv_get_property_dump.
-We have modified logs in this func to be suitable only for getting
 properties.
Signed-off-by: Satendra Singh Thakur 
---
 drivers/media/dvb-core/dvb_frontend.c | 135 ++
 1 file changed, 73 insertions(+), 62 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c 
b/drivers/media/dvb-core/dvb_frontend.c
index 2fcba16..013476e 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1092,22 +1092,19 @@ static struct dtv_cmds_h dtv_cmds[DTV_MAX_COMMAND + 1] 
= {
_DTV_CMD(DTV_STAT_TOTAL_BLOCK_COUNT, 0, 0),
 };
 
-static void dtv_property_dump(struct dvb_frontend *fe,
- bool is_set,
+static void dtv_get_property_dump(struct dvb_frontend *fe,
  struct dtv_property *tvp)
 {
int i;
 
if (tvp->cmd <= 0 || tvp->cmd > DTV_MAX_COMMAND) {
-   dev_warn(fe->dvb->device, "%s: %s tvp.cmd = 0x%08x undefined\n",
-   __func__,
-   is_set ? "SET" : "GET",
+   dev_warn(fe->dvb->device, "%s: GET tvp.cmd = 0x%08x undefined\n"
+   , __func__,
tvp->cmd);
return;
}
 
-   dev_dbg(fe->dvb->device, "%s: %s tvp.cmd= 0x%08x (%s)\n", __func__,
-   is_set ? "SET" : "GET",
+   dev_dbg(fe->dvb->device, "%s: GET tvp.cmd= 0x%08x (%s)\n", __func__,
tvp->cmd,
dtv_cmds[tvp->cmd].name);
 
@@ -1526,7 +1523,7 @@ static int dtv_property_process_get(struct dvb_frontend 
*fe,
return r;
}
 
-   dtv_property_dump(fe, false, tvp);
+   dtv_get_property_dump(fe, tvp);
 
return 0;
 }
@@ -1749,23 +1746,35 @@ static int dvbv3_set_delivery_system(struct 
dvb_frontend *fe)
return emulate_delivery_system(fe, delsys);
 }
 
+/**
+ * dtv_property_process_set -  Sets a single DTV property
+ * @fe:Pointer to  dvb_frontend
+ * @file:  Pointer to  file
+ * @cmd:   Digital TV command
+ * @data:  An unsigned 32-bits number
+ *
+ * This routine assigns the property
+ * value to the corresponding member of
+ *  dtv_frontend_properties
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
 static int dtv_property_process_set(struct dvb_frontend *fe,
-   struct dtv_property *tvp,
-   struct file *file)
+   struct file *file,
+   u32 cmd, u32 data)
 {
int r = 0;
struct dtv_frontend_properties *c = >dtv_property_cache;
-
-   /* Allow the frontend to validate incoming properties */
-   if (fe->ops.set_property) {
-   r = fe->ops.set_property(fe, tvp);
-   if (r < 0)
-   return r;
-   }
-
-   dtv_property_dump(fe, true, tvp);
-
-   switch(tvp->cmd) {
+   /** Dump DTV command name and value*/
+   if (!cmd || cmd > DTV_MAX_COMMAND)
+   dev_warn(fe->dvb->device, "%s: SET cmd 0x%08x undefined\n",
+__func__, cmd);
+   else
+   dev_dbg(fe->dvb->device,
+   "%s: SET cmd 0x%08x (%s) to 0x%08x\n",
+   __func__, cmd, dtv_cmds[cmd].name, data);
+   switch (cmd) {
case DTV_CLEAR:
/*
 * Reset a cache of data specific to the frontend here. This 
does
@@ -1778,140 +1787,140 @@ static int dtv_property_process_set(struct 
dvb_frontend *fe,
 * tunerequest so we can pass validation in the FE_SET_FRONTEND
 * ioctl.
 */
-   c->state = tvp->cmd;
+   c->state = cmd;
dev_dbg(fe->dvb->device, "%s: Finalised property cache\n",
__func__);
 
r = dtv_set_frontend(fe);
break;
case DTV_FREQUENCY:
-   c->frequency = tvp->u.data;
+   c->frequency = data;
break;
case DTV_MODULATION:
-   c->modulation = tvp->u.data;
+   c->modulation = data;
break;
case DTV_BANDWIDTH_HZ:
-   c->bandwidth_hz = tvp->u.data;
+   c->bandwidth_hz = data;

cron job: media_tree daily build: WARNINGS

[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:   Wed Sep 20 05:00:18 CEST 2017
media-tree git hash:1efdf1776e2253b77413c997bed862410e4b6aaf
media_build git hash:   19087750b61fc0c5528e798c47ff845f9234
v4l-utils git hash: 9ee29df352dad950784f0f6f4a1bb96c0aefacc4
gcc version:i686-linux-gcc (GCC) 7.1.0
sparse version: v0.5.0
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.12.0-164

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-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: OK
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: WARNINGS
linux-3.0.60-i686: WARNINGS
linux-3.1.10-i686: WARNINGS
linux-3.2.37-i686: WARNINGS
linux-3.3.8-i686: WARNINGS
linux-3.4.27-i686: WARNINGS
linux-3.5.7-i686: WARNINGS
linux-3.6.11-i686: WARNINGS
linux-3.7.4-i686: WARNINGS
linux-3.8-i686: WARNINGS
linux-3.9.2-i686: WARNINGS
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: WARNINGS
linux-3.12.67-i686: WARNINGS
linux-3.13.11-i686: WARNINGS
linux-3.14.9-i686: WARNINGS
linux-3.15.2-i686: WARNINGS
linux-3.16.7-i686: WARNINGS
linux-3.17.8-i686: WARNINGS
linux-3.18.7-i686: WARNINGS
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-4.13-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: WARNINGS
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: WARNINGS
linux-3.15.2-x86_64: WARNINGS
linux-3.16.7-x86_64: WARNINGS
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
linux-4.13-x86_64: OK
apps: OK
spec-git: OK

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

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

[PATCH] ov9655: fix potential integer overflow

[Gustavo A. R. Silva]

Add suffix ULL to constant 1 rather than casting the result of the
operation in order to avoid a potential integer overflow. This constant
is used in a context that expects an expression of type u64.

Addresses-Coverity-ID: 1324146
Signed-off-by: Gustavo A. R. Silva 
---
 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 6ffb460..91a09ee 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -1129,8 +1129,8 @@ static int __ov965x_set_frame_interval(struct ov965x 
*ov965x,
if (fi->interval.denominator == 0)
return -EINVAL;
 
-   req_int = (u64)(fi->interval.numerator * 1) /
-   fi->interval.denominator;
+   req_int = (fi->interval.numerator * 1ULL) /
+ fi->interval.denominator;
 
for (i = 0; i < ARRAY_SIZE(ov965x_intervals); i++) {
const struct ov965x_interval *iv = _intervals[i];
-- 
2.7.4

[PATCH] siano: fix a potential integer overflow

[Gustavo A. R. Silva]

Add suffix ULL to constant 65535 in order to avoid a potential
integer overflow. This constant is used in a context that
expects an expression of type u64.

Addresses-Coverity-ID: 1056806
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/media/common/siano/smsdvb-main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/common/siano/smsdvb-main.c 
b/drivers/media/common/siano/smsdvb-main.c
index affde14..166428c 100644
--- a/drivers/media/common/siano/smsdvb-main.c
+++ b/drivers/media/common/siano/smsdvb-main.c
@@ -271,7 +271,7 @@ static void smsdvb_update_per_slices(struct smsdvb_client_t 
*client,
c->post_bit_count.stat[0].uvalue += p->ber_bit_count;
 
/* Legacy PER/BER */
-   tmp = p->ets_packets * 65535;
+   tmp = p->ets_packets * 65535ULL;
if (p->ts_packets + p->ets_packets)
do_div(tmp, p->ts_packets + p->ets_packets);
client->legacy_per = tmp;
-- 
2.7.4

Re: [RESEND PATCH v2 4/6] dt: bindings: as3645a: Improve label documentation, DT example

[Jacek Anaszewski]

Hi Pavel,

On 09/18/2017 10:54 PM, Pavel Machek wrote:
> On Mon 2017-09-18 17:49:23, Sakari Ailus wrote:
>> Hi Pavel,
>>
>> On Mon, Sep 18, 2017 at 12:56:55PM +0200, Pavel Machek wrote:
>>> Hi!
>>>
 Specify the exact label used if the label property is omitted in DT, as
 well as use label in the example that conforms to LED device naming.

 @@ -69,11 +73,11 @@ Example
flash-max-microamp = <32>;
led-max-microamp = <6>;
ams,input-max-microamp = <175>;
 -  label = "as3645a:flash";
 +  label = "as3645a:white:flash";
};
indicator@1 {
reg = <0x1>;
led-max-microamp = <1>;
 -  label = "as3645a:indicator";
 +  label = "as3645a:red:indicator";
};
};
>>>
>>> Ok, but userspace still has no chance to determine if this is flash
>>> from main camera or flash for front camera; todays smartphones have
>>> flashes on both cameras.
>>>
>>> So.. Can I suggset as3645a:white:main_camera_flash or main_flash or
>>> ?
>>
>> If there's just a single one in the device, could you use that?
>>
>> Even if we name this so for N9 (and N900), the application still would only
>> work with the two devices.
> 
> Well, I'd plan to name it on other devices, too.
> 
>> My suggestion would be to look for a flash LED, and perhaps the maximum
>> current as well. That should generally work better than assumptions on the
>> label.
> 
> If you just look for flash LED, you don't know if it is front one or
> back one. Its true that if you have just one flash it is usually on
> the back camera, but you can't know if maybe driver is not available
> for the main flash.
> 
> Lets get this right, please "main_camera_flash" is 12 bytes more than
> "flash", and it saves application logic.. more than 12 bytes, I'm sure. 

What you are trying to introduce is yet another level of LED class
device naming standard, one level below devicename:colour:function.
It seems you want also to come up with the set of standarized LED
function names. This would certainly have to be covered for consistency.

-- 
Best regards,
Jacek Anaszewski

[PATCH] [media] staging: atomisp: use clock framework for camera clocks

[Pierre-Louis Bossart]

The Atom ISP driver initializes and configures PMC clocks which are
already handled by the clock framework.

Remove all legacy vlv2_platform_clock stuff and move to the clk API to
avoid conflicts, e.g. with audio machine drivers enabling the MCLK for
external codecs

Tested-by: Carlo Caione 
Signed-off-by: Pierre-Louis Bossart 
---
 drivers/staging/media/atomisp/Kconfig  |   1 +
 drivers/staging/media/atomisp/platform/Makefile|   1 -
 .../staging/media/atomisp/platform/clock/Makefile  |   6 -
 .../platform/clock/platform_vlv2_plat_clk.c|  40 
 .../platform/clock/platform_vlv2_plat_clk.h|  27 ---
 .../media/atomisp/platform/clock/vlv2_plat_clock.c | 247 -
 .../platform/intel-mid/atomisp_gmin_platform.c |  63 +-
 7 files changed, 52 insertions(+), 333 deletions(-)
 delete mode 100644 drivers/staging/media/atomisp/platform/clock/Makefile
 delete mode 100644 
drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.c
 delete mode 100644 
drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.h
 delete mode 100644 
drivers/staging/media/atomisp/platform/clock/vlv2_plat_clock.c

diff --git a/drivers/staging/media/atomisp/Kconfig 
b/drivers/staging/media/atomisp/Kconfig
index 8eb13c3ba29c..7cdebea35ccf 100644
--- a/drivers/staging/media/atomisp/Kconfig
+++ b/drivers/staging/media/atomisp/Kconfig
@@ -1,6 +1,7 @@
 menuconfig INTEL_ATOMISP
 bool "Enable support to Intel MIPI camera drivers"
 depends on X86 && EFI && MEDIA_CONTROLLER && PCI && ACPI
+   select COMMON_CLK
 help
   Enable support for the Intel ISP2 camera interfaces and MIPI
   sensor drivers.
diff --git a/drivers/staging/media/atomisp/platform/Makefile 
b/drivers/staging/media/atomisp/platform/Makefile
index df157630bda9..0e3b7e1c81c6 100644
--- a/drivers/staging/media/atomisp/platform/Makefile
+++ b/drivers/staging/media/atomisp/platform/Makefile
@@ -2,5 +2,4 @@
 # Makefile for camera drivers.
 #
 
-obj-$(CONFIG_INTEL_ATOMISP) += clock/
 obj-$(CONFIG_INTEL_ATOMISP) += intel-mid/
diff --git a/drivers/staging/media/atomisp/platform/clock/Makefile 
b/drivers/staging/media/atomisp/platform/clock/Makefile
deleted file mode 100644
index 82fbe8b6968a..
diff --git 
a/drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.c 
b/drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.c
deleted file mode 100644
index 0aae9b0283bb..
diff --git 
a/drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.h 
b/drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.h
deleted file mode 100644
index b730ab0e8223..
diff --git a/drivers/staging/media/atomisp/platform/clock/vlv2_plat_clock.c 
b/drivers/staging/media/atomisp/platform/clock/vlv2_plat_clock.c
deleted file mode 100644
index f96789a31819..
diff --git 
a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c 
b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index edaae93af8f9..17b4cfae5abf 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -4,10 +4,10 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
-#include "../../include/linux/vlv2_plat_clock.h"
 #include 
 #include 
 #include 
@@ -17,11 +17,7 @@
 
 #define MAX_SUBDEVS 8
 
-/* Should be defined in vlv2_plat_clock API, isn't: */
-#define VLV2_CLK_PLL_19P2MHZ 1
-#define VLV2_CLK_XTAL_19P2MHZ 0
-#define VLV2_CLK_ON  1
-#define VLV2_CLK_OFF 2
+#define VLV2_CLK_PLL_19P2MHZ 1 /* XTAL on CHT */
 #define ELDO1_SEL_REG  0x19
 #define ELDO1_1P8V 0x16
 #define ELDO1_CTRL_SHIFT 0x00
@@ -33,6 +29,7 @@ struct gmin_subdev {
struct v4l2_subdev *subdev;
int clock_num;
int clock_src;
+   struct clk *pmc_clk;
struct gpio_desc *gpio0;
struct gpio_desc *gpio1;
struct regulator *v1p8_reg;
@@ -344,6 +341,9 @@ static int gmin_platform_deinit(void)
return 0;
 }
 
+#define GMIN_PMC_CLK_NAME 14 /* "pmc_plt_clk_[0..5]" */
+static char gmin_pmc_clk_name[GMIN_PMC_CLK_NAME];
+
 static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 {
int i, ret;
@@ -377,6 +377,37 @@ static struct gmin_subdev *gmin_subdev_add(struct 
v4l2_subdev *subdev)
gmin_subdevs[i].gpio0 = gpiod_get_index(dev, NULL, 0, GPIOD_OUT_LOW);
gmin_subdevs[i].gpio1 = gpiod_get_index(dev, NULL, 1, GPIOD_OUT_LOW);
 
+   /* get PMC clock with clock framework */
+   snprintf(gmin_pmc_clk_name,
+sizeof(gmin_pmc_clk_name),
+"%s_%d", "pmc_plt_clk", gmin_subdevs[i].clock_num);
+
+   gmin_subdevs[i].pmc_clk = devm_clk_get(dev, gmin_pmc_clk_name);
+   if (IS_ERR(gmin_subdevs[i].pmc_clk)) {
+   ret 

Re: [PATCH v10 20/24] dt: bindings: smiapp: Document lens-focus and flash properties

[Rob Herring]

On Mon, Sep 18, 2017 at 4:56 PM, Sakari Ailus
 wrote:
> Hi Rob,
>
>
> Rob Herring wrote:
>>
>> On Mon, Sep 11, 2017 at 11:00:04AM +0300, Sakari Ailus wrote:
>>>
>>> Document optional lens-focus and flash properties for the smiapp driver.
>>>
>>> Signed-off-by: Sakari Ailus 
>>> ---
>>>  Documentation/devicetree/bindings/media/i2c/nokia,smia.txt | 2 ++
>>>  1 file changed, 2 insertions(+)
>>
>>
>> Acked-by: Rob Herring 
>
>
> Thanks for the ack. There have been since a few iterations of the set, and
> the corresponding patch in v13 has minor changes to this:

My review script can't deal with subject changes...

> 
>
> Essentially "flash" was renamed to "flash-leds" as the current flash devices
> we have are all LEDs and the referencing assumes LED framework's ways to
> describe LEDs. The same change is present in the patch adding the property

So we're kind of creating a binding that mirrors the gpio bindings
(*-gpios) which is a bit of an oddball as all other bindings have gone
with a fixed property name and then a *-names property to name them.
The main downside to this form is a prefixed property name is harder
to parse and validate. So perhaps we should follow the more common
pattern, but we're not really describing a h/w connection just an
association. And now we also have the trigger source binding to
associate LEDs with device nodes, so perhaps that should be used here.
We shouldn't really have 2 ways to associate things in DT even if how
that gets handled in the kernel is different.

Rob

Re: [PATCH] Support HVR-1200 analog video as a clone of HVR-1500. Tested, composite and s-video inputs.

[Nigel Kettlewell]

[adding kernel mailing lists missed from my reply]

Thank you, yes I think I cribbed too much from the 1500. I think the 
tuner part is not necessary: I have no analog over-the-air signal so I 
cannot test it, hence I have removed the tuner element from the patch 
(below).


I have tested DVB-T which works fine. dmesg shows no errors (attached).

DISPLAY=xxx:0.0 vlc dvb-t://frequency=49800:bandwidth=8 
--dvb-adapter=0 --programs=8373



/usr/local/bin/v4l2-ctl --set-input 1
/usr/local/bin/v4l2-ctl -s 0x00f7
cat /dev/video0 > /tmp/svideo.raw

ffmpeg -f rawvideo -pix_fmt yuyv422 -r 25 -s:v 720x576 -i 
/tmp/svideo.raw -vcodec mpeg2video -vb 2000k -y /tmp/svideo.mpg



Revised patch:

---
 drivers/media/pci/cx23885/cx23885-cards.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/drivers/media/pci/cx23885/cx23885-cards.c 
b/drivers/media/pci/cx23885/cx23885-cards.c

index 0350f13..1b685f0 100644
--- a/drivers/media/pci/cx23885/cx23885-cards.c
+++ b/drivers/media/pci/cx23885/cx23885-cards.c
@@ -196,7 +196,22 @@ struct cx23885_board cx23885_boards[] = {
},
[CX23885_BOARD_HAUPPAUGE_HVR1200] = {
.name   = "Hauppauge WinTV-HVR1200",
+   .porta  = CX23885_ANALOG_VIDEO,
.portc  = CX23885_MPEG_DVB,
+   .input  = {{
+   .type   = CX23885_VMUX_COMPOSITE1,
+   .vmux   =   CX25840_VIN7_CH3 |
+   CX25840_VIN4_CH2 |
+   CX25840_VIN6_CH1,
+   .gpio0  = 0,
+   }, {
+   .type   = CX23885_VMUX_SVIDEO,
+   .vmux   =   CX25840_VIN7_CH3 |
+   CX25840_VIN4_CH2 |
+   CX25840_VIN8_CH1 |
+   CX25840_SVIDEO_ON,
+   .gpio0  = 0,
+   } },
},
[CX23885_BOARD_HAUPPAUGE_HVR1700] = {
.name   = "Hauppauge WinTV-HVR1700",
@@ -2260,6 +2275,7 @@ void cx23885_card_setup(struct cx23885_dev *dev)
case CX23885_BOARD_HAUPPAUGE_HVR1290:
case CX23885_BOARD_LEADTEK_WINFAST_PXTV1200:
case CX23885_BOARD_GOTVIEW_X5_3D_HYBRID:
+   case CX23885_BOARD_HAUPPAUGE_HVR1200:
case CX23885_BOARD_HAUPPAUGE_HVR1500:
case CX23885_BOARD_MPX885:
case CX23885_BOARD_MYGICA_X8507:
--
2.9.4


Nigel Kettlewell 
19 September 2017 12:49
Thank you, yes I think I cribbed too much from the 1500. I think the 
tuner part is not necessary: I have no analog over-the-air signal so I 
cannot test it, hence I have removed the tuner element from the patch 
(below).


I have tested DVB-T which works fine. dmesg shows no errors (attached).

DISPLAY=xxx:0.0 vlc dvb-t://frequency=49800:bandwidth=8 
--dvb-adapter=0 --programs=8373



/usr/local/bin/v4l2-ctl --set-input 1
/usr/local/bin/v4l2-ctl -s 0x00f7
cat /dev/video0 > /tmp/svideo.raw

ffmpeg -f rawvideo -pix_fmt yuyv422 -r 25 -s:v 720x576 -i 
/tmp/svideo.raw -vcodec mpeg2video -vb 2000k -y /tmp/svideo.mpg



Revised patch:

---
 drivers/media/pci/cx23885/cx23885-cards.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/drivers/media/pci/cx23885/cx23885-cards.c 
b/drivers/media/pci/cx23885/cx23885-cards.c

index 0350f13..1b685f0 100644
--- a/drivers/media/pci/cx23885/cx23885-cards.c
+++ b/drivers/media/pci/cx23885/cx23885-cards.c
@@ -196,7 +196,22 @@ struct cx23885_board cx23885_boards[] = {
},
[CX23885_BOARD_HAUPPAUGE_HVR1200] = {
.name   = "Hauppauge WinTV-HVR1200",
+   .porta  = CX23885_ANALOG_VIDEO,
.portc  = CX23885_MPEG_DVB,
+   .input  = {{
+   .type   = CX23885_VMUX_COMPOSITE1,
+   .vmux   =   CX25840_VIN7_CH3 |
+   CX25840_VIN4_CH2 |
+   CX25840_VIN6_CH1,
+   .gpio0  = 0,
+   }, {
+   .type   = CX23885_VMUX_SVIDEO,
+   .vmux   =   CX25840_VIN7_CH3 |
+   CX25840_VIN4_CH2 |
+   CX25840_VIN8_CH1 |
+   CX25840_SVIDEO_ON,
+   .gpio0  = 0,
+   } },
},
[CX23885_BOARD_HAUPPAUGE_HVR1700] = {
.name   = "Hauppauge WinTV-HVR1700",
@@ -2260,6 +2275,7 @@ void cx23885_card_setup(struct cx23885_dev *dev)
case CX23885_BOARD_HAUPPAUGE_HVR1290:
case CX23885_BOARD_LEADTEK_WINFAST_PXTV1200:
case CX23885_BOARD_GOTVIEW_X5_3D_HYBRID:
+   case CX23885_BOARD_HAUPPAUGE_HVR1200:
case CX23885_BOARD_HAUPPAUGE_HVR1500:
case 

[PATCH 3/3] [media] hdpvr: Return an error code only as a constant in hdpvr_alloc_buffers()

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 19 Sep 2017 19:32:36 +0200

* Return an error code without storing it in an intermediate variable.

* Delete the local variable "retval" which became unnecessary
  with this refactoring.

Signed-off-by: Markus Elfring 
---
 drivers/media/usb/hdpvr/hdpvr-video.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c 
b/drivers/media/usb/hdpvr/hdpvr-video.c
index f06efcd70c14..2b39834309d2 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -136,6 +136,5 @@ int hdpvr_free_buffers(struct hdpvr_device *dev)
 int hdpvr_alloc_buffers(struct hdpvr_device *dev, uint count)
 {
uint i;
-   int retval = -ENOMEM;
u8 *mem;
struct hdpvr_buffer *buf;
@@ -181,6 +180,6 @@ int hdpvr_alloc_buffers(struct hdpvr_device *dev, uint 
count)
kfree(buf);
 exit:
hdpvr_free_buffers(dev);
-   return retval;
+   return -ENOMEM;
 }
 
-- 
2.14.1

[PATCH 2/3] [media] hdpvr: Improve a size determination in hdpvr_alloc_buffers()

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 19 Sep 2017 19:27:53 +0200

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/media/usb/hdpvr/hdpvr-video.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c 
b/drivers/media/usb/hdpvr/hdpvr-video.c
index 657d910dfa1d..f06efcd70c14 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -145,6 +145,5 @@ int hdpvr_alloc_buffers(struct hdpvr_device *dev, uint 
count)
 "allocating %u buffers\n", count);
 
for (i = 0; i < count; i++) {
-
-   buf = kzalloc(sizeof(struct hdpvr_buffer), GFP_KERNEL);
+   buf = kzalloc(sizeof(*buf), GFP_KERNEL);
if (!buf)
-- 
2.14.1

[PATCH 1/3] [media] hdpvr: Delete three error messages for a failed memory allocation

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 19 Sep 2017 09:33:26 +0200

Omit extra messages for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/media/usb/hdpvr/hdpvr-core.c  | 8 ++--
 drivers/media/usb/hdpvr/hdpvr-video.c | 5 ++---
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-core.c 
b/drivers/media/usb/hdpvr/hdpvr-core.c
index dbe29c6c4d8b..f1c156814e10 100644
--- a/drivers/media/usb/hdpvr/hdpvr-core.c
+++ b/drivers/media/usb/hdpvr/hdpvr-core.c
@@ -282,6 +282,4 @@ static int hdpvr_probe(struct usb_interface *interface,
-   if (!dev) {
-   dev_err(>dev, "Out of memory\n");
+   if (!dev)
goto error;
-   }
 
/* init video transfer queues first of all */
@@ -302,6 +300,4 @@ static int hdpvr_probe(struct usb_interface *interface,
-   if (!dev->usbc_buf) {
-   v4l2_err(>v4l2_dev, "Out of memory\n");
+   if (!dev->usbc_buf)
goto error;
-   }
 
init_waitqueue_head(>wait_buffer);
diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c 
b/drivers/media/usb/hdpvr/hdpvr-video.c
index 7fb036d6a86e..657d910dfa1d 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -150,7 +150,6 @@ int hdpvr_alloc_buffers(struct hdpvr_device *dev, uint 
count)
-   if (!buf) {
-   v4l2_err(>v4l2_dev, "cannot allocate buffer\n");
+   if (!buf)
goto exit;
-   }
+
buf->dev = dev;
 
urb = usb_alloc_urb(0, GFP_KERNEL);
-- 
2.14.1

[PATCH 0/3] [media] HD PVR USB: Adjustments for two function implementations

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 19 Sep 2017 20:21:23 +0200

Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Delete three error messages for a failed memory allocation
  Improve a size determination in hdpvr_alloc_buffers()
  Return an error code only as a constant in hdpvr_alloc_buffers()

 drivers/media/usb/hdpvr/hdpvr-core.c  |  8 ++--
 drivers/media/usb/hdpvr/hdpvr-video.c | 11 ---
 2 files changed, 6 insertions(+), 13 deletions(-)

-- 
2.14.1

Re: [PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs

[Laurent Pinchart]

Hi Sakari,

On Tuesday, 19 September 2017 18:03:48 EEST Sakari Ailus wrote:
> On Tue, Sep 19, 2017 at 05:58:32PM +0300, Sakari Ailus wrote:
> >> This skips adding the notifier to the notifier_list. Won't this result
> >> in an oops when calling list_del(>list) in
> >> v4l2_async_notifier_unregister() ?
> > 
> > Good point. I'll add initialising the list head to the register function,
> > with an appropriate comment.
> 
> I'll set v4l2_dev NULL instead; no tricks with lists needed.

Shouldn't the notifier still be added to the notifier_list ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 14/25] v4l: async: Allow binding notifiers to sub-devices

[Laurent Pinchart]

Hi Sakari,

On Tuesday, 19 September 2017 18:17:32 EEST Sakari Ailus wrote:
> On Tue, Sep 19, 2017 at 04:52:29PM +0300, Laurent Pinchart wrote:
> > On Friday, 15 September 2017 17:17:13 EEST Sakari Ailus wrote:
> > > Registering a notifier has required the knowledge of struct v4l2_device
> >> for the reason that sub-devices generally are registered to the
> >> v4l2_device (as well as the media device, also available through
> >> v4l2_device).
> >> 
> >> This information is not available for sub-device drivers at probe time.
> >> 
> >> What this patch does is that it allows registering notifiers without
> >> having v4l2_device around. Instead the sub-device pointer is stored in
> >> the notifier. Once the sub-device of the driver that registered the
> >> notifier is registered, the notifier will gain the knowledge of the
> >> v4l2_device, and the binding of async sub-devices from the sub-device
> >> driver's notifier may proceed.
> >> 
> >> The root notifier's complete callback is only called when all sub-device
> >> notifiers are completed.
> > 
> > This is a bit hard to review, shouldn't it be split in two patches, one
> > that refactors the functions, and another one that allows binding
> > notifiers to subdevs ?
> > 
> >> Signed-off-by: Sakari Ailus 
> >> ---
> >> 
> >>  drivers/media/v4l2-core/v4l2-async.c | 218 -
> >>  include/media/v4l2-async.h   |  16 ++-
> >>  2 files changed, 203 insertions(+), 31 deletions(-)
> >> 
> >> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> >> b/drivers/media/v4l2-core/v4l2-async.c index 4be2f16af051..52fe22b9b6b4
> >> 100644
> >> --- a/drivers/media/v4l2-core/v4l2-async.c
> >> +++ b/drivers/media/v4l2-core/v4l2-async.c

[snip]

> >> +/* Unbind all sub-devices in the notifier tree. */
> >> +static void v4l2_async_notifier_unbind_all_subdevs(
> >> +  struct v4l2_async_notifier *notifier)
> >> +{
> >> +  struct v4l2_subdev *sd, *tmp;
> >> 
> >>list_for_each_entry_safe(sd, tmp, >done, async_list) {
> >> 
> >> +  struct v4l2_async_notifier *subdev_notifier =
> >> +  v4l2_async_find_subdev_notifier(sd);
> >> +
> >> +  if (subdev_notifier)
> >> +  v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
> >> +
> >>v4l2_async_cleanup(sd);
> >>
> >>v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
> >> -  }
> >> 
> >> -  mutex_unlock(_lock);
> >> +  list_del(>async_list);
> >> +  list_add(>async_list, _list);
> > 
> > How about list_move() ?
> 
> Yeah.
> 
> > This seems to be new code, and by the look of it, I wonder whether it
> > doesn't belong in the reprobing removal patch.
> 
> This is not related to re-probing. Here we're moving an async sub-device
> back to the global sub-device list when its notifier is going away.

In order to make the subdev bindable again when the notifier will be re-
registered. This wasn't needed before, as reprobing took care of that.

> >> +  }
> >> 
> >> +  notifier->parent = NULL;
> >> +  notifier->sd = NULL;
> >>notifier->v4l2_dev = NULL;
> >>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/4] ARM: dts: exynos: Add clean name of compatible.

[Krzysztof Kozlowski]

On Wed, Sep 13, 2017 at 08:41:53PM +0900, Hoegeun Kwon wrote:
> Exynos 5250 and 5420 have different hardware rotation limits. However,
> currently it uses only one compatible - "exynos5-gsc". Since we have
> to distinguish between these two, we add different compatible.
> 
> Signed-off-by: Hoegeun Kwon 
> ---
>  arch/arm/boot/dts/exynos5250.dtsi | 8 
>  arch/arm/boot/dts/exynos5420.dtsi | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 

Thanks, applied with modified subject.

Best regards,
Krzysztof

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Mauro Carvalho Chehab]

Em Tue, 19 Sep 2017 17:24:45 +0200
Philipp Zabel  escreveu:

> Hi Dave,
> 
> On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
> > The existing fixed value of 16 worked for UYVY 720P60 over
> > 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
> > 1080P60 needs 6 lanes at 594MHz).
> > It doesn't allow for lower resolutions to work as the FIFO
> > underflows.
> > 
> > Using a value of 300 works for all resolutions down to VGA60,
> > and the increase in frame delay is <4usecs for 1080P60 UYVY
> > (2.55usecs for RGB888).
> > 
> > Signed-off-by: Dave Stevenson   
> 
> Can we increase this to 320? This would also allow
> 720p60 at 594 Mbps / 4 lanes, according to the xls.

Hmm... if this is dependent on the resolution and frame rate, wouldn't
it be better to dynamically adjust it accordingly?

Regards,
Maur

Thanks,
Mauro

Re: [PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

[Laurent Pinchart]

Hi Sakari,

On Tuesday, 19 September 2017 17:50:49 EEST Sakari Ailus wrote:
> On Tue, Sep 19, 2017 at 03:43:48PM +0300, Laurent Pinchart wrote:
> > On Tuesday, 19 September 2017 15:13:11 EEST Sakari Ailus wrote:
> >> On Tue, Sep 19, 2017 at 03:01:14PM +0300, Laurent Pinchart wrote:
> >>> On Friday, 15 September 2017 17:17:10 EEST Sakari Ailus wrote:
>  Add three helper functions to call async operations callbacks.
>  Besides simplifying callbacks, this allows async notifiers to have no
>  ops set, i.e. it can be left NULL.
>  
>  Signed-off-by: Sakari Ailus 
>  Acked-by: Hans Verkuil 
>  ---
>  
>   drivers/media/v4l2-core/v4l2-async.c | 49 ++
>   include/media/v4l2-async.h   |  1 +
>   2 files changed, 37 insertions(+), 13 deletions(-)
>  
>  diff --git a/drivers/media/v4l2-core/v4l2-async.c
>  b/drivers/media/v4l2-core/v4l2-async.c index
>  7b2125b3d62f..c35d04b9122f
>  100644
>  --- a/drivers/media/v4l2-core/v4l2-async.c
>  +++ b/drivers/media/v4l2-core/v4l2-async.c
>  @@ -25,6 +25,34 @@
>  
>   #include 
>   #include 
>  
>  +static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier
>  *n,
>  +  struct v4l2_subdev *subdev,
>  +  struct v4l2_async_subdev *asd)
>  +{
>  +if (!n->ops || !n->ops->bound)
>  +return 0;
>  +
>  +return n->ops->bound(n, subdev, asd);
>  +}
>  +
>  +static void v4l2_async_notifier_call_unbind(struct
>  v4l2_async_notifier
>  *n,
>  +struct v4l2_subdev *subdev,
>  +struct v4l2_async_subdev 
>  *asd)
>  +{
>  +if (!n->ops || !n->ops->unbind)
>  +return;
>  +
>  +n->ops->unbind(n, subdev, asd);
>  +}
>  +
>  +static int v4l2_async_notifier_call_complete(struct
>  v4l2_async_notifier
>  *n)
>  +{
>  +if (!n->ops || !n->ops->complete)
>  +return 0;
>  +
>  +return n->ops->complete(n);
>  +}
>  +
> >>> 
> >>> Wouldn't it be enough to add a single v4l2_async_notifier_call() macro
> >>> ?
> >>> 
> >>> #define v4l2_async_notifier_call(n, op, args...) \
> >>> 
> >>>   ((n)->ops && (n)->ops->op ? (n)->ops->op(n, ##args) : 0)
> >> 
> >> I actually had that in an earlier version but I changed it based on
> >> review comments from Hans. A single macro isn't enough: some functions
> >> have int return type. I think the way it is now is nicer.
> > 
> > What bothers me there is the overhead of a function call.
> 
> Overhead... of a function call?
> 
> Do you really mean what you're saying? :-) The functions will be called a
> relatively small number of times during module loading / device probing.

That's why I haven't said it's a big deal :-) There's of course no need to 
optimize that if the tradeoff is large, but if all operations had the same 
return type a macro could have been useful (although in this very specific 
case I'm more concerned about code size than about CPU cycles).

> > By the way, what's the use case for ops being NULL ?
> 
> If a driver has no need for any of the callbacks, there's no benefit from
> having to set ops struct either. This applies to devices that are
> associated to the sensor, for instance.

So in that case the subdev notifier is only registered to delay the complete() 
callback until the flash and lens controllers are available, with the sensor 
itself having no need to access the flash and lens controllers ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 06/25] omap3isp: Use generic parser for parsing fwnode endpoints

[Laurent Pinchart]

Hi Sakari,

On Tuesday, 19 September 2017 17:47:22 EEST Sakari Ailus wrote:
> On Tue, Sep 19, 2017 at 03:46:18PM +0300, Laurent Pinchart wrote:
> > On Tuesday, 19 September 2017 15:43:26 EEST Sakari Ailus wrote:
> >> On Tue, Sep 19, 2017 at 02:40:29PM +0300, Laurent Pinchart wrote:
>  @@ -2256,7 +2210,9 @@ static int isp_probe(struct platform_device
>  *pdev)
>   if (ret)
>   return ret;
>  
>  -ret = isp_fwnodes_parse(>dev, >notifier);
>  +ret = v4l2_async_notifier_parse_fwnode_endpoints(
>  +>dev, >notifier, sizeof(struct 
>  isp_async_subdev),
>  +isp_fwnode_parse);
>   if (ret < 0)
> >>> 
> >>> The documentation in patch 05/25 states that
> >>> v4l2_async_notifier_release() should be called even if
> >>> v4l2_async_notifier_parse_fwnode_endpoints() fails. I don't think that's
> >>> needed here, so you might want to update the documentation (and possibly
> >>> the implementation of the function).
> >> 
> >> It is. If parsing fails, async sub-devices may have been already set up.
> >> This happens e.g. when the parsing fails after the first one has been
> >> successfully set up already.
> > 
> > But for v4l2_async_notifier_parse_fwnode_endpoints() we could clean up
> > internally when an error occurs. Otherwise you need to call
> > v4l2_async_notifier_release() here.
> 
> The functions that set up async sub-devices can be called multiple times
> (on separate references). This is quite alike setting up a control handler
> really, so I adopted the same pattern.
> 
> If there is a failure, how many async sub-devices should be cleaned up, if
> there have been async sub-devices already set up before calling this
> function?

I'm not opposed to that pattern, I just thought that cleanup could be 
automated for v4l2_async_notifier_parse_fwnode_endpoints() failures, as 
opposed to v4l2_async_notifier_parse_fwnode_endpoints_by_port() failures.

As this patch, written by the author of 
v4l2_async_notifier_parse_fwnode_endpoints() and part of the same patch 
series, is missing a call to v4l2_async_notifier_release(), I expect driver 
authors to make the same mistake and was thus wondering how to prevent that.

I believe that the issue here is that initialization of the notifier is done 
implicitly by the first call to a parsing function. With explicit notification 
it should be clear to driver authors that they need to call the cleanup 
function:

ret = init()
if (ret)
return ret;

ret = parse()
if (ret) {
cleanup();
return ret;
}

ret = parse()
if (ret) {
cleanup();
return ret;
}

ret = parse()
if (ret) {
cleanup();
return ret;
}

But with an implicit initialization it's easy to miss cleanup when 
v4l2_async_notifier_parse_fwnode_endpoints() fails, as that function can be 
considered as an initilization function that performs cleanup internally.

I'm not sure what the best pattern would be. At the very least you need to fix 
this patch, but that wouldn't prevent future mistakes.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Philipp Zabel]

Hi Dave,

On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
> The existing fixed value of 16 worked for UYVY 720P60 over
> 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
> 1080P60 needs 6 lanes at 594MHz).
> It doesn't allow for lower resolutions to work as the FIFO
> underflows.
> 
> Using a value of 300 works for all resolutions down to VGA60,
> and the increase in frame delay is <4usecs for 1080P60 UYVY
> (2.55usecs for RGB888).
> 
> Signed-off-by: Dave Stevenson 

Can we increase this to 320? This would also allow
720p60 at 594 Mbps / 4 lanes, according to the xls.

regards
Philipp

Re: [PATCHv2 0/2] drm/bridge/adv7511: add CEC support

[Geert Uytterhoeven]

Hi Hans,

On Tue, Sep 19, 2017 at 9:33 AM, Hans Verkuil  wrote:
> From: Hans Verkuil 
>
> I should have posted this a month ago, but I completely forgot about
> it. Apologies for that.
>
> This patch series adds CEC support to the drm adv7511/adv7533 drivers.
>
> I have tested this with the Qualcomm Dragonboard C410 (adv7533 based)
> and the Renesas R-Car Koelsch board (adv7511 based).
>
> I only have the Koelsch board to test with, but it looks like other
> R-Car boards use the same adv7511. It would be nice if someone can
> add CEC support to the other R-Car boards as well. The main thing
> to check is if they all use the same 12 MHz fixed CEC clock source.

Have a 12 MHz fixed CEC clock source connected to the CEC_CLK pin
on ADV7511:
  - r8a7790/lager
  - r8a7791/koelsch
  - r8a7791/porter
  - r8a7792/blanche
  - r8a7793/gose
  - r8a7794/alt
  - r8a7794/silk

I don't know about r8a7792/wheat. But according to its .dts file, it has two
instances of the ADV7513, not ADV7511.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v13 14/25] v4l: async: Allow binding notifiers to sub-devices

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 04:52:29PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 17:17:13 EEST Sakari Ailus wrote:
> > Registering a notifier has required the knowledge of struct v4l2_device
> > for the reason that sub-devices generally are registered to the
> > v4l2_device (as well as the media device, also available through
> > v4l2_device).
> > 
> > This information is not available for sub-device drivers at probe time.
> > 
> > What this patch does is that it allows registering notifiers without
> > having v4l2_device around. Instead the sub-device pointer is stored in the
> > notifier. Once the sub-device of the driver that registered the notifier
> > is registered, the notifier will gain the knowledge of the v4l2_device,
> > and the binding of async sub-devices from the sub-device driver's notifier
> > may proceed.
> > 
> > The root notifier's complete callback is only called when all sub-device
> > notifiers are completed.
> 
> This is a bit hard to review, shouldn't it be split in two patches, one that 
> refactors the functions, and another one that allows binding notifiers to 
> subdevs ?
> 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 218 +++-
> >  include/media/v4l2-async.h   |  16 ++-
> >  2 files changed, 203 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c index 4be2f16af051..52fe22b9b6b4
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -53,6 +53,10 @@ static int v4l2_async_notifier_call_complete(struct
> > v4l2_async_notifier *n) return n->ops->complete(n);
> >  }
> > 
> > +static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> > +  struct v4l2_subdev *sd,
> > +  struct v4l2_async_subdev *asd);
> 
> Forward declarations are often a sign that something is wrong :-/ If you 
> really need to keep this I'd move it right before the function that needs it.

"Something being wrong" here is that we have a data structure (the graph)
and a portion of the graph is parsed at any given point of time; there is
no central location with the knowledge of each node in the graph. Therefore
the process is recursive: you only learn of new nodes to parse when you
have parsed something.

I can move the declaration closer to where it's used.

> 
> >  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev
> > *asd) {
> >  #if IS_ENABLED(CONFIG_I2C)
> > @@ -124,14 +128,127 @@ static struct v4l2_async_subdev
> > *v4l2_async_find_match( return NULL;
> >  }
> > 
> > +/* Find the sub-device notifier registered by a sub-device driver. */
> > +static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> > +   struct v4l2_subdev *sd)
> > +{
> > +   struct v4l2_async_notifier *n;
> > +
> > +   list_for_each_entry(n, _list, list)
> > +   if (n->sd == sd)
> > +   return n;
> > +
> > +   return NULL;
> > +}
> > +
> > +/* Return true if all sub-device notifiers are complete, false otherwise.
> > */ 
> > +static bool v4l2_async_subdev_notifiers_complete(
> > +   struct v4l2_async_notifier *notifier)
> > +{
> > +   struct v4l2_subdev *sd;
> > +
> > +   if (!list_empty(>waiting))
> > +   return false;
> > +
> > +   list_for_each_entry(sd, >done, async_list) {
> > +   struct v4l2_async_notifier *subdev_notifier =
> > +   v4l2_async_find_subdev_notifier(sd);
> > +
> > +   if (subdev_notifier &&
> > +   !v4l2_async_subdev_notifiers_complete(subdev_notifier))
> > +   return false;
> 
> This will loop forever if two subdevs add each other to their respective 
> notifiers. We might not have any use case for that right now, but it's bound 
> to happen, at least as a bug during development, and an infinite loop (with 
> an 
> additional stack overflow bonus) isn't very nice to debug.

Well, yes. If you have a driver bug then this is what could happen.

One option is to check whether an fwnode has already been associated with
an async subdev and fail if it is. I was originally thinking of adding that
but then ended up postponing that for later. I can add that to v14.

> 
> > +   }
> > +
> > +   return true;
> > +}
> > +
> > +/* Get v4l2_device related to the notifier if one can be found. */
> > +static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
> > +   struct v4l2_async_notifier *notifier)
> > +{
> > +   while (notifier->parent)
> > +   notifier = notifier->parent;
> > +
> > +   return notifier->v4l2_dev;
> > +}
> > +
> > +/* Test all async sub-devices in a notifier for a match. */
> > +static int v4l2_async_notifier_try_all_subdevs(
> > +   struct v4l2_async_notifier *notifier)
> > +{
> > +   struct 

Re: [PATCH v2 2/4] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Dave Stevenson]

Hi Rob.

Thanks for the review.

On 19 September 2017 at 15:32, Rob Herring  wrote:
> On Wed, Sep 13, 2017 at 04:07:47PM +0100, Dave Stevenson wrote:
>> Document the DT bindings for the CSI2/CCP2 receiver peripheral
>> (known as Unicam) on BCM283x SoCs.
>>
>> Signed-off-by: Dave Stevenson 
>> ---
>>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 107 
>> +
>>  1 file changed, 107 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt 
>> b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> new file mode 100644
>> index 000..2ee5af7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> @@ -0,0 +1,107 @@
>> +Broadcom BCM283x Camera Interface (Unicam)
>> +--
>> +
>> +The Unicam block on BCM283x SoCs is the receiver for either
>> +CSI-2 or CCP2 data from image sensors or similar devices.
>> +
>> +There are two camera drivers in the kernel for BCM283x - this one
>> +and bcm2835-camera (currently in staging).
>
> Linux detail that is n/a for bindings.
>
>> +
>> +This driver is purely the kernel controlling the Unicam peripheral - there
>
> Bindings describe h/w blocks, not drivers.
>
>> +is no involvement with the VideoCore firmware. Unicam receives CSI-2
>> +(or CCP2) data and writes it into SDRAM. There is no additional processing
>> +performed.
>> +It should be possible to connect it to any sensor with a
>> +suitable output interface and V4L2 subdevice driver.
>> +
>> +bcm2835-camera uses the VideoCore firmware to control the sensor,
>> +Unicam, ISP, and various tuner control loops. Fully processed frames are
>> +delivered to the driver by the firmware. It only has sensor drivers
>> +for Omnivision OV5647, and Sony IMX219 sensors, and is closed source.
>> +
>> +The two drivers are mutually exclusive for the same Unicam instance.
>> +The firmware checks the device tree configuration during boot. If
>> +it finds device tree nodes called csi0 or csi1 then it will block the
>> +firmware from accessing the peripheral, and bcm2835-camera will
>> +not be able to stream data.
>
> All interesting, but irrelavent to the binding other than the part about
> node name. Reword to just state the requirements to get the firmware to
> ignore things.

Hans had suggested it was potentially appropriate for DT bindings too
on last review [1] (which I apologise for missing off devicetree folk
from), but I'll remove all mention of Linux/driver detail for v3.
That will leave only a reworded version of the bit about making the
firmware ignore the blocks.

[1] http://www.spinics.net/lists/linux-media/msg117047.html

>> +It should be possible to use bcm2835-camera on one camera interface
>> +and bcm2835-unicam on the other interface if there is a need to.
>
> For upstream, I don't think we care to support that. We don't need 2
> bindings.

I'll remove as irrelevant, but in that case wouldn't you have 2
bindings - one to bcm2835-camera which references the vchi node to the
GPU, and one to bcm2835-unicam that gives the full block config as
being discussed in this doc.
(Currently bcm2835-camera is a platform device so has no binding doc).
This probably isn't the place for such a discussion.

>> +
>> +Required properties:
>> +===
>> +- compatible : must be "brcm,bcm2835-unicam".
>> +- reg: physical base address and length of the register 
>> sets for the
>> +   device.
>> +- interrupts : should contain the IRQ line for this Unicam instance.
>> +- clocks : list of clock specifiers, corresponding to entries in
>> +   clock-names property.
>> +- clock-names: must contain an "lp_clock" entry, matching entries
>> +   in the clocks property.
>
> _clock is redundant.

So just "lp" as the clock name? Will assume so.

>> +
>> +Unicam supports a single port node. It should contain one 'port' child node
>> +with child 'endpoint' node. Please refer to the bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Within the endpoint node, the following properties are mandatory:
>> +- remote-endpoint: links to the source device endpoint.
>> +- data-lanes : An array denoting how many data lanes are physically
>> +   present for this CSI-2 receiver instance. This can
>> +   be limited by either the SoC itself, or by the
>> +   breakout on the platform.
>> +   Lane reordering is not supported, so lanes must be
>> +   in order, starting at 1.
>
> Just refer to docs for standard properties. Just add any info on limits
> of values like number of cells.

So again from v1 Sakari had asked for a statement of mandatory
properties for the endpoint [1].

Re: [PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs

[Sakari Ailus]

On Tue, Sep 19, 2017 at 05:58:32PM +0300, Sakari Ailus wrote:
> > This skips adding the notifier to the notifier_list. Won't this result in 
> > an 
> > oops when calling list_del(>list) in 
> > v4l2_async_notifier_unregister() ?
> 
> Good point. I'll add initialising the list head to the register function,
> with an appropriate comment.

I'll set v4l2_dev NULL instead; no tricks with lists needed.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 03:52:02PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday, 19 September 2017 15:04:43 EEST Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > Thank you for the patch.
> > 
> > On Friday, 15 September 2017 17:17:12 EEST Sakari Ailus wrote:
> > > The information on how many async sub-devices would be bindable to a
> > > notifier is typically dependent on information from platform firmware and
> > > it's not driver's business to be aware of that.
> > > 
> > > Many V4L2 main drivers are perfectly usable (and useful) without async
> > > sub-devices and so if there aren't any around, just proceed call the
> > > notifier's complete callback immediately without registering the notifier
> > > itself.
> > > 
> > > If a driver needs to check whether there are async sub-devices available,
> > > it can be done by inspecting the notifier's num_subdevs field which tells
> > > the number of async sub-devices.
> > > 
> > > Signed-off-by: Sakari Ailus 
> > > Acked-by: Hans Verkuil 
> > 
> > Reviewed-by: Laurent Pinchart 
> 
> I take this back.
> 
> > > ---
> > > 
> > >  drivers/media/v4l2-core/v4l2-async.c | 6 --
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > > b/drivers/media/v4l2-core/v4l2-async.c index 9895b610e2a0..4be2f16af051
> > > 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -170,14 +170,16 @@ int v4l2_async_notifier_register(struct v4l2_device
> > > *v4l2_dev, struct v4l2_async_subdev *asd;
> > > 
> > >   int i;
> > > 
> > > - if (!v4l2_dev || !notifier->num_subdevs ||
> > > - notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> > > + if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> > > 
> > >   return -EINVAL;
> > >   
> > >   notifier->v4l2_dev = v4l2_dev;
> > >   INIT_LIST_HEAD(>waiting);
> > >   INIT_LIST_HEAD(>done);
> > > 
> > > + if (!notifier->num_subdevs)
> > > + return v4l2_async_notifier_call_complete(notifier);
> > > +
> 
> This skips adding the notifier to the notifier_list. Won't this result in an 
> oops when calling list_del(>list) in 
> v4l2_async_notifier_unregister() ?

Good point. I'll add initialising the list head to the register function,
with an appropriate comment.

> 
> > >   for (i = 0; i < notifier->num_subdevs; i++) {
> > >   
> > >   asd = notifier->subdevs[i];
> 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 03:43:48PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday, 19 September 2017 15:13:11 EEST Sakari Ailus wrote:
> > On Tue, Sep 19, 2017 at 03:01:14PM +0300, Laurent Pinchart wrote:
> > > On Friday, 15 September 2017 17:17:10 EEST Sakari Ailus wrote:
> > >> Add three helper functions to call async operations callbacks. Besides
> > >> simplifying callbacks, this allows async notifiers to have no ops set,
> > >> i.e. it can be left NULL.
> > >> 
> > >> Signed-off-by: Sakari Ailus 
> > >> Acked-by: Hans Verkuil 
> > >> ---
> > >> 
> > >>  drivers/media/v4l2-core/v4l2-async.c | 49 +
> > >>  include/media/v4l2-async.h   |  1 +
> > >>  2 files changed, 37 insertions(+), 13 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > >> b/drivers/media/v4l2-core/v4l2-async.c index 7b2125b3d62f..c35d04b9122f
> > >> 100644
> > >> --- a/drivers/media/v4l2-core/v4l2-async.c
> > >> +++ b/drivers/media/v4l2-core/v4l2-async.c
> > >> @@ -25,6 +25,34 @@
> > >> 
> > >>  #include 
> > >>  #include 
> > >> 
> > >> +static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier
> > >> *n,
> > >> +  struct v4l2_subdev *subdev,
> > >> +  struct v4l2_async_subdev *asd)
> > >> +{
> > >> +if (!n->ops || !n->ops->bound)
> > >> +return 0;
> > >> +
> > >> +return n->ops->bound(n, subdev, asd);
> > >> +}
> > >> +
> > >> +static void v4l2_async_notifier_call_unbind(struct v4l2_async_notifier
> > >> *n,
> > >> +struct v4l2_subdev *subdev,
> > >> +struct v4l2_async_subdev 
> > >> *asd)
> > >> +{
> > >> +if (!n->ops || !n->ops->unbind)
> > >> +return;
> > >> +
> > >> +n->ops->unbind(n, subdev, asd);
> > >> +}
> > >> +
> > >> +static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier
> > >> *n)
> > >> +{
> > >> +if (!n->ops || !n->ops->complete)
> > >> +return 0;
> > >> +
> > >> +return n->ops->complete(n);
> > >> +}
> > >> +
> > > 
> > > Wouldn't it be enough to add a single v4l2_async_notifier_call() macro ?
> > > 
> > > #define v4l2_async_notifier_call(n, op, args...) \
> > > 
> > >   ((n)->ops && (n)->ops->op ? (n)->ops->op(n, ##args) : 0)
> > 
> > I actually had that in an earlier version but I changed it based on review
> > comments from Hans. A single macro isn't enough: some functions have int
> > return type. I think the way it is now is nicer.
> 
> What bothers me there is the overhead of a function call.

Overhead... of a function call?

Do you really mean what you're saying? :-) The functions will be called a
relatively small number of times during module loading / device probing.

> 
> By the way, what's the use case for ops being NULL ?

If a driver has no need for any of the callbacks, there's no benefit from
having to set ops struct either. This applies to devices that are
associated to the sensor, for instance.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v13 06/25] omap3isp: Use generic parser for parsing fwnode endpoints

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 03:46:18PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday, 19 September 2017 15:43:26 EEST Sakari Ailus wrote:
> > On Tue, Sep 19, 2017 at 02:40:29PM +0300, Laurent Pinchart wrote:
> > > > @@ -2256,7 +2210,9 @@ static int isp_probe(struct platform_device *pdev)
> > > > 
> > > > if (ret)
> > > > 
> > > > return ret;
> > > > 
> > > > -   ret = isp_fwnodes_parse(>dev, >notifier);
> > > > +   ret = v4l2_async_notifier_parse_fwnode_endpoints(
> > > > +   >dev, >notifier, sizeof(struct 
> > > > isp_async_subdev),
> > > > +   isp_fwnode_parse);
> > > > 
> > > > if (ret < 0)
> > > 
> > > The documentation in patch 05/25 states that v4l2_async_notifier_release()
> > > should be called even if v4l2_async_notifier_parse_fwnode_endpoints()
> > > fails. I don't think that's needed here, so you might want to update the
> > > documentation (and possibly the implementation of the function).
> > 
> > It is. If parsing fails, async sub-devices may have been already set up.
> > This happens e.g. when the parsing fails after the first one has been
> > successfully set up already.
> 
> But for v4l2_async_notifier_parse_fwnode_endpoints() we could clean up 
> internally when an error occurs. Otherwise you need to call 
> v4l2_async_notifier_release() here.

The functions that set up async sub-devices can be called multiple times
(on separate references). This is quite alike setting up a control handler
really, so I adopted the same pattern.

If there is a failure, how many async sub-devices should be cleaned up, if
there have been async sub-devices already set up before calling this
function?

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v2 2/4] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Rob Herring]

On Wed, Sep 13, 2017 at 04:07:47PM +0100, Dave Stevenson wrote:
> Document the DT bindings for the CSI2/CCP2 receiver peripheral
> (known as Unicam) on BCM283x SoCs.
> 
> Signed-off-by: Dave Stevenson 
> ---
>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 107 
> +
>  1 file changed, 107 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt 
> b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> new file mode 100644
> index 000..2ee5af7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
> @@ -0,0 +1,107 @@
> +Broadcom BCM283x Camera Interface (Unicam)
> +--
> +
> +The Unicam block on BCM283x SoCs is the receiver for either
> +CSI-2 or CCP2 data from image sensors or similar devices.
> +
> +There are two camera drivers in the kernel for BCM283x - this one
> +and bcm2835-camera (currently in staging).

Linux detail that is n/a for bindings.

> +
> +This driver is purely the kernel controlling the Unicam peripheral - there

Bindings describe h/w blocks, not drivers.

> +is no involvement with the VideoCore firmware. Unicam receives CSI-2
> +(or CCP2) data and writes it into SDRAM. There is no additional processing
> +performed.
> +It should be possible to connect it to any sensor with a
> +suitable output interface and V4L2 subdevice driver.
> +
> +bcm2835-camera uses the VideoCore firmware to control the sensor,
> +Unicam, ISP, and various tuner control loops. Fully processed frames are
> +delivered to the driver by the firmware. It only has sensor drivers
> +for Omnivision OV5647, and Sony IMX219 sensors, and is closed source.
> +
> +The two drivers are mutually exclusive for the same Unicam instance.
> +The firmware checks the device tree configuration during boot. If
> +it finds device tree nodes called csi0 or csi1 then it will block the
> +firmware from accessing the peripheral, and bcm2835-camera will
> +not be able to stream data.

All interesting, but irrelavent to the binding other than the part about 
node name. Reword to just state the requirements to get the firmware to 
ignore things.

> +It should be possible to use bcm2835-camera on one camera interface
> +and bcm2835-unicam on the other interface if there is a need to.

For upstream, I don't think we care to support that. We don't need 2 
bindings.

> +
> +Required properties:
> +===
> +- compatible : must be "brcm,bcm2835-unicam".
> +- reg: physical base address and length of the register sets 
> for the
> +   device.
> +- interrupts : should contain the IRQ line for this Unicam instance.
> +- clocks : list of clock specifiers, corresponding to entries in
> +   clock-names property.
> +- clock-names: must contain an "lp_clock" entry, matching entries
> +   in the clocks property.

_clock is redundant.

> +
> +Unicam supports a single port node. It should contain one 'port' child node
> +with child 'endpoint' node. Please refer to the bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Within the endpoint node, the following properties are mandatory:
> +- remote-endpoint: links to the source device endpoint.
> +- data-lanes : An array denoting how many data lanes are physically
> +   present for this CSI-2 receiver instance. This can
> +   be limited by either the SoC itself, or by the
> +   breakout on the platform.
> +   Lane reordering is not supported, so lanes must be
> +   in order, starting at 1.

Just refer to docs for standard properties. Just add any info on limits 
of values like number of cells.


> +
> +Lane reordering is not supported on the clock lane, so the optional property
> +"clock-lane" will implicitly be <0>.
> +Similarly lane inversion is not supported, therefore "lane-polarities" will
> +implicitly be <0 0 0 0 0>.
> +Neither of these values will be checked.
> +
> +Example:
> + csi1: csi@7e801000 {

I thought the node had to be called csi0 or csi1. The label (csi1) will 
be gone from the compiled dtb.

> + compatible = "brcm,bcm2835-unicam";
> + reg = <0x7e801000 0x800>,
> +   <0x7e802004 0x4>;
> + interrupts = <2 7>;
> + clocks = < BCM2835_CLOCK_CAM1>;
> + clock-names = "lp_clock";
> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;

Don't need these with a single endpoint.

> +
> + csi1_ep: endpoint {
> + remote-endpoint = <_0>;
> + data-lanes = <1 2>;
> + };
> + };
> + };
> +
> + 

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Måns Rullgård]

Marc Gonzalez  writes:

> + Rob & Mark for the DT bindings question.
>
> On 19/09/2017 14:21, Måns Rullgård wrote:
>
>> Marc Gonzalez writes:
>> 
>>> On 18/09/2017 17:33, Måns Rullgård wrote:
>>>
 What have you changed compared to my original code?
>>>
>>> I forgot to mention one change you may not approve of, so we should
>>> probably discuss it.
>>>
>>> Your driver supported an optional DT property "linux,rc-map-name"
>>> to override the RC_MAP_EMPTY map. Since the IR decoder supports
>>> multiple protocols, I found it odd to specify a scancode map in
>>> something as low-level as the device tree.
>>>
>>> I saw only one board using that property:
>>> $ git grep "linux,rc-map-name" arch/
>>> arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts: 
>>> linux,rc-map-name = "rc-geekbox";
>>>
>>> So I removed support for "linux,rc-map-name" and used ir-keytable
>>> to load a given map from user-space, depending on which RC I use.
>>>
>>> Mans, Sean, what do you think?
>> 
>> The property is documented as common for IR receivers although only a
>> few drivers seem to actually implement the feature.  Since driver
>> support is trivial, I see no reason to skip it.  Presumably someone
>> had a use for it, or it wouldn't have been added.
>
> I do not dispute the usefulness of the "linux,rc-map-name" property
> in general, e.g. for boards that support a single remote control.
>
> I am arguing that the person writing the device tree has no way of
> knowing which rc-map a given user will be using, because it depends
> on the actual remote control being used.

Most products (DVD players, TVs, etc) are only intended for use with the
supplied remote control.

-- 
Måns Rullgård

Re: [PATCH v13 17/25] v4l: fwnode: Add a helper function for parsing generic references

[Laurent Pinchart]

Hi Sakari,

On Tuesday, 19 September 2017 16:04:53 EEST Sakari Ailus wrote:
> On Tue, Sep 19, 2017 at 03:19:59PM +0300, Laurent Pinchart wrote:
> > On Friday, 15 September 2017 17:17:16 EEST Sakari Ailus wrote:
> > > Add function v4l2_fwnode_reference_parse() for parsing them as async
> > > sub-devices. This can be done on e.g. flash or lens async sub-devices
> > > that are not part of but are associated with a sensor.
> > > 
> > > Signed-off-by: Sakari Ailus 
> > > ---
> > > 
> > >  drivers/media/v4l2-core/v4l2-fwnode.c | 69 
> > >  1 file changed, 69 insertions(+)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > b/drivers/media/v4l2-core/v4l2-fwnode.c index 44ee35f6aad5..65e84ea1cc35
> > > 100644
> > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -498,6 +498,75 @@ int
> > > v4l2_async_notifier_parse_fwnode_endpoints_by_port( }
> > > 
> > >  EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints_by_port);
> > > 
> > > +/*
> > > + * v4l2_fwnode_reference_parse - parse references for async sub-devices
> > > + * @dev: the device node the properties of which are parsed for
> > > references
> > > + * @notifier: the async notifier where the async subdevs will be added
> > > + * @prop: the name of the property
> > > + *
> > > + * Return: 0 on success
> > > + *  -ENOENT if no entries were found
> > > + *  -ENOMEM if memory allocation failed
> > > + *  -EINVAL if property parsing failed
> > > + */
> > > +static int v4l2_fwnode_reference_parse(
> > > + struct device *dev, struct v4l2_async_notifier *notifier,
> > > + const char *prop)
> > > +{
> > > + struct fwnode_reference_args args;
> > > + unsigned int index;
> > > + int ret;
> > > +
> > > + for (index = 0;
> > > +  !(ret = fwnode_property_get_reference_args(
> > > +dev_fwnode(dev), prop, NULL, 0, index, ));
> > > +  index++)
> > > + fwnode_handle_put(args.fwnode);
> > 
> > This seems to indicate that the fwnode API is missing a function to count
> > the number of references in a property. Should that be fixed ?
> 
> I can send a patch adding that.
> 
> OF has one available but it's only for cases where the number of integer
> arguments isn't fixed.

Yes, in all other cases the size of one reference is known (it's the sum of 
the sizes of all arguments), so the number of references can be computed with 
size(property) / size(reference).

> > The rest looks OK to me.
> > 
> > > + if (!index)
> > > + return -ENOENT;
> > > +
> > > + /*
> > > +  * Note that right now both -ENODATA and -ENOENT may signal
> > > +  * out-of-bounds access. Return the error in cases other than that.
> > > +  */
> > > + if (ret != -ENOENT && ret != -ENODATA)
> > > + return ret;
> > > +
> > > + ret = v4l2_async_notifier_realloc(notifier,
> > > +   notifier->num_subdevs + index);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + for (index = 0; !fwnode_property_get_reference_args(
> > > +  dev_fwnode(dev), prop, NULL, 0, index, );
> > > +  index++) {
> > > + struct v4l2_async_subdev *asd;
> > > +
> > > + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {
> > > + ret = -EINVAL;
> > > + goto error;
> > > + }
> > > +
> > > + asd = kzalloc(sizeof(*asd), GFP_KERNEL);
> > > + if (!asd) {
> > > + ret = -ENOMEM;
> > > + goto error;
> > > + }
> > > +
> > > + notifier->subdevs[notifier->num_subdevs] = asd;
> > > + asd->match.fwnode.fwnode = args.fwnode;
> > > + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > > + notifier->num_subdevs++;
> > > + }
> > > +
> > > + return 0;
> > > +
> > > +error:
> > > + fwnode_handle_put(args.fwnode);
> > > + return ret;
> > > +}
> > > +
> > > 
> > >  MODULE_LICENSE("GPL");
> > >  MODULE_AUTHOR("Sakari Ailus ");
> > >  MODULE_AUTHOR("Sylwester Nawrocki ");


-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 14/25] v4l: async: Allow binding notifiers to sub-devices

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:13 EEST Sakari Ailus wrote:
> Registering a notifier has required the knowledge of struct v4l2_device
> for the reason that sub-devices generally are registered to the
> v4l2_device (as well as the media device, also available through
> v4l2_device).
> 
> This information is not available for sub-device drivers at probe time.
> 
> What this patch does is that it allows registering notifiers without
> having v4l2_device around. Instead the sub-device pointer is stored in the
> notifier. Once the sub-device of the driver that registered the notifier
> is registered, the notifier will gain the knowledge of the v4l2_device,
> and the binding of async sub-devices from the sub-device driver's notifier
> may proceed.
> 
> The root notifier's complete callback is only called when all sub-device
> notifiers are completed.

This is a bit hard to review, shouldn't it be split in two patches, one that 
refactors the functions, and another one that allows binding notifiers to 
subdevs ?

> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 218 +++-
>  include/media/v4l2-async.h   |  16 ++-
>  2 files changed, 203 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 4be2f16af051..52fe22b9b6b4
> 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -53,6 +53,10 @@ static int v4l2_async_notifier_call_complete(struct
> v4l2_async_notifier *n) return n->ops->complete(n);
>  }
> 
> +static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> +struct v4l2_subdev *sd,
> +struct v4l2_async_subdev *asd);

Forward declarations are often a sign that something is wrong :-/ If you 
really need to keep this I'd move it right before the function that needs it.

>  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) {
>  #if IS_ENABLED(CONFIG_I2C)
> @@ -124,14 +128,127 @@ static struct v4l2_async_subdev
> *v4l2_async_find_match( return NULL;
>  }
> 
> +/* Find the sub-device notifier registered by a sub-device driver. */
> +static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> + struct v4l2_subdev *sd)
> +{
> + struct v4l2_async_notifier *n;
> +
> + list_for_each_entry(n, _list, list)
> + if (n->sd == sd)
> + return n;
> +
> + return NULL;
> +}
> +
> +/* Return true if all sub-device notifiers are complete, false otherwise.
> */ 
> +static bool v4l2_async_subdev_notifiers_complete(
> + struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_subdev *sd;
> +
> + if (!list_empty(>waiting))
> + return false;
> +
> + list_for_each_entry(sd, >done, async_list) {
> + struct v4l2_async_notifier *subdev_notifier =
> + v4l2_async_find_subdev_notifier(sd);
> +
> + if (subdev_notifier &&
> + !v4l2_async_subdev_notifiers_complete(subdev_notifier))
> + return false;

This will loop forever if two subdevs add each other to their respective 
notifiers. We might not have any use case for that right now, but it's bound 
to happen, at least as a bug during development, and an infinite loop (with an 
additional stack overflow bonus) isn't very nice to debug.

> + }
> +
> + return true;
> +}
> +
> +/* Get v4l2_device related to the notifier if one can be found. */
> +static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
> + struct v4l2_async_notifier *notifier)
> +{
> + while (notifier->parent)
> + notifier = notifier->parent;
> +
> + return notifier->v4l2_dev;
> +}
> +
> +/* Test all async sub-devices in a notifier for a match. */
> +static int v4l2_async_notifier_try_all_subdevs(
> + struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_subdev *sd;
> +
> + if (!v4l2_async_notifier_find_v4l2_dev(notifier))
> + return 0;
> +
> +again:
> + list_for_each_entry(sd, _list, async_list) {
> + struct v4l2_async_subdev *asd;
> + int ret;
> +
> + asd = v4l2_async_find_match(notifier, sd);
> + if (!asd)
> + continue;
> +
> + ret = v4l2_async_match_notify(notifier, sd, asd);
> + if (ret < 0)
> + return ret;
> +
> + /*
> +  * v4l2_async_match_notify() may lead to registering a
> +  * new notifier and thus changing the async subdevs
> +  * list. In order to proceed safely from here, restart
> +  * parsing the list from the beginning.
> +  */
> + goto again;
> + }
> +
> + return 0;
> +}
> +
> +/* Try completing a notifier. */
> 

[PATCH 2/6] media: dvb_frontend: cleanup ioctl handling logic

[Mauro Carvalho Chehab]

Currently, there are two handlers for ioctls:
 - dvb_frontend_ioctl_properties()
 - dvb_frontend_ioctl_legacy()

Despite their names, both handles non-legacy DVB ioctls.

Besides that, there's no reason why to not handle all ioctls
on a single handler function.

So, merge them into a single function (dvb_frontend_handle_ioctl)
and reorganize the ioctl's to indicate what's the current DVB
API and what's deprecated.

Despite the big diff, the handling logic for each ioctl is the
same as before.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/dvb-core/dvb_frontend.c | 402 +-
 1 file changed, 195 insertions(+), 207 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c 
b/drivers/media/dvb-core/dvb_frontend.c
index 725cb1c8a088..cbe697a094d2 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1315,10 +1315,8 @@ static int dtv_get_frontend(struct dvb_frontend *fe,
return 0;
 }
 
-static int dvb_frontend_ioctl_legacy(struct file *file,
-   unsigned int cmd, void *parg);
-static int dvb_frontend_ioctl_properties(struct file *file,
-   unsigned int cmd, void *parg);
+static int dvb_frontend_handle_ioctl(struct file *file,
+unsigned int cmd, void *parg);
 
 static int dtv_property_process_get(struct dvb_frontend *fe,
const struct dtv_frontend_properties *c,
@@ -1816,12 +1814,12 @@ static int dtv_property_process_set(struct dvb_frontend 
*fe,
break;
case DTV_VOLTAGE:
c->voltage = tvp->u.data;
-   r = dvb_frontend_ioctl_legacy(file, FE_SET_VOLTAGE,
+   r = dvb_frontend_handle_ioctl(file, FE_SET_VOLTAGE,
(void *)c->voltage);
break;
case DTV_TONE:
c->sectone = tvp->u.data;
-   r = dvb_frontend_ioctl_legacy(file, FE_SET_TONE,
+   r = dvb_frontend_handle_ioctl(file, FE_SET_TONE,
(void *)c->sectone);
break;
case DTV_CODE_RATE_HP:
@@ -1928,14 +1926,13 @@ static int dtv_property_process_set(struct dvb_frontend 
*fe,
return r;
 }
 
-static int dvb_frontend_ioctl(struct file *file,
-   unsigned int cmd, void *parg)
+static int dvb_frontend_ioctl(struct file *file, unsigned int cmd, void *parg)
 {
struct dvb_device *dvbdev = file->private_data;
struct dvb_frontend *fe = dvbdev->priv;
struct dtv_frontend_properties *c = >dtv_property_cache;
struct dvb_frontend_private *fepriv = fe->frontend_priv;
-   int err = -EOPNOTSUPP;
+   int err;
 
dev_dbg(fe->dvb->device, "%s: (%d)\n", __func__, _IOC_NR(cmd));
if (down_interruptible(>sem))
@@ -1953,121 +1950,13 @@ static int dvb_frontend_ioctl(struct file *file,
return -EPERM;
}
 
-   if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY))
-   err = dvb_frontend_ioctl_properties(file, cmd, parg);
-   else {
-   c->state = DTV_UNDEFINED;
-   err = dvb_frontend_ioctl_legacy(file, cmd, parg);
-   }
+   c->state = DTV_UNDEFINED;
+   err = dvb_frontend_handle_ioctl(file, cmd, parg);
 
up(>sem);
return err;
 }
 
-static int dvb_frontend_ioctl_properties(struct file *file,
-   unsigned int cmd, void *parg)
-{
-   struct dvb_device *dvbdev = file->private_data;
-   struct dvb_frontend *fe = dvbdev->priv;
-   struct dvb_frontend_private *fepriv = fe->frontend_priv;
-   struct dtv_frontend_properties *c = >dtv_property_cache;
-   int err, i;
-
-   dev_dbg(fe->dvb->device, "%s:\n", __func__);
-
-   switch(cmd) {
-   case FE_SET_PROPERTY: {
-   struct dtv_properties *tvps = parg;
-   struct dtv_property *tvp = NULL;
-
-   dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
-   __func__, tvps->num);
-   dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
-   __func__, tvps->props);
-
-   /*
-* Put an arbitrary limit on the number of messages that can
-* be sent at once
-*/
-   if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
-   return -EINVAL;
-
-   tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
-   if (IS_ERR(tvp))
-   return PTR_ERR(tvp);
-
-   for (i = 0; i < tvps->num; i++) {
-   err = dtv_property_process_set(fe, tvp + i, file);
-   if (err < 0) {
-   kfree(tvp);
-   return err;
-   }
-   (tvp + i)->result = err;
-   }
-
-  

[PATCH 4/6] media: dvb_frontend.h: fix alignment at the cache properties

[Mauro Carvalho Chehab]

There are too much tabs on several properties, for no good
reason. That sounds confusing while reading the struct, so
adjust them.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/dvb-core/dvb_frontend.h | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.h 
b/drivers/media/dvb-core/dvb_frontend.h
index 1bdeb10f0d78..d273ed2f72c9 100644
--- a/drivers/media/dvb-core/dvb_frontend.h
+++ b/drivers/media/dvb-core/dvb_frontend.h
@@ -557,15 +557,15 @@ struct dtv_frontend_properties {
 
enum fe_sec_voltage voltage;
enum fe_sec_tone_mode   sectone;
-   enum fe_spectral_inversion  inversion;
-   enum fe_code_rate   fec_inner;
+   enum fe_spectral_inversion inversion;
+   enum fe_code_rate   fec_inner;
enum fe_transmit_mode   transmission_mode;
u32 bandwidth_hz;   /* 0 = AUTO */
enum fe_guard_interval  guard_interval;
-   enum fe_hierarchy   hierarchy;
+   enum fe_hierarchy   hierarchy;
u32 symbol_rate;
-   enum fe_code_rate   code_rate_HP;
-   enum fe_code_rate   code_rate_LP;
+   enum fe_code_rate   code_rate_HP;
+   enum fe_code_rate   code_rate_LP;
 
enum fe_pilot   pilot;
enum fe_rolloff rolloff;
-- 
2.13.5

[PATCH 5/6] media: dvb_frontend: better document the -EPERM condition

[Mauro Carvalho Chehab]

Two readonly ioctls can't be allowed if the frontend device
is opened in read only mode. Explain why.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/dvb-core/dvb_frontend.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c 
b/drivers/media/dvb-core/dvb_frontend.c
index d0a17d67ab1b..db3f8c597a24 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1940,9 +1940,23 @@ static int dvb_frontend_ioctl(struct file *file, 
unsigned int cmd, void *parg)
return -ENODEV;
}
 
-   if ((file->f_flags & O_ACCMODE) == O_RDONLY &&
-   (_IOC_DIR(cmd) != _IOC_READ || cmd == FE_GET_EVENT ||
-cmd == FE_DISEQC_RECV_SLAVE_REPLY)) {
+   /*
+* If the frontend is opened in read-only mode, only the ioctls
+* that don't interfere at the tune logic should be accepted.
+* That allows an external application to monitor the DVB QoS and
+* statistics parameters.
+*
+* That matches all _IOR() ioctls, except for two special cases:
+*   - FE_GET_EVENT is part of the tuning logic on a DVB application;
+*   - FE_DISEQC_RECV_SLAVE_REPLY is part of DiSEqC 2.0
+* setup
+* So, those two ioctls should also return -EPERM, as otherwise
+* reading from them would interfere with a DVB tune application
+*/
+   if ((file->f_flags & O_ACCMODE) == O_RDONLY
+   && (_IOC_DIR(cmd) != _IOC_READ
+   || cmd == FE_GET_EVENT
+   || cmd == FE_DISEQC_RECV_SLAVE_REPLY)) {
up(>sem);
return -EPERM;
}
-- 
2.13.5

[PATCH 6/6] media: dvb_frontend: fix return values for FE_SET_PROPERTY

[Mauro Carvalho Chehab]

There are several problems with regards to the return of
FE_SET_PROPERTY. The original idea were to return per-property
return codes via tvp->result field, and to return an updated
set of values.

However, that never worked. What's actually implemented is:

- the FE_SET_PROPERTY implementation doesn't call .get_frontend
  callback in order to get the actual parameters after return;

- the tvp->result field is only filled if there's no error.
  So, it is always filled with zero;

- FE_SET_PROPERTY doesn't call memdup_user() nor any other
  copy_to_user() function. So, any changes at the properies
  will be lost;

- FE_SET_PROPERTY is declared as a write-only ioctl (IOW).

While we could fix the above, it could cause regressions.

So, let's just assume what the code really does, updating
the documentation accordingly and removing the logic that
would update the discarded tvp->result.

Signed-off-by: Mauro Carvalho Chehab 
---
 Documentation/media/uapi/dvb/fe-get-property.rst | 7 +--
 drivers/media/dvb-core/dvb_frontend.c| 2 --
 include/uapi/linux/dvb/frontend.h| 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/media/uapi/dvb/fe-get-property.rst 
b/Documentation/media/uapi/dvb/fe-get-property.rst
index 948d2ba84f2c..b69741d9cedf 100644
--- a/Documentation/media/uapi/dvb/fe-get-property.rst
+++ b/Documentation/media/uapi/dvb/fe-get-property.rst
@@ -48,8 +48,11 @@ depends on the delivery system and on the device:
 
-  This call requires read/write access to the device.
 
-   -  At return, the values are updated to reflect the actual parameters
-  used.
+.. note::
+
+   At return, the values aren't updated to reflect the actual
+   parameters used. If the actual parameters are needed, an explicit
+   call to ``FE_GET_PROPERTY`` is needed.
 
 -  ``FE_GET_PROPERTY:``
 
diff --git a/drivers/media/dvb-core/dvb_frontend.c 
b/drivers/media/dvb-core/dvb_frontend.c
index db3f8c597a24..4b5acc9d0124 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2142,7 +2142,6 @@ static int dvb_frontend_handle_ioctl(struct file *file,
kfree(tvp);
return err;
}
-   (tvp + i)->result = err;
}
kfree(tvp);
break;
@@ -2187,7 +2186,6 @@ static int dvb_frontend_handle_ioctl(struct file *file,
kfree(tvp);
return err;
}
-   (tvp + i)->result = err;
}
 
if (copy_to_user((void __user *)tvps->props, tvp,
diff --git a/include/uapi/linux/dvb/frontend.h 
b/include/uapi/linux/dvb/frontend.h
index 861cacd5711f..6bc26f35217b 100644
--- a/include/uapi/linux/dvb/frontend.h
+++ b/include/uapi/linux/dvb/frontend.h
@@ -830,7 +830,7 @@ struct dtv_fe_stats {
  * @cmd:   Digital TV command.
  * @reserved:  Not used.
  * @u: Union with the values for the command.
- * @result:Result of the command set (currently unused).
+ * @result:Unused
  *
  * The @u union may have either one of the values below:
  *
-- 
2.13.5

[PATCH 1/6] media: dvb_frontend: cleanup dvb_frontend_ioctl_properties()

[Mauro Carvalho Chehab]

Use a switch() on this function, just like on other ioctl
handlers and handle parameters inside each part of the
switch.

That makes it easier to integrate with the already existing
ioctl handler function.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/dvb-core/dvb_frontend.c | 83 +--
 1 file changed, 51 insertions(+), 32 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c 
b/drivers/media/dvb-core/dvb_frontend.c
index 8abe4f541a36..725cb1c8a088 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -1971,21 +1971,25 @@ static int dvb_frontend_ioctl_properties(struct file 
*file,
struct dvb_frontend *fe = dvbdev->priv;
struct dvb_frontend_private *fepriv = fe->frontend_priv;
struct dtv_frontend_properties *c = >dtv_property_cache;
-   int err = 0;
-
-   struct dtv_properties *tvps = parg;
-   struct dtv_property *tvp = NULL;
-   int i;
+   int err, i;
 
dev_dbg(fe->dvb->device, "%s:\n", __func__);
 
-   if (cmd == FE_SET_PROPERTY) {
-   dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, 
tvps->num);
-   dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", 
__func__, tvps->props);
+   switch(cmd) {
+   case FE_SET_PROPERTY: {
+   struct dtv_properties *tvps = parg;
+   struct dtv_property *tvp = NULL;
 
-   /* Put an arbitrary limit on the number of messages that can
-* be sent at once */
-   if ((tvps->num == 0) || (tvps->num > DTV_IOCTL_MAX_MSGS))
+   dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
+   __func__, tvps->num);
+   dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
+   __func__, tvps->props);
+
+   /*
+* Put an arbitrary limit on the number of messages that can
+* be sent at once
+*/
+   if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
return -EINVAL;
 
tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
@@ -1994,23 +1998,34 @@ static int dvb_frontend_ioctl_properties(struct file 
*file,
 
for (i = 0; i < tvps->num; i++) {
err = dtv_property_process_set(fe, tvp + i, file);
-   if (err < 0)
-   goto out;
+   if (err < 0) {
+   kfree(tvp);
+   return err;
+   }
(tvp + i)->result = err;
}
 
if (c->state == DTV_TUNE)
dev_dbg(fe->dvb->device, "%s: Property cache is full, 
tuning\n", __func__);
 
-   } else if (cmd == FE_GET_PROPERTY) {
+   kfree(tvp);
+   break;
+   }
+   case FE_GET_PROPERTY: {
+   struct dtv_properties *tvps = parg;
+   struct dtv_property *tvp = NULL;
struct dtv_frontend_properties getp = fe->dtv_property_cache;
 
-   dev_dbg(fe->dvb->device, "%s: properties.num = %d\n", __func__, 
tvps->num);
-   dev_dbg(fe->dvb->device, "%s: properties.props = %p\n", 
__func__, tvps->props);
+   dev_dbg(fe->dvb->device, "%s: properties.num = %d\n",
+   __func__, tvps->num);
+   dev_dbg(fe->dvb->device, "%s: properties.props = %p\n",
+   __func__, tvps->props);
 
-   /* Put an arbitrary limit on the number of messages that can
-* be sent at once */
-   if ((tvps->num == 0) || (tvps->num > DTV_IOCTL_MAX_MSGS))
+   /*
+* Put an arbitrary limit on the number of messages that can
+* be sent at once
+*/
+   if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
return -EINVAL;
 
tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
@@ -2025,28 +2040,32 @@ static int dvb_frontend_ioctl_properties(struct file 
*file,
 */
if (fepriv->state != FESTATE_IDLE) {
err = dtv_get_frontend(fe, , NULL);
-   if (err < 0)
-   goto out;
+   if (err < 0) {
+   kfree(tvp);
+   return err;
+   }
}
for (i = 0; i < tvps->num; i++) {
err = dtv_property_process_get(fe, , tvp + i, 
file);
-   if (err < 0)
-   goto out;
+   if (err < 0) {
+   kfree(tvp);
+   return err;
+   }
 

[PATCH 3/6] media: dvb_frontend: get rid of proprierty cache's state

[Mauro Carvalho Chehab]

In the past, I guess the idea was to use state in order to
allow an autofush logic. However, in the current code, it is
used only for debug messages, on a poor man's solution, as
there's already a debug message to indicate when the properties
got flushed.

So, just get rid of it for good.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/dvb-core/dvb_frontend.c | 20 ++--
 drivers/media/dvb-core/dvb_frontend.h |  5 -
 2 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c 
b/drivers/media/dvb-core/dvb_frontend.c
index cbe697a094d2..d0a17d67ab1b 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -951,8 +951,6 @@ static int dvb_frontend_clear_cache(struct dvb_frontend *fe)
memset(c, 0, offsetof(struct dtv_frontend_properties, strength));
c->delivery_system = delsys;
 
-   c->state = DTV_CLEAR;
-
dev_dbg(fe->dvb->device, "%s: Clearing cache for delivery system %d\n",
__func__, c->delivery_system);
 
@@ -1775,13 +1773,13 @@ static int dtv_property_process_set(struct dvb_frontend 
*fe,
dvb_frontend_clear_cache(fe);
break;
case DTV_TUNE:
-   /* interpret the cache of data, build either a traditional 
frontend
-* tunerequest so we can pass validation in the FE_SET_FRONTEND
-* ioctl.
+   /*
+* Use the cached Digital TV properties to tune the
+* frontend
 */
-   c->state = tvp->cmd;
-   dev_dbg(fe->dvb->device, "%s: Finalised property cache\n",
-   __func__);
+   dev_dbg(fe->dvb->device,
+   "%s: Setting the frontend from property cache\n",
+   __func__);
 
r = dtv_set_frontend(fe);
break;
@@ -1930,7 +1928,6 @@ static int dvb_frontend_ioctl(struct file *file, unsigned 
int cmd, void *parg)
 {
struct dvb_device *dvbdev = file->private_data;
struct dvb_frontend *fe = dvbdev->priv;
-   struct dtv_frontend_properties *c = >dtv_property_cache;
struct dvb_frontend_private *fepriv = fe->frontend_priv;
int err;
 
@@ -1950,7 +1947,6 @@ static int dvb_frontend_ioctl(struct file *file, unsigned 
int cmd, void *parg)
return -EPERM;
}
 
-   c->state = DTV_UNDEFINED;
err = dvb_frontend_handle_ioctl(file, cmd, parg);
 
up(>sem);
@@ -2134,10 +2130,6 @@ static int dvb_frontend_handle_ioctl(struct file *file,
}
(tvp + i)->result = err;
}
-
-   if (c->state == DTV_TUNE)
-   dev_dbg(fe->dvb->device, "%s: Property cache is full, 
tuning\n", __func__);
-
kfree(tvp);
break;
}
diff --git a/drivers/media/dvb-core/dvb_frontend.h 
b/drivers/media/dvb-core/dvb_frontend.h
index 852b91ba49d2..1bdeb10f0d78 100644
--- a/drivers/media/dvb-core/dvb_frontend.h
+++ b/drivers/media/dvb-core/dvb_frontend.h
@@ -620,11 +620,6 @@ struct dtv_frontend_properties {
struct dtv_fe_stats post_bit_count;
struct dtv_fe_stats block_error;
struct dtv_fe_stats block_count;
-
-   /* private: */
-   /* Cache State */
-   u32 state;
-
 };
 
 #define DVB_FE_NO_EXIT  0
-- 
2.13.5

[PATCH 0/3] [media] tc358743: Support for a wider range of inputs

[Dave Stevenson]

Three minor changes to the TC358743 HDMI to CSI2 bridge chip driver.
- Correct the clock mode reported via g_mbus_config to match that set.
- Increase the FIFO level to allow resolutions lower than 720P60 to work.
- Add settings for a new link frequency of 972Mbit/s. This allows
  1080P50 UYVY to work on two lanes (useful on the Raspberry Pi which
  only brings out two CSI2 lanes to the camera connector).

I'd like to extend the last one to dynamically calculate all the values
for an arbitrary link speed, but time hasn't allowed as yet.

Dave Stevenson (3):
  [media] tc358743: Correct clock mode reported in g_mbus_config
  [media] tc358743: Increase FIFO level to 300.
  [media] tc358743: Add support for 972Mbit/s link freq.

 drivers/media/i2c/tc358743.c | 62 +++-
 1 file changed, 44 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH 3/3] [media] tc358743: Add support for 972Mbit/s link freq.

[Dave Stevenson]

Adds register setups for running the CSI lanes at 972Mbit/s,
which allows 1080P50 UYVY down 2 lanes.

Signed-off-by: Dave Stevenson 
---
 drivers/media/i2c/tc358743.c | 47 +++-
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 7632daf..dcc100e 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1809,6 +1809,7 @@ static int tc358743_probe_of(struct tc358743_state *state)
/*
 * The CSI bps per lane must be between 62.5 Mbps and 1 Gbps.
 * The default is 594 Mbps for 4-lane 1080p60 or 2-lane 720p60.
+* 972 Mbps allows 1080P50 UYVY over 2-lane.
 */
bps_pr_lane = 2 * endpoint->link_frequencies[0];
if (bps_pr_lane < 6250U || bps_pr_lane > 10U) {
@@ -1821,23 +1822,41 @@ static int tc358743_probe_of(struct tc358743_state 
*state)
   state->pdata.refclk_hz * state->pdata.pll_prd;
 
/*
-* FIXME: These timings are from REF_02 for 594 Mbps per lane (297 MHz
-* link frequency). In principle it should be possible to calculate
+* FIXME: These timings are from REF_02 for 594 or 972 Mbps per lane
+* (297 MHz or 495 MHz link frequency).
+* In principle it should be possible to calculate
 * them based on link frequency and resolution.
 */
-   if (bps_pr_lane != 59400U)
+   switch (bps_pr_lane) {
+   default:
dev_warn(dev, "untested bps per lane: %u bps\n", bps_pr_lane);
-   state->pdata.lineinitcnt = 0xe80;
-   state->pdata.lptxtimecnt = 0x003;
-   /* tclk-preparecnt: 3, tclk-zerocnt: 20 */
-   state->pdata.tclk_headercnt = 0x1403;
-   state->pdata.tclk_trailcnt = 0x00;
-   /* ths-preparecnt: 3, ths-zerocnt: 1 */
-   state->pdata.ths_headercnt = 0x0103;
-   state->pdata.twakeup = 0x4882;
-   state->pdata.tclk_postcnt = 0x008;
-   state->pdata.ths_trailcnt = 0x2;
-   state->pdata.hstxvregcnt = 0;
+   case 59400U:
+   state->pdata.lineinitcnt = 0xe80;
+   state->pdata.lptxtimecnt = 0x003;
+   /* tclk-preparecnt: 3, tclk-zerocnt: 20 */
+   state->pdata.tclk_headercnt = 0x1403;
+   state->pdata.tclk_trailcnt = 0x00;
+   /* ths-preparecnt: 3, ths-zerocnt: 1 */
+   state->pdata.ths_headercnt = 0x0103;
+   state->pdata.twakeup = 0x4882;
+   state->pdata.tclk_postcnt = 0x008;
+   state->pdata.ths_trailcnt = 0x2;
+   state->pdata.hstxvregcnt = 0;
+   break;
+   case 97200U:
+   state->pdata.lineinitcnt = 0xFA0;
+   state->pdata.lptxtimecnt = 0x007;
+   /* tclk-preparecnt: 6, tclk-zerocnt: 40 */
+   state->pdata.tclk_headercnt = 0x2806;
+   state->pdata.tclk_trailcnt = 0x00;
+   /* ths-preparecnt: 3, ths-zerocnt: 1 */
+   state->pdata.ths_headercnt = 0x0806;
+   state->pdata.twakeup = 0x4268;
+   state->pdata.tclk_postcnt = 0x008;
+   state->pdata.ths_trailcnt = 0x5;
+   state->pdata.hstxvregcnt = 0;
+   break;
+   }
 
state->reset_gpio = devm_gpiod_get_optional(dev, "reset",
GPIOD_OUT_LOW);
-- 
2.7.4

[PATCH 1/3] [media] tc358743: Correct clock mode reported in g_mbus_config

[Dave Stevenson]

Support for non-continuous clock had previously been added via
device tree, but a comment and the value reported by g_mbus_config
still stated that it wasn't supported.
Remove the comment, and return the correct value in g_mbus_config.

Signed-off-by: Dave Stevenson 
---
 drivers/media/i2c/tc358743.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index e6f5c36..6b0fd07 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1461,8 +1461,9 @@ static int tc358743_g_mbus_config(struct v4l2_subdev *sd,
 
cfg->type = V4L2_MBUS_CSI2;
 
-   /* Support for non-continuous CSI-2 clock is missing in the driver */
-   cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+   cfg->flags = state->bus.flags &
+   (V4L2_MBUS_CSI2_CONTINUOUS_CLOCK |
+V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK);
 
switch (state->csi_lanes_in_use) {
case 1:
-- 
2.7.4

[PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Dave Stevenson]

The existing fixed value of 16 worked for UYVY 720P60 over
2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
1080P60 needs 6 lanes at 594MHz).
It doesn't allow for lower resolutions to work as the FIFO
underflows.

Using a value of 300 works for all resolutions down to VGA60,
and the increase in frame delay is <4usecs for 1080P60 UYVY
(2.55usecs for RGB888).

Signed-off-by: Dave Stevenson 
---
 drivers/media/i2c/tc358743.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 6b0fd07..7632daf 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1782,8 +1782,14 @@ static int tc358743_probe_of(struct tc358743_state 
*state)
state->pdata.refclk_hz = clk_get_rate(refclk);
state->pdata.ddc5v_delay = DDC5V_DELAY_100_MS;
state->pdata.enable_hdcp = false;
-   /* A FIFO level of 16 should be enough for 2-lane 720p60 at 594 MHz. */
-   state->pdata.fifo_level = 16;
+   /*
+* A FIFO level of 16 should be enough for 2-lane 720p60 at 594 MHz,
+* but is insufficient for lower resolutions.
+* A value of 300 allows for resolutions down to VGA60 (and possibly
+* lower) to work, whilst still leaving the delay for 1080P60
+* stilll below 4usecs.
+*/
+   state->pdata.fifo_level = 300;
/*
 * The PLL input clock is obtained by dividing refclk by pll_prd.
 * It must be between 6 MHz and 40 MHz, lower frequency is better.
-- 
2.7.4

Re: [PATCH v13 17/25] v4l: fwnode: Add a helper function for parsing generic references

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 03:19:59PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 17:17:16 EEST Sakari Ailus wrote:
> > Add function v4l2_fwnode_reference_parse() for parsing them as async
> > sub-devices. This can be done on e.g. flash or lens async sub-devices that
> > are not part of but are associated with a sensor.
> > 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 69 
> >  1 file changed, 69 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > b/drivers/media/v4l2-core/v4l2-fwnode.c index 44ee35f6aad5..65e84ea1cc35
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -498,6 +498,75 @@ int v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> > }
> >  EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints_by_port);
> > 
> > +/*
> > + * v4l2_fwnode_reference_parse - parse references for async sub-devices
> > + * @dev: the device node the properties of which are parsed for references
> > + * @notifier: the async notifier where the async subdevs will be added
> > + * @prop: the name of the property
> > + *
> > + * Return: 0 on success
> > + *-ENOENT if no entries were found
> > + *-ENOMEM if memory allocation failed
> > + *-EINVAL if property parsing failed
> > + */
> > +static int v4l2_fwnode_reference_parse(
> > +   struct device *dev, struct v4l2_async_notifier *notifier,
> > +   const char *prop)
> > +{
> > +   struct fwnode_reference_args args;
> > +   unsigned int index;
> > +   int ret;
> > +
> > +   for (index = 0;
> > +!(ret = fwnode_property_get_reference_args(
> > +  dev_fwnode(dev), prop, NULL, 0, index, ));
> > +index++)
> > +   fwnode_handle_put(args.fwnode);
> 
> This seems to indicate that the fwnode API is missing a function to count the 
> number of references in a property. Should that be fixed ?

I can send a patch adding that.

OF has one available but it's only for cases where the number of integer
arguments isn't fixed.

> 
> The rest looks OK to me.
> 
> > +   if (!index)
> > +   return -ENOENT;
> > +
> > +   /*
> > +* Note that right now both -ENODATA and -ENOENT may signal
> > +* out-of-bounds access. Return the error in cases other than that.
> > +*/
> > +   if (ret != -ENOENT && ret != -ENODATA)
> > +   return ret;
> > +
> > +   ret = v4l2_async_notifier_realloc(notifier,
> > + notifier->num_subdevs + index);
> > +   if (ret)
> > +   return ret;
> > +
> > +   for (index = 0; !fwnode_property_get_reference_args(
> > +dev_fwnode(dev), prop, NULL, 0, index, );
> > +index++) {
> > +   struct v4l2_async_subdev *asd;
> > +
> > +   if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {
> > +   ret = -EINVAL;
> > +   goto error;
> > +   }
> > +
> > +   asd = kzalloc(sizeof(*asd), GFP_KERNEL);
> > +   if (!asd) {
> > +   ret = -ENOMEM;
> > +   goto error;
> > +   }
> > +
> > +   notifier->subdevs[notifier->num_subdevs] = asd;
> > +   asd->match.fwnode.fwnode = args.fwnode;
> > +   asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +   notifier->num_subdevs++;
> > +   }
> > +
> > +   return 0;
> > +
> > +error:
> > +   fwnode_handle_put(args.fwnode);
> > +   return ret;
> > +}
> > +
> >  MODULE_LICENSE("GPL");
> >  MODULE_AUTHOR("Sakari Ailus ");
> >  MODULE_AUTHOR("Sylwester Nawrocki ");
> 
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs

[Laurent Pinchart]

Hi Sakari,

On Tuesday, 19 September 2017 15:04:43 EEST Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 17:17:12 EEST Sakari Ailus wrote:
> > The information on how many async sub-devices would be bindable to a
> > notifier is typically dependent on information from platform firmware and
> > it's not driver's business to be aware of that.
> > 
> > Many V4L2 main drivers are perfectly usable (and useful) without async
> > sub-devices and so if there aren't any around, just proceed call the
> > notifier's complete callback immediately without registering the notifier
> > itself.
> > 
> > If a driver needs to check whether there are async sub-devices available,
> > it can be done by inspecting the notifier's num_subdevs field which tells
> > the number of async sub-devices.
> > 
> > Signed-off-by: Sakari Ailus 
> > Acked-by: Hans Verkuil 
> 
> Reviewed-by: Laurent Pinchart 

I take this back.

> > ---
> > 
> >  drivers/media/v4l2-core/v4l2-async.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c index 9895b610e2a0..4be2f16af051
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -170,14 +170,16 @@ int v4l2_async_notifier_register(struct v4l2_device
> > *v4l2_dev, struct v4l2_async_subdev *asd;
> > 
> > int i;
> > 
> > -   if (!v4l2_dev || !notifier->num_subdevs ||
> > -   notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> > +   if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> > 
> > return -EINVAL;
> > 
> > notifier->v4l2_dev = v4l2_dev;
> > INIT_LIST_HEAD(>waiting);
> > INIT_LIST_HEAD(>done);
> > 
> > +   if (!notifier->num_subdevs)
> > +   return v4l2_async_notifier_call_complete(notifier);
> > +

This skips adding the notifier to the notifier_list. Won't this result in an 
oops when calling list_del(>list) in 
v4l2_async_notifier_unregister() ?

> > for (i = 0; i < notifier->num_subdevs; i++) {
> > 
> > asd = notifier->subdevs[i];

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 06/25] omap3isp: Use generic parser for parsing fwnode endpoints

[Laurent Pinchart]

Hi Sakari,

On Tuesday, 19 September 2017 15:43:26 EEST Sakari Ailus wrote:
> On Tue, Sep 19, 2017 at 02:40:29PM +0300, Laurent Pinchart wrote:
> > > @@ -2256,7 +2210,9 @@ static int isp_probe(struct platform_device *pdev)
> > > 
> > >   if (ret)
> > >   
> > >   return ret;
> > > 
> > > - ret = isp_fwnodes_parse(>dev, >notifier);
> > > + ret = v4l2_async_notifier_parse_fwnode_endpoints(
> > > + >dev, >notifier, sizeof(struct isp_async_subdev),
> > > + isp_fwnode_parse);
> > > 
> > >   if (ret < 0)
> > 
> > The documentation in patch 05/25 states that v4l2_async_notifier_release()
> > should be called even if v4l2_async_notifier_parse_fwnode_endpoints()
> > fails. I don't think that's needed here, so you might want to update the
> > documentation (and possibly the implementation of the function).
> 
> It is. If parsing fails, async sub-devices may have been already set up.
> This happens e.g. when the parsing fails after the first one has been
> successfully set up already.

But for v4l2_async_notifier_parse_fwnode_endpoints() we could clean up 
internally when an error occurs. Otherwise you need to call 
v4l2_async_notifier_release() here.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 06/25] omap3isp: Use generic parser for parsing fwnode endpoints

[Sakari Ailus]

On Tue, Sep 19, 2017 at 02:40:29PM +0300, Laurent Pinchart wrote:
> > @@ -2256,7 +2210,9 @@ static int isp_probe(struct platform_device *pdev)
> > if (ret)
> > return ret;
> > 
> > -   ret = isp_fwnodes_parse(>dev, >notifier);
> > +   ret = v4l2_async_notifier_parse_fwnode_endpoints(
> > +   >dev, >notifier, sizeof(struct isp_async_subdev),
> > +   isp_fwnode_parse);
> > if (ret < 0)
> 
> The documentation in patch 05/25 states that v4l2_async_notifier_release() 
> should be called even if v4l2_async_notifier_parse_fwnode_endpoints() fails. 
> I 
> don't think that's needed here, so you might want to update the documentation 
> (and possibly the implementation of the function).

It is. If parsing fails, async sub-devices may have been already set up.
This happens e.g. when the parsing fails after the first one has been
successfully set up already.

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

[Laurent Pinchart]

Hi Sakari,

On Tuesday, 19 September 2017 15:13:11 EEST Sakari Ailus wrote:
> On Tue, Sep 19, 2017 at 03:01:14PM +0300, Laurent Pinchart wrote:
> > On Friday, 15 September 2017 17:17:10 EEST Sakari Ailus wrote:
> >> Add three helper functions to call async operations callbacks. Besides
> >> simplifying callbacks, this allows async notifiers to have no ops set,
> >> i.e. it can be left NULL.
> >> 
> >> Signed-off-by: Sakari Ailus 
> >> Acked-by: Hans Verkuil 
> >> ---
> >> 
> >>  drivers/media/v4l2-core/v4l2-async.c | 49 +
> >>  include/media/v4l2-async.h   |  1 +
> >>  2 files changed, 37 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> >> b/drivers/media/v4l2-core/v4l2-async.c index 7b2125b3d62f..c35d04b9122f
> >> 100644
> >> --- a/drivers/media/v4l2-core/v4l2-async.c
> >> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >> @@ -25,6 +25,34 @@
> >> 
> >>  #include 
> >>  #include 
> >> 
> >> +static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier
> >> *n,
> >> +struct v4l2_subdev *subdev,
> >> +struct v4l2_async_subdev *asd)
> >> +{
> >> +  if (!n->ops || !n->ops->bound)
> >> +  return 0;
> >> +
> >> +  return n->ops->bound(n, subdev, asd);
> >> +}
> >> +
> >> +static void v4l2_async_notifier_call_unbind(struct v4l2_async_notifier
> >> *n,
> >> +  struct v4l2_subdev *subdev,
> >> +  struct v4l2_async_subdev *asd)
> >> +{
> >> +  if (!n->ops || !n->ops->unbind)
> >> +  return;
> >> +
> >> +  n->ops->unbind(n, subdev, asd);
> >> +}
> >> +
> >> +static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier
> >> *n)
> >> +{
> >> +  if (!n->ops || !n->ops->complete)
> >> +  return 0;
> >> +
> >> +  return n->ops->complete(n);
> >> +}
> >> +
> > 
> > Wouldn't it be enough to add a single v4l2_async_notifier_call() macro ?
> > 
> > #define v4l2_async_notifier_call(n, op, args...) \
> > 
> > ((n)->ops && (n)->ops->op ? (n)->ops->op(n, ##args) : 0)
> 
> I actually had that in an earlier version but I changed it based on review
> comments from Hans. A single macro isn't enough: some functions have int
> return type. I think the way it is now is nicer.

What bothers me there is the overhead of a function call.

By the way, what's the use case for ops being NULL ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Marc Gonzalez]

+ Rob & Mark for the DT bindings question.

On 19/09/2017 14:21, Måns Rullgård wrote:

> Marc Gonzalez writes:
> 
>> On 18/09/2017 17:33, Måns Rullgård wrote:
>>
>>> What have you changed compared to my original code?
>>
>> I forgot to mention one change you may not approve of, so we should
>> probably discuss it.
>>
>> Your driver supported an optional DT property "linux,rc-map-name"
>> to override the RC_MAP_EMPTY map. Since the IR decoder supports
>> multiple protocols, I found it odd to specify a scancode map in
>> something as low-level as the device tree.
>>
>> I saw only one board using that property:
>> $ git grep "linux,rc-map-name" arch/
>> arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts: 
>> linux,rc-map-name = "rc-geekbox";
>>
>> So I removed support for "linux,rc-map-name" and used ir-keytable
>> to load a given map from user-space, depending on which RC I use.
>>
>> Mans, Sean, what do you think?
> 
> The property is documented as common for IR receivers although only a
> few drivers seem to actually implement the feature.  Since driver
> support is trivial, I see no reason to skip it.  Presumably someone
> had a use for it, or it wouldn't have been added.

I do not dispute the usefulness of the "linux,rc-map-name" property
in general, e.g. for boards that support a single remote control.

I am arguing that the person writing the device tree has no way of
knowing which rc-map a given user will be using, because it depends
on the actual remote control being used.

Maybe I'm missing something.

Regards.

Re: [PATCH v13 06/25] omap3isp: Use generic parser for parsing fwnode endpoints

[Sakari Ailus]

Hi Laurent,

Thanks for the review!

On Tue, Sep 19, 2017 at 02:40:29PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 17:17:05 EEST Sakari Ailus wrote:
> > Instead of using driver implementation, use
> 
> Did you mean s/using driver implementation/using a driver implementation/ (or 
> perhaps "custom driver implementation") ?

I think "custom driver implementation" best describes this. I'll use it.

> 
> > v4l2_async_notifier_parse_fwnode_endpoints() to parse the fwnode endpoints
> > of the device.
> > 
> > Signed-off-by: Sakari Ailus 
> > Acked-by: Hans Verkuil 
> > ---
> >  drivers/media/platform/omap3isp/isp.c | 115 ++-
> >  drivers/media/platform/omap3isp/isp.h |   5 +-
> >  2 files changed, 37 insertions(+), 83 deletions(-)
> > 
> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index 1a428fe9f070..a546cf774d40
> > 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> 
> [snip]
> 
> > @@ -2256,7 +2210,9 @@ static int isp_probe(struct platform_device *pdev)
> > if (ret)
> > return ret;
> > 
> > -   ret = isp_fwnodes_parse(>dev, >notifier);
> > +   ret = v4l2_async_notifier_parse_fwnode_endpoints(
> > +   >dev, >notifier, sizeof(struct isp_async_subdev),
> > +   isp_fwnode_parse);
> > if (ret < 0)
> 
> The documentation in patch 05/25 states that v4l2_async_notifier_release() 
> should be called even if v4l2_async_notifier_parse_fwnode_endpoints() fails. 
> I 
> don't think that's needed here, so you might want to update the documentation 
> (and possibly the implementation of the function).
> 
> Apart from that,
> 
> Reviewed-by: Laurent Pinchart 

Thanks!

> 
> > return ret;
> > 
> > @@ -2407,6 +2363,7 @@ static int isp_probe(struct platform_device *pdev)
> > __omap3isp_put(isp, false);
> >  error:
> > mutex_destroy(>isp_mutex);
> > +   v4l2_async_notifier_release(>notifier);
> > 
> > return ret;
> >  }
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH v13 07/25] rcar-vin: Use generic parser for parsing fwnode endpoints

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 02:53:16PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 17:17:06 EEST Sakari Ailus wrote:
> > Instead of using driver implementation, use
> 
> Same comment as for patch 06/25.

Will fix.

> 
> > v4l2_async_notifier_parse_fwnode_endpoints() to parse the fwnode endpoints
> > of the device.
> > 
> > Signed-off-by: Sakari Ailus 
> > Acked-by: Hans Verkuil 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 112 -
> >  drivers/media/platform/rcar-vin/rcar-dma.c  |  10 +--
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c |  14 ++--
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |   4 +-
> >  4 files changed, 48 insertions(+), 92 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > b/drivers/media/platform/rcar-vin/rcar-core.c index
> > 142de447..62b4a94f9a39 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> 
> [snip]
> 
> > @@ -120,117 +121,70 @@ static int rvin_digital_notify_bound(struct
> 
> [snip]
> 
> > -static int rvin_digitial_parse_v4l2(struct rvin_dev *vin,
> > -   struct device_node *ep,
> > -   struct v4l2_mbus_config *mbus_cfg)
> > +static int rvin_digital_parse_v4l2(struct device *dev,
> > +  struct v4l2_fwnode_endpoint *vep,
> > +  struct v4l2_async_subdev *asd)
> >  {
> > -   struct v4l2_fwnode_endpoint v4l2_ep;
> > -   int ret;
> > +   struct rvin_dev *vin = dev_get_drvdata(dev);
> 
> Doesn't this show that we miss a context argument to the callback function ? 
> Storing the context in device driver data is probably OK if the driver 
> parsing 
> the endpoints controls the struct device, but is that always the case ?

How does a driver know the hardware other than, uh, the device?

I guess we could add a private pointer when the async notifier is
registered if there's a real need for it. The notifier could be an
alternative but it wouldn't be applicable to sub-devices.

> 
> > +   struct rvin_graph_entity *rvge =
> > +   container_of(asd, struct rvin_graph_entity, asd);
> > 
> > -   ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), _ep);
> > -   if (ret) {
> > -   vin_err(vin, "Could not parse v4l2 endpoint\n");
> > -   return -EINVAL;
> > -   }
> > +   if (vep->base.port || vep->base.id)
> > +   return -ENOTCONN;
> > 
> > -   mbus_cfg->type = v4l2_ep.bus_type;
> > +   rvge->mbus_cfg.type = vep->bus_type;
> > 
> > -   switch (mbus_cfg->type) {
> > +   switch (rvge->mbus_cfg.type) {
> > case V4L2_MBUS_PARALLEL:
> > vin_dbg(vin, "Found PARALLEL media bus\n");
> > -   mbus_cfg->flags = v4l2_ep.bus.parallel.flags;
> > +   rvge->mbus_cfg.flags = vep->bus.parallel.flags;
> > break;
> > case V4L2_MBUS_BT656:
> > vin_dbg(vin, "Found BT656 media bus\n");
> > -   mbus_cfg->flags = 0;
> > +   rvge->mbus_cfg.flags = 0;
> > break;
> > default:
> > vin_err(vin, "Unknown media bus type\n");
> > return -EINVAL;
> > }
> > 
> > -   return 0;
> > -}
> > -
> > -static int rvin_digital_graph_parse(struct rvin_dev *vin)
> > -{
> > -   struct device_node *ep, *np;
> > -   int ret;
> > -
> > -   vin->digital.asd.match.fwnode.fwnode = NULL;
> > -   vin->digital.subdev = NULL;
> > -
> > -   /*
> > -* Port 0 id 0 is local digital input, try to get it.
> > -* Not all instances can or will have this, that is OK
> > -*/
> > -   ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
> > -   if (!ep)
> > -   return 0;
> > -
> > -   np = of_graph_get_remote_port_parent(ep);
> > -   if (!np) {
> > -   vin_err(vin, "No remote parent for digital input\n");
> > -   of_node_put(ep);
> > -   return -EINVAL;
> > -   }
> > -   of_node_put(np);
> > -
> > -   ret = rvin_digitial_parse_v4l2(vin, ep, >digital.mbus_cfg);
> > -   of_node_put(ep);
> > -   if (ret)
> > -   return ret;
> > -
> > -   vin->digital.asd.match.fwnode.fwnode = of_fwnode_handle(np);
> > -   vin->digital.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +   vin->digital = rvge;
> > 
> > return 0;
> >  }
> > 
> >  static int rvin_digital_graph_init(struct rvin_dev *vin)
> >  {
> > -   struct v4l2_async_subdev **subdevs = NULL;
> > int ret;
> > 
> > -   ret = rvin_digital_graph_parse(vin);
> > +   ret = v4l2_async_notifier_parse_fwnode_endpoints(
> > +   vin->dev, >notifier,
> > +   sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
> > if (ret)
> > return ret;
> > 
> > -   if (!vin->digital.asd.match.fwnode.fwnode) {
> > -   vin_dbg(vin, "No digital subdevice found\n");
> > -   return -ENODEV;
> > -   }
> > 

Re: [PATCH v13 21/25] smiapp: Add support for flash and lens devices

[Laurent Pinchart]

Hi Sakari,

On Tuesday, 19 September 2017 15:20:34 EEST Sakari Ailus wrote:
> On Tue, Sep 19, 2017 at 03:08:25PM +0300, Laurent Pinchart wrote:
> > On Friday, 15 September 2017 17:17:20 EEST Sakari Ailus wrote:
> >> Parse async sub-devices by using
> >> v4l2_subdev_fwnode_reference_parse_sensor_common().
> >> 
> >> These types devices aren't directly related to the sensor, but are
> >> nevertheless handled by the smiapp driver due to the relationship of
> >> these component to the main part of the camera module --- the sensor.
> >> 
> >> This does not yet address providing the user space with information on
> >> how to associate the sensor or lens devices but the kernel now has the
> >> necessary information to do that.
> >> 
> >> Signed-off-by: Sakari Ailus 
> >> Acked-by: Hans Verkuil 
> >> Acked-by: Pavel Machek 
> > 
> > Something is bothering me here, not so much in the contents of the patch
> > itself than in its nature. There are four patches in this series that add
> > support for flash and lens devices to the smiapp, et8ek8, ov5670 and
> > ov13858 drivers. We have way more sensor drivers than that, and I don't
> > really want to patch them all to support flash and lens. I believe a less
> > intrusive approach, more focussed on the V4L2 core, is needed.
> 
> You could move this to the framework, yes. Nothing prevents that. Perhaps a
> flag telling the framework whether this should be done? Just being
> opportunistic might work as well.

How is would be done is another matter which I haven't thought about yet :-) 
My point is that I don't think converting all drivers is a good idea, you 
should aim for an implementation in the core.

> >> ---
> >> 
> >>  drivers/media/i2c/smiapp/smiapp-core.c | 38 ---
> >>  drivers/media/i2c/smiapp/smiapp.h  |  4 +++-
> >>  2 files changed, 33 insertions(+), 9 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 05/25] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Laurent Pinchart]

Hi Sakari,

On Tuesday, 19 September 2017 15:11:32 EEST Sakari Ailus wrote:
> On Tue, Sep 19, 2017 at 02:35:01PM +0300, Laurent Pinchart wrote:
> > On Friday, 15 September 2017 17:17:04 EEST Sakari Ailus wrote:
> >> Add two functions for parsing devices graph endpoints:
> >> v4l2_async_notifier_parse_fwnode_endpoints and
> >> v4l2_async_notifier_parse_fwnode_endpoints_by_port. The former iterates
> >> over all endpoints whereas the latter only iterates over the endpoints
> >> in a given port.
> >> 
> >> The former is mostly useful for existing drivers that currently
> >> implement the iteration over all the endpoints themselves whereas the
> >> latter is especially intended for devices with both sinks and sources:
> >> async sub-devices for external devices connected to the device's sources
> >> will have already been set up, or they are part of the master device.
> > 
> > Did you mean s/or they/as they/ ?
> 
> No. There are two options here: either the sub-devices a sub-device is
> connected to (through a graph endpoint) is instantiated through the async
> framework *or* through the master device driver. But not by both of them at
> the same time.

The message is then contradicting itself:

"async sub-devices for external devices connected to the device's sources will 
have already been set up, or they are part of the master device."

They refers to "async sub-devices". If they're part of the master device, 
they're not async sub-devices.

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 1/2] dt-bindings: adi,adv7511.txt: document cec clock

[Sergei Shtylyov]

On 09/19/2017 01:59 PM, Hans Verkuil wrote:


From: Hans Verkuil 

Document the cec clock binding.

Signed-off-by: Hans Verkuil 
Acked-by: Rob Herring 
---
  Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 4 
  1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt 
b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 06668bca7ffc..4497ae054d49 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -68,6 +68,8 @@ Optional properties:
  - adi,disable-timing-generator: Only for ADV7533. Disables the internal 
timing
generator. The chip will rely on the sync signals in the DSI data lanes,
rather than generate its own timings for HDMI output.
+- clocks: from common clock binding: handle to CEC clock.


It's called "phandle" in the DT speak. :-)
Are you sure the clock specifier would always be absent?


Sorry? I don't understand the question. Did you mean: "can be absent?"?


   No, you only say that there'll be the clock phandle only. The clock
specifier may follow the phandle for the clock devices that have
"#clock-cells" prop != 0.


I have to say that I just copy-and-pasted this from other bindings.


  :-)


Would this be better?

- clocks: list of clock specifiers, corresponding to entries in
 the clock-names property;


  Didn't you say that there'll be only one clock, "cec"? If so, there's
gonna  be a single clock phandle+specifier pair. They always go in pairs. :-)


- clock-names: from common clock binding: must be "cec".


- clocks: cec clock phandle, corresponding to the clock-names entry.


 The clock phandle and specifier.


- clock-names: from common clock binding: must be "cec".

This OK?


 Well, you seem to be going in circles, the above was almost the same as
the original prop description...


- clocks: from common clock binding: reference to the CEC clock.
- clock-names: from common clock binding: must be "cec".

This avoids the whole issue of having just a phandle or a phandle + specifier.


   OK, let's go with this one. Thank you!


Regards,

Hans


MBR, Sergei

Re: [PATCH] media: default for RC_CORE should be n

[Geert Uytterhoeven]

On Fri, Sep 8, 2017 at 6:39 PM, Stephen Hemminger
 wrote:
> The Linus policy on Kconfig is that the default should be no
> for all new devices. I.e the user rebuild a new kernel from an
> old config should not by default get a larger kernel.
>
> Fixes: b4c184e506a4 ("[media] media: reorganize the main Kconfig items")
> Signed-off-by: Stephen Hemminger 
> ---
>  drivers/media/rc/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index d9ce8ff55d0c..5aa384afcfef 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -2,7 +2,7 @@
>  menuconfig RC_CORE
> tristate "Remote Controller support"
> depends on INPUT
> -   default y
> +   default n

"default n" is the default, so you can just drop this line.

For the principle:
Acked-by: Geert Uytterhoeven 

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Måns Rullgård]

Marc Gonzalez  writes:

> On 18/09/2017 17:33, Måns Rullgård wrote:
>
>> What have you changed compared to my original code?
>
> I forgot to mention one change you may not approve of, so we should
> probably discuss it.
>
> Your driver supported an optional DT property "linux,rc-map-name"
> to override the RC_MAP_EMPTY map. Since the IR decoder supports
> multiple protocols, I found it odd to specify a scancode map in
> something as low-level as the device tree.
>
> I saw only one board using that property:
> $ git grep "linux,rc-map-name" arch/
> arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts: 
> linux,rc-map-name = "rc-geekbox";
>
> So I removed support for "linux,rc-map-name" and used ir-keytable
> to load a given map from user-space, depending on which RC I use.
>
> Mans, Sean, what do you think?

The property is documented as common for IR receivers although only a
few drivers seem to actually implement the feature.  Since driver
support is trivial, I see no reason to skip it.  Presumably someone
had a use for it, or it wouldn't have been added.

-- 
Måns Rullgård

Re: [PATCH v13 21/25] smiapp: Add support for flash and lens devices

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 03:08:25PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 17:17:20 EEST Sakari Ailus wrote:
> > Parse async sub-devices by using
> > v4l2_subdev_fwnode_reference_parse_sensor_common().
> > 
> > These types devices aren't directly related to the sensor, but are
> > nevertheless handled by the smiapp driver due to the relationship of these
> > component to the main part of the camera module --- the sensor.
> > 
> > This does not yet address providing the user space with information on how
> > to associate the sensor or lens devices but the kernel now has the
> > necessary information to do that.
> > 
> > Signed-off-by: Sakari Ailus 
> > Acked-by: Hans Verkuil 
> > Acked-by: Pavel Machek 
> 
> Something is bothering me here, not so much in the contents of the patch 
> itself than in its nature. There are four patches in this series that add 
> support for flash and lens devices to the smiapp, et8ek8, ov5670 and ov13858 
> drivers. We have way more sensor drivers than that, and I don't really want 
> to 
> patch them all to support flash and lens. I believe a less intrusive 
> approach, 
> more focussed on the V4L2 core, is needed.

You could move this to the framework, yes. Nothing prevents that. Perhaps a
flag telling the framework whether this should be done? Just being
opportunistic might work as well.

> 
> > ---
> >  drivers/media/i2c/smiapp/smiapp-core.c | 38 ---
> >  drivers/media/i2c/smiapp/smiapp.h  |  4 +++-
> >  2 files changed, 33 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> > b/drivers/media/i2c/smiapp/smiapp-core.c index 700f433261d0..a4735a96ea41
> > 100644
> > --- a/drivers/media/i2c/smiapp/smiapp-core.c
> > +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> > @@ -31,7 +31,7 @@
> >  #include 
> >  #include 
> >  #include 
> > -#include 
> > +#include 
> >  #include 
> >  #include 
> > 
> > @@ -2887,17 +2887,24 @@ static int smiapp_probe(struct i2c_client *client,
> > v4l2_i2c_subdev_init(>src->sd, client, _ops);
> > sensor->src->sd.internal_ops = _internal_src_ops;
> > 
> > +   rval = v4l2_async_notifier_parse_fwnode_sensor_common(
> > +   >dev, >notifier);
> > +   if (rval < 0)
> > +   return rval;
> > +
> > sensor->vana = devm_regulator_get(>dev, "vana");
> > if (IS_ERR(sensor->vana)) {
> > dev_err(>dev, "could not get regulator for vana\n");
> > -   return PTR_ERR(sensor->vana);
> > +   rval = PTR_ERR(sensor->vana);
> > +   goto out_release_async_notifier;
> > }
> > 
> > sensor->ext_clk = devm_clk_get(>dev, NULL);
> > if (IS_ERR(sensor->ext_clk)) {
> > dev_err(>dev, "could not get clock (%ld)\n",
> > PTR_ERR(sensor->ext_clk));
> > -   return -EPROBE_DEFER;
> > +   rval = -EPROBE_DEFER;
> > +   goto out_release_async_notifier;
> > }
> > 
> > rval = clk_set_rate(sensor->ext_clk, sensor->hwcfg->ext_clk);
> > @@ -2905,17 +2912,19 @@ static int smiapp_probe(struct i2c_client *client,
> > dev_err(>dev,
> > "unable to set clock freq to %u\n",
> > sensor->hwcfg->ext_clk);
> > -   return rval;
> > +   goto out_release_async_notifier;
> > }
> > 
> > sensor->xshutdown = devm_gpiod_get_optional(>dev, "xshutdown",
> > GPIOD_OUT_LOW);
> > -   if (IS_ERR(sensor->xshutdown))
> > -   return PTR_ERR(sensor->xshutdown);
> > +   if (IS_ERR(sensor->xshutdown)) {
> > +   rval = PTR_ERR(sensor->xshutdown);
> > +   goto out_release_async_notifier;
> > +   }
> > 
> > rval = smiapp_power_on(>dev);
> > if (rval < 0)
> > -   return rval;
> > +   goto out_release_async_notifier;
> > 
> > rval = smiapp_identify_module(sensor);
> > if (rval) {
> > @@ -3092,9 +3101,14 @@ static int smiapp_probe(struct i2c_client *client,
> > if (rval < 0)
> > goto out_media_entity_cleanup;
> > 
> > +   rval = v4l2_async_subdev_notifier_register(>src->sd,
> > +  >notifier);
> > +   if (rval)
> > +   goto out_media_entity_cleanup;
> > +
> > rval = v4l2_async_register_subdev(>src->sd);
> > if (rval < 0)
> > -   goto out_media_entity_cleanup;
> > +   goto out_unregister_async_notifier;
> > 
> > pm_runtime_set_active(>dev);
> > pm_runtime_get_noresume(>dev);
> > @@ -3105,6 +3119,9 @@ static int smiapp_probe(struct i2c_client *client,
> > 
> > return 0;
> > 
> > +out_unregister_async_notifier:
> > +   v4l2_async_notifier_unregister(>notifier);
> > +
> >  out_media_entity_cleanup:
> > media_entity_cleanup(>src->sd.entity);
> > 
> > @@ -3114,6 +3131,9 @@ static int smiapp_probe(struct 

Re: [PATCH v13 17/25] v4l: fwnode: Add a helper function for parsing generic references

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:16 EEST Sakari Ailus wrote:
> Add function v4l2_fwnode_reference_parse() for parsing them as async
> sub-devices. This can be done on e.g. flash or lens async sub-devices that
> are not part of but are associated with a sensor.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 69 
>  1 file changed, 69 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> b/drivers/media/v4l2-core/v4l2-fwnode.c index 44ee35f6aad5..65e84ea1cc35
> 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -498,6 +498,75 @@ int v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> }
>  EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints_by_port);
> 
> +/*
> + * v4l2_fwnode_reference_parse - parse references for async sub-devices
> + * @dev: the device node the properties of which are parsed for references
> + * @notifier: the async notifier where the async subdevs will be added
> + * @prop: the name of the property
> + *
> + * Return: 0 on success
> + *  -ENOENT if no entries were found
> + *  -ENOMEM if memory allocation failed
> + *  -EINVAL if property parsing failed
> + */
> +static int v4l2_fwnode_reference_parse(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + const char *prop)
> +{
> + struct fwnode_reference_args args;
> + unsigned int index;
> + int ret;
> +
> + for (index = 0;
> +  !(ret = fwnode_property_get_reference_args(
> +dev_fwnode(dev), prop, NULL, 0, index, ));
> +  index++)
> + fwnode_handle_put(args.fwnode);

This seems to indicate that the fwnode API is missing a function to count the 
number of references in a property. Should that be fixed ?

The rest looks OK to me.

> + if (!index)
> + return -ENOENT;
> +
> + /*
> +  * Note that right now both -ENODATA and -ENOENT may signal
> +  * out-of-bounds access. Return the error in cases other than that.
> +  */
> + if (ret != -ENOENT && ret != -ENODATA)
> + return ret;
> +
> + ret = v4l2_async_notifier_realloc(notifier,
> +   notifier->num_subdevs + index);
> + if (ret)
> + return ret;
> +
> + for (index = 0; !fwnode_property_get_reference_args(
> +  dev_fwnode(dev), prop, NULL, 0, index, );
> +  index++) {
> + struct v4l2_async_subdev *asd;
> +
> + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {
> + ret = -EINVAL;
> + goto error;
> + }
> +
> + asd = kzalloc(sizeof(*asd), GFP_KERNEL);
> + if (!asd) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + notifier->subdevs[notifier->num_subdevs] = asd;
> + asd->match.fwnode.fwnode = args.fwnode;
> + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> + notifier->num_subdevs++;
> + }
> +
> + return 0;
> +
> +error:
> + fwnode_handle_put(args.fwnode);
> + return ret;
> +}
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Sakari Ailus ");
>  MODULE_AUTHOR("Sylwester Nawrocki ");


-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 15/25] dt: bindings: Add a binding for flash LED devices associated to a sensor

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 03:12:42PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 17:17:14 EEST Sakari Ailus wrote:
> > Camera flash drivers (and LEDs) are separate from the sensor devices in
> > DT. In order to make an association between the two, provide the
> > association information to the software.
> > 
> > Signed-off-by: Sakari Ailus 
> > Acked-by: Rob Herring 
> > Acked-by: Hans Verkuil 
> > Acked-by: Pavel Machek 
> > ---
> >  Documentation/devicetree/bindings/media/video-interfaces.txt | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > b/Documentation/devicetree/bindings/media/video-interfaces.txt index
> > 852041a7480c..fdba30479b47 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -67,6 +67,14 @@ are required in a relevant parent node:
> > identifier, should be 1.
> >   - #size-cells: should be zero.
> > 
> > +
> > +Optional properties
> > +---
> > +
> > +- flash-leds: An array of phandles, each referring to a flash LED, a
> > sub-node
> > +  of the LED driver device node.
> 
> What happens with non-LED flash controllers ?

We don't have any at the moment.

The way the bindings are currently defined (LED references are to
individual LEDs for instance) are specific to LED bindings. I'd rather not
make assumptions for e.g. Xenon flash devices. Which might never appear:
LED luminosity, efficiency and maximum current has been steadily increasing
over the past years.

-- 
Regards,

Sakari Ailus
sakari.ai...@linux.intel.com

[PATCH] Fix for hanging si2168 in PCTV 292e, making the code match

[Nigel Kettlewell]

[re-sending as plain text]

Fix for hanging si2168 in PCTV 292e USB, making the code match the comment.

Using firmware v4.0.11 the 292e would work once and then hang on 
subsequent attempts to view DVB channels, until physically unplugged and 
plugged back in.


With this patch, the warm state is reset for v4.0.11 and it appears to 
work both on the first attempt and on subsequent attempts.


(Patch basis Linux 4.11.9 f82a53b87594f460f2dd9983eeb851a5840e8df8)

---
 drivers/media/dvb-frontends/si2168.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/si2168.c 
b/drivers/media/dvb-frontends/si2168.c

index 680ba06..523acd1 100644
--- a/drivers/media/dvb-frontends/si2168.c
+++ b/drivers/media/dvb-frontends/si2168.c
@@ -582,7 +582,7 @@ static int si2168_sleep(struct dvb_frontend *fe)
dev->active = false;

/* Firmware B 4.0-11 or later loses warm state during sleep */
-   if (dev->version > ('B' << 24 | 4 << 16 | 0 << 8 | 11 << 0))
+   if (dev->version >= ('B' << 24 | 4 << 16 | 0 << 8 | 11 << 0))
dev->warm = false;

memcpy(cmd.args, "\x13", 1);
--
2.9.4

Re: [PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 03:01:14PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 17:17:10 EEST Sakari Ailus wrote:
> > Add three helper functions to call async operations callbacks. Besides
> > simplifying callbacks, this allows async notifiers to have no ops set,
> > i.e. it can be left NULL.
> > 
> > Signed-off-by: Sakari Ailus 
> > Acked-by: Hans Verkuil 
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 49 
> >  include/media/v4l2-async.h   |  1 +
> >  2 files changed, 37 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c index 7b2125b3d62f..c35d04b9122f
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -25,6 +25,34 @@
> >  #include 
> >  #include 
> > 
> > +static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier *n,
> > + struct v4l2_subdev *subdev,
> > + struct v4l2_async_subdev *asd)
> > +{
> > +   if (!n->ops || !n->ops->bound)
> > +   return 0;
> > +
> > +   return n->ops->bound(n, subdev, asd);
> > +}
> > +
> > +static void v4l2_async_notifier_call_unbind(struct v4l2_async_notifier *n,
> > +   struct v4l2_subdev *subdev,
> > +   struct v4l2_async_subdev *asd)
> > +{
> > +   if (!n->ops || !n->ops->unbind)
> > +   return;
> > +
> > +   n->ops->unbind(n, subdev, asd);
> > +}
> > +
> > +static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier *n)
> > +{
> > +   if (!n->ops || !n->ops->complete)
> > +   return 0;
> > +
> > +   return n->ops->complete(n);
> > +}
> > +
> 
> Wouldn't it be enough to add a single v4l2_async_notifier_call() macro ?
> 
> #define v4l2_async_notifier_call(n, op, args...) \
>   ((n)->ops && (n)->ops->op ? (n)->ops->op(n, ##args) : 0)

I actually had that in an earlier version but I changed it based on review
comments from Hans. A single macro isn't enough: some functions have int
return type. I think the way it is now is nicer.

> 
> Apart from that,
> 
> Reviewed-by: Laurent Pinchart 
> 
> >  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev
> > *asd) {
> >  #if IS_ENABLED(CONFIG_I2C)
> > @@ -102,16 +130,13 @@ static int v4l2_async_match_notify(struct
> > v4l2_async_notifier *notifier, {
> > int ret;
> > 
> > -   if (notifier->ops->bound) {
> > -   ret = notifier->ops->bound(notifier, sd, asd);
> > -   if (ret < 0)
> > -   return ret;
> > -   }
> > +   ret = v4l2_async_notifier_call_bound(notifier, sd, asd);
> > +   if (ret < 0)
> > +   return ret;
> > 
> > ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> > if (ret < 0) {
> > -   if (notifier->ops->unbind)
> > -   notifier->ops->unbind(notifier, sd, asd);
> > +   v4l2_async_notifier_call_unbind(notifier, sd, asd);
> > return ret;
> > }
> > 
> > @@ -123,8 +148,8 @@ static int v4l2_async_match_notify(struct
> > v4l2_async_notifier *notifier, /* Move from the global subdevice list to
> > notifier's done */
> > list_move(>async_list, >done);
> > 
> > -   if (list_empty(>waiting) && notifier->ops->complete)
> > -   return notifier->ops->complete(notifier);
> > +   if (list_empty(>waiting))
> > +   return v4l2_async_notifier_call_complete(notifier);
> > 
> > return 0;
> >  }
> > @@ -210,8 +235,7 @@ void v4l2_async_notifier_unregister(struct
> > v4l2_async_notifier *notifier) list_for_each_entry_safe(sd, tmp,
> > >done, async_list) { v4l2_async_cleanup(sd);
> > 
> > -   if (notifier->ops->unbind)
> > -   notifier->ops->unbind(notifier, sd, sd->asd);
> > +   v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
> > }
> > 
> > mutex_unlock(_lock);
> > @@ -300,8 +324,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev
> > *sd)
> > 
> > v4l2_async_cleanup(sd);
> > 
> > -   if (notifier->ops->unbind)
> > -   notifier->ops->unbind(notifier, sd, sd->asd);
> > +   v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
> > 
> > mutex_unlock(_lock);
> >  }
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index 3c48f8b66d12..3bc8a7c0d83f 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -164,4 +164,5 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd);
> >   * @sd: pointer to  v4l2_subdev
> >   */
> >  void v4l2_async_unregister_subdev(struct v4l2_subdev *sd);
> > +
> >  #endif
> 
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH v13 15/25] dt: bindings: Add a binding for flash LED devices associated to a sensor

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:14 EEST Sakari Ailus wrote:
> Camera flash drivers (and LEDs) are separate from the sensor devices in
> DT. In order to make an association between the two, provide the
> association information to the software.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Rob Herring 
> Acked-by: Hans Verkuil 
> Acked-by: Pavel Machek 
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt
> b/Documentation/devicetree/bindings/media/video-interfaces.txt index
> 852041a7480c..fdba30479b47 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -67,6 +67,14 @@ are required in a relevant parent node:
>   identifier, should be 1.
>   - #size-cells: should be zero.
> 
> +
> +Optional properties
> +---
> +
> +- flash-leds: An array of phandles, each referring to a flash LED, a
> sub-node
> +  of the LED driver device node.

What happens with non-LED flash controllers ?

> +
> +
>  Optional endpoint properties
>  


-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 05/25] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 02:35:01PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.

Thanks for the review!

> 
> On Friday, 15 September 2017 17:17:04 EEST Sakari Ailus wrote:
> > Add two functions for parsing devices graph endpoints:
> > v4l2_async_notifier_parse_fwnode_endpoints and
> > v4l2_async_notifier_parse_fwnode_endpoints_by_port. The former iterates
> > over all endpoints whereas the latter only iterates over the endpoints in
> > a given port.
> > 
> > The former is mostly useful for existing drivers that currently implement
> > the iteration over all the endpoints themselves whereas the latter is
> > especially intended for devices with both sinks and sources: async
> > sub-devices for external devices connected to the device's sources will
> > have already been set up, or they are part of the master device.
> 
> Did you mean s/or they/as they/ ?

No. There are two options here: either the sub-devices a sub-device is
connected to (through a graph endpoint) is instantiated through the async
framework *or* through the master device driver. But not by both of them at
the same time.

> 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c  |  30 ++
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 185 +++
> >  include/media/v4l2-async.h|  24 -
> >  include/media/v4l2-fwnode.h   | 117 +
> >  4 files changed, 354 insertions(+), 2 deletions(-)
> 
> [snip]
> 
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > b/drivers/media/v4l2-core/v4l2-fwnode.c index 706f9e7b90f1..44ee35f6aad5
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> 
> [snip]
> 
> > +static int v4l2_async_notifier_fwnode_parse_endpoint(
> > +   struct device *dev, struct v4l2_async_notifier *notifier,
> > +   struct fwnode_handle *endpoint, unsigned int asd_struct_size,
> > +   int (*parse_endpoint)(struct device *dev,
> > +   struct v4l2_fwnode_endpoint *vep,
> > +   struct v4l2_async_subdev *asd))
> > +{
> > +   struct v4l2_async_subdev *asd;
> > +   struct v4l2_fwnode_endpoint *vep;
> > +   int ret = 0;
> > +
> > +   asd = kzalloc(asd_struct_size, GFP_KERNEL);
> > +   if (!asd)
> > +   return -ENOMEM;
> > +
> > +   asd->match.fwnode.fwnode =
> > +   fwnode_graph_get_remote_port_parent(endpoint);
> > +   if (!asd->match.fwnode.fwnode) {
> > +   dev_warn(dev, "bad remote port parent\n");
> > +   ret = -EINVAL;
> > +   goto out_err;
> > +   }
> > +
> > +   /* Ignore endpoints the parsing of which failed. */
> 
> I think this comment is outdated.

Hmm. Yes. I'll remove it.

> 
> > +   vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> > +   if (IS_ERR(vep)) {
> > +   ret = PTR_ERR(vep);
> > +   dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
> > +ret);
> > +   goto out_err;
> > +   }
> > +
> > +   ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;
> > +   if (ret == -ENOTCONN)
> > +   dev_dbg(dev, "ignoring endpoint %u,%u\n", vep->base.port,
> > +   vep->base.id);
> 
> How about "ignoring port@%u/endpoint@%u\n" ? It would make the message more 
> explicit.

Works for me.

> 
> > +   else if (ret < 0)
> > +   dev_warn(dev, "driver could not parse endpoint %u,%u (%d)\n",
> 
> Same here.
> 
> > +vep->base.port, vep->base.id, ret);
> > +   v4l2_fwnode_endpoint_free(vep);
> > +   if (ret < 0)
> > +   goto out_err;
> > +
> > +   asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> 
> I'd move this line right before setting asd->match.fwnode.fwnode.

I'll move it.

> 
> > +   notifier->subdevs[notifier->num_subdevs] = asd;
> > +   notifier->num_subdevs++;
> > +
> > +   return 0;
> > +
> > +out_err:
> > +   fwnode_handle_put(asd->match.fwnode.fwnode);
> > +   kfree(asd);
> > +
> > +   return ret == -ENOTCONN ? 0 : ret;
> > +}
> > +
> > +static int __v4l2_async_notifier_parse_fwnode_endpoints(
> > +   struct device *dev, struct v4l2_async_notifier *notifier,
> > +   size_t asd_struct_size, unsigned int port, bool has_port,
> > +   int (*parse_endpoint)(struct device *dev,
> > +   struct v4l2_fwnode_endpoint *vep,
> > +   struct v4l2_async_subdev *asd))
> > +{
> > +   struct fwnode_handle *fwnode = NULL;
> > +   unsigned int max_subdevs = notifier->max_subdevs;
> > +   int ret;
> > +
> > +   if (WARN_ON(asd_struct_size < sizeof(struct v4l2_async_subdev)))
> > +   return -EINVAL;
> > +
> > +   for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> > +dev_fwnode(dev), fwnode)); ) {
> > +   if (!fwnode_device_is_available(
> > +   fwnode_graph_get_port_parent(fwnode)))
> 
> Doesn't fwnode_graph_get_port_parent() 

Re: [PATCH v13 21/25] smiapp: Add support for flash and lens devices

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:20 EEST Sakari Ailus wrote:
> Parse async sub-devices by using
> v4l2_subdev_fwnode_reference_parse_sensor_common().
> 
> These types devices aren't directly related to the sensor, but are
> nevertheless handled by the smiapp driver due to the relationship of these
> component to the main part of the camera module --- the sensor.
> 
> This does not yet address providing the user space with information on how
> to associate the sensor or lens devices but the kernel now has the
> necessary information to do that.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 
> Acked-by: Pavel Machek 

Something is bothering me here, not so much in the contents of the patch 
itself than in its nature. There are four patches in this series that add 
support for flash and lens devices to the smiapp, et8ek8, ov5670 and ov13858 
drivers. We have way more sensor drivers than that, and I don't really want to 
patch them all to support flash and lens. I believe a less intrusive approach, 
more focussed on the V4L2 core, is needed.

> ---
>  drivers/media/i2c/smiapp/smiapp-core.c | 38 ---
>  drivers/media/i2c/smiapp/smiapp.h  |  4 +++-
>  2 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> b/drivers/media/i2c/smiapp/smiapp-core.c index 700f433261d0..a4735a96ea41
> 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -31,7 +31,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
> 
> @@ -2887,17 +2887,24 @@ static int smiapp_probe(struct i2c_client *client,
>   v4l2_i2c_subdev_init(>src->sd, client, _ops);
>   sensor->src->sd.internal_ops = _internal_src_ops;
> 
> + rval = v4l2_async_notifier_parse_fwnode_sensor_common(
> + >dev, >notifier);
> + if (rval < 0)
> + return rval;
> +
>   sensor->vana = devm_regulator_get(>dev, "vana");
>   if (IS_ERR(sensor->vana)) {
>   dev_err(>dev, "could not get regulator for vana\n");
> - return PTR_ERR(sensor->vana);
> + rval = PTR_ERR(sensor->vana);
> + goto out_release_async_notifier;
>   }
> 
>   sensor->ext_clk = devm_clk_get(>dev, NULL);
>   if (IS_ERR(sensor->ext_clk)) {
>   dev_err(>dev, "could not get clock (%ld)\n",
>   PTR_ERR(sensor->ext_clk));
> - return -EPROBE_DEFER;
> + rval = -EPROBE_DEFER;
> + goto out_release_async_notifier;
>   }
> 
>   rval = clk_set_rate(sensor->ext_clk, sensor->hwcfg->ext_clk);
> @@ -2905,17 +2912,19 @@ static int smiapp_probe(struct i2c_client *client,
>   dev_err(>dev,
>   "unable to set clock freq to %u\n",
>   sensor->hwcfg->ext_clk);
> - return rval;
> + goto out_release_async_notifier;
>   }
> 
>   sensor->xshutdown = devm_gpiod_get_optional(>dev, "xshutdown",
>   GPIOD_OUT_LOW);
> - if (IS_ERR(sensor->xshutdown))
> - return PTR_ERR(sensor->xshutdown);
> + if (IS_ERR(sensor->xshutdown)) {
> + rval = PTR_ERR(sensor->xshutdown);
> + goto out_release_async_notifier;
> + }
> 
>   rval = smiapp_power_on(>dev);
>   if (rval < 0)
> - return rval;
> + goto out_release_async_notifier;
> 
>   rval = smiapp_identify_module(sensor);
>   if (rval) {
> @@ -3092,9 +3101,14 @@ static int smiapp_probe(struct i2c_client *client,
>   if (rval < 0)
>   goto out_media_entity_cleanup;
> 
> + rval = v4l2_async_subdev_notifier_register(>src->sd,
> +>notifier);
> + if (rval)
> + goto out_media_entity_cleanup;
> +
>   rval = v4l2_async_register_subdev(>src->sd);
>   if (rval < 0)
> - goto out_media_entity_cleanup;
> + goto out_unregister_async_notifier;
> 
>   pm_runtime_set_active(>dev);
>   pm_runtime_get_noresume(>dev);
> @@ -3105,6 +3119,9 @@ static int smiapp_probe(struct i2c_client *client,
> 
>   return 0;
> 
> +out_unregister_async_notifier:
> + v4l2_async_notifier_unregister(>notifier);
> +
>  out_media_entity_cleanup:
>   media_entity_cleanup(>src->sd.entity);
> 
> @@ -3114,6 +3131,9 @@ static int smiapp_probe(struct i2c_client *client,
>  out_power_off:
>   smiapp_power_off(>dev);
> 
> +out_release_async_notifier:
> + v4l2_async_notifier_release(>notifier);
> +
>   return rval;
>  }
> 
> @@ -3124,6 +3144,8 @@ static int smiapp_remove(struct i2c_client *client)
>   unsigned int i;
> 
>   v4l2_async_unregister_subdev(subdev);
> + v4l2_async_notifier_unregister(>notifier);
> + 

Re: [PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:12 EEST Sakari Ailus wrote:
> The information on how many async sub-devices would be bindable to a
> notifier is typically dependent on information from platform firmware and
> it's not driver's business to be aware of that.
> 
> Many V4L2 main drivers are perfectly usable (and useful) without async
> sub-devices and so if there aren't any around, just proceed call the
> notifier's complete callback immediately without registering the notifier
> itself.
> 
> If a driver needs to check whether there are async sub-devices available,
> it can be done by inspecting the notifier's num_subdevs field which tells
> the number of async sub-devices.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 9895b610e2a0..4be2f16af051
> 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -170,14 +170,16 @@ int v4l2_async_notifier_register(struct v4l2_device
> *v4l2_dev, struct v4l2_async_subdev *asd;
>   int i;
> 
> - if (!v4l2_dev || !notifier->num_subdevs ||
> - notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> + if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
>   return -EINVAL;
> 
>   notifier->v4l2_dev = v4l2_dev;
>   INIT_LIST_HEAD(>waiting);
>   INIT_LIST_HEAD(>done);
> 
> + if (!notifier->num_subdevs)
> + return v4l2_async_notifier_call_complete(notifier);
> +
>   for (i = 0; i < notifier->num_subdevs; i++) {
>   asd = notifier->subdevs[i];


-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 12/25] v4l: async: Register sub-devices before calling bound callback

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:11 EEST Sakari Ailus wrote:
> Register the sub-device before calling the notifier's bound callback.
> Doing this the other way around is problematic as the struct v4l2_device
> has not assigned for the sub-device yet and may be required by the bound
> callback.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index c35d04b9122f..9895b610e2a0
> 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -130,13 +130,13 @@ static int v4l2_async_match_notify(struct
> v4l2_async_notifier *notifier, {
>   int ret;
> 
> - ret = v4l2_async_notifier_call_bound(notifier, sd, asd);
> + ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
>   if (ret < 0)
>   return ret;
> 
> - ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> + ret = v4l2_async_notifier_call_bound(notifier, sd, asd);
>   if (ret < 0) {
> - v4l2_async_notifier_call_unbind(notifier, sd, asd);
> + v4l2_device_unregister_subdev(sd);
>   return ret;
>   }


-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 11/25] v4l: async: Introduce helpers for calling async ops callbacks

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:10 EEST Sakari Ailus wrote:
> Add three helper functions to call async operations callbacks. Besides
> simplifying callbacks, this allows async notifiers to have no ops set,
> i.e. it can be left NULL.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 49 
>  include/media/v4l2-async.h   |  1 +
>  2 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 7b2125b3d62f..c35d04b9122f
> 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -25,6 +25,34 @@
>  #include 
>  #include 
> 
> +static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier *n,
> +   struct v4l2_subdev *subdev,
> +   struct v4l2_async_subdev *asd)
> +{
> + if (!n->ops || !n->ops->bound)
> + return 0;
> +
> + return n->ops->bound(n, subdev, asd);
> +}
> +
> +static void v4l2_async_notifier_call_unbind(struct v4l2_async_notifier *n,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + if (!n->ops || !n->ops->unbind)
> + return;
> +
> + n->ops->unbind(n, subdev, asd);
> +}
> +
> +static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier *n)
> +{
> + if (!n->ops || !n->ops->complete)
> + return 0;
> +
> + return n->ops->complete(n);
> +}
> +

Wouldn't it be enough to add a single v4l2_async_notifier_call() macro ?

#define v4l2_async_notifier_call(n, op, args...) \
((n)->ops && (n)->ops->op ? (n)->ops->op(n, ##args) : 0)

Apart from that,

Reviewed-by: Laurent Pinchart 

>  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) {
>  #if IS_ENABLED(CONFIG_I2C)
> @@ -102,16 +130,13 @@ static int v4l2_async_match_notify(struct
> v4l2_async_notifier *notifier, {
>   int ret;
> 
> - if (notifier->ops->bound) {
> - ret = notifier->ops->bound(notifier, sd, asd);
> - if (ret < 0)
> - return ret;
> - }
> + ret = v4l2_async_notifier_call_bound(notifier, sd, asd);
> + if (ret < 0)
> + return ret;
> 
>   ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
>   if (ret < 0) {
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, asd);
> + v4l2_async_notifier_call_unbind(notifier, sd, asd);
>   return ret;
>   }
> 
> @@ -123,8 +148,8 @@ static int v4l2_async_match_notify(struct
> v4l2_async_notifier *notifier, /* Move from the global subdevice list to
> notifier's done */
>   list_move(>async_list, >done);
> 
> - if (list_empty(>waiting) && notifier->ops->complete)
> - return notifier->ops->complete(notifier);
> + if (list_empty(>waiting))
> + return v4l2_async_notifier_call_complete(notifier);
> 
>   return 0;
>  }
> @@ -210,8 +235,7 @@ void v4l2_async_notifier_unregister(struct
> v4l2_async_notifier *notifier) list_for_each_entry_safe(sd, tmp,
> >done, async_list) { v4l2_async_cleanup(sd);
> 
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, sd->asd);
> + v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
>   }
> 
>   mutex_unlock(_lock);
> @@ -300,8 +324,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev
> *sd)
> 
>   v4l2_async_cleanup(sd);
> 
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, sd->asd);
> + v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
> 
>   mutex_unlock(_lock);
>  }
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 3c48f8b66d12..3bc8a7c0d83f 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -164,4 +164,5 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd);
>   * @sd: pointer to  v4l2_subdev
>   */
>  void v4l2_async_unregister_subdev(struct v4l2_subdev *sd);
> +
>  #endif


-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 09/25] omap3isp: Print the name of the entity where no source pads could be found

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:08 EEST Sakari Ailus wrote:
> If no source pads are found in an entity, print the name of the entity.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 
> Acked-by: Pavel Machek 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/omap3isp/isp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 3b1a9cd0e591..9a694924e46e
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1669,8 +1669,8 @@ static int isp_link_entity(
>   break;
>   }
>   if (i == entity->num_pads) {
> - dev_err(isp->dev, "%s: no source pad in external entity\n",
> - __func__);
> + dev_err(isp->dev, "%s: no source pad in external entity %s\n",
> + __func__, entity->name);
>   return -EINVAL;
>   }


-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 08/25] omap3isp: Fix check for our own sub-devices

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:07 EEST Sakari Ailus wrote:
> We only want to link sub-devices that were bound to the async notifier the
> isp driver registered but there may be other sub-devices in the
> v4l2_device as well. Check for the correct async notifier.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 
> Acked-by: Pavel Machek 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/omap3isp/isp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index a546cf774d40..3b1a9cd0e591
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2155,7 +2155,7 @@ static int isp_subdev_notifier_complete(struct
> v4l2_async_notifier *async) return ret;
> 
>   list_for_each_entry(sd, _dev->subdevs, list) {
> - if (!sd->asd)
> + if (sd->notifier != >notifier)
>   continue;
> 
>   ret = isp_link_entity(isp, >entity,


-- 
Regards,

Laurent Pinchart

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Måns Rullgård]

Marc Gonzalez  writes:

> On 19/09/2017 11:48, Måns Rullgård wrote:
>
>> Did you test the NEC32 variant?  I don't have anything that produces
>> such codes.
>
> I don't have a NEC32 IR remote control either.
>
> IIUC, NEC32 means 16-bit address and 16-bit command.
>
> I checked the RTL with a HW engineer. The HW block translates the IR
> pulses into logical 1s and 0s according to the protocol parameters,
> stuffs the logical bits into a register, and fires an IRQ when there
> are 32 bits available. The block doesn't care if the bits are significant
> or just checksums (that is left up to software).

In that case I suppose it ought to just work.

-- 
Måns Rullgård

Re: [PATCH v13 07/25] rcar-vin: Use generic parser for parsing fwnode endpoints

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:06 EEST Sakari Ailus wrote:
> Instead of using driver implementation, use

Same comment as for patch 06/25.

> v4l2_async_notifier_parse_fwnode_endpoints() to parse the fwnode endpoints
> of the device.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 112 -
>  drivers/media/platform/rcar-vin/rcar-dma.c  |  10 +--
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  14 ++--
>  drivers/media/platform/rcar-vin/rcar-vin.h  |   4 +-
>  4 files changed, 48 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 142de447..62b4a94f9a39 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c

[snip]

> @@ -120,117 +121,70 @@ static int rvin_digital_notify_bound(struct

[snip]

> -static int rvin_digitial_parse_v4l2(struct rvin_dev *vin,
> - struct device_node *ep,
> - struct v4l2_mbus_config *mbus_cfg)
> +static int rvin_digital_parse_v4l2(struct device *dev,
> +struct v4l2_fwnode_endpoint *vep,
> +struct v4l2_async_subdev *asd)
>  {
> - struct v4l2_fwnode_endpoint v4l2_ep;
> - int ret;
> + struct rvin_dev *vin = dev_get_drvdata(dev);

Doesn't this show that we miss a context argument to the callback function ? 
Storing the context in device driver data is probably OK if the driver parsing 
the endpoints controls the struct device, but is that always the case ?

> + struct rvin_graph_entity *rvge =
> + container_of(asd, struct rvin_graph_entity, asd);
> 
> - ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), _ep);
> - if (ret) {
> - vin_err(vin, "Could not parse v4l2 endpoint\n");
> - return -EINVAL;
> - }
> + if (vep->base.port || vep->base.id)
> + return -ENOTCONN;
> 
> - mbus_cfg->type = v4l2_ep.bus_type;
> + rvge->mbus_cfg.type = vep->bus_type;
> 
> - switch (mbus_cfg->type) {
> + switch (rvge->mbus_cfg.type) {
>   case V4L2_MBUS_PARALLEL:
>   vin_dbg(vin, "Found PARALLEL media bus\n");
> - mbus_cfg->flags = v4l2_ep.bus.parallel.flags;
> + rvge->mbus_cfg.flags = vep->bus.parallel.flags;
>   break;
>   case V4L2_MBUS_BT656:
>   vin_dbg(vin, "Found BT656 media bus\n");
> - mbus_cfg->flags = 0;
> + rvge->mbus_cfg.flags = 0;
>   break;
>   default:
>   vin_err(vin, "Unknown media bus type\n");
>   return -EINVAL;
>   }
> 
> - return 0;
> -}
> -
> -static int rvin_digital_graph_parse(struct rvin_dev *vin)
> -{
> - struct device_node *ep, *np;
> - int ret;
> -
> - vin->digital.asd.match.fwnode.fwnode = NULL;
> - vin->digital.subdev = NULL;
> -
> - /*
> -  * Port 0 id 0 is local digital input, try to get it.
> -  * Not all instances can or will have this, that is OK
> -  */
> - ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
> - if (!ep)
> - return 0;
> -
> - np = of_graph_get_remote_port_parent(ep);
> - if (!np) {
> - vin_err(vin, "No remote parent for digital input\n");
> - of_node_put(ep);
> - return -EINVAL;
> - }
> - of_node_put(np);
> -
> - ret = rvin_digitial_parse_v4l2(vin, ep, >digital.mbus_cfg);
> - of_node_put(ep);
> - if (ret)
> - return ret;
> -
> - vin->digital.asd.match.fwnode.fwnode = of_fwnode_handle(np);
> - vin->digital.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> + vin->digital = rvge;
> 
>   return 0;
>  }
> 
>  static int rvin_digital_graph_init(struct rvin_dev *vin)
>  {
> - struct v4l2_async_subdev **subdevs = NULL;
>   int ret;
> 
> - ret = rvin_digital_graph_parse(vin);
> + ret = v4l2_async_notifier_parse_fwnode_endpoints(
> + vin->dev, >notifier,
> + sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
>   if (ret)
>   return ret;
> 
> - if (!vin->digital.asd.match.fwnode.fwnode) {
> - vin_dbg(vin, "No digital subdevice found\n");
> - return -ENODEV;
> - }
> -
> - /* Register the subdevices notifier. */
> - subdevs = devm_kzalloc(vin->dev, sizeof(*subdevs), GFP_KERNEL);
> - if (subdevs == NULL)
> - return -ENOMEM;
> -
> - subdevs[0] = >digital.asd;
> -
> - vin_dbg(vin, "Found digital subdevice %pOF\n",
> - to_of_node(subdevs[0]->match.fwnode.fwnode));
> + if (vin->digital)
> + vin_dbg(vin, "Found digital subdevice %pOF\n",
> + to_of_node(
> + 

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Marc Gonzalez]

On 18/09/2017 17:33, Måns Rullgård wrote:

> What have you changed compared to my original code?

I forgot to mention one change you may not approve of, so we should
probably discuss it.

Your driver supported an optional DT property "linux,rc-map-name"
to override the RC_MAP_EMPTY map. Since the IR decoder supports
multiple protocols, I found it odd to specify a scancode map in
something as low-level as the device tree.

I saw only one board using that property:
$ git grep "linux,rc-map-name" arch/
arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts: 
linux,rc-map-name = "rc-geekbox";

So I removed support for "linux,rc-map-name" and used ir-keytable
to load a given map from user-space, depending on which RC I use.

Mans, Sean, what do you think?

Regards.

Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

Hi Hans.

Thanks for the response.

On 19 September 2017 at 11:20, Hans Verkuil  wrote:
> On 09/19/17 11:50, Dave Stevenson wrote:
>> Hi Eric.
>>
>> Thanks for the review.
>>
>> On 18 September 2017 at 19:18, Eric Anholt  wrote:
>>> Dave Stevenson  writes:
 diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c 
 b/drivers/media/platform/bcm2835/bcm2835-unicam.c
 new file mode 100644
 index 000..5b1adc3
 --- /dev/null
 +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
 @@ -0,0 +1,2192 @@
 +/*
 + * BCM2835 Unicam capture Driver
 + *
 + * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.
 + *
 + * Dave Stevenson 
 + *
 + * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and
 + * TI CAL camera interface driver by Benoit Parrot.
 + *
>>>
>>> Possible future improvement: this description of the driver is really
>>> nice and could be turned into kernel-doc.
>>
>> Documentation?! Surely not :-)
>> For now I'll leave it as a task for another day.
>>
 + * There are two camera drivers in the kernel for BCM283x - this one
 + * and bcm2835-camera (currently in staging).
 + *
 + * This driver is purely the kernel control the Unicam peripheral - there
>>>
>>> Maybe "This driver directly controls..."?
>>
>> Will do in v3.
>>
 + * is no involvement with the VideoCore firmware. Unicam receives CSI-2
 + * or CCP2 data and writes it into SDRAM. The only processing options are
 + * to repack Bayer data into an alternate format, and applying windowing
 + * (currently not implemented).
 + * It should be possible to connect it to any sensor with a
 + * suitable output interface and V4L2 subdevice driver.
 + *
 + * bcm2835-camera uses with the VideoCore firmware to control the sensor,
>>>
>>> "uses the"
>>
>> Will do in v3.
>>
 + * Unicam, ISP, and all tuner control loops. Fully processed frames are
 + * delivered to the driver by the firmware. It only has sensor drivers
 + * for Omnivision OV5647, and Sony IMX219 sensors.
 + *
 + * The two drivers are mutually exclusive for the same Unicam instance.
 + * The VideoCore firmware checks the device tree configuration during 
 boot.
 + * If it finds device tree nodes called csi0 or csi1 it will block the
 + * firmware from accessing the peripheral, and bcm2835-camera will
 + * not be able to stream data.
>>>
>>> Thanks for describing this here!
>>>
 +/*
 + * The peripheral can unpack and repack between several of
 + * the Bayer raw formats, so any Bayer format can be advertised
 + * as the same Bayer order in each of the supported bit depths.
 + * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8
 + * formats.
 + */
 +#define PIX_FMT_ALL_BGGR  v4l2_fourcc('b', 'g', 'g', 'r')
 +#define PIX_FMT_ALL_RGGB  v4l2_fourcc('r', 'g', 'g', 'b')
 +#define PIX_FMT_ALL_GBRG  v4l2_fourcc('g', 'b', 'r', 'g')
 +#define PIX_FMT_ALL_GRBG  v4l2_fourcc('g', 'r', 'b', 'g')
>>>
>>> Should thes fourccs be defined in a common v4l2 header, to reserve it
>>> from clashing with others later?
>>
>> I'm only using them as flags and probably in a manner that nothing
>> else is likely to copy, so it seems a little excessive to put them in
>> a common header.
>> Perhaps it's better to switch to 0xFFF0 to 0xFFF3 or other
>> value that won't come up as a fourcc under any normal circumstance.
>> Any thoughts from other people?
>
> I think that's better, yes.

OK, happy to do that.

>>
>>> This is really the only question I have about this driver before seeing
>>> it merged.  As far as me wearing my platform maintainer hat, I'm happy
>>> with the driver, and my other little notes are optional.
>>>
 +static int unicam_probe(struct platform_device *pdev)
 +{
 + struct unicam_cfg *unicam_cfg;
 + struct unicam_device *unicam;
 + struct v4l2_ctrl_handler *hdl;
 + struct resource *res;
 + int ret;
 +
 + unicam = devm_kzalloc(>dev, sizeof(*unicam), GFP_KERNEL);
 + if (!unicam)
 + return -ENOMEM;
 +
 + unicam->pdev = pdev;
 + unicam_cfg = >cfg;
 +
 + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 + unicam_cfg->base = devm_ioremap_resource(>dev, res);
 + if (IS_ERR(unicam_cfg->base)) {
 + unicam_err(unicam, "Failed to get main io block\n");
 + return PTR_ERR(unicam_cfg->base);
 + }
 +
 + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 + unicam_cfg->clk_gate_base = devm_ioremap_resource(>dev, res);
 + if (IS_ERR(unicam_cfg->clk_gate_base)) {
 + unicam_err(unicam, "Failed to get 2nd io block\n");
 + return PTR_ERR(unicam_cfg->clk_gate_base);
 +   

Re: [PATCH v13 06/25] omap3isp: Use generic parser for parsing fwnode endpoints

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:05 EEST Sakari Ailus wrote:
> Instead of using driver implementation, use

Did you mean s/using driver implementation/using a driver implementation/ (or 
perhaps "custom driver implementation") ?

> v4l2_async_notifier_parse_fwnode_endpoints() to parse the fwnode endpoints
> of the device.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 
> ---
>  drivers/media/platform/omap3isp/isp.c | 115 ++-
>  drivers/media/platform/omap3isp/isp.h |   5 +-
>  2 files changed, 37 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 1a428fe9f070..a546cf774d40
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c

[snip]

> @@ -2256,7 +2210,9 @@ static int isp_probe(struct platform_device *pdev)
>   if (ret)
>   return ret;
> 
> - ret = isp_fwnodes_parse(>dev, >notifier);
> + ret = v4l2_async_notifier_parse_fwnode_endpoints(
> + >dev, >notifier, sizeof(struct isp_async_subdev),
> + isp_fwnode_parse);
>   if (ret < 0)

The documentation in patch 05/25 states that v4l2_async_notifier_release() 
should be called even if v4l2_async_notifier_parse_fwnode_endpoints() fails. I 
don't think that's needed here, so you might want to update the documentation 
(and possibly the implementation of the function).

Apart from that,

Reviewed-by: Laurent Pinchart 

>   return ret;
> 
> @@ -2407,6 +2363,7 @@ static int isp_probe(struct platform_device *pdev)
>   __omap3isp_put(isp, false);
>  error:
>   mutex_destroy(>isp_mutex);
> + v4l2_async_notifier_release(>notifier);
> 
>   return ret;
>  }

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 05/25] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 12:30:34PM +0300, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Tuesday, 19 September 2017 11:40:14 EEST Hans Verkuil wrote:
> > On 09/19/2017 10:20 AM, Sakari Ailus wrote:
> > > On Tue, Sep 19, 2017 at 10:03:27AM +0200, Hans Verkuil wrote:
> > >> On 09/15/2017 04:17 PM, Sakari Ailus wrote:
> > >>> Add two functions for parsing devices graph endpoints:
> > >>> v4l2_async_notifier_parse_fwnode_endpoints and
> > >>> v4l2_async_notifier_parse_fwnode_endpoints_by_port. The former iterates
> > >>> over all endpoints whereas the latter only iterates over the endpoints
> > >>> in a given port.
> > >>> 
> > >>> The former is mostly useful for existing drivers that currently
> > >>> implement the iteration over all the endpoints themselves whereas the
> > >>> latter is especially intended for devices with both sinks and sources:
> > >>> async sub-devices for external devices connected to the device's sources
> > >>> will have already been set up, or they are part of the master device.
> > >>> 
> > >>> Signed-off-by: Sakari Ailus 
> > >>> ---
> > >>> 
> > >>>  drivers/media/v4l2-core/v4l2-async.c  |  30 ++
> > >>>  drivers/media/v4l2-core/v4l2-fwnode.c | 185 +++
> > >>>  include/media/v4l2-async.h|  24 -
> > >>>  include/media/v4l2-fwnode.h   | 117 +
> > >>>  4 files changed, 354 insertions(+), 2 deletions(-)
> 
> [snip]
> 
> > >>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > >>> index 68eb22ba571b..83afac48ea6b 100644
> > >>> --- a/include/media/v4l2-fwnode.h
> > >>> +++ b/include/media/v4l2-fwnode.h
> > >>> @@ -25,6 +25,8 @@
> > >>> 
> > >>>  #include 
> > >>>  
> > >>>  struct fwnode_handle;
> > >>> +struct v4l2_async_notifier;
> > >>> +struct v4l2_async_subdev;
> > >>> 
> > >>>  #define V4L2_FWNODE_CSI2_MAX_DATA_LANES4
> > >>> 
> > >>> @@ -201,4 +203,119 @@ int v4l2_fwnode_parse_link(struct fwnode_handle
> > >>> *fwnode,
> > >>>   */
> > >>>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> > >>> 
> > >>> +/**
> > >>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode
> > >>> endpoints in a
> > >>> + * device node
> > >>> + * @dev: the device the endpoints of which are to be parsed
> > >>> + * @notifier: notifier for @dev
> > >>> + * @asd_struct_size: size of the driver's async sub-device struct,
> > >>> including
> > >>> + *  sizeof(struct v4l2_async_subdev). The 
> > >>> + *  v4l2_async_subdev shall be the first member of
> > >>> + *  the driver's async sub-device struct, i.e. both
> > >>> + *  begin at the same memory address.
> > >>> + * @parse_endpoint: Driver's callback function called on each V4L2
> > >>> fwnode
> > >>> + * endpoint. Optional.
> > >>> + * Return: %0 on success
> > >>> + * %-ENOTCONN if the endpoint is to be skipped 
> > >>> but this
> > >>> + *should not be considered as an 
> > >>> error
> > >>> + * %-EINVAL if the endpoint configuration is 
> > >>> invalid
> > >>> + *
> > >>> + * Parse the fwnode endpoints of the @dev device and populate the async
> > >>> sub-
> > >>> + * devices array of the notifier. The @parse_endpoint callback function
> > >>> is
> > >>> + * called for each endpoint with the corresponding async sub-device
> > >>> pointer to
> > >>> + * let the caller initialize the driver-specific part of the async sub-
> > >>> device
> > >>> + * structure.
> > >>> + *
> > >>> + * The notifier memory shall be zeroed before this function is called
> > >>> on the
> > >>> + * notifier.
> > >>> + *
> > >>> + * This function may not be called on a registered notifier and may be
> > >>> called on
> > >>> + * a notifier only once.
> > >>> + *
> > >>> + * Do not change the notifier's subdevs array, take references to the
> > >>> subdevs
> > >>> + * array itself or change the notifier's num_subdevs field. This is
> > >>> because this
> > >>> + * function allocates and reallocates the subdevs array based on
> > >>> parsing
> > >>> + * endpoints.
> > >>> + *
> > >>> + * The @struct v4l2_fwnode_endpoint passed to the callback function
> > >>> + * @parse_endpoint is released once the function is finished. If there
> > >>> is a need
> > >>> + * to retain that configuration, the user needs to allocate memory for
> > >>> it.
> > >>> + *
> > >>> + * Any notifier populated using this function must be released with a
> > >>> call to
> > >>> + * v4l2_async_notifier_release() after it has been unregistered and the
> > >>> async
> > >>> + * sub-devices are no longer in use, even if the function returned an
> > >>> error.
> > >>> + *
> > >>> + * Return: %0 on success, including when no async sub-devices are found
> > >>> + *%-ENOMEM if memory allocation failed
> > >>> + *

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Marc Gonzalez]

On 19/09/2017 11:48, Måns Rullgård wrote:

> Did you test the NEC32 variant?  I don't have anything that produces
> such codes.

I don't have a NEC32 IR remote control either.

IIUC, NEC32 means 16-bit address and 16-bit command.

I checked the RTL with a HW engineer. The HW block translates the IR
pulses into logical 1s and 0s according to the protocol parameters,
stuffs the logical bits into a register, and fires an IRQ when there
are 32 bits available. The block doesn't care if the bits are significant
or just checksums (that is left up to software).

Regards.

Re: [PATCH v13 05/25] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:04 EEST Sakari Ailus wrote:
> Add two functions for parsing devices graph endpoints:
> v4l2_async_notifier_parse_fwnode_endpoints and
> v4l2_async_notifier_parse_fwnode_endpoints_by_port. The former iterates
> over all endpoints whereas the latter only iterates over the endpoints in
> a given port.
> 
> The former is mostly useful for existing drivers that currently implement
> the iteration over all the endpoints themselves whereas the latter is
> especially intended for devices with both sinks and sources: async
> sub-devices for external devices connected to the device's sources will
> have already been set up, or they are part of the master device.

Did you mean s/or they/as they/ ?

> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/v4l2-core/v4l2-async.c  |  30 ++
>  drivers/media/v4l2-core/v4l2-fwnode.c | 185 +++
>  include/media/v4l2-async.h|  24 -
>  include/media/v4l2-fwnode.h   | 117 +
>  4 files changed, 354 insertions(+), 2 deletions(-)

[snip]

> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> b/drivers/media/v4l2-core/v4l2-fwnode.c index 706f9e7b90f1..44ee35f6aad5
> 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c

[snip]

> +static int v4l2_async_notifier_fwnode_parse_endpoint(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + struct fwnode_handle *endpoint, unsigned int asd_struct_size,
> + int (*parse_endpoint)(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd))
> +{
> + struct v4l2_async_subdev *asd;
> + struct v4l2_fwnode_endpoint *vep;
> + int ret = 0;
> +
> + asd = kzalloc(asd_struct_size, GFP_KERNEL);
> + if (!asd)
> + return -ENOMEM;
> +
> + asd->match.fwnode.fwnode =
> + fwnode_graph_get_remote_port_parent(endpoint);
> + if (!asd->match.fwnode.fwnode) {
> + dev_warn(dev, "bad remote port parent\n");
> + ret = -EINVAL;
> + goto out_err;
> + }
> +
> + /* Ignore endpoints the parsing of which failed. */

I think this comment is outdated.

> + vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> + if (IS_ERR(vep)) {
> + ret = PTR_ERR(vep);
> + dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
> +  ret);
> + goto out_err;
> + }
> +
> + ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;
> + if (ret == -ENOTCONN)
> + dev_dbg(dev, "ignoring endpoint %u,%u\n", vep->base.port,
> + vep->base.id);

How about "ignoring port@%u/endpoint@%u\n" ? It would make the message more 
explicit.

> + else if (ret < 0)
> + dev_warn(dev, "driver could not parse endpoint %u,%u (%d)\n",

Same here.

> +  vep->base.port, vep->base.id, ret);
> + v4l2_fwnode_endpoint_free(vep);
> + if (ret < 0)
> + goto out_err;
> +
> + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;

I'd move this line right before setting asd->match.fwnode.fwnode.

> + notifier->subdevs[notifier->num_subdevs] = asd;
> + notifier->num_subdevs++;
> +
> + return 0;
> +
> +out_err:
> + fwnode_handle_put(asd->match.fwnode.fwnode);
> + kfree(asd);
> +
> + return ret == -ENOTCONN ? 0 : ret;
> +}
> +
> +static int __v4l2_async_notifier_parse_fwnode_endpoints(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + size_t asd_struct_size, unsigned int port, bool has_port,
> + int (*parse_endpoint)(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd))
> +{
> + struct fwnode_handle *fwnode = NULL;
> + unsigned int max_subdevs = notifier->max_subdevs;
> + int ret;
> +
> + if (WARN_ON(asd_struct_size < sizeof(struct v4l2_async_subdev)))
> + return -EINVAL;
> +
> + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> +  dev_fwnode(dev), fwnode)); ) {
> + if (!fwnode_device_is_available(
> + fwnode_graph_get_port_parent(fwnode)))

Doesn't fwnode_graph_get_port_parent() increment the refcount on the parent, 
which you should then release ?

> + continue;
> +
> + if (has_port) {
> + struct fwnode_endpoint ep;
> +
> + ret = fwnode_graph_parse_endpoint(fwnode, );
> + if (ret) {
> + fwnode_handle_put(fwnode);
> + return ret;
> + }
> +
> + if (ep.port != port)
> + continue;
> + }

Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 02:14:39PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday, 19 September 2017 14:10:37 EEST Sakari Ailus wrote:
> > On Tue, Sep 19, 2017 at 01:48:27PM +0300, Laurent Pinchart wrote:
> > > On Friday, 15 September 2017 17:17:00 EEST Sakari Ailus wrote:
> > >> In V4L2 the practice is to have the KernelDoc documentation in the
> > >> header and not in .c source code files. This consequently makes the V4L2
> > >> fwnode function documentation part of the Media documentation build.
> > >> 
> > >> Also correct the link related function and argument naming in
> > >> documentation.
> > >> 
> > >> Signed-off-by: Sakari Ailus 
> > >> Reviewed-by: Niklas Söderlund 
> > >> Acked-by: Hans Verkuil 
> > >> Acked-by: Pavel Machek 
> > > 
> > > I'm still very opposed to this. In addition to increasing the risk of
> > > documentation becoming stale, it also makes review more difficult. I'm
> > > reviewing patch 05/25 of this series and I have to jump around the patch
> > > to verify that the documentation matches the implementation, it's really
> > > annoying.
> > 
> > I'd like to have this discussion separately from the patchset, which is
> > right now in its 13th version. This patch simply makes the current state
> > consistent; V4L2 async was the only part of V4L2 with KernelDoc
> > documentation in .c files.
> 
> But there's no need to move the documentation at this point until we reach an 
> agreement, is there ?

The status quo has is that the KernelDoc is in headers. Generally, if you
change parts that appear to lack framework-wide changes already done, you
do those changes before making other changes since it's a no-brainer.

Which is what this patch represents.

If we end up moving the KernelDoc to .c files moving this back could result
into an extra patch. I'm not too worried about that frankly.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header

[Sakari Ailus]

Hi Hans,

On Tue, Sep 19, 2017 at 01:04:36PM +0200, Hans Verkuil wrote:
> On 09/19/17 12:48, Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > Thank you for the patch.
> > 
> > On Friday, 15 September 2017 17:17:00 EEST Sakari Ailus wrote:
> >> In V4L2 the practice is to have the KernelDoc documentation in the header
> >> and not in .c source code files. This consequently makes the V4L2 fwnode
> >> function documentation part of the Media documentation build.
> >>
> >> Also correct the link related function and argument naming in
> >> documentation.
> >>
> >> Signed-off-by: Sakari Ailus 
> >> Reviewed-by: Niklas Söderlund 
> >> Acked-by: Hans Verkuil 
> >> Acked-by: Pavel Machek 
> > 
> > I'm still very opposed to this. In addition to increasing the risk of 
> > documentation becoming stale, it also makes review more difficult. I'm 
> > reviewing patch 05/25 of this series and I have to jump around the patch to 
> > verify that the documentation matches the implementation, it's really 
> > annoying.
> > 
> > We should instead move all function documentation from header files to 
> > source 
> > files.
> 
> I disagree with this. Yes, it makes reviewing harder, but when you have to
> *use* these functions as e.g. a driver developer, then having it in the
> header is much more convenient.

For developers writing a driver and _not_ using e.g. the HTML
documentation, programs like cscope point the user to the implementation of
the function --- which is in the .c file, not the header. This is what I
personally tend to do at least; for most of the time I ignore where exactly
a given function is implemented (this is actually not self-evident in V4L2
outside async / fwnode).

The rest of the kernel appears to generally have the KernelDoc in .c files,
for a reason or another:

14:05:15 nauris sailus [~/scratch/src/linux]git grep '/\*\*$' -- include/|wc -l
6997
14:14:46 nauris sailus [~/scratch/src/linux]git grep '/\*\*$' -- drivers/ net/ 
mm/ lib/ kernel/ fs/ firmware/ init/ ipc/ block/ crypto/ |wc -l
44756

I think I'm slightly leaning towards moving it: having the documentation
where the implementation is does help keeping it up-to-date. It's currently
all too easy to change a function without realising it was actually
documented somewhere.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header

[Laurent Pinchart]

Hi Sakari,

On Tuesday, 19 September 2017 14:10:37 EEST Sakari Ailus wrote:
> On Tue, Sep 19, 2017 at 01:48:27PM +0300, Laurent Pinchart wrote:
> > On Friday, 15 September 2017 17:17:00 EEST Sakari Ailus wrote:
> >> In V4L2 the practice is to have the KernelDoc documentation in the
> >> header and not in .c source code files. This consequently makes the V4L2
> >> fwnode function documentation part of the Media documentation build.
> >> 
> >> Also correct the link related function and argument naming in
> >> documentation.
> >> 
> >> Signed-off-by: Sakari Ailus 
> >> Reviewed-by: Niklas Söderlund 
> >> Acked-by: Hans Verkuil 
> >> Acked-by: Pavel Machek 
> > 
> > I'm still very opposed to this. In addition to increasing the risk of
> > documentation becoming stale, it also makes review more difficult. I'm
> > reviewing patch 05/25 of this series and I have to jump around the patch
> > to verify that the documentation matches the implementation, it's really
> > annoying.
> 
> I'd like to have this discussion separately from the patchset, which is
> right now in its 13th version. This patch simply makes the current state
> consistent; V4L2 async was the only part of V4L2 with KernelDoc
> documentation in .c files.

But there's no need to move the documentation at this point until we reach an 
agreement, is there ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2017 at 01:48:27PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 17:17:00 EEST Sakari Ailus wrote:
> > In V4L2 the practice is to have the KernelDoc documentation in the header
> > and not in .c source code files. This consequently makes the V4L2 fwnode
> > function documentation part of the Media documentation build.
> > 
> > Also correct the link related function and argument naming in
> > documentation.
> > 
> > Signed-off-by: Sakari Ailus 
> > Reviewed-by: Niklas Söderlund 
> > Acked-by: Hans Verkuil 
> > Acked-by: Pavel Machek 
> 
> I'm still very opposed to this. In addition to increasing the risk of 
> documentation becoming stale, it also makes review more difficult. I'm 
> reviewing patch 05/25 of this series and I have to jump around the patch to 
> verify that the documentation matches the implementation, it's really 
> annoying.

I'd like to have this discussion separately from the patchset, which is
right now in its 13th version. This patch simply makes the current state
consistent; V4L2 async was the only part of V4L2 with KernelDoc
documentation in .c files.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header

[Laurent Pinchart]

Hi Hans,

On Tuesday, 19 September 2017 14:04:36 EEST Hans Verkuil wrote:
> On 09/19/17 12:48, Laurent Pinchart wrote:
> > On Friday, 15 September 2017 17:17:00 EEST Sakari Ailus wrote:
> >> In V4L2 the practice is to have the KernelDoc documentation in the header
> >> and not in .c source code files. This consequently makes the V4L2 fwnode
> >> function documentation part of the Media documentation build.
> >> 
> >> Also correct the link related function and argument naming in
> >> documentation.
> >> 
> >> Signed-off-by: Sakari Ailus 
> >> Reviewed-by: Niklas Söderlund 
> >> Acked-by: Hans Verkuil 
> >> Acked-by: Pavel Machek 
> > 
> > I'm still very opposed to this. In addition to increasing the risk of
> > documentation becoming stale, it also makes review more difficult. I'm
> > reviewing patch 05/25 of this series and I have to jump around the patch
> > to verify that the documentation matches the implementation, it's really
> > annoying.
> > 
> > We should instead move all function documentation from header files to
> > source files.
> 
> I disagree with this. Yes, it makes reviewing harder, but when you have to
> *use* these functions as e.g. a driver developer, then having it in the
> header is much more convenient.

When writing a driver you can use the compiled documentation. We're lacking 
reviewers in V4L2, we should make their life easier if we want to attract 
more.

Furthermore, if documentation becomes stale, it will become useless for driver 
authors, regardless of where it's stored.

> >> ---
> >> 
> >>  drivers/media/v4l2-core/v4l2-fwnode.c | 75 -
> >>  include/media/v4l2-fwnode.h   | 81 +++-
> >>  2 files changed, 80 insertions(+), 76 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header

[Hans Verkuil]

On 09/19/17 12:48, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 15 September 2017 17:17:00 EEST Sakari Ailus wrote:
>> In V4L2 the practice is to have the KernelDoc documentation in the header
>> and not in .c source code files. This consequently makes the V4L2 fwnode
>> function documentation part of the Media documentation build.
>>
>> Also correct the link related function and argument naming in
>> documentation.
>>
>> Signed-off-by: Sakari Ailus 
>> Reviewed-by: Niklas Söderlund 
>> Acked-by: Hans Verkuil 
>> Acked-by: Pavel Machek 
> 
> I'm still very opposed to this. In addition to increasing the risk of 
> documentation becoming stale, it also makes review more difficult. I'm 
> reviewing patch 05/25 of this series and I have to jump around the patch to 
> verify that the documentation matches the implementation, it's really 
> annoying.
> 
> We should instead move all function documentation from header files to source 
> files.

I disagree with this. Yes, it makes reviewing harder, but when you have to
*use* these functions as e.g. a driver developer, then having it in the
header is much more convenient.

Regards,

Hans

> 
>> ---
>>  drivers/media/v4l2-core/v4l2-fwnode.c | 75 
>> include/media/v4l2-fwnode.h| 81 +++-
>> 2 files changed, 80 insertions(+), 76 deletions(-)
> 
> [snip]
> 

Re: [PATCHv2 1/2] dt-bindings: adi,adv7511.txt: document cec clock

[Hans Verkuil]

On 09/19/17 12:42, Sergei Shtylyov wrote:
> On 9/19/2017 1:35 PM, Hans Verkuil wrote:
> 
 From: Hans Verkuil 

 Document the cec clock binding.

 Signed-off-by: Hans Verkuil 
 Acked-by: Rob Herring 
 ---
  Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt 
 | 4 
  1 file changed, 4 insertions(+)

 diff --git 
 a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt 
 b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
 index 06668bca7ffc..4497ae054d49 100644
 --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
 +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
 @@ -68,6 +68,8 @@ Optional properties:
  - adi,disable-timing-generator: Only for ADV7533. Disables the 
 internal timing
generator. The chip will rely on the sync signals in the DSI 
 data lanes,
rather than generate its own timings for HDMI output.
 +- clocks: from common clock binding: handle to CEC clock.
>>>
>>>It's called "phandle" in the DT speak. :-)
>>>Are you sure the clock specifier would always be absent?
>>
>> Sorry? I don't understand the question. Did you mean: "can be absent?"?
>
>   No, you only say that there'll be the clock phandle only. The clock
> specifier may follow the phandle for the clock devices that have
> "#clock-cells" prop != 0.

 I have to say that I just copy-and-pasted this from other bindings.
>>>
>>>  :-)
>>>
 Would this be better?

 - clocks: list of clock specifiers, corresponding to entries in
 the clock-names property;
>>>
>>>  Didn't you say that there'll be only one clock, "cec"? If so, there's
>>> gonna  be a single clock phandle+specifier pair. They always go in pairs. 
>>> :-)
>>>
 - clock-names: from common clock binding: must be "cec".
>>
>> - clocks: cec clock phandle, corresponding to the clock-names entry.
> 
> The clock phandle and specifier.
> 
>> - clock-names: from common clock binding: must be "cec".
>>
>> This OK?
> 
> Well, you seem to be going in circles, the above was almost the same as 
> the original prop description...

- clocks: from common clock binding: reference to the CEC clock.
- clock-names: from common clock binding: must be "cec".

This avoids the whole issue of having just a phandle or a phandle + specifier.

Regards,

Hans

Re: [PATCH v13 01/25] v4l: fwnode: Move KernelDoc documentation to the header

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:00 EEST Sakari Ailus wrote:
> In V4L2 the practice is to have the KernelDoc documentation in the header
> and not in .c source code files. This consequently makes the V4L2 fwnode
> function documentation part of the Media documentation build.
> 
> Also correct the link related function and argument naming in
> documentation.
> 
> Signed-off-by: Sakari Ailus 
> Reviewed-by: Niklas Söderlund 
> Acked-by: Hans Verkuil 
> Acked-by: Pavel Machek 

I'm still very opposed to this. In addition to increasing the risk of 
documentation becoming stale, it also makes review more difficult. I'm 
reviewing patch 05/25 of this series and I have to jump around the patch to 
verify that the documentation matches the implementation, it's really 
annoying.

We should instead move all function documentation from header files to source 
files.

> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 75 
> include/media/v4l2-fwnode.h| 81 +++-
> 2 files changed, 80 insertions(+), 76 deletions(-)

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 1/2] dt-bindings: adi,adv7511.txt: document cec clock

[Sergei Shtylyov]

On 9/19/2017 1:35 PM, Hans Verkuil wrote:


From: Hans Verkuil 

Document the cec clock binding.

Signed-off-by: Hans Verkuil 
Acked-by: Rob Herring 
---
 Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt 
b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 06668bca7ffc..4497ae054d49 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -68,6 +68,8 @@ Optional properties:
 - adi,disable-timing-generator: Only for ADV7533. Disables the internal 
timing
   generator. The chip will rely on the sync signals in the DSI data lanes,
   rather than generate its own timings for HDMI output.
+- clocks: from common clock binding: handle to CEC clock.


   It's called "phandle" in the DT speak. :-)
   Are you sure the clock specifier would always be absent?


Sorry? I don't understand the question. Did you mean: "can be absent?"?


  No, you only say that there'll be the clock phandle only. The clock
specifier may follow the phandle for the clock devices that have
"#clock-cells" prop != 0.


I have to say that I just copy-and-pasted this from other bindings.


 :-)


Would this be better?

- clocks: list of clock specifiers, corresponding to entries in
the clock-names property;


 Didn't you say that there'll be only one clock, "cec"? If so, there's
gonna  be a single clock phandle+specifier pair. They always go in pairs. :-)


- clock-names: from common clock binding: must be "cec".


- clocks: cec clock phandle, corresponding to the clock-names entry.


   The clock phandle and specifier.


- clock-names: from common clock binding: must be "cec".

This OK?


   Well, you seem to be going in circles, the above was almost the same as 
the original prop description...



Regards,

Hans


MBR, Sergei

Re: [PATCHv2 1/2] dt-bindings: adi,adv7511.txt: document cec clock

[Hans Verkuil]

On 09/19/17 12:15, Sergei Shtylyov wrote:
> On 9/19/2017 1:07 PM, Hans Verkuil wrote:
> 
>> From: Hans Verkuil 
>>
>> Document the cec clock binding.
>>
>> Signed-off-by: Hans Verkuil 
>> Acked-by: Rob Herring 
>> ---
>> Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 4 
>> 
>> 1 file changed, 4 insertions(+)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt 
>> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> index 06668bca7ffc..4497ae054d49 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> @@ -68,6 +68,8 @@ Optional properties:
>> - adi,disable-timing-generator: Only for ADV7533. Disables the 
>> internal timing
>>   generator. The chip will rely on the sync signals in the DSI data 
>> lanes,
>>   rather than generate its own timings for HDMI output.
>> +- clocks: from common clock binding: handle to CEC clock.
>
>   It's called "phandle" in the DT speak. :-)
>   Are you sure the clock specifier would always be absent?

 Sorry? I don't understand the question. Did you mean: "can be absent?"?
>>>
>>>  No, you only say that there'll be the clock phandle only. The clock
>>> specifier may follow the phandle for the clock devices that have
>>> "#clock-cells" prop != 0.
>>
>> I have to say that I just copy-and-pasted this from other bindings.
> 
> :-)
> 
>> Would this be better?
>>
>> - clocks: list of clock specifiers, corresponding to entries in
>>the clock-names property;
> 
> Didn't you say that there'll be only one clock, "cec"? If so, there's 
> gonna  be a single clock phandle+specifier pair. They always go in pairs. :-)
> 
>> - clock-names: from common clock binding: must be "cec".

- clocks: cec clock phandle, corresponding to the clock-names entry.
- clock-names: from common clock binding: must be "cec".

This OK?

Regards,

Hans

Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Hans Verkuil]

On 09/19/17 11:50, Dave Stevenson wrote:
> Hi Eric.
> 
> Thanks for the review.
> 
> On 18 September 2017 at 19:18, Eric Anholt  wrote:
>> Dave Stevenson  writes:
>>> diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c 
>>> b/drivers/media/platform/bcm2835/bcm2835-unicam.c
>>> new file mode 100644
>>> index 000..5b1adc3
>>> --- /dev/null
>>> +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
>>> @@ -0,0 +1,2192 @@
>>> +/*
>>> + * BCM2835 Unicam capture Driver
>>> + *
>>> + * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.
>>> + *
>>> + * Dave Stevenson 
>>> + *
>>> + * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and
>>> + * TI CAL camera interface driver by Benoit Parrot.
>>> + *
>>
>> Possible future improvement: this description of the driver is really
>> nice and could be turned into kernel-doc.
> 
> Documentation?! Surely not :-)
> For now I'll leave it as a task for another day.
> 
>>> + * There are two camera drivers in the kernel for BCM283x - this one
>>> + * and bcm2835-camera (currently in staging).
>>> + *
>>> + * This driver is purely the kernel control the Unicam peripheral - there
>>
>> Maybe "This driver directly controls..."?
> 
> Will do in v3.
> 
>>> + * is no involvement with the VideoCore firmware. Unicam receives CSI-2
>>> + * or CCP2 data and writes it into SDRAM. The only processing options are
>>> + * to repack Bayer data into an alternate format, and applying windowing
>>> + * (currently not implemented).
>>> + * It should be possible to connect it to any sensor with a
>>> + * suitable output interface and V4L2 subdevice driver.
>>> + *
>>> + * bcm2835-camera uses with the VideoCore firmware to control the sensor,
>>
>> "uses the"
> 
> Will do in v3.
> 
>>> + * Unicam, ISP, and all tuner control loops. Fully processed frames are
>>> + * delivered to the driver by the firmware. It only has sensor drivers
>>> + * for Omnivision OV5647, and Sony IMX219 sensors.
>>> + *
>>> + * The two drivers are mutually exclusive for the same Unicam instance.
>>> + * The VideoCore firmware checks the device tree configuration during boot.
>>> + * If it finds device tree nodes called csi0 or csi1 it will block the
>>> + * firmware from accessing the peripheral, and bcm2835-camera will
>>> + * not be able to stream data.
>>
>> Thanks for describing this here!
>>
>>> +/*
>>> + * The peripheral can unpack and repack between several of
>>> + * the Bayer raw formats, so any Bayer format can be advertised
>>> + * as the same Bayer order in each of the supported bit depths.
>>> + * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8
>>> + * formats.
>>> + */
>>> +#define PIX_FMT_ALL_BGGR  v4l2_fourcc('b', 'g', 'g', 'r')
>>> +#define PIX_FMT_ALL_RGGB  v4l2_fourcc('r', 'g', 'g', 'b')
>>> +#define PIX_FMT_ALL_GBRG  v4l2_fourcc('g', 'b', 'r', 'g')
>>> +#define PIX_FMT_ALL_GRBG  v4l2_fourcc('g', 'r', 'b', 'g')
>>
>> Should thes fourccs be defined in a common v4l2 header, to reserve it
>> from clashing with others later?
> 
> I'm only using them as flags and probably in a manner that nothing
> else is likely to copy, so it seems a little excessive to put them in
> a common header.
> Perhaps it's better to switch to 0xFFF0 to 0xFFF3 or other
> value that won't come up as a fourcc under any normal circumstance.
> Any thoughts from other people?

I think that's better, yes.

> 
>> This is really the only question I have about this driver before seeing
>> it merged.  As far as me wearing my platform maintainer hat, I'm happy
>> with the driver, and my other little notes are optional.
>>
>>> +static int unicam_probe(struct platform_device *pdev)
>>> +{
>>> + struct unicam_cfg *unicam_cfg;
>>> + struct unicam_device *unicam;
>>> + struct v4l2_ctrl_handler *hdl;
>>> + struct resource *res;
>>> + int ret;
>>> +
>>> + unicam = devm_kzalloc(>dev, sizeof(*unicam), GFP_KERNEL);
>>> + if (!unicam)
>>> + return -ENOMEM;
>>> +
>>> + unicam->pdev = pdev;
>>> + unicam_cfg = >cfg;
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + unicam_cfg->base = devm_ioremap_resource(>dev, res);
>>> + if (IS_ERR(unicam_cfg->base)) {
>>> + unicam_err(unicam, "Failed to get main io block\n");
>>> + return PTR_ERR(unicam_cfg->base);
>>> + }
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>> + unicam_cfg->clk_gate_base = devm_ioremap_resource(>dev, res);
>>> + if (IS_ERR(unicam_cfg->clk_gate_base)) {
>>> + unicam_err(unicam, "Failed to get 2nd io block\n");
>>> + return PTR_ERR(unicam_cfg->clk_gate_base);
>>> + }
>>> +
>>> + unicam->clock = devm_clk_get(>dev, "lp_clock");
>>> + if (IS_ERR(unicam->clock)) {
>>> + unicam_err(unicam, "Failed to get clock\n");
>>> + return PTR_ERR(unicam->clock);
>>> + }
>>> +
>>> + 

Re: [PATCH v13 18/25] v4l: fwnode: Add a helper function to obtain device / integer references

[Sakari Ailus]

Hi Hans,

On Tue, Sep 19, 2017 at 11:21:50AM +0200, Hans Verkuil wrote:
> On 09/19/17 10:45, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > Thank you for the review.
> > 
> > On Tue, Sep 19, 2017 at 10:31:41AM +0200, Hans Verkuil wrote:
> >> Hi Sakari,
> >>
> >> I'm slowly starting to understand this. The example helped a lot. But I 
> >> still have
> >> some questions, see below.
> >>
> >> On 09/15/2017 04:17 PM, Sakari Ailus wrote:
> >>> v4l2_fwnode_reference_parse_int_prop() will find an fwnode such that under
> >>> the device's own fwnode, it will follow child fwnodes with the given
> >>> property-value pair and return the resulting fwnode.
> >>
> >> I think both the subject, commit log, function comment and function name 
> >> should
> >> reflect the fact that this function is for an ACPI reference.
> >>
> >> It's only called for ACPI (from patch 19):
> >>
> >> +  if (props[i].props && is_acpi_node(dev_fwnode(dev)))
> >> +  ret = v4l2_fwnode_reference_parse_int_props(
> >>
> >> So renaming it to v4l2_fwnode_acpi_reference_parse_int_props or something 
> >> similar
> >> would clarify this fact.
> > 
> > I don't think we'll see many like this one. I presume we won't use it on DT
> > albeit there are no direct references to ACPI in the code itself.
> > 
> > How about v4l2_fwnode_parse_acpi_reference (+ "s" for the one below)?
> 
> Sounds good.
> 
> > 
> >>
> >>>
> >>> Signed-off-by: Sakari Ailus 
> >>> ---
> >>>  drivers/media/v4l2-core/v4l2-fwnode.c | 201 
> >>> ++
> >>>  1 file changed, 201 insertions(+)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> >>> b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> index 65e84ea1cc35..968a345a288f 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> @@ -567,6 +567,207 @@ static int v4l2_fwnode_reference_parse(
> >>>   return ret;
> >>>  }
> >>>  
> >>> +/*
> >>> + * v4l2_fwnode_reference_get_int_prop - parse a reference with integer
> >>> + *   arguments
> >>> + * @dev: struct device pointer
> >>> + * @notifier: notifier for @dev
> >>> + * @prop: the name of the property
> >>> + * @index: the index of the reference to get
> >>> + * @props: the array of integer property names
> >>> + * @nprops: the number of integer property names in @nprops
> >>
> >> You mean 'in @props'?
> > 
> > Yes, I'll fix that.
> > 
> >>
> >> One thing that is not clear to me is when you would use an nprops value > 
> >> 1.
> >> What's the use-case for that? It only makes sense (I think) if you would 
> >> have
> >> property names that are all aliases of one another.
> > 
> > There may be several flash LEDs related to a sensor. That's the use case,
> > for instance.
> 
> I think it would be helpful if the example shows two LEDs related to a
> sensor. Part of the problem I have in understanding this code is that I
> have zero experience with ACPI (and that is probably true for most other
> developers), so I don't know how this is encoded. With a good example it
> is much easier to understand.

I'm a bit at loss here. Are you happy with the example below, or do you
think something is missing in the documentation here.

> 
> > 
> >>
> >>> + *
> >>> + * Find fwnodes referred to by a property @prop, then under that
> >>> + * iteratively, @nprops times, follow each child node which has a
> >>> + * property in @props array at a given child index the value of which
> >>> + * matches the integer argument at an index.
> >>> + *
> >>> + * For example, if this function was called with arguments and values
> >>> + * @dev corresponding to device "SEN", @prop == "flash-leds", @index
> >>> + * == 1, @props == { "led" }, @nprops == 1, with the ASL snippet below
> >>> + * it would return the node marked with THISONE. The @dev argument in
> >>> + * the ASL below.
> >>
> >> That last sentence about the @dev seems incomplete. I'm not sure what is
> >> meant by it.
> > 
> > I think it was meant to convey some information but it got added to the
> > previous sentence. I'll remove it.
> > 
> >>
> >>> + *
> >>> + *   Device (LED)
> >>> + *   {
> >>> + *   Name (_DSD, Package () {
> >>> + *   ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> >>> + *   Package () {
> >>> + *   Package () { "led0", "LED0" },
> >>> + *   Package () { "led1", "LED1" },
> >>> + *   }
> >>> + *   })
> >>> + *   Name (LED0, Package () {
> >>> + *   ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >>> + *   Package () {
> >>> + *   Package () { "led", 0 },
> >>> + *   }
> >>> + *   })
> >>> + *   Name (LED1, Package () {
> >>> + *   // THISONE
> >>> + * 

Re: [PATCHv2 1/2] dt-bindings: adi,adv7511.txt: document cec clock

[Sergei Shtylyov]

On 9/19/2017 1:07 PM, Hans Verkuil wrote:


From: Hans Verkuil 

Document the cec clock binding.

Signed-off-by: Hans Verkuil 
Acked-by: Rob Herring 
---
Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 4 
1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt 
b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 06668bca7ffc..4497ae054d49 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -68,6 +68,8 @@ Optional properties:
- adi,disable-timing-generator: Only for ADV7533. Disables the internal 
timing
  generator. The chip will rely on the sync signals in the DSI data lanes,
  rather than generate its own timings for HDMI output.
+- clocks: from common clock binding: handle to CEC clock.


  It's called "phandle" in the DT speak. :-)
  Are you sure the clock specifier would always be absent?


Sorry? I don't understand the question. Did you mean: "can be absent?"?


 No, you only say that there'll be the clock phandle only. The clock
specifier may follow the phandle for the clock devices that have
"#clock-cells" prop != 0.


I have to say that I just copy-and-pasted this from other bindings.


   :-)


Would this be better?

- clocks: list of clock specifiers, corresponding to entries in
   the clock-names property;


   Didn't you say that there'll be only one clock, "cec"? If so, there's 
gonna  be a single clock phandle+specifier pair. They always go in pairs. :-)



- clock-names: from common clock binding: must be "cec".

Regards,

Hans


MBR, Sergei

Re: [PATCH v13 05/25] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Hans Verkuil]

On 09/19/17 12:00, Sakari Ailus wrote:
> Hi Hans,
> 
> On Tue, Sep 19, 2017 at 10:40:14AM +0200, Hans Verkuil wrote:
>> On 09/19/2017 10:20 AM, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> Thank you for the review.
>>>
>>> On Tue, Sep 19, 2017 at 10:03:27AM +0200, Hans Verkuil wrote:
 On 09/15/2017 04:17 PM, Sakari Ailus wrote:
> ...
> +static int __v4l2_async_notifier_parse_fwnode_endpoints(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + size_t asd_struct_size, unsigned int port, bool has_port,
> + int (*parse_endpoint)(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd))
> +{
> + struct fwnode_handle *fwnode = NULL;
> + unsigned int max_subdevs = notifier->max_subdevs;
> + int ret;
> +
> + if (WARN_ON(asd_struct_size < sizeof(struct v4l2_async_subdev)))
> + return -EINVAL;
> +
> + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> +  dev_fwnode(dev), fwnode)); ) {

 You can replace this by:

while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), 
 fwnode))) {

> + if (!fwnode_device_is_available(
> + fwnode_graph_get_port_parent(fwnode)))
> + continue;
> +
> + if (has_port) {
> + struct fwnode_endpoint ep;
> +
> + ret = fwnode_graph_parse_endpoint(fwnode, );
> + if (ret) {
> + fwnode_handle_put(fwnode);
> + return ret;
> + }
> +
> + if (ep.port != port)
> + continue;
> + }
> + max_subdevs++;
> + }
> +
> + /* No subdevs to add? Return here. */
> + if (max_subdevs == notifier->max_subdevs)
> + return 0;
> +
> + ret = v4l2_async_notifier_realloc(notifier, max_subdevs);
> + if (ret)
> + return ret;
> +
> + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> +  dev_fwnode(dev), fwnode)); ) {

 Same here: this can be a 'while'.
>>>
>>> The fwnode = NULL assignment still needs to be done. A for loop has a
>>> natural initialiser for the loop, I think it's cleaner than using while
>>> here.
>>
>> After the previous while fwnode is NULL again (since that's when the while
>> stops).
>>
>>>
>>> The macro would be implemented this way as well.
>>>
>>> For the loop above this one, I'd use for for consistency: it's the same
>>> loop after all.
>>>
>>> This reminds me --- I'll send the patch for the macro.
>>
>> If this is going to be replaced by a macro, then disregard my comment.
> 
> Yes. I just sent that to linux-acpi (as well as devicetree and to you).
> 
> ...
> 
> +/**
> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode 
> endpoints in a
> + *   device node
> + * @dev: the device the endpoints of which are to be parsed
> + * @notifier: notifier for @dev
> + * @asd_struct_size: size of the driver's async sub-device struct, 
> including
> + *sizeof(struct v4l2_async_subdev). The 
> + *v4l2_async_subdev shall be the first member of
> + *the driver's async sub-device struct, i.e. both
> + *begin at the same memory address.
> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> + *   endpoint. Optional.
> + *   Return: %0 on success
> + *   %-ENOTCONN if the endpoint is to be skipped 
> but this
> + *  should not be considered as an 
> error
> + *   %-EINVAL if the endpoint configuration is 
> invalid
> + *
> + * Parse the fwnode endpoints of the @dev device and populate the async 
> sub-
> + * devices array of the notifier. The @parse_endpoint callback function 
> is
> + * called for each endpoint with the corresponding async sub-device 
> pointer to
> + * let the caller initialize the driver-specific part of the async 
> sub-device
> + * structure.
> + *
> + * The notifier memory shall be zeroed before this function is called on 
> the
> + * notifier.
> + *
> + * This function may not be called on a registered notifier and may be 
> called on
> + * a notifier only once.
> + *
> + * Do not change the notifier's subdevs array, take references to the 
> subdevs
> + * array itself or change the notifier's num_subdevs field. This is 
> because this
> + * function allocates and reallocates the subdevs array based on parsing
> 

Re: [PATCHv2 1/2] dt-bindings: adi,adv7511.txt: document cec clock

[Hans Verkuil]

On 09/19/17 11:35, Sergei Shtylyov wrote:
> On 9/19/2017 12:29 PM, Hans Verkuil wrote:
> 
 From: Hans Verkuil 

 Document the cec clock binding.

 Signed-off-by: Hans Verkuil 
 Acked-by: Rob Herring 
 ---
Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 4 
 
1 file changed, 4 insertions(+)

 diff --git 
 a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt 
 b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
 index 06668bca7ffc..4497ae054d49 100644
 --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
 +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
 @@ -68,6 +68,8 @@ Optional properties:
- adi,disable-timing-generator: Only for ADV7533. Disables the internal 
 timing
  generator. The chip will rely on the sync signals in the DSI data 
 lanes,
  rather than generate its own timings for HDMI output.
 +- clocks: from common clock binding: handle to CEC clock.
>>>
>>>  It's called "phandle" in the DT speak. :-)
>>>  Are you sure the clock specifier would always be absent?
>>
>> Sorry? I don't understand the question. Did you mean: "can be absent?"?
> 
> No, you only say that there'll be the clock phandle only. The clock 
> specifier may follow the phandle for the clock devices that have 
> "#clock-cells" prop != 0.

I have to say that I just copy-and-pasted this from other bindings. Would
this be better?

- clocks: list of clock specifiers, corresponding to entries in
  the clock-names property;
- clock-names: from common clock binding: must be "cec".

Regards,

Hans

Re: [PATCH v13 05/25] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Sakari Ailus]

Hi Hans,

On Tue, Sep 19, 2017 at 10:40:14AM +0200, Hans Verkuil wrote:
> On 09/19/2017 10:20 AM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > Thank you for the review.
> > 
> > On Tue, Sep 19, 2017 at 10:03:27AM +0200, Hans Verkuil wrote:
> >> On 09/15/2017 04:17 PM, Sakari Ailus wrote:
...
> >>> +static int __v4l2_async_notifier_parse_fwnode_endpoints(
> >>> + struct device *dev, struct v4l2_async_notifier *notifier,
> >>> + size_t asd_struct_size, unsigned int port, bool has_port,
> >>> + int (*parse_endpoint)(struct device *dev,
> >>> + struct v4l2_fwnode_endpoint *vep,
> >>> + struct v4l2_async_subdev *asd))
> >>> +{
> >>> + struct fwnode_handle *fwnode = NULL;
> >>> + unsigned int max_subdevs = notifier->max_subdevs;
> >>> + int ret;
> >>> +
> >>> + if (WARN_ON(asd_struct_size < sizeof(struct v4l2_async_subdev)))
> >>> + return -EINVAL;
> >>> +
> >>> + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> >>> +  dev_fwnode(dev), fwnode)); ) {
> >>
> >> You can replace this by:
> >>
> >>while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), 
> >> fwnode))) {
> >>
> >>> + if (!fwnode_device_is_available(
> >>> + fwnode_graph_get_port_parent(fwnode)))
> >>> + continue;
> >>> +
> >>> + if (has_port) {
> >>> + struct fwnode_endpoint ep;
> >>> +
> >>> + ret = fwnode_graph_parse_endpoint(fwnode, );
> >>> + if (ret) {
> >>> + fwnode_handle_put(fwnode);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + if (ep.port != port)
> >>> + continue;
> >>> + }
> >>> + max_subdevs++;
> >>> + }
> >>> +
> >>> + /* No subdevs to add? Return here. */
> >>> + if (max_subdevs == notifier->max_subdevs)
> >>> + return 0;
> >>> +
> >>> + ret = v4l2_async_notifier_realloc(notifier, max_subdevs);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> >>> +  dev_fwnode(dev), fwnode)); ) {
> >>
> >> Same here: this can be a 'while'.
> > 
> > The fwnode = NULL assignment still needs to be done. A for loop has a
> > natural initialiser for the loop, I think it's cleaner than using while
> > here.
> 
> After the previous while fwnode is NULL again (since that's when the while
> stops).
> 
> > 
> > The macro would be implemented this way as well.
> > 
> > For the loop above this one, I'd use for for consistency: it's the same
> > loop after all.
> > 
> > This reminds me --- I'll send the patch for the macro.
> 
> If this is going to be replaced by a macro, then disregard my comment.

Yes. I just sent that to linux-acpi (as well as devicetree and to you).

...

> >>> +/**
> >>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode 
> >>> endpoints in a
> >>> + *   device node
> >>> + * @dev: the device the endpoints of which are to be parsed
> >>> + * @notifier: notifier for @dev
> >>> + * @asd_struct_size: size of the driver's async sub-device struct, 
> >>> including
> >>> + *sizeof(struct v4l2_async_subdev). The 
> >>> + *v4l2_async_subdev shall be the first member of
> >>> + *the driver's async sub-device struct, i.e. both
> >>> + *begin at the same memory address.
> >>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> >>> + *   endpoint. Optional.
> >>> + *   Return: %0 on success
> >>> + *   %-ENOTCONN if the endpoint is to be skipped 
> >>> but this
> >>> + *  should not be considered as an 
> >>> error
> >>> + *   %-EINVAL if the endpoint configuration is 
> >>> invalid
> >>> + *
> >>> + * Parse the fwnode endpoints of the @dev device and populate the async 
> >>> sub-
> >>> + * devices array of the notifier. The @parse_endpoint callback function 
> >>> is
> >>> + * called for each endpoint with the corresponding async sub-device 
> >>> pointer to
> >>> + * let the caller initialize the driver-specific part of the async 
> >>> sub-device
> >>> + * structure.
> >>> + *
> >>> + * The notifier memory shall be zeroed before this function is called on 
> >>> the
> >>> + * notifier.
> >>> + *
> >>> + * This function may not be called on a registered notifier and may be 
> >>> called on
> >>> + * a notifier only once.
> >>> + *
> >>> + * Do not change the notifier's subdevs array, take references to the 
> >>> subdevs
> >>> + * array itself or change the notifier's num_subdevs field. This is 
> >>> because this
> >>> + * function allocates and reallocates the subdevs array based on parsing
> >>> + * endpoints.
> >>> + *
> >>> + * The @struct 

Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

Hi Eric.

Thanks for the review.

On 18 September 2017 at 19:18, Eric Anholt  wrote:
> Dave Stevenson  writes:
>> diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c 
>> b/drivers/media/platform/bcm2835/bcm2835-unicam.c
>> new file mode 100644
>> index 000..5b1adc3
>> --- /dev/null
>> +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
>> @@ -0,0 +1,2192 @@
>> +/*
>> + * BCM2835 Unicam capture Driver
>> + *
>> + * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.
>> + *
>> + * Dave Stevenson 
>> + *
>> + * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and
>> + * TI CAL camera interface driver by Benoit Parrot.
>> + *
>
> Possible future improvement: this description of the driver is really
> nice and could be turned into kernel-doc.

Documentation?! Surely not :-)
For now I'll leave it as a task for another day.

>> + * There are two camera drivers in the kernel for BCM283x - this one
>> + * and bcm2835-camera (currently in staging).
>> + *
>> + * This driver is purely the kernel control the Unicam peripheral - there
>
> Maybe "This driver directly controls..."?

Will do in v3.

>> + * is no involvement with the VideoCore firmware. Unicam receives CSI-2
>> + * or CCP2 data and writes it into SDRAM. The only processing options are
>> + * to repack Bayer data into an alternate format, and applying windowing
>> + * (currently not implemented).
>> + * It should be possible to connect it to any sensor with a
>> + * suitable output interface and V4L2 subdevice driver.
>> + *
>> + * bcm2835-camera uses with the VideoCore firmware to control the sensor,
>
> "uses the"

Will do in v3.

>> + * Unicam, ISP, and all tuner control loops. Fully processed frames are
>> + * delivered to the driver by the firmware. It only has sensor drivers
>> + * for Omnivision OV5647, and Sony IMX219 sensors.
>> + *
>> + * The two drivers are mutually exclusive for the same Unicam instance.
>> + * The VideoCore firmware checks the device tree configuration during boot.
>> + * If it finds device tree nodes called csi0 or csi1 it will block the
>> + * firmware from accessing the peripheral, and bcm2835-camera will
>> + * not be able to stream data.
>
> Thanks for describing this here!
>
>> +/*
>> + * The peripheral can unpack and repack between several of
>> + * the Bayer raw formats, so any Bayer format can be advertised
>> + * as the same Bayer order in each of the supported bit depths.
>> + * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8
>> + * formats.
>> + */
>> +#define PIX_FMT_ALL_BGGR  v4l2_fourcc('b', 'g', 'g', 'r')
>> +#define PIX_FMT_ALL_RGGB  v4l2_fourcc('r', 'g', 'g', 'b')
>> +#define PIX_FMT_ALL_GBRG  v4l2_fourcc('g', 'b', 'r', 'g')
>> +#define PIX_FMT_ALL_GRBG  v4l2_fourcc('g', 'r', 'b', 'g')
>
> Should thes fourccs be defined in a common v4l2 header, to reserve it
> from clashing with others later?

I'm only using them as flags and probably in a manner that nothing
else is likely to copy, so it seems a little excessive to put them in
a common header.
Perhaps it's better to switch to 0xFFF0 to 0xFFF3 or other
value that won't come up as a fourcc under any normal circumstance.
Any thoughts from other people?

> This is really the only question I have about this driver before seeing
> it merged.  As far as me wearing my platform maintainer hat, I'm happy
> with the driver, and my other little notes are optional.
>
>> +static int unicam_probe(struct platform_device *pdev)
>> +{
>> + struct unicam_cfg *unicam_cfg;
>> + struct unicam_device *unicam;
>> + struct v4l2_ctrl_handler *hdl;
>> + struct resource *res;
>> + int ret;
>> +
>> + unicam = devm_kzalloc(>dev, sizeof(*unicam), GFP_KERNEL);
>> + if (!unicam)
>> + return -ENOMEM;
>> +
>> + unicam->pdev = pdev;
>> + unicam_cfg = >cfg;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + unicam_cfg->base = devm_ioremap_resource(>dev, res);
>> + if (IS_ERR(unicam_cfg->base)) {
>> + unicam_err(unicam, "Failed to get main io block\n");
>> + return PTR_ERR(unicam_cfg->base);
>> + }
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + unicam_cfg->clk_gate_base = devm_ioremap_resource(>dev, res);
>> + if (IS_ERR(unicam_cfg->clk_gate_base)) {
>> + unicam_err(unicam, "Failed to get 2nd io block\n");
>> + return PTR_ERR(unicam_cfg->clk_gate_base);
>> + }
>> +
>> + unicam->clock = devm_clk_get(>dev, "lp_clock");
>> + if (IS_ERR(unicam->clock)) {
>> + unicam_err(unicam, "Failed to get clock\n");
>> + return PTR_ERR(unicam->clock);
>> + }
>> +
>> + ret = platform_get_irq(pdev, 0);
>> + if (ret <= 0) {
>> + dev_err(>dev, "No IRQ resource\n");
>> + return -ENODEV;
>> + }
>> + unicam_cfg->irq = ret;
>> +
>> + ret = 

Re: [PATCH v1] media: rc: Add driver for tango IR decoder

[Måns Rullgård]

Marc Gonzalez  writes:

> On 18/09/2017 17:33, Måns Rullgård wrote:
>
>> Marc Gonzalez writes:
>> 
>>> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
>>> index d9ce8ff55d0c..f84923289964 100644
>>> --- a/drivers/media/rc/Kconfig
>>> +++ b/drivers/media/rc/Kconfig
>>> @@ -469,6 +469,11 @@ config IR_SIR
>>>To compile this driver as a module, choose M here: the module will
>>>be called sir-ir.
>>>  +config IR_TANGO
>>> +   tristate "Sigma Designs SMP86xx IR decoder"
>>> +   depends on RC_CORE
>>> +   depends on ARCH_TANGO || COMPILE_TEST
>>> +
>>>  config IR_ZX
>>> tristate "ZTE ZX IR remote control"
>>> depends on RC_CORE
>> 
>> This hunk looks damaged.
>
> It appears that the SMTP server has been mangling outgoing messages
> for a few months. I will find a work-around.
>
>> What have you changed compared to my original code?
>
> o Rename tangox to tango
> o Handle protocol selection (enable/disable) in the change_protocol callback,
> instead of unconditionally in open/close
> o Delete open/close callbacks
> o Rebase driver on top of linuxtv/master
> o Use ir_nec_bytes_to_scancode() in tango_ir_handle_nec()
> o Use devm_rc_allocate_device() in tango_ir_probe()
> o Use Use devm_rc_register_device() in tango_ir_probe()
> o Rename rc->input_name to rc->device_name
> o List all NEC variants for rc->allowed_protocols

Did you test the NEC32 variant?  I don't have anything that produces
such codes.

> o Change type of clkrate to u64
> o Fix tango_ir_probe and tango_ir_remove for devm
> o Move around some init calls in tango_ir_probe() for devm
> o Use relaxed variants of MMIO accessors

Thanks for fixing it up.

>> I tested all three protocols with a few random remotes I had lying
>> around back when I wrote the driver, but that's quite a while ago.
>
> OK, I don't think I changed anything wrt RC-5 and RC-6 handling.
> It would be great if you could give the driver a quick spin to check
> these two protocols. But if you don't have time, no problem.

I'll try to find the time.

>> You should also write a devicetree binding.
>
> Will do.
>
>> Finally, when sending patches essentially written by someone else,
>> please make sure to set a From: line for correct attribution.  It's not
>> nice to take other people's code and apparently pass it off as your own
>> even you've made a few small changes.
>
> It was not my intent to "take other people's code and apparently pass it off
> as my own". I clearly stated where I got the driver from, and your copyright
> notice is right there, at the top of the driver. But I understand that you
> also want to be credited as the author in the git log, and I will fix that
> in v2. Please accept my apologies.

No problem.  I know you're not intentionally trying to mislead anyone.

-- 
Måns Rullgård

Re: [PATCHv2 1/2] dt-bindings: adi,adv7511.txt: document cec clock

[Sergei Shtylyov]

On 9/19/2017 12:29 PM, Hans Verkuil wrote:


From: Hans Verkuil 

Document the cec clock binding.

Signed-off-by: Hans Verkuil 
Acked-by: Rob Herring 
---
   Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 4 
   1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt 
b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 06668bca7ffc..4497ae054d49 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -68,6 +68,8 @@ Optional properties:
   - adi,disable-timing-generator: Only for ADV7533. Disables the internal 
timing
 generator. The chip will rely on the sync signals in the DSI data lanes,
 rather than generate its own timings for HDMI output.
+- clocks: from common clock binding: handle to CEC clock.


 It's called "phandle" in the DT speak. :-)
 Are you sure the clock specifier would always be absent?


Sorry? I don't understand the question. Did you mean: "can be absent?"?


   No, you only say that there'll be the clock phandle only. The clock 
specifier may follow the phandle for the clock devices that have 
"#clock-cells" prop != 0.



Regards,

 Hans

[...]

MBR, Sergei

Re: [PATCH v4] [DVB][FRONTEND] Added a new ioctl for optimizing frontend property set operation

[Mauro Carvalho Chehab]

Em Mon, 18 Sep 2017 05:35:45 -0400
Satendra Singh Thakur  escreveu:

> Hello  Mr Chehab,
> It seems that there is a mismatch among tab spacing
> in local patch on my PC, the patch in email
> and the patch in lkml site.
> This is causing alignment problem. Even if I fix alignment problem
> in my PC, alignment is different in lkml and email.
> Anyway, I have run checkpatch and got 0 err and warnings
> Please review it.
> Thanks for the patience.

Please, always put patch description here.

> 
> Signed-off-by: Satendra Singh Thakur 
> ---

If you have any comments to maintainers/reviewers, add here,
after the "---" line.

>  Documentation/media/uapi/dvb/fe-get-property.rst |  24 ++-
>  drivers/media/dvb-core/dvb_frontend.c| 191 
> ++-
>  include/uapi/linux/dvb/frontend.h|  24 +++
>  3 files changed, 158 insertions(+), 81 deletions(-)
> 
> diff --git a/Documentation/media/uapi/dvb/fe-get-property.rst 
> b/Documentation/media/uapi/dvb/fe-get-property.rst
> index 015d4db..b63a588 100644
> --- a/Documentation/media/uapi/dvb/fe-get-property.rst
> +++ b/Documentation/media/uapi/dvb/fe-get-property.rst
> @@ -2,14 +2,14 @@
>  
>  .. _FE_GET_PROPERTY:
>  
> -**
> -ioctl FE_SET_PROPERTY, FE_GET_PROPERTY
> -**
> +**
> +ioctl FE_SET_PROPERTY, FE_GET_PROPERTY, FE_SET_PROPERTY_SHORT
> +**
>  
>  Name
>  
>  
> -FE_SET_PROPERTY - FE_GET_PROPERTY - FE_SET_PROPERTY sets one or more 
> frontend properties. - FE_GET_PROPERTY returns one or more frontend 
> properties.
> +FE_SET_PROPERTY and FE_SET_PROPERTY_SHORT set one or more frontend 
> properties. FE_GET_PROPERTY returns one or more frontend properties.
>  
>  
>  Synopsis
> @@ -21,6 +21,8 @@ Synopsis
>  .. c:function:: int ioctl( int fd, FE_SET_PROPERTY, struct dtv_properties 
> *argp )
>  :name: FE_SET_PROPERTY
>  
> +.. c:function:: int ioctl( int fd, FE_SET_PROPERTY_SHORT, struct 
> dtv_properties_short *argp )
> +:name: FE_SET_PROPERTY_SHORT
>  
>  Arguments
>  =
> @@ -29,15 +31,16 @@ Arguments
>  File descriptor returned by :ref:`open() `.
>  
>  ``argp``
> -pointer to struct :c:type:`dtv_properties`
> +Pointer to struct :c:type:`dtv_properties` or
> + struct :c:type:`dtv_properties_short`.
>  
>  
>  Description
>  ===
>  
> -All DVB frontend devices support the ``FE_SET_PROPERTY`` and
> -``FE_GET_PROPERTY`` ioctls. The supported properties and statistics
> -depends on the delivery system and on the device:
> +All DVB frontend devices support the ``FE_SET_PROPERTY``, ``FE_GET_PROPERTY``
> +and ``FE_SET_PROPERTY_SHORT`` ioctls. The supported  properties and
> +statistics depends on the delivery system and on the device:
>  
>  -  ``FE_SET_PROPERTY:``
>  
> @@ -60,6 +63,11 @@ depends on the delivery system and on the device:
>  
> -  This call only requires read-only access to the device.
>  
> +-  ``FE_SET_PROPERTY_SHORT:``
> +
> +   -  This ioctl is similar to FE_SET_PROPERTY ioctl mentioned above
> +  except that the arguments of the former utilize a struct 
> :c:type:`dtv_property_short`
> +  which is smaller in size.
>  
>  Return Value
>  

I'm still in doubt about something: what's the real requirement from userspace
to justify a new ioctl?

Userspace will still need to allocate the same memory in order to get back
the tuner parameters with FE_GET_PROPERTY and to get statistics. Also, tuning
happens just once per channel, so hardly this would bring *any* real 
performance improvement while tuning into a channel.

> diff --git a/drivers/media/dvb-core/dvb_frontend.c 
> b/drivers/media/dvb-core/dvb_frontend.c
> index e3fff8f..7c96197 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -1081,28 +1081,25 @@ static struct dtv_cmds_h dtv_cmds[DTV_MAX_COMMAND + 
> 1] = {
>   _DTV_CMD(DTV_STAT_TOTAL_BLOCK_COUNT, 0, 0),
>  };
>  
> -static void dtv_property_dump(struct dvb_frontend *fe,
> -   bool is_set,
> +static void dtv_get_property_dump(struct dvb_frontend *fe,
> struct dtv_property *tvp)
>  {
>   int i;
>  
>   if (tvp->cmd <= 0 || tvp->cmd > DTV_MAX_COMMAND) {
> - dev_warn(fe->dvb->device, "%s: %s tvp.cmd = 0x%08x undefined\n",
> - __func__,
> - is_set ? "SET" : "GET",
> - tvp->cmd);
> + dev_warn(fe->dvb->device, "%s: GET tvp.cmd = 0x%08x undefined\n"
> +  , __func__,
> +  tvp->cmd);
>   return;
>   }
>  
> - dev_dbg(fe->dvb->device, "%s: %s tvp.cmd= 0x%08x (%s)\n", __func__,
> - is_set ? "SET" : "GET",

Re: [PATCH v13 05/25] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Laurent Pinchart]

Hi Hans,

On Tuesday, 19 September 2017 11:40:14 EEST Hans Verkuil wrote:
> On 09/19/2017 10:20 AM, Sakari Ailus wrote:
> > On Tue, Sep 19, 2017 at 10:03:27AM +0200, Hans Verkuil wrote:
> >> On 09/15/2017 04:17 PM, Sakari Ailus wrote:
> >>> Add two functions for parsing devices graph endpoints:
> >>> v4l2_async_notifier_parse_fwnode_endpoints and
> >>> v4l2_async_notifier_parse_fwnode_endpoints_by_port. The former iterates
> >>> over all endpoints whereas the latter only iterates over the endpoints
> >>> in a given port.
> >>> 
> >>> The former is mostly useful for existing drivers that currently
> >>> implement the iteration over all the endpoints themselves whereas the
> >>> latter is especially intended for devices with both sinks and sources:
> >>> async sub-devices for external devices connected to the device's sources
> >>> will have already been set up, or they are part of the master device.
> >>> 
> >>> Signed-off-by: Sakari Ailus 
> >>> ---
> >>> 
> >>>  drivers/media/v4l2-core/v4l2-async.c  |  30 ++
> >>>  drivers/media/v4l2-core/v4l2-fwnode.c | 185 +++
> >>>  include/media/v4l2-async.h|  24 -
> >>>  include/media/v4l2-fwnode.h   | 117 +
> >>>  4 files changed, 354 insertions(+), 2 deletions(-)

[snip]

> >>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> >>> index 68eb22ba571b..83afac48ea6b 100644
> >>> --- a/include/media/v4l2-fwnode.h
> >>> +++ b/include/media/v4l2-fwnode.h
> >>> @@ -25,6 +25,8 @@
> >>> 
> >>>  #include 
> >>>  
> >>>  struct fwnode_handle;
> >>> +struct v4l2_async_notifier;
> >>> +struct v4l2_async_subdev;
> >>> 
> >>>  #define V4L2_FWNODE_CSI2_MAX_DATA_LANES  4
> >>> 
> >>> @@ -201,4 +203,119 @@ int v4l2_fwnode_parse_link(struct fwnode_handle
> >>> *fwnode,
> >>>   */
> >>>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >>> 
> >>> +/**
> >>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode
> >>> endpoints in a
> >>> + *   device node
> >>> + * @dev: the device the endpoints of which are to be parsed
> >>> + * @notifier: notifier for @dev
> >>> + * @asd_struct_size: size of the driver's async sub-device struct,
> >>> including
> >>> + *sizeof(struct v4l2_async_subdev). The 
> >>> + *v4l2_async_subdev shall be the first member of
> >>> + *the driver's async sub-device struct, i.e. both
> >>> + *begin at the same memory address.
> >>> + * @parse_endpoint: Driver's callback function called on each V4L2
> >>> fwnode
> >>> + *   endpoint. Optional.
> >>> + *   Return: %0 on success
> >>> + *   %-ENOTCONN if the endpoint is to be skipped 
> >>> but this
> >>> + *  should not be considered as an 
> >>> error
> >>> + *   %-EINVAL if the endpoint configuration is 
> >>> invalid
> >>> + *
> >>> + * Parse the fwnode endpoints of the @dev device and populate the async
> >>> sub-
> >>> + * devices array of the notifier. The @parse_endpoint callback function
> >>> is
> >>> + * called for each endpoint with the corresponding async sub-device
> >>> pointer to
> >>> + * let the caller initialize the driver-specific part of the async sub-
> >>> device
> >>> + * structure.
> >>> + *
> >>> + * The notifier memory shall be zeroed before this function is called
> >>> on the
> >>> + * notifier.
> >>> + *
> >>> + * This function may not be called on a registered notifier and may be
> >>> called on
> >>> + * a notifier only once.
> >>> + *
> >>> + * Do not change the notifier's subdevs array, take references to the
> >>> subdevs
> >>> + * array itself or change the notifier's num_subdevs field. This is
> >>> because this
> >>> + * function allocates and reallocates the subdevs array based on
> >>> parsing
> >>> + * endpoints.
> >>> + *
> >>> + * The @struct v4l2_fwnode_endpoint passed to the callback function
> >>> + * @parse_endpoint is released once the function is finished. If there
> >>> is a need
> >>> + * to retain that configuration, the user needs to allocate memory for
> >>> it.
> >>> + *
> >>> + * Any notifier populated using this function must be released with a
> >>> call to
> >>> + * v4l2_async_notifier_release() after it has been unregistered and the
> >>> async
> >>> + * sub-devices are no longer in use, even if the function returned an
> >>> error.
> >>> + *
> >>> + * Return: %0 on success, including when no async sub-devices are found
> >>> + *  %-ENOMEM if memory allocation failed
> >>> + *  %-EINVAL if graph or endpoint parsing failed
> >>> + *  Other error codes as returned by @parse_endpoint
> >>> + */
> >>> +int v4l2_async_notifier_parse_fwnode_endpoints(
> >>> + struct device *dev, struct v4l2_async_notifier *notifier,
> >>> + size_t asd_struct_size,
> >>> + 

Re: [PATCHv2 1/2] dt-bindings: adi,adv7511.txt: document cec clock

[Hans Verkuil]

On 09/19/17 11:20, Sergei Shtylyov wrote:
> Hello!
> 
> On 9/19/2017 10:33 AM, Hans Verkuil wrote:
> 
>> From: Hans Verkuil 
>>
>> Document the cec clock binding.
>>
>> Signed-off-by: Hans Verkuil 
>> Acked-by: Rob Herring 
>> ---
>>   Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 4 
>>   1 file changed, 4 insertions(+)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt 
>> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> index 06668bca7ffc..4497ae054d49 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> @@ -68,6 +68,8 @@ Optional properties:
>>   - adi,disable-timing-generator: Only for ADV7533. Disables the internal 
>> timing
>> generator. The chip will rely on the sync signals in the DSI data lanes,
>> rather than generate its own timings for HDMI output.
>> +- clocks: from common clock binding: handle to CEC clock.
> 
> It's called "phandle" in the DT speak. :-)
> Are you sure the clock specifier would always be absent?

Sorry? I don't understand the question. Did you mean: "can be absent?"?

Regards,

Hans

> 
>> +- clock-names: from common clock binding: must be "cec".
>>   
>>   Required nodes:
>>   
> [...]
> 
> MBR, Sergei
> 

Re: [PATCH v13 18/25] v4l: fwnode: Add a helper function to obtain device / integer references

[Hans Verkuil]

On 09/19/17 10:45, Sakari Ailus wrote:
> Hi Hans,
> 
> Thank you for the review.
> 
> On Tue, Sep 19, 2017 at 10:31:41AM +0200, Hans Verkuil wrote:
>> Hi Sakari,
>>
>> I'm slowly starting to understand this. The example helped a lot. But I 
>> still have
>> some questions, see below.
>>
>> On 09/15/2017 04:17 PM, Sakari Ailus wrote:
>>> v4l2_fwnode_reference_parse_int_prop() will find an fwnode such that under
>>> the device's own fwnode, it will follow child fwnodes with the given
>>> property-value pair and return the resulting fwnode.
>>
>> I think both the subject, commit log, function comment and function name 
>> should
>> reflect the fact that this function is for an ACPI reference.
>>
>> It's only called for ACPI (from patch 19):
>>
>> +if (props[i].props && is_acpi_node(dev_fwnode(dev)))
>> +ret = v4l2_fwnode_reference_parse_int_props(
>>
>> So renaming it to v4l2_fwnode_acpi_reference_parse_int_props or something 
>> similar
>> would clarify this fact.
> 
> I don't think we'll see many like this one. I presume we won't use it on DT
> albeit there are no direct references to ACPI in the code itself.
> 
> How about v4l2_fwnode_parse_acpi_reference (+ "s" for the one below)?

Sounds good.

> 
>>
>>>
>>> Signed-off-by: Sakari Ailus 
>>> ---
>>>  drivers/media/v4l2-core/v4l2-fwnode.c | 201 
>>> ++
>>>  1 file changed, 201 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
>>> b/drivers/media/v4l2-core/v4l2-fwnode.c
>>> index 65e84ea1cc35..968a345a288f 100644
>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>>> @@ -567,6 +567,207 @@ static int v4l2_fwnode_reference_parse(
>>> return ret;
>>>  }
>>>  
>>> +/*
>>> + * v4l2_fwnode_reference_get_int_prop - parse a reference with integer
>>> + * arguments
>>> + * @dev: struct device pointer
>>> + * @notifier: notifier for @dev
>>> + * @prop: the name of the property
>>> + * @index: the index of the reference to get
>>> + * @props: the array of integer property names
>>> + * @nprops: the number of integer property names in @nprops
>>
>> You mean 'in @props'?
> 
> Yes, I'll fix that.
> 
>>
>> One thing that is not clear to me is when you would use an nprops value > 1.
>> What's the use-case for that? It only makes sense (I think) if you would have
>> property names that are all aliases of one another.
> 
> There may be several flash LEDs related to a sensor. That's the use case,
> for instance.

I think it would be helpful if the example shows two LEDs related to a
sensor. Part of the problem I have in understanding this code is that I
have zero experience with ACPI (and that is probably true for most other
developers), so I don't know how this is encoded. With a good example it
is much easier to understand.

> 
>>
>>> + *
>>> + * Find fwnodes referred to by a property @prop, then under that
>>> + * iteratively, @nprops times, follow each child node which has a
>>> + * property in @props array at a given child index the value of which
>>> + * matches the integer argument at an index.
>>> + *
>>> + * For example, if this function was called with arguments and values
>>> + * @dev corresponding to device "SEN", @prop == "flash-leds", @index
>>> + * == 1, @props == { "led" }, @nprops == 1, with the ASL snippet below
>>> + * it would return the node marked with THISONE. The @dev argument in
>>> + * the ASL below.
>>
>> That last sentence about the @dev seems incomplete. I'm not sure what is
>> meant by it.
> 
> I think it was meant to convey some information but it got added to the
> previous sentence. I'll remove it.
> 
>>
>>> + *
>>> + * Device (LED)
>>> + * {
>>> + * Name (_DSD, Package () {
>>> + * ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>>> + * Package () {
>>> + * Package () { "led0", "LED0" },
>>> + * Package () { "led1", "LED1" },
>>> + * }
>>> + * })
>>> + * Name (LED0, Package () {
>>> + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>> + * Package () {
>>> + * Package () { "led", 0 },
>>> + * }
>>> + * })
>>> + * Name (LED1, Package () {
>>> + * // THISONE
>>> + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>> + * Package () {
>>> + * Package () { "led", 1 },
>>> + * }
>>> + * })
>>> + * }
>>> + *
>>> + * Device (SEN)
>>> + * {
>>> + * Name (_DSD, Package () {
>>> + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>> + * Package () {
>>> + * Package () {
>>> + * "flash-leds",
>>> + * Package () { ^LED, 0, ^LED, 1 

Re: [PATCH v13 02/25] v4l: async: Remove re-probing support

[Laurent Pinchart]

Hi Sakari,

(CC'ing Mike Turquette)

Thank you for the patch.

On Friday, 15 September 2017 17:17:01 EEST Sakari Ailus wrote:
> Remove V4L2 async re-probing support. The re-probing support has been
> there to support cases where the sub-devices require resources provided by
> the main driver's hardware to function, such as clocks.
> 
> Reprobing has allowed unbinding and again binding the main driver without
> explicilty unbinding the sub-device drivers. This is certainly not a
> common need, and the responsibility will be the user's going forward.
> 
> An alternative could have been to introduce notifier specific locks.
> Considering the complexity of the re-probing and that it isn't really a
> solution to a problem but a workaround, remove re-probing instead.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 

As stated before we need a plan to fix the issue that reprobind was supposed 
to address.

To my knowledge the only intended user of the reprobind code was the OMAP3 ISP 
when it provides a clock to the sensor. I've briefly discussed this with Mike 
last week, and he believed we could handle the issue by "un-orphaning" the 
orphaned clock when the OMAP3 ISP is reprobed. Mike, have you had time to 
check whether that would be feasible without too much effort and/or pain ?

In the meantime, for this patch,

Acked-by: Laurent Pinchart 

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 54 
>  1 file changed, 54 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index d741a8e0fdac..e109d9da4653
> 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -198,78 +198,24 @@ EXPORT_SYMBOL(v4l2_async_notifier_register);
>  void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  {
>   struct v4l2_subdev *sd, *tmp;
> - unsigned int notif_n_subdev = notifier->num_subdevs;
> - unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
> - struct device **dev;
> - int i = 0;
> 
>   if (!notifier->v4l2_dev)
>   return;
> 
> - dev = kvmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
> - if (!dev) {
> - dev_err(notifier->v4l2_dev->dev,
> - "Failed to allocate device cache!\n");
> - }
> -
>   mutex_lock(_lock);
> 
>   list_del(>list);
> 
>   list_for_each_entry_safe(sd, tmp, >done, async_list) {
> - struct device *d;
> -
> - d = get_device(sd->dev);
> -
>   v4l2_async_cleanup(sd);
> 
> - /* If we handled USB devices, we'd have to lock the parent too 
> */
> - device_release_driver(d);
> -
>   if (notifier->unbind)
>   notifier->unbind(notifier, sd, sd->asd);
> -
> - /*
> -  * Store device at the device cache, in order to call
> -  * put_device() on the final step
> -  */
> - if (dev)
> - dev[i++] = d;
> - else
> - put_device(d);
>   }
> 
>   mutex_unlock(_lock);
> 
> - /*
> -  * Call device_attach() to reprobe devices
> -  *
> -  * NOTE: If dev allocation fails, i is 0, and the whole loop won't be
> -  * executed.
> -  */
> - while (i--) {
> - struct device *d = dev[i];
> -
> - if (d && device_attach(d) < 0) {
> - const char *name = "(none)";
> - int lock = device_trylock(d);
> -
> - if (lock && d->driver)
> - name = d->driver->name;
> - dev_err(d, "Failed to re-probe to %s\n", name);
> - if (lock)
> - device_unlock(d);
> - }
> - put_device(d);
> - }
> - kvfree(dev);
> -
>   notifier->v4l2_dev = NULL;
> -
> - /*
> -  * Don't care about the waiting list, it is initialised and populated
> -  * upon notifier registration.
> -  */
>  }
>  EXPORT_SYMBOL(v4l2_async_notifier_unregister);


-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 1/2] dt-bindings: adi,adv7511.txt: document cec clock

[Sergei Shtylyov]

Hello!

On 9/19/2017 10:33 AM, Hans Verkuil wrote:


From: Hans Verkuil 

Document the cec clock binding.

Signed-off-by: Hans Verkuil 
Acked-by: Rob Herring 
---
  Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 4 
  1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt 
b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 06668bca7ffc..4497ae054d49 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -68,6 +68,8 @@ Optional properties:
  - adi,disable-timing-generator: Only for ADV7533. Disables the internal timing
generator. The chip will rely on the sync signals in the DSI data lanes,
rather than generate its own timings for HDMI output.
+- clocks: from common clock binding: handle to CEC clock.


   It's called "phandle" in the DT speak. :-)
   Are you sure the clock specifier would always be absent?


+- clock-names: from common clock binding: must be "cec".
  
  Required nodes:
  

[...]

MBR, Sergei

Re: [PATCH 5/6] [media] go7007: Use common error handling code in go7007_snd_init()

[Dan Carpenter]

On Mon, Sep 18, 2017 at 03:58:37PM +0200, SF Markus Elfring wrote:
> diff --git a/drivers/media/usb/go7007/snd-go7007.c 
> b/drivers/media/usb/go7007/snd-go7007.c
> index 68e421bf38e1..7ae4d03ed3f7 100644
> --- a/drivers/media/usb/go7007/snd-go7007.c
> +++ b/drivers/media/usb/go7007/snd-go7007.c
> @@ -243,22 +243,18 @@ int go7007_snd_init(struct go7007 *go)
>   gosnd->capturing = 0;
>   ret = snd_card_new(go->dev, index[dev], id[dev], THIS_MODULE, 0,
>  >card);
> - if (ret < 0) {
> - kfree(gosnd);
> - return ret;
> - }
> + if (ret < 0)
> + goto free_snd;
> +
>   ret = snd_device_new(gosnd->card, SNDRV_DEV_LOWLEVEL, go,
>   _snd_device_ops);
> - if (ret < 0) {
> - kfree(gosnd);
> - return ret;
> - }
> + if (ret < 0)
> + goto free_snd;
> +


I think the original code is buggy.  It should probably call
snd_card_free() if snd_device_new() fails.

regards,
dan carpenter