Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
Hi Darius,
On 19 March 2012 05:16, Jingoo Han wrote:
[...]
>
> Because there are not multiple windows and driver sees only single window as
> you mentiond,
> Only 'struct s3c_fb_pd_win mini6410_fb_win[0]' should be used and
> 'struct s3c_fb_pd_win mini6410_fb_win[1]' should not be used.
> s3c_fb_pd_win[0] means window 0 of FIMD IP, s3c_fb_pd_win[1] means window 1
> of FIMD IP.
>
> Therefore, if mini6410_fb_win[0] and mini6410_fb_win[1] are included, it
> means that
> 2 windows of FIMD IP are used in single LCD.
>
> If you want to support multiple LCDs, you should use struct
> s3c_fb_platdata[0], struct s3c_fb_platdata[1],
> because 'struct s3c_fb_platdata' means LCD panel.
>
> If you understand the purpose of s3c_fb_platdata and s3c_fb_pd_win, you
> should do, at least, as below.
>
> static struct s3c_fb_pd_win mini6410_fb_lcd0_win[] = {
> {
> .win_mode = { /* 4.3" 480x272 */
> .left_margin = 3,
> .right_margin = 2,
> .upper_margin = 1,
> .lower_margin = 1,
> .hsync_len = 40,
> .vsync_len = 1,
> .xres = 480,
> .yres = 272,
> },
> .max_bpp = 32,
> .default_bpp = 16,
> },
> };
>
> static struct s3c_fb_platdata mini6410_lcd_pdata[0] __initdata = {
> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
> .win[0] = &mini6410_fb_lcd0_win[0],
> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
> };
>
> static struct s3c_fb_pd_win mini6410_fb_lcd1_win[] = {
> {
> .win_mode = { /* 7.0" 800x480 */
> .left_margin = 8,
> .right_margin = 13,
> .upper_margin = 7,
> .lower_margin = 5,
> .hsync_len = 3,
> .vsync_len = 1,
> .xres = 800,
> .yres = 480,
> },
> .max_bpp = 32,
> .default_bpp = 16,
> },
> };
>
> static struct s3c_fb_platdata mini6410_lcd_pdata[1] __initdata = {
> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
> .win[0] = &mini6410_fb_lcd1_win[0],
> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
> };
>
> Each struct means:
> mini6410_lcd_pdata[0]: 4.3" 480x272 LCD
> mini6410_fb_lcd0_win[0]: window 0 used for 4.3" 480x272 LCD
>
> mini6410_lcd_pdata[1]: 7.0" 800x480 LCD
> mini6410_fb_lcd1_win[0]: window 0 used for 7.0" 800x480 LCD
>
> In this case, 4.3" or 7.0" LCD can be selected using mini6410_lcd_pdata[],
> instead of selecting mini6410_fb_win[0] or
> mini6410_fb_win[1].
>
> My point is that 's3c_fb_platdata and s3c_fb_pd_win should be used exactly'.
I agree with solution which Jingoo Han has proposed. Instead of
selecting one instance of s3c_fb_pd_win over the other at runtime, the
features->lcd_index can be used to select one of the instances of
platform data for s3c-fb driver.
s3c_fb_pd_win is used to describe a window supported by the fimd
controller. It is not meant to carry lcd timing information. The lcd
panel timing remains the same irrespective of the window sizes. And
this patchset is a step towards untangling window information and
video timing.
(Apologizes for the delayed reply. I was out of office for last 4 days.)
Thanks,
Thomas.
--
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 v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
Hi,
On Mon, Mar 19, 2012 at 9:29 AM, Thomas Abraham
wrote:
> Hi Darius,
>
> On 19 March 2012 05:16, Jingoo Han wrote:
> [...]
>
>>
>> Because there are not multiple windows and driver sees only single window as
>> you mentiond,
>> Only 'struct s3c_fb_pd_win mini6410_fb_win[0]' should be used and
>> 'struct s3c_fb_pd_win mini6410_fb_win[1]' should not be used.
>> s3c_fb_pd_win[0] means window 0 of FIMD IP, s3c_fb_pd_win[1] means window 1
>> of FIMD IP.
>>
>> Therefore, if mini6410_fb_win[0] and mini6410_fb_win[1] are included, it
>> means that
>> 2 windows of FIMD IP are used in single LCD.
>>
>> If you want to support multiple LCDs, you should use struct
>> s3c_fb_platdata[0], struct s3c_fb_platdata[1],
>> because 'struct s3c_fb_platdata' means LCD panel.
>>
>> If you understand the purpose of s3c_fb_platdata and s3c_fb_pd_win, you
>> should do, at least, as below.
>>
>> static struct s3c_fb_pd_win mini6410_fb_lcd0_win[] = {
>> {
>> .win_mode = { /* 4.3" 480x272 */
>> .left_margin = 3,
>> .right_margin = 2,
>> .upper_margin = 1,
>> .lower_margin = 1,
>> .hsync_len = 40,
>> .vsync_len = 1,
>> .xres = 480,
>> .yres = 272,
>> },
>> .max_bpp = 32,
>> .default_bpp = 16,
>> },
>> };
>>
>> static struct s3c_fb_platdata mini6410_lcd_pdata[0] __initdata = {
>> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
>> .win[0] = &mini6410_fb_lcd0_win[0],
>> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
>> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
>> };
>>
>> static struct s3c_fb_pd_win mini6410_fb_lcd1_win[] = {
>> {
>> .win_mode = { /* 7.0" 800x480 */
>> .left_margin = 8,
>> .right_margin = 13,
>> .upper_margin = 7,
>> .lower_margin = 5,
>> .hsync_len = 3,
>> .vsync_len = 1,
>> .xres = 800,
>> .yres = 480,
>> },
>> .max_bpp = 32,
>> .default_bpp = 16,
>> },
>> };
>>
>> static struct s3c_fb_platdata mini6410_lcd_pdata[1] __initdata = {
>> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
>> .win[0] = &mini6410_fb_lcd1_win[0],
>> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
>> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
>> };
>>
>> Each struct means:
>> mini6410_lcd_pdata[0]: 4.3" 480x272 LCD
>> mini6410_fb_lcd0_win[0]: window 0 used for 4.3" 480x272 LCD
>>
>> mini6410_lcd_pdata[1]: 7.0" 800x480 LCD
>> mini6410_fb_lcd1_win[0]: window 0 used for 7.0" 800x480 LCD
>>
>> In this case, 4.3" or 7.0" LCD can be selected using mini6410_lcd_pdata[],
>> instead of selecting mini6410_fb_win[0] or
>> mini6410_fb_win[1].
>>
>> My point is that 's3c_fb_platdata and s3c_fb_pd_win should be used exactly'.
>
> I agree with solution which Jingoo Han has proposed. Instead of
> selecting one instance of s3c_fb_pd_win over the other at runtime, the
> features->lcd_index can be used to select one of the instances of
> platform data for s3c-fb driver.
I agree with it too. Would you make such patch based on your s3c-fb patch set?
Somebody should make it and definitely not to remove multiple LCD
suport for these boards.
>
> s3c_fb_pd_win is used to describe a window supported by the fimd
> controller. It is not meant to carry lcd timing information. The lcd
> panel timing remains the same irrespective of the window sizes. And
> this patchset is a step towards untangling window information and
> video timing.
Right now s3c_fb_pd_win contains exactly LCD timing values.
Your patch will move these timing values to struct fb_videomode.
I could make necessary patch for that too, but I think you will spend
less time doing it by yourself than syncing with me.
What do you think?
regards,
Darius
--
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 v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
On 19 March 2012 13:10, Darius Augulis wrote:
> Hi,
>
> On Mon, Mar 19, 2012 at 9:29 AM, Thomas Abraham
> wrote:
>> Hi Darius,
>>
>> On 19 March 2012 05:16, Jingoo Han wrote:
>> [...]
>>
>>>
>>> Because there are not multiple windows and driver sees only single window
>>> as you mentiond,
>>> Only 'struct s3c_fb_pd_win mini6410_fb_win[0]' should be used and
>>> 'struct s3c_fb_pd_win mini6410_fb_win[1]' should not be used.
>>> s3c_fb_pd_win[0] means window 0 of FIMD IP, s3c_fb_pd_win[1] means window 1
>>> of FIMD IP.
>>>
>>> Therefore, if mini6410_fb_win[0] and mini6410_fb_win[1] are included, it
>>> means that
>>> 2 windows of FIMD IP are used in single LCD.
>>>
>>> If you want to support multiple LCDs, you should use struct
>>> s3c_fb_platdata[0], struct s3c_fb_platdata[1],
>>> because 'struct s3c_fb_platdata' means LCD panel.
>>>
>>> If you understand the purpose of s3c_fb_platdata and s3c_fb_pd_win, you
>>> should do, at least, as below.
>>>
>>> static struct s3c_fb_pd_win mini6410_fb_lcd0_win[] = {
>>> {
>>> .win_mode = { /* 4.3" 480x272 */
>>> .left_margin = 3,
>>> .right_margin = 2,
>>> .upper_margin = 1,
>>> .lower_margin = 1,
>>> .hsync_len = 40,
>>> .vsync_len = 1,
>>> .xres = 480,
>>> .yres = 272,
>>> },
>>> .max_bpp = 32,
>>> .default_bpp = 16,
>>> },
>>> };
>>>
>>> static struct s3c_fb_platdata mini6410_lcd_pdata[0] __initdata = {
>>> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
>>> .win[0] = &mini6410_fb_lcd0_win[0],
>>> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
>>> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
>>> };
>>>
>>> static struct s3c_fb_pd_win mini6410_fb_lcd1_win[] = {
>>> {
>>> .win_mode = { /* 7.0" 800x480 */
>>> .left_margin = 8,
>>> .right_margin = 13,
>>> .upper_margin = 7,
>>> .lower_margin = 5,
>>> .hsync_len = 3,
>>> .vsync_len = 1,
>>> .xres = 800,
>>> .yres = 480,
>>> },
>>> .max_bpp = 32,
>>> .default_bpp = 16,
>>> },
>>> };
>>>
>>> static struct s3c_fb_platdata mini6410_lcd_pdata[1] __initdata = {
>>> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
>>> .win[0] = &mini6410_fb_lcd1_win[0],
>>> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
>>> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
>>> };
>>>
>>> Each struct means:
>>> mini6410_lcd_pdata[0]: 4.3" 480x272 LCD
>>> mini6410_fb_lcd0_win[0]: window 0 used for 4.3" 480x272 LCD
>>>
>>> mini6410_lcd_pdata[1]: 7.0" 800x480 LCD
>>> mini6410_fb_lcd1_win[0]: window 0 used for 7.0" 800x480 LCD
>>>
>>> In this case, 4.3" or 7.0" LCD can be selected using mini6410_lcd_pdata[],
>>> instead of selecting mini6410_fb_win[0] or
>>> mini6410_fb_win[1].
>>>
>>> My point is that 's3c_fb_platdata and s3c_fb_pd_win should be used exactly'.
>>
>> I agree with solution which Jingoo Han has proposed. Instead of
>> selecting one instance of s3c_fb_pd_win over the other at runtime, the
>> features->lcd_index can be used to select one of the instances of
>> platform data for s3c-fb driver.
>
> I agree with it too. Would you make such patch based on your s3c-fb patch set?
> Somebody should make it and definitely not to remove multiple LCD
> suport for these boards.
Sure. I will make a patch for that. I did not notice the way two lcd's
were supported in your code while preparing this patchset. Thanks for
bringing up this point.
>
>>
>> s3c_fb_pd_win is used to describe a window supported by the fimd
>> controller. It is not meant to carry lcd timing information. The lcd
>> panel timing remains the same irrespective of the window sizes. And
>> this patchset is a step towards untangling window information and
>> video timing.
>
> Right now s3c_fb_pd_win contains exactly LCD timing values.
> Your patch will move these timing values to struct fb_videomode.
>
> I could make necessary patch for that too, but I think you will spend
> less time doing it by yourself than syncing with me.
> What do you think?
Yes, I will prepare an additional patch in this series that will
maintain backward compatibility for real6410 and mini6410 with the
approach suggested by Jingoo Han. As I do not have either of these
boards, an ack from you would be very helpful when the next version of
this patchset is posted.
Thanks,
Thomas.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
On Mon, Mar 19, 2012 at 9:49 AM, Thomas Abraham wrote: > On 19 March 2012 13:10, Darius Augulis wrote: >> Right now s3c_fb_pd_win contains exactly LCD timing values. >> Your patch will move these timing values to struct fb_videomode. >> >> I could make necessary patch for that too, but I think you will spend >> less time doing it by yourself than syncing with me. >> What do you think? > > Yes, I will prepare an additional patch in this series that will > maintain backward compatibility for real6410 and mini6410 with the > approach suggested by Jingoo Han. As I do not have either of these > boards, an ack from you would be very helpful when the next version of > this patchset is posted. It's fine - I will review and probably test it. -- 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] mmc: dwmci: Add new register bit at IP version of 240A.
use_hold_reg bit is added at 240A version.
This register is..
Use Hold Register
0 - CMD and DATA sent to card bypassing HOLD Register
1 - CMD and DATA sent to card through the HOLD Register
Note:
Set to 1'b1 for SDR12 and SDR25
(with non-zero phase-shifted cclk_in_drv)
zero phase shift is not allowed in these modes.
Set to 1'b0 for SDR50, SDR104, and DDR50
(with zero phaseshifted cclk_in_drv)
Set to 1'b1 for SDR50, SDR104, and DDR50
(with non-zero phase-shifted cclk_in_drv)
Signed-off-by: JaeHun Jung
---
drivers/mmc/host/dw_mmc.c |5 +
drivers/mmc/host/dw_mmc.h |1 +
2 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 8bec1c3..e23bd5b 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -232,6 +232,7 @@ static void dw_mci_set_timeout(struct dw_mci *host)
static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command
*cmd)
{
struct mmc_data *data;
+ struct dw_mci_slot *slot = mmc_priv(mmc);
u32 cmdr;
cmd->error = -EINPROGRESS;
@@ -261,6 +262,10 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc,
struct mmc_command *cmd)
cmdr |= SDMMC_CMD_DAT_WR;
}
+ /* Use hold bit register */
+ if (slot->host->pdata->set_io_timing)
+ cmdr |= SDMMC_USE_HOLD_REG;
+
return cmdr;
}
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index df392a1..06c2818 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -111,6 +111,7 @@
#define SDMMC_INT_ERROR0xbfc2
/* Command register defines */
#define SDMMC_CMD_STARTBIT(31)
+#define SDMMC_USE_HOLD_REG BIT(29)
#define SDMMC_CMD_CCS_EXP BIT(23)
#define SDMMC_CMD_CEATA_RD BIT(22)
#define SDMMC_CMD_UPD_CLK BIT(21)
--
1.7.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] mmc: dwmci: Add new register bit at IP version of 240A.
Hi Jaehun.
I have sent the similar to your patch.
(http://www.spinics.net/lists/linux-mmc/msg10305.html)
I think good that add the checking point whether implement hold_reg in dwmmc-IP
or not.
On 03/19/2012 06:26 PM, JaeHun Jung wrote:
> use_hold_reg bit is added at 240A version.
> This register is..
> Use Hold Register
> 0 - CMD and DATA sent to card bypassing HOLD Register
> 1 - CMD and DATA sent to card through the HOLD Register
> Note:
> Set to 1'b1 for SDR12 and SDR25
> (with non-zero phase-shifted cclk_in_drv)
> zero phase shift is not allowed in these modes.
> Set to 1'b0 for SDR50, SDR104, and DDR50
> (with zero phaseshifted cclk_in_drv)
> Set to 1'b1 for SDR50, SDR104, and DDR50
> (with non-zero phase-shifted cclk_in_drv)
>
> Signed-off-by: JaeHun Jung
> ---
> drivers/mmc/host/dw_mmc.c |5 +
> drivers/mmc/host/dw_mmc.h |1 +
> 2 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 8bec1c3..e23bd5b 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -232,6 +232,7 @@ static void dw_mci_set_timeout(struct dw_mci *host)
> static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command
> *cmd)
> {
> struct mmc_data *data;
> + struct dw_mci_slot *slot = mmc_priv(mmc);
> u32 cmdr;
> cmd->error = -EINPROGRESS;
>
> @@ -261,6 +262,10 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc,
> struct mmc_command *cmd)
> cmdr |= SDMMC_CMD_DAT_WR;
> }
>
> + /* Use hold bit register */
> + if (slot->host->pdata->set_io_timing)
> + cmdr |= SDMMC_USE_HOLD_REG;
Where is defined set_io_timing?
> +
> return cmdr;
> }
>
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index df392a1..06c2818 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -111,6 +111,7 @@
> #define SDMMC_INT_ERROR 0xbfc2
> /* Command register defines */
> #define SDMMC_CMD_START BIT(31)
> +#define SDMMC_USE_HOLD_REG BIT(29)
> #define SDMMC_CMD_CCS_EXPBIT(23)
> #define SDMMC_CMD_CEATA_RD BIT(22)
> #define SDMMC_CMD_UPD_CLKBIT(21)
Best Regards,
Jaehoon Chung
--
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
s3c64xx wake up question
Hi, I am trying to get wake-up from IDLE and SLEEP working on my board based on S3C6410. The problem I see is that once the CPU goes to SLEEP or IDLE state, it does not wake up despite configuring activity on Keypad. I have verified the following: 1. VDD_ALIVE is powered up @1.2V 2. Dumped EINT_MASK and EINT0_MASK - reset with 0 (enable) for almost all the sources. 3. PWR_CFG is configured to be woken up for all sources (HSMMCx, RTC, KEYPAD etc) So, not sure if have something else to verify or check. I hope someone could provide some pointers on the problem. Regards, Madhvapathi Sriram Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog -- 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 V2 3/6] thermal: Add generic cpuhotplug cooling implementation
On 03/19/2012 11:47 AM, Amit Daniel Kachhap wrote:
> This patch adds support for generic cpu thermal cooling low level
> implementations using cpuhotplug based on the thermal level requested
> from user. Different cpu related cooling devices can be registered by the
> user and the binding of these cooling devices to the corresponding
> trip points can be easily done as the registration APIs return the
> cooling device pointer. The user of these APIs are responsible for
> passing the cpumask.
>
I am really not aware of the cpu thermal cooling stuff, but since this patch
deals with CPU Hotplug (which I am interested in), I have some questions
below..
> Signed-off-by: Amit Daniel Kachhap
> +
> +static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
> + unsigned long *state)
> +{
> + int ret = -EINVAL;
> + struct hotplug_cooling_device *hotplug_dev;
> +
> + mutex_lock(&cooling_cpuhotplug_lock);
> + list_for_each_entry(hotplug_dev, &cooling_cpuhotplug_list, node) {
> + if (hotplug_dev && hotplug_dev->cool_dev == cdev) {
> + *state = hotplug_dev->hotplug_state;
> + ret = 0;
> + break;
> + }
> + }
> + mutex_unlock(&cooling_cpuhotplug_lock);
> + return ret;
> +}
> +
> +/*This cooling may be as ACTIVE type*/
> +static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
> + unsigned long state)
> +{
> + int cpuid, this_cpu = smp_processor_id();
What prevents this task from being migrated to another CPU?
IOW, what ensures that 'this_cpu' remains valid throughout this function?
I see that you are acquiring mutex locks below.. So this is definitely not
a preempt disabled section.. so I guess my question above is valid..
Or is this code bound to a particular cpu?
> + struct hotplug_cooling_device *hotplug_dev;
> +
> + mutex_lock(&cooling_cpuhotplug_lock);
> + list_for_each_entry(hotplug_dev, &cooling_cpuhotplug_list, node)
> + if (hotplug_dev && hotplug_dev->cool_dev == cdev)
> + break;
> +
> + mutex_unlock(&cooling_cpuhotplug_lock);
> + if (!hotplug_dev || hotplug_dev->cool_dev != cdev)
> + return -EINVAL;
> +
> + if (hotplug_dev->hotplug_state == state)
> + return 0;
> +
> + /*
> + * This cooling device may be of type ACTIVE, so state field can
> + * be 0 or 1
> + */
> + if (state == 1) {
> + for_each_cpu(cpuid, hotplug_dev->allowed_cpus) {
> + if (cpu_online(cpuid) && (cpuid != this_cpu))
What prevents the cpu numbered cpuid from being taken down right at this moment?
Don't you need explicit synchronization with CPU Hotplug using something like
get_online_cpus()/put_online_cpus() here?
> + cpu_down(cpuid);
> + }
> + } else if (state == 0) {
> + for_each_cpu(cpuid, hotplug_dev->allowed_cpus) {
> + if (!cpu_online(cpuid) && (cpuid != this_cpu))
Same here.
> + cpu_up(cpuid);
> + }
> + } else {
> + return -EINVAL;
> + }
> +
> + hotplug_dev->hotplug_state = state;
> +
> + return 0;
> +}
> +/* bind hotplug callbacks to cpu hotplug cooling device */
> +static struct thermal_cooling_device_ops cpuhotplug_cooling_ops = {
> + .get_max_state = cpuhotplug_get_max_state,
> + .get_cur_state = cpuhotplug_get_cur_state,
> + .set_cur_state = cpuhotplug_set_cur_state,
> +};
> +
--
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] usb: gadget: s3c-hsotg: fix kernel panic
On Fri, 16 Mar 2012 13:35:20 + Sangwook Lee wrote: > Fix kernel panic from s3c_hsotg_udc_stop. > if udc_is_newstyle is true, s3c_hsotg_udc_stop should not > call disconnect, unbind. > > As running rmmod g_mass_storage, kernel panic happens. > > (composite_unbind+0x14/0x164 [g_mass_storage]) > from [] (s3c_hsotg_udc_stop) > > This patch is based on Lukasz Majewski's patches: > [PATCH 0/9] USB: s3c-hsotg: USB S3C-HSOTG driver fixes and code cleanu > > in order to test g_mass_storage in Origen board: > > Signed-off-by: Sangwook Lee > --- > drivers/usb/gadget/s3c-hsotg.c | 12 ++-- > 1 files changed, 6 insertions(+), 6 deletions(-) > Hi Sangwook, Test HW: GONI S5PV210 module: g_ether.ko Yes, this patch works. Thanks for spotting the error. Tested-by: Lukasz Majewski -- Best regards, Lukasz Majewski Samsung Poland R&D Center Platform Group -- 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 2/3] i2c-s3c2410: Rework device type handling
On Thu, Mar 15, 2012 at 05:54:33PM +0100, Karol Lewandowski wrote: > If you consider code to be inherently less readable because of this > approach I'll rework it. If it's not a such big deal for you I would > prefer to keep it as is. The thing that was causing me to think the code was funny was mainly the fact that we're now combining both quirk based selection and chip type based selection into the same bitmask. If the chip types were quirks it'd probably not have looked odd, and that might just be a case of renaming them - I can't remember what they do. signature.asc Description: Digital signature
Re: [PATCH 1/5] pinctrl: add samsung pinctrl and gpiolib driver
On 03/11/2012 06:46 AM, Thomas Abraham wrote:
> Add a new pinctrl and gpiolib driver for Samsung SoC's. This driver provides a
> common framework for all Samsung SoC's to interface with the pinctrl and
> gpiolib subsystems.
>
> This driver is split into two parts: the pinctrl interface and the gpiolib
> interface. The pinctrl interface registers pinctrl devices with the pinctrl
> subsystem and gpiolib interface registers gpio chips with the gpiolib
> subsystem. The information about the pins, pin groups, pin functions and
> gpio chips, which are SoC specific, are all provided to the driver using
> driver data. The driver registers all the pinctrl devices and gpio chips
> which are found in the driver data.
> diff --git a/arch/arm/plat-samsung/include/plat/pinctrl.h
> b/arch/arm/plat-samsung/include/plat/pinctrl.h
It'd be nice to name this samsung-pinctrl.h, or something other than
just "pinctrl.h". That way, this new header won't cause problems for a
multi-SoC kernel in the future where multiple plat-*/include/plat or
mach-*/include/mach directories are in the include path.
> diff --git a/drivers/pinctrl/pinctrl-samsung.c
> b/drivers/pinctrl/pinctrl-samsung.c
> +/* check if the selector is a valid pin function selector */
> +static int samsung_pinmux_list_funcs(struct pinctrl_dev *pctldev,
> + unsigned selector)
> +{
> + struct samsung_pinctrl_drv_data *drvdata;
> +
> + drvdata = pinctrl_dev_get_drvdata(pctldev);
> + if (selector >= drvdata->nr_groups)
> + return -EINVAL;
That test should be against something other than nr_groups; nr_functions
or similar, right?
> +static void samsung_pimux_setup(struct pinctrl_dev *pctldev, unsigned
> selector,
s/pimux/pinmux/
...
> + const unsigned int *pin;
...
> + pin = drvdata->pin_groups[group].pins;
It might be a little clearer to rename "pin" to "pins", since it's an
array...
> +
> + /*
> + * for each pin in the pin group selected, program the correspoding pin
> + * pin function number in the config register.
> + */
> + for (cnt = 0; cnt < drvdata->pin_groups[group].num_pins; cnt++, pin++) {
> + pin_to_reg_bank(drvdata->gc, *pin - drvdata->ctrl->base,
> + ®, &pin_offset, &bank);
... and say pins[cnt] instead of *pin here (and remove pin++ from the
for loop statement)
But it's just a slight suggestion; your call.
> +static int samsung_pinmux_gpio_set_direction(struct pinctrl_dev *pctldev,
> + struct pinctrl_gpio_range *range, unsigned offset, bool input)
...
> + pin_to_reg_bank(range->gc, offset, ®, &pin_offset, &bank);
> + mask = (1 << bank->func_width) - 1;
> + shift = pin_offset * bank->func_width;
It might be useful to put those 3 lines into a helper function since
they're duplicating with samsung_pimux_setup() and similar code is in
samsung_pinconf_set() too.
> +static int samsung_pinconf_group_set(struct pinctrl_dev *pctldev,
> + unsigned group, unsigned long config)
I think you can leave out group_set(), and the pinctrl core will loop
over all pins in the group for you.
> +static void samsung_gpio_set(struct gpio_chip *gc, unsigned offset, int
> value)
...
> + data = readl(reg + DAT_REG);
...
> + __raw_writel(data, reg + DAT_REG);
Why sometimes use the __raw variants and sometimes not?
> +static int samsung_gpio_direction_output(struct gpio_chip *gc, unsigned
> offset,
> + int value)
...
> + ret = pinctrl_gpio_direction_output(gc->base + offset);
> + if (!ret)
> + samsung_gpio_set(gc, offset, value);
This will set the GPIO to output direction before programming the output
value, which might cause a glitch. You may want to try and swap those
two function calls.
> +static int __devinit samsung_pinctrl_probe(struct platform_device *pdev)
...
> + res = request_mem_region(res->start, resource_size(res),
> + pdev->name);
> + if (!res) {
> + dev_err(&pdev->dev, "request for mem region failed\n");
> + return -EBUSY;
> + }
> +
> + drvdata->virt_base = ioremap(res->start, resource_size(res));
Perhaps replace those two function calls with
devm_request_and_ioremap(), and as a bonus you won't have to unmap or
release the region either.
> + if (!drvdata->virt_base) {
> + dev_err(&pdev->dev, "ioremap failed\n");
i.e. you wouldn't have to add the missing error-handling here, and below.
> + return -EINVAL;
> + }
> +/* driver data for various samsung soc's */
> +#ifdef CONFIG_CPU_EXYNOS4210
> +
> +#define EXYNOS4210_PCTRL_DRVDATA
> ((kernel_ulong_t)&exynos4210_pinctrl_drv_data)
> +#else
> +#define EXYNOS4210_PCTRL_DRVDATA ((kernel_ulong_t)NULL)
> +#endif /* CONFIG_CPU_EXYNOS4210 */
Doesn't that interact badly with samsung_pinctrl_get_driver_data()
above, which just blindly adds to the .d
Re: [PATCH 5/5] mmc: sdhci-s3c: setup pins using pinctrl interface
On 03/11/2012 06:46 AM, Thomas Abraham wrote:
> The platform specific callback to setup the sdhci pin mux and pin config
> is removed and the pinctrl subsystem interface is used to setup the
> mux and config.
> @@ -643,8 +649,13 @@ static int __devinit sdhci_s3c_probe(struct
> platform_device *pdev)
> }
>
> /* Ensure we have minimal gpio selected CMD/CLK/Detect */
> - if (pdata->cfg_gpio)
> - pdata->cfg_gpio(pdev, pdata->max_width);
> + pstate = pdata->max_width ? "sdhci-pcfg8" : "sdhci-pcfg4";
If the driver is going to select a single state ("sdhci-pcfg8" or
"sdhci-pcfg4" above) at probe() time and never change it (which seems
quite reasonable for an SDHCI controller), then the driver should always
use state PINCTRL_STATE_DEFAULT, and it should be up to the board to set
up the mapping table such that PINCTRL_STATE_DEFAULT sets up the pins
for either 4-bit or 8-bit as appropriate for the board.
--
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
[GIT PULL] Power management updates for 3.4
Hi Linus, Please pull power management updates for 3.4 since commit 192cfd58774b4d17b2fe8bdc77d89c2ef4e0591d: Linux 3.3-rc6 with top-most commit 98e8bdafeb4728a6af7bbcbcc3984967d1cf2bc1 Merge branch 'pm-domains' from the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git pm-for-3.4 They include: * Introduction of early/late suspend/hibernation device callbacks. * Generic PM domains extensions and fixes. * devfreq updates from Axel Lin and MyungJoo Ham. * Device PM QoS updates. * Fixes of concurrency problems with wakeup sources. * System suspend and hibernation fixes. Thanks! Documentation/ABI/testing/sysfs-devices-power | 18 ++ .../bindings/arm/exynos/power_domain.txt | 21 ++ Documentation/power/devices.txt| 93 +--- Documentation/power/freezing-of-tasks.txt | 21 ++ arch/arm/mach-exynos/Kconfig | 10 +- arch/arm/mach-exynos/Makefile |2 +- arch/arm/mach-exynos/dev-pd.c | 139 --- arch/arm/mach-exynos/mach-nuri.c | 11 - arch/arm/mach-exynos/mach-origen.c | 14 - arch/arm/mach-exynos/mach-smdkv310.c | 12 - arch/arm/mach-exynos/mach-universal_c210.c | 17 -- arch/arm/mach-exynos/pm_domains.c | 195 +++ arch/arm/mach-shmobile/setup-sh7372.c |2 + arch/x86/kernel/apm_32.c | 11 +- drivers/base/power/domain.c| 253 +++- drivers/base/power/generic_ops.c | 157 drivers/base/power/main.c | 247 +-- drivers/base/power/power.h |4 + drivers/base/power/qos.c | 61 + drivers/base/power/sysfs.c | 47 drivers/base/power/wakeup.c| 85 +-- drivers/clocksource/sh_cmt.c |4 + drivers/clocksource/sh_mtu2.c |4 + drivers/clocksource/sh_tmu.c |4 + drivers/devfreq/devfreq.c | 112 +- drivers/devfreq/exynos4_bus.c | 23 +- drivers/devfreq/governor_performance.c |5 +- drivers/devfreq/governor_powersave.c |2 +- drivers/devfreq/governor_simpleondemand.c | 12 +- drivers/devfreq/governor_userspace.c | 15 +- drivers/mmc/host/sh_mmcif.c|5 + drivers/mmc/host/tmio_mmc_pio.c|5 + drivers/xen/manage.c |6 +- fs/jbd/journal.c |2 + fs/jbd2/journal.c |2 + include/linux/devfreq.h| 25 ++- include/linux/pm.h | 59 - include/linux/pm_domain.h | 25 ++- include/linux/pm_qos.h | 64 ++ include/linux/pm_wakeup.h | 22 ++- include/linux/suspend.h|4 + kernel/exit.c |2 +- kernel/freezer.c |6 +- kernel/kexec.c |8 +- kernel/power/Makefile |3 +- kernel/power/hibernate.c | 47 ++-- kernel/power/main.c| 20 +- kernel/power/power.h |7 +- kernel/power/process.c | 24 +- kernel/power/qos.c | 23 +- kernel/power/snapshot.c|7 +- kernel/power/suspend.c | 84 kernel/power/user.c| 12 +- 53 files changed, 1452 insertions(+), 611 deletions(-) --- Alex Frid (1): PM / QoS: Simplify PM QoS expansion/merge Axel Lin (2): devfreq: exynos4_bus: Use dev_get_drvdata at appropriate places devfreq: Remove MODULE_ALIAS for exynos4 busfreq driver Bjorn Helgaas (1): PM / Hibernate: print physical addresses consistently with other parts of kernel Guennadi Liakhovetski (1): PM / Domains: Provide a dummy dev_gpd_data() when generic domains are not used Jean Pihet (1): PM / QoS: unconditionally build the feature Magnus Damm (1): PM / Domains: Fix include for PM_GENERIC_DOMAINS=n case Marcos Paulo de Souza (2): PM / Suspend: Avoid code duplication in suspend statistics update PM / Freezer: Remove references to TIF_FREEZE in comments MyungJoo Ham (3): PM / devfreq: fixed syntax errors. PM / devfreq: add min/max_freq limit requested by users. PM / devfreq: add relation o
Re: [PATCH V2 3/6] thermal: Add generic cpuhotplug cooling implementation
On 19 March 2012 17:15, Srivatsa S. Bhat
wrote:
> On 03/19/2012 11:47 AM, Amit Daniel Kachhap wrote:
>
>> This patch adds support for generic cpu thermal cooling low level
>> implementations using cpuhotplug based on the thermal level requested
>> from user. Different cpu related cooling devices can be registered by the
>> user and the binding of these cooling devices to the corresponding
>> trip points can be easily done as the registration APIs return the
>> cooling device pointer. The user of these APIs are responsible for
>> passing the cpumask.
>>
>
>
> I am really not aware of the cpu thermal cooling stuff, but since this patch
> deals with CPU Hotplug (which I am interested in), I have some questions
> below..
>
>
>> Signed-off-by: Amit Daniel Kachhap
>> +
>> +static int cpuhotplug_get_cur_state(struct thermal_cooling_device *cdev,
>> + unsigned long *state)
>> +{
>> + int ret = -EINVAL;
>> + struct hotplug_cooling_device *hotplug_dev;
>> +
>> + mutex_lock(&cooling_cpuhotplug_lock);
>> + list_for_each_entry(hotplug_dev, &cooling_cpuhotplug_list, node) {
>> + if (hotplug_dev && hotplug_dev->cool_dev == cdev) {
>> + *state = hotplug_dev->hotplug_state;
>> + ret = 0;
>> + break;
>> + }
>> + }
>> + mutex_unlock(&cooling_cpuhotplug_lock);
>> + return ret;
>> +}
>> +
>> +/*This cooling may be as ACTIVE type*/
>> +static int cpuhotplug_set_cur_state(struct thermal_cooling_device *cdev,
>> + unsigned long state)
>> +{
>> + int cpuid, this_cpu = smp_processor_id();
>
>
> What prevents this task from being migrated to another CPU?
> IOW, what ensures that 'this_cpu' remains valid throughout this function?
>
> I see that you are acquiring mutex locks below.. So this is definitely not
> a preempt disabled section.. so I guess my question above is valid..
>
> Or is this code bound to a particular cpu?
No this thread is not bound to a cpu. Your comment is valid and some
synchronization is needed for preemption. Thanks for pointing this
out.
>
>> + struct hotplug_cooling_device *hotplug_dev;
>> +
>> + mutex_lock(&cooling_cpuhotplug_lock);
>> + list_for_each_entry(hotplug_dev, &cooling_cpuhotplug_list, node)
>> + if (hotplug_dev && hotplug_dev->cool_dev == cdev)
>> + break;
>> +
>> + mutex_unlock(&cooling_cpuhotplug_lock);
>> + if (!hotplug_dev || hotplug_dev->cool_dev != cdev)
>> + return -EINVAL;
>> +
>> + if (hotplug_dev->hotplug_state == state)
>> + return 0;
>> +
>> + /*
>> + * This cooling device may be of type ACTIVE, so state field can
>> + * be 0 or 1
>> + */
>> + if (state == 1) {
>> + for_each_cpu(cpuid, hotplug_dev->allowed_cpus) {
>> + if (cpu_online(cpuid) && (cpuid != this_cpu))
>
>
> What prevents the cpu numbered cpuid from being taken down right at this
> moment?
> Don't you need explicit synchronization with CPU Hotplug using something like
> get_online_cpus()/put_online_cpus() here?
>
>> + cpu_down(cpuid);
>> + }
>> + } else if (state == 0) {
>> + for_each_cpu(cpuid, hotplug_dev->allowed_cpus) {
>> + if (!cpu_online(cpuid) && (cpuid != this_cpu))
>
>
> Same here.
>
>> + cpu_up(cpuid);
>> + }
>> + } else {
>> + return -EINVAL;
>> + }
>> +
>> + hotplug_dev->hotplug_state = state;
>> +
>> + return 0;
>> +}
>> +/* bind hotplug callbacks to cpu hotplug cooling device */
>> +static struct thermal_cooling_device_ops cpuhotplug_cooling_ops = {
>> + .get_max_state = cpuhotplug_get_max_state,
>> + .get_cur_state = cpuhotplug_get_cur_state,
>> + .set_cur_state = cpuhotplug_set_cur_state,
>> +};
>> +
>
--
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
