rockchip: correctly set vop0 or vop1

rockchip: correctly set vop0 or vop1

[Patrick Wildt]

The EDP_LCDC_SEL bit has to be set correctly to select vop0 or
vop1, but so far we have set it in both conditions, which is not
correct.

Can someone verify this is the correct way round?  vop1 -> set,
vop0 -> clear?

Signed-off-by: Patrick Wildt <patrick@blueri.se>

diff --git a/drivers/video/rockchip/rk_edp.c b/drivers/video/rockchip/rk_edp.c
index 92188be9275..000bd481408 100644
--- a/drivers/video/rockchip/rk_edp.c
+++ b/drivers/video/rockchip/rk_edp.c
@@ -1062,7 +1062,8 @@ static int rk_edp_probe(struct udevice *dev)
 	rk_setreg(&priv->grf->soc_con12, 1 << 4);
 
 	/* select epd signal from vop0 or vop1 */
-	rk_setreg(&priv->grf->soc_con6, (vop_id == 1) ? (1 << 5) : (1 << 5));
+	rk_clrsetreg(&priv->grf->soc_con6, (1 << 5),
+	    (vop_id == 1) ? (1 << 5) : (0 << 5));
 
 	rockchip_edp_wait_hpd(priv);
 

rockchip: correctly set vop0 or vop1

[Arnaud Patard]

Patrick Wildt <patrick@blueri.se> writes:

Hi,

> The EDP_LCDC_SEL bit has to be set correctly to select vop0 or
> vop1, but so far we have set it in both conditions, which is not
> correct.
>
> Can someone verify this is the correct way round?  vop1 -> set,
> vop0 -> clear?
>
> Signed-off-by: Patrick Wildt <patrick@blueri.se>
>
> diff --git a/drivers/video/rockchip/rk_edp.c b/drivers/video/rockchip/rk_edp.c
> index 92188be9275..000bd481408 100644
> --- a/drivers/video/rockchip/rk_edp.c
> +++ b/drivers/video/rockchip/rk_edp.c
> @@ -1062,7 +1062,8 @@ static int rk_edp_probe(struct udevice *dev)
>  	rk_setreg(&priv->grf->soc_con12, 1 << 4);
>  
>  	/* select epd signal from vop0 or vop1 */
> -	rk_setreg(&priv->grf->soc_con6, (vop_id == 1) ? (1 << 5) : (1 << 5));
> +	rk_clrsetreg(&priv->grf->soc_con6, (1 << 5),
> +	    (vop_id == 1) ? (1 << 5) : (0 << 5));

While working on PBP EDP support, found this too but I'm not sure it's
fine or not. For rk3399, my (not yet published) patch is doing:

+       if (vop_id == 0)
+               rk_clrreg(&priv->grf->soc_con20, (1 << 5));
+       else
+               rk_setreg(&priv->grf->soc_con20, (1 << 5));

I believe that the rk3288 may need similar treatment but I've yet to
look@the rk3288 manual.

Arnaud

rockchip: correctly set vop0 or vop1

[Patrick Wildt]

On Mon, Jun 08, 2020 at 10:18:19AM +0200, Arnaud Patard wrote:
> Patrick Wildt <patrick@blueri.se> writes:
> 
> Hi,
> 
> > The EDP_LCDC_SEL bit has to be set correctly to select vop0 or
> > vop1, but so far we have set it in both conditions, which is not
> > correct.
> >
> > Can someone verify this is the correct way round?  vop1 -> set,
> > vop0 -> clear?
> >
> > Signed-off-by: Patrick Wildt <patrick@blueri.se>
> >
> > diff --git a/drivers/video/rockchip/rk_edp.c b/drivers/video/rockchip/rk_edp.c
> > index 92188be9275..000bd481408 100644
> > --- a/drivers/video/rockchip/rk_edp.c
> > +++ b/drivers/video/rockchip/rk_edp.c
> > @@ -1062,7 +1062,8 @@ static int rk_edp_probe(struct udevice *dev)
> >  	rk_setreg(&priv->grf->soc_con12, 1 << 4);
> >  
> >  	/* select epd signal from vop0 or vop1 */
> > -	rk_setreg(&priv->grf->soc_con6, (vop_id == 1) ? (1 << 5) : (1 << 5));
> > +	rk_clrsetreg(&priv->grf->soc_con6, (1 << 5),
> > +	    (vop_id == 1) ? (1 << 5) : (0 << 5));
> 
> While working on PBP EDP support, found this too but I'm not sure it's
> fine or not. For rk3399, my (not yet published) patch is doing:
> 
> +       if (vop_id == 0)
> +               rk_clrreg(&priv->grf->soc_con20, (1 << 5));
> +       else
> +               rk_setreg(&priv->grf->soc_con20, (1 << 5));
> 
> I believe that the rk3288 may need similar treatment but I've yet to
> look at the rk3288 manual.
> 
> Arnaud

