RE: atmel_mxt_ts: defaulting irqflags to IRQF_TRIGGER_FALLING
Sekhar Nori wrote at Tuesday, July 01, 2014 3:52 AM: Nick, I have been using your for-next branch to base my development of touchscreen support on TI's DRA7x EVM. With the recent updates, it has worked out great and once I got the configuration right, it was just a question of adding DT data for the platform. Now, there is one problem with Stephen's patch defaulting the irqflags to IRQF_TRIGGER_FALLING. The interrupt controller I am using (ARM GIC) does not seem to support that and the device fails to probe: On the Tegra systems I have, IRQF_TRIGGER_FALLING is the correct (or at least a valid) choice. That's probably because the Atmel IRQ signal is routed to our GPIO controller, which is also an IRQ controller, and then forwarded up the chain to the GIC, with the polarity the GIC expects. If IRQ_TRIGGER_FALLING doesn't work everywhere, then we'll need to add some kind of DT property to configure the polarity of the IRQ output. [1.932798] genirq: Setting trigger mode 2 for irq 151 failed (gic_set_type+0x0/0xf0) [1.941340] atmel_mxt_ts 0-004a: Failed to register interrupt [1.947452] atmel_mxt_ts: probe of 0-004a failed with error -22 Attached patch does the trick for me: diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 71154c2..f2d3f72 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -3155,9 +3155,6 @@ static struct mxt_platform_data *mxt_parse_dt(struct i2c_client *client) pdata-gpio_reset = of_get_named_gpio_flags(dev-of_node, atmel,reset-gpio, 0, NULL); - /* Default to this. Properties can be added to configure it later */ - pdata-irqflags = IRQF_TRIGGER_FALLING; - of_property_read_string(dev-of_node, atmel,cfg_name, pdata-cfg_name); Can you switch to leaving the platform to specify flags (at least for the DT case)? Thanks, Sekhar -- nvpublic -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: convert all mov.* pc, reg to bx reg for ARMv6+ (part1)
On 07/01/2014 10:19 AM, Russell King wrote: ARMv6 and greater introduced a new instruction (bx) which can be used to return from function calls. Recent CPUs perform better when the bx lr instruction is used rather than the mov pc, lr instruction, and this sequence is strongly recommended to be used by the ARM architecture manual (section A.4.1.1). We provide a new macro ret with all its variants for the condition code which will resolve to the appropriate instruction. Rather than doing this piecemeal, and miss some instances, change all the mov pc instances to use the new macro, with the exception of the movs instruction and the kprobes code. This allows us to detect the mov pc, lr case and fix it up - and also gives us the possibility of deploying this for other registers depending on the CPU selection. Tested-by: Stephen Warren swar...@nvidia.com (On an NVIDIA Tegra Jetson TK1 board, both CPU hotplug and system sleep were tested, which are the use-cases that actually use the edited assembly files) -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 0/6] regulator: palmas: cleanup and fixes
On 06/30/2014 09:57 AM, Nishanth Menon wrote: Hi, Original thread (v1): http://marc.info/?t=14038076654r=1w=2 This series is based on: git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git branch: topic/palmas (4c0c9ca Merge remote-tracking branch 'regulator/fix/palmas' into regulator-palmas) This series does a few cleanups to help ensure we dont make typo mistakes and finally provides a fix as attempted by (v1). Basic cpufreq tests performed on OMAP5uEVM, DRA7-evm, DRA72-evm all of which are palmas variants. The patches are also available here: https://github.com/nmenon/linux-2.6-playground/commits/broonie-topic-palmas-fixes The series, Tested-by: Stephen Warren swar...@nvidia.com (An NVIDIA Dalmore board boots and shuts down OK with these applied on top of next-20140630, and various peripherals such as LCD panel, audio, USB, and SPI flash all work) -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled
On 06/20/2014 11:26 AM, Nishanth Menon wrote: We use regmap regulator ops to enable/disable and check if regulator is enabled for various SMPS. However, these depend on valid enable_reg, enable_mask and enable_value in regulator descriptor. Currently we do not populate these for SMPS other than SMPS10, this results in spurious results as regmap assumes that the values are valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas variants that do have RTC! To fix this, we update proper parameters for the descriptor fields. Further, we want to ensure the behavior consistent with logic prior to commit dbabd624d4eec50b6, where, once you do a set_mode, enable/disable ensure the logic remains consistent and configures Palmas to the configuration that we set with set_mode (since the configuration register is common). To do this, we can rely on the regulator core's regulator_register behavior where the regulator descriptor pointer provided by the regulator driver is stored. (no reallocation and copy is done). This lets us update the enable_value post registration, to remain consistent with the mode we configure as part of set_mode. Fixes: dbabd624d4eec50b6 (regulator: palmas: Reemove open coded functions with helper functions) Reported-by: Alexandre Courbot acour...@nvidia.com Signed-off-by: Nishanth Menon n...@ti.com Unfortunately, there is still some lingering problem in the original commit. In next-20130623 (and indeed since at least next-20140611), neither the LCD panel or HDMI work on the NVIDIA Dalmore board. Reverting this commit (just for conflicts) and the original problematic commit regulator: palmas: Reemove open coded functions with helper functions solves this. I see the following on boot: [3.558776] tegra-dsi 5430.dsi: cannot get VDD supply [3.564272] platform 5430.dsi: Driver tegra-dsi requests probe deferral [3.571990] tegra-hdmi 5428.hdmi: failed to get PLL regulator [3.578377] platform 5428.hdmi: Driver tegra-hdmi requests probe deferral ... but probe deferral never completes, yet with your remove open coded ... patch reverted, it all works fine. Can you please take another look at the original patch? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled
On 06/23/2014 02:11 PM, Nishanth Menon wrote:
On Mon, Jun 23, 2014 at 2:50 PM, Stephen Warren swar...@wwwdotorg.org wrote:
On 06/20/2014 11:26 AM, Nishanth Menon wrote:
We use regmap regulator ops to enable/disable and check if regulator
is enabled for various SMPS. However, these depend on valid
enable_reg, enable_mask and enable_value in regulator descriptor.
Currently we do not populate these for SMPS other than SMPS10, this
results in spurious results as regmap assumes that the values are
valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas
variants that do have RTC! To fix this, we update proper parameters
for the descriptor fields.
Further, we want to ensure the behavior consistent with logic
prior to commit dbabd624d4eec50b6, where, once you do a set_mode,
enable/disable ensure the logic remains consistent and configures
Palmas to the configuration that we set with set_mode (since the
configuration register is common). To do this, we can rely on the
regulator core's regulator_register behavior where the regulator
descriptor pointer provided by the regulator driver is stored. (no
reallocation and copy is done). This lets us update the enable_value
post registration, to remain consistent with the mode we configure as
part of set_mode.
Fixes: dbabd624d4eec50b6 (regulator: palmas: Reemove open coded functions
with helper functions)
Reported-by: Alexandre Courbot acour...@nvidia.com
Signed-off-by: Nishanth Menon n...@ti.com
Unfortunately, there is still some lingering problem in the original
commit. In next-20130623 (and indeed since at least next-20140611),
neither the LCD panel or HDMI work on the NVIDIA Dalmore board.
Reverting this commit (just for conflicts) and the original problematic
commit regulator: palmas: Reemove open coded functions with helper
functions solves this. I see the following on boot:
[3.558776] tegra-dsi 5430.dsi: cannot get VDD supply
[3.564272] platform 5430.dsi: Driver tegra-dsi requests probe
deferral
[3.571990] tegra-hdmi 5428.hdmi: failed to get PLL regulator
[3.578377] platform 5428.hdmi: Driver tegra-hdmi requests probe
deferral
... but probe deferral never completes, yet with your remove open coded
... patch reverted, it all works fine.
Can you please take another look at the original patch?
Will let keerthy (original patch author) comment on it.
arch/arm/boot/dts/tegra114-dalmore.dts tps65090, avdd_lcd_reg I
suppose is the path in question?
Seems to use drivers/mfd/tps65090.c and
drivers/regulator/tps65090-regulator.c and not palmas?
Am I looking at the right dts?
Yes, that's the right DTS.
There's both a 65090 and a Palmas on the board. It's probably simpler to
look at the HDMI PLL regulator, since the path to the Palmas is more
obvious:
/ {
host1x@5000 {
hdmi@5428 {
pll-supply = palmas_smps3_reg;
...
i2c@7000d000 {
palmas: tps65913@58 {
compatible = ti,palmas;
pmic {
compatible = ti,tps65913-pmic,
ti,palmas-pmic;
regulators {
palmas_smps3_reg: smps3 {
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] regulator: palmas: fix typo in enable_reg calculation
From: Stephen Warren swar...@nvidia.com When setting up .enable_reg for an SMPS regulator, presumably we should call PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE, ...) rather than using LDO_BASE. This change makes the LCD panel and HDMI work again on the NVIDIA Dalmore board anyway. Cc: Alex Courbot gnu...@gmail.com Cc: Keerthy j-keer...@ti.com Cc: Nishanth Menon n...@ti.com Cc: linux-omap@vger.kernel.org Cc: linux-te...@vger.kernel.org Fixes: 318dbb02b50c (regulator: palmas: Fix SMPS enable/disable/is_enabled) Fixes: dbabd624d4eec50b6 (regulator: palmas: Reemove open coded functions with helper functions) Signed-off-by: Stephen Warren swar...@nvidia.com --- drivers/regulator/palmas-regulator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c index 3c861d5f9245..93b4ad842901 100644 --- a/drivers/regulator/palmas-regulator.c +++ b/drivers/regulator/palmas-regulator.c @@ -970,7 +970,7 @@ static int palmas_regulators_probe(struct platform_device *pdev) PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; pmic-desc[id].enable_reg = - PALMAS_BASE_TO_REG(PALMAS_LDO_BASE, + PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE, palmas_regs_info[id].ctrl_addr); pmic-desc[id].enable_mask = PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled
On 06/23/2014 02:20 PM, Stephen Warren wrote: On 06/23/2014 02:11 PM, Nishanth Menon wrote: On Mon, Jun 23, 2014 at 2:50 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 06/20/2014 11:26 AM, Nishanth Menon wrote: We use regmap regulator ops to enable/disable and check if regulator is enabled for various SMPS. However, these depend on valid enable_reg, enable_mask and enable_value in regulator descriptor. Currently we do not populate these for SMPS other than SMPS10, this results in spurious results as regmap assumes that the values are valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas variants that do have RTC! To fix this, we update proper parameters for the descriptor fields. Further, we want to ensure the behavior consistent with logic prior to commit dbabd624d4eec50b6, where, once you do a set_mode, enable/disable ensure the logic remains consistent and configures Palmas to the configuration that we set with set_mode (since the configuration register is common). To do this, we can rely on the regulator core's regulator_register behavior where the regulator descriptor pointer provided by the regulator driver is stored. (no reallocation and copy is done). This lets us update the enable_value post registration, to remain consistent with the mode we configure as part of set_mode. Fixes: dbabd624d4eec50b6 (regulator: palmas: Reemove open coded functions with helper functions) Reported-by: Alexandre Courbot acour...@nvidia.com Signed-off-by: Nishanth Menon n...@ti.com Unfortunately, there is still some lingering problem in the original commit. In next-20130623 (and indeed since at least next-20140611), neither the LCD panel or HDMI work on the NVIDIA Dalmore board. Reverting this commit (just for conflicts) and the original problematic commit regulator: palmas: Reemove open coded functions with helper functions solves this. I see the following on boot: [3.558776] tegra-dsi 5430.dsi: cannot get VDD supply [3.564272] platform 5430.dsi: Driver tegra-dsi requests probe deferral [3.571990] tegra-hdmi 5428.hdmi: failed to get PLL regulator [3.578377] platform 5428.hdmi: Driver tegra-hdmi requests probe deferral ... but probe deferral never completes, yet with your remove open coded ... patch reverted, it all works fine. Can you please take another look at the original patch? Will let keerthy (original patch author) comment on it. arch/arm/boot/dts/tegra114-dalmore.dts tps65090, avdd_lcd_reg I suppose is the path in question? Seems to use drivers/mfd/tps65090.c and drivers/regulator/tps65090-regulator.c and not palmas? Am I looking at the right dts? Yes, that's the right DTS. There's both a 65090 and a Palmas on the board. It's probably simpler to look at the HDMI PLL regulator, since the path to the Palmas is more obvious: ... I think I found the problem; I sent [PATCH] regulator: palmas: fix typo in enable_reg calculation with what's probably the fix. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] irqchip: gic: always mask interrupts during suspend
On 06/10/2014 05:48 PM, Brian Norris wrote: On Wed, Jun 11, 2014 at 01:34:39AM +0200, Thomas Gleixner wrote: On Tue, 10 Jun 2014, Brian Norris wrote: Other random thought: it seems like any irqchip driver which does lazy IRQ masking ought to use IRQCHIP_MASK_ON_SUSPEND. So maybe the IRQ core should just do something like: if (!chip-irq_disable) chip-flags |= IRQCHIP_MASK_ON_SUSPEND; No. Lazy irq disable and the suspend logic are different beasts. OK, fair enough. Drop that random thought then. It's not in the patch content anyway. That's up to the platform to decide this. Just for the record: there is a world outside of ARM... OK. But GIC is ARM-specific, so we can still constrain this patch and related topics to the world of ARM. But that brings me to a different question: Why are you not putting that customization into the device tree instead of trying to add this to some random arch/arm/mach-foo files? I'm not adding customization to arch/arm/mach-foo files. I'm trying to remove it. This property could be added to device tree, if there was really a valid use case for a GIC which leaves its interrupts unmasked for suspend. My question in this patch is essentially: does such a use case exist? DT should genernally only contain data that's expected to vary between boards, or just possibly between SoCs. Anything that the kernel knows simply because it knows what HW model it's driving has no place in DT, since it just adds redundant work to parse the DT and end up with the same data. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ASoC: free jack GPIOs before the sound card is freed
From: Stephen Warren swar...@nvidia.com
This is the same change as commit fb6b8e71448a ASoC: tegra: free jack
GPIOs before the sound card is freed, but applied to all other ASoC
machine drivers where code inspection indicates the same problem exists.
That commit's description is:
==
snd_soc_jack_add_gpios() schedules a work queue item to poll the GPIO to
generate an initial jack status report. If sound card initialization
fails, that work item needs to be cancelled, so it doesn't run after the
card has been freed. Specifically, freeing the card calls
snd_jack_dev_free() which calls snd_jack_dev_disconnect() which sets
jack-input_dev = NULL, and input_dev is used by snd_jack_report(), which
is called from the work queue item.
snd_soc_jack_free_gpios() cancels the work item. The Tegra ASoC machine
drivers do call this function in the platform driver remove() callback.
However, this happens after the sound card is freed, at least when the
card is freed due to errors late during snd_soc_instantiate_card(). This
leaves a window where the work item can execute after the card is freed.
In next-20140522, sound card initialization does fail for unrelated
reasons, and hits the problem described above.
To solve this, fix the Tegra ASoC machine drivers to clean up the Jack
GPIOs during the snd_soc_card's .remove() callback, which is executed
before the overall card object is freed. also, guard the cleanup call
based on whether we actually setup up the GPIOs in the first place.
Ideally, we'd do the cleanup in a struct snd_soc_dai_link .fini/remove
function to match where the GPIOs get set up. However, there is no such
callback.
==
Note that I have not even compile-tested this in most cases, since most
of the drivers rely on specific mach-* support I don't have enabled, and
don't support COMPILE_TEST. Testing by the relevant board maintainers
would be useful.
Cc: Peter Ujfalusi peter.ujfal...@ti.com (maintainer:OMAP AUDIO SUPPORT)
Cc: Jarkko Nikula jarkko.nik...@bitmer.com (maintainer:OMAP AUDIO SUPPORT)
Cc: Philipp Zabel philipp.za...@gmail.com (maintainer:ARM/H4700 (HP IPA...)
Cc: Paul Parsons lost.dista...@yahoo.com (maintainer:ARM/H4700 (HP IPA...)
Cc: Eric Miao eric.y.m...@gmail.com (maintainer:PXA2xx/PXA3xx SUP...)
Cc: Russell King li...@arm.linux.org.uk (maintainer:PXA2xx/PXA3xx SUP...)
Cc: Haojian Zhuang haojian.zhu...@gmail.com (maintainer:PXA2xx/PXA3xx SUP...)
Cc: Ben Dooks ben-li...@fluff.org (maintainer:ARM/SAMSUNG ARM A...)
Cc: Kukjin Kim kgene@samsung.com (maintainer:ARM/SAMSUNG ARM A...)
Cc: Sangbeom Kim sbki...@samsung.com (supporter:SAMSUNG AUDIO (AS...)
Cc: linux-omap@vger.kernel.org (open list:OMAP AUDIO SUPPORT)
Cc: linux-samsung-...@vger.kernel.org (moderated list:ARM/SAMSUNG ARM A...)
Signed-off-by: Stephen Warren swar...@nvidia.com
---
Mark, do you need this patch split up by architecture for topic branches?
I assume not if you've already sent your pull requests. This applies fine
on top of tag asoc-v3.16.
---
sound/soc/omap/ams-delta.c | 14 ++
sound/soc/omap/omap-twl4030.c | 28 ++--
sound/soc/omap/rx51.c | 18 +-
sound/soc/pxa/hx4700.c | 9 -
sound/soc/samsung/h1940_uda1380.c | 11 +--
sound/soc/samsung/rx1950_uda1380.c | 12 ++--
sound/soc/samsung/smartq_wm8987.c | 11 +--
7 files changed, 69 insertions(+), 34 deletions(-)
diff --git a/sound/soc/omap/ams-delta.c b/sound/soc/omap/ams-delta.c
index bb243c663e6b..1f41951d8b7f 100644
--- a/sound/soc/omap/ams-delta.c
+++ b/sound/soc/omap/ams-delta.c
@@ -527,6 +527,15 @@ static int ams_delta_cx20442_init(struct
snd_soc_pcm_runtime *rtd)
return 0;
}
+static int ams_delta_card_remove(struct snd_soc_pcm_runtime *rtd)
+{
+ snd_soc_jack_free_gpios(ams_delta_hook_switch,
+ ARRAY_SIZE(ams_delta_hook_switch_gpios),
+ ams_delta_hook_switch_gpios);
+
+ return 0;
+}
+
/* DAI glue - connects codec -- CPU */
static struct snd_soc_dai_link ams_delta_dai_link = {
.name = CX20442,
@@ -543,6 +552,7 @@ static struct snd_soc_dai_link ams_delta_dai_link = {
static struct snd_soc_card ams_delta_audio_card = {
.name = AMS_DELTA,
.owner = THIS_MODULE,
+ .remove = ams_delta_card_remove,
.dai_link = ams_delta_dai_link,
.num_links = 1,
@@ -579,10 +589,6 @@ static int ams_delta_remove(struct platform_device *pdev)
dev_warn(pdev-dev,
failed to unregister V253 line discipline\n);
- snd_soc_jack_free_gpios(ams_delta_hook_switch,
- ARRAY_SIZE(ams_delta_hook_switch_gpios),
- ams_delta_hook_switch_gpios);
-
snd_soc_unregister_card(card);
card-dev = NULL;
return 0;
diff --git a/sound/soc/omap/omap-twl4030.c b/sound/soc/omap/omap-twl4030.c
index 64141db311b2..b4e282871658 100644
--- a/sound/soc/omap
Re: [PATCH 51/97] ARM: l2c: remove platforms/SoCs setting early BRESP
On 04/28/2014 06:02 PM, Simon Horman wrote: On Mon, Apr 28, 2014 at 08:30:32PM +0100, Russell King wrote: Since we now automatically enable early BRESP in core L2C-310 code when we detect a Cortex-A9, we don't need platforms/SoCs to set this bit explicitly. Instead, they should seek to preserve the value of bit 30 in the auxiliary control register. Acked-by: Tony Lindgren t...@atomide.com Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk I would prefer if this patch was broken out into individual patches for each board or SoC file and that they were then picked up by their respective platform maintainers. Likewise for patch 66/97. Although it is only for shmobile I would prefer it broken out. There are far too many dependencies in this series to break out the board file patches to be merged separately; it'd take either a whole bunch of kernel releases to merge it all that way, or a twisty maze of tiny topic branches cross-merged all over the place. Neither option is realistic. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 51/97] ARM: l2c: remove platforms/SoCs setting early BRESP
On 04/28/2014 01:30 PM, Russell King wrote: Since we now automatically enable early BRESP in core L2C-310 code when we detect a Cortex-A9, we don't need platforms/SoCs to set this bit explicitly. Instead, they should seek to preserve the value of bit 30 in the auxiliary control register. Acked-by: Stephen Warren swar...@nvidia.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 49/97] ARM: l2c: fix register naming
On 04/28/2014 01:30 PM, Russell King wrote: We have a mixture of different devices with different register layouts, but we group all the bits together in an opaque mess. Split them out into those which are L2C-310 specific and ones which refer to earlier devices. Provide full auxiliary control register definitions. Acked-by: Stephen Warren swar...@nvidia.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: rename 'phy' field of 'struct usb_hcd' to 'transceiver'
On 04/09/2014 07:57 AM, Sergei Shtylyov wrote: Return to the 'phy' field of 'struct usb_hcd' its historic name 'transceiver'. This is in preparation to adding the generic PHY support. Surely if the correct term is transceiver, we should be adding generic transceiver support not generic PHY support? To be honest, this rename feels like churn, especially since the APIs and DT bindings all still include the work phy so now everything will be inconsistent. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: rename 'phy' field of 'struct usb_hcd' to 'transceiver'
On 04/09/2014 10:27 AM, Sergei Shtylyov wrote: Hello. On 04/09/2014 07:31 PM, Stephen Warren wrote: Return to the 'phy' field of 'struct usb_hcd' its historic name 'transceiver'. This is in preparation to adding the generic PHY support. Surely if the correct term is transceiver, we should be adding generic transceiver support not generic PHY support? To be honest, this rename feels like churn, especially since the APIs and DT bindings all still include the work phy so now everything will be inconsistent. How about 'usb_phy'? That certainly would make things more consistent, but I wonder why usb_phy is better than phy when the code/struct in question is something USB-specific; the usb_ prefix seems implicit to me due to context. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: rename 'phy' field of 'struct usb_hcd' to 'transceiver'
On 04/09/2014 10:53 AM, Sergei Shtylyov wrote: On 04/09/2014 08:48 PM, Stephen Warren wrote: Return to the 'phy' field of 'struct usb_hcd' its historic name 'transceiver'. This is in preparation to adding the generic PHY support. Surely if the correct term is transceiver, we should be adding generic transceiver support not generic PHY support? To be honest, this rename feels like churn, especially since the APIs and DT bindings all still include the work phy so now everything will be inconsistent. How about 'usb_phy'? That certainly would make things more consistent, but I wonder why usb_phy is better than phy when the code/struct in question is something USB-specific; the usb_ prefix seems implicit to me due to context. I tend to agree. However, I need to name the new field of stype 'struct phy *' somehow... perhaps something like 'gen_phy' for it would do? Ok, the existing field is being replaced by something? I didn't get that from the patch description; I thought the new name in this patch was going to be it. In that case, a temporary name of usb_phy for the existing field, or adding the new field as gen_phy sound reasonable. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: rename 'phy' field of 'struct usb_hcd' to 'transceiver'
On 04/09/2014 12:16 PM, Sergei Shtylyov wrote: Hello. On 04/09/2014 09:56 PM, Alan Stern wrote: Ok, the existing field is being replaced by something? I didn't get that No, not replaced. I'm adding the support for generic PHY to the existing USB PHY support. I thought that was clear from the changelog. from the patch description; I thought the new name in this patch was going to be it. In that case, a temporary name of usb_phy for the existing field, or adding the new field as gen_phy sound reasonable. OK, I'll respin the patch #2 with 'gen_phy' and remove the patch #1. What is the reason for all of this? That is, can you explain the difference between USB PHY support and general PHY support, and why we need both? The generic PHY framework (drivers/phy/phy-core.c) supports multifunction complex PHYs (some functions of which may be related to USB). My case is a Renesas R-Car generation 2 PHY that can switch two USB ports between different USB controllers (one PCI and one non-PCI on each port); I just haven't CCed linux-usb on my driver submission. Though there's already drivers/phy/usb/ driver for that hardware, it failed to meet the expectations (dynamic setting of the port multiplexing depending on what USB host/gadget drivers are loaded), so I had to write a new driver. I guess I don't need to describe drivers/phy/usb/ framework in detail, do I? It only provides for single-function simple USB PHYs... Naively, it sounds like the complex PHY driver should also be a pinctrl driver, since it sounds like the main feature it has beyond a simple PHY is the ability to do pin muxing. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] regulator: fixed: update to devm_* API
On 01/27/2014 08:12 PM, Manish Badarkhe wrote: Update the code to use devm_* API so that driver core will manage resources. I'm not sure why this patch is sent to linux-te...@vger.kernel.org; it seems nothing to do with Tegra (or Samsung or OMAP for that matter). -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: commit c368e5fc2a190923b786f2de3e79430ea3566a25 regresses MMC
On 11/21/2013 11:36 AM, Mark Brown wrote: On Thu, Nov 21, 2013 at 09:43:03AM -0700, Stephen Warren wrote: FYI, the way I deal with this is that my preferred email account subscribes to the mailing list, and I have a filter such that anything that's to/cc either *that* email address *or* any of my other email addresses gets handled the same, and dumped in my (list) inbox rather than in a mailing list folder. I used to do that but I found it tended to create annoying duplicates more often than it was helpful. It's also a bit of an issue for me that there are other people with the same name who work upstream so I do also find people from time to time picking the wrong Mark Brown to e-mail. Ah yes. I also have a delivery-time filter that remembers the last n Message-Id headers I've received, and it dumps any duplicates into a separate folder that I ignore. It seems to work pretty well. It looks like n==1 for me right now. I could post the scripts on github if people think they're useful. Hopefully there aren't security bugs:-) -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: commit c368e5fc2a190923b786f2de3e79430ea3566a25 regresses MMC
On 11/21/2013 03:49 AM, Mark Brown wrote: On Wed, Nov 20, 2013 at 08:48:41PM -0600, Felipe Balbi wrote: actually, I didn't miss you at all. your broo...@linaro.org was in Cc of original email thread. The same email which was used to sign-off on original commit. It's not what's advertised in MAINTAINERS, you should be sending stuff to that address. My work account is completely separate and for various reasons (including the fact that a lot of the mails for upstream sent there are duplicates of mails sent here) non-automatic upstream stuff has a good chance of getting dropped on the floor and at the very least will be dealt with more slowly. FYI, the way I deal with this is that my preferred email account subscribes to the mailing list, and I have a filter such that anything that's to/cc either *that* email address *or* any of my other email addresses gets handled the same, and dumped in my (list) inbox rather than in a mailing list folder. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clk: add flags to distinguish xtal clocks
On 11/08/2013 11:00 AM, Felipe Balbi wrote: From: Luciano Coelho l...@coelho.fi Add a flag that indicate whether the clock is a crystal or not. Additionally, parse a new device tree binding in clk-fixed-rate to set this flag. If clock-xtal isn't set, the clock framework will assume clock to be generated by an oscillator. There's only one user for this binding right now which is Texas Instruments' WiLink devices which need to know details about the clock in order to initialize the underlying WiFi HW correctly. Why on earth does it care? Surely the WiFi HW doesn't care about crystal-vs-non-crystal, but rather some facet of the clock signal that the type of source implies. Shouldn't the DT property describe that facet of the signal, rather than the reason why it has that facet? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] RFC: interrupt consistency check for OF GPIO IRQs
On 10/20/2013 01:41 PM, Laurent Pinchart wrote:
Hi Grant,
On Tuesday 17 September 2013 17:36:32 Grant Likely wrote:
On Thu, 12 Sep 2013 17:57:00 +0200, Alexander Holler wrote:
Am 12.09.2013 17:19, schrieb Stephen Warren:
IRQs, DMA channels, and GPIOs are all different things. Their bindings
are defined independently. While it's good to define new types of
bindings consistently with other bindings, this hasn't always happened,
so you can make zero assumptions about the IRQ bindings by reading the
documentation for any other kind of binding.
Multiple interrupts are defined as follows:
// Optional; otherwise inherited from parent/grand-parent/...
interrupt-parent = gpio6;
// Must be in a fixed order, unless binding defines that the
// optional interrupt-names property is to be used.
interrupts = 1 IRQF_TRIGGER_HIGH 2 IRQF_TRIGGER_LOW;
// Optional; binding for device defines whether it must
// be present
interrupt-names = foo, bar;
If you need multiple interrupts, each with a different parent, you need
to use an interrupt-map property...
...
Actually, I think it is solveable but doing so requires a new binding
for interrupts. I took a shot at implementing it earlier this week and
I've got working patches that I'll be posting soon. I created a new
interrupts-extended property that uses a phandle+args type of
binding like this:
...
device@3000 {
interrupts-extended = intc1 5 intc2 3 4 intc1 6;
};
...
Any progress on this ? I'll need to use multiple interrupts with different
parents in the near future, I can take this over if needed.
I've also been thinking that we could possibly reuse the interrupts
property
without defining a new interrupts-extended. When parsing the property the
code would use the current DT bindings if an interrupt-parent is present, and
the new DT bindings if it isn't.
interrupt-parents doesn't have to be present in individual nodes; it can
be inherited from the parent. That means you'd have to convert whole
sub-trees at once. It seems much more flexible to use a new property and
hence make it explicit what format the data is in.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/10] pwm-backlight: Refactor backlight power on/off
On 09/23/2013 03:40 PM, Thierry Reding wrote:
In preparation for adding an optional regulator and enable GPIO to the
driver, split the power on and power off sequences into separate
functions to reduce code duplication at the multiple call sites.
diff --git a/drivers/video/backlight/pwm_bl.c
b/drivers/video/backlight/pwm_bl.c
+static void pwm_backlight_power_off(struct pwm_bl_data *pb)
+{
+ pwm_disable(pb-pwm);
Both the call-sites you're replacing do the following before pwm_disable():
pwm_config(pb-pwm, 0, pb-period);
While I agree that probably shouldn't be necessary, I think it's at
least worth mentioning that in the commit description just to make it
obvious that it was a deliberate change. Splitting that change into a
separate patch might be reasonable in order to keep refactoring and
functional changes separate, although perhaps it's not worth it.
There are also a couple unrelated whitespace changes thrown in here.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/10] ARM: SAMSUNG: Initialize PWM backlight enable_gpio field
On 09/23/2013 03:41 PM, Thierry Reding wrote:
The GPIO API defines 0 as being a valid GPIO number, so this field needs
to be initialized explicitly.
static void __init smdkv210_map_io(void)
@@ -70,6 +70,7 @@ static struct samsung_bl_drvdata samsung_dfl_bl_data
__initdata = {
.max_brightness = 255,
.dft_brightness = 255,
.pwm_period_ns = 78770,
+ .enable_gpio= -1,
.init = samsung_bl_init,
.exit = samsung_bl_exit,
},
@@ -121,6 +122,10 @@ void __init samsung_bl_set(struct samsung_bl_gpio_info
*gpio_info,
samsung_bl_data-lth_brightness = bl_data-lth_brightness;
if (bl_data-pwm_period_ns)
samsung_bl_data-pwm_period_ns = bl_data-pwm_period_ns;
+ if (bl_data-enable_gpio)
+ samsung_bl_data-enable_gpio = bl_data-enable_gpio;
+ if (bl_data-enable_gpio_flags)
+ samsung_bl_data-enable_gpio_flags = bl_data-enable_gpio_flags;
Won't this cause the core pwm_bl driver to request/manipulate the GPIO,
whereas this driver already does that inside the samsung_bl_init/exit
callbacks? I think you either need to adjust those callbacks, or not set
the new standard GPIO property in samsung_bl_data.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] pwm-backlight: Use new enable_gpio field
On 09/23/2013 03:41 PM, Thierry Reding wrote:
Make use of the new enable_gpio field and allow it to be set from DT as
well. Now that all legacy users of platform data have been converted to
initialize this field to an invalid value, it is safe to use the field
from the driver.
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
Optional properties:
+ - enable-gpios: a list of GPIOs to enable and disable the backlight
That seems to imply that multiple GPIOs are legal. Was that intended? If
not, I would suggest:
- enable-gpios: contains a single GPIO specifier for the GPIO which
enables/disables the backlight. See [1] for the format.
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
+ [1]: Documentation/devicetree/bindings/gpio/gpio.txt
diff --git a/drivers/video/backlight/pwm_bl.c
b/drivers/video/backlight/pwm_bl.c
@@ -51,12 +55,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data
*pb, int brightness,
pb-lth_brightness;
pwm_config(pb-pwm, duty_cycle, pb-period);
+
+ if (gpio_is_valid(pb-enable_gpio)) {
+ if (pb-enable_gpio_flags PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
+ gpio_set_value(pb-enable_gpio, 0);
+ else
+ gpio_set_value(pb-enable_gpio, 1);
+ }
Feel completely free to ignore this, but when the difference in two
code-paths is solely in function parameters, I prefer to calculate the
parameter inside the if statement, then call the function outside the
conditional code, to highlight the common/different parts:
int value;
/* or an if statement for the next 1 line, if you don't like ?: */
value = (pb-enable_gpio_flags PWM_BACKLIGHT_GPIO_ACTIVE_LOW) ? 0 : 1;
gpio_set_value((pb-enable_gpio, value);
@@ -148,11 +168,10 @@ static int pwm_backlight_parse_dt(struct device *dev,
+ data-enable_gpio = of_get_named_gpio_flags(node, enable-gpios, 0,
+ flags);
+ if (gpio_is_valid(data-enable_gpio) (flags OF_GPIO_ACTIVE_LOW))
+ data-enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW;
This doesn't seem to handle deferred probe correctly; I would expect
something more like:
data-enable_gpio = of_get_named_gpio_flags(...);
if (data-enable_gpio == -EPROBE_DEFERRED)
return data-enable_gpio;
if (gpio_is_valid(...))
...
return 0;
I suppose it's possible that the value filters down to
gpio_request_one() and this actually does work out OK, but that feels
very accidental/implicit to me.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] pwm-backlight: Use an optional power supply
On 09/23/2013 03:41 PM, Thierry Reding wrote:
Many backlights require a power supply to work properly. This commit
uses a power-supply regulator, if available, to power up and power down
the panel.
I think that all backlights require a power supply, albeit the supply
may not be SW-controllable. Hence, shouldn't the regulator be mandatory
in the binding, yet the driver be defensively coded such that if one
isn't specified, the driver continues to work?
diff --git a/drivers/video/backlight/pwm_bl.c
b/drivers/video/backlight/pwm_bl.c
@@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct platform_device
*pdev)
}
}
+ pb-power_supply = devm_regulator_get_optional(pdev-dev, power);
... so I think that should be devm_regulator_get(), since the regulator
isn't really optional.
+ if (IS_ERR(pb-power_supply)) {
+ if (PTR_ERR(pb-power_supply) != -ENODEV) {
+ ret = PTR_ERR(pb-power_supply);
+ goto err_gpio;
+ }
+
+ pb-power_supply = NULL;
If devm_regulator_get_optional() returns an error value or a valid
value, then I don't think that this driver should transmute error values
into NULL; NULL might be a perfectly valid regulator value. Related, I
think the if (pb-power_supply) tests should be replaced with if
(!IS_ERR(pb-power_supply)) instead.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/10] pwm-backlight: Allow backlight to remain disabled on boot
On 09/23/2013 03:41 PM, Thierry Reding wrote:
The default for backlight devices is to be enabled immediately when
registering with the backlight core. This can be useful for setups that
use a simple framebuffer device and where the backlight cannot otherwise
be hooked up to the panel.
However, when dealing with more complex setups, such as those of recent
ARM SoCs, this can be problematic. Since the backlight is usually setup
separately from the display controller, the probe order is not usually
deterministic. That can lead to situations where the backlight will be
powered up and the panel will show an uninitialized framebuffer.
Furthermore, subsystems such as DRM have advanced functionality to set
the power mode of a panel. In order to allow such setups to power up the
panel at exactly the right moment, a way is needed to prevent the
backlight core from powering the backlight up automatically when it is
registered.
This commit introduces a new boot_off field in the platform data (and
also implements getting the same information from device tree). When set
the initial backlight power mode will be set to off.
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+ - backlight-boot-off: keep the backlight disabled on boot
A few thoughts:
1) Does this property indicate that:
a) The current state of the backlight at boot. In which case, this will
need bootloader involvement to modify the value in the DT at boot time
based on whether the bootloader turned on the backlight:-(
b) That the driver should not turn on the backlight immediately? That
seems to describe driver behaviour more than HW. Is it appropriate to
put that into DT?
Your suggestion to make the backlight not turn on by default might be a
better fix?
2) Should the driver instead attempt to read the current state of the
GPIO output to determine whether the backlight is on? This may not be
possible on all HW.
3) Doesn't the following code in probe() (added in a previous patch)
need to be updated?
+ if (gpio_is_valid(pb-enable_gpio)) {
+ if (pb-enable_gpio_flags PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
+ gpio_set_value(pb-enable_gpio, 0);
+ else
+ gpio_set_value(pb-enable_gpio, 1);
+ }
... That assumes that the backlight is on at boot, and hence presumably
after this patch still always turns on the backlight, only to turn it
off very quickly once the following code in this patch executes:
(and perhaps we also need to avoid turning the backlight off then on if
it was already on at boot)
diff --git a/drivers/video/backlight/pwm_bl.c
b/drivers/video/backlight/pwm_bl.c
@@ -318,6 +320,12 @@ static int pwm_backlight_probe(struct platform_device
*pdev)
}
bl-props.brightness = data-dft_brightness;
+
+ if (data-boot_off)
+ bl-props.power = FB_BLANK_POWERDOWN;
+ else
+ bl-props.power = FB_BLANK_UNBLANK;
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/10] ARM: SAMSUNG: Initialize PWM backlight enable_gpio field
On 10/01/2013 02:43 PM, Thierry Reding wrote:
On Tue, Oct 01, 2013 at 12:31:04PM -0600, Stephen Warren wrote:
On 09/23/2013 03:41 PM, Thierry Reding wrote:
The GPIO API defines 0 as being a valid GPIO number, so this
field needs to be initialized explicitly.
static void __init smdkv210_map_io(void)
@@ -70,6 +70,7 @@ static struct samsung_bl_drvdata
samsung_dfl_bl_data __initdata = { .max_brightness = 255,
.dft_brightness = 255, .pwm_period_ns = 78770, + .enable_gpio
= -1, .init = samsung_bl_init, .exit =
samsung_bl_exit, }, @@ -121,6 +122,10 @@ void __init
samsung_bl_set(struct samsung_bl_gpio_info *gpio_info,
samsung_bl_data-lth_brightness = bl_data-lth_brightness; if
(bl_data-pwm_period_ns) samsung_bl_data-pwm_period_ns =
bl_data-pwm_period_ns; + if (bl_data-enable_gpio) +
samsung_bl_data-enable_gpio = bl_data-enable_gpio; + if
(bl_data-enable_gpio_flags) +
samsung_bl_data-enable_gpio_flags =
bl_data-enable_gpio_flags;
Won't this cause the core pwm_bl driver to request/manipulate the
GPIO, whereas this driver already does that inside the
samsung_bl_init/exit callbacks? I think you either need to adjust
those callbacks, or not set the new standard GPIO property in
samsung_bl_data.
I don't think so. The samsung_bl_data is a copy of
samsung_dfl_bl_data augmented by board-specific settings. So in
fact copying these values here is essential to allow boards to
override the enable_gpio and flags fields. Currently no board sets
the enable_gpio to a valid GPIO so it's all still handled by the
callbacks only.
Oh yes, you're right. I was confusing the new enable_gpio field in
pwm_bl's platform data with some other field in a custom data structure.
One minor point though:
+ if (bl_data-enable_gpio) + samsung_bl_data-enable_gpio =
bl_data-enable_gpio;
That assumes that enable_gpio==0 means none, whereas you've gone to
great pains in the rest of the series to allow 0 to be a valid GPIO
ID. right now, the default value of samsung_bl_data-enable_gpio is
-1, and if !bl_data-enable_gpio, that value won't be propagated across.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] pwm-backlight: Use an optional power supply
On 10/01/2013 02:53 PM, Thierry Reding wrote:
On Tue, Oct 01, 2013 at 12:43:57PM -0600, Stephen Warren wrote:
On 09/23/2013 03:41 PM, Thierry Reding wrote:
Many backlights require a power supply to work properly. This
commit uses a power-supply regulator, if available, to power up
and power down the panel.
I think that all backlights require a power supply, albeit the
supply may not be SW-controllable. Hence, shouldn't the regulator
be mandatory in the binding, yet the driver be defensively coded
such that if one isn't specified, the driver continues to work?
That has already changed in my local version of this patch.
diff --git a/drivers/video/backlight/pwm_bl.c
b/drivers/video/backlight/pwm_bl.c
@@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct
platform_device *pdev) } }
+ pb-power_supply = devm_regulator_get_optional(pdev-dev,
power);
... so I think that should be devm_regulator_get(), since the
regulator isn't really optional.
+ if (IS_ERR(pb-power_supply)) { + if
(PTR_ERR(pb-power_supply) != -ENODEV) { + ret =
PTR_ERR(pb-power_supply); +goto err_gpio; +
} + +
pb-power_supply = NULL;
If devm_regulator_get_optional() returns an error value or a
valid value, then I don't think that this driver should transmute
error values into NULL; NULL might be a perfectly valid regulator
value. Related, I think the if (pb-power_supply) tests should be
replaced with if (!IS_ERR(pb-power_supply)) instead.
All of that is already done in my local tree. This actually turns
out to work rather smoothly with the new support for optional
regulators. The regulator core will give you a dummy regulator
(assuming it's there physically but hasn't been wired up in
software) that's always on, so the driver doesn't even have to
special case it anymore.
OK, hopefully it (the regulator core) complains about the missing DT
property though; I assume you're using regulator_get() not
regulator_get_optional(), since the supply really is not optional.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 00/15] Device Tree schemas and validation
On 09/24/2013 10:52 AM, Benoit Cousson wrote:
Hi All,
Following the discussion that happened during LCE-2013 and the email
thread started by Tomasz few months ago [1], here is a first attempt
to introduce:
- a schema language to define the bindings accurately
- DTS validation during device tree compilation in DTC itself
Sorry, this is probably going to sound a bit negative. Hopefully you
find it constructive though.
The syntax for a schema is the same as the one for dts. This choice has
been made to simplify its development, to maximize the code reuse and
finally because the format is human-readable.
I'm not convinced that's a good decision.
DT is a language for representing data.
The validation checks described by schemas are rules, or code, and not
static data.
So, while I'm sure it's possible to shoe-horn at least some reasonable
subset of DT validation into DT syntax itself, I feel it's unlikely to
yield something that's scalable enough.
For example, it's easy to specify that a property must be 2 cells long.
What if it could be any multiple of two? That's a lot of numbers to
explicitly enumerate as data. Sure, you can then invent syntax to
represent that specific rule (parameterized by 2), but what about the
next similar-but-different rule? The only approach I can think of to
that is to allow the schema to contain arbitrary expressions, which
would likely need to morph into arbitary statements not just
expressions. Once you're there, I think the schema would be better
represented as a programming language rather than as a data structure
that could have code hooked into it.
How to:
* Associate a schema to one or several nodes
As said earlier a schema can be used to validate one or several nodes
from a dts. To do this the compatible properties from the nodes which
should be validated must be present in the schema.
timer1: timer@4a318000 {
compatible = ti,omap3430-timer;
...
To write a schema which will validate OMAP Timers like the one above,
one may write the following schema:
/dts-v1/;
/ {
compatible = ti,omap[0-9]+-timer;
What about DT nodes that don't have a compatible value? We certainly
have some of those already like /memory and /chosen. We should be able
to validate their schema too. This probably doesn't invalidate being
able to look things up by compatible value though; it just means we need
some additional mechanisms too.
* Define constraints on properties
To define constraints on a property one has to create a node in a schema
which has as name the name of the property that one want to validate.
To specify constraints on the property ti,hwmods of OMAP Timers one
can write this schema:
/dts-v1/;
/ {
compatible = ti,omap[0-9]+-timer;
ti,hwmods {
...
};
compatible and ti,hwmods are both properties in the DT file. However, in
the schema above, one appears as a property, and one as a node. I don't
like that inconsistency. It'd be better if compatible was a node too.
If one want to use a regular as property name one can write this schema:
/dts-v1/;
/ {
compatible = abc;
def {
name = def[0-9];
Isn't it valid to have a property named name within the node itself?
How do you differentiate between specifying the node name and the name
property?
What if the node name needs more validation than just a regex. For
example, suppose we want to validate the
unit-name-must-match-reg-address rule. We need to write some complex
expression using data extracted from reg to calculate the unit address.
Equally, the node name perhaps has to exist in some global list of
acceptable node names. It would be extremely tricky if not impossible to
do that with a regex.
...
};
};
Above one can see that the name property override the node name.
Override implies that dtc would change the node name during compilation.
I think s/override/validate/ or s/override/overrides the validation
rules for/?
* Require the presence of a property inside a node or inside one of its
parents
...
/dts-v1/;
/ {
compatible = ti,twl[0-9]+-rtc;
interrupt-controller {
is-required;
can-be-inherited;
interrupt-controller isn't a good example here, since it isn't a
property that would typically be inherited. Why not use interrupt-parent
instead?
One can check if 'node' has the following subnode 'subnode1', 'subnode2',
and 'abc' with the schema below:
/dts-v1/;
/ {
compatible = comp;
children = abc, subnode[0-9];
};
How is the schema for each sub-node specified?
What if some nodes are optional and some required? The conditions where
a sub-node is required might be complex, and I think we'd always want to
be able to represent them in whatever schema language we chose.
The most obvious way would be to
Re: [RFC v2] gpio/omap: auto-setup a GPIO when used as an IRQ
On 09/24/2013 01:58 AM, Javier Martinez Canillas wrote: The OMAP GPIO controller HW requires a pin to be configured in GPIO input mode in order to operate as an interrupt input. Since drivers should not be aware of whether an interrupt pin is also a GPIO or not, the HW should be fully configured/enabled as an IRQ if a driver solely uses IRQ APIs such as request_irq(), and never calls any GPIO-related APIs. As such, add the missing HW setup to the OMAP GPIO controller's irq_chip driver. Since this bypasses the GPIO subsystem we have to ensure that another caller won't be able to request the same GPIO pin that is used as an IRQ and set its direction as output. Requesting the GPIO and setting its direction as input is allowed though. FWIW, the concept of this patch, Acked-by: Stephen Warren swar...@nvidia.com I didn't review the code; just skimmed it to see where the new functionality was implemented. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 3/3] ARM: dts: N900: Add SSI information
On 09/23/2013 05:46 PM, Sebastian Reichel wrote: Hi, On Mon, Sep 23, 2013 at 02:35:35PM -0600, Stephen Warren wrote: On 09/15/2013 02:44 PM, Sebastian Reichel wrote: Add SSI device tree data for OMAP34xx and Nokia N900. ... +- ti,hwmods: Name of the hwmod associated to the controller, which + is ssi. I don't think we should add any more of that, for new bindings. That basically means not adding new drivers until hwmod is completly removed, since no new drivers not using DT are accepted anymore. hwmod still holds some information, which are not yet mapped to DT. Tony, is defining hwmod properties for new OMAP bindings what everyone is currently doing? I'm not sure how that will work with a stable DT ABI... I wonder if it makes sense not to define the ti,hwmods property in the binding document (so it doesn't become part of the ABI), but put it into the DTS file simply to make the current Linux code work? I'm not sure if that's any better though. +Required Port sub-node properties: +- compatible: Should be set to the following value + ti,omap3-ssi-port (applicable to OMAP34xx devices) Hmm. Is it really the case that there is 1 controller with n ports? Yes with n=2. Are the ports really dependent upon some shared resource? Yes and runtime power management. Couldn't the ports be represented as separate top-level SSI controllers? Maybe with some phandles. The current layout is cleaner IMHO. The ports are part of the controller and actually most boards only use one of them. In the original driver only the controller hat platform data with memory areas called port1_rx etc. If the HW block really does include 2 ports, then representing it as a single node in DT is fine; I was just making sure. +- interrupts: Contains the interrupt information for the port. +- interrupt-names: Contains the names of the interrupts. It's expected, +that mpu_irq0 and mpu_irq1 are provided. What exactly are those interrupts? MPU sounds like an external micro-controller/processor... +- ti,ssi-cawake-gpio: Defines which GPIO pin is used to signify CAWAKE +events for the port. This is an optional board-specific +property. If it's missing the port will not be +enabled. That also sounds like something that's a higher-level protocol, rather than whatever low-level transport SSI implements. Should this be part of a child node that represents the device attached to the SSI controller? Both the interrupts and the cawake-gpio are used as irqs for starting data transfers. As far as I understand it none of them are specific to the attached device. But are the interrupts and GPIO actually part of the HSI protocol itself, or something layered on top? While your particular board has them wired up, is it strictly necessary for all boards using HSI to have those IRQs/GPIOs? Does the SSI controller (or its ports) not need any clocks, resets, regulators, ...? The only other stuff needed is taken care of by hwmod, which can be seen in this patch: https://lkml.org/lkml/2013/9/15/97 It would be best to completely define the DT binding so that all required clocks etc. are already present in the DT. That way, the DT ABI won't change once people stop using hwmods. Tony, is that possible on OMAP at present, irrespective of whether those e.g. clock properties will actually be used by Linux? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] gpio/omap: auto-setup a GPIO when used as an IRQ
On 09/22/2013 08:40 AM, Javier Martinez Canillas wrote: To use a GPIO pin as an interrupt line, two previous configurations have to be made: a) Map the GPIO pin as an interrupt line into the Linux irq space b) Enable the GPIO bank and configure the GPIO direction as input Most GPIO/IRQ chip drivers just create a mapping for every single GPIO pin with irq_create_mapping() on .probe so users usually can assume a) and only have to do b) by using the following sequence: gpio_request(gpio, foo IRQ); gpio_direction_input(gpio); and then request a IRQ with: irq = gpio_to_irq(gpio); request_irq(irq, ...); Some drivers know that their IRQ line is being driven by a GPIO and use a similar sequence as the described above but others are not aware or don't care wether their IRQ is a real line from an interrupt controller or a GPIO pin acting as an IRQ. ... I think that explanation is a bit like retro-actively implying that drivers /should/ be aware of whether their IRQ is a GPIO or not, and should be acting differently. However, they should not. I would much rather see a simpler patch description along the lines of: The OMAP GPIO controller HW requires that a pin be configured in GPIO mode in order to operate as an interrupt input. Since drivers should not be aware of whether an interrupt pin is also a GPIO or not, the HW should be fully configured/enabled as an IRQ if a driver solely uses IRQ APIs such as request_irq, and never calls any GPIO-related APIs. As such, add the missing HW setup to the OMAP GPIO controller's irq_chip driver. The code change looks like it does what I would expect though. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 3/3] ARM: dts: N900: Add SSI information
On 09/15/2013 02:44 PM, Sebastian Reichel wrote: Add SSI device tree data for OMAP34xx and Nokia N900. What is an SSI device, ... diff --git a/Documentation/devicetree/bindings/hsi/omap_ssi.txt b/Documentation/devicetree/bindings/hsi/omap_ssi.txt ... and what is the HSI subsystem? +OMAP SSI controller bindings + +Required properties: +- compatible:Should be set to the following value +ti,omap3-ssi (applicable to OMAP34xx devices) I think that'd be better phrased as: Should include ti,omap3-ssi. The binding should not preclude other compatibel values being present (e.g. a SoC-specific compatible value, to allow SoC-specific quirks to be enabled later). +- ti,hwmods: Name of the hwmod associated to the controller, which + is ssi. I don't think we should add any more of that, for new bindings. +- reg: Contains SSI register address range (base address and + length). +- reg-names: Contains the names of the address ranges. It's +expected, that sys and gdd address ranges are + provided. Why two entries in reg-names but only one in reg? I think it'd be better to write: reg-names: Contains the values sys and gdd. reg:Contains a register specifier for each entry in reg-names. A similar re-ordering/-wording would apply to interrupts/interrupt-names. +- ranges Required as an empty node s/node/property/ Why must ranges be empty? As long as the content correctly represents the bus setup, why does the content matter at all. How about: ranges Represents the bus address mapping between the main controller node and the child nodes below. +Each port is represented as a sub-node of the ti,omap3-ssi device. + +Required Port sub-node properties: +- compatible:Should be set to the following value +ti,omap3-ssi-port (applicable to OMAP34xx devices) Hmm. Is it really the case that there is 1 controller with n ports? Are the ports really dependent upon some shared resource? Couldn't the ports be represented as separate top-level SSI controllers? +- interrupts:Contains the interrupt information for the port. +- interrupt-names: Contains the names of the interrupts. It's expected, + that mpu_irq0 and mpu_irq1 are provided. What exactly are those interrupts? MPU sounds like an external micro-controller/processor... +- ti,ssi-cawake-gpio:Defines which GPIO pin is used to signify CAWAKE + events for the port. This is an optional board-specific + property. If it's missing the port will not be + enabled. That also sounds like something that's a higher-level protocol, rather than whatever low-level transport SSI implements. Should this be part of a child node that represents the device attached to the SSI controller? Does the SSI controller (or its ports) not need any clocks, resets, regulators, ...? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] usb: dwc3: msm: Add device tree binding information
On 09/23/2013 01:32 PM, Felipe Balbi wrote: Hi, On Wed, Aug 21, 2013 at 04:29:44PM +0300, Ivan T. Ivanov wrote: From: Ivan T. Ivanov iiva...@mm-sol.com MSM USB3.0 core wrapper consist of USB3.0 IP from Synopsys (SNPS) and HS, SS PHY's control and configuration registers. It could operate in device mode (SS, HS, FS) and host mode (SS, HS, FS, LS). Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com and here's a new version from same patch The binding looks pretty simple, so I don't think it's too contentious. diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt +MSM DWC3 controller wrapper +Optional properties : +- gdsc-supply : phandle to the globally distributed switch controller + regulator node to the USB controller. If that's a regulator node, why not use xxx-supply properties to interface with it? Aside from that, the binding, Acked-by: Stephen Warren swar...@nvidia.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] RFC: interrupt consistency check for OF GPIO IRQs
On 09/12/2013 05:37 AM, Alexander Holler wrote: Am 12.09.2013 13:26, schrieb Alexander Holler: Am 12.09.2013 13:09, schrieb Alexander Holler: Am 12.09.2013 12:28, schrieb Alexander Holler: Am 12.09.2013 12:11, schrieb Javier Martinez Canillas: On 09/12/2013 10:55 AM, Alexander Holler wrote: ... So, if I understood the code correctly the DT IRQ core doesn't expect a device node to have more than one interrupt-parent property. The root-cause is the binding definition, not the code. The interrupts property does not contain the phandle of the IRQ controller, but rather the interrupt-parent property does. Thus, there is a single interrupt parent for each node, unless you employ some tricks (see below). It *should* work though if you have multiple interrupts properties defined and all of them have the same interrupt-parent: interrupt-parent = gpio6; interrupts = 1 IRQF_TRIGGER_HIGH; /* GPIO6_1 */ interrupts = 2 IRQF_TRIGGER_LOW; /* GPIO6_2 */ DT is a key/value data structure, not a list of property (name, value) pairs. In other words, you can't have multiple properties of the same name in a node. At least in the compiled DTB; you /might/ be able to compile the DT above with dtc, but if so the second definition of the property will just over-write the first. ... I've just seen how they solved it for dma: dmas = edma0 16 edma0 17; dma-names = rx, tx; ... And looking at how gpios are defined, I think it should be like that: interrupts = gpio6 1 IRQF_TRIGGER_HIGH gpio7 2 IRQF_TRIGGER_LOW ; interrupt-names = foo, bar; So without that interrupt-parent. IRQs, DMA channels, and GPIOs are all different things. Their bindings are defined independently. While it's good to define new types of bindings consistently with other bindings, this hasn't always happened, so you can make zero assumptions about the IRQ bindings by reading the documentation for any other kind of binding. Multiple interrupts are defined as follows: // Optional; otherwise inherited from parent/grand-parent/... interrupt-parent = gpio6; // Must be in a fixed order, unless binding defines that the // optional interrupt-names property is to be used. interrupts = 1 IRQF_TRIGGER_HIGH 2 IRQF_TRIGGER_LOW; // Optional; binding for device defines whether it must // be present interrupt-names = foo, bar; If you need multiple interrupts, each with a different parent, you need to use an interrupt-map property (Google it for a more complete explanation I guess). Unlike interrupts, interrupt-map has a phandle in each entry, and hence each entry can refer to a different IRQ controller. You end up defining a dummy interrupt controller node (which may be the leaf node with multiple IRQ outputs, which then points at itself as the interrupt parent), pointing the leaf node's interrupt-parent at that node, and then having interrupt-map demux the N interrupt outputs to the various interrupt controllers. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 08/29/2013 05:48 AM, George Cherian wrote: On 8/29/2013 4:07 PM, Chanwoo Choi wrote: ... I tested various development board based on Samsung Exynos series SoC. Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, this gpio state could mean off state, disconnected or un-powered state according to gpio. Of course, above explanation about specific gpio don't include all gpios. This is meaning that there is possibility. okay then how about adding a flag for inverted logic too. something like this if(of_property_read_bool(np,inverted_gpio)) gpio_usbvid-gpio_inv = 1; And always check on this before deciding? Don't do this. GPIO specifiers in DT typically include a flags cell that describes certain GPIO properties. One of those properties is signal inversion. You can use of_get_named_gpio_flags() to retrieve those flags. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 08/28/2013 11:33 AM, George Cherian wrote:
Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
the ID/VBUS pin are connected via GPIOs. This driver is tested on
DRA7x board were the ID pin is routed via GPIOs. The driver supports
both VBUS and ID pin configuration and ID pin only configuration.
diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
+EXTCON FOR USB VIA GPIO
Please write a brief explanation of the HW that this binding represents.
Extcon is a Linux-specific term. Binding documentation should be
written in an OS-agnostic manner. Bindings should not reference
OS-specific terminology, and should be a pure description of the HW.
+Required Properties:
+ - compatible : Should be ti,gpio-usb-vid for USB VBUS-ID detector
+ using gpios or ti,gpio-usb-id for USB ID pin detector
Those souond like two rather different things. Should they be separate
bindings, or at the least, separate sections in the document?
If this binding is meant to represent some generic hardware, the vendor
prefix probably isn't required. So, remove ti, from the compatible values.
+ - gpios : phandle and args ID pin gpio and VBUS gpio.
s/and args/and specifier/.
+The first gpio used for ID pin detection
+and the second used for VBUS detection.
+ID pin gpio is mandatory and VBUS is optional
+depending on implementation.
It'd be better to use named GPIO properties here, rather than having
multiple different kinds of GPIO in the same property. How about
id-gpios and vbus-gpios as the property names?
The semantics of each property should be specified. Are these input
GPIOs that reflect the ID and VBUS state? Do they directly represent the
signal state on the connector, or are they processed somehow? Does the
VBUS GPIO control the board's VBUS output, or is it an input? If it
controls VBUS output, it probably should be represented as a regulator
rather than a GPIO.
I think the following might work:
Required properties:
id-gpios: GPIO specifier for the connector's ID pin input. This directly
represents the value present on the ID pin at the connector.
Optional properties:
vbus-gpios: GPIO specifier for the connector's VBUS signal. This
directly represents whether VBUS being driven by the USB cable itself.
(although perhaps that isn't an optional property, but rather a required
property of the second compatible value?)
+Please refer to ../gpio/gpio.txt for details of the common GPIO bindings
+
+Example:
+
+ gpio_usbvid_extcon1 {
+ compatible = ti,gpio-usb-vid;
+ gpios = gpio1 1 0,
+ gpio2 2 0;
+ };
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] ARM: dts: dra7-evm: Add extcon nodes for USB ID pin detection
On 08/28/2013 11:33 AM, George Cherian wrote:
Add
-extcon nodes for USB ID pin detection.
-i2c nodes.
-pcf nodes to which USB ID pin is connected.
diff --git a/arch/arm/boot/dts/dra7-evm.dts b/arch/arm/boot/dts/dra7-evm.dts
dwc3_1 {
- dr_mode = otg;
+ dr_mode = host;
};
I wonder why one cares about ID/VBUS detection if the port doesn't
operate in OTG mode?
dwc3_2 {
dr_mode = host;
};
+
+usb1 {
+ extcon = extcon1;
+};
+
+usb2 {
+ extcon = extcon2;
+};
I assume the extcon property is already fully documented in the
binding for the USB controller? For some reason, extcon looks like an
odd property name; I would have expected something more HW-oriented that
Linux-subsystem-oriented, such as connector, or usb-connector.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] DMA: add help function to check whether dma controller registered
On 08/22/2013 07:17 PM, Richard Zhao wrote: On Fri, Aug 23, 2013 at 04:36:53AM +0800, Stephen Warren wrote: On 08/22/2013 12:43 AM, Richard Zhao wrote: DMA client device driver usually needs to know at probe time whether dma controller has been registered to deffer probe. So add a help function of_dma_check_controller. DMA request channel functions can also used to check it, but they are usually called at open() time. This new function is almost identical to the existing of_dma_request_slave_channel(). Surely the code should be shared? ofdma-of_dma_xlate(dma_spec, ofdma); The above is called holding of_dma_lock. If I want to abstract the common lines, there' two options. What is the problem with acquiring the lock? If request_slave_channel() needs to take the lock, then there must be a reason which presumably applies to the other path too. ... But that said, I don't see any need for a new function; why can't drivers simply call of_dma_request_slave_channel() at probe time; It'll mislead user. channel supposed to be request at open time. I don't agree. from what I can see, that function doesn't actually request the channel, but rather simply looks it up, just like this one. The only difference is that of_dma_xlate() is also called, but that's just doing some data transformation, not actually recording channel ownership. xlate function request the channel if things go well. Oh. xlate should not do that; that's a design flaw. xlate should do nothing more than translate the DT content to an internal channel ID. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DMA: let filter functions of of_dma_simple_xlate possible check of_node
On 08/22/2013 07:29 PM, Richard Zhao wrote: On Fri, Aug 23, 2013 at 04:18:27AM +0800, Stephen Warren wrote: On 08/21/2013 11:19 PM, Richard Zhao wrote: On Fri, Aug 02, 2013 at 10:00:00AM +0800, Richard Zhao wrote: pass of_phandle_args dma_spec to dma_request_channel in of_dma_simple_xlate, so the filter function could access of_node in of_phandle_args. It also remove restriction of #dma-cells has to be one. Signed-off-by: Richard Zhao riz...@nvidia.com --- drivers/dma/edma.c | 7 +-- drivers/dma/of-dma.c | 10 -- drivers/dma/omap-dma.c | 6 -- 3 files changed, 13 insertions(+), 10 deletions(-) Hi Vinod, Can you please pick up this change? Hi Stephen, Can you please give a ack or reviewed-by etc? Hmm. Looking at the patch, I'm not sure it's right. This patch simply passes all the specfier args to the filter function, and the code to check the equality of the of_node to the filter args is still duplicated in each DMA driver. Instead, the DMA core should be implementing the equality check, and only even calling the driver-specific filter function for devices where the client's phandle matches the DMA providing device's of_node handle. Filter function is called in dmaengine core code, independent of dt. The core code can still check if a dmaengine's driver was instantiated from DT and take additional actions in that case. And the reason why the driver has to write its own filter function is it has to store slave id there in its own way. I'm not saying don't call the driver's filter function, but rather that the dmaengine core should perform the common checks before doing so. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] extcon: palmas: Added a new compatible type *ti,palmas-usb-vid*
On 08/23/2013 05:28 AM, Kishon Vijay Abraham I wrote: Hi, On Friday 23 August 2013 02:20 AM, Stephen Warren wrote: On 08/22/2013 02:31 AM, Kishon Vijay Abraham I wrote: The Palmas device contains only a USB VBUS-ID detector, so added a compatible type *ti,palmas-usb-vid*. Didn't remove the existing compatible types for backward compatibility. diff --git a/Documentation/devicetree/bindings/extcon/extcon-palmas.txt b/Documentation/devicetree/bindings/extcon/extcon-palmas.txt PALMAS USB COMPARATOR Required Properties: - - compatible : Should be ti,palmas-usb or ti,twl6035-usb + - compatible : Should be ti,palmas-usb-vid. ti,twl6035-usb and + ti,palmas-usb is deprecated and is kept for backward compatibility. So this defines one new value and deprecates the two old values. yeah. Why isn't a new ti,twl6035-usb-vid entry useful? Don't you still need yeah, it should be added too. SoC-specific compatible values so the driver can enable any SoC-specific bug-fixes/workarounds later if needed? hmm.. Palmas is external to SoC. So not sure if adding SoC specific compatible values is such a good idea. In this case, but SoC, I meant the Palmas chip rather than the application processor. Is twl6035 a name for Palmas or something else? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DMA: let filter functions of of_dma_simple_xlate possible check of_node
On 08/21/2013 11:19 PM, Richard Zhao wrote: On Fri, Aug 02, 2013 at 10:00:00AM +0800, Richard Zhao wrote: pass of_phandle_args dma_spec to dma_request_channel in of_dma_simple_xlate, so the filter function could access of_node in of_phandle_args. It also remove restriction of #dma-cells has to be one. Signed-off-by: Richard Zhao riz...@nvidia.com --- drivers/dma/edma.c | 7 +-- drivers/dma/of-dma.c | 10 -- drivers/dma/omap-dma.c | 6 -- 3 files changed, 13 insertions(+), 10 deletions(-) Hi Vinod, Can you please pick up this change? Hi Stephen, Can you please give a ack or reviewed-by etc? Hmm. Looking at the patch, I'm not sure it's right. This patch simply passes all the specfier args to the filter function, and the code to check the equality of the of_node to the filter args is still duplicated in each DMA driver. Instead, the DMA core should be implementing the equality check, and only even calling the driver-specific filter function for devices where the client's phandle matches the DMA providing device's of_node handle. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] DMA: add help function to check whether dma controller registered
On 08/22/2013 12:43 AM, Richard Zhao wrote: DMA client device driver usually needs to know at probe time whether dma controller has been registered to deffer probe. So add a help function of_dma_check_controller. DMA request channel functions can also used to check it, but they are usually called at open() time. This new function is almost identical to the existing of_dma_request_slave_channel(). Surely the code should be shared? But that said, I don't see any need for a new function; why can't drivers simply call of_dma_request_slave_channel() at probe time; from what I can see, that function doesn't actually request the channel, but rather simply looks it up, just like this one. The only difference is that of_dma_xlate() is also called, but that's just doing some data transformation, not actually recording channel ownership. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] extcon: palmas: Added a new compatible type *ti,palmas-usb-vid*
On 08/22/2013 02:31 AM, Kishon Vijay Abraham I wrote: The Palmas device contains only a USB VBUS-ID detector, so added a compatible type *ti,palmas-usb-vid*. Didn't remove the existing compatible types for backward compatibility. diff --git a/Documentation/devicetree/bindings/extcon/extcon-palmas.txt b/Documentation/devicetree/bindings/extcon/extcon-palmas.txt PALMAS USB COMPARATOR Required Properties: - - compatible : Should be ti,palmas-usb or ti,twl6035-usb + - compatible : Should be ti,palmas-usb-vid. ti,twl6035-usb and + ti,palmas-usb is deprecated and is kept for backward compatibility. So this defines one new value and deprecates the two old values. Why isn't a new ti,twl6035-usb-vid entry useful? Don't you still need SoC-specific compatible values so the driver can enable any SoC-specific bug-fixes/workarounds later if needed? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
On 08/21/2013 07:06 AM, George Cherian wrote:
Hi Stephen,
On 8/20/2013 10:23 PM, Stephen Warren wrote:
ID pins are connected to pcf8575, and the pcf8575's interrupt line is
inturn connected to
gpio bank6 pin 11, we use this gpio interrupt to detect the ID pin
change.
In that case, the PCF8575 node needs to be a GPIO controller and an IRQ
controller, as does the driver for the PCF8575. This binding should have
a single entry in the gpios property, and the driver can call
gpio_to_irq() on that so it knows which IRQ to request.
You meant some thing like this?
pcf_usb: pcf8575@21 {
compatible = ti,pcf8575;
reg = 0x21;
gpio-controller;
#gpio-cells = 2;
interrupt-parent = gpio6;
interrupts = 11 2;
interrupt-controller;
#interrupt-cells = 2;
};
usb_vid_gpio {
compatible = ti,dra7xx-usb;
gpios = pcf_usb 1 0;
};
Yes.
Except that the compatible value for the usb_vid_gpio node still looks
wrong, since I think that node isn't anything to do with any particular SoC.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
On 08/20/2013 12:55 AM, George Cherian wrote: Hi Stephen, Thanks for your review. On 8/20/2013 1:01 AM, Stephen Warren wrote: On 08/16/2013 04:13 AM, George Cherian wrote: Adding extcon driver for USB ID detection to dynamically configure USB Host/Peripheral mode. diff --git a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt +EXTCON FOR DRA7xx + +Required Properties: Please at lest explain what a DRA7xxx is, and the purpose of the HW module this binding describes. DRA7xx is the SoC name and the USB VID detection is implemented via gpio's Basically it does only ID detection via GPIO and there is no VBUS detection in h/w. If there's no SoC-specific HW, then the binding has nothing to do with the SoC; it's entirely generic. I think instead, you need to define a generic gpio-usb-vid binding instead. Otherwise, ever SoC that doesn't have explicit USB VID detection HW is going to re-invent the exact same binding, with pointless differences, rather than using a single generic binding. + - compatible : Should be ti,dra7xx-usb If this is a USB VID detector rather than a full USB host controller, then usb in the binding is a bit over-reaching. Perhaps -usb-vid or -usb-vid-detector would be more accurate. This will be renamed to dra7xx-extcon. So no, that rename doesn't sound correct. + - gpios : phandle to ID pin and interrupt gpio. Why does the interrupt line need to be included in a list of GPIOs? ID pins are connected to pcf8575, and the pcf8575's interrupt line is inturn connected to gpio bank6 pin 11, we use this gpio interrupt to detect the ID pin change. In that case, the PCF8575 node needs to be a GPIO controller and an IRQ controller, as does the driver for the PCF8575. This binding should have a single entry in the gpios property, and the driver can call gpio_to_irq() on that so it knows which IRQ to request. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] extcon: palmas: Modified the compatible type to *ti,palmas-usb-vid*
On 08/18/2013 11:05 PM, Kishon Vijay Abraham I wrote: Hi, On Saturday 17 August 2013 03:51 AM, Stephen Warren wrote: On 08/16/2013 04:20 AM, Kishon Vijay Abraham I wrote: The Palmas device contains only a USB VID detector, so modified the compatible type to *ti,palmas-usb-vid*. diff --git a/Documentation/devicetree/bindings/extcon/extcon-palmas.txt b/Documentation/devicetree/bindings/extcon/extcon-palmas.txt PALMAS USB COMPARATOR Required Properties: - - compatible : Should be ti,palmas-usb or ti,twl6035-usb + - compatible : Should be ti,palmas-usb-vid. Has the old value been published in a release kernel? If so, it makes No. This was merged only in 3.11-rc1. So I think we should take this version? Chanwoo can you take this patch? So anything in 3.11-rc1 will make it into 3.11 final, and hence has already been published, and hence should be an ABI. This current patch is only going into 3.12, so really should be amended not to break the ABI. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] usb: dwc3: msm: Add device tree binding information
On 08/19/2013 06:27 AM, Ivan T. Ivanov wrote: Hi, On Fri, 2013-08-16 at 16:44 -0600, Stephen Warren wrote: On 08/14/2013 06:59 AM, Ivan T. Ivanov wrote: From: Ivan T. Ivanov iiva...@mm-sol.com MSM USB3.0 core wrapper consist of USB3.0 IP from Synopsys (SNPS) and HS, SS PHY's control and configuration registers. It could operate in device mode (SS, HS, FS) and host mode (SS, HS, FS, LS). diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt +- clock-names : ... + sleep_a_clk : Sleep clock, used when USB3 core goes into low ... + ref_clk : Reference clock - used in host mode. ... + core_clk : Master/Core clock, have to be = 125 MHz for SS ... + iface_clk : System bus AXI clock + sleep_clk : Sleep clock, used when USB3 core goes into low ... + utmi_clk : Generated by HS-PHY. Used to clock the low power I think it makes sense to remove _clk from all those names, unless the HW documentation really talks about a clock named e.g. iface_clk yet some other clock names in the documentation don't have the _clk suffix, e.g. the xo I didn't quote. From limited information that I have, I could not say how clock inputs are named from the controller perspective, but I agree that _clk suffix looks redundant. Side question: if for example label in controller says UTMI, should I also use capital letters for the resource or this could be utmi? All the clock-names entries I've seen so far have been lower-case, but I suppose there's no hard-and-fast rule that they couldn't be upper-/mixed-case if that best matched the HW documentation. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
On 08/16/2013 04:13 AM, George Cherian wrote:
Adding extcon driver for USB ID detection to dynamically
configure USB Host/Peripheral mode.
diff --git a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
+EXTCON FOR DRA7xx
+
+Required Properties:
Please at lest explain what a DRA7xxx is, and the purpose of the HW
module this binding describes.
+ - compatible : Should be ti,dra7xx-usb
If this is a USB VID detector rather than a full USB host controller,
then usb in the binding is a bit over-reaching. Perhaps -usb-vid or
-usb-vid-detector would be more accurate.
+ - gpios : phandle to ID pin and interrupt gpio.
This isn't just a phandle; it's phandle+args, or a GPIO specifier. Some
reference should be made to ../gpio/gpio.txt for the format.
Why does the interrupt line need to be included in a list of GPIOs?
If the DRA7xxx is a VID detector, why does it even need/have any GPIOs
associated with it; surely it has a dedicated VID input pin. Can you
provide more details re: how the HW is structured.
+Optional Properties:
+ - interrupt-parent : interrupt controller phandle
+ - interrupts : interrupt number
+
+
It's typical insert the following between those two blank lines:
Example:
... or delete one of the blank lines.
+dra7x_extcon1 {
+ compatible = ti,dra7xx-usb;
+ gpios = pcf_usb 1 0,
+ gpio6 11 2;
+ interrupt-parent = gpio6;
+ interrupts = 11;
+ };
+
No need for that trailing blank line.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] extcon: palmas: Modified the compatible type to *ti,palmas-usb-vid*
On 08/16/2013 04:20 AM, Kishon Vijay Abraham I wrote:
The Palmas device contains only a USB VID detector, so modified the
compatible type to *ti,palmas-usb-vid*.
diff --git a/Documentation/devicetree/bindings/extcon/extcon-palmas.txt
b/Documentation/devicetree/bindings/extcon/extcon-palmas.txt
PALMAS USB COMPARATOR
Required Properties:
- - compatible : Should be ti,palmas-usb or ti,twl6035-usb
+ - compatible : Should be ti,palmas-usb-vid.
Has the old value been published in a release kernel? If so, it makes
sense to document both values, but say the old one is deprecated, and ...
diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
static struct of_device_id of_palmas_match_tbl[] = {
- { .compatible = ti,palmas-usb, },
- { .compatible = ti,twl6035-usb, },
+ { .compatible = ti,palmas-usb-vid, },
... perhaps just add the new value here, and don't remove the old values?
If not, ignore this.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: dts: AM4372: add few nodes
On 08/16/2013 04:17 AM, Benoit Cousson wrote:
Hi Afzal,
On 12/08/2013 08:48, Afzal Mohammed wrote:
Hi Mark,
On Saturday 10 August 2013 07:53 PM, Mark Rutland wrote:
+mac: ethernet@4a10 {
+compatible = ti,am4372-cpsw,ti,cpsw;
One point worth mentioning is that the ti,am4372-cpsw string isn't
documented. Will the ti,am4372-cpsw binding definitely be a superset
of the ti,cpsw binding, and if you were to take the DT as of this
patch, and attempt to use it with a future kernel, can you guarantee
it'll work?
ti,am4372-cpsw was not documented as OMAP DT maintainer didn't prefer
documenting only for a new compatible.
My point was more that creating a new compatible for the exact same
version of the IP is pointeless and could lead to tons of compatible
strings that would never be used since this is always the exact same IP.
Well, there are two aspects: Version of the IP block, and the
integration into the SoC. We need an entry in compatible for any/all of
these. If the IP block doesn't have some standalone versioning scheme
because the design flow isn't IP-block-centric, the SoC name makes a
reasonable substitute.
...
My second point is, even if we want to differentiate the IPs in various
SoCs, using the name of the SoC in the IP compatible string is not a
very good practice anyway.
There may be integration-specific issues, so even if SoC A and B use the
exact same IP block version, there should still be compatible values for
the SoC as well as the IP block version, so you can quirk on those later.
... although I suppose it might not be too bad if the IP block
compatible value only contained the IP block version, and if any
integration-specific quirks were needed, they could be triggered off
entries in the top-level node's compatible value? That only works for
on-Soc modules, and not complex MFD-like modules, unless you have an
easy way to find the DT node for the top-level of the MFD...
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] extcon: palmas: Added a new compatible type *ti,palmas-usb-vid*
On 08/13/2013 11:24 PM, Kishon Vijay Abraham I wrote: Hi, On Wednesday 14 August 2013 12:43 AM, Stephen Warren wrote: On 08/12/2013 11:37 PM, Kishon Vijay Abraham I wrote: The Palmas device contains only a USB VID detector, so added a compatible type *ti,palmas-usb-vid*. Dint remove the existing compatible s/Dint/Didn't/ diff --git a/Documentation/devicetree/bindings/extcon/extcon-twl.txt b/Documentation/devicetree/bindings/extcon/extcon-twl.txt PALMAS USB COMPARATOR Required Properties: - - compatible : Should be ti,palmas-usb or ti,twl6035-usb + - compatible : Should be ti,palmas-usb or ti,twl6035-usb or + ti,palmas-usb-vid. So are ti,palmas-usb and ti,twl6035-usb full EHCI controllers then? No. I thought I shouldn't remove those if someone is already using those compatible value. Sigh. Perhaps it's best to be consistent with the existing bad naming then:-( -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] extcon: palmas: Added a new compatible type *ti,palmas-usb-vid*
On 08/12/2013 11:37 PM, Kishon Vijay Abraham I wrote: The Palmas device contains only a USB VID detector, so added a compatible type *ti,palmas-usb-vid*. Dint remove the existing compatible s/Dint/Didn't/ diff --git a/Documentation/devicetree/bindings/extcon/extcon-twl.txt b/Documentation/devicetree/bindings/extcon/extcon-twl.txt PALMAS USB COMPARATOR Required Properties: - - compatible : Should be ti,palmas-usb or ti,twl6035-usb + - compatible : Should be ti,palmas-usb or ti,twl6035-usb or + ti,palmas-usb-vid. So are ti,palmas-usb and ti,twl6035-usb full EHCI controllers then? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 1/3] usb: dwc3: msm: Add device tree binding information
On 08/09/2013 03:53 AM, Ivan T. Ivanov wrote: From: Ivan T. Ivanov iiva...@mm-sol.com MSM USB3.0 core wrapper consist of USB3.0 IP (SNPS) and HS, SS PHY's controll and configuration registers. s/controll/control/ It could operate in device mode (SS, HS, FS) and host mode (SS, HS, FS, LS). diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt +MSM SuperSpeed DWC3 USB SoC controller + +Required properities : +- compatible : sould be qcom,dwc3-hsphy; ... +Required properities : +- compatible : sould be qcom,dwc3-ssphy; I would expect different compatible values to be documented in different files. +Optional properties : +- gdsc-supply : phandle to the globally distributed switch controller + regulator node to the USB controller. Which of the 3 compatible values that were described above is that property optional for? +Sub nodes: +- Sub node for DWC3 USB3 controller. + This sub node is required property for device node. The properties + of this subnode are specified in dwc3.txt. Why not represent the PHY and USB controller as separate top-level nodes? They can point at each-other with phandles if they need to know each-others' identity. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] arm: omap5: dts: add palmas-usb node
On 08/12/2013 04:17 AM, Kishon Vijay Abraham I wrote:
From: Felipe Balbi ba...@ti.com
Without this node, there will be no palmas
driver to notify dwc3 that a cable has
been connected and, without that, dwc3
will never initialize.
diff --git a/arch/arm/boot/dts/omap5-uevm.dts
b/arch/arm/boot/dts/omap5-uevm.dts
interrupt-controller;
#interrupt-cells = 2;
+ extcon_usb3: palmas_usb {
+ compatible = ti,palmas-usb;
This isn't so much a comment on this patch as the Palmas binding:
Presumably, the Palmas device contains a USB VID detector, not a whole
USB controller. I'd expect the compatible value to indicate this more
directly, i.e. be something like ti,palmas-usb-vid or
ti,palmas-usb-vid-detector.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DMA: let filter functions of of_dma_simple_xlate possible check of_node
On 08/02/2013 06:06 AM, Santosh Shilimkar wrote:
On Thursday 01 August 2013 10:00 PM, Richard Zhao wrote:
pass of_phandle_args dma_spec to dma_request_channel in of_dma_simple_xlate,
so the filter function could access of_node in of_phandle_args.
Am just curious the reasoning behind doing so. Can you please expand
above bit more with why you need to change it.
I believe that this patch is attempting to solve an issue I pointed out
with the following patch, which enhances the Tegra DMA controller driver
to support the standard DMA DT bindings:
https://lkml.org/lkml/2013/7/24/7
[PATCH 2/9] dma: tegra20-apbdma: move to generic device tree bindings
The issue is in particular that patch included:
+static bool tegra_dma_filter_fn(struct dma_chan *dc, void *param)
+{
+ if (dc-device-dev-driver == tegra_dmac_driver.driver) {
+ struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
+ unsigned req = *(unsigned *)param;
+
+ tdc-slave_id = req;
+
+ return true;
+ }
+ return false;
+}
Which is checking that the provider of the DMA channel is the correct
DMA controller. The DMA core should be able to work this out, since at
least under DT, the DT property specifies both the DMA controller's
phandle and the DMA specifier, so that DMA core should be able to
validate that it only attempts to match DMA channels for the specified
DMA controller, without each DMA controller driver having to implement a
custom filter function to do that.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: allow DEBUG_UNCOMPRESS for omap2plus
On 07/31/2013 12:46 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130730 16:08]: On 07/30/2013 04:52 PM, Russell King - ARM Linux wrote: On Tue, Jul 30, 2013 at 04:49:18PM -0600, Stephen Warren wrote: From: Stephen Warren swar...@nvidia.com DEBUG_UNCOMPRESS was previously disallowed for omap2plus due to omap2plus.S's use of .data, which is not allowed in the decompressor. Solve this by placing that data into .text when building the file into the decompressor. This relies on .text actually being writable in the decompressor, which it is in practice. Unless you decide to use ZBOOT and flash the zImage. I knew there had to be a catch:-) I have no idea if ZBOOT is a use-case that's relevant to OMAP? On Tegra at least (the same issue applies to the other patch I just sent), that use-case is almost impossible; even if the boot ROM directly booted a kernel, the boot ROM is hard-coded to copy whatever it's booting to SDRAM first, although I suppose if that was a boot-loader it could just jump back to a ROM location. That said, NOR flash is extremely rare on Tegra. So, I don't know if we care about this issue. Is it reasonable to just say If you use ZBOOT, don't enable DEBUG_UNCOMPRESS? Perhaps these patches should not completely remove the !DEBUG_TEGRA_UART from config DEBUG_UNCOMPRESS, but instead say: default y if DEBUG_LL (!DEBUG_TEGRA_UART || !ZBOOT)? I think we're best off removing the remaining uncompress code configured port detection features as the port properties are now defined in kconfig anyways. That simplifies the code quite a bit. If you want to do that with OMAP, I'm happy to drop this patch. For Tegra, automatic determination of the DEBUG_LL UART is rather useful, so I'm not going to give that up:-) -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ARM: allow DEBUG_UNCOMPRESS for omap2plus
From: Stephen Warren swar...@nvidia.com DEBUG_UNCOMPRESS was previously disallowed for omap2plus due to omap2plus.S's use of .data, which is not allowed in the decompressor. Solve this by placing that data into .text when building the file into the decompressor. This relies on .text actually being writable in the decompressor, which it is in practice. Cc: Shawn Guo shawn@linaro.org Signed-off-by: Stephen Warren swar...@nvidia.com --- Note that I have build-tested this with omap2plus_defconfig, but not run-time tested it in any way. --- arch/arm/Kconfig.debug | 2 +- arch/arm/include/debug/omap2plus.S | 4 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug index d8ff7f7..8eb04b1 100644 --- a/arch/arm/Kconfig.debug +++ b/arch/arm/Kconfig.debug @@ -1046,7 +1046,7 @@ config DEBUG_UART_8250_FLOW_CONTROL config DEBUG_UNCOMPRESS bool depends on ARCH_MULTIPLATFORM - default y if DEBUG_LL !DEBUG_OMAP2PLUS_UART + default y if DEBUG_LL help This option influences the normal decompressor output for multiplatform kernels. Normally, multiplatform kernels disable diff --git a/arch/arm/include/debug/omap2plus.S b/arch/arm/include/debug/omap2plus.S index 6d867ae..364ae35 100644 --- a/arch/arm/include/debug/omap2plus.S +++ b/arch/arm/include/debug/omap2plus.S @@ -58,11 +58,15 @@ #define UART_OFFSET(addr) ((addr) 0x00ff) +#if !defined(ZIMAGE) .pushsection .data +#endif omap_uart_phys:.word 0 omap_uart_virt:.word 0 omap_uart_lsr: .word 0 +#if !defined(ZIMAGE) .popsection +#endif .macro addruart, rp, rv, tmp -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: allow DEBUG_UNCOMPRESS for omap2plus
On 07/30/2013 04:52 PM, Russell King - ARM Linux wrote: On Tue, Jul 30, 2013 at 04:49:18PM -0600, Stephen Warren wrote: From: Stephen Warren swar...@nvidia.com DEBUG_UNCOMPRESS was previously disallowed for omap2plus due to omap2plus.S's use of .data, which is not allowed in the decompressor. Solve this by placing that data into .text when building the file into the decompressor. This relies on .text actually being writable in the decompressor, which it is in practice. Unless you decide to use ZBOOT and flash the zImage. I knew there had to be a catch:-) I have no idea if ZBOOT is a use-case that's relevant to OMAP? On Tegra at least (the same issue applies to the other patch I just sent), that use-case is almost impossible; even if the boot ROM directly booted a kernel, the boot ROM is hard-coded to copy whatever it's booting to SDRAM first, although I suppose if that was a boot-loader it could just jump back to a ROM location. That said, NOR flash is extremely rare on Tegra. So, I don't know if we care about this issue. Is it reasonable to just say If you use ZBOOT, don't enable DEBUG_UNCOMPRESS? Perhaps these patches should not completely remove the !DEBUG_TEGRA_UART from config DEBUG_UNCOMPRESS, but instead say: default y if DEBUG_LL (!DEBUG_TEGRA_UART || !ZBOOT)? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] spi: Provide core support for runtime PM during transfers
On 07/28/2013 08:43 AM, Mark Brown wrote: From: Mark Brown broo...@linaro.org Most SPI drivers that implement runtime PM support use identical code to do so: they acquire a runtime PM lock in prepare_transfer_hardware() and then they release it in unprepare_transfer_hardware(). The variations in this are mostly missing error checking and the choice to use autosuspend. Since these runtime PM calls are normally the only thing in the prepare and unprepare callbacks and the autosuspend API transparently does the right thing on devices with autosuspend disabled factor all of this out into the core with a flag to enable the behaviour. Patch 1, 9, 10, 11, Reviewed-by: Stephen Warren swar...@nvidia.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/29/2013 03:05 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130719 11:59]: On 07/19/2013 01:29 AM, Tony Lindgren wrote: I'd vote for keeping the existing behaviour with pinctrl_select_state() when no active state is defined. Yes, I think that will work, since the active state cannot exist before this new scheme is in place. Right. But, this needs to be very clearly spell out in the DT binding documentation: If you have states default/idle/sleep, they're complete alternatives, whereas if you have states default/active/idle/sleep, the latter 3 are alternatives that build on top of the first. I foresee mass confusion, but perhaps I'm being pessimistic. I'm hoping we can automate the runtime PM handling with default/active/idle completely from the consumer driver point of view. And then when that's working, we can probably deprecate any runtime PM related handling using pinctr_select_state() and print warnings. And we can also improve the documentation so no new users will use the default/idle/sleep for runtime PM unless they really want to. I was thinking more about people writing the device trees that define these states; they need to explicitly make the choice re: overlapping states or independent states. We should not plan to obsolete any current usage of overlapping states since that will mean an incompatible change to the DT ABI (deprecate yes so that no more usage is added, but the kernel should still support the old way). -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/29/2013 03:21 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130719 12:05]: On 07/19/2013 01:39 AM, Tony Lindgren wrote: I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances. For the pin muxing part, I'd do this: i2c1 instance i2c2 instance notes default_state 0 pins 0 pins (or dedicated pins only) active_stateall pinsalls pins idle_state safe mode safe mode Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2. I see two issues with that approach: 1) Runtime PM doesn't always put a device into an idle state as soon as its work is done. Sometimes (always?) there is a delay between when the device is last used and when the HW is actually made idle so that if the device is re-activated quickly, the kernel hasn't wasted time making it idle then active again. You'd have to force that delay to complete when switching between the two virtual controller devices. There is the autosuspend timeout for delayed transitions, but I think runtime PM already has calls for making the state change immediate also. True, but I /think/ that then you could never use the APIs that perform delayed idle, since you'd always need to force immediate idle to guarantee you could always immediately switch to the other state/virtual-controller. 2) This requires two separate device objects for the two I2C instances. I guess you could have the driver for the one I2C mux node end up instantiating two child devices for this purpose, and hence make that happen without needing to change the DT ABI. However, that sure feels complex... Yes but you will also automatically get quite a bit of sanity to your I2C driver code rather than trying to handle the two separate instances within the driver alone. Consider things like scanning the I2C buses for devices and just dev_dbg(). The I2C mux is already very simple. IIRC, it instantiates a separate struct i2c_adapter for each virtual I2C controller. It actually looks like each of those already embeds its own struct device, so perhaps this issue is a little moot. However, we'd have to find some way of redirecting pinctrl requests from those child devices to the top-level I2C mux struct device itself, since that's what the pinctrl mapping table entries are defined for. It seems much simpler to just leave the pinctrl stuff controlled by that top-level device, rather than pushing it down to the child virtual devices/adapters. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On 07/19/2013 12:36 AM, Kishon Vijay Abraham I wrote: Hi, On Friday 19 July 2013 11:59 AM, Greg KH wrote: On Fri, Jul 19, 2013 at 11:25:44AM +0530, Kishon Vijay Abraham I wrote: Hi, On Friday 19 July 2013 11:13 AM, Greg KH wrote: On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote: + ret = dev_set_name(phy-dev, %s.%d, dev_name(dev), id); Your naming is odd, no phy anywhere in it? You rely on the sender to never send a duplicate name.id pair? Why not create your own ids based on the number of phys in the system, like almost all other classes and subsystems do? hmm.. some PHY drivers use the id they provide to perform some of their internal operation as in [1] (This is used only if a single PHY provider implements multiple PHYS). Probably I'll add an option like PLATFORM_DEVID_AUTO to give the PHY drivers an option to use auto id. [1] - http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html No, who cares about the id? No one outside of the phy core ever should, because you pass back the only pointer that they really do care about, if they need to do anything with the device. Use that, and then you can hmm.. ok. rip out all of the search for a phy by a string logic, as that's not Actually this is needed for non-dt boot case. In the case of dt boot, we use a phandle by which the controller can get a reference to the phy. But in the case of non-dt boot, the controller can get a reference to the phy only by label. I don't understand. They registered the phy, and got back a pointer to it. Why can't they save it in their local structure to use it again later if needed? They should never have to ask for the device, as the One is a *PHY provider* driver which is a driver for some PHY device. This will use phy_create to create the phy. The other is a *PHY consumer* driver which might be any controller driver (can be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference to the phy (by *phandle* in the case of dt boot and *label* in the case of non-dt boot). device id might be unknown if there are multiple devices in the system. I agree with you on the device id part. That need not be known to the PHY driver. How does a consumer know which label to use in a non-dt system if there are multiple PHYs in the system? That should be passed using platform data. I don't think that's right. That's just like passing clock names in platform data, which I believe is frowned upon. Instead, why not take the approach that e.g. regulators have taken? Each driver defines the names of the objects that it requires. There is a table (registered by a board file) that has lookup key (device name, regulator name), and the output is the regulator device (or global regulator name). This is almost the same way that DT works, except that in DT, the table is represented as properties in the DT. The DT binding defines the names of those properties, or the strings that must be in the foo-names properties, just like a driver in non-DT Linux is going to define the names it expects to be provided with. That way, you don't need platform data to define the names. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] arm: dts: dra7: Add initial status for the devices.
On 07/19/2013 10:24 AM, Sourav Poddar wrote: This patch disabled I2C/SPI/UART device initially(status = disabled). This devices will only be probed, if the devices are present in the dts file(status = okay). interrupts = 0 72 0x4; ti,hwmods = uart1; clock-frequency = 4800; Those lines (and I assume the whole file) is indented with TABs. +status = disabled; Whereas all the new lines use spaces. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/19/2013 04:29 AM, Grygorii Strashko wrote: Hi Tony, Stephen On 07/19/2013 10:39 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130718 12:33]: On 07/18/2013 01:36 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:30]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: ... Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? The idea is that runtime PM will never touch the basic mux settings at all. The default state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device. For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init. So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM. So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses: a) bus 1: I2C controller is muxed out onto one set of pins. b) bus 2: I2C controller is muxed out onto another set of pins. Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to the active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as active... I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances. For the pin muxing part, I'd do this: i2c1 instancei2c2 instancenotes default_state0 pins0 pins(or dedicated pins only) active_stateall pinsalls pins idle_statesafe modesafe mode Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2. First of all, I'd like to mention that these patches do *not* connect pinctrl to PM runtime, so until driver will call pinctrl_select_state() or pinctrl_pm_select_*() there will be no pins state changes. Isn't the whole point of the pinctrl_pm_select*() APIs to eventually be called automatically by the runtime PM core, so that we don't need to write code to do this in every single driver, just like we moved the call to pinctrl_select_state(default) into the device core so that we didn't have to make every device do that manually? (As result, i2c-mux is not good example, seems:)) As such, I think all situations are good examples, because a generic feature has to work in all cases. The description you gave of the behavioural changes this patch creates seems accurate at a quick glance. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/19/2013 01:39 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130718 12:33]: On 07/18/2013 01:36 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:30]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: ... Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? The idea is that runtime PM will never touch the basic mux settings at all. The default state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device. For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init. So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM. So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses: a) bus 1: I2C controller is muxed out onto one set of pins. b) bus 2: I2C controller is muxed out onto another set of pins. Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to the active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as active... I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances. For the pin muxing part, I'd do this: i2c1 instance i2c2 instance notes default_state 0 pins 0 pins (or dedicated pins only) active_state all pinsalls pins idle_statesafe mode safe mode Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2. I see two issues with that approach: 1) Runtime PM doesn't always put a device into an idle state as soon as its work is done. Sometimes (always?) there is a delay between when the device is last used and when the HW is actually made idle so that if the device is re-activated quickly, the kernel hasn't wasted time making it idle then active again. You'd have to force that delay to complete when switching between the two virtual controller devices. 2) This requires two separate device objects for the two I2C instances. I guess you could have the driver for the one I2C mux node end up instantiating two child devices for this purpose, and hence make that happen without needing to change the DT ABI. However, that sure feels complex... I wonder if it wouldn't be better to have active/idle/sleep as modifiers to the state name rather than alternatives, so you end up with states named: default nobus:active nobus:idle nobus:sleep bus0:active bus0:idle bus0:sleep bus1:active bus1:idle bus1:sleep Of course, if you continue on with that approach (i.e. you add more sub-fields each separated by a colon), you end up with a huge combinatorial mess of state names. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/19/2013 01:29 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130718 12:27]: On 07/18/2013 01:25 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:21]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state. Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users. With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime. Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks. Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic(). @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta return 0; if (IS_ERR(state)) return 0; /* No such state */ - ret = pinctrl_select_state(pins-p, state); + + /* Configured for proper dynamic muxing? */ + if (!IS_ERR(dev-pins-active_state)) + ret = pinctrl_select_dynamic(pins-p, state); + else + ret = pinctrl_select_state(pins-p, state); Hmmm. I'm not quite sure this is right... Surely this function should just do nothing if the dynamic states aren't defined? The system should just, and only, be in the default state and never do anything else. This keeps the existing behaviour. We should be able to drop the call to pinctrl_select_state() here after the existing use in drivers has been fixed. How many DT-based systems already have multiple of default/idle/sleep states defined in DT? Right now, since the kernel code uses pinctrl_select_state() to switch between those, the state definitions need to define /all/ pins used by those states, not just the dynamic ones. However, with this series in place, the default state should only include the static pins, and the active/idle/sleep states should only include the dynamic pins. That's a change to the DT bindings, since it changes how the DT must be written, and causes older DTs to be incompatible with newer kernels and vice-versa. Well we can keep the current behaviour with pinctrl_select_state() around as long as needed. For the legacy cases, there is no active state defined and we fall back to pinctrl_select_state(). So, do we just ignore this DT ABI change again, or insist on doing it in some compatible way? DT ABI-ness is a PITA:-( I'd vote for keeping the existing behaviour with pinctrl_select_state() when no active state is defined. Yes, I think that will work, since the active state cannot exist before this new scheme is in place. But, this needs to be very clearly spell out in the DT binding documentation: If you have states default/idle/sleep, they're complete alternatives, whereas if you have states default/active/idle/sleep, the latter 3 are alternatives that build on top of the first. I foresee mass confusion, but perhaps I'm being pessimistic. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/4] Add PWM polarity flag macro for DT
On 07/17/2013 04:54 PM, Laurent Pinchart wrote: Hello, Here's a small patch set that replaces PWM polarity numerical constants with macros in DT. The series, Reviewed-by: Stephen Warren swar...@nvidia.com I'm (very very) slightly hesitant about patch 3/4, since it's moving towards all PWMs having to use the same specifier format, whereas specifiers are at least potentially binding-specific, not device-type-specific. However, consistency is good; there's no need to do something different just for the heck of it. Equally, there's nothing actually stopping a new binding from defining its own format rather than simply deferring to pwm.txt if it absolutely has to, so I think this will turn out fine. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/18/2013 01:25 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:21]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state. Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users. With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime. Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks. Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic(). @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta return 0; if (IS_ERR(state)) return 0; /* No such state */ - ret = pinctrl_select_state(pins-p, state); + + /* Configured for proper dynamic muxing? */ + if (!IS_ERR(dev-pins-active_state)) + ret = pinctrl_select_dynamic(pins-p, state); + else + ret = pinctrl_select_state(pins-p, state); Hmmm. I'm not quite sure this is right... Surely this function should just do nothing if the dynamic states aren't defined? The system should just, and only, be in the default state and never do anything else. This keeps the existing behaviour. We should be able to drop the call to pinctrl_select_state() here after the existing use in drivers has been fixed. How many DT-based systems already have multiple of default/idle/sleep states defined in DT? Right now, since the kernel code uses pinctrl_select_state() to switch between those, the state definitions need to define /all/ pins used by those states, not just the dynamic ones. However, with this series in place, the default state should only include the static pins, and the active/idle/sleep states should only include the dynamic pins. That's a change to the DT bindings, since it changes how the DT must be written, and causes older DTs to be incompatible with newer kernels and vice-versa. So, do we just ignore this DT ABI change again, or insist on doing it in some compatible way? DT ABI-ness is a PITA:-( -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/18/2013 01:36 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:30]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: ... Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? The idea is that runtime PM will never touch the basic mux settings at all. The default state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device. For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init. So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM. So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses: a) bus 1: I2C controller is muxed out onto one set of pins. b) bus 2: I2C controller is muxed out onto another set of pins. Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to the active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as active... -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
On 07/17/2013 05:00 AM, Laurent Pinchart wrote: On Monday 15 July 2013 21:39:31 Stephen Warren wrote: ... But then there's a problem where people assume that the common flags are always available, and somewhere they aren't... Care is needed in the choice of which common flags to define and/or how they're used. Exactly. That's why I think listing the supported common flags in individual bindings makes sense when some of the flags are not supported by all devices. As the only PWM flags currently used are common to all PWM devices I can leave this out now. I have no strong preference, I'll follow your opinion on this. Yes, I guess separating the concept of defining common flags and which devices use them is good. And then indeed individual devices need to define which of the common flags they support. I'd still like to see the *definition* of those common flags in some central place (i.e. pwm.txt or a header that defines constants for it), and the other device bindings simply reference that for the actual definitions. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] pinctrl: Allow pinctrl to have multiple active states
On 07/16/2013 03:05 AM, Tony Lindgren wrote:
It's quite common that we need to dynamically change some pins for a
device for runtime PM, or toggle a pin between rx and tx. Changing all
the pins for a device is not efficient way of doing it.
So let's allow setting up multiple active states for pinctrl. Currently
we only need PINCTRL_STATIC and PINCTRL_DYNAMIC, where PINCTRL_STATIC
covers the current default pins, and PINCTRL_DYNAMIC holds the dynamic
pins that need to be toggled.
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
+enum pinctr_states {
s/pinctr/pinctrl/
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/16/2013 03:05 AM, Tony Lindgren wrote:
To toggle dynamic states, let's add the optional active state in
addition to the static default state. Then if the optional active
state is defined, we can require that idle and sleep states cover
the same pingroups as the active state.
Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic()
to use instead of pinctrl_select() to avoid breaking existing users.
With pinctrl_check_dynamic() we can check that idle and sleep states
match the active state for pingroups during init, and don't need to
do it during runtime.
Then with the states pre-validated, pinctrl_select_dynamic() can
just toggle between the dynamic states without extra checks.
Note that pinctr_select_state() still has valid use cases, such as
changing states when the pins can be shared between two drivers
and don't necessarily cover the same pingroups. For dynamic runtime
toggling of pin states, we should eventually always use just
pinctrl_select_dynamic().
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
@@ -887,6 +887,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
list_for_each_entry_safe(setting, n2, state-settings, node) {
pinctrl_free_setting(state == p-state[PINCTRL_STATIC],
setting);
+ pinctrl_free_setting(state == p-state[PINCTRL_DYNAMIC],
+ setting);
This will cause pinmux_free_setting() or pinconf_free_setting() to be
called twice on the setting object. Right now they don't do anything,
but if they start to kfree() some dynamically-allocated data that's
stored within the setting, that'll cause problems. I would suggest a
loop body something more like:
bool disable_setting = state == XXX || state == YYY;
pinctrl_free_setting(disable_setting, setting);
+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+ struct pinctrl_state *st2)
+{
+ struct pinctrl_setting *s1, *s2;
+
+ list_for_each_entry(s1, st1-settings, node) {
...
+ list_for_each_entry(s2, st2-settings, node) {
...
+ if (pctldev1 != pctldev2) {
+ dev_dbg(dev, pctldev must be the same for
states\n);
+ return -EINVAL;
+ }
I don't think we should require that; it's perfectly legal at the moment
for some device's pinctrl configuration to include settings from
multiple different pin controllers.
+ for (i = 0; i num_pins1; i++) {
+ int pin1 = pins1[i];
+
+ for (j = 0; j num_pins2; j++) {
+ int pin2 = pins2[j];
+
+ if (pin1 == pin2) {
+ found++;
+ break;
+ }
+ }
+ }
+
+ if (found != num_pins1) {
I guess this make sure that every entry in the dynamic state is in the
state state, but not vice-versa; the static state can affect more stuff
than the dynamic state?
If so, shouldn't that check be if (found != num_pins2)?
+int pinctrl_select_dynamic(struct pinctrl *p, struct pinctrl_state *state)
The body of this function is rather duplicated with pinctrl_select().
@@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev,
struct pinctrl_state *sta
return 0;
if (IS_ERR(state))
return 0; /* No such state */
- ret = pinctrl_select_state(pins-p, state);
+
+ /* Configured for proper dynamic muxing? */
I don't think proper is correct here; it's just fine not to have any
dynamic configuration.
+ if (!IS_ERR(dev-pins-active_state))
+ ret = pinctrl_select_dynamic(pins-p, state);
+ else
+ ret = pinctrl_select_state(pins-p, state);
Hmmm. I'm not quite sure this is right... Surely this function should
just do nothing if the dynamic states aren't defined? The system should
just, and only, be in the default state and never do anything else.
Looking back at patch 1, I see the are
pinctrl_pm_select_{default,sleep,idle}_state(). Shouldn't that list be
active/sleep/idle, since I assume default is that static state, and
the other three are the dynamic states?
+static int pinctrl_pm_check_state(struct device *dev,
+ struct pinctrl_state *state)
Naming this pinctrl_pm_is_state_error() or pinctrl_pm_is_state_ok()
might give more clue what its return value is expected to be.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] drivers: Add pinctrl handling for dynamic pin states
On 07/16/2013 03:05 AM, Tony Lindgren wrote:
We want to have static pin states handled separately from
dynamic pin states, so let's add optional state_active.
Then if state_active is defined, let's check and make sure
state_idle and state_sleep match state_active for the
pin groups to avoid checking them during runtime as the
active and idle pins may need to be toggled for many
devices every time we enter and exit idle.
+ * Note that if active state is defined, sleep and idle states must
+ * cover the same pin groups as active state.
*/
dev-pins-sleep_state = pinctrl_lookup_state(dev-pins-p,
PINCTRL_STATE_SLEEP);
- if (IS_ERR(dev-pins-sleep_state))
+ if (IS_ERR(dev-pins-sleep_state)) {
/* Not supplying this state is perfectly legal */
dev_dbg(dev, no sleep pinctrl state\n);
+ } else if (!IS_ERR(dev-pins-active_state)) {
+ ret = pinctrl_check_dynamic(dev, dev-pins-active_state,
+ dev-pins-sleep_state);
Oh, I see you're trying to check that the set of pins in the active,
sleep, and idle states are identical.
But I think that pinctrl_check_dynamic() only checks that one state is a
subset of the other, not that the two states are equal. Instead, I think
you want to comparison coded in pinctrl_check_dynamic() to be:
gen_group_list_of_pinctrl_state(s1, array1);
gen_group_list_of_pinctrl_state(s2, array2);
mismatch = memcmp(array1, array2, length);
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/16/2013 03:05 AM, Tony Lindgren wrote: To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state. Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users. With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime. Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks. Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic(). Something in this series should edit Documentation/pinctrl.txt to explain the philosophy behind the static/dynamic state split. That philosophy and/or semantics are more important to review than the code... Related to that, I'm worried that using pinctrl_select_state() and pinctrl_select_dynamic() appear to be mutually-exclusive options here. Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
On 07/15/2013 07:10 PM, Laurent Pinchart wrote: On Friday 12 July 2013 08:42:41 Stephen Warren wrote: ... I think the values for any common system-wide flags should be defined once in some system-wide place, and the values for any HW-specific values should be defined only in the documentation for that specific HW. You could try and avoid conflicts by either: a) Allocating system-wide flags from bit 0 up, and HW-specific flags from bit 31 down. or: b) Using 1 cell for standard flags, and a separate cell for any HW-specific flags. Drivers can quite easily adapt to adding extra cells to #pwm-cells, thus making adding a HW-specific cell later backwards-compatible. I wasn't referring to HW-specific flags, but rather to system-wide flags that might not be supported by all drivers. If we later add new system-wide flags I think the individual DT bindings should explicitly document which flags they support. Shouldn't all system-wide flags be supported by all HW, perhaps being implemented by the core subsystem rather than individual drivers to ensure that? Consider the case of the GPIO active-low flag which is actually implemented in SW, hence can work with any GPIO controller. Perhaps that's not possible in all cases, in which case, yes, it does make sense to define which of the common flags a particular HW module supports. But then there's a problem where people assume that the common flags are always available, and somewhere they aren't... Care is needed in the choice of which common flags to define and/or how they're used. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
On 07/12/2013 04:41 AM, Laurent Pinchart wrote:
Hi Stephen,
On Thursday 11 July 2013 11:40:37 Stephen Warren wrote:
On 07/11/2013 08:37 AM, Laurent Pinchart wrote:
Define PWM_POLARITY_NORMAL and PWM_POLARITY_INVERTED macros in
include/dt-bindings/pwm/pwm.h to be used by device tree sources.
Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt | 6 +++---
Documentation/devicetree/bindings/pwm/pwm-samsung.txt | 5 +++--
Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 5 +++--
Documentation/devicetree/bindings/pwm/pwm.txt | 8 +---
Documentation/devicetree/bindings/pwm/vt8500-pwm.txt| 5 +++--
arch/arm/boot/dts/am335x-evm.dts| 3 ++-
arch/arm/boot/dts/am335x-evmsk.dts | 3 ++-
arch/arm/boot/dts/wm8850-w70v2.dts | 3 ++-
include/dt-bindings/pwm/pwm.h | 15
include/linux/pwm.h | 4 ++--
I think this needs to be separate patches; at least the new pwm.h should
be introduced separately to the board-specific *.dts edits, and perhaps
further split up?
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.
That way, the one patch that introduces dt-bindings/pwm.h would be
available to be merged into any other tree that wanted to take patches
to use the new defines.
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).
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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
On 07/12/2013 05:01 AM, Laurent Pinchart wrote: Hi, On Thursday 11 July 2013 14:06:44 Stephen Warren wrote: On 07/11/2013 01:32 PM, Thierry Reding wrote: On Thu, Jul 11, 2013 at 11:50:48AM -0600, Stephen Warren wrote: On 07/11/2013 09:36 AM, Thierry Reding wrote: On Thu, Jul 11, 2013 at 04:37:48PM +0200, Laurent Pinchart wrote: [...] diff --git a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt index de0eaed..be09be4 100644 --- a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt +++ b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt @@ -4,9 +4,9 @@ Required properties: - compatible: should be atmel,tcb-pwm - #pwm-cells: Should be 3. The first cell specifies the per-chip index of the PWM to use, the second cell is the period in nanoseconds and - bit 0 in the third cell is used to encode the polarity of PWM output. - Set bit 0 of the third cell in PWM specifier to 1 for inverse polarity - set to 0 for normal polarity. + the third cell is used to encode the polarity of PWM output. Set the + PWM_POLARITY_NORMAL flag for normal polarity or the PWM_POLARITY_INVERSED + flag for inverted polarity. PWM flags are defined in dt-bindings/pwm/pwm.h. - tc-block: The Timer Counter block to use as a PWM chip. Example: I'd prefer for the original text to stay in place and the reference to the dt-bindings/pwm/pwm.h file to go below that block. I disagree here. The whole point of creating header files for the constants in binding definitions was so that you wouldn't have to duplicate all the values into the binding definitions. Rather, you'd simply say see dt-bindings/xxx.h. But that's not something that this patch solves. Well, if the comments I made on the patch re: that linux/pwm.h should simply #include dt-bindings/pwm/pwm.h instead of duplicating the constants, then yet this patch will solve that. There will be a single place where the constants are defined. As explained in another reply, this would require replacing the enum with an unsigned int. I can write a patch if we agree on this. And it could be solved even in the absence of the header file defining the symbolic constants. If all the standard flags that dt-bindings/pwm/pwm.txt now specifies were to be listed in pwm.txt (they actually are) then referring to that document as the canonical source works equally well. If that's all the happens, then there will still be duplication between pwm.txt and linux/pwm.h. I've explicitly mentioned the flags in individual DT bindings to ease adding new flags in the future. At the moment the defined flags are either all supported or not used at all by drivers. If we later add a new flag supported by a subset of drivers only the driver bindings should list supported flags for each driver. I'm fine with removing the explicit mentions of individual flags right now and adding it back when needed if you think that's better. I think the values for any common system-wide flags should be defined once in some system-wide place, and the values for any HW-specific values should be defined only in the documentation for that specific HW. You could try and avoid conflicts by either: a) Allocating system-wide flags from bit 0 up, and HW-specific flags from bit 31 down. or: b) Using 1 cell for standard flags, and a separate cell for any HW-specific flags. Drivers can quite easily adapt to adding extra cells to #pwm-cells, thus making adding a HW-specific cell later backwards-compatible. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
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...
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
On 07/11/2013 08:37 AM, Laurent Pinchart wrote:
Define PWM_POLARITY_NORMAL and PWM_POLARITY_INVERTED macros in
include/dt-bindings/pwm/pwm.h to be used by device tree sources.
Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt | 6 +++---
Documentation/devicetree/bindings/pwm/pwm-samsung.txt | 5 +++--
Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 5 +++--
Documentation/devicetree/bindings/pwm/pwm.txt | 8 +---
Documentation/devicetree/bindings/pwm/vt8500-pwm.txt| 5 +++--
arch/arm/boot/dts/am335x-evm.dts| 3 ++-
arch/arm/boot/dts/am335x-evmsk.dts | 3 ++-
arch/arm/boot/dts/wm8850-w70v2.dts | 3 ++-
include/dt-bindings/pwm/pwm.h | 15 +++
include/linux/pwm.h | 4 ++--
I think this needs to be separate patches; at least the new pwm.h should
be introduced separately to the board-specific *.dts edits, and perhaps
further split up?
That way, the one patch that introduces dt-bindings/pwm.h would be
available to be merged into any other tree that wanted to take patches
to use the new defines.
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?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
On 07/11/2013 09:36 AM, Thierry Reding wrote: On Thu, Jul 11, 2013 at 04:37:48PM +0200, Laurent Pinchart wrote: [...] diff --git a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt index de0eaed..be09be4 100644 --- a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt +++ b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt @@ -4,9 +4,9 @@ Required properties: - compatible: should be atmel,tcb-pwm - #pwm-cells: Should be 3. The first cell specifies the per-chip index of the PWM to use, the second cell is the period in nanoseconds and - bit 0 in the third cell is used to encode the polarity of PWM output. - Set bit 0 of the third cell in PWM specifier to 1 for inverse polarity - set to 0 for normal polarity. + the third cell is used to encode the polarity of PWM output. Set the + PWM_POLARITY_NORMAL flag for normal polarity or the PWM_POLARITY_INVERSED + flag for inverted polarity. PWM flags are defined in dt-bindings/pwm/pwm.h. - tc-block: The Timer Counter block to use as a PWM chip. Example: I'd prefer for the original text to stay in place and the reference to the dt-bindings/pwm/pwm.h file to go below that block. I disagree here. The whole point of creating header files for the constants in binding definitions was so that you wouldn't have to duplicate all the values into the binding definitions. Rather, you'd simply say see dt-bindings/xxx.h. A comment on the original patch: You're creating dt-bindings/pwm/pwm.h. That filename sounds entirely generic, as if the values are defining something PWM-wide rather than specific to individual PWM bindings. As such, I think the text in each individual binding should simply say something like: - the third cell encodes standard PWM flags, as defined in dt-bindings/pwm/pwm.h. - Similarly, pwm.txt should mention that standard flags exist, and refer to that same file for definitions. The reason is that perhaps somebody will look at an older version of a given DT (with the numerical value) and not have access to the include and therefore not know which flag was being set by just looking at the documentation. It's pretty trivial to find the header that defines the values; just grab the latest source. If you're looking at an old version of the .dts file, it's almost certain that's within the context of an old Linux kernel soruce tree, in which case you trivially have access to the old version of the binding document that spelled out the values. I'm also not sure about what the plans are with respect to shipping device trees in a separate repository and if they are, whether that repository would ship the includes as well. It would have to. That separate repository would be upstream for dt-bindings/ files, which would be imported into the kernel periodically as drivers wanted to make use of any new headers/values defined there. Another issue might be that people without access to recent versions of DTC won't be able to use the new #include feature, so keeping the documentation backwards compatible seems like a good idea. The dtc source tree is duplicated into the kernel source tree, so that isn't an issue for now. Besides, the dtc version is an entirely unrelated issue to how the documentation is written. Perhaps the include file should even only be mentioned in the general PWM binding documentation. I wondered about that too. It's slightly indirect in that you'd read foo-pwm.txt which would say something like: - the third cell encodes standard PWM flags, as defined by the core PWM binding in pwm.txt in this directory. - and pwm.txt would say: - The PWM specifier should include a flags cell to contain standard PWM flags. Values for that cell are defined in dt-bindings/pwm/pwm.h. - Which is somewhat like following a lot of symlinks. Still, I agree that's arguably the correct thing to do. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT
On 07/11/2013 01:32 PM, Thierry Reding wrote: On Thu, Jul 11, 2013 at 11:50:48AM -0600, Stephen Warren wrote: On 07/11/2013 09:36 AM, Thierry Reding wrote: On Thu, Jul 11, 2013 at 04:37:48PM +0200, Laurent Pinchart wrote: [...] diff --git a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt index de0eaed..be09be4 100644 --- a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt +++ b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt @@ -4,9 +4,9 @@ Required properties: - compatible: should be atmel,tcb-pwm - #pwm-cells: Should be 3. The first cell specifies the per-chip index of the PWM to use, the second cell is the period in nanoseconds and - bit 0 in the third cell is used to encode the polarity of PWM output. - Set bit 0 of the third cell in PWM specifier to 1 for inverse polarity - set to 0 for normal polarity. + the third cell is used to encode the polarity of PWM output. Set the + PWM_POLARITY_NORMAL flag for normal polarity or the PWM_POLARITY_INVERSED + flag for inverted polarity. PWM flags are defined in dt-bindings/pwm/pwm.h. - tc-block: The Timer Counter block to use as a PWM chip. Example: I'd prefer for the original text to stay in place and the reference to the dt-bindings/pwm/pwm.h file to go below that block. I disagree here. The whole point of creating header files for the constants in binding definitions was so that you wouldn't have to duplicate all the values into the binding definitions. Rather, you'd simply say see dt-bindings/xxx.h. But that's not something that this patch solves. Well, if the comments I made on the patch re: that linux/pwm.h should simply #include dt-bindings/pwm/pwm.h instead of duplicating the constants, then yet this patch will solve that. There will be a single place where the constants are defined. And it could be solved even in the absence of the header file defining the symbolic constants. If all the standard flags that dt-bindings/pwm/pwm.txt now specifies were to be listed in pwm.txt (they actually are) then referring to that document as the canonical source works equally well. If that's all the happens, then there will still be duplication between pwm.txt and linux/pwm.h. If we can take both of the above for granted, then sure, let's refer to the header from within the generic pwm.txt file and add a reference to that in bindings for drivers that use the standard flags. Another issue might be that people without access to recent versions of DTC won't be able to use the new #include feature, so keeping the documentation backwards compatible seems like a good idea. The dtc source tree is duplicated into the kernel source tree, so that isn't an issue for now. Besides, the dtc version is an entirely unrelated issue to how the documentation is written. Well, not really. If the documentation specifies the binding in a way that the DTC can't handle that's still a problem. People will end up with a DTS that their DTC can't compile. I guess that can be resolved by adding a note to the upstream device tree repository about the minimum required version of DTC. Yes, the separate repository would obviously require a version of dtc that's able to compile the files there; i.e. a version equivalent to what's already in the kernel tree (upstream 1.4.0 specifically). Again, right now, all of the binding docs, the *.dts files, and the dtc required to use them are part of the kernel; a single package, so there's no scope for issues re: using dtc features that aren't supported. If those components get separated later, obviously there will be a requirement to install a specific version of dtc to use with the separated *.dts and binding files. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] MFD: Palmas: Check if irq is valid
On 06/18/2013 11:57 PM, Keerthy wrote: From: J Keerthy j-keer...@ti.com Check if irq value obtained is valid. If it is not valid then skip the irq request step and go ahead with the probe. Reviewed-by: Stephen Warren swar...@nvidia.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property exists and then only request irq
On 06/18/2013 04:01 AM, J Keerthy wrote: Check if interrupts property exists and then only request irq. On some boards INT line might not be connected to a valid irq line on the application processor. Hence keeping a check before requesting irq. When there is no interrupts property, surely i2c-irq == 0, which is an invalid IRQ, and hence there's no need to check this before copying the value? In other words, I think this whole patch could just be: + palmas-irq = i2c-irq; right? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] MFD: Palmas: Add SMPS10_BOOST feature
On 06/18/2013 04:03 AM, J Keerthy wrote:
The SMPS10 regulator is not presesnt in all the variants
of the PALMAS PMIC family. Hence adding a feature to distingush
between them.
diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
+ match = of_match_device(of_match_ptr(of_palmas_match_tbl), i2c-dev);
+
+ if (match) {
+ features = (unsigned int *)match-data;
+ palmas-features = *features;
+ } else {
+ return -ENODATA;
+ }
I think it's more common to do the error-checking first. That way, the
success-case code doesn't need an indent:
match = of_match...(...);
if (!match)
return -ENODATA;
features = ...;
palmas-features = *features;
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property exists and then only request irq
On 06/18/2013 10:54 AM, J, KEERTHY wrote: Hi Stephen, -Original Message- From: Stephen Warren [mailto:swar...@wwwdotorg.org] Sent: Tuesday, June 18, 2013 9:22 PM To: J, KEERTHY Cc: linux-omap@vger.kernel.org; broo...@kernel.org; ldewan...@nvidia.com; sa...@linux.intel.com; grant.lik...@secretlab.ca; swar...@nvidia.com; linux-ker...@vger.kernel.org; linux- d...@vger.kernel.org; g...@slimlogic.co.uk Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property exists and then only request irq On 06/18/2013 04:01 AM, J Keerthy wrote: Check if interrupts property exists and then only request irq. On some boards INT line might not be connected to a valid irq line on the application processor. Hence keeping a check before requesting irq. When there is no interrupts property, surely i2c-irq == 0, which is an invalid IRQ, and hence there's no need to check this before copying the value? The intent here is NOT to request irq with 0 or Invalid IRQ. Sure. The board File will not populate the interrupts entry if the INT line is not Connected. Do you mean the interrupts DT property won't be present if there is no interrupt. If so, sure. Hence the patch checks for the 'interrupts' property. That shouldn't be necessary; IIRC, the I2C core has already parsed the interrupts property if there was one, and if there wasn't, it has set i2c-irq to some invalid value already. So, you simply need to check the value in i2c-irq, and don't need to look at the DT at all. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property exists and then only request irq
On 06/18/2013 11:19 AM, J, KEERTHY wrote: -Original Message- From: Stephen Warren [mailto:swar...@wwwdotorg.org] Sent: Tuesday, June 18, 2013 10:38 PM To: J, KEERTHY Cc: linux-omap@vger.kernel.org; broo...@kernel.org; ldewan...@nvidia.com; sa...@linux.intel.com; grant.lik...@secretlab.ca; swar...@nvidia.com; linux-ker...@vger.kernel.org; linux- d...@vger.kernel.org; g...@slimlogic.co.uk Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property exists and then only request irq On 06/18/2013 10:54 AM, J, KEERTHY wrote: Hi Stephen, -Original Message- From: Stephen Warren [mailto:swar...@wwwdotorg.org] Sent: Tuesday, June 18, 2013 9:22 PM To: J, KEERTHY Cc: linux-omap@vger.kernel.org; broo...@kernel.org; ldewan...@nvidia.com; sa...@linux.intel.com; grant.lik...@secretlab.ca; swar...@nvidia.com; linux-ker...@vger.kernel.org; linux- d...@vger.kernel.org; g...@slimlogic.co.uk Subject: Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property exists and then only request irq On 06/18/2013 04:01 AM, J Keerthy wrote: Check if interrupts property exists and then only request irq. On some boards INT line might not be connected to a valid irq line on the application processor. Hence keeping a check before requesting irq. When there is no interrupts property, surely i2c-irq == 0, which is an invalid IRQ, and hence there's no need to check this before copying the value? The intent here is NOT to request irq with 0 or Invalid IRQ. Sure. The board File will not populate the interrupts entry if the INT line is not Connected. Do you mean the interrupts DT property won't be present if there is no interrupt. If so, sure. Yes. Hence the patch checks for the 'interrupts' property. That shouldn't be necessary; IIRC, the I2C core has already parsed the interrupts property if there was one, and if there wasn't, it has set i2c-irq to some invalid value already. So, you simply need to check the value in i2c-irq, and don't need to look at the DT at all. Instead of checking the Invalid irq value which most likely can be 0. I am not sure. I am explicitly checking if the interrupts property exists or not. If not present then It throws out a warning. Either there is no Valid INT line connection or the DeviceTree was not populated fully. This additional piece of information is good to have in the driver IMHO. Let me know if this is rational enough to have in the driver. No, you should just check the IRQ number. Consider this: If the device was instantiated from a board file *or* a device tree, i2c-irq is correctly set. Hence, checking that value works in both cases. If you check the interrupts DT property, that will only work if the device was instantiated from device tree, and not if it was instantiated from a board file; the property will never exist in the board file case, and hence you'll never be able to have a board file provide an interrupt. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] MFD: Palmas: Check if interrupts property exists and then only request irq
On 06/18/2013 11:33 AM, J, KEERTHY wrote: Stephen Warren wrote at Tuesday, June 18, 2013 10:53 PM: ... No, you should just check the IRQ number. Hmmm...so something like (!i2c-irq) Yes. Consider this: If the device was instantiated from a board file *or* a device tree, i2c-irq is correctly set. Hence, checking that value works in both cases. If you check the interrupts DT property, that will only work if the device was instantiated from device tree, and not if it was instantiated from a board file; the property will never exist in the board file case, and hence you'll never be able to have a board file provide an interrupt. The board file approach is getting deprecated for this. I Myself removed board file related pdata stuff in one of the patches. http://www.mail-archive.com/linux-omap@vger.kernel.org/msg90598.html So going the DeviceTree way. Even if you're 100% sure this driver will only ever work with DT (which seems like a bad assumption to make no matter what the circumstance), it'd still be best to detect whether an IRQ was specified in a generic way. That way, nobody will read this driver, assume the code is generic, and just copy/paste it without thinking. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] ARM: dts: OMAP5: Add palmas MFD node and regulator nodes
On 06/12/2013 02:19 AM, J Keerthy wrote:
This patch adds Palmas MFD node and the regulator nodes for OMAP5.
The node definitions are based on: https://lkml.org/lkml/2013/6/6/25
Boot tested on omap5-uevm board.
Reviewed-by: Stephen Warren swar...@nvidia.com
diff --git a/arch/arm/boot/dts/omap5-uevm.dts
b/arch/arm/boot/dts/omap5-uevm.dts
+ palmas: palmas@48 {
+ reg = 0x48;
+ interrupts = GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH; /* IRQ_SYS_1N */
+ interrupt-parent = gic;
+ compatible = ti,palmas;
... although personally I prefer the compatible property to be right at
the top of each node, since it's what implies the schema for the rest of
the node. That's a tiny nit though; feel free to ignore it if the OMAP
files don't follow that convention.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: dts: OMAP5: Add palmas MFD node and regulator nodes
On 06/10/2013 11:30 PM, J Keerthy wrote:
This patch adds Palmas MFD node and the regulator nodes for OMAP5.
The node definitions are based on: https://lkml.org/lkml/2013/6/6/25
Boot tested on omap5-uevm board.
diff --git a/arch/arm/boot/dts/omap5-uevm.dts
b/arch/arm/boot/dts/omap5-uevm.dts
+ palmas: palmas@48 {
+ reg = 0x48;
+ interrupts = GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH; /* IRQ_SYS_1N */
+ interrupt-parent = gic;
+ };
+};
+
+palmas {
+ compatible = ti,palmas;
+ interrupt-controller;
+ #interrupt-cells = 2;
I don't really see the point of splitting the node into two parts if
it's all going into a single file. It made sense if part of the node
came from a common .dtsi file, but not so much when it doesn't.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] ARM: dts: Add headers with constants for MTD partitions
On 06/11/2013 08:48 AM, Florian Vaussard wrote: These constants can be used to easily declare MTD partitions inside DTS. The constants MTDPART_OFS_* are purposely not included. Indeed, parse_ofpart_partitions() is expecting u64, but a DT cell is u32. Negative constants, as defined by MTDPART_OFS_*, would be wrongly interpreted by parse_ofpart_partitions(). Two cells should be used to correctly encode the negative constants, but this breaks current usage. I think addition of common headers like this needs an ack from Grant/Rob. I CC'd them here. diff --git a/include/dt-bindings/mtd/partitions.h b/include/dt-bindings/mtd/partitions.h + * This header provides constants used with MTD partitions. ... +/* Partition size */ +#define MTDPART_SIZ_FULL 0 Which binding document in Documentation/devicetree/bindings is this definition associated with? The comment above should really mention this. Documentation/devicetree/bindings/mtd/partition.txt doesn't seem to mention this value. diff --git a/include/dt-bindings/sizes.h b/include/dt-bindings/sizes.h ... +#define SZ_1G0x4000 +#define SZ_2G0x8000 + +#endif For MTD partitions specifically, SZ_4G and onwards would be useful in theory, although that would end up putting two cell values into a single macro. and then the values couldn't be added/or'd together. So, I'm not really sure if we want to add those larger values, but food for thought... -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] ARM: dts: OMAP3: Use MTD constants for OMAP3 boards
On 06/11/2013 08:48 AM, Florian Vaussard wrote: Use the MTD constants for NAND and OneNAND nodes used in OMAP3 DTS. I don't quite understand the split between patches 2/3 and 3/3; isn't the edit to omap3-overo.dtsi (part of) a board file, and hence logically part of this patch? I'd be tempted just to squash the two together. But, this is a nit; not a big deal. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: dts: add dtsi for palmas
On 06/10/2013 03:29 AM, Benoit Cousson wrote:
Hi Keerthy,
On 06/10/2013 06:03 AM, J, KEERTHY wrote:
Hi Stephen,
Thanks for the review comments.
From: Stephen Warren [swar...@wwwdotorg.org]
Sent: Saturday, June 08, 2013 1:26 AM
To: J, KEERTHY
Cc: Cousson, Benoit; devicetree-disc...@lists.ozlabs.org;
linux-omap@vger.kernel.org; linux-ker...@vger.kernel.org;
ldewan...@nvidia.com; grant.lik...@secretlab.ca; swar...@nvidia.com;
sa...@linux.intel.com; g...@slimlogic.co.uk; lee.jo...@linaro.org
Subject: Re: [PATCH] ARM: dts: add dtsi for palmas
On 06/07/2013 05:28 AM, J Keerthy wrote:
Adds palmas mfd and palmas regulator nodes. This is
based on the patch series:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg89957.html
The device tree nodes are based on:
https://lkml.org/lkml/2013/6/6/25
diff --git a/arch/arm/boot/dts/palmas.dtsi b/arch/arm/boot/dts/palmas.dtsi
+palmas {
Hmmm. That (i.e. requiring the board file to declare the node, then
setting up all the content by later including this file) is an
interesting approach. I guess it's reasonable. The one issue is that it
makes it a little harder for the board file to override any of the
properties in this file., although it certainly is possible by including
those overrides after the include.
Irrespective of that, some comments on this:
+ palmas_pmic {
+ ti,ldo6-vibrator;
For example, what if the board doesn't want to have the property set?
+
+ regulators {
+ smps123_reg: smps123 {
+ regulator-name = smps123;
+ regulator-min-microvolt = 60;
+ regulator-max-microvolt = 150;
Or what if the board wants to limit the voltage range of this regulator
due to what it's used for on the board.
+ regulator-always-on;
+ regulator-boot-on;
And those two properties are almost certainly board-specific policy.
Totally agree to all the above concerns. So can we have a custom .dtsi file
for a board+pmic combination? Or have only the required properties over
ridden
in the board file?
Yes, you can do that potentially if most OMAP5 boards will reuse the
same kind of settings. Kevin has just done that for OMAP3 + twl4030.
In this case, since we do have only one board, I'm not sure it worth the
effort.
IIUC, various Tegra boards use this PMIC, so there's more than one board
in question here.
Or were you talking about e.g.:
palmas.dtsi - base Palmas DT node for everything
omap-palmas.dtsi - common Palmas settings for all OMAP boards
... where the latter file would only be used by one board? If so, then
yes, creating the latter file might not make sense yet.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: dts: add dtsi for palmas
On 06/09/2013 10:03 PM, J, KEERTHY wrote: Hi Stephen, Thanks for the review comments. Cam you please fix your email client to quote the messages you're replyiing to correctly? In the message you sent, there's no differentiation between what I originally wrote and you quoted, vs. the new text you wrote as a response. That makes the message very confusing to read... -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] ARM: dts: add dtsi for palmas
On 06/10/2013 03:04 AM, J Keerthy wrote:
Adds palmas mfd and palmas regulator nodes.
Nits:
Well, you're really adding files that provide the nodes, not the nodes
themselves.
Palmas and MFD should be correctly capitalized.
diff --git a/arch/arm/boot/dts/palmas.dtsi b/arch/arm/boot/dts/palmas.dtsi
+palmas {
...
+ palmas_pmic {
...
+ ti,ldo6-vibrator;
Isn't that board-specific? I don't know the HW well enough to say, but I
assume that since there's a property, the whole point must be that some
boards want it set and some don't.
+ regulators {
+ smps123_reg: smps123 {
+ regulator-name = smps123;
+ };
So the node labels here duplicate those in omap5-uevm.dts in patch 2/2.
While dtc allows this, I don't think there's much point duplicating the
labels. I'd tend to only put the labels in omap5-uevm.dts and not put
them here. That will completely avoid the possibility of the labels in
this file from conflicting with any other labels in any top-level
board.dts file.
I also wonder if this file is actually terribly useful. The only thing
that this file provides is the regulator-name property. It seems simpler
just to put that into each board.dts file. The added advantage of doing
that, is that if a board doesn't use a particular regulator, the node
won't appear, and the regulator won't get registered.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: dts: add dtsi for palmas
On 06/07/2013 06:27 AM, Benoit Cousson wrote:
Hi Keerthy,
On 06/07/2013 01:28 PM, J Keerthy wrote:
Adds palmas mfd and palmas regulator nodes. This is
based on the patch series:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg89957.html
The device tree nodes are based on:
https://lkml.org/lkml/2013/6/6/25
...
diff --git a/arch/arm/boot/dts/palmas.dtsi b/arch/arm/boot/dts/palmas.dtsi
...
+palmas {
+compatible = ti,palmas;
+interrupt-controller;
+#interrupt-cells = 2;
The I2C address should be there as well. We used to put it in the board
file, but this is really an I2C device specific information and not
board specific in that case.
I thought the I2C address was variable depending on OTP settings, and
hence the I2C address is board-specific; depending on which variant of
the PMIC was stuffed onto the board?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: dts: add dtsi for palmas
On 06/07/2013 05:28 AM, J Keerthy wrote:
Adds palmas mfd and palmas regulator nodes. This is
based on the patch series:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg89957.html
The device tree nodes are based on:
https://lkml.org/lkml/2013/6/6/25
diff --git a/arch/arm/boot/dts/palmas.dtsi b/arch/arm/boot/dts/palmas.dtsi
+palmas {
Hmmm. That (i.e. requiring the board file to declare the node, then
setting up all the content by later including this file) is an
interesting approach. I guess it's reasonable. The one issue is that it
makes it a little harder for the board file to override any of the
properties in this file., although it certainly is possible by including
those overrides after the include.
Irrespective of that, some comments on this:
+ palmas_pmic {
+ ti,ldo6-vibrator;
For example, what if the board doesn't want to have the property set?
+
+ regulators {
+ smps123_reg: smps123 {
+ regulator-name = smps123;
+ regulator-min-microvolt = 60;
+ regulator-max-microvolt = 150;
Or what if the board wants to limit the voltage range of this regulator
due to what it's used for on the board.
+ regulator-always-on;
+ regulator-boot-on;
And those two properties are almost certainly board-specific policy.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 11/14] Documentation: dt: binding: omap: am43x timer
On 05/29/2013 02:39 AM, Benoit Cousson wrote: Hi Afzal, On 05/29/2013 10:06 AM, Mohammed, Afzal wrote: Hi Jon, On Wed, May 29, 2013 at 03:35:10, Stephen Warren wrote: On 05/28/2013 03:25 PM, Jon Hunter wrote: ti,am335x-timer (applicable to AM335x devices) ti,am335x-timer-1ms (applicable to AM335x devices) + ti,am4372-timer-1ms, ti,am335x-timer-1ms for AM43x 1ms timer + ti,am4372-timer, ti,am335x-timer for AM43x timers other than 1ms one If you are adding more compatibility strings, then this implies that the AM43x timers are not 100% compatible with any other device listed (such as am335x or any omap device). That's fine but you should state that in the changelog. If the AM43x timer registers are 100% compatible with existing devices you should not add these. I'm not sure that's true; .dts files should always include a compatible value that describes the most specific model of the HW, plus any baseline compatible value that the HW is compatible with. This allows any required quirks/fixes/... to be applied for the specific HW model later even if nobody knows right now they'll be needed. Hence, defining new compatible values doesn't necessarily mean incompatible HW. Stephen took words out of my finger ;) Some explanations,I don;t 1. first compatible should be exact device [A], followed by compatible model (if one) 2. Minor effort in getting DT right the first time may help prevent difficult effort later modifying it (if a necessity comes), considering the fact that DT sources has to move out of Kernel at some point of time. And DT is not supposed to be modified, which may cause difficulty for the users (I had been a minor victim of this during rebase). As we both were in GPMC land earlier, an example, If my memory is right, GPMC IP in am335x is rev 6, and IP has 8 chip select, but one is not pinned out. Now assume that same IP is integrated in another SoC (probably OMAP4 has rev 6). Here if we use same compatible for both, driver cannot handle it properly (w/o knowledge about platform). But if exact compatible is mentioned, without modifying DT (which should be considered as a firmware) just by modifying Kernel, deciding based on compatible would help achieve what is required. That's true for the DTS itself, but here your are changing the binding documentation which is supposed to reflect the driver interface in the Device Tree model description. Since the driver does not support any new compatible string, you should not update the binding. I don't agree here; the DT binding should define all the required and/or allowed values that must/should/can be present in the DT - the entire legal schema. The set of all compatible values is included in that, irrespective of whether a particular value actually (currently) defines a different HW interface or not. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/5] ARM: dts: OMAP2+: use preprocessor for device trees
On 05/23/2013 09:36 AM, Florian Vaussard wrote: Hello, Following a similar proposal by Stephen Warren for tegra [1], this series makes use of the C preprocessor when compiling OMAP DT files, and accomplishes some improvements to improve overall readability. Patch 1 is a preparation for the rest of the series. Patch 2 uses existing constants for GPIOs. Patch 3 does the same for IRQs. Patch 4 creates a new header for OMAP's padmux, and patch 5 uses it to simplify pinctrl DT. For all targets, the .dtb files were diff-tested before and after applying the series to guarantee identity. The series briefly, Reviewed-by: Stephen Warren swar...@nvidia.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 11/14] Documentation: dt: binding: omap: am43x timer
On 05/28/2013 03:25 PM, Jon Hunter wrote: On 27/05/13 15:37, Afzal Mohammed wrote: AM43x timer bindings. Signed-off-by: Afzal Mohammed af...@ti.com --- Documentation/devicetree/bindings/arm/omap/timer.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt index d02e27c..70cb398 100644 --- a/Documentation/devicetree/bindings/arm/omap/timer.txt +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt @@ -14,6 +14,8 @@ Required properties: ti,omap5430-timer (applicable to OMAP543x devices) ti,am335x-timer (applicable to AM335x devices) ti,am335x-timer-1ms (applicable to AM335x devices) +ti,am4372-timer-1ms, ti,am335x-timer-1ms for AM43x 1ms timer +ti,am4372-timer, ti,am335x-timer for AM43x timers other than 1ms one - reg: Contains timer register address range (base address and length). If you are adding more compatibility strings, then this implies that the AM43x timers are not 100% compatible with any other device listed (such as am335x or any omap device). That's fine but you should state that in the changelog. If the AM43x timer registers are 100% compatible with existing devices you should not add these. I'm not sure that's true; .dts files should always include a compatible value that describes the most specific model of the HW, plus any baseline compatible value that the HW is compatible with. This allows any required quirks/fixes/... to be applied for the specific HW model later even if nobody knows right now they'll be needed. Hence, defining new compatible values doesn't necessarily mean incompatible HW. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
