[PATCH 0/6] Remove last users of v4l2-clk and remove v4l2-clk

[PATCH 0/6] Remove last users of v4l2-clk and remove v4l2-clk

[Ezequiel Garcia]

The V4L2 temporary clock helper API is used by just one last capture
driver, pxa-camera, which registers a dummy clock; and then by just
a few sensor drivers, consuming clocks through the v4l2-clk API.

It's possible to convert these few last users, and so remove
the v4l2-clk API, which hasn't been used for a few years.

The sensor drivers are already using the CCF API,
which v4l2-clk API uses as fallback.

To convert the pxa-camera driver, a fixed-rate clock
is registered for the mach-based platforms that still exist,
for mt9m111 to work (the only sensor that PXA currently
registers).

Ezequiel Garcia (6):
  media: mach-pxa: Register the camera sensor fixed-rate clock
  media: pxa_camera: Drop the v4l2-clk clock register
  media: ov9640: Use the generic clock framework
  media: mt9m111: Use the generic clock framework
  media: ov6650: Use the generic clock framework
  media: Remove the legacy v4l2-clk API

 .../driver-api/media/v4l2-clocks.rst          |  31 --
 Documentation/driver-api/media/v4l2-core.rst  |   1 -
 arch/arm/mach-pxa/devices.c                   |   8 +
 drivers/media/i2c/mt9m111.c                   |  17 +-
 drivers/media/i2c/ov6650.c                    |  26 +-
 drivers/media/i2c/ov9640.c                    |  15 +-
 drivers/media/i2c/ov9640.h                    |   4 +-
 drivers/media/platform/pxa_camera.c           |  30 +-
 drivers/media/v4l2-core/Makefile              |   2 +-
 drivers/media/v4l2-core/v4l2-clk.c            | 321 ------------------
 include/media/v4l2-clk.h                      |  73 ----
 11 files changed, 37 insertions(+), 491 deletions(-)
 delete mode 100644 Documentation/driver-api/media/v4l2-clocks.rst
 delete mode 100644 drivers/media/v4l2-core/v4l2-clk.c
 delete mode 100644 include/media/v4l2-clk.h

-- 
2.29.2

[PATCH 1/6] media: mach-pxa: Register the camera sensor fixed-rate clock

[Ezequiel Garcia]

The pxa-camera capture driver currently registers a v4l2-clk
clock, named "mclk", to represent the mt9m111 sensor clock.

Register a proper fixed-rate clock using the generic clock framework,
which will allow to remove the v4l2-clk clock in the pxa-camera
driver in a follow-up commit.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Robert Jarzmik <robert.jarzmik@free.fr>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 arch/arm/mach-pxa/devices.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c
index 524d6093e0c7..09b8495f3fd9 100644
--- a/arch/arm/mach-pxa/devices.c
+++ b/arch/arm/mach-pxa/devices.c
@@ -4,6 +4,7 @@
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/clkdev.h>
+#include <linux/clk-provider.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
 #include <linux/spi/pxa2xx_spi.h>