Yes, it does.  If you look at the linux code, they have:

static const struct rockchip_dp_chip_data rk3399_edp = {
        .lcdsel_grf_reg = RK3399_GRF_SOC_CON20,
        .lcdsel_big = HIWORD_UPDATE(0, RK3399_EDP_LCDC_SEL),
        .lcdsel_lit = HIWORD_UPDATE(RK3399_EDP_LCDC_SEL, RK3399_EDP_LCDC_SEL),
        .chip_type = RK3399_EDP,
};

static const struct rockchip_dp_chip_data rk3288_dp = {
        .lcdsel_grf_reg = RK3288_GRF_SOC_CON6,
        .lcdsel_big = HIWORD_UPDATE(0, RK3288_EDP_LCDC_SEL),
        .lcdsel_lit = HIWORD_UPDATE(RK3288_EDP_LCDC_SEL, RK3288_EDP_LCDC_SEL),
        .chip_type = RK3288_DP,
};

which indicates that for rk3399 *and* rk3288 the bit has to be set to
select "lit".  Now your diff looks equivalent to mine, apart from using
a different operation to achieve the same goal.

The linux code does

        ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
        if (ret < 0)
                return;

        if (ret)
                val = dp->data->lcdsel_lit;
        else
                val = dp->data->lcdsel_big;

Assuming that endpoint_id essentiall returns vop id 0 or vop id 1, this
would mean that vop1 -> lit -> set bit and vop0 -> big -> clr bit.

That said, my diff seems to be fine, and your RK3399 code as well.  Do
you agree?

Patrick

rockchip: correctly set vop0 or vop1

[Arnaud Patard]

Patrick Wildt <patrick@blueri.se> writes:

> On Mon, Jun 08, 2020 at 10:18:19AM +0200, Arnaud Patard wrote:
>> Patrick Wildt <patrick@blueri.se> writes:
>> 
>> Hi,
>> 
>> > The EDP_LCDC_SEL bit has to be set correctly to select vop0 or
>> > vop1, but so far we have set it in both conditions, which is not
>> > correct.
>> >
>> > Can someone verify this is the correct way round?  vop1 -> set,
>> > vop0 -> clear?
>> >
>> > Signed-off-by: Patrick Wildt <patrick@blueri.se>
>> >
>> > diff --git a/drivers/video/rockchip/rk_edp.c b/drivers/video/rockchip/rk_edp.c
>> > index 92188be9275..000bd481408 100644
>> > --- a/drivers/video/rockchip/rk_edp.c
>> > +++ b/drivers/video/rockchip/rk_edp.c
>> > @@ -1062,7 +1062,8 @@ static int rk_edp_probe(struct udevice *dev)
>> >  	rk_setreg(&priv->grf->soc_con12, 1 << 4);
>> >  
>> >  	/* select epd signal from vop0 or vop1 */
>> > -	rk_setreg(&priv->grf->soc_con6, (vop_id == 1) ? (1 << 5) : (1 << 5));
>> > +	rk_clrsetreg(&priv->grf->soc_con6, (1 << 5),
>> > +	    (vop_id == 1) ? (1 << 5) : (0 << 5));
>> 
>> While working on PBP EDP support, found this too but I'm not sure it's
>> fine or not. For rk3399, my (not yet published) patch is doing:
>> 
>> +       if (vop_id == 0)
>> +               rk_clrreg(&priv->grf->soc_con20, (1 << 5));
>> +       else
>> +               rk_setreg(&priv->grf->soc_con20, (1 << 5));
>> 
>> I believe that the rk3288 may need similar treatment but I've yet to
>> look at the rk3288 manual.
>> 
>> Arnaud
>
> Yes, it does.  If you look at the linux code, they have:
>
> static const struct rockchip_dp_chip_data rk3399_edp = {
>         .lcdsel_grf_reg = RK3399_GRF_SOC_CON20,
>         .lcdsel_big = HIWORD_UPDATE(0, RK3399_EDP_LCDC_SEL),
>         .lcdsel_lit = HIWORD_UPDATE(RK3399_EDP_LCDC_SEL, RK3399_EDP_LCDC_SEL),
>         .chip_type = RK3399_EDP,
> };
>
> static const struct rockchip_dp_chip_data rk3288_dp = {
>         .lcdsel_grf_reg = RK3288_GRF_SOC_CON6,
>         .lcdsel_big = HIWORD_UPDATE(0, RK3288_EDP_LCDC_SEL),
>         .lcdsel_lit = HIWORD_UPDATE(RK3288_EDP_LCDC_SEL, RK3288_EDP_LCDC_SEL),
>         .chip_type = RK3288_DP,
> };
>
> which indicates that for rk3399 *and* rk3288 the bit has to be set to
> select "lit".  Now your diff looks equivalent to mine, apart from using
> a different operation to achieve the same goal.
>
> The linux code does
>
>         ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
>         if (ret < 0)
>                 return;
>
>         if (ret)
>                 val = dp->data->lcdsel_lit;
>         else
>                 val = dp->data->lcdsel_big;
>
> Assuming that endpoint_id essentiall returns vop id 0 or vop id 1, this
> would mean that vop1 -> lit -> set bit and vop0 -> big -> clr bit.
>
> That said, my diff seems to be fine, and your RK3399 code as well.  Do
> you agree?

