linux-kernel - Re: [RFC 1/8] serial:st-asc: Add ST ASC driver.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20130508143908.GJ24282@game.jcrosoft.org>
Date:	Wed, 8 May 2013 16:39:08 +0200
From:	Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	Srinivas KANDAGATLA <srinivas.kandagatla@...com>,
	linux-doc@...r.kernel.org, Viresh Kumar <viresh.kumar@...aro.org>,
	Will Deacon <will.deacon@....com>, jslaby@...e.cz,
	Russell King <linux@....linux.org.uk>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Nicolas Pitre <nico@...aro.org>,
	Stephen Gallimore <stephen.gallimore@...com>,
	linux-serial@...r.kernel.org, Jason Cooper <jason@...edaemon.net>,
	devicetree-discuss@...ts.ozlabs.org,
	Rob Herring <rob.herring@...xeda.com>,
	Stuart Menefy <stuart.menefy@...com>,
	Stephen Warren <swarren@...dia.com>,
	Dong Aisheng <dong.aisheng@...aro.org>,
	linux-arm-kernel@...ts.infradead.org, gregkh@...uxfoundation.org,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC 1/8] serial:st-asc: Add ST ASC driver.

On 16:34 Wed 08 May     , Arnd Bergmann wrote:
> On Wednesday 08 May 2013, Srinivas KANDAGATLA wrote:
> > From: Srinivas Kandagatla <srinivas.kandagatla@...com>
> 
> > +*st-asc(Serial Port)
> > +
> > +Required properties:
> > +- compatible : Should be "st,asc".
> 
> Are there any hardware revision numbers for the asc? If there are potentially
> incompatible or backwards-compatible variants, it would be good to include
> the version in this string.

The st,asc come from st20 or st200 IIRC which was not ARM but a ST arch
and then used on st40 series

an the st,asc for st200 and st40 are not compatible completly
> 
> > +- reg, reg-names, interrupts, interrupt-names	: Standard way to define device
> > +			resources with names. look in
> > +			Documentation/devicetree/bindings/resource-names.txt
> > +
> > +Optional properties:
> > +- st,hw-flow-ctrl	bool flag to enable hardware flow control.
> > +- st,force-m1		bool flat to force asc to be in Mode-1 recommeded
> > +			for high bit rates (above 19.2K)
> > +Example:
> > +serial@...40000{
> > +    compatible    = "st,asc";
> > +    reg         = <0xfe440000 0x2c>;
> > +    interrupts     =  <0 209 0>;
> > +};
> 
> I would also recommed adding a way to set the default baud rate through
> a property. Following the example of the 8250 driver, you should probably
> call that "current-speed".
on st it has always be 38k

> 
> > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> > index 7e7006f..346f325 100644
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -1484,6 +1484,25 @@ config SERIAL_RP2_NR_UARTS
> >  	  If multiple cards are present, the default limit of 32 ports may
> >  	  need to be increased.
> >  
> > +config SERIAL_ST_ASC
> > +	tristate "ST ASC serial port support"
> > +	depends on PLAT_STIXXXX
> > +	default y
> > +	select SERIAL_CORE
> > +	help
> > +	  This driver is for the on-chip Asychronous Serial Controller on
> > +	  STMicroelectronics STixxxx SoCs.
> > +	  ASC is embedded in ST COMMS IP block. It supports Rx & Tx functionality.
> > +	  It support all industry standard baud rates.
> > +
> > +	  If unsure, say N.
> 
> I would not make it "default y".
me too
> 
> > +config SERIAL_ST_ASC_CONSOLE
> > +	bool "Support for console on ST ASC"
> > +	depends on SERIAL_ST_ASC
> > +	default y
> > +	select SERIAL_CORE_CONSOLE
> > +
> >  endmenu
> 
> This needs to be "depends on SERIAL_ST_ASC=y". You would get a link error
> if you try to make SERIAL_ST_ASC a loadable module and SERIAL_ST_ASC_CONSOLE
> built-in.
> 
> > +
> > +static struct asc_port asc_ports[ASC_MAX_PORTS];
> > +static struct uart_driver asc_uart_driver;
> > +
> > +/*---- Forward function declarations---------------------------*/
> > +static irqreturn_t asc_interrupt(int irq, void *ptr);
> > +static void asc_transmit_chars(struct uart_port *);
> > +static int asc_set_baud(struct asc_port *ascport, int baud);
> > +
> 
> Please remove all forward declarations, by reordering the functions in
> the way they are called.
> 
and drop the dummy functions
> 
> > diff --git a/drivers/tty/serial/st-asc.h b/drivers/tty/serial/st-asc.h
> > new file mode 100644
> > index 0000000..e59f818
> > --- /dev/null
> > +++ b/drivers/tty/serial/st-asc.h
> > +#ifndef _ST_ASC_H
> > +#define _ST_ASC_H
> > +
> > +#include <linux/serial_core.h>
> > +#include <linux/clk.h>
> > +
> > +struct asc_port {
> > +	struct uart_port port;
> > +	struct clk *clk;
> > +	unsigned int hw_flow_control:1;
> > +	unsigned int check_parity:1;
> > +	unsigned int force_m1:1;
> > +};
> 
> Since this header file is only used in one place, just merge it into
> the driver itself.
> 
> > +#define ASC_MAJOR		204
> > +#define ASC_MINOR_START		40
> 
> I don't know what the current policy is on allocating major/minor numbers,
> but I'm sure you cannot just reuse one that is already used.
> 
> Documentation/devices.txt lists the ones that are officially assigned.
> Can't you use dynamic allocation here?
> 
> 	Arnd
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@...ts.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/