From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Subject: Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT Date: Fri, 12 Jul 2013 11:40:44 -0600 Message-ID: <51E03F9C.2060903@wwwdotorg.org> References: <1373553468-6564-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <1373553468-6564-3-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <51DEEE15.7040801@wwwdotorg.org> <3211864.2dENa1E110@avalon> <51E01547.4030204@wwwdotorg.org> <20130712172441.GB9620@dhcp-172-17-186-34.nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130712172441.GB9620@dhcp-172-17-186-34.nvidia.com> Sender: linux-omap-owner@vger.kernel.org To: Thierry Reding Cc: Laurent Pinchart , Laurent Pinchart , linux-pwm@vger.kernel.org, linux-omap@vger.kernel.org, Philip Avinash , Boris BREZILLON , Steffen Trumtrar , devicetree-discuss@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.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 >>>> directory was that drivers could include >>>> the binding header files directly to avoid having to >>>> duplicate the constant definitions. Can't >>>> include 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 >> , 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 and , 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...