According to the code you've shown, it should be fine for rk3288 I guess
but not for rk3399. Please note that it's grf soc_con6 register for rk3288
but grf soc_con20 for rk3399.

Arnaud

rockchip: correctly set vop0 or vop1

[Patrick Wildt]

On Mon, Jun 08, 2020 at 02:24:32PM +0200, Arnaud Patard wrote:
> Patrick Wildt <patrick@blueri.se> writes:
> 
> > On Mon, Jun 08, 2020 at 10:18:19AM +0200, Arnaud Patard wrote:
> >> Patrick Wildt <patrick@blueri.se> writes:
> >> 
> >> Hi,
> >> 
> >> > The EDP_LCDC_SEL bit has to be set correctly to select vop0 or
> >> > vop1, but so far we have set it in both conditions, which is not
> >> > correct.
> >> >
> >> > Can someone verify this is the correct way round?  vop1 -> set,
> >> > vop0 -> clear?
> >> >
> >> > Signed-off-by: Patrick Wildt <patrick@blueri.se>
> >> >
> >> > diff --git a/drivers/video/rockchip/rk_edp.c b/drivers/video/rockchip/rk_edp.c
> >> > index 92188be9275..000bd481408 100644
> >> > --- a/drivers/video/rockchip/rk_edp.c
> >> > +++ b/drivers/video/rockchip/rk_edp.c
> >> > @@ -1062,7 +1062,8 @@ static int rk_edp_probe(struct udevice *dev)
> >> >  	rk_setreg(&priv->grf->soc_con12, 1 << 4);
> >> >  
> >> >  	/* select epd signal from vop0 or vop1 */
> >> > -	rk_setreg(&priv->grf->soc_con6, (vop_id == 1) ? (1 << 5) : (1 << 5));
> >> > +	rk_clrsetreg(&priv->grf->soc_con6, (1 << 5),
> >> > +	    (vop_id == 1) ? (1 << 5) : (0 << 5));
> >> 
> >> While working on PBP EDP support, found this too but I'm not sure it's
> >> fine or not. For rk3399, my (not yet published) patch is doing:
> >> 
> >> +       if (vop_id == 0)
> >> +               rk_clrreg(&priv->grf->soc_con20, (1 << 5));
> >> +       else
> >> +               rk_setreg(&priv->grf->soc_con20, (1 << 5));
> >> 
> >> I believe that the rk3288 may need similar treatment but I've yet to
> >> look at the rk3288 manual.
> >> 
> >> Arnaud
> >
> > Yes, it does.  If you look at the linux code, they have:
> >
> > static const struct rockchip_dp_chip_data rk3399_edp = {
> >         .lcdsel_grf_reg = RK3399_GRF_SOC_CON20,
> >         .lcdsel_big = HIWORD_UPDATE(0, RK3399_EDP_LCDC_SEL),
> >         .lcdsel_lit = HIWORD_UPDATE(RK3399_EDP_LCDC_SEL, RK3399_EDP_LCDC_SEL),
> >         .chip_type = RK3399_EDP,
> > };
> >
> > static const struct rockchip_dp_chip_data rk3288_dp = {
> >         .lcdsel_grf_reg = RK3288_GRF_SOC_CON6,
> >         .lcdsel_big = HIWORD_UPDATE(0, RK3288_EDP_LCDC_SEL),
> >         .lcdsel_lit = HIWORD_UPDATE(RK3288_EDP_LCDC_SEL, RK3288_EDP_LCDC_SEL),
> >         .chip_type = RK3288_DP,
> > };
> >
> > which indicates that for rk3399 *and* rk3288 the bit has to be set to
> > select "lit".  Now your diff looks equivalent to mine, apart from using
> > a different operation to achieve the same goal.
> >
> > The linux code does
> >
> >         ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
> >         if (ret < 0)
> >                 return;
> >
> >         if (ret)
> >                 val = dp->data->lcdsel_lit;
> >         else
> >                 val = dp->data->lcdsel_big;
> >
> > Assuming that endpoint_id essentiall returns vop id 0 or vop id 1, this
> > would mean that vop1 -> lit -> set bit and vop0 -> big -> clr bit.
> >
> > That said, my diff seems to be fine, and your RK3399 code as well.  Do
> > you agree?
> 
> According to the code you've shown, it should be fine for rk3288 I guess
> but not for rk3399. Please note that it's grf soc_con6 register for rk3288
> but grf soc_con20 for rk3399.
> 
> Arnaud

