Hello Jakub, On 03.10.2015 19:20, Jakub Kicinski wrote: > Sorry for late response and thanks for CC-ing me! > > On Thu, 1 Oct 2015 00:02:42 +0300, Vladimir Zapolskiy wrote: >> The change removes SERIAL_SC16IS7XX_CORE, SERIAL_SC16IS7XX_I2C and >> SERIAL_SC16IS7XX_SPI kernel configuration options by merging code protected >> by SERIAL_SC16IS7XX_I2C and SERIAL_SC16IS7XX_SPI, also it noticeably >> simplifies driver code. > > How does it simplify the code? You patch merely removes 6 ifdef/endifs > which are all on registrestion/boilerplate code. I hope you are aware of Documentation/CodingStyle, that's what it states: Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c files; doing so makes code harder to read and logic harder to follow. This case clearly follows the accepted coding style, here it is possible to avoid 6 pairs of ifdef/endifs for a little price. >> Statistics for armv5 compilation, stripped and unstripped module sizes: >> >> | | unstripped | stripped | diff, bytes | diff, % | >> | only SPI enabled | 166496 | 16204 | 1724 | +10.6% | >> | only I2C enabled | 164832 | 16020 | 1908 | +11.9% | >> | SPI and I2C enabled | 172684 | 17928 | 0 | 0 | > > The main purpose of having the options separately is to allow people to > build the driver without SPI or I2C core built. and the main purpose of having one kconfig option is to avoid unwanted mess in drivers/tty/serial/Kconfig >> Hopefully partially the increased module size is compensated by >> reduced size of config.gz, which at the moment may contain something like >> >> CONFIG_SERIAL_SC16IS7XX_CORE=m >> CONFIG_SERIAL_SC16IS7XX=m >> CONFIG_SERIAL_SC16IS7XX_I2C=y >> CONFIG_SERIAL_SC16IS7XX_SPI=y > > Nobody cares about the size of the config. Sorry, I take it as your personal opinion. >> There are other device drivers, which describe devices with both >> I2C and SPI interfaces, all of them do not provide separate compilation >> options for I2C and/or SPI cases, the drivers are: >> >> iio/dac/ad5446.c >> iio/dac/ad5064.c >> iio/dac/ad5380.c >> gpio/gpio-mcp23s08.c > > There is even more drivers which do provide the choice. so, it won't be a problem for you to give several examples of other single I2C/SPI device drivers, which do provide a similar build time configuration, could you please do you it for me? > Can you please tell us what is your real motivation behind this patch? Reduce overbloated kernel configuration. I don't think that a driver, which does not have even a single user in vanilla deserves four Kconfig build options. > Did you experience any problems with the config options of the driver? > Are the options hard to understand? I appreciate that your patches are > properly formated and follow the kernel guidelines but please make your > motivation more clear. > NAK I would appreciate to get here a maintainer's opinion as well. Thanks. -- With best wishes, Vladimir -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
