Re: [PATCH v7 5/7] media: i2c: add DS90UB960 driver

From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
To: Andy Shevchenko <andriy.shevchenko@intel.com>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Wolfram Sang" <wsa@kernel.org>,
	"Luca Ceresoli" <luca.ceresoli@bootlin.com>,
	"Matti Vaittinen" <Matti.Vaittinen@fi.rohmeurope.com>,
	"Laurent Pinchart" <laurent.pinchart+renesas@ideasonboard.com>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Peter Rosin" <peda@axentia.se>,
	"Liam Girdwood" <lgirdwood@gmail.com>,
	"Mark Brown" <broonie@kernel.org>,
	"Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Michael Tretter" <m.tretter@pengutronix.de>,
	"Shawn Tu" <shawnx.tu@intel.com>,
	"Hans Verkuil" <hverkuil@xs4all.nl>,
	"Mike Pagano" <mpagano@gentoo.org>,
	"Krzysztof Hałasa" <khalasa@piap.pl>,
	"Marek Vasut" <marex@denx.de>
Subject: Re: [PATCH v7 5/7] media: i2c: add DS90UB960 driver
Date: Thu, 26 Jan 2023 10:41:47 +0200	[thread overview]
Message-ID: <04a82b08-524f-8d03-ac47-73d826907fc3@ideasonboard.com> (raw)
In-Reply-To: <Y9FKcoVlgUWR4rhn@smile.fi.intel.com>

On 25/01/2023 17:27, Andy Shevchenko wrote:

>>>>>>>>>> +struct ds90ub9xx_platform_data {
>>>>>>>>>> +	u32 port;
>>>>>>>>>> +	struct i2c_atr *atr;
>>>>>>>>>> +	unsigned long bc_rate;
>>>>>>>>>
>>>>>>>>> Not sure why we need this to be public except, probably, atr...
>>>>>>>>
>>>>>>>> The port and atr are used by the serializers, for atr. The bc_rate is used
>>>>>>>> by the serializers to figure out the clocking (they may use the FPD-Link's
>>>>>>>> frequency internally).
>>>>>>>
>>>>>>> The plain numbers can be passed as device properties. That's why the question
>>>>>>> about platform data. Platform data in general is discouraged to be used in a
>>>>>>> new code.
>>>>>>
>>>>>> Device properties, as in, coming from DT?
>>>>>
>>>>>    From anywhere.
>>>>>
>>>>>> The port could be in the DT, but
>>>>>> the others are not hardware properties.
>>>>>
>>>>> Why do we need them? For example, bc_rate.
>>>>
>>>> The atr pointer is needed so that the serializers (ub913, ub953) can add
>>>> their i2c adapter to the deserializer's i2c-atr. The port is also needed for
>>>> that.
>>>>
>>>> The bc rate (back-channel rate) is the FPD-Link back-channel rate which the
>>>> serializers use for various functionalities. At the moment only the ub953
>>>> uses it for calculating an output clock rate.
>>>>
>>>> The bc-rate could be implemented using the clock framework, even if it's not
>>>> quite a plain clock. I had that code at some point, but it felt a bit off
>>>> and as we needed the pdata for the ATR, I added the bc-rate there.
>>>
>>> And I don't see why it is not a property of the device.
>>
>> It with a "property of the device" you mean a hardware property, it's not
>> because we don't know it, it can be changed at runtime. It's not supposed to
>> change after probing the serializer, but up to that point it can change.
> 
> Yes, which is still property of the device, isn't it?

No, I don't see it as a property of the serializer device.

The deserializer sends messages to the serializer over the back-channel. 
The rate of the back-channel is defined by the clock used for 
deserializer's refclock, and internal deserializer configuration. The 
serializer may use the back-channel rate for its own operations.

>>>>>> Yes, I don't like using platform data. We need some way to pass information
>>>>>> between the drivers.
>>>>>
>>>>> Device properties allow that and targeting to remove the legacy platform data
>>>>> in zillions of the drivers.
>>>>
>>>> Do you have any pointers to guide me into the right direction? I couldn't
>>>> find anything with some grepping and googling.
>>>>
>>>> If you mean "device properties" as in ACPI, and so similar to DT properties,
>>>> aren't those hardware properties? Only the port here is about the hardware.
>>>
>>> About hardware, or PCB, or as quirks for missing DT/ACPI/any FW properties,
>>> like clock rates.
>>>
>>> The Linux kernel layer for that is called software nodes. The rough
>>> approximation to see where and how it's being used can be achieved
>>> by grepping for specific macros:
>>>
>>> 	git grep -lw PROPERTY_ENTRY_.*
>>>
>>> E.g. arch/arm/mach-tegra/board-paz00.c tegra_paz00_wifikill_init()
>>> implementation.
>>
>> Thanks, I'll have a look. But I presume we can only pass "plain" values, so
>> it won't work for the ATR pointer anyway.
> 
> Yes, that's what I have told at the very beginning when answering to your
> patch.
> 
> But I probably don't understand the ATR structure and what exactly we need to
> pass to it, perhaps it also can be replaced with properties (note, that we have
> some interesting ones that called references, which is an alternative to DT
> phandle).

Well, maybe this needs a Linux bus implementation. I'm not that familiar 
with implementing a bus, but I think that would make it easier to share 
data between the deserializer and the serializer. A bus sounds a bit 
like an overkill for a 1-to-1 connection, used by a few drivers, but 
maybe it wouldn't be too much code.

  Tomi