linux-kernel - Re: [PATCH v13 6/8] media: i2c: add DS90UB960 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 16 May 2023 14:35:49 +0200
From:   Ludwig Zenz <lzenz@...electronics.com>
To:     <tomi.valkeinen@...asonboard.com>
CC:     <Matti.Vaittinen@...rohmeurope.com>, <andriy.shevchenko@...el.com>,
        <andriy.shevchenko@...ux.intel.com>, <broonie@...nel.org>,
        <devicetree@...r.kernel.org>, <hverkuil@...all.nl>,
        <khalasa@...p.pl>, <krzysztof.kozlowski+dt@...aro.org>,
        <laurent.pinchart+renesas@...asonboard.com>, <lgirdwood@...il.com>,
        <linux-i2c@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-media@...r.kernel.org>, <luca.ceresoli@...tlin.com>,
        <m.tretter@...gutronix.de>, <marex@...x.de>, <mchehab@...nel.org>,
        <mpagano@...too.org>, <peda@...ntia.se>, <robh+dt@...nel.org>,
        <sakari.ailus@...ux.intel.com>, <satish.nagireddy@...cruise.com>,
        <wsa@...nel.org>
Subject: Re: [PATCH v13 6/8] media: i2c: add DS90UB960 driver

On Wed, 26 Apr 2023 14:51:12 +0300, Tomi Valkeinen wrote:

[...]

