IRS1125 sensor driver updates #3544

Coimbra1984 · 2020-04-14T11:20:44Z

Changed a few functions:

i2c_transfer used for reading
minor refactoring including renaming
V4L2 control for safe reconfiguration changed to allow individual access for each sequence
Reconfiguration of modulation PLLs after imager reset fixed

6by9

A couple of comments for you to consider. You are the only users of this driver, therefore I'm not going to fuss over them excessively. Should you ever try to upstream this then you will get the same sort of queries there, and breaking backwards compatibility would be a big no-no.

6by9 · 2020-04-14T11:59:13Z

drivers/media/i2c/irs1125.h

+#define IRS1125_CID_SEQ_CONFIG		(IRS1125_CID_CUSTOM_BASE + 46)
+#define IRS1125_CID_IDENT0		(IRS1125_CID_CUSTOM_BASE + 47)
+#define IRS1125_CID_IDENT1		(IRS1125_CID_CUSTOM_BASE + 48)
+#define IRS1125_CID_IDENT2		(IRS1125_CID_CUSTOM_BASE + 49)


You are the only user of this driver at present, but rewriting all the control CIDs like this means you have totally destroyed any backwards compatibility.
Generally speaking you should retain the same CID where possible, deprectate (and not reuse) those which are obsolete, and add new controls to the end. A user app can then know that the control matches the headers that it has.

Sure, you are right, of course.

6by9 · 2020-04-14T12:08:59Z

drivers/media/i2c/irs1125.c

@@ -29,6 +29,7 @@
 #include <media/v4l2-image-sizes.h>
 #include <media/v4l2-mediabus.h>
 #include <media/v4l2-ctrls.h>
+#include <asm/unaligned.h>


Includes should normally be in alhpabetical order.

Okay, I didn't know that

6by9 · 2020-04-14T12:09:57Z

drivers/media/i2c/irs1125.c


-	*val = rdval[1] | (rdval[0] << 8);
+	*val = get_unaligned_be16(data_buf);


Personally I prefer the explicit assignment rather than the use of the be16 functions here as the reassembly isn't down to CPU endianness. Others seem to use them in drivers/media/i2c though, so I'm not going to fuss.

No problem, I'll use the previous version.

6by9

A few concerns/comments.

The commit texts could be more descriptive too.

6by9 · 2020-06-09T07:58:24Z

drivers/media/i2c/irs1125.c

-		dev_err(&client->dev, "%s: i2c read error, reg: %x\n",
-			__func__, reg);
+	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret != ARRAY_SIZE(msgs))


https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L1987
"Returns negative errno, else the number of messages executed."
So this masks any error number and will always return -EIO on failure.

if (ret != ARRAY_SIZE(msgs)) { if (ret >= 0) ret = -EIO; return ret; }

You are right. This should be fixed now

6by9 · 2020-06-09T08:05:57Z

drivers/media/i2c/irs1125.c

-			}
+			addr = 0xC3A0 + i * 3;
+			val = mod_new[i].pllcfg1;
+			err = irs1125_write(&dev->sd, addr, val);


Seeing as addr is only used the once, the calculation may as well be in the call to irs1125_write
Likewise you only use val once.
err = irs1125_write(&dev->sd, 0xC3A0 + i * 3, mod_new[i].pllcfg1);

Same all through this function.

There could be a minor optimisation in computing i3 (and i5) once per loop, and then using that as a pll_offset value to be added in each call.

right, done!

6by9 · 2020-06-09T08:13:26Z

drivers/media/i2c/irs1125.c

+
+		snprintf(name, sizeof(name),
+			 "Safe reconfiguration of framerate of sequence %d", i);
+		ctrl_cfg.id += 1;


ctrl_cfg.id++; would be more normal.

I changed that now

6by9 · 2020-06-09T09:29:31Z

drivers/media/i2c/irs1125.c

