| Message ID | 20210913143255.RFC.v2.1.I8ad7a535bb18a1f41f3858f83379beedb397a9db@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2,1/2] drm/bridge: parade-ps8640: Use regmap APIs | expand |
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>
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>
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);
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(-)