On Tue, 26 Jan 2016, Josh Wu wrote:
> Hi, Guennadi
>
> 2016-01-25 0:58 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@xxxxxx>:
> > On Mon, 18 Jan 2016, Josh Wu wrote:
> >
> >> In current code, we use a interrupt to check whether ISI reset/disable
> >> action is done. Actually, we also can check ISI SR to check the
> >> reset/disable action by polling, and it is simpler and straight forward.
> >>
> >> So this patch use isi_hw_wait_status() to check the action status. As
> >> the interrupt checking the status is useless, so just remove the interrupt
> >> & completion code.
> >
> > Sorry, I'm not convinced. Switching from interrupt-driven to polling seems
> > counter-productive to me. Why do you want this?
>
> Yes, it is counter-productive If such code is called very frequently.
> But here we only used for reset and disable the hardware.
>
> I think it is more straightforward and simple when we just check
> status register after write the reset or disable the ISI control
> register. Using interrupt here is over designed.
I still don't see how you're gaining from this. If you were writing new
code and you would say: "you know, this path will only be hit rarely, we
don't know whether interrupt would work, but we have here this simple
polling solution, that works" I would understand. But you already have a
technically-superior solution in place! And it works, right? Why change
it? On the one hand it's your hardware and it's up to you which direction
to develop it, and my task is just to help you integrate your work into
the mainline. On the other hand I really don't see any benefits, sorry.
I'd prefer not to do this unless you can explain the benefits.
> for the sake of performance I should use cpu_releax() instead of
> msleep() when waiting for the status bit.
Don't think that's a good idea. cpu_relax() allows the scheduler to switch
over to a different task, but if none is active, it'll come back instead
of going to sleep. Unless of course your event really kicks in very
quickly - much faster than 1ms. But depending on your architecture
implementation, msleep(1) might also be implemented as a busy look, then
cpu_relax() would be better.
Thanks
Guennadi
>
> Best Regards,
> Josh Wu
>
> >
> > Thanks
> > Guennadi
> >
> >>
> >> Signed-off-by: Josh Wu <rainyfeeling@xxxxxxxxx>
> >> ---
> >>
> >> drivers/media/platform/soc_camera/atmel-isi.c | 59 ++++++---------------------
> >> 1 file changed, 13 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> >> index f0508ea..4ddc309 100644
> >> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> >> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> >> @@ -12,7 +12,6 @@
> >> */
> >>
> >> #include <linux/clk.h>
> >> -#include <linux/completion.h>
> >> #include <linux/delay.h>
> >> #include <linux/fs.h>
> >> #include <linux/init.h>
> >> @@ -81,7 +80,6 @@ struct atmel_isi {
> >> struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
> >> bool enable_preview_path;
> >>
> >> - struct completion complete;
> >> /* ISI peripherial clock */
> >> struct clk *pclk;
> >> unsigned int irq;
> >> @@ -281,51 +279,14 @@ static irqreturn_t isi_interrupt(int irq, void *dev_id)
> >> mask = isi_readl(isi, ISI_INTMASK);
> >> pending = status & mask;
> >>
> >> - if (pending & ISI_CTRL_SRST) {
> >> - complete(&isi->complete);
> >> - isi_writel(isi, ISI_INTDIS, ISI_CTRL_SRST);
> >> - ret = IRQ_HANDLED;
> >> - } else if (pending & ISI_CTRL_DIS) {
> >> - complete(&isi->complete);
> >> - isi_writel(isi, ISI_INTDIS, ISI_CTRL_DIS);
> >> - ret = IRQ_HANDLED;
> >> - } else {
> >> - if (likely(pending & ISI_SR_CXFR_DONE) ||
> >> - likely(pending & ISI_SR_PXFR_DONE))
> >> - ret = atmel_isi_handle_streaming(isi);
> >> - }
> >> + if (likely(pending & ISI_SR_CXFR_DONE) ||
> >> + likely(pending & ISI_SR_PXFR_DONE))
> >> + ret = atmel_isi_handle_streaming(isi);
> >>
> >> spin_unlock(&isi->lock);
> >> return ret;
> >> }
> >>
> >> -#define WAIT_ISI_RESET 1
> >> -#define WAIT_ISI_DISABLE 0
> >> -static int atmel_isi_wait_status(struct atmel_isi *isi, int wait_reset)
> >> -{
> >> - unsigned long timeout;
> >> - /*
> >> - * The reset or disable will only succeed if we have a
> >> - * pixel clock from the camera.
> >> - */
> >> - init_completion(&isi->complete);
> >> -
> >> - if (wait_reset) {
> >> - isi_writel(isi, ISI_INTEN, ISI_CTRL_SRST);
> >> - isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
> >> - } else {
> >> - isi_writel(isi, ISI_INTEN, ISI_CTRL_DIS);
> >> - isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> >> - }
> >> -
> >> - timeout = wait_for_completion_timeout(&isi->complete,
> >> - msecs_to_jiffies(500));
> >> - if (timeout == 0)
> >> - return -ETIMEDOUT;
> >> -
> >> - return 0;
> >> -}
> >> -
> >> /* ------------------------------------------------------------------
> >> Videobuf operations
> >> ------------------------------------------------------------------*/
> >> @@ -493,8 +454,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
> >> pm_runtime_get_sync(ici->v4l2_dev.dev);
> >>
> >> /* Reset ISI */
> >> - ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
> >> - if (ret < 0) {
> >> + isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
> >> +
> >> + /* Check Reset status */
> >> + ret = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
> >> + if (ret) {
> >> dev_err(icd->parent, "Reset ISI timed out\n");
> >> pm_runtime_put(ici->v4l2_dev.dev);
> >> return ret;
> >> @@ -549,8 +513,11 @@ static void stop_streaming(struct vb2_queue *vq)
> >> ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
> >>
> >> /* Disable ISI and wait for it is done */
> >> - ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
> >> - if (ret < 0)
> >> + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> >> +
> >> + /* Check Reset status */
> >> + ret = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
> >> + if (ret)
> >> dev_err(icd->parent, "Disable ISI timed out\n");
> >>
> >> pm_runtime_put(ici->v4l2_dev.dev);
> >> --
> >> 1.9.1
> >>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
