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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 26 Jan 2023 10:41:47 +0200
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Andy Shevchenko <andriy.shevchenko@...el.com>
Cc:     linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Wolfram Sang <wsa@...nel.org>,
        Luca Ceresoli <luca.ceresoli@...tlin.com>,
        Matti Vaittinen <Matti.Vaittinen@...rohmeurope.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Peter Rosin <peda@...ntia.se>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Michael Tretter <m.tretter@...gutronix.de>,
        Shawn Tu <shawnx.tu@...el.com>,
        Hans Verkuil <hverkuil@...all.nl>,
        Mike Pagano <mpagano@...too.org>,
        Krzysztof Hałasa <khalasa@...p.pl>,
        Marek Vasut <marex@...x.de>
Subject: Re: [PATCH v7 5/7] media: i2c: add DS90UB960 driver

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