linux-rockchip.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] RK3588 ADC support
@ 2023-05-16 23:00 Shreeya Patel
  2023-05-16 23:00 ` [PATCH 1/7] iio: adc: rockchip_saradc: Add support for RK3588 Shreeya Patel
                   ` (7 more replies)
  0 siblings, 8 replies; 18+ messages in thread
From: Shreeya Patel @ 2023-05-16 23:00 UTC (permalink / raw)
  To: jic23, lars, heiko, robh+dt, krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky,
	Shreeya Patel

This patch series adds ADC support for RK3588 and updates
the DT bindings for the same.

To test ADC support on Rock 5B board, a voltage divider circuit
was created using the gpio pin 22 ( SARADC_IN4 ) and few more
tests were ran for testing the buffer and trigger using the
iio_generic_buffer tool.

Shreeya Patel (7):
  iio: adc: rockchip_saradc: Add support for RK3588
  iio: adc: rockchip_saradc: Make use of devm_clk_get_enabled
  iio: adc: rockchip_saradc: Use of_device_get_match_data
  iio: adc: rockchip_saradc: Match alignment with open parenthesis
  iio: adc: rockchip_saradc: Use dev_err_probe
  arm64: dts: rockchip: Add DT node for ADC support in RK3588
  dt-bindings: iio: adc: Add rockchip,rk3588-saradc string

 .../bindings/iio/adc/rockchip-saradc.yaml     |   1 +
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi     |  12 +
 drivers/iio/adc/rockchip_saradc.c             | 260 ++++++++++--------
 3 files changed, 164 insertions(+), 109 deletions(-)

-- 
2.30.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/7] iio: adc: rockchip_saradc: Add support for RK3588
  2023-05-16 23:00 [PATCH 0/7] RK3588 ADC support Shreeya Patel
@ 2023-05-16 23:00 ` Shreeya Patel
  2023-05-17 10:32   ` AngeloGioacchino Del Regno
  2023-05-20 15:57   ` Jonathan Cameron
  2023-05-16 23:00 ` [PATCH 2/7] iio: adc: rockchip_saradc: Make use of devm_clk_get_enabled Shreeya Patel
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 18+ messages in thread
From: Shreeya Patel @ 2023-05-16 23:00 UTC (permalink / raw)
  To: jic23, lars, heiko, robh+dt, krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky,
	Shreeya Patel, Simon Xue

Refactor conversion operation to support rk3588 saradc and
add separate start, read, powerdown in respective hooks.

Signed-off-by: Simon Xue <xxm@rock-chips.com>
Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 drivers/iio/adc/rockchip_saradc.c | 127 +++++++++++++++++++++++++++---
 1 file changed, 115 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index 79448c5ffc2a..ac6fdf8e673b 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -38,10 +38,29 @@
 #define SARADC_TIMEOUT			msecs_to_jiffies(100)
 #define SARADC_MAX_CHANNELS		8
 
