Re: [PATCH -next v3 2/2] media: ov772x: use SCCB helpers

[Sebastian Reichel]

Hi,

On Mon, Jul 09, 2018 at 06:14:43PM +0200, Wolfram Sang wrote:
> >  static int ov772x_read(struct i2c_client *client, u8 addr)
> >  {
> > -   int ret;
> > -   u8 val;
> > -
> > -   ret = i2c_master_send(client, , 1);
> > -   if (ret < 0)
> > -   return ret;
> > -   ret = i2c_master_recv(client, , 1);
> > -   if (ret < 0)
> > -   return ret;
> > -
> > -   return val;
> > +   return sccb_read_byte(client, addr);
> >  }
> >  
> >  static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 
> > value)
> >  {
> > -   return i2c_smbus_write_byte_data(client, addr, value);
> > +   return sccb_write_byte(client, addr, value);
> >  }

Reviewed-by: Sebastian Reichel 

> Minor nit: I'd rather drop these two functions and use the
> sccb-accessors directly.
> 
> However, I really like how this looks here: It is totally clear we are
> doing SCCB and hide away all the details.

I think it would be even better to introduce a SSCB regmap layer
and use that.

-- Sebastian


signature.asc
Description: PGP signature

Re: [RFC PATCH v2] media: i2c: add SCCB helpers

[Sebastian Reichel]

Hi,

On Wed, Jun 13, 2018 at 12:34:46AM +0900, Akinobu Mita wrote:
> (This is 2nd version of SCCB helpers patch.  After 1st version was
> submitted, I sent alternative patch titled "i2c: add I2C_M_FORCE_STOP".
> But it wasn't accepted because it makes the I2C core code unreadable.
> I couldn't find out a way to untangle it, so I returned to the original
> approach.)
> 
> This adds Serial Camera Control Bus (SCCB) helper functions (sccb_read_byte
> and sccb_write_byte) that are intended to be used by some of Omnivision
> sensor drivers.
> 
> The ov772x driver is going to use these functions in order to make it work
> with most i2c controllers.
> 
> As the ov772x device doesn't support repeated starts, this driver currently
> requires I2C_FUNC_PROTOCOL_MANGLING that is not supported by many i2c
> controller drivers.
> 
> With the sccb_read_byte() that issues two separated requests in order to
> avoid repeated start, the driver doesn't require I2C_FUNC_PROTOCOL_MANGLING.
> 
> Cc: Sebastian Reichel 
> Cc: Wolfram Sang 
> Cc: Jacopo Mondi 
> Cc: Laurent Pinchart 
> Cc: Hans Verkuil 
> Cc: Sakari Ailus 
> Cc: Mauro Carvalho Chehab 
> Signed-off-by: Akinobu Mita 
> ---

Reviewed-by: Sebastian Reichel 

-- Sebastian

> * v2
> - Convert all helpers into static inline functions, and remove C source
>   and Kconfig option.
> - Acquire i2c adapter lock while issuing two requests for sccb_read_byte
> 
>  drivers/media/i2c/sccb.h | 74 
> 
>  1 file changed, 74 insertions(+)
>  create mode 100644 drivers/media/i2c/sccb.h
> 
> diff --git a/drivers/media/i2c/sccb.h b/drivers/media/i2c/sccb.h
> new file mode 100644
> index 000..a531fdc
> --- /dev/null
> +++ b/drivers/media/i2c/sccb.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Serial Camera Control Bus (SCCB) helper functions
> + */
> +
> +#ifndef __SCCB_H__
> +#define __SCCB_H__
> +
> +#include 
> +
> +/**
> + * sccb_read_byte - Read data from SCCB slave device
> + * @client: Handle to slave device
> + * @addr: Register to be read from
> + *
> + * This executes the 2-phase write transmission cycle that is followed by a
> + * 2-phase read transmission cycle, returning negative errno else a data byte
> + * received from the device.
> + */
> +static inline int sccb_read_byte(struct i2c_client *client, u8 addr)
> +{
> + u8 val;
> + struct i2c_msg msg[] = {
> + {
> + .addr = client->addr,
> + .len = 1,
> + .buf = ,
> + },
> + {
> + .addr = client->addr,
> + .flags = I2C_M_RD,
> + .len = 1,
> + .buf = ,
> + },
> + };
> + int ret;
> + int i;
> +
> + i2c_lock_adapter(client->adapter);
> +
> + /* Issue two separated requests in order to avoid repeated start */
> + for (i = 0; i < 2; i++) {
> + ret = __i2c_transfer(client->adapter, [i], 1);
> + if (ret != 1)
> + break;
> + }
> +
> + i2c_unlock_adapter(client->adapter);
> +
> + return i == 2 ? val : ret;
> +}
> +
> +/**
> + * sccb_write_byte - Write data to SCCB slave device
> + * @client: Handle to slave device
> + * @addr: Register to write to
> + * @data: Value to be written
> + *
> + * This executes the SCCB 3-phase write transmission cycle, returning 
> negative
> + * errno else zero on success.
> + */
> +static inline int sccb_write_byte(struct i2c_client *client, u8 addr, u8 
> data)
> +{
> + int ret;
> + unsigned char msgbuf[] = { addr, data };
> +
> + ret = i2c_master_send(client, msgbuf, 2);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +#endif /* __SCCB_H__ */
> -- 
> 2.7.4
> 


signature.asc
Description: PGP signature

Re: [PATCH v2.2 2/2] smiapp: Support the "rotation" property

[Sebastian Reichel]

Hi,

On Fri, May 25, 2018 at 04:52:35PM +0300, Sakari Ailus wrote:
> Use the "rotation" property to tell that the sensor is mounted upside
> down. This reverses the behaviour of the VFLIP and HFLIP controls as well
> as the pixel order.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> since v2.2:
> 
> - Fix property name in code.
> 
>  .../devicetree/bindings/media/i2c/nokia,smia.txt |  2 ++
>  drivers/media/i2c/smiapp/smiapp-core.c   | 16 
> 
>  2 files changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt 
> b/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
> index 33f10a94c381..6f509657470e 100644
> --- a/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
> @@ -29,6 +29,8 @@ Optional properties
>  - reset-gpios: XSHUTDOWN GPIO
>  - flash-leds: See ../video-interfaces.txt
>  - lens-focus: See ../video-interfaces.txt
> +- rotation: Integer property; valid values are 0 (sensor mounted upright)
> + and 180 (sensor mounted upside down).
>  
>  
>  Endpoint node mandatory properties
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
> b/drivers/media/i2c/smiapp/smiapp-core.c
> index e1f8208581aa..e9e0f21efc2a 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -2764,6 +2764,7 @@ static struct smiapp_hwconfig 
> *smiapp_get_hwconfig(struct device *dev)
>   struct v4l2_fwnode_endpoint *bus_cfg;
>   struct fwnode_handle *ep;
>   struct fwnode_handle *fwnode = dev_fwnode(dev);
> + u32 rotation;
>   int i;
>   int rval;
>  
> @@ -2800,6 +2801,21 @@ static struct smiapp_hwconfig 
> *smiapp_get_hwconfig(struct device *dev)
>  
>   dev_dbg(dev, "lanes %u\n", hwcfg->lanes);
>  
> + rval = fwnode_property_read_u32(fwnode, "rotation", );
> + if (!rval) {
> + switch (rotation) {
> + case 180:
> + hwcfg->module_board_orient =
> + SMIAPP_MODULE_BOARD_ORIENT_180;
> + /* Fall through */
> + case 0:
> + break;
> + default:
> + dev_err(dev, "invalid rotation %u\n", rotation);
> + goto out_err;
> + }
> + }
> +
>   /* NVM size is not mandatory */
>   fwnode_property_read_u32(fwnode, "nokia,nvm-size", >nvm_size);
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v2 1/2] dt-bindings: media: Define "rotation" property for sensors

[Sebastian Reichel]

Hi,

On Fri, May 25, 2018 at 03:27:25PM +0300, Sakari Ailus wrote:
> Sensors are occasionally mounted upside down to systems such as mobile
> phones or tablets. In order to use such a sensor without having to turn
> every image upside down, most camera sensors support reversing the readout
> order by setting both horizontal and vertical flipping.
> 
> This patch documents the "rotation" property for camera sensors, mirroring
> what is defined for displays in
> Documentation/devicetree/bindings/display/panel/panel.txt .
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  Documentation/devicetree/bindings/media/video-interfaces.txt | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt 
> b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 258b8dfddf48..52b7c7b57842 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -85,6 +85,10 @@ Optional properties
>  
>  - lens-focus: A phandle to the node of the focus lens controller.
>  
> +- rotation: The device, typically an image sensor, is not mounted upright,
> +  but a number of degrees counter clockwise. Typical values are 0 and 180
> +  (upside down).
> +
>  
>  Optional endpoint properties
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] smiapp: Support the "upside-down" property

[Sebastian Reichel]

Hi,

On Fri, May 25, 2018 at 03:27:26PM +0300, Sakari Ailus wrote:
> Use the "upside-down" property to tell that the sensor is mounted upside
> down. This reverses the behaviour of the VFLIP and HFLIP controls as well
> as the pixel order.
> 
> Signed-off-by: Sakari Ailus 
> ---

Patch subject and description should be s/"upside-down"/"rotation"/g ?

>  .../devicetree/bindings/media/i2c/nokia,smia.txt |  2 ++
>  drivers/media/i2c/smiapp/smiapp-core.c   | 16 
> 
>  2 files changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt 
> b/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
> index 33f10a94c381..6f509657470e 100644
> --- a/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
> @@ -29,6 +29,8 @@ Optional properties
>  - reset-gpios: XSHUTDOWN GPIO
>  - flash-leds: See ../video-interfaces.txt
>  - lens-focus: See ../video-interfaces.txt
> +- rotation: Integer property; valid values are 0 (sensor mounted upright)
> + and 180 (sensor mounted upside down).
>  
>  
>  Endpoint node mandatory properties
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
> b/drivers/media/i2c/smiapp/smiapp-core.c
> index e1f8208581aa..32286df6ab43 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -2764,6 +2764,7 @@ static struct smiapp_hwconfig 
> *smiapp_get_hwconfig(struct device *dev)
>   struct v4l2_fwnode_endpoint *bus_cfg;
>   struct fwnode_handle *ep;
>   struct fwnode_handle *fwnode = dev_fwnode(dev);
> + u32 rotation;
>   int i;
>   int rval;
>  
> @@ -2800,6 +2801,21 @@ static struct smiapp_hwconfig 
> *smiapp_get_hwconfig(struct device *dev)
>  
>   dev_dbg(dev, "lanes %u\n", hwcfg->lanes);
>  
> + rval = fwnode_property_read_u32(fwnode, "upside-down", );

"rotation"

> + if (!rval) {
> + switch (rotation) {
> + case 180:
> + hwcfg->module_board_orient =
> + SMIAPP_MODULE_BOARD_ORIENT_180;
> + /* Fall through */
> + case 0:
> + break;
> + default:
> + dev_err(dev, "invalid rotation %u\n", rotation);
> + goto out_err;
> + }
> + }
> +
>   /* NVM size is not mandatory */
>   fwnode_property_read_u32(fwnode, "nokia,nvm-size", >nvm_size);

-- Sebastian


signature.asc
Description: PGP signature

Re: [RFC PATCH] media: i2c: add SCCB helpers

[Sebastian Reichel]

Hi,

On Fri, Apr 27, 2018 at 01:13:32AM +0900, Akinobu Mita wrote:
> diff --git a/drivers/media/i2c/sccb.c b/drivers/media/i2c/sccb.c
> new file mode 100644
> index 000..80a3fb7
> --- /dev/null
> +++ b/drivers/media/i2c/sccb.c
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include 
> +
> +int sccb_read_byte(struct i2c_client *client, u8 addr)
> +{
> + int ret;
> + u8 val;
> +
> + /* Issue two separated requests in order to avoid repeated start */
> +
> + ret = i2c_master_send(client, , 1);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_master_recv(client, , 1);
> + if (ret < 0)
> + return ret;
> +
> + return val;
> +}
> +EXPORT_SYMBOL_GPL(sccb_read_byte);
> +
> +int sccb_write_byte(struct i2c_client *client, u8 addr, u8 data)
> +{
> + int ret;
> + unsigned char msgbuf[] = { addr, data };
> +
> + ret = i2c_master_send(client, msgbuf, 2);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(sccb_write_byte);
> diff --git a/drivers/media/i2c/sccb.h b/drivers/media/i2c/sccb.h
> new file mode 100644
> index 000..68da0e9
> --- /dev/null
> +++ b/drivers/media/i2c/sccb.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * SCCB helper functions
> + */
> +
> +#ifndef __SCCB_H__
> +#define __SCCB_H__
> +
> +#include 
> +
> +int sccb_read_byte(struct i2c_client *client, u8 addr);
> +int sccb_write_byte(struct i2c_client *client, u8 addr, u8 data);
> +
> +#endif /* __SCCB_H__ */

The functions look very simple. Have you considered moving them into
sccb.h as static inline?

-- Sebastian


signature.asc
Description: PGP signature

4.17-rc3 regression in UVC driver

[Sebastian Reichel]

Hi,

I just got the following error message every ms with 4.17-rc3 after
upgrading to for first ~192 seconds after system start (Debian
4.17~rc3-1~exp1 kernel) on my Thinkpad X250:

> uvcvideo: Failed to query (GET_MIN) UVC control 2 on unit 1: -32 (exp. 1).

I see /dev/video0 and /dev/video1. The first one seems to be
functional. The second one does not work and does not make
sense to me (the system has only one webcam). I did not try to
bisect anything. Here is some more information, that might
be useful:

> sre@earth ~ % mpv /dev/video1 
> Playing: /dev/video1
> [ffmpeg/demuxer] video4linux2,v4l2: ioctl(VIDIOC_G_INPUT): Inappropriate 
> ioctl for device
> [lavf] avformat_open_input() failed
> Failed to recognize file format.
> sre@earth ~ % udevadm info /dev/video0
> P: /devices/pci:00/:00:14.0/usb1/1-8/1-8:1.0/video4linux/video0
> N: video0
> E: DEVNAME=/dev/video0
> E: 
> DEVPATH=/devices/pci:00/:00:14.0/usb1/1-8/1-8:1.0/video4linux/video0
> E: MAJOR=81
> E: MINOR=0
> E: SUBSYSTEM=video4linux
> sre@earth ~ % udevadm info /dev/video1
> P: /devices/pci:00/:00:14.0/usb1/1-8/1-8:1.0/video4linux/video1
> N: video1
> E: DEVNAME=/dev/video1
> E: 
> DEVPATH=/devices/pci:00/:00:14.0/usb1/1-8/1-8:1.0/video4linux/video1
> E: MAJOR=81
> E: MINOR=1
> E: SUBSYSTEM=video4linux
> sre@earth ~ % lsusb -d 04ca:703c
> Bus 001 Device 004: ID 04ca:703c Lite-On Technology Corp. 

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH 1/2] dt-bindings: media: Add "upside-down" property to tell sensor orientation

[Sebastian Reichel]

Hi,

On Thu, May 03, 2018 at 12:31:14AM +0300, Sakari Ailus wrote:
> Camera sensors are occasionally mounted upside down. In order to use such
> a sensor without having to turn every image upside down separately, most
> camera sensors support reversing the readout order by setting both
> horizontal and vertical flipping.
> 
> This patch adds a boolean property to tell a sensor is mounted upside
> down.
> 
> Signed-off-by: Sakari Ailus 

I think the DT binding should use a rotation property instead,
similar to the panel bindings:

Documentation/devicetree/bindings/display/panel/panel.txt

-- Sebastian

> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt 
> b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 258b8dfddf48..2a3e4ec4ea27 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -85,6 +85,9 @@ Optional properties
>  
>  - lens-focus: A phandle to the node of the focus lens controller.
>  
> +- upside-down: The device, typically an image sensor, is mounted upside
> +  down in the system.
> +
>  
>  Optional endpoint properties
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH] media: et8ek8: select V4L2_FWNODE

[Sebastian Reichel]

Hi,

On Mon, Nov 13, 2017 at 02:56:45PM +0100, Arnd Bergmann wrote:
> v4l2_async_register_subdev_sensor_common() is only provided when
> CONFIG_V4L2_FWNODE is enabled, otherwise we get a link failure:
> 
> drivers/media/i2c/et8ek8/et8ek8_driver.o: In function `et8ek8_probe':
> et8ek8_driver.c:(.text+0x884): undefined reference to 
> `v4l2_async_register_subdev_sensor_common'
> 
> This adds a Kconfig 'select' statement like all the other users of
> this interface have.
> 
> Fixes: d8932f38c10f ("media: et8ek8: Add support for flash and lens devices")
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/i2c/et8ek8/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/i2c/et8ek8/Kconfig 
> b/drivers/media/i2c/et8ek8/Kconfig
> index 14399365ad7f..9fe409e95666 100644
> --- a/drivers/media/i2c/et8ek8/Kconfig
> +++ b/drivers/media/i2c/et8ek8/Kconfig
> @@ -1,6 +1,7 @@
>  config VIDEO_ET8EK8
>   tristate "ET8EK8 camera sensor support"
>   depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + select V4L2_FWNODE
>   ---help---
> This is a driver for the Toshiba ET8EK8 5 MP camera sensor.
> It is used for example in Nokia N900 (RX-51).
> -- 
> 2.9.0
> 


signature.asc
Description: PGP signature

Re: et8ek8: Document support for flash and lens devices

[Sebastian Reichel]

Hi,

On Sun, Nov 12, 2017 at 12:27:29PM +0100, Pavel Machek wrote:
> 
> Document dts support of LED/focus.
> 
> Signed-off-by: Pavel Machek <pa...@ucw.cz>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> diff --git a/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt 
> b/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
> index 0b7b6a4..e80d589 100644
> --- a/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
> @@ -20,6 +20,13 @@ Mandatory properties
>is in hardware standby mode when the signal is in the low state.
>  
>  
> +Optional properties
> +---
> +
> +- flash-leds: See ../video-interfaces.txt
> +- lens-focus: See ../video-interfaces.txt
> +
> +
>  Endpoint node mandatory properties
>  --
>  
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) 
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html




signature.asc
Description: PGP signature

Re: [PATCH] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Sebastian Reichel]

Hi,

On Wed, Nov 01, 2017 at 11:55:33AM +, Sean Young wrote:
> The ir-rx51 is a pwm-based TX driver specific to the n900. This can be
> handled entirely by the generic pwm-ir-tx driver.
> 
> Note that the suspend code in the ir-rx51 driver is unnecessary, since
> during transmit, the current process is not in interruptable sleep. The
> process is not put to sleep until the transmit completes.

Nice to see, that we can use generic driver now :)

