Re: [PATCH 4/5] ARM: shmobile: r8a7790: add IIC(B) clocks to dtsi

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 1715 bytes --]

On Wed, Mar 12, 2014 at 08:05:26PM +0900, Magnus Damm wrote:
> Hi Wolfram,
> 
> On Wed, Mar 12, 2014 at 6:24 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > From: Wolfram Sang <wsa@sang-engineering.com>
> >
> > Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
> > ---
> > Note: Adding clocks whilst keeping the current sorting is very likely to
> > break a previously working clock IMO. Imagine adding PCIEC clock inbetween IIC0
> > and IIC1 here. Adding chronologically and grouped by similar function blocks is
> > easier to track. An example addition could then look like:
> >
> >         R8A7790_CLK_TPU0
> >         R8A7790_CLK_SDHI3 R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0
> >         R8A7790_CLK_MMCIF1 R8A7790_CLK_MMCIF0
> >         R8A7790_CLK_CMT1
> >         R8A7790_CLK_IIC2 R8A7790_CLK_IIC1 R8A7790_CLK_IIC0
> > +       R8A7790_CLK_PCIEC
> 
> Can you please care to explain a bit more about why you see a risk
> here? Is it a risk for typo or something else?

The risk here is if you put something in the "middle" then there is a
chance you might be off, and, for example, mix up parent clocks.
Especially when you need to reformat paragraphs because of too long
lines. Yeah, all this can be avoided by careful review, but this review
really needs to be careful.

> It looks to me that this is just a matter about adding the entry at
> the right position in several places.

Yes. IMO adding it with the pattern I sketched above will make it a
piece of cake since the diff is a lot more simple than the patch I
originally submitted.

That being said, I will keep the current sorting for consistency reasons
but I personally made up my mind ;)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]