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?
> >>> 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?
> 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?
Thierry
Attachment:
signature.asc
Description: Digital signature
