linux-kernel - Re: [PATCH 2/9] drm/bridge: tc358767: Simplify tc_stream_clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190304122021.GD6325@pendragon.ideasonboard.com>
Date:   Mon, 4 Mar 2019 14:20:21 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Andrzej Hajda <a.hajda@...sung.com>
Cc:     Andrey Smirnov <andrew.smirnov@...il.com>,
        dri-devel@...ts.freedesktop.org,
        Archit Taneja <architt@...eaurora.org>,
        Chris Healy <cphealy@...il.com>,
        Lucas Stach <l.stach@...gutronix.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/9] drm/bridge: tc358767: Simplify tc_stream_clock_calc()

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@...il.com>
> > Cc: Archit Taneja <architt@...eaurora.org>
> > Cc: Andrzej Hajda <a.hajda@...sung.com>
> > Cc: Laurent Pinchart <Laurent.pinchart@...asonboard.com>
> > Cc: Chris Healy <cphealy@...il.com>
> > Cc: Lucas Stach <l.stach@...gutronix.de>
> > Cc: dri-devel@...ts.freedesktop.org
> > Cc: linux-kernel@...r.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)

-- 
Regards,

Laurent Pinchart