On Wed, 20 May 2020, Lubomir Rintel wrote:
> This driver provides access to the EC RAM of said embedded controller
> attached to the I2C bus as well as optionally supporting its slightly weird
> power-off/restart protocol.
>
> A particular implementation of the EC firmware can be identified by a
> model byte. If this driver identifies the Dell Ariel platform, it
> registers the appropriate cells.
>
> Signed-off-by: Lubomir Rintel <lkundrak@xxxxx>
>
> ---
> Changes since v2:
> - Sort the includes
> - s/EC_MODEL_ID/EC_MODEL/
> - Add a couple of clarifying comments
> - Use #defines for values used in poweroff routine
> - Remove priority from a restart notifier block
> - s/priv/ddata/
> - s/ec_ram/ram_regmap/ for the regmap name
> - Fix the error handling when getting off gpios was not successful
> - Remove a useless dev_info at the end of probe()
> - Use i2c probe_new() callback, drop i2c_device_id
> - Modify the logic in checking the model ID
>
> drivers/mfd/Kconfig | 10 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/ene-kb3930.c | 215 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 226 insertions(+)
> create mode 100644 drivers/mfd/ene-kb3930.c
Really starting to take shape.
Just a couple of nits, then we're good to go.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 0a59249198d3..dae18a2beab5 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -398,6 +398,16 @@ config MFD_DLN2
> etc. must be enabled in order to use the functionality of
> the device.
>
> +config MFD_ENE_KB3930
> + tristate "ENE KB3930 Embedded Controller support"
> + depends on I2C
> + depends on MACH_MMP3_DT || COMPILE_TEST
> + select MFD_CORE
> + help
> + This adds support for accessing the registers on ENE KB3930, Embedded
> + Controller. Additional drivers such as LEDS_ARIEL must be enabled in
> + order to use the functionality of the device.
Can you mention/describe all of the sub-devices please?
[...]
> +struct kb3930 *global_kb3930;
Can we call this kb3930_power_off please.
[...]
> +static int kb3930_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct device_node *np = dev->of_node;
> + struct kb3930 *ddata;
> + unsigned int model;
> + int ret;
> +
> + if (global_kb3930)
> + return -EEXIST;
This should not happen. If .probe() is called twice, either
-EDEFER_PROBE was returned or a new device was registered.
[...]
> + /* These are the cells valid for model == 'J' only. */
> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> + ariel_ec_cells,
> + ARRAY_SIZE(ariel_ec_cells),
> + NULL, 0, NULL);
> + if (ret < 0)
if (ret)
> + return ret;
[...]
> +static struct i2c_driver kb3930_driver = {
> + .probe_new = kb3930_probe,
> + .remove = kb3930_remove,
> + .driver = {
> + .name = "ene-kb3930",
> + .of_match_table = of_match_ptr(kb3930_dt_ids),
> + },
> +};
> +
Remove this line please.
> +module_i2c_driver(kb3930_driver);
> +
> +MODULE_AUTHOR("Lubomir Rintel <lkundrak@xxxxx>");
> +MODULE_DESCRIPTION("ENE KB3930 Embedded Controller Driver");
> +MODULE_LICENSE("Dual BSD/GPL");
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
