From: Jonathan Liu <net147@gmail.com> To: Maxime Ripard <maxime.ripard@free-electrons.com> Cc: David Airlie <airlied@linux.ie>, Chen-Yu Tsai <wens@csie.org>, linux-kernel <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, linux-sunxi <linux-sunxi@googlegroups.com> Subject: Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus Date: Fri, 30 Jun 2017 08:22:13 +1000 [thread overview] Message-ID: <CANwerB3bKWYhTsD7Jh1r6r-+AP_96y5rOtU6mCwpY1_jFZC8yg@mail.gmail.com> (raw) In-Reply-To: <20170629155620.4keqi4cumbtvv63u@flea> Hi Maxime, On 30 June 2017 at 01:56, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote: >> >> + u32 int_status; >> >> + u32 fifo_status; >> >> + /* Read needs empty flag unset, write needs full flag unset */ >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY : >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL; >> >> + int ret; >> >> + >> >> + /* Wait until error or FIFO ready */ >> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG, >> >> + int_status, >> >> + is_err_status(int_status) || >> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag), >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time, >> >> + 100000); >> >> + >> >> + if (is_err_status(int_status)) >> >> + return -EIO; >> >> + if (ret) >> >> + return -ETIMEDOUT; >> > >> > Why not just have >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg, >> > !(reg & flag), 100, 100000); >> > >> > if (ret < 0) >> > if (is_err_status()) >> > return -EIO; >> > return ret; >> > >> > >> >> If I check error status after readl_poll_timeout and there is an error >> (e.g. the I2C address does not have a corresponding device connected >> or nothing connected to HDMI port) it will keep checking the fifo >> status even though error bit is set in the int status and then timeout >> after 100 ms. If it checks the int status register at the same time, >> it will error after 100 nanoseconds. I don't want to introduce >> unnecessary delays considering part of the reason for adding this >> driver to make it more usable for non-standard use cases. > > Well, polling for 100ms doesn't seem great either. What was the > rationale behind that timeout? > When an error occurs one of the error bits will be set in the INT_STATUS register so this is detected very quickly if I check the INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in case the I2C slave does clock stretching in which case the transfer may take longer than the predicted time. > And we can also reverse the check and look at the INT_STATUS > register. The errors will be there, and we can program the threshold > we want in both directions and use the > DDC_FIFO_Request_Interrupt_Status bit. > I did try that when I was doing the v3 patch but I couldn't get it to work as mentioned previously in the v3 patch discussion. I programmed the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl register at the same time as setting FIFO_Address_Clear but the request interrupt status bit did not get updated to the appropriate state that is consistent with the FIFO level and the thresholds. I did try this several times for subsequent patch versions without success. > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Regards, Jonathan
WARNING: multiple messages have this Message-ID (diff)
From: net147@gmail.com (Jonathan Liu) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus Date: Fri, 30 Jun 2017 08:22:13 +1000 [thread overview] Message-ID: <CANwerB3bKWYhTsD7Jh1r6r-+AP_96y5rOtU6mCwpY1_jFZC8yg@mail.gmail.com> (raw) In-Reply-To: <20170629155620.4keqi4cumbtvv63u@flea> Hi Maxime, On 30 June 2017 at 01:56, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote: >> >> + u32 int_status; >> >> + u32 fifo_status; >> >> + /* Read needs empty flag unset, write needs full flag unset */ >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY : >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL; >> >> + int ret; >> >> + >> >> + /* Wait until error or FIFO ready */ >> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG, >> >> + int_status, >> >> + is_err_status(int_status) || >> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag), >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time, >> >> + 100000); >> >> + >> >> + if (is_err_status(int_status)) >> >> + return -EIO; >> >> + if (ret) >> >> + return -ETIMEDOUT; >> > >> > Why not just have >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg, >> > !(reg & flag), 100, 100000); >> > >> > if (ret < 0) >> > if (is_err_status()) >> > return -EIO; >> > return ret; >> > >> > >> >> If I check error status after readl_poll_timeout and there is an error >> (e.g. the I2C address does not have a corresponding device connected >> or nothing connected to HDMI port) it will keep checking the fifo >> status even though error bit is set in the int status and then timeout >> after 100 ms. If it checks the int status register at the same time, >> it will error after 100 nanoseconds. I don't want to introduce >> unnecessary delays considering part of the reason for adding this >> driver to make it more usable for non-standard use cases. > > Well, polling for 100ms doesn't seem great either. What was the > rationale behind that timeout? > When an error occurs one of the error bits will be set in the INT_STATUS register so this is detected very quickly if I check the INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in case the I2C slave does clock stretching in which case the transfer may take longer than the predicted time. > And we can also reverse the check and look at the INT_STATUS > register. The errors will be there, and we can program the threshold > we want in both directions and use the > DDC_FIFO_Request_Interrupt_Status bit. > I did try that when I was doing the v3 patch but I couldn't get it to work as mentioned previously in the v3 patch discussion. I programmed the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl register at the same time as setting FIFO_Address_Clear but the request interrupt status bit did not get updated to the appropriate state that is consistent with the FIFO level and the thresholds. I did try this several times for subsequent patch versions without success. > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Regards, Jonathan
next prev parent reply other threads:[~2017-06-29 22:22 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-06-27 14:36 [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus Jonathan Liu 2017-06-27 14:36 ` Jonathan Liu 2017-06-27 14:36 ` Jonathan Liu 2017-06-28 9:20 ` Maxime Ripard 2017-06-28 9:20 ` Maxime Ripard 2017-06-28 9:20 ` Maxime Ripard 2017-06-28 10:39 ` Jonathan Liu 2017-06-28 10:39 ` Jonathan Liu 2017-06-28 10:39 ` Jonathan Liu 2017-06-29 15:56 ` Maxime Ripard 2017-06-29 15:56 ` Maxime Ripard 2017-06-29 15:56 ` Maxime Ripard 2017-06-29 22:22 ` Jonathan Liu [this message] 2017-06-29 22:22 ` Jonathan Liu 2017-06-30 3:16 ` Chen-Yu Tsai 2017-06-30 3:16 ` Chen-Yu Tsai 2017-06-30 3:16 ` Chen-Yu Tsai 2017-06-30 14:14 ` Jonathan Liu 2017-06-30 14:14 ` Jonathan Liu 2017-06-30 16:01 ` Maxime Ripard 2017-06-30 16:01 ` Maxime Ripard 2017-06-30 16:01 ` Maxime Ripard 2017-06-30 9:34 ` Maxime Ripard 2017-06-30 9:34 ` Maxime Ripard 2017-06-30 9:34 ` Maxime Ripard 2017-06-30 9:58 ` Jonathan Liu 2017-06-30 9:58 ` Jonathan Liu 2017-06-30 9:58 ` Jonathan Liu 2017-07-01 6:29 ` Jonathan Liu 2017-07-01 6:29 ` Jonathan Liu 2017-07-01 6:29 ` Jonathan Liu 2017-06-28 22:06 ` kbuild test robot 2017-06-28 22:06 ` kbuild test robot 2017-06-28 22:06 ` kbuild test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CANwerB3bKWYhTsD7Jh1r6r-+AP_96y5rOtU6mCwpY1_jFZC8yg@mail.gmail.com \ --to=net147@gmail.com \ --cc=airlied@linux.ie \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-sunxi@googlegroups.com \ --cc=maxime.ripard@free-electrons.com \ --cc=wens@csie.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.