linux-kernel - Re: [PATCH 3/5] media: dt-bindings: Add Apple ISP

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]

Message-ID: <28a714b7-f71f-4c9e-8365-4592bbcbb7b9@kernel.org>
Date: Sun, 23 Feb 2025 09:58:06 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Sasha Finkelstein <fnkl.kernel@...il.com>
Cc: Sven Peter <sven@...npeter.dev>, Alyssa Rosenzweig
 <alyssa@...enzweig.io>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Hector Martin <marcan@...can.st>,
 Ulf Hansson <ulf.hansson@...aro.org>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, asahi@...ts.linux.dev,
 linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
 linux-media@...r.kernel.org, imx@...ts.linux.dev
Subject: Re: [PATCH 3/5] media: dt-bindings: Add Apple ISP

On 19/02/2025 10:54, Sasha Finkelstein wrote:
> On Wed, 19 Feb 2025 at 10:37, Krzysztof Kozlowski <krzk@...nel.org> wrote:
>>> +
>>> +  apple,platform-id:
>>> +    description: Platform id for firmware
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>
>>
>> No, use firmware-name.
> 
> Not sure how is firmware-name an appropriate field, fw-name is a string
> that references a firmware file, while this field is an id that is sent to the
> coprocessor firmware in order to identify the platform.

You get advice how you explain the hardware. Why do you need to identify
the platform? Compatible already tells that. Who identifies the
platform? For what purpose?


> 
>>> +  apple,temporal-filter:
>>> +    description: Whether temporal filter should be enabled in firmware
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>
>> And why is this not enabled always? Why this is board specific?
> 
> Not every board has support for this feature.

Board for the same SoC, right?  It's fine then, but you should explain
all this in binding description.


> 
>> You miss here ports or port. ISP usually gets signal from some camera or
>> other block.
> 
> For complex cameras - yes, but this is closer to a UVC camera connected
> via a bespoke protocol. We do not need to deal with the sensor access,
> all of it is managed by the coprocessor firmware.

And there is nothing more in this pipeline, like some other processing
units? It's fine then, but you should explain all this in binding
description.

> 
>>> +        properties:
>>> +          apple,config-index:
>>> +            description: Firmware config index
>>> +            $ref: /schemas/types.yaml#/definitions/uint32
>>
>>
>> No duplicated indices. You have reg for this, assuming this is index.
> 
> There are duplicated indices, see isp-imx248.dtsi in patch 5 for an example.

I still do no understand their point and description does not help me much.

> 
>> All these do not look like hardware properties but rather configuration
>> of sensor which should be done runtime by OS, not by DT.
> 
> Those are board-specific and not discoverable via the ISP protocol.

No, I rather suggest these should be runtime or deduced from the
compatible of the sensor. You know embed the sensor characteristics into
DT... or if not sensor then some user choice?


Best regards,
Krzysztof