Re: [PATCH v2 2/3] clk: clk-apple-nco: Add driver for Apple NCO

[Martin Povišer]

Hi Stephen,

thanks for the review.

> On 20. 1. 2022, at 6:38, Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> Quoting Martin Povišer (2022-01-18 11:21:10)
>> diff --git a/drivers/clk/clk-apple-nco.c b/drivers/clk/clk-apple-nco.c
>> new file mode 100644
>> index 000000000000..593f5b5ce5b7
>> --- /dev/null
>> +++ b/drivers/clk/clk-apple-nco.c


>> +static u32 nco_div_translate(struct nco_tables *tbl, unsigned int div)
>> +{
>> +       unsigned int coarse = div / 4;
>> +
>> +       if (WARN_ON(nco_div_out_of_range(div)))
>
> Maybe worth knowing which clk is out of range?

This can only happen when nco_div_translate is used wrong, which should never
happen with the code as-is, so I wouldn't think it is worth expanding on.

>> +
>> +static int nco_set_rate(struct clk_hw *hw, unsigned long rate,
>> +                               unsigned long parent_rate)
>> +{
>> +       struct nco_channel *chan = to_nco_channel(hw);
>> +       unsigned long flags;
>> +       u32 div;
>> +       s32 inc1, inc2;
>> +       bool was_enabled;
>> +
>> +       div = 2 * parent_rate / rate;
>> +       inc1 = 2 * parent_rate - div * rate;
>> +       inc2 = -((s32) (rate - inc1));
>
> Is the cast necessary?

Answering that prompted me to get back to reading some C specification and now
I am confident in moving away from signed types here and in nco_recalc_rate
altogether.

>>
>> +
>> +static unsigned long nco_recalc_rate(struct clk_hw *hw,
>> +                               unsigned long parent_rate)
>> +{

>> +
>> +       /* Scale both sides of division by incbase to maintain precision */
>> +       incbase = inc1 - inc2;
>> +
>> +       return div_u64(((u64) parent_rate) * 2 * incbase,
>> +                       ((u64) div) * incbase + inc1);
>
> Why is the divisor casted to 64 bits? div_u64() takes a u32 divisor so
> if it's going to overflow 32 bits we're in trouble.

Yeah, we need div64_u64, great you caught that.

>> +       struct nco_tables *tbl;
>> +       unsigned int nchannels;
>> +       int ret, i;
>> +
>> +       regs = devm_platform_get_and_ioremap_resource(pdev, 0, &regs_res);
>> +       if (IS_ERR(regs))
>> +               return PTR_ERR(regs);
>> +
>> +       if (resource_size(regs_res) < NCO_CHANNEL_REGSIZE)
>> +               return -EINVAL;
>> +       nchannels = (resource_size(regs_res) - NCO_CHANNEL_REGSIZE)
>> +                       / NCO_CHANNEL_STRIDE + 1;
>
> Is this some sort of DIV_ROUND_UP()?

Almost. I will shop around for a macro replacement.

Martin