diff mbox series

[RFC,v1,2/2] platform: toradex: add preliminary support for Embedded Controller

Message ID 20250313144331.70591-3-francesco@dolcini.it (mailing list archive)
State New
Headers show
Series platform: toradex: Add toradex embedded controller | expand

Commit Message

Francesco Dolcini March 13, 2025, 2:43 p.m. UTC 
From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

Add new platform driver for the Embedded Controller currently used
in Toradex SMARC iMX8MP and SMARC iMX95.
It currently provides power-off and restart (reset) handlers.

Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 MAINTAINERS                           |   1 +
 drivers/platform/Kconfig              |   2 +
 drivers/platform/Makefile             |   1 +
 drivers/platform/toradex/Kconfig      |  18 +++
 drivers/platform/toradex/Makefile     |   1 +
 drivers/platform/toradex/toradex-ec.c | 155 ++++++++++++++++++++++++++
 6 files changed, 178 insertions(+)
 create mode 100644 drivers/platform/toradex/Kconfig
 create mode 100644 drivers/platform/toradex/Makefile
 create mode 100644 drivers/platform/toradex/toradex-ec.c

Comments

Andy Shevchenko March 13, 2025, 2:53 p.m. UTC | #1 
On Thu, Mar 13, 2025 at 03:43:31PM +0100, Francesco Dolcini wrote:

> Add new platform driver for the Embedded Controller currently used
> in Toradex SMARC iMX8MP and SMARC iMX95.
> It currently provides power-off and restart (reset) handlers.

...

+ array_size.h
+ device.h
+ err.h

> +#include <linux/i2c.h>

+ mod_devicetable,h

> +#include <linux/module.h>
> +#include <linux/reboot.h>
> +#include <linux/regmap.h>

+ types.h

...

> +#define EC_CHIP_ID_REG                  0x00
> +#define EC_VERSION_REG_MAJOR            0x01
> +#define EC_VERSION_REG_MINOR            0x02
> +#define EC_CMD_REG                      0xD0
> +#define EC_REG_MAX                      0xD0
> +
> +#define EC_CHIP_ID_SMARC_IMX95          0x11
> +#define EC_CHIP_ID_SMARC_IMX8MP         0x12
> +
> +#define EC_POWEROFF_CMD                 0x01
> +#define EC_RESET_CMD                    0x02

Can you interleave the register offsets with the values in them, so we can
easily see the relationship?

...

> +struct tdx_ec {

> +	struct i2c_client *client;

Why do you need this? What for?..

> +	struct regmap *regmap;

...note, the device pointer you may retrieve from the regmap.

> +};

...

> +static int tdx_ec_register_power_off_restart(struct device *dev, struct tdx_ec *ec)
> +{
> +	int err;
> +
> +	err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART,
> +					    SYS_OFF_PRIO_FIRMWARE,
> +					    tdx_ec_restart, ec);
> +	if (err)
> +		return err;

> +	err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF,
> +					    SYS_OFF_PRIO_FIRMWARE,
> +					    tdx_ec_power_off, ec);
> +	return err;

	return devm_...

> +}

...

> +static int tdx_ec_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	u8 reg_val[EC_ID_VERSION_LEN];
> +	struct tdx_ec *ec;
> +	int err;
> +
> +	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> +	if (!ec)
> +		return -ENOMEM;
> +
> +	ec->client = client;

> +	i2c_set_clientdata(client, ec);

What for?

> +	ec->regmap = devm_regmap_init_i2c(client, &regmap_config);
> +	if (IS_ERR(ec->regmap))
> +		return PTR_ERR(ec->regmap);
> +
> +	err = regmap_bulk_read(ec->regmap, EC_CHIP_ID_REG, &reg_val, EC_ID_VERSION_LEN);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "Cannot read id and version registers\n");
> +
> +	dev_info(dev, "Toradex Embedded Controller id %x - Firmware %d.%d\n",

Specifiers are semirandom. Why signed? Why x and not %u?

> +		 reg_val[0], reg_val[1], reg_val[2]);

> +	err = tdx_ec_register_power_off_restart(dev, ec);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "Cannot register system restart handler\n");
> +
> +	return 0;
> +}
Francesco Dolcini March 17, 2025, 8:39 a.m. UTC | #2 
Hello Andy,
thanks for the review.

On Thu, Mar 13, 2025 at 04:53:35PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 13, 2025 at 03:43:31PM +0100, Francesco Dolcini wrote:
> 
> > Add new platform driver for the Embedded Controller currently used
> > in Toradex SMARC iMX8MP and SMARC iMX95.
> > It currently provides power-off and restart (reset) handlers.

...