+/* v2 registers */
+#define SARADC2_CONV_CON		0x0
+#define SARADC_T_PD_SOC			0x4
+#define SARADC_T_DAS_SOC		0xc
+#define SARADC2_END_INT_EN		0x104
+#define SARADC2_ST_CON			0x108
+#define SARADC2_STATUS			0x10c
+#define SARADC2_END_INT_ST		0x110
+#define SARADC2_DATA_BASE		0x120
+
+#define SARADC2_EN_END_INT		BIT(0)
+#define SARADC2_START			BIT(4)
+#define SARADC2_SINGLE_MODE		BIT(5)
+
+struct rockchip_saradc;
+
 struct rockchip_saradc_data {
 	const struct iio_chan_spec	*channels;
 	int				num_channels;
 	unsigned long			clk_rate;
+	void (*start)(struct rockchip_saradc *info, int chn);
+	int (*read)(struct rockchip_saradc *info);
+	void (*power_down)(struct rockchip_saradc *info);
 };
 
 struct rockchip_saradc {
@@ -60,27 +79,77 @@ struct rockchip_saradc {
 	struct notifier_block nb;
 };
 
-static void rockchip_saradc_power_down(struct rockchip_saradc *info)
+static void rockchip_saradc_reset_controller(struct reset_control *reset);
+
+static void rockchip_saradc_start_v1(struct rockchip_saradc *info, int chn)
+{
+	/* 8 clock periods as delay between power up and start cmd */
+	writel_relaxed(8, info->regs + SARADC_DLY_PU_SOC);
+	/* Select the channel to be used and trigger conversion */
+	writel(SARADC_CTRL_POWER_CTRL | (chn & SARADC_CTRL_CHN_MASK) |
+	       SARADC_CTRL_IRQ_ENABLE, info->regs + SARADC_CTRL);
+}
+
+static void rockchip_saradc_start_v2(struct rockchip_saradc *info, int chn)
+{
+	int val;
+
+	if (info->reset)
+		rockchip_saradc_reset_controller(info->reset);
+
+	writel_relaxed(0xc, info->regs + SARADC_T_DAS_SOC);
+	writel_relaxed(0x20, info->regs + SARADC_T_PD_SOC);
+	val = SARADC2_EN_END_INT << 16 | SARADC2_EN_END_INT;
+	writel_relaxed(val, info->regs + SARADC2_END_INT_EN);
+	val = SARADC2_START | SARADC2_SINGLE_MODE | chn;
+	writel(val << 16 | val, info->regs + SARADC2_CONV_CON);
+}
+
+static void rockchip_saradc_start(struct rockchip_saradc *info, int chn)
+{
+	info->data->start(info, chn);
+}
+
+static int rockchip_saradc_read_v1(struct rockchip_saradc *info)
+{
+	return readl_relaxed(info->regs + SARADC_DATA);
+}
+
+static int rockchip_saradc_read_v2(struct rockchip_saradc *info)
+{
+	int offset;
+
+	/* Clear irq */
+	writel_relaxed(0x1, info->regs + SARADC2_END_INT_ST);
+
+	offset = SARADC2_DATA_BASE + info->last_chan->channel * 0x4;
+
+	return readl_relaxed(info->regs + offset);
+}
+
+static int rockchip_saradc_read(struct rockchip_saradc *info)
+{
+	return info->data->read(info);
+}
+
+static void rockchip_saradc_power_down_v1(struct rockchip_saradc *info)
 {
-	/* Clear irq & power down adc */
 	writel_relaxed(0, info->regs + SARADC_CTRL);
 }
 
+static void rockchip_saradc_power_down(struct rockchip_saradc *info)
+{
+	if (info->data->power_down)
+		info->data->power_down(info);
+}
+
 static int rockchip_saradc_conversion(struct rockchip_saradc *info,
 				   struct iio_chan_spec const *chan)
 {
 	reinit_completion(&info->completion);
 
-	/* 8 clock periods as delay between power up and start cmd */
-	writel_relaxed(8, info->regs + SARADC_DLY_PU_SOC);
-
 	info->last_chan = chan;
-
-	/* Select the channel to be used and trigger conversion */
-	writel(SARADC_CTRL_POWER_CTRL
-			| (chan->channel & SARADC_CTRL_CHN_MASK)
-			| SARADC_CTRL_IRQ_ENABLE,
-		   info->regs + SARADC_CTRL);
+	rockchip_saradc_start(info, chan->channel);
 
 	if (!wait_for_completion_timeout(&info->completion, SARADC_TIMEOUT))
 		return -ETIMEDOUT;
@@ -123,7 +192,7 @@ static irqreturn_t rockchip_saradc_isr(int irq, void *dev_id)
 	struct rockchip_saradc *info = dev_id;
 
 	/* Read value */
-	info->last_val = readl_relaxed(info->regs + SARADC_DATA);
+	info->last_val = rockchip_saradc_read(info);
 	info->last_val &= GENMASK(info->last_chan->scan_type.realbits - 1, 0);
 
 	rockchip_saradc_power_down(info);
@@ -163,6 +232,9 @@ static const struct rockchip_saradc_data saradc_data = {
 	.channels = rockchip_saradc_iio_channels,
 	.num_channels = ARRAY_SIZE(rockchip_saradc_iio_channels),
 	.clk_rate = 1000000,
+	.start = rockchip_saradc_start_v1,
+	.read = rockchip_saradc_read_v1,
+	.power_down = rockchip_saradc_power_down_v1,
 };
 
 static const struct iio_chan_spec rockchip_rk3066_tsadc_iio_channels[] = {
@@ -174,6 +246,9 @@ static const struct rockchip_saradc_data rk3066_tsadc_data = {
 	.channels = rockchip_rk3066_tsadc_iio_channels,
 	.num_channels = ARRAY_SIZE(rockchip_rk3066_tsadc_iio_channels),
 	.clk_rate = 50000,
+	.start = rockchip_saradc_start_v1,
+	.read = rockchip_saradc_read_v1,
+	.power_down = rockchip_saradc_power_down_v1,
 };
 
 static const struct iio_chan_spec rockchip_rk3399_saradc_iio_channels[] = {
@@ -189,6 +264,9 @@ static const struct rockchip_saradc_data rk3399_saradc_data = {
 	.channels = rockchip_rk3399_saradc_iio_channels,
 	.num_channels = ARRAY_SIZE(rockchip_rk3399_saradc_iio_channels),
 	.clk_rate = 1000000,
+	.start = rockchip_saradc_start_v1,
+	.read = rockchip_saradc_read_v1,
+	.power_down = rockchip_saradc_power_down_v1,
 };
 
 static const struct iio_chan_spec rockchip_rk3568_saradc_iio_channels[] = {
@@ -206,6 +284,28 @@ static const struct rockchip_saradc_data rk3568_saradc_data = {
 	.channels = rockchip_rk3568_saradc_iio_channels,
 	.num_channels = ARRAY_SIZE(rockchip_rk3568_saradc_iio_channels),
 	.clk_rate = 1000000,
+	.start = rockchip_saradc_start_v1,
+	.read = rockchip_saradc_read_v1,
+	.power_down = rockchip_saradc_power_down_v1,
+};
+
+static const struct iio_chan_spec rockchip_rk3588_saradc_iio_channels[] = {
+	SARADC_CHANNEL(0, "adc0", 12),
+	SARADC_CHANNEL(1, "adc1", 12),
+	SARADC_CHANNEL(2, "adc2", 12),
+	SARADC_CHANNEL(3, "adc3", 12),
+	SARADC_CHANNEL(4, "adc4", 12),
+	SARADC_CHANNEL(5, "adc5", 12),
+	SARADC_CHANNEL(6, "adc6", 12),
+	SARADC_CHANNEL(7, "adc7", 12),
+};
+
+static const struct rockchip_saradc_data rk3588_saradc_data = {
+	.channels = rockchip_rk3588_saradc_iio_channels,
+	.num_channels = ARRAY_SIZE(rockchip_rk3588_saradc_iio_channels),
+	.clk_rate = 1000000,
+	.start = rockchip_saradc_start_v2,
+	.read = rockchip_saradc_read_v2,
 };
 
 static const struct of_device_id rockchip_saradc_match[] = {
@@ -221,6 +321,9 @@ static const struct of_device_id rockchip_saradc_match[] = {
 	}, {
 		.compatible = "rockchip,rk3568-saradc",
 		.data = &rk3568_saradc_data,
+	}, {
+		.compatible = "rockchip,rk3588-saradc",
+		.data = &rk3588_saradc_data,
 	},
 	{},
 };
-- 
2.30.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/7] iio: adc: rockchip_saradc: Make use of devm_clk_get_enabled
  2023-05-16 23:00 [PATCH 0/7] RK3588 ADC support Shreeya Patel
  2023-05-16 23:00 ` [PATCH 1/7] iio: adc: rockchip_saradc: Add support for RK3588 Shreeya Patel
@ 2023-05-16 23:00 ` Shreeya Patel
  2023-05-20 15:59   ` Jonathan Cameron
  2023-05-16 23:00 ` [PATCH 3/7] iio: adc: rockchip_saradc: Use of_device_get_match_data Shreeya Patel
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Shreeya Patel @ 2023-05-16 23:00 UTC (permalink / raw)
  To: jic23, lars, heiko, robh+dt, krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky,
	Shreeya Patel

Use devm_clk_get_enabled() to avoid manually disabling the
clock.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 drivers/iio/adc/rockchip_saradc.c | 77 +++++--------------------------
 1 file changed, 11 insertions(+), 66 deletions(-)

diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index ac6fdf8e673b..05b66eff9a44 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -339,20 +339,6 @@ static void rockchip_saradc_reset_controller(struct reset_control *reset)
 	reset_control_deassert(reset);
 }
 
-static void rockchip_saradc_clk_disable(void *data)
-{
-	struct rockchip_saradc *info = data;
-
-	clk_disable_unprepare(info->clk);
-}
-
-static void rockchip_saradc_pclk_disable(void *data)
-{
-	struct rockchip_saradc *info = data;
-
-	clk_disable_unprepare(info->pclk);
-}
-
 static void rockchip_saradc_regulator_disable(void *data)
 {
 	struct rockchip_saradc *info = data;
@@ -486,16 +472,6 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	info->pclk = devm_clk_get(&pdev->dev, "apb_pclk");
-	if (IS_ERR(info->pclk))
-		return dev_err_probe(&pdev->dev, PTR_ERR(info->pclk),
-				     "failed to get pclk\n");
-
-	info->clk = devm_clk_get(&pdev->dev, "saradc");
-	if (IS_ERR(info->clk))
-		return dev_err_probe(&pdev->dev, PTR_ERR(info->clk),
-				     "failed to get adc clock\n");
-
 	info->vref = devm_regulator_get(&pdev->dev, "vref");
 	if (IS_ERR(info->vref))
 		return dev_err_probe(&pdev->dev, PTR_ERR(info->vref),
@@ -504,6 +480,16 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 	if (info->reset)
 		rockchip_saradc_reset_controller(info->reset);
 
+	info->pclk = devm_clk_get_enabled(&pdev->dev, "apb_pclk");
+	if (IS_ERR(info->pclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(info->pclk),
+				     "failed to get pclk\n");
+
+	info->clk = devm_clk_get_enabled(&pdev->dev, "saradc");
+	if (IS_ERR(info->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(info->clk),
+				     "failed to get adc clock\n");
+
 	/*
 	 * Use a default value for the converter clock.
 	 * This may become user-configurable in the future.
@@ -533,32 +519,6 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 
 	info->uv_vref = ret;
 
-	ret = clk_prepare_enable(info->pclk);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to enable pclk\n");
-		return ret;
-	}
-	ret = devm_add_action_or_reset(&pdev->dev,
-				       rockchip_saradc_pclk_disable, info);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register devm action, %d\n",
-			ret);
-		return ret;
-	}
-
-	ret = clk_prepare_enable(info->clk);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to enable converter clock\n");
-		return ret;
-	}
-	ret = devm_add_action_or_reset(&pdev->dev,
-				       rockchip_saradc_clk_disable, info);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register devm action, %d\n",
-			ret);
-		return ret;
-	}
-
 	platform_set_drvdata(pdev, indio_dev);
 
 	indio_dev->name = dev_name(&pdev->dev);
@@ -594,8 +554,6 @@ static int rockchip_saradc_suspend(struct device *dev)
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct rockchip_saradc *info = iio_priv(indio_dev);
 
-	clk_disable_unprepare(info->clk);
-	clk_disable_unprepare(info->pclk);
 	regulator_disable(info->vref);
 
 	return 0;
@@ -605,21 +563,8 @@ static int rockchip_saradc_resume(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct rockchip_saradc *info = iio_priv(indio_dev);
-	int ret;
-
-	ret = regulator_enable(info->vref);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(info->pclk);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(info->clk);
-	if (ret)
-		clk_disable_unprepare(info->pclk);
 
-	return ret;
+	return regulator_enable(info->vref);
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(rockchip_saradc_pm_ops,
-- 
2.30.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 3/7] iio: adc: rockchip_saradc: Use of_device_get_match_data
  2023-05-16 23:00 [PATCH 0/7] RK3588 ADC support Shreeya Patel
  2023-05-16 23:00 ` [PATCH 1/7] iio: adc: rockchip_saradc: Add support for RK3588 Shreeya Patel
  2023-05-16 23:00 ` [PATCH 2/7] iio: adc: rockchip_saradc: Make use of devm_clk_get_enabled Shreeya Patel
@ 2023-05-16 23:00 ` Shreeya Patel
  2023-05-17 10:34   ` AngeloGioacchino Del Regno
  2023-05-16 23:00 ` [PATCH 4/7] iio: adc: rockchip_saradc: Match alignment with open parenthesis Shreeya Patel
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Shreeya Patel @ 2023-05-16 23:00 UTC (permalink / raw)
  To: jic23, lars, heiko, robh+dt, krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky,
	Shreeya Patel

Use of_device_get_match_data() to simplify the code.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 drivers/iio/adc/rockchip_saradc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index 05b66eff9a44..05ea823a8f16 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -408,10 +408,10 @@ static void rockchip_saradc_regulator_unreg_notifier(void *data)
 
 static int rockchip_saradc_probe(struct platform_device *pdev)
 {
+	const struct rockchip_saradc_data *match_data;
 	struct rockchip_saradc *info = NULL;
 	struct device_node *np = pdev->dev.of_node;
 	struct iio_dev *indio_dev = NULL;
-	const struct of_device_id *match;
 	int ret;
 	int irq;
 
@@ -425,13 +425,13 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 	}
 	info = iio_priv(indio_dev);
 
-	match = of_match_device(rockchip_saradc_match, &pdev->dev);
-	if (!match) {
+	match_data = of_device_get_match_data(&pdev->dev);
+	if (!match_data) {
 		dev_err(&pdev->dev, "failed to match device\n");
 		return -ENODEV;
 	}
 
-	info->data = match->data;
+	info->data = match_data;
 
 	/* Sanity check for possible later IP variants with more channels */
 	if (info->data->num_channels > SARADC_MAX_CHANNELS) {
-- 
2.30.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 4/7] iio: adc: rockchip_saradc: Match alignment with open parenthesis
  2023-05-16 23:00 [PATCH 0/7] RK3588 ADC support Shreeya Patel
                   ` (2 preceding siblings ...)
  2023-05-16 23:00 ` [PATCH 3/7] iio: adc: rockchip_saradc: Use of_device_get_match_data Shreeya Patel
@ 2023-05-16 23:00 ` Shreeya Patel
  2023-05-17 10:34   ` AngeloGioacchino Del Regno
  2023-05-16 23:00 ` [PATCH 5/7] iio: adc: rockchip_saradc: Use dev_err_probe Shreeya Patel
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Shreeya Patel @ 2023-05-16 23:00 UTC (permalink / raw)
  To: jic23, lars, heiko, robh+dt, krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky,
	Shreeya Patel

Match alignment with open parenthesis for improving the code
readability.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 drivers/iio/adc/rockchip_saradc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index 05ea823a8f16..5e1e8575bc76 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -144,7 +144,7 @@ static void rockchip_saradc_power_down(struct rockchip_saradc *info)
 }
 
 static int rockchip_saradc_conversion(struct rockchip_saradc *info,
-				   struct iio_chan_spec const *chan)
+				      struct iio_chan_spec const *chan)
 {
 	reinit_completion(&info->completion);
 
@@ -387,8 +387,7 @@ static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p)
 }
 
 static int rockchip_saradc_volt_notify(struct notifier_block *nb,
-						   unsigned long event,
-						   void *data)
+				       unsigned long event, void *data)
 {
 	struct rockchip_saradc *info =
 			container_of(nb, struct rockchip_saradc, nb);
-- 
2.30.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 5/7] iio: adc: rockchip_saradc: Use dev_err_probe
  2023-05-16 23:00 [PATCH 0/7] RK3588 ADC support Shreeya Patel
                   ` (3 preceding siblings ...)
  2023-05-16 23:00 ` [PATCH 4/7] iio: adc: rockchip_saradc: Match alignment with open parenthesis Shreeya Patel
@ 2023-05-16 23:00 ` Shreeya Patel
  2023-05-17 10:40   ` AngeloGioacchino Del Regno
  2023-05-16 23:00 ` [PATCH 6/7] arm64: dts: rockchip: Add DT node for ADC support in RK3588 Shreeya Patel
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Shreeya Patel @ 2023-05-16 23:00 UTC (permalink / raw)
  To: jic23, lars, heiko, robh+dt, krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky,
	Shreeya Patel

Use dev_err_probe instead of dev_err in probe function,
which simplifies code a little bit and prints the error
code.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 drivers/iio/adc/rockchip_saradc.c | 45 ++++++++++++++-----------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index 5e1e8575bc76..a52021fd477d 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -418,25 +418,23 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
-	if (!indio_dev) {
-		dev_err(&pdev->dev, "failed allocating iio device\n");
-		return -ENOMEM;
-	}
+	if (!indio_dev)
+		return dev_err_probe(&pdev->dev, -ENOMEM,
+				     "failed allocating iio device\n");
+
 	info = iio_priv(indio_dev);
 
 	match_data = of_device_get_match_data(&pdev->dev);
-	if (!match_data) {
-		dev_err(&pdev->dev, "failed to match device\n");
-		return -ENODEV;
-	}
+	if (!match_data)
+		return dev_err_probe(&pdev->dev, -ENODEV,
+				     "failed to match device\n");
 
 	info->data = match_data;
 
 	/* Sanity check for possible later IP variants with more channels */
-	if (info->data->num_channels > SARADC_MAX_CHANNELS) {
-		dev_err(&pdev->dev, "max channels exceeded");
-		return -EINVAL;
-	}
+	if (info->data->num_channels > SARADC_MAX_CHANNELS)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "max channels exceeded");
 
 	info->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(info->regs))
@@ -494,23 +492,20 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 	 * This may become user-configurable in the future.
 	 */
 	ret = clk_set_rate(info->clk, info->data->clk_rate);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to set adc clk rate, %d\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to set adc clk rate\n");
 
 	ret = regulator_enable(info->vref);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to enable vref regulator\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to enable vref regulator\n");
+
 	ret = devm_add_action_or_reset(&pdev->dev,
 				       rockchip_saradc_regulator_disable, info);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register devm action, %d\n",
-			ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to register devm action\n");
 
 	ret = regulator_get_voltage(info->vref);
 	if (ret < 0)
-- 
2.30.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 6/7] arm64: dts: rockchip: Add DT node for ADC support in RK3588
  2023-05-16 23:00 [PATCH 0/7] RK3588 ADC support Shreeya Patel
                   ` (4 preceding siblings ...)
  2023-05-16 23:00 ` [PATCH 5/7] iio: adc: rockchip_saradc: Use dev_err_probe Shreeya Patel
@ 2023-05-16 23:00 ` Shreeya Patel
  2023-05-17 10:41   ` AngeloGioacchino Del Regno
  2023-05-16 23:00 ` [PATCH 7/7] dt-bindings: iio: adc: Add rockchip,rk3588-saradc string Shreeya Patel
  2023-05-20 16:00 ` [PATCH 0/7] RK3588 ADC support Jonathan Cameron
  7 siblings, 1 reply; 18+ messages in thread
From: Shreeya Patel @ 2023-05-16 23:00 UTC (permalink / raw)
  To: jic23, lars, heiko, robh+dt, krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky,
	Shreeya Patel

Add DT node for ADC support in RK3588.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index 657c019d27fa..6c4424bc4b1b 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -1825,6 +1825,18 @@ dmac2: dma-controller@fed10000 {
 		#dma-cells = <1>;
 	};
 
+	saradc: saradc@fec10000 {
+		compatible = "rockchip,rk3588-saradc";
+		reg = <0x0 0xfec10000 0x0 0x10000>;
+		interrupts = <GIC_SPI 398 IRQ_TYPE_LEVEL_HIGH 0>;
+		#io-channel-cells = <1>;
+		clocks = <&cru CLK_SARADC>, <&cru PCLK_SARADC>;
+		clock-names = "saradc", "apb_pclk";
+		resets = <&cru SRST_P_SARADC>;
+		reset-names = "saradc-apb";
+		status = "disabled";
+	};
+
 	system_sram2: sram@ff001000 {
 		compatible = "mmio-sram";
 		reg = <0x0 0xff001000 0x0 0xef000>;
-- 
2.30.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 7/7] dt-bindings: iio: adc: Add rockchip,rk3588-saradc string
  2023-05-16 23:00 [PATCH 0/7] RK3588 ADC support Shreeya Patel
                   ` (5 preceding siblings ...)
  2023-05-16 23:00 ` [PATCH 6/7] arm64: dts: rockchip: Add DT node for ADC support in RK3588 Shreeya Patel
@ 2023-05-16 23:00 ` Shreeya Patel
  2023-05-17  8:49   ` Krzysztof Kozlowski
  2023-05-20 16:00 ` [PATCH 0/7] RK3588 ADC support Jonathan Cameron
  7 siblings, 1 reply; 18+ messages in thread
From: Shreeya Patel @ 2023-05-16 23:00 UTC (permalink / raw)
  To: jic23, lars, heiko, robh+dt, krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky,
	Shreeya Patel

Add rockchip,rk3588-saradc compatible string.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 Documentation/devicetree/bindings/iio/adc/rockchip-saradc.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.yaml
index da50b529c157..11c27ea451c8 100644
--- a/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.yaml
@@ -21,6 +21,7 @@ properties:
               - rockchip,rk3308-saradc
               - rockchip,rk3328-saradc
               - rockchip,rk3568-saradc
+              - rockchip,rk3588-saradc
               - rockchip,rv1108-saradc
               - rockchip,rv1126-saradc
           - const: rockchip,rk3399-saradc
-- 
2.30.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 7/7] dt-bindings: iio: adc: Add rockchip,rk3588-saradc string
  2023-05-16 23:00 ` [PATCH 7/7] dt-bindings: iio: adc: Add rockchip,rk3588-saradc string Shreeya Patel
@ 2023-05-17  8:49   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 18+ messages in thread
From: Krzysztof Kozlowski @ 2023-05-17  8:49 UTC (permalink / raw)
  To: Shreeya Patel, jic23, lars, heiko, robh+dt,
	krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky

On 17/05/2023 01:00, Shreeya Patel wrote:
> Add rockchip,rk3588-saradc compatible string.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
>  Documentation/devicetree/bindings/iio/adc/rockchip-saradc.yaml | 1 +

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/7] iio: adc: rockchip_saradc: Add support for RK3588
  2023-05-16 23:00 ` [PATCH 1/7] iio: adc: rockchip_saradc: Add support for RK3588 Shreeya Patel
@ 2023-05-17 10:32   ` AngeloGioacchino Del Regno
  2023-05-20 15:57   ` Jonathan Cameron
  1 sibling, 0 replies; 18+ messages in thread
From: AngeloGioacchino Del Regno @ 2023-05-17 10:32 UTC (permalink / raw)
  To: Shreeya Patel, jic23, lars, heiko, robh+dt,
	krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky,
	Simon Xue

Il 17/05/23 01:00, Shreeya Patel ha scritto:
> Refactor conversion operation to support rk3588 saradc and
> add separate start, read, powerdown in respective hooks.
> 
> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
>   drivers/iio/adc/rockchip_saradc.c | 127 +++++++++++++++++++++++++++---
>   1 file changed, 115 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index 79448c5ffc2a..ac6fdf8e673b 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c
> @@ -38,10 +38,29 @@
>   #define SARADC_TIMEOUT			msecs_to_jiffies(100)
>   #define SARADC_MAX_CHANNELS		8
>   
> +/* v2 registers */
> +#define SARADC2_CONV_CON		0x0
> +#define SARADC_T_PD_SOC			0x4
> +#define SARADC_T_DAS_SOC		0xc
> +#define SARADC2_END_INT_EN		0x104
> +#define SARADC2_ST_CON			0x108
> +#define SARADC2_STATUS			0x10c
> +#define SARADC2_END_INT_ST		0x110
> +#define SARADC2_DATA_BASE		0x120
> +
> +#define SARADC2_EN_END_INT		BIT(0)
> +#define SARADC2_START			BIT(4)
> +#define SARADC2_SINGLE_MODE		BIT(5)
> +
> +struct rockchip_saradc;
> +
>   struct rockchip_saradc_data {
>   	const struct iio_chan_spec	*channels;
>   	int				num_channels;
>   	unsigned long			clk_rate;
> +	void (*start)(struct rockchip_saradc *info, int chn);
> +	int (*read)(struct rockchip_saradc *info);
> +	void (*power_down)(struct rockchip_saradc *info);
>   };
>   
>   struct rockchip_saradc {
> @@ -60,27 +79,77 @@ struct rockchip_saradc {
>   	struct notifier_block nb;
>   };
>   
> -static void rockchip_saradc_power_down(struct rockchip_saradc *info)
> +static void rockchip_saradc_reset_controller(struct reset_control *reset);
> +
> +static void rockchip_saradc_start_v1(struct rockchip_saradc *info, int chn)
> +{
> +	/* 8 clock periods as delay between power up and start cmd */
> +	writel_relaxed(8, info->regs + SARADC_DLY_PU_SOC);
> +	/* Select the channel to be used and trigger conversion */
> +	writel(SARADC_CTRL_POWER_CTRL | (chn & SARADC_CTRL_CHN_MASK) |
> +	       SARADC_CTRL_IRQ_ENABLE, info->regs + SARADC_CTRL);
> +}
> +
> +static void rockchip_saradc_start_v2(struct rockchip_saradc *info, int chn)
> +{
> +	int val;
> +
> +	if (info->reset)
> +		rockchip_saradc_reset_controller(info->reset);
> +
> +	writel_relaxed(0xc, info->regs + SARADC_T_DAS_SOC);
> +	writel_relaxed(0x20, info->regs + SARADC_T_PD_SOC);
> +	val = SARADC2_EN_END_INT << 16 | SARADC2_EN_END_INT;

What about using bitfield macros?

/* "LO" and "HI" may get a better name, if there's any possible one! */
#define SARADC2_EN_END_INT_LO BIT(0)
#define SARADC2_EN_END_INT_HI BIT(16)

val = FIELD_PREP(SARADC_EN_END_INT_LO, 1);
val |= FIELD_PREP(SARADC_EN_END_INT_HI, 1);
writel ....

Otherwise, if it's about two really "specular" instances, you can probably
keep the current definition as SARADC2_EN_END_INT and do

val = FIELD_PREP(SARADC_EN_END_INT, 1);
val |= val << 16;
writel ...

/* note: high, low bits are unknown to me, I assumed it's 16 bits :-) */
#define SARADC2_CONV_CHANNELS GENMASK(15, 0)

val = FIELD_PREP(SARADC2_START, 1);
val |= FIELD_PREP(SARADC2_SINGLE_MODE, 1);
val |= FIELD_PREP(SARADC2_CONV_CHANNELS, chn);
val |= val << 16;
writel ...


> +	writel_relaxed(val, info->regs + SARADC2_END_INT_EN);
> +	val = SARADC2_START | SARADC2_SINGLE_MODE | chn;
> +	writel(val << 16 | val, info->regs + SARADC2_CONV_CON);
> +}
> +

Cheers,
Angelo


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/7] iio: adc: rockchip_saradc: Use of_device_get_match_data
  2023-05-16 23:00 ` [PATCH 3/7] iio: adc: rockchip_saradc: Use of_device_get_match_data Shreeya Patel
@ 2023-05-17 10:34   ` AngeloGioacchino Del Regno
  0 siblings, 0 replies; 18+ messages in thread
From: AngeloGioacchino Del Regno @ 2023-05-17 10:34 UTC (permalink / raw)
  To: Shreeya Patel, jic23, lars, heiko, robh+dt,
	krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky

Il 17/05/23 01:00, Shreeya Patel ha scritto:
> Use of_device_get_match_data() to simplify the code.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
>   drivers/iio/adc/rockchip_saradc.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index 05b66eff9a44..05ea823a8f16 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c
> @@ -408,10 +408,10 @@ static void rockchip_saradc_regulator_unreg_notifier(void *data)
>   
>   static int rockchip_saradc_probe(struct platform_device *pdev)
>   {
> +	const struct rockchip_saradc_data *match_data;

s/match_data/data/g maybe?

Regardless of that,

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

>   	struct rockchip_saradc *info = NULL;
>   	struct device_node *np = pdev->dev.of_node;
>   	struct iio_dev *indio_dev = NULL;
> -	const struct of_device_id *match;
>   	int ret;
>   	int irq;
>   
> @@ -425,13 +425,13 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>   	}
>   	info = iio_priv(indio_dev);
>   
> -	match = of_match_device(rockchip_saradc_match, &pdev->dev);
> -	if (!match) {
> +	match_data = of_device_get_match_data(&pdev->dev);
> +	if (!match_data) {
>   		dev_err(&pdev->dev, "failed to match device\n");
>   		return -ENODEV;
>   	}
>   
> -	info->data = match->data;
> +	info->data = match_data;
>   
>   	/* Sanity check for possible later IP variants with more channels */
>   	if (info->data->num_channels > SARADC_MAX_CHANNELS) {



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/7] iio: adc: rockchip_saradc: Match alignment with open parenthesis
  2023-05-16 23:00 ` [PATCH 4/7] iio: adc: rockchip_saradc: Match alignment with open parenthesis Shreeya Patel
@ 2023-05-17 10:34   ` AngeloGioacchino Del Regno
  0 siblings, 0 replies; 18+ messages in thread
From: AngeloGioacchino Del Regno @ 2023-05-17 10:34 UTC (permalink / raw)
  To: Shreeya Patel, jic23, lars, heiko, robh+dt,
	krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky

Il 17/05/23 01:00, Shreeya Patel ha scritto:
> Match alignment with open parenthesis for improving the code
> readability.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com

> ---
>   drivers/iio/adc/rockchip_saradc.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index 05ea823a8f16..5e1e8575bc76 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c
> @@ -144,7 +144,7 @@ static void rockchip_saradc_power_down(struct rockchip_saradc *info)
>   }
>   
>   static int rockchip_saradc_conversion(struct rockchip_saradc *info,
> -				   struct iio_chan_spec const *chan)
> +				      struct iio_chan_spec const *chan)
>   {
>   	reinit_completion(&info->completion);
>   
> @@ -387,8 +387,7 @@ static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p)
>   }
>   
>   static int rockchip_saradc_volt_notify(struct notifier_block *nb,
> -						   unsigned long event,
> -						   void *data)
> +				       unsigned long event, void *data)
>   {
>   	struct rockchip_saradc *info =
>   			container_of(nb, struct rockchip_saradc, nb);



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 5/7] iio: adc: rockchip_saradc: Use dev_err_probe
  2023-05-16 23:00 ` [PATCH 5/7] iio: adc: rockchip_saradc: Use dev_err_probe Shreeya Patel
@ 2023-05-17 10:40   ` AngeloGioacchino Del Regno
  2023-05-20 15:49     ` Jonathan Cameron
  0 siblings, 1 reply; 18+ messages in thread
From: AngeloGioacchino Del Regno @ 2023-05-17 10:40 UTC (permalink / raw)
  To: Shreeya Patel, jic23, lars, heiko, robh+dt,
	krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky

Il 17/05/23 01:00, Shreeya Patel ha scritto:
> Use dev_err_probe instead of dev_err in probe function,
> which simplifies code a little bit and prints the error
> code.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
>   drivers/iio/adc/rockchip_saradc.c | 45 ++++++++++++++-----------------
>   1 file changed, 20 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index 5e1e8575bc76..a52021fd477d 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c

..snip..

> @@ -494,23 +492,20 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>   	 * This may become user-configurable in the future.
>   	 */
>   	ret = clk_set_rate(info->clk, info->data->clk_rate);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to set adc clk rate, %d\n", ret);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to set adc clk rate\n");
>   
>   	ret = regulator_enable(info->vref);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to enable vref regulator\n");
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to enable vref regulator\n");
> +
>   	ret = devm_add_action_or_reset(&pdev->dev,
>   				       rockchip_saradc_regulator_disable, info);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> -			ret);
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to register devm action\n");

It's not your fault - and it's about a pre-existing issue, but there's that:
you're returning an error if devm_add_action_or_reset() fails (which is highly
unlikely), but you're leaving the regulator enabled!

