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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 5 Apr 2017 18:10:03 -0700
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Rick Altherr <raltherr@...gle.com>
Cc:     OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        Linux Kernel Mailing List <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 04/05, Rick Altherr wrote:
> On Wed, Apr 5, 2017 at 1:27 PM, Stephen Boyd <sboyd@...eaurora.org> wrote:
> > 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.
> 
> I'm not terribly worried about not using DT for this driver as it is
> for an ARM SoC's built-in ADC which is only supported via DT.  I take
> your point though.  What's the right way to do this?  Use clk_get() to
> request by name and clock-names in the DT?

Yes that will work. When we add probe defer to clk_get() things
will work better. Presumably the clocks property already exists
for this device so that of_clk_get_parent_name() works so it's
not a binding change?

> 
> >
> >> +
> >> +     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.
> 
> Do you mean call of_clk_add_hw_provider with of_clk_hw_simple_get or
> something else?  I'm still wrapping my head around the distinction
> between clk, clk_hw, and a clk provider.
> 

Yes. Unless you don't want to expose these details in DT because
it's all internal to the device?

In that case we need to go merge that patch on the clk list to
have clk_hw_create_clk() or something like that, so we can turn a
clk_hw structure into a clk pointer without directly referencing
the clk member of clk_hw.

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