Hello Arnd,
On Fri, Dec 23, 2011 at 10:35:22AM +0000, Arnd Bergmann wrote:
> On Thursday 22 December 2011, Uwe Kleine-König wrote:
>
> > @@ -0,0 +1,14 @@
> > +* Energymicro efm32 UART
> > +
> > +Required properties:
> > +- compatible : Should be "efm32,usart"
> > +- reg : Address and length of the register set
> > +- interrupts : Should contain uart interrupt
> > +
> > +Example:
> > +
> > +uart@0x4000c400 {
> > + compatible = "efm32,usart";
> > + reg = <0x4000c400 0x400>;
> > + interrupts = <15>;
> > +};
>
> Do you know if the usart was actually designed by energymicro or licensed
> from another party? If it is a licensed part, it would be better to
> list the "compatible" value under the company name that made it.
I don't know so I passed the question to them.
> I would suggest that you also support the "clock-frequency" and/or
> "current-speed" properties that are defined for serial ports, see the
> of_serial driver.
I will have a look to find out what they mean and update the patch
accordingly.
> > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> > index 925a1e5..cfeb0f3 100644
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -1610,4 +1610,14 @@ config SERIAL_XILINX_PS_UART_CONSOLE
> > help
> > Enable a Xilinx PS UART port to be the system console.
> >
> > +config SERIAL_EFM32_USART
> > + bool "EFM32 USART port."
> > + depends on ARCH_EFM32
> > + select SERIAL_CORE
>
> Why not tristate? If it's not a console, you should be able to use
> it as a module.
Hmm, in theory yes. (In practice modules on mmu are not that trivial,
see the corresponding thread on lakml.) I will update but I guess I can
only build test that configuration.
> I would generally prefer not to make the driver depend on the
> platform, so we can get better compile time coverage. I think a better
> set of dependencies would be
>
> depends on HAVE_CLK
> depends on OF
> default ARCH_EFM32
I'd prefer something like:
depends on HAVE_CLK
depends on ARCH_EFM32 || I_DO_BUILD_COVERAGE_TESTING
This would make it easier for Joe User to pick the right options for his
kernel (assuming he found out to better keep I_DO_BUILD_COVERAGE_TESTING
disabled). There is no hard dependency on OF in the driver and I guess
at least the in-production (and out-of-tree) usage will still stick to
non-OF because it saves some RAM. So I'd prefer not to add that.
(And it would help build coverage testing ;-)
> > +static void efm32_usart_write32(struct efm32_usart_port *efm_port,
> > + u32 value, unsigned offset)
> > +{
> > + __raw_writel(value, efm_port->port.membase + offset);
> > +}
> > +
> > +static u32 efm32_usart_read32(struct efm32_usart_port *efm_port,
> > + unsigned offset)
> > +{
> > + return __raw_readl(efm_port->port.membase + offset);
> > +}
>
> Please use writel_relaxed() instead of __raw_writel().
ok
> > +static int __devinit efm32_usart_probe(struct platform_device *pdev)
> > +{
> > + struct efm32_usart_port *efm_port;
> > + struct resource *res;
> > + int ret;
> > +
> > + efm_port = kzalloc(sizeof(*efm_port), GFP_KERNEL);
> > + if (!efm_port) {
> > + dev_dbg(&pdev->dev, "failed to allocate private data\n");
> > + return -ENOMEM;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res) {
> > + ret = -ENODEV;
> > + dev_dbg(&pdev->dev, "failed to determine base address\n");
> > + goto err_get_base;
> > + }
> > +
> > + if (resource_size(res) < 60) {
> > + ret = -EINVAL;
> > + dev_dbg(&pdev->dev, "memory resource too small\n");
> > + goto err_too_small;
> > + }
>
> of_iomap() would be simpler here. I think you can leave out the assignment to
> mapbase, and go straight to membase here.
but ioremap is only done in .request_port, so it's necessary to have two
steps here, isn't it?
> checking the resource size should not be necessary, because that would imply
> having an invalid device tree, which you don't have to check at run time.
hmm, I prefer an error message over an access to unmapped memory even if
that can only happen when the device tree is broken.
Best regards and thanks for your comments,
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
