| Message ID | 20181008211205.2900-5-vz@mleia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mfd/pinctrl: add initial support of TI DS90Ux9xx ICs | expand |
Hi Vladimir,
I love your patch! Perhaps something to improve:
[auto build test WARNING on ljones-mfd/for-mfd-next]
[also build test WARNING on v4.19-rc7 next-20181008]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Vladimir-Zapolskiy/mfd-pinctrl-add-initial-support-of-TI-DS90Ux9xx-ICs/20181009-090135
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
drivers/mfd/ds90ux9xx-core.c: In function 'ds90ux9xx_probe':
>> drivers/mfd/ds90ux9xx-core.c:789:4: warning: 'id_code' may be used uninitialized in this function [-Wmaybe-uninitialized]
dev_err(ds90ux9xx->dev,
^~~~~~~~~~~~~~~~~~~~~~~
drivers/mfd/ds90ux9xx-core.c:755:14: note: 'id_code' was declared here
const char *id_code;
^~~~~~~
vim +/id_code +789 drivers/mfd/ds90ux9xx-core.c
749
750 static int ds90ux9xx_read_ic_type(struct i2c_client *client,
751 struct ds90ux9xx *ds90ux9xx)
752 {
753 u8 device_id[DEVICE_ID_LEN + 1] = { 0 };
754 const struct i2c_device_id *id;
755 const char *id_code;
756 unsigned int i;
757 int ret;
758
759 ret = regmap_raw_read(ds90ux9xx->regmap, SER_DES_DEVICE_ID_REG,
760 device_id, DEVICE_ID_LEN);
761 if (ret < 0) {
762 dev_err(ds90ux9xx->dev, "Failed to read device id: %d\n", ret);
763 return ret;
764 }
765
766 id = i2c_match_id(ds90ux9xx_ids, client);
767 if (id) {
768 id_code = ds90ux9xx_devices[id->driver_data].id;
769 if (!strncmp(device_id, id_code, DEVICE_ID_LEN)) {
770 ds90ux9xx->dev_id = id->driver_data;
771 return 0;
772 }
773 }
774
775 /* DS90UH925 device quirk */
776 if (!memcmp(device_id, "\0\0\0\0\0\0", DEVICE_ID_LEN)) {
777 if (id && id->driver_data != TI_DS90UH925)
778 dev_err(ds90ux9xx->dev,
779 "Device ID returned all zeroes, assuming device is DS90UH925\n");
780 ds90ux9xx->dev_id = TI_DS90UH925;
781 return 0;
782 }
783
784 for (i = 0; i < ARRAY_SIZE(ds90ux9xx_devices); i++) {
785 if (strncmp(device_id, ds90ux9xx_devices[i].id, DEVICE_ID_LEN))
786 continue;
787
788 if (id)
> 789 dev_err(ds90ux9xx->dev,
790 "Mismatch between probed device ID [%s] and HW device ID [%s]\n",
791 id_code, device_id);
792
793 ds90ux9xx->dev_id = i;
794 return 0;
795 }
796
797 dev_err(ds90ux9xx->dev, "Device ID [%s] is not supported\n", device_id);
798
799 return -ENODEV;
800 }
801
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 10/09/2018 07:08 AM, kbuild test robot wrote: > Hi Vladimir, > > I love your patch! Perhaps something to improve: > > [auto build test WARNING on ljones-mfd/for-mfd-next] > [also build test WARNING on v4.19-rc7 next-20181008] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Vladimir-Zapolskiy/mfd-pinctrl-add-initial-support-of-TI-DS90Ux9xx-ICs/20181009-090135 > base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next > config: i386-allyesconfig (attached as .config) > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 > reproduce: > # save the attached .config to linux build tree > make ARCH=i386 > > Note: it may well be a FALSE warning. FWIW you are at least aware of it now. > http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings > And it is a false positive. -- Best wishes, Vladimir
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?
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?
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?
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?
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 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 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 >
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. + config MFD_LP3943 tristate "TI/National Semiconductor LP3943 MFD Driver" depends on I2C diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 12980a4ad460..cc92bf5394b7 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -224,6 +224,7 @@ obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o obj-$(CONFIG_MFD_DLN2) += dln2.o obj-$(CONFIG_MFD_RT5033) += rt5033.o obj-$(CONFIG_MFD_SKY81452) += sky81452.o +obj-$(CONFIG_MFD_DS90UX9XX) += ds90ux9xx-core.o intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o diff --git a/drivers/mfd/ds90ux9xx-core.c b/drivers/mfd/ds90ux9xx-core.c new file mode 100644 index 000000000000..ad96c109a451 --- /dev/null +++ b/drivers/mfd/ds90ux9xx-core.c @@ -0,0 +1,879 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * TI DS90Ux9xx MFD driver + * + * Copyright (c) 2017-2018 Mentor Graphics Inc. + */ + +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/mfd/core.h> +#include <linux/mfd/ds90ux9xx.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of_platform.h> +#include <linux/regmap.h> + +/* Serializer Registers */ +#define SER_REG_MODE_SELECT 0x04 +#define SER_REG_GEN_STATUS 0x0C +#define SER_REG_CTRL_STATUS 0x13 + +/* Deserializer Registers */ +#define DES_REG_GEN_CONFIG0 0x02 +#define DES_REG_GEN_STATUS 0x1C +#define DES_REG_MODE_STATUS 0x23 +#define DES_REG_SSCG_CTRL 0x2C +#define DES_REG_DUAL_RX_CTRL 0x34 +#define DES_REG_MAPSEL 0x49 +#define DES_REG_INDIRECT_ADDR 0x6C +#define DES_REG_INDIRECT_DATA 0x6D + +#define DES_FPD_MODE_MASK (BIT(4) | BIT(3)) +#define DES_FPD_HW_MODE_AUTO 0x0 +#define DES_FPD_HW_MODE_PRIMARY BIT(4) +#define DES_FPD_HW_MODE_SECONDARY (BIT(4) | BIT(3)) + +#define SSCG_REG_MASK 0x0F +#define SSCG_MAX_VALUE 0x7 +#define SSCG_ENABLE BIT(3) + +/* Common Registers and bitfields */ +#define SER_DES_REG_CONFIG 0x03 + +#define SER_DE_GATE_RGB BIT(4) +#define DES_DE_GATE_RGB BIT(1) + +#define PIXEL_CLK_EDGE_RISING BIT(0) + +#define MAPSEL_MASK (BIT(6) | BIT(5)) +#define MAPSEL_SET BIT(6) +#define MAPSEL_MSB BIT(5) + +#define BKWD_MODE_OVERRIDE BIT(3) +#define BKWD_MODE_ENABLED BIT(2) +#define LF_MODE_OVERRIDE BIT(1) +#define LF_MODE_ENABLED BIT(0) + +#define LF_MODE_PIN_STATUS BIT(3) +#define BKWD_MODE_PIN_STATUS BIT(1) + +#define GEN_STATUS_LOCK BIT(0) + +#define SER_DES_DEVICE_ID_REG 0xF0 +#define DEVICE_ID_LEN 6 + +enum ds90ux9xx_capability { + DS90UX9XX_CAP_SERIALIZER, + DS90UX9XX_CAP_HDCP, + DS90UX9XX_CAP_MODES, + DS90UX9XX_CAP_SSCG, + DS90UX9XX_CAP_PIXEL_CLK_EDGE, + DS90UX9XX_CAP_MAPSEL, + DS90UX9XX_CAP_DE_GATE, +}; + +struct ds90ux9xx_device_property { + const char id[DEVICE_ID_LEN]; + unsigned int num_links; + const u32 caps; +}; + +struct ds90ux9xx { + struct device *dev; + struct regmap *regmap; + struct mutex indirect; /* serializes access to indirect registers */ + struct gpio_desc *power_gpio; + enum ds90ux9xx_device_id dev_id; + const struct ds90ux9xx_device_property *property; +}; + +#define TO_CAP(_cap, _enabled) (_enabled ? BIT(DS90UX9XX_CAP_ ## _cap) : 0x0) + +#define DS90UX9XX_DEVICE(_id, _links, _ser, _hdcp, _modes, _sscg, \ + _pixel, _mapsel, _de_gate) \ + [TI_DS90 ## _id] = { \ + .id = "_" __stringify(_id), \ + .num_links = _links, \ + .caps = \ + TO_CAP(SERIALIZER, _ser) | \ + TO_CAP(HDCP, _hdcp) | \ + TO_CAP(MODES, _modes) | \ + TO_CAP(SSCG, _sscg) | \ + TO_CAP(PIXEL_CLK_EDGE, _pixel) | \ + TO_CAP(MAPSEL, _mapsel) | \ + TO_CAP(DE_GATE, _de_gate) \ + } + +static const struct ds90ux9xx_device_property ds90ux9xx_devices[] = { + /* + * List of TI DS90Ux9xx properties: + * # FPD-links, serializer, HDCP, BW/LF modes, SSCG, + * pixel clock edge, mapsel, RGB DE Gate + */ + DS90UX9XX_DEVICE(UB925, 1, 1, 0, 1, 0, 1, 0, 1), + DS90UX9XX_DEVICE(UH925, 1, 1, 1, 1, 0, 1, 0, 1), + DS90UX9XX_DEVICE(UB927, 1, 1, 0, 1, 0, 0, 1, 1), + DS90UX9XX_DEVICE(UH927, 1, 1, 1, 1, 0, 0, 1, 1), + DS90UX9XX_DEVICE(UB926, 1, 0, 0, 1, 1, 1, 0, 0), + DS90UX9XX_DEVICE(UH926, 1, 0, 1, 1, 1, 1, 0, 0), + DS90UX9XX_DEVICE(UB928, 1, 0, 0, 1, 0, 0, 1, 0), + DS90UX9XX_DEVICE(UH928, 1, 0, 1, 1, 0, 0, 1, 0), + DS90UX9XX_DEVICE(UB940, 2, 0, 0, 0, 0, 0, 0, 1), + DS90UX9XX_DEVICE(UH940, 2, 0, 1, 0, 0, 0, 0, 1), +}; + +static const struct regmap_config ds90ux9xx_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .max_register = 0xFF, + .cache_type = REGCACHE_NONE, +}; + +static bool ds90ux9xx_has_capability(struct ds90ux9xx *ds90ux9xx, + enum ds90ux9xx_capability cap) +{ + return ds90ux9xx->property->caps & BIT(cap); +} + +bool ds90ux9xx_is_serializer(struct device *dev) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + + return ds90ux9xx_has_capability(ds90ux9xx, DS90UX9XX_CAP_SERIALIZER); +} +EXPORT_SYMBOL_GPL(ds90ux9xx_is_serializer); + +unsigned int ds90ux9xx_num_fpd_links(struct device *dev) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + + return ds90ux9xx->property->num_links; +} +EXPORT_SYMBOL_GPL(ds90ux9xx_num_fpd_links); + +int ds90ux9xx_update_bits_indirect(struct device *dev, u8 reg, u8 mask, u8 val) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + int ret; + + mutex_lock(&ds90ux9xx->indirect); + + ret = regmap_write(ds90ux9xx->regmap, DES_REG_INDIRECT_ADDR, reg); + if (ret) { + mutex_unlock(&ds90ux9xx->indirect); + return ret; + } + + ret = regmap_update_bits(ds90ux9xx->regmap, DES_REG_INDIRECT_DATA, + mask, val); + + mutex_unlock(&ds90ux9xx->indirect); + + return ret; +} +EXPORT_SYMBOL_GPL(ds90ux9xx_update_bits_indirect); + +int ds90ux9xx_write_indirect(struct device *dev, u8 reg, u8 val) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + int ret; + + mutex_lock(&ds90ux9xx->indirect); + + ret = regmap_write(ds90ux9xx->regmap, DES_REG_INDIRECT_ADDR, reg); + if (ret) { + mutex_unlock(&ds90ux9xx->indirect); + return ret; + } + + ret = regmap_write(ds90ux9xx->regmap, DES_REG_INDIRECT_DATA, val); + + mutex_unlock(&ds90ux9xx->indirect); + + return ret; +} +EXPORT_SYMBOL_GPL(ds90ux9xx_write_indirect); + +int ds90ux9xx_read_indirect(struct device *dev, u8 reg, u8 *val) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + unsigned int data; + int ret; + + mutex_lock(&ds90ux9xx->indirect); + + ret = regmap_write(ds90ux9xx->regmap, DES_REG_INDIRECT_ADDR, reg); + if (ret) { + mutex_unlock(&ds90ux9xx->indirect); + return ret; + } + + ret = regmap_read(ds90ux9xx->regmap, DES_REG_INDIRECT_DATA, &data); + + mutex_unlock(&ds90ux9xx->indirect); + + if (!ret) + *val = data; + + return ret; +} +EXPORT_SYMBOL_GPL(ds90ux9xx_read_indirect); + +static int ds90ux9xx_read_active_link(struct ds90ux9xx *ds90ux9xx, + unsigned int *link) +{ + unsigned int val; + int ret; + + if (ds90ux9xx->property->num_links == 1) { + *link = 0; + return 0; + } + + ret = regmap_read(ds90ux9xx->regmap, DES_REG_DUAL_RX_CTRL, &val); + if (ret) { + dev_err(ds90ux9xx->dev, "Failed to read FPD mode\n"); + return ret; + } + + switch (val & DES_FPD_MODE_MASK) { + case DES_FPD_HW_MODE_AUTO: + case DES_FPD_HW_MODE_PRIMARY: + *link = 0; + break; + case DES_FPD_HW_MODE_SECONDARY: + *link = 1; + break; + default: + dev_err(ds90ux9xx->dev, "Unhandled FPD mode\n"); + return -EINVAL; + } + + return 0; +} + +/* Note that lock status is reported if video pattern generator is enabled */ +int ds90ux9xx_get_link_status(struct device *dev, unsigned int *link, + bool *locked) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + unsigned int reg, status; + int ret; + + ret = ds90ux9xx_read_active_link(ds90ux9xx, link); + if (ret) + return ret; + + if (ds90ux9xx_is_serializer(dev)) + reg = SER_REG_GEN_STATUS; + else + reg = DES_REG_GEN_STATUS; + + ret = regmap_read(ds90ux9xx->regmap, reg, &status); + if (ret) { + dev_err(dev, "Unable to get lock status\n"); + return ret; + } + + *locked = status & GEN_STATUS_LOCK ? true : false; + + return 0; +} +EXPORT_SYMBOL_GPL(ds90ux9xx_get_link_status); + +enum ds90ux9xx_device_id ds90ux9xx_get_ic_type(struct device *dev) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + + return ds90ux9xx->dev_id; +} +EXPORT_SYMBOL_GPL(ds90ux9xx_get_ic_type); + +static int ds90ux9xx_set_de_gate(struct ds90ux9xx *ds90ux9xx, bool enable) +{ + unsigned int reg, val; + + if (!ds90ux9xx_has_capability(ds90ux9xx, DS90UX9XX_CAP_DE_GATE)) + return 0; + + if (ds90ux9xx_is_serializer(ds90ux9xx->dev)) { + reg = SER_REG_MODE_SELECT; + val = SER_DE_GATE_RGB; + } else { + reg = SER_DES_REG_CONFIG; + val = DES_DE_GATE_RGB; + } + + return regmap_update_bits(ds90ux9xx->regmap, reg, val, + enable ? val : 0x0); +} + +static int ds90ux9xx_set_bcmode(struct ds90ux9xx *ds90ux9xx, bool enable) +{ + unsigned int reg, val; + int ret; + + if (ds90ux9xx_is_serializer(ds90ux9xx->dev)) + reg = SER_REG_MODE_SELECT; + else + reg = DES_REG_GEN_CONFIG0; + + val = BKWD_MODE_OVERRIDE; + if (enable) + val |= BKWD_MODE_ENABLED; + + ret = regmap_update_bits(ds90ux9xx->regmap, reg, + BKWD_MODE_OVERRIDE | BKWD_MODE_ENABLED, val); + if (ret) + return ret; + + return ds90ux9xx_set_de_gate(ds90ux9xx, !enable); +} + +static int ds90ux9xx_set_lfmode(struct ds90ux9xx *ds90ux9xx, bool enable) +{ + unsigned int reg, val; + + if (ds90ux9xx_is_serializer(ds90ux9xx->dev)) + reg = SER_REG_MODE_SELECT; + else + reg = DES_REG_GEN_CONFIG0; + + val = LF_MODE_OVERRIDE; + if (enable) + val |= LF_MODE_ENABLED; + + return regmap_update_bits(ds90ux9xx->regmap, reg, + LF_MODE_OVERRIDE | LF_MODE_ENABLED, val); +} + +static int ds90ux9xx_set_sscg(struct ds90ux9xx *ds90ux9xx, unsigned int val) +{ + if ((val & ~SSCG_ENABLE) > SSCG_MAX_VALUE) { + dev_err(ds90ux9xx->dev, "Invalid SSCG value: %#x", val); + return -EINVAL; + } + + return regmap_write(ds90ux9xx->regmap, DES_REG_SSCG_CTRL, val); +} + +static int ds90ux9xx_set_pixel_clk_edge(struct ds90ux9xx *ds90ux9xx, + bool rising) +{ + return regmap_update_bits(ds90ux9xx->regmap, SER_DES_REG_CONFIG, + PIXEL_CLK_EDGE_RISING, + rising ? PIXEL_CLK_EDGE_RISING : 0x0); +} + +static ssize_t backward_compatible_mode_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + unsigned int cfg_reg, sts_reg, val; + int ret, bcmode; + + if (ds90ux9xx_is_serializer(ds90ux9xx->dev)) { + cfg_reg = SER_REG_MODE_SELECT; + sts_reg = SER_REG_CTRL_STATUS; + } else { + cfg_reg = DES_REG_GEN_CONFIG0; + sts_reg = DES_REG_MODE_STATUS; + } + + ret = regmap_read(ds90ux9xx->regmap, cfg_reg, &val); + if (ret) + return ret; + + if (val & BKWD_MODE_OVERRIDE) { + bcmode = val & BKWD_MODE_ENABLED; + } else { + /* Read mode from pin status */ + ret = regmap_read(ds90ux9xx->regmap, sts_reg, &val); + if (ret) + return ret; + + bcmode = val & BKWD_MODE_PIN_STATUS; + } + + return sprintf(buf, "%d\n", bcmode ? 1 : 0); +} + +static ssize_t backward_compatible_mode_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + unsigned int val; + int ret; + + ret = kstrtouint(buf, 0, &val); + if (ret) + return ret; + + ret = ds90ux9xx_set_bcmode(ds90ux9xx, val); + if (ret) + return ret; + + return count; +} +static DEVICE_ATTR_RW(backward_compatible_mode); + +static ssize_t low_frequency_mode_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + unsigned int cfg_reg, sts_reg, val; + int ret, lfmode; + + if (ds90ux9xx_is_serializer(ds90ux9xx->dev)) { + cfg_reg = SER_REG_MODE_SELECT; + sts_reg = SER_REG_CTRL_STATUS; + } else { + cfg_reg = DES_REG_GEN_CONFIG0; + sts_reg = DES_REG_MODE_STATUS; + } + + ret = regmap_read(ds90ux9xx->regmap, cfg_reg, &val); + if (ret) + return ret; + + if (val & LF_MODE_OVERRIDE) { + lfmode = val & LF_MODE_ENABLED; + } else { + /* Read mode from pin status */ + ret = regmap_read(ds90ux9xx->regmap, sts_reg, &val); + if (ret) + return ret; + + lfmode = val & LF_MODE_PIN_STATUS; + } + + return sprintf(buf, "%d\n", lfmode ? 1 : 0); +} + +static ssize_t low_frequency_mode_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + unsigned int val; + int ret; + + ret = kstrtouint(buf, 0, &val); + if (ret) + return ret; + + ret = ds90ux9xx_set_lfmode(ds90ux9xx, val); + if (ret) + return ret; + + return count; +} +static DEVICE_ATTR_RW(low_frequency_mode); + +static ssize_t sscg_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + unsigned int val; + int ret; + + ret = regmap_read(ds90ux9xx->regmap, DES_REG_SSCG_CTRL, &val); + if (ret) + return ret; + + return sprintf(buf, "%d\n", val & SSCG_REG_MASK); +} + +static ssize_t sscg_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + int ret, val; + + ret = kstrtoint(buf, 0, &val); + if (ret) + return ret; + + ret = ds90ux9xx_set_sscg(ds90ux9xx, val); + if (ret) + return ret; + + return count; +} +static DEVICE_ATTR_RW(sscg); + +static ssize_t pixel_clock_edge_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + unsigned int val; + int ret; + + ret = regmap_read(ds90ux9xx->regmap, SER_DES_REG_CONFIG, &val); + if (ret) + return ret; + + return sprintf(buf, "%s\n", + (val & PIXEL_CLK_EDGE_RISING) ? "rising" : "falling"); +} + +static ssize_t pixel_clock_edge_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + bool rising; + int ret; + + if (sysfs_streq(buf, "rising") || sysfs_streq(buf, "1")) + rising = true; + else if (sysfs_streq(buf, "falling") || sysfs_streq(buf, "0")) + rising = false; + else + return -EINVAL; + + ret = ds90ux9xx_set_pixel_clk_edge(ds90ux9xx, rising); + if (ret) + return ret; + + return count; +} +static DEVICE_ATTR_RW(pixel_clock_edge); + +static ssize_t link_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + unsigned int link; + bool locked; + int ret; + + ret = ds90ux9xx_get_link_status(dev, &link, &locked); + if (ret) + return ret; + + return sprintf(buf, "%u: %s\n", link, locked ? "locked" : "unlocked"); +} +static DEVICE_ATTR_RO(link); + +static struct attribute *ds90ux9xx_attributes[] = { + &dev_attr_backward_compatible_mode.attr, + &dev_attr_low_frequency_mode.attr, + &dev_attr_sscg.attr, + &dev_attr_pixel_clock_edge.attr, + &dev_attr_link.attr, + NULL, +}; + +static umode_t ds90ux9xx_attr_is_visible(struct kobject *kobj, + struct attribute *attr, int index) +{ + struct device *dev = container_of(kobj, struct device, kobj); + struct ds90ux9xx *ds90ux9xx = dev_get_drvdata(dev); + enum ds90ux9xx_capability cap; + + /* Attribute indices match the ds90ux9xx_attributes[] array indices */ + switch (index) { + case 0: /* Backward compatible mode */ + case 1: /* Low frequency mode */ + cap = DS90UX9XX_CAP_MODES; + break; + case 2: /* Spread spectrum clock generator */ + cap = DS90UX9XX_CAP_SSCG; + break; + case 3: /* Pixel clock edge */ + cap = DS90UX9XX_CAP_PIXEL_CLK_EDGE; + break; + case 4: /* FPD-III link lock status - visible for all chips */ + return attr->mode; + default: + return 0; + } + + return ds90ux9xx_has_capability(ds90ux9xx, cap) ? attr->mode : 0; +} + +static const struct attribute_group ds90ux9xx_attr_group = { + .attrs = ds90ux9xx_attributes, + .is_visible = ds90ux9xx_attr_is_visible, +}; +__ATTRIBUTE_GROUPS(ds90ux9xx_attr); + +static int ds90ux9xx_config_modes(struct ds90ux9xx *ds90ux9xx) +{ + struct device_node *np = ds90ux9xx->dev->of_node; + u32 mode; + int ret; + + if (!ds90ux9xx_has_capability(ds90ux9xx, DS90UX9XX_CAP_MODES)) + return 0; + + ret = of_property_read_u32(np, "ti,backward-compatible-mode", &mode); + if (!ret) { + ret = ds90ux9xx_set_bcmode(ds90ux9xx, mode); + if (ret) + return ret; + } else { + ret = ds90ux9xx_set_de_gate(ds90ux9xx, true); + if (ret) + return ret; + } + + ret = of_property_read_u32(np, "ti,low-frequency-mode", &mode); + if (!ret) { + ret = ds90ux9xx_set_lfmode(ds90ux9xx, mode); + if (ret) + return ret; + } + + return 0; +} + +static int ds90ux9xx_config_sscg(struct ds90ux9xx *ds90ux9xx) +{ + struct device_node *np = ds90ux9xx->dev->of_node; + u32 sscg; + int ret; + + if (!ds90ux9xx_has_capability(ds90ux9xx, DS90UX9XX_CAP_SSCG)) + return 0; + + ret = of_property_read_u32(np, "ti,spread-spectrum-clock-generation", + &sscg); + if (ret) + return 0; + + return ds90ux9xx_set_sscg(ds90ux9xx, sscg); +} + +static int ds90ux9xx_config_pixel_clk_edge(struct ds90ux9xx *ds90ux9xx) +{ + struct device_node *np = ds90ux9xx->dev->of_node; + const char *pixel; + bool rising; + + if (!ds90ux9xx_has_capability(ds90ux9xx, DS90UX9XX_CAP_PIXEL_CLK_EDGE)) + return 0; + + if (of_property_read_string(np, "ti,pixel-clock-edge", &pixel)) + return 0; + + if (!strcmp(pixel, "rising")) + rising = true; + else if (!strcmp(pixel, "falling")) + rising = false; + else + return -EINVAL; + + return ds90ux9xx_set_pixel_clk_edge(ds90ux9xx, rising); +} + +static int ds90ux9xx_config_map_select(struct ds90ux9xx *ds90ux9xx) +{ + struct device_node *np = ds90ux9xx->dev->of_node; + unsigned int reg, val; + + if (!ds90ux9xx_has_capability(ds90ux9xx, DS90UX9XX_CAP_MAPSEL)) + return 0; + + if (ds90ux9xx_is_serializer(ds90ux9xx->dev)) + reg = SER_REG_CTRL_STATUS; + else + reg = DES_REG_MAPSEL; + + if (of_property_read_bool(np, "ti,video-map-select-msb")) + val = MAPSEL_SET | MAPSEL_MSB; + else if (of_property_read_bool(np, "ti,video-map-select-lsb")) + val = MAPSEL_SET; + else + val = 0; + + return regmap_update_bits(ds90ux9xx->regmap, reg, MAPSEL_MASK, val); +} + +static int ds90ux9xx_config_properties(struct ds90ux9xx *ds90ux9xx) +{ + int ret; + + ret = ds90ux9xx_config_modes(ds90ux9xx); + if (ret) + return ret; + + ret = ds90ux9xx_config_sscg(ds90ux9xx); + if (ret) + return ret; + + ret = ds90ux9xx_config_pixel_clk_edge(ds90ux9xx); + if (ret) + return ret; + + return ds90ux9xx_config_map_select(ds90ux9xx); +} + +static const struct i2c_device_id ds90ux9xx_ids[] = { + /* Supported serializers */ + { "ds90ub925q", TI_DS90UB925 }, + { "ds90uh925q", TI_DS90UH925 }, + { "ds90ub927q", TI_DS90UB927 }, + { "ds90uh927q", TI_DS90UH927 }, + + /* Supported deserializers */ + { "ds90ub926q", TI_DS90UB926 }, + { "ds90uh926q", TI_DS90UH926 }, + { "ds90ub928q", TI_DS90UB928 }, + { "ds90uh928q", TI_DS90UH928 }, + { "ds90ub940q", TI_DS90UB940 }, + { "ds90uh940q", TI_DS90UH940 }, + { }, +}; +MODULE_DEVICE_TABLE(i2c, ds90ux9xx_ids); + +static const struct of_device_id ds90ux9xx_dt_ids[] = { + { .compatible = "ti,ds90ux9xx", }, + { .compatible = "ti,ds90ub925q", }, + { .compatible = "ti,ds90uh925q", }, + { .compatible = "ti,ds90ub927q", }, + { .compatible = "ti,ds90uh927q", }, + { .compatible = "ti,ds90ub926q", }, + { .compatible = "ti,ds90uh926q", }, + { .compatible = "ti,ds90ub928q", }, + { .compatible = "ti,ds90uh928q", }, + { .compatible = "ti,ds90ub940q", }, + { .compatible = "ti,ds90uh940q", }, + { }, +}; +MODULE_DEVICE_TABLE(of, ds90ux9xx_dt_ids); + +static int ds90ux9xx_read_ic_type(struct i2c_client *client, + struct ds90ux9xx *ds90ux9xx) +{ + u8 device_id[DEVICE_ID_LEN + 1] = { 0 }; + const struct i2c_device_id *id; + const char *id_code; + unsigned int i; + int ret; + + ret = regmap_raw_read(ds90ux9xx->regmap, SER_DES_DEVICE_ID_REG, + device_id, DEVICE_ID_LEN); + if (ret < 0) { + dev_err(ds90ux9xx->dev, "Failed to read device id: %d\n", ret); + return ret; + } + + id = i2c_match_id(ds90ux9xx_ids, client); + if (id) { + id_code = ds90ux9xx_devices[id->driver_data].id; + if (!strncmp(device_id, id_code, DEVICE_ID_LEN)) { + ds90ux9xx->dev_id = id->driver_data; + return 0; + } + } + + /* DS90UH925 device quirk */ + if (!memcmp(device_id, "\0\0\0\0\0\0", DEVICE_ID_LEN)) { + if (id && id->driver_data != TI_DS90UH925) + dev_err(ds90ux9xx->dev, + "Device ID returned all zeroes, assuming device is DS90UH925\n"); + ds90ux9xx->dev_id = TI_DS90UH925; + return 0; + } + + for (i = 0; i < ARRAY_SIZE(ds90ux9xx_devices); i++) { + if (strncmp(device_id, ds90ux9xx_devices[i].id, DEVICE_ID_LEN)) + continue; + + if (id) + dev_err(ds90ux9xx->dev, + "Mismatch between probed device ID [%s] and HW device ID [%s]\n", + id_code, device_id); + + ds90ux9xx->dev_id = i; + return 0; + } + + dev_err(ds90ux9xx->dev, "Device ID [%s] is not supported\n", device_id); + + return -ENODEV; +} + +static int ds90ux9xx_probe(struct i2c_client *client) +{ + struct ds90ux9xx *ds90ux9xx; + int ret; + + ds90ux9xx = devm_kzalloc(&client->dev, sizeof(*ds90ux9xx), GFP_KERNEL); + if (!ds90ux9xx) + return -ENOMEM; + + /* Get optional GPIO connected to PDB pin */ + ds90ux9xx->power_gpio = devm_gpiod_get_optional(&client->dev, "power", + GPIOD_OUT_HIGH); + if (IS_ERR(ds90ux9xx->power_gpio)) + return PTR_ERR(ds90ux9xx->power_gpio); + + /* Give time to the controller to initialize itself after power-up */ + if (ds90ux9xx->power_gpio) + usleep_range(2000, 2500); + + ds90ux9xx->dev = &client->dev; + ds90ux9xx->regmap = devm_regmap_init_i2c(client, + &ds90ux9xx_regmap_config); + if (IS_ERR(ds90ux9xx->regmap)) + return PTR_ERR(ds90ux9xx->regmap); + + mutex_init(&ds90ux9xx->indirect); + + ret = ds90ux9xx_read_ic_type(client, ds90ux9xx); + if (ret) + return ret; + + ds90ux9xx->property = &ds90ux9xx_devices[ds90ux9xx->dev_id]; + + i2c_set_clientdata(client, ds90ux9xx); + + ret = ds90ux9xx_config_properties(ds90ux9xx); + if (ret) + return ret; + + ret = sysfs_create_groups(&ds90ux9xx->dev->kobj, ds90ux9xx_attr_groups); + if (ret) + return ret; + + ret = devm_of_platform_populate(ds90ux9xx->dev); + if (ret) + sysfs_remove_groups(&ds90ux9xx->dev->kobj, + ds90ux9xx_attr_groups); + + return ret; +} + +static int ds90ux9xx_remove(struct i2c_client *client) +{ + struct ds90ux9xx *ds90ux9xx = i2c_get_clientdata(client); + + sysfs_remove_groups(&ds90ux9xx->dev->kobj, ds90ux9xx_attr_groups); + + if (ds90ux9xx->power_gpio) + gpiod_set_value_cansleep(ds90ux9xx->power_gpio, 0); + + return 0; +} + +static struct i2c_driver ds90ux9xx_driver = { + .driver = { + .name = "ds90ux9xx", + .of_match_table = ds90ux9xx_dt_ids, + }, + .probe_new = ds90ux9xx_probe, + .remove = ds90ux9xx_remove, + .id_table = ds90ux9xx_ids, +}; +module_i2c_driver(ds90ux9xx_driver); + +MODULE_AUTHOR("Balasubramani Vivekanandan <balasubramani_vivekanandan@mentor.com>"); +MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>"); +MODULE_DESCRIPTION("TI DS90Ux9xx MFD driver"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/mfd/ds90ux9xx.h b/include/linux/mfd/ds90ux9xx.h new file mode 100644 index 000000000000..72a5928b6ad1 --- /dev/null +++ b/include/linux/mfd/ds90ux9xx.h @@ -0,0 +1,42 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * TI DS90Ux9xx MFD driver + * + * Copyright (c) 2017-2018 Mentor Graphics Inc. + */ + +#ifndef __LINUX_MFD_DS90UX9XX_H +#define __LINUX_MFD_DS90UX9XX_H + +#include <linux/types.h> + +enum ds90ux9xx_device_id { + /* Supported serializers */ + TI_DS90UB925, + TI_DS90UH925, + TI_DS90UB927, + TI_DS90UH927, + + /* Supported deserializers */ + TI_DS90UB926, + TI_DS90UH926, + TI_DS90UB928, + TI_DS90UH928, + TI_DS90UB940, + TI_DS90UH940, +}; + +struct device; + +bool ds90ux9xx_is_serializer(struct device *dev); +enum ds90ux9xx_device_id ds90ux9xx_get_ic_type(struct device *dev); +unsigned int ds90ux9xx_num_fpd_links(struct device *dev); + +int ds90ux9xx_get_link_status(struct device *dev, unsigned int *link, + bool *locked); + +int ds90ux9xx_update_bits_indirect(struct device *dev, u8 reg, u8 mask, u8 val); +int ds90ux9xx_write_indirect(struct device *dev, unsigned char reg, u8 val); +int ds90ux9xx_read_indirect(struct device *dev, u8 reg, u8 *val); + +#endif /*__LINUX_MFD_DS90UX9XX_H */