Am 03.02.2016 um 00:49 schrieb Peter Hurley:
> On 01/29/2016 12:21 AM, Oleksij Rempel wrote:
>> Hi Peter,
>>
>> thank you for comments.
>>
>> Am 28.01.2016 um 17:40 schrieb Peter Hurley:
>>> Hi Oleksij,
>>>
>>> On 12/14/2015 11:24 AM, Oleksij Rempel wrote:
>>>> Alphascale ASM9260 uart IP has some common registers with
>>>> Freescale STMP37XX. This patch provide changes which
>>>> allow to reuse mxs-auart.c code for ASM9260.
>>>>
>>>> Signed-off-by: Oleksij Rempel <linux@xxxxxxxxxxxxxxxx>
>>>> ---
>>>> .../devicetree/bindings/serial/fsl-mxs-auart.txt | 10 +-
>>>> drivers/tty/serial/Kconfig | 5 +-
>>>> drivers/tty/serial/mxs-auart.c | 667 +++++++++++++++++----
>>>> 3 files changed, 577 insertions(+), 105 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>>> index 7c408c8..5bb2e32 100644
>>>> --- a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>>> +++ b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>>> @@ -1,8 +1,8 @@
>>>> * Freescale MXS Application UART (AUART)
>>>>
>>>> -Required properties:
>>>> +Required properties for all SoCs:
>>>> - compatible : Should be "fsl,<soc>-auart". The supported SoCs include
>>>> - imx23 and imx28.
>>>> + imx23 and imx28. Or "alphascale,asm9260-auart".
>>>
>>> Can you rewrite this to make it clear which compatible string is
>>> appropriate for which SoC?
>>
>> I would assume it is self descriptive, since "compatible" usually
>> contains: company name; SoC name and component. Any way, if it is
>> required i will change it.
>
> I'm thinking of the next 5 additions that go:
>
> - imx23 and imx28. Or "alphascale,asm9260-auart".
> + imx23 and imx28. Or "alphascale,asm9260-auart". Or "mySoC, that-uart".
>
> - imx23 and imx28. Or "alphascale,asm9260-auart". Or "mySoC, that-uart".
> + imx23 and imx28. Or "alphascale,asm9260-auart". Or "mySoC, that-uart". Or
> + "theirSoC, their-uart".
>
> etc, etc. Better just to format it cleanly now.
ok.
>>>> - reg : Address and length of the register set for the device
>>>> - interrupts : Should contain the auart interrupt numbers
>>>> - dmas: DMA specifier, consisting of a phandle to DMA controller node
>>>> #define AUART_STAT 0x00000070
>>>> #define AUART_DEBUG 0x00000080
>>>> @@ -136,11 +123,387 @@
>>>> #define AUART_STAT_FERR (1 << 16)
>>>> #define AUART_STAT_RXCOUNT_MASK 0xffff
>>>>
>>>> +/* Start of Alphascale asm9260 defines */
>>>
>>> Why are you (re)defining all these bits below when they're identical
>>> to the MXS register layout?
>>
>> To provide some kind of documentation. As you can see, there are more
>> registers as by iMX controller. Not one else have access to docs. In any
>> case, if it is not welcome i'll remove it.
>
> For any bits that are not like iMX either in behavior or location,
> that'd be great. Otherwise, it's just noise.
Ok, i'll create kind of diff list. Still not sure if it will reduce the
noise, because now there is no visible proof if the some bit exist on
all configurations.
>>>> +/* RX ctrl register */
>>>> +#define ASM9260_HW_CTRL0 0x0000
>>>> +/* RW. Set to zero for normal operation. */
>> ....
>>>> static inline bool auart_dma_enabled(struct mxs_auart_port *s)
>>>> {
>>>> return s->flags & MXS_AUART_DMA_ENABLED;
>>>> @@ -295,19 +670,19 @@ static void mxs_auart_tx_chars(struct mxs_auart_port *s)
>>>> }
>>>>
>>>>
>>>> - while (!(readl(s->port.membase + AUART_STAT) &
>>>> + while (!(readl(s->regs.stat) &
>>>> AUART_STAT_TXFF)) {
>>>> if (s->port.x_char) {
>>>> s->port.icount.tx++;
>>>> writel(s->port.x_char,
>>>> - s->port.membase + AUART_DATA);
>>>> + s->regs.data);
>>>
>>> The convention for adding clones that trivially move register addresses
>>> is to add i/o accessors that perform the register lookup. For example,
>>>
>>> writel(s->port.xchar, s->port.membase + AUART_DATA);
>>>
>>> becomes
>>>
>>> mxs_write(s->port.xchar, s, AUART_DATA);
>>
>> I assume we have here some confusion.
>> It was:
>>
>> writel(s->port.x_char, s->port.membase + AUART_DATA);
>>
>> and i changed it to:
>>
>> writel(s->port.x_char, s->regs.data);
>>
>> what kind of advantage will bring new IO function?
>
> Ease of maintenance.
> Plus the inevitable follow-on clone that specializes other registers.
> Just went through this with the amba-pl011 driver.
I understand what you mean, but i still do not understand how it will
help in this driver. The reason why i do not understand is - we have 3
kind of write options in this driver: write; set and remove. It means i
will add 3 new functions for write and one to read. At same time i do
not see any advantages for maintenance or readability.
Or may be i still misunderstand you.
--
Regards,
Oleksij
Attachment:
signature.asc
Description: OpenPGP digital signature
