RFC: MFD: driver for Atmel Microcontroller on iPaq h3xxx

This adds a driver for the Atmel Microcontroller found on the
iPAQ h3xxx series. This device handles some keys, the
touchscreen, and the battery monitoring.

This is a port of a driver from handhelds.org 2.6.21 kernel,
written by Alessandro Gardich based on Andrew Christians
original HAL-driver. It has been heavily cleaned and
converted to mfd-core by Dmitry Artamonow and rewritten
again for the v3.x series kernels by Linus Walleij,
bringing back some of the functionality lost from Andrew's
original driver.

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Signed-off-by: Alessandro Gardich <gremlin@gremlin.it>
Signed-off-by: Dmitry Artamonow <mad_soft@inbox.ru>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Hi Greg, Sam, Lee:

This is a peculiar driver I found out-of-tree. Basically the
problem with it is this: the UART port is used only for
kernel-internal purposes, and exposing it to userspace or
making it subject of a line discipline is irrelevant.

Compare to SPI or I2C buses: those have custom devices
connected to them, they appear as kernel-internal things.
i2c_register_board_info() or spi_register_board_info()
can be used to hook a device directly to these buses
with no userspace interaction.

For UART devices these are implicitly assumed to be TTYs
or modems and thus basically userspace business. But this
thing isn't: this MFD device spawns a backlight, LED,
power (battery) etc, and nothing userspace-related at all.

They actually have to come up *before* userspace is up,
trying to attach a line discipline from userspace would
just be shoehorning.

The solution in this current driver is to cut out the
problem by accessing the serial port registers directly to
send/recieve messages to the device, rather than reusing
the code in tty/serial/sa1100.c in this case.

Any hints what to do about creatures like this?

Part of me wants to create a mechanism for kernel-internal
devices using UARTs, part of me wants to do like this
patch does just to avoid the hassle. So I'm ambivalent
about this whole thing.
---
 drivers/mfd/Kconfig            |  10 +
 drivers/mfd/Makefile           |   1 +
 drivers/mfd/ipaq-micro.c       | 488 +++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/ipaq-micro.h | 149 +++++++++++++
 4 files changed, 648 insertions(+)
 create mode 100644 drivers/mfd/ipaq-micro.c
 create mode 100644 include/linux/mfd/ipaq-micro.h

On Fri, Jan 31, 2014 at 2:34 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> This adds a driver for the Atmel Microcontroller found on the
> iPAQ h3xxx series. This device handles some keys, the
> touchscreen, and the battery monitoring.

Any opinions on this driver or should we queue it in MFD as-is?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

> > This adds a driver for the Atmel Microcontroller found on the
> > iPAQ h3xxx series. This device handles some keys, the
> > touchscreen, and the battery monitoring.
> 
> Any opinions on this driver or should we queue it in MFD as-is?

I have no problem taking this through the MFD tree with Greg's nod.

> This adds a driver for the Atmel Microcontroller found on the
> iPAQ h3xxx series. This device handles some keys, the
> touchscreen, and the battery monitoring.
> 
> This is a port of a driver from handhelds.org 2.6.21 kernel,
> written by Alessandro Gardich based on Andrew Christians
> original HAL-driver. It has been heavily cleaned and
> converted to mfd-core by Dmitry Artamonow and rewritten
> again for the v3.x series kernels by Linus Walleij,
> bringing back some of the functionality lost from Andrew's
> original driver.
> 
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> Signed-off-by: Alessandro Gardich <gremlin@gremlin.it>
> Signed-off-by: Dmitry Artamonow <mad_soft@inbox.ru>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

>  drivers/mfd/Kconfig            |  10 +
>  drivers/mfd/Makefile           |   1 +
>  drivers/mfd/ipaq-micro.c       | 488 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/ipaq-micro.h | 149 +++++++++++++
>  4 files changed, 648 insertions(+)
>  create mode 100644 drivers/mfd/ipaq-micro.c
>  create mode 100644 include/linux/mfd/ipaq-micro.h

<snip>

> +++ b/drivers/mfd/ipaq-micro.c
> @@ -0,0 +1,488 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

Can you put this at the bottom of the header?

> + * Compaq iPAQ h3xxx Atmel microcontroller companion support
> + *
> + * This is an Atmel AT90LS8535 with a special flashed-in firmware that
> + * implements the special protocol used by this driver.
> + *
> + * based on previous kernel 2.4 version by Andrew Christian
> + * Author : Alessandro Gardich <gremlin@gremlin.it>
> + * Author : Dmitry Artamonow <mad_soft@inbox.ru>
> + * Author : Linus Walleij <linus.walleij@linaro.org>
> + */
> +
> +#include <linux/module.h>