+			dev_err(dev, "Failed to init custom control %s\n",
+				ctrl_cfg.name);
+
+		snprintf(name, sizeof(name),


This has me worried.
struct v4l2_ctrl only has a const char *name you're passing in a stack allocated pointer, and I don't believe anything in the framework is going to take a persistent copy of that. That means that name is pointer into random stuff, and is the same for all of your controls.

Does v4l2-ctl --list-ctrls actually list out the right thing?
I suspect not, and you really need two arrays of const char* names, with IRS1125_NUM_SEQ_ENTRIES entries in each.

Doing that would also mean that it'd be neater to have two loops, one for adding the exposure ctrls, and one for the framerate ones. Initialise the struct v4l2_ctrl_config outside the loop, and then insert id and pointer to the name. i+=2 in the loop increment.

You are right, the ctrl did not show up as expected. I changed this to use const char* arrays now.

I tested it again with v4l2-ctl --lsit-ctrls and it shows the expected results now.

6by9 · 2020-06-09T09:32:02Z

drivers/media/i2c/irs1125.c

@@ -748,33 +759,22 @@ static const struct v4l2_ctrl_config irs1125_custom_ctrls[] = {
 		.id = IRS1125_CID_MOD_PLL,
 		.name = "Reconfigure modulation PLLs",
 		.type = V4L2_CTRL_TYPE_U16,
-		.flags = V4L2_CTRL_FLAG_HAS_PAYLOAD,
+		.flags = V4L2_CTRL_FLAG_HAS_PAYLOAD |
+			V4L2_CTRL_FLAG_EXECUTE_ON_WRITE,


Do you really need to write it every time, even if it hasn't changed? I'm willing to believe it, but it's slightly unusual.

No this is not needed, you are right.

6by9 · 2020-06-09T09:33:30Z

drivers/media/i2c/irs1125.c

+		int s;

+		s = sizeof(struct irs1125_mod_pll) * IRS1125_NUM_MOD_PLLS;


Can be done as the one line
int s = sizeof(struct irs1125_mod_pll) * IRS1125_NUM_MOD_PLLS;
(line wrap at the * if it's over 80 chars)

Sure, this is changed now

6by9 · 2020-06-09T09:35:15Z

drivers/media/i2c/irs1125.c

+			}
+		}
+		if (!valid)
+			break;


Do we not want to return an error if invalid?

I now skip only writing this control in case of internal sync.

6by9 · 2020-06-09T09:43:49Z

drivers/media/i2c/irs1125.c

+		for (pval = (u8 *)mod_new; pval < (u8 *)mod_new + s; pval++) {
+			if (*pval != 0) {
+				valid = true;
+				break;
+			}
+		}


I'm a little confused by the validity test.
If any one 8bit value within an array of 8 16bit values is non-zero, then the whole thing is valid?

So all of these are all equally valid
0x0000 0x0000 0x0000 0x0001 0x0000 0x0000 0x0000 0x0000
0xdead 0xbeef 0xdead 0xbeef 0xdead 0xbeef 0xdead 0xbeef
0x0000 0x0000 0x0000 0x000 0x0000 0x0000 0x0000 0x8000
0x8000 0x0000 0x0000 0x000 0x0000 0x0000 0x0000 0x0000
I'll believe you. Do you really need to constrain this because the device dies if all 0's are set, or can you just let userspace worry about setting something sane?

And why test as 8bit values? The same comparison can be done on the u16 values, again treated as an array.

I have a conceptual problem here. I would also like userspace to worry about setting something sane, but IRS1125_CID_MOD_PLL is a compound control. I want to keep controls and hardware in sync which I guess is a key concept of V4L2 (I'll remove all unnecessary V4L2_CTRL_FLAG_EXECUTE_ON_WRITE).

Now the problem is, that when userspace calls a "close" on the video device, the imager goes into deep sleep, which I guess is a good idea to safe power. But in this state, it also looses all its register contents and the V4L2 controls need to be synced with the HW again after the wakeup.
I do that inside __sensor_init with v4l2_ctrl_handler_setup(...). Here the IRS1125_CID_MOD_PLL contains a default value of "all zeros" which kills the imager. I'll look deeper into that problem, I think that the pre-devider value of 0 might be the problem, which would make the code much clearer here.

Anyway I did not find any other solution to that problem. Maybe you have one...? I think a p_def member to v4l2_ctrl_config is added to more recent kernel versions (https://lore.kernel.org/patchwork/patch/1147819/) which would give me the possibility to add a senseful default value for IRS1125_CID_MOD_PLL.

OK, so you need IRS1125_CID_MOD_PLL initialised to something, or skip it if it hasn't been set.

Do you also need to guard against userspace setting all 0's? This will show up as a standard V4L2 device, so if all zeros is really fatal (hardware damage fatal?) then the driver ought to guard against it.
Minor nitpick - if (*pval) { is neater than if (*pval != 0) {
It would also be nice if the sensor would just stream with default values, but I realise that this may not be terribly useful.

If you rebase against 5.4, then I'm happy for those patches from 5.5 to be cherry-picked back (probably ignore the vivid ones). We do insert a line
Commit <hash> upstream
into the commit text when cherry-picking back, just to make maintenance easier.
You can then set a default :-)

You can look at the current change. I think it is a good idea, that the userspace is responsible for writing senseful data in this control. I skip writing this control only in case of the internal sync after HW power up.

6by9

Down to the minor nit picks now.

6by9 · 2020-06-16T08:26:49Z

drivers/media/i2c/irs1125.c

@@ -1019,6 +1108,7 @@ static int irs1125_probe(struct i2c_client *client,
 	}

 	sensor->num_seq = 5;
+	sensor->ctrl_init = false;


Technically redudnant as you did a kzalloc of sensor.

Right, removed

6by9 · 2020-06-16T08:28:51Z

drivers/media/i2c/irs1125.c

@@ -552,133 +612,147 @@ static int irs1125_s_ctrl(struct v4l2_ctrl *ctrl)
 					struct irs1125, ctrl_handler);
 	struct i2c_client *client = v4l2_get_subdevdata(&dev->sd);
 	int err, i;
-	struct irs1125_mod_pll *mod_cur, *mod_new;
-	struct irs1125_seq_cfg *cfg_cur, *cfg_new;
-	u16 addr, val;

 	err = 0;


Observation in passing, it's more normal to declare an initialise something like this in one step, ie

int err = 0, i;

at line 614.

Okay, I changed that. Actually I always separate declarations and initializations. A former colleague told me to do so. If I remember correctly, he was a freeBSD developer and they are very strict there, an excerpt of their coding styles:

Be careful to not obfuscate the code by initializing variables in the
declarations. Use this feature only thoughtfully.

6by9 · 2020-06-16T08:29:13Z

drivers/media/i2c/irs1125.c

+		struct irs1125_mod_pll *mod_new;
+
+		if (dev->ctrl_init)
+			return 0;


Seeing as err is already 0, you could just break;

6by9 · 2020-06-16T09:29:10Z

drivers/media/i2c/irs1125.c

@@ -82,6 +82,7 @@ struct irs1125 {
 	struct v4l2_ctrl *ctrl_numseq;

 	int power_count;
+	bool ctrl_init;


I can't comment on the commit text directly, but the line

Thus, this ctrl is not written inside the driver into after power up. The userspace has to take care to write senseful data here.

is a little oddly phrased.

Thus, this ctrl is not written to the hardware until it has been initialised by userspace. Userspace must take care to write meaningful data to the control - it is not validated.

Secondly ctrl_init is rather generic. It's specifically pll_ctrl_init, or pll_ctrl_set.

I hope, the comment is better now.

I renamed ctrl_init to mod_pll_init

Reading data over i2c is done by using i2c_transfer to ensure that this operation can't be interrupted. Signed-off-by: Markus Proeller <[email protected]>

Changed some variable names to comply with checkpatch --strict mode. Debug messages added. Signed-off-by: Markus Proeller <[email protected]>

Instead of changing the exposure and framerate settings for all sequences, they can be changed for every sequence individually now. Therefore the IRS1125_CID_SAFE_RECONFIG ctrl has been removed and replaced by IRS1125_CID_SAFE_RECONFIG_S<seq_num>_EXPO and *_FRAME ctrls. The consistency check in the sequence ctrl IRS1125_CID_SEQ_CONFIG is removed. Signed-off-by: Markus Proeller <[email protected]>

When closing the video device, the irs1125 is put in power down state. To keep V4L2 ctrls and the HW in sync, v4l2_ctrl_handler_setup is called after power up. The compound ctrl IRS1125_CID_MOD_PLL however has a default value of all zeros, which puts the imager into a non responding state. Thus, this ctrl is not written by the driver into HW after power up. The userspace has to take care to write senseful data. Signed-off-by: Markus Proeller <[email protected]>

Coimbra1984 · 2020-06-18T14:28:07Z

@6by9 I think I have reworked everything according to your comments.

6by9 · 2020-06-18T15:05:34Z

Thanks. As long as this still works with the hardware then I think I'm happy.

bjajoh · 2020-06-18T15:44:29Z

@Coimbra1984 @6by9 I have tested it and it works just fine.

Coimbra1984 · 2020-06-19T07:07:18Z

@6by9 I tested it also. Works fine. @bjajoh is our best employee. He also tested this version with success. I think it is good.

pelwell · 2020-06-19T07:49:56Z

Based on @6by9's review, the fact that it looks sensible, and the limited scope for collateral damage, I'm happy to merge.

kernel: brcmfmac: Prefer a ccode from OTP over nvram file kernel: staging: bcm2835-audio: Add missing MODULE_ALIAS See: raspberrypi/linux#3681 kernel: IRS1125 sensor driver updates See: raspberrypi/linux#3544 kernel: Revert downstream SPI patches See: raspberrypi/linux#3687 kernel: PCI: brcmstb: Add DT property to control L1SS firmware: hdmi: Set HD_CTL_WHOLSMP and HD_CTL_CHALIGN_SET See: https://forum.kodi.tv/showthread.php?tid=354589 firmware: arm_ldconfig: Honour the kernel8 text offset See: #1415 firmware: jpeghw: Skip repeated 0xFF padding bytes between markers See: RPi-Distro/vlc#8 filesystem: Add support for SD cards with GPT partition table

kernel: brcmfmac: Prefer a ccode from OTP over nvram file kernel: staging: bcm2835-audio: Add missing MODULE_ALIAS See: raspberrypi/linux#3681 kernel: IRS1125 sensor driver updates See: raspberrypi/linux#3544 kernel: Revert downstream SPI patches See: raspberrypi/linux#3687 kernel: PCI: brcmstb: Add DT property to control L1SS firmware: hdmi: Set HD_CTL_WHOLSMP and HD_CTL_CHALIGN_SET See: https://forum.kodi.tv/showthread.php?tid=354589 firmware: arm_ldconfig: Honour the kernel8 text offset See: raspberrypi/firmware#1415 firmware: jpeghw: Skip repeated 0xFF padding bytes between markers See: RPi-Distro/vlc#8 filesystem: Add support for SD cards with GPT partition table

6by9 reviewed Apr 14, 2020

View reviewed changes

Coimbra1984 force-pushed the rpi-4.19.y branch from 4e339a3 to 0c55c63 Compare April 14, 2020 19:21

Coimbra1984 changed the title ~~IRS1125 sensor driver updates~~ WIP: IRS1125 sensor driver updates Apr 29, 2020

Coimbra1984 changed the title ~~WIP: IRS1125 sensor driver updates~~ IRS1125 sensor driver updates May 22, 2020

6by9 requested changes Jun 9, 2020

View reviewed changes

Coimbra1984 force-pushed the rpi-4.19.y branch from bdc8d7b to 189a412 Compare June 16, 2020 08:01

6by9 reviewed Jun 16, 2020

View reviewed changes

Coimbra1984 added 4 commits June 16, 2020 13:24

media: irs1125: Using i2c_transfer for ic2 reads

3d51fc3

Reading data over i2c is done by using i2c_transfer to ensure that this operation can't be interrupted. Signed-off-by: Markus Proeller <[email protected]>

media: irs1125: Refactoring and debug messages

6badc80

Changed some variable names to comply with checkpatch --strict mode. Debug messages added. Signed-off-by: Markus Proeller <[email protected]>

Coimbra1984 force-pushed the rpi-4.19.y branch from 189a412 to 1f7f7b0 Compare June 16, 2020 11:34

pelwell merged commit a03605b into raspberrypi:rpi-4.19.y Jun 19, 2020

6by9 mentioned this pull request Aug 14, 2020

IRS1125 camera kernel driver outputs unvalid data since 5.4.51 #3793

Closed


		*val = rdval[1] \| (rdval[0] << 8);
		*val = get_unaligned_be16(data_buf);

		int s;

		s = sizeof(struct irs1125_mod_pll) * IRS1125_NUM_MOD_PLLS;

IRS1125 sensor driver updates #3544

IRS1125 sensor driver updates #3544

Conversation

Coimbra1984 commented Apr 14, 2020 • edited Loading

6by9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

6by9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

6by9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Coimbra1984 commented Jun 18, 2020

6by9 commented Jun 18, 2020

bjajoh commented Jun 18, 2020

Coimbra1984 commented Jun 19, 2020

pelwell commented Jun 19, 2020

Coimbra1984 commented Apr 14, 2020 •

edited

Loading