[2/9] drm/bridge: tc358767: Simplify tc_stream_clock_calc()
diff mbox series

Message ID 20190226193609.9862-3-andrew.smirnov@gmail.com
State New
Headers show
Series
  • tc358767 driver improvements
Related show

Commit Message

Andrey Smirnov Feb. 26, 2019, 7:36 p.m. UTC
Drop the use of tc_write() as well as "magicly" used "ret" and "err:"
and replace it with a simple call to regmap_write(). No functional
change intended.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Andrzej Hajda March 4, 2019, 9:42 a.m. UTC | #1
On 26.02.2019 20:36, Andrey Smirnov wrote:
> Drop the use of tc_write() as well as "magicly" used "ret" and "err:"
> and replace it with a simple call to regmap_write(). No functional
> change intended.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index b0f8264a1285..86ebd49194b7 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -504,7 +504,6 @@ static int tc_pxl_pll_dis(struct tc_data *tc)
>  
>  static int tc_stream_clock_calc(struct tc_data *tc)
>  {
> -	int ret;
>  	/*
>  	 * If the Stream clock and Link Symbol clock are
>  	 * asynchronous with each other, the value of M changes over
> @@ -520,11 +519,7 @@ static int tc_stream_clock_calc(struct tc_data *tc)
>  	 * M/N = f_STRMCLK / f_LSCLK
>  	 *
>  	 */
> -	tc_write(DP0_VIDMNGEN1, 32768);
> -
> -	return 0;
> -err:
> -	return ret;
> +	return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);


The patch looks semantically correct, but you are dropping here custom
accessor (tc_write) in favor of regmap API.

I think the best would be consistent across the whole driver: either use
only  accessors, either drop them entirely and use regmap API.

And it would be good to have comment of the original authors about this
change.


Regards

Andrzej


>  }
>  
>  static int tc_aux_link_setup(struct tc_data *tc)
Laurent Pinchart March 4, 2019, 12:20 p.m. UTC | #2
Hello,

On Mon, Mar 04, 2019 at 10:42:20AM +0100, Andrzej Hajda wrote:
> On 26.02.2019 20:36, Andrey Smirnov wrote:
> > Drop the use of tc_write() as well as "magicly" used "ret" and "err:"
> > and replace it with a simple call to regmap_write(). No functional
> > change intended.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Chris Healy <cphealy@gmail.com>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  drivers/gpu/drm/bridge/tc358767.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index b0f8264a1285..86ebd49194b7 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -504,7 +504,6 @@ static int tc_pxl_pll_dis(struct tc_data *tc)
> >  
> >  static int tc_stream_clock_calc(struct tc_data *tc)
> >  {
> > -	int ret;
> >  	/*
> >  	 * If the Stream clock and Link Symbol clock are
> >  	 * asynchronous with each other, the value of M changes over
> > @@ -520,11 +519,7 @@ static int tc_stream_clock_calc(struct tc_data *tc)
> >  	 * M/N = f_STRMCLK / f_LSCLK
> >  	 *
> >  	 */
> > -	tc_write(DP0_VIDMNGEN1, 32768);
> > -
> > -	return 0;
> > -err:
> > -	return ret;
> > +	return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
> 
> 
> The patch looks semantically correct, but you are dropping here custom
> accessor (tc_write) in favor of regmap API.
> 
> I think the best would be consistent across the whole driver: either use
> only  accessors, either drop them entirely and use regmap API.

I agree with this. The tc_write macro with its goto err is pretty nasty,
and I'd like to see it removed from the whole driver, or at least
replaced with an accessor that wouldn't hide the goto.

> And it would be good to have comment of the original authors about this
> change.
> 
> >  }
> >  
> >  static int tc_aux_link_setup(struct tc_data *tc)
Andrey Smirnov March 11, 2019, 5:51 p.m. UTC | #3
On Mon, Mar 4, 2019 at 4:20 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> On Mon, Mar 04, 2019 at 10:42:20AM +0100, Andrzej Hajda wrote:
> > On 26.02.2019 20:36, Andrey Smirnov wrote:
> > > Drop the use of tc_write() as well as "magicly" used "ret" and "err:"
> > > and replace it with a simple call to regmap_write(). No functional
> > > change intended.
> > >
> > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > > Cc: Archit Taneja <architt@codeaurora.org>
> > > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > > Cc: Chris Healy <cphealy@gmail.com>
> > > Cc: Lucas Stach <l.stach@pengutronix.de>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: linux-kernel@vger.kernel.org
> > > ---
> > >  drivers/gpu/drm/bridge/tc358767.c | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > > index b0f8264a1285..86ebd49194b7 100644
> > > --- a/drivers/gpu/drm/bridge/tc358767.c
> > > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > > @@ -504,7 +504,6 @@ static int tc_pxl_pll_dis(struct tc_data *tc)
> > >
> > >  static int tc_stream_clock_calc(struct tc_data *tc)
> > >  {
> > > -   int ret;
> > >     /*
> > >      * If the Stream clock and Link Symbol clock are
> > >      * asynchronous with each other, the value of M changes over
> > > @@ -520,11 +519,7 @@ static int tc_stream_clock_calc(struct tc_data *tc)
> > >      * M/N = f_STRMCLK / f_LSCLK
> > >      *
> > >      */
> > > -   tc_write(DP0_VIDMNGEN1, 32768);
> > > -
> > > -   return 0;
> > > -err:
> > > -   return ret;
> > > +   return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
> >
> >
> > The patch looks semantically correct, but you are dropping here custom
> > accessor (tc_write) in favor of regmap API.
> >
> > I think the best would be consistent across the whole driver: either use
> > only  accessors, either drop them entirely and use regmap API.
>
> I agree with this. The tc_write macro with its goto err is pretty nasty,
> and I'd like to see it removed from the whole driver, or at least
> replaced with an accessor that wouldn't hide the goto.
>
> > And it would be good to have comment of the original authors about this
> > change.
> >

OK, I'll create a patch to remove tc_write/read in v2 and add original
authors to CC.

Thanks,
Andrey Smirnov

Patch
diff mbox series

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index b0f8264a1285..86ebd49194b7 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -504,7 +504,6 @@  static int tc_pxl_pll_dis(struct tc_data *tc)
 
 static int tc_stream_clock_calc(struct tc_data *tc)
 {
-	int ret;
 	/*
 	 * If the Stream clock and Link Symbol clock are
 	 * asynchronous with each other, the value of M changes over
@@ -520,11 +519,7 @@  static int tc_stream_clock_calc(struct tc_data *tc)
 	 * M/N = f_STRMCLK / f_LSCLK
 	 *
 	 */
-	tc_write(DP0_VIDMNGEN1, 32768);
-
-	return 0;
-err:
-	return ret;
+	return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
 }
 
 static int tc_aux_link_setup(struct tc_data *tc)