| Message ID | 1fd684bcf5ddb0346aad234072f54e976a5210fb.1650816929.git.pisa@cmp.felk.cvut.cz (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: ctucanfd: clenup acoording to the actual rules and documentation linking | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
Hi Pavel, On Mon. 25 Apr. 2022 at 14:11, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote: > This and remove of inline keyword from the local static functions > should make happy all checks in actual versions of the both checkpatch.pl > and patchwork tools. The title and the description say two different things. When looking at the code, it just seemed that you squashed together two different patches: one to remove the inlines and one to remove the debug. I guess you should split it again. > Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz> > --- > drivers/net/can/ctucanfd/ctucanfd_base.c | 33 +++--------------------- > drivers/net/can/ctucanfd/ctucanfd_pci.c | 22 +++++----------- > 2 files changed, 9 insertions(+), 46 deletions(-) > > diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c > index 7a4550f60abb..a1f6d37fca11 100644 > --- a/drivers/net/can/ctucanfd/ctucanfd_base.c > +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c > @@ -133,13 +133,12 @@ static u32 ctucan_read32_be(struct ctucan_priv *priv, > return ioread32be(priv->mem_base + reg); > } > > -static inline void ctucan_write32(struct ctucan_priv *priv, enum ctu_can_fd_can_registers reg, > - u32 val) > +static void ctucan_write32(struct ctucan_priv *priv, enum ctu_can_fd_can_registers reg, u32 val) > { > priv->write_reg(priv, reg, val); > } > > -static inline u32 ctucan_read32(struct ctucan_priv *priv, enum ctu_can_fd_can_registers reg) > +static u32 ctucan_read32(struct ctucan_priv *priv, enum ctu_can_fd_can_registers reg) > { > return priv->read_reg(priv, reg); > } > @@ -179,8 +178,6 @@ static int ctucan_reset(struct net_device *ndev) > struct ctucan_priv *priv = netdev_priv(ndev); > int i = 100; > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > ctucan_write32(priv, CTUCANFD_MODE, REG_MODE_RST); > clear_bit(CTUCANFD_FLAG_RX_FFW_BUFFERED, &priv->drv_flags); > > @@ -266,8 +263,6 @@ static int ctucan_set_bittiming(struct net_device *ndev) > struct ctucan_priv *priv = netdev_priv(ndev); > struct can_bittiming *bt = &priv->can.bittiming; > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > /* Note that bt may be modified here */ > return ctucan_set_btr(ndev, bt, true); > } > @@ -283,8 +278,6 @@ static int ctucan_set_data_bittiming(struct net_device *ndev) > struct ctucan_priv *priv = netdev_priv(ndev); > struct can_bittiming *dbt = &priv->can.data_bittiming; > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > /* Note that dbt may be modified here */ > return ctucan_set_btr(ndev, dbt, false); > } > @@ -302,8 +295,6 @@ static int ctucan_set_secondary_sample_point(struct net_device *ndev) > int ssp_offset = 0; > u32 ssp_cfg = 0; /* No SSP by default */ > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > if (CTU_CAN_FD_ENABLED(priv)) { > netdev_err(ndev, "BUG! Cannot set SSP - CAN is enabled\n"); > return -EPERM; > @@ -390,8 +381,6 @@ static int ctucan_chip_start(struct net_device *ndev) > int err; > struct can_ctrlmode mode; > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > priv->txb_prio = 0x01234567; > priv->txb_head = 0; > priv->txb_tail = 0; > @@ -457,8 +446,6 @@ static int ctucan_do_set_mode(struct net_device *ndev, enum can_mode mode) > { > int ret; > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > switch (mode) { > case CAN_MODE_START: > ret = ctucan_reset(ndev); > @@ -486,7 +473,7 @@ static int ctucan_do_set_mode(struct net_device *ndev, enum can_mode mode) > * > * Return: Status of TXT buffer > */ > -static inline enum ctucan_txtb_status ctucan_get_tx_status(struct ctucan_priv *priv, u8 buf) > +static enum ctucan_txtb_status ctucan_get_tx_status(struct ctucan_priv *priv, u8 buf) > { > u32 tx_status = ctucan_read32(priv, CTUCANFD_TX_STATUS); > enum ctucan_txtb_status status = (tx_status >> (buf * 4)) & 0x7; > @@ -1123,8 +1110,6 @@ static irqreturn_t ctucan_interrupt(int irq, void *dev_id) > u32 imask; > int irq_loops; > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > for (irq_loops = 0; irq_loops < 10000; irq_loops++) { > /* Get the interrupt status */ > isr = ctucan_read32(priv, CTUCANFD_INT_STAT); > @@ -1198,8 +1183,6 @@ static void ctucan_chip_stop(struct net_device *ndev) > u32 mask = 0xffffffff; > u32 mode; > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > /* Disable interrupts and disable CAN */ > ctucan_write32(priv, CTUCANFD_INT_ENA_CLR, mask); > ctucan_write32(priv, CTUCANFD_INT_MASK_SET, mask); > @@ -1222,8 +1205,6 @@ static int ctucan_open(struct net_device *ndev) > struct ctucan_priv *priv = netdev_priv(ndev); > int ret; > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > ret = pm_runtime_get_sync(priv->dev); > if (ret < 0) { > netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", > @@ -1283,8 +1264,6 @@ static int ctucan_close(struct net_device *ndev) > { > struct ctucan_priv *priv = netdev_priv(ndev); > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > netif_stop_queue(ndev); > napi_disable(&priv->napi); > ctucan_chip_stop(ndev); > @@ -1310,8 +1289,6 @@ static int ctucan_get_berr_counter(const struct net_device *ndev, struct can_ber > struct ctucan_priv *priv = netdev_priv(ndev); > int ret; > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > ret = pm_runtime_get_sync(priv->dev); > if (ret < 0) { > netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", __func__, ret); > @@ -1337,8 +1314,6 @@ int ctucan_suspend(struct device *dev) > struct net_device *ndev = dev_get_drvdata(dev); > struct ctucan_priv *priv = netdev_priv(ndev); > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > if (netif_running(ndev)) { > netif_stop_queue(ndev); > netif_device_detach(ndev); > @@ -1355,8 +1330,6 @@ int ctucan_resume(struct device *dev) > struct net_device *ndev = dev_get_drvdata(dev); > struct ctucan_priv *priv = netdev_priv(ndev); > > - ctucan_netdev_dbg(ndev, "%s\n", __func__); > - > priv->can.state = CAN_STATE_ERROR_ACTIVE; > > if (netif_running(ndev)) { > diff --git a/drivers/net/can/ctucanfd/ctucanfd_pci.c b/drivers/net/can/ctucanfd/ctucanfd_pci.c > index c37a42480533..8f2956a8ae43 100644 > --- a/drivers/net/can/ctucanfd/ctucanfd_pci.c > +++ b/drivers/net/can/ctucanfd/ctucanfd_pci.c > @@ -45,14 +45,6 @@ > #define CTUCAN_WITHOUT_CTUCAN_ID 0 > #define CTUCAN_WITH_CTUCAN_ID 1 > > -static bool use_msi = true; > -module_param(use_msi, bool, 0444); > -MODULE_PARM_DESC(use_msi, "PCIe implementation use MSI interrupts. Default: 1 (yes)"); > - > -static bool pci_use_second = true; > -module_param(pci_use_second, bool, 0444); > -MODULE_PARM_DESC(pci_use_second, "Use the second CAN core on PCIe card. Default: 1 (yes)"); > - > struct ctucan_pci_board_data { > void __iomem *bar0_base; > void __iomem *cra_base; > @@ -117,13 +109,11 @@ static int ctucan_pci_probe(struct pci_dev *pdev, > goto err_disable_device; > } > > - if (use_msi) { > - ret = pci_enable_msi(pdev); > - if (!ret) { > - dev_info(dev, "MSI enabled\n"); > - pci_set_master(pdev); > - msi_ok = 1; > - } > + ret = pci_enable_msi(pdev); > + if (!ret) { > + dev_info(dev, "MSI enabled\n"); > + pci_set_master(pdev); > + msi_ok = 1; > } > > dev_info(dev, "ctucan BAR0 0x%08llx 0x%08llx\n", > @@ -184,7 +174,7 @@ static int ctucan_pci_probe(struct pci_dev *pdev, > > core_i++; > > - while (pci_use_second && (core_i < num_cores)) { > + while (core_i < num_cores) { > addr += 0x4000; > ret = ctucan_probe_common(dev, addr, irq, ntxbufs, 100000000, > 0, ctucan_pci_set_drvdata); > -- > 2.20.1 > >
Hello Vincent, On Monday 25 of April 2022 09:48:51 Vincent Mailhol wrote: > On Mon. 25 Apr. 2022 at 14:11, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote: > > This and remove of inline keyword from the local static functions > > should make happy all checks in actual versions of the both checkpatch.pl > > and patchwork tools. > > The title and the description say two different things. > > When looking at the code, it just seemed that you squashed > together two different patches: one to remove the inlines and one > to remove the debug. I guess you should split it again. if you or somebody else confirms that the three lines change worth separate patch I regenerate the series. The changes are not based on third party patches but only on indications reported by static analysis tools. Remove of inline in the local static functions probably does not even change code generation by current compiler generation. Removed debug outputs are under local ifdef disabled by default, so only real change is step down from option to use module parameter to check for possible broken MSI causing the problems on PCIe CTU CAN FD integration. So I thought that single relatively small cleanup patch is less load to maintainers. But I have no strong preference there and will do as confirmed. By the way, what is preference for CC, should the series be sent to linux-kernel and netdev or it is preferred for these local changes to send it only to linux-can to not load others? Same for CC to David Miller. Best wishes, Pavel
On Mon. 25 Apr 2022 at 17:10, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote: > Hello Vincent, > > On Monday 25 of April 2022 09:48:51 Vincent Mailhol wrote: > > On Mon. 25 Apr. 2022 at 14:11, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote: > > > This and remove of inline keyword from the local static functions > > > should make happy all checks in actual versions of the both checkpatch.pl > > > and patchwork tools. > > > > The title and the description say two different things. > > > > When looking at the code, it just seemed that you squashed > > together two different patches: one to remove the inlines and one > > to remove the debug. I guess you should split it again. > > if you or somebody else confirms that the three lines change > worth separate patch I regenerate the series. I was just troubled that the title was saying "remove debug" and that the body was saying "remove inline". I genuinely thought that you inadvertently squashed two different patches together. I just expect the body of the patch to give extended explanations of what is in the title, not to introduce something else, and this regardless of the number of lines being changed. > The changes are not based on third party patches but only > on indications reported by static analysis tools. > Remove of inline in the local static functions probably > does not even change code generation by current compiler > generation. Removed debug outputs are under local ifdef > disabled by default, so only real change is step down from > option to use module parameter to check for possible > broken MSI causing the problems on PCIe CTU CAN FD integration. > So I thought that single relatively small cleanup patch is > less load to maintainers. > > But I have no strong preference there and will do as confirmed. > > By the way, what is preference for CC, should the series > be sent to linux-kernel and netdev or it is preferred for these > local changes to send it only to linux-can to not load others? > Same for CC to David Miller. I used to include them in the past because of get_maitainer.pl. But Oliver pointed out that it is not necessary. Now, I just sent it to linux-can and Marc (and maybe some driver maintainers when relevant). > Best wishes, > > Pavel > -- > Pavel Pisa > phone: +420 603531357 > e-mail: pisa@cmp.felk.cvut.cz > Department of Control Engineering FEE CVUT > Karlovo namesti 13, 121 35, Prague 2 > university: http://control.fel.cvut.cz/ > personal: http://cmp.felk.cvut.cz/~pisa > projects: https://www.openhub.net/accounts/ppisa > CAN related:http://canbus.pages.fel.cvut.cz/ > Open Technologies Research Education and Exchange Services > https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home >
diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c index 7a4550f60abb..a1f6d37fca11 100644 --- a/drivers/net/can/ctucanfd/ctucanfd_base.c +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c @@ -133,13 +133,12 @@ static u32 ctucan_read32_be(struct ctucan_priv *priv, return ioread32be(priv->mem_base + reg); } -static inline void ctucan_write32(struct ctucan_priv *priv, enum ctu_can_fd_can_registers reg, - u32 val) +static void ctucan_write32(struct ctucan_priv *priv, enum ctu_can_fd_can_registers reg, u32 val) { priv->write_reg(priv, reg, val); } -static inline u32 ctucan_read32(struct ctucan_priv *priv, enum ctu_can_fd_can_registers reg) +static u32 ctucan_read32(struct ctucan_priv *priv, enum ctu_can_fd_can_registers reg) { return priv->read_reg(priv, reg); } @@ -179,8 +178,6 @@ static int ctucan_reset(struct net_device *ndev) struct ctucan_priv *priv = netdev_priv(ndev); int i = 100; - ctucan_netdev_dbg(ndev, "%s\n", __func__); - ctucan_write32(priv, CTUCANFD_MODE, REG_MODE_RST); clear_bit(CTUCANFD_FLAG_RX_FFW_BUFFERED, &priv->drv_flags); @@ -266,8 +263,6 @@ static int ctucan_set_bittiming(struct net_device *ndev) struct ctucan_priv *priv = netdev_priv(ndev); struct can_bittiming *bt = &priv->can.bittiming; - ctucan_netdev_dbg(ndev, "%s\n", __func__); - /* Note that bt may be modified here */ return ctucan_set_btr(ndev, bt, true); } @@ -283,8 +278,6 @@ static int ctucan_set_data_bittiming(struct net_device *ndev) struct ctucan_priv *priv = netdev_priv(ndev); struct can_bittiming *dbt = &priv->can.data_bittiming; - ctucan_netdev_dbg(ndev, "%s\n", __func__); - /* Note that dbt may be modified here */ return ctucan_set_btr(ndev, dbt, false); } @@ -302,8 +295,6 @@ static int ctucan_set_secondary_sample_point(struct net_device *ndev) int ssp_offset = 0; u32 ssp_cfg = 0; /* No SSP by default */ - ctucan_netdev_dbg(ndev, "%s\n", __func__); - if (CTU_CAN_FD_ENABLED(priv)) { netdev_err(ndev, "BUG! Cannot set SSP - CAN is enabled\n"); return -EPERM; @@ -390,8 +381,6 @@ static int ctucan_chip_start(struct net_device *ndev) int err; struct can_ctrlmode mode; - ctucan_netdev_dbg(ndev, "%s\n", __func__); - priv->txb_prio = 0x01234567; priv->txb_head = 0; priv->txb_tail = 0; @@ -457,8 +446,6 @@ static int ctucan_do_set_mode(struct net_device *ndev, enum can_mode mode) { int ret; - ctucan_netdev_dbg(ndev, "%s\n", __func__); - switch (mode) { case CAN_MODE_START: ret = ctucan_reset(ndev); @@ -486,7 +473,7 @@ static int ctucan_do_set_mode(struct net_device *ndev, enum can_mode mode) * * Return: Status of TXT buffer */ -static inline enum ctucan_txtb_status ctucan_get_tx_status(struct ctucan_priv *priv, u8 buf) +static enum ctucan_txtb_status ctucan_get_tx_status(struct ctucan_priv *priv, u8 buf) { u32 tx_status = ctucan_read32(priv, CTUCANFD_TX_STATUS); enum ctucan_txtb_status status = (tx_status >> (buf * 4)) & 0x7; @@ -1123,8 +1110,6 @@ static irqreturn_t ctucan_interrupt(int irq, void *dev_id) u32 imask; int irq_loops; - ctucan_netdev_dbg(ndev, "%s\n", __func__); - for (irq_loops = 0; irq_loops < 10000; irq_loops++) { /* Get the interrupt status */ isr = ctucan_read32(priv, CTUCANFD_INT_STAT); @@ -1198,8 +1183,6 @@ static void ctucan_chip_stop(struct net_device *ndev) u32 mask = 0xffffffff; u32 mode; - ctucan_netdev_dbg(ndev, "%s\n", __func__); - /* Disable interrupts and disable CAN */ ctucan_write32(priv, CTUCANFD_INT_ENA_CLR, mask); ctucan_write32(priv, CTUCANFD_INT_MASK_SET, mask); @@ -1222,8 +1205,6 @@ static int ctucan_open(struct net_device *ndev) struct ctucan_priv *priv = netdev_priv(ndev); int ret; - ctucan_netdev_dbg(ndev, "%s\n", __func__); - ret = pm_runtime_get_sync(priv->dev); if (ret < 0) { netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", @@ -1283,8 +1264,6 @@ static int ctucan_close(struct net_device *ndev) { struct ctucan_priv *priv = netdev_priv(ndev); - ctucan_netdev_dbg(ndev, "%s\n", __func__); - netif_stop_queue(ndev); napi_disable(&priv->napi); ctucan_chip_stop(ndev); @@ -1310,8 +1289,6 @@ static int ctucan_get_berr_counter(const struct net_device *ndev, struct can_ber struct ctucan_priv *priv = netdev_priv(ndev); int ret; - ctucan_netdev_dbg(ndev, "%s\n", __func__); - ret = pm_runtime_get_sync(priv->dev); if (ret < 0) { netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", __func__, ret); @@ -1337,8 +1314,6 @@ int ctucan_suspend(struct device *dev) struct net_device *ndev = dev_get_drvdata(dev); struct ctucan_priv *priv = netdev_priv(ndev); - ctucan_netdev_dbg(ndev, "%s\n", __func__); - if (netif_running(ndev)) { netif_stop_queue(ndev); netif_device_detach(ndev); @@ -1355,8 +1330,6 @@ int ctucan_resume(struct device *dev) struct net_device *ndev = dev_get_drvdata(dev); struct ctucan_priv *priv = netdev_priv(ndev); - ctucan_netdev_dbg(ndev, "%s\n", __func__); - priv->can.state = CAN_STATE_ERROR_ACTIVE; if (netif_running(ndev)) { diff --git a/drivers/net/can/ctucanfd/ctucanfd_pci.c b/drivers/net/can/ctucanfd/ctucanfd_pci.c index c37a42480533..8f2956a8ae43 100644 --- a/drivers/net/can/ctucanfd/ctucanfd_pci.c +++ b/drivers/net/can/ctucanfd/ctucanfd_pci.c @@ -45,14 +45,6 @@ #define CTUCAN_WITHOUT_CTUCAN_ID 0 #define CTUCAN_WITH_CTUCAN_ID 1 -static bool use_msi = true; -module_param(use_msi, bool, 0444); -MODULE_PARM_DESC(use_msi, "PCIe implementation use MSI interrupts. Default: 1 (yes)"); - -static bool pci_use_second = true; -module_param(pci_use_second, bool, 0444); -MODULE_PARM_DESC(pci_use_second, "Use the second CAN core on PCIe card. Default: 1 (yes)"); - struct ctucan_pci_board_data { void __iomem *bar0_base; void __iomem *cra_base; @@ -117,13 +109,11 @@ static int ctucan_pci_probe(struct pci_dev *pdev, goto err_disable_device; } - if (use_msi) { - ret = pci_enable_msi(pdev); - if (!ret) { - dev_info(dev, "MSI enabled\n"); - pci_set_master(pdev); - msi_ok = 1; - } + ret = pci_enable_msi(pdev); + if (!ret) { + dev_info(dev, "MSI enabled\n"); + pci_set_master(pdev); + msi_ok = 1; } dev_info(dev, "ctucan BAR0 0x%08llx 0x%08llx\n", @@ -184,7 +174,7 @@ static int ctucan_pci_probe(struct pci_dev *pdev, core_i++; - while (pci_use_second && (core_i < num_cores)) { + while (core_i < num_cores) { addr += 0x4000; ret = ctucan_probe_common(dev, addr, irq, ntxbufs, 100000000, 0, ctucan_pci_set_drvdata);
This and remove of inline keyword from the local static functions should make happy all checks in actual versions of the both checkpatch.pl and patchwork tools. Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz> --- drivers/net/can/ctucanfd/ctucanfd_base.c | 33 +++--------------------- drivers/net/can/ctucanfd/ctucanfd_pci.c | 22 +++++----------- 2 files changed, 9 insertions(+), 46 deletions(-)