LinuxLists.cc - [PATCH v2 1/3] iio: adc: ad7192: Clean up dev

2024-06-05 07:52:49

Subject: [PATCH v2 1/3] iio: adc: ad7192: Clean up dev

Clean up by using a local variable struct device *dev.

Signed-off-by: Alisa-Dariana Roman <[email protected]>
---
drivers/iio/adc/ad7192.c | 45 ++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 0789121236d6..f06cb7ac4b42 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -1196,17 +1196,18 @@ static void ad7192_reg_disable(void *reg)

static int ad7192_probe(struct spi_device *spi)
{
+ struct device *dev = &spi->dev;
struct ad7192_state *st;
struct iio_dev *indio_dev;
struct regulator *aincom;
int ret;

if (!spi->irq) {
- dev_err(&spi->dev, "no IRQ?\n");
+ dev_err(dev, "no IRQ?\n");
return -ENODEV;
}

- indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
if (!indio_dev)
return -ENOMEM;

@@ -1219,71 +1220,71 @@ static int ad7192_probe(struct spi_device *spi)
* Newer firmware should provide a zero volt fixed supply if wired to
* ground.
*/
- aincom = devm_regulator_get_optional(&spi->dev, "aincom");
+ aincom = devm_regulator_get_optional(dev, "aincom");
if (IS_ERR(aincom)) {
if (PTR_ERR(aincom) != -ENODEV)
- return dev_err_probe(&spi->dev, PTR_ERR(aincom),
+ return dev_err_probe(dev, PTR_ERR(aincom),
"Failed to get AINCOM supply\n");

st->aincom_mv = 0;
} else {
ret = regulator_enable(aincom);
if (ret)
- return dev_err_probe(&spi->dev, ret,
+ return dev_err_probe(dev, ret,
"Failed to enable specified AINCOM supply\n");

- ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
+ ret = devm_add_action_or_reset(dev, ad7192_reg_disable, aincom);
if (ret)
return ret;

ret = regulator_get_voltage(aincom);
if (ret < 0)
- return dev_err_probe(&spi->dev, ret,
+ return dev_err_probe(dev, ret,
"Device tree error, AINCOM voltage undefined\n");
st->aincom_mv = ret / MILLI;
}

- st->avdd = devm_regulator_get(&spi->dev, "avdd");
+ st->avdd = devm_regulator_get(dev, "avdd");
if (IS_ERR(st->avdd))
return PTR_ERR(st->avdd);

ret = regulator_enable(st->avdd);
if (ret) {
- dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
+ dev_err(dev, "Failed to enable specified AVdd supply\n");
return ret;
}

- ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->avdd);
+ ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st->avdd);
if (ret)
return ret;

- ret = devm_regulator_get_enable(&spi->dev, "dvdd");
+ ret = devm_regulator_get_enable(dev, "dvdd");
if (ret)
- return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVdd supply\n");
+ return dev_err_probe(dev, ret, "Failed to enable specified DVdd supply\n");

- st->vref = devm_regulator_get_optional(&spi->dev, "vref");
+ st->vref = devm_regulator_get_optional(dev, "vref");
if (IS_ERR(st->vref)) {
if (PTR_ERR(st->vref) != -ENODEV)
return PTR_ERR(st->vref);

ret = regulator_get_voltage(st->avdd);
if (ret < 0)
- return dev_err_probe(&spi->dev, ret,
+ return dev_err_probe(dev, ret,
"Device tree error, AVdd voltage undefined\n");
} else {
ret = regulator_enable(st->vref);
if (ret) {
- dev_err(&spi->dev, "Failed to enable specified Vref supply\n");
+ dev_err(dev, "Failed to enable specified Vref supply\n");
return ret;
}

- ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->vref);
+ ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st->vref);
if (ret)
return ret;

ret = regulator_get_voltage(st->vref);
if (ret < 0)
- return dev_err_probe(&spi->dev, ret,
+ return dev_err_probe(dev, ret,
"Device tree error, Vref voltage undefined\n");
}
st->int_vref_mv = ret / 1000;
@@ -1305,13 +1306,13 @@ static int ad7192_probe(struct spi_device *spi)
if (ret)
return ret;

- ret = devm_ad_sd_setup_buffer_and_trigger(&spi->dev, indio_dev);
+ ret = devm_ad_sd_setup_buffer_and_trigger(dev, indio_dev);
if (ret)
return ret;

st->fclk = AD7192_INT_FREQ_MHZ;

- st->mclk = devm_clk_get_optional_enabled(&spi->dev, "mclk");
+ st->mclk = devm_clk_get_optional_enabled(dev, "mclk");
if (IS_ERR(st->mclk))
return PTR_ERR(st->mclk);

@@ -1321,17 +1322,17 @@ static int ad7192_probe(struct spi_device *spi)
st->clock_sel == AD7192_CLK_EXT_MCLK2) {
st->fclk = clk_get_rate(st->mclk);
if (!ad7192_valid_external_frequency(st->fclk)) {
- dev_err(&spi->dev,
+ dev_err(dev,
"External clock frequency out of bounds\n");
return -EINVAL;
}
}

- ret = ad7192_setup(indio_dev, &spi->dev);
+ ret = ad7192_setup(indio_dev, dev);
if (ret)
return ret;

- return devm_iio_device_register(&spi->dev, indio_dev);
+ return devm_iio_device_register(dev, indio_dev);
}

static const struct of_device_id ad7192_of_match[] = {
--
2.34.1

2024-06-05 07:53:26

by Alisa-Dariana Roman

[permalink] [raw]

Subject: [PATCH v2 2/3] dt-bindings: iio: adc: ad7192: Fix clock config

2024-06-05 08:07:42

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] dt-bindings: iio: adc: ad7192: Fix clock config

On 05/06/2024 09:51, Alisa-Dariana Roman wrote:
> There are actually 4 configuration modes of clock source for AD719X
> devices. Either a crystal can be attached externally between MCLK1 and
> MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
> pin. The other 2 modes make use of the 4.92MHz internal clock, which can
> be made available on the MCLK2 pin.
>
> The presence of an external clock is optional, not required.

Why?

>
> Fixes: f7356e47032c ("dt-bindings: iio: adc: ad7192: Add binding documentation for AD7192")
> Signed-off-by: Alisa-Dariana Roman <[email protected]>
> ---
> .../bindings/iio/adc/adi,ad7192.yaml | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index a03da9489ed9..c5a4219a9388 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> @@ -39,11 +39,16 @@ properties:
>
> clocks:
> maxItems: 1
> - description: phandle to the master clock (mclk)
> + description: |
> + Optionally, either a crystal can be attached externally between MCLK1 and
> + MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
> + pin. If absent, internal 4.92MHz clock is used which can be made available
> + on MCLK2.
>
> clock-names:
> - items:
> - - const: mclk
> + enum:
> + - xtal
> + - clk

This mclk->clk breaks the users.

Best regards,
Krzysztof

2024-06-05 08:13:39

by Alisa-Dariana Roman

[permalink] [raw]

Subject: [PATCH v2 3/3] iio: adc: ad7192: Fix clock config

There are actually 4 configuration modes of clock source for AD719X
devices. Either a crystal can be attached externally between MCLK1 and
MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
pin. The other 2 modes make use of the 4.92MHz internal clock, which can
be made available on the MCLK2 pin.

Rename mclk to ext_clk for clarity.

Note that the fix tag is for the commit that moved the driver out of
staging.

Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
Signed-off-by: Alisa-Dariana Roman <[email protected]>
---
drivers/iio/adc/ad7192.c | 153 ++++++++++++++++++++++++++++++---------
1 file changed, 119 insertions(+), 34 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index f06cb7ac4b42..75b0724142b1 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -8,6 +8,7 @@
#include <linux/interrupt.h>
#include <linux/bitfield.h>
#include <linux/clk.h>
+#include <linux/clk-provider.h>
#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/slab.h>
@@ -202,7 +203,8 @@ struct ad7192_state {
const struct ad7192_chip_info *chip_info;
struct regulator *avdd;
struct regulator *vref;
- struct clk *mclk;
+ struct clk *ext_clk;
+ struct clk_hw int_clk_hw;
u16 int_vref_mv;
u32 aincom_mv;
u32 fclk;
@@ -398,27 +400,6 @@ static inline bool ad7192_valid_external_frequency(u32 freq)
freq <= AD7192_EXT_FREQ_MHZ_MAX);
}

