Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 20 Sep 2019 15:09:52 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Brian Masney <masneyb@onstation.org>
Cc:     a.hajda@samsung.com, narmstrong@baylibre.com, jonas@kwiboo.se,
        jernej.skrabec@siol.net, airlied@linux.ie, daniel@ffwll.ch,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        enric.balletbo@collabora.com
Subject: Re: [PATCH v2] drm/bridge: analogix-anx78xx: add support for 7808
 addresses
Message-ID: <20190920120952.GG12950@pendragon.ideasonboard.com>
References: <20190920101438.6912-1-masneyb@onstation.org>
 <20190920104945.GC12950@pendragon.ideasonboard.com>
 <20190920112859.GA32015@onstation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20190920112859.GA32015@onstation.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@onstation.org>
> >> ---
> >> V1 of this patch with some discussion:
> >> https://lore.kernel.org/lkml/20190815004854.19860-6-masneyb@onstation.org/
> >> 
> >>  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@ideasonboard.com>

-- 
Regards,

Laurent Pinchart