* Re: [PATCH 12/23] video: da8xx-fb: fix 24bpp raster configuration
From: Tomi Valkeinen @ 2013-06-26 8:07 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1372170171-9561-13-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1216 bytes --]
On 25/06/13 17:22, Darren Etheridge wrote:
> From: Manjunathappa, Prakash <prakash.pm@ti.com>
>
> Set only LCD_V2_TFT_24BPP_MODE bit for 24bpp and LCD_V2_TFT_24BPP_UNPACK
> bit along with LCD_V2_TFT_24BPP_MODE for 32bpp configuration.
>
> Patch is tested on am335x-evm for 24bpp and da850-evm for 16bpp
> configurations.
>
> Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> Signed-off-by: Darren Etheridge <detheridge@ti.com>
> ---
> drivers/video/da8xx-fb.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index 35a33ca..7f92f37 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -550,10 +550,10 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
> case 4:
> case 16:
> break;
> - case 24:
> - reg |= LCD_V2_TFT_24BPP_MODE;
> case 32:
> reg |= LCD_V2_TFT_24BPP_UNPACK;
> + case 24:
> + reg |= LCD_V2_TFT_24BPP_MODE;
> break;
>
I'd suggest not to use fall-through here. It just makes the code more
difficult to read and more error prone.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 10/23] video: da8xx-fb: fb_set_par support
From: Tomi Valkeinen @ 2013-06-26 7:58 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1372170171-9561-11-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]
On 25/06/13 17:22, Darren Etheridge wrote:
> From: Afzal Mohammed <afzal@ti.com>
>
> fb_set_par helps in runtime configuration of lcd controller like
> changing resolution, pixel clock etc. (eg. using fbset utility)
>
> Reconfigure lcd controller based on information passed by framework.
> Enable raster back if it was already enabled.
>
> As fb_set_par would get invoked indirectly from probe via fb_set_var,
> remove existing lcdc initialization in probe and do lcdc reset in
> probe so that reset happens only at the begining.
>
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> Signed-off-by: Darren Etheridge <detheridge@ti.com>
> ---
> drivers/video/da8xx-fb.c | 60 +++++++++++++++++++++++++++++++++++++--------
> 1 files changed, 49 insertions(+), 11 deletions(-)
>
<snip>
> @@ -1201,9 +1201,52 @@ static int da8xx_pan_display(struct fb_var_screeninfo *var,
> return ret;
> }
>
> +static int da8xxfb_set_par(struct fb_info *info)
> +{
> + struct da8xx_fb_par *par = info->par;
> + int ret;
> + bool raster = da8xx_fb_is_raster_enabled();
> +
> + if (raster)
> + lcd_disable_raster(true);
> + else
> + lcd_disable_raster(false);
Not actually part of this patch, but: that kind of functions are quite
unclear. What does disable(true) mean? You have to read the function to
understand the call.
I think it'd be much better to take the wait-for-frame-done code out
from the lcd_disable_raster function, and call the wait function
explicitly here in the if (raster) block.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 07/23] video: da8xx-fb: pix clk and clk div handling cleanup
From: Tomi Valkeinen @ 2013-06-26 7:55 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1372170171-9561-8-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]
On 25/06/13 17:22, Darren Etheridge wrote:
> From: Afzal Mohammed <afzal@ti.com>
>
> Use the new modedb field to store pix clk. Reorganize existing clock
> divider functions with names now corresponding to what they do, add
> common function prefix.
>
> Fix existing panel modedb pixclock to be in ps instead of Hz. This
> needed a change in the way clock divider is calculated. As modedb
> pixclock information is now in ps, override on "var" pixclock over
> modedb to var conversion is removed.
>
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> Signed-off-by: Darren Etheridge <detheridge@ti.com>
> ---
> drivers/video/da8xx-fb.c | 48 +++++++++++++++++----------------------------
> 1 files changed, 18 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index f1d88ac..7f08644 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -160,7 +160,6 @@ struct da8xx_fb_par {
> struct clk *lcdc_clk;
> int irq;
> unsigned int palette_sz;
> - unsigned int pxl_clk;
> int blank;
> wait_queue_head_t vsync_wait;
> int vsync_flag;
> @@ -201,7 +200,7 @@ static struct fb_videomode known_lcd_panels[] = {
> .name = "Sharp_LCD035Q3DG01",
> .xres = 320,
> .yres = 240,
> - .pixclock = 4608000,
> + .pixclock = 217014,
Maybe it'd be better to use a macro here and convert from Hz to ps. The
panel specs always (afaik) report the pixel clock in Hz, and it also
makes the units clear to the reader.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 11/23] video: da8xx-fb: make io operations safe
From: Tomi Valkeinen @ 2013-06-26 7:43 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1372170171-9561-12-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 226 bytes --]
On 25/06/13 17:22, Darren Etheridge wrote:
> From: Afzal Mohammed <afzal@ti.com>
>
> Replace __raw_readl/__raw_writel with readl/writel; this driver is
> reused on ARMv7 (AM335x SoC).
Why is this needed?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
From: Felipe Balbi @ 2013-06-26 5:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51CA0FE1.9090107@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]
On Tue, Jun 25, 2013 at 11:47:13PM +0200, Sylwester Nawrocki wrote:
> Hi,
>
> On 06/25/2013 10:54 PM, Felipe Balbi wrote:
> >>>>+static int exynos_video_phy_probe(struct platform_device *pdev)
> >>>>> >> +{
> >>>>> >> + struct exynos_video_phy *state;
> >>>>> >> + struct device *dev =&pdev->dev;
> >>>>> >> + struct resource *res;
> >>>>> >> + struct phy_provider *phy_provider;
> >>>>> >> + int i;
> >>>>> >> +
> >>>>> >> + state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> >>>>> >> + if (!state)
> >>>>> >> + return -ENOMEM;
> >>>>> >> +
> >>>>> >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>>>> >> +
> >>>>> >> + state->regs = devm_ioremap_resource(dev, res);
> >>>>> >> + if (IS_ERR(state->regs))
> >>>>> >> + return PTR_ERR(state->regs);
> >>>>> >> +
> >>>>> >> + dev_set_drvdata(dev, state);
> >>>> >
> >>>> > you can use platform_set_drvdata(pdev, state);
> >>>
> >>> I had it in the previous version, but changed for symmetry with
> >>> dev_set_drvdata(). I guess those could be replaced with
> >>> phy_{get, set}_drvdata as you suggested.
> >
> >hmm, you do need to set the drvdata() to the phy object, but also to the
> >pdev object (should you need it on a suspend/resume callback, for
> >instance). Those are separate struct device instances.
>
> Indeed, I somehow confused phy->dev with with phy->dev.parent. I'm going
> to just drop the above call, since the pdev drvdata is currently not
> referenced anywhere.
ok cool :-)
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH -next] video: mxsfb: remove redundant dev_err call in mxsfb_probe()
From: Wei Yongjun @ 2013-06-26 1:50 UTC (permalink / raw)
To: plagnioj-sclMFOaUSTBWk0Htik3J/w, tomi.valkeinen-l0cyMroinI0,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
There is a error message within devm_ioremap_resource
already, so remove the dev_err call to avoid redundant
error message.
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
drivers/video/mxsfb.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
index 21223d4..251bbec 100644
--- a/drivers/video/mxsfb.c
+++ b/drivers/video/mxsfb.c
@@ -899,7 +899,6 @@ static int mxsfb_probe(struct platform_device *pdev)
host->base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(host->base)) {
- dev_err(&pdev->dev, "ioremap failed\n");
ret = PTR_ERR(host->base);
goto fb_release;
}
^ permalink raw reply related
* [PATCH 9/9] radeon: use pdev->pm_cap instead of pci_find_capability(..,PCI_CAP_ID_PM)
From: Yijing Wang @ 2013-06-26 1:13 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Yijing Wang, Benjamin Herrenschmidt,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, linux-fbdev
In-Reply-To: <1371543891-11460-1-git-send-email-wangyijing@huawei.com>
Pci core has been saved pm cap register offset by pdev->pm_cap in pci_pm_init()
in init path. So we can use pdev->pm_cap instead of using
pci_find_capability(pdev, PCI_CAP_ID_PM) for better performance and simplified code.
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
drivers/video/aty/radeon_pm.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/aty/radeon_pm.c b/drivers/video/aty/radeon_pm.c
index 92bda58..f7091ec 100644
--- a/drivers/video/aty/radeon_pm.c
+++ b/drivers/video/aty/radeon_pm.c
@@ -2805,7 +2805,7 @@ static void radeonfb_early_resume(void *data)
void radeonfb_pm_init(struct radeonfb_info *rinfo, int dynclk, int ignore_devlist, int force_sleep)
{
/* Find PM registers in config space if any*/
- rinfo->pm_reg = pci_find_capability(rinfo->pdev, PCI_CAP_ID_PM);
+ rinfo->pm_reg = rinfo->pdev->pm_cap;
/* Enable/Disable dynamic clocks: TODO add sysfs access */
if (rinfo->family = CHIP_FAMILY_RS480)
--
1.7.1
^ permalink raw reply related
* [PATCH 3/9] aty128fb: use pdev->pm_cap instead of pci_find_capability(..,PCI_CAP_ID_PM)
From: Yijing Wang @ 2013-06-26 1:13 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Yijing Wang, Paul Mackerras,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, linux-fbdev
In-Reply-To: <1371543307-14360-1-git-send-email-wangyijing@huawei.com>
Pci core has been saved pm cap register offset by pdev->pm_cap in pci_pm_init()
in init path. So we can use pdev->pm_cap instead of using
pci_find_capability(pdev, PCI_CAP_ID_PM) for better performance and simplified code.
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
---
drivers/video/aty/aty128fb.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/aty/aty128fb.c b/drivers/video/aty/aty128fb.c
index 8c55011..a4dfe8c 100644
--- a/drivers/video/aty/aty128fb.c
+++ b/drivers/video/aty/aty128fb.c
@@ -2016,7 +2016,7 @@ static int aty128_init(struct pci_dev *pdev, const struct pci_device_id *ent)
aty128_init_engine(par);
- par->pm_reg = pci_find_capability(pdev, PCI_CAP_ID_PM);
+ par->pm_reg = pdev->pm_cap;
par->pdev = pdev;
par->asleep = 0;
par->lock_blank = 0;
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
From: Sylwester Nawrocki @ 2013-06-25 21:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130625205452.GC9748@arwen.pp.htv.fi>
Hi,
On 06/25/2013 10:54 PM, Felipe Balbi wrote:
>>>> +static int exynos_video_phy_probe(struct platform_device *pdev)
>>>> > >> +{
>>>> > >> + struct exynos_video_phy *state;
>>>> > >> + struct device *dev =&pdev->dev;
>>>> > >> + struct resource *res;
>>>> > >> + struct phy_provider *phy_provider;
>>>> > >> + int i;
>>>> > >> +
>>>> > >> + state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
>>>> > >> + if (!state)
>>>> > >> + return -ENOMEM;
>>>> > >> +
>>>> > >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> > >> +
>>>> > >> + state->regs = devm_ioremap_resource(dev, res);
>>>> > >> + if (IS_ERR(state->regs))
>>>> > >> + return PTR_ERR(state->regs);
>>>> > >> +
>>>> > >> + dev_set_drvdata(dev, state);
>>> > >
>>> > > you can use platform_set_drvdata(pdev, state);
>> >
>> > I had it in the previous version, but changed for symmetry with
>> > dev_set_drvdata(). I guess those could be replaced with
>> > phy_{get, set}_drvdata as you suggested.
>
> hmm, you do need to set the drvdata() to the phy object, but also to the
> pdev object (should you need it on a suspend/resume callback, for
> instance). Those are separate struct device instances.
Indeed, I somehow confused phy->dev with with phy->dev.parent. I'm going
to just drop the above call, since the pdev drvdata is currently not
referenced anywhere.
Thanks,
Sylwester
^ permalink raw reply
* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
From: Felipe Balbi @ 2013-06-25 20:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51C9D714.4000703@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 3653 bytes --]
Hi,
On Tue, Jun 25, 2013 at 07:44:52PM +0200, Sylwester Nawrocki wrote:
> >> +struct exynos_video_phy {
> >> + spinlock_t slock;
> >> + struct phy *phys[NUM_PHYS];
> >
> > more than one phy ? This means you should instantiate driver multiple
> > drivers. Each phy id should call probe again.
>
> Why ? This single PHY _provider_ can well handle multiple PHYs.
> I don't see a good reason to further complicate this driver like
> this. Please note that MIPI-CSIS 0 and MIPI DSIM 0 share MMIO
> register, so does MIPI CSIS 1 and MIPI DSIM 1. There are only 2
> registers for those 4 PHYs. I could have the involved object
> multiplied, but it would have been just a waste of resources
> with no difference to the PHY consumers.
alright, I misunderstood your code then. When I looked over your id
usage I missed the "/2" part and assumed that you would have separate
EXYNOS_MIPI_PHY_CONTROL() register for each ;-)
My bad, you can disregard the other comments.
> >> +static int exynos_video_phy_probe(struct platform_device *pdev)
> >> +{
> >> + struct exynos_video_phy *state;
> >> + struct device *dev = &pdev->dev;
> >> + struct resource *res;
> >> + struct phy_provider *phy_provider;
> >> + int i;
> >> +
> >> + state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> >> + if (!state)
> >> + return -ENOMEM;
> >> +
> >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +
> >> + state->regs = devm_ioremap_resource(dev, res);
> >> + if (IS_ERR(state->regs))
> >> + return PTR_ERR(state->regs);
> >> +
> >> + dev_set_drvdata(dev, state);
> >
> > you can use platform_set_drvdata(pdev, state);
>
> I had it in the previous version, but changed for symmetry with
> dev_set_drvdata(). I guess those could be replaced with
> phy_{get, set}_drvdata as you suggested.
hmm, you do need to set the drvdata() to the phy object, but also to the
pdev object (should you need it on a suspend/resume callback, for
instance). Those are separate struct device instances.
> >> +static const struct of_device_id exynos_video_phy_of_match[] = {
> >> + { .compatible = "samsung,s5pv210-mipi-video-phy" },
> >
> > and this should contain all PHY IDs:
> >
> > { .compatible = "samsung,s5pv210-mipi-video-dsim0-phy",
> > .data = (const void *) DSIM0, },
> > { .compatible = "samsung,s5pv210-mipi-video-dsim1-phy",
> > .data = (const void *) DSIM1, },
> > { .compatible = "samsung,s5pv210-mipi-video-csi0-phy"
> > .data = (const void *) CSI0, },
> > { .compatible = "samsung,s5pv210-mipi-video-csi1-phy"
> > .data = (const void *) CSI1, },
> >
> > then on your probe you can fetch that data field and use it as phy->id.
>
> This looks wrong to me, it doesn't look like a right usage of 'compatible'
> property. MIPI-CSIS0/MIPI-DSIM0, MIPI-CSIS1/MIPI-DSIM1 are identical pairs,
> so one compatible property would need to be used for them. We don't use
> different compatible strings for different instances of same device.
> And MIPI DSIM and MIPI CSIS share one MMIO register, so they need to be
> handled by one provider, to synchronize accesses. That's one of the main
> reasons I turned to the generic PHY framework for those devices.
understood :-)
> >> +static struct platform_driver exynos_video_phy_driver = {
> >> + .probe = exynos_video_phy_probe,
> >
> > you *must* provide a remove method. drivers with NULL remove are
> > non-removable :-)
>
> Oops, my bad. I've forgotten to update this, after enabling build
> as module. Will update and test that. It will be an empty callback
> though.
right, because of devm.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
From: Tomasz Figa @ 2013-06-25 18:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51C9D714.4000703@samsung.com>
Hi Sylwester, Felipe,
On Tuesday 25 of June 2013 19:44:52 Sylwester Nawrocki wrote:
> Hi Felipe,
>
> Thanks for the review.
>
> On 06/25/2013 05:06 PM, Felipe Balbi wrote:
> > On Tue, Jun 25, 2013 at 04:21:46PM +0200, Sylwester Nawrocki wrote:
> >> +enum phy_id {
> >> + PHY_CSIS0,
> >> + PHY_DSIM0,
> >> + PHY_CSIS1,
> >> + PHY_DSIM1,
> >> + NUM_PHYS
> >
> > please prepend these with EXYNOS_PHY_ or EXYNOS_MIPI_PHY_
>
> OK, will fix that.
>
> >> +struct exynos_video_phy {
> >> + spinlock_t slock;
> >> + struct phy *phys[NUM_PHYS];
> >
> > more than one phy ? This means you should instantiate driver multiple
> > drivers. Each phy id should call probe again.
>
> Why ? This single PHY _provider_ can well handle multiple PHYs.
> I don't see a good reason to further complicate this driver like
> this. Please note that MIPI-CSIS 0 and MIPI DSIM 0 share MMIO
> register, so does MIPI CSIS 1 and MIPI DSIM 1. There are only 2
> registers for those 4 PHYs. I could have the involved object
> multiplied, but it would have been just a waste of resources
> with no difference to the PHY consumers.
IMHO one driver instance should represent one instance of IP block. Since
this is a single IP block containing multiple PHYs with shared control
interface, I think Sylwester did the right thing.
[snip]
> >> +static struct phy *exynos_video_phy_xlate(struct device *dev,
> >> + struct of_phandle_args *args)
> >> +{
> >> + struct exynos_video_phy *state = dev_get_drvdata(dev);
> >> +
> >> + if (WARN_ON(args->args[0] > NUM_PHYS))
> >> + return NULL;
> >
> > please remove this check.
>
> args->args[0] comes from DT as the PHY id and there is nothing
> preventing it from being greater or equal to the state->phys[]
> array length, unless I'm missing something. Actually it should
> have been 'if (args->args[0] >= NUM_PHYS)'.
The xlate() callback gets directly whatever parsed from device tree, so it
is possible for an out of range value to get here and so this check is
valid. However I think it should rather return an ERR_PTR, not NULL. See
of_phy_get().
> >> + return state->phys[args->args[0]];
> >
> > and your xlate is 'wrong'.
>
> What exactly is wrong here ?
Felipe, could you elaborate a bit more on this? I can't find any serious
problems with this code.
[snip]
> >> + phy_provider = devm_of_phy_provider_register(dev,
> >> + exynos_video_phy_xlate);
> >> + if (IS_ERR(phy_provider))
> >> + return PTR_ERR(phy_provider);
> >> +
> >> + for (i = 0; i < NUM_PHYS; i++) {
> >> + char label[8];
> >> +
> >> + snprintf(label, sizeof(label), "%s.%d",
> >> + i = PHY_DSIM0 || i = PHY_DSIM1 ?
> >> + "dsim" : "csis", i / 2);
> >> +
> >> + state->phys[i] = devm_phy_create(dev, i,
&exynos_video_phy_ops,
> >> + label,
state);
> >> + if (IS_ERR(state->phys[i])) {
> >> + dev_err(dev, "failed to create PHY %s\n", label);
> >> + return PTR_ERR(state->phys[i]);
> >> + }
> >> + }
> >
> > this really doesn't look correct to me. It looks like you have
> > multiple
> > PHYs, one for each ID. So your probe should be called for each PHY ID
> > and you have several phy_providers too.
>
> Yes, multiple PHY objects, but a single provider. There is no need
> whatsoever for multiple PHY providers.
The whole concept of whatever-provider is to allow managing multiple
objects by one parent object, like one clock provider for the whole clock
controller, one interrupt controller object for all interrupts of an
interrupt controller block, etc.
This is why a phandle has args, to allow addressing subobjects inside a
provider.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
From: Sylwester Nawrocki @ 2013-06-25 17:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130625150649.GA21334@arwen.pp.htv.fi>
Hi Felipe,
Thanks for the review.
On 06/25/2013 05:06 PM, Felipe Balbi wrote:
> On Tue, Jun 25, 2013 at 04:21:46PM +0200, Sylwester Nawrocki wrote:
>> +enum phy_id {
>> + PHY_CSIS0,
>> + PHY_DSIM0,
>> + PHY_CSIS1,
>> + PHY_DSIM1,
>> + NUM_PHYS
>
> please prepend these with EXYNOS_PHY_ or EXYNOS_MIPI_PHY_
OK, will fix that.
>> +struct exynos_video_phy {
>> + spinlock_t slock;
>> + struct phy *phys[NUM_PHYS];
>
> more than one phy ? This means you should instantiate driver multiple
> drivers. Each phy id should call probe again.
Why ? This single PHY _provider_ can well handle multiple PHYs.
I don't see a good reason to further complicate this driver like
this. Please note that MIPI-CSIS 0 and MIPI DSIM 0 share MMIO
register, so does MIPI CSIS 1 and MIPI DSIM 1. There are only 2
registers for those 4 PHYs. I could have the involved object
multiplied, but it would have been just a waste of resources
with no difference to the PHY consumers.
>> +static int __set_phy_state(struct exynos_video_phy *state,
>> + enum phy_id id, unsigned int on)
>> +{
>> + void __iomem *addr;
>> + unsigned long flags;
>> + u32 reg, reset;
>> +
>> + if (WARN_ON(id > NUM_PHYS))
>> + return -EINVAL;
>
> you don't want to do this, actually. It'll bug you everytime you want to
> add another phy ID :-)
If there is an SoC with more PHYs enum phy_id would be extended, and this
part would not need to be touched. OTOH @id cannot be normally greater
than NUM_PHYS. I think I'll drop that then.
>> + addr = state->regs + EXYNOS_MIPI_PHY_CONTROL(id / 2);
>> +
>> + if (id = PHY_DSIM0 || id = PHY_DSIM1)
>> + reset = EXYNOS_MIPI_PHY_MRESETN;
>> + else
>> + reset = EXYNOS_MIPI_PHY_SRESETN;
>> +
>> + spin_lock_irqsave(&state->slock, flags);
>> + reg = readl(addr);
>> + if (on)
>> + reg |= reset;
>> + else
>> + reg &= ~reset;
>> + writel(reg, addr);
>> +
>> + /* Clear ENABLE bit only if MRESETN, SRESETN bits are not set. */
>> + if (on)
>> + reg |= EXYNOS_MIPI_PHY_ENABLE;
>> + else if (!(reg & EXYNOS_MIPI_PHY_RESET_MASK))
>> + reg &= ~EXYNOS_MIPI_PHY_ENABLE;
>> +
>> + writel(reg, addr);
>> + spin_unlock_irqrestore(&state->slock, flags);
>> +
>> + pr_debug("%s(): id: %d, on: %d, addr: %#x, base: %#x\n",
>> + __func__, id, on, (u32)addr, (u32)state->regs);
>
> use dev_dbg() instead.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_video_phy_power_on(struct phy *phy)
>> +{
>> + struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
>
> looks like we should have phy_get_drvdata() helper :-) Kishon ?
Indeed, that might be useful.
>> +static struct phy *exynos_video_phy_xlate(struct device *dev,
>> + struct of_phandle_args *args)
>> +{
>> + struct exynos_video_phy *state = dev_get_drvdata(dev);
>> +
>> + if (WARN_ON(args->args[0] > NUM_PHYS))
>> + return NULL;
>
> please remove this check.
args->args[0] comes from DT as the PHY id and there is nothing
preventing it from being greater or equal to the state->phys[]
array length, unless I'm missing something. Actually it should
have been 'if (args->args[0] >= NUM_PHYS)'.
>> + return state->phys[args->args[0]];
>
> and your xlate is 'wrong'.
What exactly is wrong here ?
>> +}
>> +
>> +static struct phy_ops exynos_video_phy_ops = {
>> + .power_on = exynos_video_phy_power_on,
>> + .power_off = exynos_video_phy_power_off,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int exynos_video_phy_probe(struct platform_device *pdev)
>> +{
>> + struct exynos_video_phy *state;
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct phy_provider *phy_provider;
>> + int i;
>> +
>> + state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
>> + if (!state)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + state->regs = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(state->regs))
>> + return PTR_ERR(state->regs);
>> +
>> + dev_set_drvdata(dev, state);
>
> you can use platform_set_drvdata(pdev, state);
I had it in the previous version, but changed for symmetry with
dev_set_drvdata(). I guess those could be replaced with
phy_{get, set}_drvdata as you suggested.
> same thing though, no strong feelings.
>
>> + phy_provider = devm_of_phy_provider_register(dev,
>> + exynos_video_phy_xlate);
>> + if (IS_ERR(phy_provider))
>> + return PTR_ERR(phy_provider);
>> +
>> + for (i = 0; i < NUM_PHYS; i++) {
>> + char label[8];
>> +
>> + snprintf(label, sizeof(label), "%s.%d",
>> + i = PHY_DSIM0 || i = PHY_DSIM1 ?
>> + "dsim" : "csis", i / 2);
>> +
>> + state->phys[i] = devm_phy_create(dev, i, &exynos_video_phy_ops,
>> + label, state);
>> + if (IS_ERR(state->phys[i])) {
>> + dev_err(dev, "failed to create PHY %s\n", label);
>> + return PTR_ERR(state->phys[i]);
>> + }
>> + }
>
> this really doesn't look correct to me. It looks like you have multiple
> PHYs, one for each ID. So your probe should be called for each PHY ID
> and you have several phy_providers too.
Yes, multiple PHY objects, but a single provider. There is no need
whatsoever for multiple PHY providers.
>> +static const struct of_device_id exynos_video_phy_of_match[] = {
>> + { .compatible = "samsung,s5pv210-mipi-video-phy" },
>
> and this should contain all PHY IDs:
>
> { .compatible = "samsung,s5pv210-mipi-video-dsim0-phy",
> .data = (const void *) DSIM0, },
> { .compatible = "samsung,s5pv210-mipi-video-dsim1-phy",
> .data = (const void *) DSIM1, },
> { .compatible = "samsung,s5pv210-mipi-video-csi0-phy"
> .data = (const void *) CSI0, },
> { .compatible = "samsung,s5pv210-mipi-video-csi1-phy"
> .data = (const void *) CSI1, },
>
> then on your probe you can fetch that data field and use it as phy->id.
This looks wrong to me, it doesn't look like a right usage of 'compatible'
property. MIPI-CSIS0/MIPI-DSIM0, MIPI-CSIS1/MIPI-DSIM1 are identical pairs,
so one compatible property would need to be used for them. We don't use
different compatible strings for different instances of same device.
And MIPI DSIM and MIPI CSIS share one MMIO register, so they need to be
handled by one provider, to synchronize accesses. That's one of the main
reasons I turned to the generic PHY framework for those devices.
>> +static struct platform_driver exynos_video_phy_driver = {
>> + .probe = exynos_video_phy_probe,
>
> you *must* provide a remove method. drivers with NULL remove are
> non-removable :-)
Oops, my bad. I've forgotten to update this, after enabling build
as module. Will update and test that. It will be an empty callback
though.
Thanks,
Sylwester
^ permalink raw reply
* Re: [PATCH v2 5/5] ARM: Samsung: Remove MIPI PHY setup code
From: Felipe Balbi @ 2013-06-25 15:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1372170110-12993-5-git-send-email-s.nawrocki@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 609 bytes --]
On Tue, Jun 25, 2013 at 04:21:50PM +0200, Sylwester Nawrocki wrote:
> Generic PHY drivers are used to handle the MIPI CSIS and MIPI DSIM
> DPHYs so we can remove now unused code at arch/arm/plat-samsung.
> In case there is any board file for S5PV210 platforms using MIPI
> CSIS/DSIM (not any upstream currently) it should use the generic
> PHY API to bind the PHYs to respective PHY consumer drivers.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
pretty cool stuff
Acked-by: Felipe Balbi <balbi@ti.com>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 4/5] exynos4-is: Use generic MIPI CSIS PHY driver
From: Felipe Balbi @ 2013-06-25 15:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1372170110-12993-4-git-send-email-s.nawrocki@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 449 bytes --]
On Tue, Jun 25, 2013 at 04:21:49PM +0200, Sylwester Nawrocki wrote:
> Use the generic PHY API instead of the platform callback to control
> the MIPI CSIS DPHY. The 'phy_label' field is added to the platform
> data structure to allow PHY lookup on non-dt platforms
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Felipe Balbi <balbi@ti.com>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 3/5] video: exynos_mipi_dsim: Use generic PHY driver
From: Felipe Balbi @ 2013-06-25 15:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1372170110-12993-3-git-send-email-s.nawrocki@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 480 bytes --]
Hi,
On Tue, Jun 25, 2013 at 04:21:48PM +0200, Sylwester Nawrocki wrote:
> Use the generic PHY API instead of the platform callback to control
> the MIPI DSIM DPHY. The 'phy_label' field is added to the platform
> data structure to allow PHY lookup on non-dt platforms.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
this is awesome :-)
Acked-by: Felipe Balbi <balbi@ti.com>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
From: Felipe Balbi @ 2013-06-25 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1372170110-12993-1-git-send-email-s.nawrocki@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 4608 bytes --]
Hi,
On Tue, Jun 25, 2013 at 04:21:46PM +0200, Sylwester Nawrocki wrote:
> +enum phy_id {
> + PHY_CSIS0,
> + PHY_DSIM0,
> + PHY_CSIS1,
> + PHY_DSIM1,
> + NUM_PHYS
please prepend these with EXYNOS_PHY_ or EXYNOS_MIPI_PHY_
> +struct exynos_video_phy {
> + spinlock_t slock;
> + struct phy *phys[NUM_PHYS];
more than one phy ? This means you should instantiate driver multiple
drivers. Each phy id should call probe again.
> +static int __set_phy_state(struct exynos_video_phy *state,
> + enum phy_id id, unsigned int on)
> +{
> + void __iomem *addr;
> + unsigned long flags;
> + u32 reg, reset;
> +
> + if (WARN_ON(id > NUM_PHYS))
> + return -EINVAL;
you don't want to do this, actually. It'll bug you everytime you want to
add another phy ID :-)
> + addr = state->regs + EXYNOS_MIPI_PHY_CONTROL(id / 2);
> +
> + if (id == PHY_DSIM0 || id == PHY_DSIM1)
> + reset = EXYNOS_MIPI_PHY_MRESETN;
> + else
> + reset = EXYNOS_MIPI_PHY_SRESETN;
> +
> + spin_lock_irqsave(&state->slock, flags);
> + reg = readl(addr);
> + if (on)
> + reg |= reset;
> + else
> + reg &= ~reset;
> + writel(reg, addr);
> +
> + /* Clear ENABLE bit only if MRESETN, SRESETN bits are not set. */
> + if (on)
> + reg |= EXYNOS_MIPI_PHY_ENABLE;
> + else if (!(reg & EXYNOS_MIPI_PHY_RESET_MASK))
> + reg &= ~EXYNOS_MIPI_PHY_ENABLE;
> +
> + writel(reg, addr);
> + spin_unlock_irqrestore(&state->slock, flags);
> +
> + pr_debug("%s(): id: %d, on: %d, addr: %#x, base: %#x\n",
> + __func__, id, on, (u32)addr, (u32)state->regs);
use dev_dbg() instead.
> +
> + return 0;
> +}
> +
> +static int exynos_video_phy_power_on(struct phy *phy)
> +{
> + struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
looks like we should have phy_get_drvdata() helper :-) Kishon ?
> +static struct phy *exynos_video_phy_xlate(struct device *dev,
> + struct of_phandle_args *args)
> +{
> + struct exynos_video_phy *state = dev_get_drvdata(dev);
> +
> + if (WARN_ON(args->args[0] > NUM_PHYS))
> + return NULL;
please remove this check.
> + return state->phys[args->args[0]];
and your xlate is 'wrong'.
> +}
> +
> +static struct phy_ops exynos_video_phy_ops = {
> + .power_on = exynos_video_phy_power_on,
> + .power_off = exynos_video_phy_power_off,
> + .owner = THIS_MODULE,
> +};
> +
> +static int exynos_video_phy_probe(struct platform_device *pdev)
> +{
> + struct exynos_video_phy *state;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + struct phy_provider *phy_provider;
> + int i;
> +
> + state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> + if (!state)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> + state->regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(state->regs))
> + return PTR_ERR(state->regs);
> +
> + dev_set_drvdata(dev, state);
you can use platform_set_drvdata(pdev, state);
same thing though, no strong feelings.
> + phy_provider = devm_of_phy_provider_register(dev,
> + exynos_video_phy_xlate);
> + if (IS_ERR(phy_provider))
> + return PTR_ERR(phy_provider);
> +
> + for (i = 0; i < NUM_PHYS; i++) {
> + char label[8];
> +
> + snprintf(label, sizeof(label), "%s.%d",
> + i == PHY_DSIM0 || i == PHY_DSIM1 ?
> + "dsim" : "csis", i / 2);
> +
> + state->phys[i] = devm_phy_create(dev, i, &exynos_video_phy_ops,
> + label, state);
> + if (IS_ERR(state->phys[i])) {
> + dev_err(dev, "failed to create PHY %s\n", label);
> + return PTR_ERR(state->phys[i]);
> + }
> + }
this really doesn't look correct to me. It looks like you have multiple
PHYs, one for each ID. So your probe should be called for each PHY ID
and you have several phy_providers too.
> +static const struct of_device_id exynos_video_phy_of_match[] = {
> + { .compatible = "samsung,s5pv210-mipi-video-phy" },
and this should contain all PHY IDs:
{ .compatible = "samsung,s5pv210-mipi-video-dsim0-phy",
.data = (const void *) DSIM0, },
{ .compatible = "samsung,s5pv210-mipi-video-dsim1-phy",
.data = (const void *) DSIM1, },
{ .compatible = "samsung,s5pv210-mipi-video-csi0-phy"
.data = (const void *) CSI0, },
{ .compatible = "samsung,s5pv210-mipi-video-csi1-phy"
.data = (const void *) CSI1, },
then on your probe you can fetch that data field and use it as phy->id.
> +static struct platform_driver exynos_video_phy_driver = {
> + .probe = exynos_video_phy_probe,
you *must* provide a remove method. drivers with NULL remove are
non-removable :-)
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC PATCH] dmabuf-sync: Introduce buffer synchronization framework
From: Jerome Glisse @ 2013-06-25 14:49 UTC (permalink / raw)
To: Inki Dae
Cc: Rob Clark, linux-fbdev, Russell King - ARM Linux,
DRI mailing list, Kyungmin Park, myungjoo.ham, YoungJun Cho,
linux-media@vger.kernel.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAAQKjZNjjgG3hoKU2RLsG7w+B-2v7CpTT5hfnnTTJ2DgTEk0vA@mail.gmail.com>
On Tue, Jun 25, 2013 at 10:17 AM, Inki Dae <daeinki@gmail.com> wrote:
> 2013/6/25 Rob Clark <robdclark@gmail.com>:
>> On Tue, Jun 25, 2013 at 5:09 AM, Inki Dae <daeinki@gmail.com> wrote:
>>>> that
>>>> should be the role of kernel memory management which of course needs
>>>> synchronization btw A and B. But in no case this should be done using
>>>> dma-buf. dma-buf is for sharing content btw different devices not
>>>> sharing resources.
>>>>
>>>
>>> hmm, is that true? And are you sure? Then how do you think about
>>> reservation? the reservation also uses dma-buf with same reason as long as I
>>> know: actually, we use reservation to use dma-buf. As you may know, a
>>> reservation object is allocated and initialized when a buffer object is
>>> exported to a dma buf.
>>
>> no, this is why the reservation object can be passed in when you
>> construction the dmabuf.
>
> Right, that way, we could use dma buf for buffer synchronization. I
> just wanted to ask for why Jerome said that "dma-buf is for sharing
> content btw different devices not sharing resources".
From memory, the motivation of dma-buf was to done for few use case,
among them webcam capturing frame into a buffer and having gpu using
it directly without memcpy, or one big gpu rendering a scene into a
buffer that is then use by low power gpu for display ie it was done to
allow different device to operate on same data using same backing
memory.
AFAICT you seem to want to use dma-buf to create scratch buffer, ie a
process needs to use X amount of memory for an operation, it can
release|free this memory once its done and a process B can the use
this X memory for its own operation discarding content of process A. I
presume that next frame would have the sequence repeat, process A do
something, then process B does its thing. So to me it sounds like you
want to implement global scratch buffer using the dmabuf API and that
sounds bad to me.
I know most closed driver have several pool of memory, long lived
object, short lived object and scratch space, then user space allocate
from one of this pool and there is synchronization done by driver
using driver specific API to reclaim memory. Of course this work
nicely if you only talking about one logic block or at very least hw
that have one memory controller.
Now if you are thinking of doing scratch buffer for several different
device and share the memory among then you need to be aware of
security implication, most obvious being that you don't want process B
being able to read process A scratch memory. I know the argument about
it being graphic but one day this might become gpu code and it might
be able to insert jump to malicious gpu code.
Cheers,
Jerome
^ permalink raw reply
* Re: [V2 1/7] video: mmp: rb swap setting update for new LCD driver
From: Daniel Drake @ 2013-06-25 14:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACDDiy_CpKuZkevm4jQud1DKBEDa-ZhtrWb+rFpTXvK3zm6aHw@mail.gmail.com>
On Mon, Jun 24, 2013 at 8:23 PM, jett zhou <jett.zhou@gmail.com> wrote:
>> What if I'm working with a display that doesn't need or want RB
>> swapping? Lets say I am working with format PIXFMT_RGB565, and running
>> your patch. dmafetch_set_fmt() gets called, and fmt_to_reg() sets
>> rbswap to 1.
>> This means that dmafetch_set_fmt() writes a '1' into the appropriate
>> RB-swapping bit in the LCD_PN_CTRL0 register, and this triggers the
>> "DMA input" swapping that you mentioned. But I never asked for RB
>> swapping...
>
> Yes, if you configure it as PIXFMT_RGB565, it will set rbswap in "DMA
> input" part.
> So, for your case, you need to use PIXFMT_BGR565 instead of PIXFMT_RGB565.
So let me get this straight. I have a display that wants RGB565
format, no RB swapping. I don't do anything special in link_config to
affect any swapping.
After your patch, I must request BGR565 format in order to get RGB565?
That sounds backwards to me.
>> Your comment above suggests that this RB-swapping behaviour is
>> something that is imposed by the output device. In which case, this
>> should be a configuration parameter on the panel, not on the path
>> structure.
>>
>>> TTC_dkb does not support dsi, the link_config is no used anymore.
>>
>> Then you should fix up ttc_dkb before submitting this patch.
>
> After we add one new field for this output rbswap setting based on dsi
> interface, it can be used by new stepping of mmp display controller,
> ttc_dkb platform just leave and not touch it, it will be tranparent
> for ttc_dkb, does not need to nothing for platform configuration for
> ttc_dkb usage.
> It means , ttc_dkb can only configure rbswap in "dma input" part, not
> support rbswap in dsi interface part.
> What do you think?
The point I am trying to make is that your patch is changing behaviour
for ttc_dkb, so you need to address that at the same time.
Right now ttc_dkb does this:
#define CFG_GRA_SWAPRB(x) (x << 0) /* 1: rbswap enabled */
.link_config = CFG_DUMBMODE(0x2) | CFG_GRA_SWAPRB(0x1),
i.e. SWAPRB requested through bit 0 in link_config
And this is obeyed by the existing code in fmt_to_reg:
u32 link_config = path_to_path_plat(overlay->path)->link_config;
rbswap = link_config & 0x1;
Your patch removes the handling of link_config bit 0, without fixing
up its only user (even if that user was incorrect). That is not good
practice.
Another question: why is this change needed?
We can request rb swapping either in DMA input or in the output
interface. I can understand the driver maybe supporting one option or
the other. But after your patch, it seems like both are supported: RB
swapping could be enabled either through choice of input format, or
through configuration of output parameters.
Daniel
^ permalink raw reply
* [PATCH 23/23] video/da8xx-fb adding am33xx as dependency
From: Darren Etheridge @ 2013-06-25 14:22 UTC (permalink / raw)
To: linux-fbdev
Updating Kconfig to allow am33xx to include lcdc fbdev driver
including some extra dependencies needed by device tree mods.
Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
drivers/video/Kconfig | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 2e937bd..8ae6d2a 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2226,15 +2226,16 @@ config FB_SH7760
panels <= 320 pixel horizontal resolution.
config FB_DA8XX
- tristate "DA8xx/OMAP-L1xx Framebuffer support"
- depends on FB && ARCH_DAVINCI_DA8XX
+ tristate "DA8xx/OMAP-L1xx/AM335x Framebuffer support"
+ depends on FB && (ARCH_DAVINCI_DA8XX || SOC_AM33XX)
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
select FB_CFB_REV_PIXELS_IN_BYTE
+ select VIDEOMODE_HELPERS
---help---
This is the frame buffer device driver for the TI LCD controller
- found on DA8xx/OMAP-L1xx SoCs.
+ found on DA8xx/OMAP-L1xx/AM335x SoCs.
If unsure, say N.
config FB_VIRTUAL
--
1.7.0.4
^ permalink raw reply related
* [PATCH 22/23] video: da8xx-fb: set upstream clock rate (if reqd)
From: Darren Etheridge @ 2013-06-25 14:22 UTC (permalink / raw)
To: linux-fbdev
From: Afzal Mohammed <afzal@ti.com>
LCDC IP has a clock divider to adjust pixel clock, this limits pixel
clock range to fck/255 - fck/2(fck - rate of input clock to LCDC IP).
In the case of AM335x, where this IP is present, default fck is not
sufficient to provide normal pixel clock rates, hence rendering this
driver unusable on AM335x.
If input clock too is configurable, allowable range of pixel clock
would increase. Here initially it is checked whether with present fck,
divider in IP could be configured to obtain required rate, if not,
fck is adjusted. This makes it usable on AM335x.
Note:
Another solution would be to model an inherited basic clock divider of
CCF, an advantage would be a better possible resolution for pixel clk.
And trying to instantiate a CCF clock would mean that to be consistent,
3 bits being turned on to enable clocks of LCDC IP would have to be
modeled as gate clocks. Now that would bring in a total of 4 clocks,
including necessity to create a new inherited divider clock, and that
mean a branch of clock tree would be present in LCDC driver. This
would add complexity to LCDC driver bringing in considerable amount
of clock handling code, and this would not bring in much advantage
for existing use cases other than providing a higher resolution of
pixel clock. And existing use cases work without relying on clock
modeling. Another fact is that out of the two platform's using this
driver DaVinci is not yet converted to CCF. In future if higher
resolution of pixel clock is required, and probably after DaVinci is
CCF'ed, modeling clock nodes inside driver may be considered.
Signed-off-by: Afzal Mohammed <afzal@ti.com>
Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
drivers/video/da8xx-fb.c | 76 +++++++++++++++++++++++++++++++++++-----------
1 files changed, 58 insertions(+), 18 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 5455682..09dfa12 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -133,6 +133,9 @@
#define WSI_TIMEOUT 50
#define PALETTE_SIZE 256
+#define CLK_MIN_DIV 2
+#define CLK_MAX_DIV 255
+
static void __iomem *da8xx_fb_reg_base;
static struct resource *lcdc_regs;
static unsigned int lcd_revision;
@@ -683,23 +686,21 @@ static void da8xx_fb_lcd_reset(void)
}
}
-static inline unsigned da8xx_fb_calc_clk_divider(struct da8xx_fb_par *par,
- unsigned pixclock)
-{
- return par->lcd_fck_rate / (PICOS2KHZ(pixclock) * 1000);
-}
-
-static inline unsigned da8xx_fb_round_clk(struct da8xx_fb_par *par,
- unsigned pixclock)
+static int da8xx_fb_config_clk_divider(struct da8xx_fb_par *par,
+ unsigned div, unsigned rate)
{
- unsigned div;
+ int ret;
- div = da8xx_fb_calc_clk_divider(par, pixclock);
- return KHZ2PICOS(par->lcd_fck_rate / (1000 * div));
-}
+ if (par->lcd_fck_rate != rate) {
+ ret = clk_set_rate(par->lcdc_clk, rate);
+ if (IS_ERR_VALUE(ret)) {
+ dev_err(par->dev,
+ "unable to set clock rate at %u\n", rate);
+ return ret;
+ }
+ par->lcd_fck_rate = clk_get_rate(par->lcdc_clk);
+ }
-static inline void da8xx_fb_config_clk_divider(unsigned div)
-{
/* Configure the LCD clock divisor. */
lcdc_write(LCD_CLK_DIVISOR(div) |
(LCD_RASTER_MODE & 0x1), LCD_CTRL_REG);
@@ -707,14 +708,49 @@ static inline void da8xx_fb_config_clk_divider(unsigned div)
if (lcd_revision = LCD_VERSION_2)
lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);
+
+ return 0;
+}
+
+static unsigned int da8xx_fb_calc_clk_divider(struct da8xx_fb_par *par,
+ unsigned pixclock,
+ unsigned *rate)
+{
+ unsigned div;
+
+ pixclock = PICOS2KHZ(pixclock) * 1000;
+
+ *rate = par->lcd_fck_rate;
+
+ if (pixclock < (*rate / CLK_MAX_DIV)) {
+ *rate = clk_round_rate(par->lcdc_clk, pixclock * CLK_MAX_DIV);
+ div = CLK_MAX_DIV;
+ } else if (pixclock > (*rate / CLK_MIN_DIV)) {
+ *rate = clk_round_rate(par->lcdc_clk, pixclock * CLK_MIN_DIV);
+ div = CLK_MIN_DIV;
+ } else {
+ div = *rate / pixclock;
+ }
+
+ return div;
}
-static inline void da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
+static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
struct fb_videomode *mode)
{
- unsigned div = da8xx_fb_calc_clk_divider(par, mode->pixclock);
+ unsigned rate;
+ unsigned div = da8xx_fb_calc_clk_divider(par, mode->pixclock, &rate);
- da8xx_fb_config_clk_divider(div);
+ return da8xx_fb_config_clk_divider(par, div, rate);
+}
+
+static inline unsigned da8xx_fb_round_clk(struct da8xx_fb_par *par,
+ unsigned pixclock)
+{
+ unsigned div, rate;
+
+ div = da8xx_fb_calc_clk_divider(par, pixclock, &rate);
+ return KHZ2PICOS(rate / (1000 * div));
}
static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
@@ -723,7 +759,11 @@ static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
u32 bpp;
int ret = 0;
- da8xx_fb_calc_config_clk_divider(par, panel);
+ ret = da8xx_fb_calc_config_clk_divider(par, panel);
+ if (IS_ERR_VALUE(ret)) {
+ dev_err(par->dev, "unable to configure clock\n");
+ return ret;
+ }
if (panel->sync & FB_SYNC_CLK_INVERT)
lcdc_write((lcdc_read(LCD_RASTER_TIMING_2_REG) |
--
1.7.0.4
^ permalink raw reply related
* [PATCH 21/23] video: da8xx-fb: setup struct lcd_ctrl_config for dt
From: Darren Etheridge @ 2013-06-25 14:22 UTC (permalink / raw)
To: linux-fbdev
From: Afzal Mohammed <afzal@ti.com>
strcut lcd_ctrl_config information required for driver is currently
obtained via platform data. To handle DT probing, create
lcd_ctrl_config and populate it with default values, these values are
sufficient for the panels so far used with this controller to work.
Signed-off-by: Afzal Mohammed <afzal@ti.com>
Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
drivers/video/da8xx-fb.c | 34 +++++++++++++++++++++++++++++++++-
1 files changed, 33 insertions(+), 1 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 1c1a616..5455682 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1254,6 +1254,35 @@ static struct fb_ops da8xx_fb_ops = {
.fb_blank = cfb_blank,
};
+static struct lcd_ctrl_config *da8xx_fb_create_cfg(struct platform_device *dev)
+{
+ struct lcd_ctrl_config *cfg;
+
+ cfg = devm_kzalloc(&dev->dev, sizeof(struct fb_videomode), GFP_KERNEL);
+ if (!cfg) {
+ dev_err(&dev->dev, "memory allocation failed\n");
+ return NULL;
+ }
+
+ /* default values */
+
+ if (lcd_revision = LCD_VERSION_1)
+ cfg->bpp = 16;
+ else
+ cfg->bpp = 32;
+
+ /*
+ * For panels so far used with this LCDC, below statement is sufficient.
+ * For new panels, if required, struct lcd_ctrl_cfg fields to be updated
+ * with additional/modified values. Those values would have to be then
+ * obtained from dt(requiring new dt bindings).
+ */
+
+ cfg->panel_shade = COLOR_ACTIVE;
+
+ return cfg;
+}
+
static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
{
struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
@@ -1345,7 +1374,10 @@ static int fb_probe(struct platform_device *device)
break;
}
- lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
+ if (device->dev.of_node)
+ lcd_cfg = da8xx_fb_create_cfg(device);
+ else
+ lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
if (!lcd_cfg) {
ret = -EINVAL;
--
1.7.0.4
^ permalink raw reply related
* [PATCH 20/23] video: da8xx-fb: ensure pdata only for non-dt
From: Darren Etheridge @ 2013-06-25 14:22 UTC (permalink / raw)
To: linux-fbdev
From: Afzal Mohammed <afzal@ti.com>
This driver is DT probe-able, hence ensure presence of platform data
only for non-DT boot.
Signed-off-by: Afzal Mohammed <afzal@ti.com>
Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
drivers/video/da8xx-fb.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 0c68712..1c1a616 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1303,7 +1303,7 @@ static int fb_probe(struct platform_device *device)
int ret;
unsigned long ulcm;
- if (fb_pdata = NULL) {
+ if (fb_pdata = NULL && !device->dev.of_node) {
dev_err(&device->dev, "Can not get platform data\n");
return -ENOENT;
}
--
1.7.0.4
^ permalink raw reply related
* [PATCH 19/23] video: da8xx-fb: obtain fb_videomode info from dt
From: Darren Etheridge @ 2013-06-25 14:22 UTC (permalink / raw)
To: linux-fbdev
From: Afzal Mohammed <afzal@ti.com>
Obtain fb_videomode details for the connected lcd panel using the
display timing details present in DT.
Signed-off-by: Afzal Mohammed <afzal@ti.com>
Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
.../devicetree/bindings/video/fb-da8xx.txt | 21 ++++++++++++++++++++
drivers/video/da8xx-fb.c | 17 ++++++++++++++++
2 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
index 581e014..0741f78 100644
--- a/Documentation/devicetree/bindings/video/fb-da8xx.txt
+++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
@@ -6,6 +6,12 @@ Required properties:
AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
- reg: Address range of lcdc register set
- interrupts: lcdc interrupt
+- display-timings: typical videomode of lcd panel, represented as child.
+ Refer Documentation/devicetree/bindings/video/display-timing.txt for
+ display timing binding details. If multiple videomodes are mentioned
+ in display timings node, typical videomode has to be mentioned as the
+ native mode or it has to be first child (driver cares only for native
+ videomode).
Example:
@@ -13,4 +19,19 @@ lcdc@4830e000 {
compatible = "ti,am3352-lcdc", "ti,da830-lcdc";
reg = <0x4830e000 0x1000>;
interrupts = <36>;
+ display-timings {
+ 800x480p62 {
+ clock-frequency = <30000000>;
+ hactive = <800>;
+ vactive = <480>;
+ hfront-porch = <39>;
+ hback-porch = <39>;
+ hsync-len = <47>;
+ vback-porch = <29>;
+ vfront-porch = <13>;
+ vsync-len = <2>;
+ hsync-active = <1>;
+ vsync-active = <1>;
+ };
+ };
};
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 0beed20..0c68712 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 <video/of_display_timing.h>
#include <video/da8xx-fb.h>
#include <asm/div64.h>
@@ -1257,8 +1258,24 @@ static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
{
struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
struct fb_videomode *lcdc_info;
+ struct device_node *np = dev->dev.of_node;
int i;
+ if (np) {
+ lcdc_info = devm_kzalloc(&dev->dev,
+ sizeof(struct fb_videomode),
+ GFP_KERNEL);
+ if (!lcdc_info) {
+ dev_err(&dev->dev, "memory allocation failed\n");
+ return NULL;
+ }
+ if (of_get_fb_videomode(np, lcdc_info, OF_USE_NATIVE_MODE)) {
+ dev_err(&dev->dev, "timings not available in DT\n");
+ return NULL;
+ }
+ return lcdc_info;
+ }
+
for (i = 0, lcdc_info = known_lcd_panels;
i < ARRAY_SIZE(known_lcd_panels); i++, lcdc_info++) {
if (strcmp(fb_pdata->type, lcdc_info->name) = 0)
--
1.7.0.4
^ permalink raw reply related
* [PATCH 18/23] video: da8xx-fb: invoke platform callback safely
From: Darren Etheridge @ 2013-06-25 14:22 UTC (permalink / raw)
To: linux-fbdev
From: Afzal Mohammed <afzal@ti.com>
Ensure that platform data is present before checking whether platform
callback is present (the one used to control backlight). So far this
was not an issue as driver was purely non-DT triggered, but now DT
support has been added.
Signed-off-by: Afzal Mohammed <afzal@ti.com>
Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
drivers/video/da8xx-fb.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 08ee8eb..0beed20 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1347,7 +1347,7 @@ static int fb_probe(struct platform_device *device)
par->dev = &device->dev;
par->lcdc_clk = fb_clk;
par->lcd_fck_rate = clk_get_rate(fb_clk);
- if (fb_pdata->panel_power_ctrl) {
+ if (fb_pdata && fb_pdata->panel_power_ctrl) {
par->panel_power_ctrl = fb_pdata->panel_power_ctrl;
par->panel_power_ctrl(1);
}
--
1.7.0.4
^ permalink raw reply related
* [PATCH 17/23] video: da8xx-fb: minimal dt support
From: Darren Etheridge @ 2013-06-25 14:22 UTC (permalink / raw)
To: linux-fbdev
From: Afzal Mohammed <afzal@ti.com>
Driver is provided a means to have the probe triggered by DT.
Signed-off-by: Afzal Mohammed <afzal@ti.com>
Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
.../devicetree/bindings/video/fb-da8xx.txt | 16 ++++++++++++++++
drivers/video/da8xx-fb.c | 7 +++++++
2 files changed, 23 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt
diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
new file mode 100644
index 0000000..581e014
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
@@ -0,0 +1,16 @@
+TI LCD Controller on DA830/DA850/AM335x SoC's
+
+Required properties:
+- compatible:
+ DA830 - "ti,da830-lcdc"
+ AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
+- reg: Address range of lcdc register set
+- interrupts: lcdc interrupt
+
+Example:
+
+lcdc@4830e000 {
+ compatible = "ti,am3352-lcdc", "ti,da830-lcdc";
+ reg = <0x4830e000 0x1000>;
+ interrupts = <36>;
+};
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index b6ea5e9..08ee8eb 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1595,6 +1595,12 @@ static int fb_resume(struct platform_device *dev)
#define fb_resume NULL
#endif
+static const struct of_device_id da8xx_fb_of_match[] = {
+ {.compatible = "ti,da830-lcdc", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, da8xx_fb_of_match);
+
static struct platform_driver da8xx_fb_driver = {
.probe = fb_probe,
.remove = fb_remove,
@@ -1603,6 +1609,7 @@ static struct platform_driver da8xx_fb_driver = {
.driver = {
.name = DRIVER_NAME,
.owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(da8xx_fb_of_match),
},
};
--
1.7.0.4
^ permalink raw reply related
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