Re: [PATCH] drm/msm/dpu: tidy up some error checking
On Thu, Jun 08, 2023 at 02:26:14AM +0300, Dmitry Baryshkov wrote:
> On 07/06/2023 17:42, Dan Carpenter wrote:
> > On Tue, Jun 06, 2023 at 10:23:46PM +0200, Marijn Suijten wrote:
> > > > @@ -359,8 +359,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
> > > > * frequency divided by the no. of rows (lines) in the LCDpanel.
> > > > */
> > > > vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync");
> > > > - if (vsync_hz <= 0) {
> > > > - DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %u\n",
> > > > + if (!vsync_hz) {
> > > > + DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %lu\n",
> > > > vsync_hz);
> > >
> > > Nit: no need to print the value here, you know it's zero. Could be
> > > clarified to just "no vsync clock".
> > >
> >
> > Yeah. That's obviously not useful. Sorry, I will resend.
>
> I'll fix while applying. Seems easier.
Thanks!
regards,
dan carpenter
Re: [PATCH] drm/msm/dpu: tidy up some error checking
On Tue, 06 Jun 2023 11:33:03 +0300, Dan Carpenter wrote: > The "vsync_hz" variable is unsigned int so it can't be less > than zero. The dpu_kms_get_clk_rate() function used to return a u64 > but I previously changed it to return an unsigned long and zero on > error so it matches clk_get_rate(). > > Change the "vsync_hz" type to unsigned long as well and change the > error checking to check for zero instead of negatives. This change > does not affect runtime at all. It's just a clean up. > > [...] Applied, thanks! [1/1] drm/msm/dpu: tidy up some error checking https://gitlab.freedesktop.org/lumag/msm/-/commit/e7a2cf8e058e Best regards, -- Dmitry Baryshkov
Re: [PATCH] drm/msm/dpu: tidy up some error checking
On 07/06/2023 17:42, Dan Carpenter wrote:
On Tue, Jun 06, 2023 at 10:23:46PM +0200, Marijn Suijten wrote:
@@ -359,8 +359,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
* frequency divided by the no. of rows (lines) in the LCDpanel.
*/
vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync");
- if (vsync_hz <= 0) {
- DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %u\n",
+ if (!vsync_hz) {
+ DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %lu\n",
vsync_hz);
Nit: no need to print the value here, you know it's zero. Could be
clarified to just "no vsync clock".
Yeah. That's obviously not useful. Sorry, I will resend.
I'll fix while applying. Seems easier.
--
With best wishes
Dmitry
Re: [PATCH] drm/msm/dpu: tidy up some error checking
On Tue, Jun 06, 2023 at 10:23:46PM +0200, Marijn Suijten wrote:
> > @@ -359,8 +359,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
> > * frequency divided by the no. of rows (lines) in the LCDpanel.
> > */
> > vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync");
> > - if (vsync_hz <= 0) {
> > - DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %u\n",
> > + if (!vsync_hz) {
> > + DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %lu\n",
> > vsync_hz);
>
> Nit: no need to print the value here, you know it's zero. Could be
> clarified to just "no vsync clock".
>
Yeah. That's obviously not useful. Sorry, I will resend.
regards,
dan carpenter
Re: [PATCH] drm/msm/dpu: tidy up some error checking
On 2023-06-06 11:33:03, Dan Carpenter wrote:
> The "vsync_hz" variable is unsigned int so it can't be less
> than zero. The dpu_kms_get_clk_rate() function used to return a u64
> but I previously changed it to return an unsigned long and zero on
> error so it matches clk_get_rate().
>
> Change the "vsync_hz" type to unsigned long as well and change the
> error checking to check for zero instead of negatives. This change
> does not affect runtime at all. It's just a clean up.
>
> Signed-off-by: Dan Carpenter
Suggested-by: Marijn Suijten
Reviewed-by: Marijn Suijten
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index d8ed85a238af..6aecaba14e7e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -324,7 +324,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
> struct dpu_hw_tear_check tc_cfg = { 0 };
> struct drm_display_mode *mode;
> bool tc_enable = true;
> - u32 vsync_hz;
> + unsigned long vsync_hz;
> struct dpu_kms *dpu_kms;
>
> if (phys_enc->has_intf_te) {
> @@ -359,8 +359,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
>* frequency divided by the no. of rows (lines) in the LCDpanel.
>*/
> vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync");
> - if (vsync_hz <= 0) {
> - DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %u\n",
> + if (!vsync_hz) {
> + DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %lu\n",
>vsync_hz);
Nit: no need to print the value here, you know it's zero. Could be
clarified to just "no vsync clock".
- Marijn
> return;
> }
> @@ -381,7 +381,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
> tc_cfg.rd_ptr_irq = mode->vdisplay + 1;
>
> DPU_DEBUG_CMDENC(cmd_enc,
> - "tc vsync_clk_speed_hz %u vtotal %u vrefresh %u\n",
> + "tc vsync_clk_speed_hz %lu vtotal %u vrefresh %u\n",
> vsync_hz, mode->vtotal, drm_mode_vrefresh(mode));
> DPU_DEBUG_CMDENC(cmd_enc,
> "tc enable %u start_pos %u rd_ptr_irq %u\n",
> --
> 2.39.2
>
Re: [PATCH] drm/msm/dpu: tidy up some error checking
On 6/6/2023 1:33 AM, Dan Carpenter wrote: The "vsync_hz" variable is unsigned int so it can't be less than zero. The dpu_kms_get_clk_rate() function used to return a u64 but I previously changed it to return an unsigned long and zero on error so it matches clk_get_rate(). Change the "vsync_hz" type to unsigned long as well and change the error checking to check for zero instead of negatives. This change does not affect runtime at all. It's just a clean up. Signed-off-by: Dan Carpenter --- Reviewed-by: Abhinav Kumar
[PATCH] drm/msm/dpu: tidy up some error checking
The "vsync_hz" variable is unsigned int so it can't be less
than zero. The dpu_kms_get_clk_rate() function used to return a u64
but I previously changed it to return an unsigned long and zero on
error so it matches clk_get_rate().
Change the "vsync_hz" type to unsigned long as well and change the
error checking to check for zero instead of negatives. This change
does not affect runtime at all. It's just a clean up.
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index d8ed85a238af..6aecaba14e7e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -324,7 +324,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
struct dpu_hw_tear_check tc_cfg = { 0 };
struct drm_display_mode *mode;
bool tc_enable = true;
- u32 vsync_hz;
+ unsigned long vsync_hz;
struct dpu_kms *dpu_kms;
if (phys_enc->has_intf_te) {
@@ -359,8 +359,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
* frequency divided by the no. of rows (lines) in the LCDpanel.
*/
vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync");
- if (vsync_hz <= 0) {
- DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %u\n",
+ if (!vsync_hz) {
+ DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %lu\n",
vsync_hz);
return;
}
@@ -381,7 +381,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
tc_cfg.rd_ptr_irq = mode->vdisplay + 1;
DPU_DEBUG_CMDENC(cmd_enc,
- "tc vsync_clk_speed_hz %u vtotal %u vrefresh %u\n",
+ "tc vsync_clk_speed_hz %lu vtotal %u vrefresh %u\n",
vsync_hz, mode->vtotal, drm_mode_vrefresh(mode));
DPU_DEBUG_CMDENC(cmd_enc,
"tc enable %u start_pos %u rd_ptr_irq %u\n",
--
2.39.2