>  +static int ub960_configure_ports_for_streaming(struct ub960_data *priv,
>  +					       struct v4l2_subdev_state *state)
>  +{
>  +	u8 fwd_ctl;
>  +	struct {
>  +		u32 num_streams;
>  +		u8 pixel_dt;
>  +		u8 meta_dt;
>  +		u32 meta_lines;
>  +		u32 tx_port;
>  +	} rx_data[UB960_MAX_RX_NPORTS] = {};
>  +	u8 vc_map[UB960_MAX_RX_NPORTS] = {};
>  +	struct v4l2_subdev_route *route;
>  +	unsigned int nport;
>  +	int ret;
>  +
>  +	ret = ub960_validate_stream_vcs(priv);
>  +	if (ret)
>  +		return ret;
>  +
>  +	ub960_get_vc_maps(priv, state, vc_map);
>  +
>  +	for_each_active_route(&state->routing, route) {
>  +		struct ub960_rxport *rxport;
>  +		struct ub960_txport *txport;
>  +		struct v4l2_mbus_framefmt *fmt;
>  +		const struct ub960_format_info *ub960_fmt;
>  +		unsigned int nport;
>  +
>  +		nport = ub960_pad_to_port(priv, route->sink_pad);
>  +
>  +		rxport = priv->rxports[nport];
>  +		if (!rxport)
>  +			return -EINVAL;
>  +
>  +		txport = priv->txports[ub960_pad_to_port(priv, route->source_pad)];
>  +		if (!txport)
>  +			return -EINVAL;
>  +
>  +		rx_data[nport].tx_port = ub960_pad_to_port(priv, route->source_pad);
>  +
>  +		rx_data[nport].num_streams++;
>  +
>  +		/* For the rest, we are only interested in parallel busses */
>  +		if (rxport->rx_mode == RXPORT_MODE_CSI2_SYNC ||
>  +		    rxport->rx_mode == RXPORT_MODE_CSI2_ASYNC)
>  +			continue;
>  +
>  +		if (rx_data[nport].num_streams > 2)
>  +			return -EPIPE;
>  +
>  +		fmt = v4l2_subdev_state_get_stream_format(state,
>  +							  route->sink_pad,
>  +							  route->sink_stream);
>  +		if (!fmt)
>  +			return -EPIPE;
>  +
>  +		ub960_fmt = ub960_find_format(fmt->code);
>  +		if (!ub960_fmt)
>  +			return -EPIPE;
>  +
>  +		if (ub960_fmt->meta) {
>  +			if (fmt->height > 3) {
>  +				dev_err(&priv->client->dev,
>  +					"rx%u: unsupported metadata height %u\n",
>  +					nport, fmt->height);
>  +				return -EPIPE;
>  +			}
>  +
>  +			rx_data[nport].meta_dt = ub960_fmt->datatype;
>  +			rx_data[nport].meta_lines = fmt->height;
>  +		} else {
>  +			rx_data[nport].pixel_dt = ub960_fmt->datatype;
>  +		}
>  +	}
>  +
>  +	/* Configure RX ports */
>  +
>  +	fwd_ctl = 0;

Hello, I have only used the first RX port in my setup (ds90ub933 to ds90ub964). The logic for activating/deactivating the Rx ports did not work for me. My suggestion is:

-	fwd_ctl = 0;
+	fwd_ctl = 0xF0; /* Default: Disable forwarding for all RX ports */

>  +
>  +	for (nport = 0; nport < priv->hw_data->num_rxports; nport++) {
>  +		struct ub960_rxport *rxport = priv->rxports[nport];
>  +		u8 vc = vc_map[nport];
>  +
>  +		if (rx_data[nport].num_streams == 0)
>  +			continue;
>  +
>  +		switch (rxport->rx_mode) {
>  +		case RXPORT_MODE_RAW10:
>  +			ub960_rxport_write(priv, nport, UB960_RR_RAW10_ID,
>  +				rx_data[nport].pixel_dt | (vc << UB960_RR_RAW10_ID_VC_SHIFT));
>  +
>  +			ub960_rxport_write(priv, rxport->nport,
>  +				UB960_RR_RAW_EMBED_DTYPE,
>  +				(rx_data[nport].meta_lines << UB960_RR_RAW_EMBED_DTYPE_LINES_SHIFT) |
>  +					rx_data[nport].meta_dt);
>  +
>  +			break;
>  +
>  +		case RXPORT_MODE_RAW12_HF:
>  +		case RXPORT_MODE_RAW12_LF:
>  +			/* Not implemented */
>  +			break;
>  +
>  +		case RXPORT_MODE_CSI2_SYNC:
>  +		case RXPORT_MODE_CSI2_ASYNC:
>  +			if (!priv->hw_data->is_ub9702) {
>  +				/* Map all VCs from this port to the same VC */
>  +				ub960_rxport_write(priv, nport, UB960_RR_CSI_VC_MAP,
>  +						   (vc << UB960_RR_CSI_VC_MAP_SHIFT(3)) |
>  +						   (vc << UB960_RR_CSI_VC_MAP_SHIFT(2)) |
>  +						   (vc << UB960_RR_CSI_VC_MAP_SHIFT(1)) |
>  +						   (vc << UB960_RR_CSI_VC_MAP_SHIFT(0)));
>  +			} else {
>  +				unsigned int i;
>  +
>  +				/* Map all VCs from this port to VC(nport) */
>  +				for (i = 0; i < 8; i++)
>  +					ub960_rxport_write(priv, nport,
>  +							   UB960_RR_VC_ID_MAP(i),
>  +							   nport);
>  +			}
>  +
>  +			break;
>  +		}
>  +
>  +		/* Forwarding */
>  +
>  +		fwd_ctl |= BIT(4 + nport); /* forward disable */

-		fwd_ctl |= BIT(4 + nport); /* forward disable */
+		fwd_ctl &= ~UB960_SR_FWD_CTL1_PORT_DIS(nport); /* clear disable
+								  forwarding */

According to the data sheet, the ds90ub960 and the ds90ub964 should be identical, so this change should fit both devices.

>  +		if (rx_data[nport].tx_port == 1)
>  +			fwd_ctl |= BIT(nport); /* forward to TX1 */
>  +		else
>  +			fwd_ctl &= ~BIT(nport); /* forward to TX0 */
>  +	}
>  +
>  +	ub960_write(priv, UB960_SR_FWD_CTL1, fwd_ctl);
>  +
>  +	return 0;
>  +}

[...]

Thanks and regards,
Ludwig Zenz