From: Alex Courbot <acourbot@nvidia.com> To: Stephen Warren <swarren@wwwdotorg.org> Cc: Stephen Warren <swarren@nvidia.com>, Thierry Reding <thierry.reding@avionic-design.de>, Simon Glass <sjg@chromium.org>, Grant Likely <grant.likely@secretlab.ca>, Rob Herring <rob.herring@calxeda.com>, Mark Brown <broonie@opensource.wolfsonmicro.com>, Anton Vorontsov <cbou@mail.ru>, David Woodhouse <dwmw2@infradead.org>, Arnd Bergmann <arnd@arndb.de>, Leela Krishna Amudala <leelakrishna.a@gmail.com>, "linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-fbdev@vger.kernel.org" <linux-fbdev@vger.kernel.org>, "devicetree-discuss@lists.ozlabs.org" <devicetree-discuss@lists.ozlabs.org>, "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>, "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org> Subject: Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences Date: Fri, 7 Sep 2012 17:21:52 +0900 [thread overview] Message-ID: <1380082.Ud3NsnIqpg@percival> (raw) In-Reply-To: <504789B1.50205@wwwdotorg.org> Hi Stephen, Skipping the typos and rephrasing issues (which will all be addressed, thanks!), these issues caught my attention more particularly: On Thursday 06 September 2012 01:19:45 Stephen Warren wrote: > > +"regulator" type required properties: > > + - id: name of the regulator to use. Regulator is obtained by > > + regulator_get(dev, id) > > + - enable / disable: one of these two empty properties must be present > > to > > + enable or disable the resource > > + > > +"pwm" type required properties: > > + - id: name of the PWM to use. PWM is obtained by pwm_get(dev, id) > > + - enable / disable: one of these two empty properties must be present > > to > > + enable or disable the resource > > For those two, would "name" be a better property name than "id"? IIRC "name" is a reserved property name in the DT (I remember seeing an error message when compiling nodes with a "name" property that did not match the node's name). "id" was the second best candidate in my mind. > > +"gpio" type required properties: > > + - number: phandle of the GPIO to use. > > Naming the property "gpio" would seem more consistent with our GPIO > properties. Ok, although "gpio.gpio" might look strange in the platform data. > > + - enable / disable: one of these two empty properties must be present > > to > > + enable or disable the resource > > You can't really enable or disable a GPIO (well, perhaps you can, but > I'd consider that to affect tri-state rather than value); it's more that > you're setting the output value to 0 or 1. I think a "value" or > "set-value" property with value <0> or <1> would be better. Right - will fix that. > Overall, the general structure of the bindings looks reasonable to me. Great, maybe we are finally headed to something stable! > I'm not sure what "pdata" is supposed to point at; platform data applies > to the original "struct device", not one of the resources used by the > power sequences. Right - more on this later. > Hmmm. It'd be nice not to need separate functions for the non-DT and DT > cases. That would require that devm_power_seq_set_build() be able to > find the power sequence definitions somewhere other than platform data > in the non-DT case - that's exactly why the regulator and pinctrl > subsystems represent the device<->data mapping table separately from the > device's platform data. Oh, that sounds better indeed - I was still mentally stuck in the previous scheme where power sequences were not accessible from a fixed node of the device. Thanks. > > +++ b/drivers/power/power_seq/Kconfig > > > > +config POWER_SEQ > > + bool > > Some kind of help text might be useful? As this option was not user-visible but automatically enabled by drivers, I did not judge it to be necessary - but after reading your other mail we might need to make it visible and documented after all. > > +/** > > + * struct platform_power_seq_step - platform data for power sequences > > steps + * @type: The type of this step. This decides which member of the > > union is + * valid for this step. > > + * @delay: Used if type == POWER_SEQ_DELAY > > + * @regulator: Used if type == POWER_SEQ_REGULATOR > > + * @pwm: Used if type == POWER_SEQ_PWN > > + * @gpio: Used if type == POWER_SEQ_GPIO > > In those last 4 line, I think s/type/@type/ since you're referencing > another parameter? Absolutely. > > +struct power_seq_resource { > > + /* relevant for resolving the resource and knowing its type */ > > + struct platform_power_seq_step *pdata; > > Aha. So this isn't really platform data for the resource, but actually a > step definition that referenced it. I think it'd be better to rename > this field "step", and amend the documentation above not to refer to > "pdata" but explicitly talk about a step definition; the step may have > been defined in pdata, but isn't in the DT case. This is a little bit confusing, isn't it? The resource identifier is duplicated in the platform data by every step that uses it. To avoid copying that information again into the resource structure, I just use this pointer to the first step that uses this resource. So a step not only contains step information, but also part of it might be used by a resource instance. Ugh, that's horribly confusing, actually. > Alternatively, why not just copy the step type enum here, rather than > referencing the step definition? On top of the type we will also need the identifier of the resource (string for pwm and regulator, number for GPIO) so we can compare the resource against platform data when building the sequence to avoid allocating it twice. That's a little bit of extra memory, but the gain in clarity is probably worth it. Alex.
WARNING: multiple messages have this Message-ID (diff)
From: Alex Courbot <acourbot@nvidia.com> To: Stephen Warren <swarren@wwwdotorg.org> Cc: Stephen Warren <swarren@nvidia.com>, Thierry Reding <thierry.reding@avionic-design.de>, Simon Glass <sjg@chromium.org>, Grant Likely <grant.likely@secretlab.ca>, Rob Herring <rob.herring@calxeda.com>, Mark Brown <broonie@opensource.wolfsonmicro.com>, Anton Vorontsov <cbou@mail.ru>, David Woodhouse <dwmw2@infradead.org>, Arnd Bergmann <arnd@arndb.de>, Leela Krishna Amudala <leelakrishna.a@gmail.com>, "linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-fbdev@vger.kernel.org" <linux-fbdev@vger.kernel.org>, "devicetree-discuss@lists.ozlabs.org" <devicetree-discuss@lists.ozlabs.org>, "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>, "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org> Subject: Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences Date: Fri, 07 Sep 2012 08:21:52 +0000 [thread overview] Message-ID: <1380082.Ud3NsnIqpg@percival> (raw) In-Reply-To: <504789B1.50205@wwwdotorg.org> Hi Stephen, Skipping the typos and rephrasing issues (which will all be addressed, thanks!), these issues caught my attention more particularly: On Thursday 06 September 2012 01:19:45 Stephen Warren wrote: > > +"regulator" type required properties: > > + - id: name of the regulator to use. Regulator is obtained by > > + regulator_get(dev, id) > > + - enable / disable: one of these two empty properties must be present > > to > > + enable or disable the resource > > + > > +"pwm" type required properties: > > + - id: name of the PWM to use. PWM is obtained by pwm_get(dev, id) > > + - enable / disable: one of these two empty properties must be present > > to > > + enable or disable the resource > > For those two, would "name" be a better property name than "id"? IIRC "name" is a reserved property name in the DT (I remember seeing an error message when compiling nodes with a "name" property that did not match the node's name). "id" was the second best candidate in my mind. > > +"gpio" type required properties: > > + - number: phandle of the GPIO to use. > > Naming the property "gpio" would seem more consistent with our GPIO > properties. Ok, although "gpio.gpio" might look strange in the platform data. > > + - enable / disable: one of these two empty properties must be present > > to > > + enable or disable the resource > > You can't really enable or disable a GPIO (well, perhaps you can, but > I'd consider that to affect tri-state rather than value); it's more that > you're setting the output value to 0 or 1. I think a "value" or > "set-value" property with value <0> or <1> would be better. Right - will fix that. > Overall, the general structure of the bindings looks reasonable to me. Great, maybe we are finally headed to something stable! > I'm not sure what "pdata" is supposed to point at; platform data applies > to the original "struct device", not one of the resources used by the > power sequences. Right - more on this later. > Hmmm. It'd be nice not to need separate functions for the non-DT and DT > cases. That would require that devm_power_seq_set_build() be able to > find the power sequence definitions somewhere other than platform data > in the non-DT case - that's exactly why the regulator and pinctrl > subsystems represent the device<->data mapping table separately from the > device's platform data. Oh, that sounds better indeed - I was still mentally stuck in the previous scheme where power sequences were not accessible from a fixed node of the device. Thanks. > > +++ b/drivers/power/power_seq/Kconfig > > > > +config POWER_SEQ > > + bool > > Some kind of help text might be useful? As this option was not user-visible but automatically enabled by drivers, I did not judge it to be necessary - but after reading your other mail we might need to make it visible and documented after all. > > +/** > > + * struct platform_power_seq_step - platform data for power sequences > > steps + * @type: The type of this step. This decides which member of the > > union is + * valid for this step. > > + * @delay: Used if type = POWER_SEQ_DELAY > > + * @regulator: Used if type = POWER_SEQ_REGULATOR > > + * @pwm: Used if type = POWER_SEQ_PWN > > + * @gpio: Used if type = POWER_SEQ_GPIO > > In those last 4 line, I think s/type/@type/ since you're referencing > another parameter? Absolutely. > > +struct power_seq_resource { > > + /* relevant for resolving the resource and knowing its type */ > > + struct platform_power_seq_step *pdata; > > Aha. So this isn't really platform data for the resource, but actually a > step definition that referenced it. I think it'd be better to rename > this field "step", and amend the documentation above not to refer to > "pdata" but explicitly talk about a step definition; the step may have > been defined in pdata, but isn't in the DT case. This is a little bit confusing, isn't it? The resource identifier is duplicated in the platform data by every step that uses it. To avoid copying that information again into the resource structure, I just use this pointer to the first step that uses this resource. So a step not only contains step information, but also part of it might be used by a resource instance. Ugh, that's horribly confusing, actually. > Alternatively, why not just copy the step type enum here, rather than > referencing the step definition? On top of the type we will also need the identifier of the resource (string for pwm and regulator, number for GPIO) so we can compare the resource against platform data when building the sequence to avoid allocating it twice. That's a little bit of extra memory, but the gain in clarity is probably worth it. Alex.
next prev parent reply other threads:[~2012-09-07 8:21 UTC|newest] Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-08-31 11:34 [PATCH v5 0/4] Runtime Interpreted Power Sequences Alexandre Courbot 2012-08-31 11:34 ` Alexandre Courbot 2012-08-31 11:34 ` Alexandre Courbot [not found] ` <1346412846-17102-1-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2012-08-31 11:34 ` [PATCH v5 1/4] " Alexandre Courbot 2012-08-31 11:34 ` Alexandre Courbot 2012-08-31 11:34 ` Alexandre Courbot 2012-09-05 17:19 ` Stephen Warren 2012-09-05 17:19 ` Stephen Warren 2012-09-07 8:21 ` Alex Courbot [this message] 2012-09-07 8:21 ` Alex Courbot 2012-09-07 8:21 ` Alex Courbot 2012-09-06 14:14 ` Heiko Stübner 2012-09-06 14:14 ` Heiko Stübner [not found] ` <201209061614.54022.heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 2012-09-07 8:04 ` Alex Courbot 2012-09-07 8:04 ` Alex Courbot 2012-09-07 8:04 ` Alex Courbot 2012-09-07 8:15 ` Mark Brown 2012-09-07 8:15 ` Mark Brown 2012-09-07 9:08 ` Heiko Stübner 2012-09-07 9:08 ` Heiko Stübner 2012-09-07 9:08 ` Heiko Stübner [not found] ` <201209071108.42083.heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 2012-09-07 16:36 ` Stephen Warren 2012-09-07 16:36 ` Stephen Warren 2012-09-07 16:36 ` Stephen Warren 2012-08-31 11:34 ` [PATCH v5 2/4] pwm_backlight: use power sequences Alexandre Courbot 2012-08-31 11:34 ` Alexandre Courbot 2012-08-31 11:34 ` Alexandre Courbot [not found] ` <1346412846-17102-3-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2012-09-05 17:25 ` Stephen Warren 2012-09-05 17:25 ` Stephen Warren 2012-09-05 17:25 ` Stephen Warren 2012-09-07 8:28 ` Alex Courbot 2012-09-07 8:28 ` Alex Courbot 2012-09-07 8:28 ` Alex Courbot 2012-09-07 8:29 ` Mark Brown 2012-09-07 8:29 ` Mark Brown 2012-09-07 8:29 ` Mark Brown [not found] ` <20120907082835.GC17749-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> 2012-09-07 8:34 ` Alex Courbot 2012-09-07 8:34 ` Alex Courbot 2012-09-07 8:34 ` Alex Courbot 2012-08-31 11:34 ` [PATCH v5 3/4] tegra: dt: add label to tegra20's PWM Alexandre Courbot 2012-08-31 11:34 ` Alexandre Courbot 2012-08-31 11:34 ` Alexandre Courbot 2012-08-31 11:34 ` [PATCH v5 4/4] tegra: ventana: add pwm backlight DT nodes Alexandre Courbot 2012-08-31 11:34 ` Alexandre Courbot 2012-08-31 11:34 ` Alexandre Courbot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1380082.Ud3NsnIqpg@percival \ --to=acourbot@nvidia.com \ --cc=arnd@arndb.de \ --cc=broonie@opensource.wolfsonmicro.com \ --cc=cbou@mail.ru \ --cc=devicetree-discuss@lists.ozlabs.org \ --cc=dwmw2@infradead.org \ --cc=grant.likely@secretlab.ca \ --cc=leelakrishna.a@gmail.com \ --cc=linux-doc@vger.kernel.org \ --cc=linux-fbdev@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=linux-tegra@vger.kernel.org \ --cc=rob.herring@calxeda.com \ --cc=sjg@chromium.org \ --cc=swarren@nvidia.com \ --cc=swarren@wwwdotorg.org \ --cc=thierry.reding@avionic-design.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.