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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
