| Message ID | 20200814163017.35001-6-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/7] media: ipu3-cio2: Simplify cleanup code | expand |
On 8/15/20 12:30 AM, Andy Shevchenko wrote: > We may use special helper macro to poll IO till condition or timeout occurs. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index f4175dc1501a..5f8ff91dbf09 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -14,6 +14,7 @@ > > #include <linux/delay.h> > #include <linux/interrupt.h> > +#include <linux/iopoll.h> > #include <linux/module.h> > #include <linux/pci.h> > #include <linux/pfn.h> > @@ -509,7 +510,10 @@ static int cio2_hw_init(struct cio2_device *cio2, struct cio2_queue *q) > static void cio2_hw_exit(struct cio2_device *cio2, struct cio2_queue *q) > { > void __iomem *base = cio2->base; > - unsigned int i, maxloops = 1000; > + void __iomem *dma = base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN); > + unsigned int i; > + u32 value; > + int ret; > > /* Disable CSI receiver and MIPI backend devices */ > writel(0, q->csi_rx_base + CIO2_REG_IRQCTRL_MASK); > @@ -518,14 +522,9 @@ static void cio2_hw_exit(struct cio2_device *cio2, struct cio2_queue *q) > writel(0, q->csi_rx_base + CIO2_REG_MIPIBE_ENABLE); > > /* Halt DMA */ > - writel(0, base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN)); > - do { > - if (readl(base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN)) & > - CIO2_CDMAC0_DMA_HALTED) > - break; > - usleep_range(1000, 2000); > - } while (--maxloops); > - if (!maxloops) > + writel(0, dma); > + ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000); This line is too long, need a break, others look good for me. > + if (ret) > dev_err(&cio2->pci_dev->dev, > "DMA %i can not be halted\n", CIO2_DMA_CHAN); > >
On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote: > On 8/15/20 12:30 AM, Andy Shevchenko wrote: > > We may use special helper macro to poll IO till condition or timeout occurs. > > + ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000); > > This line is too long, need a break, others look good for me. checkpatch doesn't complain, but if you insist, I'll split it in v2.
On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote: > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote: > > On 8/15/20 12:30 AM, Andy Shevchenko wrote: > > > We may use special helper macro to poll IO till condition or timeout occurs. > > > > + ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000); > > > > This line is too long, need a break, others look good for me. > > checkpatch doesn't complain, but if you insist, I'll split it in v2. The coding style hasn't changed, it's just the checkpatch.pl warning that has.
On Mon, Aug 17, 2020 at 02:13:16PM +0300, Sakari Ailus wrote: > On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote: > > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote: > > > On 8/15/20 12:30 AM, Andy Shevchenko wrote: > > > > We may use special helper macro to poll IO till condition or timeout occurs. > > > > > > + ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000); > > > > > > This line is too long, need a break, others look good for me. > > > > checkpatch doesn't complain, but if you insist, I'll split it in v2. > > The coding style hasn't changed, it's just the checkpatch.pl warning that > has. Joe, it seems we have inconsistency now between checkpatch and coding style. Shouldn't we revert 100 limit warning to 80?
On Mon, Aug 17, 2020 at 02:20:06PM +0300, Andy Shevchenko wrote: > On Mon, Aug 17, 2020 at 02:13:16PM +0300, Sakari Ailus wrote: > > On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote: > > > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote: > > > > On 8/15/20 12:30 AM, Andy Shevchenko wrote: > > > > > We may use special helper macro to poll IO till condition or timeout occurs. > > > > > > > > + ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000); > > > > > > > > This line is too long, need a break, others look good for me. > > > > > > checkpatch doesn't complain, but if you insist, I'll split it in v2. > > > > The coding style hasn't changed, it's just the checkpatch.pl warning that > > has. > > Joe, it seems we have inconsistency now between checkpatch and coding style. > Shouldn't we revert 100 limit warning to 80? There are sometimes genuine reasons for having longer lines than 80, and depending on the code, that happens more often in some places than elsewhere. This tended to generate lots of checkpatch.pl warnings in the past. While I didn't see the patch removing the 80 chars per line limit until it made the news, I think it was a quite reasonable compromise.
On Mon, Aug 17, 2020 at 02:27:47PM +0300, Sakari Ailus wrote: > On Mon, Aug 17, 2020 at 02:20:06PM +0300, Andy Shevchenko wrote: > > On Mon, Aug 17, 2020 at 02:13:16PM +0300, Sakari Ailus wrote: > > > On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote: > > > > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote: > > > > > On 8/15/20 12:30 AM, Andy Shevchenko wrote: > > > > > > We may use special helper macro to poll IO till condition or timeout occurs. > > > > > > > > > > + ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000); > > > > > > > > > > This line is too long, need a break, others look good for me. > > > > > > > > checkpatch doesn't complain, but if you insist, I'll split it in v2. > > > > > > The coding style hasn't changed, it's just the checkpatch.pl warning that > > > has. > > > > Joe, it seems we have inconsistency now between checkpatch and coding style. > > Shouldn't we revert 100 limit warning to 80? > > There are sometimes genuine reasons for having longer lines than 80, and > depending on the code, that happens more often in some places than > elsewhere. This tended to generate lots of checkpatch.pl warnings in the > past. > > While I didn't see the patch removing the 80 chars per line limit until it > made the news, I think it was a quite reasonable compromise. But doesn't it make harder life for reviewers like you? You have to keep in mind all these inconsistencies and rule either way. That said, we either would fix the doc, or revert the checkpatch change. Jonathan, what is your opinion?
On Mon, Aug 17, 2020 at 02:33:22PM +0300, Andy Shevchenko wrote: > On Mon, Aug 17, 2020 at 02:27:47PM +0300, Sakari Ailus wrote: > > On Mon, Aug 17, 2020 at 02:20:06PM +0300, Andy Shevchenko wrote: > > > On Mon, Aug 17, 2020 at 02:13:16PM +0300, Sakari Ailus wrote: > > > > On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote: > > > > > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote: > > > > > > On 8/15/20 12:30 AM, Andy Shevchenko wrote: > > > > > > > We may use special helper macro to poll IO till condition or timeout occurs. > > > > > > > > > > > > + ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000); > > > > > > > > > > > > This line is too long, need a break, others look good for me. > > > > > > > > > > checkpatch doesn't complain, but if you insist, I'll split it in v2. > > > > > > > > The coding style hasn't changed, it's just the checkpatch.pl warning that > > > > has. > > > > > > Joe, it seems we have inconsistency now between checkpatch and coding style. > > > Shouldn't we revert 100 limit warning to 80? > > > > There are sometimes genuine reasons for having longer lines than 80, and > > depending on the code, that happens more often in some places than > > elsewhere. This tended to generate lots of checkpatch.pl warnings in the > > past. > > > > While I didn't see the patch removing the 80 chars per line limit until it > > made the news, I think it was a quite reasonable compromise. > > But doesn't it make harder life for reviewers like you? You have to keep in > mind all these inconsistencies and rule either way. > > That said, we either would fix the doc, or revert the checkpatch change. I don't think the documentation should be changed. There *are* reasons for the 80-column limit. Although the exact number can be connected to the width of traditional text terminals, using very long lines makes reading text harder. IOW, there's a connection to the same reason why wide newspaper articles are split into several columns. Would it be reasonable if checkpatch.pl warned about exceeding 80-column limits if such lines are rare or nonexistent in the same file?
On Mon, Aug 17, 2020 at 02:55:38PM +0300, Sakari Ailus wrote: > On Mon, Aug 17, 2020 at 02:33:22PM +0300, Andy Shevchenko wrote: > > On Mon, Aug 17, 2020 at 02:27:47PM +0300, Sakari Ailus wrote: > > > On Mon, Aug 17, 2020 at 02:20:06PM +0300, Andy Shevchenko wrote: > > > > On Mon, Aug 17, 2020 at 02:13:16PM +0300, Sakari Ailus wrote: > > > > > On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote: > > > > > > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote: > > > > > > > On 8/15/20 12:30 AM, Andy Shevchenko wrote: > > > > > > > > We may use special helper macro to poll IO till condition or timeout occurs. > > > > > > > > > > > > > > + ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000); > > > > > > > > > > > > > > This line is too long, need a break, others look good for me. > > > > > > > > > > > > checkpatch doesn't complain, but if you insist, I'll split it in v2. > > > > > > > > > > The coding style hasn't changed, it's just the checkpatch.pl warning that > > > > > has. > > > > > > > > Joe, it seems we have inconsistency now between checkpatch and coding style. > > > > Shouldn't we revert 100 limit warning to 80? > > > > > > There are sometimes genuine reasons for having longer lines than 80, and > > > depending on the code, that happens more often in some places than > > > elsewhere. This tended to generate lots of checkpatch.pl warnings in the > > > past. > > > > > > While I didn't see the patch removing the 80 chars per line limit until it > > > made the news, I think it was a quite reasonable compromise. > > > > But doesn't it make harder life for reviewers like you? You have to keep in > > mind all these inconsistencies and rule either way. > > > > That said, we either would fix the doc, or revert the checkpatch change. > > I don't think the documentation should be changed. There *are* reasons for > the 80-column limit. Although the exact number can be connected to the > width of traditional text terminals, using very long lines makes reading > text harder. IOW, there's a connection to the same reason why wide > newspaper articles are split into several columns. > Would it be reasonable if checkpatch.pl warned about exceeding 80-column > limits if such lines are rare or nonexistent in the same file? I don't think so. If somebody decides to *gradually* move to 100 limit in the driver, it will be painful with all these new old warnings.
On Mon, Aug 17, 2020 at 03:29:11PM +0300, Andy Shevchenko wrote: > On Mon, Aug 17, 2020 at 02:55:38PM +0300, Sakari Ailus wrote: > > On Mon, Aug 17, 2020 at 02:33:22PM +0300, Andy Shevchenko wrote: > > > On Mon, Aug 17, 2020 at 02:27:47PM +0300, Sakari Ailus wrote: > > > > On Mon, Aug 17, 2020 at 02:20:06PM +0300, Andy Shevchenko wrote: > > > > > On Mon, Aug 17, 2020 at 02:13:16PM +0300, Sakari Ailus wrote: > > > > > > On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote: > > > > > > > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote: > > > > > > > > On 8/15/20 12:30 AM, Andy Shevchenko wrote: > > > > > > > > > We may use special helper macro to poll IO till condition or timeout occurs. > > > > > > > > > > > > > > > > + ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000); > > > > > > > > > > > > > > > > This line is too long, need a break, others look good for me. > > > > > > > > > > > > > > checkpatch doesn't complain, but if you insist, I'll split it in v2. > > > > > > > > > > > > The coding style hasn't changed, it's just the checkpatch.pl warning that > > > > > > has. > > > > > > > > > > Joe, it seems we have inconsistency now between checkpatch and coding style. > > > > > Shouldn't we revert 100 limit warning to 80? > > > > > > > > There are sometimes genuine reasons for having longer lines than 80, and > > > > depending on the code, that happens more often in some places than > > > > elsewhere. This tended to generate lots of checkpatch.pl warnings in the > > > > past. > > > > > > > > While I didn't see the patch removing the 80 chars per line limit until it > > > > made the news, I think it was a quite reasonable compromise. > > > > > > But doesn't it make harder life for reviewers like you? You have to keep in > > > mind all these inconsistencies and rule either way. > > > > > > That said, we either would fix the doc, or revert the checkpatch change. > > > > I don't think the documentation should be changed. There *are* reasons for > > the 80-column limit. Although the exact number can be connected to the > > width of traditional text terminals, using very long lines makes reading > > text harder. IOW, there's a connection to the same reason why wide > > newspaper articles are split into several columns. > > > Would it be reasonable if checkpatch.pl warned about exceeding 80-column > > limits if such lines are rare or nonexistent in the same file? > > I don't think so. If somebody decides to *gradually* move to 100 limit in the > driver, it will be painful with all these new old warnings. There would be no more such warnings than with the 80-column limit. I'd also question whether this is a realistic example. If a file was fine without long lines in the past, it's unlikely a sudden and sustained need for having them arises. I think common reasons for those long lines are e.g. referring to definitions from standards as-is or IOCTL command definitions. Or flags used on various interfaces to keep them descriptive. All either exist in given files or do not. That generally doesn't change over the lifetime of the said files.
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index f4175dc1501a..5f8ff91dbf09 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -14,6 +14,7 @@ #include <linux/delay.h> #include <linux/interrupt.h> +#include <linux/iopoll.h> #include <linux/module.h> #include <linux/pci.h> #include <linux/pfn.h> @@ -509,7 +510,10 @@ static int cio2_hw_init(struct cio2_device *cio2, struct cio2_queue *q) static void cio2_hw_exit(struct cio2_device *cio2, struct cio2_queue *q) { void __iomem *base = cio2->base; - unsigned int i, maxloops = 1000; + void __iomem *dma = base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN); + unsigned int i; + u32 value; + int ret; /* Disable CSI receiver and MIPI backend devices */ writel(0, q->csi_rx_base + CIO2_REG_IRQCTRL_MASK); @@ -518,14 +522,9 @@ static void cio2_hw_exit(struct cio2_device *cio2, struct cio2_queue *q) writel(0, q->csi_rx_base + CIO2_REG_MIPIBE_ENABLE); /* Halt DMA */ - writel(0, base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN)); - do { - if (readl(base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN)) & - CIO2_CDMAC0_DMA_HALTED) - break; - usleep_range(1000, 2000); - } while (--maxloops); - if (!maxloops) + writel(0, dma); + ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000); + if (ret) dev_err(&cio2->pci_dev->dev, "DMA %i can not be halted\n", CIO2_DMA_CHAN);
We may use special helper macro to poll IO till condition or timeout occurs. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/media/pci/intel/ipu3/ipu3-cio2.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)