* Re: [PATCH 19/20] OMAPDSS: DSI: fix DSI channel source initialization
From: Tomi Valkeinen @ 2013-03-11 7:02 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <513D754A.4000607@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]
On 2013-03-11 08:10, Archit Taneja wrote:
> Hi,
>
> On Friday 08 March 2013 05:21 PM, Tomi Valkeinen wrote:
>> During the initialization of the DSI protocol registers, we always set
>> the sources of all DSI channels to L4. However, we don't update the
>> value in the dsi_data, so we may end up with a different value in the
>> register and in the dsi_data, leading to DSI problems.
>>
>> This patch fixes the issue by initializing also the channel source in
>> the dsi_data.
>
> We set in omap_dsihw_probe:
>
> static int __init omap_dsihw_probe(struct platform_device *dsidev)
> {
> ...
> ...
> /* DSI VCs initialization */
> for (i = 0; i < ARRAY_SIZE(dsi->vc); i++) {
> dsi->vc[i].source = DSI_VC_SOURCE_L4;
> dsi->vc[i].dssdev = NULL;
> dsi->vc[i].vc_id = 0;
> }
> ...
> ...
> }
Hmm... I did have a bug related to this when prototyping CDF. Ah.
Consider this:
Panel powers up and uses DSI normally. A DSI VC is set to video mode.
Then the panel power down. Then it powers up again, and enables DSI. At
this time, dsi_vc_initial_config() is called again, setting the source
in the registers to L4. But the source in dsi_data is still VP.
So perhaps the whole piece of code from omap_dsihw_probe should be moved
to somewhere else (dsi_vc_initial_config() sounds like a good place), so
that they are initialized each time the registers are initialized.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply
* [PATCH] matrox: fix coccicheck warning in matroxfb_base.c
From: Silviu-Mihai Popescu @ 2013-03-11 7:32 UTC (permalink / raw)
To: linux-fbdev; +Cc: FlorianSchandinat, linux-kernel, Silviu-Mihai Popescu
This replaces a call to kmalloc() followed by memset() with just one
call to kzalloc().
Signed-off-by: Silviu-Mihai Popescu <silviupopescu1990@gmail.com>
---
drivers/video/matrox/matroxfb_base.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/video/matrox/matroxfb_base.c b/drivers/video/matrox/matroxfb_base.c
index 401a56e..2456529 100644
--- a/drivers/video/matrox/matroxfb_base.c
+++ b/drivers/video/matrox/matroxfb_base.c
@@ -2029,10 +2029,9 @@ static int matroxfb_probe(struct pci_dev* pdev, const struct pci_device_id* dumm
return -1;
}
- minfo = kmalloc(sizeof(*minfo), GFP_KERNEL);
+ minfo = kzalloc(sizeof(*minfo), GFP_KERNEL);
if (!minfo)
return -1;
- memset(minfo, 0, sizeof(*minfo));
minfo->pcidev = pdev;
minfo->dead = 0;
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 19/20] OMAPDSS: DSI: fix DSI channel source initialization
From: Tomi Valkeinen @ 2013-03-11 8:25 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <513D92A5.5050509@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2211 bytes --]
On 2013-03-11 10:15, Archit Taneja wrote:
> On Monday 11 March 2013 12:32 PM, Tomi Valkeinen wrote:
>> On 2013-03-11 08:10, Archit Taneja wrote:
>>> Hi,
>>>
>>> On Friday 08 March 2013 05:21 PM, Tomi Valkeinen wrote:
>>>> During the initialization of the DSI protocol registers, we always set
>>>> the sources of all DSI channels to L4. However, we don't update the
>>>> value in the dsi_data, so we may end up with a different value in the
>>>> register and in the dsi_data, leading to DSI problems.
>>>>
>>>> This patch fixes the issue by initializing also the channel source in
>>>> the dsi_data.
>>>
>>> We set in omap_dsihw_probe:
>>>
>>> static int __init omap_dsihw_probe(struct platform_device *dsidev)
>>> {
>>> ...
>>> ...
>>> /* DSI VCs initialization */
>>> for (i = 0; i < ARRAY_SIZE(dsi->vc); i++) {
>>> dsi->vc[i].source = DSI_VC_SOURCE_L4;
>>> dsi->vc[i].dssdev = NULL;
>>> dsi->vc[i].vc_id = 0;
>>> }
>>> ...
>>> ...
>>> }
>>
>> Hmm... I did have a bug related to this when prototyping CDF. Ah.
>> Consider this:
>>
>> Panel powers up and uses DSI normally. A DSI VC is set to video mode.
>> Then the panel power down. Then it powers up again, and enables DSI. At
>> this time, dsi_vc_initial_config() is called again, setting the source
>> in the registers to L4. But the source in dsi_data is still VP.
>
> Oh ok.
>
>>
>> So perhaps the whole piece of code from omap_dsihw_probe should be moved
>> to somewhere else (dsi_vc_initial_config() sounds like a good place), so
>> that they are initialized each time the registers are initialized.
>
> VC source is something which seems fine to do in
> dsi_vc_initial_config(). I'm not sure about the dssdev and vc_id fields
> though, we would need to call omap_dsi_request_vc() and
> omap_dsi_set_vc_id() again after a power off. Currently, we do it only
> once in taal_probe().
Oh, right. Well, neither dssdev nor vc_id is written to registers, so
they won't have similar issues in any case. So I guess this patch is
fine as it is, and we do not need to touch dssdev nor vc_id.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply
* Re: [PATCH 19/20] OMAPDSS: DSI: fix DSI channel source initialization
From: Archit Taneja @ 2013-03-11 8:27 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <513D8198.7070702@ti.com>
On Monday 11 March 2013 12:32 PM, Tomi Valkeinen wrote:
> On 2013-03-11 08:10, Archit Taneja wrote:
>> Hi,
>>
>> On Friday 08 March 2013 05:21 PM, Tomi Valkeinen wrote:
>>> During the initialization of the DSI protocol registers, we always set
>>> the sources of all DSI channels to L4. However, we don't update the
>>> value in the dsi_data, so we may end up with a different value in the
>>> register and in the dsi_data, leading to DSI problems.
>>>
>>> This patch fixes the issue by initializing also the channel source in
>>> the dsi_data.
>>
>> We set in omap_dsihw_probe:
>>
>> static int __init omap_dsihw_probe(struct platform_device *dsidev)
>> {
>> ...
>> ...
>> /* DSI VCs initialization */
>> for (i = 0; i < ARRAY_SIZE(dsi->vc); i++) {
>> dsi->vc[i].source = DSI_VC_SOURCE_L4;
>> dsi->vc[i].dssdev = NULL;
>> dsi->vc[i].vc_id = 0;
>> }
>> ...
>> ...
>> }
>
> Hmm... I did have a bug related to this when prototyping CDF. Ah.
> Consider this:
>
> Panel powers up and uses DSI normally. A DSI VC is set to video mode.
> Then the panel power down. Then it powers up again, and enables DSI. At
> this time, dsi_vc_initial_config() is called again, setting the source
> in the registers to L4. But the source in dsi_data is still VP.
Oh ok.
>
> So perhaps the whole piece of code from omap_dsihw_probe should be moved
> to somewhere else (dsi_vc_initial_config() sounds like a good place), so
> that they are initialized each time the registers are initialized.
VC source is something which seems fine to do in
dsi_vc_initial_config(). I'm not sure about the dssdev and vc_id fields
though, we would need to call omap_dsi_request_vc() and
omap_dsi_set_vc_id() again after a power off. Currently, we do it only
once in taal_probe().
Archit
^ permalink raw reply
* Re: [PATCH v2 2/2] video: imxfb: Add DT support
From: Mark Rutland @ 2013-03-11 10:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1362504608-15839-3-git-send-email-mpa@pengutronix.de>
Hi,
Any reason for not CCing devicetree-discuss?
I have a couple of comments on the binding and the way it's parsed.
On Tue, Mar 05, 2013 at 05:30:08PM +0000, Markus Pargmann wrote:
> Add devicetree support for imx framebuffer driver. It uses the generic
> display bindings and helper functions.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> ---
>
> Notes:
> Changes in v2:
> - Removed pwmr register property
> - Cleanup of devicetree binding documentation
> - Use default values for pwmr and lscr1
>
> .../devicetree/bindings/video/fsl,imx-fb.txt | 49 ++++++
> drivers/video/imxfb.c | 182 +++++++++++++++++----
> 2 files changed, 197 insertions(+), 34 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/video/fsl,imx-fb.txt
>
> diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> new file mode 100644
> index 0000000..e1a53a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> @@ -0,0 +1,49 @@
> +Freescale imx21 Framebuffer
> +
> +This framebuffer driver supports devices imx1, imx21, imx25, and imx27.
> +
> +Required properties:
> +- compatible : "fsl,<chip>-fb", chip should be imx1 or imx21
> +- reg : Should contain 1 register ranges(address and length)
> +- interrupts : One interrupt of the fb dev
> +
> +Required nodes:
> +- display: Phandle to a display node as described in
> + Documentation/devicetree/bindings/video/display-timing.txt
> + Additional, the display node has to define properties:
> + - bpp: Bits per pixel
> + - pcr: LCDC PCR value
As these are non-standard, it would be good to prefix them (e.g. "fsl,pcr").
If you need them, why are they not a good fit for the generic binding?
I'm not familiar with the hardware, what is the PCR exactly?
[...]
> -static int __init imxfb_probe(struct platform_device *pdev)
> +static int imxfb_of_read_mode(struct device_node *np,
> + struct imx_fb_videomode *imxfb_mode)
> +{
> + int ret;
> + struct fb_videomode *of_mode = &imxfb_mode->mode;
> + u32 bpp;
> + u32 pcr;
> +
> + ret = of_property_read_string(np, "model", &of_mode->name);
> + if (ret)
> + of_mode->name = NULL;
> +
> + ret = of_get_fb_videomode(np, of_mode, OF_USE_NATIVE_MODE);
> + if (ret)
> + return ret;
> +
> + ret = of_property_read_u32(np, "bpp", &bpp);
> + ret |= of_property_read_u32(np, "pcr", &pcr);
> +
> + if (ret)
> + return ret;
Is this return value used anywhere in anything more than an "if (!err)"
capacity? If so it may be worth having individual return value checks:
If one call returns -EINVAL (-22) and the other -ENODATA (-61), out the other
end we'd get -EISDIR (-21). If we don't care particularly about which error
code we actually pass on, we could always return a sensible code when ret is
nonzero:
if (ret)
return -EINVAL;
> +
> + if (bpp > 255)
> + return -EINVAL;
Might it also be worth checking for 0 here?
[...]
> @@ -837,15 +914,51 @@ static int __init imxfb_probe(struct platform_device *pdev)
>
> fbi = info->par;
>
> - if (!fb_mode)
> - fb_mode = pdata->mode[0].mode.name;
> -
> platform_set_drvdata(pdev, info);
>
> ret = imxfb_init_fbinfo(pdev);
> if (ret < 0)
> goto failed_init;
>
> + if (pdata) {
> + if (!fb_mode)
> + fb_mode = pdata->mode[0].mode.name;
> +
> + fbi->mode = pdata->mode;
> + fbi->num_modes = pdata->num_modes;
> + } else {
> + struct device_node *display_np;
> + fb_mode = NULL;
> +
> + display_np = of_parse_phandle(pdev->dev.of_node, "display", 0);
> + if (!display_np) {
> + dev_err(&pdev->dev, "No display defined in devicetree\n");
> + ret = -EINVAL;
> + goto failed_of_parse;
> + }
> +
> + /*
> + * imxfb does not support more modes, we choose only the native
> + * mode.
> + */
> + fbi->num_modes = 1;
> +
> + fbi->mode = devm_kzalloc(&pdev->dev,
> + sizeof(struct imx_fb_videomode), GFP_KERNEL);
> + if (!fbi->mode) {
> + ret = -ENOMEM;
> + goto failed_of_parse;
> + }
> +
> + ret = imxfb_of_read_mode(display_np, fbi->mode);
> + if (ret)
> + goto failed_of_parse;
> + }
> +
> + for (i = 0, m = &fbi->mode[0]; i < fbi->num_modes; i++, m++)
> + info->fix.smem_len = max_t(size_t, info->fix.smem_len,
> + m->mode.xres * m->mode.yres * m->bpp / 8);
Surely this is broken if bpp is not as multiple of 8?
If we can only handle multiples of 8, could we not sanity check this earlier?
If there's no strong preference for describing it in bits, could we not
describe it in bytes and side-step the issue?
[...]
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH 10/20] OMAPDSS: Resolve channels for outputs
From: Tomi Valkeinen @ 2013-03-11 11:02 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <513D6D06.7030902@ti.com>
[-- Attachment #1: Type: text/plain, Size: 4497 bytes --]
On 2013-03-11 07:35, Archit Taneja wrote:
> Hi,
>
> On Friday 08 March 2013 05:21 PM, Tomi Valkeinen wrote:
>> The DISPC channel used for each output is currently passed in panel
>> platform data from the board files.
>>
>> To simplify this, and to make the panel drivers less dependent on OMAP,
>> this patch changes omapdss to resolve the channel independently. The
>> channel is resolved based on the OMAP version and, in case of DSI, the
>> DSI module id.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>> drivers/video/omap2/dss/dpi.c | 37 ++++++++++++++++++++++++++-----
>> drivers/video/omap2/dss/dsi.c | 48
>> ++++++++++++++++++++++++++++++++++++++++
>> drivers/video/omap2/dss/rfbi.c | 2 ++
>> drivers/video/omap2/dss/sdi.c | 2 ++
>> 4 files changed, 84 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dpi.c
>> b/drivers/video/omap2/dss/dpi.c
>> index e282456..3261644 100644
>> --- a/drivers/video/omap2/dss/dpi.c
>> +++ b/drivers/video/omap2/dss/dpi.c
>> @@ -396,12 +396,44 @@ static int __init dpi_verify_dsi_pll(struct
>> platform_device *dsidev)
>> return 0;
>> }
>>
>> +/*
>> + * Return a hardcoded channel for the DPI output. This should work for
>> + * current use cases, but this can be later expanded to either resolve
>> + * the channel in some more dynamic manner, or get the channel as a user
>> + * parameter.
>> + */
>> +static enum omap_channel dpi_get_channel(void)
>> +{
>> + switch (omapdss_get_version()) {
>> + case OMAPDSS_VER_OMAP24xx:
>> + case OMAPDSS_VER_OMAP34xx_ES1:
>> + case OMAPDSS_VER_OMAP34xx_ES3:
>> + case OMAPDSS_VER_OMAP3630:
>> + case OMAPDSS_VER_AM35xx:
>> + return OMAP_DSS_CHANNEL_LCD;
>> +
>> + case OMAPDSS_VER_OMAP4430_ES1:
>> + case OMAPDSS_VER_OMAP4430_ES2:
>> + case OMAPDSS_VER_OMAP4:
>> + return OMAP_DSS_CHANNEL_LCD2;
>> +
>> + case OMAPDSS_VER_OMAP5:
>> + return OMAP_DSS_CHANNEL_LCD2;
>> +
>> + default:
>> + DSSWARN("unsupported DSS version\n");
>> + return OMAP_DSS_CHANNEL_LCD;
>> + }
>> +}
>> +
>> static int __init dpi_init_display(struct omap_dss_device *dssdev)
>> {
>> struct platform_device *dsidev;
>>
>> DSSDBG("init_display\n");
>>
>> + dssdev->channel = dpi_get_channel();
>
> In patch 14 of the series, we remove these dssdev->channel assignments.
> I don't see the point of adding them in this patch in the first place.
> The dssdev->channel assignments will not be modified in this series, so
> we don't need to worry about a kernel crash or something after this patch.
>
> I feel this patch should only add the dpi_get_channel and
> dsi_get_channel funcs.
Yes, you're right. It's just extra going back and forth. I think I'll
merge this with the patch adding recommended channel.
>> +
>> if (dss_has_feature(FEAT_DPI_USES_VDDS_DSI) &&
>> dpi.vdds_dsi_reg == NULL) {
>> struct regulator *vdds_dsi;
>> @@ -416,11 +448,6 @@ static int __init dpi_init_display(struct
>> omap_dss_device *dssdev)
>> dpi.vdds_dsi_reg = vdds_dsi;
>> }
>>
>> - /*
>> - * XXX We shouldn't need dssdev->channel for this. The dsi pll clock
>> - * source for DPI is SoC integration detail, not something that
>> should
>> - * be configured in the dssdev
>> - */
>> dsidev = dpi_get_dsidev(dssdev->channel);
>>
>> if (dsidev && dpi_verify_dsi_pll(dsidev)) {
>> diff --git a/drivers/video/omap2/dss/dsi.c
>> b/drivers/video/omap2/dss/dsi.c
>> index 1a6ad6f..c39ca86 100644
>> --- a/drivers/video/omap2/dss/dsi.c
>> +++ b/drivers/video/omap2/dss/dsi.c
>> @@ -4946,6 +4946,52 @@ void omapdss_dsi_set_videomode_timings(struct
>> omap_dss_device *dssdev,
>> }
>> EXPORT_SYMBOL(omapdss_dsi_set_videomode_timings);
>>
>> +/*
>> + * Return a hardcoded channel for the DSI output. This should work for
>> + * current use cases, but this can be later expanded to either resolve
>> + * the channel in some more dynamic manner, or get the channel as a user
>> + * parameter.
>> + */
>> +static enum omap_channel dsi_get_channel(int module_id)
>> +{
>> + switch (omapdss_get_version()) {
>> + case OMAPDSS_VER_OMAP24xx:
>
> We should remove the above case so that we hit the default case and get
> a warning about omap2 not having DSI.
Yep, I'll fix that.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply
* Re: [PATCH 10/20] OMAPDSS: Resolve channels for outputs
From: Tomi Valkeinen @ 2013-03-11 11:54 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <513D716C.2050807@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2978 bytes --]
On 2013-03-11 07:53, Archit Taneja wrote:
> On Friday 08 March 2013 05:21 PM, Tomi Valkeinen wrote:
>> The DISPC channel used for each output is currently passed in panel
>> platform data from the board files.
>>
>> To simplify this, and to make the panel drivers less dependent on OMAP,
>> this patch changes omapdss to resolve the channel independently. The
>> channel is resolved based on the OMAP version and, in case of DSI, the
>> DSI module id.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>> drivers/video/omap2/dss/dpi.c | 37 ++++++++++++++++++++++++++-----
>> drivers/video/omap2/dss/dsi.c | 48
>> ++++++++++++++++++++++++++++++++++++++++
>> drivers/video/omap2/dss/rfbi.c | 2 ++
>> drivers/video/omap2/dss/sdi.c | 2 ++
>> 4 files changed, 84 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dpi.c
>> b/drivers/video/omap2/dss/dpi.c
>> index e282456..3261644 100644
>> --- a/drivers/video/omap2/dss/dpi.c
>> +++ b/drivers/video/omap2/dss/dpi.c
>> @@ -396,12 +396,44 @@ static int __init dpi_verify_dsi_pll(struct
>> platform_device *dsidev)
>> return 0;
>> }
>>
>> +/*
>> + * Return a hardcoded channel for the DPI output. This should work for
>> + * current use cases, but this can be later expanded to either resolve
>> + * the channel in some more dynamic manner, or get the channel as a user
>> + * parameter.
>> + */
>> +static enum omap_channel dpi_get_channel(void)
>> +{
>> + switch (omapdss_get_version()) {
>> + case OMAPDSS_VER_OMAP24xx:
>> + case OMAPDSS_VER_OMAP34xx_ES1:
>> + case OMAPDSS_VER_OMAP34xx_ES3:
>> + case OMAPDSS_VER_OMAP3630:
>> + case OMAPDSS_VER_AM35xx:
>> + return OMAP_DSS_CHANNEL_LCD;
>> +
>> + case OMAPDSS_VER_OMAP4430_ES1:
>> + case OMAPDSS_VER_OMAP4430_ES2:
>> + case OMAPDSS_VER_OMAP4:
>> + return OMAP_DSS_CHANNEL_LCD2;
>> +
>> + case OMAPDSS_VER_OMAP5:
>> + return OMAP_DSS_CHANNEL_LCD2;
>> +
>> + default:
>> + DSSWARN("unsupported DSS version\n");
>> + return OMAP_DSS_CHANNEL_LCD;
>> + }
>> +}
>
> I had another comment for this patch. On OMAP5, it makes sense for us to
> not use LCD2 as the recommended channel. LCD2_CLK's only source is
> DSS_CLK from PRCM. So it's not a very flexible channel to use. We could
> use LCD3 (at the cost of not using DSI2).
Ok. Yes, this looks to be the case. I'll use LCD3 for DPI for now. In
the worst case we may need to pass some channel setup parameters via
omapdss DT data or platform data, but I'd really much like the driver to
be able to resolve the dispc channels by itself...
> We also need to fix dpi_get_dsidev() for OMAP5. Currently, it assumes
> that LCD2_CLK can be sourced from DSI2 PLL, we need to ensure DPI has a
> dsidev only if it's LCD1 or LCD3.
Right. I'll add if (OMAP5) there, and return DSI2 PLL for LCD3, and NULL
for LCD2.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply
* Re: [PATCH 10/20] OMAPDSS: Resolve channels for outputs
From: Tomi Valkeinen @ 2013-03-11 12:01 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <513D716C.2050807@ti.com>
[-- Attachment #1: Type: text/plain, Size: 4243 bytes --]
On 2013-03-11 07:53, Archit Taneja wrote:
> On Friday 08 March 2013 05:21 PM, Tomi Valkeinen wrote:
>> The DISPC channel used for each output is currently passed in panel
>> platform data from the board files.
>>
>> To simplify this, and to make the panel drivers less dependent on OMAP,
>> this patch changes omapdss to resolve the channel independently. The
>> channel is resolved based on the OMAP version and, in case of DSI, the
>> DSI module id.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>> drivers/video/omap2/dss/dpi.c | 37 ++++++++++++++++++++++++++-----
>> drivers/video/omap2/dss/dsi.c | 48
>> ++++++++++++++++++++++++++++++++++++++++
>> drivers/video/omap2/dss/rfbi.c | 2 ++
>> drivers/video/omap2/dss/sdi.c | 2 ++
>> 4 files changed, 84 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dpi.c
>> b/drivers/video/omap2/dss/dpi.c
>> index e282456..3261644 100644
>> --- a/drivers/video/omap2/dss/dpi.c
>> +++ b/drivers/video/omap2/dss/dpi.c
>> @@ -396,12 +396,44 @@ static int __init dpi_verify_dsi_pll(struct
>> platform_device *dsidev)
>> return 0;
>> }
>>
>> +/*
>> + * Return a hardcoded channel for the DPI output. This should work for
>> + * current use cases, but this can be later expanded to either resolve
>> + * the channel in some more dynamic manner, or get the channel as a user
>> + * parameter.
>> + */
>> +static enum omap_channel dpi_get_channel(void)
>> +{
>> + switch (omapdss_get_version()) {
>> + case OMAPDSS_VER_OMAP24xx:
>> + case OMAPDSS_VER_OMAP34xx_ES1:
>> + case OMAPDSS_VER_OMAP34xx_ES3:
>> + case OMAPDSS_VER_OMAP3630:
>> + case OMAPDSS_VER_AM35xx:
>> + return OMAP_DSS_CHANNEL_LCD;
>> +
>> + case OMAPDSS_VER_OMAP4430_ES1:
>> + case OMAPDSS_VER_OMAP4430_ES2:
>> + case OMAPDSS_VER_OMAP4:
>> + return OMAP_DSS_CHANNEL_LCD2;
>> +
>> + case OMAPDSS_VER_OMAP5:
>> + return OMAP_DSS_CHANNEL_LCD2;
>> +
>> + default:
>> + DSSWARN("unsupported DSS version\n");
>> + return OMAP_DSS_CHANNEL_LCD;
>> + }
>> +}
>
> I had another comment for this patch. On OMAP5, it makes sense for us to
> not use LCD2 as the recommended channel. LCD2_CLK's only source is
> DSS_CLK from PRCM. So it's not a very flexible channel to use. We could
> use LCD3 (at the cost of not using DSI2).
>
> We also need to fix dpi_get_dsidev() for OMAP5. Currently, it assumes
> that LCD2_CLK can be sourced from DSI2 PLL, we need to ensure DPI has a
> dsidev only if it's LCD1 or LCD3.
I fixed it this way:
Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Mon Mar 11 13:57:38 2013 +0200
OMAPDSS: DPI: fix dpi_get_dsidev() for omap5
On OMAP5 the DISPC channels and DSI PLLs are not connected the same way.
LCD2 on OMAP5 cannot use any DSI PLL as a source clock, but LCD3 can use
DSI2's PLL.
This patch fixes dpi_get_dsidev() by adding separate case for OMAP5 to
handle the difference.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 409e53b..433eff7 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -63,15 +63,29 @@ static struct platform_device *dpi_get_dsidev(enum omap_channel channel)
case OMAPDSS_VER_OMAP3630:
case OMAPDSS_VER_AM35xx:
return NULL;
- default:
- break;
- }
- switch (channel) {
- case OMAP_DSS_CHANNEL_LCD:
- return dsi_get_dsidev_from_id(0);
- case OMAP_DSS_CHANNEL_LCD2:
- return dsi_get_dsidev_from_id(1);
+ case OMAPDSS_VER_OMAP4430_ES1:
+ case OMAPDSS_VER_OMAP4430_ES2:
+ case OMAPDSS_VER_OMAP4:
+ switch (channel) {
+ case OMAP_DSS_CHANNEL_LCD:
+ return dsi_get_dsidev_from_id(0);
+ case OMAP_DSS_CHANNEL_LCD2:
+ return dsi_get_dsidev_from_id(1);
+ default:
+ return NULL;
+ }
+
+ case OMAPDSS_VER_OMAP5:
+ switch (channel) {
+ case OMAP_DSS_CHANNEL_LCD:
+ return dsi_get_dsidev_from_id(0);
+ case OMAP_DSS_CHANNEL_LCD3:
+ return dsi_get_dsidev_from_id(1);
+ default:
+ return NULL;
+ }
+
default:
return NULL;
}
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply related
* Re: [PATCH 10/20] OMAPDSS: Resolve channels for outputs
From: Tomi Valkeinen @ 2013-03-11 12:05 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <513D6D06.7030902@ti.com>
[-- Attachment #1: Type: text/plain, Size: 11242 bytes --]
On 2013-03-11 07:35, Archit Taneja wrote:
> Hi,
>
> On Friday 08 March 2013 05:21 PM, Tomi Valkeinen wrote:
>> The DISPC channel used for each output is currently passed in panel
>> platform data from the board files.
>>
>> To simplify this, and to make the panel drivers less dependent on OMAP,
>> this patch changes omapdss to resolve the channel independently. The
>> channel is resolved based on the OMAP version and, in case of DSI, the
>> DSI module id.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>> drivers/video/omap2/dss/dpi.c | 37 ++++++++++++++++++++++++++-----
>> drivers/video/omap2/dss/dsi.c | 48
>> ++++++++++++++++++++++++++++++++++++++++
>> drivers/video/omap2/dss/rfbi.c | 2 ++
>> drivers/video/omap2/dss/sdi.c | 2 ++
>> 4 files changed, 84 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dpi.c
>> b/drivers/video/omap2/dss/dpi.c
>> index e282456..3261644 100644
>> --- a/drivers/video/omap2/dss/dpi.c
>> +++ b/drivers/video/omap2/dss/dpi.c
>> @@ -396,12 +396,44 @@ static int __init dpi_verify_dsi_pll(struct
>> platform_device *dsidev)
>> return 0;
>> }
>>
>> +/*
>> + * Return a hardcoded channel for the DPI output. This should work for
>> + * current use cases, but this can be later expanded to either resolve
>> + * the channel in some more dynamic manner, or get the channel as a user
>> + * parameter.
>> + */
>> +static enum omap_channel dpi_get_channel(void)
>> +{
>> + switch (omapdss_get_version()) {
>> + case OMAPDSS_VER_OMAP24xx:
>> + case OMAPDSS_VER_OMAP34xx_ES1:
>> + case OMAPDSS_VER_OMAP34xx_ES3:
>> + case OMAPDSS_VER_OMAP3630:
>> + case OMAPDSS_VER_AM35xx:
>> + return OMAP_DSS_CHANNEL_LCD;
>> +
>> + case OMAPDSS_VER_OMAP4430_ES1:
>> + case OMAPDSS_VER_OMAP4430_ES2:
>> + case OMAPDSS_VER_OMAP4:
>> + return OMAP_DSS_CHANNEL_LCD2;
>> +
>> + case OMAPDSS_VER_OMAP5:
>> + return OMAP_DSS_CHANNEL_LCD2;
>> +
>> + default:
>> + DSSWARN("unsupported DSS version\n");
>> + return OMAP_DSS_CHANNEL_LCD;
>> + }
>> +}
>> +
>> static int __init dpi_init_display(struct omap_dss_device *dssdev)
>> {
>> struct platform_device *dsidev;
>>
>> DSSDBG("init_display\n");
>>
>> + dssdev->channel = dpi_get_channel();
>
> In patch 14 of the series, we remove these dssdev->channel assignments.
> I don't see the point of adding them in this patch in the first place.
> The dssdev->channel assignments will not be modified in this series, so
> we don't need to worry about a kernel crash or something after this patch.
>
> I feel this patch should only add the dpi_get_channel and
> dsi_get_channel funcs.
Here's the combined patch. I changed the recommended_channel to dispc_channel.
"recommended" was not that good name, as currently the channel cannot be changed
later, because, for example in DPI case, we store the DSI PLL which is based on
the channel.
Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Wed Feb 13 11:23:54 2013 +0200
OMAPDSS: add output->dispc_channel
The DISPC channel used for each output is currently passed in panel
platform data from the board files.
To simplify this, and to make the panel drivers less dependent on OMAP,
this patch changes omapdss to resolve the channel independently. The
channel is resolved based on the OMAP version and, in case of DSI, the
DSI module id. This resolved channel is stored into a new field in
output, dispc_channel.
The few places where dssdev->channel was used are changed to use
output->recommended_channel. After this patch, dssdev->channel is
obsolete.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index e282456..409e53b 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -396,6 +396,36 @@ static int __init dpi_verify_dsi_pll(struct platform_device *dsidev)
return 0;
}
+/*
+ * Return a hardcoded channel for the DPI output. This should work for
+ * current use cases, but this can be later expanded to either resolve
+ * the channel in some more dynamic manner, or get the channel as a user
+ * parameter.
+ */
+static enum omap_channel dpi_get_channel(void)
+{
+ switch (omapdss_get_version()) {
+ case OMAPDSS_VER_OMAP24xx:
+ case OMAPDSS_VER_OMAP34xx_ES1:
+ case OMAPDSS_VER_OMAP34xx_ES3:
+ case OMAPDSS_VER_OMAP3630:
+ case OMAPDSS_VER_AM35xx:
+ return OMAP_DSS_CHANNEL_LCD;
+
+ case OMAPDSS_VER_OMAP4430_ES1:
+ case OMAPDSS_VER_OMAP4430_ES2:
+ case OMAPDSS_VER_OMAP4:
+ return OMAP_DSS_CHANNEL_LCD2;
+
+ case OMAPDSS_VER_OMAP5:
+ return OMAP_DSS_CHANNEL_LCD3;
+
+ default:
+ DSSWARN("unsupported DSS version\n");
+ return OMAP_DSS_CHANNEL_LCD;
+ }
+}
+
static int __init dpi_init_display(struct omap_dss_device *dssdev)
{
struct platform_device *dsidev;
@@ -416,12 +446,7 @@ static int __init dpi_init_display(struct omap_dss_device *dssdev)
dpi.vdds_dsi_reg = vdds_dsi;
}
- /*
- * XXX We shouldn't need dssdev->channel for this. The dsi pll clock
- * source for DPI is SoC integration detail, not something that should
- * be configured in the dssdev
- */
- dsidev = dpi_get_dsidev(dssdev->channel);
+ dsidev = dpi_get_dsidev(dpi.output.dispc_channel);
if (dsidev && dpi_verify_dsi_pll(dsidev)) {
dsidev = NULL;
@@ -513,6 +538,7 @@ static void __init dpi_init_output(struct platform_device *pdev)
out->id = OMAP_DSS_OUTPUT_DPI;
out->type = OMAP_DISPLAY_TYPE_DPI;
out->name = "dpi";
+ out->dispc_channel = dpi_get_channel();
dss_register_output(out);
}
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 1a6ad6f..28e0b99 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -4946,6 +4946,55 @@ void omapdss_dsi_set_videomode_timings(struct omap_dss_device *dssdev,
}
EXPORT_SYMBOL(omapdss_dsi_set_videomode_timings);
+/*
+ * Return a hardcoded channel for the DSI output. This should work for
+ * current use cases, but this can be later expanded to either resolve
+ * the channel in some more dynamic manner, or get the channel as a user
+ * parameter.
+ */
+static enum omap_channel dsi_get_channel(int module_id)
+{
+ switch (omapdss_get_version()) {
+ case OMAPDSS_VER_OMAP24xx:
+ DSSWARN("DSI not supported\n");
+ return OMAP_DSS_CHANNEL_LCD;
+
+ case OMAPDSS_VER_OMAP34xx_ES1:
+ case OMAPDSS_VER_OMAP34xx_ES3:
+ case OMAPDSS_VER_OMAP3630:
+ case OMAPDSS_VER_AM35xx:
+ return OMAP_DSS_CHANNEL_LCD;
+
+ case OMAPDSS_VER_OMAP4430_ES1:
+ case OMAPDSS_VER_OMAP4430_ES2:
+ case OMAPDSS_VER_OMAP4:
+ switch (module_id) {
+ case 0:
+ return OMAP_DSS_CHANNEL_LCD;
+ case 1:
+ return OMAP_DSS_CHANNEL_LCD2;
+ default:
+ DSSWARN("unsupported module id\n");
+ return OMAP_DSS_CHANNEL_LCD;
+ }
+
+ case OMAPDSS_VER_OMAP5:
+ switch (module_id) {
+ case 0:
+ return OMAP_DSS_CHANNEL_LCD;
+ case 1:
+ return OMAP_DSS_CHANNEL_LCD3;
+ default:
+ DSSWARN("unsupported module id\n");
+ return OMAP_DSS_CHANNEL_LCD;
+ }
+
+ default:
+ DSSWARN("unsupported DSS version\n");
+ return OMAP_DSS_CHANNEL_LCD;
+ }
+}
+
static int __init dsi_init_display(struct omap_dss_device *dssdev)
{
struct platform_device *dsidev =
@@ -5184,6 +5233,7 @@ static void __init dsi_init_output(struct platform_device *dsidev)
out->type = OMAP_DISPLAY_TYPE_DSI;
out->name = dsi->module_id == 0 ? "dsi0" : "dsi1";
+ out->dispc_channel = dsi_get_channel(dsi->module_id);
dss_register_output(out);
}
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 888cfe3..e03619a 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -1012,8 +1012,6 @@ static void __init hdmi_probe_pdata(struct platform_device *pdev)
hdmi.ls_oe_gpio = priv->ls_oe_gpio;
hdmi.hpd_gpio = priv->hpd_gpio;
- dssdev->channel = OMAP_DSS_CHANNEL_DIGIT;
-
r = hdmi_init_display(dssdev);
if (r) {
DSSERR("device %s init failed: %d\n", dssdev->name, r);
@@ -1047,6 +1045,7 @@ static void __init hdmi_init_output(struct platform_device *pdev)
out->id = OMAP_DSS_OUTPUT_HDMI;
out->type = OMAP_DISPLAY_TYPE_HDMI;
out->name = "hdmi";
+ out->dispc_channel = OMAP_DSS_CHANNEL_DIGIT;
dss_register_output(out);
}
diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index a47a9e5..05c0646 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c
@@ -1026,6 +1026,7 @@ static void __init rfbi_init_output(struct platform_device *pdev)
out->id = OMAP_DSS_OUTPUT_DBI;
out->type = OMAP_DISPLAY_TYPE_DBI;
out->name = "rfbi";
+ out->dispc_channel = OMAP_DSS_CHANNEL_LCD;
dss_register_output(out);
}
diff --git a/drivers/video/omap2/dss/sdi.c b/drivers/video/omap2/dss/sdi.c
index 0802927..5d37db5 100644
--- a/drivers/video/omap2/dss/sdi.c
+++ b/drivers/video/omap2/dss/sdi.c
@@ -279,6 +279,7 @@ static void __init sdi_init_output(struct platform_device *pdev)
out->id = OMAP_DSS_OUTPUT_SDI;
out->type = OMAP_DISPLAY_TYPE_SDI;
out->name = "sdi";
+ out->dispc_channel = OMAP_DSS_CHANNEL_LCD;
dss_register_output(out);
}
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index c8130f8..866e015 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -786,8 +786,6 @@ static void __init venc_probe_pdata(struct platform_device *vencdev)
dss_copy_device_pdata(dssdev, plat_dssdev);
- dssdev->channel = OMAP_DSS_CHANNEL_DIGIT;
-
r = venc_init_display(dssdev);
if (r) {
DSSERR("device %s init failed: %d\n", dssdev->name, r);
@@ -820,6 +818,7 @@ static void __init venc_init_output(struct platform_device *pdev)
out->id = OMAP_DSS_OUTPUT_VENC;
out->type = OMAP_DISPLAY_TYPE_VENC;
out->name = "venc";
+ out->dispc_channel = OMAP_DSS_CHANNEL_DIGIT;
dss_register_output(out);
}
diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index ca585ef..f38348e 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -2388,7 +2388,7 @@ static int omapfb_init_connections(struct omapfb2_device *fbdev,
struct omap_dss_device *dssdev = fbdev->displays[i].dssdev;
struct omap_dss_output *out = dssdev->output;
- mgr = omap_dss_get_overlay_manager(dssdev->channel);
+ mgr = omap_dss_get_overlay_manager(out->dispc_channel);
if (!mgr || !out)
continue;
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index ba9cea7..ec322a7 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -547,6 +547,9 @@ struct omap_dss_output {
/* display type supported by the output */
enum omap_display_type type;
+ /* DISPC channel for this output */
+ enum omap_channel dispc_channel;
+
/* output instance */
enum omap_dss_output_id id;
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply related
* Re: [PATCH 2/2] omapfb: fix broken build on Amstrad E3/Delta
From: Tomi Valkeinen @ 2013-03-11 12:22 UTC (permalink / raw)
To: Tony Lindgren; +Cc: Aaro Koskinen, linux-fbdev, linux-omap
In-Reply-To: <20130304223911.GZ11806@atomide.com>
[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]
On 2013-03-05 00:39, Tony Lindgren wrote:
> Hi,
>
> * Aaro Koskinen <aaro.koskinen@iki.fi> [130304 13:08]:
>> Fix the following build regression in 3.9-rc1 by including
>> <machine/hardware.h>:
>>
>> drivers/video/omap/lcd_ams_delta.c: In function 'ams_delta_lcd_set_power':
>> drivers/video/omap/lcd_ams_delta.c:48:4: error: implicit declaration of function 'omap_writeb' [-Werror=implicit-function-declaration]
>> drivers/video/omap/lcd_ams_delta.c:49:6: error: 'OMAP_PWL_ENABLE' undeclared (first use in this function)
>> drivers/video/omap/lcd_ams_delta.c:49:6: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/video/omap/lcd_ams_delta.c:50:19: error: 'OMAP_PWL_CLK_ENABLE' undeclared (first use in this function)
>> drivers/video/omap/lcd_ams_delta.c: In function 'ams_delta_lcd_set_contrast':
>> drivers/video/omap/lcd_ams_delta.c:66:22: error: 'OMAP_PWL_ENABLE' undeclared (first use in this function)
>
> I have this already queued up in omap-for-v3.9-rc1/fixes as 0adcbaf78
> (ARM: OMAP1: Fix build related to kgdb.h no longer including serial_8250.h)
> and for lcd_osk.c as omap1_defconfig was broken.
>
> I don't anything for the first patch you posted, so I suggest
> Tomi applies that one.
Ok, I'll take the first patch.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply
* Re: [PATCH 10/20] OMAPDSS: Resolve channels for outputs
From: Archit Taneja @ 2013-03-11 12:31 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <513DC870.3030908@ti.com>
On Monday 11 March 2013 05:35 PM, Tomi Valkeinen wrote:
> On 2013-03-11 07:35, Archit Taneja wrote:
>> Hi,
>>
>> On Friday 08 March 2013 05:21 PM, Tomi Valkeinen wrote:
>>> The DISPC channel used for each output is currently passed in panel
>>> platform data from the board files.
>>>
>>> To simplify this, and to make the panel drivers less dependent on OMAP,
>>> this patch changes omapdss to resolve the channel independently. The
>>> channel is resolved based on the OMAP version and, in case of DSI, the
>>> DSI module id.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> ---
>>> drivers/video/omap2/dss/dpi.c | 37 ++++++++++++++++++++++++++-----
>>> drivers/video/omap2/dss/dsi.c | 48
>>> ++++++++++++++++++++++++++++++++++++++++
>>> drivers/video/omap2/dss/rfbi.c | 2 ++
>>> drivers/video/omap2/dss/sdi.c | 2 ++
>>> 4 files changed, 84 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/video/omap2/dss/dpi.c
>>> b/drivers/video/omap2/dss/dpi.c
>>> index e282456..3261644 100644
>>> --- a/drivers/video/omap2/dss/dpi.c
>>> +++ b/drivers/video/omap2/dss/dpi.c
>>> @@ -396,12 +396,44 @@ static int __init dpi_verify_dsi_pll(struct
>>> platform_device *dsidev)
>>> return 0;
>>> }
>>>
>>> +/*
>>> + * Return a hardcoded channel for the DPI output. This should work for
>>> + * current use cases, but this can be later expanded to either resolve
>>> + * the channel in some more dynamic manner, or get the channel as a user
>>> + * parameter.
>>> + */
>>> +static enum omap_channel dpi_get_channel(void)
>>> +{
>>> + switch (omapdss_get_version()) {
>>> + case OMAPDSS_VER_OMAP24xx:
>>> + case OMAPDSS_VER_OMAP34xx_ES1:
>>> + case OMAPDSS_VER_OMAP34xx_ES3:
>>> + case OMAPDSS_VER_OMAP3630:
>>> + case OMAPDSS_VER_AM35xx:
>>> + return OMAP_DSS_CHANNEL_LCD;
>>> +
>>> + case OMAPDSS_VER_OMAP4430_ES1:
>>> + case OMAPDSS_VER_OMAP4430_ES2:
>>> + case OMAPDSS_VER_OMAP4:
>>> + return OMAP_DSS_CHANNEL_LCD2;
>>> +
>>> + case OMAPDSS_VER_OMAP5:
>>> + return OMAP_DSS_CHANNEL_LCD2;
>>> +
>>> + default:
>>> + DSSWARN("unsupported DSS version\n");
>>> + return OMAP_DSS_CHANNEL_LCD;
>>> + }
>>> +}
>>> +
>>> static int __init dpi_init_display(struct omap_dss_device *dssdev)
>>> {
>>> struct platform_device *dsidev;
>>>
>>> DSSDBG("init_display\n");
>>>
>>> + dssdev->channel = dpi_get_channel();
>>
>> In patch 14 of the series, we remove these dssdev->channel assignments.
>> I don't see the point of adding them in this patch in the first place.
>> The dssdev->channel assignments will not be modified in this series, so
>> we don't need to worry about a kernel crash or something after this patch.
>>
>> I feel this patch should only add the dpi_get_channel and
>> dsi_get_channel funcs.
>
> Here's the combined patch. I changed the recommended_channel to dispc_channel.
> "recommended" was not that good name, as currently the channel cannot be changed
> later, because, for example in DPI case, we store the DSI PLL which is based on
> the channel.
The patch looks fine now.
Reviewed-by: Archit Taneja <archit@ti.com>
Archit
>
>
> Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Wed Feb 13 11:23:54 2013 +0200
>
> OMAPDSS: add output->dispc_channel
>
> The DISPC channel used for each output is currently passed in panel
> platform data from the board files.
>
> To simplify this, and to make the panel drivers less dependent on OMAP,
> this patch changes omapdss to resolve the channel independently. The
> channel is resolved based on the OMAP version and, in case of DSI, the
> DSI module id. This resolved channel is stored into a new field in
> output, dispc_channel.
>
> The few places where dssdev->channel was used are changed to use
> output->recommended_channel. After this patch, dssdev->channel is
> obsolete.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>
> diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
> index e282456..409e53b 100644
> --- a/drivers/video/omap2/dss/dpi.c
> +++ b/drivers/video/omap2/dss/dpi.c
> @@ -396,6 +396,36 @@ static int __init dpi_verify_dsi_pll(struct platform_device *dsidev)
> return 0;
> }
>
> +/*
> + * Return a hardcoded channel for the DPI output. This should work for
> + * current use cases, but this can be later expanded to either resolve
> + * the channel in some more dynamic manner, or get the channel as a user
> + * parameter.
> + */
> +static enum omap_channel dpi_get_channel(void)
> +{
> + switch (omapdss_get_version()) {
> + case OMAPDSS_VER_OMAP24xx:
> + case OMAPDSS_VER_OMAP34xx_ES1:
> + case OMAPDSS_VER_OMAP34xx_ES3:
> + case OMAPDSS_VER_OMAP3630:
> + case OMAPDSS_VER_AM35xx:
> + return OMAP_DSS_CHANNEL_LCD;
> +
> + case OMAPDSS_VER_OMAP4430_ES1:
> + case OMAPDSS_VER_OMAP4430_ES2:
> + case OMAPDSS_VER_OMAP4:
> + return OMAP_DSS_CHANNEL_LCD2;
> +
> + case OMAPDSS_VER_OMAP5:
> + return OMAP_DSS_CHANNEL_LCD3;
> +
> + default:
> + DSSWARN("unsupported DSS version\n");
> + return OMAP_DSS_CHANNEL_LCD;
> + }
> +}
> +
> static int __init dpi_init_display(struct omap_dss_device *dssdev)
> {
> struct platform_device *dsidev;
> @@ -416,12 +446,7 @@ static int __init dpi_init_display(struct omap_dss_device *dssdev)
> dpi.vdds_dsi_reg = vdds_dsi;
> }
>
> - /*
> - * XXX We shouldn't need dssdev->channel for this. The dsi pll clock
> - * source for DPI is SoC integration detail, not something that should
> - * be configured in the dssdev
> - */
> - dsidev = dpi_get_dsidev(dssdev->channel);
> + dsidev = dpi_get_dsidev(dpi.output.dispc_channel);
>
> if (dsidev && dpi_verify_dsi_pll(dsidev)) {
> dsidev = NULL;
> @@ -513,6 +538,7 @@ static void __init dpi_init_output(struct platform_device *pdev)
> out->id = OMAP_DSS_OUTPUT_DPI;
> out->type = OMAP_DISPLAY_TYPE_DPI;
> out->name = "dpi";
> + out->dispc_channel = dpi_get_channel();
>
> dss_register_output(out);
> }
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index 1a6ad6f..28e0b99 100644
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -4946,6 +4946,55 @@ void omapdss_dsi_set_videomode_timings(struct omap_dss_device *dssdev,
> }
> EXPORT_SYMBOL(omapdss_dsi_set_videomode_timings);
>
> +/*
> + * Return a hardcoded channel for the DSI output. This should work for
> + * current use cases, but this can be later expanded to either resolve
> + * the channel in some more dynamic manner, or get the channel as a user
> + * parameter.
> + */
> +static enum omap_channel dsi_get_channel(int module_id)
> +{
> + switch (omapdss_get_version()) {
> + case OMAPDSS_VER_OMAP24xx:
> + DSSWARN("DSI not supported\n");
> + return OMAP_DSS_CHANNEL_LCD;
> +
> + case OMAPDSS_VER_OMAP34xx_ES1:
> + case OMAPDSS_VER_OMAP34xx_ES3:
> + case OMAPDSS_VER_OMAP3630:
> + case OMAPDSS_VER_AM35xx:
> + return OMAP_DSS_CHANNEL_LCD;
> +
> + case OMAPDSS_VER_OMAP4430_ES1:
> + case OMAPDSS_VER_OMAP4430_ES2:
> + case OMAPDSS_VER_OMAP4:
> + switch (module_id) {
> + case 0:
> + return OMAP_DSS_CHANNEL_LCD;
> + case 1:
> + return OMAP_DSS_CHANNEL_LCD2;
> + default:
> + DSSWARN("unsupported module id\n");
> + return OMAP_DSS_CHANNEL_LCD;
> + }
> +
> + case OMAPDSS_VER_OMAP5:
> + switch (module_id) {
> + case 0:
> + return OMAP_DSS_CHANNEL_LCD;
> + case 1:
> + return OMAP_DSS_CHANNEL_LCD3;
> + default:
> + DSSWARN("unsupported module id\n");
> + return OMAP_DSS_CHANNEL_LCD;
> + }
> +
> + default:
> + DSSWARN("unsupported DSS version\n");
> + return OMAP_DSS_CHANNEL_LCD;
> + }
> +}
> +
> static int __init dsi_init_display(struct omap_dss_device *dssdev)
> {
> struct platform_device *dsidev > @@ -5184,6 +5233,7 @@ static void __init dsi_init_output(struct platform_device *dsidev)
>
> out->type = OMAP_DISPLAY_TYPE_DSI;
> out->name = dsi->module_id = 0 ? "dsi0" : "dsi1";
> + out->dispc_channel = dsi_get_channel(dsi->module_id);
>
> dss_register_output(out);
> }
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index 888cfe3..e03619a 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -1012,8 +1012,6 @@ static void __init hdmi_probe_pdata(struct platform_device *pdev)
> hdmi.ls_oe_gpio = priv->ls_oe_gpio;
> hdmi.hpd_gpio = priv->hpd_gpio;
>
> - dssdev->channel = OMAP_DSS_CHANNEL_DIGIT;
> -
> r = hdmi_init_display(dssdev);
> if (r) {
> DSSERR("device %s init failed: %d\n", dssdev->name, r);
> @@ -1047,6 +1045,7 @@ static void __init hdmi_init_output(struct platform_device *pdev)
> out->id = OMAP_DSS_OUTPUT_HDMI;
> out->type = OMAP_DISPLAY_TYPE_HDMI;
> out->name = "hdmi";
> + out->dispc_channel = OMAP_DSS_CHANNEL_DIGIT;
>
> dss_register_output(out);
> }
> diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
> index a47a9e5..05c0646 100644
> --- a/drivers/video/omap2/dss/rfbi.c
> +++ b/drivers/video/omap2/dss/rfbi.c
> @@ -1026,6 +1026,7 @@ static void __init rfbi_init_output(struct platform_device *pdev)
> out->id = OMAP_DSS_OUTPUT_DBI;
> out->type = OMAP_DISPLAY_TYPE_DBI;
> out->name = "rfbi";
> + out->dispc_channel = OMAP_DSS_CHANNEL_LCD;
>
> dss_register_output(out);
> }
> diff --git a/drivers/video/omap2/dss/sdi.c b/drivers/video/omap2/dss/sdi.c
> index 0802927..5d37db5 100644
> --- a/drivers/video/omap2/dss/sdi.c
> +++ b/drivers/video/omap2/dss/sdi.c
> @@ -279,6 +279,7 @@ static void __init sdi_init_output(struct platform_device *pdev)
> out->id = OMAP_DSS_OUTPUT_SDI;
> out->type = OMAP_DISPLAY_TYPE_SDI;
> out->name = "sdi";
> + out->dispc_channel = OMAP_DSS_CHANNEL_LCD;
>
> dss_register_output(out);
> }
> diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
> index c8130f8..866e015 100644
> --- a/drivers/video/omap2/dss/venc.c
> +++ b/drivers/video/omap2/dss/venc.c
> @@ -786,8 +786,6 @@ static void __init venc_probe_pdata(struct platform_device *vencdev)
>
> dss_copy_device_pdata(dssdev, plat_dssdev);
>
> - dssdev->channel = OMAP_DSS_CHANNEL_DIGIT;
> -
> r = venc_init_display(dssdev);
> if (r) {
> DSSERR("device %s init failed: %d\n", dssdev->name, r);
> @@ -820,6 +818,7 @@ static void __init venc_init_output(struct platform_device *pdev)
> out->id = OMAP_DSS_OUTPUT_VENC;
> out->type = OMAP_DISPLAY_TYPE_VENC;
> out->name = "venc";
> + out->dispc_channel = OMAP_DSS_CHANNEL_DIGIT;
>
> dss_register_output(out);
> }
> diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
> index ca585ef..f38348e 100644
> --- a/drivers/video/omap2/omapfb/omapfb-main.c
> +++ b/drivers/video/omap2/omapfb/omapfb-main.c
> @@ -2388,7 +2388,7 @@ static int omapfb_init_connections(struct omapfb2_device *fbdev,
> struct omap_dss_device *dssdev = fbdev->displays[i].dssdev;
> struct omap_dss_output *out = dssdev->output;
>
> - mgr = omap_dss_get_overlay_manager(dssdev->channel);
> + mgr = omap_dss_get_overlay_manager(out->dispc_channel);
>
> if (!mgr || !out)
> continue;
> diff --git a/include/video/omapdss.h b/include/video/omapdss.h
> index ba9cea7..ec322a7 100644
> --- a/include/video/omapdss.h
> +++ b/include/video/omapdss.h
> @@ -547,6 +547,9 @@ struct omap_dss_output {
> /* display type supported by the output */
> enum omap_display_type type;
>
> + /* DISPC channel for this output */
> + enum omap_channel dispc_channel;
> +
> /* output instance */
> enum omap_dss_output_id id;
>
>
>
^ permalink raw reply
* Re: [PATCH linux-next] mfd: max8925: max8925_backlight_probe: Silence 'statement with no effect' war
From: Arnd Bergmann @ 2013-03-11 15:29 UTC (permalink / raw)
To: Tim Gardner
Cc: linux-kernel, Richard Purdie, Florian Tobias Schandinat,
linux-fbdev
In-Reply-To: <1362939145-88329-1-git-send-email-tim.gardner@canonical.com>
On Sunday 10 March 2013, Tim Gardner wrote:
> drivers/video/backlight/max8925_bl.c: In function 'max8925_backlight_probe':
> drivers/video/backlight/max8925_bl.c:177:3: warning: statement with no effect [-Wunused-value]
>
> gcc version 4.6.3
>
> Convert max8925_backlight_dt_init() to an 'inline void' since it is only
> called from one place where the return code is ignored. Protect the
> guts of the function with '#ifdef CONFIG_OF'.
>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
> drivers/video/backlight/max8925_bl.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
I had already sent a better patch for this earlier, see "mfd: max8925:
fix trivial build warning for non-dt". Unfortunately when I looked into
the problem again, I found more problems with the original patches,
apparently the DT bindings were never properly reviewed. It may be
better to revert the original patch for 3.9 and do it better for 3.10.
Arnd
^ permalink raw reply
* Re: [PATCH v2 2/2] video: imxfb: Add DT support
From: Markus Pargmann @ 2013-03-11 19:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130311102540.GB23082@e106331-lin.cambridge.arm.com>
Hi,
On Mon, Mar 11, 2013 at 10:25:40AM +0000, Mark Rutland wrote:
> Hi,
>
> Any reason for not CCing devicetree-discuss?
There is no reason, sorry, I forgot CC, I will add it to CC for the next
version.
>
> I have a couple of comments on the binding and the way it's parsed.
>
> On Tue, Mar 05, 2013 at 05:30:08PM +0000, Markus Pargmann wrote:
> > Add devicetree support for imx framebuffer driver. It uses the generic
> > display bindings and helper functions.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > Cc: Fabio Estevam <festevam@gmail.com>
> > ---
> >
> > Notes:
> > Changes in v2:
> > - Removed pwmr register property
> > - Cleanup of devicetree binding documentation
> > - Use default values for pwmr and lscr1
> >
> > .../devicetree/bindings/video/fsl,imx-fb.txt | 49 ++++++
> > drivers/video/imxfb.c | 182 +++++++++++++++++----
> > 2 files changed, 197 insertions(+), 34 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> >
> > diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > new file mode 100644
> > index 0000000..e1a53a3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > @@ -0,0 +1,49 @@
> > +Freescale imx21 Framebuffer
> > +
> > +This framebuffer driver supports devices imx1, imx21, imx25, and imx27.
> > +
> > +Required properties:
> > +- compatible : "fsl,<chip>-fb", chip should be imx1 or imx21
> > +- reg : Should contain 1 register ranges(address and length)
> > +- interrupts : One interrupt of the fb dev
> > +
> > +Required nodes:
> > +- display: Phandle to a display node as described in
> > + Documentation/devicetree/bindings/video/display-timing.txt
> > + Additional, the display node has to define properties:
> > + - bpp: Bits per pixel
> > + - pcr: LCDC PCR value
>
> As these are non-standard, it would be good to prefix them (e.g. "fsl,pcr").
Okay.
> If you need them, why are they not a good fit for the generic binding?
I think bpp could be used by some other drivers but not of the majority.
There are actually already some of them having bindings for bpp, e.g.
Documentation/devicetree/bindings/video/via,vt8500-fb.txt .
>
> I'm not familiar with the hardware, what is the PCR exactly?
PCR is an integer that encodes a lot of bools to specify the behavior of
the imxfb-lcd interaction. The alternative would be a lot of optional
bool properties which are parsed in the driver to construct it that way.
>
> [...]
>
> > -static int __init imxfb_probe(struct platform_device *pdev)
> > +static int imxfb_of_read_mode(struct device_node *np,
> > + struct imx_fb_videomode *imxfb_mode)
> > +{
> > + int ret;
> > + struct fb_videomode *of_mode = &imxfb_mode->mode;
> > + u32 bpp;
> > + u32 pcr;
> > +
> > + ret = of_property_read_string(np, "model", &of_mode->name);
> > + if (ret)
> > + of_mode->name = NULL;
> > +
> > + ret = of_get_fb_videomode(np, of_mode, OF_USE_NATIVE_MODE);
> > + if (ret)
> > + return ret;
> > +
> > + ret = of_property_read_u32(np, "bpp", &bpp);
> > + ret |= of_property_read_u32(np, "pcr", &pcr);
> > +
> > + if (ret)
> > + return ret;
>
> Is this return value used anywhere in anything more than an "if (!err)"
> capacity? If so it may be worth having individual return value checks:
>
> If one call returns -EINVAL (-22) and the other -ENODATA (-61), out the other
> end we'd get -EISDIR (-21). If we don't care particularly about which error
> code we actually pass on, we could always return a sensible code when ret is
> nonzero:
>
> if (ret)
> return -EINVAL;
Yes, the error codes are directly passed through the probe function,
so I will change it to return -EINVAL and print an device error message.
>
> > +
> > + if (bpp > 255)
> > + return -EINVAL;
>
> Might it also be worth checking for 0 here?
Yes, changed.
>
> [...]
>
> > @@ -837,15 +914,51 @@ static int __init imxfb_probe(struct platform_device *pdev)
> >
> > fbi = info->par;
> >
> > - if (!fb_mode)
> > - fb_mode = pdata->mode[0].mode.name;
> > -
> > platform_set_drvdata(pdev, info);
> >
> > ret = imxfb_init_fbinfo(pdev);
> > if (ret < 0)
> > goto failed_init;
> >
> > + if (pdata) {
> > + if (!fb_mode)
> > + fb_mode = pdata->mode[0].mode.name;
> > +
> > + fbi->mode = pdata->mode;
> > + fbi->num_modes = pdata->num_modes;
> > + } else {
> > + struct device_node *display_np;
> > + fb_mode = NULL;
> > +
> > + display_np = of_parse_phandle(pdev->dev.of_node, "display", 0);
> > + if (!display_np) {
> > + dev_err(&pdev->dev, "No display defined in devicetree\n");
> > + ret = -EINVAL;
> > + goto failed_of_parse;
> > + }
> > +
> > + /*
> > + * imxfb does not support more modes, we choose only the native
> > + * mode.
> > + */
> > + fbi->num_modes = 1;
> > +
> > + fbi->mode = devm_kzalloc(&pdev->dev,
> > + sizeof(struct imx_fb_videomode), GFP_KERNEL);
> > + if (!fbi->mode) {
> > + ret = -ENOMEM;
> > + goto failed_of_parse;
> > + }
> > +
> > + ret = imxfb_of_read_mode(display_np, fbi->mode);
> > + if (ret)
> > + goto failed_of_parse;
> > + }
> > +
> > + for (i = 0, m = &fbi->mode[0]; i < fbi->num_modes; i++, m++)
> > + info->fix.smem_len = max_t(size_t, info->fix.smem_len,
> > + m->mode.xres * m->mode.yres * m->bpp / 8);
>
> Surely this is broken if bpp is not as multiple of 8?
>
> If we can only handle multiples of 8, could we not sanity check this earlier?
>
> If there's no strong preference for describing it in bits, could we not
> describe it in bytes and side-step the issue?
I think it is more common using bits per pixel. Indeed the for loop
seems to be broken. I fixed it by calculating the maximum bytes used per
pixel before. A grep through the kernel shows that there seem to be some
displays using bpp that are not a multiple of 8.
Thanks for your comments,
Markus
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [PATCH] backlight: fix typo "MACH_SAM9...EK" three times
From: Paul Bolle @ 2013-03-11 20:47 UTC (permalink / raw)
To: Richard Purdie, Florian Tobias Schandinat; +Cc: linux-fbdev, linux-kernel
Fix three typos (originally) introduced by commit
a9a84c37d1ee50db8f3752b117caf2b48dcd4f8a ("atmel_lcdfb: backlight
control").
Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
0) Tested by grepping the tree.
1) So two of these typos were introduced in v2.6.25. (The third was
introduced in commit 915190f7d4f08e413e5fde6b0abcd5375aeacdf4 ("[ARM]
5614/1: at91: atmel_lcdfb: add at91sam9g10 support to atmel LCD
driver")). Checking these commits reveals that the default value of 'y'
has never been set automatically in all releases since v2.6.25! Perhaps
this line might as well be dropped.
drivers/video/backlight/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index db10d01..a4481df 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -161,7 +161,7 @@ if BACKLIGHT_CLASS_DEVICE
config BACKLIGHT_ATMEL_LCDC
bool "Atmel LCDC Contrast-as-Backlight control"
depends on FB_ATMEL
- default y if MACH_SAM9261EK || MACH_SAM9G10EK || MACH_SAM9263EK
+ default y if MACH_AT91SAM9261EK || MACH_AT91SAM9G10EK || MACH_AT91SAM9263EK
help
This provides a backlight control internal to the Atmel LCDC
driver. If the LCD "contrast control" on your board is wired
--
1.7.11.7
^ permalink raw reply related
* [PATCH v2] video: backlight: adp5520: fix compiler warning in adp5520_show
From: Devendra Naga @ 2013-03-11 23:27 UTC (permalink / raw)
To: linux-fbdev
while compiling with make W=1 (gcc gcc (GCC) 4.7.2 20121109 (Red Hat 4.7.2-8))
found the following warning
drivers/video/backlight/adp5520_bl.c: In function ‘adp5520_show’:
drivers/video/backlight/adp5520_bl.c:146:6: warning: variable ‘error’ set but not used [-Wunused-but-set-variable]
fixed by checking the return value of the variable
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Michael Hennerich <michael.hennerich@analog.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
---
drivers/video/backlight/adp5520_bl.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/video/backlight/adp5520_bl.c b/drivers/video/backlight/adp5520_bl.c
index a1e41d4..d923f23 100644
--- a/drivers/video/backlight/adp5520_bl.c
+++ b/drivers/video/backlight/adp5520_bl.c
@@ -143,13 +143,16 @@ static int adp5520_bl_setup(struct backlight_device *bl)
static ssize_t adp5520_show(struct device *dev, char *buf, int reg)
{
struct adp5520_bl *data = dev_get_drvdata(dev);
- int error;
+ int ret;
uint8_t reg_val;
mutex_lock(&data->lock);
- error = adp5520_read(data->master, reg, ®_val);
+ ret = adp5520_read(data->master, reg, ®_val);
mutex_unlock(&data->lock);
+ if (ret < 0)
+ return ret;
+
return sprintf(buf, "%u\n", reg_val);
}
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH] backlight: fix typo "MACH_SAM9...EK" three times
From: Jingoo Han @ 2013-03-11 23:50 UTC (permalink / raw)
To: 'Paul Bolle', 'Andrew Morton'
Cc: 'Richard Purdie', 'Florian Tobias Schandinat',
linux-fbdev, linux-kernel
In-Reply-To: <1363034823.3137.110.camel@x61.thuisdomein>
On Tuesday, March 12, 2013 5:47 AM, Paul Bolle wrote:
>
> Fix three typos (originally) introduced by commit
> a9a84c37d1ee50db8f3752b117caf2b48dcd4f8a ("atmel_lcdfb: backlight
> control").
>
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
CC'ed Andrew Morton.
It looks good.
Acked-by: Jingoo Han <jg1.han@samsung.com>
Best regards,
Jingoo Han
> ---
> 0) Tested by grepping the tree.
>
> 1) So two of these typos were introduced in v2.6.25. (The third was
> introduced in commit 915190f7d4f08e413e5fde6b0abcd5375aeacdf4 ("[ARM]
> 5614/1: at91: atmel_lcdfb: add at91sam9g10 support to atmel LCD
> driver")). Checking these commits reveals that the default value of 'y'
> has never been set automatically in all releases since v2.6.25! Perhaps
> this line might as well be dropped.
>
> drivers/video/backlight/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index db10d01..a4481df 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -161,7 +161,7 @@ if BACKLIGHT_CLASS_DEVICE
> config BACKLIGHT_ATMEL_LCDC
> bool "Atmel LCDC Contrast-as-Backlight control"
> depends on FB_ATMEL
> - default y if MACH_SAM9261EK || MACH_SAM9G10EK || MACH_SAM9263EK
> + default y if MACH_AT91SAM9261EK || MACH_AT91SAM9G10EK || MACH_AT91SAM9263EK
> help
> This provides a backlight control internal to the Atmel LCDC
> driver. If the LCD "contrast control" on your board is wired
> --
> 1.7.11.7
>
^ permalink raw reply
* Re: [PATCH v2] video: backlight: adp5520: fix compiler warning in adp5520_show
From: Jingoo Han @ 2013-03-11 23:52 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1363044454-25111-1-git-send-email-devendra.aaru@gmail.com>
On Tuesday, March 12, 2013 8:28 AM, Devendra Naga wrote:
>
> while compiling with make W=1 (gcc gcc (GCC) 4.7.2 20121109 (Red Hat 4.7.2-8))
> found the following warning
>
> drivers/video/backlight/adp5520_bl.c: In function ‘adp5520_show’:
> drivers/video/backlight/adp5520_bl.c:146:6: warning: variable ‘error’ set but not used [-Wunused-but-
> set-variable]
>
> fixed by checking the return value of the variable
>
> Cc: Jingoo Han <jg1.han@samsung.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
Best regards,
Jingoo Han
> Cc: Michael Hennerich <michael.hennerich@analog.com>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
> ---
> drivers/video/backlight/adp5520_bl.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/adp5520_bl.c b/drivers/video/backlight/adp5520_bl.c
> index a1e41d4..d923f23 100644
> --- a/drivers/video/backlight/adp5520_bl.c
> +++ b/drivers/video/backlight/adp5520_bl.c
> @@ -143,13 +143,16 @@ static int adp5520_bl_setup(struct backlight_device *bl)
> static ssize_t adp5520_show(struct device *dev, char *buf, int reg)
> {
> struct adp5520_bl *data = dev_get_drvdata(dev);
> - int error;
> + int ret;
> uint8_t reg_val;
>
> mutex_lock(&data->lock);
> - error = adp5520_read(data->master, reg, ®_val);
> + ret = adp5520_read(data->master, reg, ®_val);
> mutex_unlock(&data->lock);
>
> + if (ret < 0)
> + return ret;
> +
> return sprintf(buf, "%u\n", reg_val);
> }
>
> --
> 1.8.1.2
^ permalink raw reply
* [PATCH 1/5] videomode: simplify videomode Kconfig and Makefile
From: Tomi Valkeinen @ 2013-03-12 10:19 UTC (permalink / raw)
To: Steffen Trumtrar, linux-fbdev, dri-devel, Laurent Pinchart; +Cc: Tomi Valkeinen
This patch simplifies videomode related Kconfig and Makefile. After this
patch, there's only one non-user selectable Kconfig option left,
VIDEOMODE_HELPERS. The reasons for the change:
* Videomode helper functions are not something that should be shown in
the kernel configuration options. The related code should just be
included if it's needed, i.e. selected by drivers using videomode.
* There's no need to have separate Kconfig options for videomode and
display_timing. First of all, the amount of code for both is quite
small. Second, videomode depends on display_timing, and display_timing
in itself is not really useful, so both would be included in any case.
* CONFIG_VIDEOMODE is a bit vague name, and CONFIG_VIDEOMODE_HELPERS
describes better what's included.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
drivers/gpu/drm/drm_modes.c | 8 ++++----
drivers/gpu/drm/tilcdc/Kconfig | 3 +--
drivers/video/Kconfig | 22 ++--------------------
drivers/video/Makefile | 8 ++++----
drivers/video/fbmon.c | 8 ++++----
5 files changed, 15 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 04fa6f1..0698c0e 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -506,7 +506,7 @@ drm_gtf_mode(struct drm_device *dev, int hdisplay, int vdisplay, int vrefresh,
}
EXPORT_SYMBOL(drm_gtf_mode);
-#if IS_ENABLED(CONFIG_VIDEOMODE)
+#ifdef CONFIG_VIDEOMODE_HELPERS
int drm_display_mode_from_videomode(const struct videomode *vm,
struct drm_display_mode *dmode)
{
@@ -540,9 +540,8 @@ int drm_display_mode_from_videomode(const struct videomode *vm,
return 0;
}
EXPORT_SYMBOL_GPL(drm_display_mode_from_videomode);
-#endif
-#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
+#ifdef CONFIG_OF
/**
* of_get_drm_display_mode - get a drm_display_mode from devicetree
* @np: device_node with the timing specification
@@ -572,7 +571,8 @@ int of_get_drm_display_mode(struct device_node *np,
return 0;
}
EXPORT_SYMBOL_GPL(of_get_drm_display_mode);
-#endif
+#endif /* CONFIG_OF */
+#endif /* CONFIG_VIDEOMODE_HELPERS */
/**
* drm_mode_set_name - set the name on a mode
diff --git a/drivers/gpu/drm/tilcdc/Kconfig b/drivers/gpu/drm/tilcdc/Kconfig
index d24d040..e461e99 100644
--- a/drivers/gpu/drm/tilcdc/Kconfig
+++ b/drivers/gpu/drm/tilcdc/Kconfig
@@ -4,8 +4,7 @@ config DRM_TILCDC
select DRM_KMS_HELPER
select DRM_KMS_CMA_HELPER
select DRM_GEM_CMA_HELPER
- select OF_VIDEOMODE
- select OF_DISPLAY_TIMING
+ select VIDEOMODE_HELPERS
select BACKLIGHT_CLASS_DEVICE
help
Choose this option if you have an TI SoC with LCDC display
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 4c1546f..2a81b11 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -31,26 +31,8 @@ config VIDEO_OUTPUT_CONTROL
This framework adds support for low-level control of the video
output switch.
-config DISPLAY_TIMING
- bool
-
-config VIDEOMODE
- bool
-
-config OF_DISPLAY_TIMING
- bool "Enable device tree display timing support"
- depends on OF
- select DISPLAY_TIMING
- help
- helper to parse display timings from the devicetree
-
-config OF_VIDEOMODE
- bool "Enable device tree videomode support"
- depends on OF
- select VIDEOMODE
- select OF_DISPLAY_TIMING
- help
- helper to get videomodes from the devicetree
+config VIDEOMODE_HELPERS
+ bool
config HDMI
bool
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 9df3873..e414378 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -171,7 +171,7 @@ obj-$(CONFIG_FB_VIRTUAL) += vfb.o
#video output switch sysfs driver
obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
-obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
-obj-$(CONFIG_OF_DISPLAY_TIMING) += of_display_timing.o
-obj-$(CONFIG_VIDEOMODE) += videomode.o
-obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
+obj-$(CONFIG_VIDEOMODE_HELPERS) += display_timing.o videomode.o
+ifeq ($(CONFIG_OF),y)
+obj-$(CONFIG_VIDEOMODE_HELPERS) += of_display_timing.o of_videomode.o
+endif
diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
index 94ad0f7..368cedf 100644
--- a/drivers/video/fbmon.c
+++ b/drivers/video/fbmon.c
@@ -1376,7 +1376,7 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf
return err;
}
-#if IS_ENABLED(CONFIG_VIDEOMODE)
+#ifdef CONFIG_VIDEOMODE_HELPERS
int fb_videomode_from_videomode(const struct videomode *vm,
struct fb_videomode *fbmode)
{
@@ -1424,9 +1424,8 @@ int fb_videomode_from_videomode(const struct videomode *vm,
return 0;
}
EXPORT_SYMBOL_GPL(fb_videomode_from_videomode);
-#endif
-#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
+#ifdef CONFIG_OF
static inline void dump_fb_videomode(const struct fb_videomode *m)
{
pr_debug("fb_videomode = %ux%u@%uHz (%ukHz) %u %u %u %u %u %u %u %u %u\n",
@@ -1465,7 +1464,8 @@ int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
return 0;
}
EXPORT_SYMBOL_GPL(of_get_fb_videomode);
-#endif
+#endif /* CONFIG_OF */
+#endif /* CONFIG_VIDEOMODE_HELPERS */
#else
int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var)
--
1.7.10.4
^ permalink raw reply related
* [PATCH 2/5] videomode: combine videomode dmt_flags and data_flags
From: Tomi Valkeinen @ 2013-03-12 10:19 UTC (permalink / raw)
To: Steffen Trumtrar, linux-fbdev, dri-devel, Laurent Pinchart; +Cc: Tomi Valkeinen
In-Reply-To: <1363083578-17062-1-git-send-email-tomi.valkeinen@ti.com>
Both videomode and display_timing contain flags describing the modes.
These are stored in dmt_flags and data_flags. There's no need to
separate these flags, and having separate fields just makes the flags
more difficult to use.
This patch combines the fields and renames VESA_DMT_* flags to
DISPLAY_FLAGS_*.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
drivers/gpu/drm/drm_modes.c | 12 ++++++------
drivers/video/fbmon.c | 8 ++++----
drivers/video/of_display_timing.c | 19 +++++++++----------
drivers/video/videomode.c | 3 +--
include/video/display_timing.h | 26 +++++++++++---------------
include/video/videomode.h | 3 +--
6 files changed, 32 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 0698c0e..f83f071 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -523,17 +523,17 @@ int drm_display_mode_from_videomode(const struct videomode *vm,
dmode->clock = vm->pixelclock / 1000;
dmode->flags = 0;
- if (vm->dmt_flags & VESA_DMT_HSYNC_HIGH)
+ if (vm->flags & DISPLAY_FLAGS_HSYNC_HIGH)
dmode->flags |= DRM_MODE_FLAG_PHSYNC;
- else if (vm->dmt_flags & VESA_DMT_HSYNC_LOW)
+ else if (vm->flags & DISPLAY_FLAGS_HSYNC_LOW)
dmode->flags |= DRM_MODE_FLAG_NHSYNC;
- if (vm->dmt_flags & VESA_DMT_VSYNC_HIGH)
+ if (vm->flags & DISPLAY_FLAGS_VSYNC_HIGH)
dmode->flags |= DRM_MODE_FLAG_PVSYNC;
- else if (vm->dmt_flags & VESA_DMT_VSYNC_LOW)
+ else if (vm->flags & DISPLAY_FLAGS_VSYNC_LOW)
dmode->flags |= DRM_MODE_FLAG_NVSYNC;
- if (vm->data_flags & DISPLAY_FLAGS_INTERLACED)
+ if (vm->flags & DISPLAY_FLAGS_INTERLACED)
dmode->flags |= DRM_MODE_FLAG_INTERLACE;
- if (vm->data_flags & DISPLAY_FLAGS_DOUBLESCAN)
+ if (vm->flags & DISPLAY_FLAGS_DOUBLESCAN)
dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
drm_mode_set_name(dmode);
diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
index 368cedf..e5cc2fd 100644
--- a/drivers/video/fbmon.c
+++ b/drivers/video/fbmon.c
@@ -1398,13 +1398,13 @@ int fb_videomode_from_videomode(const struct videomode *vm,
fbmode->sync = 0;
fbmode->vmode = 0;
- if (vm->dmt_flags & VESA_DMT_HSYNC_HIGH)
+ if (vm->flags & DISPLAY_FLAGS_HSYNC_HIGH)
fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
- if (vm->dmt_flags & VESA_DMT_HSYNC_HIGH)
+ if (vm->flags & DISPLAY_FLAGS_HSYNC_HIGH)
fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
- if (vm->data_flags & DISPLAY_FLAGS_INTERLACED)
+ if (vm->flags & DISPLAY_FLAGS_INTERLACED)
fbmode->vmode |= FB_VMODE_INTERLACED;
- if (vm->data_flags & DISPLAY_FLAGS_DOUBLESCAN)
+ if (vm->flags & DISPLAY_FLAGS_DOUBLESCAN)
fbmode->vmode |= FB_VMODE_DOUBLE;
fbmode->flag = 0;
diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
index 13ecd98..56009bc 100644
--- a/drivers/video/of_display_timing.c
+++ b/drivers/video/of_display_timing.c
@@ -79,25 +79,24 @@ static struct display_timing *of_get_display_timing(struct device_node *np)
ret |= parse_timing_property(np, "vsync-len", &dt->vsync_len);
ret |= parse_timing_property(np, "clock-frequency", &dt->pixelclock);
- dt->dmt_flags = 0;
- dt->data_flags = 0;
+ dt->flags = 0;
if (!of_property_read_u32(np, "vsync-active", &val))
- dt->dmt_flags |= val ? VESA_DMT_VSYNC_HIGH :
- VESA_DMT_VSYNC_LOW;
+ dt->flags |= val ? DISPLAY_FLAGS_VSYNC_HIGH :
+ DISPLAY_FLAGS_VSYNC_LOW;
if (!of_property_read_u32(np, "hsync-active", &val))
- dt->dmt_flags |= val ? VESA_DMT_HSYNC_HIGH :
- VESA_DMT_HSYNC_LOW;
+ dt->flags |= val ? DISPLAY_FLAGS_HSYNC_HIGH :
+ DISPLAY_FLAGS_HSYNC_LOW;
if (!of_property_read_u32(np, "de-active", &val))
- dt->data_flags |= val ? DISPLAY_FLAGS_DE_HIGH :
+ dt->flags |= val ? DISPLAY_FLAGS_DE_HIGH :
DISPLAY_FLAGS_DE_LOW;
if (!of_property_read_u32(np, "pixelclk-active", &val))
- dt->data_flags |= val ? DISPLAY_FLAGS_PIXDATA_POSEDGE :
+ dt->flags |= val ? DISPLAY_FLAGS_PIXDATA_POSEDGE :
DISPLAY_FLAGS_PIXDATA_NEGEDGE;
if (of_property_read_bool(np, "interlaced"))
- dt->data_flags |= DISPLAY_FLAGS_INTERLACED;
+ dt->flags |= DISPLAY_FLAGS_INTERLACED;
if (of_property_read_bool(np, "doublescan"))
- dt->data_flags |= DISPLAY_FLAGS_DOUBLESCAN;
+ dt->flags |= DISPLAY_FLAGS_DOUBLESCAN;
if (ret) {
pr_err("%s: error reading timing properties\n",
diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
index 21c47a2..810afff 100644
--- a/drivers/video/videomode.c
+++ b/drivers/video/videomode.c
@@ -31,8 +31,7 @@ int videomode_from_timing(const struct display_timings *disp,
vm->vback_porch = display_timing_get_value(&dt->vback_porch, TE_TYP);
vm->vsync_len = display_timing_get_value(&dt->vsync_len, TE_TYP);
- vm->dmt_flags = dt->dmt_flags;
- vm->data_flags = dt->data_flags;
+ vm->flags = dt->flags;
return 0;
}
diff --git a/include/video/display_timing.h b/include/video/display_timing.h
index 71e9a38..a8a4be5 100644
--- a/include/video/display_timing.h
+++ b/include/video/display_timing.h
@@ -12,19 +12,16 @@
#include <linux/bitops.h>
#include <linux/types.h>
-/* VESA display monitor timing parameters */
-#define VESA_DMT_HSYNC_LOW BIT(0)
-#define VESA_DMT_HSYNC_HIGH BIT(1)
-#define VESA_DMT_VSYNC_LOW BIT(2)
-#define VESA_DMT_VSYNC_HIGH BIT(3)
-
-/* display specific flags */
-#define DISPLAY_FLAGS_DE_LOW BIT(0) /* data enable flag */
-#define DISPLAY_FLAGS_DE_HIGH BIT(1)
-#define DISPLAY_FLAGS_PIXDATA_POSEDGE BIT(2) /* drive data on pos. edge */
-#define DISPLAY_FLAGS_PIXDATA_NEGEDGE BIT(3) /* drive data on neg. edge */
-#define DISPLAY_FLAGS_INTERLACED BIT(4)
-#define DISPLAY_FLAGS_DOUBLESCAN BIT(5)
+#define DISPLAY_FLAGS_HSYNC_LOW BIT(0)
+#define DISPLAY_FLAGS_HSYNC_HIGH BIT(1)
+#define DISPLAY_FLAGS_VSYNC_LOW BIT(2)
+#define DISPLAY_FLAGS_VSYNC_HIGH BIT(3)
+#define DISPLAY_FLAGS_DE_LOW BIT(4) /* data enable flag */
+#define DISPLAY_FLAGS_DE_HIGH BIT(5)
+#define DISPLAY_FLAGS_PIXDATA_POSEDGE BIT(6) /* drive data on pos. edge */
+#define DISPLAY_FLAGS_PIXDATA_NEGEDGE BIT(7) /* drive data on neg. edge */
+#define DISPLAY_FLAGS_INTERLACED BIT(8)
+#define DISPLAY_FLAGS_DOUBLESCAN BIT(9)
/*
* A single signal can be specified via a range of minimal and maximal values
@@ -72,8 +69,7 @@ struct display_timing {
struct timing_entry vback_porch; /* ver. back porch */
struct timing_entry vsync_len; /* ver. sync len */
- unsigned int dmt_flags; /* VESA DMT flags */
- unsigned int data_flags; /* video data flags */
+ unsigned int flags; /* display flags */
};
/*
diff --git a/include/video/videomode.h b/include/video/videomode.h
index a421562..f4ae6ed 100644
--- a/include/video/videomode.h
+++ b/include/video/videomode.h
@@ -29,8 +29,7 @@ struct videomode {
u32 vback_porch;
u32 vsync_len;
- unsigned int dmt_flags; /* VESA DMT flags */
- unsigned int data_flags; /* video data flags */
+ unsigned int flags; /* display flags */
};
/**
--
1.7.10.4
^ permalink raw reply related
* [PATCH 3/5] videomode: create enum for videomode's display flags
From: Tomi Valkeinen @ 2013-03-12 10:19 UTC (permalink / raw)
To: Steffen Trumtrar, linux-fbdev, dri-devel, Laurent Pinchart; +Cc: Tomi Valkeinen
In-Reply-To: <1363083578-17062-1-git-send-email-tomi.valkeinen@ti.com>
Instead of having plain defines for the videomode's flags, add an enum
for the flags. This makes the flags clearer to use, as the enum tells
which values can be used with the flags field.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
include/video/display_timing.h | 28 +++++++++++++++++-----------
include/video/videomode.h | 2 +-
2 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/include/video/display_timing.h b/include/video/display_timing.h
index a8a4be5..b63471d 100644
--- a/include/video/display_timing.h
+++ b/include/video/display_timing.h
@@ -12,16 +12,22 @@
#include <linux/bitops.h>
#include <linux/types.h>
-#define DISPLAY_FLAGS_HSYNC_LOW BIT(0)
-#define DISPLAY_FLAGS_HSYNC_HIGH BIT(1)
-#define DISPLAY_FLAGS_VSYNC_LOW BIT(2)
-#define DISPLAY_FLAGS_VSYNC_HIGH BIT(3)
-#define DISPLAY_FLAGS_DE_LOW BIT(4) /* data enable flag */
-#define DISPLAY_FLAGS_DE_HIGH BIT(5)
-#define DISPLAY_FLAGS_PIXDATA_POSEDGE BIT(6) /* drive data on pos. edge */
-#define DISPLAY_FLAGS_PIXDATA_NEGEDGE BIT(7) /* drive data on neg. edge */
-#define DISPLAY_FLAGS_INTERLACED BIT(8)
-#define DISPLAY_FLAGS_DOUBLESCAN BIT(9)
+enum display_flags {
+ DISPLAY_FLAGS_HSYNC_LOW = BIT(0),
+ DISPLAY_FLAGS_HSYNC_HIGH = BIT(1),
+ DISPLAY_FLAGS_VSYNC_LOW = BIT(2),
+ DISPLAY_FLAGS_VSYNC_HIGH = BIT(3),
+
+ /* data enable flag */
+ DISPLAY_FLAGS_DE_LOW = BIT(4),
+ DISPLAY_FLAGS_DE_HIGH = BIT(5),
+ /* drive data on pos. edge */
+ DISPLAY_FLAGS_PIXDATA_POSEDGE = BIT(6),
+ /* drive data on neg. edge */
+ DISPLAY_FLAGS_PIXDATA_NEGEDGE = BIT(7),
+ DISPLAY_FLAGS_INTERLACED = BIT(8),
+ DISPLAY_FLAGS_DOUBLESCAN = BIT(9),
+};
/*
* A single signal can be specified via a range of minimal and maximal values
@@ -69,7 +75,7 @@ struct display_timing {
struct timing_entry vback_porch; /* ver. back porch */
struct timing_entry vsync_len; /* ver. sync len */
- unsigned int flags; /* display flags */
+ enum display_flags flags; /* display flags */
};
/*
diff --git a/include/video/videomode.h b/include/video/videomode.h
index f4ae6ed..8b12e60 100644
--- a/include/video/videomode.h
+++ b/include/video/videomode.h
@@ -29,7 +29,7 @@ struct videomode {
u32 vback_porch;
u32 vsync_len;
- unsigned int flags; /* display flags */
+ enum display_flags flags; /* display flags */
};
/**
--
1.7.10.4
^ permalink raw reply related
* [PATCH 4/5] videomode: remove timing_entry_index
From: Tomi Valkeinen @ 2013-03-12 10:19 UTC (permalink / raw)
To: Steffen Trumtrar, linux-fbdev, dri-devel, Laurent Pinchart; +Cc: Tomi Valkeinen
In-Reply-To: <1363083578-17062-1-git-send-email-tomi.valkeinen@ti.com>
Display timing's fields have minimum, typical and maximum values. These
can be accessed by using timing_entry_index enum, and
display_timing_get_value() function.
There's no real need for this extra complexity. The values can be
accessed more easily by just using the min/typ/max fields.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
drivers/video/videomode.c | 18 +++++++++---------
include/video/display_timing.h | 25 -------------------------
2 files changed, 9 insertions(+), 34 deletions(-)
diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
index 810afff..a3d95f2 100644
--- a/drivers/video/videomode.c
+++ b/drivers/video/videomode.c
@@ -20,16 +20,16 @@ int videomode_from_timing(const struct display_timings *disp,
if (!dt)
return -EINVAL;
- vm->pixelclock = display_timing_get_value(&dt->pixelclock, TE_TYP);
- vm->hactive = display_timing_get_value(&dt->hactive, TE_TYP);
- vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, TE_TYP);
- vm->hback_porch = display_timing_get_value(&dt->hback_porch, TE_TYP);
- vm->hsync_len = display_timing_get_value(&dt->hsync_len, TE_TYP);
+ vm->pixelclock = dt->pixelclock.typ;
+ vm->hactive = dt->hactive.typ;
+ vm->hfront_porch = dt->hfront_porch.typ;
+ vm->hback_porch = dt->hback_porch.typ;
+ vm->hsync_len = dt->hsync_len.typ;
- vm->vactive = display_timing_get_value(&dt->vactive, TE_TYP);
- vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, TE_TYP);
- vm->vback_porch = display_timing_get_value(&dt->vback_porch, TE_TYP);
- vm->vsync_len = display_timing_get_value(&dt->vsync_len, TE_TYP);
+ vm->vactive = dt->vactive.typ;
+ vm->vfront_porch = dt->vfront_porch.typ;
+ vm->vback_porch = dt->vback_porch.typ;
+ vm->vsync_len = dt->vsync_len.typ;
vm->flags = dt->flags;
diff --git a/include/video/display_timing.h b/include/video/display_timing.h
index b63471d..5d0259b 100644
--- a/include/video/display_timing.h
+++ b/include/video/display_timing.h
@@ -39,12 +39,6 @@ struct timing_entry {
u32 max;
};
-enum timing_entry_index {
- TE_MIN = 0,
- TE_TYP = 1,
- TE_MAX = 2,
-};
-
/*
* Single "mode" entry. This describes one set of signal timings a display can
* have in one setting. This struct can later be converted to struct videomode
@@ -91,25 +85,6 @@ struct display_timings {
struct display_timing **timings;
};
-/* get value specified by index from struct timing_entry */
-static inline u32 display_timing_get_value(const struct timing_entry *te,
- enum timing_entry_index index)
-{
- switch (index) {
- case TE_MIN:
- return te->min;
- break;
- case TE_TYP:
- return te->typ;
- break;
- case TE_MAX:
- return te->max;
- break;
- default:
- return te->typ;
- }
-}
-
/* get one entry from struct display_timings */
static inline struct display_timing *display_timings_get(const struct
display_timings *disp,
--
1.7.10.4
^ permalink raw reply related
* [PATCH 5/5] videomode: rename fields
From: Tomi Valkeinen @ 2013-03-12 10:19 UTC (permalink / raw)
To: Steffen Trumtrar, linux-fbdev, dri-devel, Laurent Pinchart; +Cc: Tomi Valkeinen
In-Reply-To: <1363083578-17062-1-git-send-email-tomi.valkeinen@ti.com>
Structs videomode and display_timing have rather long field names for
the timing values. Nothing wrong with that as such, but this patch
changes them to abbreviations for the following reasons:
* The timing values often need to be used in calculations, and long
field names makes their direct use clumsier.
* The current names are a bit of a mishmash: some words are used as
such, some are shortened, and for some only first letter is used. Some
names use underscode, some don't. All this makes it difficult to
remember what the field names are.
* The abbreviations used in this patch are very common, and there
shouldn't be any misunderstanding about their meaning.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
drivers/gpu/drm/drm_modes.c | 18 +++++++++---------
drivers/video/fbmon.c | 24 +++++++++++-------------
drivers/video/of_display_timing.c | 16 ++++++++--------
drivers/video/videomode.c | 16 ++++++++--------
include/video/display_timing.h | 16 ++++++++--------
include/video/videomode.h | 18 +++++++++---------
6 files changed, 53 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index f83f071..d744e95 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -510,15 +510,15 @@ EXPORT_SYMBOL(drm_gtf_mode);
int drm_display_mode_from_videomode(const struct videomode *vm,
struct drm_display_mode *dmode)
{
- dmode->hdisplay = vm->hactive;
- dmode->hsync_start = dmode->hdisplay + vm->hfront_porch;
- dmode->hsync_end = dmode->hsync_start + vm->hsync_len;
- dmode->htotal = dmode->hsync_end + vm->hback_porch;
+ dmode->hdisplay = vm->hact;
+ dmode->hsync_start = dmode->hdisplay + vm->hfp;
+ dmode->hsync_end = dmode->hsync_start + vm->hsl;
+ dmode->htotal = dmode->hsync_end + vm->hbp;
- dmode->vdisplay = vm->vactive;
- dmode->vsync_start = dmode->vdisplay + vm->vfront_porch;
- dmode->vsync_end = dmode->vsync_start + vm->vsync_len;
- dmode->vtotal = dmode->vsync_end + vm->vback_porch;
+ dmode->vdisplay = vm->vact;
+ dmode->vsync_start = dmode->vdisplay + vm->vfp;
+ dmode->vsync_end = dmode->vsync_start + vm->vsl;
+ dmode->vtotal = dmode->vsync_end + vm->vbp;
dmode->clock = vm->pixelclock / 1000;
@@ -565,7 +565,7 @@ int of_get_drm_display_mode(struct device_node *np,
drm_display_mode_from_videomode(&vm, dmode);
pr_debug("%s: got %dx%d display mode from %s\n",
- of_node_full_name(np), vm.hactive, vm.vactive, np->name);
+ of_node_full_name(np), vm.hact, vm.vact, np->name);
drm_mode_debug_printmodeline(dmode);
return 0;
diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
index e5cc2fd..8103fc9 100644
--- a/drivers/video/fbmon.c
+++ b/drivers/video/fbmon.c
@@ -1382,15 +1382,15 @@ int fb_videomode_from_videomode(const struct videomode *vm,
{
unsigned int htotal, vtotal;
- fbmode->xres = vm->hactive;
- fbmode->left_margin = vm->hback_porch;
- fbmode->right_margin = vm->hfront_porch;
- fbmode->hsync_len = vm->hsync_len;
+ fbmode->xres = vm->hact;
+ fbmode->left_margin = vm->hbp;
+ fbmode->right_margin = vm->hfp;
+ fbmode->hsync_len = vm->hsl;
- fbmode->yres = vm->vactive;
- fbmode->upper_margin = vm->vback_porch;
- fbmode->lower_margin = vm->vfront_porch;
- fbmode->vsync_len = vm->vsync_len;
+ fbmode->yres = vm->vact;
+ fbmode->upper_margin = vm->vbp;
+ fbmode->lower_margin = vm->vfp;
+ fbmode->vsync_len = vm->vsl;
/* prevent division by zero in KHZ2PICOS macro */
fbmode->pixclock = vm->pixelclock ?
@@ -1408,10 +1408,8 @@ int fb_videomode_from_videomode(const struct videomode *vm,
fbmode->vmode |= FB_VMODE_DOUBLE;
fbmode->flag = 0;
- htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
- vm->hsync_len;
- vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
- vm->vsync_len;
+ htotal = vm->hact + vm->hfp + vm->hbp + vm->hsl;
+ vtotal = vm->vact + vm->vfp + vm->vbp + vm->vsl;
/* prevent division by zero */
if (htotal && vtotal) {
fbmode->refresh = vm->pixelclock / (htotal * vtotal);
@@ -1458,7 +1456,7 @@ int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
fb_videomode_from_videomode(&vm, fb);
pr_debug("%s: got %dx%d display mode from %s\n",
- of_node_full_name(np), vm.hactive, vm.vactive, np->name);
+ of_node_full_name(np), vm.hact, vm.vact, np->name);
dump_fb_videomode(fb);
return 0;
diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
index 56009bc..79660937 100644
--- a/drivers/video/of_display_timing.c
+++ b/drivers/video/of_display_timing.c
@@ -69,14 +69,14 @@ static struct display_timing *of_get_display_timing(struct device_node *np)
return NULL;
}
- ret |= parse_timing_property(np, "hback-porch", &dt->hback_porch);
- ret |= parse_timing_property(np, "hfront-porch", &dt->hfront_porch);
- ret |= parse_timing_property(np, "hactive", &dt->hactive);
- ret |= parse_timing_property(np, "hsync-len", &dt->hsync_len);
- ret |= parse_timing_property(np, "vback-porch", &dt->vback_porch);
- ret |= parse_timing_property(np, "vfront-porch", &dt->vfront_porch);
- ret |= parse_timing_property(np, "vactive", &dt->vactive);
- ret |= parse_timing_property(np, "vsync-len", &dt->vsync_len);
+ ret |= parse_timing_property(np, "hback-porch", &dt->hbp);
+ ret |= parse_timing_property(np, "hfront-porch", &dt->hfp);
+ ret |= parse_timing_property(np, "hactive", &dt->hact);
+ ret |= parse_timing_property(np, "hsync-len", &dt->hsl);
+ ret |= parse_timing_property(np, "vback-porch", &dt->vbp);
+ ret |= parse_timing_property(np, "vfront-porch", &dt->vfp);
+ ret |= parse_timing_property(np, "vactive", &dt->vact);
+ ret |= parse_timing_property(np, "vsync-len", &dt->vsl);
ret |= parse_timing_property(np, "clock-frequency", &dt->pixelclock);
dt->flags = 0;
diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
index a3d95f2..c42689a 100644
--- a/drivers/video/videomode.c
+++ b/drivers/video/videomode.c
@@ -21,15 +21,15 @@ int videomode_from_timing(const struct display_timings *disp,
return -EINVAL;
vm->pixelclock = dt->pixelclock.typ;
- vm->hactive = dt->hactive.typ;
- vm->hfront_porch = dt->hfront_porch.typ;
- vm->hback_porch = dt->hback_porch.typ;
- vm->hsync_len = dt->hsync_len.typ;
+ vm->hact = dt->hact.typ;
+ vm->hfp = dt->hfp.typ;
+ vm->hbp = dt->hbp.typ;
+ vm->hsl = dt->hsl.typ;
- vm->vactive = dt->vactive.typ;
- vm->vfront_porch = dt->vfront_porch.typ;
- vm->vback_porch = dt->vback_porch.typ;
- vm->vsync_len = dt->vsync_len.typ;
+ vm->vact = dt->vact.typ;
+ vm->vfp = dt->vfp.typ;
+ vm->vbp = dt->vbp.typ;
+ vm->vsl = dt->vsl.typ;
vm->flags = dt->flags;
diff --git a/include/video/display_timing.h b/include/video/display_timing.h
index 5d0259b..db98ef9 100644
--- a/include/video/display_timing.h
+++ b/include/video/display_timing.h
@@ -59,15 +59,15 @@ struct timing_entry {
struct display_timing {
struct timing_entry pixelclock;
- struct timing_entry hactive; /* hor. active video */
- struct timing_entry hfront_porch; /* hor. front porch */
- struct timing_entry hback_porch; /* hor. back porch */
- struct timing_entry hsync_len; /* hor. sync len */
+ struct timing_entry hact; /* hor. active video */
+ struct timing_entry hfp; /* hor. front porch */
+ struct timing_entry hbp; /* hor. back porch */
+ struct timing_entry hsl; /* hor. sync len */
- struct timing_entry vactive; /* ver. active video */
- struct timing_entry vfront_porch; /* ver. front porch */
- struct timing_entry vback_porch; /* ver. back porch */
- struct timing_entry vsync_len; /* ver. sync len */
+ struct timing_entry vact; /* ver. active video */
+ struct timing_entry vfp; /* ver. front porch */
+ struct timing_entry vbp; /* ver. back porch */
+ struct timing_entry vsl; /* ver. sync len */
enum display_flags flags; /* display flags */
};
diff --git a/include/video/videomode.h b/include/video/videomode.h
index 8b12e60..b601c0c 100644
--- a/include/video/videomode.h
+++ b/include/video/videomode.h
@@ -19,15 +19,15 @@
struct videomode {
unsigned long pixelclock; /* pixelclock in Hz */
- u32 hactive;
- u32 hfront_porch;
- u32 hback_porch;
- u32 hsync_len;
-
- u32 vactive;
- u32 vfront_porch;
- u32 vback_porch;
- u32 vsync_len;
+ u32 hact;
+ u32 hfp;
+ u32 hbp;
+ u32 hsl;
+
+ u32 vact;
+ u32 vfp;
+ u32 vbp;
+ u32 vsl;
enum display_flags flags; /* display flags */
};
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH 1/5] videomode: simplify videomode Kconfig and Makefile
From: Laurent Pinchart @ 2013-03-12 13:34 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, dri-devel, Steffen Trumtrar
In-Reply-To: <1363083578-17062-1-git-send-email-tomi.valkeinen@ti.com>
Hi Tomi,
Thanks for the patch.
On Tuesday 12 March 2013 12:19:34 Tomi Valkeinen wrote:
> This patch simplifies videomode related Kconfig and Makefile. After this
> patch, there's only one non-user selectable Kconfig option left,
> VIDEOMODE_HELPERS. The reasons for the change:
>
> * Videomode helper functions are not something that should be shown in
> the kernel configuration options. The related code should just be
> included if it's needed, i.e. selected by drivers using videomode.
>
> * There's no need to have separate Kconfig options for videomode and
> display_timing. First of all, the amount of code for both is quite
> small. Second, videomode depends on display_timing, and display_timing
> in itself is not really useful, so both would be included in any case.
>
> * CONFIG_VIDEOMODE is a bit vague name, and CONFIG_VIDEOMODE_HELPERS
> describes better what's included.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
I like that.
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 5/5] videomode: rename fields
From: Laurent Pinchart @ 2013-03-12 13:37 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, dri-devel, Steffen Trumtrar
In-Reply-To: <1363083578-17062-5-git-send-email-tomi.valkeinen@ti.com>
Hi Tomi,
Thanks for the patch.
On Tuesday 12 March 2013 12:19:38 Tomi Valkeinen wrote:
> Structs videomode and display_timing have rather long field names for
> the timing values. Nothing wrong with that as such, but this patch
> changes them to abbreviations for the following reasons:
>
> * The timing values often need to be used in calculations, and long
> field names makes their direct use clumsier.
>
> * The current names are a bit of a mishmash: some words are used as
> such, some are shortened, and for some only first letter is used. Some
> names use underscode, some don't. All this makes it difficult to
> remember what the field names are.
>
> * The abbreviations used in this patch are very common, and there
> shouldn't be any misunderstanding about their meaning.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
I have no strong opinion on this, but I find the existing names easier to
read. I might be biased by having read them often though.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 5/5] videomode: rename fields
From: Tomi Valkeinen @ 2013-03-12 13:40 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-fbdev, dri-devel, Steffen Trumtrar
In-Reply-To: <16625567.P7GdYge43z@avalon>
[-- Attachment #1: Type: text/plain, Size: 1479 bytes --]
Hi,
On 2013-03-12 15:37, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thanks for the patch.
>
> On Tuesday 12 March 2013 12:19:38 Tomi Valkeinen wrote:
>> Structs videomode and display_timing have rather long field names for
>> the timing values. Nothing wrong with that as such, but this patch
>> changes them to abbreviations for the following reasons:
>>
>> * The timing values often need to be used in calculations, and long
>> field names makes their direct use clumsier.
>>
>> * The current names are a bit of a mishmash: some words are used as
>> such, some are shortened, and for some only first letter is used. Some
>> names use underscode, some don't. All this makes it difficult to
>> remember what the field names are.
>>
>> * The abbreviations used in this patch are very common, and there
>> shouldn't be any misunderstanding about their meaning.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
>> ---
>
> I have no strong opinion on this, but I find the existing names easier to
> read. I might be biased by having read them often though.
Yes, the last patch was a bit of a "teaser" =). I found myself typoing
them a lot, using helper local variables to shorten the code lines, and
as I mention in the description, I find them a bit of a mishmash. So,
while they're not used in any drivers yet, I thought it'd be worth a
shot to change them.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ 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