Re: [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro

[Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>]

Hi Laurent,

+Greg, +Sasha to opine on the merit of whether this should go to stable
trees (for my future learning and understanding more so than this
specific case)

On 11/12/2019 17:58, Laurent Pinchart wrote:
> Hello,
> 
> On Wed, Dec 11, 2019 at 12:59:57PM +0000, Kieran Bingham wrote:
>> Hi Morimoto-san,
>>
>> Thank you for the patch,
> 
> Likewise :-)
> 
>> On 11/12/2019 01:55, Kuninori Morimoto wrote:
>>>
>>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>
>>> The address of VSP2_VI6_HGT_LBx_H are
>>> 	VSP2_VI6_HGT_LB0_H : 0x3428
>>> 	VSP2_VI6_HGT_LB1_H : 0x3430
>>> 	VSP2_VI6_HGT_LB2_H : 0x3438
>>> 	VSP2_VI6_HGT_LB3_H : 0x3440
>>>
>>> Thus, VI6_HGT_LBn_H() macro should start from 0x3420 instead of 0x3430.
>>> This patch fixup it.

s/fixup/fixes/


>> I think this deserves a fixes tag:
>>
>> Fixes: 26e0ca22c3b8 ("[media] v4l: Renesas R-Car VSP1 driver")
> 
> Given that this macro is not used, we could argue that it doesn't fix
> anything yet :-) I'd rather avoid having this backported to stable
> kernels as it's not useful to have it there, and thus not add a Fixes

I'm sorry - I'm not sure I can agree here, Do you know that no one will
use this macro when they back port the HGT functionality to an LTSI kernel?

We know the Renesas BSP uses LTSI kernels, and the very nature of the
fact that this typo has been spotted by the Renesas BSP team suggests
that they are indeed looking at/using this functionality ...

(Ok, so maybe they will thus apply the fix themselves, but that's not my
point, and if they 'have' to apply the fix - it should be in stable?)

It feels a bit presumptuous to state that we shouldn't fix this because
/we/ don't utilise it yet, when this issue is in mainline regardless ...


> tag. Kieran, would that be OK with you ?

I would suspect that the work Sasha does would potentially pick this up
anyway; automatically even without the tag?
 (Especially with the keyword fixup/fixes in the commit message)

If I've misunderstood the purpose of the stable trees here, then please
let me know.

Maybe it's more pragmatic to only fix features that are used, but it
seems to me like a bug is a bug ... and if it's there, it should be fixed?

And I don't think that this is a particularly high 'expense' to the
stable trees?

--
Kieran



>>> Reported-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>
>> Otherwise,
>>
>> Yes I can clearly see that this offset is marked as H'3428 at page 32-39
>> within the Gen3 datasheet.
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> and taken in my branch.
> 
>>> ---
>>>  drivers/media/platform/vsp1/vsp1_regs.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h
>>> index 5c67ff9..fe3130d 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_regs.h
>>> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
>>> @@ -706,7 +706,7 @@
>>>  #define VI6_HGT_HUE_AREA_LOWER_SHIFT	16
>>>  #define VI6_HGT_HUE_AREA_UPPER_SHIFT	0
>>>  #define VI6_HGT_LB_TH			0x3424
>>> -#define VI6_HGT_LBn_H(n)		(0x3438 + (n) * 8)
>>> +#define VI6_HGT_LBn_H(n)		(0x3428 + (n) * 8)
>>>  #define VI6_HGT_LBn_V(n)		(0x342c + (n) * 8)
>>>  #define VI6_HGT_HISTO(m, n)		(0x3450 + (m) * 128 + (n) * 4)
>>>  #define VI6_HGT_MAXMIN			0x3750