Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752426AbdFMLPJ (ORCPT ); Tue, 13 Jun 2017 07:15:09 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:52070 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752033AbdFMLPH (ORCPT ); Tue, 13 Jun 2017 07:15:07 -0400 Date: Tue, 13 Jun 2017 13:15:05 +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 v2] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus Message-ID: <20170613111505.afyh4iotxdz5kceh@flea.lan> References: <20170612055235.31019-1-net147@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="c3tuk7pmda5md4l3" Content-Disposition: inline In-Reply-To: <20170612055235.31019-1-net147@gmail.com> User-Agent: NeoMutt/20170602 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15605 Lines: 518 --c3tuk7pmda5md4l3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jun 12, 2017 at 03:52:35PM +1000, Jonathan Liu wrote: > The drm_get_edid function should be used instead of drm_do_get_edid by > exposing the DDC bus as an I2C adapter. Implement this for A10s. >=20 > Signed-off-by: Jonathan Liu Your commit log should explain *why* that function should be used instead, and why it's better to expose it as an i2c adadpter. > --- > Changes for v2: > - Rebased against Maxime's sunxi-drm/for-next branch > - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is delet= ed if > any of the calls after the I2C adapter is created fails > - Remove unnecessary includes in sun4i_hdmi_i2c.c >=20 > drivers/gpu/drm/sun4i/Makefile | 1 + > drivers/gpu/drm/sun4i/sun4i_hdmi.h | 11 ++- > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 103 +++------------------ > drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 163 +++++++++++++++++++++++++++= ++++++ > 4 files changed, 189 insertions(+), 89 deletions(-) > create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c >=20 > diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makef= ile > index e29fd3a2ba9c..43c753cafc88 100644 > --- a/drivers/gpu/drm/sun4i/Makefile > +++ b/drivers/gpu/drm/sun4i/Makefile > @@ -2,6 +2,7 @@ sun4i-drm-y +=3D sun4i_drv.o > sun4i-drm-y +=3D sun4i_framebuffer.o > =20 > sun4i-drm-hdmi-y +=3D sun4i_hdmi_enc.o > +sun4i-drm-hdmi-y +=3D sun4i_hdmi_i2c.o > sun4i-drm-hdmi-y +=3D sun4i_hdmi_ddc_clk.o > sun4i-drm-hdmi-y +=3D sun4i_hdmi_tmds_clk.o You probably also need to depend on the I2C framework in order to work, right? > =20 > diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/s= un4i_hdmi.h > index 2f2f2ff1ea63..4c01dbe89cd9 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h > @@ -97,6 +97,7 @@ > #define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30) > #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8) > #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8) > +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) All the other bit defines are ordered by descending orders, please keep it consistent. > #define SUN4I_HDMI_DDC_CTRL_RESET BIT(0) > =20 > #define SUN4I_HDMI_DDC_ADDR_REG 0x504 > @@ -105,6 +106,10 @@ > #define SUN4I_HDMI_DDC_ADDR_OFFSET(off) (((off) & 0xff) << 8) > #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr) ((addr) & 0xff) > =20 > +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG 0x50c It is called INT_STATUS in the datasheet > +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST BIT(4) > +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE BIT(0) > + > #define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510 > #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31) > =20 > @@ -112,7 +117,8 @@ > #define SUN4I_HDMI_DDC_BYTE_COUNT_REG 0x51c > =20 > #define SUN4I_HDMI_DDC_CMD_REG 0x520 > -#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ 6 > +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE 3 > +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ 5 Same remark here, and why do you remove the old one? > =20 > #define SUN4I_HDMI_DDC_CLK_REG 0x528 > #define SUN4I_HDMI_DDC_CLK_M(m) (((m) & 0x7) << 3) > @@ -146,6 +152,8 @@ struct sun4i_hdmi { > struct clk *ddc_clk; > struct clk *tmds_clk; > =20 > + struct i2c_adapter *i2c; > + > struct sun4i_drv *drv; > =20 > bool hdmi_monitor; > @@ -153,5 +161,6 @@ struct sun4i_hdmi { > =20 > int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk); > int sun4i_tmds_create(struct sun4i_hdmi *hdmi); > +int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi); > =20 > #endif /* _SUN4I_HDMI_H_ */ > diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun= 4i/sun4i_hdmi_enc.c > index d3398f6250ef..2a8c0b14eabc 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c > @@ -29,8 +29,6 @@ > #include "sun4i_hdmi.h" > #include "sun4i_tcon.h" > =20 > -#define DDC_SEGMENT_ADDR 0x30 > - > static inline struct sun4i_hdmi * > drm_encoder_to_sun4i_hdmi(struct drm_encoder *encoder) > { > @@ -184,93 +182,13 @@ static const struct drm_encoder_funcs sun4i_hdmi_fu= ncs =3D { > .destroy =3D drm_encoder_cleanup, > }; > =20 > -static int sun4i_hdmi_read_sub_block(struct sun4i_hdmi *hdmi, > - unsigned int blk, unsigned int offset, > - u8 *buf, unsigned int count) > -{ > - unsigned long reg; > - int i; > - > - reg =3D readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); > - reg &=3D ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK; > - writel(reg | SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ, > - hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); > - > - writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) | > - SUN4I_HDMI_DDC_ADDR_EDDC(DDC_SEGMENT_ADDR << 1) | > - SUN4I_HDMI_DDC_ADDR_OFFSET(offset) | > - SUN4I_HDMI_DDC_ADDR_SLAVE(DDC_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); > - > - writel(count, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG); > - writel(SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ, > - 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 (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg, > - !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD), > - 100, 100000)) > - return -EIO; > - > - for (i =3D 0; i < count; i++) > - buf[i] =3D readb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG); > - > - return 0; > -} > - > -static int sun4i_hdmi_read_edid_block(void *data, u8 *buf, unsigned int = blk, > - size_t length) > -{ > - struct sun4i_hdmi *hdmi =3D data; > - int retry =3D 2, i; > - > - do { > - for (i =3D 0; i < length; i +=3D SUN4I_HDMI_DDC_FIFO_SIZE) { > - unsigned char offset =3D blk * EDID_LENGTH + i; > - unsigned int count =3D min((unsigned int)SUN4I_HDMI_DDC_FIFO_SIZE, > - length - i); > - int ret; > - > - ret =3D sun4i_hdmi_read_sub_block(hdmi, blk, offset, > - buf + i, count); > - if (ret) > - return ret; > - } > - } while (!drm_edid_block_valid(buf, blk, true, NULL) && (retry--)); > - > - return 0; > -} > - > static int sun4i_hdmi_get_modes(struct drm_connector *connector) > { > struct sun4i_hdmi *hdmi =3D drm_connector_to_sun4i_hdmi(connector); > - unsigned long reg; > struct edid *edid; > int ret; > =20 > - /* 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); > - > - edid =3D drm_do_get_edid(connector, sun4i_hdmi_read_edid_block, hdmi); > + edid =3D drm_get_edid(connector, hdmi->i2c); > if (!edid) > return 0; > =20 > @@ -282,8 +200,6 @@ static int sun4i_hdmi_get_modes(struct drm_connector = *connector) > ret =3D drm_add_edid_modes(connector, edid); > kfree(edid); > =20 > - clk_disable_unprepare(hdmi->ddc_clk); > - > return ret; > } > =20 > @@ -413,6 +329,12 @@ static int sun4i_hdmi_bind(struct device *dev, struc= t device *master, > return ret; > } > =20 > + ret =3D sun4i_hdmi_i2c_create(hdmi); > + if (ret) { > + dev_err(dev, "Couldn't create the HDMI I2C adapter\n"); > + return ret; > + } > + > drm_encoder_helper_add(&hdmi->encoder, > &sun4i_hdmi_helper_funcs); > ret =3D drm_encoder_init(drm, > @@ -422,13 +344,15 @@ static int sun4i_hdmi_bind(struct device *dev, stru= ct device *master, > NULL); > if (ret) { > dev_err(dev, "Couldn't initialise the HDMI encoder\n"); > - return ret; > + goto err_del_i2c_adapter; > } > =20 > hdmi->encoder.possible_crtcs =3D drm_of_find_possible_crtcs(drm, > dev->of_node); > - if (!hdmi->encoder.possible_crtcs) > - return -EPROBE_DEFER; > + if (!hdmi->encoder.possible_crtcs) { > + ret =3D -EPROBE_DEFER; > + goto err_del_i2c_adapter; > + } > =20 > drm_connector_helper_add(&hdmi->connector, > &sun4i_hdmi_connector_helper_funcs); > @@ -451,6 +375,8 @@ static int sun4i_hdmi_bind(struct device *dev, struct= device *master, > =20 > err_cleanup_connector: > drm_encoder_cleanup(&hdmi->encoder); > +err_del_i2c_adapter: > + i2c_del_adapter(hdmi->i2c); > return ret; > } > =20 > @@ -461,6 +387,7 @@ static void sun4i_hdmi_unbind(struct device *dev, str= uct device *master, > =20 > drm_connector_cleanup(&hdmi->connector); > drm_encoder_cleanup(&hdmi->encoder); > + i2c_del_adapter(hdmi->i2c); > } > =20 > static const struct component_ops sun4i_hdmi_ops =3D { > diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c b/drivers/gpu/drm/sun= 4i/sun4i_hdmi_i2c.c > new file mode 100644 > index 000000000000..389b770be09b > --- /dev/null > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c > @@ -0,0 +1,163 @@ > +/* > + * Copyright (C) 2017 Jonathan Liu > + * > + * Jonathan Liu Is it? > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + */ > + > +#include > +#include > +#include > + > +#include "sun4i_hdmi.h" > + > +static int xfer_msg_chunk(struct sun4i_hdmi *hdmi, struct i2c_msg *msg) > +{ > + u32 count =3D min_t(u32, msg->len, SUN4I_HDMI_DDC_FIFO_SIZE); > + u32 reg; > + int i; > + > + 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); > + > + writel(count, hdmi->base > + + SUN4I_HDMI_DDC_BYTE_COUNT_REG); It's pretty much everywhere in that file, but the operator should be at the end of the first line, not the beginning of the second. (and you don't really need to wrap that one do you?) > + writel(msg->flags & I2C_M_RD Parenthesis around the condition would make it easier to read. > + ? 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 < count; i++) { > + writeb(*msg->buf++, hdmi->base > + + SUN4I_HDMI_DDC_FIFO_DATA_REG); writesb? > + --msg->len; > + } > + } > + > + 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_INTERRUPT_STATUS_REG); > + reg &=3D ~SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST; You don't need to use that mask. > + if (reg !=3D SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE) > + return -EIO; !(reg & SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE) would be enough. > + > + if (msg->flags & I2C_M_RD) { > + for (i =3D 0; i < count; i++) { > + *msg->buf++ =3D readb(hdmi->base > + + SUN4I_HDMI_DDC_FIFO_DATA_REG); readsb ? > + --msg->len; You shouldn't modify that length. > + } > + } > + > + return 0; > +} > + > +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg) > +{ > + u32 reg; > + int ret; > + > + if (!msg->len) > + return -EIO; > + > + while (msg->len) { > + reg =3D readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); > + reg &=3D ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK; > + > + writel(reg | (msg->flags & I2C_M_RD > + ? SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ > + : SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE), You can put that in a separate assignment after the mask has been applied, it will be easier to read. > + hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); > + > + writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->addr), > + hdmi->base + SUN4I_HDMI_DDC_ADDR_REG); What about the segment, offset and eddc addresses? > + > + ret =3D xfer_msg_chunk(hdmi, msg); > + if (ret) > + return ret; > + } > + > + 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, ret =3D num; > + int i; > + > + /* 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 { const? > + .owner =3D THIS_MODULE, > + .class =3D I2C_CLASS_DDC, > + .algo =3D &sun4i_hdmi_i2c_algorithm, > + .name =3D "sun4i_hdmi_i2c adapter", > +}; > + > +int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi) > +{ > + int ret =3D 0; > + > + i2c_set_adapdata(&sun4i_hdmi_i2c_adapter, hdmi); > + > + ret =3D i2c_add_adapter(&sun4i_hdmi_i2c_adapter); > + if (ret) > + return ret; It should probably create the DDC clock too. I'd also like to have this patch split. First converting the current code to a i2c (read-only) dirver, and then adding write support. Thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --c3tuk7pmda5md4l3 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJZP8k5AAoJEBx+YmzsjxAgWY4QALSV6GJlghokRikhKc+mGUWl 78H3gR6O+tBtAqZz+CUS87Ig4xXZfcZlPdV0HLrgfekniYtaCSWmLwx0Ff3BIJEq 7SkoghqIcxFZfM8Plo1G2KhJsJb2NQsJAweMzOeu05iVbkyIvwo6UlZAQc9bK4oB +TBHh9KpXB5ICpzKgDjdVKUFynOjsjYS3358G6lIVd/Q2YAi1sm89/WllY9/YQbZ 2SGp7NjdcR3pZwOh6keS+fTfN+9ej5si2LynVu3dwXu176gCagf3C1R97XzVJ3wk 3HOemsJ6WVj2rzPVgkO2Np6K4cApQQHCN48REKOEL7sEGy1h+jrtZdwaX7IdXktZ gHl2feRNd6du6JXZqV2F5GUnY3FJSB9yPloSznxEpffhtcp+XdfX5WapTLXFY0LM d3SLq2s/IZILZsvT1hpLRiXSXMbSmTp8vpMAH7T1A4BiPzKmK2P0oZyFdSOJIU+3 dU4yMfcpl2AubrhbPN9XNJxPu9iTEtb/zfe5YkJZA56vi88UVKR91lXYQCfOdAjW ugGof2hMW3cqhLMcm5czxPbFRaFAuZjWxkKp4w2NGaXCgq9Q2yknyUZ4fRc7JRZY cwQ6AcTyALIdfIwMijSciARiOWXrXzPpiIrekolfyFbWUrWoiWNgxjKFgJw0MDdv PlLYQ+FhNQeqtr7Z2Ncu =tTKR -----END PGP SIGNATURE----- --c3tuk7pmda5md4l3--