@@ -634,6 +635,13 @@ static struct platform_device pxa27x_device_camera = {
 
 void __init pxa_set_camera_info(struct pxacamera_platform_data *info)
 {
+	struct clk *mclk;
+
+	/* Register a fixed-rate clock for camera sensors. */
+	mclk = clk_register_fixed_rate(NULL, "pxa_camera_clk", NULL, 0,
+					     info->mclk_10khz * 10000);
+	if (!IS_ERR(mclk))
+		clkdev_create(mclk, "mclk", NULL);
 	pxa_register_device(&pxa27x_device_camera, info);
 }
 
-- 
2.29.2

[PATCH 2/6] media: pxa_camera: Drop the v4l2-clk clock register

[Ezequiel Garcia]

Now that mach-based PXA platforms are registering proper
fixed-rate clocks through the CCF, the v4l2-clk clock
is no longer required.

Drop this clock, so the driver no longer depends on the
legacy v4l2-clk API.

Cc: Robert Jarzmik <robert.jarzmik@free.fr>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/pxa_camera.c | 30 +----------------------------
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index b664ce7558a1..8cfa39108162 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -31,7 +31,6 @@
 #include <linux/dma/pxa-dma.h>
 
 #include <media/v4l2-async.h>
-#include <media/v4l2-clk.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -677,7 +676,6 @@ struct pxa_camera_dev {
 	unsigned long		ciclk;
 	unsigned long		mclk;
 	u32			mclk_divisor;
-	struct v4l2_clk		*mclk_clk;
 	u16			width_flags;	/* max 10 bits */
 
 	struct list_head	capture;
@@ -2030,9 +2028,6 @@ static const struct v4l2_ioctl_ops pxa_camera_ioctl_ops = {
 	.vidioc_unsubscribe_event	= v4l2_event_unsubscribe,
 };
 
-static const struct v4l2_clk_ops pxa_camera_mclk_ops = {
-};
-
 static const struct video_device pxa_camera_videodev_template = {
 	.name = "pxa-camera",
 	.minor = -1,
@@ -2140,11 +2135,6 @@ static void pxa_camera_sensor_unbind(struct v4l2_async_notifier *notifier,
 
 	pxa_camera_destroy_formats(pcdev);
 
-	if (pcdev->mclk_clk) {
-		v4l2_clk_unregister(pcdev->mclk_clk);
-		pcdev->mclk_clk = NULL;
-	}
-
 	video_unregister_device(&pcdev->vdev);
 	pcdev->sensor = NULL;
 
@@ -2278,7 +2268,6 @@ static int pxa_camera_probe(struct platform_device *pdev)
 		.src_maxburst = 8,
 		.direction = DMA_DEV_TO_MEM,
 	};
-	char clk_name[V4L2_CLK_NAME_SIZE];
 	int irq;
 	int err = 0, i;
 
@@ -2417,23 +2406,11 @@ static int pxa_camera_probe(struct platform_device *pdev)
 	if (err)
 		goto exit_notifier_cleanup;
 
-	v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
-			  pcdev->asd.match.i2c.adapter_id,
-			  pcdev->asd.match.i2c.address);
-
-	pcdev->mclk_clk = v4l2_clk_register(&pxa_camera_mclk_ops, clk_name, NULL);
-	if (IS_ERR(pcdev->mclk_clk)) {
-		err = PTR_ERR(pcdev->mclk_clk);
-		goto exit_notifier_cleanup;
-	}
-
 	err = v4l2_async_notifier_register(&pcdev->v4l2_dev, &pcdev->notifier);
 	if (err)
-		goto exit_free_clk;
+		goto exit_notifier_cleanup;
 
 	return 0;
-exit_free_clk:
-	v4l2_clk_unregister(pcdev->mclk_clk);
 exit_notifier_cleanup:
 	v4l2_async_notifier_cleanup(&pcdev->notifier);
 exit_free_v4l2dev:
@@ -2463,11 +2440,6 @@ static int pxa_camera_remove(struct platform_device *pdev)
 	v4l2_async_notifier_unregister(&pcdev->notifier);
 	v4l2_async_notifier_cleanup(&pcdev->notifier);
 
-	if (pcdev->mclk_clk) {
-		v4l2_clk_unregister(pcdev->mclk_clk);
-		pcdev->mclk_clk = NULL;
-	}
-
 	v4l2_device_unregister(&pcdev->v4l2_dev);
 
 	dev_info(&pdev->dev, "PXA Camera driver unloaded\n");
-- 
2.29.2

[PATCH 3/6] media: ov9640: Use the generic clock framework

[Ezequiel Garcia]

Commit 63839882c597 ("media: mach-pxa: palmz72/pcm990: remove soc_camera dependencies")
removed the last in-tree user of this sensor. New users
will be required to use the generic clock framework,
so it's possible to convert the driver to use it.

Convert the driver to use the CCF, and drop the legacy
v4l2-clk API.

Cc: Petr Cvek <petrcvekcz@gmail.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/i2c/ov9640.c | 15 ++++++---------
 drivers/media/i2c/ov9640.h |  4 +++-
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/ov9640.c b/drivers/media/i2c/ov9640.c
index e2a25240fc85..d36b04c49628 100644
--- a/drivers/media/i2c/ov9640.c
+++ b/drivers/media/i2c/ov9640.c
@@ -17,6 +17,7 @@
  * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
  */
 
+#include <linux/clk.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/i2c.h>
@@ -26,7 +27,6 @@
 #include <linux/videodev2.h>
 
 #include <media/v4l2-async.h>
-#include <media/v4l2-clk.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -333,13 +333,13 @@ static int ov9640_s_power(struct v4l2_subdev *sd, int on)
 	if (on) {
 		gpiod_set_value(priv->gpio_power, 1);
 		usleep_range(1000, 2000);
-		ret = v4l2_clk_enable(priv->clk);
+		ret = clk_prepare_enable(priv->clk);
 		usleep_range(1000, 2000);
 		gpiod_set_value(priv->gpio_reset, 0);
 	} else {
 		gpiod_set_value(priv->gpio_reset, 1);
 		usleep_range(1000, 2000);
-		v4l2_clk_disable(priv->clk);
+		clk_disable_unprepare(priv->clk);
 		usleep_range(1000, 2000);
 		gpiod_set_value(priv->gpio_power, 0);
 	}
@@ -719,7 +719,7 @@ static int ov9640_probe(struct i2c_client *client,
 
 	priv->subdev.ctrl_handler = &priv->hdl;
 
-	priv->clk = v4l2_clk_get(&client->dev, "mclk");
+	priv->clk = devm_clk_get(&client->dev, "mclk");
 	if (IS_ERR(priv->clk)) {
 		ret = PTR_ERR(priv->clk);
 		goto ectrlinit;
@@ -727,17 +727,15 @@ static int ov9640_probe(struct i2c_client *client,
 
 	ret = ov9640_video_probe(client);
 	if (ret)
-		goto eprobe;
+		goto ectrlinit;
 
 	priv->subdev.dev = &client->dev;
 	ret = v4l2_async_register_subdev(&priv->subdev);
 	if (ret)
-		goto eprobe;
+		goto ectrlinit;
 
 	return 0;
 
-eprobe:
-	v4l2_clk_put(priv->clk);
 ectrlinit:
 	v4l2_ctrl_handler_free(&priv->hdl);
 
@@ -749,7 +747,6 @@ static int ov9640_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct ov9640_priv *priv = to_ov9640_sensor(sd);
 
-	v4l2_clk_put(priv->clk);
 	v4l2_async_unregister_subdev(&priv->subdev);
 	v4l2_ctrl_handler_free(&priv->hdl);
 
diff --git a/drivers/media/i2c/ov9640.h b/drivers/media/i2c/ov9640.h
index a8ed6992c1a8..a1f9150b2050 100644
--- a/drivers/media/i2c/ov9640.h
+++ b/drivers/media/i2c/ov9640.h
@@ -180,6 +180,8 @@ enum {
 };
 #define	H_SXGA	960
 
+struct clk;
+
 /* Misc. structures */
 struct ov9640_reg_alt {
 	u8	com7;
@@ -196,7 +198,7 @@ struct ov9640_reg {
 struct ov9640_priv {
 	struct v4l2_subdev		subdev;
 	struct v4l2_ctrl_handler	hdl;
-	struct v4l2_clk			*clk;
+	struct clk			*clk;
 	struct gpio_desc		*gpio_power;
 	struct gpio_desc		*gpio_reset;
 
-- 
2.29.2

[PATCH 4/6] media: mt9m111: Use the generic clock framework

[Ezequiel Garcia]

This sensor driver has a proper device-tree binding, and also
all its platform-data based in-tree users have been converted to use
the generic clock framework.

Convert the driver to use the CCF, and drop the legacy
v4l2-clk API.

Cc: Robert Jarzmik <robert.jarzmik@free.fr>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/i2c/mt9m111.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 69697386ffcd..0e11734f75aa 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2008, Robert Jarzmik <robert.jarzmik@free.fr>
  */
+#include <linux/clk.h>
 #include <linux/videodev2.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
@@ -16,7 +17,6 @@
 #include <linux/property.h>
 
 #include <media/v4l2-async.h>
-#include <media/v4l2-clk.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -232,7 +232,7 @@ struct mt9m111 {
 	struct v4l2_ctrl *gain;
 	struct mt9m111_context *ctx;
 	struct v4l2_rect rect;	/* cropping rectangle */
-	struct v4l2_clk *clk;
+	struct clk *clk;
 	unsigned int width;	/* output */
 	unsigned int height;	/* sizes */
 	struct v4l2_fract frame_interval;
@@ -977,7 +977,7 @@ static int mt9m111_power_on(struct mt9m111 *mt9m111)
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
 	int ret;
 
-	ret = v4l2_clk_enable(mt9m111->clk);
+	ret = clk_prepare_enable(mt9m111->clk);
 	if (ret < 0)
 		return ret;
 
@@ -995,7 +995,7 @@ static int mt9m111_power_on(struct mt9m111 *mt9m111)
 	regulator_disable(mt9m111->regulator);
 
 out_clk_disable:
-	v4l2_clk_disable(mt9m111->clk);
+	clk_disable_unprepare(mt9m111->clk);
 
 	dev_err(&client->dev, "Failed to resume the sensor: %d\n", ret);
 
@@ -1006,7 +1006,7 @@ static void mt9m111_power_off(struct mt9m111 *mt9m111)
 {
 	mt9m111_suspend(mt9m111);
 	regulator_disable(mt9m111->regulator);
-	v4l2_clk_disable(mt9m111->clk);
+	clk_disable_unprepare(mt9m111->clk);
 }
 
 static int mt9m111_s_power(struct v4l2_subdev *sd, int on)
@@ -1266,7 +1266,7 @@ static int mt9m111_probe(struct i2c_client *client)
 			return ret;
 	}
 
-	mt9m111->clk = v4l2_clk_get(&client->dev, "mclk");
+	mt9m111->clk = devm_clk_get(&client->dev, "mclk");
 	if (IS_ERR(mt9m111->clk))
 		return PTR_ERR(mt9m111->clk);
 
@@ -1311,7 +1311,7 @@ static int mt9m111_probe(struct i2c_client *client)
 	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
 	if (mt9m111->hdl.error) {
 		ret = mt9m111->hdl.error;
-		goto out_clkput;
+		return ret;
 	}
 
 #ifdef CONFIG_MEDIA_CONTROLLER
@@ -1354,8 +1354,6 @@ static int mt9m111_probe(struct i2c_client *client)
 out_hdlfree:
 #endif
 	v4l2_ctrl_handler_free(&mt9m111->hdl);
-out_clkput:
-	v4l2_clk_put(mt9m111->clk);
 
 	return ret;
 }
@@ -1366,7 +1364,6 @@ static int mt9m111_remove(struct i2c_client *client)
 
 	v4l2_async_unregister_subdev(&mt9m111->subdev);
 	media_entity_cleanup(&mt9m111->subdev.entity);
-	v4l2_clk_put(mt9m111->clk);
 	v4l2_ctrl_handler_free(&mt9m111->hdl);
 
 	return 0;
-- 
2.29.2

[PATCH 5/6] media: ov6650: Use the generic clock framework

[Ezequiel Garcia]

Commit ce548396a433 ("media: mach-omap1: board-ams-delta.c: remove soc_camera dependencies")
removed the last in-tree user of this sensor. New users
will be required to use the generic clock framework,
so it's possible to convert the driver to use it.

Convert the driver to use the CCF, and drop the legacy
v4l2-clk API.

Cc: Janusz Krzysztofik <jmkrzyszt@gmail.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/i2c/ov6650.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index d73f9f540932..0f8242054603 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -22,13 +22,13 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/v4l2-mediabus.h>
 #include <linux/module.h>
 
-#include <media/v4l2-clk.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 
@@ -194,7 +194,7 @@ struct ov6650 {
 		struct v4l2_ctrl *blue;
 		struct v4l2_ctrl *red;
 	};
-	struct v4l2_clk		*clk;
+	struct clk		*clk;
 	bool			half_scale;	/* scale down output by 2 */
 	struct v4l2_rect	rect;		/* sensor cropping window */
 	struct v4l2_fract	tpf;		/* as requested with s_frame_interval */
@@ -459,9 +459,9 @@ static int ov6650_s_power(struct v4l2_subdev *sd, int on)
 	int ret = 0;
 
 	if (on)
-		ret = v4l2_clk_enable(priv->clk);
+		ret = clk_prepare_enable(priv->clk);
 	else
-		v4l2_clk_disable(priv->clk);
+		clk_disable_unprepare(priv->clk);
 
 	return ret;
 }
@@ -821,14 +821,14 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
 	u8 pidh, pidl, midh, midl;
 	int i, ret = 0;
 
-	priv->clk = v4l2_clk_get(&client->dev, NULL);
+	priv->clk = devm_clk_get(&client->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		ret = PTR_ERR(priv->clk);
-		dev_err(&client->dev, "v4l2_clk request err: %d\n", ret);
+		dev_err(&client->dev, "clk request err: %d\n", ret);
 		return ret;
 	}
 
-	rate = v4l2_clk_get_rate(priv->clk);
+	rate = clk_get_rate(priv->clk);
 	for (i = 0; rate && i < ARRAY_SIZE(ov6650_xclk); i++) {
 		if (rate != ov6650_xclk[i].rate)
 			continue;
@@ -839,8 +839,8 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
 		break;
 	}
 	for (i = 0; !xclk && i < ARRAY_SIZE(ov6650_xclk); i++) {
-		ret = v4l2_clk_set_rate(priv->clk, ov6650_xclk[i].rate);
-		if (ret || v4l2_clk_get_rate(priv->clk) != ov6650_xclk[i].rate)
+		ret = clk_set_rate(priv->clk, ov6650_xclk[i].rate);
+		if (ret || clk_get_rate(priv->clk) != ov6650_xclk[i].rate)
 			continue;
 
 		xclk = &ov6650_xclk[i];
@@ -852,12 +852,12 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
 		dev_err(&client->dev, "unable to get supported clock rate\n");
 		if (!ret)
 			ret = -EINVAL;
-		goto eclkput;
+		return ret;
 	}
 
 	ret = ov6650_s_power(sd, 1);
 	if (ret < 0)
-		goto eclkput;
+		return ret;
 
 	msleep(20);
 
@@ -901,9 +901,6 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
 	ov6650_s_power(sd, 0);
 	if (!ret)
 		return 0;
-eclkput:
-	v4l2_clk_put(priv->clk);
-
 	return ret;
 }
 
@@ -1089,7 +1086,6 @@ static int ov6650_remove(struct i2c_client *client)
 {
 	struct ov6650 *priv = to_ov6650(client);
 
-	v4l2_clk_put(priv->clk);
 	v4l2_async_unregister_subdev(&priv->subdev);
 	v4l2_ctrl_handler_free(&priv->hdl);
 	return 0;
-- 
2.29.2

[PATCH 6/6] media: Remove the legacy v4l2-clk API

[Ezequiel Garcia]

The V4L2 temporary clock helper API, was introduced
in late 2012 and, as mentioned in the documentation,
meant to be replaced by the generic clock API,
once the generic clock framework became available
on all relevant architectures.

The generic clock API is a well-established API (since a few
years now). The last few media capture drivers and sensors
using v4l2-clk have been converted to the generic clock framework.

We can now remove the v4l2-clk API.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 .../driver-api/media/v4l2-clocks.rst          |  31 --
 Documentation/driver-api/media/v4l2-core.rst  |   1 -
 drivers/media/v4l2-core/Makefile              |   2 +-
 drivers/media/v4l2-core/v4l2-clk.c            | 321 ------------------
 include/media/v4l2-clk.h                      |  73 ----
 5 files changed, 1 insertion(+), 427 deletions(-)
 delete mode 100644 Documentation/driver-api/media/v4l2-clocks.rst
 delete mode 100644 drivers/media/v4l2-core/v4l2-clk.c
 delete mode 100644 include/media/v4l2-clk.h

diff --git a/Documentation/driver-api/media/v4l2-clocks.rst b/Documentation/driver-api/media/v4l2-clocks.rst
deleted file mode 100644
index 5c22eecab7ba..000000000000
--- a/Documentation/driver-api/media/v4l2-clocks.rst
+++ /dev/null
@@ -1,31 +0,0 @@
-.. SPDX-License-Identifier: GPL-2.0
-
-V4L2 clocks
------------
-
-.. attention::
-
-	This is a temporary API and it shall be replaced by the generic
-	clock API, when the latter becomes widely available.
-
-Many subdevices, like camera sensors, TV decoders and encoders, need a clock
-signal to be supplied by the system. Often this clock is supplied by the
-respective bridge device. The Linux kernel provides a Common Clock Framework for
-this purpose. However, it is not (yet) available on all architectures. Besides,
-the nature of the multi-functional (clock, data + synchronisation, I2C control)
-connection of subdevices to the system might impose special requirements on the
-clock API usage. E.g. V4L2 has to support clock provider driver unregistration
-while a subdevice driver is holding a reference to the clock. For these reasons
-a V4L2 clock helper API has been developed and is provided to bridge and
-subdevice drivers.
-
-The API consists of two parts: two functions to register and unregister a V4L2
-clock source: v4l2_clk_register() and v4l2_clk_unregister() and calls to control
-a clock object, similar to the respective generic clock API calls:
-v4l2_clk_get(), v4l2_clk_put(), v4l2_clk_enable(), v4l2_clk_disable(),
-v4l2_clk_get_rate(), and v4l2_clk_set_rate(). Clock suppliers have to provide
-clock operations that will be called when clock users invoke respective API
-methods.
-
-It is expected that once the CCF becomes available on all relevant
-architectures this API will be removed.
diff --git a/Documentation/driver-api/media/v4l2-core.rst b/Documentation/driver-api/media/v4l2-core.rst
index 0dcad7a23141..1a8c4a5f256b 100644
--- a/Documentation/driver-api/media/v4l2-core.rst
+++ b/Documentation/driver-api/media/v4l2-core.rst
@@ -15,7 +15,6 @@ Video4Linux devices
     v4l2-controls
     v4l2-videobuf
     v4l2-videobuf2
-    v4l2-clocks
     v4l2-dv-timings
     v4l2-flash-led-class
     v4l2-mc
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 2ef0c7c958a2..e4cd589b99a5 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -6,7 +6,7 @@
 tuner-objs	:=	tuner-core.o
 
 videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
-			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \
+			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o \
 			v4l2-async.o v4l2-common.o
 videodev-$(CONFIG_COMPAT) += v4l2-compat-ioctl32.o
 videodev-$(CONFIG_TRACEPOINTS) += v4l2-trace.o
diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
deleted file mode 100644
index 91274eee6977..000000000000
--- a/drivers/media/v4l2-core/v4l2-clk.c
+++ /dev/null
@@ -1,321 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * V4L2 clock service
- *
- * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
- */
-
-#include <linux/atomic.h>
-#include <linux/clk.h>
-#include <linux/device.h>
-#include <linux/errno.h>
-#include <linux/list.h>
-#include <linux/module.h>
-#include <linux/mutex.h>
-#include <linux/of.h>
-#include <linux/slab.h>
-#include <linux/string.h>
-
-#include <media/v4l2-clk.h>
-#include <media/v4l2-subdev.h>
-
-static DEFINE_MUTEX(clk_lock);
-static LIST_HEAD(clk_list);
-
-static struct v4l2_clk *v4l2_clk_find(const char *dev_id)
-{
-	struct v4l2_clk *clk;
-
-	list_for_each_entry(clk, &clk_list, list)
-		if (!strcmp(dev_id, clk->dev_id))
-			return clk;
-
-	return ERR_PTR(-ENODEV);
-}
-
-struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
-{
-	struct v4l2_clk *clk;
-	struct clk *ccf_clk = clk_get(dev, id);
-	char clk_name[V4L2_CLK_NAME_SIZE];
-
-	if (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
-		return ERR_PTR(-EPROBE_DEFER);
-
-	if (!IS_ERR_OR_NULL(ccf_clk)) {
-		clk = kzalloc(sizeof(*clk), GFP_KERNEL);
-		if (!clk) {
-			clk_put(ccf_clk);
-			return ERR_PTR(-ENOMEM);
-		}
-		clk->clk = ccf_clk;
-
-		return clk;
-	}
-
-	mutex_lock(&clk_lock);
-	clk = v4l2_clk_find(dev_name(dev));
-
-	/* if dev_name is not found, try use the OF name to find again  */
-	if (PTR_ERR(clk) == -ENODEV && dev->of_node) {
-		v4l2_clk_name_of(clk_name, sizeof(clk_name), dev->of_node);
-		clk = v4l2_clk_find(clk_name);
-	}
-
-	if (!IS_ERR(clk))
-		atomic_inc(&clk->use_count);
-	mutex_unlock(&clk_lock);
-
-	return clk;
-}
-EXPORT_SYMBOL(v4l2_clk_get);
-
-void v4l2_clk_put(struct v4l2_clk *clk)
-{
-	struct v4l2_clk *tmp;
-
-	if (IS_ERR(clk))
-		return;
-
-	if (clk->clk) {
-		clk_put(clk->clk);
-		kfree(clk);
-		return;
-	}
-
-	mutex_lock(&clk_lock);
-
-	list_for_each_entry(tmp, &clk_list, list)
-		if (tmp == clk)
-			atomic_dec(&clk->use_count);
-
-	mutex_unlock(&clk_lock);
-}
-EXPORT_SYMBOL(v4l2_clk_put);
-
-static int v4l2_clk_lock_driver(struct v4l2_clk *clk)
-{
-	struct v4l2_clk *tmp;
-	int ret = -ENODEV;
-
-	mutex_lock(&clk_lock);
-
-	list_for_each_entry(tmp, &clk_list, list)
-		if (tmp == clk) {
-			ret = !try_module_get(clk->ops->owner);
-			if (ret)
-				ret = -EFAULT;
-			break;
-		}
-
-	mutex_unlock(&clk_lock);
-
-	return ret;
-}
-
-static void v4l2_clk_unlock_driver(struct v4l2_clk *clk)
-{
-	module_put(clk->ops->owner);
-}
-
-int v4l2_clk_enable(struct v4l2_clk *clk)
-{
-	int ret;
-
-	if (clk->clk)
-		return clk_prepare_enable(clk->clk);
-
-	ret = v4l2_clk_lock_driver(clk);
-	if (ret < 0)
-		return ret;
-
-	mutex_lock(&clk->lock);
-
-	if (++clk->enable == 1 && clk->ops->enable) {
-		ret = clk->ops->enable(clk);
-		if (ret < 0)
-			clk->enable--;
-	}
-
-	mutex_unlock(&clk->lock);
-
-	return ret;
-}
-EXPORT_SYMBOL(v4l2_clk_enable);
-
-/*
- * You might Oops if you try to disabled a disabled clock, because then the
- * driver isn't locked and could have been unloaded by now, so, don't do that
- */
-void v4l2_clk_disable(struct v4l2_clk *clk)
-{
-	int enable;
-
-	if (clk->clk)
-		return clk_disable_unprepare(clk->clk);
-
-	mutex_lock(&clk->lock);
-
-	enable = --clk->enable;
-	if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__,
-		 clk->dev_id))
-		clk->enable++;
-	else if (!enable && clk->ops->disable)
-		clk->ops->disable(clk);
-
-	mutex_unlock(&clk->lock);
-
-	v4l2_clk_unlock_driver(clk);
-}
-EXPORT_SYMBOL(v4l2_clk_disable);
-
-unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
-{
-	int ret;
-
-	if (clk->clk)
-		return clk_get_rate(clk->clk);
-
-	ret = v4l2_clk_lock_driver(clk);
-	if (ret < 0)
-		return ret;
-
-	mutex_lock(&clk->lock);
-	if (!clk->ops->get_rate)
-		ret = -ENOSYS;
-	else
-		ret = clk->ops->get_rate(clk);
-	mutex_unlock(&clk->lock);
-
-	v4l2_clk_unlock_driver(clk);
-
-	return ret;
-}
-EXPORT_SYMBOL(v4l2_clk_get_rate);
-
-int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
-{
-	int ret;
-
-	if (clk->clk) {
-		long r = clk_round_rate(clk->clk, rate);
-		if (r < 0)
-			return r;
-		return clk_set_rate(clk->clk, r);
-	}
-
-	ret = v4l2_clk_lock_driver(clk);
-
-	if (ret < 0)
-		return ret;
-
-	mutex_lock(&clk->lock);
-	if (!clk->ops->set_rate)
-		ret = -ENOSYS;
-	else
-		ret = clk->ops->set_rate(clk, rate);
-	mutex_unlock(&clk->lock);
-
-	v4l2_clk_unlock_driver(clk);
-
-	return ret;
-}
-EXPORT_SYMBOL(v4l2_clk_set_rate);
-
-struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
-				   const char *dev_id,
-				   void *priv)
-{
-	struct v4l2_clk *clk;
-	int ret;
-
-	if (!ops || !dev_id)
-		return ERR_PTR(-EINVAL);
-
-	clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
-	if (!clk)
-		return ERR_PTR(-ENOMEM);
-
-	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
-	if (!clk->dev_id) {
-		ret = -ENOMEM;
-		goto ealloc;
-	}
-	clk->ops = ops;
-	clk->priv = priv;
-	atomic_set(&clk->use_count, 0);
-	mutex_init(&clk->lock);
-
-	mutex_lock(&clk_lock);
-	if (!IS_ERR(v4l2_clk_find(dev_id))) {
-		mutex_unlock(&clk_lock);
-		ret = -EEXIST;
-		goto eexist;
-	}
-	list_add_tail(&clk->list, &clk_list);
-	mutex_unlock(&clk_lock);
-
-	return clk;
-
-eexist:
-ealloc:
-	kfree(clk->dev_id);
-	kfree(clk);
-	return ERR_PTR(ret);
-}
-EXPORT_SYMBOL(v4l2_clk_register);
-
-void v4l2_clk_unregister(struct v4l2_clk *clk)
-{
-	if (WARN(atomic_read(&clk->use_count),
-		 "%s(): Refusing to unregister ref-counted %s clock!\n",
-		 __func__, clk->dev_id))
-		return;
-
-	mutex_lock(&clk_lock);
-	list_del(&clk->list);
-	mutex_unlock(&clk_lock);
-
-	kfree(clk->dev_id);
-	kfree(clk);
-}
-EXPORT_SYMBOL(v4l2_clk_unregister);
-
-struct v4l2_clk_fixed {
-	unsigned long rate;
-	struct v4l2_clk_ops ops;
-};
-
-static unsigned long fixed_get_rate(struct v4l2_clk *clk)
-{
-	struct v4l2_clk_fixed *priv = clk->priv;
-	return priv->rate;
-}
-
-struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
-				unsigned long rate, struct module *owner)
-{
-	struct v4l2_clk *clk;
-	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-
-	if (!priv)
-		return ERR_PTR(-ENOMEM);
-
-	priv->rate = rate;
-	priv->ops.get_rate = fixed_get_rate;
-	priv->ops.owner = owner;
-
-	clk = v4l2_clk_register(&priv->ops, dev_id, priv);
-	if (IS_ERR(clk))
-		kfree(priv);
-
-	return clk;
-}
-EXPORT_SYMBOL(__v4l2_clk_register_fixed);
-
-void v4l2_clk_unregister_fixed(struct v4l2_clk *clk)
-{
-	kfree(clk->priv);
-	v4l2_clk_unregister(clk);
-}
-EXPORT_SYMBOL(v4l2_clk_unregister_fixed);
diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
deleted file mode 100644
index d9d21a43a834..000000000000
--- a/include/media/v4l2-clk.h
+++ /dev/null
@@ -1,73 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * V4L2 clock service
- *
- * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
- *
- * ATTENTION: This is a temporary API and it shall be replaced by the generic
- * clock API, when the latter becomes widely available.
- */
-
-#ifndef MEDIA_V4L2_CLK_H
-#define MEDIA_V4L2_CLK_H
-
-#include <linux/atomic.h>
-#include <linux/export.h>
-#include <linux/list.h>
-#include <linux/mutex.h>
-
-struct module;
-struct device;
-
-struct clk;
-struct v4l2_clk {
-	struct list_head list;
-	const struct v4l2_clk_ops *ops;
-	const char *dev_id;
-	int enable;
-	struct mutex lock; /* Protect the enable count */
-	atomic_t use_count;
-	struct clk *clk;
-	void *priv;
-};
-
-struct v4l2_clk_ops {
-	struct module	*owner;
-	int		(*enable)(struct v4l2_clk *clk);
-	void		(*disable)(struct v4l2_clk *clk);
-	unsigned long	(*get_rate)(struct v4l2_clk *clk);
-	int		(*set_rate)(struct v4l2_clk *clk, unsigned long);
-};
-
-struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
-				   const char *dev_name,
-				   void *priv);
-void v4l2_clk_unregister(struct v4l2_clk *clk);
-struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id);
-void v4l2_clk_put(struct v4l2_clk *clk);
-int v4l2_clk_enable(struct v4l2_clk *clk);
-void v4l2_clk_disable(struct v4l2_clk *clk);
-unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk);
-int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate);
-
-struct module;
-
-struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
-			unsigned long rate, struct module *owner);
-void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
-
-static inline struct v4l2_clk *v4l2_clk_register_fixed(const char *dev_id,
-							unsigned long rate)
-{
-	return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE);
-}
-
-#define V4L2_CLK_NAME_SIZE 64
-
-#define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, \
-			  "%d-%04x", adap, client)
-
-#define v4l2_clk_name_of(name, size, node) snprintf(name, size, \
-			  "of-%pOF", node)
-
-#endif
-- 
2.29.2

