Message-ID: <fc15b8e5-c7ae-4e0e-b3b7-7ce5b5139455@collabora.com>
Date: Thu, 15 Jan 2026 15:50:59 +0100
From: Michael Riesch <michael.riesch@...labora.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Mehdi Djait <mehdi.djait@...ux.intel.com>,
 Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Hans Verkuil <hverkuil@...nel.org>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Philipp Zabel <p.zabel@...gutronix.de>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Nicolas Dufresne <nicolas.dufresne@...labora.com>,
 Collabora Kernel Team <kernel@...labora.com>, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] media: rockchip: add driver for the rockchip mipi
 csi-2 receiver

Hi Sakari,

On 1/14/26 10:34, Sakari Ailus wrote:
> Hi Michael,
> 
> Thanks for the set.

Thanks for the review :-)

>> [...]
>> diff --git a/drivers/media/platform/rockchip/rkcsi/rkcsi.c b/drivers/media/platform/rockchip/rkcsi/rkcsi.c
>> new file mode 100644
>> index 000000000000..03597f03f08f
>> --- /dev/null
>> +++ b/drivers/media/platform/rockchip/rkcsi/rkcsi.c
>> @@ -0,0 +1,742 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Rockchip MIPI CSI-2 Receiver Driver
>> + *
>> + * Copyright (C) 2019 Rockchip Electronics Co., Ltd.
>> + * Copyright (C) 2025 Michael Riesch <michael.riesch@...fvision.net>
>> + * Copyright (C) 2026 Collabora, Ltd.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/of_platform.h>
> 
> Are the above two needed?

They are not, apparently. I'll remove them.

> 
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
> 
> #include <linux/property.h>

Ack.

