[RFC,04/10] mfd: Add base driver for Netronix embedded controller

Third-party hardware documentation is available at
https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller

The EC supports interrupts, but the driver doesn't make use of them so
far.

Known problems:
- The reboot handler is installed in such a way that it directly calls
  into the i2c subsystem to send the reboot command to the EC. This
  means that the reboot handler may sleep, which is not allowed.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 drivers/mfd/Kconfig       |   7 ++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/ntxec.c       | 188 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/ntxec.h |  30 ++++++
 4 files changed, 226 insertions(+)
 create mode 100644 drivers/mfd/ntxec.c
 create mode 100644 include/linux/mfd/ntxec.h

--
2.27.0

On Sun, 21 Jun 2020, Jonathan Neuschäfer wrote:

Description of the device here please.

> Third-party hardware documentation is available at

'\n'

> https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller

Indent this with a couple of spaces.

> The EC supports interrupts, but the driver doesn't make use of them so
> far.
> 
> Known problems:
> - The reboot handler is installed in such a way that it directly calls
>   into the i2c subsystem to send the reboot command to the EC. This
>   means that the reboot handler may sleep, which is not allowed.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  drivers/mfd/Kconfig       |   7 ++
>  drivers/mfd/Makefile      |   1 +
>  drivers/mfd/ntxec.c       | 188 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/ntxec.h |  30 ++++++
>  4 files changed, 226 insertions(+)
>  create mode 100644 drivers/mfd/ntxec.c
>  create mode 100644 include/linux/mfd/ntxec.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index a37d7d1713820..78410b928648e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -978,6 +978,13 @@ config MFD_VIPERBOARD
>  	  You need to select the mfd cell drivers separately.
>  	  The drivers do not support all features the board exposes.
> 
> +config MFD_NTXEC
> +	bool "Netronix Embedded Controller"
> +	depends on I2C && OF
> +	help
> +	  Say yes here if you want to support the embedded controller of
> +	  certain e-book readers designed by the ODM Netronix.

s/of certain/found in certain/.

>  config MFD_RETU
>  	tristate "Nokia Retu and Tahvo multi-function device"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 9367a92f795a6..19d9391ed6f32 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -218,6 +218,7 @@ obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
>  obj-$(CONFIG_MFD_INTEL_PMC_BXT)	+= intel_pmc_bxt.o
>  obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
>  obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
> +obj-$(CONFIG_MFD_NTXEC)		+= ntxec.o
>  obj-$(CONFIG_MFD_RC5T583)	+= rc5t583.o rc5t583-irq.o
>  obj-$(CONFIG_MFD_RK808)		+= rk808.o
>  obj-$(CONFIG_MFD_RN5T618)	+= rn5t618.o
> diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c
> new file mode 100644
> index 0000000000000..82adea34ea746
> --- /dev/null
> +++ b/drivers/mfd/ntxec.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright 2020 Jonathan Neuschäfer
> +//
> +// MFD driver for the usually MSP430-based embedded controller used in certain
> +// Netronix ebook reader board designs

Only the SPDX line should contain C++ style comments.

Description at the top, copyright after.

An "MFD driver" isn't really a thing.  Please describe the device.

> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/ntxec.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm.h>
> +#include <linux/reboot.h>
> +#include <linux/types.h>
> +
> +

No need for double line spacing.

> +#define NTXEC_VERSION		0x00
> +#define NTXEC_POWEROFF		0x50
> +#define NTXEC_POWERKEEP		0x70
> +#define NTXEC_RESET		0x90

Are these registers?  Might be worth pre/post-fixing with _REG.