Re: [PATCH 0/6] Remove last users of v4l2-clk and remove v4l2-clk

[Ezequiel Garcia]

Robert's mailbox is bouncing, so let's add Daniel
and Haojian.

Could you guys review the PXA changes?

Thanks,
Ezequiel

On Mon, 2021-01-04 at 13:57 -0300, Ezequiel Garcia wrote:
> The V4L2 temporary clock helper API is used by just one last capture
> driver, pxa-camera, which registers a dummy clock; and then by just
> a few sensor drivers, consuming clocks through the v4l2-clk API.
> 
> It's possible to convert these few last users, and so remove
> the v4l2-clk API, which hasn't been used for a few years.
> 
> The sensor drivers are already using the CCF API,
> which v4l2-clk API uses as fallback.
> 
> To convert the pxa-camera driver, a fixed-rate clock
> is registered for the mach-based platforms that still exist,
> for mt9m111 to work (the only sensor that PXA currently
> registers).
> 
> Ezequiel Garcia (6):
>   media: mach-pxa: Register the camera sensor fixed-rate clock
>   media: pxa_camera: Drop the v4l2-clk clock register
>   media: ov9640: Use the generic clock framework
>   media: mt9m111: Use the generic clock framework
>   media: ov6650: Use the generic clock framework
>   media: Remove the legacy v4l2-clk API
> 
>  .../driver-api/media/v4l2-clocks.rst          |  31 --
>  Documentation/driver-api/media/v4l2-core.rst  |   1 -
>  arch/arm/mach-pxa/devices.c                   |   8 +
>  drivers/media/i2c/mt9m111.c                   |  17 +-
>  drivers/media/i2c/ov6650.c                    |  26 +-
>  drivers/media/i2c/ov9640.c                    |  15 +-
>  drivers/media/i2c/ov9640.h                    |   4 +-
>  drivers/media/platform/pxa_camera.c           |  30 +-
>  drivers/media/v4l2-core/Makefile              |   2 +-
>  drivers/media/v4l2-core/v4l2-clk.c            | 321 ------------------
>  include/media/v4l2-clk.h                      |  73 ----
>  11 files changed, 37 insertions(+), 491 deletions(-)
>  delete mode 100644 Documentation/driver-api/media/v4l2-clocks.rst
>  delete mode 100644 drivers/media/v4l2-core/v4l2-clk.c
>  delete mode 100644 include/media/v4l2-clk.h
> 
> -- 
> 2.29.2
> 
> 

