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 <[email protected]>
> ---
>
> 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!
> 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 <[email protected]>
>> ---
>>
>> 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.)
>
The depends on is pushed to the interface selection... i think that would be
better as it be collated accordingly.
>> 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.
>
Yeah a better one..will take it...
>> +config SERIAL_SC16IS7XX_I2C
>> + bool "SC16IS7xx for I2C interface"
>
> Please add "default y" to minimize oldconfig pain for those already
> using the driver.
>
yes, missed this.
>> + 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.
short description for the flags fails in checkpatch.pl and hence the text..
>
>> +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.
>
Yes, that being the reason, and for the driver to work, there should be any one the
interfaces to be enabled by default.
> You should also extend the binding information to include the new SPI
> interface (Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt)
>
Yes, this needs to be updated, i think that shall be a separate patch...
>
> Thanks!
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-
> 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 <[email protected]>
>> ---
>>
>> 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
>>
To avoid error or warning on build, i think this would be the probable solution.
I thinking to go with this, any comments on this please.
config SERIAL_SC16IS7XX
??? bool
config SERIAL_SC16IS7XX_SELECT
??? tristate "SC16IS7xx serial support"
??? select SERIAL_CORE
??? depends on I2C || SPI_MASTER
??? select REGMAP_I2C if I2C
??? select REGMAP_SPI if SPI_MASTER
??? help
??? ? 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"
??? depends on SERIAL_SC16IS7XX_SELECT
??? select SERIAL_SC16IS7XX
??? default y
??? help
??? ? Enable SC16IS7xx driver on I2C bus.
config SERIAL_SC16IS7XX_SPI
??? bool "SC16IS7xx for spi interface"
??? depends on SERIAL_SC16IS7XX_SELECT
??? select SERIAL_SC16IS7XX
??? help
??? ? Enable SC16IS7xx driver on SPI bus.
>> 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!
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-
On Thu, 14 May 2015 13:16:20 +0530, ram kiran wrote:
> > 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 <[email protected]>
> >> ---
> >>
> >> 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
> >>
> To avoid error or warning on build, i think this would be the probable solution.
> I thinking to go with this, any comments on this please.
>
> config SERIAL_SC16IS7XX
> bool
>
> config SERIAL_SC16IS7XX_SELECT
> tristate "SC16IS7xx serial support"
> select SERIAL_CORE
> depends on I2C || SPI_MASTER
> select REGMAP_I2C if I2C
> select REGMAP_SPI if SPI_MASTER
> help
> 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"
> depends on SERIAL_SC16IS7XX_SELECT
> select SERIAL_SC16IS7XX
> default y
> help
> Enable SC16IS7xx driver on I2C bus.
>
> config SERIAL_SC16IS7XX_SPI
> bool "SC16IS7xx for spi interface"
> depends on SERIAL_SC16IS7XX_SELECT
> select SERIAL_SC16IS7XX
> help
> Enable SC16IS7xx driver on SPI bus.
>
This looks quite elegant! Should we aslo make SERIAL_SC16IS7XX depend
on SERIAL_SC16IS7XX_I2C || SERIAL_SC16IS7XX_SPI? Would that work?
I know little about kbuild but I'm worried that someone doing oldconfig
can still get SERIAL_SC16IS7XX selected while saying no to all the
others.
Other option would be to swap the names between SERIAL_SC16IS7XX and
SERIAL_SC16IS7XX_SELECT, oldconfig would run smoother.
Thanks!
> On Thu, 14 May 2015 13:16:20 +0530, ram kiran wrote:
>>> 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 <[email protected]>
>>>> ---
>>>>
>>>> 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
>>>>
>> To avoid error or warning on build, i think this would be the probable solution.
>> I thinking to go with this, any comments on this please.
>>
>> config SERIAL_SC16IS7XX
>> bool
>>
>> config SERIAL_SC16IS7XX_SELECT
>> tristate "SC16IS7xx serial support"
>> select SERIAL_CORE
>> depends on I2C || SPI_MASTER
>> select REGMAP_I2C if I2C
>> select REGMAP_SPI if SPI_MASTER
>> help
>> 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"
>> depends on SERIAL_SC16IS7XX_SELECT
>> select SERIAL_SC16IS7XX
>> default y
>> help
>> Enable SC16IS7xx driver on I2C bus.
>>
>> config SERIAL_SC16IS7XX_SPI
>> bool "SC16IS7xx for spi interface"
>> depends on SERIAL_SC16IS7XX_SELECT
>> select SERIAL_SC16IS7XX
>> help
>> Enable SC16IS7xx driver on SPI bus.
>>
>
> This looks quite elegant! Should we aslo make SERIAL_SC16IS7XX depend
> on SERIAL_SC16IS7XX_I2C || SERIAL_SC16IS7XX_SPI? Would that work?
>
This would be additional protection, need to check if that is too much to do or would be good to go.
> I know little about kbuild but I'm worried that someone doing oldconfig
> can still get SERIAL_SC16IS7XX selected while saying no to all the
> others.
>
> Other option would be to swap the names between SERIAL_SC16IS7XX and
> SERIAL_SC16IS7XX_SELECT, oldconfig would run smoother.
I think, with the above, there would need a configuration change for sure.
It should be okay, as I2C is default Y.
Swap names would need Makefile changes, i was just thinking to avoid this.
obj-$(CONFIG_SERIAL_SC16IS7XX) += sc16is7xx.o
would be
obj-$(CONFIG_SERIAL_SC16IS7XX_SELECT) += sc16is7xx.o
I think its some that need not be there. Do suggest..
Thanks
>
> Thanks!
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-
On Thu, 14 May 2015 14:45:47 +0530, ram kiran wrote:
> > I know little about kbuild but I'm worried that someone doing oldconfig
> > can still get SERIAL_SC16IS7XX selected while saying no to all the
> > others.
> >
> > Other option would be to swap the names between SERIAL_SC16IS7XX and
> > SERIAL_SC16IS7XX_SELECT, oldconfig would run smoother.
> I think, with the above, there would need a configuration change for sure.
> It should be okay, as I2C is default Y.
Exactly, but with what you proposed we need a configuration change as
well, no? SERIAL_SC16IS7XX_SELECT is new so users would have to know
that it's what SERIAL_SC16IS7XX used to be.
> Swap names would need Makefile changes, i was just thinking to avoid this.
> obj-$(CONFIG_SERIAL_SC16IS7XX) += sc16is7xx.o
> would be
> obj-$(CONFIG_SERIAL_SC16IS7XX_SELECT) += sc16is7xx.o
>
> I think its some that need not be there. Do suggest..
Perhaps *_SELECT is not the best name then but we could use something
like *_CORE or *_BASE. Changes to the Makefile are not user-visible so
no worries. It would be nice if people who run oldconfig by default got
the same behaviour as they did so far (i2c if SC16IS7XX was enabled in
previous config). I think with names swapped and modification of
Makefile we would get exactly that.
On Thu, 14 May 2015 14:45:47 +0530, ram kiran wrote:
> > I know little about kbuild but I'm worried that someone doing oldconfig
> > can still get SERIAL_SC16IS7XX selected while saying no to all the
> > others.
> >
> > Other option would be to swap the names between SERIAL_SC16IS7XX and
> > SERIAL_SC16IS7XX_SELECT, oldconfig would run smoother.
> I think, with the above, there would need a configuration change for sure.
> It should be okay, as I2C is default Y.
Exactly, but with what you proposed we need a configuration change as
well, no? SERIAL_SC16IS7XX_SELECT is new so users would have to know
that it's what SERIAL_SC16IS7XX used to be.
> Swap names would need Makefile changes, i was just thinking to avoid this.
> obj-$(CONFIG_SERIAL_SC16IS7XX) += sc16is7xx.o
> would be
> obj-$(CONFIG_SERIAL_SC16IS7XX_SELECT) += sc16is7xx.o
>
> I think its some that need not be there. Do suggest..
Perhaps *_SELECT is not the best name then but we could use something
like *_CORE or *_BASE. Changes to the Makefile are not user-visible so
no worries. It would be nice if people who run oldconfig by default got
the same behaviour as they did so far (i2c if SC16IS7XX was enabled in
previous config). I think with names swapped and modification of
Makefile we would get exactly that.