> Date: Fri, 14 Feb 2025 13:21:46 +0100 > From: Edgar Fuß <e...@math.uni-bonn.de> > > > Can you please file a PR > Yes, sure, but ... > > > explaining this use-case > this sounds like I'm doing something unreasonable?
If you have a reason to do it it may not be unreasonable! But usually, if a machine's designers meant for the operating system to reach into that part of the i2c address space, they would have described it in some kind of firmware binding. Often, the hardware interface is actually used by the firmware and exposed to the operating system through some hardware-independent interface like acpitz(4). If the OS tries to use the hardware interface directly, it might interfere with the firmware's simultaneous use of it and confuse everyone. If the hardware interface is meant to be used directly, usually there is some enumerable firmware description of it -- and we should use that instead of hard-coding magic numbers into the kernel config. `acpidump -dt' might help find it if it's there. It's possible that the machine's designers put the i2c device on the bus and _didn't_ put any kind of firmware wrapping around it. If that happened, I would expect to find it at least described in some kind of motherboard manual. But I guess it's also possible that the designers put extra sensor hardware in, wired it up to the i2c bus, and then didn't tell anyone, yet you found it anyway. > > and how you know it's safe on this board > That once again sounds like I was doing something dangerous or unreasonable. > I can't remember how I came to that configuration (probabably from some > "not configured" messages), but it gives me the data I want If you can find those `not configured' messages I'd be curious to see them. The only way this will happen if there is some bus or firmware enumeration (like ACPI) that describes the device to the OS -- and if so, we should be using the bus's description, not a magic number in the kernel config. > (and doesn't > cause any harm on hardware where that chip is not present). This is not a priori clear. Some other device could be wired up to the part of the i2c address space you've configured dbcool(4) to use. Who knows how it will interpret the register reads and writes that dbcool(4) issues? > > Adding a null test is probably all this code needs > Do you mean one guarding the prop_object_retain(sc->sc_prop) call? That and the prop_dictionary_get call in dbcool_attach_sensor, yes. > I would be afraid there's a complementing prop_object_release() call > somewhere, but I don't see one in dbcool_detach(). Most probably I'm > looking in the wrong place. Yes, dbcool_detach should do prop_object_release -- also conditional on whether sc_prop is null.