Re: [PATCH 0/6] Remove last users of v4l2-clk and remove v4l2-clk

[Petr Cvek]

I don't have a working magician setup at the moment, so I can only test the compilation (which works).

Petr

Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a):
> The V4L2 temporary clock helper API is used by just one last capture
> driver, pxa-camera, which registers a dummy clock; and then by just
> a few sensor drivers, consuming clocks through the v4l2-clk API.
> 
> It's possible to convert these few last users, and so remove
> the v4l2-clk API, which hasn't been used for a few years.
> 
> The sensor drivers are already using the CCF API,
> which v4l2-clk API uses as fallback.
> 
> To convert the pxa-camera driver, a fixed-rate clock
> is registered for the mach-based platforms that still exist,
> for mt9m111 to work (the only sensor that PXA currently
> registers).
> 
> Ezequiel Garcia (6):
>   media: mach-pxa: Register the camera sensor fixed-rate clock
>   media: pxa_camera: Drop the v4l2-clk clock register
>   media: ov9640: Use the generic clock framework
>   media: mt9m111: Use the generic clock framework
>   media: ov6650: Use the generic clock framework
>   media: Remove the legacy v4l2-clk API
> 
>  .../driver-api/media/v4l2-clocks.rst          |  31 --
>  Documentation/driver-api/media/v4l2-core.rst  |   1 -
>  arch/arm/mach-pxa/devices.c                   |   8 +
>  drivers/media/i2c/mt9m111.c                   |  17 +-
>  drivers/media/i2c/ov6650.c                    |  26 +-
>  drivers/media/i2c/ov9640.c                    |  15 +-
>  drivers/media/i2c/ov9640.h                    |   4 +-
>  drivers/media/platform/pxa_camera.c           |  30 +-
>  drivers/media/v4l2-core/Makefile              |   2 +-
>  drivers/media/v4l2-core/v4l2-clk.c            | 321 ------------------
>  include/media/v4l2-clk.h                      |  73 ----
>  11 files changed, 37 insertions(+), 491 deletions(-)
>  delete mode 100644 Documentation/driver-api/media/v4l2-clocks.rst
>  delete mode 100644 drivers/media/v4l2-core/v4l2-clk.c
>  delete mode 100644 include/media/v4l2-clk.h
> 