Exactly, which is why you had that if defined() in your diff, to compile
one part of the code for RK3288, and the other for RK3399. :)  The bit
though happens to be the same.

rockchip: correctly set vop0 or vop1

[Kever Yang]

+Andy Yan for this topic,

Hi Patrick and Arnaud,

 ??? I would like to leave this patch until the code fits for all the socs,

Thanks,

- Kever

On 2020/6/8 ??8:39, Patrick Wildt wrote:
> On Mon, Jun 08, 2020 at 02:24:32PM +0200, Arnaud Patard wrote:
>> Patrick Wildt <patrick@blueri.se> writes:
>>
>>> On Mon, Jun 08, 2020 at 10:18:19AM +0200, Arnaud Patard wrote:
>>>> Patrick Wildt <patrick@blueri.se> writes:
>>>>
>>>> Hi,
>>>>
>>>>> The EDP_LCDC_SEL bit has to be set correctly to select vop0 or
>>>>> vop1, but so far we have set it in both conditions, which is not
>>>>> correct.
>>>>>
>>>>> Can someone verify this is the correct way round?  vop1 -> set,
>>>>> vop0 -> clear?
>>>>>
>>>>> Signed-off-by: Patrick Wildt <patrick@blueri.se>
>>>>>
>>>>> diff --git a/drivers/video/rockchip/rk_edp.c b/drivers/video/rockchip/rk_edp.c
>>>>> index 92188be9275..000bd481408 100644
>>>>> --- a/drivers/video/rockchip/rk_edp.c
>>>>> +++ b/drivers/video/rockchip/rk_edp.c
>>>>> @@ -1062,7 +1062,8 @@ static int rk_edp_probe(struct udevice *dev)
>>>>>   	rk_setreg(&priv->grf->soc_con12, 1 << 4);
>>>>>   
>>>>>   	/* select epd signal from vop0 or vop1 */
>>>>> -	rk_setreg(&priv->grf->soc_con6, (vop_id == 1) ? (1 << 5) : (1 << 5));
>>>>> +	rk_clrsetreg(&priv->grf->soc_con6, (1 << 5),
>>>>> +	    (vop_id == 1) ? (1 << 5) : (0 << 5));
>>>> While working on PBP EDP support, found this too but I'm not sure it's
>>>> fine or not. For rk3399, my (not yet published) patch is doing:
>>>>
>>>> +       if (vop_id == 0)
>>>> +               rk_clrreg(&priv->grf->soc_con20, (1 << 5));
>>>> +       else
>>>> +               rk_setreg(&priv->grf->soc_con20, (1 << 5));
>>>>
>>>> I believe that the rk3288 may need similar treatment but I've yet to
>>>> look at the rk3288 manual.
>>>>
>>>> Arnaud
>>> Yes, it does.  If you look at the linux code, they have:
>>>
>>> static const struct rockchip_dp_chip_data rk3399_edp = {
>>>          .lcdsel_grf_reg = RK3399_GRF_SOC_CON20,
>>>          .lcdsel_big = HIWORD_UPDATE(0, RK3399_EDP_LCDC_SEL),
>>>          .lcdsel_lit = HIWORD_UPDATE(RK3399_EDP_LCDC_SEL, RK3399_EDP_LCDC_SEL),
>>>          .chip_type = RK3399_EDP,
>>> };
>>>
>>> static const struct rockchip_dp_chip_data rk3288_dp = {
>>>          .lcdsel_grf_reg = RK3288_GRF_SOC_CON6,
>>>          .lcdsel_big = HIWORD_UPDATE(0, RK3288_EDP_LCDC_SEL),
>>>          .lcdsel_lit = HIWORD_UPDATE(RK3288_EDP_LCDC_SEL, RK3288_EDP_LCDC_SEL),
>>>          .chip_type = RK3288_DP,
>>> };
>>>
>>> which indicates that for rk3399 *and* rk3288 the bit has to be set to
>>> select "lit".  Now your diff looks equivalent to mine, apart from using
>>> a different operation to achieve the same goal.
>>>
>>> The linux code does
>>>
>>>          ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
>>>          if (ret < 0)
>>>                  return;
>>>
>>>          if (ret)
>>>                  val = dp->data->lcdsel_lit;
>>>          else
>>>                  val = dp->data->lcdsel_big;
>>>
>>> Assuming that endpoint_id essentiall returns vop id 0 or vop id 1, this
>>> would mean that vop1 -> lit -> set bit and vop0 -> big -> clr bit.
>>>
>>> That said, my diff seems to be fine, and your RK3399 code as well.  Do
>>> you agree?
>> According to the code you've shown, it should be fine for rk3288 I guess
>> but not for rk3399. Please note that it's grf soc_con6 register for rk3288
>> but grf soc_con20 for rk3399.
>>
>> Arnaud
> Exactly, which is why you had that if defined() in your diff, to compile
> one part of the code for RK3288, and the other for RK3399. :)  The bit
> though happens to be the same.
>
>

