lkml.org 
[lkml]   [2017]   [Jun]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/

--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--

\
 
 \ /
  Last update: 2017-06-22 19:58    [W:0.119 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site