Re: [PATCH V4 00/30] thermal: exynos: Add thermal driver for exynos5440
On 14-05-2013 05:58, Amit Daniel Kachhap wrote: Changes in V4: Almost all the changes in this version is as per suggestion from Eduardo.The major ones are listed below, * Added kconfig symbol ARCH_HAS_TMU which needs to be enabled by platform. With this change existing symbol EXYNOS_TMU_DATA is not needed. I was more thinking if we could have a single flag that we could use in thermal drivers. Besides, my proposal for ARCH_HAS_BANDGAP is still not merged yet. * Movement of freq_clip_table from exynos_tmu.h to exynos_thermal_common.h is explained in the commit logs. * Wrote all register description documentation. * Split 5440 TMU support patch into controller change, configuration data and feature addition patches. * Remove all *LINUX_* in the header files. * Still regulator enable is kept optional but a TODO: comment is added to fix it later. Changes in V3: * Added proper dependency of different exynos thermal Kconfig symbols. Basically 3 Kconfig can be enabled now and corresponds to tmu driver. exynos common part and exynos configuration data. This issue was raised by Rui Zhang. Changes in V2: * Separated SOC data from TMU driver. This is as per suggestion from Eduardo. * Merged the new file created for exynos5440 TMU controller with the existing TMU controller code. * Removed the DT parsing code as now the SOC specific data are cleanly put inside the data specific file. * Even the register definations/bitfields are treated as data as there is some variation across SOC's. This patchset adds TMU(Thermal management Unit) driver support for exynos5440 platform. There are 3 instances of the TMU controllers so necessary cleanup/re-structure is done to handle multiple thermal zone. Patch (exynos4: Add documentation for Exynos SoC thermal bindings) from Lukasz Majewski is already posted to mainline. Adding it here for completeness. (http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg17817.html) Patch (thermal: exynos: Support thermal tripping ) from Jonghwan Choi is added here with some changes. (https://patchwork.kernel.org/patch/1668371/) Patch (thermal: exynos: Support for TMU regulator defined at device tree) is a repost of my earlier patch(https://patchwork-mail1.kernel.org/patch/2510771/) and adds regulator support. Patch (ARM: dts: Add device tree node for exynos5440 TMU controller) and patch (arm: exynos: enable ARCH_HAS_TMU) can be merged through exynos platform maintainer as this can cause merge conflict. All these patches are based on thermal maintainers git tree, git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git next. Amit Daniel Kachhap (29): thermal: exynos: Moving exynos thermal files into samsung directory thermal: exynos: Add ARCH_HAS_TMU config to know the supported soc's thermal: exynos: Remove CPU_THERMAL dependency for using TMU driver thermal: exynos: Bifurcate exynos thermal common and tmu controller code thermal: exynos: Rename exynos_thermal.c to exynos_tmu.c thermal: exynos: Move exynos_thermal.h from include/* to driver/* folder thermal: exynos: Bifurcate exynos tmu driver and configuration data thermal: exynos: Add missing definations and code cleanup thermal: exynos: Add extra entries in the tmu platform data thermal: exynos: Support thermal tripping thermal: exynos: Move register definitions from driver file to data file thermal: exynos: Fix to clear only the generated interrupts thermal: exynos: Add support for instance based register/unregister thermal: exynos: Modify private_data to appropriate name driver_data thermal: exynos: Return success even if no cooling data supplied thermal: exynos: Make the zone handling dependent on trip count thermal: exynos: Add support to handle many instances of TMU thermal: exynos: Add TMU features to check instead of using SOC type thermal: exynos: use device resource management infrastructure thermal: exynos: Add support to access common register for multistance thermal: exynos: Add support for exynos5440 TMU sensor. thermal: exynos: Fix to set the second point correction value properly thermal: exynos: Add thermal configuration data for exynos5440 TMU sensor thermal: exynos: Add a compensation logic on swapped e-fuse values thermal: exynos: Add hardware mode thermal calibration support Documentation: thermal: Explain the exynos thermal driver model thermal: exynos: Support for TMU regulator defined at device tree ARM: dts: Add device tree node for exynos5440 TMU controller arm: exynos: enable ARCH_HAS_TMU Lukasz Majewski (1): ARM: dts: thermal: exynos4: Add documentation for Exynos SoC thermal bindings .../devicetree/bindings/thermal/exynos-thermal.txt | 53 + Documentation/thermal/exynos_thermal | 43 +- arch/arm/boot/dts/exynos5440.dtsi | 30 +
[PATCH] ARM: EXYNOS: Fix support of Exynos4210 rev0 SoC
This patch extends exynos_init_time() function to handle Exynos4210 rev0
SoC, which differs in availability of system timers and needs different
clocksource initialization.
This makes it possible to use exynos_init_time() function as init_time
callback for all Exynos-based boards, including Universal_C210, which
originally had to use samsung_timer_init().
Signed-off-by: Tomasz Figa t.f...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
arch/arm/mach-exynos/Kconfig | 3 ++-
arch/arm/mach-exynos/common.c | 30 +-
arch/arm/mach-exynos/common.h | 2 ++
arch/arm/mach-exynos/mach-universal_c210.c | 5 ++---
4 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
index d19edff..ff18fc2 100644
--- a/arch/arm/mach-exynos/Kconfig
+++ b/arch/arm/mach-exynos/Kconfig
@@ -250,6 +250,7 @@ config MACH_ARMLEX4210
config MACH_UNIVERSAL_C210
bool Mobile UNIVERSAL_C210 Board
select CLKSRC_MMIO
+ select CLKSRC_SAMSUNG_PWM
select CPU_EXYNOS4210
select EXYNOS4_SETUP_FIMC
select EXYNOS4_SETUP_FIMD0
@@ -281,7 +282,6 @@ config MACH_UNIVERSAL_C210
select S5P_DEV_TV
select S5P_GPIO_INT
select S5P_SETUP_MIPIPHY
- select SAMSUNG_HRT
help
Machine support for Samsung Mobile Universal S5PC210 Reference
Board.
@@ -410,6 +410,7 @@ config MACH_EXYNOS4_DT
depends on ARCH_EXYNOS4
select ARM_AMBA
select CLKSRC_OF
+ select CLKSRC_SAMSUNG_PWM if CPU_EXYNOS4210
select CPU_EXYNOS4210
select KEYBOARD_SAMSUNG if INPUT_KEYBOARD
select PINCTRL
diff --git a/arch/arm/mach-exynos/common.c b/arch/arm/mach-exynos/common.c
index 745e304..a2d2012 100644
--- a/arch/arm/mach-exynos/common.c
+++ b/arch/arm/mach-exynos/common.c
@@ -10,12 +10,14 @@
*/
#include linux/kernel.h
+#include linux/bitops.h
#include linux/interrupt.h
#include linux/irq.h
#include linux/irqchip.h
#include linux/io.h
#include linux/device.h
#include linux/gpio.h
+#include clocksource/samsung_pwm.h
#include linux/sched.h
#include linux/serial_core.h
#include linux/of.h
@@ -302,6 +304,13 @@ static struct map_desc exynos5440_iodesc0[] __initdata = {
},
};
+static struct samsung_pwm_variant exynos4_pwm_variant = {
+ .bits = 32,
+ .div_base = 0,
+ .has_tint_cstat = true,
+ .tclk_mask = 0,
+};
+
void exynos4_restart(char mode, const char *cmd)
{
__raw_writel(0x1, S5P_SWRESET);
@@ -442,8 +451,20 @@ static void __init exynos5440_map_io(void)
iotable_init(exynos5440_iodesc0, ARRAY_SIZE(exynos5440_iodesc0));
}
+void __init exynos_set_timer_source(u8 channels)
+{
+ exynos4_pwm_variant.output_mask = BIT(SAMSUNG_PWM_NUM) - 1;
+ exynos4_pwm_variant.output_mask = ~channels;
+}
+
void __init exynos_init_time(void)
{
+ unsigned int timer_irqs[SAMSUNG_PWM_NUM] = {
+ EXYNOS4_IRQ_TIMER0_VIC, EXYNOS4_IRQ_TIMER1_VIC,
+ EXYNOS4_IRQ_TIMER2_VIC, EXYNOS4_IRQ_TIMER3_VIC,
+ EXYNOS4_IRQ_TIMER4_VIC,
+ };
+
if (of_have_populated_dt()) {
#ifdef CONFIG_OF
of_clk_init(NULL);
@@ -455,7 +476,14 @@ void __init exynos_init_time(void)
exynos4_clk_init(NULL, !soc_is_exynos4210(), S5P_VA_CMU,
readl(S5P_VA_CHIPID + 8) 1);
exynos4_clk_register_fixed_ext(xxti_f, xusbxti_f);
#endif
- mct_init(S5P_VA_SYSTIMER, EXYNOS4_IRQ_MCT_G0,
EXYNOS4_IRQ_MCT_L0, EXYNOS4_IRQ_MCT_L1);
+#ifdef CONFIG_CLKSRC_SAMSUNG_PWM
+ if (soc_is_exynos4210() samsung_rev() == EXYNOS4210_REV_0)
+ samsung_pwm_clocksource_init(S3C_VA_TIMER,
+ timer_irqs, exynos4_pwm_variant);
+ else
+#endif
+ mct_init(S5P_VA_SYSTIMER, EXYNOS4_IRQ_MCT_G0,
+ EXYNOS4_IRQ_MCT_L0, EXYNOS4_IRQ_MCT_L1);
}
}
diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
index 60dd35c..11fc1e2 100644
--- a/arch/arm/mach-exynos/common.h
+++ b/arch/arm/mach-exynos/common.h
@@ -32,6 +32,8 @@ void exynos4_clk_register_fixed_ext(unsigned long, unsigned
long);
void exynos_firmware_init(void);
+void exynos_set_timer_source(u8 channels);
+
#ifdef CONFIG_PM_GENERIC_DOMAINS
int exynos_pm_late_initcall(void);
#else
diff --git a/arch/arm/mach-exynos/mach-universal_c210.c
b/arch/arm/mach-exynos/mach-universal_c210.c
index 327d50d..74ddb2b 100644
--- a/arch/arm/mach-exynos/mach-universal_c210.c
+++ b/arch/arm/mach-exynos/mach-universal_c210.c
@@ -41,7 +41,6 @@
#include plat/mfc.h
#include plat/sdhci.h
#include plat/fimc-core.h
-#include plat/samsung-time.h
#include plat/camport.h
#include mach/map.h
@@ -1094,7 +1093,7 @@ static void __init universal_map_io(void)
{
[PATCH] ARM: SAMSUNG: devs: Add names to fimd0 IRQ resources
Since commit 1977e6d8 (drm/exynos: change the method for getting the
interrupt) the Exynos DRM FIMD driver requires IRQ resources to be
named. This patch fixes probe failure in non-DT cases by adding
appropriate resource names to fimd0 platform device.
Signed-off-by: Tomasz Figa t.f...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
arch/arm/plat-samsung/devs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/plat-samsung/devs.c b/arch/arm/plat-samsung/devs.c
index 30c2fe2..0f9c3f4 100644
--- a/arch/arm/plat-samsung/devs.c
+++ b/arch/arm/plat-samsung/devs.c
@@ -311,9 +311,9 @@ struct platform_device s5p_device_jpeg = {
#ifdef CONFIG_S5P_DEV_FIMD0
static struct resource s5p_fimd0_resource[] = {
[0] = DEFINE_RES_MEM(S5P_PA_FIMD0, SZ_32K),
- [1] = DEFINE_RES_IRQ(IRQ_FIMD0_VSYNC),
- [2] = DEFINE_RES_IRQ(IRQ_FIMD0_FIFO),
- [3] = DEFINE_RES_IRQ(IRQ_FIMD0_SYSTEM),
+ [1] = DEFINE_RES_IRQ_NAMED(IRQ_FIMD0_VSYNC, vsync),
+ [2] = DEFINE_RES_IRQ_NAMED(IRQ_FIMD0_FIFO, fifo),
+ [3] = DEFINE_RES_IRQ_NAMED(IRQ_FIMD0_SYSTEM, lcd_sys),
};
struct platform_device s5p_device_fimd0 = {
--
1.8.2.1
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] rtc: max8998: Check for pdata presence before dereferencing
Currently the driver can crash with a NULL pointer dereference if no pdata
is provided, despite of successful registration of MFD part. This patch
fixes the problem by adding a NULL check before dereferencing the pdata
pointer.
Signed-off-by: Tomasz Figa t.f...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/rtc/rtc-max8998.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-max8998.c b/drivers/rtc/rtc-max8998.c
index 48b6612..d5af7ba 100644
--- a/drivers/rtc/rtc-max8998.c
+++ b/drivers/rtc/rtc-max8998.c
@@ -285,7 +285,7 @@ static int max8998_rtc_probe(struct platform_device *pdev)
info-irq, ret);
dev_info(pdev-dev, RTC CHIP NAME: %s\n, pdev-id_entry-name);
- if (pdata-rtc_delay) {
+ if (pdata pdata-rtc_delay) {
info-lp3974_bug_workaround = true;
dev_warn(pdev-dev, LP3974 with RTC REGERR option.
RTC updates will be extremely slow.\n);
--
1.8.2.1
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rtc: max8998: Check for pdata presence before dereferencing
CCing Andrew Morton.
On 15 May 2013 20:46, Tomasz Figa t.f...@samsung.com wrote:
Currently the driver can crash with a NULL pointer dereference if no pdata
is provided, despite of successful registration of MFD part. This patch
fixes the problem by adding a NULL check before dereferencing the pdata
pointer.
Signed-off-by: Tomasz Figa t.f...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/rtc/rtc-max8998.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-max8998.c b/drivers/rtc/rtc-max8998.c
index 48b6612..d5af7ba 100644
--- a/drivers/rtc/rtc-max8998.c
+++ b/drivers/rtc/rtc-max8998.c
@@ -285,7 +285,7 @@ static int max8998_rtc_probe(struct platform_device *pdev)
info-irq, ret);
dev_info(pdev-dev, RTC CHIP NAME: %s\n, pdev-id_entry-name);
- if (pdata-rtc_delay) {
+ if (pdata pdata-rtc_delay) {
info-lp3974_bug_workaround = true;
dev_warn(pdev-dev, LP3974 with RTC REGERR option.
RTC updates will be extremely slow.\n);
--
1.8.2.1
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
With warm regards,
Sachin
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Pulls and drive strengths in the pinctrl world
Linus,
I'm currently working towards adapting exynos5250-snow (the ARM
Chromebook) to work well in the new world of pinctrl. We've got a
backport of exynos5250 pinctrl in our kernel-3.8 tree and are now
fixing all of the bugs that have popped up. Patches will be sent
upstream (where applicable) shortly.
...but I'm running into an issue when trying to specify pullups /
pulldowns and drive strengths on lines that are just GPIOs or just
interrupts. This is important not just for power usage but also for
proper functioning (the default internal pulldown was overpowering the
weak external pullup in one case). In the old GPIO specifier you
could do specify pulls, but the new simpler one doesn't allow it.
I've managed to make things work and you can see my progress at
https://gerrit.chromium.org/gerrit/#/c/51232/, but it feels a bit
awkward. Is there a better way? If so, then I'm all ears! :)
If not, then I guess that what I have will have to do for now. ...but
I really wish that:
* The GPIO specifier could specify initial drive strength and pull
values for pins.
* The interrupt specifier could specify pull values for pins (drive
strength shouldn't be needed since these are inputs).
Some examples from the gerrit CL referenced above...
Here's how I need to do things when I'm using just an interrupt:
pinctrl@1140 {
cyapa_irq: cyapa-irq {
samsung,pins = gpx1-2;
samsung,pin-function = 0xf;
samsung,pin-pud = 0;
samsung,pin-drv = 0;
};
};
trackpad {
reg = 0x67;
compatible = cypress,cyapa;
interrupts = 2 0;
interrupt-parent = gpx1;
pinctrl-names = default;
pinctrl-0 = cyapa_irq;
wakeup-source;
};
I really wish I could add a 3rd number to the interrupt specifier for
pud and skip the pinctrl bit:
trackpad {
reg = 0x67;
compatible = cypress,cyapa;
interrupts = 2 0 0;
interrupt-parent = gpx1;
wakeup-source;
};
An example with the GPIO specifier instead of the interrupt one:
pinctrl@1140 {
ptn3460_gpios: ptn3460-gpios {
samsung,pins = gpy2-5, gpx1-5;
samsung,pin-function = 1;
samsung,pin-pud = 0;
samsung,pin-drv = 0;
};
};
ptn3460-bridge@20 {
compatible = nxp,ptn3460;
reg = 0x20;
powerdown-gpio = gpy2 5 0;
reset-gpio = gpx1 5 0;
edid-emulation = 5;
pinctrl-names = default;
pinctrl-0 = ptn3460_gpios;
};
I don't want to specify function/direction (code can handle that), but do wish
I could specify the pulls and strength. Perhaps:
ptn3460-bridge@20 {
compatible = nxp,ptn3460;
reg = 0x20;
powerdown-gpio = gpy2 5 0 0 0;
reset-gpio = gpx1 5 0 0 0;
edid-emulation = 5;
};
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
Hi Doug,
There is no better way at the moment, but...
On Wednesday 15 of May 2013 09:44:22 Doug Anderson wrote:
Linus,
I'm currently working towards adapting exynos5250-snow (the ARM
Chromebook) to work well in the new world of pinctrl. We've got a
backport of exynos5250 pinctrl in our kernel-3.8 tree and are now
fixing all of the bugs that have popped up. Patches will be sent
upstream (where applicable) shortly.
...but I'm running into an issue when trying to specify pullups /
pulldowns and drive strengths on lines that are just GPIOs or just
interrupts. This is important not just for power usage but also for
proper functioning (the default internal pulldown was overpowering the
weak external pullup in one case). In the old GPIO specifier you
could do specify pulls, but the new simpler one doesn't allow it.
I've managed to make things work and you can see my progress at
https://gerrit.chromium.org/gerrit/#/c/51232/, but it feels a bit
awkward. Is there a better way? If so, then I'm all ears! :)
If not, then I guess that what I have will have to do for now. ...but
I really wish that:
* The GPIO specifier could specify initial drive strength and pull
values for pins.
* The interrupt specifier could specify pull values for pins (drive
strength shouldn't be needed since these are inputs).
Some examples from the gerrit CL referenced above...
Here's how I need to do things when I'm using just an interrupt:
pinctrl@1140 {
cyapa_irq: cyapa-irq {
samsung,pins = gpx1-2;
samsung,pin-function = 0xf;
You can omit samsung,pin-function here.
samsung,pin-pud = 0;
samsung,pin-drv = 0;
For inputs I guess you can omit samsung,pin-drv as well.
};
};
trackpad {
reg = 0x67;
compatible = cypress,cyapa;
interrupts = 2 0;
interrupt-parent = gpx1;
pinctrl-names = default;
pinctrl-0 = cyapa_irq;
wakeup-source;
};
I really wish I could add a 3rd number to the interrupt specifier for
pud and skip the pinctrl bit:
trackpad {
reg = 0x67;
compatible = cypress,cyapa;
interrupts = 2 0 0;
Hmm, looks pretty good to me.
interrupt-parent = gpx1;
wakeup-source;
};
An example with the GPIO specifier instead of the interrupt one:
pinctrl@1140 {
ptn3460_gpios: ptn3460-gpios {
samsung,pins = gpy2-5, gpx1-5;
samsung,pin-function = 1;
samsung,pin-pud = 0;
samsung,pin-drv = 0;
};
};
ptn3460-bridge@20 {
compatible = nxp,ptn3460;
reg = 0x20;
powerdown-gpio = gpy2 5 0;
reset-gpio = gpx1 5 0;
edid-emulation = 5;
pinctrl-names = default;
pinctrl-0 = ptn3460_gpios;
};
I don't want to specify function/direction (code can handle that), but
do wish I could specify the pulls and strength. Perhaps:
ptn3460-bridge@20 {
compatible = nxp,ptn3460;
reg = 0x20;
powerdown-gpio = gpy2 5 0 0 0;
reset-gpio = gpx1 5 0 0 0;
This looks fine to me as well.
Implementation of both shouldn't be too complicated, so it might be worth
giving a try. Keep in mind that old bindings must be supported as well
(based on #interrupt-cells and #gpio-cells values, I guess).
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
On Wed, May 15, 2013 at 10:26 AM, Tomasz Figa tomasz.f...@gmail.com wrote:
I don't want to specify function/direction (code can handle that), but
do wish I could specify the pulls and strength. Perhaps:
ptn3460-bridge@20 {
compatible = nxp,ptn3460;
reg = 0x20;
powerdown-gpio = gpy2 5 0 0 0;
reset-gpio = gpx1 5 0 0 0;
This looks fine to me as well.
Implementation of both shouldn't be too complicated, so it might be worth
giving a try. Keep in mind that old bindings must be supported as well
(based on #interrupt-cells and #gpio-cells values, I guess).
Given the late discovery of this pretty major drawback, I don't think
we should worry too much about backwards compatibility in this case
and instead just move everyone over asap to whatever the new binding
is (when we agree on something).
Also, it looks like the gpio bindings were never updated with the
pinctrl changes. Tsk, tsk, tsk. Bad.
We have the option of either adding new fields for pulls and strength,
but we can also split the existing flags field similar to how we did
with the old binding, and take 8 bits each for pulls and strength, or
somesuch. That'll be less of a change w.r.t. existing device trees and
bindings.
-Olof
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
On Wed, May 15, 2013 at 6:44 PM, Doug Anderson diand...@google.com wrote:
Pls include Stephen Warren on mailings regarding DT mappings,
he's very good at this stuff.
I'm running into an issue when trying to specify pullups /
pulldowns and drive strengths on lines that are just GPIOs or just
interrupts. This is important not just for power usage but also for
proper functioning (the default internal pulldown was overpowering the
weak external pullup in one case). In the old GPIO specifier you
could do specify pulls, but the new simpler one doesn't allow it.
Background:
The idea with the subsystems is that the GPIO subsystem will
handle any aspect of GPIOs which are not necessarily
synonymous to pins.
So the two subsystems are orthogonal and the decisions in
each subsystem may result on combined electrical effects.
If there are cross-dependencies, GPIO ranges are used to
cross-map the GPIOs to pins.
I've managed to make things work and you can see my progress at
https://gerrit.chromium.org/gerrit/#/c/51232/, but it feels a bit
awkward. Is there a better way? If so, then I'm all ears! :
This seems good. default states are used to set up pins.
But please use the preprocessor to provide symbolic names for
the configurations. See for example these two patches from
J-C:
http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg32164.html
I really wish that:
* The GPIO specifier could specify initial drive strength and pull
values for pins.
* The interrupt specifier could specify pull values for pins (drive
strength shouldn't be needed since these are inputs).
The subsystem does not differentiate between different configs,
what you say is that you want GPIO and interrupt specifiers
to pass arbitrary configs.
Here's how I need to do things when I'm using just an interrupt:
pinctrl@1140 {
cyapa_irq: cyapa-irq {
samsung,pins = gpx1-2;
samsung,pin-function = 0xf;
samsung,pin-pud = 0;
samsung,pin-drv = 0;
};
};
trackpad {
reg = 0x67;
compatible = cypress,cyapa;
interrupts = 2 0;
interrupt-parent = gpx1;
pinctrl-names = default;
pinctrl-0 = cyapa_irq;
wakeup-source;
};
I really wish I could add a 3rd number to the interrupt specifier for
pud and skip the pinctrl bit:
trackpad {
reg = 0x67;
compatible = cypress,cyapa;
interrupts = 2 0 0;
interrupt-parent = gpx1;
wakeup-source;
};
I don't think the idea with device tree is to write as compact trees
as possible, but as expressive and exact yet abstract trees as
possible for OS independence.
Instead of referring to a node containing the config relevant for
another piece of hardware and associating this with the ampersand
notation (as is done with regulators, clocks, dma channels ...)
you here want to break that pattern totally and just hardcode
a numeric argument into the specifier. (Well could be a #define
using includes, but...)
That is not the pattern used so far I've seen to indicate dependent
resources. Dependent resources are passed using the ampersand.
While a number may suffice to describe all config for your hardware,
other pinctrl hardware needs more than one single numeric
argument. And device trees should be similar to each other.
If you're not doing that using the ampersand, the same could
potentially be done for a regulator powering a GPIO pin
and you get a fourth argument, and a fifth argument supplying
the color of the LED at the other end and ... I guess this is
why ampersands are being used.
Other than that I think you should ask a DT expert and I'm
no such.
An example with the GPIO specifier instead of the interrupt one:
pinctrl@1140 {
ptn3460_gpios: ptn3460-gpios {
samsung,pins = gpy2-5, gpx1-5;
samsung,pin-function = 1;
samsung,pin-pud = 0;
samsung,pin-drv = 0;
};
};
ptn3460-bridge@20 {
compatible = nxp,ptn3460;
reg = 0x20;
powerdown-gpio = gpy2 5 0;
reset-gpio = gpx1 5 0;
edid-emulation = 5;
pinctrl-names = default;
pinctrl-0 = ptn3460_gpios;
};
I don't want to specify function/direction (code can handle that), but do wish
I could specify the pulls and strength. Perhaps:
ptn3460-bridge@20 {
compatible = nxp,ptn3460;
reg = 0x20;
powerdown-gpio = gpy2 5 0 0 0;
reset-gpio = gpx1 5 0 0 0;
edid-emulation = 5;
};
Dito.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs
On Tue, May 14, 2013 at 3:51 PM, Heiko Stübner he...@sntech.de wrote: Am Dienstag, 14. Mai 2013, 14:47:19 schrieb Linus Walleij: On Sat, May 11, 2013 at 1:31 PM, Heiko Stübner he...@sntech.de wrote: Conceptually the s3c24xx-dma feels like a distant relative of the pl08x with numerous virtual channels being mapped to a lot less physical ones. The driver therefore borrows a lot from the amba-pl08x driver in this regard. Functionality-wise the driver gains a memcpy ability in addition to the slave_sg one. The driver currently only supports the newer SoCs which can use any physical channel for any dma slave. Support for the older SoCs where each channel only supports a subset of possible dma slaves will have to be added later. Tested on a s3c2416-based board, memcpy using the dmatest module and slave_sg partially using the spi-s3c64xx driver. Signed-off-by: Heiko Stuebner he...@sntech.de So have I understood correctly if I assume that *some* S3C variants, i.e. this: arch/arm/mach-s3c64xx/dma.c have a vanilla, unmodified, or just slightly modified PL08x block, while this DMAC is something probably based on the PL08x where some ASIC engineer has had a good time hacking around in the VHDL code to meet some feature requirements. Correct? Or plausible guess? You're guess is at good as mine :-) . The public s3c64xx (ARM11 based) documentation says that it is using s PL080 as dma controller while the s3c24xx (ARM9 based) SoCs have this one, which doesn't come with any label in the manuals. Similar to the s3c64xx using a vic, while the s3c24xx uses something homegrown. The relationship description was more based on the concepts used, i.e. the virtual channel concept and general handling of dma transfers feel somehow similar - as I said these are my first steps into this, so I still need to understand a lot. OK then, a separate driver seems required, will look a bit closer at the patch as such. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs
On Sat, May 11, 2013 at 1:31 PM, Heiko Stübner he...@sntech.de wrote:
This adds a new driver to support the s3c24xx dma using the dmaengine
and make the old one in mach-s3c24xx obsolete in the long run.
Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
with numerous virtual channels being mapped to a lot less physical ones.
The driver therefore borrows a lot from the amba-pl08x driver in this
regard. Functionality-wise the driver gains a memcpy ability in addition
to the slave_sg one.
The driver currently only supports the newer SoCs which can use any
physical channel for any dma slave. Support for the older SoCs where
each channel only supports a subset of possible dma slaves will have to
be added later.
Tested on a s3c2416-based board, memcpy using the dmatest module and
slave_sg partially using the spi-s3c64xx driver.
Signed-off-by: Heiko Stuebner he...@sntech.de
(...)
+static u32 s3c24xx_dma_getbytes_chan(struct s3c24xx_dma_chan *s3cchan)
+{
+ struct s3c24xx_dma_phy *phy = s3cchan-phy;
+ struct s3c24xx_txd *txd = s3cchan-at;
+ u32 tc = readl(phy-base + DSTAT) DSTAT_CURRTC_MASK;
+
+ switch (txd-dcon DCON_DSZ_MASK) {
+ case DCON_DSZ_BYTE:
+ return tc;
+ case DCON_DSZ_HALFWORD:
+ return tc * 2;
+ case DCON_DSZ_WORD:
+ return tc * 4;
+ default:
+ break;
+ }
+
+ BUG();
Isn't that a bit nasty. This macro should be used with care and we
should recover if possible. dev_err()?
+/*
+ * Set the initial DMA register values.
+ * Put them into the hardware and start the transfer.
+ */
+static void s3c24xx_dma_start_next_txd(struct s3c24xx_dma_chan *s3cchan)
+{
+ struct s3c24xx_dma_engine *s3cdma = s3cchan-host;
+ struct s3c24xx_dma_phy *phy = s3cchan-phy;
+ struct virt_dma_desc *vd = vchan_next_desc(s3cchan-vc);
+ struct s3c24xx_txd *txd = to_s3c24xx_txd(vd-tx);
+ u32 dcon = txd-dcon;
+ u32 val;
+
+ list_del(txd-vd.node);
+
+ s3cchan-at = txd;
+
+ /* Wait for channel inactive */
+ while (s3c24xx_dma_phy_busy(phy))
+ cpu_relax();
+
+ /* transfer-size and -count from len and width */
+ switch (txd-width) {
+ case 1:
+ dcon |= DCON_DSZ_BYTE | txd-len;
+ break;
+ case 2:
+ dcon |= DCON_DSZ_HALFWORD | (txd-len / 2);
+ break;
+ case 4:
+ dcon |= DCON_DSZ_WORD | (txd-len / 4);
+ break;
+ }
+
+ if (s3cchan-slave) {
+ if (s3cdma-sdata-has_reqsel) {
+ int reqsel = s3cdma-pdata-reqsel_map[s3cchan-id];
+ writel((reqsel 1) | DMAREQSEL_HW,
+ phy-base + DMAREQSEL);
+ } else {
+ dev_err(s3cdma-pdev-dev, non-reqsel
unsupported\n);
+ return;
+ dcon |= DCON_HWTRIG;
+ }
+ } else {
+ if (s3cdma-sdata-has_reqsel) {
+ writel(0, phy-base + DMAREQSEL);
+ } else {
+ dev_err(s3cdma-pdev-dev, non-reqsel
unsupported\n);
+ return;
+ }
+ }
+
+ writel(txd-src_addr, phy-base + DISRC);
+ writel(txd-disrcc, phy-base + DISRCC);
+ writel(txd-dst_addr, phy-base + DIDST);
+ writel(txd-didstc, phy-base + DIDSTC);
+
+ writel(dcon, phy-base + DCON);
+
+ val = readl(phy-base + DMASKTRIG);
+ val = ~DMASKTRIG_STOP;
+ val |= DMASKTRIG_ON;
+ writel(val, phy-base + DMASKTRIG);
+
+ /* trigger the dma operation for memcpy transfers */
+ if (!s3cchan-slave) {
+ val = readl(phy-base + DMASKTRIG);
+ val |= DMASKTRIG_SWTRIG;
+ writel(val, phy-base + DMASKTRIG);
+ }
+}
Since you have a few readl() and writel() in potentially
time-critical code, consider using readl_relaxed() and
writel_relaxed().
+static irqreturn_t s3c24xx_dma_irq(int irq, void *data)
+{
+ struct s3c24xx_dma_phy *phy = data;
+ struct s3c24xx_dma_chan *s3cchan = phy-serving;
+ struct s3c24xx_txd *txd;
+
+ dev_dbg(phy-host-pdev-dev, interrupt on channel %d\n, phy-id);
+
+ if (!s3cchan) {
+ dev_err(phy-host-pdev-dev, interrupt on unused channel
%d\n,
+ phy-id);
+ return IRQ_NONE;
+ }
+
+ spin_lock(s3cchan-vc.lock);
+ txd = s3cchan-at;
+ if (txd) {
+ s3cchan-at = NULL;
+ vchan_cookie_complete(txd-vd);
+
+ /*
+* And start the next descriptor (if any),
+* otherwise free this channel.
+*/
+ if
Re: [RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs
Am Mittwoch, 15. Mai 2013, 20:53:32 schrieb Linus Walleij:
On Sat, May 11, 2013 at 1:31 PM, Heiko Stübner he...@sntech.de wrote:
This adds a new driver to support the s3c24xx dma using the dmaengine
and make the old one in mach-s3c24xx obsolete in the long run.
Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
with numerous virtual channels being mapped to a lot less physical ones.
The driver therefore borrows a lot from the amba-pl08x driver in this
regard. Functionality-wise the driver gains a memcpy ability in addition
to the slave_sg one.
The driver currently only supports the newer SoCs which can use any
physical channel for any dma slave. Support for the older SoCs where
each channel only supports a subset of possible dma slaves will have to
be added later.
Tested on a s3c2416-based board, memcpy using the dmatest module and
slave_sg partially using the spi-s3c64xx driver.
Signed-off-by: Heiko Stuebner he...@sntech.de
(...)
+static u32 s3c24xx_dma_getbytes_chan(struct s3c24xx_dma_chan *s3cchan)
+{
+ struct s3c24xx_dma_phy *phy = s3cchan-phy;
+ struct s3c24xx_txd *txd = s3cchan-at;
+ u32 tc = readl(phy-base + DSTAT) DSTAT_CURRTC_MASK;
+
+ switch (txd-dcon DCON_DSZ_MASK) {
+ case DCON_DSZ_BYTE:
+ return tc;
+ case DCON_DSZ_HALFWORD:
+ return tc * 2;
+ case DCON_DSZ_WORD:
+ return tc * 4;
+ default:
+ break;
+ }
+
+ BUG();
Isn't that a bit nasty. This macro should be used with care and we
should recover if possible. dev_err()?
runtime_config already denies any settings not in the 1,2 or 4bytes range -
the default-part should therefore never be reached. So if any other value
magically appears in the register and triggers the default-part, something is
seriously wrong. So my guess is, the BUG might be appropriate.
On the other hand the whole default+BUG part could also simply go away, for
the same reasons.
+/*
+ * Set the initial DMA register values.
+ * Put them into the hardware and start the transfer.
+ */
+static void s3c24xx_dma_start_next_txd(struct s3c24xx_dma_chan *s3cchan)
+{
+ struct s3c24xx_dma_engine *s3cdma = s3cchan-host;
+ struct s3c24xx_dma_phy *phy = s3cchan-phy;
+ struct virt_dma_desc *vd = vchan_next_desc(s3cchan-vc);
+ struct s3c24xx_txd *txd = to_s3c24xx_txd(vd-tx);
+ u32 dcon = txd-dcon;
+ u32 val;
+
+ list_del(txd-vd.node);
+
+ s3cchan-at = txd;
+
+ /* Wait for channel inactive */
+ while (s3c24xx_dma_phy_busy(phy))
+ cpu_relax();
+
+ /* transfer-size and -count from len and width */
+ switch (txd-width) {
+ case 1:
+ dcon |= DCON_DSZ_BYTE | txd-len;
+ break;
+ case 2:
+ dcon |= DCON_DSZ_HALFWORD | (txd-len / 2);
+ break;
+ case 4:
+ dcon |= DCON_DSZ_WORD | (txd-len / 4);
+ break;
+ }
+
+ if (s3cchan-slave) {
+ if (s3cdma-sdata-has_reqsel) {
+ int reqsel =
s3cdma-pdata-reqsel_map[s3cchan-id]; +
writel((reqsel 1) | DMAREQSEL_HW,
+ phy-base + DMAREQSEL);
+ } else {
+ dev_err(s3cdma-pdev-dev, non-reqsel
unsupported\n); + return;
+ dcon |= DCON_HWTRIG;
+ }
+ } else {
+ if (s3cdma-sdata-has_reqsel) {
+ writel(0, phy-base + DMAREQSEL);
+ } else {
+ dev_err(s3cdma-pdev-dev, non-reqsel
unsupported\n); + return;
+ }
+ }
+
+ writel(txd-src_addr, phy-base + DISRC);
+ writel(txd-disrcc, phy-base + DISRCC);
+ writel(txd-dst_addr, phy-base + DIDST);
+ writel(txd-didstc, phy-base + DIDSTC);
+
+ writel(dcon, phy-base + DCON);
+
+ val = readl(phy-base + DMASKTRIG);
+ val = ~DMASKTRIG_STOP;
+ val |= DMASKTRIG_ON;
+ writel(val, phy-base + DMASKTRIG);
+
+ /* trigger the dma operation for memcpy transfers */
+ if (!s3cchan-slave) {
+ val = readl(phy-base + DMASKTRIG);
+ val |= DMASKTRIG_SWTRIG;
+ writel(val, phy-base + DMASKTRIG);
+ }
+}
Since you have a few readl() and writel() in potentially
time-critical code, consider using readl_relaxed() and
writel_relaxed().
You're right of course.
If I understand the writel semantics and the thread from you from 2011 [0]
correctly, only the writel to DMASKTRIG mustn't be relaxed to make sure the
settings registers are written to before, so like:
Re: Pulls and drive strengths in the pinctrl world
Tomasz,
Thanks for your comments. I'm glad I'm not totally off-track. I'll
respond to most things in reply to Linus' email, but a few here:
On Wed, May 15, 2013 at 10:26 AM, Tomasz Figa tomasz.f...@gmail.com wrote:
pinctrl@1140 {
cyapa_irq: cyapa-irq {
samsung,pins = gpx1-2;
samsung,pin-function = 0xf;
You can omit samsung,pin-function here.
One potential reason for leaving them is the hopes that it might cause
a little less line glitching, especially in the case of outputs.
There is some delay between the pinmux being configured at the start
of device probe and the device actually claiming the GPIO. Things
might be worse in the case of deferred probe (?). Can you think of
any reason to remove (other than yet more lines of device tree to deal
with)?
samsung,pin-pud = 0;
samsung,pin-drv = 0;
For inputs I guess you can omit samsung,pin-drv as well.
I will probably leave them even for inputs. They shouldn't matter but
I like the idea of initting things to a known state...
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs
On 05/15/2013 10:31 PM, Heiko Stübner wrote: + BUG(); Isn't that a bit nasty. This macro should be used with care and we should recover if possible. dev_err()? runtime_config already denies any settings not in the 1,2 or 4bytes range - the default-part should therefore never be reached. So if any other value magically appears in the register and triggers the default-part, something is seriously wrong. So my guess is, the BUG might be appropriate. On the other hand the whole default+BUG part could also simply go away, for the same reasons. IMHO BUG() is not needed at all. As Linus suggested dev_err() is such case or WARN_ON() would be more appropriate. This has been discussed in the past extensively, not sure if you are aware of the other Linus' opinion on BUG()/BUG_ON() proliferation: https://lkml.org/lkml/2012/9/27/461 Regards, Sylwester -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
Linus,
Thank you for your comments. See below...
Stephen: sorry for missing you earlier! :(
On Wed, May 15, 2013 at 11:29 AM, Linus Walleij
linus.wall...@linaro.org wrote:
But please use the preprocessor to provide symbolic names for
the configurations. See for example these two patches from
J-C:
http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg32164.html
Ah, that does look nice! This probably needs to be addressed in a
separate patch to cleanup all of the samsung pinctrl devicetrees.
I don't think the idea with device tree is to write as compact trees
as possible, but as expressive and exact yet abstract trees as
possible for OS independence.
The compactness was one benefit, but also it was about trying to avoid
excessive duplication of information. I found it awkward that I
needed to list that my interrupt was gpx1-2 in two different ways.
I would find it just as good if I could express things like this (for
interrupts):
pinctrl@1140 {
cyapa_irq: cyapa-irq {
samsung,pins = gpx1-2;
samsung,pin-function = 0xf;
samsung,pin-pud = 0;
samsung,pin-drv = 0;
interrupt-controller;
#interrupt-cells = 1;
};
};
trackpad {
reg = 0x67;
compatible = cypress,cyapa;
interrupt-parent = cyapa_irq;
interrupts = 0;
wakeup-source;
};
In this case I'm not saying that my interrupt parent is gpx1-2 in
two separate places that could diverge.
I could come up with a similar example for GPIOs.
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
On Wednesday 15 of May 2013 14:19:18 Doug Anderson wrote:
Tomasz,
Thanks for your comments. I'm glad I'm not totally off-track. I'll
respond to most things in reply to Linus' email, but a few here:
On Wed, May 15, 2013 at 10:26 AM, Tomasz Figa tomasz.f...@gmail.com
wrote:
pinctrl@1140 {
cyapa_irq: cyapa-irq {
samsung,pins = gpx1-2;
samsung,pin-function = 0xf;
You can omit samsung,pin-function here.
One potential reason for leaving them is the hopes that it might cause
a little less line glitching, especially in the case of outputs.
There is some delay between the pinmux being configured at the start
of device probe and the device actually claiming the GPIO. Things
might be worse in the case of deferred probe (?). Can you think of
any reason to remove (other than yet more lines of device tree to deal
with)?
Well, actually in case of interrupts the function should not be configured
manually, because it is likely to cause a false interrupt to be caught,
before appropriate interrupt trigger type is configured. The correct way
is to leave setting pin function to EINT to the pin control driver once
the trigger gets configured (the pin control driver configures pin
function from set_irq_type callback).
samsung,pin-pud = 0;
samsung,pin-drv = 0;
For inputs I guess you can omit samsung,pin-drv as well.
I will probably leave them even for inputs. They shouldn't matter but
I like the idea of initting things to a known state...
Well, the binding you proposed for interrupts doesn't initialize it. This
is why I pointed that it can be omitted using current way as well.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
On Wednesday 15 of May 2013 14:31:20 Doug Anderson wrote:
Linus,
Thank you for your comments. See below...
Stephen: sorry for missing you earlier! :(
On Wed, May 15, 2013 at 11:29 AM, Linus Walleij
linus.wall...@linaro.org wrote:
But please use the preprocessor to provide symbolic names for
the configurations. See for example these two patches from
J-C:
http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg321
64.html
Ah, that does look nice! This probably needs to be addressed in a
separate patch to cleanup all of the samsung pinctrl devicetrees.
I don't think the idea with device tree is to write as compact trees
as possible, but as expressive and exact yet abstract trees as
possible for OS independence.
The compactness was one benefit, but also it was about trying to avoid
excessive duplication of information. I found it awkward that I
needed to list that my interrupt was gpx1-2 in two different ways.
I would find it just as good if I could express things like this (for
interrupts):
pinctrl@1140 {
cyapa_irq: cyapa-irq {
samsung,pins = gpx1-2;
samsung,pin-function = 0xf;
samsung,pin-pud = 0;
samsung,pin-drv = 0;
interrupt-controller;
#interrupt-cells = 1;
};
};
trackpad {
reg = 0x67;
compatible = cypress,cyapa;
interrupt-parent = cyapa_irq;
interrupts = 0;
wakeup-source;
};
In this case I'm not saying that my interrupt parent is gpx1-2 in
two separate places that could diverge.
This will be hard, since the phandle in interrupt-parent is represented by
an IRQ domain in kernel code. One-interrupt IRQ domains seem a bit awkward
to me.
Since we are already going to modify the binding, let's think a bit more
about this problem and try to figure out a solution that doesn't add any
disadvantages (at least any significant) to avoid such situation in future
again.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
On Wednesday 15 of May 2013 23:41:54 Tomasz Figa wrote:
On Wednesday 15 of May 2013 14:31:20 Doug Anderson wrote:
Linus,
Thank you for your comments. See below...
Stephen: sorry for missing you earlier! :(
On Wed, May 15, 2013 at 11:29 AM, Linus Walleij
linus.wall...@linaro.org wrote:
But please use the preprocessor to provide symbolic names for
the configurations. See for example these two patches from
J-C:
http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg3
21
64.html
Ah, that does look nice! This probably needs to be addressed in a
separate patch to cleanup all of the samsung pinctrl devicetrees.
I don't think the idea with device tree is to write as compact trees
as possible, but as expressive and exact yet abstract trees as
possible for OS independence.
The compactness was one benefit, but also it was about trying to avoid
excessive duplication of information. I found it awkward that I
needed to list that my interrupt was gpx1-2 in two different ways.
I would find it just as good if I could express things like this (for
interrupts):
pinctrl@1140 {
cyapa_irq: cyapa-irq {
samsung,pins = gpx1-2;
samsung,pin-function = 0xf;
samsung,pin-pud = 0;
samsung,pin-drv = 0;
interrupt-controller;
#interrupt-cells = 1;
};
};
trackpad {
reg = 0x67;
compatible = cypress,cyapa;
interrupt-parent = cyapa_irq;
interrupts = 0;
wakeup-source;
};
In this case I'm not saying that my interrupt parent is gpx1-2 in
two separate places that could diverge.
This will be hard, since the phandle in interrupt-parent is represented
by an IRQ domain in kernel code. One-interrupt IRQ domains seem a bit
awkward to me.
Ahh, not even saying that a single interrupt signal is not really an
interrupt controller, which would make device tree diverge from real
hardware description.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs
Am Mittwoch, 15. Mai 2013, 23:20:08 schrieb Sylwester Nawrocki: On 05/15/2013 10:31 PM, Heiko Stübner wrote: + BUG(); Isn't that a bit nasty. This macro should be used with care and we should recover if possible. dev_err()? runtime_config already denies any settings not in the 1,2 or 4bytes range - the default-part should therefore never be reached. So if any other value magically appears in the register and triggers the default-part, something is seriously wrong. So my guess is, the BUG might be appropriate. On the other hand the whole default+BUG part could also simply go away, for the same reasons. IMHO BUG() is not needed at all. As Linus suggested dev_err() is such case or WARN_ON() would be more appropriate. This has been discussed in the past extensively, not sure if you are aware of the other Linus' opinion on BUG()/BUG_ON() proliferation: https://lkml.org/lkml/2012/9/27/461 Very interesting read and I'll keep this in mind in the future. What about the other option ... i.e. simply getting rid of the whole error handling, as the other code paths should already make sure that only valid values get written into the register. Can the value change in the register somehow on its own without kernel intervention, or does this not happen? Thanks Heiko -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
Tomasz, On Wed, May 15, 2013 at 2:36 PM, Tomasz Figa tomasz.f...@gmail.com wrote: One potential reason for leaving them is the hopes that it might cause a little less line glitching, especially in the case of outputs. There is some delay between the pinmux being configured at the start of device probe and the device actually claiming the GPIO. Things might be worse in the case of deferred probe (?). Can you think of any reason to remove (other than yet more lines of device tree to deal with)? Well, actually in case of interrupts the function should not be configured manually, because it is likely to cause a false interrupt to be caught, before appropriate interrupt trigger type is configured. The correct way is to leave setting pin function to EINT to the pin control driver once the trigger gets configured (the pin control driver configures pin function from set_irq_type callback). Ah, OK. I'll set to input for these. I will probably leave them even for inputs. They shouldn't matter but I like the idea of initting things to a known state... Well, the binding you proposed for interrupts doesn't initialize it. This is why I pointed that it can be omitted using current way as well. Agreed. ...though you could say that the actual code in that case would just be setting the drive strength to 0 (for consistency). ;) -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
Tomasz, On Wed, May 15, 2013 at 2:41 PM, Tomasz Figa tomasz.f...@gmail.com wrote: This will be hard, since the phandle in interrupt-parent is represented by an IRQ domain in kernel code. One-interrupt IRQ domains seem a bit awkward to me. Since we are already going to modify the binding, let's think a bit more about this problem and try to figure out a solution that doesn't add any disadvantages (at least any significant) to avoid such situation in future again. I'm definitely not super familiar with the implementation at that level of detail, so don't take my proposed syntax as something I've thought all the way through. ...but hopefully you understand what I'm getting at in terms of eliminating duplication? Thanks! :) -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs
On Wednesday 15 of May 2013 23:48:31 Heiko Stübner wrote:
Am Mittwoch, 15. Mai 2013, 23:20:08 schrieb Sylwester Nawrocki:
On 05/15/2013 10:31 PM, Heiko Stübner wrote:
+ BUG();
Isn't that a bit nasty. This macro should be used with care and
we
should recover if possible. dev_err()?
runtime_config already denies any settings not in the 1,2 or 4bytes
range - the default-part should therefore never be reached. So if
any other value magically appears in the register and triggers the
default-part, something is seriously wrong. So my guess is, the BUG
might be appropriate.
On the other hand the whole default+BUG part could also simply go
away,
for the same reasons.
IMHO BUG() is not needed at all. As Linus suggested dev_err() is such
case or WARN_ON() would be more appropriate. This has been discussed
in the past extensively, not sure if you are aware of the other
Linus' opinion on BUG()/BUG_ON() proliferation:
https://lkml.org/lkml/2012/9/27/461
Very interesting read and I'll keep this in mind in the future. What
about the other option ... i.e. simply getting rid of the whole error
handling, as the other code paths should already make sure that only
valid values get written into the register.
Can the value change in the register somehow on its own without kernel
intervention, or does this not happen?
Hmm, it depends on hardware, I guess. Not sure how it works on this
particular IP.
Still, the mentioned BUG() was about a value in a driver-filled struct,
wasn't it?
/* Quoting the the code for reference */
+static u32 s3c24xx_dma_getbytes_chan(struct s3c24xx_dma_chan *s3cchan)
+{
+ struct s3c24xx_dma_phy *phy = s3cchan-phy;
+ struct s3c24xx_txd *txd = s3cchan-at;
+ u32 tc = readl(phy-base + DSTAT) DSTAT_CURRTC_MASK;
+
+ switch (txd-dcon DCON_DSZ_MASK) {
+ case DCON_DSZ_BYTE:
+ return tc;
+ case DCON_DSZ_HALFWORD:
+ return tc * 2;
+ case DCON_DSZ_WORD:
+ return tc * 4;
+ default:
+ break;
+ }
+
+ BUG();
(Btw. I don't see anything setting the DCON_DSZ bits in this field. Am I
missing something?)
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
On Wednesday 15 of May 2013 15:01:23 Doug Anderson wrote: Tomasz, On Wed, May 15, 2013 at 2:41 PM, Tomasz Figa tomasz.f...@gmail.com wrote: This will be hard, since the phandle in interrupt-parent is represented by an IRQ domain in kernel code. One-interrupt IRQ domains seem a bit awkward to me. Since we are already going to modify the binding, let's think a bit more about this problem and try to figure out a solution that doesn't add any disadvantages (at least any significant) to avoid such situation in future again. I'm definitely not super familiar with the implementation at that level of detail, so don't take my proposed syntax as something I've thought all the way through. ...but hopefully you understand what I'm getting at in terms of eliminating duplication? Yes. I don't like the current way too much either, duplication being one of the reasons. Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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 1/3] [media] s5p-jpeg: Add support for Exynos4x12 and 5250
Hi George, Thanks for the patches. Sorry, I can't review the $subject patch in detail as is, there is way too many things done in this single patch. It looks more like a driver replacement. It is even hard to edit due to its size in my e-mail client. Hence, may I ask you to split it into several patches, each possibly including single logical change, with an explanation what the patch does and why, e.g.: - encoder/decoder code split into different files (I'm not 100% sure it is needed), - multiplanar format support addition, - software watchdog addition, - the quantization/Huffman tables modification, - device tree support addition, - ... The reason I'm asking for it is also there seems to be some unrelated or unnecessary changes, like, e.g. introducing several JPEG fourccs for different YCbCr subsampling or adding unused v4l2 control ioctls ( jpeg_enc_vidioc_g/s_ctrl, jpeg_enc_vidioc_g/s_ctrl). It should be also be easier to test and bisect set of smaller changes when needed. I know it means more work for you, but maybe the exynos4210 regression described in your cover letter could be avoided that way. A general note, please don't remove s5p_ prefix from functions that are not static. jpeg_ sounds a bit to generic prefix for symbols in this single driver. Also please make sure indentation is not broken, it looks like you are using TAB size different than 8 characters. It might be worth testing the driver as a loadable module, it doesn't appear it has been tested, looking at the Makefile modifications. And it doesn't even build currently: drivers/media/platform/s5p-jpeg/jpeg-core: struct platform_device_id is 24 bytes. The last of 3 is: 0x65 0x78 0x79 0x6e 0x6f 0x73 0x34 0x32 0x31 0x32 0x2d 0x6a 0x70 0x65 0x67 0x00 0x00 0x00 0x00 0x00 0x44 0x01 0x00 0x00 FATAL: drivers/media/platform/s5p-jpeg/jpeg-core: struct platform_device_id is not terminated with a NULL entry! make[1]: *** [__modpost] Error 1 When I fix that there are errors due to your incorrect Makefile changes (separate module for each file ??): ERROR: jpeg_get_frame_size [drivers/media/platform/s5p-jpeg/jpeg-dec.ko] undefined! ERROR: jpeg_set_dec_bitstream_size [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_dec_out_fmt [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_dec_scaling [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_frame_buf_address [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_enc_dec_mode [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_encode_hoff_cnt [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_stream_buf_address [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_enc_in_fmt [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_enc_out_fmt [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_stream_size [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_encode_tbl_select [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_enc_tbl [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_huf_table_enable [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_set_interrupt [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_sw_reset [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_get_stream_size [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: jpeg_get_int_status [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: get_jpeg_dec_v4l2_ioctl_ops [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! ERROR: get_jpeg_enc_v4l2_ioctl_ops [drivers/media/platform/s5p-jpeg/jpeg-core.ko] undefined! make[1]: *** [__modpost] Error 1 make: *** [modules] Error 2 Could you please add Andrzej Pietrasiewicz to Cc next time ? He might be busy with other things, nevertheless I wouldn't like to miss any comments/ remarks from his side. Thanks, Sylwester -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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 2/3] [media] s5p-jpeg: Add DT support to JPEG driver
On 05/14/2013 01:53 PM, George Joseph wrote:
From: George Joseph Palathingalgeorge...@samsung.com
Adding DT support to the driver. Driver supports Exynos 4210, 4412 and 5250.
Signed-off-by: George Joseph Palathingalgeorge...@samsung.com
Cc: devicetree-disc...@lists.ozlabs.org
---
drivers/media/platform/s5p-jpeg/jpeg-core.c | 36 +--
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c
b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index f964566..b2bf412 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -970,6 +970,8 @@ const struct jpeg_vb2 jpeg_vb2_dma = {
.plane_addr = vb2_dma_contig_plane_dma_addr,
};
+static void *jpeg_get_drv_data(struct platform_device *pdev);
How about putting definition of the jpeg_get_drv_data() function and
the exynos_jpeg_match array above jpeg_probe() to avoid this forward
declaration ?
static int jpeg_probe(struct platform_device *pdev)
{
struct jpeg_dev *dev;
@@ -982,8 +984,7 @@ static int jpeg_probe(struct platform_device *pdev)
return -ENOMEM;
dev-plat_dev = pdev;
- dev-variant = (struct s5p_jpeg_variant *)
- platform_get_device_id(pdev)-driver_data;
+ dev-variant = (struct s5p_jpeg_variant *)jpeg_get_drv_data(pdev);
Casting is not needed here.
spin_lock_init(dev-slock);
/* setup jpeg control */
@@ -1232,6 +1233,36 @@ static struct platform_device_id jpeg_driver_ids[] = {
MODULE_DEVICE_TABLE(platform, jpeg_driver_ids);
+static const struct of_device_id exynos_jpeg_match[] = {
+ {
+ .compatible = samsung,s5pv210-jpeg,
+ .data =jpeg_drvdata_v1,
+ }, {
+ .compatible = samsung,exynos4212-jpeg,
+ .data =jpeg_drvdata_v2,
+ },
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, exynos_jpeg_match);
+
+static void *jpeg_get_drv_data(struct platform_device *pdev)
+{
+ struct s5p_jpeg_variant *driver_data = NULL;
Unnecessary NULL assignment.
+ if (pdev-dev.of_node) {
+ const struct of_device_id *match;
+ match = of_match_node(of_match_ptr(exynos_jpeg_match),
+pdev-dev.of_node);
+ if (match)
+ driver_data = (struct s5p_jpeg_variant *)match-data;
+ } else {
Indentation is wrong here.
+ driver_data = (struct s5p_jpeg_variant *)
+ platform_get_device_id(pdev)-driver_data;
+ }
+ return driver_data;
+}
How about rewriting this function to something like:
static const void *jpeg_get_drv_data(struct platform_device *pdev)
{
struct device_node *node = pdev-dev.of_node;
const struct of_device_id *match;
if (node) {
match = of_match_node(exynos_jpeg_match, node);
return match ? match-data : NULL;
}
return (void *)platform_get_device_id(pdev)-driver_data;
}
?
static struct platform_driver jpeg_driver = {
.probe = jpeg_probe,
.remove = jpeg_remove,
@@ -1241,6 +1272,7 @@ static struct platform_driver jpeg_driver = {
.driver = {
.owner = THIS_MODULE,
.name = JPEG_NAME,
+ .of_match_table = exynos_jpeg_match,
.pm = NULL,
},
};
Thanks,
Sylwester
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs
Am Donnerstag, 16. Mai 2013, 00:02:40 schrieb Tomasz Figa:
On Wednesday 15 of May 2013 23:48:31 Heiko Stübner wrote:
Am Mittwoch, 15. Mai 2013, 23:20:08 schrieb Sylwester Nawrocki:
On 05/15/2013 10:31 PM, Heiko Stübner wrote:
+ BUG();
Isn't that a bit nasty. This macro should be used with care and
we
should recover if possible. dev_err()?
runtime_config already denies any settings not in the 1,2 or 4bytes
range - the default-part should therefore never be reached. So if
any other value magically appears in the register and triggers the
default-part, something is seriously wrong. So my guess is, the BUG
might be appropriate.
On the other hand the whole default+BUG part could also simply go
away,
for the same reasons.
IMHO BUG() is not needed at all. As Linus suggested dev_err() is such
case or WARN_ON() would be more appropriate. This has been discussed
in the past extensively, not sure if you are aware of the other
Linus' opinion on BUG()/BUG_ON() proliferation:
https://lkml.org/lkml/2012/9/27/461
Very interesting read and I'll keep this in mind in the future. What
about the other option ... i.e. simply getting rid of the whole error
handling, as the other code paths should already make sure that only
valid values get written into the register.
Can the value change in the register somehow on its own without kernel
intervention, or does this not happen?
Hmm, it depends on hardware, I guess. Not sure how it works on this
particular IP.
Still, the mentioned BUG() was about a value in a driver-filled struct,
wasn't it?
/* Quoting the the code for reference */
+static u32 s3c24xx_dma_getbytes_chan(struct s3c24xx_dma_chan *s3cchan)
+{
+ struct s3c24xx_dma_phy *phy = s3cchan-phy;
+ struct s3c24xx_txd *txd = s3cchan-at;
+ u32 tc = readl(phy-base + DSTAT) DSTAT_CURRTC_MASK;
+
+ switch (txd-dcon DCON_DSZ_MASK) {
+ case DCON_DSZ_BYTE:
+ return tc;
+ case DCON_DSZ_HALFWORD:
+ return tc * 2;
+ case DCON_DSZ_WORD:
+ return tc * 4;
+ default:
+ break;
+ }
+
+ BUG();
(Btw. I don't see anything setting the DCON_DSZ bits in this field. Am I
missing something?)
this is for calculating the remaining bytes of the transaction. which is used
in s3c24xx_dma_tx_status.
And when looking at it again, I can't really fathom why I did it this way with
decoding the DSZ from the dcon value of the s3c24xx_txd again instead of
simply using the width value of the same struct
So it can be much simpler as
(...)
u32 tc = readl(phy-base + DSTAT) DSTAT_CURRTC_MASK;
return tc * txd-width;
getting rid of this stuff alltogether
still puzzled how I came up with this strangeness in the first place
Heiko
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs
On Thursday 16 of May 2013 00:45:03 Heiko Stübner wrote:
Am Donnerstag, 16. Mai 2013, 00:02:40 schrieb Tomasz Figa:
On Wednesday 15 of May 2013 23:48:31 Heiko Stübner wrote:
Am Mittwoch, 15. Mai 2013, 23:20:08 schrieb Sylwester Nawrocki:
On 05/15/2013 10:31 PM, Heiko Stübner wrote:
+ BUG();
Isn't that a bit nasty. This macro should be used with care
and
we
should recover if possible. dev_err()?
runtime_config already denies any settings not in the 1,2 or
4bytes
range - the default-part should therefore never be reached. So
if
any other value magically appears in the register and triggers
the
default-part, something is seriously wrong. So my guess is, the
BUG
might be appropriate.
On the other hand the whole default+BUG part could also simply
go
away,
for the same reasons.
IMHO BUG() is not needed at all. As Linus suggested dev_err() is
such
case or WARN_ON() would be more appropriate. This has been
discussed
in the past extensively, not sure if you are aware of the other
Linus' opinion on BUG()/BUG_ON() proliferation:
https://lkml.org/lkml/2012/9/27/461
Very interesting read and I'll keep this in mind in the future. What
about the other option ... i.e. simply getting rid of the whole
error
handling, as the other code paths should already make sure that
only
valid values get written into the register.
Can the value change in the register somehow on its own without
kernel
intervention, or does this not happen?
Hmm, it depends on hardware, I guess. Not sure how it works on this
particular IP.
Still, the mentioned BUG() was about a value in a driver-filled
struct,
wasn't it?
/* Quoting the the code for reference */
+static u32 s3c24xx_dma_getbytes_chan(struct s3c24xx_dma_chan
*s3cchan)
+{
+ struct s3c24xx_dma_phy *phy = s3cchan-phy;
+ struct s3c24xx_txd *txd = s3cchan-at;
+ u32 tc = readl(phy-base + DSTAT) DSTAT_CURRTC_MASK;
+
+ switch (txd-dcon DCON_DSZ_MASK) {
+ case DCON_DSZ_BYTE:
+ return tc;
+ case DCON_DSZ_HALFWORD:
+ return tc * 2;
+ case DCON_DSZ_WORD:
+ return tc * 4;
+ default:
+ break;
+ }
+
+ BUG();
(Btw. I don't see anything setting the DCON_DSZ bits in this field. Am
I missing something?)
this is for calculating the remaining bytes of the transaction. which is
used in s3c24xx_dma_tx_status.
And when looking at it again, I can't really fathom why I did it this
way with decoding the DSZ from the dcon value of the s3c24xx_txd again
instead of simply using the width value of the same struct
So it can be much simpler as
(...)
u32 tc = readl(phy-base + DSTAT) DSTAT_CURRTC_MASK;
return tc * txd-width;
getting rid of this stuff alltogether
still puzzled how I came up with this strangeness in the first place
Hehe, happens.
I'm still yet to review the whole series, but I'm failing to find enough
time. Hopefully will get to do it soon.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
On 05/15/2013 12:29 PM, Linus Walleij wrote:
On Wed, May 15, 2013 at 6:44 PM, Doug Anderson diand...@google.com wrote:
...
Here's how I need to do things when I'm using just an interrupt:
pinctrl@1140 {
cyapa_irq: cyapa-irq {
samsung,pins = gpx1-2;
samsung,pin-function = 0xf;
samsung,pin-pud = 0;
samsung,pin-drv = 0;
};
};
trackpad {
reg = 0x67;
compatible = cypress,cyapa;
interrupts = 2 0;
interrupt-parent = gpx1;
pinctrl-names = default;
pinctrl-0 = cyapa_irq;
wakeup-source;
};
I don't really see much disadvantage here; the interrupt bindings
specify things related to interrupts and the pinctrl bindings specify
thing related to pin configuration.
If you want to condense the DT, I'd suggest using a the pinctrl hogging
feature, i.e. don't put pinctrl-* properties in the trackpad node, but
rather define a system-wide default pinctrl state in the pin
controller node itself. That configuration will be applied as soon as
the pin controller driver is registered. That'd be the same as the
above, with the following added:
pinctrl@1140 {
pinctrl-names = default;
pinctrl-0 = cyapa_irq;
};
except that the pinctrl-0 property would probably end up configuring a
whole bunch of basic pinctrl state rather than just that one interrupt pin.
I prefer to put all the static pinctrl configuration in the pinctrl hog,
and only the dynamic stuff in the individual device nodes.
I know LinusW won't like this suggestion much though:-)
I really wish I could add a 3rd number to the interrupt specifier for
pud and skip the pinctrl bit:
trackpad {
reg = 0x67;
compatible = cypress,cyapa;
interrupts = 2 0 0;
interrupt-parent = gpx1;
wakeup-source;
};
I don't like that myself, since it makes the interrupt binding (and I
assume you'd want to go back to the similar misuse in the GPIO binding)
also configure pinctrl-related stuff.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
Tomasz / Linus, On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa tomasz.f...@gmail.com wrote: Yes. I don't like the current way too much either, duplication being one of the reasons. Do you have any other ideas? It sounds like Linus didn't like my suggestion and makes some good points... I don't have any other great ideas other than having an argument about whether the concept of pulls should be added to the GPIO subsystem (and backed by pinmux). Then we could make an argument that default pull state belonged in the GPIO specifier. OK, maybe we should just pretend that I didn't bring that up. ;) ...but I'm definitely interested in other ideas to eliminate the duplication. Until then I'm planning on submitting what I have locally. I'll probably send some version of it upstream fairly soon. I will probably submit without trying to get all the preprocessor symbols names done and will understand if Linus NAKs because of that. I don't have time to take that on at the moment but can always come back and rework that patch later. ;) -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
Stephen,
On Wed, May 15, 2013 at 4:51 PM, Stephen Warren swar...@wwwdotorg.org wrote:
I don't really see much disadvantage here; the interrupt bindings
specify things related to interrupts and the pinctrl bindings specify
thing related to pin configuration.
OK. If this is the best way then I can accept that and maybe we
should just drop this thread. What do people think? It means less
work for me in the short term since I've already got it implemented
that way and then I don't need to submit any patches to try to change
things! ;)
If you want to condense the DT, I'd suggest using a the pinctrl hogging
feature, i.e. don't put pinctrl-* properties in the trackpad node, but
rather define a system-wide default pinctrl state in the pin
controller node itself. That configuration will be applied as soon as
the pin controller driver is registered. That'd be the same as the
above, with the following added:
pinctrl@1140 {
pinctrl-names = default;
pinctrl-0 = cyapa_irq;
};
except that the pinctrl-0 property would probably end up configuring a
whole bunch of basic pinctrl state rather than just that one interrupt pin.
I prefer to put all the static pinctrl configuration in the pinctrl hog,
and only the dynamic stuff in the individual device nodes.
I know LinusW won't like this suggestion much though:-)
Ah right! I forgot about hogs in this case. That's also reasonable
as a solution and is similar to what we've got in the tree for
powerdown configuration of pins (I'll try to post this patch soon too,
WIP at https://gerrit.chromium.org/gerrit/#/c/51292/ and
https://gerrit.chromium.org/gerrit/#/c/51372/.
It sounds like there's a bit of disagreement about the best way so
I'll probably keep the way I have. ...but I'll keep hogs in my back
pocket.
I don't like that myself, since it makes the interrupt binding (and I
assume you'd want to go back to the similar misuse in the GPIO binding)
also configure pinctrl-related stuff.
Fair enough. :)
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote: Tomasz / Linus, On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa tomasz.f...@gmail.com wrote: Yes. I don't like the current way too much either, duplication being one of the reasons. Do you have any other ideas? It sounds like Linus didn't like my suggestion and makes some good points... I don't have anything interesting at the moment. It's a bit late now here (2 AM), so I'm going to get some sleep first. Also after reading Stephen's reply, I'm wondering if hogging wouldn't solve the problem indeed. (It might have to be fixed on pinctrl-samsung first, as last time I tried to use it, it caused some errors from pinctrl core, but haven't time to track them down, as it wasn't anything important at that time). Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
On Wednesday 15 of May 2013 17:03:44 Doug Anderson wrote:
Stephen,
On Wed, May 15, 2013 at 4:51 PM, Stephen Warren swar...@wwwdotorg.org
wrote:
I don't really see much disadvantage here; the interrupt bindings
specify things related to interrupts and the pinctrl bindings specify
thing related to pin configuration.
OK. If this is the best way then I can accept that and maybe we
should just drop this thread. What do people think? It means less
work for me in the short term since I've already got it implemented
that way and then I don't need to submit any patches to try to change
things! ;)
If you want to condense the DT, I'd suggest using a the pinctrl
hogging
feature, i.e. don't put pinctrl-* properties in the trackpad node, but
rather define a system-wide default pinctrl state in the pin
controller node itself. That configuration will be applied as soon as
the pin controller driver is registered. That'd be the same as the
above, with the following added:
pinctrl@1140 {
pinctrl-names = default;
pinctrl-0 = cyapa_irq;
};
except that the pinctrl-0 property would probably end up configuring a
whole bunch of basic pinctrl state rather than just that one interrupt
pin.
I prefer to put all the static pinctrl configuration in the pinctrl
hog, and only the dynamic stuff in the individual device nodes.
I know LinusW won't like this suggestion much though:-)
Ah right! I forgot about hogs in this case. That's also reasonable
as a solution and is similar to what we've got in the tree for
powerdown configuration of pins (I'll try to post this patch soon too,
WIP at https://gerrit.chromium.org/gerrit/#/c/51292/ and
https://gerrit.chromium.org/gerrit/#/c/51372/.
Hmm, last thing before I fell asleep: We already have support for power
down configuration in pinctrl-samsung. See samsung,pin-conpdn and
samsung,pin-pudpdn.
Also I already have patches for suspend/resume support of pinctrl-samsung
and pinctrl-exynos, as well as configuration of wake-up sources. I'm going
to post them soon.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
On 05/15/2013 06:13 PM, Tomasz Figa wrote:
On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote:
Tomasz / Linus,
On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa tomasz.f...@gmail.com
wrote:
Yes. I don't like the current way too much either, duplication being
one of the reasons.
Do you have any other ideas? It sounds like Linus didn't like my
suggestion and makes some good points...
I don't have anything interesting at the moment. It's a bit late now here
(2 AM), so I'm going to get some sleep first.
Also after reading Stephen's reply, I'm wondering if hogging wouldn't
solve the problem indeed. (It might have to be fixed on pinctrl-samsung
first, as last time I tried to use it, it caused some errors from pinctrl
core, but haven't time to track them down, as it wasn't anything important
at that time).
One issue I noticed with the DT fragments earlier in this thread. It
looks like hogs in the Samsung pinctrl bingings end up looking like:
pinctrl {
pina {
samsung,pins = PIN_A PIN_B PIN_C;
samsung,pin-function = 0xf;
samsung,pin-pud = 0;
...
};
pinp {
samsung,pins = PIN_P PIN_Q;
samsung,pin-function = 0xe;
samsung,pin-pud = 1;
...
};
pinx {
samsung,pins = PIN_X PIN_Y PIN_Z;
samsung,pin-function = 0xd;
samsung,pin-pud = 2;
...
};
pinctrl-names = default;
pinctrl-0 = pina pinp pinx;
};
That pinctrl-0 property could get rather large (hard to write/maintain,
unwieldy) if it needs to set up lots of different configurations. That's
why I made the equivalent Tegra bindings be:
pinctrl {
pins_default {
pina {
samsung,pins = PIN_A PIN_B PIN_C;
samsung,pin-function = 0xf;
samsung,pin-pud = 0;
...
};
pinp {
samsung,pins = PIN_P PIN_Q;
samsung,pin-function = 0xe;
samsung,pin-pud = 1;
...
};
pinx {
samsung,pins = PIN_X PIN_Y PIN_Z;
samsung,pin-function = 0xd;
samsung,pin-pud = 2;
...
};
};
pinctrl-names = default;
pinctrl-0 = pins_default;
};
The extra level within the pinctrl configuration node (pins_default
here) makes the pinctrl-0 property a lot easier to write, and the
advantage happens at every use-site that needs to configure different
subsets of the relevant pins in different ways.
If you're changing all the bindings anyway, introducing this extra level
might be something to think about.
I did try to explain my philosophy here when we all got together to
design the pinctrl bindings, but I obviously didn't explain it well
enough, or people didn't like it anyway.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
Tomasz, On Wed, May 15, 2013 at 5:13 PM, Tomasz Figa tomasz.f...@gmail.com wrote: I don't have anything interesting at the moment. It's a bit late now here (2 AM), so I'm going to get some sleep first. Sorry for keeping you up. Sleep is good! Also after reading Stephen's reply, I'm wondering if hogging wouldn't solve the problem indeed. (It might have to be fixed on pinctrl-samsung first, as last time I tried to use it, it caused some errors from pinctrl core, but haven't time to track them down, as it wasn't anything important at that time). I will give it a shot tomorrow morning and see how it looks. I'll also evaluate Stephen's suggestions then once I've see how it looks with the current bindings... -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pulls and drive strengths in the pinctrl world
Tomasz, On Wed, May 15, 2013 at 5:19 PM, Tomasz Figa tomasz.f...@gmail.com wrote: Hmm, last thing before I fell asleep: We already have support for power down configuration in pinctrl-samsung. See samsung,pin-conpdn and samsung,pin-pudpdn. Dang it! OK, we'll work on using those. Also I already have patches for suspend/resume support of pinctrl-samsung and pinctrl-exynos, as well as configuration of wake-up sources. I'm going to post them soon. Huh, I wonder if they look just like the one we've been working on: * https://gerrit.chromium.org/gerrit/#/c/51336/ * https://gerrit.chromium.org/gerrit/#/c/51342/ Those are about ready for upstream, too. I was going to send them this morning when I found out that we were missing a bunch of pinctrl patches and had to rework then. :( Anyway, we can compare our solutions and figure out which is better. ;) I'm happy with anything that works! -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rtc: max8998: Check for pdata presence before dereferencing
Thursday, May 16, 2013 12:16 AM, Tomasz Figa wrote:
Currently the driver can crash with a NULL pointer dereference if no pdata
is provided, despite of successful registration of MFD part. This patch
fixes the problem by adding a NULL check before dereferencing the pdata
pointer.
Signed-off-by: Tomasz Figa t.f...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC'ed Andrew Morton
Reviewed-by: Jingoo Han jg1@samsung.com
Best regards,
Jingoo Han
---
drivers/rtc/rtc-max8998.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-max8998.c b/drivers/rtc/rtc-max8998.c
index 48b6612..d5af7ba 100644
--- a/drivers/rtc/rtc-max8998.c
+++ b/drivers/rtc/rtc-max8998.c
@@ -285,7 +285,7 @@ static int max8998_rtc_probe(struct platform_device *pdev)
info-irq, ret);
dev_info(pdev-dev, RTC CHIP NAME: %s\n, pdev-id_entry-name);
- if (pdata-rtc_delay) {
+ if (pdata pdata-rtc_delay) {
info-lp3974_bug_workaround = true;
dev_warn(pdev-dev, LP3974 with RTC REGERR option.
RTC updates will be extremely slow.\n);
--
1.8.2.1
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
N떑꿩�r툤y鉉싕b쾊Ф푤v�^�)頻{.n�+돴쪐{굇ß틏,″㎍썳變}찠꼿쟺�j:+v돣�쳭喩zZ+�+zf＂톒쉱�~넮녬i鎬z� 췿ⅱ�?솳鈺��)刪f
Re: [PATCH 1/2] video: exynos_dp: Add parsing of gpios pins to exynos-dp driver
Tuesday, May 14, 2013 11:17 PM, Vikas Sajjan wrote: Hi Vikas, On Tuesday 14 of May 2013 18:25:51 Vikas Sajjan wrote: Adds GPIO parsing functionality for LCD backlight and LCD enable GPIO pins of exynos dp controller. Signed-off-by: Vikas Sajjan vikas.saj...@linaro.org --- drivers/video/exynos/exynos_dp_core.c | 45 + 1 file changed, 45 insertions(+) I don't think that Exynos DP driver is right place for such code. Backlight and LCD drivers are responsible for backlight and LCD power control using backlight and LCD subsystems. IMHO the correct solution would be to either extend existing backlight/lcd drivers found in drivers/video/backlight to support direct GPIO control and parse GPIO pins from device tree or create new gpio_bl and gpio_lcd drivers. Hi Vikas Sajian, I agree with Tomasz Figa's opinion. Backlight/LCD framework should be used. eDP panel backlight on SMDK5210 board can be controlled by PWM; thus, pwm-backlight driver should be used. Also, eDP panel reset pin should be controlled by using platform-lcd driver. CCing Richard, Florian and linux-fbdev. Also, I have been doing backlight reviews instead of Richard, please do CC'ing me. Best regards, Jingoo Han Best regards, Tomasz
Re: [RFC 2/4] dma: add dmaengine driver for Samsung s3c24xx SoCs
On Tuesday, May 14, 2013 11:22 PM Tomasz Figa wrote: Hi Linus, Heiko, On Tuesday 14 of May 2013 14:47:19 Linus Walleij wrote: On Sat, May 11, 2013 at 1:31 PM, Heiko St?bner he...@sntech.de wrote: Conceptually the s3c24xx-dma feels like a distant relative of the pl08x with numerous virtual channels being mapped to a lot less physical ones. The driver therefore borrows a lot from the amba-pl08x driver in this regard. Functionality-wise the driver gains a memcpy ability in addition to the slave_sg one. The driver currently only supports the newer SoCs which can use any physical channel for any dma slave. Support for the older SoCs where each channel only supports a subset of possible dma slaves will have to be added later. Tested on a s3c2416-based board, memcpy using the dmatest module and slave_sg partially using the spi-s3c64xx driver. Signed-off-by: Heiko Stuebner he...@sntech.de So have I understood correctly if I assume that *some* S3C variants, i.e. this: arch/arm/mach-s3c64xx/dma.c have a vanilla, unmodified, or just slightly modified PL08x block, while this DMAC is something probably based on the PL08x where some ASIC engineer has had a good time hacking around in the VHDL code to meet some feature requirements. Correct? Or plausible guess? Exactly *how* far away from the pl08x hardware is it? AFAIK the DMAC of S3C24xx is completely different from PL08x. I think Heiko just meant that it uses similar concepts, like virtual channels. Yes, right. the DMAC of S3C24xx is completely different from PL08x. As Heiko mentioned, the DMAC of S3C24xx is 'home grown' as other IPs of S3C24xx. Best regards, Jingoo Han
