Re: [PATCH] ARM: EXYNOS: Add MFC device tree support
On 08/16/2012 08:42 PM, Thomas Abraham wrote:
> On 16 August 2012 18:01, Arun Kumar K wrote:
>> + - interrupts : MFC interupt number to the CPU.
>> +
>> + - samsung,mfc-r : Base address of the first memory bank used by MFC
>> + for DMA contiguous memory allocation.
>> +
>> + - samsung,mfc-r-size : Size of the first memory bank.
>
> It is not allowed to pass buffer base address and size from device
> tree. Device tree node should describe only the MFC controller
> hardware. Any memory management related information should be handled
> outside of device tree. This helps the bindings to be reusable across
> multiple operating systems.
The question is where elsewhere this should be described as this is strictly
board-dependent option (number and size of RAM banks are important here).
I agree that base addresses are bad, but I'm not aware of any functionality
that would allow driver (actually, its platform dependent part in
exynosN_reserve() function) to enumerate available memory banks and grab
memory chunks from two distinct banks.
My (lack of) knowledge ARM might be to blame here but I simply don't know
how to achieve this. Any suggestions?
On 08/16/2012 09:31 PM, Arun Kumar K wrote:
> +static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
> + phys_addr_t lbase, unsigned int lsize) {
> +
> + if (memblock_remove(lbase, lsize)) {
> + pr_err("Failed to reserve bank1 memory for MFC device\n");
> + WARN_ON(1);
> + }
> +
> + if (memblock_remove(rbase, rsize)) {
> + pr_err("Failed to reserve bank2 memory for MFC device\n");
> + WARN_ON(1);
> + }
> +}
non-static function with the same name is already defined in
arch/arm/plat-samsung/s5p-dev-mfc.c. Please don't duplicate it,
especially that you seem to be trying to do that twice!
> diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c
> b/arch/arm/mach-exynos/mach-exynos5-dt.c
> index ef770bc..898d2de 100644
> --- a/arch/arm/mach-exynos/mach-exynos5-dt.c
> +++ b/arch/arm/mach-exynos/mach-exynos5-dt.c
...
> +static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
> + phys_addr_t lbase, unsigned int lsize) {
> +
> + if (memblock_remove(lbase, lsize)) {
> + pr_err("Failed to reserve bank1 memory for MFC device\n");
> + WARN_ON(1);
> + }
> +
> + if (memblock_remove(rbase, rsize)) {
> + pr_err("Failed to reserve bank2 memory for MFC device\n");
> + WARN_ON(1);
> + }
> +}
See above.
> +
> +static void __init exynos5_reserve(void)
> +{
> + s5p_mfc_reserve_mem(0x4300, 8 << 20, 0x5100, 8 << 20);
I think it would make sense to make this memory reservation dependent
on "mfc*" node being present in DTS. It's to early to use of_* functions
(because tree is not populated at this stage) but fdt_* family of functions
work just fine.
Regards,
--
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform
--
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 v6 1/6] thermal: add generic cpufreq cooling implementation
On δΊ”, 2012-08-17 at 11:56 +0300, Valentin, Eduardo wrote: > Hello, > >>> + > >>> + > >>> +1.2 CPU cooling action notifier register/unregister interface > >>> +1.2.1 int cputherm_register_notifier(struct notifier_block *nb, > >>> + unsigned int list) > >>> + > >>> +This interface registers a driver with cpu cooling layer. The driver > >>> will > >>> +be notified when any cpu cooling action is called. > >>> + > >>> +nb: notifier function to register > >>> +list: CPUFREQ_COOLING_START or CPUFREQ_COOLING_STOP > >>> + > >>> +1.2.2 int cputherm_unregister_notifier(struct notifier_block *nb, > >>> + unsigned int list) > >>> + > >>> +This interface registers a driver with cpu cooling layer. The driver > >>> will > >>> +be notified when any cpu cooling action is called. > >>> + > >>> +nb: notifier function to register > >>> +list: CPUFREQ_COOLING_START or CPUFREQ_COOLING_STOP > >> > >> what are these two APIs used for? > >> I did not see they are used in your patch set, do I miss something? > > No currently they are not used by my patches. I added them on request > > from Eduardo and others > > Yeah, this was a suggestion as we didn't really know how the FW part > would evolve by that time. > > The reasoning is to allow any interested user (in kernel) to be > notified when max frequency changes. in this case, the cooling device should be updated. Say all the target of the thermal_instances of a cooling devices is 0, which means they want the cpu to run at maximum frequency, when the max frequency changes, we should set the processor to the new max frequency as well. Using notification is not a good idea as user can not handle this without interacting with the thermal framework. IMO, we should introduce a new API to handle this, rather than just notifications to users. > Actually, the use case behind > this is to allow such users to perform some handshaking or stop their > activities or even change some paramenters, in case the max frequency > would change. It is the cooling device driver that notice this change first, and it should invoke something like thermal_cooling_device_update/rebind() to tell the thermal framework this change. > Ideally it would be possible to nack the cooling > transition. But that is yet a wider discussion. So far we don't have > users for this. > yep. I thought about this before, but I'd prefer to do this when there is a real user. Or else, we are kind of over-designing here. how about removing this piece of code for now? thanks, rui > >> > >>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > >>> index 7dd8c34..996003b 100644 > >>> --- a/drivers/thermal/Kconfig > >>> +++ b/drivers/thermal/Kconfig > >>> @@ -19,6 +19,17 @@ config THERMAL_HWMON > >>> depends on HWMON=y || HWMON=THERMAL > >>> default y > >>> > >>> +config CPU_THERMAL > >>> + bool "generic cpu cooling support" > >>> + depends on THERMAL && CPU_FREQ > >>> + help > >>> + This implements the generic cpu cooling mechanism through > >>> frequency > >>> + reduction, cpu hotplug and any other ways of reducing > >>> temperature. An > >>> + ACPI version of this already > >>> exists(drivers/acpi/processor_thermal.c). > >>> + This will be useful for platforms using the generic thermal > >>> interface > >>> + and not the ACPI interface. > >>> + If you want this support, you should say Y here. > >>> + > >>> config SPEAR_THERMAL > >>> bool "SPEAr thermal sensor driver" > >>> depends on THERMAL > >>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile > >>> index fd9369a..aae59ad 100644 > >>> --- a/drivers/thermal/Makefile > >>> +++ b/drivers/thermal/Makefile > >>> @@ -3,5 +3,6 @@ > >>> # > >>> > >>> obj-$(CONFIG_THERMAL)+= thermal_sys.o > >>> +obj-$(CONFIG_CPU_THERMAL)+= cpu_cooling.o > >>> obj-$(CONFIG_SPEAR_THERMAL) += spear_thermal.o > >>> obj-$(CONFIG_RCAR_THERMAL) += rcar_thermal.o > >>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > >>> new file mode 100644 > >>> index 000..c42e557 > >>> --- /dev/null > >>> +++ b/drivers/thermal/cpu_cooling.c > >>> @@ -0,0 +1,512 @@ > >>> +/* > >>> + * linux/drivers/thermal/cpu_cooling.c > >>> + * > >>> + * Copyright (C) 2012 Samsung Electronics Co., > >>> Ltd(http://www.samsung.com) > >>> + * Copyright (C) 2012 Amit Daniel > >>> + * > >>> + * > >>> ~~ > >>> + * This program is free software; you can redistribute it and/or modify > >>> + * it under the terms of the GNU General Public License as published by > >>> + * the Free Software Foundation; version 2 of the License. > >>> + * > >>> + * This program is distributed in the hope that it will be useful, but > >>> + * WITHOUT ANY WARRANTY; without even the implied warranty of > >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR
Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation
Hi Laurent and Sylwester, My apologies for the late answer. On Fri, Jul 27, 2012 at 12:50:13AM +0200, Laurent Pinchart wrote: > Hi Sylwester, > > On Thursday 26 July 2012 22:39:31 Sylwester Nawrocki wrote: > > On 07/26/2012 05:21 PM, Laurent Pinchart wrote: > > > On Friday 25 May 2012 21:52:48 Sylwester Nawrocki wrote: > > >> The driver initializes all board related properties except the s_power() > > >> callback to board code. The platforms that require this callback are not > > >> supported by this driver yet for CONFIG_OF=y. > > >> > > >> Signed-off-by: Sylwester Nawrocki > > >> Signed-off-by: Bartlomiej Zolnierkiewicz > > >> Signed-off-by: Kyungmin Park > > >> --- > > >> > > >> .../bindings/camera/samsung-s5k6aafx.txt | 57 + > > >> drivers/media/video/s5k6aa.c | 129 + > > >> 2 files changed, 146 insertions(+), 40 deletions(-) > > >> create mode 100644 > > >> > > >> Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt > > >> > > >> diff --git > > >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file > > >> mode 100644 > > >> index 000..6685a9c > > >> --- /dev/null > > >> +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt > > >> @@ -0,0 +1,57 @@ > > >> +Samsung S5K6AAFX camera sensor > > >> +-- > > >> + > > >> +Required properties: > > >> + > > >> +- compatible : "samsung,s5k6aafx"; > > >> +- reg : base address of the device on I2C bus; > > >> +- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601; > > >> +- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V); > > >> +- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V); > > >> +- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to > > >> 1.9V) + or 2.8V (2.6V to 3.0); > > >> +- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V) > > >> + or 2.8V (2.5V to 3.1V); > > >> + > > >> +Optional properties: > > >> + > > >> +- clock-frequency : the IP's main (system bus) clock frequency in Hz, > > >> the default > > > > > > Is that the input clock frequency ? Can't it vary ? Instead of accessing > > > the > > Yes, the description is incorrect in this patch, it should read: > > > > +- clock-frequency : the sensor's master clock frequency in Hz; > > > > and be a required property. As in this patch: > > https://github.com/snawrocki/linux/commit/e8a5f890dec0d7414b656bb1d1ac97d5e7 > > abe563 > > > > It could vary (as this is a PLL input frequency), so probably a range would > > be a better alternative. Given that host device won't always be able to set > > this exact value... > > A range sounds good, or perhaps a list of ranges. Sakari, what would you need > for the SMIA++ driver ? Typically the sensor's external clock is derived from another clock using a divisor. This means there's usually quite limited selection of possible clock frequencies since the sensors usually have a small range of possible frequencies such as 6 -- 27 MHz or even less. Also it's important to choose the frequency in such a way that it doesn't interfere with other devices in the system. The frequency also must be picked so that one can achieve the desired highest data transfer rate. The sensors are also quite flexible in their internal clock tree configuration, but in the situation where the desired data rate is close to sensor limits there may be additional constraints. Still it's always possible to come up with a best value for the board while other values are inferior. For these reasons I see little point in providing anything else than just a single value for the external clock frequency. This value is board-specific. > > > sensor clock frequency from the FIMC driver you should reference a clock > > > in the sensor DT node. That obviously requires generic clock support, > > > which might not be available for your platform yet (that's one of the > > > reasons the OMAP3 ISP driver doesn't support DT yet). > > > > I agree it might be better, but waiting unknown number of kernel releases > > for the platforms to get converted to common clock API is not a good > > alternative either. I guess we could have some transitional solutions while > > other subsystems are getting adapted. > > I agree, we need an interim solution. The SMIA++ driver allows the platform data to have either the clock name or a function pointer to set the clock frequency. If the clock name is there it'll be used instead. The function pointer can be removed once it's no longer needed. Kind regards, -- Sakari Ailus e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk -- 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 14/14] drivers/spi/spi-s3c24xx.c: fix error return code
From: Julia Lawall
Initialize return variable before exiting on an error path.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
//
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
//
Signed-off-by: Julia Lawall
---
Perhaps -EINVAL is not the right value in this case.
drivers/spi/spi-s3c24xx.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/spi/spi-s3c24xx.c b/drivers/spi/spi-s3c24xx.c
index 8ee7d79..a2a080b 100644
--- a/drivers/spi/spi-s3c24xx.c
+++ b/drivers/spi/spi-s3c24xx.c
@@ -611,6 +611,7 @@ static int __devinit s3c24xx_spi_probe(struct
platform_device *pdev)
if (!pdata->set_cs) {
if (pdata->pin_cs < 0) {
dev_err(&pdev->dev, "No chipselect pin\n");
+ err = -EINVAL;
goto err_register;
}
--
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
