Improvements to ov5693 driver

[djrscally]

This batch of commits improves upon the ov5693 driver.

The first three commits perform a whole bunch of cleanup to make the driver less painful to look at, then replace the existing power management in the driver with pm_runtime support. The following three are just minor tidyup.

I have another commit to follow which tidies up the startup sequence, modelling it on the ov5648 driver. At the moment though, there's one bug left to iron out.

[fabwu]

Nice work Dan, this really improves the driver code.

I've tested your changes and now I get a black image in qcam (not just underexposed). Maybe it's just my setup but let's if @qzed has the same issue.

There is also a problem with unloading. After unloading ipu3_cio2 -> ov5693 the laptop freezes after a couple of seconds (I wasn't able to extract an error message). I think the regulator_put issue is still there and pm doesn't work properly.

Sorry for giving just a brief feedback but exams are coming up and I have to step back for a while. I can still test your changes, but I don't have time for long debugging sessions (although I love them ^^).

[fabwu]

Just return here (no need for the extra variable).

[djrscally]

Ack! I realised this was stupid and fixed it in the next commit (which still needs a bit more work), but since this will need changes anyway I'll update it here.

[fabwu]

I don't like the if. Maybe it's better to use two functions sensor_power_up/sensor_power_down.

[djrscally]

Fair enough; I'm easy either way, so I'll split back to separate ones.

[fabwu]

My LED still flashes on startup but that's a minor issue.

[djrscally]

That's...weird. I'd love to understand why it still does that...maybe the polarity is different between the machines, then...but then that would mean the LED outputs when the sensor turns on / off would be reversed.

Pass; but like you say pretty minor. I'll think about that one in the future.

[fabwu]

Good catch!

[qzed]

Is ov5693->otp_data NULL if ov5693->otp_size is zero? In that case you can skip the check and just call kfree as kfree(NULL) is well defined.

[djrscally]

@qzed No; the pointer's kzalloced here:

static u8 *ov5693_otp_read(struct i2c_client *client)
{
	u8 *buf;
	int ret;

	buf = devm_kzalloc(&client->dev, (OV5693_OTP_DATA_SIZE + 16), GFP_KERNEL);
	if (!buf)
		return ERR_PTR(-ENOMEM);

<snip>

	return buf;
}

And then not modified until set in ov5693_read_otp_reg_array(), so it's either 0 or, I think, potentially garbage - that code is a little unclear.

[qzed]

Ah, I think this should be devm_kfree then.

[djrscally]

@qzed oops, you're right of course. Actually since I'm just freeing in remove that probably was fine to begin with

[djrscally]

Nice work Dan, this really improves the driver code.

Thanks!

I've tested your changes and now I get a black image in qcam (not just underexposed). Maybe it's just my setup but let's if @qzed has the same issue.

Nuts! Anything in dmesg? Do you get the fps counter ticking by in the terminal? And does the initial output of qcam (before the fps lines) report anything informative?

There is also a problem with unloading. After unloading ipu3_cio2 -> ov5693 the laptop freezes after a couple of seconds (I wasn't able to extract an error message). I think the regulator_put issue is still there and pm doesn't work properly.

Ah true to form I forgot to test unloading, sorry. I'll look into that and fix it.

Sorry for giving just a brief feedback but exams are coming up and I have to step back for a while. I can still test your changes, but I don't have time for long debugging sessions (although I love them ^^).

No problem - best of luck when the exams come! I am very pleased that that period of my life is through.

[fabwu]

Nuts! Anything in dmesg? Do you get the fps counter ticking by in the terminal? And does the initial output of qcam (before the fps lines) report anything informative?

No, nothing unusal in dmseg or qcam. The fps counter is ticking in qcam so the sensor emits frames. Did you change something on the exposure control?

Ah true to form I forgot to test unloading, sorry. I'll look into that and fix it.

Great thanks!

No problem - best of luck when the exams come! I am very pleased that that period of my life is through.

Thanks! Luckily, it's my last exam session but there's still the thesis ^^

[djrscally]

Nuts! Anything in dmesg? Do you get the fps counter ticking by in the terminal? And does the initial output of qcam (before the fps lines) report anything informative?

No, nothing unusal in dmseg or qcam. The fps counter is ticking in qcam so the sensor emits frames. Did you change something on the exposure control?

No, I don't think so (certainly not intentionally, and I can't see an accidental change...). What if you shine a light at the sensor, can you see the light then? I sometimes experience that (i.e. black stream but can see my phone's torch when I shine it), but usually in the evenings when the evenings when there's very little natural light.

No problem - best of luck when the exams come! I am very pleased that that period of my life is through.

Thanks! Luckily, it's my last exam session but there's still the thesis ^^

Ouch!

[fabwu]

No, I don't think so (certainly not intentionally, and I can't see an accidental change...). What if you shine a light at the sensor, can you see the light then? I sometimes experience that (i.e. black stream but can see my phone's torch when I shine it), but usually in the evenings when the evenings when there's very little natural light.

I actually get an error message. I thought it was related to the unloading bug but here it is:

[   53.656570] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: DPHY synchronization error
[   53.656757] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded
[   53.656800] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded
[   53.656881] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded
[   53.656936] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded
[   53.657016] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: multiple packet header errors detected
[   53.657017] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: DPHY synchronization error
[   53.657018] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded
[   53.657057] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: multiple packet header errors detected
[   53.657058] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: DPHY synchronization error
[   53.657143] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded
[   53.657201] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded

The whole dmesg is filled with these messages but I have no clue what they mean.

[djrscally]

No, I don't think so (certainly not intentionally, and I can't see an accidental change...). What if you shine a light at the sensor, can you see the light then? I sometimes experience that (i.e. black stream but can see my phone's torch when I shine it), but usually in the evenings when the evenings when there's very little natural light.

I actually get an error message. I thought it was related to the unloading bug but here it is:

[   53.656570] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: DPHY synchronization error
[   53.656757] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded
[   53.656800] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded
[   53.656881] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded
[   53.656936] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded
[   53.657016] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: multiple packet header errors detected
[   53.657017] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: DPHY synchronization error
[   53.657018] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded
[   53.657057] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: multiple packet header errors detected
[   53.657058] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: DPHY synchronization error
[   53.657143] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded
[   53.657201] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: inter-frame long packet discarded

The whole dmesg is filled with these messages but I have no clue what they mean.

Ahhh, that's what I was getting on the next commit; I've been wracking my brain trying to see what change there broke it...but it must be something in this set...weird that I wasn't getting it in this set though.

Alright; that's a bit of a blocker obviously; let me go back to it over the weekend, sorry guys!

[qzed]

Just tested it. Got the same issues as @fabwu, apart from something that looks a bit odd in probe (not touched in this PR though) only a couple of minor additional comments from my side.

[qzed]

Not really part of this PR, but shouldn't there be a return or something here and on the if (ret) above? Otherwise you're overwriting the ret value below and continue to probe even though ov5693_init_controls() or media_entity_pads_init() may have failed.

[djrscally]

Yes, agree. There's a lot of stuff like that throughout tbh - I think we've had so many people hacking at it that it's gotten a bit wild. I'm going to go through function by function and iron out problems like that when I get time...but after I get the exposure fixed.

[qzed]

The whole dmesg is filled with these messages but I have no clue what they mean.

Weirdly enough, I don't see them. I do have a black image, but if I shine my phone light against it it does show at least that.

[djrscally]

Too much changed whilst I was getting the timing issues here sorted, so rather than update this I'm going to open a new.