Re: [PATCH v2] drm/bridge: analogix-anx78xx: add support for 7808 addresses
From: Laurent Pinchart
Date: Fri Sep 20 2019 - 08:10:34 EST
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