Morning Tomi,
On 2/7/22 18:23, Tomi Valkeinen wrote:
> On 07/02/2022 16:38, Vaittinen, Matti wrote:
>> 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 admit that I don't know MFD drivers too well, but I was thinking about
> this some time back and I wasn't quite sure about using MFD here.
>
> My thinking was that MFD is fine and good when a device contains more or
> less independent functionalities, like a PMIC with, say, gpios and
> regulators, both of which just work as long as the PMIC is powered up.
>
> Here all the functionalities depend on the link (fpdlink or some other
> "link" =), and the serializers. In other words, the link status or any
> changes to the link or the serializers might affect the GPIO/I2C/IRQ
> functionalities.
My use case has been such that once the link between DES & SER
established, it should not go away. If it does it is some kind of an
error and there is no recovery mechanims (at least not yet). Hence I
haven't prepared full solution how to handle dropping/re-connecting the
link or re-initializing des/ser/slaves.
> So, I don't have any clear concern here. Just a vague feeling that the
> functionalities in this kind of devices may be more tightly tied
> together than in normal MFDs. I could be totally wrong here.
I can't prove you're wrong even if that would be so cool :p
I guess a lot of this boils down how the SER behaves when link is
dropped. Does it maintain the configuration or reset to some other
state? And what happens on des & what we need to do in order to reconnect.
My initial feeling is that the DES should always be available as it is
directly connected to I2C. So DES should always be there.
Access to SERs and the devices on remote buses is naturally depending on
the link. So dropping the link means access to SERs and remote devices
start failing - which is probably visible to the MFD sub-devices as
failing regmap accesses. This needs then appropriate handling.
After that being said, I think we can't get over this problem even when
not using MFD. As far as I read your code, the SER and DES have
independent drivers also when MFD is not used. So dropping the link is
still someting that pulls the legs from the SER, right? I also guess the
remote I2C devices like sensors are also implemented as independent drivers.
Well, (I hope) I'll see where I end up with my code... It really makes
this discussion a bit dull when I can't just show the code for
comparison :/ I don't (yet) see why the MFD approach could not work, and
I still think it's worth trying - but I now certainly understand why you
hesitated using MFD. Thanks for taking the time to explain this to me.
Best Regards
--Matti
--
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 ~~
Hi,
On 08/02/2022 08:40, Vaittinen, Matti wrote:
> Morning Tomi,
>
> On 2/7/22 18:23, Tomi Valkeinen wrote:
>> On 07/02/2022 16:38, Vaittinen, Matti wrote:
>>> 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 admit that I don't know MFD drivers too well, but I was thinking about
>> this some time back and I wasn't quite sure about using MFD here.
>>
>> My thinking was that MFD is fine and good when a device contains more or
>> less independent functionalities, like a PMIC with, say, gpios and
>> regulators, both of which just work as long as the PMIC is powered up.
>>
>> Here all the functionalities depend on the link (fpdlink or some other
>> "link" =), and the serializers. In other words, the link status or any
>> changes to the link or the serializers might affect the GPIO/I2C/IRQ
>> functionalities.
>
> My use case has been such that once the link between DES & SER
> established, it should not go away. If it does it is some kind of an
> error and there is no recovery mechanims (at least not yet). Hence I
> haven't prepared full solution how to handle dropping/re-connecting the
> link or re-initializing des/ser/slaves.
>
>> So, I don't have any clear concern here. Just a vague feeling that the
>> functionalities in this kind of devices may be more tightly tied
>> together than in normal MFDs. I could be totally wrong here.
>
> I can't prove you're wrong even if that would be so cool :p
>
> I guess a lot of this boils down how the SER behaves when link is
> dropped. Does it maintain the configuration or reset to some other
> state? And what happens on des & what we need to do in order to reconnect.
>
> My initial feeling is that the DES should always be available as it is
> directly connected to I2C. So DES should always be there.
Yes, I don't see how DES would be affected. But all the services offered
by the MFDs are behind the link.
> Access to SERs and the devices on remote buses is naturally depending on
> the link. So dropping the link means access to SERs and remote devices
> start failing - which is probably visible to the MFD sub-devices as
> failing regmap accesses. This needs then appropriate handling.
I was also thinking about cases like BIST or link-analysis which
temporarily affect the link. They're not errors, but I guess from MFD's
point of view they could be handled the same way (whatever that way is).
> After that being said, I think we can't get over this problem even when
> not using MFD. As far as I read your code, the SER and DES have
> independent drivers also when MFD is not used. So dropping the link is
> still someting that pulls the legs from the SER, right? I also guess the
> remote I2C devices like sensors are also implemented as independent drivers.
That's true. I don't think the problem is really different with or
without MFDs. My thinking was just that it's easier to manage all the
problem cases if there are no walls between the components.
> Well, (I hope) I'll see where I end up with my code... It really makes
> this discussion a bit dull when I can't just show the code for
> comparison :/ I don't (yet) see why the MFD approach could not work, and
> I still think it's worth trying - but I now certainly understand why you
> hesitated using MFD. Thanks for taking the time to explain this to me.
I don't think MFD approach could not work. I just don't see why to use
it here.
I'm curious, why do you think using MFDs makes the driver so much
cleaner? The current fpdlink driver is in one file, but, say, if we
split it to multiple files, based on the function, while still keeping
it as a single driver, would that be so much different from an MFD
solution? Is there something in the MFD approach that makes the code
simpler?
Tomi
On 2/8/22 10:28, Tomi Valkeinen wrote:
> I'm curious, why do you think using MFDs makes the driver so much
> cleaner? The current fpdlink driver is in one file, but, say, if we
> split it to multiple files, based on the function, while still keeping
> it as a single driver, would that be so much different from an MFD
> solution? Is there something in the MFD approach that makes the code
> simpler?
Fair question.
I personally have two reasons - one technical which I could just throw
here and hope everyone buys it :) But I think the main reason for me to
initially think of MFD is not a technical one.
Last few years I've spent working with PMICs/chargers - which were MFD
to the bone. Before that I worked on a proprietary clocking/all-purpose
FPGA as well as with ASIC routing RP3/RP301 links + providing timing
facilities / clocks. Those were also MFD devices - and one of those used
MFD drivers, the other didn't although it really should have.
So the non technical reason for me is that I am used to seeing
multi-function devices implemented as MFD devices - hence I immediately
saw the SERDES as one too.
One the technical benefit from MFD is that it (in many cases) allows one
to use standard way to re-use code. Eg, it's not a rare case that same
HW blocks are used in many projects. One can for example see three
different PMICs, all having same RTC and clk blocks, while different
regulators and GPIOs - or some just omitting one of those.
MFD allows 'collecting' these IP blocks under different umbrellas by
kicking same subdevices alive via different MFD devices in a standard
way. The IP block (say GPIO controller) we are driving can be
implemented on SER connected by FPDLINK III to DES - or it can be
implemented in PMIC - the absolutely same standrd (mfd sub) platform
GPIO driver can be used in both cases.
Other than that, the use of MFD allows one to write pinctrl/gpio driver
as any pinctrl/gpio platform device driver. It will be looking familiar
to anyone who has worked with pinctrl/gpio - even if he has never seen
media/v4l2 ;) This is where my thought of clarity came from.
Rest is slightly offtopic - you can stop reading.
I am not sure how TI does this and if you know whether same blocks can
be used in other devices. I just have learned never to trust it when a
HW engineer (in Nokia, Rohm or other company) tells me "this is the last
IC using this technology". It may be my problem though as nor do I buy
it if someone says me: "the next version will be just same to this
previous - there is no software impact" :rolleyes: I for example once
heard that when the "next product" maintained same register offsets for
some functions - but did add new ones - and changed the registers from
16bit => 32bit and connecting bus from PCIe => I2C... I remember that
project with a nicknames 're-estimate' 'reschedule' and 'panic-button' :)
Yours
-- Matti
--
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 ~~