Re: [PATCH v1 2/4] arm64: dts: qcom: sm8550: Add JPEG encoder node

[Bryan O'Donoghue <bod@kernel.org>]

On 13/06/2026 10:24, Atanas Filipov wrote:
> Thank you for the feedback. I understand the reasoning, but I
> respectfully disagree with this approach for the following reasons.
> 
> While it is true that the JPEG encoder shares the same camera NOC and
> power domain infrastructure as CAMSS, that is a hardware topology detail
> — not a sufficient justification for imposing a software dependency. The
> driver is a fully
> self-contained V4L2 mem2mem encoder, implemented like every other JPEG
> encoder driver currently in the kernel (imx-jpeg, s5p-jpeg, mtk-jpeg,
> nxp-jpeg). None of those are sub-nodes of a parent ISP or camera
> subsystem driver.

That's a backwards understanding of the ethos of DT, which is to 
describe hardware architecture, to describe hardware, not to subscribe 
to or proscribe a particular software architecture.

Those jpeg blocks are standalone, whereas the CAMSS jpeg encoder lives 
inside of the CAMSS power-island.
> Making the JPEG encoder a sub-node of camss would introduce an
> unnecessary and artificial coupling: the JPEG encoder cannot be probed,
> built, or used independently of the CAMSS driver, even on platforms
> where CAMSS is disabled. This directly contradicts the kernel's
> principle of independent, single-purpose drivers.

- Probed true
- Built true
- Used untrue

Once probed your current driver can chug along pretty much unperturbed, 
however I don't believe that statement can hold true as more of the 
camera hardware gets enabled.
> The shared hardware resources (clocks, interconnects, IOMMU stream IDs,
> power domain) are already fully described in the device tree node and
> handled by the standard kernel frameworks — there is no functional
> reason to nest the node under camss.

Except that it is a real description of the hardware. "We can model it 
separately != we have modeled it correctly".

And at least one thing you are leaving out here is the cam noc - which 
eventually we will have to start to enable and will almost certainly 
have to be controlled by the core driver which also owns the 
power-collapse and muxes, the thing that will also program CPAs - the 
core CAMSS driver.

Perhaps we choose to model that NOC as a separate driver or perhaps we 
expose an API in CAMSS to vote, either way its an intrinsic part of the 
voltage and clocks in this block.

Either way sure we could model it as a fully separate node but, that is 
not really how/where the block lives. It lives inside of a defined CAMSS 
block, which is its own power-island.

Switching on the JPEG part of it by inference switches on the top-level 
of the island so, its not separate at all.
> For these reasons I would prefer to keep the JPEG encoder as a
> standalone platform device with its own DT node, consistent with how all
> comparable JPEG encoder drivers are structured in the kernel today.
> 
> afilipov
> 
> On 6/13/2026 2:14 AM, Bryan O'Donoghue wrote:
>> On 12/06/2026 20:44, Atanas Filipov wrote:
>>> +        qcom_jpeg_enc: jpeg-encoder@ac4e000 {
>>
>> One key bit of review feedback I gave in the previous leaked version of
>> this driver is that since the jpeg-encoder is part of the CAMSS block it
>> should be a sub-node of camss as OPE, CSIPHY and other blocks will be.
>>
>> Please take that feedback onboard in your v2.
>>
>> ---
>> bod
> 
> 

And please no top posting !

---
bod