diff mbox series

[05/11] drm/bridge: analogix-anx78xx: correct value of TX_P0

Message ID 20190815004854.19860-6-masneyb@onstation.org (mailing list archive)
State New, archived
Headers show
Series ARM: dts: qcom: msm8974: add support for external display | expand

Commit Message

Brian Masney Aug. 15, 2019, 12:48 a.m. UTC
When attempting to configure this driver on a Nexus 5 phone (msm8974),
setting up the dummy i2c bus for TX_P0 would fail due to an -EBUSY
error. The downstream MSM kernel sources [1] shows that the proper value
for TX_P0 is 0x78, not 0x70, so correct the value to allow device
probing to succeed.

[1] https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/video/slimport/slimport_tx_reg.h

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/gpu/drm/bridge/analogix-anx78xx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrzej Hajda Sept. 16, 2019, 10:02 a.m. UTC | #1
On 15.08.2019 02:48, Brian Masney wrote:
> When attempting to configure this driver on a Nexus 5 phone (msm8974),
> setting up the dummy i2c bus for TX_P0 would fail due to an -EBUSY
> error. The downstream MSM kernel sources [1] shows that the proper value
> for TX_P0 is 0x78, not 0x70, so correct the value to allow device
> probing to succeed.
>
> [1] https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/video/slimport/slimport_tx_reg.h
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/gpu/drm/bridge/analogix-anx78xx.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h b/drivers/gpu/drm/bridge/analogix-anx78xx.h
> index 25e063bcecbc..bc511fc605c9 100644
> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.h
> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.h
> @@ -6,7 +6,7 @@
>  #ifndef __ANX78xx_H
>  #define __ANX78xx_H
>  
> -#define TX_P0				0x70
> +#define TX_P0				0x78


