Hi Kieran,
On Wed, Dec 11, 2019 at 09:58:11PM +0000, Kieran Bingham wrote:
> 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)
Good idea, I can learn too :-)
> On 11/12/2019 17:58, Laurent Pinchart wrote:
> > 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@xxxxxxxxxxx>
> >>>
> >>> 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?
Before backporting it, it would have to be implemented :-) HGT support
is in mainline, but not support for the HGT LB (Letter Box) isn't.
There's no plan I'm aware of to implement this feature at the moment. I
assume that, if and when that feature will be implemented and
backported, this patch could be backported as well.
> 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?
>
> >>> Reported-by: Koji Matsuoka <koji.matsuoka.xm@xxxxxxxxxxx>
> >>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxxxx>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >
> > 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
--
Regards,
Laurent Pinchart
