| Message ID | 1395855704-19908-2-git-send-email-Jason.Jin@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Also, use dev_info() instead of pr_err, and never use exclamation marks in driver messages. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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@180000 { compatible = "fsl,t1040-diu", "fsl,diu"; - reg = <0x180000 1000>; + reg = <0x180000 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. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jason.Jin@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@180000 { > compatible = "fsl,t1040-diu", "fsl,diu"; > - reg = <0x180000 1000>; > + reg = <0x180000 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. 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. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Jason.Jin@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@180000 { > > compatible = "fsl,t1040-diu", "fsl,diu"; > > - reg = <0x180000 1000>; > > + reg = <0x180000 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? -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/fsl-diu-fb.c b/drivers/video/fsl-diu-fb.c index 4640846..fbdd166 100644 --- a/drivers/video/fsl-diu-fb.c +++ b/drivers/video/fsl-diu-fb.c @@ -372,6 +372,7 @@ struct fsl_diu_data { unsigned int irq; enum fsl_diu_monitor_port monitor_port; struct diu __iomem *diu_reg; + void __iomem *pixelclk_ptr; spinlock_t reg_lock; u8 dummy_aoi[4 * 4 * 4]; struct diu_ad dummy_ad __aligned(8); @@ -383,6 +384,7 @@ struct fsl_diu_data { __le16 blank_cursor[MAX_CURS * MAX_CURS] __aligned(32); uint8_t edid_data[EDID_LENGTH]; bool has_edid; + u32 saved_pixel_clock; } __aligned(32); /* Determine the DMA address of a member of the fsl_diu_data structure */ @@ -1625,19 +1627,47 @@ static irqreturn_t fsl_diu_isr(int irq, void *dev_id) static int fsl_diu_suspend(struct platform_device *ofdev, pm_message_t state) { struct fsl_diu_data *data; + struct resource res; + int ret; + ret = 0; data = dev_get_drvdata(&ofdev->dev); disable_lcdc(data->fsl_diu_info); - return 0; + 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); + } + } + + return ret; } static int fsl_diu_resume(struct platform_device *ofdev) { struct fsl_diu_data *data; + unsigned int i; data = dev_get_drvdata(&ofdev->dev); - enable_lcdc(data->fsl_diu_info); + if (!diu_ops.set_pixel_clock && data->pixelclk_ptr) + out_be32(data->pixelclk_ptr, data->saved_pixel_clock); + + fsl_diu_enable_interrupts(data); + update_lcdc(data->fsl_diu_info); + + for (i = 0; i < NUM_AOIS; i++) { + if (data->mfb[i].count) + fsl_diu_enable_panel(&data->fsl_diu_info[i]); + } return 0; }