Re: [PATCH v2] clk: meson: Fix the determine rate error in clk_regmap_divider_ro_ops

[Jerome Brunet <jbrunet@baylibre.com>]

On Wed 13 Nov 2024 at 14:55, Chuan Liu <chuan.liu@amlogic.com> wrote:

 file changed, 20 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/clk/meson/clk-regmap.c
>>> b/drivers/clk/meson/clk-regmap.c
>>> index 07f7e441b916..edf65ca92c7a 100644
>>> --- a/drivers/clk/meson/clk-regmap.c
>>> +++ b/drivers/clk/meson/clk-regmap.c
>>> @@ -80,21 +80,6 @@ static int clk_regmap_div_determine_rate(struct
>>> clk_hw *hw,
>>>   {
>>>       struct clk_regmap *clk = to_clk_regmap(hw);
>>>       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>>> -     unsigned int val;
>>> -     int ret;
>>> -
>>> -     /* if read only, just return current value */
>>> -     if (div->flags & CLK_DIVIDER_READ_ONLY) {
>>
>> You're breaking current code by no more checking this flag,
>> the new clk_regmap_div_ro_determine_rate() is fine, but you should call
>> it from here if CLK_DIVIDER_READ_ONLY is set.
>
> My idea is that the newly added clk_regmap_div_ro_determine_rate()

Whatever your idea is, what has been submitted is broken, as Neil
pointed out

> implements the functionality of handling CLK_DIVIDER_READ_ONLY in
> clk_regmap_div_determine_rate(). If we still keep the logic for
> handling CLK_DIVIDER_READ_ONLY here, it will make
> clk_regmap_div_determine_rate() ambiguous and easily confused.

That would just shift the problem from one function to other.
Please apply Neil's suggestion.

>
>>
>> Neil
>>
>>> -             ret = regmap_read(clk->map, div->offset, &val);
>>> -             if (ret)
>>> -                     return ret;
>>> -
>>> -             val >>= div->shift;
>>> -             val &= clk_div_mask(div->width);
>>> -
>>> -             return divider_ro_determine_rate(hw, req, div->table,
>>> -                                              div->width, div->flags,
>>> val);
>>> -     }
>>>
>>>       return divider_determine_rate(hw, req, div->table, div->width,
>>>                                     div->flags);
>>> @@ -127,9 +112,28 @@ const struct clk_ops clk_regmap_divider_ops = {
>>>   };
>>>   EXPORT_SYMBOL_NS_GPL(clk_regmap_divider_ops, CLK_MESON);
>>>
>>> +static int clk_regmap_div_ro_determine_rate(struct clk_hw *hw,
>>> +                                         struct clk_rate_request *req)
>>> +{
>>> +     struct clk_regmap *clk = to_clk_regmap(hw);
>>> +     struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>>> +     unsigned int val;
>>> +     int ret;
>>> +
>>> +     ret = regmap_read(clk->map, div->offset, &val);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     val >>= div->shift;
>>> +     val &= clk_div_mask(div->width);
>>> +
>>> +     return divider_ro_determine_rate(hw, req, div->table, div->width,
>>> +                                      div->flags, val);
>>> +}
>>> +
>>>   const struct clk_ops clk_regmap_divider_ro_ops = {
>>>       .recalc_rate = clk_regmap_div_recalc_rate,
>>> -     .determine_rate = clk_regmap_div_determine_rate,
>>> +     .determine_rate = clk_regmap_div_ro_determine_rate,
>>>   };
>>>   EXPORT_SYMBOL_NS_GPL(clk_regmap_divider_ro_ops, CLK_MESON);
>>>
>>>
>>> ---
>>> base-commit: 664988eb47dd2d6ae1d9e4188ec91832562f8f26
>>> change-id:
>>> 20241111-fix_childclk_of_roclk_has_been_tampered_with-61dbcc623746
>>>
>>> Best regards,
>>

-- 
Jerome