Re: [PATCH v3 2/2] mfd: ene-kb3930: Add driver for ENE KB3930 Embedded Controller

From: Lee Jones <lee.jones@linaro.org>
To: Lubomir Rintel <lkundrak@v3.sk>
Cc: Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/2] mfd: ene-kb3930: Add driver for ENE KB3930 Embedded Controller
Date: Thu, 21 May 2020 08:27:57 +0100	[thread overview]
Message-ID: <20200521072757.GM271301@dell> (raw)
In-Reply-To: <20200520185955.2144252-3-lkundrak@v3.sk>

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@v3.sk>
> 
> ---
> 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@v3.sk>");
> +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