Re: [RFC PATCH 2/5] mfd: Add ROHM BD79124 ADC/GPO

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Fri, 31 Jan 2025 15:37:06 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Add core driver for the ROHM BD79124 ADC / GPO.
> 
> The core driver launches the sub-drivers for the pinmux/GPO and for the
> IIO ADC. It also provides the regmap, and forwards the IRQ resource to
> the ADC.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
As per response in cover letter. This is a common device combination and so
far I don't think we ever bothered with an MFD. Lots of ADCs provide
GPIO chips as well so I'd just squash it into the ADC driver.

> ---
>  drivers/mfd/Kconfig              |  12 +++
>  drivers/mfd/Makefile             |   1 +
>  drivers/mfd/rohm-bd79124.c       | 165 +++++++++++++++++++++++++++++++
>  include/linux/mfd/rohm-bd79124.h |  32 ++++++
>  4 files changed, 210 insertions(+)
>  create mode 100644 drivers/mfd/rohm-bd79124.c
>  create mode 100644 include/linux/mfd/rohm-bd79124.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ae23b317a64e..f024256fb180 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2113,6 +2113,18 @@ config MFD_ROHM_BD71828
>  	  also a single-cell linear charger, a Coulomb counter, a real-time
>  	  clock (RTC), GPIOs and a 32.768 kHz clock gate.
>  
> +config MFD_ROHM_BD79124
> +	tristate "Rohm BD79124 core driver"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Say yes here to build support for the ROHM BD79124 ADC core. The
> +	  ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
> +	  also an automatic measurement mode, with an alarm interrupt for
> +	  out-of-window measurements. The window is configurable for each
> +	  channel. The ADC inputs can optionally be used as general purpose
> +	  outputs.
> +
>  config MFD_ROHM_BD957XMUF
>  	tristate "ROHM BD9576MUF and BD9573MUF Power Management ICs"
>  	depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e057d6d6faef..c7d64e933a7d 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -270,6 +270,7 @@ obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
>  obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
>  obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
>  obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
> +obj-$(CONFIG_MFD_ROHM_BD79124)	+= rohm-bd79124.o
>  obj-$(CONFIG_MFD_ROHM_BD957XMUF)	+= rohm-bd9576.o
>  obj-$(CONFIG_MFD_ROHM_BD96801)	+= rohm-bd96801.o
>  obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
> diff --git a/drivers/mfd/rohm-bd79124.c b/drivers/mfd/rohm-bd79124.c
> new file mode 100644
> index 000000000000..c35ab0e03b0b
> --- /dev/null
> +++ b/drivers/mfd/rohm-bd79124.c
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (C) 2025 ROHM Semiconductors
> +//
> +// ROHM BD79124 ADC / GPO driver
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>

mod_devicetable.h

> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/rohm-bd79124.h>
> +
> +static struct resource adc_alert;

What if we have two of these?