As for how to proceed here, I would suggest to fix this issue in a separated
commit (before the dev_err_probe() conversion); it'd look like...

	if (ret) {
		regulator_disable(info->vref);
		dev_err( .... blurb );
		return ret;
	}

and after the conversion it'd look like...

	if (ret) {
		regulator_disable(info->vref);
		return dev_err_probe( ... blurb );
	}

Cheers,
Angelo

>   
>   	ret = regulator_get_voltage(info->vref);
>   	if (ret < 0)


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 6/7] arm64: dts: rockchip: Add DT node for ADC support in RK3588
  2023-05-16 23:00 ` [PATCH 6/7] arm64: dts: rockchip: Add DT node for ADC support in RK3588 Shreeya Patel
@ 2023-05-17 10:41   ` AngeloGioacchino Del Regno
  0 siblings, 0 replies; 18+ messages in thread
From: AngeloGioacchino Del Regno @ 2023-05-17 10:41 UTC (permalink / raw)
  To: Shreeya Patel, jic23, lars, heiko, robh+dt,
	krzysztof.kozlowski+dt, sebastian.reichel
  Cc: linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky

Il 17/05/23 01:00, Shreeya Patel ha scritto:
> Add DT node for ADC support in RK3588.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 5/7] iio: adc: rockchip_saradc: Use dev_err_probe
  2023-05-17 10:40   ` AngeloGioacchino Del Regno
@ 2023-05-20 15:49     ` Jonathan Cameron
  0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2023-05-20 15:49 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: Shreeya Patel, lars, heiko, robh+dt, krzysztof.kozlowski+dt,
	sebastian.reichel, linux-iio, devicetree, linux-arm-kernel,
	linux-rockchip, linux-kernel, kernel, gustavo.padovan,
	serge.broslavsky

