| Message ID | 20220406094654.29722-2-rex-bc.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Correct disp_aal dt-binding | expand |
Il 06/04/22 11:46, Rex-BC Chen ha scritto: > The driver data of MT8183 and MT8173 are different. > The value of has_gamma for MT8173 is true while the value of MT8183 is > false. Therefore, the compatible of disp_aal for MT8183 is not suitable > for the compatible for MT8173. > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> Hello! Yes, I definitely agree with this change, it makes no sense to have both 8173 and 8183 compatibles for 8183 (and similar). Though, please, add a note in this commit message advertising that the mistake was only in the yaml binding, as the driver doesn't require any functional changes, so that things are clear for people with less expertise on MediaTek platforms. After that: Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On Wed, 2022-04-06 at 12:38 +0200, AngeloGioacchino Del Regno wrote: > Il 06/04/22 11:46, Rex-BC Chen ha scritto: > > The driver data of MT8183 and MT8173 are different. > > The value of has_gamma for MT8173 is true while the value of MT8183 > > is > > false. Therefore, the compatible of disp_aal for MT8183 is not > > suitable > > for the compatible for MT8173. > > > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > Hello! > Yes, I definitely agree with this change, it makes no sense to have > both > 8173 and 8183 compatibles for 8183 (and similar). > > Though, please, add a note in this commit message advertising that > the > mistake was only in the yaml binding, as the driver doesn't require > any > functional changes, so that things are clear for people with less > expertise > on MediaTek platforms. > > After that: > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> Hello Angelo, Thanks for your review. I will add them to the commit message in next version. BRs, Rex
On 06/04/2022 11:46, Rex-BC Chen wrote: > The driver data of MT8183 and MT8173 are different. > The value of has_gamma for MT8173 is true while the value of MT8183 is > false. Therefore, the compatible of disp_aal for MT8183 is not suitable > for the compatible for MT8173. Just because one feature is not supported, it does not mean they are incompatible, which you claim in the patch below. Are you sure they are really incompatible and MT8173 fallback cannot be used? Best regards, Krzysztof
On Wed, 2022-04-06 at 16:44 +0200, Krzysztof Kozlowski wrote: > On 06/04/2022 11:46, Rex-BC Chen wrote: > > The driver data of MT8183 and MT8173 are different. > > The value of has_gamma for MT8173 is true while the value of MT8183 > > is > > false. Therefore, the compatible of disp_aal for MT8183 is not > > suitable > > for the compatible for MT8173. > > Just because one feature is not supported, it does not mean they are > incompatible, which you claim in the patch below. Are you sure they > are > really incompatible and MT8173 fallback cannot be used? > > > Best regards, > Krzysztof Hello Krzysztof, Thanks for your review. The difference of disp_aal for each MediaTek SoCs is "has_gamma". And we only control this variable for different MediaTek SoCs. The value of has_gamma for MT8173 is true. The value of has_gamma for MT8183 is false. (Moreover, the driver data is null for MT8183) From this situation, I think it's not compatible between MT8173 and MT8183. reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/mediatek/mtk_disp_aal.c?h=v5.18-rc1#n153 BRs, Rex
On 07/04/2022 08:22, Rex-BC Chen wrote: > On Wed, 2022-04-06 at 16:44 +0200, Krzysztof Kozlowski wrote: >> On 06/04/2022 11:46, Rex-BC Chen wrote: >>> The driver data of MT8183 and MT8173 are different. >>> The value of has_gamma for MT8173 is true while the value of MT8183 >>> is >>> false. Therefore, the compatible of disp_aal for MT8183 is not >>> suitable >>> for the compatible for MT8173. >> >> Just because one feature is not supported, it does not mean they are >> incompatible, which you claim in the patch below. Are you sure they >> are >> really incompatible and MT8173 fallback cannot be used? >> >> >> Best regards, >> Krzysztof > > Hello Krzysztof, > > Thanks for your review. > > The difference of disp_aal for each MediaTek SoCs is "has_gamma". > And we only control this variable for different MediaTek SoCs. > > The value of has_gamma for MT8173 is true. > The value of has_gamma for MT8183 is false. (Moreover, the driver data > is null for MT8183) > > From this situation, I think it's not compatible between MT8173 and > MT8183. You repeated the commit msg without bringing any new information... but let it be, I assume setting gamma on MTT8183 is incorrect or produces wrong results. Best regards, Krzysztof
On Thu, 2022-04-07 at 08:49 +0200, Krzysztof Kozlowski wrote: > On 07/04/2022 08:22, Rex-BC Chen wrote: > > On Wed, 2022-04-06 at 16:44 +0200, Krzysztof Kozlowski wrote: > > > On 06/04/2022 11:46, Rex-BC Chen wrote: > > > > The driver data of MT8183 and MT8173 are different. > > > > The value of has_gamma for MT8173 is true while the value of > > > > MT8183 > > > > is > > > > false. Therefore, the compatible of disp_aal for MT8183 is not > > > > suitable > > > > for the compatible for MT8173. > > > > > > Just because one feature is not supported, it does not mean they > > > are > > > incompatible, which you claim in the patch below. Are you sure > > > they > > > are > > > really incompatible and MT8173 fallback cannot be used? > > > > > > > > > Best regards, > > > Krzysztof > > > > Hello Krzysztof, > > > > Thanks for your review. > > > > The difference of disp_aal for each MediaTek SoCs is "has_gamma". > > And we only control this variable for different MediaTek SoCs. > > > > The value of has_gamma for MT8173 is true. > > The value of has_gamma for MT8183 is false. (Moreover, the driver > > data > > is null for MT8183) > > > > From this situation, I think it's not compatible between MT8173 and > > MT8183. > > You repeated the commit msg without bringing any new information... > but > let it be, I assume setting gamma on MTT8183 is incorrect or produces > wrong results. > > > Best regards, > Krzysztof Hello Krzysztof, Sorry for this. I can explain more detailed. For MT8173, the gamma module is inside disp_aal. When we need to adjust gamma value, we need to use "has_gamma" to control gamma function inside disp_aal to adjust gamma value. Since MT8183 (MT8192/MT8195/MT8186), display gamma is seperated from disp_aal. We just need to control disp_gamma directly and we don't need to control gamma function inside disp_aal. I will add these comments to commit message in next version to explain it. BRs, Rex
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml index cb4a2f39ff88..f118075e4a89 100644 --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml @@ -21,12 +21,12 @@ description: | properties: compatible: oneOf: - - items: - - const: mediatek,mt8173-disp-aal + - enum: + - mediatek,mt8173-disp-aal + - mediatek,mt8183-disp-aal - items: - enum: - mediatek,mt2712-disp-aal - - mediatek,mt8183-disp-aal - mediatek,mt8192-disp-aal - mediatek,mt8195-disp-aal - const: mediatek,mt8173-disp-aal
The driver data of MT8183 and MT8173 are different. The value of has_gamma for MT8173 is true while the value of MT8183 is false. Therefore, the compatible of disp_aal for MT8183 is not suitable for the compatible for MT8173. Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> --- .../devicetree/bindings/display/mediatek/mediatek,aal.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)