Re: Serial driver for ARM s3c4510

[Roland Caßebohm <roland dot cassebohm at visionsystems dot de>]

On Dienstag, 21. October 2003 17:29, Jonathan Larmour wrote:
> Andrew Lunn wrote:
> >>Additionally the AIM platform includes a 16550 UART, so this serial
> >> interface should be implemented as /dev/ser2 also.
> >>Is it possible and useful to make a generic s3c4510 serial driver, use
> >> the generic 16x5x driver but make only one package (devs/serial/arm/aim)
> >> which includes this two drivers. Or is it better to make two packages
> >> like devs/serial/arm/aim_s3c4510 and devs/serial/arm/aim_16x5x?
> >
> > I would go for two packages. There is no logical connection between
> > the two, so why should they share a package? What happens if in the
> > future you replace your 16x5x with something else etc?
> >
> > You are the second person who recently needed two different serial
> > drivers in the same configuration. I don't really know the serial
> > driver architecture that well, but from when i looked last the
> > architecture is not very good at this. If you have time, it would be
> > good it you could study the architecture and see if you can make is
> > better for supporting multiple devices.
>
> I think for that reason it may be best to include all platform specific
> elements of a serial driver in a single package. But even then, it's
> possible there might be hiccups.
>
> The most obvious issue is this in e.g. the 16x5x driver:
>          puts $::cdl_system_header "#ifndef CYGDAT_IO_SERIAL_DEVICE_HEADER"
>          puts $::cdl_system_header "#define CYGDAT_IO_SERIAL_DEVICE_HEADER
> <pkgconf/io_serial_generic_16x5x.h>"
>
> However I'm not sure what info actually _must_ be made public like this,
> other than e.g.:
>          cdl_option CYGPRI_SER_TEST_SER_DEV {
>              display       "Serial device used for testing"
>              flavor        data
>              default_value { CYGDAT_IO_SERIAL_I386_PC_SERIAL0_NAME }
>          }
>
>          define_proc {
>              puts $::cdl_header "#define CYGPRI_SER_TEST_CRASH_ID
> \"i386pc\"" puts $::cdl_header "#define CYGPRI_SER_TEST_TTY_DEV
> \"/dev/tty0\""
>          }
>
> There would be two ways to fix this probably.... change those #defines to
> $::cdl_system_header instead, so that the generic serial layer can get
> them that way; or create a CYGPRI_SER_TEST_CRASH_ID and
> CYGPRI_SER_TEST_TTY_DEV option in the generic serial driver (or perhaps a
> subtle rename to avoid clashes with existing clashes) and use CDL requires
> statements in the child packages to set values. The latter would be
> cleaner as it doesn't involve touching system.h which could cause
> unnecessary rebuilds.
>
> It's a shame that CDL has this restriction with the "define" CDL property:
> "The -file option can be used to specify an alternative destination. At
> the time of writing the only valid alternative definition is
> -file=system.h, which will send the output to the global configuration
> header file pkgconf/system.h." I'll create a bugzilla bug for this.
>
> Jifl

What I have done now is I have two packages (aim711_s3c4510, aim711_16x5x) 
which configures the generic serial drivers s3c4510 and 16x5x.
But additionally I have made a package which includes only one cdl file 
looking like that:

cdl_package CYGPKG_IO_SERIAL_ARM_AIM711 {
    display       "ARM Industrial Module AIM 711 serial device drivers"

    parent        CYGPKG_IO_SERIAL_DEVICES
    active_if     CYGPKG_IO_SERIAL

    include_dir   cyg/io
    include_files ; # none _exported_ whatsoever
    description   "
           This package contains serial device drivers for the
           ARM Industrial Module AIM 711."

    define_proc {
        puts $::cdl_system_header "/** serial driver proc output start **/"
        puts $::cdl_system_header "#ifndef CYGDAT_IO_SERIAL_DEVICE_HEADER"
        puts $::cdl_system_header "#define CYGDAT_IO_SERIAL_DEVICE_HEADER
                     <pkgconf/io_serial_arm_aim711.h>"
        puts $::cdl_system_header "#endif"
        puts $::cdl_system_header "/**  serial driver proc output end  **/"
        puts $::cdl_header "#include <pkgconf/system.h>";
        puts $::cdl_header "#include <pkgconf/io_serial_arm_s3c4510.h>";
        puts $::cdl_header "#include <pkgconf/io_serial_generic_16x5x.h>";
        puts $::cdl_header "#include CYGDAT_IO_SERIAL_ARM_S3C4510_CFG";
        puts $::cdl_header "#include CYGDAT_IO_SERIAL_GENERIC_16X5X_CFG";
    }
}

Now both configurations are included in CYGDAT_IO_SERIAL_DEVICE_HEADER.
The cdl_option CYGPRI_SER_TEST_SER_DEV I have only implemented in one package 
(aim711_16x5x), so there is no conflict. Maybe this could be made 
congureable.

But if I do it like that, I have three directorys for AIM in devs/serial/arm 
(aim711 aim_s3c4510 aim711_16x5x). Maybe I should make subdirectorys under 
devs/arm/aim711? Or I however make it to only one package because it is only 
one platform?

Roland