-
Notifications
You must be signed in to change notification settings - Fork 897
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
base: rpi-6.6.y_GSOC_2025_MAX22531
Are you sure you want to change the base?
iio: adc: Add initial driver support for MAX22531 #2854
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @KernelShinobi , a few comments about this initial driver version.
Note that, aside from the comments above, there are still some improvements that need to be made to make this driver usable (e.g. read ADC sample data, provide scale to convert output codes to mV). Those features shall be implemented next. Though, overall, this looks good to me as an initial driver version.
Hello, so I've set up MAX2531_EVKIT_A and tested the initial MAX22531 driver version. I've added "brcm,bcm2712" to device tree overlay compatible list and added another @KernelShinobi , please, update the PR code according to the provided suggestions. Despite the message printed to kernel logs, the MAX22531_REG_PROD_ID read is clearly not successful, as we should get 0x0001 from that register. As we talked about in a previous meeting, drop the regmap implementation and make direct use of SPI transfers to comply with the protocol described on datasheet page 19 correctly. I'll test it again after you update the driver code and register access functions accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @KernelShinobi , the MAX22531 driver is looking pretty good overall.
Aside from a few suggestions of mine, I think there's not much else to do to get to a functional working driver.
I'll add another comment with the code for register access when I get it properly working.
Oh, one more thing, please, also run checkpatch to fix any leftover codestyle issue (if any). E.g.
./scripts/checkpatch.pl --terse --codespell --color=always --strict --file drivers/iio/adc/max22531.c
Cheers,
Marcelo
vddpl-supply: | ||
description: | ||
Isolated DC-DC converter power supply. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the "DO attempt to make bindings complete even if a driver doesn't support some features." guideline, we will also need to document the interrupt.
There should be plenty of examples of devices that can fire interrupts when readings surpass/undergo a threshold.
One device I recall having something similar is AD7091R-2/-4/-5/-8 (see Documentation/devicetree/bindings/iio/adc/adi,ad7091r5.yaml).
I get the impression max22531 interrupt will be a bit trickier to document since the interrupt may be triggered by conditions other than the threshold.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max22531 and max14001 are similar in the interrupts they can provide.
I think max22531's interrupts can be documented more or less like the following.
interrupts:
maxItems: 5
items:
- description: |
Interrupt for signaling when conversion results exceed the upper
threshold for ADC readings or fall below the lower threshold. The
interrupt source must be attached to one of COUT1 to COUT4 pins.
- description: |
Alert output that asserts low during a number of different error
conditions. The interrupt source must be attached to INT pin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
Hi @KernelShinobi , So, this is my suggestion for MAX22531 register read and write functions. #define MAX22531_REG_ADDR_MASK GENMASK(7, 2)
#define MAX22531_REG_WRITE_MASK BIT(1)
...
static int max22531_reg_read(struct max22531 *adc, unsigned int reg,
unsigned int *readval)
{
u8 cmd;
cmd = FIELD_PREP(MAX22531_REG_ADDR_MASK, reg);
*readval = spi_w8r16be(adc->spi_dev, cmd);
if (*readval < 0)
return *readval;
return 0;
}
static int max22531_reg_write(struct max22531 *adc, unsigned int reg,
unsigned int writeval)
{
u8 tx_buf[3];
tx_buf[0] = FIELD_PREP(MAX22531_REG_ADDR_MASK, reg) | MAX22531_REG_WRITE_MASK;
put_unaligned_be16(writeval, &tx_buf[1]);
return spi_write_then_read(adc->spi_dev, tx_buf, ARRAY_SIZE(tx_buf), NULL, 0);
} I've tested those with MAX22531_EVKIT_A, reading and writing from/to many different registers and the results met the expectations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @KernelShinobi,
The initial version of max22531 driver looks good overall.
It just needs to be updated according to the suggestions and I think it will be good to go to the mailing lists.
vddpl-supply: | ||
description: | ||
Isolated DC-DC converter power supply. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max22531 and max14001 are similar in the interrupts they can provide.
I think max22531's interrupts can be documented more or less like the following.
interrupts:
maxItems: 5
items:
- description: |
Interrupt for signaling when conversion results exceed the upper
threshold for ADC readings or fall below the lower threshold. The
interrupt source must be attached to one of COUT1 to COUT4 pins.
- description: |
Alert output that asserts low during a number of different error
conditions. The interrupt source must be attached to INT pin.
Add device tree overlay for MAX22531 device. Signed-off-by: Abhinav Jain <[email protected]>
5bd91fc
to
180f2a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @KernelShinobi ,
The patches look good.
Besides a few comments (most of which are about minor adjustments) there are two commit/patch related things that need to be changed.
(1) The dt-binding patch need to come before the driver code patch. The current order is opposite, meaning the driver code patch is being introduced before the dt-binding patch. Fixing the patch order will also help you fix the MAINTAINER file entry.
(2) The report about how the code was tested must not be in the commit log. It has to be stated in the cover letter and/or under the ---
of the patch file.
Before I forget, you'll also need to port the patches to the testing branch of the IIO tree.
Grab the IIO development tree.
git clone git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
cd iio
git fetch origin testing:testing
git checkout testing
then use git format-patch
and git am <patch>
to generate patches from your rpi tree and apply them to the IIO tree. You'll probably need to edit the patches to make them apply.
I have reversed the order and fixed conflicts. Will port to iio-testing and submit updates to this PR. |
180f2a9
to
d8ef1c4
Compare
Add device tree documentation for MAX22530-MAX22532 family of ADCs. The MAX22530–MAX22532 are galvanically isolated, 4-channel, multiplexed, 12-bit, analog-to-digital converters (ADC) in the MAXSafe™ family product line. An integrated, isolated, DC-DC converter powers all fieldside circuitry, and this allows field-side diagnostics even when no input signal is present. Signed-off-by: Abhinav Jain <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @KernelShinobi , the commits look better now.
A few last bits from me.
drivers/iio/adc/max22531.c
Outdated
|
||
adc = iio_priv(indio_dev); | ||
adc->spi_dev = spi; | ||
adc->chip_info = chip_info; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found this while testing but had forgotten to mention it
- adc->chip_info = chip_info;
+ adc->chip_info = spi_get_device_match_data(spi);
+ if (!adc->chip_info)
+ return dev_err_probe(&spi->dev, -EINVAL, "no chip info\n");
Otherwise we get a NullPointerException when probing the driver.
drivers/iio/adc/max22531.c
Outdated
|
||
static int max22531_probe(struct spi_device *spi) | ||
{ | ||
const struct max22531_chip_info *chip_info; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then this is not needed
drivers/iio/adc/max22531.c
Outdated
#define MAX22531_REG_ADC_CHAN(x) ((x) + 1) | ||
#define MAX22531_REG_FADC_CHAN(x) ((x) + 1) | ||
|
||
#define MAX22531_VREF_MV (18 * MILLI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oof, I guess I was not very awake when I suggested using MILLI here. That was a misleading suggestion.
(18 * MILLI) results in 18000, which I think is not the correct voltage reference in millivolts.
IIRC, the internal reference is 1.8 V, so revert the define back to 1800.
|
||
#include <linux/units.h> | ||
#include <linux/module.h> | ||
#include <asm/unaligned.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After porting the pacthes to the IIO tree, update this to #include <linux/unaligned.h>
.
-#include <asm/unaligned.h>
+#include <linux/unaligned.h>
No need to update this PR with a version of the driver from IIO tree. I think that would actually mess up the PR with potentially a lot of changes unrelated to max22531 driver. Update the driver according to comments, port the patches to IIO tree, generate patches from IIO, send the patches to yourself and optionally to me to check they are okay, then, everything being alright, send the patches to the mailing lists as explained by email and exemplified in our last meeting. |
Add device support for MAX22530-MAX22531. Implement scale and read functionality for raw/filtered ADC readings. Signed-off-by: Abhinav Jain <[email protected]>
d8ef1c4
to
ccc2372
Compare
I have submitted the final changes. For the header, I have kept |
Patch has been submitted based on the IIO tree: |
MAX22531 is a galvanically isolated, field-side self-powered, 4-channel, multiplexed, 12-bit ADC.
PR Description
Basic IIO ADC max22531 driver features:
PR Type
PR Checklist