[RFC PATCH v2 1/2] drm/bridge: parade-ps8640: Use regmap APIs

[RFC PATCH v2 1/2] drm/bridge: parade-ps8640: Use regmap APIs

[Philip Chen]

Replace the direct i2c access (i2c_smbus_* functions) with regmap APIs,
which will simplify the future update on ps8640 driver.

Signed-off-by: Philip Chen <philipchen@chromium.org>
---

Changes in v2:
- Add separate reg map config per page

 drivers/gpu/drm/bridge/parade-ps8640.c | 89 ++++++++++++++++++--------
 1 file changed, 63 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 685e9c38b2db..1b2414601538 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -9,6 +9,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/of_graph.h>
+#include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
 #include <drm/drm_bridge.h>
@@ -31,6 +32,11 @@
 
 #define NUM_MIPI_LANES		4
 
+#define COMMON_PS8640_REGMAP_CONFIG \
+	.reg_bits = 8, \
+	.val_bits = 8, \
+	.cache_type = REGCACHE_NONE
+
 /*
  * PS8640 uses multiple addresses:
  * page[0]: for DP control
@@ -64,12 +70,48 @@ struct ps8640 {
 	struct drm_bridge *panel_bridge;
 	struct mipi_dsi_device *dsi;
 	struct i2c_client *page[MAX_DEVS];
+	struct regmap	*regmap[MAX_DEVS];
 	struct regulator_bulk_data supplies[2];
 	struct gpio_desc *gpio_reset;
 	struct gpio_desc *gpio_powerdown;
 	bool powered;
 };
 
+static const struct regmap_config ps8640_regmap_config[] = {
+	[PAGE0_DP_CNTL] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xbf
+	},
+	[PAGE1_VDO_BDG] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xff
+	},
+	[PAGE2_TOP_CNTL] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xff
+	},
+	[PAGE3_DSI_CNTL1] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xff
+	},
+	[PAGE4_MIPI_PHY] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xff
+	},
+	[PAGE5_VPLL] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0x7f
+	},
+	[PAGE6_DSI_CNTL2] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xff
+	},
+	[PAGE7_SPI_CNTL] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xff
+	}
+};
+
 static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
 {
 	return container_of(e, struct ps8640, bridge);
@@ -78,13 +120,13 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
 static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
 				     const enum ps8640_vdo_control ctrl)
 {
-	struct i2c_client *client = ps_bridge->page[PAGE3_DSI_CNTL1];
+	struct regmap *map = ps_bridge->regmap[PAGE3_DSI_CNTL1];
 	u8 vdo_ctrl_buf[] = { VDO_CTL_ADD, ctrl };
 	int ret;
 
-	ret = i2c_smbus_write_i2c_block_data(client, PAGE3_SET_ADD,
-					     sizeof(vdo_ctrl_buf),
-					     vdo_ctrl_buf);
+	ret = regmap_bulk_write(map, PAGE3_SET_ADD,
+				vdo_ctrl_buf, sizeof(vdo_ctrl_buf));
+
 	if (ret < 0) {
 		DRM_ERROR("failed to %sable VDO: %d\n",
 			  ctrl == ENABLE ? "en" : "dis", ret);
@@ -96,8 +138,7 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
 
 static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 {
-	struct i2c_client *client = ps_bridge->page[PAGE2_TOP_CNTL];
-	unsigned long timeout;
+	struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
 	int ret, status;
 
 	if (ps_bridge->powered)
@@ -121,18 +162,12 @@ static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 	 */
 	msleep(200);
 
-	timeout = jiffies + msecs_to_jiffies(200) + 1;
+	ret = regmap_read_poll_timeout(map, PAGE2_GPIO_H, status,
+			status & PS_GPIO9, 20 * 1000, 200 * 1000);
 
-	while (time_is_after_jiffies(timeout)) {
-		status = i2c_smbus_read_byte_data(client, PAGE2_GPIO_H);
-		if (status < 0) {
-			DRM_ERROR("failed read PAGE2_GPIO_H: %d\n", status);
-			goto err_regulators_disable;
-		}
-		if ((status & PS_GPIO9) == PS_GPIO9)
-			break;
-
-		msleep(20);
+	if (ret < 0) {
+		DRM_ERROR("failed read PAGE2_GPIO_H: %d\n", ret);
+		goto err_regulators_disable;
 	}
 
 	msleep(50);
@@ -144,22 +179,15 @@ static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 	 * disabled by the manufacturer. Once disabled, all MCS commands are
 	 * ignored by the display interface.
 	 */
-	status = i2c_smbus_read_byte_data(client, PAGE2_MCS_EN);
-	if (status < 0) {
-		DRM_ERROR("failed read PAGE2_MCS_EN: %d\n", status);
-		goto err_regulators_disable;
-	}
 
-	ret = i2c_smbus_write_byte_data(client, PAGE2_MCS_EN,
-					status & ~MCS_EN);
+	ret = regmap_update_bits(map, PAGE2_MCS_EN, MCS_EN, 0);
 	if (ret < 0) {
 		DRM_ERROR("failed write PAGE2_MCS_EN: %d\n", ret);
 		goto err_regulators_disable;
 	}
 
 	/* Switch access edp panel's edid through i2c */
-	ret = i2c_smbus_write_byte_data(client, PAGE2_I2C_BYPASS,
-					I2C_BYPASS_EN);
+	ret = regmap_write(map, PAGE2_I2C_BYPASS, I2C_BYPASS_EN);
 	if (ret < 0) {
 		DRM_ERROR("failed write PAGE2_I2C_BYPASS: %d\n", ret);
 		goto err_regulators_disable;
@@ -362,6 +390,10 @@ static int ps8640_probe(struct i2c_client *client)
 
 	ps_bridge->page[PAGE0_DP_CNTL] = client;
 
+	ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config);
+	if (IS_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]))
+		return PTR_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]);
+
 	for (i = 1; i < ARRAY_SIZE(ps_bridge->page); i++) {
 		ps_bridge->page[i] = devm_i2c_new_dummy_device(&client->dev,
 							     client->adapter,
@@ -371,6 +403,11 @@ static int ps8640_probe(struct i2c_client *client)
 				client->addr + i);
 			return PTR_ERR(ps_bridge->page[i]);
 		}
+
+		ps_bridge->regmap[i] = devm_regmap_init_i2c(ps_bridge->page[i],
+						ps8640_regmap_config + i);
+		if (IS_ERR(ps_bridge->regmap[i]))
+			return PTR_ERR(ps_bridge->regmap[i]);
 	}
 
 	i2c_set_clientdata(client, ps_bridge);
