overlays: i2c-fan: add compatible for upstream compatibility

[nullr0ute]

The way upstream deals with compatibles is to add emc2305 and then deal with the variations in the driver as it's detectable so add emc2305 to the compat so we work with the upstream driver.

[nullr0ute]

FYI: the upstream driver looks in a better state recently.

[pelwell]

Tailoring the Device Tree to match the Linux driver implementation seems to go against the upstream DT rules.

[6by9]

Tailoring the Device Tree to match the Linux driver implementation seems to go against the upstream DT rules.

It's as required by the bindings
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/hwmon/microchip%2Cemc2305.yaml#L21-L26

  compatible:
    oneOf:
      - enum:
          - microchip,emc2305
      - items:
          - enum:
              - microchip,emc2303
              - microchip,emc2302
              - microchip,emc2301
          - const: microchip,emc2305

It's using emc2305 as the fallback that the driver picks up.
Should some issue be discovered with emc2301 that can't be handled by autodetection, then microchip,emc2301 gets added to the driver as a specific compatible with whatever special handling is required.

LGTM.

[pelwell]

I should have been more explicit: I think the upstream driver is breaking the principles of OS-neutral DT files.

[nullr0ute]

I should have been more explicit: I think the upstream driver is breaking the principles of OS-neutral DT files.

I completely agree, I tried to send the compatibles upstream and I got this response.

With this change it works with the upstream and I didn't have the strength to argue. I was considering also reverting the fork you have downstream (which appears to be while upstream worked out what to do) and pull back the upstream changes but I thought I would start with this.

[pelwell]

I didn't have the strength to argue

I know the feeling. It probably shouldn't be the case, but if you'd included some of this in your PR I would probably have merged it sooner.

[6by9]

With this change it works with the upstream and I didn't have the strength to argue.

Isn't that everyone's response to trying to do anything device tree related, especially bindings!

I was considering also reverting the fork you have downstream (which appears to be while upstream worked out what to do) and pull back the upstream changes but I thought I would start with this.

Yes it was an earlier patch set. The CM4IO board uses EMC2301 and no one had checked there was a driver at that point when deciding to use the chip.

The DT docs do say to use fallback compatibles "when devices are the same as or a superset of prior implementations", but emc2305 didn't exist before emc2301, so why did it get precedence? A wildcard of emc230x would have made more sense for the fallback.

It sounds like we need to review the mainline emc2305 driver again to see what state it's in.
It would be nice if most of 8ee1366 can be ditched, but it looks like thermal zones still aren't used. Perhaps that's been merged into the core (I can hope).

I'd hope we'll have it cleaned up before the next LTS release. Seeing as we always ship kernel and DT files together, we don't care as much about ABI in DT.

[nullr0ute]

I know the feeling. It probably shouldn't be the case, but if you'd included some of this in your PR I would probably have merged it sooner.

It was still pretty quick ;-) and I sort of did both in parallel and wasn't fully up on threads

[nullr0ute]

The DT docs do say to use fallback compatibles "when devices are the same as or a superset of prior implementations", but emc2305 didn't exist before emc2301, so why did it get precedence? A wildcard of emc230x would have made more sense for the fallback.

No idea, and all the upstream examples are the 2301 so ¯_(ツ)_/¯ I think to some degree the HW can be queries for the exact variant so essentially the DT should be generic or something.

It sounds like we need to review the mainline emc2305 driver again to see what state it's in. It would be nice if most of 8ee1366 can be ditched, but it looks like thermal zones still aren't used. Perhaps that's been merged into the core (I can hope).

Yes, so it has the cooling devices pieces, which you then link to a thermal zone, you can see an example here. I think that should have all the bits needed to be able to set that up.

I'd hope we'll have it cleaned up before the next LTS release. Seeing as we always ship kernel and DT files together, we don't care as much about ABI in DT.

When I get a moment I'm happy to do a PR to clean stuff up, let me know what branch you'd like to do it against.

[6by9]

It looks like the earlier versions of the driver had extra DT parameters that got dropped (emc2305,cooling-levels, emc2305,pwm-max, emc2305,pwm-min, and emc2305,pwm-channel). It looks like those got folded into the fan child nodes from the example you linked to, eg

				fan0: fan@0 {
					reg = <0x0>;
					pwms = <&fan_controller 26000 1 PWM_POLARITY_INVERTED>;
					#cooling-cells = <2>;
				};

There's also code left over from a non-DT option which is redundant.

I think the only fragment we probably do still want is

	/* Acknowledge any existing faults. Stops the device responding on the
	 * SMBus alert address.
	 */
	i2c_smbus_read_byte_data(client, EMC2305_REG_FAN_STALL_STATUS);
	i2c_smbus_read_byte_data(client, EMC2305_REG_FAN_STATUS);

to do what it says.

It you did want to look, then it's easiest from our side to take the latest released branch (currently rpi-7.1.y), revert anything patches you don't want (ie currently 8ee1366), and then create clean patches for the bits we do want to keep. That means that when rebasing the next branch we can drop the unwanted patches and their reverts, and just have the desired bits.

[6by9]

I happen to have half an hour spare now, so I'm looking at the driver cleanups.

[nullr0ute]

awesome, thx 👍