| 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
| ||
|
Date: Fri, 23 Jan 2015 08:12:55 -0500
From: Peter Hurley <peter@...leysoftware.com>
To: Lyra Zhang <zhang.lyra@...il.com>
CC: Chunyan Zhang <chunyan.zhang@...eadtrum.com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Arnd Bergmann <arnd@...db.de>,
"gnomes@...rguk.ukuu.org.uk" <gnomes@...rguk.ukuu.org.uk>,
Shawn Guo <shawn.guo@...aro.org>,
Pawel Moll <pawel.moll@....com>,
"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
"jslaby@...e.cz" <jslaby@...e.cz>,
Grant Likely <grant.likely@...aro.org>,
Heiko Stübner <heiko@...ech.de>,
"jason@...edaemon.net" <jason@...edaemon.net>,
"florian.vaussard@...l.ch" <florian.vaussard@...l.ch>,
"andrew@...n.ch" <andrew@...n.ch>,
Hayato Suzuki <hytszk@...il.com>,
"antonynpavlov@...il.com" <antonynpavlov@...il.com>,
Orson Zhai <orsonzhai@...il.com>,
"geng.ren@...eadtrum.com" <geng.ren@...eadtrum.com>,
"zhizhou.zhang" <zhizhou.zhang@...eadtrum.com>,
"lanqing.liu@...eadtrum.com" <lanqing.liu@...eadtrum.com>,
"Wei Qiao (乔伟)" <wei.qiao@...eadtrum.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>
Subject: Re: [PATCH v6 2/2] tty/serial: Add Spreadtrum sc9836-uart driver
support
On 01/23/2015 02:23 AM, Lyra Zhang wrote:
> Hi, Peter
>
> Many thanks to you for reviewing so carefully and giving us so many
> suggestions and so clear explanations.
:)
> I'll address all of your comments and send an updated patch soon.
>
> On Fri, Jan 23, 2015 at 11:57 AM, Peter Hurley <peter@...leysoftware.com> wrote:
[...]
>>> +static void sprd_set_termios(struct uart_port *port,
>>> + struct ktermios *termios,
>>> + struct ktermios *old)
>>> +{
>>> + unsigned int baud, quot;
>>> + unsigned int lcr, fc;
>>> + unsigned long flags;
>>> +
>>> + /* ask the core to calculate the divisor for us */
>>> + baud = uart_get_baud_rate(port, termios, old, 1200, 3000000);
>> ^^^^ ^^^^^^
>> mabye derive these from uartclk?
>
> I'm afraid I can't understand very clearly, Could you explain more
> details please?
Is the fixed clock divider == 8 and the uartclk == 26000000 ?
If so,
baud = uartclk / 8 = 3250000
I see now this is clamping baud inside the maximum, so this is fine.
Please disregard my comment.
[...]
>>> +static int sprd_probe(struct platform_device *pdev)
>>> +{
>>> + struct resource *res;
>>> + struct uart_port *up;
>>> + struct clk *clk;
>>> + int irq;
>>> + int index;
>>> + int ret;
>>> +
>>> + for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
>>> + if (sprd_port[index] == NULL)
>>> + break;
>>> +
>>> + if (index == ARRAY_SIZE(sprd_port))
>>> + return -EBUSY;
>>> +
>>> + index = sprd_probe_dt_alias(index, &pdev->dev);
>>> +
>>> + sprd_port[index] = devm_kzalloc(&pdev->dev,
>>> + sizeof(*sprd_port[index]), GFP_KERNEL);
>>> + if (!sprd_port[index])
>>> + return -ENOMEM;
>>> +
>>> + up = &sprd_port[index]->port;
>>> + up->dev = &pdev->dev;
>>> + up->line = index;
>>> + up->type = PORT_SPRD;
>>> + up->iotype = SERIAL_IO_PORT;
>>> + up->uartclk = SPRD_DEF_RATE;
>>> + up->fifosize = SPRD_FIFO_SIZE;
>>> + up->ops = &serial_sprd_ops;
>>> + up->flags = ASYNC_BOOT_AUTOCONF;
>> ^^^^^^^^^
>> UPF_BOOT_AUTOCONF
>>
>> sparse will catch errors like this. See Documentation/sparse.txt
>
> you mean we should use UPF_BOOT_AUTOCONF, right?
Yes. Only UPF_* flag definitions should be used with the uart_port.flags
field.
My comment regarding the sparse tool and documentation is because the
flags field and UPF_* definitions use a type mechanism to generate
warnings using the sparse tool if regular integer values are used
with the flags field.
The type mechanism was specifically introduced to catch using ASYNC_*
definitions with the uart_port.flags field.
[...]
>>> +static int sprd_suspend(struct device *dev)
>>> +{
>>> + int id = to_platform_device(dev)->id;
>>> + struct uart_port *port = &sprd_port[id]->port;
>>
>> I'm a little confused regarding the port indexing;
>> is platform_device->id == line ? Where did that happen?
>>
>
> Oh, I'll change to assign platform_device->id with port->line in probe()
I apologize; I should have made my comment clearer.
The ->id should not be assigned.
Replace
int id = to_platform_device(dev)->id;
struct uart_port *port = &sprd_port[id]->port;
uart_suspend_port(&sprd_uart_driver, port);
with
struct sprd_uart_port *sup = dev_get_drvdata(dev);
uart_suspend_port(&sprd_uart_driver, &sup->port);
I know it's not obvious but platform_get/set_drvdata() is really
dev_get/set_drvdata() using the embedded struct device dev.
>
>>
>>> +
>>> + uart_suspend_port(&sprd_uart_driver, port);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int sprd_resume(struct device *dev)
>>> +{
>>> + int id = to_platform_device(dev)->id;
>>> + struct uart_port *port = &sprd_port[id]->port;
>>> +
>>> + uart_resume_port(&sprd_uart_driver, port);
same here
>>> + return 0;
--
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/
Powered by blists - more mailing lists