[PATCH v14 1/2] drm: add kms driver for loongson display controller

[WANG Xuerui]

On 2023/5/22 16:02, Sui Jingfeng wrote:
> Hi,
> 
> On 2023/5/21 20:21, WANG Xuerui wrote:
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/loongson/Kconfig
>>> @@ -0,0 +1,17 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +
>>> +config DRM_LOONGSON
>>> +    tristate "DRM support for Loongson Graphics"
>>> +    depends on DRM && PCI && MMU
>>> +    select DRM_KMS_HELPER
>>> +    select DRM_TTM
>>> +    select I2C
>>> +    select I2C_ALGOBIT
>>> +    help
>>> +      This is a DRM driver for Loongson Graphics, it may including
>>
>> Drop "it may"; "including" should be enough.
>>
> 'it may' is more *precise* here, because currently we don't ship with 
> the support for loongson 2K series SoC.
> 
> I'm try to be precise as far as I can, we avoid made this driver too 
> large by ignore loongson 2K series SoC temporary.

That's a good idea! For now the patch is so large that my review reply 
is said to be dropped by the lists. Focusing on one bunch of similar 
models first then adding support for the rest not-so-similar models is 
very friendly towards the reviewing process and will help code quality too.

> 
>>> +      LS7A2000, LS7A1000, LS2K2000 and LS2K1000 etc. Loongson LS7A
>>> +      series are bridge chipset, while Loongson LS2K series are SoC.
>>> +
>>> +      If "M" is selected, the module will be called loongson.
>>
>> Just "loongson"? 
> 
> Yes,  when compile this driver as module,  loongson.ko will be generated.
> 
>   drm radeon is also doing so, See drm/radeon/Kconfig.
> 
>> I know it's like this for ages (at least dating back to the MIPS days) 
>> but you really don't want to imply Loongson is mainly a GPU company. 
>> Something like "loongson_drm" or "lsdc" or "gsgpu" could be better. 
> 
> No, these name may have backward compatibility problems.
> 
> Downstream driver already taken those name.
> 
> userspace driver need to differentiate them who is who.

IMO this shouldn't be a problem. Let me try explaining this: currently, 
upstream / the "new world" doesn't have any support for this driver at 
all, so any name will work; just use whatever is appropriate from an 
upstream's perspective, then make the userspace bits recognize both 
variants, and you'll be fine. And the "existing" userspace drivers can 
also carry the change, it'll just be a branch never taken in that setup.

So, I'm still in favor of keeping the upstream "clean" without dubious 
names like this (bare "loongson"). What do you think about my suggestion 
above?

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/