media: i2c: add DS90UB960 driver

Quoted 109 lines

Quoted 96 lines · Ludwig Zenz

Quoted 95 lines

Quoted 87 lines

Hi,

Quoted 84 lines · Ludwig Zenz

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

[...]

Quoted 79 lines

+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;

Quoted 5 lines

Quoted 2 lines

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:

Why doesn't it work? What happens?

Tomi

Hello Tomi,

Quoted 8 lines

Quoted 6 lines

the port rx0 which I need was disabled and the other ports rx1 to rx3 were enabled. In other words, the exact inverse of the required selection.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

+ /* Forwarding */
+
+ fwd_ctl |= BIT(4 + nport); /* forward disable */

According to the data sheet, a set bit4-7 in fwd_ctl means that the channel is disabled. So the comment 'forward disable' is correct. While debugging, however, this code was only reached for the ports to be enabled but not for the ones which should be disabled.

This is just a setup phase, where we initialize the registers for the ports we want to use. The forwarding is then enabled later, in ub960_enable_rx_port, and even later disabled in ub960_disable_rx_port.

Thank you for the clarification. I had misinterpreted the intention of the code here.

Quoted 2 lines

This assumes that the forwarding is disabled in the registers by default (which it is in UB960).

Quoted 9 lines

Quoted 5 lines

I need to try this on my HW to verify my understanding is correct, but looking at the code, it is indeed a bit buggy.

At this setup phase we disable the forwarding for ports we'll use, and enable the forwarding for ports we don't use (which doesn't make sense). Later, when the streaming is started for that port, we enable the forwarding. So here we should just always disable the forwarding for all ports.

The unused Rx ports were not disabled in my tests. Disabling all ports here should also work for my setup.

Quoted 1 line

Saying "disable the forwarding" is perhaps a bit confusing here, as the the

forwarding should already be disabled in the HW here anyway. But as we write the UB960_SR_FWD_CTL1, we need to set that bit.

Quoted 2 lines

Quoted 2 lines

So. You should see the rx0 getting enabled (later, in ub960_enable_rx_port),

and I'm curious why you don't see that.

Quoted 2 lines

I will have another look at that next week. It could well be that in the end only

the enabled but unused ports are the problem.

This should fix the issue:

@ -2486,7 +2488,7 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv,

         /* Configure RX ports */

(quoted diff) +2 -1

-       fwd_ctl = 0;
+       fwd_ctl = GENMASK(7, 4);
 
         for (nport = 0; nport < priv->hw_data->num_rxports; nport++) {
                 struct ub960_rxport *rxport = priv->rxports[nport]; @@ -2536,8
+2538,6 @@ static int ub960_configure_ports_for_streaming(struct

ub960_data *priv,

                 /* Forwarding */