Re: [PATCH 3/6] media: ov9640: Use the generic clock framework

[Petr Cvek]

> diff --git a/drivers/media/i2c/ov9640.h b/drivers/media/i2c/ov9640.h
> index a8ed6992c1a8..a1f9150b2050 100644
> --- a/drivers/media/i2c/ov9640.h
> +++ b/drivers/media/i2c/ov9640.h
> @@ -180,6 +180,8 @@ enum {
>  };
>  #define	H_SXGA	960
>  
> +struct clk;
> +

Seems to be unnecessary as struct clk will be included from ov9640.c (the same way struct v4l2_clk was).

The rest seems fine by me.

Petr

Re: [PATCH 1/6] media: mach-pxa: Register the camera sensor fixed-rate clock

[Petr Cvek]

Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a):
> The pxa-camera capture driver currently registers a v4l2-clk
> clock, named "mclk", to represent the mt9m111 sensor clock.
> 
> Register a proper fixed-rate clock using the generic clock framework,
> which will allow to remove the v4l2-clk clock in the pxa-camera
> driver in a follow-up commit.
> 

BTW the mclk output to a sensor is actually a variable rate, divided from lcdclk (which can be changed too). PXA camera driver  is using variable pcdev->mclk_divisor to generate the mclk from lcdclk. 

The rate change is done in pxa_camera_activate():

https://elixir.bootlin.com/linux/v5.11-rc2/source/drivers/media/platform/pxa_camera.c#L1136

	__raw_writel(pcdev->mclk_divisor | cicr4, pcdev->base + CICR4);

Would it be possible to register a correct clock type with possibility to change the divisor by the standard way?

