Re: [PATCH v3 05/15] drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors

[Andrey Smirnov]

On Thu, Jun 6, 2019 at 3:34 AM Andrzej Hajda <a.hajda@xxxxxxxxxxx> wrote:
>
> On 05.06.2019 09:04, Andrey Smirnov wrote:
> > A very unfortunate aspect of tc_write()/tc_read() macro helpers is
> > that they capture quite a bit of context around them and thus require
> > the caller to have magic variables 'ret' and 'tc' as well as label
> > 'err'. That makes a number of code paths rather counterintuitive and
> > somewhat clunky, for example tc_stream_clock_calc() ends up being like
> > this:
> >
> >       int ret;
> >
> >       tc_write(DP0_VIDMNGEN1, 32768);
> >
> >       return 0;
> > err:
> >       return ret;
> >
> > which is rather surprising when you read the code for the first
> > time. Since those helpers arguably aren't really saving that much code
> > and there's no way of fixing them without making them too verbose to
> > be worth it change the driver code to not use them at all.
>
>
> On the other side, error checking after every registry access is very
> annoying and significantly augments the code, makes it redundant and
> less readable. To avoid it one can cache error state, and do not perform
> real work until the error is clear. For example with following accessor:
>
> void tc_write(struct tc_data *tc, u32 reg, u32 val){
>
>     int ret;
>
>     if (tc->error) //This check is IMPORTANT
>
>         return;
>
>     ret =regmap_write(...);
>
>     if (ret >= 0)
>
>         return;
>
>     tc->error = ret;
>
>     dev_err(tc->dev, "Error writing register %#x\n", reg);
>
> }
>
> You can safely write code like:
>
>     tc_write(tc, DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);
>
>     tc_write(tc, DP0_PLLCTRL, PLLUPDATE | PLLEN);
>
>     tc_write(tc, DP1_PLLCTRL, PLLUPDATE | PLLEN);
>
>     if (tc->error) {
>
>         tc->error = 0;
>
>         goto err;
>
>     }
>
> This is of course loose suggestion.
>

I am going to have to disagree with you on this one, unfortunately.
Using regmap API explicitly definitely makes code more verbose, less
readable or more annoying though? Not really from my perspective. With
regmap code I know what the code is doing the moment I look at it,
with the example above, not so much. I also find it annoying that I
now have to remember the tricks that tc_write is pulling internally as
well as be mindful of a global-ish error state object. My problem with
original code was that a) it traded explicitness for conciseness in a
an unfavorable way, which I still think is true for code above b) it
didn't provide a comprehensive abstraction completely removing regmap
API and still relied on things like regmap_update_bits() explicitly,
making the code even more confusing (true for above example as well).
I think this driver isn't big enough to have a dedicated person always
working on it and it will mostly see occasional commits from somewhat
random folks who are coming to the codebase fresh, so creating as
little "institutional knowledge", so to speak, in a form of a custom
exception-like mechanism and opting for explicit but verbose code
seems like a preferable choice.

Anyway, I get it that's it is a loose suggestion :-), just wanted to
provide a detailed explanation why I'd rather not go that way.

Thanks,
Andrey Smirnov