-static int ad7192_clock_select(struct ad7192_state *st)
-{
- struct device *dev = &st->sd.spi->dev;
- unsigned int clock_sel;
-
- clock_sel = AD7192_CLK_INT;
-
- /* use internal clock */
- if (!st->mclk) {
- if (device_property_read_bool(dev, "adi,int-clock-output-enable"))
- clock_sel = AD7192_CLK_INT_CO;
- } else {
- if (device_property_read_bool(dev, "adi,clock-xtal"))
- clock_sel = AD7192_CLK_EXT_MCLK1_2;
- else
- clock_sel = AD7192_CLK_EXT_MCLK2;
- }
-
- return clock_sel;
-}
-
static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev)
{
struct ad7192_state *st = iio_priv(indio_dev);
@@ -1194,6 +1175,96 @@ static void ad7192_reg_disable(void *reg)
regulator_disable(reg);
}

+static const char *const ad7192_clock_names[] = {
+ "xtal",
+ "clk"
+};
+
+static struct ad7192_state *clk_hw_to_ad7192(struct clk_hw *hw)
+{
+ return container_of(hw, struct ad7192_state, int_clk_hw);
+}
+
+static void ad7192_clk_disable_unprepare(void *clk)
+{
+ clk_disable_unprepare(clk);
+}
+
+static unsigned long ad7192_clk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ return AD7192_INT_FREQ_MHZ;
+}
+
+static int ad7192_clk_output_is_enabled(struct clk_hw *hw)
+{
+ struct ad7192_state *st = clk_hw_to_ad7192(hw);
+
+ return st->clock_sel == AD7192_CLK_INT_CO;
+}
+
+static int ad7192_clk_prepare(struct clk_hw *hw)
+{
+ struct ad7192_state *st = clk_hw_to_ad7192(hw);
+ int ret;
+
+ st->mode &= ~AD7192_MODE_CLKSRC_MASK;
+ st->mode |= AD7192_CLK_INT_CO;
+
+ ret = ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
+ if (ret)
+ return ret;
+
+ st->clock_sel = AD7192_CLK_INT_CO;
+
+ return 0;
+}
+
+static void ad7192_clk_unprepare(struct clk_hw *hw)
+{
+ struct ad7192_state *st = clk_hw_to_ad7192(hw);
+ int ret;
+
+ st->mode &= ~AD7192_MODE_CLKSRC_MASK;
+ st->mode |= AD7192_CLK_INT;
+
+ ret = ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
+ if (ret)
+ return;
+
+ st->clock_sel = AD7192_CLK_INT;
+}
+
+static const struct clk_ops ad7192_int_clk_ops = {
+ .recalc_rate = ad7192_clk_recalc_rate,
+ .is_enabled = ad7192_clk_output_is_enabled,
+ .prepare = ad7192_clk_prepare,
+ .unprepare = ad7192_clk_unprepare,
+};
+
+static int ad7192_register_clk_provider(struct iio_dev *indio_dev)
+{
+ struct ad7192_state *st = iio_priv(indio_dev);
+ struct device *dev = indio_dev->dev.parent;
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
+ struct clk_init_data init = {};
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_COMMON_CLK))
+ return 0;
+
+ init.name = fwnode_get_name(fwnode);
+ init.ops = &ad7192_int_clk_ops;
+
+ st->int_clk_hw.init = &init;
+ ret = devm_clk_hw_register(dev, &st->int_clk_hw);
+ if (ret)
+ return ret;
+
+ return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+ &st->int_clk_hw);
+}
+
static int ad7192_probe(struct spi_device *spi)
{
struct device *dev = &spi->dev;
@@ -1312,20 +1383,34 @@ static int ad7192_probe(struct spi_device *spi)

st->fclk = AD7192_INT_FREQ_MHZ;

- st->mclk = devm_clk_get_optional_enabled(dev, "mclk");
- if (IS_ERR(st->mclk))
- return PTR_ERR(st->mclk);
+ ret = device_property_match_property_string(dev, "clock-names",
+ ad7192_clock_names,
+ ARRAY_SIZE(ad7192_clock_names));
+ if (ret < 0) {
+ st->clock_sel = AD7192_CLK_INT;
+ st->fclk = AD7192_INT_FREQ_MHZ;

- st->clock_sel = ad7192_clock_select(st);
+ ret = ad7192_register_clk_provider(indio_dev);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Registration of clock provider failed\n");
+ } else {
+ st->clock_sel = AD7192_CLK_EXT_MCLK1_2 + ret;

- if (st->clock_sel == AD7192_CLK_EXT_MCLK1_2 ||
- st->clock_sel == AD7192_CLK_EXT_MCLK2) {
- st->fclk = clk_get_rate(st->mclk);
- if (!ad7192_valid_external_frequency(st->fclk)) {
- dev_err(dev,
- "External clock frequency out of bounds\n");
- return -EINVAL;
- }
+ st->ext_clk = devm_clk_get_enabled(dev, ad7192_clock_names[ret]);
+ if (IS_ERR(st->ext_clk))
+ return PTR_ERR(st->ext_clk);
+
+ ret = devm_add_action_or_reset(dev,
+ ad7192_clk_disable_unprepare,
+ st->ext_clk);
+ if (ret)
+ return ret;
+
+ st->fclk = clk_get_rate(st->ext_clk);
+ if (!ad7192_valid_external_frequency(st->fclk))
+ return dev_err_probe(dev, -EINVAL,
+ "External clock frequency out of bounds\n");
}

ret = ad7192_setup(indio_dev, dev);
--
2.34.1

2024-06-05 08:17:31

by Alisa-Dariana Roman

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] iio: adc: ad7192: Clean up dev

Hello! I apologize for the messed up cover letter!

Please consider applying the commits in order!

Kind regards,
Alisa-Dariana Roman

2024-06-05 08:28:27

by Nuno Sá

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] dt-bindings: iio: adc: ad7192: Fix clock config