> Cc: Ivaylo Dimitrov 
> Cc: Pali Rohár 
> Cc: Pavel Machek 
> Cc: Timo Kokkonen 
> Signed-off-by: Sean Young 
> ---
>  arch/arm/boot/dts/omap3-n900.dts |   2 +-
>  drivers/media/rc/Kconfig |  10 --
>  drivers/media/rc/Makefile|   1 -
>  drivers/media/rc/ir-rx51.c   | 316 
> ---
>  4 files changed, 1 insertion(+), 328 deletions(-)
>  delete mode 100644 drivers/media/rc/ir-rx51.c
> 
> diff --git a/arch/arm/boot/dts/omap3-n900.dts 
> b/arch/arm/boot/dts/omap3-n900.dts
> index 4acd32a1c4ef..fccd2b119c0a 100644
> --- a/arch/arm/boot/dts/omap3-n900.dts
> +++ b/arch/arm/boot/dts/omap3-n900.dts
> @@ -152,7 +152,7 @@
>   };
>  
>   ir: n900-ir {
> - compatible = "nokia,n900-ir";
> + compatible = "pwm-ir-tx";

I think we should update DTS to look like this:

compatible = "nokia,n900-ir", "pwm-ir-tx";

This will keep new DTS working with old kernel. Also we want it
working the other way around (old DTS with new kernel), so we
need to add the "nokia,n900-ir" compatible to the generic PWM
IR driver.

Also the DT binding document needs update:

Documentation/devicetree/bindings/media/nokia,n900-ir

[Adding Rob Herring to Cc]

-- Sebastian

>   pwms = < 0 26316 0>; /* 38000 Hz */
>   };
>  
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index 0f863822889e..354ee5224758 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -353,16 +353,6 @@ config IR_TTUSBIR
>  To compile this driver as a module, choose M here: the module will
>  be called ttusbir.
>  
> -config IR_RX51
> - tristate "Nokia N900 IR transmitter diode"
> - depends on (OMAP_DM_TIMER && PWM_OMAP_DMTIMER && ARCH_OMAP2PLUS || 
> COMPILE_TEST) && RC_CORE
> - ---help---
> -Say Y or M here if you want to enable support for the IR
> -transmitter diode built in the Nokia N900 (RX51) device.
> -
> -The driver uses omap DM timers for generating the carrier
> -wave and pulses.
> -
>  source "drivers/media/rc/img-ir/Kconfig"
>  
>  config RC_LOOPBACK
> diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
> index a1ef86767aef..59b99a2dc2d4 100644
> --- a/drivers/media/rc/Makefile
> +++ b/drivers/media/rc/Makefile
> @@ -25,7 +25,6 @@ obj-$(CONFIG_IR_MESON) += meson-ir.o
>  obj-$(CONFIG_IR_NUVOTON) += nuvoton-cir.o
>  obj-$(CONFIG_IR_ENE) += ene_ir.o
>  obj-$(CONFIG_IR_REDRAT3) += redrat3.o
> -obj-$(CONFIG_IR_RX51) += ir-rx51.o
>  obj-$(CONFIG_IR_SPI) += ir-spi.o
>  obj-$(CONFIG_IR_STREAMZAP) += streamzap.o
>  obj-$(CONFIG_IR_WINBOND_CIR) += winbond-cir.o
> diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
> deleted file mode 100644
> index 49265f02e772..
> --- a/drivers/media/rc/ir-rx51.c
> +++ /dev/null
> @@ -1,316 +0,0 @@
> -/*
> - *  Copyright (C) 2008 Nokia Corporation
> - *
> - *  Based on lirc_serial.c
> - *
> - *  This program is free software; you can redistribute it and/or modify
> - *  it under the terms of the GNU General Public License as published by
> - *  the Free Software Foundation; either version 2 of the License, or
> - *  (at your option) any later version.
> - *
> - *  This program is distributed in the hope that it will be useful,
> - *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> - *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - *  GNU General Public License for more details.
> - */
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -
> -#include 
> -#include 
> -
> -#define WBUF_LEN 256
> -
> -struct ir_rx51 {
> - struct rc_dev *rcdev;
> - struct pwm_device *pwm;
> - struct hrtimer timer;
> - struct device*dev;
> - struct ir_rx51_platform_data *pdata;
> - wait_queue_head_t wqueue;
> -
> - unsigned intfreq;   /* carrier frequency */
> - unsigned intduty_cycle; /* carrier duty cycle */
> - int wbuf[WBUF_LEN];
> - int wbuf_index;
> - unsigned long   device_is_open;
> -};
> -
> -static inline void ir_rx51_on(struct ir_rx51 *ir_rx51)
> -{
> - pwm_enable(ir_rx51->pwm);
> -}
> -
> -static inline void ir_rx51_off(struct ir_rx51 *ir_rx51)
> -{
> - pwm_disable(ir_rx51->pwm);
> -}
> -
> -static int init_timing_params(struct ir_rx51 *ir_rx51)
> -{
> - struct pwm_device 

Re: [PATCH] media: v4l2-fwnode: use the cached value instead of getting again

[Sebastian Reichel]

Hi,

On Tue, Oct 31, 2017 at 02:22:59PM -0400, Mauro Carvalho Chehab wrote:
> There is a get/put operation in order to get firmware is_available
> data there at the __v4l2_async_notifier_parse_fwnode_endpoints()
> function. However, instead of using it, the code just reads again
> without the lock. That's probably a mistake, as a similar code on
> another function use the cached value.
> 
> This solves this smatch warning:
> 
> drivers/media/v4l2-core/v4l2-fwnode.c:453:8: warning: variable 'is_available' 
> set but not used [-Wunused-but-set-variable]
>bool is_available;
> ^~~~
> 
> Fixes: 9ca465312132 ("media: v4l: fwnode: Support generic parsing of graph 
> endpoints in a device")
> Cc: Sakari Ailus <sakari.ai...@iki.fi>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/v4l2-core/v4l2-fwnode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 3b9c6afb49a3..681b192420d9 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -455,8 +455,7 @@ static int __v4l2_async_notifier_parse_fwnode_endpoints(
>   dev_fwnode = fwnode_graph_get_port_parent(fwnode);
>   is_available = fwnode_device_is_available(dev_fwnode);
>   fwnode_handle_put(dev_fwnode);
> -
> - if (!fwnode_device_is_available(dev_fwnode))
> + if (!is_available)
>   continue;
>  
>   if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {
> -- 
> 2.13.6
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 32/32] arm: dts: omap3: N9/N950: Add flash references to the camera

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:42AM +0300, Sakari Ailus wrote:
> Add flash and indicator LED phandles to the sensor node.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  arch/arm/boot/dts/omap3-n9.dts   | 1 +
>  arch/arm/boot/dts/omap3-n950-n9.dtsi | 4 ++--
>  arch/arm/boot/dts/omap3-n950.dts | 1 +
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/omap3-n9.dts b/arch/arm/boot/dts/omap3-n9.dts
> index b9e58c536afd..39e35f8b8206 100644
> --- a/arch/arm/boot/dts/omap3-n9.dts
> +++ b/arch/arm/boot/dts/omap3-n9.dts
> @@ -26,6 +26,7 @@
>   clocks = < 0>;
>   clock-frequency = <960>;
>   nokia,nvm-size = <(16 * 64)>;
> + flash-leds = <_flash _indicator>;
>   port {
>   smia_1_1: endpoint {
>   link-frequencies = /bits/ 64 <19920 
> 21000 49920>;
> diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi 
> b/arch/arm/boot/dts/omap3-n950-n9.dtsi
> index 1b0bd72945f2..12fbb3da5fce 100644
> --- a/arch/arm/boot/dts/omap3-n950-n9.dtsi
> +++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
> @@ -271,14 +271,14 @@
>   #size-cells = <0>;
>   reg = <0x30>;
>   compatible = "ams,as3645a";
> - flash@0 {
> + as3645a_flash: flash@0 {
>   reg = <0x0>;
>   flash-timeout-us = <15>;
>   flash-max-microamp = <32>;
>   led-max-microamp = <6>;
>   ams,input-max-microamp = <175>;
>   };
> - indicator@1 {
> + as3645a_indicator: indicator@1 {
>   reg = <0x1>;
>   led-max-microamp = <1>;
>   };
> diff --git a/arch/arm/boot/dts/omap3-n950.dts 
> b/arch/arm/boot/dts/omap3-n950.dts
> index 646601a3ebd8..c354a1ed1e70 100644
> --- a/arch/arm/boot/dts/omap3-n950.dts
> +++ b/arch/arm/boot/dts/omap3-n950.dts
> @@ -60,6 +60,7 @@
>   clocks = < 0>;
>   clock-frequency = <960>;
>   nokia,nvm-size = <(16 * 64)>;
> + flash-leds = <_flash _indicator>;
>   port {
>   smia_1_1: endpoint {
>   link-frequencies = /bits/ 64 <21000 
> 33360 39840>;
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 31/32] ov13858: Add support for flash and lens devices

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:41AM +0300, Sakari Ailus wrote:
> Parse async sub-devices related to the sensor by switching the async
> sub-device registration function.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/i2c/ov13858.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c
> index fdce2befed02..bf7d06f3f21a 100644
> --- a/drivers/media/i2c/ov13858.c
> +++ b/drivers/media/i2c/ov13858.c
> @@ -1761,7 +1761,7 @@ static int ov13858_probe(struct i2c_client *client,
>   goto error_handler_free;
>   }
>  
> - ret = v4l2_async_register_subdev(>sd);
> + ret = v4l2_async_register_subdev_sensor_common(>sd);
>   if (ret < 0)
>   goto error_media_entity;
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 30/32] ov5670: Add support for flash and lens devices

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:40AM +0300, Sakari Ailus wrote:
> Parse async sub-devices related to the sensor by switching the async
> sub-device registration function.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/i2c/ov5670.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index a65469f88e36..9f9196568eb8 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -2529,7 +2529,7 @@ static int ov5670_probe(struct i2c_client *client)
>   }
>  
>   /* Async register for subdev */
> - ret = v4l2_async_register_subdev(>sd);
> + ret = v4l2_async_register_subdev_sensor_common(>sd);
>   if (ret < 0) {
>   err_msg = "v4l2_async_register_subdev() error";
>   goto error_entity_cleanup;
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 29/32] et8ek8: Add support for flash and lens devices

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:39AM +0300, Sakari Ailus wrote:
> Parse async sub-devices related to the sensor by switching the async
> sub-device registration function.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/i2c/et8ek8/et8ek8_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c 
> b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> index c14f0fd6ded3..e9eff9039ef5 100644
> --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
> +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> @@ -1453,7 +1453,7 @@ static int et8ek8_probe(struct i2c_client *client,
>   goto err_mutex;
>   }
>  
> - ret = v4l2_async_register_subdev(>subdev);
> + ret = v4l2_async_register_subdev_sensor_common(>subdev);
>   if (ret < 0)
>   goto err_entity;
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 28/32] smiapp: Add support for flash and lens devices

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:38AM +0300, Sakari Ailus wrote:
> Parse async sub-devices related to the sensor by switching the async
> sub-device registration function.
> 
> 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 <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/i2c/smiapp/smiapp-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
> b/drivers/media/i2c/smiapp/smiapp-core.c
> index fbd851be51d2..a87c50373813 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -3118,7 +3118,7 @@ static int smiapp_probe(struct i2c_client *client,
>   if (rval < 0)
>   goto out_media_entity_cleanup;
>  
> - rval = v4l2_async_register_subdev(>src->sd);
> + rval = v4l2_async_register_subdev_sensor_common(>src->sd);
>   if (rval < 0)
>   goto out_media_entity_cleanup;
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 27/32] dt: bindings: smiapp: Document lens-focus and flash-leds properties

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:37AM +0300, Sakari Ailus wrote:
> Document optional lens-focus and flash-leds properties for the smiapp
> driver.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  Documentation/devicetree/bindings/media/i2c/nokia,smia.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt 
> b/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
> index 855e1faf73e2..33f10a94c381 100644
> --- a/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
> @@ -27,6 +27,8 @@ Optional properties
>  - nokia,nvm-size: The size of the NVM, in bytes. If the size is not given,
>the NVM contents will not be read.
>  - reset-gpios: XSHUTDOWN GPIO
> +- flash-leds: See ../video-interfaces.txt
> +- lens-focus: See ../video-interfaces.txt
>  
>  
>  Endpoint node mandatory properties
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 26/32] v4l: fwnode: Add a convenience function for registering sensors

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:36AM +0300, Sakari Ailus wrote:
> Add a convenience function for parsing firmware for information on related
> devices using v4l2_async_notifier_parse_fwnode_sensor_common() registering
> the notifier and finally the async sub-device itself.
> 
> This should be useful for sensor drivers that do not have device specific
> requirements related to firmware information parsing or the async
> framework.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/v4l2-core/v4l2-async.c  | 19 
>  drivers/media/v4l2-core/v4l2-fwnode.c | 41 
> +++
>  include/media/v4l2-async.h| 22 +++
>  include/media/v4l2-subdev.h   |  3 +++
>  4 files changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index b4e88eef195f..e81a72b8d46e 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -474,19 +474,25 @@ int v4l2_async_subdev_notifier_register(struct 
> v4l2_subdev *sd,
>  }
>  EXPORT_SYMBOL(v4l2_async_subdev_notifier_register);
>  
> -void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> +static void __v4l2_async_notifier_unregister(
> + struct v4l2_async_notifier *notifier)
>  {
> - if (!notifier->v4l2_dev && !notifier->sd)
> + if (!notifier || (!notifier->v4l2_dev && !notifier->sd))
>   return;
>  
> - mutex_lock(_lock);
> -
>   v4l2_async_notifier_unbind_all_subdevs(notifier);
>  
>   notifier->sd = NULL;
>   notifier->v4l2_dev = NULL;
>  
>   list_del(>list);
> +}
> +
> +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> +{
> + mutex_lock(_lock);
> +
> + __v4l2_async_notifier_unregister(notifier);
>  
>   mutex_unlock(_lock);
>  }
> @@ -596,6 +602,11 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  {
>   mutex_lock(_lock);
>  
> + __v4l2_async_notifier_unregister(sd->subdev_notifier);
> + v4l2_async_notifier_cleanup(sd->subdev_notifier);
> + kfree(sd->subdev_notifier);
> + sd->subdev_notifier = NULL;
> +
>   if (sd->asd) {
>   struct v4l2_async_notifier *notifier = sd->notifier;
>  
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 1234bd1a2f49..82af608fd626 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -29,6 +29,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  enum v4l2_fwnode_bus_type {
>   V4L2_FWNODE_BUS_TYPE_GUESS = 0,
> @@ -900,6 +901,46 @@ int v4l2_async_notifier_parse_fwnode_sensor_common(
>  }
>  EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_sensor_common);
>  
> +int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd)
> +{
> + struct v4l2_async_notifier *notifier;
> + int ret;
> +
> + if (WARN_ON(!sd->dev))
> + return -ENODEV;
> +
> + notifier = kzalloc(sizeof(*notifier), GFP_KERNEL);
> + if (!notifier)
> + return -ENOMEM;
> +
> + ret = v4l2_async_notifier_parse_fwnode_sensor_common(sd->dev,
> +  notifier);
> + if (ret < 0)
> + goto out_cleanup;
> +
> + ret = v4l2_async_subdev_notifier_register(sd, notifier);
> + if (ret < 0)
> + goto out_cleanup;
> +
> + ret = v4l2_async_register_subdev(sd);
> + if (ret < 0)
> + goto out_unregister;
> +
> + sd->subdev_notifier = notifier;
> +
> + return 0;
> +
> +out_unregister:
> + v4l2_async_notifier_unregister(notifier);
> +
> +out_cleanup:
> + v4l2_async_notifier_cleanup(notifier);
> + kfree(notifier);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Sakari Ailus <sakari.ai...@linux.intel.com>");
>  MODULE_AUTHOR("Sylwester Nawrocki <s.nawro...@samsung.com>");
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 8d8cfc3f3100..6152434cbe82 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -174,6 +174,28 @@ void v4l

Re: [PATCH v16.1 25/32] v4l: fwnode: Add convenience function for parsing common external refs

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 06:03:27PM +0300, Sakari Ailus wrote:
> Add v4l2_fwnode_parse_reference_sensor_common for parsing common
> sensor properties that refer to adjacent devices such as flash or lens
> driver chips.
> 
> As this is an association only, there's little a regular driver needs to
> know about these devices as such.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>

Reviewd-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
> since v16:
> 
> - use const char * const *props for string arrays with property names.
> 
>  drivers/media/v4l2-core/v4l2-fwnode.c | 35 
> +++
>  include/media/v4l2-async.h|  3 ++-
>  include/media/v4l2-fwnode.h   | 21 +
>  3 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index f8cd88f791c4..39387dc6cadd 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -865,6 +865,41 @@ static int v4l2_fwnode_reference_parse_int_props(
>   return ret;
>  }
>  
> +int v4l2_async_notifier_parse_fwnode_sensor_common(
> + struct device *dev, struct v4l2_async_notifier *notifier)
> +{
> + static const char * const led_props[] = { "led" };
> + static const struct {
> + const char *name;
> + const char * const *props;
> + unsigned int nprops;
> + } props[] = {
> + { "flash-leds", led_props, ARRAY_SIZE(led_props) },
> + { "lens-focus", NULL, 0 },
> + };
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(props); i++) {
> + int ret;
> +
> + if (props[i].props && is_acpi_node(dev_fwnode(dev)))
> + ret = v4l2_fwnode_reference_parse_int_props(
> + dev, notifier, props[i].name,
> + props[i].props, props[i].nprops);
> + else
> + ret = v4l2_fwnode_reference_parse(
> + dev, notifier, props[i].name);
> + if (ret && ret != -ENOENT) {
> + dev_warn(dev, "parsing property \"%s\" failed (%d)\n",
> +  props[i].name, ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_sensor_common);
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Sakari Ailus <sakari.ai...@linux.intel.com>");
>  MODULE_AUTHOR("Sylwester Nawrocki <s.nawro...@samsung.com>");
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 17c4ac7c73e8..8d8cfc3f3100 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -156,7 +156,8 @@ void v4l2_async_notifier_unregister(struct 
> v4l2_async_notifier *notifier);
>   * Release memory resources related to a notifier, including the async
>   * sub-devices allocated for the purposes of the notifier but not the 
> notifier
>   * itself. The user is responsible for calling this function to clean up the
> - * notifier after calling @v4l2_async_notifier_parse_fwnode_endpoints.
> + * notifier after calling @v4l2_async_notifier_parse_fwnode_endpoints or
> + * @v4l2_fwnode_reference_parse_sensor_common.
>   *
>   * There is no harm from calling v4l2_async_notifier_cleanup in other
>   * cases as long as its memory has been zeroed after it has been
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 105cfeee44ef..ca50108dfd8f 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -319,4 +319,25 @@ int v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> struct v4l2_fwnode_endpoint *vep,
> struct v4l2_async_subdev *asd));
>  
> +/**
> + * v4l2_fwnode_reference_parse_sensor_common - parse common references on
> + *  sensors 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
> + *
> + * Parse common sensor properties for remote devices related to the
> + * sensor and set up async sub-devices for them.
> + *
> + * Any notifier populated using this function must be released with a call to
> +

Re: [PATCH v16.1 24/32] v4l: fwnode: Add a helper function to obtain device / integer references

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 06:01:58PM +0300, 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.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> since v16:
> 
> - use const char * const *props for string arrays with property names.
> 
>  drivers/media/v4l2-core/v4l2-fwnode.c | 287 
> ++
>  1 file changed, 287 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index edd2e8d983a1..f8cd88f791c4 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -578,6 +578,293 @@ static int v4l2_fwnode_reference_parse(
>   return ret;
>  }
>  
> +/*
> + * v4l2_fwnode_reference_get_int_prop - parse a reference with integer
> + *   arguments
> + * @fwnode: fwnode to read @prop from
> + * @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
> + *
> + * First find an fwnode referred to by the reference at @index in @prop.
> + *
> + * Then under that fwnode, @nprops times, for each property in @props,
> + * iteratively follow child nodes starting from fwnode such that they have 
> the
> + * property in @props array at the index of the child node distance from the
> + * root node and the value of that property matching with the integer 
> argument
> + * of the reference, at the same index.
> + *
> + * The child fwnode reched at the end of the iteration is then returned to 
> the
> + * caller.
> + *
> + * The core reason for this is that you cannot refer to just any node in 
> ACPI.
> + * So to refer to an endpoint (easy in DT) you need to refer to a device, 
> then
> + * provide a list of (property name, property value) tuples where each tuple
> + * uniquely identifies a child node. The first tuple identifies a child 
> directly
> + * underneath the device fwnode, the next tuple identifies a child node
> + * underneath the fwnode identified by the previous tuple, etc. until you
> + * reached the fwnode you need.
> + *
> + * An example with a graph, as defined in Documentation/acpi/dsd/graph.txt:
> + *
> + *   Scope (\_SB.PCI0.I2C2)
> + *   {
> + *   Device (CAM0)
> + *   {
> + *   Name (_DSD, Package () {
> + *   ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + *   Package () {
> + *   Package () {
> + *   "compatible",
> + *   Package () { "nokia,smia" }
> + *   },
> + *   },
> + *   ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> + *   Package () {
> + *   Package () { "port0", "PRT0" },
> + *   }
> + *   })
> + *   Name (PRT0, Package() {
> + *   ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + *   Package () {
> + *   Package () { "port", 0 },
> + *   },
> + *   ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> + *   Package () {
> + *   Package () { "endpoint0", "EP00" },
> + *   }
> + *   })
> + *   Name (EP00, Package() {
> + *   ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + *   Package () {
> + *   Package () { "endpoint", 0 },
> + *   Package () {
> + *   "remote-endpoint",
> + *   Package() {
> + *   \_SB.PCI0.ISP, 4, 0
> + *   }
> + * 

Re: [PATCH v16 23/32] v4l: fwnode: Add a helper function for parsing generic references

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:33AM +0300, 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 <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  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 65bdcd59744a..edd2e8d983a1 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -509,6 +509,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);
> +
> + 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 <sakari.ai...@linux.intel.com>");
>  MODULE_AUTHOR("Sylwester Nawrocki <s.nawro...@samsung.com>");
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 22/32] v4l: fwnode: Move KernelDoc documentation to the header

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:32AM +0300, 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 and add an asterisk to v4l2_fwnode_endpoint_free()
> documentation to make it proper KernelDoc documentation.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/v4l2-core/v4l2-fwnode.c | 75 
>  include/media/v4l2-fwnode.h   | 81 
> ++-
>  2 files changed, 80 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index df0695b7bbcc..65bdcd59744a 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -183,25 +183,6 @@ v4l2_fwnode_endpoint_parse_csi1_bus(struct fwnode_handle 
> *fwnode,
>   vep->bus_type = V4L2_MBUS_CSI1;
>  }
>  
> -/**
> - * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
> - * @fwnode: pointer to the endpoint's fwnode handle
> - * @vep: pointer to the V4L2 fwnode data structure
> - *
> - * All properties are optional. If none are found, we don't set any flags. 
> This
> - * means the port has a static configuration and no properties have to be
> - * specified explicitly. If any properties that identify the bus as parallel
> - * are found and slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if
> - * we recognise the bus as serial CSI-2 and clock-noncontinuous isn't set, we
> - * set the V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag. The caller should hold a
> - * reference to @fwnode.
> - *
> - * NOTE: This function does not parse properties the size of which is 
> variable
> - * without a low fixed limit. Please use v4l2_fwnode_endpoint_alloc_parse() 
> in
> - * new drivers instead.
> - *
> - * Return: 0 on success or a negative error code on failure.
> - */
>  int v4l2_fwnode_endpoint_parse(struct fwnode_handle *fwnode,
>  struct v4l2_fwnode_endpoint *vep)
>  {
> @@ -241,14 +222,6 @@ int v4l2_fwnode_endpoint_parse(struct fwnode_handle 
> *fwnode,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_parse);
>  
> -/*
> - * v4l2_fwnode_endpoint_free() - free the V4L2 fwnode acquired by
> - * v4l2_fwnode_endpoint_alloc_parse()
> - * @vep - the V4L2 fwnode the resources of which are to be released
> - *
> - * It is safe to call this function with NULL argument or on a V4L2 fwnode 
> the
> - * parsing of which failed.
> - */
>  void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vep)
>  {
>   if (IS_ERR_OR_NULL(vep))
> @@ -259,29 +232,6 @@ void v4l2_fwnode_endpoint_free(struct 
> v4l2_fwnode_endpoint *vep)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_free);
>  
> -/**
> - * v4l2_fwnode_endpoint_alloc_parse() - parse all fwnode node properties
> - * @fwnode: pointer to the endpoint's fwnode handle
> - *
> - * All properties are optional. If none are found, we don't set any flags. 
> This
> - * means the port has a static configuration and no properties have to be
> - * specified explicitly. If any properties that identify the bus as parallel
> - * are found and slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if
> - * we recognise the bus as serial CSI-2 and clock-noncontinuous isn't set, we
> - * set the V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag. The caller should hold a
> - * reference to @fwnode.
> - *
> - * v4l2_fwnode_endpoint_alloc_parse() has two important differences to
> - * v4l2_fwnode_endpoint_parse():
> - *
> - * 1. It also parses variable size data.
> - *
> - * 2. The memory it has allocated to store the variable size data must be 
> freed
> - *using v4l2_fwnode_endpoint_free() when no longer needed.
> - *
> - * Return: Pointer to v4l2_fwnode_endpoint if successful, on an error pointer
> - * on error.
> - */
>  struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse(
>   struct fwnode_handle *fwnode)
>  {
> @@ -324,24 +274,6 @@ struct v4l2_fwnode_endpoint 
> *v4l2_fwnode_endpoint_alloc_parse(
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_alloc_parse);
>  
> -/**
> - * v4l2_fwnode_endpoint_parse_link() - parse a

Re: [PATCH v16 20/32] dt: bindings: Add a binding for flash LED devices associated to a sensor

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:30AM +0300, 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 <sakari.ai...@linux.intel.com>
> Cc: Rob Herring <r...@kernel.org>
> Cc: devicet...@vger.kernel.org
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>
> Acked-by: Rob Herring <r...@kernel.org>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  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 bd6474920510..dc66e3224692 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -76,6 +76,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.
> +
> +
>  Optional endpoint properties
>  
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16.1 19/32] v4l: async: Ensure only unique fwnodes are registered to notifiers

[Sebastian Reichel]

t; - "Invalid match type %u on %p\n",
> + dev_err(dev, "Invalid match type %u on %p\n",
>   asd->match_type, asd);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out_unlock;
>   }
>   list_add_tail(>list, >waiting);
>   }
>  
> - mutex_lock(_lock);
> -
>   ret = v4l2_async_notifier_try_all_subdevs(notifier);
> - if (ret)
> + if (ret < 0)
>   goto err_unbind;

the error path does no unlock?

>   ret = v4l2_async_notifier_try_complete(notifier);
> - if (ret)
> + if (ret < 0)
>   goto err_unbind;

dito

> + ret = 0;
>  
>   /* Keep also completed notifiers on the list */
>   list_add(>list, _list);
>  
> +out_unlock:
>   mutex_unlock(_lock);
>  
> - return 0;
> + return ret;
>  
>  err_unbind:
>   /*

Otherwise:

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v16.1 18/32] v4l: async: Allow binding notifiers to sub-devices

[Sebastian Reichel]

Hi,

On Fri, Oct 27, 2017 at 11:27:09AM +0300, 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 complete callback of the root notifier will be called only when the
> v4l2_device is available and no notifier has pending sub-devices to bind.
> No complete callbacks are supported for sub-device notifiers.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/v4l2-core/v4l2-async.c | 212 
> ---
>  include/media/v4l2-async.h   |  19 +++-
>  2 files changed, 189 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 6265717769d2..ed539c4fd5dc 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -124,11 +124,87 @@ 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;
> +}
> +
> +/* 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;
> +}
> +
> +/*
> + * Return true if all child sub-device notifiers are complete, false 
> otherwise.
> + */
> +static bool v4l2_async_notifier_can_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_notifier_can_complete(subdev_notifier))
> + return false;
> + }
> +
> + return true;
> +}
> +
> +/*
> + * Complete the master notifier if possible. This is done when all async
> + * sub-devices have been bound; v4l2_device is also available then.
> + */
> +static int v4l2_async_notifier_try_complete(
> + struct v4l2_async_notifier *notifier)
> +{
> + /* Quick check whether there are still more sub-devices here. */
> + if (!list_empty(>waiting))
> + return 0;
> +
> + /* Check the entire notifier tree; find the root notifier first. */
> + while (notifier->parent)
> + notifier = notifier->parent;
> +
> + /* This is root if it has v4l2_dev. */
> + if (!notifier->v4l2_dev)
> + return 0;
> +
> + /* Is everything ready? */
> + if (!v4l2_async_notifier_can_complete(notifier))
> + return 0;
> +
> + return v4l2_async_notifier_call_complete(notifier);
> +}
> +
> +static int v4l2_async_notifier_try_all_subdevs(
> + struct v4l2_async_notifier *notifier);
> +
>  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>  struct v4l2_device *v4l2_dev,
>  struct v4l2_subdev *sd,
>  struct v4l2_async_subdev *asd)
>  {
> + struct v4l2_async_notifier *subdev_notifier;
>   int ret;
>  
>   ret = v4l2_device_register_subdev(v4l2_dev, sd);
> @@ -149,17 +225,36 @@ static int v4l2_async_match_notify(struct 
> v4l2_async_notifier *notifier,
>   /* Move from the global subdevice 

Re: [PATCH v16.1 17/32] v4l: async: Prepare for async sub-device notifiers

[Sebastian Reichel]

Hi,

On Fri, Oct 27, 2017 at 11:26:29AM +0300, Sakari Ailus wrote:
> Refactor the V4L2 async framework a little in preparation for async
> sub-device notifiers. This avoids making some structural changes in the
> patch actually implementing sub-device notifiers, making that patch easier
> to review.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/v4l2-core/v4l2-async.c | 69 
> ++--
>  1 file changed, 50 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 1b536d68cedf..6265717769d2 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -125,12 +125,13 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
>  }
>  
>  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> +struct v4l2_device *v4l2_dev,
>  struct v4l2_subdev *sd,
>  struct v4l2_async_subdev *asd)
>  {
>   int ret;
>  
> - ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> + ret = v4l2_device_register_subdev(v4l2_dev, sd);
>   if (ret < 0)
>   return ret;
>  
> @@ -151,6 +152,29 @@ static int v4l2_async_match_notify(struct 
> v4l2_async_notifier *notifier,
>   return 0;
>  }
>  
> +/* 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_device *v4l2_dev = notifier->v4l2_dev;
> + struct v4l2_subdev *sd, *tmp;
> +
> + list_for_each_entry_safe(sd, tmp, _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, v4l2_dev, sd, asd);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
>  static void v4l2_async_cleanup(struct v4l2_subdev *sd)
>  {
>   v4l2_device_unregister_subdev(sd);
> @@ -172,18 +196,15 @@ static void v4l2_async_notifier_unbind_all_subdevs(
>   }
>  }
>  
> -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> -  struct v4l2_async_notifier *notifier)
> +static int __v4l2_async_notifier_register(struct v4l2_async_notifier 
> *notifier)
>  {
> - struct v4l2_subdev *sd, *tmp;
>   struct v4l2_async_subdev *asd;
>   int ret;
>   int i;
>  
> - if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> + if (notifier->num_subdevs > V4L2_MAX_SUBDEVS)
>   return -EINVAL;
>  
> - notifier->v4l2_dev = v4l2_dev;
>   INIT_LIST_HEAD(>waiting);
>   INIT_LIST_HEAD(>done);
>  
> @@ -216,18 +237,10 @@ int v4l2_async_notifier_register(struct v4l2_device 
> *v4l2_dev,
>  
>   mutex_lock(_lock);
>  
> - list_for_each_entry_safe(sd, tmp, _list, async_list) {
> - int ret;
> -
> - asd = v4l2_async_find_match(notifier, sd);
> - if (!asd)
> - continue;
> -
> - ret = v4l2_async_match_notify(notifier, sd, asd);
> - if (ret < 0) {
> - mutex_unlock(_lock);
> - return ret;
> - }
> + ret = v4l2_async_notifier_try_all_subdevs(notifier);
> + if (ret) {
> + mutex_unlock(_lock);
> + return ret;
>   }
>  
>   if (list_empty(>waiting)) {
> @@ -250,6 +263,23 @@ int v4l2_async_notifier_register(struct v4l2_device 
> *v4l2_dev,
>  
>   return ret;
>  }
> +
> +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> +  struct v4l2_async_notifier *notifier)
> +{
> + int ret;
> +
> + if (WARN_ON(!v4l2_dev))
> + return -EINVAL;
> +
> + notifier->v4l2_dev = v4l2_dev;
> +
> + ret = __v4l2_async_notifier_register(notifier);
> + if (ret)
> + notifier->v4l2_dev = NULL;
> +
> + return ret;
> +}
>  EXPORT_SYMBOL(v4l2_async_notifier_register);
>  
>  void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> @@ -324,7 +354,8 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>   if (!asd)
>   continue;
>  
> - ret = v4l2_async_match_notify(notifier, sd, asd);
> + ret = v4l2_async_match_notify(notifier, notifier->v4l2_dev, sd,
> +   asd);
>   if (ret)
>   goto err_unlock;
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 15/32] v4l: async: Register sub-devices before calling bound callback

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:25AM +0300, 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 <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  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 e170682dae78..46db85685894 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;
>   }
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 16/32] v4l: async: Allow async notifier register call succeed with no subdevs

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:26AM +0300, 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 <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/v4l2-core/v4l2-async.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 46db85685894..1b536d68cedf 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -180,14 +180,22 @@ int v4l2_async_notifier_register(struct v4l2_device 
> *v4l2_dev,
>   int ret;
>   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) {
> + int ret;
> +
> + ret = v4l2_async_notifier_call_complete(notifier);
> + notifier->v4l2_dev = NULL;
> +
> + return ret;
> + }
> +
>   for (i = 0; i < notifier->num_subdevs; i++) {
>   asd = notifier->subdevs[i];
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 12/32] omap3isp: Print the name of the entity where no source pads could be found

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:22AM +0300, Sakari Ailus wrote:
> If no source pads are found in an entity, print the name of the entity.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  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 4afd7ba4fad6..35687c9707e0 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;
>   }
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 14/32] v4l: async: Introduce helpers for calling async ops callbacks

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:24AM +0300, 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 <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/v4l2-core/v4l2-async.c | 56 
> +---
>  1 file changed, 39 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 9d6fc5f25619..e170682dae78 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);
> +}
> +
>  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;
>   }
>  
> @@ -140,8 +165,7 @@ static void v4l2_async_notifier_unbind_all_subdevs(
>   struct v4l2_subdev *sd, *tmp;
>  
>   list_for_each_entry_safe(sd, tmp, >done, async_list) {
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, sd->asd);
> + v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
>   v4l2_async_cleanup(sd);
>  
>   list_move(>async_list, _list);
> @@ -198,8 +222,8 @@ int v4l2_async_notifier_register(struct v4l2_device 
> *v4l2_dev,
>   }
>   }
>  
> - if (list_empty(>waiting) && notifier->ops->complete) {
> - ret = notifier->ops->complete(notifier);
> + if (list_empty(>waiting)) {
> + ret = v4l2_async_notifier_call_complete(notifier);
>   if (ret)
>   goto err_complete;
>   }
> @@ -296,10 +320,10 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>   if (ret)
>   goto err_unlock;
>  
> - if (!list_empty(>waiting) || !notifier->ops->complete)
> + if (!list_empty(>waiting))
>   goto out_unlock;
>  
> - ret = notifier->ops->complete(notifier);
> + ret = v4l2_async_notifier_call_complete(notifier);
>   if (ret)
>   goto err_cleanup;
>  
> @@ -315,8 +339,7 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>   return 0;
>  
>  err_cleanup:
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, sd->asd);
> + v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
>   v4l2_async_cleanup(sd);
>  
>  err_unlock:
> @@ -335,8 +358,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  
>   list_add(>asd->list, >waiting);
>  
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, sd->asd);
> + v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
>   }
>  
>   v4l2_async_cleanup(sd);
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 13/32] v4l: async: Move async subdev notifier operations to a separate structure

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:23AM +0300, Sakari Ailus wrote:
> From: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> 
> The async subdev notifier .bound(), .unbind() and .complete() operations
> are function pointers stored directly in the v4l2_async_subdev
> structure. As the structure isn't immutable, this creates a potential
> security risk as the function pointers are mutable.
> 
> To fix this, move the function pointers to a new
> v4l2_async_subdev_operations structure that can be made const in
> drivers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/platform/am437x/am437x-vpfe.c|  8 +--
>  drivers/media/platform/atmel/atmel-isc.c   | 10 ++---
>  drivers/media/platform/atmel/atmel-isi.c   | 10 ++---
>  drivers/media/platform/davinci/vpif_capture.c  |  8 +--
>  drivers/media/platform/davinci/vpif_display.c  |  8 +--
>  drivers/media/platform/exynos4-is/media-dev.c  |  8 +--
>  drivers/media/platform/omap3isp/isp.c  |  6 +-
>  drivers/media/platform/pxa_camera.c|  8 +--
>  drivers/media/platform/qcom/camss-8x16/camss.c |  8 +--
>  drivers/media/platform/rcar-vin/rcar-core.c| 10 ++---
>  drivers/media/platform/rcar_drif.c | 10 ++---
>  drivers/media/platform/soc_camera/soc_camera.c | 14 ++--
>  drivers/media/platform/stm32/stm32-dcmi.c  | 10 ++---
>  drivers/media/platform/ti-vpe/cal.c|  8 +--
>  drivers/media/platform/xilinx/xilinx-vipp.c|  8 +--
>  drivers/media/v4l2-core/v4l2-async.c   | 30 
> --
>  drivers/staging/media/imx/imx-media-dev.c  |  8 +--
>  include/media/v4l2-async.h | 29 -
>  18 files changed, 135 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c 
> b/drivers/media/platform/am437x/am437x-vpfe.c
> index dfcc484cab89..0997c640191d 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -2417,6 +2417,11 @@ static int vpfe_async_complete(struct 
> v4l2_async_notifier *notifier)
>   return vpfe_probe_complete(vpfe);
>  }
>  
> +static const struct v4l2_async_notifier_operations vpfe_async_ops = {
> + .bound = vpfe_async_bound,
> + .complete = vpfe_async_complete,
> +};
> +
>  static struct vpfe_config *
>  vpfe_get_pdata(struct platform_device *pdev)
>  {
> @@ -2590,8 +2595,7 @@ static int vpfe_probe(struct platform_device *pdev)
>  
>   vpfe->notifier.subdevs = vpfe->cfg->asd;
>   vpfe->notifier.num_subdevs = ARRAY_SIZE(vpfe->cfg->asd);
> - vpfe->notifier.bound = vpfe_async_bound;
> - vpfe->notifier.complete = vpfe_async_complete;
> + vpfe->notifier.ops = _async_ops;
>   ret = v4l2_async_notifier_register(>v4l2_dev,
>   >notifier);
>   if (ret) {
> diff --git a/drivers/media/platform/atmel/atmel-isc.c 
> b/drivers/media/platform/atmel/atmel-isc.c
> index 2f8e345d297e..382fe355e616 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -1637,6 +1637,12 @@ static int isc_async_complete(struct 
> v4l2_async_notifier *notifier)
>   return 0;
>  }
>  
> +static const struct v4l2_async_notifier_operations isc_async_ops = {
> + .bound = isc_async_bound,
> + .unbind = isc_async_unbind,
> + .complete = isc_async_complete,
> +};
> +
>  static void isc_subdev_cleanup(struct isc_device *isc)
>  {
>   struct isc_subdev_entity *subdev_entity;
> @@ -1849,9 +1855,7 @@ static int atmel_isc_probe(struct platform_device *pdev)
>   list_for_each_entry(subdev_entity, >subdev_entities, list) {
>   subdev_entity->notifier.subdevs = _entity->asd;
>   subdev_entity->notifier.num_subdevs = 1;
> - subdev_entity->notifier.bound = isc_async_bound;
> - subdev_entity->notifier.unbind = isc_async_unbind;
> - subdev_entity->notifier.complete = isc_async_complete;
> + subdev_entity->notifier.ops = _async_ops;
>  
>   ret = v4l2_async_notifier_register(>v4l2_dev,
>  _entity->notifier);
> diff --git a/drivers/media/platform/atmel/atmel-isi.c 
> b/drivers/media/platform/atmel/atmel-isi.c
> index 463c0146915e..e900995143a3 100644
> --- a/drivers/media/platf

Re: [PATCH v16 11/32] omap3isp: Fix check for our own sub-devices

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:21AM +0300, 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 <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  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 97a5206b6ddc..4afd7ba4fad6 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,
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v16 09/32] omap3isp: Use generic parser for parsing fwnode endpoints

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:19AM +0300, Sakari Ailus wrote:
> Instead of using a custom driver implementation, use
> v4l2_async_notifier_parse_fwnode_endpoints() to parse the fwnode endpoints
> of the device.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/platform/omap3isp/isp.c | 121 
> +++---
>  drivers/media/platform/omap3isp/isp.h |   5 +-
>  2 files changed, 40 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c 
> b/drivers/media/platform/omap3isp/isp.c
> index 1a428fe9f070..97a5206b6ddc 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2001,6 +2001,7 @@ static int isp_remove(struct platform_device *pdev)
>   __omap3isp_put(isp, false);
>  
>   media_entity_enum_cleanup(>crashed);
> + v4l2_async_notifier_cleanup(>notifier);
>  
>   return 0;
>  }
> @@ -2011,44 +2012,41 @@ enum isp_of_phy {
>   ISP_OF_PHY_CSIPHY2,
>  };
>  
> -static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
> - struct isp_async_subdev *isd)
> +static int isp_fwnode_parse(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd)
>  {
> + struct isp_async_subdev *isd =
> + container_of(asd, struct isp_async_subdev, asd);
>   struct isp_bus_cfg *buscfg = >bus;
> - struct v4l2_fwnode_endpoint vep;
> - unsigned int i;
> - int ret;
>   bool csi1 = false;
> -
> - ret = v4l2_fwnode_endpoint_parse(fwnode, );
> - if (ret)
> - return ret;
> + unsigned int i;
>  
>   dev_dbg(dev, "parsing endpoint %pOF, interface %u\n",
> - to_of_node(fwnode), vep.base.port);
> + to_of_node(vep->base.local_fwnode), vep->base.port);
>  
> - switch (vep.base.port) {
> + switch (vep->base.port) {
>   case ISP_OF_PHY_PARALLEL:
>   buscfg->interface = ISP_INTERFACE_PARALLEL;
>   buscfg->bus.parallel.data_lane_shift =
> - vep.bus.parallel.data_shift;
> + vep->bus.parallel.data_shift;
>   buscfg->bus.parallel.clk_pol =
> - !!(vep.bus.parallel.flags
> + !!(vep->bus.parallel.flags
>  & V4L2_MBUS_PCLK_SAMPLE_FALLING);
>   buscfg->bus.parallel.hs_pol =
> - !!(vep.bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
> + !!(vep->bus.parallel.flags & 
> V4L2_MBUS_VSYNC_ACTIVE_LOW);
>   buscfg->bus.parallel.vs_pol =
> - !!(vep.bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
> + !!(vep->bus.parallel.flags & 
> V4L2_MBUS_HSYNC_ACTIVE_LOW);
>   buscfg->bus.parallel.fld_pol =
> - !!(vep.bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
> + !!(vep->bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
>   buscfg->bus.parallel.data_pol =
> - !!(vep.bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
> - buscfg->bus.parallel.bt656 = vep.bus_type == V4L2_MBUS_BT656;
> + !!(vep->bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
> + buscfg->bus.parallel.bt656 = vep->bus_type == V4L2_MBUS_BT656;
>   break;
>  
>   case ISP_OF_PHY_CSIPHY1:
>   case ISP_OF_PHY_CSIPHY2:
> - switch (vep.bus_type) {
> + switch (vep->bus_type) {
>   case V4L2_MBUS_CCP2:
>   case V4L2_MBUS_CSI1:
>   dev_dbg(dev, "CSI-1/CCP-2 configuration\n");
> @@ -2060,11 +2058,11 @@ static int isp_fwnode_parse(struct device *dev, 
> struct fwnode_handle *fwnode,
>   break;
>   default:
>   dev_err(dev, "unsupported bus type %u\n",
> - vep.bus_type);
> + vep->bus_type);
>   return -EINVAL;
>   }
>  
> - switch (vep.base.port) {
> + switch (vep->base.port) {
>   case ISP_OF_PHY_CSIPHY1:
>   if (csi

Re: [PATCH v16 04/32] v4l: async: Fix notifier complete callback error handling

[Sebastian Reichel]

Hi,

On Thu, Oct 26, 2017 at 10:53:14AM +0300, Sakari Ailus wrote:
> The notifier complete callback may return an error. This error code was
> simply returned to the caller but never handled properly.
> 
> Move calling the complete callback function to the caller from
> v4l2_async_test_notify and undo the work that was done either in async
> sub-device or async notifier registration.
> 
> Reported-by: Russell King <rmk+ker...@armlinux.org.uk>
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/v4l2-core/v4l2-async.c | 78 
> +++-
>  1 file changed, 60 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index ca281438a0ae..4924481451ca 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -122,9 +122,6 @@ static int v4l2_async_test_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->complete)
> - return notifier->complete(notifier);
> -
>   return 0;
>  }
>  
> @@ -136,11 +133,27 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd)
>   sd->asd = NULL;
>  }
>  
> +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) {
> + if (notifier->unbind)
> + notifier->unbind(notifier, sd, sd->asd);
> +
> + v4l2_async_cleanup(sd);
> +
> + list_move(>async_list, _list);
> + }
> +}
> +
>  int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>struct v4l2_async_notifier *notifier)
>  {
>   struct v4l2_subdev *sd, *tmp;
>   struct v4l2_async_subdev *asd;
> + int ret;
>   int i;
>  
>   if (!v4l2_dev || !notifier->num_subdevs ||
> @@ -185,19 +198,30 @@ int v4l2_async_notifier_register(struct v4l2_device 
> *v4l2_dev,
>   }
>   }
>  
> + if (list_empty(>waiting) && notifier->complete) {
> + ret = notifier->complete(notifier);
> + if (ret)
> + goto err_complete;
> + }
> +
>   /* Keep also completed notifiers on the list */
>   list_add(>list, _list);
>  
>   mutex_unlock(_lock);
>  
>   return 0;
> +
> +err_complete:
> + v4l2_async_notifier_unbind_all_subdevs(notifier);
> +
> + mutex_unlock(_lock);
> +
> + return ret;
>  }
>  EXPORT_SYMBOL(v4l2_async_notifier_register);
>  
>  void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  {
> - struct v4l2_subdev *sd, *tmp;
> -
>   if (!notifier->v4l2_dev)
>   return;
>  
> @@ -205,14 +229,7 @@ void v4l2_async_notifier_unregister(struct 
> v4l2_async_notifier *notifier)
>  
>   list_del(>list);
>  
> - list_for_each_entry_safe(sd, tmp, >done, async_list) {
> - if (notifier->unbind)
> - notifier->unbind(notifier, sd, sd->asd);
> -
> - v4l2_async_cleanup(sd);
> -
> - list_move(>async_list, _list);
> - }
> + v4l2_async_notifier_unbind_all_subdevs(notifier);
>  
>   mutex_unlock(_lock);
>  
> @@ -223,6 +240,7 @@ EXPORT_SYMBOL(v4l2_async_notifier_unregister);
>  int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  {
>   struct v4l2_async_notifier *notifier;
> + int ret;
>  
>   /*
>* No reference taken. The reference is held by the device
> @@ -238,19 +256,43 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  
>   list_for_each_entry(notifier, _list, list) {
>   struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, 
> sd);
> - if (asd) {
> - int ret = v4l2_async_test_notify(notifier, sd, asd);
> - mutex_unlock(_lock);
> - return ret;
> - }
> + int ret;
> +
> + if (!asd)
> + continue;
> +
> + ret = v4l2_async_test_notify(notifier, sd, asd);
> + if (ret)
> + goto err_unlock;
> +
> + if (!list_empty(>waiting) || !notifier->

Re: [PATCH v15 07/32] v4l: async: Add V4L2 async documentation to the documentation build

[Sebastian Reichel]

Hi,

On Thu, Oct 05, 2017 at 12:50:26AM +0300, Sakari Ailus wrote:
> The V4L2 async wasn't part of the documentation build. Fix this.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  Documentation/media/kapi/v4l2-async.rst | 3 +++
>  Documentation/media/kapi/v4l2-core.rst  | 1 +
>  2 files changed, 4 insertions(+)
>  create mode 100644 Documentation/media/kapi/v4l2-async.rst
> 
> diff --git a/Documentation/media/kapi/v4l2-async.rst 
> b/Documentation/media/kapi/v4l2-async.rst
> new file mode 100644
> index ..523ff9eb09a0
> --- /dev/null
> +++ b/Documentation/media/kapi/v4l2-async.rst
> @@ -0,0 +1,3 @@
> +V4L2 async kAPI
> +^^^
> +.. kernel-doc:: include/media/v4l2-async.h
> diff --git a/Documentation/media/kapi/v4l2-core.rst 
> b/Documentation/media/kapi/v4l2-core.rst
> index c7434f38fd9c..5cf292037a48 100644
> --- a/Documentation/media/kapi/v4l2-core.rst
> +++ b/Documentation/media/kapi/v4l2-core.rst
> @@ -19,6 +19,7 @@ Video4Linux devices
>  v4l2-mc
>  v4l2-mediabus
>  v4l2-mem2mem
> +v4l2-async
>  v4l2-fwnode
>  v4l2-rect
>  v4l2-tuner
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v15 06/32] v4l: async: Use more intuitive names for internal functions

[Sebastian Reichel]

Hi,

On Thu, Oct 05, 2017 at 12:50:25AM +0300, Sakari Ailus wrote:
> Rename internal functions to make the names of the functions better
> describe what they do.
> 
>   Old nameNew name
>   v4l2_async_test_notify  v4l2_async_match_notify
>   v4l2_async_belongs  v4l2_async_find_match
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 19 ++-
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index cde2cf2ab4b0..8b84fea50c2a 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -60,8 +60,8 @@ static LIST_HEAD(subdev_list);
>  static LIST_HEAD(notifier_list);
>  static DEFINE_MUTEX(list_lock);
>  
> -static struct v4l2_async_subdev *v4l2_async_belongs(struct 
> v4l2_async_notifier *notifier,
> - struct v4l2_subdev *sd)
> +static struct v4l2_async_subdev *v4l2_async_find_match(
> + struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
>  {
>   bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
>   struct v4l2_async_subdev *asd;
> @@ -95,9 +95,9 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct 
> v4l2_async_notifier *
>   return NULL;
>  }
>  
> -static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> -   struct v4l2_subdev *sd,
> -   struct v4l2_async_subdev *asd)
> +static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> +struct v4l2_subdev *sd,
> +struct v4l2_async_subdev *asd)
>  {
>   int ret;
>  
> @@ -187,11 +187,11 @@ int v4l2_async_notifier_register(struct v4l2_device 
> *v4l2_dev,
>   list_for_each_entry_safe(sd, tmp, _list, async_list) {
>   int ret;
>  
> - asd = v4l2_async_belongs(notifier, sd);
> + asd = v4l2_async_find_match(notifier, sd);
>   if (!asd)
>   continue;
>  
> - ret = v4l2_async_test_notify(notifier, sd, asd);
> + ret = v4l2_async_match_notify(notifier, sd, asd);
>   if (ret < 0) {
>   mutex_unlock(_lock);
>   return ret;
> @@ -255,13 +255,14 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>   INIT_LIST_HEAD(>async_list);
>  
>   list_for_each_entry(notifier, _list, list) {
> - struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, 
> sd);
> + struct v4l2_async_subdev *asd = v4l2_async_find_match(notifier,
> +   sd);
>   int ret;
>  
>   if (!asd)
>   continue;
>  
> - ret = v4l2_async_test_notify(notifier, sd, asd);
> + ret = v4l2_async_match_notify(notifier, sd, asd);
>   if (ret)
>   goto err_unlock;
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v15 05/32] v4l: async: Correctly serialise async sub-device unregistration

[Sebastian Reichel]

Hi,

On Thu, Oct 05, 2017 at 12:50:24AM +0300, Sakari Ailus wrote:
> The check whether an async sub-device is bound to a notifier was performed
> without list_lock held, making it possible for another process to
> unbind the async sub-device before the sub-device unregistration function
> proceeds to take the lock.
> 
> Fix this by first acquiring the lock and then proceeding with the check.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 18 +++---
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 4924481451ca..cde2cf2ab4b0 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -298,20 +298,16 @@ EXPORT_SYMBOL(v4l2_async_register_subdev);
>  
>  void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  {
> - struct v4l2_async_notifier *notifier = sd->notifier;
> -
> - if (!sd->asd) {
> - if (!list_empty(>async_list))
> - v4l2_async_cleanup(sd);
> - return;
> - }
> -
>   mutex_lock(_lock);
>  
> - list_add(>asd->list, >waiting);
> + if (sd->asd) {
> + struct v4l2_async_notifier *notifier = sd->notifier;
>  
> - if (notifier->unbind)
> - notifier->unbind(notifier, sd, sd->asd);
> + list_add(>asd->list, >waiting);
> +
> + if (notifier->unbind)
> + notifier->unbind(notifier, sd, sd->asd);
> + }
>  
>   v4l2_async_cleanup(sd);
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v15 03/32] v4l: async: fix unbind error in v4l2_async_notifier_unregister()

[Sebastian Reichel]

Hi,

On Thu, Oct 05, 2017 at 12:50:22AM +0300, Sakari Ailus wrote:
> From: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> 
> The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it to
> notifier->unbind() have no effect and leaves the notifier confused. Call
> the unbind() callback prior to cleaning up the subdevice to avoid this.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 21c748bf3a7b..ca281438a0ae 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -206,11 +206,11 @@ void v4l2_async_notifier_unregister(struct 
> v4l2_async_notifier *notifier)
>   list_del(>list);
>  
>   list_for_each_entry_safe(sd, tmp, >done, async_list) {
> - v4l2_async_cleanup(sd);
> -
>   if (notifier->unbind)
>   notifier->unbind(notifier, sd, sd->asd);
>  
> + v4l2_async_cleanup(sd);
> +
>   list_move(>async_list, _list);
>   }
>  
> @@ -268,11 +268,11 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev 
> *sd)
>  
>   list_add(>asd->list, >waiting);
>  
> - v4l2_async_cleanup(sd);
> -
>   if (notifier->unbind)
>   notifier->unbind(notifier, sd, sd->asd);
>  
> + v4l2_async_cleanup(sd);
> +
>   mutex_unlock(_lock);
>  }
>  EXPORT_SYMBOL(v4l2_async_unregister_subdev);
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v15 02/32] v4l: async: Don't set sd->dev NULL in v4l2_async_cleanup

[Sebastian Reichel]

Hi,

On Thu, Oct 05, 2017 at 12:50:21AM +0300, Sakari Ailus wrote:
> v4l2_async_cleanup() is called when the async sub-device is unbound from
> the media device. As the pointer is set by the driver registering the
> async sub-device, leave the pointer as set by the driver.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/v4l2-core/v4l2-async.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 60a1a50b9537..21c748bf3a7b 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -134,7 +134,6 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd)
>   /* Subdevice driver will reprobe and put the subdev back onto the list 
> */
>   list_del_init(>async_list);
>   sd->asd = NULL;
> - sd->dev = NULL;
>  }
>  
>  int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v15 01/32] v4l: async: Remove re-probing support

[Sebastian Reichel]

Hi,

On Thu, Oct 05, 2017 at 12:50:20AM +0300, 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 <sakari.ai...@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

>  drivers/media/v4l2-core/v4l2-async.c | 54 
> +---
>  1 file changed, 1 insertion(+), 53 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index d741a8e0fdac..60a1a50b9537 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -198,78 +198,26 @@ 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);
> + list_move(>async_list, _list);
>   }
>  
>   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);
>  
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH] media: omap3isp: fix uninitialized variable use

