[no subject]
Good Day, I am Mr. Alfred Cheuk Yu Chow, the Director for Credit & Marketing Chong Hing Bank, Hong Kong, Chong Hing Bank Center, 24 Des Voeux Road Central, Hong Kong. I have a business proposal of $ 38,980,369.00. All confirmable documents to back up the claims will be made available to you prior to your acceptance and as soon as I receive your return mail. Best Regards, Alfred Chow.
Re: [PATCH v2 3/5] [RFT] ARM: dts: wheat: Fix ADV7513 address usage
Hi Kieran, I love your patch! Yet something to improve: [auto build test ERROR on linuxtv-media/master] [also build test ERROR on v4.16-rc1 next-20180213] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Kieran-Bingham/dt-bindings-media-adv7604-Add-support-for-i2c_new_secondary_device/20180214-032943 base: git://linuxtv.org/media_tree.git master config: arm-allyesconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm All errors (new ones prefixed by >>): >> Error: arch/arm/boot/dts/r8a7792-wheat.dts:251.24-25 syntax error FATAL ERROR: Unable to parse input tree --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH 1/2] dt-bindings: media: Binding document for OV7251 camera sensor
On Thu, Feb 08, 2018 at 10:53:37AM +0200, Todor Tomov wrote:
> Add the document for ov7251 device tree binding.
>
> CC: Rob Herring
> CC: Mark Rutland
> CC: devicet...@vger.kernel.org
> Signed-off-by: Todor Tomov
> ---
> .../devicetree/bindings/media/i2c/ov7251.txt | 51
> ++
> 1 file changed, 51 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov7251.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov7251.txt
> b/Documentation/devicetree/bindings/media/i2c/ov7251.txt
> new file mode 100644
> index 000..c807646
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov7251.txt
> @@ -0,0 +1,51 @@
> +* Omnivision 1/7.5-Inch B&W VGA CMOS Digital Image Sensor
> +
> +The Omnivision OV7251 is a 1/7.5-Inch CMOS active pixel digital image sensor
> with
> +an active array size of 640H x 480V. It is programmable through a serial I2C
> +interface.
> +
> +Required Properties:
> +- compatible: Value should be "ovti,ov7251".
> +- clocks: Reference to the xclk clock.
> +- clock-names: Should be "xclk".
> +- clock-frequency: Frequency of the xclk clock.
> +- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This
> corresponds
> + to the hardware pin XSHUTDOWN which is physically active low.
> +- vdddo-supply: Chip digital IO regulator.
> +- vdda-supply: Chip analog regulator.
> +- vddd-supply: Chip digital core regulator.
> +
> +The device node must contain one 'port' child node for its digital output
> +video port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> + &i2c1 {
> + ...
> +
> + ov7251: ov7251@60 {
camera-sensor@60
With that,
Reviewed-by: Rob Herring
> + compatible = "ovti,ov7251";
> + reg = <0x60>;
> +
> + enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&camera_bw_default>;
> +
> + clocks = <&clks 200>;
> + clock-names = "xclk";
> + clock-frequency = <2400>;
> +
> + vdddo-supply = <&camera_dovdd_1v8>;
> + vdda-supply = <&camera_avdd_2v8>;
> + vddd-supply = <&camera_dvdd_1v2>;
> +
> + port {
> + ov7251_ep: endpoint {
> + clock-lanes = <1>;
> + data-lanes = <0>;
> + remote-endpoint = <&csi0_ep>;
> + };
> + };
> + };
> + };
> --
> 2.7.4
>
Re: [PATCH v3 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 TX Device Tree bindings
Hi Laurent, On Thu, Feb 08, 2018 at 09:00:19PM +0200, Laurent Pinchart wrote: > Hi Maxime, > > Thank you for the patch. > > On Wednesday, 7 February 2018 16:26:42 EET Maxime Ripard wrote: > > The Cadence MIPI-CSI2 TX controller is a CSI2 bridge that supports up to 4 > > video streams and can output on up to 4 CSI-2 lanes, depending on the > > hardware implementation. > > > > It can operate with an external D-PHY, an internal one or no D-PHY at all > > in some configurations. > > > > Acked-by: Rob Herring > > Signed-off-by: Maxime Ripard > > --- > > .../devicetree/bindings/media/cdns,csi2tx.txt | 98 +++ > > 1 file changed, 98 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/cdns,csi2tx.txt > > > > diff --git a/Documentation/devicetree/bindings/media/cdns,csi2tx.txt > > b/Documentation/devicetree/bindings/media/cdns,csi2tx.txt new file mode > > 100644 > > index ..acbbd625a75f > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/cdns,csi2tx.txt > > @@ -0,0 +1,98 @@ > > +Cadence MIPI-CSI2 TX controller > > +=== > > + > > +The Cadence MIPI-CSI2 TX controller is a CSI-2 bridge supporting up to > > +4 CSI lanes in output, and up to 4 different pixel streams in input. > > + > > +Required properties: > > + - compatible: must be set to "cdns,csi2tx" > > + - reg: base address and size of the memory mapped region > > + - clocks: phandles to the clocks driving the controller > > + - clock-names: must contain: > > +* esc_clk: escape mode clock > > +* p_clk: register bank clock > > +* pixel_if[0-3]_clk: pixel stream output clock, one for each stream > > + implemented in hardware, between 0 and 3 > > + > > +Optional properties > > + - phys: phandle to the D-PHY. If it is set, phy-names need to be set > > + - phy-names: must contain dphy > > Nitpicking, I'd write "dphy" with double quotes. > > > +Required subnodes: > > + - ports: A ports node with one port child node per device input and > > output > > + port, in accordance with the video interface bindings defined in > > + Documentation/devicetree/bindings/media/video-interfaces.txt. > > The > > + port nodes numbered as follows. > > s/numbered/are numbered/ > > > + > > + Port Description > > + - > > + 0CSI-2 output > > + 1Stream 0 input > > + 2Stream 1 input > > + 3Stream 2 input > > + 4Stream 3 input > > + > > + The stream input port nodes are optional if they are not > > + connected to anything at the hardware level or implemented > > + in the design. > > Are they optional (and thus valid if present), or should they be forbidden in > case they're not implemented in the hardware ? I'd go for the latter and write > > "One stream input port node is required per implemented hardware input, and > no > stream input port node can be present for unimplemented inputs." > > > Since there is only one endpoint per port, > > + the endpoints are not numbered. > > I think it would be valid to number endpoints even if not required. I think > that what you should document is that at most one endpoint is supported per > port. If you allow them to be numbered, then the driver must be able to use any endpoint number. This provides no additional information on the hardware while it is more difficult to parse. That would require reg property in endpoints --- which is not defined here. I think all new drivers pretty much define it in a similar way (while the old definitions did not specify it explicitly). -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
this is what you need
Hi, I wanted to check in with you, did you receive my email from last week? I want to share a proven system with you. This system allows you to try the whole thing for f.r_ee for 30 days. You can finally change your future without giving up any sensitive information in advance. I s-ig-ned up myself just a while ago and I'm already making more than in my regular nine to five job that I plan on quitting any day now. Despite of this, this is probably the best thing that ever happened to you if you take action now. Please reply if interested. Thanks, Peter
Re: [PATCH V2 3/3] media: dvb-core: Added documentation for ca sysfs timer nodes
Hi! Please hold on in merging this series, because I have to investigate a hint I got related to the buffer size handshake of the protocol driver: https://www.linuxtv.org/pipermail/linux-dvb/2007-July/019116.html BR, Jasmin On 12/21/2017 02:22 PM, Jasmin J. wrote: > From: Jasmin Jessich > > Added the documentation for the new ca? sysfs nodes in > /sys/class/dvb/dvb?/ca?/tim_wr_. > > Signed-off-by: Jasmin Jessich > --- > Documentation/ABI/testing/sysfs-class-ca| 63 ++ > Documentation/media/uapi/dvb/ca-sysfs-nodes.rst | 85 > + > Documentation/media/uapi/dvb/ca.rst | 1 + > 3 files changed, 149 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-ca > create mode 100644 Documentation/media/uapi/dvb/ca-sysfs-nodes.rst > > diff --git a/Documentation/ABI/testing/sysfs-class-ca > b/Documentation/ABI/testing/sysfs-class-ca > new file mode 100644 > index 000..7a2a52c > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-ca > @@ -0,0 +1,63 @@ > +What:/sys/class/dvb/dvbN/ > +Date:Dec 2017 > +KernelVersion: 4.15 > +Contact: Jasmin Jessich > +Description: > +The dvbN/ class sub-directory belongs to the Adapter with the > +index N. It is created for each found Adapter and depends on > +the DVB hardware. > + > +What:/sys/class/dvb/dvbN/caM > +Date:Dec 2017 > +KernelVersion: 4.15 > +Contact: Jasmin Jessich > +Description: > +The dvbN/caM/ class sub-directory belongs to the CA device with > +the index M on the Adapter with the index N. It is created for > +each found Conditional Access Interface where M is the number > +of the CA Interface. > + > +What:/sys/class/dvb/dvbN/caM/tim_wr_high > +Date:Dec 2017 > +KernelVersion: 4.15 > +Contact: Jasmin Jessich > +Description: > +Reading this file returns the wait time after writing the > +length high byte to the CAM. The default timeout it '0', which > +means no 'no timeout'. Any other value specifies the timeout in > +micro seconds. > + > +Writing a value will change the timeout. > + > +Write fails with ``EINVAL`` if an invalid value has been written > +(valid values are 0..10). > + > +What:/sys/class/dvb/dvbN/caM/tim_wr_low > +Date:Dec 2017 > +KernelVersion: 4.15 > +Contact: Jasmin Jessich > +Description: > +Reading this file returns the wait time after writing the > +length low byte to the CAM. The default timeout it '0', which > +means no 'no timeout'. Any other value specifies the timeout in > +micro seconds. > + > +Writing a value will change the timeout. > + > +Write fails with ``EINVAL`` if an invalid value has been written > +(valid values are 0..10). > + > +What:/sys/class/dvb/dvbN/caM/tim_wr_data > +Date:Dec 2017 > +KernelVersion: 4.15 > +Contact: Jasmin Jessich > +Description: > +Reading this file returns the wait time between data bytes sent > +to the CAM. The default timeout it '0', which means no 'no timeout'. > +Any other value specifies the timeout in micro seconds. > + > +Writing a value will change the timeout. > + > +Write fails with ``EINVAL`` if an invalid value has been written > +(valid values are 0..10). > + > diff --git a/Documentation/media/uapi/dvb/ca-sysfs-nodes.rst > b/Documentation/media/uapi/dvb/ca-sysfs-nodes.rst > new file mode 100644 > index 000..4a26afd > --- /dev/null > +++ b/Documentation/media/uapi/dvb/ca-sysfs-nodes.rst > @@ -0,0 +1,85 @@ > +.. -*- coding: utf-8; mode: rst -*- > + > +.. _ca_sysfs_nodes: > + > +** > +Conditional Access sysfs nodes > +** > + > +As defined at ``Documentation/ABI/testing/sysfs-class-ca``, those are > +the sysfs nodes that control the en50221 CA driver: > + > + > +.. _sys_class_dvb_dvbN: > + > +/sys/class/dvb/dvbN/ > + > + > +The ``/sys/class/dvb/dvbN/`` class sub-directory belongs to the Adapter > +with the index N. It is created for each found Adapter and depends on the > +DVB hardware. > + > + > +.. _sys_class_dvb_dvbN_caM: > + > +/sys/class/dvb/dvbN/caM > +=== > + > +The ``/sys/class/dvb/dvbN/caM`` class sub-directory belongs to the CA device > +with the index M on the Adapter with the index N. It is created for each > +found Conditional Access Interface where M is the number of the CA Interface. > +A Conditional Access Module (CAM) will be inserted into the CI interface. The > +caM device is used to communicate to the CAM. > + > +The communication protocol contains a length field followed by the dat
Re: [PATCH V2 1/3] media: dvb-core: Store device structure in dvb_register_device
Hi!
Please hold on in merging this series, because I have to investigate a hint
I got related to the buffer size handshake of the protocol driver:
https://www.linuxtv.org/pipermail/linux-dvb/2007-July/019116.html
BR,
Jasmin
On 12/21/2017 02:22 PM, Jasmin J. wrote:
> From: Jasmin Jessich
>
> The device created by device_create in dvb_register_device was not
> available for DVB device drivers.
> Added "struct device *dev" to "struct dvb_device" and store the created
> device.
>
> Signed-off-by: Jasmin Jessich
> Acked-by: Ralph Metzler
> ---
> drivers/media/dvb-core/dvbdev.c | 1 +
> drivers/media/dvb-core/dvbdev.h | 4 +++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index 060c60d..f55eff1 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -538,6 +538,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct
> dvb_device **pdvbdev,
> __func__, adap->num, dnames[type], id, PTR_ERR(clsdev));
> return PTR_ERR(clsdev);
> }
> + dvbdev->dev = clsdev;
> dprintk("DVB: register adapter%d/%s%d @ minor: %i (0x%02x)\n",
> adap->num, dnames[type], id, minor, minor);
>
> diff --git a/drivers/media/dvb-core/dvbdev.h b/drivers/media/dvb-core/dvbdev.h
> index bbc1c20..1f2d2ff 100644
> --- a/drivers/media/dvb-core/dvbdev.h
> +++ b/drivers/media/dvb-core/dvbdev.h
> @@ -147,10 +147,11 @@ struct dvb_adapter {
> * @tsout_num_entities: Number of Transport Stream output entities
> * @tsout_entity: array with MC entities associated to each TS output node
> * @tsout_pads: array with the source pads for each @tsout_entity
> + * @dev: pointer to struct device that is associated with the dvb device
> *
> * This structure is used by the DVB core (frontend, CA, net, demux) in
> * order to create the device nodes. Usually, driver should not initialize
> - * this struct diretly.
> + * this struct directly.
> */
> struct dvb_device {
> struct list_head list_head;
> @@ -183,6 +184,7 @@ struct dvb_device {
> #endif
>
> void *priv;
> + struct device *dev;
> };
>
> /**
>
Re: [PATCH V2 2/3] media: dvb-core: Added timers for dvb_ca_en50221_write_data
Hi!
Please hold on in merging this series, because I have to investigate a hint
I got related to the buffer size handshake of the protocol driver:
https://www.linuxtv.org/pipermail/linux-dvb/2007-July/019116.html
BR,
Jasmin
On 12/21/2017 02:22 PM, Jasmin J. wrote:
> From: Jasmin Jessich
>
> Some (older) CAMs are really slow in accepting data. The CI interface
> specification doesn't define a handshake for accepted data. Thus, the
> en50221 protocol driver can't control if a data byte has been correctly
> written to the CAM.
>
> The current implementation writes the length and the data quick after
> each other. Thus, the slow CAMs may generate a WR error, which leads to
> the known error logging
>"CAM tried to send a buffer larger than the ecount size".
>
> To solve this issue the en50221 protocol driver needs to wait some CAM
> depending time between the different bytes to be written. Because the
> time is CAM dependent, an individual value per CAM needs to be set. For
> that SysFS is used in favor of ioctl's to allow the control of the timer
> values independent from any user space application.
>
> This patch adds the timers and the SysFS nodes to set/get the timeout
> values and the timer waiting between the different steps of the CAM write
> access. A timer value of 0 (default) means "no timeout".
>
> Signed-off-by: Jasmin Jessich
> Acked-by: Ralph Metzler
> ---
> drivers/media/dvb-core/dvb_ca_en50221.c | 132
> +++-
> 1 file changed, 131 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c
> b/drivers/media/dvb-core/dvb_ca_en50221.c
> index a3b2754..9b45d6b 100644
> --- a/drivers/media/dvb-core/dvb_ca_en50221.c
> +++ b/drivers/media/dvb-core/dvb_ca_en50221.c
> @@ -86,6 +86,13 @@ MODULE_PARM_DESC(cam_debug, "enable verbose debug
> messages");
> #define DVB_CA_SLOTSTATE_WAITFR 6
> #define DVB_CA_SLOTSTATE_LINKINIT 7
>
> +enum dvb_ca_timers {
> + DVB_CA_TIM_WR_HIGH /* wait after writing length high */
> +,DVB_CA_TIM_WR_LOW /* wait after writing length low */
> +,DVB_CA_TIM_WR_DATA /* wait between data bytes */
> +,DVB_CA_TIM_MAX
> +};
> +
> /* Information on a CA slot */
> struct dvb_ca_slot {
> /* current state of the CAM */
> @@ -119,6 +126,11 @@ struct dvb_ca_slot {
> unsigned long timeout;
> };
>
> +struct dvb_ca_timer {
> + unsigned long min;
> + unsigned long max;
> +};
> +
> /* Private CA-interface information */
> struct dvb_ca_private {
> struct kref refcount;
> @@ -161,6 +173,14 @@ struct dvb_ca_private {
>
> /* mutex serializing ioctls */
> struct mutex ioctl_mutex;
> +
> + struct dvb_ca_timer timers[DVB_CA_TIM_MAX];
> +};
> +
> +static const char dvb_ca_tim_names[DVB_CA_TIM_MAX][15] = {
> + "tim_wr_high"
> +,"tim_wr_low"
> +,"tim_wr_data"
> };
>
> static void dvb_ca_private_free(struct dvb_ca_private *ca)
> @@ -223,6 +243,14 @@ static char *findstr(char *haystack, int hlen, char
> *needle, int nlen)
> return NULL;
> }
>
> +static void dvb_ca_sleep(struct dvb_ca_private *ca, enum dvb_ca_timers tim)
> +{
> + unsigned long min = ca->timers[tim].min;
> +
> + if (min)
> + usleep_range(min, ca->timers[tim].max);
> +}
> +
> /*
> ** */
> /* EN50221 physical interface functions */
>
> @@ -869,10 +897,13 @@ static int dvb_ca_en50221_write_data(struct
> dvb_ca_private *ca, int slot,
> bytes_write >> 8);
> if (status)
> goto exit;
> + dvb_ca_sleep(ca, DVB_CA_TIM_WR_HIGH);
> +
> status = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_SIZE_LOW,
> bytes_write & 0xff);
> if (status)
> goto exit;
> + dvb_ca_sleep(ca, DVB_CA_TIM_WR_LOW);
>
> /* send the buffer */
> for (i = 0; i < bytes_write; i++) {
> @@ -880,6 +911,7 @@ static int dvb_ca_en50221_write_data(struct
> dvb_ca_private *ca, int slot,
> buf[i]);
> if (status)
> goto exit;
> + dvb_ca_sleep(ca, DVB_CA_TIM_WR_DATA);
> }
>
> /* check for write error (WE should now be 0) */
> @@ -1834,6 +1866,97 @@ static const struct dvb_device dvbdev_ca = {
> };
>
> /*
> ** */
> +/* EN50221 device attributes (SysFS) */
> +
> +static int dvb_ca_tim_idx(struct dvb_ca_private *ca, const char *name)
> +{
> + int tim_idx;
> +
> + for (tim_idx = 0; tim_idx < DVB_CA_TIM_MAX; tim_idx++) {
> + if (!strcmp(dvb_ca_tim_names[tim_idx], name))
> + return tim_idx;
> + }
> + return -1;
> +}
> +
> +static ssize_t dvb_ca_tim_show(struct device *device,
> +struct device_attribute *
Re: [PATCH V2 0/3] Add timers to en50221 protocol driver
Hi! Please hold on in merging this series, because I have to investigate a hint I got related to the buffer size handshake of the protocol driver: https://www.linuxtv.org/pipermail/linux-dvb/2007-July/019116.html BR, Jasmin On 12/21/2017 02:22 PM, Jasmin J. wrote: > From: Jasmin Jessich > > Some (older) CAMs are really slow in accepting data. I got sometimes the > already known error "CAM tried to send a buffer larger than the ecount > size". I could track it down to the dvb_ca_en50221_write_data function not > waiting between sending the data length high/low and data bytes. In fact > the CAM reported a WR error, which triggered later on the mentioned error. > > The problem is that a simple module parameter can't be used to solve this > by adding timer values, because the protocol handler is used for any CI > interface. A module parameter would be influence all the CAMs on all CI > interfaces. Thus individual timer definitions per CI interface and CAM are > required. > There are two possibilities to implement that, ioctl's and SysFS. > ioctl's require changes in usermode programs and it may take a lot of time > to get this implemented there. > SysFS can be used by simple "cat" and "echo" commands and can be therefore > simply controlled by scripting, which is immediately available. > > I decided to go for the SysFS approach, but the required device to add the > SysFS files was not available in the "struct dvb_device". The first patch > of this series adds this device to the structure and also the setting code. > > The second patch adds the functions to create the SysFS nodes for all > timers and the new timeouts in the en50221 protocol driver. > > The third patch adds the SysFS node documentation. > > Jasmin Jessich (3): > media: dvb-core: Store device structure in dvb_register_device > media: dvb-core: Added timers for dvb_ca_en50221_write_data > media: dvb-core: Added documentation for ca sysfs timer nodes > > Documentation/ABI/testing/sysfs-class-ca| 63 +++ > Documentation/media/uapi/dvb/ca-sysfs-nodes.rst | 85 +++ > Documentation/media/uapi/dvb/ca.rst | 1 + > drivers/media/dvb-core/dvb_ca_en50221.c | 132 > +++- > drivers/media/dvb-core/dvbdev.c | 1 + > drivers/media/dvb-core/dvbdev.h | 4 +- > 6 files changed, 284 insertions(+), 2 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-class-ca > create mode 100644 Documentation/media/uapi/dvb/ca-sysfs-nodes.rst >
Re: [PATCH v10 10/30] rcar-vin: fix handling of single field frames (top, bottom and alternate fields)
Hi Niklas,
On Wednesday, 14 February 2018 01:12:50 EET Niklas Söderlund wrote:
> On 2018-02-14 00:31:21 +0200, Laurent Pinchart wrote:
> > On Tuesday, 13 February 2018 18:47:04 EET Niklas Söderlund wrote:
> >> On 2018-02-13 18:26:34 +0200, Laurent Pinchart wrote:
> >>> On Monday, 29 January 2018 18:34:15 EET Niklas Söderlund wrote:
> There was never proper support in the VIN driver to deliver
> ALTERNATING field format to user-space, remove this field option. The
> problem is that ALTERNATING filed order requires the sequence numbers
> of buffers returned to userspace to reflect if fields where dropped or
> not, something which is not possible with the VIN drivers capture
> logic.
>
> The VIN driver can still capture from a video source which delivers
> frames in ALTERNATING field order, but needs to combine them using
> the VIN hardware into INTERLACED field order. Before this change if a
> source was delivering fields using ALTERNATE the driver would default
> to combining them using this hardware feature. Only if the user
> explicitly requested ALTERNATE filed order would incorrect frames be
> delivered.
>
> The height should not be cut in half for the format for TOP or BOTTOM
> fields settings. This was a mistake and it was made visible by the
> scaling refactoring. Correct behavior is that the user should request
> a frame size that fits the half height frame reflected in the field
> setting. If not the VIN will do its best to scale the top or bottom
> to the requested format and cropping and scaling do not work as
> expected.
>
> Signed-off-by: Niklas Söderlund
>
> Reviewed-by: Hans Verkuil
> ---
>
> drivers/media/platform/rcar-vin/rcar-dma.c | 15 +---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 53
> +++
> 2 files changed, 24 insertions(+), 44 deletions(-)
> >
> > [snip]
> >
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 4d5be2d0c79c9c9a..9f7902d29c62e205 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -103,6 +103,28 @@ static int rvin_get_source_format(struct
> rvin_dev *vin,
> if (ret)
> return ret;
>
> +switch (fmt.format.field) {
> +case V4L2_FIELD_TOP:
> +case V4L2_FIELD_BOTTOM:
> +case V4L2_FIELD_NONE:
> +case V4L2_FIELD_INTERLACED_TB:
> +case V4L2_FIELD_INTERLACED_BT:
> +case V4L2_FIELD_INTERLACED:
> +break;
> +case V4L2_FIELD_ALTERNATE:
> +/*
> + * Driver do not (yet) support outputting ALTERNATE to a
> + * userspace. It dose support outputting INTERLACED so
> use
> >>>
> >>> s/dose/does/
> >>>
> + * the VIN hardware to combine the two fields.
> + */
> +fmt.format.field = V4L2_FIELD_INTERLACED;
> +fmt.format.height *= 2;
> +break;
> >>>
> >>> I don't like this much. The rvin_get_source_format() function is
> >>> supposed to return the media bus format for the bus between the source
> >>> and the VIN. It's the caller that should take the field limitations into
> >>> account, otherwise you end up with a mix of source and VIN data in the
> >>> same structure.
> >>
> >> When I read your comments I understand your argument better. And I
> >> understand this function is perhaps poorly named. Maybe it should be
> >> renamed to rvin_get_vin_format_from_source().
> >
> > If you add a comment above the function I could live with that. Would it
> > make sense to pass a v4l2_pix_format structure instead of a
> > v4l2_mbus_framefmt ?
>
> I now see that the function name is misleading and I will change it as
> per above. I will also add a comment and swap to v4l2_pix_format (which
> was used before v10 but was changed due to your review comments, I'm
> happy you come around :-)
The argument type has to be consistent with the function's purpose and name.
Now that you propose changing the function's purpose, my previous comments
have to be updated. And I'm annoyed that you have such a good memory, it
forces me to invent excuses :-)
> >> The source format is fetched at s_stream() time in order to do format
> >> validation. At this time the field is also taken into account once more
> >> to validate that the VIN format (calculated here) still is valid. It
> >> also handles the question you ask later at s_stream() time, see bellow.
> >>
> +default:
> +vin->format.field = V4L2_FIELD_NONE;
> +break;
> +}
> +
> memcp
Re: [PATCH v10 10/30] rcar-vin: fix handling of single field frames (top, bottom and alternate fields)
Hi Laurent,
On 2018-02-14 00:31:21 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> On Tuesday, 13 February 2018 18:47:04 EET Niklas Söderlund wrote:
> > On 2018-02-13 18:26:34 +0200, Laurent Pinchart wrote:
> > > On Monday, 29 January 2018 18:34:15 EET Niklas Söderlund wrote:
> > >> There was never proper support in the VIN driver to deliver ALTERNATING
> > >> field format to user-space, remove this field option. The problem is
> > >> that ALTERNATING filed order requires the sequence numbers of buffers
> > >> returned to userspace to reflect if fields where dropped or not,
> > >> something which is not possible with the VIN drivers capture logic.
> > >>
> > >> The VIN driver can still capture from a video source which delivers
> > >> frames in ALTERNATING field order, but needs to combine them using the
> > >> VIN hardware into INTERLACED field order. Before this change if a source
> > >> was delivering fields using ALTERNATE the driver would default to
> > >> combining them using this hardware feature. Only if the user explicitly
> > >> requested ALTERNATE filed order would incorrect frames be delivered.
> > >>
> > >> The height should not be cut in half for the format for TOP or BOTTOM
> > >> fields settings. This was a mistake and it was made visible by the
> > >> scaling refactoring. Correct behavior is that the user should request a
> > >> frame size that fits the half height frame reflected in the field
> > >> setting. If not the VIN will do its best to scale the top or bottom to
> > >> the requested format and cropping and scaling do not work as expected.
> > >>
> > >> Signed-off-by: Niklas Söderlund
> > >> Reviewed-by: Hans Verkuil
> > >> ---
> > >>
> > >> drivers/media/platform/rcar-vin/rcar-dma.c | 15 +---
> > >> drivers/media/platform/rcar-vin/rcar-v4l2.c | 53 +++
> > >> 2 files changed, 24 insertions(+), 44 deletions(-)
>
> [snip]
>
> > >> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > >> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > >> 4d5be2d0c79c9c9a..9f7902d29c62e205 100644
> > >> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > >> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > >> @@ -103,6 +103,28 @@ static int rvin_get_source_format(struct rvin_dev
> > >> *vin,
> > >> if (ret)
> > >> return ret;
> > >>
> > >> +switch (fmt.format.field) {
> > >> +case V4L2_FIELD_TOP:
> > >> +case V4L2_FIELD_BOTTOM:
> > >> +case V4L2_FIELD_NONE:
> > >> +case V4L2_FIELD_INTERLACED_TB:
> > >> +case V4L2_FIELD_INTERLACED_BT:
> > >> +case V4L2_FIELD_INTERLACED:
> > >> +break;
> > >> +case V4L2_FIELD_ALTERNATE:
> > >> +/*
> > >> + * Driver do not (yet) support outputting ALTERNATE to a
> > >> + * userspace. It dose support outputting INTERLACED so
> > >> use
> > >
> > > s/dose/does/
> > >
> > >> + * the VIN hardware to combine the two fields.
> > >> + */
> > >> +fmt.format.field = V4L2_FIELD_INTERLACED;
> > >> +fmt.format.height *= 2;
> > >> +break;
> > >
> > > I don't like this much. The rvin_get_source_format() function is supposed
> > > to return the media bus format for the bus between the source and the
> > > VIN. It's the caller that should take the field limitations into account,
> > > otherwise you end up with a mix of source and VIN data in the same
> > > structure.
> >
> > When I read your comments I understand your argument better. And I
> > understand this function is perhaps poorly named. Maybe it should be
> > renamed to rvin_get_vin_format_from_source().
>
> If you add a comment above the function I could live with that. Would it make
> sense to pass a v4l2_pix_format structure instead of a v4l2_mbus_framefmt ?
I now see that the function name is misleading and I will change it as
per above. I will also add a comment and swap to v4l2_pix_format (which
was used before v10 but was changed due to your review comments, I'm
happy you come around :-)
>
> > The source format is fetched at s_stream() time in order to do format
> > validation. At this time the field is also taken into account once more
> > to validate that the VIN format (calculated here) still is valid. It
> > also handles the question you ask later at s_stream() time, see bellow.
> >
> > >> +default:
> > >> +vin->format.field = V4L2_FIELD_NONE;
> > >> +break;
> > >> +}
> > >> +
> > >> memcpy(mbus_fmt, &fmt.format, sizeof(*mbus_fmt));
> > >>
> > >> return 0;
> > >> @@ -139,33 +161,6 @@ static int rvin_reset_format(struct rvin_dev *vin)
> > >>
> > >> v4l2_fill_pix_format(&vin->format, &source_fmt);
> > >>
> > >> -/*
> > >> - * If the subdevice uses ALTERNATE field mode and G_STD is
> > >> - * implemented use the
Re: [PATCH] videodev2.h: add helper to validate colorspace
Hi Laurent and Niklas,
On Wed, Feb 14, 2018 at 12:23:05AM +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Wednesday, 14 February 2018 00:08:47 EET Niklas Söderlund wrote:
> > There is no way for drivers to validate a colorspace value, which could
> > be provided by user-space by VIDIOC_S_FMT for example. Add a helper to
> > validate that the colorspace value is part of enum v4l2_colorspace.
> >
> > Signed-off-by: Niklas Söderlund
> > ---
> > include/uapi/linux/videodev2.h | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > Hi,
> >
> > I hope this is the correct header to add this helper to. I think it's
> > since if it's in uapi not only can v4l2 drivers use it but tools like
> > v4l-compliance gets access to it and can be updated to use this instead
> > of the hard-coded check of just < 0xff as it was last time I checked.
> >
> > // Niklas
> >
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 9827189651801e12..843afd7c5b000553 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -238,6 +238,11 @@ enum v4l2_colorspace {
> > V4L2_COLORSPACE_DCI_P3= 12,
> > };
> >
> > +/* Determine if a colorspace is defined in enum v4l2_colorspace */
> > +#define V4L2_COLORSPACE_IS_VALID(colorspace) \
> > + (((colorspace) >= V4L2_COLORSPACE_DEFAULT) && \
> > +((colorspace) <= V4L2_COLORSPACE_DCI_P3))
> > +
>
> This looks pretty good to me. I'd remove the parentheses around each test
> though.
Agreed.
>
> One potential issue is that if this macro operates on an unsigned value (for
> instance an u32, which is the type used for the colorspace field in various
> structures) the compiler will generate a warning:
>
> enum.c: In function ‘test_4’:
>
>
> enum.c:30:16: warning: comparison of unsigned expression >= 0 is always true
> [-Wtype-limits]
>
> return V4L2_COLORSPACE_IS_VALID(colorspace);
>
> Dropping the first check would fix that, but wouldn't catch invalid values
> when operating on a signed type, such as int or enum v4l2_colorspace.
How about simply casting it to u32 first (and removing the first test)?
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v10 10/30] rcar-vin: fix handling of single field frames (top, bottom and alternate fields)
Hi Niklas,
On Tuesday, 13 February 2018 18:47:04 EET Niklas Söderlund wrote:
> On 2018-02-13 18:26:34 +0200, Laurent Pinchart wrote:
> > On Monday, 29 January 2018 18:34:15 EET Niklas Söderlund wrote:
> >> There was never proper support in the VIN driver to deliver ALTERNATING
> >> field format to user-space, remove this field option. The problem is
> >> that ALTERNATING filed order requires the sequence numbers of buffers
> >> returned to userspace to reflect if fields where dropped or not,
> >> something which is not possible with the VIN drivers capture logic.
> >>
> >> The VIN driver can still capture from a video source which delivers
> >> frames in ALTERNATING field order, but needs to combine them using the
> >> VIN hardware into INTERLACED field order. Before this change if a source
> >> was delivering fields using ALTERNATE the driver would default to
> >> combining them using this hardware feature. Only if the user explicitly
> >> requested ALTERNATE filed order would incorrect frames be delivered.
> >>
> >> The height should not be cut in half for the format for TOP or BOTTOM
> >> fields settings. This was a mistake and it was made visible by the
> >> scaling refactoring. Correct behavior is that the user should request a
> >> frame size that fits the half height frame reflected in the field
> >> setting. If not the VIN will do its best to scale the top or bottom to
> >> the requested format and cropping and scaling do not work as expected.
> >>
> >> Signed-off-by: Niklas Söderlund
> >> Reviewed-by: Hans Verkuil
> >> ---
> >>
> >> drivers/media/platform/rcar-vin/rcar-dma.c | 15 +---
> >> drivers/media/platform/rcar-vin/rcar-v4l2.c | 53 +++
> >> 2 files changed, 24 insertions(+), 44 deletions(-)
[snip]
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> >> 4d5be2d0c79c9c9a..9f7902d29c62e205 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >> @@ -103,6 +103,28 @@ static int rvin_get_source_format(struct rvin_dev
> >> *vin,
> >>if (ret)
> >>return ret;
> >>
> >> + switch (fmt.format.field) {
> >> + case V4L2_FIELD_TOP:
> >> + case V4L2_FIELD_BOTTOM:
> >> + case V4L2_FIELD_NONE:
> >> + case V4L2_FIELD_INTERLACED_TB:
> >> + case V4L2_FIELD_INTERLACED_BT:
> >> + case V4L2_FIELD_INTERLACED:
> >> + break;
> >> + case V4L2_FIELD_ALTERNATE:
> >> + /*
> >> + * Driver do not (yet) support outputting ALTERNATE to a
> >> + * userspace. It dose support outputting INTERLACED so use
> >
> > s/dose/does/
> >
> >> + * the VIN hardware to combine the two fields.
> >> + */
> >> + fmt.format.field = V4L2_FIELD_INTERLACED;
> >> + fmt.format.height *= 2;
> >> + break;
> >
> > I don't like this much. The rvin_get_source_format() function is supposed
> > to return the media bus format for the bus between the source and the
> > VIN. It's the caller that should take the field limitations into account,
> > otherwise you end up with a mix of source and VIN data in the same
> > structure.
>
> When I read your comments I understand your argument better. And I
> understand this function is perhaps poorly named. Maybe it should be
> renamed to rvin_get_vin_format_from_source().
If you add a comment above the function I could live with that. Would it make
sense to pass a v4l2_pix_format structure instead of a v4l2_mbus_framefmt ?
> The source format is fetched at s_stream() time in order to do format
> validation. At this time the field is also taken into account once more
> to validate that the VIN format (calculated here) still is valid. It
> also handles the question you ask later at s_stream() time, see bellow.
>
> >> + default:
> >> + vin->format.field = V4L2_FIELD_NONE;
> >> + break;
> >> + }
> >> +
> >>memcpy(mbus_fmt, &fmt.format, sizeof(*mbus_fmt));
> >>
> >>return 0;
> >> @@ -139,33 +161,6 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >>
> >>v4l2_fill_pix_format(&vin->format, &source_fmt);
> >>
> >> - /*
> >> - * If the subdevice uses ALTERNATE field mode and G_STD is
> >> - * implemented use the VIN HW to combine the two fields to
> >> - * one INTERLACED frame. The ALTERNATE field mode can still
> >> - * be requested in S_FMT and be respected, this is just the
> >> - * default which is applied at probing or when S_STD is called.
> >> - */
> >> - if (vin->format.field == V4L2_FIELD_ALTERNATE &&
> >> - v4l2_subdev_has_op(vin_to_source(vin), video, g_std))
> >> - vin->format.field = V4L2_FIELD_INTERLACED;
> >> -
> >> - switch (vin->format.field) {
> >> - case V4L2_FIELD_TOP:
> >> - case V4L2_FIELD_BOTTOM:
> >> - case V4L2_FIELD_ALTERNATE:
> >> - vin->format.height /= 2;
> >> - break;
> >> - case V4L2_FIELD_NONE:
> >> - case V4L2_FIELD_
Re: [PATCH] videodev2.h: add helper to validate colorspace
Hi Niklas,
Thank you for the patch.
On Wednesday, 14 February 2018 00:08:47 EET Niklas Söderlund wrote:
> There is no way for drivers to validate a colorspace value, which could
> be provided by user-space by VIDIOC_S_FMT for example. Add a helper to
> validate that the colorspace value is part of enum v4l2_colorspace.
>
> Signed-off-by: Niklas Söderlund
> ---
> include/uapi/linux/videodev2.h | 5 +
> 1 file changed, 5 insertions(+)
>
> Hi,
>
> I hope this is the correct header to add this helper to. I think it's
> since if it's in uapi not only can v4l2 drivers use it but tools like
> v4l-compliance gets access to it and can be updated to use this instead
> of the hard-coded check of just < 0xff as it was last time I checked.
>
> // Niklas
>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 9827189651801e12..843afd7c5b000553 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -238,6 +238,11 @@ enum v4l2_colorspace {
> V4L2_COLORSPACE_DCI_P3= 12,
> };
>
> +/* Determine if a colorspace is defined in enum v4l2_colorspace */
> +#define V4L2_COLORSPACE_IS_VALID(colorspace) \
> + (((colorspace) >= V4L2_COLORSPACE_DEFAULT) && \
> + ((colorspace) <= V4L2_COLORSPACE_DCI_P3))
> +
This looks pretty good to me. I'd remove the parentheses around each test
though.
One potential issue is that if this macro operates on an unsigned value (for
instance an u32, which is the type used for the colorspace field in various
structures) the compiler will generate a warning:
enum.c: In function ‘test_4’:
enum.c:30:16: warning: comparison of unsigned expression >= 0 is always true
[-Wtype-limits]
return V4L2_COLORSPACE_IS_VALID(colorspace);
Dropping the first check would fix that, but wouldn't catch invalid values
when operating on a signed type, such as int or enum v4l2_colorspace.
> /*
> * Determine how COLORSPACE_DEFAULT should map to a proper colorspace.
> * This depends on whether this is a SDTV image (use SMPTE 170M), an
--
Regards,
Laurent Pinchart
[PATCH] videodev2.h: add helper to validate colorspace
There is no way for drivers to validate a colorspace value, which could
be provided by user-space by VIDIOC_S_FMT for example. Add a helper to
validate that the colorspace value is part of enum v4l2_colorspace.
Signed-off-by: Niklas Söderlund
---
include/uapi/linux/videodev2.h | 5 +
1 file changed, 5 insertions(+)
Hi,
I hope this is the correct header to add this helper to. I think it's
since if it's in uapi not only can v4l2 drivers use it but tools like
v4l-compliance gets access to it and can be updated to use this instead
of the hard-coded check of just < 0xff as it was last time I checked.
// Niklas
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 9827189651801e12..843afd7c5b000553 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -238,6 +238,11 @@ enum v4l2_colorspace {
V4L2_COLORSPACE_DCI_P3= 12,
};
+/* Determine if a colorspace is defined in enum v4l2_colorspace */
+#define V4L2_COLORSPACE_IS_VALID(colorspace) \
+ (((colorspace) >= V4L2_COLORSPACE_DEFAULT) && \
+((colorspace) <= V4L2_COLORSPACE_DCI_P3))
+
/*
* Determine how COLORSPACE_DEFAULT should map to a proper colorspace.
* This depends on whether this is a SDTV image (use SMPTE 170M), an
--
2.16.1
Re: [PATCH v10 30/30] rcar-vin: enable support for r8a77970
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:35 EET Niklas Söderlund wrote:
> Add the SoC specific information for Renesas r8a77970.
>
> Signed-off-by: Niklas Söderlund
Reviewed-by: Laurent Pinchart
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 23 +++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 2305fedd293db241..496b7d2189d73d37 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -954,6 +954,25 @@ static const struct rvin_info rcar_info_r8a7796 = {
> .routes = rcar_info_r8a7796_routes,
> };
>
> +static const struct rvin_group_route _rcar_info_r8a77970_routes[] = {
> + { .vin = 0, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(0) | BIT(3) },
> + { .vin = 1, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(2) },
> + { .vin = 1, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(3) },
> + { .vin = 2, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(1) },
> + { .vin = 2, .csi = RVIN_CSI40, .chan = 2, .mask = BIT(3) },
> + { .vin = 3, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(0) },
> + { .vin = 3, .csi = RVIN_CSI40, .chan = 3, .mask = BIT(3) },
> + { /* Sentinel */ }
> +};
> +
> +static const struct rvin_info rcar_info_r8a77970 = {
> + .model = RCAR_GEN3,
> + .use_mc = true,
> + .max_width = 4096,
> + .max_height = 4096,
> + .routes = _rcar_info_r8a77970_routes,
> +};
> +
> static const struct of_device_id rvin_of_id_table[] = {
> {
> .compatible = "renesas,vin-r8a7778",
> @@ -991,6 +1010,10 @@ static const struct of_device_id rvin_of_id_table[] =
> { .compatible = "renesas,vin-r8a7796",
> .data = &rcar_info_r8a7796,
> },
> + {
> + .compatible = "renesas,vin-r8a77970",
> + .data = &rcar_info_r8a77970,
> + },
> { /* Sentinel */ },
> };
> MODULE_DEVICE_TABLE(of, rvin_of_id_table);
--
Regards,
Laurent Pinchart
Re: [PATCH v10 29/30] rcar-vin: enable support for r8a7796
On Tuesday, 13 February 2018 23:54:55 EET Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Monday, 29 January 2018 18:34:34 EET Niklas Söderlund wrote:
> > Add the SoC specific information for Renesas r8a7796.
> >
> > Signed-off-by: Niklas Söderlund
And also
Reviewed-by: Laurent Pinchart
> > ---
> >
> > drivers/media/platform/rcar-vin/rcar-core.c | 44
> > ++
> > 1 file changed, 44 insertions(+)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > b/drivers/media/platform/rcar-vin/rcar-core.c index
> > 43d2fa83875817f0..2305fedd293db241 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -914,6 +914,46 @@ static const struct rvin_info rcar_info_r8a7795es1 =
> > {
> >
> > .routes = rcar_info_r8a7795es1_routes,
> >
> > };
> >
> > +static const struct rvin_group_route rcar_info_r8a7796_routes[] = {
> > + { .vin = 0, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(0) | BIT(3) },
> > + { .vin = 0, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(1) | BIT(4) },
> > + { .vin = 1, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(0) },
> > + { .vin = 1, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(2) },
> > + { .vin = 1, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(3) },
> > + { .vin = 1, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(4) },
> > + { .vin = 2, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(1) },
> > + { .vin = 2, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(2) },
> > + { .vin = 2, .csi = RVIN_CSI40, .chan = 2, .mask = BIT(3) },
> > + { .vin = 2, .csi = RVIN_CSI20, .chan = 2, .mask = BIT(4) },
> > + { .vin = 3, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(0) },
> > + { .vin = 3, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(1) },
> > + { .vin = 3, .csi = RVIN_CSI40, .chan = 3, .mask = BIT(3) },
> > + { .vin = 3, .csi = RVIN_CSI20, .chan = 3, .mask = BIT(4) },
> > + { .vin = 4, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(0) | BIT(3) },
> > + { .vin = 4, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(1) | BIT(4) },
> > + { .vin = 5, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(0) },
> > + { .vin = 5, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(2) },
> > + { .vin = 5, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(3) },
> > + { .vin = 5, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(4) },
> > + { .vin = 6, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(1) },
> > + { .vin = 6, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(2) },
> > + { .vin = 6, .csi = RVIN_CSI40, .chan = 2, .mask = BIT(3) },
> > + { .vin = 6, .csi = RVIN_CSI20, .chan = 2, .mask = BIT(4) },
> > + { .vin = 7, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(0) },
> > + { .vin = 7, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(1) },
> > + { .vin = 7, .csi = RVIN_CSI40, .chan = 3, .mask = BIT(3) },
> > + { .vin = 7, .csi = RVIN_CSI20, .chan = 3, .mask = BIT(4) },
> > + { /* Sentinel */ }
> > +};
> > +
> > +static const struct rvin_info rcar_info_r8a7796 = {
> > + .model = RCAR_GEN3,
> > + .use_mc = true,
> > + .max_width = 4096,
> > + .max_height = 4096,
> > + .routes = rcar_info_r8a7796_routes,
> > +};
> > +
> >
> > static const struct of_device_id rvin_of_id_table[] = {
> >
> > {
> >
> > .compatible = "renesas,vin-r8a7778",
> >
> > @@ -947,6 +987,10 @@ static const struct of_device_id rvin_of_id_table[] =
> > { .compatible = "renesas,vin-r8a7795",
> >
> > .data = &rcar_info_r8a7795,
> >
> > },
> >
> > + {
> > + .compatible = "renesas,vin-r8a7796",
> > + .data = &rcar_info_r8a7796,
> > + },
> >
> > { /* Sentinel */ },
> >
> > };
> > MODULE_DEVICE_TABLE(of, rvin_of_id_table);
--
Regards,
Laurent Pinchart
Re: [PATCH v10 29/30] rcar-vin: enable support for r8a7796
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:34 EET Niklas Söderlund wrote:
> Add the SoC specific information for Renesas r8a7796.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 44 ++
> 1 file changed, 44 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 43d2fa83875817f0..2305fedd293db241 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -914,6 +914,46 @@ static const struct rvin_info rcar_info_r8a7795es1 = {
> .routes = rcar_info_r8a7795es1_routes,
> };
>
> +static const struct rvin_group_route rcar_info_r8a7796_routes[] = {
> + { .vin = 0, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(0) | BIT(3) },
> + { .vin = 0, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(1) | BIT(4) },
> + { .vin = 1, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(0) },
> + { .vin = 1, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(2) },
> + { .vin = 1, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(3) },
> + { .vin = 1, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(4) },
> + { .vin = 2, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(1) },
> + { .vin = 2, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(2) },
> + { .vin = 2, .csi = RVIN_CSI40, .chan = 2, .mask = BIT(3) },
> + { .vin = 2, .csi = RVIN_CSI20, .chan = 2, .mask = BIT(4) },
> + { .vin = 3, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(0) },
> + { .vin = 3, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(1) },
> + { .vin = 3, .csi = RVIN_CSI40, .chan = 3, .mask = BIT(3) },
> + { .vin = 3, .csi = RVIN_CSI20, .chan = 3, .mask = BIT(4) },
> + { .vin = 4, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(0) | BIT(3) },
> + { .vin = 4, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(1) | BIT(4) },
> + { .vin = 5, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(0) },
> + { .vin = 5, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(2) },
> + { .vin = 5, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(3) },
> + { .vin = 5, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(4) },
> + { .vin = 6, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(1) },
> + { .vin = 6, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(2) },
> + { .vin = 6, .csi = RVIN_CSI40, .chan = 2, .mask = BIT(3) },
> + { .vin = 6, .csi = RVIN_CSI20, .chan = 2, .mask = BIT(4) },
> + { .vin = 7, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(0) },
> + { .vin = 7, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(1) },
> + { .vin = 7, .csi = RVIN_CSI40, .chan = 3, .mask = BIT(3) },
> + { .vin = 7, .csi = RVIN_CSI20, .chan = 3, .mask = BIT(4) },
> + { /* Sentinel */ }
> +};
> +
> +static const struct rvin_info rcar_info_r8a7796 = {
> + .model = RCAR_GEN3,
> + .use_mc = true,
> + .max_width = 4096,
> + .max_height = 4096,
> + .routes = rcar_info_r8a7796_routes,
> +};
> +
> static const struct of_device_id rvin_of_id_table[] = {
> {
> .compatible = "renesas,vin-r8a7778",
> @@ -947,6 +987,10 @@ static const struct of_device_id rvin_of_id_table[] = {
> .compatible = "renesas,vin-r8a7795",
> .data = &rcar_info_r8a7795,
> },
> + {
> + .compatible = "renesas,vin-r8a7796",
> + .data = &rcar_info_r8a7796,
> + },
> { /* Sentinel */ },
> };
> MODULE_DEVICE_TABLE(of, rvin_of_id_table);
--
Regards,
Laurent Pinchart
Re: [PATCH v10 28/30] rcar-vin: enable support for r8a7795
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:33 EET Niklas Söderlund wrote:
> Add the SoC specific information for Renesas r8a7795 ES1.x and ES2.0.
>
> Signed-off-by: Niklas Söderlund
Reviewed-by: Laurent Pinchart
> ---
> drivers/media/platform/rcar-vin/Kconfig | 2 +-
> drivers/media/platform/rcar-vin/rcar-core.c | 120 +
> 2 files changed, 121 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/rcar-vin/Kconfig
> b/drivers/media/platform/rcar-vin/Kconfig index
> af4c98b44d2e22cb..8fa7ee468c63afb9 100644
> --- a/drivers/media/platform/rcar-vin/Kconfig
> +++ b/drivers/media/platform/rcar-vin/Kconfig
> @@ -6,7 +6,7 @@ config VIDEO_RCAR_VIN
> select V4L2_FWNODE
> ---help---
> Support for Renesas R-Car Video Input (VIN) driver.
> - Supports R-Car Gen2 SoCs.
> + Supports R-Car Gen2 and Gen3 SoCs.
>
> To compile this driver as a module, choose M here: the
> module will be called rcar-vin.
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 7ceff0de40078580..43d2fa83875817f0 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -815,6 +816,104 @@ static const struct rvin_info rcar_info_gen2 = {
> .max_height = 2048,
> };
>
> +static const struct rvin_group_route rcar_info_r8a7795_routes[] = {
> + { .vin = 0, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(0) | BIT(3) },
> + { .vin = 0, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(1) | BIT(4) },
> + { .vin = 0, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(2) },
> + { .vin = 1, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(0) },
> + { .vin = 1, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(1) | BIT(3) },
> + { .vin = 1, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(2) },
> + { .vin = 1, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(4) },
> + { .vin = 2, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(0) },
> + { .vin = 2, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(1) },
> + { .vin = 2, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(2) },
> + { .vin = 2, .csi = RVIN_CSI40, .chan = 2, .mask = BIT(3) },
> + { .vin = 2, .csi = RVIN_CSI20, .chan = 2, .mask = BIT(4) },
> + { .vin = 3, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(0) },
> + { .vin = 3, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(1) | BIT(2) },
> + { .vin = 3, .csi = RVIN_CSI40, .chan = 3, .mask = BIT(3) },
> + { .vin = 3, .csi = RVIN_CSI20, .chan = 3, .mask = BIT(4) },
> + { .vin = 4, .csi = RVIN_CSI41, .chan = 0, .mask = BIT(0) | BIT(3) },
> + { .vin = 4, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(1) | BIT(4) },
> + { .vin = 4, .csi = RVIN_CSI41, .chan = 1, .mask = BIT(2) },
> + { .vin = 5, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(0) },
> + { .vin = 5, .csi = RVIN_CSI41, .chan = 1, .mask = BIT(1) | BIT(3) },
> + { .vin = 5, .csi = RVIN_CSI41, .chan = 0, .mask = BIT(2) },
> + { .vin = 5, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(4) },
> + { .vin = 6, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(0) },
> + { .vin = 6, .csi = RVIN_CSI41, .chan = 0, .mask = BIT(1) },
> + { .vin = 6, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(2) },
> + { .vin = 6, .csi = RVIN_CSI41, .chan = 2, .mask = BIT(3) },
> + { .vin = 6, .csi = RVIN_CSI20, .chan = 2, .mask = BIT(4) },
> + { .vin = 7, .csi = RVIN_CSI41, .chan = 1, .mask = BIT(0) },
> + { .vin = 7, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(1) | BIT(2) },
> + { .vin = 7, .csi = RVIN_CSI41, .chan = 3, .mask = BIT(3) },
> + { .vin = 7, .csi = RVIN_CSI20, .chan = 3, .mask = BIT(4) },
> + { /* Sentinel */ }
> +};
> +
> +static const struct rvin_info rcar_info_r8a7795 = {
> + .model = RCAR_GEN3,
> + .use_mc = true,
> + .max_width = 4096,
> + .max_height = 4096,
> + .routes = rcar_info_r8a7795_routes,
> +};
> +
> +static const struct rvin_group_route rcar_info_r8a7795es1_routes[] = {
> + { .vin = 0, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(0) | BIT(3) },
> + { .vin = 0, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(1) | BIT(4) },
> + { .vin = 0, .csi = RVIN_CSI21, .chan = 0, .mask = BIT(2) | BIT(5) },
> + { .vin = 1, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(0) },
> + { .vin = 1, .csi = RVIN_CSI21, .chan = 0, .mask = BIT(1) },
> + { .vin = 1, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(2) },
> + { .vin = 1, .csi = RVIN_CSI40, .chan = 1, .mask = BIT(3) },
> + { .vin = 1, .csi = RVIN_CSI20, .chan = 1, .mask = BIT(4) },
> + { .vin = 1, .csi = RVIN_CSI21, .chan = 1, .mask = BIT(5) },
> + { .vin = 2, .csi = RVIN_CSI21, .chan = 0, .mask = BIT(0) },
> + { .vin = 2, .csi = RVIN_CSI40, .chan = 0, .mask = BIT(1) },
> + { .vin = 2, .csi = RVIN_CSI20, .chan = 0, .mask = BIT(2) },
> + { .v
Re: [PATCH v10 27/30] rcar-vin: extend {start,stop}_streaming to work with media controller
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:32 EET Niklas Söderlund wrote:
> The procedure to start or stop streaming using the non-MC single
> subdevice and the MC graph and multiple subdevices are quite different.
> Create a new function to abstract which method is used based on which
> mode the driver is running in and add logic to start the MC graph.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 123 +++--
> 1 file changed, 116 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> 811d8f8638d21200..6784e7eb3d96e1c0 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1087,15 +1087,126 @@ static void rvin_buffer_queue(struct vb2_buffer
> *vb) spin_unlock_irqrestore(&vin->qlock, flags);
> }
>
> +static int rvin_set_stream(struct rvin_dev *vin, int on)
> +{
> + struct v4l2_subdev_format fmt = {
> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + };
> + struct media_pipeline *pipe;
> + struct media_device *mdev;
> + struct v4l2_subdev *sd;
> + struct media_pad *pad;
> + int ret;
> +
> + /* No media controller used, simply pass operation to subdevice */
> + if (!vin->info->use_mc) {
> + ret = v4l2_subdev_call(vin->digital->subdev, video, s_stream,
> +on);
> +
> + return ret == -ENOIOCTLCMD ? 0 : ret;
> + }
> +
> + pad = media_entity_remote_pad(&vin->pad);
> + if (!pad)
> + return -EPIPE;
> +
> + sd = media_entity_to_v4l2_subdev(pad->entity);
> +
> + if (!on) {
> + media_pipeline_stop(&vin->vdev.entity);
> + return v4l2_subdev_call(sd, video, s_stream, 0);
> + }
> +
> + fmt.pad = pad->index;
> + if (v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt))
> + return -EPIPE;
> +
> + switch (fmt.format.code) {
> + case MEDIA_BUS_FMT_YUYV8_1X16:
> + case MEDIA_BUS_FMT_UYVY8_2X8:
> + case MEDIA_BUS_FMT_UYVY10_2X10:
> + case MEDIA_BUS_FMT_RGB888_1X24:
> + vin->code = fmt.format.code;
> + break;
> + default:
> + return -EPIPE;
> + }
> +
> + switch (fmt.format.field) {
> + case V4L2_FIELD_TOP:
> + case V4L2_FIELD_BOTTOM:
> + case V4L2_FIELD_NONE:
> + case V4L2_FIELD_INTERLACED_TB:
> + case V4L2_FIELD_INTERLACED_BT:
> + case V4L2_FIELD_INTERLACED:
> + case V4L2_FIELD_SEQ_TB:
> + case V4L2_FIELD_SEQ_BT:
> + /* Supported natively */
> + break;
> + case V4L2_FIELD_ALTERNATE:
> + switch (vin->format.field) {
> + case V4L2_FIELD_TOP:
> + case V4L2_FIELD_BOTTOM:
> + case V4L2_FIELD_NONE:
> + break;
> + case V4L2_FIELD_INTERLACED_TB:
> + case V4L2_FIELD_INTERLACED_BT:
> + case V4L2_FIELD_INTERLACED:
> + case V4L2_FIELD_SEQ_TB:
> + case V4L2_FIELD_SEQ_BT:
> + /* Use VIN hardware to combine the two fields */
> + fmt.format.height *= 2;
> + break;
> + default:
> + return -EPIPE;
> + }
> + break;
> + default:
> + return -EPIPE;
> + }
> +
> + if (fmt.format.width != vin->format.width ||
> + fmt.format.height != vin->format.height ||
> + fmt.format.code != vin->code)
> + return -EPIPE;
I'd create a rvin_mc_validate_format() function and move this code there. This
function is growing a bit too big.
> + mdev = vin->vdev.entity.graph_obj.mdev;
> +
> + /*
> + * The graph lock needs to be taken to protect concurrent
> + * starts of multiple VIN instances as they might share
> + * a common subdevice down the line and then should use
> + * the same pipe.
> + */
> + mutex_lock(&mdev->graph_mutex);
I'd say mutex_lock_interruptible(), but videobuf2 won't support that :-S
> + pipe = sd->entity.pipe ? sd->entity.pipe : &vin->vdev.pipe;
> + ret = __media_pipeline_start(&vin->vdev.entity, pipe);
> + mutex_unlock(&mdev->graph_mutex);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> + if (ret == -ENOIOCTLCMD)
> + ret = 0;
> + if (ret)
> + media_pipeline_stop(&vin->vdev.entity);
> +
> + return ret;
> +}
> +
> static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
> {
> struct rvin_dev *vin = vb2_get_drv_priv(vq);
> - struct v4l2_subdev *sd;
> unsigned long flags;
> int ret;
>
> - sd = vin_to_source(vin);
> - v4l2_subdev_call(sd, video, s_stream, 1);
> + ret = rvin_set_stream(vin, 1);
> + if (ret) {
> +
Re: [PATCH v10 26/30] rcar-vin: add link notify for Gen3
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:31 EET Niklas Söderlund wrote:
> Add the ability to process media device link change request. Link
> enabling is a bit complicated on Gen3, whether or not it's possible to
> enable a link depends on what other links already are enabled. On Gen3
> the 8 VINs are split into two subgroup's (VIN0-3 and VIN4-7) and from a
> routing perspective these two groups are independent of each other.
> Each subgroup's routing is controlled by the subgroup VIN master
> instance (VIN0 and VIN4).
>
> There are a limited number of possible route setups available for each
> subgroup and the configuration of each setup is dictated by the
> hardware. On H3 for example there are 6 possible route setups for each
> subgroup to choose from.
>
> This leads to the media device link notification code being rather large
> since it will find the best routing configuration to try and accommodate
> as many links as possible. When it's not possible to enable a new link
> due to hardware constrains the link_notifier callback will return
> -EMLINK.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 129 +
> 1 file changed, 129 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> f08277a0dc11f477..7ceff0de40078580 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -24,6 +24,7 @@
>
> #include
> #include
> +#include
>
> #include "rcar-vin.h"
>
> @@ -44,6 +45,133 @@
> */
> #define rvin_group_id_to_master(vin) ((vin) < 4 ? 0 : 4)
>
> +/*
> + * Media Controller link notification
> + */
> +
> +/* group lock should be held when calling this function */
> +static int rvin_group_entity_to_csi_id(struct rvin_group *group,
> + struct media_entity *entity)
> +{
> + struct v4l2_subdev *sd;
> + int i;
unsigned int.
> +
> + if (!is_media_entity_v4l2_subdev(entity))
> + return -ENODEV;
Can this happen ?
> + sd = media_entity_to_v4l2_subdev(entity);
> +
> + for (i = 0; i < RVIN_CSI_MAX; i++)
> + if (group->csi[i].subdev == sd)
> + return i;
> +
> + return -ENODEV;
> +}
> +
> +static unsigned int rvin_group_get_mask(struct rvin_dev *vin,
> + enum rvin_csi_id csi_id,
> + unsigned char chan)
> +{
> + const struct rvin_group_route *route;
> + unsigned int mask = 0;
> +
> + for (route = vin->info->routes; route->mask; route++) {
Please document the fact that the array needs to be terminated by an empty
element in the kerneldoc for vin->info->routes.
> + if (route->vin == vin->id &&
> + route->csi == csi_id &&
> + route->chan == chan) {
> + vin_dbg(vin, "Adding route: vin: %d csi: %d chan: %d\n",
> + route->vin, route->csi, route->chan);
> + mask |= route->mask;
> + }
> + }
> +
> + return mask;
> +}
> +
> +static int rvin_group_link_notify(struct media_link *link, u32 flags,
> + unsigned int notification)
> +{
> + struct rvin_group *group = container_of(link->graph_obj.mdev,
> + struct rvin_group, mdev);
> + unsigned int i, master_id, chan, mask_new, mask = ~0;
I'm sure you could spare a few more lines :-)
> + struct media_entity *entity;
> + struct video_device *vdev;
> + struct media_pad *csi_pad;
> + struct rvin_dev *vin = NULL;
> + int csi_id, ret;
> +
> + ret = v4l2_pipeline_link_notify(link, flags, notification);
> + if (ret)
> + return ret;
> +
> + /* Only care about link enablement for VIN nodes */
> + if (!(flags & MEDIA_LNK_FL_ENABLED) ||
> + !is_media_entity_v4l2_video_device(link->sink->entity))
> + return 0;
> +
> + /* If any entity are in use don't allow link changes */
s/are/is/
> + media_device_for_each_entity(entity, &group->mdev)
> + if (entity->use_count)
> + return -EBUSY;
> +
> + mutex_lock(&group->lock);
> +
> + /* Find VIN and its master for which the link */
The words make sense individually. Maybe you could try a different order ? :-)
> + entity = link->sink->entity;
> + vdev = media_entity_to_video_device(entity);
You can combine those two lines and remove the entity variable.
> + for (i = 0; i < RCAR_VIN_NUM; i++) {
> + if (group->vin[i] && &group->vin[i]->vdev == vdev) {
> + vin = group->vin[i];
> + master_id = rvin_group_id_to_master(vin->id);
> + break;
> +
Re: [PATCH v10 25/30] rcar-vin: parse Gen3 OF and setup media graph
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:30 EET Niklas Söderlund wrote:
> The parsing and registering CSI-2 subdevices with the v4l2 async
> framework is a collaborative effort shared between the VIN instances
> which are part of the group. When the last VIN in the group is probed it
> asks all other VINs to parse its share of OF and record the async
> subdevices it finds in the notifier belonging to the last probed VIN.
>
> Once all CSI-2 subdevices in this notifier are bound proceed to register
> all VIN video devices of the group and crate media device links between
> all CSI-2 and VIN entities according to the SoC specific routing
> configuration.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 250 -
> drivers/media/platform/rcar-vin/rcar-vin.h | 12 +-
> 2 files changed, 258 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 4a64df5019ce45f7..f08277a0dc11f477 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -27,6 +27,23 @@
>
> #include "rcar-vin.h"
>
> +/*
> + * The companion CSI-2 receiver driver (rcar-csi2) is known
> + * and we know it have one source pad (pad 0) and four sink
s/have/has/
> + * pads (pad 1-4). So to translate a pad on the remote
> + * CSI-2 receiver to/from the VIN internal channel number simply
> + * subtract/add or one from the pad/chan number.
s/or one/one/
and maybe s/chan/channel/ ?
> + */
> +#define rvin_group_csi_pad_to_chan(pad) ((pad) - 1)
> +#define rvin_group_csi_chan_to_pad(chan) ((chan) + 1)
> +
> +/*
> + * Not all VINs are created equal, master VINs control the
> + * routing for other VIN's. We can figure out which VIN is
> + * master by looking at a VINs id
> + */
> +#define rvin_group_id_to_master(vin) ((vin) < 4 ? 0 : 4)
> +
> /*
> * Gen3 CSI2 Group Allocator
> */
> @@ -77,6 +94,8 @@ static int rvin_group_init(struct rvin_group *group,
> struct rvin_dev *vin) snprintf(mdev->bus_info, sizeof(mdev->bus_info),
> "platform:%s",
>dev_name(mdev->dev));
>
> + group->notifier = NULL;
> +
The group has been allocated with kzalloc() so this isn't necessary.
> media_device_init(mdev);
>
> ret = media_device_register(&group->mdev);
> @@ -406,6 +425,218 @@ static int rvin_digital_graph_init(struct rvin_dev
> *vin) return 0;
> }
>
> +/*
> + * Group async notifier
> + */
> +
> +static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct rvin_dev *vin = notifier_to_vin(notifier);
> + const struct rvin_group_route *route;
> + unsigned int i;
> + int ret;
> +
> + ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
> + if (ret) {
> + vin_err(vin, "Failed to register subdev nodes\n");
> + return ret;
> + }
> +
> + /* Register all video nodes for the group */
> + for (i = 0; i < RCAR_VIN_NUM; i++) {
> + if (vin->group->vin[i]) {
> + ret = rvin_v4l2_register(vin->group->vin[i]);
> + if (ret)
> + return ret;
> + }
> + }
> +
> + /* Create all media device links between VINs and CSI-2's */
> + mutex_lock(&vin->group->lock);
> + for (route = vin->info->routes; route->mask; route++) {
> + struct media_pad *source_pad, *sink_pad;
> + struct media_entity *source, *sink;
> + unsigned int source_idx;
> +
> + /* Check that VIN is part of the group */
> + if (!vin->group->vin[route->vin])
> + continue;
> +
> + /* Check that VIN' master is part of the group */
> + if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
> + continue;
> +
> + /* Check that CSI-2 is part of the group */
> + if (!vin->group->csi[route->csi].subdev)
> + continue;
> +
> + source = &vin->group->csi[route->csi].subdev->entity;
> + source_idx = rvin_group_csi_chan_to_pad(route->chan);
> + source_pad = &source->pads[source_idx];
> +
> + sink = &vin->group->vin[route->vin]->vdev.entity;
> + sink_pad = &sink->pads[0];
> +
> + /* Skip if link already exists */
> + if (media_entity_find_link(source_pad, sink_pad))
> + continue;
> +
> + ret = media_create_pad_link(source, source_idx, sink, 0, 0);
> + if (ret) {
> + vin_err(vin, "Error adding link from %s to %s\n",
> + source->name, sink->name);
> + break;
> +
Re: [PATCH v2 8/8] platform: vivid-cec: use 64-bit arithmetic instead of 32-bit
On Mon 2018-02-05 22:29:41, Hans Verkuil wrote: > On 02/05/2018 09:36 PM, Gustavo A. R. Silva wrote: > > Add suffix ULL to constant 10 in order to give the compiler complete > > information about the proper arithmetic to use. Notice that this > > constant is used in a context that expects an expression of type > > u64 (64 bits, unsigned). > > > > The expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is currently being > > evaluated using 32-bit arithmetic. > > > > Also, remove unnecessary parentheses and add a code comment to make it > > clear what is the reason of the code change. > > > > Addresses-Coverity-ID: 1454996 > > Signed-off-by: Gustavo A. R. Silva > > --- > > Changes in v2: > > - Update subject and changelog to better reflect the proposed code changes. > > - Add suffix ULL to constant instead of casting a variable. > > - Remove unncessary parentheses. > > unncessary -> unnecessary > > > - Add code comment. > > > > drivers/media/platform/vivid/vivid-cec.c | 11 +-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/vivid/vivid-cec.c > > b/drivers/media/platform/vivid/vivid-cec.c > > index b55d278..614787b 100644 > > --- a/drivers/media/platform/vivid/vivid-cec.c > > +++ b/drivers/media/platform/vivid/vivid-cec.c > > @@ -82,8 +82,15 @@ static void vivid_cec_pin_adap_events(struct cec_adapter > > *adap, ktime_t ts, > > > > if (adap == NULL) > > return; > > - ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL + > > - len * 10 * CEC_TIM_DATA_BIT_TOTAL)); > > + > > + /* > > +* Suffix ULL on constant 10 makes the expression > > +* CEC_TIM_START_BIT_TOTAL + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL > > +* be evaluated using 64-bit unsigned arithmetic (u64), which > > +* is what ktime_sub_us expects as second argument. > > +*/ > > That's not really the comment that I was looking for. It still doesn't > explain *why* this is needed at all. How about something like this: > > /* > * Add the ULL suffix to the constant 10 to work around a false Coverity > * "Unintentional integer overflow" warning. Coverity isn't smart enough > * to understand that len is always <= 16, so there is no chance of an > * integer overflow. > */ Or maybe it would be better to add comment about Coverity having false-positive and not to modify the code? Hmm. Could we do something like BUG_ON(len > 16) to make Coverity understand the ranges? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v10 24/30] rcar-vin: add chsel information to rvin_info
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:29 EET Niklas Söderlund wrote:
> Each Gen3 SoC has a limited set of predefined routing possibilities for
> which CSI-2 device and virtual channel can be routed to which VIN
> instance. Prepare to store this information in the struct rvin_info.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-vin.h | 30 +++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> 903d8fb8426a7860..ca2c2a23cef8506c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -43,6 +43,14 @@ enum model_id {
> RCAR_GEN3,
> };
>
> +enum rvin_csi_id {
> + RVIN_CSI20,
> + RVIN_CSI21,
> + RVIN_CSI40,
> + RVIN_CSI41,
> + RVIN_CSI_MAX,
> +};
> +
> /**
> * STOPPED - No operation in progress
> * RUNNING - Operation in progress have buffers
> @@ -81,12 +89,33 @@ struct rvin_graph_entity {
> unsigned int sink_pad;
> };
>
> +/** struct rvin_group_route - Map a CSI-2 receiver and channel to a CHSEL
If my understanding is correct an entry describes a route from a channel of a
CSI-2 receiver to a VIN, and how to configure the hardware to enable that
route (in the mask field). Could you expand this single line of documentation
to explain that more clearly ?
> + * @vin: Which VIN the CSI-2 and VC describes
VC ? Is that virtual channel ? Isn't that internal to the CSI-2 receiver only
?
> + * @csi: VIN internal number for CSI-2 device
Or just "CSI-2 receiver ID" ?
> + * @chan:Output channel of the CSI-2 receiver. Each R-Car CSI-2
Would "channel" be too long ?
> + * receiver has four output channels facing the VIN
> + * devices, each channel can carry one CSI-2 Virtual
> + * Channel (VC) and there are no correlation between
> + * output channel number and CSI-2 VC. It's up to the
> + * CSI-2 receiver driver to configure which VC is
> + * outputted on which channel, the VIN devices only
s/outputted/output/
> + * cares about output channels.
s/cares/care/
> + * @mask:Bitmask of chsel values which accommodates route
s/which/that/
Reading the documentation I'm not sure to understand how this works. In
particular the mask field documentation isn't clear enough.
> + */
> +struct rvin_group_route {
> + unsigned int vin;
> + enum rvin_csi_id csi;
> + unsigned char chan;
You can make this an unsigned int, the compiler will pad the field anyway.
I think it would be clearer to order the fields in "from -> to: configuration"
order (csi, channel, vin, mask).
> + unsigned int mask;
> +};
> +
> /**
> * struct rvin_info - Information about the particular VIN implementation
> * @model: VIN model
> * @use_mc: use media controller instead of controlling subdevice
> * @max_width: max input width the VIN supports
> * @max_height: max input height the VIN supports
> + * @routes: routing table VIN <-> CSI-2 for the chsel values
> */
> struct rvin_info {
> enum model_id model;
> @@ -94,6 +123,7 @@ struct rvin_info {
>
> unsigned int max_width;
> unsigned int max_height;
> + const struct rvin_group_route *routes;
> };
>
> /**
--
Regards,
Laurent Pinchart
Re: [PATCH v10 23/30] rcar-vin: change name of video device
Hi Niklas, Thank you for the patch. On Monday, 29 January 2018 18:34:28 EET Niklas Söderlund wrote: > The rcar-vin driver needs to be part of a media controller to support > Gen3. Give each VIN instance a unique name so it can be referenced from > userspace. > > Signed-off-by: Niklas Söderlund Reviewed-by: Laurent Pinchart > --- > drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index > 292e1f22a4be36c7..3ac6cdcb18ce4a21 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -1012,7 +1012,7 @@ int rvin_v4l2_register(struct rvin_dev *vin) > /* video node */ > vdev->v4l2_dev = &vin->v4l2_dev; > vdev->queue = &vin->queue; > - strlcpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name)); > + snprintf(vdev->name, sizeof(vdev->name), "VIN%u output", vin->id); > vdev->release = video_device_release_empty; > vdev->lock = &vin->lock; > vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | -- Regards, Laurent Pinchart
Re: [PATCH v10 22/30] rcar-vin: add group allocator functions
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:27 EET Niklas Söderlund wrote:
> In media controller mode all VIN instances needs to be part of the same
> media graph. There is also a need for each VIN instance to know about
> and in some cases be able to communicate with other VIN instances.
>
> Add an allocator framework where the first VIN instance to be probed
> creates a shared data structure and registers a media device.
> Consecutive VINs insert themself into the global group.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 177 -
> drivers/media/platform/rcar-vin/rcar-vin.h | 31 +
> 2 files changed, 206 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 0c6960756c33f86c..4a64df5019ce45f7 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -20,12 +20,177 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
>
> #include "rcar-vin.h"
>
> +/*
> + * Gen3 CSI2 Group Allocator
> + */
> +
> +/* FIXME: This should if we find a system that supports more
> + * then one group for the whole system be replaced with a linked
s/then/than/
> + * list of groups. And eventually all of this should be replaced
> + * with a global device allocator API.
> + *
> + * But for now this works as on all supported systems there will
> + * be only one group for all instances.
> + */
> +
> +static DEFINE_MUTEX(rvin_group_lock);
> +static struct rvin_group *rvin_group_data;
> +
> +static void rvin_group_cleanup(struct rvin_group *group)
> +{
> + media_device_unregister(&group->mdev);
> + media_device_cleanup(&group->mdev);
> + mutex_destroy(&group->lock);
> +}
> +
> +static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> +{
> + struct media_device *mdev = &group->mdev;
> + const struct of_device_id *match;
> + struct device_node *np;
> + int ret;
> +
> + mutex_init(&group->lock);
> +
> + /* Count number of VINs in the system */
> + group->count = 0;
> + for_each_matching_node(np, vin->dev->driver->of_match_table)
> + if (of_device_is_available(np))
> + group->count++;
> +
> + vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
> +
> + mdev->dev = vin->dev;
> +
> + match = of_match_node(vin->dev->driver->of_match_table,
> + vin->dev->of_node);
> +
> + strlcpy(mdev->driver_name, KBUILD_MODNAME, sizeof(mdev->driver_name));
> + strlcpy(mdev->model, match->compatible, sizeof(mdev->model));
> + snprintf(mdev->bus_info, sizeof(mdev->bus_info), "platform:%s",
> + dev_name(mdev->dev));
> +
> + media_device_init(mdev);
> +
> + ret = media_device_register(&group->mdev);
> + if (ret)
> + rvin_group_cleanup(group);
> +
> + return ret;
> +}
> +
> +static void rvin_group_release(struct kref *kref)
> +{
> + struct rvin_group *group =
> + container_of(kref, struct rvin_group, refcount);
> +
> + mutex_lock(&rvin_group_lock);
> +
> + rvin_group_data = NULL;
> +
> + rvin_group_cleanup(group);
> +
> + kfree(group);
> +
> + mutex_unlock(&rvin_group_lock);
> +}
> +
> +static int rvin_group_get(struct rvin_dev *vin)
> +{
> + struct rvin_group *group;
> + u32 id;
> + int ret;
> +
> + /* Make sure VIN id is present and sane */
> + ret = of_property_read_u32(vin->dev->of_node, "renesas,id", &id);
> + if (ret) {
> + vin_err(vin, "%pOF: No renesas,id property found\n",
> + vin->dev->of_node);
> + return -EINVAL;
> + }
> +
> + if (id >= RCAR_VIN_NUM) {
> + vin_err(vin, "%pOF: Invalid renesas,id '%u'\n",
> + vin->dev->of_node, id);
> + return -EINVAL;
> + }
I'd move this out of this function to an OF parsing function, but we don't
have one yet. Something to keep in mind for later.
> + /* Join or create a VIN group */
> + mutex_lock(&rvin_group_lock);
> + if (rvin_group_data) {
> + group = rvin_group_data;
> + kref_get(&group->refcount);
> + } else {
> + group = kzalloc(sizeof(*group), GFP_KERNEL);
> + if (!group) {
> + ret = -ENOMEM;
> + goto err_group;
> + }
> +
> + ret = rvin_group_init(group, vin);
> + if (ret) {
> + kfree(group);
> + vin_err(vin, "Failed to initialize group\n");
> + goto err_group;
> + }
> +
> + kref_init(&group->refcount);
> +
> + rvin_group_data = group;
> + }
> + mutex_unlock(&rvin
Re: [PATCH v10 21/30] rcar-vin: prepare for media controller mode initialization
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:26 EET Niklas Söderlund wrote:
> Prepare for media controller by calling a different initialization then
> for when running in device centric mode. Add trivial configuration of
s/then for when/than when/
> the mbus and creation of the media pad for the video device entity.
>
> While we are at it clearly mark the digital device centric notifier
> functions with a comment.
>
> Signed-off-by: Niklas Söderlund
> Reviewed-by: Hans Verkuil
Reviewed-by: Laurent Pinchart
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 20 ++--
> drivers/media/platform/rcar-vin/rcar-vin.h | 4
> 2 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 64034c96f384b3ed..0c6960756c33f86c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -46,6 +46,10 @@ static int rvin_find_pad(struct v4l2_subdev *sd, int
> direction) return -EINVAL;
> }
>
> +/*
> + * Digital async notifier
> + */
> +
> /* The vin lock shuld be held when calling the subdevice attach and detach
> */ static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
>struct v4l2_subdev *subdev)
> @@ -237,6 +241,16 @@ static int rvin_digital_graph_init(struct rvin_dev
> *vin) return 0;
> }
>
> +static int rvin_mc_init(struct rvin_dev *vin)
> +{
> + /* All our sources are CSI-2 */
> + vin->mbus_cfg.type = V4L2_MBUS_CSI2;
> + vin->mbus_cfg.flags = 0;
> +
> + vin->pad.flags = MEDIA_PAD_FL_SINK;
> + return media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> +}
> +
> /*
> * Platform Device Driver
> */
> @@ -325,8 +339,10 @@ static int rcar_vin_probe(struct platform_device *pdev)
> return ret;
>
> platform_set_drvdata(pdev, vin);
> -
> - ret = rvin_digital_graph_init(vin);
> + if (vin->info->use_mc)
> + ret = rvin_mc_init(vin);
> + else
> + ret = rvin_digital_graph_init(vin);
> if (ret < 0)
> goto error;
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> 64476bc5c8abc6d0..4caef7193db09c5b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -101,6 +101,8 @@ struct rvin_info {
> * @notifier:V4L2 asynchronous subdevs notifier
> * @digital: entity in the DT for local digital subdevice
> *
> + * @pad: media pad for the video device entity
> + *
> * @lock:protects @queue
> * @queue: vb2 buffers queue
> *
> @@ -130,6 +132,8 @@ struct rvin_dev {
> struct v4l2_async_notifier notifier;
> struct rvin_graph_entity *digital;
>
> + struct media_pad pad;
> +
> struct mutex lock;
> struct vb2_queue queue;
--
Regards,
Laurent Pinchart
Re: [PATCH v10 20/30] rcar-vin: use different v4l2 operations in media controller mode
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:25 EET Niklas Söderlund wrote:
> When the driver runs in media controller mode it should not directly
> control the subdevice instead userspace will be responsible for
> configuring the pipeline. To be able to run in this mode a different set
> of v4l2 operations needs to be used.
>
> Add a new set of v4l2 operations to support operation without directly
> interacting with the source subdevice.
>
> Signed-off-by: Niklas Söderlund
> Reviewed-by: Hans Verkuil
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 3 +-
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 155 -
> 2 files changed, 154 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> ae286742f15a3ab5..811d8f8638d21200 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -628,7 +628,8 @@ static int rvin_setup(struct rvin_dev *vin)
> /* Default to TB */
> vnmc = VNMC_IM_FULL;
> /* Use BT if video standard can be read and is 60 Hz format */
> - if (!v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
> + if (!vin->info->use_mc &&
> + !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
> if (std & V4L2_STD_525_60)
> vnmc = VNMC_IM_FULL | VNMC_FOC;
> }
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> f69ae76b3fda50c7..292e1f22a4be36c7 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -18,11 +18,14 @@
>
> #include
> #include
> +#include
> #include
>
> #include "rcar-vin.h"
>
> #define RVIN_DEFAULT_FORMAT V4L2_PIX_FMT_YUYV
> +#define RVIN_DEFAULT_WIDTH 800
> +#define RVIN_DEFAULT_HEIGHT 600
> #define RVIN_DEFAULT_FIELD V4L2_FIELD_NONE
> #define RVIN_DEFAULT_COLORSPACE V4L2_COLORSPACE_SRGB
>
> @@ -698,6 +701,83 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
> .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> };
>
> +/*
> + * V4L2 Media Controller
> + */
> +
> +static int __rvin_mc_try_format(struct rvin_dev *vin,
> + struct v4l2_pix_format *pix)
> +{
> + if (pix->field == V4L2_FIELD_ANY)
> + pix->field = RVIN_DEFAULT_FIELD;
> +
> + return rvin_format_align(vin, pix);
> +}
> +
> +static int rvin_mc_try_fmt_vid_cap(struct file *file, void *priv,
> +struct v4l2_format *f)
> +{
> + struct rvin_dev *vin = video_drvdata(file);
> +
> + return __rvin_mc_try_format(vin, &f->fmt.pix);
> +}
> +
> +static int rvin_mc_s_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_format *f)
> +{
> + struct rvin_dev *vin = video_drvdata(file);
> + int ret;
> +
> + if (vb2_is_busy(&vin->queue))
> + return -EBUSY;
> +
> + ret = __rvin_mc_try_format(vin, &f->fmt.pix);
> + if (ret)
> + return ret;
> +
> + vin->format = f->fmt.pix;
> +
> + return 0;
> +}
> +
> +static int rvin_mc_enum_input(struct file *file, void *priv,
> + struct v4l2_input *i)
> +{
> + if (i->index != 0)
> + return -EINVAL;
> +
> + i->type = V4L2_INPUT_TYPE_CAMERA;
> + strlcpy(i->name, "Camera", sizeof(i->name));
> +
> + return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
> + .vidioc_querycap= rvin_querycap,
> + .vidioc_try_fmt_vid_cap = rvin_mc_try_fmt_vid_cap,
> + .vidioc_g_fmt_vid_cap = rvin_g_fmt_vid_cap,
> + .vidioc_s_fmt_vid_cap = rvin_mc_s_fmt_vid_cap,
> + .vidioc_enum_fmt_vid_cap= rvin_enum_fmt_vid_cap,
> +
> + .vidioc_enum_input = rvin_mc_enum_input,
> + .vidioc_g_input = rvin_g_input,
> + .vidioc_s_input = rvin_s_input,
> +
> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
> + .vidioc_querybuf= vb2_ioctl_querybuf,
> + .vidioc_qbuf= vb2_ioctl_qbuf,
> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> + .vidioc_expbuf = vb2_ioctl_expbuf,
> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> + .vidioc_streamon= vb2_ioctl_streamon,
> + .vidioc_streamoff = vb2_ioctl_streamoff,
> +
> + .vidioc_log_status = v4l2_ctrl_log_status,
> + .vidioc_subscribe_event = rvin_subscribe_event,
> + .vidioc_unsubscribe_event = v4l2_event_unsubs
[PATCH v4 0/5] Add support for i2c_new_secondary_device
From: Kieran Bingham Back in 2014, Jean-Michel provided patches [0] to implement a means of describing software defined I2C addresses for devices through the DT nodes. The patch to implement the function "i2c_new_secondary_device()" was integrated, but the corresponding driver update didn't get applied. This short series re-bases Jean-Michel's patch to mainline for the ADV7604 driver in linux-media, and also provides a patch for the ADV7511 DRM Bridge driver taking the same approach. This series allows us to define the I2C address allocations of these devices in the device tree for the Renesas D3 platform where these two devices reside on the same bus and conflict with each other presently.. [0] https://lkml.org/lkml/2014/10/22/468 [1] https://lkml.org/lkml/2014/10/22/469 v2: - dt bindings split from driver changes - fixed up dt binding property descriptions - Update missing edid-i2c address setting (adv7511) - Provide update for r8a7792 DTB to account for address conflict v3: - Split map register addresses into individual declarations across all uses v4: - Normalise the usage of the I2C term throughout - Fix registration/cleanup of packet client in adv7511 - Rename adv76xx structures - Update commit titles of dt-bindings patches. Jean-Michel Hautbois (2): dt-bindings: media: adv7604: Extend bindings to allow specifying slave map addresses media: adv7604: Add support for i2c_new_secondary_device Kieran Bingham (3): dt-bindings: adv7511: Extend bindings to allow specifying slave map addresses [RFT] ARM: dts: wheat: Fix ADV7513 address usage drm: adv7511: Add support for i2c_new_secondary_device .../bindings/display/bridge/adi,adv7511.txt| 18 ++- .../devicetree/bindings/media/i2c/adv7604.txt | 18 ++- arch/arm/boot/dts/r8a7792-wheat.dts| 12 - drivers/gpu/drm/bridge/adv7511/adv7511.h | 6 +++ drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 42 +-- drivers/media/i2c/adv7604.c| 62 ++ 6 files changed, 115 insertions(+), 43 deletions(-) -- 2.7.4
[PATCH v4 1/5] dt-bindings: media: adv7604: Extend bindings to allow specifying slave map addresses
From: Jean-Michel Hautbois
The ADV7604 has thirteen 256-byte maps that can be accessed via the main
I2C ports. Each map has it own I2C address and acts as a standard slave
device on the I2C bus.
Extend the device tree node bindings to be able to override the default
addresses so that address conflicts with other devices on the same bus
may be resolved at the board description level.
Signed-off-by: Jean-Michel Hautbois
[Kieran: Re-adapted for mainline]
Signed-off-by: Kieran Bingham
Reviewed-by: Rob Herring
Reviewed-by: Laurent Pinchart
---
Based upon the original posting :
https://lkml.org/lkml/2014/10/22/469
v2:
- DT Binding update separated from code change
- Minor reword to commit message to account for DT only change.
- Collected Rob's RB tag.
v3:
- Split map register addresses into individual declarations.
v4:
- Collect Laurents RB tag
- Adapt commit title
- Normalise I2C usage (I²C is harder to grep for)
.../devicetree/bindings/media/i2c/adv7604.txt | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
index 9cbd92eb5d05..dcf57e7c60eb 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
@@ -13,7 +13,11 @@ Required Properties:
- "adi,adv7611" for the ADV7611
- "adi,adv7612" for the ADV7612
- - reg: I2C slave address
+ - reg: I2C slave addresses
+The ADV76xx has up to thirteen 256-byte maps that can be accessed via the
+main I2C ports. Each map has it own I2C address and acts as a standard
+slave device on the I2C bus. The main address is mandatory, others are
+optional and revert to defaults if not specified.
- hpd-gpios: References to the GPIOs that control the HDMI hot-plug
detection pins, one per HDMI input. The active flag indicates the GPIO
@@ -35,6 +39,11 @@ Optional Properties:
- reset-gpios: Reference to the GPIO connected to the device's reset pin.
- default-input: Select which input is selected after reset.
+ - reg-names : Names of maps with programmable addresses.
+ It can contain any map needing a non-default address.
+ Possible maps names are :
+ "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe",
+ "rep", "edid", "hdmi", "test", "cp", "vdp"
Optional Endpoint Properties:
@@ -52,7 +61,12 @@ Example:
hdmi_receiver@4c {
compatible = "adi,adv7611";
- reg = <0x4c>;
+ /*
+* The edid page will be accessible @ 0x66 on the I2C bus. All
+* other maps will retain their default addresses.
+*/
+ reg = <0x4c>, <0x66>;
+ reg-names "main", "edid";
reset-gpios = <&ioexp 0 GPIO_ACTIVE_LOW>;
hpd-gpios = <&ioexp 2 GPIO_ACTIVE_HIGH>;
--
2.7.4
Re: [PATCH v10 19/30] rcar-vin: set a default field to fallback on
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:24 EET Niklas Söderlund wrote:
> If the field is not supported by the driver it should not try to keep
> the current field. Instead it should set it to a default fallback. Since
> trying a format should always result in the same state regardless of the
> current state of the device.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 6403650aff22a2ed..f69ae76b3fda50c7 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -23,6 +23,7 @@
> #include "rcar-vin.h"
>
> #define RVIN_DEFAULT_FORMAT V4L2_PIX_FMT_YUYV
> +#define RVIN_DEFAULT_FIELD V4L2_FIELD_NONE
> #define RVIN_DEFAULT_COLORSPACE V4L2_COLORSPACE_SRGB
>
> /*
> @@ -171,7 +172,7 @@ static int rvin_get_source_format(struct rvin_dev
> *vin, fmt.format.height *= 2;
> break;
> default:
> - vin->format.field = V4L2_FIELD_NONE;
> + vin->format.field = RVIN_DEFAULT_FIELD;
> break;
> }
>
> @@ -267,9 +268,8 @@ static int __rvin_try_format(struct rvin_dev *vin,
> {
> int ret;
>
> - /* Keep current field if no specific one is asked for */
> if (pix->field == V4L2_FIELD_ANY)
> - pix->field = vin->format.field;
> + pix->field = RVIN_DEFAULT_FIELD;
Won't this also be caught by the field check in the above function, called
from __rvin_try_format_source() ? You could just remove this check completely.
However as mentioned in a comment for a previous patch I don't think the field
handling belongs in rvin_get_source_format(), so you could merge both here.
Or, if you repurpose and rename rvin_get_source_format(), then the check can
probably be removed completely. I haven't checked the consolidated code after
applying all patches from this series, but some refactoring might be useful.
We'll see.
> /* Limit to source capabilities */
> ret = __rvin_try_format_source(vin, which, pix);
--
Regards,
Laurent Pinchart
[PATCH v4 2/5] dt-bindings: adv7511: Extend bindings to allow specifying slave map addresses
From: Kieran Bingham
The ADV7511 has four 256-byte maps that can be accessed via the main I2C
ports. Each map has it own I2C address and acts as a standard slave
device on the I2C bus.
Extend the device tree node bindings to be able to override the default
addresses so that address conflicts with other devices on the same bus
may be resolved at the board description level.
Signed-off-by: Kieran Bingham
Reviewed-by: Rob Herring
Reviewed-by: Laurent Pinchart
---
v2:
- Fixed up reg: property description to account for multiple optional
addresses.
- Minor reword to commit message to account for DT only change
- Collected Robs RB tag
v3:
- Split map register addresses into individual declarations.
v4:
- Update commit title
- Collect Laurent's RB tag
- Fix nitpickings
- Normalise I2C usage (I²C is harder to grep for)
.../devicetree/bindings/display/bridge/adi,adv7511.txt | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 0047b1394c70..2c887536258c 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -14,7 +14,13 @@ Required properties:
"adi,adv7513"
"adi,adv7533"
-- reg: I2C slave address
+- reg: I2C slave addresses
+ The ADV7511 internal registers are split into four pages exposed through
+ different I2C addresses, creating four register maps. Each map has it own
+ I2C address and acts as a standard slave device on the I2C bus. The main
+ address is mandatory, others are optional and revert to defaults if not
+ specified.
+
The ADV7511 supports a large number of input data formats that differ by their
color depth, color format, clock mode, bit justification and random
@@ -70,6 +76,9 @@ Optional properties:
rather than generate its own timings for HDMI output.
- clocks: from common clock binding: reference to the CEC clock.
- clock-names: from common clock binding: must be "cec".
+- reg-names : Names of maps with programmable addresses.
+ It can contain any map needing a non-default address.
+ Possible maps names are : "main", "edid", "cec", "packet"
Required nodes:
@@ -88,7 +97,12 @@ Example
adv7511w: hdmi@39 {
compatible = "adi,adv7511w";
- reg = <39>;
+ /*
+* The EDID page will be accessible on address 0x66 on the I2C
+* bus. All other maps continue to use their default addresses.
+*/
+ reg = <0x39>, <0x66>;
+ reg-names = "main", "edid";
interrupt-parent = <&gpio3>;
interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
clocks = <&cec_clock>;
--
2.7.4
[PATCH v4 4/5] media: adv7604: Add support for i2c_new_secondary_device
From: Jean-Michel Hautbois
The ADV7604 has thirteen 256-byte maps that can be accessed via the main
I2C ports. Each map has it own I2C address and acts as a standard slave
device on the I2C bus.
Allow a device tree node to override the default addresses so that
address conflicts with other devices on the same bus may be resolved at
the board description level.
Signed-off-by: Jean-Michel Hautbois
[Kieran: Re-adapted for mainline]
Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
---
Based upon the original posting :
https://lkml.org/lkml/2014/10/22/469
v2:
- Split out DT bindings from driver updates
- Return -EINVAL on error paths from adv76xx_dummy_client()
v3:
- No change
v4:
- s/struct adv76xx_register/struct adv76xx_register_map/
- s/adv76xx_secondary_names/adv76xx_default_addresses/
- Normalise I2C usage (s/I²C/I2C/)
drivers/media/i2c/adv7604.c | 62 +
1 file changed, 40 insertions(+), 22 deletions(-)
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 1544920ec52d..d52c624a2970 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -2734,6 +2734,27 @@ static const struct v4l2_ctrl_config
adv76xx_ctrl_free_run_color = {
/* --- */
+struct adv76xx_register_map {
+ const char *name;
+ u8 default_addr;
+};
+
+static const struct adv76xx_register_map adv76xx_default_addresses[] = {
+ [ADV76XX_PAGE_IO] = { "main", 0x4c },
+ [ADV7604_PAGE_AVLINK] = { "avlink", 0x42 },
+ [ADV76XX_PAGE_CEC] = { "cec", 0x40 },
+ [ADV76XX_PAGE_INFOFRAME] = { "infoframe", 0x3e },
+ [ADV7604_PAGE_ESDP] = { "esdp", 0x38 },
+ [ADV7604_PAGE_DPP] = { "dpp", 0x3c },
+ [ADV76XX_PAGE_AFE] = { "afe", 0x26 },
+ [ADV76XX_PAGE_REP] = { "rep", 0x32 },
+ [ADV76XX_PAGE_EDID] = { "edid", 0x36 },
+ [ADV76XX_PAGE_HDMI] = { "hdmi", 0x34 },
+ [ADV76XX_PAGE_TEST] = { "test", 0x30 },
+ [ADV76XX_PAGE_CP] = { "cp", 0x22 },
+ [ADV7604_PAGE_VDP] = { "vdp", 0x24 },
+};
+
static int adv76xx_core_init(struct v4l2_subdev *sd)
{
struct adv76xx_state *state = to_state(sd);
@@ -2834,13 +2855,26 @@ static void adv76xx_unregister_clients(struct
adv76xx_state *state)
}
static struct i2c_client *adv76xx_dummy_client(struct v4l2_subdev *sd,
- u8 addr, u8 io_reg)
+ unsigned int page)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct adv76xx_state *state = to_state(sd);
+ struct adv76xx_platform_data *pdata = &state->pdata;
+ unsigned int io_reg = 0xf2 + page;
+ struct i2c_client *new_client;
+
+ if (pdata && pdata->i2c_addresses[page])
+ new_client = i2c_new_dummy(client->adapter,
+ pdata->i2c_addresses[page]);
+ else
+ new_client = i2c_new_secondary_device(client,
+ adv76xx_default_addresses[page].name,
+ adv76xx_default_addresses[page].default_addr);
- if (addr)
- io_write(sd, io_reg, addr << 1);
- return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1);
+ if (new_client)
+ io_write(sd, io_reg, new_client->addr << 1);
+
+ return new_client;
}
static const struct adv76xx_reg_seq adv7604_recommended_settings_afe[] = {
@@ -3115,20 +3149,6 @@ static int adv76xx_parse_dt(struct adv76xx_state *state)
/* Disable the interrupt for now as no DT-based board uses it. */
state->pdata.int1_config = ADV76XX_INT1_CONFIG_DISABLED;
- /* Use the default I2C addresses. */
- state->pdata.i2c_addresses[ADV7604_PAGE_AVLINK] = 0x42;
- state->pdata.i2c_addresses[ADV76XX_PAGE_CEC] = 0x40;
- state->pdata.i2c_addresses[ADV76XX_PAGE_INFOFRAME] = 0x3e;
- state->pdata.i2c_addresses[ADV7604_PAGE_ESDP] = 0x38;
- state->pdata.i2c_addresses[ADV7604_PAGE_DPP] = 0x3c;
- state->pdata.i2c_addresses[ADV76XX_PAGE_AFE] = 0x26;
- state->pdata.i2c_addresses[ADV76XX_PAGE_REP] = 0x32;
- state->pdata.i2c_addresses[ADV76XX_PAGE_EDID] = 0x36;
- state->pdata.i2c_addresses[ADV76XX_PAGE_HDMI] = 0x34;
- state->pdata.i2c_addresses[ADV76XX_PAGE_TEST] = 0x30;
- state->pdata.i2c_addresses[ADV76XX_PAGE_CP] = 0x22;
- state->pdata.i2c_addresses[ADV7604_PAGE_VDP] = 0x24;
-
/* Hardcode the remaining platform data fields. */
state->pdata.disable_pwrdnb = 0;
state->pdata.disable_cable_det_rst = 0;
@@ -3478,11 +3498,9 @@ static int adv76xx_probe(struct i2c_client *client,
if (!(BIT(i) & state->info->page_mask))
continue;
- state->i2c_clients[i] =
- adv76xx_dummy_client(sd, state->pdata.i2c_add
[PATCH v4 5/5] drm: adv7511: Add support for i2c_new_secondary_device
From: Kieran Bingham
The ADV7511 has four 256-byte maps that can be accessed via the main I2C
ports. Each map has it own I2C address and acts as a standard slave
device on the I2C bus.
Allow a device tree node to override the default addresses so that
address conflicts with other devices on the same bus may be resolved at
the board description level.
Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
---
v2:
- Update missing edid-i2c address setting
- Split out DT bindings
- Rename and move the I2C default addresses to their own section
v3:
- No change
v4:
- Change registration order of packet/cec to fix error path and
simplify code change.
- Collect Laurent's RB tag
drivers/gpu/drm/bridge/adv7511/adv7511.h | 6
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 42 ++--
2 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index d034b2cb5eee..73d8ccb97742 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -93,6 +93,11 @@
#define ADV7511_REG_CHIP_ID_HIGH 0xf5
#define ADV7511_REG_CHIP_ID_LOW0xf6
+/* Hardware defined default addresses for I2C register maps */
+#define ADV7511_CEC_I2C_ADDR_DEFAULT 0x3c
+#define ADV7511_EDID_I2C_ADDR_DEFAULT 0x3f
+#define ADV7511_PACKET_I2C_ADDR_DEFAULT0x38
+
#define ADV7511_CSC_ENABLE BIT(7)
#define ADV7511_CSC_UPDATE_MODEBIT(5)
@@ -321,6 +326,7 @@ enum adv7511_type {
struct adv7511 {
struct i2c_client *i2c_main;
struct i2c_client *i2c_edid;
+ struct i2c_client *i2c_packet;
struct i2c_client *i2c_cec;
struct regmap *regmap;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index efa29db5fc2b..802bc433f54a 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -586,7 +586,7 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
/* Reading the EDID only works if the device is powered */
if (!adv7511->powered) {
unsigned int edid_i2c_addr =
- (adv7511->i2c_main->addr << 1) + 4;
+ (adv7511->i2c_edid->addr << 1);
__adv7511_power_on(adv7511);
@@ -969,10 +969,10 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
{
int ret;
- adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
-adv->i2c_main->addr - 1);
+ adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec",
+ ADV7511_CEC_I2C_ADDR_DEFAULT);
if (!adv->i2c_cec)
- return -ENOMEM;
+ return -EINVAL;
i2c_set_clientdata(adv->i2c_cec, adv);
adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
@@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const
struct i2c_device_id *id)
struct adv7511_link_config link_config;
struct adv7511 *adv7511;
struct device *dev = &i2c->dev;
- unsigned int main_i2c_addr = i2c->addr << 1;
- unsigned int edid_i2c_addr = main_i2c_addr + 4;
unsigned int val;
int ret;
@@ -1153,23 +1151,34 @@ static int adv7511_probe(struct i2c_client *i2c, const
struct i2c_device_id *id)
if (ret)
goto uninit_regulators;
- regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
- regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
-main_i2c_addr - 0xa);
- regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
-main_i2c_addr - 2);
-
adv7511_packet_disable(adv7511, 0x);
- adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
+ adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
+ ADV7511_EDID_I2C_ADDR_DEFAULT);
if (!adv7511->i2c_edid) {
- ret = -ENOMEM;
+ ret = -EINVAL;
goto uninit_regulators;
}
+ regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
+adv7511->i2c_edid->addr << 1);
+
+ adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
+ ADV7511_PACKET_I2C_ADDR_DEFAULT);
+ if (!adv7511->i2c_packet) {
+ ret = -EINVAL;
+ goto err_i2c_unregister_edid;
+ }
+
+ regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
+adv7511->i2c_packet->addr << 1);
+
ret = adv7511_init_cec_regmap(adv7511);
if (ret)
- goto err_i2c_unregister_edid;
+ goto err_i2c_unregister
[PATCH v4 3/5] [RFT] ARM: dts: wheat: Fix ADV7513 address usage
From: Kieran Bingham
The r8a7792 Wheat board has two ADV7513 devices sharing a single I2C
bus, however in low power mode the ADV7513 will reset it's slave maps to
use the hardware defined default addresses.
The ADV7511 driver was adapted to allow the two devices to be registered
correctly - but it did not take into account the fault whereby the
devices reset the addresses.
This results in an address conflict between the device using the default
addresses, and the other device if it is in low-power-mode.
Repair this issue by moving both devices away from the default address
definitions.
Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
---
v2:
- Addition to series
v3:
- Split map register addresses into individual declarations.
v4:
- Normalise I2C usage
arch/arm/boot/dts/r8a7792-wheat.dts | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/r8a7792-wheat.dts
b/arch/arm/boot/dts/r8a7792-wheat.dts
index b9471b67b728..42fff8837eab 100644
--- a/arch/arm/boot/dts/r8a7792-wheat.dts
+++ b/arch/arm/boot/dts/r8a7792-wheat.dts
@@ -240,9 +240,16 @@
status = "okay";
clock-frequency = <40>;
+ /*
+* The adv75xx resets its addresses to defaults during low power power
+* mode. Because we have two ADV7513 devices on the same bus, we must
+* change both of them away from the defaults so that they do not
+* conflict.
+*/
hdmi@3d {
compatible = "adi,adv7513";
- reg = <0x3d>;
+ reg = <0x3d>, <0x2d>, <0x4d>, <0x5d>;
+ reg-names = "main", "cec", "edid", "packet";
adi,input-depth = <8>;
adi,input-colorspace = "rgb";
@@ -272,7 +279,8 @@
hdmi@39 {
compatible = "adi,adv7513";
- reg = <0x39>;
+ reg = <0x39>, <0x29>, <0x49>, <0x59>;
+ reg-names = "main", "cec", "edid", "packet";
adi,input-depth = <8>;
adi,input-colorspace = "rgb";
--
2.7.4
Re: [PATCH 3/5] auxdisplay: charlcd: add escape sequence for brightness on NEC µPD16314
On Mon, Feb 12, 2018 at 12:56:58PM +0100, Miguel Ojeda wrote:
> On Mon, Jan 15, 2018 at 10:58 AM, Sean Young wrote:
> > The NEC µPD16314 can alter the the brightness of the LCD. Make it possible
> > to set this via escape sequence Y0 - Y3. B and R were already taken, so
> > I picked Y for luminance.
> >
> > Signed-off-by: Sean Young
>
> CC'ing Willy and Geert.
>
> > ---
> > drivers/auxdisplay/charlcd.c | 20 ++--
> > 1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
> > index a16c72779722..7a671ad959d1 100644
> > --- a/drivers/auxdisplay/charlcd.c
> > +++ b/drivers/auxdisplay/charlcd.c
> > @@ -39,6 +39,8 @@
> > #define LCD_FLAG_F 0x0020 /* Large font mode */
> > #define LCD_FLAG_N 0x0040 /* 2-rows mode */
> > #define LCD_FLAG_L 0x0080 /* Backlight enabled */
> > +#define LCD_BRIGHTNESS_MASK0x0300 /* Brightness */
> > +#define LCD_BRIGHTNESS_SHIFT 8
>
> Not sure about the name (since the brightness is also used in
> priv->flags). By the way, should we start using the bitops.h stuff
> (e.g. BIT(9) | BIT(8), GENMASK(9, 8)...) in new code? Not sure how
> widespread they are.
The brightness is not really a flag, maybe it belongs in a separate field;
we could use bit fields in order to waste less space.
BIT() would be nicer and is more idiomatic by current standards.
Note that x, y and flags are currently unsigned long, they could be u8.
>
> >
> > /* LCD commands */
> > #define LCD_CMD_DISPLAY_CLEAR 0x01/* Clear entire display */
> > @@ -490,6 +492,17 @@ static inline int handle_lcd_special_code(struct
> > charlcd *lcd)
> > charlcd_gotoxy(lcd);
> > processed = 1;
> > break;
> > + case 'Y': /* brightness (luma) */
> > + switch (esc[1]) {
> > + case '0': /* 25% */
> > + case '1': /* 50% */
> > + case '2': /* 75% */
> > + case '3': /* 100% */
> > + priv->flags = (priv->flags &
> > ~(LCD_BRIGHTNESS_MASK)) |
> > + (('3' - esc[1]) << LCD_BRIGHTNESS_SHIFT);
> > + processed = 1;
> > + break;
> > + }
> > }
> >
> > /* TODO: This indent party here got ugly, clean it! */
> > @@ -507,12 +520,15 @@ static inline int handle_lcd_special_code(struct
> > charlcd *lcd)
> > ((priv->flags & LCD_FLAG_C) ? LCD_CMD_CURSOR_ON :
> > 0) |
> > ((priv->flags & LCD_FLAG_B) ? LCD_CMD_BLINK_ON :
> > 0));
> > /* check whether one of F,N flags was changed */
>
> Should we add "or brightness" to the comment?
Indeed we should.
>
> > - else if ((oldflags ^ priv->flags) & (LCD_FLAG_F | LCD_FLAG_N))
> > + else if ((oldflags ^ priv->flags) & (LCD_FLAG_F | LCD_FLAG_N |
> > +LCD_BRIGHTNESS_MASK))
> > lcd->ops->write_cmd(lcd,
> > LCD_CMD_FUNCTION_SET |
> > ((lcd->ifwidth == 8) ? LCD_CMD_DATA_LEN_8BITS : 0) |
> > ((priv->flags & LCD_FLAG_F) ?
> > LCD_CMD_FONT_5X10_DOTS : 0) |
> > - ((priv->flags & LCD_FLAG_N) ? LCD_CMD_TWO_LINES :
> > 0));
> > + ((priv->flags & LCD_FLAG_N) ? LCD_CMD_TWO_LINES :
> > 0) |
> > + ((priv->flags & LCD_BRIGHTNESS_MASK) >>
> > +
> > LCD_BRIGHTNESS_SHIFT));
> > /* check whether L flag was changed */
> > else if ((oldflags ^ priv->flags) & LCD_FLAG_L)
> > charlcd_backlight(lcd, !!(priv->flags & LCD_FLAG_L));
> > --
> > 2.14.3
> >
I've discovered an issue in my sasem driver, I'll send out a new version
of these patch series once that is resolved.
Thanks,
Sean
Re: [PATCH v10 13/30] rcar-vin: add function to manipulate Gen3 chsel value
Hi Laurent,
On 2018-02-13 19:02:38 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> On Tuesday, 13 February 2018 18:58:09 EET Niklas Söderlund wrote:
> > On 2018-02-13 18:41:33 +0200, Laurent Pinchart wrote:
> > > On Monday, 29 January 2018 18:34:18 EET Niklas Söderlund wrote:
> > > > On Gen3 the CSI-2 routing is controlled by the VnCSI_IFMD register. One
> > > > feature of this register is that it's only present in the VIN0 and VIN4
> > > > instances. The register in VIN0 controls the routing for VIN0-3 and the
> > > > register in VIN4 controls routing for VIN4-7.
> > > >
> > > > To be able to control routing from a media device this function is need
> > > > to control runtime PM for the subgroup master (VIN0 and VIN4). The
> > > > subgroup master must be switched on before the register is manipulated,
> > > > once the operation is complete it's safe to switch the master off and
> > > > the new routing will still be in effect.
> > > >
> > > > Signed-off-by: Niklas Söderlund
> > > > ---
> > >>
> > >> drivers/media/platform/rcar-vin/rcar-dma.c | 28+
> > >> drivers/media/platform/rcar-vin/rcar-vin.h | 2 ++
> > >> 2 files changed, 30 insertions(+)
> > >>
> > >> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > >> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> > >> 2f9ad1bec1c8a92f..ae286742f15a3ab5 100644
> > >> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > >> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > >> @@ -16,6 +16,7 @@
> > >>
> > >> #include
> > >> #include
> > >> +#include
> > >>
> > >> #include
> > >>
> > >> @@ -1228,3 +1229,30 @@ int rvin_dma_register(struct rvin_dev *vin, int
> > >> irq)
> > >> return ret;
> > >> }
> > >>
> > >> +
> > >> +/* -
> > >> + * Gen3 CHSEL manipulation
> > >> + */
> > >> +
> > >> +void rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
> > >> +{
> > >> +u32 ifmd, vnmc;
> > >> +
> > >> +pm_runtime_get_sync(vin->dev);
> > >
> > > No need to check for errors ?
> >
> > You asked the samething for v9 so I will copy paste the same reply :-)
>
> Oh so you expect me to remember what happened with previous versions ? :-)
:-)
>
> > Sakari asked the same thing in v4 :-)
> >
> > In short no its not needed please see Geert's response [1]. If I
> > recall correctly this was also discussed in more detail in another
> > thread for some other driver whit a bit longer answer saying that it
> > pm_runtime_get_sync() fails you have big problems but I can't find
> > that thread now :-(
> >
> > 1. https://www.spinics.net/lists/linux-media/msg115241.html
>
> If kmalloc() fails we also have big problems, but we nonetheless check every
> memory allocation.
I did some quick and dirty statistics for current upstream behavior,
$ git grep pm_runtime_get_sync | wc -l
1044
$ git grep pm_runtime_get_sync | grep = | wc -l
367
It looks like a less then half checks the return value :-) But as it
will take at least one more incarnation of this patch set I will add a
check for it get to the good side of things.
>
> > >> +
> > >> +/* Make register writes take effect immediately */
> > >> +vnmc = rvin_read(vin, VNMC_REG);
> > >> +rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG);
> > >> +
> > >> +ifmd = VNCSI_IFMD_DES2 | VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 |
> > >> +VNCSI_IFMD_CSI_CHSEL(chsel);
> > >> +
> > >> +rvin_write(vin, ifmd, VNCSI_IFMD_REG);
> > >> +
> > >> +vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
> > >> +
> > >> +/* Restore VNMC */
> > >> +rvin_write(vin, vnmc, VNMC_REG);
> > >
> > > No need for locking around all this ? What happens if this VIN instance
> > > decides to write to another VIN register (for instance due to a userpace
> > > call) when this function has disabled VNMC_VUP ?
> >
> > You also asked a related question to this in v9 as a start I will copy
> > in that reply.
> >
> > Media link changes are not allowed when any VIN in the group are
> > streaming so this should not be an issue.
> >
> > And to compliment that. This function is only valid for a VIN which has
> > the CHSEL register which currently is VIN0 and VIN4. It can only be
> > modified when a media link is enabled. Catching media links are only
> > allowed when all VIN in the system are _not_ streaming. And VNMC_VUP is
> > only enabled when a VIN is streaming so there is no need for locking
> > here.
>
> This seems a bit fragile to me, could you please capture the explanation in a
> comment ?
>
Will do.
> > >> +pm_runtime_put(vin->dev);
> > >> +}
> > >> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> > >> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> > >> 146683142e6533fa..a5dae5b5e9cb704b 100644
> > >> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > >> +++ b/drivers/media/platform/rcar-vin/rcar-
Re: [PATCH v3 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 TX Device Tree bindings
Hi Laurent, On Thu, Feb 08, 2018 at 09:00:19PM +0200, Laurent Pinchart wrote: > > + > > + Port Description > > + - > > + 0CSI-2 output > > + 1Stream 0 input > > + 2Stream 1 input > > + 3Stream 2 input > > + 4Stream 3 input > > + > > + The stream input port nodes are optional if they are not > > + connected to anything at the hardware level or implemented > > + in the design. > > Are they optional (and thus valid if present), or should they be forbidden in > case they're not implemented in the hardware ? I'd go for the latter and write > > "One stream input port node is required per implemented hardware input, and > no > stream input port node can be present for unimplemented inputs." That works for me. > > Since there is only one endpoint per port, > > + the endpoints are not numbered. > > I think it would be valid to number endpoints even if not required. I think > that what you should document is that at most one endpoint is supported per > port. Sakari asked to have it worded that way in this review: https://www.spinics.net/lists/linux-media/msg122713.html What should I do? Thanks! Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v10 18/30] rcar-vin: add check for colorspace
Hi Niklas, Thank you for the patch. On Monday, 29 January 2018 18:34:23 EET Niklas Söderlund wrote: > Add a check to ensure the colorspace from user-space is good. On Gen2 it > works without this change as the sensor sets the colorspace but on Gen3 > this can fail if the colorspace provided by the user is not good. The > values to check for comes from v4l2-compliance sources which is the tool > that found this error. If this check is not preformed v4l2-compliance s/preformed/performed/ > fails when it tests colorspace. > > Signed-off-by: Niklas Söderlund > --- > drivers/media/platform/rcar-vin/rcar-v4l2.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index > 841d62ca27e026d7..6403650aff22a2ed 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -23,6 +23,7 @@ > #include "rcar-vin.h" > > #define RVIN_DEFAULT_FORMAT V4L2_PIX_FMT_YUYV > +#define RVIN_DEFAULT_COLORSPACE V4L2_COLORSPACE_SRGB > > /* > * Format Conversions > @@ -115,6 +116,10 @@ static int rvin_format_align(struct rvin_dev *vin, > struct v4l2_pix_format *pix) break; > } > > + /* Check that colorspace is reasonable */ > + if (!pix->colorspace || pix->colorspace >= 0xff) I'd write the first check as pix->colorspace == V4L2_COLORSPACE_DEFAULT For the second check I don't think 0xff is a meaningful value. We currently have 12 colorspaces defined. If we want to be future-proof I'd add a V4L2_COLORSPACE_MAX entry to enum v4l2_colorspace and use that for the check. Alternatively you could use V4L2_COLORSPACE_DCI_P3 but the driver would need to be updated when new colorspaces get added. > + pix->colorspace = RVIN_DEFAULT_COLORSPACE; > + > /* HW limit width to a multiple of 32 (2^5) for NV16 else 2 (2^1) */ > walign = vin->format.pixelformat == V4L2_PIX_FMT_NV16 ? 5 : 1; -- Regards, Laurent Pinchart
Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 TX driver
Hi Laurent,
On Thu, Feb 08, 2018 at 10:05:05PM +0200, Laurent Pinchart wrote:
> On Wednesday, 7 February 2018 16:26:43 EET Maxime Ripard wrote:
> > The Cadence MIPI-CSI2 TX controller is an hardware block meant to be used
> > as a bridge between pixel interfaces and a CSI-2 bus.
> >
> > It supports operating with an internal or external D-PHY, with up to 4
> > lanes, or without any D-PHY. The current code only supports the former
> > case.
> >
> > While the virtual channel input on the pixel interface can be directly
> > mapped to CSI2, the datatype input is actually a selection signal (3-bits)
> > mapping to a table of up to 8 preconfigured datatypes/formats (programmed
> > at start-up)
> >
> > The block supports up to 8 input datatypes.
>
> The DT bindings document four input streams. Does this mean that, to use more
> than 4 data types, a system would need to transmit multiplexed streams on a
> single input stream with the 3 selection bits qualifying each sample ? That
> would be an interesting setup.
My understanding is that each input stream has an additional signal
that defines a data type encoded on 3 bits. So yeah, I guess that
would be possible if the upstream device is able to synchronize its
stream generation and the datatype sent.
> > Signed-off-by: Maxime Ripard
> > ---
> > drivers/media/platform/cadence/Kconfig | 6 +
> > drivers/media/platform/cadence/Makefile | 1 +
> > drivers/media/platform/cadence/cdns-csi2tx.c | 582
> > 3 files changed, 589 insertions(+)
> > create mode 100644 drivers/media/platform/cadence/cdns-csi2tx.c
> >
> > diff --git a/drivers/media/platform/cadence/Kconfig
> > b/drivers/media/platform/cadence/Kconfig index d1b6bbb6a0eb..db49328ee8b2
> > 100644
> > --- a/drivers/media/platform/cadence/Kconfig
> > +++ b/drivers/media/platform/cadence/Kconfig
> > @@ -9,4 +9,10 @@ config VIDEO_CADENCE_CSI2RX
> > depends on VIDEO_V4L2_SUBDEV_API
> > select V4L2_FWNODE
> >
> > +config VIDEO_CADENCE_CSI2TX
> > + tristate "Cadence MIPI-CSI2 TX Controller"
> > + depends on MEDIA_CONTROLLER
> > + depends on VIDEO_V4L2_SUBDEV_API
> > + select V4L2_FWNODE
>
> A bit of documentation maybe ?
Yeah, it was already queued for the next iteration :)
> > +static const struct v4l2_mbus_framefmt fmt_default = {
> > + .width = 320,
> > + .height = 180,
>
> That's a pretty small default resolution. Is there any reason for not using a
> more common format ?
What would be your suggestion?
> > +static int csi2tx_init_cfg(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *cfg)
> > +{
> > + struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
> > + unsigned int i;
> > +
> > + for (i = 0; i < subdev->entity.num_pads; i++)
> > + csi2tx->pad_fmts[i] = fmt_default;
>
> .init_cfg() should initialize the formats stored in the cfg structure. The
> active formats stored in your driver-specific structure should be initialized
> at probe time.
Ok, I'll change it.
> > +static int csi2tx_set_pad_format(struct v4l2_subdev *subdev,
> > +struct v4l2_subdev_pad_config *cfg,
> > +struct v4l2_subdev_format *fmt)
> > +{
> > + struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
> > +
> > + csi2tx->pad_fmts[fmt->pad] = fmt->format;
>
> Doesn't the IP have frame width or height limitations ? Or format
> limitations ?
In its current state, not really. There's also no clear limitations
from the registers on the formats / resolution used, since it's not
configured at all in the device.
The only constraint is that there's no buffer in the IP, so the input
data rate and the output data rate should match. However, the way to
do that is currently uncertain, so I guess we can address that later
on.
> > +static void csi2tx_reset(struct csi2tx_priv *csi2tx)
> > +{
> > + writel(CSI2TX_CONFIG_SRST_REQ, csi2tx->base + CSI2TX_CONFIG_REG);
> > +
> > + usleep_range(10, 20);
>
> As for the RX driver, udelay() might be better. Same comment for the other
> small delays below.
I was asked by Benoit to change it to usleep_range in an earlier
iteration, which one should I use?
I'll change the driver according to your other comments.
Thanks!
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
signature.asc
Description: PGP signature
Re: [PATCH v10 17/30] rcar-vin: update pixelformat check for M1
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:22 EET Niklas Söderlund wrote:
> If the pixelformat is not supported it should not fail but be set to
> something that works. While we are at it move the check together with
> other pixelformat checks of this function.
Please ignore my related comment to patch 16/30 :-) However, could you move
this patch before 16/30 ?
> Signed-off-by: Niklas Söderlund
Reviewed-by: Laurent Pinchart
> ---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 10 --
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> bca6e204a574772f..841d62ca27e026d7 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -97,6 +97,10 @@ static int rvin_format_align(struct rvin_dev *vin, struct
> v4l2_pix_format *pix) pix->pixelformat = RVIN_DEFAULT_FORMAT;
> }
>
> + if (vin->info->model == RCAR_M1 &&
> + pix->pixelformat == V4L2_PIX_FMT_XBGR32)
> + pix->pixelformat = RVIN_DEFAULT_FORMAT;
> +
> /* Reject ALTERNATE until support is added to the driver */
> switch (pix->field) {
> case V4L2_FIELD_TOP:
> @@ -121,12 +125,6 @@ static int rvin_format_align(struct rvin_dev *vin,
> struct v4l2_pix_format *pix) pix->bytesperline =
> rvin_format_bytesperline(pix);
> pix->sizeimage = rvin_format_sizeimage(pix);
>
> - if (vin->info->model == RCAR_M1 &&
> - pix->pixelformat == V4L2_PIX_FMT_XBGR32) {
> - vin_err(vin, "pixel format XBGR32 not supported on M1\n");
> - return -EINVAL;
> - }
> -
> vin_dbg(vin, "Format %ux%u bpl: %d size: %d\n",
> pix->width, pix->height, pix->bytesperline, pix->sizeimage);
--
Regards,
Laurent Pinchart
Re: [PATCH v10 13/30] rcar-vin: add function to manipulate Gen3 chsel value
Hi Niklas,
On Tuesday, 13 February 2018 18:58:09 EET Niklas Söderlund wrote:
> On 2018-02-13 18:41:33 +0200, Laurent Pinchart wrote:
> > On Monday, 29 January 2018 18:34:18 EET Niklas Söderlund wrote:
> > > On Gen3 the CSI-2 routing is controlled by the VnCSI_IFMD register. One
> > > feature of this register is that it's only present in the VIN0 and VIN4
> > > instances. The register in VIN0 controls the routing for VIN0-3 and the
> > > register in VIN4 controls routing for VIN4-7.
> > >
> > > To be able to control routing from a media device this function is need
> > > to control runtime PM for the subgroup master (VIN0 and VIN4). The
> > > subgroup master must be switched on before the register is manipulated,
> > > once the operation is complete it's safe to switch the master off and
> > > the new routing will still be in effect.
> > >
> > > Signed-off-by: Niklas Söderlund
> > > ---
> >>
> >> drivers/media/platform/rcar-vin/rcar-dma.c | 28+
> >> drivers/media/platform/rcar-vin/rcar-vin.h | 2 ++
> >> 2 files changed, 30 insertions(+)
> >>
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> >> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> >> 2f9ad1bec1c8a92f..ae286742f15a3ab5 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> >> @@ -16,6 +16,7 @@
> >>
> >> #include
> >> #include
> >> +#include
> >>
> >> #include
> >>
> >> @@ -1228,3 +1229,30 @@ int rvin_dma_register(struct rvin_dev *vin, int
> >> irq)
> >>return ret;
> >> }
> >>
> >> +
> >> +/* -
> >> + * Gen3 CHSEL manipulation
> >> + */
> >> +
> >> +void rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
> >> +{
> >> + u32 ifmd, vnmc;
> >> +
> >> + pm_runtime_get_sync(vin->dev);
> >
> > No need to check for errors ?
>
> You asked the samething for v9 so I will copy paste the same reply :-)
Oh so you expect me to remember what happened with previous versions ? :-)
> Sakari asked the same thing in v4 :-)
>
> In short no its not needed please see Geert's response [1]. If I
> recall correctly this was also discussed in more detail in another
> thread for some other driver whit a bit longer answer saying that it
> pm_runtime_get_sync() fails you have big problems but I can't find
> that thread now :-(
>
> 1. https://www.spinics.net/lists/linux-media/msg115241.html
If kmalloc() fails we also have big problems, but we nonetheless check every
memory allocation.
> >> +
> >> + /* Make register writes take effect immediately */
> >> + vnmc = rvin_read(vin, VNMC_REG);
> >> + rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG);
> >> +
> >> + ifmd = VNCSI_IFMD_DES2 | VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 |
> >> + VNCSI_IFMD_CSI_CHSEL(chsel);
> >> +
> >> + rvin_write(vin, ifmd, VNCSI_IFMD_REG);
> >> +
> >> + vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
> >> +
> >> + /* Restore VNMC */
> >> + rvin_write(vin, vnmc, VNMC_REG);
> >
> > No need for locking around all this ? What happens if this VIN instance
> > decides to write to another VIN register (for instance due to a userpace
> > call) when this function has disabled VNMC_VUP ?
>
> You also asked a related question to this in v9 as a start I will copy
> in that reply.
>
> Media link changes are not allowed when any VIN in the group are
> streaming so this should not be an issue.
>
> And to compliment that. This function is only valid for a VIN which has
> the CHSEL register which currently is VIN0 and VIN4. It can only be
> modified when a media link is enabled. Catching media links are only
> allowed when all VIN in the system are _not_ streaming. And VNMC_VUP is
> only enabled when a VIN is streaming so there is no need for locking
> here.
This seems a bit fragile to me, could you please capture the explanation in a
comment ?
> >> + pm_runtime_put(vin->dev);
> >> +}
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> >> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> >> 146683142e6533fa..a5dae5b5e9cb704b 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> >> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> >> @@ -165,4 +165,6 @@ const struct rvin_video_format
> >> *rvin_format_from_pixel(u32 pixelformat); /* Cropping, composing and
> >> scaling */
> >>
> >> void rvin_crop_scale_comp(struct rvin_dev *vin);
> >>
> >> +void rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
> >> +
> >> #endif
--
Regards,
Laurent Pinchart
Re: [PATCH v10 16/30] rcar-vin: update bytesperline and sizeimage calculation
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:21 EET Niklas Söderlund wrote:
> Remove over complicated logic to calculate the value for bytesperline
s/over complicated/overcomplicated/
> and sizeimage that was carried over from the soc_camera port. Update the
> calculations to match how other drivers are doing it.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 11 ++-
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 1169e6a279ecfb55..bca6e204a574772f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -118,10 +118,8 @@ static int rvin_format_align(struct rvin_dev *vin,
> struct v4l2_pix_format *pix) v4l_bound_align_image(&pix->width, 2,
> vin->info->max_width, walign, &pix->height, 4, vin->info->max_height, 2,
> 0);
>
> - pix->bytesperline = max_t(u32, pix->bytesperline,
> - rvin_format_bytesperline(pix));
> - pix->sizeimage = max_t(u32, pix->sizeimage,
> -rvin_format_sizeimage(pix));
> + pix->bytesperline = rvin_format_bytesperline(pix);
> + pix->sizeimage = rvin_format_sizeimage(pix);
Thus this mean that the driver will stop supporting configurable strides ?
Isn't that a regression ?
> if (vin->info->model == RCAR_M1 &&
> pix->pixelformat == V4L2_PIX_FMT_XBGR32) {
> @@ -270,11 +268,6 @@ static int __rvin_try_format(struct rvin_dev *vin,
> if (pix->field == V4L2_FIELD_ANY)
> pix->field = vin->format.field;
>
> -
> - /* Always recalculate */
> - pix->bytesperline = 0;
> - pix->sizeimage = 0;
> -
> /* Limit to source capabilities */
> ret = __rvin_try_format_source(vin, which, pix);
> if (ret)
--
Regards,
Laurent Pinchart
Re: [PATCH v10 13/30] rcar-vin: add function to manipulate Gen3 chsel value
Hi Laurent,
On 2018-02-13 18:41:33 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Monday, 29 January 2018 18:34:18 EET Niklas Söderlund wrote:
> > On Gen3 the CSI-2 routing is controlled by the VnCSI_IFMD register. One
> > feature of this register is that it's only present in the VIN0 and VIN4
> > instances. The register in VIN0 controls the routing for VIN0-3 and the
> > register in VIN4 controls routing for VIN4-7.
> >
> > To be able to control routing from a media device this function is need
> > to control runtime PM for the subgroup master (VIN0 and VIN4). The
> > subgroup master must be switched on before the register is manipulated,
> > once the operation is complete it's safe to switch the master off and
> > the new routing will still be in effect.
> >
> > Signed-off-by: Niklas Söderlund
> > ---
> > drivers/media/platform/rcar-vin/rcar-dma.c | 28 +++
> > drivers/media/platform/rcar-vin/rcar-vin.h | 2 ++
> > 2 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > b/drivers/media/platform/rcar-vin/rcar-dma.c index
> > 2f9ad1bec1c8a92f..ae286742f15a3ab5 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -16,6 +16,7 @@
> >
> > #include
> > #include
> > +#include
> >
> > #include
> >
> > @@ -1228,3 +1229,30 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
> >
> > return ret;
> > }
> > +
> > +/*
> > + * Gen3 CHSEL manipulation
> > + */
> > +
> > +void rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
> > +{
> > + u32 ifmd, vnmc;
> > +
> > + pm_runtime_get_sync(vin->dev);
>
> No need to check for errors ?
You asked the samething for v9 so I will copy paste the same reply :-)
Sakari asked the same thing in v4 :-)
In short no its not needed please see Geert's response [1]. If I
recall correctly this was also discussed in more detail in another
thread for some other driver whit a bit longer answer saying that it
pm_runtime_get_sync() fails you have big problems but I can't find
that thread now :-(
1. https://www.spinics.net/lists/linux-media/msg115241.html
>
> > +
> > + /* Make register writes take effect immediately */
> > + vnmc = rvin_read(vin, VNMC_REG);
> > + rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG);
> > +
> > + ifmd = VNCSI_IFMD_DES2 | VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 |
> > + VNCSI_IFMD_CSI_CHSEL(chsel);
> > +
> > + rvin_write(vin, ifmd, VNCSI_IFMD_REG);
> > +
> > + vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
> > +
> > + /* Restore VNMC */
> > + rvin_write(vin, vnmc, VNMC_REG);
>
> No need for locking around all this ? What happens if this VIN instance
> decides to write to another VIN register (for instance due to a userpace
> call)
> when this function has disabled VNMC_VUP ?
You also asked a related question to this in v9 as a start I will copy
in that reply.
Media link changes are not allowed when any VIN in the group are
streaming so this should not be an issue.
And to compliment that. This function is only valid for a VIN which has
the CHSEL register which currently is VIN0 and VIN4. It can only be
modified when a media link is enabled. Catching media links are only
allowed when all VIN in the system are _not_ streaming. And VNMC_VUP is
only enabled when a VIN is streaming so there is no need for locking
here.
>
> > + pm_runtime_put(vin->dev);
> > +}
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> > b/drivers/media/platform/rcar-vin/rcar-vin.h index
> > 146683142e6533fa..a5dae5b5e9cb704b 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -165,4 +165,6 @@ const struct rvin_video_format
> > *rvin_format_from_pixel(u32 pixelformat); /* Cropping, composing and
> > scaling */
> > void rvin_crop_scale_comp(struct rvin_dev *vin);
> >
> > +void rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
> > +
> > #endif
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Regards,
Niklas Söderlund
Re: [PATCH v10 15/30] rcar-vin: break out format alignment and checking
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:20 EET Niklas Söderlund wrote:
> Part of the format alignment and checking can be shared with the Gen3
> format handling. Break that part out to a separate function.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 93 +---
> 1 file changed, 50 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> c606942e59b5d934..1169e6a279ecfb55 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -86,6 +86,55 @@ static u32 rvin_format_sizeimage(struct v4l2_pix_format
> *pix) return pix->bytesperline * pix->height;
> }
>
> +static int rvin_format_align(struct rvin_dev *vin, struct v4l2_pix_format
> *pix)
> +{
> + u32 walign;
> +
> + /* If requested format is not supported fallback to the default */
> + if (!rvin_format_from_pixel(pix->pixelformat)) {
> + vin_dbg(vin, "Format 0x%x not found, using default 0x%x\n",
> + pix->pixelformat, RVIN_DEFAULT_FORMAT);
I think you can drop the message.
> + pix->pixelformat = RVIN_DEFAULT_FORMAT;
> + }
> +
> + /* Reject ALTERNATE until support is added to the driver */
> + switch (pix->field) {
> + case V4L2_FIELD_TOP:
> + case V4L2_FIELD_BOTTOM:
> + case V4L2_FIELD_NONE:
> + case V4L2_FIELD_INTERLACED_TB:
> + case V4L2_FIELD_INTERLACED_BT:
> + case V4L2_FIELD_INTERLACED:
> + break;
> + default:
> + pix->field = V4L2_FIELD_NONE;
> + break;
> + }
> +
> + /* HW limit width to a multiple of 32 (2^5) for NV16 else 2 (2^1) */
> + walign = vin->format.pixelformat == V4L2_PIX_FMT_NV16 ? 5 : 1;
> +
> + /* Limit to VIN capabilities */
> + v4l_bound_align_image(&pix->width, 2, vin->info->max_width, walign,
> + &pix->height, 4, vin->info->max_height, 2, 0);
> +
> + pix->bytesperline = max_t(u32, pix->bytesperline,
> + rvin_format_bytesperline(pix));
> + pix->sizeimage = max_t(u32, pix->sizeimage,
> +rvin_format_sizeimage(pix));
> +
> + if (vin->info->model == RCAR_M1 &&
> + pix->pixelformat == V4L2_PIX_FMT_XBGR32) {
> + vin_err(vin, "pixel format XBGR32 not supported on M1\n");
> + return -EINVAL;
This shouldn't print a message nor return an error, but default to a supported
format. You can move the check to the beginning of the function to do so.
> + }
> +
> + vin_dbg(vin, "Format %ux%u bpl: %d size: %d\n",
> + pix->width, pix->height, pix->bytesperline, pix->sizeimage);
> +
> + return 0;
> +}
> +
> /*
> ---
> -- * V4L2
> */
> @@ -215,19 +264,12 @@ static int __rvin_try_format_source(struct rvin_dev
> *vin, static int __rvin_try_format(struct rvin_dev *vin,
>u32 which, struct v4l2_pix_format *pix)
> {
> - u32 walign;
> int ret;
>
> /* Keep current field if no specific one is asked for */
> if (pix->field == V4L2_FIELD_ANY)
> pix->field = vin->format.field;
>
> - /* If requested format is not supported fallback to the default */
> - if (!rvin_format_from_pixel(pix->pixelformat)) {
> - vin_dbg(vin, "Format 0x%x not found, using default 0x%x\n",
> - pix->pixelformat, RVIN_DEFAULT_FORMAT);
> - pix->pixelformat = RVIN_DEFAULT_FORMAT;
> - }
>
> /* Always recalculate */
> pix->bytesperline = 0;
> @@ -238,42 +280,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
> if (ret)
> return ret;
>
> - /* Reject ALTERNATE until support is added to the driver */
> - switch (pix->field) {
> - case V4L2_FIELD_TOP:
> - case V4L2_FIELD_BOTTOM:
> - case V4L2_FIELD_NONE:
> - case V4L2_FIELD_INTERLACED_TB:
> - case V4L2_FIELD_INTERLACED_BT:
> - case V4L2_FIELD_INTERLACED:
> - break;
> - default:
> - pix->field = V4L2_FIELD_NONE;
> - break;
> - }
> -
> - /* HW limit width to a multiple of 32 (2^5) for NV16 else 2 (2^1) */
> - walign = vin->format.pixelformat == V4L2_PIX_FMT_NV16 ? 5 : 1;
> -
> - /* Limit to VIN capabilities */
> - v4l_bound_align_image(&pix->width, 2, vin->info->max_width, walign,
> - &pix->height, 4, vin->info->max_height, 2, 0);
> -
> - pix->bytesperline = max_t(u32, pix->bytesperline,
> - rvin_format_bytesperline(pix));
> - pix->sizeimage = max_t(u32, pix->sizeimage,
> -rvin_format_sizeimage(pix));
> -
> - if (vin->info->model == RCAR_M1 &&
> - pix->pixelformat == V4L2_PIX_FMT_XBG
Re: [PATCH v10 10/30] rcar-vin: fix handling of single field frames (top, bottom and alternate fields)
Hi Laurent,
On 2018-02-13 18:26:34 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
Thanks for your comments.
>
> On Monday, 29 January 2018 18:34:15 EET Niklas Söderlund wrote:
> > There was never proper support in the VIN driver to deliver ALTERNATING
> > field format to user-space, remove this field option. The problem is
> > that ALTERNATING filed order requires the sequence numbers of buffers
> > returned to userspace to reflect if fields where dropped or not,
> > something which is not possible with the VIN drivers capture logic.
> >
> > The VIN driver can still capture from a video source which delivers
> > frames in ALTERNATING field order, but needs to combine them using the
> > VIN hardware into INTERLACED field order. Before this change if a source
> > was delivering fields using ALTERNATE the driver would default to
> > combining them using this hardware feature. Only if the user explicitly
> > requested ALTERNATE filed order would incorrect frames be delivered.
> >
> > The height should not be cut in half for the format for TOP or BOTTOM
> > fields settings. This was a mistake and it was made visible by the
> > scaling refactoring. Correct behavior is that the user should request a
> > frame size that fits the half height frame reflected in the field
> > setting. If not the VIN will do its best to scale the top or bottom to
> > the requested format and cropping and scaling do not work as expected.
> >
> > Signed-off-by: Niklas Söderlund
> > Reviewed-by: Hans Verkuil
> > ---
> > drivers/media/platform/rcar-vin/rcar-dma.c | 15 +---
> > drivers/media/platform/rcar-vin/rcar-v4l2.c | 53 --
> > 2 files changed, 24 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > b/drivers/media/platform/rcar-vin/rcar-dma.c index
> > fd14be20a6604d7a..c8831e189d362c8b 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -617,7 +617,6 @@ static int rvin_setup(struct rvin_dev *vin)
> > case V4L2_FIELD_INTERLACED_BT:
> > vnmc = VNMC_IM_FULL | VNMC_FOC;
> > break;
> > - case V4L2_FIELD_ALTERNATE:
> > case V4L2_FIELD_NONE:
> > if (vin->continuous) {
> > vnmc = VNMC_IM_ODD_EVEN;
> > @@ -757,18 +756,6 @@ static int rvin_get_active_slot(struct rvin_dev *vin,
> > u32 vnms) return 0;
> > }
> >
> > -static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32
> > vnms)
> > -{
> > - if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> > - /* If FS is set it's a Even field */
> > - if (vnms & VNMS_FS)
> > - return V4L2_FIELD_BOTTOM;
> > - return V4L2_FIELD_TOP;
> > - }
> > -
> > - return vin->format.field;
> > -}
> > -
> > static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t
> > addr) {
> > const struct rvin_video_format *fmt;
> > @@ -941,7 +928,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
> > goto done;
> >
> > /* Capture frame */
> > - vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> > + vin->queue_buf[slot]->field = vin->format.field;
> > vin->queue_buf[slot]->sequence = sequence;
> > vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> > vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > 4d5be2d0c79c9c9a..9f7902d29c62e205 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -103,6 +103,28 @@ static int rvin_get_source_format(struct rvin_dev *vin,
> > if (ret)
> > return ret;
> >
> > + switch (fmt.format.field) {
> > + case V4L2_FIELD_TOP:
> > + case V4L2_FIELD_BOTTOM:
> > + case V4L2_FIELD_NONE:
> > + case V4L2_FIELD_INTERLACED_TB:
> > + case V4L2_FIELD_INTERLACED_BT:
> > + case V4L2_FIELD_INTERLACED:
> > + break;
> > + case V4L2_FIELD_ALTERNATE:
> > + /*
> > +* Driver do not (yet) support outputting ALTERNATE to a
> > +* userspace. It dose support outputting INTERLACED so use
>
> s/dose/does/
>
> > +* the VIN hardware to combine the two fields.
> > +*/
> > + fmt.format.field = V4L2_FIELD_INTERLACED;
> > + fmt.format.height *= 2;
> > + break;
>
> I don't like this much. The rvin_get_source_format() function is supposed to
> return the media bus format for the bus between the source and the VIN. It's
> the caller that should take the field limitations into account, otherwise you
> end up with a mix of source and VIN data in the same structure.
When I read your comments I understand your argument better. And I
understand this function is perhaps poorly named. Maybe it should be
renamed to rvi
Re: [PATCH v10 13/30] rcar-vin: add function to manipulate Gen3 chsel value
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:18 EET Niklas Söderlund wrote:
> On Gen3 the CSI-2 routing is controlled by the VnCSI_IFMD register. One
> feature of this register is that it's only present in the VIN0 and VIN4
> instances. The register in VIN0 controls the routing for VIN0-3 and the
> register in VIN4 controls routing for VIN4-7.
>
> To be able to control routing from a media device this function is need
> to control runtime PM for the subgroup master (VIN0 and VIN4). The
> subgroup master must be switched on before the register is manipulated,
> once the operation is complete it's safe to switch the master off and
> the new routing will still be in effect.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 28 +++
> drivers/media/platform/rcar-vin/rcar-vin.h | 2 ++
> 2 files changed, 30 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> 2f9ad1bec1c8a92f..ae286742f15a3ab5 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -16,6 +16,7 @@
>
> #include
> #include
> +#include
>
> #include
>
> @@ -1228,3 +1229,30 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
>
> return ret;
> }
> +
> +/*
> + * Gen3 CHSEL manipulation
> + */
> +
> +void rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
> +{
> + u32 ifmd, vnmc;
> +
> + pm_runtime_get_sync(vin->dev);
No need to check for errors ?
> +
> + /* Make register writes take effect immediately */
> + vnmc = rvin_read(vin, VNMC_REG);
> + rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG);
> +
> + ifmd = VNCSI_IFMD_DES2 | VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 |
> + VNCSI_IFMD_CSI_CHSEL(chsel);
> +
> + rvin_write(vin, ifmd, VNCSI_IFMD_REG);
> +
> + vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
> +
> + /* Restore VNMC */
> + rvin_write(vin, vnmc, VNMC_REG);
No need for locking around all this ? What happens if this VIN instance
decides to write to another VIN register (for instance due to a userpace call)
when this function has disabled VNMC_VUP ?
> + pm_runtime_put(vin->dev);
> +}
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> 146683142e6533fa..a5dae5b5e9cb704b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -165,4 +165,6 @@ const struct rvin_video_format
> *rvin_format_from_pixel(u32 pixelformat); /* Cropping, composing and
> scaling */
> void rvin_crop_scale_comp(struct rvin_dev *vin);
>
> +void rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
> +
> #endif
--
Regards,
Laurent Pinchart
Re: [PATCH v10 11/30] rcar-vin: move media bus configuration to struct rvin_info
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:16 EET Niklas Söderlund wrote:
> Bus configuration will once the driver is extended to support Gen3
> contain information not specific to only the directly connected parallel
> subdevice. Move it to struct rvin_dev to show it's not always coupled
> to the parallel subdevice.
The subject line still mentions rvin_info instead of rvin_dev.
> Signed-off-by: Niklas Söderlund
> Reviewed-by: Hans Verkuil
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 18 +-
> drivers/media/platform/rcar-vin/rcar-dma.c | 11 ++-
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +-
> drivers/media/platform/rcar-vin/rcar-vin.h | 9 -
> 4 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> cc863e4ec9a4d4b3..ce1c90405c6002eb 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -65,10 +65,10 @@ static int rvin_digital_subdevice_attach(struct rvin_dev
> *vin, vin->digital->sink_pad = ret < 0 ? 0 : ret;
>
> /* Find compatible subdevices mbus format */
> - vin->digital->code = 0;
> + vin->code = 0;
> code.index = 0;
> code.pad = vin->digital->source_pad;
> - while (!vin->digital->code &&
> + while (!vin->code &&
> !v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
> code.index++;
> switch (code.code) {
> @@ -76,16 +76,16 @@ static int rvin_digital_subdevice_attach(struct rvin_dev
> *vin, case MEDIA_BUS_FMT_UYVY8_2X8:
> case MEDIA_BUS_FMT_UYVY10_2X10:
> case MEDIA_BUS_FMT_RGB888_1X24:
> - vin->digital->code = code.code;
> + vin->code = code.code;
> vin_dbg(vin, "Found media bus format for %s: %d\n",
> - subdev->name, vin->digital->code);
> + subdev->name, vin->code);
> break;
> default:
> break;
> }
> }
>
> - if (!vin->digital->code) {
> + if (!vin->code) {
> vin_err(vin, "Unsupported media bus format for %s\n",
> subdev->name);
> return -EINVAL;
> @@ -190,16 +190,16 @@ static int rvin_digital_parse_v4l2(struct device *dev,
> if (vep->base.port || vep->base.id)
> return -ENOTCONN;
>
> - rvge->mbus_cfg.type = vep->bus_type;
> + vin->mbus_cfg.type = vep->bus_type;
>
> - switch (rvge->mbus_cfg.type) {
> + switch (vin->mbus_cfg.type) {
> case V4L2_MBUS_PARALLEL:
> vin_dbg(vin, "Found PARALLEL media bus\n");
> - rvge->mbus_cfg.flags = vep->bus.parallel.flags;
> + vin->mbus_cfg.flags = vep->bus.parallel.flags;
> break;
> case V4L2_MBUS_BT656:
> vin_dbg(vin, "Found BT656 media bus\n");
> - rvge->mbus_cfg.flags = 0;
> + vin->mbus_cfg.flags = 0;
> break;
> default:
> vin_err(vin, "Unknown media bus type\n");
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> c8831e189d362c8b..561500f65cfa2e74 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -633,7 +633,7 @@ static int rvin_setup(struct rvin_dev *vin)
> /*
>* Input interface
>*/
> - switch (vin->digital->code) {
> + switch (vin->code) {
> case MEDIA_BUS_FMT_YUYV8_1X16:
> /* BT.601/BT.1358 16bit YCbCr422 */
> vnmc |= VNMC_INF_YUV16;
> @@ -641,7 +641,7 @@ static int rvin_setup(struct rvin_dev *vin)
> break;
> case MEDIA_BUS_FMT_UYVY8_2X8:
> /* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
> - vnmc |= vin->digital->mbus_cfg.type == V4L2_MBUS_BT656 ?
> + vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ?
> VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
> input_is_yuv = true;
> break;
> @@ -650,7 +650,7 @@ static int rvin_setup(struct rvin_dev *vin)
> break;
> case MEDIA_BUS_FMT_UYVY10_2X10:
> /* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */
> - vnmc |= vin->digital->mbus_cfg.type == V4L2_MBUS_BT656 ?
> + vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ?
> VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601;
> input_is_yuv = true;
> break;
> @@ -662,11 +662,11 @@ static int rvin_setup(struct rvin_dev *vin)
> dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
>
> /* Hsync Signal Polarity Select */
> - if (!(vin->digital->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
> + if (!(vin->mbus_cfg.
Re: [PATCH v10 10/30] rcar-vin: fix handling of single field frames (top, bottom and alternate fields)
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:15 EET Niklas Söderlund wrote:
> There was never proper support in the VIN driver to deliver ALTERNATING
> field format to user-space, remove this field option. The problem is
> that ALTERNATING filed order requires the sequence numbers of buffers
> returned to userspace to reflect if fields where dropped or not,
> something which is not possible with the VIN drivers capture logic.
>
> The VIN driver can still capture from a video source which delivers
> frames in ALTERNATING field order, but needs to combine them using the
> VIN hardware into INTERLACED field order. Before this change if a source
> was delivering fields using ALTERNATE the driver would default to
> combining them using this hardware feature. Only if the user explicitly
> requested ALTERNATE filed order would incorrect frames be delivered.
>
> The height should not be cut in half for the format for TOP or BOTTOM
> fields settings. This was a mistake and it was made visible by the
> scaling refactoring. Correct behavior is that the user should request a
> frame size that fits the half height frame reflected in the field
> setting. If not the VIN will do its best to scale the top or bottom to
> the requested format and cropping and scaling do not work as expected.
>
> Signed-off-by: Niklas Söderlund
> Reviewed-by: Hans Verkuil
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 15 +---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 53 --
> 2 files changed, 24 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> fd14be20a6604d7a..c8831e189d362c8b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -617,7 +617,6 @@ static int rvin_setup(struct rvin_dev *vin)
> case V4L2_FIELD_INTERLACED_BT:
> vnmc = VNMC_IM_FULL | VNMC_FOC;
> break;
> - case V4L2_FIELD_ALTERNATE:
> case V4L2_FIELD_NONE:
> if (vin->continuous) {
> vnmc = VNMC_IM_ODD_EVEN;
> @@ -757,18 +756,6 @@ static int rvin_get_active_slot(struct rvin_dev *vin,
> u32 vnms) return 0;
> }
>
> -static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32
> vnms)
> -{
> - if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> - /* If FS is set it's a Even field */
> - if (vnms & VNMS_FS)
> - return V4L2_FIELD_BOTTOM;
> - return V4L2_FIELD_TOP;
> - }
> -
> - return vin->format.field;
> -}
> -
> static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t
> addr) {
> const struct rvin_video_format *fmt;
> @@ -941,7 +928,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
> goto done;
>
> /* Capture frame */
> - vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> + vin->queue_buf[slot]->field = vin->format.field;
> vin->queue_buf[slot]->sequence = sequence;
> vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 4d5be2d0c79c9c9a..9f7902d29c62e205 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -103,6 +103,28 @@ static int rvin_get_source_format(struct rvin_dev *vin,
> if (ret)
> return ret;
>
> + switch (fmt.format.field) {
> + case V4L2_FIELD_TOP:
> + case V4L2_FIELD_BOTTOM:
> + case V4L2_FIELD_NONE:
> + case V4L2_FIELD_INTERLACED_TB:
> + case V4L2_FIELD_INTERLACED_BT:
> + case V4L2_FIELD_INTERLACED:
> + break;
> + case V4L2_FIELD_ALTERNATE:
> + /*
> + * Driver do not (yet) support outputting ALTERNATE to a
> + * userspace. It dose support outputting INTERLACED so use
s/dose/does/
> + * the VIN hardware to combine the two fields.
> + */
> + fmt.format.field = V4L2_FIELD_INTERLACED;
> + fmt.format.height *= 2;
> + break;
I don't like this much. The rvin_get_source_format() function is supposed to
return the media bus format for the bus between the source and the VIN. It's
the caller that should take the field limitations into account, otherwise you
end up with a mix of source and VIN data in the same structure.
> + default:
> + vin->format.field = V4L2_FIELD_NONE;
> + break;
> + }
> +
> memcpy(mbus_fmt, &fmt.format, sizeof(*mbus_fmt));
>
> return 0;
> @@ -139,33 +161,6 @@ static int rvin_reset_format(struct rvin_dev *vin)
>
> v4l2_fill_pix_format(&vin->format, &source_fmt);
>
> - /*
> - * If the subdevice uses ALTERNATE field
Re: [PATCH v10 09/30] rcar-vin: read subdevice format for crop only when needed
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:14 EET Niklas Söderlund wrote:
> Instead of caching the subdevice format each time the video device
> format is set read it directly when it's needed. As it turns out the
> format is only needed when figuring out the max rectangle for cropping.
>
> This simplifies the code and makes it clearer what the source format is
> used for.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 112 +---
> drivers/media/platform/rcar-vin/rcar-vin.h | 12 ---
> 2 files changed, 61 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> c2265324c7c96308..4d5be2d0c79c9c9a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -90,35 +90,54 @@ static u32 rvin_format_sizeimage(struct v4l2_pix_format
> *pix) * V4L2
> */
>
> -static void rvin_reset_crop_compose(struct rvin_dev *vin)
> +static int rvin_get_source_format(struct rvin_dev *vin,
> + struct v4l2_mbus_framefmt *mbus_fmt)
> {
> + struct v4l2_subdev_format fmt = {
> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + .pad = vin->digital->source_pad,
> + };
> + int ret;
> +
> + ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt);
> + if (ret)
> + return ret;
> +
> + memcpy(mbus_fmt, &fmt.format, sizeof(*mbus_fmt));
You can use
*mbus_fmt = fmt.format;
That way the compiler will catch more mistakes, for instance incompatible
types between the two arguments.
> +
> + return 0;
> +}
> +
> +static int rvin_reset_crop_compose(struct rvin_dev *vin)
> +{
> + struct v4l2_mbus_framefmt source_fmt;
> + int ret;
> +
> + ret = rvin_get_source_format(vin, &source_fmt);
> + if (ret)
> + return ret;
> +
> vin->crop.top = vin->crop.left = 0;
> - vin->crop.width = vin->source.width;
> - vin->crop.height = vin->source.height;
> + vin->crop.width = source_fmt.width;
> + vin->crop.height = source_fmt.height;
>
> vin->compose.top = vin->compose.left = 0;
> vin->compose.width = vin->format.width;
> vin->compose.height = vin->format.height;
> +
> + return 0;
> }
>
> static int rvin_reset_format(struct rvin_dev *vin)
> {
> - struct v4l2_subdev_format fmt = {
> - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> - };
> - struct v4l2_mbus_framefmt *mf = &fmt.format;
> + struct v4l2_mbus_framefmt source_fmt;
> int ret;
>
> - fmt.pad = vin->digital->source_pad;
> -
> - ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt);
> + ret = rvin_get_source_format(vin, &source_fmt);
> if (ret)
> return ret;
You retrieve the source format once here...
> - vin->format.width = mf->width;
> - vin->format.height = mf->height;
> - vin->format.colorspace = mf->colorspace;
> - vin->format.field = mf->field;
> + v4l2_fill_pix_format(&vin->format, &source_fmt);
>
> /*
>* If the subdevice uses ALTERNATE field mode and G_STD is
> @@ -147,7 +166,9 @@ static int rvin_reset_format(struct rvin_dev *vin)
> break;
> }
>
> - rvin_reset_crop_compose(vin);
> + ret = rvin_reset_crop_compose(vin);
... and this function then retrieves it a second time. Can't you pass it to
rvin_reset_crop_compose() ? If the source changes its format autonomously
between the two calls you'll end up with an inconsistent result otherwise.
> + if (ret)
> + return ret;
>
> vin->format.bytesperline = rvin_format_bytesperline(&vin->format);
> vin->format.sizeimage = rvin_format_sizeimage(&vin->format);
> @@ -156,9 +177,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
> }
>
> static int __rvin_try_format_source(struct rvin_dev *vin,
> - u32 which,
> - struct v4l2_pix_format *pix,
> - struct rvin_source_fmt *source)
> + u32 which, struct v4l2_pix_format *pix)
> {
> struct v4l2_subdev *sd;
> struct v4l2_subdev_pad_config *pad_cfg;
> @@ -190,25 +209,16 @@ static int __rvin_try_format_source(struct rvin_dev
> *vin,
>
> v4l2_fill_pix_format(pix, &format.format);
>
> - source->width = pix->width;
> - source->height = pix->height;
> -
> pix->field = field;
> pix->width = width;
> pix->height = height;
> -
> - vin_dbg(vin, "Source resolution: %ux%u\n", source->width,
> - source->height);
> -
> done:
> v4l2_subdev_free_pad_config(pad_cfg);
> return ret;
> }
>
> static int __rvin_try_format(struct rvin_dev *vin,
> - u32 which,
> - struct v4l2_pi
Re: [PATCH v10 04/30] rcar-vin: move subdevice handling to async callbacks
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:09 EET Niklas Söderlund wrote:
> In preparation for Gen3 support move the subdevice initialization and
> clean up from rvin_v4l2_{register,unregister}() directly to the async
> callbacks. This simplifies the addition of Gen3 support as the
> rvin_v4l2_register() can be shared for both Gen2 and Gen3 while direct
> subdevice control are only used on Gen2.
>
> While moving this code drop a large comment which is copied from the
> framework documentation and fold rvin_mbus_supported() into its only
> caller. Also move the initialization and cleanup code to separate
> functions to increase readability.
>
> Signed-off-by: Niklas Söderlund
Reviewed-by: Laurent Pinchart
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 108 ++--
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 35 -
> 2 files changed, 74 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 47f06acde2e698f2..663309ca9c04f208 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -46,46 +46,88 @@ static int rvin_find_pad(struct v4l2_subdev *sd, int
> direction) return -EINVAL;
> }
>
> -static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
> +/* The vin lock shuld be held when calling the subdevice attach and detach
> */ +static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
> + struct v4l2_subdev *subdev)
> {
> - struct v4l2_subdev *sd = entity->subdev;
> struct v4l2_subdev_mbus_code_enum code = {
> .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> };
> + int ret;
>
> + /* Find source and sink pad of remote subdevice */
> + ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> + if (ret < 0)
> + return ret;
> + vin->digital->source_pad = ret;
> +
> + ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> + vin->digital->sink_pad = ret < 0 ? 0 : ret;
> +
> + /* Find compatible subdevices mbus format */
> + vin->digital->code = 0;
> code.index = 0;
> - code.pad = entity->source_pad;
> - while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code)) {
> + code.pad = vin->digital->source_pad;
> + while (!vin->digital->code &&
> +!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
> code.index++;
> switch (code.code) {
> case MEDIA_BUS_FMT_YUYV8_1X16:
> case MEDIA_BUS_FMT_UYVY8_2X8:
> case MEDIA_BUS_FMT_UYVY10_2X10:
> case MEDIA_BUS_FMT_RGB888_1X24:
> - entity->code = code.code;
> - return true;
> + vin->digital->code = code.code;
> + vin_dbg(vin, "Found media bus format for %s: %d\n",
> + subdev->name, vin->digital->code);
> + break;
> default:
> break;
> }
> }
>
> - return false;
> -}
> -
> -static int rvin_digital_notify_complete(struct v4l2_async_notifier
> *notifier) -{
> - struct rvin_dev *vin = notifier_to_vin(notifier);
> - int ret;
> -
> - /* Verify subdevices mbus format */
> - if (!rvin_mbus_supported(vin->digital)) {
> + if (!vin->digital->code) {
> vin_err(vin, "Unsupported media bus format for %s\n",
> - vin->digital->subdev->name);
> + subdev->name);
> return -EINVAL;
> }
>
> - vin_dbg(vin, "Found media bus format for %s: %d\n",
> - vin->digital->subdev->name, vin->digital->code);
> + /* Read tvnorms */
> + ret = v4l2_subdev_call(subdev, video, g_tvnorms, &vin->vdev.tvnorms);
> + if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> + return ret;
> +
> + /* Add the controls */
> + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> + if (ret < 0)
> + return ret;
> +
> + ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
> + NULL);
> + if (ret < 0) {
> + v4l2_ctrl_handler_free(&vin->ctrl_handler);
> + return ret;
> + }
> +
> + vin->vdev.ctrl_handler = &vin->ctrl_handler;
> +
> + vin->digital->subdev = subdev;
> +
> + return 0;
> +}
> +
> +static void rvin_digital_subdevice_detach(struct rvin_dev *vin)
> +{
> + rvin_v4l2_unregister(vin);
> + v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +
> + vin->vdev.ctrl_handler = NULL;
> + vin->digital->subdev = NULL;
> +}
> +
> +static int rvin_digital_notify_complete(struct v4l2_async_notifier
> *notifier) +{
> + struct rvin_dev *vin = notifier_to_vin(notifier);
> + int ret;
>
> ret = v4l2_device_register_subdev_nodes(&
Re: [PATCH v10 01/30] rcar-vin: add Gen3 devicetree bindings documentation
Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:06 EET Niklas Söderlund wrote:
> Document the devicetree bindings for the CSI-2 inputs available on Gen3.
>
> There is a need to add a custom property 'renesas,id' and to define
> which CSI-2 input is described in which endpoint under the port@1 node.
> This information is needed since there are a set of predefined routes
> between each VIN and CSI-2 block. This routing table will be kept
> inside the driver but in order for it to act on it it must know which
> VIN and CSI-2 is which.
>
> Signed-off-by: Niklas Söderlund
> Acked-by: Rob Herring
Reviewed-by: Laurent Pinchart
> ---
> .../devicetree/bindings/media/rcar_vin.txt | 118 +++---
> 1 file changed, 106 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt
> b/Documentation/devicetree/bindings/media/rcar_vin.txt index
> c60e6b0a89b67a8c..90d92836284b7f68 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -2,8 +2,12 @@ Renesas R-Car Video Input driver (rcar_vin)
> ---
>
> The rcar_vin device provides video input capabilities for the Renesas R-Car
> -family of devices. The current blocks are always slaves and suppot one
> input -channel which can be either RGB, YUYV or BT656.
> +family of devices.
> +
> +Each VIN instance has a single parallel input that supports RGB and YUV
> video, +with both external synchronization and BT.656 synchronization for
> the latter. +Depending on the instance the VIN input is connected to
> external SoC pins, or +on Gen3 platforms to a CSI-2 receiver.
>
> - compatible: Must be one or more of the following
> - "renesas,vin-r8a7743" for the R8A7743 device
> @@ -16,6 +20,8 @@ channel which can be either RGB, YUYV or BT656.
> - "renesas,vin-r8a7793" for the R8A7793 device
> - "renesas,vin-r8a7794" for the R8A7794 device
> - "renesas,vin-r8a7795" for the R8A7795 device
> + - "renesas,vin-r8a7796" for the R8A7796 device
> + - "renesas,vin-r8a77970" for the R8A77970 device
> - "renesas,rcar-gen2-vin" for a generic R-Car Gen2 or RZ/G1 compatible
> device.
> - "renesas,rcar-gen3-vin" for a generic R-Car Gen3 compatible device.
> @@ -31,21 +37,38 @@ channel which can be either RGB, YUYV or BT656.
> Additionally, an alias named vinX will need to be created to specify
> which video input device this is.
>
> -The per-board settings:
> +The per-board settings Gen2 platforms:
> - port sub-node describing a single endpoint connected to the vin
> as described in video-interfaces.txt[1]. Only the first one will
> be considered as each vin interface has one input port.
>
> - These settings are used to work out video input format and widths
> - into the system.
> +The per-board settings Gen3 platforms:
>
> +Gen3 platforms can support both a single connected parallel input source
> +from external SoC pins (port0) and/or multiple parallel input sources
> +from local SoC CSI-2 receivers (port1) depending on SoC.
>
> -Device node example
>
> +- renesas,id - ID number of the VIN, VINx in the documentation.
> +- ports
> +- port 0 - sub-node describing a single endpoint connected to the VIN
> + from external SoC pins described in video-interfaces.txt[1].
> + Describing more then one endpoint in port 0 is invalid. Only VIN
> + instances that are connected to external pins should have port 0.
> +- port 1 - sub-nodes describing one or more endpoints connected to
> + the VIN from local SoC CSI-2 receivers. The endpoint numbers must
> + use the following schema.
>
> - aliases {
> -vin0 = &vin0;
> - };
> +- Endpoint 0 - sub-node describing the endpoint connected to CSI20
> +- Endpoint 1 - sub-node describing the endpoint connected to CSI21
> +- Endpoint 2 - sub-node describing the endpoint connected to CSI40
> +- Endpoint 3 - sub-node describing the endpoint connected to CSI41
> +
> +Device node example for Gen2 platforms
> +--
> +
> +aliases {
> +vin0 = &vin0;
> +};
>
> vin0: vin@e6ef {
> compatible = "renesas,vin-r8a7790",
> "renesas,rcar-gen2-vin"; @@ -55,8 +78,8 @@ Device node example
> status = "disabled";
> };
>
> -Board setup example (vin1 composite video input)
> -
> +Board setup example for Gen2 platforms (vin1 composite video input)
> +---
>
> &i2c2 {
> status = "ok";
> @@ -95,6 +118,77 @@ Board setup example (vin1 composite video input)
> };
> };
>
> +Device node example for Gen3 platforms
> +--
>
> +vin0: video@e6
[PATCH v2] gspca: dtcs033: use %*ph to print small buffer
Use %*ph format to print small buffer as hex string.
Suggested-by: Andy Shevchenko
Signed-off-by: Antonio Cardace
---
- don't do independent changes in one patch
---
drivers/media/usb/gspca/dtcs033.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/usb/gspca/dtcs033.c
b/drivers/media/usb/gspca/dtcs033.c
index cdf27cf0112a..041d3c0e907d 100644
--- a/drivers/media/usb/gspca/dtcs033.c
+++ b/drivers/media/usb/gspca/dtcs033.c
@@ -76,12 +76,10 @@ static int reg_reqs(struct gspca_dev *gspca_dev,
} else if (preq->bRequestType & USB_DIR_IN) {
gspca_dbg(gspca_dev, D_STREAM,
- "USB IN (%d) returned[%d] %02X %02X %02X
%s\n",
+ "USB IN (%d) returned[%d] %3ph %s\n",
i,
preq->wLength,
- gspca_dev->usb_buf[0],
- gspca_dev->usb_buf[1],
- gspca_dev->usb_buf[2],
+ gspca_dev->usb_buf,
preq->wLength > 3 ? "...\n" : "\n");
}
--
2.15.1.354.g95ec6b1b3
Dear friend,
Dear friend, Greetings to you and your family. I am Mrs. Aletta Dobson a widow is and I am 55 years old, suffering from pancreatic cancer, I do not think so much more. The doctors and if it is the will of God that I will die, then so Shall it be. Nevertheless, I am willing to donate the sum of $ 1,600,000 million. I am hoping for the poor people and I hope you have the kind of heart and the ability to help others. I wait to see your response. My regards, Mrs. Aletta Dobson
Re: [PATCH v3 5/5] drm: adv7511: Add support for i2c_new_secondary_device
Hi Laurent,
Thanks for the review,
On 13/02/18 12:23, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Tuesday, 13 February 2018 00:07:53 EET Kieran Bingham wrote:
>> From: Kieran Bingham
>>
>> The ADV7511 has four 256-byte maps that can be accessed via the main I²C
>> ports. Each map has it own I²C address and acts as a standard slave
>> device on the I²C bus.
>>
>> Allow a device tree node to override the default addresses so that
>> address conflicts with other devices on the same bus may be resolved at
>> the board description level.
>>
>> Signed-off-by: Kieran Bingham
>> ---
>> v2:
>> - Update missing edid-i2c address setting
>> - Split out DT bindings
>> - Rename and move the I2C default addresses to their own section
>>
>> drivers/gpu/drm/bridge/adv7511/adv7511.h | 6
>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 42
>> 2 files changed, 33 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index d034b2cb5eee..04e6759ee45b
>> 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -93,6 +93,11 @@
>> #define ADV7511_REG_CHIP_ID_HIGH0xf5
>> #define ADV7511_REG_CHIP_ID_LOW 0xf6
>>
>> +/* Hardware defined default addresses for i2c register maps */
>
> s/i2c/I2C/ ? That's really because I had to find something :-)
The I²C comes from JMH's original patch, but is much harder to grep for, so
normalising to I2C throughout.
>
> Reviewed-by: Laurent Pinchart
>> +#define ADV7511_CEC_I2C_ADDR_DEFAULT0x3c
>> +#define ADV7511_EDID_I2C_ADDR_DEFAULT 0x3f
>> +#define ADV7511_PACKET_I2C_ADDR_DEFAULT 0x38
>> +
>> #define ADV7511_CSC_ENABLE BIT(7)
>> #define ADV7511_CSC_UPDATE_MODE BIT(5)
>>
>> @@ -322,6 +327,7 @@ struct adv7511 {
>> struct i2c_client *i2c_main;
>> struct i2c_client *i2c_edid;
>> struct i2c_client *i2c_cec;
>> +struct i2c_client *i2c_packet;
>>
>> struct regmap *regmap;
>> struct regmap *regmap_cec;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index
>> efa29db5fc2b..5e61b928c9c0 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -586,7 +586,7 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>> /* Reading the EDID only works if the device is powered */
>> if (!adv7511->powered) {
>> unsigned int edid_i2c_addr =
>> -(adv7511->i2c_main->addr << 1) + 4;
>> +(adv7511->i2c_edid->addr << 1);
>>
>> __adv7511_power_on(adv7511);
>>
>> @@ -969,10 +969,10 @@ static int adv7511_init_cec_regmap(struct adv7511
>> *adv) {
>> int ret;
>>
>> -adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
>> - adv->i2c_main->addr - 1);
>> +adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec",
>> +ADV7511_CEC_I2C_ADDR_DEFAULT);
>> if (!adv->i2c_cec)
>> -return -ENOMEM;
>> +return -EINVAL;
>> i2c_set_clientdata(adv->i2c_cec, adv);
>>
>> adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
>> @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const
>> struct i2c_device_id *id) struct adv7511_link_config link_config;
>> struct adv7511 *adv7511;
>> struct device *dev = &i2c->dev;
>> -unsigned int main_i2c_addr = i2c->addr << 1;
>> -unsigned int edid_i2c_addr = main_i2c_addr + 4;
>> unsigned int val;
>> int ret;
>>
>> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c,
>> const struct i2c_device_id *id) if (ret)
>> goto uninit_regulators;
>>
>> -regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
>> -regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
>> - main_i2c_addr - 0xa);
>> -regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
>> - main_i2c_addr - 2);
>> -
>> adv7511_packet_disable(adv7511, 0x);
>>
>> -adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
>> +adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
>> +ADV7511_EDID_I2C_ADDR_DEFAULT);
>> if (!adv7511->i2c_edid) {
>> -ret = -ENOMEM;
>> +ret = -EINVAL;
>> goto uninit_regulators;
>> }
>>
>> +regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
>> + adv7511->i2c_edid->addr << 1);
>> +
>> ret = adv7511_init_cec_regmap(adv7511);
>> if (ret)
>> goto err_i2c_unregister_edid;
>>
>> +regmap_write(adv7511->regmap, ADV7511_REG_
Re: [PATCH 2/5] auxdisplay: charlcd: add flush function
On Mon, Feb 12, 2018 at 10:44 PM, Miguel Ojeda wrote: > On Mon, Jan 15, 2018 at 10:58 AM, Sean Young wrote: >> The Sasem Remote Controller has an LCD, which is connnected via usb. >> Multiple write reg or write data commands can be combined into one usb >> packet. >> >> The latency of usb is such that if we send commands one by one, we get >> very obvious tearing on the LCD. >> >> By adding a flush function, we can buffer all commands until either >> the usb packet is full or the lcd changes are complete. > Cc'ing Arnd and Greg since this touches include/misc as well. >> --- a/include/misc/charlcd.h >> +++ b/include/misc/charlcd.h As far as I can see better to create a subfolder under include for auxdisplay stuff. Currently we have three candidates here: linux/cfag12864b.h linux/ks0108.h misc/charlcd.h Another possibility to get rid of them under include/ by (re)moving to drivers/auxdisplay/. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/5] dt-bindings: media: adv7604: Add support for i2c_new_secondary_device
Hi Kieran,
On Tuesday, 13 February 2018 15:14:43 EET Kieran Bingham wrote:
> On 13/02/18 12:06, Laurent Pinchart wrote:
> > On Tuesday, 13 February 2018 00:07:49 EET Kieran Bingham wrote:
> >> From: Jean-Michel Hautbois
> >>
> >> The ADV7604 has thirteen 256-byte maps that can be accessed via the main
> >> I²C ports. Each map has it own I²C address and acts as a standard slave
> >> device on the I²C bus.
> >>
> >> Extend the device tree node bindings to be able to override the default
> >> addresses so that address conflicts with other devices on the same bus
> >> may be resolved at the board description level.
> >>
> >> Signed-off-by: Jean-Michel Hautbois
> >> [Kieran: Re-adapted for mainline]
> >> Signed-off-by: Kieran Bingham
> >> Reviewed-by: Rob Herring
> >
> > Nitpicking, I might not mention i2c_new_secondary_device in the subject,
> > as this is a DT bindings change. I don't mind too much though, as long as
> > the bindings themselves don't contain Linux-specific information, and they
> > don't, so
>
> How about: ... adv7604: Extend bindings to allow specifying slave map
> addresses
Sounds good to me.
> > Reviewed-by: Laurent Pinchart
>
> Collected, thanks.
>
> --
> Kieran
>
> >> ---
> >>
> >> Based upon the original posting :
> >> https://lkml.org/lkml/2014/10/22/469
> >>
> >> v2:
> >> - DT Binding update separated from code change
> >> - Minor reword to commit message to account for DT only change.
> >> - Collected Rob's RB tag.
> >>
> >> v3:
> >> - Split map register addresses into individual declarations.
> >>
> >> .../devicetree/bindings/media/i2c/adv7604.txt | 18
> >>
> >> -- 1 file changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> >> b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
> >> 9cbd92eb5d05..ebb5f070c05b 100644
> >> --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> >> +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> >>
> >> @@ -13,7 +13,11 @@ Required Properties:
> >> - "adi,adv7611" for the ADV7611
> >> - "adi,adv7612" for the ADV7612
> >>
> >> - - reg: I2C slave address
> >> + - reg: I2C slave addresses
> >> +The ADV76xx has up to thirteen 256-byte maps that can be accessed
> >> via
> >> the +main I²C ports. Each map has it own I²C address and acts as a
> >> standard +slave device on the I²C bus. The main address is mandatory,
> >> others are +optional and revert to defaults if not specified.
> >>
> >>- hpd-gpios: References to the GPIOs that control the HDMI hot-plug
> >>
> >> detection pins, one per HDMI input. The active flag indicates the
> >> GPIO
> >>
> >> @@ -35,6 +39,11 @@ Optional Properties:
> >>- reset-gpios: Reference to the GPIO connected to the device's reset
> >>pin.
> >>
> >> - default-input: Select which input is selected after reset.
> >> + - reg-names : Names of maps with programmable addresses.
> >> + It can contain any map needing a non-default address.
> >> + Possible maps names are :
> >> +"main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe",
> >> +"rep", "edid", "hdmi", "test", "cp", "vdp"
> >>
> >> Optional Endpoint Properties:
> >> @@ -52,7 +61,12 @@ Example:
> >>hdmi_receiver@4c {
> >>
> >>compatible = "adi,adv7611";
> >>
> >> - reg = <0x4c>;
> >> + /*
> >> + * The edid page will be accessible @ 0x66 on the i2c bus. All
> >> + * other maps will retain their default addresses.
> >> + */
> >> + reg = <0x4c>, <0x66>;
> >> + reg-names "main", "edid";
> >>
> >>reset-gpios = <&ioexp 0 GPIO_ACTIVE_LOW>;
> >>hpd-gpios = <&ioexp 2 GPIO_ACTIVE_HIGH>;
--
Regards,
Laurent Pinchart
Re: [PATCH v3 1/5] dt-bindings: media: adv7604: Add support for i2c_new_secondary_device
Hi Laurent,
On 13/02/18 12:06, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
Thank you for your review,
> On Tuesday, 13 February 2018 00:07:49 EET Kieran Bingham wrote:
>> From: Jean-Michel Hautbois
>>
>> The ADV7604 has thirteen 256-byte maps that can be accessed via the main
>> I²C ports. Each map has it own I²C address and acts as a standard slave
>> device on the I²C bus.
>>
>> Extend the device tree node bindings to be able to override the default
>> addresses so that address conflicts with other devices on the same bus
>> may be resolved at the board description level.
>>
>> Signed-off-by: Jean-Michel Hautbois
>> [Kieran: Re-adapted for mainline]
>> Signed-off-by: Kieran Bingham
>> Reviewed-by: Rob Herring
>
> Nitpicking, I might not mention i2c_new_secondary_device in the subject, as
> this is a DT bindings change. I don't mind too much though, as long as the
> bindings themselves don't contain Linux-specific information, and they don't,
> so
How about: ... adv7604: Extend bindings to allow specifying slave map addresses
> Reviewed-by: Laurent Pinchart
Collected, thanks.
--
Kieran
>
>> ---
>> Based upon the original posting :
>> https://lkml.org/lkml/2014/10/22/469
>>
>> v2:
>> - DT Binding update separated from code change
>> - Minor reword to commit message to account for DT only change.
>> - Collected Rob's RB tag.
>>
>> v3:
>> - Split map register addresses into individual declarations.
>>
>> .../devicetree/bindings/media/i2c/adv7604.txt | 18
>> -- 1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
>> b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
>> 9cbd92eb5d05..ebb5f070c05b 100644
>> --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
>> +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
>> @@ -13,7 +13,11 @@ Required Properties:
>> - "adi,adv7611" for the ADV7611
>> - "adi,adv7612" for the ADV7612
>>
>> - - reg: I2C slave address
>> + - reg: I2C slave addresses
>> +The ADV76xx has up to thirteen 256-byte maps that can be accessed via
>> the +main I²C ports. Each map has it own I²C address and acts as a
>> standard +slave device on the I²C bus. The main address is mandatory,
>> others are +optional and revert to defaults if not specified.
>>
>>- hpd-gpios: References to the GPIOs that control the HDMI hot-plug
>> detection pins, one per HDMI input. The active flag indicates the GPIO
>> @@ -35,6 +39,11 @@ Optional Properties:
>>
>>- reset-gpios: Reference to the GPIO connected to the device's reset pin.
>> - default-input: Select which input is selected after reset.
>> + - reg-names : Names of maps with programmable addresses.
>> +It can contain any map needing a non-default address.
>> +Possible maps names are :
>> + "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe",
>> + "rep", "edid", "hdmi", "test", "cp", "vdp"
>>
>> Optional Endpoint Properties:
>>
>> @@ -52,7 +61,12 @@ Example:
>>
>> hdmi_receiver@4c {
>> compatible = "adi,adv7611";
>> -reg = <0x4c>;
>> +/*
>> + * The edid page will be accessible @ 0x66 on the i2c bus. All
>> + * other maps will retain their default addresses.
>> + */
>> +reg = <0x4c>, <0x66>;
>> +reg-names "main", "edid";
>>
>> reset-gpios = <&ioexp 0 GPIO_ACTIVE_LOW>;
>> hpd-gpios = <&ioexp 2 GPIO_ACTIVE_HIGH>;
>
>
Re: [PATCH v2 5/5] drm: adv7511: Add support for i2c_new_secondary_device
Hi Dan
Thank you for the review,
On 13/02/18 07:23, Dan Carpenter wrote:
> On Mon, Feb 12, 2018 at 06:11:57PM +, Kieran Bingham wrote:
>> +adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
>> +ADV7511_PACKET_I2C_ADDR_DEFAULT);
>> +if (!adv7511->i2c_packet) {
>> +ret = -EINVAL;
>> +goto err_unregister_cec;
>> +}
>> +
>> +regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
>> + adv7511->i2c_packet->addr << 1);
>> +
>> INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
>>
>> if (i2c->irq) {
>> @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const
>> struct i2c_device_id *id)
>> IRQF_ONESHOT, dev_name(dev),
>> adv7511);
>> if (ret)
>> -goto err_unregister_cec;
>> +goto err_unregister_packet;
>> }
>>
>> adv7511_power_off(adv7511);
>
> There is another goto which needs to be updated if adv7511_cec_init()
> fails.
Aha - thanks - I'll look into this now.
>
>> @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const
>> struct i2c_device_id *id)
>> adv7511_audio_init(dev, adv7511);
>> return 0;
>>
>> +err_unregister_packet:
>> +i2c_unregister_device(adv7511->i2c_packet);
>> err_unregister_cec:
>> i2c_unregister_device(adv7511->i2c_cec);
>> if (adv7511->cec_clk)
>
>
> regards,
> dan carpenter
>
[RESEND PATCH v5 1/6] media: rc: update sunxi-ir driver to get base clock frequency from devicetree
This patch updates the sunxi-ir driver to set the base clock frequency from
devicetree.
This is necessary since there are different ir receivers on the
market, that operate with different frequencies. So this value could be
set if the attached ir receiver needs a different base clock frequency,
than the default 8 MHz.
Signed-off-by: Philipp Rossak
Reviewed-by: Andi Shyti
Acked-by: Sean Young
---
drivers/media/rc/sunxi-cir.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
index 97f367b446c4..f500cea228a9 100644
--- a/drivers/media/rc/sunxi-cir.c
+++ b/drivers/media/rc/sunxi-cir.c
@@ -72,12 +72,8 @@
/* CIR_REG register idle threshold */
#define REG_CIR_ITHR(val)(((val) << 8) & (GENMASK(15, 8)))
-/* Required frequency for IR0 or IR1 clock in CIR mode */
+/* Required frequency for IR0 or IR1 clock in CIR mode (default) */
#define SUNXI_IR_BASE_CLK 800
-/* Frequency after IR internal divider */
-#define SUNXI_IR_CLK (SUNXI_IR_BASE_CLK / 64)
-/* Sample period in ns */
-#define SUNXI_IR_SAMPLE (10ul / SUNXI_IR_CLK)
/* Noise threshold in samples */
#define SUNXI_IR_RXNOISE 1
/* Idle Threshold in samples */
@@ -122,7 +118,8 @@ static irqreturn_t sunxi_ir_irq(int irqno, void *dev_id)
/* for each bit in fifo */
dt = readb(ir->base + SUNXI_IR_RXFIFO_REG);
rawir.pulse = (dt & 0x80) != 0;
- rawir.duration = ((dt & 0x7f) + 1) * SUNXI_IR_SAMPLE;
+ rawir.duration = ((dt & 0x7f) + 1) *
+ir->rc->rx_resolution;
ir_raw_event_store_with_filter(ir->rc, &rawir);
}
}
@@ -148,6 +145,7 @@ static int sunxi_ir_probe(struct platform_device *pdev)
struct device_node *dn = dev->of_node;
struct resource *res;
struct sunxi_ir *ir;
+ u32 b_clk_freq = SUNXI_IR_BASE_CLK;
ir = devm_kzalloc(dev, sizeof(struct sunxi_ir), GFP_KERNEL);
if (!ir)
@@ -172,6 +170,9 @@ static int sunxi_ir_probe(struct platform_device *pdev)
return PTR_ERR(ir->clk);
}
+ /* Base clock frequency (optional) */
+ of_property_read_u32(dn, "clock-frequency", &b_clk_freq);
+
/* Reset (optional) */
ir->rst = devm_reset_control_get_optional_exclusive(dev, NULL);
if (IS_ERR(ir->rst))
@@ -180,11 +181,12 @@ static int sunxi_ir_probe(struct platform_device *pdev)
if (ret)
return ret;
- ret = clk_set_rate(ir->clk, SUNXI_IR_BASE_CLK);
+ ret = clk_set_rate(ir->clk, b_clk_freq);
if (ret) {
dev_err(dev, "set ir base clock failed!\n");
goto exit_reset_assert;
}
+ dev_dbg(dev, "set base clock frequency to %d Hz.\n", b_clk_freq);
if (clk_prepare_enable(ir->apb_clk)) {
dev_err(dev, "try to enable apb_ir_clk failed\n");
@@ -225,7 +227,8 @@ static int sunxi_ir_probe(struct platform_device *pdev)
ir->rc->map_name = ir->map_name ?: RC_MAP_EMPTY;
ir->rc->dev.parent = dev;
ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
- ir->rc->rx_resolution = SUNXI_IR_SAMPLE;
+ /* Frequency after IR internal divider with sample period in ns */
+ ir->rc->rx_resolution = (10ul / (b_clk_freq / 64));
ir->rc->timeout = MS_TO_NS(SUNXI_IR_TIMEOUT);
ir->rc->driver_name = SUNXI_IR_DEV;
--
2.11.0
[RESEND PATCH v5 0/6] IR support for A83T
This patch series adds support for the sunxi A83T ir module and enhances the sunxi-ir driver. Right now the base clock frequency for the ir driver is a hard coded define and is set to 8 MHz. This works for the most common ir receivers. On the Sinovoip Bananapi M3 the ir receiver needs, a 3 MHz base clock frequency to work without problems with this driver. This patch series adds support for an optinal property that makes it able to override the default base clock frequency and enables the ir interface on the a83t and the Bananapi M3. changes since v4: * rename cir pin from cir_pins to r_cir_pin * drop unit-adress from r_cir_pin * add a83t compatible to the cir node * move muxing options to dtsi * rename cir label and reorder it in the bananpim3.dts file changes since v3: * collecting all acks & reviewd by * fixed typos changes since v2: * reorder cir pin (alphabetical) * fix typo in documentation changes since v1: * fix typos, reword Documentation * initialize 'b_clk_freq' to 'SUNXI_IR_BASE_CLK' & remove if statement * change dev_info() to dev_dbg() * change naming to cir* in dts/dtsi * Added acked Ackedi-by to related patch * use whole memory block instead of registers needed + fix for h3/h5 changes since rfc: * The property is now optinal. If the property is not available in the dtb the driver uses the default base clock frequency. * the driver prints out the the selected base clock frequency. * changed devicetree property from base-clk-frequency to clock-frequency Regards, Philipp Philipp Rossak (6): media: rc: update sunxi-ir driver to get base clock frequency from devicetree media: dt: bindings: Update binding documentation for sunxi IR controller arm: dts: sun8i: a83t: Add the cir pin for the A83T arm: dts: sun8i: a83t: Add support for the cir interface arm: dts: sun8i: a83t: bananapi-m3: Enable IR controller arm: dts: sun8i: h3-h5: ir register size should be the whole memory block Documentation/devicetree/bindings/media/sunxi-ir.txt | 3 +++ arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 5 + arch/arm/boot/dts/sun8i-a83t.dtsi| 18 ++ arch/arm/boot/dts/sunxi-h3-h5.dtsi | 2 +- drivers/media/rc/sunxi-cir.c | 19 +++ 5 files changed, 38 insertions(+), 9 deletions(-) -- 2.11.0
[RESEND PATCH v5 6/6] arm: dts: sun8i: h3-h5: ir register size should be the whole memory block
The size of the register should be the size of the whole memory block, not just the registers, that are needed. Signed-off-by: Philipp Rossak --- arch/arm/boot/dts/sunxi-h3-h5.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi index 7a83b15225c7..22f6e126b8df 100644 --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi @@ -712,7 +712,7 @@ clock-names = "apb", "ir"; resets = <&r_ccu RST_APB0_IR>; interrupts = ; - reg = <0x01f02000 0x40>; + reg = <0x01f02000 0x400>; status = "disabled"; }; -- 2.11.0
[RESEND PATCH v5 4/6] arm: dts: sun8i: a83t: Add support for the cir interface
The cir interface is like on the H3 located at 0x01f02000 and is exactly
the same. This patch adds support for the ir interface on the A83T.
Signed-off-by: Philipp Rossak
---
arch/arm/boot/dts/sun8i-a83t.dtsi | 13 +
1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi
b/arch/arm/boot/dts/sun8i-a83t.dtsi
index f7f78a27e21d..1e04a5cfd32d 100644
--- a/arch/arm/boot/dts/sun8i-a83t.dtsi
+++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
@@ -704,6 +704,19 @@
#reset-cells = <1>;
};
+ r_cir: ir@1f02000 {
+ compatible = "allwinner,sun8i-a83t-ir",
+"allwinner,sun5i-a13-ir";
+ clocks = <&r_ccu CLK_APB0_IR>, <&r_ccu CLK_IR>;
+ clock-names = "apb", "ir";
+ resets = <&r_ccu RST_APB0_IR>;
+ interrupts = ;
+ reg = <0x01f02000 0x400>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&r_cir_pin>;
+ status = "disabled";
+ };
+
r_pio: pinctrl@1f02c00 {
compatible = "allwinner,sun8i-a83t-r-pinctrl";
reg = <0x01f02c00 0x400>;
--
2.11.0
[RESEND PATCH v5 3/6] arm: dts: sun8i: a83t: Add the cir pin for the A83T
The CIR Pin of the A83T is located at PL12.
Signed-off-by: Philipp Rossak
---
arch/arm/boot/dts/sun8i-a83t.dtsi | 5 +
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi
b/arch/arm/boot/dts/sun8i-a83t.dtsi
index 7f4955a5fab7..f7f78a27e21d 100644
--- a/arch/arm/boot/dts/sun8i-a83t.dtsi
+++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
@@ -716,6 +716,11 @@
interrupt-controller;
#interrupt-cells = <3>;
+ r_cir_pin: r-cir-pin {
+ pins = "PL12";
+ function = "s_cir_rx";
+ };
+
r_rsb_pins: r-rsb-pins {
pins = "PL0", "PL1";
function = "s_rsb";
--
2.11.0
[RESEND PATCH v5 2/6] media: dt: bindings: Update binding documentation for sunxi IR controller
This patch updates documentation for Device-Tree bindings for sunxi IR
controller and adds the new optional property for the base clock
frequency.
Signed-off-by: Philipp Rossak
Acked-by: Maxime Ripard
Reviewed-by: Rob Herring
---
Documentation/devicetree/bindings/media/sunxi-ir.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/sunxi-ir.txt
b/Documentation/devicetree/bindings/media/sunxi-ir.txt
index 91648c569b1e..278098987edb 100644
--- a/Documentation/devicetree/bindings/media/sunxi-ir.txt
+++ b/Documentation/devicetree/bindings/media/sunxi-ir.txt
@@ -11,6 +11,8 @@ Required properties:
Optional properties:
- linux,rc-map-name: see rc.txt file in the same directory.
- resets : phandle + reset specifier pair
+- clock-frequency : IR Receiver clock frequency, in Hertz. Defaults to 8 MHz
+if missing.
Example:
@@ -18,6 +20,7 @@ ir0: ir@1c21800 {
compatible = "allwinner,sun4i-a10-ir";
clocks = <&apb0_gates 6>, <&ir0_clk>;
clock-names = "apb", "ir";
+ clock-frequency = <300>;
resets = <&apb0_rst 1>;
interrupts = <0 5 1>;
reg = <0x01C21800 0x40>;
--
2.11.0
[RESEND PATCH v5 5/6] arm: dts: sun8i: a83t: bananapi-m3: Enable IR controller
The Bananapi M3 has an onboard IR receiver.
This enables the onboard IR receiver subnode.
Unlike the other IR receivers this one needs a base clock frequency
of 300 Hz (3 MHz), to be able to work.
Signed-off-by: Philipp Rossak
Acked-by: Chen-Yu Tsai
---
arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 5 +
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
index 6550bf0e594b..26c015fd4f4d 100644
--- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
@@ -145,6 +145,11 @@
status = "okay";
};
+&r_cir {
+ clock-frequency = <300>;
+ status = "okay";
+};
+
&r_rsb {
status = "okay";
--
2.11.0
Re: [PATCH v3 5/5] drm: adv7511: Add support for i2c_new_secondary_device
Hi Kieran,
Thank you for the patch.
On Tuesday, 13 February 2018 00:07:53 EET Kieran Bingham wrote:
> From: Kieran Bingham
>
> The ADV7511 has four 256-byte maps that can be accessed via the main I²C
> ports. Each map has it own I²C address and acts as a standard slave
> device on the I²C bus.
>
> Allow a device tree node to override the default addresses so that
> address conflicts with other devices on the same bus may be resolved at
> the board description level.
>
> Signed-off-by: Kieran Bingham
> ---
> v2:
> - Update missing edid-i2c address setting
> - Split out DT bindings
> - Rename and move the I2C default addresses to their own section
>
> drivers/gpu/drm/bridge/adv7511/adv7511.h | 6
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 42
> 2 files changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index d034b2cb5eee..04e6759ee45b
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -93,6 +93,11 @@
> #define ADV7511_REG_CHIP_ID_HIGH 0xf5
> #define ADV7511_REG_CHIP_ID_LOW 0xf6
>
> +/* Hardware defined default addresses for i2c register maps */
s/i2c/I2C/ ? That's really because I had to find something :-)
Reviewed-by: Laurent Pinchart
> +#define ADV7511_CEC_I2C_ADDR_DEFAULT 0x3c
> +#define ADV7511_EDID_I2C_ADDR_DEFAULT0x3f
> +#define ADV7511_PACKET_I2C_ADDR_DEFAULT 0x38
> +
> #define ADV7511_CSC_ENABLE BIT(7)
> #define ADV7511_CSC_UPDATE_MODE BIT(5)
>
> @@ -322,6 +327,7 @@ struct adv7511 {
> struct i2c_client *i2c_main;
> struct i2c_client *i2c_edid;
> struct i2c_client *i2c_cec;
> + struct i2c_client *i2c_packet;
>
> struct regmap *regmap;
> struct regmap *regmap_cec;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index
> efa29db5fc2b..5e61b928c9c0 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -586,7 +586,7 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
> /* Reading the EDID only works if the device is powered */
> if (!adv7511->powered) {
> unsigned int edid_i2c_addr =
> - (adv7511->i2c_main->addr << 1) + 4;
> + (adv7511->i2c_edid->addr << 1);
>
> __adv7511_power_on(adv7511);
>
> @@ -969,10 +969,10 @@ static int adv7511_init_cec_regmap(struct adv7511
> *adv) {
> int ret;
>
> - adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
> - adv->i2c_main->addr - 1);
> + adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec",
> + ADV7511_CEC_I2C_ADDR_DEFAULT);
> if (!adv->i2c_cec)
> - return -ENOMEM;
> + return -EINVAL;
> i2c_set_clientdata(adv->i2c_cec, adv);
>
> adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
> @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const
> struct i2c_device_id *id) struct adv7511_link_config link_config;
> struct adv7511 *adv7511;
> struct device *dev = &i2c->dev;
> - unsigned int main_i2c_addr = i2c->addr << 1;
> - unsigned int edid_i2c_addr = main_i2c_addr + 4;
> unsigned int val;
> int ret;
>
> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id) if (ret)
> goto uninit_regulators;
>
> - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
> - regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> - main_i2c_addr - 0xa);
> - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> - main_i2c_addr - 2);
> -
> adv7511_packet_disable(adv7511, 0x);
>
> - adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
> + adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
> + ADV7511_EDID_I2C_ADDR_DEFAULT);
> if (!adv7511->i2c_edid) {
> - ret = -ENOMEM;
> + ret = -EINVAL;
> goto uninit_regulators;
> }
>
> + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> + adv7511->i2c_edid->addr << 1);
> +
> ret = adv7511_init_cec_regmap(adv7511);
> if (ret)
> goto err_i2c_unregister_edid;
>
> + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> + adv7511->i2c_cec->addr << 1);
> +
> + adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
> + ADV7511_PACKET_I2C_ADDR_DEFAULT);
> + if (!adv7511->i2c_packet) {
> +
Re: [PATCH v3 4/5] media: adv7604: Add support for i2c_new_secondary_device
Hi Kieran,
Thank you for the patch.
On Tuesday, 13 February 2018 00:07:52 EET Kieran Bingham wrote:
> From: Jean-Michel Hautbois
>
> The ADV7604 has thirteen 256-byte maps that can be accessed via the main
> I²C ports. Each map has it own I²C address and acts as a standard slave
> device on the I²C bus.
>
> Allow a device tree node to override the default addresses so that
> address conflicts with other devices on the same bus may be resolved at
> the board description level.
>
> Signed-off-by: Jean-Michel Hautbois
> [Kieran: Re-adapted for mainline]
> Signed-off-by: Kieran Bingham
>
> ---
> Based upon the original posting :
> https://lkml.org/lkml/2014/10/22/469
>
> v2:
> - Split out DT bindings from driver updates
> - Return -EINVAL on error paths from adv76xx_dummy_client()
>
> drivers/media/i2c/adv7604.c | 62 ++
> 1 file changed, 40 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 1544920ec52d..872e124793f8 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -2734,6 +2734,27 @@ static const struct v4l2_ctrl_config
> adv76xx_ctrl_free_run_color = {
>
> /* - */
>
> +struct adv76xx_register {
adv76xx_register seems to imply that this describes a particular register,
while the structure describes a registers map. How about adv76xx_register_map,
adv76xx_register_bank or adv76xx_register_page ?
> + const char *name;
> + u8 default_addr;
> +};
> +
> +static const struct adv76xx_register adv76xx_secondary_names[] = {
The table doesn't contain secondary names only as there's an entry for the
main map. How about calling it adv76xx_default_addresses or something along
the same line ?
> + [ADV76XX_PAGE_IO] = { "main", 0x4c },
> + [ADV7604_PAGE_AVLINK] = { "avlink", 0x42 },
> + [ADV76XX_PAGE_CEC] = { "cec", 0x40 },
> + [ADV76XX_PAGE_INFOFRAME] = { "infoframe", 0x3e },
> + [ADV7604_PAGE_ESDP] = { "esdp", 0x38 },
> + [ADV7604_PAGE_DPP] = { "dpp", 0x3c },
> + [ADV76XX_PAGE_AFE] = { "afe", 0x26 },
> + [ADV76XX_PAGE_REP] = { "rep", 0x32 },
> + [ADV76XX_PAGE_EDID] = { "edid", 0x36 },
> + [ADV76XX_PAGE_HDMI] = { "hdmi", 0x34 },
> + [ADV76XX_PAGE_TEST] = { "test", 0x30 },
> + [ADV76XX_PAGE_CP] = { "cp", 0x22 },
> + [ADV7604_PAGE_VDP] = { "vdp", 0x24 },
> +};
> +
> static int adv76xx_core_init(struct v4l2_subdev *sd)
> {
> struct adv76xx_state *state = to_state(sd);
> @@ -2834,13 +2855,26 @@ static void adv76xx_unregister_clients(struct
> adv76xx_state *state) }
>
> static struct i2c_client *adv76xx_dummy_client(struct v4l2_subdev *sd,
> - u8 addr, u8 io_reg)
> +unsigned int i)
Maybe unsigned int page ?
With these fixed,
Reviewed-by: Laurent Pinchart
> {
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct adv76xx_state *state = to_state(sd);
> + struct adv76xx_platform_data *pdata = &state->pdata;
> + unsigned int io_reg = 0xf2 + i;
> + struct i2c_client *new_client;
> +
> + if (pdata && pdata->i2c_addresses[i])
> + new_client = i2c_new_dummy(client->adapter,
> +pdata->i2c_addresses[i]);
> + else
> + new_client = i2c_new_secondary_device(client,
> + adv76xx_secondary_names[i].name,
> + adv76xx_secondary_names[i].default_addr);
>
> - if (addr)
> - io_write(sd, io_reg, addr << 1);
> - return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1);
> + if (new_client)
> + io_write(sd, io_reg, new_client->addr << 1);
> +
> + return new_client;
> }
>
> static const struct adv76xx_reg_seq adv7604_recommended_settings_afe[] = {
> @@ -3115,20 +3149,6 @@ static int adv76xx_parse_dt(struct adv76xx_state
> *state) /* Disable the interrupt for now as no DT-based board uses it. */
> state->pdata.int1_config = ADV76XX_INT1_CONFIG_DISABLED;
>
> - /* Use the default I2C addresses. */
> - state->pdata.i2c_addresses[ADV7604_PAGE_AVLINK] = 0x42;
> - state->pdata.i2c_addresses[ADV76XX_PAGE_CEC] = 0x40;
> - state->pdata.i2c_addresses[ADV76XX_PAGE_INFOFRAME] = 0x3e;
> - state->pdata.i2c_addresses[ADV7604_PAGE_ESDP] = 0x38;
> - state->pdata.i2c_addresses[ADV7604_PAGE_DPP] = 0x3c;
> - state->pdata.i2c_addresses[ADV76XX_PAGE_AFE] = 0x26;
> - state->pdata.i2c_addresses[ADV76XX_PAGE_REP] = 0x32;
> - state->pdata.i2c_addresses[ADV76XX_PAGE_EDID] = 0x36;
> - state->pdata.i2c_addresses[ADV76XX_PAGE_HDMI] = 0x34;
> - state->pdata.i2c_addresses[ADV76XX_PAGE_TEST] = 0x30;
> - state->pdata.i2c_addresses[ADV76XX_PAGE_CP] = 0x22;
> - state->pdata.i2c_addresses[ADV7604_PAGE_VDP] = 0x24;
> -
> /* Har
Re: [PATCH v3 3/5] [RFT] ARM: dts: wheat: Fix ADV7513 address usage
Hi Kieran,
Thank you for the patch.
On Tuesday, 13 February 2018 00:07:51 EET Kieran Bingham wrote:
> From: Kieran Bingham
>
> The r8a7792 Wheat board has two ADV7513 devices sharing a single i2c
> bus, however in low power mode the ADV7513 will reset it's slave maps to
> use the hardware defined default addresses.
>
> The ADV7511 driver was adapted to allow the two devices to be registered
> correctly - but it did not take into account the fault whereby the
> devices reset the addresses.
>
> This results in an address conflict between the device using the default
> addresses, and the other device if it is in low-power-mode.
>
> Repair this issue by moving both devices away from the default address
> definitions.
>
> Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
> ---
> v2:
> - Addition to series
>
> v3:
> - Split map register addresses into individual declarations.
>
> arch/arm/boot/dts/r8a7792-wheat.dts | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/r8a7792-wheat.dts
> b/arch/arm/boot/dts/r8a7792-wheat.dts index b9471b67b728..42fff8837eab
> 100644
> --- a/arch/arm/boot/dts/r8a7792-wheat.dts
> +++ b/arch/arm/boot/dts/r8a7792-wheat.dts
> @@ -240,9 +240,16 @@
> status = "okay";
> clock-frequency = <40>;
>
> + /*
> + * The adv75xx resets its addresses to defaults during low power power
> + * mode. Because we have two ADV7513 devices on the same bus, we must
> + * change both of them away from the defaults so that they do not
> + * conflict.
> + */
> hdmi@3d {
> compatible = "adi,adv7513";
> - reg = <0x3d>;
> + reg = <0x3d>, <0x2d>, <0x4d>, <0x5d>;
> + reg-names = "main", "cec", "edid", "packet";
>
> adi,input-depth = <8>;
> adi,input-colorspace = "rgb";
> @@ -272,7 +279,8 @@
>
> hdmi@39 {
> compatible = "adi,adv7513";
> - reg = <0x39>;
> + reg = <0x39>, <0x29>, <0x49>, <0x59>;
> + reg-names = "main", "cec", "edid", "packet";
>
> adi,input-depth = <8>;
> adi,input-colorspace = "rgb";
--
Regards,
Laurent Pinchart
Re: [PATCH v3 2/5] dt-bindings: adv7511: Add support for i2c_new_secondary_device
Hi Kieran,
Thank you for the patch.
On Tuesday, 13 February 2018 00:07:50 EET Kieran Bingham wrote:
> From: Kieran Bingham
>
> The ADV7511 has four 256-byte maps that can be accessed via the main I²C
> ports. Each map has it own I²C address and acts as a standard slave
> device on the I²C bus.
>
> Extend the device tree node bindings to be able to override the default
> addresses so that address conflicts with other devices on the same bus
> may be resolved at the board description level.
>
> Signed-off-by: Kieran Bingham
> Reviewed-by: Rob Herring
Same comment as for 1/5 about the subject line.
> ---
> v2:
> - Fixed up reg: property description to account for multiple optional
>addresses.
> - Minor reword to commit message to account for DT only change
> - Collected Robs RB tag
>
> v3:
> - Split map register addresses into individual declarations.
>
> .../devicetree/bindings/display/bridge/adi,adv7511.txt | 18 +--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index
> 0047b1394c70..3f85c351dd39 100644
> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> @@ -14,7 +14,13 @@ Required properties:
> "adi,adv7513"
> "adi,adv7533"
>
> -- reg: I2C slave address
> +- reg: I2C slave addresses
> + The ADV7511 internal registers are split into four pages exposed through
> + different I2C addresses, creating four register maps. Each map has it own
> + I2C address and acts as a standard slave device on the I²C bus. The main
> + address is mandatory, others are optional and revert to defaults if not
> + specified.
Nitpicking again, you're mixing I2C and I²C.
> +
>
> The ADV7511 supports a large number of input data formats that differ by
> their color depth, color format, clock mode, bit justification and random
> @@ -70,6 +76,9 @@ Optional properties:
>rather than generate its own timings for HDMI output.
> - clocks: from common clock binding: reference to the CEC clock.
> - clock-names: from common clock binding: must be "cec".
> +- reg-names : Names of maps with programmable addresses.
> + It can contain any map needing a non-default address.
> + Possible maps names are : "main", "edid", "cec", "packet"
>
> Required nodes:
>
> @@ -88,7 +97,12 @@ Example
>
> adv7511w: hdmi@39 {
> compatible = "adi,adv7511w";
> - reg = <39>;
> + /*
> + * The EDID page will be accessible on address 0x66 on the i2c
And now you're using lowercase :-)
> + * bus. All other maps continue to use their default addresses.
> + */
> + reg = <0x39>, <0x66>;
> + reg-names = "main", "edid";
> interrupt-parent = <&gpio3>;
> interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> clocks = <&cec_clock>;
With these fixed (or not, up to you),
Reviewed-by: Laurent Pinchart
--
Regards,
Laurent Pinchart
Re: [PATCH v3 1/5] dt-bindings: media: adv7604: Add support for i2c_new_secondary_device
Hi Kieran,
Thank you for the patch.
On Tuesday, 13 February 2018 00:07:49 EET Kieran Bingham wrote:
> From: Jean-Michel Hautbois
>
> The ADV7604 has thirteen 256-byte maps that can be accessed via the main
> I²C ports. Each map has it own I²C address and acts as a standard slave
> device on the I²C bus.
>
> Extend the device tree node bindings to be able to override the default
> addresses so that address conflicts with other devices on the same bus
> may be resolved at the board description level.
>
> Signed-off-by: Jean-Michel Hautbois
> [Kieran: Re-adapted for mainline]
> Signed-off-by: Kieran Bingham
> Reviewed-by: Rob Herring
Nitpicking, I might not mention i2c_new_secondary_device in the subject, as
this is a DT bindings change. I don't mind too much though, as long as the
bindings themselves don't contain Linux-specific information, and they don't,
so
Reviewed-by: Laurent Pinchart
> ---
> Based upon the original posting :
> https://lkml.org/lkml/2014/10/22/469
>
> v2:
> - DT Binding update separated from code change
> - Minor reword to commit message to account for DT only change.
> - Collected Rob's RB tag.
>
> v3:
> - Split map register addresses into individual declarations.
>
> .../devicetree/bindings/media/i2c/adv7604.txt | 18
> -- 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
> 9cbd92eb5d05..ebb5f070c05b 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> @@ -13,7 +13,11 @@ Required Properties:
> - "adi,adv7611" for the ADV7611
> - "adi,adv7612" for the ADV7612
>
> - - reg: I2C slave address
> + - reg: I2C slave addresses
> +The ADV76xx has up to thirteen 256-byte maps that can be accessed via
> the +main I²C ports. Each map has it own I²C address and acts as a
> standard +slave device on the I²C bus. The main address is mandatory,
> others are +optional and revert to defaults if not specified.
>
>- hpd-gpios: References to the GPIOs that control the HDMI hot-plug
> detection pins, one per HDMI input. The active flag indicates the GPIO
> @@ -35,6 +39,11 @@ Optional Properties:
>
>- reset-gpios: Reference to the GPIO connected to the device's reset pin.
> - default-input: Select which input is selected after reset.
> + - reg-names : Names of maps with programmable addresses.
> + It can contain any map needing a non-default address.
> + Possible maps names are :
> + "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe",
> + "rep", "edid", "hdmi", "test", "cp", "vdp"
>
> Optional Endpoint Properties:
>
> @@ -52,7 +61,12 @@ Example:
>
> hdmi_receiver@4c {
> compatible = "adi,adv7611";
> - reg = <0x4c>;
> + /*
> + * The edid page will be accessible @ 0x66 on the i2c bus. All
> + * other maps will retain their default addresses.
> + */
> + reg = <0x4c>, <0x66>;
> + reg-names "main", "edid";
>
> reset-gpios = <&ioexp 0 GPIO_ACTIVE_LOW>;
> hpd-gpios = <&ioexp 2 GPIO_ACTIVE_HIGH>;
--
Regards,
Laurent Pinchart
drivers/media/dvb-frontends/stb0899_drv.h:151:36: error: weak declaration of 'stb0899_attach' being applied to a already existing, static definition
Hi Wolfgang,
FYI, the error/warning still remains.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
master
head: 178e834c47b0d01352c48730235aae69898fbc02
commit: 6cdeaed3b1420bd2569891be0c4123ff59628e9e media: dvb_usb_pctv452e:
module refcount changes were unbalanced
date: 9 weeks ago
config: x86_64-randconfig-s4-02131647 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
git checkout 6cdeaed3b1420bd2569891be0c4123ff59628e9e
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
In file included from drivers/media/usb/dvb-usb/pctv452e.c:20:0:
drivers/media/usb/dvb-usb/pctv452e.c: In function 'pctv452e_frontend_attach':
>> drivers/media/dvb-frontends/stb0899_drv.h:151:36: error: weak declaration of
>> 'stb0899_attach' being applied to a already existing, static definition
static inline struct dvb_frontend *stb0899_attach(struct stb0899_config
*config,
^~
vim +/stb0899_attach +151 drivers/media/dvb-frontends/stb0899_drv.h
ae9902da9 drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08
150
ae9902da9 drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08
@151 static inline struct dvb_frontend *stb0899_attach(struct stb0899_config
*config,
ae9902da9 drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08
152 struct i2c_adapter *i2c)
ae9902da9 drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08
153 {
ae9902da9 drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08
154printk(KERN_WARNING "%s: Driver disabled by Kconfig\n", __func__);
ae9902da9 drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08
155return NULL;
ae9902da9 drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08
156 }
ae9902da9 drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08
157
:: The code at line 151 was first introduced by commit
:: ae9902da96b4d2d82707706c7fbc93a8e501dde8 V4L/DVB (9417): DVB_ATTACH for
STB0899, STB6100, TDA8261
:: TO: Manu Abraham
:: CC: Mauro Carvalho Chehab
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
include/linux/compiler.h:183: undefined reference to `__tracepoint_vb2_buf_queue'
Hi Mauro,
FYI, the error/warning still remains.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
master
head: 178e834c47b0d01352c48730235aae69898fbc02
commit: 03fbdb2fc2b8bb27b0ee0534fd3e9c57cdc3854a media: move videobuf2 to
drivers/media/common
date: 7 weeks ago
config: x86_64-randconfig-s5-02131532 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
git checkout 03fbdb2fc2b8bb27b0ee0534fd3e9c57cdc3854a
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/media/common/videobuf/videobuf2-core.o: In function
`__read_once_size':
>> include/linux/compiler.h:183: undefined reference to
>> `__tracepoint_vb2_buf_queue'
>> include/linux/compiler.h:183: undefined reference to
>> `__tracepoint_vb2_buf_queue'
>> include/linux/compiler.h:183: undefined reference to
>> `__tracepoint_vb2_buf_queue'
>> include/linux/compiler.h:183: undefined reference to
>> `__tracepoint_vb2_buf_queue'
>> include/linux/compiler.h:183: undefined reference to
>> `__tracepoint_vb2_buf_done'
>> include/linux/compiler.h:183: undefined reference to
>> `__tracepoint_vb2_buf_done'
>> include/linux/compiler.h:183: undefined reference to
>> `__tracepoint_vb2_buf_done'
>> include/linux/compiler.h:183: undefined reference to
>> `__tracepoint_vb2_buf_done'
>> include/linux/compiler.h:183: undefined reference to `__tracepoint_vb2_qbuf'
>> include/linux/compiler.h:183: undefined reference to `__tracepoint_vb2_qbuf'
>> include/linux/compiler.h:183: undefined reference to `__tracepoint_vb2_qbuf'
>> include/linux/compiler.h:183: undefined reference to `__tracepoint_vb2_qbuf'
>> include/linux/compiler.h:183: undefined reference to `__tracepoint_vb2_dqbuf'
>> include/linux/compiler.h:183: undefined reference to `__tracepoint_vb2_dqbuf'
>> include/linux/compiler.h:183: undefined reference to `__tracepoint_vb2_dqbuf'
>> include/linux/compiler.h:183: undefined reference to `__tracepoint_vb2_dqbuf'
drivers/media/common/videobuf/videobuf2-v4l2.o: In function `vb2_poll':
drivers/media/common/videobuf/videobuf2-v4l2.c:678: undefined reference to
`video_devdata'
drivers/media/common/videobuf/videobuf2-v4l2.c:685: undefined reference to
`v4l2_event_pending'
drivers/media/common/videobuf/videobuf2-v4l2.o: In function
`vb2_ioctl_reqbufs':
drivers/media/common/videobuf/videobuf2-v4l2.c:714: undefined reference to
`video_devdata'
drivers/media/common/videobuf/videobuf2-v4l2.o: In function
`vb2_ioctl_create_bufs':
drivers/media/common/videobuf/videobuf2-v4l2.c:733: undefined reference to
`video_devdata'
drivers/media/common/videobuf/videobuf2-v4l2.o: In function
`vb2_ioctl_prepare_buf':
drivers/media/common/videobuf/videobuf2-v4l2.c:759: undefined reference to
`video_devdata'
drivers/media/common/videobuf/videobuf2-v4l2.o: In function
`vb2_ioctl_querybuf':
drivers/media/common/videobuf/videobuf2-v4l2.c:769: undefined reference to
`video_devdata'
drivers/media/common/videobuf/videobuf2-v4l2.o: In function `vb2_ioctl_qbuf':
drivers/media/common/videobuf/videobuf2-v4l2.c:778: undefined reference to
`video_devdata'
drivers/media/common/videobuf/videobuf2-v4l2.o:drivers/media/common/videobuf/videobuf2-v4l2.c:788:
more undefined references to `video_devdata' follow
drivers/media/common/videobuf/videobuf2-v4l2.o: In function
`_vb2_fop_release':
drivers/media/common/videobuf/videobuf2-v4l2.c:848: undefined reference to
`v4l2_fh_release'
vim +183 include/linux/compiler.h
d976441f Andrey Ryabinin 2015-10-19 179
d976441f Andrey Ryabinin 2015-10-19 180 static __always_inline
d976441f Andrey Ryabinin 2015-10-19 181 void __read_once_size(const
volatile void *p, void *res, int size)
230fa253 Christian Borntraeger 2014-11-25 182 {
d976441f Andrey Ryabinin 2015-10-19 @183 __READ_ONCE_SIZE;
230fa253 Christian Borntraeger 2014-11-25 184 }
d976441f Andrey Ryabinin 2015-10-19 185
:: The code at line 183 was first introduced by commit
:: d976441f44bc5d48635d081d277aa76556ffbf8b compiler, atomics, kasan:
Provide READ_ONCE_NOCHECK()
:: TO: Andrey Ryabinin
:: CC: Ingo Molnar
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