>> +#include <linux/reset.h>
>> [...]
>> +
>> +static const struct rkcsi_format *
>> +rkcsi_find_format(struct rkcsi_device *csi_dev, u32 mbus_code)
>> +{
>> +	const struct rkcsi_format *format;
> 
> This can be declared within the loop.

Ack.

> 
>> +
>> +	WARN_ON(csi_dev->formats_num == 0);
>> +
>> +	for (int i = 0; i < csi_dev->formats_num; i++) {
> 
> unsigned int?

Ack.

> 
>> +		format = &csi_dev->formats[i];
>> +		if (format->code == mbus_code)
>> +			return format;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static int rkcsi_start(struct rkcsi_device *csi_dev)
>> +{
>> +	struct media_pad *source_pad =
>> +		&csi_dev->source_sd->entity.pads[csi_dev->source_pad];
>> +	enum v4l2_mbus_type bus_type = csi_dev->vep.bus_type;
>> +	union phy_configure_opts opts;
>> +	s64 link_freq;
>> +	u32 lanes = csi_dev->vep.bus.mipi_csi2.num_data_lanes;
>> +	u32 control = 0;
>> +	int ret;
>> +
>> +	if (lanes < 1 || lanes > 4)
>> +		return -EINVAL;
>> +
>> +	/* set mult and div to 0, thus completely rely on V4L2_CID_LINK_FREQ */
>> +	link_freq = v4l2_get_link_freq(source_pad, 0, 0);
>> +	if (link_freq <= 0)
>> +		return -EINVAL;
> 
> Could you return link_freq here?

Ack.

> 
>> +
>> +	if (bus_type == V4L2_MBUS_CSI2_DPHY) {
>> +		struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
>> +
>> +		ret = phy_mipi_dphy_get_default_config_for_hsclk(link_freq * 2,
>> +								 lanes, cfg);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_set_mode(csi_dev->phy, PHY_MODE_MIPI_DPHY);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_configure(csi_dev->phy, &opts);
>> +		if (ret)
>> +			return ret;
>> +
>> +		control |= SW_CPHY_EN(0);
>> +
>> +	} else if (bus_type == V4L2_MBUS_CSI2_CPHY) {
>> +		/* TODO: implement CPHY configuration */
>> +		return -EOPNOTSUPP;
> 
> How about a switch()?

Ok!

> 
>> +	} else {
>> +		return -EINVAL;
>> +	}
>> +
>> +	control |= SW_DATATYPE_FS(0x00) | SW_DATATYPE_FE(0x01) |
>> +		   SW_DATATYPE_LS(0x02) | SW_DATATYPE_LE(0x03);
>> +
>> +	rkcsi_write(csi_dev, CSI2HOST_N_LANES, lanes - 1);
>> +	rkcsi_write(csi_dev, CSI2HOST_CONTROL, control);
>> +	rkcsi_write(csi_dev, CSI2HOST_CSI2_RESETN, 1);
>> +
>> +	ret = phy_power_on(csi_dev->phy);
> 
> 	return ...;

Ack.

> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static void rkcsi_stop(struct rkcsi_device *csi_dev)
>> +{
>> +	phy_power_off(csi_dev->phy);
>> +
>> +	rkcsi_write(csi_dev, CSI2HOST_CSI2_RESETN, 0);
>> +	rkcsi_write(csi_dev, CSI2HOST_MSK1, ~0);
>> +	rkcsi_write(csi_dev, CSI2HOST_MSK2, ~0);
>> +}
>> +
>> +static const struct media_entity_operations rkcsi_media_ops = {
>> +	.link_validate = v4l2_subdev_link_validate,
>> +};
>> +
>> +static int rkcsi_enum_mbus_code(struct v4l2_subdev *sd,
>> +				struct v4l2_subdev_state *sd_state,
>> +				struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> +	struct rkcsi_device *csi_dev = to_rkcsi(sd);
>> +
>> +	if (code->pad == RKCSI_PAD_SRC) {
>> +		const struct v4l2_mbus_framefmt *sink_fmt;
>> +
>> +		if (code->index)
>> +			return -EINVAL;
>> +
>> +		sink_fmt = v4l2_subdev_state_get_format(sd_state,
>> +							RKCSI_PAD_SINK);
>> +		code->code = sink_fmt->code;
>> +
>> +		return 0;
>> +	} else if (code->pad == RKCSI_PAD_SINK) {
> 
> switch() would be nice here.

Ack.

> 
>> +		if (code->index > csi_dev->formats_num)
>> +			return -EINVAL;
>> +
>> +		code->code = csi_dev->formats[code->index].code;
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int rkcsi_set_fmt(struct v4l2_subdev *sd,
>> +			 struct v4l2_subdev_state *state,
>> +			 struct v4l2_subdev_format *format)
>> +{
>> +	struct rkcsi_device *csi_dev = to_rkcsi(sd);
>> +	const struct rkcsi_format *fmt;
>> +	struct v4l2_mbus_framefmt *sink, *src;
>> +
>> +	/* the format on the source pad always matches the sink pad */
>> +	if (format->pad == RKCSI_PAD_SRC)
>> +		return v4l2_subdev_get_fmt(sd, state, format);
>> +
>> +	sink = v4l2_subdev_state_get_format(state, format->pad, format->stream);
>> +	if (!sink)
>> +		return -EINVAL;
>> +
>> +	fmt = rkcsi_find_format(csi_dev, format->format.code);
>> +	if (!fmt)
>> +		format->format = default_format;
>> +
>> +	*sink = format->format;
>> +
>> +	/* propagate the format to the source pad */
>> +	src = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
>> +							   format->stream);
>> +	if (!src)
>> +		return -EINVAL;
>> +
>> +	*src = *sink;
>> +
>> +	return 0;
>> +}
>> +
>> +static int rkcsi_set_routing(struct v4l2_subdev *sd,
>> +			     struct v4l2_subdev_state *state,
>> +			     enum v4l2_subdev_format_whence which,
>> +			     struct v4l2_subdev_krouting *routing)
>> +{
>> +	int ret;
>> +
>> +	ret = v4l2_subdev_routing_validate(sd, routing,
>> +					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing,
>> +					       &default_format);
> 
> 	return ...;

Ack.

> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int rkcsi_enable_streams(struct v4l2_subdev *sd,
>> +				struct v4l2_subdev_state *state, u32 pad,
>> +				u64 streams_mask)
>> +{
>> +	struct rkcsi_device *csi_dev = to_rkcsi(sd);
>> +	struct v4l2_subdev *remote_sd;
>> +	struct media_pad *sink_pad, *remote_pad;
>> +	struct device *dev = csi_dev->dev;
>> +	u64 mask;
>> +	int ret;
>> +
>> +	sink_pad = &sd->entity.pads[RKCSI_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, RKCSI_PAD_SINK,
>> +					       RKCSI_PAD_SRC, &streams_mask);
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret)
>> +		goto err;
>> +
>> +	ret = rkcsi_start(csi_dev);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable CSI hardware\n");
>> +		goto err_pm_runtime_put;
>> +	}
>> +
>> +	ret = v4l2_subdev_enable_streams(remote_sd, remote_pad->index, mask);
>> +	if (ret)
>> +		goto err_csi_stop;
>> +
>> +	return 0;
>> +
>> +err_csi_stop:
>> +	rkcsi_stop(csi_dev);
>> +err_pm_runtime_put:
>> +	pm_runtime_put_sync(dev);
>> +err:
>> +	return ret;
>> +}
>> +
>> +static int rkcsi_disable_streams(struct v4l2_subdev *sd,
>> +				 struct v4l2_subdev_state *state, u32 pad,
>> +				 u64 streams_mask)
>> +{
>> +	struct rkcsi_device *csi_dev = to_rkcsi(sd);
>> +	struct v4l2_subdev *remote_sd;
>> +	struct media_pad *sink_pad, *remote_pad;
>> +	struct device *dev = csi_dev->dev;
>> +	u64 mask;
>> +	int ret;
>> +
>> +	sink_pad = &sd->entity.pads[RKCSI_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, RKCSI_PAD_SINK,
>> +					       RKCSI_PAD_SRC, &streams_mask);
>> +
>> +	ret = v4l2_subdev_disable_streams(remote_sd, remote_pad->index, mask);
>> +
>> +	rkcsi_stop(csi_dev);
>> +
>> +	pm_runtime_mark_last_busy(dev);
> 
> Redundant, please drop.

Ack.

> 
>> +	pm_runtime_put_autosuspend(dev);
> 
> The driver doesn't enable autosuspend so the autosuspend variant isn't
> useful. Maybe just switch to pm_runtime_put(), as resume probably takes
> very little time?

Ack.

> 
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct v4l2_subdev_pad_ops rkcsi_pad_ops = {
>> +	.enum_mbus_code = rkcsi_enum_mbus_code,
>> +	.get_fmt = v4l2_subdev_get_fmt,
>> +	.set_fmt = rkcsi_set_fmt,
>> +	.set_routing = rkcsi_set_routing,
>> +	.enable_streams = rkcsi_enable_streams,
>> +	.disable_streams = rkcsi_disable_streams,
>> +};
>> +
>> +static const struct v4l2_subdev_ops rkcsi_ops = {
>> +	.pad = &rkcsi_pad_ops,
>> +};
>> +
>> +static int rkcsi_init_state(struct v4l2_subdev *sd,
>> +			    struct v4l2_subdev_state *state)
>> +{
>> +	struct v4l2_subdev_route routes[] = {
>> +		{
>> +			.sink_pad = RKCSI_PAD_SINK,
>> +			.sink_stream = 0,
>> +			.source_pad = RKCSI_PAD_SRC,
>> +			.source_stream = 0,
>> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
>> +		},
>> +	};
>> +	struct v4l2_subdev_krouting routing = {
>> +		.len_routes = ARRAY_SIZE(routes),
>> +		.num_routes = ARRAY_SIZE(routes),
>> +		.routes = routes,
>> +	};
>> +	int ret;
>> +
>> +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
>> +					       &default_format);
> 
> 	return ...;

Ack.

> 
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct v4l2_subdev_internal_ops rkcsi_internal_ops = {
>> +	.init_state = rkcsi_init_state,
>> +};
>> +
>> +static int rkcsi_notifier_bound(struct v4l2_async_notifier *notifier,
>> +				struct v4l2_subdev *sd,
>> +				struct v4l2_async_connection *asd)
>> +{
>> +	struct rkcsi_device *csi_dev =
>> +		container_of(notifier, struct rkcsi_device, notifier);
>> +	int source_pad;
>> +
>> +	source_pad = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode,
>> +						 MEDIA_PAD_FL_SOURCE);
> 
> Could you use v4l2_create_fwnode_links_to_pad()?

Ack.

> 
>> +	if (source_pad < 0) {
>> +		dev_err(csi_dev->dev, "failed to find source pad for %s\n",
>> +			sd->name);
>> +		return source_pad;
>> +	}
>> +
>> +	csi_dev->source_sd = sd;
> 
> I'd just use media_pad_remote_pad_unique() to obtain the remote pad when
> needed.

Ack.

> 
>> +	csi_dev->source_pad = source_pad;
>> +
>> +	return media_create_pad_link(&sd->entity, source_pad,
>> +				     &csi_dev->sd.entity, RKCSI_PAD_SINK,
>> +				     MEDIA_LNK_FL_ENABLED);
>> +}
>> +
>> +static const struct v4l2_async_notifier_operations rkcsi_notifier_ops = {
>> +	.bound = rkcsi_notifier_bound,
>> +};
>> +
>> +static int rkcsi_register_notifier(struct rkcsi_device *csi_dev)
>> +{
>> +	struct v4l2_async_connection *asd;
>> +	struct v4l2_async_notifier *ntf = &csi_dev->notifier;
>> +	struct v4l2_fwnode_endpoint *vep = &csi_dev->vep;
>> +	struct v4l2_subdev *sd = &csi_dev->sd;
>> +	struct device *dev = csi_dev->dev;
>> +	struct fwnode_handle *ep;
>> +	int ret = 0;
> 
> Redundant initialisation.

Ack.

> 
>> +
>> +	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);
>> +	if (!ep)
>> +		return dev_err_probe(dev, -ENODEV, "failed to get endpoint\n");
>> +
>> +	vep->bus_type = V4L2_MBUS_UNKNOWN;
> 
> If you only need the number of lanes and PHY type, I'd store them to the
> driver's own context struct. struct v4l2_fwnode_endpoint isn't tiny.

Ack.

> 
>> +	ret = v4l2_fwnode_endpoint_parse(ep, vep);
>> +	if (ret) {
>> +		ret = dev_err_probe(dev, ret, "failed to parse endpoint\n");
>> +		goto out;
>> +	}
>> +
>> +	if (vep->bus_type != V4L2_MBUS_CSI2_DPHY &&
>> +	    vep->bus_type != V4L2_MBUS_CSI2_CPHY) {
>> +		ret = dev_err_probe(dev, -EINVAL,
>> +				    "invalid bus type of endpoint\n");
>> +		goto out;
>> +	}
>> +
>> +	v4l2_async_subdev_nf_init(ntf, sd);
>> +	ntf->ops = &rkcsi_notifier_ops;
>> +
>> +	asd = v4l2_async_nf_add_fwnode_remote(ntf, ep,
>> +					      struct v4l2_async_connection);
>> +	if (IS_ERR(asd)) {
>> +		ret = PTR_ERR(asd);
>> +		goto err_nf_cleanup;
>> +	}
>> +
>> +	ret = v4l2_async_nf_register(ntf);
>> +	if (ret) {
>> +		ret = dev_err_probe(dev, ret, "failed to register notifier\n");
>> +		goto err_nf_cleanup;
>> +	}
>> +
>> +	goto out;
>> +
>> +err_nf_cleanup:
>> +	v4l2_async_nf_cleanup(ntf);
>> +out:
>> +	fwnode_handle_put(ep);
>> +	return ret;
>> +}
>> +
>> +static int rkcsi_register(struct rkcsi_device *csi_dev)
>> +{
>> +	struct media_pad *pads = csi_dev->pads;
>> +	struct v4l2_subdev *sd = &csi_dev->sd;
>> +	int ret;
>> +
>> +	ret = rkcsi_register_notifier(csi_dev);
>> +	if (ret)
>> +		goto err;
>> +
>> +	v4l2_subdev_init(sd, &rkcsi_ops);
>> +	sd->dev = csi_dev->dev;
>> +	sd->entity.ops = &rkcsi_media_ops;
>> +	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
>> +	sd->internal_ops = &rkcsi_internal_ops;
>> +	sd->owner = THIS_MODULE;
>> +	snprintf(sd->name, sizeof(sd->name), "rockchip-mipi-csi %s",
>> +		 dev_name(csi_dev->dev));
> 
> Do you need to give it an explicit name? (v4l2_i2c_subdev_init() already
> does that.)

I do, actually. I don't call the _i2c_ variant. This is a platform device.

> 
>> +
>> +	pads[RKCSI_PAD_SINK].flags = MEDIA_PAD_FL_SINK |
>> +				     MEDIA_PAD_FL_MUST_CONNECT;
>> +	pads[RKCSI_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE;
>> +	ret = media_entity_pads_init(&sd->entity, RKCSI_PAD_MAX, pads);
>> +	if (ret)
>> +		goto err_notifier_unregister;
>> +
>> +	ret = v4l2_subdev_init_finalize(sd);
>> +	if (ret)
>> +		goto err_entity_cleanup;
>> +
>> +	ret = v4l2_async_register_subdev(sd);
>> +	if (ret) {
>> +		dev_err(sd->dev, "failed to register CSI subdev\n");
>> +		goto err_subdev_cleanup;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_subdev_cleanup:
>> +	v4l2_subdev_cleanup(sd);
>> +err_entity_cleanup:
>> +	media_entity_cleanup(&sd->entity);
>> +err_notifier_unregister:
>> +	v4l2_async_nf_unregister(&csi_dev->notifier);
>> +	v4l2_async_nf_cleanup(&csi_dev->notifier);
>> +err:
>> +	return ret;
>> +}
>> +
>> +static void rkcsi_unregister(struct rkcsi_device *csi_dev)
>> +{
>> +	struct v4l2_subdev *sd = &csi_dev->sd;
>> +
>> +	v4l2_async_unregister_subdev(sd);
>> +	v4l2_subdev_cleanup(sd);
>> +	media_entity_cleanup(&sd->entity);
>> +	v4l2_async_nf_unregister(&csi_dev->notifier);
>> +	v4l2_async_nf_cleanup(&csi_dev->notifier);
>> +}
>> +
>> +static const struct of_device_id rkcsi_of_match[] = {
>> +	{
>> +		.compatible = "rockchip,rk3568-mipi-csi",
>> +	},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, rkcsi_of_match);
>> +
>> +static int rkcsi_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct rkcsi_device *csi_dev;
>> +	int ret;
>> +
>> +	csi_dev = devm_kzalloc(dev, sizeof(*csi_dev), GFP_KERNEL);
>> +	if (!csi_dev)
>> +		return -ENOMEM;
>> +	csi_dev->dev = dev;
>> +	dev_set_drvdata(dev, csi_dev);
>> +
>> +	csi_dev->base_addr = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(csi_dev->base_addr))
>> +		return PTR_ERR(csi_dev->base_addr);
>> +
>> +	ret = devm_clk_bulk_get_all(dev, &csi_dev->clks);
>> +	if (ret != RKCSI_CLKS_MAX)
>> +		return dev_err_probe(dev, -ENODEV, "failed to get clocks\n");
>> +	csi_dev->clks_num = ret;
>> +
>> +	csi_dev->phy = devm_phy_get(dev, NULL);
>> +	if (IS_ERR(csi_dev->phy))
>> +		return dev_err_probe(dev, PTR_ERR(csi_dev->phy),
>> +				     "failed to get MIPI CSI PHY\n");
>> +
>> +	csi_dev->reset = devm_reset_control_get_exclusive(dev, NULL);
>> +	if (IS_ERR(csi_dev->reset))
>> +		return dev_err_probe(dev, PTR_ERR(csi_dev->reset),
>> +				     "failed to get reset\n");
>> +
>> +	csi_dev->formats = formats;
>> +	csi_dev->formats_num = ARRAY_SIZE(formats);
>> +
>> +	pm_runtime_enable(dev);
> 
> You're not resuming the device in probe(). I guess you depend on CONFIG_PM?

I do depend on CONFIG_PM.

I don't think there is need to resume in probe -> resume in
_enable_streams should do the trick, right?

> 
>> +
>> +	ret = phy_init(csi_dev->phy);
>> +	if (ret) {
>> +		ret = dev_err_probe(dev, ret,
>> +				    "failed to initialize MIPI CSI PHY\n");
>> +		goto err_pm_runtime_disable;
>> +	}
>> +
>> +	ret = rkcsi_register(csi_dev);
>> +	if (ret)
>> +		goto err_phy_exit;
>> +
>> +	return 0;
>> +
>> +err_phy_exit:
>> +	phy_exit(csi_dev->phy);
>> +err_pm_runtime_disable:
>> +	pm_runtime_disable(dev);
> 
> A newline would be nice here.

Ack.

> 
>> +	return ret;
>> +}
>> +
>> +static void rkcsi_remove(struct platform_device *pdev)
>> +{
>> +	struct rkcsi_device *csi_dev = platform_get_drvdata(pdev);
>> +	struct device *dev = &pdev->dev;
>> +
>> +	rkcsi_unregister(csi_dev);
>> +	phy_exit(csi_dev->phy);
>> +	pm_runtime_disable(dev);
>> +}
>> +
>> +static int rkcsi_runtime_suspend(struct device *dev)
>> +{
>> +	struct rkcsi_device *csi_dev = dev_get_drvdata(dev);
>> +
>> +	clk_bulk_disable_unprepare(csi_dev->clks_num, csi_dev->clks);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rkcsi_runtime_resume(struct device *dev)
>> +{
>> +	struct rkcsi_device *csi_dev = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	reset_control_assert(csi_dev->reset);
>> +	udelay(5);
>> +	reset_control_deassert(csi_dev->reset);
>> +
>> +	ret = clk_bulk_prepare_enable(csi_dev->clks_num, csi_dev->clks);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable clocks\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct dev_pm_ops rkcsi_pm_ops = {
>> +	.runtime_suspend = rkcsi_runtime_suspend,
>> +	.runtime_resume = rkcsi_runtime_resume,
>> +};
> 
> Could you use DEFINE_RUNTIME_DEV_PM_OPS() (add static in front)?

Ack.

v4 coming up soon!

Best regards,
Michael

> 
>> +
>> +static struct platform_driver rkcsi_drv = {
>> +	.driver = {
>> +		   .name = "rockchip-mipi-csi",
>> +		   .of_match_table = rkcsi_of_match,
>> +		   .pm = &rkcsi_pm_ops,
>> +	},
>> +	.probe = rkcsi_probe,
>> +	.remove = rkcsi_remove,
>> +};
>> +module_platform_driver(rkcsi_drv);
>> +
>> +MODULE_DESCRIPTION("Rockchip MIPI CSI-2 Receiver platform driver");
>> +MODULE_LICENSE("GPL");
>>
> 

Powered by blists - more mailing lists