From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751944AbdAaMHO (ORCPT ); Tue, 31 Jan 2017 07:07:14 -0500 Received: from mailout4.w1.samsung.com ([210.118.77.14]:27532 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751870AbdAaMHD (ORCPT ); Tue, 31 Jan 2017 07:07:03 -0500 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 X-AuditID: cbfec7f1-f793f6d000007796-17-58907d82fba5 Subject: Re: [PATCH v8 2/3] drm/panel: Add support for S6E3HA2 panel driver on TM2 board To: Thierry Reding , Inki Dae Cc: Hoegeun Kwon , robh@kernel.org, Krzysztof Kozlowski , airlied@linux.ie, kgene@kernel.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org, cw00.choi@samsung.com, jh80.chung@samsung.com, andi.shyti@samsung.com, Donghwa Lee , Hyungwon Hwang From: Andrzej Hajda Message-id: Date: Tue, 31 Jan 2017 13:05:20 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 In-reply-to: <20170131085449.GA19348@ulmo.ba.sec> Content-transfer-encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA02Se0hTcRTH+e0+Ha1u0/JgljQKysg0LC5WphGxP4U0RYIaelHTqWxqKmlR YTrLxzRaGqWWTwhjic5HZVupKbnESb7SwlDxka9pKmjtehX873N+53zP95zDj8akJsKJjoiO 41TRiigZKcZrW1bMx++m5AS593W7so86v4jYuk+jBPvdOkawLz512sjszVoWZ0h2plxDsqW6 LILVDufgbO9qGsFmj0xirNn8hmK7G56RrM78XsT+azJQ7ErDc9yHkdcX/KDk+qoMUl639JOQ D2e2iuRZNVVIvqA/4EcGi8+GclERCZzqhPd1cXhh+28sduFQYlOxlbiDNM4aZEcD4wmlA39F Au+Fb0PVpAaJaSlTiiB/uWkzWEDwNdci2lI8MJUSPEuZMgSN2ks8S5jdsJw3hPOMMUdh3KrF BfEogs9aLeIT9kww5BUvbYgdGD8oLlnE+CKMeYVBpbWL4hOkTb32ts9mTdu6esP4Bwn/jDOH obK8ZGOIPUwQGFsGMJ7tGA8o0yyQgrELfLSMbRgDs0KB0TSJ832A2Q/6ZkxY4CJYph9usj1M tNZQAjtDd17mpjYTwXx2GyUE+QjWZ3WbijNgau0iBLedoK19ggkGEkhPkwolcljV9SKBfcH6 7j4lXMIigorBfjIHuRRsu1jBtosVbFuiCGFVyIGLVyvDOPVJN7VCqY6PDnMLiVHqke1rday3 zhnQTJuXETE0ku2QHEnLDpISigR1ktKIgMZkDpK+xJwgqSRUkZTMqWKuqeKjOLUR7aNxmaOk qcgSKGXCFHFcJMfFcqqtrIi2c7qDIu5GxB4LGHG91p8xeCN3bezcVd8G0Whdbw2d/HktfT6k Z84YEvOg2r0n0nDhdX3sqRFaH6fROl8OzLcUpk/4D+pUYbqOXz4Hd6WSjqbTqop70zdL2sVa g+zlrR7/82bHuiMlwakpj33prituvbenQn/Mpv552uwZQDWmZRROeUlluDpc4eGKqdSK/21M /AhWAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrAIsWRmVeSWpSXmKPExsVy+t/xK7oNtRMiDNavtLHoPXeSyWL7kWes Fte/PGe1mH/kHJB13s7iytf3bBbvl3exWSyd0cdqMen+BBaLG7/aWC36H79mtjh/fgO7xeVd c9gsZpzfx2Txf88Odoufu+axOAh47Jx1l91j06pONo/t3x6wetzvPs7k0bdlFaPH501yAWxR bjYZqYkpqUUKqXnJ+SmZeem2SqEhbroWSgp5ibmptkoRur4hQUoKZYk5pUCekQEacHAOcA9W 0rdLcMuYfeoJc8FnlYo9C7+wNjB2yXQxcnJICJhItB9eygphi0lcuLeerYuRi0NIYAmjxNKf vewgCV4BQYkfk++xdDFycDALqEtMmZILUfOMUeJf2zUmkBphgSiJc9Pfgg0SEfCTuDR3BitE 0RUmiRlzZ4JNZRZYwizxasNcZpAqNgFNib+bb7KBTOUVsJN4sZ8XJMwioCqxcvkiJpCwqECE RMPhdJAwp4ChxLKuz2wgNrOAvMTBK89ZJjAKzEJy3iyE82YhqVrAyLyKUSS1tDg3PbfYSK84 Mbe4NC9dLzk/dxMjMHa3Hfu5ZQdj17vgQ4wCHIxKPLwabf0RQqyJZcWVuYcYJTiYlUR4b1ZM iBDiTUmsrEotyo8vKs1JLT7EaAp06kRmKdHkfGBaySuJNzQxNLc0NDK2sDA3MlIS55364Uq4 kEB6YklqdmpqQWoRTB8TB6dUA2NPsKhAlodBcrMyY0SdzLclNrwH/TL/V97LfDH7RmzvwoUi 82+9CHfl8y//0cB4587WXylfxJad+fL0ffG0mO+BdRM+y19TvX3GpydjpV/WMnUhxxap9T7L Y4TY5vcum/fT6uPNiqU8FgtUj3UwSG2v7RWf+3Czcv69bJu/BuJzF+y96KvBfkGJpTgj0VCL uag4EQAt9QOq8wIAAA== X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170131120522eucas1p265317fca6503d507bdc89bbca235eba5 X-Msg-Generator: CA X-Sender-IP: 182.198.249.180 X-Local-Sender: =?UTF-8?B?QW5kcnplaiBIYWpkYRtTUlBPTC1LZXJuZWwgKFRQKRvsgrw=?= =?UTF-8?B?7ISx7KCE7J6QG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Global-Sender: =?UTF-8?B?QW5kcnplaiBIYWpkYRtTUlBPTC1LZXJuZWwgKFRQKRtTYW1z?= =?UTF-8?B?dW5nIEVsZWN0cm9uaWNzG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjczOTI=?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20170111063408epcas5p2e6ec091549c3ffed8462fd95d11d3c82 X-RootMTR: 20170111063408epcas5p2e6ec091549c3ffed8462fd95d11d3c82 References: <1484116439-7275-1-git-send-email-hoegeun.kwon@samsung.com> <1484116439-7275-3-git-send-email-hoegeun.kwon@samsung.com> <08c5d94b-c76f-af14-c08f-478e26a34a7c@samsung.com> <588FD3C3.7080508@samsung.com> <20170131085449.GA19348@ulmo.ba.sec> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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