Message-Id: <48605183-78B6-461E-9476-C96C8E55A55D@collabora.com>
Date: Sat, 28 Jun 2025 10:05:11 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <lossin@...nel.org>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Danilo Krummrich <dakr@...nel.org>,
 Daniel Stone <daniels@...labora.com>,
 Rob Herring <robh@...nel.org>,
 Alice Ryhl <alice.ryhl@...gle.com>,
 Beata Michalska <beata.michalska@....com>,
 Carsten Haitzler <carsten.haitzler@...s.arm.com>,
 Boris Brezillon <boris.brezillon@...labora.com>,
 Ashley Smith <ashley.smith@...labora.com>,
 linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org,
 rust-for-linux@...r.kernel.org,
 kernel@...labora.com
Subject: Re: [PATCH] Introduce Tyr

Hi Miguel,

> On 28 Jun 2025, at 06:44, Miguel Ojeda <miguel.ojeda.sandonis@...il.com> wrote:
> 
> Hi Daniel,
> 
> Some procedural notes and general comments, and please note that some
> may apply several times.
> 
> On Sat, Jun 28, 2025 at 12:35 AM Daniel Almeida
> <daniel.almeida@...labora.com> wrote:
>> 
>> Signed-off-by: Rob Herring <robh@...nel.org>
>> 
>> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>
> 
> No newline.
> 
>> [2]: https://gitlab.freedesktop.org/panfrost/linux/-/tree/tyr?ref_type=heads
> 
> The base commit seems to be the one in this branch, but the branch is
> a custom one that is not intended to land as-is, right?
> 
> If all the patches are in the list already (like the regulator ones),
> then I would suggest putting the links to those instead. Otherwise, I
> would mark the patch as RFC, since it is not meant to be applied
> as-is.
> 
> Maybe I am just missing context, and this is all crystal clear for
> everyone else, but normally patches are supposed to be candidates to
> be applied, possibly with other dependencies, all coming from the
> list.
> 

The branch I shared is drm-misc-next plus a few dependencies, i.e.: 10 commits
total if I counted it correctly - all of which have been sent to the list
already and most of which have seen a quite a few iterations. I should have
explicitly said this, though.

Anyway, I thought that having a branch would be more tidy than listing them, as
the branch shows in what order they're applied and etc. For example, the patch
for read_poll_timeout was cherry-picked from Fujita's v12 series, and that was
subsequently dropped in v13 until the rest of the series was merged on v15. I
thought that referring to v12 of that series would be slightly confusing.

IOW: this should be appliable as soon as the dependencies themselves are
merged. I am hoping that this can happen on the 6.17 merge window as the
comments on most of them appear to be dying down so maybe the r-b's will start
coming soon. It also gives a user to read_poll_timeout(), which may prompt Fujita
to keep working on it.


>> +use core::pin::Pin
>   
> This should already be able to come from the prelude.
> 
>> +/// Convienence type alias for the DRM device type for this driver
> 
> "Convenience"

Yeah, it's a constant battle between having spelling check enabled (which on my
case flags the code itself, thereby producing a mountain of false positives) vs
not. In this case, the bad spelling won :)

Thanks for catching it, though.

> 
> Also, please end comments/docs with periods.

Right

> 
>> +unsafe impl Send for TyrData {}
>> +unsafe impl Sync for TyrData {}
> 
> Clippy should catch this (orthogonal to what Danilo mentioned).
> 
>> +use kernel::alloc::flags::*;
> 
> Prelude covers this.
> 
>> +// SAFETY:
>> +//
>> +// This type is the same type exposed by Panthor's uAPI. As it's declared as
>> +// #repr(C), we can be sure that the layout is the same. Therefore, it is safe
>> +// to expose this to userspace.
> 
> If they are not bullets, please don't add newlines, i.e. you can start
> in the same line.
> 
> Also, `#[repr(C)]`.
> 
> Regarding the safety comment, it should explain how it covers the
> requirements of `AsBytes`:
> 
>    Values of this type may not contain any uninitialized bytes. This
> type must not have interior mutability.
> 
>> +#[allow(dead_code)]
> 
> Could it be `expect`?

Hmm, I must say I did not know that this was a thing.

Why is it better than [#allow] during the development phase?

> 
>> +/// Powers on the l2 block.
>> +pub(crate) fn l2_power_on(iomem: &Devres<IoMem>) -> Result<()> {
> 
> -> Result
> 
>> +#![allow(dead_code)]
> 
> Could it be `expect`?
> 
>> +    author: "The Tyr driver authors",
> 
> Please use the `authors` key (this one is going away) -- with it now
> you could mention each author.
> 
>> +#include<uapi/drm/panthor_drm.h>
> 
> Missing space.
> 
> Cheers,
> Miguel

— Daniel

Powered by blists - more mailing lists