similar to ad5706r, the spi controller can be used for multiple devices (with the same chip sellect!). There are those IDx pins that defines device addressing. I wonder if this node should define #address-cells and #size-cells too. So the main driver that you're putting together might need to handle up to four instances.

Even if your current driver cannot handle this yet, the dt-bindings might need cover that:

spi {
	#address-cells = <1>;
	#size-cells = <0>;

	multi-dac@0 {
		compatible = "adi,ad5529r-16";
		reg = <0>;
		spi-max-frequency = <25000000>;

		#address-cells = <1>;
		#size-cells = <0>;

		dac@0 {
			reg = <0>;
			vdd-supply = <&vdd_regulator>;
			avdd-supply = <&avdd_regulator>;
			hvdd-supply = <&hvdd_regulator>;
			hvss-supply = <&hvss_regulator>;

			reset-gpios = <&gpio0 87 GPIO_ACTIVE_LOW>;

			#address-cells = <1>;
			#size-cells = <0>;

			channel@0 {
				reg = <0>;
				adi,output-range-microvolt = <0 5000000>;
			};

			channel@1 {
				reg = <1>;
				adi,output-range-microvolt = <(-10000000) 10000000>;
			};

			channel@2 {
				reg = <2>;
				adi,output-range-microvolt = <0 40000000>;
			};
		}

		dac@1 {
			reg = <1>;
			vdd-supply = <&vdd_regulator>;
			avdd-supply = <&avdd_regulator>;
			hvdd-supply = <&hvdd_regulator>;
			hvss-supply = <&hvss_regulator>;

			reset-gpios = <&gpio0 88 GPIO_ACTIVE_LOW>;

			#address-cells = <1>;
			#size-cells = <0>;

			channel@0 {
				reg = <0>;
				adi,output-range-microvolt = <0 5000000>;
			};

			channel@1 {
				reg = <1>;
				adi,output-range-microvolt = <(-10000000) 10000000>;
			};
		}
	};
};

Although some things can be common, each device can have their own supplies and resources.
Need to double check if each dac node needs a separate compatible, being a separate driver
(not sure if it would make sense to mix and match ad5529r-16 and ad5529r-12).

In fact, the register map of ad5529r and ad5706r are so similar, we should consider creating a common driver for them (especially if they belong to the same family). @actorreno, what do you think?

EDIT: actually, there some differences. Probably it is fine as separate drivers.

Can't we have the above outside of the child nodes? I see Rodrigo made some comments regarding this but does it make sense to actually have a multidevice scheme with both 12 and 16 bits variants?

This is an edge case, but the hardware does technically allow mixing 12 bit and a 16 bit variant on the same SPI bus using the ID pins. This was the only option I could think of, to accommodate this case. Do you suggest keeping it simple and support just one variant in the chain?

Having three different compatibles definitely looks odd to me. What I suggested would make it possible to only care about two compatibles but I still find it a bit odd to have a compatible in here if there is no driver to bind against it.

Maybe just keep it simple. Multi-device is already something that was not asked for now and we can see it already as an edge case and this might be even more niche. Moreover, if adding a compatible in the child nodes is accepted by the maintainers, I don't see a big problem in adding that when really needed.

There's something which is not exactly the same but with some similarities:

https://elixir.bootlin.com/linux/v7.0.11/source/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml#L97

But in the above case we really create a platform device and have a driver for it. But the driver details don't really matter for the bindings

Are the ID pins always required? I mean for the normal case where we just want one device? The above makes it feel it's mandatory.

The ID pin should actually map to the SPI command. The user is also allowed to configure it to a value which is not 0, even while operating it as a single device.

The present driver just considers the default value as ID=0 and does not accommodate any other configuration

I see. Well this might be ok as a simplification in the sense that we always end up using the i. But it does seem like unnecessary SPI churn for (maybe?) 99% of the usecases? So making the niche, the rule feels a bit odd.

Does the above makes sense? Might be hard to actual put this into yaml but I think we're forgetting the normal case where only one device is to be used and ID pins don't really matter? But yeah, might not be a big deal in the end of the day

Going back to this story maybe we can just treat the parent node (in this case what you have with "Shared chip-select endpoint for one or more AD5529R DAC devices.") as the default dac0 node with implied ID pins = 0 (whether that matters or not) and then we can have optional dac nodes from 1 to 3? And to avoid duplicated property definition we could maybe do something like in here?

https://elixir.bootlin.com/linux/v7.0.11/source/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml#L21

Thougts? @rodrigo455, @jansunil

I suppose that works, but it gets the dt-bindings even more complicated. To me that seems like a workaround on a design that didn't foresee future needs. My concern was just not to lock the multi-device feature. I suppose that the device ID should be clear in the device-tree, because even if using only one single device, depending on how those ID pins are connected, the spi message needs to refer to the correct ID, otherwise the part will ignore the payload. The assumption that ID = 0 for the single device case is not necessarily true. Maybe with proper documentation we can get by.

I suppose that works, but it gets the dt-bindings even more complicated.

I don't fully agree with it. As it stands right now, it's more complicated and non obvious what's going on. 3 compatibles as we have now looks very odd to me. We could have an explicit adi,pin-id property and only add the dac nodes (or make them required) if that property is given but that would complicate bindings a lot.

I suppose that the device ID should be clear in the device-tree, because even if using only one single device, depending on how those ID pins are connected, the spi message needs to refer to the correct ID, otherwise the part will ignore the payload. The assumption that ID = 0 for the single device case is not necessarily true. Maybe with proper documentation we can get by.