rockchip: correctly set vop0 or vop1

[Kever Yang]

On 2020/6/8 ??2:36, Patrick Wildt wrote:
> The EDP_LCDC_SEL bit has to be set correctly to select vop0 or
> vop1, but so far we have set it in both conditions, which is not
> correct.
>
> Can someone verify this is the correct way round?  vop1 -> set,
> vop0 -> clear?
>
> Signed-off-by: Patrick Wildt <patrick@blueri.se>

I will take this fix for rk3288 and later you can send support for rk3399.


Reviewed-by: Kever Yang <kever.yang@rock-chips.com>


Thanks,
- Kever
>
> diff --git a/drivers/video/rockchip/rk_edp.c b/drivers/video/rockchip/rk_edp.c
> index 92188be9275..000bd481408 100644
> --- a/drivers/video/rockchip/rk_edp.c
> +++ b/drivers/video/rockchip/rk_edp.c
> @@ -1062,7 +1062,8 @@ static int rk_edp_probe(struct udevice *dev)
>   	rk_setreg(&priv->grf->soc_con12, 1 << 4);
>   
>   	/* select epd signal from vop0 or vop1 */
> -	rk_setreg(&priv->grf->soc_con6, (vop_id == 1) ? (1 << 5) : (1 << 5));
> +	rk_clrsetreg(&priv->grf->soc_con6, (1 << 5),
> +	    (vop_id == 1) ? (1 << 5) : (0 << 5));
>   
>   	rockchip_edp_wait_hpd(priv);
>   
>
>

rockchip: correctly set vop0 or vop1

[Andy Yan]

Hi :

