Skip to content

iio: adc: Add initial driver support for MAX22531 #2854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: rpi-6.6.y_GSOC_2025_MAX22531
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions arch/arm/boot/dts/overlays/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,7 @@ dtbo-$(CONFIG_ARCH_BCM2835) += \
rpi-ltc6952.dtbo \
rpi-max14830-i2c.dtbo \
rpi-max14830-spi.dtbo \
rpi-max22531.dtbo \
rpi-max31335.dtbo \
rpi-poe.dtbo \
rpi-poe-plus.dtbo \
Expand Down
32 changes: 32 additions & 0 deletions arch/arm/boot/dts/overlays/rpi-max22531-overlay.dts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// SPDX-License-Identifier: (GPL-2.0+)
/*
* Device Tree Overlay for MAX22531
*/

/dts-v1/;
/plugin/;

/ {
compatible = "brcm,bcm2835-spi", "brcm,bcm2711";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may need to add brcm,bcm2712 to this list to actually indicate this overlay is compatible with rpi5.

-	compatible = "brcm,bcm2835-spi", "brcm,bcm2711";
+	compatible = "brcm,bcm2712", "brcm,bcm2835-spi", "brcm,bcm2711";


reg_vdd: regulator {
compatible = "regulator-fixed";
regulator-name = "vdd";
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
regulator-boot-on;
regulator-always-on;
};
};

&spi0 {
status = "okay";

max22531@0 {
compatible = "maxim,max22531";
reg = <0>; /* Using CS0 on spi0 */
spi-max-frequency = <10000000>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 MHz is relatively fast for a rpi SPI controller. If you observe transfers bringing strange results or failing when you receive the hw, then consider decreasing the SPI clock frequency.

vref-supply = <&reg_vdd>;

};
};
6 changes: 6 additions & 0 deletions drivers/iio/adc/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -1126,6 +1126,12 @@ config MAX1363
To compile this driver as a module, choose M here: the module will be
called max1363.

config MAX22531
tristate "Analog Devices MAX22531 ADC Driver"
depends on SPI
help
Say yes here to build support for this driver.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a thing crucial for the moment, but I'd add the usual config help text boilerplate here. Upstream folks may request it, and there's nothing preventing us from adding a few more lines of text here.


config MAX77541_ADC
tristate "Analog Devices MAX77541 ADC driver"
depends on MFD_MAX77541
Expand Down
1 change: 1 addition & 0 deletions drivers/iio/adc/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ obj-$(CONFIG_MAX11205) += max11205.o
obj-$(CONFIG_MAX11410) += max11410.o
obj-$(CONFIG_MAX1241) += max1241.o
obj-$(CONFIG_MAX1363) += max1363.o
obj-$(CONFIG_MAX22531) += max22531.o
obj-$(CONFIG_MAX77541_ADC) += max77541-adc.o
obj-$(CONFIG_MAX9611) += max9611.o
obj-$(CONFIG_MCP320X) += mcp320x.o
Expand Down
170 changes: 170 additions & 0 deletions drivers/iio/adc/max22531.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
#include <linux/module.h>
#include <linux/spi/spi.h>
#include <linux/iio/iio.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>

#define MAX22531_REG_PROD_ID 0x00
#define MAX22531_REG_ADC1 0x01
#define MAX22531_REG_ADC2 0x02
#define MAX22531_REG_ADC3 0x03
#define MAX22531_REG_ADC4 0x04
#define MAX22531_REG_FADC1 0x05
#define MAX22531_REG_FADC2 0x06
#define MAX22531_REG_FADC3 0x07
#define MAX22531_REG_FADC4 0x08
#define MAX22531_REG_COUTHI1 0x09
#define MAX22531_REG_COUTHI2 0x0a
#define MAX22531_REG_COUTHI3 0x0b
#define MAX22531_REG_COUTHI4 0x0c
#define MAX22531_REG_COUTLO1 0x0d
#define MAX22531_REG_COUTLO2 0x0e
#define MAX22531_REG_COUTLO3 0x0f
#define MAX22531_REG_COUTLO4 0x10
#define MAX22531_REG_COUT_STATUS 0x11
#define MAX22531_REG_INTERRUPT_STATUS 0x12
#define MAX22531_REG_INTERRUPT_ENABLE 0x13
#define MAX22531_REG_CONTROL 0x14

enum max22531_id {
max22531,
};

struct max22531 {
struct spi_device *spi_dev;
struct regulator *vref;
struct regulator *vddl;
struct regulator *vddf;
struct regmap *regmap;
};

#define MAX22531_CHANNEL(ch) \
{ \
.type = IIO_VOLTAGE, \
.indexed = 1, \
.channel = (ch), \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
.scan_index = ch, \
.output = 0, \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed

-		.output = 0,						\

}

static const struct iio_chan_spec max22531_channels[] = {
MAX22531_CHANNEL(0),
MAX22531_CHANNEL(1),
MAX22531_CHANNEL(2),
MAX22531_CHANNEL(3),
IIO_CHAN_SOFT_TIMESTAMP(2),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave the timestamp channel for after buffer support is implemented. Timestamp channels are not very useful on their own.

};

static const struct regmap_config regmap_config = {
.reg_bits = 16,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SPI read/write commands (datasheet Table 1 and Table 2) have 8-bit header so we would have .reg_bits = 8 (or reg_bits = 6 maybe).
Though, the header also contains the W/R and burst bits.
I think the W/R can be handled with
.write_flag_mask = BIT(1)
the burst bit skipped with
.pad_bits = 1
See https://elixir.bootlin.com/linux/v6.15.6/source/include/linux/regmap.h#L254

.val_bits = 16,
.max_register = 0x14,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to think this becomes slightly more readable when using the register definition

-	.max_register = 0x14,
+	.max_register = MAX22531_REG_CONTROL,

Though, we have both ways of setting max_register upstream, so up to you to use the address or reg define.

};

