Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

From: Wolfram Sang <wsa@the-dreams.de>
To: Luca Ceresoli <luca@lucaceresoli.net>
Cc: linux-media@vger.kernel.org, linux-i2c@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	Jacopo Mondi <jacopo@jmondi.org>,
	Vladimir Zapolskiy <vz@mleia.com>, Peter Rosin <peda@axentia.se>
Subject: Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer
Date: Tue, 3 Sep 2019 16:16:40 +0200	[thread overview]
Message-ID: <20190903141640.GA6718@kunai> (raw)
In-Reply-To: <f3640020-0291-f837-8639-c3ba39800ead@lucaceresoli.net>

[-- Attachment #1: Type: text/plain, Size: 2742 bytes --]

Hi Luca,

> Now I got what you mean: the aliases in a pool are reserved to an ATR
> and not available to another ATR. What about hoisting the pool one level
> up in DT, to the adapter where the ATRs are connected?

Frankly, it sounds error prone to keep them in sync.

> As I understand it, we are referring to the same use case:
> 
> .---------------.       .-----.   .-------------------.
> | adapter (SoC) |---+---| ATR |---| remote slave 0x10 |
> '---------------'   |   '-----'   '-------------------'
>                     |
>               .----------.
>               | device X |
>               '----------'
> 
> Here device X with hard-coded address 0x20 is plugged in at runtime.

Yes. For the generic case, it doesn't even need to be plugged at
runtime. A baseboard can have various additional modules with various
requirements. Hot-plugging (not a favourite topic for I2C) adds to it.

> As you say it, if 0x20 is in the ATR pool we have a problem.

Right.

> But even without an explicit pool, initially 0x20 is unused and the ATR
> can use it for remote slave 0x10. Then at some point device X is
> connected, and suddenly 0x20 conflicts.

I haven't considered hot-plugging so far, but you are right here.

> To a limited extend the explicit pool could help if the list of possible
> addons is known: one can put in the pool only address that will never
> appear in addons.

And this is why I kinda accepted the all-unoccupied-addresses-are-free
approach. For a generic solution, the list of possible add-ons is
unknown and we should expect "the worst".

> Hey, wait, the no-pool solution could have a way to handle this
> conflict. When device X is connected, the adapter can look for a new
> alias (0x21), call the i2c_atr_deconfigure_hw() and i2c_atr_setup_hw()
> callbacks to stop using 0x20 and start using 0x21. Doesn't look very
> lovely, but may be worth considering.

Thanks for also thinking about the other approach! Everything combining
I2C and hot-plugging is likely to not be lovely, so this may be OK given
it really works in practice.

Another note: I was hoping we could keep the number of callbacks to one.
E.g. if i2c_atr_setup_hw() has the same values for 'address' and 'alias'
(or one of them is zero), then we unregister an alias, otherwise we
register. Not sure if this will work out in the end, but only one
callback is what I'd like to have. (It could be put into the
i2c_algorithm struct maybe)

> Definitely.  And having a list of use cases would help a lot IMO. I had
> never considered the use case described above, for example.

Yes. I hope to see some new faces in the room giving us their cases.

Kind regards,

   Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]