Re: [PATCH v2 13/13] tty: pruss SUART driver

From: Subhasish Ghosh
Date: Wed Feb 23 2011 - 08:34:47 EST


Hello,

Anything regarding this.

--------------------------------------------------
From: "Subhasish Ghosh" <subhasish@xxxxxxxxxxxxxxxxxxxx>
Sent: Wednesday, February 23, 2011 11:00 AM
To: "Greg KH" <gregkh@xxxxxxx>; "Alan Cox" <alan@xxxxxxxxxxxxxxxxxxx>
Cc: "Arnd Bergmann" <arnd@xxxxxxxx>; <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; "Thomas Gleixner" <tglx@xxxxxxxxxxxxx>; <sachi@xxxxxxxxxxxxxxxxxxxx>; <davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx>; <nsekhar@xxxxxx>; "open list" <linux-kernel@xxxxxxxxxxxxxxx>; <m-watkins@xxxxxx>; "Stalin Srinivasan" <stalin.s@xxxxxxxxxxxxxxxxxxxx>
Subject: Re: [PATCH v2 13/13] tty: pruss SUART driver

I could not follow the recommendations clearly.
This is just to clarify.

Currently, I have the following files for the suart implementation:

drivers/tty/serial/da8xx_pruss/pruss_suart_api.h
drivers/tty/serial/da8xx_pruss/pruss_suart_err.h
drivers/tty/serial/da8xx_pruss/pruss_suart_regs.h
drivers/tty/serial/da8xx_pruss/pruss_suart_board.h
drivers/tty/serial/da8xx_pruss/pruss_suart_mcasp.h
drivers/tty/serial/da8xx_pruss/pruss_suart_utils.h

drivers/tty/serial/da8xx_pruss/pruss_suart_api.c
drivers/tty/serial/da8xx_pruss/pruss_suart.c
drivers/tty/serial/da8xx_pruss/pruss_suart_utils.c

Of these, I will be removing pruss_suart_err.h as part of the Linux error code cleanup.
But, I need to keep at least pruss_suart_board.h as a separate file, as this defines
configurations which will be often modified by users, I don't want to mix it with other files.

Should I combine rest of the headers into a single file ? and keep the other three .c files under "drivers/tty/serial/"
and remove the da8xx_pruss directory altogether.


--------------------------------------------------
From: "Greg KH" <gregkh@xxxxxxx>
Sent: Tuesday, February 22, 2011 8:07 PM
To: "Subhasish Ghosh" <subhasish@xxxxxxxxxxxxxxxxxxxx>
Cc: "Arnd Bergmann" <arnd@xxxxxxxx>; <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; "Thomas Gleixner" <tglx@xxxxxxxxxxxxx>; "Alan Cox" <alan@xxxxxxxxxxxxxxxxxxx>; <sachi@xxxxxxxxxxxxxxxxxxxx>; <davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx>; <nsekhar@xxxxxx>; "open list" <linux-kernel@xxxxxxxxxxxxxxx>; <m-watkins@xxxxxx>
Subject: Re: [PATCH v2 13/13] tty: pruss SUART driver

On Tue, Feb 22, 2011 at 02:12:32PM +0530, Subhasish Ghosh wrote:
Hello,

I had kept separate files to affirm the modularity and ease of
portability of the system.

There are three different interfaces,
1. The Linux driver interface
2. The PRU control interface
3. The McASP serializer interface.

To maintain modularity, I had classified the files respectively as :
1. pruss_suart.c
2. pruss_suart_api.c
3. pruss_suart_utils.c

This is not a single device which can be expressed as a single file,
but functionally different devices logically cascaded together to
work in unison.

We use the PRU for packet processing, but the actual data is
transmitted/received through the
McASP, which we use as a serializer.

I feel to combine these disparate functionalities into a single file
will not

1. Help better understanding the device. I mean, why should a TTY
UART driver be aware of the McASP or the PRU.
2. In case of a bug in the API layer or McASP, the driver need not
be touched, thus improve maintainability.
3. If we need to port it to another Linux version, just editing the
driver file should suffice, this will reduce bugs while porting.

If your code is in the kernel tree, you do not need to ever port it to a
new version, as it will happen automatically as new kernels are
released, so this really isn't anything to worry about.

To me, combining all of these into a single file only creates a
mess. This is the reason I had separated them into different files!!
I don't understand why should it be better to have all of these into
a single file.

As Alan stated, just use 3 files in the directory with the other
drivers, you don't need a subdir for something small like this.

thanks,

greg k-h

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/