Does it matter that you're using:

  module_platform_driver();

... yet you can't build this as a module?

<snip>

> +static void ipaq_micro_trigger_tx(struct ipaq_micro *micro)
> +{
> +	struct ipaq_micro_txdev *tx = &micro->tx;
> +	struct ipaq_micro_msg *msg = micro->msg;
> +	int i, j;
> +	u8 checksum;
> +	u32 val;
> +
> +	j = 0;

Tiny nit:
  The naming convention of 'j' could be more indicative of its function. 

> +	tx->buf[j++] = (u8) CHAR_SOF;

Is this cast required?

> +	checksum = ((msg->id & 0x0f) << 4) | (msg->tx_len & 0x0f);
> +	tx->buf[j++] = checksum;
> +
> +	for (i = 0; i < msg->tx_len; i++) {
> +		tx->buf[j++] = msg->tx_data[i];
> +		checksum += msg->tx_data[i];
> +	}
> +
> +	tx->buf[j++] = checksum;
> +	tx->len = j;
> +	tx->index = 0;
> +	print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_OFFSET, 16, 1,
> +		       tx->buf, tx->len, true);
> +
> +

Too many new lines.

> +	val = readl(micro->base + UTCR3);
> +	val |= UTCR3_TIE; /* enable interrupt */
> +	writel(val, micro->base + UTCR3);

The comment should encompass all three lines of code.

> +}
> +
> +

Too many new lines.

> +int ipaq_micro_tx_msg(struct ipaq_micro *micro, struct ipaq_micro_msg *msg)
> +{
> +	unsigned long flags;
> +
> +	dev_dbg(micro->dev, "TX msg: %02x, %d bytes\n", msg->id, msg->tx_len);
> +
> +	spin_lock_irqsave(&micro->lock, flags);
> +	if (micro->msg != NULL) {

if (!micro->msg) is preferred.

> +		list_add_tail(&msg->node, &micro->queue);
> +		spin_unlock_irqrestore(&micro->lock, flags);
> +		return 0;
> +	}
> +	micro->msg = msg;
> +	ipaq_micro_trigger_tx(micro);
> +	spin_unlock_irqrestore(&micro->lock, flags);
> +	return 0;
> +}
> +EXPORT_SYMBOL(ipaq_micro_tx_msg);
> +
> +static void micro_rx_msg(struct ipaq_micro *micro, u8 id, int len, u8 *data)
> +{
> +	int i;
> +

<snip>

> +	case MSG_KEYBOARD:
> +		if (micro->key != NULL)

!micro->key

> +			micro->key(micro->key_data, len, data);
> +		else
> +			dev_dbg(micro->dev, "ipaq micro : key message ignored, "
> +				"no handle \n");

Log print spread over two lines. Doesn't Checkpatch complain about
this?

> +		break;
> +	case MSG_TOUCHSCREEN:
> +		if (micro->ts != NULL)

!micro->ts ...  etc etc

> +			micro->ts(micro->ts_data, len, data);
> +		else
> +			dev_dbg(micro->dev, "ipaq micro : touchscreen message "
> +				"ignored, no handle \n");

Print spill.

> +static char *ipaq_micro_str(u8 *wchar, u8 len)
> +{
> +	char retstr[256];
> +	u8 i;
> +
> +	for (i = 0; i < len / 2; i++)
> +		retstr[i] = wchar[i*2];

It's common practice to space out the [i*2] for readability.

<snip>

> +static void micro_tx_chars(struct ipaq_micro *micro)
> +{
> +	struct ipaq_micro_txdev *tx = &micro->tx;
> +	u32 val;
> +
> +	while ((tx->index < tx->len) &&
> +	       (readl(micro->base + UTSR1) & UTSR1_TNF)) {
> +		writel(tx->buf[tx->index], micro->base + UTDR);

This is pretty messy. Better broken out? Your call.

<snip>

> +	/* Stop interrupts */
> +	val = readl(micro->base + UTCR3);
> +	val &= ~UTCR3_TIE;
> +	writel(val, micro->base + UTCR3);

Ah, so it's correct here. Please see my comment surrounding "start
interrupts" above.

> +}
> +
> +static void micro_reset_comm(struct ipaq_micro *micro)
> +{
> +	struct ipaq_micro_rxdev *rx = &micro->rx;
> +	u32 val;

New line here please.

> +	if (micro->msg)
> +		complete(&micro->msg->ack);
> +
> +	/* Initialize Serial channel protocol frame */
> +	rx->state = STATE_SOF;  /* Reset the state machine */
> +
> +	/* Set up interrupts */
> +	writel(0x01, micro->sdlc + 0x0); /* Select UART mode */
> +
> +	/* Clean up CR3 */
> +	writel(0x0, micro->base + UTCR3);
> +	/* Format: 8N1 */
> +	writel(UTCR0_8BitData | UTCR0_1StpBit, micro->base + UTCR0);
> +	/* Baud rate: 115200 */
> +	writel(0x0, micro->base + UTCR1);
> +	writel(0x1, micro->base + UTCR2);

Any reason for squishing these together?

> +	/* Clear SR0 */
> +	writel(0xff, micro->base + UTSR0);
> +	/* Enable RX int */
> +	writel(UTCR3_TXE | UTCR3_RXE | UTCR3_RIE, micro->base + UTCR3);
> +	val = readl(micro->base + UTCR3);
> +	val &= ~UTCR3_TIE; /* Disable TX int */
> +	writel(val, micro->base + UTCR3);
> +}

<snip>

> +static struct mfd_cell micro_cells[] = {
> +	{
> +		.name = "ipaq-micro-backlight",
> +	},
> +	{
> +		.name = "ipaq-micro-battery",
> +	},
> +	{
> +		.name = "ipaq-micro-keys",
> +	},
> +	{
> +		.name = "ipaq-micro-ts",
> +	},
> +	{
> +		.name = "ipaq-micro-leds",
> +	},
> +};

Is Device Tree ever going to be supported on the iPAC? If not, I think
it's okay for you to make these single line entries.

> +static int micro_suspend(struct device *dev)
> +{
> +	return 0;
> +}

Doesn't the PM framework check for NULL .suspend?

> +static int micro_probe(struct platform_device *pdev)
> +{
> +	struct ipaq_micro *micro;
> +	struct resource *res;
> +	int ret;
> +	int irq;
> +
> +	micro = devm_kzalloc(&pdev->dev, sizeof(*micro), GFP_KERNEL);
> +	if (!micro)
> +		return -ENOMEM;
> +	micro->dev = &pdev->dev;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EINVAL;
> +	micro->base = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!micro->base)
> +		return -ENOMEM;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res)
> +		return -EINVAL;
> +	micro->sdlc = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!micro->sdlc)
> +		return -ENOMEM;
> +	micro_reset_comm(micro);
> +	irq = platform_get_irq(pdev, 0);
> +	if (!irq)
> +		return -EINVAL;

Can you space all of these out a bit?

> +	ret = devm_request_irq(&pdev->dev, irq, micro_serial_isr,
> +			       IRQF_SHARED, "ipaq-micro",
> +			       micro);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: unable to grab serial port IRQ\n",
> +			__func__);

