-
Notifications
You must be signed in to change notification settings - Fork 845
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
[GSoC 2024] Add support for AD7294 #2578
base: rpi-6.1.y
Are you sure you want to change the base?
Conversation
fragment@2 { | ||
target = <&ad7294>; | ||
__dormant__ { | ||
compatible = "adi,ad7294-2"; | ||
}; |
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.
This doesn't make much sense. You just introduced ad7294 node above. Why not have compatible = "adi,ad7294-2";
in ad7294 node? I suggest removing this fragment entirely.
#size-cells = <0>; | ||
status = "okay"; | ||
ad7294: ad7294@62 { | ||
compatible = "adi,ad7294"; |
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.
If you have AD7294-2 then just use the compatible for that directly.
compatible = "adi,ad7294-2";
drivers/iio/addac/Kconfig
Outdated
@@ -19,6 +19,16 @@ config AD74115 | |||
To compile this driver as a module, choose M here: the | |||
module will be called ad74115. | |||
|
|||
config AD7294 | |||
tristate "Analog Devices AD7294/AD7294-2 driver" |
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.
Nitpick: I would pick only one device to name the driver. For example, "AD7294 driver" or "AD7294-2 driver". Someone will see the driver supports both devices reading the help text or looking at the driver code.
#define AD7294_VOLTAGE_CHENNEL_COUNT 4 | ||
|
||
struct ad7294_state { | ||
struct mutex lock; |
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.
it is good practice to comment on what concurrency contention mechanisms protect. e.g.
struct mutex lock; /* Protect device write operations */
drivers/iio/addac/ad7294.c
Outdated
int ret; | ||
struct i2c_client *client = context; | ||
unsigned char buffer[3] = { reg }; | ||
int reg_size = ad7294_reg_size(reg); |
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.
Nitpick: Linux maintainers often ask to make local variable declarations in reverse xmas tree style.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst?h=v6.11-rc3#n650
So this would look better in the following order
struct i2c_client *client = context;
int reg_size = ad7294_reg_size(reg);
unsigned char buffer[3] = { reg };
int ret;
drivers/iio/addac/ad7294.c
Outdated
#define AD7294_VOLTAGE_CHAN(_type, _chan_id) { \ | ||
.type = _type, \ |
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.
Can a voltage channel be of a different type than voltage? If not, I'd suggest removing the _type
parameter and just having .type = IIO_VOLTAGE
within the macro.
Same for the current channel macro.
drivers/iio/addac/ad7294.c
Outdated
struct iio_dev *indio_dev = private; | ||
struct ad7294_state *st = iio_priv(indio_dev); | ||
unsigned int voltage_status, temp_status, current_status; | ||
s64 timestamp = iio_get_time_ns(indio_dev); |
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.
reverse fir/xmas tree order
IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i, | ||
IIO_EV_TYPE_THRESH, | ||
IIO_EV_DIR_FALLING), | ||
timestamp); |
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.
Nitpick: align timestamp
with the other arguments.
iio_push_event(indio_dev,
IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
IIO_EV_TYPE_THRESH,
IIO_EV_DIR_FALLING),
timestamp);
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.
timestamp
is actually the argument for iio_push_event
not the IIO_UNMOD_EVENT_CODE
macro, should it still be indented as like the arguments for IIO_UNMOD_EVENT_CODE
?
iio_push_event(indio_dev,
IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
IIO_EV_TYPE_THRESH,
IIO_EV_DIR_FALLING),
timestamp);
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.
oh, that's correct. Uncommon code pattern this function call has. Sure, forget what I said and keep timestamp
aligned with iio_push_event
.
drivers/iio/addac/ad7294.c
Outdated
goto adc_read; | ||
case IIO_VOLTAGE: | ||
if (chan->output) { | ||
*val = st->dac_value[chan->channel]; | ||
return IIO_VAL_INT; | ||
} | ||
adc_read: | ||
ret = regmap_write(st->regmap, AD7294_REG_CMD, | ||
BIT(chan->channel + base)); | ||
if (ret) | ||
return ret; | ||
ret = regmap_read(st->regmap, AD7294_REG_RESULT, | ||
®val); | ||
if (ret) | ||
return ret; | ||
*val = regval & AD7294_ADC_VALUE_MASK; | ||
return IIO_VAL_INT; |
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.
Jumping from one case to another with goto is uncommon in IIO.
I'd suggest put the common part of both cases into a separate function and call that when needed.
drivers/iio/addac/ad7294.c
Outdated
if (chan->output) { | ||
if (st->dac_vref_reg) { | ||
ret = regulator_get_voltage( | ||
st->dac_vref_reg); | ||
if (ret < 0) | ||
return ret; | ||
*val = ret / MILLI; | ||
} else { | ||
*val = AD7294_DAC_INTERNAL_VREF_MV; | ||
} | ||
} else { | ||
if (st->adc_vref_reg) { | ||
ret = regulator_get_voltage( | ||
st->adc_vref_reg); | ||
if (ret < 0) | ||
return ret; | ||
*val = ret / MILLI; | ||
} else { | ||
*val = AD7294_ADC_INTERNAL_VREF_MV; | ||
} | ||
} |
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.
It's so many levels of indentation here that you can't even get gain much in readability by breaking regulator_get_voltage()
call. Try do the scale read logic in a separate function to reduce overall indentation.
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.
Driver looks pretty good overall. There are some parts that can be improved (mainly code style stuff) and some other changes we discussed this morning but this is already a fairly comprehensive driver.
Keep up the good work @ArchUsr64 and this driver will soon be complete.
IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i, | ||
IIO_EV_TYPE_THRESH, | ||
IIO_EV_DIR_FALLING), | ||
timestamp); |
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.
oh, that's correct. Uncommon code pattern this function call has. Sure, forget what I said and keep timestamp
aligned with iio_push_event
.
drivers/iio/addac/ad7294.c
Outdated
#define AD7294_REG_DATA_HIGH(x) ((x) * 3 + 0x0C) | ||
#define AD7294_REG_HYSTERESIS(x) ((x) * 3 + 0x0D) | ||
|
||
#define AD7294_TEMP_ALERT_MASK GENMASK(6, 0) |
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.
If not handling overtemp alert (bit D6) this can be GENMASK(5, 0)
drivers/iio/addac/ad7294.c
Outdated
#define AD7294_ADC_INTERNAL_VREF_MV 2500 | ||
#define AD7294_DAC_INTERNAL_VREF_MV 2500 | ||
#define AD7294_RESOLUTION 12 | ||
#define AD7294_VOLTAGE_CHENNEL_COUNT 4 |
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.
Nitpick: chennel -> channel
#define AD7294_VOLTAGE_CHANNEL_COUNT 4
drivers/iio/addac/ad7294.c
Outdated
for_each_set_bit(i, (long *)&temp_status, AD7294_TEMP_ALERT_MASK) { | ||
if (i & 1) | ||
iio_push_event(indio_dev, | ||
IIO_UNMOD_EVENT_CODE(IIO_TEMP, i >> 1, | ||
IIO_EV_TYPE_THRESH, | ||
IIO_EV_DIR_RISING), | ||
timestamp); | ||
else | ||
iio_push_event(indio_dev, | ||
IIO_UNMOD_EVENT_CODE(IIO_TEMP, i >> 1, | ||
IIO_EV_TYPE_THRESH, | ||
IIO_EV_DIR_FALLING), | ||
timestamp); | ||
} |
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.
Not ideal to use the mask as for_each_set_bit()
size argument. Declare a define for max temperature channels (like you have for the voltage channels) and use that here. Also, to me, it looks like this part of the code doesn't really work. The bit
/i
local variable will have "the bit number for the next set bit" (0 (if D0), 1(if D1), 3 (if not D2 but D3 set), ...) so (i & 1)
will only be true if D0 is 1?
I'm wondering if it would be better to use a normal for
statement to traverse the bits from Alert Status Register B and Alert Status Register C. Declare macros similar to AD7294_ALERT_LOW()
to check the alert bit. The will be no need to shift i >> 1
and this gets more readable (in my opinion).
drivers/iio/addac/ad7294.c
Outdated
switch (chan->type) { | ||
case IIO_VOLTAGE: | ||
offset = chan->channel; | ||
break; | ||
case IIO_CURRENT: | ||
offset = chan->channel + 4; | ||
break; | ||
case IIO_TEMP: | ||
offset = chan->channel + 6; | ||
break; | ||
default: | ||
return 0; | ||
} | ||
|
||
switch (info) { | ||
case IIO_EV_INFO_VALUE: | ||
if (dir == IIO_EV_DIR_FALLING) | ||
return AD7294_REG_DATA_LOW(offset); | ||
else | ||
return AD7294_REG_DATA_HIGH(offset); | ||
case IIO_EV_INFO_HYSTERESIS: | ||
return AD7294_REG_HYSTERESIS(offset); |
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.
I see the effort to reuse code and how it allows concise read/write_event_value()
functions but these register offsets can look a bit mysterious at first glance. I would probably declare separate macros for those
#define AD7294_REG_VOLTAGE_DATA_LOW(x) ((x) * 3 + 0x0B)
#define AD7294_REG_VOLTAGE_DATA_HIGH(x) ((x) * 3 + 0x0C)
#define AD7294_REG_VOLTAGE_HYSTERESIS(x) ((x) * 3 + 0x0D)
#define AD7294_REG_CURRENT_DATA_LOW(x) ((x) * 3 + 0x17)
#define AD7294_REG_CURRENT_DATA_HIGH(x) ((x) * 3 + 0x18)
#define AD7294_REG_CURRENT_HYSTERESIS(x) ((x) * 3 + 0x19)
#define AD7294_REG_TEMP_DATA_LOW(x) ((x) * 3 + 0x1D)
#define AD7294_REG_TEMP_DATA_HIGH(x) ((x) * 3 + 0x1E)
#define AD7294_REG_TEMP_HYSTERESIS(x) ((x) * 3 + 0x1F)
so there would be no extra offset besides the ones used within the macros
switch (chan->type) {
case IIO_VOLTAGE:
switch (info) {
case IIO_EV_INFO_VALUE:
if (dir == IIO_EV_DIR_FALLING)
return AD7294_REG_VOLTAGE_DATA_LOW(chan->channel);
else
return AD7294_REG_VOLTAGE_DATA_HIGH(chan->channel);
case IIO_EV_INFO_HYSTERESIS:
return AD7294_REG_VOLTAGE_HYSTERESIS(chan->channel);
break;
case IIO_CURRENT:
switch (info) {
case IIO_EV_INFO_VALUE:
if (dir == IIO_EV_DIR_FALLING)
return AD7294_REG_CURRENT_DATA_LOW(chan->channel);
else
return AD7294_REG_CURRENT_DATA_HIGH(chan->channel);
case IIO_EV_INFO_HYSTERESIS:
return AD7294_REG_CURRENT_HYSTERESIS(chan->channel);
case IIO_TEMP:
switch (info) {
case IIO_EV_INFO_VALUE:
if (dir == IIO_EV_DIR_FALLING)
return AD7294_REG_TEMP_DATA_LOW(chan->channel);
else
return AD7294_REG_TEMP_DATA_HIGH(chan->channel);
case IIO_EV_INFO_HYSTERESIS:
return AD7294_REG_TEMP_HYSTERESIS(chan->channel);
break;
default:
return 0;
}
it becomes a bit longer, but it's clearer what register is being returned in each case.
if (IS_ERR(st->regmap)) | ||
return PTR_ERR(st->regmap); | ||
|
||
ret = regmap_read(st->regmap, AD7294_REG_PWDN, &pwdn_config); |
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.
We know the default value of power-down register on power up (0x70).
This read would not be needed if using the default value of power-down reg.
#define AD7294_REG_POWER_DOWN_DEFAULT 0x70
...
int pwdn_config = AD7294_REG_POWER_DOWN_DEFAULT;
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.
The default values varies at Bit 6 for AD7294 and AD7294-2, with default values of 0x60 and 0x70 respectively. The bit is reserved so functionality shouldn't be affected regardless. Should I go ahead with 0x70 as default values?
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.
Good point. Yeah their reset values are indeed different. Hmm, I would keep the more careful approach and avoid touching reserved bits.
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.
Hello @ArchUsr64
The AD7294 driver looks good. My comments now are mostly about minor improvements and code style nitpicking. There is actually only one important decision to make regarding the support for differential configuration (see comments).
With those last neats sorted, I see no reason not to approve and merge this.
This mentor has not been very responsive, but now that I have come back to this, I'll keep reviewing it until it gets good.
return AD7294_REG_VOLTAGE_DATA_LOW( | ||
chan->channel); |
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.
nitpick: Don't end lines with (
.
In this particular code snippet I think it's more readable to have
return AD7294_REG_VOLTAGE_DATA_LOW(chan->channel);
even if it exceeds 80 columns for a few characters.
Same to other cases in ad7294_threshold_reg() function.
run ./scripts/checkpatch.pl --terse --codespell --color=always -strict --file drivers/iio/addac/ad7294.c
to see the other ocurrences.
return AD7294_REG_VOLTAGE_DATA_HIGH( | ||
chan->channel); |
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.
return AD7294_REG_VOLTAGE_DATA_HIGH(chan->channel);
}, | ||
}; | ||
|
||
// clang-format off |
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.
What are these clang-format flags about? Are they really needed?
|
||
interrupts: | ||
maxItems: 1 | ||
description: Alert interrupt |
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.
improve the description with:
Interrupt for signaling when conversion results (either voltage, current, or temperature) exceed their high limit or fall below the low limit on a channel basis configuration. The interrupt source must be attached to the ALERT/BUSY pin.
st->regmap = | ||
devm_regmap_init(&i2c->dev, NULL, i2c, &ad7294_regmap_config); |
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.
nitpick: I would say the code looks better with this in one line.
st->regmap = devm_regmap_init(&i2c->dev, NULL, i2c, &ad7294_regmap_config);
Another option would be
st->regmap = devm_regmap_init(&i2c->dev, NULL, i2c,
&ad7294_regmap_config);
but also fine if you prefer to keep it the way it is now.
dac-vref-supply = <&dac_vref>; | ||
avdd-supply = <&avdd>; | ||
vdrive-supply = <&vdrive>; | ||
shunt-resistor-ohms = <1000 1000>; |
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.
This would come in the example
channel@0 {
reg = <0>;
};
channel@1 {
reg = <1>;
};
channel@2 {
reg = <2>;
diff-channels = <2 3>;
};
|
||
return regmap_write(st->regmap, | ||
ad7294_threshold_reg(chan, dir, IIO_EV_INFO_VALUE), | ||
state > 0 ? AD7294_ADC_VALUE_MASK : 0); |
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.
state ? AD7294_ADC_VALUE_MASK : 0
should also work
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ | ||
.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ |
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.
The 2Vref range can be set separately for each Vin channel in the Configuration Register. That allows each channel to have different vref which will lead to different _scale
for each channel. Let's use the separate mask for the scale too.
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
BIT(IIO_CHAN_INFO_SCALE), \
int ret = 0; | ||
|
||
guard(mutex)(&st->lock); | ||
ret = ad7294_set_alert_status(st, chan, dir, dir > 0); |
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.
The state
specifies whether the user wants to enable or disable IIO events for the channel.
Maybe pass that to the set function?
ret = ad7294_set_alert_status(st, chan, dir, state);
|
||
if (chan->output) { | ||
if (st->dac_vref_reg) { | ||
ret = regulator_get_voltage(st->dac_vref_reg); |
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.
I recall now that a more experienced developer told me once that the voltage provided by a regulator never really changes. So, the voltage reference could be read once during device probe, stored in the state struct, then reused here. Sorry to bring this up this late in the review process. I think it should also be fine to have it how it is now though.
Thanks for the review @machschmitt , I will make the requested changes in the next few commits. |
PR Description
TODO
PR Type
PR Checklist