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

[Ludwig Zenz <lzenz@dh-electronics.com>]

> Hi,
> 
> On 16/05/2023 15:35, Ludwig Zenz wrote:
>> 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:
> 
> Why doesn't it work? What happens?
> 
>  Tomi

Hello Tomi,

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.

>>>  +		/* 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.

regards,
Ludwig