Re: [PATCH v2 1/2] media: i2c: Add driver for the Analog Devices ADDI9036 ToF front-end

From: Sakari Ailus
Date: Wed Oct 07 2020 - 07:18:34 EST

Next message: Joel Stanley: "Re: [PATCH 1/3] configs: aspeed: enable UHCI driver in defconfig"
Previous message: Borislav Petkov: "Re: [tip: ras/core] x86, powerpc: Rename memcpy_mcsafe() to copy_mc_to_{user, kernel}()"
In reply to: Rob Herring: "Re: [PATCH v2 2/2] media: dt-bindings: media: i2c: Add bindings for ADDI9036"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Bogdan,

On Fri, Oct 02, 2020 at 04:35:16PM +0300, Bogdan Togorean wrote:
> The ADDI9036 is a complete, 45 MHz, front-end solution for charge
> coupled device (CCD) time of flight (TOF) imaging applications.
>
> It has 2-lane MIPI CSI-2 RAW12 data output and i2c control interface.
>
> The programming of calibration and firmware is performed by driver
> using Linux Firmware API.
>
> Signed-off-by: Bogdan Togorean <bogdan.togorean@xxxxxxxxxx>
> ---
>
> v2: implemented reading of FW using Linux Firmware API
> removed custom controls for FW programming
> added custom control to select operating mode
> implemented V1 review remarks
> cleaned includes list
> ---
> MAINTAINERS | 10 +
> drivers/media/i2c/Kconfig | 14 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/addi9036.c | 754 +++++++++++++++++++++++++++++
> include/uapi/linux/v4l2-controls.h | 6 +
> 5 files changed, 785 insertions(+)
> create mode 100644 drivers/media/i2c/addi9036.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0d0862b19ce5..4e3878e6c0ba 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -477,6 +477,16 @@ W: http://wiki.analog.com/AD7879
> W: http://ez.analog.com/community/linux-device-drivers
> F: drivers/input/touchscreen/ad7879.c
>
> +ADDI9036 TOF FRONTEND DRIVER
> +M: Bogdan Togorean <bogdan.togorean@xxxxxxxxxx>
> +L: linux-media@xxxxxxxxxxxxxxx
> +S: Supported
> +W: https://www.analog.com/en/products/addi9036.html
> +W: http://ez.analog.com/community/linux-device-drivers
> +T: git git://linuxtv.org/media_tree.git
> +F: Documentation/devicetree/bindings/media/i2c/adi,addi9036.yaml
> +F: drivers/media/i2c/addi9036.c
> +
> ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
> M: Jiri Kosina <jikos@xxxxxxxxxx>
> S: Maintained
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index c7ba76fee599..087dd307505d 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -725,6 +725,20 @@ config VIDEO_APTINA_PLL
> config VIDEO_SMIAPP_PLL
> tristate
>
> +config VIDEO_ADDI9036
> + tristate "Analog Devices ADDI9036 ToF front-end support"
> + depends on I2C && VIDEO_V4L2
> + select MEDIA_CONTROLLER
> + select VIDEO_V4L2_SUBDEV_API
> + select V4L2_FWNODE
> + select REGMAP_I2C
> + help
> + This is a Video4Linux2 driver for Analog Devices ADDI9036
> + Time of Flight front-end.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called addi9036.
> +
> 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 f0a77473979d..631a7c7612ca 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_VIDEO_AK7375) += ak7375.o
> obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
> obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
> obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
> +obj-$(CONFIG_VIDEO_ADDI9036) += addi9036.o
> obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
> obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o
> diff --git a/drivers/media/i2c/addi9036.c b/drivers/media/i2c/addi9036.c
> new file mode 100644
> index 000000000000..e38e70afd23d
> --- /dev/null
> +++ b/drivers/media/i2c/addi9036.c
> @@ -0,0 +1,754 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the Analog Devices ADDI9036 ToF camera sensor.
> + *
> + * Copyright (C) 2019-2020 Analog Devices, All Rights Reserved.
> + *
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/firmware.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define FW_FILE_NAME "adi/addi9036-fw.bin"
> +#define ADDI_MAGIC "ADDI9036"
> +
> +struct addi9036_mode_info {
> + u32 width;
> + u32 height;
> + u32 pixel_rate;
> + u32 link_freq_idx;
> +};
> +
> +struct addi9036_mode_fw_block {
> + const struct reg_sequence *mode_regs;
> + ssize_t regs_count;
> +};
> +
> +struct addi9036_fw_header {
> + unsigned char magic[8];
> + __le32 modes_nr;
> +} __packed;
> +
> +struct addi9036_mode_block {
> + __le32 mode_id;
> + __le32 size_bytes;
> + __le16 data[];
> +} __packed;
> +
> +struct addi9036 {
> + struct regmap *regmap;
> + struct device *dev;
> + struct v4l2_subdev sd;
> + struct media_pad pad;
> + struct v4l2_fwnode_endpoint ep;
> + struct v4l2_mbus_framefmt fmt;
> + struct v4l2_rect crop;
> +
> + const struct addi9036_mode_info *current_mode;
> +
> + struct v4l2_ctrl_handler ctrls;
> + struct v4l2_ctrl *pixel_rate;
> + struct v4l2_ctrl *link_freq;
> + /* custom controls */
> + struct v4l2_ctrl *set_operating_range;
> +
> + /* lock to protect power state */
> + struct mutex power_lock;
> + int power_count;
> + bool streaming;
> +
> + struct gpio_desc *rst_gpio;
> +
> + /* firmware blocks for each operating mode */
> + struct addi9036_mode_fw_block *mode_fw_blocks;
> + u8 curr_operating_mode;
> +
> + const struct firmware *fw;
> +};
> +
> +static inline struct addi9036 *to_addi9036(struct v4l2_subdev *sd)
> +{
> + return container_of(sd, struct addi9036, sd);
> +}
> +
> +#define V4L2_CID_ADDI9036_OPERATING_MODE (V4L2_CID_USER_ADDI9036_BASE + 0)
> +
> +static const struct reg_sequence addi9036_powerup_setting[] = {
> + { 0xc4c0, 0x001c },
> + { 0xc4c3, 0x001c },
> + { 0xc4d7, 0x0000 },
> + { 0xc4d5, 0x0002 },
> + { 0xc4da, 0x0001 },
> + { 0xc4f0, 0x0000 },
> + { 0xc427, 0x0003 },
> + { 0xc427, 0x0001 },
> + { 0xc427, 0x0000 },
> + { 0xc426, 0x0030 },
> + { 0xc426, 0x0010 },
> + { 0xc426, 0x0000 },
> + { 0xc423, 0x0080 },
> + { 0xc431, 0x0080 },
> + { 0x4001, 0x0007 },
> + { 0x7c22, 0x0004 }
> +};
> +
> +static const struct reg_sequence addi9036_powerdown_setting[] = {
> + { 0xc022, 0x0001 },
> + { 0x4001, 0x0006 },
> + { 0x7c22, 0x0004 },
> + { 0xc431, 0x0082 },
> + { 0xc423, 0x0000 },
> + { 0xc426, 0x0020 },
> + { 0xc427, 0x0002 },
> + { 0xc4c0, 0x003c },
> + { 0xc4c3, 0x003c },
> + { 0xc4d5, 0x0003 },
> + { 0xc4da, 0x0000 },
> + { 0xc4d7, 0x0001 },
> + { 0xc4f0, 0x0001 }
> +};
> +
> +static const s64 link_freq_tbl[] = {
> + 110529000,
> + 221184000,

Are these a hardware property or should they come from DT? Either way, not
both of them could be available on all boards, which suggests DT.

> +};
> +
> +/* Elements of the structure must be ordered ascending by width & height */
> +static const struct addi9036_mode_info addi9036_mode_info_data[] = {
> + {
> + .width = 640,
> + .height = 480,
> + .pixel_rate = 36864000,
> + .link_freq_idx = 0 /* an index in link_freq_tbl[] */
> + },
> + {
> + .width = 640,
> + .height = 960,
> + .pixel_rate = 73728000,
> + .link_freq_idx = 1 /* an index in link_freq_tbl[] */
> + },
> +};
> +
> +static bool addi9306_readable_register(struct device *dev, unsigned int reg)
> +{
> + if (((reg >= 0x4000) && (reg <= 0x6fff)) ||
> + ((reg >= 0x7c00) && (reg <= 0x7fff)) ||
> + ((reg >= 0xc000) && (reg <= 0xc200)) ||
> + ((reg >= 0xc300) && (reg <= 0xc6bf)))
> + return true;
> + else
> + return false;
> +}
> +
> +static bool addi9306_writeable_register(struct device *dev, unsigned int reg)
> +{
> + if (((reg >= 0x4000) && (reg <= 0x6fff)) ||
> + ((reg >= 0x7c00) && (reg <= 0x7fff)) ||
> + ((reg >= 0xc000) && (reg <= 0xc113)) ||
> + ((reg >= 0xc300) && (reg <= 0xc7ff)))
> + return true;
> + else
> + return false;
> +}
> +
> +static const struct regmap_config addi9036_i2c_regmap_config = {
> + .reg_bits = 16,
> + .val_bits = 16,
> + .max_register = 0xc7ff,
> + .writeable_reg = addi9306_writeable_register,
> + .readable_reg = addi9306_readable_register,
> + .cache_type = REGCACHE_NONE,
> +};
> +
> +static int addi9036_set_power_on(struct addi9036 *addi9036)
> +{
> + int ret;
> +
> + if (addi9036->rst_gpio)
> + gpiod_set_value_cansleep(addi9036->rst_gpio, 0);
> +
> + ret = regmap_register_patch(addi9036->regmap, addi9036_powerup_setting,
> + ARRAY_SIZE(addi9036_powerup_setting));
> + if (ret)
> + dev_err(addi9036->dev, "Could not set power up registers\n");
> +
> + return ret;
> +}
> +
> +static int addi9036_set_power_off(struct addi9036 *addi9036)
> +{
> + int ret;
> +
> + ret = regmap_register_patch(addi9036->regmap,
> + addi9036_powerdown_setting,
> + ARRAY_SIZE(addi9036_powerdown_setting));
> + if (ret)
> + dev_err(addi9036->dev, "could not set power down registers\n");
> +
> + if (addi9036->rst_gpio)
> + gpiod_set_value_cansleep(addi9036->rst_gpio, 1);
> +
> + return ret;
> +}
> +
> +static int addi9036_s_power(struct v4l2_subdev *sd, int on)
> +{
> + struct addi9036 *addi9036 = to_addi9036(sd);
> + int ret = 0;
> +
> + dev_dbg(addi9036->dev, "s_power: %d\n", on);
> +
> + mutex_lock(&addi9036->power_lock);
> +
> + /* If the power count is modified from 0 to != 0 or from != 0 to 0,
> + * update the power state.
> + */
> + if (addi9036->power_count == !on) {
> + if (on)
> + ret = addi9036_set_power_on(addi9036);
> + else
> + ret = addi9036_set_power_off(addi9036);
> + }
> +
> + if (!ret) {
> + /* Update the power count. */
> + addi9036->power_count += on ? 1 : -1;
> + WARN(addi9036->power_count < 0, "Unbalanced power count\n");
> + WARN(addi9036->power_count > 1, "Duplicated s_power call\n");
> + }
> +
> + mutex_unlock(&addi9036->power_lock);
> +
> + return ret;
> +}
> +
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static int addi9036_g_register(struct v4l2_subdev *sd,
> + struct v4l2_dbg_register *reg)
> +{
> + struct addi9036 *addi9036 = to_addi9036(sd);
> + unsigned int read_val;
> + int ret;
> +
> + reg->size = 2;
> + ret = regmap_read(addi9036->regmap, reg->reg, &read_val);
> + reg->val = read_val;
> +
> + return ret;
> +}
> +
> +static int addi9036_s_register(struct v4l2_subdev *sd,
> + const struct v4l2_dbg_register *reg)
> +{
> + struct addi9036 *addi9036 = to_addi9036(sd);
> +
> + return regmap_write(addi9036->regmap, reg->reg, reg->val);
> +}
> +#endif
> +
> +static int addi9036_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct addi9036 *addi9036 = container_of(ctrl->handler,
> + struct addi9036, ctrls);
> + int ret = 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_ADDI9036_OPERATING_MODE:
> + addi9036->curr_operating_mode = ctrl->val;
> + break;
> + case V4L2_CID_PIXEL_RATE:
> + case V4L2_CID_LINK_FREQ:
> + break;
> + default:
> + dev_err(addi9036->dev, "%s > Unhandled: %x param=%x\n",
> + __func__, ctrl->id, ctrl->val);
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops addi9036_ctrl_ops = {
> + .s_ctrl = addi9036_s_ctrl,
> +};
> +
> +static const struct v4l2_ctrl_config addi9036_ctrl_operating_mode = {
> + .ops = &addi9036_ctrl_ops,
> + .id = V4L2_CID_ADDI9036_OPERATING_MODE,
> + .name = "Operating Mode",
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .def = 0,
> + .min = 0,
> + .max = 1,
> + .step = 1,
> +};
> +
> +static int addi9036_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + if (code->index > 0)
> + return -EINVAL;
> +
> + code->code = MEDIA_BUS_FMT_SBGGR12_1X12;
> +
> + return 0;
> +}
> +
> +static int addi9036_enum_frame_size(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + if (fse->code != MEDIA_BUS_FMT_SBGGR12_1X12)
> + return -EINVAL;
> +
> + if (fse->index >= ARRAY_SIZE(addi9036_mode_info_data))
> + return -EINVAL;
> +
> + fse->min_width = addi9036_mode_info_data[fse->index].width;
> + fse->max_width = addi9036_mode_info_data[fse->index].width;
> + fse->min_height = addi9036_mode_info_data[fse->index].height;
> + fse->max_height = addi9036_mode_info_data[fse->index].height;
> +
> + return 0;
> +}
> +
> +static struct v4l2_mbus_framefmt *
> +addi9036_get_pad_format(struct addi9036 *addi9036,
> + struct v4l2_subdev_pad_config *cfg, unsigned int pad,
> + enum v4l2_subdev_format_whence which)
> +{
> + switch (which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> + return v4l2_subdev_get_try_format(&addi9036->sd, cfg, pad);
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + return &addi9036->fmt;
> + default:
> + return NULL;

I'd suggest never return NULL here. Maybe issue a warning and return either
of the valid ones? Or add NULL checks elsewhere.

> + }
> +}
> +
> +static int addi9036_get_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *format)
> +{
> + struct addi9036 *addi9036 = to_addi9036(sd);
> + struct v4l2_mbus_framefmt *pad_format;
> +
> + pad_format = addi9036_get_pad_format(addi9036, cfg, format->pad,
> + format->which);
> +
> + if (!pad_format)
> + return -EINVAL;
> +
> + format->format = *pad_format;
> +
> + return 0;
> +}
> +
> +static struct v4l2_rect *
> +addi9036_get_pad_crop(struct addi9036 *addi9036,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad, enum v4l2_subdev_format_whence which)
> +{
> + switch (which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> + return v4l2_subdev_get_try_crop(&addi9036->sd, cfg, pad);
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + return &addi9036->crop;
> + default:
> + return NULL;
> + }
> +}
> +
> +static int addi9036_set_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *format)
> +{
> + struct addi9036 *addi9036 = to_addi9036(sd);
> + struct v4l2_mbus_framefmt *framefmt;
> + struct v4l2_rect *crop;
> + const struct addi9036_mode_info *new_mode;
> + int ret;
> +
> + dev_dbg(addi9036->dev, "set_fmt: %x %dx%d %d\n",
> + format->format.code, format->format.width,
> + format->format.height, format->which);
> +
> + crop = addi9036_get_pad_crop(addi9036, cfg, format->pad,
> + format->which);
> +
> + if (!crop)
> + return -EINVAL;
> +
> + new_mode = v4l2_find_nearest_size(addi9036_mode_info_data,
> + ARRAY_SIZE(addi9036_mode_info_data),
> + width, height, format->format.width,
> + format->format.height);
> + crop->width = new_mode->width;
> + crop->height = new_mode->height;
> +
> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + ret = v4l2_ctrl_s_ctrl_int64(addi9036->pixel_rate,
> + new_mode->pixel_rate);
> + if (ret < 0)
> + return ret;
> +
> + ret = v4l2_ctrl_s_ctrl(addi9036->link_freq,
> + new_mode->link_freq_idx);
> + if (ret < 0)
> + return ret;
> +
> + addi9036->current_mode = new_mode;
> + }
> +
> + framefmt = addi9036_get_pad_format(addi9036, cfg, format->pad,
> + format->which);