Petr


> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Robert Jarzmik <robert.jarzmik@free.fr>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  arch/arm/mach-pxa/devices.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c
> index 524d6093e0c7..09b8495f3fd9 100644
> --- a/arch/arm/mach-pxa/devices.c
> +++ b/arch/arm/mach-pxa/devices.c
> @@ -4,6 +4,7 @@
>  #include <linux/init.h>
>  #include <linux/platform_device.h>
>  #include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/dmaengine.h>
>  #include <linux/spi/pxa2xx_spi.h>
> @@ -634,6 +635,13 @@ static struct platform_device pxa27x_device_camera = {
>  
>  void __init pxa_set_camera_info(struct pxacamera_platform_data *info)
>  {
> +	struct clk *mclk;
> +
> +	/* Register a fixed-rate clock for camera sensors. */
> +	mclk = clk_register_fixed_rate(NULL, "pxa_camera_clk", NULL, 0,
> +					     info->mclk_10khz * 10000);
> +	if (!IS_ERR(mclk))
> +		clkdev_create(mclk, "mclk", NULL);
>  	pxa_register_device(&pxa27x_device_camera, info);
>  }
>  
> 

Re: [PATCH 3/6] media: ov9640: Use the generic clock framework

[Ezequiel Garcia]

On Tue, 2021-01-05 at 17:18 +0100, Petr Cvek wrote:
> > diff --git a/drivers/media/i2c/ov9640.h b/drivers/media/i2c/ov9640.h
> > index a8ed6992c1a8..a1f9150b2050 100644
> > --- a/drivers/media/i2c/ov9640.h
> > +++ b/drivers/media/i2c/ov9640.h
> > @@ -180,6 +180,8 @@ enum {
> >  };
> >  #define        H_SXGA  960
> >  
> > +struct clk;
> > +
> 
> Seems to be unnecessary as struct clk will be included from ov9640.c (the same way struct v4l2_clk was).
> 
> The rest seems fine by me.
> 

Hm, I'm now wondering why I added that.

Guess it can be dropped, yes.

Thanks,
Ezequiel

Re: [PATCH 0/6] Remove last users of v4l2-clk and remove v4l2-clk

[Ezequiel Garcia]

Hi Petr,

On Tue, 2021-01-05 at 17:08 +0100, Petr Cvek wrote:
> I don't have a working magician setup at the moment, so I can only test the compilation (which works).
> 

Thanks for the testing! Does that mean I can take your Tested-by ?

> Petr
> 
> Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a):
> > The V4L2 temporary clock helper API is used by just one last capture
> > driver, pxa-camera, which registers a dummy clock; and then by just
> > a few sensor drivers, consuming clocks through the v4l2-clk API.
> > 
> > It's possible to convert these few last users, and so remove
> > the v4l2-clk API, which hasn't been used for a few years.
> > 
> > The sensor drivers are already using the CCF API,
> > which v4l2-clk API uses as fallback.
> > 
> > To convert the pxa-camera driver, a fixed-rate clock
> > is registered for the mach-based platforms that still exist,
> > for mt9m111 to work (the only sensor that PXA currently
> > registers).
> > 
> > Ezequiel Garcia (6):
> >   media: mach-pxa: Register the camera sensor fixed-rate clock
> >   media: pxa_camera: Drop the v4l2-clk clock register
> >   media: ov9640: Use the generic clock framework
> >   media: mt9m111: Use the generic clock framework
> >   media: ov6650: Use the generic clock framework
> >   media: Remove the legacy v4l2-clk API
> > 
> >  .../driver-api/media/v4l2-clocks.rst          |  31 --
> >  Documentation/driver-api/media/v4l2-core.rst  |   1 -
> >  arch/arm/mach-pxa/devices.c                   |   8 +
> >  drivers/media/i2c/mt9m111.c                   |  17 +-
> >  drivers/media/i2c/ov6650.c                    |  26 +-
> >  drivers/media/i2c/ov9640.c                    |  15 +-
> >  drivers/media/i2c/ov9640.h                    |   4 +-
> >  drivers/media/platform/pxa_camera.c           |  30 +-
> >  drivers/media/v4l2-core/Makefile              |   2 +-
> >  drivers/media/v4l2-core/v4l2-clk.c            | 321 ------------------
> >  include/media/v4l2-clk.h                      |  73 ----
> >  11 files changed, 37 insertions(+), 491 deletions(-)
> >  delete mode 100644 Documentation/driver-api/media/v4l2-clocks.rst
> >  delete mode 100644 drivers/media/v4l2-core/v4l2-clk.c
> >  delete mode 100644 include/media/v4l2-clk.h
> > 

Re: [PATCH 1/6] media: mach-pxa: Register the camera sensor fixed-rate clock

[Ezequiel Garcia]

Hi Petr,

Thanks a lot for reviewing and testing the series.

On Tue, 2021-01-05 at 17:41 +0100, Petr Cvek wrote:
> 
> Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a):
> > The pxa-camera capture driver currently registers a v4l2-clk
> > clock, named "mclk", to represent the mt9m111 sensor clock.
> > 
> > Register a proper fixed-rate clock using the generic clock framework,
> > which will allow to remove the v4l2-clk clock in the pxa-camera
> > driver in a follow-up commit.
> > 
> 
> BTW the mclk output to a sensor is actually a variable rate, divided from lcdclk (which can be changed too). PXA camera driver  is using variable
> pcdev->mclk_divisor to generate the mclk from lcdclk. 
> 

Hm, now that I look at this, I see the pxa-camera driver
is requiring a clock:

        pcdev->clk = devm_clk_get(&pdev->dev, NULL);                             
        if (IS_ERR(pcdev->clk))                                                  
                return PTR_ERR(pcdev->clk);   

Where is this clock registered in the non-devicetree case?

> The rate change is done in pxa_camera_activate():
> 
> https://elixir.bootlin.com/linux/v5.11-rc2/source/drivers/media/platform/pxa_camera.c#L1136
> 
>         __raw_writel(pcdev->mclk_divisor | cicr4, pcdev->base + CICR4);
> 
> Would it be possible to register a correct clock type with possibility to change the divisor by the standard way?
> 

Right, so you mean the pxa-camera driver is the one providing the clock for the sensors?

In that case, I guess the pxa-camera driver should be the one registering
a CCF clock. Other drivers are doing this, through clk_register for instance.

However, for the sake of this series, which is meant to get rid
of the v4l2-clk API, I would say it's fine to just register a fixed-rate.

This is similar to what v4l2_clk_register was doing, which was registering
a dummy clock.

Having said that, as I mentioned above, I'm wondering if the mach-pxa
boards are really working, given I'm not seeing the clock for pxa-camera.

Maybe the best way forward is to just accept that pxa-camera
is only supported for the device tree platforms, and therefore drop the
support from mach-pxa/ boards.

Thanks,
Ezequiel
 

