RE: [PATCH 2/2] Make the diu driver work without board level initilization
+ if (!diu_ops.set_pixel_clock) {
+ data-pixelclk_reg = of_iomap(np, 1);
+ if (!data-pixelclk_reg) {
+ dev_err(pdev-dev, Cannot map pixelclk registers,
please \
+ provide the diu_ops for pixclk setting
instead.\n);
The error message should be in one line if possible, or it will hard to
grep.
If cannot, should split it into two or more lines, like:
+dev_err(pdev-dev, Cannot map pixelclk registers,\n
+please provide the diu_ops for pixclk setting
instead.\n);
Thanks, This has been fixed in the update version, please help to review it at:
http://patchwork.ozlabs.org/patch/335225/
I forgot to add the V2 information in the subject in the update patch so this
may confuse the reviewer, sorry for that.
Best Regards,
Jason
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 2/2] Make the diu driver work without board level initilization
Thanks, This has been fixed in the update version, please help to review it at: http://patchwork.ozlabs.org/patch/335225/ I forgot to add the V2 information in the subject in the update patch so this may confuse the reviewer, sorry for that. It is not fixed in that patch (or did you link the wrong version?). You should never use \ to continue a line in C, other than in macros. Further, it is not permitted to wrap kernel output strings. This is an exception to the 80-column rule. Checkpatch should have told you this. [Jason Jin-R64188] Thanks for pointing out this, I confused the internal version and external version. I'll fix it in next version. Thanks. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 1/2] video/fsl: make the diu driver work without platform hooks
jason@freescale.com wrote: However, the DIU is already initialized in u-boot and we can rely on the settings in u-boot for corenet platform, I don't like that at all. [Jason Jin-R64188] What's the potential issue of this? Most of the IPs need the platform initialization in u-boot. For example, the hwconfig in u-boot used for board specific settings for most platform. The potential problem is that it's not stable. Power management breaks your code. I won't NACK your patch, but it feels like a band-aid rather than a real solution. [Jason Jin-R64188] Thanks. The power management will break the pixel clock only. We can try to fix it separately. I'll try to explain it in that mail. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 2/2] video/fsl: Fix the sleep function for FSL DIU module
On 03/26/2014 12:41 PM, Jason Jin wrote:
+ if (!diu_ops.set_pixel_clock) {
+ data-saved_pixel_clock = 0;
+ if (of_address_to_resource(ofdev-dev.of_node, 1, res))
+ pr_err(KERN_ERR No pixel clock set func and no pixel
node!\n);
+ else {
+ data-pixelclk_ptr =
+ devm_ioremap(ofdev-dev, res.start,
resource_size(res));
+ if (!data-pixelclk_ptr) {
+ pr_err(KERN_ERR fslfb: could not map pixelclk
register!\n);
+ ret = -ENOMEM;
+ } else
+ data-saved_pixel_clock = in_be32(data-
pixelclk_ptr);
+ }
+ }
This seems very hackish. What node does ofdev point to? I wonder if
this code should be in the platform file instead.
[Jason Jin-R64188] It's not hackish, we can provide the pixel clock register in
the DIU node, I did not provide the dts update as this is only tested on T1040
platform. For other platforms such as p1022 and 8610, we still can use the
pixel clock setting function in the platform.
The dts node update for T1040 is:
display:display@18 {
compatible = fsl,t1040-diu, fsl,diu;
- reg = 0x18 1000;
+ reg = 0x18 1000 0xfc028 4;
interrupts = 74 2 0 0;
};
If saving the pixel clock register likes a hackish. We can provide more
information in the node to move the pixel clock settings to the driver. It
seems that we only need to provide another parameter(the ratio) for pixel clock
setting.
Also, use dev_info() instead of pr_err, and never use exclamation marks
in driver messages.
Ok, Thanks.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 1/2] video/fsl: make the diu driver work without platform hooks
On 03/26/2014 12:41 PM, Jason Jin wrote: This board sepecific initialization mechanism is not feasible i for corenet platform as the corenet platform file is a abstraction of serveral platforms. You can't make it 100% abstract. The DIU driver requires some sort of board support. I think you can make a generic platform file for the DIU. [Jason Jin-R64188] Provide the generic diu platform file is reasonable, but it is will be just the board specific initialization file collection, if we really need to reinitialize something in kernel. However, the DIU is already initialized in u-boot and we can rely on the settings in u-boot for corenet platform, I don't like that at all. [Jason Jin-R64188] What's the potential issue of this? Most of the IPs need the platform initialization in u-boot. For example, the hwconfig in u-boot used for board specific settings for most platform. The diu_ops structure is still open for any board specific initialization if the platform needed. We at least can let the platform to select to use the diu_ops or not. This is the purse for this patch. the only issue is that when DIU wake up from the deepsleep, some of the board specific initialization will lost, such as the pixel clock setting. That is a BIG issue. This is why we have a platform file -- to handle the platform issues. [Jason Jin-R64188] Yes, this is an issue, Actually, this is a SOC level issue, We can try to fix it in the driver by saving the pixel clock in suspend function and restored it in resume function. Thanks. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 2/2] video/fsl: Fix the sleep function for FSL DIU module
jason@freescale.com wrote:
[Jason Jin-R64188] It's not hackish, we can provide the pixel clock
register in the DIU node, I did not provide the dts update as this is
only tested on T1040 platform. For other platforms such as p1022 and 8610,
we still can use the pixel clock setting function in the platform.
The dts node update for T1040 is:
display:display@18 {
compatible = fsl,t1040-diu, fsl,diu;
- reg = 0x18 1000;
+ reg = 0x18 1000 0xfc028 4;
interrupts = 74 2 0 0;
};
This is hackish because you're specifying a single register that you want
to preserve in the DTS file, instead of a platform function which is
where it's supposed to be.
[Jason Jin-R64188] The pixel clock register is actually part of the DIU
registers although it is not implemented in the diu module. Actually we can use
it to set pixel clock in the driver not just saving it in suspend. We can
provide the patch for discussion.
I will think about this some more. I think you are trying too hard to
avoid a platform file, which is why some of this code is hackish to me.
[Jason Jin-R64188] Thanks. Could you please share you thinking for how to setup
the platform file then?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
