Re: [RFC PATCH] mfd: add simple regmap based I2C driver

[Lee Jones]

On Wed, 01 Jul 2020, Michael Walle wrote:

> There are I2C devices which contain several different functions but
> doesn't require any special access functions. For these kind of drivers
> an I2C regmap should be enough.
> 
> Create an I2C driver which creates an I2C regmap and enumerates its
> children. If a device wants to use this as its MFD core driver, it has
> to add an individual compatible string. It may provide its own regmap
> configuration and a .pre_probe hook. The latter allows simple checks
> and abort probing, for example a version check.
> 
> Subdevices can use dev_get_regmap() on the parent to get their regmap
> instance.
> 
> Signed-off-by: Michael Walle <michael@xxxxxxxx>
> ---
> 
> I don't think the syscon-i2c is the way to go:
> 
>    "TBC, while fine for a driver to bind on 'simple-mfd', a
>     DT compatible with that alone is not fine." [1]
> 
>    "Yes, this is why specific compatible strings are
>     required." [1]
> 
>    "No. I'd like to just kill off syscon and simple-mfd really. Those are
> 	just hints meaning a specific compatible is still needed, but I see
> 	them all the time alone (or combined like above). 'syscon' just serves
> 	to create a regmap. This could be accomplished just with a list of
> 	compatibles to register a regmap for. That might be a longish list, but
> 	wanting a regmap is really a kernel implementation detail and
> 	decision." [2]

This is taken out of context.  Rob is suggesting that 'simple-mfd'
and/or 'syscon' shouldn't be used as separate entities.  Instead they
should be coupled with a real and descriptive compatible string.
Either 'simple-mfd' nor 'syscon' is going away.

> So I don't get it why we would now add another syscon type. Instead, here
> is a new try to generalize the idea to just have a simple driver which just
> have a bunch of compatible strings for supported devices (and thus can be
> added quirks to it without changing the DT) and just creates a regmap. A
> simple device can just add itself to the list of compatible strings. If
> quirks are needed later, they can either be added right to simple-mfd-i2c.c
> or split off to an own file, but time will tell.
> 
> Right now, there is just a .pre_probe() hook which can be used to cancel
> the probing of the device or print some useful info to the kernel log. Yes,
> this is for "my" version check. And TBH I don't see a problem with adding
> that to this generic driver.
> 
> [1] https://lore.kernel.org/linux-devicetree/CAL_JsqKr1aDVzgAMjwwK8E8O_f29vSrx1HXk81FF+rd3sEe==w@xxxxxxxxxxxxxx/
> [2] https://lore.kernel.org/linux-devicetree/20200609165401.GB1019634@bogus/
> 
>  drivers/mfd/Kconfig          |  9 ++++++
>  drivers/mfd/Makefile         |  1 +
>  drivers/mfd/simple-mfd-i2c.c | 57 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 67 insertions(+)
>  create mode 100644 drivers/mfd/simple-mfd-i2c.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 7e72ed3441f1..96055c7e5c55 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1162,6 +1162,15 @@ config MFD_SI476X_CORE
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called si476x-core.
>  
> +config MFD_SIMPLE_MFD_I2C
> +	tristate "Simple regmap based I2C devices"
> +	depends on I2C
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	help
> +	  This is a consolidated driver for all MFD devices which are
> +	  basically just a regmap bus driver.
> +
>  config MFD_SM501
>  	tristate "Silicon Motion SM501"
>  	depends on HAS_DMA
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7e26e7f98ac2..fa3a621a5a21 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -265,4 +265,5 @@ obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
>  
>  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
>  
> +obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
>  obj-$(CONFIG_MFD_SL28CPLD)	+= sl28cpld.o
> diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
> new file mode 100644
> index 000000000000..60708e95f1a0
> --- /dev/null
> +++ b/drivers/mfd/simple-mfd-i2c.c
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/regmap.h>
> +
> +struct simple_mfd_i2c_config {
> +	int (*pre_probe)(struct i2c_client *i2c, struct regmap *regmap);

This has the potential to get out of control.

> +	const struct regmap_config *regmap_config;
> +};
> +
> +static const struct regmap_config simple_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static int simple_mfd_i2c_probe(struct i2c_client *i2c)
> +{
> +	const struct regmap_config *regmap_config = &simple_regmap_config;
> +	const struct simple_mfd_i2c_config *config;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	config = device_get_match_data(&i2c->dev);
> +
> +	if (config && config->regmap_config)
> +		regmap_config = config->regmap_config;
> +
> +	regmap = devm_regmap_init_i2c(i2c, regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	if (config && config->pre_probe) {
> +		ret = config->pre_probe(i2c, regmap);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return devm_of_platform_populate(&i2c->dev);
> +}
> +
> +static const struct of_device_id simple_mfd_i2c_of_match[] = {
> +	{}
> +};
> +
> +static struct i2c_driver simple_mfd_i2c_driver = {
> +	.probe_new = simple_mfd_i2c_probe,
> +	.driver = {
> +		.name = "simple-mfd-i2c",
> +		.of_match_table = simple_mfd_i2c_of_match,
> +	},
> +};
> +builtin_i2c_driver(simple_mfd_i2c_driver);

I'm not completely adverse to this idea.  There are a few things in
here I'd change, but the template is reasonable enough, despite the
opportunity for abuse.

However, before exploring this I'd like to properly see out the I2C
syscon conversation.

-- 
Lee Jones [æçæ]
Senior Technical Lead - Developer Services
Linaro.org â Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog