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@xxxxxxxxxx>
> >>>>>>
> >>>>>> 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@xxxxxxxxxx>
> >>>>>> ---
> >>>>>>
> >>>>>> 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.
--
Regards,
Laurent Pinchart
