Re: [PATCH v2 2/2] rtc: omap: Add external clock enabling support
On Mon, Aug 17, 2015 at 10:25:38AM +0530, Keerthy wrote:
Configure the clock source to either internal clock
or external clock based on the availability of the clocks.
External clock is preferred as it can be ticking during suspend.
Signed-off-by: Keerthy j-keer...@ti.com
---
Changes in V2:
* Changed clk_prepare calls to clk_prepare_enable.
* Changed clk_unprepare calls to clk_disable_unprepare.
* Added clk pointers for external and internal clock to omap_rtc structure.
drivers/rtc/rtc-omap.c | 40
1 file changed, 40 insertions(+)
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 8b6355f..46b3944 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -25,6 +25,7 @@
#include linux/of_device.h
#include linux/pm_runtime.h
#include linux/io.h
+#include linux/clk.h
/*
* The OMAP RTC is a year/month/day/hours/minutes/seconds BCD clock
@@ -107,6 +108,7 @@
/* OMAP_RTC_OSC_REG bit fields: */
#define OMAP_RTC_OSC_32KCLK_EN BIT(6)
+#define OMAP_RTC_OSC_SEL_32KCLK_SRC BIT(3)
/* OMAP_RTC_IRQWAKEEN bit fields: */
#define OMAP_RTC_IRQWAKEEN_ALARM_WAKEEN BIT(1)
@@ -132,10 +134,13 @@ struct omap_rtc_device_type {
struct omap_rtc {
struct rtc_device *rtc;
void __iomem *base;
+ struct clk *ext_clk;
+ struct clk *int_clk;
As was already suggested, you could just use one clock here for now.
int irq_alarm;
int irq_timer;
u8 interrupts_reg;
bool is_pmic_controller;
+ bool has_ext_clk;
const struct omap_rtc_device_type *type;
};
@@ -553,6 +558,17 @@ static int omap_rtc_probe(struct platform_device *pdev)
if (rtc-irq_alarm = 0)
return -ENOENT;
+ rtc-ext_clk = devm_clk_get(pdev-dev, ext-clk);
+ if (!IS_ERR(rtc-ext_clk)) {
+ rtc-has_ext_clk = true;
+ clk_prepare_enable(rtc-ext_clk);
+ } else {
+ rtc-int_clk = devm_clk_get(pdev-dev, int-clk);
+
+ if (!IS_ERR(rtc-int_clk))
+ clk_prepare_enable(rtc-int_clk);
+ }
+
Which would allow some simplification here.
But shouldn't enabling the internal clock go in its own patch before you
add support for the external clock?
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
rtc-base = devm_ioremap_resource(pdev-dev, res);
if (IS_ERR(rtc-base))
@@ -627,6 +643,16 @@ static int omap_rtc_probe(struct platform_device *pdev)
if (reg != new_ctrl)
rtc_write(rtc, OMAP_RTC_CTRL_REG, new_ctrl);
+ /*
+ * If we have the external clock then switch to it so we can keep
+ * ticking acorss suspend.
You forgot to fix the acorss typo.
+ */
+ if (rtc-has_ext_clk) {
+ reg = rtc_read(rtc, OMAP_RTC_OSC_REG);
+ rtc_write(rtc, OMAP_RTC_OSC_REG, reg |
+ OMAP_RTC_OSC_SEL_32KCLK_SRC);
Odd line break, break after the final comma instead?
+ }
+
rtc-type-lock(rtc);
device_init_wakeup(pdev-dev, true);
@@ -672,6 +698,7 @@ err:
static int __exit omap_rtc_remove(struct platform_device *pdev)
{
struct omap_rtc *rtc = platform_get_drvdata(pdev);
+ u8 reg;
if (pm_power_off == omap_rtc_power_off
omap_rtc_power_off_rtc == rtc) {
@@ -681,10 +708,23 @@ static int __exit omap_rtc_remove(struct
platform_device *pdev)
device_init_wakeup(pdev-dev, 0);
+ if (!IS_ERR(rtc-ext_clk)) {
+ clk_disable_unprepare(rtc-ext_clk);
+ } else {
+ if (!IS_ERR(rtc-int_clk))
+ clk_disable_unprepare(rtc-int_clk);
+ }
+
This could also be simplified with a single clock entry in rtc.
rtc-type-unlock(rtc);
/* leave rtc running, but disable irqs */
rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, 0);
+ if (rtc-has_ext_clk) {
+ reg = rtc_read(rtc, OMAP_RTC_OSC_REG);
+ reg = ~OMAP_RTC_OSC_SEL_32KCLK_SRC;
+ rtc_write(rtc, OMAP_RTC_OSC_REG, reg);
+ }
+
rtc-type-lock(rtc);
/* Disable the clock/module */
Johan
--
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/2] rtc: omap: Add external clock enabling support
On Mon, Aug 17, 2015 at 05:08:55PM +0530, Keerthy wrote:
On Monday 17 August 2015 05:00 PM, Johan Hovold wrote:
On Mon, Aug 17, 2015 at 10:25:38AM +0530, Keerthy wrote:
Configure the clock source to either internal clock
or external clock based on the availability of the clocks.
External clock is preferred as it can be ticking during suspend.
Signed-off-by: Keerthy j-keer...@ti.com
---
Changes in V2:
* Changed clk_prepare calls to clk_prepare_enable.
* Changed clk_unprepare calls to clk_disable_unprepare.
* Added clk pointers for external and internal clock to omap_rtc
structure.
drivers/rtc/rtc-omap.c | 40
1 file changed, 40 insertions(+)
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 8b6355f..46b3944 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -25,6 +25,7 @@
#include linux/of_device.h
#include linux/pm_runtime.h
#include linux/io.h
+#include linux/clk.h
/*
* The OMAP RTC is a year/month/day/hours/minutes/seconds BCD clock
@@ -107,6 +108,7 @@
/* OMAP_RTC_OSC_REG bit fields: */
#define OMAP_RTC_OSC_32KCLK_EN BIT(6)
+#define OMAP_RTC_OSC_SEL_32KCLK_SRC BIT(3)
/* OMAP_RTC_IRQWAKEEN bit fields: */
#define OMAP_RTC_IRQWAKEEN_ALARM_WAKEEN BIT(1)
@@ -132,10 +134,13 @@ struct omap_rtc_device_type {
struct omap_rtc {
struct rtc_device *rtc;
void __iomem *base;
+ struct clk *ext_clk;
+ struct clk *int_clk;
As was already suggested, you could just use one clock here for now.
If the intent is to enable dynamic switching in future why not have both
the clock pointers ready?
Because it needlessly complicates the clock handling below, and those
bits would need to be rewritten anyway when/if dynamic switching is
implemented.
int irq_alarm;
int irq_timer;
u8 interrupts_reg;
bool is_pmic_controller;
+ bool has_ext_clk;
const struct omap_rtc_device_type *type;
};
@@ -553,6 +558,17 @@ static int omap_rtc_probe(struct platform_device
*pdev)
if (rtc-irq_alarm = 0)
return -ENOENT;
+ rtc-ext_clk = devm_clk_get(pdev-dev, ext-clk);
+ if (!IS_ERR(rtc-ext_clk)) {
+ rtc-has_ext_clk = true;
+ clk_prepare_enable(rtc-ext_clk);
+ } else {
+ rtc-int_clk = devm_clk_get(pdev-dev, int-clk);
+
+ if (!IS_ERR(rtc-int_clk))
+ clk_prepare_enable(rtc-int_clk);
+ }
+
Which would allow some simplification here.
But shouldn't enabling the internal clock go in its own patch before you
add support for the external clock?
I am okay with splitting but for now its either or situation so enabling
in one patch made sense to me.
Since you're doing two different things here (fixing the missing clock
enable and adding support for the external clock), I'd suggest doing it
in two patches.
Johan
--
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] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On Fri, Aug 14, 2015 at 2:40 PM, Lars-Peter Clausen l...@metafoo.de wrote: On 08/14/2015 02:34 PM, Linus Walleij wrote: [...] Every chip will get their own lock class on the heap. But I think it is a bit kludgy. Is it not possible to have the lock key in struct gpio_chip be a real member instead of a pointer and get a per-chip lock that way? (...) struct lock_class_key lock_key; instead of: struct lock_class_key *lock_key; - problem solved, without kludgy header defines? Lock keys need to be in persistent memory since they have a unlimited life time. Once registered it is expected to exist until the system is reset. Aha I see. OK if we fix the documentation comment I guess we can go with this, even if it makes the header file somewhat hard to read. I have a bit of problem with it, because lockdep instrumentation is supposed to make development easier, not harder, now it helps in one end with lock validation and screws up in another end by creating API headers that are hopeless to read :( Linus -- 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/2] rtc: omap: Add external clock enabling support
On Monday 17 August 2015 05:00 PM, Johan Hovold wrote:
On Mon, Aug 17, 2015 at 10:25:38AM +0530, Keerthy wrote:
Configure the clock source to either internal clock
or external clock based on the availability of the clocks.
External clock is preferred as it can be ticking during suspend.
Signed-off-by: Keerthy j-keer...@ti.com
---
Changes in V2:
* Changed clk_prepare calls to clk_prepare_enable.
* Changed clk_unprepare calls to clk_disable_unprepare.
* Added clk pointers for external and internal clock to omap_rtc structure.
drivers/rtc/rtc-omap.c | 40
1 file changed, 40 insertions(+)
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 8b6355f..46b3944 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -25,6 +25,7 @@
#include linux/of_device.h
#include linux/pm_runtime.h
#include linux/io.h
+#include linux/clk.h
/*
* The OMAP RTC is a year/month/day/hours/minutes/seconds BCD clock
@@ -107,6 +108,7 @@
/* OMAP_RTC_OSC_REG bit fields: */
#define OMAP_RTC_OSC_32KCLK_ENBIT(6)
+#define OMAP_RTC_OSC_SEL_32KCLK_SRCBIT(3)
/* OMAP_RTC_IRQWAKEEN bit fields: */
#define OMAP_RTC_IRQWAKEEN_ALARM_WAKEEN BIT(1)
@@ -132,10 +134,13 @@ struct omap_rtc_device_type {
struct omap_rtc {
struct rtc_device *rtc;
void __iomem *base;
+ struct clk *ext_clk;
+ struct clk *int_clk;
As was already suggested, you could just use one clock here for now.
If the intent is to enable dynamic switching in future why not have both
the clock pointers ready?
int irq_alarm;
int irq_timer;
u8 interrupts_reg;
bool is_pmic_controller;
+ bool has_ext_clk;
const struct omap_rtc_device_type *type;
};
@@ -553,6 +558,17 @@ static int omap_rtc_probe(struct platform_device *pdev)
if (rtc-irq_alarm = 0)
return -ENOENT;
+ rtc-ext_clk = devm_clk_get(pdev-dev, ext-clk);
+ if (!IS_ERR(rtc-ext_clk)) {
+ rtc-has_ext_clk = true;
+ clk_prepare_enable(rtc-ext_clk);
+ } else {
+ rtc-int_clk = devm_clk_get(pdev-dev, int-clk);
+
+ if (!IS_ERR(rtc-int_clk))
+ clk_prepare_enable(rtc-int_clk);
+ }
+
Which would allow some simplification here.
But shouldn't enabling the internal clock go in its own patch before you
add support for the external clock?
I am okay with splitting but for now its either or situation so enabling
in one patch made sense to me.
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
rtc-base = devm_ioremap_resource(pdev-dev, res);
if (IS_ERR(rtc-base))
@@ -627,6 +643,16 @@ static int omap_rtc_probe(struct platform_device *pdev)
if (reg != new_ctrl)
rtc_write(rtc, OMAP_RTC_CTRL_REG, new_ctrl);
+ /*
+* If we have the external clock then switch to it so we can keep
+* ticking acorss suspend.
You forgot to fix the acorss typo.
I will fix it.
+*/
+ if (rtc-has_ext_clk) {
+ reg = rtc_read(rtc, OMAP_RTC_OSC_REG);
+ rtc_write(rtc, OMAP_RTC_OSC_REG, reg |
+ OMAP_RTC_OSC_SEL_32KCLK_SRC);
Odd line break, break after the final comma instead?
Okay. I will fix it.
+ }
+
rtc-type-lock(rtc);
device_init_wakeup(pdev-dev, true);
@@ -672,6 +698,7 @@ err:
static int __exit omap_rtc_remove(struct platform_device *pdev)
{
struct omap_rtc *rtc = platform_get_drvdata(pdev);
+ u8 reg;
if (pm_power_off == omap_rtc_power_off
omap_rtc_power_off_rtc == rtc) {
@@ -681,10 +708,23 @@ static int __exit omap_rtc_remove(struct platform_device
*pdev)
device_init_wakeup(pdev-dev, 0);
+ if (!IS_ERR(rtc-ext_clk)) {
+ clk_disable_unprepare(rtc-ext_clk);
+ } else {
+ if (!IS_ERR(rtc-int_clk))
+ clk_disable_unprepare(rtc-int_clk);
+ }
+
This could also be simplified with a single clock entry in rtc.
rtc-type-unlock(rtc);
/* leave rtc running, but disable irqs */
rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, 0);
+ if (rtc-has_ext_clk) {
+ reg = rtc_read(rtc, OMAP_RTC_OSC_REG);
+ reg = ~OMAP_RTC_OSC_SEL_32KCLK_SRC;
+ rtc_write(rtc, OMAP_RTC_OSC_REG, reg);
+ }
+
rtc-type-lock(rtc);
/* Disable the clock/module */
Johan
--
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] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On 08/14/2015 03:34 PM, Linus Walleij wrote:
On Thu, Aug 13, 2015 at 4:58 PM, Grygorii Strashko
grygorii.stras...@ti.com wrote:
Since IRQ chip helpers were introduced drivers lose ability to
register separate lockdep classes for each registered GPIO IRQ
chip and the gpiolib now is using shared lockdep class for
all GPIO IRQ chips (gpiochip_irq_lock_class).
As result, lockdep will produce warning when there are min two
stacked GPIO chips and all of them are interrupt controllers.
HW configuration which generates lockdep warning (TI dra7-evm):
(...)
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: Roger Quadros rog...@ti.com
Reported-by: Roger Quadros rog...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
Ah, I see...
* implies that if the chip supports IRQs, these IRQs need to be
threaded
* as the chip access may sleep when e.g. reading out the IRQ status
* registers.
+ * @exported: flags if the gpiochip is exported for use from sysfs. Private.
* @irq_not_threaded: flag must be set if @can_sleep is set but the
* IRQs don't need to be threaded
*
@@ -126,6 +128,7 @@ struct gpio_chip {
irq_flow_handler_t irq_handler;
unsigned intirq_default_type;
int irq_parent;
+ struct lock_class_key *lock_key;
There is something weird with the kerneldoc. It is documenting something
else but not documenting the new member.
Sorry, for that.
There are no kerneldocs for any of GPIO IRQ chip's specific fields,
Do you prefer me to add doc for lock_key or all of them?
--
regards,
-grygorii
--
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 v2 1/2] gpiolib: irqchip: use different lockdep class for each gpio irqchip
Since IRQ chip helpers were introduced drivers lose ability to
register separate lockdep classes for each registered GPIO IRQ
chip and the gpiolib now is using shared lockdep class for
all GPIO IRQ chips (gpiochip_irq_lock_class).
As result, lockdep will produce warning when there are min two
stacked GPIO chips and all of them are interrupt controllers.
HW configuration which generates lockdep warning (TI dra7-evm):
[SOC GPIO bankA.gpioX]
- irq - [pcf875x.gpioY]
- irq - DevZ.enable_irq_wake(pcf_gpioY_irq);
The issue was reported in [1] and discussed [2].
=
[ INFO: possible recursive locking detected ]
4.2.0-rc6-00013-g5d050ed-dirty #55 Not tainted
-
sh/63 is trying to acquire lock:
(class){..}, at: [c009b91c] __irq_get_desc_lock+0x50/0x94
but task is already holding lock:
(class){..}, at: [c009b91c] __irq_get_desc_lock+0x50/0x94
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
lock(class);
lock(class);
*** DEADLOCK ***
May be due to missing lock nesting notation
7 locks held by sh/63:
#0: (sb_writers#4){.+.+.+}, at: [c016bbb8] vfs_write+0x13c/0x164
#1: (of-mutex){+.+.+.}, at: [c01debf4] kernfs_fop_write+0x4c/0x1a0
#2: (s_active#36){.+.+.+}, at: [c01debfc] kernfs_fop_write+0x54/0x1a0
#3: (pm_mutex){+.+.+.}, at: [c009758c] pm_suspend+0xec/0x4c4
#4: (dev-mutex){..}, at: [c03f77f8] __device_suspend+0xd4/0x398
#5: (gpio-lock){+.+.+.}, at: [c009b940] __irq_get_desc_lock+0x74/0x94
#6: (class){..}, at: [c009b91c] __irq_get_desc_lock+0x50/0x94
stack backtrace:
CPU: 0 PID: 63 Comm: sh Not tainted 4.2.0-rc6-00013-g5d050ed-dirty #55
Hardware name: Generic DRA74X (Flattened Device Tree)
[c0016e24] (unwind_backtrace) from [c0013338] (show_stack+0x10/0x14)
[c0013338] (show_stack) from [c05f6b24] (dump_stack+0x84/0x9c)
[c05f6b24] (dump_stack) from [c00903f4] (__lock_acquire+0x19c0/0x1e20)
[c00903f4] (__lock_acquire) from [c0091098] (lock_acquire+0xa8/0x128)
[c0091098] (lock_acquire) from [c05fd61c] (_raw_spin_lock_irqsave+0x38/0x4c)
[c05fd61c] (_raw_spin_lock_irqsave) from [c009b91c]
(__irq_get_desc_lock+0x50/0x94)
[c009b91c] (__irq_get_desc_lock) from [c009c4f4]
(irq_set_irq_wake+0x20/0xfc)
[c009c4f4] (irq_set_irq_wake) from [c0393ac4]
(pcf857x_irq_set_wake+0x24/0x54)
[c0393ac4] (pcf857x_irq_set_wake) from [c009c560]
(irq_set_irq_wake+0x8c/0xfc)
[c009c560] (irq_set_irq_wake) from [c04a02ac] (gpio_keys_suspend+0x70/0xd4)
[c04a02ac] (gpio_keys_suspend) from [c03f6a00] (dpm_run_callback+0x50/0x124)
[c03f6a00] (dpm_run_callback) from [c03f7830] (__device_suspend+0x10c/0x398)
[c03f7830] (__device_suspend) from [c03f90f0] (dpm_suspend+0x134/0x2f4)
[c03f90f0] (dpm_suspend) from [c0096e20]
(suspend_devices_and_enter+0xa8/0x728)
[c0096e20] (suspend_devices_and_enter) from [c00977cc]
(pm_suspend+0x32c/0x4c4)
[c00977cc] (pm_suspend) from [c0096060] (state_store+0x64/0xb8)
[c0096060] (state_store) from [c01dec64] (kernfs_fop_write+0xbc/0x1a0)
[c01dec64] (kernfs_fop_write) from [c016b280] (__vfs_write+0x20/0xd8)
[c016b280] (__vfs_write) from [c016bb0c] (vfs_write+0x90/0x164)
[c016bb0c] (vfs_write) from [c016c330] (SyS_write+0x44/0x9c)
[c016c330] (SyS_write) from [c000f500] (ret_fast_syscall+0x0/0x54)
Lets fix it by using separate lockdep class for each registered GPIO
IRQ Chip. This is done by wrapping gpiochip_irqchip_add call into macros.
The implementation of this patch inspired by solution done by Nicolas
Boichat for regmap [3]
[1] http://www.spinics.net/lists/linux-gpio/msg05844.html
[2] http://www.spinics.net/lists/linux-gpio/msg06021.html
[3] http://www.spinics.net/lists/arm-kernel/msg429834.html
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: Roger Quadros rog...@ti.com
Reported-by: Roger Quadros rog...@ti.com
Tested-by: Roger Quadros rog...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
Changes in v2:
- removed accidental change in gpio chip structure description.
drivers/gpio/gpiolib.c | 27 ++-
include/linux/gpio/driver.h | 26 +-
2 files changed, 35 insertions(+), 18 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 34f95fb..b562dd3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -462,12 +462,6 @@ void gpiochip_set_chained_irqchip(struct gpio_chip
*gpiochip,
}
EXPORT_SYMBOL_GPL(gpiochip_set_chained_irqchip);
-/*
- * This lock class tells lockdep that GPIO irqs are in a different
- * category than their parents, so it won't report false recursion.
- */
-static struct lock_class_key gpiochip_irq_lock_class;
-
/**
* gpiochip_irq_map() - maps an IRQ into a GPIO irqchip
* @d: the irqdomain used by this irqchip
@@ -484,7 +478,11 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned
int irq,
struct gpio_chip *chip = d-host_data;
irq_set_chip_data(irq, chip);
[PATCH v2 2/2] gpiolib: add description for gpio irqchip fields in struct gpio_chip
Add missed description for GPIO irqchip fields in struct gpio_chip. Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com --- Changes in v2: - New patch. include/linux/gpio/driver.h | 11 +++ 1 file changed, 11 insertions(+) diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 0c7004d..1aed31c 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -65,6 +65,17 @@ struct seq_file; * registers. * @irq_not_threaded: flag must be set if @can_sleep is set but the * IRQs don't need to be threaded + * @irqchip: GPIO IRQ chip impl, provided by GPIO driver + * @irqdomain: Interrupt translation domain; responsible for mapping + * between GPIO hwirq number and linux irq number + * @irq_base: first linux IRQ number assigned to GPIO IRQ chip (deprecated) + * @irq_handler: the irq handler to use (often a predefined irq core function) + * for GPIO IRQs, provided by GPIO driver + * @irq_default_type: default IRQ triggering type applied during GPIO driver + * initialization, provided by GPIO driver + * @irq_parent: GPIO IRQ chip parent/bank linux irq number, + * provided by GPIO driver + * @lock_key: per GPIO IRQ chip lockdep class * * A gpio_chip can help platforms abstract various sources of GPIOs so * they can all be accessed through a common programing interface. -- 2.5.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 RFC v3 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders
On 08/17/15 10:57, Jyri Sarha wrote:
Missed one commet first time around...
On 08/14/15 19:18, Mark Brown wrote:
On Fri, Aug 14, 2015 at 12:30:41PM +0300, Jyri Sarha wrote:
...
+/* HDMI codec initalization data */
+struct hdmi_codec_pdata {
+struct device *dev; /* The HDMI encoder registering the codec */
Shouldn't this just be dev-parent?
No. The HDMI encoder device is the parent for HDMI-codec.
The patch you took in in the last round uses the ASoC component drivers
parent's of-node if the component driver does not have one itself. In
this case the phandle in the binding points to the HDMI encoder's node,
which is the parent of the HDMI codec.
After reading the comment again I see now what you meant.
Yes, I can extract the encoder device from the dev-parent pointer. No
need to have it in pdata.
Thanks,
Jyri
--
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] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On Mon, Aug 17, 2015 at 2:35 PM, Grygorii Strashko grygorii.stras...@ti.com wrote: Since IRQ chip helpers were introduced drivers lose ability to register separate lockdep classes for each registered GPIO IRQ chip and the gpiolib now is using shared lockdep class for all GPIO IRQ chips (gpiochip_irq_lock_class). As result, lockdep will produce warning when there are min two stacked GPIO chips and all of them are interrupt controllers. HW configuration which generates lockdep warning (TI dra7-evm): (...) Cc: Geert Uytterhoeven ge...@linux-m68k.org Cc: Roger Quadros rog...@ti.com Reported-by: Roger Quadros rog...@ti.com Tested-by: Roger Quadros rog...@ti.com Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com --- Changes in v2: - removed accidental change in gpio chip structure description. This v2 patch applied. I see no better alternative. Yours, Linus Walleij -- 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] regulator: pbias: use untranslated address to program pbias regulator
Hi Mark Brown, On Friday 14 August 2015 11:30 PM, Mark Brown wrote: On Mon, Jul 27, 2015 at 04:54:09PM +0530, Kishon Vijay Abraham I wrote: vsel_reg and enable_reg of the pbias regulator descriptor should actually have the offset from syscon. However after the pbias device tree node I'm having a hard time understanding this statement, sorry. What makes you say that they shouild actually have the offset from syscon? What is the problem that this is supposed to fix? The register to program pbias regulator is 0x4A002E00. The syscon base address is 0x4a002000. So the vsel_reg and enable_reg should have the offset from syscon base address. regulator_enable_regmap gets the base address from 'regmap' and offset from 'enable_reg' in order to program the pbias regulator. But without this patch vsel_reg and enable_reg have the absolute address instead of just the offset. is moved as a child node of syscon, vsel_reg and enable_reg has the absolute address because of the address translation that happens while creating device from device tree node. So avoid using platform_get_resource and use of_get_address in order to get only the offset (untranslated address) and populate these in vsel_reg and enable_reg. This sounds like we're going in the wrong direction, we're moving from a more generic API to a firmware specific one. Why is this a good fix? platform_get_resource can be used if we need the absolute address but here we need only the offset. Thanks Kishon -- 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 v3 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders
On Mon, Aug 17, 2015 at 10:07:55AM +0300, Jyri Sarha wrote: On 08/14/15 19:18, Mark Brown wrote: On Fri, Aug 14, 2015 at 12:30:41PM +0300, Jyri Sarha wrote: + /* Called when ASoC starts an audio stream setup. The call +* provides an audio abort callback for stoping an ongoing +* stream if the HDMI audio becomes unavailable. +* Optional */ + int (*audio_startup)(struct device *dev, +void (*abort_cb)(struct device *dev)); I'm a bit confused about what is going to use abort_cb() and why they wouldn't just call shutdown instead? audio_shutdown() is for ASoC side to tell video side that audio playback has stopped. The abort_cb() is for video side to inform ASoC that current audio stream can not continue anymore and it should be aborted. The similar mechanism is currently in use in sound/soc/omap/omap-hdmi-audio.c. Someone reading the code needs to be able to understand this. signature.asc Description: Digital signature
Re: [PATCH RFC v3 2/7] ASoC: hdmi: Remove obsolete dummy HDMI codec
On Mon, Aug 17, 2015 at 10:00:07AM +0300, Jyri Sarha wrote: On 08/14/15 19:10, Mark Brown wrote: Don't you mean omap-hdmi-audio, that is implemented in sound/soc/omap/omap-hdmi-audio.c ? That driver is bit different. It implements ASoC card and uses generic dummy codec. The hdmi-audio-codec has not been used for OMAP HDMI audio for several releases already. Possibly, yes, or I was on an old tree by mistake. It's definitely not used now like you say. signature.asc Description: Digital signature
Re: [PATCH RFC v3 6/7] drm/i2c: tda998x: Register ASoC HDMI codec for audio functionality
On 08/14/15 13:06, Russell King - ARM Linux wrote:
On Fri, Aug 14, 2015 at 12:30:44PM +0300, Jyri Sarha wrote:
+static int tda998x_write_aif(struct tda998x_priv *priv,
+struct hdmi_audio_infoframe *cea)
+{
+ uint8_t buf[HDMI_INFOFRAME_SIZE(AUDIO)];
+ int len;
+
+ len = hdmi_audio_infoframe_pack(cea, buf, sizeof(buf));
+ if (len 0) {
+ dev_err(priv-hdmi-dev,
+ Failed to pack audio infoframe: %d\n, len);
+ return len;
+ }
+
+ /* Write the audio information packet */
+ tda998x_write_if(priv, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf, len);
+ return 0;
+}
+
I have such a function already queued up, but I can't push it out at the
moment because of too many conflicts across all my DRM work. I'm waiting
for after 4.3-rc1 before publishing anything from or accepting anything
else into DRM branches.
Ok, is the code available some where? Could take a look so I can align
my code to that.
static void
tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
{
@@ -670,19 +691,24 @@ static void tda998x_audio_mute(struct tda998x_priv *priv,
bool on)
}
}
-static void
+static int
tda998x_configure_audio(struct tda998x_priv *priv,
- struct drm_display_mode *mode, struct tda998x_encoder_params *p)
+ int mode_clock,
+ int ena_ap,
+ int dai_format,
+ int sample_width,
+ int sample_rate,
+ const u8 *status)
I don't think this is an improvement.
Still it makes the function more generic and enables its usage in
HDMI-codec API implementation. I'll try to make it look tidier.
+static int tda998x_audio_get_eld(struct device *dev, uint8_t *buf, size_t len)
+{
+ struct tda998x_priv *priv = dev_get_drvdata(dev);
+ struct drm_mode_config *config = priv-encoder-dev-mode_config;
+ struct drm_connector *connector;
+ int ret = -ENODEV;
+
+ mutex_lock(config.mutex);
+ list_for_each_entry(connector, config-connector_list, head) {
+ if (priv-encoder == connector-encoder) {
+ memcpy(buf, connector-eld,
+ min(sizeof(connector-eld), len));
+ ret = 0;
+ }
+ }
+ mutex_unlock(config.mutex);
Obviously untested. Should be config-mutex.
Sorry. Should never do these last minute changes. I must have been
compiling and testing different code version. I first had this function
using config-connection_mutex, but then - after reading the eld related
code - found that mode_config mutex should be used instead.
But in any case, when I kill the DRM slave encoder code in here, this
becomes unnecessary.
Ok, The information from ELD needs be passed to audio side constraints
somehow. I'd love to see the code you have in queue.
Best regards,
Jyri
--
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 v3 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders
Missed one commet first time around...
On 08/14/15 19:18, Mark Brown wrote:
On Fri, Aug 14, 2015 at 12:30:41PM +0300, Jyri Sarha wrote:
...
+/* HDMI codec initalization data */
+struct hdmi_codec_pdata {
+ struct device *dev; /* The HDMI encoder registering the codec */
Shouldn't this just be dev-parent?
No. The HDMI encoder device is the parent for HDMI-codec.
The patch you took in in the last round uses the ASoC component drivers
parent's of-node if the component driver does not have one itself. In
this case the phandle in the binding points to the HDMI encoder's node,
which is the parent of the HDMI codec.
Best regards,
Jyri
--
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 v3 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders
On 08/17/15 21:41, Mark Brown wrote: On Mon, Aug 17, 2015 at 10:07:55AM +0300, Jyri Sarha wrote: On 08/14/15 19:18, Mark Brown wrote: On Fri, Aug 14, 2015 at 12:30:41PM +0300, Jyri Sarha wrote: + /* Called when ASoC starts an audio stream setup. The call +* provides an audio abort callback for stoping an ongoing +* stream if the HDMI audio becomes unavailable. +* Optional */ + int (*audio_startup)(struct device *dev, +void (*abort_cb)(struct device *dev)); I'm a bit confused about what is going to use abort_cb() and why they wouldn't just call shutdown instead? audio_shutdown() is for ASoC side to tell video side that audio playback has stopped. The abort_cb() is for video side to inform ASoC that current audio stream can not continue anymore and it should be aborted. The similar mechanism is currently in use in sound/soc/omap/omap-hdmi-audio.c. Someone reading the code needs to be able to understand this. Ok, I'll improve the comment above. Thanks, Jyri -- 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: [RFT PATCH] arm: omap1_defconfig: convert to use libata PATA drivers
* Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com [150814 10:10]: IDE subsystem has been deprecated since 2009 and the majority (if not all) of Linux distributions have switched to use libata for ATA support exclusively. However there are still some users (mostly old or/and embedded non-x86 systems) that have not converted from using IDE subsystem to libata PATA drivers. This doesn't seem to be good thing in the long-term for Linux as while there is less and less PATA systems left in use: * testing efforts are divided between two subsystems * having duplicate drivers for same hardware confuses users This patch converts omap1_defconfig to use libata PATA drivers. Cc: Tony Lindgren t...@atomide.com Cc: linux-omap@vger.kernel.org Signed-off-by: Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com --- Build tested only. If you have affected hardware please test. Thank you. All my omap1 devices are in a rack using NFSroot or MMC so I can't test this. Seems like a good idea to me thoug, and does not cause merge conflicts. So if you want to queue this via along with other PATA related patches, please feel free to add: Acked-by: Tony Lindgren t...@atomide.com arch/arm/configs/omap1_defconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/arm/configs/omap1_defconfig b/arch/arm/configs/omap1_defconfig index 0c8a787..6ffc984 100644 --- a/arch/arm/configs/omap1_defconfig +++ b/arch/arm/configs/omap1_defconfig @@ -96,14 +96,14 @@ CONFIG_BLK_DEV_LOOP=y CONFIG_BLK_DEV_RAM=y CONFIG_BLK_DEV_RAM_COUNT=2 CONFIG_BLK_DEV_RAM_SIZE=8192 -CONFIG_IDE=m -CONFIG_BLK_DEV_IDECS=m CONFIG_SCSI=y # CONFIG_SCSI_PROC_FS is not set CONFIG_BLK_DEV_SD=y CONFIG_CHR_DEV_ST=y CONFIG_BLK_DEV_SR=y CONFIG_CHR_DEV_SG=y +CONFIG_ATA=m +CONFIG_PATA_PCMCIA=m CONFIG_NETDEVICES=y CONFIG_TUN=y CONFIG_PHYLIB=y -- 1.9.1 -- 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 v3 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders
On 08/14/15 12:57, Russell King - ARM Linux wrote:
On Fri, Aug 14, 2015 at 12:30:41PM +0300, Jyri Sarha wrote:
+static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+ struct hdmi_codec_params hp = {
+ .cea = { 0 },
Unnecessary initialisation - because you are initialising this structure,
all unnamed fields will be zeroed.
True, I just tried to be explicit.
+ .iec = {
+ .status = {
+ IEC958_AES0_CON_NOT_COPYRIGHT,
+ IEC958_AES1_CON_GENERAL,
+ IEC958_AES2_CON_SOURCE_UNSPEC,
+ IEC958_AES3_CON_CLOCK_VARIABLE,
+ },
...
+ hdmi_audio_infoframe_init(hp.cea);
+ hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_PCM;
Something tells me here that you haven't read the HDMI specification.
HDMI says that the coding type will be zero (refer to stream header).
The same goes for much of the CEA audio infoframe. Please see the
Audio InfoFrame details in the HDMI specification.
Must admit, that I have not read it end to end. Obviously I have missed
a relevant piece of information here. I'll fix that and check the
related items too.
+ hp.cea.channels = params_channels(params);
+
+ switch (params_width(params)) {
+ case 16:
+ hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_20_16;
+ hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_16;
+ break;
+ case 18:
+ hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_22_18;
+ hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_20;
+ break;
+ case 20:
+ hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_24_20;
+ hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_20;
+ break;
+ case 24:
+ case 32:
+ hp.iec.status[4] |= IEC958_AES4_CON_MAX_WORDLEN_24 |
+ IEC958_AES4_CON_WORDLEN_24_20;
Why not use the generic code to generate the AES channel status bits?
See sound/core/pcm_iec958.c.
Thanks, I did not know that exist. I'll make use of that.
Best regards,
Jyri
--
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 v3 2/7] ASoC: hdmi: Remove obsolete dummy HDMI codec
On 08/14/15 19:10, Mark Brown wrote: On Fri, Aug 14, 2015 at 12:30:40PM +0300, Jyri Sarha wrote: The hdmi stub codec has not been used since refactoring of OMAP HDMI audio support. grep tells me that the OMAP HDMI4 and HDMI5 drivers are still registering this device in -next... Really? My source trees for mainline and linux-next look like this: $ git grep hdmi-audio-codec sound/soc/codecs/hdmi.c:#define DRV_NAME hdmi-audio-codec Don't you mean omap-hdmi-audio, that is implemented in sound/soc/omap/omap-hdmi-audio.c ? That driver is bit different. It implements ASoC card and uses generic dummy codec. The hdmi-audio-codec has not been used for OMAP HDMI audio for several releases already. Best regards, Jyri -- 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 v3 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders
On 08/14/15 19:18, Mark Brown wrote:
On Fri, Aug 14, 2015 at 12:30:41PM +0300, Jyri Sarha wrote:
+struct hdmi_codec_ops {
+ /* For runtime clock configuration from ASoC machine driver.
+* A direct forward from set_sysclk in struct snd_soc_dai_ops.
+* Optional */
+ int (*set_clk)(struct device *dev, int clk_id, int freq);
I'd be much happier if we were using the clock API as the external
interface here, it's where we want to be internally too and it's going
to be easier to not introduce any external dependencies on the ASoC
internal stuff.
Sounds better. I'll change that.
+ /* Called when ASoC starts an audio stream setup. The call
+* provides an audio abort callback for stoping an ongoing
+* stream if the HDMI audio becomes unavailable.
+* Optional */
+ int (*audio_startup)(struct device *dev,
+void (*abort_cb)(struct device *dev));
I'm a bit confused about what is going to use abort_cb() and why they
wouldn't just call shutdown instead?
audio_shutdown() is for ASoC side to tell video side that audio playback
has stopped.
The abort_cb() is for video side to inform ASoC that current audio
stream can not continue anymore and it should be aborted. The similar
mechanism is currently in use in sound/soc/omap/omap-hdmi-audio.c.
+/* HDMI codec initalization data */
+struct hdmi_codec_pdata {
+ struct device *dev; /* The HDMI encoder registering the codec */
Shouldn't this just be dev-parent?
+enum {
+ DAI_ID_I2C = 0,
+ DAI_ID_SPDIF,
+};
I2C? :P
Right, should be I2S. Thanks!
Best regards,
Jyri
--
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 v2 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders
On 08/14/15 22:25, Mark Brown wrote:
On Tue, May 26, 2015 at 09:59:07PM +0300, Jyri Sarha wrote:
+
+ mutex_lock(hcp-current_stream_lock);
+ if (hcp-current_stream hcp-current_stream-runtime
+ snd_pcm_running(hcp-current_stream)) {
+ dev_info(dev, HDMI audio playback aborted\n);
Does this really need to be dev_info()?
No, I'll change that to debug level.
+ if (hcp-hcd.ops-get_eld) {
+ hcp-eld = hcp-hcd.ops-get_eld(hcp-hcd.dev);
+
+ /* Call snd_pcm_hw_constraint_eld here */
+ }
...
+ dev_dbg(dai-dev, %s()\n, __func__);
+
+ mutex_lock(hcp-current_stream_lock);
+ BUG_ON(hcp-current_stream != substream);
+ hcp-current_stream = NULL;
+ mutex_unlock(hcp-current_stream_lock);
+
+ hcp-hcd.ops-audio_shutdown(hcp-hcd.dev);
Shouldn't the callback be in or before the lock? Otherwise we could
potentially race with starting a new stream.
Yes, it should be before it. I'll fix that.
If it is inside, there is a possible deadlock warning (atleast in the
similar place in omap-hdmi-audio there was) if those warnings are turned
on and there is another lock that is taken in the video side callbacks.
Thanks,
Jyri
--
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