-- 
2.33.0.309.g3052b89438-goog

[RFC PATCH v2 2/2] drm/bridge: parade-ps8640: Add support for AUX channel

[Philip Chen]

Implement the first version of AUX support, which will be useful as
we expand the driver to support varied use cases.

WARNING: This patch is not fully verified by hardware. But as AUX CH
is not implemented for ps8640 driver until now, the patch shouldn't
cause any functional regression in practice.

Signed-off-by: Philip Chen <philipchen@chromium.org>
---

Changes in v2:
- Handle the case where an AUX transaction has no payload
- Add a reg polling for p0.0x83 to confirm AUX cmd is issued and
  read data is returned
- Replace regmap_noinc_read/write with looped regmap_read/write,
  as regmap_noinc_read/write doesn't read one byte at a time unless
  max_raw_read/write is set to 1.
- Register/Unregister the AUX device explicitly when the bridge is
  attached/detached
- Remove the use of runtime PM
- Program AUX addr/cmd/len in a single regmap_bulk_write()
- Add newlines for DRM_ERROR mesages

 drivers/gpu/drm/bridge/parade-ps8640.c | 156 ++++++++++++++++++++++++-
 1 file changed, 153 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 1b2414601538..3b28e992bb3e 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -13,11 +13,32 @@
 #include <linux/regulator/consumer.h>
 
 #include <drm/drm_bridge.h>
+#include <drm/drm_dp_helper.h>
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 
+#define PAGE0_AUXCH_CFG3	0x76
+#define  AUXCH_CFG3_RESET	0xff
+#define PAGE0_AUX_ADDR_7_0	0x7d
+#define PAGE0_AUX_ADDR_15_8	0x7e
+#define PAGE0_AUX_ADDR_23_16	0x7f
+#define  AUX_ADDR_19_16_MASK	GENMASK(3, 0)
+#define  AUX_CMD_MASK		GENMASK(7, 4)
+#define PAGE0_AUX_LENGTH	0x80
+#define  AUX_LENGTH_MASK	GENMASK(3, 0)
+#define  AUX_NO_PAYLOAD		BIT(7)
+#define PAGE0_AUX_WDATA		0x81
+#define PAGE0_AUX_RDATA		0x82
+#define PAGE0_AUX_CTRL		0x83
+#define  AUX_SEND		BIT(0)
+#define PAGE0_AUX_STATUS	0x84
+#define  AUX_STATUS_MASK	GENMASK(7, 5)
+#define  AUX_STATUS_TIMEOUT	(0x7 << 5)
+#define  AUX_STATUS_DEFER	(0x2 << 5)
+#define  AUX_STATUS_NACK	(0x1 << 5)
+
 #define PAGE2_GPIO_H		0xa7
 #define  PS_GPIO9		BIT(1)
 #define PAGE2_I2C_BYPASS	0xea
@@ -68,6 +89,7 @@ enum ps8640_vdo_control {
 struct ps8640 {
 	struct drm_bridge bridge;
 	struct drm_bridge *panel_bridge;
+	struct drm_dp_aux aux;
 	struct mipi_dsi_device *dsi;
 	struct i2c_client *page[MAX_DEVS];
 	struct regmap	*regmap[MAX_DEVS];
@@ -117,6 +139,114 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
 	return container_of(e, struct ps8640, bridge);
 }
 
+static inline struct ps8640 *aux_to_ps8640(struct drm_dp_aux *aux)
+{
+	return container_of(aux, struct ps8640, aux);
+}
+
+static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux,
+				   struct drm_dp_aux_msg *msg)
+{
+	struct ps8640 *ps_bridge = aux_to_ps8640(aux);
+	struct regmap *map = ps_bridge->regmap[PAGE0_DP_CNTL];
+	unsigned int len = msg->size;
+	unsigned int data;
+	int ret;
+	u8 request = msg->request &
+		     ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE);
+	u8 *buf = msg->buffer;
+	u8 addr_len[PAGE0_AUX_LENGTH + 1 - PAGE0_AUX_ADDR_7_0];
+	u8 i;
+	bool is_native_aux = false;
+
+	if (len > DP_AUX_MAX_PAYLOAD_BYTES)
+		return -EINVAL;
+
+	switch (request) {
+	case DP_AUX_NATIVE_WRITE:
+	case DP_AUX_NATIVE_READ:
+		is_native_aux = true;
+	case DP_AUX_I2C_WRITE:
+	case DP_AUX_I2C_READ:
+		ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
+		break;
+	default:
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	/* Assume it's good */
+	msg->reply = 0;
+
+	addr_len[0] = msg->address & 0xff;
+	addr_len[1] = (msg->address >> 8) & 0xff;
+	addr_len[2] = ((request << 4) & AUX_CMD_MASK) |
+		((msg->address >> 16) & AUX_ADDR_19_16_MASK);
+	addr_len[3] = (len == 0) ? AUX_NO_PAYLOAD :
+			((len - 1) & AUX_LENGTH_MASK);
+
+	regmap_bulk_write(map, PAGE0_AUX_ADDR_7_0, addr_len,
+			  ARRAY_SIZE(addr_len));
+
+	if (len && (request == DP_AUX_NATIVE_WRITE ||
+		    request == DP_AUX_I2C_WRITE)) {
+		/* Write to the internal FIFO buffer */
+		for (i = 0; i < len; i++) {
+			ret = regmap_write(map, PAGE0_AUX_WDATA, buf[i]);
+			if (ret < 0) {
+				DRM_ERROR("failed to write PAGE0_AUX_WDATA\n");
+				goto exit;
+			}
+		}
+	}
+
+	regmap_write(map, PAGE0_AUX_CTRL, AUX_SEND);
+
+	/* Zero delay loop because i2c transactions are slow already */
+	ret = regmap_read_poll_timeout(map, PAGE0_AUX_CTRL, data,
+				       !(data & AUX_SEND), 0, 50 * 1000);
+	if (ret)
+		goto exit;
+
+	regmap_read(map, PAGE0_AUX_STATUS, &data);
+	switch (data & AUX_STATUS_MASK) {
+	case AUX_STATUS_DEFER:
+		if (is_native_aux)
+			msg->reply |= DP_AUX_NATIVE_REPLY_DEFER;
+		else
+			msg->reply |= DP_AUX_I2C_REPLY_DEFER;
+		ret = -EBUSY;
+		goto exit;
+	case AUX_STATUS_NACK:
+		if (is_native_aux)
+			msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
+		else
+			msg->reply |= DP_AUX_I2C_REPLY_NACK;
+		ret = -EBUSY;
+		goto exit;
+	case AUX_STATUS_TIMEOUT:
+		ret = -ETIMEDOUT;
+		goto exit;
+	}
+
+	if (len && (request == DP_AUX_NATIVE_READ ||
+		    request == DP_AUX_I2C_READ)) {
+		/* Read from the internal FIFO buffer */
+		for (i = 0; i < len; i++) {
+			ret = regmap_read(map, PAGE0_AUX_WDATA, &data);
+			buf[i] = data;
+			if (ret < 0)
+				DRM_ERROR("failed to read PAGE0_AUX_RDATA\n");
+		}
+	}
+
+exit:
+	if (ret)
+		return ret;
+
+	return len;
+}
+
 static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
 				     const enum ps8640_vdo_control ctrl)
 {
@@ -286,18 +416,32 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->lanes = NUM_MIPI_LANES;
 	ret = mipi_dsi_attach(dsi);
-	if (ret)
-		goto err_dsi_attach;
+	if (ret) {
+		dev_err(dev, "failed to attach dsi device: %d\n", ret);
+		goto exit;
+	}
+
+	ret = drm_dp_aux_register(&ps_bridge->aux);
+	if (ret) {
+		dev_err(dev, "failed to register DP AUX channel: %d\n", ret);
+		goto exit;
+	}
 
 	/* Attach the panel-bridge to the dsi bridge */
 	return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
 				 &ps_bridge->bridge, flags);
 
