Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[Krzysztof Kozlowski <krzk@kernel.org>]

On 07/08/2024 10:23, 郑豪威 wrote:
> 
> 在 2024/8/4 16:43, Krzysztof Kozlowski 写道:
>> On 04/08/2024 08:38,zhenghaowei@loongson.cn wrote:
>>
>> Due to lack of changelog, I assume you send the same patch, so:
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>> </form letter>
>>
>> Also:
>>
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  fractional-division:
>> Where are this and following defined? In which schema?
>>
> These and the ones below are new definitions, can I use them like this?
> 
> +  fractional-division:
> +    description: Enables fractional-N division. Currently,
> +      only LS2K1500 and LS2K2000 support this feature.
> +    type: boolean
> 

Missing vendor prefix, but what's more important, why would this be
property of DT? Just enable it always...

>>> +    description: Enables fractional-N division. Currently,
>>> +      only LS2K1500 and LS2K2000 support this feature.
>>> +
>>> +  rts-invert:
>>> +    description: Inverts the RTS value in the MCR register.
>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>> +      series CPUs, and Loongson LS7A bridge chips.
>>> +
>>> +  dtr-invert:
>>> +    description: Inverts the DTR value in the MCR register.
>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>> +      series CPUs, and Loongson LS7A bridge chips.
>>> +
>>> +  cts-invert:
>>> +    description: Inverts the CTS value in the MSR register.
>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>> +      and Loongson LS7A bridge chips.
>>> +
>>> +  dsr-invert:
>>> +    description: Inverts the DSR value in the MSR register.
>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>> +      and Loongson LS7A bridge chips.

Same questions for all these. Why choosing invert is a board level
decision? If it "should be used" then why it is not used always?

>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - clocks
>>> +
>>> +allOf:
>>> +  - $ref: serial.yaml
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>> +    #include <dt-bindings/clock/loongson,ls2k-clk.h>
>>> +
>>> +    serial@1fe001e0 {
>>> +        compatible = "loongson,ls7a-uart";
>>> +        reg = <0x0 0x1fe001e0 0x0 0x10>;
>>> +        clock-frequency = <100000000>;
>>> +        interrupt-parent = <&liointc>;
>>> +        interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
>>> +        fractional-division;
>>> +        rts-invert;
>>> +        dtr-invert;
>>> +    };
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 8766f3e5e87e..a6306327dba5 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -13189,6 +13189,13 @@ S:	Maintained
>>>   F:	Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
>>>   F:	drivers/i2c/busses/i2c-ls2x.c
>>>   
>>> +LOONGSON UART DRIVER
>>> +M:	Haowei Zheng<zhenghaowei@loongson.cn>
>>> +L:	linux-serial@vger.kernel.org
>>> +S:	Maintained
>>> +F:	Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml
>>> +F:	drivers/tty/serial/8250/8250_loongson.c
>> There is no such file.
>>
>> Best regards,
>> Krzysztof
> 
> The file "drivers/tty/serial/8250/8250_loongson.c" will be created in 
> the patch
> 
> "tty: serial: 8250: Add loongson uart driver support". Is it 
> inappropriate to reference it here?

Apply this patch and run get_maintainers self tests. What do you see?

Of course it is not appropriate here. The file does not exist.

Best regards,
Krzysztof