Re: [U-Boot] [PATCH] Convert CONFIG_SYS_I2C_OMAP24XX et al to Kconfig
On Sun, Aug 06, 2017 at 01:27:15PM -0500, Adam Ford wrote: > On Thu, Jul 27, 2017 at 6:55 AM, Tom Rini wrote: > > On Wed, Jul 26, 2017 at 09:22:06PM -0500, Adam Ford wrote: > >> On Wed, Jul 26, 2017 at 8:52 PM, Tom Rini wrote: > >> > On Wed, Jul 26, 2017 at 09:03:37AM -0500, Adam Ford wrote: > >> > > >> >> This converts the following to Kconfig: > >> >>CONFIG_SYS_I2C_OMAP24XX > >> >>CONFIG_SYS_I2C_OMAP34XX > >> >> > >> >> Signed-off-by: Adam Ford > >> > > >> > This needs some manual attention. We should just drop > >> > CONFIG_SYS_I2C_OMAP24XX as it's meaningless now. Also: > >> > > >> I thought the same thing, but I looked at the driver and the driver > >> has some explicit differences that are unique to the > >> CONFIG_SYS_I2C_OMAP34XX. > >> > >> As an example: > >> #if defined(CONFIG_OMAP34XX) > >> /* > >> * Have to enable interrupts for OMAP2/3, these IPs don't have > >> * an 'irqstatus_raw' register and we shall have to poll 'stat' > >> */ > >> writew(I2C_IE_XRDY_IE | I2C_IE_RRDY_IE | I2C_IE_ARDY_IE | > >> I2C_IE_NACK_IE | I2C_IE_AL_IE, &i2c_base->ie); > >> #endif > >> > >> > >> > >> The comment in the code even states there are some minor differences: > >> "Status functions now read irqstatus_raw as per TRM guidelines > >> (except for OMAP243X and OMAP34XX)" > >> > >> So I think we need both. > >> Looking at the ti_omap4_common.h, it defines CONFIG_SYS_I2C_OMAP24XX, > >> but not OMAP34XX, so it appears to me like we might want a naming > >> convention change. > > > > But nothing toggles off of SYS_I2C_OMAP24XX vs SYS_I2C_OMAP34XX is the > > key. It might have back when we supported omap1/2 systems as well, but > > it doesn't today. Everything inside the driver keys off of > > OMAP34XX/AM33XX/etc/etc now. > > > > Got it. That makes sense. Since the name of the source files is > omap24xx_i2c.c/.h , would you object to dumping the > CONFIG_SYS_I2C_OMAP34XX in favor of keeping the > CONFIG_SYS_I2C_OMAP24XX for consistency? That seems the most reasonable course, thanks! -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] Convert CONFIG_SYS_I2C_OMAP24XX et al to Kconfig
On Thu, Jul 27, 2017 at 6:55 AM, Tom Rini wrote: > On Wed, Jul 26, 2017 at 09:22:06PM -0500, Adam Ford wrote: >> On Wed, Jul 26, 2017 at 8:52 PM, Tom Rini wrote: >> > On Wed, Jul 26, 2017 at 09:03:37AM -0500, Adam Ford wrote: >> > >> >> This converts the following to Kconfig: >> >>CONFIG_SYS_I2C_OMAP24XX >> >>CONFIG_SYS_I2C_OMAP34XX >> >> >> >> Signed-off-by: Adam Ford >> > >> > This needs some manual attention. We should just drop >> > CONFIG_SYS_I2C_OMAP24XX as it's meaningless now. Also: >> > >> I thought the same thing, but I looked at the driver and the driver >> has some explicit differences that are unique to the >> CONFIG_SYS_I2C_OMAP34XX. >> >> As an example: >> #if defined(CONFIG_OMAP34XX) >> /* >> * Have to enable interrupts for OMAP2/3, these IPs don't have >> * an 'irqstatus_raw' register and we shall have to poll 'stat' >> */ >> writew(I2C_IE_XRDY_IE | I2C_IE_RRDY_IE | I2C_IE_ARDY_IE | >> I2C_IE_NACK_IE | I2C_IE_AL_IE, &i2c_base->ie); >> #endif >> >> >> >> The comment in the code even states there are some minor differences: >> "Status functions now read irqstatus_raw as per TRM guidelines >> (except for OMAP243X and OMAP34XX)" >> >> So I think we need both. >> Looking at the ti_omap4_common.h, it defines CONFIG_SYS_I2C_OMAP24XX, >> but not OMAP34XX, so it appears to me like we might want a naming >> convention change. > > But nothing toggles off of SYS_I2C_OMAP24XX vs SYS_I2C_OMAP34XX is the > key. It might have back when we supported omap1/2 systems as well, but > it doesn't today. Everything inside the driver keys off of > OMAP34XX/AM33XX/etc/etc now. > Got it. That makes sense. Since the name of the source files is omap24xx_i2c.c/.h , would you object to dumping the CONFIG_SYS_I2C_OMAP34XX in favor of keeping the CONFIG_SYS_I2C_OMAP24XX for consistency? adam > -- > Tom ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] Convert CONFIG_SYS_I2C_OMAP24XX et al to Kconfig
On Wed, Jul 26, 2017 at 09:22:06PM -0500, Adam Ford wrote: > On Wed, Jul 26, 2017 at 8:52 PM, Tom Rini wrote: > > On Wed, Jul 26, 2017 at 09:03:37AM -0500, Adam Ford wrote: > > > >> This converts the following to Kconfig: > >>CONFIG_SYS_I2C_OMAP24XX > >>CONFIG_SYS_I2C_OMAP34XX > >> > >> Signed-off-by: Adam Ford > > > > This needs some manual attention. We should just drop > > CONFIG_SYS_I2C_OMAP24XX as it's meaningless now. Also: > > > I thought the same thing, but I looked at the driver and the driver > has some explicit differences that are unique to the > CONFIG_SYS_I2C_OMAP34XX. > > As an example: > #if defined(CONFIG_OMAP34XX) > /* > * Have to enable interrupts for OMAP2/3, these IPs don't have > * an 'irqstatus_raw' register and we shall have to poll 'stat' > */ > writew(I2C_IE_XRDY_IE | I2C_IE_RRDY_IE | I2C_IE_ARDY_IE | > I2C_IE_NACK_IE | I2C_IE_AL_IE, &i2c_base->ie); > #endif > > > > The comment in the code even states there are some minor differences: > "Status functions now read irqstatus_raw as per TRM guidelines > (except for OMAP243X and OMAP34XX)" > > So I think we need both. > Looking at the ti_omap4_common.h, it defines CONFIG_SYS_I2C_OMAP24XX, > but not OMAP34XX, so it appears to me like we might want a naming > convention change. But nothing toggles off of SYS_I2C_OMAP24XX vs SYS_I2C_OMAP34XX is the key. It might have back when we supported omap1/2 systems as well, but it doesn't today. Everything inside the driver keys off of OMAP34XX/AM33XX/etc/etc now. -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] Convert CONFIG_SYS_I2C_OMAP24XX et al to Kconfig
On Wed, Jul 26, 2017 at 8:52 PM, Tom Rini wrote: > On Wed, Jul 26, 2017 at 09:03:37AM -0500, Adam Ford wrote: > >> This converts the following to Kconfig: >>CONFIG_SYS_I2C_OMAP24XX >>CONFIG_SYS_I2C_OMAP34XX >> >> Signed-off-by: Adam Ford > > This needs some manual attention. We should just drop > CONFIG_SYS_I2C_OMAP24XX as it's meaningless now. Also: > I thought the same thing, but I looked at the driver and the driver has some explicit differences that are unique to the CONFIG_SYS_I2C_OMAP34XX. As an example: #if defined(CONFIG_OMAP34XX) /* * Have to enable interrupts for OMAP2/3, these IPs don't have * an 'irqstatus_raw' register and we shall have to poll 'stat' */ writew(I2C_IE_XRDY_IE | I2C_IE_RRDY_IE | I2C_IE_ARDY_IE | I2C_IE_NACK_IE | I2C_IE_AL_IE, &i2c_base->ie); #endif The comment in the code even states there are some minor differences: "Status functions now read irqstatus_raw as per TRM guidelines (except for OMAP243X and OMAP34XX)" So I think we need both. Looking at the ti_omap4_common.h, it defines CONFIG_SYS_I2C_OMAP24XX, but not OMAP34XX, so it appears to me like we might want a naming convention change. >> diff --git a/include/configs/devkit8000.h b/include/configs/devkit8000.h >> index 5476961..e15eaa6 100644 >> --- a/include/configs/devkit8000.h >> +++ b/include/configs/devkit8000.h >> @@ -65,8 +65,6 @@ >> #undef CONFIG_OMAP3_SPI >> >> /* I2C */ >> -#undef CONFIG_SYS_I2C_OMAP24XX >> -#define CONFIG_SYS_I2C_OMAP34XX > > When you see cases like this, you should delete the comment (and extra > blank line) by hand as well, in the commit. There's a few instances of > this in the patch. Thanks! I can revisit this and fix it, but I'll wait to hear a reply to my above comment. adam > > -- > Tom ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] Convert CONFIG_SYS_I2C_OMAP24XX et al to Kconfig
On Wed, Jul 26, 2017 at 09:03:37AM -0500, Adam Ford wrote: > This converts the following to Kconfig: >CONFIG_SYS_I2C_OMAP24XX >CONFIG_SYS_I2C_OMAP34XX > > Signed-off-by: Adam Ford This needs some manual attention. We should just drop CONFIG_SYS_I2C_OMAP24XX as it's meaningless now. Also: > diff --git a/include/configs/devkit8000.h b/include/configs/devkit8000.h > index 5476961..e15eaa6 100644 > --- a/include/configs/devkit8000.h > +++ b/include/configs/devkit8000.h > @@ -65,8 +65,6 @@ > #undef CONFIG_OMAP3_SPI > > /* I2C */ > -#undef CONFIG_SYS_I2C_OMAP24XX > -#define CONFIG_SYS_I2C_OMAP34XX When you see cases like this, you should delete the comment (and extra blank line) by hand as well, in the commit. There's a few instances of this in the patch. Thanks! -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
