| Message ID | 20230602152626.284324-6-hugo@hugovil.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: sc16is7xx: fix GPIO regression and rs485 improvements | expand |
Hi Hugo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 9e87b63ed37e202c77aa17d4112da6ae0c7c097c]
url: https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-fix-broken-port-0-uart-init/20230602-232811
base: 9e87b63ed37e202c77aa17d4112da6ae0c7c097c
patch link: https://lore.kernel.org/r/20230602152626.284324-6-hugo%40hugovil.com
patch subject: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration
config: microblaze-randconfig-s052-20230531 (https://download.01.org/0day-ci/archive/20230603/202306030535.SjzsDJSD-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 12.3.0
reproduce:
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/24626643fe711f447b04a2421ef68e8e8cce86d1
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hugo-Villeneuve/serial-sc16is7xx-fix-broken-port-0-uart-init/20230602-232811
git checkout 24626643fe711f447b04a2421ef68e8e8cce86d1
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze 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>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306030535.SjzsDJSD-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_probe':
>> drivers/tty/serial/sc16is7xx.c:1450:12: warning: variable 'mctrl_mask' set but not used [-Wunused-but-set-variable]
1450 | u8 mctrl_mask;
| ^~~~~~~~~~
vim +/mctrl_mask +1450 drivers/tty/serial/sc16is7xx.c
1443
1444 static int sc16is7xx_probe(struct device *dev,
1445 const struct sc16is7xx_devtype *devtype,
1446 struct regmap *regmap, int irq)
1447 {
1448 unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
1449 unsigned int val;
> 1450 u8 mctrl_mask;
1451 u32 uartclk = 0;
1452 int i, ret;
1453 struct sc16is7xx_port *s;
1454
1455 if (IS_ERR(regmap))
1456 return PTR_ERR(regmap);
1457
1458 /*
1459 * This device does not have an identification register that would
1460 * tell us if we are really connected to the correct device.
1461 * The best we can do is to check if communication is at all possible.
1462 */
1463 ret = regmap_read(regmap,
1464 SC16IS7XX_LSR_REG << SC16IS7XX_REG_SHIFT, &val);
1465 if (ret < 0)
1466 return -EPROBE_DEFER;
1467
1468 /* Alloc port structure */
1469 s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
1470 if (!s) {
1471 dev_err(dev, "Error allocating port structure\n");
1472 return -ENOMEM;
1473 }
1474
1475 /* Always ask for fixed clock rate from a property. */
1476 device_property_read_u32(dev, "clock-frequency", &uartclk);
1477
1478 s->clk = devm_clk_get_optional(dev, NULL);
1479 if (IS_ERR(s->clk))
1480 return PTR_ERR(s->clk);
1481
1482 ret = clk_prepare_enable(s->clk);
1483 if (ret)
1484 return ret;
1485
1486 freq = clk_get_rate(s->clk);
1487 if (freq == 0) {
1488 if (uartclk)
1489 freq = uartclk;
1490 if (pfreq)
1491 freq = *pfreq;
1492 if (freq)
1493 dev_dbg(dev, "Clock frequency: %luHz\n", freq);
1494 else
1495 return -EINVAL;
1496 }
1497
1498 s->regmap = regmap;
1499 s->devtype = devtype;
1500 dev_set_drvdata(dev, s);
1501 mutex_init(&s->efr_lock);
1502
1503 kthread_init_worker(&s->kworker);
1504 s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
1505 "sc16is7xx");
1506 if (IS_ERR(s->kworker_task)) {
1507 ret = PTR_ERR(s->kworker_task);
1508 goto out_clk;
1509 }
1510 sched_set_fifo(s->kworker_task);
1511
1512 /* reset device, purging any pending irq / data */
1513 regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
1514 SC16IS7XX_IOCONTROL_SRESET_BIT);
1515
1516 for (i = 0; i < devtype->nr_uart; ++i) {
1517 s->p[i].line = i;
1518 /* Initialize port data */
1519 s->p[i].port.dev = dev;
1520 s->p[i].port.irq = irq;
1521 s->p[i].port.type = PORT_SC16IS7XX;
1522 s->p[i].port.fifosize = SC16IS7XX_FIFO_SIZE;
1523 s->p[i].port.flags = UPF_FIXED_TYPE | UPF_LOW_LATENCY;
1524 s->p[i].port.iobase = i;
1525 s->p[i].port.membase = (void __iomem *)~0;
1526 s->p[i].port.iotype = UPIO_PORT;
1527 s->p[i].port.uartclk = freq;
1528 s->p[i].port.rs485_config = sc16is7xx_config_rs485;
1529 s->p[i].port.rs485_supported = sc16is7xx_rs485_supported;
1530 s->p[i].port.ops = &sc16is7xx_ops;
1531 s->p[i].old_mctrl = 0;
1532 s->p[i].port.line = sc16is7xx_alloc_line();
1533
1534 if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
1535 ret = -ENOMEM;
1536 goto out_ports;
1537 }
1538
1539 /* Disable all interrupts */
1540 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
1541 /* Disable TX/RX */
1542 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
1543 SC16IS7XX_EFCR_RXDISABLE_BIT |
1544 SC16IS7XX_EFCR_TXDISABLE_BIT);
1545
1546 /* Initialize kthread work structs */
1547 kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
1548 kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
1549 kthread_init_delayed_work(&s->p[i].ms_work, sc16is7xx_ms_proc);
1550 /* Register port */
1551 uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
1552
1553 /* Enable EFR */
1554 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
1555 SC16IS7XX_LCR_CONF_MODE_B);
1556
1557 regcache_cache_bypass(s->regmap, true);
1558
1559 /* Enable write access to enhanced features */
1560 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFR_REG,
1561 SC16IS7XX_EFR_ENABLE_BIT);
1562
1563 regcache_cache_bypass(s->regmap, false);
1564
1565 /* Restore access to general registers */
1566 sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, 0x00);
1567
1568 /* Go to suspend mode */
1569 sc16is7xx_power(&s->p[i].port, 0);
1570 }
1571
1572 if (dev->of_node) {
1573 struct property *prop;
1574 const __be32 *p;
1575 u32 u;
1576
1577 of_property_for_each_u32(dev->of_node, "irda-mode-ports",
1578 prop, p, u)
1579 if (u < devtype->nr_uart)
1580 s->p[u].irda_mode = true;
1581 }
1582
1583 mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
1584
On Sun, Jun 4, 2023 at 10:47 AM Greg KH <gregkh@linuxfoundation.org> wrote: > On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote: ... > > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev) > > This returns what, mctrl? If so, please document that, it doesn't look > obvious. Good suggestion. Because I also stumbled over the returned type. > And as the kernel test robot reported, you do nothing with the > return value so why compute it? It seems that the entire function and respective call has to be moved under #ifdef CONFIG_GPIOLIB. > And you have a real port here, no need to pass in a "raw" struct device, > right?
On Sun, 4 Jun 2023 09:47:44 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote: > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > changed the function of the GPIOs pins to act as modem control > > lines without any possibility of selecting GPIO function. > > > > As a consequence, applications that depends on GPIO lines configured > > by default as GPIO pins no longer work as expected. > > > > Also, the change to select modem control lines function was done only > > for channel A of dual UART variants (752/762). This was not documented > > in the log message. > > > > Allow to specify GPIO or modem control line function in the device > > tree, and for each of the ports (A or B). > > > > Do so by using the new device-tree property named > > "modem-control-line-ports" (property added in separate patch). > > > > When registering GPIO chip controller, mask-out GPIO pins declared as > > modem control lines according to this new "modem-control-line-ports" > > DT property. > > > > Boards that need to have GPIOS configured as modem control lines > > should add that property to their device tree. Here is a list of > > boards using the sc16is7xx driver in their device tree and that may > > need to be modified: > > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts > > mips/boot/dts/ingenic/cu1830-neo.dts > > mips/boot/dts/ingenic/cu1000-neo.dts > > > > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines") > > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines") > > Cc: <stable@vger.kernel.org> # 6.1.x: 35210b22 dt-bindings: sc16is7xx: Add property to change GPIO function > > Cc: <stable@vger.kernel.org> # 6.1.x: 7d61ca47 serial: sc16is7xx: refactor GPIO controller registration > > Cc: <stable@vger.kernel.org> # 6.1.x: 322470ed serial: sc16is7xx: mark IOCONTROL register as volatile > > Cc: <stable@vger.kernel.org> # 6.1.x: a0077362 serial: sc16is7xx: fix broken port 0 uart init > > Cc: <stable@vger.kernel.org> # 6.1.x > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > --- > > drivers/tty/serial/sc16is7xx.c | 103 ++++++++++++++++++++++++++------- > > 1 file changed, 82 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > > index 7d50674d2d0e..edc83f5f6340 100644 > > --- a/drivers/tty/serial/sc16is7xx.c > > +++ b/drivers/tty/serial/sc16is7xx.c > > @@ -236,7 +236,8 @@ > > > > /* IOControl register bits (Only 750/760) */ > > #define SC16IS7XX_IOCONTROL_LATCH_BIT (1 << 0) /* Enable input latching */ > > -#define SC16IS7XX_IOCONTROL_MODEM_BIT (1 << 1) /* Enable GPIO[7:4] as modem pins */ > > +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT (1 << 1) /* Enable GPIO[7:4] as modem A pins */ > > +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT (1 << 2) /* Enable GPIO[3:0] as modem B pins */ > > #define SC16IS7XX_IOCONTROL_SRESET_BIT (1 << 3) /* Software Reset */ > > > > /* EFCR register bits */ > > @@ -301,12 +302,12 @@ > > /* Misc definitions */ > > #define SC16IS7XX_FIFO_SIZE (64) > > #define SC16IS7XX_REG_SHIFT 2 > > +#define SC16IS7XX_GPIOS_PER_BANK 4 > > > > struct sc16is7xx_devtype { > > char name[10]; > > int nr_gpio; > > int nr_uart; > > - int has_mctrl; > > }; > > > > #define SC16IS7XX_RECONF_MD (1 << 0) > > @@ -336,6 +337,7 @@ struct sc16is7xx_port { > > struct clk *clk; > > #ifdef CONFIG_GPIOLIB > > struct gpio_chip gpio; > > + unsigned long gpio_valid_mask; > > #endif > > unsigned char buf[SC16IS7XX_FIFO_SIZE]; > > struct kthread_worker kworker; > > @@ -447,35 +449,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) > > @@ -1350,16 +1347,45 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip, > > return 0; > > } > > > > -static int sc16is7xx_setup_gpio_chip(struct device *dev) > > +static int sc16is7xx_gpio_init_valid_mask(struct gpio_chip *chip, > > + unsigned long *valid_mask, > > + unsigned int ngpios) > > +{ > > + struct sc16is7xx_port *s = gpiochip_get_data(chip); > > + > > + *valid_mask = s->gpio_valid_mask; > > + > > + return 0; > > +} > > + > > +static int sc16is7xx_setup_gpio_chip(struct device *dev, u8 mctrl_mask) > > { > > struct sc16is7xx_port *s = dev_get_drvdata(dev); > > > > if (!s->devtype->nr_gpio) > > return 0; > > > > + switch (mctrl_mask) { > > + case 0: > > + s->gpio_valid_mask = GENMASK(7, 0); > > + break; > > + case SC16IS7XX_IOCONTROL_MODEM_A_BIT: > > + s->gpio_valid_mask = GENMASK(3, 0); > > + break; > > + case SC16IS7XX_IOCONTROL_MODEM_B_BIT: > > + s->gpio_valid_mask = GENMASK(7, 4); > > + break; > > + default: > > + break; > > + } > > + > > + if (s->gpio_valid_mask == 0) > > + return 0; > > + > > s->gpio.owner = THIS_MODULE; > > s->gpio.parent = dev; > > s->gpio.label = dev_name(dev); > > + s->gpio.init_valid_mask = sc16is7xx_gpio_init_valid_mask; > > s->gpio.direction_input = sc16is7xx_gpio_direction_input; > > s->gpio.get = sc16is7xx_gpio_get; > > s->gpio.direction_output = sc16is7xx_gpio_direction_output; > > @@ -1371,6 +1397,44 @@ static int sc16is7xx_setup_gpio_chip(struct device *dev) > > } > > #endif > > > > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev) > > This returns what, mctrl? If so, please document that, it doesn't look > obvious. And as the kernel test robot reported, you do nothing with the > return value so why compute it? Hi Greg, I will the following comment to document return value: /* * Configure ports designated to operate as modem control lines. * Return mask of ports configured as modem control lines. */ static u8 sc16is7xx_setup_mctrl_ports(struct device *dev) The kernel test robot identified a case, when CONFIG_GPIOLIB is not defined, where the value returned by sc16is7xx_setup_mctrl_ports() is not used. But the function sc16is7xx_setup_mctrl_ports() still need to be called to configure the modem status line ports correctly in that case. And if CONFIG_GPIOLIB is defined, the value is definitely used (and needed). Here is what I suggest to silence the warning: mctrl_mask = sc16is7xx_setup_mctrl_ports(dev); #ifdef CONFIG_GPIOLIB ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask); if (ret) goto out_thread; #else (void) mctrl_mask; #endif I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port... > And you have a real port here, no need to pass in a "raw" struct device, > right? The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the struct sc16is7xx_port from it: struct sc16is7xx_port *s = dev_get_drvdata(dev); Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this: static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s) { struct device *dev = &s->p[0].port.dev; But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device... Hugo.
On Sun, Jun 04, 2023 at 01:43:44PM -0400, Hugo Villeneuve wrote: > Here is what I suggest to silence the warning: > > mctrl_mask = sc16is7xx_setup_mctrl_ports(dev); > > #ifdef CONFIG_GPIOLIB > ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask); > if (ret) > goto out_thread; > #else > (void) mctrl_mask; > #endif Eeek, no, please no... First off, please don't put #ifdef in .c files if at all possible. Secondly, that (void) craziness is just that. Rework this to not be an issue some other way please. > I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port... Sure, that sounds best. > > And you have a real port here, no need to pass in a "raw" struct device, > > right? > > The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the > struct sc16is7xx_port from it: > > struct sc16is7xx_port *s = dev_get_drvdata(dev); > > Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this: > > static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s) > { > struct device *dev = &s->p[0].port.dev; > > But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device... You should never need a "raw" struct device for stuff (if so, something is really odd). Except for error messages, but that's not really a big deal, right? Don't pass around struct device in a driver, use the real types as you know you have it and it saves odd casting around and it just doesn't look safe at all to do so. And if you have that crazy s->p.... stuff in multiple places, then perhaps you might want to rethink the structure somehow? Or at the very least, write an inline function to get it when needed. Also, meta comment, you might want to use some \n characters in your emails, your lines are really long :) thanks, greg k-h
On Sun, 4 Jun 2023 20:29:58 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Sun, Jun 04, 2023 at 01:43:44PM -0400, Hugo Villeneuve wrote: > > Here is what I suggest to silence the warning: > > > > mctrl_mask = sc16is7xx_setup_mctrl_ports(dev); > > > > #ifdef CONFIG_GPIOLIB > > ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask); > > if (ret) > > goto out_thread; > > #else > > (void) mctrl_mask; > > #endif > > Eeek, no, please no... > > First off, please don't put #ifdef in .c files if at all possible. Hi Greg, Andy also made a similar comment, but couldn't suggest a valid alternative when I asked him what to do about that. Just as a sidenote, I didn't add those #ifdef, they were already present in the driver in multiple places. What would be your suggestion to get rid of those #ifdef, simply delete them all? If you suggest me what to do, I will be happy to submit a future patch after this series is finalized to clean that aspect. > Secondly, that (void) craziness is just that. Rework this to not be an > issue some other way please. > > > I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port... > > Sure, that sounds best. Ok, I will do that. > > > And you have a real port here, no need to pass in a "raw" struct device, > > > right? > > > > The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the > > struct sc16is7xx_port from it: > > > > struct sc16is7xx_port *s = dev_get_drvdata(dev); > > > > Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this: > > > > static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s) > > { > > struct device *dev = &s->p[0].port.dev; > > > > But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device... > > You should never need a "raw" struct device for stuff (if so, something > is really odd). Except for error messages, but that's not really a big > deal, right? > Don't pass around struct device in a driver, use the real types as you > know you have it and it saves odd casting around and it just doesn't > look safe at all to do so. If you look at the patch, you will see that I need "struct device *dev" at two places in the sc16is7xx_setup_mctrl_ports() function to read the device properties: ... +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev) ... + count = device_property_count_u32(dev,... ... + ret = device_property_read_u32_array(dev, ... I do not understand why this is odd? > And if you have that crazy s->p.... stuff in multiple places, the > perhaps you might want to rethink the structure somehow? Or at the very > least, write an inline function to get it when needed. I am not sure what you mean by that, since again that "crazy" stuff is already used everywhere in this driver? > Also, meta comment, you might want to use some \n characters in your > emails, your lines are really long :) Strange, I use sylpheed as a mail client, and the option "Wrap lines at 72 characters" is enabled by default, but somehow you must also check the box "Wrap on input" for it to work, not very intuitive :) Thanks for pointing that to me. Hugo.
On Sun, 4 Jun 2023 22:31:04 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sun, Jun 4, 2023 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > On Sun, 4 Jun 2023 14:57:31 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Sun, Jun 4, 2023 at 10:47 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote: > > > > > > ... > > > > > > > > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev) > > > > > > > > This returns what, mctrl? If so, please document that, it doesn't look > > > > obvious. > > > > > > Good suggestion. Because I also stumbled over the returned type. > > > > > > > And as the kernel test robot reported, you do nothing with the > > > > return value so why compute it? > > > > > > It seems that the entire function and respective call has to be moved > > > under #ifdef CONFIG_GPIOLIB. > > > > Hi, > > it cannot. See my explanations in response to Greg's comments. > > Then as Greg suggested, store in the structure and make this function > to return an error code (with int), with this amendment you don't need > to add a comment about the returned variable anymore. Hi, Yes, that is what I have done for V8. Simplifies/clean things a lot. Hugo.
On Sun, 4 Jun 2023 19:16:13 -0400 Hugo Villeneuve <hugo@hugovil.com> wrote: > On Sun, 4 Jun 2023 20:29:58 +0200 > Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Sun, Jun 04, 2023 at 01:43:44PM -0400, Hugo Villeneuve wrote: > > > Here is what I suggest to silence the warning: > > > > > > mctrl_mask = sc16is7xx_setup_mctrl_ports(dev); > > > > > > #ifdef CONFIG_GPIOLIB > > > ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask); > > > if (ret) > > > goto out_thread; > > > #else > > > (void) mctrl_mask; > > > #endif > > > > Eeek, no, please no... > > > > First off, please don't put #ifdef in .c files if at all possible. > > Hi Greg, > Andy also made a similar comment, but couldn't suggest a valid > alternative when I asked him what to do about that. > > Just as a sidenote, I didn't add those #ifdef, they were already > present in the driver in multiple places. > > What would be your suggestion to get rid of those #ifdef, simply delete > them all? > > If you suggest me what to do, I will be happy to submit a > future patch after this series is finalized to clean that aspect. > > > > Secondly, that (void) craziness is just that. Rework this to not be an > > issue some other way please. > > > > > I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port... > > > > Sure, that sounds best. > > Ok, I will do that. > > > > > > And you have a real port here, no need to pass in a "raw" struct device, > > > > right? > > > > > > The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the > > > struct sc16is7xx_port from it: > > > > > > struct sc16is7xx_port *s = dev_get_drvdata(dev); > > > > > > Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this: > > > > > > static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s) > > > { > > > struct device *dev = &s->p[0].port.dev; > > > > > > But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device... > > > > You should never need a "raw" struct device for stuff (if so, something > > is really odd). Except for error messages, but that's not really a big > > deal, right? > > > Don't pass around struct device in a driver, use the real types as you > > know you have it and it saves odd casting around and it just doesn't > > look safe at all to do so. > > If you look at the patch, you will see that I need "struct device *dev" > at two places in the sc16is7xx_setup_mctrl_ports() function to read the > device properties: > > ... > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev) > ... > + count = device_property_count_u32(dev,... > ... > + ret = device_property_read_u32_array(dev, > ... > > I do not understand why this is odd? Hi Greg, I finally added a "struct device" member inside "struct sc16is7xx_port" and now I simply pass "struct sc16is7xx_port *s as the sole argument to sc16is7xx_setup_mctrl_ports(). That should take care of your concern I hope, and I will submit a V8 soon with all these changes. Hugo. > > And if you have that crazy s->p.... stuff in multiple places, the > > perhaps you might want to rethink the structure somehow? Or at the very > > least, write an inline function to get it when needed. > > I am not sure what you mean by that, since again that "crazy" stuff is > already used everywhere in this driver? > > > > Also, meta comment, you might want to use some \n characters in your > > emails, your lines are really long :) > > Strange, I use sylpheed as a mail client, and the option "Wrap lines at > 72 characters" is enabled by default, but somehow you must also check > the box "Wrap on input" for it to work, not very intuitive :) Thanks for > pointing that to me. > > Hugo.
On Sun, 4 Jun 2023 22:31:04 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sun, Jun 4, 2023 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > On Sun, 4 Jun 2023 14:57:31 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Sun, Jun 4, 2023 at 10:47 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote: > > > > > > ... > > > > > > > > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev) > > > > > > > > This returns what, mctrl? If so, please document that, it doesn't look > > > > obvious. > > > > > > Good suggestion. Because I also stumbled over the returned type. > > > > > > > And as the kernel test robot reported, you do nothing with the > > > > return value so why compute it? > > > > > > It seems that the entire function and respective call has to be moved > > > under #ifdef CONFIG_GPIOLIB. > > > > Hi, > > it cannot. See my explanations in response to Greg's comments. > > Then as Greg suggested, store in the structure and make this function > to return an error code (with int), with this amendment you don't need > to add a comment about the returned variable anymore. Hi Andy, did you have a chance to look at V8 (sent two weks ago) which fixed all of what we discussed? Thank you, Hugo.
On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > On Sun, 4 Jun 2023 22:31:04 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: ... > did you have a chance to look at V8 (sent two weks ago) which fixed all > of what we discussed? The patch 6 already has my tag, anything specific you want me to do?
On Tue, 20 Jun 2023 18:18:12 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > On Sun, 4 Jun 2023 22:31:04 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > ... > > > did you have a chance to look at V8 (sent two weks ago) which fixed all > > of what we discussed? > > The patch 6 already has my tag, anything specific you want me to do? Hi Andy, I forgot to remove your "Reviewed-by: Andy..." tag before sending V8 since there were some changes involved in patch 6 and I wanted you to review them. Can you confirm if the changes are correct? I also added a new patch "remove obsolete out_thread label". It has no real impact on the code generation itself, but maybe you can review and confirm if tags are ok or not, based on commit message and also additional commit message. Thank you, Hugo.
On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > On Tue, 20 Jun 2023 18:18:12 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > On Sun, 4 Jun 2023 22:31:04 +0300 > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: ... > > > did you have a chance to look at V8 (sent two weks ago) which fixed all > > > of what we discussed? > > > > The patch 6 already has my tag, anything specific you want me to do? > > Hi Andy, > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8 > since there were some changes involved in patch 6 and I wanted you to > review them. Can you confirm if the changes are correct? > > I also added a new patch "remove obsolete out_thread label". It has no > real impact on the code generation itself, but maybe you can review and > confirm if tags are ok or not, based on commit message and also > additional commit message. Both are fine to me.
On Tue, 20 Jun 2023 18:35:48 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > On Tue, 20 Jun 2023 18:18:12 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > On Sun, 4 Jun 2023 22:31:04 +0300 > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > ... > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all > > > > of what we discussed? > > > > > > The patch 6 already has my tag, anything specific you want me to do? > > > > Hi Andy, > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8 > > since there were some changes involved in patch 6 and I wanted you to > > review them. Can you confirm if the changes are correct? > > > > I also added a new patch "remove obsolete out_thread label". It has no > > real impact on the code generation itself, but maybe you can review and > > confirm if tags are ok or not, based on commit message and also > > additional commit message. > > Both are fine to me. Hi, Ok, thank you for reviewing this. I guess now we are good to go with this series if the stable tags and patches order are good after Greg's review? Hugo.
On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > On Tue, 20 Jun 2023 18:35:48 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > On Tue, 20 Jun 2023 18:18:12 +0300 > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > On Sun, 4 Jun 2023 22:31:04 +0300 > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: ... > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all > > > > > of what we discussed? > > > > > > > > The patch 6 already has my tag, anything specific you want me to do? > > > > > > Hi Andy, > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8 > > > since there were some changes involved in patch 6 and I wanted you to > > > review them. Can you confirm if the changes are correct? > > > > > > I also added a new patch "remove obsolete out_thread label". It has no > > > real impact on the code generation itself, but maybe you can review and > > > confirm if tags are ok or not, based on commit message and also > > > additional commit message. > > > > Both are fine to me. > > Hi, > Ok, thank you for reviewing this. > > I guess now we are good to go with this series if the stable tags and > patches order are good after Greg's review? Taking into account that we are at rc7, and even with Fixes tags in your series I think Greg might take this after v6.5-0rc1 is out. It's up to him how to proceed with that. Note, he usually has thousands of patches in backlog, you might need to respin it after the above mentioned rc1.
On Tue, 20 Jun 2023 18:45:51 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > On Tue, 20 Jun 2023 18:35:48 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > On Tue, 20 Jun 2023 18:18:12 +0300 > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300 > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > ... > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all > > > > > > of what we discussed? > > > > > > > > > > The patch 6 already has my tag, anything specific you want me to do? > > > > > > > > Hi Andy, > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8 > > > > since there were some changes involved in patch 6 and I wanted you to > > > > review them. Can you confirm if the changes are correct? > > > > > > > > I also added a new patch "remove obsolete out_thread label". It has no > > > > real impact on the code generation itself, but maybe you can review and > > > > confirm if tags are ok or not, based on commit message and also > > > > additional commit message. > > > > > > Both are fine to me. > > > > Hi, > > Ok, thank you for reviewing this. > > > > I guess now we are good to go with this series if the stable tags and > > patches order are good after Greg's review? > > Taking into account that we are at rc7, and even with Fixes tags in > your series I think Greg might take this after v6.5-0rc1 is out. It's > up to him how to proceed with that. Note, he usually has thousands of > patches in backlog, you might need to respin it after the above > mentioned rc1. Ok, understood. Let's wait then. Thank you. Hugo.
On Tue, 20 Jun 2023 12:16:45 -0400 Hugo Villeneuve <hugo@hugovil.com> wrote: > On Tue, 20 Jun 2023 18:45:51 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > On Tue, 20 Jun 2023 18:35:48 +0300 > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > On Tue, 20 Jun 2023 18:18:12 +0300 > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300 > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > ... > > > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all > > > > > > > of what we discussed? > > > > > > > > > > > > The patch 6 already has my tag, anything specific you want me to do? > > > > > > > > > > Hi Andy, > > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8 > > > > > since there were some changes involved in patch 6 and I wanted you to > > > > > review them. Can you confirm if the changes are correct? > > > > > > > > > > I also added a new patch "remove obsolete out_thread label". It has no > > > > > real impact on the code generation itself, but maybe you can review and > > > > > confirm if tags are ok or not, based on commit message and also > > > > > additional commit message. > > > > > > > > Both are fine to me. > > > > > > Hi, > > > Ok, thank you for reviewing this. > > > > > > I guess now we are good to go with this series if the stable tags and > > > patches order are good after Greg's review? > > > > Taking into account that we are at rc7, and even with Fixes tags in > > your series I think Greg might take this after v6.5-0rc1 is out. It's > > up to him how to proceed with that. Note, he usually has thousands of > > patches in backlog, you might need to respin it after the above > > mentioned rc1. > > Ok, understood. > > Let's wait then. Hi Andy/Greg, we are now at v6.5-rc2 and I still do not see any of our patches in linus or gregkh_tty repos. Is there something missing from my part (or someone else) to go forward with integrating these patches (v8) for v6.5? Thank you, Hugo.
On Wed, Jul 19, 2023 at 02:40:48PM -0400, Hugo Villeneuve wrote: > On Tue, 20 Jun 2023 12:16:45 -0400 > Hugo Villeneuve <hugo@hugovil.com> wrote: > > > On Tue, 20 Jun 2023 18:45:51 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > On Tue, 20 Jun 2023 18:35:48 +0300 > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > On Tue, 20 Jun 2023 18:18:12 +0300 > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300 > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > ... > > > > > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all > > > > > > > > of what we discussed? > > > > > > > > > > > > > > The patch 6 already has my tag, anything specific you want me to do? > > > > > > > > > > > > Hi Andy, > > > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8 > > > > > > since there were some changes involved in patch 6 and I wanted you to > > > > > > review them. Can you confirm if the changes are correct? > > > > > > > > > > > > I also added a new patch "remove obsolete out_thread label". It has no > > > > > > real impact on the code generation itself, but maybe you can review and > > > > > > confirm if tags are ok or not, based on commit message and also > > > > > > additional commit message. > > > > > > > > > > Both are fine to me. > > > > > > > > Hi, > > > > Ok, thank you for reviewing this. > > > > > > > > I guess now we are good to go with this series if the stable tags and > > > > patches order are good after Greg's review? > > > > > > Taking into account that we are at rc7, and even with Fixes tags in > > > your series I think Greg might take this after v6.5-0rc1 is out. It's > > > up to him how to proceed with that. Note, he usually has thousands of > > > patches in backlog, you might need to respin it after the above > > > mentioned rc1. > > > > Ok, understood. > > > > Let's wait then. > > Hi Andy/Greg, > we are now at v6.5-rc2 and I still do not see any of our patches in > linus or gregkh_tty repos. > > Is there something missing from my part (or someone else) to go forward > with integrating these patches (v8) for v6.5? My queue is huge right now, please be patient, I want to have them all handled by the end of next week... You can always help out by reviewing other patches on the mailing list to reduce my review load. thanks, greg k-h
On Wed, Jul 19, 2023 at 09:14:23PM +0200, Greg KH wrote: > On Wed, Jul 19, 2023 at 02:40:48PM -0400, Hugo Villeneuve wrote: > > On Tue, 20 Jun 2023 12:16:45 -0400 > > Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > On Tue, 20 Jun 2023 18:45:51 +0300 > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > On Tue, 20 Jun 2023 18:35:48 +0300 > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > On Tue, 20 Jun 2023 18:18:12 +0300 > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300 > > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > ... > > > > > > > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all > > > > > > > > > of what we discussed? > > > > > > > > > > > > > > > > The patch 6 already has my tag, anything specific you want me to do? > > > > > > > > > > > > > > Hi Andy, > > > > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8 > > > > > > > since there were some changes involved in patch 6 and I wanted you to > > > > > > > review them. Can you confirm if the changes are correct? > > > > > > > > > > > > > > I also added a new patch "remove obsolete out_thread label". It has no > > > > > > > real impact on the code generation itself, but maybe you can review and > > > > > > > confirm if tags are ok or not, based on commit message and also > > > > > > > additional commit message. > > > > > > > > > > > > Both are fine to me. > > > > > > > > > > Hi, > > > > > Ok, thank you for reviewing this. > > > > > > > > > > I guess now we are good to go with this series if the stable tags and > > > > > patches order are good after Greg's review? > > > > > > > > Taking into account that we are at rc7, and even with Fixes tags in > > > > your series I think Greg might take this after v6.5-0rc1 is out. It's > > > > up to him how to proceed with that. Note, he usually has thousands of > > > > patches in backlog, you might need to respin it after the above > > > > mentioned rc1. > > > > > > Ok, understood. > > > > > > Let's wait then. > > > > Hi Andy/Greg, > > we are now at v6.5-rc2 and I still do not see any of our patches in > > linus or gregkh_tty repos. > > > > Is there something missing from my part (or someone else) to go forward > > with integrating these patches (v8) for v6.5? > > My queue is huge right now, please be patient, I want to have them all > handled by the end of next week... > > You can always help out by reviewing other patches on the mailing list > to reduce my review load. Wait, no, this series was superseeded by v8, and in there you said you were going to send a new series. So please, fix it up and send the updated version of the series, this one isn't going to be applied for obvious reasons. thanks, greg k-h
On Thu, 20 Jul 2023 21:38:21 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Wed, Jul 19, 2023 at 09:14:23PM +0200, Greg KH wrote: > > On Wed, Jul 19, 2023 at 02:40:48PM -0400, Hugo Villeneuve wrote: > > > On Tue, 20 Jun 2023 12:16:45 -0400 > > > Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > On Tue, 20 Jun 2023 18:45:51 +0300 > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > On Tue, 20 Jun 2023 18:35:48 +0300 > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > On Tue, 20 Jun 2023 18:18:12 +0300 > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300 > > > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > > > ... > > > > > > > > > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all > > > > > > > > > > of what we discussed? > > > > > > > > > > > > > > > > > > The patch 6 already has my tag, anything specific you want me to do? > > > > > > > > > > > > > > > > Hi Andy, > > > > > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8 > > > > > > > > since there were some changes involved in patch 6 and I wanted you to > > > > > > > > review them. Can you confirm if the changes are correct? > > > > > > > > > > > > > > > > I also added a new patch "remove obsolete out_thread label". It has no > > > > > > > > real impact on the code generation itself, but maybe you can review and > > > > > > > > confirm if tags are ok or not, based on commit message and also > > > > > > > > additional commit message. > > > > > > > > > > > > > > Both are fine to me. > > > > > > > > > > > > Hi, > > > > > > Ok, thank you for reviewing this. > > > > > > > > > > > > I guess now we are good to go with this series if the stable tags and > > > > > > patches order are good after Greg's review? > > > > > > > > > > Taking into account that we are at rc7, and even with Fixes tags in > > > > > your series I think Greg might take this after v6.5-0rc1 is out. It's > > > > > up to him how to proceed with that. Note, he usually has thousands of > > > > > patches in backlog, you might need to respin it after the above > > > > > mentioned rc1. > > > > > > > > Ok, understood. > > > > > > > > Let's wait then. > > > > > > Hi Andy/Greg, > > > we are now at v6.5-rc2 and I still do not see any of our patches in > > > linus or gregkh_tty repos. > > > > > > Is there something missing from my part (or someone else) to go forward > > > with integrating these patches (v8) for v6.5? > > > > My queue is huge right now, please be patient, I want to have them all > > handled by the end of next week... > > > > You can always help out by reviewing other patches on the mailing list > > to reduce my review load. > > Wait, no, this series was superseeded by v8, and in there you said you > were going to send a new series. So please, fix it up and send the > updated version of the series, this one isn't going to be applied for > obvious reasons. Hi Greg, I never said that I would resend another update for this current serie (unless of course if it was to address a new comment). Re-reading that email made me realise that it was maybe not perfectly clear the way I wrote it. What I said was that, once V8 was finally applied and incorporated in the kernel, then I would send a completely new and different serie to address issues/concerns/improvements/suggestions noted during the review of this serie (example: conversion of bindings to YAML and improve DTS node names, etc). We already agreed with some maintainers (ex: Conor Dooley) that it was reasonnable to do so. That is why I asked Andy if we were good to go with V8 and he confirmed that, and that it was now up to you to integrate it if your review was satisfactory. Hope this clears things and we can integrate it soon. Thank you, Hugo.
On Fri, Jul 21, 2023 at 11:25:17AM -0400, Hugo Villeneuve wrote: > On Thu, 20 Jul 2023 21:38:21 +0200 > Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Wed, Jul 19, 2023 at 09:14:23PM +0200, Greg KH wrote: > > > On Wed, Jul 19, 2023 at 02:40:48PM -0400, Hugo Villeneuve wrote: > > > > On Tue, 20 Jun 2023 12:16:45 -0400 > > > > Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > On Tue, 20 Jun 2023 18:45:51 +0300 > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > > > > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > On Tue, 20 Jun 2023 18:35:48 +0300 > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > On Tue, 20 Jun 2023 18:18:12 +0300 > > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300 > > > > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > > > > > ... > > > > > > > > > > > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all > > > > > > > > > > > of what we discussed? > > > > > > > > > > > > > > > > > > > > The patch 6 already has my tag, anything specific you want me to do? > > > > > > > > > > > > > > > > > > Hi Andy, > > > > > > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8 > > > > > > > > > since there were some changes involved in patch 6 and I wanted you to > > > > > > > > > review them. Can you confirm if the changes are correct? > > > > > > > > > > > > > > > > > > I also added a new patch "remove obsolete out_thread label". It has no > > > > > > > > > real impact on the code generation itself, but maybe you can review and > > > > > > > > > confirm if tags are ok or not, based on commit message and also > > > > > > > > > additional commit message. > > > > > > > > > > > > > > > > Both are fine to me. > > > > > > > > > > > > > > Hi, > > > > > > > Ok, thank you for reviewing this. > > > > > > > > > > > > > > I guess now we are good to go with this series if the stable tags and > > > > > > > patches order are good after Greg's review? > > > > > > > > > > > > Taking into account that we are at rc7, and even with Fixes tags in > > > > > > your series I think Greg might take this after v6.5-0rc1 is out. It's > > > > > > up to him how to proceed with that. Note, he usually has thousands of > > > > > > patches in backlog, you might need to respin it after the above > > > > > > mentioned rc1. > > > > > > > > > > Ok, understood. > > > > > > > > > > Let's wait then. > > > > > > > > Hi Andy/Greg, > > > > we are now at v6.5-rc2 and I still do not see any of our patches in > > > > linus or gregkh_tty repos. > > > > > > > > Is there something missing from my part (or someone else) to go forward > > > > with integrating these patches (v8) for v6.5? > > > > > > My queue is huge right now, please be patient, I want to have them all > > > handled by the end of next week... > > > > > > You can always help out by reviewing other patches on the mailing list > > > to reduce my review load. > > > > Wait, no, this series was superseeded by v8, and in there you said you > > were going to send a new series. So please, fix it up and send the > > updated version of the series, this one isn't going to be applied for > > obvious reasons. > > Hi Greg, > I never said that I would resend another update for this current > serie (unless of course if it was to address a new comment). Re-reading > that email made me realise that it was maybe not perfectly clear the > way I wrote it. > > What I said was that, once V8 was finally applied and > incorporated in the kernel, then I would send a completely new and > different serie to address issues/concerns/improvements/suggestions > noted during the review of this serie (example: conversion of bindings > to YAML and improve DTS node names, etc). We already agreed with some > maintainers (ex: Conor Dooley) that it was reasonnable to do so. > > That is why I asked Andy if we were good to go with V8 and he > confirmed that, and that it was now up to you to integrate it if your > review was satisfactory. > > Hope this clears things and we can integrate it soon. I don't have any of your series in my review queue at all, so please resend them. thanks, greg k-h
On Fri, 21 Jul 2023 17:40:18 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Fri, Jul 21, 2023 at 11:25:17AM -0400, Hugo Villeneuve wrote: > > On Thu, 20 Jul 2023 21:38:21 +0200 > > Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > On Wed, Jul 19, 2023 at 09:14:23PM +0200, Greg KH wrote: > > > > On Wed, Jul 19, 2023 at 02:40:48PM -0400, Hugo Villeneuve wrote: > > > > > On Tue, 20 Jun 2023 12:16:45 -0400 > > > > > Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > On Tue, 20 Jun 2023 18:45:51 +0300 > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > > > > > > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > On Tue, 20 Jun 2023 18:35:48 +0300 > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > On Tue, 20 Jun 2023 18:18:12 +0300 > > > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > > > > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300 > > > > > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all > > > > > > > > > > > > of what we discussed? > > > > > > > > > > > > > > > > > > > > > > The patch 6 already has my tag, anything specific you want me to do? > > > > > > > > > > > > > > > > > > > > Hi Andy, > > > > > > > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8 > > > > > > > > > > since there were some changes involved in patch 6 and I wanted you to > > > > > > > > > > review them. Can you confirm if the changes are correct? > > > > > > > > > > > > > > > > > > > > I also added a new patch "remove obsolete out_thread label". It has no > > > > > > > > > > real impact on the code generation itself, but maybe you can review and > > > > > > > > > > confirm if tags are ok or not, based on commit message and also > > > > > > > > > > additional commit message. > > > > > > > > > > > > > > > > > > Both are fine to me. > > > > > > > > > > > > > > > > Hi, > > > > > > > > Ok, thank you for reviewing this. > > > > > > > > > > > > > > > > I guess now we are good to go with this series if the stable tags and > > > > > > > > patches order are good after Greg's review? > > > > > > > > > > > > > > Taking into account that we are at rc7, and even with Fixes tags in > > > > > > > your series I think Greg might take this after v6.5-0rc1 is out. It's > > > > > > > up to him how to proceed with that. Note, he usually has thousands of > > > > > > > patches in backlog, you might need to respin it after the above > > > > > > > mentioned rc1. > > > > > > > > > > > > Ok, understood. > > > > > > > > > > > > Let's wait then. > > > > > > > > > > Hi Andy/Greg, > > > > > we are now at v6.5-rc2 and I still do not see any of our patches in > > > > > linus or gregkh_tty repos. > > > > > > > > > > Is there something missing from my part (or someone else) to go forward > > > > > with integrating these patches (v8) for v6.5? > > > > > > > > My queue is huge right now, please be patient, I want to have them all > > > > handled by the end of next week... > > > > > > > > You can always help out by reviewing other patches on the mailing list > > > > to reduce my review load. > > > > > > Wait, no, this series was superseeded by v8, and in there you said you > > > were going to send a new series. So please, fix it up and send the > > > updated version of the series, this one isn't going to be applied for > > > obvious reasons. > > > > Hi Greg, > > I never said that I would resend another update for this current > > serie (unless of course if it was to address a new comment). Re-reading > > that email made me realise that it was maybe not perfectly clear the > > way I wrote it. > > > > What I said was that, once V8 was finally applied and > > incorporated in the kernel, then I would send a completely new and > > different serie to address issues/concerns/improvements/suggestions > > noted during the review of this serie (example: conversion of bindings > > to YAML and improve DTS node names, etc). We already agreed with some > > maintainers (ex: Conor Dooley) that it was reasonnable to do so. > > > > That is why I asked Andy if we were good to go with V8 and he > > confirmed that, and that it was now up to you to integrate it if your > > review was satisfactory. > > > > Hope this clears things and we can integrate it soon. > > I don't have any of your series in my review queue at all, so please > resend them. OK, I will resend V8 then. Hugo.
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index 7d50674d2d0e..edc83f5f6340 100644 --- a/drivers/tty/serial/sc16is7xx.c +++ b/drivers/tty/serial/sc16is7xx.c @@ -236,7 +236,8 @@ /* IOControl register bits (Only 750/760) */ #define SC16IS7XX_IOCONTROL_LATCH_BIT (1 << 0) /* Enable input latching */ -#define SC16IS7XX_IOCONTROL_MODEM_BIT (1 << 1) /* Enable GPIO[7:4] as modem pins */ +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT (1 << 1) /* Enable GPIO[7:4] as modem A pins */ +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT (1 << 2) /* Enable GPIO[3:0] as modem B pins */ #define SC16IS7XX_IOCONTROL_SRESET_BIT (1 << 3) /* Software Reset */ /* EFCR register bits */ @@ -301,12 +302,12 @@ /* Misc definitions */ #define SC16IS7XX_FIFO_SIZE (64) #define SC16IS7XX_REG_SHIFT 2 +#define SC16IS7XX_GPIOS_PER_BANK 4 struct sc16is7xx_devtype { char name[10]; int nr_gpio; int nr_uart; - int has_mctrl; }; #define SC16IS7XX_RECONF_MD (1 << 0) @@ -336,6 +337,7 @@ struct sc16is7xx_port { struct clk *clk; #ifdef CONFIG_GPIOLIB struct gpio_chip gpio; + unsigned long gpio_valid_mask; #endif unsigned char buf[SC16IS7XX_FIFO_SIZE]; struct kthread_worker kworker; @@ -447,35 +449,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) @@ -1350,16 +1347,45 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip, return 0; } -static int sc16is7xx_setup_gpio_chip(struct device *dev) +static int sc16is7xx_gpio_init_valid_mask(struct gpio_chip *chip, + unsigned long *valid_mask, + unsigned int ngpios) +{ + struct sc16is7xx_port *s = gpiochip_get_data(chip); + + *valid_mask = s->gpio_valid_mask; + + return 0; +} + +static int sc16is7xx_setup_gpio_chip(struct device *dev, u8 mctrl_mask) { struct sc16is7xx_port *s = dev_get_drvdata(dev); if (!s->devtype->nr_gpio) return 0; + switch (mctrl_mask) { + case 0: + s->gpio_valid_mask = GENMASK(7, 0); + break; + case SC16IS7XX_IOCONTROL_MODEM_A_BIT: + s->gpio_valid_mask = GENMASK(3, 0); + break; + case SC16IS7XX_IOCONTROL_MODEM_B_BIT: + s->gpio_valid_mask = GENMASK(7, 4); + break; + default: + break; + } + + if (s->gpio_valid_mask == 0) + return 0; + s->gpio.owner = THIS_MODULE; s->gpio.parent = dev; s->gpio.label = dev_name(dev); + s->gpio.init_valid_mask = sc16is7xx_gpio_init_valid_mask; s->gpio.direction_input = sc16is7xx_gpio_direction_input; s->gpio.get = sc16is7xx_gpio_get; s->gpio.direction_output = sc16is7xx_gpio_direction_output; @@ -1371,6 +1397,44 @@ static int sc16is7xx_setup_gpio_chip(struct device *dev) } #endif +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev) +{ + struct sc16is7xx_port *s = dev_get_drvdata(dev); + int i; + int ret; + int count; + u32 mctrl_port[2]; + u8 mctrl_mask; + + count = device_property_count_u32(dev, "nxp,modem-control-line-ports"); + if (count < 0 || count > ARRAY_SIZE(mctrl_port)) + return 0; + + ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports", + mctrl_port, count); + if (ret) + return 0; + + mctrl_mask = 0; + + for (i = 0; i < count; i++) { + /* Use GPIO lines as modem control lines */ + if (mctrl_port[i] == 0) + mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT; + else if (mctrl_port[i] == 1) + mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT; + } + + if (mctrl_mask) + regmap_update_bits( + s->regmap, + SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT, + SC16IS7XX_IOCONTROL_MODEM_A_BIT | + SC16IS7XX_IOCONTROL_MODEM_B_BIT, mctrl_mask); + + return mctrl_mask; +} + static const struct serial_rs485 sc16is7xx_rs485_supported = { .flags = SER_RS485_ENABLED | SER_RS485_RTS_AFTER_SEND, .delay_rts_before_send = 1, @@ -1383,6 +1447,7 @@ static int sc16is7xx_probe(struct device *dev, { unsigned long freq = 0, *pfreq = dev_get_platdata(dev); unsigned int val; + u8 mctrl_mask; u32 uartclk = 0; int i, ret; struct sc16is7xx_port *s; @@ -1478,12 +1543,6 @@ static int sc16is7xx_probe(struct device *dev, 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); - /* Initialize kthread work structs */ kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc); kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc); @@ -1521,8 +1580,10 @@ static int sc16is7xx_probe(struct device *dev, s->p[u].irda_mode = true; } + mctrl_mask = sc16is7xx_setup_mctrl_ports(dev); + #ifdef CONFIG_GPIOLIB - ret = sc16is7xx_setup_gpio_chip(dev); + ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask); if (ret) goto out_thread; #endif @@ -1547,7 +1608,7 @@ static int sc16is7xx_probe(struct device *dev, return 0; #ifdef CONFIG_GPIOLIB - if (devtype->nr_gpio) + if (s->gpio_valid_mask) gpiochip_remove(&s->gpio); out_thread: @@ -1573,7 +1634,7 @@ static void sc16is7xx_remove(struct device *dev) int i; #ifdef CONFIG_GPIOLIB - if (s->devtype->nr_gpio) + if (s->gpio_valid_mask) gpiochip_remove(&s->gpio); #endif