Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755952AbdDEU1y (ORCPT ); Wed, 5 Apr 2017 16:27:54 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:33880 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755590AbdDEU1r (ORCPT ); Wed, 5 Apr 2017 16:27:47 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 05858607A2 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=sboyd@codeaurora.org Date: Wed, 5 Apr 2017 13:27:44 -0700 From: Stephen Boyd To: Rick Altherr Cc: openbmc@lists.ozlabs.org, linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, Quentin Schulz , David Lechner , Geert Uytterhoeven , William Breathitt Gray , linux-clk@vger.kernel.org, Andreas Klinger , Marek Vasut , Jonathan Cameron , Peter Meerwald-Stadler , Martin Blumenstingl , Michael Turquette , Rob Herring , Alison Schofield , Fabrice Gasnier , Hartmut Knaack , Lars-Peter Clausen , Crestez Dan Leonard , Akinobu Mita , Matt Ranostay Subject: Re: [PATCH v4 2/2] iio: Aspeed ADC Message-ID: <20170405202744.GB7065@codeaurora.org> References: <20170323184136.7349-1-raltherr@google.com> <20170323184136.7349-2-raltherr@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170323184136.7349-2-raltherr@google.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2375 Lines: 68 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