Any need to print the function name if you know the name of the
device?

> +		return ret;
> +	} else
> +		dev_info(&pdev->dev, "grabbed serial port IRQ\n");
> +
> +

Too many new lines.

> +	spin_lock_init(&micro->lock);
> +	INIT_LIST_HEAD(&micro->queue);
> +	platform_set_drvdata(pdev, micro);
> +
> +	ret = mfd_add_devices(&pdev->dev, pdev->id, micro_cells,
> +			      ARRAY_SIZE(micro_cells), NULL, 0, NULL);
> +	if (ret) {
> +		dev_err(&pdev->dev, "error adding MFD cells");
> +		return ret;
> +	}

New line here.

> +	/* Check version */
> +	ipaq_micro_get_version(micro);
> +	dev_info(&pdev->dev, "Atmel micro ASIC version %s\n", micro->version);
> +	ipaq_micro_eeprom_dump(micro);
> +
> +	return 0;
> +}
> +
> +static int micro_remove(struct platform_device *pdev)
> +{
> +	struct ipaq_micro *micro = platform_get_drvdata(pdev);
> +	u32 val;
> +
> +	mfd_remove_devices(&pdev->dev);
> +	val = readl(micro->base + UTCR3);
> +	val &= ~(UTCR3_RXE | UTCR3_RIE); /* disable receive interrupt */
> +	val &= ~(UTCR3_TXE | UTCR3_TIE); /* disable transmit interrupt */
> +	writel(val, micro->base + UTCR3);
> +	return 0;

Break these up please.

> +}
> +
> +static const struct dev_pm_ops micro_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(micro_suspend, micro_resume)
> +};
> +
> +static struct platform_driver micro_device_driver = {
> +	.driver   = {
> +		.name	= "ipaq-h3xxx-micro",
> +		.pm	= &micro_dev_pm_ops,
> +	},
> +	.probe    = micro_probe,
> +	.remove   = micro_remove,
> +	/* .shutdown = micro_suspend, // FIXME */
> +};
> +module_platform_driver(micro_device_driver);