static int max22531_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
{
struct regmap **regmap = iio_priv(indio_dev);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In probe(), iio_dev is allocated with an extra size for struct max22531. So

-	struct regmap **regmap = iio_priv(indio_dev);
+	struct max22531 *adc = iio_priv(indio_dev);

int ret;

/* mock for now */
switch(mask) {
case IIO_CHAN_INFO_RAW:
ret = regmap_read(*regmap, chan->address, val);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then this needs to be updated

-		ret = regmap_read(*regmap, chan->address, val);
+		ret = regmap_read(adc->regmap, chan->address, val);

if (ret)
return ret;
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
return IIO_VAL_INT;
default:
return -EINVAL;
}
}

static const struct iio_info max22531_info = {
.read_raw = max22531_read_raw,
};

static void max22531_regulator_disable(void *reg)
{
regulator_disable(reg);
}

static int max22531_probe(struct spi_device *spi)
{
dev_info(&spi->dev, "MAX22531: probing ADC\n");

unsigned int ret, prod_id;
struct max22531 *adc;
struct iio_dev *indio_dev;

indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
if (!indio_dev) {
dev_err(&spi->dev, "MAX22531: Failed to allocate memory"
"for IIO device.\n");
Comment on lines +106 to +107
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is odd. Don't break strings like that ...

-		dev_err(&spi->dev, "MAX22531: Failed to allocate memory"
-			       "for IIO device.\n");
+		dev_err(&spi->dev, "Failed to allocate memory for IIO device.\n");

Actually, dev_err_probe() will make this more concise.

-	if (!indio_dev) {
-		dev_err(&spi->dev, "MAX22531: Failed to allocate memory"
-			       "for IIO device.\n");
-		return -ENOMEM;
-	}
+	if (!indio_dev)
+		dev_err_probe(&spi->dev,  -ENOMEM, "Failed to allocate memory for IIO device.\n");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I run checkpatch.pl on this, it would show error because the line crosses 75 char limit.
Is there any better option to do this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was lazy when suggesting the dev_err_probe() use. That can be coded as:

	if (!indio_dev)
		dev_err_probe(&spi->dev,  -ENOMEM,
				"Failed to allocate memory for IIO device.\n");

or similar to that. The starting " of the string needs to be aligned (same column) as the & of &spi->dev. Notice kernel codestyle is 8 size tab indentation and spaces to align when needed.
Hard to see if the above suggestion complies to that but you're probably be able to set that properly on a decent code editor. And no worries if putting text on a separate line still goes beyond 80 columns. Checkpatch doesn't always need to be followed strictly.

return -ENOMEM;
}

adc = iio_priv(indio_dev);
adc->spi_dev = spi;

indio_dev->name = "max22531";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be better set according to the specific device (max22530, max22531, or max22532) if a chip_info struct is implemented. See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/ad7124.c?h=v6.16-rc6#n196 for an example.

indio_dev->info = &max22531_info;
indio_dev->channels = max22531_channels;
indio_dev->num_channels = ARRAY_SIZE(max22531_channels);

adc->regmap = devm_regmap_init_spi(spi, &regmap_config);
if (IS_ERR(adc->regmap))
dev_err(&spi->dev, "regmap init failure\n");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually when something critical fails in the probe it is better to stop and return, like:
return dev_err_probe(&spi->dev, PTR_ERR(adc->regmap), "regmap init failure\n")

Same for other similar cases here.


ret = regmap_read(adc->regmap, MAX22531_REG_PROD_ID, &prod_id);
if (ret)
dev_err(&spi->dev, "Failed to read PROD_ID\n");
else
dev_info(&spi->dev, "MAX22531: Successfully read PROD_ID"
": %d from the driver.\n", ret);

adc->vref = devm_regulator_get(&spi->dev, "vref");
if (IS_ERR(adc->vref))
dev_err(&spi->dev, "Failed to retrieve vref\n");

ret = regulator_enable(adc->vref);
if (ret)
return ret;
Comment on lines +130 to +136
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vref is an internal voltage reference that is always on/enabled. If I'm not missing anything, we can just use vref. E.g.

#define MAX22531_VREF_MV	1800

thus we don't need to keep a vref regulator field in struct max22351.
For regulators, the driver should request VDDL and VDDPL regulators, as those must be supplied to power the MAX22531 chip.


ret = devm_add_action_or_reset(&spi->dev, max22531_regulator_disable,
adc->vref);
if (ret)
return ret;

return devm_iio_device_register(&spi->dev, indio_dev);
}

static const struct spi_device_id max22531_id[] = {
{ "max22531" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provide entries for max22530, and max22532 too

{ }
};
MODULE_DEVICE_TABLE(spi, max22531_id);

static const struct of_device_id max22531_spi_of_id[] = {
{ .compatible = "adi,max22531" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, add OF entries for matching with max22530 and max22532.

{ }
};
MODULE_DEVICE_TABLE(of, max22531_spi_of_id);

static struct spi_driver max22531_driver = {
.driver = {
.name = "max22531",
.of_match_table = max22531_spi_of_id,
},
.probe = max22531_probe,
.id_table = max22531_id,
};
module_spi_driver(max22531_driver);

MODULE_AUTHOR("Abhinav Jain <[email protected]>");
MODULE_DESCRIPTION("MAX22531 ADC");
MODULE_LICENSE("GPL v2");