[Sebastian Reichel]

Hi,

On Wed, Aug 23, 2017 at 04:58:27PM +0300, Sakari Ailus wrote:
> On Wed, Aug 23, 2017 at 03:30:19PM +0200, Arnd Bergmann wrote:
> > A debug printk statement was copied incorrectly into the new
> > csi1 parser code and causes a warning there:
> > 
> > drivers/media/platform/omap3isp/isp.c: In function 'isp_probe':
> > include/linux/dynamic_debug.h:134:3: error: 'i' may be used uninitialized 
> > in this function [-Werror=maybe-uninitialized]
> > 
> > Since there is only one lane, the index is never set. This
> > changes the debug print to always print a zero instead,
> > keeping the original format of the message.
> > 
> > Fixes: 9211434bad30 ("media: omap3isp: Parse CSI1 configuration from the 
> > device tree")
> > Signed-off-by: Arnd Bergmann <a...@arndb.de>
> > ---
> >  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 83aea08b832d..30c825bf80d9 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2092,7 +2092,7 @@ static int isp_fwnode_parse(struct device *dev, 
> > struct fwnode_handle *fwnode,
> > buscfg->bus.ccp2.lanecfg.data[0].pol =
> > vep.bus.mipi_csi1.lane_polarity[1];
> >  
> > -   dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> > +   dev_dbg(dev, "data lane 0 polarity %u, pos %u\n",
> > buscfg->bus.ccp2.lanecfg.data[0].pol,
> > buscfg->bus.ccp2.lanecfg.data[0].pos);
> >  
> 
> Thanks! I removed "0 "; CCP2 always has a single lane. The patch is now:
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c 
> b/drivers/media/platform/omap3isp/isp.c
> index 83aea08b832d..1a428fe9f070 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2092,7 +2092,7 @@ static int isp_fwnode_parse(struct device *dev, struct 
> fwnode_handle *fwnode,
>   buscfg->bus.ccp2.lanecfg.data[0].pol =
>   vep.bus.mipi_csi1.lane_polarity[1];
>  
> -         dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> + dev_dbg(dev, "data lane polarity %u, pos %u\n",
>   buscfg->bus.ccp2.lanecfg.data[0].pol,
>   buscfg->bus.ccp2.lanecfg.data[0].pos);

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH 2/7] omap3isp: Parse CSI1 configuration from the device tree

[Sebastian Reichel]

Hi,

On Tue, Jul 18, 2017 at 01:01:11AM +0300, Sakari Ailus wrote:
> From: Pavel Machek <pa...@ucw.cz>
> 
> Add support for parsing CSI1 configuration.
> 
> Signed-off-by: Pavel Machek <pa...@ucw.cz>
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  drivers/media/platform/omap3isp/isp.c  | 106 
> +
>  drivers/media/platform/omap3isp/omap3isp.h |   1 +
>  2 files changed, 80 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c 
> b/drivers/media/platform/omap3isp/isp.c
> index 441eba1e02eb..80ed5a5f862a 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2017,6 +2017,7 @@ static int isp_fwnode_parse(struct device *dev, struct 
> fwnode_handle *fwnode,
>   struct v4l2_fwnode_endpoint vep;
>   unsigned int i;
>   int ret;
> + bool csi1 = false;
>  
>   ret = v4l2_fwnode_endpoint_parse(fwnode, );
>   if (ret)
> @@ -2045,41 +2046,92 @@ static int isp_fwnode_parse(struct device *dev, 
> struct fwnode_handle *fwnode,
>  
>   case ISP_OF_PHY_CSIPHY1:
>   case ISP_OF_PHY_CSIPHY2:
> - /* FIXME: always assume CSI-2 for now. */
> + switch (vep.bus_type) {
> + case V4L2_MBUS_CCP2:
> + case V4L2_MBUS_CSI1:
> + dev_dbg(dev, "csi1/ccp2 configuration\n");
> + csi1 = true;
> + break;
> + case V4L2_MBUS_CSI2:
> + dev_dbg(dev, "csi2 configuration\n");
> + csi1 = false;
> + break;
> + default:
> + dev_err(dev, "unsupported bus type %u\n",
> + vep.bus_type);
> + return -EINVAL;
> + }
> +
>   switch (vep.base.port) {
>   case ISP_OF_PHY_CSIPHY1:
> - buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> + if (csi1)
> + buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
> + else
> + buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
>   break;
>   case ISP_OF_PHY_CSIPHY2:
> - buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> + if (csi1)
> + buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
> + else
> + buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
>   break;
>   }
> - buscfg->bus.csi2.lanecfg.clk.pos = vep.bus.mipi_csi2.clock_lane;
> - buscfg->bus.csi2.lanecfg.clk.pol =
> - vep.bus.mipi_csi2.lane_polarities[0];
> - dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> - buscfg->bus.csi2.lanecfg.clk.pol,
> - buscfg->bus.csi2.lanecfg.clk.pos);
> -
> - buscfg->bus.csi2.num_data_lanes =
> - vep.bus.mipi_csi2.num_data_lanes;
> -
> - for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {
> - buscfg->bus.csi2.lanecfg.data[i].pos =
> - vep.bus.mipi_csi2.data_lanes[i];
> - buscfg->bus.csi2.lanecfg.data[i].pol =
> - vep.bus.mipi_csi2.lane_polarities[i + 1];
> + if (csi1) {
> + buscfg->bus.ccp2.lanecfg.clk.pos =
> + vep.bus.mipi_csi1.clock_lane;
> + buscfg->bus.ccp2.lanecfg.clk.pol =
> + vep.bus.mipi_csi1.lane_polarity[0];
> + dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> + buscfg->bus.ccp2.lanecfg.clk.pol,
> + buscfg->bus.ccp2.lanecfg.clk.pos);
> +
> + buscfg->bus.ccp2.lanecfg.data[0].pos =
> + vep.bus.mipi_csi1.data_lane;
> + buscfg->bus.ccp2.lanecfg.data[0].pol =
> + vep.bus.mipi_csi1.lane_polarity[1];
> +
>   dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> - buscfg->bus.csi2.lanecfg.data[i].pol,
> - buscfg->bus.csi2.lanecfg.data[i].pos);
> + buscfg->bus.ccp2.lane

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Sebastian Reichel]

Hi,

On Tue, Jul 18, 2017 at 01:01:13AM +0300, Sakari Ailus wrote:
> From: Pavel Machek 
> 
> If regulator returns -EPROBE_DEFER, we need to return it too, so that
> omap3isp will be re-probed when regulator is ready.
> 
> Signed-off-by: Pavel Machek 
> Reviewed-by: Laurent Pinchart 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/platform/omap3isp/isp.c | 3 ++-
>  drivers/media/platform/omap3isp/ispccp2.c | 5 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c 
> b/drivers/media/platform/omap3isp/isp.c
> index 80ed5a5f862a..4e6ba7f90e35 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1880,7 +1880,8 @@ static int isp_initialize_modules(struct isp_device 
> *isp)
>  
>   ret = omap3isp_ccp2_init(isp);
>   if (ret < 0) {
> - dev_err(isp->dev, "CCP2 initialization failed\n");
> + if (ret != -EPROBE_DEFER)
> + dev_err(isp->dev, "CCP2 initialization failed\n");
>   goto error_ccp2;
>   }
>  
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c 
> b/drivers/media/platform/omap3isp/ispccp2.c
> index 4f8fd0c00748..47210b102bcb 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>   if (isp->revision == ISP_REVISION_2_0) {
>   ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
>   if (IS_ERR(ccp2->vdds_csib)) {
> + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> + dev_dbg(isp->dev,
> + "Can't get regulator vdds_csib, 
> deferring probing\n");
> + return -EPROBE_DEFER;
> + }

I wonder if the right approach wouldn't be to always bail out for
errors. devm_regulator_get should provide a dummy regulator if
none is specified. If we get an error here it means something is
configured incorrectly or we have serious problems.

>   dev_dbg(isp->dev,
>   "Could not get regulator vdds_csib\n");
>   ccp2->vdds_csib = NULL;

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH 1/7] omap3isp: Ignore endpoints with invalid configuration

[Sebastian Reichel]

Hi,

On Tue, Jul 18, 2017 at 01:01:10AM +0300, Sakari Ailus wrote:
> If endpoint has an invalid configuration, ignore it instead of happily
> proceeding to use it nonetheless. Ignoring such an endpoint is better than
> failing since there could be multiple endpoints, only some of which are
> bad.

I would expect a dev_warn(dev, "Ignore endpoint (broken configuration)!");

-- Sebastian

> Signed-off-by: Sakari Ailus 
> Tested-by: Pavel Machek 
> ---
>  drivers/media/platform/omap3isp/isp.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c 
> b/drivers/media/platform/omap3isp/isp.c
> index db2cccb57ceb..441eba1e02eb 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2110,10 +2110,12 @@ static int isp_fwnodes_parse(struct device *dev,
>   if (!isd)
>   goto error;
>  
> - notifier->subdevs[notifier->num_subdevs] = >asd;
> + if (isp_fwnode_parse(dev, fwnode, isd)) {
> + devm_kfree(dev, isd);
> + continue;
> + }
>  
> - if (isp_fwnode_parse(dev, fwnode, isd))
> - goto error;
> + notifier->subdevs[notifier->num_subdevs] = >asd;
>  
>   isd->asd.match.fwnode.fwnode =
>   fwnode_graph_get_remote_port_parent(fwnode);
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH 7/8] omap3isp: Check for valid port in endpoints

[Sebastian Reichel]

Hi,

On Thu, Jul 06, 2017 at 02:00:18AM +0300, Sakari Ailus wrote:
> Check that we do have a valid port in an endpoint, return an error if not.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  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 2d45bf471c82..0676be725d7c 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2081,7 +2081,7 @@ static int isp_fwnode_parse(struct device *dev, struct 
> fwnode_handle *fwnode,
>   default:
>   dev_warn(dev, "%s: invalid interface %u\n",
>to_of_node(fwnode)->full_name, vep.base.port);
> - break;
> + return -EINVAL;
>   }
>  
>   return 0;
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH 5/8] v4l: Add support for CSI-1 and CCP2 busses

[Sebastian Reichel]

Hi,

On Thu, Jul 06, 2017 at 02:00:16AM +0300, Sakari Ailus wrote:
> From: Sakari Ailus <sakari.ai...@iki.fi>
> 
> CCP2 and CSI-1, are older single data lane serial busses.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Signed-off-by: Pavel Machek <pa...@ucw.cz>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  drivers/media/platform/pxa_camera.c  |  3 ++
>  drivers/media/platform/soc_camera/soc_mediabus.c |  3 ++
>  drivers/media/v4l2-core/v4l2-fwnode.c| 58 
> +++-
>  include/media/v4l2-fwnode.h  | 19 
>  include/media/v4l2-mediabus.h|  4 ++
>  5 files changed, 76 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/pxa_camera.c 
> b/drivers/media/platform/pxa_camera.c
> index 399095170b6e..17e797c9559f 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -638,6 +638,9 @@ static unsigned int pxa_mbus_config_compatible(const 
> struct v4l2_mbus_config *cf
>   mipi_clock = common_flags & (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK 
> |
>V4L2_MBUS_CSI2_CONTINUOUS_CLOCK);
>   return (!mipi_lanes || !mipi_clock) ? 0 : common_flags;
> + default:
> + __WARN();
> + return -EINVAL;
>   }
>   return 0;
>  }
> diff --git a/drivers/media/platform/soc_camera/soc_mediabus.c 
> b/drivers/media/platform/soc_camera/soc_mediabus.c
> index 57581f626f4c..43192d80beef 100644
> --- a/drivers/media/platform/soc_camera/soc_mediabus.c
> +++ b/drivers/media/platform/soc_camera/soc_mediabus.c
> @@ -508,6 +508,9 @@ unsigned int soc_mbus_config_compatible(const struct 
> v4l2_mbus_config *cfg,
>   mipi_clock = common_flags & (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK 
> |
>V4L2_MBUS_CSI2_CONTINUOUS_CLOCK);
>   return (!mipi_lanes || !mipi_clock) ? 0 : common_flags;
> + default:
> + __WARN();
> + return -EINVAL;
>   }
>   return 0;
>  }
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index d71dd3913cd9..76a88f210cb6 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -154,6 +154,31 @@ static void v4l2_fwnode_endpoint_parse_parallel_bus(
>  
>  }
>  
> +void v4l2_fwnode_endpoint_parse_csi1_bus(struct fwnode_handle *fwnode,
> +  struct v4l2_fwnode_endpoint *vep,
> +  u32 bus_type)
> +{
> +   struct v4l2_fwnode_bus_mipi_csi1 *bus = >bus.mipi_csi1;
> +   u32 v;
> +
> +   if (!fwnode_property_read_u32(fwnode, "clock-inv", ))
> +   bus->clock_inv = v;
> +
> +   if (!fwnode_property_read_u32(fwnode, "strobe", ))
> +   bus->strobe = v;
> +
> +   if (!fwnode_property_read_u32(fwnode, "data-lanes", ))
> +   bus->data_lane = v;
> +
> +   if (!fwnode_property_read_u32(fwnode, "clock-lanes", ))
> +   bus->clock_lane = v;
> +
> +   if (bus_type == V4L2_FWNODE_BUS_TYPE_CCP2)
> +vep->bus_type = V4L2_MBUS_CCP2;
> +   else
> +vep->bus_type = V4L2_MBUS_CSI1;
> +}
> +
>  /**
>   * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
>   * @fwnode: pointer to the endpoint's fwnode handle
> @@ -187,17 +212,28 @@ int v4l2_fwnode_endpoint_parse(struct fwnode_handle 
> *fwnode,
>  
>   fwnode_property_read_u32(fwnode, "bus-type", _type);
>  
> - rval = v4l2_fwnode_endpoint_parse_csi2_bus(fwnode, vep);
> - if (rval)
> - return rval;
> - /*
> -  * Parse the parallel video bus properties only if none
> -  * of the MIPI CSI-2 specific properties were found.
> -  */
> - if (vep->bus.mipi_csi2.flags == 0)
> - v4l2_fwnode_endpoint_parse_parallel_bus(fwnode, vep);
> -
> - return 0;
> + switch (bus_type) {
> + case V4L2_FWNODE_BUS_TYPE_GUESS:
> + rval = v4l2_fwnode_endpoint_parse_csi2_bus(fwnode, vep);
> + if (rval)
> + return rval;
> + /*
> +  * Parse the parallel video bus properties only if none
> +  * of the MIPI CSI-2 specific properties were found.
> +  */
> + if (vep->bus.mipi_csi2.flags == 0)
> + v4l2_fwnode_endpoint_parse_parallel_bus(fwnode, 

Re: [PATCH 3/8] v4l: fwnode: Call CSI2 bus csi2, not csi

[Sebastian Reichel]

Hi,

On Thu, Jul 06, 2017 at 02:00:14AM +0300, Sakari Ailus wrote:
> The function to parse CSI2 bus parameters was called
> v4l2_fwnode_endpoint_parse_csi_bus(), rename it as
> v4l2_fwnode_endpoint_parse_csi2_bus() in anticipation of CSI1/CCP2
> support.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 153c53ca3925..8df26010d006 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -28,8 +28,8 @@
>  
>  #include 
>  
> -static int v4l2_fwnode_endpoint_parse_csi_bus(struct fwnode_handle *fwnode,
> -   struct v4l2_fwnode_endpoint *vep)
> +static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> +struct v4l2_fwnode_endpoint *vep)
>  {
>   struct v4l2_fwnode_bus_mipi_csi2 *bus = >bus.mipi_csi2;
>   bool have_clk_lane = false;
> @@ -176,7 +176,7 @@ int v4l2_fwnode_endpoint_parse(struct fwnode_handle 
> *fwnode,
>   memset(>bus_type, 0, sizeof(*vep) -
>  offsetof(typeof(*vep), bus_type));
>  
> - rval = v4l2_fwnode_endpoint_parse_csi_bus(fwnode, vep);
> + rval = v4l2_fwnode_endpoint_parse_csi2_bus(fwnode, vep);
>   if (rval)
>   return rval;
>   /*
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH 4/8] v4l: fwnode: Obtain data bus type from FW

[Sebastian Reichel]

Hi,

On Thu, Jul 06, 2017 at 02:00:15AM +0300, Sakari Ailus wrote:
> From: Sakari Ailus <sakari.ai...@iki.fi>
> 
> Just obtain it. It'll actually get used soon with CSI-1/CCP2.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 8df26010d006..d71dd3913cd9 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -28,6 +28,14 @@
>  
>  #include 
>  
> +enum v4l2_fwnode_bus_type {
> + V4L2_FWNODE_BUS_TYPE_GUESS = 0,
> + V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
> + V4L2_FWNODE_BUS_TYPE_CSI1,
> + V4L2_FWNODE_BUS_TYPE_CCP2,
> + NR_OF_V4L2_FWNODE_BUS_TYPE,
> +};
> +
>  static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
>  struct v4l2_fwnode_endpoint *vep)
>  {
> @@ -168,6 +176,7 @@ static void v4l2_fwnode_endpoint_parse_parallel_bus(
>  int v4l2_fwnode_endpoint_parse(struct fwnode_handle *fwnode,
>  struct v4l2_fwnode_endpoint *vep)
>  {
> + u32 bus_type = 0;
>   int rval;
>  
>   fwnode_graph_parse_endpoint(fwnode, >base);
> @@ -176,6 +185,8 @@ int v4l2_fwnode_endpoint_parse(struct fwnode_handle 
> *fwnode,
>   memset(>bus_type, 0, sizeof(*vep) -
>  offsetof(typeof(*vep), bus_type));
>  
> + fwnode_property_read_u32(fwnode, "bus-type", _type);
> +
>   rval = v4l2_fwnode_endpoint_parse_csi2_bus(fwnode, vep);
>   if (rval)
>   return rval;
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH 5/8] v4l2-flash: Flash ops aren't mandatory

[Sebastian Reichel]

Hi,

On Thu, Jun 15, 2017 at 03:32:10PM +0300, Sakari Ailus wrote:
> On Thu, Jun 15, 2017 at 11:24:26AM +0200, Sebastian Reichel wrote:
> > On Wed, Jun 14, 2017 at 12:47:16PM +0300, Sakari Ailus wrote:
> > > None of the flash operations are not mandatory and therefore there should
> > > be no need for the flash ops structure either. Accept NULL.
> > 
> > I think you negated one time too much :). Otherwise:
> > 
> > Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>
> 
> Thanks!
> 
> The new one reads:
> 
> None of the flash operations are mandatory and therefore there should be no
> need for the flash ops structure either. Accept NULL.

Fine with me.

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH 4/8] v4l2-flash: Use led_classdev instead of led_classdev_flash for indicator

[Sebastian Reichel]

Hi,

On Wed, Jun 14, 2017 at 12:47:15PM +0300, Sakari Ailus wrote:
> The V4L2 flash class initialisation expects struct led_classdev_flash that
> describes an indicator but only uses struct led_classdev which is a field
> iled_cdev in the struct. Use struct iled_cdev only.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 19 +++
>  include/media/v4l2-flash-led-class.h   |  6 +++---
>  2 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c 
> b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> index 7b82881..6d69119 100644
> --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> @@ -110,7 +110,7 @@ static void v4l2_flash_set_led_brightness(struct 
> v4l2_flash *v4l2_flash,
>   led_set_brightness_sync(_flash->fled_cdev->led_cdev,
>   brightness);
>   } else {
> - led_set_brightness_sync(_flash->iled_cdev->led_cdev,
> + led_set_brightness_sync(v4l2_flash->iled_cdev,
>   brightness);
>   }
>  }
> @@ -133,7 +133,7 @@ static int v4l2_flash_update_led_brightness(struct 
> v4l2_flash *v4l2_flash,
>   return 0;
>   led_cdev = _flash->fled_cdev->led_cdev;
>   } else {
> - led_cdev = _flash->iled_cdev->led_cdev;
> + led_cdev = v4l2_flash->iled_cdev;
>   }
>  
>   ret = led_update_brightness(led_cdev);
> @@ -529,8 +529,7 @@ static int v4l2_flash_open(struct v4l2_subdev *sd, struct 
> v4l2_subdev_fh *fh)
>   struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
>   struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>   struct led_classdev *led_cdev = _cdev->led_cdev;
> - struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
> - struct led_classdev *led_cdev_ind = NULL;
> + struct led_classdev *led_cdev_ind = v4l2_flash->iled_cdev;
>   int ret = 0;
>  
>   if (!v4l2_fh_is_singular(>vfh))
> @@ -543,9 +542,7 @@ static int v4l2_flash_open(struct v4l2_subdev *sd, struct 
> v4l2_subdev_fh *fh)
>  
>   mutex_unlock(_cdev->led_access);
>  
> - if (iled_cdev) {
> - led_cdev_ind = _cdev->led_cdev;
> -
> + if (led_cdev_ind) {
>   mutex_lock(_cdev_ind->led_access);
>  
>   led_sysfs_disable(led_cdev_ind);
> @@ -578,7 +575,7 @@ static int v4l2_flash_close(struct v4l2_subdev *sd, 
> struct v4l2_subdev_fh *fh)
>   struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
>   struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>   struct led_classdev *led_cdev = _cdev->led_cdev;
> - struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
> + struct led_classdev *led_cdev_ind = v4l2_flash->iled_cdev;
>   int ret = 0;
>  
>   if (!v4l2_fh_is_singular(>vfh))
> @@ -593,9 +590,7 @@ static int v4l2_flash_close(struct v4l2_subdev *sd, 
> struct v4l2_subdev_fh *fh)
>  
>   mutex_unlock(_cdev->led_access);
>  
> - if (iled_cdev) {
> - struct led_classdev *led_cdev_ind = _cdev->led_cdev;
> -
> + if (led_cdev_ind) {
>   mutex_lock(_cdev_ind->led_access);
>   led_sysfs_enable(led_cdev_ind);
>   mutex_unlock(_cdev_ind->led_access);
> @@ -614,7 +609,7 @@ static const struct v4l2_subdev_ops v4l2_flash_subdev_ops;
>  struct v4l2_flash *v4l2_flash_init(
>   struct device *dev, struct fwnode_handle *fwn,
>   struct led_classdev_flash *fled_cdev,
> - struct led_classdev_flash *iled_cdev,
> + struct led_classdev *iled_cdev,
>   const struct v4l2_flash_ops *ops,
>   struct v4l2_flash_config *config)
>  {
> diff --git a/include/media/v4l2-flash-led-class.h 
> b/include/media/v4l2-flash-led-class.h
> index f9dcd54..54e31a8 100644
> --- a/include/media/v4l2-flash-led-class.h
> +++ b/include/media/v4l2-flash-led-class.h
> @@ -85,7 +85,7 @@ struct v4l2_flash_config {
>   */
>  struct v4l2_flash {
>   struct led_classdev_flash *fled_cdev;
> - struct led_classdev_flash *iled_cdev;
> + struct led_classdev *iled_cdev;
>   const struct v4l2_flash_ops *ops;
>  
>   struct v4l2_subdev sd;
> @@ -124,7 +124,7 @@ static inline struct v4l2_flash 
> *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c)
>  struct v4l2_flash *v4l2_flash_init(
>  

Re: [PATCH 8/8] arm: dts: omap3: N9/N950: Add AS3645A camera flash

[Sebastian Reichel]

Hi,

On Wed, Jun 14, 2017 at 12:47:19PM +0300, Sakari Ailus wrote:
> From: Sakari Ailus <sakari.ai...@iki.fi>
> 
> Add the as3645a flash controller to the DT source as well as the flash
> property with the as3645a device phandle to the sensor DT node.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  arch/arm/boot/dts/omap3-n9.dts   |  1 +
>  arch/arm/boot/dts/omap3-n950-n9.dtsi | 14 ++
>  arch/arm/boot/dts/omap3-n950.dts |  1 +
>  3 files changed, 16 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/omap3-n9.dts b/arch/arm/boot/dts/omap3-n9.dts
> index b9e58c5..f95e7b1 100644
> --- a/arch/arm/boot/dts/omap3-n9.dts
> +++ b/arch/arm/boot/dts/omap3-n9.dts
> @@ -26,6 +26,7 @@
>   clocks = < 0>;
>   clock-frequency = <960>;
>   nokia,nvm-size = <(16 * 64)>;
> + flash = <>;
>   port {
>   smia_1_1: endpoint {
>   link-frequencies = /bits/ 64 <19920 
> 21000 49920>;
> diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi 
> b/arch/arm/boot/dts/omap3-n950-n9.dtsi
> index df3366f..8bd6673 100644
> --- a/arch/arm/boot/dts/omap3-n950-n9.dtsi
> +++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
> @@ -265,6 +265,20 @@
>  
>   {
>   clock-frequency = <40>;
> +
> + as3645a: flash@30 {
> + reg = <0x30>;
> + compatible = "ams,as3645a";
> + flash {
> + flash-timeout-us = <15>;
> + flash-max-microamp = <32>;
> + led-max-microamp = <6>;
> + peak-current-limit = <175>;
> + };
> + indicator {
> + led-max-microamp = <1>;
> + };
> + };
>  };
>  
>   {
> diff --git a/arch/arm/boot/dts/omap3-n950.dts 
> b/arch/arm/boot/dts/omap3-n950.dts
> index 646601a..8fca038 100644
> --- a/arch/arm/boot/dts/omap3-n950.dts
> +++ b/arch/arm/boot/dts/omap3-n950.dts
> @@ -60,6 +60,7 @@
>   clocks = < 0>;
>   clock-frequency = <960>;
>   nokia,nvm-size = <(16 * 64)>;
> + flash = <>;
>   port {
>   smia_1_1: endpoint {
>   link-frequencies = /bits/ 64 <21000 
> 33360 39840>;
> -- 
> 2.1.4
> 


signature.asc
Description: PGP signature

Re: [PATCH 5/8] v4l2-flash: Flash ops aren't mandatory

[Sebastian Reichel]

Hi,

On Wed, Jun 14, 2017 at 12:47:16PM +0300, Sakari Ailus wrote:
> None of the flash operations are not mandatory and therefore there should
> be no need for the flash ops structure either. Accept NULL.

I think you negated one time too much :). Otherwise:

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c 
> b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> index 6d69119..fdb79da 100644
> --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> @@ -18,7 +18,7 @@
>  #include 
>  
>  #define has_flash_op(v4l2_flash, op) \
> - (v4l2_flash && v4l2_flash->ops->op)
> + (v4l2_flash && v4l2_flash->ops && v4l2_flash->ops->op)
>  
>  #define call_flash_op(v4l2_flash, op, arg)   \
>   (has_flash_op(v4l2_flash, op) ? \
> @@ -618,7 +618,7 @@ struct v4l2_flash *v4l2_flash_init(
>   struct v4l2_subdev *sd;
>   int ret;
>  
> - if (!fled_cdev || !ops || !config)
> + if (!fled_cdev || !config)
>   return ERR_PTR(-EINVAL);
>  
>   led_cdev = _cdev->led_cdev;
> -- 
> 2.1.4
> 


signature.asc
Description: PGP signature

Re: [PATCH 1/8] dt: bindings: Add a binding for flash devices associated to a sensor

[Sebastian Reichel]

Hi,

On Wed, Jun 14, 2017 at 12:47:12PM +0300, 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 <sakari.ai...@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
>  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 9cd2a36..9723f7e 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: phandle referring to the flash driver chip. A flash driver may
> +  have multiple flashes connected to it.
> +
> +
>  Optional endpoint properties
>  
>  
> -- 
> 2.1.4
> 


signature.asc
Description: PGP signature

Re: [PATCH v5 2/3] platform: add video-multiplexer subdevice driver

[Sebastian Reichel]

Hi,

On Wed, May 17, 2017 at 05:15:06PM +0200, Philipp Zabel wrote:
> This driver can handle SoC internal and external video bus multiplexers,
> controlled by mux controllers provided by the mux controller framework,
> such as MMIO register bitfields or GPIOs. The subdevice passes through
> the mbus configuration of the active input to the output side.
> 
> Signed-off-by: Sascha Hauer <s.ha...@pengutronix.de>
> Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> Signed-off-by: Steve Longerbeam <steve_longerb...@mentor.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
> No changes since v4 [1]:
> 
> This patch depends on the mux subsystem [2] and on the mmio-mux driver [3]
> to work on i.MX6. The follow-up patch will make this usable until the mux
> framework is merged.
> 
> [1] https://patchwork.kernel.org/patch/9712131/
> [2] https://patchwork.kernel.org/patch/9725911/
> [3] https://patchwork.kernel.org/patch/9725893/
> ---
>  drivers/media/platform/Kconfig |   6 +
>  drivers/media/platform/Makefile|   2 +
>  drivers/media/platform/video-mux.c | 295 
> +
>  3 files changed, 303 insertions(+)
>  create mode 100644 drivers/media/platform/video-mux.c
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index ac026ee1ca074..259c0ff780937 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
> To compile this driver as a module, choose M here: the
> module will be called arv.
>  
> +config VIDEO_MUX
> + tristate "Video Multiplexer"
> + depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && 
> MULTIPLEXER
> + help
> +   This driver provides support for N:1 video bus multiplexers.
> +
>  config VIDEO_OMAP3
>   tristate "OMAP 3 Camera support"
>   depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 63303d63c64cf..a6363023f981e 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -28,6 +28,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)  += sh_veu.o
>  
>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)  += m2m-deinterlace.o
>  
> +obj-$(CONFIG_VIDEO_MUX)  += video-mux.o
> +
>  obj-$(CONFIG_VIDEO_S3C_CAMIF)+= s3c-camif/
>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS)   += exynos4-is/
>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG) += s5p-jpeg/
> diff --git a/drivers/media/platform/video-mux.c 
> b/drivers/media/platform/video-mux.c
> new file mode 100644
> index 0..e35ffa18126f3
> --- /dev/null
> +++ b/drivers/media/platform/video-mux.c
> @@ -0,0 +1,295 @@
> +/*
> + * video stream multiplexer controlled via mux control
> + *
> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <ker...@pengutronix.de>
> + * Copyright (C) 2016-2017 Pengutronix, Philipp Zabel <ker...@pengutronix.de>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct video_mux {
> + struct v4l2_subdev subdev;
> + struct media_pad *pads;
> + struct v4l2_mbus_framefmt *format_mbus;
> + struct mux_control *mux;
> + struct mutex lock;
> + int active;
> +};
> +
> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev 
> *sd)
> +{
> + return container_of(sd, struct video_mux, subdev);
> +}
> +
> +static int video_mux_link_setup(struct media_entity *entity,
> + const struct media_pad *local,
> + const struct media_pad *remote, u32 flags)
> +{
> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> + int ret = 0;
> +
> + /*
> +  * The mux state is determined by the enabled sink pad link.
>

Re: [RFC v2 3/3] dt: bindings: Add a binding for referencing EEPROM from camera sensors

[Sebastian Reichel]

Hi,

On Fri, May 05, 2017 at 11:48:30AM +0300, Sakari Ailus wrote:
> Many camera sensor devices contain EEPROM chips that describe the
> properties of a given unit --- the data is specific to a given unit can
> thus is not stored e.g. in user space or the driver.
> 
> Some sensors embed the EEPROM chip and it can be accessed through the
> sensor's I2C interface. This property is to be used for devices where the
> EEPROM chip is accessed through a different I2C address than the sensor.
> 
> The intent is to later provide this information to the user space.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt 
> b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 0a33240..38e3916 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -79,6 +79,9 @@ Optional properties
>  
>  - lens-focus: A phandle to the node of the focus lens controller.
>  
> +- eeprom: A phandle to the node of the EEPROM describing the camera sensor
> +  (i.e. device specific calibration data), in case it differs from the
> +  sensor node.
>  
>  Optional endpoint properties
>  

-- Sebastian


signature.asc
Description: PGP signature

Re: [RFC v2 2/3] dt: bindings: Add lens-focus binding for image sensors

[Sebastian Reichel]

Hi,

On Fri, May 05, 2017 at 11:48:29AM +0300, Sakari Ailus wrote:
> The lens-focus property contains a phandle to the lens voice coil driver
> that is associated to the sensor; typically both are contained in the same
> camera module.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> Acked-by: Pavel Machek <pa...@ucw.cz>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt 
> b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index dac764b..0a33240 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -77,6 +77,8 @@ Optional properties
>single LED, then the phandles here refer to the child nodes of the LED
>driver describing individual LEDs.
>  
> +- lens-focus: A phandle to the node of the focus lens controller.
> +
>  
>  Optional endpoint properties
>  

-- Sebastian


signature.asc
Description: PGP signature

Re: [RFC 1/3] dt: bindings: Add a binding for flash devices associated to a sensor

[Sebastian Reichel]

Hi,

On Fri, May 05, 2017 at 11:28:34AM +0300, Sakari Ailus wrote:
> Sebastian Reichel wrote:
> > On Tue, May 02, 2017 at 01:25:47PM +0300, Sakari Ailus wrote:
> > > +- flash: An array of phandles that refer to the flash light sources
> > > +  related to an image sensor. These could be e.g. LEDs. In case the LED
> > > +  driver drives more than a single LED, then the phandles here refer to
> > > +  the child nodes of the LED driver describing individual LEDs. Only
> > > +  valid for device nodes that are related to an image sensor.
> > 
> > s/driver/controller/g - DT describes HW. Otherwise
> 
> Driver is hardware in this case. :-) The chip that acts as a current sink or
> source for the LED is the driver. E.g. the adp1653 documentation describes
> the chip as "Compact, High Efficiency, High Power, Flash/Torch LED Driver
> with Dual Interface".
> 
> It might be still possible to improve the wording. Software oriented folks
> are more likely to misunderstand the meaning of driver here, but controller
> might seem ambiguous for hardware oriented people.
> 
> How about:
> 
> - flash: An array of phandles that refer to the flash light sources
>   related to an image sensor. These could be e.g. LEDs. In case the LED
>   driver (current sink or source chip for the LED(s)) drives more than a
>   single LED, then the phandles here refer to the child nodes of the LED
>   driver describing individual LEDs. Only valid for device nodes that are
>   related to an image sensor.

Maybe drop the last sentence? The requirement is already in the
first one. Otherwise:

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian


signature.asc
Description: PGP signature

Re: [RFC 3/3] dt: bindings: Add a binding for referencing EEPROM from camera sensors

[Sebastian Reichel]

Hi,

On Tue, May 02, 2017 at 01:25:49PM +0300, Sakari Ailus wrote:
> Many camera sensor devices contain EEPROM chips that describe the
> properties of a given unit --- the data is specific to a given unit can
> thus is not stored e.g. in user space or the driver.
> 
> Some sensors embed the EEPROM chip and it can be accessed through the
> sensor's I²C interface. This property is to be used for devices where the
> EEPROM chip is accessed through a different I²C address than the sensor.
> 
> The intent is to later provide this information to the user space.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt 
> b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index e52aefc..9bd2005 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -80,6 +80,9 @@ Optional properties
>  - lens-focus: A phandle to the node of the lens. Only valid for device
>nodes that are related to an image sensor.
>  
> +- eeprom: A phandle to the node of the related EEPROM. Only valid for
> +  device nodes that are related to an image sensor.

Here it's even more obvious, that the second sentence is redundant.
The requirement is already in the first sentence :) Instead it
should be mentioned, that this is to be used by devices not having
their own embedded eeprom. How about:

eeprom: A phandle to the node of the EEPROM describing the camera
sensor (i.e. device specific calibration data), in case it differs
from the sensor node.

-- Sebastian


signature.asc
Description: PGP signature

Re: [RFC 2/3] dt: bindings: Add lens-focus binding for image sensors

[Sebastian Reichel]

Hi,

On Tue, May 02, 2017 at 01:25:48PM +0300, Sakari Ailus wrote:
> The lens-focus property contains a phandle to the lens voice coil driver
> that is associated to the sensor; typically both are contained in the same
> camera module.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt 
> b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index d6c62bc..e52aefc 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -77,6 +77,9 @@ Optional properties
>the child nodes of the LED driver describing individual LEDs. Only
>valid for device nodes that are related to an image sensor.
>  
> +- lens-focus: A phandle to the node of the lens. Only valid for device
> +  nodes that are related to an image sensor.

s/of the lens/of the focus lens controller/

Also I wonder about the second sentence. That sentence could
basically be added to every property, so can't we just drop it?

-- Sebastian


signature.asc
Description: PGP signature

Re: [RFC 1/3] dt: bindings: Add a binding for flash devices associated to a sensor

[Sebastian Reichel]

Hi Sakari,

On Tue, May 02, 2017 at 01:25:47PM +0300, 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 <sakari.ai...@linux.intel.com>
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt 
> b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 9cd2a36..d6c62bc 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -67,6 +67,17 @@ are required in a relevant parent node:
>   identifier, should be 1.
>   - #size-cells: should be zero.
>  
> +
> +Optional properties
> +---
> +
> +- flash: An array of phandles that refer to the flash light sources
> +  related to an image sensor. These could be e.g. LEDs. In case the LED
> +  driver drives more than a single LED, then the phandles here refer to
> +  the child nodes of the LED driver describing individual LEDs. Only
> +  valid for device nodes that are related to an image sensor.

s/driver/controller/g - DT describes HW. Otherwise

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v3 2/2] [media] platform: add video-multiplexer subdevice driver

[Sebastian Reichel]

Hi,

On Thu, May 04, 2017 at 03:38:57PM +0200, Philipp Zabel wrote:
> This driver can handle SoC internal and external video bus multiplexers,
> controlled by mux controllers provided by the mux controller framework,
> such as MMIO register bitfields or GPIOs. The subdevice passes through
> the mbus configuration of the active input to the output side.
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Philipp Zabel 
> Signed-off-by: Steve Longerbeam 
> ---
> Changes since v2 [1]:
>  - Extend vmux->lock to protect mbus format against simultaneous access
>from get/set_format calls.
>  - Drop is_source_pad(), check pad->flags & MEDIA_PAD_FL_SOURCE directly.
>  - Replace v4l2_of_parse_endpoint call with v4l2_fwnode_endpoint_parse,
>include media/v4l2-fwnode.h instead of media/v4l2-of.h.
>  - Constify ops structures.
> 
> This was previously sent as part of Steve's i.MX media series [2].
> Tested against
> https://git.linuxtv.org/sailus/media_tree.git/log/?h=v4l2-acpi-merge
> 
> [1] https://patchwork.kernel.org/patch/9708237/
> [2] https://patchwork.kernel.org/patch/9647869/

Looks fine to me. Just one nitpick:

> +static int video_mux_probe(struct platform_device *pdev)
> +{

[...]

> + vmux->pads = devm_kzalloc(dev, sizeof(*vmux->pads) * num_pads,
> +   GFP_KERNEL);
> + vmux->format_mbus = devm_kzalloc(dev, sizeof(*vmux->format_mbus) *
> +  num_pads, GFP_KERNEL);
> + vmux->endpoint = devm_kzalloc(dev, sizeof(*vmux->endpoint) *
> +   (num_pads - 1), GFP_KERNEL);

devm_kcalloc(dev, num_pads, sizeof(*foo), GFP_KERNEL).

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v3 1/2] [media] dt-bindings: Add bindings for video-multiplexer device

[Sebastian Reichel]

Hi,

On Thu, May 04, 2017 at 03:38:56PM +0200, Philipp Zabel wrote:
> Add bindings documentation for the video multiplexer device.
> 
> Signed-off-by: Sascha Hauer <s.ha...@pengutronix.de>
> Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> Signed-off-by: Steve Longerbeam <steve_longerb...@mentor.com>
> Acked-by: Peter Rosin <p...@axentia.se>
> Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>

-- Sebastian

> ---
> No changes since v2 [1].
> 
> This was previously sent as part of Steve's i.MX media series [2].
> 
> [1] https://patchwork.kernel.org/patch/9708235/
> [2] https://patchwork.kernel.org/patch/9647951/
> ---
>  .../devicetree/bindings/media/video-mux.txt| 60 
> ++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/video-mux.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/video-mux.txt 
> b/Documentation/devicetree/bindings/media/video-mux.txt
> new file mode 100644
> index 0..63b9dc913e456
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/video-mux.txt
> @@ -0,0 +1,60 @@
> +Video Multiplexer
> +=
> +
> +Video multiplexers allow to select between multiple input ports. Video 
> received
> +on the active input port is passed through to the output port. Muxes 
> described
> +by this binding are controlled by a multiplexer controller that is described 
> by
> +the bindings in Documentation/devicetree/bindings/mux/mux-controller.txt
> +
> +Required properties:
> +- compatible : should be "video-mux"
> +- mux-controls : mux controller node to use for operating the mux
> +- #address-cells: should be <1>
> +- #size-cells: should be <0>
> +- port@*: at least three port nodes containing endpoints connecting to the
> +  source and sink devices according to of_graph bindings. The last port is
> +  the output port, all others are inputs.
> +
> +Optionally, #address-cells, #size-cells, and port nodes can be grouped under 
> a
> +ports node as described in Documentation/devicetree/bindings/graph.txt.
> +
> +Example:
> +
> + mux: mux-controller {
> + compatible = "gpio-mux";
> + #mux-control-cells = <0>;
> +
> + mux-gpios = < 15 GPIO_ACTIVE_HIGH>;
> + };
> +
> + video-mux {
> + compatible = "video-mux";
> + mux-controls = <>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + mux_in0: endpoint {
> + remote-endpoint = <_source0_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + mux_in1: endpoint {
> + remote-endpoint = <_source1_out>;
> + };
> + };
> +
> + port@2 {
> + reg = <2>;
> +
> + mux_out: endpoint {
> + remote-endpoint = <_interface_in>;
> + };
> + };
> + };
> +};
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH 1/8] arm: omap4: enable CEC pin for Pandaboard A4 and ES

[Sebastian Reichel]

Hi,

On Fri, Apr 28, 2017 at 08:08:59AM -0700, Tony Lindgren wrote:
> * Tomi Valkeinen  [170428 04:15]:
> > On 14/04/17 13:25, Hans Verkuil wrote:
> > > From: Hans Verkuil 
> > > 
> > > The CEC pin was always pulled up, making it impossible to use it.
> > > 
> > > Change to PIN_INPUT so it can be used by the new CEC support.
> ...
> 
> > Reviewed-by: Tomi Valkeinen 
> > 
> > Tony, can you queue this? It's safe to apply separately from the rest of
> > the HDMI CEC work.
> 
> Sure will do.

I guess the same patch should be applied to Droid 4?

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH] omap3isp: add support for CSI1 bus

[Sebastian Reichel]

Hi,

On Wed, Feb 15, 2017 at 11:23:01AM +0100, Pavel Machek wrote:
> It seems csiphy_routing_cfg_3430 is not called at all. I added
> printks, but they don't trigger. If you have an idea what is going on
> there, it would help...

You added printk to csiphy_routing_cfg_3630 instead of csiphy_routing_cfg_3430
and N900 has OMAP3430. Function should be called when you start (or
stop) using the camera:

csiphy_routing_cfg_3430(...)
csiphy_routing_cfg(...)
omap3isp_csiphy_config(...)
omap3isp_csiphy_acquire(...) & omap3isp_csiphy_release(...)
ccp2_s_stream(...)

-- Sebastian

> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c 
> b/drivers/media/platform/omap3isp/ispcsiphy.c
> index 6b814e1..fe9303a 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -30,6 +30,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
>   u32 reg;
>   u32 shift, mode;
>  
> + printk("routing cfg 3630: iface %d, %d\n", iface, 
> ISP_INTERFACE_CCP2B_PHY1);
> + 
>   regmap_read(phy->isp->syscon, phy->isp->syscon_offset, );
>  
>   switch (iface) {
> @@ -74,6 +76,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, 
> u32 iface, bool on,
>   u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
>   | OMAP343X_CONTROL_CSIRXFE_RESET;
>  
> + /* FIXME: can this be used instead of if (isp->revision) in ispccp2.c? 
> */
> + 
> + printk("routing cfg: iface %d, %d\n", iface, ISP_INTERFACE_CCP2B_PHY1);
>   /* Only the CCP2B on PHY1 is configurable. */
>   if (iface != ISP_INTERFACE_CCP2B_PHY1)
>   return;
> @@ -105,6 +110,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
>  enum isp_interface_type iface, bool on,
>  bool ccp2_strobe)
>  {
> + printk("csiphy_routing_cfg\n");
>   if (phy->isp->phy_type == ISP_PHY_TYPE_3630 && on)
>   return csiphy_routing_cfg_3630(phy, iface, ccp2_strobe);
>   if (phy->isp->phy_type == ISP_PHY_TYPE_3430)


signature.asc
Description: PGP signature

Re: [RFC 06/13] v4l2-async: per notifier locking

[Sebastian Reichel]

Hi,

On Tue, Feb 14, 2017 at 02:39:56PM +0100, Pavel Machek wrote:
> From: Sebastian Reichel <s...@kernel.org>
> 
> Without this, camera support breaks boot on N900.

That's kind of vague. I just checked my original patch and it looks
like I did not bother to write a proper patch description. I suggest
to make this

"Fix v4l2-async locking, so that v4l2_async_notifiers can be used
recursively. This is important for sensors, that are only reachable
by the image signal processor through some intermediate device."

You should probably move move this patch directly before the
video-bus-switch patch, since its a preparation for it.

Also this is missing my SoB:

Signed-off-by: Sebastian Reichel <s...@kernel.org>

> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov...@gmail.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 54 
> ++--
>  include/media/v4l2-async.h   |  2 ++
>  2 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 96cc733..26492a2 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -57,7 +57,6 @@ static bool match_custom(struct v4l2_subdev *sd, struct 
> v4l2_async_subdev *asd)
>  
>  static LIST_HEAD(subdev_list);
>  static LIST_HEAD(notifier_list);
> -static DEFINE_MUTEX(list_lock);
>  
>  static struct v4l2_async_subdev *v4l2_async_belongs(struct 
> v4l2_async_notifier *notifier,
>   struct v4l2_subdev *sd)
> @@ -102,12 +101,15 @@ static int v4l2_async_test_notify(struct 
> v4l2_async_notifier *notifier,
>  
>   if (notifier->bound) {
>   ret = notifier->bound(notifier, sd, asd);
> - if (ret < 0)
> + if (ret < 0) {
> + dev_warn(notifier->v4l2_dev->dev, "subdev bound 
> failed\n");
>   return ret;
> + }
>   }
>  
>   ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
>   if (ret < 0) {
> + dev_warn(notifier->v4l2_dev->dev, "subdev register failed\n");
>   if (notifier->unbind)
>   notifier->unbind(notifier, sd, asd);
>   return ret;
> @@ -141,7 +143,7 @@ int v4l2_async_notifier_register(struct v4l2_device 
> *v4l2_dev,
>  {
>   struct v4l2_subdev *sd, *tmp;
>   struct v4l2_async_subdev *asd;
> - int i;
> + int ret = 0, i;
>  
>   if (!notifier->num_subdevs || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
>   return -EINVAL;
> @@ -149,6 +151,7 @@ int v4l2_async_notifier_register(struct v4l2_device 
> *v4l2_dev,
>   notifier->v4l2_dev = v4l2_dev;
>   INIT_LIST_HEAD(>waiting);
>   INIT_LIST_HEAD(>done);
> + mutex_init(>lock);
>  
>   for (i = 0; i < notifier->num_subdevs; i++) {
>   asd = notifier->subdevs[i];
> @@ -168,28 +171,22 @@ int v4l2_async_notifier_register(struct v4l2_device 
> *v4l2_dev,
>   list_add_tail(>list, >waiting);
>   }
>  
> - mutex_lock(_lock);
> + /* Keep also completed notifiers on the list */
> + list_add(>list, _list);
> + mutex_lock(>lock);
>  
>   list_for_each_entry_safe(sd, tmp, _list, async_list) {
> - int ret;
> -
>   asd = v4l2_async_belongs(notifier, sd);
>   if (!asd)
>   continue;
>  
>   ret = v4l2_async_test_notify(notifier, sd, asd);
> - if (ret < 0) {
> - mutex_unlock(_lock);
> - return ret;
> - }
> + if (ret < 0)
> + break;
>   }
> + mutex_unlock(>lock);
>  
> - /* Keep also completed notifiers on the list */
> - list_add(>list, _list);
> -
> - mutex_unlock(_lock);
> -
> - return 0;
> + return ret;
>  }
>  EXPORT_SYMBOL(v4l2_async_notifier_register);
>  
> @@ -210,7 +207,7 @@ void v4l2_async_notifier_unregister(struct 
> v4l2_async_notifier *notifier)
>   "Failed to allocate device cache!\n");
>   }
>  
> - mutex_lock(_lock);
> + mutex_lock(>lock);
>  
>   list_del(>list);
>  
> @@ -237,7 +234,7 @@ void v4l2_async_notifier_unregister(struct 
> v4l2_async_notifier *notifier)
>   put_device(d);
>   }
>  
> - mutex_unlock(_lock);
> + mutex_unlock(>lock);
>  
>   /*
>* Call device_attach() t

Re: [PATCH] devicetree: Add video bus switch

[Sebastian Reichel]

Hi,

On Sun, Feb 05, 2017 at 10:12:20PM +0100, Pavel Machek wrote:
> > > 9) Highly reconfigurable hardware - Julien Beraud
> > > 
> > > - 44 sub-devices connected with an interconnect.
> > > - As long as formats match, any sub-device could be connected to any
> > > - other sub-device through a link.
> > > - The result is 44 * 44 links at worst.
> > > - A switch sub-device proposed as the solution to model the
> > > - interconnect. The sub-devices are connected to the switch
> > > - sub-devices through the hardware links that connect to the
> > > - interconnect.
> > > - The switch would be controlled through new IOCTLs S_ROUTING and
> > > - G_ROUTING.
> > > - Patches available:
> > >  http://git.linuxtv.org/cgit.cgi/pinchartl/media.git/log/?h=xilinx-wip
> > > 
> > > but the patches are from 2005. So I guess I'll need some guidance here...
> > 
> > Yeah, that's where it began (2015?), but right now I can only suggest to
> > wait until there's more. My estimate is within next couple of weeks /
> > months. But it won't be years.
> 
> Ok, week or two would be ok, couple of months is not. And all I need
> is single hook in common structure.
> 
> So if g_endpoint_config hook looks sane to _you_, I suggest we simply
> proceed. Now, maybe Mauro Carvalho Chehab 
> or Laurent or Julien will want a different solution, but
> then... they'll have to suggest something doable now, not in couple of
> months.
> 
> Does that sound like a plan?
> 
> Mauro added to cc list, so we can get some input.

side note: It's an kernel-internal API only used by the media
subsystem. Code can be easily updated to use an improved API
at any time.

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH] devicetree: Add video bus switch

[Sebastian Reichel]

Hi,

On Fri, Feb 03, 2017 at 10:07:28PM +0100, Pavel Machek wrote:
> On Fri 2017-02-03 14:32:19, Pali Rohár wrote:
> > On Friday 03 February 2017 13:35:08 Pavel Machek wrote:
> > > N900 contains front and back camera, with a switch between the
> > > two. This adds support for the switch component, and it is now
> > > possible to select between front and back cameras during runtime.
> > 
> > IIRC for controlling cameras on N900 there are two GPIOs. Should
> > not you have both in switch driver?
>
> I guess you recall wrongly :-). Switch seems to work. The issue was
> with switch GPIO also serving as reset GPIO for one sensor, or
> something like that, if _I_ recall correctly ;-).

I have a schematic in my master thesis, which shows how the camera
sensors are connected to the SoC. The PDF is available here:

https://www.uni-oldenburg.de/fileadmin/user_upload/informatik/ag/svs/download/thesis/Reichel_Sebastian.pdf

The schematic is on page 37 (or 45 if your PDF reader does not
use different numbers for the preamble stuff).

--Sebastian


signature.asc
Description: PGP signature

Re: [PATCHv2] dt: bindings: Add support for CSI1 bus

[Sebastian Reichel]

Hi,

On Wed, Jan 11, 2017 at 11:53:35PM +0100, Pavel Machek wrote:
> From: Sakari Ailus <sakari.ai...@iki.fi>
> 
> In the vast majority of cases the bus type is known to the driver(s)
> since a receiver or transmitter can only support a single one. There
> are cases however where different options are possible.
> 
> The existing V4L2 OF support tries to figure out the bus type and
> parse the bus parameters based on that. This does not scale too well
> as there are multiple serial busses that share common properties.
> 
> Some hardware also supports multiple types of busses on the same
> interfaces.
> 
> Document the CSI1/CCP2 property strobe. It signifies the clock or
> strobe mode.
>  
> Signed-off-by: Sakari Ailus <sakari.ai...@iki.fi>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov...@gmail.com>
> Signed-off-by: Pavel Machek <pa...@ucw.cz>
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt 
> b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 9cd2a36..08c4498 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -76,6 +76,11 @@ Optional endpoint properties
>mode horizontal and vertical synchronization signals are provided to the
>slave device (data source) by the master device (data sink). In the master
>mode the data source device is also the source of the synchronization 
> signals.
> +- bus-type: data bus type. Possible values are:
> +  0 - MIPI CSI2
> +  1 - parallel / Bt656
> +  2 - MIPI CSI1
> +  3 - CCP2
>  - bus-width: number of data lines actively used, valid for the parallel 
> busses.
>  - data-shift: on the parallel data busses, if bus-width is used to specify 
> the
>number of data lines, data-shift can be used to specify which data lines 
> are
> @@ -112,7 +117,8 @@ Optional endpoint properties
>should be the combined length of data-lanes and clock-lanes properties.
>If the lane-polarities property is omitted, the value must be interpreted
>as 0 (normal). This property is valid for serial busses only.
> -
> +- strobe: Whether the clock signal is used as clock or strobe. Used
> +  with CCP2, for instance.
>  
>  Example
>  ---
> 
> 

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH] mark myself as mainainer for camera on N900

[Sebastian Reichel]

Hi,

On Tue, Dec 27, 2016 at 09:59:23PM +0100, Pavel Machek wrote:
> Mark and Sakari as maintainers for Nokia N900 camera pieces.

^^^ missing me after Mark. Otherwise Mark looks like a name :)

> Signed-off-by: Pavel Machek <pa...@ucw.cz>
> 
> ---
> 
> Hi!
> 
> > Yeah, there was big flamewar about the permissions. In the end Linus
> > decided that everyone knows the octal numbers, but the constants are
> > tricky. It began with patch series with 1000 patches...
> > 
> > > Btw. should we update maintainers as well? Would you like to put yourself
> > > there? Feel free to add me, too...
> > 
> > Ok, will do.
> 
> Something like this? Actually, I guess we could merge ADP1653 entry
> there. Yes, it is random collection of devices, but are usually tested
> "together", so I believe one entry makes sense.
> 
> (But I have no problem with having multiple entries, too.)
> 
> Thanks,
>   Pavel
> 
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 63cefa6..1cb1d97 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8613,6 +8613,14 @@ T: git 
> git://git.kernel.org/pub/scm/linux/kernel/git/lftan/nios2.git
>  S:   Maintained
>  F:   arch/nios2/
>  
> +NOKIA N900 CAMERA SUPPORT (ET8EK8 SENSOR, AD5820 FOCUS)
> +M:   Pavel Machek <pa...@ucw.cz>
> +M:   Sakari Ailus <sakari.ai...@iki.fi>
> +L:   linux-media@vger.kernel.org
> +S:   Maintained
> +F:   drivers/media/i2c/et8ek8
> +F:   drivers/media/i2c/ad5820.c

Not sure if this is useful information, but I solved it like this
for N900 power supply drivers:

NOKIA N900 POWER SUPPLY DRIVERS
R:  Pali Rohár <pali.ro...@gmail.com>
F:  include/linux/power/bq2415x_charger.h
F:  include/linux/power/bq27xxx_battery.h
F:  include/linux/power/isp1704_charger.h
F:  drivers/power/supply/bq2415x_charger.c
F:  drivers/power/supply/bq27xxx_battery.c
F:  drivers/power/supply/bq27xxx_battery_i2c.c
F:  drivers/power/supply/isp1704_charger.c
F:  drivers/power/supply/rx51_battery.c

TI BQ27XXX POWER SUPPLY DRIVER
R:  Andrew F. Davis <a...@ti.com>
F:  include/linux/power/bq27xxx_battery.h
F:  drivers/power/supply/bq27xxx_battery.c
F:  drivers/power/supply/bq27xxx_battery_i2c.c

POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS
M:  Sebastian Reichel <s...@kernel.org>
L:  linux...@vger.kernel.org
T:  git 
git://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git
S:  Maintained
F:  Documentation/devicetree/bindings/power/supply/
F:  include/linux/power_supply.h
F:  drivers/power/supply/

This makes it quite easy to see who applies patches and who should
be Cc'd for what reason:

$ ./scripts/get_maintainer.pl -f drivers/power/supply/bq27xxx_battery.c 
"Pali Rohár" <pali.ro...@gmail.com> (reviewer:NOKIA N900 POWER SUPPLY DRIVERS)
"Andrew F. Davis" <a...@ti.com> (reviewer:TI BQ27XXX POWER SUPPLY DRIVER)
Sebastian Reichel <s...@kernel.org> (maintainer:POWER SUPPLY CLASS/SUBSYSTEM 
and DRIVERS)
linux...@vger.kernel.org (open list:POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS)
linux-ker...@vger.kernel.org (open list)

-- Sebastian


signature.asc
Description: PGP signature

Re: [RFC/PATCH] media: Add video bus switch

[Sebastian Reichel]

Hi,

On Thu, Dec 22, 2016 at 11:42:26PM +0100, Pavel Machek wrote:
> On Thu 2016-12-22 15:32:44, Sebastian Reichel wrote:
> > Hi Pavel,
> > 
> > On Thu, Dec 22, 2016 at 02:39:38PM +0100, Pavel Machek wrote:
> > > N900 contains front and back camera, with a switch between the
> > > two. This adds support for the swich component.
> > > 
> > > Signed-off-by: Sebastian Reichel <s...@kernel.org>
> > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov...@gmail.com>
> > > Signed-off-by: Pavel Machek <pa...@ucw.cz>
> > > 
> > > --
> > > 
> > > I see this needs dts documentation, anything else than needs to be
> > > done?
> > 
> > Yes. This driver takes care of the switch gpio, but the cameras also
> > use different bus settings. Currently omap3isp gets the bus-settings
> > from the link connected to the CCP2 port in DT at probe time (*).
> > 
> > So there are two general problems:
> > 
> > 1. Settings must be applied before the streaming starts instead of
> > at probe time, since the settings may change (based one the selected
> > camera). That should be fairly easy to implement by just moving the
> > code to the s_stream callback as far as I can see.
> > 
> > 2. omap3isp should try to get the bus settings from using a callback
> > in the connected driver instead of loading it from DT. Then the
> > video-bus-switch can load the bus-settings from its downstream links
> > in DT and propagate the correct ones to omap3isp based on the
> > selected port. The DT loading part should actually remain in omap3isp
> > as fallback, in case it does not find a callback in the connected driver.
> > That way everything is backward compatible and the DT variant is
> > nice for 1-on-1 scenarios.
> 
> So... did I understood it correctly? (Needs some work to be done...)

I had a quick look and yes, that's basically what I had in mind to
solve the issue. If callback is not available the old system should
be used of course.

> [...]
>
>  static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
> diff --git a/drivers/media/platform/video-bus-switch.c 
> b/drivers/media/platform/video-bus-switch.c
> index 1a5d944..3a2d442 100644
> --- a/drivers/media/platform/video-bus-switch.c
> +++ b/drivers/media/platform/video-bus-switch.c
> @@ -247,12 +247,21 @@ static int vbs_s_stream(struct v4l2_subdev *sd, int 
> enable)
>  {
>   struct v4l2_subdev *subdev = vbs_get_remote_subdev(sd);
>  
> + /* FIXME: we need to set the GPIO here */
> +

The gpio is set when the pad is selected, so no need to do it again.
The gpio selection actually works with your branch (assuming its
based on Ivo's).

>   if (IS_ERR(subdev))
>   return PTR_ERR(subdev);
>  
>   return v4l2_subdev_call(subdev, video, s_stream, enable);
>  }
>  
> +static int vbs_g_endpoint_config(struct v4l2_subdev *sd, struct isp_bus_cfg 
> *cfg)
> +{
> + printk("vbs_g_endpoint_config...\n");
> + return 0;
> +}

Would be nice to find something more abstract than isp_bus_cfg,
which is specific to omap3isp.

-- Sebastian


signature.asc
Description: PGP signature

Re: [RFC/PATCH] media: Add video bus switch

[Sebastian Reichel]

Hi,

On Thu, Dec 22, 2016 at 09:53:17PM +0100, Pavel Machek wrote:
> > 1. Settings must be applied before the streaming starts instead of
> > at probe time, since the settings may change (based one the selected
> > camera). That should be fairly easy to implement by just moving the
> > code to the s_stream callback as far as I can see.
> 
> Ok, I see, where "the code" is basically in vbs_link_setup, right?

I'm not talking about the video bus switch, but about omap3isp.
omap3isp must update the buscfg registers when it starts streaming
instead of at probe time. I just checked and it actually seems to do
so already. So the problem is only updating the buscfg inside of
ccp2_s_stream() before writing the device registers. See next
paragraph for more details.

> > 2. omap3isp should try to get the bus settings from using a callback
> > in the connected driver instead of loading it from DT. Then the
> > video-bus-switch can load the bus-settings from its downstream links
> > in DT and propagate the correct ones to omap3isp based on the
> > selected port. The DT loading part should actually remain in omap3isp
> > as fallback, in case it does not find a callback in the connected driver.
> > That way everything is backward compatible and the DT variant is
> > nice for 1-on-1 scenarios.
> 
> So basically... (struct isp_bus_cfg *) isd->bus would change in
> isp_pipeline_enable()...? 

isp_of_parse_node_csi1(), which is called by isp_of_parse_node()
inits buscfg using DT information. This does not work for N900,
which needs two different buscfg settings based on the selected
camera. I suggest to add a callback to the subdevice instead.

So something like pseudocode is needed in ccp2_s_stream():

/* get current buscfg */
if (subdevice->get_buscfg)
buscfg = subdevice->get_buscfg();
else
buscfg = isp_of_parse_node_csi1();

/* configure device registers */
ccp2_if_configure(buscfg);

This new callback must be implemented in the video-bus-switch,
so that it returns the buscfg based upon the selected camera.

> > Apart from that Sakari told me at ELCE, that the port numbers
> > should be reversed to match the order of other drivers. That's
> > obviously very easy to do :)
> 
> Ok, I guess that can come later :-).
> 
> > Regarding the binding document. I actually did write one:
> > https://git.kernel.org/cgit/linux/kernel/git/sre/linux-n900.git/commit/?h=n900-camera=81e74af53fe6d180616b05792f78badc615e871f
> 
> Thanks, got it.
> 
> > So all in all it shouldn't be that hard to implement the remaining
> > bits.
> 
> :-).
> 
> > (*) Actually it does not for CCP2, but there are some old patches
> > from Sakari adding it for CCP2. It is implemented for parallel port
> > and CSI in this way.
> 
> I think I got the patches in my tree. Camera currently works for me.

If you have working camera you have the CCP2 DT patches in your tree.
They are not yet mainline, though. As far as I can see.

-- Sebastian


signature.asc
Description: PGP signature

Re: [RFC/PATCH] media: Add video bus switch

[Sebastian Reichel]

Hi Pavel,

On Thu, Dec 22, 2016 at 02:39:38PM +0100, Pavel Machek wrote:
> N900 contains front and back camera, with a switch between the
> two. This adds support for the swich component.
> 
> Signed-off-by: Sebastian Reichel <s...@kernel.org>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov...@gmail.com>
> Signed-off-by: Pavel Machek <pa...@ucw.cz>
> 
> --
> 
> I see this needs dts documentation, anything else than needs to be
> done?

Yes. This driver takes care of the switch gpio, but the cameras also
use different bus settings. Currently omap3isp gets the bus-settings
from the link connected to the CCP2 port in DT at probe time (*).

So there are two general problems:

1. Settings must be applied before the streaming starts instead of
at probe time, since the settings may change (based one the selected
camera). That should be fairly easy to implement by just moving the
code to the s_stream callback as far as I can see.

2. omap3isp should try to get the bus settings from using a callback
in the connected driver instead of loading it from DT. Then the
video-bus-switch can load the bus-settings from its downstream links
in DT and propagate the correct ones to omap3isp based on the
selected port. The DT loading part should actually remain in omap3isp
as fallback, in case it does not find a callback in the connected driver.
That way everything is backward compatible and the DT variant is
nice for 1-on-1 scenarios.

Apart from that Sakari told me at ELCE, that the port numbers
should be reversed to match the order of other drivers. That's
obviously very easy to do :)

Regarding the binding document. I actually did write one:
https://git.kernel.org/cgit/linux/kernel/git/sre/linux-n900.git/commit/?h=n900-camera=81e74af53fe6d180616b05792f78badc615e871f

So all in all it shouldn't be that hard to implement the remaining
bits.

(*) Actually it does not for CCP2, but there are some old patches
from Sakari adding it for CCP2. It is implemented for parallel port
and CSI in this way.

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v4] media: Driver for Toshiba et8ek8 5MP sensor

[Sebastian Reichel]

Hi Sakari,

On Mon, Nov 14, 2016 at 11:58:28PM +0200, Sakari Ailus wrote:
> [...]
>
> On Fri, Nov 04, 2016 at 01:05:25AM +0100, Sebastian Reichel wrote:
> > I'm not sure what part relevant for video-bus-switch is currently
> > not supported?
> > 
> > video-bus-switch registers its own async notifier and only registers
> > itself as subdevices to omap3isp, once its own subdevices have been
> > registered successfully.
> 
> Do you happen to have patches for this?
> I still think we should clean up the V4L2 async framework though.

http://git.kernel.org/cgit/linux/kernel/git/sre/linux-n900.git/tree/drivers/media/platform/video-bus-switch.c?h=n900-camera-ivo

It was inside of the RFC series Ivo sent in April.

> [...]

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v4] media: Driver for Toshiba et8ek8 5MP sensor

[Sebastian Reichel]

Hi,

On Fri, Nov 04, 2016 at 01:05:01AM +0200, Sakari Ailus wrote:
> On Thu, Nov 03, 2016 at 11:48:43PM +0100, Sebastian Reichel wrote:
> > On Tue, Nov 01, 2016 at 12:54:08AM +0200, Sakari Ailus wrote:
> > > > > Thanks, this answered half of my questions already. ;-)
> > > > :-).
> > > > 
> > > > I'll have to go through the patches, et8ek8 driver is probably not
> > > > enough to get useful video. platform/video-bus-switch.c is needed for
> > > > camera switching, then some omap3isp patches to bind flash and
> > > > autofocus into the subdevice.
> > > > 
> > > > Then, device tree support on n900 can be added.
> > > 
> > > I briefly discussed with with Sebastian.
> > > 
> > > Do you think the elusive support for the secondary camera is worth keeping
> > > out the main camera from the DT in mainline? As long as there's a 
> > > reasonable
> > > way to get it working, I'd just merge that. If someone ever gets the
> > > secondary camera working properly and nicely with the video bus switch,
> > > that's cool, we'll somehow deal with the problem then. But frankly I don't
> > > think it's very useful even if we get there: the quality is really bad.
> > 
> > If we want to keep open the option to add proper support for the
> > second camera, we could also add the bus switch and not add the
> > front camera node in DT. Then adding the front camera does not
> > require DT or userspace API changes. It would need an additional
> > DT quirk in arch/arm/mach-omap2/board-generic.c for RX51, which
> > adds the CCP2 bus settings from the camera node to the bus
> > switch node to keep isp_of_parse_node happy. That should be
> > easy to implement and not add much delay in upstreaming.
> 
> By adding the video bus switch we have a little bit more complex system as a
> whole. The V4L2 async does not currently support this. There's more here:
> 
> <URL:http://www.spinics.net/lists/linux-media/msg107262.html>

I'm not sure what part relevant for video-bus-switch is currently
not supported?

video-bus-switch registers its own async notifier and only registers
itself as subdevices to omap3isp, once its own subdevices have been
registered successfully.

> What I thought was that once we have everything that's required in
> place, we can just change what's in DT. But the software needs to
> continue to work with the old DT content.

Right, so DT is not a problem. But adding the switch would change
the media-graph, which is exposed to userspace.

> > For actually getting both cameras available with runtime-switching
> > the proper solution would probably involve moving the parsing of
> > the bus-settings to the sensor driver and providing a callback.
> > This callback can be called by omap3isp when it wants to configure
> > the phy (which is basically when it starts streaming). That seems
> > to be the only place needing the buscfg anyways.
> > 
> > Then the video-bus-switch could do something like this (pseudocode):
> > 
> > static void get_buscfg(struct *this, struct *buscfg) {
> > if (selected_cam == 0)
> > return this->sensor_a->get_buscfg(buscfg);
> > else
> > return this->sensor_b->get_buscfg(buscfg);
> > }
> > 
> > Regarding the usefulness: I noticed, that the Neo900 people also
> > plan to have the bus-switch [0]. It's still the same crappy front-cam,
> > though. Nevertheless it might be useful for testing. It has nice
> > test-image capabilities, which might be useful for regression
> > testing once everything is in place.
> > 
> > [0] http://neo900.org/stuff/block-diagrams/neo900/neo900.html
> 
> Seriously? I suppose there should be no need for that anymore, is there?
> 
> I think they wanted to save one GPIO in order to shave off 0,0001 cents from
> the manufacturing costs or something like that. And the result is...
> painful. :-I

CSI1/CCP2 is more than a single I/O pin, isn't it? Or do you
reference to the GPIO dual use to enable frontcam and switch
between the cameras? That is indeed a really ugly solution :(

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v4] media: Driver for Toshiba et8ek8 5MP sensor

[Sebastian Reichel]

Hi,

On Tue, Nov 01, 2016 at 12:54:08AM +0200, Sakari Ailus wrote:
> > > Thanks, this answered half of my questions already. ;-)
> > :-).
> > 
> > I'll have to go through the patches, et8ek8 driver is probably not
> > enough to get useful video. platform/video-bus-switch.c is needed for
> > camera switching, then some omap3isp patches to bind flash and
> > autofocus into the subdevice.
> > 
> > Then, device tree support on n900 can be added.
> 
> I briefly discussed with with Sebastian.
> 
> Do you think the elusive support for the secondary camera is worth keeping
> out the main camera from the DT in mainline? As long as there's a reasonable
> way to get it working, I'd just merge that. If someone ever gets the
> secondary camera working properly and nicely with the video bus switch,
> that's cool, we'll somehow deal with the problem then. But frankly I don't
> think it's very useful even if we get there: the quality is really bad.

