| Message ID | 20220915003522.1227073-5-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add imx577 and imx477 compatible to imx412 | expand |
Hi Bryan On Thu, 15 Sept 2022 at 01:36, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > The Sony imx477 uses the same silicon enabling reference code from Sony in > the available examples provided. Have you had any conversations with Sony on whether the same register settings are genuinely valid for all of these sensors? IMX477 is the sensor in the Raspberry Pi HQ camera, but the register set that we have from Sony has many undocumented (to us at least) registers for image quality tuning that aren't in the imx412 driver - we're up at 417 registers vs imx412's 231 [1]. Raspberry Pi has previously had discussions with Sony regarding IMX378 vs IMX477, which is also in the same family. Whilst nearly identical in register setup, they gave us 3 additional registers that have to be set for IMX378 to avoid image quality issues. I wouldn't be surprised if something similar weren't true for IMX412 and IMX577. I'm not saying it's impossible to remove a load of duplicated register settings by combining these sensors into one driver, but this current patch looks a little simplistic, and probably needs to be looking to have a struct for sensor specific registers, not just changing the compatible string and advertised name. Or perhaps we extend that later once the compatible string has been added? Merging them does bring issues with regard to testing as it is unlikely to be practical to test any new features across all variants. Dave [1] https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/imx477.c > Add an imx477 compatible string and re-use the existing imx412 code. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/media/i2c/imx412.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c > index bc7fdb24235f..1013a5afc85f 100644 > --- a/drivers/media/i2c/imx412.c > +++ b/drivers/media/i2c/imx412.c > @@ -1289,6 +1289,7 @@ static const struct dev_pm_ops imx412_pm_ops = { > > static const struct of_device_id imx412_of_match[] = { > { .compatible = "sony,imx412", .data = "imx412" }, > + { .compatible = "sony,imx477", .data = "imx477" }, > { } > }; > > -- > 2.34.1 >
On 15/09/2022 23:03, Dave Stevenson wrote: > Hi Bryan > > On Thu, 15 Sept 2022 at 01:36, Bryan O'Donoghue > <bryan.odonoghue@linaro.org> wrote: >> >> The Sony imx477 uses the same silicon enabling reference code from Sony in >> the available examples provided. > > Have you had any conversations with Sony on whether the same register > settings are genuinely valid for all of these sensors? There's alot to unpack here but... this shows me the main thing https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/imx477.c#L37 The vendor driver reference code I have expects 0x0577. I've discussed with Daniele previously - that imx412 and imx577 both return 0x0577 in the chip id field and this imx412.c driver won't work with any chip reporting anything else. Your chip though is returning 0x0477 so it will need to have its own upstream driver anyway. So the conclusion I drew from the Nvidia/Leopard stack is wrong they call the driver imx477 but pointedly and suspiciously comment out the check for chip id. And its pretty clear why, its so the "imx477" driver they have with work with chips identifying as 0x0577 and 0x0477. > IMX477 is the sensor in the Raspberry Pi HQ camera, but the register > set that we have from Sony has many undocumented (to us at least) > registers for image quality tuning that aren't in the imx412 driver - > we're up at 417 registers vs imx412's 231 [1]. > > Raspberry Pi has previously had discussions with Sony regarding IMX378 > vs IMX477, which is also in the same family. Whilst nearly identical > in register setup, they gave us 3 additional registers that have to be > set for IMX378 to avoid image quality issues. I wouldn't be surprised > if something similar weren't true for IMX412 and IMX577. > > I'm not saying it's impossible to remove a load of duplicated register > settings by combining these sensors into one driver, but this current > patch looks a little simplistic, and probably needs to be looking to > have a struct for sensor specific registers, not just changing the > compatible string and advertised name. Or perhaps we extend that later > once the compatible string has been added? > Merging them does bring issues with regard to testing as it is > unlikely to be practical to test any new features across all variants. No so you're right the init sequence for imx477 is different or different enough that Sony spun new silicon for it so you can tweak it. So, I'll drop the imx477 for V3, thanks for catching this and FWIW I will take your suggestion will reach out to my board vendor and see if I can convince them to share documentation with me or put me in contact with a Sony person who is authorised to do so. We should get a definitive answer on imx412 (v) imx577 - is it different silicon or is it a packaging / optics filter level difference ? --- bod
Hi Bryan, On Fri, Sep 16, 2022 at 01:32:06AM +0100, Bryan O'Donoghue wrote: > On 15/09/2022 23:03, Dave Stevenson wrote: > > Hi Bryan > > > > On Thu, 15 Sept 2022 at 01:36, Bryan O'Donoghue > > <bryan.odonoghue@linaro.org> wrote: > > > > > > The Sony imx477 uses the same silicon enabling reference code from Sony in > > > the available examples provided. > > > > Have you had any conversations with Sony on whether the same register > > settings are genuinely valid for all of these sensors? > > There's alot to unpack here but... this shows me the main thing > > https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/imx477.c#L37 > > The vendor driver reference code I have expects 0x0577. I've discussed with > Daniele previously - that imx412 and imx577 both return 0x0577 in the chip > id field and this imx412.c driver won't work with any chip reporting > anything else. > > Your chip though is returning 0x0477 so it will need to have its own > upstream driver anyway. That doesn't matter. What does however matter is what that sensor does and how it's controlled. It is entirely possible that earlier versions of such a sensor simply report a wrong ID. > > So the conclusion I drew from the Nvidia/Leopard stack is wrong they call > the driver imx477 but pointedly and suspiciously comment out the check for > chip id. > > And its pretty clear why, its so the "imx477" driver they have with work > with chips identifying as 0x0577 and 0x0477. I wouldn't be so worried about the model ID *if* the registers (and their values) programmed to the sensor are otherwise the same. And even if you have small differences in the registers you'll need to write there, you can still differentiate between the sensors based on the compatible string. I don't have strong opinion on the grey areas though. Still if the register set is exactly the same, then the driver should also be the same.
On 16/09/2022 12:03, Sakari Ailus wrote: > And even if you have small differences in the registers you'll need to > write there, you can still differentiate between the sensors based on the > compatible string. > > I don't have strong opinion on the grey areas though. Still if the register > set is exactly the same, then the driver should also be the same. Right now we have - An imx412 driver that works on imx577 unmodified on Qcom hardware - A Nvidia driver modified by Leopard imaging that ignores the chip id and uses the same init sequence. This driver is called "imx477" and I can verify it works with imx412 and imx577. The code for this driver modifies the original out of tree driver they had and stops validating the CHIP_ID So I think we can take it as read it works with imx412, imx477 and imx577 - I've verified the first and last is the case. We know the upstream driver works with the Intel platform and I've tested/used it on Qcom with minimal change, so I'm happy to stand over listing both imx412 and imx577. Its pretty clear the init sequence works for imx412, imx477 and imx577 so, it feels to me like the right thing to do is to add in the compatible strings and if/when we get better chip specific data say for higher resolutions, add that resolution init sequence in when the compat matches. --- bod
On 16/09/2022 12:14, Bryan O'Donoghue wrote: > - A Nvidia driver modified by Leopard imaging that ignores the chip id > and uses the same init sequence. > This driver is called "imx477" and I can verify it works with > imx412 and imx577. > The code for this driver modifies the original out of tree driver they > had and stops validating the CHIP_ID > So I think we can take it as read it works with imx412, imx477 and > imx577 - I've verified the first and last is the case. I should have added - An RPI driver that works with imx477 and imx378 https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/imx477.c#L38
diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c index bc7fdb24235f..1013a5afc85f 100644 --- a/drivers/media/i2c/imx412.c +++ b/drivers/media/i2c/imx412.c @@ -1289,6 +1289,7 @@ static const struct dev_pm_ops imx412_pm_ops = { static const struct of_device_id imx412_of_match[] = { { .compatible = "sony,imx412", .data = "imx412" }, + { .compatible = "sony,imx477", .data = "imx477" }, { } };
The Sony imx477 uses the same silicon enabling reference code from Sony in the available examples provided. Add an imx477 compatible string and re-use the existing imx412 code. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/media/i2c/imx412.c | 1 + 1 file changed, 1 insertion(+)