It may seem silly for these piece of h/w, but don't we have a 'no new
device drivers without DT support rule'?

> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("driver for iPAQ Atmel micro core and backlight");
> diff --git a/include/linux/mfd/ipaq-micro.h b/include/linux/mfd/ipaq-micro.h
> new file mode 100644
> index 000000000000..9466e97ff236
> --- /dev/null
> +++ b/include/linux/mfd/ipaq-micro.h
> @@ -0,0 +1,149 @@
> +/*
> + * Header file for the compaq Micro MFD
> + */
> +
> +#ifndef _MFD_IPAQ_MICRO_H_
> +#define _MFD_IPAQ_MICRO_H_
> +
> +#include <linux/spinlock.h>
> +#include <linux/completion.h>
> +#include <linux/list.h>
> +struct device;

What's this for?

<snip>

> +static inline int
> +ipaq_micro_tx_msg_sync(struct ipaq_micro *micro,
> +		       struct ipaq_micro_msg *msg)
> +{
> +	int ret;
> +
> +	init_completion(&msg->ack);
> +	ret = ipaq_micro_tx_msg(micro, msg);
> +	wait_for_completion(&msg->ack);
> +
> +	return ret;
> +}
> +
> +static inline int
> +ipaq_micro_tx_msg_async(struct ipaq_micro *micro,
> +			struct ipaq_micro_msg *msg)
> +{
> +	init_completion(&msg->ack);
> +	return ipaq_micro_tx_msg(micro, msg);
> +}

Why are these in here? Where else are they called from?

On Mon, Feb 17, 2014 at 10:21 AM, Lee Jones <lee.jones@linaro.org> wrote:

>> +#include <linux/module.h>
>
> Does it matter that you're using:
>
>   module_platform_driver();
>
> ... yet you can't build this as a module?

I think it's a conceptual thing. Module means two things (IIUC):
(a) it is a run-time loadable module
(b) it's a piece of compartmentalized code

In this case the macro refers to (b). module_platform_driver()
resolves to module_driver() from <linux/device.h>
and it's clearly a core part of the device core and available
no matter whether the file is used as module at runtime or not.
It just simplifies the initcalls and that's no different depending
on whether the code is compiled-in or used as module.

>> +     case MSG_KEYBOARD:
>> +             if (micro->key != NULL)
>
> !micro->key

I guess you mean if (micro->key) {} rather than the !invert,
but I get it, changed everywhere we check for != NULL like this.

>> +                     micro->key(micro->key_data, len, data);
>> +             else
>> +                     dev_dbg(micro->dev, "ipaq micro : key message ignored, "
>> +                             "no handle \n");
>
> Log print spread over two lines. Doesn't Checkpatch complain about
> this?

Rather the other way around but you're right, it's better if
we keep it on one line so changed messages to a single oneliner
everywhere.