> Petr
> 
> 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Robert Jarzmik <robert.jarzmik@free.fr>
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  arch/arm/mach-pxa/devices.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c
> > index 524d6093e0c7..09b8495f3fd9 100644
> > --- a/arch/arm/mach-pxa/devices.c
> > +++ b/arch/arm/mach-pxa/devices.c
> > @@ -4,6 +4,7 @@
> >  #include <linux/init.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/clkdev.h>
> > +#include <linux/clk-provider.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/dmaengine.h>
> >  #include <linux/spi/pxa2xx_spi.h>
> > @@ -634,6 +635,13 @@ static struct platform_device pxa27x_device_camera = {
> >  
> >  void __init pxa_set_camera_info(struct pxacamera_platform_data *info)
> >  {
> > +       struct clk *mclk;
> > +
> > +       /* Register a fixed-rate clock for camera sensors. */
> > +       mclk = clk_register_fixed_rate(NULL, "pxa_camera_clk", NULL, 0,
> > +                                            info->mclk_10khz * 10000);
> > +       if (!IS_ERR(mclk))
> > +               clkdev_create(mclk, "mclk", NULL);
> >         pxa_register_device(&pxa27x_device_camera, info);
> >  }
> >  
> > 

Re: [PATCH 1/6] media: mach-pxa: Register the camera sensor fixed-rate clock

[Petr Cvek]

Dne 06. 01. 21 v 16:53 Ezequiel Garcia napsal(a):
> Hi Petr,
> 
> Thanks a lot for reviewing and testing the series.
> 
> On Tue, 2021-01-05 at 17:41 +0100, Petr Cvek wrote:
>>
>> Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a):
>>> The pxa-camera capture driver currently registers a v4l2-clk
>>> clock, named "mclk", to represent the mt9m111 sensor clock.
>>>
>>> Register a proper fixed-rate clock using the generic clock framework,
>>> which will allow to remove the v4l2-clk clock in the pxa-camera
>>> driver in a follow-up commit.
>>>
>>
>> BTW the mclk output to a sensor is actually a variable rate, divided from lcdclk (which can be changed too). PXA camera driver  is using variable
>> pcdev->mclk_divisor to generate the mclk from lcdclk. 
>>
> 
> Hm, now that I look at this, I see the pxa-camera driver
> is requiring a clock:
> 
>         pcdev->clk = devm_clk_get(&pdev->dev, NULL);                             
>         if (IS_ERR(pcdev->clk))                                                  
>                 return PTR_ERR(pcdev->clk);   
> 
> Where is this clock registered in the non-devicetree case?
> 

I think this is where the clock is defined 

	PXA27X_CKEN_1RATE("pxa27x-camera.0", NULL, CAMERA, pxa27x_lcd_bus_parents, 0),

https://elixir.bootlin.com/linux/v5.10.2/source/drivers/clk/pxa/clk-pxa27x.c#L180


>> The rate change is done in pxa_camera_activate():
>>
>> https://elixir.bootlin.com/linux/v5.11-rc2/source/drivers/media/platform/pxa_camera.c#L1136
>>
>>         __raw_writel(pcdev->mclk_divisor | cicr4, pcdev->base + CICR4);
>>
>> Would it be possible to register a correct clock type with possibility to change the divisor by the standard way?
>>
> 
> Right, so you mean the pxa-camera driver is the one providing the clock for the sensors?
> 
> In that case, I guess the pxa-camera driver should be the one registering
> a CCF clock. Other drivers are doing this, through clk_register for instance.
> 

Yeah that would make the sense, because the camera controller controls the divider and enable signals.

> However, for the sake of this series, which is meant to get rid
> of the v4l2-clk API, I would say it's fine to just register a fixed-rate.
> 
> This is similar to what v4l2_clk_register was doing, which was registering
> a dummy clock.
> 

I guess. Just the 1:1 replacement. 

> Having said that, as I mentioned above, I'm wondering if the mach-pxa
> boards are really working, given I'm not seeing the clock for pxa-camera.
> 
> Maybe the best way forward is to just accept that pxa-camera
> is only supported for the device tree platforms, and therefore drop the
> support from mach-pxa/ boards.
> 

PXA camera worked without devicetree without problems (I'm not sure if I ever used devicetree). The definition should be in that file above (but I'm not that familiar with the clock framework).

best regards,
Petr

> Thanks,
> Ezequiel
>  
> 
>> Petr
>>
>>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Robert Jarzmik <robert.jarzmik@free.fr>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>>  arch/arm/mach-pxa/devices.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c
>>> index 524d6093e0c7..09b8495f3fd9 100644
>>> --- a/arch/arm/mach-pxa/devices.c
>>> +++ b/arch/arm/mach-pxa/devices.c
>>> @@ -4,6 +4,7 @@
>>>  #include <linux/init.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/clkdev.h>
>>> +#include <linux/clk-provider.h>
>>>  #include <linux/dma-mapping.h>
>>>  #include <linux/dmaengine.h>
>>>  #include <linux/spi/pxa2xx_spi.h>
>>> @@ -634,6 +635,13 @@ static struct platform_device pxa27x_device_camera = {
>>>  
>>>  void __init pxa_set_camera_info(struct pxacamera_platform_data *info)
>>>  {
>>> +       struct clk *mclk;
>>> +
>>> +       /* Register a fixed-rate clock for camera sensors. */
>>> +       mclk = clk_register_fixed_rate(NULL, "pxa_camera_clk", NULL, 0,
>>> +                                            info->mclk_10khz * 10000);
>>> +       if (!IS_ERR(mclk))
>>> +               clkdev_create(mclk, "mclk", NULL);
>>>         pxa_register_device(&pxa27x_device_camera, info);
>>>  }
>>>  
>>>
> 
> 

Re: [PATCH 0/6] Remove last users of v4l2-clk and remove v4l2-clk

[Petr Cvek]

Dne 06. 01. 21 v 15:24 Ezequiel Garcia napsal(a):
> Hi Petr,
> 
> On Tue, 2021-01-05 at 17:08 +0100, Petr Cvek wrote:
>> I don't have a working magician setup at the moment, so I can only test the compilation (which works).
>>
> 
> Thanks for the testing! Does that mean I can take your Tested-by ?

Well it was just a compilation and I don't know ov6650 nor mt9m111. And I may have some time in the next month to try it on the real device.


> 
>> Petr
>>
>> Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a):
>>> The V4L2 temporary clock helper API is used by just one last capture
>>> driver, pxa-camera, which registers a dummy clock; and then by just
>>> a few sensor drivers, consuming clocks through the v4l2-clk API.
>>>
>>> It's possible to convert these few last users, and so remove
>>> the v4l2-clk API, which hasn't been used for a few years.
>>>
>>> The sensor drivers are already using the CCF API,
>>> which v4l2-clk API uses as fallback.
>>>
>>> To convert the pxa-camera driver, a fixed-rate clock
>>> is registered for the mach-based platforms that still exist,
>>> for mt9m111 to work (the only sensor that PXA currently
>>> registers).
>>>
>>> Ezequiel Garcia (6):
>>>   media: mach-pxa: Register the camera sensor fixed-rate clock
>>>   media: pxa_camera: Drop the v4l2-clk clock register
>>>   media: ov9640: Use the generic clock framework
>>>   media: mt9m111: Use the generic clock framework
>>>   media: ov6650: Use the generic clock framework
>>>   media: Remove the legacy v4l2-clk API
>>>
>>>  .../driver-api/media/v4l2-clocks.rst          |  31 --
>>>  Documentation/driver-api/media/v4l2-core.rst  |   1 -
>>>  arch/arm/mach-pxa/devices.c                   |   8 +
>>>  drivers/media/i2c/mt9m111.c                   |  17 +-
>>>  drivers/media/i2c/ov6650.c                    |  26 +-
>>>  drivers/media/i2c/ov9640.c                    |  15 +-
>>>  drivers/media/i2c/ov9640.h                    |   4 +-
>>>  drivers/media/platform/pxa_camera.c           |  30 +-
>>>  drivers/media/v4l2-core/Makefile              |   2 +-
>>>  drivers/media/v4l2-core/v4l2-clk.c            | 321 ------------------
>>>  include/media/v4l2-clk.h                      |  73 ----
>>>  11 files changed, 37 insertions(+), 491 deletions(-)
>>>  delete mode 100644 Documentation/driver-api/media/v4l2-clocks.rst
>>>  delete mode 100644 drivers/media/v4l2-core/v4l2-clk.c
>>>  delete mode 100644 include/media/v4l2-clk.h
>>>
> 
> 

Re: [PATCH 5/6] media: ov6650: Use the generic clock framework

[Janusz Krzysztofik]

