Sorry for late response. On Sun, 04 Oct 2015 22:30:31 +0100, Vladimir Zapolskiy wrote: > 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. Yes, the purpose of this rule is to make sure that people don't sprinkle the logic with #ifdef/#endif. Using it for the boilerplate code is perfectly fine and common. > >> 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 Sorry, but I don't see any "mess" in the 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. Sorry, I should be more explicit. People care about the code size because the target devices may have limited resources. What I ment here is that the config does not have to be installed on the target (if one unselects IKCONFIG) so it's size is not an issue for anyone. > >> 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? I don't like where this is going. $ grep -nrI "depends on" . | grep SPI | grep I2C | grep '||' | wc -l 16 > > 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. I didn't see this as a problem when Rama was developing this code, but I agree that perhaps we don't need separate options for SPI and I2C and we can just depend directly on CONFIG_I2C and CONFIG_SPI_MASTER being set, like iio/dac/ad5380.c does. Looking forward to a patch which does that. Please make sure that the change does not break oldconfig for people with different SC16IS7xx selections, though. Thanks! -- 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
