Hi Thomas,

On Mon, Apr 08, 2019 at 04:20:58PM +0200, Thomas Bogendoerfer wrote:

To compile this driver as a module...

serio->port_data is void pointer, no need to cast.

+	unsigned long timeout = 0;
+	u32 mask;
+
+	mask = (dev == d->aux) ? KM_CSR_M_WRT_PEND : KM_CSR_K_WRT_PEND;
+	while ((readl(&d->regs->km_csr) & mask) && (timeout < 1000)) {
+		udelay(100);
+		timeout++;
+	}
+
+	if (dev == d->aux)
+		writel(((u32)val) & 0x000000ff, &d->regs->m_wd);
+	else
+		writel(((u32)val) & 0x000000ff, &d->regs->k_wd);

Why do you need to mask the upper bits if you start with u8? Can you
just say:

	writel(val, dev == d->aux ? &d->regs->m_wd, &d->regs->k_wd);

+
+	if (timeout >= 1000)
+		return -1;

So you attempt to write even if controller indicates that it is busy?

+	return 0;
+}
+
+static irqreturn_t ioc3kbd_intr(int itq, void *dev_id)
+{
+	struct ioc3kbd_data *d = dev_id;
+	u32 data_k, data_m;
+
+	data_k = readl(&d->regs->k_rd);
+	data_m = readl(&d->regs->m_rd);
+
+	if (data_k & KM_RD_VALID_0)
+		serio_interrupt(d->kbd,
+		(data_k >> KM_RD_DATA_0_SHIFT) & 0xff, 0);
+	if (data_k & KM_RD_VALID_1)
+		serio_interrupt(d->kbd,
+		(data_k >> KM_RD_DATA_1_SHIFT) & 0xff, 0);
+	if (data_k & KM_RD_VALID_2)
+		serio_interrupt(d->kbd,
+		(data_k >> KM_RD_DATA_2_SHIFT) & 0xff, 0);
+	if (data_m & KM_RD_VALID_0)
+		serio_interrupt(d->aux,
+		(data_m >> KM_RD_DATA_0_SHIFT) & 0xff, 0);
+	if (data_m & KM_RD_VALID_1)
+		serio_interrupt(d->aux,
+		(data_m >> KM_RD_DATA_1_SHIFT) & 0xff, 0);
+	if (data_m & KM_RD_VALID_2)
+		serio_interrupt(d->aux,
+		(data_m >> KM_RD_DATA_2_SHIFT) & 0xff, 0);
+
+	return 0;
+}
+
+static int ioc3kbd_open(struct serio *dev)
+{
+	return 0;
+}
+
+static void ioc3kbd_close(struct serio *dev)
+{
+	/* Empty */
+}

You do not need empty stubs.

+
+static struct ioc3kbd_data *ioc3kbd_alloc_port(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ioc3_serioregs __iomem *regs;
+	struct ioc3kbd_data *d;
+	struct serio *sk, *sa;
+	struct resource *mem;
+	int irq;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mem)
+		return NULL;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return NULL;
+
+	regs = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
+	if (!regs)
+		return NULL;
+
+	sk = devm_kzalloc(dev, sizeof(struct serio), GFP_KERNEL);
+	if (!sk)
+		return NULL;
+
+	sa = devm_kzalloc(dev, sizeof(struct serio), GFP_KERNEL);
+	if (!sa)
+		return NULL;

serio ports are refcounted, they must not be allocated with devm_* API
as their lifetime may extend past the remove() time.

+
+	d = devm_kzalloc(dev, sizeof(struct ioc3kbd_data),
+			 GFP_KERNEL);
+	if (!d)
+		return NULL;
+
+	if (request_irq(irq, ioc3kbd_intr, IRQF_SHARED, "ioc3-kbd", d))
+		return NULL;

This is unfortunate that you lose error code. You also fail to handle
-EPROPE_DEFER from request_irq().

+
+	sk->id.type = SERIO_8042;
+	sk->write = ioc3kbd_write;
+	sk->open = ioc3kbd_open;
+	sk->close = ioc3kbd_close;
+	snprintf(sk->name, sizeof(sk->name), "IOC3 keyboard %d", pdev->id);
+	snprintf(sk->phys, sizeof(sk->phys), "ioc3/serio%dkbd", pdev->id);
+	sk->port_data = d;
+	sk->dev.parent = &pdev->dev;
+
+	sa->id.type = SERIO_8042;
+	sa->write = ioc3kbd_write;
+	sa->open = ioc3kbd_open;
+	sa->close = ioc3kbd_close;
+	snprintf(sa->name, sizeof(sa->name), "IOC3 auxiliary %d", pdev->id);
+	snprintf(sa->phys, sizeof(sa->phys), "ioc3/serio%daux", pdev->id);
+	sa->port_data = d;
+	sa->dev.parent = dev;
+
+	d->regs = regs;
+	d->kbd = sk;
+	d->aux = sa;
+	return d;
+}
+
+static int ioc3kbd_probe(struct platform_device *pdev)
+{
+	struct ioc3kbd_data *d;
+
+	d = ioc3kbd_alloc_port(pdev);
+	if (!d)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, d);
+	serio_register_port(d->kbd);
+	serio_register_port(d->aux);
+
+	return 0;
+}
+
+static int ioc3kbd_remove(struct platform_device *pdev)
+{
+	struct ioc3kbd_data *d = platform_get_drvdata(pdev);
+
+	serio_unregister_port(d->kbd);
+	serio_unregister_port(d->aux);
+	return 0;
+}
+
+static struct platform_driver ioc3kbd_driver = {
+	.probe          = ioc3kbd_probe,
+	.remove         = ioc3kbd_remove,
+	.driver = {
+		.name = "ioc3-kbd",
+	},
+};
+module_platform_driver(ioc3kbd_driver);
+
+MODULE_AUTHOR("Stanislaw Skowronek [off-list ref]");
+MODULE_DESCRIPTION("SGI IOC3 serio driver");
+MODULE_LICENSE("GPL");

"GPL v2" to match SPDX header?

-- 
2.13.7

Thanks.

-- 
Dmitry