From 0d346d2a6f54f06f36b224fd27cd6eafe8c83be9 Mon Sep 17 00:00:00 2001 From: Tomi Valkeinen Date: Thu, 10 Jun 2021 17:55:58 +0300 Subject: media: v4l2-subdev: add subdev-wide state struct We have 'struct v4l2_subdev_pad_config' which contains configuration for a single pad used for the TRY functionality, and an array of those structs is passed to various v4l2_subdev_pad_ops. I was working on subdev internal routing between pads, and realized that there's no way to add TRY functionality for routes, which is not pad specific configuration. Adding a separate struct for try-route config wouldn't work either, as e.g. set-fmt needs to know the try-route configuration to propagate the settings. This patch adds a new struct, 'struct v4l2_subdev_state' (which at the moment only contains the v4l2_subdev_pad_config array) and the new struct is used in most of the places where v4l2_subdev_pad_config was used. All v4l2_subdev_pad_ops functions taking v4l2_subdev_pad_config are changed to instead take v4l2_subdev_state. The changes to drivers/media/v4l2-core/v4l2-subdev.c and include/media/v4l2-subdev.h were written by hand, and all the driver changes were done with the semantic patch below. The spatch needs to be applied to a select list of directories. I used the following shell commands to apply the spatch: dirs="drivers/media/i2c drivers/media/platform drivers/media/usb drivers/media/test-drivers/vimc drivers/media/pci drivers/staging/media" for dir in $dirs; do spatch -j8 --dir --include-headers --no-show-diff --in-place --sp-file v4l2-subdev-state.cocci $dir; done Note that Coccinelle chokes on a few drivers (gcc extensions?). With minor changes we can make Coccinelle run fine, and these changes can be reverted after spatch. The diff for these changes is: For drivers/media/i2c/s5k5baf.c: @@ -1481,7 +1481,7 @@ static int s5k5baf_set_selection(struct v4l2_subdev *sd, &s5k5baf_cis_rect, v4l2_subdev_get_try_crop(sd, cfg, PAD_CIS), v4l2_subdev_get_try_compose(sd, cfg, PAD_CIS), - v4l2_subdev_get_try_crop(sd, cfg, PAD_OUT) + v4l2_subdev_get_try_crop(sd, cfg, PAD_OUT), }; s5k5baf_set_rect_and_adjust(rects, rtype, &sel->r); return 0; For drivers/media/platform/s3c-camif/camif-capture.c: @@ -1230,7 +1230,7 @@ static int s3c_camif_subdev_get_fmt(struct v4l2_subdev *sd, *mf = camif->mbus_fmt; break; - case CAMIF_SD_PAD_SOURCE_C...CAMIF_SD_PAD_SOURCE_P: + case CAMIF_SD_PAD_SOURCE_C: /* crop rectangle at camera interface input */ mf->width = camif->camif_crop.width; mf->height = camif->camif_crop.height; @@ -1332,7 +1332,7 @@ static int s3c_camif_subdev_set_fmt(struct v4l2_subdev *sd, } break; - case CAMIF_SD_PAD_SOURCE_C...CAMIF_SD_PAD_SOURCE_P: + case CAMIF_SD_PAD_SOURCE_C: /* Pixel format can be only changed on the sink pad. */ mf->code = camif->mbus_fmt.code; mf->width = crop->width; The semantic patch is: // // Change function parameter @@ identifier func; identifier cfg; @@ func(..., - struct v4l2_subdev_pad_config *cfg + struct v4l2_subdev_state *sd_state , ...) { <... - cfg + sd_state ...> } // Change function declaration parameter @@ identifier func; identifier cfg; type T; @@ T func(..., - struct v4l2_subdev_pad_config *cfg + struct v4l2_subdev_state *sd_state , ...); // Change function return value @@ identifier func; @@ - struct v4l2_subdev_pad_config + struct v4l2_subdev_state *func(...) { ... } // Change function declaration return value @@ identifier func; @@ - struct v4l2_subdev_pad_config + struct v4l2_subdev_state *func(...); // Some drivers pass a local pad_cfg for a single pad to a called function. Wrap it // inside a pad_state. @@ identifier func; identifier pad_cfg; @@ func(...) { ... struct v4l2_subdev_pad_config pad_cfg; + struct v4l2_subdev_state pad_state = { .pads = &pad_cfg }; <+... ( v4l2_subdev_call | sensor_call | isi_try_fse | isc_try_fse | saa_call_all ) (..., - &pad_cfg + &pad_state ,...) ...+> } // If the function uses fields from pad_config, access via state->pads @@ identifier func; identifier state; @@ func(..., struct v4l2_subdev_state *state , ...) { <... ( - state->try_fmt + state->pads->try_fmt | - state->try_crop + state->pads->try_crop | - state->try_compose + state->pads->try_compose ) ...> } // If the function accesses the filehandle, use fh->state instead @@ struct v4l2_subdev_fh *fh; @@ - fh->pad + fh->state @@ struct v4l2_subdev_fh fh; @@ - fh.pad + fh.state // Start of vsp1 specific @@ @@ struct vsp1_entity { ... - struct v4l2_subdev_pad_config *config; + struct v4l2_subdev_state *config; ... }; @@ symbol entity; @@ vsp1_entity_init(...) { ... entity->config = - v4l2_subdev_alloc_pad_config + v4l2_subdev_alloc_state (&entity->subdev); ... } @@ symbol entity; @@ vsp1_entity_destroy(...) { ... - v4l2_subdev_free_pad_config + v4l2_subdev_free_state (entity->config); ... } @exists@ identifier func =~ "(^vsp1.*)|(hsit_set_format)|(sru_enum_frame_size)|(sru_set_format)|(uif_get_selection)|(uif_set_selection)|(uds_enum_frame_size)|(uds_set_format)|(brx_set_format)|(brx_get_selection)|(histo_get_selection)|(histo_set_selection)|(brx_set_selection)"; symbol config; @@ func(...) { ... - struct v4l2_subdev_pad_config *config; + struct v4l2_subdev_state *config; ... } // End of vsp1 specific // Start of rcar specific @@ identifier sd; identifier pad_cfg; @@ rvin_try_format(...) { ... - struct v4l2_subdev_pad_config *pad_cfg; + struct v4l2_subdev_state *sd_state; ... - pad_cfg = v4l2_subdev_alloc_pad_config(sd); + sd_state = v4l2_subdev_alloc_state(sd); <... - pad_cfg + sd_state ...> - v4l2_subdev_free_pad_config(pad_cfg); + v4l2_subdev_free_state(sd_state); ... } // End of rcar specific // Start of rockchip specific @@ identifier func =~ "(rkisp1_rsz_get_pad_fmt)|(rkisp1_rsz_get_pad_crop)|(rkisp1_rsz_register)"; symbol rsz; symbol pad_cfg; @@ func(...) { + struct v4l2_subdev_state state = { .pads = rsz->pad_cfg }; ... - rsz->pad_cfg + &state ... } @@ identifier func =~ "(rkisp1_isp_get_pad_fmt)|(rkisp1_isp_get_pad_crop)"; symbol isp; symbol pad_cfg; @@ func(...) { + struct v4l2_subdev_state state = { .pads = isp->pad_cfg }; ... - isp->pad_cfg + &state ... } @@ symbol rkisp1; symbol isp; symbol pad_cfg; @@ rkisp1_isp_register(...) { + struct v4l2_subdev_state state = { .pads = rkisp1->isp.pad_cfg }; ... - rkisp1->isp.pad_cfg + &state ... } // End of rockchip specific // Start of tegra-video specific @@ identifier sd; identifier pad_cfg; @@ __tegra_channel_try_format(...) { ... - struct v4l2_subdev_pad_config *pad_cfg; + struct v4l2_subdev_state *sd_state; ... - pad_cfg = v4l2_subdev_alloc_pad_config(sd); + sd_state = v4l2_subdev_alloc_state(sd); <... - pad_cfg + sd_state ...> - v4l2_subdev_free_pad_config(pad_cfg); + v4l2_subdev_free_state(sd_state); ... } @@ identifier sd_state; @@ __tegra_channel_try_format(...) { ... struct v4l2_subdev_state *sd_state; <... - sd_state->try_crop + sd_state->pads->try_crop ...> } // End of tegra-video specific // Signed-off-by: Tomi Valkeinen Acked-by: Laurent Pinchart Acked-by: Sakari Ailus Signed-off-by: Hans Verkuil Signed-off-by: Mauro Carvalho Chehab --- drivers/media/i2c/rdacm21.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/media/i2c/rdacm21.c') diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index 179d107f494c..5b78d8185773 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -281,7 +281,7 @@ static int rdacm21_s_stream(struct v4l2_subdev *sd, int enable) } static int rdacm21_enum_mbus_code(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_state *sd_state, struct v4l2_subdev_mbus_code_enum *code) { if (code->pad || code->index > 0) @@ -293,7 +293,7 @@ static int rdacm21_enum_mbus_code(struct v4l2_subdev *sd, } static int rdacm21_get_fmt(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_state *sd_state, struct v4l2_subdev_format *format) { struct v4l2_mbus_framefmt *mf = &format->format; -- cgit v1.2.3-58-ga151 From 9e0bf8393d0602cc7fda749b77cf8ec7f81249cb Mon Sep 17 00:00:00 2001 From: Jacopo Mondi Date: Wed, 16 Jun 2021 14:46:08 +0200 Subject: media: i2c: max9271: Introduce wake_up() function The MAX9271 chip manual prescribes a delay of 5 milliseconds after the chip exits from low power state. Add a new function to the max9271 library driver that wakes up the chip with a dummy i2c transaction and implements the correct delay of 5 milliseconds after the chip exits from low power state. Use the newly introduced function in the rdacm20 and rdacm21 camera drivers. The former was not respecting the required delay while the latter was waiting for a too-short timeout. Signed-off-by: Jacopo Mondi Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Hans Verkuil Signed-off-by: Mauro Carvalho Chehab --- drivers/media/i2c/max9271.c | 12 ++++++++++++ drivers/media/i2c/max9271.h | 9 +++++++++ drivers/media/i2c/rdacm20.c | 4 +--- drivers/media/i2c/rdacm21.c | 5 +---- 4 files changed, 23 insertions(+), 7 deletions(-) (limited to 'drivers/media/i2c/rdacm21.c') diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c index 2c7dc7fb9846..ff86c8c4ea61 100644 --- a/drivers/media/i2c/max9271.c +++ b/drivers/media/i2c/max9271.c @@ -80,6 +80,18 @@ static int max9271_pclk_detect(struct max9271_device *dev) return -EIO; } +void max9271_wake_up(struct max9271_device *dev) +{ + /* + * Use the chip default address as this function has to be called + * before any other one. + */ + dev->client->addr = MAX9271_DEFAULT_ADDR; + i2c_smbus_read_byte(dev->client); + usleep_range(5000, 8000); +} +EXPORT_SYMBOL_GPL(max9271_wake_up); + int max9271_set_serial_link(struct max9271_device *dev, bool enable) { int ret; diff --git a/drivers/media/i2c/max9271.h b/drivers/media/i2c/max9271.h index d78fb21441e9..dc5e4e70ba6f 100644 --- a/drivers/media/i2c/max9271.h +++ b/drivers/media/i2c/max9271.h @@ -85,6 +85,15 @@ struct max9271_device { struct i2c_client *client; }; +/** + * max9271_wake_up() - Wake up the serializer by issuing an i2c transaction + * @dev: The max9271 device + * + * This function shall be called before any other interaction with the + * serializer. + */ +void max9271_wake_up(struct max9271_device *dev); + /** * max9271_set_serial_link() - Enable/disable serial link * @dev: The max9271 device diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c index a4b639cf8063..ecd8bf97aae1 100644 --- a/drivers/media/i2c/rdacm20.c +++ b/drivers/media/i2c/rdacm20.c @@ -455,9 +455,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev) unsigned int retry = 3; int ret; - /* Verify communication with the MAX9271: ping to wakeup. */ - dev->serializer->client->addr = MAX9271_DEFAULT_ADDR; - i2c_smbus_read_byte(dev->serializer->client); + max9271_wake_up(dev->serializer); /* Serial link disabled during config as it needs a valid pixel clock. */ ret = max9271_set_serial_link(dev->serializer, false); diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index 5b78d8185773..67ed1e5c450d 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -450,10 +450,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev) { int ret; - /* Verify communication with the MAX9271: ping to wakeup. */ - dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; - i2c_smbus_read_byte(dev->serializer.client); - usleep_range(3000, 5000); + max9271_wake_up(&dev->serializer); /* Enable reverse channel and disable the serial link. */ ret = max9271_set_serial_link(&dev->serializer, false); -- cgit v1.2.3-58-ga151 From 7028772092b7f2fc50de7f00aa0817505b3b11f7 Mon Sep 17 00:00:00 2001 From: Jacopo Mondi Date: Wed, 16 Jun 2021 14:46:09 +0200 Subject: media: i2c: rdacm21: Add delay after OV490 reset Add a delay after the OV490 chip is put in reset state. The reset signal shall be held low for at least 250 useconds. Signed-off-by: Jacopo Mondi Reviewed-by: Laurent Pinchart Reviewed-by: Kieran Bingham Signed-off-by: Hans Verkuil Signed-off-by: Mauro Carvalho Chehab --- drivers/media/i2c/rdacm21.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'drivers/media/i2c/rdacm21.c') diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index 67ed1e5c450d..336fd5d482b7 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -469,7 +469,10 @@ static int rdacm21_initialize(struct rdacm21_device *dev) if (ret) return ret; - /* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */ + /* + * Enable GPIO1 and hold OV490 in reset during max9271 configuration. + * The reset signal has to be asserted for at least 250 useconds. + */ ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT); if (ret) return ret; @@ -477,6 +480,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev) ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT); if (ret) return ret; + usleep_range(250, 500); ret = max9271_configure_gmsl_link(&dev->serializer); if (ret) -- cgit v1.2.3-58-ga151 From ff75332b260cd33cc19000fdb5d256d9db4470d1 Mon Sep 17 00:00:00 2001 From: Jacopo Mondi Date: Wed, 16 Jun 2021 14:46:10 +0200 Subject: media: i2c: rdacm21: Fix OV10640 powerup The OV10640 image sensor powerdown signal is controlled by the first line of the OV490 GPIO pad #1, but the pad #0 identifier OV490_GPIO_OUTPUT_VALUE0 was erroneously used. As a result the image sensor powerdown signal was never asserted but was left floating and kept high by an internal pull-up resistor, causing sporadic failures during the image sensor startup phase. Fix this by using the correct GPIO pad identifier and wait the mandatory 1.5 millisecond delay after the powerup lane is asserted. The reset delay is not characterized in the chip manual if not as "255 XVCLK + initialization". Wait for at least 3 milliseconds to guarantee the SCCB bus is available. While at it also fix the reset sequence, as the reset line was released before the powerdown one, and the line was not cycled. This commit fixes a sporadic start-up error triggered by a failure to read the OV10640 chip ID: rdacm21 8-0054: OV10640 ID mismatch: (0x01) Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module") Signed-off-by: Jacopo Mondi Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Hans Verkuil Signed-off-by: Mauro Carvalho Chehab --- drivers/media/i2c/rdacm21.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'drivers/media/i2c/rdacm21.c') diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index 336fd5d482b7..6ab8a894e94b 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -333,13 +333,19 @@ static int ov10640_initialize(struct rdacm21_device *dev) { u8 val; - /* Power-up OV10640 by setting RESETB and PWDNB pins high. */ + /* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */ ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0); ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0); ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0); ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0); + + /* Power up OV10640 and then reset it. */ + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE1, OV490_SPWDN0); + usleep_range(1500, 3000); + + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, 0x00); + usleep_range(1500, 3000); ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); - ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0); usleep_range(3000, 5000); /* Read OV10640 ID to test communications. */ -- cgit v1.2.3-58-ga151 From 2b821698dc73c00719e3dc367db712f727bbda85 Mon Sep 17 00:00:00 2001 From: Jacopo Mondi Date: Wed, 16 Jun 2021 14:46:11 +0200 Subject: media: i2c: rdacm21: Power up OV10640 before OV490 The current RDACM21 initialization routine powers up the OV10640 image sensor after the OV490 ISP. The ISP is programmed with a firmware loaded from an embedded serial flash that (most probably) tries to interact and program also the image sensor connected to the ISP. As described in commit "media: i2c: rdacm21: Fix OV10640 powerup" the image sensor powerdown signal is kept high by an internal pull up resistor and occasionally fails to startup correctly if the powerdown line is not asserted explicitly. Failures in the OV10640 startup causes the OV490 firmware to fail to boot correctly resulting in the camera module initialization to fail consequentially. Fix this by powering up the OV10640 image sensor before testing the OV490 firmware boot completion, by splitting the ov10640_initialize() function in an ov10640_power_up() one and an ov10640_check_id() one. Also make sure the OV10640 identification procedure gives enough time to the image sensor to resume after the programming phase performed by the OV490 firmware by repeating the ID read procedure. This commit fixes a sporadic start-up error triggered by a failure to detect the OV490 firmware boot completion: rdacm21 8-0054: Timeout waiting for firmware boot [hverkuil: fixed two typos in commit log] Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module") Signed-off-by: Jacopo Mondi Reviewed-by: Laurent Pinchart Reviewed-by: Kieran Bingham Signed-off-by: Hans Verkuil Signed-off-by: Mauro Carvalho Chehab --- drivers/media/i2c/rdacm21.c | 46 +++++++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 14 deletions(-) (limited to 'drivers/media/i2c/rdacm21.c') diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index 6ab8a894e94b..12ec5467ed1e 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -69,6 +69,7 @@ #define OV490_ISP_VSIZE_LOW 0x80820062 #define OV490_ISP_VSIZE_HIGH 0x80820063 +#define OV10640_PID_TIMEOUT 20 #define OV10640_ID_HIGH 0xa6 #define OV10640_CHIP_ID 0x300a #define OV10640_PIXEL_RATE 55000000 @@ -329,10 +330,8 @@ static const struct v4l2_subdev_ops rdacm21_subdev_ops = { .pad = &rdacm21_subdev_pad_ops, }; -static int ov10640_initialize(struct rdacm21_device *dev) +static void ov10640_power_up(struct rdacm21_device *dev) { - u8 val; - /* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */ ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0); ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0); @@ -347,18 +346,35 @@ static int ov10640_initialize(struct rdacm21_device *dev) usleep_range(1500, 3000); ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); usleep_range(3000, 5000); +} - /* Read OV10640 ID to test communications. */ - ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ); - ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8); - ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID & 0xff); - - /* Trigger SCCB slave transaction and give it some time to complete. */ - ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER); - usleep_range(1000, 1500); +static int ov10640_check_id(struct rdacm21_device *dev) +{ + unsigned int i; + u8 val; - ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val); - if (val != OV10640_ID_HIGH) { + /* Read OV10640 ID to test communications. */ + for (i = 0; i < OV10640_PID_TIMEOUT; ++i) { + ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, + OV490_SCCB_SLAVE_READ); + ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, + OV10640_CHIP_ID >> 8); + ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, + OV10640_CHIP_ID & 0xff); + + /* + * Trigger SCCB slave transaction and give it some time + * to complete. + */ + ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER); + usleep_range(1000, 1500); + + ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val); + if (val == OV10640_ID_HIGH) + break; + usleep_range(1000, 1500); + } + if (i == OV10640_PID_TIMEOUT) { dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val); return -ENODEV; } @@ -374,6 +390,8 @@ static int ov490_initialize(struct rdacm21_device *dev) unsigned int i; int ret; + ov10640_power_up(dev); + /* * Read OV490 Id to test communications. Give it up to 40msec to * exit from reset. @@ -411,7 +429,7 @@ static int ov490_initialize(struct rdacm21_device *dev) return -ENODEV; } - ret = ov10640_initialize(dev); + ret = ov10640_check_id(dev); if (ret) return ret; -- cgit v1.2.3-58-ga151