-err_dsi_attach:
+exit:
 	mipi_dsi_device_unregister(dsi);
 	return ret;
 }
 
+
+static void ps8640_bridge_detach(struct drm_bridge *bridge)
+{
+	drm_dp_aux_unregister(&bridge_to_ps8640(bridge)->aux);
+}
+
 static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
 					   struct drm_connector *connector)
 {
@@ -334,6 +478,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
 
 static const struct drm_bridge_funcs ps8640_bridge_funcs = {
 	.attach = ps8640_bridge_attach,
+	.detach = ps8640_bridge_detach,
 	.get_edid = ps8640_bridge_get_edid,
 	.post_disable = ps8640_post_disable,
 	.pre_enable = ps8640_pre_enable,
@@ -412,6 +557,11 @@ static int ps8640_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, ps_bridge);
 
+	ps_bridge->aux.name = "parade-ps8640-aux";
+	ps_bridge->aux.dev = dev;
+	ps_bridge->aux.transfer = ps8640_aux_transfer;
+	drm_dp_aux_init(&ps_bridge->aux);
+
 	drm_bridge_add(&ps_bridge->bridge);
 
 	return 0;
-- 
2.33.0.309.g3052b89438-goog

Re: [RFC PATCH v2 2/2] drm/bridge: parade-ps8640: Add support for AUX channel

[Doug Anderson]

Hi,

On Mon, Sep 13, 2021 at 2:33 PM Philip Chen <philipchen@chromium.org> wrote:
>
> Implement the first version of AUX support, which will be useful as
> we expand the driver to support varied use cases.
>
> WARNING: This patch is not fully verified by hardware. But as AUX CH
> is not implemented for ps8640 driver until now, the patch shouldn't
> cause any functional regression in practice.

Thanks for the heads up. NOTE: having this patch posted to do early
code review is fine, but fair warning that I don't think there'd be
much benefit in landing until the patch is verified more.


> Signed-off-by: Philip Chen <philipchen@chromium.org>
> ---
>
> Changes in v2:
> - Handle the case where an AUX transaction has no payload
> - Add a reg polling for p0.0x83 to confirm AUX cmd is issued and
>   read data is returned
> - Replace regmap_noinc_read/write with looped regmap_read/write,
>   as regmap_noinc_read/write doesn't read one byte at a time unless
>   max_raw_read/write is set to 1.

What about if you set val_bytes? I think you just need to set that to
"1" and it'll work?


> - Register/Unregister the AUX device explicitly when the bridge is
>   attached/detached
> - Remove the use of runtime PM

I suspect runtime PM will need to be added back in at some point since
AUX channel needs to be functional even if the bridge hasn't been
"pre_enable"ed.


> - Program AUX addr/cmd/len in a single regmap_bulk_write()
> - Add newlines for DRM_ERROR mesages
>
>  drivers/gpu/drm/bridge/parade-ps8640.c | 156 ++++++++++++++++++++++++-
>  1 file changed, 153 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 1b2414601538..3b28e992bb3e 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -13,11 +13,32 @@
>  #include <linux/regulator/consumer.h>
>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_dp_helper.h>
>  #include <drm/drm_mipi_dsi.h>
>  #include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_print.h>
>
> +#define PAGE0_AUXCH_CFG3       0x76
> +#define  AUXCH_CFG3_RESET      0xff
> +#define PAGE0_AUX_ADDR_7_0     0x7d
> +#define PAGE0_AUX_ADDR_15_8    0x7e
> +#define PAGE0_AUX_ADDR_23_16   0x7f

nit: my manual calls the above "SWAUX_ADDR". Can you add the "SW"? I
know it doesn't look pretty, but matching the manual is really nice.
Similar with other commands below.


> +#define  AUX_ADDR_19_16_MASK   GENMASK(3, 0)
> +#define  AUX_CMD_MASK          GENMASK(7, 4)
> +#define PAGE0_AUX_LENGTH       0x80
> +#define  AUX_LENGTH_MASK       GENMASK(3, 0)
> +#define  AUX_NO_PAYLOAD                BIT(7)
> +#define PAGE0_AUX_WDATA                0x81
> +#define PAGE0_AUX_RDATA                0x82
> +#define PAGE0_AUX_CTRL         0x83
> +#define  AUX_SEND              BIT(0)
> +#define PAGE0_AUX_STATUS       0x84
> +#define  AUX_STATUS_MASK       GENMASK(7, 5)
> +#define  AUX_STATUS_TIMEOUT    (0x7 << 5)
> +#define  AUX_STATUS_DEFER      (0x2 << 5)
> +#define  AUX_STATUS_NACK       (0x1 << 5)
> +
>  #define PAGE2_GPIO_H           0xa7
>  #define  PS_GPIO9              BIT(1)
>  #define PAGE2_I2C_BYPASS       0xea
> @@ -68,6 +89,7 @@ enum ps8640_vdo_control {
>  struct ps8640 {
>         struct drm_bridge bridge;
>         struct drm_bridge *panel_bridge;
> +       struct drm_dp_aux aux;
>         struct mipi_dsi_device *dsi;
>         struct i2c_client *page[MAX_DEVS];
>         struct regmap   *regmap[MAX_DEVS];
> @@ -117,6 +139,114 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
>         return container_of(e, struct ps8640, bridge);
>  }
>
> +static inline struct ps8640 *aux_to_ps8640(struct drm_dp_aux *aux)
> +{
> +       return container_of(aux, struct ps8640, aux);
> +}
> +
> +static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux,
> +                                  struct drm_dp_aux_msg *msg)
> +{
> +       struct ps8640 *ps_bridge = aux_to_ps8640(aux);
> +       struct regmap *map = ps_bridge->regmap[PAGE0_DP_CNTL];
> +       unsigned int len = msg->size;
> +       unsigned int data;
> +       int ret;
> +       u8 request = msg->request &
> +                    ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE);
> +       u8 *buf = msg->buffer;
> +       u8 addr_len[PAGE0_AUX_LENGTH + 1 - PAGE0_AUX_ADDR_7_0];
> +       u8 i;
> +       bool is_native_aux = false;
> +
> +       if (len > DP_AUX_MAX_PAYLOAD_BYTES)
> +               return -EINVAL;
> +
> +       switch (request) {
> +       case DP_AUX_NATIVE_WRITE:
> +       case DP_AUX_NATIVE_READ:
> +               is_native_aux = true;

I think you need a "fallthrough;" here.


> +       case DP_AUX_I2C_WRITE:
> +       case DP_AUX_I2C_READ:
> +               ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);

Why not move the regmap_write() out of the switch statement? Also: you
store the "ret" but you never check it. You should handle the error.


> +               break;
> +       default:
> +               ret = -EINVAL;
> +               goto exit;
> +       }
> +
> +       /* Assume it's good */
> +       msg->reply = 0;
> +
> +       addr_len[0] = msg->address & 0xff;
> +       addr_len[1] = (msg->address >> 8) & 0xff;
> +       addr_len[2] = ((request << 4) & AUX_CMD_MASK) |