With the above I can agree to some extent. Implying things (even more implying HW) is probably never a good thing. Maybe we just need an explicit ID property and have an explicit value for "don't care"? And that being the default value?

I feel we're discussing a feature that might not be supported at all and even if it does, should account for 1% of usecases? But yes, having proper bindings now can save us some time in the future (possibly :))

yes, an ID property solves the issue for the single device use case, which would be the most common. I also agree with you to some extent. However, I don't understand how we find difficulty on supporting this, as if it would be really that painful... Why not avoid vendor specific property and use address-cells/reg which are meant exactly for this (device addressing).

We can use address/reg as we're doing now. But that is only meaningful for when we want multi-device support no? Which is the niche case. So having those sub nodes mandatory seems a bit of extra churn.

Then, we can say, well just do dac@0 for the normal case but then reg has nothing to do with the ID pins given that in the normal scenario, the ID pins are meaningless (so back to where we are now). As said, also don't love the 3 compatible we have in place. The more natural way to me is

dac@0 {
    reg = <0>; //real chip select
    // all other properties
}

Above is normal mode; Then,

dac@0 {
    reg = <0>; //real chip select
    // all other properties and should we have an explicit id pin?
   
   dac@1 {
      reg = <1>;
      // all other properties
   }
}

So the main node always define one device whether we have multi device or not. To avoid duplication defining the same properties, I suggested that way to define a custom node. The above is still not great because there's a clear overlap between a possible vendor id pin and reg so it needs more care! Just an idea...

However, I don't understand how we find difficulty on supporting this, as if it would be really that painful...

Well, that's up to @jansunil but I also don't think it's trivial either (at least proper HW description). And without a proper usecase and way to test it (which would require asking additional chips), you just risk shooting yourself in the foot. Not worth the risk IMHO. And here I just mean the driver implementation

We should also make sure if we really need to duplicate all the properties? I would think that, at least, supplies would be shared between the devices?

dac@0 {
    reg = <0>; //real chip select
    // all other properties and should we have an explicit id pin?
   
   dac@1 {
      reg = <1>;
      // all other properties
   }
}

This is reasonable when adding multi-device support later, once the initial design didn't consider that at all. Allows for a phased approach (so the dt-bindings can be extended later) but ends up with a weird node hierarchy. The reason that this is not a good design is mostly a "separation of concerns" argument. Having an address field in the SPI command means we kind have virtual layers: bus control and dac control (similar to I2C). A top-level driver handles spi stuff and the lower-level ones handles the dac. I tend to prefer a design meant for extensibility, rather than allow for organic growth. For a simple driver that might not matter much though.

And without a proper usecase and way to test it (which would require asking additional chips), you just risk shooting yourself in the foot. Not worth the risk IMHO.

Analyzing the SPI protocol messages and having it working for a single-device is enough to validate the multi-device use case. The address field just need to have the proper value.

A top-level driver handles spi stuff and the lower-level ones handles the dac. I tend to prefer a design meant for extensibility, rather than allow for organic growth. For a simple driver that might not matter much though.

We are not designing a subsystem in here :). I agree its a bit "weird node hierarchy" but I also don't like much of the virtual top level (with hacky compatible) device that only exists to "create" the real devices. Not really how things are laid down in HW (for this usecase).

If we ever get to implement this we might also try to get this done directly in the spi subsystem. If I'm not missing nothing the only issue is that we can't reuse CS (at least that's how it used to be) so we can just justify support for that. In fact, there might be already something we could use:

https://elixir.bootlin.com/linux/v7.1-rc7/source/drivers/spi/spi.c#L610

Did not looked that deeply but it seems it does not fit our usecase 100% as we can have 4 ID pins + 1 real CS. So, 4 virtual CS. Could be as easy as incrementing:

https://elixir.bootlin.com/linux/v7.1-rc7/source/include/linux/spi/spi.h#L24

And the above would resolve the ID pins. But we would still need to have other properties that would need to map to the proper device and that does not fit that well. @jansunil have you checked with the apps engineer if in this mode we really have different supplies, resets, etc? It seemed to me this mode is more to see the whole thing as an "aggregate" device that can extend to 16 channels...

Could make more sense to just have a spi DT property (together with proper spi core support) to allow devices to share physical CS lines so our bindings would become trivial and we would just have 4 different dac nodes and a vendor ID pin property.

Yes- while using multi device topology, the system is capable of having their own supplies. pins etc

But what is the typical case? At least for the supplies, I would be surprised...

I would move this into the firmware parsing function. Also you device_property APIs. With it, I guess of.h can be dropped

Yeah, not really sure this compatible per node is easy very nice. Maybe adding platform_devices can be an option as Rodrigo suggested but OTOH having a separate driver for the child nodes seems too much.

So the bigger question again is, does it make sense to have multi device with 16 and 12 bit variants mixed?

Also use device_property APIs where possible.

There's a lot of duplication below. If @rodrigo455 is fine with it, I would just not bother with this for now. From all the discussions we had so far, it seems we can extend bindings and the driver if we have a real usecase for this. Right now there's no enough consensus :).

But up to you! You had the work already so if you want to bring this upstream, your call

Yeah... In this state I would not even add those. As adding them later would not break the ABI, and would tell a better story on why this strategy took place.

Repetition is avoided adding the $defs Nuno pointed out before.

I still do not agree with all of this though! Something we may want to discuss upstream with dt folks.

In the pass, these toggle pins were also handled by clocks. What does the datasheet states in here?

TG0-TG3 are plain digital inputs as per the DS. They need to be pulsed to trigger DAC updates for function generation in case of non transparent mode. These connect to a PWM peripheral.

But what is the typical case? At least for the supplies, I would be surprised...