* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
@ 2021-12-28 14:53 ` Matthias Brugger
0 siblings, 0 replies; 28+ messages in thread
From: Matthias Brugger @ 2021-12-28 14:53 UTC (permalink / raw)
To: Miles Chen, Chun-Kuang Hu, Philipp Zabel, David Airlie,
Daniel Vetter
Cc: dri-devel, linux-mediatek, linux-arm-kernel, linux-kernel
On 28/12/2021 10:25, Miles Chen wrote:
> Fix unused-but-set variable warning:
> drivers/gpu/drm/mediatek/mtk_cec.c:85:6: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]
>
Actually we ignore the value passed to mtk_cec_mask. In case of
mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
We are not setting CEC_32K_PDN. I wonder which side effect will it have to set
that bit.
Anyway, if it's the right thing to do, we should add:
Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
Regards,
Matthias
> Signed-off-by: Miles Chen <miles.chen@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_cec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_cec.c b/drivers/gpu/drm/mediatek/mtk_cec.c
> index e9cef5c0c8f7..cdfa648910b2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_cec.c
> +++ b/drivers/gpu/drm/mediatek/mtk_cec.c
> @@ -85,7 +85,7 @@ static void mtk_cec_mask(struct mtk_cec *cec, unsigned int offset,
> u32 tmp = readl(cec->regs + offset) & ~mask;
>
> tmp |= val & mask;
> - writel(val, cec->regs + offset);
> + writel(tmp, cec->regs + offset);
> }
>
> void mtk_cec_set_hpd_event(struct device *dev,
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
@ 2021-12-28 14:53 ` Matthias Brugger
0 siblings, 0 replies; 28+ messages in thread
From: Matthias Brugger @ 2021-12-28 14:53 UTC (permalink / raw)
To: Miles Chen, Chun-Kuang Hu, Philipp Zabel, David Airlie,
Daniel Vetter
Cc: linux-mediatek, linux-arm-kernel, dri-devel, linux-kernel
On 28/12/2021 10:25, Miles Chen wrote:
> Fix unused-but-set variable warning:
> drivers/gpu/drm/mediatek/mtk_cec.c:85:6: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]
>
Actually we ignore the value passed to mtk_cec_mask. In case of
mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
We are not setting CEC_32K_PDN. I wonder which side effect will it have to set
that bit.
Anyway, if it's the right thing to do, we should add:
Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
Regards,
Matthias
> Signed-off-by: Miles Chen <miles.chen@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_cec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_cec.c b/drivers/gpu/drm/mediatek/mtk_cec.c
> index e9cef5c0c8f7..cdfa648910b2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_cec.c
> +++ b/drivers/gpu/drm/mediatek/mtk_cec.c
> @@ -85,7 +85,7 @@ static void mtk_cec_mask(struct mtk_cec *cec, unsigned int offset,
> u32 tmp = readl(cec->regs + offset) & ~mask;
>
> tmp |= val & mask;
> - writel(val, cec->regs + offset);
> + writel(tmp, cec->regs + offset);
> }
>
> void mtk_cec_set_hpd_event(struct device *dev,
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
@ 2021-12-28 14:53 ` Matthias Brugger
0 siblings, 0 replies; 28+ messages in thread
From: Matthias Brugger @ 2021-12-28 14:53 UTC (permalink / raw)
To: Miles Chen, Chun-Kuang Hu, Philipp Zabel, David Airlie,
Daniel Vetter
Cc: dri-devel, linux-mediatek, linux-arm-kernel, linux-kernel
On 28/12/2021 10:25, Miles Chen wrote:
> Fix unused-but-set variable warning:
> drivers/gpu/drm/mediatek/mtk_cec.c:85:6: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]
>
Actually we ignore the value passed to mtk_cec_mask. In case of
mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
We are not setting CEC_32K_PDN. I wonder which side effect will it have to set
that bit.
Anyway, if it's the right thing to do, we should add:
Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
Regards,
Matthias
> Signed-off-by: Miles Chen <miles.chen@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_cec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_cec.c b/drivers/gpu/drm/mediatek/mtk_cec.c
> index e9cef5c0c8f7..cdfa648910b2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_cec.c
> +++ b/drivers/gpu/drm/mediatek/mtk_cec.c
> @@ -85,7 +85,7 @@ static void mtk_cec_mask(struct mtk_cec *cec, unsigned int offset,
> u32 tmp = readl(cec->regs + offset) & ~mask;
>
> tmp |= val & mask;
> - writel(val, cec->regs + offset);
> + writel(tmp, cec->regs + offset);
> }
>
> void mtk_cec_set_hpd_event(struct device *dev,
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
2021-12-28 14:53 ` Matthias Brugger
(?)
(?)
@ 2021-12-29 3:04 ` miles.chen
-1 siblings, 0 replies; 28+ messages in thread
From: miles.chen @ 2021-12-29 3:04 UTC (permalink / raw)
To: matthias.bgg
Cc: airlied, chunkuang.hu, daniel, dri-devel, linux-arm-kernel,
linux-kernel, linux-mediatek, miles.chen, p.zabel
Hi,
On 28/12/2021 10:25, Miles Chen wrote:
> Fix unused-but-set variable warning:
>> drivers/gpu/drm/mediatek/mtk_cec.c:85:6: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]
>>
>
>Actually we ignore the value passed to mtk_cec_mask. In case of
>mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
>
>
>We are not setting CEC_32K_PDN. I wonder which side effect will it have to set
>that bit.
I am confused about "not setting CEC_32K_PDN" part,
in case mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
CEC_32K_PDN (BIT(19)) is set.
for exmaple:
CEC_32K_PDN is BIT(19)
PDN is BIT(16)
say tmp = 0xffffffff;
mask = PDN | CEC_32K_PDN;
val = 0 | CEC_32K_PDN;
tmp = fff6ffff, mask = 90000
val = 80000, tmp = fffeffff
u32 tmp = readl(cec->regs + offset) & ~mask; // tmp = fff6ffff
tmp |= val & mask; // tmp = fffeffff
writel(val, cec->regs + offset); // val = 80000, tmp = fffeffff
in both val and tmp case, CEC_32K_PDN is set.
>Anyway, if it's the right thing to do, we should add:
>
>Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
I will add the Fixes tag, thanks.
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
@ 2021-12-29 3:04 ` miles.chen
0 siblings, 0 replies; 28+ messages in thread
From: miles.chen @ 2021-12-29 3:04 UTC (permalink / raw)
To: matthias.bgg
Cc: airlied, chunkuang.hu, daniel, dri-devel, linux-arm-kernel,
linux-kernel, linux-mediatek, miles.chen, p.zabel
Hi,
On 28/12/2021 10:25, Miles Chen wrote:
> Fix unused-but-set variable warning:
>> drivers/gpu/drm/mediatek/mtk_cec.c:85:6: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]
>>
>
>Actually we ignore the value passed to mtk_cec_mask. In case of
>mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
>
>
>We are not setting CEC_32K_PDN. I wonder which side effect will it have to set
>that bit.
I am confused about "not setting CEC_32K_PDN" part,
in case mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
CEC_32K_PDN (BIT(19)) is set.
for exmaple:
CEC_32K_PDN is BIT(19)
PDN is BIT(16)
say tmp = 0xffffffff;
mask = PDN | CEC_32K_PDN;
val = 0 | CEC_32K_PDN;
tmp = fff6ffff, mask = 90000
val = 80000, tmp = fffeffff
u32 tmp = readl(cec->regs + offset) & ~mask; // tmp = fff6ffff
tmp |= val & mask; // tmp = fffeffff
writel(val, cec->regs + offset); // val = 80000, tmp = fffeffff
in both val and tmp case, CEC_32K_PDN is set.
>Anyway, if it's the right thing to do, we should add:
>
>Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
I will add the Fixes tag, thanks.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
@ 2021-12-29 3:04 ` miles.chen
0 siblings, 0 replies; 28+ messages in thread
From: miles.chen @ 2021-12-29 3:04 UTC (permalink / raw)
To: matthias.bgg
Cc: chunkuang.hu, airlied, linux-kernel, dri-devel, miles.chen,
linux-mediatek, linux-arm-kernel
Hi,
On 28/12/2021 10:25, Miles Chen wrote:
> Fix unused-but-set variable warning:
>> drivers/gpu/drm/mediatek/mtk_cec.c:85:6: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]
>>
>
>Actually we ignore the value passed to mtk_cec_mask. In case of
>mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
>
>
>We are not setting CEC_32K_PDN. I wonder which side effect will it have to set
>that bit.
I am confused about "not setting CEC_32K_PDN" part,
in case mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
CEC_32K_PDN (BIT(19)) is set.
for exmaple:
CEC_32K_PDN is BIT(19)
PDN is BIT(16)
say tmp = 0xffffffff;
mask = PDN | CEC_32K_PDN;
val = 0 | CEC_32K_PDN;
tmp = fff6ffff, mask = 90000
val = 80000, tmp = fffeffff
u32 tmp = readl(cec->regs + offset) & ~mask; // tmp = fff6ffff
tmp |= val & mask; // tmp = fffeffff
writel(val, cec->regs + offset); // val = 80000, tmp = fffeffff
in both val and tmp case, CEC_32K_PDN is set.
>Anyway, if it's the right thing to do, we should add:
>
>Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
I will add the Fixes tag, thanks.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
@ 2021-12-29 3:04 ` miles.chen
0 siblings, 0 replies; 28+ messages in thread
From: miles.chen @ 2021-12-29 3:04 UTC (permalink / raw)
To: matthias.bgg
Cc: airlied, chunkuang.hu, daniel, dri-devel, linux-arm-kernel,
linux-kernel, linux-mediatek, miles.chen, p.zabel
Hi,
On 28/12/2021 10:25, Miles Chen wrote:
> Fix unused-but-set variable warning:
>> drivers/gpu/drm/mediatek/mtk_cec.c:85:6: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]
>>
>
>Actually we ignore the value passed to mtk_cec_mask. In case of
>mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
>
>
>We are not setting CEC_32K_PDN. I wonder which side effect will it have to set
>that bit.
I am confused about "not setting CEC_32K_PDN" part,
in case mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
CEC_32K_PDN (BIT(19)) is set.
for exmaple:
CEC_32K_PDN is BIT(19)
PDN is BIT(16)
say tmp = 0xffffffff;
mask = PDN | CEC_32K_PDN;
val = 0 | CEC_32K_PDN;
tmp = fff6ffff, mask = 90000
val = 80000, tmp = fffeffff
u32 tmp = readl(cec->regs + offset) & ~mask; // tmp = fff6ffff
tmp |= val & mask; // tmp = fffeffff
writel(val, cec->regs + offset); // val = 80000, tmp = fffeffff
in both val and tmp case, CEC_32K_PDN is set.
>Anyway, if it's the right thing to do, we should add:
>
>Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
I will add the Fixes tag, thanks.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
2021-12-29 3:04 ` miles.chen
(?)
(?)
@ 2021-12-29 14:25 ` Matthias Brugger
-1 siblings, 0 replies; 28+ messages in thread
From: Matthias Brugger @ 2021-12-29 14:25 UTC (permalink / raw)
To: miles.chen
Cc: airlied, chunkuang.hu, daniel, dri-devel, linux-arm-kernel,
linux-kernel, linux-mediatek, p.zabel
On 29/12/2021 04:04, miles.chen@mediatek.com wrote:
> Hi,
>
> On 28/12/2021 10:25, Miles Chen wrote:
>> Fix unused-but-set variable warning:
>>> drivers/gpu/drm/mediatek/mtk_cec.c:85:6: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]
>>>
>>
>> Actually we ignore the value passed to mtk_cec_mask. In case of
>> mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
>>
>>
>> We are not setting CEC_32K_PDN. I wonder which side effect will it have to set
>> that bit.
>
> I am confused about "not setting CEC_32K_PDN" part,
> in case mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
> CEC_32K_PDN (BIT(19)) is set.
>
> for exmaple:
> CEC_32K_PDN is BIT(19)
> PDN is BIT(16)
> say tmp = 0xffffffff;
You mean reading the register returns 0xffffffff, correct?
>
> mask = PDN | CEC_32K_PDN;
mask = 0x48000
> val = 0 | CEC_32K_PDN;
val = 0x40000
>
> tmp = fff6ffff, mask = 90000
> val = 80000, tmp = fffeffff
>
> u32 tmp = readl(cec->regs + offset) & ~mask; // tmp = fff6ffff
tmp = 0xffffffff & ~(0x48000) // tmp = 0xffb7ffff
> tmp |= val & mask; // tmp = fffeffff
tmp |= 0x40000 & 0x48000 // tmp = 0xff7fffff
> writel(val, cec->regs + offset); // val = 80000, tmp = fffeffff
val = 0x40000, tmp = 0xff7fffff
>
> in both val and tmp case, CEC_32K_PDN is set.
>
You are right, in both cases the bit is set, but the funciton does not do what
it is supposed to do.
With you fix:
With the mask we define bits that
a) are set to zero if not present in val
b) set to one, when part of val, independent of what the value was in the
register read.
mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
Will set CEC_32K_PDN to one and clear PDN in the register.
mtk_cec_mask(cec, RX_GEN_WD, 0, HDMI_PORD_INT_32K_CLR | RX_INT_32K_CLR |
HDMI_HTPLG_INT_32K_CLR | HDMI_PORD_INT_32K_EN |
RX_INT_32K_EN | HDMI_HTPLG_INT_32K_EN);
Will just clear all bits of the mask.
Without your patch, we will just write the val to the register and don't care
what the register value was before that.
We should somehow mention that in the commit message, as it's not only about a
not used variable, it actually has an influence on the value we write(-back) to
the register.
Regards,
Matthias
>> Anyway, if it's the right thing to do, we should add:
>>
>> Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
>
> I will add the Fixes tag, thanks.
>
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
@ 2021-12-29 14:25 ` Matthias Brugger
0 siblings, 0 replies; 28+ messages in thread
From: Matthias Brugger @ 2021-12-29 14:25 UTC (permalink / raw)
To: miles.chen
Cc: airlied, chunkuang.hu, daniel, dri-devel, linux-arm-kernel,
linux-kernel, linux-mediatek, p.zabel
On 29/12/2021 04:04, miles.chen@mediatek.com wrote:
> Hi,
>
> On 28/12/2021 10:25, Miles Chen wrote:
>> Fix unused-but-set variable warning:
>>> drivers/gpu/drm/mediatek/mtk_cec.c:85:6: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]
>>>
>>
>> Actually we ignore the value passed to mtk_cec_mask. In case of
>> mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
>>
>>
>> We are not setting CEC_32K_PDN. I wonder which side effect will it have to set
>> that bit.
>
> I am confused about "not setting CEC_32K_PDN" part,
> in case mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
> CEC_32K_PDN (BIT(19)) is set.
>
> for exmaple:
> CEC_32K_PDN is BIT(19)
> PDN is BIT(16)
> say tmp = 0xffffffff;
You mean reading the register returns 0xffffffff, correct?
>
> mask = PDN | CEC_32K_PDN;
mask = 0x48000
> val = 0 | CEC_32K_PDN;
val = 0x40000
>
> tmp = fff6ffff, mask = 90000
> val = 80000, tmp = fffeffff
>
> u32 tmp = readl(cec->regs + offset) & ~mask; // tmp = fff6ffff
tmp = 0xffffffff & ~(0x48000) // tmp = 0xffb7ffff
> tmp |= val & mask; // tmp = fffeffff
tmp |= 0x40000 & 0x48000 // tmp = 0xff7fffff
> writel(val, cec->regs + offset); // val = 80000, tmp = fffeffff
val = 0x40000, tmp = 0xff7fffff
>
> in both val and tmp case, CEC_32K_PDN is set.
>
You are right, in both cases the bit is set, but the funciton does not do what
it is supposed to do.
With you fix:
With the mask we define bits that
a) are set to zero if not present in val
b) set to one, when part of val, independent of what the value was in the
register read.
mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
Will set CEC_32K_PDN to one and clear PDN in the register.
mtk_cec_mask(cec, RX_GEN_WD, 0, HDMI_PORD_INT_32K_CLR | RX_INT_32K_CLR |
HDMI_HTPLG_INT_32K_CLR | HDMI_PORD_INT_32K_EN |
RX_INT_32K_EN | HDMI_HTPLG_INT_32K_EN);
Will just clear all bits of the mask.
Without your patch, we will just write the val to the register and don't care
what the register value was before that.
We should somehow mention that in the commit message, as it's not only about a
not used variable, it actually has an influence on the value we write(-back) to
the register.
Regards,
Matthias
>> Anyway, if it's the right thing to do, we should add:
>>
>> Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
>
> I will add the Fixes tag, thanks.
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
@ 2021-12-29 14:25 ` Matthias Brugger
0 siblings, 0 replies; 28+ messages in thread
From: Matthias Brugger @ 2021-12-29 14:25 UTC (permalink / raw)
To: miles.chen
Cc: chunkuang.hu, airlied, linux-kernel, dri-devel, linux-mediatek,
linux-arm-kernel
On 29/12/2021 04:04, miles.chen@mediatek.com wrote:
> Hi,
>
> On 28/12/2021 10:25, Miles Chen wrote:
>> Fix unused-but-set variable warning:
>>> drivers/gpu/drm/mediatek/mtk_cec.c:85:6: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]
>>>
>>
>> Actually we ignore the value passed to mtk_cec_mask. In case of
>> mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
>>
>>
>> We are not setting CEC_32K_PDN. I wonder which side effect will it have to set
>> that bit.
>
> I am confused about "not setting CEC_32K_PDN" part,
> in case mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
> CEC_32K_PDN (BIT(19)) is set.
>
> for exmaple:
> CEC_32K_PDN is BIT(19)
> PDN is BIT(16)
> say tmp = 0xffffffff;
You mean reading the register returns 0xffffffff, correct?
>
> mask = PDN | CEC_32K_PDN;
mask = 0x48000
> val = 0 | CEC_32K_PDN;
val = 0x40000
>
> tmp = fff6ffff, mask = 90000
> val = 80000, tmp = fffeffff
>
> u32 tmp = readl(cec->regs + offset) & ~mask; // tmp = fff6ffff
tmp = 0xffffffff & ~(0x48000) // tmp = 0xffb7ffff
> tmp |= val & mask; // tmp = fffeffff
tmp |= 0x40000 & 0x48000 // tmp = 0xff7fffff
> writel(val, cec->regs + offset); // val = 80000, tmp = fffeffff
val = 0x40000, tmp = 0xff7fffff
>
> in both val and tmp case, CEC_32K_PDN is set.
>
You are right, in both cases the bit is set, but the funciton does not do what
it is supposed to do.
With you fix:
With the mask we define bits that
a) are set to zero if not present in val
b) set to one, when part of val, independent of what the value was in the
register read.
mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
Will set CEC_32K_PDN to one and clear PDN in the register.
mtk_cec_mask(cec, RX_GEN_WD, 0, HDMI_PORD_INT_32K_CLR | RX_INT_32K_CLR |
HDMI_HTPLG_INT_32K_CLR | HDMI_PORD_INT_32K_EN |
RX_INT_32K_EN | HDMI_HTPLG_INT_32K_EN);
Will just clear all bits of the mask.
Without your patch, we will just write the val to the register and don't care
what the register value was before that.
We should somehow mention that in the commit message, as it's not only about a
not used variable, it actually has an influence on the value we write(-back) to
the register.
Regards,
Matthias
>> Anyway, if it's the right thing to do, we should add:
>>
>> Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
>
> I will add the Fixes tag, thanks.
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
@ 2021-12-29 14:25 ` Matthias Brugger
0 siblings, 0 replies; 28+ messages in thread
From: Matthias Brugger @ 2021-12-29 14:25 UTC (permalink / raw)
To: miles.chen
Cc: airlied, chunkuang.hu, daniel, dri-devel, linux-arm-kernel,
linux-kernel, linux-mediatek, p.zabel
On 29/12/2021 04:04, miles.chen@mediatek.com wrote:
> Hi,
>
> On 28/12/2021 10:25, Miles Chen wrote:
>> Fix unused-but-set variable warning:
>>> drivers/gpu/drm/mediatek/mtk_cec.c:85:6: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]
>>>
>>
>> Actually we ignore the value passed to mtk_cec_mask. In case of
>> mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
>>
>>
>> We are not setting CEC_32K_PDN. I wonder which side effect will it have to set
>> that bit.
>
> I am confused about "not setting CEC_32K_PDN" part,
> in case mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
> CEC_32K_PDN (BIT(19)) is set.
>
> for exmaple:
> CEC_32K_PDN is BIT(19)
> PDN is BIT(16)
> say tmp = 0xffffffff;
You mean reading the register returns 0xffffffff, correct?
>
> mask = PDN | CEC_32K_PDN;
mask = 0x48000
> val = 0 | CEC_32K_PDN;
val = 0x40000
>
> tmp = fff6ffff, mask = 90000
> val = 80000, tmp = fffeffff
>
> u32 tmp = readl(cec->regs + offset) & ~mask; // tmp = fff6ffff
tmp = 0xffffffff & ~(0x48000) // tmp = 0xffb7ffff
> tmp |= val & mask; // tmp = fffeffff
tmp |= 0x40000 & 0x48000 // tmp = 0xff7fffff
> writel(val, cec->regs + offset); // val = 80000, tmp = fffeffff
val = 0x40000, tmp = 0xff7fffff
>
> in both val and tmp case, CEC_32K_PDN is set.
>
You are right, in both cases the bit is set, but the funciton does not do what
it is supposed to do.
With you fix:
With the mask we define bits that
a) are set to zero if not present in val
b) set to one, when part of val, independent of what the value was in the
register read.
mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
Will set CEC_32K_PDN to one and clear PDN in the register.
mtk_cec_mask(cec, RX_GEN_WD, 0, HDMI_PORD_INT_32K_CLR | RX_INT_32K_CLR |
HDMI_HTPLG_INT_32K_CLR | HDMI_PORD_INT_32K_EN |
RX_INT_32K_EN | HDMI_HTPLG_INT_32K_EN);
Will just clear all bits of the mask.
Without your patch, we will just write the val to the register and don't care
what the register value was before that.
We should somehow mention that in the commit message, as it's not only about a
not used variable, it actually has an influence on the value we write(-back) to
the register.
Regards,
Matthias
>> Anyway, if it's the right thing to do, we should add:
>>
>> Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
>
> I will add the Fixes tag, thanks.
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
2021-12-28 14:53 ` Matthias Brugger
(?)
(?)
@ 2021-12-30 6:56 ` miles.chen
-1 siblings, 0 replies; 28+ messages in thread
From: miles.chen @ 2021-12-30 6:56 UTC (permalink / raw)
To: matthias.bgg
Cc: airlied, chunkuang.hu, daniel, dri-devel, linux-arm-kernel,
linux-kernel, linux-mediatek, miles.chen, p.zabel
>You are right, in both cases the bit is set, but the funciton does not do what
>it is supposed to do.
>Will just clear all bits of the mask.
>
>Without your patch, we will just write the val to the register and don't care
>what the register value was before that.
>
>We should somehow mention that in the commit message, as it's not only about a
>not used variable, it actually has an influence on the value we write(-back) to
>the register.
thanks for the comment. I understand that it's not only about a not used
variable. I talked to our hdmi experts and they think mtk_cec_mask() should
write tmp instead of write val to the register.
I will mention this in the commit message and submit next patch.
Happy new year!
Miles
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
@ 2021-12-30 6:56 ` miles.chen
0 siblings, 0 replies; 28+ messages in thread
From: miles.chen @ 2021-12-30 6:56 UTC (permalink / raw)
To: matthias.bgg
Cc: airlied, chunkuang.hu, daniel, dri-devel, linux-arm-kernel,
linux-kernel, linux-mediatek, miles.chen, p.zabel
>You are right, in both cases the bit is set, but the funciton does not do what
>it is supposed to do.
>Will just clear all bits of the mask.
>
>Without your patch, we will just write the val to the register and don't care
>what the register value was before that.
>
>We should somehow mention that in the commit message, as it's not only about a
>not used variable, it actually has an influence on the value we write(-back) to
>the register.
thanks for the comment. I understand that it's not only about a not used
variable. I talked to our hdmi experts and they think mtk_cec_mask() should
write tmp instead of write val to the register.
I will mention this in the commit message and submit next patch.
Happy new year!
Miles
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
@ 2021-12-30 6:56 ` miles.chen
0 siblings, 0 replies; 28+ messages in thread
From: miles.chen @ 2021-12-30 6:56 UTC (permalink / raw)
To: matthias.bgg
Cc: chunkuang.hu, airlied, linux-kernel, dri-devel, miles.chen,
linux-mediatek, linux-arm-kernel
>You are right, in both cases the bit is set, but the funciton does not do what
>it is supposed to do.
>Will just clear all bits of the mask.
>
>Without your patch, we will just write the val to the register and don't care
>what the register value was before that.
>
>We should somehow mention that in the commit message, as it's not only about a
>not used variable, it actually has an influence on the value we write(-back) to
>the register.
thanks for the comment. I understand that it's not only about a not used
variable. I talked to our hdmi experts and they think mtk_cec_mask() should
write tmp instead of write val to the register.
I will mention this in the commit message and submit next patch.
Happy new year!
Miles
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning
@ 2021-12-30 6:56 ` miles.chen
0 siblings, 0 replies; 28+ messages in thread
From: miles.chen @ 2021-12-30 6:56 UTC (permalink / raw)
To: matthias.bgg
Cc: airlied, chunkuang.hu, daniel, dri-devel, linux-arm-kernel,
linux-kernel, linux-mediatek, miles.chen, p.zabel
>You are right, in both cases the bit is set, but the funciton does not do what
>it is supposed to do.
>Will just clear all bits of the mask.
>
>Without your patch, we will just write the val to the register and don't care
>what the register value was before that.
>
>We should somehow mention that in the commit message, as it's not only about a
>not used variable, it actually has an influence on the value we write(-back) to
>the register.
thanks for the comment. I understand that it's not only about a not used
variable. I talked to our hdmi experts and they think mtk_cec_mask() should
write tmp instead of write val to the register.
I will mention this in the commit message and submit next patch.
Happy new year!
Miles
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3] drm/mediatek: Fix unused-but-set variable warning
2021-12-28 14:53 ` Matthias Brugger
(?)
(?)
@ 2022-01-02 23:46 ` miles.chen
-1 siblings, 0 replies; 28+ messages in thread
From: miles.chen @ 2022-01-02 23:46 UTC (permalink / raw)
To: matthias.bgg, Nathan Chancellor, Nick Desaulniers, Jie Qiu,
Junzhi Zhao, Philipp Zabel
Cc: airlied, chunkuang.hu, daniel, dri-devel, linux-arm-kernel,
linux-kernel, linux-mediatek, miles.chen, llvm
> I'm still not happy with the commit subject, I think it is misleading. Clang
> only helped to find the bug, but the we are fixing something else, that's not
> just a clang warning. But I don't want to nit-pick too much so:
>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
thanks. I think you are right.
I will change the subject to "drm/mediatek: Fix mtk_cec_mask()", remove the
clang part and submit patch v4.
e.g,
"""
drm/mediatek: Fix mtk_cec_mask()
In current implementation, mtk_cec_mask() writes val into target register
and ignores the mask. After talking to our hdmi experts, mtk_cec_mask()
should read a register, clean only mask bits, and update (val | mask) bits
to the register.
Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
"""
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3] drm/mediatek: Fix unused-but-set variable warning
@ 2022-01-02 23:46 ` miles.chen
0 siblings, 0 replies; 28+ messages in thread
From: miles.chen @ 2022-01-02 23:46 UTC (permalink / raw)
To: matthias.bgg, Nathan Chancellor, Nick Desaulniers, Jie Qiu,
Junzhi Zhao, Philipp Zabel
Cc: chunkuang.hu, airlied, llvm, linux-kernel, dri-devel, miles.chen,
linux-mediatek, linux-arm-kernel
> I'm still not happy with the commit subject, I think it is misleading. Clang
> only helped to find the bug, but the we are fixing something else, that's not
> just a clang warning. But I don't want to nit-pick too much so:
>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
thanks. I think you are right.
I will change the subject to "drm/mediatek: Fix mtk_cec_mask()", remove the
clang part and submit patch v4.
e.g,
"""
drm/mediatek: Fix mtk_cec_mask()
In current implementation, mtk_cec_mask() writes val into target register
and ignores the mask. After talking to our hdmi experts, mtk_cec_mask()
should read a register, clean only mask bits, and update (val | mask) bits
to the register.
Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
"""
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3] drm/mediatek: Fix unused-but-set variable warning
@ 2022-01-02 23:46 ` miles.chen
0 siblings, 0 replies; 28+ messages in thread
From: miles.chen @ 2022-01-02 23:46 UTC (permalink / raw)
To: matthias.bgg, Nathan Chancellor, Nick Desaulniers, Jie Qiu,
Junzhi Zhao, Philipp Zabel
Cc: airlied, chunkuang.hu, daniel, dri-devel, linux-arm-kernel,
linux-kernel, linux-mediatek, miles.chen, llvm
> I'm still not happy with the commit subject, I think it is misleading. Clang
> only helped to find the bug, but the we are fixing something else, that's not
> just a clang warning. But I don't want to nit-pick too much so:
>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
thanks. I think you are right.
I will change the subject to "drm/mediatek: Fix mtk_cec_mask()", remove the
clang part and submit patch v4.
e.g,
"""
drm/mediatek: Fix mtk_cec_mask()
In current implementation, mtk_cec_mask() writes val into target register
and ignores the mask. After talking to our hdmi experts, mtk_cec_mask()
should read a register, clean only mask bits, and update (val | mask) bits
to the register.
Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
"""
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3] drm/mediatek: Fix unused-but-set variable warning
@ 2022-01-02 23:46 ` miles.chen
0 siblings, 0 replies; 28+ messages in thread
From: miles.chen @ 2022-01-02 23:46 UTC (permalink / raw)
To: matthias.bgg, Nathan Chancellor, Nick Desaulniers, Jie Qiu,
Junzhi Zhao, Philipp Zabel
Cc: airlied, chunkuang.hu, daniel, dri-devel, linux-arm-kernel,
linux-kernel, linux-mediatek, miles.chen, llvm
> I'm still not happy with the commit subject, I think it is misleading. Clang
> only helped to find the bug, but the we are fixing something else, that's not
> just a clang warning. But I don't want to nit-pick too much so:
>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
thanks. I think you are right.
I will change the subject to "drm/mediatek: Fix mtk_cec_mask()", remove the
clang part and submit patch v4.
e.g,
"""
drm/mediatek: Fix mtk_cec_mask()
In current implementation, mtk_cec_mask() writes val into target register
and ignores the mask. After talking to our hdmi experts, mtk_cec_mask()
should read a register, clean only mask bits, and update (val | mask) bits
to the register.
Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
"""
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3] drm/mediatek: Fix unused-but-set variable warning
2022-01-02 23:46 ` miles.chen
(?)
(?)
@ 2022-01-07 1:40 ` Miles Chen
-1 siblings, 0 replies; 28+ messages in thread
From: Miles Chen @ 2022-01-07 1:40 UTC (permalink / raw)
To: matthias.bgg
Cc: airlied, chunkuang.hu, daniel, dri-devel, jie.qiu, junzhi.zhao,
linux-arm-kernel, linux-kernel, linux-mediatek, llvm, p.zabel
Hi Matthias,
>> I'm still not happy with the commit subject, I think it is misleading. Clang
>> only helped to find the bug, but the we are fixing something else, that's not
>> just a clang warning. But I don't want to nit-pick too much so:
>>
>> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> thanks. I think you are right.
> I will change the subject to "drm/mediatek: Fix mtk_cec_mask()", remove the
> clang part and submit patch v4.
I posted patch v4 https://lore.kernel.org/linux-mediatek/20220103054706.8072-1-miles.chen@mediatek.com/
without your reviewed-by tag.
Would you mind taking a look at the patch?
Miles
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3] drm/mediatek: Fix unused-but-set variable warning
@ 2022-01-07 1:40 ` Miles Chen
0 siblings, 0 replies; 28+ messages in thread
From: Miles Chen @ 2022-01-07 1:40 UTC (permalink / raw)
To: matthias.bgg
Cc: chunkuang.hu, junzhi.zhao, jie.qiu, airlied, llvm, linux-kernel,
dri-devel, linux-mediatek, linux-arm-kernel
Hi Matthias,
>> I'm still not happy with the commit subject, I think it is misleading. Clang
>> only helped to find the bug, but the we are fixing something else, that's not
>> just a clang warning. But I don't want to nit-pick too much so:
>>
>> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> thanks. I think you are right.
> I will change the subject to "drm/mediatek: Fix mtk_cec_mask()", remove the
> clang part and submit patch v4.
I posted patch v4 https://lore.kernel.org/linux-mediatek/20220103054706.8072-1-miles.chen@mediatek.com/
without your reviewed-by tag.
Would you mind taking a look at the patch?
Miles
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3] drm/mediatek: Fix unused-but-set variable warning
@ 2022-01-07 1:40 ` Miles Chen
0 siblings, 0 replies; 28+ messages in thread
From: Miles Chen @ 2022-01-07 1:40 UTC (permalink / raw)
To: matthias.bgg
Cc: airlied, chunkuang.hu, daniel, dri-devel, jie.qiu, junzhi.zhao,
linux-arm-kernel, linux-kernel, linux-mediatek, llvm, p.zabel
Hi Matthias,
>> I'm still not happy with the commit subject, I think it is misleading. Clang
>> only helped to find the bug, but the we are fixing something else, that's not
>> just a clang warning. But I don't want to nit-pick too much so:
>>
>> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> thanks. I think you are right.
> I will change the subject to "drm/mediatek: Fix mtk_cec_mask()", remove the
> clang part and submit patch v4.
I posted patch v4 https://lore.kernel.org/linux-mediatek/20220103054706.8072-1-miles.chen@mediatek.com/
without your reviewed-by tag.
Would you mind taking a look at the patch?
Miles
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3] drm/mediatek: Fix unused-but-set variable warning
@ 2022-01-07 1:40 ` Miles Chen
0 siblings, 0 replies; 28+ messages in thread
From: Miles Chen @ 2022-01-07 1:40 UTC (permalink / raw)
To: matthias.bgg
Cc: airlied, chunkuang.hu, daniel, dri-devel, jie.qiu, junzhi.zhao,
linux-arm-kernel, linux-kernel, linux-mediatek, llvm, p.zabel
Hi Matthias,
>> I'm still not happy with the commit subject, I think it is misleading. Clang
>> only helped to find the bug, but the we are fixing something else, that's not
>> just a clang warning. But I don't want to nit-pick too much so:
>>
>> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> thanks. I think you are right.
> I will change the subject to "drm/mediatek: Fix mtk_cec_mask()", remove the
> clang part and submit patch v4.
I posted patch v4 https://lore.kernel.org/linux-mediatek/20220103054706.8072-1-miles.chen@mediatek.com/
without your reviewed-by tag.
Would you mind taking a look at the patch?
Miles
^ permalink raw reply [flat|nested] 28+ messages in thread