* [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"
@ 2023-05-15 16:02 Hugo Villeneuve
2023-05-15 16:20 ` Greg Kroah-Hartman
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Hugo Villeneuve @ 2023-05-15 16:02 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Lech Perczak, Tomasz Moń
Cc: hugo, Hugo Villeneuve, linux-serial, linux-kernel
From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
This reverts commit 679875d1d8802669590ef4d69b0e7d13207ebd61.
Because of this commit, it is no longer possible to use the 16 GPIO
lines as dedicated GPIOs on the SC16IS752.
Reverting it makes it work again.
The log message of the original commit states:
"Export only the GPIOs that are not shared with hardware modem
control lines"
But there is no explanation as to why this decision was taken to
permanently set the function of the GPIO lines as modem control
lines. AFAIK, there is no problem with using these lines as GPIO or modem
control lines.
Maybe after reverting this commit, we could define a new
device-tree property named, for example,
"use-modem-control-lines", so that both options can be supported.
Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
lines")
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
drivers/tty/serial/sc16is7xx.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 5bd98e4316f5..25f1b2f6ec51 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -306,7 +306,6 @@ struct sc16is7xx_devtype {
char name[10];
int nr_gpio;
int nr_uart;
- int has_mctrl;
};
#define SC16IS7XX_RECONF_MD (1 << 0)
@@ -447,35 +446,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
.name = "SC16IS74X",
.nr_gpio = 0,
.nr_uart = 1,
- .has_mctrl = 0,
};
static const struct sc16is7xx_devtype sc16is750_devtype = {
.name = "SC16IS750",
- .nr_gpio = 4,
+ .nr_gpio = 8,
.nr_uart = 1,
- .has_mctrl = 1,
};
static const struct sc16is7xx_devtype sc16is752_devtype = {
.name = "SC16IS752",
- .nr_gpio = 0,
+ .nr_gpio = 8,
.nr_uart = 2,
- .has_mctrl = 1,
};
static const struct sc16is7xx_devtype sc16is760_devtype = {
.name = "SC16IS760",
- .nr_gpio = 4,
+ .nr_gpio = 8,
.nr_uart = 1,
- .has_mctrl = 1,
};
static const struct sc16is7xx_devtype sc16is762_devtype = {
.name = "SC16IS762",
- .nr_gpio = 0,
+ .nr_gpio = 8,
.nr_uart = 2,
- .has_mctrl = 1,
};
static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"
2023-05-15 16:02 [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines" Hugo Villeneuve
@ 2023-05-15 16:20 ` Greg Kroah-Hartman
2023-05-15 16:51 ` Hugo Villeneuve
2023-05-16 5:18 ` kernel test robot
2023-05-16 6:20 ` kernel test robot
2 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2023-05-15 16:20 UTC (permalink / raw)
To: Hugo Villeneuve
Cc: Jiri Slaby, Lech Perczak, Tomasz Moń, Hugo Villeneuve,
linux-serial, linux-kernel
On Mon, May 15, 2023 at 12:02:07PM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> This reverts commit 679875d1d8802669590ef4d69b0e7d13207ebd61.
>
> Because of this commit, it is no longer possible to use the 16 GPIO
> lines as dedicated GPIOs on the SC16IS752.
>
> Reverting it makes it work again.
>
> The log message of the original commit states:
> "Export only the GPIOs that are not shared with hardware modem
> control lines"
>
> But there is no explanation as to why this decision was taken to
> permanently set the function of the GPIO lines as modem control
> lines. AFAIK, there is no problem with using these lines as GPIO or modem
> control lines.
>
> Maybe after reverting this commit, we could define a new
> device-tree property named, for example,
> "use-modem-control-lines", so that both options can be supported.
>
> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> lines")
Please do not line-wrap these lines.
>
Nor is a blank line needed here.
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
> drivers/tty/serial/sc16is7xx.c | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 5bd98e4316f5..25f1b2f6ec51 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -306,7 +306,6 @@ struct sc16is7xx_devtype {
> char name[10];
> int nr_gpio;
> int nr_uart;
> - int has_mctrl;
> };
>
> #define SC16IS7XX_RECONF_MD (1 << 0)
> @@ -447,35 +446,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
> .name = "SC16IS74X",
> .nr_gpio = 0,
> .nr_uart = 1,
> - .has_mctrl = 0,
> };
>
> static const struct sc16is7xx_devtype sc16is750_devtype = {
> .name = "SC16IS750",
> - .nr_gpio = 4,
> + .nr_gpio = 8,
I think this one line change is all you really need here, right? the
otner changes do nothing in this patch, so you should just create a new
one changing this value. Oh, and this one:
> .nr_uart = 1,
> - .has_mctrl = 1,
> };
>
> static const struct sc16is7xx_devtype sc16is752_devtype = {
> .name = "SC16IS752",
> - .nr_gpio = 0,
> + .nr_gpio = 8,
right?
Don't mess with the has_mctrl stuff, that's not relevant here.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"
2023-05-15 16:20 ` Greg Kroah-Hartman
@ 2023-05-15 16:51 ` Hugo Villeneuve
2023-05-16 8:50 ` Lech Perczak
0 siblings, 1 reply; 10+ messages in thread
From: Hugo Villeneuve @ 2023-05-15 16:51 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, Lech Perczak, Tomasz Moń, Hugo Villeneuve,
linux-serial, linux-kernel
Hi Greg,
On Mon, 15 May 2023 18:20:02 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Mon, May 15, 2023 at 12:02:07PM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >
> > This reverts commit 679875d1d8802669590ef4d69b0e7d13207ebd61.
> >
> > Because of this commit, it is no longer possible to use the 16 GPIO
> > lines as dedicated GPIOs on the SC16IS752.
> >
> > Reverting it makes it work again.
> >
> > The log message of the original commit states:
> > "Export only the GPIOs that are not shared with hardware modem
> > control lines"
> >
> > But there is no explanation as to why this decision was taken to
> > permanently set the function of the GPIO lines as modem control
> > lines. AFAIK, there is no problem with using these lines as GPIO or modem
> > control lines.
> >
> > Maybe after reverting this commit, we could define a new
> > device-tree property named, for example,
> > "use-modem-control-lines", so that both options can be supported.
> >
> > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> > lines")
>
> Please do not line-wrap these lines.
Ok.
> >
>
> Nor is a blank line needed here.
Ok.
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> > drivers/tty/serial/sc16is7xx.c | 14 ++++----------
> > 1 file changed, 4 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 5bd98e4316f5..25f1b2f6ec51 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -306,7 +306,6 @@ struct sc16is7xx_devtype {
> > char name[10];
> > int nr_gpio;
> > int nr_uart;
> > - int has_mctrl;
> > };
> >
> > #define SC16IS7XX_RECONF_MD (1 << 0)
> > @@ -447,35 +446,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
> > .name = "SC16IS74X",
> > .nr_gpio = 0,
> > .nr_uart = 1,
> > - .has_mctrl = 0,
> > };
> >
> > static const struct sc16is7xx_devtype sc16is750_devtype = {
> > .name = "SC16IS750",
> > - .nr_gpio = 4,
> > + .nr_gpio = 8,
>
> I think this one line change is all you really need here, right? the
> otner changes do nothing in this patch, so you should just create a new
> one changing this value. Oh, and this one:
>
> > .nr_uart = 1,
> > - .has_mctrl = 1,
> > };
> >
> > static const struct sc16is7xx_devtype sc16is752_devtype = {
> > .name = "SC16IS752",
> > - .nr_gpio = 0,
> > + .nr_gpio = 8,
>
> right?
>
> Don't mess with the has_mctrl stuff, that's not relevant here.
Sorry, I just noticed that simply reverting commit 679875d1d880 is not sufficient (and will not compile). We must also revert part of commit:
21144bab4f11 ("sc16is7xx: Handle modem status lines").
The problem is that the commit 679875d1d880 was incomplete, and it was (unfortunately) completed by integrating it in commit 21144bab4f11 ("sc16is7xx: Handle modem status lines"). The relevant change was only these 5 new lines, burried deeply into the second commit:
@@ -1353,9 +1452,17 @@ static int sc16is7xx_probe(struct device *dev,
sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
SC16IS7XX_EFCR_RXDISABLE_BIT |
SC16IS7XX_EFCR_TXDISABLE_BIT);
+
+ /* Use GPIO lines as modem status registers */
+ if (devtype->has_mctrl)
+ sc16is7xx_port_write(&s->p[i].port,
+ SC16IS7XX_IOCONTROL_REG,
+ SC16IS7XX_IOCONTROL_MODEM_BIT);
+
Therefore, I should also remove these lines if we go forward with a revert of the patch (should I add another tag "Fixes..." in that case?).
And what do you think of my proposal to maybe replace has_mctrl with a device tree property so that both modes can be fully supported?
Thank you,
Hugo.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"
2023-05-15 16:02 [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines" Hugo Villeneuve
2023-05-15 16:20 ` Greg Kroah-Hartman
@ 2023-05-16 5:18 ` kernel test robot
2023-05-16 6:20 ` kernel test robot
2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2023-05-16 5:18 UTC (permalink / raw)
To: Hugo Villeneuve; +Cc: oe-kbuild-all
Hi Hugo,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on tty/tty-next tty/tty-linus linus/master v6.4-rc2 next-20230516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/Revert-sc16is7xx-Separate-GPIOs-from-modem-control-lines/20230516-022330
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/r/20230515160206.2801991-1-hugo%40hugovil.com
patch subject: [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"
config: loongarch-randconfig-r024-20230515 (https://download.01.org/0day-ci/archive/20230516/202305161358.uIM7kJbI-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/07f35c5b5f2d204418ea211f585d140c9df5506b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hugo-Villeneuve/Revert-sc16is7xx-Separate-GPIOs-from-modem-control-lines/20230516-022330
git checkout 07f35c5b5f2d204418ea211f585d140c9df5506b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=loongarch olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=loongarch SHELL=/bin/bash drivers/tty/serial/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305161358.uIM7kJbI-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_probe':
>> drivers/tty/serial/sc16is7xx.c:1454:28: error: 'const struct sc16is7xx_devtype' has no member named 'has_mctrl'
1454 | if (devtype->has_mctrl)
| ^~
vim +1454 drivers/tty/serial/sc16is7xx.c
267913ecf73745 Ilpo Järvinen 2022-06-06 1352
dfeae619d781de Jon Ringle 2014-04-24 1353 static int sc16is7xx_probe(struct device *dev,
68be64ca7e2adc Jakub Kicinski 2015-07-31 1354 const struct sc16is7xx_devtype *devtype,
37f3965d74d5b5 Daniel Mack 2020-05-21 1355 struct regmap *regmap, int irq)
dfeae619d781de Jon Ringle 2014-04-24 1356 {
24bc6e68efa00f Andy Shevchenko 2019-03-18 1357 unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
2aa916e67db3e6 Daniel Mack 2020-05-21 1358 unsigned int val;
24bc6e68efa00f Andy Shevchenko 2019-03-18 1359 u32 uartclk = 0;
dfeae619d781de Jon Ringle 2014-04-24 1360 int i, ret;
dfeae619d781de Jon Ringle 2014-04-24 1361 struct sc16is7xx_port *s;
dfeae619d781de Jon Ringle 2014-04-24 1362
dfeae619d781de Jon Ringle 2014-04-24 1363 if (IS_ERR(regmap))
dfeae619d781de Jon Ringle 2014-04-24 1364 return PTR_ERR(regmap);
dfeae619d781de Jon Ringle 2014-04-24 1365
2aa916e67db3e6 Daniel Mack 2020-05-21 1366 /*
2aa916e67db3e6 Daniel Mack 2020-05-21 1367 * This device does not have an identification register that would
2aa916e67db3e6 Daniel Mack 2020-05-21 1368 * tell us if we are really connected to the correct device.
2aa916e67db3e6 Daniel Mack 2020-05-21 1369 * The best we can do is to check if communication is at all possible.
2aa916e67db3e6 Daniel Mack 2020-05-21 1370 */
2aa916e67db3e6 Daniel Mack 2020-05-21 1371 ret = regmap_read(regmap,
2aa916e67db3e6 Daniel Mack 2020-05-21 1372 SC16IS7XX_LSR_REG << SC16IS7XX_REG_SHIFT, &val);
2aa916e67db3e6 Daniel Mack 2020-05-21 1373 if (ret < 0)
158e800e0fde91 Annaliese McDermond 2021-03-29 1374 return -EPROBE_DEFER;
2aa916e67db3e6 Daniel Mack 2020-05-21 1375
dfeae619d781de Jon Ringle 2014-04-24 1376 /* Alloc port structure */
84f1c5c0174ace Gustavo A. R. Silva 2019-01-04 1377 s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
dfeae619d781de Jon Ringle 2014-04-24 1378 if (!s) {
dfeae619d781de Jon Ringle 2014-04-24 1379 dev_err(dev, "Error allocating port structure\n");
dfeae619d781de Jon Ringle 2014-04-24 1380 return -ENOMEM;
dfeae619d781de Jon Ringle 2014-04-24 1381 }
dfeae619d781de Jon Ringle 2014-04-24 1382
24bc6e68efa00f Andy Shevchenko 2019-03-18 1383 /* Always ask for fixed clock rate from a property. */
24bc6e68efa00f Andy Shevchenko 2019-03-18 1384 device_property_read_u32(dev, "clock-frequency", &uartclk);
24bc6e68efa00f Andy Shevchenko 2019-03-18 1385
cb1b206cff461d Andy Shevchenko 2021-05-17 1386 s->clk = devm_clk_get_optional(dev, NULL);
cb1b206cff461d Andy Shevchenko 2021-05-17 1387 if (IS_ERR(s->clk))
cb1b206cff461d Andy Shevchenko 2021-05-17 1388 return PTR_ERR(s->clk);
cb1b206cff461d Andy Shevchenko 2021-05-17 1389
cb1b206cff461d Andy Shevchenko 2021-05-17 1390 ret = clk_prepare_enable(s->clk);
cb1b206cff461d Andy Shevchenko 2021-05-17 1391 if (ret)
cb1b206cff461d Andy Shevchenko 2021-05-17 1392 return ret;
cb1b206cff461d Andy Shevchenko 2021-05-17 1393
cb1b206cff461d Andy Shevchenko 2021-05-17 1394 freq = clk_get_rate(s->clk);
cb1b206cff461d Andy Shevchenko 2021-05-17 1395 if (freq == 0) {
24bc6e68efa00f Andy Shevchenko 2019-03-18 1396 if (uartclk)
24bc6e68efa00f Andy Shevchenko 2019-03-18 1397 freq = uartclk;
dfeae619d781de Jon Ringle 2014-04-24 1398 if (pfreq)
dfeae619d781de Jon Ringle 2014-04-24 1399 freq = *pfreq;
24bc6e68efa00f Andy Shevchenko 2019-03-18 1400 if (freq)
24bc6e68efa00f Andy Shevchenko 2019-03-18 1401 dev_dbg(dev, "Clock frequency: %luHz\n", freq);
dfeae619d781de Jon Ringle 2014-04-24 1402 else
cb1b206cff461d Andy Shevchenko 2021-05-17 1403 return -EINVAL;
dfeae619d781de Jon Ringle 2014-04-24 1404 }
dfeae619d781de Jon Ringle 2014-04-24 1405
dfeae619d781de Jon Ringle 2014-04-24 1406 s->regmap = regmap;
dfeae619d781de Jon Ringle 2014-04-24 1407 s->devtype = devtype;
dfeae619d781de Jon Ringle 2014-04-24 1408 dev_set_drvdata(dev, s);
30ec514d440cf2 Phil Elwell 2018-09-12 1409 mutex_init(&s->efr_lock);
dfeae619d781de Jon Ringle 2014-04-24 1410
3989144f863ac5 Petr Mladek 2016-10-11 1411 kthread_init_worker(&s->kworker);
9e6f4ca3e567d5 Jakub Kicinski 2015-05-29 1412 s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
9e6f4ca3e567d5 Jakub Kicinski 2015-05-29 1413 "sc16is7xx");
9e6f4ca3e567d5 Jakub Kicinski 2015-05-29 1414 if (IS_ERR(s->kworker_task)) {
9e6f4ca3e567d5 Jakub Kicinski 2015-05-29 1415 ret = PTR_ERR(s->kworker_task);
c64349722d1417 Jakub Kicinski 2015-07-31 1416 goto out_clk;
9e6f4ca3e567d5 Jakub Kicinski 2015-05-29 1417 }
28d2f209cd1620 Peter Zijlstra 2020-04-21 1418 sched_set_fifo(s->kworker_task);
9e6f4ca3e567d5 Jakub Kicinski 2015-05-29 1419
43c51bb573aab6 Florian Vallee 2016-07-19 1420 /* reset device, purging any pending irq / data */
43c51bb573aab6 Florian Vallee 2016-07-19 1421 regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
43c51bb573aab6 Florian Vallee 2016-07-19 1422 SC16IS7XX_IOCONTROL_SRESET_BIT);
43c51bb573aab6 Florian Vallee 2016-07-19 1423
dfeae619d781de Jon Ringle 2014-04-24 1424 for (i = 0; i < devtype->nr_uart; ++i) {
e92a886bf7841c Jakub Kicinski 2015-07-31 1425 s->p[i].line = i;
dfeae619d781de Jon Ringle 2014-04-24 1426 /* Initialize port data */
dfeae619d781de Jon Ringle 2014-04-24 1427 s->p[i].port.dev = dev;
dfeae619d781de Jon Ringle 2014-04-24 1428 s->p[i].port.irq = irq;
dfeae619d781de Jon Ringle 2014-04-24 1429 s->p[i].port.type = PORT_SC16IS7XX;
dfeae619d781de Jon Ringle 2014-04-24 1430 s->p[i].port.fifosize = SC16IS7XX_FIFO_SIZE;
dfeae619d781de Jon Ringle 2014-04-24 1431 s->p[i].port.flags = UPF_FIXED_TYPE | UPF_LOW_LATENCY;
5da6b1c079e680 Daniel Mack 2020-09-01 1432 s->p[i].port.iobase = i;
dfeae619d781de Jon Ringle 2014-04-24 1433 s->p[i].port.iotype = UPIO_PORT;
dfeae619d781de Jon Ringle 2014-04-24 1434 s->p[i].port.uartclk = freq;
b57d15fe8b37fe Ricardo Ribalda 2014-11-06 1435 s->p[i].port.rs485_config = sc16is7xx_config_rs485;
0139da50dc53f0 Ilpo Järvinen 2022-07-04 1436 s->p[i].port.rs485_supported = sc16is7xx_rs485_supported;
dfeae619d781de Jon Ringle 2014-04-24 1437 s->p[i].port.ops = &sc16is7xx_ops;
21144bab4f1191 Tomasz Moń 2022-03-01 1438 s->p[i].old_mctrl = 0;
c64349722d1417 Jakub Kicinski 2015-07-31 1439 s->p[i].port.line = sc16is7xx_alloc_line();
21144bab4f1191 Tomasz Moń 2022-03-01 1440
c64349722d1417 Jakub Kicinski 2015-07-31 1441 if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
c64349722d1417 Jakub Kicinski 2015-07-31 1442 ret = -ENOMEM;
c64349722d1417 Jakub Kicinski 2015-07-31 1443 goto out_ports;
c64349722d1417 Jakub Kicinski 2015-07-31 1444 }
c64349722d1417 Jakub Kicinski 2015-07-31 1445
dfeae619d781de Jon Ringle 2014-04-24 1446 /* Disable all interrupts */
dfeae619d781de Jon Ringle 2014-04-24 1447 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
dfeae619d781de Jon Ringle 2014-04-24 1448 /* Disable TX/RX */
dfeae619d781de Jon Ringle 2014-04-24 1449 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
dfeae619d781de Jon Ringle 2014-04-24 1450 SC16IS7XX_EFCR_RXDISABLE_BIT |
dfeae619d781de Jon Ringle 2014-04-24 1451 SC16IS7XX_EFCR_TXDISABLE_BIT);
21144bab4f1191 Tomasz Moń 2022-03-01 1452
21144bab4f1191 Tomasz Moń 2022-03-01 1453 /* Use GPIO lines as modem status registers */
21144bab4f1191 Tomasz Moń 2022-03-01 @1454 if (devtype->has_mctrl)
21144bab4f1191 Tomasz Moń 2022-03-01 1455 sc16is7xx_port_write(&s->p[i].port,
21144bab4f1191 Tomasz Moń 2022-03-01 1456 SC16IS7XX_IOCONTROL_REG,
21144bab4f1191 Tomasz Moń 2022-03-01 1457 SC16IS7XX_IOCONTROL_MODEM_BIT);
21144bab4f1191 Tomasz Moń 2022-03-01 1458
a0104085362ff9 Jakub Kicinski 2015-05-29 1459 /* Initialize kthread work structs */
3989144f863ac5 Petr Mladek 2016-10-11 1460 kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
3989144f863ac5 Petr Mladek 2016-10-11 1461 kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
21144bab4f1191 Tomasz Moń 2022-03-01 1462 kthread_init_delayed_work(&s->p[i].ms_work, sc16is7xx_ms_proc);
dfeae619d781de Jon Ringle 2014-04-24 1463 /* Register port */
c64349722d1417 Jakub Kicinski 2015-07-31 1464 uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
43c51bb573aab6 Florian Vallee 2016-07-19 1465
43c51bb573aab6 Florian Vallee 2016-07-19 1466 /* Enable EFR */
43c51bb573aab6 Florian Vallee 2016-07-19 1467 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
43c51bb573aab6 Florian Vallee 2016-07-19 1468 SC16IS7XX_LCR_CONF_MODE_B);
43c51bb573aab6 Florian Vallee 2016-07-19 1469
43c51bb573aab6 Florian Vallee 2016-07-19 1470 regcache_cache_bypass(s->regmap, true);
43c51bb573aab6 Florian Vallee 2016-07-19 1471
43c51bb573aab6 Florian Vallee 2016-07-19 1472 /* Enable write access to enhanced features */
43c51bb573aab6 Florian Vallee 2016-07-19 1473 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFR_REG,
43c51bb573aab6 Florian Vallee 2016-07-19 1474 SC16IS7XX_EFR_ENABLE_BIT);
43c51bb573aab6 Florian Vallee 2016-07-19 1475
43c51bb573aab6 Florian Vallee 2016-07-19 1476 regcache_cache_bypass(s->regmap, false);
43c51bb573aab6 Florian Vallee 2016-07-19 1477
43c51bb573aab6 Florian Vallee 2016-07-19 1478 /* Restore access to general registers */
43c51bb573aab6 Florian Vallee 2016-07-19 1479 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, 0x00);
43c51bb573aab6 Florian Vallee 2016-07-19 1480
dfeae619d781de Jon Ringle 2014-04-24 1481 /* Go to suspend mode */
dfeae619d781de Jon Ringle 2014-04-24 1482 sc16is7xx_power(&s->p[i].port, 0);
dfeae619d781de Jon Ringle 2014-04-24 1483 }
dfeae619d781de Jon Ringle 2014-04-24 1484
9eb90d57b55a0a Pascal Huerst 2020-05-29 1485 if (dev->of_node) {
9eb90d57b55a0a Pascal Huerst 2020-05-29 1486 struct property *prop;
9eb90d57b55a0a Pascal Huerst 2020-05-29 1487 const __be32 *p;
9eb90d57b55a0a Pascal Huerst 2020-05-29 1488 u32 u;
9eb90d57b55a0a Pascal Huerst 2020-05-29 1489
9eb90d57b55a0a Pascal Huerst 2020-05-29 1490 of_property_for_each_u32(dev->of_node, "irda-mode-ports",
9eb90d57b55a0a Pascal Huerst 2020-05-29 1491 prop, p, u)
9eb90d57b55a0a Pascal Huerst 2020-05-29 1492 if (u < devtype->nr_uart)
9eb90d57b55a0a Pascal Huerst 2020-05-29 1493 s->p[u].irda_mode = true;
9eb90d57b55a0a Pascal Huerst 2020-05-29 1494 }
9eb90d57b55a0a Pascal Huerst 2020-05-29 1495
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"
2023-05-15 16:02 [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines" Hugo Villeneuve
2023-05-15 16:20 ` Greg Kroah-Hartman
2023-05-16 5:18 ` kernel test robot
@ 2023-05-16 6:20 ` kernel test robot
2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2023-05-16 6:20 UTC (permalink / raw)
To: Hugo Villeneuve; +Cc: llvm, oe-kbuild-all
Hi Hugo,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on tty/tty-next tty/tty-linus linus/master v6.4-rc2 next-20230516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/Revert-sc16is7xx-Separate-GPIOs-from-modem-control-lines/20230516-022330
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/r/20230515160206.2801991-1-hugo%40hugovil.com
patch subject: [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"
config: i386-randconfig-i053-20230515 (https://download.01.org/0day-ci/archive/20230516/202305161450.DFSDFYLS-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/07f35c5b5f2d204418ea211f585d140c9df5506b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hugo-Villeneuve/Revert-sc16is7xx-Separate-GPIOs-from-modem-control-lines/20230516-022330
git checkout 07f35c5b5f2d204418ea211f585d140c9df5506b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/tty/serial/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305161450.DFSDFYLS-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/tty/serial/sc16is7xx.c:1454:16: error: no member named 'has_mctrl' in 'struct sc16is7xx_devtype'
if (devtype->has_mctrl)
~~~~~~~ ^
1 error generated.
vim +1454 drivers/tty/serial/sc16is7xx.c
267913ecf73745 Ilpo Järvinen 2022-06-06 1352
dfeae619d781de Jon Ringle 2014-04-24 1353 static int sc16is7xx_probe(struct device *dev,
68be64ca7e2adc Jakub Kicinski 2015-07-31 1354 const struct sc16is7xx_devtype *devtype,
37f3965d74d5b5 Daniel Mack 2020-05-21 1355 struct regmap *regmap, int irq)
dfeae619d781de Jon Ringle 2014-04-24 1356 {
24bc6e68efa00f Andy Shevchenko 2019-03-18 1357 unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
2aa916e67db3e6 Daniel Mack 2020-05-21 1358 unsigned int val;
24bc6e68efa00f Andy Shevchenko 2019-03-18 1359 u32 uartclk = 0;
dfeae619d781de Jon Ringle 2014-04-24 1360 int i, ret;
dfeae619d781de Jon Ringle 2014-04-24 1361 struct sc16is7xx_port *s;
dfeae619d781de Jon Ringle 2014-04-24 1362
dfeae619d781de Jon Ringle 2014-04-24 1363 if (IS_ERR(regmap))
dfeae619d781de Jon Ringle 2014-04-24 1364 return PTR_ERR(regmap);
dfeae619d781de Jon Ringle 2014-04-24 1365
2aa916e67db3e6 Daniel Mack 2020-05-21 1366 /*
2aa916e67db3e6 Daniel Mack 2020-05-21 1367 * This device does not have an identification register that would
2aa916e67db3e6 Daniel Mack 2020-05-21 1368 * tell us if we are really connected to the correct device.
2aa916e67db3e6 Daniel Mack 2020-05-21 1369 * The best we can do is to check if communication is at all possible.
2aa916e67db3e6 Daniel Mack 2020-05-21 1370 */
2aa916e67db3e6 Daniel Mack 2020-05-21 1371 ret = regmap_read(regmap,
2aa916e67db3e6 Daniel Mack 2020-05-21 1372 SC16IS7XX_LSR_REG << SC16IS7XX_REG_SHIFT, &val);
2aa916e67db3e6 Daniel Mack 2020-05-21 1373 if (ret < 0)
158e800e0fde91 Annaliese McDermond 2021-03-29 1374 return -EPROBE_DEFER;
2aa916e67db3e6 Daniel Mack 2020-05-21 1375
dfeae619d781de Jon Ringle 2014-04-24 1376 /* Alloc port structure */
84f1c5c0174ace Gustavo A. R. Silva 2019-01-04 1377 s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
dfeae619d781de Jon Ringle 2014-04-24 1378 if (!s) {
dfeae619d781de Jon Ringle 2014-04-24 1379 dev_err(dev, "Error allocating port structure\n");
dfeae619d781de Jon Ringle 2014-04-24 1380 return -ENOMEM;
dfeae619d781de Jon Ringle 2014-04-24 1381 }
dfeae619d781de Jon Ringle 2014-04-24 1382
24bc6e68efa00f Andy Shevchenko 2019-03-18 1383 /* Always ask for fixed clock rate from a property. */
24bc6e68efa00f Andy Shevchenko 2019-03-18 1384 device_property_read_u32(dev, "clock-frequency", &uartclk);
24bc6e68efa00f Andy Shevchenko 2019-03-18 1385
cb1b206cff461d Andy Shevchenko 2021-05-17 1386 s->clk = devm_clk_get_optional(dev, NULL);
cb1b206cff461d Andy Shevchenko 2021-05-17 1387 if (IS_ERR(s->clk))
cb1b206cff461d Andy Shevchenko 2021-05-17 1388 return PTR_ERR(s->clk);
cb1b206cff461d Andy Shevchenko 2021-05-17 1389
cb1b206cff461d Andy Shevchenko 2021-05-17 1390 ret = clk_prepare_enable(s->clk);
cb1b206cff461d Andy Shevchenko 2021-05-17 1391 if (ret)
cb1b206cff461d Andy Shevchenko 2021-05-17 1392 return ret;
cb1b206cff461d Andy Shevchenko 2021-05-17 1393
cb1b206cff461d Andy Shevchenko 2021-05-17 1394 freq = clk_get_rate(s->clk);
cb1b206cff461d Andy Shevchenko 2021-05-17 1395 if (freq == 0) {
24bc6e68efa00f Andy Shevchenko 2019-03-18 1396 if (uartclk)
24bc6e68efa00f Andy Shevchenko 2019-03-18 1397 freq = uartclk;
dfeae619d781de Jon Ringle 2014-04-24 1398 if (pfreq)
dfeae619d781de Jon Ringle 2014-04-24 1399 freq = *pfreq;
24bc6e68efa00f Andy Shevchenko 2019-03-18 1400 if (freq)
24bc6e68efa00f Andy Shevchenko 2019-03-18 1401 dev_dbg(dev, "Clock frequency: %luHz\n", freq);
dfeae619d781de Jon Ringle 2014-04-24 1402 else
cb1b206cff461d Andy Shevchenko 2021-05-17 1403 return -EINVAL;
dfeae619d781de Jon Ringle 2014-04-24 1404 }
dfeae619d781de Jon Ringle 2014-04-24 1405
dfeae619d781de Jon Ringle 2014-04-24 1406 s->regmap = regmap;
dfeae619d781de Jon Ringle 2014-04-24 1407 s->devtype = devtype;
dfeae619d781de Jon Ringle 2014-04-24 1408 dev_set_drvdata(dev, s);
30ec514d440cf2 Phil Elwell 2018-09-12 1409 mutex_init(&s->efr_lock);
dfeae619d781de Jon Ringle 2014-04-24 1410
3989144f863ac5 Petr Mladek 2016-10-11 1411 kthread_init_worker(&s->kworker);
9e6f4ca3e567d5 Jakub Kicinski 2015-05-29 1412 s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
9e6f4ca3e567d5 Jakub Kicinski 2015-05-29 1413 "sc16is7xx");
9e6f4ca3e567d5 Jakub Kicinski 2015-05-29 1414 if (IS_ERR(s->kworker_task)) {
9e6f4ca3e567d5 Jakub Kicinski 2015-05-29 1415 ret = PTR_ERR(s->kworker_task);
c64349722d1417 Jakub Kicinski 2015-07-31 1416 goto out_clk;
9e6f4ca3e567d5 Jakub Kicinski 2015-05-29 1417 }
28d2f209cd1620 Peter Zijlstra 2020-04-21 1418 sched_set_fifo(s->kworker_task);
9e6f4ca3e567d5 Jakub Kicinski 2015-05-29 1419
43c51bb573aab6 Florian Vallee 2016-07-19 1420 /* reset device, purging any pending irq / data */
43c51bb573aab6 Florian Vallee 2016-07-19 1421 regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
43c51bb573aab6 Florian Vallee 2016-07-19 1422 SC16IS7XX_IOCONTROL_SRESET_BIT);
43c51bb573aab6 Florian Vallee 2016-07-19 1423
dfeae619d781de Jon Ringle 2014-04-24 1424 for (i = 0; i < devtype->nr_uart; ++i) {
e92a886bf7841c Jakub Kicinski 2015-07-31 1425 s->p[i].line = i;
dfeae619d781de Jon Ringle 2014-04-24 1426 /* Initialize port data */
dfeae619d781de Jon Ringle 2014-04-24 1427 s->p[i].port.dev = dev;
dfeae619d781de Jon Ringle 2014-04-24 1428 s->p[i].port.irq = irq;
dfeae619d781de Jon Ringle 2014-04-24 1429 s->p[i].port.type = PORT_SC16IS7XX;
dfeae619d781de Jon Ringle 2014-04-24 1430 s->p[i].port.fifosize = SC16IS7XX_FIFO_SIZE;
dfeae619d781de Jon Ringle 2014-04-24 1431 s->p[i].port.flags = UPF_FIXED_TYPE | UPF_LOW_LATENCY;
5da6b1c079e680 Daniel Mack 2020-09-01 1432 s->p[i].port.iobase = i;
dfeae619d781de Jon Ringle 2014-04-24 1433 s->p[i].port.iotype = UPIO_PORT;
dfeae619d781de Jon Ringle 2014-04-24 1434 s->p[i].port.uartclk = freq;
b57d15fe8b37fe Ricardo Ribalda 2014-11-06 1435 s->p[i].port.rs485_config = sc16is7xx_config_rs485;
0139da50dc53f0 Ilpo Järvinen 2022-07-04 1436 s->p[i].port.rs485_supported = sc16is7xx_rs485_supported;
dfeae619d781de Jon Ringle 2014-04-24 1437 s->p[i].port.ops = &sc16is7xx_ops;
21144bab4f1191 Tomasz Moń 2022-03-01 1438 s->p[i].old_mctrl = 0;
c64349722d1417 Jakub Kicinski 2015-07-31 1439 s->p[i].port.line = sc16is7xx_alloc_line();
21144bab4f1191 Tomasz Moń 2022-03-01 1440
c64349722d1417 Jakub Kicinski 2015-07-31 1441 if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
c64349722d1417 Jakub Kicinski 2015-07-31 1442 ret = -ENOMEM;
c64349722d1417 Jakub Kicinski 2015-07-31 1443 goto out_ports;
c64349722d1417 Jakub Kicinski 2015-07-31 1444 }
c64349722d1417 Jakub Kicinski 2015-07-31 1445
dfeae619d781de Jon Ringle 2014-04-24 1446 /* Disable all interrupts */
dfeae619d781de Jon Ringle 2014-04-24 1447 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
dfeae619d781de Jon Ringle 2014-04-24 1448 /* Disable TX/RX */
dfeae619d781de Jon Ringle 2014-04-24 1449 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
dfeae619d781de Jon Ringle 2014-04-24 1450 SC16IS7XX_EFCR_RXDISABLE_BIT |
dfeae619d781de Jon Ringle 2014-04-24 1451 SC16IS7XX_EFCR_TXDISABLE_BIT);
21144bab4f1191 Tomasz Moń 2022-03-01 1452
21144bab4f1191 Tomasz Moń 2022-03-01 1453 /* Use GPIO lines as modem status registers */
21144bab4f1191 Tomasz Moń 2022-03-01 @1454 if (devtype->has_mctrl)
21144bab4f1191 Tomasz Moń 2022-03-01 1455 sc16is7xx_port_write(&s->p[i].port,
21144bab4f1191 Tomasz Moń 2022-03-01 1456 SC16IS7XX_IOCONTROL_REG,
21144bab4f1191 Tomasz Moń 2022-03-01 1457 SC16IS7XX_IOCONTROL_MODEM_BIT);
21144bab4f1191 Tomasz Moń 2022-03-01 1458
a0104085362ff9 Jakub Kicinski 2015-05-29 1459 /* Initialize kthread work structs */
3989144f863ac5 Petr Mladek 2016-10-11 1460 kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
3989144f863ac5 Petr Mladek 2016-10-11 1461 kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
21144bab4f1191 Tomasz Moń 2022-03-01 1462 kthread_init_delayed_work(&s->p[i].ms_work, sc16is7xx_ms_proc);
dfeae619d781de Jon Ringle 2014-04-24 1463 /* Register port */
c64349722d1417 Jakub Kicinski 2015-07-31 1464 uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
43c51bb573aab6 Florian Vallee 2016-07-19 1465
43c51bb573aab6 Florian Vallee 2016-07-19 1466 /* Enable EFR */
43c51bb573aab6 Florian Vallee 2016-07-19 1467 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
43c51bb573aab6 Florian Vallee 2016-07-19 1468 SC16IS7XX_LCR_CONF_MODE_B);
43c51bb573aab6 Florian Vallee 2016-07-19 1469
43c51bb573aab6 Florian Vallee 2016-07-19 1470 regcache_cache_bypass(s->regmap, true);
43c51bb573aab6 Florian Vallee 2016-07-19 1471
43c51bb573aab6 Florian Vallee 2016-07-19 1472 /* Enable write access to enhanced features */
43c51bb573aab6 Florian Vallee 2016-07-19 1473 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFR_REG,
43c51bb573aab6 Florian Vallee 2016-07-19 1474 SC16IS7XX_EFR_ENABLE_BIT);
43c51bb573aab6 Florian Vallee 2016-07-19 1475
43c51bb573aab6 Florian Vallee 2016-07-19 1476 regcache_cache_bypass(s->regmap, false);
43c51bb573aab6 Florian Vallee 2016-07-19 1477
43c51bb573aab6 Florian Vallee 2016-07-19 1478 /* Restore access to general registers */
43c51bb573aab6 Florian Vallee 2016-07-19 1479 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, 0x00);
43c51bb573aab6 Florian Vallee 2016-07-19 1480
dfeae619d781de Jon Ringle 2014-04-24 1481 /* Go to suspend mode */
dfeae619d781de Jon Ringle 2014-04-24 1482 sc16is7xx_power(&s->p[i].port, 0);
dfeae619d781de Jon Ringle 2014-04-24 1483 }
dfeae619d781de Jon Ringle 2014-04-24 1484
9eb90d57b55a0a Pascal Huerst 2020-05-29 1485 if (dev->of_node) {
9eb90d57b55a0a Pascal Huerst 2020-05-29 1486 struct property *prop;
9eb90d57b55a0a Pascal Huerst 2020-05-29 1487 const __be32 *p;
9eb90d57b55a0a Pascal Huerst 2020-05-29 1488 u32 u;
9eb90d57b55a0a Pascal Huerst 2020-05-29 1489
9eb90d57b55a0a Pascal Huerst 2020-05-29 1490 of_property_for_each_u32(dev->of_node, "irda-mode-ports",
9eb90d57b55a0a Pascal Huerst 2020-05-29 1491 prop, p, u)
9eb90d57b55a0a Pascal Huerst 2020-05-29 1492 if (u < devtype->nr_uart)
9eb90d57b55a0a Pascal Huerst 2020-05-29 1493 s->p[u].irda_mode = true;
9eb90d57b55a0a Pascal Huerst 2020-05-29 1494 }
9eb90d57b55a0a Pascal Huerst 2020-05-29 1495
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"
2023-05-15 16:51 ` Hugo Villeneuve
@ 2023-05-16 8:50 ` Lech Perczak
2023-05-16 15:59 ` Hugo Villeneuve
0 siblings, 1 reply; 10+ messages in thread
From: Lech Perczak @ 2023-05-16 8:50 UTC (permalink / raw)
To: Hugo Villeneuve, Greg Kroah-Hartman
Cc: Jiri Slaby, Lech Perczak, Tomasz Moń, Hugo Villeneuve,
linux-serial, linux-kernel, Krzysztof Drobiński
Hi Hugo,
Please see my answers inline.
W dniu 15.05.2023 o 18:51, Hugo Villeneuve pisze:
> Hi Greg,
>
> On Mon, 15 May 2023 18:20:02 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>
>> On Mon, May 15, 2023 at 12:02:07PM -0400, Hugo Villeneuve wrote:
>>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>>>
>>> This reverts commit 679875d1d8802669590ef4d69b0e7d13207ebd61.
>>>
>>> Because of this commit, it is no longer possible to use the 16 GPIO
>>> lines as dedicated GPIOs on the SC16IS752.
>>>
>>> Reverting it makes it work again.
>>>
>>> The log message of the original commit states:
>>> "Export only the GPIOs that are not shared with hardware modem
>>> control lines"
>>>
>>> But there is no explanation as to why this decision was taken to
>>> permanently set the function of the GPIO lines as modem control
>>> lines. AFAIK, there is no problem with using these lines as GPIO or modem
>>> control lines.
>>>
>>> Maybe after reverting this commit, we could define a new
>>> device-tree property named, for example,
>>> "use-modem-control-lines", so that both options can be supported.
>>>
>>> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
>>> lines")
>> Please do not line-wrap these lines.
> Ok.
>
>> Nor is a blank line needed here.
> Ok.
>
>>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>>> ---
>>> drivers/tty/serial/sc16is7xx.c | 14 ++++----------
>>> 1 file changed, 4 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
>>> index 5bd98e4316f5..25f1b2f6ec51 100644
>>> --- a/drivers/tty/serial/sc16is7xx.c
>>> +++ b/drivers/tty/serial/sc16is7xx.c
>>> @@ -306,7 +306,6 @@ struct sc16is7xx_devtype {
>>> char name[10];
>>> int nr_gpio;
>>> int nr_uart;
>>> - int has_mctrl;
>>> };
>>>
>>> #define SC16IS7XX_RECONF_MD (1 << 0)
>>> @@ -447,35 +446,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
>>> .name = "SC16IS74X",
>>> .nr_gpio = 0,
>>> .nr_uart = 1,
>>> - .has_mctrl = 0,
>>> };
>>>
>>> static const struct sc16is7xx_devtype sc16is750_devtype = {
>>> .name = "SC16IS750",
>>> - .nr_gpio = 4,
>>> + .nr_gpio = 8,
>> I think this one line change is all you really need here, right? the
>> otner changes do nothing in this patch, so you should just create a new
>> one changing this value. Oh, and this one:
>>
>>> .nr_uart = 1,
>>> - .has_mctrl = 1,
>>> };
>>>
>>> static const struct sc16is7xx_devtype sc16is752_devtype = {
>>> .name = "SC16IS752",
>>> - .nr_gpio = 0,
>>> + .nr_gpio = 8,
>> right?
>>
>> Don't mess with the has_mctrl stuff, that's not relevant here.
> Sorry, I just noticed that simply reverting commit 679875d1d880 is not sufficient (and will not compile). We must also revert part of commit:
> 21144bab4f11 ("sc16is7xx: Handle modem status lines").
>
> The problem is that the commit 679875d1d880 was incomplete, and it was (unfortunately) completed by integrating it in commit 21144bab4f11 ("sc16is7xx: Handle modem status lines"). The relevant change was only these 5 new lines, burried deeply into the second commit:
Just as you noticed, this was required to support full set of flow control lines on this device.
The commit you're trying to revert was a preparation for it. Disabling has_mctrl will break it.
I kindly suggest to suggest a fix, instead of hurrying a revert, and waiting for a proper fix later.
>
> @@ -1353,9 +1452,17 @@ static int sc16is7xx_probe(struct device *dev,
> sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
> SC16IS7XX_EFCR_RXDISABLE_BIT |
> SC16IS7XX_EFCR_TXDISABLE_BIT);
> +
> + /* Use GPIO lines as modem status registers */
> + if (devtype->has_mctrl)
> + sc16is7xx_port_write(&s->p[i].port,
> + SC16IS7XX_IOCONTROL_REG,
> + SC16IS7XX_IOCONTROL_MODEM_BIT);
> +
>
> Therefore, I should also remove these lines if we go forward with a revert of the patch (should I add another tag "Fixes..." in that case?).
>
> And what do you think of my proposal to maybe replace has_mctrl with a device tree property so that both modes can be fully supported?
I think the proper solution here, is not to invent a new device tree property for every single use case.
I would start by looking for other drivers, if, and how they handle similar cases.
For example, imx-serial driver respects "uart-has-rtscts" property, as do a lot of other controllers built into SoC-s.
On the other hand, other devices which can also provide GPIOs, respect "gpio-controller" property.
According to SC16IS752 datasheet [1], respecting one of those should be enough,
as GPIOs can be enabled in groups of four pins even for dual UART version.
Every group matches a single port, so probably this can be probably selected per UART even on dual-port versions.
I'll be more than happy to assist with that.
>
> Thank you,
> Hugo.
>
[1] https://www.nxp.com/docs/en/data-sheet/SC16IS752_SC16IS762.pdf
--
Pozdrawiam/With kind regards,
Lech Perczak
Sr. Software Engineer
Camlin Technologies Poland Limited Sp. z o.o.
Strzegomska 54,
53-611 Wroclaw
Tel: (+48) 71 75 000 16
Email: lech.perczak@camlingroup.com
Website: http://www.camlingroup.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"
2023-05-16 8:50 ` Lech Perczak
@ 2023-05-16 15:59 ` Hugo Villeneuve
2023-05-16 22:09 ` Hugo Villeneuve
0 siblings, 1 reply; 10+ messages in thread
From: Hugo Villeneuve @ 2023-05-16 15:59 UTC (permalink / raw)
To: Lech Perczak
Cc: Greg Kroah-Hartman, Jiri Slaby, Lech Perczak, Tomasz Moń,
Hugo Villeneuve, linux-serial, linux-kernel,
Krzysztof Drobiński, hugo
On Tue, 16 May 2023 10:50:11 +0200
Lech Perczak <lech.perczak@camlingroup.com> wrote:
> Hi Hugo,
>
> Please see my answers inline.
>
> W dniu 15.05.2023 o 18:51, Hugo Villeneuve pisze:
> > Hi Greg,
> >
> > On Mon, 15 May 2023 18:20:02 +0200
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >
> >> On Mon, May 15, 2023 at 12:02:07PM -0400, Hugo Villeneuve wrote:
> >>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >>>
> >>> This reverts commit 679875d1d8802669590ef4d69b0e7d13207ebd61.
> >>>
> >>> Because of this commit, it is no longer possible to use the 16 GPIO
> >>> lines as dedicated GPIOs on the SC16IS752.
> >>>
> >>> Reverting it makes it work again.
> >>>
> >>> The log message of the original commit states:
> >>> "Export only the GPIOs that are not shared with hardware modem
> >>> control lines"
> >>>
> >>> But there is no explanation as to why this decision was taken to
> >>> permanently set the function of the GPIO lines as modem control
> >>> lines. AFAIK, there is no problem with using these lines as GPIO or modem
> >>> control lines.
> >>>
> >>> Maybe after reverting this commit, we could define a new
> >>> device-tree property named, for example,
> >>> "use-modem-control-lines", so that both options can be supported.
> >>>
> >>> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> >>> lines")
> >> Please do not line-wrap these lines.
> > Ok.
> >
> >> Nor is a blank line needed here.
> > Ok.
> >
> >>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >>> ---
> >>> drivers/tty/serial/sc16is7xx.c | 14 ++++----------
> >>> 1 file changed, 4 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> >>> index 5bd98e4316f5..25f1b2f6ec51 100644
> >>> --- a/drivers/tty/serial/sc16is7xx.c
> >>> +++ b/drivers/tty/serial/sc16is7xx.c
> >>> @@ -306,7 +306,6 @@ struct sc16is7xx_devtype {
> >>> char name[10];
> >>> int nr_gpio;
> >>> int nr_uart;
> >>> - int has_mctrl;
> >>> };
> >>>
> >>> #define SC16IS7XX_RECONF_MD (1 << 0)
> >>> @@ -447,35 +446,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
> >>> .name = "SC16IS74X",
> >>> .nr_gpio = 0,
> >>> .nr_uart = 1,
> >>> - .has_mctrl = 0,
> >>> };
> >>>
> >>> static const struct sc16is7xx_devtype sc16is750_devtype = {
> >>> .name = "SC16IS750",
> >>> - .nr_gpio = 4,
> >>> + .nr_gpio = 8,
> >> I think this one line change is all you really need here, right? the
> >> otner changes do nothing in this patch, so you should just create a new
> >> one changing this value. Oh, and this one:
> >>
> >>> .nr_uart = 1,
> >>> - .has_mctrl = 1,
> >>> };
> >>>
> >>> static const struct sc16is7xx_devtype sc16is752_devtype = {
> >>> .name = "SC16IS752",
> >>> - .nr_gpio = 0,
> >>> + .nr_gpio = 8,
> >> right?
> >>
> >> Don't mess with the has_mctrl stuff, that's not relevant here.
> > Sorry, I just noticed that simply reverting commit 679875d1d880 is not sufficient (and will not compile). We must also revert part of commit:
> > 21144bab4f11 ("sc16is7xx: Handle modem status lines").
> >
> > The problem is that the commit 679875d1d880 was incomplete, and it was (unfortunately) completed by integrating it in commit 21144bab4f11 ("sc16is7xx: Handle modem status lines"). The relevant change was only these 5 new lines, burried deeply into the second commit:
> Just as you noticed, this was required to support full set of flow control lines on this device.
> The commit you're trying to revert was a preparation for it. Disabling has_mctrl will break it.
> I kindly suggest to suggest a fix, instead of hurrying a revert, and waiting for a proper fix later.
Hi Lech,
the [RFC] in the subject was there to discuss about a possible revert, and/or maybe a possible fix that would allow both modes to be supported. I am not hurrying anything and I am certainly not waiting for a later fix, as I very much want to help and maybe submit such a fix myself.
But the reality is that commits 679875d1d880/21144bab4f11 broke userspace by forcing GPIOs as modem control lines. I understand that reverting these patches could also potentially break things for applications depending on these patches. I am simply wondering what is the proper course of action here: revert patches and work on a fix to support both modes, or skip revert and work on a fix (my preference)?
> > @@ -1353,9 +1452,17 @@ static int sc16is7xx_probe(struct device *dev,
> > sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
> > SC16IS7XX_EFCR_RXDISABLE_BIT |
> > SC16IS7XX_EFCR_TXDISABLE_BIT);
> > +
> > + /* Use GPIO lines as modem status registers */
> > + if (devtype->has_mctrl)
> > + sc16is7xx_port_write(&s->p[i].port,
> > + SC16IS7XX_IOCONTROL_REG,
> > + SC16IS7XX_IOCONTROL_MODEM_BIT);
> > +
> >
> > Therefore, I should also remove these lines if we go forward with a revert of the patch (should I add another tag "Fixes..." in that case?).
> >
> > And what do you think of my proposal to maybe replace has_mctrl with a device tree property so that both modes can be fully supported?
> I think the proper solution here, is not to invent a new device tree property for every single use case.
> I would start by looking for other drivers, if, and how they handle similar cases.
> For example, imx-serial driver respects "uart-has-rtscts" property, as do a lot of other controllers built into SoC-s.
> On the other hand, other devices which can also provide GPIOs, respect "gpio-controller" property.
I think that testing the presence of the "uart-has-rtscts" to force GPIOs as modem control lines would make a lot of sense.
> According to SC16IS752 datasheet [1], respecting one of those should be enough,
> as GPIOs can be enabled in groups of four pins even for dual UART version.
> Every group matches a single port, so probably this can be probably selected per UART even on dual-port versions.
I am trying to see how we could set "uart-has-rtscts" for only UART channel A or B in the device tree, but cannot find any example or documentation about that. How do you propose to do it?
From what I understand, the property "uart-has-rtscts" can be set only for the whole chip (channels A and B)...
Hugo.
> I'll be more than happy to assist with that.
>
> >
> > Thank you,
> > Hugo.
> >
> [1] https://www.nxp.com/docs/en/data-sheet/SC16IS752_SC16IS762.pdf
>
> --
> Pozdrawiam/With kind regards,
> Lech Perczak
>
> Sr. Software Engineer
> Camlin Technologies Poland Limited Sp. z o.o.
> Strzegomska 54,
> 53-611 Wroclaw
> Tel: (+48) 71 75 000 16
> Email: lech.perczak@camlingroup.com
> Website: http://www.camlingroup.com
>
>
--
Hugo Villeneuve
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"
2023-05-16 15:59 ` Hugo Villeneuve
@ 2023-05-16 22:09 ` Hugo Villeneuve
2023-05-17 9:18 ` Lech Perczak
0 siblings, 1 reply; 10+ messages in thread
From: Hugo Villeneuve @ 2023-05-16 22:09 UTC (permalink / raw)
To: Hugo Villeneuve
Cc: Lech Perczak, Greg Kroah-Hartman, Jiri Slaby, Lech Perczak,
Tomasz Moń, Hugo Villeneuve, linux-serial, linux-kernel,
Krzysztof Drobiński, hugo
On Tue, 16 May 2023 11:59:06 -0400
Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Tue, 16 May 2023 10:50:11 +0200
> Lech Perczak <lech.perczak@camlingroup.com> wrote:
>
> > Hi Hugo,
> >
> > Please see my answers inline.
> >
> > W dniu 15.05.2023 o 18:51, Hugo Villeneuve pisze:
> > > Hi Greg,
> > >
> > > On Mon, 15 May 2023 18:20:02 +0200
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > >
> > >> On Mon, May 15, 2023 at 12:02:07PM -0400, Hugo Villeneuve wrote:
> > >>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > >>>
> > >>> This reverts commit 679875d1d8802669590ef4d69b0e7d13207ebd61.
> > >>>
> > >>> Because of this commit, it is no longer possible to use the 16 GPIO
> > >>> lines as dedicated GPIOs on the SC16IS752.
> > >>>
> > >>> Reverting it makes it work again.
> > >>>
> > >>> The log message of the original commit states:
> > >>> "Export only the GPIOs that are not shared with hardware modem
> > >>> control lines"
> > >>>
> > >>> But there is no explanation as to why this decision was taken to
> > >>> permanently set the function of the GPIO lines as modem control
> > >>> lines. AFAIK, there is no problem with using these lines as GPIO or modem
> > >>> control lines.
> > >>>
> > >>> Maybe after reverting this commit, we could define a new
> > >>> device-tree property named, for example,
> > >>> "use-modem-control-lines", so that both options can be supported.
> > >>>
> > >>> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> > >>> lines")
> > >> Please do not line-wrap these lines.
> > > Ok.
> > >
> > >> Nor is a blank line needed here.
> > > Ok.
> > >
> > >>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > >>> ---
> > >>> drivers/tty/serial/sc16is7xx.c | 14 ++++----------
> > >>> 1 file changed, 4 insertions(+), 10 deletions(-)
> > >>>
> > >>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > >>> index 5bd98e4316f5..25f1b2f6ec51 100644
> > >>> --- a/drivers/tty/serial/sc16is7xx.c
> > >>> +++ b/drivers/tty/serial/sc16is7xx.c
> > >>> @@ -306,7 +306,6 @@ struct sc16is7xx_devtype {
> > >>> char name[10];
> > >>> int nr_gpio;
> > >>> int nr_uart;
> > >>> - int has_mctrl;
> > >>> };
> > >>>
> > >>> #define SC16IS7XX_RECONF_MD (1 << 0)
> > >>> @@ -447,35 +446,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
> > >>> .name = "SC16IS74X",
> > >>> .nr_gpio = 0,
> > >>> .nr_uart = 1,
> > >>> - .has_mctrl = 0,
> > >>> };
> > >>>
> > >>> static const struct sc16is7xx_devtype sc16is750_devtype = {
> > >>> .name = "SC16IS750",
> > >>> - .nr_gpio = 4,
> > >>> + .nr_gpio = 8,
> > >> I think this one line change is all you really need here, right? the
> > >> otner changes do nothing in this patch, so you should just create a new
> > >> one changing this value. Oh, and this one:
> > >>
> > >>> .nr_uart = 1,
> > >>> - .has_mctrl = 1,
> > >>> };
> > >>>
> > >>> static const struct sc16is7xx_devtype sc16is752_devtype = {
> > >>> .name = "SC16IS752",
> > >>> - .nr_gpio = 0,
> > >>> + .nr_gpio = 8,
> > >> right?
> > >>
> > >> Don't mess with the has_mctrl stuff, that's not relevant here.
> > > Sorry, I just noticed that simply reverting commit 679875d1d880 is not sufficient (and will not compile). We must also revert part of commit:
> > > 21144bab4f11 ("sc16is7xx: Handle modem status lines").
> > >
> > > The problem is that the commit 679875d1d880 was incomplete, and it was (unfortunately) completed by integrating it in commit 21144bab4f11 ("sc16is7xx: Handle modem status lines"). The relevant change was only these 5 new lines, burried deeply into the second commit:
> > Just as you noticed, this was required to support full set of flow control lines on this device.
> > The commit you're trying to revert was a preparation for it. Disabling has_mctrl will break it.
> > I kindly suggest to suggest a fix, instead of hurrying a revert, and waiting for a proper fix later.
>
> Hi Lech,
> the [RFC] in the subject was there to discuss about a possible revert, and/or maybe a possible fix that would allow both modes to be supported. I am not hurrying anything and I am certainly not waiting for a later fix, as I very much want to help and maybe submit such a fix myself.
>
> But the reality is that commits 679875d1d880/21144bab4f11 broke userspace by forcing GPIOs as modem control lines. I understand that reverting these patches could also potentially break things for applications depending on these patches. I am simply wondering what is the proper course of action here: revert patches and work on a fix to support both modes, or skip revert and work on a fix (my preference)?
>
> > > @@ -1353,9 +1452,17 @@ static int sc16is7xx_probe(struct device *dev,
> > > sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
> > > SC16IS7XX_EFCR_RXDISABLE_BIT |
> > > SC16IS7XX_EFCR_TXDISABLE_BIT);
> > > +
> > > + /* Use GPIO lines as modem status registers */
> > > + if (devtype->has_mctrl)
> > > + sc16is7xx_port_write(&s->p[i].port,
> > > + SC16IS7XX_IOCONTROL_REG,
> > > + SC16IS7XX_IOCONTROL_MODEM_BIT);
> > > +
> > >
> > > Therefore, I should also remove these lines if we go forward with a revert of the patch (should I add another tag "Fixes..." in that case?).
> > >
> > > And what do you think of my proposal to maybe replace has_mctrl with a device tree property so that both modes can be fully supported?
> > I think the proper solution here, is not to invent a new device tree property for every single use case.
> > I would start by looking for other drivers, if, and how they handle similar cases.
> > For example, imx-serial driver respects "uart-has-rtscts" property, as do a lot of other controllers built into SoC-s.
> > On the other hand, other devices which can also provide GPIOs, respect "gpio-controller" property.
>
> I think that testing the presence of the "uart-has-rtscts" to force GPIOs as modem control lines would make a lot of sense.
>
> > According to SC16IS752 datasheet [1], respecting one of those should be enough,
> > as GPIOs can be enabled in groups of four pins even for dual UART version.
> > Every group matches a single port, so probably this can be probably selected per UART even on dual-port versions.
>
> I am trying to see how we could set "uart-has-rtscts" for only UART channel A or B in the device tree, but cannot find any example or documentation about that. How do you propose to do it?
>
> From what I understand, the property "uart-has-rtscts" can be set only for the whole chip (channels A and B)...
After some analysis, I don't think that we should be using the property "uart-has-rtscts". For our chip, this doesn't make sense because RTS/CTS are dedicated pins. also, like I said, this property applies to the whole chip/device, not to indivual A or B channels (like sc16is752).
The way to go would be to define a new DT property similar to "irda-mode-ports" (for the same sc16is7xx driver). Defining a new property named "modem-control-line-ports" would allow us to specify an array that lists the indices of the port that should have shared GPIO lines configured as modem control lines.
I already implemented that as a proof of concept and it works great.
Hugo.
> > I'll be more than happy to assist with that.
> >
> > >
> > > Thank you,
> > > Hugo.
> > >
> > [1] https://www.nxp.com/docs/en/data-sheet/SC16IS752_SC16IS762.pdf
> >
> > --
> > Pozdrawiam/With kind regards,
> > Lech Perczak
> >
> > Sr. Software Engineer
> > Camlin Technologies Poland Limited Sp. z o.o.
> > Strzegomska 54,
> > 53-611 Wroclaw
> > Tel: (+48) 71 75 000 16
> > Email: lech.perczak@camlingroup.com
> > Website: http://www.camlingroup.com
> >
> >
>
>
> --
> Hugo Villeneuve
>
--
Hugo Villeneuve
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"
2023-05-16 22:09 ` Hugo Villeneuve
@ 2023-05-17 9:18 ` Lech Perczak
2023-05-17 13:58 ` Hugo Villeneuve
0 siblings, 1 reply; 10+ messages in thread
From: Lech Perczak @ 2023-05-17 9:18 UTC (permalink / raw)
To: Hugo Villeneuve
Cc: Greg Kroah-Hartman, Jiri Slaby, Lech Perczak, Tomasz Moń,
Hugo Villeneuve, linux-serial, linux-kernel,
Krzysztof Drobiński
W dniu 17.05.2023 o 00:09, Hugo Villeneuve pisze:
> On Tue, 16 May 2023 11:59:06 -0400
> Hugo Villeneuve <hugo@hugovil.com> wrote:
>
> > On Tue, 16 May 2023 10:50:11 +0200
> > Lech Perczak <lech.perczak@camlingroup.com> wrote:
> >
> > > Hi Hugo,
> > >
> > > Please see my answers inline.
> > >
> > > W dniu 15.05.2023 o 18:51, Hugo Villeneuve pisze:
> > > > Hi Greg,
> > > >
> > > > On Mon, 15 May 2023 18:20:02 +0200
> > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > >
> > > >> On Mon, May 15, 2023 at 12:02:07PM -0400, Hugo Villeneuve wrote:
> > > >>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > >>>
> > > >>> This reverts commit 679875d1d8802669590ef4d69b0e7d13207ebd61.
> > > >>>
> > > >>> Because of this commit, it is no longer possible to use the 16 GPIO
> > > >>> lines as dedicated GPIOs on the SC16IS752.
> > > >>>
> > > >>> Reverting it makes it work again.
> > > >>>
> > > >>> The log message of the original commit states:
> > > >>> "Export only the GPIOs that are not shared with hardware modem
> > > >>> control lines"
> > > >>>
> > > >>> But there is no explanation as to why this decision was taken to
> > > >>> permanently set the function of the GPIO lines as modem control
> > > >>> lines. AFAIK, there is no problem with using these lines as GPIO or modem
> > > >>> control lines.
> > > >>>
> > > >>> Maybe after reverting this commit, we could define a new
> > > >>> device-tree property named, for example,
> > > >>> "use-modem-control-lines", so that both options can be supported.
> > > >>>
> > > >>> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> > > >>> lines")
> > > >> Please do not line-wrap these lines.
> > > > Ok.
> > > >
> > > >> Nor is a blank line needed here.
> > > > Ok.
> > > >
> > > >>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > >>> ---
> > > >>> drivers/tty/serial/sc16is7xx.c | 14 ++++----------
> > > >>> 1 file changed, 4 insertions(+), 10 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > >>> index 5bd98e4316f5..25f1b2f6ec51 100644
> > > >>> --- a/drivers/tty/serial/sc16is7xx.c
> > > >>> +++ b/drivers/tty/serial/sc16is7xx.c
> > > >>> @@ -306,7 +306,6 @@ struct sc16is7xx_devtype {
> > > >>> char name[10];
> > > >>> int nr_gpio;
> > > >>> int nr_uart;
> > > >>> - int has_mctrl;
> > > >>> };
> > > >>>
> > > >>> #define SC16IS7XX_RECONF_MD (1 << 0)
> > > >>> @@ -447,35 +446,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
> > > >>> .name = "SC16IS74X",
> > > >>> .nr_gpio = 0,
> > > >>> .nr_uart = 1,
> > > >>> - .has_mctrl = 0,
> > > >>> };
> > > >>>
> > > >>> static const struct sc16is7xx_devtype sc16is750_devtype = {
> > > >>> .name = "SC16IS750",
> > > >>> - .nr_gpio = 4,
> > > >>> + .nr_gpio = 8,
> > > >> I think this one line change is all you really need here, right? the
> > > >> otner changes do nothing in this patch, so you should just create a new
> > > >> one changing this value. Oh, and this one:
> > > >>
> > > >>> .nr_uart = 1,
> > > >>> - .has_mctrl = 1,
> > > >>> };
> > > >>>
> > > >>> static const struct sc16is7xx_devtype sc16is752_devtype = {
> > > >>> .name = "SC16IS752",
> > > >>> - .nr_gpio = 0,
> > > >>> + .nr_gpio = 8,
> > > >> right?
> > > >>
> > > >> Don't mess with the has_mctrl stuff, that's not relevant here.
> > > > Sorry, I just noticed that simply reverting commit 679875d1d880 is not sufficient (and will not compile). We must also revert part of commit:
> > > > 21144bab4f11 ("sc16is7xx: Handle modem status lines").
> > > >
> > > > The problem is that the commit 679875d1d880 was incomplete, and it was (unfortunately) completed by integrating it in commit 21144bab4f11 ("sc16is7xx: Handle modem status lines"). The relevant change was only these 5 new lines, burried deeply into the second commit:
> > > Just as you noticed, this was required to support full set of flow control lines on this device.
> > > The commit you're trying to revert was a preparation for it. Disabling has_mctrl will break it.
> > > I kindly suggest to suggest a fix, instead of hurrying a revert, and waiting for a proper fix later.
> >
> > Hi Lech,
> > the [RFC] in the subject was there to discuss about a possible revert, and/or maybe a possible fix that would allow both modes to be supported. I am not hurrying anything and I am certainly not waiting for a later fix, as I very much want to help and maybe submit such a fix myself.
> >
> > But the reality is that commits 679875d1d880/21144bab4f11 broke userspace by forcing GPIOs as modem control lines. I understand that reverting these patches could also potentially break things for applications depending on these patches. I am simply wondering what is the proper course of action here: revert patches and work on a fix to support both modes, or skip revert and work on a fix (my preference)?
> >
> > > > @@ -1353,9 +1452,17 @@ static int sc16is7xx_probe(struct device *dev,
> > > > sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
> > > > SC16IS7XX_EFCR_RXDISABLE_BIT |
> > > > SC16IS7XX_EFCR_TXDISABLE_BIT);
> > > > +
> > > > + /* Use GPIO lines as modem status registers */
> > > > + if (devtype->has_mctrl)
> > > > + sc16is7xx_port_write(&s->p[i].port,
> > > > + SC16IS7XX_IOCONTROL_REG,
> > > > + SC16IS7XX_IOCONTROL_MODEM_BIT);
> > > > +
> > > >
> > > > Therefore, I should also remove these lines if we go forward with a revert of the patch (should I add another tag "Fixes..." in that case?).
> > > >
> > > > And what do you think of my proposal to maybe replace has_mctrl with a device tree property so that both modes can be fully supported?
> > > I think the proper solution here, is not to invent a new device tree property for every single use case.
> > > I would start by looking for other drivers, if, and how they handle similar cases.
> > > For example, imx-serial driver respects "uart-has-rtscts" property, as do a lot of other controllers built into SoC-s.
> > > On the other hand, other devices which can also provide GPIOs, respect "gpio-controller" property.
> >
> > I think that testing the presence of the "uart-has-rtscts" to force GPIOs as modem control lines would make a lot of sense.
> >
> > > According to SC16IS752 datasheet [1], respecting one of those should be enough,
> > > as GPIOs can be enabled in groups of four pins even for dual UART version.
> > > Every group matches a single port, so probably this can be probably selected per UART even on dual-port versions.
> >
> > I am trying to see how we could set "uart-has-rtscts" for only UART channel A or B in the device tree, but cannot find any example or documentation about that. How do you propose to do it?
> >
> > From what I understand, the property "uart-has-rtscts" can be set only for the whole chip (channels A and B)...
>
> After some analysis, I don't think that we should be using the property "uart-has-rtscts". For our chip, this doesn't make sense because RTS/CTS are dedicated pins. also, like I said, this property applies to the whole chip/device, not to indivual A or B channels (like sc16is752).
Hi Hugo,
That's correct, my idea was to analyze what is available and pick the best one, if at all possible.
"gpio-controller" could also be used - in theory - but it isn't a great choice either, because it doesn't allow us to specify, which pin groups should be used as GPIOs.
>
> The way to go would be to define a new DT property similar to "irda-mode-ports" (for the same sc16is7xx driver). Defining a new property named "modem-control-line-ports" would allow us to specify an array that lists the indices of the port that should have shared GPIO lines configured as modem control lines.
>
> I already implemented that as a proof of concept and it works great.
There is nothing wrong per se in adding new device tree property, I'd just like to avoid jumping the gun in inventing one.
After quickly reviewing documentation of available bindings I see that it's most likely unavoidable.
The "modem-control-line-ports" proposal with a mask of ports sounds very reasonable - please share your PoC,
it will be easier to discuss having a concrete example.
The general approach I noticed in other places (for example, the WF8960 audio codec) is setting a register value directly.
This would allow us to control the IOLATCH bit in IOControl register, to make inputs register behave as interrupt flag register,
but I think that if we ever need it, it would be cleaner to set it with a separate boolean property - I'm in favor of modem-control-line-ports.
I think it would be a good idea to include DT and GPIO maintainers and mailing lists as well.
Especially the DT maintainers - they would like to see this property documented. They however, are not concerned with the code changes themselves.
BTW, the commit split between adding has_mctrl property and the rest of implementation warrants some explanation - this was based on my previous patches,
which Tomasz reworked and submitted. The split was kept to split up the changes to minimal, logical chunks, and to maintain correct authorship of the changes.
With kind regards,
Lech
>
> Hugo.
>
>
> > > I'll be more than happy to assist with that.
> > >
> > > >
> > > > Thank you,
> > > > Hugo.
> > > >
> > > [1] https://www.nxp.com/docs/en/data-sheet/SC16IS752_SC16IS762.pdf <https://www.nxp.com/docs/en/data-sheet/SC16IS752_SC16IS762.pdf>
> > >
> > > --
> > > Pozdrawiam/With kind regards,
> > > Lech Perczak
> > >
> > > Sr. Software Engineer
> > > Camlin Technologies Poland Limited Sp. z o.o.
> > > Strzegomska 54,
> > > 53-611 Wroclaw
> > > Tel: (+48) 71 75 000 16
> > > Email: lech.perczak@camlingroup.com
> > > Website: http://www.camlingroup.com <http://www.camlingroup.com>
> > >
> > >
> >
> >
> > --
> > Hugo Villeneuve
> >
>
>
> --
> Hugo Villeneuve
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"
2023-05-17 9:18 ` Lech Perczak
@ 2023-05-17 13:58 ` Hugo Villeneuve
0 siblings, 0 replies; 10+ messages in thread
From: Hugo Villeneuve @ 2023-05-17 13:58 UTC (permalink / raw)
To: Lech Perczak
Cc: Greg Kroah-Hartman, Jiri Slaby, Lech Perczak, Tomasz Moń,
Hugo Villeneuve, linux-serial, linux-kernel,
Krzysztof Drobiński, hugo
On Wed, 17 May 2023 11:18:57 +0200
Lech Perczak <lech.perczak@camlingroup.com> wrote:
>
> W dniu 17.05.2023 o 00:09, Hugo Villeneuve pisze:
> > On Tue, 16 May 2023 11:59:06 -0400
> > Hugo Villeneuve <hugo@hugovil.com> wrote:
> >
> > > On Tue, 16 May 2023 10:50:11 +0200
> > > Lech Perczak <lech.perczak@camlingroup.com> wrote:
> > >
> > > > Hi Hugo,
> > > >
> > > > Please see my answers inline.
> > > >
> > > > W dniu 15.05.2023 o 18:51, Hugo Villeneuve pisze:
> > > > > Hi Greg,
> > > > >
> > > > > On Mon, 15 May 2023 18:20:02 +0200
> > > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > >> On Mon, May 15, 2023 at 12:02:07PM -0400, Hugo Villeneuve wrote:
> > > > >>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > >>>
> > > > >>> This reverts commit 679875d1d8802669590ef4d69b0e7d13207ebd61.
> > > > >>>
> > > > >>> Because of this commit, it is no longer possible to use the 16 GPIO
> > > > >>> lines as dedicated GPIOs on the SC16IS752.
> > > > >>>
> > > > >>> Reverting it makes it work again.
> > > > >>>
> > > > >>> The log message of the original commit states:
> > > > >>> "Export only the GPIOs that are not shared with hardware modem
> > > > >>> control lines"
> > > > >>>
> > > > >>> But there is no explanation as to why this decision was taken to
> > > > >>> permanently set the function of the GPIO lines as modem control
> > > > >>> lines. AFAIK, there is no problem with using these lines as GPIO or modem
> > > > >>> control lines.
> > > > >>>
> > > > >>> Maybe after reverting this commit, we could define a new
> > > > >>> device-tree property named, for example,
> > > > >>> "use-modem-control-lines", so that both options can be supported.
> > > > >>>
> > > > >>> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> > > > >>> lines")
> > > > >> Please do not line-wrap these lines.
> > > > > Ok.
> > > > >
> > > > >> Nor is a blank line needed here.
> > > > > Ok.
> > > > >
> > > > >>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > >>> ---
> > > > >>> drivers/tty/serial/sc16is7xx.c | 14 ++++----------
> > > > >>> 1 file changed, 4 insertions(+), 10 deletions(-)
> > > > >>>
> > > > >>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > > >>> index 5bd98e4316f5..25f1b2f6ec51 100644
> > > > >>> --- a/drivers/tty/serial/sc16is7xx.c
> > > > >>> +++ b/drivers/tty/serial/sc16is7xx.c
> > > > >>> @@ -306,7 +306,6 @@ struct sc16is7xx_devtype {
> > > > >>> char name[10];
> > > > >>> int nr_gpio;
> > > > >>> int nr_uart;
> > > > >>> - int has_mctrl;
> > > > >>> };
> > > > >>>
> > > > >>> #define SC16IS7XX_RECONF_MD (1 << 0)
> > > > >>> @@ -447,35 +446,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
> > > > >>> .name = "SC16IS74X",
> > > > >>> .nr_gpio = 0,
> > > > >>> .nr_uart = 1,
> > > > >>> - .has_mctrl = 0,
> > > > >>> };
> > > > >>>
> > > > >>> static const struct sc16is7xx_devtype sc16is750_devtype = {
> > > > >>> .name = "SC16IS750",
> > > > >>> - .nr_gpio = 4,
> > > > >>> + .nr_gpio = 8,
> > > > >> I think this one line change is all you really need here, right? the
> > > > >> otner changes do nothing in this patch, so you should just create a new
> > > > >> one changing this value. Oh, and this one:
> > > > >>
> > > > >>> .nr_uart = 1,
> > > > >>> - .has_mctrl = 1,
> > > > >>> };
> > > > >>>
> > > > >>> static const struct sc16is7xx_devtype sc16is752_devtype = {
> > > > >>> .name = "SC16IS752",
> > > > >>> - .nr_gpio = 0,
> > > > >>> + .nr_gpio = 8,
> > > > >> right?
> > > > >>
> > > > >> Don't mess with the has_mctrl stuff, that's not relevant here.
> > > > > Sorry, I just noticed that simply reverting commit 679875d1d880 is not sufficient (and will not compile). We must also revert part of commit:
> > > > > 21144bab4f11 ("sc16is7xx: Handle modem status lines").
> > > > >
> > > > > The problem is that the commit 679875d1d880 was incomplete, and it was (unfortunately) completed by integrating it in commit 21144bab4f11 ("sc16is7xx: Handle modem status lines"). The relevant change was only these 5 new lines, burried deeply into the second commit:
> > > > Just as you noticed, this was required to support full set of flow control lines on this device.
> > > > The commit you're trying to revert was a preparation for it. Disabling has_mctrl will break it.
> > > > I kindly suggest to suggest a fix, instead of hurrying a revert, and waiting for a proper fix later.
> > >
> > > Hi Lech,
> > > the [RFC] in the subject was there to discuss about a possible revert, and/or maybe a possible fix that would allow both modes to be supported. I am not hurrying anything and I am certainly not waiting for a later fix, as I very much want to help and maybe submit such a fix myself.
> > >
> > > But the reality is that commits 679875d1d880/21144bab4f11 broke userspace by forcing GPIOs as modem control lines. I understand that reverting these patches could also potentially break things for applications depending on these patches. I am simply wondering what is the proper course of action here: revert patches and work on a fix to support both modes, or skip revert and work on a fix (my preference)?
> > >
> > > > > @@ -1353,9 +1452,17 @@ static int sc16is7xx_probe(struct device *dev,
> > > > > sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
> > > > > SC16IS7XX_EFCR_RXDISABLE_BIT |
> > > > > SC16IS7XX_EFCR_TXDISABLE_BIT);
> > > > > +
> > > > > + /* Use GPIO lines as modem status registers */
> > > > > + if (devtype->has_mctrl)
> > > > > + sc16is7xx_port_write(&s->p[i].port,
> > > > > + SC16IS7XX_IOCONTROL_REG,
> > > > > + SC16IS7XX_IOCONTROL_MODEM_BIT);
> > > > > +
> > > > >
> > > > > Therefore, I should also remove these lines if we go forward with a revert of the patch (should I add another tag "Fixes..." in that case?).
> > > > >
> > > > > And what do you think of my proposal to maybe replace has_mctrl with a device tree property so that both modes can be fully supported?
> > > > I think the proper solution here, is not to invent a new device tree property for every single use case.
> > > > I would start by looking for other drivers, if, and how they handle similar cases.
> > > > For example, imx-serial driver respects "uart-has-rtscts" property, as do a lot of other controllers built into SoC-s.
> > > > On the other hand, other devices which can also provide GPIOs, respect "gpio-controller" property.
> > >
> > > I think that testing the presence of the "uart-has-rtscts" to force GPIOs as modem control lines would make a lot of sense.
> > >
> > > > According to SC16IS752 datasheet [1], respecting one of those should be enough,
> > > > as GPIOs can be enabled in groups of four pins even for dual UART version.
> > > > Every group matches a single port, so probably this can be probably selected per UART even on dual-port versions.
> > >
> > > I am trying to see how we could set "uart-has-rtscts" for only UART channel A or B in the device tree, but cannot find any example or documentation about that. How do you propose to do it?
> > >
> > > From what I understand, the property "uart-has-rtscts" can be set only for the whole chip (channels A and B)...
> >
> > After some analysis, I don't think that we should be using the property "uart-has-rtscts". For our chip, this doesn't make sense because RTS/CTS are dedicated pins. also, like I said, this property applies to the whole chip/device, not to indivual A or B channels (like sc16is752).
>
> Hi Hugo,
>
> That's correct, my idea was to analyze what is available and pick the best one, if at all possible.
> "gpio-controller" could also be used - in theory - but it isn't a great choice either, because it doesn't allow us to specify, which pin groups should be used as GPIOs.
> >
> > The way to go would be to define a new DT property similar to "irda-mode-ports" (for the same sc16is7xx driver). Defining a new property named "modem-control-line-ports" would allow us to specify an array that lists the indices of the port that should have shared GPIO lines configured as modem control lines.
> >
> > I already implemented that as a proof of concept and it works great.
>
> There is nothing wrong per se in adding new device tree property, I'd just like to avoid jumping the gun in inventing one.
> After quickly reviewing documentation of available bindings I see that it's most likely unavoidable.
> The "modem-control-line-ports" proposal with a mask of ports sounds very reasonable - please share your PoC,
> it will be easier to discuss having a concrete example.
>
> The general approach I noticed in other places (for example, the WF8960 audio codec) is setting a register value directly.
> This would allow us to control the IOLATCH bit in IOControl register, to make inputs register behave as interrupt flag register,
> but I think that if we ever need it, it would be cleaner to set it with a separate boolean property - I'm in favor of modem-control-line-ports.
Hi Lech,
I think that control of the IOLATCH bit should be part of a separate discussion (patch) if we want to change it. For the moment, I just want to restore the GPIO function as it was before your patches.
> I think it would be a good idea to include DT and GPIO maintainers and mailing lists as well.
> Especially the DT maintainers - they would like to see this property documented. They however, are not concerned with the code changes themselves.
Ok, I will submit my PoC soon.
Thank you, Hugo.
> BTW, the commit split between adding has_mctrl property and the rest of implementation warrants some explanation - this was based on my previous patches,
> which Tomasz reworked and submitted. The split was kept to split up the changes to minimal, logical chunks, and to maintain correct authorship of the changes.
>
> With kind regards,
> Lech
> >
> > Hugo.
> >
> >
> > > > I'll be more than happy to assist with that.
> > > >
> > > > >
> > > > > Thank you,
> > > > > Hugo.
> > > > >
> > > > [1] https://www.nxp.com/docs/en/data-sheet/SC16IS752_SC16IS762.pdf <https://www.nxp.com/docs/en/data-sheet/SC16IS752_SC16IS762.pdf>
> > > >
> > > > --
> > > > Pozdrawiam/With kind regards,
> > > > Lech Perczak
> > > >
> > > > Sr. Software Engineer
> > > > Camlin Technologies Poland Limited Sp. z o.o.
> > > > Strzegomska 54,
> > > > 53-611 Wroclaw
> > > > Tel: (+48) 71 75 000 16
> > > > Email: lech.perczak@camlingroup.com
> > > > Website: http://www.camlingroup.com <http://www.camlingroup.com>
> > > >
> > > >
> > >
> > >
> > > --
> > > Hugo Villeneuve
> > >
> >
> >
> > --
> > Hugo Villeneuve
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-05-17 13:58 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-15 16:02 [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines" Hugo Villeneuve
2023-05-15 16:20 ` Greg Kroah-Hartman
2023-05-15 16:51 ` Hugo Villeneuve
2023-05-16 8:50 ` Lech Perczak
2023-05-16 15:59 ` Hugo Villeneuve
2023-05-16 22:09 ` Hugo Villeneuve
2023-05-17 9:18 ` Lech Perczak
2023-05-17 13:58 ` Hugo Villeneuve
2023-05-16 5:18 ` kernel test robot
2023-05-16 6:20 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.