>> +static void micro_tx_chars(struct ipaq_micro *micro)
>> +{
>> +     struct ipaq_micro_txdev *tx = &micro->tx;
>> +     u32 val;
>> +
>> +     while ((tx->index < tx->len) &&
>> +            (readl(micro->base + UTSR1) & UTSR1_TNF)) {
>> +             writel(tx->buf[tx->index], micro->base + UTDR);
>
> This is pretty messy. Better broken out? Your call.

The best I can think of is converting to a for-loop but that
is just as hard to read I think :-/

Breaking out the readl() invitably result in two lines of code
like reading the flag before entering the while and once inside
the loop which is nastier IMO.

>> +static struct mfd_cell micro_cells[] = {
>> +     {
>> +             .name = "ipaq-micro-backlight",
>> +     },
>> +     {
>> +             .name = "ipaq-micro-battery",
>> +     },
>> +     {
>> +             .name = "ipaq-micro-keys",
>> +     },
>> +     {
>> +             .name = "ipaq-micro-ts",
>> +     },
>> +     {
>> +             .name = "ipaq-micro-leds",
>> +     },
>> +};
>
> Is Device Tree ever going to be supported on the iPAC?

No clue.

> If not, I think
> it's okay for you to make these single line entries.

OK doing that for now.

>> +static int micro_suspend(struct device *dev)
>> +{
>> +     return 0;
>> +}
>
> Doesn't the PM framework check for NULL .suspend?

No it's just like:

#ifdef CONFIG_SUSPEND
        case PM_EVENT_SUSPEND:
                return ops->suspend;
        case PM_EVENT_RESUME:
                return ops->resume;
#endif /* CONFIG_SUSPEND */

(drivers/power/main.c)

>> +module_platform_driver(micro_device_driver);
>
> It may seem silly for these piece of h/w, but don't we have a 'no new
> device drivers without DT support rule'?

It's more like a "no new sub-architectures without DT support".

The SA1100 systems (such as this H3600 iPAQ) are in a "legacy"
category of architectures, that will probably never get converted
over to device tree. But they are compartmentalized so they're OK.

>> +static inline int
>> +ipaq_micro_tx_msg_sync(struct ipaq_micro *micro,
>> +                    struct ipaq_micro_msg *msg)
>> +{
>> +     int ret;
>> +
>> +     init_completion(&msg->ack);
>> +     ret = ipaq_micro_tx_msg(micro, msg);
>> +     wait_for_completion(&msg->ack);
>> +
>> +     return ret;
>> +}
>> +
>> +static inline int
>> +ipaq_micro_tx_msg_async(struct ipaq_micro *micro,
>> +                     struct ipaq_micro_msg *msg)
>> +{
>> +     init_completion(&msg->ack);
>> +     return ipaq_micro_tx_msg(micro, msg);
>> +}
>
> Why are these in here? Where else are they called from?

This MFD driver provided infrastructure for all the subdrivers, LEDs,
keys etc.

These may want to send messages through the core and wait for
the message to come back, or they may want to send off a
message and await its completion at another time. Since the
most common is to send the message and then immediately
wait for it to complete, but sometimes not, these inline helpers
are provided.

Example use in the battery driver:

static void micro_battery_work(struct work_struct *work)
{
(...)
        struct ipaq_micro_msg msg_battery = {
                .id = MSG_BATTERY,
        };
(...)

        /* First send battery message */
        ipaq_micro_tx_msg_sync(mb->micro, &msg_battery);
        if (msg_battery.rx_len < 4)
                pr_info("ERROR");

        /*
         * Returned message format:
         * byte 0:   0x00 = Not plugged in
         *           0x01 = AC adapter plugged in
         * byte 1:   chemistry
         * byte 2:   voltage LSB
         * byte 3:   voltage MSB
         * byte 4:   flags
         * byte 5-9: same for battery 2
         */
        mb->ac = msg_battery.rx_data[0];
(...)

All other comments are fixed up.

I'm sending out a v2 targeted for v3.16.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> > Does it matter that you're using:
> >
> >   module_platform_driver();
> >
> > ... yet you can't build this as a module?
> 
> I think it's a conceptual thing. Module means two things (IIUC):
> (a) it is a run-time loadable module
> (b) it's a piece of compartmentalized code
> 
> In this case the macro refers to (b). module_platform_driver()
> resolves to module_driver() from <linux/device.h>
> and it's clearly a core part of the device core and available
> no matter whether the file is used as module at runtime or not.
> It just simplifies the initcalls and that's no different depending
> on whether the code is compiled-in or used as module.

Fair enough.

> >> +     case MSG_KEYBOARD:
> >> +             if (micro->key != NULL)
> >
> > !micro->key
> 
> I guess you mean if (micro->key) {} rather than the !invert,
> but I get it, changed everywhere we check for != NULL like this.

Yes sorry, I'm so used to seeing (thing == NULL), but you knew what I
meant.

[...]

> >> +static int micro_suspend(struct device *dev)
> >> +{
> >> +     return 0;
> >> +}
> >
> > Doesn't the PM framework check for NULL .suspend?
> 
> No it's just like:
> 
> #ifdef CONFIG_SUSPEND
>         case PM_EVENT_SUSPEND:
>                 return ops->suspend;
>         case PM_EVENT_RESUME:
>                 return ops->resume;
> #endif /* CONFIG_SUSPEND */
> 
> (drivers/power/main.c)

  base/ ---^

Right, but a NULL return _is_ valid from here and is subsequently dealt
with in the correct way.

[...]
        callback = pm_op(dev->type->pm, state);
        goto Driver;

[...]

Driver:
       if (!callback && dev->driver && dev->driver->pm) {
               info = "driver ";
               callback = pm_op(dev->driver->pm, state);
       }
[...]