On 6/27/20 8:56 PM, Kever Yang wrote:
> +Andy Yan for this topic,
>
> Hi Patrick and Arnaud,
>
> ??? I would like to leave this patch until the code fits for all the 
> socs,
>
> Thanks,
>
> - Kever
>
> On 2020/6/8 ??8:39, Patrick Wildt wrote:
>> On Mon, Jun 08, 2020 at 02:24:32PM +0200, Arnaud Patard wrote:
>>> Patrick Wildt <patrick@blueri.se> writes:
>>>
>>>> On Mon, Jun 08, 2020 at 10:18:19AM +0200, Arnaud Patard wrote:
>>>>> Patrick Wildt <patrick@blueri.se> writes:
>>>>>
>>>>> Hi,
>>>>>
>>>>>> The EDP_LCDC_SEL bit has to be set correctly to select vop0 or
>>>>>> vop1, but so far we have set it in both conditions, which is not
>>>>>> correct.
>>>>>>
>>>>>> Can someone verify this is the correct way round?? vop1 -> set,
>>>>>> vop0 -> clear?
>>>>>>
>>>>>> Signed-off-by: Patrick Wildt <patrick@blueri.se>
>>>>>>
>>>>>> diff --git a/drivers/video/rockchip/rk_edp.c 
>>>>>> b/drivers/video/rockchip/rk_edp.c
>>>>>> index 92188be9275..000bd481408 100644
>>>>>> --- a/drivers/video/rockchip/rk_edp.c
>>>>>> +++ b/drivers/video/rockchip/rk_edp.c
>>>>>> @@ -1062,7 +1062,8 @@ static int rk_edp_probe(struct udevice *dev)
>>>>>> ????? rk_setreg(&priv->grf->soc_con12, 1 << 4);
>>>>>> ? ????? /* select epd signal from vop0 or vop1 */
>>>>>> -??? rk_setreg(&priv->grf->soc_con6, (vop_id == 1) ? (1 << 5) : 
>>>>>> (1 << 5));
>>>>>> +??? rk_clrsetreg(&priv->grf->soc_con6, (1 << 5),
>>>>>> +??????? (vop_id == 1) ? (1 << 5) : (0 << 5));
>>>>> While working on PBP EDP support, found this too but I'm not sure 
>>>>> it's
>>>>> fine or not. For rk3399, my (not yet published) patch is doing:
>>>>>
>>>>> +?????? if (vop_id == 0)
>>>>> +?????????????? rk_clrreg(&priv->grf->soc_con20, (1 << 5));
>>>>> +?????? else
>>>>> +?????????????? rk_setreg(&priv->grf->soc_con20, (1 << 5));
>>>>>
>>>>> I believe that the rk3288 may need similar treatment but I've yet to
>>>>> look at the rk3288 manual.
>>>>>
>>>>> Arnaud
>>>> Yes, it does.? If you look at the linux code, they have:
>>>>
>>>> static const struct rockchip_dp_chip_data rk3399_edp = {
>>>> ???????? .lcdsel_grf_reg = RK3399_GRF_SOC_CON20,
>>>> ???????? .lcdsel_big = HIWORD_UPDATE(0, RK3399_EDP_LCDC_SEL),
>>>> ???????? .lcdsel_lit = HIWORD_UPDATE(RK3399_EDP_LCDC_SEL, 
>>>> RK3399_EDP_LCDC_SEL),
>>>> ???????? .chip_type = RK3399_EDP,
>>>> };
>>>>
>>>> static const struct rockchip_dp_chip_data rk3288_dp = {
>>>> ???????? .lcdsel_grf_reg = RK3288_GRF_SOC_CON6,
>>>> ???????? .lcdsel_big = HIWORD_UPDATE(0, RK3288_EDP_LCDC_SEL),
>>>> ???????? .lcdsel_lit = HIWORD_UPDATE(RK3288_EDP_LCDC_SEL, 
>>>> RK3288_EDP_LCDC_SEL),
>>>> ???????? .chip_type = RK3288_DP,
>>>> };
>>>>

It's true that different soc have different grf register for selecting 
lcdc/vop, and so it is for other modules such as rockchip_gmac/pinctrl. 
The above code in linux kernel is a example for how? we handle this case.


>>>> which indicates that for rk3399 *and* rk3288 the bit has to be set to
>>>> select "lit".? Now your diff looks equivalent to mine, apart from 
>>>> using
>>>> a different operation to achieve the same goal.
>>>>
>>>> The linux code does
>>>>
>>>> ???????? ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, 
>>>> encoder);
>>>> ???????? if (ret < 0)
>>>> ???????????????? return;
>>>>
>>>> ???????? if (ret)
>>>> ???????????????? val = dp->data->lcdsel_lit;
>>>> ???????? else
>>>> ???????????????? val = dp->data->lcdsel_big;
>>>>
>>>> Assuming that endpoint_id essentiall returns vop id 0 or vop id 1, 
>>>> this
>>>> would mean that vop1 -> lit -> set bit and vop0 -> big -> clr bit.
>>>>
>>>> That said, my diff seems to be fine, and your RK3399 code as well.? Do
>>>> you agree?
>>> According to the code you've shown, it should be fine for rk3288 I 
>>> guess
>>> but not for rk3399. Please note that it's grf soc_con6 register for 
>>> rk3288
>>> but grf soc_con20 for rk3399.
>>>
>>> Arnaud
>> Exactly, which is why you had that if defined() in your diff, to compile
>> one part of the code for RK3288, and the other for RK3399. :) The bit
>> though happens to be the same.
>>
>>
>
>
>