Message-id: <e72546f1-bd3c-ac0b-9e94-352942293ff9@samsung.com>
Date:   Tue, 31 Jan 2017 13:05:20 +0100
From:   Andrzej Hajda <a.hajda@...sung.com>
To:     Thierry Reding <thierry.reding@...il.com>,
        Inki Dae <inki.dae@...sung.com>
Cc:     Hoegeun Kwon <hoegeun.kwon@...sung.com>, robh@...nel.org,
        Krzysztof Kozlowski <krzk@...nel.org>, airlied@...ux.ie,
        kgene@...nel.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-samsung-soc@...r.kernel.org, cw00.choi@...sung.com,
        jh80.chung@...sung.com, andi.shyti@...sung.com,
        Donghwa Lee <dh09.lee@...sung.com>,
        Hyungwon Hwang <human.hwang@...sung.com>
Subject: Re: [PATCH v8 2/3] drm/panel: Add support for S6E3HA2 panel driver on
 TM2 board

On 31.01.2017 09:54, Thierry Reding wrote:
> On Tue, Jan 31, 2017 at 09:01:07AM +0900, Inki Dae wrote:
>>
>> 2017년 01월 24일 10:50에 Hoegeun Kwon 이(가) 쓴 글:
>>> Dear Thierry,
>>>
>>> Could you please review this patch?
>> Thierry, I think this patch has been reviewed enough but no comment
>> from you. Seems you are busy. I will pick up this.
> Sorry, but that's not how it works. This patch has gone through 8
> revisions within 4 weeks, and I tend to ignore patches like that until
> the dust settles.
>
> Other than that, this continues the same madness that I've repeatedly
> complained about in the past. The whole mechanism of running through a
> series of writes and not caring about errors until the very end is
> something we've discussed at length in the past. 

Yes, we have discussed the pattern, but without any conclusion. The
pattern is correct, used in different places in kernel (see below for
examples) and significantly decreases source code size. Disallowing it
in panels subsystem just because of personal preferences of the
maintainer does not seem to be proper.

> It also in large parts
> duplicates a bunch of functions from other Samsung panel drivers that I
> already said should eventually be moved to something saner.

Currently there are two Samsung panel drivers, one is on SPI bus,
another one is on DSI.
I am (co-)author of both drivers, they have some similarities but I did
not see any gain in making additional abstractions over transport layer
just to make one or two small functions common.
Could you be more precise what are you talking about, or could you give
a link to the mail where you said it. Anything I remember was a
discussion about ugly magic initialization sequences due to poor
documentation.

And below promised examples of the error pattern, it was time consuming
to find them, so please at least read them :)

Almost exactly the same patterns for the same purpose:

1. http://lxr.free-electrons.com/source/drivers/net/ieee802154/atusb.c#L63
    Citation from the code:
    To reduce the number of error checks in the code, we record the
first error
    in atusb->err and reject all subsequent requests until the error is
cleared.

2. http://lxr.free-electrons.com/source/drivers/md/dm-thin.c?v=4.4#L917
3. http://lxr.free-electrons.com/source/net/9p/trans_fd.c?v=3.18#L297
4.
http://lxr.free-electrons.com/source/drivers/media/v4l2-core/v4l2-ctrls.c#L2234
5. http://lxr.free-electrons.com/source/drivers/media/i2c/s5k5baf.c#L451
    This is my driver, I mention it here just to show it was not a
problem to merge it to media subsystem.

Similar patterns:

6. http://lxr.free-electrons.com/source/fs/seq_file.c#L398
    Do not process object if buffer is full, it allows to do not check
buffer size after every write, for example:
>     seq_printf(m, " hardirq-safe locks:            %11lu\n",
>             nr_hardirq_safe);
>     seq_printf(m, " hardirq-unsafe locks:          %11lu\n",
>             nr_hardirq_unsafe);
>     seq_printf(m, " softirq-safe locks:            %11lu\n",
>             nr_softirq_safe);
>     seq_printf(m, " softirq-unsafe locks:          %11lu\n",
>             nr_softirq_unsafe);
>     seq_printf(m, " irq-safe locks:                %11lu\n",
>             nr_irq_safe);
>     seq_printf(m, " irq-unsafe locks:              %11lu\n",
>             nr_irq_unsafe);
>
>     seq_printf(m, " hardirq-read-safe locks:       %11lu\n",
>             nr_hardirq_read_safe);
>     seq_printf(m, " hardirq-read-unsafe locks:     %11lu\n",
>             nr_hardirq_read_unsafe);
>     seq_printf(m, " softirq-read-safe locks:       %11lu\n",
>             nr_softirq_read_safe);
>     seq_printf(m, " softirq-read-unsafe locks:     %11lu\n",
>             nr_softirq_read_unsafe);
>     seq_printf(m, " irq-read-safe locks:           %11lu\n",
>             nr_irq_read_safe);
>     seq_printf(m, " irq-read-unsafe locks:         %11lu\n",
>             nr_irq_read_unsafe);
>
>     seq_printf(m, " uncategorized locks:           %11lu\n",
>             nr_uncategorized);
>     seq_printf(m, " unused locks:                  %11lu\n",
>             nr_unused);
>     seq_printf(m, " max locking depth:             %11u\n",
>             max_lockdep_depth);

Now try to imagine how it would look like if you add error checking
after each call.

7. http://lxr.free-electrons.com/source/lib/devres.c#L129
    Postponed error check:
> */res = platform_get_resource(pdev, IORESOURCE_MEM, 0);/*
> /*b*/*/ase = devm_ioremap_resource(&pdev->dev, res);/*
> */if (IS_ERR(base))/*
> /**/*/return PTR_ERR(base);/*


Regards
Andrzej

Powered by blists - more mailing lists