> +/* Register access */
> +
> +int ntxec_read16(struct ntxec *ec, u8 addr)
> +{
> +	u8 request[1] = { addr };
> +	u8 response[2];
> +	int res;
> +
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = ec->client->addr,
> +			.flags = ec->client->flags,
> +			.len = sizeof(request),
> +			.buf = request
> +		}, {
> +			.addr = ec->client->addr,
> +			.flags = ec->client->flags | I2C_M_RD,
> +			.len = sizeof(response),
> +			.buf = response
> +		}
> +	};
> +
> +	res = i2c_transfer(ec->client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (res < 0)
> +		return res;
> +	if (res != ARRAY_SIZE(msgs))
> +		return -EIO;
> +
> +	return get_unaligned_be16(response);
> +}
> +EXPORT_SYMBOL(ntxec_read16);
> +
> +int ntxec_write16(struct ntxec *ec, u8 addr, u16 value)
> +{
> +	u8 request[3] = { addr, };
> +	int res;
> +
> +	put_unaligned_be16(value, request + 1);
> +
> +	res = i2c_transfer_buffer_flags(ec->client, request, sizeof(request),
> +					ec->client->flags);
> +	if (res < 0)
> +		return res;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ntxec_write16);
> +
> +int ntxec_read8(struct ntxec *ec, u8 addr)
> +{
> +	int res = ntxec_read16(ec, addr);
> +
> +	if (res < 0)
> +		return res;
> +
> +	return (res >> 8) & 0xff;
> +}
> +EXPORT_SYMBOL(ntxec_read8);
> +
> +int ntxec_write8(struct ntxec *ec, u8 addr, u8 value)
> +{
> +	return ntxec_write16(ec, addr, value << 8);
> +}
> +EXPORT_SYMBOL(ntxec_write8);

Why are you hand-rolling your own accessors?

> +
> +
> +/* Reboot/poweroff handling */

These comments seem to be stating the obvious.

Please remove them.

