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.
>
>> - 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.
>
>> +/* 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?
--
Regards,
Oleksij
Attachment:
signature.asc
Description: OpenPGP digital signature
