[v2,03/16] pwm: cros-ec: update documentation regarding pwm-cells

Message ID 1515766983-15151-4-git-send-email-claudiu.beznea@microchip.com
State Changes Requested, archived
Headers show
Series
  • extend PWM framework to support PWM modes
Related show

Commit Message

Claudiu Beznea Jan. 12, 2018, 2:22 p.m.
pwm-cells should be at least 2 to provide channel number and period value.

Cc: Brian Norris <briannorris@chromium.org>
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Brian Norris Jan. 12, 2018, 6:31 p.m. | #1
On Fri, Jan 12, 2018 at 04:22:50PM +0200, Claudiu Beznea wrote:
> pwm-cells should be at least 2 to provide channel number and period value.

Nacked-by: Brian Norris <briannorris@chromium.org>

We don't control the period from the kernel; only the duty cycle. (Now,
that's perhaps not a wise firmware interface, and we may fix that
someday, but you can't just declare a breaking change to a documented,
reviewed binding.)

> Cc: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
> index 472bd46ab5a4..03347fd302b5 100644
> --- a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
> @@ -8,7 +8,7 @@ Documentation/devicetree/bindings/mfd/cros-ec.txt).
>  
>  Required properties:
>  - compatible: Must contain "google,cros-ec-pwm"
> -- #pwm-cells: Should be 1. The cell specifies the PWM index.
> +- #pwm-cells: Should be 2. The cell specifies the PWM index.

Umm, "2 cells", but you use the singular "cell", and don't document what
the second one is? That's nonsense.

Brian

>  
>  Example:
>  	cros-ec@0 {
> @@ -18,6 +18,6 @@ Example:
>  
>  		cros_ec_pwm: ec-pwm {
>  			compatible = "google,cros-ec-pwm";
> -			#pwm-cells = <1>;
> +			#pwm-cells = <2>;
>  		};
>  	};
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Claudiu Beznea Jan. 15, 2018, 9:01 a.m. | #2
On 12.01.2018 20:31, Brian Norris wrote:
> On Fri, Jan 12, 2018 at 04:22:50PM +0200, Claudiu Beznea wrote:
>> pwm-cells should be at least 2 to provide channel number and period value.
> 
> Nacked-by: Brian Norris <briannorris@chromium.org>
> 
> We don't control the period from the kernel; only the duty cycle.
I agree, I saw this in the driver. This is the way I put the 0xffff
period in the patch 7 of this series. I though that since all the drivers
which uses PWM framework uses the generic PWM bindings (except pwm-pxa.c,
pwm-cros-ec.c and pwm-clps711x.c) I though it would be simpler (from the
driver's perspective and also from core's perspective) to have generic
bindings for all as follows:
pwms = <&controller PWM-channel PWM-period PWM-flags>;