2024-06-05 08:38:06

by Nuno Sá

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] iio: adc: ad7192: Clean up dev

On Wed, 2024-06-05 at 10:51 +0300, Alisa-Dariana Roman wrote:
> Clean up by using a local variable struct device *dev.
>
> Signed-off-by: Alisa-Dariana Roman <[email protected]>
> ---
> drivers/iio/adc/ad7192.c | 45 ++++++++++++++++++++--------------------
> 1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 0789121236d6..f06cb7ac4b42 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -1196,17 +1196,18 @@ static void ad7192_reg_disable(void *reg)
>
> static int ad7192_probe(struct spi_device *spi)
> {
> + struct device *dev = &spi->dev;
> struct ad7192_state *st;
> struct iio_dev *indio_dev;
> struct regulator *aincom;
> int ret;
>
> if (!spi->irq) {
> - dev_err(&spi->dev, "no IRQ?\n");
> + dev_err(dev, "no IRQ?\n");

Since you're here, dev_err_probe()?

> return -ENODEV;
> }
>
> - indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> if (!indio_dev)
> return -ENOMEM;
>
> @@ -1219,71 +1220,71 @@ static int ad7192_probe(struct spi_device *spi)
> * Newer firmware should provide a zero volt fixed supply if wired to
> * ground.
> */
> - aincom = devm_regulator_get_optional(&spi->dev, "aincom");
> + aincom = devm_regulator_get_optional(dev, "aincom");
> if (IS_ERR(aincom)) {
> if (PTR_ERR(aincom) != -ENODEV)
> - return dev_err_probe(&spi->dev, PTR_ERR(aincom),
> + return dev_err_probe(dev, PTR_ERR(aincom),
>      "Failed to get AINCOM
> supply\n");
>
> st->aincom_mv = 0;
> } else {
> ret = regulator_enable(aincom);
> if (ret)
> - return dev_err_probe(&spi->dev, ret,
> + return dev_err_probe(dev, ret,
>      "Failed to enable specified
> AINCOM supply\n");
>
> - ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable,
> aincom);
> + ret = devm_add_action_or_reset(dev, ad7192_reg_disable,
> aincom);
> if (ret)
> return ret;
>
> ret = regulator_get_voltage(aincom);
> if (ret < 0)
> - return dev_err_probe(&spi->dev, ret,
> + return dev_err_probe(dev, ret,
>      "Device tree error, AINCOM
> voltage undefined\n");
> st->aincom_mv = ret / MILLI;
> }
>
> - st->avdd = devm_regulator_get(&spi->dev, "avdd");
> + st->avdd = devm_regulator_get(dev, "avdd");
> if (IS_ERR(st->avdd))
> return PTR_ERR(st->avdd);
>
> ret = regulator_enable(st->avdd);
> if (ret) {
> - dev_err(&spi->dev, "Failed to enable specified AVdd
> supply\n");
> + dev_err(dev, "Failed to enable specified AVdd supply\n");

ditto

> return ret;
> }
>
> - ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st-
> >avdd);
> + ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st->avdd);
> if (ret)
> return ret;
>
> - ret = devm_regulator_get_enable(&spi->dev, "dvdd");
> + ret = devm_regulator_get_enable(dev, "dvdd");
> if (ret)
> - return dev_err_probe(&spi->dev, ret, "Failed to enable
> specified DVdd supply\n");
> + return dev_err_probe(dev, ret, "Failed to enable specified
> DVdd supply\n");
>
> - st->vref = devm_regulator_get_optional(&spi->dev, "vref");
> + st->vref = devm_regulator_get_optional(dev, "vref");
> if (IS_ERR(st->vref)) {
> if (PTR_ERR(st->vref) != -ENODEV)
> return PTR_ERR(st->vref);
>
> ret = regulator_get_voltage(st->avdd);
> if (ret < 0)
> - return dev_err_probe(&spi->dev, ret,
> + return dev_err_probe(dev, ret,
>      "Device tree error, AVdd voltage
> undefined\n");
> } else {
> ret = regulator_enable(st->vref);
> if (ret) {
> - dev_err(&spi->dev, "Failed to enable specified Vref
> supply\n");
> + dev_err(dev, "Failed to enable specified Vref
> supply\n");

ditto

> return ret;
> }
>
> - ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable,
> st->vref);
> + ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st-
> >vref);
> if (ret)
> return ret;
>
> ret = regulator_get_voltage(st->vref);
> if (ret < 0)
> - return dev_err_probe(&spi->dev, ret,
> + return dev_err_probe(dev, ret,
>      "Device tree error, Vref voltage
> undefined\n");
> }
> st->int_vref_mv = ret / 1000;
> @@ -1305,13 +1306,13 @@ static int ad7192_probe(struct spi_device *spi)
> if (ret)
> return ret;
>
> - ret = devm_ad_sd_setup_buffer_and_trigger(&spi->dev, indio_dev);
> + ret = devm_ad_sd_setup_buffer_and_trigger(dev, indio_dev);
> if (ret)
> return ret;
>
> st->fclk = AD7192_INT_FREQ_MHZ;
>
> - st->mclk = devm_clk_get_optional_enabled(&spi->dev, "mclk");
> + st->mclk = devm_clk_get_optional_enabled(dev, "mclk");
> if (IS_ERR(st->mclk))
> return PTR_ERR(st->mclk);
>
> @@ -1321,17 +1322,17 @@ static int ad7192_probe(struct spi_device *spi)
>     st->clock_sel == AD7192_CLK_EXT_MCLK2) {
> st->fclk = clk_get_rate(st->mclk);
> if (!ad7192_valid_external_frequency(st->fclk)) {
> - dev_err(&spi->dev,
> + dev_err(dev,
> "External clock frequency out of bounds\n");

ditto

- Nuno Sá

2024-06-05 09:05:28

by Nuno Sá

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] iio: adc: ad7192: Fix clock config

On Wed, 2024-06-05 at 10:51 +0300, Alisa-Dariana Roman wrote:
> There are actually 4 configuration modes of clock source for AD719X
> devices. Either a crystal can be attached externally between MCLK1 and
> MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
> pin. The other 2 modes make use of the 4.92MHz internal clock, which can
> be made available on the MCLK2 pin.
>
> Rename mclk to ext_clk for clarity.
>
> Note that the fix tag is for the commit that moved the driver out of
> staging.
>
> Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
> Signed-off-by: Alisa-Dariana Roman <[email protected]>
> ---
> drivers/iio/adc/ad7192.c | 153 ++++++++++++++++++++++++++++++---------
> 1 file changed, 119 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index f06cb7ac4b42..75b0724142b1 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -8,6 +8,7 @@
> #include <linux/interrupt.h>
> #include <linux/bitfield.h>
> #include <linux/clk.h>
> +#include <linux/clk-provider.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/slab.h>
> @@ -202,7 +203,8 @@ struct ad7192_state {
> const struct ad7192_chip_info *chip_info;
> struct regulator *avdd;
> struct regulator *vref;
> - struct clk *mclk;
> + struct clk *ext_clk;
> + struct clk_hw int_clk_hw;
> u16 int_vref_mv;
> u32 aincom_mv;
> u32 fclk;
> @@ -398,27 +400,6 @@ static inline bool ad7192_valid_external_frequency(u32
> freq)
> freq <= AD7192_EXT_FREQ_MHZ_MAX);
> }
>
> -static int ad7192_clock_select(struct ad7192_state *st)
> -{
> - struct device *dev = &st->sd.spi->dev;
> - unsigned int clock_sel;
> -
> - clock_sel = AD7192_CLK_INT;
> -
> - /* use internal clock */
> - if (!st->mclk) {
> - if (device_property_read_bool(dev, "adi,int-clock-output-
> enable"))
> - clock_sel = AD7192_CLK_INT_CO;
> - } else {
> - if (device_property_read_bool(dev, "adi,clock-xtal"))
> - clock_sel = AD7192_CLK_EXT_MCLK1_2;
> - else
> - clock_sel = AD7192_CLK_EXT_MCLK2;
> - }
> -
> - return clock_sel;
> -}
> -
> static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev)
> {
> struct ad7192_state *st = iio_priv(indio_dev);
> @@ -1194,6 +1175,96 @@ static void ad7192_reg_disable(void *reg)
> regulator_disable(reg);
> }
>
> +static const char *const ad7192_clock_names[] = {
> + "xtal",
> + "clk"
> +};

This could be moved closer where it will be used...

> +
> +static struct ad7192_state *clk_hw_to_ad7192(struct clk_hw *hw)
> +{
> + return container_of(hw, struct ad7192_state, int_clk_hw);
> +}
> +
> +static void ad7192_clk_disable_unprepare(void *clk)
> +{
> + clk_disable_unprepare(clk);
> +}
> +
> +static unsigned long ad7192_clk_recalc_rate(struct clk_hw *hw,
> +     unsigned long parent_rate)
> +{
> + return AD7192_INT_FREQ_MHZ;
> +}
> +
> +static int ad7192_clk_output_is_enabled(struct clk_hw *hw)
> +{
> + struct ad7192_state *st = clk_hw_to_ad7192(hw);
> +
> + return st->clock_sel == AD7192_CLK_INT_CO;
> +}
> +
> +static int ad7192_clk_prepare(struct clk_hw *hw)
> +{
> + struct ad7192_state *st = clk_hw_to_ad7192(hw);
> + int ret;
> +
> + st->mode &= ~AD7192_MODE_CLKSRC_MASK;
> + st->mode |= AD7192_CLK_INT_CO;
> +
> + ret = ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> + if (ret)
> + return ret;
> +
> + st->clock_sel = AD7192_CLK_INT_CO;
> +
> + return 0;
> +}
> +
> +static void ad7192_clk_unprepare(struct clk_hw *hw)
> +{
> + struct ad7192_state *st = clk_hw_to_ad7192(hw);
> + int ret;
> +
> + st->mode &= ~AD7192_MODE_CLKSRC_MASK;
> + st->mode |= AD7192_CLK_INT;
> +
> + ret = ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> + if (ret)
> + return;
> +
> + st->clock_sel = AD7192_CLK_INT;
> +}
> +
> +static const struct clk_ops ad7192_int_clk_ops = {
> + .recalc_rate = ad7192_clk_recalc_rate,
> + .is_enabled = ad7192_clk_output_is_enabled,
> + .prepare = ad7192_clk_prepare,
> + .unprepare = ad7192_clk_unprepare,
> +};
> +
> +static int ad7192_register_clk_provider(struct iio_dev *indio_dev)
> +{
> + struct ad7192_state *st = iio_priv(indio_dev);
> + struct device *dev = indio_dev->dev.parent;
> + struct fwnode_handle *fwnode = dev_fwnode(dev);
> + struct clk_init_data init = {};
> + int ret;
> +
> + if (!IS_ENABLED(CONFIG_COMMON_CLK))
> + return 0;
> +
> + init.name = fwnode_get_name(fwnode);
> + init.ops = &ad7192_int_clk_ops;
> +
> + st->int_clk_hw.init = &init;
> + ret = devm_clk_hw_register(dev, &st->int_clk_hw);
> + if (ret)
> + return ret;
> +
> + return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> +    &st->int_clk_hw);
> +}

The above code is unrelated... Should be in another patch (also needs changes in
the bindings). It's also a new feature which does not match much with the series
subject :)

> +
> static int ad7192_probe(struct spi_device *spi)
> {
> struct device *dev = &spi->dev;
> @@ -1312,20 +1383,34 @@ static int ad7192_probe(struct spi_device *spi)
>
> st->fclk = AD7192_INT_FREQ_MHZ;
>
> - st->mclk = devm_clk_get_optional_enabled(dev, "mclk");
> - if (IS_ERR(st->mclk))
> - return PTR_ERR(st->mclk);
> + ret = device_property_match_property_string(dev, "clock-names",
> +     ad7192_clock_names,
> +
> ARRAY_SIZE(ad7192_clock_names));
> + if (ret < 0) {
> + st->clock_sel = AD7192_CLK_INT;
> + st->fclk = AD7192_INT_FREQ_MHZ;
>
> - st->clock_sel = ad7192_clock_select(st);
> + ret = ad7192_register_clk_provider(indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret,
> +      "Registration of clock provider
> failed\n");
> + } else {
> + st->clock_sel = AD7192_CLK_EXT_MCLK1_2 + ret;
>
> - if (st->clock_sel == AD7192_CLK_EXT_MCLK1_2 ||
> -     st->clock_sel == AD7192_CLK_EXT_MCLK2) {
> - st->fclk = clk_get_rate(st->mclk);
> - if (!ad7192_valid_external_frequency(st->fclk)) {
> - dev_err(dev,
> - "External clock frequency out of bounds\n");
> - return -EINVAL;
> - }
> + st->ext_clk = devm_clk_get_enabled(dev,
> ad7192_clock_names[ret]);
> + if (IS_ERR(st->ext_clk))
> + return PTR_ERR(st->ext_clk);
> +
> + ret = devm_add_action_or_reset(dev,
> +        ad7192_clk_disable_unprepare,
> +        st->ext_clk);

No need for this... Check what devm_clk_get_enabled() is doing :)

> + if (ret)
> + return ret;
> +
> + st->fclk = clk_get_rate(st->ext_clk);
> + if (!ad7192_valid_external_frequency(st->fclk))
> + return dev_err_probe(dev, -EINVAL,
> +      "External clock frequency out of
> bounds\n");

Maybe the above could be placed in a proper setup function... Like renaming
ad7192_clock_select() -> ad7192_clock_setup()?

One other thing is, if this is a fix, then it should come first in the series.
The reasoning is that we may want to backport the fix but there's no reason to
backport unneeded patches like your first patch that's only about cosmetics.

- Nuno Sá

2024-06-08 17:50:17

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] iio: adc: ad7192: Fix clock config

On Wed, 5 Jun 2024 10:51:54 +0300
Alisa-Dariana Roman <[email protected]> wrote:

> There are actually 4 configuration modes of clock source for AD719X
> devices. Either a crystal can be attached externally between MCLK1 and
> MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
> pin. The other 2 modes make use of the 4.92MHz internal clock, which can
> be made available on the MCLK2 pin.
>
> Rename mclk to ext_clk for clarity.
>
> Note that the fix tag is for the commit that moved the driver out of
> staging.
>
> Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
> Signed-off-by: Alisa-Dariana Roman <[email protected]>
A few comments inline.

> ---
> drivers/iio/adc/ad7192.c | 153 ++++++++++++++++++++++++++++++---------
> 1 file changed, 119 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index f06cb7ac4b42..75b0724142b1 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -8,6 +8,7 @@
> #include <linux/interrupt.h>
> #include <linux/bitfield.h>
> #include <linux/clk.h>
> +#include <linux/clk-provider.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/slab.h>
> @@ -202,7 +203,8 @@ struct ad7192_state {
> const struct ad7192_chip_info *chip_info;
> struct regulator *avdd;
> struct regulator *vref;
> - struct clk *mclk;
> + struct clk *ext_clk;
> + struct clk_hw int_clk_hw;
> u16 int_vref_mv;
> u32 aincom_mv;
> u32 fclk;
> @@ -398,27 +400,6 @@ static inline bool ad7192_valid_external_frequency(u32 freq)
> freq <= AD7192_EXT_FREQ_MHZ_MAX);
> }
>

> +
> +static const struct clk_ops ad7192_int_clk_ops = {
> + .recalc_rate = ad7192_clk_recalc_rate,
> + .is_enabled = ad7192_clk_output_is_enabled,
> + .prepare = ad7192_clk_prepare,
> + .unprepare = ad7192_clk_unprepare,
> +};
> +
> +static int ad7192_register_clk_provider(struct iio_dev *indio_dev)
As Nuno pointed out - new feature, separate patch.

> +{
> + struct ad7192_state *st = iio_priv(indio_dev);
> + struct device *dev = indio_dev->dev.parent;
> + struct fwnode_handle *fwnode = dev_fwnode(dev);
> + struct clk_init_data init = {};
> + int ret;
> +
> + if (!IS_ENABLED(CONFIG_COMMON_CLK))
> + return 0;
> +
> + init.name = fwnode_get_name(fwnode);
> + init.ops = &ad7192_int_clk_ops;
> +
> + st->int_clk_hw.init = &init;
> + ret = devm_clk_hw_register(dev, &st->int_clk_hw);
> + if (ret)
> + return ret;
> +
> + return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> + &st->int_clk_hw);
> +}
> +
> static int ad7192_probe(struct spi_device *spi)
> {
> struct device *dev = &spi->dev;
> @@ -1312,20 +1383,34 @@ static int ad7192_probe(struct spi_device *spi)
>
> st->fclk = AD7192_INT_FREQ_MHZ;
>
> - st->mclk = devm_clk_get_optional_enabled(dev, "mclk");
> - if (IS_ERR(st->mclk))
> - return PTR_ERR(st->mclk);
> + ret = device_property_match_property_string(dev, "clock-names",
> + ad7192_clock_names,
> + ARRAY_SIZE(ad7192_clock_names));
> + if (ret < 0) {
> + st->clock_sel = AD7192_CLK_INT;
> + st->fclk = AD7192_INT_FREQ_MHZ;
>
> - st->clock_sel = ad7192_clock_select(st);
> + ret = ad7192_register_clk_provider(indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Registration of clock provider failed\n");
> + } else {
> + st->clock_sel = AD7192_CLK_EXT_MCLK1_2 + ret;
>
> - if (st->clock_sel == AD7192_CLK_EXT_MCLK1_2 ||
> - st->clock_sel == AD7192_CLK_EXT_MCLK2) {
> - st->fclk = clk_get_rate(st->mclk);
> - if (!ad7192_valid_external_frequency(st->fclk)) {
> - dev_err(dev,
> - "External clock frequency out of bounds\n");
> - return -EINVAL;
> - }
> + st->ext_clk = devm_clk_get_enabled(dev, ad7192_clock_names[ret]);
> + if (IS_ERR(st->ext_clk))
> + return PTR_ERR(st->ext_clk);
> +
> + ret = devm_add_action_or_reset(dev,
> + ad7192_clk_disable_unprepare,

I'm confused. Why do you need this if using devm_clk_get_enabled()?
I'd assume that did this for you.

> + st->ext_clk);
> + if (ret)
> + return ret;
> +
> + st->fclk = clk_get_rate(st->ext_clk);
> + if (!ad7192_valid_external_frequency(st->fclk))
> + return dev_err_probe(dev, -EINVAL,
> + "External clock frequency out of bounds\n");
> }
>
> ret = ad7192_setup(indio_dev, dev);