All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] drm/bridge: analogix-anx78xx: add support for 7808 addresses
@ 2019-09-20 10:14 Brian Masney
  2019-09-20 10:49 ` Laurent Pinchart
  0 siblings, 1 reply; 5+ messages in thread
From: Brian Masney @ 2019-09-20 10:14 UTC (permalink / raw)
  To: a.hajda, narmstrong, Laurent.pinchart
  Cc: jonas, jernej.skrabec, airlied, daniel, dri-devel, linux-kernel,
	enric.balletbo

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,
 };
 
 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                  */
 /***************************************************************/
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/bridge: analogix-anx78xx: add support for 7808 addresses
  2019-09-20 10:14 [PATCH v2] drm/bridge: analogix-anx78xx: add support for 7808 addresses Brian Masney
@ 2019-09-20 10:49 ` Laurent Pinchart
  2019-09-20 11:28   ` Brian Masney
  0 siblings, 1 reply; 5+ messages in thread
From: Laurent Pinchart @ 2019-09-20 10:49 UTC (permalink / raw)
  To: Brian Masney
  Cc: a.hajda, narmstrong, jonas, jernej.skrabec, airlied, daniel,
	dri-devel, linux-kernel, enric.balletbo

Hi Brian,

Thank you for the patch.

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.

>  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 ?

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/bridge: analogix-anx78xx: add support for 7808 addresses
  2019-09-20 10:49 ` Laurent Pinchart
@ 2019-09-20 11:28   ` Brian Masney
  2019-09-20 12:09       ` Laurent Pinchart
  0 siblings, 1 reply; 5+ messages in thread
From: Brian Masney @ 2019-09-20 11:28 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: a.hajda, narmstrong, jonas, jernej.skrabec, airlied, daniel,
	dri-devel, linux-kernel, enric.balletbo

On Fri, Sep 20, 2019 at 01:49:45PM +0300, Laurent Pinchart wrote:
> Hi Brian,
> 
> Thank you for the patch.
> 
> 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.

> 
> >  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?

> 
> Apart from that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks,

Brian

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/bridge: analogix-anx78xx: add support for 7808 addresses
  2019-09-20 11:28   ` Brian Masney
@ 2019-09-20 12:09       ` Laurent Pinchart
  0 siblings, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2019-09-20 12:09 UTC (permalink / raw)
  To: Brian Masney
  Cc: a.hajda, narmstrong, jonas, jernej.skrabec, airlied, daniel,
	dri-devel, linux-kernel, enric.balletbo

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/bridge: analogix-anx78xx: add support for 7808 addresses
@ 2019-09-20 12:09       ` Laurent Pinchart
  0 siblings, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2019-09-20 12:09 UTC (permalink / raw)
  To: Brian Masney
  Cc: jernej.skrabec, jonas, airlied, narmstrong, linux-kernel,
	dri-devel, enric.balletbo

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-20 12:10 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-20 10:14 [PATCH v2] drm/bridge: analogix-anx78xx: add support for 7808 addresses Brian Masney
2019-09-20 10:49 ` Laurent Pinchart
2019-09-20 11:28   ` Brian Masney
2019-09-20 12:09     ` Laurent Pinchart
2019-09-20 12:09       ` Laurent Pinchart

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.