> +static struct ntxec *poweroff_restart_instance;
> +
> +static void ntxec_poweroff(void)
> +{
> +	ntxec_write8(poweroff_restart_instance, NTXEC_POWEROFF, 0x01);

All magic numbers should be defined?

> +	msleep(5000);

Why 5000?

> +}
> +
> +static int ntxec_restart(struct notifier_block *nb,
> +		unsigned long action, void *data)
> +{
> +	/* FIXME: The I2C driver sleeps, but restart handlers may not sleep */

Then this is not allowed.

You need to fix this *before* submitting upstream.

> +	ntxec_write8(poweroff_restart_instance, NTXEC_RESET, 0xff);
> +	/* TODO: delay? */

TODO *before* submitting upstream.  This is not a development branch.

> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block ntxec_restart_handler = {
> +	.notifier_call = ntxec_restart,
> +	.priority = 128
> +};
> +
> +
> +/* Driver setup */
> +
> +static int ntxec_probe(struct i2c_client *client,
> +			    const struct i2c_device_id *ids)
> +{
> +	struct ntxec *ec;
> +	int res;

What does 'res' mean?

> +	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> +	if (!ec)
> +		return -ENOMEM;
> +
> +	ec->dev = &client->dev;
> +	ec->client = client;

You don't need both (if any).

> +	/* Determine the firmware version */
> +	res = ntxec_read16(ec, NTXEC_VERSION);
> +	if (res < 0) {
> +		dev_dbg(ec->dev, "Failed to read firmware version number\n");

Why dbg?

> +		return res;
> +	}
> +	ec->version = res;
> +
> +	dev_info(ec->dev,
> +		 "Netronix embedded controller version %04x detected.\n",
> +		 ec->version);
> +
> +	/* For now, we don't support the new register layout. */
> +	if (ntxec_has_new_layout(ec))
> +		return -EOPNOTSUPP;

What is the new layout?

Why don't you support it?

> +	if (of_device_is_system_power_controller(ec->dev->of_node)) {
> +		/*
> +		 * Set the 'powerkeep' bit. This is necessary on some boards
> +		 * in order to keep the system running.
> +		 */
> +		res = ntxec_write8(ec, NTXEC_POWERKEEP, 0x08);
> +		if (res < 0)
> +			return res;
> +
> +		/* Install poweroff handler */
> +		WARN_ON(poweroff_restart_instance);

Why WARN?

> +		poweroff_restart_instance = ec;
> +		if (pm_power_off != NULL)

if (pm_power_off)

> +			/* TODO: Refactor among all poweroff drivers */

Nope.

> +			dev_err(ec->dev, "pm_power_off already assigned\n");

Error, but execution carries on anyway?

> +		else
> +			pm_power_off = ntxec_poweroff;
> +
> +		/* Install board reset handler */
> +		res = register_restart_handler(&ntxec_restart_handler);
> +		if (res < 0)

Is >0 valid?

> +			dev_err(ec->dev,
> +				"Failed to register restart handler: %d\n", res);
> +	}
> +
> +	i2c_set_clientdata(client, ec);

Where do you use this?

> +	return devm_of_platform_populate(ec->dev);
> +}
> +
> +static const struct of_device_id of_ntxec_match_table[] = {
> +	{ .compatible = "netronix,ntxec", },
> +	{}
> +};

Use .probe_new() and drop this.

> +static struct i2c_driver ntxec_driver = {
> +	.driver	= {
> +		.name	= "ntxec",
> +		.of_match_table = of_ntxec_match_table,
> +	},
> +	.probe		= ntxec_probe,

Nothing to .remove()?

> +};
> +builtin_i2c_driver(ntxec_driver);

Only built-in?

> diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h
> new file mode 100644
> index 0000000000000..9f9d6f2141751
> --- /dev/null
> +++ b/include/linux/mfd/ntxec.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright 2020 Jonathan Neuschäfer
> + *
> + * MFD access functions for the Netronix embedded controller.

No such thing as an MFD.

"Embedded Controller"

> + */
> +
> +#ifndef NTXEC_H
> +#define NTXEC_H
> +
> +#include <linux/types.h>
> +
> +struct ntxec {
> +	struct device *dev;
> +	struct i2c_client *client;
> +	u16 version;
> +	/* TODO: Add a mutex to protect actions consisting of multiple accesses? */

No TODOs.  Please fix before upstreaming.

> +};
> +
> +static inline bool ntxec_has_new_layout(struct ntxec *ec)
> +{
> +	return ec->version == 0xe916;

Why don't you just check for the devices you *do* support?

> +}
> +
> +int ntxec_read16(struct ntxec *ec, u8 addr);
> +int ntxec_write16(struct ntxec *ec, u8 addr, u16 value);
> +int ntxec_read8(struct ntxec *ec, u8 addr);
> +int ntxec_write8(struct ntxec *ec, u8 addr, u8 value);
> +
> +#endif

#endif /* NTXEC_H */

On Sun, 21 Jun 2020 00:42:15 +0200
Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:

> Third-party hardware documentation is available at
> https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller
> 
> The EC supports interrupts, but the driver doesn't make use of them so
> far.
> 
> Known problems:
> - The reboot handler is installed in such a way that it directly calls
>   into the i2c subsystem to send the reboot command to the EC. This
>   means that the reboot handler may sleep, which is not allowed.
> 
see
https://patchwork.ozlabs.org/project/linux-i2c/patch/20190415213432.8972-3-contact@stefanchrist.eu/

for a fix of such problems. 

> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  drivers/mfd/Kconfig       |   7 ++
>  drivers/mfd/Makefile      |   1 +
>  drivers/mfd/ntxec.c       | 188 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/ntxec.h |  30 ++++++
>  4 files changed, 226 insertions(+)
>  create mode 100644 drivers/mfd/ntxec.c
>  create mode 100644 include/linux/mfd/ntxec.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index a37d7d1713820..78410b928648e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -978,6 +978,13 @@ config MFD_VIPERBOARD
>  	  You need to select the mfd cell drivers separately.
>  	  The drivers do not support all features the board exposes.
> 
> +config MFD_NTXEC
> +	bool "Netronix Embedded Controller"
> +	depends on I2C && OF
> +	help
> +	  Say yes here if you want to support the embedded controller of
> +	  certain e-book readers designed by the ODM Netronix.
> +
>  config MFD_RETU
>  	tristate "Nokia Retu and Tahvo multi-function device"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 9367a92f795a6..19d9391ed6f32 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -218,6 +218,7 @@ obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
>  obj-$(CONFIG_MFD_INTEL_PMC_BXT)	+= intel_pmc_bxt.o
>  obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
>  obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
> +obj-$(CONFIG_MFD_NTXEC)		+= ntxec.o
>  obj-$(CONFIG_MFD_RC5T583)	+= rc5t583.o rc5t583-irq.o
>  obj-$(CONFIG_MFD_RK808)		+= rk808.o
>  obj-$(CONFIG_MFD_RN5T618)	+= rn5t618.o
> diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c
> new file mode 100644
> index 0000000000000..82adea34ea746
> --- /dev/null
> +++ b/drivers/mfd/ntxec.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright 2020 Jonathan Neuschäfer
> +//
> +// MFD driver for the usually MSP430-based embedded controller used in certain
> +// Netronix ebook reader board designs
> +
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/ntxec.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm.h>
> +#include <linux/reboot.h>
> +#include <linux/types.h>
> +
> +
> +#define NTXEC_VERSION		0x00
> +#define NTXEC_POWEROFF		0x50
> +#define NTXEC_POWERKEEP		0x70
> +#define NTXEC_RESET		0x90
> +
> +
> +/* Register access */
> +
> +int ntxec_read16(struct ntxec *ec, u8 addr)
> +{
> +	u8 request[1] = { addr };
> +	u8 response[2];
> +	int res;
> +
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = ec->client->addr,
> +			.flags = ec->client->flags,
> +			.len = sizeof(request),
> +			.buf = request
> +		}, {
> +			.addr = ec->client->addr,
> +			.flags = ec->client->flags | I2C_M_RD,
> +			.len = sizeof(response),
> +			.buf = response
> +		}
> +	};
> +
> +	res = i2c_transfer(ec->client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (res < 0)
> +		return res;
> +	if (res != ARRAY_SIZE(msgs))
> +		return -EIO;
> +
> +	return get_unaligned_be16(response);
> +}
> +EXPORT_SYMBOL(ntxec_read16);
> +
> +int ntxec_write16(struct ntxec *ec, u8 addr, u16 value)
> +{
> +	u8 request[3] = { addr, };
> +	int res;
> +
> +	put_unaligned_be16(value, request + 1);
> +
> +	res = i2c_transfer_buffer_flags(ec->client, request, sizeof(request),
> +					ec->client->flags);
> +	if (res < 0)
> +		return res;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ntxec_write16);
> +
> +int ntxec_read8(struct ntxec *ec, u8 addr)
> +{
> +	int res = ntxec_read16(ec, addr);
> +
> +	if (res < 0)
> +		return res;
> +
> +	return (res >> 8) & 0xff;
> +}
> +EXPORT_SYMBOL(ntxec_read8);
> +
> +int ntxec_write8(struct ntxec *ec, u8 addr, u8 value)
> +{
> +	return ntxec_write16(ec, addr, value << 8);
> +}
> +EXPORT_SYMBOL(ntxec_write8);
> +

