Received-SPF: pass (google.com: domain of linux-kernel+bounces-133142-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <9b2bab7a-24a7-49da-8cb8-f58e28baf065@baylibre.com>
Date: Fri, 5 Apr 2024 10:19:20 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 0/3] pwm: add support for duty_offset
To: =?UTF-8?Q?Uwe_Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Cc: linux-pwm@vger.kernel.org, linux-kernel@vger.kernel.org,
 michael.hennerich@analog.com, nuno.sa@analog.com, dlechner@baylibre.com
References: <20240405003025.739603-1-tgamblin@baylibre.com>
 <fhrderkiwqzc7fw2hptz2la3aco72wlossqbrr4m42qllcpcpw@vhxflah2ex2a>
Content-Language: en-US
From: Trevor Gamblin <tgamblin@baylibre.com>
In-Reply-To: <fhrderkiwqzc7fw2hptz2la3aco72wlossqbrr4m42qllcpcpw@vhxflah2ex2a>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 2024-04-05 08:23, Uwe Kleine-König wrote:
> Hello Trevor,
>
> In general I really like your effort to generalize the pwm framework.
> Thanks a lot!
Thanks! I'm glad that it (mostly) fits.
>
> On Thu, Apr 04, 2024 at 08:30:22PM -0400, Trevor Gamblin wrote:
>> This series extends the PWM subsystem to support the duty_offset feature
>> found on some PWM devices. It includes a patch to enable this feature
>> for the axi-pwmgen driver, which can also serve as an example of how to
>> implement it for other devices. It also contains a patch adding a new
>> pwm_config_full() function mirroring the behavior of pwm_config() but
> Please don't. pwm_config() is a function I want to get rid of in the
> long term. Consumers that want to make use of it should use
> pwm_apply_*().
Okay, I'll drop this patch.
>
>> with duty_offset included, to help maintain compatibility for drivers
>> that don't support the feature.
>>
>> The series was tested on actual hardware using a Zedboard. An
>> oscilloscope was used to validate that the generated PWM signals matched
>> the requested ones. The libpwm [1] tool was also used for testing the
>> char device functionality.
>>
>> The series is marked RFC as there are some outstanding questions about
>> implementation:
>>
>> 1. In drivers/pwm/core.c, __pwm_apply() was modified to check that the
>> sum of state->duty_offset + state->duty_cycle does not exceed
>> state->period, but in the character device section these values are
>> being checked separately. Is this intentional? What is the intended
>> behavior?
> state->duty_offset + state->duty_cycle doesn't necessarily need to be
> less or equal to state->period. Consider this waveform, where ^ marks
> the start of a period.
>
>
>          ___       _________       __...
>             \_____/         \_____/
>          ^               ^
>
> This one has duty_offset = 9 and duty_cycle = 10 while
> period = 16 < 10 + 9.
Alright, I'll make a change here.
>
>> 2. Should __pwm_apply() explicitly disallow PWM_POLARITY_INVERSED and
>> duty_offset together?
> While there is no technical need for that, a configuration with both
> PWM_POLARITY_INVERSED and duty_offset > 0 is irritating. So I'd say yes,
> it should be disallowed. When I created the cdev API I even considered
> dropping .polarity for lowlevel drivers and convert them all to
> .duty_offset.
Will do.
>
> Having said that I don't like the addition of .supports_offset to
> struct pwm_chip, which only signals a new incomplete evolution of the
> pwm framework. Better adapt all drivers and then assume all of them
> support it.
Can you clarify what you mean here - is the intent to put basic handling 
of duty_offset (even if that means simply setting it to 0) in each driver?
>
>> 3. Are there other places that would need duty_offset handling which
>> have been missed?
> I'm happy you adapted the tracing stuff. I didn't look closely, but I
> don't think something important was missed.
>
>> Note that in addition to the other patches in this series, the changes
>> to the axi-pwmgen driver rely on [2] and [3], which haven't been picked
>> up yet.
> Best regards
> Uwe
>