On Monday, January 4, 2021 5:57:38 P.M. CET Ezequiel Garcia wrote:
> Commit ce548396a433 ("media: mach-omap1: board-ams-delta.c: remove soc_camera dependencies")
> removed the last in-tree user of this sensor. New users
> will be required to use the generic clock framework,
> so it's possible to convert the driver to use it.
> 
> Convert the driver to use the CCF, and drop the legacy
> v4l2-clk API.
> 
> Cc: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/i2c/ov6650.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index d73f9f540932..0f8242054603 100644
> --- a/drivers/media/i2c/ov6650.c
> +++ b/drivers/media/i2c/ov6650.c
> @@ -22,13 +22,13 @@
>   */
>  
>  #include <linux/bitops.h>
> +#include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
>  #include <linux/v4l2-mediabus.h>
>  #include <linux/module.h>
>  
> -#include <media/v4l2-clk.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  
> @@ -194,7 +194,7 @@ struct ov6650 {
>  		struct v4l2_ctrl *blue;
>  		struct v4l2_ctrl *red;
>  	};
> -	struct v4l2_clk		*clk;
> +	struct clk		*clk;
>  	bool			half_scale;	/* scale down output by 2 */
>  	struct v4l2_rect	rect;		/* sensor cropping window */
>  	struct v4l2_fract	tpf;		/* as requested with s_frame_interval */
> @@ -459,9 +459,9 @@ static int ov6650_s_power(struct v4l2_subdev *sd, int on)
>  	int ret = 0;
>  
>  	if (on)
> -		ret = v4l2_clk_enable(priv->clk);
> +		ret = clk_prepare_enable(priv->clk);
>  	else
> -		v4l2_clk_disable(priv->clk);
> +		clk_disable_unprepare(priv->clk);
>  
>  	return ret;
>  }
> @@ -821,14 +821,14 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
>  	u8 pidh, pidl, midh, midl;
>  	int i, ret = 0;
>  
> -	priv->clk = v4l2_clk_get(&client->dev, NULL);
> +	priv->clk = devm_clk_get(&client->dev, NULL);
>  	if (IS_ERR(priv->clk)) {
>  		ret = PTR_ERR(priv->clk);
> -		dev_err(&client->dev, "v4l2_clk request err: %d\n", ret);
> +		dev_err(&client->dev, "clk request err: %d\n", ret);
>  		return ret;
>  	}
>  
> -	rate = v4l2_clk_get_rate(priv->clk);
> +	rate = clk_get_rate(priv->clk);
>  	for (i = 0; rate && i < ARRAY_SIZE(ov6650_xclk); i++) {
>  		if (rate != ov6650_xclk[i].rate)
>  			continue;
> @@ -839,8 +839,8 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
>  		break;
>  	}
>  	for (i = 0; !xclk && i < ARRAY_SIZE(ov6650_xclk); i++) {
> -		ret = v4l2_clk_set_rate(priv->clk, ov6650_xclk[i].rate);
> -		if (ret || v4l2_clk_get_rate(priv->clk) != ov6650_xclk[i].rate)
> +		ret = clk_set_rate(priv->clk, ov6650_xclk[i].rate);
> +		if (ret || clk_get_rate(priv->clk) != ov6650_xclk[i].rate)
>  			continue;
>  
>  		xclk = &ov6650_xclk[i];
> @@ -852,12 +852,12 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
>  		dev_err(&client->dev, "unable to get supported clock rate\n");
>  		if (!ret)
>  			ret = -EINVAL;
> -		goto eclkput;
> +		return ret;
>  	}
>  
>  	ret = ov6650_s_power(sd, 1);
>  	if (ret < 0)
> -		goto eclkput;
> +		return ret;
>  
>  	msleep(20);
>  
> @@ -901,9 +901,6 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
>  	ov6650_s_power(sd, 0);
>  	if (!ret)
>  		return 0;

I think the above two lines are no longer needed and should be removed.  
Anyway,

Reviewed-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Thanks,
Janusz

> -eclkput:
> -	v4l2_clk_put(priv->clk);
> -
>  	return ret;
>  }
>  
> @@ -1089,7 +1086,6 @@ static int ov6650_remove(struct i2c_client *client)
>  {
>  	struct ov6650 *priv = to_ov6650(client);
>  
> -	v4l2_clk_put(priv->clk);
>  	v4l2_async_unregister_subdev(&priv->subdev);
>  	v4l2_ctrl_handler_free(&priv->hdl);
>  	return 0;
> 

Re: [PATCH 1/6] media: mach-pxa: Register the camera sensor fixed-rate clock

[Ezequiel Garcia]

On Fri, 2021-01-08 at 12:02 +0100, Petr Cvek wrote:
> Dne 06. 01. 21 v 16:53 Ezequiel Garcia napsal(a):
> > Hi Petr,
> > 
> > Thanks a lot for reviewing and testing the series.
> > 
> > On Tue, 2021-01-05 at 17:41 +0100, Petr Cvek wrote:
> > > 
> > > Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a):
> > > > The pxa-camera capture driver currently registers a v4l2-clk
> > > > clock, named "mclk", to represent the mt9m111 sensor clock.
> > > > 
> > > > Register a proper fixed-rate clock using the generic clock framework,
> > > > which will allow to remove the v4l2-clk clock in the pxa-camera
> > > > driver in a follow-up commit.
> > > > 
> > > 
> > > BTW the mclk output to a sensor is actually a variable rate, divided from lcdclk (which can be changed too). PXA camera driver  is using
> > > variable
> > > pcdev->mclk_divisor to generate the mclk from lcdclk. 
> > > 
> > 
> > Hm, now that I look at this, I see the pxa-camera driver
> > is requiring a clock:
> > 
> >         pcdev->clk = devm_clk_get(&pdev->dev, NULL);                             
> >         if (IS_ERR(pcdev->clk))                                                  
> >                 return PTR_ERR(pcdev->clk);   
> > 
> > Where is this clock registered in the non-devicetree case?
> > 
> 
> I think this is where the clock is defined 
> 
>         PXA27X_CKEN_1RATE("pxa27x-camera.0", NULL, CAMERA, pxa27x_lcd_bus_parents, 0),
> 
> https://elixir.bootlin.com/linux/v5.10.2/source/drivers/clk/pxa/clk-pxa27x.c#L180
> 

Ah, nice. That's the part I was missing.
> 
> > > The rate change is done in pxa_camera_activate():
> > > 
> > > https://elixir.bootlin.com/linux/v5.11-rc2/source/drivers/media/platform/pxa_camera.c#L1136
> > > 
> > >         __raw_writel(pcdev->mclk_divisor | cicr4, pcdev->base + CICR4);
> > > 
> > > Would it be possible to register a correct clock type with possibility to change the divisor by the standard way?
> > > 
> > 
> > Right, so you mean the pxa-camera driver is the one providing the clock for the sensors?
> > 
> > In that case, I guess the pxa-camera driver should be the one registering
> > a CCF clock. Other drivers are doing this, through clk_register for instance.
> > 
> 
> Yeah that would make the sense, because the camera controller controls the divider and enable signals.
> 
> > However, for the sake of this series, which is meant to get rid
> > of the v4l2-clk API, I would say it's fine to just register a fixed-rate.
> > 
> > This is similar to what v4l2_clk_register was doing, which was registering
> > a dummy clock.
> > 
> 
> I guess. Just the 1:1 replacement. 
> 

Exactly.

> > Having said that, as I mentioned above, I'm wondering if the mach-pxa
> > boards are really working, given I'm not seeing the clock for pxa-camera.
> > 
> > Maybe the best way forward is to just accept that pxa-camera
> > is only supported for the device tree platforms, and therefore drop the
> > support from mach-pxa/ boards.
> > 
> 
> PXA camera worked without devicetree without problems (I'm not sure if I ever used devicetree). The definition should be in that file above (but I'm
> not that familiar with the clock framework).
> 

That's fine.

I'm quite sure this series should be fine,
since we are just replacing the dummy v4l2-clk with
dummy proper CCF clock.

Thanks!
Ezequiel

Re: [PATCH 1/6] media: mach-pxa: Register the camera sensor fixed-rate clock

[Arnd Bergmann]

On Mon, Jan 4, 2021 at 5:57 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> The pxa-camera capture driver currently registers a v4l2-clk
> clock, named "mclk", to represent the mt9m111 sensor clock.
>
> Register a proper fixed-rate clock using the generic clock framework,
> which will allow to remove the v4l2-clk clock in the pxa-camera
> driver in a follow-up commit.
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Robert Jarzmik <robert.jarzmik@free.fr>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

If there are no objections to the change itself, please take it through
the v4l2 git tree. For arch/arm/mach-*/

Acked-by: Arnd Bergmann <arnd@arndb.de>