[GIT PULL] Backlight for v6.1

[GIT PULL] Backlight for v6.1

[Lee Jones]

Good afternoon Linus,

The following changes since commit 568035b01cfb107af8d2e4bd2fb9aea22cf5b868:

  Linux 6.0-rc1 (2022-08-14 15:50:18 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git tags/backlight-next-6.1

for you to fetch changes up to e7647de058cba3c05b0d4753d84cbc042d02956a:

  video: backlight: mt6370: Add MediaTek MT6370 support (2022-09-08 08:50:57 +0100)

----------------------------------------------------------------
 - New Drivers
   - Add support for MediaTek MT6370 Backlight

----------------------------------------------------------------
ChiYuan Huang (1):
      dt-bindings: backlight: Add MediaTek MT6370 backlight

ChiaEn Wu (1):
      video: backlight: mt6370: Add MediaTek MT6370 support

 .../leds/backlight/mediatek,mt6370-backlight.yaml  | 121 +++++++
 drivers/video/backlight/Kconfig                    |  13 +
 drivers/video/backlight/Makefile                   |   1 +
 drivers/video/backlight/mt6370-backlight.c         | 351 +++++++++++++++++++++
 4 files changed, 486 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/mediatek,mt6370-backlight.yaml
 create mode 100644 drivers/video/backlight/mt6370-backlight.c

-- 
Lee Jones [李琼斯]

Re: [GIT PULL] Backlight for v6.1

[Linus Torvalds]

On Wed, Oct 5, 2022 at 5:44 AM Lee Jones <lee@kernel.org> wrote:
>
>    - Add support for MediaTek MT6370 Backlight

Hmm. This new driver has a

        depends on MFD_MT6370

but there is no such symbol anywhere.

It turns out the same is true of the MT6370 regulator driver that was
added during the previous merge window.

I do see that MFD_MT6370 in linux-next, but I don't see any pull
request for this, and now that I started looking I do see that we had
this already in 6.0.

I do *not* believe that it's ok to randomly take "drivers" that depend
on functionality that hasn't even been merged yet, and that are
basically just dead code but hidden away this non-obvious way.

I've pulled this, but I want to just state that this is bad, bad, bad.
If it has dependencies that aren't met, it damn well shouldn't be sent
upstream in a form where upstream can't even build test the thing.

                   Linus

Re: [GIT PULL] Backlight for v6.1

[pr-tracker-bot]

The pull request you sent on Wed, 5 Oct 2022 13:44:48 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git tags/backlight-next-6.1

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a47e60729d9624e931f988709ab76e043e2ee8b9

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] Backlight for v6.1

[Lee Jones]

On Wed, 05 Oct 2022, Linus Torvalds wrote:

> On Wed, Oct 5, 2022 at 5:44 AM Lee Jones <lee@kernel.org> wrote:
> >
> >    - Add support for MediaTek MT6370 Backlight
> 
> Hmm. This new driver has a
> 
>         depends on MFD_MT6370
> 
> but there is no such symbol anywhere.
> 
> It turns out the same is true of the MT6370 regulator driver that was
> added during the previous merge window.
> 
> I do see that MFD_MT6370 in linux-next, but I don't see any pull
> request for this, and now that I started looking I do see that we had
> this already in 6.0.
> 
> I do *not* believe that it's ok to randomly take "drivers" that depend
> on functionality that hasn't even been merged yet, and that are
> basically just dead code but hidden away this non-obvious way.
> 
> I've pulled this, but I want to just state that this is bad, bad, bad.
> If it has dependencies that aren't met, it damn well shouldn't be sent
> upstream in a form where upstream can't even build test the thing.

PR satisfying this dependency was submitted the following day:

https://lore.kernel.org/all/Yz6Zi7B2RS16sXhT@google.com/

-- 
Lee Jones [李琼斯]

Re: [GIT PULL] Backlight for v6.1

[Linus Torvalds]

On Fri, Oct 7, 2022 at 6:16 AM Lee Jones <lee@kernel.org> wrote:
>
> PR satisfying this dependency was submitted the following day:

.. you ignored the whole "another driver hit v6.0 without ever getting
the dependency".

So this whole thing with "dead driver because the config option it
depended on didn't even exist" wasn't some little temporary thing.
It's literally there in a released kernel, which has a whole driver in
it that cannot actually even be enabled.

And now that I *did* get the MFD update, I notice that the build
coverage of *that* is pitiful too.

In particular, there was a silent semantic conflict in the Crystal
Cove (intel PMIC) driver with the i2c changes. I noticed it because
there were other things going on, and I went and looked.

But most notably I *didn't* notice it due to any build coverage,
because the Kconfig for that thing seems designed to hide the driver.
It does have

        depends on (X86 && ACPI) || COMPILE_TEST

so that it *looks* like it should get compile test coverage even
outside of x86, but in reality,  even on x86 it's actually really hard
to test, because it also has

        depends on I2C_DESIGNWARE_PLATFORM=y

so if you do a regular "allmodconfig" build, that driver doesn't
actually get any build testing at all, because while that platform is
indeed enabled, it's only a module.

So I caught this particular issue, but I really think that code that
cannot be enabled at all even for build testing - or code that is
quite hard to enable either intentionally or by mistake - is a
problem. And right now MFD was involved in both of the issues I've
noticed this merge window.

That's not a great look.

                Linus

Re: [GIT PULL] Backlight for v6.1

[Andy Shevchenko]

On Fri, Oct 07, 2022 at 11:45:27AM -0700, Linus Torvalds wrote:
> On Fri, Oct 7, 2022 at 6:16 AM Lee Jones <lee@kernel.org> wrote:
> >
> > PR satisfying this dependency was submitted the following day:
> 
> .. you ignored the whole "another driver hit v6.0 without ever getting
> the dependency".
> 
> So this whole thing with "dead driver because the config option it
> depended on didn't even exist" wasn't some little temporary thing.
> It's literally there in a released kernel, which has a whole driver in
> it that cannot actually even be enabled.
> 
> And now that I *did* get the MFD update, I notice that the build
> coverage of *that* is pitiful too.
> 
> In particular, there was a silent semantic conflict in the Crystal
> Cove (intel PMIC) driver with the i2c changes. I noticed it because
> there were other things going on, and I went and looked.
> 
> But most notably I *didn't* notice it due to any build coverage,
> because the Kconfig for that thing seems designed to hide the driver.
> It does have
> 
>         depends on (X86 && ACPI) || COMPILE_TEST
> 
> so that it *looks* like it should get compile test coverage even
> outside of x86, but in reality,  even on x86 it's actually really hard
> to test, because it also has
> 
>         depends on I2C_DESIGNWARE_PLATFORM=y

The Intel PMICs are the beasts when we want to run the code on the real
hardware.  The above mentioned dependency is required due to ACPI wants
to poke some PMIC registers via OpRegion in the AML. And since it may
happen quite early at the boot time, we can't wait for I2C host controller
driver to be loaded later on. I don't remember all the details of what will
go bad for the user in such cases, perhaps Hans can clarify these bits.

> so if you do a regular "allmodconfig" build, that driver doesn't
> actually get any build testing at all, because while that platform is
> indeed enabled, it's only a module.
> 
> So I caught this particular issue, but I really think that code that
> cannot be enabled at all even for build testing - or code that is
> quite hard to enable either intentionally or by mistake - is a
> problem. And right now MFD was involved in both of the issues I've
> noticed this merge window.
> 
> That's not a great look.

-- 
With Best Regards,
Andy Shevchenko

Re: [GIT PULL] Backlight for v6.1

[Linus Torvalds]

On Sat, Oct 8, 2022 at 11:31 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> The Intel PMICs are the beasts when we want to run the code on the real
> hardware.

Yeah, I don't expect he driver to work on real hardware, but I'm
mostly worried about the fact that it also gets very little build
coverage.