To allow pwm-cross-ec.c to use this generic binding, since it is uses a
fix period and of_pwm_xlate() xlate DT arguments without taking care of
the cross-ec particularity, using 0xffff period in the pwms binding will
not harm this driver (correct me if I'm wrong). For this, the pwm-cells
argument need to be increased at 2. In patch 7 of this series I used
pwms = <&cros_ec_pwm 1 65535>;
which initialize the PWM 1 with 0xffff period.

Thanks,
Claudiu

(Now,
> that's perhaps not a wise firmware interface, and we may fix that
> someday, but you can't just declare a breaking change to a documented,
> reviewed binding.
> 
>> Cc: Brian Norris <briannorris@chromium.org>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
>> index 472bd46ab5a4..03347fd302b5 100644
>> --- a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
>> +++ b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
>> @@ -8,7 +8,7 @@ Documentation/devicetree/bindings/mfd/cros-ec.txt).
>>  
>>  Required properties:
>>  - compatible: Must contain "google,cros-ec-pwm"
>> -- #pwm-cells: Should be 1. The cell specifies the PWM index.
>> +- #pwm-cells: Should be 2. The cell specifies the PWM index.
> 
> Umm, "2 cells", but you use the singular "cell", and don't document what
> the second one is? That's nonsense.
> 
> Brian
> 
>>  
>>  Example:
>>  	cros-ec@0 {
>> @@ -18,6 +18,6 @@ Example:
>>  
>>  		cros_ec_pwm: ec-pwm {
>>  			compatible = "google,cros-ec-pwm";
>> -			#pwm-cells = <1>;
>> +			#pwm-cells = <2>;
>>  		};
>>  	};
>> -- 
>> 2.7.4
>>
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Claudiu Beznea Jan. 17, 2018, 8:29 a.m. | #3
On 15.01.2018 11:01, Claudiu Beznea wrote:
> 
> 
> On 12.01.2018 20:31, Brian Norris wrote:
>> On Fri, Jan 12, 2018 at 04:22:50PM +0200, Claudiu Beznea wrote:
>>> pwm-cells should be at least 2 to provide channel number and period value.
>>
>> Nacked-by: Brian Norris <briannorris@chromium.org>
>>
>> We don't control the period from the kernel; only the duty cycle.
> I agree, I saw this in the driver. This is the way I put the 0xffff
> period in the patch 7 of this series. I though that since all the drivers
> which uses PWM framework uses the generic PWM bindings (except pwm-pxa.c,
> pwm-cros-ec.c and pwm-clps711x.c) I though it would be simpler (from the
> driver's perspective and also from core's perspective) to have generic
> bindings for all as follows:
> pwms = <&controller PWM-channel PWM-period PWM-flags>;
> 
> To allow pwm-cross-ec.c to use this generic binding, since it is uses a
> fix period and of_pwm_xlate() xlate DT arguments without taking care of
> the cross-ec particularity, using 0xffff period in the pwms binding will
> not harm this driver (correct me if I'm wrong). For this, the pwm-cells
> argument need to be increased at 2. In patch 7 of this series I used
> pwms = <&cros_ec_pwm 1 65535>;
> which initialize the PWM 1 with 0xffff period.
> 
> Thanks,
> Claudiu
> 
> (Now,
>> that's perhaps not a wise firmware interface, and we may fix that
>> someday, but you can't just declare a breaking change to a documented,
>> reviewed binding.
>>
>>> Cc: Brian Norris <briannorris@chromium.org>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>>> ---
>>>  Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
>>> index 472bd46ab5a4..03347fd302b5 100644
>>> --- a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
>>> +++ b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
>>> @@ -8,7 +8,7 @@ Documentation/devicetree/bindings/mfd/cros-ec.txt).
>>>  
>>>  Required properties:
>>>  - compatible: Must contain "google,cros-ec-pwm"
>>> -- #pwm-cells: Should be 1. The cell specifies the PWM index.
>>> +- #pwm-cells: Should be 2. The cell specifies the PWM index.
>>
>> Umm, "2 cells", but you use the singular "cell", and don't document what
>> the second one is? That's nonsense.
I didn't saw this comment. The second cell is from the standard PWM binding
as all the other PWM drivers uses. e.g.:
pwms=<&controller PWM-channel PWM-period PWM-flags>

With these changes, if pwm-cells=1 then only PWM-channel will be parsed,
if it is 2 PWM-channel and PWM-period will be parsed, if pwm-cells=3
then PWM-channel, PWM-period and PWM-flags will be parsed.
In your driver you used to have only one cell because you wanted to allow
user to give as argument only PWM channel, and you did not want a change
of PWM period (and in of_xlate function you initialize pwm period with 0xffff
value: this is why I changed the binding in patch 7 of this series, file
rk3399-gru-kevin.dts). But e.g. sysfs could try to change the PWM period,
there is no restriction to change the PWM period from sysfs, in the sysfs
interface but the restriction is in PWM apply of the drive. The same things
happens with these changes too. The user could introduce any PWM period via
DT but the pwm apply function of the driver will return error.

Thanks,
Claudiu

>>
>> Brian
>>
>>>  
>>>  Example:
>>>  	cros-ec@0 {
>>> @@ -18,6 +18,6 @@ Example:
>>>  
>>>  		cros_ec_pwm: ec-pwm {
>>>  			compatible = "google,cros-ec-pwm";
>>> -			#pwm-cells = <1>;
>>> +			#pwm-cells = <2>;
>>>  		};
>>>  	};
>>> -- 
>>> 2.7.4
>>>
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brian Norris Jan. 17, 2018, 11:10 p.m. | #4
On Wed, Jan 17, 2018 at 10:29:53AM +0200, Claudiu Beznea wrote:
> With these changes, if pwm-cells=1 then only PWM-channel will be parsed,

I'm not sure if I'm understanding you correctly but...no. If cells is 1,
then your driver change just causes us not to parse correctly, and
everything fails.

> if it is 2 PWM-channel and PWM-period will be parsed, if pwm-cells=3
> then PWM-channel, PWM-period and PWM-flags will be parsed.
> In your driver you used to have only one cell because you wanted to allow
> user to give as argument only PWM channel, and you did not want a change
> of PWM period (and in of_xlate function you initialize pwm period with 0xffff
> value: this is why I changed the binding in patch 7 of this series, file

It's not a matter of "allow", it's a matter of description. The period
isn't actually even 0xffff, that's just a pseudo-period, to reflect that
you have a choice of duty cycles of 0 to 0xffff. I (justifiably, I
think) didn't think putting this false value in the device tree was
accurate.

> rk3399-gru-kevin.dts). But e.g. sysfs could try to change the PWM period,
> there is no restriction to change the PWM period from sysfs, in the sysfs
> interface but the restriction is in PWM apply of the drive. The same things
> happens with these changes too. The user could introduce any PWM period via
> DT but the pwm apply function of the driver will return error.

sysfs has no bearing on a device tree binding. Just because we have a
broken interface here doesn't mean we should change how we describe the
hardware.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Claudiu Beznea Jan. 18, 2018, 9:18 a.m. | #5
On 18.01.2018 01:10, Brian Norris wrote:
> On Wed, Jan 17, 2018 at 10:29:53AM +0200, Claudiu Beznea wrote:
>> With these changes, if pwm-cells=1 then only PWM-channel will be parsed,
> 
> I'm not sure if I'm understanding you correctly but...no. If cells is 1,
> then your driver change just causes us not to parse correctly, and
> everything fails.
My bad, agree with you, will fail with pwm-cells=1. I forgot about:
+	if (args->args_count < PWM_ARGS_CNT_XLATE_PERIOD ||
+	    args->args_count > PWM_ARGS_CNT_XLATE_MAX)
 		return ERR_PTR(-EINVAL);
restriction.

> 
>> if it is 2 PWM-channel and PWM-period will be parsed, if pwm-cells=3
>> then PWM-channel, PWM-period and PWM-flags will be parsed.
>> In your driver you used to have only one cell because you wanted to allow
>> user to give as argument only PWM channel, and you did not want a change
>> of PWM period (and in of_xlate function you initialize pwm period with 0xffff
>> value: this is why I changed the binding in patch 7 of this series, file
> 
> It's not a matter of "allow", it's a matter of description. The period
> isn't actually even 0xffff, that's just a pseudo-period, to reflect that
> you have a choice of duty cycles of 0 to 0xffff. I (justifiably, I
> think) didn't think putting this false value in the device tree was
> accurate.
Ok, I didn't investigate the driver to see what is truly set in HW.
> 
>> rk3399-gru-kevin.dts). But e.g. sysfs could try to change the PWM period,
>> there is no restriction to change the PWM period from sysfs, in the sysfs
>> interface but the restriction is in PWM apply of the drive. The same things
>> happens with these changes too. The user could introduce any PWM period via
>> DT but the pwm apply function of the driver will return error.
> 
> sysfs has no bearing on a device tree binding. Just because we have a
> broken interface here doesn't mean we should change how we describe the
> hardware.
> 
Based on [1] and the comments I will drop the first 7 patches of this series.

Thanks,
Claudiu 

[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/ABI.txt
> Brian
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
index 472bd46ab5a4..03347fd302b5 100644
--- a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
@@ -8,7 +8,7 @@  Documentation/devicetree/bindings/mfd/cros-ec.txt).
 
 Required properties:
 - compatible: Must contain "google,cros-ec-pwm"
-- #pwm-cells: Should be 1. The cell specifies the PWM index.
+- #pwm-cells: Should be 2. The cell specifies the PWM index.
 
 Example:
 	cros-ec@0 {
@@ -18,6 +18,6 @@  Example:
 
 		cros_ec_pwm: ec-pwm {
 			compatible = "google,cros-ec-pwm";
-			#pwm-cells = <1>;
+			#pwm-cells = <2>;
 		};
 	};