From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752850AbdF2P4t (ORCPT ); Thu, 29 Jun 2017 11:56:49 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:40861 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752454AbdF2P4c (ORCPT ); Thu, 29 Jun 2017 11:56:32 -0400 Date: Thu, 29 Jun 2017 17:56:20 +0200 From: Maxime Ripard To: Jonathan Liu Cc: David Airlie , Chen-Yu Tsai , linux-kernel , dri-devel , linux-arm-kernel , linux-sunxi Subject: Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus Message-ID: <20170629155620.4keqi4cumbtvv63u@flea> References: <20170627143652.13075-1-net147@gmail.com> <20170628092024.ejxy6itqj3hx6yew@flea> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="7pwavh3sysogazgv" Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --7pwavh3sysogazgv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 =3D read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY : > >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL; > >> + int ret; > >> + > >> + /* Wait until error or FIFO ready */ > >> + ret =3D readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATU= S_REG, > >> + int_status, > >> + is_err_status(int_status) || > >> + is_fifo_flag_unset(hdmi, &fifo_status, = flag), > >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * by= te_time, > >> + 100000); > >> + > >> + if (is_err_status(int_status)) > >> + return -EIO; > >> + if (ret) > >> + return -ETIMEDOUT; > > > > Why not just have > > ret =3D 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; > > > > >=20 > 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? 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. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --7pwavh3sysogazgv Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJZVSMjAAoJEBx+YmzsjxAgMOkP/01T/9wp7QfdiB6xrTTA6Etb YWETWDgE9mfHIIyE+BpinSmyRHChdX4c91NK1TY6cEPNcQ+Ej1B/8liDB0kiFnnN RuRNkil87vR36OwgPryh13+GtykfpBrLOfo8YTFVwqzzblAgLvDziUC28GTnyCXw WjIVMiPXxKympFkmdK47RNlYSWMcvW7QHrNuy6j8cVfm1PR+SxuUOkT1RuxcMt76 5Zsw79FPsTSvqr2jt6aJO+vUnENeSJpdBfcnTiKTD7YrrYZ+fXUueH7p4Hi1a+H9 1XOTauMn4sku/AnMP4mZsfXMIbCqWF6jCao0T7HFDbOTa0xc1R7c1g1od2viDipj 0/qwoc0R3WreSS5p6o6nXlncj6IVLuxwQIpirzsqYYCJ3+gcq5ImUCfh99LU3iEz 1TEebyp8KHshoYpCLscHbTj6pSMMDEYDltHdKiu/EILx/h9CuB80URQ2Vz1rXHhK +WSVyF+/qvk9joFmnltlteTuttu1oy9VnwavKhrZeGSQF1C6flsHXSLHKhBra5ec 1g3VqebZo3pPcqp/P+oa/WtIYqtdyGDmMUn/iVeMEWc6zWVtVIDOtKuzkho0lsIF Vt/J9bBmvB1yc6iVTPlf8+Bkze8CaIoXHWSJOiHajpDMEuDLz+EpOXoVlzi/UbWN eYBnMtKpEpiGCpcsPXrG =bPgo -----END PGP SIGNATURE----- --7pwavh3sysogazgv-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: maxime.ripard@free-electrons.com (Maxime Ripard) Date: Thu, 29 Jun 2017 17:56:20 +0200 Subject: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus In-Reply-To: References: <20170627143652.13075-1-net147@gmail.com> <20170628092024.ejxy6itqj3hx6yew@flea> Message-ID: <20170629155620.4keqi4cumbtvv63u@flea> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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? 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. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Ripard Subject: Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus Date: Thu, 29 Jun 2017 17:56:20 +0200 Message-ID: <20170629155620.4keqi4cumbtvv63u@flea> References: <20170627143652.13075-1-net147@gmail.com> <20170628092024.ejxy6itqj3hx6yew@flea> Reply-To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="7pwavh3sysogazgv" Return-path: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Content-Disposition: inline In-Reply-To: List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Jonathan Liu Cc: David Airlie , Chen-Yu Tsai , linux-kernel , dri-devel , linux-arm-kernel , linux-sunxi List-Id: dri-devel@lists.freedesktop.org --7pwavh3sysogazgv Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline 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? 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. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --7pwavh3sysogazgv--