Hi Laurent,
On Thu, Feb 08, 2018 at 10:05:05PM +0200, Laurent Pinchart wrote:
> On Wednesday, 7 February 2018 16:26:43 EET Maxime Ripard wrote:
> > The Cadence MIPI-CSI2 TX controller is an hardware block meant to be used
> > as a bridge between pixel interfaces and a CSI-2 bus.
> >
> > It supports operating with an internal or external D-PHY, with up to 4
> > lanes, or without any D-PHY. The current code only supports the former
> > case.
> >
> > While the virtual channel input on the pixel interface can be directly
> > mapped to CSI2, the datatype input is actually a selection signal (3-bits)
> > mapping to a table of up to 8 preconfigured datatypes/formats (programmed
> > at start-up)
> >
> > The block supports up to 8 input datatypes.
>
> The DT bindings document four input streams. Does this mean that, to use more
> than 4 data types, a system would need to transmit multiplexed streams on a
> single input stream with the 3 selection bits qualifying each sample ? That
> would be an interesting setup.
My understanding is that each input stream has an additional signal
that defines a data type encoded on 3 bits. So yeah, I guess that
would be possible if the upstream device is able to synchronize its
stream generation and the datatype sent.
> > Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> > ---
> > drivers/media/platform/cadence/Kconfig | 6 +
> > drivers/media/platform/cadence/Makefile | 1 +
> > drivers/media/platform/cadence/cdns-csi2tx.c | 582 ++++++++++++++++++++++++
> > 3 files changed, 589 insertions(+)
> > create mode 100644 drivers/media/platform/cadence/cdns-csi2tx.c
> >
> > diff --git a/drivers/media/platform/cadence/Kconfig
> > b/drivers/media/platform/cadence/Kconfig index d1b6bbb6a0eb..db49328ee8b2
> > 100644
> > --- a/drivers/media/platform/cadence/Kconfig
> > +++ b/drivers/media/platform/cadence/Kconfig
> > @@ -9,4 +9,10 @@ config VIDEO_CADENCE_CSI2RX
> > depends on VIDEO_V4L2_SUBDEV_API
> > select V4L2_FWNODE
> >
> > +config VIDEO_CADENCE_CSI2TX
> > + tristate "Cadence MIPI-CSI2 TX Controller"
> > + depends on MEDIA_CONTROLLER
> > + depends on VIDEO_V4L2_SUBDEV_API
> > + select V4L2_FWNODE
>
> A bit of documentation maybe ?
Yeah, it was already queued for the next iteration :)
> > +static const struct v4l2_mbus_framefmt fmt_default = {
> > + .width = 320,
> > + .height = 180,
>
> That's a pretty small default resolution. Is there any reason for not using a
> more common format ?
What would be your suggestion?
> > +static int csi2tx_init_cfg(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *cfg)
> > +{
> > + struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
> > + unsigned int i;
> > +
> > + for (i = 0; i < subdev->entity.num_pads; i++)
> > + csi2tx->pad_fmts[i] = fmt_default;
>
> .init_cfg() should initialize the formats stored in the cfg structure. The
> active formats stored in your driver-specific structure should be initialized
> at probe time.
Ok, I'll change it.
> > +static int csi2tx_set_pad_format(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct csi2tx_priv *csi2tx = v4l2_subdev_to_csi2tx(subdev);
> > +
> > + csi2tx->pad_fmts[fmt->pad] = fmt->format;
>
> Doesn't the IP have frame width or height limitations ? Or format
> limitations ?
In its current state, not really. There's also no clear limitations
from the registers on the formats / resolution used, since it's not
configured at all in the device.
The only constraint is that there's no buffer in the IP, so the input
data rate and the output data rate should match. However, the way to
do that is currently uncertain, so I guess we can address that later
on.
> > +static void csi2tx_reset(struct csi2tx_priv *csi2tx)
> > +{
> > + writel(CSI2TX_CONFIG_SRST_REQ, csi2tx->base + CSI2TX_CONFIG_REG);
> > +
> > + usleep_range(10, 20);
>
> As for the RX driver, udelay() might be better. Same comment for the other
> small delays below.
I was asked by Benoit to change it to usleep_range in an earlier
iteration, which one should I use?
I'll change the driver according to your other comments.
Thanks!
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
Attachment:
signature.asc
Description: PGP signature
