On Fri, 31 Jan 2025 15:37:06 +0200
Matti Vaittinen [off-list ref] 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.

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.

No update on that date?

+ *
+ * Author: Matti Vaittinen [off-list ref]
+ */
+
+#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