| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <af53ae18-d997-4afa-a3e5-7ff5f624bdc3@baylibre.com>
Date: Tue, 13 May 2025 11:59:47 -0500
From: David Lechner <dlechner@...libre.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: jic23@...nel.org, lars@...afoo.de, Michael.Hennerich@...log.com,
nuno.sa@...log.com, andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, linus.walleij@...aro.org, brgl@...ev.pl,
marcelo.schmitt1@...il.com
Subject: Re: [PATCH v3 07/10] iio: adc: ad4170: Add clock provider support
On 5/13/25 7:35 AM, Marcelo Schmitt wrote:
> The AD4170 chip can use an externally supplied clock at the XTAL2 pin, or
> an external crystal connected to the XTAL1 and XTAL2 pins. Alternatively,
> the AD4170 can provide its 16 MHz internal clock at the XTAL2 pin. Extend
> the AD4170 driver so it effectively uses the provided external clock, if
> any, or supplies its own clock as a clock provider.
Is support for CLKDIV intentionally omitted? Might be worth mentioning
if that is the case.
>
> Reviewed-by: Nuno Sá <nuno.sa@...log.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> ---
...
> @@ -329,6 +348,9 @@ struct ad4170_state {
> struct completion completion;
> int pins_fn[AD4170_NUM_ANALOG_PINS];
> u32 int_pin_sel;
> + struct clk_hw int_clk_hw;
> + struct clk *ext_clk;
This isn't used outside of ad4170_clock_select() so can be made a local variable there.
> + unsigned int clock_ctrl;
> /*
> * DMA (thus cache coherency maintenance) requires the transfer buffers
> * to live in their own cache lines.
> @@ -1656,6 +1678,120 @@ static int ad4170_parse_channels(struct iio_dev *indio_dev)
> return 0;
> }
>
> +static struct ad4170_state *clk_hw_to_ad4170(struct clk_hw *hw)
> +{
> + return container_of(hw, struct ad4170_state, int_clk_hw);
> +}
> +
> +static unsigned long ad4170_sel_clk(struct ad4170_state *st,
> + unsigned int clk_sel)
> +{
> + st->clock_ctrl &= ~AD4170_CLOCK_CTRL_CLOCKSEL_MSK;
> + st->clock_ctrl |= FIELD_PREP(AD4170_CLOCK_CTRL_CLOCKSEL_MSK, clk_sel);
Do we need to claim direct mode here to avoid poking registers during
a buffered read?
> + return regmap_write(st->regmap, AD4170_CLOCK_CTRL_REG, st->clock_ctrl);
> +}
> +
> +static unsigned long ad4170_clk_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + return AD4170_INT_CLOCK_16MHZ;
> +}
> +
> +static int ad4170_clk_output_is_enabled(struct clk_hw *hw)
> +{
> + struct ad4170_state *st = clk_hw_to_ad4170(hw);
> + u32 clk_sel;
> +
> + clk_sel = FIELD_GET(AD4170_CLOCK_CTRL_CLOCKSEL_MSK, st->clock_ctrl);
> + return clk_sel == AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT;
> +}
> +
> +static int ad4170_clk_output_prepare(struct clk_hw *hw)
> +{
> + struct ad4170_state *st = clk_hw_to_ad4170(hw);
> +
> + return ad4170_sel_clk(st, AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT);
> +}
> +
> +static void ad4170_clk_output_unprepare(struct clk_hw *hw)
> +{
> + struct ad4170_state *st = clk_hw_to_ad4170(hw);
> +
> + ad4170_sel_clk(st, AD4170_CLOCK_CTRL_CLOCKSEL_INT);
> +}
> +
> +static const struct clk_ops ad4170_int_clk_ops = {
> + .recalc_rate = ad4170_clk_recalc_rate,
> + .is_enabled = ad4170_clk_output_is_enabled,
> + .prepare = ad4170_clk_output_prepare,
> + .unprepare = ad4170_clk_output_unprepare,
> +};
> +
> +static int ad4170_register_clk_provider(struct iio_dev *indio_dev)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> + struct device *dev = indio_dev->dev.parent;
> + struct clk_init_data init = {};
> + int ret;
> +
> + if (!IS_ENABLED(CONFIG_COMMON_CLK))
Driver depends on COMMON_CLK so isn't this dead code?
> + return 0;
> +
> + if (device_property_read_string(dev, "clock-output-names", &init.name)) {
> + init.name = devm_kasprintf(dev, GFP_KERNEL, "%pfw",
> + dev_fwnode(dev));
> + if (!init.name)
> + return -ENOMEM;
> + }
> +
> + init.ops = &ad4170_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 ad4170_clock_select(struct iio_dev *indio_dev)
> +{
> + struct ad4170_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->spi->dev;
> + int ret;
> +
> + st->mclk_hz = AD4170_INT_CLOCK_16MHZ;
Would make more sense to move this inside of the if (ret < 0) instead of
writing over it later.
> + ret = device_property_match_property_string(dev, "clock-names",
> + ad4170_clk_sel,
> + ARRAY_SIZE(ad4170_clk_sel));
> + if (ret < 0) {
> + /* Use internal clock reference */
> + st->clock_ctrl |= FIELD_PREP(AD4170_CLOCK_CTRL_CLOCKSEL_MSK,
> + AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT);
Seems like we would want to start in the AD4170_CLOCK_CTRL_CLOCKSEL_INT state.
Also, could skip registering clock provider if #clock-cells is not present.
> + return ad4170_register_clk_provider(indio_dev);
> + }
> +
> + /* Use external clock reference */
> + st->ext_clk = devm_clk_get_enabled(dev, ad4170_clk_sel[ret]);
> + if (IS_ERR(st->ext_clk))
> + return dev_err_probe(dev, PTR_ERR(st->ext_clk),
> + "Failed to get external clock\n");
> +
> + st->clock_ctrl |= FIELD_PREP(AD4170_CLOCK_CTRL_CLOCKSEL_MSK,
> + AD4170_CLOCK_CTRL_CLOCKSEL_EXT + ret);
> +
> + st->mclk_hz = clk_get_rate(st->ext_clk);
> + if (st->mclk_hz < AD4170_EXT_CLOCK_MHZ_MIN ||
> + st->mclk_hz > AD4170_EXT_CLOCK_MHZ_MAX) {
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid external clock frequency %u\n",
> + st->mclk_hz);
> + }
> +
> + return 0;
> +}
> +
> static int ad4170_parse_firmware(struct iio_dev *indio_dev)
> {
> struct ad4170_state *st = iio_priv(indio_dev);
> @@ -1663,7 +1799,13 @@ static int ad4170_parse_firmware(struct iio_dev *indio_dev)
> int reg_data, ret;
> unsigned int i;
>
> - st->mclk_hz = AD4170_INT_CLOCK_16MHZ;
> + ret = ad4170_clock_select(indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to setup device clock\n");
> +
> + ret = regmap_write(st->regmap, AD4170_CLOCK_CTRL_REG, st->clock_ctrl);
> + if (ret)
> + return ret;
Why not just do the regmap_write() in ad4170_clock_select() to keep it all
together?
>
> for (i = 0; i < AD4170_NUM_ANALOG_PINS; i++)
> st->pins_fn[i] = AD4170_PIN_UNASIGNED;
Powered by blists - more mailing lists