linux-kernel - Re: [PATCH v3] serial: 8250_uniphier: add UniPhier serial 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] [day] [month] [year] [list]

Date:	Sat, 23 May 2015 21:55:35 +0900
From:	Masahiro Yamada <yamada.masahiro@...ionext.com>
To:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:	Peter Hurley <peter@...leysoftware.com>,
	Tony Lindgren <tony@...mide.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Joachim Eastwood <manabian@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-serial@...r.kernel.org,
	Matthias Brugger <matthias.bgg@...il.com>,
	Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>,
	John Crispin <blogic@...nwrt.org>, Jiri Slaby <jslaby@...e.cz>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	Alan Cox <alan@...ux.intel.com>
Subject: Re: [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver

Hi Andy,

Thanks for reviewing my patch.


2015-05-20 21:56 GMT+09:00 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>:
> On Tue, 2015-05-19 at 10:11 +0900, Masahiro Yamada wrote:
>> Add the driver for on-chip UART used on UniPhier SoCs.
>>
>> This hardware is similar to 8250 with a slightly different register
>> mapping, so it should go into drivers/tty/serial/8250 directory.
>
> Few comments below.
>
> First of all what is the differences with the original 8250? It worth to
> list them here in the changelog.
>

I noted this in the git-log


>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/serial_8250.h>
>> +#include <linux/serial_core.h>
>> +#include <linux/serial_reg.h>
>
> Shouldn't be enough to have only serial_8250.h here?
>
> + empty line between #include <> and "".


I did so in v4.




>> +static int uniphier_uart_probe(struct platform_device *pdev)
>> +{
>> +     struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     struct resource *irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>
> Please use platform_get_irq().

OK. Fixed.


>> +     struct device *dev = &pdev->dev;
>> +     struct uniphier8250_priv *priv;
>> +     struct uart_8250_port up;
>> +     int ret;
>> +     void __iomem *membase;
>> +
>> +     if (!regs || !irq) {
>> +             dev_err(dev, "missing registers or irq\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +     if (!priv)
>> +             return -ENOMEM;
>> +
>> +     membase = devm_ioremap(dev, regs->start, resource_size(regs));
>
> Shouldn't be devm_ioremap_resource() ?


I tried this, but it broke my driver.

It looks like the cause of error is in the devm_request_mem_region()
called from that function,


8250_core.c also calls request_mem_region().
I think it fails if the memory region is already reserved
before calling serial8250_register_8250_port().

So, I am sticking to devm_ioremap().


-- 
Best Regards
Masahiro Yamada
--
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/