On Wed, 17 May 2023 12:40:37 +0200
AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Il 17/05/23 01:00, Shreeya Patel ha scritto:
> > Use dev_err_probe instead of dev_err in probe function,
> > which simplifies code a little bit and prints the error
> > code.
> > 
> > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> > ---
> >   drivers/iio/adc/rockchip_saradc.c | 45 ++++++++++++++-----------------
> >   1 file changed, 20 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> > index 5e1e8575bc76..a52021fd477d 100644
> > --- a/drivers/iio/adc/rockchip_saradc.c
> > +++ b/drivers/iio/adc/rockchip_saradc.c  
> 
> ..snip..
> 
> > @@ -494,23 +492,20 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
> >   	 * This may become user-configurable in the future.
> >   	 */
> >   	ret = clk_set_rate(info->clk, info->data->clk_rate);
> > -	if (ret < 0) {
> > -		dev_err(&pdev->dev, "failed to set adc clk rate, %d\n", ret);
> > -		return ret;
> > -	}
> > +	if (ret < 0)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "failed to set adc clk rate\n");
> >   
> >   	ret = regulator_enable(info->vref);
> > -	if (ret < 0) {
> > -		dev_err(&pdev->dev, "failed to enable vref regulator\n");
> > -		return ret;
> > -	}
> > +	if (ret < 0)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "failed to enable vref regulator\n");
> > +
> >   	ret = devm_add_action_or_reset(&pdev->dev,
> >   				       rockchip_saradc_regulator_disable, info);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> > -			ret);
> > -		return ret;
> > -	}
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "failed to register devm action\n");  
> 
> It's not your fault - and it's about a pre-existing issue, but there's that:
> you're returning an error if devm_add_action_or_reset() fails (which is highly
> unlikely), but you're leaving the regulator enabled!
Which regulator?  The _or_reset() part means that the callback is called if
the devm_add_action_or_reset is going to return an error.  So in the
path you talk about
rockchip_saradc_regulator_disable() is called and disables vref

