Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752785AbdFUV0b (ORCPT ); Wed, 21 Jun 2017 17:26:31 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:47783 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752075AbdFUV03 (ORCPT ); Wed, 21 Jun 2017 17:26:29 -0400 Date: Wed, 21 Jun 2017 23:26:16 +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 v3] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus Message-ID: <20170621212616.uzmxspziaknxs2ju@flea.lan> References: <20170614152933.19482-1-net147@gmail.com> <20170621085159.6atrdspsqgus63an@flea.lan> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="quexvzfl35x5t5xj" 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 Content-Length: 9981 Lines: 271 --quexvzfl35x5t5xj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 21, 2017 at 07:42:47PM +1000, Jonathan Liu wrote: > >> +static int wait_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 flag) > >> +{ > >> + /* 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_cl= k)) * 9; > >> + ktime_t wait_timeout =3D ktime_add_us(ktime_get(), 100000); > >> + u32 reg; > >> + > >> + for (;;) { > >> + /* Check for errors */ > >> + reg =3D readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG= ); > >> + if (reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) { > >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STAT= US_REG); > >> + return -EIO; > >> + } > >> + > >> + /* Check if requested FIFO flag is unset */ > >> + reg =3D readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_RE= G); > >> + if (!(reg & flag)) > >> + return 0; > >> + > >> + /* Timeout */ > >> + if (ktime_compare(ktime_get(), wait_timeout) > 0) > >> + return -EIO; > >> + > >> + /* Wait for 1-2 bytes to transfer */ > >> + usleep_range(byte_time, 2 * byte_time); > >> + } > >> + > >> + return -EIO; > >> +} > >> + > >> +static int wait_fifo_read_ready(struct sun4i_hdmi *hdmi) > >> +{ > >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_EMP= TY); > >> +} > >> + > >> +static int wait_fifo_write_ready(struct sun4i_hdmi *hdmi) > >> +{ > >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_FUL= L); > >> +} > > > > Both of these can use readl_poll_timeout, and I'm not sure you need to > > be that aggressive in your timings. > > > I have set my timings to minimize communication delays - e.g. when > userspace is reading from I2C one byte at a time (like i2cdump from > i2c-tools). I wouldn't try to optimise that too much. There's already a lot of overhead, it's way inefficient, and it's not really a significant use case anyway. > readl_poll_timeout can't be used to check 2 registers at > once and I couldn't get DDC_FIFO_Request_Interrupt_Status_Bit in > DDC_Int_Status register to behave properly. Can't you just use readl_poll_timeout, and then if it timed out check for errors? > I also wanted to minimize the chance of FIFO underrun/overrun. >=20 > >> + > >> +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg) > >> +{ > >> + u32 reg; > >> + int i; > >> + > >> + reg =3D readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); > >> + reg &=3D ~SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK; > >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); > >> + > >> + reg =3D readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); > >> + reg &=3D ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK; > >> + reg |=3D (msg->flags & I2C_M_RD) ? > >> + SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ : > >> + SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE; > >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); > >> + > >> + writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->addr), > >> + hdmi->base + SUN4I_HDMI_DDC_ADDR_REG); > >> + > >> + reg =3D readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG); > >> + writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR, > >> + hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG); > >> + > >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG, > >> + reg, > >> + !(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR), > >> + 100, 100000)) > >> + return -EIO; > >> + > >> + writel(msg->len, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG); > >> + writel(msg->flags & I2C_M_RD ? > >> + SUN4I_HDMI_DDC_CMD_IMPLICIT_READ : > >> + SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE, > >> + hdmi->base + SUN4I_HDMI_DDC_CMD_REG); > >> + > >> + reg =3D readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); > >> + writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD, > >> + hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); > >> + > >> + if (msg->flags & I2C_M_RD) { > >> + for (i =3D 0; i < msg->len; i++) { > >> + if (wait_fifo_read_ready(hdmi)) > >> + return -EIO; > > > > it seems weird that a function called read_ready would return 1 if > > there's an error. > > I will change this so wait_fifo_flag_unset returns false on error > and true on success. Then the wait_fifo_read_ready and > wait_fifo_write_ready conditions will be inverted. Or just invert the return value, whatever makes sense > > > >> + > >> + *msg->buf++ =3D readb(hdmi->base + > >> + SUN4I_HDMI_DDC_FIFO_DATA_REG= ); > > > > Don't you have an hardware FIFO you can rely on intead of reading > > everything byte per byte? > > > Hardware FIFO by nature is transferring one byte at a time. This is my > first kernel driver so I still have a lot to learn. In the future it > can be improved by using DMA and interrupts. Yes, you can read one byte at a time, but you don't need to check whether you're ready each byte you're reading. Can't you use FIFO_LEVEL or the byte counter to read the number of bytes you need to read in a single shot ? > >> + } > >> + } else { > >> + for (i =3D 0; i < msg->len; i++) { > >> + if (wait_fifo_write_ready(hdmi)) > >> + return -EIO; > >> + > >> + writeb(*msg->buf++, hdmi->base + > >> + SUN4I_HDMI_DDC_FIFO_DATA_REG); > >> + } > >> + } > >> + > >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, > >> + reg, > >> + !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD), > >> + 100, 100000)) > >> + return -EIO; > >> + > >> + reg =3D readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); > >> + > >> + /* Check for errors */ > >> + if ((reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) || > >> + !(reg & SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE)) { > >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); > >> + return -EIO; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static int sun4i_hdmi_i2c_xfer(struct i2c_adapter *adap, > >> + struct i2c_msg *msgs, int num) > >> +{ > >> + struct sun4i_hdmi *hdmi =3D i2c_get_adapdata(adap); > >> + u32 reg; > >> + int err, i, ret =3D num; > >> + > >> + for (i =3D 0; i < num; i++) { > >> + if (!msgs[i].len) > >> + return -EINVAL; > >> + if (msgs[i].len > SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE) > >> + return -EINVAL; > >> + } > >> + > >> + /* Reset I2C controller */ > >> + writel(SUN4I_HDMI_DDC_CTRL_ENABLE | SUN4I_HDMI_DDC_CTRL_RESET, > >> + hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); > >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg, > >> + !(reg & SUN4I_HDMI_DDC_CTRL_RESET), > >> + 100, 2000)) > >> + return -EIO; > >> + > >> + writel(SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE | > >> + SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE, > >> + hdmi->base + SUN4I_HDMI_DDC_LINE_CTRL_REG); > >> + > >> + clk_prepare_enable(hdmi->ddc_clk); > >> + clk_set_rate(hdmi->ddc_clk, 100000); > >> + > >> + for (i =3D 0; i < num; i++) { > >> + err =3D xfer_msg(hdmi, &msgs[i]); > >> + if (err) { > >> + ret =3D err; > >> + break; > >> + } > >> + } > >> + > >> + clk_disable_unprepare(hdmi->ddc_clk); > >> + return ret; > >> +} > >> + > >> +static u32 sun4i_hdmi_i2c_func(struct i2c_adapter *adap) > >> +{ > >> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; > >> +} > >> + > >> +static const struct i2c_algorithm sun4i_hdmi_i2c_algorithm =3D { > >> + .master_xfer =3D sun4i_hdmi_i2c_xfer, > >> + .functionality =3D sun4i_hdmi_i2c_func, > >> +}; > >> + > >> +static struct i2c_adapter sun4i_hdmi_i2c_adapter =3D { > >> + .owner =3D THIS_MODULE, > >> + .class =3D I2C_CLASS_DDC, > >> + .algo =3D &sun4i_hdmi_i2c_algorithm, > >> + .name =3D "sun4i_hdmi_i2c adapter", > >> +}; > > > > const? >=20 > i2c_add_adapter and i2c_del_adapter take "struct i2c_adapter *adapter" > argument so I can't make it const. Ah, right, they modify it. You can't allocate it statically then, otherwise if you get two instances, they'll both step on each other's toe. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --quexvzfl35x5t5xj Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJZSuR4AAoJEBx+YmzsjxAgxKoP/iSQBZ+PcS3ihn9bG2OyMObV zZwc5VZScvhEUlgAamEc1qp3p9qxsrz6wQNoWh4+zMEIeR2eAFAwHlkyAVA309op J+NVon/ZMs45O04dVI9chS/K4eXf69ODIZFCtu8fQWyIMfll/b7mPs8yt0+H2OD4 hSZYvXYpDC9qpTQTWLyl3B4kIwKpmqUt+1X2YtH9TM2llOsDlsRK1dErrmX8gk0v ESkMLkFfoWzbImtAbB/dGBpeq5IkptqxPQznitnxURmcAhDwJoF3ztw8CYcODR97 ZLz4wiaBSaJC6TRaLl+AcAwjQmTtJRInFNoKIx6Sq9muwgFyezOKtMk9KdHSSbi6 BoJsUznHHraZNALk/8H6Er2c9t75rPaKoB8tEuqwLZ7dEnADb8LHuXxnG5IrXLak prgy9jrNYbv7QPMTZeRj5tOuFq92xap4kNIgiLSRrBkOBretxq8vg983UPgxHMQk 9hZKmYEBt+GPHe64l/FWstbnJuv02LFuQy+DOTRiaj2Y+Ns80r3Uq7D10Ipd4KCQ NLKmcDigwoyqxLooSy76gQf1R37L6uC+ODXjJF4lUCOqPegG68QPQhOAmVelu+TO U0uEwGkCdBxYpi8bHWBrDQrMH0I+/qUbSfsbU7opUfhpJCJRrm5i0RG4v14nvn+t O+toD1KE4XI/9KBHiKZi =iGxJ -----END PGP SIGNATURE----- --quexvzfl35x5t5xj--