Replace the direct i2c access (i2c_smbus_* functions) with regmap APIs, which will simplify the future update on ps8640 driver.
Reviewed-by: Douglas Anderson dianders@chromium.org Signed-off-by: Philip Chen philipchen@chromium.org ---
(no changes since v4)
Changes in v4: - Remove excessive error logging from the probe function
Changes in v3: - Fix the nits from v2 review
Changes in v2: - Add separate reg map config per page
drivers/gpu/drm/bridge/parade-ps8640.c | 94 ++++++++++++++++++-------- 1 file changed, 64 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 685e9c38b2db..18328e75bf90 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; - - 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; + ret = regmap_read_poll_timeout(map, PAGE2_GPIO_H, status, + status & PS_GPIO9, 20 * 1000, 200 * 1000);
- 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,15 +390,21 @@ 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, client->addr + i); - if (IS_ERR(ps_bridge->page[i])) { - dev_err(dev, "failed i2c dummy device, address %02x\n", - client->addr + i); + if (IS_ERR(ps_bridge->page[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);
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 philipchen@chromium.org ---
Changes in v5: - Add a couple of syntax fixes accidentally uncommited in v4
Changes in v4: - Fix aux_transfer function: - Replace dev_err with DRM_DEV_ERROR - Reorg the bit manipulation around address/len/request registers - Make SWAUX_STATUS_I2C_NACK fall through to SWAUX_STATUS_ACKM and store the number of read bytes to `len` w/o returning immediately
Changes in v3: - Verify with HW and thus remove WARNING from the patch description - Fix the reg names to better match the manual - Fix aux_transfer function: - Fix the switch statement which handles aux request - Write the original (unstripped) aux request code to the register - Replace DRM_ERROR with dev_err - Remove goto and just return ret - Fix the switch statement which handles aux status - When reading returned data, read from RDATA instead of WDATA - Fix attach function: - Call mipi_dsi_detach() when aux_register fails
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 messages
drivers/gpu/drm/bridge/parade-ps8640.c | 181 ++++++++++++++++++++++++- 1 file changed, 180 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 18328e75bf90..a9d5733e6b24 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -13,11 +13,37 @@ #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_SWAUX_ADDR_7_0 0x7d +#define PAGE0_SWAUX_ADDR_15_8 0x7e +#define PAGE0_SWAUX_ADDR_23_16 0x7f +#define SWAUX_ADDR_19_16_MASK GENMASK(3, 0) +#define SWAUX_CMD_MASK GENMASK(7, 4) +#define PAGE0_SWAUX_LENGTH 0x80 +#define SWAUX_LENGTH_MASK GENMASK(3, 0) +#define SWAUX_NO_PAYLOAD BIT(7) +#define PAGE0_SWAUX_WDATA 0x81 +#define PAGE0_SWAUX_RDATA 0x82 +#define PAGE0_SWAUX_CTRL 0x83 +#define SWAUX_SEND BIT(0) +#define PAGE0_SWAUX_STATUS 0x84 +#define SWAUX_M_MASK GENMASK(4, 0) +#define SWAUX_STATUS_MASK GENMASK(7, 5) +#define SWAUX_STATUS_NACK (0x1 << 5) +#define SWAUX_STATUS_DEFER (0x2 << 5) +#define SWAUX_STATUS_ACKM (0x3 << 5) +#define SWAUX_STATUS_INVALID (0x4 << 5) +#define SWAUX_STATUS_I2C_NACK (0x5 << 5) +#define SWAUX_STATUS_I2C_DEFER (0x6 << 5) +#define SWAUX_STATUS_TIMEOUT (0x7 << 5) + #define PAGE2_GPIO_H 0xa7 #define PS_GPIO9 BIT(1) #define PAGE2_I2C_BYPASS 0xea @@ -68,6 +94,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 +144,136 @@ 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]; + struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev; + + unsigned int len = msg->size; + unsigned int data; + unsigned int base; + int ret; + u8 request = msg->request & + ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE); + u8 *buf = msg->buffer; + u8 addr_len[PAGE0_SWAUX_LENGTH + 1 - PAGE0_SWAUX_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; + fallthrough; + case DP_AUX_I2C_WRITE: + case DP_AUX_I2C_READ: + break; + default: + return -EINVAL; + } + + ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET); + if (ret) { + DRM_DEV_ERROR(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", + ret); + return ret; + } + + /* Assume it's good */ + msg->reply = 0; + + base = PAGE0_SWAUX_ADDR_7_0; + addr_len[PAGE0_SWAUX_ADDR_7_0 - base] = msg->address; + addr_len[PAGE0_SWAUX_ADDR_15_8 - base] = msg->address >> 8; + addr_len[PAGE0_SWAUX_ADDR_23_16 - base] = (msg->address >> 16) & + SWAUX_ADDR_19_16_MASK; + addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= (msg->request << 4) & + SWAUX_CMD_MASK; + addr_len[PAGE0_SWAUX_LENGTH - base] = (len == 0) ? SWAUX_NO_PAYLOAD : + ((len - 1) & SWAUX_LENGTH_MASK); + + regmap_bulk_write(map, PAGE0_SWAUX_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_SWAUX_WDATA, buf[i]); + if (ret) { + DRM_DEV_ERROR(dev, + "failed to write WDATA: %d\n", + ret); + return ret; + } + } + } + + regmap_write(map, PAGE0_SWAUX_CTRL, SWAUX_SEND); + + /* Zero delay loop because i2c transactions are slow already */ + regmap_read_poll_timeout(map, PAGE0_SWAUX_CTRL, data, + !(data & SWAUX_SEND), 0, 50 * 1000); + + regmap_read(map, PAGE0_SWAUX_STATUS, &data); + if (ret) { + DRM_DEV_ERROR(dev, "failed to read PAGE0_SWAUX_STATUS: %d\n", + ret); + return ret; + } + + switch (data & SWAUX_STATUS_MASK) { + /* Ignore the DEFER cases as they are already handled in hardware */ + case SWAUX_STATUS_NACK: + case SWAUX_STATUS_I2C_NACK: + /* + * The programming guide is not clear about whether a I2C NACK + * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So + * we handle both cases together. + */ + if (is_native_aux) + msg->reply |= DP_AUX_NATIVE_REPLY_NACK; + else + msg->reply |= DP_AUX_I2C_REPLY_NACK; + + fallthrough; + case SWAUX_STATUS_ACKM: + len = data & SWAUX_M_MASK; + break; + case SWAUX_STATUS_INVALID: + return -EOPNOTSUPP; + case SWAUX_STATUS_TIMEOUT: + return -ETIMEDOUT; + } + + 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_SWAUX_RDATA, + (unsigned int *)(buf + i)); + if (ret) { + DRM_DEV_ERROR(dev, + "failed to read RDATA: %d\n", + ret); + return ret; + } + } + } + + return len; +} + static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge, const enum ps8640_vdo_control ctrl) { @@ -286,18 +443,34 @@ 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) + if (ret) { + dev_err(dev, "failed to attach dsi device: %d\n", ret); goto err_dsi_attach; + } + + ret = drm_dp_aux_register(&ps_bridge->aux); + if (ret) { + dev_err(dev, "failed to register DP AUX channel: %d\n", ret); + goto err_aux_register; + }
/* Attach the panel-bridge to the dsi bridge */ return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge, &ps_bridge->bridge, flags);
+err_aux_register: + mipi_dsi_detach(dsi); err_dsi_attach: 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 +507,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, @@ -409,6 +583,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;
Hi Philip, On Sat, Sep 18, 2021 at 10:21:17AM -0700, Philip Chen wrote:
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 philipchen@chromium.org
Patch is: Signed-off-by: Sam Ravnborg sam@ravnborg.org
please consider a few follow-up patches: 1) Replace deprecated drm_bridge_funcs with the _atomic_ variants. 2) Replace the deprecated drm_bridge_chain_pre_enable() with the atomic variant. 3) Use pr_() and dev_() logging in favour of DRM_ logging. DRM_ logging is deprecated these days and do not belong in bridge drivers anyway.
Maxime has a few patches pending for this driver - it would be great if you could look them up and review them. Maybe you can get some review in feedback.
Sam
Hi Sam,
On Sat, Sep 18, 2021 at 1:29 PM Sam Ravnborg sam@ravnborg.org wrote:
Hi Philip, On Sat, Sep 18, 2021 at 10:21:17AM -0700, Philip Chen wrote:
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 philipchen@chromium.org
Patch is: Signed-off-by: Sam Ravnborg sam@ravnborg.org
please consider a few follow-up patches:
- Replace deprecated drm_bridge_funcs with the _atomic_ variants.
- Replace the deprecated drm_bridge_chain_pre_enable() with the atomic variant.
- Use pr_() and dev_() logging in favour of DRM_ logging. DRM_ logging is deprecated these days and do not belong in bridge drivers anyway.
Maxime has a few patches pending for this driver - it would be great if you could look them up and review them. Maybe you can get some review in feedback.
Yes, I'll do. Thanks for reviewing.
Sam
Hi,
On Sat, Sep 18, 2021 at 1:29 PM Sam Ravnborg sam@ravnborg.org wrote:
Hi Philip, On Sat, Sep 18, 2021 at 10:21:17AM -0700, Philip Chen wrote:
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 philipchen@chromium.org
Patch is: Signed-off-by: Sam Ravnborg sam@ravnborg.org
I'm curious: did you mean "Signed-off-by" or "Acked-by" here?
-Doug
On Mon, Sep 20, 2021 at 02:42:09PM -0700, Doug Anderson wrote:
Hi,
On Sat, Sep 18, 2021 at 1:29 PM Sam Ravnborg sam@ravnborg.org wrote:
Hi Philip, On Sat, Sep 18, 2021 at 10:21:17AM -0700, Philip Chen wrote:
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 philipchen@chromium.org
Patch is: Signed-off-by: Sam Ravnborg sam@ravnborg.org
I'm curious: did you mean "Signed-off-by" or "Acked-by" here?
Should have been - thanks for noticing. Reviewed-by: Sam Ravnborg sam@ravnborg.org
Sam
Hi,
On Sat, Sep 18, 2021 at 10:21 AM Philip Chen philipchen@chromium.org wrote:
+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];struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev;unsigned int len = msg->size;
nit: usually no blank lines in the variable definition section.
base = PAGE0_SWAUX_ADDR_7_0;addr_len[PAGE0_SWAUX_ADDR_7_0 - base] = msg->address;addr_len[PAGE0_SWAUX_ADDR_15_8 - base] = msg->address >> 8;addr_len[PAGE0_SWAUX_ADDR_23_16 - base] = (msg->address >> 16) &SWAUX_ADDR_19_16_MASK;addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= (msg->request << 4) &SWAUX_CMD_MASK;
optional nit: Probably you could get rid of the mask for the request. After all, you're storing it to a thing that's a byte (so bits above bit 7 will implicitly be masked) and you're left shifting by 4 (so bits 0-3 will implicitly be masked) so this just makes it uglier. ;-)
optional nit: In theory you could also get rid of the SWAUX_ADDR_19_16_MASK and if you really wanted to you could error check that the address wasn't bigger than 20-bits since giving an error for an invalid address would actually be better than silently masking it anyway...
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_SWAUX_RDATA,(unsigned int *)(buf + i));
The cast to "unsigned int *" looks wrong to me. You can't just cast like this for a number of reasons. Go back to reading into a local variable and copy the byte into your buffer.
Other than the regmap_read() this looks fine to me. If you send a v6 with that fixed I'll plan to wait a day or two and then apply it with Sam's tags.
-Doug
Hi
On Tue, Sep 21, 2021 at 9:02 AM Doug Anderson dianders@chromium.org wrote:
Hi,
On Sat, Sep 18, 2021 at 10:21 AM Philip Chen philipchen@chromium.org wrote:
+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];struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev;unsigned int len = msg->size;nit: usually no blank lines in the variable definition section.
Fixed in v6. PTAL.
base = PAGE0_SWAUX_ADDR_7_0;addr_len[PAGE0_SWAUX_ADDR_7_0 - base] = msg->address;addr_len[PAGE0_SWAUX_ADDR_15_8 - base] = msg->address >> 8;addr_len[PAGE0_SWAUX_ADDR_23_16 - base] = (msg->address >> 16) &SWAUX_ADDR_19_16_MASK;addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= (msg->request << 4) &SWAUX_CMD_MASK;optional nit: Probably you could get rid of the mask for the request. After all, you're storing it to a thing that's a byte (so bits above bit 7 will implicitly be masked) and you're left shifting by 4 (so bits 0-3 will implicitly be masked) so this just makes it uglier. ;-)
Fixed in v6. PTAL.
optional nit: In theory you could also get rid of the SWAUX_ADDR_19_16_MASK and if you really wanted to you could error check that the address wasn't bigger than 20-bits since giving an error for an invalid address would actually be better than silently masking it anyway...
Fixed in v6. 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_SWAUX_RDATA,(unsigned int *)(buf + i));The cast to "unsigned int *" looks wrong to me. You can't just cast like this for a number of reasons. Go back to reading into a local variable and copy the byte into your buffer.
Previously I was not 100% sure about this change either. Now I'm sure it is bad after some experiments. In v6, I reverted to how this was handled in v3. PTAL.
Other than the regmap_read() this looks fine to me. If you send a v6 with that fixed I'll plan to wait a day or two and then apply it with Sam's tags.
-Doug
Hi Philip,
On Sat, Sep 18, 2021 at 10:21:16AM -0700, Philip Chen wrote:
Replace the direct i2c access (i2c_smbus_* functions) with regmap APIs, which will simplify the future update on ps8640 driver.
Reviewed-by: Douglas Anderson dianders@chromium.org Signed-off-by: Philip Chen philipchen@chromium.org
Looks good, Acked-by: Sam Ravnborg sam@ravnborg.org
dri-devel@lists.freedesktop.org
-
Doug Anderson -
Philip Chen -
Sam Ravnborg