Instead of "request", needs to be the version of "request" without
DP_AUX_I2C_MOT and DP_AUX_I2C_WRITE_STATUS_UPDATE stripped out. In the
TI bridge chip it calls this "request_val".


> +               ((msg->address >> 16) & AUX_ADDR_19_16_MASK);
> +       addr_len[3] = (len == 0) ? AUX_NO_PAYLOAD :
> +                       ((len - 1) & AUX_LENGTH_MASK);
> +
> +       regmap_bulk_write(map, PAGE0_AUX_ADDR_7_0, addr_len,
> +                         ARRAY_SIZE(addr_len));
> +
> +       if (len && (request == DP_AUX_NATIVE_WRITE ||
> +                   request == DP_AUX_I2C_WRITE)) {
> +               /* Write to the internal FIFO buffer */
> +               for (i = 0; i < len; i++) {
> +                       ret = regmap_write(map, PAGE0_AUX_WDATA, buf[i]);
> +                       if (ret < 0) {
> +                               DRM_ERROR("failed to write PAGE0_AUX_WDATA\n");

nit: can you use dev_err() so that the dev gets printed? Also, can you
print the error code?


> +                               goto exit;

Unless you re-add pm_runtime (which you'll have to eventually), the
above "goto exit" should just be "return ret". Same with other "goto
exit"s in your patch.


> +                       }
> +               }
> +       }
> +
> +       regmap_write(map, PAGE0_AUX_CTRL, AUX_SEND);
> +
> +       /* Zero delay loop because i2c transactions are slow already */
> +       ret = regmap_read_poll_timeout(map, PAGE0_AUX_CTRL, data,
> +                                      !(data & AUX_SEND), 0, 50 * 1000);
> +       if (ret)
> +               goto exit;
> +
> +       regmap_read(map, PAGE0_AUX_STATUS, &data);
> +       switch (data & AUX_STATUS_MASK) {
> +       case AUX_STATUS_DEFER:
> +               if (is_native_aux)
> +                       msg->reply |= DP_AUX_NATIVE_REPLY_DEFER;
> +               else
> +                       msg->reply |= DP_AUX_I2C_REPLY_DEFER;
> +               ret = -EBUSY;
> +               goto exit;

In the TI bridge chip driver we decided that we shouldn't handle the
defer case since the hardware was already handling it. Specifically in
the case of this bridge you can see that AUXCH_CFG1 shows a default
value of retrying 7 times. ...so presumably we shouldn't ever actually
get a defer here.


> +       case AUX_STATUS_NACK:
> +               if (is_native_aux)
> +                       msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
> +               else
> +                       msg->reply |= DP_AUX_I2C_REPLY_NACK;
> +               ret = -EBUSY;

I believe that you shouldn't be setting "ret = -EBUSY" here. You're
supposed to be returning the number of bytes that were read / written
before the NAK happened. In the read case you should also be returning
the data that was actually read before the NAK too. Basically, add
something that reads "M" and stores it in "len"


> +               goto exit;
> +       case AUX_STATUS_TIMEOUT:
> +               ret = -ETIMEDOUT;
> +               goto exit;

You seem to be missing handlers for:
* ACKM
* Invalid reply
* I2C NACK
* I2C defer (probably don't need to deal with this?)


> +       }
> +
> +       if (len && (request == DP_AUX_NATIVE_READ ||
> +                   request == DP_AUX_I2C_READ)) {
> +               /* Read from the internal FIFO buffer */
> +               for (i = 0; i < len; i++) {
> +                       ret = regmap_read(map, PAGE0_AUX_WDATA, &data);

Oops, the above should be RDATA, not WDATA.


> +                       buf[i] = data;
> +                       if (ret < 0)
> +                               DRM_ERROR("failed to read PAGE0_AUX_RDATA\n");

Return the error? Print the error code?


> +               }
> +       }
> +
> +exit:
> +       if (ret)
> +               return ret;
> +
> +       return len;
> +}
> +
>  static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
>                                      const enum ps8640_vdo_control ctrl)
>  {
> @@ -286,18 +416,32 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
>         dsi->format = MIPI_DSI_FMT_RGB888;
>         dsi->lanes = NUM_MIPI_LANES;
>         ret = mipi_dsi_attach(dsi);
> -       if (ret)
> -               goto err_dsi_attach;
> +       if (ret) {
> +               dev_err(dev, "failed to attach dsi device: %d\n", ret);
> +               goto exit;
> +       }
> +
> +       ret = drm_dp_aux_register(&ps_bridge->aux);
> +       if (ret) {
> +               dev_err(dev, "failed to register DP AUX channel: %d\n", ret);
> +               goto exit;

Don't you need to go to an "error" label that causes mipi_dsi_detach()
to get called?


> +       }
>
>         /* Attach the panel-bridge to the dsi bridge */
>         return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
>                                  &ps_bridge->bridge, flags);
>
> -err_dsi_attach:
> +exit:
>         mipi_dsi_device_unregister(dsi);
>         return ret;
>  }
>
> +
> +static void ps8640_bridge_detach(struct drm_bridge *bridge)
> +{
> +       drm_dp_aux_unregister(&bridge_to_ps8640(bridge)->aux);

I suspect that the lack of a "detach" was a prexisting bug. Perhaps
you can submit a patch before yours that fixes it? It should undo the
things that attach did, like calling mipi_dsi_detach() and
mipi_dsi_device_unregister()


> +}
> +
>  static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>                                            struct drm_connector *connector)
>  {
> @@ -334,6 +478,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>
>  static const struct drm_bridge_funcs ps8640_bridge_funcs = {
>         .attach = ps8640_bridge_attach,
> +       .detach = ps8640_bridge_detach,
>         .get_edid = ps8640_bridge_get_edid,
>         .post_disable = ps8640_post_disable,
>         .pre_enable = ps8640_pre_enable,
> @@ -412,6 +557,11 @@ static int ps8640_probe(struct i2c_client *client)
>
>         i2c_set_clientdata(client, ps_bridge);
>
> +       ps_bridge->aux.name = "parade-ps8640-aux";
> +       ps_bridge->aux.dev = dev;
> +       ps_bridge->aux.transfer = ps8640_aux_transfer;
> +       drm_dp_aux_init(&ps_bridge->aux);
> +

Eventually you're going to want these here:

devm_of_dp_aux_populate_ep_devices(&ps_bridge->aux);
wait_for_device_probe()

...which I believe will enable DP AUX bus support. The second of those
two is just me being paranoid to handle the case where the AUX
endpoint device has an async probe.

In order to make the above work you'll need proper pm_runtime support
though since the aux channel needs to be usable before pre_enable().
That means you'll have to power the bridge on in the pm_runtime code
(enough to talk over the aux channel) and you'll need to use
"autosuspend" so that the bridge doesn't constantly power down between
aux transactions.

-Doug

Re: [RFC PATCH v2 1/2] drm/bridge: parade-ps8640: Use regmap APIs

[Doug Anderson]

Hi,

On Mon, Sep 13, 2021 at 2:33 PM Philip Chen <philipchen@chromium.org> wrote:
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 685e9c38b2db..1b2414601538 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -9,6 +9,7 @@
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/of_graph.h>
> +#include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>
>  #include <drm/drm_bridge.h>
> @@ -31,6 +32,11 @@
>
>  #define NUM_MIPI_LANES         4
>
> +#define COMMON_PS8640_REGMAP_CONFIG \
> +       .reg_bits = 8, \
> +       .val_bits = 8, \
> +       .cache_type = REGCACHE_NONE

At some point we should see if we get any speed gains by actually
caching, but that could be done later and isn't terribly high
priority.


> +
>  /*
>   * PS8640 uses multiple addresses:
>   * page[0]: for DP control
> @@ -64,12 +70,48 @@ struct ps8640 {
>         struct drm_bridge *panel_bridge;
>         struct mipi_dsi_device *dsi;
>         struct i2c_client *page[MAX_DEVS];
> +       struct regmap   *regmap[MAX_DEVS];
>         struct regulator_bulk_data supplies[2];
>         struct gpio_desc *gpio_reset;
>         struct gpio_desc *gpio_powerdown;
>         bool powered;
>  };
>
> +static const struct regmap_config ps8640_regmap_config[] = {
> +       [PAGE0_DP_CNTL] = {
> +               COMMON_PS8640_REGMAP_CONFIG,
> +               .max_register = 0xbf
> +       },
> +       [PAGE1_VDO_BDG] = {
> +               COMMON_PS8640_REGMAP_CONFIG,
> +               .max_register = 0xff
> +       },
> +       [PAGE2_TOP_CNTL] = {
> +               COMMON_PS8640_REGMAP_CONFIG,
> +               .max_register = 0xff
> +       },
> +       [PAGE3_DSI_CNTL1] = {
> +               COMMON_PS8640_REGMAP_CONFIG,
> +               .max_register = 0xff
> +       },
> +       [PAGE4_MIPI_PHY] = {
> +               COMMON_PS8640_REGMAP_CONFIG,
> +               .max_register = 0xff
> +       },
> +       [PAGE5_VPLL] = {
> +               COMMON_PS8640_REGMAP_CONFIG,
> +               .max_register = 0x7f
> +       },
> +       [PAGE6_DSI_CNTL2] = {
> +               COMMON_PS8640_REGMAP_CONFIG,
> +               .max_register = 0xff
> +       },
> +       [PAGE7_SPI_CNTL] = {
> +               COMMON_PS8640_REGMAP_CONFIG,
> +               .max_register = 0xff
> +       }

nit: stylistically it's nice to add a "," after the last brace too.
It's not technically needed but it makes diffs cleaner if another
config is later added.


> @@ -362,6 +390,10 @@ static int ps8640_probe(struct i2c_client *client)
>
>         ps_bridge->page[PAGE0_DP_CNTL] = client;
>
> +       ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config);
> +       if (IS_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]))
> +               return PTR_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]);

I'm a huge fan of dev_err_probe(). I wonder if it makes sense to use
it here? Untested:

if (IS_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]))
  return dev_err_probe(dev, PTR_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]),
                       "Error initting page 0 regmap\n");


All of that is just nits, so:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [RFC PATCH v2 1/2] drm/bridge: parade-ps8640: Use regmap APIs

[Philip Chen]

Hi, Doug

Thanks for the review.
I fixed the nits you pointed out in v3.
PTAL.

On Mon, Sep 13, 2021 at 5:32 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Sep 13, 2021 at 2:33 PM Philip Chen <philipchen@chromium.org> wrote:
> >
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index 685e9c38b2db..1b2414601538 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/module.h>
> >  #include <linux/of_graph.h>
> > +#include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> >
> >  #include <drm/drm_bridge.h>
> > @@ -31,6 +32,11 @@
> >
> >  #define NUM_MIPI_LANES         4
> >
> > +#define COMMON_PS8640_REGMAP_CONFIG \
> > +       .reg_bits = 8, \
> > +       .val_bits = 8, \
> > +       .cache_type = REGCACHE_NONE
>
> At some point we should see if we get any speed gains by actually
> caching, but that could be done later and isn't terribly high
> priority.
>
>
> > +
> >  /*
> >   * PS8640 uses multiple addresses:
> >   * page[0]: for DP control
> > @@ -64,12 +70,48 @@ struct ps8640 {
> >         struct drm_bridge *panel_bridge;
> >         struct mipi_dsi_device *dsi;
> >         struct i2c_client *page[MAX_DEVS];
> > +       struct regmap   *regmap[MAX_DEVS];
> >         struct regulator_bulk_data supplies[2];
> >         struct gpio_desc *gpio_reset;
> >         struct gpio_desc *gpio_powerdown;
> >         bool powered;
> >  };
> >
> > +static const struct regmap_config ps8640_regmap_config[] = {
> > +       [PAGE0_DP_CNTL] = {
> > +               COMMON_PS8640_REGMAP_CONFIG,
> > +               .max_register = 0xbf
> > +       },
> > +       [PAGE1_VDO_BDG] = {
> > +               COMMON_PS8640_REGMAP_CONFIG,
> > +               .max_register = 0xff
> > +       },
> > +       [PAGE2_TOP_CNTL] = {
> > +               COMMON_PS8640_REGMAP_CONFIG,
> > +               .max_register = 0xff
> > +       },
> > +       [PAGE3_DSI_CNTL1] = {
> > +               COMMON_PS8640_REGMAP_CONFIG,
> > +               .max_register = 0xff
> > +       },
> > +       [PAGE4_MIPI_PHY] = {
> > +               COMMON_PS8640_REGMAP_CONFIG,
> > +               .max_register = 0xff
> > +       },
> > +       [PAGE5_VPLL] = {
> > +               COMMON_PS8640_REGMAP_CONFIG,
> > +               .max_register = 0x7f
> > +       },
> > +       [PAGE6_DSI_CNTL2] = {
> > +               COMMON_PS8640_REGMAP_CONFIG,
> > +               .max_register = 0xff
> > +       },
> > +       [PAGE7_SPI_CNTL] = {
> > +               COMMON_PS8640_REGMAP_CONFIG,
> > +               .max_register = 0xff
> > +       }
>
> nit: stylistically it's nice to add a "," after the last brace too.
> It's not technically needed but it makes diffs cleaner if another
> config is later added.
>
>
> > @@ -362,6 +390,10 @@ static int ps8640_probe(struct i2c_client *client)
> >
> >         ps_bridge->page[PAGE0_DP_CNTL] = client;
> >
> > +       ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config);
> > +       if (IS_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]))
> > +               return PTR_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]);
>
> I'm a huge fan of dev_err_probe(). I wonder if it makes sense to use
> it here? Untested:
>
> if (IS_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]))
>   return dev_err_probe(dev, PTR_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]),
>                        "Error initting page 0 regmap\n");
>
>
> All of that is just nits, so:
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [RFC PATCH v2 2/2] drm/bridge: parade-ps8640: Add support for AUX channel

[Philip Chen]

Hi

On Mon, Sep 13, 2021 at 4:43 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Sep 13, 2021 at 2:33 PM Philip Chen <philipchen@chromium.org> wrote:
> >
> > Implement the first version of AUX support, which will be useful as
> > we expand the driver to support varied use cases.
> >
> > WARNING: This patch is not fully verified by hardware. But as AUX CH
> > is not implemented for ps8640 driver until now, the patch shouldn't
> > cause any functional regression in practice.
>
> Thanks for the heads up. NOTE: having this patch posted to do early
> code review is fine, but fair warning that I don't think there'd be
> much benefit in landing until the patch is verified more.
FYI - I've verified v3 patch with real hardware.
So this WARNING message is removed accordingly in v3.

>
>
> > Signed-off-by: Philip Chen <philipchen@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Handle the case where an AUX transaction has no payload
> > - Add a reg polling for p0.0x83 to confirm AUX cmd is issued and
> >   read data is returned
> > - Replace regmap_noinc_read/write with looped regmap_read/write,
> >   as regmap_noinc_read/write doesn't read one byte at a time unless
> >   max_raw_read/write is set to 1.
>
> What about if you set val_bytes? I think you just need to set that to
> "1" and it'll work?
I think val_bytes is already set to 1 as we set val_bits to 8. See:
map->format.val_bytes = DIV_ROUND_UP(config->val_bits, 8);

>
>
> > - Register/Unregister the AUX device explicitly when the bridge is
> >   attached/detached
> > - Remove the use of runtime PM
>
> I suspect runtime PM will need to be added back in at some point since
> AUX channel needs to be functional even if the bridge hasn't been
> "pre_enable"ed.
Thanks for the reminder.
I'll leave it to the follow-up patches.

>
>
> > - Program AUX addr/cmd/len in a single regmap_bulk_write()
> > - Add newlines for DRM_ERROR mesages
> >
> >  drivers/gpu/drm/bridge/parade-ps8640.c | 156 ++++++++++++++++++++++++-
> >  1 file changed, 153 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index 1b2414601538..3b28e992bb3e 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -13,11 +13,32 @@
> >  #include <linux/regulator/consumer.h>
> >
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_dp_helper.h>
> >  #include <drm/drm_mipi_dsi.h>
> >  #include <drm/drm_of.h>
> >  #include <drm/drm_panel.h>
> >  #include <drm/drm_print.h>
> >
> > +#define PAGE0_AUXCH_CFG3       0x76
> > +#define  AUXCH_CFG3_RESET      0xff
> > +#define PAGE0_AUX_ADDR_7_0     0x7d
> > +#define PAGE0_AUX_ADDR_15_8    0x7e
> > +#define PAGE0_AUX_ADDR_23_16   0x7f
>
> nit: my manual calls the above "SWAUX_ADDR". Can you add the "SW"? I
> know it doesn't look pretty, but matching the manual is really nice.
> Similar with other commands below.
Fixed in v3. PTAL.

>
>
> > +#define  AUX_ADDR_19_16_MASK   GENMASK(3, 0)
> > +#define  AUX_CMD_MASK          GENMASK(7, 4)
> > +#define PAGE0_AUX_LENGTH       0x80
> > +#define  AUX_LENGTH_MASK       GENMASK(3, 0)
> > +#define  AUX_NO_PAYLOAD                BIT(7)
> > +#define PAGE0_AUX_WDATA                0x81
> > +#define PAGE0_AUX_RDATA                0x82
> > +#define PAGE0_AUX_CTRL         0x83
> > +#define  AUX_SEND              BIT(0)
> > +#define PAGE0_AUX_STATUS       0x84
> > +#define  AUX_STATUS_MASK       GENMASK(7, 5)
> > +#define  AUX_STATUS_TIMEOUT    (0x7 << 5)
> > +#define  AUX_STATUS_DEFER      (0x2 << 5)
> > +#define  AUX_STATUS_NACK       (0x1 << 5)
> > +
> >  #define PAGE2_GPIO_H           0xa7
> >  #define  PS_GPIO9              BIT(1)
> >  #define PAGE2_I2C_BYPASS       0xea
> > @@ -68,6 +89,7 @@ enum ps8640_vdo_control {
> >  struct ps8640 {
> >         struct drm_bridge bridge;
> >         struct drm_bridge *panel_bridge;
> > +       struct drm_dp_aux aux;
> >         struct mipi_dsi_device *dsi;
> >         struct i2c_client *page[MAX_DEVS];
> >         struct regmap   *regmap[MAX_DEVS];
> > @@ -117,6 +139,114 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> >         return container_of(e, struct ps8640, bridge);
> >  }
> >
> > +static inline struct ps8640 *aux_to_ps8640(struct drm_dp_aux *aux)
> > +{
> > +       return container_of(aux, struct ps8640, aux);
> > +}
> > +
> > +static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux,
> > +                                  struct drm_dp_aux_msg *msg)
> > +{
> > +       struct ps8640 *ps_bridge = aux_to_ps8640(aux);
> > +       struct regmap *map = ps_bridge->regmap[PAGE0_DP_CNTL];
> > +       unsigned int len = msg->size;
> > +       unsigned int data;
> > +       int ret;
> > +       u8 request = msg->request &
> > +                    ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE);
> > +       u8 *buf = msg->buffer;
> > +       u8 addr_len[PAGE0_AUX_LENGTH + 1 - PAGE0_AUX_ADDR_7_0];
> > +       u8 i;
> > +       bool is_native_aux = false;
> > +
> > +       if (len > DP_AUX_MAX_PAYLOAD_BYTES)
> > +               return -EINVAL;
> > +
> > +       switch (request) {
> > +       case DP_AUX_NATIVE_WRITE:
> > +       case DP_AUX_NATIVE_READ:
> > +               is_native_aux = true;
>
> I think you need a "fallthrough;" here.
Fixed in v3. PTAL.
>
>
> > +       case DP_AUX_I2C_WRITE:
> > +       case DP_AUX_I2C_READ:
> > +               ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
>
> Why not move the regmap_write() out of the switch statement? Also: you
> store the "ret" but you never check it. You should handle the error.
Fixed in v3. PTAL.
>
>
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +               goto exit;
> > +       }
> > +
> > +       /* Assume it's good */
> > +       msg->reply = 0;
> > +
> > +       addr_len[0] = msg->address & 0xff;
> > +       addr_len[1] = (msg->address >> 8) & 0xff;
> > +       addr_len[2] = ((request << 4) & AUX_CMD_MASK) |
>
> Instead of "request", needs to be the version of "request" without
> DP_AUX_I2C_MOT and DP_AUX_I2C_WRITE_STATUS_UPDATE stripped out. In the
> TI bridge chip it calls this "request_val".
Thanks for catching this.
I fixed this in v3. PTAL.
>
>
> > +               ((msg->address >> 16) & AUX_ADDR_19_16_MASK);
> > +       addr_len[3] = (len == 0) ? AUX_NO_PAYLOAD :
> > +                       ((len - 1) & AUX_LENGTH_MASK);
> > +
> > +       regmap_bulk_write(map, PAGE0_AUX_ADDR_7_0, addr_len,
> > +                         ARRAY_SIZE(addr_len));
> > +
> > +       if (len && (request == DP_AUX_NATIVE_WRITE ||
> > +                   request == DP_AUX_I2C_WRITE)) {
> > +               /* Write to the internal FIFO buffer */
> > +               for (i = 0; i < len; i++) {
> > +                       ret = regmap_write(map, PAGE0_AUX_WDATA, buf[i]);
> > +                       if (ret < 0) {
> > +                               DRM_ERROR("failed to write PAGE0_AUX_WDATA\n");
>
> nit: can you use dev_err() so that the dev gets printed? Also, can you
> print the error code?
Fxed this in v3. PTAL.

>
>
> > +                               goto exit;
>
> Unless you re-add pm_runtime (which you'll have to eventually), the
> above "goto exit" should just be "return ret". Same with other "goto
> exit"s in your patch.
In v3, I replaced "goto exit" with "return ret" in this function.
PTAL.
>
>
> > +                       }
> > +               }
> > +       }
> > +
> > +       regmap_write(map, PAGE0_AUX_CTRL, AUX_SEND);
> > +
> > +       /* Zero delay loop because i2c transactions are slow already */
> > +       ret = regmap_read_poll_timeout(map, PAGE0_AUX_CTRL, data,
> > +                                      !(data & AUX_SEND), 0, 50 * 1000);
> > +       if (ret)
> > +               goto exit;
> > +
> > +       regmap_read(map, PAGE0_AUX_STATUS, &data);
> > +       switch (data & AUX_STATUS_MASK) {
> > +       case AUX_STATUS_DEFER:
> > +               if (is_native_aux)
> > +                       msg->reply |= DP_AUX_NATIVE_REPLY_DEFER;
> > +               else
> > +                       msg->reply |= DP_AUX_I2C_REPLY_DEFER;
> > +               ret = -EBUSY;
> > +               goto exit;
>
> In the TI bridge chip driver we decided that we shouldn't handle the
> defer case since the hardware was already handling it. Specifically in
> the case of this bridge you can see that AUXCH_CFG1 shows a default
> value of retrying 7 times. ...so presumably we shouldn't ever actually
> get a defer here.
Removed the handlers for DEFER cases and added comments in v3.
PTAL.
>
>
> > +       case AUX_STATUS_NACK:
> > +               if (is_native_aux)
> > +                       msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
> > +               else
> > +                       msg->reply |= DP_AUX_I2C_REPLY_NACK;
> > +               ret = -EBUSY;
>
> I believe that you shouldn't be setting "ret = -EBUSY" here. You're
> supposed to be returning the number of bytes that were read / written
> before the NAK happened. In the read case you should also be returning
> the data that was actually read before the NAK too. Basically, add
> something that reads "M" and stores it in "len"
Fixed this in v3. PTAL.
>
>
> > +               goto exit;
> > +       case AUX_STATUS_TIMEOUT:
> > +               ret = -ETIMEDOUT;
> > +               goto exit;
>
> You seem to be missing handlers for:
> * ACKM
> * Invalid reply
> * I2C NACK
> * I2C defer (probably don't need to deal with this?)
In v3, I added the handlers for all status codes except for DEFER.
PTAL.
>
>
> > +       }
> > +
> > +       if (len && (request == DP_AUX_NATIVE_READ ||
> > +                   request == DP_AUX_I2C_READ)) {
> > +               /* Read from the internal FIFO buffer */
> > +               for (i = 0; i < len; i++) {
> > +                       ret = regmap_read(map, PAGE0_AUX_WDATA, &data);
>
> Oops, the above should be RDATA, not WDATA.
Thanks for catching this.
I fixed this in v3. PTAL.
>
>
> > +                       buf[i] = data;
> > +                       if (ret < 0)
> > +                               DRM_ERROR("failed to read PAGE0_AUX_RDATA\n");
>
> Return the error? Print the error code?
I fixed this in v3. PTAL.
>
>
> > +               }
> > +       }
> > +
> > +exit:
> > +       if (ret)
> > +               return ret;
> > +
> > +       return len;
> > +}
> > +
> >  static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
> >                                      const enum ps8640_vdo_control ctrl)
> >  {
> > @@ -286,18 +416,32 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
> >         dsi->format = MIPI_DSI_FMT_RGB888;
> >         dsi->lanes = NUM_MIPI_LANES;
> >         ret = mipi_dsi_attach(dsi);
> > -       if (ret)
> > -               goto err_dsi_attach;
> > +       if (ret) {
> > +               dev_err(dev, "failed to attach dsi device: %d\n", ret);
> > +               goto exit;
> > +       }
> > +
> > +       ret = drm_dp_aux_register(&ps_bridge->aux);
> > +       if (ret) {
> > +               dev_err(dev, "failed to register DP AUX channel: %d\n", ret);
> > +               goto exit;
>
> Don't you need to go to an "error" label that causes mipi_dsi_detach()
> to get called?
I fixed this in v3. PTAL.
>
>
> > +       }
> >
> >         /* Attach the panel-bridge to the dsi bridge */
> >         return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
> >                                  &ps_bridge->bridge, flags);
> >
> > -err_dsi_attach:
> > +exit:
> >         mipi_dsi_device_unregister(dsi);
> >         return ret;
> >  }
> >
> > +
> > +static void ps8640_bridge_detach(struct drm_bridge *bridge)
> > +{
> > +       drm_dp_aux_unregister(&bridge_to_ps8640(bridge)->aux);
>
> I suspect that the lack of a "detach" was a prexisting bug. Perhaps
> you can submit a patch before yours that fixes it? It should undo the
> things that attach did, like calling mipi_dsi_detach() and
> mipi_dsi_device_unregister()
This doesn't look easy to get right.
Since it's pre-existing, can leave it alone for now?
>
>
> > +}
> > +
> >  static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
> >                                            struct drm_connector *connector)
> >  {
> > @@ -334,6 +478,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
> >
> >  static const struct drm_bridge_funcs ps8640_bridge_funcs = {
> >         .attach = ps8640_bridge_attach,
> > +       .detach = ps8640_bridge_detach,
> >         .get_edid = ps8640_bridge_get_edid,
> >         .post_disable = ps8640_post_disable,
> >         .pre_enable = ps8640_pre_enable,
> > @@ -412,6 +557,11 @@ static int ps8640_probe(struct i2c_client *client)
> >
> >         i2c_set_clientdata(client, ps_bridge);
> >
> > +       ps_bridge->aux.name = "parade-ps8640-aux";
> > +       ps_bridge->aux.dev = dev;
> > +       ps_bridge->aux.transfer = ps8640_aux_transfer;
> > +       drm_dp_aux_init(&ps_bridge->aux);
> > +
>
> Eventually you're going to want these here:
>
> devm_of_dp_aux_populate_ep_devices(&ps_bridge->aux);
> wait_for_device_probe()
>
> ...which I believe will enable DP AUX bus support. The second of those
> two is just me being paranoid to handle the case where the AUX
> endpoint device has an async probe.
>
> In order to make the above work you'll need proper pm_runtime support
> though since the aux channel needs to be usable before pre_enable().
> That means you'll have to power the bridge on in the pm_runtime code
> (enough to talk over the aux channel) and you'll need to use
> "autosuspend" so that the bridge doesn't constantly power down between
> aux transactions.
Thanks for the summary.
My next steps for this driver will be:
(1) Enable pm_runtime
(2) Populate endpoint devices in AUX CH

>
> -Doug

Re: [RFC PATCH v2 2/2] drm/bridge: parade-ps8640: Add support for AUX channel

[Doug Anderson]

Hi,

On Tue, Sep 14, 2021 at 5:28 PM Philip Chen <philipchen@chromium.org> wrote:
>
> > > Changes in v2:
> > > - Handle the case where an AUX transaction has no payload
> > > - Add a reg polling for p0.0x83 to confirm AUX cmd is issued and
> > >   read data is returned
> > > - Replace regmap_noinc_read/write with looped regmap_read/write,
> > >   as regmap_noinc_read/write doesn't read one byte at a time unless
> > >   max_raw_read/write is set to 1.
> >
> > What about if you set val_bytes? I think you just need to set that to
> > "1" and it'll work?
> I think val_bytes is already set to 1 as we set val_bits to 8. See:
> map->format.val_bytes = DIV_ROUND_UP(config->val_bits, 8);

To me that feels like a bug in the regmap API, then. I can't see how
it would make any sense for this function not to take val_bytes into
account...

I wonder if other users are somehow getting lucky today. Maybe users
that are using this for MMIO get lucky because max_raw_read is set
properly. ...and maybe other i2c users get lucky because some
peripherals are OK w/ this bug? AKA, maybe this actually works in most
cases for FIFOs:

write address of bridge chip on i2c bus
write R/W bit on i2c bus
write FIFO register address on i2c bus
read byte
read byte
read byte
...
read byte
read byte
end transaction

Normally for i2c you assume that the other side will read from
subsequent register addresses for each "read byte", but I suppose it's
possible that some i2c devices are setup to realize that if the
register address was the address of a FIFO that it shouldn't read from
the next register address but should just read the next byte in the
FIFO?


In any case, it's fine to do it with a loop like you're doing but it
still seems weird that you'd need to.


-Doug