| Message ID | 53B3D1F8.7080106@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
You missed me as the maintainer of the driver: NXP TDA998X DRM DRIVER M: Russell King <rmk+kernel@arm.linux.org.uk> S: Supported F: drivers/gpu/drm/i2c/tda998x_drv.c F: include/drm/i2c/tda998x.h On Wed, Jul 02, 2014 at 10:33:44AM +0100, Andrew Jackson wrote: > @@ -1274,9 +1275,9 @@ tda998x_encoder_init(struct i2c_client *client, > > priv->current_page = 0xff; > priv->hdmi = client; > - priv->cec = i2c_new_dummy(client->adapter, 0x34); > - if (!priv->cec) > - priv->cec = i2c_new_dummy(client->adapter, 0x35); > + /* CEC I2C address bound to TDA998x I2C addr by configuration pins */ > + cec_addr = 0x34 + (client->addr & 3); > + priv->cec = i2c_new_dummy(client->adapter, cec_addr); We discussed the issue of the I2C CEC address when the DT binding first came up... Do we know for certain that the above is always true for all the devices that this driver supports?
On 07/02/14 11:25, Russell King - ARM Linux wrote: > You missed me as the maintainer of the driver: Sorry, an over-sight on my part. > NXP TDA998X DRM DRIVER > M: Russell King <rmk+kernel@arm.linux.org.uk> > S: Supported > F: drivers/gpu/drm/i2c/tda998x_drv.c > F: include/drm/i2c/tda998x.h > > On Wed, Jul 02, 2014 at 10:33:44AM +0100, Andrew Jackson wrote: >> @@ -1274,9 +1275,9 @@ tda998x_encoder_init(struct i2c_client *client, >> >> priv->current_page = 0xff; >> priv->hdmi = client; >> - priv->cec = i2c_new_dummy(client->adapter, 0x34); >> - if (!priv->cec) >> - priv->cec = i2c_new_dummy(client->adapter, 0x35); >> + /* CEC I2C address bound to TDA998x I2C addr by configuration pins */ >> + cec_addr = 0x34 + (client->addr & 3); >> + priv->cec = i2c_new_dummy(client->adapter, cec_addr); > > We discussed the issue of the I2C CEC address when the DT binding first > came up... > > Do we know for certain that the above is always true for all the devices > that this driver supports? I know that it is true for the following: * TDA9989 (Both lower LSBs are zero) * TDA19988 (LSBs are set from configuration pins) * TDA19989 (Both lower LSBs are zero) I don't know about the N2 variants of the various devices. I have tested the patch on board containing two TDA19988s. Andrew -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
On Wed, Jul 02, 2014 at 11:45:56AM +0100, Andrew Jackson wrote: > On 07/02/14 11:25, Russell King - ARM Linux wrote: > > You missed me as the maintainer of the driver: > > Sorry, an over-sight on my part. > > > NXP TDA998X DRM DRIVER > > M: Russell King <rmk+kernel@arm.linux.org.uk> > > S: Supported > > F: drivers/gpu/drm/i2c/tda998x_drv.c > > F: include/drm/i2c/tda998x.h > > > > On Wed, Jul 02, 2014 at 10:33:44AM +0100, Andrew Jackson wrote: > >> @@ -1274,9 +1275,9 @@ tda998x_encoder_init(struct i2c_client *client, > >> > >> priv->current_page = 0xff; > >> priv->hdmi = client; > >> - priv->cec = i2c_new_dummy(client->adapter, 0x34); > >> - if (!priv->cec) > >> - priv->cec = i2c_new_dummy(client->adapter, 0x35); > >> + /* CEC I2C address bound to TDA998x I2C addr by configuration pins */ > >> + cec_addr = 0x34 + (client->addr & 3); > >> + priv->cec = i2c_new_dummy(client->adapter, cec_addr); > > > > We discussed the issue of the I2C CEC address when the DT binding first > > came up... > > > > Do we know for certain that the above is always true for all the devices > > that this driver supports? > > I know that it is true for the following: > > * TDA9989 (Both lower LSBs are zero) > * TDA19988 (LSBs are set from configuration pins) > * TDA19989 (Both lower LSBs are zero) > > I don't know about the N2 variants of the various devices. > > I have tested the patch on board containing two TDA19988s. Thanks, that's useful information to have. Also, it's worth pointing out that the patch is whitespace damaged - tabs are converted to spaces, which makes it unable to be applied. There's other reasons (see below) that makes us unable to apply this patch too... As we all know, email clients know better than us humans how to format email messages, which gives them carte blanche to screw up patches in this way. I'd recommend that you talk to others in ARM Ltd about how they get around this problem. > -- IMPORTANT NOTICE: The contents of this email and any attachments > are confidential and may also be privileged. If you are not the > intended recipient, please notify the sender immediately and do > not disclose the contents to any other person, use it for any > purpose, or store or copy the information in any medium. Thank you. This means that I can't take the patch, since applying it into a git tree and then publishing it would violate this statement. Doing so copies and stores the information, and makes it available to people who were not the intended recipient of your email.
On 07/02/14 16:34, Russell King - ARM Linux wrote: > On Wed, Jul 02, 2014 at 11:45:56AM +0100, Andrew Jackson wrote: >> On 07/02/14 11:25, Russell King - ARM Linux wrote: >>> On Wed, Jul 02, 2014 at 10:33:44AM +0100, Andrew Jackson wrote: > Thanks, that's useful information to have. Also, it's worth pointing out > that the patch is whitespace damaged - tabs are converted to spaces, > which makes it unable to be applied. There's other reasons (see below) > that makes us unable to apply this patch too... > > As we all know, email clients know better than us humans how to format > email messages, which gives them carte blanche to screw up patches in > this way. > > I'd recommend that you talk to others in ARM Ltd about how they get > around this problem. > >> -- IMPORTANT NOTICE: The contents of this email and any attachments >> are confidential and may also be privileged. If you are not the >> intended recipient, please notify the sender immediately and do >> not disclose the contents to any other person, use it for any >> purpose, or store or copy the information in any medium. Thank you. > > This means that I can't take the patch, since applying it into a git > tree and then publishing it would violate this statement. Doing so > copies and stores the information, and makes it available to people > who were not the intended recipient of your email. > Indeed. I've now defeated the signature issue, or so I assert, so I will re-submit a clean version of the patch with additional documentation regarding the information on the various TDA998x devices. Thank you for your tolerance. Andrew
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index c2a690d..08a2a26 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1260,6 +1260,7 @@ tda998x_encoder_init(struct i2c_client *client, struct device_node *np = client->dev.of_node; u32 video; int rev_lo, rev_hi, ret; + unsigned short cec_addr; priv = kzalloc(sizeof(*priv), GFP_KERNEL); if (!priv) @@ -1274,9 +1275,9 @@ tda998x_encoder_init(struct i2c_client *client, priv->current_page = 0xff; priv->hdmi = client; - priv->cec = i2c_new_dummy(client->adapter, 0x34); - if (!priv->cec) - priv->cec = i2c_new_dummy(client->adapter, 0x35); + /* CEC I2C address bound to TDA998x I2C addr by configuration pins */ + cec_addr = 0x34 + (client->addr & 3); + priv->cec = i2c_new_dummy(client->adapter, cec_addr); if (!priv->cec) { kfree(priv); return -ENODEV;
The two LSBs of the TDA998x's I2C address are set by pins on the chip. The associated CEC peripheral's I2C address is set in the same way, so base the CEC address on that of the TDA998x. Signed-off-by: Andrew Jackson <Andrew.Jackson@arm.com> --- drivers/gpu/drm/i2c/tda998x_drv.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) -- 1.7.1 -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782