Hi, On 11/25/20 12:20 PM, Andy Shevchenko wrote: > On Wed, Nov 25, 2020 at 1:11 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >> On 11/25/20 11:55 AM, Andy Shevchenko wrote: >>> On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > ... > >>> I'm wondering if we can meanwhile update hwdb to support >>> i2c-multi-instantiate cases in the future and in a few years switch to >>> it. >> >> Even if we fix current hwdb entries to match on both, then there >> is no guarantee newly added entries will also contain the new match. >> >> Now with the code to get the matrix from the ACPI tables new entries >> should happen less often, but I saw at least one model where the ACPI >> provided matrix appears to be wrong (if the ACPI matrix was always >> correct then breaking hwdb would not really be an issue). >> >> So I don't think this is going to work and all in all it feels like >> a lot of work for little gain. > > Okay! > > ... > >>>> + .dev_name = "BOSC0200:base", >>> >>> Hmm... Can we use '.' (dot) rather than ':' (colon) to avoid confusion >>> with ACPI device naming schema? (Or was it on purpose?) >> >> So with the ':' the end result is: >> >> [root@localhost ~]# cd /sys/bus/i2c/devices/ >> [root@localhost devices]# ls | cat >> 6-0050 >> i2c-0 >> i2c-1 >> i2c-2 >> i2c-3 >> i2c-4 >> i2c-5 >> i2c-6 >> i2c-BOSC0200:00 >> i2c-BOSC0200:base >> i2c-WCOM50BD:00 >> >> Which looks nice and consistent, which is why I went with the ':' >> and since base is not a number, there is no chance on conflicting with >> ACPI device names (it does look somewhat like an ACPI device name, but >> it is an ACPI enumerated device, so ...). > > I see. So this was made on purpose. > >> Anyways if there is a strong preference for changing this to a '.' >> I would be happy to make this change. > > No strong preferences from my side. > >>> And this seems to be the only device in the system, second as this is >>> not allowed as far as I understand. Right? >> >> I don't understand what you are trying to say here, sorry. >> >>> But theoretically I can >>> create an ACPI SSDT with quite similar excerpt and sensor and >>> enumerate it via ConfigFS (I understand that is quite unlikely). > > What if you have two devices with the same ID and both have two > I2cSerialBusV2() resources? Second one can't be instantiated because > 'base' is already here. > Making it like i2c-BOSC0200:00.base would be much better in my opinion. Ah I see, that is a somewhat valid point. But I really never expect there to be 2 ACPI devices with a BOSC0200 hw-id, while also specifying more then 1 i2c-client per node. That would just be all kinds of messed-up. Thinking about this I think that getting a WARN_ON (and thus a bug report) about a duplicate kobject-name when this happens would actually be good, because then we need to figure out what the beep is going on on that system. Note that other then triggering a WARN_ON the second i2c_acpi_new_device will simply fail in this very unlikely scenario (I know because I triggered this by accident while working on the patch). Since in a way getting this WARN_ON is actually good (lets us know about completely unexpected circumstances) and that making the name dynamic as you suggest requires a bit of extra code I would actually prefer to keep this as. Please let me know if that is ok with you. Regards, Hans