I believe you'll need to serialise access to framefmt.

> + framefmt->width = crop->width;
> + framefmt->height = crop->height;
> + framefmt->code = MEDIA_BUS_FMT_SBGGR12_1X12;
> + framefmt->field = V4L2_FIELD_NONE;
> + framefmt->colorspace = V4L2_COLORSPACE_SRGB;
> +
> + format->format = *framefmt;
> +
> + return 0;
> +}
> +
> +static int addi9036_entity_init_cfg(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *cfg)
> +{
> + struct v4l2_subdev_format fmt = { 0 };
> +
> + fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> + fmt.format.width = addi9036_mode_info_data[1].width;
> + fmt.format.height = addi9036_mode_info_data[1].height;
> +
> + addi9036_set_format(subdev, cfg, &fmt);
> +
> + return 0;
> +}
> +
> +static int addi9036_get_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_selection *sel)
> +{
> + struct addi9036 *addi9036 = to_addi9036(sd);
> +
> + if (sel->target != V4L2_SEL_TGT_CROP)
> + return -EINVAL;
> +
> + sel->r = *addi9036_get_pad_crop(addi9036, cfg, sel->pad, sel->which);
> +
> + return 0;
> +}
> +
> +static int addi9036_s_stream(struct v4l2_subdev *subdev, int enable)
> +{
> + struct addi9036 *addi9036 = to_addi9036(subdev);
> + uint8_t mode = addi9036->curr_operating_mode;
> + int ret = 0;
> +
> + dev_dbg(addi9036->dev, "s_stream: %d\n", enable);
> +
> + if (addi9036->streaming == enable)
> + return 0;
> +
> + if (enable) {
> + if (addi9036->mode_fw_blocks[mode].mode_regs == NULL) {
> + dev_err(addi9036->dev, "Selected mode has no data\n");
> + return -EINVAL;
> + }
> +
> + dev_dbg(addi9036->dev, "Applying mode: %u\n", mode);
> + ret = regmap_multi_reg_write(addi9036->regmap,
> + addi9036->mode_fw_blocks[mode].mode_regs,
> + addi9036->mode_fw_blocks[mode].regs_count);
> +
> + dev_dbg(addi9036->dev, "Writen %lu registers\n",
> + addi9036->mode_fw_blocks[mode].regs_count);
> + }
> +
> + addi9036->streaming = enable;
> + return ret;
> +}
> +
> +static const struct v4l2_subdev_core_ops addi9036_core_ops = {
> + .s_power = addi9036_s_power,

Please add support for runtime PM instead.

> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> + .g_register = addi9036_g_register,
> + .s_register = addi9036_s_register,
> +#endif
> +};
> +
> +static const struct v4l2_subdev_video_ops addi9036_video_ops = {
> + .s_stream = addi9036_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops addi9036_subdev_pad_ops = {
> + .init_cfg = addi9036_entity_init_cfg,
> + .enum_mbus_code = addi9036_enum_mbus_code,
> + .enum_frame_size = addi9036_enum_frame_size,
> + .get_fmt = addi9036_get_format,
> + .set_fmt = addi9036_set_format,
> + .get_selection = addi9036_get_selection,
> +};
> +
> +static const struct v4l2_subdev_ops addi9036_subdev_ops = {
> + .core = &addi9036_core_ops,
> + .video = &addi9036_video_ops,
> + .pad = &addi9036_subdev_pad_ops,
> +};
> +
> +static int addi9036_g_modes_from_firmware(struct v4l2_subdev *sd)
> +{
> + struct addi9036 *addi9036 = to_addi9036(sd);
> + const struct firmware *fw = addi9036->fw;
> + const struct addi9036_fw_header *fw_head;
> + const struct addi9036_mode_block *mode_block;
> + unsigned int reg_nr, chunk_len, pos, modes_nr, i, j, k;
> + struct reg_sequence *reg_seq;
> +
> + /*
> + * Reject too small or unreasonable large files.
> + */
> +
> + if (fw->size < sizeof(struct addi9036_fw_header) ||
> + fw->size >= 0x4000000) {
> + dev_err(addi9036->dev, "FW loading failed: Invalid size\n");
> + return -EINVAL;
> + }
> +
> + fw_head = (struct addi9036_fw_header *)fw->data;
> +
> + if (memcmp(fw_head->magic, ADDI_MAGIC, ARRAY_SIZE(fw_head->magic))) {
> + dev_err(addi9036->dev, "FW loading failed: Invalid magic\n");
> + return -EINVAL;
> + }
> +
> + modes_nr = le32_to_cpu(fw_head->modes_nr);
> +
> + if (modes_nr == 0) {
> + dev_err(addi9036->dev, "FW should contain at least 1 mode.\n");
> + return -EINVAL;
> + }
> +
> + __v4l2_ctrl_modify_range(addi9036->set_operating_range,
> + addi9036->set_operating_range->minimum,
> + modes_nr - 1, 1, 0);
> +
> + addi9036->mode_fw_blocks = devm_kzalloc(addi9036->dev,
> + sizeof(struct addi9036_mode_fw_block) * modes_nr,

devm_kcalloc, please.

> + GFP_KERNEL);
> + if (!addi9036->mode_fw_blocks)
> + return -ENOMEM;
> +
> + pos = sizeof(struct addi9036_fw_header);
> +
> + for (i = 0; i < modes_nr; i++) {
> + mode_block = (struct addi9036_mode_block *)(fw->data + pos);

I think you need more validation here. For instance, make sure the firmware
binary is at least as big as the offset you're accessing.

> +
> + chunk_len = le32_to_cpu(mode_block->size_bytes);
> + reg_nr = chunk_len / sizeof(uint16_t) / 2;
> +
> + reg_seq = devm_kzalloc(addi9036->dev,
> + sizeof(struct reg_sequence) * reg_nr,
> + GFP_KERNEL);
> + if (!reg_seq)
> + return -ENOMEM;
> +
> + k = 0;
> + for (j = 0; j < reg_nr * 2; j += 2) {

j = 0, k = 0

> + reg_seq[k].reg = le16_to_cpu(mode_block->data[j]);
> + reg_seq[k].def = le16_to_cpu(mode_block->data[j + 1]);
> + k++;
> + }
> +
> + addi9036->mode_fw_blocks[i].mode_regs = reg_seq;
> + addi9036->mode_fw_blocks[i].regs_count = reg_nr;
> +
> + pos += chunk_len + sizeof(struct addi9036_mode_block);
> + }
> + return 0;
> +}
> +
> +static int addi9036_mode_firmware_load(struct v4l2_subdev *sd)
> +{
> + struct addi9036 *addi9036 = to_addi9036(sd);
> + int ret;
> +
> + ret = request_firmware(&addi9036->fw, FW_FILE_NAME, addi9036->dev);
> + if (ret < 0) {
> + dev_err(addi9036->dev, "FW request failed\n");
> + return ret;
> + }
> +
> + ret = addi9036_g_modes_from_firmware(sd);
> +
> + release_firmware(addi9036->fw);
> + if (ret < 0) {
> + dev_err(addi9036->dev, "FW parsing failed\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int addi9036_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct fwnode_handle *endpoint;
> + struct addi9036 *addi9036;
> + int ret;
> +
> + dev_dbg(dev, "%s: i2c addr = 0x%x\n", __func__, client->addr);
> +
> + addi9036 = devm_kzalloc(dev, sizeof(struct addi9036), GFP_KERNEL);
> + if (!addi9036)
> + return -ENOMEM;
> +
> + addi9036->dev = dev;
> +
> + addi9036->regmap = devm_regmap_init_i2c(client,
> + &addi9036_i2c_regmap_config);
> + if (IS_ERR(addi9036->regmap)) {
> + dev_err(dev, "Error initializing i2c regmap\n");
> + return PTR_ERR(addi9036->regmap);
> + }
> +
> + mutex_init(&addi9036->power_lock);
> +
> + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);

Could you use fwnode_graph_get_endpoint_by_id()?

> + if (!endpoint) {
> + dev_err(dev, "endpoint node not found\n");
> + return -EINVAL;
> + }
> +
> + ret = v4l2_fwnode_endpoint_parse(endpoint, &addi9036->ep);
> + if (ret < 0) {
> + dev_err(dev, "parsing endpoint node failed\n");
> + return ret;
> + }
> + fwnode_handle_put(endpoint);
> +
> + if (addi9036->ep.bus_type != V4L2_MBUS_CSI2_DPHY) {

If you expect D-PHY, please specify the bus type before calling
v4l2_fwnode_endpoint_parse().

> + dev_err(dev, "invalid bus type, must be MIPI CSI2\n");
> + return -EINVAL;
> + }
> +
> + addi9036->rst_gpio = gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(addi9036->rst_gpio))
> + dev_info(dev, "Unable to get \"reset\" gpio\n");
> +
> + v4l2_ctrl_handler_init(&addi9036->ctrls, 3);
> +
> + addi9036->pixel_rate = v4l2_ctrl_new_std(&addi9036->ctrls,
> + &addi9036_ctrl_ops,
> + V4L2_CID_PIXEL_RATE,
> + 1, INT_MAX, 1, 1);
> + addi9036->link_freq = v4l2_ctrl_new_int_menu(&addi9036->ctrls,
> + &addi9036_ctrl_ops,
> + V4L2_CID_LINK_FREQ,
> + ARRAY_SIZE(
> + link_freq_tbl) - 1,
> + 0, link_freq_tbl);
> + if (addi9036->link_freq)
> + addi9036->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + addi9036->set_operating_range = v4l2_ctrl_new_custom(&addi9036->ctrls,
> + &addi9036_ctrl_operating_mode,
> + NULL);
> +
> + ret = addi9036->ctrls.error;
> + if (ret) {
> + dev_err(dev, "%s: control initialization error %d\n",
> + __func__, ret);
> + goto free_ctrl;
> + }
> + addi9036->sd.ctrl_handler = &addi9036->ctrls;
> +
> + v4l2_i2c_subdev_init(&addi9036->sd, client, &addi9036_subdev_ops);
> + addi9036->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + addi9036->pad.flags = MEDIA_PAD_FL_SOURCE;
> + addi9036->sd.dev = &client->dev;
> + addi9036->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> + ret = media_entity_pads_init(&addi9036->sd.entity, 1, &addi9036->pad);
> + if (ret < 0) {
> + dev_err(dev, "could not register media entity\n");
> + goto free_ctrl;
> + }
> +
> + ret = addi9036_mode_firmware_load(&addi9036->sd);
> + if (ret < 0)
> + return ret;
> +
> + addi9036_entity_init_cfg(&addi9036->sd, NULL);
> +
> + ret = v4l2_async_register_subdev(&addi9036->sd);
> + if (ret < 0) {
> + dev_err(dev, "could not register v4l2 device\n");
> + goto free_entity;
> + }
> +
> + return 0;
> +
> +free_entity:
> + media_entity_cleanup(&addi9036->sd.entity);
> +free_ctrl:
> + v4l2_ctrl_handler_free(&addi9036->ctrls);
> + mutex_destroy(&addi9036->power_lock);
> +
> + return ret;
> +}
> +
> +static int addi9036_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct addi9036 *addi9036 = to_addi9036(sd);
> +
> + v4l2_async_unregister_subdev(&addi9036->sd);
> + media_entity_cleanup(&addi9036->sd.entity);
> + if (addi9036->rst_gpio)
> + gpiod_put(addi9036->rst_gpio);
> + v4l2_ctrl_handler_free(&addi9036->ctrls);
> + mutex_destroy(&addi9036->power_lock);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id addi9036_id[] = {

Do you really need the I²C device table? Please remove if not.

> + { "addi9036", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, addi9036_id);
> +
> +static const struct of_device_id addi9036_of_match[] = {
> + { .compatible = "adi,addi9036" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, addi9036_of_match);
> +
> +static struct i2c_driver addi9036_i2c_driver = {
> + .driver = {
> + .of_match_table = addi9036_of_match,
> + .name = "addi9036",
> + },
> + .probe_new = addi9036_probe,
> + .remove = addi9036_remove,
> + .id_table = addi9036_id,
> +};
> +
> +module_i2c_driver(addi9036_i2c_driver);
> +
> +MODULE_DESCRIPTION("Analog Devices ADDI9036 Camera Driver");
> +MODULE_AUTHOR("Bogdan Togorean");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 62271418c1be..f88b56479bc1 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -198,6 +198,12 @@ enum v4l2_colorfx {
> */
> #define V4L2_CID_USER_ATMEL_ISC_BASE (V4L2_CID_USER_BASE + 0x10c0)
>
> +/*
> + * The base for the addi9036 driver controls.
> + * We reserve 16 controls for this driver.
> + */
> +#define V4L2_CID_USER_ADDI9036_BASE (V4L2_CID_USER_BASE + 0x10e0)
> +
> /* MPEG-class control IDs */
> /* The MPEG controls are applicable to all codec controls
> * and the 'MPEG' part of the define is historical */

--
Sakari Ailus

Next message: Joel Stanley: "Re: [PATCH 1/3] configs: aspeed: enable UHCI driver in defconfig"
Previous message: Borislav Petkov: "Re: [tip: ras/core] x86, powerpc: Rename memcpy_mcsafe() to copy_mc_to_{user, kernel}()"
In reply to: Rob Herring: "Re: [PATCH v2 2/2] media: dt-bindings: media: i2c: Add bindings for ADDI9036"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]