diff mbox series

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

Message ID 20190226193609.9862-3-andrew.smirnov@gmail.com (mailing list archive)
State New, archived
Headers show
Series tc358767 driver improvements | expand

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
diff mbox series

Patch

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)