diff mbox series

[4/7] mfd: ds90ux9xx: add TI DS90Ux9xx de-/serializer MFD driver

Message ID 20181008211205.2900-5-vz@mleia.com
State New
Headers show
Series mfd/pinctrl: add initial support of TI DS90Ux9xx ICs | expand

Commit Message

Vladimir Zapolskiy Oct. 8, 2018, 9:12 p.m. UTC 
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

Comments

kernel test robot Oct. 9, 2018, 4:08 a.m. UTC | #1 
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
Vladimir Zapolskiy Oct. 9, 2018, 11:14 a.m. UTC | #2 
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
Lee Jones Oct. 12, 2018, 6:03 a.m. UTC | #3 
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?
Vladimir Zapolskiy Oct. 12, 2018, 7:41 a.m. UTC | #4 
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
Lee Jones Oct. 12, 2018, 8:39 a.m. UTC | #5 
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?
Kieran Bingham Oct. 12, 2018, 9:20 a.m. UTC | #6 
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?
Vladimir Zapolskiy Oct. 12, 2018, 10:58 a.m. UTC | #7 
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?
>
Vladimir Zapolskiy Oct. 12, 2018, 11:24 a.m. UTC | #8 
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
Lee Jones Oct. 12, 2018, 11:34 a.m. UTC | #9 
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?
Lee Jones Oct. 12, 2018, 11:43 a.m. UTC | #10 
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?
Kieran Bingham Oct. 12, 2018, 11:47 a.m. UTC | #11 
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?
>>
Laurent Pinchart Oct. 12, 2018, 1:01 p.m. UTC | #12 
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.
Laurent Pinchart Oct. 12, 2018, 1:07 p.m. UTC | #13 
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?
Vladimir Zapolskiy Oct. 12, 2018, 1:59 p.m. UTC | #14 
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
Vladimir Zapolskiy Oct. 12, 2018, 2:13 p.m. UTC | #15 
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
Vladimir Zapolskiy Oct. 12, 2018, 2:23 p.m. UTC | #16 
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
Lee Jones Oct. 12, 2018, 2:25 p.m. UTC | #17 
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?
Vladimir Zapolskiy Oct. 13, 2018, 12:33 p.m. UTC | #18 
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
Vladimir Zapolskiy Oct. 13, 2018, 3:10 p.m. UTC | #19 
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
Laurent Pinchart Oct. 16, 2018, 1:08 p.m. UTC | #20 
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.
Laurent Pinchart Oct. 16, 2018, 1:12 p.m. UTC | #21 
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.
Vladimir Zapolskiy Oct. 16, 2018, 6:32 p.m. UTC | #22 
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
Vladimir Zapolskiy Oct. 23, 2018, 8:16 a.m. UTC | #23 
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
Luca Ceresoli Oct. 30, 2018, 4:44 p.m. UTC | #24 
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 mbox series

Patch

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 */