On 01/05/2017 03:13 PM, Laurent Pinchart wrote:
> Hi Marek,
Hi!
[...]
>>>>> +static unsigned long vc5_mux_recalc_rate(struct clk_hw *hw,
>>>>> + unsigned long parent_rate)
>>>>> +{
>>>>> + struct vc5_driver_data *vc5 =
>>>>> + container_of(hw, struct vc5_driver_data, clk_mux);
>>>>> + unsigned long idiv;
>>>>> + u8 div;
>>>>> +
>>>>> + /* FIXME: Needs locking ? */
>>>
>>> Let's fix it then :-)
>>
>> I would like to get feedback on this one, does it ?
>
> That's a question for Mike or Stephen I believe.
OK
>>>>> + /* CLKIN within range of PLL input, feed directly to PLL. */
>>>>> + if (parent_rate <= 50000000) {
>>>>> + regmap_update_bits(vc5->regmap, VC5_VCO_CTRL_AND_PREDIV,
>>>>> +
>>>>> VC5_VCO_CTRL_AND_PREDIV_BYPASS_PREDIV,
>>>>> +
>>>>> VC5_VCO_CTRL_AND_PREDIV_BYPASS_PREDIV);
>>>>> + regmap_update_bits(vc5->regmap, VC5_REF_DIVIDER, 0xff,
>>>>> 0x00);
>>>>> + return parent_rate;
>>>>> + }
>>>>> +
>>>>> + idiv = DIV_ROUND_UP(parent_rate, 50000000);
>>>>> + if (idiv > 127)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + /* We have dedicated div-2 predivider. */
>>>>> + if (idiv == 2)
>>>>> + div = VC5_REF_DIVIDER_SEL_PREDIV2;
>>>>> + else
>>>>> + div = VC5_REF_DIVIDER_REF_DIV(idiv);
>>>>> +
>>>>> + regmap_update_bits(vc5->regmap, VC5_REF_DIVIDER, 0xff, div);
>>>>> + regmap_update_bits(vc5->regmap, VC5_VCO_CTRL_AND_PREDIV,
>>>>> + VC5_VCO_CTRL_AND_PREDIV_BYPASS_PREDIV, 0);
>>>>> +
>>>>> + return parent_rate / idiv;
>>>>> +}
>
> [snip]
>
>>>>> +static int vc5_clk_out_set_parent(struct clk_hw *hw, u8 index)
>>>>> +{
>>>>> + struct vc5_hw_data *hwdata = container_of(hw, struct
>>>>> vc5_hw_data, hw);
>>>>> + struct vc5_driver_data *vc5 = hwdata->vc5;
>>>>> + const u8 mask = VC5_OUT_DIV_CONTROL_RESET |
>>>>> + VC5_OUT_DIV_CONTROL_SELB_NORM |
>>>>> + VC5_OUT_DIV_CONTROL_SEL_EXT |
>>>>> + VC5_OUT_DIV_CONTROL_EN_FOD;
>>>>> + const u8 extclk = VC5_OUT_DIV_CONTROL_SELB_NORM |
>>>>> + VC5_OUT_DIV_CONTROL_SEL_EXT;
>>>>> + u8 src = VC5_OUT_DIV_CONTROL_RESET;
>>>>> +
>>>>> + if (index == 0)
>>>>> + src |= VC5_OUT_DIV_CONTROL_EN_FOD;
>>>>> + else if (index == 1)
>>>>> + src |= extclk;
>>>>> + else
>>>>> + return -EINVAL;
>>>
>>> Can this happen given that the number of parents is set to 2 ?
>>
>> I believe it should not happen, but I'd rather sanitize the input here
>> to be safe. Shall I remove it ?
>
> I'd remove it as it can't happen. Being called with index > 1 would be
> similar
> to being called with hw == NULL, which you don't check explicitly. I don't
> think we should sanitize every input parameter value in every driver when the
> core is supposed to take care of that.
Dropped
The rest is done and I'll sort out the remaining bits (pm, gating) and
repost once I receive the kit.
--
Best regards,
Marek Vasut
