Re: [PATCH 4/9] drivers/pwm: Add helper to configure pwm using clock divisor and duty percent

From: Shobhit Kumar <shobhit.kumar@linux.intel.com>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Alexandre Courbot <gnurou@gmail.com>,
	Samuel Ortiz <sameo@linux.intel.com>,
	Jani Nikula <jani.nikula@intel.com>,
	Shobhit Kumar <shobhit.kumar@intel.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH 4/9] drivers/pwm: Add helper to configure pwm using clock divisor and duty percent
Date: Mon, 13 Apr 2015 13:32:31 +0530	[thread overview]
Message-ID: <552B7817.7090208@linux.intel.com> (raw)
In-Reply-To: <20150410082954.GA4139@ulmo.nvidia.com>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/10/2015 01:59 PM, Thierry Reding wrote:
> On Wed, Apr 01, 2015 at 11:58:50AM +0530, Shobhit Kumar wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>> 
>> On 03/24/2015 01:53 PM, Thierry Reding wrote:
>>> On Fri, Mar 13, 2015 at 07:28:02PM +0530, Shobhit Kumar wrote:
>>>> Some chips instead of using period_ns and duty_ns can be 
>>>> configured using the clock divisor and duty percent. Adds an 
>>>> alternative configuration method for such chips
>>> 
>>> I don't see a need to introduce this alternative configuration
>>>  mechanism. Most, of not all, of the other drivers program a
>>> clock divisor and some percentage of the duty cycle as well and
>>> it should be easy to convert to that internally from the period
>>> and duty_cycle parameters that you get in ->config().
>> 
>> Perhaps. Probably I misunderstood but as per
>> Documentation/pwm.txt, it is suggested that rather than
>> calculating in the driver, we can add additional helpers. So I
>> tried doing just that. And it also means that the consumer(which
>> is directly aware of the percent it wants) has to do the
>> calculation and pass as ns values and we internally again convert
>> back to percentage ?
> 
> Yes. The interface assumes that you'll pass in absolute values for
> the period and duty cycle. Existing drivers, such as pwm-backlight,
> already convert a percentage or other internal representation to
> these absolute values. If your driver internally works with percent
> you can easily convert to that from the absolute values.
> 
> The documentation only makes a suggestion. I think it'd be fine if
> you kept this conversion internal to the driver. We can turn it
> into a more generic helper if a second driver appears that needs
> the same conversion.

Okay, will change driver implementation and avoid this patch

> 
>>> Adding an alternative means of configuring the PWM also means
>>> that every user driver now potentially needs to support both
>>> the traditional and the alternative way because PWM providers
>>> may not implement both.
>> 
>> I just assumed either or implementation should suffice. Even in
>> my implementation the error checks assumes either of the two
>> should be available else to fail the pwmchip_add
> 
> Your implementation requires that users call either pwm_config()
> or pwm_config_alternate(). PWM drivers may only have to implement
> either callback, but users will be required to support both (or
> otherwise only work with a subset of PWM drivers).

Yeah, I overlooked this. Will push a new patch for the driver.

Regards
Shobhit

> 
> Thierry
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJVK3gXAAoJEHuQFv2//5KqRhIIAMKvvSuJ3yyiPrBULOk6PyZg
AyNpICHg/pwhnjdns45eui1YnWb6Hrasbs5+UZRxlUAubs/+CDa1ZvGvtAZZQCO0
g8YO0EiafdGUg8KMif2qblJZf0oJFWs1j8sUQaarA7Uh2/1m4elvijQ39J30yzCt
+4N2JQ3Nazx2KWS5P8Wo9i2Km733vz7p8nY5lqXlstHer1x4QoaCz6utNPMgcUE+
N5wCUpOzEzqM4Lle63R2UO/uCfC+169Q+bZ2r9a1UxSeLhA+fhkZWgusaUeqi1UL
kIy4YSyelTNYIBa8dufp+IQL1w2cSbZ9JoPj7Zc7agTYqbhOuLhbM1wC9DWMWW0=
=1wV2
-----END PGP SIGNATURE-----
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx