Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro
       [not found] ` <redmine.issue-245033.20191211005426.161918957b73008d@dm.renesas.com>
@ 2019-12-11  1:55   ` Kuninori Morimoto
  2019-12-11 12:59     ` Kieran Bingham
  0 siblings, 1 reply; 11+ messages in thread
From: Kuninori Morimoto @ 2019-12-11  1:55 UTC (permalink / raw)
  To: Laurent Pinchart, Kieran Bingham, Mauro Carvalho Chehab
  Cc: linux-media, linux-renesas-soc, koji.matsuoka.xm,
	takeshi.kihara.df, harunobu.kurokawa.dn, khiem.nguyen.xt,
	hien.dang.eb


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.

Reported-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 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
-- 
2.7.4


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro
  2019-12-11  1:55   ` [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro Kuninori Morimoto
@ 2019-12-11 12:59     ` Kieran Bingham
  2019-12-11 17:58       ` Laurent Pinchart
  0 siblings, 1 reply; 11+ messages in thread
From: Kieran Bingham @ 2019-12-11 12:59 UTC (permalink / raw)
  To: Kuninori Morimoto, Laurent Pinchart, Mauro Carvalho Chehab
  Cc: linux-media, linux-renesas-soc, koji.matsuoka.xm,
	takeshi.kihara.df, harunobu.kurokawa.dn, khiem.nguyen.xt,
	hien.dang.eb

Hi Morimoto-san,

Thank you for the patch,

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.

I think this deserves a fixes tag:

Fixes: 26e0ca22c3b8 ("[media] v4l: Renesas R-Car VSP1 driver")

> 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>

> ---
>  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
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro
  2019-12-11 12:59     ` Kieran Bingham
@ 2019-12-11 17:58       ` Laurent Pinchart
  2019-12-11 21:58         ` Kieran Bingham
  2020-02-12  0:43         ` Kuninori Morimoto
  0 siblings, 2 replies; 11+ messages in thread
From: Laurent Pinchart @ 2019-12-11 17:58 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: Kuninori Morimoto, Mauro Carvalho Chehab, linux-media,
	linux-renesas-soc, koji.matsuoka.xm, takeshi.kihara.df,
	harunobu.kurokawa.dn, khiem.nguyen.xt, hien.dang.eb

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.
> 
> 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
tag. Kieran, would that be OK with you ?

> > 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

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro
  2019-12-11 17:58       ` Laurent Pinchart
@ 2019-12-11 21:58         ` Kieran Bingham
  2019-12-11 23:45           ` Laurent Pinchart
  2019-12-12  7:33           ` Greg Kroah-Hartman
  2020-02-12  0:43         ` Kuninori Morimoto
  1 sibling, 2 replies; 11+ messages in thread
From: Kieran Bingham @ 2019-12-11 21:58 UTC (permalink / raw)
  To: Laurent Pinchart, Greg Kroah-Hartman, Sasha Levin
  Cc: Kuninori Morimoto, Mauro Carvalho Chehab, linux-media,
	linux-renesas-soc, koji.matsuoka.xm, takeshi.kihara.df,
	harunobu.kurokawa.dn, khiem.nguyen.xt, hien.dang.eb

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro
  2019-12-11 21:58         ` Kieran Bingham
@ 2019-12-11 23:45           ` Laurent Pinchart
  2019-12-12  7:33           ` Greg Kroah-Hartman
  1 sibling, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2019-12-11 23:45 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: Greg Kroah-Hartman, Sasha Levin, Kuninori Morimoto,
	Mauro Carvalho Chehab, linux-media, linux-renesas-soc,
	koji.matsuoka.xm, takeshi.kihara.df, harunobu.kurokawa.dn,
	khiem.nguyen.xt, hien.dang.eb

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@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?

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@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

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro
  2019-12-11 21:58         ` Kieran Bingham
  2019-12-11 23:45           ` Laurent Pinchart
@ 2019-12-12  7:33           ` Greg Kroah-Hartman
  2019-12-13 11:55             ` Kieran Bingham
  1 sibling, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2019-12-12  7:33 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: Laurent Pinchart, Sasha Levin, Kuninori Morimoto,
	Mauro Carvalho Chehab, linux-media, linux-renesas-soc,
	koji.matsuoka.xm, takeshi.kihara.df, harunobu.kurokawa.dn,
	khiem.nguyen.xt, hien.dang.eb

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)
> 
> 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 ...

Nothing should be in the kernel tree that is not already used by
something in that specific kernel tree.  We don't care about out-of-tree
code, and especially for stable kernel patches, it does not matter in
the least.

If you have out-of-tree code, you are on your own here, sorry.

So no, no backporting of stuff that no one actually uses in the codebase
itself.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro
  2019-12-12  7:33           ` Greg Kroah-Hartman
@ 2019-12-13 11:55             ` Kieran Bingham
  2019-12-13 13:22               ` Geert Uytterhoeven
  2019-12-15 15:45               ` Sasha Levin
  0 siblings, 2 replies; 11+ messages in thread
From: Kieran Bingham @ 2019-12-13 11:55 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Laurent Pinchart, Sasha Levin, Kuninori Morimoto,
	Mauro Carvalho Chehab, linux-media, linux-renesas-soc,
	koji.matsuoka.xm, takeshi.kihara.df, harunobu.kurokawa.dn,
	khiem.nguyen.xt, hien.dang.eb

Hi Greg, Laurent, Sasha,

On 12/12/2019 07:33, Greg Kroah-Hartman wrote:
> 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)
>>
>> 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 ...
> 
> Nothing should be in the kernel tree that is not already used by
> something in that specific kernel tree.  We don't care about out-of-tree
> code, and especially for stable kernel patches, it does not matter in
> the least.

So perhaps this patch should actually remove this macro rather than fix it?

> If you have out-of-tree code, you are on your own here, sorry.
> 
> So no, no backporting of stuff that no one actually uses in the codebase
> itself.

Ok understood, It was really the 'the macro exists in the kernel, but is
wrong' part that got me.

Along with the fact that we now have various automated machinery that
would likely pick this patch up and backport it anyway?

(Sasha, is that assumption accurate? Or would you/your system have
identified that this macro is not used?)

--
Kieran


> 
> thanks,
> 
> greg k-h
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro
  2019-12-13 11:55             ` Kieran Bingham
@ 2019-12-13 13:22               ` Geert Uytterhoeven
  2019-12-13 22:43                 ` Laurent Pinchart
  2019-12-15 15:45               ` Sasha Levin
  1 sibling, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2019-12-13 13:22 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: Greg Kroah-Hartman, Laurent Pinchart, Sasha Levin,
	Kuninori Morimoto, Mauro Carvalho Chehab,
	Linux Media Mailing List, Linux-Renesas, Koji Matsuoka,
	Takeshi Kihara, Harunobu Kurokawa, Khiem Nguyen, Hien Dang

Hi Kieran,

On Fri, Dec 13, 2019 at 12:55 PM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
> On 12/12/2019 07:33, Greg Kroah-Hartman wrote:
> > On Wed, Dec 11, 2019 at 09:58:11PM +0000, Kieran Bingham wrote:
> >> +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:
> >>> On Wed, Dec 11, 2019 at 12:59:57PM +0000, Kieran Bingham wrote:
> >>>> 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 ...
> >
> > Nothing should be in the kernel tree that is not already used by
> > something in that specific kernel tree.  We don't care about out-of-tree
> > code, and especially for stable kernel patches, it does not matter in
> > the least.
>
> So perhaps this patch should actually remove this macro rather than fix it?

IMHO removing all unused register and register bit definitions from drivers
would not improve them. On the contrary.
These also serve as a kind of documentation, as low-level documentation
about the hardware is not always available publicly.

> > If you have out-of-tree code, you are on your own here, sorry.
> >
> > So no, no backporting of stuff that no one actually uses in the codebase
> > itself.
>
> Ok understood, It was really the 'the macro exists in the kernel, but is
> wrong' part that got me.

There is a difference between code that is not used, and register definitions.

> Along with the fact that we now have various automated machinery that
> would likely pick this patch up and backport it anyway?
>
> (Sasha, is that assumption accurate? Or would you/your system have
> identified that this macro is not used?)

IMHO the real danger lies in not backporting this, and forgetting to
backport this fix when future code that does use this definition is
backported.
This may happen +5 years from now, and the feature may be backported to
a 10-year old LTS(i) kernel, causing subtle issues in your 8 year old car...

Basically we can do 4 things:
  1. Fix the buggy definition, and not backport the fix,
      => may trigger the issue above.
  2. idem, but backport the fix,
  3. Remove the buggy definition, and not backport the removal,
      => may trigger the issue above, if people miss-backport the re-addition.
      => people may just revert the removal when they need the definition.
  4. idem, but backport the removal.
      => same as 3.

So I am in favor of fixing the buggy definition, with a Fixes tag, so
stable will pick it up, and anyone else (if any not using LTS) can look
for Fixes tags and know what fixes to backport.

Of course, all of this can be avoided by using the One Way Of Linux ;-)
https://society.oftrolls.com/@geert/102984898449908163

Just my 2€c.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro
  2019-12-13 13:22               ` Geert Uytterhoeven
@ 2019-12-13 22:43                 ` Laurent Pinchart
  0 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2019-12-13 22:43 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Kieran Bingham, Greg Kroah-Hartman, Sasha Levin,
	Kuninori Morimoto, Mauro Carvalho Chehab,
	Linux Media Mailing List, Linux-Renesas, Koji Matsuoka,
	Takeshi Kihara, Harunobu Kurokawa, Khiem Nguyen, Hien Dang

Hello,

On Fri, Dec 13, 2019 at 02:22:56PM +0100, Geert Uytterhoeven wrote:
> On Fri, Dec 13, 2019 at 12:55 PM Kieran Bingham wrote:
> > On 12/12/2019 07:33, Greg Kroah-Hartman wrote:
> > > On Wed, Dec 11, 2019 at 09:58:11PM +0000, Kieran Bingham wrote:
> > >> +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:
> > >>> On Wed, Dec 11, 2019 at 12:59:57PM +0000, Kieran Bingham wrote:
> > >>>> 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 ...
> > >
> > > Nothing should be in the kernel tree that is not already used by
> > > something in that specific kernel tree.  We don't care about out-of-tree
> > > code, and especially for stable kernel patches, it does not matter in
> > > the least.
> >
> > So perhaps this patch should actually remove this macro rather than fix it?
> 
> IMHO removing all unused register and register bit definitions from drivers
> would not improve them. On the contrary.
> These also serve as a kind of documentation, as low-level documentation
> about the hardware is not always available publicly.
> 
> > > If you have out-of-tree code, you are on your own here, sorry.
> > >
> > > So no, no backporting of stuff that no one actually uses in the codebase
> > > itself.
> >
> > Ok understood, It was really the 'the macro exists in the kernel, but is
> > wrong' part that got me.
> 
> There is a difference between code that is not used, and register definitions.
> 
> > Along with the fact that we now have various automated machinery that
> > would likely pick this patch up and backport it anyway?
> >
> > (Sasha, is that assumption accurate? Or would you/your system have
> > identified that this macro is not used?)
> 
> IMHO the real danger lies in not backporting this, and forgetting to
> backport this fix when future code that does use this definition is
> backported.
> This may happen +5 years from now, and the feature may be backported to
> a 10-year old LTS(i) kernel, causing subtle issues in your 8 year old car...
> 
> Basically we can do 4 things:
>   1. Fix the buggy definition, and not backport the fix,
>       => may trigger the issue above.
>   2. idem, but backport the fix,
>   3. Remove the buggy definition, and not backport the removal,
>       => may trigger the issue above, if people miss-backport the re-addition.
>       => people may just revert the removal when they need the definition.
>   4. idem, but backport the removal.
>       => same as 3.
> 
> So I am in favor of fixing the buggy definition, with a Fixes tag, so
> stable will pick it up, and anyone else (if any not using LTS) can look
> for Fixes tags and know what fixes to backport.
> 
> Of course, all of this can be avoided by using the One Way Of Linux ;-)
> https://society.oftrolls.com/@geert/102984898449908163
> 
> Just my 2€c.

You 2¢ resulted in the Fixed: line being added to the patch in my tree
:-)

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro
  2019-12-13 11:55             ` Kieran Bingham
  2019-12-13 13:22               ` Geert Uytterhoeven
@ 2019-12-15 15:45               ` Sasha Levin
  1 sibling, 0 replies; 11+ messages in thread
From: Sasha Levin @ 2019-12-15 15:45 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: Greg Kroah-Hartman, Laurent Pinchart, Kuninori Morimoto,
	Mauro Carvalho Chehab, linux-media, linux-renesas-soc,
	koji.matsuoka.xm, takeshi.kihara.df, harunobu.kurokawa.dn,
	khiem.nguyen.xt, hien.dang.eb

On Fri, Dec 13, 2019 at 11:55:14AM +0000, Kieran Bingham wrote:
>Hi Greg, Laurent, Sasha,
>
>On 12/12/2019 07:33, Greg Kroah-Hartman wrote:
>> 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)
>>>
>>> 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 ...
>>
>> Nothing should be in the kernel tree that is not already used by
>> something in that specific kernel tree.  We don't care about out-of-tree
>> code, and especially for stable kernel patches, it does not matter in
>> the least.
>
>So perhaps this patch should actually remove this macro rather than fix it?
>
>> If you have out-of-tree code, you are on your own here, sorry.
>>
>> So no, no backporting of stuff that no one actually uses in the codebase
>> itself.
>
>Ok understood, It was really the 'the macro exists in the kernel, but is
>wrong' part that got me.
>
>Along with the fact that we now have various automated machinery that
>would likely pick this patch up and backport it anyway?
>
>(Sasha, is that assumption accurate? Or would you/your system have
>identified that this macro is not used?)

No, it would have likely just backported it.

The right thing to do is to just remove the macro if no one is using it.

-- 
Thanks,
Sasha

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro
  2019-12-11 17:58       ` Laurent Pinchart
  2019-12-11 21:58         ` Kieran Bingham
@ 2020-02-12  0:43         ` Kuninori Morimoto
  1 sibling, 0 replies; 11+ messages in thread
From: Kuninori Morimoto @ 2020-02-12  0:43 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Kieran Bingham, Mauro Carvalho Chehab, linux-media,
	linux-renesas-soc, koji.matsuoka.xm, takeshi.kihara.df,
	harunobu.kurokawa.dn, khiem.nguyen.xt, hien.dang.eb


Hi Laurent

> > > 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.
> > 
> > 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
> tag. Kieran, would that be OK with you ?
> 
> > > 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.

My understanding is that this patch is applied to your branch,
but, is it correct ?
I still can't find it at linus/master and/or linux-next/master

Thank you for your help !!
Best regards
---
Kuninori Morimoto

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <redmine.issue-245033.20191211005426@dm.renesas.com>
     [not found] ` <redmine.issue-245033.20191211005426.161918957b73008d@dm.renesas.com>
2019-12-11  1:55   ` [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro Kuninori Morimoto
2019-12-11 12:59     ` Kieran Bingham
2019-12-11 17:58       ` Laurent Pinchart
2019-12-11 21:58         ` Kieran Bingham
2019-12-11 23:45           ` Laurent Pinchart
2019-12-12  7:33           ` Greg Kroah-Hartman
2019-12-13 11:55             ` Kieran Bingham
2019-12-13 13:22               ` Geert Uytterhoeven
2019-12-13 22:43                 ` Laurent Pinchart
2019-12-15 15:45               ` Sasha Levin
2020-02-12  0:43         ` Kuninori Morimoto

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org
	public-inbox-index linux-renesas-soc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git