Re: [PATCH net-next v4] Add Mellanox BlueField Gigabit Ethernet driver

[Andrew Lunn <andrew@lunn.ch>]

> > > +	if (device_property_read_u32(&pdev->dev, "version", &version)) {
> > > +		dev_err(&pdev->dev, "Version Info not found\n");
> > > +		return -EINVAL;
> > > +	}
> > 
> > Is this a device tree property? ACPI? If it is device tree property you need to
> > document the binding, Documentation/devicetree/bindinds/...
> > 
> 
> This driver gets its properties from an ACPI table, not from device tree.
> The "version" property is read from the ACPI table, and if an incompatible
> table version is found the driver does not load.  This logic allows the version
> of the driver and the version of the ACPI table to change and compatibility
> is ensured.  If there's a different/better way to do this, please let me know.

I tend to avoid ACPI. DT seems a better system. DT tries hard to not
break backwards compatibility. Hence it is not really versioned. The
fact you are versioning things and won't load if there is a version
mismatch suggests you don't think backward compatibility is that
important. I personally would not have a version, and promise to keep
backwards compatibility. The current properties you have seem simple
enough.

> 
> > > +
> > > +	if (version != (int)DRV_VERSION) {
> > > +		dev_err(&pdev->dev, "Version Mismatch. Expected %d
> > Returned %d\n",
> > > +			(int)DRV_VERSION, version);
> > > +		return -EINVAL;
> > > +	}
> > 
> > That is odd. Doubt odd. First of, why (int)1.19? Why not just set DRV_VERSION
> > to 1? This is the only place you use this, so the .19 seems pointless. Secondly,
> > what does this version in DT/ACPI actually represent? The hardware version?
> > Then you should be using a compatible string? Or read a hardware register which
> > tells you have hardware version.
> > 
> 
> The value of DRV_VERSION is 1.19 because it specifies the <major>.<minor>
> version of the driver.

Driver versions are pretty pointless. Don't you expect somebody to
back port this into a vendor kernel? Everything around it has
changed. What does this version then tell you? Nothing particularly
useful, since what you really want to know is the release tag in the
source tree, so you have the driver and everything around it you can
then debug.

> > > +
> > > +	err = device_property_read_u32(&pdev->dev, "phy-int-gpio",
> > &phy_int_gpio);
> > > +	if (err < 0)
> > > +		phy_int_gpio = MLXBF_GIGE_DEFAULT_PHY_INT_GPIO;
> > 
> > Again, this probably needs documenting. This is not how you do interrupts with
> > DT. I also don't think it is correct for ACPI, but i don't know ACPI.
> > 
> 
> Right, the "phy-int-gpio" is a property read from the ACPI table.
> Is there a convention for ACPI table use/documentation?

https://www.kernel.org/doc/html/latest/firmware-guide/acpi/gpio-properties.html

This seems to be the correct way to describe GPIOs in ACPI. But you
are using the GPIO as an interrupt sources. Either you need to get the
GPIO descriptor and then map it to an interrupt, or there might be
another way to describe interrupts in ACPI. I very much doubt
device_property_read_u32() is the correct way to do this, but as i
said, i try to avoid ACPI.
 
> > > +	phydev = phy_find_first(priv->mdiobus);
> > > +	if (!phydev) {
> > > +		mlxbf_gige_mdio_remove(priv);
> > > +		return -ENODEV;
> > > +	}
> > 
> > If you are using DT, please use a phandle to the device on the MDIO bus.
> > 
> 
> The code is not using DT.

Yes, you just have to hope the first device on the bus is the correct
device.

	Andrew