| Message ID | 20170328223920.841-1-jernej.skrabec@siol.net |
|---|---|
| State | Rejected |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
Hi, On 28 March 2017 at 16:39, Jernej Skrabec <jernej.skrabec@siol.net> wrote: > I2C_EDID currently selects DM_I2C. However, it is not needed. I2C_EDID > is used for building edid.c, which doesn't even use I2C bus, and by I2C > command, which knows how to use DM and old style I2C interface, so it is > not directly affected by this removal. > > Furthermore, this selection can generate warning if DM display driver > is used on platform which doesn't implement DM I2C driver (for example, > sunxi platform with upcoming DM video & display driver). > > Patch was tested with rockchip and sunxi boards and successfully > compiled exynos and tegra targets. They are the only consumers of > CONFIG_DISPLAY option, which is the only one which selects I2C_EDID. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > > cmd/i2c.c | 10 ++++++---- > drivers/video/Kconfig | 1 - > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/cmd/i2c.c b/cmd/i2c.c > index 473153fbd4..7b6306e525 100644 > --- a/cmd/i2c.c > +++ b/cmd/i2c.c > @@ -1630,7 +1630,8 @@ static int do_sdram (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[]) > * Syntax: > * i2c edid {i2c_chip} > */ > -#if defined(CONFIG_I2C_EDID) > +#if defined(CONFIG_I2C_EDID) && \ > + (defined(CONFIG_SYS_I2C) || defined(CONFIG_DM_I2C)) The correct solution here I think is to convert sunxi to DM_I2C. We should not be adding new features to the old code. > int do_edid(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) > { > uint chip; > @@ -1665,7 +1666,7 @@ int do_edid(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) > return 0; > > } > -#endif /* CONFIG_I2C_EDID */ > +#endif > > #ifdef CONFIG_DM_I2C > static void show_bus(struct udevice *bus) > @@ -1936,9 +1937,10 @@ static cmd_tbl_t cmd_i2c_sub[] = { > defined(CONFIG_I2C_MULTI_BUS) || defined(CONFIG_DM_I2C) > U_BOOT_CMD_MKENT(dev, 1, 1, do_i2c_bus_num, "", ""), > #endif /* CONFIG_I2C_MULTI_BUS */ > -#if defined(CONFIG_I2C_EDID) > +#if defined(CONFIG_I2C_EDID) && \ > + (defined(CONFIG_SYS_I2C) || defined(CONFIG_DM_I2C)) > U_BOOT_CMD_MKENT(edid, 1, 1, do_edid, "", ""), > -#endif /* CONFIG_I2C_EDID */ > +#endif > U_BOOT_CMD_MKENT(loop, 3, 1, do_i2c_loop, "", ""), > U_BOOT_CMD_MKENT(md, 3, 1, do_i2c_md, "", ""), > U_BOOT_CMD_MKENT(mm, 2, 1, do_i2c_mm, "", ""), > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig > index 2069576958..2f340235ee 100644 > --- a/drivers/video/Kconfig > +++ b/drivers/video/Kconfig > @@ -355,7 +355,6 @@ config VIDEO_MVEBU > > config I2C_EDID > bool "Enable EDID library" > - depends on DM_I2C > default n > help > This enables library for accessing EDID data from an LCD panel. > -- > 2.12.1 > Regards, Simon
Hi, Dne nedelja, 09. april 2017 ob 21:28:47 CEST je Simon Glass napisal(a): > Hi, > > On 28 March 2017 at 16:39, Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > I2C_EDID currently selects DM_I2C. However, it is not needed. I2C_EDID > > is used for building edid.c, which doesn't even use I2C bus, and by I2C > > command, which knows how to use DM and old style I2C interface, so it is > > not directly affected by this removal. > > > > Furthermore, this selection can generate warning if DM display driver > > is used on platform which doesn't implement DM I2C driver (for example, > > sunxi platform with upcoming DM video & display driver). > > > > Patch was tested with rockchip and sunxi boards and successfully > > compiled exynos and tegra targets. They are the only consumers of > > CONFIG_DISPLAY option, which is the only one which selects I2C_EDID. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > cmd/i2c.c | 10 ++++++---- > > drivers/video/Kconfig | 1 - > > 2 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/cmd/i2c.c b/cmd/i2c.c > > index 473153fbd4..7b6306e525 100644 > > --- a/cmd/i2c.c > > +++ b/cmd/i2c.c > > @@ -1630,7 +1630,8 @@ static int do_sdram (cmd_tbl_t * cmdtp, int flag, > > int argc, char * const argv[])> > > * Syntax: > > * i2c edid {i2c_chip} > > */ > > > > -#if defined(CONFIG_I2C_EDID) > > +#if defined(CONFIG_I2C_EDID) && \ > > + (defined(CONFIG_SYS_I2C) || defined(CONFIG_DM_I2C)) > > The correct solution here I think is to convert sunxi to DM_I2C. We > should not be adding new features to the old code. With the "old code" you referring to i2c command? Actually, I'm not sure if "i2c edid" command can be useful on most platforms. I know that rk3288 has multiplexed I2C controller pins with HDMI DDC pins, where this make sense. But for example on sunxi, in order to be useful, it would mean that dw_hdmi driver has to register DDC as I2C driver. I'm also not sure why "i2c edid" code knows how to use old and DM I2C interface when it is surrounded by a symbol, which always selects DM_I2C. Well, in sunxi case, that actually prevents build failure, but still produces unwanted warning. Otherwise I agree that converting sunxi to DM_I2C should be done and patch for that already exists, but it was not merged yet: https://patchwork.ozlabs.org/patch/734375/ Regards, Jernej > > > int do_edid(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) > > { > > > > uint chip; > > > > @@ -1665,7 +1666,7 @@ int do_edid(cmd_tbl_t *cmdtp, int flag, int argc, > > char *const argv[])> > > return 0; > > > > } > > > > -#endif /* CONFIG_I2C_EDID */ > > +#endif > > > > #ifdef CONFIG_DM_I2C > > static void show_bus(struct udevice *bus) > > > > @@ -1936,9 +1937,10 @@ static cmd_tbl_t cmd_i2c_sub[] = { > > > > defined(CONFIG_I2C_MULTI_BUS) || defined(CONFIG_DM_I2C) > > U_BOOT_CMD_MKENT(dev, 1, 1, do_i2c_bus_num, "", ""), > > > > #endif /* CONFIG_I2C_MULTI_BUS */ > > > > -#if defined(CONFIG_I2C_EDID) > > +#if defined(CONFIG_I2C_EDID) && \ > > + (defined(CONFIG_SYS_I2C) || defined(CONFIG_DM_I2C)) > > > > U_BOOT_CMD_MKENT(edid, 1, 1, do_edid, "", ""), > > > > -#endif /* CONFIG_I2C_EDID */ > > +#endif > > > > U_BOOT_CMD_MKENT(loop, 3, 1, do_i2c_loop, "", ""), > > U_BOOT_CMD_MKENT(md, 3, 1, do_i2c_md, "", ""), > > U_BOOT_CMD_MKENT(mm, 2, 1, do_i2c_mm, "", ""), > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig > > index 2069576958..2f340235ee 100644 > > --- a/drivers/video/Kconfig > > +++ b/drivers/video/Kconfig > > @@ -355,7 +355,6 @@ config VIDEO_MVEBU > > > > config I2C_EDID > > > > bool "Enable EDID library" > > > > - depends on DM_I2C > > > > default n > > help > > > > This enables library for accessing EDID data from an LCD panel. > > > > -- > > 2.12.1 > > Regards, > Simon
Hi, On 9 April 2017 at 15:30, Jernej Škrabec <jernej.skrabec@siol.net> wrote: > Hi, > > Dne nedelja, 09. april 2017 ob 21:28:47 CEST je Simon Glass napisal(a): >> Hi, >> >> On 28 March 2017 at 16:39, Jernej Skrabec <jernej.skrabec@siol.net> wrote: >> > I2C_EDID currently selects DM_I2C. However, it is not needed. I2C_EDID >> > is used for building edid.c, which doesn't even use I2C bus, and by I2C >> > command, which knows how to use DM and old style I2C interface, so it is >> > not directly affected by this removal. >> > >> > Furthermore, this selection can generate warning if DM display driver >> > is used on platform which doesn't implement DM I2C driver (for example, >> > sunxi platform with upcoming DM video & display driver). >> > >> > Patch was tested with rockchip and sunxi boards and successfully >> > compiled exynos and tegra targets. They are the only consumers of >> > CONFIG_DISPLAY option, which is the only one which selects I2C_EDID. >> > >> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> >> > --- >> > >> > cmd/i2c.c | 10 ++++++---- >> > drivers/video/Kconfig | 1 - >> > 2 files changed, 6 insertions(+), 5 deletions(-) >> > >> > diff --git a/cmd/i2c.c b/cmd/i2c.c >> > index 473153fbd4..7b6306e525 100644 >> > --- a/cmd/i2c.c >> > +++ b/cmd/i2c.c >> > @@ -1630,7 +1630,8 @@ static int do_sdram (cmd_tbl_t * cmdtp, int flag, >> > int argc, char * const argv[])> >> > * Syntax: >> > * i2c edid {i2c_chip} >> > */ >> > >> > -#if defined(CONFIG_I2C_EDID) >> > +#if defined(CONFIG_I2C_EDID) && \ >> > + (defined(CONFIG_SYS_I2C) || defined(CONFIG_DM_I2C)) >> >> The correct solution here I think is to convert sunxi to DM_I2C. We >> should not be adding new features to the old code. > > With the "old code" you referring to i2c command? Actually, I'm not sure if > "i2c edid" command can be useful on most platforms. I know that rk3288 has > multiplexed I2C controller pins with HDMI DDC pins, where this make sense. But > for example on sunxi, in order to be useful, it would mean that dw_hdmi driver > has to register DDC as I2C driver. > > I'm also not sure why "i2c edid" code knows how to use old and DM I2C > interface when it is surrounded by a symbol, which always selects DM_I2C. > Well, in sunxi case, that actually prevents build failure, but still produces > unwanted warning. > > Otherwise I agree that converting sunxi to DM_I2C should be done and patch for > that already exists, but it was not merged yet: > https://patchwork.ozlabs.org/patch/734375/ Sounds good. Let's get that merged and then we don't need to worry about the legacy I2C. Regards Simon
diff --git a/cmd/i2c.c b/cmd/i2c.c index 473153fbd4..7b6306e525 100644 --- a/cmd/i2c.c +++ b/cmd/i2c.c @@ -1630,7 +1630,8 @@ static int do_sdram (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[]) * Syntax: * i2c edid {i2c_chip} */ -#if defined(CONFIG_I2C_EDID) +#if defined(CONFIG_I2C_EDID) && \ + (defined(CONFIG_SYS_I2C) || defined(CONFIG_DM_I2C)) int do_edid(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) { uint chip; @@ -1665,7 +1666,7 @@ int do_edid(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) return 0; } -#endif /* CONFIG_I2C_EDID */ +#endif #ifdef CONFIG_DM_I2C static void show_bus(struct udevice *bus) @@ -1936,9 +1937,10 @@ static cmd_tbl_t cmd_i2c_sub[] = { defined(CONFIG_I2C_MULTI_BUS) || defined(CONFIG_DM_I2C) U_BOOT_CMD_MKENT(dev, 1, 1, do_i2c_bus_num, "", ""), #endif /* CONFIG_I2C_MULTI_BUS */ -#if defined(CONFIG_I2C_EDID) +#if defined(CONFIG_I2C_EDID) && \ + (defined(CONFIG_SYS_I2C) || defined(CONFIG_DM_I2C)) U_BOOT_CMD_MKENT(edid, 1, 1, do_edid, "", ""), -#endif /* CONFIG_I2C_EDID */ +#endif U_BOOT_CMD_MKENT(loop, 3, 1, do_i2c_loop, "", ""), U_BOOT_CMD_MKENT(md, 3, 1, do_i2c_md, "", ""), U_BOOT_CMD_MKENT(mm, 2, 1, do_i2c_mm, "", ""), diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index 2069576958..2f340235ee 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -355,7 +355,6 @@ config VIDEO_MVEBU config I2C_EDID bool "Enable EDID library" - depends on DM_I2C default n help This enables library for accessing EDID data from an LCD panel.
I2C_EDID currently selects DM_I2C. However, it is not needed. I2C_EDID is used for building edid.c, which doesn't even use I2C bus, and by I2C command, which knows how to use DM and old style I2C interface, so it is not directly affected by this removal. Furthermore, this selection can generate warning if DM display driver is used on platform which doesn't implement DM I2C driver (for example, sunxi platform with upcoming DM video & display driver). Patch was tested with rockchip and sunxi boards and successfully compiled exynos and tegra targets. They are the only consumers of CONFIG_DISPLAY option, which is the only one which selects I2C_EDID. Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- cmd/i2c.c | 10 ++++++---- drivers/video/Kconfig | 1 - 2 files changed, 6 insertions(+), 5 deletions(-)