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