| Message ID | ZH7vP2Swu8CYpgUL@moroto (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dpu: tidy up some error checking | expand |
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 <dan.carpenter@linaro.org> > --- Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
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 <dan.carpenter@linaro.org> Suggested-by: Marijn Suijten <marijn.suijten@somainline.org> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > --- > 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 >
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
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.
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,
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
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",
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 <dan.carpenter@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)