Hi Brian, On Fri, Sep 20, 2019 at 07:28:59AM -0400, Brian Masney wrote: > On Fri, Sep 20, 2019 at 01:49:45PM +0300, Laurent Pinchart wrote: > > On Fri, Sep 20, 2019 at 06:14:38AM -0400, Brian Masney wrote: > >> According to the downstream Android sources, the anx7808 variants use > >> address 0x78 for TX_P0 and the anx781x variants use address 0x70. Since > >> the datasheets aren't available for these devices, and we only have the > >> downstream kernel sources to look at, let's assume that these addresses > >> are fixed based on the model, and pass the i2c addresses to the data > >> pointer in the driver's of_match_table. > >> > >> Signed-off-by: Brian Masney <masneyb@xxxxxxxxxxxxx> > >> --- > >> V1 of this patch with some discussion: > >> https://lore.kernel.org/lkml/20190815004854.19860-6-masneyb@xxxxxxxxxxxxx/ > >> > >> drivers/gpu/drm/bridge/analogix-anx78xx.c | 36 +++++++++++++++-------- > >> drivers/gpu/drm/bridge/analogix-anx78xx.h | 7 ----- > >> 2 files changed, 23 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c > >> index 48adf010816c..e25fae36dbe1 100644 > >> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c > >> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c > >> @@ -38,12 +38,20 @@ > >> #define AUX_CH_BUFFER_SIZE 16 > >> #define AUX_WAIT_TIMEOUT_MS 15 > >> > >> -static const u8 anx78xx_i2c_addresses[] = { > >> - [I2C_IDX_TX_P0] = TX_P0, > >> - [I2C_IDX_TX_P1] = TX_P1, > >> - [I2C_IDX_TX_P2] = TX_P2, > >> - [I2C_IDX_RX_P0] = RX_P0, > >> - [I2C_IDX_RX_P1] = RX_P1, > >> +static const u8 anx7808_i2c_addresses[] = { > >> + [I2C_IDX_TX_P0] = 0x78, > >> + [I2C_IDX_TX_P1] = 0x7a, > >> + [I2C_IDX_TX_P2] = 0x72, > >> + [I2C_IDX_RX_P0] = 0x7e, > >> + [I2C_IDX_RX_P1] = 0x80, > >> +}; > >> + > >> +static const u8 anx781x_i2c_addresses[] = { > >> + [I2C_IDX_TX_P0] = 0x70, > >> + [I2C_IDX_TX_P1] = 0x7a, > >> + [I2C_IDX_TX_P2] = 0x72, > >> + [I2C_IDX_RX_P0] = 0x7e, > >> + [I2C_IDX_RX_P1] = 0x80, > >> }; > > > > If those addresses are really fixed they should have been added to DT, > > in order for all fixed I2C addresses used on an I2C bus to be reported > > in DT. I guess it's too late now :-/ This seems to be the best we can do > > to solve the problem with existing DT. Updating the bindings could > > however still be a good idea to make this clear moving forward. > > One thing to be aware of is that these addresses are actually divided by > two in order to get the real i2c address. > > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L1353 > > I'm not sure why the addresses are represented this way but downstream > does this as well. If I recall correctly, 0x77 is the maximum i2c > address on a bus, and there's a 0x80 above. Sure, the addresses in DT would be half of the above values :-) > >> struct anx78xx_platform_data { > >> @@ -1348,6 +1356,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client, > >> struct anx78xx *anx78xx; > >> struct anx78xx_platform_data *pdata; > >> unsigned int i, idl, idh, version; > >> + const u8 *i2c_addresses; > >> bool found = false; > >> int err; > >> > >> @@ -1387,15 +1396,16 @@ static int anx78xx_i2c_probe(struct i2c_client *client, > >> } > >> > >> /* Map slave addresses of ANX7814 */ > >> + i2c_addresses = device_get_match_data(&client->dev); > >> for (i = 0; i < I2C_NUM_ADDRESSES; i++) { > >> struct i2c_client *i2c_dummy; > >> > >> i2c_dummy = i2c_new_dummy_device(client->adapter, > >> - anx78xx_i2c_addresses[i] >> 1); > >> + i2c_addresses[i] >> 1); > >> if (IS_ERR(i2c_dummy)) { > >> err = PTR_ERR(i2c_dummy); > >> DRM_ERROR("Failed to reserve I2C bus %02x: %d\n", > >> - anx78xx_i2c_addresses[i], err); > >> + i2c_addresses[i], err); > >> goto err_unregister_i2c; > >> } > >> > >> @@ -1405,7 +1415,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client, > >> if (IS_ERR(anx78xx->map[i])) { > >> err = PTR_ERR(anx78xx->map[i]); > >> DRM_ERROR("Failed regmap initialization %02x\n", > >> - anx78xx_i2c_addresses[i]); > >> + i2c_addresses[i]); > >> goto err_unregister_i2c; > >> } > >> } > >> @@ -1504,10 +1514,10 @@ MODULE_DEVICE_TABLE(i2c, anx78xx_id); > >> > >> #if IS_ENABLED(CONFIG_OF) > >> static const struct of_device_id anx78xx_match_table[] = { > >> - { .compatible = "analogix,anx7808", }, > >> - { .compatible = "analogix,anx7812", }, > >> - { .compatible = "analogix,anx7814", }, > >> - { .compatible = "analogix,anx7818", }, > >> + { .compatible = "analogix,anx7808", .data = anx7808_i2c_addresses }, > >> + { .compatible = "analogix,anx7812", .data = anx781x_i2c_addresses }, > >> + { .compatible = "analogix,anx7814", .data = anx781x_i2c_addresses }, > >> + { .compatible = "analogix,anx7818", .data = anx781x_i2c_addresses }, > >> { /* sentinel */ }, > >> }; > >> MODULE_DEVICE_TABLE(of, anx78xx_match_table); > >> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h b/drivers/gpu/drm/bridge/analogix-anx78xx.h > >> index 25e063bcecbc..8697647709f7 100644 > >> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.h > >> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.h > >> @@ -6,13 +6,6 @@ > >> #ifndef __ANX78xx_H > >> #define __ANX78xx_H > >> > >> -#define TX_P0 0x70 > >> -#define TX_P1 0x7a > >> -#define TX_P2 0x72 > >> - > >> -#define RX_P0 0x7e > >> -#define RX_P1 0x80 > >> - > >> /***************************************************************/ > >> /* Register definition of device address 0x7e */ > >> /***************************************************************/ > > > > Should you also rename the headers to mention [RT]X P[012] instead of > > the numerical addresses ? > > Have #defines for ANX781X_TX_P0, ANX7808_TX_P0, etc in the header file? No, I mean -/* Register definition of device address 0x7e */ +/* Register definitions for RX P0 */ or something similar (I'm not sure what P0 means). > > Apart from that, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
