drivers: adc: ad7124: Updating the Device IDs #2183
Conversation
D-Disha
requested review from
buha,
CiprianRegus,
dbogdan,
amiclaus,
rbolboac and
danmois
as code owners
drivers/adc/ad7124/ad7124.h
Outdated
| /* AD7124-4 Device IDs */ | ||
| #define AD7124_4_ID 0x06 | ||
| /* Chip ID for new silicon in AD7124-4 */ | ||
| #define AD7124_4_NEW_ID 0x07 |
There was a problem hiding this comment.
ID and NEW_ID naming isn't all that suggestive... what should the user choose? based on what?
I would prefer to have a suggestive naming, related maybe to hardware revision?
There was a problem hiding this comment.
The user per say is not choosing any ID, the intention is that if he uses a board with new silicon or the old silicon , the chip id read check should not fail.
In terms of the boards. It is only the silicon that has been revised. They will only replace the old AD7124 IC on the board with the new silicon before releasing it. I am not sure if they will call the newer boards with a new revision number or just replace the rev B boards with new silicon and still call it rev B boards. I will confirm this.
There was a problem hiding this comment.
I have checked with the Apps Engineer, the new silicon that is redesigned has no name as such and we can keep it as new_id
drivers/adc/ad7124/ad7124.h
Outdated
| #define AD7124_8_ID1 0x14 | ||
| #define AD7124_8_ID2 0x16 |
There was a problem hiding this comment.
what is ID1 and what is ID2 and how can the user differentiate between these? again, the naming should be more suggestive and the comment as well
There was a problem hiding this comment.
The ID can be either 0x14 / 0x16 for ad7124-8 older silicon
@rbolboac how can we better name this?
There was a problem hiding this comment.
I was referring to the 1 and 2 part. if the 1 is related to the old silicon and the 2 is related to the new silicon maybe there is a way to differentiate between them, like AD7124_8_ID_REV_0 and AD7124_8_ID_REV_A.
But I see there is also AD7124_8_NEW_ID. All of these values are compared against the read id_val but it is unclear to me what they actually represent (from chip version point of view).
There was a problem hiding this comment.
@rbolboac I found this information on ezone https://ez.analog.com/data_converters/precision_adcs/w/documents/15293/ad7124-4-8-standard-silicon-b-grade-and-w-grade
I would rename the ID1 and ID2 as standard and B/W_GRADE respectively
drivers/adc/ad7124/ad7124.c
Outdated
| if (id_val != AD7124_8_ID1 && id_val != AD7124_8_ID2 | ||
| && id_val != AD7124_8_NEW_ID) |
There was a problem hiding this comment.
I think a switch case would fit better here
drivers/adc/ad7124/ad7124.c
Outdated
| if (dev->active_device == ID_AD7124_4) { | ||
| if (dev->regs[AD7124_ID_REG].value != AD7124_4_ID) | ||
| if (id_val != AD7124_4_ID && id_val != AD7124_4_NEW_ID) |
There was a problem hiding this comment.
a switch case might fit better here, especially if there will be other 'new ids' in the future or other supported devices by the same driver
Removed comment to change device ID by user for new silicon and added macros for the same. Also updated the device ID checks in ad7124_setup function. Signed-off-by: Disha D <Disha.D@analog.com>
|
Hi all, are there any more comments on this PR or does it look good to merge? |
Removed comment to change device ID by user for new silicon and added macros for the same. Also updated the device ID checks in ad7124_setup function.
Pull Request Description
Please replace this with a detailed description and motivation of the changes.
You can tick the checkboxes below with an 'x' between square brackets or just check them after publishing the PR.
If this PR contains a breaking change, list dependent PRs and try to push all related PRs at the same time.
PR Type
PR Checklist