This bothers me little. There are no upstream users, grepping android
sources suggests that both values can be used [1][2]  (grep for "#define
TX_P0"), moreover there is code suggesting both values can be valid [3].

Could you verify datasheet which i2c slave addresses are valid for this
chip, if both I guess this patch should be reworked.


[1]:
https://android.googlesource.com/kernel/msm/+/android-msm-flo-3.4-jb-mr2/drivers/misc/slimport_anx7808/slimport_tx_reg.h

[2]:
https://github.com/AndroidGX/SimpleGX-MM-6.0_H815_20d/blob/master/drivers/video/slimport/anx7812/slimport7812_tx_reg.h

[3]:
https://github.com/commaai/android_kernel_leeco_msm8996/blob/master/drivers/video/msm/mdss/dp/slimport_custom_declare.h#L73


Regards

Andrzej


>  #define TX_P1				0x7a
>  #define TX_P2				0x72
>
Brian Masney Sept. 16, 2019, 10:36 a.m. UTC | #2
On Mon, Sep 16, 2019 at 12:02:09PM +0200, Andrzej Hajda wrote:
> On 15.08.2019 02:48, Brian Masney wrote:
> > When attempting to configure this driver on a Nexus 5 phone (msm8974),
> > setting up the dummy i2c bus for TX_P0 would fail due to an -EBUSY
> > error. The downstream MSM kernel sources [1] shows that the proper value
> > for TX_P0 is 0x78, not 0x70, so correct the value to allow device
> > probing to succeed.
> >
> > [1] https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/video/slimport/slimport_tx_reg.h
> >
> > Signed-off-by: Brian Masney <masneyb@onstation.org>
> > ---
> >  drivers/gpu/drm/bridge/analogix-anx78xx.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h b/drivers/gpu/drm/bridge/analogix-anx78xx.h
> > index 25e063bcecbc..bc511fc605c9 100644
> > --- a/drivers/gpu/drm/bridge/analogix-anx78xx.h
> > +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.h
> > @@ -6,7 +6,7 @@
> >  #ifndef __ANX78xx_H
> >  #define __ANX78xx_H
> >  
> > -#define TX_P0				0x70
> > +#define TX_P0				0x78
> 
> 
> This bothers me little. There are no upstream users, grepping android
> sources suggests that both values can be used [1][2]  (grep for "#define
> TX_P0"), moreover there is code suggesting both values can be valid [3].
> 
> Could you verify datasheet which i2c slave addresses are valid for this
> chip, if both I guess this patch should be reworked.
> 
> 
> [1]:
> https://android.googlesource.com/kernel/msm/+/android-msm-flo-3.4-jb-mr2/drivers/misc/slimport_anx7808/slimport_tx_reg.h
> 
> [2]:
> https://github.com/AndroidGX/SimpleGX-MM-6.0_H815_20d/blob/master/drivers/video/slimport/anx7812/slimport7812_tx_reg.h
> 
> [3]:
> https://github.com/commaai/android_kernel_leeco_msm8996/blob/master/drivers/video/msm/mdss/dp/slimport_custom_declare.h#L73

This address is 0x78 on my Nexus 5. Given [3] above it looks like we
need to support both addresses. What do you think about moving these
addresses into device tree?

The downstream and upstream kernel sources divide these addresses by two
to get the i2c address. Here's the code in upstream:

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L1353
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L41

I'm not sure why the actual i2c address isn't used in this code.

Brian
Enric Balletbo i Serra Sept. 16, 2019, 10:36 a.m. UTC | #3
Hi Andrzej and Brian

On 16/9/19 12:02, Andrzej Hajda wrote:
> On 15.08.2019 02:48, Brian Masney wrote:
>> When attempting to configure this driver on a Nexus 5 phone (msm8974),
>> setting up the dummy i2c bus for TX_P0 would fail due to an -EBUSY
>> error. The downstream MSM kernel sources [1] shows that the proper value
>> for TX_P0 is 0x78, not 0x70, so correct the value to allow device
>> probing to succeed.
>>
>> [1] https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/video/slimport/slimport_tx_reg.h
>>
>> Signed-off-by: Brian Masney <masneyb@onstation.org>
>> ---
>>  drivers/gpu/drm/bridge/analogix-anx78xx.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h b/drivers/gpu/drm/bridge/analogix-anx78xx.h
>> index 25e063bcecbc..bc511fc605c9 100644
>> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.h
>> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.h
>> @@ -6,7 +6,7 @@
>>  #ifndef __ANX78xx_H
>>  #define __ANX78xx_H
>>  
>> -#define TX_P0				0x70
>> +#define TX_P0				0x78
> 
> 
> This bothers me little. There are no upstream users, grepping android
> sources suggests that both values can be used [1][2]  (grep for "#define
> TX_P0"), moreover there is code suggesting both values can be valid [3].
> 
> Could you verify datasheet which i2c slave addresses are valid for this
> chip, if both I guess this patch should be reworked.
> 

On my case the valid i2c slave address is 0x70 (from datasheet, very sorry I
can't share it) and the bridge used is an ANX7814, it could be that ANX7808 or
ANX7812 have different slave addresses?

Regards,
 Enric

> 
> [1]:
> https://android.googlesource.com/kernel/msm/+/android-msm-flo-3.4-jb-mr2/drivers/misc/slimport_anx7808/slimport_tx_reg.h
> 
> [2]:
> https://github.com/AndroidGX/SimpleGX-MM-6.0_H815_20d/blob/master/drivers/video/slimport/anx7812/slimport7812_tx_reg.h
> 
> [3]:
> https://github.com/commaai/android_kernel_leeco_msm8996/blob/master/drivers/video/msm/mdss/dp/slimport_custom_declare.h#L73
> 
> 
> Regards
> 
> Andrzej
> 
> 
>>  #define TX_P1				0x7a
>>  #define TX_P2				0x72
>>  
> 
>
Brian Masney Sept. 16, 2019, 10:40 a.m. UTC | #4
On Mon, Sep 16, 2019 at 12:36:19PM +0200, Enric Balletbo i Serra wrote:
> Hi Andrzej and Brian
> 
> On 16/9/19 12:02, Andrzej Hajda wrote:
> > On 15.08.2019 02:48, Brian Masney wrote:
> >> When attempting to configure this driver on a Nexus 5 phone (msm8974),
> >> setting up the dummy i2c bus for TX_P0 would fail due to an -EBUSY
> >> error. The downstream MSM kernel sources [1] shows that the proper value
> >> for TX_P0 is 0x78, not 0x70, so correct the value to allow device
> >> probing to succeed.
> >>
> >> [1] https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/video/slimport/slimport_tx_reg.h
> >>
> >> Signed-off-by: Brian Masney <masneyb@onstation.org>
> >> ---
> >>  drivers/gpu/drm/bridge/analogix-anx78xx.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h b/drivers/gpu/drm/bridge/analogix-anx78xx.h
> >> index 25e063bcecbc..bc511fc605c9 100644
> >> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.h
> >> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.h
> >> @@ -6,7 +6,7 @@
> >>  #ifndef __ANX78xx_H
> >>  #define __ANX78xx_H
> >>  
> >> -#define TX_P0				0x70
> >> +#define TX_P0				0x78
> > 
> > 
> > This bothers me little. There are no upstream users, grepping android
> > sources suggests that both values can be used [1][2]  (grep for "#define
> > TX_P0"), moreover there is code suggesting both values can be valid [3].
> > 
> > Could you verify datasheet which i2c slave addresses are valid for this
> > chip, if both I guess this patch should be reworked.
> > 
> 
> On my case the valid i2c slave address is 0x70 (from datasheet, very sorry I
> can't share it) and the bridge used is an ANX7814, it could be that ANX7808 or
> ANX7812 have different slave addresses?

I haven't been able to find any of the datasheets for these devices
online. Product briefs are online (such as 
https://www.analogix.com/en/system/files/ANX7808_product_brief.pdf), but
they don't provide this type of information.

Brian
Laurent Pinchart Sept. 16, 2019, 10:49 a.m. UTC | #5
Hi Brian,

On Mon, Sep 16, 2019 at 06:36:14AM -0400, Brian Masney wrote:
> On Mon, Sep 16, 2019 at 12:02:09PM +0200, Andrzej Hajda wrote:
> > On 15.08.2019 02:48, Brian Masney wrote:
> > > When attempting to configure this driver on a Nexus 5 phone (msm8974),
> > > setting up the dummy i2c bus for TX_P0 would fail due to an -EBUSY
> > > error. The downstream MSM kernel sources [1] shows that the proper value
> > > for TX_P0 is 0x78, not 0x70, so correct the value to allow device
> > > probing to succeed.
> > >
> > > [1] https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/video/slimport/slimport_tx_reg.h
> > >
> > > Signed-off-by: Brian Masney <masneyb@onstation.org>
> > > ---
> > >  drivers/gpu/drm/bridge/analogix-anx78xx.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h b/drivers/gpu/drm/bridge/analogix-anx78xx.h
> > > index 25e063bcecbc..bc511fc605c9 100644
> > > --- a/drivers/gpu/drm/bridge/analogix-anx78xx.h
> > > +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.h
> > > @@ -6,7 +6,7 @@
> > >  #ifndef __ANX78xx_H
> > >  #define __ANX78xx_H
> > >  
> > > -#define TX_P0				0x70
> > > +#define TX_P0				0x78
> > 
> > 
> > This bothers me little. There are no upstream users, grepping android
> > sources suggests that both values can be used [1][2]  (grep for "#define
> > TX_P0"), moreover there is code suggesting both values can be valid [3].
> > 
> > Could you verify datasheet which i2c slave addresses are valid for this
> > chip, if both I guess this patch should be reworked.
> > 
> > 
> > [1]:
> > https://android.googlesource.com/kernel/msm/+/android-msm-flo-3.4-jb-mr2/drivers/misc/slimport_anx7808/slimport_tx_reg.h
> > 
> > [2]:
> > https://github.com/AndroidGX/SimpleGX-MM-6.0_H815_20d/blob/master/drivers/video/slimport/anx7812/slimport7812_tx_reg.h
> > 
> > [3]:
> > https://github.com/commaai/android_kernel_leeco_msm8996/blob/master/drivers/video/msm/mdss/dp/slimport_custom_declare.h#L73
> 
> This address is 0x78 on my Nexus 5. Given [3] above it looks like we
> need to support both addresses. What do you think about moving these
> addresses into device tree?

Assuming that the device supports different addresses (I can't validate
that as I don't have access to the datasheet), and different addresses
need to be used on different systems, then the address to be used needs
to be provided by the firmware (DT in this case). Two options are
possible, either specifying the address explicitly in the device's DT
node, or specifying free addresses (in the form of a white list or black
list) and allocating an address from that pool. The latter has been
discussed in a BoF at the Linux Plumbers Conference last week,
https://linuxplumbersconf.org/event/4/contributions/542/.

> The downstream and upstream kernel sources divide these addresses by two
> to get the i2c address. Here's the code in upstream:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L1353
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L41
> 
> I'm not sure why the actual i2c address isn't used in this code.
Enric Balletbo i Serra Sept. 16, 2019, 11:32 a.m. UTC | #6
Hi,

On 16/9/19 12:49, Laurent Pinchart wrote:
> Hi Brian,
> 
> On Mon, Sep 16, 2019 at 06:36:14AM -0400, Brian Masney wrote:
>> On Mon, Sep 16, 2019 at 12:02:09PM +0200, Andrzej Hajda wrote:
>>> On 15.08.2019 02:48, Brian Masney wrote:
>>>> When attempting to configure this driver on a Nexus 5 phone (msm8974),
>>>> setting up the dummy i2c bus for TX_P0 would fail due to an -EBUSY
>>>> error. The downstream MSM kernel sources [1] shows that the proper value
>>>> for TX_P0 is 0x78, not 0x70, so correct the value to allow device
>>>> probing to succeed.
>>>>
>>>> [1] https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/video/slimport/slimport_tx_reg.h
>>>>
>>>> Signed-off-by: Brian Masney <masneyb@onstation.org>
>>>> ---
>>>>  drivers/gpu/drm/bridge/analogix-anx78xx.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h b/drivers/gpu/drm/bridge/analogix-anx78xx.h
>>>> index 25e063bcecbc..bc511fc605c9 100644
>>>> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.h
>>>> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.h
>>>> @@ -6,7 +6,7 @@
>>>>  #ifndef __ANX78xx_H
>>>>  #define __ANX78xx_H
>>>>  
>>>> -#define TX_P0				0x70
>>>> +#define TX_P0				0x78
>>>
>>>
>>> This bothers me little. There are no upstream users, grepping android
>>> sources suggests that both values can be used [1][2]  (grep for "#define
>>> TX_P0"), moreover there is code suggesting both values can be valid [3].
>>>
>>> Could you verify datasheet which i2c slave addresses are valid for this
>>> chip, if both I guess this patch should be reworked.
>>>
>>>
>>> [1]:
>>> https://android.googlesource.com/kernel/msm/+/android-msm-flo-3.4-jb-mr2/drivers/misc/slimport_anx7808/slimport_tx_reg.h
>>>
>>> [2]:
>>> https://github.com/AndroidGX/SimpleGX-MM-6.0_H815_20d/blob/master/drivers/video/slimport/anx7812/slimport7812_tx_reg.h
>>>
>>> [3]:
>>> https://github.com/commaai/android_kernel_leeco_msm8996/blob/master/drivers/video/msm/mdss/dp/slimport_custom_declare.h#L73
>>
>> This address is 0x78 on my Nexus 5. Given [3] above it looks like we
>> need to support both addresses. What do you think about moving these
>> addresses into device tree?
> 
> Assuming that the device supports different addresses (I can't validate
> that as I don't have access to the datasheet), and different addresses
> need to be used on different systems, then the address to be used needs
> to be provided by the firmware (DT in this case). Two options are
> possible, either specifying the address explicitly in the device's DT
> node, or specifying free addresses (in the form of a white list or black
> list) and allocating an address from that pool. The latter has been
> discussed in a BoF at the Linux Plumbers Conference last week,
> https://linuxplumbersconf.org/event/4/contributions/542/.
> 
>> The downstream and upstream kernel sources divide these addresses by two
>> to get the i2c address. Here's the code in upstream:
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L1353
>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L41
>>
>> I'm not sure why the actual i2c address isn't used in this code.
> 

The ANX7802/12/14/16 has a slave I2C bus that provides the interface to access
or control the chip from the AP. The I2C slave addresses used to control the
ANX7802/12/14/16 are 70h, 72h, 7Ah, 7Eh and 80h. Every address allows you to
access to different registers of the chip and AFAICS is not configurable.

I don't think these addresses should be configured via DT but for the driver itself.

My wild guess is that the ANX7808 has different addresses, but I don't have the
datasheet of this version.

Best regards,
 Enric
Brian Masney Sept. 16, 2019, 12:02 p.m. UTC | #7
On Mon, Sep 16, 2019 at 01:32:58PM +0200, Enric Balletbo i Serra wrote:
> Hi,
> 
> On 16/9/19 12:49, Laurent Pinchart wrote:
> > Hi Brian,
> > 
> > On Mon, Sep 16, 2019 at 06:36:14AM -0400, Brian Masney wrote:
> >> On Mon, Sep 16, 2019 at 12:02:09PM +0200, Andrzej Hajda wrote:
> >>> On 15.08.2019 02:48, Brian Masney wrote:
> >>>> When attempting to configure this driver on a Nexus 5 phone (msm8974),
> >>>> setting up the dummy i2c bus for TX_P0 would fail due to an -EBUSY
> >>>> error. The downstream MSM kernel sources [1] shows that the proper value
> >>>> for TX_P0 is 0x78, not 0x70, so correct the value to allow device
> >>>> probing to succeed.
> >>>>
> >>>> [1] https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/video/slimport/slimport_tx_reg.h
> >>>>
> >>>> Signed-off-by: Brian Masney <masneyb@onstation.org>
> >>>> ---
> >>>>  drivers/gpu/drm/bridge/analogix-anx78xx.h | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h b/drivers/gpu/drm/bridge/analogix-anx78xx.h
> >>>> index 25e063bcecbc..bc511fc605c9 100644
> >>>> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.h
> >>>> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.h
> >>>> @@ -6,7 +6,7 @@
> >>>>  #ifndef __ANX78xx_H
> >>>>  #define __ANX78xx_H
> >>>>  
> >>>> -#define TX_P0				0x70
> >>>> +#define TX_P0				0x78
> >>>
> >>>
> >>> This bothers me little. There are no upstream users, grepping android
> >>> sources suggests that both values can be used [1][2]  (grep for "#define
> >>> TX_P0"), moreover there is code suggesting both values can be valid [3].
> >>>
> >>> Could you verify datasheet which i2c slave addresses are valid for this
> >>> chip, if both I guess this patch should be reworked.
> >>>
> >>>
> >>> [1]:
> >>> https://android.googlesource.com/kernel/msm/+/android-msm-flo-3.4-jb-mr2/drivers/misc/slimport_anx7808/slimport_tx_reg.h
> >>>
> >>> [2]:
> >>> https://github.com/AndroidGX/SimpleGX-MM-6.0_H815_20d/blob/master/drivers/video/slimport/anx7812/slimport7812_tx_reg.h
> >>>
> >>> [3]:
> >>> https://github.com/commaai/android_kernel_leeco_msm8996/blob/master/drivers/video/msm/mdss/dp/slimport_custom_declare.h#L73
> >>
> >> This address is 0x78 on my Nexus 5. Given [3] above it looks like we
> >> need to support both addresses. What do you think about moving these
> >> addresses into device tree?
> > 
> > Assuming that the device supports different addresses (I can't validate
> > that as I don't have access to the datasheet), and different addresses
> > need to be used on different systems, then the address to be used needs
> > to be provided by the firmware (DT in this case). Two options are
> > possible, either specifying the address explicitly in the device's DT
> > node, or specifying free addresses (in the form of a white list or black
> > list) and allocating an address from that pool. The latter has been
> > discussed in a BoF at the Linux Plumbers Conference last week,
> > https://linuxplumbersconf.org/event/4/contributions/542/.
> > 
> >> The downstream and upstream kernel sources divide these addresses by two
> >> to get the i2c address. Here's the code in upstream:
> >>
> >> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L1353
> >> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L41
> >>
> >> I'm not sure why the actual i2c address isn't used in this code.
> > 
> 
> The ANX7802/12/14/16 has a slave I2C bus that provides the interface to access
> or control the chip from the AP. The I2C slave addresses used to control the
> ANX7802/12/14/16 are 70h, 72h, 7Ah, 7Eh and 80h. Every address allows you to
> access to different registers of the chip and AFAICS is not configurable.
> 
> I don't think these addresses should be configured via DT but for the driver itself.
> 
> My wild guess is that the ANX7808 has different addresses, but I don't have the
> datasheet of this version.

I'm able to communicate with the 7808 on my Nexus 5 using the 0x78
address. Given that the addresses appear to be fixed per model, maybe it
makes sense to drop the address #defines and add the addresses to the
data pointer in the driver's of_match_table like so:

static const struct of_device_id anx78xx_match_table[] = {
        { .compatible = "analogix,anx7808", .data = PTR_TO_7808_ADDRS },
        { .compatible = "analogix,anx7812", .data = PTR_TO_781X_ADDRS },
        { .compatible = "analogix,anx7814", .data = PTR_TO_781X_ADDRS },
        { .compatible = "analogix,anx7818", .data = PTR_TO_781X_ADDRS },
        { /* sentinel */ },
};

Brian
Andrzej Hajda Sept. 16, 2019, 12:25 p.m. UTC | #8
On 16.09.2019 14:02, Brian Masney wrote:
> On Mon, Sep 16, 2019 at 01:32:58PM +0200, Enric Balletbo i Serra wrote:
>> Hi,
>>
>> On 16/9/19 12:49, Laurent Pinchart wrote:
>>> Hi Brian,
>>>
>>> On Mon, Sep 16, 2019 at 06:36:14AM -0400, Brian Masney wrote:
>>>> On Mon, Sep 16, 2019 at 12:02:09PM +0200, Andrzej Hajda wrote:
>>>>> On 15.08.2019 02:48, Brian Masney wrote:
>>>>>> When attempting to configure this driver on a Nexus 5 phone (msm8974),
>>>>>> setting up the dummy i2c bus for TX_P0 would fail due to an -EBUSY
>>>>>> error. The downstream MSM kernel sources [1] shows that the proper value
>>>>>> for TX_P0 is 0x78, not 0x70, so correct the value to allow device
>>>>>> probing to succeed.
>>>>>>
>>>>>> [1] https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/video/slimport/slimport_tx_reg.h
>>>>>>
>>>>>> Signed-off-by: Brian Masney <masneyb@onstation.org>
>>>>>> ---
>>>>>>  drivers/gpu/drm/bridge/analogix-anx78xx.h | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h b/drivers/gpu/drm/bridge/analogix-anx78xx.h
>>>>>> index 25e063bcecbc..bc511fc605c9 100644
>>>>>> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.h
>>>>>> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.h
>>>>>> @@ -6,7 +6,7 @@
>>>>>>  #ifndef __ANX78xx_H
>>>>>>  #define __ANX78xx_H
>>>>>>  
>>>>>> -#define TX_P0				0x70
>>>>>> +#define TX_P0				0x78
>>>>>
>>>>> This bothers me little. There are no upstream users, grepping android
>>>>> sources suggests that both values can be used [1][2]  (grep for "#define
>>>>> TX_P0"), moreover there is code suggesting both values can be valid [3].
>>>>>
>>>>> Could you verify datasheet which i2c slave addresses are valid for this
>>>>> chip, if both I guess this patch should be reworked.
>>>>>
>>>>>
>>>>> [1]:
>>>>> https://android.googlesource.com/kernel/msm/+/android-msm-flo-3.4-jb-mr2/drivers/misc/slimport_anx7808/slimport_tx_reg.h
>>>>>
>>>>> [2]:
>>>>> https://github.com/AndroidGX/SimpleGX-MM-6.0_H815_20d/blob/master/drivers/video/slimport/anx7812/slimport7812_tx_reg.h
>>>>>
>>>>> [3]:
>>>>> https://github.com/commaai/android_kernel_leeco_msm8996/blob/master/drivers/video/msm/mdss/dp/slimport_custom_declare.h#L73
>>>> This address is 0x78 on my Nexus 5. Given [3] above it looks like we
>>>> need to support both addresses. What do you think about moving these
>>>> addresses into device tree?
>>> Assuming that the device supports different addresses (I can't validate
>>> that as I don't have access to the datasheet), and different addresses
>>> need to be used on different systems, then the address to be used needs
>>> to be provided by the firmware (DT in this case). Two options are
>>> possible, either specifying the address explicitly in the device's DT
>>> node, or specifying free addresses (in the form of a white list or black
>>> list) and allocating an address from that pool. The latter has been
>>> discussed in a BoF at the Linux Plumbers Conference last week,
>>> https://linuxplumbersconf.org/event/4/contributions/542/.
>>>
>>>> The downstream and upstream kernel sources divide these addresses by two
>>>> to get the i2c address. Here's the code in upstream:
>>>>
>>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L1353
>>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L41
>>>>
>>>> I'm not sure why the actual i2c address isn't used in this code.
>> The ANX7802/12/14/16 has a slave I2C bus that provides the interface to access
>> or control the chip from the AP. The I2C slave addresses used to control the
>> ANX7802/12/14/16 are 70h, 72h, 7Ah, 7Eh and 80h. Every address allows you to
>> access to different registers of the chip and AFAICS is not configurable.
>>
>> I don't think these addresses should be configured via DT but for the driver itself.
>>
>> My wild guess is that the ANX7808 has different addresses, but I don't have the
>> datasheet of this version.
> I'm able to communicate with the 7808 on my Nexus 5 using the 0x78
> address. Given that the addresses appear to be fixed per model, maybe it
> makes sense to drop the address #defines and add the addresses to the
> data pointer in the driver's of_match_table like so:
>
> static const struct of_device_id anx78xx_match_table[] = {
>         { .compatible = "analogix,anx7808", .data = PTR_TO_7808_ADDRS },
>         { .compatible = "analogix,anx7812", .data = PTR_TO_781X_ADDRS },
>         { .compatible = "analogix,anx7814", .data = PTR_TO_781X_ADDRS },
>         { .compatible = "analogix,anx7818", .data = PTR_TO_781X_ADDRS },
>         { /* sentinel */ },
> };
>
> Brian
>
>

I have spotted following comment on chromium's ML[1]:

> The locations are hard coded in the register spec.  Furthermore, each
> one can be changed independently--for example the Android driver puts
> 0x38 at 0x3c but leaves the rest alone.

It is not entirely clear, but IMO it suggests these addresses are
hardware configurable.


[1]:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/44601/2/drivers/auxdisplay/slimport.c#331


Regards

Andrzej
Andrzej Hajda Sept. 17, 2019, 7:28 a.m. UTC | #9
On 16.09.2019 14:02, Brian Masney wrote:
> On Mon, Sep 16, 2019 at 01:32:58PM +0200, Enric Balletbo i Serra wrote:
>> Hi,
>>
>> On 16/9/19 12:49, Laurent Pinchart wrote:
>>> Hi Brian,
>>>
>>> On Mon, Sep 16, 2019 at 06:36:14AM -0400, Brian Masney wrote:
>>>> On Mon, Sep 16, 2019 at 12:02:09PM +0200, Andrzej Hajda wrote:
>>>>> On 15.08.2019 02:48, Brian Masney wrote:
>>>>>> When attempting to configure this driver on a Nexus 5 phone (msm8974),
>>>>>> setting up the dummy i2c bus for TX_P0 would fail due to an -EBUSY
>>>>>> error. The downstream MSM kernel sources [1] shows that the proper value
>>>>>> for TX_P0 is 0x78, not 0x70, so correct the value to allow device
>>>>>> probing to succeed.
>>>>>>
>>>>>> [1] https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/video/slimport/slimport_tx_reg.h
>>>>>>
>>>>>> Signed-off-by: Brian Masney <masneyb@onstation.org>
>>>>>> ---
>>>>>>  drivers/gpu/drm/bridge/analogix-anx78xx.h | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h b/drivers/gpu/drm/bridge/analogix-anx78xx.h
>>>>>> index 25e063bcecbc..bc511fc605c9 100644
>>>>>> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.h
>>>>>> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.h
>>>>>> @@ -6,7 +6,7 @@
>>>>>>  #ifndef __ANX78xx_H
>>>>>>  #define __ANX78xx_H
>>>>>>  
>>>>>> -#define TX_P0				0x70
>>>>>> +#define TX_P0				0x78
>>>>>
>>>>> This bothers me little. There are no upstream users, grepping android
>>>>> sources suggests that both values can be used [1][2]  (grep for "#define
>>>>> TX_P0"), moreover there is code suggesting both values can be valid [3].
>>>>>
>>>>> Could you verify datasheet which i2c slave addresses are valid for this
>>>>> chip, if both I guess this patch should be reworked.
>>>>>
>>>>>
>>>>> [1]:
>>>>> https://android.googlesource.com/kernel/msm/+/android-msm-flo-3.4-jb-mr2/drivers/misc/slimport_anx7808/slimport_tx_reg.h
>>>>>
>>>>> [2]:
>>>>> https://github.com/AndroidGX/SimpleGX-MM-6.0_H815_20d/blob/master/drivers/video/slimport/anx7812/slimport7812_tx_reg.h
>>>>>
>>>>> [3]:
>>>>> https://github.com/commaai/android_kernel_leeco_msm8996/blob/master/drivers/video/msm/mdss/dp/slimport_custom_declare.h#L73
>>>> This address is 0x78 on my Nexus 5. Given [3] above it looks like we
>>>> need to support both addresses. What do you think about moving these
>>>> addresses into device tree?
>>> Assuming that the device supports different addresses (I can't validate
>>> that as I don't have access to the datasheet), and different addresses
>>> need to be used on different systems, then the address to be used needs
>>> to be provided by the firmware (DT in this case). Two options are
>>> possible, either specifying the address explicitly in the device's DT
>>> node, or specifying free addresses (in the form of a white list or black
>>> list) and allocating an address from that pool. The latter has been
>>> discussed in a BoF at the Linux Plumbers Conference last week,
>>> https://linuxplumbersconf.org/event/4/contributions/542/.
>>>
>>>> The downstream and upstream kernel sources divide these addresses by two
>>>> to get the i2c address. Here's the code in upstream:
>>>>
>>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L1353
>>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L41
>>>>
>>>> I'm not sure why the actual i2c address isn't used in this code.
>> The ANX7802/12/14/16 has a slave I2C bus that provides the interface to access
>> or control the chip from the AP. The I2C slave addresses used to control the
>> ANX7802/12/14/16 are 70h, 72h, 7Ah, 7Eh and 80h. Every address allows you to
>> access to different registers of the chip and AFAICS is not configurable.
>>
>> I don't think these addresses should be configured via DT but for the driver itself.
>>
>> My wild guess is that the ANX7808 has different addresses, but I don't have the
>> datasheet of this version.
> I'm able to communicate with the 7808 on my Nexus 5 using the 0x78
> address. Given that the addresses appear to be fixed per model, maybe it
> makes sense to drop the address #defines and add the addresses to the
> data pointer in the driver's of_match_table like so:
>
> static const struct of_device_id anx78xx_match_table[] = {
>         { .compatible = "analogix,anx7808", .data = PTR_TO_7808_ADDRS },
>         { .compatible = "analogix,anx7812", .data = PTR_TO_781X_ADDRS },
>         { .compatible = "analogix,anx7814", .data = PTR_TO_781X_ADDRS },
>         { .compatible = "analogix,anx7818", .data = PTR_TO_781X_ADDRS },
>         { /* sentinel */ },
> };


With given feedback from other users and lack of datasheets for chips
(except anx7814) we can try this approach.


Regards

Andrzej


>
> Brian
>
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h b/drivers/gpu/drm/bridge/analogix-anx78xx.h
index 25e063bcecbc..bc511fc605c9 100644
--- a/drivers/gpu/drm/bridge/analogix-anx78xx.h
+++ b/drivers/gpu/drm/bridge/analogix-anx78xx.h
@@ -6,7 +6,7 @@ 
 #ifndef __ANX78xx_H
 #define __ANX78xx_H
 
-#define TX_P0				0x70
+#define TX_P0				0x78
 #define TX_P1				0x7a
 #define TX_P2				0x72