If we want to keep open the option to add proper support for the
second camera, we could also add the bus switch and not add the
front camera node in DT. Then adding the front camera does not
require DT or userspace API changes. It would need an additional
DT quirk in arch/arm/mach-omap2/board-generic.c for RX51, which
adds the CCP2 bus settings from the camera node to the bus
switch node to keep isp_of_parse_node happy. That should be
easy to implement and not add much delay in upstreaming.

For actually getting both cameras available with runtime-switching
the proper solution would probably involve moving the parsing of
the bus-settings to the sensor driver and providing a callback.
This callback can be called by omap3isp when it wants to configure
the phy (which is basically when it starts streaming). That seems
to be the only place needing the buscfg anyways.

Then the video-bus-switch could do something like this (pseudocode):

static void get_buscfg(struct *this, struct *buscfg) {
if (selected_cam == 0)
return this->sensor_a->get_buscfg(buscfg);
else
return this->sensor_b->get_buscfg(buscfg);
}

Regarding the usefulness: I noticed, that the Neo900 people also
plan to have the bus-switch [0]. It's still the same crappy front-cam,
though. Nevertheless it might be useful for testing. It has nice
test-image capabilities, which might be useful for regression
testing once everything is in place.

[0] http://neo900.org/stuff/block-diagrams/neo900/neo900.html

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v1.1 4/5] smiapp: Use runtime PM

