devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v13 6/8] media: i2c: add DS90UB960 driver
@ 2023-05-22 11:42 Ludwig Zenz
  0 siblings, 0 replies; 9+ messages in thread
From: Ludwig Zenz @ 2023-05-22 11:42 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: Matti.Vaittinen@fi.rohmeurope.com, andriy.shevchenko@intel.com,
	andriy.shevchenko@linux.intel.com, broonie@kernel.org,
	devicetree@vger.kernel.org, hverkuil@xs4all.nl, khalasa@piap.pl,
	krzysztof.kozlowski+dt@linaro.org,
	laurent.pinchart+renesas@ideasonboard.com, lgirdwood@gmail.com,
	linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-media@vger.kernel.org, luca.ceresoli@bootlin.com,
	m.tretter@pengutronix.de, marex@denx.de, mchehab@kernel.org,
	mpagano@gentoo.org, peda@axentia.se, robh+dt@kernel.org,
	sakari.ailus@linux.intel.com, satish.nagireddy@getcruise.com,
	wsa@kernel.org

> On 16/05/2023 22:05, Ludwig Zenz wrote:
>>> On 16/05/2023 16:32, Ludwig Zenz wrote:
>>>>> 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.
>>
>>> 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.
>>
>>> This assumes that the forwarding is disabled in the registers by default (which it is in UB960).
>>>
>>> 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.
>>
>>> 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.
>>>
>>> So. You should see the rx0 getting enabled (later, in ub960_enable_rx_port),
> and I'm curious why you don't see that.
>>
>> 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 */
> 
> -       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 */
> 
> -               fwd_ctl |= BIT(4 + nport); /* forward disable */
> -
>                  if (rx_data[nport].tx_port == 1)
>                          fwd_ctl |= BIT(nport); /* forward to TX1 */
>                  else
> 

Hello Tomi,

yes, this does fix the issue. I was able to test it successfully today with my hardware (ds90ub933 + ds90ub964 + i.MX8Mplus).

Thanks and regards,
Ludwig

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v13 6/8] media: i2c: add DS90UB960 driver
@ 2023-05-16 19:05 Ludwig Zenz
  2023-05-19 12:19 ` Tomi Valkeinen
  0 siblings, 1 reply; 9+ messages in thread
From: Ludwig Zenz @ 2023-05-16 19:05 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: Matti.Vaittinen@fi.rohmeurope.com, andriy.shevchenko@intel.com,
	andriy.shevchenko@linux.intel.com, broonie@kernel.org,
	devicetree@vger.kernel.org, hverkuil@xs4all.nl, khalasa@piap.pl,
	krzysztof.kozlowski+dt@linaro.org,
	laurent.pinchart+renesas@ideasonboard.com, lgirdwood@gmail.com,
	linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-media@vger.kernel.org, luca.ceresoli@bootlin.com,
	m.tretter@pengutronix.de, marex@denx.de, mchehab@kernel.org,
	mpagano@gentoo.org, peda@axentia.se, robh+dt@kernel.org,
	sakari.ailus@linux.intel.com, satish.nagireddy@getcruise.com,
	wsa@kernel.org

> On 16/05/2023 16:32, Ludwig Zenz wrote:
>>> 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.

> 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.

> This assumes that the forwarding is disabled in the registers by default (which it is in UB960).
> 
> 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.

> 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.
> 
> So. You should see the rx0 getting enabled (later, in ub960_enable_rx_port), and I'm curious why you don't see that.

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.

regards,
Ludwig

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v13 6/8] media: i2c: add DS90UB960 driver
@ 2023-05-16 13:32 Ludwig Zenz
  2023-05-16 13:58 ` Tomi Valkeinen
  0 siblings, 1 reply; 9+ messages in thread
From: Ludwig Zenz @ 2023-05-16 13:32 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: Matti.Vaittinen@fi.rohmeurope.com, andriy.shevchenko@intel.com,
	andriy.shevchenko@linux.intel.com, broonie@kernel.org,
	devicetree@vger.kernel.org, hverkuil@xs4all.nl, khalasa@piap.pl,
	krzysztof.kozlowski+dt@linaro.org,
	laurent.pinchart+renesas@ideasonboard.com, lgirdwood@gmail.com,
	linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-media@vger.kernel.org, luca.ceresoli@bootlin.com,
	m.tretter@pengutronix.de, marex@denx.de, mchehab@kernel.org,
	mpagano@gentoo.org, peda@axentia.se, robh+dt@kernel.org,
	sakari.ailus@linux.intel.com, satish.nagireddy@getcruise.com,
	wsa@kernel.org

> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH v13 0/8] i2c-atr and FPDLink
@ 2023-04-26 11:51 Tomi Valkeinen
  2023-04-26 11:51 ` [PATCH v13 6/8] media: i2c: add DS90UB960 driver Tomi Valkeinen
  0 siblings, 1 reply; 9+ messages in thread
From: Tomi Valkeinen @ 2023-04-26 11:51 UTC (permalink / raw)
  To: linux-media, devicetree, linux-kernel, linux-i2c, Rob Herring,
	Krzysztof Kozlowski, Wolfram Sang, Luca Ceresoli, Andy Shevchenko,
	Matti Vaittinen, Laurent Pinchart
  Cc: Mauro Carvalho Chehab, Peter Rosin, Liam Girdwood, Mark Brown,
	Sakari Ailus, Michael Tretter, Hans Verkuil, Mike Pagano,
	Krzysztof Hałasa, Marek Vasut, Satish Nagireddy,
	Tomi Valkeinen

Hi,

You can find v12 from:

https://lore.kernel.org/all/20230425072601.51031-1-tomi.valkeinen@ideasonboard.com/

Diff to v12 is included below.

The changes are:

- Change the i2c-alias-pool to u32 array. The i2c-atr.c will reject any
  addresses that have bits in the upper 16 bits. This could be changed
  later if those bits are needed.

- Add maxItems to ub960's i2c-alias-pool. ub960 has 4 rx ports, and each
  can have 8 aliases, so the absolute maximum is 32.

 Tomi

Luca Ceresoli (1):
  i2c: add I2C Address Translator (ATR) support

Tomi Valkeinen (7):
  dt-bindings: i2c: Add I2C Address Translator (ATR)
  dt-bindings: media: add TI DS90UB913 FPD-Link III Serializer
  dt-bindings: media: add TI DS90UB953 FPD-Link III Serializer
  dt-bindings: media: add TI DS90UB960 FPD-Link III Deserializer
  media: i2c: add DS90UB960 driver
  media: i2c: add DS90UB913 driver
  media: i2c: add DS90UB953 driver

 .../devicetree/bindings/i2c/i2c-atr.yaml      |   34 +
 .../bindings/media/i2c/ti,ds90ub913.yaml      |  133 +
 .../bindings/media/i2c/ti,ds90ub953.yaml      |  134 +
 .../bindings/media/i2c/ti,ds90ub960.yaml      |  427 ++
 Documentation/i2c/i2c-address-translators.rst |   96 +
 Documentation/i2c/index.rst                   |    1 +
 MAINTAINERS                                   |   16 +
 drivers/i2c/Kconfig                           |    9 +
 drivers/i2c/Makefile                          |    1 +
 drivers/i2c/i2c-atr.c                         |  710 +++
 drivers/media/i2c/Kconfig                     |   47 +
 drivers/media/i2c/Makefile                    |    3 +
 drivers/media/i2c/ds90ub913.c                 |  906 ++++
 drivers/media/i2c/ds90ub953.c                 | 1400 ++++++
 drivers/media/i2c/ds90ub960.c                 | 4049 +++++++++++++++++
 include/linux/i2c-atr.h                       |  116 +
 include/media/i2c/ds90ub9xx.h                 |   22 +
 17 files changed, 8104 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-atr.yaml
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
 create mode 100644 Documentation/i2c/i2c-address-translators.rst
 create mode 100644 drivers/i2c/i2c-atr.c
 create mode 100644 drivers/media/i2c/ds90ub913.c
 create mode 100644 drivers/media/i2c/ds90ub953.c
 create mode 100644 drivers/media/i2c/ds90ub960.c
 create mode 100644 include/linux/i2c-atr.h
 create mode 100644 include/media/i2c/ds90ub9xx.h

Interdiff against v12:
diff --git a/Documentation/devicetree/bindings/i2c/i2c-atr.yaml b/Documentation/devicetree/bindings/i2c/i2c-atr.yaml
index 470cc6c9af35..1939ab339bfc 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-atr.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-atr.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/i2c/i2c-atr.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Common i2c address translator properties.
+title: Common i2c address translator properties
 
 maintainers:
   - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
@@ -20,7 +20,7 @@ description:
 
 properties:
   i2c-alias-pool:
-    $ref: /schemas/types.yaml#/definitions/uint16-array
+    $ref: /schemas/types.yaml#/definitions/uint32-array
     description:
       I2C alias pool is a pool of I2C addresses on the main I2C bus that can be
       used to access the remote peripherals on the serializer's I2C bus. The
diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
index 1d5362bea09a..289737721c2c 100644
--- a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
@@ -13,6 +13,9 @@ description:
   The TI DS90UB9XX devices are FPD-Link video deserializers with I2C and GPIO
   forwarding.
 
+allOf:
+  - $ref: /schemas/i2c/i2c-atr.yaml#
+
 properties:
   compatible:
     enum:
@@ -37,7 +40,8 @@ properties:
       Specifier for the GPIO connected to the PDB pin.
 
   i2c-alias-pool:
-    $ref: /schemas/i2c/i2c-atr.yaml#/properties/i2c-alias-pool
+    minItems: 1
+    maxItems: 32
 
   links:
     type: object
@@ -225,7 +229,7 @@ required:
   - clock-names
   - ports
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |
@@ -245,7 +249,7 @@ examples:
 
         powerdown-gpios = <&pca9555 7 GPIO_ACTIVE_LOW>;
 
-        i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
+        i2c-alias-pool = <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
 
         ports {
           #address-cells = <1>;
diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 402182a04efd..8ca1daadec93 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -428,10 +428,12 @@ static int i2c_atr_parse_alias_pool(struct i2c_atr *atr)
 	struct device *dev = atr->dev;
 	unsigned long *alias_use_mask;
 	size_t num_aliases;
-	u16 *aliases;
+	unsigned int i;
+	u32 *aliases32;
+	u16 *aliases16;
 	int ret;
 
-	ret = fwnode_property_count_u16(dev_fwnode(dev), "i2c-alias-pool");
+	ret = fwnode_property_count_u32(dev_fwnode(dev), "i2c-alias-pool");
 	if (ret < 0) {
 		dev_err(dev, "Failed to count 'i2c-alias-pool' property: %d\n",
 			ret);
@@ -443,32 +445,56 @@ static int i2c_atr_parse_alias_pool(struct i2c_atr *atr)
 	if (!num_aliases)
 		return 0;
 
-	aliases = kcalloc(num_aliases, sizeof(*aliases), GFP_KERNEL);
-	if (!aliases)
+	aliases32 = kcalloc(num_aliases, sizeof(*aliases32), GFP_KERNEL);
+	if (!aliases32)
 		return -ENOMEM;
 
-	ret = fwnode_property_read_u16_array(dev_fwnode(dev), "i2c-alias-pool",
-					     aliases, num_aliases);
+	ret = fwnode_property_read_u32_array(dev_fwnode(dev), "i2c-alias-pool",
+					     aliases32, num_aliases);
 	if (ret < 0) {
 		dev_err(dev, "Failed to read 'i2c-alias-pool' property: %d\n",
 			ret);
-		kfree(aliases);
-		return ret;
+		goto err_free_aliases32;
+	}
+
+	aliases16 = kcalloc(num_aliases, sizeof(*aliases16), GFP_KERNEL);
+	if (!aliases16) {
+		ret = -ENOMEM;
+		goto err_free_aliases32;
+	}
+
+	for (i = 0; i < num_aliases; i++) {
+		if (!(aliases32[i] & 0xffff0000)) {
+			aliases16[i] = aliases32[i];
+			continue;
+		}
+
+		dev_err(dev, "Failed to parse 'i2c-alias-pool' property: I2C flags are not supported\n");
+		ret = -EINVAL;
+		goto err_free_aliases16;
 	}
 
 	alias_use_mask = bitmap_zalloc(num_aliases, GFP_KERNEL);
 	if (!alias_use_mask) {
-		kfree(aliases);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_free_aliases16;
 	}
 
+	kfree(aliases32);
+
 	atr->num_aliases = num_aliases;
-	atr->aliases = aliases;
+	atr->aliases = aliases16;
 	atr->alias_use_mask = alias_use_mask;
 
 	dev_dbg(dev, "i2c-alias-pool has %zu aliases", atr->num_aliases);
 
 	return 0;
+
+err_free_aliases16:
+	kfree(aliases16);
+err_free_aliases32:
+	kfree(aliases32);
+	return ret;
 }
 
 struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-05-22 11:44 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-22 11:42 [PATCH v13 6/8] media: i2c: add DS90UB960 driver Ludwig Zenz
  -- strict thread matches above, loose matches on Subject: below --
2023-05-16 19:05 Ludwig Zenz
2023-05-19 12:19 ` Tomi Valkeinen
2023-05-16 13:32 Ludwig Zenz
2023-05-16 13:58 ` Tomi Valkeinen
2023-04-26 11:51 [PATCH v13 0/8] i2c-atr and FPDLink Tomi Valkeinen
2023-04-26 11:51 ` [PATCH v13 6/8] media: i2c: add DS90UB960 driver Tomi Valkeinen
2023-05-16 12:35   ` Ludwig Zenz
2023-05-16 13:02     ` Tomi Valkeinen
2023-05-16 12:44   ` Sakari Ailus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).