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/[email protected]/
>>
>>
>> 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/[email protected]/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
Hi again Luca,
On 2/7/22 16:07, Luca Ceresoli wrote:
> 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.
>>
>>
>> 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/[email protected]/T/#m9b01af81665ac956af3c6d57810239420c3f8cee
>
> TL;DR: there have been strong opposition the the MFD idea.
Hm. I may be missing something but I didn't see opposition to using MFD
or splitting the drivers. I do see opposition to adding _functionality_
in MFD. If I read this correctly, Lee did oppose adding the I2C stuff,
sysfs attributes etc in MFD. Quoting his reply:
"This driver does too much real work ('stuff') to be an MFD driver.
MFD drivers should not need to care of; links, gates, modes, pixels,
frequencies maps or properties. Nor should they contain elaborate
sysfs structures to control the aforementioned 'stuff'.
Granted, there may be some code in there which could be appropriate
for an MFD driver. However most of it needs moving out into a
function driver (or two)."
And I tend to agree with Lee here. I would not put I2C bridge stuff or
sysfs attributes in MFD. But I think it does not mean SERDESes should
not use MFD when they clearly contain more IP blocks than the
video/media ones :) I am confident Lee and others might be much more
welcoming for driver which simply configures regmap and kicks subdriver
for doing the ATR / I2C stuff.
I did add minimal mandatory register initializations in order to avoid
synchronizing the sub-devices - but I hope that would be too much.
(Synchronizing sub-devices to when the I2C reads over the link becomes
available.)
What comes to regmap/regmap IRQ initialization in MFD - that's not
exceptional. I think it's quite standard for MFD to prepare IRQs/regmaps
when many sub-devices use these resources.
> 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.
No problem. I am definitely trying to tell you how these TI drivers must
be done. Even I don't have the guts to do that ;D
I am simply saying that the MFD approach could be used. It does have
certain merits if we manage to keep the MFD layer thin enough.
>> 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
Let me rephrase. It's greaf idea ;)
(I really meant a "good idea" :])
Best Regards
-- Matti Vaittinen
--
The Linux Kernel guy at ROHM Semiconductors
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~ this year is the year of a signature writers block ~~