Re: [PATCH 08/14] media: sunxi: Add support for the A31 MIPI CSI-2 controller

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 5883 bytes --]

On Wed, Nov 04, 2020 at 12:34:58PM +0100, Paul Kocialkowski wrote:
> > > +	regmap_write(regmap, SUN6I_MIPI_CSI2_CFG_REG,
> > > +		     SUN6I_MIPI_CSI2_CFG_CHANNEL_MODE(1) |
> > > +		     SUN6I_MIPI_CSI2_CFG_LANE_COUNT(lanes_count));
> > 
> > It's not really clear what the channel is here? The number of virtual
> > channels? Something else?
> 
> That's somewhat described in the controller documentation. Channels refers to
> physical channels of the controller, which can be used to redirect data
> matching either a specific data type, a specific virtual channel, or both.
> There's a somewhat similar concept of channels in the CSI controller too.
> 
> We're currently only using one...
> 
> > > +	regmap_write(regmap, SUN6I_MIPI_CSI2_VCDT_RX_REG,
> > > +		     SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(3, 3) |
> > > +		     SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(2, 2) |
> > > +		     SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(1, 1) |
> > > +		     SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(0, 0) |
> > > +		     SUN6I_MIPI_CSI2_VCDT_RX_CH_DT(0, data_type));
> 
> ... but it's safer to configure them all to virtual channel numbers so we don't
> end up with multiple channels matching virtual channel 0.
> 
> I'll add a comment about that.

Maybe we should have pads for all of them then, even if we don't support
changing anything?

> > > +static const struct v4l2_subdev_pad_ops sun6i_mipi_csi2_subdev_pad_ops = {
> > > +	.enum_mbus_code		= sun6i_mipi_csi2_enum_mbus_code,
> > > +	.get_fmt		= sun6i_mipi_csi2_get_fmt,
> > > +	.set_fmt		= sun6i_mipi_csi2_set_fmt,
> > > +	.enum_frame_size	= sun6i_mipi_csi2_enum_frame_size,
> > > +	.enum_frame_interval	= sun6i_mipi_csi2_enum_frame_interval,
> > > +};
> > > +
> > > +/* Subdev */
> > > +
> > > +static const struct v4l2_subdev_ops sun6i_mipi_csi2_subdev_ops = {
> > > +	.core		= &sun6i_mipi_csi2_subdev_core_ops,
> > > +	.video		= &sun6i_mipi_csi2_subdev_video_ops,
> > > +	.pad		= &sun6i_mipi_csi2_subdev_pad_ops,
> > > +};
> > > +
> > > +/* Notifier */
> > > +
> > > +static int sun6i_mipi_csi2_notifier_bound(struct v4l2_async_notifier *notifier,
> > > +					  struct v4l2_subdev *remote_subdev,
> > > +					  struct v4l2_async_subdev *remote_subdev_async)
> > > +{
> > > +	struct v4l2_subdev *subdev = notifier->sd;
> > > +	struct sun6i_mipi_csi2_video *video =
> > > +		sun6i_mipi_csi2_subdev_video(subdev);
> > > +	struct sun6i_mipi_csi2_dev *cdev = sun6i_mipi_csi2_video_dev(video);
> > > +	int source_pad;
> > > +	int ret;
> > > +
> > > +	source_pad = media_entity_get_fwnode_pad(&remote_subdev->entity,
> > > +						 remote_subdev->fwnode,
> > > +						 MEDIA_PAD_FL_SOURCE);
> > > +	if (source_pad < 0)
> > > +		return source_pad;
> > > +
> > > +	ret = media_create_pad_link(&remote_subdev->entity, source_pad,
> > > +				    &subdev->entity, 0,
> > > +				    MEDIA_LNK_FL_ENABLED |
> > > +				    MEDIA_LNK_FL_IMMUTABLE);
> > > +	if (ret) {
> > > +		dev_err(cdev->dev, "failed to create %s:%u -> %s:%u link\n",
> > > +			remote_subdev->entity.name, source_pad,
> > > +			subdev->entity.name, 0);
> > > +		return ret;
> > > +	}
> > > +
> > > +	video->remote_subdev = remote_subdev;
> > > +	video->remote_pad_index = source_pad;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct v4l2_async_notifier_operations sun6i_mipi_csi2_notifier_ops = {
> > > +	.bound		= sun6i_mipi_csi2_notifier_bound,
> > > +};
> > > +
> > > +/* Media Entity */
> > > +
> > > +static int sun6i_mipi_csi2_link_validate(struct media_link *link)
> > > +{
> > > +	struct v4l2_subdev *subdev =
> > > +		container_of(link->sink->entity, struct v4l2_subdev, entity);
> > > +	struct sun6i_mipi_csi2_video *video =
> > > +		sun6i_mipi_csi2_subdev_video(subdev);
> > > +	struct v4l2_subdev *remote_subdev;
> > > +	struct v4l2_subdev_format format = { 0 };
> > > +	int ret;
> > > +
> > > +	if (!is_media_entity_v4l2_subdev(link->source->entity))
> > > +		return -EINVAL;
> > > +
> > > +	remote_subdev = media_entity_to_v4l2_subdev(link->source->entity);
> > > +
> > > +	format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > +	format.pad = link->source->index;
> > > +
> > > +	ret = v4l2_subdev_call(remote_subdev, pad, get_fmt, NULL, &format);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	video->mbus_code = format.format.code;
> > > +
> > > +	return 0;
> > > +}
> > 
> > I'm not really sure what you're trying to validate here?
> 
> The whole purpose is to retreive video->mbus_code from the subdev, like it's
> done in the sun6i-csi driver. Maybe there is a more appropriate op to do it?

I'm not sure why you need to do that in the link_validate though?

You just need to init the pad format, and then you'll have a
get_fmt/set_fmt for your pads.

> > > +	cdev->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "bus", io_base,
> > > +						 &sun6i_mipi_csi2_regmap_config);
> > > +	if (IS_ERR(cdev->regmap)) {
> > > +		dev_err(&pdev->dev, "failed to init register map\n");
> > > +		return PTR_ERR(cdev->regmap);
> > > +	}
> > 
> > Yeah, so that won't work. regmap expects to have access to those
> > registers when you enable that clock, but that won't happen since the
> > reset line can be disabled. You would be better off using runtime_pm
> > here.
> 
> I don't understand what you mean here or what the problem could be.
> Here we're just initializing regmap and while this is done before the
> registers are available for I/O, I don't see why it would cause any
> issue at this point.

The regmap here is supposed to take care of the resources, except it
only does it for some of the resources here, which kind of breaks the
expectations. And it doesn't allow you to have the reset / clock
sequence properly done.

> The exact same thing is done in the CSI driver.

That's not an argument though, is it? :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]