* RE: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Wang Dongsheng @ 2015-11-24 11:01 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
VGhhbmtzIFRvbWkuDQoNClJlZ2FyZHMsDQotRG9uZ3NoZW5nDQoNCj4gDQo+IE9uIDI0LzExLzE1
IDA4OjI3LCBEb25nc2hlbmcgV2FuZyB3cm90ZToNCj4gPiBGcm9tOiBXYW5nIERvbmdzaGVuZyA8
ZG9uZ3NoZW5nLndhbmdAZnJlZXNjYWxlLmNvbT4NCj4gPg0KPiA+IElmIGRpdV9vcHMgaXMgbm90
IGltcGxlbWVudGVkIG9uIHBsYXRmb3JtLCBrZXJuZWwgd2lsbCBhY2Nlc3MgYSBudWxsDQo+ID4g
cG9pbnRlci4gd2UgbmVlZCB0byBjaGVjayB0aGlzIHBvaW50ZXIgaW4gZGl1IGluaXRpYWxpemF0
aW9uLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTogV2FuZyBEb25nc2hlbmcgPGRvbmdzaGVuZy53
YW5nQGZyZWVzY2FsZS5jb20+DQo+ID4NCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy92aWRlby9m
YmRldi9mc2wtZGl1LWZiLmMNCj4gPiBiL2RyaXZlcnMvdmlkZW8vZmJkZXYvZnNsLWRpdS1mYi5j
DQo+ID4gaW5kZXggYjMzNWMxYS4uMjg4YjVlNCAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL3Zp
ZGVvL2ZiZGV2L2ZzbC1kaXUtZmIuYw0KPiA+ICsrKyBiL2RyaXZlcnMvdmlkZW8vZmJkZXYvZnNs
LWRpdS1mYi5jDQo+ID4gQEAgLTQ3OSw3ICs0NzksMTAgQEAgc3RhdGljIGVudW0gZnNsX2RpdV9t
b25pdG9yX3BvcnQNCj4gZnNsX2RpdV9uYW1lX3RvX3BvcnQoY29uc3QgY2hhciAqcykNCj4gPiAg
CQkJcG9ydCA9IEZTTF9ESVVfUE9SVF9ETFZEUzsNCj4gPiAgCX0NCj4gPg0KPiA+IC0JcmV0dXJu
IGRpdV9vcHMudmFsaWRfbW9uaXRvcl9wb3J0KHBvcnQpOw0KPiA+ICsJaWYgKGRpdV9vcHMudmFs
aWRfbW9uaXRvcl9wb3J0KQ0KPiA+ICsJCXBvcnQgPSBkaXVfb3BzLnZhbGlkX21vbml0b3JfcG9y
dChwb3J0KTsNCj4gPiArDQo+ID4gKwlyZXR1cm4gcG9ydDsNCj4gPiAgfQ0KPiA+DQo+ID4gIC8q
DQo+ID4gQEAgLTE2OTcsNiArMTcwMCw5IEBAIHN0YXRpYyBpbnQgZnNsX2RpdV9wcm9iZShzdHJ1
Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2KQ0KPiA+ICAJdW5zaWduZWQgaW50IGk7DQo+ID4gIAlp
bnQgcmV0Ow0KPiA+DQo+ID4gKwlpZiAoIWRpdV9vcHMuc2V0X3BpeGVsX2Nsb2NrKQ0KPiA+ICsJ
CXJldHVybiAtRU5PREVWOw0KPiA+ICsNCj4gPiAgCWRhdGEgPSBkbWFtX2FsbG9jX2NvaGVyZW50
KCZwZGV2LT5kZXYsIHNpemVvZihzdHJ1Y3QgZnNsX2RpdV9kYXRhKSwNCj4gPiAgCQkJCSAgICZk
bWFfYWRkciwgR0ZQX0RNQSB8IF9fR0ZQX1pFUk8pOw0KPiA+ICAJaWYgKCFkYXRhKQ0KPiA+DQo+
IA0KPiBUaGFua3MsIHF1ZXVlZCBmb3IgNC41Lg0KPiANCj4gIFRvbWkNCg0K
^ permalink raw reply
* Re: [PATCH] video: constify geode ops structures
From: Tomi Valkeinen @ 2015-11-24 11:28 UTC (permalink / raw)
To: Julia Lawall, Jean-Christophe Plagniol-Villard
Cc: kernel-janitors, linux-geode, linux-fbdev, linux-kernel
In-Reply-To: <1447018493-20631-1-git-send-email-Julia.Lawall@lip6.fr>
[-- Attachment #1: Type: text/plain, Size: 598 bytes --]
On 08/11/15 23:34, Julia Lawall wrote:
> These geode ops structures are never modified, so declare them as const.
>
> Done with the help of Coccinelle.
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
> drivers/video/fbdev/geode/display_gx1.c | 2 +-
> drivers/video/fbdev/geode/display_gx1.h | 2 +-
> drivers/video/fbdev/geode/geodefb.h | 4 ++--
> drivers/video/fbdev/geode/video_cs5530.c | 2 +-
> drivers/video/fbdev/geode/video_cs5530.h | 2 +-
> 5 files changed, 6 insertions(+), 6 deletions(-)
Thanks, queued for 4.5.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: DISPC: Remove boolean comparisons
From: Tomi Valkeinen @ 2015-11-24 11:35 UTC (permalink / raw)
To: Luis de Bethencourt, linux-kernel
Cc: plagnioj, laurent.pinchart, linux-omap, linux-fbdev
In-Reply-To: <1444912178-15989-1-git-send-email-luisbg@osg.samsung.com>
[-- Attachment #1: Type: text/plain, Size: 463 bytes --]
On 15/10/15 15:29, Luis de Bethencourt wrote:
> Boolean tests do not need explicit comparison to true or false.
>
> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
> ---
> drivers/video/fbdev/omap2/dss/dispc-compat.c | 6 +++---
> drivers/video/fbdev/omap2/dss/dispc.c | 6 +++---
> drivers/video/fbdev/omap2/dss/manager.c | 2 +-
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
Thanks, queued for 4.5.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] video: omapdss: delete unneeded of_node_put
From: Tomi Valkeinen @ 2015-11-24 11:37 UTC (permalink / raw)
To: Julia Lawall
Cc: kernel-janitors, Jean-Christophe Plagniol-Villard, linux-omap,
linux-fbdev, linux-kernel, Russell King - ARM Linux,
Thomas Petazzoni, Andrew Lunn, Bjorn Helgaas, Jason Cooper
In-Reply-To: <1444682596-3065-2-git-send-email-Julia.Lawall@lip6.fr>
[-- Attachment #1: Type: text/plain, Size: 759 bytes --]
On 12/10/15 23:43, Julia Lawall wrote:
> Device node iterators perform an of_node_put on each iteration, so putting
> an of_node_put before a continue results in a double put.
>
> A simplified version of the semantic match that finds this problem is as
> follows (http://coccinelle.lip6.fr):
>
> // <smpl>
> @@
> expression root,e;
> local idexpression child;
> iterator i;
> @@
>
> i(..., child, ...) {
> ... when != of_node_get(child)
> * of_node_put(child);
> ...
> * continue;
> }
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
> drivers/video/fbdev/omap2/dss/omapdss-boot-init.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
Thanks, queued for 4.5.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Timur Tabi @ 2015-11-24 16:04 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, Nov 24, 2015 at 5:46 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 24/11/15 08:27, Dongsheng Wang wrote:
>> @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device *pdev)
>> unsigned int i;
>> int ret;
>>
>> + if (!diu_ops.set_pixel_clock)
>> + return -ENODEV;
>> +
>> data = dmam_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
>> &dma_addr, GFP_DMA | __GFP_ZERO);
>> if (!data)
>>
>
> Thanks, queued for 4.5.
Could you please wait for me to review the patch first? I am the
maintainer for the driver, and I see a problem with it.
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Timur Tabi @ 2015-11-24 16:12 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, Nov 24, 2015 at 1:27 AM, Dongsheng Wang
<dongsheng.wang@freescale.com> wrote:
> @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device *pdev)
> unsigned int i;
> int ret;
>
> + if (!diu_ops.set_pixel_clock)
> + return -ENODEV;
> +
> data = dmam_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
> &dma_addr, GFP_DMA | __GFP_ZERO);
> if (!data)
This doesn't make any sense. If set_pixel_clock() is not defined,
then the whole driver aborts the probe. When could that ever happen?
If the platform code does not exist, then don't let the driver be
probed.
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Scott Wood @ 2015-11-24 16:15 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, 2015-11-24 at 11:12 -0500, Timur Tabi wrote:
> On Tue, Nov 24, 2015 at 1:27 AM, Dongsheng Wang
> <dongsheng.wang@freescale.com> wrote:
> > @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device
> > *pdev)
> > unsigned int i;
> > int ret;
> >
> > + if (!diu_ops.set_pixel_clock)
> > + return -ENODEV;
> > +
> > data = dmam_alloc_coherent(&pdev->dev, sizeof(struct
> > fsl_diu_data),
> > &dma_addr, GFP_DMA | __GFP_ZERO);
> > if (!data)
>
> This doesn't make any sense. If set_pixel_clock() is not defined,
> then the whole driver aborts the probe.
That's what this patch is trying to accomplish. Currently it crashes instead.
> When could that ever happen?
> If the platform code does not exist, then don't let the driver be
> probed.
How do you propose to accomplish that other than with such a check?
-Scott
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Tomi Valkeinen @ 2015-11-24 16:15 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
[-- Attachment #1: Type: text/plain, Size: 920 bytes --]
On 24/11/15 18:04, Timur Tabi wrote:
> On Tue, Nov 24, 2015 at 5:46 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On 24/11/15 08:27, Dongsheng Wang wrote:
>>> @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device *pdev)
>>> unsigned int i;
>>> int ret;
>>>
>>> + if (!diu_ops.set_pixel_clock)
>>> + return -ENODEV;
>>> +
>>> data = dmam_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
>>> &dma_addr, GFP_DMA | __GFP_ZERO);
>>> if (!data)
>>>
>>
>> Thanks, queued for 4.5.
>
> Could you please wait for me to review the patch first? I am the
> maintainer for the driver, and I see a problem with it.
Sorry, I was too hasty (and tired). I thought I was looking at a patch
that's been on the list for a while, but apparently it was only posted
today...
Anyway, dropped this.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5] pwm-backlight: Avoid backlight flicker when probed from DT
From: Lee Jones @ 2015-11-24 16:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1447866745-21567-1-git-send-email-p.zabel@pengutronix.de>
On Wed, 18 Nov 2015, Philipp Zabel wrote:
> If the driver is probed from the device tree, and there is a phandle
> property set on it, and the enable GPIO is already configured as output,
> and the backlight is currently disabled, keep it disabled.
> If all these conditions are met, assume there will be some other driver
> that can enable the backlight at the appropriate time.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> ---
> - Rebased onto v4.4-rc1
> - Added Heiko's Tested-by
> ---
> drivers/video/backlight/pwm_bl.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
Applied, thanks.
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index ae3c6b6..3daf9cc 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -199,6 +199,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> struct backlight_properties props;
> struct backlight_device *bl;
> struct pwm_bl_data *pb;
> + phandle phandle = pdev->dev.of_node->phandle;
> + int initial_blank = FB_BLANK_UNBLANK;
> int ret;
>
> if (!data) {
> @@ -242,7 +244,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> pb->enabled = false;
>
> pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> - GPIOD_OUT_HIGH);
> + GPIOD_ASIS);
> if (IS_ERR(pb->enable_gpio)) {
> ret = PTR_ERR(pb->enable_gpio);
> goto err_alloc;
> @@ -264,12 +266,30 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> pb->enable_gpio = gpio_to_desc(data->enable_gpio);
> }
>
> + if (pb->enable_gpio) {
> + /*
> + * If the driver is probed from the device tree and there is a
> + * phandle link pointing to the backlight node, it is safe to
> + * assume that another driver will enable the backlight at the
> + * appropriate time. Therefore, if it is disabled, keep it so.
> + */
> + if (phandle &&
> + gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT &&
> + gpiod_get_value(pb->enable_gpio) = 0)
> + initial_blank = FB_BLANK_POWERDOWN;
> + else
> + gpiod_direction_output(pb->enable_gpio, 1);
> + }
> +
> pb->power_supply = devm_regulator_get(&pdev->dev, "power");
> if (IS_ERR(pb->power_supply)) {
> ret = PTR_ERR(pb->power_supply);
> goto err_alloc;
> }
>
> + if (phandle && !regulator_is_enabled(pb->power_supply))
> + initial_blank = FB_BLANK_POWERDOWN;
> +
> pb->pwm = devm_pwm_get(&pdev->dev, NULL);
> if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER
> && !pdev->dev.of_node) {
> @@ -320,6 +340,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> }
>
> bl->props.brightness = data->dft_brightness;
> + bl->props.power = initial_blank;
> backlight_update_status(bl);
>
> platform_set_drvdata(pdev, bl);
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] backlight: tps65217_bl: Add MODULE_DEVICE_TABLE.
From: Lee Jones @ 2015-11-24 16:31 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
jingoohan1-Re5JQEeQqe8AvxtiuMwx3w,
plagnioj-sclMFOaUSTBWk0Htik3J/w, tomi.valkeinen-l0cyMroinI0,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1447417148-733-1-git-send-email-enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
On Fri, 13 Nov 2015, Enric Balletbo i Serra wrote:
> The device table is required to load modules based on modaliases.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> drivers/video/backlight/tps65217_bl.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/video/backlight/tps65217_bl.c b/drivers/video/backlight/tps65217_bl.c
> index 61d72bf..37a9731 100644
> --- a/drivers/video/backlight/tps65217_bl.c
> +++ b/drivers/video/backlight/tps65217_bl.c
> @@ -320,10 +320,21 @@ static int tps65217_bl_probe(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id tps65217_bl_of_match[] = {
> + { .compatible = "ti,tps65217-bl", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, tps65217_bl_of_match);
> +#endif
> +
> static struct platform_driver tps65217_bl_driver = {
> .probe = tps65217_bl_probe,
> .driver = {
> .name = "tps65217-bl",
> +#ifdef CONFIG_OF
> + .of_match_table = tps65217_bl_of_match,
> +#endif
Please use of_match_ptr() instead.
> },
> };
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Timur Tabi @ 2015-11-24 16:54 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, Nov 24, 2015 at 11:15 AM, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, 2015-11-24 at 11:12 -0500, Timur Tabi wrote:
>> On Tue, Nov 24, 2015 at 1:27 AM, Dongsheng Wang
>> <dongsheng.wang@freescale.com> wrote:
>> > @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device
>> > *pdev)
>> > unsigned int i;
>> > int ret;
>> >
>> > + if (!diu_ops.set_pixel_clock)
>> > + return -ENODEV;
>> > +
>> > data = dmam_alloc_coherent(&pdev->dev, sizeof(struct
>> > fsl_diu_data),
>> > &dma_addr, GFP_DMA | __GFP_ZERO);
>> > if (!data)
>>
>> This doesn't make any sense. If set_pixel_clock() is not defined,
>> then the whole driver aborts the probe.
>
> That's what this patch is trying to accomplish. Currently it crashes instead.
>
>> When could that ever happen?
>> If the platform code does not exist, then don't let the driver be
>> probed.
>
> How do you propose to accomplish that other than with such a check?
Well, if you're concern is that there's no platform code, then there
should be a check that says, "see if there's any platform code", not
"let's check this obscure function and abort without explanation if
it's not initialized."
Alternatively, why can't you just do this, in update_lcdc():
>
> -Scott
>
> --
> 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] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Scott Wood @ 2015-11-24 16:55 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, 2015-11-24 at 11:54 -0500, Timur Tabi wrote:
> On Tue, Nov 24, 2015 at 11:15 AM, Scott Wood <scottwood@freescale.com>
> wrote:
> > On Tue, 2015-11-24 at 11:12 -0500, Timur Tabi wrote:
> > > On Tue, Nov 24, 2015 at 1:27 AM, Dongsheng Wang
> > > <dongsheng.wang@freescale.com> wrote:
> > > > @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device
> > > > *pdev)
> > > > unsigned int i;
> > > > int ret;
> > > >
> > > > + if (!diu_ops.set_pixel_clock)
> > > > + return -ENODEV;
> > > > +
> > > > data = dmam_alloc_coherent(&pdev->dev, sizeof(struct
> > > > fsl_diu_data),
> > > > &dma_addr, GFP_DMA | __GFP_ZERO);
> > > > if (!data)
> > >
> > > This doesn't make any sense. If set_pixel_clock() is not defined,
> > > then the whole driver aborts the probe.
> >
> > That's what this patch is trying to accomplish. Currently it crashes
> > instead.
> >
> > > When could that ever happen?
> > > If the platform code does not exist, then don't let the driver be
> > > probed.
> >
> > How do you propose to accomplish that other than with such a check?
>
> Well, if you're concern is that there's no platform code, then there
> should be a check that says, "see if there's any platform code", not
> "let's check this obscure function and abort without explanation if
> it's not initialized."
Do you have a *specific* better way to "see if there's any platform code"?
>
> Alternatively, why can't you just do this, in update_lcdc():
>
>
>
Do nothing?
-Scott
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Timur Tabi @ 2015-11-24 16:56 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, Nov 24, 2015 at 11:54 AM, Timur Tabi <timur@tabi.org> wrote:
> Well, if you're concern is that there's no platform code, then there
> should be a check that says, "see if there's any platform code", not
> "let's check this obscure function and abort without explanation if
> it's not initialized."
>
> Alternatively, why can't you just do this, in update_lcdc():
[Stupid gmail sent my message before I was done typing]
if (diu_ops.set_pixel_clock)
diu_ops.set_pixel_clock(var->pixclock);
^ permalink raw reply
* RE: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Wang Dongsheng @ 2015-11-24 16:57 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
SGkgVGltdXIsDQoNClRoYW5rcyBmb3IgeW91ciByZXZpZXcuDQoNCj4gIE9uIFR1ZSwgTm92IDI0
LCAyMDE1IGF0IDE6MjcgQU0sIERvbmdzaGVuZyBXYW5nDQo+IDxkb25nc2hlbmcud2FuZ0BmcmVl
c2NhbGUuY29tPiB3cm90ZToNCj4gPiBAQCAtMTY5Nyw2ICsxNzAwLDkgQEAgc3RhdGljIGludCBm
c2xfZGl1X3Byb2JlKHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKnBkZXYpDQo+ID4gICAgICAgICB1
bnNpZ25lZCBpbnQgaTsNCj4gPiAgICAgICAgIGludCByZXQ7DQo+ID4NCj4gPiArICAgICAgIGlm
ICghZGl1X29wcy5zZXRfcGl4ZWxfY2xvY2spDQo+ID4gKyAgICAgICAgICAgICAgIHJldHVybiAt
RU5PREVWOw0KPiA+ICsNCj4gPiAgICAgICAgIGRhdGEgPSBkbWFtX2FsbG9jX2NvaGVyZW50KCZw
ZGV2LT5kZXYsIHNpemVvZihzdHJ1Y3QgZnNsX2RpdV9kYXRhKSwNCj4gPiAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICZkbWFfYWRkciwgR0ZQX0RNQSB8IF9fR0ZQX1pFUk8pOw0K
PiA+ICAgICAgICAgaWYgKCFkYXRhKQ0KPiANCj4gVGhpcyBkb2Vzbid0IG1ha2UgYW55IHNlbnNl
LiAgSWYgc2V0X3BpeGVsX2Nsb2NrKCkgaXMgbm90IGRlZmluZWQsDQo+IHRoZW4gdGhlIHdob2xl
IGRyaXZlciBhYm9ydHMgdGhlIHByb2JlLiAgV2hlbiBjb3VsZCB0aGF0IGV2ZXIgaGFwcGVuPw0K
PiBJZiB0aGUgcGxhdGZvcm0gY29kZSBkb2VzIG5vdCBleGlzdCwgdGhlbiBkb24ndCBsZXQgdGhl
IGRyaXZlciBiZQ0KPiBwcm9iZWQuDQoNCkFub3RoZXIgcGF0Y2ggdmlkZW86IGZiZGV2OiBmc2w6
IFNwbGl0IERJVSBpbml0aWFsaXphdGlvbiBlbnRyeTpbaHR0cHM6Ly9wYXRjaHdvcmsua2VybmVs
Lm9yZy9wYXRjaC83MzgxMzUxL10NCm1vZHVsZV9pbml0KGZzbF9kaXVfaW5pdCkgd2lsbCBiZSBy
ZXBsYWNlLCBhbmQgYWxsIG9mIHRoZSBpbml0aWFsaXphdGlvbiB3aWxsIGJlIGNvbXBsZXRlZCBp
biB0aGUgcHJvYmUNCmluY2x1ZGUgdGhpcyBjaGVjay4gU28ganVzdCBkbyBhIHF1aWNrIGZpeCBm
b3IgdGhpcyBib290IGtlcm5lbCBjcmFzaCBpc3N1ZS4NCg0KUmVnYXJkcywNCi1Eb25nc2hlbmcN
Cg=
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Scott Wood @ 2015-11-24 16:59 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, 2015-11-24 at 18:15 +0200, Tomi Valkeinen wrote:
>
> On 24/11/15 18:04, Timur Tabi wrote:
> > On Tue, Nov 24, 2015 at 5:46 AM, Tomi Valkeinen <tomi.valkeinen@ti.com>
> > wrote:
> > > On 24/11/15 08:27, Dongsheng Wang wrote:
> > > > @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device
> > > > *pdev)
> > > > unsigned int i;
> > > > int ret;
> > > >
> > > > + if (!diu_ops.set_pixel_clock)
> > > > + return -ENODEV;
> > > > +
> > > > data = dmam_alloc_coherent(&pdev->dev, sizeof(struct
> > > > fsl_diu_data),
> > > > &dma_addr, GFP_DMA | __GFP_ZERO);
> > > > if (!data)
> > > >
> > >
> > > Thanks, queued for 4.5.
> >
> > Could you please wait for me to review the patch first? I am the
> > maintainer for the driver, and I see a problem with it.
>
> Sorry, I was too hasty (and tired). I thought I was looking at a patch
> that's been on the list for a while, but apparently it was only posted
> today...
>
> Anyway, dropped this.
Also, this is a bugfix (certain platforms are currently crashing on boot) and
once review is settled should go into 4.4.
-Scott
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Scott Wood @ 2015-11-24 17:00 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, 2015-11-24 at 11:56 -0500, Timur Tabi wrote:
> On Tue, Nov 24, 2015 at 11:54 AM, Timur Tabi <timur@tabi.org> wrote:
> > Well, if you're concern is that there's no platform code, then there
> > should be a check that says, "see if there's any platform code", not
> > "let's check this obscure function and abort without explanation if
> > it's not initialized."
> >
> > Alternatively, why can't you just do this, in update_lcdc():
>
> [Stupid gmail sent my message before I was done typing]
>
> if (diu_ops.set_pixel_clock)
> diu_ops.set_pixel_clock(var->pixclock);
Because it's more obviously correct to abort the probe than to continue with
some operations nooped. This is meant to be a quick and obvious fix to the
crashing bug. There's another patch pending to reorganize the init of this
driver.
FWIW, I don't like the fact that this driver requires "platform code" at all.
Why isn't it self-contained, with knowledge of the relevant boards?
-Scott
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Timur Tabi @ 2015-11-24 17:02 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, Nov 24, 2015 at 11:55 AM, Scott Wood <scottwood@freescale.com> wrote:
>> Well, if you're concern is that there's no platform code, then there
>> should be a check that says, "see if there's any platform code", not
>> "let's check this obscure function and abort without explanation if
>> it's not initialized."
>
> Do you have a *specific* better way to "see if there's any platform code"?
Well, for one thing, the check should be done in the _init function,
not the _probe. Secondly, it should be documented as such, e.g. "/*
Check to see that we have platform code that initializes diu_ops. If
not, then abort. */". Third, you should probably add a boolean field
to platform_diu_data_ops that gets set to True if/when the platform
code initializes the rest of the structure.
Of course, an even better solution would be to get rid of the global
structure altogether and come up with something more robust, but I
understand that that's overkill.
^ permalink raw reply
* RE: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Wang Dongsheng @ 2015-11-24 17:02 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
PiBPbiBUdWUsIE5vdiAyNCwgMjAxNSBhdCAxMTo1NCBBTSwgVGltdXIgVGFiaSA8dGltdXJAdGFi
aS5vcmc+IHdyb3RlOg0KPiA+IFdlbGwsIGlmIHlvdSdyZSBjb25jZXJuIGlzIHRoYXQgdGhlcmUn
cyBubyBwbGF0Zm9ybSBjb2RlLCB0aGVuIHRoZXJlDQo+ID4gc2hvdWxkIGJlIGEgY2hlY2sgdGhh
dCBzYXlzLCAic2VlIGlmIHRoZXJlJ3MgYW55IHBsYXRmb3JtIGNvZGUiLCBub3QNCj4gPiAibGV0
J3MgY2hlY2sgdGhpcyBvYnNjdXJlIGZ1bmN0aW9uIGFuZCBhYm9ydCB3aXRob3V0IGV4cGxhbmF0
aW9uIGlmDQo+ID4gaXQncyBub3QgaW5pdGlhbGl6ZWQuIg0KPiA+DQo+ID4gQWx0ZXJuYXRpdmVs
eSwgd2h5IGNhbid0IHlvdSBqdXN0IGRvIHRoaXMsIGluIHVwZGF0ZV9sY2RjKCk6DQo+IA0KPiBb
U3R1cGlkIGdtYWlsIHNlbnQgbXkgbWVzc2FnZSBiZWZvcmUgSSB3YXMgZG9uZSB0eXBpbmddDQo+
IA0KPiBpZiAoZGl1X29wcy5zZXRfcGl4ZWxfY2xvY2spDQo+ICAgIGRpdV9vcHMuc2V0X3BpeGVs
X2Nsb2NrKHZhci0+cGl4Y2xvY2spOw0KDQpIZXJlIGFyZSBub3QgZnJpZW5kbHkgZm9yIGtlcm5l
bCwgYmVjYXVzZSBpZiB3ZSBsb3N0IHRoZSBjbG9jayB3ZSBjYW5ub3QNCnRvIGRpc3BsYXkuIFdl
IGRvIHNvIG11Y2ggdGhpbmdzIGluIGtlcm5lbCBidXQgd2Ugc3RpbGwgY2Fubm90IHRvIGRpc3Bs
YXkgdGhhdA0KaXMgYmVmb3JlIHdlIGNhbiBrbm93IGl0Li4uU28gc3RpbGwgdGhpbmsgd2UgbmVl
ZCB0byBjaGVjayB0aGUgaG9vayBpbiBpbml0aWFsaXphdGlvbg0KZmxvdy4NCg0KUmVnYXJkcywN
Ci1Eb25nc2hlbmcNCg=
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Timur Tabi @ 2015-11-24 17:05 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, Nov 24, 2015 at 12:00 PM, Scott Wood <scottwood@freescale.com> wrote:
>
> FWIW, I don't like the fact that this driver requires "platform code" at all.
> Why isn't it self-contained, with knowledge of the relevant boards?
Yeah, I was never crazy about that either. To make the change you
want, we would need Dongsheng's other patch that moves all hardware
init into the _probe function first. And then we would need to get
rid of the #ifdefs for 5121. I'm all for that, but at the time this
code was written, no one expected Freescale to continue using the DIU
after the P1022.
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Scott Wood @ 2015-11-24 17:05 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, 2015-11-24 at 12:02 -0500, Timur Tabi wrote:
> On Tue, Nov 24, 2015 at 11:55 AM, Scott Wood <scottwood@freescale.com>
> wrote:
>
> > > Well, if you're concern is that there's no platform code, then there
> > > should be a check that says, "see if there's any platform code", not
> > > "let's check this obscure function and abort without explanation if
> > > it's not initialized."
> >
> > Do you have a *specific* better way to "see if there's any platform code"?
>
> Well, for one thing, the check should be done in the _init function,
> not the _probe.
I asked Dongsheng to put it in probe() during internal review because at the
time he was printing an error, and I didn't want the error to be printed if
the device wasn't present. Again, there's another non-bugfix patch pending
that moves all the rest into probe() where it belongs.
> Secondly, it should be documented as such, e.g. "/*
> Check to see that we have platform code that initializes diu_ops. If
> not, then abort. */".
OK.
> Third, you should probably add a boolean field
> to platform_diu_data_ops that gets set to True if/when the platform
> code initializes the rest of the structure.
Why do you want to complicate a simple bugfix with a requirement to modify all
platforms that use the driver, introducing a possible regression if one is
missed?
-Scott
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Timur Tabi @ 2015-11-24 17:06 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, Nov 24, 2015 at 12:02 PM, Wang Dongsheng
<Dongsheng.Wang@freescale.com> wrote:
>
> Here are not friendly for kernel, because if we lost the clock we cannot
> to display. We do so much things in kernel but we still cannot to display that
> is before we can know it...So still think we need to check the hook in initialization
> flow.
Ok, I agree with that. But the check has to be more obvious, and I
think it should be done in the _init code.
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Scott Wood @ 2015-11-24 17:10 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, 2015-11-24 at 10:59 -0600, Scott Wood wrote:
> On Tue, 2015-11-24 at 18:15 +0200, Tomi Valkeinen wrote:
> >
> > On 24/11/15 18:04, Timur Tabi wrote:
> > > On Tue, Nov 24, 2015 at 5:46 AM, Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > wrote:
> > > > On 24/11/15 08:27, Dongsheng Wang wrote:
> > > > > @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct
> > > > > platform_device
> > > > > *pdev)
> > > > > unsigned int i;
> > > > > int ret;
> > > > >
> > > > > + if (!diu_ops.set_pixel_clock)
> > > > > + return -ENODEV;
> > > > > +
> > > > > data = dmam_alloc_coherent(&pdev->dev, sizeof(struct
> > > > > fsl_diu_data),
> > > > > &dma_addr, GFP_DMA | __GFP_ZERO);
> > > > > if (!data)
> > > > >
> > > >
> > > > Thanks, queued for 4.5.
> > >
> > > Could you please wait for me to review the patch first? I am the
> > > maintainer for the driver, and I see a problem with it.
> >
> > Sorry, I was too hasty (and tired). I thought I was looking at a patch
> > that's been on the list for a while, but apparently it was only posted
> > today...
> >
> > Anyway, dropped this.
>
> Also, this is a bugfix (certain platforms are currently crashing on boot)
> and
> once review is settled should go into 4.4.
...and stable, since the defconfig changes that enabled this driver on the
problematic platforms went into 4.3.
-Scott
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Timur Tabi @ 2015-11-24 17:16 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, Nov 24, 2015 at 12:05 PM, Scott Wood <scottwood@freescale.com> wrote:
> I asked Dongsheng to put it in probe() during internal review because at the
> time he was printing an error, and I didn't want the error to be printed if
> the device wasn't present. Again, there's another non-bugfix patch pending
> that moves all the rest into probe() where it belongs.
I think it should be in _init, and not display an error.
>> Third, you should probably add a boolean field
>> to platform_diu_data_ops that gets set to True if/when the platform
>> code initializes the rest of the structure.
>
> Why do you want to complicate a simple bugfix with a requirement to modify all
> platforms that use the driver, introducing a possible regression if one is
> missed?
Fair enough, but I think it should at least be documented by saying
something about set_pixel_clock must be defined, so if it isn't, then
that means the platform code does not support DIU at all, so just
abort.
^ permalink raw reply
* Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented
From: Scott Wood @ 2015-11-24 17:17 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com>
On Tue, 2015-11-24 at 12:16 -0500, Timur Tabi wrote:
> On Tue, Nov 24, 2015 at 12:05 PM, Scott Wood <scottwood@freescale.com>
> wrote:
>
> > I asked Dongsheng to put it in probe() during internal review because at
> > the
> > time he was printing an error, and I didn't want the error to be printed
> > if
> > the device wasn't present. Again, there's another non-bugfix patch
> > pending
> > that moves all the rest into probe() where it belongs.
>
> I think it should be in _init, and not display an error.
As long as it doesn't display anything I don't care much either way.
> > > Third, you should probably add a boolean field
> > > to platform_diu_data_ops that gets set to True if/when the platform
> > > code initializes the rest of the structure.
> >
> > Why do you want to complicate a simple bugfix with a requirement to modify
> > all
> > platforms that use the driver, introducing a possible regression if one is
> > missed?
>
> Fair enough, but I think it should at least be documented by saying
> something about set_pixel_clock must be defined, so if it isn't, then
> that means the platform code does not support DIU at all, so just
> abort.
Sure.
-Scott
^ permalink raw reply
* [PATCH RESEND] fbdev: Make fb-notify a no-op if CONFIG_FB=n
From: Ezequiel Garcia @ 2015-11-25 3:11 UTC (permalink / raw)
To: linux-fbdev
There's no point in having support for framebuffer notifications
is CONFIG_FB is disabled. This commit adds the necessary stubs
for code to link properly when CONFIG_FB=n and moves fb-notify.o
to be built only when CONFIG_FB=y.
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
drivers/video/fbdev/Kconfig | 4 ++++
drivers/video/fbdev/core/Makefile | 2 +-
include/linux/fb.h | 18 ++++++++++++++++++
3 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index e6d16d65e4e6..6d881260856e 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -5,6 +5,7 @@
menuconfig FB
tristate "Support for frame buffer devices"
select FB_CMDLINE
+ select FB_NOTIFY
---help---
The frame buffer device provides an abstraction for the graphics
hardware. It represents the frame buffer of some video hardware and
@@ -56,6 +57,9 @@ config FIRMWARE_EDID
config FB_CMDLINE
bool
+config FB_NOTIFY
+ bool
+
config FB_DDC
tristate
depends on FB
diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
index 23d86a8b7d7b..9e3ddf225393 100644
--- a/drivers/video/fbdev/core/Makefile
+++ b/drivers/video/fbdev/core/Makefile
@@ -1,5 +1,5 @@
-obj-y += fb_notify.o
obj-$(CONFIG_FB_CMDLINE) += fb_cmdline.o
+obj-$(CONFIG_FB_NOTIFY) += fb_notify.o
obj-$(CONFIG_FB) += fb.o
fb-y := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
modedb.o fbcvt.o
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 3d003805aac3..55433f86f0a3 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -175,9 +175,27 @@ struct fb_blit_caps {
u32 flags;
};
+#ifdef CONFIG_FB_NOTIFY
extern int fb_register_client(struct notifier_block *nb);
extern int fb_unregister_client(struct notifier_block *nb);
extern int fb_notifier_call_chain(unsigned long val, void *v);
+#else
+static inline int fb_register_client(struct notifier_block *nb)
+{
+ return 0;
+};
+
+static inline int fb_unregister_client(struct notifier_block *nb)
+{
+ return 0;
+};
+
+static inline int fb_notifier_call_chain(unsigned long val, void *v)
+{
+ return 0;
+};
+#endif
+
/*
* Pixmap structure definition
*
--
2.6.2
^ 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