Re: [PATCH v4 2/3] media: synopsys: add driver for the designware mipi csi-2 receiver

[Michael Riesch <michael.riesch@collabora.com>]

Hi Sakari,

Thanks for your review!

On 1/16/26 11:10, Sakari Ailus wrote:
> Hi Michael,
> 
> Thanks for the update. A few minor comments below...
> 
> On Thu, Jan 15, 2026 at 07:26:08PM +0100, Michael Riesch via B4 Relay wrote:
>> From: Michael Riesch <michael.riesch@collabora.com>
>>
>> The Synopsys DesignWare MIPI CSI-2 Receiver is a CSI-2 bridge with
>> one input port and one output port. It receives the data with the
>> help of an external MIPI PHY (C-PHY or D-PHY) and passes it to e.g.,
>> the Rockchip Video Capture (VICAP) block on recent Rockchip SoCs.
>>
>> Add a V4L2 subdevice driver for this unit.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>
>> Signed-off-by: Michael Riesch <michael.riesch@collabora.com>
>> ---
>>  MAINTAINERS                                        |   1 +
>>  drivers/media/platform/synopsys/Kconfig            |   1 +
>>  drivers/media/platform/synopsys/Makefile           |   1 +
>>  .../media/platform/synopsys/dw-mipi-csi2/Kconfig   |  17 +
>>  .../media/platform/synopsys/dw-mipi-csi2/Makefile  |   2 +
>>  .../platform/synopsys/dw-mipi-csi2/dw-mipi-csi2.c  | 735 +++++++++++++++++++++
> 
> How about dw-mipi-csi2rx (or dw-csi2rx)? There might be a tx chip, too...

Heiko and I had an offlist name bikeshedding discussion, and he already
responded to that concern. Please find the rationale for this there.

> 
>>  [...]
>> +static int dw_mipi_csi2_disable_streams(struct v4l2_subdev *sd,
>> +					struct v4l2_subdev_state *state,
>> +					u32 pad, u64 streams_mask)
>> +{
>> +	struct dw_mipi_csi2_device *csi2 = to_csi2(sd);
>> +	struct v4l2_subdev *remote_sd;
>> +	struct media_pad *sink_pad, *remote_pad;
>> +	struct device *dev = csi2->dev;
>> +	u64 mask;
>> +	int ret;
>> +
>> +	sink_pad = &sd->entity.pads[DW_MIPI_CSI2_PAD_SINK];
>> +	remote_pad = media_pad_remote_pad_first(sink_pad);
>> +	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
>> +
>> +	mask = v4l2_subdev_state_xlate_streams(state, DW_MIPI_CSI2_PAD_SINK,
>> +					       DW_MIPI_CSI2_PAD_SRC,
>> +					       &streams_mask);
>> +
>> +	ret = v4l2_subdev_disable_streams(remote_sd, remote_pad->index, mask);
>> +
>> +	dw_mipi_csi2_stop(csi2);
>> +
>> +	pm_runtime_put_sync(dev);
> 
> How about just pm_runtime_put()?

Ah right, for some reason I used the _sync variant in the error path of
_enable_streams and used it here, too.

But I can change both instances to plain _put.


> [...]
>> +static struct platform_driver dw_mipi_csi2_drv = {
>> +	.driver = {
>> +		   .name = "dw-mipi-csi2",
>> +		   .of_match_table = dw_mipi_csi2_of_match,
>> +		   .pm = &dw_mipi_csi2_pm_ops,
> 
> Just tabs, please.

Oops! Not sure how this could happen. Sorry for that, will fix!

I'll send out v5 soon.

Best regards,
Michael

> 
>> +	},
>> +	.probe = dw_mipi_csi2_probe,
>> +	.remove = dw_mipi_csi2_remove,
>> +};
>> +module_platform_driver(dw_mipi_csi2_drv);
>> +
>> +MODULE_DESCRIPTION("Synopsys DesignWare MIPI CSI-2 Receiver platform driver");
>> +MODULE_LICENSE("GPL");
>>
>