The fact that I found the i2c semantic conflict mainly because I was
looking for it (it's cropped up multiple times this merge window) but
*without* seeing it as a build error, that's what worries me.

So I think the

    depends on I2C_DESIGNWARE_PLATFORM=y

might be better with a "|| COMPILE_TEST" to at least find the build
issues, even if actual runtime testing is a different anumal entirely.

               Linus

Re: [GIT PULL] Backlight for v6.1

[Hans de Goede]

Hi Linus,

On 10/8/22 21:02, Linus Torvalds wrote:
> On Sat, Oct 8, 2022 at 11:31 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> The Intel PMICs are the beasts when we want to run the code on the real
>> hardware.
> 
> Yeah, I don't expect he driver to work on real hardware,

I'm not sure what you mean here. I guess you mean that you
do not expect to be able to test the driver on real hw
yourself?

I have several x86 tablets with this PMIC and the driver
does actually work on real hw, I test it regularly.

As for the admittedly weird:

	depends on I2C_DESIGNWARE_PLATFORM=y

dependency, as Andy mentioned on most of these devices
the ACPI tables (often the _PS0 / _PS3 power on/off methods)
poke at the PMIC through ACPI Opregions which are registered
through the MFD driver. This all needs to be available early
on which is why I2C_DESIGNWARE_PLATFORM needs to be builtin
(the PMIC is connected to the system through a designware
I2C controller).

When built as a module we get a whole bunch of ACPI subsys
errors about OpRegion calls to a non registered OpRegion in
dmesg and this can also results in real bugs like e.g.
a touchscreen not working because it was not powered on.

After several bug reports about this I decided to add this
dependency to force distros to built I2C_DESIGNWARE_PLATFORM
into the main vmlinuz image if they want to enable these
drivers.

<snip>

>     depends on I2C_DESIGNWARE_PLATFORM=y
> 
> might be better with a "|| COMPILE_TEST" to at least find the build
> issues, even if actual runtime testing is a different anumal entirely.

Adding "|| COMPILE_TEST" sounds like a good idea to me. Note that as
I tried to explain the:
	
	depends on I2C_DESIGNWARE_PLATFORM=y

is actually there to avoid known runtime issues with having that
built as a module in combination with this (and a few other similar)
PMIC drivers.

Regards,

Hans

Re: [GIT PULL] Backlight for v6.1

[Linus Torvalds]

On Sat, Oct 8, 2022 at 12:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Yeah, I don't expect he driver to work on real hardware,
>
> I'm not sure what you mean here. I guess you mean that you
> do not expect to be able to test the driver on real hw
> yourself?

Well, that too, but I really along the lines of "make it build as a
module when I2C_DESIGNWARE_PLATFORM is a module"

Because if it depends on some symbols from I2C_DESIGNWARE_PLATFORM,
and that one can be a module, then the Intel PMI driver also needs to
be built as a module to just get the build coverage, at least.

And I can imagine that that will not work very well on actual hardware
with some of these core drivers that may want to initialize early?

But I'd love to at least have the build coverage.

                 Linus

Re: [GIT PULL] Backlight for v6.1

[Hans de Goede]

Hi,

On 10/9/22 01:23, Linus Torvalds wrote:
> On Sat, Oct 8, 2022 at 12:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Yeah, I don't expect he driver to work on real hardware,
>>
>> I'm not sure what you mean here. I guess you mean that you
>> do not expect to be able to test the driver on real hw
>> yourself?
> 
> Well, that too, but I really along the lines of "make it build as a
> module when I2C_DESIGNWARE_PLATFORM is a module"

Actually because the driver only works properly when builtin the
driver itself is a bool option not a tristate. So I am not sure
if even if we change the the:

        depends on I2C_DESIGNWARE_PLATFORM=y

to:

        depends on I2C_DESIGNWARE_PLATFORM=y || COMPILE_TEST

it still will not be build with an "allmodconfig" build? I must
admit I don't know what that does with bool options.

If it says yes to bool options, then yes adding the " || COMPILE_TEST"
will give you build coverage with an "allmodconfig" build and
I would be happy to submit a patch adding this for all the affected
x86 PMIC drivers.

> Because if it depends on some symbols from I2C_DESIGNWARE_PLATFORM,
> and that one can be a module, then the Intel PMIC driver also needs to
> be built as a module to just get the build coverage, at least.

See above, currently these are bool-s I guess we may be able to
come up with some Kconfig magic where they can be build as module
only when COMPILE_TEST is set ? Anyone now the right Kconfig magic
for this ?

> And I can imagine that that will not work very well on actual hardware
> with some of these core drivers that may want to initialize early?

Right, we want to enforce things, including the i2c controller driver
to be build in because that is necessary on actual hw and allowing
these to be build as modules has lead to bug reports in the past.

> But I'd love to at least have the build coverage.

I agree, see above for some gotchas though.

FWIW as a side-project I actively work on keeping these platforms working
well with the latest mainline kernel, so I do build these with the latest
rc kernels all the time.

Regards.

Hans

Re: [GIT PULL] Backlight for v6.1

[Lee Jones]

On Fri, 07 Oct 2022, Linus Torvalds wrote:

> On Fri, Oct 7, 2022 at 6:16 AM Lee Jones <lee@kernel.org> wrote:
> >
> > PR satisfying this dependency was submitted the following day:
> 
> .. you ignored the whole "another driver hit v6.0 without ever getting
> the dependency".

Not ignored.  I provided you with a response applicable to the
situation surrounding *this* pull-request.  As for the actions /
motives of other maintainers, I cannot / should not comment.

Admittedly, that is not to say this could not have happened solely
between 2 subsystems that I maintain!  The other subsystem maintainers
and I work together regularly, utilising immutable branches to ensure
we do not cause build breakage at merge-time, but we (clearly) do not
work to the same levels of due diligence with respect to dependencies
preventing full build test / coverage.

> In particular, there was a silent semantic conflict in the Crystal
> Cove (intel PMIC) driver with the i2c changes. I noticed it because
> there were other things going on, and I went and looked.

It appears as though, Andy, Hans and yourself are having a nice
conversation about this particular instance already - I'll leave you
to it.

> So I caught this particular issue, but I really think that code that
> cannot be enabled at all even for build testing - or code that is
> quite hard to enable either intentionally or by mistake - is a
> problem.

Unbuildable / untestable code is an interesting problem.  One which, I
must say, I haven't taken a particularly deep look into.  Even though 
MFDs (and their associated children) are particularly susceptible to
dependency issues that would otherwise prevent testing, I very much
doubt this problem is unique to MFD.

To your knowledge, has there been any research into unbuildable
drivers (/ subsystems!)?  There must have been some notable studies on
(static / running) code coverage analysis, but I'd be surprised if
these cover code that simply cannot be built / executed.

Until this point, I assumed my build-coverage was rather good.  It
covers varying compilers, 7 architectures, and many different
*_defconfigs which include allmodconfig and allyesconfig, totalling 70
unique kernel builds.

You have been mentioning allmodconfig a fair bit.  Are you also
including allyesconfig in your observations?  Does that not alleviate 
some of the angst around what should be built-in vs modules in terms
of buildability?

If this is as big of an issue as you say, perhaps we should invest
a little time to investigate some sound method(s) to scan for similar
instances.  Tricky to do, seeing how many different architectures /
platforms the kernel supports.

-- 
Lee Jones [李琼斯]

Re: [GIT PULL] Backlight for v6.1

[Randy Dunlap]

On 10/9/22 05:58, Hans de Goede wrote:
> Hi,
> 
> On 10/9/22 01:23, Linus Torvalds wrote:
>> On Sat, Oct 8, 2022 at 12:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Yeah, I don't expect he driver to work on real hardware,
>>>
>>> I'm not sure what you mean here. I guess you mean that you
>>> do not expect to be able to test the driver on real hw
>>> yourself?
>>
>> Well, that too, but I really along the lines of "make it build as a
>> module when I2C_DESIGNWARE_PLATFORM is a module"
> 
> Actually because the driver only works properly when builtin the
> driver itself is a bool option not a tristate. So I am not sure
> if even if we change the the:
> 
>         depends on I2C_DESIGNWARE_PLATFORM=y
> 
> to:
> 
>         depends on I2C_DESIGNWARE_PLATFORM=y || COMPILE_TEST

That change worked for me.

drivers/mfd/intel_soc_pmic_crc.o is built and
drivers/i2c/busses/i2c-designware-platform.ko is built.

> it still will not be build with an "allmodconfig" build? I must
> admit I don't know what that does with bool options.

I don't see a problem...

but I didn't test all of the other x86 PMIC drivers.
They are all built as loadable modules since I didn't make any changes
to their Kconfig entries.

> If it says yes to bool options, then yes adding the " || COMPILE_TEST"
> will give you build coverage with an "allmodconfig" build and
> I would be happy to submit a patch adding this for all the affected
> x86 PMIC drivers.
> 
>> Because if it depends on some symbols from I2C_DESIGNWARE_PLATFORM,
>> and that one can be a module, then the Intel PMIC driver also needs to
>> be built as a module to just get the build coverage, at least.

I don't see intel_soc_pmic_crc.c using any direct calls into
i2c-designware-platform code. If it calls into it, it must be thru some
indirect pointers (?).
It's not causing any build problems once the "|| COMPILE_TEST"
change is made.  (I am build testing on linux-next-20221019.)

> See above, currently these are bool-s I guess we may be able to
> come up with some Kconfig magic where they can be build as module
> only when COMPILE_TEST is set ? Anyone now the right Kconfig magic
> for this ?
> 
>> And I can imagine that that will not work very well on actual hardware
>> with some of these core drivers that may want to initialize early?
> 
> Right, we want to enforce things, including the i2c controller driver
> to be build in because that is necessary on actual hw and allowing
> these to be build as modules has lead to bug reports in the past.
> 
>> But I'd love to at least have the build coverage.
> 
> I agree, see above for some gotchas though.
> 
> FWIW as a side-project I actively work on keeping these platforms working
> well with the latest mainline kernel, so I do build these with the latest
> rc kernels all the time.


-- 
~Randy

Re: [GIT PULL] Backlight for v6.1

[Andy Shevchenko]

On Wed, Oct 19, 2022 at 08:31:33PM -0700, Randy Dunlap wrote:
> On 10/9/22 05:58, Hans de Goede wrote:
> > On 10/9/22 01:23, Linus Torvalds wrote:
> >> On Sat, Oct 8, 2022 at 12:59 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> >> Because if it depends on some symbols from I2C_DESIGNWARE_PLATFORM,
> >> and that one can be a module, then the Intel PMIC driver also needs to
> >> be built as a module to just get the build coverage, at least.
> 
> I don't see intel_soc_pmic_crc.c using any direct calls into
> i2c-designware-platform code. If it calls into it, it must be thru some
> indirect pointers (?).

It's on hardware level, the PMIC is connected to the I²C host controller,
which is Synopsys DesignWare and being services by the respective driver.

Any access to the PMIC's registers requires the I²C to be involved.
What we talked above is even bigger loop, that takes AML code in
the chain.

-- 
With Best Regards,
Andy Shevchenko

Re: [GIT PULL] Backlight for v6.1

[Hans de Goede]

Hi,

On 10/20/22 15:48, Andy Shevchenko wrote:
> On Wed, Oct 19, 2022 at 08:31:33PM -0700, Randy Dunlap wrote:
>> On 10/9/22 05:58, Hans de Goede wrote:
>>> On 10/9/22 01:23, Linus Torvalds wrote:
>>>> On Sat, Oct 8, 2022 at 12:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>>> Because if it depends on some symbols from I2C_DESIGNWARE_PLATFORM,
>>>> and that one can be a module, then the Intel PMIC driver also needs to
>>>> be built as a module to just get the build coverage, at least.
>>
>> I don't see intel_soc_pmic_crc.c using any direct calls into
>> i2c-designware-platform code. If it calls into it, it must be thru some
>> indirect pointers (?).
> 
> It's on hardware level, the PMIC is connected to the I²C host controller,
> which is Synopsys DesignWare and being services by the respective driver.
> 
> Any access to the PMIC's registers requires the I²C to be involved.
> What we talked above is even bigger loop, that takes AML code in
> the chain.

Right and the involvement of AML means that we need this all to
work early on during boot, which means that all the bits, including
the I2C controller needs to be builtin. So things will compile fine
without the "depends on I2C_DESIGNWARE_PLATFORM=y" but then things
start breaking at runtime.

After a bunch of bug-reports due to wrong kernel configs I decided to
add the "depends on I2C_DESIGNWARE_PLATFORM=y" and since then we have
received no more bug reports about it. So although technically there
is no dependency on symbols from the i2c-designware driver I would
still very much like to keep the dependency around.

As I already mentioned earlier in the thread adding a
' || COMPILE_TEST' to this is absolutely fine and if I get Cc-ed
on such a patch I'm more then happy to Ack it.

Regards,

Hans