* [PATCH v7 0/2] On Semi AR0521 sensor driver
@ 2022-01-03 13:29 Krzysztof Hałasa
2022-01-03 13:32 ` [PATCH v7 1/2] dt-binding: media: document ON Semi AR0521 sensor bindings Krzysztof Hałasa
2022-01-03 13:36 ` [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor Krzysztof Hałasa
0 siblings, 2 replies; 10+ messages in thread
From: Krzysztof Hałasa @ 2022-01-03 13:29 UTC (permalink / raw)
To: Rob Herring, Mauro Carvalho Chehab
Cc: devicetree, linux-media, linux-kernel, Laurent Pinchart,
Sakari Ailus, Jacopo Mondi, Joe Perches
Rob, Mauro, media subsystem reviewers,
This is the 7th version of my On Semi AR0521 sensor driver.
Is there anything here that should be changed in order to get it merged?
The documentation patch (1/2) hasn't been changed from v4:
onnn,ar0521.yaml | 112
1 file changed, 112 insertions(+)
The actual driver (2/2) stats:
MAINTAINERS | 7
drivers/media/i2c/Kconfig | 13
drivers/media/i2c/Makefile | 1
drivers/media/i2c/ar0521.c | 1093
4 files changed, 1114 insertions
v7:
- removed AR0521_NUM_SUPPLIES macro: ARRAY_SIZE(ar0521_supply_names)
is now used directly.
- fixed ar0521_power_off() return type, reported-by: kernel test robot
<lkp@intel.com> (apparently can't add this tag for the whole patch).
- moved pm_runtime_get_if_in_use()/pm_runtime_put() up the stack.
The old way was causing problems when used in sensor power_on(),
before initial pm_runtime setup.
- clearer REGS() macro
v6:
- I reformatted the code to fit in 80 columns. Nobody should be asked to
make his code worse (and the 80-column version IS worse), and multiple
high-profile Linux developers (including the top one) appear to share
my opinion, but nevertheless - if it's something that will make it go
in, I won't care.
- Basically the same applies to the // comments.
- I have removed the "interval" support (frames per second).
Unfortunately this cripples the driver further a bit - the userspace
will not be able to set precise frame timings needed for broadcast
quality video. I will have to keep a private patch for that.
Another effect of this change is that the pixel clock is now fixed at
184 MHz, which by default produces ca. 30 FPS at 2560x1920. This may
be problematic on systems with less than 4 MIPI lanes, and/or on ones
which can't support higher frequency MIPI bus (the previous version
used a calculated clock). Perhaps it will be possible to fix this
issue in the future, with a couple of core V4L2 changes.
- the driver now provides the .pre_streamon() for setting LP-11 state on
MIPI data and clock lanes. This is compatible with i.MX6 receiver.
- s_power() converted to SET_RUNTIME_PM_OPS().
- the "initial" I2C registers have been all converted to a table of
multi-register files, to minimize time spent on I2C bus.
And a lot of smaller changes suggested by Laurent, Sakari, Jacopo, Joe
and possibly others.
--
Krzysztof "Chris" Hałasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v7 1/2] dt-binding: media: document ON Semi AR0521 sensor bindings
2022-01-03 13:29 [PATCH v7 0/2] On Semi AR0521 sensor driver Krzysztof Hałasa
@ 2022-01-03 13:32 ` Krzysztof Hałasa
2022-01-03 13:36 ` [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor Krzysztof Hałasa
1 sibling, 0 replies; 10+ messages in thread
From: Krzysztof Hałasa @ 2022-01-03 13:32 UTC (permalink / raw)
To: Rob Herring, Mauro Carvalho Chehab
Cc: devicetree, linux-media, linux-kernel, Laurent Pinchart,
Sakari Ailus, Jacopo Mondi
This file documents DT bindings for the AR0521 camera sensor driver.
Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Rob Herring <robh@kernel.org>
diff --git
a/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
b/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
new file mode 100644
index 000000000000..b617cc5c6a9f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
@@ -0,0 +1,112 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/onnn,ar0521.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ON Semiconductor AR0521 MIPI CSI-2 sensor
+
+maintainers:
+ - Krzysztof Hałasa <khalasa@piap.pl>
+
+description: |-
+ The AR0521 is a raw CMOS image sensor with MIPI CSI-2 and
+ I2C-compatible control interface.
+
+properties:
+ compatible:
+ const: onnn,ar0521
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ clock-names:
+ const: extclk
+
+ vaa-supply:
+ description:
+ Definition of the regulator used as analog (2.7 V) voltage supply.
+
+ vdd-supply:
+ description:
+ Definition of the regulator used as digital core (1.2 V) voltage supply.
+
+ vdd_io-supply:
+ description:
+ Definition of the regulator used as digital I/O (1.8 V) voltage supply.
+
+ reset-gpios:
+ description: reset GPIO, usually active low
+ maxItems: 1
+
+ port:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ unevaluatedProperties: false
+ description: |
+ Video output port.
+
+ properties:
+ endpoint:
+ $ref: /schemas/media/video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ bus-type:
+ const: 4
+ data-lanes:
+ anyOf:
+ - items:
+ - const: 1
+ - items:
+ - const: 1
+ - const: 2
+ - items:
+ - const: 1
+ - const: 2
+ - const: 3
+ - const: 4
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - vaa-supply
+ - vdd-supply
+ - vdd_io-supply
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/clock/imx6qdl-clock.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ar0521: camera-sensor@36 {
+ compatible = "onnn,ar0521";
+ reg = <0x36>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_mipi_camera>;
+ clocks = <&clks IMX6QDL_CLK_CKO>;
+ clock-names = "extclk";
+ reset-gpios = <&gpio1 7 GPIO_ACTIVE_LOW>;
+ vaa-supply = <®_2p7v>;
+ vdd-supply = <®_1p2v>;
+ vdd_io-supply = <®_1p8v>;
+
+ port {
+ mipi_camera_to_mipi_csi2: endpoint {
+ remote-endpoint = <&mipi_csi2_in>;
+ data-lanes = <1 2 3 4>;
+ };
+ };
+ };
+ };
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor
2022-01-03 13:29 [PATCH v7 0/2] On Semi AR0521 sensor driver Krzysztof Hałasa
2022-01-03 13:32 ` [PATCH v7 1/2] dt-binding: media: document ON Semi AR0521 sensor bindings Krzysztof Hałasa
@ 2022-01-03 13:36 ` Krzysztof Hałasa
2022-01-09 15:34 ` Jacopo Mondi
1 sibling, 1 reply; 10+ messages in thread
From: Krzysztof Hałasa @ 2022-01-03 13:36 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Rob Herring
Cc: devicetree, linux-media, linux-kernel, Laurent Pinchart,
Sakari Ailus, Jacopo Mondi, Joe Perches
The driver has been extensively tested in an i.MX6-based system.
AR0521 is a 5.7 mm x 4.3 mm, 5 MPix RGGB MIPI/HiSPi BSI CMOS sensor
from On Semiconductor.
Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
diff --git a/MAINTAINERS b/MAINTAINERS
index 79fd8a012893..5d33f0277ad2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1383,6 +1383,13 @@ S: Supported
W: http://www.aquantia.com
F: drivers/net/ethernet/aquantia/atlantic/aq_ptp*
+AR0521 ON SEMICONDUCTOR CAMERA SENSOR DRIVER
+M: Krzysztof Hałasa <khalasa@piap.pl>
+L: linux-media@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
+F: drivers/media/i2c/ar0521.c
+
ARASAN NAND CONTROLLER DRIVER
M: Miquel Raynal <miquel.raynal@bootlin.com>
M: Naga Sureshkumar Relli <nagasure@xilinx.com>
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 69c56e24a612..741ae073623e 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -733,6 +733,19 @@ config VIDEO_APTINA_PLL
config VIDEO_CCS_PLL
tristate
+config VIDEO_AR0521
+ tristate "ON Semiconductor AR0521 sensor support"
+ depends on I2C && VIDEO_V4L2
+ select MEDIA_CONTROLLER
+ select VIDEO_V4L2_SUBDEV_API
+ select V4L2_FWNODE
+ help
+ This is a Video4Linux2 sensor driver for the ON Semiconductor
+ AR0521 camera.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ar0521.
+
config VIDEO_HI556
tristate "Hynix Hi-556 sensor support"
depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index b01f6cd05ee8..4e4986e16071 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -118,6 +118,7 @@ obj-$(CONFIG_VIDEO_I2C) += video-i2c.o
obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o
obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
+obj-$(CONFIG_VIDEO_AR0521) += ar0521.o
obj-$(CONFIG_VIDEO_HI556) += hi556.o
obj-$(CONFIG_VIDEO_HI846) += hi846.o
obj-$(CONFIG_VIDEO_IMX208) += imx208.o
diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
new file mode 100644
index 000000000000..0adbbd8ea3b5
--- /dev/null
+++ b/drivers/media/i2c/ar0521.c
@@ -0,0 +1,1093 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021 Sieć Badawcza Łukasiewicz
+ * - Przemysłowy Instytut Automatyki i Pomiarów PIAP
+ * Written by Krzysztof Hałasa
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/pm_runtime.h>
+
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+/* External clock (extclk) frequencies */
+#define AR0521_EXTCLK_MIN (10 * 1000 * 1000)
+#define AR0521_EXTCLK_MAX (48 * 1000 * 1000)
+
+/* PLL and PLL2 */
+#define AR0521_PLL_MIN (320 * 1000 * 1000)
+#define AR0521_PLL_MAX (1280 * 1000 * 1000)
+
+/* Effective pixel clocks, the registers may be DDR */
+#define AR0521_PIXEL_CLOCK_RATE (184 * 1000 * 1000)
+#define AR0521_PIXEL_CLOCK_MIN (168 * 1000 * 1000)
+#define AR0521_PIXEL_CLOCK_MAX (414 * 1000 * 1000)
+
+#define AR0521_WIDTH_MIN 8u
+#define AR0521_WIDTH_MAX 2608u
+#define AR0521_HEIGHT_MIN 8u
+#define AR0521_HEIGHT_MAX 1958u
+
+#define AR0521_WIDTH_BLANKING_MIN 572u
+#define AR0521_HEIGHT_BLANKING_MIN 38u /* must be even */
+#define AR0521_TOTAL_WIDTH_MIN 2968u
+
+/* AR0521 registers */
+#define AR0521_REG_VT_PIX_CLK_DIV 0x0300
+#define AR0521_REG_FRAME_LENGTH_LINES 0x0340
+
+#define AR0521_REG_CHIP_ID 0x3000
+#define AR0521_REG_COARSE_INTEGRATION_TIME 0x3012
+#define AR0521_REG_ROW_SPEED 0x3016
+#define AR0521_REG_EXTRA_DELAY 0x3018
+#define AR0521_REG_RESET 0x301A
+#define AR0521_REG_RESET_DEFAULTS 0x0238
+#define AR0521_REG_RESET_GROUP_PARAM_HOLD 0x8000
+#define AR0521_REG_RESET_STREAM BIT(2)
+#define AR0521_REG_RESET_RESTART BIT(1)
+#define AR0521_REG_RESET_INIT BIT(0)
+
+#define AR0521_REG_GREEN1_GAIN 0x3056
+#define AR0521_REG_BLUE_GAIN 0x3058
+#define AR0521_REG_RED_GAIN 0x305A
+#define AR0521_REG_GREEN2_GAIN 0x305C
+#define AR0521_REG_GLOBAL_GAIN 0x305E
+
+#define AR0521_REG_HISPI_TEST_MODE 0x3066
+#define AR0521_REG_HISPI_TEST_MODE_LP11 0x0004
+
+#define AR0521_REG_TEST_PATTERN_MODE 0x3070
+
+#define AR0521_REG_SERIAL_FORMAT 0x31AE
+#define AR0521_REG_SERIAL_FORMAT_MIPI 0x0200
+
+#define AR0521_REG_HISPI_CONTROL_STATUS 0x31C6
+#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80
+
+#define be cpu_to_be16
+
+static const char * const ar0521_supply_names[] = {
+ "vdd_io", /* I/O (1.8V) supply */
+ "vdd", /* Core, PLL and MIPI (1.2V) supply */
+ "vaa", /* Analog (2.7V) supply */
+};
+
+struct ar0521_ctrls {
+ struct v4l2_ctrl_handler handler;
+ struct {
+ struct v4l2_ctrl *gain;
+ struct v4l2_ctrl *red_balance;
+ struct v4l2_ctrl *blue_balance;
+ };
+ struct {
+ struct v4l2_ctrl *hblank;
+ struct v4l2_ctrl *vblank;
+ };
+ struct v4l2_ctrl *pixrate;
+ struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *test_pattern;
+};
+
+struct ar0521_dev {
+ struct i2c_client *i2c_client;
+ struct v4l2_subdev sd;
+ struct media_pad pad;
+ struct clk *extclk;
+ u32 extclk_freq;
+
+ struct regulator *supplies[ARRAY_SIZE(ar0521_supply_names)];
+ struct gpio_desc *reset_gpio;
+
+ /* lock to protect all members below */
+ struct mutex lock;
+
+ struct v4l2_mbus_framefmt fmt;
+ struct ar0521_ctrls ctrls;
+ unsigned int lane_count;
+ u16 total_width;
+ u16 total_height;
+ u16 pll_pre;
+ u16 pll_mult;
+ u16 pll_pre2;
+ u16 pll_mult2;
+ bool streaming;
+};
+
+static inline struct ar0521_dev *to_ar0521_dev(struct v4l2_subdev *sd)
+{
+ return container_of(sd, struct ar0521_dev, sd);
+}
+
+static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
+{
+ return &container_of(ctrl->handler, struct ar0521_dev,
+ ctrls.handler)->sd;
+}
+
+static u32 div64_round(u64 v, u32 d)
+{
+ return div_u64(v + (d >> 1), d);
+}
+
+static u32 div64_round_up(u64 v, u32 d)
+{
+ return div_u64(v + d - 1, d);
+}
+
+/* Data must be BE16, the first value is the register address */
+static int ar0521_write_regs(struct ar0521_dev *sensor, const __be16 *data,
+ unsigned int count)
+{
+ struct i2c_client *client = sensor->i2c_client;
+ struct i2c_msg msg;
+ int ret;
+
+ msg.addr = client->addr;
+ msg.flags = client->flags;
+ msg.buf = (u8 *)data;
+ msg.len = count * sizeof(*data);
+
+ ret = i2c_transfer(client->adapter, &msg, 1);
+
+ if (ret < 0) {
+ v4l2_err(&sensor->sd, "%s: I2C write error\n", __func__);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int ar0521_write_reg(struct ar0521_dev *sensor, u16 reg, u16 val)
+{
+ __be16 buf[2] = {be(reg), be(val)};
+
+ return ar0521_write_regs(sensor, buf, 2);
+}
+
+static int ar0521_set_geometry(struct ar0521_dev *sensor)
+{
+ /* All dimensions are unsigned 12-bit integers */
+ u16 x = (AR0521_WIDTH_MAX - sensor->fmt.width) / 2;
+ u16 y = ((AR0521_HEIGHT_MAX - sensor->fmt.height) / 2) & ~1;
+ __be16 regs[] = {
+ be(AR0521_REG_FRAME_LENGTH_LINES),
+ be(sensor->total_height),
+ be(sensor->total_width),
+ be(x),
+ be(y),
+ be(x + sensor->fmt.width - 1),
+ be(y + sensor->fmt.height - 1),
+ be(sensor->fmt.width),
+ be(sensor->fmt.height)
+ };
+
+ dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+
+ return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
+}
+
+static int ar0521_set_gains(struct ar0521_dev *sensor)
+{
+ int green = sensor->ctrls.gain->val;
+ int red = max(green + sensor->ctrls.red_balance->val, 0);
+ int blue = max(green + sensor->ctrls.blue_balance->val, 0);
+ unsigned int gain = min(red, min(green, blue));
+ unsigned int analog = min(gain, 64u); /* range is 0 - 127 */
+ __be16 regs[5];
+
+ dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+
+ red = min(red - analog + 64, 511u);
+ green = min(green - analog + 64, 511u);
+ blue = min(blue - analog + 64, 511u);
+ regs[0] = be(AR0521_REG_GREEN1_GAIN);
+ regs[1] = be(green << 7 | analog);
+ regs[2] = be(blue << 7 | analog);
+ regs[3] = be(red << 7 | analog);
+ regs[4] = be(green << 7 | analog);
+
+ return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
+}
+
+static u32 calc_pll(struct ar0521_dev *sensor, int num, u32 freq, u16 *pre_ptr,
+ u16 *mult_ptr)
+{
+ u16 pre = 1, mult = 1, new_pre;
+ u32 pll = AR0521_PLL_MAX + 1;
+
+ for (new_pre = 1; new_pre < 64; new_pre++) {
+ u32 new_pll;
+ u32 new_mult = div64_round_up((u64)freq * new_pre,
+ sensor->extclk_freq);
+
+ if (new_mult < 32)
+ continue; /* Minimum value */
+ if (new_mult > 254)
+ break; /* Maximum, larger pre won't work either */
+ if (sensor->extclk_freq * (u64)new_mult < AR0521_PLL_MIN *
+ new_pre)
+ continue;
+ if (sensor->extclk_freq * (u64)new_mult > AR0521_PLL_MAX *
+ new_pre)
+ break; /* Larger pre won't work either */
+ new_pll = div64_round_up(sensor->extclk_freq * (u64)new_mult,
+ new_pre);
+ if (new_pll < pll) {
+ pll = new_pll;
+ pre = new_pre;
+ mult = new_mult;
+ }
+ }
+
+ pll = div64_round(sensor->extclk_freq * (u64)mult, pre);
+ *pre_ptr = pre;
+ *mult_ptr = mult;
+ return pll;
+}
+
+#define DIV 4
+static void ar0521_calc_mode(struct ar0521_dev *sensor)
+{
+ unsigned int speed_mod = 4 / sensor->lane_count; /* 1 with 4 DDR lanes */
+ u16 total_width = max(sensor->fmt.width + AR0521_WIDTH_BLANKING_MIN,
+ AR0521_TOTAL_WIDTH_MIN);
+ u16 total_height = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN;
+
+ /* Calculate approximate pixel clock first */
+ u64 pix_clk = AR0521_PIXEL_CLOCK_RATE;
+
+ /* PLL1 drives pixel clock - dual rate */
+ pix_clk = calc_pll(sensor, 1, pix_clk * (DIV / 2), &sensor->pll_pre,
+ &sensor->pll_mult);
+ pix_clk = div64_round(pix_clk, (DIV / 2));
+ calc_pll(sensor, 2, pix_clk * (DIV / 2) * speed_mod, &sensor->pll_pre2,
+ &sensor->pll_mult2);
+
+ sensor->total_width = total_width;
+ sensor->total_height = total_height;
+}
+
+static int ar0521_write_mode(struct ar0521_dev *sensor)
+{
+ __be16 pll_regs[] = {
+ be(AR0521_REG_VT_PIX_CLK_DIV),
+ /* 0x300 */ be(4), /* vt_pix_clk_div = number of bits / 2 */
+ /* 0x302 */ be(1), /* vt_sys_clk_div */
+ /* 0x304 */ be((sensor->pll_pre2 << 8) | sensor->pll_pre),
+ /* 0x306 */ be((sensor->pll_mult2 << 8) | sensor->pll_mult),
+ /* 0x308 */ be(8), /* op_pix_clk_div = 2 * vt_pix_clk_div */
+ /* 0x30A */ be(1) /* op_sys_clk_div */
+ };
+ int ret;
+
+ dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+
+ /* Stop streaming for just a moment */
+ ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
+ AR0521_REG_RESET_DEFAULTS);
+ if (ret)
+ return ret;
+
+ ret = ar0521_set_geometry(sensor);
+ if (ret)
+ return ret;
+
+ ret = ar0521_write_regs(sensor, pll_regs, ARRAY_SIZE(pll_regs));
+ if (ret)
+ return ret;
+
+ ret = ar0521_write_reg(sensor, AR0521_REG_COARSE_INTEGRATION_TIME,
+ sensor->ctrls.exposure->val);
+ if (ret)
+ return ret;
+
+ ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
+ AR0521_REG_RESET_DEFAULTS |
+ AR0521_REG_RESET_STREAM);
+ if (ret)
+ return ret;
+
+ ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE,
+ sensor->ctrls.test_pattern->val);
+ if (ret)
+ return ret;
+
+ dev_dbg(&sensor->i2c_client->dev,
+ "AR0521: %ux%u, total %ux%u\n",
+ sensor->fmt.width, sensor->fmt.height, sensor->total_width,
+ sensor->total_height);
+ return 0;
+}
+
+static int ar0521_set_stream(struct ar0521_dev *sensor, bool on)
+{
+ int ret;
+
+ dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, on);
+
+ if (on) {
+ ret = pm_runtime_resume_and_get(&sensor->i2c_client->dev);
+ if (ret < 0)
+ return ret;
+
+ ar0521_calc_mode(sensor);
+ ret = ar0521_write_mode(sensor);
+ if (ret)
+ goto err;
+
+ ret = ar0521_set_gains(sensor);
+ if (ret)
+ goto err;
+
+ /* Exit LP-11 mode on clock and data lanes */
+ ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS,
+ 0);
+ if (ret)
+ goto err;
+
+ /* Start streaming */
+ ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
+ AR0521_REG_RESET_DEFAULTS |
+ AR0521_REG_RESET_STREAM);
+ if (ret)
+ goto err;
+
+ return 0;
+
+err:
+ pm_runtime_put(&sensor->i2c_client->dev);
+ return ret;
+
+ } else {
+ /* Reset gain, the sensor may produce all white pixels without
+ this */
+ ret = ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, 0x2000);
+ if (ret)
+ return ret;
+
+ /* Stop streaming */
+ ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
+ AR0521_REG_RESET_DEFAULTS);
+ if (ret)
+ return ret;
+
+ pm_runtime_put(&sensor->i2c_client->dev);
+ return 0;
+ }
+}
+
+static void ar0521_adj_fmt(struct v4l2_mbus_framefmt *fmt)
+{
+ fmt->width = clamp(ALIGN(fmt->width, 4), AR0521_WIDTH_MIN,
+ AR0521_WIDTH_MAX);
+ fmt->height = clamp(ALIGN(fmt->height, 4), AR0521_HEIGHT_MIN,
+ AR0521_HEIGHT_MAX);
+ fmt->code = MEDIA_BUS_FMT_SGRBG8_1X8;
+ fmt->field = V4L2_FIELD_NONE;
+ fmt->colorspace = V4L2_COLORSPACE_SRGB;
+ fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+ fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+ fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
+}
+
+static int ar0521_get_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_format *format)
+{
+ struct ar0521_dev *sensor = to_ar0521_dev(sd);
+ struct v4l2_mbus_framefmt *fmt;
+
+ dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, format->which);
+
+ mutex_lock(&sensor->lock);
+
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY)
+ fmt = v4l2_subdev_get_try_format(&sensor->sd, sd_state, 0
+ /* pad */);
+ else
+ fmt = &sensor->fmt;
+
+ format->format = *fmt;
+
+ mutex_unlock(&sensor->lock);
+ return 0;
+}
+
+static int ar0521_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_format *format)
+{
+ struct ar0521_dev *sensor = to_ar0521_dev(sd);
+ int ret = 0;
+
+ dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, format->which);
+
+ ar0521_adj_fmt(&format->format);
+
+ mutex_lock(&sensor->lock);
+
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+ struct v4l2_mbus_framefmt *fmt;
+
+ fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
+ *fmt = format->format;
+ } else {
+ sensor->fmt = format->format;
+ ar0521_calc_mode(sensor);
+ }
+
+ mutex_unlock(&sensor->lock);
+ return ret;
+}
+
+static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
+ struct ar0521_dev *sensor = to_ar0521_dev(sd);
+ int ret;
+
+ /* v4l2_ctrl_lock() locks our own mutex */
+
+ dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, ctrl->id);
+
+ switch (ctrl->id) {
+ case V4L2_CID_HBLANK:
+ case V4L2_CID_VBLANK:
+ sensor->total_width = sensor->fmt.width +
+ sensor->ctrls.hblank->val;
+ sensor->total_height = sensor->fmt.width +
+ sensor->ctrls.vblank->val;
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ // access the sensor only if it's powered up
+ if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
+ return 0;
+
+ switch (ctrl->id) {
+ case V4L2_CID_HBLANK:
+ case V4L2_CID_VBLANK:
+ ret = ar0521_set_geometry(sensor);
+ break;
+ case V4L2_CID_GAIN:
+ case V4L2_CID_RED_BALANCE:
+ case V4L2_CID_BLUE_BALANCE:
+ ret = ar0521_set_gains(sensor);
+ break;
+ case V4L2_CID_EXPOSURE:
+ ret = ar0521_write_reg(sensor,
+ AR0521_REG_COARSE_INTEGRATION_TIME,
+ ctrl->val);
+ break;
+ case V4L2_CID_TEST_PATTERN:
+ ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE,
+ ctrl->val);
+ break;
+ }
+
+ pm_runtime_put(&sensor->i2c_client->dev);
+ return ret;
+}
+
+static const struct v4l2_ctrl_ops ar0521_ctrl_ops = {
+ .s_ctrl = ar0521_s_ctrl,
+};
+
+static const char * const test_pattern_menu[] = {
+ "Disabled",
+ "Solid color",
+ "Color bars",
+ "Faded color bars"
+};
+
+static int ar0521_init_controls(struct ar0521_dev *sensor)
+{
+ const struct v4l2_ctrl_ops *ops = &ar0521_ctrl_ops;
+ struct ar0521_ctrls *ctrls = &sensor->ctrls;
+ struct v4l2_ctrl_handler *hdl = &ctrls->handler;
+ int ret;
+
+ v4l2_ctrl_handler_init(hdl, 32);
+
+ /* We can use our own mutex for the ctrl lock */
+ hdl->lock = &sensor->lock;
+
+ /* Manual gain */
+ ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 511, 1, 0);
+ ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE,
+ -512, 511, 1, 0);
+ ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BLUE_BALANCE,
+ -512, 511, 1, 0);
+ v4l2_ctrl_cluster(3, &ctrls->gain);
+
+ ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
+ AR0521_WIDTH_BLANKING_MIN, 4094, 1,
+ AR0521_WIDTH_BLANKING_MIN);
+ ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
+ AR0521_HEIGHT_BLANKING_MIN, 4094, 2,
+ AR0521_HEIGHT_BLANKING_MIN);
+ v4l2_ctrl_cluster(2, &ctrls->hblank);
+
+ /* Read-only */
+ ctrls->pixrate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
+ AR0521_PIXEL_CLOCK_MIN,
+ AR0521_PIXEL_CLOCK_MAX, 1,
+ AR0521_PIXEL_CLOCK_RATE);
+
+ /* Manual exposure time */
+ ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
+ 65535, 1, 360);
+
+ ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops,
+ V4L2_CID_TEST_PATTERN,
+ ARRAY_SIZE(test_pattern_menu) - 1,
+ 0, 0, test_pattern_menu);
+
+ if (hdl->error) {
+ ret = hdl->error;
+ goto free_ctrls;
+ }
+
+ sensor->sd.ctrl_handler = hdl;
+ return 0;
+
+free_ctrls:
+ v4l2_ctrl_handler_free(hdl);
+ return ret;
+}
+
+#define REGS_ENTRY(a) {(a), ARRAY_SIZE(a)}
+#define REGS(...) REGS_ENTRY(((const __be16[]){__VA_ARGS__}))
+
+static const struct initial_reg {
+ const __be16 *data; /* data[0] is register address */
+ unsigned int count;
+} initial_regs[] = {
+ REGS(be(0x0112), be(0x0808)), /* 8-bit/8-bit mode */
+
+ /* PEDESTAL+2 :+2 is a workaround for 10bit mode +0.5 rounding */
+ REGS(be(0x301E), be(0x00AA)),
+
+ /* corrections_recommended_bayer */
+ REGS(be(0x3042),
+ be(0x0004), /* 3042: RNC: enable b/w rnc mode */
+ be(0x4580)), /* 3044: RNC: enable row noise correction */
+
+ REGS(be(0x30D2),
+ be(0x0000), /* 30D2: CRM/CC: enable crm on Visible and CC rows */
+ be(0x0000), /* 30D4: CC: CC enabled with 16 samples per column */
+ /* 30D6: CC: bw mode enabled/12 bit data resolution/bw mode */
+ be(0x2FFF)),
+
+ REGS(be(0x30DA),
+ be(0x0FFF), /* 30DA: CC: column correction clip level 2 is 0 */
+ be(0x0FFF), /* 30DC: CC: column correction clip level 3 is 0 */
+ be(0x0000)), /* 30DE: CC: Group FPN correction */
+
+ /* RNC: rnc scaling factor = * 54 / 64 (32 / 38 * 64 = 53.9) */
+ REGS(be(0x30EE), be(0x1136)),
+ REGS(be(0x30FA), be(0xFD00)), /* GPIO0 = flash, GPIO1 = shutter */
+ REGS(be(0x3120), be(0x0005)), /* p1 dither enabled for 10bit mode */
+ REGS(be(0x3172), be(0x0206)), /* txlo clk divider options */
+ /* FDOC:fdoc settings with fdoc every frame turned of */
+ REGS(be(0x3180), be(0x9434)),
+
+ REGS(be(0x31B0),
+ be(0x008B), /* 31B0: frame_preamble - FIXME check WRT lanes# */
+ be(0x0050)), /* 31B2: line_preamble - FIXME check WRT lanes# */
+
+ /* don't use continuous clock mode while shut down */
+ REGS(be(0x31BC), be(0x068C)),
+ REGS(be(0x31E0), be(0x0781)), /* Fuse/2DDC: enable 2ddc */
+
+ /* analog_setup_recommended_10bit */
+ REGS(be(0x341A), be(0x4735)), /* Samp&Hold pulse in ADC */
+ REGS(be(0x3420), be(0x4735)), /* Samp&Hold pulse in ADC */
+ REGS(be(0x3426), be(0x8A1A)), /* ADC offset distribution pulse */
+ REGS(be(0x342A), be(0x0018)), /* pulse_config */
+
+ /* pixel_timing_recommended */
+ REGS(be(0x3D00),
+ /* 3D00 */ be(0x043E), be(0x4760), be(0xFFFF), be(0xFFFF),
+ /* 3D08 */ be(0x8000), be(0x0510), be(0xAF08), be(0x0252),
+ /* 3D10 */ be(0x486F), be(0x5D5D), be(0x8056), be(0x8313),
+ /* 3D18 */ be(0x0087), be(0x6A48), be(0x6982), be(0x0280),
+ /* 3D20 */ be(0x8359), be(0x8D02), be(0x8020), be(0x4882),
+ /* 3D28 */ be(0x4269), be(0x6A95), be(0x5988), be(0x5A83),
+ /* 3D30 */ be(0x5885), be(0x6280), be(0x6289), be(0x6097),
+ /* 3D38 */ be(0x5782), be(0x605C), be(0xBF18), be(0x0961),
+ /* 3D40 */ be(0x5080), be(0x2090), be(0x4390), be(0x4382),
+ /* 3D48 */ be(0x5F8A), be(0x5D5D), be(0x9C63), be(0x8063),
+ /* 3D50 */ be(0xA960), be(0x9757), be(0x8260), be(0x5CFF),
+ /* 3D58 */ be(0xBF10), be(0x1681), be(0x0802), be(0x8000),
+ /* 3D60 */ be(0x141C), be(0x6000), be(0x6022), be(0x4D80),
+ /* 3D68 */ be(0x5C97), be(0x6A69), be(0xAC6F), be(0x4645),
+ /* 3D70 */ be(0x4400), be(0x0513), be(0x8069), be(0x6AC6),
+ /* 3D78 */ be(0x5F95), be(0x5F70), be(0x8040), be(0x4A81),
+ /* 3D80 */ be(0x0300), be(0xE703), be(0x0088), be(0x4A83),
+ /* 3D88 */ be(0x40FF), be(0xFFFF), be(0xFD70), be(0x8040),
+ /* 3D90 */ be(0x4A85), be(0x4FA8), be(0x4F8C), be(0x0070),
+ /* 3D98 */ be(0xBE47), be(0x8847), be(0xBC78), be(0x6B89),
+ /* 3DA0 */ be(0x6A80), be(0x6986), be(0x6B8E), be(0x6B80),
+ /* 3DA8 */ be(0x6980), be(0x6A88), be(0x7C9F), be(0x866B),
+ /* 3DB0 */ be(0x8765), be(0x46FF), be(0xE365), be(0xA679),
+ /* 3DB8 */ be(0x4A40), be(0x4580), be(0x44BC), be(0x7000),
+ /* 3DC0 */ be(0x8040), be(0x0802), be(0x10EF), be(0x0104),
+ /* 3DC8 */ be(0x3860), be(0x5D5D), be(0x5682), be(0x1300),
+ /* 3DD0 */ be(0x8648), be(0x8202), be(0x8082), be(0x598A),
+ /* 3DD8 */ be(0x0280), be(0x2048), be(0x3060), be(0x8042),
+ /* 3DE0 */ be(0x9259), be(0x865A), be(0x8258), be(0x8562),
+ /* 3DE8 */ be(0x8062), be(0x8560), be(0x9257), be(0x8221),
+ /* 3DF0 */ be(0x10FF), be(0xB757), be(0x9361), be(0x1019),
+ /* 3DF8 */ be(0x8020), be(0x9043), be(0x8E43), be(0x845F),
+ /* 3E00 */ be(0x835D), be(0x805D), be(0x8163), be(0x8063),
+ /* 3E08 */ be(0xA060), be(0x9157), be(0x8260), be(0x5CFF),
+ /* 3E10 */ be(0xFFFF), be(0xFFE5), be(0x1016), be(0x2048),
+ /* 3E18 */ be(0x0802), be(0x1C60), be(0x0014), be(0x0060),
+ /* 3E20 */ be(0x2205), be(0x8120), be(0x908F), be(0x6A80),
+ /* 3E28 */ be(0x6982), be(0x5F9F), be(0x6F46), be(0x4544),
+ /* 3E30 */ be(0x0005), be(0x8013), be(0x8069), be(0x6A80),
+ /* 3E38 */ be(0x7000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3E40 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3E48 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3E50 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3E58 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3E60 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3E68 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3E70 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3E78 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3E80 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3E88 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3E90 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3E98 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3EA0 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3EA8 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+ /* 3EB0 */ be(0x0000), be(0x0000), be(0x0000)),
+
+ REGS(be(0x3EB6), be(0x004C)), /* ECL */
+
+ REGS(be(0x3EBA),
+ be(0xAAAD), /* 3EBA */
+ be(0x0086)), /* 3EBC: Bias currents for FSC/ECL */
+
+ REGS(be(0x3EC0),
+ be(0x1E00), /* 3EC0: SFbin/SH mode settings */
+ be(0x100A), /* 3EC2: CLK divider for ramp for 10 bit 400MH */
+ /* 3EC4: FSC clamps for HDR mode and adc comp power down co */
+ be(0x3300),
+ be(0xEA44), /* 3EC6: VLN and clk gating controls */
+ be(0x6F6F), /* 3EC8: Txl0 and Txlo1 settings for normal mode */
+ be(0x2F4A), /* 3ECA: CDAC/Txlo2/RSTGHI/RSTGLO settings */
+ be(0x0506), /* 3ECC: RSTDHI/RSTDLO/CDAC/TXHI settings */
+ /* 3ECE: Ramp buffer settings and Booster enable (bits 0-5) */
+ be(0x203B),
+ be(0x13F0), /* 3ED0: TXLO from atest/sf bin settings */
+ be(0xA53D), /* 3ED2: Ramp offset */
+ be(0x862F), /* 3ED4: TXLO open loop/row driver settings */
+ be(0x4081), /* 3ED6: Txlatch fr cfpn rows/vln bias */
+ be(0x8003), /* 3ED8: Ramp step setting for 10 bit 400 Mhz */
+ be(0xA580), /* 3EDA: Ramp Offset */
+ be(0xC000), /* 3EDC: over range for rst and under range for sig */
+ be(0xC103)), /* 3EDE: over range for sig and col dec clk settings */
+
+ /* corrections_recommended_bayer */
+ REGS(be(0x3F00),
+ be(0x0017), /* 3F00: BM_T0 */
+ be(0x02DD), /* 3F02: BM_T1 */
+ /* 3F04: if Ana_gain less than 2, use noise_floor0, multipl */
+ be(0x0020),
+ /* 3F06: if Ana_gain between 4 and 7, use noise_floor2 and */
+ be(0x0040),
+ /* 3F08: if Ana_gain between 4 and 7, use noise_floor2 and */
+ be(0x0070),
+ /* 3F0A: Define noise_floor0(low address) and noise_floor1 */
+ be(0x0101),
+ be(0x0302)), /* 3F0C: Define noise_floor2 and noise_floor3 */
+
+ REGS(be(0x3F10),
+ be(0x0505), /* 3F10: single k factor 0 */
+ be(0x0505), /* 3F12: single k factor 1 */
+ be(0x0505), /* 3F14: single k factor 2 */
+ be(0x01FF), /* 3F16: cross factor 0 */
+ be(0x01FF), /* 3F18: cross factor 1 */
+ be(0x01FF), /* 3F1A: cross factor 2 */
+ be(0x0022)), /* 3F1E */
+
+ /* GTH_THRES_RTN: 4max,4min filtered out of every 46 samples and */
+ REGS(be(0x3F2C), be(0x442E)),
+
+ REGS(be(0x3F3E),
+ be(0x0000), /* 3F3E: Switch ADC from 12 bit to 10 bit mode */
+ be(0x1511), /* 3F40: couple k factor 0 */
+ be(0x1511), /* 3F42: couple k factor 1 */
+ be(0x0707)), /* 3F44: couple k factor 2 */
+};
+
+static int ar0521_power_off(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct ar0521_dev *sensor = to_ar0521_dev(sd);
+ int i;
+
+ dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+ clk_disable_unprepare(sensor->extclk);
+
+ if (sensor->reset_gpio)
+ gpiod_set_value(sensor->reset_gpio, 1); /* assert RESET signal */
+
+ for (i = ARRAY_SIZE(ar0521_supply_names) - 1; i >= 0; i--) {
+ if (sensor->supplies[i])
+ regulator_disable(sensor->supplies[i]);
+ }
+ return 0;
+}
+
+static int ar0521_power_on(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct ar0521_dev *sensor = to_ar0521_dev(sd);
+ unsigned int cnt;
+ int ret;
+
+ dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+ for (cnt = 0; cnt < ARRAY_SIZE(ar0521_supply_names); cnt++)
+ if (sensor->supplies[cnt]) {
+ ret = regulator_enable(sensor->supplies[cnt]);
+ if (ret < 0)
+ goto off;
+
+ usleep_range(1000, 1500); /* min 1 ms */
+ }
+
+ ret = clk_prepare_enable(sensor->extclk);
+ if (ret < 0) {
+ v4l2_err(&sensor->sd, "error enabling sensor clock\n");
+ goto off;
+ }
+ usleep_range(1000, 1500); /* min 1 ms */
+
+ if (sensor->reset_gpio)
+ /* deassert RESET signal */
+ gpiod_set_value(sensor->reset_gpio, 0);
+ usleep_range(4500, 5000); /* min 45000 clocks */
+
+ for (cnt = 0; cnt < ARRAY_SIZE(initial_regs); cnt++)
+ if (ar0521_write_regs(sensor, initial_regs[cnt].data,
+ initial_regs[cnt].count))
+ goto off;
+
+ ret = ar0521_write_reg(sensor, AR0521_REG_SERIAL_FORMAT,
+ AR0521_REG_SERIAL_FORMAT_MIPI |
+ sensor->lane_count);
+ if (ret)
+ goto off;
+
+ /* set MIPI test mode - disabled for now */
+ ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_TEST_MODE,
+ ((0x40 << sensor->lane_count) - 0x40) |
+ AR0521_REG_HISPI_TEST_MODE_LP11);
+ if (ret)
+ goto off;
+
+ ret = ar0521_write_reg(sensor, AR0521_REG_ROW_SPEED, 0x110 |
+ 4 / sensor->lane_count);
+ if (ret)
+ goto off;
+
+ return 0;
+off:
+ ar0521_power_off(dev);
+ return ret;
+}
+
+static int ar0521_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ struct ar0521_dev *sensor = to_ar0521_dev(sd);
+
+ if (code->index)
+ return -EINVAL;
+
+ code->code = sensor->fmt.code;
+ dev_dbg(&sensor->i2c_client->dev, "%s() = %X\n", __func__, code->code);
+ return 0;
+}
+
+static int ar0521_pre_streamon(struct v4l2_subdev *sd, u32 flags)
+{
+ struct ar0521_dev *sensor = to_ar0521_dev(sd);
+ int ret;
+
+ dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, flags);
+ if (!(flags & V4L2_SUBDEV_PRE_STREAMON_FL_MANUAL_LP))
+ return -EACCES;
+
+ ret = pm_runtime_resume_and_get(&sensor->i2c_client->dev);
+ if (ret < 0)
+ return ret;
+
+ /* Set LP-11 on clock and data lanes */
+ ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS,
+ AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE);
+ if (ret)
+ goto err;
+
+ /* Start streaming LP-11 */
+ ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
+ AR0521_REG_RESET_DEFAULTS |
+ AR0521_REG_RESET_STREAM);
+ if (ret)
+ goto err;
+ return 0;
+
+err:
+ pm_runtime_put(&sensor->i2c_client->dev);
+ return ret;
+}
+
+static int ar0521_post_streamoff(struct v4l2_subdev *sd)
+{
+ struct ar0521_dev *sensor = to_ar0521_dev(sd);
+
+ dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+
+ pm_runtime_put(&sensor->i2c_client->dev);
+ return 0;
+}
+
+static int ar0521_s_stream(struct v4l2_subdev *sd, int enable)
+{
+ struct ar0521_dev *sensor = to_ar0521_dev(sd);
+ int ret;
+
+ dev_dbg(&sensor->i2c_client->dev, "%s(%i)\n", __func__, enable);
+ mutex_lock(&sensor->lock);
+
+ ret = ar0521_set_stream(sensor, enable);
+ if (!ret)
+ sensor->streaming = enable;
+
+ mutex_unlock(&sensor->lock);
+ return ret;
+}
+
+static const struct v4l2_subdev_core_ops ar0521_core_ops = {
+ .log_status = v4l2_ctrl_subdev_log_status,
+};
+
+static const struct v4l2_subdev_video_ops ar0521_video_ops = {
+ .s_stream = ar0521_s_stream,
+ .pre_streamon = ar0521_pre_streamon,
+ .post_streamoff = ar0521_post_streamoff,
+};
+
+static const struct v4l2_subdev_pad_ops ar0521_pad_ops = {
+ .enum_mbus_code = ar0521_enum_mbus_code,
+ .get_fmt = ar0521_get_fmt,
+ .set_fmt = ar0521_set_fmt,
+};
+
+static const struct v4l2_subdev_ops ar0521_subdev_ops = {
+ .core = &ar0521_core_ops,
+ .video = &ar0521_video_ops,
+ .pad = &ar0521_pad_ops,
+};
+
+static int __maybe_unused ar0521_suspend(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct ar0521_dev *sensor = to_ar0521_dev(sd);
+
+ dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+ if (sensor->streaming)
+ ar0521_set_stream(sensor, 0);
+
+ return 0;
+}
+
+static int __maybe_unused ar0521_resume(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct ar0521_dev *sensor = to_ar0521_dev(sd);
+
+ dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+ if (sensor->streaming)
+ return ar0521_set_stream(sensor, 1);
+
+ return 0;
+}
+
+static int ar0521_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct v4l2_fwnode_endpoint ep = {
+ .bus_type = V4L2_MBUS_CSI2_DPHY
+ };
+ struct device *dev = &client->dev;
+ struct fwnode_handle *endpoint;
+ struct ar0521_dev *sensor;
+ unsigned int cnt;
+ int ret;
+
+ dev_dbg(dev, "%s()\n", __func__);
+ sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+ if (!sensor)
+ return -ENOMEM;
+
+ sensor->i2c_client = client;
+ sensor->fmt.width = AR0521_WIDTH_MAX;
+ sensor->fmt.height = AR0521_HEIGHT_MAX;
+ ar0521_adj_fmt(&sensor->fmt);
+
+ endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
+ FWNODE_GRAPH_ENDPOINT_NEXT);
+ if (!endpoint) {
+ dev_err(dev, "endpoint node not found\n");
+ return -EINVAL;
+ }
+
+ ret = v4l2_fwnode_endpoint_parse(endpoint, &ep);
+ fwnode_handle_put(endpoint);
+ if (ret) {
+ dev_err(dev, "could not parse endpoint\n");
+ return ret;
+ }
+
+ if (ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
+ dev_err(dev, "invalid bus type, must be MIPI CSI2\n");
+ return -EINVAL;
+ }
+
+ sensor->lane_count = ep.bus.mipi_csi2.num_data_lanes;
+ switch (sensor->lane_count) {
+ case 1:
+ case 2:
+ case 4:
+ break;
+ default:
+ dev_err(dev, "invalid number of MIPI data lanes\n");
+ return -EINVAL;
+ }
+
+ /* Get master clock (extclk) */
+ sensor->extclk = devm_clk_get(dev, "extclk");
+ if (IS_ERR(sensor->extclk)) {
+ dev_err(dev, "failed to get extclk\n");
+ return PTR_ERR(sensor->extclk);
+ }
+
+ sensor->extclk_freq = clk_get_rate(sensor->extclk);
+
+ if (sensor->extclk_freq < AR0521_EXTCLK_MIN ||
+ sensor->extclk_freq > AR0521_EXTCLK_MAX) {
+ dev_err(dev, "extclk frequency out of range: %u Hz\n",
+ sensor->extclk_freq);
+ return -EINVAL;
+ }
+
+ /* Request optional reset pin (usually active low) and assert it */
+ sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+ GPIOD_OUT_HIGH);
+
+ v4l2_i2c_subdev_init(&sensor->sd, client, &ar0521_subdev_ops);
+
+ sensor->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
+ sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
+ sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+ ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
+ if (ret)
+ return ret;
+
+ for (cnt = 0; cnt < ARRAY_SIZE(ar0521_supply_names); cnt++) {
+ struct regulator *supply = devm_regulator_get(dev,
+ ar0521_supply_names[cnt]);
+
+ if (IS_ERR(supply)) {
+ dev_info(dev, "no %s regulator found: %li\n",
+ ar0521_supply_names[cnt], PTR_ERR(supply));
+ return PTR_ERR(supply);
+ }
+ sensor->supplies[cnt] = supply;
+ }
+
+ mutex_init(&sensor->lock);
+
+ ret = ar0521_init_controls(sensor);
+ if (ret)
+ goto entity_cleanup;
+
+ ar0521_adj_fmt(&sensor->fmt);
+
+ ret = v4l2_async_register_subdev(&sensor->sd);
+ if (ret)
+ goto free_ctrls;
+
+ /* Turn on the device and enable runtime PM */
+ ret = ar0521_power_on(&client->dev);
+ if (ret)
+ goto disable;
+ pm_runtime_set_active(&client->dev);
+ pm_runtime_enable(&client->dev);
+ dev_dbg(dev, "AR0521 driver initialized, master clock frequency: %u Hz, %u MIPI data lanes\n",
+ sensor->extclk_freq, sensor->lane_count);
+ return 0;
+
+disable:
+ v4l2_async_unregister_subdev(&sensor->sd);
+ media_entity_cleanup(&sensor->sd.entity);
+free_ctrls:
+ v4l2_ctrl_handler_free(&sensor->ctrls.handler);
+entity_cleanup:
+ media_entity_cleanup(&sensor->sd.entity);
+ mutex_destroy(&sensor->lock);
+ return ret;
+}
+
+static int ar0521_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct ar0521_dev *sensor = to_ar0521_dev(sd);
+
+ v4l2_async_unregister_subdev(&sensor->sd);
+ media_entity_cleanup(&sensor->sd.entity);
+ v4l2_ctrl_handler_free(&sensor->ctrls.handler);
+ pm_runtime_disable(&client->dev);
+ if (!pm_runtime_status_suspended(&client->dev))
+ ar0521_power_off(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
+ mutex_destroy(&sensor->lock);
+ return 0;
+}
+
+static const struct dev_pm_ops ar0521_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(ar0521_suspend, ar0521_resume)
+ SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL)
+};
+static const struct of_device_id ar0521_dt_ids[] = {
+ {.compatible = "onnn,ar0521"},
+ {}
+};
+MODULE_DEVICE_TABLE(of, ar0521_dt_ids);
+
+static struct i2c_driver ar0521_i2c_driver = {
+ .driver = {
+ .name = "ar0521",
+ .pm = &ar0521_pm_ops,
+ .of_match_table = ar0521_dt_ids,
+ },
+ .probe = ar0521_probe,
+ .remove = ar0521_remove,
+};
+
+module_i2c_driver(ar0521_i2c_driver);
+
+MODULE_DESCRIPTION("AR0521 MIPI Camera subdev driver");
+MODULE_AUTHOR("Krzysztof Hałasa <khalasa@piap.pl>");
+MODULE_LICENSE("GPL v2");
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor
2022-01-03 13:36 ` [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor Krzysztof Hałasa
@ 2022-01-09 15:34 ` Jacopo Mondi
2022-01-09 19:01 ` Joe Perches
2022-02-25 12:15 ` Krzysztof Hałasa
0 siblings, 2 replies; 10+ messages in thread
From: Jacopo Mondi @ 2022-01-09 15:34 UTC (permalink / raw)
To: Krzysztof Hałasa
Cc: Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media,
linux-kernel, Laurent Pinchart, Sakari Ailus, Joe Perches
Hi Krzysztof
On Mon, Jan 03, 2022 at 02:36:18PM +0100, Krzysztof Hałasa wrote:
> The driver has been extensively tested in an i.MX6-based system.
> AR0521 is a 5.7 mm x 4.3 mm, 5 MPix RGGB MIPI/HiSPi BSI CMOS sensor
> from On Semiconductor.
>
> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 79fd8a012893..5d33f0277ad2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1383,6 +1383,13 @@ S: Supported
> W: http://www.aquantia.com
> F: drivers/net/ethernet/aquantia/atlantic/aq_ptp*
>
> +AR0521 ON SEMICONDUCTOR CAMERA SENSOR DRIVER
> +M: Krzysztof Hałasa <khalasa@piap.pl>
> +L: linux-media@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
> +F: drivers/media/i2c/ar0521.c
> +
> ARASAN NAND CONTROLLER DRIVER
> M: Miquel Raynal <miquel.raynal@bootlin.com>
> M: Naga Sureshkumar Relli <nagasure@xilinx.com>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 69c56e24a612..741ae073623e 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -733,6 +733,19 @@ config VIDEO_APTINA_PLL
> config VIDEO_CCS_PLL
> tristate
>
> +config VIDEO_AR0521
> + tristate "ON Semiconductor AR0521 sensor support"
> + depends on I2C && VIDEO_V4L2
> + select MEDIA_CONTROLLER
> + select VIDEO_V4L2_SUBDEV_API
> + select V4L2_FWNODE
> + help
> + This is a Video4Linux2 sensor driver for the ON Semiconductor
> + AR0521 camera.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ar0521.
> +
> config VIDEO_HI556
> tristate "Hynix Hi-556 sensor support"
> depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index b01f6cd05ee8..4e4986e16071 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -118,6 +118,7 @@ obj-$(CONFIG_VIDEO_I2C) += video-i2c.o
> obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o
> obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
> obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
> +obj-$(CONFIG_VIDEO_AR0521) += ar0521.o
> obj-$(CONFIG_VIDEO_HI556) += hi556.o
> obj-$(CONFIG_VIDEO_HI846) += hi846.o
> obj-$(CONFIG_VIDEO_IMX208) += imx208.o
> diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> new file mode 100644
> index 000000000000..0adbbd8ea3b5
> --- /dev/null
> +++ b/drivers/media/i2c/ar0521.c
> @@ -0,0 +1,1093 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Sieć Badawcza Łukasiewicz
> + * - Przemysłowy Instytut Automatyki i Pomiarów PIAP
> + * Written by Krzysztof Hałasa
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +/* External clock (extclk) frequencies */
> +#define AR0521_EXTCLK_MIN (10 * 1000 * 1000)
> +#define AR0521_EXTCLK_MAX (48 * 1000 * 1000)
> +
> +/* PLL and PLL2 */
> +#define AR0521_PLL_MIN (320 * 1000 * 1000)
> +#define AR0521_PLL_MAX (1280 * 1000 * 1000)
> +
> +/* Effective pixel clocks, the registers may be DDR */
> +#define AR0521_PIXEL_CLOCK_RATE (184 * 1000 * 1000)
> +#define AR0521_PIXEL_CLOCK_MIN (168 * 1000 * 1000)
> +#define AR0521_PIXEL_CLOCK_MAX (414 * 1000 * 1000)
> +
> +#define AR0521_WIDTH_MIN 8u
> +#define AR0521_WIDTH_MAX 2608u
> +#define AR0521_HEIGHT_MIN 8u
> +#define AR0521_HEIGHT_MAX 1958u
> +
> +#define AR0521_WIDTH_BLANKING_MIN 572u
> +#define AR0521_HEIGHT_BLANKING_MIN 38u /* must be even */
> +#define AR0521_TOTAL_WIDTH_MIN 2968u
> +
> +/* AR0521 registers */
> +#define AR0521_REG_VT_PIX_CLK_DIV 0x0300
> +#define AR0521_REG_FRAME_LENGTH_LINES 0x0340
> +
> +#define AR0521_REG_CHIP_ID 0x3000
> +#define AR0521_REG_COARSE_INTEGRATION_TIME 0x3012
> +#define AR0521_REG_ROW_SPEED 0x3016
> +#define AR0521_REG_EXTRA_DELAY 0x3018
> +#define AR0521_REG_RESET 0x301A
> +#define AR0521_REG_RESET_DEFAULTS 0x0238
> +#define AR0521_REG_RESET_GROUP_PARAM_HOLD 0x8000
> +#define AR0521_REG_RESET_STREAM BIT(2)
> +#define AR0521_REG_RESET_RESTART BIT(1)
> +#define AR0521_REG_RESET_INIT BIT(0)
> +
> +#define AR0521_REG_GREEN1_GAIN 0x3056
> +#define AR0521_REG_BLUE_GAIN 0x3058
> +#define AR0521_REG_RED_GAIN 0x305A
> +#define AR0521_REG_GREEN2_GAIN 0x305C
> +#define AR0521_REG_GLOBAL_GAIN 0x305E
> +
> +#define AR0521_REG_HISPI_TEST_MODE 0x3066
> +#define AR0521_REG_HISPI_TEST_MODE_LP11 0x0004
> +
> +#define AR0521_REG_TEST_PATTERN_MODE 0x3070
> +
> +#define AR0521_REG_SERIAL_FORMAT 0x31AE
> +#define AR0521_REG_SERIAL_FORMAT_MIPI 0x0200
> +
> +#define AR0521_REG_HISPI_CONTROL_STATUS 0x31C6
> +#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80
> +
> +#define be cpu_to_be16
> +
> +static const char * const ar0521_supply_names[] = {
> + "vdd_io", /* I/O (1.8V) supply */
> + "vdd", /* Core, PLL and MIPI (1.2V) supply */
> + "vaa", /* Analog (2.7V) supply */
> +};
> +
> +struct ar0521_ctrls {
> + struct v4l2_ctrl_handler handler;
> + struct {
> + struct v4l2_ctrl *gain;
> + struct v4l2_ctrl *red_balance;
> + struct v4l2_ctrl *blue_balance;
> + };
> + struct {
> + struct v4l2_ctrl *hblank;
> + struct v4l2_ctrl *vblank;
> + };
> + struct v4l2_ctrl *pixrate;
> + struct v4l2_ctrl *exposure;
> + struct v4l2_ctrl *test_pattern;
> +};
> +
> +struct ar0521_dev {
> + struct i2c_client *i2c_client;
> + struct v4l2_subdev sd;
> + struct media_pad pad;
> + struct clk *extclk;
> + u32 extclk_freq;
> +
> + struct regulator *supplies[ARRAY_SIZE(ar0521_supply_names)];
> + struct gpio_desc *reset_gpio;
> +
> + /* lock to protect all members below */
> + struct mutex lock;
> +
> + struct v4l2_mbus_framefmt fmt;
> + struct ar0521_ctrls ctrls;
> + unsigned int lane_count;
> + u16 total_width;
> + u16 total_height;
> + u16 pll_pre;
> + u16 pll_mult;
> + u16 pll_pre2;
> + u16 pll_mult2;
> + bool streaming;
> +};
> +
> +static inline struct ar0521_dev *to_ar0521_dev(struct v4l2_subdev *sd)
> +{
> + return container_of(sd, struct ar0521_dev, sd);
> +}
> +
> +static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> +{
> + return &container_of(ctrl->handler, struct ar0521_dev,
> + ctrls.handler)->sd;
> +}
> +
> +static u32 div64_round(u64 v, u32 d)
> +{
> + return div_u64(v + (d >> 1), d);
> +}
> +
> +static u32 div64_round_up(u64 v, u32 d)
> +{
> + return div_u64(v + d - 1, d);
> +}
> +
> +/* Data must be BE16, the first value is the register address */
> +static int ar0521_write_regs(struct ar0521_dev *sensor, const __be16 *data,
> + unsigned int count)
> +{
> + struct i2c_client *client = sensor->i2c_client;
> + struct i2c_msg msg;
> + int ret;
> +
> + msg.addr = client->addr;
> + msg.flags = client->flags;
> + msg.buf = (u8 *)data;
> + msg.len = count * sizeof(*data);
> +
> + ret = i2c_transfer(client->adapter, &msg, 1);
> +
> + if (ret < 0) {
> + v4l2_err(&sensor->sd, "%s: I2C write error\n", __func__);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ar0521_write_reg(struct ar0521_dev *sensor, u16 reg, u16 val)
> +{
> + __be16 buf[2] = {be(reg), be(val)};
> +
> + return ar0521_write_regs(sensor, buf, 2);
> +}
> +
> +static int ar0521_set_geometry(struct ar0521_dev *sensor)
> +{
> + /* All dimensions are unsigned 12-bit integers */
> + u16 x = (AR0521_WIDTH_MAX - sensor->fmt.width) / 2;
> + u16 y = ((AR0521_HEIGHT_MAX - sensor->fmt.height) / 2) & ~1;
> + __be16 regs[] = {
> + be(AR0521_REG_FRAME_LENGTH_LINES),
> + be(sensor->total_height),
> + be(sensor->total_width),
> + be(x),
> + be(y),
> + be(x + sensor->fmt.width - 1),
> + be(y + sensor->fmt.height - 1),
> + be(sensor->fmt.width),
> + be(sensor->fmt.height)
> + };
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +
> + return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
> +}
> +
> +static int ar0521_set_gains(struct ar0521_dev *sensor)
> +{
> + int green = sensor->ctrls.gain->val;
> + int red = max(green + sensor->ctrls.red_balance->val, 0);
> + int blue = max(green + sensor->ctrls.blue_balance->val, 0);
> + unsigned int gain = min(red, min(green, blue));
> + unsigned int analog = min(gain, 64u); /* range is 0 - 127 */
> + __be16 regs[5];
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +
> + red = min(red - analog + 64, 511u);
> + green = min(green - analog + 64, 511u);
> + blue = min(blue - analog + 64, 511u);
> + regs[0] = be(AR0521_REG_GREEN1_GAIN);
> + regs[1] = be(green << 7 | analog);
> + regs[2] = be(blue << 7 | analog);
> + regs[3] = be(red << 7 | analog);
> + regs[4] = be(green << 7 | analog);
> +
> + return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
> +}
> +
> +static u32 calc_pll(struct ar0521_dev *sensor, int num, u32 freq, u16 *pre_ptr,
> + u16 *mult_ptr)
> +{
> + u16 pre = 1, mult = 1, new_pre;
> + u32 pll = AR0521_PLL_MAX + 1;
> +
> + for (new_pre = 1; new_pre < 64; new_pre++) {
> + u32 new_pll;
> + u32 new_mult = div64_round_up((u64)freq * new_pre,
> + sensor->extclk_freq);
> +
> + if (new_mult < 32)
> + continue; /* Minimum value */
> + if (new_mult > 254)
> + break; /* Maximum, larger pre won't work either */
> + if (sensor->extclk_freq * (u64)new_mult < AR0521_PLL_MIN *
> + new_pre)
> + continue;
> + if (sensor->extclk_freq * (u64)new_mult > AR0521_PLL_MAX *
> + new_pre)
> + break; /* Larger pre won't work either */
> + new_pll = div64_round_up(sensor->extclk_freq * (u64)new_mult,
> + new_pre);
> + if (new_pll < pll) {
> + pll = new_pll;
> + pre = new_pre;
> + mult = new_mult;
> + }
> + }
> +
> + pll = div64_round(sensor->extclk_freq * (u64)mult, pre);
> + *pre_ptr = pre;
> + *mult_ptr = mult;
> + return pll;
> +}
> +
> +#define DIV 4
> +static void ar0521_calc_mode(struct ar0521_dev *sensor)
> +{
> + unsigned int speed_mod = 4 / sensor->lane_count; /* 1 with 4 DDR lanes */
> + u16 total_width = max(sensor->fmt.width + AR0521_WIDTH_BLANKING_MIN,
> + AR0521_TOTAL_WIDTH_MIN);
> + u16 total_height = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN;
> +
> + /* Calculate approximate pixel clock first */
> + u64 pix_clk = AR0521_PIXEL_CLOCK_RATE;
> +
> + /* PLL1 drives pixel clock - dual rate */
> + pix_clk = calc_pll(sensor, 1, pix_clk * (DIV / 2), &sensor->pll_pre,
> + &sensor->pll_mult);
> + pix_clk = div64_round(pix_clk, (DIV / 2));
> + calc_pll(sensor, 2, pix_clk * (DIV / 2) * speed_mod, &sensor->pll_pre2,
> + &sensor->pll_mult2);
> +
> + sensor->total_width = total_width;
> + sensor->total_height = total_height;
> +}
> +
> +static int ar0521_write_mode(struct ar0521_dev *sensor)
> +{
> + __be16 pll_regs[] = {
> + be(AR0521_REG_VT_PIX_CLK_DIV),
> + /* 0x300 */ be(4), /* vt_pix_clk_div = number of bits / 2 */
> + /* 0x302 */ be(1), /* vt_sys_clk_div */
> + /* 0x304 */ be((sensor->pll_pre2 << 8) | sensor->pll_pre),
> + /* 0x306 */ be((sensor->pll_mult2 << 8) | sensor->pll_mult),
> + /* 0x308 */ be(8), /* op_pix_clk_div = 2 * vt_pix_clk_div */
> + /* 0x30A */ be(1) /* op_sys_clk_div */
> + };
> + int ret;
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +
> + /* Stop streaming for just a moment */
> + ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
> + AR0521_REG_RESET_DEFAULTS);
> + if (ret)
> + return ret;
> +
> + ret = ar0521_set_geometry(sensor);
> + if (ret)
> + return ret;
> +
> + ret = ar0521_write_regs(sensor, pll_regs, ARRAY_SIZE(pll_regs));
> + if (ret)
> + return ret;
> +
> + ret = ar0521_write_reg(sensor, AR0521_REG_COARSE_INTEGRATION_TIME,
> + sensor->ctrls.exposure->val);
> + if (ret)
> + return ret;
> +
> + ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
> + AR0521_REG_RESET_DEFAULTS |
> + AR0521_REG_RESET_STREAM);
> + if (ret)
> + return ret;
> +
> + ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE,
> + sensor->ctrls.test_pattern->val);
> + if (ret)
> + return ret;
> +
> + dev_dbg(&sensor->i2c_client->dev,
> + "AR0521: %ux%u, total %ux%u\n",
> + sensor->fmt.width, sensor->fmt.height, sensor->total_width,
> + sensor->total_height);
> + return 0;
> +}
> +
> +static int ar0521_set_stream(struct ar0521_dev *sensor, bool on)
> +{
> + int ret;
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, on);
> +
> + if (on) {
> + ret = pm_runtime_resume_and_get(&sensor->i2c_client->dev);
> + if (ret < 0)
> + return ret;
> +
> + ar0521_calc_mode(sensor);
> + ret = ar0521_write_mode(sensor);
> + if (ret)
> + goto err;
> +
> + ret = ar0521_set_gains(sensor);
> + if (ret)
> + goto err;
> +
> + /* Exit LP-11 mode on clock and data lanes */
> + ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS,
> + 0);
> + if (ret)
> + goto err;
> +
> + /* Start streaming */
> + ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
> + AR0521_REG_RESET_DEFAULTS |
> + AR0521_REG_RESET_STREAM);
> + if (ret)
> + goto err;
> +
> + return 0;
> +
> +err:
> + pm_runtime_put(&sensor->i2c_client->dev);
> + return ret;
> +
> + } else {
> + /* Reset gain, the sensor may produce all white pixels without
> + this */
> + ret = ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, 0x2000);
> + if (ret)
> + return ret;
> +
> + /* Stop streaming */
> + ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
> + AR0521_REG_RESET_DEFAULTS);
> + if (ret)
> + return ret;
> +
> + pm_runtime_put(&sensor->i2c_client->dev);
> + return 0;
> + }
> +}
> +
> +static void ar0521_adj_fmt(struct v4l2_mbus_framefmt *fmt)
> +{
> + fmt->width = clamp(ALIGN(fmt->width, 4), AR0521_WIDTH_MIN,
> + AR0521_WIDTH_MAX);
> + fmt->height = clamp(ALIGN(fmt->height, 4), AR0521_HEIGHT_MIN,
> + AR0521_HEIGHT_MAX);
> + fmt->code = MEDIA_BUS_FMT_SGRBG8_1X8;
> + fmt->field = V4L2_FIELD_NONE;
> + fmt->colorspace = V4L2_COLORSPACE_SRGB;
> + fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> + fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +}
> +
> +static int ar0521_get_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *format)
> +{
> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> + struct v4l2_mbus_framefmt *fmt;
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, format->which);
> +
> + mutex_lock(&sensor->lock);
> +
> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> + fmt = v4l2_subdev_get_try_format(&sensor->sd, sd_state, 0
> + /* pad */);
> + else
> + fmt = &sensor->fmt;
> +
> + format->format = *fmt;
> +
> + mutex_unlock(&sensor->lock);
> + return 0;
> +}
> +
> +static int ar0521_set_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *format)
> +{
> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> + int ret = 0;
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, format->which);
> +
> + ar0521_adj_fmt(&format->format);
> +
> + mutex_lock(&sensor->lock);
> +
> + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> + struct v4l2_mbus_framefmt *fmt;
> +
> + fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
> + *fmt = format->format;
> + } else {
> + sensor->fmt = format->format;
> + ar0521_calc_mode(sensor);
> + }
> +
> + mutex_unlock(&sensor->lock);
> + return ret;
> +}
> +
> +static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> + int ret;
> +
> + /* v4l2_ctrl_lock() locks our own mutex */
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, ctrl->id);
> +
> + switch (ctrl->id) {
> + case V4L2_CID_HBLANK:
> + case V4L2_CID_VBLANK:
> + sensor->total_width = sensor->fmt.width +
> + sensor->ctrls.hblank->val;
> + sensor->total_height = sensor->fmt.width +
> + sensor->ctrls.vblank->val;
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + // access the sensor only if it's powered up
> + if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
As you correctly do not access the chip's registers if it's powered
off, you have to call __v4l2_ctrl_handler_setup() at power on time to
make sure controls are actually set.
> + return 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_HBLANK:
> + case V4L2_CID_VBLANK:
> + ret = ar0521_set_geometry(sensor);
> + break;
> + case V4L2_CID_GAIN:
> + case V4L2_CID_RED_BALANCE:
> + case V4L2_CID_BLUE_BALANCE:
> + ret = ar0521_set_gains(sensor);
> + break;
> + case V4L2_CID_EXPOSURE:
> + ret = ar0521_write_reg(sensor,
> + AR0521_REG_COARSE_INTEGRATION_TIME,
> + ctrl->val);
> + break;
> + case V4L2_CID_TEST_PATTERN:
> + ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE,
> + ctrl->val);
> + break;
> + }
> +
> + pm_runtime_put(&sensor->i2c_client->dev);
> + return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops ar0521_ctrl_ops = {
> + .s_ctrl = ar0521_s_ctrl,
> +};
> +
> +static const char * const test_pattern_menu[] = {
> + "Disabled",
> + "Solid color",
> + "Color bars",
> + "Faded color bars"
> +};
> +
> +static int ar0521_init_controls(struct ar0521_dev *sensor)
> +{
> + const struct v4l2_ctrl_ops *ops = &ar0521_ctrl_ops;
> + struct ar0521_ctrls *ctrls = &sensor->ctrls;
> + struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> + int ret;
> +
> + v4l2_ctrl_handler_init(hdl, 32);
> +
> + /* We can use our own mutex for the ctrl lock */
> + hdl->lock = &sensor->lock;
> +
> + /* Manual gain */
> + ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 511, 1, 0);
> + ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE,
> + -512, 511, 1, 0);
> + ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BLUE_BALANCE,
> + -512, 511, 1, 0);
> + v4l2_ctrl_cluster(3, &ctrls->gain);
> +
> + ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
> + AR0521_WIDTH_BLANKING_MIN, 4094, 1,
> + AR0521_WIDTH_BLANKING_MIN);
> + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
> + AR0521_HEIGHT_BLANKING_MIN, 4094, 2,
> + AR0521_HEIGHT_BLANKING_MIN);
> + v4l2_ctrl_cluster(2, &ctrls->hblank);
Is it intended to have vblank and hblank as cluster, can't they change
independently ?
> +
> + /* Read-only */
> + ctrls->pixrate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
> + AR0521_PIXEL_CLOCK_MIN,
> + AR0521_PIXEL_CLOCK_MAX, 1,
> + AR0521_PIXEL_CLOCK_RATE);
so you have to mark it as read-only
ctrl->pixelrate->flags = V4L2_CTRL_FLAG_READ_ONLY;
> +
> + /* Manual exposure time */
> + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
> + 65535, 1, 360);
> +
> + ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(test_pattern_menu) - 1,
> + 0, 0, test_pattern_menu);
> +
> + if (hdl->error) {
> + ret = hdl->error;
> + goto free_ctrls;
> + }
> +
> + sensor->sd.ctrl_handler = hdl;
> + return 0;
> +
> +free_ctrls:
> + v4l2_ctrl_handler_free(hdl);
> + return ret;
> +}
> +
> +#define REGS_ENTRY(a) {(a), ARRAY_SIZE(a)}
> +#define REGS(...) REGS_ENTRY(((const __be16[]){__VA_ARGS__}))
> +
> +static const struct initial_reg {
> + const __be16 *data; /* data[0] is register address */
> + unsigned int count;
> +} initial_regs[] = {
> + REGS(be(0x0112), be(0x0808)), /* 8-bit/8-bit mode */
> +
> + /* PEDESTAL+2 :+2 is a workaround for 10bit mode +0.5 rounding */
> + REGS(be(0x301E), be(0x00AA)),
> +
> + /* corrections_recommended_bayer */
> + REGS(be(0x3042),
> + be(0x0004), /* 3042: RNC: enable b/w rnc mode */
> + be(0x4580)), /* 3044: RNC: enable row noise correction */
> +
> + REGS(be(0x30D2),
> + be(0x0000), /* 30D2: CRM/CC: enable crm on Visible and CC rows */
> + be(0x0000), /* 30D4: CC: CC enabled with 16 samples per column */
> + /* 30D6: CC: bw mode enabled/12 bit data resolution/bw mode */
> + be(0x2FFF)),
> +
> + REGS(be(0x30DA),
> + be(0x0FFF), /* 30DA: CC: column correction clip level 2 is 0 */
> + be(0x0FFF), /* 30DC: CC: column correction clip level 3 is 0 */
> + be(0x0000)), /* 30DE: CC: Group FPN correction */
> +
> + /* RNC: rnc scaling factor = * 54 / 64 (32 / 38 * 64 = 53.9) */
> + REGS(be(0x30EE), be(0x1136)),
> + REGS(be(0x30FA), be(0xFD00)), /* GPIO0 = flash, GPIO1 = shutter */
> + REGS(be(0x3120), be(0x0005)), /* p1 dither enabled for 10bit mode */
> + REGS(be(0x3172), be(0x0206)), /* txlo clk divider options */
> + /* FDOC:fdoc settings with fdoc every frame turned of */
> + REGS(be(0x3180), be(0x9434)),
> +
> + REGS(be(0x31B0),
> + be(0x008B), /* 31B0: frame_preamble - FIXME check WRT lanes# */
> + be(0x0050)), /* 31B2: line_preamble - FIXME check WRT lanes# */
> +
> + /* don't use continuous clock mode while shut down */
> + REGS(be(0x31BC), be(0x068C)),
> + REGS(be(0x31E0), be(0x0781)), /* Fuse/2DDC: enable 2ddc */
> +
> + /* analog_setup_recommended_10bit */
> + REGS(be(0x341A), be(0x4735)), /* Samp&Hold pulse in ADC */
> + REGS(be(0x3420), be(0x4735)), /* Samp&Hold pulse in ADC */
> + REGS(be(0x3426), be(0x8A1A)), /* ADC offset distribution pulse */
> + REGS(be(0x342A), be(0x0018)), /* pulse_config */
> +
> + /* pixel_timing_recommended */
> + REGS(be(0x3D00),
> + /* 3D00 */ be(0x043E), be(0x4760), be(0xFFFF), be(0xFFFF),
> + /* 3D08 */ be(0x8000), be(0x0510), be(0xAF08), be(0x0252),
> + /* 3D10 */ be(0x486F), be(0x5D5D), be(0x8056), be(0x8313),
> + /* 3D18 */ be(0x0087), be(0x6A48), be(0x6982), be(0x0280),
> + /* 3D20 */ be(0x8359), be(0x8D02), be(0x8020), be(0x4882),
> + /* 3D28 */ be(0x4269), be(0x6A95), be(0x5988), be(0x5A83),
> + /* 3D30 */ be(0x5885), be(0x6280), be(0x6289), be(0x6097),
> + /* 3D38 */ be(0x5782), be(0x605C), be(0xBF18), be(0x0961),
> + /* 3D40 */ be(0x5080), be(0x2090), be(0x4390), be(0x4382),
> + /* 3D48 */ be(0x5F8A), be(0x5D5D), be(0x9C63), be(0x8063),
> + /* 3D50 */ be(0xA960), be(0x9757), be(0x8260), be(0x5CFF),
> + /* 3D58 */ be(0xBF10), be(0x1681), be(0x0802), be(0x8000),
> + /* 3D60 */ be(0x141C), be(0x6000), be(0x6022), be(0x4D80),
> + /* 3D68 */ be(0x5C97), be(0x6A69), be(0xAC6F), be(0x4645),
> + /* 3D70 */ be(0x4400), be(0x0513), be(0x8069), be(0x6AC6),
> + /* 3D78 */ be(0x5F95), be(0x5F70), be(0x8040), be(0x4A81),
> + /* 3D80 */ be(0x0300), be(0xE703), be(0x0088), be(0x4A83),
> + /* 3D88 */ be(0x40FF), be(0xFFFF), be(0xFD70), be(0x8040),
> + /* 3D90 */ be(0x4A85), be(0x4FA8), be(0x4F8C), be(0x0070),
> + /* 3D98 */ be(0xBE47), be(0x8847), be(0xBC78), be(0x6B89),
> + /* 3DA0 */ be(0x6A80), be(0x6986), be(0x6B8E), be(0x6B80),
> + /* 3DA8 */ be(0x6980), be(0x6A88), be(0x7C9F), be(0x866B),
> + /* 3DB0 */ be(0x8765), be(0x46FF), be(0xE365), be(0xA679),
> + /* 3DB8 */ be(0x4A40), be(0x4580), be(0x44BC), be(0x7000),
> + /* 3DC0 */ be(0x8040), be(0x0802), be(0x10EF), be(0x0104),
> + /* 3DC8 */ be(0x3860), be(0x5D5D), be(0x5682), be(0x1300),
> + /* 3DD0 */ be(0x8648), be(0x8202), be(0x8082), be(0x598A),
> + /* 3DD8 */ be(0x0280), be(0x2048), be(0x3060), be(0x8042),
> + /* 3DE0 */ be(0x9259), be(0x865A), be(0x8258), be(0x8562),
> + /* 3DE8 */ be(0x8062), be(0x8560), be(0x9257), be(0x8221),
> + /* 3DF0 */ be(0x10FF), be(0xB757), be(0x9361), be(0x1019),
> + /* 3DF8 */ be(0x8020), be(0x9043), be(0x8E43), be(0x845F),
> + /* 3E00 */ be(0x835D), be(0x805D), be(0x8163), be(0x8063),
> + /* 3E08 */ be(0xA060), be(0x9157), be(0x8260), be(0x5CFF),
> + /* 3E10 */ be(0xFFFF), be(0xFFE5), be(0x1016), be(0x2048),
> + /* 3E18 */ be(0x0802), be(0x1C60), be(0x0014), be(0x0060),
> + /* 3E20 */ be(0x2205), be(0x8120), be(0x908F), be(0x6A80),
> + /* 3E28 */ be(0x6982), be(0x5F9F), be(0x6F46), be(0x4544),
> + /* 3E30 */ be(0x0005), be(0x8013), be(0x8069), be(0x6A80),
> + /* 3E38 */ be(0x7000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3E40 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3E48 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3E50 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3E58 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3E60 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3E68 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3E70 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3E78 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3E80 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3E88 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3E90 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3E98 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3EA0 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3EA8 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> + /* 3EB0 */ be(0x0000), be(0x0000), be(0x0000)),
> +
> + REGS(be(0x3EB6), be(0x004C)), /* ECL */
> +
> + REGS(be(0x3EBA),
> + be(0xAAAD), /* 3EBA */
> + be(0x0086)), /* 3EBC: Bias currents for FSC/ECL */
> +
> + REGS(be(0x3EC0),
> + be(0x1E00), /* 3EC0: SFbin/SH mode settings */
> + be(0x100A), /* 3EC2: CLK divider for ramp for 10 bit 400MH */
> + /* 3EC4: FSC clamps for HDR mode and adc comp power down co */
> + be(0x3300),
> + be(0xEA44), /* 3EC6: VLN and clk gating controls */
> + be(0x6F6F), /* 3EC8: Txl0 and Txlo1 settings for normal mode */
> + be(0x2F4A), /* 3ECA: CDAC/Txlo2/RSTGHI/RSTGLO settings */
> + be(0x0506), /* 3ECC: RSTDHI/RSTDLO/CDAC/TXHI settings */
> + /* 3ECE: Ramp buffer settings and Booster enable (bits 0-5) */
> + be(0x203B),
> + be(0x13F0), /* 3ED0: TXLO from atest/sf bin settings */
> + be(0xA53D), /* 3ED2: Ramp offset */
> + be(0x862F), /* 3ED4: TXLO open loop/row driver settings */
> + be(0x4081), /* 3ED6: Txlatch fr cfpn rows/vln bias */
> + be(0x8003), /* 3ED8: Ramp step setting for 10 bit 400 Mhz */
> + be(0xA580), /* 3EDA: Ramp Offset */
> + be(0xC000), /* 3EDC: over range for rst and under range for sig */
> + be(0xC103)), /* 3EDE: over range for sig and col dec clk settings */
> +
> + /* corrections_recommended_bayer */
> + REGS(be(0x3F00),
> + be(0x0017), /* 3F00: BM_T0 */
> + be(0x02DD), /* 3F02: BM_T1 */
> + /* 3F04: if Ana_gain less than 2, use noise_floor0, multipl */
> + be(0x0020),
> + /* 3F06: if Ana_gain between 4 and 7, use noise_floor2 and */
> + be(0x0040),
> + /* 3F08: if Ana_gain between 4 and 7, use noise_floor2 and */
> + be(0x0070),
> + /* 3F0A: Define noise_floor0(low address) and noise_floor1 */
> + be(0x0101),
> + be(0x0302)), /* 3F0C: Define noise_floor2 and noise_floor3 */
> +
> + REGS(be(0x3F10),
> + be(0x0505), /* 3F10: single k factor 0 */
> + be(0x0505), /* 3F12: single k factor 1 */
> + be(0x0505), /* 3F14: single k factor 2 */
> + be(0x01FF), /* 3F16: cross factor 0 */
> + be(0x01FF), /* 3F18: cross factor 1 */
> + be(0x01FF), /* 3F1A: cross factor 2 */
> + be(0x0022)), /* 3F1E */
> +
> + /* GTH_THRES_RTN: 4max,4min filtered out of every 46 samples and */
> + REGS(be(0x3F2C), be(0x442E)),
> +
> + REGS(be(0x3F3E),
> + be(0x0000), /* 3F3E: Switch ADC from 12 bit to 10 bit mode */
> + be(0x1511), /* 3F40: couple k factor 0 */
> + be(0x1511), /* 3F42: couple k factor 1 */
> + be(0x0707)), /* 3F44: couple k factor 2 */
> +};
> +
> +static int ar0521_power_off(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> + int i;
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> + clk_disable_unprepare(sensor->extclk);
> +
> + if (sensor->reset_gpio)
> + gpiod_set_value(sensor->reset_gpio, 1); /* assert RESET signal */
> +
> + for (i = ARRAY_SIZE(ar0521_supply_names) - 1; i >= 0; i--) {
> + if (sensor->supplies[i])
> + regulator_disable(sensor->supplies[i]);
> + }
> + return 0;
> +}
> +
> +static int ar0521_power_on(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> + unsigned int cnt;
> + int ret;
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> + for (cnt = 0; cnt < ARRAY_SIZE(ar0521_supply_names); cnt++)
> + if (sensor->supplies[cnt]) {
> + ret = regulator_enable(sensor->supplies[cnt]);
> + if (ret < 0)
> + goto off;
> +
> + usleep_range(1000, 1500); /* min 1 ms */
> + }
> +
> + ret = clk_prepare_enable(sensor->extclk);
> + if (ret < 0) {
> + v4l2_err(&sensor->sd, "error enabling sensor clock\n");
> + goto off;
> + }
> + usleep_range(1000, 1500); /* min 1 ms */
> +
> + if (sensor->reset_gpio)
> + /* deassert RESET signal */
> + gpiod_set_value(sensor->reset_gpio, 0);
> + usleep_range(4500, 5000); /* min 45000 clocks */
> +
> + for (cnt = 0; cnt < ARRAY_SIZE(initial_regs); cnt++)
> + if (ar0521_write_regs(sensor, initial_regs[cnt].data,
> + initial_regs[cnt].count))
> + goto off;
> +
> + ret = ar0521_write_reg(sensor, AR0521_REG_SERIAL_FORMAT,
> + AR0521_REG_SERIAL_FORMAT_MIPI |
> + sensor->lane_count);
> + if (ret)
> + goto off;
> +
> + /* set MIPI test mode - disabled for now */
> + ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_TEST_MODE,
> + ((0x40 << sensor->lane_count) - 0x40) |
> + AR0521_REG_HISPI_TEST_MODE_LP11);
> + if (ret)
> + goto off;
> +
> + ret = ar0521_write_reg(sensor, AR0521_REG_ROW_SPEED, 0x110 |
> + 4 / sensor->lane_count);
> + if (ret)
> + goto off;
> +
> + return 0;
> +off:
> + ar0521_power_off(dev);
> + return ret;
> +}
> +
> +static int ar0521_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> + if (code->index)
> + return -EINVAL;
> +
> + code->code = sensor->fmt.code;
> + dev_dbg(&sensor->i2c_client->dev, "%s() = %X\n", __func__, code->code);
> + return 0;
> +}
> +
> +static int ar0521_pre_streamon(struct v4l2_subdev *sd, u32 flags)
> +{
> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> + int ret;
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, flags);
> + if (!(flags & V4L2_SUBDEV_PRE_STREAMON_FL_MANUAL_LP))
> + return -EACCES;
> +
> + ret = pm_runtime_resume_and_get(&sensor->i2c_client->dev);
> + if (ret < 0)
> + return ret;
> +
> + /* Set LP-11 on clock and data lanes */
> + ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS,
> + AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE);
> + if (ret)
> + goto err;
> +
> + /* Start streaming LP-11 */
> + ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
> + AR0521_REG_RESET_DEFAULTS |
> + AR0521_REG_RESET_STREAM);
> + if (ret)
> + goto err;
> + return 0;
> +
> +err:
> + pm_runtime_put(&sensor->i2c_client->dev);
> + return ret;
> +}
> +
> +static int ar0521_post_streamoff(struct v4l2_subdev *sd)
> +{
> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +
> + pm_runtime_put(&sensor->i2c_client->dev);
> + return 0;
> +}
> +
> +static int ar0521_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> + int ret;
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s(%i)\n", __func__, enable);
> + mutex_lock(&sensor->lock);
> +
> + ret = ar0521_set_stream(sensor, enable);
> + if (!ret)
> + sensor->streaming = enable;
> +
> + mutex_unlock(&sensor->lock);
> + return ret;
> +}
> +
> +static const struct v4l2_subdev_core_ops ar0521_core_ops = {
> + .log_status = v4l2_ctrl_subdev_log_status,
> +};
> +
> +static const struct v4l2_subdev_video_ops ar0521_video_ops = {
> + .s_stream = ar0521_s_stream,
> + .pre_streamon = ar0521_pre_streamon,
> + .post_streamoff = ar0521_post_streamoff,
> +};
> +
> +static const struct v4l2_subdev_pad_ops ar0521_pad_ops = {
> + .enum_mbus_code = ar0521_enum_mbus_code,
> + .get_fmt = ar0521_get_fmt,
> + .set_fmt = ar0521_set_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops ar0521_subdev_ops = {
> + .core = &ar0521_core_ops,
> + .video = &ar0521_video_ops,
> + .pad = &ar0521_pad_ops,
> +};
> +
> +static int __maybe_unused ar0521_suspend(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> + if (sensor->streaming)
> + ar0521_set_stream(sensor, 0);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused ar0521_resume(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> + dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> + if (sensor->streaming)
> + return ar0521_set_stream(sensor, 1);
> +
> + return 0;
> +}
> +
> +static int ar0521_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct v4l2_fwnode_endpoint ep = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY
> + };
> + struct device *dev = &client->dev;
> + struct fwnode_handle *endpoint;
> + struct ar0521_dev *sensor;
> + unsigned int cnt;
> + int ret;
> +
> + dev_dbg(dev, "%s()\n", __func__);
Rather useless, please drop. Same for all other debug functions that
just print the current function name in other parts of the driver.
While maybe useful when developing, they're mostly noise for users
imo.
> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> + if (!sensor)
> + return -ENOMEM;
> +
> + sensor->i2c_client = client;
> + sensor->fmt.width = AR0521_WIDTH_MAX;
> + sensor->fmt.height = AR0521_HEIGHT_MAX;
> + ar0521_adj_fmt(&sensor->fmt);
> +
> + endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> + FWNODE_GRAPH_ENDPOINT_NEXT);
The drivers supports a single endpoint right ? Then
fwnode_graph_get_next_enpoint() can be used
> + if (!endpoint) {
> + dev_err(dev, "endpoint node not found\n");
> + return -EINVAL;
> + }
> +
> + ret = v4l2_fwnode_endpoint_parse(endpoint, &ep);
> + fwnode_handle_put(endpoint);
> + if (ret) {
> + dev_err(dev, "could not parse endpoint\n");
> + return ret;
> + }
> +
> + if (ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
> + dev_err(dev, "invalid bus type, must be MIPI CSI2\n");
> + return -EINVAL;
> + }
> +
> + sensor->lane_count = ep.bus.mipi_csi2.num_data_lanes;
> + switch (sensor->lane_count) {
> + case 1:
> + case 2:
> + case 4:
> + break;
> + default:
> + dev_err(dev, "invalid number of MIPI data lanes\n");
> + return -EINVAL;
> + }
> +
> + /* Get master clock (extclk) */
> + sensor->extclk = devm_clk_get(dev, "extclk");
> + if (IS_ERR(sensor->extclk)) {
> + dev_err(dev, "failed to get extclk\n");
> + return PTR_ERR(sensor->extclk);
> + }
> +
> + sensor->extclk_freq = clk_get_rate(sensor->extclk);
> +
> + if (sensor->extclk_freq < AR0521_EXTCLK_MIN ||
> + sensor->extclk_freq > AR0521_EXTCLK_MAX) {
> + dev_err(dev, "extclk frequency out of range: %u Hz\n",
> + sensor->extclk_freq);
> + return -EINVAL;
> + }
> +
> + /* Request optional reset pin (usually active low) and assert it */
> + sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> + GPIOD_OUT_HIGH);
> +
> + v4l2_i2c_subdev_init(&sensor->sd, client, &ar0521_subdev_ops);
> +
> + sensor->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> + sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> + sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> + ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> + if (ret)
> + return ret;
> +
> + for (cnt = 0; cnt < ARRAY_SIZE(ar0521_supply_names); cnt++) {
> + struct regulator *supply = devm_regulator_get(dev,
> + ar0521_supply_names[cnt]);
> +
> + if (IS_ERR(supply)) {
> + dev_info(dev, "no %s regulator found: %li\n",
> + ar0521_supply_names[cnt], PTR_ERR(supply));
> + return PTR_ERR(supply);
> + }
> + sensor->supplies[cnt] = supply;
> + }
Using the regulator bulk api would simplify this.
See drivers/media/i2c/ccs/ccs-core.c
> +
> + mutex_init(&sensor->lock);
> +
> + ret = ar0521_init_controls(sensor);
> + if (ret)
> + goto entity_cleanup;
> +
> + ar0521_adj_fmt(&sensor->fmt);
Called already here above. Can anything in here change the format ?
> +
> + ret = v4l2_async_register_subdev(&sensor->sd);
> + if (ret)
> + goto free_ctrls;
> +
> + /* Turn on the device and enable runtime PM */
> + ret = ar0521_power_on(&client->dev);
> + if (ret)
> + goto disable;
Does the device stay powered all the time ?
Doesn't your resume callback call power_on() already ?
> + pm_runtime_set_active(&client->dev);
> + pm_runtime_enable(&client->dev);
> + dev_dbg(dev, "AR0521 driver initialized, master clock frequency: %u Hz, %u MIPI data lanes\n",
> + sensor->extclk_freq, sensor->lane_count);
> + return 0;
> +
> +disable:
> + v4l2_async_unregister_subdev(&sensor->sd);
> + media_entity_cleanup(&sensor->sd.entity);
> +free_ctrls:
> + v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> +entity_cleanup:
> + media_entity_cleanup(&sensor->sd.entity);
> + mutex_destroy(&sensor->lock);
> + return ret;
> +}
> +
> +static int ar0521_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> + v4l2_async_unregister_subdev(&sensor->sd);
> + media_entity_cleanup(&sensor->sd.entity);
> + v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> + pm_runtime_disable(&client->dev);
> + if (!pm_runtime_status_suspended(&client->dev))
> + ar0521_power_off(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
> + mutex_destroy(&sensor->lock);
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ar0521_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(ar0521_suspend, ar0521_resume)
> + SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL)
> +};
> +static const struct of_device_id ar0521_dt_ids[] = {
> + {.compatible = "onnn,ar0521"},
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ar0521_dt_ids);
> +
> +static struct i2c_driver ar0521_i2c_driver = {
> + .driver = {
> + .name = "ar0521",
> + .pm = &ar0521_pm_ops,
> + .of_match_table = ar0521_dt_ids,
> + },
> + .probe = ar0521_probe,
You could use probe_new and drop the "const struct i2c_device_id *id"
argument to probe()
Thanks
j
> + .remove = ar0521_remove,
> +};
> +
> +module_i2c_driver(ar0521_i2c_driver);
> +
> +MODULE_DESCRIPTION("AR0521 MIPI Camera subdev driver");
> +MODULE_AUTHOR("Krzysztof Hałasa <khalasa@piap.pl>");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor
2022-01-09 15:34 ` Jacopo Mondi
@ 2022-01-09 19:01 ` Joe Perches
2022-02-25 12:15 ` Krzysztof Hałasa
1 sibling, 0 replies; 10+ messages in thread
From: Joe Perches @ 2022-01-09 19:01 UTC (permalink / raw)
To: Jacopo Mondi, Krzysztof Hałasa
Cc: Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media,
linux-kernel, Laurent Pinchart, Sakari Ailus
On Sun, 2022-01-09 at 16:34 +0100, Jacopo Mondi wrote:
> Hi Krzysztof
Hi Jacopo
Can you please trim your replies to just the sections that
you are commenting on?
Finding the reply in the duplicated reply content isn't easy.
> > + // access the sensor only if it's powered up
> > + if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
>
> As you correctly do not access the chip's registers if it's powered
> off, you have to call __v4l2_ctrl_handler_setup() at power on time to
> make sure controls are actually set.
It was 25 pages down in the reply to find this.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor
2022-01-09 15:34 ` Jacopo Mondi
2022-01-09 19:01 ` Joe Perches
@ 2022-02-25 12:15 ` Krzysztof Hałasa
2022-02-25 13:18 ` Sakari Ailus
2022-03-01 9:01 ` Jacopo Mondi
1 sibling, 2 replies; 10+ messages in thread
From: Krzysztof Hałasa @ 2022-02-25 12:15 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media,
linux-kernel, Laurent Pinchart, Sakari Ailus, Joe Perches
Hi Jacopo,
Sorry it took that long. Unfortunately I no longer have much time to
work on this, thus the delays.
Jacopo Mondi <jacopo@jmondi.org> writes:
>> +static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
>> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
>> + int ret;
>> +
>> + /* v4l2_ctrl_lock() locks our own mutex */
>> +
>> + dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, ctrl->id);
>> +
>> + switch (ctrl->id) {
>> + case V4L2_CID_HBLANK:
>> + case V4L2_CID_VBLANK:
>> + sensor->total_width = sensor->fmt.width +
>> + sensor->ctrls.hblank->val;
>> + sensor->total_height = sensor->fmt.width +
>> + sensor->ctrls.vblank->val;
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + // access the sensor only if it's powered up
>> + if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
>
> As you correctly do not access the chip's registers if it's powered
> off, you have to call __v4l2_ctrl_handler_setup() at power on time to
> make sure controls are actually set.
These registers are also written in ar0521_set_stream(), isn't it
enough?
>> + ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
>> + AR0521_WIDTH_BLANKING_MIN, 4094, 1,
>> + AR0521_WIDTH_BLANKING_MIN);
>> + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
>> + AR0521_HEIGHT_BLANKING_MIN, 4094, 2,
>> + AR0521_HEIGHT_BLANKING_MIN);
>> + v4l2_ctrl_cluster(2, &ctrls->hblank);
>
> Is it intended to have vblank and hblank as cluster, can't they change
> independently ?
They can. It appears, though, that clusters aren't about controls which
can't change independently. Both of the two are written to the hardware
at the same time, which appears to be a valid reason to combine them
into a cluster.
This is similar to the gain/balance controls, and BTW the use of
clusters there was suggested by you.
Please correct me if I'm wrong.
>> + /* Read-only */
>> + ctrls->pixrate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
>> + AR0521_PIXEL_CLOCK_MIN,
>> + AR0521_PIXEL_CLOCK_MAX, 1,
>> + AR0521_PIXEL_CLOCK_RATE);
>
> so you have to mark it as read-only
Oh, I'd be happy for this control to be R/W. Unfortunately the core
media core enforces R/O. This is only a comment about what the core code
does, currently at least.
>> + dev_dbg(dev, "%s()\n", __func__);
>
> Rather useless, please drop. Same for all other debug functions that
> just print the current function name in other parts of the driver.
> While maybe useful when developing, they're mostly noise for users
> imo.
Fine, will drop the rest of the debug. In fact, I already dropped the
most useful parts (I2C register access debugging).
>> + endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
>> + FWNODE_GRAPH_ENDPOINT_NEXT);
>
> The drivers supports a single endpoint right ? Then
> fwnode_graph_get_next_enpoint() can be used
Well, I originally used
fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), NULL).
Sakari suggested I should use the above
fwnode_graph_get_endpoint_by_id().
It could be a good idea to agree on this. Not sure about the difference.
>> + for (cnt = 0; cnt < ARRAY_SIZE(ar0521_supply_names); cnt++) {
>> + struct regulator *supply = devm_regulator_get(dev,
>> + ar0521_supply_names[cnt]);
>> +
>> + if (IS_ERR(supply)) {
>> + dev_info(dev, "no %s regulator found: %li\n",
>> + ar0521_supply_names[cnt], PTR_ERR(supply));
>> + return PTR_ERR(supply);
>> + }
>> + sensor->supplies[cnt] = supply;
>> + }
>
> Using the regulator bulk api would simplify this.
> See drivers/media/i2c/ccs/ccs-core.c
The bulk API doesn't allow for delays between individual supplies, does it?
The delays are mandated by the manufacturer.
>> +
>> + mutex_init(&sensor->lock);
>> +
>> + ret = ar0521_init_controls(sensor);
>> + if (ret)
>> + goto entity_cleanup;
>> +
>> + ar0521_adj_fmt(&sensor->fmt);
>
> Called already here above.
Right, I will remove the first one.
>> + ret = v4l2_async_register_subdev(&sensor->sd);
>> + if (ret)
>> + goto free_ctrls;
>> +
>> + /* Turn on the device and enable runtime PM */
>> + ret = ar0521_power_on(&client->dev);
>> + if (ret)
>> + goto disable;
>
> Does the device stay powered all the time ?
Depends on the hw, but the power could be disabled, yes.
> Doesn't your resume callback call power_on() already ?
It does, when the PM is enabled.
Should the above code be changed?
>> +static struct i2c_driver ar0521_i2c_driver = {
>> + .driver = {
>> + .name = "ar0521",
>> + .pm = &ar0521_pm_ops,
>> + .of_match_table = ar0521_dt_ids,
>> + },
>> + .probe = ar0521_probe,
>
> You could use probe_new and drop the "const struct i2c_device_id *id"
> argument to probe()
You are right again.
That said, I wonder if it would be better (like easier) to have this
driver added to the staging area instead.
--
Krzysztof "Chris" Hałasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor
2022-02-25 12:15 ` Krzysztof Hałasa
@ 2022-02-25 13:18 ` Sakari Ailus
2022-02-28 7:48 ` Krzysztof Hałasa
2022-03-01 9:01 ` Jacopo Mondi
1 sibling, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2022-02-25 13:18 UTC (permalink / raw)
To: Krzysztof Hałasa
Cc: Jacopo Mondi, Mauro Carvalho Chehab, Rob Herring, devicetree,
linux-media, linux-kernel, Laurent Pinchart, Joe Perches
Hi Krysztof,
On Fri, Feb 25, 2022 at 01:15:11PM +0100, Krzysztof Hałasa wrote:
> Hi Jacopo,
>
> Sorry it took that long. Unfortunately I no longer have much time to
> work on this, thus the delays.
>
> Jacopo Mondi <jacopo@jmondi.org> writes:
>
> >> +static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
> >> +{
> >> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> >> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> >> + int ret;
> >> +
> >> + /* v4l2_ctrl_lock() locks our own mutex */
> >> +
> >> + dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, ctrl->id);
> >> +
> >> + switch (ctrl->id) {
> >> + case V4L2_CID_HBLANK:
> >> + case V4L2_CID_VBLANK:
> >> + sensor->total_width = sensor->fmt.width +
> >> + sensor->ctrls.hblank->val;
> >> + sensor->total_height = sensor->fmt.width +
> >> + sensor->ctrls.vblank->val;
> >> + break;
> >> + default:
> >> + ret = -EINVAL;
> >> + break;
> >> + }
> >> +
> >> + // access the sensor only if it's powered up
/* This is the preferred comment style */
> >> + if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
> >
> > As you correctly do not access the chip's registers if it's powered
> > off, you have to call __v4l2_ctrl_handler_setup() at power on time to
> > make sure controls are actually set.
>
> These registers are also written in ar0521_set_stream(), isn't it
> enough?
>
> >> + ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
> >> + AR0521_WIDTH_BLANKING_MIN, 4094, 1,
> >> + AR0521_WIDTH_BLANKING_MIN);
> >> + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
> >> + AR0521_HEIGHT_BLANKING_MIN, 4094, 2,
> >> + AR0521_HEIGHT_BLANKING_MIN);
> >> + v4l2_ctrl_cluster(2, &ctrls->hblank);
> >
> > Is it intended to have vblank and hblank as cluster, can't they change
> > independently ?
>
> They can. It appears, though, that clusters aren't about controls which
> can't change independently. Both of the two are written to the hardware
> at the same time, which appears to be a valid reason to combine them
> into a cluster.
> This is similar to the gain/balance controls, and BTW the use of
> clusters there was suggested by you.
>
> Please correct me if I'm wrong.
>
> >> + /* Read-only */
> >> + ctrls->pixrate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
> >> + AR0521_PIXEL_CLOCK_MIN,
> >> + AR0521_PIXEL_CLOCK_MAX, 1,
> >> + AR0521_PIXEL_CLOCK_RATE);
> >
> > so you have to mark it as read-only
>
> Oh, I'd be happy for this control to be R/W. Unfortunately the core
> media core enforces R/O. This is only a comment about what the core code
> does, currently at least.
>
> >> + dev_dbg(dev, "%s()\n", __func__);
> >
> > Rather useless, please drop. Same for all other debug functions that
> > just print the current function name in other parts of the driver.
> > While maybe useful when developing, they're mostly noise for users
> > imo.
>
> Fine, will drop the rest of the debug. In fact, I already dropped the
> most useful parts (I2C register access debugging).
>
> >> + endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> >> + FWNODE_GRAPH_ENDPOINT_NEXT);
> >
> > The drivers supports a single endpoint right ? Then
> > fwnode_graph_get_next_enpoint() can be used
>
> Well, I originally used
> fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), NULL).
> Sakari suggested I should use the above
> fwnode_graph_get_endpoint_by_id().
> It could be a good idea to agree on this. Not sure about the difference.
The OF folks have shunned to the use of the iterative varants as that can
often lead to complicated parsing of the endpoints. As obtaining the
endpoint based on port and endpoint IDs works well in all cases I've
suggested people to use that. But as the backend, at least currently, uses
iterative functions, they're unlikely to disappear in the future.
>
> >> + for (cnt = 0; cnt < ARRAY_SIZE(ar0521_supply_names); cnt++) {
> >> + struct regulator *supply = devm_regulator_get(dev,
> >> + ar0521_supply_names[cnt]);
> >> +
> >> + if (IS_ERR(supply)) {
> >> + dev_info(dev, "no %s regulator found: %li\n",
> >> + ar0521_supply_names[cnt], PTR_ERR(supply));
> >> + return PTR_ERR(supply);
> >> + }
> >> + sensor->supplies[cnt] = supply;
> >> + }
> >
> > Using the regulator bulk api would simplify this.
> > See drivers/media/i2c/ccs/ccs-core.c
>
> The bulk API doesn't allow for delays between individual supplies, does it?
> The delays are mandated by the manufacturer.
If that is the case, then you can't use the bulk API.
That also requires the board to be wired in such a way that these
regulators have no other users.
>
> >> +
> >> + mutex_init(&sensor->lock);
> >> +
> >> + ret = ar0521_init_controls(sensor);
> >> + if (ret)
> >> + goto entity_cleanup;
> >> +
> >> + ar0521_adj_fmt(&sensor->fmt);
> >
> > Called already here above.
>
> Right, I will remove the first one.
>
> >> + ret = v4l2_async_register_subdev(&sensor->sd);
> >> + if (ret)
> >> + goto free_ctrls;
> >> +
> >> + /* Turn on the device and enable runtime PM */
> >> + ret = ar0521_power_on(&client->dev);
> >> + if (ret)
> >> + goto disable;
> >
> > Does the device stay powered all the time ?
>
> Depends on the hw, but the power could be disabled, yes.
>
> > Doesn't your resume callback call power_on() already ?
>
> It does, when the PM is enabled.
>
> Should the above code be changed?
I think you'll need pm_runtime_idle() call after enabling runtime PM.
>
> >> +static struct i2c_driver ar0521_i2c_driver = {
> >> + .driver = {
> >> + .name = "ar0521",
> >> + .pm = &ar0521_pm_ops,
> >> + .of_match_table = ar0521_dt_ids,
> >> + },
> >> + .probe = ar0521_probe,
> >
> > You could use probe_new and drop the "const struct i2c_device_id *id"
> > argument to probe()
>
> You are right again.
>
> That said, I wonder if it would be better (like easier) to have this
> driver added to the staging area instead.
--
Regard,s
Sakari Ailus
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor
2022-02-25 13:18 ` Sakari Ailus
@ 2022-02-28 7:48 ` Krzysztof Hałasa
0 siblings, 0 replies; 10+ messages in thread
From: Krzysztof Hałasa @ 2022-02-28 7:48 UTC (permalink / raw)
To: Sakari Ailus
Cc: Jacopo Mondi, Mauro Carvalho Chehab, Rob Herring, devicetree,
linux-media, linux-kernel, Laurent Pinchart, Joe Perches
Hi Sakari,
Sakari Ailus <sakari.ailus@iki.fi> writes:
>> >> + // access the sensor only if it's powered up
>
> /* This is the preferred comment style */
I keep forgetting about this. Maybe because most other code I work with
(not drivers/media) has already switched to // (single-line) comments.
> The OF folks have shunned to the use of the iterative varants as that can
> often lead to complicated parsing of the endpoints. As obtaining the
> endpoint based on port and endpoint IDs works well in all cases I've
> suggested people to use that. But as the backend, at least currently, uses
> iterative functions, they're unlikely to disappear in the future.
I understand I should continue using fwnode_graph_get_endpoint_by_id(),
right?
Thanks.
--
Krzysztof "Chris" Hałasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor
2022-02-25 12:15 ` Krzysztof Hałasa
2022-02-25 13:18 ` Sakari Ailus
@ 2022-03-01 9:01 ` Jacopo Mondi
2022-03-01 12:19 ` Krzysztof Hałasa
1 sibling, 1 reply; 10+ messages in thread
From: Jacopo Mondi @ 2022-03-01 9:01 UTC (permalink / raw)
To: Krzysztof Hałasa
Cc: Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media,
linux-kernel, Laurent Pinchart, Sakari Ailus, Joe Perches
Hi Krzysztof
On Fri, Feb 25, 2022 at 01:15:11PM +0100, Krzysztof Hałasa wrote:
> Hi Jacopo,
>
> Sorry it took that long. Unfortunately I no longer have much time to
> work on this, thus the delays.
>
No worries! sorry it took me a few days to reply
> Jacopo Mondi <jacopo@jmondi.org> writes:
>
> >> +static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
> >> +{
> >> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> >> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> >> + int ret;
> >> +
> >> + /* v4l2_ctrl_lock() locks our own mutex */
> >> +
> >> + dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, ctrl->id);
> >> +
> >> + switch (ctrl->id) {
> >> + case V4L2_CID_HBLANK:
> >> + case V4L2_CID_VBLANK:
> >> + sensor->total_width = sensor->fmt.width +
> >> + sensor->ctrls.hblank->val;
> >> + sensor->total_height = sensor->fmt.width +
> >> + sensor->ctrls.vblank->val;
> >> + break;
> >> + default:
> >> + ret = -EINVAL;
> >> + break;
> >> + }
> >> +
> >> + // access the sensor only if it's powered up
> >> + if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
> >
> > As you correctly do not access the chip's registers if it's powered
> > off, you have to call __v4l2_ctrl_handler_setup() at power on time to
> > make sure controls are actually set.
>
> These registers are also written in ar0521_set_stream(), isn't it
> enough?
Oh I see, set_stream() sets gains and then calls write_mode() which
handles blankings, exposure and test pattern so yes, all controls seem
to be covered. It's usually safer to go through the control handler
to make sure that any control that is added later is not left behind.
But for now it's all right!
>
> >> + ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
> >> + AR0521_WIDTH_BLANKING_MIN, 4094, 1,
> >> + AR0521_WIDTH_BLANKING_MIN);
> >> + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
> >> + AR0521_HEIGHT_BLANKING_MIN, 4094, 2,
> >> + AR0521_HEIGHT_BLANKING_MIN);
> >> + v4l2_ctrl_cluster(2, &ctrls->hblank);
> >
> > Is it intended to have vblank and hblank as cluster, can't they change
> > independently ?
>
> They can. It appears, though, that clusters aren't about controls which
> can't change independently. Both of the two are written to the hardware
> at the same time, which appears to be a valid reason to combine them
> into a cluster.
Is there a reason to write them to hw at the same time ? :)
> This is similar to the gain/balance controls, and BTW the use of
> clusters there was suggested by you.
>
> Please correct me if I'm wrong.
I don't think it's strictly necessary to clusterize them, but
defintely not a big deal
>
> >> + /* Read-only */
> >> + ctrls->pixrate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
> >> + AR0521_PIXEL_CLOCK_MIN,
> >> + AR0521_PIXEL_CLOCK_MAX, 1,
> >> + AR0521_PIXEL_CLOCK_RATE);
> >
> > so you have to mark it as read-only
>
> Oh, I'd be happy for this control to be R/W. Unfortunately the core
> media core enforces R/O. This is only a comment about what the core code
> does, currently at least.
You right, the core does that for you
>
> >> + dev_dbg(dev, "%s()\n", __func__);
> >
> > Rather useless, please drop. Same for all other debug functions that
> > just print the current function name in other parts of the driver.
> > While maybe useful when developing, they're mostly noise for users
> > imo.
>
> Fine, will drop the rest of the debug. In fact, I already dropped the
> most useful parts (I2C register access debugging).
>
> >> + endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> >> + FWNODE_GRAPH_ENDPOINT_NEXT);
> >
> > The drivers supports a single endpoint right ? Then
> > fwnode_graph_get_next_enpoint() can be used
>
> Well, I originally used
> fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), NULL).
> Sakari suggested I should use the above
> fwnode_graph_get_endpoint_by_id().
> It could be a good idea to agree on this. Not sure about the difference.
>
Let's stick with what Sakari suggested.
> >> + for (cnt = 0; cnt < ARRAY_SIZE(ar0521_supply_names); cnt++) {
> >> + struct regulator *supply = devm_regulator_get(dev,
> >> + ar0521_supply_names[cnt]);
> >> +
> >> + if (IS_ERR(supply)) {
> >> + dev_info(dev, "no %s regulator found: %li\n",
> >> + ar0521_supply_names[cnt], PTR_ERR(supply));
> >> + return PTR_ERR(supply);
> >> + }
> >> + sensor->supplies[cnt] = supply;
> >> + }
> >
> > Using the regulator bulk api would simplify this.
> > See drivers/media/i2c/ccs/ccs-core.c
>
> The bulk API doesn't allow for delays between individual supplies, does it?
> The delays are mandated by the manufacturer.
>
Ack then, no bulk API
> >> +
> >> + mutex_init(&sensor->lock);
> >> +
> >> + ret = ar0521_init_controls(sensor);
> >> + if (ret)
> >> + goto entity_cleanup;
> >> +
> >> + ar0521_adj_fmt(&sensor->fmt);
> >
> > Called already here above.
>
> Right, I will remove the first one.
>
> >> + ret = v4l2_async_register_subdev(&sensor->sd);
> >> + if (ret)
> >> + goto free_ctrls;
> >> +
> >> + /* Turn on the device and enable runtime PM */
> >> + ret = ar0521_power_on(&client->dev);
> >> + if (ret)
> >> + goto disable;
> >
> > Does the device stay powered all the time ?
>
> Depends on the hw, but the power could be disabled, yes.
>
> > Doesn't your resume callback call power_on() already ?
>
> It does, when the PM is enabled.
>
> Should the above code be changed?
>
As it doesn't seem to me you access registers during probe (to
identify the sensor in example) there's no need to power up the device
during probe, but should be enough to let runtime_pm do so when
requested.
> >> +static struct i2c_driver ar0521_i2c_driver = {
> >> + .driver = {
> >> + .name = "ar0521",
> >> + .pm = &ar0521_pm_ops,
> >> + .of_match_table = ar0521_dt_ids,
> >> + },
> >> + .probe = ar0521_probe,
> >
> > You could use probe_new and drop the "const struct i2c_device_id *id"
> > argument to probe()
>
> You are right again.
>
> That said, I wonder if it would be better (like easier) to have this
> driver added to the staging area instead.
Personally I don't think this driver qualifies for staging, it can
be accepted in this form and be eventually changed on top in
drivers/media. Let's see what maintainers think.
I'll review the last version and hope to see this merged soon.
Thanks
j
> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor
2022-03-01 9:01 ` Jacopo Mondi
@ 2022-03-01 12:19 ` Krzysztof Hałasa
0 siblings, 0 replies; 10+ messages in thread
From: Krzysztof Hałasa @ 2022-03-01 12:19 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media,
linux-kernel, Laurent Pinchart, Sakari Ailus, Joe Perches
Jacopo Mondi <jacopo@jmondi.org> writes:
>> They can. It appears, though, that clusters aren't about controls which
>> can't change independently. Both of the two are written to the hardware
>> at the same time, which appears to be a valid reason to combine them
>> into a cluster.
>
> Is there a reason to write them to hw at the same time ? :)
Sure, they are written in a single merged burst on I2C.
> As it doesn't seem to me you access registers during probe (to
> identify the sensor in example) there's no need to power up the device
> during probe, but should be enough to let runtime_pm do so when
> requested.
Yes, in fact I never try to read anything from the sensor :-)
Unfortunately I'm not sure how to initialize the (possibly nonexistent)
PM without powering it up, cleanly, in all kernel configs. I mean, I can
check if PM failed and power it up "by hand", but it's a bit messy.
Suggestions are welcome, though.
--
Krzysztof "Chris" Hałasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-03-01 12:19 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-03 13:29 [PATCH v7 0/2] On Semi AR0521 sensor driver Krzysztof Hałasa
2022-01-03 13:32 ` [PATCH v7 1/2] dt-binding: media: document ON Semi AR0521 sensor bindings Krzysztof Hałasa
2022-01-03 13:36 ` [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor Krzysztof Hałasa
2022-01-09 15:34 ` Jacopo Mondi
2022-01-09 19:01 ` Joe Perches
2022-02-25 12:15 ` Krzysztof Hałasa
2022-02-25 13:18 ` Sakari Ailus
2022-02-28 7:48 ` Krzysztof Hałasa
2022-03-01 9:01 ` Jacopo Mondi
2022-03-01 12:19 ` Krzysztof Hałasa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.