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

Andy Shevchenko
2025-03-13T14:53:42Z

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