From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751798AbdF1JVC (ORCPT ); Wed, 28 Jun 2017 05:21:02 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:40897 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751810AbdF1JUw (ORCPT ); Wed, 28 Jun 2017 05:20:52 -0400 Date: Wed, 28 Jun 2017 11:20:24 +0200 From: Maxime Ripard To: Jonathan Liu Cc: David Airlie , Chen-Yu Tsai , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@googlegroups.com Subject: Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus Message-ID: <20170628092024.ejxy6itqj3hx6yew@flea> References: <20170627143652.13075-1-net147@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="v3updldfse3hqdsh" Content-Disposition: inline In-Reply-To: <20170627143652.13075-1-net147@gmail.com> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --v3updldfse3hqdsh Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 28, 2017 at 12:36:52AM +1000, Jonathan Liu wrote: > +#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \ > + SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \ > + SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \ > + SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \ > + SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \ > + SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \ > + SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \ > +) > + > +static bool is_err_status(u32 int_status) > +{ > + return !!(int_status & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK); > +} > + > +static bool is_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 *fifo_status, > + u32 flag) > +{ > + *fifo_status =3D readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG); > + return !(*fifo_status & flag); > +} > + > +static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool= read) > +{ > + /* 1 byte takes 9 clock cycles (8 bits + 1 ACK) */ > + unsigned long byte_time =3D DIV_ROUND_UP(USEC_PER_SEC, > + clk_get_rate(hdmi->ddc_clk)) * 9; There's no real need for it to be dynamic. The clock rate will not change, and the order of magnitude is roughly 100us, so let's just use that (and make a comment). > + 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_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 =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; > + > + /* Read FIFO level */ > + int level =3D (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK= ); and explicitly read the fifo status here. That will make you remove that function that does two things while claiming that it does only one, and it will be more obvious. You can also just use reg at this point, instead of reading it once again. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --v3updldfse3hqdsh Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJZU3TYAAoJEBx+YmzsjxAg/DYP/1VzE0HiPLdQkz9BIdGzdHjS UXqpzn8JD9moS+tbSSTn76FYKI/fPEJpdlVdzgS8f7xYqQVajIuUi9qWg9QP+Nab 3prLy/XlTMltsrROHkRcSP+Sarab4TctTk7SwLjItNyDa61CmE/Iua3nkDUxhcUl bFfHAQIgfI3yAQFoaDu3I/gqhQOE8o5h6SRlRu3jSXIgoeu6ZgPqCYeuwXoV6X8x PG4vsdoG2SLnS+0/Jt8EXgsNGRwk9QXRpcEzAJs9/gkrncR+4BLp2t26kOH7mkHD LtXWNqx4lL1PUnCieB6qkVai/yrsMoqzshV0G7JZT/yHBKdnlaoPkdTDk3teCmK7 KJF69ODZbM7QtQl+LMco+Et5CwoueEsbvHBoWwm4PiLi8UWmfJjblsXF2aYamq+r fp5PTF3+l6vc1+Tg5M9jTtLjX5LZtDOefAsipv0adDQyqajfr4v729n6HLyu759D NjKZhY7Q4BMMvgNa89+8SuJZLGtm5CapKBUFHbGNZPHOmBnkYEeI5YuO1DXyWOpp JHiEbrLsbDvbn/8704wMf8exTTlDwQKEANY+cJ9P5Gu29M57pAP10EXDYpdNTpip RraFd7f0BxBXb0wL8IJUx7ef+OYk/X1Wt+i04bZVT+jKU7pUB0VhkxkeNjup8Ki9 HpyQAL/KfHBpo5xLm1G7 =HINp -----END PGP SIGNATURE----- --v3updldfse3hqdsh--