by Andrey Smirnov

[permalink] [raw]

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

On Thu, Jun 6, 2019 at 3:34 AM Andrzej Hajda <[email protected]> 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

2019-06-06 21:56:36

by Andrey Smirnov

[permalink] [raw]

Subject: Re: [PATCH v3 06/15] drm/bridge: tc358767: Simplify AUX data read

On Thu, Jun 6, 2019 at 3:59 AM Andrzej Hajda <[email protected]> wrote:
>
> On 05.06.2019 09:04, Andrey Smirnov wrote:
> > Simplify AUX data read by removing index arithmetic and shifting with
> > a helper functions that does three things:
> >
> > 1. Fetch data from up to 4 32-bit registers from the chip
> > 2. Optionally fix data endianness (not needed on LE hosts)
> > 3. Copy read data into user provided array.
> >
> > Signed-off-by: Andrey Smirnov <[email protected]>
> > Cc: Archit Taneja <[email protected]>
> > Cc: Andrzej Hajda <[email protected]>
> > Cc: Laurent Pinchart <[email protected]>
> > Cc: Tomi Valkeinen <[email protected]>
> > Cc: Andrey Gusakov <[email protected]>
> > Cc: Philipp Zabel <[email protected]>
> > Cc: Cory Tusar <[email protected]>
> > Cc: Chris Healy <[email protected]>
> > Cc: Lucas Stach <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > drivers/gpu/drm/bridge/tc358767.c | 40 +++++++++++++++++++++----------
> > 1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index e197ce0fb166..da47d81e7109 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -321,6 +321,29 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
> > return 0;
> > }
> >
> > +static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
> > +{
> > + u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
> > + int ret, i, count = DIV_ROUND_UP(size, sizeof(u32));
> > +
> > + ret = regmap_bulk_read(tc->regmap, DP0_AUXRDATA(0), auxrdata, count);
> > + if (ret)
> > + return ret;
> > +
> > + for (i = 0; i < count; i++) {
> > + /*
> > + * Our regmap is configured as LE for register data,
> > + * so we need undo any byte swapping that might have
> > + * happened to preserve original byte order.
> > + */
> > + le32_to_cpus(&auxrdata[i]);
> > + }
> > +
> > + memcpy(data, auxrdata, size);
> > +
> > + return size;
> > +}
> > +
>
>
> Hmm, cannot we just use regmap_raw_read?

I'll give it a try in v4.

Thanks,
Andrey Smirnov