[Sebastian Reichel]

Hi,

On Fri, Sep 16, 2016 at 01:53:29AM +0300, Sakari Ailus wrote:
> Use runtime PM to manage power. The s_power() core sub-device callback is
> removed as it is no longer needed.
> 
> The power management of the sensor is changed so that it is no longer
> dependent on open file descriptors on sub-device or use_count in the media
> entity but solely will be powered on as needed for probing and streaming.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> ---
> since v1:
> 
> - Both smiapp_set_ctrl() and smiapp_update_mode() perform work which is
>   unrelated to the power state of the device. Instead, check the power
>   state in smiapp_write() which is more appropriate.
> 
> - Don't explicitly disable streaming in smiapp_remove(). It'd be an
>   unrelated change.
> 
>  drivers/media/i2c/smiapp/smiapp-core.c | 130 
> -
>  drivers/media/i2c/smiapp/smiapp-regs.c |   5 ++
>  drivers/media/i2c/smiapp/smiapp.h  |  11 +--
>  3 files changed, 67 insertions(+), 79 deletions(-)

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v1.1 5/5] smiapp: Implement support for autosuspend

[Sebastian Reichel]

Hi,

On Tue, Sep 20, 2016 at 03:29:58PM +0300, Sakari Ailus wrote:
> Delay suspending the device by 1000 ms by default.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> ---
> 
> since v1:
> 
> - Increment usage count before register write using
>   pm_runtime_get_noresume(), and decrement it before returning. This
>   avoids a serialisation problem with autosuspend.
> 
>  drivers/media/i2c/smiapp/smiapp-core.c | 10 +++---
>  drivers/media/i2c/smiapp/smiapp-regs.c | 21 +++--
>  2 files changed, 22 insertions(+), 9 deletions(-)

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v3 00/18] More smiapp cleanups, fixes

