linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: sboyd@codeaurora.org (Stephen Boyd)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860
Date: Thu, 29 Jun 2017 18:41:17 -0700	[thread overview]
Message-ID: <20170630014117.GJ22780@codeaurora.org> (raw)
In-Reply-To: <CAAfSe-s5oPB_G=sSAqVpYkgxhD=AE3nxv-V7-t0v+qw_AtxH3g@mail.gmail.com>

On 06/22, Chunyan Zhang wrote:
> Hi Stephen,
> 
> On 20 June 2017 at 09:41, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 06/18, Chunyan Zhang wrote:
> >> +++ b/include/dt-bindings/clock/sc9860-ccu.h
> >> @@ -0,0 +1,19 @@
> >> +/*
> >> + * Spreadtrum SC9860 platform clocks
> >> + *
> >> + * Copyright (C) 2017, Spreadtrum Communications Inc.
> >> + *
> >> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >> + */
> >> +
> >> +#ifndef _DT_BINDINGS_CLK_SC9860_CCU_H_
> >> +#define _DT_BINDINGS_CLK_SC9860_CCU_H_
> >> +
> >> +#define CLK_FAC_1M   2
> >> +#define CLK_EMMC_2X_EN       29
> >> +#define CLK_L0_409M6 60
> >> +#define CLK_EMMC_2X  88
> >> +#define CLK_EMMC_EB  158
> >
> > Why are only a handful exposed in the header file? Not exposing
> > everything is mostly a maintenance nightmare right now.
> 
> No special reason here, my thought simply was that there's no much
> Spreadtrum's device driver in mainline at present, so most of the
> clocks wouldn't be needed for now, I planned to expose only those when
> the device driver they provide clock to, that's saying when we add a
> new device driver we'll expose the clocks this device needs.

Please no. I don't want to get the one-off patch in my inbox to
add more and more ids here "just because" nobody was using the
numbers before. I really don't care that someone has added the
driver for some random device on their SoC now and so they need
to expose the number into a header file. I know some people are
doing this, and it's really not meaningful. I'm much happier if
we're exposing every single clk number to DT and reducing churn
in the include/dt-bindings/ directory. And really, the raw number
could be used in dt, so the enforcement of any sort of ABI needs
to be in the clk driver anyway.

Now if a clk was left out of the clk driver implementation, then
the number could be left out of the header file, but even then it
doesn't really need to be. The clk driver could be implemented in
a way to return an error if the number doesn't map to a clk_hw
structure in the driver. In the future, the clk could be added to
the driver, and then DT wouldn't need to change and consumer
drivers would start to work when various branches are merged
together.

Also, sometimes clk driver authors don't know about all the clks
that they have on their hardware (I doubt this includes you
because you work at the company making the SoC here). In this
case, it's OK to leave out the ids that aren't known to the
binding author because we can't expect more from these people.
And if you know for a fact that a certain clk will never need to
be exposed in the binding, like some random internal clk that
nobody will care to use, then it's also OK to leave that out from
the binding.

> 
> Another reason is, I'm not sure if there are still some clocks I
> haven't listed for SC9860 , so if we need to add a clock in the
> feature, the value of these macros defined in
> "include/dt-bindings/clock/sc9860-ccu.h" may be changed, that means I
> need to change the index of all clocks following the clock inserted
> into.

When modifying a dt-bindings header file to add more clk ids you
should _never_ modify existing numbers. That would be a backwards
incompatible change of the DT binding. If anything, just keep
adding more numbers to the end of the number space. If something
needs to be removed, make that number map to an error or some
no-op clk_hw structure in the clk driver.

> 
> But why will not exposing everything bring trouble to maintenance? :)
> 

Mostly it's because I spend too much time worrying about these
include/dt-bindings files and how they're going to land in Linus'
tree and not enough time reviewing driver and core framework
patches. I'm trying to reduce the time spent worrying about these
header files to a manageable amount.

Hopefully that's helpful. Sorry for the long email.

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

  reply	other threads:[~2017-06-30  1:41 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-18  1:58 [PATCH V1 0/9] add clock driver for Spreadtrum platforms Chunyan Zhang
2017-06-18  1:58 ` [PATCH V1 1/9] dt-bindings: Add Spreadtrum CCU binding documentation Chunyan Zhang
2017-06-23 20:05   ` Rob Herring
2017-06-18  1:58 ` [PATCH V1 2/9] clk: sprd: Add common infrastructure Chunyan Zhang
2017-06-20  1:29   ` Stephen Boyd
2017-06-22 10:12     ` Chunyan Zhang
2017-06-18  1:58 ` [PATCH V1 3/9] clk: sprd: add gate clock support Chunyan Zhang
2017-06-20  1:31   ` Stephen Boyd
2017-06-22 10:16     ` Chunyan Zhang
2017-06-30  1:43       ` Stephen Boyd
2017-06-18  1:58 ` [PATCH V1 4/9] clk: sprd: add mux " Chunyan Zhang
2017-06-18  1:58 ` [PATCH V1 5/9] clk: sprd: add divider " Chunyan Zhang
2017-06-18  1:58 ` [PATCH V1 6/9] clk: sprd: add composite " Chunyan Zhang
2017-06-19  0:13   ` kbuild test robot
2017-06-18  1:58 ` [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang
2017-06-20  1:37   ` Stephen Boyd
2017-06-22 10:17     ` Chunyan Zhang
2017-06-22 11:15       ` Arnd Bergmann
2017-06-22 12:06         ` Chunyan Zhang
2017-06-30  1:44       ` Stephen Boyd
2017-06-30  7:55         ` Chunyan Zhang
2017-06-30 19:22           ` Stephen Boyd
2017-07-03  7:41             ` Chunyan Zhang
2017-06-18  1:58 ` [PATCH V1 8/9] clk: sprd: add clocks support for SC9860 Chunyan Zhang
2017-06-20  1:41   ` Stephen Boyd
2017-06-22 10:21     ` Chunyan Zhang
2017-06-30  1:41       ` Stephen Boyd [this message]
2017-06-18  1:58 ` [PATCH V1 9/9] arm64: dts: add ccu " Chunyan Zhang
2017-06-20  1:24   ` Stephen Boyd
2017-06-22 10:24     ` Chunyan Zhang
2017-06-30  0:57       ` Stephen Boyd
2017-06-30  7:37         ` Chunyan Zhang
2017-06-20  1:25 ` [PATCH V1 0/9] add clock driver for Spreadtrum platforms Stephen Boyd
2017-06-22 10:07   ` Chunyan Zhang
2017-06-30  0:45     ` Stephen Boyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170630014117.GJ22780@codeaurora.org \
    --to=sboyd@codeaurora.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).