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

[Andy Shevchenko <andy@xxxxxxxxxx>]

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