Re: [PATCH/RFC v3 20/22] clk: renesas: r8a7778: Remove obsolete r8a7778_clocks_init()

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Dirk,

On Thu, Jun 2, 2016 at 7:54 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> On 01.06.2016 21:21, Geert Uytterhoeven wrote:
>> The R-Car M1A board code no longer calls r8a7778_clocks_init().
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> v3:
>>   - New.
>> ---
>>  drivers/clk/renesas/clk-r8a7778.c | 13 -------------
>>  include/linux/clk/renesas.h       |  1 -
>>  2 files changed, 14 deletions(-)
>>
>> diff --git a/drivers/clk/renesas/clk-r8a7778.c
>> b/drivers/clk/renesas/clk-r8a7778.c
>> index 07ea411098a75ad1..886a8380e91247a1 100644
>> --- a/drivers/clk/renesas/clk-r8a7778.c
>> +++ b/drivers/clk/renesas/clk-r8a7778.c
>> @@ -143,16 +143,3 @@ static void __init r8a7778_cpg_clocks_init(struct
>> device_node *np)
>>
>>  CLK_OF_DECLARE(r8a7778_cpg_clks, "renesas,r8a7778-cpg-clocks",
>>                r8a7778_cpg_clocks_init);
>> -
>> -void __init r8a7778_clocks_init(u32 mode)
>> -{
>> -       BUG_ON(!(mode & BIT(19)));
>> -
>> -       cpg_mode_rates = (!!(mode & BIT(18)) << 2) |
>> -                        (!!(mode & BIT(12)) << 1) |
>> -                        (!!(mode & BIT(11)));
>> -       cpg_mode_divs = (!!(mode & BIT(2)) << 1) |
>> -                       (!!(mode & BIT(1)));
>> -
>> -       of_clk_init(NULL);
>> -}
>
> Just a question on how you structured the patches: Is there a special reason
> why you do the adding of new code and the removal of dead code in two
> patches?
>
> Having both in one patch normally makes it more obvious which old code is
> replaced by which new code.
>
> An example: In patch
>
> [PATCH/RFC v3 11/22] clk: renesas: r8a7778: Obtain mode pin values using
> R-Car RST driver
>
> I wondered where the section
>
> +       BUG_ON(!(mode & BIT(19)));
> +
> +       cpg_mode_rates = (!!(mode & BIT(18)) << 2) |
> +                        (!!(mode & BIT(12)) << 1) |
> +                        (!!(mode & BIT(11)));
> +       cpg_mode_divs = (!!(mode & BIT(2)) << 1) |
> +                       (!!(mode & BIT(1)));
>
> comes from. This becomes obvious with this patch 20/22. But it's 9 patches
> later ;) So why don't make the whole replacement visible in one patch?

Because that would break bisection.
Path 20 depends on patch 17, which depends on patch 11.
These three can't be a single patch because they touch multiple subsystems.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds