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 [off-list ref] wrote:
[...]
quoted
quoted
+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.
[...]
quoted
quoted
+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.
[...]
quoted
quoted
+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.
quoted
quoted
+
+ 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
quoted
quoted
+ return 0;