> > +	err = regmap_bulk_read(ec->regmap, EC_CHIP_ID_REG, &reg_val, EC_ID_VERSION_LEN);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "Cannot read id and version registers\n");
> > +
> > +	dev_info(dev, "Toradex Embedded Controller id %x - Firmware %d.%d\n",
> 
> Specifiers are semirandom. Why signed? Why x and not %u?

The firmware version ("Firmware %d.%d") is two unsigned decimal number,
so yes, I will change to "Firmware %u.%u".

The ID is just an identifier that is documented as hex, therefore I
think that the most convenient way to display it is as a hex number.

Francesco
Andy Shevchenko March 17, 2025, 9:31 a.m. UTC | #3 
On Mon, Mar 17, 2025 at 09:39:11AM +0100, Francesco Dolcini wrote:
> On Thu, Mar 13, 2025 at 04:53:35PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 13, 2025 at 03:43:31PM +0100, Francesco Dolcini wrote:

...

> > > +	err = regmap_bulk_read(ec->regmap, EC_CHIP_ID_REG, &reg_val, EC_ID_VERSION_LEN);
> > > +	if (err)
> > > +		return dev_err_probe(dev, err,
> > > +				     "Cannot read id and version registers\n");
> > > +
> > > +	dev_info(dev, "Toradex Embedded Controller id %x - Firmware %d.%d\n",
> > 
> > Specifiers are semirandom. Why signed? Why x and not %u?
> 
> The firmware version ("Firmware %d.%d") is two unsigned decimal number,
> so yes, I will change to "Firmware %u.%u".
> 
> The ID is just an identifier that is documented as hex, therefore I
> think that the most convenient way to display it is as a hex number.

Thanks, don't forget to summarize this in the commit message, it seems
useful information.
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 19d7c17c0115..fa1630f0f725 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23977,6 +23977,7 @@  M:	Emanuele Ghidoli <ghidoliemanuele@gmail.com>
 M:	Francesco Dolcini <francesco@dolcini.it>
 S:	Maintained
 F:	Documentation/devicetree/bindings/firmware/toradex,embedded-controller.yaml
+F:	drivers/platform/toradex/toradex-ec.c
 
 TORTURE-TEST MODULES
 M:	Davidlohr Bueso <dave@stgolabs.net>
diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
index 960fd6a82450..84aabb88fb4a 100644
--- a/drivers/platform/Kconfig
+++ b/drivers/platform/Kconfig
@@ -15,6 +15,8 @@  source "drivers/platform/olpc/Kconfig"
 
 source "drivers/platform/surface/Kconfig"
 
+source "drivers/platform/toradex/Kconfig"
+
 source "drivers/platform/x86/Kconfig"
 
 source "drivers/platform/arm64/Kconfig"
diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile
index 19ac54648586..2d849a8f3ccf 100644
--- a/drivers/platform/Makefile
+++ b/drivers/platform/Makefile
@@ -13,3 +13,4 @@  obj-$(CONFIG_CHROME_PLATFORMS)	+= chrome/
 obj-$(CONFIG_CZNIC_PLATFORMS)	+= cznic/
 obj-$(CONFIG_SURFACE_PLATFORMS)	+= surface/
 obj-$(CONFIG_ARM64_PLATFORM_DEVICES)	+= arm64/
+obj-$(CONFIG_TORADEX_PLATFORMS)	+= toradex/
diff --git a/drivers/platform/toradex/Kconfig b/drivers/platform/toradex/Kconfig
new file mode 100644
index 000000000000..635d955df79d
--- /dev/null
+++ b/drivers/platform/toradex/Kconfig
@@ -0,0 +1,18 @@ 
+menuconfig TORADEX_PLATFORMS
+	bool "Platform support for Toradex hardware"
+	help
+	  Say Y here to be able to choose driver support for Toradex
+	  devices. This option alone does not add any kernel code.
+
+if TORADEX_PLATFORMS
+config TORADEX_EC
+	tristate "Toradex Embedded Controller driver"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say Y here to add support for the features implemented by the
+	  Embedded Controller on the Toradex Modules.
+	  To compile this driver as a module, choose M here; the module will be
+	  called toradex-ec.
+
+endif # TORADEX_PLATFORMS
diff --git a/drivers/platform/toradex/Makefile b/drivers/platform/toradex/Makefile
new file mode 100644
index 000000000000..eb918819ad35
--- /dev/null
+++ b/drivers/platform/toradex/Makefile
@@ -0,0 +1 @@ 
+obj-$(CONFIG_TORADEX_EC)			+= toradex-ec.o
diff --git a/drivers/platform/toradex/toradex-ec.c b/drivers/platform/toradex/toradex-ec.c
new file mode 100644
index 000000000000..f01addf2a0a6
--- /dev/null
+++ b/drivers/platform/toradex/toradex-ec.c
@@ -0,0 +1,155 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Toradex Embedded Controller driver
+ *
+ * Copyright (C) 2025 Toradex
+ *
+ * Author: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/reboot.h>
+#include <linux/regmap.h>
+
+#define EC_CHIP_ID_REG                  0x00
+#define EC_VERSION_REG_MAJOR            0x01
+#define EC_VERSION_REG_MINOR            0x02
+#define EC_CMD_REG                      0xD0
+#define EC_REG_MAX                      0xD0
+
+#define EC_CHIP_ID_SMARC_IMX95          0x11
+#define EC_CHIP_ID_SMARC_IMX8MP         0x12
+
+#define EC_POWEROFF_CMD                 0x01
+#define EC_RESET_CMD                    0x02
+
+#define EC_ID_VERSION_LEN               3
+
+struct tdx_ec {
+	struct i2c_client *client;
+	struct regmap *regmap;
+};
+
+static const struct regmap_range volatile_ranges[] = {
+	regmap_reg_range(EC_CMD_REG, EC_CMD_REG),
+};
+
+static const struct regmap_access_table volatile_table = {
+	.yes_ranges	= volatile_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(volatile_ranges),
+};
+
+static const struct regmap_range read_ranges[] = {
+	regmap_reg_range(EC_CHIP_ID_REG, EC_VERSION_REG_MINOR),
+};
+
+static const struct regmap_access_table read_table = {
+	.yes_ranges	= read_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(read_ranges),
+};
+
+static const struct regmap_config regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= EC_REG_MAX,
+	.cache_type	= REGCACHE_RBTREE,
+	.rd_table	= &read_table,
+	.volatile_table = &volatile_table,
+};
+
+static int tdx_ec_cmd(struct tdx_ec *ec, u8 cmd)
+{
+	int err = regmap_write(ec->regmap, EC_CMD_REG, cmd);
+
+	if (err)
+		dev_err(&ec->client->dev, "Failed to send command 0x%02X: %d\n", cmd, err);
+	return err;
+}
+
+static int tdx_ec_power_off(struct sys_off_data *data)
+{
+	struct tdx_ec *ec = data->cb_data;
+	int err;
+
+	err = tdx_ec_cmd(ec, EC_POWEROFF_CMD);
+	return err ? NOTIFY_BAD : NOTIFY_DONE;
+}
+
+static int tdx_ec_restart(struct sys_off_data *data)
+{
+	struct tdx_ec *ec = data->cb_data;
+	int err;
+
+	err = tdx_ec_cmd(ec, EC_RESET_CMD);
+	return err ? NOTIFY_BAD : NOTIFY_DONE;
+}
+
+static int tdx_ec_register_power_off_restart(struct device *dev, struct tdx_ec *ec)
+{
+	int err;
+
+	err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART,
+					    SYS_OFF_PRIO_FIRMWARE,
+					    tdx_ec_restart, ec);
+	if (err)
+		return err;
+
+	err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF,
+					    SYS_OFF_PRIO_FIRMWARE,
+					    tdx_ec_power_off, ec);
+	return err;
+}
+
+static int tdx_ec_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	u8 reg_val[EC_ID_VERSION_LEN];
+	struct tdx_ec *ec;
+	int err;
+
+	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
+	if (!ec)
+		return -ENOMEM;
+
+	ec->client = client;
+	i2c_set_clientdata(client, ec);
+
+	ec->regmap = devm_regmap_init_i2c(client, &regmap_config);
+	if (IS_ERR(ec->regmap))
+		return PTR_ERR(ec->regmap);
+
+	err = regmap_bulk_read(ec->regmap, EC_CHIP_ID_REG, &reg_val, EC_ID_VERSION_LEN);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "Cannot read id and version registers\n");
+
+	dev_info(dev, "Toradex Embedded Controller id %x - Firmware %d.%d\n",
+		 reg_val[0], reg_val[1], reg_val[2]);
+
+	err = tdx_ec_register_power_off_restart(dev, ec);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "Cannot register system restart handler\n");
+
+	return 0;
+}
+
+static const struct of_device_id __maybe_unused of_tdx_ec_match[] = {
+	{ .compatible = "toradex,embedded-controller" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_tdx_ec_match);
+
+static struct i2c_driver toradex_ec_driver = {
+	.probe			= tdx_ec_probe,
+	.driver			= {
+		.name		= "toradex-ec",
+		.of_match_table = of_tdx_ec_match,
+	},
+};
+module_i2c_driver(toradex_ec_driver);
+
+MODULE_AUTHOR("Emanuele Ghidoli <emanuele.ghidoli@toradex.com>");
+MODULE_DESCRIPTION("Toradex Embedded Controller driver");
+MODULE_LICENSE("GPL");