[Sebastian Reichel]

Hi,

On Tue, Sep 20, 2016 at 01:02:33AM +0300, Sakari Ailus wrote:
> This set further cleans up the smiapp driver and prepares for
> later changes.
> 
> since v2:
> 
> - Fix badly formatted debug message on wrong frame format model type
> 
> - Add a debug message on faulty frame descriptor (image data lines are
>   among embedded data lines)
> 
> - Fix error handling in registered() callback, add  unregistered()
>   callback
> 
> - smiapp_create_subdev() will return immediately if its ssd argument is
>   NULL. No need for caller to check this.

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH 2/5] smiapp: Set device for pixel array and binner

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:29:18PM +0300, Sakari Ailus wrote:
> The dev field of the v4l2_subdev was left NULL for the pixel array and
> binner sub-devices. Fix this.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH 3/5] smiapp: Set use suspend and resume ops for other functions

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:29:19PM +0300, Sakari Ailus wrote:
> Use the suspend and resume ops for freeze, thaw, poweroff and restore
> callbacks as well.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v1.1 4/5] smiapp: Use runtime PM

[Sebastian Reichel]

Hi,

On Fri, Sep 16, 2016 at 01:53:29AM +0300, Sakari Ailus wrote:
> [...]
>
> diff --git a/drivers/media/i2c/smiapp/smiapp-regs.c 
> b/drivers/media/i2c/smiapp/smiapp-regs.c
> index 1e501c0..a9c7baf 100644
> --- a/drivers/media/i2c/smiapp/smiapp-regs.c
> +++ b/drivers/media/i2c/smiapp/smiapp-regs.c
> @@ -18,6 +18,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #include "smiapp.h"
>  #include "smiapp-regs.h"
> @@ -288,8 +289,12 @@ int smiapp_write_no_quirk(struct smiapp_sensor *sensor, 
> u32 reg, u32 val)
>   */
>  int smiapp_write(struct smiapp_sensor *sensor, u32 reg, u32 val)
>  {
> + struct i2c_client *client = v4l2_get_subdevdata(>src->sd);
>   int rval;
>  
> + if (pm_runtime_suspended(>dev))
> + return 0;
> +

This looks racy. What if idle countdown runs out immediately after
this check? If you can't call get_sync in this function you can
call pm_runtime_get() before the suspend check and pm_runtime_put
before returning from the function, so that the device keeps being
enabled.

Also I would expect some error code instead of success for early
return due to device being suspended?

>   rval = smiapp_call_quirk(sensor, reg_access, true, , );
>   if (rval == -ENOIOCTLCMD)
>   return 0;
>
> [...]

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 03/17] smiapp: Initialise media entity after sensor init

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:17PM +0300, Sakari Ailus wrote:
> This allows determining the number of pads in the entity based on the
> sensor.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> ---
>  drivers/media/i2c/smiapp/smiapp-core.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
> b/drivers/media/i2c/smiapp/smiapp-core.c
> index be74ba3..0a03f30 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -3056,12 +3056,7 @@ static int smiapp_probe(struct i2c_client *client,
>   sensor->src->sd.internal_ops = _internal_src_ops;
>   sensor->src->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>   sensor->src->sensor = sensor;
> -
>   sensor->src->pads[0].flags = MEDIA_PAD_FL_SOURCE;
> - rval = media_entity_pads_init(>src->sd.entity, 2,
> -  sensor->src->pads);
> - if (rval < 0)
> - return rval;
>  
>   if (client->dev.of_node) {
>   rval = smiapp_init(sensor);
> @@ -3069,6 +3064,11 @@ static int smiapp_probe(struct i2c_client *client,
>   goto out_media_entity_cleanup;
>   }
>  
> + rval = media_entity_pads_init(>src->sd.entity, 2,
> +  sensor->src->pads);
> + if (rval < 0)
> + goto out_media_entity_cleanup;
> +
>   rval = v4l2_async_register_subdev(>src->sd);
>   if (rval < 0)
>   goto out_media_entity_cleanup;

As far as I can see this is not strictly needed, but:

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 17/17] smiapp-pll: Don't complain aloud about failing PLL calculation

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:31PM +0300, Sakari Ailus wrote:
> Don't complain about a failure to compute the pre_pll divisor. The
> function is used to determine whether a particular combination of bits per
> sample value and a link frequency can be used, in which case there are
> lots of unnecessary driver messages. During normal operation the failure
> generally does not happen. Use dev_dbg() instead.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 16/17] smiapp: Drop a debug print on frame size and bit depth

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:30PM +0300, Sakari Ailus wrote:
> The first time the sensor is powered on, the information is not yet
> available.
> 
> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 15/17] smiapp: Obtain correct media bus code for try format

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:29PM +0300, Sakari Ailus wrote:
> The media bus code obtained for try format may have been a code that the
> sensor did not even support. Use a supported code with the current pixel
> order.

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 14/17] smiapp: Remove useless newlines and other small cleanups

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:28PM +0300, Sakari Ailus wrote:
> The code probably has been unindented at some point but rewrapping has not
> been done. Do it now.
> 
> Also remove a useless memory allocation failure message.

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 13/17] smiapp: Improve debug messages from frame layout reading

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:27PM +0300, Sakari Ailus wrote:
> Provide more debugging information on reading the frame layout.
> 
> [...]
>
> @@ -130,7 +127,7 @@ static int smiapp_read_frame_fmt(struct smiapp_sensor 
> *sensor)
>   pixels = desc & SMIAPP_FRAME_FORMAT_DESC_4_PIXELS_MASK;
>   } else {
>   dev_dbg(>dev,
> - "invalid frame format model type %d\n",
> + "0x8.8x invalid frame format model type %d\n",

0x8.8x doesn't look very interesting ;)

Apart from the '%' the actual argument is also missing.

> [...]

Once that is fixed:

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 09/17] smiapp: Read frame format earlier

[Sebastian Reichel]

Hi,

On Tue, Sep 20, 2016 at 12:19:54AM +0300, Sakari Ailus wrote:
> Hi Sebastian,
> 
> Sebastian Reichel wrote:
> > Hi,
> > 
> > On Thu, Sep 15, 2016 at 02:22:23PM +0300, Sakari Ailus wrote:
> > > The information gathered during frame format reading will be required
> > > earlier in the initialisation when it was available. Also return an error
> > > if frame format cannot be obtained.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> > > ---
> > >   drivers/media/i2c/smiapp/smiapp-core.c | 8 ++--
> > >   1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
> > > b/drivers/media/i2c/smiapp/smiapp-core.c
> > > index 0b5671c..c9aee83 100644
> > > --- a/drivers/media/i2c/smiapp/smiapp-core.c
> > > +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> > > @@ -2890,6 +2890,12 @@ static int smiapp_probe(struct i2c_client *client,
> > >   goto out_power_off;
> > >   }
> > > 
> > > + rval = smiapp_read_frame_fmt(sensor);
> > > + if (rval) {
> > > + rval = -ENODEV;
> > > + goto out_power_off;
> > > + }
> > > +
> > >   /*
> > >* Handle Sensor Module orientation on the board.
> > >*
> > > @@ -3013,8 +3019,6 @@ static int smiapp_probe(struct i2c_client *client,
> > > 
> > >   sensor->pixel_array->sd.entity.function = 
> > > MEDIA_ENT_F_CAM_SENSOR;
> > > 
> > > - /* final steps */
> > > - smiapp_read_frame_fmt(sensor);
> > >   rval = smiapp_init_controls(sensor);
> > >   if (rval < 0)
> > >   goto out_cleanup;
> > 
> > Is this missing a Fixes tag, or will it only be required earlier for
> > future patches?
> 
> It's primarily for future patches. Reading the frame format will require
> limits but hardly any other information. On the other hand, the frame format
> will very likely be needed elsewhere, hence the move.
> 
> The missing return value check is just a bug which I believe has been there
> since around 2011.

ok, so the move is for future patches. Then

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 12/17] smiapp: Obtain frame layout from the frame descriptor

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:26PM +0300, Sakari Ailus wrote:
> Besides the image data, SMIA++ compliant sensors also provide embedded
> data in form of registers used to capture the image. Store this
> information for later use in frame descriptor and routing.

