drivers: adc: ti_k3: initial support #87462
Conversation
zephyrbot
added
platform: TI SimpleLink
zephyrbot
requested review from
anangl,
dnltz,
gramsay0,
vaishnavachath and
vanti
This patch adds the ADC driver for TI K3 family of SoCs. Technical reference can be found in the Technical Reference Manual (TRM) of the board. Signed-off-by: Amneesh Singh <a-singh7@ti.com>
|
is checkpatch.pl reporting false-negatives here? |
|
that was my first thought as well, that is likely, will read the script later |
|
|
||
| description: TI K3-family ADC | ||
|
|
||
| compatible: "ti,adc-k3" |
There was a problem hiding this comment.
I honestly see no pattern in compatible strings added by TI. The GPIO is called davince, I2C is called omap and the ADC is now called k3.
Linux documentation says [1] this ADC is compatible with AM335x and K3. So, I think this is just a general ADC block used in multiple chips. I don't think someone will boot Zephyr on AM335x anymore, but your next generation of chips will most likely use this block again. Probably looks odd when the k4 architecture uses ti,adc-k3.
It doesn't look like Zephyr supports multiple compatible strings per driver. So, I personally would just call it ti,tscadc.
1: https://www.kernel.org/doc/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
There was a problem hiding this comment.
I agree that the compatible strings are indeed confusing, but I don't think adding ti,tscadc is a good idea since that would imply a touchscreen controller as well. I am at a loss here but feel free to suggest more compatible strings, maybe "ti,adc-am335x" or "ti,am335x-adc" for linux compatibility is a good idea?
There was a problem hiding this comment.
@vaishnavachath any thoughts on that topic?
@natto1784 maybe ti,k3-adc is indeed the best name and future platforms just use compatible = "ti,k3-adc", "ti,k4-adc" in the device-tree. Similar what happened in Linux. Using am335 here might indicate there is actually support for that product. We also don't have to copy everything from Linux and could start fresh ... since neither the am335 nor am65 are supported.
It looks like that most compatible use <vendor>,<ip>-adc.
There was a problem hiding this comment.
@dnltz @natto1784 If the driver is present in Linux kernel then we try to match the bindings and compatible with corresponding ones in Linux wherever possible, this enables future reuse of device nodes directly from Linux DT. Since the IP is same my recommendation would be to reuse the same for the benefit of reusing nodes from Linux DT and scalling acrosss platforms with Linux support becomes easier (with some minor fixups for core specific irq connectivity .etc),
| #define ADC_K3_TOTAL_STEPS 16 | ||
|
|
||
| struct adc_k3_regs { | ||
| uint8_t RESERVED_1[0x28]; /**< Reserved, offset: 0xA0 - 0x28 */ |
There was a problem hiding this comment.
@natto1784 can you please replace all __IO with volatile. These _IO definitions are not c-standard and coming from the STM32 HAL.
This should also fix these checkpatch.pl warnings.
There was a problem hiding this comment.
It actually comes from the cmsis module but it is an HAL regardless so I think it is better to do as you say.
There was a problem hiding this comment.
I will update drivers/i2c/i2c_omap.c. I saw the I2C driver has an empty line in the register struct. Probably to get rid of this warning.
This patch adds the ADC driver for TI K3 family of SoCs. Technical reference can be found in the Technical Reference Manual (TRM) of the board.