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 <[email protected]>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 66 +++++++++++++++-----------
1 file changed, 39 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 685e9c38b2db..a16725dbf912 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>
@@ -64,12 +65,29 @@ 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_range ps8640_volatile_ranges[] = {
+ { .range_min = 0, .range_max = 0xff },
+};
+
+static const struct regmap_access_table ps8640_volatile_table = {
+ .yes_ranges = ps8640_volatile_ranges,
+ .n_yes_ranges = ARRAY_SIZE(ps8640_volatile_ranges),
+};
+
+static const struct regmap_config ps8640_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .volatile_table = &ps8640_volatile_table,
+ .cache_type = REGCACHE_NONE,
+};
+
static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
{
return container_of(e, struct ps8640, bridge);
@@ -78,13 +96,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];
- u8 vdo_ctrl_buf[] = { VDO_CTL_ADD, ctrl };
+ 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 +114,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 +138,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 +155,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;
@@ -361,6 +365,10 @@ static int ps8640_probe(struct i2c_client *client)
ps_bridge->bridge.type = DRM_MODE_CONNECTOR_eDP;
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,
@@ -371,6 +379,10 @@ 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);
+ if (IS_ERR(ps_bridge->regmap))
+ return PTR_ERR(ps_bridge->regmap[i]);
}
i2c_set_clientdata(client, ps_bridge);
--
2.33.0.153.gba50c8fa24-goog
Implement the first version of AUX support, which will be useful as
we expand the driver to support varied use cases.
Signed-off-by: Philip Chen <[email protected]>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 123 +++++++++++++++++++++++++
1 file changed, 123 insertions(+)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index a16725dbf912..3f0241a60357 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -9,15 +9,36 @@
#include <linux/i2c.h>
#include <linux/module.h>
#include <linux/of_graph.h>
+#include <linux/pm_runtime.h>
#include <linux/regmap.h>
#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 PAGE0_AUX_WDATA 0x81
+#define PAGE0_AUX_RDATA 0x82
+#define PAGE0_AUX_CTRL 0x83
+#define AUX_START 0x01
+#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
@@ -63,6 +84,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];
@@ -93,6 +115,102 @@ 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 i2c_client *client = ps_bridge->page[PAGE0_DP_CNTL];
+ 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;
+ bool is_native_aux = false;
+
+ if (len > DP_AUX_MAX_PAYLOAD_BYTES)
+ return -EINVAL;
+
+ pm_runtime_get_sync(&client->dev);
+
+ 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:
+ regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
+ break;
+ default:
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ /* Assume it's good */
+ msg->reply = 0;
+
+ data = ((request << 4) & AUX_CMD_MASK) |
+ ((msg->address >> 16) & AUX_ADDR_19_16_MASK);
+ regmap_write(map, PAGE0_AUX_ADDR_23_16, data);
+ data = (msg->address >> 8) & 0xff;
+ regmap_write(map, PAGE0_AUX_ADDR_15_8, data);
+ data = msg->address & 0xff;
+ regmap_write(map, PAGE0_AUX_ADDR_7_0, msg->address & 0xff);
+
+ data = (len - 1) & AUX_LENGTH_MASK;
+ regmap_write(map, PAGE0_AUX_LENGTH, data);
+
+ if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE) {
+ ret = regmap_noinc_write(map, PAGE0_AUX_WDATA, buf, len);
+ if (ret < 0) {
+ DRM_ERROR("failed to write PAGE0_AUX_WDATA");
+ goto exit;
+ }
+ }
+
+ regmap_write(map, PAGE0_AUX_CTRL, AUX_START);
+
+ 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;
+ 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;
+ goto exit;
+ case AUX_STATUS_TIMEOUT:
+ ret = -ETIMEDOUT;
+ goto exit;
+ }
+
+ if (request == DP_AUX_NATIVE_READ || request == DP_AUX_I2C_READ) {
+ ret = regmap_noinc_read(map, PAGE0_AUX_RDATA, buf, len);
+ if (ret < 0)
+ DRM_ERROR("failed to read PAGE0_AUX_RDATA");
+ }
+
+exit:
+ pm_runtime_mark_last_busy(&client->dev);
+ pm_runtime_put_autosuspend(&client->dev);
+
+ if (ret)
+ return ret;
+
+ return len;
+}
+
static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
const enum ps8640_vdo_control ctrl)
{
@@ -387,6 +505,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.153.gba50c8fa24-goog
Quoting Philip Chen (2021-09-08 11:18:05)
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 685e9c38b2db..a16725dbf912 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -64,12 +65,29 @@ 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_range ps8640_volatile_ranges[] = {
> + { .range_min = 0, .range_max = 0xff },
Is the plan to fill this out later or is 0xff the max register? If it's
the latter then I think adding the max register to regmap_config is
simpler.
> +};
> +
> +static const struct regmap_access_table ps8640_volatile_table = {
> + .yes_ranges = ps8640_volatile_ranges,
> + .n_yes_ranges = ARRAY_SIZE(ps8640_volatile_ranges),
> +};
> +
> +static const struct regmap_config ps8640_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .volatile_table = &ps8640_volatile_table,
> + .cache_type = REGCACHE_NONE,
> +};
> +
> static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> {
> return container_of(e, struct ps8640, bridge);
> @@ -78,13 +96,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];
> - u8 vdo_ctrl_buf[] = { VDO_CTL_ADD, ctrl };
> + struct regmap *map = ps_bridge->regmap[PAGE3_DSI_CNTL1];
> + u8 vdo_ctrl_buf[] = {VDO_CTL_ADD, ctrl};
Nitpick: Add a space after { and before }.
> 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);
Quoting Philip Chen (2021-09-08 11:18:06)
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index a16725dbf912..3f0241a60357 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -93,6 +115,102 @@ 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 i2c_client *client = ps_bridge->page[PAGE0_DP_CNTL];
> + 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;
> + bool is_native_aux = false;
> +
> + if (len > DP_AUX_MAX_PAYLOAD_BYTES)
> + return -EINVAL;
> +
> + pm_runtime_get_sync(&client->dev);
Is this driver using runtime PM? Probably can't add this until it is
actually runtime PM enabled.
> +
> + 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:
> + regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
> + break;
> + default:
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + /* Assume it's good */
> + msg->reply = 0;
> +
> + data = ((request << 4) & AUX_CMD_MASK) |
> + ((msg->address >> 16) & AUX_ADDR_19_16_MASK);
> + regmap_write(map, PAGE0_AUX_ADDR_23_16, data);
> + data = (msg->address >> 8) & 0xff;
> + regmap_write(map, PAGE0_AUX_ADDR_15_8, data);
> + data = msg->address & 0xff;
> + regmap_write(map, PAGE0_AUX_ADDR_7_0, msg->address & 0xff);
Can we pack this into a three byte buffer and write it in one
regmap_bulk_write()? That would be nice because it looks like the
addresses are all next to each other in the i2c address space.
> +
> + data = (len - 1) & AUX_LENGTH_MASK;
> + regmap_write(map, PAGE0_AUX_LENGTH, data);
> +
> + if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE) {
> + ret = regmap_noinc_write(map, PAGE0_AUX_WDATA, buf, len);
> + if (ret < 0) {
> + DRM_ERROR("failed to write PAGE0_AUX_WDATA");
Needs a newline.
> + goto exit;
> + }
> + }
> +
> + regmap_write(map, PAGE0_AUX_CTRL, AUX_START);
> +
> + 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;
> + 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;
> + goto exit;
> + case AUX_STATUS_TIMEOUT:
> + ret = -ETIMEDOUT;
> + goto exit;
> + }
> +
> + if (request == DP_AUX_NATIVE_READ || request == DP_AUX_I2C_READ) {
> + ret = regmap_noinc_read(map, PAGE0_AUX_RDATA, buf, len);
> + if (ret < 0)
> + DRM_ERROR("failed to read PAGE0_AUX_RDATA");
Needs a newline.
> + }
> +
> +exit:
> + pm_runtime_mark_last_busy(&client->dev);
> + pm_runtime_put_autosuspend(&client->dev);
> +
> + if (ret)
> + return ret;
> +
> + return len;
> +}
> +
> static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
> const enum ps8640_vdo_control ctrl)
> {
Hi,
On Wed, Sep 8, 2021 at 3:27 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Philip Chen (2021-09-08 11:18:06)
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index a16725dbf912..3f0241a60357 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -93,6 +115,102 @@ 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 i2c_client *client = ps_bridge->page[PAGE0_DP_CNTL];
> > + 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;
> > + bool is_native_aux = false;
> > +
> > + if (len > DP_AUX_MAX_PAYLOAD_BYTES)
> > + return -EINVAL;
> > +
> > + pm_runtime_get_sync(&client->dev);
>
> Is this driver using runtime PM? Probably can't add this until it is
> actually runtime PM enabled.
Thanks - I think this driver doesn't enable runtime PM yet.
I'll remove all of the pm_runtime_* calls for now.
>
> > +
> > + 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:
> > + regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + goto exit;
> > + }
> > +
> > + /* Assume it's good */
> > + msg->reply = 0;
> > +
> > + data = ((request << 4) & AUX_CMD_MASK) |
> > + ((msg->address >> 16) & AUX_ADDR_19_16_MASK);
> > + regmap_write(map, PAGE0_AUX_ADDR_23_16, data);
> > + data = (msg->address >> 8) & 0xff;
> > + regmap_write(map, PAGE0_AUX_ADDR_15_8, data);
> > + data = msg->address & 0xff;
> > + regmap_write(map, PAGE0_AUX_ADDR_7_0, msg->address & 0xff);
>
> Can we pack this into a three byte buffer and write it in one
> regmap_bulk_write()? That would be nice because it looks like the
> addresses are all next to each other in the i2c address space.
Sure, I will address this in the next version.
>
> > +
> > + data = (len - 1) & AUX_LENGTH_MASK;
> > + regmap_write(map, PAGE0_AUX_LENGTH, data);
> > +
> > + if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE) {
> > + ret = regmap_noinc_write(map, PAGE0_AUX_WDATA, buf, len);
> > + if (ret < 0) {
> > + DRM_ERROR("failed to write PAGE0_AUX_WDATA");
>
> Needs a newline.
Adding an empty line here doesn't look like a common Linux style?
Could you point me to any similar instances in the Linux codebase?
>
> > + goto exit;
> > + }
> > + }
> > +
> > + regmap_write(map, PAGE0_AUX_CTRL, AUX_START);
> > +
> > + 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;
> > + 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;
> > + goto exit;
> > + case AUX_STATUS_TIMEOUT:
> > + ret = -ETIMEDOUT;
> > + goto exit;
> > + }
> > +
> > + if (request == DP_AUX_NATIVE_READ || request == DP_AUX_I2C_READ) {
> > + ret = regmap_noinc_read(map, PAGE0_AUX_RDATA, buf, len);
> > + if (ret < 0)
> > + DRM_ERROR("failed to read PAGE0_AUX_RDATA");
>
> Needs a newline.
>
> > + }
> > +
> > +exit:
> > + pm_runtime_mark_last_busy(&client->dev);
> > + pm_runtime_put_autosuspend(&client->dev);
> > +
> > + if (ret)
> > + return ret;
> > +
> > + return len;
> > +}
> > +
> > static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
> > const enum ps8640_vdo_control ctrl)
> > {
Hi,
On Wed, Sep 8, 2021 at 2:54 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Philip Chen (2021-09-08 11:18:05)
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index 685e9c38b2db..a16725dbf912 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -64,12 +65,29 @@ 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_range ps8640_volatile_ranges[] = {
> > + { .range_min = 0, .range_max = 0xff },
>
> Is the plan to fill this out later or is 0xff the max register? If it's
> the latter then I think adding the max register to regmap_config is
> simpler.
It's the former.
The real accessible register range is different per page, E.g.:
- For page0, the register range is 0x00 - 0xbf.
- For page1, the register range is 0x00 - 0xff.
- For page2, the register range is 0x80 - 0xff.
Even if we don't specify the accurate per-page register range later,
the default register range here (0x00 - 0xff) can cover the available
registers in each page.
>
> > +};
> > +
> > +static const struct regmap_access_table ps8640_volatile_table = {
> > + .yes_ranges = ps8640_volatile_ranges,
> > + .n_yes_ranges = ARRAY_SIZE(ps8640_volatile_ranges),
> > +};
> > +
> > +static const struct regmap_config ps8640_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .volatile_table = &ps8640_volatile_table,
> > + .cache_type = REGCACHE_NONE,
> > +};
> > +
> > static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> > {
> > return container_of(e, struct ps8640, bridge);
> > @@ -78,13 +96,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];
> > - u8 vdo_ctrl_buf[] = { VDO_CTL_ADD, ctrl };
> > + struct regmap *map = ps_bridge->regmap[PAGE3_DSI_CNTL1];
> > + u8 vdo_ctrl_buf[] = {VDO_CTL_ADD, ctrl};
>
> Nitpick: Add a space after { and before }.
Thanks. Will fix this in the next version.
>
> > 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);
Quoting Philip Chen (2021-09-09 11:15:27)
> On Wed, Sep 8, 2021 at 3:27 PM Stephen Boyd <[email protected]> wrote:
> >
> > Quoting Philip Chen (2021-09-08 11:18:06)
> >
> > > +
> > > + data = (len - 1) & AUX_LENGTH_MASK;
> > > + regmap_write(map, PAGE0_AUX_LENGTH, data);
> > > +
> > > + if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE) {
> > > + ret = regmap_noinc_write(map, PAGE0_AUX_WDATA, buf, len);
> > > + if (ret < 0) {
> > > + DRM_ERROR("failed to write PAGE0_AUX_WDATA");
> >
> > Needs a newline.
> Adding an empty line here doesn't look like a common Linux style?
> Could you point me to any similar instances in the Linux codebase?
Sorry. I meant on the DRM_ERROR message itself. Add a newline.
Quoting Philip Chen (2021-09-09 11:29:19)
> Hi,
>
> On Wed, Sep 8, 2021 at 2:54 PM Stephen Boyd <[email protected]> wrote:
> >
> > Quoting Philip Chen (2021-09-08 11:18:05)
> > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > index 685e9c38b2db..a16725dbf912 100644
> > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > @@ -64,12 +65,29 @@ 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_range ps8640_volatile_ranges[] = {
> > > + { .range_min = 0, .range_max = 0xff },
> >
> > Is the plan to fill this out later or is 0xff the max register? If it's
> > the latter then I think adding the max register to regmap_config is
> > simpler.
> It's the former.
> The real accessible register range is different per page, E.g.:
> - For page0, the register range is 0x00 - 0xbf.
> - For page1, the register range is 0x00 - 0xff.
> - For page2, the register range is 0x80 - 0xff.
Oh does this have register pages? regmap has support for pages where you
write some indirection register and then access the same i2c address for
the next page. This looks different though and has a different i2c
address for each page?
> Even if we don't specify the accurate per-page register range later,
> the default register range here (0x00 - 0xff) can cover the available
> registers in each page.
That could be done with max address in the config though, right?
volatile ranges is to indicate which registers are volatile and can't be
cached. I sort of doubt the entire rage is volatile.
Quoting Doug Anderson (2021-09-09 14:14:29)
> On Thu, Sep 9, 2021 at 12:09 PM Stephen Boyd <[email protected]> wrote:
> >
> >
> > Oh does this have register pages? regmap has support for pages where you
> > write some indirection register and then access the same i2c address for
> > the next page. This looks different though and has a different i2c
> > address for each page?
>
> I haven't looked in tons of detail, but I think the right solution
> here is a separate regmap config per page.
Yes. And then a different .max_register value for each config struct. A
different .name might be useful to indicate which page it is too.
Hi,
On Thu, Sep 9, 2021 at 12:09 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Philip Chen (2021-09-09 11:29:19)
> > Hi,
> >
> > On Wed, Sep 8, 2021 at 2:54 PM Stephen Boyd <[email protected]> wrote:
> > >
> > > Quoting Philip Chen (2021-09-08 11:18:05)
> > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > > index 685e9c38b2db..a16725dbf912 100644
> > > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > > @@ -64,12 +65,29 @@ 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_range ps8640_volatile_ranges[] = {
> > > > + { .range_min = 0, .range_max = 0xff },
> > >
> > > Is the plan to fill this out later or is 0xff the max register? If it's
> > > the latter then I think adding the max register to regmap_config is
> > > simpler.
> > It's the former.
> > The real accessible register range is different per page, E.g.:
> > - For page0, the register range is 0x00 - 0xbf.
> > - For page1, the register range is 0x00 - 0xff.
> > - For page2, the register range is 0x80 - 0xff.
>
> Oh does this have register pages? regmap has support for pages where you
> write some indirection register and then access the same i2c address for
> the next page. This looks different though and has a different i2c
> address for each page?
I haven't looked in tons of detail, but I think the right solution
here is a separate regmap config per page.
-Doug
On Thu, Sep 9, 2021 at 12:07 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Philip Chen (2021-09-09 11:15:27)
> > On Wed, Sep 8, 2021 at 3:27 PM Stephen Boyd <[email protected]> wrote:
> > >
> > > Quoting Philip Chen (2021-09-08 11:18:06)
> > >
> > > > +
> > > > + data = (len - 1) & AUX_LENGTH_MASK;
> > > > + regmap_write(map, PAGE0_AUX_LENGTH, data);
> > > > +
> > > > + if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE) {
> > > > + ret = regmap_noinc_write(map, PAGE0_AUX_WDATA, buf, len);
> > > > + if (ret < 0) {
> > > > + DRM_ERROR("failed to write PAGE0_AUX_WDATA");
> > >
> > > Needs a newline.
> > Adding an empty line here doesn't look like a common Linux style?
> > Could you point me to any similar instances in the Linux codebase?
>
> Sorry. I meant on the DRM_ERROR message itself. Add a newline.
Fixed in v2. PTAL.
On Thu, Sep 9, 2021 at 2:27 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Doug Anderson (2021-09-09 14:14:29)
> > On Thu, Sep 9, 2021 at 12:09 PM Stephen Boyd <[email protected]> wrote:
> > >
> > >
> > > Oh does this have register pages? regmap has support for pages where you
> > > write some indirection register and then access the same i2c address for
> > > the next page. This looks different though and has a different i2c
> > > address for each page?
> >
> > I haven't looked in tons of detail, but I think the right solution
> > here is a separate regmap config per page.
>
> Yes. And then a different .max_register value for each config struct. A
> different .name might be useful to indicate which page it is too.
I see.
I posted v2 with the fix for this.
PTAL.