Hi, On Wed 04 Nov 20, 19:56, Maxime Ripard wrote: > 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? If that's something we can add later (I think it is), I would rather do this in a sub-sequent series to keep the current one lightweight and merged ASAP. It would also require some investigation to find out if the MIPI CSI-2 channel number i goes directly to the CSI controller channel number i or if some remapping can take place. What do you think? > > > > +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. Okay I may have misunderstood how manual/automatic propagation is supposed to work then. The fact that this is done this way in the CSI driver maybe gave me a bad example to follow. I'll revisit this. > > > > + 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. I'm not following any of this. Could you break it down some more? In particular I don't see the relationship between initializing the regmap API (which does take IORESOURCE_MEM in) and reset / clocks. Just initializing the regmap API for I/O mem doesn't mean that the registers are ready for use, does it? We're not requesting any I/O operation from it at this point. > > The exact same thing is done in the CSI driver. > > That's not an argument though, is it? :) That's not the point, the point is rather that if this is wrong, we should fix it in the CSI driver as well. Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com