linux-kernel - Re: [PATCH v4 2/2] iio: Aspeed ADC

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 Apr 2017 13:27:44 -0700
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Rick Altherr <raltherr@...gle.com>
Cc:     openbmc@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        linux-iio@...r.kernel.org,
        Quentin Schulz <quentin.schulz@...e-electrons.com>,
        David Lechner <david@...hnology.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        William Breathitt Gray <vilhelm.gray@...il.com>,
        linux-clk@...r.kernel.org, Andreas Klinger <ak@...klinger.de>,
        Marek Vasut <marek.vasut+renesas@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Michael Turquette <mturquette@...libre.com>,
        Rob Herring <robh@...nel.org>,
        Alison Schofield <amsfield22@...il.com>,
        Fabrice Gasnier <fabrice.gasnier@...com>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Crestez Dan Leonard <leonard.crestez@...el.com>,
        Akinobu Mita <akinobu.mita@...il.com>,
        Matt Ranostay <mranostay@...il.com>
Subject: Re: [PATCH v4 2/2] iio: Aspeed ADC

On 03/23, Rick Altherr wrote:
> +
> +static int aspeed_adc_probe(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev;
> +	struct aspeed_adc_data *data;
> +	const struct aspeed_adc_model_data *model_data;
> +	struct resource *res;
> +	const char *clk_parent_name;
> +	int ret;
> +	u32 adc_engine_control_reg_val;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	data->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data->base))
> +		return PTR_ERR(data->base);
> +
> +	/* Register ADC clock prescaler with source specified by device tree. */
> +	spin_lock_init(&data->clk_lock);
> +	clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);

What if the parent clk is not registered yet? Or if we're not
always using DT in this driver? Put another way, this code is
fragile. But I guess it probably works well enough for now so no
big deal, just pointing out my fear.

> +
> +	data->clk_prescaler = clk_hw_register_divider(
> +				&pdev->dev, "prescaler", clk_parent_name, 0,
> +				data->base + ASPEED_REG_CLOCK_CONTROL,
> +				17, 15, 0, &data->clk_lock);
> +	if (IS_ERR(data->clk_prescaler))
> +		return PTR_ERR(data->clk_prescaler);
> +
> +	/*
> +	 * Register ADC clock scaler downstream from the prescaler. Allow rate
> +	 * setting to adjust the prescaler as well.
> +	 */
> +	data->clk_scaler = clk_hw_register_divider(
> +				&pdev->dev, "scaler", "prescaler",
> +				CLK_SET_RATE_PARENT,
> +				data->base + ASPEED_REG_CLOCK_CONTROL,
> +				0, 10, 0, &data->clk_lock);
> +	if (IS_ERR(data->clk_scaler)) {
> +		ret = PTR_ERR(data->clk_scaler);
> +		goto scaler_error;
> +	}
> +
> +	/* Start all channels in normal mode. */
> +	clk_prepare_enable(data->clk_scaler->clk);

Eventually we'd like to get rid of hw->clk pointer so that users
have to call some sort of clk_get() API and then we get warm
fuzzies from knowing who is consuming a clk structure. Can you
change this to register a clk provider and call clk_get()? I
think a device that references itself should be OK in DT still,
and would properly reflect what's going on.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project