Message-ID: <425fe73f-8cb2-4238-9e15-55403ed9daf6@samsung.com>
Date: Tue, 1 Jul 2025 10:52:23 +0200
From: Michal Wilczynski <m.wilczynski@...sung.com>
To: Uwe Kleine-König <ukleinek@...nel.org>
Cc: 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 v5 1/9] rust: pwm: Add Kconfig and basic data structures



On 6/29/25 11:23, Uwe Kleine-König wrote:
> Hello Michal,
> 
> On Sat, Jun 28, 2025 at 04:38:15PM +0200, Michal Wilczynski wrote:
>> On 6/27/25 17:10, Uwe Kleine-König wrote:
>>> On Mon, Jun 23, 2025 at 08:08:49PM +0200, Michal Wilczynski wrote:
>>>> +/// From C: `#define WFHWSIZE 20`
>>>> +pub const WFHW_MAX_SIZE: usize = 20;
>>>
>>> Can we somehow enforce that this doesn't diverge if the C define is
>>> increased?
>>
>> You are absolutely right. The hardcoded value is a maintenance risk. The
>> #define is in core.c, so bindgen cannot see it.
>>
>> I can address this by submitting a patch to move the #define WFHWSIZE to
>> include/linux/pwm.h. This will make it part of the public API, allow
>> bindgen to generate a binding for it, and ensure the Rust code can never
>> diverge. Is this fine ?
> 
> I wonder if that is the opportunity to create a file
> include/linux/pwm-provider.h. In that file we could collect all the bits
> that are only relevant for drivers (pwm_ops, pwm_chip, pwmchip_parent,
> pwmchip_alloc ...). (Some inline functions depend on some of these, so
> some might have to stay in pwm.h)
> 
> I can address that in parallel, don't add this quest to your series. So
> yes, move WFHWSIZE to include/linux/pwm.h (and rename it to PWM_WFHWSIZE
> to not pollute the global namespace).
>  
>>> Please don't expose these non-atomic callbacks. pwm_disable() would be
>>> fine.
>>>
>>> Otherwise I'd prefer if pwm_set_waveform_might_sleep() is the API
>>> exposed to/by Rust.
>>
>>
>> OK, I'll remove all the setters from the State, while will keep the
>> getters, as they would be useful in apply callbacks.
> 
> How so? They might be useful for consumers, but my preferred idiom for
> them is that they know at each point in time what they want completely
> and don't make that dependant on the previou setting.

Oh, this is not just to check the previous state, let me bring my
implementation of apply from the v1 of the series:

impl pwm::PwmOps for Th1520PwmChipData {
    // This driver implements get_state
   fn apply(
        pwm_chip_ref: &mut pwm::Chip,
        pwm_dev: &mut pwm::Device,
        target_state: &pwm::State,
    ) -> Result {
        let data: &Th1520PwmChipData = pwm_chip_ref.get_drvdata().ok_or(EINVAL)?;
        let hwpwm = pwm_dev.hwpwm();

        if !target_state.enabled() {
            if pwm_dev.state().enabled() {
                data._disable(hwpwm)?;
            }

            return Ok(());
        }

        // Configure period, duty, and polarity.
        // This function also latches period/duty with CFG_UPDATE.
        // It returns the control value that was written with CFG_UPDATE set.
        let ctrl_val_after_config = data._config(
            hwpwm,
            target_state.duty_cycle(),
            target_state.period(),
            target_state.polarity(),
        )?;

        // Enable by setting START bit if it wasn't enabled before this apply call
        if !pwm_dev.state().enabled() {
            data._enable(hwpwm, ctrl_val_after_config)?;
        }

        Ok(())
    }
}

So the target state values are also accessed by those getters, not just
previous state.

> 
>> Will implement additional functions for Device i.e set_waveform,
>> round_waveform and get_waveform, and the new enum to expose the result
>> of the round_waveform more idiomatically.
>>
>> /// Describes the outcome of a `round_waveform` operation.
>> #[derive(Debug, Clone, Copy, PartialEq, Eq)]
>> pub enum RoundingOutcome {
>>     /// The requested waveform was achievable exactly or by rounding values down.
>>     ExactOrRoundedDown,
>>
>>     /// The requested waveform could only be achieved by rounding up.
>>     RoundedUp,
>> }
> 
> Sounds good. Hoever I have some doubts about the C semantic here, too.
> Is it really helpful to provide that info? A user of that return value
> has to check anyhow which parameter got rounded up. If you have an
> opinion here, please share.

FWIW; In my opinion, it is helpful.

The 1 (rounded up) vs. 0 (rounded down/exact) return value provides a
simple summary flag for the most common validation case: ensuring a
strict requirement, like a minimum frequency, is not violated by
rounding.

>  
> Best regards
> Uwe

Best regards,
-- 
Michal Wilczynski <m.wilczynski@...sung.com>

Powered by blists - more mailing lists