| Message ID | 20181008211205.2900-1-vz@mleia.com |
|---|---|
| Headers | show |
| Series | mfd/pinctrl: add initial support of TI DS90Ux9xx ICs | expand |
Hi Vladimir, thanks for your patch! Some review comments: On Mon, Oct 8, 2018 at 11:12 PM Vladimir Zapolskiy <vz@mleia.com> wrote: > From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > > The change adds an MFD cell driver for managing pin functions on > TI DS90Ux9xx de-/serializers. > > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> Please mention in the commit that you are also adding a GPIO chip driver. > +// SPDX-License-Identifier: GPL-2.0-or-later I prefer the simple "GPL-2.0+" here. > +#include <linux/of_gpio.h> You should not need this include. If you do, something is wrong. > +#define SER_REG_PIN_CTRL 0x12 > +#define PIN_CTRL_RGB18 BIT(2) > +#define PIN_CTRL_I2S_DATA_ISLAND BIT(1) > +#define PIN_CTRL_I2S_CHANNEL_B (BIT(0) | BIT(3)) > + > +#define SER_REG_I2S_SURROUND 0x1A > +#define PIN_CTRL_I2S_SURR_BIT BIT(0) > + > +#define DES_REG_INDIRECT_PASS 0x16 > + > +#define OUTPUT_HIGH BIT(3) > +#define REMOTE_CONTROL BIT(2) > +#define DIR_INPUT BIT(1) > +#define ENABLE_GPIO BIT(0) > + > +#define GPIO_AS_INPUT (ENABLE_GPIO | DIR_INPUT) > +#define GPIO_AS_OUTPUT ENABLE_GPIO > +#define GPIO_OUTPUT_HIGH (GPIO_AS_OUTPUT | OUTPUT_HIGH) > +#define GPIO_OUTPUT_LOW GPIO_AS_OUTPUT > +#define GPIO_OUTPUT_REMOTE (GPIO_AS_OUTPUT | REMOTE_CONTROL) These have a creepily generic look, like they hit the global GPIO namespace without really clashing. It gets confusing when reading the code. Do you think you could prefix them with DS90_* or something so it is clear that these defines belong in this driver? > +static const struct gpio_chip ds90ux9xx_gpio_chip = { > + .owner = THIS_MODULE, > + .get = ds90ux9xx_gpio_get, > + .set = ds90ux9xx_gpio_set, > + .get_direction = ds90ux9xx_gpio_get_direction, > + .direction_input = ds90ux9xx_gpio_direction_input, > + .direction_output = ds90ux9xx_gpio_direction_output, > + .base = -1, > + .can_sleep = 1, This is bool so set it = true; Overall it's a very nice driver. It is pretty complex but pin control is complex so that's a fact of life. Yours, Linus Walleij
On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > > The change adds I2C device driver for TI DS90Ux9xx de-/serializers, > support of subdevice controllers is done in separate drivers, because > not all IC functionality may be needed in particular situations, and > this can be fine grained controlled in device tree. > > The development of the driver was a collaborative work, the > contribution done by Balasubramani Vivekanandan includes: > * original implementation of the driver based on a reference driver, > * regmap powered interrupt controller support on serializers, > * support of implicitly or improperly specified in device tree ICs, > * support of device properties and attributes: backward compatible > mode, low frequency operation mode, spread spectrum clock generator. > > Contribution by Steve Longerbeam: > * added ds90ux9xx_read_indirect() function, > * moved number of links property and added ds90ux9xx_num_fpd_links(), > * moved and updated ds90ux9xx_get_link_status() function to core driver, > * added fpd_link_show device attribute. > > Sandeep Jain added support of pixel clock edge configuration. > > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > --- > drivers/mfd/Kconfig | 14 + > drivers/mfd/Makefile | 1 + > drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ > include/linux/mfd/ds90ux9xx.h | 42 ++ > 4 files changed, 936 insertions(+) > create mode 100644 drivers/mfd/ds90ux9xx-core.c > create mode 100644 include/linux/mfd/ds90ux9xx.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 8c5dfdce4326..a969fa123f64 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP > boards. MSP430 firmware manages resets and power sequencing, > inputs from buttons and the IR remote, LEDs, an RTC, and more. > > +config MFD_DS90UX9XX > + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" > + depends on I2C && OF > + select MFD_CORE > + select REGMAP_I2C > + help > + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. > + > + This driver provides basic support for setting up the de-/serializer > + chips. Additional functionalities like connection handling to > + remote de-/serializers, I2C bridging, pin multiplexing, GPIO > + controller and so on are provided by separate drivers and should > + enabled individually. This is not an MFD driver. After a 30 second Google of what this device actually does, perhaps drivers/media might be a better fit?
On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > > The change adds TI DS90Ux9xx I2C bridge/alias subdevice driver and > FPD Link connection handling mechanism. > > Access to I2C devices connected to a remote de-/serializer is done in > a transparent way, on established link detection event such devices > are registered on an I2C bus, which serves a local de-/serializer IC. > > The development of the driver was a collaborative work, the > contribution done by Balasubramani Vivekanandan includes: > * original simplistic implementation of the driver, > * support of implicitly specified devices in device tree, > * support of multiple FPD links for TI DS90Ux9xx, > * other kind of valuable review comments, clean-ups and fixes. > > Also Steve Longerbeam made the following changes: > * clear address maps after linked device removal, > * disable pass-through in disconnection, > * qualify locked status with non-zero remote address. > > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > --- > drivers/mfd/Kconfig | 8 + > drivers/mfd/Makefile | 1 + > drivers/mfd/ds90ux9xx-i2c-bridge.c | 764 +++++++++++++++++++++++++++++ > 3 files changed, 773 insertions(+) > create mode 100644 drivers/mfd/ds90ux9xx-i2c-bridge.c Shouldn't this live in drivers/i2c?
Hi Lee, On 10/12/2018 09:04 AM, Lee Jones wrote: > On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > >> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >> >> The change adds TI DS90Ux9xx I2C bridge/alias subdevice driver and >> FPD Link connection handling mechanism. >> >> Access to I2C devices connected to a remote de-/serializer is done in >> a transparent way, on established link detection event such devices >> are registered on an I2C bus, which serves a local de-/serializer IC. >> >> The development of the driver was a collaborative work, the >> contribution done by Balasubramani Vivekanandan includes: >> * original simplistic implementation of the driver, >> * support of implicitly specified devices in device tree, >> * support of multiple FPD links for TI DS90Ux9xx, >> * other kind of valuable review comments, clean-ups and fixes. >> >> Also Steve Longerbeam made the following changes: >> * clear address maps after linked device removal, >> * disable pass-through in disconnection, >> * qualify locked status with non-zero remote address. >> >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >> --- >> drivers/mfd/Kconfig | 8 + >> drivers/mfd/Makefile | 1 + >> drivers/mfd/ds90ux9xx-i2c-bridge.c | 764 +++++++++++++++++++++++++++++ >> 3 files changed, 773 insertions(+) >> create mode 100644 drivers/mfd/ds90ux9xx-i2c-bridge.c > > Shouldn't this live in drivers/i2c? no, the driver is not for an I2C controller of any kind, and the driver does not register itself in the I2C subsystem by calling i2c_add_adapter() or i2c_add_numbered_adapter() or i2c_mux_add_adapter() etc, this topic was discussed with Wolfram also. Formally the driver converts the managed IC into a multi-address I2C slave device, I understand that it does not sound like a well suited driver for MFD, but ds90ux9xx-core.c and ds90ux9xx-i2c-bridge.c drivers are quite tightly coupled. -- Best wishes, Vladimir
Hi Lee, On 10/12/2018 09:03 AM, Lee Jones wrote: > On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > >> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >> >> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, >> support of subdevice controllers is done in separate drivers, because >> not all IC functionality may be needed in particular situations, and >> this can be fine grained controlled in device tree. >> >> The development of the driver was a collaborative work, the >> contribution done by Balasubramani Vivekanandan includes: >> * original implementation of the driver based on a reference driver, >> * regmap powered interrupt controller support on serializers, >> * support of implicitly or improperly specified in device tree ICs, >> * support of device properties and attributes: backward compatible >> mode, low frequency operation mode, spread spectrum clock generator. >> >> Contribution by Steve Longerbeam: >> * added ds90ux9xx_read_indirect() function, >> * moved number of links property and added ds90ux9xx_num_fpd_links(), >> * moved and updated ds90ux9xx_get_link_status() function to core driver, >> * added fpd_link_show device attribute. >> >> Sandeep Jain added support of pixel clock edge configuration. >> >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >> --- >> drivers/mfd/Kconfig | 14 + >> drivers/mfd/Makefile | 1 + >> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ >> include/linux/mfd/ds90ux9xx.h | 42 ++ >> 4 files changed, 936 insertions(+) >> create mode 100644 drivers/mfd/ds90ux9xx-core.c >> create mode 100644 include/linux/mfd/ds90ux9xx.h >> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> index 8c5dfdce4326..a969fa123f64 100644 >> --- a/drivers/mfd/Kconfig >> +++ b/drivers/mfd/Kconfig >> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP >> boards. MSP430 firmware manages resets and power sequencing, >> inputs from buttons and the IR remote, LEDs, an RTC, and more. >> >> +config MFD_DS90UX9XX >> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" >> + depends on I2C && OF >> + select MFD_CORE >> + select REGMAP_I2C >> + help >> + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. >> + >> + This driver provides basic support for setting up the de-/serializer >> + chips. Additional functionalities like connection handling to >> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO >> + controller and so on are provided by separate drivers and should >> + enabled individually. > > This is not an MFD driver. Why do you think so? The representation of the ICs into device tree format of hardware description shows that this is a truly MFD driver with multiple IP subcomponents naturally mapped into MFD cells. Basically it is possible to replace explicit of_platform_populate() by adding a "simple-mfd" compatible, if it is desired. > After a 30 second Google of what this device actually does, perhaps > drivers/media might be a better fit? > I assume it would be quite unusual to add a driver with NO media functions and controls into drivers/media. Laurent, can you please share your opinion? -- Best wishes, Vladimir
On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > On 10/12/2018 09:03 AM, Lee Jones wrote: > > On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > > > >> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >> > >> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, > >> support of subdevice controllers is done in separate drivers, because > >> not all IC functionality may be needed in particular situations, and > >> this can be fine grained controlled in device tree. > >> > >> The development of the driver was a collaborative work, the > >> contribution done by Balasubramani Vivekanandan includes: > >> * original implementation of the driver based on a reference driver, > >> * regmap powered interrupt controller support on serializers, > >> * support of implicitly or improperly specified in device tree ICs, > >> * support of device properties and attributes: backward compatible > >> mode, low frequency operation mode, spread spectrum clock generator. > >> > >> Contribution by Steve Longerbeam: > >> * added ds90ux9xx_read_indirect() function, > >> * moved number of links property and added ds90ux9xx_num_fpd_links(), > >> * moved and updated ds90ux9xx_get_link_status() function to core driver, > >> * added fpd_link_show device attribute. > >> > >> Sandeep Jain added support of pixel clock edge configuration. > >> > >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >> --- > >> drivers/mfd/Kconfig | 14 + > >> drivers/mfd/Makefile | 1 + > >> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ > >> include/linux/mfd/ds90ux9xx.h | 42 ++ > >> 4 files changed, 936 insertions(+) > >> create mode 100644 drivers/mfd/ds90ux9xx-core.c > >> create mode 100644 include/linux/mfd/ds90ux9xx.h > >> > >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >> index 8c5dfdce4326..a969fa123f64 100644 > >> --- a/drivers/mfd/Kconfig > >> +++ b/drivers/mfd/Kconfig > >> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP > >> boards. MSP430 firmware manages resets and power sequencing, > >> inputs from buttons and the IR remote, LEDs, an RTC, and more. > >> > >> +config MFD_DS90UX9XX > >> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" > >> + depends on I2C && OF > >> + select MFD_CORE > >> + select REGMAP_I2C > >> + help > >> + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. > >> + > >> + This driver provides basic support for setting up the de-/serializer > >> + chips. Additional functionalities like connection handling to > >> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO > >> + controller and so on are provided by separate drivers and should > >> + enabled individually. > > > > This is not an MFD driver. > > Why do you think so? The representation of the ICs into device tree format > of hardware description shows that this is a truly MFD driver with multiple > IP subcomponents naturally mapped into MFD cells. 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). > Basically it is possible to replace explicit of_platform_populate() by > adding a "simple-mfd" compatible, if it is desired. > > > After a 30 second Google of what this device actually does, perhaps > > drivers/media might be a better fit? > > I assume it would be quite unusual to add a driver with NO media functions > and controls into drivers/media. drivers/media may very well not be the correct place for this. In my 30 second Google, I saw that this device has a lot to do with cameras, hence my media association. If *all* else fails, there is always drivers/misc, but this should be avoided if at all possible. > Laurent, can you please share your opinion?
On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > On 10/12/2018 09:04 AM, Lee Jones wrote: > > On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > > > >> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >> > >> The change adds TI DS90Ux9xx I2C bridge/alias subdevice driver and > >> FPD Link connection handling mechanism. > >> > >> Access to I2C devices connected to a remote de-/serializer is done in > >> a transparent way, on established link detection event such devices > >> are registered on an I2C bus, which serves a local de-/serializer IC. > >> > >> The development of the driver was a collaborative work, the > >> contribution done by Balasubramani Vivekanandan includes: > >> * original simplistic implementation of the driver, > >> * support of implicitly specified devices in device tree, > >> * support of multiple FPD links for TI DS90Ux9xx, > >> * other kind of valuable review comments, clean-ups and fixes. > >> > >> Also Steve Longerbeam made the following changes: > >> * clear address maps after linked device removal, > >> * disable pass-through in disconnection, > >> * qualify locked status with non-zero remote address. > >> > >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >> --- > >> drivers/mfd/Kconfig | 8 + > >> drivers/mfd/Makefile | 1 + > >> drivers/mfd/ds90ux9xx-i2c-bridge.c | 764 +++++++++++++++++++++++++++++ > >> 3 files changed, 773 insertions(+) > >> create mode 100644 drivers/mfd/ds90ux9xx-i2c-bridge.c > > > > Shouldn't this live in drivers/i2c? > > no, the driver is not for an I2C controller of any kind, and the driver does > not register itself in the I2C subsystem by calling i2c_add_adapter() or > i2c_add_numbered_adapter() or i2c_mux_add_adapter() etc, this topic was > discussed with Wolfram also. > > Formally the driver converts the managed IC into a multi-address I2C > slave device, I understand that it does not sound like a well suited driver > for MFD, but ds90ux9xx-core.c and ds90ux9xx-i2c-bridge.c drivers are quite > tightly coupled. Using MFD as the default dumping ground for anything which does more than one thing has also been discussed before. :) You need to figure out what function this device provides and re-locate it into the subsystem which it best fits.
Hi Vladimir, On 12/10/18 09:39, Lee Jones wrote: > On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: >> On 10/12/2018 09:03 AM, Lee Jones wrote: >>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: >>> >>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>> >>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, >>>> support of subdevice controllers is done in separate drivers, because >>>> not all IC functionality may be needed in particular situations, and >>>> this can be fine grained controlled in device tree. >>>> >>>> The development of the driver was a collaborative work, the >>>> contribution done by Balasubramani Vivekanandan includes: >>>> * original implementation of the driver based on a reference driver, >>>> * regmap powered interrupt controller support on serializers, >>>> * support of implicitly or improperly specified in device tree ICs, >>>> * support of device properties and attributes: backward compatible >>>> mode, low frequency operation mode, spread spectrum clock generator. >>>> >>>> Contribution by Steve Longerbeam: >>>> * added ds90ux9xx_read_indirect() function, >>>> * moved number of links property and added ds90ux9xx_num_fpd_links(), >>>> * moved and updated ds90ux9xx_get_link_status() function to core driver, >>>> * added fpd_link_show device attribute. >>>> >>>> Sandeep Jain added support of pixel clock edge configuration. >>>> >>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>> --- >>>> drivers/mfd/Kconfig | 14 + >>>> drivers/mfd/Makefile | 1 + >>>> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ >>>> include/linux/mfd/ds90ux9xx.h | 42 ++ >>>> 4 files changed, 936 insertions(+) >>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c >>>> create mode 100644 include/linux/mfd/ds90ux9xx.h >>>> >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>> index 8c5dfdce4326..a969fa123f64 100644 >>>> --- a/drivers/mfd/Kconfig >>>> +++ b/drivers/mfd/Kconfig >>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP >>>> boards. MSP430 firmware manages resets and power sequencing, >>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. >>>> >>>> +config MFD_DS90UX9XX >>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" >>>> + depends on I2C && OF >>>> + select MFD_CORE >>>> + select REGMAP_I2C >>>> + help >>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. >>>> + >>>> + This driver provides basic support for setting up the de-/serializer >>>> + chips. Additional functionalities like connection handling to >>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO >>>> + controller and so on are provided by separate drivers and should >>>> + enabled individually. >>> >>> This is not an MFD driver. >> >> Why do you think so? The representation of the ICs into device tree format >> of hardware description shows that this is a truly MFD driver with multiple >> IP subcomponents naturally mapped into MFD cells. > > 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). > >> Basically it is possible to replace explicit of_platform_populate() by >> adding a "simple-mfd" compatible, if it is desired. >> >>> After a 30 second Google of what this device actually does, perhaps >>> drivers/media might be a better fit? >> >> I assume it would be quite unusual to add a driver with NO media functions >> and controls into drivers/media. > > drivers/media may very well not be the correct place for this. In my > 30 second Google, I saw that this device has a lot to do with cameras, > hence my media association. > > If *all* else fails, there is always drivers/misc, but this should be > avoided if at all possible. The device as a whole is FPD Link for camera devices I believe, but it certainly has different functions which are broken out in this implementation. I think it might be quite awkward having the i2c components in drivers/i2c and the media components in drivers/media/i2c, so what about creating drivers/media/i2c/fpd-link (or such) as a container? Our GMSL implementation is also a complex camera(s) device - but does not yet use the MFD framework, perhaps that's something to add to my todo list. We currently keep all of the complexity within the max9286.c driver, but I could foresee that being split further if more devices add to the complexity of managing the bus. At which point we might want an equivalent drivers/media/i2c/gmsl/ perhaps? -- Regards Kieran >> Laurent, can you please share your opinion?
Hi Kieran, On 10/12/2018 12:20 PM, Kieran Bingham wrote: > Hi Vladimir, > > On 12/10/18 09:39, Lee Jones wrote: >> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: >>> On 10/12/2018 09:03 AM, Lee Jones wrote: >>>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: >>>> >>>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>>> >>>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, >>>>> support of subdevice controllers is done in separate drivers, because >>>>> not all IC functionality may be needed in particular situations, and >>>>> this can be fine grained controlled in device tree. >>>>> >>>>> The development of the driver was a collaborative work, the >>>>> contribution done by Balasubramani Vivekanandan includes: >>>>> * original implementation of the driver based on a reference driver, >>>>> * regmap powered interrupt controller support on serializers, >>>>> * support of implicitly or improperly specified in device tree ICs, >>>>> * support of device properties and attributes: backward compatible >>>>> mode, low frequency operation mode, spread spectrum clock generator. >>>>> >>>>> Contribution by Steve Longerbeam: >>>>> * added ds90ux9xx_read_indirect() function, >>>>> * moved number of links property and added ds90ux9xx_num_fpd_links(), >>>>> * moved and updated ds90ux9xx_get_link_status() function to core driver, >>>>> * added fpd_link_show device attribute. >>>>> >>>>> Sandeep Jain added support of pixel clock edge configuration. >>>>> >>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>>> --- >>>>> drivers/mfd/Kconfig | 14 + >>>>> drivers/mfd/Makefile | 1 + >>>>> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ >>>>> include/linux/mfd/ds90ux9xx.h | 42 ++ >>>>> 4 files changed, 936 insertions(+) >>>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c >>>>> create mode 100644 include/linux/mfd/ds90ux9xx.h >>>>> >>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>>> index 8c5dfdce4326..a969fa123f64 100644 >>>>> --- a/drivers/mfd/Kconfig >>>>> +++ b/drivers/mfd/Kconfig >>>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP >>>>> boards. MSP430 firmware manages resets and power sequencing, >>>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. >>>>> >>>>> +config MFD_DS90UX9XX >>>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" >>>>> + depends on I2C && OF >>>>> + select MFD_CORE >>>>> + select REGMAP_I2C >>>>> + help >>>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. >>>>> + >>>>> + This driver provides basic support for setting up the de-/serializer >>>>> + chips. Additional functionalities like connection handling to >>>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO >>>>> + controller and so on are provided by separate drivers and should >>>>> + enabled individually. >>>> >>>> This is not an MFD driver. >>> >>> Why do you think so? The representation of the ICs into device tree format >>> of hardware description shows that this is a truly MFD driver with multiple >>> IP subcomponents naturally mapped into MFD cells. >> >> 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). >> >>> Basically it is possible to replace explicit of_platform_populate() by >>> adding a "simple-mfd" compatible, if it is desired. >>> >>>> After a 30 second Google of what this device actually does, perhaps >>>> drivers/media might be a better fit? >>> >>> I assume it would be quite unusual to add a driver with NO media functions >>> and controls into drivers/media. >> >> drivers/media may very well not be the correct place for this. In my >> 30 second Google, I saw that this device has a lot to do with cameras, >> hence my media association. >> >> If *all* else fails, there is always drivers/misc, but this should be >> avoided if at all possible. > > The device as a whole is FPD Link for camera devices I believe, but it I still don't understand (I could be biased though) why there is such a strong emphasis on cameras and media stuff in the discussion. No, "the device as a whole is FPD Link for camera devices" is a wrong statement. On hand I have a number of boards with serializers/deserializers from the TI DS90Ux9xx IC series and sensors are NOT connected to them. > certainly has different functions which are broken out in this > implementation. No, there is absolutely nothing broken out from the presented MFD drivers, the drivers are completely integral and basically I don't expect any. If you are concerned about media functionality, the correspondent MFD *cell* drivers will be added into drivers/media, drivers/gpu/drm or whatever is to be a proper place. > I think it might be quite awkward having the i2c components in > drivers/i2c and the media components in drivers/media/i2c, so what about > creating drivers/media/i2c/fpd-link (or such) as a container? I open drivers/media/i2c/Kconfig and all entries with no exception are under from 'if VIDEO_V4L2'. The MFD drivers do NOT require on depend on VIDEO_V4L2 or any other multimedia frameworks, nor the MFD drivers export any multimedia controls. > Our GMSL implementation is also a complex camera(s) device - but does > not yet use the MFD framework, perhaps that's something to add to my > todo list. > Okay, but the TI DS90Ux9xx is NOT a camera device, and it is NOT a multimedia device, but it is a pure MFD device so the argument is not applicable. > We currently keep all of the complexity within the max9286.c driver, but > I could foresee that being split further if more devices add to the > complexity of managing the bus. At which point we might want an > equivalent drivers/media/i2c/gmsl/ perhaps? > -- Best wishes, Vladimir >>> Laurent, can you please share your opinion? >
On 10/12/2018 11:39 AM, Lee Jones wrote: > On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: >> On 10/12/2018 09:03 AM, Lee Jones wrote: >>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: >>> >>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>> >>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, >>>> support of subdevice controllers is done in separate drivers, because >>>> not all IC functionality may be needed in particular situations, and >>>> this can be fine grained controlled in device tree. >>>> >>>> The development of the driver was a collaborative work, the >>>> contribution done by Balasubramani Vivekanandan includes: >>>> * original implementation of the driver based on a reference driver, >>>> * regmap powered interrupt controller support on serializers, >>>> * support of implicitly or improperly specified in device tree ICs, >>>> * support of device properties and attributes: backward compatible >>>> mode, low frequency operation mode, spread spectrum clock generator. >>>> >>>> Contribution by Steve Longerbeam: >>>> * added ds90ux9xx_read_indirect() function, >>>> * moved number of links property and added ds90ux9xx_num_fpd_links(), >>>> * moved and updated ds90ux9xx_get_link_status() function to core driver, >>>> * added fpd_link_show device attribute. >>>> >>>> Sandeep Jain added support of pixel clock edge configuration. >>>> >>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>> --- >>>> drivers/mfd/Kconfig | 14 + >>>> drivers/mfd/Makefile | 1 + >>>> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ >>>> include/linux/mfd/ds90ux9xx.h | 42 ++ >>>> 4 files changed, 936 insertions(+) >>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c >>>> create mode 100644 include/linux/mfd/ds90ux9xx.h >>>> >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>> index 8c5dfdce4326..a969fa123f64 100644 >>>> --- a/drivers/mfd/Kconfig >>>> +++ b/drivers/mfd/Kconfig >>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP >>>> boards. MSP430 firmware manages resets and power sequencing, >>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. >>>> >>>> +config MFD_DS90UX9XX >>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" >>>> + depends on I2C && OF >>>> + select MFD_CORE >>>> + select REGMAP_I2C >>>> + help >>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. >>>> + >>>> + This driver provides basic support for setting up the de-/serializer >>>> + chips. Additional functionalities like connection handling to >>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO >>>> + controller and so on are provided by separate drivers and should >>>> + enabled individually. >>> >>> This is not an MFD driver. >> >> Why do you think so? The representation of the ICs into device tree format >> of hardware description shows that this is a truly MFD driver with multiple >> IP subcomponents naturally mapped into MFD cells. > > 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'. > What is the reason why device drivers for sort of multimedia ICs like WL1273, WM8994 and other numerous codecs are found in drivers/mfd? If the same reason can not be applied to this DS90Ux9xx driver, why? > 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). > >> Basically it is possible to replace explicit of_platform_populate() by >> adding a "simple-mfd" compatible, if it is desired. >> >>> After a 30 second Google of what this device actually does, perhaps >>> drivers/media might be a better fit? >> >> I assume it would be quite unusual to add a driver with NO media functions >> and controls into drivers/media. > > drivers/media may very well not be the correct place for this. In my > 30 second Google, I saw that this device has a lot to do with cameras, > hence my media association. Well, the argument is similar to the statement that Google says that camera sensors *can* be connected to NXP i.MX6 SoC, thus arch/arm/mach-imx contents should be placed into drivers/media A few TI DS90Ux9xx *cell* drivers may be added to drivers/media, but it is out of the scope of the current series, which is completely integral per se, and actually the cover letter says that the series of drivers immediately allows to output video over DRM to panels, but the discussion is around sensors by some reason. But I hope it won't be seen as a misleading reason to consider to add the MFD driver into drivers/gpu/drm > If *all* else fails, there is always drivers/misc, but this should be > avoided if at all possible. drivers/misc does not sound like a proper place for the MFD driver... >> Laurent, can you please share your opinion? > -- Best wishes, Vladimir
On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > On 10/12/2018 12:20 PM, Kieran Bingham wrote: > > Hi Vladimir, > > On 12/10/18 09:39, Lee Jones wrote: > >> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > >>> On 10/12/2018 09:03 AM, Lee Jones wrote: > >>>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > >>>> > >>>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>>> > >>>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, > >>>>> support of subdevice controllers is done in separate drivers, because > >>>>> not all IC functionality may be needed in particular situations, and > >>>>> this can be fine grained controlled in device tree. > >>>>> > >>>>> The development of the driver was a collaborative work, the > >>>>> contribution done by Balasubramani Vivekanandan includes: > >>>>> * original implementation of the driver based on a reference driver, > >>>>> * regmap powered interrupt controller support on serializers, > >>>>> * support of implicitly or improperly specified in device tree ICs, > >>>>> * support of device properties and attributes: backward compatible > >>>>> mode, low frequency operation mode, spread spectrum clock generator. > >>>>> > >>>>> Contribution by Steve Longerbeam: > >>>>> * added ds90ux9xx_read_indirect() function, > >>>>> * moved number of links property and added ds90ux9xx_num_fpd_links(), > >>>>> * moved and updated ds90ux9xx_get_link_status() function to core driver, > >>>>> * added fpd_link_show device attribute. > >>>>> > >>>>> Sandeep Jain added support of pixel clock edge configuration. > >>>>> > >>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>>> --- > >>>>> drivers/mfd/Kconfig | 14 + > >>>>> drivers/mfd/Makefile | 1 + > >>>>> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ > >>>>> include/linux/mfd/ds90ux9xx.h | 42 ++ > >>>>> 4 files changed, 936 insertions(+) > >>>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c > >>>>> create mode 100644 include/linux/mfd/ds90ux9xx.h > >>>>> > >>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >>>>> index 8c5dfdce4326..a969fa123f64 100644 > >>>>> --- a/drivers/mfd/Kconfig > >>>>> +++ b/drivers/mfd/Kconfig > >>>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP > >>>>> boards. MSP430 firmware manages resets and power sequencing, > >>>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. > >>>>> > >>>>> +config MFD_DS90UX9XX > >>>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" > >>>>> + depends on I2C && OF > >>>>> + select MFD_CORE > >>>>> + select REGMAP_I2C > >>>>> + help > >>>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. > >>>>> + > >>>>> + This driver provides basic support for setting up the de-/serializer > >>>>> + chips. Additional functionalities like connection handling to > >>>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO > >>>>> + controller and so on are provided by separate drivers and should > >>>>> + enabled individually. > >>>> > >>>> This is not an MFD driver. > >>> > >>> Why do you think so? The representation of the ICs into device tree format > >>> of hardware description shows that this is a truly MFD driver with multiple > >>> IP subcomponents naturally mapped into MFD cells. > >> > >> 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). > >> > >>> Basically it is possible to replace explicit of_platform_populate() by > >>> adding a "simple-mfd" compatible, if it is desired. > >>> > >>>> After a 30 second Google of what this device actually does, perhaps > >>>> drivers/media might be a better fit? > >>> > >>> I assume it would be quite unusual to add a driver with NO media functions > >>> and controls into drivers/media. > >> > >> drivers/media may very well not be the correct place for this. In my > >> 30 second Google, I saw that this device has a lot to do with cameras, > >> hence my media association. > >> > >> If *all* else fails, there is always drivers/misc, but this should be > >> avoided if at all possible. > > > > The device as a whole is FPD Link for camera devices I believe, but it > > I still don't understand (I could be biased though) why there is such > a strong emphasis on cameras and media stuff in the discussion. > > No, "the device as a whole is FPD Link for camera devices" is a wrong > statement. On hand I have a number of boards with serializers/deserializers > from the TI DS90Ux9xx IC series and sensors are NOT connected to them. > > > certainly has different functions which are broken out in this > > implementation. > > No, there is absolutely nothing broken out from the presented MFD drivers, > the drivers are completely integral and basically I don't expect any. > > If you are concerned about media functionality, the correspondent MFD > *cell* drivers will be added into drivers/media, drivers/gpu/drm or > whatever is to be a proper place. > > > I think it might be quite awkward having the i2c components in > > drivers/i2c and the media components in drivers/media/i2c, so what about > > creating drivers/media/i2c/fpd-link (or such) as a container? > > I open drivers/media/i2c/Kconfig and all entries with no exception are > under from 'if VIDEO_V4L2'. The MFD drivers do NOT require on depend on > VIDEO_V4L2 or any other multimedia frameworks, nor the MFD drivers export > any multimedia controls. > > > Our GMSL implementation is also a complex camera(s) device - but does > > not yet use the MFD framework, perhaps that's something to add to my > > todo list. > > > > Okay, but the TI DS90Ux9xx is NOT a camera device, and it is NOT a multimedia > device, but it is a pure MFD device so the argument is not applicable. You keep saying that "this is an MFD device" without any obvious comprehension of what an MFD is. Just saying that it is one over-and-over does not make it so. An MFD should be little more than parent to other functional devices. Their role is to register children which in turn conduct operations on the hardware in a useful way. Some MFDs also house common functions to save repetition of code in each of the child devices. They do not tend to offer any useful functionality (stuff) in their own right. As I already mentioned, you need to figure out what this device is and move all of the functionality into the appropriate subsystem. Since an MFD isn't a real thing/device (it's a Linuxy-shim which only serves to register sub-devices and (sometimes) provide a space for common functionality to be located), drivers/mfd is not the subsystem which you seek. > > We currently keep all of the complexity within the max9286.c driver, but > > I could foresee that being split further if more devices add to the > > complexity of managing the bus. At which point we might want an > > equivalent drivers/media/i2c/gmsl/ perhaps?
Hi Vladimir, Thank you for the patches. On Tuesday, 9 October 2018 00:11:58 EEST Vladimir Zapolskiy wrote: > The published drivers describe the essential and generic parts of > TI DS90Ux9xx series of ICs, which allow to transfer video, audio and > control signals between FPD-Link III serializers and deserializers. > > The placement of TI DS90Ux9xx I2C client driver was selected to be > drivers/mfd as the most natural location of a true MFD driver, Why does this have to be an MFD driver ? It's not uncommon for random devices to expose a few GPIOs or clocks to the outside world, to support their main feature (video transmission in this case). HDMI cables transport I2C on the DDC pins, and HDMI encoders usually have an I2C master controller to read EDID, and a GPIO pin to connect to the HPD signal. We don't model them as MFD drivers. You can register an I2C adapter and a GPIO controller from within any driver without a need to involve the MFD framework. I think it's completely overkill in this case. > apparently drivers/media/i2c is for another type of device drivers, > also DS90Ux9xx I2C bridge subcontroller driver is placed nearby, > because drivers/i2c for it would be an inappropriate destination > as well. Informally the TI DS90Ux9xx ICs serve a similar function > to SMSC/Microchip MOST, and its drivers are in drivers/staging/most, > the final destination is unknown to me. Please feel free to advise > a better location for the published drivers, at the moment the core > drivers are in drivers/mfd, but I select linux-media as a mailing list. > > The published drivers instantly give a chance to test video bridge > functionality to a TI DS90Ux9xx deserializer equipped display panel > with the aide of Laurent's "lvds-encoder" driver by misusing it > as a generic and transparent drm bridge with no particular LVDS > specifics in it, for that it should be sufficient just to add the > corresponding device node and input/output ports as children of > a serializer connected to an application controller. > > While the selected scheme of IC description by a list of subdevices, > where each one described in its own device node, works pretty well, > it might lead to unnecessary overcomplicated description of connections > between subdevices on serializer and deserializer sides, i.e. for > proper description of links/connections video serializer should > be linked to video deserializer, audio serializer should be linked > to audio deserializer and so on, however formally there is just one > FPD-III Link connection between two ICs. Could you please provide a complete DT example ? The series is titled "initial support", but it's hard to ascertain that it's taking the right direction without seeing where you want to go. In particular, I want to see how devices on both sides of the serializer and deserializer will be modeled. > The series of patches is rebased on top of linux-next, and there are > more changes in the queue to provide better support of TI DS90Ux9xx ICs. > > The introduction to the ICs and drivers can be found in my presentation > https://schd.ws/hosted_files/ossalsjp18/8a/vzapolskiy_als2018.pdf > > Sandeep Jain (1): > dt-bindings: mfd: ds90ux9xx: add description of TI DS90Ux9xx ICs > > Vladimir Zapolskiy (6): > dt-bindings: mfd: ds90ux9xx: add description of TI DS90Ux9xx I2C bridge > dt-bindings: pinctrl: ds90ux9xx: add description of TI DS90Ux9xx pinmux > mfd: ds90ux9xx: add TI DS90Ux9xx de-/serializer MFD driver > mfd: ds90ux9xx: add I2C bridge/alias and link connection driver > pinctrl: ds90ux9xx: add TI DS90Ux9xx pinmux and GPIO controller driver > MAINTAINERS: add entry for TI DS90Ux9xx FPD-Link III drivers > > .../bindings/mfd/ti,ds90ux9xx-i2c-bridge.txt | 61 ++ > .../devicetree/bindings/mfd/ti,ds90ux9xx.txt | 66 ++ > .../bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt | 83 ++ > MAINTAINERS | 10 + > drivers/mfd/Kconfig | 22 + > drivers/mfd/Makefile | 2 + > drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++ > drivers/mfd/ds90ux9xx-i2c-bridge.c | 764 ++++++++++++++ > drivers/pinctrl/Kconfig | 11 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/pinctrl-ds90ux9xx.c | 970 ++++++++++++++++++ > include/linux/mfd/ds90ux9xx.h | 42 + > 12 files changed, 2911 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/mfd/ti,ds90ux9xx-i2c-bridge.txt create > mode 100644 Documentation/devicetree/bindings/mfd/ti,ds90ux9xx.txt create > mode 100644 > Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt create > mode 100644 drivers/mfd/ds90ux9xx-core.c > create mode 100644 drivers/mfd/ds90ux9xx-i2c-bridge.c > create mode 100644 drivers/pinctrl/pinctrl-ds90ux9xx.c > create mode 100644 include/linux/mfd/ds90ux9xx.h
On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > On 10/12/2018 11:39 AM, Lee Jones wrote: > > On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > >> On 10/12/2018 09:03 AM, Lee Jones wrote: > >>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > >>> > >>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>> > >>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, > >>>> support of subdevice controllers is done in separate drivers, because > >>>> not all IC functionality may be needed in particular situations, and > >>>> this can be fine grained controlled in device tree. > >>>> > >>>> The development of the driver was a collaborative work, the > >>>> contribution done by Balasubramani Vivekanandan includes: > >>>> * original implementation of the driver based on a reference driver, > >>>> * regmap powered interrupt controller support on serializers, > >>>> * support of implicitly or improperly specified in device tree ICs, > >>>> * support of device properties and attributes: backward compatible > >>>> mode, low frequency operation mode, spread spectrum clock generator. > >>>> > >>>> Contribution by Steve Longerbeam: > >>>> * added ds90ux9xx_read_indirect() function, > >>>> * moved number of links property and added ds90ux9xx_num_fpd_links(), > >>>> * moved and updated ds90ux9xx_get_link_status() function to core driver, > >>>> * added fpd_link_show device attribute. > >>>> > >>>> Sandeep Jain added support of pixel clock edge configuration. > >>>> > >>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>> --- > >>>> drivers/mfd/Kconfig | 14 + > >>>> drivers/mfd/Makefile | 1 + > >>>> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ > >>>> include/linux/mfd/ds90ux9xx.h | 42 ++ > >>>> 4 files changed, 936 insertions(+) > >>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c > >>>> create mode 100644 include/linux/mfd/ds90ux9xx.h > >>>> > >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >>>> index 8c5dfdce4326..a969fa123f64 100644 > >>>> --- a/drivers/mfd/Kconfig > >>>> +++ b/drivers/mfd/Kconfig > >>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP > >>>> boards. MSP430 firmware manages resets and power sequencing, > >>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. > >>>> > >>>> +config MFD_DS90UX9XX > >>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" > >>>> + depends on I2C && OF > >>>> + select MFD_CORE > >>>> + select REGMAP_I2C > >>>> + help > >>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. > >>>> + > >>>> + This driver provides basic support for setting up the de-/serializer > >>>> + chips. Additional functionalities like connection handling to > >>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO > >>>> + controller and so on are provided by separate drivers and should > >>>> + enabled individually. > >>> > >>> This is not an MFD driver. > >> > >> Why do you think so? The representation of the ICs into device tree format > >> of hardware description shows that this is a truly MFD driver with multiple > >> IP subcomponents naturally mapped into MFD cells. > > > > 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'. > > > > What is the reason why device drivers for sort of multimedia ICs like > WL1273, WM8994 and other numerous codecs are found in drivers/mfd? > > If the same reason can not be applied to this DS90Ux9xx driver, why? > > > 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). > > > >> Basically it is possible to replace explicit of_platform_populate() by > >> adding a "simple-mfd" compatible, if it is desired. > >> > >>> After a 30 second Google of what this device actually does, perhaps > >>> drivers/media might be a better fit? > >> > >> I assume it would be quite unusual to add a driver with NO media functions > >> and controls into drivers/media. > > > > drivers/media may very well not be the correct place for this. In my > > 30 second Google, I saw that this device has a lot to do with cameras, > > hence my media association. > > Well, the argument is similar to the statement that Google says that > camera sensors *can* be connected to NXP i.MX6 SoC, thus arch/arm/mach-imx > contents should be placed into drivers/media > > A few TI DS90Ux9xx *cell* drivers may be added to drivers/media, but it is > out of the scope of the current series, which is completely integral per se, > and actually the cover letter says that the series of drivers immediately > allows to output video over DRM to panels, but the discussion is around > sensors by some reason. But I hope it won't be seen as a misleading > reason to consider to add the MFD driver into drivers/gpu/drm This discussion isn't about not adding enough child devices. It's about there being too much functional work being done in an MFD driver, where it doesn't belong. > > If *all* else fails, there is always drivers/misc, but this should be > > avoided if at all possible. > > drivers/misc does not sound like a proper place for the MFD driver... I'd agree with you if this were an MFD driver. As I mentioned before, there may well be an argument for and MFD driver to be part of this driver-set. However it needs to be significantly reduced with any functional code removed and placed where it belongs. > >> Laurent, can you please share your opinion?
Hi Vladimir, On 12/10/18 11:58, Vladimir Zapolskiy wrote: > Hi Kieran, > > On 10/12/2018 12:20 PM, Kieran Bingham wrote: >> Hi Vladimir, >> >> On 12/10/18 09:39, Lee Jones wrote: >>> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: >>>> On 10/12/2018 09:03 AM, Lee Jones wrote: >>>>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: >>>>> >>>>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>>>> >>>>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, >>>>>> support of subdevice controllers is done in separate drivers, because >>>>>> not all IC functionality may be needed in particular situations, and >>>>>> this can be fine grained controlled in device tree. >>>>>> >>>>>> The development of the driver was a collaborative work, the >>>>>> contribution done by Balasubramani Vivekanandan includes: >>>>>> * original implementation of the driver based on a reference driver, >>>>>> * regmap powered interrupt controller support on serializers, >>>>>> * support of implicitly or improperly specified in device tree ICs, >>>>>> * support of device properties and attributes: backward compatible >>>>>> mode, low frequency operation mode, spread spectrum clock generator. >>>>>> >>>>>> Contribution by Steve Longerbeam: >>>>>> * added ds90ux9xx_read_indirect() function, >>>>>> * moved number of links property and added ds90ux9xx_num_fpd_links(), >>>>>> * moved and updated ds90ux9xx_get_link_status() function to core driver, >>>>>> * added fpd_link_show device attribute. >>>>>> >>>>>> Sandeep Jain added support of pixel clock edge configuration. >>>>>> >>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>>>> --- >>>>>> drivers/mfd/Kconfig | 14 + >>>>>> drivers/mfd/Makefile | 1 + >>>>>> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ >>>>>> include/linux/mfd/ds90ux9xx.h | 42 ++ >>>>>> 4 files changed, 936 insertions(+) >>>>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c >>>>>> create mode 100644 include/linux/mfd/ds90ux9xx.h >>>>>> >>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>>>> index 8c5dfdce4326..a969fa123f64 100644 >>>>>> --- a/drivers/mfd/Kconfig >>>>>> +++ b/drivers/mfd/Kconfig >>>>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP >>>>>> boards. MSP430 firmware manages resets and power sequencing, >>>>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. >>>>>> >>>>>> +config MFD_DS90UX9XX >>>>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" >>>>>> + depends on I2C && OF >>>>>> + select MFD_CORE >>>>>> + select REGMAP_I2C >>>>>> + help >>>>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. >>>>>> + >>>>>> + This driver provides basic support for setting up the de-/serializer >>>>>> + chips. Additional functionalities like connection handling to >>>>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO >>>>>> + controller and so on are provided by separate drivers and should >>>>>> + enabled individually. >>>>> >>>>> This is not an MFD driver. >>>> >>>> Why do you think so? The representation of the ICs into device tree format >>>> of hardware description shows that this is a truly MFD driver with multiple >>>> IP subcomponents naturally mapped into MFD cells. >>> >>> 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). >>> >>>> Basically it is possible to replace explicit of_platform_populate() by >>>> adding a "simple-mfd" compatible, if it is desired. >>>> >>>>> After a 30 second Google of what this device actually does, perhaps >>>>> drivers/media might be a better fit? >>>> >>>> I assume it would be quite unusual to add a driver with NO media functions >>>> and controls into drivers/media. >>> >>> drivers/media may very well not be the correct place for this. In my >>> 30 second Google, I saw that this device has a lot to do with cameras, >>> hence my media association. >>> >>> If *all* else fails, there is always drivers/misc, but this should be >>> avoided if at all possible. >> >> The device as a whole is FPD Link for camera devices I believe, but it > > I still don't understand (I could be biased though) why there is such > a strong emphasis on cameras and media stuff in the discussion. > > No, "the device as a whole is FPD Link for camera devices" is a wrong > statement. On hand I have a number of boards with serializers/deserializers > from the TI DS90Ux9xx IC series and sensors are NOT connected to them. Yes - My apologies, this is a good point. Especially as the clue is in the name "Flat Panel Display". I have been stuck with my GMSL hat on for too long. Even GMSL is in the same boat. It's just that 'we' are using GMSL for cameras, but it can be used equally for displays and data. These devices are serialiser-deserialiser pairs with power and control paths. Essentially they are multi purpose buses - which do not yet have a home. We have used media as a home because of our use case. The use case whether they transfer frames from a camera or to a display are of course closely related, but ultimately covered by two separate subsystems at the pixel level (DRM vs V4L, or other for other data) Perhaps as they are buses - on a level with USB or I2C (except they can of course carry I2C or Serial as well as 'bi-directional video' etc ), they are looking for their own subsystem. Except I don't think we don't want to add a new subsystem for just one (or two) devices... -- Kieran >> certainly has different functions which are broken out in this >> implementation. > > No, there is absolutely nothing broken out from the presented MFD drivers, > the drivers are completely integral and basically I don't expect any. > > If you are concerned about media functionality, the correspondent MFD > *cell* drivers will be added into drivers/media, drivers/gpu/drm or > whatever is to be a proper place. > >> I think it might be quite awkward having the i2c components in >> drivers/i2c and the media components in drivers/media/i2c, so what about >> creating drivers/media/i2c/fpd-link (or such) as a container? > > I open drivers/media/i2c/Kconfig and all entries with no exception are > under from 'if VIDEO_V4L2'. The MFD drivers do NOT require on depend on > VIDEO_V4L2 or any other multimedia frameworks, nor the MFD drivers export > any multimedia controls. > >> Our GMSL implementation is also a complex camera(s) device - but does >> not yet use the MFD framework, perhaps that's something to add to my >> todo list. >> > > Okay, but the TI DS90Ux9xx is NOT a camera device, and it is NOT a multimedia > device, but it is a pure MFD device so the argument is not applicable. > >> We currently keep all of the complexity within the max9286.c driver, but >> I could foresee that being split further if more devices add to the >> complexity of managing the bus. At which point we might want an >> equivalent drivers/media/i2c/gmsl/ perhaps? >> > > -- > Best wishes, > Vladimir > >>>> Laurent, can you please share your opinion? >>
Hello Vladimir, On Friday, 12 October 2018 14:47:52 EEST Kieran Bingham wrote: > On 12/10/18 11:58, Vladimir Zapolskiy wrote: > > On 10/12/2018 12:20 PM, Kieran Bingham wrote: > >> On 12/10/18 09:39, Lee Jones wrote: > >>> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > >>>> On 10/12/2018 09:03 AM, Lee Jones wrote: > >>>>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > >>>>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>>>> > >>>>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, > >>>>>> support of subdevice controllers is done in separate drivers, because > >>>>>> not all IC functionality may be needed in particular situations, and > >>>>>> this can be fine grained controlled in device tree. > >>>>>> > >>>>>> The development of the driver was a collaborative work, the > >>>>>> contribution done by Balasubramani Vivekanandan includes: > >>>>>> * original implementation of the driver based on a reference driver, > >>>>>> * regmap powered interrupt controller support on serializers, > >>>>>> * support of implicitly or improperly specified in device tree ICs, > >>>>>> * support of device properties and attributes: backward compatible > >>>>>> > >>>>>> mode, low frequency operation mode, spread spectrum clock > >>>>>> generator. > >>>>>> > >>>>>> Contribution by Steve Longerbeam: > >>>>>> * added ds90ux9xx_read_indirect() function, > >>>>>> * moved number of links property and added ds90ux9xx_num_fpd_links(), > >>>>>> * moved and updated ds90ux9xx_get_link_status() function to core > >>>>>> driver, > >>>>>> * added fpd_link_show device attribute. > >>>>>> > >>>>>> Sandeep Jain added support of pixel clock edge configuration. > >>>>>> > >>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>>>> --- > >>>>>> > >>>>>> drivers/mfd/Kconfig | 14 + > >>>>>> drivers/mfd/Makefile | 1 + > >>>>>> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++ > >>>>>> include/linux/mfd/ds90ux9xx.h | 42 ++ > >>>>>> 4 files changed, 936 insertions(+) > >>>>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c > >>>>>> create mode 100644 include/linux/mfd/ds90ux9xx.h > >>>>>> > >>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >>>>>> index 8c5dfdce4326..a969fa123f64 100644 > >>>>>> --- a/drivers/mfd/Kconfig > >>>>>> +++ b/drivers/mfd/Kconfig > >>>>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP > >>>>>> > >>>>>> boards. MSP430 firmware manages resets and power sequencing, > >>>>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. > >>>>>> > >>>>>> +config MFD_DS90UX9XX > >>>>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" > >>>>>> + depends on I2C && OF > >>>>>> + select MFD_CORE > >>>>>> + select REGMAP_I2C > >>>>>> + help > >>>>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer > >>>>>> ICs. > >>>>>> + > >>>>>> + This driver provides basic support for setting up the > >>>>>> de-/serializer > >>>>>> + chips. Additional functionalities like connection handling to > >>>>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO > >>>>>> + controller and so on are provided by separate drivers and should > >>>>>> + enabled individually. > >>>>> > >>>>> This is not an MFD driver. > >>>> > >>>> Why do you think so? The representation of the ICs into device tree > >>>> format of hardware description shows that this is a truly MFD driver > >>>> with multiple IP subcomponents naturally mapped into MFD cells. > >>> > >>> 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). > >>> > >>>> Basically it is possible to replace explicit of_platform_populate() by > >>>> adding a "simple-mfd" compatible, if it is desired. > >>>> > >>>>> After a 30 second Google of what this device actually does, perhaps > >>>>> drivers/media might be a better fit? > >>>> > >>>> I assume it would be quite unusual to add a driver with NO media > >>>> functions and controls into drivers/media. > >>> > >>> drivers/media may very well not be the correct place for this. In my > >>> 30 second Google, I saw that this device has a lot to do with cameras, > >>> hence my media association. > >>> > >>> If *all* else fails, there is always drivers/misc, but this should be > >>> avoided if at all possible. > >> > >> The device as a whole is FPD Link for camera devices I believe, but it > > > > I still don't understand (I could be biased though) why there is such > > a strong emphasis on cameras and media stuff in the discussion. > > > > No, "the device as a whole is FPD Link for camera devices" is a wrong > > statement. On hand I have a number of boards with > > serializers/deserializers from the TI DS90Ux9xx IC series and sensors are > > NOT connected to them. I understand what is not connected to them, but could you tell us what is connected then ? :-) > Yes - My apologies, this is a good point. > > Especially as the clue is in the name "Flat Panel Display". > I have been stuck with my GMSL hat on for too long. > > Even GMSL is in the same boat. It's just that 'we' are using GMSL for > cameras, but it can be used equally for displays and data. > > These devices are serialiser-deserialiser pairs with power and control > paths. And data paths, that are designed to transport video (and audio in this case). The devices are thus media-focussed, although in a broader sense than drivers/ media/ > Essentially they are multi purpose buses - which do not yet have a home. > We have used media as a home because of our use case. > > The use case whether they transfer frames from a camera or to a display > are of course closely related, but ultimately covered by two separate > subsystems at the pixel level (DRM vs V4L, or other for other data) > > Perhaps as they are buses - on a level with USB or I2C (except they can > of course carry I2C or Serial as well as 'bi-directional video' etc ), > they are looking for their own subsystem. > > Except I don't think we don't want to add a new subsystem for just one > (or two) devices... I'm not sure a new subsystem is needed. As you've noted there's an overlap between drivers/media/ and drivers/gpu/drm/ in terms of supported hardware. We even have a devices supported by two drivers, one in drivers/media/ and one in drivers/gpu/drm/ (I'm thinking about the adv7511 in particular). This is a well known issue, and so far nothing has been done in mainline to try and solve it. Trying to find another home in drivers/mfd/ to escape from the problem isn't a good solution in my opinion. The best option from a Linux point of view would be to unify V4L2 and DRM/KMS when it comes to bridge support, but that's a long way down the road (I won't complain if you want to give it a go though :-)). As your use cases are display, focused, I would propose to start with drivers/gpu/drm/bridge/, and leave the problem of camera support for first person who will have such a use case. > >> certainly has different functions which are broken out in this > >> implementation. > > > > No, there is absolutely nothing broken out from the presented MFD drivers, > > the drivers are completely integral and basically I don't expect any. > > > > If you are concerned about media functionality, the correspondent MFD > > *cell* drivers will be added into drivers/media, drivers/gpu/drm or > > whatever is to be a proper place. > > > >> I think it might be quite awkward having the i2c components in > >> drivers/i2c and the media components in drivers/media/i2c, so what about > >> creating drivers/media/i2c/fpd-link (or such) as a container? > > > > I open drivers/media/i2c/Kconfig and all entries with no exception are > > under from 'if VIDEO_V4L2'. The MFD drivers do NOT require on depend on > > VIDEO_V4L2 or any other multimedia frameworks, nor the MFD drivers export > > any multimedia controls. > > > >> Our GMSL implementation is also a complex camera(s) device - but does > >> not yet use the MFD framework, perhaps that's something to add to my > >> todo list. Nope, please don't :-) The GMSL (de)serializers are no MFD devices either. > > Okay, but the TI DS90Ux9xx is NOT a camera device, and it is NOT a > > multimedia device, but it is a pure MFD device so the argument is not > > applicable. For the reasons pointed out in the review of other patches, and also pointed out by Lee in his review of this patch, I disagree with that. This isn't an MFD device. > >> We currently keep all of the complexity within the max9286.c driver, but > >> I could foresee that being split further if more devices add to the > >> complexity of managing the bus. At which point we might want an > >> equivalent drivers/media/i2c/gmsl/ perhaps? > >> > >>>> Laurent, can you please share your opinion? Done :-) I'd like to mention that I would prefer focusing on the DT bindings first, and then move to the driver side. In that area I would like to have a full example of a system using these chips, as the "initial support" is too limited for a proper review. This won't come as a surprise, but I will expect the OF graph bindings to be used to model data connections.
Hi Vladimir, On Friday, 12 October 2018 14:24:08 EEST Vladimir Zapolskiy wrote: > On 10/12/2018 11:39 AM, Lee Jones wrote: > > On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > >> On 10/12/2018 09:03 AM, Lee Jones wrote: > >>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > >>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>> > >>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, > >>>> support of subdevice controllers is done in separate drivers, because > >>>> not all IC functionality may be needed in particular situations, and > >>>> this can be fine grained controlled in device tree. > >>>> > >>>> The development of the driver was a collaborative work, the > >>>> contribution done by Balasubramani Vivekanandan includes: > >>>> * original implementation of the driver based on a reference driver, > >>>> * regmap powered interrupt controller support on serializers, > >>>> * support of implicitly or improperly specified in device tree ICs, > >>>> * support of device properties and attributes: backward compatible > >>>> > >>>> mode, low frequency operation mode, spread spectrum clock generator. > >>>> > >>>> Contribution by Steve Longerbeam: > >>>> * added ds90ux9xx_read_indirect() function, > >>>> * moved number of links property and added ds90ux9xx_num_fpd_links(), > >>>> * moved and updated ds90ux9xx_get_link_status() function to core > >>>> driver, > >>>> * added fpd_link_show device attribute. > >>>> > >>>> Sandeep Jain added support of pixel clock edge configuration. > >>>> > >>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>> --- > >>>> > >>>> drivers/mfd/Kconfig | 14 + > >>>> drivers/mfd/Makefile | 1 + > >>>> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ > >>>> include/linux/mfd/ds90ux9xx.h | 42 ++ > >>>> 4 files changed, 936 insertions(+) > >>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c > >>>> create mode 100644 include/linux/mfd/ds90ux9xx.h > >>>> > >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >>>> index 8c5dfdce4326..a969fa123f64 100644 > >>>> --- a/drivers/mfd/Kconfig > >>>> +++ b/drivers/mfd/Kconfig > >>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP > >>>> > >>>> boards. MSP430 firmware manages resets and power sequencing, > >>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. > >>>> > >>>> +config MFD_DS90UX9XX > >>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" > >>>> + depends on I2C && OF > >>>> + select MFD_CORE > >>>> + select REGMAP_I2C > >>>> + help > >>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. > >>>> + > >>>> + This driver provides basic support for setting up the > >>>> de-/serializer > >>>> + chips. Additional functionalities like connection handling to > >>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO > >>>> + controller and so on are provided by separate drivers and should > >>>> + enabled individually. > >>> > >>> This is not an MFD driver. > >> > >> Why do you think so? The representation of the ICs into device tree > >> format of hardware description shows that this is a truly MFD driver with > >> multiple IP subcomponents naturally mapped into MFD cells. > > > > 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'. > > What is the reason why device drivers for sort of multimedia ICs like > WL1273, WM8994 and other numerous codecs are found in drivers/mfd? I won't comment on those because I don't know them (Lee might have a better knowledge in this area), but let's not rule out historical mistakes, that we may not want to repeat. > If the same reason can not be applied to this DS90Ux9xx driver, why? > > > 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). > > > >> Basically it is possible to replace explicit of_platform_populate() by > >> adding a "simple-mfd" compatible, if it is desired. > >> > >>> After a 30 second Google of what this device actually does, perhaps > >>> drivers/media might be a better fit? > >> > >> I assume it would be quite unusual to add a driver with NO media > >> functions and controls into drivers/media. > > > > drivers/media may very well not be the correct place for this. In my > > 30 second Google, I saw that this device has a lot to do with cameras, > > hence my media association. > > Well, the argument is similar to the statement that Google says that > camera sensors *can* be connected to NXP i.MX6 SoC, thus arch/arm/mach-imx > contents should be placed into drivers/media If there are any camera drivers in arch/arm/mach-imx, they should certainly be moved. > A few TI DS90Ux9xx *cell* drivers may be added to drivers/media, but it is > out of the scope of the current series, which is completely integral per se, > and actually the cover letter says that the series of drivers immediately > allows to output video over DRM to panels, but the discussion is around > sensors by some reason. Possibly because a quick search for more information about the device returned camera use cases. Let's not focus on that. > But I hope it won't be seen as a misleading reason to consider to add the > MFD driver into drivers/gpu/drm Not a misleading one, a very good one :-) This should go in drivers/gpu/drm/ bridge/. > > If *all* else fails, there is always drivers/misc, but this should be > > avoided if at all possible. > > drivers/misc does not sound like a proper place for the MFD driver... drivers/misc/ isn't right either. > >> Laurent, can you please share your opinion?
Hi Vladimir, (CC'ing Wolfram) On Friday, 12 October 2018 10:32:32 EEST Vladimir Zapolskiy wrote: > On 10/12/2018 09:04 AM, Lee Jones wrote: > > On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > >> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >> > >> The change adds TI DS90Ux9xx I2C bridge/alias subdevice driver and > >> FPD Link connection handling mechanism. > >> > >> Access to I2C devices connected to a remote de-/serializer is done in > >> a transparent way, on established link detection event such devices > >> are registered on an I2C bus, which serves a local de-/serializer IC. > >> > >> The development of the driver was a collaborative work, the > >> contribution done by Balasubramani Vivekanandan includes: > >> * original simplistic implementation of the driver, > >> * support of implicitly specified devices in device tree, > >> * support of multiple FPD links for TI DS90Ux9xx, > >> * other kind of valuable review comments, clean-ups and fixes. > >> > >> Also Steve Longerbeam made the following changes: > >> * clear address maps after linked device removal, > >> * disable pass-through in disconnection, > >> * qualify locked status with non-zero remote address. > >> > >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >> --- > >> > >> drivers/mfd/Kconfig | 8 + > >> drivers/mfd/Makefile | 1 + > >> drivers/mfd/ds90ux9xx-i2c-bridge.c | 764 +++++++++++++++++++++++++++++ > >> 3 files changed, 773 insertions(+) > >> create mode 100644 drivers/mfd/ds90ux9xx-i2c-bridge.c > > > > Shouldn't this live in drivers/i2c? > > no, the driver is not for an I2C controller of any kind, and the driver does > not register itself in the I2C subsystem by calling i2c_add_adapter() or > i2c_add_numbered_adapter() or i2c_mux_add_adapter() etc, this topic was > discussed with Wolfram also. (Who is now on CC) > Formally the driver converts the managed IC into a multi-address I2C > slave device, I understand that it does not sound like a well suited driver > for MFD, but ds90ux9xx-core.c and ds90ux9xx-i2c-bridge.c drivers are quite > tightly coupled. As mentioned in other e-mails in this thread I don't think this should be split out to a separate driver, I would move the functionality to the ds90ux9xx driver. You may want to register an I2C mux, but as you have a single port, that could be overkill. I haven't studied in details how to best support this chip using the existing I2C subsystems APIs (which we may want to extend if it needed), but I believe that (in your use cases) the deserializer should be a child of the serializer, and modeled as an I2C device.
Hello Laurent. On 10/12/2018 04:01 PM, Laurent Pinchart wrote: > Hello Vladimir, > > On Friday, 12 October 2018 14:47:52 EEST Kieran Bingham wrote: >> On 12/10/18 11:58, Vladimir Zapolskiy wrote: >>> On 10/12/2018 12:20 PM, Kieran Bingham wrote: >>>> On 12/10/18 09:39, Lee Jones wrote: >>>>> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: >>>>>> On 10/12/2018 09:03 AM, Lee Jones wrote: >>>>>>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: >>>>>>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>>>>>> >>>>>>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, >>>>>>>> support of subdevice controllers is done in separate drivers, because >>>>>>>> not all IC functionality may be needed in particular situations, and >>>>>>>> this can be fine grained controlled in device tree. >>>>>>>> >>>>>>>> The development of the driver was a collaborative work, the >>>>>>>> contribution done by Balasubramani Vivekanandan includes: >>>>>>>> * original implementation of the driver based on a reference driver, >>>>>>>> * regmap powered interrupt controller support on serializers, >>>>>>>> * support of implicitly or improperly specified in device tree ICs, >>>>>>>> * support of device properties and attributes: backward compatible >>>>>>>> >>>>>>>> mode, low frequency operation mode, spread spectrum clock >>>>>>>> generator. >>>>>>>> >>>>>>>> Contribution by Steve Longerbeam: >>>>>>>> * added ds90ux9xx_read_indirect() function, >>>>>>>> * moved number of links property and added ds90ux9xx_num_fpd_links(), >>>>>>>> * moved and updated ds90ux9xx_get_link_status() function to core >>>>>>>> driver, >>>>>>>> * added fpd_link_show device attribute. >>>>>>>> >>>>>>>> Sandeep Jain added support of pixel clock edge configuration. >>>>>>>> >>>>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>>>>>> --- >>>>>>>> >>>>>>>> drivers/mfd/Kconfig | 14 + >>>>>>>> drivers/mfd/Makefile | 1 + >>>>>>>> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++ >>>>>>>> include/linux/mfd/ds90ux9xx.h | 42 ++ >>>>>>>> 4 files changed, 936 insertions(+) >>>>>>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c >>>>>>>> create mode 100644 include/linux/mfd/ds90ux9xx.h >>>>>>>> >>>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>>>>>> index 8c5dfdce4326..a969fa123f64 100644 >>>>>>>> --- a/drivers/mfd/Kconfig >>>>>>>> +++ b/drivers/mfd/Kconfig >>>>>>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP >>>>>>>> >>>>>>>> boards. MSP430 firmware manages resets and power sequencing, >>>>>>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. >>>>>>>> >>>>>>>> +config MFD_DS90UX9XX >>>>>>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" >>>>>>>> + depends on I2C && OF >>>>>>>> + select MFD_CORE >>>>>>>> + select REGMAP_I2C >>>>>>>> + help >>>>>>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer >>>>>>>> ICs. >>>>>>>> + >>>>>>>> + This driver provides basic support for setting up the >>>>>>>> de-/serializer >>>>>>>> + chips. Additional functionalities like connection handling to >>>>>>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO >>>>>>>> + controller and so on are provided by separate drivers and should >>>>>>>> + enabled individually. >>>>>>> >>>>>>> This is not an MFD driver. >>>>>> >>>>>> Why do you think so? The representation of the ICs into device tree >>>>>> format of hardware description shows that this is a truly MFD driver >>>>>> with multiple IP subcomponents naturally mapped into MFD cells. >>>>> >>>>> 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). >>>>> >>>>>> Basically it is possible to replace explicit of_platform_populate() by >>>>>> adding a "simple-mfd" compatible, if it is desired. >>>>>> >>>>>>> After a 30 second Google of what this device actually does, perhaps >>>>>>> drivers/media might be a better fit? >>>>>> >>>>>> I assume it would be quite unusual to add a driver with NO media >>>>>> functions and controls into drivers/media. >>>>> >>>>> drivers/media may very well not be the correct place for this. In my >>>>> 30 second Google, I saw that this device has a lot to do with cameras, >>>>> hence my media association. >>>>> >>>>> If *all* else fails, there is always drivers/misc, but this should be >>>>> avoided if at all possible. >>>> >>>> The device as a whole is FPD Link for camera devices I believe, but it >>> >>> I still don't understand (I could be biased though) why there is such >>> a strong emphasis on cameras and media stuff in the discussion. >>> >>> No, "the device as a whole is FPD Link for camera devices" is a wrong >>> statement. On hand I have a number of boards with >>> serializers/deserializers from the TI DS90Ux9xx IC series and sensors are >>> NOT connected to them. > > I understand what is not connected to them, but could you tell us what is > connected then ? :-) You got it right, the most two common ways of using the ICs: 1) SoC -> serializer ("local") -> deserializer ("remote") -> panel, 2) sensor -> serializer ("remote") -> deserializer ("local") -> SoC. The point is that the published drivers naturally support both data chains and even more of them, e.g. transferring audio data only or just setting GPIO line signals on a "remote" PCB. >> Yes - My apologies, this is a good point. >> >> Especially as the clue is in the name "Flat Panel Display". >> I have been stuck with my GMSL hat on for too long. >> >> Even GMSL is in the same boat. It's just that 'we' are using GMSL for >> cameras, but it can be used equally for displays and data. >> >> These devices are serialiser-deserialiser pairs with power and control >> paths. > > And data paths, that are designed to transport video (and audio in this case). > The devices are thus media-focussed, although in a broader sense than drivers/ > media/ The devices are media-focused only in the sense that media functionality casts a shadow onto inalienable GPIO or I2C bridging functionality. Anyway MFD driver representation of the ICs allows to keep pinmuxing, V4L2, DRM, audio bridging, interrupt controller and all other subcontroller functions separately, configure them separately, store under separate driver frameworks etc. That's a perfect flexibility on drivers level as I can see it. >> Essentially they are multi purpose buses - which do not yet have a home. >> We have used media as a home because of our use case. >> >> The use case whether they transfer frames from a camera or to a display >> are of course closely related, but ultimately covered by two separate >> subsystems at the pixel level (DRM vs V4L, or other for other data) >> >> Perhaps as they are buses - on a level with USB or I2C (except they can >> of course carry I2C or Serial as well as 'bi-directional video' etc ), >> they are looking for their own subsystem. >> >> Except I don't think we don't want to add a new subsystem for just one >> (or two) devices... > > I'm not sure a new subsystem is needed. As you've noted there's an overlap > between drivers/media/ and drivers/gpu/drm/ in terms of supported hardware. We > even have a devices supported by two drivers, one in drivers/media/ and one in > drivers/gpu/drm/ (I'm thinking about the adv7511 in particular). This is a > well known issue, and so far nothing has been done in mainline to try and > solve it. Right, presumably this IC series would have *cell* drivers in both drivers/media and drivers/gpu/drm/ and other folders like drivers/pinctrl/ or sound/. The interesting thing is that the published drivers do NOT require any new cell drivers (at least non-trivial ones with >200 LoC) in drivers/media/ or drivers/gpu/drm/ to get the expected operation of DS90Ux925/926/927/928 ICs (any ser-des connection combination), and we have a DS90Ux940 cell driver targeted to drivers/media/ > Trying to find another home in drivers/mfd/ to escape from the problem isn't a > good solution in my opinion. The best option from a Linux point of view would > be to unify V4L2 and DRM/KMS when it comes to bridge support, but that's a > long way down the road (I won't complain if you want to give it a go though > :-)). As your use cases are display, focused, I would propose to start with > drivers/gpu/drm/bridge/, and leave the problem of camera support for first > person who will have such a use case. Well, what should I do with pinctrl or audio bridging device drivers? Stick all drivers together into an unmaintainable clod of tangled code? Let's better exploit the excellent opportunity for code modularity given by the MFD framework. >>>> certainly has different functions which are broken out in this >>>> implementation. >>> >>> No, there is absolutely nothing broken out from the presented MFD drivers, >>> the drivers are completely integral and basically I don't expect any. >>> >>> If you are concerned about media functionality, the correspondent MFD >>> *cell* drivers will be added into drivers/media, drivers/gpu/drm or >>> whatever is to be a proper place. >>> >>>> I think it might be quite awkward having the i2c components in >>>> drivers/i2c and the media components in drivers/media/i2c, so what about >>>> creating drivers/media/i2c/fpd-link (or such) as a container? >>> >>> I open drivers/media/i2c/Kconfig and all entries with no exception are >>> under from 'if VIDEO_V4L2'. The MFD drivers do NOT require on depend on >>> VIDEO_V4L2 or any other multimedia frameworks, nor the MFD drivers export >>> any multimedia controls. >>> >>>> Our GMSL implementation is also a complex camera(s) device - but does >>>> not yet use the MFD framework, perhaps that's something to add to my >>>> todo list. > > Nope, please don't :-) The GMSL (de)serializers are no MFD devices either. > >>> Okay, but the TI DS90Ux9xx is NOT a camera device, and it is NOT a >>> multimedia device, but it is a pure MFD device so the argument is not >>> applicable. > > For the reasons pointed out in the review of other patches, and also pointed > out by Lee in his review of this patch, I disagree with that. This isn't an > MFD device. Eh, it is an MFD device. Just probably drivers/mfd is really not the best place to store this particular MFD device driver... >>>> We currently keep all of the complexity within the max9286.c driver, but >>>> I could foresee that being split further if more devices add to the >>>> complexity of managing the bus. At which point we might want an >>>> equivalent drivers/media/i2c/gmsl/ perhaps? >>>> >>>>>> Laurent, can you please share your opinion? > > Done :-) > > I'd like to mention that I would prefer focusing on the DT bindings first, and Sure, thank you for your comments :) > then move to the driver side. In that area I would like to have a full example > of a system using these chips, as the "initial support" is too limited for a > proper review. This won't come as a surprise, but I will expect the OF graph > bindings to be used to model data connections. > The leverage of "the initial support" to "the complete support" requires: * audio bridge cell driver -- trivial, just one mute/unmute control, * interrupt controller cell driver -- trivial, but for sake of perfection it requires some minimal changes in drivers/base/regmap/regmap-irq.c * DS90Ux940 MIPI CSI-2 -- non-trivial one, but we have it ready, I just don't want to add it to the pile at the moment, otherwise we'll continue discussing cameras, and I'd like to postpone it :) No more than that is needed to get absolutely complete support of 5 claimed DS90UB9xx ICs, really. 5 other DS90UH9xx will require HDCP support in addition. I'll try to roll out an example of DTS snippet soon. -- Best wishes, Vladimir
Hi Lee, On 10/12/2018 02:34 PM, Lee Jones wrote: > On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: >> On 10/12/2018 12:20 PM, Kieran Bingham wrote: >>> Hi Vladimir, >>> On 12/10/18 09:39, Lee Jones wrote: >>>> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: >>>>> On 10/12/2018 09:03 AM, Lee Jones wrote: >>>>>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: >>>>>> >>>>>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>>>>> >>>>>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, >>>>>>> support of subdevice controllers is done in separate drivers, because >>>>>>> not all IC functionality may be needed in particular situations, and >>>>>>> this can be fine grained controlled in device tree. >>>>>>> >>>>>>> The development of the driver was a collaborative work, the >>>>>>> contribution done by Balasubramani Vivekanandan includes: >>>>>>> * original implementation of the driver based on a reference driver, >>>>>>> * regmap powered interrupt controller support on serializers, >>>>>>> * support of implicitly or improperly specified in device tree ICs, >>>>>>> * support of device properties and attributes: backward compatible >>>>>>> mode, low frequency operation mode, spread spectrum clock generator. >>>>>>> >>>>>>> Contribution by Steve Longerbeam: >>>>>>> * added ds90ux9xx_read_indirect() function, >>>>>>> * moved number of links property and added ds90ux9xx_num_fpd_links(), >>>>>>> * moved and updated ds90ux9xx_get_link_status() function to core driver, >>>>>>> * added fpd_link_show device attribute. >>>>>>> >>>>>>> Sandeep Jain added support of pixel clock edge configuration. >>>>>>> >>>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>>>>> --- >>>>>>> drivers/mfd/Kconfig | 14 + >>>>>>> drivers/mfd/Makefile | 1 + >>>>>>> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ >>>>>>> include/linux/mfd/ds90ux9xx.h | 42 ++ >>>>>>> 4 files changed, 936 insertions(+) >>>>>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c >>>>>>> create mode 100644 include/linux/mfd/ds90ux9xx.h >>>>>>> >>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>>>>> index 8c5dfdce4326..a969fa123f64 100644 >>>>>>> --- a/drivers/mfd/Kconfig >>>>>>> +++ b/drivers/mfd/Kconfig >>>>>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP >>>>>>> boards. MSP430 firmware manages resets and power sequencing, >>>>>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. >>>>>>> >>>>>>> +config MFD_DS90UX9XX >>>>>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" >>>>>>> + depends on I2C && OF >>>>>>> + select MFD_CORE >>>>>>> + select REGMAP_I2C >>>>>>> + help >>>>>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. >>>>>>> + >>>>>>> + This driver provides basic support for setting up the de-/serializer >>>>>>> + chips. Additional functionalities like connection handling to >>>>>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO >>>>>>> + controller and so on are provided by separate drivers and should >>>>>>> + enabled individually. >>>>>> >>>>>> This is not an MFD driver. >>>>> >>>>> Why do you think so? The representation of the ICs into device tree format >>>>> of hardware description shows that this is a truly MFD driver with multiple >>>>> IP subcomponents naturally mapped into MFD cells. >>>> >>>> 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). >>>> >>>>> Basically it is possible to replace explicit of_platform_populate() by >>>>> adding a "simple-mfd" compatible, if it is desired. >>>>> >>>>>> After a 30 second Google of what this device actually does, perhaps >>>>>> drivers/media might be a better fit? >>>>> >>>>> I assume it would be quite unusual to add a driver with NO media functions >>>>> and controls into drivers/media. >>>> >>>> drivers/media may very well not be the correct place for this. In my >>>> 30 second Google, I saw that this device has a lot to do with cameras, >>>> hence my media association. >>>> >>>> If *all* else fails, there is always drivers/misc, but this should be >>>> avoided if at all possible. >>> >>> The device as a whole is FPD Link for camera devices I believe, but it >> >> I still don't understand (I could be biased though) why there is such >> a strong emphasis on cameras and media stuff in the discussion. >> >> No, "the device as a whole is FPD Link for camera devices" is a wrong >> statement. On hand I have a number of boards with serializers/deserializers >> from the TI DS90Ux9xx IC series and sensors are NOT connected to them. >> >>> certainly has different functions which are broken out in this >>> implementation. >> >> No, there is absolutely nothing broken out from the presented MFD drivers, >> the drivers are completely integral and basically I don't expect any. >> >> If you are concerned about media functionality, the correspondent MFD >> *cell* drivers will be added into drivers/media, drivers/gpu/drm or >> whatever is to be a proper place. >> >>> I think it might be quite awkward having the i2c components in >>> drivers/i2c and the media components in drivers/media/i2c, so what about >>> creating drivers/media/i2c/fpd-link (or such) as a container? >> >> I open drivers/media/i2c/Kconfig and all entries with no exception are >> under from 'if VIDEO_V4L2'. The MFD drivers do NOT require on depend on >> VIDEO_V4L2 or any other multimedia frameworks, nor the MFD drivers export >> any multimedia controls. >> >>> Our GMSL implementation is also a complex camera(s) device - but does >>> not yet use the MFD framework, perhaps that's something to add to my >>> todo list. >>> >> >> Okay, but the TI DS90Ux9xx is NOT a camera device, and it is NOT a multimedia >> device, but it is a pure MFD device so the argument is not applicable. > > You keep saying that "this is an MFD device" without any obvious > comprehension of what an MFD is. Just saying that it is one > over-and-over does not make it so. > An MFD should be little more than parent to other functional devices. > Their role is to register children which in turn conduct operations > on the hardware in a useful way. Some MFDs also house common functions > to save repetition of code in each of the child devices. They do not > tend to offer any useful functionality (stuff) in their own right. This describes the presented MFD driver quite closely, if I remove a few OF controls from ds90ux9xx-core.c: * ti,video-map-select-*, * ti,pixel-clock-edge, * ti,spread-spectrum-clock-generation Then the MFD device driver will not have any useful functionality apart of what you've listed above, please feel free to recheck. Should I just go ahead and do the change with the assumption that the modified MFD driver suits MFD framework? > As I already mentioned, you need to figure out what this device is > and move all of the functionality into the appropriate subsystem. By definition as I comprehend it only MFD cell device drivers should be relocated into the correspondent subsystems, but ds90ux9xx-core remains in drivers/mfd, no? Probably ds90ux9xx-i2c-bridge cell driver could enter drivers/misc. > Since an MFD isn't a real thing/device (it's a Linuxy-shim which > only serves to register sub-devices and (sometimes) provide a space > for common functionality to be located), drivers/mfd is not the > subsystem which you seek. Oh, that's exactly the case with DS90Ux9xx driver 'ds90ux9xx-core.c', it's just a common place to store the shared boilerplate code snippets for all cell device drivers and various flavours of ICs from the series. >>> We currently keep all of the complexity within the max9286.c driver, but >>> I could foresee that being split further if more devices add to the >>> complexity of managing the bus. At which point we might want an >>> equivalent drivers/media/i2c/gmsl/ perhaps? > -- Best wishes, Vladimir
On 10/12/2018 02:43 PM, Lee Jones wrote: > On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > >> On 10/12/2018 11:39 AM, Lee Jones wrote: >>> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: >>>> On 10/12/2018 09:03 AM, Lee Jones wrote: >>>>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: >>>>> >>>>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>>>> >>>>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, >>>>>> support of subdevice controllers is done in separate drivers, because >>>>>> not all IC functionality may be needed in particular situations, and >>>>>> this can be fine grained controlled in device tree. >>>>>> >>>>>> The development of the driver was a collaborative work, the >>>>>> contribution done by Balasubramani Vivekanandan includes: >>>>>> * original implementation of the driver based on a reference driver, >>>>>> * regmap powered interrupt controller support on serializers, >>>>>> * support of implicitly or improperly specified in device tree ICs, >>>>>> * support of device properties and attributes: backward compatible >>>>>> mode, low frequency operation mode, spread spectrum clock generator. >>>>>> >>>>>> Contribution by Steve Longerbeam: >>>>>> * added ds90ux9xx_read_indirect() function, >>>>>> * moved number of links property and added ds90ux9xx_num_fpd_links(), >>>>>> * moved and updated ds90ux9xx_get_link_status() function to core driver, >>>>>> * added fpd_link_show device attribute. >>>>>> >>>>>> Sandeep Jain added support of pixel clock edge configuration. >>>>>> >>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>>>> --- >>>>>> drivers/mfd/Kconfig | 14 + >>>>>> drivers/mfd/Makefile | 1 + >>>>>> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ >>>>>> include/linux/mfd/ds90ux9xx.h | 42 ++ >>>>>> 4 files changed, 936 insertions(+) >>>>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c >>>>>> create mode 100644 include/linux/mfd/ds90ux9xx.h >>>>>> >>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>>>> index 8c5dfdce4326..a969fa123f64 100644 >>>>>> --- a/drivers/mfd/Kconfig >>>>>> +++ b/drivers/mfd/Kconfig >>>>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP >>>>>> boards. MSP430 firmware manages resets and power sequencing, >>>>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. >>>>>> >>>>>> +config MFD_DS90UX9XX >>>>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" >>>>>> + depends on I2C && OF >>>>>> + select MFD_CORE >>>>>> + select REGMAP_I2C >>>>>> + help >>>>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. >>>>>> + >>>>>> + This driver provides basic support for setting up the de-/serializer >>>>>> + chips. Additional functionalities like connection handling to >>>>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO >>>>>> + controller and so on are provided by separate drivers and should >>>>>> + enabled individually. >>>>> >>>>> This is not an MFD driver. >>>> >>>> Why do you think so? The representation of the ICs into device tree format >>>> of hardware description shows that this is a truly MFD driver with multiple >>>> IP subcomponents naturally mapped into MFD cells. >>> >>> 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'. >>> >> >> What is the reason why device drivers for sort of multimedia ICs like >> WL1273, WM8994 and other numerous codecs are found in drivers/mfd? >> >> If the same reason can not be applied to this DS90Ux9xx driver, why? >> >>> 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). >>> >>>> Basically it is possible to replace explicit of_platform_populate() by >>>> adding a "simple-mfd" compatible, if it is desired. >>>> >>>>> After a 30 second Google of what this device actually does, perhaps >>>>> drivers/media might be a better fit? >>>> >>>> I assume it would be quite unusual to add a driver with NO media functions >>>> and controls into drivers/media. >>> >>> drivers/media may very well not be the correct place for this. In my >>> 30 second Google, I saw that this device has a lot to do with cameras, >>> hence my media association. >> >> Well, the argument is similar to the statement that Google says that >> camera sensors *can* be connected to NXP i.MX6 SoC, thus arch/arm/mach-imx >> contents should be placed into drivers/media >> >> A few TI DS90Ux9xx *cell* drivers may be added to drivers/media, but it is >> out of the scope of the current series, which is completely integral per se, >> and actually the cover letter says that the series of drivers immediately >> allows to output video over DRM to panels, but the discussion is around >> sensors by some reason. But I hope it won't be seen as a misleading >> reason to consider to add the MFD driver into drivers/gpu/drm > > This discussion isn't about not adding enough child devices. It's > about there being too much functional work being done in an MFD > driver, where it doesn't belong. Please can you elaborate what is "too much functional work" here more precisely? >>> If *all* else fails, there is always drivers/misc, but this should be >>> avoided if at all possible. >> >> drivers/misc does not sound like a proper place for the MFD driver... > > I'd agree with you if this were an MFD driver. > > As I mentioned before, there may well be an argument for and MFD > driver to be part of this driver-set. However it needs to be > significantly reduced with any functional code removed and placed > where it belongs. > I can name just settings of a few bitfields from OF and sysfs to be moved to another location (media or DRM, Laurent?), and some of them like "backward compatible mode" (used to connect ICs of different generations) setting should remain in the core driver. Clearly I'd like to know what exactly should be changed in the ds90ux9xx-core.c code to get it accepted as an MFD device driver, probably you can comment on the code about anything to remove/relocate? -- Best wishes, Vladimir
On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > Hi Lee, > > On 10/12/2018 02:34 PM, Lee Jones wrote: > > On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > >> On 10/12/2018 12:20 PM, Kieran Bingham wrote: > >>> Hi Vladimir, > >>> On 12/10/18 09:39, Lee Jones wrote: > >>>> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > >>>>> On 10/12/2018 09:03 AM, Lee Jones wrote: > >>>>>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > >>>>>> > >>>>>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>>>>> > >>>>>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, > >>>>>>> support of subdevice controllers is done in separate drivers, because > >>>>>>> not all IC functionality may be needed in particular situations, and > >>>>>>> this can be fine grained controlled in device tree. > >>>>>>> > >>>>>>> The development of the driver was a collaborative work, the > >>>>>>> contribution done by Balasubramani Vivekanandan includes: > >>>>>>> * original implementation of the driver based on a reference driver, > >>>>>>> * regmap powered interrupt controller support on serializers, > >>>>>>> * support of implicitly or improperly specified in device tree ICs, > >>>>>>> * support of device properties and attributes: backward compatible > >>>>>>> mode, low frequency operation mode, spread spectrum clock generator. > >>>>>>> > >>>>>>> Contribution by Steve Longerbeam: > >>>>>>> * added ds90ux9xx_read_indirect() function, > >>>>>>> * moved number of links property and added ds90ux9xx_num_fpd_links(), > >>>>>>> * moved and updated ds90ux9xx_get_link_status() function to core driver, > >>>>>>> * added fpd_link_show device attribute. > >>>>>>> > >>>>>>> Sandeep Jain added support of pixel clock edge configuration. > >>>>>>> > >>>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>>>>> --- > >>>>>>> drivers/mfd/Kconfig | 14 + > >>>>>>> drivers/mfd/Makefile | 1 + > >>>>>>> drivers/mfd/ds90ux9xx-core.c | 879 ++++++++++++++++++++++++++++++++++ > >>>>>>> include/linux/mfd/ds90ux9xx.h | 42 ++ > >>>>>>> 4 files changed, 936 insertions(+) > >>>>>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c > >>>>>>> create mode 100644 include/linux/mfd/ds90ux9xx.h > >>>>>>> > >>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >>>>>>> index 8c5dfdce4326..a969fa123f64 100644 > >>>>>>> --- a/drivers/mfd/Kconfig > >>>>>>> +++ b/drivers/mfd/Kconfig > >>>>>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP > >>>>>>> boards. MSP430 firmware manages resets and power sequencing, > >>>>>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. > >>>>>>> > >>>>>>> +config MFD_DS90UX9XX > >>>>>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" > >>>>>>> + depends on I2C && OF > >>>>>>> + select MFD_CORE > >>>>>>> + select REGMAP_I2C > >>>>>>> + help > >>>>>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer ICs. > >>>>>>> + > >>>>>>> + This driver provides basic support for setting up the de-/serializer > >>>>>>> + chips. Additional functionalities like connection handling to > >>>>>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO > >>>>>>> + controller and so on are provided by separate drivers and should > >>>>>>> + enabled individually. > >>>>>> > >>>>>> This is not an MFD driver. > >>>>> > >>>>> Why do you think so? The representation of the ICs into device tree format > >>>>> of hardware description shows that this is a truly MFD driver with multiple > >>>>> IP subcomponents naturally mapped into MFD cells. > >>>> > >>>> 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). > >>>> > >>>>> Basically it is possible to replace explicit of_platform_populate() by > >>>>> adding a "simple-mfd" compatible, if it is desired. > >>>>> > >>>>>> After a 30 second Google of what this device actually does, perhaps > >>>>>> drivers/media might be a better fit? > >>>>> > >>>>> I assume it would be quite unusual to add a driver with NO media functions > >>>>> and controls into drivers/media. > >>>> > >>>> drivers/media may very well not be the correct place for this. In my > >>>> 30 second Google, I saw that this device has a lot to do with cameras, > >>>> hence my media association. > >>>> > >>>> If *all* else fails, there is always drivers/misc, but this should be > >>>> avoided if at all possible. > >>> > >>> The device as a whole is FPD Link for camera devices I believe, but it > >> > >> I still don't understand (I could be biased though) why there is such > >> a strong emphasis on cameras and media stuff in the discussion. > >> > >> No, "the device as a whole is FPD Link for camera devices" is a wrong > >> statement. On hand I have a number of boards with serializers/deserializers > >> from the TI DS90Ux9xx IC series and sensors are NOT connected to them. > >> > >>> certainly has different functions which are broken out in this > >>> implementation. > >> > >> No, there is absolutely nothing broken out from the presented MFD drivers, > >> the drivers are completely integral and basically I don't expect any. > >> > >> If you are concerned about media functionality, the correspondent MFD > >> *cell* drivers will be added into drivers/media, drivers/gpu/drm or > >> whatever is to be a proper place. > >> > >>> I think it might be quite awkward having the i2c components in > >>> drivers/i2c and the media components in drivers/media/i2c, so what about > >>> creating drivers/media/i2c/fpd-link (or such) as a container? > >> > >> I open drivers/media/i2c/Kconfig and all entries with no exception are > >> under from 'if VIDEO_V4L2'. The MFD drivers do NOT require on depend on > >> VIDEO_V4L2 or any other multimedia frameworks, nor the MFD drivers export > >> any multimedia controls. > >> > >>> Our GMSL implementation is also a complex camera(s) device - but does > >>> not yet use the MFD framework, perhaps that's something to add to my > >>> todo list. > >>> > >> > >> Okay, but the TI DS90Ux9xx is NOT a camera device, and it is NOT a multimedia > >> device, but it is a pure MFD device so the argument is not applicable. > > > > You keep saying that "this is an MFD device" without any obvious > > comprehension of what an MFD is. Just saying that it is one > > over-and-over does not make it so. > > An MFD should be little more than parent to other functional devices. > > Their role is to register children which in turn conduct operations > > on the hardware in a useful way. Some MFDs also house common functions > > to save repetition of code in each of the child devices. They do not > > tend to offer any useful functionality (stuff) in their own right. > > This describes the presented MFD driver quite closely, if I remove > a few OF controls from ds90ux9xx-core.c: > * ti,video-map-select-*, > * ti,pixel-clock-edge, > * ti,spread-spectrum-clock-generation > > Then the MFD device driver will not have any useful functionality > apart of what you've listed above, please feel free to recheck. > > Should I just go ahead and do the change with the assumption that > the modified MFD driver suits MFD framework? Since this device seems to be truly multi-function, I have no qualms with it being represented by a parent MFD driver. Providing any useful functionality (code that actually does stuff) is removed and placed in a more suitable location, it sounds like a reasonably good fit for MFD. > > As I already mentioned, you need to figure out what this device is > > and move all of the functionality into the appropriate subsystem. > > By definition as I comprehend it only MFD cell device drivers should > be relocated into the correspondent subsystems, but ds90ux9xx-core > remains in drivers/mfd, no? Sounds about right. > Probably ds90ux9xx-i2c-bridge cell driver could enter drivers/misc. Let's see what Wolfram has to say WRT Laurent's suggestion. > > Since an MFD isn't a real thing/device (it's a Linuxy-shim which > > only serves to register sub-devices and (sometimes) provide a space > > for common functionality to be located), drivers/mfd is not the > > subsystem which you seek. > > Oh, that's exactly the case with DS90Ux9xx driver 'ds90ux9xx-core.c', > it's just a common place to store the shared boilerplate code > snippets for all cell device drivers and various flavours of ICs > from the series. What you're using it for now is out-of-scope of an MFD driver. Which if the functions are called from more than 1 call-site? Those have a chance of residing - we'll discuss those on an ad-hoc basis. Anything else needs relocating to the relevant subsystem. We should also speak to Mark Brown about the indirect Regmap stuff. Perhaps this would better suit a header file. > >>> We currently keep all of the complexity within the max9286.c driver, but > >>> I could foresee that being split further if more devices add to the > >>> complexity of managing the bus. At which point we might want an > >>> equivalent drivers/media/i2c/gmsl/ perhaps?
Hi Laurent On 10/12/2018 04:12 PM, Laurent Pinchart wrote: > Hi Vladimir, > > (CC'ing Wolfram) > > On Friday, 12 October 2018 10:32:32 EEST Vladimir Zapolskiy wrote: >> On 10/12/2018 09:04 AM, Lee Jones wrote: >>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: >>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>> >>>> The change adds TI DS90Ux9xx I2C bridge/alias subdevice driver and >>>> FPD Link connection handling mechanism. >>>> >>>> Access to I2C devices connected to a remote de-/serializer is done in >>>> a transparent way, on established link detection event such devices >>>> are registered on an I2C bus, which serves a local de-/serializer IC. >>>> >>>> The development of the driver was a collaborative work, the >>>> contribution done by Balasubramani Vivekanandan includes: >>>> * original simplistic implementation of the driver, >>>> * support of implicitly specified devices in device tree, >>>> * support of multiple FPD links for TI DS90Ux9xx, >>>> * other kind of valuable review comments, clean-ups and fixes. >>>> >>>> Also Steve Longerbeam made the following changes: >>>> * clear address maps after linked device removal, >>>> * disable pass-through in disconnection, >>>> * qualify locked status with non-zero remote address. >>>> >>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>>> --- >>>> >>>> drivers/mfd/Kconfig | 8 + >>>> drivers/mfd/Makefile | 1 + >>>> drivers/mfd/ds90ux9xx-i2c-bridge.c | 764 +++++++++++++++++++++++++++++ >>>> 3 files changed, 773 insertions(+) >>>> create mode 100644 drivers/mfd/ds90ux9xx-i2c-bridge.c >>> >>> Shouldn't this live in drivers/i2c? >> >> no, the driver is not for an I2C controller of any kind, and the driver does >> not register itself in the I2C subsystem by calling i2c_add_adapter() or >> i2c_add_numbered_adapter() or i2c_mux_add_adapter() etc, this topic was >> discussed with Wolfram also. > > (Who is now on CC) > Wolfram has copies of the drivers and discussion right from the beginning, hopefully he won't get two copies ;) >> Formally the driver converts the managed IC into a multi-address I2C >> slave device, I understand that it does not sound like a well suited driver >> for MFD, but ds90ux9xx-core.c and ds90ux9xx-i2c-bridge.c drivers are quite >> tightly coupled. > > As mentioned in other e-mails in this thread I don't think this should be > split out to a separate driver,> I would move the functionality to the > ds90ux9xx driver. The proposal may have the grounds, but the I2C bridging functionality of ICs is quite detached from all other ones, thus it found its place in the cell driver per se. > You may want to register an I2C mux, but as you have a single port, that > could be overkill. I haven't studied in details how to best support this > chip using the existing I2C subsystems APIs (which we may want to extend > if it needed), but I believe that (in your use cases) the deserializer > should be a child of the serializer, and modeled as an I2C device. > Formally in OF terms to define a link between devices by a phandle should be sufficient, panels are not the children of LVDS controllers under OF graph constraints in DT representation, the panels become secondary in runtime only, I'd like to reuse the concept. Also it adds a better sense of symmetry of deserializer <-> serializer connections relatively to a SoC/data source. -- Best wishes, Vladimir
Hi Kieran, On 10/12/2018 02:47 PM, Kieran Bingham wrote: > Hi Vladimir, > [snip] > > Essentially they are multi purpose buses - which do not yet have a home. > We have used media as a home because of our use case. > > The use case whether they transfer frames from a camera or to a display > are of course closely related, but ultimately covered by two separate > subsystems at the pixel level (DRM vs V4L, or other for other data) > > Perhaps as they are buses - on a level with USB or I2C (except they can > of course carry I2C or Serial as well as 'bi-directional video' etc ), > they are looking for their own subsystem. > > Except I don't think we don't want to add a new subsystem for just one > (or two) devices... > I suppose that the incomplete list includes Maxim GMSL, TI FPD-Link III, SMSC/Microchip MOST (drivers/staging/most -- what's the destination after exiting staging?) an Inova APIX. -- Best wishes, Vladimir
Hi Laurent, On 10/12/2018 04:01 PM, Laurent Pinchart wrote: > Hello Vladimir, > > On Friday, 12 October 2018 14:47:52 EEST Kieran Bingham wrote: >> On 12/10/18 11:58, Vladimir Zapolskiy wrote: [snip] >> Essentially they are multi purpose buses - which do not yet have a home. >> We have used media as a home because of our use case. >> >> The use case whether they transfer frames from a camera or to a display >> are of course closely related, but ultimately covered by two separate >> subsystems at the pixel level (DRM vs V4L, or other for other data) >> >> Perhaps as they are buses - on a level with USB or I2C (except they can >> of course carry I2C or Serial as well as 'bi-directional video' etc ), >> they are looking for their own subsystem. >> >> Except I don't think we don't want to add a new subsystem for just one >> (or two) devices... > > I'm not sure a new subsystem is needed. As you've noted there's an overlap > between drivers/media/ and drivers/gpu/drm/ in terms of supported hardware. > We even have a devices supported by two drivers, one in drivers/media/ and > one in drivers/gpu/drm/ (I'm thinking about the adv7511 in particular). > This is a well known issue, and so far nothing has been done in mainline > to try and solve it. I agree that there's an overlap between drivers/media/ and drivers/gpu/drm/, formally a hypothetical (sic!) DS90Ux9xx video bridge cell driver should be added into both subsystems also, and the actual driver of two should be selected in runtime. I call such a driver 'hypothetical', because in fact I don't have it, and I'm not so sure that its existence is justified, but that's only because DS90Ux9xx video bridge functionality is _transparent_, it does not have any controls literally, but it is a pure luck eventually. So, as I've stated in my cover letter, I can misuse yours 'lvds-encoder' driver only for the purpose of establishing a mediated link between an LVDS controller and a panel over a serializer-deserializer pair. > Trying to find another home in drivers/mfd/ to escape from the problem isn't a > good solution in my opinion. The best option from a Linux point of view would > be to unify V4L2 and DRM/KMS when it comes to bridge support, but that's a > long way down the road (I won't complain if you want to give it a go though > :-)). I return you a wider smile :) > As your use cases are display, focused, I would propose to start with > drivers/gpu/drm/bridge/, and leave the problem of camera support for first > person who will have such a use case. Frankly speaking I would like to start from copy-pasting your 'lvds-encoder' driver into an 'absolutely-transparent-video-bridge' driver with no LVDS or 'encoder' specifics, adding just a new compatible may suffice, if the driver is renamed/redefined. PS, I remember I owe you a reference OF snippet of data path to a panel. -- Best wishes, Vladimir
Hi Vladimir, On Friday, 12 October 2018 16:59:27 EEST Vladimir Zapolskiy wrote: > On 10/12/2018 04:01 PM, Laurent Pinchart wrote: > > On Friday, 12 October 2018 14:47:52 EEST Kieran Bingham wrote: > >> On 12/10/18 11:58, Vladimir Zapolskiy wrote: > >>> On 10/12/2018 12:20 PM, Kieran Bingham wrote: > >>>> On 12/10/18 09:39, Lee Jones wrote: > >>>>> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote: > >>>>>> On 10/12/2018 09:03 AM, Lee Jones wrote: > >>>>>>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > >>>>>>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>>>>>> > >>>>>>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers, > >>>>>>>> support of subdevice controllers is done in separate drivers, > >>>>>>>> because not all IC functionality may be needed in particular > >>>>>>>> situations, and this can be fine grained controlled in device tree. > >>>>>>>> > >>>>>>>> The development of the driver was a collaborative work, the > >>>>>>>> contribution done by Balasubramani Vivekanandan includes: > >>>>>>>> * original implementation of the driver based on a reference > >>>>>>>> driver, > >>>>>>>> * regmap powered interrupt controller support on serializers, > >>>>>>>> * support of implicitly or improperly specified in device tree ICs, > >>>>>>>> * support of device properties and attributes: backward compatible > >>>>>>>> mode, low frequency operation mode, spread spectrum clock > >>>>>>>> generator. > >>>>>>>> > >>>>>>>> Contribution by Steve Longerbeam: > >>>>>>>> * added ds90ux9xx_read_indirect() function, > >>>>>>>> * moved number of links property and added > >>>>>>>> ds90ux9xx_num_fpd_links(), > >>>>>>>> * moved and updated ds90ux9xx_get_link_status() function to core > >>>>>>>> driver, > >>>>>>>> * added fpd_link_show device attribute. > >>>>>>>> > >>>>>>>> Sandeep Jain added support of pixel clock edge configuration. > >>>>>>>> > >>>>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>>>>>> --- > >>>>>>>> > >>>>>>>> drivers/mfd/Kconfig | 14 + > >>>>>>>> drivers/mfd/Makefile | 1 + > >>>>>>>> drivers/mfd/ds90ux9xx-core.c | 879 > >>>>>>>> ++++++++++++++++++++++++++++++++ > >>>>>>>> include/linux/mfd/ds90ux9xx.h | 42 ++ > >>>>>>>> 4 files changed, 936 insertions(+) > >>>>>>>> create mode 100644 drivers/mfd/ds90ux9xx-core.c > >>>>>>>> create mode 100644 include/linux/mfd/ds90ux9xx.h > >>>>>>>> > >>>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >>>>>>>> index 8c5dfdce4326..a969fa123f64 100644 > >>>>>>>> --- a/drivers/mfd/Kconfig > >>>>>>>> +++ b/drivers/mfd/Kconfig > >>>>>>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP > >>>>>>>> > >>>>>>>> boards. MSP430 firmware manages resets and power sequencing, > >>>>>>>> inputs from buttons and the IR remote, LEDs, an RTC, and more. > >>>>>>>> > >>>>>>>> +config MFD_DS90UX9XX > >>>>>>>> + tristate "TI DS90Ux9xx FPD-Link de-/serializer driver" > >>>>>>>> + depends on I2C && OF > >>>>>>>> + select MFD_CORE > >>>>>>>> + select REGMAP_I2C > >>>>>>>> + help > >>>>>>>> + Say yes here to enable support for TI DS90UX9XX de-/serializer > >>>>>>>> ICs. > >>>>>>>> + > >>>>>>>> + This driver provides basic support for setting up the > >>>>>>>> de-/serializer > >>>>>>>> + chips. Additional functionalities like connection handling to > >>>>>>>> + remote de-/serializers, I2C bridging, pin multiplexing, GPIO > >>>>>>>> + controller and so on are provided by separate drivers and > >>>>>>>> should > >>>>>>>> + enabled individually. > >>>>>>> > >>>>>>> This is not an MFD driver. > >>>>>> > >>>>>> Why do you think so? The representation of the ICs into device tree > >>>>>> format of hardware description shows that this is a truly MFD driver > >>>>>> with multiple IP subcomponents naturally mapped into MFD cells. > >>>>> > >>>>> 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). > >>>>> > >>>>>> Basically it is possible to replace explicit of_platform_populate() > >>>>>> by adding a "simple-mfd" compatible, if it is desired. > >>>>>> > >>>>>>> After a 30 second Google of what this device actually does, perhaps > >>>>>>> drivers/media might be a better fit? > >>>>>> > >>>>>> I assume it would be quite unusual to add a driver with NO media > >>>>>> functions and controls into drivers/media. > >>>>> > >>>>> drivers/media may very well not be the correct place for this. In my > >>>>> 30 second Google, I saw that this device has a lot to do with cameras, > >>>>> hence my media association. > >>>>> > >>>>> If *all* else fails, there is always drivers/misc, but this should be > >>>>> avoided if at all possible. > >>>> > >>>> The device as a whole is FPD Link for camera devices I believe, but it > >>> > >>> I still don't understand (I could be biased though) why there is such > >>> a strong emphasis on cameras and media stuff in the discussion. > >>> > >>> No, "the device as a whole is FPD Link for camera devices" is a wrong > >>> statement. On hand I have a number of boards with > >>> serializers/deserializers from the TI DS90Ux9xx IC series and sensors > >>> are NOT connected to them. > > > > I understand what is not connected to them, but could you tell us what is > > connected then ? :-) > > You got it right, the most two common ways of using the ICs: > > 1) SoC -> serializer ("local") -> deserializer ("remote") -> panel, > 2) sensor -> serializer ("remote") -> deserializer ("local") -> SoC. > > The point is that the published drivers naturally support both data chains > and even more of them, e.g. transferring audio data only or just setting > GPIO line signals on a "remote" PCB. At the cost of code being added where it doesn't belong (as pointed out by Lee) and DT bindings not following the standard models (data connections described by OF graph and control buses described by parent-child relationships). Let's focus on fixing the DT bindings first, and we'll then see how we can handle the driver side. > >> Yes - My apologies, this is a good point. > >> > >> Especially as the clue is in the name "Flat Panel Display". > >> I have been stuck with my GMSL hat on for too long. > >> > >> Even GMSL is in the same boat. It's just that 'we' are using GMSL for > >> cameras, but it can be used equally for displays and data. > >> > >> These devices are serialiser-deserialiser pairs with power and control > >> paths. > > > > And data paths, that are designed to transport video (and audio in this > > case). The devices are thus media-focussed, although in a broader sense > > than drivers/ media/ > > The devices are media-focused only in the sense that media functionality > casts a shadow onto inalienable GPIO or I2C bridging functionality. Seriously, do you really expect that they will be used for the sole purpose of transporting a few GPIOs ? > Anyway MFD driver representation of the ICs allows to keep pinmuxing, V4L2, > DRM, audio bridging, interrupt controller and all other subcontroller > functions separately, configure them separately, store under separate > driver frameworks etc. That's a perfect flexibility on drivers level > as I can see it. The resulting complexity is still overkill. As mentioned above, let's focus on the DT bindings first. > >> Essentially they are multi purpose buses - which do not yet have a home. > >> We have used media as a home because of our use case. > >> > >> The use case whether they transfer frames from a camera or to a display > >> are of course closely related, but ultimately covered by two separate > >> subsystems at the pixel level (DRM vs V4L, or other for other data) > >> > >> Perhaps as they are buses - on a level with USB or I2C (except they can > >> of course carry I2C or Serial as well as 'bi-directional video' etc ), > >> they are looking for their own subsystem. > >> > >> Except I don't think we don't want to add a new subsystem for just one > >> (or two) devices... > > > > I'm not sure a new subsystem is needed. As you've noted there's an overlap > > between drivers/media/ and drivers/gpu/drm/ in terms of supported > > hardware. We even have a devices supported by two drivers, one in > > drivers/media/ and one in drivers/gpu/drm/ (I'm thinking about the > > adv7511 in particular). This is a well known issue, and so far nothing > > has been done in mainline to try and solve it. > > Right, presumably this IC series would have *cell* drivers in both > drivers/media and drivers/gpu/drm/ and other folders like drivers/pinctrl/ > or sound/. Even with cell drivers you'll have the problem of DRM vs. V4L2. My advice is to go for DRM only for now. > The interesting thing is that the published drivers do NOT require any new > cell drivers (at least non-trivial ones with >200 LoC) in drivers/media/ > or drivers/gpu/drm/ to get the expected operation of DS90Ux925/926/927/928 > ICs (any ser-des connection combination), and we have a DS90Ux940 cell > driver targeted to drivers/media/ That's because you abuse the rest of the APIs to cover up for what's lacking in media/ and gpu/drm/ :-) > > Trying to find another home in drivers/mfd/ to escape from the problem > > isn't a good solution in my opinion. The best option from a Linux point > > of view would be to unify V4L2 and DRM/KMS when it comes to bridge > > support, but that's a long way down the road (I won't complain if you > > want to give it a go though :-)). As your use cases are display, focused, > > I would propose to start with drivers/gpu/drm/bridge/, and leave the > > problem of camera support for first person who will have such a use case. > > Well, what should I do with pinctrl or audio bridging device drivers? I'll defer the audio problems to people more knowledgeable about audio. For pinctrl I believe you're making it way more complicated than it should be. Again, DT bindings first, drivers second. And please provide DT examples for real use cases, I think that will be key to proper DT bindings design. > Stick all drivers together into an unmaintainable clod of tangled code? You know this isn't what I proposed, let's stay fair in this discussion. Your proposal is too complex in my opinion, I want to simplify it, which will result in easier to maintain code. > Let's better exploit the excellent opportunity for code modularity given > by the MFD framework. > > >>>> certainly has different functions which are broken out in this > >>>> implementation. > >>> > >>> No, there is absolutely nothing broken out from the presented MFD > >>> drivers, the drivers are completely integral and basically I don't > >>> expect any. > >>> > >>> If you are concerned about media functionality, the correspondent MFD > >>> *cell* drivers will be added into drivers/media, drivers/gpu/drm or > >>> whatever is to be a proper place. > >>> > >>>> I think it might be quite awkward having the i2c components in > >>>> drivers/i2c and the media components in drivers/media/i2c, so what > >>>> about creating drivers/media/i2c/fpd-link (or such) as a container? > >>> > >>> I open drivers/media/i2c/Kconfig and all entries with no exception are > >>> under from 'if VIDEO_V4L2'. The MFD drivers do NOT require on depend on > >>> VIDEO_V4L2 or any other multimedia frameworks, nor the MFD drivers > >>> export any multimedia controls. > >>> > >>>> Our GMSL implementation is also a complex camera(s) device - but does > >>>> not yet use the MFD framework, perhaps that's something to add to my > >>>> todo list. > > > > Nope, please don't :-) The GMSL (de)serializers are no MFD devices either. > > > >>> Okay, but the TI DS90Ux9xx is NOT a camera device, and it is NOT a > >>> multimedia device, but it is a pure MFD device so the argument is not > >>> applicable. > > > > For the reasons pointed out in the review of other patches, and also > > pointed out by Lee in his review of this patch, I disagree with that. > > This isn't an MFD device. > > Eh, it is an MFD device. Just probably drivers/mfd is really not the best > place to store this particular MFD device driver... We still disagree. Those chips are video serializers and deserializers with a few additional features to support those use cases. The fact that additional support features are available don't automatically make them a true MFD. To give you another example, my camera ISP that happens to be able to output a clock for the camera sensor is also not an MFD, and I can still support clock control through CCF without requiring the MFD framework. > >>>> We currently keep all of the complexity within the max9286.c driver, > >>>> but I could foresee that being split further if more devices add to the > >>>> complexity of managing the bus. At which point we might want an > >>>> equivalent drivers/media/i2c/gmsl/ perhaps? > >>>> > >>>>>> Laurent, can you please share your opinion? > > > > Done :-) > > > > I'd like to mention that I would prefer focusing on the DT bindings first, > > and > > Sure, thank you for your comments :) > > > then move to the driver side. In that area I would like to have a full > > example of a system using these chips, as the "initial support" is too > > limited for a proper review. This won't come as a surprise, but I will > > expect the OF graph bindings to be used to model data connections. > > The leverage of "the initial support" to "the complete support" requires: > * audio bridge cell driver -- trivial, just one mute/unmute control, > * interrupt controller cell driver -- trivial, but for sake of perfection > it requires some minimal changes in drivers/base/regmap/regmap-irq.c This is a topic we haven't discussed yet, don't jump to conclusions and consider that an interrupt controller driver is needed. Could you please explain the use cases and point to the right documentation ? > * DS90Ux940 MIPI CSI-2 -- non-trivial one, but we have it ready, I just > don't want to add it to the pile at the moment, otherwise we'll continue > discussing cameras, and I'd like to postpone it :) I think it plays a crucial role in the architecture design. I don't want to reach an agreement on an architecture that wouldn't include CSI-2 support and then find out that it was the wrong path to add CSI-2 support. > No more than that is needed to get absolutely complete support of 5 claimed > DS90UB9xx ICs, really. 5 other DS90UH9xx will require HDCP support in > addition. I'm not too concern by HDCP, as long as you have a DRM bridge driver, it shouldn't be a big issue. > I'll try to roll out an example of DTS snippet soon.
Hi Vladimir, On Saturday, 13 October 2018 18:10:25 EEST Vladimir Zapolskiy wrote: > On 10/12/2018 04:01 PM, Laurent Pinchart wrote: > > On Friday, 12 October 2018 14:47:52 EEST Kieran Bingham wrote: > >> On 12/10/18 11:58, Vladimir Zapolskiy wrote: > [snip] > > >> Essentially they are multi purpose buses - which do not yet have a home. > >> We have used media as a home because of our use case. > >> > >> The use case whether they transfer frames from a camera or to a display > >> are of course closely related, but ultimately covered by two separate > >> subsystems at the pixel level (DRM vs V4L, or other for other data) > >> > >> Perhaps as they are buses - on a level with USB or I2C (except they can > >> of course carry I2C or Serial as well as 'bi-directional video' etc ), > >> they are looking for their own subsystem. > >> > >> Except I don't think we don't want to add a new subsystem for just one > >> (or two) devices... > > > > I'm not sure a new subsystem is needed. As you've noted there's an overlap > > between drivers/media/ and drivers/gpu/drm/ in terms of supported > > hardware. We even have a devices supported by two drivers, one in drivers/ > > media/ and one in drivers/gpu/drm/ (I'm thinking about the adv7511 in > > particular). This is a well known issue, and so far nothing has been done > > in mainline to try and solve it. > > I agree that there's an overlap between drivers/media/ and drivers/gpu/drm/, > formally a hypothetical (sic!) DS90Ux9xx video bridge cell driver should be > added into both subsystems also, and the actual driver of two should be > selected in runtime. I call such a driver 'hypothetical', because in fact I > don't have it, and I'm not so sure that its existence is justified, but > that's only because DS90Ux9xx video bridge functionality is _transparent_, > it does not have any controls literally, but it is a pure luck eventually. I don't think that's entirely correct, there's at least the video bus width (18-bit/24-bit) that needs to be selected. You currently do so through a pinctrl property, but that's not right. > So, as I've stated in my cover letter, I can misuse yours 'lvds-encoder' > driver only for the purpose of establishing a mediated link between > an LVDS controller and a panel over a serializer-deserializer pair. > > > Trying to find another home in drivers/mfd/ to escape from the problem > > isn't a good solution in my opinion. The best option from a Linux point > > of view would be to unify V4L2 and DRM/KMS when it comes to bridge > > support, but that's a long way down the road (I won't complain if you > > want to give it a go though> > > :-)). > > I return you a wider smile :) > > > As your use cases are display, focused, I would propose to start with > > drivers/gpu/drm/bridge/, and leave the problem of camera support for first > > person who will have such a use case. > > Frankly speaking I would like to start from copy-pasting your 'lvds-encoder' > driver into an 'absolutely-transparent-video-bridge' driver with no LVDS or > 'encoder' specifics, adding just a new compatible may suffice, if the > driver is renamed/redefined. > > PS, I remember I owe you a reference OF snippet of data path to a panel.
Hi Vladimir, On Friday, 12 October 2018 17:36:39 EEST Vladimir Zapolskiy wrote: > On 10/12/2018 04:12 PM, Laurent Pinchart wrote: > > On Friday, 12 October 2018 10:32:32 EEST Vladimir Zapolskiy wrote: > >> On 10/12/2018 09:04 AM, Lee Jones wrote: > >>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote: > >>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>> > >>>> The change adds TI DS90Ux9xx I2C bridge/alias subdevice driver and > >>>> FPD Link connection handling mechanism. > >>>> > >>>> Access to I2C devices connected to a remote de-/serializer is done in > >>>> a transparent way, on established link detection event such devices > >>>> are registered on an I2C bus, which serves a local de-/serializer IC. > >>>> > >>>> The development of the driver was a collaborative work, the > >>>> contribution done by Balasubramani Vivekanandan includes: > >>>> * original simplistic implementation of the driver, > >>>> * support of implicitly specified devices in device tree, > >>>> * support of multiple FPD links for TI DS90Ux9xx, > >>>> * other kind of valuable review comments, clean-ups and fixes. > >>>> > >>>> Also Steve Longerbeam made the following changes: > >>>> * clear address maps after linked device removal, > >>>> * disable pass-through in disconnection, > >>>> * qualify locked status with non-zero remote address. > >>>> > >>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > >>>> --- > >>>> > >>>> drivers/mfd/Kconfig | 8 + > >>>> drivers/mfd/Makefile | 1 + > >>>> drivers/mfd/ds90ux9xx-i2c-bridge.c | 764 +++++++++++++++++++++++++++++ > >>>> 3 files changed, 773 insertions(+) > >>>> create mode 100644 drivers/mfd/ds90ux9xx-i2c-bridge.c > >>> > >>> Shouldn't this live in drivers/i2c? > >> > >> no, the driver is not for an I2C controller of any kind, and the driver > >> does not register itself in the I2C subsystem by calling > >> i2c_add_adapter() or i2c_add_numbered_adapter() or i2c_mux_add_adapter() > >> etc, this topic was discussed with Wolfram also. > > > > (Who is now on CC) > > Wolfram has copies of the drivers and discussion right from the beginning, > hopefully he won't get two copies ;) > > >> Formally the driver converts the managed IC into a multi-address I2C > >> slave device, I understand that it does not sound like a well suited > >> driver for MFD, but ds90ux9xx-core.c and ds90ux9xx-i2c-bridge.c drivers > >> are quite tightly coupled. > > > > As mentioned in other e-mails in this thread I don't think this should be > > split out to a separate driver,> I would move the functionality to the > > ds90ux9xx driver. > > The proposal may have the grounds, but the I2C bridging functionality of ICs > is quite detached from all other ones, thus it found its place in the cell > driver per se. > > > You may want to register an I2C mux, but as you have a single port, that > > could be overkill. I haven't studied in details how to best support this > > chip using the existing I2C subsystems APIs (which we may want to extend > > if it needed), but I believe that (in your use cases) the deserializer > > should be a child of the serializer, and modeled as an I2C device. > > Formally in OF terms to define a link between devices by a phandle should > be sufficient, panels are not the children of LVDS controllers under OF > graph constraints in DT representation, the panels become secondary in > runtime only, I'd like to reuse the concept. Also it adds a better sense of > symmetry of deserializer <-> serializer connections relatively to a > SoC/data source. As I explained, DT models control buses through parent/child relationships. That's why I2C slaves are children of their I2C master. OF graph adds a second topology to describe data connections, which are orthogonal to the control bus relationship. In your case the device at the remote side of the link is controlled over the link, and that control flow goes from the SoC to the device on the local side of the link. That's why the remote device should be a child of the local device.
Hi Laurent, On 10/16/2018 04:12 PM, Laurent Pinchart wrote: > Hi Vladimir, > > On Saturday, 13 October 2018 18:10:25 EEST Vladimir Zapolskiy wrote: >> On 10/12/2018 04:01 PM, Laurent Pinchart wrote: >>> On Friday, 12 October 2018 14:47:52 EEST Kieran Bingham wrote: >>>> On 12/10/18 11:58, Vladimir Zapolskiy wrote: >> [snip] >> >>>> Essentially they are multi purpose buses - which do not yet have a home. >>>> We have used media as a home because of our use case. >>>> >>>> The use case whether they transfer frames from a camera or to a display >>>> are of course closely related, but ultimately covered by two separate >>>> subsystems at the pixel level (DRM vs V4L, or other for other data) >>>> >>>> Perhaps as they are buses - on a level with USB or I2C (except they can >>>> of course carry I2C or Serial as well as 'bi-directional video' etc ), >>>> they are looking for their own subsystem. >>>> >>>> Except I don't think we don't want to add a new subsystem for just one >>>> (or two) devices... >>> >>> I'm not sure a new subsystem is needed. As you've noted there's an overlap >>> between drivers/media/ and drivers/gpu/drm/ in terms of supported >>> hardware. We even have a devices supported by two drivers, one in drivers/ >>> media/ and one in drivers/gpu/drm/ (I'm thinking about the adv7511 in >>> particular). This is a well known issue, and so far nothing has been done >>> in mainline to try and solve it. >> >> I agree that there's an overlap between drivers/media/ and drivers/gpu/drm/, >> formally a hypothetical (sic!) DS90Ux9xx video bridge cell driver should be >> added into both subsystems also, and the actual driver of two should be >> selected in runtime. I call such a driver 'hypothetical', because in fact I >> don't have it, and I'm not so sure that its existence is justified, but >> that's only because DS90Ux9xx video bridge functionality is _transparent_, >> it does not have any controls literally, but it is a pure luck eventually. > > I don't think that's entirely correct, there's at least the video bus width > (18-bit/24-bit) that needs to be selected. You currently do so through a > pinctrl property, but that's not right. if you deal with a complex IC/IP which supports parallel video output routed over multiplexed pins, you have to specify a pinmux configuration, it is unavoidable (for reference see arch/arm/boot/dts/imx6qdl-sabrelite.dtsi and &pinctrl_j15 pin group, why does pinctrl setting exist?), so the property will remain as a pinmux/pinctrl property in one or another form independently on a probable video bus width selection of a DS90Ux9xx video bridge cell. In this particular case the pinmux/pinctrl driver shall be aware of 'ti,video-depth-18bit' property of 'parallel' pin function to resolve pin resource conflicts with GPIO and audio bridging functions of IC, this is a clear hardware pinmux (or pinctrl of "parallel" function) property. Please don't neglect the complexity and necessity of pinmuxing and other IC functions, if all provided functions of DS90Ux9xx ICs are put aside and just video bridging is left, only then you justify the device as a media device, but the IC and its configuration is simply more complex than you describe it. And, as I've said before, the video bridging function is extremely trivial and it has no real controls, but other functions have. -- Best wishes, Vladimir
Hi Laurent, On 10/12/2018 02:59 PM, Vladimir Zapolskiy wrote: > Hello Laurent. > > On 10/12/2018 04:01 PM, Laurent Pinchart wrote: >> Hello Vladimir, >> ... >> then move to the driver side. In that area I would like to have a full example >> of a system using these chips, as the "initial support" is too limited for a >> proper review. This won't come as a surprise, but I will expect the OF graph >> bindings to be used to model data connections. >> > > The leverage of "the initial support" to "the complete support" requires: > * audio bridge cell driver -- trivial, just one mute/unmute control, > * interrupt controller cell driver -- trivial, but for sake of perfection > it requires some minimal changes in drivers/base/regmap/regmap-irq.c > * DS90Ux940 MIPI CSI-2 -- non-trivial one, but we have it ready, I just > don't want to add it to the pile at the moment, otherwise we'll continue > discussing cameras, and I'd like to postpone it :) > > No more than that is needed to get absolutely complete support of 5 claimed > DS90UB9xx ICs, really. 5 other DS90UH9xx will require HDCP support in addition. > > I'll try to roll out an example of DTS snippet soon. Below I share an example of the serializer and deserializer equipped boards described in DT. The example naturally describes *two* simplistic boards in device tree representation -- main board with an application SoC (ordinary i.MX6*) and panel display module board. For demonstation I select a simple FPD-Link III connection between two boards, note that significantly more advanced configurations are also supported by the published drivers, for example deliberately I skip audio bridging functionality. The main board features: * TI DS90UB927Q serializer (LVDS input) at 0xc, connected to SoC over I2C2, SoC GPIO5[10] signal is connected to the IC PDB pin, * a status LED connected to DS90UB927Q GPIO2, it shall turn on, if FPD-Link III connection is established, * TI DS90UB928Q GPIO0 line signal is pulled-up, * TI DS90UB927Q GPIO3 line serves as generic GPIO, it is supposed to be controlled from userspace, * TI DS90UB927Q INTB line is connected to SoC GPIO5[4], the line serves as an interrupt line routed from a touchscreen controller on a panel display module. The panel display module board features: * TI DS90UB928Q deserializer (LVDS output), *mapped* to have 0x3b address, * AUO C070EAT01 panel, * I2C EEPROM at 0x50, *mapped* to have 0x52 address on SoC I2C bus, * Atmel MaxTouch touchscreen controller at 0x4b, *mapped* to have 0x60 address on SoC I2C bus, power-up control signal is connected to DS90UB928Q GPIO4, * a status LED connected to DS90UB928Q GPIO0, its on/off status shall repeat a user-defined status of DS90UB927Q GPIO0 on the main board, * TI DS90UB928Q GPIO1 controls panel backlight, bridges DS90UB927Q GPIO1 signal level, which in turn is connected to a SoC controlled GPIO, * TI DS90UB928Q GPIO2 line signal is pulled-up, * TI DS90UB928Q GPIO3 line serves as generic GPIOs, it is supposed to be controlled from userspace. All OF hard-coded controls like pinmuxing, I2C bridging of a remote deserializer and I2C devices behind it, GPIO line state setting and so forth must be applied with no interaction from a user -- and it just works with the current / published versions of the drivers, in other words a panel display module as a whole is truly hot-pluggable over FPD-Link III connection. The example is quite close to ones found in reality, if we put aside production main boards from the real world, *the panel display modules* or sensor modules (in case of a reverted serializer to deserializer link connection to SoC) are even more complex, they host FPGAs, all kinds of sensors, RF tuners, audio sources and sinks, and loads of other incredible and fascinating stuff. The published drivers allow to support very intricate and fine grained control of "remote" PCBs, and reducing the complexity to "just a media device" level could be done only if various IC functions are excluded from the consideration. Here my purpose is to demonstrate that * pinmux and GPIO controller functions are crucial and non-replaceable, * I2C bridge function modeled as another cell device actually does not fit into the existing I2C host/mux device driver models, and it is a completely new abstraction with custom device tree properties, * video bridge is absolutely transparent, thus trivial, and is modeled as another cell device, if needed it would be possible to write a DRM driver at no cost, * reusing OF graph model fits naturally, bus vs. link discussion can be started separately, note that LVDS (a.k.a FPD-Link) is formally an electric bus, so please define the difference, * to sum up I see no real objections to the given model of IC series support in Linux as an MFD parent device driver, plus pinmux/GPIO controller cell device driver, plus other needed cell device drivers. For video bridging I fabricated a "video-bridge" device driver, it can be substituted by your "lvds-encoder" driver, here I just need a simple transparent video bridge driver with NO media controls, its only purpose is to establish OF graph links, also note that on "remote" side a video bridge cell node can be omitted (but it may ). The example is NOT build tested and it may contain errors of secondary importance, but it tends to repeat the real usage and description of TI DS90Ux9xx equipped boards. ======== The panel display module board device tree description ======== / { display-module { panel { compatible = "auo,c070eat01", "panel-lvds"; pinctrl-names = "default"; pinctrl-0 = <&panel_pins>; width-mm = <153>; height-mm = <90>; data-mapping = "jeida-24"; panel-timing { clock-frequency = <71000000>; hactive = <1280>; vactive = <800>; hsync-len = <70>; hfront-porch = <20>; hback-porch = <70>; vsync-len = <5>; vfront-porch = <3>; vback-porch = <15>; }; port { panel_input: endpoint { remote-endpoint = <&ds90ub928_output>; }; }; }; deserializer: deserializer { compatible = "ti,ds90ub928q", "ti,ds90ux9xx"; i2c-bridge { compatible = "ti,ds90ux9xx-i2c-bridge"; #address-cells = <1>; #size-cells = <0>; touchscreen@4b { compatible = "atmel,maxtouch"; reg = <0x4b>; pinctrl-names = "default"; pinctrl-0 = <&touchscreen_pins>; interrupt-parent = <&ds90ub927_intc>; interrupts = <0>; atmel,mtu = <200>; }; eeprom@50 { compatible = "microchip,24lc128"; reg = <0x50>; pagesize = <64>; }; }; ds90ub928_pctrl: pin-controller { compatible = "ti,ds90ub928q-pinctrl", "ti,ds90ux9xx-pinctrl"; gpio-controller; #gpio-cells = <2>; gpio-ranges = <&ds90ub928_pctrl 0 0 8>; pinctrl-names = "default"; pinctrl-0 = <&led_pins>; led_pins: pinmux { gpio-remote { pins = "gpio0"; function = "gpio-remote"; }; }; panel_pins: panel-pwm { gpio-remote { pins = "gpio1"; function = "gpio-remote"; }; }; touchscreen_pins: touchscreen-power-up { gpio-hog; gpios = <4 GPIO_ACTIVE_HIGH>; output-high; }; }; /* * For simplicity video-bridge can be simply removed here * by "connecting" ds90ub927_fpd to &panel_input directly. */ video-bridge { compatible = "ti,ds90ux9xx-video-bridge", "video-bridge"; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; ds90ub928_fpd: endpoint { remote-endpoint = <&ds90ub927_fpd>; }; }; port@1 { reg = <1>; ds90ub928_output: endpoint { remote-endpoint = <&panel_input>; }; }; }; }; }; }; }; ======== The main board device tree description ======== / { /* iMX6 series SoC for demonstration purpose */ &ldb { status = "okay"; lvds-channel@1 { status = "okay"; fsl,data-mapping = "jeida"; fsl,data-width = <24>; port@4 { reg = <4>; lvds1_out: endpoint { remote-endpoint = <&ds90ub927_lvds>; }; }; }; }; &i2c2 { status = "okay"; clock-frequency = <400000>; serializer: serializer@c { compatible = "ti,ds90ub927q", "ti,ds90ux9xx"; reg = <0xc>; power-gpios = <&gpio5 10 GPIO_ACTIVE_HIGH>; ti,backward-compatible-mode = <0>; ti,low-frequency-mode = <0>; i2c-bridge { compatible = "ti,ds90ux9xx-i2c-bridge"; ti,i2c-bridges = <&deserializer 0 0x3b>; ti,i2c-bridge-maps = <0 0x4b 0x60>, <0 0x50 0x52>; }; ds90ub927_intc: interrupt-controller { compatible = "ti,ds90ub927-intc"; interrupt-parent = <&gpio5>; interrupts = <4 IRQ_TYPE_EDGE_RISING>; interrupt-controller; #interrupt-cells = <1>; }; ds90ub927_pctrl: pin-controller { compatible = "ti,ds90ub927b-pinctrl", "ti,ds90ux9xx-pinctrl"; gpio-controller; #gpio-cells = <2>; gpio-ranges = <&ds90ub927_pctrl 0 0 8>; /* Wired to some SoC controlled GPIO */ pwm-backlight { gpio-hog; gpios = <1 GPIO_ACTIVE_HIGH>; input; }; led_pins: pinmux { gpio-remote { pins = "gpio2"; function = "gpio-remote"; }; }; }; video-bridge { compatible = "ti,ds90ux9xx-video-bridge", "video-bridge"; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; ds90ub927_lvds: endpoint { remote-endpoint = <&lvds1_out>; }; }; port@1 { reg = <1>; ds90ub927_fpd: endpoint { remote-endpoint = <&ds90ub928_fpd>; }; }; }; }; }; }; }; ==== end of example ===== -- Best wishes, Vladimir
Hi Vladimir, On 23/10/18 10:16, Vladimir Zapolskiy wrote: > Hi Laurent, > > On 10/12/2018 02:59 PM, Vladimir Zapolskiy wrote: >> Hello Laurent. >> >> On 10/12/2018 04:01 PM, Laurent Pinchart wrote: >>> Hello Vladimir, >>> > > ... > >>> then move to the driver side. In that area I would like to have a full example >>> of a system using these chips, as the "initial support" is too limited for a >>> proper review. This won't come as a surprise, but I will expect the OF graph >>> bindings to be used to model data connections. >>> >> >> The leverage of "the initial support" to "the complete support" requires: >> * audio bridge cell driver -- trivial, just one mute/unmute control, >> * interrupt controller cell driver -- trivial, but for sake of perfection >> it requires some minimal changes in drivers/base/regmap/regmap-irq.c >> * DS90Ux940 MIPI CSI-2 -- non-trivial one, but we have it ready, I just >> don't want to add it to the pile at the moment, otherwise we'll continue >> discussing cameras, and I'd like to postpone it :) >> >> No more than that is needed to get absolutely complete support of 5 claimed >> DS90UB9xx ICs, really. 5 other DS90UH9xx will require HDCP support in addition. >> >> I'll try to roll out an example of DTS snippet soon. > > Below I share an example of the serializer and deserializer equipped boards > described in DT. > > The example naturally describes *two* simplistic boards in device tree > representation -- main board with an application SoC (ordinary i.MX6*) > and panel display module board. For demonstation I select a simple > FPD-Link III connection between two boards, note that significantly > more advanced configurations are also supported by the published > drivers, for example deliberately I skip audio bridging functionality. Thanks for the comprehensive example, it helps a lot in understanding your proposal. > The main board features: > * TI DS90UB927Q serializer (LVDS input) at 0xc, connected to SoC over I2C2, > SoC GPIO5[10] signal is connected to the IC PDB pin, > * a status LED connected to DS90UB927Q GPIO2, it shall turn on, > if FPD-Link III connection is established, > * TI DS90UB928Q GPIO0 line signal is pulled-up, > * TI DS90UB927Q GPIO3 line serves as generic GPIO, it is supposed to be > controlled from userspace, > * TI DS90UB927Q INTB line is connected to SoC GPIO5[4], the line serves > as an interrupt line routed from a touchscreen controller on a panel > display module. > > The panel display module board features: > * TI DS90UB928Q deserializer (LVDS output), *mapped* to have 0x3b address, > * AUO C070EAT01 panel, > * I2C EEPROM at 0x50, *mapped* to have 0x52 address on SoC I2C bus, > * Atmel MaxTouch touchscreen controller at 0x4b, *mapped* to have 0x60 > address on SoC I2C bus, power-up control signal is connected to DS90UB928Q GPIO4, > * a status LED connected to DS90UB928Q GPIO0, its on/off status shall > repeat a user-defined status of DS90UB927Q GPIO0 on the main board, > * TI DS90UB928Q GPIO1 controls panel backlight, bridges DS90UB927Q > GPIO1 signal level, which in turn is connected to a SoC controlled GPIO, > * TI DS90UB928Q GPIO2 line signal is pulled-up, > * TI DS90UB928Q GPIO3 line serves as generic GPIOs, it is supposed to be > controlled from userspace. > > All OF hard-coded controls like pinmuxing, I2C bridging of a remote > deserializer and I2C devices behind it, GPIO line state setting and so > forth must be applied with no interaction from a user -- and it just > works with the current / published versions of the drivers, in other > words a panel display module as a whole is truly hot-pluggable over > FPD-Link III connection. > > The example is quite close to ones found in reality, if we put aside > production main boards from the real world, *the panel display modules* > or sensor modules (in case of a reverted serializer to deserializer link > connection to SoC) are even more complex, they host FPGAs, all kinds of > sensors, RF tuners, audio sources and sinks, and loads of other > incredible and fascinating stuff. > > The published drivers allow to support very intricate and fine grained > control of "remote" PCBs, and reducing the complexity to "just a media > device" level could be done only if various IC functions are excluded > from the consideration. Here my purpose is to demonstrate that > * pinmux and GPIO controller functions are crucial and non-replaceable, > * I2C bridge function modeled as another cell device actually does not > fit into the existing I2C host/mux device driver models, and it is > a completely new abstraction with custom device tree properties, > * video bridge is absolutely transparent, thus trivial, and is modeled > as another cell device, if needed it would be possible to write a > DRM driver at no cost, > * reusing OF graph model fits naturally, bus vs. link discussion can > be started separately, note that LVDS (a.k.a FPD-Link) is formally > an electric bus, so please define the difference, > * to sum up I see no real objections to the given model of IC series > support in Linux as an MFD parent device driver, plus pinmux/GPIO > controller cell device driver, plus other needed cell device drivers. > > For video bridging I fabricated a "video-bridge" device driver, it can > be substituted by your "lvds-encoder" driver, here I just need a simple > transparent video bridge driver with NO media controls, its only purpose > is to establish OF graph links, also note that on "remote" side a > video bridge cell node can be omitted (but it may ). > > The example is NOT build tested and it may contain errors of secondary > importance, but it tends to repeat the real usage and description of > TI DS90Ux9xx equipped boards. > > ======== The panel display module board device tree description ======== > > / { > display-module { > panel { > compatible = "auo,c070eat01", "panel-lvds"; > pinctrl-names = "default"; > pinctrl-0 = <&panel_pins>; > > width-mm = <153>; > height-mm = <90>; > > data-mapping = "jeida-24"; > > panel-timing { > clock-frequency = <71000000>; > hactive = <1280>; > vactive = <800>; > hsync-len = <70>; > hfront-porch = <20>; > hback-porch = <70>; > vsync-len = <5>; > vfront-porch = <3>; > vback-porch = <15>; > }; > > port { > panel_input: endpoint { > remote-endpoint = <&ds90ub928_output>; > }; > }; > }; > > deserializer: deserializer { > compatible = "ti,ds90ub928q", "ti,ds90ux9xx"; > > i2c-bridge { > compatible = "ti,ds90ux9xx-i2c-bridge"; > #address-cells = <1>; > #size-cells = <0>; > > touchscreen@4b { > compatible = "atmel,maxtouch"; > reg = <0x4b>; > pinctrl-names = "default"; > pinctrl-0 = <&touchscreen_pins>; > interrupt-parent = <&ds90ub927_intc>; > interrupts = <0>; > atmel,mtu = <200>; > }; > > eeprom@50 { > compatible = "microchip,24lc128"; > reg = <0x50>; > pagesize = <64>; > }; > }; > > ds90ub928_pctrl: pin-controller { > compatible = "ti,ds90ub928q-pinctrl", "ti,ds90ux9xx-pinctrl"; > gpio-controller; > #gpio-cells = <2>; > gpio-ranges = <&ds90ub928_pctrl 0 0 8>; > > pinctrl-names = "default"; > pinctrl-0 = <&led_pins>; > > led_pins: pinmux { > gpio-remote { > pins = "gpio0"; > function = "gpio-remote"; > }; > }; > > panel_pins: panel-pwm { > gpio-remote { > pins = "gpio1"; > function = "gpio-remote"; > }; > }; > > touchscreen_pins: touchscreen-power-up { > gpio-hog; > gpios = <4 GPIO_ACTIVE_HIGH>; > output-high; > }; > }; > > /* > * For simplicity video-bridge can be simply removed here > * by "connecting" ds90ub927_fpd to &panel_input directly. > */ > video-bridge { > compatible = "ti,ds90ux9xx-video-bridge", "video-bridge"; > > ports { > #address-cells = <1>; > #size-cells = <0>; > > port@0 { > reg = <0>; > ds90ub928_fpd: endpoint { > remote-endpoint = <&ds90ub927_fpd>; > }; > }; > > port@1 { > reg = <1>; > ds90ub928_output: endpoint { > remote-endpoint = <&panel_input>; > }; > }; > }; > }; > }; > }; > }; > > ======== The main board device tree description ======== > > / { > /* iMX6 series SoC for demonstration purpose */ > > &ldb { > status = "okay"; > > lvds-channel@1 { > status = "okay"; > fsl,data-mapping = "jeida"; > fsl,data-width = <24>; > > port@4 { > reg = <4>; > > lvds1_out: endpoint { > remote-endpoint = <&ds90ub927_lvds>; > }; > }; > }; > }; > > &i2c2 { > status = "okay"; > clock-frequency = <400000>; > > serializer: serializer@c { > compatible = "ti,ds90ub927q", "ti,ds90ux9xx"; > reg = <0xc>; > power-gpios = <&gpio5 10 GPIO_ACTIVE_HIGH>; > ti,backward-compatible-mode = <0>; > ti,low-frequency-mode = <0>; > > i2c-bridge { > compatible = "ti,ds90ux9xx-i2c-bridge"; > ti,i2c-bridges = <&deserializer 0 0x3b>; DT should describe the connections with leaf nodes pointing towards the centre (CPU). Thus the local chip (serializer) pointing to the remote chip (serializer) is wrong here. Among others, it forbids the display-module tree from being a DT overlay inserted at runtime. This is fundamental to have different remote boards, detected at runtime. Of course runtime insertion/removal opens other issues, namely the fact that media-ctl nodes point to each other, and a media pipeline is not quite modifiable while streaming. But that is another issue. > ti,i2c-bridge-maps = <0 0x4b 0x60>, <0 0x50 0x52>; > }; > > ds90ub927_intc: interrupt-controller { > compatible = "ti,ds90ub927-intc"; > interrupt-parent = <&gpio5>; > interrupts = <4 IRQ_TYPE_EDGE_RISING>; > interrupt-controller; > #interrupt-cells = <1>; > }; > > ds90ub927_pctrl: pin-controller { > compatible = "ti,ds90ub927b-pinctrl", "ti,ds90ux9xx-pinctrl"; > gpio-controller; > #gpio-cells = <2>; > gpio-ranges = <&ds90ub927_pctrl 0 0 8>; > > /* Wired to some SoC controlled GPIO */ > pwm-backlight { > gpio-hog; > gpios = <1 GPIO_ACTIVE_HIGH>; > input; > }; > > led_pins: pinmux { > gpio-remote { > pins = "gpio2"; > function = "gpio-remote"; > }; > }; > }; > > video-bridge { > compatible = "ti,ds90ux9xx-video-bridge", "video-bridge"; > > ports { > #address-cells = <1>; > #size-cells = <0>; > > port@0 { > reg = <0>; > ds90ub927_lvds: endpoint { > remote-endpoint = <&lvds1_out>; > }; > }; > > port@1 { > reg = <1>; > ds90ub927_fpd: endpoint { > remote-endpoint = <&ds90ub928_fpd>; > }; > }; > }; > }; > }; > }; > }; > > ==== end of example ===== > > -- > Best wishes, > Vladimir >