Re: [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,)

[Luca Ceresoli <luca@lucaceresoli.net>]

Hi Matti,

On 07/02/22 14:21, Vaittinen, Matti wrote:
> Hi dee Ho peeps,
> 
> On 2/7/22 14:06, Tomi Valkeinen wrote:
>> Hi Luca,
>>
>> On 06/02/2022 13:59, Luca Ceresoli wrote:
>>> this RFCv3, codename "FOSDEM Fries", of RFC patches to support the TI
>>> DS90UB9xx serializer/deserializer chipsets with I2C address translation.
> 
> ..snip
> 
>>> Even with the above limitations I felt I'd send this v3 anyway since
>>> several people have contacted me since v2 asking whether this
>>> implementation has made progress towards mainline. Some even improved on
>>> top of my code it their own forks. As I cannot afford to work on this 
>>> topic
>>> in the near future, here is the latest and greatest version I can 
>>> produce,
>>> with all the improvements I made so far.
>>
>> I've discussed with Luca in private emails, but I'll add a short status 
>> about my work in this thread:
> 
> Thanks for CC:ing me Luca. We had a small chat during the FOSDEM.
> 
>> About a year ago I took Luca's then-latest-patches and started working 
>> on them. The aim was to get full multiplexed streams support to v4l2 so 
>> that we could support CSI-2 bus with multiple virtual channels and 
>> embedded data, and after that, add support for fpdlink devices.
>>
>> Since then I have sent multiple versions of the v4l2 work (no drivers 
>> yet, only the framework changes) to upstream lists. Some pieces have 
>> already been merged to upstream (e.g. subdev state), but most of it is 
>> still under work. Here's a link to v10 of the streams series:
>>
>> https://lore.kernel.org/all/20211130141536.891878-1-tomi.valkeinen@ideasonboard.com/ 
>>
>>
>> It has a link to my (now slightly outdated) git branch which contains 
>> the driver work too.
> 
> I have fetched this tree from Tomi and done some experimenting on 
> another SERDES. That SERDES in not from TI or Maxim, some of you may 
> guess the company though :) Unfortunately I can't publish the details or 
> the code for now - I am discussing what I am allowed to publish. My 
> personal goal is to see if I could write a Linux driver for this 
> yet-another-Video-SERDES and see if it can one day get merged to 
> upstream for anyone interested to play with.
> 
>> The fpdlink drivers have diverged from Luca's version quite a bit. The 
>> most obvious difference is the support for multiplexed streams, of 
>> course, but there are lots of other changes too. The drivers support 
>> DS90UB960 (no UB954 at the moment), DS90UB953 and DS90UB913. UB960 
>> supports all the inputs and outputs.
> 
> For the record, the SERDES I am working with does also support 
> connecting 4 cameras (4 SERs) to one DES which provides two CSI-2 
> outputs. As far as I understand the virtual channel support is also 
> there (in the HW).
> 
>   I have also dropped some code which
>> I did not need and which I wasn't sure if it's correctly implemented, to 
>> make it easier to work on the multiplexed streams version. Some of that 
>> code may need to be added back.
>>
>> I have not changed the i2c-atr driver, and my fpdlink driver uses  it 
>> more or less the same way as in Luca's version.
>>
> 
> I have also used the ATR driver as is. The SERDES I am working with does 
> also the I2C address translation.
> 
>> Considering that you're not able to work on this, my suggestion is to 
>> review the i2c-atr patches here (or perhaps send those patches in a 
>> separate series?),
> 
> It would be _really_ cool to get the ATR upstream.
> 
>   but afaics the fpdlink drivers without multiplexed
>> streams is a dead-end, as they can only support a single camera (and no 
>> embedded data), so I don't see much point in properly reviewing them.
>>
>> However, I will go through the fpdlink drivers in this series and 
>> cherry-pick the changes that make sense. I was about to start working on 
>> proper fpdlink-clock-rate and clkout support, but I see you've already 
>> done that work =).
> 
> I am not sure if I am poking in the nest of the wasps - but there's one 
> major difference with the work I've done and with Toni's / Luca's work.

You are. ;)

> The TI DES drivers (like ub960 driver) packs pretty much everything 
> under single driver at media/i2c - which (in my opinion) makes the 
> driver pretty large one.
> 
> My approach is/was to utilize MFD - and prepare the regmap + IRQs in the 
> MFD (as is pretty usual) - and parse that much of the device-tree that 
> we see how many SER devices are there - and that I get the non I2C 
> related DES<=>SER link parameters set. After that I do kick alive the 
> separate MFD cells for ATR, pinctrl/GPIO and media.
> 
> The ATR driver instantiates the SER I2C devices like Toni's ub960 does. 
> The SER compatible is once again matched in MFD (for SER) - which again 
> provides regmap for SER, does initial I2C writes so SER starts 
> responding to I2C reads and then kicks cells for media and pinctrl/gpio.
> 
> I believe splitting the functionality to MFD subdevices makes drivers 
> slightly clearer. You'll get GPIOs/pinctrl under pinctrl as usual, 
> regmaps/IRQ-chips under MFD and only media/v4l2 related parts under media.

There has been quite a fiery discussion about this in the past, you can
grab some popcorn and read
https://lore.kernel.org/linux-media/20181008211205.2900-1-vz@mleia.com/T/#m9b01af81665ac956af3c6d57810239420c3f8cee

TL;DR: there have been strong opposition the the MFD idea.

I personally don't have a super strong opinion: I wrote this as a
monolithic driver because it looked like the most natural implementation
and found it was working fine for me, I never really explored the MFD idea.

> Anyways - I opened the mail client to just say that the ATR has worked 
> nicely for me and seems pretty stable - so to me it sounds like a goof 
> idea to get ATR reviewed/merged even before the drivers have been finalized.

Sounds like a... what...? A "good idea"? Or a "goofy idea"? :-D

-- 
Luca