* [PATCH 10/11] video: da8xx-fb: adopt pinctrl support
From: Hebbar Gururaja @ 2013-05-31 10:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1369995191-20855-1-git-send-email-gururaja.hebbar@ti.com>
Amend the da8xx-fb controller to optionally take a pin control handle
and set the state of the pins to:
- "default" on boot, resume
- "sleep" on suspend()
By optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.
If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.
Todo:
- if an idle state is available for pins, add support for it.
Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: linux-fbdev@vger.kernel.org
---
:100644 100644 0810939... 10c8036... M drivers/video/da8xx-fb.c
drivers/video/da8xx-fb.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 0810939..10c8036 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -36,6 +36,7 @@
#include <linux/slab.h>
#include <linux/delay.h>
#include <linux/lcm.h>
+#include <linux/pinctrl/consumer.h>
#include <video/da8xx-fb.h>
#include <asm/div64.h>
@@ -182,6 +183,11 @@ struct da8xx_fb_par {
#endif
void (*panel_power_ctrl)(int);
u32 pseudo_palette[16];
+
+ /* two pin states - default, sleep */
+ struct pinctrl *pinctrl;
+ struct pinctrl_state *pins_default;
+ struct pinctrl_state *pins_sleep;
};
/* Variable Screen Information */
@@ -1306,6 +1312,36 @@ static int fb_probe(struct platform_device *device)
par->lcd_fck_rate = clk_get_rate(fb_clk);
#endif
par->pxl_clk = lcdc_info->pixclock;
+
+ par->pinctrl = devm_pinctrl_get(&device->dev);
+ if (!IS_ERR(par->pinctrl)) {
+ par->pins_default = pinctrl_lookup_state(par->pinctrl,
+ PINCTRL_STATE_DEFAULT);
+ if (IS_ERR(par->pins_default))
+ dev_dbg(&device->dev, "could not get default pinstate\n");
+ else
+ if (pinctrl_select_state(par->pinctrl,
+ par->pins_default))
+ dev_err(&device->dev,
+ "could not set default pinstate\n");
+
+ par->pins_sleep = pinctrl_lookup_state(par->pinctrl,
+ PINCTRL_STATE_SLEEP);
+ if (IS_ERR(par->pins_sleep))
+ dev_dbg(&device->dev, "could not get sleep pinstate\n");
+ } else {
+ /*
+ * Since we continue even when pinctrl node is not found,
+ * Invalidate pins as not available. This is to make sure that
+ * IS_ERR(pins_xxx) results in failure when used.
+ */
+ par->pins_default = ERR_PTR(-ENODATA);
+ par->pins_sleep = ERR_PTR(-ENODATA);
+
+ dev_dbg(&device->dev, "did not get pins for lcd error: %li\n",
+ PTR_ERR(par->pinctrl));
+ }
+
if (fb_pdata->panel_power_ctrl) {
par->panel_power_ctrl = fb_pdata->panel_power_ctrl;
par->panel_power_ctrl(1);
@@ -1551,6 +1587,12 @@ static int fb_suspend(struct platform_device *dev, pm_message_t state)
pm_runtime_put_sync(&dev->dev);
console_unlock();
+ /* Optionally let pins go into sleep states */
+ if (!IS_ERR(par->pins_sleep))
+ if (pinctrl_select_state(par->pinctrl, par->pins_sleep))
+ dev_err(&dev->dev,
+ "could not set pins to sleep state\n");
+
return 0;
}
static int fb_resume(struct platform_device *dev)
@@ -1560,6 +1602,12 @@ static int fb_resume(struct platform_device *dev)
console_lock();
pm_runtime_get_sync(&dev->dev);
+
+ /* Optionaly enable pins to be muxed in and configured */
+ if (!IS_ERR(par->pins_default))
+ if (pinctrl_select_state(par->pinctrl, par->pins_default))
+ dev_err(&dev->dev, "could not set default pins\n");
+
lcd_context_restore();
if (par->blank = FB_BLANK_UNBLANK) {
lcd_enable_raster();
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v2 2/3] video: xilinxfb: Do not use out_be32 IO function
From: Michal Simek @ 2013-05-31 7:39 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Michal Simek, linux-kernel, Florian Tobias Schandinat,
linux-fbdev
In-Reply-To: <3808365.SOUxDqkW9J@wuerfel>
[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]
On 05/31/2013 12:04 AM, Arnd Bergmann wrote:
> On Thursday 30 May 2013 11:41:01 Michal Simek wrote:
>> * To perform the read/write on the registers we need to check on
>> * which bus its connected and call the appropriate write API.
>> */
>> -static void xilinx_fb_out_be32(struct xilinxfb_drvdata *drvdata, u32 offset,
>> +static void xilinx_fb_out32(struct xilinxfb_drvdata *drvdata, u32 offset,
>> u32 val)
>> {
>> if (drvdata->flags & PLB_ACCESS_FLAG)
>> - out_be32(drvdata->regs + (offset << 2), val);
>> + __raw_writel(val, drvdata->regs + (offset << 2));
>> #ifdef CONFIG_PPC_DCR
>> else
>> dcr_write(drvdata->dcr_host, offset, val);
>>
>
> This is probably missing barriers, and is wrong on systems on which
> the endianess of the device is different from the CPU.
>
> You already have an indirection in there, so I guess it won't hurt
> to create a third case for little-endian registers and add
> another bit in drvdata->flags, or make it depend on the architecture,
> if the endianess of the device registers is known at compile time.
The PLB_ACCESS_FLAGS is incorrectly named. It means BUS_ACCESS.
But I will find a way how to autodetect endianess directly on IP
as I have done it for uartlite and will send v3.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* linux-next: fbdev-next inclusing
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-05-31 5:13 UTC (permalink / raw)
To: linux-fbdev
HI,
Could you include the new fbdev -next in the linux-next please?
git://git.kernel.org/pub/scm/linux/kernel/git/plagnioj/linux-fbdev.git for-next
Best Regards,
J.
^ permalink raw reply
* Re: [PATCH] backlight: Turn backlight on/off when necessary
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-05-31 5:02 UTC (permalink / raw)
To: Liu Ying; +Cc: Liu Ying, Florian Tobias Schandinat, linux-fbdev, linux-kernel
In-Reply-To: <CA+8Hj80pF_zNqr8DC70FV0=NU39QbZOZm=2YeJSGEnr4aEFT4g@mail.gmail.com>
On 10:31 Fri 31 May , Liu Ying wrote:
> 2013/5/30 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>
> On 16:13 Thu 30 May , Liu Ying wrote:
> > We don't have to turn backlight on/off everytime a blanking
> > or unblanking event comes because the backlight status may have
> > already been what we want. Another thought is that one backlight
> > device may be shared by multiple framebuffers. We don't hope that
> > blanking one of the framebuffers would turn the backlight off for
> > all the other framebuffers, since they are likely active to show
> > display content. This patch adds logic to record each framebuffer's
> > backlight status to determine the backlight device use count and
> > whether the backlight should be turned on or off.
> >
> > Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> > ---
> > drivers/video/backlight/backlight.c | 23 +++++++++++++++++------
> > include/linux/backlight.h | 6 ++++++
> > 2 files changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/video/backlight/backlight.c
> b/drivers/video/backlight/backlight.c
> > index c74e7aa..97ea2b8 100644
> > --- a/drivers/video/backlight/backlight.c
> > +++ b/drivers/video/backlight/backlight.c
> > @@ -31,13 +31,14 @@ static const char *const backlight_types[] = {
> >
> defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE))
> > /* This callback gets called when something important happens inside
> a
> > * framebuffer driver. We're looking if that important event is
> blanking,
> > - * and if it is, we're switching backlight power as well ...
> > + * and if it is and necessary, we're switching backlight power as
> well ...
> > */
> > static int fb_notifier_callback(struct notifier_block *self,
> > unsigned long event, void *data)
> > {
> > struct backlight_device *bd;
> > struct fb_event *evdata = data;
> > + int node = evdata->info->node;
> >
> > /* If we aren't interested in this event, skip it immediately
> ... */
> > if (event != FB_EVENT_BLANK && event != FB_EVENT_CONBLANK)
> > @@ -49,11 +50,21 @@ static int fb_notifier_callback(struct
> notifier_block *self,
> > if (!bd->ops->check_fb ||
> > bd->ops->check_fb(bd, evdata->info)) {
> > bd->props.fb_blank = *(int *)evdata->data;
> > - if (bd->props.fb_blank = FB_BLANK_UNBLANK)
> > - bd->props.state &= ~BL_CORE_FBBLANK;
> > - else
> > - bd->props.state |= BL_CORE_FBBLANK;
> > - backlight_update_status(bd);
> > + if (bd->props.fb_blank = FB_BLANK_UNBLANK &&
> > + !bd->fb_bl_on[node]) {
> > + bd->fb_bl_on[node] = true;
> > + if (!bd->use_count++) {
> > + bd->props.state &> ~BL_CORE_FBBLANK;
> > + backlight_update_status(bd);
> > + }
> > + } else if (bd->props.fb_blank !> FB_BLANK_UNBLANK &&
> > + bd->fb_bl_on[node]) {
> > + bd->fb_bl_on[node] = false;
> > + if (!(--bd->use_count)) {
> > + bd->props.state |> BL_CORE_FBBLANK;
> > + backlight_update_status(bd);
> > + }
> > + }
> > }
> > mutex_unlock(&bd->ops_lock);
> > return 0;
> > diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> > index da9a082..5de71a0 100644
> > --- a/include/linux/backlight.h
> > +++ b/include/linux/backlight.h
> > @@ -9,6 +9,7 @@
> > #define _LINUX_BACKLIGHT_H
> >
> > #include <linux/device.h>
> > +#include <linux/fb.h>
> > #include <linux/mutex.h>
> > #include <linux/notifier.h>
> >
> > @@ -101,6 +102,11 @@ struct backlight_device {
> > struct notifier_block fb_notif;
> >
> > struct device dev;
> > +
> > + /* Multiple framebuffers may share one backlight device */
> > + bool fb_bl_on[FB_MAX];
>
> I don't like such array at all
>
> I understand the fact you will have only on hw backlight for x fb or
> overlay
> but have a static on no
>
>
> My board has two LVDS display panels. They share one PWM backlight.
> The framebuffer HW engine may drive a background framebuffer and an
> overlay framebuffer on one panel, and only one background framebuffer on
> the other panel. The three framebuffers may be active simultaneously.
>
>
> if you want to track all user create a strcut and register it or do more
> simple just as a int to count the number of user and shut down it if 0
> and
> enable it otherwise
>
> Users may unblank a framebuffer for multiple times continuously and then
> trigger a blanking operation.
> If that framebuffer is the only user of the backlight, the backlight will
> be turned off after the blanking operation.
> This is the behavior before this patch is applied to kernel. And, this
> patch doesn't change the behavior here.
> So, it seems that it is reasonable to record backlight status(on or off)
> for framebuffers. And, I use a straightforward array for the recording.
> I thought about changing to use a list instead for the recording, but it
> appears to me it would take more CPU cycles to search and update entries.
> It is basically a kind of space-against-speed trade-off.
> You probably have already provided me a better way to do this, but it
> looks I didn't catch it. If this is the case, would you please shed more
> light on this? Thanks!
so just use a int
check who we do for clk_enable/disable on at91
arch/arm/mach-at91/clock.c
Best Regards,
J.
>
> Best Regards,
> J.
> > +
> > + int use_count;
> > };
> >
> > static inline void backlight_update_status(struct backlight_device
> *bd)
> > --
> > 1.7.1
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fbdev"
> in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Best Regards,
> Liu Ying
^ permalink raw reply
* Re: [PATCH] fbdev: bfin-lq035q1-fb: Use dev_pm_ops
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-05-31 5:01 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Rafael J. Wysocki', 'Lars-Peter Clausen',
'Michael Hennerich', 'Tomi Valkeinen',
linux-fbdev, linux-pm
In-Reply-To: <000a01ce5daf$8b228b80$a167a280$@samsung.com>
On 12:32 Fri 31 May , Jingoo Han wrote:
> On Thursday, May 30, 2013 7:46 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 09:52 Thu 30 May , Lars-Peter Clausen wrote:
> > > On 05/30/2013 09:14 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 09:32 Thu 30 May , Jingoo Han wrote:
> > > >> On Thursday, May 30, 2013 4:20 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > >>> On 16:24 Wed 29 May , Michael Hennerich wrote:
> > > >>>> On 05/29/2013 02:17 PM, Lars-Peter Clausen wrote:
> > > >>>>> Use dev_pm_ops instead of the legacy suspend/resume callbacks.
> > > >>>>>
> > > >>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > >>>> Acked-by: Michael Hennerich <michael.hennerich@analog.com>
> > > >>>>> ---
> > > >>>>> drivers/video/bfin-lq035q1-fb.c | 22 ++++++++++++++--------
> > > >>>>> 1 file changed, 14 insertions(+), 8 deletions(-)
> > > >>>>>
> > > >>>>> diff --git a/drivers/video/bfin-lq035q1-fb.c b/drivers/video/bfin-lq035q1-fb.c
> > > >>>>> index 29d8c04..6084c17 100644
> > > >>>>> --- a/drivers/video/bfin-lq035q1-fb.c
> > > >>>>> +++ b/drivers/video/bfin-lq035q1-fb.c
> > > >>>>> @@ -170,16 +170,19 @@ static int lq035q1_spidev_remove(struct spi_device *spi)
> > > >>>>> return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_SHUT);
> > > >>>>> }
> > > >>>>> -#ifdef CONFIG_PM
> > > >>>>> -static int lq035q1_spidev_suspend(struct spi_device *spi, pm_message_t state)
> > > >>>>> +#ifdef CONFIG_PM_SLEEP
> > > >>>>> +static int lq035q1_spidev_suspend(struct device *dev)
> > > >>>>> {
> > > >>>>> + struct spi_device *spi = to_spi_device(dev);
> > > >>>>> +
> > > >>>>> return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_SHUT);
> > > >>>>> }
> > > >>>>> -static int lq035q1_spidev_resume(struct spi_device *spi)
> > > >>>>> +static int lq035q1_spidev_resume(struct device *dev)
> > > >>>>> {
> > > >>>>> - int ret;
> > > >>>>> + struct spi_device *spi = to_spi_device(dev);
> > > >>>>> struct spi_control *ctl = spi_get_drvdata(spi);
> > > >>>>> + int ret;
> > > >>>>> ret = lq035q1_control(spi, LQ035_DRIVER_OUTPUT_CTL, ctl->mode);
> > > >>>>> if (ret)
> > > >>>>> @@ -187,9 +190,13 @@ static int lq035q1_spidev_resume(struct spi_device *spi)
> > > >>>>> return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_ON);
> > > >>>>> }
> > > >>>>> +
> > > >>>>> +static SIMPLE_DEV_PM_OPS(lq035q1_spidev_pm_ops, lq035q1_spidev_suspend,
> > > >>>>> + lq035q1_spidev_resume);
> > > >>>>> +#define LQ035Q1_SPIDEV_PM_OPS (&lq035q1_spidev_pm_ops)
> > > >>>>> +
> > > >>>>> #else
> > > >>>>> -# define lq035q1_spidev_suspend NULL
> > > >>>>> -# define lq035q1_spidev_resume NULL
> > > >>>>> +#define LQ035Q1_SPIDEV_PM_OPS NULL
> > > >>>>> #endif
> > > >>> we really need to ahve a macro like for DT of_match_ptr to drop the #else
> > > >>
> > > >> Hi Jean-Christophe PLAGNIOL-VILLARD,
> > > >>
> > > >> I submitted the following patch. :)
> > > >> (https://patchwork.kernel.org/patch/2502971/)
> > > >>
> > > >> --- a/include/linux/pm.h
> > > >> +++ b/include/linux/pm.h
> > > >> @@ -55,8 +55,10 @@ struct device;
> > > >>
> > > >> #ifdef CONFIG_PM
> > > >> extern const char power_group_name[]; /* = "power" */
> > > >> +#define pm_ops_ptr(_ptr) (_ptr)
> > > >> #else
> > > >> #define power_group_name NULL
> > > >> +#define pm_ops_ptr(_ptr) NULL
> > > >> #endif
> > > >>
> > > >>
> > > >> This patch was accepted by Rafael Wysocki, and will be merged to v3.11-rc1.
> > > >>
> > > > Lars-Peter please update with and
> > >
> > > Since the code depends on CONFIG_PM_SLEEP and not CONFIG_PM I don't think
> > > the macro will work.
> >
> > se please ad a similar macros too
>
> Hi Jean-Christophe,
>
> I added pm_sleep_ops_ptr() as below.
> Maybe you want it.
> There is no build warnings below 4 config situations.
> a) CONFIG_PM is enabled.
> b) only CONFIG_PM_SLEEP is enabled
> c) only CONFIG_PM_RUNTIME is enabled
> d) CONFIG_PM is NOT enabled.
>
> --- a/drivers/video/bfin-lq035q1-fb.c
> +++ b/drivers/video/bfin-lq035q1-fb.c
> @@ -170,16 +170,19 @@ static int lq035q1_spidev_remove(struct spi_device *spi)
> return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_SHUT);
> }
>
> -#ifdef CONFIG_PM
> -static int lq035q1_spidev_suspend(struct spi_device *spi, pm_message_t state)
> +#ifdef CONFIG_PM_SLEEP
> +static int lq035q1_spidev_suspend(struct device *dev)
> {
> + struct spi_device *spi = to_spi_device(dev);
> +
> return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_SHUT);
> }
>
> -static int lq035q1_spidev_resume(struct spi_device *spi)
> +static int lq035q1_spidev_resume(struct device *dev)
> {
> - int ret;
> + struct spi_device *spi = to_spi_device(dev);
> struct spi_control *ctl = spi_get_drvdata(spi);
> + int ret;
>
> ret = lq035q1_control(spi, LQ035_DRIVER_OUTPUT_CTL, ctl->mode);
> if (ret)
> @@ -187,11 +190,11 @@ static int lq035q1_spidev_resume(struct spi_device *spi)
>
> return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_ON);
> }
> -#else
> -# define lq035q1_spidev_suspend NULL
> -# define lq035q1_spidev_resume NULL
> #endif
>
> +static SIMPLE_DEV_PM_OPS(lq035q1_spidev_pm_ops, lq035q1_spidev_suspend,
> + lq035q1_spidev_resume);
> +
> /* Power down all displays on reboot, poweroff or halt */
> static void lq035q1_spidev_shutdown(struct spi_device *spi)
> {
> @@ -708,8 +711,7 @@ static int bfin_lq035q1_probe(struct platform_device *pdev)
> info->spidrv.probe = lq035q1_spidev_probe;
> info->spidrv.remove = lq035q1_spidev_remove;
> info->spidrv.shutdown = lq035q1_spidev_shutdown;
> - info->spidrv.suspend = lq035q1_spidev_suspend;
> - info->spidrv.resume = lq035q1_spidev_resume;
> + info->spidrv.driver.pm = pm_sleep_ops_ptr(&lq035q1_spidev_pm_ops);
>
> ret = spi_register_driver(&info->spidrv);
> if (ret < 0) {
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index bd50d15..999d652 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -61,6 +61,12 @@ extern const char power_group_name[]; /* = "power" */
> #define pm_ops_ptr(_ptr) NULL
> #endif
>
> +#ifdef CONFIG_PM_SLEEP
> +#define pm_sleep_ops_ptr(_ptr) (_ptr)
> +#else
> +#define pm_sleep_ops_ptr(_ptr) NULL
> +#endif
> +
>
>
> Hi Rafael J. Wysocki,
> How about adding this pm_sleep_ops_ptr() macro?
>
> My draft idea is below:
> However, I want other's better ideas. :)
>
> 1. The case that only CONFIG_PM_SLEEP is necessary.
> #ifdef CONFIG_PM_SLEEP
> static int *_suspend(struct device *dev)
> [....]
> static int *_resume(struct device *dev)
> [....]
> #endif
>
> static SIMPLE_DEV_PM_OPS(*_pm_ops, *_suspend, *_resume);
>
> static struct {platform/spi/i2c/etc}_driver *_driver = {
> .driver = {
> .pm = pm_sleep_ops_ptr(&*_pm_ops),
>
>
> 2. The case that both CONFIG_PM_SLEEP & CONFIG_PM_RUNTIME
> are necessary.
>
> #ifdef CONFIG_PM_SLEEP
> static int *_suspend(struct device *dev)
> [....]
> static int *_resume(struct device *dev)
> [....]
> #endif
>
> #ifdef CONFIG_PM_RUNTIME
> static int *_runtime_suspend(struct device *dev)
> [....]
> static int *_runtime_resume(struct device *dev)
> [....]
> #endif
>
> static const struct dev_pm_ops *_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(*_suspend, *_resume)
> SET_RUNTIME_PM_OPS(*_runtime_suspend, *_runtime_resume, NULL)
> };
>
> static struct {platform/spi/i2c/etc}_driver *_driver = {
> .driver = {
> .pm = pm_ops_ptr(&*_pm_ops),
>
>
yes for me
Best Regards,
J.
>
> Best regards,
> Jingoo Han
>
>
^ permalink raw reply
* Re: [PATCH v8, part3 06/14] mm, acornfb: use free_reserved_area() to simplify code
From: Jiang Liu @ 2013-05-31 4:31 UTC (permalink / raw)
To: Andrew Morton
Cc: Jiang Liu, David Rientjes, Wen Congyang, Mel Gorman, Minchan Kim,
KAMEZAWA Hiroyuki, Michal Hocko, James Bottomley, Sergei Shtylyov,
David Howells, Mark Salter, Jianguo Wu, linux-mm, linux-arch,
linux-kernel, Florian Tobias Schandinat, linux-fbdev
In-Reply-To: <20130530145844.902b3a947c1f7430c1c2ecf5@linux-foundation.org>
On Fri 31 May 2013 05:58:44 AM CST, Andrew Morton wrote:
> On Sun, 26 May 2013 21:38:34 +0800 Jiang Liu <liuj97@gmail.com> wrote:
>
>> Use common help function free_reserved_area() to simplify code.
>
> http://ozlabs.org/~akpm/mmots/broken-out/drivers-video-acornfbc-remove-dead-code.patch
> removes all the code which your patch alters.
Thanks Andrew, please just drop that patch.
^ permalink raw reply
* Re: [PATCH v3 3/9] drm: Update drm_addmap and drm_mmap to use PAT WC instead of MTRRs
From: Dave Airlie @ 2013-05-31 3:47 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-kernel, dri-devel, linux-fbdev, Daniel Vetter,
Jerome Glisse, Alex Deucher
In-Reply-To: <b1c6c2652b302431a4d4cff46ac872db8e520b6a.1368485053.git.luto@amacapital.net>
On Tue, May 14, 2013 at 9:58 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> Previously, DRM_FRAME_BUFFER mappings, as well as DRM_REGISTERS
> mappings with DRM_WRITE_COMBINING set, resulted in an unconditional
> MTRR being added but the actual mappings being created as UC-.
>
> Now these mappings have the MTRR added only if needed, but they will
> be mapped with pgprot_writecombine.
>
> The non-WC DRM_REGISTERS case now uses pgprot_noncached instead of
> hardcoding the bit twiddling.
>
> The DRM_AGP case is unchanged for now.
Just FYI this breaks on powerpc build, I've fixed it up and pushed the
fixed version to
drm-next-staging, I'll push that to drm-next in a couple of days, once
kbuild robot hits it.
Dave.
^ permalink raw reply
* Re: [PATCH] fbdev: bfin-lq035q1-fb: Use dev_pm_ops
From: Jingoo Han @ 2013-05-31 3:32 UTC (permalink / raw)
To: 'Jean-Christophe PLAGNIOL-VILLARD',
'Rafael J. Wysocki'
Cc: 'Lars-Peter Clausen', 'Michael Hennerich',
'Tomi Valkeinen', linux-fbdev, linux-pm, Jingoo Han
In-Reply-To: <20130530104535.GF19468@game.jcrosoft.org>
On Thursday, May 30, 2013 7:46 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 09:52 Thu 30 May , Lars-Peter Clausen wrote:
> > On 05/30/2013 09:14 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 09:32 Thu 30 May , Jingoo Han wrote:
> > >> On Thursday, May 30, 2013 4:20 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > >>> On 16:24 Wed 29 May , Michael Hennerich wrote:
> > >>>> On 05/29/2013 02:17 PM, Lars-Peter Clausen wrote:
> > >>>>> Use dev_pm_ops instead of the legacy suspend/resume callbacks.
> > >>>>>
> > >>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > >>>> Acked-by: Michael Hennerich <michael.hennerich@analog.com>
> > >>>>> ---
> > >>>>> drivers/video/bfin-lq035q1-fb.c | 22 ++++++++++++++--------
> > >>>>> 1 file changed, 14 insertions(+), 8 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/video/bfin-lq035q1-fb.c b/drivers/video/bfin-lq035q1-fb.c
> > >>>>> index 29d8c04..6084c17 100644
> > >>>>> --- a/drivers/video/bfin-lq035q1-fb.c
> > >>>>> +++ b/drivers/video/bfin-lq035q1-fb.c
> > >>>>> @@ -170,16 +170,19 @@ static int lq035q1_spidev_remove(struct spi_device *spi)
> > >>>>> return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_SHUT);
> > >>>>> }
> > >>>>> -#ifdef CONFIG_PM
> > >>>>> -static int lq035q1_spidev_suspend(struct spi_device *spi, pm_message_t state)
> > >>>>> +#ifdef CONFIG_PM_SLEEP
> > >>>>> +static int lq035q1_spidev_suspend(struct device *dev)
> > >>>>> {
> > >>>>> + struct spi_device *spi = to_spi_device(dev);
> > >>>>> +
> > >>>>> return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_SHUT);
> > >>>>> }
> > >>>>> -static int lq035q1_spidev_resume(struct spi_device *spi)
> > >>>>> +static int lq035q1_spidev_resume(struct device *dev)
> > >>>>> {
> > >>>>> - int ret;
> > >>>>> + struct spi_device *spi = to_spi_device(dev);
> > >>>>> struct spi_control *ctl = spi_get_drvdata(spi);
> > >>>>> + int ret;
> > >>>>> ret = lq035q1_control(spi, LQ035_DRIVER_OUTPUT_CTL, ctl->mode);
> > >>>>> if (ret)
> > >>>>> @@ -187,9 +190,13 @@ static int lq035q1_spidev_resume(struct spi_device *spi)
> > >>>>> return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_ON);
> > >>>>> }
> > >>>>> +
> > >>>>> +static SIMPLE_DEV_PM_OPS(lq035q1_spidev_pm_ops, lq035q1_spidev_suspend,
> > >>>>> + lq035q1_spidev_resume);
> > >>>>> +#define LQ035Q1_SPIDEV_PM_OPS (&lq035q1_spidev_pm_ops)
> > >>>>> +
> > >>>>> #else
> > >>>>> -# define lq035q1_spidev_suspend NULL
> > >>>>> -# define lq035q1_spidev_resume NULL
> > >>>>> +#define LQ035Q1_SPIDEV_PM_OPS NULL
> > >>>>> #endif
> > >>> we really need to ahve a macro like for DT of_match_ptr to drop the #else
> > >>
> > >> Hi Jean-Christophe PLAGNIOL-VILLARD,
> > >>
> > >> I submitted the following patch. :)
> > >> (https://patchwork.kernel.org/patch/2502971/)
> > >>
> > >> --- a/include/linux/pm.h
> > >> +++ b/include/linux/pm.h
> > >> @@ -55,8 +55,10 @@ struct device;
> > >>
> > >> #ifdef CONFIG_PM
> > >> extern const char power_group_name[]; /* = "power" */
> > >> +#define pm_ops_ptr(_ptr) (_ptr)
> > >> #else
> > >> #define power_group_name NULL
> > >> +#define pm_ops_ptr(_ptr) NULL
> > >> #endif
> > >>
> > >>
> > >> This patch was accepted by Rafael Wysocki, and will be merged to v3.11-rc1.
> > >>
> > > Lars-Peter please update with and
> >
> > Since the code depends on CONFIG_PM_SLEEP and not CONFIG_PM I don't think
> > the macro will work.
>
> se please ad a similar macros too
Hi Jean-Christophe,
I added pm_sleep_ops_ptr() as below.
Maybe you want it.
There is no build warnings below 4 config situations.
a) CONFIG_PM is enabled.
b) only CONFIG_PM_SLEEP is enabled
c) only CONFIG_PM_RUNTIME is enabled
d) CONFIG_PM is NOT enabled.
--- a/drivers/video/bfin-lq035q1-fb.c
+++ b/drivers/video/bfin-lq035q1-fb.c
@@ -170,16 +170,19 @@ static int lq035q1_spidev_remove(struct spi_device *spi)
return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_SHUT);
}
-#ifdef CONFIG_PM
-static int lq035q1_spidev_suspend(struct spi_device *spi, pm_message_t state)
+#ifdef CONFIG_PM_SLEEP
+static int lq035q1_spidev_suspend(struct device *dev)
{
+ struct spi_device *spi = to_spi_device(dev);
+
return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_SHUT);
}
-static int lq035q1_spidev_resume(struct spi_device *spi)
+static int lq035q1_spidev_resume(struct device *dev)
{
- int ret;
+ struct spi_device *spi = to_spi_device(dev);
struct spi_control *ctl = spi_get_drvdata(spi);
+ int ret;
ret = lq035q1_control(spi, LQ035_DRIVER_OUTPUT_CTL, ctl->mode);
if (ret)
@@ -187,11 +190,11 @@ static int lq035q1_spidev_resume(struct spi_device *spi)
return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_ON);
}
-#else
-# define lq035q1_spidev_suspend NULL
-# define lq035q1_spidev_resume NULL
#endif
+static SIMPLE_DEV_PM_OPS(lq035q1_spidev_pm_ops, lq035q1_spidev_suspend,
+ lq035q1_spidev_resume);
+
/* Power down all displays on reboot, poweroff or halt */
static void lq035q1_spidev_shutdown(struct spi_device *spi)
{
@@ -708,8 +711,7 @@ static int bfin_lq035q1_probe(struct platform_device *pdev)
info->spidrv.probe = lq035q1_spidev_probe;
info->spidrv.remove = lq035q1_spidev_remove;
info->spidrv.shutdown = lq035q1_spidev_shutdown;
- info->spidrv.suspend = lq035q1_spidev_suspend;
- info->spidrv.resume = lq035q1_spidev_resume;
+ info->spidrv.driver.pm = pm_sleep_ops_ptr(&lq035q1_spidev_pm_ops);
ret = spi_register_driver(&info->spidrv);
if (ret < 0) {
diff --git a/include/linux/pm.h b/include/linux/pm.h
index bd50d15..999d652 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -61,6 +61,12 @@ extern const char power_group_name[]; /* = "power" */
#define pm_ops_ptr(_ptr) NULL
#endif
+#ifdef CONFIG_PM_SLEEP
+#define pm_sleep_ops_ptr(_ptr) (_ptr)
+#else
+#define pm_sleep_ops_ptr(_ptr) NULL
+#endif
+
Hi Rafael J. Wysocki,
How about adding this pm_sleep_ops_ptr() macro?
My draft idea is below:
However, I want other's better ideas. :)
1. The case that only CONFIG_PM_SLEEP is necessary.
#ifdef CONFIG_PM_SLEEP
static int *_suspend(struct device *dev)
[....]
static int *_resume(struct device *dev)
[....]
#endif
static SIMPLE_DEV_PM_OPS(*_pm_ops, *_suspend, *_resume);
static struct {platform/spi/i2c/etc}_driver *_driver = {
.driver = {
.pm = pm_sleep_ops_ptr(&*_pm_ops),
2. The case that both CONFIG_PM_SLEEP & CONFIG_PM_RUNTIME
are necessary.
#ifdef CONFIG_PM_SLEEP
static int *_suspend(struct device *dev)
[....]
static int *_resume(struct device *dev)
[....]
#endif
#ifdef CONFIG_PM_RUNTIME
static int *_runtime_suspend(struct device *dev)
[....]
static int *_runtime_resume(struct device *dev)
[....]
#endif
static const struct dev_pm_ops *_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(*_suspend, *_resume)
SET_RUNTIME_PM_OPS(*_runtime_suspend, *_runtime_resume, NULL)
};
static struct {platform/spi/i2c/etc}_driver *_driver = {
.driver = {
.pm = pm_ops_ptr(&*_pm_ops),
Best regards,
Jingoo Han
^ permalink raw reply related
* Re: [PATCH v3 0/9] Clean up write-combining MTRR addition
From: Dave Airlie @ 2013-05-31 3:16 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-kernel, dri-devel, linux-fbdev, Daniel Vetter,
Jerome Glisse, Alex Deucher
In-Reply-To: <CALCETrXd=6aJwQdDL+XU9x2Pnh0V_9120iw1NHaguN1ahRhhXQ@mail.gmail.com>
On Fri, May 24, 2013 at 4:35 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, May 13, 2013 at 4:58 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> A fair number of drivers (mostly graphics) add write-combining MTRRs.
>> Most ignore errors and most add the MTRR even on PAT systems which don't
>> need to use MTRRs.
>>
>> This series adds new functions arch_phys_wc_{add,del} that, on PAT-less
>> x86 systems with MTRRs, add MTRRs and report errors, and that do nothing
>> otherwise. (Other architectures, if any, with a similar mechanism could
>> implement them.)
>
> That's the path to upstream for this? Should it go through drm-next?
> (Sorry for possible noob question -- I've never sent in anything other
> than trivial fixes to drm stuff before.)
I've pulled the v3 series into drm-next, lets see how they go for a while,
I suppose I should try and boot an AGP box with them.
Dave.
^ permalink raw reply
* Re: [PATCH v2 2/3] video: xilinxfb: Do not use out_be32 IO function
From: Timur Tabi @ 2013-05-31 1:16 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Michal Simek, lkml, Michal Simek, Florian Tobias Schandinat,
linux-fbdev@vger.kernel.org
In-Reply-To: <3808365.SOUxDqkW9J@wuerfel>
On Thu, May 30, 2013 at 5:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> This is probably missing barriers, and is wrong on systems on which
> the endianess of the device is different from the CPU.
I suggest what was done in fsl_ssi.c:
#ifdef PPC
#define read_ssi(addr) in_be32(addr)
#define write_ssi(val, addr) out_be32(addr, val)
#define write_ssi_mask(addr, clear, set) clrsetbits_be32(addr, clear, set)
#elif defined ARM
#define read_ssi(addr) readl(addr)
#define write_ssi(val, addr) writel(val, addr)
/*
* FIXME: Proper locking should be added at write_ssi_mask caller level
* to ensure this register read/modify/write sequence is race free.
*/
static inline void write_ssi_mask(u32 __iomem *addr, u32 clear, u32 set)
{
u32 val = readl(addr);
val = (val & ~clear) | set;
writel(val, addr);
}
#endif
^ permalink raw reply
* Re: [PATCH v2 2/3] video: xilinxfb: Do not use out_be32 IO function
From: Arnd Bergmann @ 2013-05-30 22:04 UTC (permalink / raw)
To: Michal Simek
Cc: linux-kernel, Michal Simek, Florian Tobias Schandinat,
linux-fbdev
In-Reply-To: <a15de155a3f32fc1416833df1b281db05d347541.1369906849.git.michal.simek@xilinx.com>
On Thursday 30 May 2013 11:41:01 Michal Simek wrote:
> * To perform the read/write on the registers we need to check on
> * which bus its connected and call the appropriate write API.
> */
> -static void xilinx_fb_out_be32(struct xilinxfb_drvdata *drvdata, u32 offset,
> +static void xilinx_fb_out32(struct xilinxfb_drvdata *drvdata, u32 offset,
> u32 val)
> {
> if (drvdata->flags & PLB_ACCESS_FLAG)
> - out_be32(drvdata->regs + (offset << 2), val);
> + __raw_writel(val, drvdata->regs + (offset << 2));
> #ifdef CONFIG_PPC_DCR
> else
> dcr_write(drvdata->dcr_host, offset, val);
>
This is probably missing barriers, and is wrong on systems on which
the endianess of the device is different from the CPU.
You already have an indirection in there, so I guess it won't hurt
to create a third case for little-endian registers and add
another bit in drvdata->flags, or make it depend on the architecture,
if the endianess of the device registers is known at compile time.
Arnd
^ permalink raw reply
* Re: [PATCH v8, part3 06/14] mm, acornfb: use free_reserved_area() to simplify code
From: Andrew Morton @ 2013-05-30 21:58 UTC (permalink / raw)
To: Jiang Liu
Cc: Jiang Liu, David Rientjes, Wen Congyang, Mel Gorman, Minchan Kim,
KAMEZAWA Hiroyuki, Michal Hocko, James Bottomley, Sergei Shtylyov,
David Howells, Mark Salter, Jianguo Wu, linux-mm, linux-arch,
linux-kernel, Florian Tobias Schandinat, linux-fbdev
In-Reply-To: <1369575522-26405-7-git-send-email-jiang.liu@huawei.com>
On Sun, 26 May 2013 21:38:34 +0800 Jiang Liu <liuj97@gmail.com> wrote:
> Use common help function free_reserved_area() to simplify code.
http://ozlabs.org/~akpm/mmots/broken-out/drivers-video-acornfbc-remove-dead-code.patch
removes all the code which your patch alters.
^ permalink raw reply
* [RESEND PATCH 1/1] MAINTAINERS: Framebuffer Layer maintainers update
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-05-30 20:23 UTC (permalink / raw)
To: linux-kernel
Cc: Jean-Christophe PLAGNIOL-VILLARD, Tomi Valkeinen, Olof Johansson,
Andrew Morton, Linus Torvalds, Arnd Bergmann,
Florian Tobias Schandinat, linux-fbdev
Tomi and I will now take care of the Framebuffer Layer
The git tree is now on kernel.org
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Olof Johansson <olof@lixom.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: linux-fbdev@vger.kernel.org
---
Hi,
resend as git send-email drop my name in the From
Best Regards,
J.
MAINTAINERS | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index fd3a495..7714c3c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3322,11 +3322,12 @@ F: drivers/net/wan/dlci.c
F: drivers/net/wan/sdla.c
FRAMEBUFFER LAYER
-M: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
+M: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
+M: Tomi Valkeinen <tomi.valkeinen@ti.com>
L: linux-fbdev@vger.kernel.org
W: http://linux-fbdev.sourceforge.net/
Q: http://patchwork.kernel.org/project/linux-fbdev/list/
-T: git git://github.com/schandinat/linux-2.6.git fbdev-next
+T: git git://git.kernel.org/pub/scm/linux/kernel/git/plagnioj/linux-fbdev.git
S: Maintained
F: Documentation/fb/
F: Documentation/devicetree/bindings/fb/
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH 05/32] OMAPDSS: fix dss_get_ctx_loss_count for DT
From: Tomi Valkeinen @ 2013-05-30 17:21 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <874ndk5sse.fsf@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 2534 bytes --]
On 30/05/13 19:36, Kevin Hilman wrote:
> Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
>
>> When using DT, dss device does not have platform data. However,
>> dss_get_ctx_loss_count() uses dss device's platform data to find the
>> get_ctx_loss_count function pointer.
>>
>> To fix this, dss_get_ctx_loss_count() needs to be changed to get the
>> platform data from the omapdss device, which is a "virtual" device and
>> always has platform data.
>
> Dumb question (since the DSS device model has always been beyond my
> grasp): how/why does the virtual device still have platform_data on a DT
> boot?
The virtual device will be created in generic omap arch code for DT boot
also.
The omapdss virtual device is an old relic. Originally we only had the
omapdss device, not the sub-devices for HW block like we have now. After
adding the sub-devices, omapdss device was still left, as it provided
useful functionality. It wasn't strictly needed anymore, but I've just
never had time to start refactoring code to remove it.
Now, with DT, it was just an easy way around to pass the func pointers.
Note that we also pass set_min_bus_tput, and a version identifier for
the DSS HW. The PM funcs could perhaps be handled some other way, but
I'm not sure how I could pass the DSS HW version.
> Also, this context_loss_count and DT boot is starting to get cumbersome,
> and there's currently no (good) way of handling the context loss in a
> generic way without pdata function pointers.
>
> I'm starting to think we should move towards dropping this OMAP-specific
> context loss counting, and just assume context is always lost. If there
> are performance problems, runtime PM autosuspend timeouts can be used to
> avoid the transitions.
>
> Or, on some devices, I suspect comparing a few registers against their
> power-on reset values might be a quicker way of detecting lost context
> and would avoid having to call into platform code all together.
Hmm, true. I'll look at this. Maybe I won't need get_ctx_loss in omapdss
at all.
How about omap_pm_set_min_bus_tput(), can that be handled in some
generic manner? Although, we currently use that in a bit hacky case.
What we're doing is not really setting min bus tput, but we're just
preventing OPP50.
The DSS clocks have different maximums on OPP50 than on OPP100, and we
need to keep the core in OPP100 to keep DSS working. So what I'd rather
use is "prevent_opp50()" than setting arbitrarily high min bus tput.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 03/32] OMAPDSS: add omap_dss_find_output()
From: Tomi Valkeinen @ 2013-05-30 16:54 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <20130530154055.GL19468@game.jcrosoft.org>
[-- Attachment #1: Type: text/plain, Size: 2266 bytes --]
On 30/05/13 18:40, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 14:40 Thu 30 May , Tomi Valkeinen wrote:
>> On 30/05/13 14:07, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> On 12:34 Thu 30 May , Tomi Valkeinen wrote:
>>>> Add a support function to find a DSS output by given name. This is used
>>>> in later patches to link the panels to DSS outputs.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> ---
>>>> drivers/video/omap2/dss/output.c | 13 +++++++++++++
>>>> include/video/omapdss.h | 1 +
>>>> 2 files changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/video/omap2/dss/output.c b/drivers/video/omap2/dss/output.c
>>>> index 5214df6..3274628 100644
>>>> --- a/drivers/video/omap2/dss/output.c
>>>> +++ b/drivers/video/omap2/dss/output.c
>>>> @@ -115,6 +115,19 @@ struct omap_dss_output *omap_dss_get_output(enum omap_dss_output_id id)
>>>> }
>>>> EXPORT_SYMBOL(omap_dss_get_output);
>>> GPL please
>>
>> The omapdss driver uses EXPORT_SYMBOL. I don't want to start mixing both
>> EXPORT_SYMBOLs and EXPORT_SYMBOL_GPLs.
>
> I do not like EXPORT_SYMBOL at all
> I stringly prefer to switch all of them to _GPL
>
> but will not refuse the patch for this in this case
I have nothing against changing omapdss to use _GPL only. I've never
heard anyone using non-GPL panel drivers with omap.
Following these patch sets I can remove lots of the old code, which
contains the majority of the EXPORT_SYMBOLs in omapdss. I'll change
omapdss to use _GPL after that removal.
>>>> +struct omap_dss_output *omap_dss_find_output(const char *name)
>>>> +{
>>>> + struct omap_dss_output *out;
>>>> +
>>>> + list_for_each_entry(out, &output_list, list) {
>>>> + if (strcmp(out->name, name) == 0)
>>>> + return out;
>>>> + }
>>>> +
>>>> + return NULL;
>>>> +}
>>> I this in so many drivers could we have a macro to generate such function?
>>
>> What would that help? Wouldn't it just increase the code size of the kernel?
>
> increase no as it's not an inline function but a macro to generate the
> function
> after help yes but people may not like so as you wish
I don't think I understand what you mean then. What kind of macro are
you talking about?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 05/32] OMAPDSS: fix dss_get_ctx_loss_count for DT
From: Kevin Hilman @ 2013-05-30 16:36 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1369906493-27538-6-git-send-email-tomi.valkeinen@ti.com>
Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
> When using DT, dss device does not have platform data. However,
> dss_get_ctx_loss_count() uses dss device's platform data to find the
> get_ctx_loss_count function pointer.
>
> To fix this, dss_get_ctx_loss_count() needs to be changed to get the
> platform data from the omapdss device, which is a "virtual" device and
> always has platform data.
Dumb question (since the DSS device model has always been beyond my
grasp): how/why does the virtual device still have platform_data on a DT
boot?
Also, this context_loss_count and DT boot is starting to get cumbersome,
and there's currently no (good) way of handling the context loss in a
generic way without pdata function pointers.
I'm starting to think we should move towards dropping this OMAP-specific
context loss counting, and just assume context is always lost. If there
are performance problems, runtime PM autosuspend timeouts can be used to
avoid the transitions.
Or, on some devices, I suspect comparing a few registers against their
power-on reset values might be a quicker way of detecting lost context
and would avoid having to call into platform code all together.
Kevin
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> drivers/video/omap2/dss/dss.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
> index 94f66f9..bd01608 100644
> --- a/drivers/video/omap2/dss/dss.c
> +++ b/drivers/video/omap2/dss/dss.c
> @@ -157,7 +157,8 @@ static void dss_restore_context(void)
>
> int dss_get_ctx_loss_count(void)
> {
> - struct omap_dss_board_info *board_data = dss.pdev->dev.platform_data;
> + struct platform_device *core_pdev = dss_get_core_pdev();
> + struct omap_dss_board_info *board_data = core_pdev->dev.platform_data;
> int cnt;
>
> if (!board_data->get_context_loss_count)
^ permalink raw reply
* Re: [PATCH] fbdev: bfin-lq035q1-fb: Use dev_pm_ops
From: Lars-Peter Clausen @ 2013-05-30 16:32 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: Jingoo Han, 'Michael Hennerich',
'Rafael J. Wysocki', 'Tomi Valkeinen',
linux-fbdev, linux-pm
In-Reply-To: <20130530104535.GF19468@game.jcrosoft.org>
On 05/30/2013 12:45 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 09:52 Thu 30 May , Lars-Peter Clausen wrote:
>> On 05/30/2013 09:14 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> On 09:32 Thu 30 May , Jingoo Han wrote:
>>>> On Thursday, May 30, 2013 4:20 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>> On 16:24 Wed 29 May , Michael Hennerich wrote:
>>>>>> On 05/29/2013 02:17 PM, Lars-Peter Clausen wrote:
>>>>>>> Use dev_pm_ops instead of the legacy suspend/resume callbacks.
>>>>>>>
>>>>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>>>> Acked-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>> ---
>>>>>>> drivers/video/bfin-lq035q1-fb.c | 22 ++++++++++++++--------
>>>>>>> 1 file changed, 14 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/video/bfin-lq035q1-fb.c b/drivers/video/bfin-lq035q1-fb.c
>>>>>>> index 29d8c04..6084c17 100644
>>>>>>> --- a/drivers/video/bfin-lq035q1-fb.c
>>>>>>> +++ b/drivers/video/bfin-lq035q1-fb.c
>>>>>>> @@ -170,16 +170,19 @@ static int lq035q1_spidev_remove(struct spi_device *spi)
>>>>>>> return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_SHUT);
>>>>>>> }
>>>>>>> -#ifdef CONFIG_PM
>>>>>>> -static int lq035q1_spidev_suspend(struct spi_device *spi, pm_message_t state)
>>>>>>> +#ifdef CONFIG_PM_SLEEP
>>>>>>> +static int lq035q1_spidev_suspend(struct device *dev)
>>>>>>> {
>>>>>>> + struct spi_device *spi = to_spi_device(dev);
>>>>>>> +
>>>>>>> return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_SHUT);
>>>>>>> }
>>>>>>> -static int lq035q1_spidev_resume(struct spi_device *spi)
>>>>>>> +static int lq035q1_spidev_resume(struct device *dev)
>>>>>>> {
>>>>>>> - int ret;
>>>>>>> + struct spi_device *spi = to_spi_device(dev);
>>>>>>> struct spi_control *ctl = spi_get_drvdata(spi);
>>>>>>> + int ret;
>>>>>>> ret = lq035q1_control(spi, LQ035_DRIVER_OUTPUT_CTL, ctl->mode);
>>>>>>> if (ret)
>>>>>>> @@ -187,9 +190,13 @@ static int lq035q1_spidev_resume(struct spi_device *spi)
>>>>>>> return lq035q1_control(spi, LQ035_SHUT_CTL, LQ035_ON);
>>>>>>> }
>>>>>>> +
>>>>>>> +static SIMPLE_DEV_PM_OPS(lq035q1_spidev_pm_ops, lq035q1_spidev_suspend,
>>>>>>> + lq035q1_spidev_resume);
>>>>>>> +#define LQ035Q1_SPIDEV_PM_OPS (&lq035q1_spidev_pm_ops)
>>>>>>> +
>>>>>>> #else
>>>>>>> -# define lq035q1_spidev_suspend NULL
>>>>>>> -# define lq035q1_spidev_resume NULL
>>>>>>> +#define LQ035Q1_SPIDEV_PM_OPS NULL
>>>>>>> #endif
>>>>> we really need to ahve a macro like for DT of_match_ptr to drop the #else
>>>>
>>>> Hi Jean-Christophe PLAGNIOL-VILLARD,
>>>>
>>>> I submitted the following patch. :)
>>>> (https://patchwork.kernel.org/patch/2502971/)
>>>>
>>>> --- a/include/linux/pm.h
>>>> +++ b/include/linux/pm.h
>>>> @@ -55,8 +55,10 @@ struct device;
>>>>
>>>> #ifdef CONFIG_PM
>>>> extern const char power_group_name[]; /* = "power" */
>>>> +#define pm_ops_ptr(_ptr) (_ptr)
>>>> #else
>>>> #define power_group_name NULL
>>>> +#define pm_ops_ptr(_ptr) NULL
>>>> #endif
>>>>
>>>>
>>>> This patch was accepted by Rafael Wysocki, and will be merged to v3.11-rc1.
>>>>
>>> Lars-Peter please update with and
>>
>> Since the code depends on CONFIG_PM_SLEEP and not CONFIG_PM I don't think
>> the macro will work.
>
> se please ad a similar macros too
I'm not really convinced that this *_ops_ptr thing is the right approach.
Besides that the scope of this patch is to convert the driver from legacy
suspend callbacks to dev_pm_ops not to remove the #ifdef's.
- Lars
^ permalink raw reply
* Re: [PATCH 05/32] OMAPDSS: fix dss_get_ctx_loss_count for DT
From: Tomi Valkeinen @ 2013-05-30 16:14 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <20130530154355.GM19468@game.jcrosoft.org>
[-- Attachment #1: Type: text/plain, Size: 3270 bytes --]
On 30/05/13 18:43, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 14:28 Thu 30 May , Tomi Valkeinen wrote:
>> On 30/05/13 14:09, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> On 12:34 Thu 30 May , Tomi Valkeinen wrote:
>>>> When using DT, dss device does not have platform data. However,
>>>> dss_get_ctx_loss_count() uses dss device's platform data to find the
>>>> get_ctx_loss_count function pointer.
>>>>
>>>> To fix this, dss_get_ctx_loss_count() needs to be changed to get the
>>>> platform data from the omapdss device, which is a "virtual" device and
>>>> always has platform data.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> ---
>>>> drivers/video/omap2/dss/dss.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
>>>> index 94f66f9..bd01608 100644
>>>> --- a/drivers/video/omap2/dss/dss.c
>>>> +++ b/drivers/video/omap2/dss/dss.c
>>>> @@ -157,7 +157,8 @@ static void dss_restore_context(void)
>>>>
>>>> int dss_get_ctx_loss_count(void)
>>>> {
>>>> - struct omap_dss_board_info *board_data = dss.pdev->dev.platform_data;
>>>> + struct platform_device *core_pdev = dss_get_core_pdev();
>>>> + struct omap_dss_board_info *board_data = core_pdev->dev.platform_data;
>>>
>>> how about store the pdata in the drivers internal struct and if !dt
>>> you ust do this
>>>
>>> dss_dev->pdata = *pdev->dev.platform_data;
>>>
>>> to copy it and we do not alloc it for dt
>>
>> It's not quite that simple. We need some OMAP arch functions (like
>> get_ctx_loss_count here) that are not currently exposed via any other
>> method to drivers except passing a function pointer with platform data.
>> We need that also when booting with DT.
>>
>> We have a bunch of devices for the display subsystem hardware blocks,
>> like the "dss" here. When booting with DT, these blocks are represented
>> in the DT data, and do not have platform data.
>>
>> We also have a "virtual" device, "omapdss", which doesn't match any hw
>> block. It's created in the arch setup stage. It's really a legacy thing,
>> but with DT it can be used conveniently to pass the platform data.
>>
>> The problem this patch fixes is that we used to pass the arch functions
>> for each of those HW block drivers. But with DT, we need to get the arch
>> functions from the "omapdss" device, gotten with dss_get_core_pdev().
>
> ok
>
> do not take it bad is it worth the effort those 54 patches?
>
> is not better to work on DRM?
> just an open question
Both omapfb and omapdrm use omapdss. omapdss provides the HW level
support, and also panel support. At some point in the future we'll
probably deprecate omapfb, and move wholly to omapdrm. At that point
omapdss and omapdrm can be merged together, simplifying the design.
And regarding the amount of the patches, there has been some bad design
decisions in the history of omapdss, and as it's a big driver (plus the
panel drivers), it takes quite a bit to fix these. There will be more
coming to convert the rest of the panel drivers, and also to implement
DT support. But those all also work for omapdrm, so it's not fbdev-only
stuff.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 06/32] OMAPDSS: DPI: fix regulators for DT
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-05-30 15:54 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Tomi Valkeinen, Grant Likely, Linus Walleij, gregkh, linux-fbdev,
linux-omap, Archit Taneja
In-Reply-To: <201305301505.00529.arnd@arndb.de>
On 15:05 Thu 30 May , Arnd Bergmann wrote:
> On Thursday 30 May 2013, Tomi Valkeinen wrote:
> > On 30/05/13 14:12, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 12:34 Thu 30 May , Tomi Valkeinen wrote:
> > >> On some platforms DPI requires a regulator to be enabled to power up the
> > >> output pins. This regulator is, for some reason, currently attached to
> > >> the virtual omapdss device, instead of the DPI device. This does not
> > >> work for DT, as the regulator mappings need to be described in the DT
> > >> data, and the virtual omapdss device is not present there.
> > >>
> > >> Fix the issue by acquiring the regulator in the DPI device. To retain
> > >> compatibility with the current board files, the old method of getting
> > >> the regulator is kept. The old method can be removed when the board
> > >> files have been changed to pass the regulator to DPI.
> > >
> > > as discuss with Arnd we should handle regular enable and disable at device
> > > probe for every device as we do for pinctrl
> >
> > I'm not sure what you mean. Enable of what? The regulator? Why would we
> > enable it in the device's probe, as the device may never even be used?
>
> It's an idea I had a while ago, but not yet discussed in the open.
>
> Jean-Christophe just posted patches to move the mapping of interrupt numbers
> into platform_drv_probe(), just before calling the driver ->probe() callback,
> and we already have similar code to set up the default pinctrl state of
> a device before calling probe().
>
> This can be extended to further subsystems, but that has to be done
> carefully to avoid regressions. Ideally we would move a lot of boilerplate
> code out of the driver specific ->probe() function into common code.
> Possible candidates for this include:
>
> * calling devm_request_mem_region for the "reg" property
> * calling devm_ioremap on the "reg" property"
> * calling devm_gpio_request for all gpio lines
> * calling devm_regulator_get on all regulators
> * calling devm_reset_control_get on all reset lines
> * calling devm_dma_request_slave_channel on all dma channels
> * calling devm_of_pwm_get for all pwm channels
> * ...
>
> For most of these (maybe all), I think we need some form of opt-in
> model on the driver side because there are cases where aquiring some
> of these resources is not mandatory, and it only works if the driver
> is using DT probing.
>
> IF we want to do this, it also needs a lot of thought, and we shouldn't
> do it carelessly. We might also need some extra infrastructure in revres
> to simplify access to the resources we got from the OF node.
>
> The irq resources are particularly trivial because we already claim
> them in of_platform_populate, so moving that to platform_drv_probe()
> is straightforward and solves existing bugs without creating a huge
> regression potential, but it's harder for the others.
Yeah I agree with Arnd
we need to start to move this way but for DT only first and carefully
Best Regards,
J.
>
> Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 05/32] OMAPDSS: fix dss_get_ctx_loss_count for DT
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-05-30 15:43 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <51A737DC.1010900@ti.com>
On 14:28 Thu 30 May , Tomi Valkeinen wrote:
> On 30/05/13 14:09, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 12:34 Thu 30 May , Tomi Valkeinen wrote:
> >> When using DT, dss device does not have platform data. However,
> >> dss_get_ctx_loss_count() uses dss device's platform data to find the
> >> get_ctx_loss_count function pointer.
> >>
> >> To fix this, dss_get_ctx_loss_count() needs to be changed to get the
> >> platform data from the omapdss device, which is a "virtual" device and
> >> always has platform data.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >> drivers/video/omap2/dss/dss.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
> >> index 94f66f9..bd01608 100644
> >> --- a/drivers/video/omap2/dss/dss.c
> >> +++ b/drivers/video/omap2/dss/dss.c
> >> @@ -157,7 +157,8 @@ static void dss_restore_context(void)
> >>
> >> int dss_get_ctx_loss_count(void)
> >> {
> >> - struct omap_dss_board_info *board_data = dss.pdev->dev.platform_data;
> >> + struct platform_device *core_pdev = dss_get_core_pdev();
> >> + struct omap_dss_board_info *board_data = core_pdev->dev.platform_data;
> >
> > how about store the pdata in the drivers internal struct and if !dt
> > you ust do this
> >
> > dss_dev->pdata = *pdev->dev.platform_data;
> >
> > to copy it and we do not alloc it for dt
>
> It's not quite that simple. We need some OMAP arch functions (like
> get_ctx_loss_count here) that are not currently exposed via any other
> method to drivers except passing a function pointer with platform data.
> We need that also when booting with DT.
>
> We have a bunch of devices for the display subsystem hardware blocks,
> like the "dss" here. When booting with DT, these blocks are represented
> in the DT data, and do not have platform data.
>
> We also have a "virtual" device, "omapdss", which doesn't match any hw
> block. It's created in the arch setup stage. It's really a legacy thing,
> but with DT it can be used conveniently to pass the platform data.
>
> The problem this patch fixes is that we used to pass the arch functions
> for each of those HW block drivers. But with DT, we need to get the arch
> functions from the "omapdss" device, gotten with dss_get_core_pdev().
ok
do not take it bad is it worth the effort those 54 patches?
is not better to work on DRM?
just an open question
Best Regards,
J.
>
> Tomi
>
>
^ permalink raw reply
* Re: [PATCH 03/32] OMAPDSS: add omap_dss_find_output()
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-05-30 15:40 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <51A73A9A.8010801@ti.com>
On 14:40 Thu 30 May , Tomi Valkeinen wrote:
> On 30/05/13 14:07, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 12:34 Thu 30 May , Tomi Valkeinen wrote:
> >> Add a support function to find a DSS output by given name. This is used
> >> in later patches to link the panels to DSS outputs.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >> drivers/video/omap2/dss/output.c | 13 +++++++++++++
> >> include/video/omapdss.h | 1 +
> >> 2 files changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/video/omap2/dss/output.c b/drivers/video/omap2/dss/output.c
> >> index 5214df6..3274628 100644
> >> --- a/drivers/video/omap2/dss/output.c
> >> +++ b/drivers/video/omap2/dss/output.c
> >> @@ -115,6 +115,19 @@ struct omap_dss_output *omap_dss_get_output(enum omap_dss_output_id id)
> >> }
> >> EXPORT_SYMBOL(omap_dss_get_output);
> > GPL please
>
> The omapdss driver uses EXPORT_SYMBOL. I don't want to start mixing both
> EXPORT_SYMBOLs and EXPORT_SYMBOL_GPLs.
I do not like EXPORT_SYMBOL at all
I stringly prefer to switch all of them to _GPL
but will not refuse the patch for this in this case
>
> >> +struct omap_dss_output *omap_dss_find_output(const char *name)
> >> +{
> >> + struct omap_dss_output *out;
> >> +
> >> + list_for_each_entry(out, &output_list, list) {
> >> + if (strcmp(out->name, name) = 0)
> >> + return out;
> >> + }
> >> +
> >> + return NULL;
> >> +}
> > I this in so many drivers could we have a macro to generate such function?
>
> What would that help? Wouldn't it just increase the code size of the kernel?
increase no as it's not an inline function but a macro to generate the
function
after help yes but people may not like so as you wish
Best Regards,
J.
>
> Tomi
>
>
^ permalink raw reply
* Re: [PATCH 1/8] video: atmel_lcdfb: fix platform data struct
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-05-30 14:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51A761DA.6020109@corscience.de>
On 16:27 Thu 30 May , Andreas Bießmann wrote:
> On 30.05.13 09:23, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 08:39 Thu 30 May , Richard Genoud wrote:
> >> 2013/5/29 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>:
> >>> On 19:44 Wed 29 May , Richard Genoud wrote:
> >>>> 2013/5/29 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>:
> >>>>> On 16:36 Wed 29 May , Richard Genoud wrote:
> >>>>>> 2013/4/11 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>:
> >>>>>>> Today we mix pdata and drivers data in the struct atmel_lcdfb_info
> >>>>>>> Fix it and introduce a new struct atmel_lcdfb_pdata for platform data only
> >>>>>>>
> >>>>>>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> >>>>>>> Cc: linux-fbdev@vger.kernel.org
> >>>>>>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> >>>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
> >>>>>>> Cc: Hans-Christian Egtvedt <egtvedt@samfundet.no>
> >>>>>>> ---
> >>>>>>> arch/arm/mach-at91/at91sam9261_devices.c | 6 +-
> >>>>>>> arch/arm/mach-at91/at91sam9263_devices.c | 6 +-
> >>>>>>> arch/arm/mach-at91/at91sam9g45_devices.c | 6 +-
> >>>>>>> arch/arm/mach-at91/at91sam9rl_devices.c | 6 +-
> >>>>>>> arch/arm/mach-at91/board-sam9261ek.c | 6 +-
> >>>>>>> arch/arm/mach-at91/board-sam9263ek.c | 4 +-
> >>>>>>> arch/arm/mach-at91/board-sam9m10g45ek.c | 4 +-
> >>>>>>> arch/arm/mach-at91/board-sam9rlek.c | 4 +-
> >>>>>>> arch/arm/mach-at91/board.h | 4 +-
> >>>>>>> arch/avr32/boards/atngw100/evklcd10x.c | 6 +-
> >>>>>>> arch/avr32/boards/atngw100/mrmt.c | 4 +-
> >>>>>>> arch/avr32/boards/atstk1000/atstk1000.h | 2 +-
> >>>>>>> arch/avr32/boards/atstk1000/setup.c | 2 +-
> >>>>>>> arch/avr32/boards/favr-32/setup.c | 2 +-
> >>>>>>> arch/avr32/boards/hammerhead/setup.c | 2 +-
> >>>>>>> arch/avr32/boards/merisc/display.c | 2 +-
> >>>>>>> arch/avr32/boards/mimc200/setup.c | 4 +-
> >>>>>>> arch/avr32/mach-at32ap/at32ap700x.c | 8 +--
> >>>>>>> arch/avr32/mach-at32ap/include/mach/board.h | 4 +-
> >>>>>>> drivers/video/atmel_lcdfb.c | 104 +++++++++++++++++----------
> >>>>>>> include/video/atmel_lcdc.h | 24 +------
> >>>>>>> 21 files changed, 109 insertions(+), 101 deletions(-)
> >>>>>>>
> >>>>>> [snip]
> >>>>>>> diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
> >>>>>>> index c1a2914..98733cd4 100644
> >>>>>>> --- a/drivers/video/atmel_lcdfb.c
> >>>>>>> +++ b/drivers/video/atmel_lcdfb.c
> >>>>>>> @@ -20,12 +20,45 @@
> >>>>>>> #include <linux/gfp.h>
> >>>>>>> #include <linux/module.h>
> >>>>>>> #include <linux/platform_data/atmel.h>
> >>>>>>> +#include <video/of_display_timing.h>
> >>>>>>>
> >>>>>>> #include <mach/cpu.h>
> >>>>>>> #include <asm/gpio.h>
> >>>>>>>
> >>>>>>> #include <video/atmel_lcdc.h>
> >>>>>>>
> >>>>>>> +struct atmel_lcdfb_config {
> >>>>>>> + bool have_alt_pixclock;
> >>>>>>> + bool have_hozval;
> >>>>>>> + bool have_intensity_bit;
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> + /* LCD Controller info data structure, stored in device platform_data */
> >>>>>>> +struct atmel_lcdfb_info {
> >>>>>>> + spinlock_t lock;
> >>>>>>> + struct fb_info *info;
> >>>>>>> + void __iomem *mmio;
> >>>>>>> + int irq_base;
> >>>>>>> + struct work_struct task;
> >>>>>>> +
> >>>>>>> + unsigned int smem_len;
> >>>>>>> + struct platform_device *pdev;
> >>>>>>> + struct clk *bus_clk;
> >>>>>>> + struct clk *lcdc_clk;
> >>>>>>> +
> >>>>>>> + struct backlight_device *backlight;
> >>>>>>> + u8 bl_power;
> >>>>>>> + bool lcdcon_pol_negative;
> >>>>>> I think lcdcon_pol_negative should be part of pdata, because it really
> >>>>>> depends on how the PWM is wired on the board.
> >>>>>>
> >>>>>
> >>>>> maybe but no one mainline use it on any pdata for non-dt boars
> >>>>> so I did not want to expose it
> >>>> Well, at least, I'm using it :)
> >>>> (and I guess that Andreas is using it also, otherwise he wouldn't have
> >>>> introduce it !)
> >>>
> >>> yes but pdata is for non-dt boards, for dt you can keep it in struct
> >>> atmel_lcdfb_info and add a property
> >>>
> >>> if non-dt boards want it my answer is I do not care switch to DT
> >>
> >> ok (I use a full DT board based on sam9g35)
> >>
> >> so I'll add something like
> >> sinfo->lcdcon_pol_negative = of_property_read_bool(display_np,
> >> "atmel,lcdcon-pwm-pulse-low");
> >> in /atmel_lcdfb.c
> >>
> >> But I thought the goal of this patch was to separate driver data from
> >> platform specific data, and IMHO, lcdcon_pol_negative is a specificity
> >> of the platform.
> >
> > You are right but as non one mainline use it as pdata I choose to drop it
> > and only keep it on the driver as we can still use it for DT
> >
> > It's a way to force peopoe to switch to DT
>
> well, we use it on an avr32 board (not mainlined; though no kernel
> update planned currently) ... but no way to use DT currently.
If there is 1 ARV32 user it's an other story
Best Regards,
J.
>
> Best regards
>
> Andreas Bießmann
^ permalink raw reply
* Re: [PATCH 1/8] video: atmel_lcdfb: fix platform data struct
From: Andreas Bießmann @ 2013-05-30 14:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130530072328.GE19468@game.jcrosoft.org>
On 30.05.13 09:23, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 08:39 Thu 30 May , Richard Genoud wrote:
>> 2013/5/29 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>:
>>> On 19:44 Wed 29 May , Richard Genoud wrote:
>>>> 2013/5/29 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>:
>>>>> On 16:36 Wed 29 May , Richard Genoud wrote:
>>>>>> 2013/4/11 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>:
>>>>>>> Today we mix pdata and drivers data in the struct atmel_lcdfb_info
>>>>>>> Fix it and introduce a new struct atmel_lcdfb_pdata for platform data only
>>>>>>>
>>>>>>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>>>>>> Cc: linux-fbdev@vger.kernel.org
>>>>>>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>>>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>>>> Cc: Hans-Christian Egtvedt <egtvedt@samfundet.no>
>>>>>>> ---
>>>>>>> arch/arm/mach-at91/at91sam9261_devices.c | 6 +-
>>>>>>> arch/arm/mach-at91/at91sam9263_devices.c | 6 +-
>>>>>>> arch/arm/mach-at91/at91sam9g45_devices.c | 6 +-
>>>>>>> arch/arm/mach-at91/at91sam9rl_devices.c | 6 +-
>>>>>>> arch/arm/mach-at91/board-sam9261ek.c | 6 +-
>>>>>>> arch/arm/mach-at91/board-sam9263ek.c | 4 +-
>>>>>>> arch/arm/mach-at91/board-sam9m10g45ek.c | 4 +-
>>>>>>> arch/arm/mach-at91/board-sam9rlek.c | 4 +-
>>>>>>> arch/arm/mach-at91/board.h | 4 +-
>>>>>>> arch/avr32/boards/atngw100/evklcd10x.c | 6 +-
>>>>>>> arch/avr32/boards/atngw100/mrmt.c | 4 +-
>>>>>>> arch/avr32/boards/atstk1000/atstk1000.h | 2 +-
>>>>>>> arch/avr32/boards/atstk1000/setup.c | 2 +-
>>>>>>> arch/avr32/boards/favr-32/setup.c | 2 +-
>>>>>>> arch/avr32/boards/hammerhead/setup.c | 2 +-
>>>>>>> arch/avr32/boards/merisc/display.c | 2 +-
>>>>>>> arch/avr32/boards/mimc200/setup.c | 4 +-
>>>>>>> arch/avr32/mach-at32ap/at32ap700x.c | 8 +--
>>>>>>> arch/avr32/mach-at32ap/include/mach/board.h | 4 +-
>>>>>>> drivers/video/atmel_lcdfb.c | 104 +++++++++++++++++----------
>>>>>>> include/video/atmel_lcdc.h | 24 +------
>>>>>>> 21 files changed, 109 insertions(+), 101 deletions(-)
>>>>>>>
>>>>>> [snip]
>>>>>>> diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
>>>>>>> index c1a2914..98733cd4 100644
>>>>>>> --- a/drivers/video/atmel_lcdfb.c
>>>>>>> +++ b/drivers/video/atmel_lcdfb.c
>>>>>>> @@ -20,12 +20,45 @@
>>>>>>> #include <linux/gfp.h>
>>>>>>> #include <linux/module.h>
>>>>>>> #include <linux/platform_data/atmel.h>
>>>>>>> +#include <video/of_display_timing.h>
>>>>>>>
>>>>>>> #include <mach/cpu.h>
>>>>>>> #include <asm/gpio.h>
>>>>>>>
>>>>>>> #include <video/atmel_lcdc.h>
>>>>>>>
>>>>>>> +struct atmel_lcdfb_config {
>>>>>>> + bool have_alt_pixclock;
>>>>>>> + bool have_hozval;
>>>>>>> + bool have_intensity_bit;
>>>>>>> +};
>>>>>>> +
>>>>>>> + /* LCD Controller info data structure, stored in device platform_data */
>>>>>>> +struct atmel_lcdfb_info {
>>>>>>> + spinlock_t lock;
>>>>>>> + struct fb_info *info;
>>>>>>> + void __iomem *mmio;
>>>>>>> + int irq_base;
>>>>>>> + struct work_struct task;
>>>>>>> +
>>>>>>> + unsigned int smem_len;
>>>>>>> + struct platform_device *pdev;
>>>>>>> + struct clk *bus_clk;
>>>>>>> + struct clk *lcdc_clk;
>>>>>>> +
>>>>>>> + struct backlight_device *backlight;
>>>>>>> + u8 bl_power;
>>>>>>> + bool lcdcon_pol_negative;
>>>>>> I think lcdcon_pol_negative should be part of pdata, because it really
>>>>>> depends on how the PWM is wired on the board.
>>>>>>
>>>>>
>>>>> maybe but no one mainline use it on any pdata for non-dt boars
>>>>> so I did not want to expose it
>>>> Well, at least, I'm using it :)
>>>> (and I guess that Andreas is using it also, otherwise he wouldn't have
>>>> introduce it !)
>>>
>>> yes but pdata is for non-dt boards, for dt you can keep it in struct
>>> atmel_lcdfb_info and add a property
>>>
>>> if non-dt boards want it my answer is I do not care switch to DT
>>
>> ok (I use a full DT board based on sam9g35)
>>
>> so I'll add something like
>> sinfo->lcdcon_pol_negative = of_property_read_bool(display_np,
>> "atmel,lcdcon-pwm-pulse-low");
>> in /atmel_lcdfb.c
>>
>> But I thought the goal of this patch was to separate driver data from
>> platform specific data, and IMHO, lcdcon_pol_negative is a specificity
>> of the platform.
>
> You are right but as non one mainline use it as pdata I choose to drop it
> and only keep it on the driver as we can still use it for DT
>
> It's a way to force peopoe to switch to DT
well, we use it on an avr32 board (not mainlined; though no kernel
update planned currently) ... but no way to use DT currently.
Best regards
Andreas Bießmann
^ permalink raw reply
* Re: [PATCH 1/8] video: atmel_lcdfb: fix platform data struct
From: Hans-Christian Egtvedt @ 2013-05-30 14:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130530072328.GE19468@game.jcrosoft.org>
Around Thu 30 May 2013 09:23:28 +0200 or thereabout, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 08:39 Thu 30 May , Richard Genoud wrote:
>> 2013/5/29 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>:
>> > On 19:44 Wed 29 May , Richard Genoud wrote:
>> >> 2013/5/29 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>:
>> >> > On 16:36 Wed 29 May , Richard Genoud wrote:
>> >> >> 2013/4/11 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>:
<snipp diff>
>> >> >> > + bool lcdcon_pol_negative;
>> >> >> I think lcdcon_pol_negative should be part of pdata, because it really
>> >> >> depends on how the PWM is wired on the board.
>> >> >>
>> >> >
>> >> > maybe but no one mainline use it on any pdata for non-dt boars
>> >> > so I did not want to expose it
>> >> Well, at least, I'm using it :)
>> >> (and I guess that Andreas is using it also, otherwise he wouldn't have
>> >> introduce it !)
>> >
>> > yes but pdata is for non-dt boards, for dt you can keep it in struct
>> > atmel_lcdfb_info and add a property
>> >
>> > if non-dt boards want it my answer is I do not care switch to DT
>>
>> ok (I use a full DT board based on sam9g35)
>>
>> so I'll add something like
>> sinfo->lcdcon_pol_negative = of_property_read_bool(display_np,
>> "atmel,lcdcon-pwm-pulse-low");
>> in /atmel_lcdfb.c
>>
>> But I thought the goal of this patch was to separate driver data from
>> platform specific data, and IMHO, lcdcon_pol_negative is a specificity
>> of the platform.
>
> You are right but as non one mainline use it as pdata I choose to drop it
> and only keep it on the driver as we can still use it for DT
>
> It's a way to force peopoe to switch to DT
FYI AVR32 architecture doesn't support device tree, nor is it likely to do
so. However there are no users of the PWM output from the LCDC block for the
boards in the current kernel, and I doubt new boards will pop.
--
Hans-Christian Egtvedt
^ permalink raw reply
* Re: [PATCH 06/32] OMAPDSS: DPI: fix regulators for DT
From: Arnd Bergmann @ 2013-05-30 13:05 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jean-Christophe PLAGNIOL-VILLARD, Grant Likely, Linus Walleij,
gregkh, linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <51A73990.5040709@ti.com>
On Thursday 30 May 2013, Tomi Valkeinen wrote:
> On 30/05/13 14:12, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 12:34 Thu 30 May , Tomi Valkeinen wrote:
> >> On some platforms DPI requires a regulator to be enabled to power up the
> >> output pins. This regulator is, for some reason, currently attached to
> >> the virtual omapdss device, instead of the DPI device. This does not
> >> work for DT, as the regulator mappings need to be described in the DT
> >> data, and the virtual omapdss device is not present there.
> >>
> >> Fix the issue by acquiring the regulator in the DPI device. To retain
> >> compatibility with the current board files, the old method of getting
> >> the regulator is kept. The old method can be removed when the board
> >> files have been changed to pass the regulator to DPI.
> >
> > as discuss with Arnd we should handle regular enable and disable at device
> > probe for every device as we do for pinctrl
>
> I'm not sure what you mean. Enable of what? The regulator? Why would we
> enable it in the device's probe, as the device may never even be used?
It's an idea I had a while ago, but not yet discussed in the open.
Jean-Christophe just posted patches to move the mapping of interrupt numbers
into platform_drv_probe(), just before calling the driver ->probe() callback,
and we already have similar code to set up the default pinctrl state of
a device before calling probe().
This can be extended to further subsystems, but that has to be done
carefully to avoid regressions. Ideally we would move a lot of boilerplate
code out of the driver specific ->probe() function into common code.
Possible candidates for this include:
* calling devm_request_mem_region for the "reg" property
* calling devm_ioremap on the "reg" property"
* calling devm_gpio_request for all gpio lines
* calling devm_regulator_get on all regulators
* calling devm_reset_control_get on all reset lines
* calling devm_dma_request_slave_channel on all dma channels
* calling devm_of_pwm_get for all pwm channels
* ...
For most of these (maybe all), I think we need some form of opt-in
model on the driver side because there are cases where aquiring some
of these resources is not mandatory, and it only works if the driver
is using DT probing.
IF we want to do this, it also needs a lot of thought, and we shouldn't
do it carelessly. We might also need some extra infrastructure in revres
to simplify access to the resources we got from the OF node.
The irq resources are particularly trivial because we already claim
them in of_platform_populate, so moving that to platform_drv_probe()
is straightforward and solves existing bugs without creating a huge
regression potential, but it's harder for the others.
Arnd
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox