Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT

[Stephen Warren <swarren@wwwdotorg.org>]

On 07/12/2013 11:24 AM, Thierry Reding wrote:
> On Fri, Jul 12, 2013 at 08:40:07AM -0600, Stephen Warren wrote:
>> On 07/12/2013 04:41 AM, Laurent Pinchart wrote:
>>> Hi Stephen,
> [...]
>>> What about splitting it in three patches that
>>> 
>>> - add the include/dt-bindings/pwm/pwm.h header, and update
>>> include/linux/pwm.h and
>>> Documentation/devicetree/bindings/pwm/pwm.txt
>>> 
>>> - update the rest of the documentation
>>> 
>>> - update the .dts files
>> 
>> I think that sounds reasonable.
> 
> Shouldn't the addition of include/dt-bindings/pwm/pwm.h be separate
> from its inclusion in include/linux/pwm.h so that it can be moved
> more easily (cherry-picked) to a separate repository?

I'm fine with that being another separate patch. However, I doubt
cherry-picking is an issue here; when the separate DT repo is created,
it seems likely that someone will simply copy the latest files from
the latest Linux kernel in order to populate the tree. cherry-picking
probably won't work because:

a) I doubt that the DT binding/header additions have always been kept
separate from kernel code changes in all of Linux's history.

b) I wouldn't be remotely surprised if the layout of the new repo was
entirely different to the kernel source tree layout, so direct
cherry-pick wouldn't work.

c) Not having a common git history would make adding a kernel remote
into the DT repo rather odd.

(b) and (c)  would at leat require some kind of git filter operation
rather than cherry-pick, and this issue could be solve in that filter
definition.

>>>>> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
>>>>> 
>>>>> enum pwm_polarity {
>>>>> 
>>>>> -	PWM_POLARITY_NORMAL, -	PWM_POLARITY_INVERSED, +
>>>>> PWM_POLARITY_NORMAL = 0, +	PWM_POLARITY_INVERSED = 1,
>>>>> 
>>>>> };
>>>> 
>>>> Rather than manually editing that to ensure the enum matches
>>>> the DT bindings header, the whole point of making a separate
>>>> <dt-bindings/...> directory was that drivers could include
>>>> the binding header files directly to avoid having to
>>>> duplicate the constant definitions. Can't <linux/pwm.h>
>>>> include <dt- bindings/pwm.h> and remove that enum?
>>> 
>>> We could do that, but we would then need to modify all drivers
>>> to replace enum_pwm_polarity with unsigned int. Thierry, what's
>>> your opinion on this ?
>> 
>> Or perhaps we could keep the enums around, but force the values
>> to match the DT constants:
>> 
>> enum pwm_polarity { PWM_POLARITY_NORMAL = PWM_POLARITY_NORMAL, 
>> PWM_POLARITY_INVERTED = PWM_POLARITY_INVERTED, };
>> 
>> (although obviously you'd need to avoid the enum and DT constants
>> having the same name).
> 
> I think I've seen stuff like the following done in a few header
> files to keep compatibility between enums and defines.
> 
> enum foo { BAR, #define BAR BAR BAZ, #define BAZ BAZ };
> 
> Which, as I understand it, won't work in this case because DTC can
> only cope with plain cpp files?

Yeah, dtc doesn't understand "enum", so you can't include an enum
definition in a DT file. You have to share simple #define headers
between DT and kernel code.

>> Although this brings up one point: let's say we support ACPI/..
>> bindings in the future. The enum possibly can't match the binding
>> values from every different kind of binding definition (DT, ACPI,
>> ...) so perhaps rather than changing the enum definition in
>> <linux/pwm.h>, what we should be doing is mapping between the
>> different name-spaces in whatever of_xlate function exists for
>> the PWM flags cell. That would be more flexible.
> 
> I'm not quite sure what exactly you are suggesting here. Can you 
> elaborate?

Suppose ACPI (or whatever else) starts representing PWM devices.
Suppose the author isn't aware that DT exists, represents PWM devices
and/or has already defined some PWM-related flags. So, ACPI picks bit
5 in some data value to represent inverted, rather than bit 0. Then,
there is no way that all of [ (a) DT binding PWM flags (b) ACPI PWM
flags (c) Linux's enum foo ] can use the same values. Hence, some
mapping is required between them.

The typical way to do this is to define an "of_xlate" function which
converts from DT cell values to Linux-internal representation.
Presumably if we added an ACPI parser, there'd be some equivalent for
that.

So, what I'm arguing for is that of_pwm_simple_xlate() (or any other
custom xlate function) should include both <dt-bindings/pwm/pwm.h> and
<linux/pwm.h>, and contain explicit code to convert between the two
name-spaces of flags definitions. Since those two name-spaces
currently are 100% identical, presumably if the code is written in the
right way, the compiler will actually just optimize it all away...