do we really need both 16bit and 8bit accessors? If not,
then simply use regmap_i2c_init and set val_bits accordingly.
Maybe just doing the << 8 in the constants?

> +
> +/* Reboot/poweroff handling */
> +
> +static struct ntxec *poweroff_restart_instance;
> +
> +static void ntxec_poweroff(void)
> +{
> +	ntxec_write8(poweroff_restart_instance, NTXEC_POWEROFF, 0x01);
> +	msleep(5000);
> +}
> +
> +static int ntxec_restart(struct notifier_block *nb,
> +		unsigned long action, void *data)
> +{
> +	/* FIXME: The I2C driver sleeps, but restart handlers may not sleep */
> +	ntxec_write8(poweroff_restart_instance, NTXEC_RESET, 0xff);
> +	/* TODO: delay? */
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block ntxec_restart_handler = {
> +	.notifier_call = ntxec_restart,
> +	.priority = 128
> +};
> +
> +
> +/* Driver setup */
> +
> +static int ntxec_probe(struct i2c_client *client,
> +			    const struct i2c_device_id *ids)
> +{
> +	struct ntxec *ec;
> +	int res;
> +
> +	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> +	if (!ec)
> +		return -ENOMEM;
> +
> +	ec->dev = &client->dev;
> +	ec->client = client;
> +
> +	/* Determine the firmware version */
> +	res = ntxec_read16(ec, NTXEC_VERSION);
> +	if (res < 0) {
> +		dev_dbg(ec->dev, "Failed to read firmware version number\n");
> +		return res;
> +	}
> +	ec->version = res;
> +
> +	dev_info(ec->dev,
> +		 "Netronix embedded controller version %04x detected.\n",
> +		 ec->version);
> +
> +	/* For now, we don't support the new register layout. */
> +	if (ntxec_has_new_layout(ec))
> +		return -EOPNOTSUPP;
> +
> +	if (of_device_is_system_power_controller(ec->dev->of_node)) {
> +		/*
> +		 * Set the 'powerkeep' bit. This is necessary on some boards
> +		 * in order to keep the system running.
> +		 */
> +		res = ntxec_write8(ec, NTXEC_POWERKEEP, 0x08);
> +		if (res < 0)
> +			return res;
> +
> +		/* Install poweroff handler */
> +		WARN_ON(poweroff_restart_instance);
> +		poweroff_restart_instance = ec;
> +		if (pm_power_off != NULL)
> +			/* TODO: Refactor among all poweroff drivers */
> +			dev_err(ec->dev, "pm_power_off already assigned\n");
> +		else
> +			pm_power_off = ntxec_poweroff;
> +
common pattern, across drivers, so I think doing something else would
be a separate cleanup issue.

Regards,
Andreas

On Mon, Jun 22, 2020 at 11:55:44AM +0100, Lee Jones wrote:
> On Sun, 21 Jun 2020, Jonathan Neuschäfer wrote:
> 
> Description of the device here please.
> 
> > Third-party hardware documentation is available at
> 
> '\n'
> 
> > https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller
> 
> Indent this with a couple of spaces.
> 
[...]
> > +	help
> > +	  Say yes here if you want to support the embedded controller of
> > +	  certain e-book readers designed by the ODM Netronix.
> 
> s/of certain/found in certain/.
[...]
> > +++ b/drivers/mfd/ntxec.c
> > @@ -0,0 +1,188 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +// Copyright 2020 Jonathan Neuschäfer
> > +//
> > +// MFD driver for the usually MSP430-based embedded controller used in certain
> > +// Netronix ebook reader board designs
> 
> Only the SPDX line should contain C++ style comments.
> 
> Description at the top, copyright after.
> 
> An "MFD driver" isn't really a thing.  Please describe the device.
[...]
> > +#include <linux/types.h>
> > +
> > +
> 
> No need for double line spacing.

Alright, I'll fix these.

> > +#define NTXEC_VERSION		0x00
> > +#define NTXEC_POWEROFF		0x50
> > +#define NTXEC_POWERKEEP		0x70
> > +#define NTXEC_RESET		0x90
> 
> Are these registers?  Might be worth pre/post-fixing with _REG.

Yes, good idea.

> 
> > +/* Register access */
> > +
> > +int ntxec_read16(struct ntxec *ec, u8 addr)
> > +{
> > +	u8 request[1] = { addr };
> > +	u8 response[2];
> > +	int res;
> > +
> > +	struct i2c_msg msgs[] = {
> > +		{
> > +			.addr = ec->client->addr,
> > +			.flags = ec->client->flags,
> > +			.len = sizeof(request),
> > +			.buf = request
> > +		}, {
> > +			.addr = ec->client->addr,
> > +			.flags = ec->client->flags | I2C_M_RD,
> > +			.len = sizeof(response),
> > +			.buf = response
> > +		}
> > +	};
> > +
> > +	res = i2c_transfer(ec->client->adapter, msgs, ARRAY_SIZE(msgs));
> > +	if (res < 0)
> > +		return res;
> > +	if (res != ARRAY_SIZE(msgs))
> > +		return -EIO;
> > +
> > +	return get_unaligned_be16(response);
> > +}
> > +EXPORT_SYMBOL(ntxec_read16);
> > +
> > +int ntxec_write16(struct ntxec *ec, u8 addr, u16 value)
> > +{
> > +	u8 request[3] = { addr, };
> > +	int res;
> > +
> > +	put_unaligned_be16(value, request + 1);
> > +
> > +	res = i2c_transfer_buffer_flags(ec->client, request, sizeof(request),
> > +					ec->client->flags);
> > +	if (res < 0)
> > +		return res;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(ntxec_write16);
> > +
> > +int ntxec_read8(struct ntxec *ec, u8 addr)
> > +{
> > +	int res = ntxec_read16(ec, addr);
> > +
> > +	if (res < 0)
> > +		return res;
> > +
> > +	return (res >> 8) & 0xff;
> > +}
> > +EXPORT_SYMBOL(ntxec_read8);
> > +
> > +int ntxec_write8(struct ntxec *ec, u8 addr, u8 value)
> > +{
> > +	return ntxec_write16(ec, addr, value << 8);
> > +}
> > +EXPORT_SYMBOL(ntxec_write8);
> 
> Why are you hand-rolling your own accessors?

There are registers which only require one byte of data and others which
require two. This didn't quite seem to fit into the regmap API.

It is possible to always use 16-bit accessors directly but that would
push shifts into call sites that only use 8 bits.
(If the 16 bits are interpreted as big endian)

I'm not sure what's ideal here.


> > +/* Reboot/poweroff handling */
> 
> These comments seem to be stating the obvious.
> 
> Please remove them.

Ok.


> > +	ntxec_write8(poweroff_restart_instance, NTXEC_POWEROFF, 0x01);
> 
> All magic numbers should be defined?

Alright, I'll move them to the register definitions.

> > +	msleep(5000);
> 
> Why 5000?

The idea was to avoid returning from the poweroff handler by allowing
enough time until the power is actually off. However:

 - I just tested it and the board I have turns off about 80ms after the
   I2C command.

 - I'm not sure if returning from the poweroff handler is actually bad.
   It does seem to be wrong, because I get a lockdep report when I
   remove the msleep and the kernel reaches do_exit in the reboot
   syscall handler:

	Requesting system poweroff
	[   14.465312] cfg80211: failed to load regulatory.db
	[   14.471171] imx-sdma 63fb0000.sdma: external firmware not found, using ROM firmware
	[   14.541097] reboot: Power down
	[   14.547296]
	[   14.548840] ====================================
	[   14.553477] WARNING: init/156 still has locks held!
	[   14.558521] 5.8.0-rc1-00011-g65740c2d29bee-dirty #329 Not tainted
	[   14.564647] ------------------------------------
	[   14.569399] 1 lock held by init/156:
	[   14.573004]  #0: c17278a8 (system_transition_mutex){+.+.}-{3:3}, at: __do_sys_reboot+0x13c/0x1fc
	[   14.581978]
	[   14.581978] stack backtrace:
	[   14.586378] CPU: 0 PID: 156 Comm: init Not tainted 5.8.0-rc1-00011-g65740c2d29bee-dirty #329
	[   14.594834] Hardware name: Freescale i.MX50 (Device Tree Support)
	[   14.600979] [<c01121c8>] (unwind_backtrace) from [<c010cf3c>] (show_stack+0x10/0x14)
	[   14.608763] [<c010cf3c>] (show_stack) from [<c05e54ec>] (dump_stack+0xe4/0x118)
	[   14.616115] [<c05e54ec>] (dump_stack) from [<c0130d54>] (do_exit+0x6ec/0xc04)
	[   14.623291] [<c0130d54>] (do_exit) from [<c01582f0>] (__do_sys_reboot+0x148/0x1fc)
	[   14.630892] [<c01582f0>] (__do_sys_reboot) from [<c0100080>] (ret_fast_syscall+0x0/0x28)
	[   14.639000] Exception stack(0xc8c6dfa8 to 0xc8c6dff0)
	[   14.644071] dfa0:                   00000000 00000000 fee1dead 28121969 4321fedc 00000000
	[   14.652266] dfc0: 00000000 00000000 00000001 00000058 00000001 00000000 00000000 00000000
	[   14.660458] dfe0: 000a2290 bef21db4 0006db1f b6f02a0c


I'll adjust the number a bit and add a comment to explain the msleep
call.


> > +static int ntxec_restart(struct notifier_block *nb,
> > +		unsigned long action, void *data)
> > +{
> > +	/* FIXME: The I2C driver sleeps, but restart handlers may not sleep */
> 
> Then this is not allowed.
> 
> You need to fix this *before* submitting upstream.
> 
> > +	ntxec_write8(poweroff_restart_instance, NTXEC_RESET, 0xff);
> > +	/* TODO: delay? */
> 
> TODO *before* submitting upstream.  This is not a development branch.

Sorry. I'll research how other drivers deal with the issue of sleeping
I/O in the reset path.


> > +/* Driver setup */
> > +
> > +static int ntxec_probe(struct i2c_client *client,
> > +			    const struct i2c_device_id *ids)
> > +{
> > +	struct ntxec *ec;
> > +	int res;
> 
> What does 'res' mean?

Result

> > +	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> > +	if (!ec)
> > +		return -ENOMEM;
> > +
> > +	ec->dev = &client->dev;
> > +	ec->client = client;
> 
> You don't need both (if any).

Ok, I'll drop ec->client.

> 
> > +	/* Determine the firmware version */
> > +	res = ntxec_read16(ec, NTXEC_VERSION);
> > +	if (res < 0) {
> > +		dev_dbg(ec->dev, "Failed to read firmware version number\n");
> 
> Why dbg?

I'm not sure anymore, but I agree that it seems more significant than
debug level.


> > +	/* For now, we don't support the new register layout. */
> > +	if (ntxec_has_new_layout(ec))
> > +		return -EOPNOTSUPP;
> 
> What is the new layout?

It's a significantly different command set, see 'if (NEWMSP) ...' in:

  https://github.com/neuschaefer/linux/blob/vendor/kobo-aura/drivers/mxc/pmic/core/pmic_core_i2c.c


> Why don't you support it?

I don't have any hardware that uses it. The downstream driver supports
both layouts so I wanted to ensure that this driver doesn't use the
wrong one.

> > +	if (of_device_is_system_power_controller(ec->dev->of_node)) {
> > +		/*
> > +		 * Set the 'powerkeep' bit. This is necessary on some boards
> > +		 * in order to keep the system running.
> > +		 */
> > +		res = ntxec_write8(ec, NTXEC_POWERKEEP, 0x08);
> > +		if (res < 0)
> > +			return res;
> > +
> > +		/* Install poweroff handler */
> > +		WARN_ON(poweroff_restart_instance);
> 
> Why WARN?

Two instances of this driver where both try to be the poweroff/restart
instance seemed like a situation that is always unintended and a bug in
the devicetree (or somewhere else).

> 
> > +		poweroff_restart_instance = ec;
> > +		if (pm_power_off != NULL)
> 
> if (pm_power_off)

Ok

> 
> > +			/* TODO: Refactor among all poweroff drivers */
> 
> Nope.

Alright.

> 
> > +			dev_err(ec->dev, "pm_power_off already assigned\n");
> 
> Error, but execution carries on anyway?

Hmm, I guess if the poweroff handler can't be installed there also isn't
much point in installing a restart handler...

> > +		/* Install board reset handler */
> > +		res = register_restart_handler(&ntxec_restart_handler);
> > +		if (res < 0)
> 
> Is >0 valid?

"Currently always returns zero"

However, other callers tend to do "if (res)".

> > +			dev_err(ec->dev,
> > +				"Failed to register restart handler: %d\n", res);
> > +	}
> > +
> > +	i2c_set_clientdata(client, ec);
> 
> Where do you use this?

The sub-devices drivers (RTC, PWM) call `dev_get_drvdata(pdev->dev.parent)`
to get a pointer to the ntxec struct, which is then passed to the
register accessors.

> > +static const struct of_device_id of_ntxec_match_table[] = {
> > +	{ .compatible = "netronix,ntxec", },
> > +	{}
> > +};
> 
> Use .probe_new() and drop this.

Ok, I'll switch to .probe_new()

Should I really drop the OF match table, though?

> > +static struct i2c_driver ntxec_driver = {
> > +	.driver	= {
> > +		.name	= "ntxec",
> > +		.of_match_table = of_ntxec_match_table,
> > +	},
> > +	.probe		= ntxec_probe,
> 
> Nothing to .remove()?

Good point, I'll add a remove method.

> > +};
> > +builtin_i2c_driver(ntxec_driver);
> 
> Only built-in?

Currently, this is consistent with the Kconfig entry, but I'll look into
making the driver buildable as a module.

> > diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h
> > new file mode 100644
> > index 0000000000000..9f9d6f2141751
> > --- /dev/null
> > +++ b/include/linux/mfd/ntxec.h
> > @@ -0,0 +1,30 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright 2020 Jonathan Neuschäfer
> > + *
> > + * MFD access functions for the Netronix embedded controller.
> 
> No such thing as an MFD.
> 
> "Embedded Controller"

Ok, I'll rephrase the comment.

> > +struct ntxec {
> > +	struct device *dev;
> > +	struct i2c_client *client;
> > +	u16 version;
> > +	/* TODO: Add a mutex to protect actions consisting of multiple accesses? */
> 
> No TODOs.  Please fix before upstreaming.

Sorry, I'll clean them up.

> > +static inline bool ntxec_has_new_layout(struct ntxec *ec)
> > +{
> > +	return ec->version == 0xe916;
> 
> Why don't you just check for the devices you *do* support?

Good point. This would be the most conservative approach. If someone
then shows up with a different version, it can be added as tested and
known working.


Thanks for the review,
Jonathan Neuschäfer

On Sat, Jun 27, 2020 at 10:17:38AM +0200, Andreas Kemnade wrote:
> On Sun, 21 Jun 2020 00:42:15 +0200
> Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> 
> > Third-party hardware documentation is available at
> > https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller
> > 
> > The EC supports interrupts, but the driver doesn't make use of them so
> > far.
> > 
> > Known problems:
> > - The reboot handler is installed in such a way that it directly calls
> >   into the i2c subsystem to send the reboot command to the EC. This
> >   means that the reboot handler may sleep, which is not allowed.
> > 
> see
> https://patchwork.ozlabs.org/project/linux-i2c/patch/20190415213432.8972-3-contact@stefanchrist.eu/
> 
> for a fix of such problems. 

So far, regmap isn't involved here, but I'll remember it when I switch
to regmap.

Between when I first wrote this driver and now, the I2C has added
support for transfers in atomic contexts very late in the system's life
(exactly what happens when you reset a system via PMIC/EC), so this
problem seems to be gone from my driver, for now.
(See commit 63b96983a5ddf ("i2c: core: introduce callbacks for atomic transfers"))


[...]
> > +int ntxec_write8(struct ntxec *ec, u8 addr, u8 value)
> > +{
> > +	return ntxec_write16(ec, addr, value << 8);
> > +}
> > +EXPORT_SYMBOL(ntxec_write8);
> > +
> 
> do we really need both 16bit and 8bit accessors?

No, the hardware/firmware doesn't care.

> If not, then simply use regmap_i2c_init and set val_bits accordingly.
> Maybe just doing the << 8 in the constants?

Thanks, I'll try this approach.

The values are not always constants, for example in the PWM driver:

	res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_HIGH, period >> 8);
	res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_LOW, period);
	res |= ntxec_write8(pwm->ec, NTXEC_DUTY_HIGH, duty >> 8);
	res |= ntxec_write8(pwm->ec, NTXEC_DUTY_LOW, duty);


Jonathan