| Message ID | 20220922104225.1375331-4-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add imx577 and imx477 compatible to imx412 | expand |
Hi Brian On Thu, 22 Sept 2022 at 11:43, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > The Sony imx477 and imx577 use the same silicon enabling reference code > from Sony in the available examples provided as the imx412. > > Add in compatible strings to differentiate the parts. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/media/i2c/imx412.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c > index 9f854a1a4c2f..93f362e3b132 100644 > --- a/drivers/media/i2c/imx412.c > +++ b/drivers/media/i2c/imx412.c > @@ -1288,6 +1288,8 @@ 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" }, IMX477 isn't compatible with this driver at present - it has 0x0477 in REG_ID (0x0016) instead of the 0x0577 that this driver looks for. The driver therefore won't probe. (I don't think I've missed a patch removing the ID check). I know you state in your cover letter: I think we have established that imx477 and imx577 do have additional settings and modes over the imx412 which, we can and hopefully will add in as time goes by. What we have upstream will work for all three parts. It may *eventually* work for all three parts, but isn't the time to add the compatible string at the point where it is actually compatible with the driver? If you're adding in broken compatible strings then, as previously mentioned, imx378 is also in the same family IIRC it streams with the base imx477 registers but with some IQ issues. It reports 0x0378 in REG_ID. Dave > + { .compatible = "sony,imx577", .data = "imx577" }, > { } > }; > > -- > 2.34.1 >
On 22/09/2022 12:16, Dave Stevenson wrote: > It may*eventually* work for all three parts, but isn't the time to > add the compatible string at the point where it is actually compatible > with the driver? Yes. I forgot about the 0x477 chip id on your part. I'm happy enough to drop 477 from the compat string or indeed to allow 0x0477 as a valid chip identifier in imx412. Sakari, what would you like to do ? --- bod
Hi Bryan, Dave, On Thu, Sep 22, 2022 at 12:19:22PM +0100, Bryan O'Donoghue wrote: > On 22/09/2022 12:16, Dave Stevenson wrote: > > It may*eventually* work for all three parts, but isn't the time to > > add the compatible string at the point where it is actually compatible > > with the driver? > > Yes. I forgot about the 0x477 chip id on your part. > > I'm happy enough to drop 477 from the compat string or indeed to allow > 0x0477 as a valid chip identifier in imx412. > > Sakari, what would you like to do ? If the driver already works with all three apart from the chip ID check, I'd just extend the check.
On 22/09/2022 12:19, Bryan O'Donoghue wrote: > On 22/09/2022 12:16, Dave Stevenson wrote: >> It may*eventually* work for all three parts, but isn't the time to >> add the compatible string at the point where it is actually compatible >> with the driver? > > Yes. I forgot about the 0x477 chip id on your part. > > I'm happy enough to drop 477 from the compat string or indeed to allow > 0x0477 as a valid chip identifier in imx412. > > Sakari, what would you like to do ? > > --- > bod Right. So I got myself the official rpi imx477 sensor and ran the imx412/imx577 driver on the rpi 5.19.y tree It looks like the rpi driver configures imx477 for two MIPI data-lanes, whereas upstream imx412 wants four MIPI data-lanes. So already that means the imx412 config as-is won't work. But, we do know these sensors are very very close. I think the right medium term thing to do is try take in the majority of the imx477 code - including the various test modes, and resolutions and support for different MIPI data-lane configurations. Its not clear to me that the imx412/imx577 and imx378/imx477 can genuinely live in the same codebase though. Anyway I think adding imx477 to the imx412 driver should be considered out of scope pending answering most of those questions and getting the code to work. Anyway that merging of rpi imx477 and upstream imx412/imx577 code feels like an entirely different series. So I'll resend this series minus the imx477 bits. --- bod
Hi Bryan, On Wed, Oct 12, 2022 at 01:56:19AM +0100, Bryan O'Donoghue wrote: > On 22/09/2022 12:19, Bryan O'Donoghue wrote: > > On 22/09/2022 12:16, Dave Stevenson wrote: > > > It may*eventually* work for all three parts, but isn't the time to > > > add the compatible string at the point where it is actually compatible > > > with the driver? > > > > Yes. I forgot about the 0x477 chip id on your part. > > > > I'm happy enough to drop 477 from the compat string or indeed to allow > > 0x0477 as a valid chip identifier in imx412. > > > > Sakari, what would you like to do ? > > > > --- > > bod > > Right. > > So I got myself the official rpi imx477 sensor and ran the imx412/imx577 > driver on the rpi 5.19.y tree > > It looks like the rpi driver configures imx477 for two MIPI data-lanes, > whereas upstream imx412 wants four MIPI data-lanes. > > So already that means the imx412 config as-is won't work. > > But, we do know these sensors are very very close. > > I think the right medium term thing to do is try take in the majority of the > imx477 code - including the various test modes, and resolutions and support > for different MIPI data-lane configurations. > > Its not clear to me that the imx412/imx577 and imx378/imx477 can genuinely > live in the same codebase though. > > Anyway I think adding imx477 to the imx412 driver should be considered out > of scope pending answering most of those questions and getting the code to > work. > > Anyway that merging of rpi imx477 and upstream imx412/imx577 code feels like > an entirely different series. > > So I'll resend this series minus the imx477 bits. I wonder if you saw my earlier reply... but this is certainly fine, too.
Hi Bryan On Wed, 12 Oct 2022 at 10:06, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Bryan, > > On Wed, Oct 12, 2022 at 01:56:19AM +0100, Bryan O'Donoghue wrote: > > On 22/09/2022 12:19, Bryan O'Donoghue wrote: > > > On 22/09/2022 12:16, Dave Stevenson wrote: > > > > It may*eventually* work for all three parts, but isn't the time to > > > > add the compatible string at the point where it is actually compatible > > > > with the driver? > > > > > > Yes. I forgot about the 0x477 chip id on your part. > > > > > > I'm happy enough to drop 477 from the compat string or indeed to allow > > > 0x0477 as a valid chip identifier in imx412. > > > > > > Sakari, what would you like to do ? > > > > > > --- > > > bod > > > > Right. > > > > So I got myself the official rpi imx477 sensor and ran the imx412/imx577 > > driver on the rpi 5.19.y tree > > > > It looks like the rpi driver configures imx477 for two MIPI data-lanes, > > whereas upstream imx412 wants four MIPI data-lanes. > > > > So already that means the imx412 config as-is won't work. It won't work on the Pi IMX477 camera module as-is due to only exposing 2 data lanes, but there are 4 lane IMX477 modules around (Arducam sell a couple). Adding support for 2 lanes isn't a huge deal as long as you understand the clock tree in the sensor. > > But, we do know these sensors are very very close. I have a conversation open with Sony about how common these sensors are. The initial response from their Japanese team was that "common form between sensors of different families is not a requirement considered". > > I think the right medium term thing to do is try take in the majority of the > > imx477 code - including the various test modes, and resolutions and support > > for different MIPI data-lane configurations. > > > > Its not clear to me that the imx412/imx577 and imx378/imx477 can genuinely > > live in the same codebase though. The way Sony tends to work is to give you a spreadsheet of the exact register set required for the mode you ask them for, and they will generally have validated the settings for image quality issues. They tend not to go for generic configuration. Presumably Intel as the original authors of imx412.c were given such a spreadsheet for their particular use case. We have such spreadsheets from Sony for our modes. As well as only using 2 data lanes, they also drive the PLLs in a different mode (dual vs single), so how many of those changes need to be preserved to ensure we don't introduce image quality issues? How many of the differences are valid for all the other supposedly common sensors? How do we test it? How big is the can of worms? The engineer in me loves to note where we have similarities between sensors, but I'm also cautious in drawing too many conclusions as it is far too easy to mess up image quality. > > Anyway I think adding imx477 to the imx412 driver should be considered out > > of scope pending answering most of those questions and getting the code to > > work. > > > > Anyway that merging of rpi imx477 and upstream imx412/imx577 code feels like > > an entirely different series. > > > > So I'll resend this series minus the imx477 bits. Ideas On Board do have a contract to upstream our imx477 driver. I will have a look at whether we can easily integrate it into the existing imx412 driver, but it remains to be seen how much common stuff remains between the sensors. > I wonder if you saw my earlier reply... but this is certainly fine, too. I guess that if we get there and find imx378/477 aren't common enough with imx412, then we split off into a second compatible binding. I can live with that. Dave
diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c index 9f854a1a4c2f..93f362e3b132 100644 --- a/drivers/media/i2c/imx412.c +++ b/drivers/media/i2c/imx412.c @@ -1288,6 +1288,8 @@ 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" }, + { .compatible = "sony,imx577", .data = "imx577" }, { } };
The Sony imx477 and imx577 use the same silicon enabling reference code from Sony in the available examples provided as the imx412. Add in compatible strings to differentiate the parts. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/media/i2c/imx412.c | 2 ++ 1 file changed, 2 insertions(+)