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