Message-ID: <CANiq72mW2ZsBh8WxY26Knh=FxxPHWK_ikbS9iZip2qRLJAXiNg@mail.gmail.com>
Date: Wed, 11 Jun 2025 15:35:04 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Michal Wilczynski <m.wilczynski@...sung.com>
Cc: Uwe Kleine-König <ukleinek@...nel.org>, 
	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>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, 
	Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, Drew Fustini <drew@...7.com>, 
	Guo Ren <guoren@...nel.org>, Fu Wei <wefu@...hat.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>, 
	Marek Szyprowski <m.szyprowski@...sung.com>, Benno Lossin <lossin@...nel.org>, 
	Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-pwm@...r.kernel.org, rust-for-linux@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org, 
	linux-clk@...r.kernel.org
Subject: Re: [PATCH v2 1/7] rust: Add basic PWM abstractions

Hi Michal,

Some docs-only/nits quick review for future versions and other
patches. Some may apply multiple times.

On Tue, Jun 10, 2025 at 2:54 PM Michal Wilczynski
<m.wilczynski@...sung.com> wrote:
>
> +//! This module provides safe Rust abstractions for working with the Linux
> +//! kernel's PWM subsystem, leveraging types generated by `bindgen`
> +//! from `<linux/pwm.h>` and `drivers/pwm/core.c`.

The second part we typically do with a "C header" reference to
`srctree/...` which will get rendered as clickable link to the file
(please check other files to see how it is usually done).

I would also simplify, e.g. typically abstractions try to be safe, and
the bindings typically come from `bindgen`, so I would just say e.g.

    //! PWM subsystem abstractions.
    //!
    //! C header: ...

What types are you using from `drivers/pwm/core.c`, by the way?

> +use crate::{
> +    bindings,
> +    device::{self, Bound},
> +    error::{self, to_result, Result},
> +    prelude::*,
> +    str::CStr,
> +    types::{ARef, AlwaysRefCounted, ForeignOwnable, Opaque},
> +};

At least a couple of these already come from the prelude.

> +/// Maximum size for the hardware-specific waveform representation buffer.
> +/// From C: #define WFHWSIZE 20

This would be rendered as a single paragraph, so I would split it into
two by adding a newline in between.

In addition, please use code spans (i.e. backquotes) where possible.

> +/// PWM polarity. Mirrors `enum pwm_polarity`.

We don't link consistently C types, but if you wanted, you could link
them (either `srctree/` link to a file or a docs.kernel.org to a
concrete C item if it is rendered there -- either is fine).

(Eventually we want to have an automatic way to do so, similar to
intra-doc links)

> +    /// Normal polarity (duty cycle defines the high period of the signal)

Please end sentences/docs with a period.

> +    Normal,
> +    /// Inversed polarity (duty cycle defines the low period of the signal)

I suggest a newline between these.

> +impl From<bindings::pwm_polarity> for Polarity {
> +    fn from(polarity: bindings::pwm_polarity) -> Self {
> +        match polarity {
> +            bindings::pwm_polarity_PWM_POLARITY_NORMAL => Polarity::Normal,
> +            bindings::pwm_polarity_PWM_POLARITY_INVERSED => Polarity::Inversed,
> +            _ => Polarity::Normal,
> +        }
> +    }
> +}

Should this be fallible? i.e. should the default case be an error / is
the C enum expected to have any other value ?

(I have no context, so this may be all expected, of course)

> +/// Represents a PWM waveform configuration. Mirrors struct pwm_waveform.

Code span.

> +    /// Duration the PWM signal is in its "active" state during one period,
> +    /// in nanoseconds. For a typical "normal" polarity configuration where active is high,
> +    /// this represents the high time of the signal.
> +    pub duty_length_ns: u64,

The first paragraph of an item is the "short description", i.e. it
acts as a title when it gets rendered in item lists. So it should be
short if possible. For instance, here I would add a new paragraph
between the two sentences.

(Ditto for other cases).

> +    /// This type must be `Copy`, `Default`, and fit within `WFHW_MAX_SIZE`.

Please use intra-doc links wherever possible, e.g. [`Copy`] and
[`WFHW_MAX_SIZE`] (assuming they work).

> +    /// # Safety
> +    /// C-callback. Pointers from C must be valid.

Please add a newline between these to match the usual style. Also,
"C-callback" is not a precondition, so I would move it outside the
safety section (above), unless you want to restrict C to be the only
one calling this or things like that (in which case I would clarify).

> +    unsafe extern "C" fn read_waveform_callback(
> +        c: *mut bindings::pwm_chip,
> +        p: *mut bindings::pwm_device,
> +        wh: *mut core::ffi::c_void,

Please avoid `core::ffi::` -- nowadays you should be able to just
write e.g. `c_void`, and that will get you the `kernel::ffi::` one
(https://docs.kernel.org/rust/coding-guidelines.html#c-ffi-types).

> +    ) -> i32 {

Unless the C side uses an explicitly `s32`, which as far as I can see
it doesn't in this case, please use `c_int` instead (i.e. please match
the C signatures in callbacks, even if they happen to resolve to that
type).

Finally, in another patch I noticed the `author` key -- we are trying
to move to the `authors` (plural) one, so please use that one.

Thanks!

Cheers,
Miguel

Powered by blists - more mailing lists