> 
> As for how to proceed here, I would suggest to fix this issue in a separated
> commit (before the dev_err_probe() conversion); it'd look like...
> 
> 	if (ret) {
> 		regulator_disable(info->vref);
> 		dev_err( .... blurb );
> 		return ret;
> 	}
> 
> and after the conversion it'd look like...
> 
> 	if (ret) {
> 		regulator_disable(info->vref);
> 		return dev_err_probe( ... blurb );
> 	}
> 
> Cheers,
> Angelo
> 
> >   
> >   	ret = regulator_get_voltage(info->vref);
> >   	if (ret < 0)  
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/7] iio: adc: rockchip_saradc: Add support for RK3588
  2023-05-16 23:00 ` [PATCH 1/7] iio: adc: rockchip_saradc: Add support for RK3588 Shreeya Patel
  2023-05-17 10:32   ` AngeloGioacchino Del Regno
@ 2023-05-20 15:57   ` Jonathan Cameron
  1 sibling, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2023-05-20 15:57 UTC (permalink / raw)
  To: Shreeya Patel
  Cc: lars, heiko, robh+dt, krzysztof.kozlowski+dt, sebastian.reichel,
	linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky,
	Simon Xue

On Wed, 17 May 2023 04:30:45 +0530
Shreeya Patel <shreeya.patel@collabora.com> wrote:

> Refactor conversion operation to support rk3588 saradc and
> add separate start, read, powerdown in respective hooks.
> 
> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
Hi Shreeya,

This particular chain of tags has unclear authorship.
If Simon wrote the driver, then the author (from address) of the
patch should be Simon's.  If it is a codeveloped situation look
at how to use the co-developed-by tag


Generally the nicest way to add new callbacks is to do them in their
own patch.  Then introduce new devices with different callbacks in a follow
on patch.  That means the first one can be reviewed to ensure there
is no functional change separate from the new code in the following patch.



> +static const struct rockchip_saradc_data rk3588_saradc_data = {
> +	.channels = rockchip_rk3588_saradc_iio_channels,
> +	.num_channels = ARRAY_SIZE(rockchip_rk3588_saradc_iio_channels),
> +	.clk_rate = 1000000,
> +	.start = rockchip_saradc_start_v2,

As mentioned above, I'd like to see the changes for old devices done in patch
1 and the support for new device in a new patch.

> +	.read = rockchip_saradc_read_v2,
>  };



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/7] iio: adc: rockchip_saradc: Make use of devm_clk_get_enabled
  2023-05-16 23:00 ` [PATCH 2/7] iio: adc: rockchip_saradc: Make use of devm_clk_get_enabled Shreeya Patel
