| Message ID | 20220206115939.3091265-1-luca@lucaceresoli.net |
|---|---|
| Headers | show |
| Series | Hi, | expand |
Hi, apologies for the incorrect cover letter subject. :( It should have been as in the present e-mail, which I'm sending mostly to help search engines find the subject... Regards.
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 sent RFCv2 back in 2019 (!). After that I have applied most of the > improvements proposed during code review, most notably device tree > representation and proper use of kernel abstractions for clocks and GPIO. I > have also done many improvements all over the drivers code. Thanks for sending this! I'll have a closer look at the code in the near future. > However I still don't consider these drivers "ready", hence the RFC status. > > One reason is that, while the I2C ATR idea has been considered good by > Wolfram, its implementation requires I2C core changes that have been tried > but never made it to mainline. I think that discussion needs to be reopened > and work has to be done on that side. Thus for the time being this code > still has the alias pool: it is an interim solution until I2C core is > ready. > > Also be aware that the only hardware where I sould test this code runs a > v4.19 kernel. I cannot guarantee it will work perfectly on mainline. > > And since my hardware has only one camera connected to each deserializer I > dropped support. However I wrote the code to be able to easily add support > for 2 and 4 camera inputs as well as 2 CSI-2 outputs (DS90UB960). > > Finally, I dropped all attempts at supporting hotplug. The goals I had 2+ > years ago are not reasonably doable even with current kernels. Luckily all > the users that I talked with are happy without hotplug. For this reason I > simplified the serializer management in the DS90UB954 driver by keeping the > serializer always instantiated. > > 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: 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. 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. 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. 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?), 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 =). But, of course, I'm open to other ideas on how to proceed. Tomi
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
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/20181008211205.2900-1-vz@mleia.com/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
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/20181008211205.2900-1-vz@mleia.com/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. 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. Tomi
On Sun, Feb 06, 2022 at 12:59:36PM +0100, Luca Ceresoli wrote: > Describe the Texas Instruments DS90UB953-Q1, a MIPI CSI-2 video serializer > with I2C Address Translator and remote GPIOs. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > > --- > > Changes RFCv2 -> RFCv3: > > - rewrite in yaml > > Changes RFCv1 -> RFCv2: none, this patch is new in RFCv2 > --- > .../bindings/media/i2c/ti,ds90ub953-q1.yaml | 96 +++++++++++++++++++ > MAINTAINERS | 7 ++ > include/dt-bindings/media/ds90ub953.h | 16 ++++ > 3 files changed, 119 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml > create mode 100644 include/dt-bindings/media/ds90ub953.h > > diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml > new file mode 100644 > index 000000000000..2a836a3e65e9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml > @@ -0,0 +1,96 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (C) 2019 Renesas Electronics Corp. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub953-q1.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Texas Instruments DS90UB953-Q1 video serializer > + > +maintainers: > + - Luca Ceresoli <luca@lucaceresoli.net> > + > +description: | > + The TI DS90UB953-Q1 is a MIPI CSI-2 video serializer that forwards a MIPI > + CSI-2 input video stream over an FPD Link 3 connection to a remote > + deserializer. It also allows access to I2C and GPIO from the deserializer. > + > + The DT definitions can be found in include/dt-bindings/media/ds90ub953.h > + > + When used as a the remote counterpart of a deserializer (e.g. the > + DS90UB954-Q1), the serializer is described in the > + "deserializer/remote-chips/remote-chip@[01]" node. > + > +properties: > + compatible: > + const: ti,ds90ub953-q1 > + > + reg: > + description: | > + Index of the remote (serializer) RX port that this serializer is > + connected to. > + maxItems: 1 > + > + clocks: > + description: FPD-Link line rate (provided by the deserializer) > + maxItems: 1 > + > + gpio-controller: true > + > + '#gpio-cells': > + const: 2 > + > + '#clock-cells': > + const: 0 > + > + ti,gpio-functions: > + description: | > + A list of 4 values defining how the 4 GPIO pins are connected in > + hardware; possible values are: > + - DS90_GPIO_FUNC_UNUSED (0): the GPIO is not unused > + - DS90_GPIO_FUNC_INPUT (1): the GPIO is an input to the ds90ub953, > + the remote chip (deserializer) can read its value > + - DS90_GPIO_FUNC_OUTPUT_REMOTE (2): the GPIO is an output from the > + ds90ub953, the remote chip (deserializer) can set its value > + For unspecified values the GPIO is assumed to be unused. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + maxItems: 4 > + > +patternProperties: > + '^ti,ds90ub953-q1-(clk|d[0-3])-inv-pol-quirk$': > + description: | > + The MIPI CSI-2 input clock lane or any of the data lanes has inverted > + polarity in hardware What's the type? > + > +required: > + - compatible > + - reg > + - clocks > + - gpio-controller > + - '#gpio-cells' > + - '#clock-cells' > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/media/ds90ub953.h> > + remote-chips { > + #address-cells = <1>; > + #size-cells = <0>; > + > + remote-chip@0 { > + reg = <0>; > + compatible = "ti,ds90ub953-q1"; > + clocks = <&deser>; > + ti,gpio-functions = > + <DS90_GPIO_FUNC_UNUSED > + DS90_GPIO_FUNC_OUTPUT_REMOTE > + DS90_GPIO_FUNC_UNUSED > + DS90_GPIO_FUNC_UNUSED>; > + > + gpio-controller; > + #gpio-cells = <2>; > + #clock-cells = <0>; > + }; > + }; > diff --git a/MAINTAINERS b/MAINTAINERS > index 7383aec87e4a..4429ce035496 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -19090,6 +19090,13 @@ F: include/linux/dma/k3-udma-glue.h > F: include/linux/dma/ti-cppi5.h > F: include/linux/dma/k3-psil.h > > +TEXAS INSTRUMENTS DS90UB953 VIDEO SERIALIZER DRIVER > +M: Luca Ceresoli <luca@lucaceresoli.net> > +L: linux-media@vger.kernel.org > +S: Maintained > +F: Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml > +F: include/dt-bindings/media/ds90ub953.h > + > TEXAS INSTRUMENTS' SYSTEM CONTROL INTERFACE (TISCI) PROTOCOL DRIVER > M: Nishanth Menon <nm@ti.com> > M: Tero Kristo <kristo@kernel.org> > diff --git a/include/dt-bindings/media/ds90ub953.h b/include/dt-bindings/media/ds90ub953.h > new file mode 100644 > index 000000000000..5359432968e9 > --- /dev/null > +++ b/include/dt-bindings/media/ds90ub953.h > @@ -0,0 +1,16 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ Dual license please. > +/** > + * Definitions for the Texas Instruments DS90UB953-Q1 video serializer > + * > + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net> > + */ > + > +#ifndef _DS90UB953_H > +#define _DS90UB953_H > + > +#define DS90_GPIO_FUNC_UNUSED 0 > +#define DS90_GPIO_FUNC_INPUT 1 > +#define DS90_GPIO_FUNC_OUTPUT_REMOTE 2 > +#define DS90_GPIO_N_FUNCS 3 > + > +#endif /* _DS90UB953_H */ > -- > 2.25.1 > >
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/20181008211205.2900-1-vz@mleia.com/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
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/20181008211205.2900-1-vz@mleia.com/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