On Tue, Mar 8, 2022 at 1:07 PM Andre Przywara <andre.przywara@xxxxxxx> wrote:
> On Tue, 8 Mar 2022 10:38:37 +0100 Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > On Mon, Mar 7, 2022 at 3:34 PM Andre Przywara <andre.przywara@xxxxxxx> wrote:
> > >
> > > Some Kconfig options have changed, some other platforms have been
> > > removed.
> >
> > Please split this up into logical chunks: list the platforms that were removed
> > and remove only the lines corresponding to those platforms in one patch,
> > do functional changes in separate patches each with a reason for doing them,
> > and cleanups (moving lines to match the savedefconfig output, removing lines
> > that are now the default) in one final patch.
>
> Actually this patch is meant to be only about the last past: to sync
> multi_v5_defconfig with the output of "make savedefconfig". .config stays
> the same. I initially tried to chase down the reason for each line change,
> but gave up quickly, because it becomes tedious to learn about this,
> especially about symbols that got *removed*. Also Kconfig is somewhat
> sensitive, a single "select" or "default" change in one random Kconfig can
> affect the result of savedefconfig.
>
> As I noted in the commit message, the .config does *not* change as a result
> of this patch, the whole purpose is just to make the next patch clearer.
Ok, then just change the symbols that move around, not the ones that
may have gone missing unintentionally.
> So I can try find the reason for each removal, if you like, but I am not
> sure that's worthwhile? It is my understanding that Kconfig changes tend
> to accumulate cruft in the various defconfigs over time. In U-Boot we gave
> up on reasoning, and just regularly sync the output of savedefconfig over
> to the *_defconfigs, to keep them minimal and meaningful.
>
> And I found Olof's commit 30b10c77837c ("ARM: defconfig: re-run
> savedefconfig on multi_v* configs") as a precedence for this kind of cleanup.
I know, but I never liked doing this because it hides regressions.
> > > CONFIG_AEABI=y
> > > CONFIG_HIGHMEM=y
> > > -CONFIG_ZBOOT_ROM_TEXT=0x0
> > > -CONFIG_ZBOOT_ROM_BSS=0x0
> > > CONFIG_ARM_APPENDED_DTB=y
> > > CONFIG_ARM_ATAG_DTB_COMPAT=y
> > > CONFIG_CPU_FREQ=y
> >
> > These were not removed, what happened here is that 'savedefconfig'
> > no longer produces the lines because they now match the defaults.
>
> Yes, I understand. Is there some policy here, for instance to keep
> those in, for clarity?
In this case, as with the moving lines around, there is no functional
change at all, and doing the same thing on older kernels will still
result in the same behavior. I'm not worried about those at all, so
just put them all into one patch.
> > > @@ -159,7 +151,6 @@ CONFIG_I2C_ASPEED=m
> > > CONFIG_I2C_AT91=y
> > > CONFIG_I2C_IMX=y
> > > CONFIG_I2C_MV64XXX=y
> > > -CONFIG_I2C_NOMADIK=y
> > > CONFIG_SPI=y
> > > CONFIG_SPI_ATMEL=y
> > > CONFIG_SPI_IMX=y
> >
> > This one is still there. Not sure why it's no longer enabled.
>
> It's not in the current .config. From what I can see, it depends on
> ARCH_AMBA, which is selected by ARCH_NOMADIK, but that one is not enabled
> by multi_v5_defconfig. Not sure if that is an oversight, or a change, a
> the dependency is bogus, or something else.
>
> If you find that useful, I can try to find those dependency chains for the
> other options, but I definitely lack the knowledge about the history of
> those old platforms, so I can't reason about them. But I could present you
> the findings and you can then say what to do?
I see this was the result of 66e0c12f9e17 ("ARM: nomadik: switch to use the
Nomadik I2C driver"). It's ok to remove this line and others like it,
just explain
what happened for these, as the driver is still enabled.
> > > CONFIG_REGULATOR_FIXED_VOLTAGE=y
> > > CONFIG_MEDIA_SUPPORT=y
> > > CONFIG_MEDIA_CAMERA_SUPPORT=y
> > > -CONFIG_V4L_PLATFORM_DRIVERS=y
> > > -CONFIG_VIDEO_ASPEED=m
> > > -CONFIG_VIDEO_ATMEL_ISI=m
> > > CONFIG_DRM=y
> > > CONFIG_DRM_ATMEL_HLCDC=m
> > > -CONFIG_DRM_PANEL_SIMPLE=y
> > > -CONFIG_DRM_PANEL_EDP=y
> > > CONFIG_DRM_ASPEED_GFX=m
> > > -CONFIG_FB_IMX=y
> > > -CONFIG_FB_ATMEL=y
> > > -CONFIG_BACKLIGHT_ATMEL_LCDC=y
> >
> > This doesn't look right at all. If you want to disable graphics support,
> > please do that in a separate patch and explain why we can't have those
> > any more. Are you running into problems with the vmlinux size?
>
> As I mentioned, the .config didn't change at all, so those options are
> already not included in mainline anymore.
>
> AFAICS, those last options depend on CONFIG_FB, which isn't enabled. Is
> that a regression due to the recent fbdev changes?
Correct, this part was clearly unintentional, and I don't ever want to see
a patch to remove lines like these with a changelog text that fails to
explain why we want them to be removed. We clearly have platforms
that are enabled in multi_v5_config that use some of those drivers, and
we had users that wanted them to be enabled.
I think there are three separate issues here: FB_IMX and FB_ATMEL
broke because of f611b1e7624c ("drm: Avoid circular dependencies for
CONFIG_FB"), VIDEO_ASPEED and VIDEO_ATMEL_ISI broke
during b0cd4fb27665 ("media: Kconfig: on !EMBEDDED && !EXPERT,
enable driver filtering"), and PANEL_EDP was introduced in 310720875efa
("ARM: configs: Everyone who had PANEL_SIMPLE now gets
PANEL_EDP") without the option ever making it in [1]. These are all
bugs that need to be addressed individually, and not just in a single
defconfig file but across all the affected platforms.
Arnd
[1] https://lore.kernel.org/all/CAD=FV=VbYcdSqxLHdSaDPh=X0hbW6VWV0mM-iFy3k0J1q+6MWg@xxxxxxxxxxxxxx/
