iio: adc: add ADC driver for XuanTie TH1520 SoC
Subsystems (4)
[email protected]
maintainercover iio: adc: add ADC driver for XuanTie TH1520 SoC
From: Wei Fu <[email protected]>
This patchset adds initial support for XuanTie TH1520 ADC driver. This is modified from XuanTie TH1520 Linux_SDK_V1.4.2(linux v5.10.113) The original author is Fugang Duan <[email protected]>
Wei Fu (3):
drivers/iio/adc: Add XuanTie TH1520 ADC driver
riscv: dts: thead: Add XuanTie TH1520 ADC device node
dt-bindings: adc: Document XuanTie TH1520 ADC
.../bindings/iio/adc/thead,th1520.yaml | 52 ++ arch/riscv/boot/dts/thead/th1520.dtsi | 10 + drivers/iio/adc/Kconfig | 13 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/xuantie-th1520-adc.c | 572 ++++++++++++++++++ drivers/iio/adc/xuantie-th1520-adc.h | 193 ++++++ 6 files changed, 841 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml create mode 100644 drivers/iio/adc/xuantie-th1520-adc.c create mode 100644 drivers/iio/adc/xuantie-th1520-adc.h
Signature · 1 line
2.44.0
[email protected]
maintainerPATCH 1/3 drivers/iio/adc: Add XuanTie TH1520 ADC driver
From: Wei Fu <[email protected]>
--- drivers/iio/adc/Kconfig | 13 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/xuantie-th1520-adc.c | 572 +++++++++++++++++++++++++++ drivers/iio/adc/xuantie-th1520-adc.h | 193 +++++++++ 4 files changed, 779 insertions(+)
drivers/iio/adc/Kconfig +13 -0
@@ -1515,4 +1515,17 @@ config XILINX_AMS The driver can also be built as a module. If so, the module will be called xilinx-ams. +config XUANTIE_TH1520_ADC + tristate "XuanTie TH1520 ADC driver" + depends on OF + depends on HAS_IOMEM + depends on ARCH_THEAD + default y + help + Say yes here to support for XuanTie TH1520 MPW analog-to-digital + converter. + + This driver can also be built as a module. If so, the module will be + called xuantie-th1520-adc. + endmenu
drivers/iio/adc/Makefile +1 -0
@@ -135,4 +135,5 @@ obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o obj-$(CONFIG_XILINX_AMS) += xilinx-ams.o +obj-$(CONFIG_XUANTIE_TH1520_ADC) += xuantie-th1520-adc.o obj-$(CONFIG_SD_ADC_MODULATOR) += sd_adc_modulator.o
drivers/iio/adc/xuantie-th1520-adc.c new file +572 -0
572 lines not on this page · raw message
drivers/iio/adc/xuantie-th1520-adc.h new file +193 -0
193 lines not on this page · raw message
Signature · 1 line
2.44.0
Jonathan Cameron
maintainerRe: [PATCH 1/3] drivers/iio/adc: Add XuanTie TH1520 ADC driver
Quoted 3 lines · On Sat, 30 Mar 2024 04:01:24 +0800 [email protected] wrote:
From: Wei Fu <[email protected]>
Signed-off-by Wei Fu <[email protected]>
Hi Wei Fu,
Comments inline.
Main thing is that there is quite a bit of code here to support a more feature rich driver, but with parts missing. Rip all that out until those features are added in future patches.
Jonathan
Quoted 12 lines
--- drivers/iio/adc/Kconfig | 13 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/xuantie-th1520-adc.c | 572 +++++++++++++++++++++++++++ drivers/iio/adc/xuantie-th1520-adc.h | 193 +++++++++ 4 files changed, 779 insertions(+)drivers/iio/adc/Kconfig +0
-0
drivers/iio/adc/Kconfig config XILINX_AMS · line 1520
Quoted 6 lines
drivers/iio/adc/Kconfig +3
-0@@ -1515,4 +1515,17 @@ config XILINX_AMS The driver can also be built as a module. If so, the module will be called xilinx-ams. +config XUANTIE_TH1520_ADC + tristate "XuanTie TH1520 ADC driver" + depends on OF
No need for that dependency. In theory you could use this with ACPI PRP0001 for example.
drivers/iio/adc/Kconfig config XILINX_AMS · line 1522
Quoted 2 lines
drivers/iio/adc/Kconfig +2
-0+ depends on HAS_IOMEM + depends on ARCH_THEAD
|| COMPILE_TEST
to get us build coverage as I don't immediately see anything arch specific.
drivers/iio/adc/Kconfig config XILINX_AMS · line 1523
Quoted 1 line
drivers/iio/adc/Kconfig +1
-0+ default y
Don't set the default (i.e. rely on default default of no.)
Quoted 24 lines
drivers/iio/adc/Kconfig +7
-0+ help + Say yes here to support for XuanTie TH1520 MPW analog-to-digital + converter. + + This driver can also be built as a module. If so, the module will be + called xuantie-th1520-adc. + endmenudrivers/iio/adc/Makefile +1
-0@@ -135,4 +135,5 @@ obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o obj-$(CONFIG_XILINX_AMS) += xilinx-ams.o +obj-$(CONFIG_XUANTIE_TH1520_ADC) += xuantie-th1520-adc.o obj-$(CONFIG_SD_ADC_MODULATOR) += sd_adc_modulator.odrivers/iio/adc/xuantie-th1520-adc.c new file +0
-0
drivers/iio/adc/xuantie-th1520-adc.c line 7
Quoted 7 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +7
-0@@ -0,0 +1,572 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* +* XuanTie TH1520 ADC driver +* +* Copyright (C) 2021-2024 Alibaba Group Holding Limited. +* Fugang Duan <[email protected]> +*
Blank line here not useful so drop it.
Quoted 4 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +4
-0+*/ + +#include <linux/module.h> +#include <linux/platform_device.h>
drivers/iio/adc/xuantie-th1520-adc.c line 21
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+#include <linux/interrupt.h> +#include <linux/delay.h> +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/io.h> +#include <linux/iopoll.h> +#include <linux/clk.h> +#include <linux/completion.h> +#include <linux/of.h> +#include <linux/of_irq.h>
As below. Unlikely these are needed in a driver.
drivers/iio/adc/xuantie-th1520-adc.c line 23
Quoted 2 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +2
-0+#include <linux/regulator/consumer.h> +#include <linux/of_platform.h>
Doubt you need that. mod_devicetable.h perhaps?
drivers/iio/adc/xuantie-th1520-adc.c line 27
Quoted 4 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +4
-0+#include <linux/err.h> + +#include <linux/iio/iio.h> +#include <linux/iio/buffer.h>
Why?
Quoted 11 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +11
-0+#include <linux/iio/sysfs.h> + +#include "xuantie-th1520-adc.h" + +static inline void th1520_adc_cfg_init (struct th1520_adc *info) +{ +struct th1520_adc_feature *adc_feature = &info->adc_feature; + +/* set default Configuration for ADC controller */ + adc_feature->selres_sel =TH1520_ADC_SELRES_12BIT ; + adc_feature->input_mode =TH1520_ADC_SINGLE_ENDED_INPUTS ;
drivers/iio/adc/xuantie-th1520-adc.c line 48
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+ adc_feature->vol_ref =TH1520_ADC_VOL_VREF ; + adc_feature->offset_mode =TH1520_ADC_OFFSET_DIS ; + adc_feature->conv_mode =TH1520_ADC_MODE_SINGLE ; + adc_feature->clk_sel =TH1520_ADC_FCLK_TYP_1M ; + + adc_feature->int_actual =TH1520_ADC_ACTUAL_ALL ; + adc_feature->int_detal =TH1520_ADC_DETAL_ALL ; + + info->ch0_offmeas =0 ; + info->ch1_offmeas =0 ;
Bring these features in when you add support to the driver. Until then stick just using the default values inline as it simplifies the code.
drivers/iio/adc/xuantie-th1520-adc.c line 57
Quoted 9 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +9
-0+} + +static void th1520_adc_reg_set (struct th1520_adc *info) +{ +u32 phy_cfg =0 ; +u32 op_ctrl =0 ; +struct th1520_adc_feature *adc_feature = &info->adc_feature; + +/* phy_cfg */
That's obvious from the code, so I'd drop the comment.
drivers/iio/adc/xuantie-th1520-adc.c line 59
Quoted 2 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +2
-0+switch (adc_feature->selres_sel) { +case TH1520_ADC_SELRES_6BIT :
Use numbers here. You could just use maths for the conversion as well to simplify things further. However I've suggested elsewhere that you just always use 12bit in which case this complexity isn't needed.
drivers/iio/adc/xuantie-th1520-adc.c line 71
Quoted 12 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +12
-0+ phy_cfg |=TH1520_ADC_PHY_CFG_SELRES_6BIT ; +break ; +case TH1520_ADC_SELRES_8BIT : + phy_cfg |=TH1520_ADC_PHY_CFG_SELRES_8BIT ; +break ; +case TH1520_ADC_SELRES_10BIT : + phy_cfg |=TH1520_ADC_PHY_CFG_SELRES_10BIT ; +break ; +case TH1520_ADC_SELRES_12BIT : + phy_cfg |=TH1520_ADC_PHY_CFG_SELRES_12BIT ; +break ; +default :
It's an enum, so we shouldn't get here with anything else and can drop the default.
drivers/iio/adc/xuantie-th1520-adc.c line 82
Quoted 11 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +11
-0+break ; + } + +switch (adc_feature->input_mode) { +case TH1520_ADC_SINGLE_ENDED_INPUTS : + phy_cfg |=TH1520_ADC_PHY_CFG_SELDIFF_SINGLE_ENDED_INPUTS ; +break ; +case TH1520_ADC_DIFFERENTIAL_INPUTS : + phy_cfg |=TH1520_ADC_PHY_CFG_SELDIFF_DIFFERENTIAL_INPUTS ; +break ; +default :
Not need as all values covered.
drivers/iio/adc/xuantie-th1520-adc.c line 95
Quoted 13 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +13
-0+break ; + } + +switch (adc_feature->vol_ref) { +case TH1520_ADC_VOL_VREF : + phy_cfg |=TH1520_ADC_PHY_CFG_SELBG_EXTERNAL | +TH1520_ADC_PHY_CFG_SELREF_EXT ; +break ; +case TH1520_ADC_VOL_INTE : + phy_cfg |=TH1520_ADC_PHY_CFG_SELBG_INTERNAL | +TH1520_ADC_PHY_CFG_SELREF_INTERNAL ; +break ; +default :
Probably also not needed.
drivers/iio/adc/xuantie-th1520-adc.c line 107
Quoted 12 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +12
-0+break ; + } + +/* op_ctrl */ +switch (adc_feature->conv_mode) { +case TH1520_ADC_MODE_SINGLE : + op_ctrl |=TH1520_ADC_OP_CTRL_MODE_SINGLE ; +break ; +case TH1520_ADC_MODE_CONTINOUS : + op_ctrl |=TH1520_ADC_OP_CTRL_MODE_CONTINOUS ; +break ; +default :
As above.
drivers/iio/adc/xuantie-th1520-adc.c line 111
Quoted 4 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +4
-0+break ; + } + +writel (phy_cfg, info->regs +TH1520_ADC_PHY_CFG );
I'd keep the writes with the register setup, so move this up above filling in op_ctrl value.
Quoted 15 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +15
-0+writel (op_ctrl, info->regs +TH1520_ADC_OP_CTRL ); + +writel (TH1520_ADC_PHY_ENCTR , info->regs +TH1520_ADC_PHY_TEST ); + +/* disable the irq */ +writel (0xff , info->regs +TH1520_ADC_INT_CTRL1 ); +writel (0xff , info->regs +TH1520_ADC_INT_CTRL2 ); + +if (adc_feature->conv_mode ==TH1520_ADC_MODE_CONTINOUS ) +writel (TH1520_ADC_PHY_CTRL_ENADC_EN , + info->regs +TH1520_ADC_PHY_CTRL ); +} + +static void th1520_adc_fclk_set (struct th1520_adc *info) +{
drivers/iio/adc/xuantie-th1520-adc.c line 136
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+int fclk_ctrl =0 ; +int start_time =0 ; +int sample_time =0 ; +struct th1520_adc_feature *adc_feature = &info->adc_feature; + +switch (adc_feature->clk_sel) { +case TH1520_ADC_FCLK_TYP_1M : + fclk_ctrl =TH1520_ADC_FCLK_CTRL_TYP_1M ; + start_time =TH1520_ADC_START_TIME_TYP_1M ; +if (adc_feature->selres_sel ==TH1520_ADC_SELRES_6BIT )
Use numbers. Also maybe just convert from resolution to register value using maths rather than an if/else stack. Otherwise, you could just have an array mapping from register value to real value and search it.
Quoted 4 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +4
-0+ sample_time =TH1520_ADC_SAMPLE_TIME_TYP_6BIT ; + else if (adc_feature->selres_sel == TH1520_ADC_SELRES_8BIT) + sample_time =TH1520_ADC_SAMPLE_TIME_TYP_8BIT ; + else if (adc_feature->selres_sel == TH1520_ADC_SELRES_10BIT)
drivers/iio/adc/xuantie-th1520-adc.c line 150
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+ sample_time =TH1520_ADC_SAMPLE_TIME_TYP_10BIT ; + else if (adc_feature->selres_sel == TH1520_ADC_SELRES_12BIT) + sample_time =TH1520_ADC_SAMPLE_TIME_TYP_12BIT ; + else { +pr_err ("[%s,%d]invalid selres select\n" , + __func__, __LINE__); +return ; + } +break ; +default :
Comment on why nothing to do here.
Quoted 13 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +13
-0+break ; + } +writel (fclk_ctrl, info->regs +TH1520_ADC_FCLK_CTRL ); +writel (start_time, info->regs +TH1520_ADC_START_TIME ); +writel (sample_time, info->regs +TH1520_ADC_SAMPLE_TIME ); +} + +static void th1520_adc_hw_init (struct th1520_adc *info) +{ +th1520_adc_reg_set (info); +th1520_adc_fclk_set (info); +} +
drivers/iio/adc/xuantie-th1520-adc.c line 173
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+static const struct iio_chan_spec th1520_adc_iio_channels[] = { +TH1520_ADC_CHAN (0 ,IIO_VOLTAGE ), +TH1520_ADC_CHAN (1 ,IIO_VOLTAGE ), +TH1520_ADC_CHAN (2 ,IIO_VOLTAGE ), +TH1520_ADC_CHAN (3 ,IIO_VOLTAGE ), +TH1520_ADC_CHAN (4 ,IIO_VOLTAGE ), +TH1520_ADC_CHAN (5 ,IIO_VOLTAGE ), +TH1520_ADC_CHAN (6 ,IIO_VOLTAGE ), +TH1520_ADC_CHAN (7 ,IIO_VOLTAGE ), +/* sentinel */
? There isn't a sentinel so drop the comment.
drivers/iio/adc/xuantie-th1520-adc.c line 180
Quoted 7 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +7
-0+}; + +static irqreturn_t th1520_adc_isr (int irq,void *dev_id) +{ +struct iio_dev *indio_dev = dev_id; +struct th1520_adc *info =iio_priv (indio_dev); +/* TBD */
? Shouldn't see a TBD in a submitted driver.
Quoted 4 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +4
-0+complete (&info->completion); +return IRQ_HANDLED ; +} +
drivers/iio/adc/xuantie-th1520-adc.c line 194
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+static int th1520_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2,long mask) +{ +int tmp; +long ret; +struct th1520_adc *info =iio_priv (indio_dev); + +switch (mask) { +case IIO_CHAN_INFO_RAW :
Could use guard() here though the saving in this case is monor.
drivers/iio/adc/xuantie-th1520-adc.c line 197
Quoted 3 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +3
-0+mutex_lock (&info->mlock); + +if (info->adc_feature.conv_mode ==TH1520_ADC_MODE_SINGLE ) {
At the moment there is no control over this mode, so I would drop the alternative path until you add such control. Also drop even tracking what that mode is set to as it won't change.
drivers/iio/adc/xuantie-th1520-adc.c line 208
Quoted 11 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +11
-0+uint ievent; +uint vld_flag; +uint phy_ctrl; +uint real_chan; +uint op_ctrl =0 ; +uint single_retry =TH1520_ADC_FIFO_DATA_SIZE ; + + op_ctrl =readl (info->regs +TH1520_ADC_OP_CTRL ); + op_ctrl &= ~TH1520_ADC_OP_CTRL_CH_EN_ALL ; + op_ctrl |= (BIT (chan->channel +TH1520_ADC_OP_CTRL_CH_EN_0 ) & +TH1520_ADC_OP_CTRL_CH_EN_ALL );
FIELD_PREP()
drivers/iio/adc/xuantie-th1520-adc.c line 209
Quoted 1 line
drivers/iio/adc/xuantie-th1520-adc.c new file +1
-0+writel (op_ctrl, info->regs +TH1520_ADC_OP_CTRL );
This first block is common to both paths. I'd factor out as much as possible
drivers/iio/adc/xuantie-th1520-adc.c line 216
Quoted 7 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +7
-0+ +writel (TH1520_ADC_PHY_CTRL_ENADC_EN , + info->regs +TH1520_ADC_PHY_CTRL ); + + vld_flag =TH1520_ADC_SAMPLE_DATA_CH0_VLD ; + +while (single_retry--) {
Add a common to justify the retries (particularly why this number of them).
This deep indentation suggests to me that some of this code should be factored out into a separate function.
drivers/iio/adc/xuantie-th1520-adc.c line 226
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+writel (TH1520_ADC_OP_SINGLE_START_EN , + info->regs +TH1520_ADC_OP_SINGLE_START ); +/* wait the sampling result */ + ret =readl_poll_timeout (info->regs + +TH1520_ADC_SAMPLE_DATA , + ievent, + ievent & vld_flag,100 , +TH1520_ADC_TIMEOUT ); +if (ret) +pr_err ("wait the sampling timeout\n" );
Given you are going to retry, shouldn't see an error message here.
drivers/iio/adc/xuantie-th1520-adc.c line 230
Quoted 4 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +4
-0+ + real_chan = + (ievent &TH1520_ADC_SAMPLE_DATA_CH0_NUMBER ) >> +TH1520_ADC_SAMPLE_DATA_CH0_NUMBER_OFF ;
FIELD_GET()
drivers/iio/adc/xuantie-th1520-adc.c line 241
Quoted 11 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +11
-0+if (real_chan == chan->channel) +break ; + } + + phy_ctrl =readl (info->regs +TH1520_ADC_PHY_CTRL ); + phy_ctrl &= ~TH1520_ADC_PHY_CTRL_ENADC_EN ; +writel (phy_ctrl, info->regs +TH1520_ADC_PHY_CTRL ); + +/* read the sampling data */ + *val = (ievent &TH1520_ADC_SAMPLE_DATA_CH0 ) >> +TH1520_ADC_SAMPLE_DATA_CH0_OFF ;
FIELD_GET()
Quoted 44 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +44
-0+ }else { +uint ievent; +uint vld_flag; +uint op_single; +uint op_ctrl =0 ; + + op_ctrl =readl (info->regs +TH1520_ADC_OP_CTRL ); + op_ctrl &= ~TH1520_ADC_OP_CTRL_CH_EN_ALL ; + op_ctrl |= (BIT (chan->channel +TH1520_ADC_OP_CTRL_CH_EN_0 ) & +TH1520_ADC_OP_CTRL_CH_EN_ALL ); +writel (op_ctrl, info->regs +TH1520_ADC_OP_CTRL ); + + op_single =readl (info->regs + +TH1520_ADC_OP_SINGLE_START ); + op_single &= ~TH1520_ADC_OP_SINGLE_START_EN ; +writel (op_single, + info->regs +TH1520_ADC_OP_SINGLE_START ); + + vld_flag =TH1520_ADC_SAMPLE_DATA_CH0_VLD | +TH1520_ADC_SAMPLE_DATA_CH1_VLD ; + +/* wait the sampling result */ + ret =readl_poll_timeout (info->regs + +TH1520_ADC_SAMPLE_DATA , + ievent, ievent & vld_flag,10 , +TH1520_ADC_TIMEOUT ); +if (ret) +pr_err ("wait the sampling timeout\n" ); + +/* read the sampling data */ + tmp =readl (info->regs +TH1520_ADC_SAMPLE_DATA ); +if (tmp &TH1520_ADC_SAMPLE_DATA_CH0_VLD ) + *val = (tmp &TH1520_ADC_SAMPLE_DATA_CH0 ) >> +TH1520_ADC_SAMPLE_DATA_CH0_OFF ; +else + *val = (tmp &TH1520_ADC_SAMPLE_DATA_CH1 ) >> +TH1520_ADC_SAMPLE_DATA_CH1_OFF ; + } + +mutex_unlock (&info->mlock); +return IIO_VAL_INT ; + +case IIO_CHAN_INFO_SCALE : + *val = info->vref_uv /1000 ;
drivers/iio/adc/xuantie-th1520-adc.c line 295
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+ *val2 = info->adc_feature.selres_sel; +return IIO_VAL_FRACTIONAL_LOG2 ; + +case IIO_CHAN_INFO_SAMP_FREQ : + *val = info->current_clk; + *val2 =0 ; +return IIO_VAL_INT ; + +default : +break ;
return -EINVAL; then drop the one below.
Quoted 43 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +42
-0+ } + +return -EINVAL ; +} + +static ssize_t th1520_adc_res_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +size_t bufpos =0 , count =5 ; +struct platform_device *pdev =to_platform_device (dev); +struct iio_dev *indio_dev =platform_get_drvdata (pdev); +struct th1520_adc *info =iio_priv (indio_dev); + +snprintf (buf + bufpos, count - bufpos,"%.*x: " ,4 , + info->adc_feature.selres_sel); + bufpos +=4 ; + buf[bufpos++] ='\n' ; + +return bufpos; +} + +static ssize_t th1520_adc_res_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t size) +{ +unsigned long res; +char *start = (char *)buf; +struct platform_device *pdev =to_platform_device (dev); +struct iio_dev *indio_dev =platform_get_drvdata (pdev); +struct th1520_adc *info =iio_priv (indio_dev); + +if (kstrtoul (start,0 , &res)) +return -EINVAL ; + +switch (res) { +case TH1520_ADC_SELRES_6BIT : +case TH1520_ADC_SELRES_8BIT : +case TH1520_ADC_SELRES_10BIT : +case TH1520_ADC_SELRES_12BIT : + info->adc_feature.selres_sel = res; +th1520_adc_reset (info); +th1520_adc_hw_init (info);
drivers/iio/adc/xuantie-th1520-adc.c line 403
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+break ; +default : +dev_err (dev,"not support res\n" ); +return -EINVAL ; + } + +return size; +} + +static DEVICE_ATTR_RW (th1520_adc_res);
Custom ABI needs documentation. We very rarely allow direct control of resolution as it normally also affects scale which is standard ABI.
Also, for drivers only providing polled reads, the saving in time of using low res readouts it negligible so just use 12bit ever time.
Quoted 4 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +4
-0+ +static int th1520_adc_probe (struct platform_device *pdev) +{ +int irq;
drivers/iio/adc/xuantie-th1520-adc.c line 417
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+int ret; +struct resource *mem; +struct th1520_adc *info; +struct iio_dev *indio_dev; + + indio_dev =devm_iio_device_alloc (&pdev->dev, +sizeof (struct th1520_adc )); +if (!indio_dev) { +dev_err (&pdev->dev,"Failed allocating iio device\n" ); +return -ENOMEM ;
dev_err_probe() here as well.
Quoted 4 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +4
-0+ } + + info =iio_priv (indio_dev); + info->dev = &pdev->dev;
drivers/iio/adc/xuantie-th1520-adc.c line 431
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+ + irq =platform_get_irq (pdev,0 ); +if (irq <0 ) +return irq; + + ret =devm_request_irq (info->dev, irq, th1520_adc_isr,0 , +dev_name (&pdev->dev), indio_dev); +if (ret <0 ) { +dev_err (&pdev->dev,"failed requesting irq, irq = %d\n" , irq); +return ret;
Use return dev_err_probe() for errors in probe() routines and functions that are only called during probe. It better handles deferred probing and also gives shorter code in general.
Quoted 13 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +13
-0+ } + + mem =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + info->regs =devm_ioremap_resource (&pdev->dev, mem); +if (IS_ERR (info->regs)) +return PTR_ERR (info->regs); + + info->clk =devm_clk_get (&pdev->dev,"adc" ); +if (IS_ERR (info->clk)) { +dev_err (&pdev->dev,"failed getting clock, err = %ld\n" , +PTR_ERR (info->clk)); +return PTR_ERR (info->clk); + }
drivers/iio/adc/xuantie-th1520-adc.c line 454
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+ + info->vref =devm_regulator_get (&pdev->dev,"vref" ); +if (IS_ERR (info->vref)) +return PTR_ERR (info->vref); + + ret =regulator_enable (info->vref); +if (ret) +return ret; + + info->vref_uv =regulator_get_voltage (info->vref);
There is a patch set on list that will replace this boiler plate with a single call to enable the regulator and get it's voltage, but we can make use of that once/if that series is accepted.
drivers/iio/adc/xuantie-th1520-adc.c line 463
Quoted 9 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +9
-0+ +platform_set_drvdata (pdev, indio_dev); + +mutex_init (&info->mlock); +init_completion (&info->completion); + + indio_dev->name =dev_name (&pdev->dev); + indio_dev->dev.parent = &pdev->dev; + indio_dev->dev.of_node = pdev->dev.of_node;
Both parent and of_node are set in the IIO core. I doubt you need to override them.
Quoted 13 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +13
-0+ indio_dev->info = &th1520_adc_iio_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = th1520_adc_iio_channels; + indio_dev->num_channels =ARRAY_SIZE (th1520_adc_iio_channels); + + ret =clk_prepare_enable (info->clk); +if (ret) { +dev_err (&pdev->dev, +"Could not prepare or enable the clock.\n" ); + goto error_adc_clk_enable; + } + +th1520_adc_cfg_init (info);
drivers/iio/adc/xuantie-th1520-adc.c line 486
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +10
-0+th1520_adc_reset (info); +th1520_adc_hw_init (info); + + ret =iio_device_register (indio_dev); +if (ret) { +dev_err (&pdev->dev,"Couldn't register the device.\n" ); + goto error_iio_device_register; + } + + ret =sysfs_create_file (&pdev->dev.kobj, &dev_attr_th1520_adc_res.attr);
This is very much the wrong way to do sysfs attributes in general, but I doubt this will end up in the final driver anyway so I won't go into how this should be done. Adding a file after registration is racey with userspace.
drivers/iio/adc/xuantie-th1520-adc.c line 492
Quoted 6 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +6
-0+if (ret) { +dev_err (&pdev->dev,"Failed to create adc debug sysfs.\n" ); + goto error_iio_device_register; + } + +dev_info (&pdev->dev,"XuanTie TH1520 adc registered.\n" );
Too noisy. There are loads of ways of finding that out that don't need a message in the kernel log. If you really want something then dev_dbg() at most.
Quoted 13 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +13
-0+return 0 ; + +error_iio_device_register: +clk_disable_unprepare (info->clk); +error_adc_clk_enable: +regulator_disable (info->vref); + +return ret; +} + +static int th1520_adc_remove (struct platform_device *pdev) +{ +struct iio_dev *indio_dev =platform_get_drvdata (pdev);
drivers/iio/adc/xuantie-th1520-adc.c line 556
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +9
-0+struct th1520_adc *info =iio_priv (indio_dev); + +iio_device_unregister (indio_dev); +regulator_disable (info->vref); +clk_disable_unprepare (info->clk); + +return 0 ; +} +static SIMPLE_DEV_PM_OPS(th1520_adc_pm_ops,
IIRC this is deprecated use.DEFINE_SIMPLE_DEV_PM_OPS()
drivers/iio/adc/xuantie-th1520-adc.c line 563
Quoted 7 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +7
-0+ th1520_adc_suspend, th1520_adc_resume); + +static struct platform_driver th1520_adc_driver = { + .probe = th1520_adc_probe, + .remove = th1520_adc_remove, + .driver = { + .name =DRIVER_NAME ,
Put the string here and don't use a define.
drivers/iio/adc/xuantie-th1520-adc.c line 565
Quoted 2 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +2
-0+ .of_match_table = th1520_adc_match, + .pm = &th1520_adc_pm_ops,
with the above, pm_sleep_ptr() around this and then if you build without PM support the unused code will be removed by the compiler.
Quoted 16 lines
drivers/iio/adc/xuantie-th1520-adc.c new file +7
-0+ }, +}; +module_platform_driver (th1520_adc_driver); + +MODULE_AUTHOR ("fugang.duan <[email protected]>" ); +MODULE_DESCRIPTION ("XuanTie TH1520 ADC driver" ); +MODULE_LICENSE ("GPL" );drivers/iio/adc/xuantie-th1520-adc.h new file +3
-0@@ -0,0 +1,193 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* +* XuanTie TH1520 ADC driver
drivers/iio/adc/xuantie-th1520-adc.h line 13
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +10
-0+* +* Copyright (C) 2021-2024 Alibaba Group Holding Limited. +* Fugang Duan <[email protected]> +* +*/ + +#include <linux/bitops.h> + +/* This will be the driver name the kernel reports */ +#define DRIVER_NAME "xuantie-th1520-adc"
No obvious reason for a header - push this down into the C file.
Quoted 23 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +23
-0+ +/* ADC registers */ +#define TH1520_ADC_PHY_CFG 0x00 +#define TH1520_ADC_PHY_CTRL 0x04 +#define TH1520_ADC_PHY_TEST 0x08 +#define TH1520_ADC_OP_CTRL 0x0C +#define TH1520_ADC_OP_SINGLE_START 0x10 +#define TH1520_ADC_FCLK_CTRL 0x14 +#define TH1520_ADC_START_TIME 0x18 +#define TH1520_ADC_SAMPLE_TIME 0x1C +#define TH1520_ADC_SAMPLE_DATA 0x20 +#define TH1520_ADC_INT_CTRL1 0x50 +#define TH1520_ADC_INT_CTRL2 0x54 +#define TH1520_ADC_INT_STATUS 0x58 +#define TH1520_ADC_INT_ACTUAL_VALUE_CH0 0x60 +#define TH1520_ADC_INT_ACTUAL_VALUE_CH1 0x64 +#define TH1520_ADC_INT_DELTA_VALUE_CH0 0x90 +#define TH1520_ADC_INT_DELTA_VALUE_CH1 0x94 + +/* Configuration register field define */ +#define TH1520_ADC_PHY_CFG_SELRES_6BIT (0x0) +#define TH1520_ADC_PHY_CFG_SELRES_8BIT (0x1) +#define TH1520_ADC_PHY_CFG_SELRES_10BIT (0x2)
drivers/iio/adc/xuantie-th1520-adc.h line 46
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +10
-0+#define TH1520_ADC_PHY_CFG_SELRES_12BIT (0x3) +#define TH1520_ADC_PHY_CFG_SELDIFF_SINGLE_ENDED_INPUTS (0x0 << 4) +#define TH1520_ADC_PHY_CFG_SELDIFF_DIFFERENTIAL_INPUTS (0x1 << 4) +#define TH1520_ADC_PHY_CFG_SELBG_INTERNAL (0x1 << 8) +#define TH1520_ADC_PHY_CFG_SELBG_EXTERNAL (0x0 << 8) +#define TH1520_ADC_PHY_CFG_SELREF_INTERNAL (0x1 << 12) +#define TH1520_ADC_PHY_CFG_SELREF_EXT (0x0 << 12) + +/* PHY CTRL register field define */ +#define TH1520_ADC_PHY_CTRL_ENOFFSET_EN (0x1 << 12)
BIT()
Quoted 50 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +50
-0+#define TH1520_ADC_PHY_CTRL_ENOFFMEAS_EN (0x1 << 8) +#define TH1520_ADC_PHY_CTRL_RST_EN (0x1 << 4) +#define TH1520_ADC_PHY_CTRL_ENADC_EN (0x1 << 0) + +/* ADC OP ctrl field define */ +#define TH1520_ADC_OP_CTRL_CH_EN_ALL GENMASK(19, 12) +#define TH1520_ADC_OP_CTRL_CH_EN_0 (12) +#define TH1520_ADC_OP_CTRL_MODE_SINGLE (0x1 << 0) +#define TH1520_ADC_OP_CTRL_MODE_CONTINOUS (0x0 << 0) + +/* ADC OP single start */ +#define TH1520_ADC_OP_SINGLE_START_EN BIT(0) + +/* ADC fclk ctrl */ +#define TH1520_ADC_FCLK_CTRL_FCLLK_DIV GENMASK(6, 0) +#define TH1520_ADC_FCLK_CTRL_TYP_1M (0x10004) +#define TH1520_ADC_START_TIME_TYP_1M (0x160) +#define TH1520_ADC_SAMPLE_TIME_TYP_1M (0x10) +#define TH1520_ADC_SAMPLE_TIME_TYP_6BIT (8) +#define TH1520_ADC_SAMPLE_TIME_TYP_8BIT (10) +#define TH1520_ADC_SAMPLE_TIME_TYP_10BIT (12) +#define TH1520_ADC_SAMPLE_TIME_TYP_12BIT (14) + +/* ADC sample data */ +#define TH1520_ADC_SAMPLE_DATA_CH1 GENMASK(27, 16) +#define TH1520_ADC_SAMPLE_DATA_CH1_OFF (16) +#define TH1520_ADC_SAMPLE_DATA_CH1_VLD BIT(31) +#define TH1520_ADC_SAMPLE_DATA_CH1_NUMBER GENMASK(30, 28) +#define TH1520_ADC_SAMPLE_DATA_CH1_NUMBER_OFF (28) +#define TH1520_ADC_SAMPLE_DATA_CH0 GENMASK(11, 0) +#define TH1520_ADC_SAMPLE_DATA_CH0_VLD BIT(15) +#define TH1520_ADC_SAMPLE_DATA_CH0_OFF (0) +#define TH1520_ADC_SAMPLE_DATA_CH0_NUMBER GENMASK(14, 12) +#define TH1520_ADC_SAMPLE_DATA_CH0_NUMBER_OFF (12) + +/* ADC INT Ctrl */ +#define TH1520_ADC_INT_CTRL1_CH1_INT_MODE BIT(1) +#define TH1520_ADC_INT_CTRL1_CH0_INT_MODE BIT(0) +#define TH1520_ADC_INT_CTRL2_CH1_INT_MASK BIT(1) +#define TH1520_ADC_INT_CTRL2_CH0_INT_MASK BIT(0) +#define TH1520_ADC_INT_STS_CH1_INT_STS BIT(1) +#define TH1520_ADC_INT_STS_CH0_INT_STS BIT(0) + +#define TH1520_ADC_ACTUAL_VALUE_CH0_HVAL GENMASK(27, 16) +#define TH1520_ADC_ACTUAL_VALUE_CH0_HVAL_OFF (16) +#define TH1520_ADC_ACTUAL_VALUE_CH0_LVAL GENMASK(11, 0) +#define TH1520_ADC_ACTUAL_VALUE_CH0_LVAL_OFF (0) +#define TH1520_ADC_ACTUAL_VALUE_CH1_HVAL GENMASK(27, 16) +#define TH1520_ADC_ACTUAL_VALUE_CH1_HVAL_OFF (16) +#define TH1520_ADC_ACTUAL_VALUE_CH1_LVAL GENMASK(11, 0)
drivers/iio/adc/xuantie-th1520-adc.h line 106
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +10
-0+#define TH1520_ADC_ACTUAL_VALUE_CH1_LVAL_OFF (0) + +#define TH1520_ADC_DLT_VALUE_CH0_HVAL GENMASK(27, 16) +#define TH1520_ADC_DLT_VALUE_CH0_HVAL_OFF (16) +#define TH1520_ADC_DLT_VALUE_CH0_LVAL GENMASK(11, 0) +#define TH1520_ADC_DLT_VALUE_CH0_LVAL_OFF (0) +#define TH1520_ADC_DLT_VALUE_CH1_HVAL GENMASK(27, 16) +#define TH1520_ADC_DLT_VALUE_CH1_HVAL_OFF (16) +#define TH1520_ADC_DLT_VALUE_CH1_LVAL GENMASK(11, 0) +#define TH1520_ADC_DLT_VALUE_CH1_LVAL_OFF (0)
Don't specify offsets. Use FIELD_GET() and FIELD_PREP() for all accesses as those work with the mask alone. Also, make it clear what is a mask via naming. Typically _MSK or similar.
Quoted 16 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +16
-0+ +#define TH1520_ADC_FIFO_DATA_SIZE 32 +#define TH1520_ADC_PHY_ENCTR 0x8e0 +#define TH1520_ADC_TIMEOUT 500000 + +#define TH1520_ADC_CHAN (_idx, _chan_type) { \ + .type = (_chan_type), \ + .indexed = 1, \ + .channel = (_idx), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ +} + +enum vol_ref { +TH1520_ADC_VOL_VREF ,
drivers/iio/adc/xuantie-th1520-adc.h line 132
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +10
-0+ TH1520_ADC_VOL_INTE, +}; + +enum input_mode_sel { +TH1520_ADC_SINGLE_ENDED_INPUTS , +TH1520_ADC_DIFFERENTIAL_INPUTS , +}; + +enum selres_sel { +TH1520_ADC_SELRES_6BIT =6 ,
These seem unnecessary given the numeric values make sense without the enum.
Quoted 14 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +14
-0+TH1520_ADC_SELRES_8BIT =8 , +TH1520_ADC_SELRES_10BIT =10 , + TH1520_ADC_SELRES_12BIT = 12, +}; + +enum offset_mode_sel { +TH1520_ADC_OFFSET_DIS =0 , +TH1520_ADC_OFFSET_EN , +}; + +enum conversion_mode_sel { +TH1520_ADC_MODE_SINGLE , +TH1520_ADC_MODE_CONTINOUS , +};
drivers/iio/adc/xuantie-th1520-adc.h line 156
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +10
-0+ +enum clk_sel { +TH1520_ADC_FCLK_TYP_1M , +}; + +enum int_actual_mask { +TH1520_ADC_ACTUAL_CH0 , +TH1520_ADC_ACTUAL_CH1 , +TH1520_ADC_ACTUAL_ALL , +
Excess blank line.
Quoted 21 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +21
-0+}; + +enum int_delta_mask { +TH1520_ADC_DETAL_CH0 , +TH1520_ADC_DETAL_CH1 , +TH1520_ADC_DETAL_ALL , +}; + +struct th1520_adc_feature { +enum selres_sel selres_sel; +enum input_mode_sel input_mode; +enum vol_ref vol_ref; +enum offset_mode_sel offset_mode; +enum conversion_mode_sel conv_mode; +enum clk_sel clk_sel; +enum int_actual_mask int_actual; +enum int_delta_mask int_detal; +}; + +struct th1520_adc { +struct device *dev;
drivers/iio/adc/xuantie-th1520-adc.h line 187
Quoted 10 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +10
-0+void __iomem *regs; +struct clk *clk; + +u32 vref_uv; +u32 value; +struct regulator *vref; +struct th1520_adc_feature adc_feature; +u32 current_clk; +u32 ch0_offmeas; +u32 ch1_offmeas;
As above - don't introduce this stuff until it is used by features in the driver. Just hard code the defaults when you write the registers. Then when control is enabled in a future patch, it's much more obvious what has actually changed.
drivers/iio/adc/xuantie-th1520-adc.h line 190
Quoted 3 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +3
-0+ +struct completion completion; +/* lock to protect against multiple access to the device */
More info please on why this is a problem. Do we have read modify write cycles, or sequences where we need to not change the config whilst an ADC read is happening?
Quoted 3 lines
drivers/iio/adc/xuantie-th1520-adc.h new file +3
-0+struct mutex mlock; +}; +
[email protected]
maintainerPATCH 2/3 riscv: dts: thead: Add XuanTie TH1520 ADC device node
In reply to [email protected]From: Wei Fu <[email protected]>
Add nodes for the XuanTie TH1520 ADC device node on the XuanTie TH1520 Soc.
--- arch/riscv/boot/dts/thead/th1520.dtsi | 10 ++++++++++ 1 file changed, 10 insertions(+)
arch/riscv/boot/dts/thead/th1520.dtsi +10 -0
@@ -417,6 +417,16 @@ uart5: serial@fff7f0c000 { status = "disabled"; }; + adc: adc@0xfffff51000 { + compatible = "thead,th1520-adc"; + reg = <0xff 0xfff51000 0x0 0x1000>; + interrupts = <61 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&aonsys_clk>; + clock-names = "adc"; + /* ADC pin is proprietary,no need to config pinctrl */ + status = "disabled"; + }; + timer4: timer@ffffc33000 { compatible = "snps,dw-apb-timer"; reg = <0xff 0xffc33000 0x0 0x14>;
Signature · 1 line
2.44.0
kernel test robot
build botRe: [PATCH 2/3] riscv: dts: thead: Add XuanTie TH1520 ADC device node
kernel test robot noticed the following build errors:
Hi,
kernel test robot noticed the following build errors:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on robh/for-next linus/master v6.9-rc3 next-20240409] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/wefu-redhat-com/drivers-iio-adc-Add-XuanTie-TH1520-ADC-driver/20240330-041029 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/20240329200241.4122000-3-wefu%40redhat.com patch subject: [PATCH 2/3] riscv: dts: thead: Add XuanTie TH1520 ADC device node config: riscv-allyesconfig (https://download.01.org/0day-ci/archive/20240410/[email protected]/config) compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 546dc2245ffc4cccd0b05b58b7a5955e355a3b27) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240410/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> ERROR: Input tree has errors, aborting (use -f to force output)
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
[email protected]
maintainerPATCH 3/3 dt-bindings: adc: Document XuanTie TH1520 ADC
In reply to [email protected]From: Wei Fu <[email protected]>
Document devicetree bindings for the XuanTie TH1520 AP sub-system ADC.
--- .../bindings/iio/adc/thead,th1520.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+)
Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml new file +52 -0
@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/thead,th1520.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: XuanTie TH1520 Analog to Digital Converter (ADC) + +maintainers: + - Fugang Duan <[email protected]> + - Wei Fu <[email protected]> + +description: | + 12-Bit Analog to Digital Converter (ADC) on XuanTie TH1520 + +properties: + compatible: + const: thead,th1520 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + const: adc + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + - status + +additionalProperties: false + +examples: + - | + adc: adc@0xfffff51000 { + compatible = "thead,th1520-adc"; + reg = <0xff 0xfff51000 0x0 0x1000>; + interrupts = <61 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&aonsys_clk>; + clock-names = "adc"; + /* ADC pin is proprietary,no need to config pinctrl */ + status = "disabled"; + };
Signature · 1 line
2.44.0
Rob Herring
maintainerRe: [PATCH 3/3] dt-bindings: adc: Document XuanTie TH1520 ADC
Quoted 9 lines · On Sat, 30 Mar 2024 04:01:26 +0800, [email protected] wrote:
From: Wei Fu <[email protected]>
Document devicetree bindings for the XuanTie TH1520 AP sub-system ADC.
Signed-off-by Wei Fu <[email protected]>--- .../bindings/iio/adc/thead,th1520.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+)
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:./Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml:45:1: [error] syntax error: found character '\t' that cannot start any token (syntax)
dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/iio/adc/thead,th1520.example.dts' Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml:45:1: found a tab character where an indentation space is expected make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/iio/adc/thead,th1520.example.dts] Error 1 make[2]: *** Waiting for unfinished jobs...../Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml:45:1: found a tab character where an indentation space is expected /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml: ignoring, error parsing file make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
kernel test robot
build botRe: [PATCH 3/3] dt-bindings: adc: Document XuanTie TH1520 ADC
In reply to [email protected]kernel test robot noticed the following build warnings:
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.9-rc1 next-20240328] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/wefu-redhat-com/drivers-iio-adc-Add-XuanTie-TH1520-ADC-driver/20240330-041029 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/20240329200241.4122000-4-wefu%40redhat.com patch subject: [PATCH 3/3] dt-bindings: adc: Document XuanTie TH1520 ADC compiler: loongarch64-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20240330/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
dtcheck warnings: (new ones prefixed by >>)
>> Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml:45:1: [error] syntax error: found character '\t' that cannot start any token (syntax) -- >> Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml:45:1: found a tab character where an indentation space is expected -- >> Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml: ignoring, error parsing file
vim +45 Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml
41
42 examples:
43 - |
44 adc: adc@0xfffff51000 {
> 45 compatible = "thead,th1520-adc";Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Jonathan Cameron
maintainerRe: [PATCH 3/3] dt-bindings: adc: Document XuanTie TH1520 ADC
In reply to [email protected]Quoted 5 lines · On Sat, 30 Mar 2024 04:01:26 +0800 [email protected] wrote:
From: Wei Fu <[email protected]>
Document devicetree bindings for the XuanTie TH1520 AP sub-system ADC.
Signed-off-by Wei Fu <[email protected]>
Hi.
Few comments inline.
Given Fugang Duan is listed as a maintainer I'd also like an ack ideally from them. BTW your +CC list seems a bit random.
Thanks,
Jonathan
Quoted 18 lines
--- .../bindings/iio/adc/thead,th1520.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+)Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml new file +8
-0@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/thead,th1520.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: XuanTie TH1520 Analog to Digital Converter (ADC) +
Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml line 18
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml new file +10
-0+maintainers: + - Fugang Duan <[email protected]> + - Wei Fu <[email protected]> + +description: | + 12-Bit Analog to Digital Converter (ADC) on XuanTie TH1520 + +properties: + compatible: + const: thead,th1520
See the example - doesn't match. Should be -adc here as well as this is just part of the SoC.
Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml line 30
Quoted 12 lines
Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml new file +12
-0+ + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + const: adc
No objection to having names, but if there is only 1 why is it required?
Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml line 38
Quoted 8 lines
Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml new file +8
-0+ +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + - status
Status doesn't need to be here
Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml line 45
Quoted 7 lines
Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml new file +7
-0+ +additionalProperties: false + +examples: + - | + adc: adc@0xfffff51000 { + compatible = "thead,th1520-adc";
As the bot pointed out, spaces not tabs for these - Please run the suggested tests in that email before sending a v2.
Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml line 51
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml new file +6
-0+ reg = <0xff 0xfff51000 0x0 0x1000>; + interrupts = <61 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&aonsys_clk>; + clock-names = "adc"; + /* ADC pin is proprietary,no need to config pinctrl */ + status = "disabled";
No need for a status entry in the example.
Quoted 1 line
Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml new file +1
-0+ };