@ 2023-05-20 15:59   ` Jonathan Cameron
  0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2023-05-20 15:59 UTC (permalink / raw)
  To: Shreeya Patel
  Cc: lars, heiko, robh+dt, krzysztof.kozlowski+dt, sebastian.reichel,
	linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky

On Wed, 17 May 2023 04:30:46 +0530
Shreeya Patel <shreeya.patel@collabora.com> wrote:

> Use devm_clk_get_enabled() to avoid manually disabling the
> clock.

Please comment on why the enable is now done earlier than before.

> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
>  drivers/iio/adc/rockchip_saradc.c | 77 +++++--------------------------
>  1 file changed, 11 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index ac6fdf8e673b..05b66eff9a44 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c
> @@ -339,20 +339,6 @@ static void rockchip_saradc_reset_controller(struct reset_control *reset)
>  	reset_control_deassert(reset);
>  }
>  
> -static void rockchip_saradc_clk_disable(void *data)
> -{
> -	struct rockchip_saradc *info = data;
> -
> -	clk_disable_unprepare(info->clk);
> -}
> -
> -static void rockchip_saradc_pclk_disable(void *data)
> -{
> -	struct rockchip_saradc *info = data;
> -
> -	clk_disable_unprepare(info->pclk);
> -}
> -
>  static void rockchip_saradc_regulator_disable(void *data)
>  {
>  	struct rockchip_saradc *info = data;
> @@ -486,16 +472,6 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	info->pclk = devm_clk_get(&pdev->dev, "apb_pclk");
> -	if (IS_ERR(info->pclk))
> -		return dev_err_probe(&pdev->dev, PTR_ERR(info->pclk),
> -				     "failed to get pclk\n");
> -
> -	info->clk = devm_clk_get(&pdev->dev, "saradc");
> -	if (IS_ERR(info->clk))
> -		return dev_err_probe(&pdev->dev, PTR_ERR(info->clk),
> -				     "failed to get adc clock\n");
> -
>  	info->vref = devm_regulator_get(&pdev->dev, "vref");
>  	if (IS_ERR(info->vref))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(info->vref),
> @@ -504,6 +480,16 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>  	if (info->reset)
>  		rockchip_saradc_reset_controller(info->reset);
>  
> +	info->pclk = devm_clk_get_enabled(&pdev->dev, "apb_pclk");
> +	if (IS_ERR(info->pclk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(info->pclk),
> +				     "failed to get pclk\n");
> +
> +	info->clk = devm_clk_get_enabled(&pdev->dev, "saradc");
> +	if (IS_ERR(info->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(info->clk),
> +				     "failed to get adc clock\n");
> +
>  	/*
>  	 * Use a default value for the converter clock.
>  	 * This may become user-configurable in the future.
> @@ -533,32 +519,6 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>  
>  	info->uv_vref = ret;
>  
> -	ret = clk_prepare_enable(info->pclk);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to enable pclk\n");
> -		return ret;
> -	}
> -	ret = devm_add_action_or_reset(&pdev->dev,
> -				       rockchip_saradc_pclk_disable, info);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> -			ret);
> -		return ret;
> -	}
> -
> -	ret = clk_prepare_enable(info->clk);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to enable converter clock\n");
> -		return ret;
> -	}
> -	ret = devm_add_action_or_reset(&pdev->dev,
> -				       rockchip_saradc_clk_disable, info);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> -			ret);
> -		return ret;
> -	}
> -
>  	platform_set_drvdata(pdev, indio_dev);
>  
>  	indio_dev->name = dev_name(&pdev->dev);
> @@ -594,8 +554,6 @@ static int rockchip_saradc_suspend(struct device *dev)
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct rockchip_saradc *info = iio_priv(indio_dev);
>  
> -	clk_disable_unprepare(info->clk);
> -	clk_disable_unprepare(info->pclk);
>  	regulator_disable(info->vref);
>  
>  	return 0;
> @@ -605,21 +563,8 @@ static int rockchip_saradc_resume(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct rockchip_saradc *info = iio_priv(indio_dev);
> -	int ret;
> -
> -	ret = regulator_enable(info->vref);
> -	if (ret)
> -		return ret;
> -
> -	ret = clk_prepare_enable(info->pclk);
> -	if (ret)
> -		return ret;
> -
> -	ret = clk_prepare_enable(info->clk);
> -	if (ret)
> -		clk_disable_unprepare(info->pclk);
>  
> -	return ret;
> +	return regulator_enable(info->vref);
>  }
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(rockchip_saradc_pm_ops,


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/7] RK3588 ADC support
  2023-05-16 23:00 [PATCH 0/7] RK3588 ADC support Shreeya Patel
                   ` (6 preceding siblings ...)
  2023-05-16 23:00 ` [PATCH 7/7] dt-bindings: iio: adc: Add rockchip,rk3588-saradc string Shreeya Patel
@ 2023-05-20 16:00 ` Jonathan Cameron
  7 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2023-05-20 16:00 UTC (permalink / raw)
  To: Shreeya Patel
  Cc: lars, heiko, robh+dt, krzysztof.kozlowski+dt, sebastian.reichel,
	linux-iio, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel, kernel, gustavo.padovan, serge.broslavsky