> +
> +enum {
> +	CELL_PINMUX,
> +	CELL_ADC,
> +};
> +
> +static struct mfd_cell bd79124_cells[] = {
> +	[CELL_PINMUX]	= { .name = "bd79124-pinmux", },
> +	[CELL_ADC]	= { .name = "bd79124-adc", },
> +};
> +
> +/* Read-only regs */
> +static const struct regmap_range bd79124_ro_ranges[] = {
> +	{
> +		.range_min = BD79124_REG_EVENT_FLAG,
> +		.range_max = BD79124_REG_EVENT_FLAG,
> +	}, {
> +		.range_min = BD79124_REG_RECENT_CH0_LSB,
> +		.range_max = BD79124_REG_RECENT_CH7_MSB,
> +	},
> +};
> +
> +static const struct regmap_access_table bd79124_ro_regs = {
> +	.no_ranges	= &bd79124_ro_ranges[0],
> +	.n_no_ranges	= ARRAY_SIZE(bd79124_ro_ranges),
> +};
> +
> +static const struct regmap_range bd79124_volatile_ranges[] = {
> +	{
> +		.range_min = BD79124_REG_RECENT_CH0_LSB,
> +		.range_max = BD79124_REG_RECENT_CH7_MSB,
> +	}, {
> +		.range_min = BD79124_REG_EVENT_FLAG,
> +		.range_max = BD79124_REG_EVENT_FLAG,
> +	}, {
> +		.range_min = BD79124_REG_EVENT_FLAG_HI,
> +		.range_max = BD79124_REG_EVENT_FLAG_HI,
> +	}, {
> +		.range_min = BD79124_REG_EVENT_FLAG_LO,
> +		.range_max = BD79124_REG_EVENT_FLAG_LO,
> +	}, {
> +		.range_min = BD79124_REG_SYSTEM_STATUS,
> +		.range_max = BD79124_REG_SYSTEM_STATUS,
> +	},
> +};
> +
> +static const struct regmap_access_table bd79124_volatile_regs = {
> +	.yes_ranges	= &bd79124_volatile_ranges[0],
> +	.n_yes_ranges	= ARRAY_SIZE(bd79124_volatile_ranges),
> +};
> +
> +static const struct regmap_range bd79124_precious_ranges[] = {
> +	{
> +		.range_min = BD79124_REG_EVENT_FLAG_HI,
> +		.range_max = BD79124_REG_EVENT_FLAG_HI,
> +	}, {
> +		.range_min = BD79124_REG_EVENT_FLAG_LO,
> +		.range_max = BD79124_REG_EVENT_FLAG_LO,
> +	},
> +};
> +
> +static const struct regmap_access_table bd79124_precious_regs = {
> +	.yes_ranges	= &bd79124_precious_ranges[0],
> +	.n_yes_ranges	= ARRAY_SIZE(bd79124_precious_ranges),
> +};
> +
> +static const struct regmap_config bd79124_regmap = {
> +	.reg_bits		= 16,
> +	.val_bits		= 8,
> +	.read_flag_mask		= BD79124_I2C_MULTI_READ,
> +	.write_flag_mask	= BD79124_I2C_MULTI_WRITE,
> +	.max_register		= BD79124_REG_MAX,
> +	.cache_type		= REGCACHE_MAPLE,
> +	.volatile_table		= &bd79124_volatile_regs,
> +	.wr_table		= &bd79124_ro_regs,
> +	.precious_table		= &bd79124_precious_regs,
> +};
> +
> +static int bd79124_probe(struct i2c_client *i2c)
> +{
> +	int ret;
> +	struct regmap *map;
> +	struct device *dev = &i2c->dev;
> +	int *adc_vref;

Wrap that in a structure.  It's just a bit too odd to have just
one integer!

> +
> +	adc_vref = devm_kzalloc(dev, sizeof(*adc_vref), GFP_KERNEL);
> +	if (!adc_vref)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Better to enable regulators here so we don't need to worry about the
> +	 * order of sub-device instantiation. We also need to deliver the
> +	 * reference voltage value to the ADC driver. This is done via
> +	 * the MFD driver's drvdata.
> +	 */
> +	*adc_vref = devm_regulator_get_enable_read_voltage(dev, "vdd");
> +	if (*adc_vref < 0)
> +		return dev_err_probe(dev, ret, "Failed to get the Vdd\n");
> +
> +	dev_set_drvdata(dev, adc_vref);
> +
> +	ret = devm_regulator_get_enable(dev, "iovdd");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to enable I/O voltage\n");
> +
> +	map = devm_regmap_init_i2c(i2c, &bd79124_regmap);
> +	if (IS_ERR(map))
> +		return dev_err_probe(dev, PTR_ERR(map),
> +				     "Failed to initialize Regmap\n");
> +
> +	if (i2c->irq) {
> +		adc_alert = DEFINE_RES_IRQ_NAMED(i2c->irq, "thresh-alert");
> +		bd79124_cells[CELL_ADC].resources = &adc_alert;
> +		bd79124_cells[CELL_ADC].num_resources = 1;
> +	}
> +
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, bd79124_cells,
> +				   ARRAY_SIZE(bd79124_cells), NULL, 0, NULL);
> +	if (ret)
> +		dev_err_probe(dev, ret, "Failed to create subdevices\n");
return dev_err_probe();

Then return 0 in other path.

> +
> +	return ret;
> +}
>
> diff --git a/include/linux/mfd/rohm-bd79124.h b/include/linux/mfd/rohm-bd79124.h
> new file mode 100644
> index 000000000000..505faeb6f135
> --- /dev/null
> +++ b/include/linux/mfd/rohm-bd79124.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright 2021 ROHM Semiconductors.

No update on that date?

> + *
> + * Author: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> + */
> +
> +#ifndef _MFD_BD79124_H
> +#define _MFD_BD79124_H
> +
> +#define BD79124_I2C_MULTI_READ		0x30
> +#define BD79124_I2C_MULTI_WRITE		0x28
> +#define BD79124_REG_MAX			0xaf
> +
> +#define BD79124_REG_SYSTEM_STATUS	0x0

Give it two digits. 0x00 for ever so slight readability advantage.

> +#define BD79124_REG_GEN_CFG		0x01
> +#define BD79124_REG_OPMODE_CFG		0x04
> +#define BD79124_REG_PINCFG		0x05
> +#define BD79124_REG_GPO_VAL		0x06
> +#define BD79124_REG_SEQUENCE_CFG	0x10
> +#define BD79124_REG_MANUAL_CHANNELS	0x11
> +#define BD79124_REG_AUTO_CHANNELS	0x12
> +#define BD79124_REG_ALERT_CH_SEL	0x14
> +#define BD79124_REG_EVENT_FLAG		0x18
> +#define BD79124_REG_EVENT_FLAG_HI	0x1a
> +#define BD79124_REG_EVENT_FLAG_LO	0x1c
> +#define BD79124_REG_HYSTERESIS_CH0	0x20
> +#define BD79124_REG_EVENTCOUNT_CH0	0x22
> +#define BD79124_REG_RECENT_CH0_LSB	0xa0
> +#define BD79124_REG_RECENT_CH7_MSB	0xaf
> +
> +#endif