RE: [patch v4 1/2] mfd: Add Mellanox regmap core driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> > folder;  Comments pointed out by Rob:
> >  - Make a separate patch /devicetree/bindings/vendor-prefixes.txt;
> >  - Add .txt to Documentation/devicetree/bindings/mfd/mellanox,mlxreg-
> core
> >    and send it within this series;
> 
> Why did you combine this? Bindings should be separate patches.

Ack.

> 
> > +			spi_burn_bios_ci {
> > +				reg = <0x32>;
> > +				mask = <0xfe>;
> > +				bit = <0x00>;
> > +				phandles = <&spi2>;
> > +			};
> > +			spi_burn_ncsi {
> > +				reg = <0x32>;
> 
> You can't have duplicate reg values. This will also generate warnings.
> Build your dtb with "W=2".

Can I use some logical name for unit, like below?
			spi_burn_bios_ci: mux@1 {
				reg = <0x32>;
				mask = <0xfe>;
				bit = <0x00>;
				hotplugs = <&spi2>;
			};
			spi_burn_ncsi: mux@2 {
				reg = <0x32>;
				mask = <0xfd>;
				bit = <0x00>;
				hotplugs = <&spi2>;
			};

I verified it with "W=2". Also I am running with CONFIG_OF_UNITTEST and
I don't have unites issues.

> 
> Again, this all looks too low level to me. You described the high level
> differences in terms of number of cards, fans, PSUs, etc., but I don't see that
> here. This isn't reviewable for anyone not familiar with your systems.

The below should describe signals, which triggers hotplugable device
Insertion or removal.
		pwr: hotplugs@1 {
			aggr = <0x08>;
			reg = <0x64>;
			mask = <0x03>;
			hotplugs = <&psu1_ctrl &psu2_ctrl>;
		};

		asic: hotplugs@3 {
			aggr = <0x01>;
			regs = <0x50>;
			mask = <0x03>;
			hotplugs = <&asic_thermal>;
		};
In DTS I have description for such devices in disabled state and they are
Connected or disconnected dynamically:
&i2c3 {
...
	psu1_ctrl: dps460@59 {
		compatible = "dps460";
		reg = <0x59>;
		status = "disabled";
	};
''

&i2c12 {
...
	asic_thermal: mlxsw_minimal@48 {
		compatible = "mlxsw_minimal";
		reg = <0x48>;
		status = "disabled";
             ...

And the same way I'd like to add spine and leaf cards, but I still don't
Have support for them in my drivers.

> 
> > +				mask = <0xfd>;
> > +				bit = <0x00>;
> > +				phandles = <&spi2>;
> 
> Not a good name other than giving type information. Name it for what
> function is being provided (e.g. clocks, interrupts, gpios).

Would it be OK if to use hotplugs instead of phandles?
It could be f.e. FAN EEPROM, PSU controller, ASIC, spine or leaf card, all these 
are hotplugable device, which could be inserted or removed.
It could be remote flashes, which are getting available after mux selection, like
in the above example for spi_burn_bios_ci and spi_burn_ncsi.
Or it could be some other device at the remote side, which is getting available to
BMC, only when remote side (f.e. COMEX) is getting power good.

Thanks,
Vadim.
 
 




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux