Message-ID: <608d23fc-eef7-c0dc-de5b-53b140fe2d0f@fi.rohmeurope.com>
Date:   Mon, 7 Feb 2022 14:38:22 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Luca Ceresoli <luca@...aceresoli.net>,
        Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
CC:     "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Wolfram Sang <wsa@...-dreams.de>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Kieran Bingham <kieran.bingham@...asonboard.com>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Vladimir Zapolskiy <vz@...ia.com>,
        Peter Rosin <peda@...ntia.se>,
        Mauro Carvalho Chehab <mchehab@...nel.org>
Subject: Re: [RFCv3 0/6] TI camera serdes and I2C address translation (Was:
 [RFCv3 0/6] Hi,)

Hi again Luca,

On 2/7/22 16:07, Luca Ceresoli wrote:
> Hi Matti,
> 
> On 07/02/22 14:21, Vaittinen, Matti wrote:
>> Hi dee Ho peeps,
>>
>> On 2/7/22 14:06, Tomi Valkeinen wrote:
>>> Hi Luca,
>>>
>>> On 06/02/2022 13:59, Luca Ceresoli wrote:
>>>> this RFCv3, codename "FOSDEM Fries", of RFC patches to support the TI
>>>> DS90UB9xx serializer/deserializer chipsets with I2C address translation.
>>
>>
>> I am not sure if I am poking in the nest of the wasps - but there's one
>> major difference with the work I've done and with Toni's / Luca's work.
> 
> You are. ;)
> 
>> The TI DES drivers (like ub960 driver) packs pretty much everything
>> under single driver at media/i2c - which (in my opinion) makes the
>> driver pretty large one.
>>
>> My approach is/was to utilize MFD - and prepare the regmap + IRQs in the
>> MFD (as is pretty usual) - and parse that much of the device-tree that
>> we see how many SER devices are there - and that I get the non I2C
>> related DES<=>SER link parameters set. After that I do kick alive the
>> separate MFD cells for ATR, pinctrl/GPIO and media.
>>
>> The ATR driver instantiates the SER I2C devices like Toni's ub960 does.
>> The SER compatible is once again matched in MFD (for SER) - which again
>> provides regmap for SER, does initial I2C writes so SER starts
>> responding to I2C reads and then kicks cells for media and pinctrl/gpio.
>>
>> I believe splitting the functionality to MFD subdevices makes drivers
>> slightly clearer. You'll get GPIOs/pinctrl under pinctrl as usual,
>> regmaps/IRQ-chips under MFD and only media/v4l2 related parts under media.
> 
> There has been quite a fiery discussion about this in the past, you can
> grab some popcorn and read
> https://lore.kernel.org/linux-media/20181008211205.2900-1-vz@mleia.com/T/#m9b01af81665ac956af3c6d57810239420c3f8cee
> 
> TL;DR: there have been strong opposition the the MFD idea.

Hm. I may be missing something but I didn't see opposition to using MFD 
or splitting the drivers. I do see opposition to adding _functionality_ 
in MFD. If I read this correctly, Lee did oppose adding the I2C stuff, 
sysfs attributes etc in MFD. Quoting his reply:

"This driver does too much real work ('stuff') to be an MFD driver.
MFD drivers should not need to care of; links, gates, modes, pixels,
frequencies maps or properties.  Nor should they contain elaborate
sysfs structures to control the aforementioned 'stuff'.

Granted, there may be some code in there which could be appropriate
for an MFD driver.  However most of it needs moving out into a
function driver (or two)."

And I tend to agree with Lee here. I would not put I2C bridge stuff or 
sysfs attributes in MFD. But I think it does not mean SERDESes should 
not use MFD when they clearly contain more IP blocks than the 
video/media ones :) I am confident Lee and others might be much more 
welcoming for driver which simply configures regmap and kicks subdriver 
for doing the ATR / I2C stuff.

I did add minimal mandatory register initializations in order to avoid 
synchronizing the sub-devices - but I hope that would be too much. 
(Synchronizing sub-devices to when the I2C reads over the link becomes 
available.)

What comes to regmap/regmap IRQ initialization in MFD - that's not 
exceptional. I think it's quite standard for MFD to prepare IRQs/regmaps 
when many sub-devices use these resources.

> I personally don't have a super strong opinion: I wrote this as a
> monolithic driver because it looked like the most natural implementation
> and found it was working fine for me, I never really explored the MFD idea.

No problem. I am definitely trying to tell you how these TI drivers must 
be done. Even I don't have the guts to do that ;D

I am simply saying that the MFD approach could be used. It does have 
certain merits if we manage to keep the MFD layer thin enough.

>> Anyways - I opened the mail client to just say that the ATR has worked
>> nicely for me and seems pretty stable - so to me it sounds like a goof
>> idea to get ATR reviewed/merged even before the drivers have been finalized.
> 
> Sounds like a... what...? A "good idea"? Or a "goofy idea"? :-D

Let me rephrase. It's greaf idea ;)

(I really meant a "good idea" :])

Best Regards
	-- Matti Vaittinen

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

Powered by blists - more mailing lists