Hi Miguel,
> On 28 Jun 2025, at 06:44, Miguel Ojeda <miguel.ojeda.sando...@gmail.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.alme...@collabora.com> wrote:
>>
>> Signed-off-by: Rob Herring <r...@kernel.org>
>>
>> Signed-off-by: Daniel Almeida <daniel.alme...@collabora.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
