Re: [PATCH v2] sc16is7xx: spi interface is added

["Jakub Kiciński" <moorray3@wp.pl>]

On Wed, 13 May 2015 16:27:58 +0530, ram.i hcltech wrote:
> spi interface for sc16is7xx is added along with Kconfig flag
> to enable spi or i2c, thus in a instance we can have either
> spi or i2c or both, in sync to the hw.
> 
> Signed-off-by: ram.i hcltech <indrakanti_ram@hotmail.com>
> ---
> 
> Changes in v2:
>  -Added seprate flags for i2c and spi
>  -Added space in the comments lines
>  -Added MODULE_ALIAS for spi interface
> ---
>  drivers/tty/serial/Kconfig     | 27 +++++++++++++++--
>  drivers/tty/serial/sc16is7xx.c | 69 +++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 92 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index f8120c1..8c505b2 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1181,13 +1181,34 @@ config SERIAL_SCCNXP_CONSOLE
>  
>  config SERIAL_SC16IS7XX
>  	tristate "SC16IS7xx serial support"
> -	depends on I2C

Please keep the dependency like this:
depends on I2C || SPI_MASTER

(or SPI, I don't know what's the difference. SPI seems fine.)

>  	select SERIAL_CORE
> -	select REGMAP_I2C if I2C
>  	help
>  	  This selects support for SC16IS7xx serial ports.
>  	  Supported ICs are SC16IS740, SC16IS741, SC16IS750, SC16IS752,
> -	  SC16IS760 and SC16IS762.
> +	  SC16IS760 and SC16IS762, over i2c or spi.
> +	  select at least one of the i2c or spi interface.

I would phrase the help message like this:

This selects support for SC16IS7xx serial ports.
Supported ICs are SC16IS740, SC16IS741, SC16IS750, SC16IS752,
SC16IS760 and SC16IS762. Select supported buses using options below.

> +config SERIAL_SC16IS7XX_I2C
> +	bool "SC16IS7xx for I2C interface"

Please add "default y" to minimize oldconfig pain for those already
using the driver.

> +	depends on SERIAL_SC16IS7XX=y

Why =y?

> +	depends on I2C
> +	select REGMAP_I2C if I2C
> +	help
> +	  to enable i2c interface for SC16IS7XX, say Y,
> +	  Otherwise, for i2c say N.
> +	  this would make the driver to interface over SPI and I2C would
> +	  be diabled.

I would phrase it simply like this:

Enable SC16IS7xx driver on I2C bus.

> +config SERIAL_SC16IS7XX_SPI
> +	bool "SC16IS7xx for spi interface"
> +	depends on SERIAL_SC16IS7XX
> +	depends on SPI_MASTER

Right now it is possible to select the driver without any bus-specific
option being set.  I don't see an easy way to avoid this but please
make sure that there are no build failures/warnings in this scenario.

You should also extend the binding information to include the new SPI
interface (Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt)


Thanks!