* [PATCH v6 0/2] On Semi AR0521 sensor driver @ 2021-12-23 6:54 Krzysztof Hałasa 2021-12-23 6:57 ` [PATCH v6 1/2] dt-binding: media: document ON Semi AR0521 sensor bindings Krzysztof Hałasa 2021-12-23 7:06 ` [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor Krzysztof Hałasa 0 siblings, 2 replies; 14+ messages in thread From: Krzysztof Hałasa @ 2021-12-23 6:54 UTC (permalink / raw) To: Rob Herring, Mauro Carvalho Chehab Cc: devicetree, linux-media, linux-kernel, Laurent Pinchart, Sakari Ailus, Jacopo Mondi Rob, Mauro, media subsystem reviewers, This is the 6th version of my On Semi AR0521 sensor driver. 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) changes: MAINTAINERS | 7 drivers/media/i2c/Kconfig | 13 drivers/media/i2c/Makefile | 1 drivers/media/i2c/ar0521.c | 1051 4 files changed, 1072 insertions(+) - 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. - a lot of smaller changes suggested by Laurent, Sakari, Jacopo 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] 14+ messages in thread
* [PATCH v6 1/2] dt-binding: media: document ON Semi AR0521 sensor bindings 2021-12-23 6:54 [PATCH v6 0/2] On Semi AR0521 sensor driver Krzysztof Hałasa @ 2021-12-23 6:57 ` Krzysztof Hałasa 2021-12-23 7:06 ` [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor Krzysztof Hałasa 1 sibling, 0 replies; 14+ messages in thread From: Krzysztof Hałasa @ 2021-12-23 6:57 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] 14+ messages in thread
* [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor 2021-12-23 6:54 [PATCH v6 0/2] On Semi AR0521 sensor driver Krzysztof Hałasa 2021-12-23 6:57 ` [PATCH v6 1/2] dt-binding: media: document ON Semi AR0521 sensor bindings Krzysztof Hałasa @ 2021-12-23 7:06 ` Krzysztof Hałasa 2021-12-23 17:49 ` Joe Perches 2021-12-31 23:14 ` kernel test robot 1 sibling, 2 replies; 14+ messages in thread From: Krzysztof Hałasa @ 2021-12-23 7:06 UTC (permalink / raw) To: Mauro Carvalho Chehab, Rob Herring Cc: devicetree, linux-media, linux-kernel, Laurent Pinchart, Sakari Ailus, Jacopo Mondi 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 38235471fed0..472a49e6df61 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..f113c1b163de --- /dev/null +++ b/drivers/media/i2c/ar0521.c @@ -0,0 +1,1051 @@ +// 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 */ +}; + +#define AR0521_NUM_SUPPLIES ARRAY_SIZE(ar0521_supply_names) + +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[AR0521_NUM_SUPPLIES]; + 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; + + if (!pm_runtime_get_if_in_use(&client->dev)) + return 0; + + msg.addr = client->addr; + msg.flags = client->flags; + msg.buf = (u8 *)data; + msg.len = count * sizeof(*data); + + ret = i2c_transfer(client->adapter, &msg, 1); + pm_runtime_put(&client->dev); + + 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 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); + + ret = ar0521_write_mode(sensor); + if (ret) + return ret; + + if (on) { + ret = ar0521_set_gains(sensor); + if (ret) + return ret; + + /* Exit LP-11 mode on clock and data lanes */ + ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS, + 0); + if (ret) + return ret; + + /* Start streaming */ + return ar0521_write_reg(sensor, AR0521_REG_RESET, + AR0521_REG_RESET_DEFAULTS | + AR0521_REG_RESET_STREAM); + } 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 */ + return ar0521_write_reg(sensor, AR0521_REG_RESET, + AR0521_REG_RESET_DEFAULTS); + } + +} + +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; +} + +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; +} + +#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_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); + ret = ar0521_write_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; + 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; + default: + ret = -EINVAL; + break; + } + + 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 ARRAY_AND_SIZE(a) (a), ARRAY_SIZE(a) +#define REGS(...) {ARRAY_AND_SIZE(((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 */ + be(0x3300), /* 3EC4: FSC clamps for HDR mode and adc comp power + down co */ + 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 */ + be(0x203B), /* 3ECE: Ramp buffer settings and Booster enable + (bits 0-5) */ + 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 void 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 = AR0521_NUM_SUPPLIES - 1; i >= 0; i--) { + if (sensor->supplies[i]) + regulator_disable(sensor->supplies[i]); + } +} + +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 < AR0521_NUM_SUPPLIES; 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; + + ar0521_calc_mode(sensor); + + ret = ar0521_set_stream(sensor, 0); + 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; + + if (!(flags & V4L2_SUBDEV_PRE_STREAMON_FL_MANUAL_LP)) + return 0; + + /* 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) + return ret; + + /* Start streaming LP-11 */ + return ar0521_write_reg(sensor, AR0521_REG_RESET, + AR0521_REG_RESET_DEFAULTS | + AR0521_REG_RESET_STREAM); +} + +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, +}; + +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); + + 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); + + 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 < AR0521_NUM_SUPPLIES; 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] 14+ messages in thread
* Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor 2021-12-23 7:06 ` [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor Krzysztof Hałasa @ 2021-12-23 17:49 ` Joe Perches 2021-12-23 18:48 ` Jacopo Mondi 2021-12-29 14:11 ` Krzysztof Hałasa 2021-12-31 23:14 ` kernel test robot 1 sibling, 2 replies; 14+ messages in thread From: Joe Perches @ 2021-12-23 17:49 UTC (permalink / raw) To: Krzysztof Hałasa, Mauro Carvalho Chehab, Rob Herring Cc: devicetree, linux-media, linux-kernel, Laurent Pinchart, Sakari Ailus, Jacopo Mondi On Thu, 2021-12-23 at 08:06 +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. trivial notes: > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c [] > +/* External clock (extclk) frequencies */ > +#define AR0521_EXTCLK_MIN (10 * 1000 * 1000) Generally, adding a prefix like AR0521_ to defines that are locally defined in a single file unnecessarily increases identifier length. It makes using short line lengths difficult. e.g. Using this identifier anywhere > +#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80 Many of the 80 column line lengths and line wrapping used in this file are not really nice to read. I believe you don't have to be strict about 80 column lines. > +#define be cpu_to_be16 It's a pity there's no way to declare an array with all members having a specific endianness. Making sure all elements in these arrays are declared with be() is tedious. > +#define AR0521_NUM_SUPPLIES ARRAY_SIZE(ar0521_supply_names) It's almost always better to use ARRAY_SIZE directly and not use a #define for the array size. > +static int ar0521_set_gains(struct ar0521_dev *sensor) > +{ [] > + dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__); ftrace works and perhaps all the similar debug logging uses aren't really necessary. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor 2021-12-23 17:49 ` Joe Perches @ 2021-12-23 18:48 ` Jacopo Mondi 2021-12-23 19:19 ` Joe Perches 2021-12-23 20:13 ` Joe Perches 2021-12-29 14:11 ` Krzysztof Hałasa 1 sibling, 2 replies; 14+ messages in thread From: Jacopo Mondi @ 2021-12-23 18:48 UTC (permalink / raw) To: Joe Perches Cc: Krzysztof Hałasa, Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media, linux-kernel, Laurent Pinchart, Sakari Ailus Hi Joe, sorry to jump in On Thu, Dec 23, 2021 at 09:49:58AM -0800, Joe Perches wrote: > On Thu, 2021-12-23 at 08:06 +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. > > trivial notes: > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > [] > > +/* External clock (extclk) frequencies */ > > +#define AR0521_EXTCLK_MIN (10 * 1000 * 1000) > > Generally, adding a prefix like AR0521_ to defines that are > locally defined in a single file unnecessarily increases > identifier length. > > It makes using short line lengths difficult. > > e.g. Using this identifier anywhere > > > +#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80 > > Many of the 80 column line lengths and line wrapping used in this > file are not really nice to read. I believe you don't have to be > strict about 80 column lines. > Krzysztof first version had much longer lines, and in facts it has been asked by me to reduce them to 80 cols. The media subsystem requires to validate patches with ./scripts/checkpatch.pl --strict --max-line-length=80 We longly debated this and I believe it's now generally accepted to go over 80 when it makes sense, but not regularly span to 120 cols like in the previous version. I think this 80-cols limit not being an hard limit anymore is doing more worse than good, as each subsystem applies a different rule, and I know how frustrating is for Krzysztof to be pushed in different direction, as the same happened to me when I contributed to other subsystems and I've been asked to span to 100 cols while I was trying to stay in 80 no matter what. Thanks j > > +#define be cpu_to_be16 > > It's a pity there's no way to declare an array with all members > having a specific endianness. Making sure all elements in these > arrays are declared with be() is tedious. > > > +#define AR0521_NUM_SUPPLIES ARRAY_SIZE(ar0521_supply_names) > > It's almost always better to use ARRAY_SIZE directly and not > use a #define for the array size. > > > +static int ar0521_set_gains(struct ar0521_dev *sensor) > > +{ > [] > > + dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__); > > ftrace works and perhaps all the similar debug logging uses aren't > really necessary. > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor 2021-12-23 18:48 ` Jacopo Mondi @ 2021-12-23 19:19 ` Joe Perches 2021-12-23 20:13 ` Joe Perches 1 sibling, 0 replies; 14+ messages in thread From: Joe Perches @ 2021-12-23 19:19 UTC (permalink / raw) To: Jacopo Mondi Cc: Krzysztof Hałasa, Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media, linux-kernel, Laurent Pinchart, Sakari Ailus On Thu, 2021-12-23 at 19:48 +0100, Jacopo Mondi wrote: > Hi Joe, > sorry to jump in No worries. It's all just a bikeshed and doesn't really matter to the correctness of the code. > On Thu, Dec 23, 2021 at 09:49:58AM -0800, Joe Perches wrote: > > On Thu, 2021-12-23 at 08:06 +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. > > > > trivial notes: > > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > [] > > > +/* External clock (extclk) frequencies */ > > > +#define AR0521_EXTCLK_MIN (10 * 1000 * 1000) > > > > Generally, adding a prefix like AR0521_ to defines that are > > locally defined in a single file unnecessarily increases > > identifier length. > > > > It makes using short line lengths difficult. > > > > e.g. Using this identifier anywhere > > > > > +#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80 > > > > Many of the 80 column line lengths and line wrapping used in this > > file are not really nice to read. I believe you don't have to be > > strict about 80 column lines. > > > > Krzysztof first version had much longer lines, and in facts it has > been asked by me to reduce them to 80 cols. The media subsystem > requires to validate patches with > > ./scripts/checkpatch.pl --strict --max-line-length=80 > > We longly debated this and I believe it's now generally accepted to go > over 80 when it makes sense, but not regularly span to 120 cols like > in the previous version. IMO: Many of the lines here could be lengthened to < 100 to improve readability. > I think this 80-cols limit not being an hard limit anymore is doing > more worse than good, as each subsystem applies a different rule, and > I know how frustrating is for Krzysztof to be pushed in different > direction, as the same happened to me when I contributed to other > subsystems and I've been asked to span to 100 cols while I was trying > to stay in 80 no matter what. Up to you all. But there's a tension between long identifiers and short lines. And anything using a 55 character identifier basically guarantees that the code will exceed 80 columns. Using identifiers with 10 character or so is generally OK, but there are dozens of longer identifiers specific to this code. I'd suggest because of these long identifiers that the code be restricted to 100 columns, but not strictly at 80. And there are quite a few long lines in drivers/media/i2c and espcially for drivers/media/ A few of them are grotesquely long. Probably all of them are historic and don't warrant change. Just for i2c: $ git ls-files -- 'drivers/media/i2c/*.[ch]' | \ xargs awk '{print length($0); }' | \ sort -rn | uniq -c 2 143 1 141 1 123 4 118 2 114 1 111 1 110 1 109 1 107 1 105 4 104 2 102 3 101 1 100 2 99 11 98 7 97 8 96 4 95 11 94 8 93 19 92 13 91 28 90 20 89 28 88 39 87 18 86 33 85 42 84 86 83 38 82 47 81 167 80 110 79 155 78 363 77 230 76 219 75 217 74 427 73 695 72 471 71 538 70 525 69 679 68 661 67 1046 66 757 65 1002 64 942 63 1053 62 967 61 1018 60 1132 59 1307 58 1366 57 3206 56 1240 55 2191 54 1829 53 1719 52 1503 51 1795 50 1714 49 1640 48 1567 47 1550 46 1880 45 2155 44 1780 43 1880 42 1854 41 1962 40 2031 39 2009 38 2022 37 2240 36 2252 35 2152 34 2178 33 2074 32 2185 31 2462 30 2478 29 2186 28 1988 27 1846 26 1926 25 2177 24 2048 23 1804 22 1267 21 1414 20 1563 19 6154 18 3619 17 7222 16 1682 15 2685 14 3037 13 2142 12 1704 11 3013 10 3191 9 1609 8 230 7 461 6 571 5 878 4 2790 3 6524 2 7732 1 24729 0 And for all of drivers/media: $ git ls-files -- 'drivers/media/*.[ch]' | \ xargs awk '{print length($0);}' | \ sort -rn | uniq -c 1 338 1 314 1 268 1 261 1 255 1 254 1 242 1 234 1 228 1 213 1 207 1 205 1 198 2 197 3 192 2 188 2 177 1 174 2 172 2 169 3 168 2 167 1 166 1 165 1 164 3 163 2 162 2 161 2 160 6 158 10 157 3 156 2 155 3 154 2 153 12 152 8 151 49 150 4 148 2 147 3 146 3 145 5 144 5 143 1 142 6 141 7 140 8 139 6 138 10 137 14 136 13 135 14 134 13 133 11 132 7 131 6 130 15 129 21 128 17 127 13 126 10 125 13 124 12 123 25 122 20 121 25 120 15 119 18 118 20 117 23 116 30 115 23 114 26 113 35 112 35 111 40 110 60 109 50 108 72 107 42 106 47 105 105 104 72 103 90 102 110 101 144 100 79 99 122 98 226 97 644 96 115 95 135 94 135 93 166 92 210 91 227 90 218 89 208 88 279 87 292 86 1260 85 1122 84 879 83 1288 82 1489 81 2505 80 6241 79 3653 78 5268 77 2012 76 2168 75 2279 74 3297 73 4343 72 3741 71 4018 70 4360 69 4487 68 4433 67 6014 66 6098 65 6547 64 6661 63 7312 62 7684 61 7610 60 8157 59 9052 58 10047 57 12064 56 9904 55 11075 54 11271 53 13259 52 11585 51 15036 50 13930 49 15159 48 14221 47 13349 46 14243 45 15887 44 17829 43 16620 42 17759 41 17569 40 16653 39 17386 38 17480 37 18296 36 18205 35 18782 34 18352 33 18137 32 19556 31 19229 30 19403 29 19570 28 19447 27 19581 26 19255 25 19300 24 17038 23 18523 22 15609 21 16188 20 14634 19 19426 18 20979 17 21548 16 13476 15 16713 14 18914 13 17577 12 12828 11 19525 10 21665 9 13912 8 3261 7 5375 6 6756 5 8260 4 23448 3 48708 2 55786 1 182329 0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor 2021-12-23 18:48 ` Jacopo Mondi 2021-12-23 19:19 ` Joe Perches @ 2021-12-23 20:13 ` Joe Perches 2021-12-23 20:27 ` Joe Perches 2021-12-24 9:22 ` Jacopo Mondi 1 sibling, 2 replies; 14+ messages in thread From: Joe Perches @ 2021-12-23 20:13 UTC (permalink / raw) To: Jacopo Mondi Cc: Krzysztof Hałasa, Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media, linux-kernel, Laurent Pinchart, Sakari Ailus On Thu, 2021-12-23 at 19:48 +0100, Jacopo Mondi wrote: > The media subsystem requires to validate patches with > > ./scripts/checkpatch.pl --strict --max-line-length=80 > > We longly debated this and I believe it's now generally accepted to go > over 80 when it makes sense, but not regularly span to 120 cols like > in the previous version. Where is this documented and do you have a link to the debate? The archive for the i2c mailing list doesn't show much debate: https://lore.kernel.org/linux-i2c/?q=%2280+columns%22 https://lore.kernel.org/linux-i2c/?q=%22line+length%22 Perhaps there should be a MAINTAINERS P: entry for this requirement. From MAINTAINERS: P: Subsystem Profile document for more details submitting patches to the given subsystem. This is either an in-tree file, or a URI. See Documentation/maintainer/maintainer-entry-profile.rst for details. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor 2021-12-23 20:13 ` Joe Perches @ 2021-12-23 20:27 ` Joe Perches 2021-12-24 9:22 ` Jacopo Mondi 1 sibling, 0 replies; 14+ messages in thread From: Joe Perches @ 2021-12-23 20:27 UTC (permalink / raw) To: Jacopo Mondi Cc: Krzysztof Hałasa, Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media, linux-kernel, Laurent Pinchart, Sakari Ailus On Thu, 2021-12-23 at 12:13 -0800, Joe Perches wrote: > On Thu, 2021-12-23 at 19:48 +0100, Jacopo Mondi wrote: > > The media subsystem requires to validate patches with > > > > ./scripts/checkpatch.pl --strict --max-line-length=80 > > > > We longly debated this and I believe it's now generally accepted to go > > over 80 when it makes sense, but not regularly span to 120 cols like > > in the previous version. [] > Perhaps there should be a MAINTAINERS P: entry for this requirement. > > From MAINTAINERS: > > P: Subsystem Profile document for more details submitting > patches to the given subsystem. This is either an in-tree file, > or a URI. See Documentation/maintainer/maintainer-entry-profile.rst > for details. Perhaps: --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index cd55b83878e05..bbfcb8e7eef06 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11977,6 +11977,7 @@ L: linux-media@vger.kernel.org S: Maintained W: https://linuxtv.org Q: http://patchwork.kernel.org/project/linux-media/list/ +P: https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/maintainer-entry-profile.html T: git git://linuxtv.org/media_tree.git F: Documentation/admin-guide/media/ F: Documentation/devicetree/bindings/media/ ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor 2021-12-23 20:13 ` Joe Perches 2021-12-23 20:27 ` Joe Perches @ 2021-12-24 9:22 ` Jacopo Mondi 2021-12-24 12:30 ` Joe Perches 2021-12-29 15:05 ` Krzysztof Hałasa 1 sibling, 2 replies; 14+ messages in thread From: Jacopo Mondi @ 2021-12-24 9:22 UTC (permalink / raw) To: Joe Perches Cc: Krzysztof Hałasa, Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media, linux-kernel, Laurent Pinchart, Sakari Ailus Hi Joe On Thu, Dec 23, 2021 at 12:13:10PM -0800, Joe Perches wrote: > On Thu, 2021-12-23 at 19:48 +0100, Jacopo Mondi wrote: > > The media subsystem requires to validate patches with > > > > ./scripts/checkpatch.pl --strict --max-line-length=80 > > > > We longly debated this and I believe it's now generally accepted to go > > over 80 when it makes sense, but not regularly span to 120 cols like > > in the previous version. > > Where is this documented and do you have a link to the debate? It's in the subsystem maintainer profile Documentation/driver-api/media/maintainer-entry-profile.rst Where of course some exceptions are listed but it's anyway enforced that "efforts should be made towards staying within 80 characters per line" - on strings, as they shouldn't be broken due to line length limits; - when a function or variable name need to have a big identifier name, which keeps hard to honor the 80 columns limit; - on arithmetic expressions, when breaking lines makes them harder to read; - when they avoid a line to end with an open parenthesis or an open bracket. The debate I mentioned was specifically on the previous version of the driver where me and Krzysztof shown quite different understanding of coding style requirements. https://patchwork.linuxtv.org/project/linux-media/patch/m3fstfoexa.fsf@t19.piap.pl/ That lead me to submit this https://patchwork.linuxtv.org/project/linux-media/patch/20211013092005.14268-1-jacopo@jmondi.org/ which I never managed to re-send, my bad. > > The archive for the i2c mailing list doesn't show much debate: > > https://lore.kernel.org/linux-i2c/?q=%2280+columns%22 > https://lore.kernel.org/linux-i2c/?q=%22line+length%22 > > Perhaps there should be a MAINTAINERS P: entry for this requirement. > > From MAINTAINERS: > > P: Subsystem Profile document for more details submitting > patches to the given subsystem. This is either an in-tree file, > or a URI. See Documentation/maintainer/maintainer-entry-profile.rst > for details. > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor 2021-12-24 9:22 ` Jacopo Mondi @ 2021-12-24 12:30 ` Joe Perches 2021-12-29 15:05 ` Krzysztof Hałasa 1 sibling, 0 replies; 14+ messages in thread From: Joe Perches @ 2021-12-24 12:30 UTC (permalink / raw) To: Jacopo Mondi Cc: Krzysztof Hałasa, Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media, linux-kernel, Laurent Pinchart, Sakari Ailus On Fri, 2021-12-24 at 10:22 +0100, Jacopo Mondi wrote: > Hi Joe hi again. > On Thu, Dec 23, 2021 at 12:13:10PM -0800, Joe Perches wrote: > > On Thu, 2021-12-23 at 19:48 +0100, Jacopo Mondi wrote: > > > The media subsystem requires to validate patches with > > > > > > ./scripts/checkpatch.pl --strict --max-line-length=80 > > > > > > We longly debated this and I believe it's now generally accepted to go > > > over 80 when it makes sense, but not regularly span to 120 cols like > > > in the previous version. > > > > Where is this documented and do you have a link to the debate? > > It's in the subsystem maintainer profile > Documentation/driver-api/media/maintainer-entry-profile.rst > > Where of course some exceptions are listed but it's anyway enforced > that "efforts should be made towards staying within 80 > characters per line" > > - on strings, as they shouldn't be broken due to line length limits; > - when a function or variable name need to have a big identifier name, > which keeps hard to honor the 80 columns limit; > - on arithmetic expressions, when breaking lines makes them harder to > read; > - when they avoid a line to end with an open parenthesis or an open > bracket. > > The debate I mentioned was specifically on the previous version of the > driver where me and Krzysztof shown quite different understanding of > coding style requirements. > https://patchwork.linuxtv.org/project/linux-media/patch/m3fstfoexa.fsf@t19.piap.pl/ Thanks for that. > That lead me to submit this > https://patchwork.linuxtv.org/project/linux-media/patch/20211013092005.14268-1-jacopo@jmondi.org/ That too. FWIW, I believe using more than 100 columns or so makes it more difficult to track quickly and efficiently to the next line. Reading with multiple visual saccades on a single line is slow. And IMO: o reverse xmas tree declarations is quite a poor style requirement o single line c99 // comments should be encouraged/preferred o identifiers longer than 20 characters or so should be discouraged ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor 2021-12-24 9:22 ` Jacopo Mondi 2021-12-24 12:30 ` Joe Perches @ 2021-12-29 15:05 ` Krzysztof Hałasa 1 sibling, 0 replies; 14+ messages in thread From: Krzysztof Hałasa @ 2021-12-29 15:05 UTC (permalink / raw) To: Jacopo Mondi Cc: Joe Perches, Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media, linux-kernel, Laurent Pinchart, Sakari Ailus Being that "stubborn developer"... I think all those always increasing restrictions are a dead end and serve no useful purpose. I strongly oppose restrictions which are just for the sake of uniformity. I think the coding style rules make sense only as long as they increase readability, and should never extend further. And in particular, we should NEVER sacrifice readability for uniformity. Unfortunately one can't measure readability easily, and what's better for one, is worse for another (think - vision problems). Yes, some arbitrary basic rules like tab size and brace style are needed, and there are certain rules needed for correctness (like "type* ptr" vs "type *ptr" which is quite visible in "type* a, b") but the rest should serve the readability. I.e., we shouldn't want to have all code identical - we should want it to be good. E.g., for me, it's better to have 99 worse source files (like wildly formatted to 80 columns) and 1 (perhaps simply more recent) file with a better, cleaner formatting than to have 100 of the former kind. But what do I know... -- 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] 14+ messages in thread
* Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor 2021-12-23 17:49 ` Joe Perches 2021-12-23 18:48 ` Jacopo Mondi @ 2021-12-29 14:11 ` Krzysztof Hałasa 1 sibling, 0 replies; 14+ messages in thread From: Krzysztof Hałasa @ 2021-12-29 14:11 UTC (permalink / raw) To: Joe Perches Cc: Mauro Carvalho Chehab, Rob Herring, devicetree, linux-media, linux-kernel, Laurent Pinchart, Sakari Ailus, Jacopo Mondi Hello Joe, Joe Perches <joe@perches.com> writes: >> +/* External clock (extclk) frequencies */ >> +#define AR0521_EXTCLK_MIN (10 * 1000 * 1000) > > Generally, adding a prefix like AR0521_ to defines that are > locally defined in a single file unnecessarily increases > identifier length. Right. In general, I don't do that (for that very reason), however in drivers/media this looks like a common practice and I didn't want to break it. > e.g. Using this identifier anywhere > >> +#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80 Right. However, such a name helps looking this up in the docs. E.g. the register name in the docs is "hispi_control_status" and the bitfield is "framer_test_mode" or something like that. Since it's just one register (+ value) and it actually fits in 80 columns without too much problems, I'd rather like to leave it unchanged. > Many of the 80 column line lengths and line wrapping used in this > file are not really nice to read. I believe you don't have to be > strict about 80 column lines. Well, personally I think we could all switch to VT100's 132 columns. Introduced in '78 :-) That's what I currently use for non-kernel tasks (not the VT100 but just the line length). OTOH I'm using that emacs wrapping mode so longer lines aren't a problem either. But here, in drivers/media, I'm told 80 column is strict. >> +#define be cpu_to_be16 > > It's a pity there's no way to declare an array with all members > having a specific endianness. Making sure all elements in these > arrays are declared with be() is tedious. Right. Unfortunately anything else would mean recoding. >> +#define AR0521_NUM_SUPPLIES ARRAY_SIZE(ar0521_supply_names) > > It's almost always better to use ARRAY_SIZE directly and not > use a #define for the array size. It's another custom in drivers/media, but I guess I don't have to follow it closely, do I? I never liked the #define. >> +static int ar0521_set_gains(struct ar0521_dev *sensor) >> +{ > [] >> + dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__); > > ftrace works and perhaps all the similar debug logging uses aren't > really necessary. TBH I've never used ftrace. It appears that it can't show the arguments, can it? If not, I'd rather leave these dev_dbg()s in place - like other drivers/media/* in fact. However obviously the code without deb_dbg()s would be cleaner, so if ftrace can show the (formatted) arguments, I'm all for it. Thanks for looking at this, -- 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] 14+ messages in thread
* Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor 2021-12-23 7:06 ` [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor Krzysztof Hałasa @ 2021-12-31 23:14 ` kernel test robot 2021-12-31 23:14 ` kernel test robot 1 sibling, 0 replies; 14+ messages in thread From: kernel test robot @ 2021-12-31 23:14 UTC (permalink / raw) To: Krzysztof Hałasa, Mauro Carvalho Chehab, Rob Herring Cc: kbuild-all, linux-media, devicetree, linux-kernel, Laurent Pinchart, Sakari Ailus, Jacopo Mondi Hi "Krzysztof, I love your patch! Yet something to improve: [auto build test ERROR on media-tree/master] [also build test ERROR on linus/master v5.16-rc7 next-20211224] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Krzysztof-Ha-asa/On-Semi-AR0521-sensor-driver/20211223-150758 base: git://linuxtv.org/media_tree.git master config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20220101/202201010737.V5A5o9x5-lkp@intel.com/config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/664482ab74a2331a7a7ead9256b0455cfc3334c7 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Krzysztof-Ha-asa/On-Semi-AR0521-sensor-driver/20211223-150758 git checkout 664482ab74a2331a7a7ead9256b0455cfc3334c7 # save the config file to linux build tree mkdir build_dir make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from include/linux/device.h:25, from include/linux/pm_runtime.h:11, from drivers/media/i2c/ar0521.c:10: >> drivers/media/i2c/ar0521.c:1029:21: error: initialization of 'int (*)(struct device *)' from incompatible pointer type 'void (*)(struct device *)' [-Werror=incompatible-pointer-types] 1029 | SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL) | ^~~~~~~~~~~~~~~~ include/linux/pm.h:341:21: note: in definition of macro 'SET_RUNTIME_PM_OPS' 341 | .runtime_suspend = suspend_fn, \ | ^~~~~~~~~~ drivers/media/i2c/ar0521.c:1029:21: note: (near initialization for 'ar0521_pm_ops.runtime_suspend') 1029 | SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL) | ^~~~~~~~~~~~~~~~ include/linux/pm.h:341:21: note: in definition of macro 'SET_RUNTIME_PM_OPS' 341 | .runtime_suspend = suspend_fn, \ | ^~~~~~~~~~ cc1: some warnings being treated as errors vim +1029 drivers/media/i2c/ar0521.c 1026 1027 static const struct dev_pm_ops ar0521_pm_ops = { 1028 SET_SYSTEM_SLEEP_PM_OPS(ar0521_suspend, ar0521_resume) > 1029 SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL) 1030 }; 1031 static const struct of_device_id ar0521_dt_ids[] = { 1032 {.compatible = "onnn,ar0521"}, 1033 {} 1034 }; 1035 MODULE_DEVICE_TABLE(of, ar0521_dt_ids); 1036 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor @ 2021-12-31 23:14 ` kernel test robot 0 siblings, 0 replies; 14+ messages in thread From: kernel test robot @ 2021-12-31 23:14 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 2992 bytes --] Hi "Krzysztof, I love your patch! Yet something to improve: [auto build test ERROR on media-tree/master] [also build test ERROR on linus/master v5.16-rc7 next-20211224] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Krzysztof-Ha-asa/On-Semi-AR0521-sensor-driver/20211223-150758 base: git://linuxtv.org/media_tree.git master config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20220101/202201010737.V5A5o9x5-lkp(a)intel.com/config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/664482ab74a2331a7a7ead9256b0455cfc3334c7 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Krzysztof-Ha-asa/On-Semi-AR0521-sensor-driver/20211223-150758 git checkout 664482ab74a2331a7a7ead9256b0455cfc3334c7 # save the config file to linux build tree mkdir build_dir make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from include/linux/device.h:25, from include/linux/pm_runtime.h:11, from drivers/media/i2c/ar0521.c:10: >> drivers/media/i2c/ar0521.c:1029:21: error: initialization of 'int (*)(struct device *)' from incompatible pointer type 'void (*)(struct device *)' [-Werror=incompatible-pointer-types] 1029 | SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL) | ^~~~~~~~~~~~~~~~ include/linux/pm.h:341:21: note: in definition of macro 'SET_RUNTIME_PM_OPS' 341 | .runtime_suspend = suspend_fn, \ | ^~~~~~~~~~ drivers/media/i2c/ar0521.c:1029:21: note: (near initialization for 'ar0521_pm_ops.runtime_suspend') 1029 | SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL) | ^~~~~~~~~~~~~~~~ include/linux/pm.h:341:21: note: in definition of macro 'SET_RUNTIME_PM_OPS' 341 | .runtime_suspend = suspend_fn, \ | ^~~~~~~~~~ cc1: some warnings being treated as errors vim +1029 drivers/media/i2c/ar0521.c 1026 1027 static const struct dev_pm_ops ar0521_pm_ops = { 1028 SET_SYSTEM_SLEEP_PM_OPS(ar0521_suspend, ar0521_resume) > 1029 SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL) 1030 }; 1031 static const struct of_device_id ar0521_dt_ids[] = { 1032 {.compatible = "onnn,ar0521"}, 1033 {} 1034 }; 1035 MODULE_DEVICE_TABLE(of, ar0521_dt_ids); 1036 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-12-31 23:15 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-12-23 6:54 [PATCH v6 0/2] On Semi AR0521 sensor driver Krzysztof Hałasa 2021-12-23 6:57 ` [PATCH v6 1/2] dt-binding: media: document ON Semi AR0521 sensor bindings Krzysztof Hałasa 2021-12-23 7:06 ` [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor Krzysztof Hałasa 2021-12-23 17:49 ` Joe Perches 2021-12-23 18:48 ` Jacopo Mondi 2021-12-23 19:19 ` Joe Perches 2021-12-23 20:13 ` Joe Perches 2021-12-23 20:27 ` Joe Perches 2021-12-24 9:22 ` Jacopo Mondi 2021-12-24 12:30 ` Joe Perches 2021-12-29 15:05 ` Krzysztof Hałasa 2021-12-29 14:11 ` Krzysztof Hałasa 2021-12-31 23:14 ` kernel test robot 2021-12-31 23:14 ` kernel test robot
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.