| Message ID | 20210908111500.1.I9f6dac462da830fa0a8ccccbe977ca918bf14e4a@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/bridge: parade-ps8640: Use regmap APIs | expand |
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);
Hi, On Wed, Sep 8, 2021 at 2:54 PM Stephen Boyd <swboyd@chromium.org> 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:29:19) > Hi, > > On Wed, Sep 8, 2021 at 2:54 PM Stephen Boyd <swboyd@chromium.org> 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.
Hi, On Thu, Sep 9, 2021 at 12:09 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Philip Chen (2021-09-09 11:29:19) > > Hi, > > > > On Wed, Sep 8, 2021 at 2:54 PM Stephen Boyd <swboyd@chromium.org> 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
Quoting Doug Anderson (2021-09-09 14:14:29) > On Thu, Sep 9, 2021 at 12:09 PM Stephen Boyd <swboyd@chromium.org> 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.
On Thu, Sep 9, 2021 at 2:27 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Doug Anderson (2021-09-09 14:14:29) > > On Thu, Sep 9, 2021 at 12:09 PM Stephen Boyd <swboyd@chromium.org> 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.
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);
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> --- drivers/gpu/drm/bridge/parade-ps8640.c | 66 +++++++++++++++----------- 1 file changed, 39 insertions(+), 27 deletions(-)