Re: [PATCH v3 09/16] platform/x86: x86-android-tablets: Add include defining struct dmi_system_id

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6690 bytes --]

On Tue, 30 Jun 2026, Uwe Kleine-König (The Capable Hub) wrote:
> On Tue, Jun 30, 2026 at 05:58:54PM +0300, Ilpo Järvinen wrote:
> > On Mon, 29 Jun 2026, Uwe Kleine-König (The Capable Hub) wrote:
> > > On Mon, Jun 29, 2026 at 02:38:35PM +0300, Ilpo Järvinen wrote:
> > > > On Sun, 28 Jun 2026, Uwe Kleine-König (The Capable Hub) wrote:
> > > > 
> > > > > Currently <linux/i2c.h> includes <linux/mod_devicetable.h> transitively
> > > > > which ensures that struct dmi_system_id is defined in
> > > > > drivers/platform/x86/x86-android-tablets/x86-android-tablets.h. However
> > > > > this include in <linux/i2c.h> will be replaced by one for i2c_device_id
> > > > > only. To ensure that dmi_system_id is available add the include for that
> > > > > explicitly.
> > > > > 
> > > > > Acked-by: Danilo Krummrich <dakr@kernel.org>
> > > > > Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>
> > > > > ---
> > > > >  drivers/platform/x86/x86-android-tablets/x86-android-tablets.h | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h b/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
> > > > > index 2498390958ad..c756961ae5fd 100644
> > > > > --- a/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
> > > > > +++ b/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
> > > > > @@ -13,6 +13,7 @@
> > > > >  #include <linux/gpio/consumer.h>
> > > > >  #include <linux/i2c.h>
> > > > >  #include <linux/irqdomain_defs.h>
> > > > > +#include <linux/device-id/dmi.h>
> > > > >  #include <linux/spi/spi.h>
> > > > >  
> > > > >  struct gpio_desc;
> > > > 
> > > > Indirect include patchs are never a good idea as it always makes header 
> > > > reorganizations minefield. So for this change,
> > > > 
> > > > Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > 
> > > > 
> > > > When it comes to the series, I certainly like the direction it goes very 
> > > > much (never have been fan of mod_devicetable.h) but I'd have preferred 
> > > > stepwise approach over trying to introduce it to some mid-rc.
> > > 
> > > The hurdle here is that at least the header part isn't easily
> > > automateable.
> > 
> > Patch 1?
> 
> No, patch 15.
>
> > > (Well it is, but the script that I have now to check all
> > > .c files already takes longer than an hour to run. I guess it's not as
> > > optimal as it could, but still.) And so getting this ready for inclusion
> > > in next and keeping it up to date until the merge window opens is a
> > > tough job.

It looks to me more a problem with the chosen approach where you wanted 
to get it all quickly done in a single all-reaching series (which IMO 
would have been fine right after -rc1 but mid-rc cycle, I'm not so 
positive about it).

> > > The further downside is that each modification to one of the highly used
> > > headers is expensive (in rebuild time) when these are merged one after
> > > another (or when bisecting). So getting these changes in in one batch is
> > > beneficial.
> > > 
> > > So while I agree with your preference, I don't think it's feasible.
> > 
> > While I understand keeping it up-to-date must be a pain, I'm not entirely 
> > convinced by the rebuild time argument as it has been the status quo so 
> > far too for anything touching mod_devicetable.h.
> 
> Yes. I don't know about others, but big rebuilds annoy me regularily.
> And with another quest that involves touching most struct *_device_id
> this really gets a pain. And just because "we" endured something in the
> past, doesn't mean we shouldn't improve.
> 
> > If one commit would move all linux/device-id/* AND ADD them all into 
> > mod_devicetable.h as includes, that's just one rebuild more (and one 
> > rebuild will occur anyway whichever way you architect this).
> 
> Yes, one rebuild isn't possible to prevent. You're talking about patch
> #1, right?
> 
> > Only if includes would be then removed one-by-one from mod_devicetable.h 
> > (e.g. per subsystem basis), I can see it causing n rebuilds (+conflicts), 
> > but they could be removed in bulk as well.
> 
> Removing those from mod_devicetable.h isn't part of the plan.

It isn't because you're not doing this in steps so you didn't put them in 
there at all (which would have been fine at any time and only enforces 
one rebuild comparable to any other linux/mod_devicetable.h edit).

> But
> replacing `#include <linux/mod_devicetable.h>` by `#include
> <linux/device-id/of.h>` in <linux/of.h> triggers again many rebuilds
> when done separately.

But this is not because you touch linux/mod_devicetable.h but linux/of.h.
It certainly should cause rebuild of all the code depending on linux/of.h 
but I assume that is much less than what linux/mod_devicetable.h causes.

> And then for <linux/pci.h> and again for <linux/platform_driver.h> and
> .... That's why those are included here, too, to at least use the "one
> rebuild needed" window for the high-impact follow up changes, and
> trigger a rebuild only once (unless you happen to hit an inbetween
> commit while bisecting).

I disagree, it's a different set of files for each. But there could, of 
course, be some overlap. Maybe the overlaps is so significant it's 
nearly as same as linux/mod_devicetable.h, I don't know but is that what 
you're trying to imply?

> > What you can achieve is preventing those "normal" mod_devicetable.h 
> > changes enforcing rebuilds right after this has been applied, but that is 
> > just the rebuilds as status quo would have without this series and no 
> > more.
> 
> This is exactly the goal. I don't get the "just" part of your argument.
> The point is that a modification to struct i2c_device_id should only
> trigger a rebuild of i2c drivers, but not of pci, platform, spi and all
> the others, too.

With "just" I meant it's no worse than status quo. If that prolongs one or 
two kernel versions more to avoid mid-rc cycle merge it doesn't really 
seem so high cost.

And I understand where you want to end up, and you could end up there 
using steps as well:

1. add linux/device-id/* + include them from mod_devicetable.h
2. convert stuff to use linux/device-id/* without removing includes from 
mod_devicetable.h
3. bulk remove linux/device-id/* includes from mod_devicetable.h

2 & 3 are chunkable/iteratable so they don't need to occur for all at once 
as long as 3 is not split per one linux/device-id/* its rebuild cost isn't 
that huge compared with the status quo.

-- 
 i.