On 07/18, Tirupathi Reddy T wrote:
>
> On 7/15/2017 2:38 AM, Stephen Boyd wrote:
> >On 07/13, Tirupathi Reddy wrote:
> >>diff --git a/Documentation/devicetree/bindings/clock/clk-qpnp-div.txt b/Documentation/devicetree/bindings/clock/clk-qpnp-div.txt
> >>new file mode 100644
> >>index 0000000..03b7b70
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/clock/clk-qpnp-div.txt
> >>@@ -0,0 +1,52 @@
> >>+Qualcomm Technologies, Inc. QPNP clock divider (clkdiv)
> >>+
> >>+clkdiv configures the clock frequency of a set of outputs on the PMIC.
> >>+These clocks are typically wired through alternate functions on
> >>+gpio pins.
> >>+
> >>+=======================
> >>+Supported Properties
> >>+=======================
> >>+
> >>+- compatible
> >>+ Usage: required
> >>+ Value type: <string>
> >>+ Definition: should be "qcom,qpnp-clkdiv".
> >>+
> >>+- reg
> >>+ Usage: required
> >>+ Value type: <prop-encoded-array>
> >>+ Definition: Addresses and sizes for the memory of this CLKDIV
> >>+ peripheral.
> >>+
> >>+- qcom,cxo-freq
> >>+ Usage: required
> >>+ Value type: <u32>
> >>+ Definition: The frequency of the crystal oscillator (CXO) clock in Hz.
> >CXO should be a parent clk then. This could have clocks and
> >clock-names properties for that and then be hooked up to
> >xo_board.
> Sure. I will address in the next patch set.
> >>+
> >>+- qcom,clkdiv-id
> >>+ Usage: required
> >>+ Value type: <u32>
> >>+ Definition: Integer value specifies the hardware identifier of this
> >>+ CLKDIV peripheral.
> >This is to name the clk? You could use clock-output-names as
> >that's more standard. But this is also sort of confusing. If
> >there are multiple clkdivs then it would be good to combine them
> >into one device node assuming they're all next to each other.
> >This would be similar to how we handle gpios and regulators. Then
> >the naming is simple enough to be an incrementing number.
> Yes, multiple clkdivs present. We add sub-nodes for each clkdiv and
> parse them inside driver as separate clock.
Please just roll them all into one node instead of a node per
clk on the PMIC.
> >>+#define Q_DIV_PERIOD_NS(cxo_clk, div) (NSEC_PER_SEC / (cxo_clk / div))
> >>+#define Q_ENABLE_DELAY_NS(cxo_clk, div) (2 * Q_CXO_PERIOD_NS(cxo_clk) + \
> >>+ (3 * Q_DIV_PERIOD_NS(cxo_clk, div)))
> >>+#define Q_DISABLE_DELAY_NS(cxo_clk, div) \
> >>+ (3 * Q_DIV_PERIOD_NS(cxo_clk, div))
> >>+
> >>+#define CLK_QPNP_DIV_OFFSET 1
> >>+
> >>+enum q_clk_div_factor {
> >>+ Q_CLKDIV_XO_DIV_1_0 = 0,
> >>+ Q_CLKDIV_XO_DIV_1,
> >>+ Q_CLKDIV_XO_DIV_2,
> >>+ Q_CLKDIV_XO_DIV_4,
> >>+ Q_CLKDIV_XO_DIV_8,
> >>+ Q_CLKDIV_XO_DIV_16,
> >>+ Q_CLKDIV_XO_DIV_32,
> >>+ Q_CLKDIV_XO_DIV_64,
> >>+ Q_CLKDIV_MAX_ALLOWED,
> >Please make #defines for these instead of enum.
> We used enum primarily for easy debug at runtime. For an example,
> The "div_factor" field
> of "struct q_clkdiv" would always show the enumerated value which
> would help in
> determining the configured absolute divider value rather than
> spending time in mapping
> register bit values to absolute divider value.
Ok. I can't find a good reference, but typically we don't do this
in kernel drivers and just use #defines instead. Please just do
that. It seems simple enough to know to translate the number to a
power of 2 after reading it.
> >
> >>+};
> >>+
> >>+enum q_clkdiv_state {
> >>+ DISABLE = false,
> >>+ ENABLE = true,
> >>+};
> >Uh no. Just use bool.
> Sure. I will address in the next patch set.
> >
> >>+
> >>+struct q_clkdiv {
> >>+ struct regmap *regmap;
> >>+ struct device *dev;
> >>+
> >>+ u16 base;
> >>+ spinlock_t lock;
> >>+
> >>+ /* clock properties */
> >>+ struct clk_hw hw;
> >>+ unsigned int cxo_hz;
> >Drop.
> cxo_hz is required to be populated at driver initialization and
> being used at runtime
> for calculating the div value to be applied.
> >
> >>+ enum q_clk_div_factor div_factor;
> >>+ bool enabled;
> >Shouldn't be needed. Read the hardware instead.
> This enabled field is required for the typical handling of set_rate
> in qpnp_clkdiv_config_freq_div().
You can't read the hardware in set_rate op?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
