From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergei Shtylyov Date: Thu, 16 May 2013 14:30:13 +0000 Subject: Re: [PATCH v3 1/1] ARM: shmobile: r8a7778: add I2C support Message-Id: <5194ED75.5000908@cogentembedded.com> List-Id: References: <201304232012.57619.sergei.shtylyov@cogentembedded.com> <201304232015.59697.sergei.shtylyov@cogentembedded.com> <87txm3wwzh.wl%kuninori.morimoto.gx@renesas.com> <5194EC49.9000508@cogentembedded.com> In-Reply-To: <5194EC49.9000508@cogentembedded.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-arm-kernel@lists.infradead.org On 16-05-2013 18:25, I wrote: >> Basically, this patch is good for me. >> But I have some comment. >>> From: Vladimir Barinov >>> Add I2C clocks and platform devices for R8A7778 SoC. >>> Don't forget to also add the peripheral clock which the I2C driver uses. >>> Signed-off-by: Vladimir Barinov >>> [Sergei: changed the registration function to >>> platform_device_register_simple(), >>> annotated I2C resources as '__initdata', regrouped MSTPxxx *enum* >>> members, added >>> the copyright.] >>> Signed-off-by: Sergei Shtylyov >>> Acked-by: Kuninori Morimoto >> (snip) [...] >>> +/* I2C */ >>> +static struct resource i2c0_resources[] __initdata = { >>> + DEFINE_RES_MEM(0xffc70000, 0x1000), >>> + DEFINE_RES_IRQ(gic_iid(0x63)), >>> +}; >>> + >>> +static struct resource i2c1_resources[] __initdata = { >>> + DEFINE_RES_MEM(0xffc71000, 0x1000), >>> + DEFINE_RES_IRQ(gic_iid(0x6e)), >>> +}; >>> + >>> +static struct resource i2c2_resources[] __initdata = { >>> + DEFINE_RES_MEM(0xffc72000, 0x1000), >>> + DEFINE_RES_IRQ(gic_iid(0x6c)), >>> +}; >>> + >>> +static struct resource i2c3_resources[] __initdata = { >>> + DEFINE_RES_MEM(0xffc73000, 0x1000), >>> + DEFINE_RES_IRQ(gic_iid(0x6d)), >>> +}; >>> + >>> +#define r8a7778_register_i2c(idx) \ >>> + platform_device_register_simple( \ >>> + "i2c-rcar", idx, \ >>> + i2c##idx##_resources, \ >>> + ARRAY_SIZE(i2c##idx##_resources)) >>> + >>> /* USB PHY */ >>> static struct resource usb_phy_resources[] __initdata = { >>> DEFINE_RES_MEM(0xffe70800, 0x100), >>> @@ -294,6 +321,11 @@ void __init r8a7778_add_standard_devices >>> >>> r8a7778_register_tmu(0); >>> r8a7778_register_tmu(1); >>> + >>> + r8a7778_register_i2c(0); >>> + r8a7778_register_i2c(1); >>> + r8a7778_register_i2c(2); >>> + r8a7778_register_i2c(3); >>> } >> i2c1 - i2c3 pins are pin-multi. > Ah, you mean there should be I2C related entries in the pinmux table > in the BOCK-W code? On the other hand, we only use I2C0 on BOCK-W which as you say is not pin-multiplexed. >> I guess using r8a7778_add_i2c_devices(idx) from board code is better >> method > Why? Not at all, I think, since we don't have the platform data. Hm, you may be right here... >> Best regards >> --- >> Kuninori Morimoto WBR, Sergei From mboxrd@z Thu Jan 1 00:00:00 1970 From: sergei.shtylyov@cogentembedded.com (Sergei Shtylyov) Date: Thu, 16 May 2013 18:30:13 +0400 Subject: [PATCH v3 1/1] ARM: shmobile: r8a7778: add I2C support In-Reply-To: <5194EC49.9000508@cogentembedded.com> References: <201304232012.57619.sergei.shtylyov@cogentembedded.com> <201304232015.59697.sergei.shtylyov@cogentembedded.com> <87txm3wwzh.wl%kuninori.morimoto.gx@renesas.com> <5194EC49.9000508@cogentembedded.com> Message-ID: <5194ED75.5000908@cogentembedded.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 16-05-2013 18:25, I wrote: >> Basically, this patch is good for me. >> But I have some comment. >>> From: Vladimir Barinov >>> Add I2C clocks and platform devices for R8A7778 SoC. >>> Don't forget to also add the peripheral clock which the I2C driver uses. >>> Signed-off-by: Vladimir Barinov >>> [Sergei: changed the registration function to >>> platform_device_register_simple(), >>> annotated I2C resources as '__initdata', regrouped MSTPxxx *enum* >>> members, added >>> the copyright.] >>> Signed-off-by: Sergei Shtylyov >>> Acked-by: Kuninori Morimoto >> (snip) [...] >>> +/* I2C */ >>> +static struct resource i2c0_resources[] __initdata = { >>> + DEFINE_RES_MEM(0xffc70000, 0x1000), >>> + DEFINE_RES_IRQ(gic_iid(0x63)), >>> +}; >>> + >>> +static struct resource i2c1_resources[] __initdata = { >>> + DEFINE_RES_MEM(0xffc71000, 0x1000), >>> + DEFINE_RES_IRQ(gic_iid(0x6e)), >>> +}; >>> + >>> +static struct resource i2c2_resources[] __initdata = { >>> + DEFINE_RES_MEM(0xffc72000, 0x1000), >>> + DEFINE_RES_IRQ(gic_iid(0x6c)), >>> +}; >>> + >>> +static struct resource i2c3_resources[] __initdata = { >>> + DEFINE_RES_MEM(0xffc73000, 0x1000), >>> + DEFINE_RES_IRQ(gic_iid(0x6d)), >>> +}; >>> + >>> +#define r8a7778_register_i2c(idx) \ >>> + platform_device_register_simple( \ >>> + "i2c-rcar", idx, \ >>> + i2c##idx##_resources, \ >>> + ARRAY_SIZE(i2c##idx##_resources)) >>> + >>> /* USB PHY */ >>> static struct resource usb_phy_resources[] __initdata = { >>> DEFINE_RES_MEM(0xffe70800, 0x100), >>> @@ -294,6 +321,11 @@ void __init r8a7778_add_standard_devices >>> >>> r8a7778_register_tmu(0); >>> r8a7778_register_tmu(1); >>> + >>> + r8a7778_register_i2c(0); >>> + r8a7778_register_i2c(1); >>> + r8a7778_register_i2c(2); >>> + r8a7778_register_i2c(3); >>> } >> i2c1 - i2c3 pins are pin-multi. > Ah, you mean there should be I2C related entries in the pinmux table > in the BOCK-W code? On the other hand, we only use I2C0 on BOCK-W which as you say is not pin-multiplexed. >> I guess using r8a7778_add_i2c_devices(idx) from board code is better >> method > Why? Not at all, I think, since we don't have the platform data. Hm, you may be right here... >> Best regards >> --- >> Kuninori Morimoto WBR, Sergei