On Wed, 17 May 2023 04:30:44 +0530
Shreeya Patel <shreeya.patel@collabora.com> wrote:

> This patch series adds ADC support for RK3588 and updates
> the DT bindings for the same.
> 
> To test ADC support on Rock 5B board, a voltage divider circuit
> was created using the gpio pin 22 ( SARADC_IN4 ) and few more
> tests were ran for testing the buffer and trigger using the
> iio_generic_buffer tool.
> 
> Shreeya Patel (7):
>   iio: adc: rockchip_saradc: Add support for RK3588
>   iio: adc: rockchip_saradc: Make use of devm_clk_get_enabled
>   iio: adc: rockchip_saradc: Use of_device_get_match_data
>   iio: adc: rockchip_saradc: Match alignment with open parenthesis
>   iio: adc: rockchip_saradc: Use dev_err_probe
>   arm64: dts: rockchip: Add DT node for ADC support in RK3588
>   dt-bindings: iio: adc: Add rockchip,rk3588-saradc string
> 
>  .../bindings/iio/adc/rockchip-saradc.yaml     |   1 +
>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi     |  12 +
>  drivers/iio/adc/rockchip_saradc.c             | 260 ++++++++++--------
>  3 files changed, 164 insertions(+), 109 deletions(-)
> 

Patches I haven't commented on all look fine to me.

Thanks,

Jonathan

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2023-05-20 15:44 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-16 23:00 [PATCH 0/7] RK3588 ADC support Shreeya Patel
2023-05-16 23:00 ` [PATCH 1/7] iio: adc: rockchip_saradc: Add support for RK3588 Shreeya Patel
2023-05-17 10:32   ` AngeloGioacchino Del Regno
2023-05-20 15:57   ` Jonathan Cameron
2023-05-16 23:00 ` [PATCH 2/7] iio: adc: rockchip_saradc: Make use of devm_clk_get_enabled Shreeya Patel
2023-05-20 15:59   ` Jonathan Cameron
2023-05-16 23:00 ` [PATCH 3/7] iio: adc: rockchip_saradc: Use of_device_get_match_data Shreeya Patel
2023-05-17 10:34   ` AngeloGioacchino Del Regno
2023-05-16 23:00 ` [PATCH 4/7] iio: adc: rockchip_saradc: Match alignment with open parenthesis Shreeya Patel
2023-05-17 10:34   ` AngeloGioacchino Del Regno
2023-05-16 23:00 ` [PATCH 5/7] iio: adc: rockchip_saradc: Use dev_err_probe Shreeya Patel
2023-05-17 10:40   ` AngeloGioacchino Del Regno
2023-05-20 15:49     ` Jonathan Cameron
2023-05-16 23:00 ` [PATCH 6/7] arm64: dts: rockchip: Add DT node for ADC support in RK3588 Shreeya Patel
2023-05-17 10:41   ` AngeloGioacchino Del Regno
2023-05-16 23:00 ` [PATCH 7/7] dt-bindings: iio: adc: Add rockchip,rk3588-saradc string Shreeya Patel
2023-05-17  8:49   ` Krzysztof Kozlowski
2023-05-20 16:00 ` [PATCH 0/7] RK3588 ADC support Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).