| Message ID | 20220206115939.3091265-1-luca@lucaceresoli.net |
|---|---|
| Headers | show |
| Series | Hi, | expand |
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 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. 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. 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. Thanks for showing the way for the rest of us Luca & others! It's much easier to follow than lead the way ;) Best Regards --Matti
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
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
On Mon, Feb 7, 2022 at 7:55 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > An ATR is a device that looks similar to an i2c-mux: it has an I2C > slave "upstream" port and N master "downstream" ports, and forwards > transactions from upstream to the appropriate downstream port. But is > is different in that the forwarded transaction has a different slave > address. The address used on the upstream bus is called the "alias" > and is (potentially) different from the physical slave address of the > downstream chip. > > Add a helper file (just like i2c-mux.c for a mux or switch) to allow > implementing ATR features in a device driver. The helper takes care or > adapter creation/destruction and translates addresses at each transaction. Why I2C mux driver can't be updated to support this feature? ... > RFCv1 was implemented inside i2c-mux.c and added yet more complexity > there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR > features are not in a MUX and vice versa, the overlapping is low. This was > almost a complete rewrite, but for the records here are the main > differences from the old implementation: While this is from a code perspective, maybe i2c mux and this one can still share some parts? ... > +config I2C_ATR > + tristate "I2C Address Translator (ATR) support" > + help > + Enable support for I2C Address Translator (ATR) chips. > + > + An ATR allows accessing multiple I2C busses from a single > + physical bus via address translation instead of bus selection as > + i2c-muxes do. What would be the module name? ... > +/** Is this a kernel doc formatted documentation? Haven't you got a warning? > + * I2C Address Translator > + * > + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net> 2019,2022? > + * > + * An I2C Address Translator (ATR) is a device with an I2C slave parent > + * ("upstream") port and N I2C master child ("downstream") ports, and > + * forwards transactions from upstream to the appropriate downstream port > + * with a modified slave address. The address used on the parent bus is > + * called the "alias" and is (potentially) different from the physical > + * slave address of the child bus. Address translation is done by the > + * hardware. > + * > + * An ATR looks similar to an i2c-mux except: > + * - the address on the parent and child busses can be different > + * - there is normally no need to select the child port; the alias used on > + * the parent bus implies it > + * > + * The ATR functionality can be provided by a chip with many other > + * features. This file provides a helper to implement an ATR within your > + * driver. > + * > + * The ATR creates a new I2C "child" adapter on each child bus. Adding > + * devices on the child bus ends up in invoking the driver code to select > + * an available alias. Maintaining an appropriate pool of available aliases > + * and picking one for each new device is up to the driver implementer. The > + * ATR maintains an table of currently assigned alias and uses it to modify > + * all I2C transactions directed to devices on the child buses. > + * > + * A typical example follows. > + * > + * Topology: > + * > + * Slave X @ 0x10 > + * .-----. | > + * .-----. | |---+---- B > + * | CPU |--A--| ATR | > + * `-----' | |---+---- C > + * `-----' | > + * Slave Y @ 0x10 > + * > + * Alias table: > + * > + * Client Alias > + * ------------- > + * X 0x20 > + * Y 0x30 > + * > + * Transaction: > + * > + * - Slave X driver sends a transaction (on adapter B), slave address 0x10 > + * - ATR driver rewrites messages with address 0x20, forwards to adapter A > + * - Physical I2C transaction on bus A, slave address 0x20 > + * - ATR chip propagates transaction on bus B with address translated to 0x10 > + * - Slave X chip replies on bus B > + * - ATR chip forwards reply on bus A > + * - ATR driver rewrites messages with address 0x10 > + * - Slave X driver gets back the msgs[], with reply and address 0x10 > + * > + * Usage: > + * > + * 1. In your driver (typically in the probe function) add an ATR by > + * calling i2c_atr_new() passing your attach/detach callbacks > + * 2. When the attach callback is called pick an appropriate alias, > + * configure it in your chip and return the chosen alias in the > + * alias_id parameter > + * 3. When the detach callback is called, deconfigure the alias from > + * your chip and put it back in the pool for later usage > + * > + * Originally based on i2c-mux.c > + */ Shouldn't this comment be somewhere under Documentation/ ? ... > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/slab.h> > +static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, > + struct i2c_msg msgs[], int num) foo[] makes not much sense in the function parameter. *foo is what will be used and it's explicit. Can this be located on one line (similar question to make compact the rest of the function declarations)? > + Redundant blank line. ... > + /* Ensure we have enough room to save the original addresses */ > + if (unlikely(chan->orig_addrs_size < num)) { > + void *new_buf = kmalloc(num * sizeof(chan->orig_addrs[0]), > + GFP_KERNEL); Use kmalloc_array() > + if (new_buf == NULL) > + return -ENOMEM; > + > + kfree(chan->orig_addrs); Hmm... is it a reimplementation of krealloc_array()? > + chan->orig_addrs = new_buf; > + chan->orig_addrs_size = num; > + } ... > + if (c2a) { > + msgs[i].addr = c2a->alias; > + } else { > + dev_err(atr->dev, "client 0x%02x not mapped!\n", > + msgs[i].addr); > + return -ENXIO; > + } 'else' would be redundant if you switch to the traditional pattern, i.e. check for errors first. ... > +/* > + * Restore all message address aliases with the original addresses. > + * > + * This function is internal for use in i2c_atr_master_xfer(). > + * > + * @see i2c_atr_map_msgs() > + */ Too sparse formatting of the comment. Can you make it compact? ... > + int ret = 0; Unneeded assignment. > + /* Switch to the right atr port */ > + if (atr->ops->select) { > + ret = atr->ops->select(atr, chan->chan_id); > + if (ret < 0) > + goto out; > + } > + > + /* Translate addresses */ > + mutex_lock(&chan->orig_addrs_lock); > + ret = i2c_atr_map_msgs(chan, msgs, num); > + if (ret < 0) { > + mutex_unlock(&chan->orig_addrs_lock); > + goto out; goto out_unlock_deselect; > + } > + > + /* Perform the transfer */ > + ret = i2c_transfer(parent, msgs, num); > + > + /* Restore addresses */ > + i2c_atr_unmap_msgs(chan, msgs, num); out_unlock_deselct: > + mutex_unlock(&chan->orig_addrs_lock); > +out: out_deselect: > + if (atr->ops->deselect) > + atr->ops->deselect(atr, chan->chan_id); > + > + return ret; > +} ... > + int err = 0; Be consistent with ret vs. err across the functions. > + if (atr->ops->select) > + err = atr->ops->select(atr, chan->chan_id); > + if (!err) Perhaps int ret; ret = 0; if (atr->ops->select) ret = atr->ops->select(atr, chan->chan_id); if (ret) goto out_deselect; > + err = i2c_smbus_xfer(parent, c2a->alias, flags, > + read_write, command, size, data); out_deselect: > + if (atr->ops->deselect) > + atr->ops->deselect(atr, chan->chan_id); > + > + return err; > +} ... > + int err = 0; Same as above: naming, useless assignment. ... > + c2a = kzalloc(sizeof(struct i2c_atr_cli2alias_pair), GFP_KERNEL); sizeof(*c2a) > + if (!c2a) { > + err = -ENOMEM; > + goto err_alloc; Useless label, return directly. > + } ... > + c2a = i2c_atr_find_mapping_by_client(&chan->alias_list, client); > + if (c2a != NULL) { if (c2a) > + list_del(&c2a->node); > + kfree(c2a); > + } ... > + char symlink_name[20]; Why 20? Do we have a predefined constant for that? > + if (dev->of_node) { This check can be dropped, also please use device property and fwnode APIs. No good of having OF-centric generic modules nowadays. > + struct device_node *atr_node; > + struct device_node *child; > + u32 reg; > + > + atr_node = of_get_child_by_name(dev->of_node, "i2c-atr"); atr_node = device_get_named_child_node(...); fwnode_for_each_child_node() { } > + for_each_child_of_node(atr_node, child) { > + err = of_property_read_u32(child, "reg", ®); > + if (err) > + continue; > + if (chan_id == reg) > + break; > + } > + > + chan->adap.dev.of_node = child; > + of_node_put(atr_node); > + } On the second thought can you utilize the parser from I2C mux? ... > + WARN(sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"), > + "can't create symlink to atr device\n"); > + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", chan_id); > + WARN(sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name), > + "can't create symlink for channel %u\n", chan_id); Doesn't sysfs already has a warning when it's really needed? ... > + if (atr->adapter[chan_id] == NULL) { > + dev_err(dev, "Adapter %d does not exist\n", chan_id); Noisy message. On freeing we usually don't issue such when we try to free already freeed resource. > + return; > + } ... > + atr = devm_kzalloc(dev, sizeof(*atr) > + + max_adapters * sizeof(atr->adapter[0]), > + GFP_KERNEL); Check overflow.h and use respective macro here. > + if (!atr) > + return ERR_PTR(-ENOMEM); ... > +/** It's not a kernel doc. > + * drivers/i2c/i2c-atr.h -- I2C Address Translator Please, no names of the files inside the files. > + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net> 2019,2022 ? > + * Based on i2c-mux.h > + */ ... > +#ifdef __KERNEL__ Why? ... > +#include <linux/i2c.h> > +#include <linux/mutex.h> Missed types.h Missed struct device;
Hi Andy, thank you for the _very_ detailed review and apologies for not having found the time to reply until now. I'm OK with most of your comments, so I'm not commenting on them for brevity. Below my comments on the remaining topics. On 08/02/22 12:16, Andy Shevchenko wrote: > On Mon, Feb 7, 2022 at 7:55 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: >> >> An ATR is a device that looks similar to an i2c-mux: it has an I2C >> slave "upstream" port and N master "downstream" ports, and forwards >> transactions from upstream to the appropriate downstream port. But is >> is different in that the forwarded transaction has a different slave >> address. The address used on the upstream bus is called the "alias" >> and is (potentially) different from the physical slave address of the >> downstream chip. >> >> Add a helper file (just like i2c-mux.c for a mux or switch) to allow >> implementing ATR features in a device driver. The helper takes care or >> adapter creation/destruction and translates addresses at each transaction. > > Why I2C mux driver can't be updated to support this feature? My first version did that. But it was very complex to shoehorn the ATR features in the i2c-mux code which already handles various [corner] cases. If memory serves, code reuse was limited to the trivial code: allocations, cleanups and the like. The root reason is that an atr and a mux have a similar electric topology, but they do very different things. An mux need to be commanded to switch from one downstream bus to another, an atr does not. An atr modifies the transaction, including the speed, a mux does not. >> RFCv1 was implemented inside i2c-mux.c and added yet more complexity >> there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR >> features are not in a MUX and vice versa, the overlapping is low. This was >> almost a complete rewrite, but for the records here are the main >> differences from the old implementation: > > While this is from a code perspective, maybe i2c mux and this one can > still share some parts? Possibly. I'd have to look into that in more detail. I must say having a separate file allowed me to be free to implement whatever is best for atr. With that done I would certainly make sense to check whether there are still enough commonalities to share code, maybe in a .c file with shared functions. >> +config I2C_ATR >> + tristate "I2C Address Translator (ATR) support" >> + help >> + Enable support for I2C Address Translator (ATR) chips. >> + >> + An ATR allows accessing multiple I2C busses from a single >> + physical bus via address translation instead of bus selection as >> + i2c-muxes do. > > What would be the module name? Isn't the module name written in Kconfig files just to avoid checkpatch complain about "too few doc lines"? :) Oook, it's i2s-atr anyway. >> +/** > > Is this a kernel doc formatted documentation? > Haven't you got a warning? Not from checkpatch, but I got one from the kernel test robot. Will fix. [...] >> + * >> + * An I2C Address Translator (ATR) is a device with an I2C slave parent >> + * ("upstream") port and N I2C master child ("downstream") ports, and >> + * forwards transactions from upstream to the appropriate downstream port >> + * with a modified slave address. The address used on the parent bus is >> + * called the "alias" and is (potentially) different from the physical >> + * slave address of the child bus. Address translation is done by the >> + * hardware. >> + * >> + * An ATR looks similar to an i2c-mux except: >> + * - the address on the parent and child busses can be different >> + * - there is normally no need to select the child port; the alias used on >> + * the parent bus implies it >> + * >> + * The ATR functionality can be provided by a chip with many other >> + * features. This file provides a helper to implement an ATR within your >> + * driver. >> + * >> + * The ATR creates a new I2C "child" adapter on each child bus. Adding >> + * devices on the child bus ends up in invoking the driver code to select >> + * an available alias. Maintaining an appropriate pool of available aliases >> + * and picking one for each new device is up to the driver implementer. The >> + * ATR maintains an table of currently assigned alias and uses it to modify >> + * all I2C transactions directed to devices on the child buses. >> + * >> + * A typical example follows. >> + * >> + * Topology: >> + * >> + * Slave X @ 0x10 >> + * .-----. | >> + * .-----. | |---+---- B >> + * | CPU |--A--| ATR | >> + * `-----' | |---+---- C >> + * `-----' | >> + * Slave Y @ 0x10 >> + * >> + * Alias table: >> + * >> + * Client Alias >> + * ------------- >> + * X 0x20 >> + * Y 0x30 >> + * >> + * Transaction: >> + * >> + * - Slave X driver sends a transaction (on adapter B), slave address 0x10 >> + * - ATR driver rewrites messages with address 0x20, forwards to adapter A >> + * - Physical I2C transaction on bus A, slave address 0x20 >> + * - ATR chip propagates transaction on bus B with address translated to 0x10 >> + * - Slave X chip replies on bus B >> + * - ATR chip forwards reply on bus A >> + * - ATR driver rewrites messages with address 0x10 >> + * - Slave X driver gets back the msgs[], with reply and address 0x10 >> + * >> + * Usage: >> + * >> + * 1. In your driver (typically in the probe function) add an ATR by >> + * calling i2c_atr_new() passing your attach/detach callbacks >> + * 2. When the attach callback is called pick an appropriate alias, >> + * configure it in your chip and return the chosen alias in the >> + * alias_id parameter >> + * 3. When the detach callback is called, deconfigure the alias from >> + * your chip and put it back in the pool for later usage >> + * >> + * Originally based on i2c-mux.c >> + */ > > Shouldn't this comment be somewhere under Documentation/ ? Uhm, yes, I agree it's a good idea to move this entire comment there. >> + if (dev->of_node) { > > This check can be dropped, also please use device property and fwnode > APIs. No good of having OF-centric generic modules nowadays. Sure! This code was written in another decade and I didn't update it... As you noticed elsewhere it also honors the old, strict 80-chars per line limit in various places where it makes no sense anymore. >> + WARN(sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"), >> + "can't create symlink to atr device\n"); >> + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", chan_id); >> + WARN(sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name), >> + "can't create symlink for channel %u\n", chan_id); > > Doesn't sysfs already has a warning when it's really needed? I have to check that. I usually don't add unnecessary log messages. [...] >> +#include <linux/i2c.h> >> +#include <linux/mutex.h> > > Missed types.h > > Missed struct device; Not sure I got your point here. This file has some 'struct device *', which do not need a declaration, and has zero non-pointer uses of 'struct device'.
On 2/16/22 10:40, Luca Ceresoli wrote: > On 08/02/22 12:16, Andy Shevchenko wrote: >> On Mon, Feb 7, 2022 at 7:55 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>> +config I2C_ATR >>> + tristate "I2C Address Translator (ATR) support" >>> + help >>> + Enable support for I2C Address Translator (ATR) chips. >>> + >>> + An ATR allows accessing multiple I2C busses from a single >>> + physical bus via address translation instead of bus selection as >>> + i2c-muxes do. >> >> What would be the module name? > > Isn't the module name written in Kconfig files just to avoid checkpatch > complain about "too few doc lines"? :) Oook, it's i2s-atr anyway. Thanks Luca! I have always wondered why people keep adding this seemingly unnecessary boilerplate. Now I finally get the purpose! --Matti
Hi dee Ho peeps! On 2/6/22 13:59, Luca Ceresoli wrote: > An ATR is a device that looks similar to an i2c-mux: it has an I2C > slave "upstream" port and N master "downstream" ports, and forwards > transactions from upstream to the appropriate downstream port. But is > is different in that the forwarded transaction has a different slave > address. The address used on the upstream bus is called the "alias" > and is (potentially) different from the physical slave address of the > downstream chip. > > Add a helper file (just like i2c-mux.c for a mux or switch) to allow > implementing ATR features in a device driver. The helper takes care or > adapter creation/destruction and translates addresses at each transaction. > snip > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig > index 438905e2a1d0..c6d1a345ea6d 100644 > --- a/drivers/i2c/Kconfig > +++ b/drivers/i2c/Kconfig > @@ -71,6 +71,15 @@ config I2C_MUX > > source "drivers/i2c/muxes/Kconfig" > > +config I2C_ATR > + tristate "I2C Address Translator (ATR) support" > + help > + Enable support for I2C Address Translator (ATR) chips. > + > + An ATR allows accessing multiple I2C busses from a single > + physical bus via address translation instead of bus selection as > + i2c-muxes do. > + I continued playing with the ROHM (de-)serializer and ended up having .config where the I2C_ATR was ='m', while my ATR driver was ='y' even though it selects the I2C_ATR. Yep, most probably my error somewhere. Anyways, this made me think that most of the I2C_ATR users are likely to just silently select the I2C_ATR, right? The I2C_ATR has no much reason to be compiled in w/o users, right? So perhaps the menu entry for selecting the I2C_ATR could be dropped(?) Do we really need this entry in already long list of configs to be manually picked? snip > +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev, > + const struct i2c_atr_ops *ops, int max_adapters) > +{ > + struct i2c_atr *atr; > + > + if (!ops || !ops->attach_client || !ops->detach_client) > + return ERR_PTR(-EINVAL); > + I believe that most of the attach_client implementations will have similar approach of allocating and populating an address-pool and searching for first unused address. As a 'further dev' it'd be great to see a common helper implementation for attach/detach - perhaps so that the atr drivers would only need to specify the slave-address configuration register(s) / mask and the use a 'generic' attach/detach helpers. Well, just thinking how to reduce the code from actual IC drivers but this is really not something that is required during this initial series :) Also, devm-variants would be great - although that falls to the same category of things that do not need to be done immediately - but would perhaps be worth considering in the future. so, perhaps reconsider the Kconfig but for what-ever it is worth: Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Yours Matti
Hi Matti, On 16/03/22 15:11, Vaittinen, Matti wrote: > Hi dee Ho peeps! > > On 2/6/22 13:59, Luca Ceresoli wrote: >> An ATR is a device that looks similar to an i2c-mux: it has an I2C >> slave "upstream" port and N master "downstream" ports, and forwards >> transactions from upstream to the appropriate downstream port. But is >> is different in that the forwarded transaction has a different slave >> address. The address used on the upstream bus is called the "alias" >> and is (potentially) different from the physical slave address of the >> downstream chip. >> >> Add a helper file (just like i2c-mux.c for a mux or switch) to allow >> implementing ATR features in a device driver. The helper takes care or >> adapter creation/destruction and translates addresses at each transaction. >> > > snip > >> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig >> index 438905e2a1d0..c6d1a345ea6d 100644 >> --- a/drivers/i2c/Kconfig >> +++ b/drivers/i2c/Kconfig >> @@ -71,6 +71,15 @@ config I2C_MUX >> >> source "drivers/i2c/muxes/Kconfig" >> >> +config I2C_ATR >> + tristate "I2C Address Translator (ATR) support" >> + help >> + Enable support for I2C Address Translator (ATR) chips. >> + >> + An ATR allows accessing multiple I2C busses from a single >> + physical bus via address translation instead of bus selection as >> + i2c-muxes do. >> + > > I continued playing with the ROHM (de-)serializer and ended up having > .config where the I2C_ATR was ='m', while my ATR driver was ='y' even > though it selects the I2C_ATR. > > Yep, most probably my error somewhere. > > Anyways, this made me think that most of the I2C_ATR users are likely to > just silently select the I2C_ATR, right? The I2C_ATR has no much reason > to be compiled in w/o users, right? So perhaps the menu entry for > selecting the I2C_ATR could be dropped(?) Do we really need this entry > in already long list of configs to be manually picked? Maybe we could make it a blind option, sure. The only reason it could be useful that it's visible is that one might implement a user driver could be written out of tree. I don't care very much about that, but it is possible. Maybe it's the reason for I2C_MUX to be a visible option too. Peter? >> +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev, >> + const struct i2c_atr_ops *ops, int max_adapters) >> +{ >> + struct i2c_atr *atr; >> + >> + if (!ops || !ops->attach_client || !ops->detach_client) >> + return ERR_PTR(-EINVAL); >> + > > I believe that most of the attach_client implementations will have > similar approach of allocating and populating an address-pool and > searching for first unused address. As a 'further dev' it'd be great to > see a common helper implementation for attach/detach - perhaps so that > the atr drivers would only need to specify the slave-address > configuration register(s) / mask and the use a 'generic' attach/detach > helpers. Well, just thinking how to reduce the code from actual IC > drivers but this is really not something that is required during this > initial series :) > > Also, devm-variants would be great - although that falls to the same > category of things that do not need to be done immediately - but would > perhaps be worth considering in the future. Both of your proposals make sense, however I did deliberately not generalize too much because I knew only one chipset. I don't like trying to generalize for an unpredictable future use case, it generally leads (me) to generalizing in the wrong direction. That means you'd be very welcome to propose helpers and/or devm variants, possibly in the same patchset as the first Rohm serdes driver. ;) > Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Thanks for your review!