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

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

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+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.

quoted

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.

quoted

quoted

quoted

quoted

quoted

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