Reviewed-By: Sebastian Reichel <s...@kernel.org>

> [...]
>
> + if (sensor->embedded_end > sensor->image_start)
> + sensor->image_start = sensor->embedded_end;

Maybe add a dev_dbg about sensor format information being broken?

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 11/17] smiapp: Use SMIAPP_PADS when referring to number of pads

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:25PM +0300, Sakari Ailus wrote:
> Replace plain value 2 with SMIAPP_PADS when referring to the number of
> pads.

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 10/17] smiapp: Unify setting up sub-devices

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:24PM +0300, Sakari Ailus wrote:
> The initialisation of the source sub-device is somewhat different as it's
> not created by the smiapp driver itself. Remove redundancy in initialising
> the two kind of sub-devices.

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 09/17] smiapp: Read frame format earlier

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:23PM +0300, Sakari Ailus wrote:
> The information gathered during frame format reading will be required
> earlier in the initialisation when it was available. Also return an error
> if frame format cannot be obtained.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/i2c/smiapp/smiapp-core.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
> b/drivers/media/i2c/smiapp/smiapp-core.c
> index 0b5671c..c9aee83 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -2890,6 +2890,12 @@ static int smiapp_probe(struct i2c_client *client,
>   goto out_power_off;
>   }
>  
> + rval = smiapp_read_frame_fmt(sensor);
> + if (rval) {
> + rval = -ENODEV;
> + goto out_power_off;
> + }
> +
>   /*
>* Handle Sensor Module orientation on the board.
>*
> @@ -3013,8 +3019,6 @@ static int smiapp_probe(struct i2c_client *client,
>  
>   sensor->pixel_array->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  
> - /* final steps */
> - smiapp_read_frame_fmt(sensor);
>   rval = smiapp_init_controls(sensor);
>   if (rval < 0)
>   goto out_cleanup;

