linux-kernel - Re: [RFC PATCH v1 2/2] platform: toradex: add preliminary support for Embedded Controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z9LxbzJ3zf0RT-JS@smile.fi.intel.com>
Date: Thu, 13 Mar 2025 16:53:35 +0200
From: Andy Shevchenko <andy@...nel.org>
To: Francesco Dolcini <francesco@...cini.it>
Cc: Emanuele Ghidoli <ghidoliemanuele@...il.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Emanuele Ghidoli <emanuele.ghidoli@...adex.com>,
	Francesco Dolcini <francesco.dolcini@...adex.com>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-i2c@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	soc@...nel.org, Hans de Goede <hdegoede@...hat.com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [RFC PATCH v1 2/2] platform: toradex: add preliminary support
 for Embedded Controller

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;
> +}

-- 
With Best Regards,
Andy Shevchenko