lkml.org 
[lkml]   [2015]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] clk: add CS2000 Fractional-N driver
Date

Hi Geert

Thank you for your review

> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >
> > This patch adds CS2000 Fractional-N driver as clock provider.
> > It is useful if it supports runtime clock setting, but it supports
> > fixed clock rate only at this point.
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> Thanks for your patch!

> As you have multiple inputs, I think it makes sense to use "clock-names".
(snip)
> Why do you call them differently from the datasheet?
> Especially the different CLK_IN can be confusing.
(snip)
> This can fail, so the error should be propagated, and checked in all
> callers of cs2000_write().
(snip)
> i2c_smbus_read_byte_data returns s32, and the return value will be
> negative in case of a failure.
(snip)
> Should check for failure.
(snip)
> unsigned int i
(snip)
> Please use do_div() for 64-by-32 divides, as full 64-bit divisions are
> usually not available on 32-bit architectures.
> "s32 val", and please check for failures.
(snip)
> const char *revision;

Thanks
will be fiex in v2

> > + clk_main = of_clk_get(np, CLK_MAIN);
> > + /* not yet provided */
> > + if (IS_ERR(clk_main))
> > + return -EPROBE_DEFER;
> > +
> > + clk_in = of_clk_get(np, CLK_IN);
> > + if (IS_ERR(clk_in))
> > + return PTR_ERR(clk_in);
>
> Named clk_get(), for both?

Thanks
I will use devm_clk_get() to avoid clk_put()

>> + if (CH_ERR(ch))
>>
>> I think it's more readable to drop the CH_ERR() macro, and open code
>> "(ch < 0) || (ch >= CH_MAX)" (also in cs2000_ratio_select()).

In my opinion, complex judgment should use same method (= macro)
Current driver is using this macro only 2 places, so,
not a big deal at this point.
But someone might add mistake in additional patch if it was opened code,
and it exists in many places.
So, I would like to keep this macro in v2.
But, CH_ERR() is a littile bit ununderstandable naming.
it will be CH_SIZE_ERR()

Best regards
---
Kuninori Morimoto


\
 
 \ /
  Last update: 2015-09-14 12:01    [W:0.148 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site