Re: [PATCH] tc358743: fix connected/active CSI-2 lane reporting

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Philipp

On 21 September 2017 at 11:24, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote:
> g_mbus_config was supposed to indicate all supported lane numbers, not
> only the number of those currently in active use. Since the tc358743
> can dynamically reduce the number of active lanes if the required
> bandwidth allows for it, report all lane numbers up to the connected
> number of lanes as supported.
> To allow communicating the number of currently active lanes, add a new
> bitfield to the v4l2_mbus_config flags. This is a temporary fix, until
> a better solution is found.
>
> Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> ---
>  drivers/media/i2c/tc358743.c  | 22 ++++++++++++----------
>  include/media/v4l2-mediabus.h |  8 ++++++++
>  2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> index e6f5c363ccab5..e2a9e6a18a49d 100644
> --- a/drivers/media/i2c/tc358743.c
> +++ b/drivers/media/i2c/tc358743.c
> @@ -1464,21 +1464,22 @@ static int tc358743_g_mbus_config(struct v4l2_subdev *sd,
>         /* Support for non-continuous CSI-2 clock is missing in the driver */
>         cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
>
> -       switch (state->csi_lanes_in_use) {
> -       case 1:
> +       if (state->bus.num_data_lanes > 0)
>                 cfg->flags |= V4L2_MBUS_CSI2_1_LANE;
> -               break;
> -       case 2:
> +       if (state->bus.num_data_lanes > 1)
>                 cfg->flags |= V4L2_MBUS_CSI2_2_LANE;
> -               break;
> -       case 3:
> +       if (state->bus.num_data_lanes > 2)
>                 cfg->flags |= V4L2_MBUS_CSI2_3_LANE;
> -               break;
> -       case 4:
> +       if (state->bus.num_data_lanes > 3)
>                 cfg->flags |= V4L2_MBUS_CSI2_4_LANE;
> -               break;
> -       default:
> +
> +       if (state->csi_lanes_in_use > 4)

One could suggest
if (state->csi_lanes_in_use > state->bus.num_data_lanes)
here. Needing to use more lanes than are configured is surely an
error, although that may be detectable at the other end. See below
too.

>                 return -EINVAL;
> +
> +       if (state->csi_lanes_in_use < state->bus.num_data_lanes) {
> +               const u32 mask = V4L2_MBUS_CSI2_LANE_MASK;
> +
> +               cfg->flags |= (state->csi_lanes_in_use << __ffs(mask)) & mask;
>         }
>
>         return 0;
> @@ -1885,6 +1886,7 @@ static int tc358743_probe(struct i2c_client *client,
>         if (pdata) {
>                 state->pdata = *pdata;
>                 state->bus.flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
> +               state->bus.num_data_lanes = 4;
>         } else {
>                 err = tc358743_probe_of(state);
>                 if (err == -ENODEV)
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 93f8afcb7a220..3467d97be5f5b 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -63,6 +63,14 @@
>                                          V4L2_MBUS_CSI2_3_LANE | V4L2_MBUS_CSI2_4_LANE)
>  #define V4L2_MBUS_CSI2_CHANNELS                (V4L2_MBUS_CSI2_CHANNEL_0 | V4L2_MBUS_CSI2_CHANNEL_1 | \
>                                          V4L2_MBUS_CSI2_CHANNEL_2 | V4L2_MBUS_CSI2_CHANNEL_3)
> +/*
> + * Number of lanes in use, 0 == use all available lanes (default)
> + *
> + * This is a temporary fix for devices that need to reduce the number of active
> + * lanes for certain modes, until g_mbus_config() can be replaced with a better
> + * solution.
> + */
> +#define V4L2_MBUS_CSI2_LANE_MASK                (3 << 10)

I know this was Hans' suggested define, but are we saying 4 lanes is
not a valid value? If it is then the mask needs to be at least (7 <<
10).

4 lanes is not necessarily "all available lanes".
- There are now CSI2 devices supporting up to 8 lanes (although
V4L2_FWNODE_CSI2_MAX_DATA_LANES limits you to 4 at the moment).
- Or you could have 2 lanes configured in DT and ask TC358743 for (eg)
1080P60 UYVY at 594Mbps (needs 4 lanes) which passes the current logic
in the TC358743 driver and would return 0, when it is actually going
to use 4 lanes. That could be classed as a driver bug though.

My view is that if a driver is going to report how many lanes to use
then it should always report it explicitly. The default 0 value should
only be used for devices that will never change it from the DT
settings. Perhaps others disagree

Otherwise the patch works for me.

  Dave.

>  /**
>   * enum v4l2_mbus_type - media bus type
> --
> 2.11.0
>



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux