dm: video: tegra124: incorrect logical condition
Subsystems (2)
RFC 1/1 dm: video: tegra124: incorrect logical condition
2 << 24 | A is always true. To use check against a bitmask we need &.
Identified with cppcheck.
---
I do not have the hardware available. But the current coding is fishy.
Please, clarify what should be coded here.
--- drivers/video/tegra124/sor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/video/tegra124/sor.c +1 -1
@@ -669,7 +669,7 @@ static void tegra_dc_sor_config_panel(struct tegra_dc_sor_data *sor,tegra_sor_write_field (sor,CSTM ,CSTM_ROTCLK_DEFAULT_MASK |CSTM_LVDS_EN_ENABLE , -2 <<CSTM_ROTCLK_SHIFT | +2 <<CSTM_ROTCLK_SHIFT & is_lvds ?CSTM_LVDS_EN_ENABLE :CSTM_LVDS_EN_DISABLE );
Signature · 1 line
2.15.1
RFC 1/1 dm: video: tegra124: incorrect logical condition
Hi all,
Quoted 1 line · On Wed, 31 Jan 2018 01:16:07 +0100 Heinrich Schuchardt xypron.glpk at gmx.de…
2 << 24 | A is always true. To use check against a bitmask we need &.
it is always true, but here we are not checking against a bitmask, so the patch is wrong.
We set or clear register bit (depending on 'is_lvds' value) together with another register bits for ROTCLK config.
So, I think the code should be
2 << CSTM_ROTCLK_SHIFT |
(is_lvds ? CSTM_LVDS_EN_ENABLE : CSTM_LVDS_EN_DISABLE)
But I do not have the hardware to test. Maybe Simon ?
Quoted 16 lines
Identified with cppcheck.
Signed-off-by Heinrich Schuchardt <[email protected]>---
I do not have the hardware available. But the current coding is fishy.Please, clarify what should be coded here.
--- drivers/video/tegra124/sor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/video/tegra124/sor.c +0
-0
drivers/video/tegra124/sor.c static void tegra_dc_sor_config_panel(struct tegra_dc_sor_data *sor, · line 675
Quoted 8 lines
drivers/video/tegra124/sor.c +1
-1@@ -669,7 +669,7 @@ static void tegra_dc_sor_config_panel(struct tegra_dc_sor_data *sor,tegra_sor_write_field (sor,CSTM ,CSTM_ROTCLK_DEFAULT_MASK |CSTM_LVDS_EN_ENABLE , -2 <<CSTM_ROTCLK_SHIFT | +2 <<CSTM_ROTCLK_SHIFT & is_lvds ?CSTM_LVDS_EN_ENABLE :CSTM_LVDS_EN_DISABLE );
Thanks,
Anatolij
RFC 1/1 dm: video: tegra124: incorrect logical condition
Quoted 10 lines · Anatolij Gustschin
Hi all,
Quoted 1 line · On Wed, 31 Jan 2018 01:16:07 +0100 Heinrich Schuchardt xypron.glpk at gmx.de…
2 << 24 | A is always true. To use check against a bitmask we need &.
it is always true, but here we are not checking against a bitmask, so the patch is wrong.
Quoted 7 lines
We set or clear register bit (depending on '
is_lvds' value) together with another register bits for ROTCLK config.So, I think the code should be
2 <<
CSTM_ROTCLK_SHIFT|
(is_lvds?CSTM_LVDS_EN_ENABLE:CSTM_LVDS_EN_DISABLE)
Yes, it could be that the author just didn't consider operator precedence in patch 00f37327524.
tegra_dc_sor_enable_lane_sequencer(), tegra_dc_sor_power_up(), and tegra_dc_sor_config_panel() are only called with is_lvds = 0. We might want to eleminate the parameter.
The U-Boot lines in question relate to Linux kernel drivers/gpu/drm/tegra/hdmi.c:
value = tegra_hdmi_readl(hdmi, HDMI_NV_PDISP_SOR_CSTM);
value &= ~SOR_CSTM_ROTCLK(~0);
value |= SOR_CSTM_ROTCLK(2);
value |= SOR_CSTM_PLLDIV;
value &= ~SOR_CSTM_LVDS_ENABLE;
value &= ~SOR_CSTM_MODE_MASK;
value |= SOR_CSTM_MODE_TMDS;
tegra_hdmi_writel(hdmi, value, HDMI_NV_PDISP_SOR_CSTM);The kernel code only uses the disabling of LVDS. And yes it is used in conjunction with the 2 << CSTM_ROTCLK_SHIFT (== SOR_CSTM_ROTCLK(2)) bit.
With the change that you suggested we would flip both bits to the values used by the kernel.
Patch 00f37327524 was about adding support for eDP LCD panels. So probably the author wanted to disable LVDS here.
Regards
Heinrich
Quoted 32 lines
But I do not have the hardware to test. Maybe Simon ?
Quoted 16 lines
Identified with cppcheck.
Signed-off-by Heinrich Schuchardt <[email protected]>---
I do not have the hardware available. But the current coding is fishy.Please, clarify what should be coded here.
--- drivers/video/tegra124/sor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/video/tegra124/sor.c +0
-0drivers/video/tegra124/sor.c static void tegra_dc_sor_config_panel(struct tegra_dc_sor_data *sor, · line 675
Quoted 8 lines
drivers/video/tegra124/sor.c +1
-1@@ -669,7 +669,7 @@ static void tegra_dc_sor_config_panel(struct tegra_dc_sor_data *sor,tegra_sor_write_field (sor,CSTM ,CSTM_ROTCLK_DEFAULT_MASK |CSTM_LVDS_EN_ENABLE , -2 <<CSTM_ROTCLK_SHIFT | +2 <<CSTM_ROTCLK_SHIFT & is_lvds ?CSTM_LVDS_EN_ENABLE :CSTM_LVDS_EN_DISABLE );Thanks,
Anatolij