Is this missing a Fixes tag, or will it only be required earlier for
future patches?

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 08/17] smiapp: Merge smiapp_init() with smiapp_probe()

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:22PM +0300, Sakari Ailus wrote:
> The smiapp_probe() is the sole caller of smiapp_init(). Unify the two.

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 04/17] smiapp: Split off sub-device registration into two

[Sebastian Reichel]

Hi,

On Mon, Sep 19, 2016 at 11:50:02PM +0300, Sakari Ailus wrote:
> Hi Sebastian,
> 
> Sebastian Reichel wrote:
> > Hi,
> > 
> > On Thu, Sep 15, 2016 at 02:22:18PM +0300, Sakari Ailus wrote:
> > > Remove the loop in sub-device registration and create each sub-device
> > > explicitly instead.
> > 
> > Reviewed-By: Sebastian Reichel <s...@kernel.org>
> 
> Thanks!
> 
> > 
> > > +static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
> > > +{
> > > + int rval;
> > > +
> > > + if (sensor->scaler) {
> > > + rval = smiapp_register_subdev(
> > > + sensor, sensor->binner, sensor->scaler,
> > > + SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
> > > + MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
> > > + if (rval < 0)
> > >   return rval;
> > > - }
> > >   }
> > > 
> > > - return 0;
> > > + return smiapp_register_subdev(
> > > + sensor, sensor->pixel_array, sensor->binner,
> > > + SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
> > > + MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
> > >   }
> > 
> > I haven't looked at the remaining code, but is sensor->scaler
> > stuff being cleaned up properly if the binner part fails?
> 
> That's a very good question. I don't think it is. But that's how
> the code has always been 

Yes, it's not a regression introduced by this patch, that's why I
gave Reviewed-By ;)

> --- there are issues left to be resolved if registered() fails for
> a reason or another. For instance, removing and reloading the
> omap3-isp module will cause a failure in the smiapp driver unless
> it's unloaded as well.
>
> I think I prefer to fix that in a different patch(set) as this one
> is quite large already.

ok.

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 07/17] smiapp: Always initialise the sensor in probe

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:21PM +0300, Sakari Ailus wrote:
> Initialise the sensor in probe. The reason why it wasn't previously done
> in case of platform data was that the probe() of the driver that provided
> the clock through the set_xclk() callback would need to finish before the
> probe() function of the smiapp driver. The set_xclk() callback no longer
> exists.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/i2c/smiapp/smiapp-core.c | 53 
> --
>  1 file changed, 19 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
> b/drivers/media/i2c/smiapp/smiapp-core.c
> index 5d251b4..13322f3 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -2530,8 +2530,19 @@ static int smiapp_register_subdev(struct smiapp_sensor 
> *sensor,
>   return 0;
>  }
>  
> -static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
> +static void smiapp_cleanup(struct smiapp_sensor *sensor)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(>src->sd);
> +
> + device_remove_file(>dev, _attr_nvm);
> + device_remove_file(>dev, _attr_ident);
> +
> + smiapp_free_controls(sensor);
> +}
> +
> +static int smiapp_registered(struct v4l2_subdev *subdev)
>  {
> + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
>   int rval;
>  
>   if (sensor->scaler) {
> @@ -2540,23 +2551,18 @@ static int smiapp_register_subdevs(struct 
> smiapp_sensor *sensor)
>   SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
>   MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
>   if (rval < 0)
> - return rval;
> + goto out_err;
>   }
>  
>   return smiapp_register_subdev(
>   sensor, sensor->pixel_array, sensor->binner,
>   SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
>   MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);

I guess you should also handle errors from the second
smiapp_register_subdev call?

> -}
>  
> -static void smiapp_cleanup(struct smiapp_sensor *sensor)
> -{
> - struct i2c_client *client = v4l2_get_subdevdata(>src->sd);
> -
> - device_remove_file(>dev, _attr_nvm);
> - device_remove_file(>dev, _attr_ident);
> +out_err:
> + smiapp_cleanup(sensor);
>  
> - smiapp_free_controls(sensor);
> + return rval;
>  }

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 06/17] smiapp: Remove unnecessary BUG_ON()'s

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:20PM +0300, Sakari Ailus wrote:
> Instead, calculate how much is needed and then allocate the memory
> dynamically.

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 05/17] smiapp: Provide a common function to obtain native pixel array size

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:19PM +0300, Sakari Ailus wrote:
> The same pixel array size is required for the active format of each
> sub-device sink pad and try format of each sink pad of each opened file
> handle as well as for the native size rectangle.

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 04/17] smiapp: Split off sub-device registration into two

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:18PM +0300, Sakari Ailus wrote:
> Remove the loop in sub-device registration and create each sub-device
> explicitly instead.

Reviewed-By: Sebastian Reichel <s...@kernel.org>

> +static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
> +{
> + int rval;
> +
> + if (sensor->scaler) {
> + rval = smiapp_register_subdev(
> + sensor, sensor->binner, sensor->scaler,
> + SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
> + MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
> + if (rval < 0)
>   return rval;
> - }
>   }
>  
> - return 0;
> + return smiapp_register_subdev(
> + sensor, sensor->pixel_array, sensor->binner,
> + SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
> + MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
>  }

I haven't looked at the remaining code, but is sensor->scaler
stuff being cleaned up properly if the binner part fails?

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v2 02/17] smiapp: Explicitly define number of pads in initialisation

[Sebastian Reichel]

Hi,

On Thu, Sep 15, 2016 at 02:22:16PM +0300, Sakari Ailus wrote:
> Define the number of pads explicitly in initialising the sub-devices.

Reviewed-By: Sebastian Reichel <s...@kernel.org>

-- Sebastian


signature.asc
Description: PGP signature