Re: [PATCH 5/5] ARM: i.MX8MM: add Hexagon Geosystems GS05

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Feb 09, 2026 at 12:39:01PM +0100, Marco Felsch wrote:
> On 26-02-09, Sascha Hauer wrote:
> > On Thu, Feb 05, 2026 at 04:45:07PM +0100, Marco Felsch wrote:
> > > +#include <common.h>
> > > +#include <deep-probe.h>
> > > +#include <envfs.h>
> > > +#include <environment.h>
> > > +#include <init.h>
> > > +#include <i2c/i2c.h>
> > > +#include <linux/phy.h>
> > > +#include <mach/imx/bbu.h>
> > > +#include <mach/imx/generic.h>
> > > +#include <mfd/hgs-efi.h>
> > > +#include <of.h>
> > > +#include <state.h>
> > > +
> > > +#define PHY_ID_AR8031	0x004dd074
> > > +#define AR_PHY_ID_MASK	0xffffffff
> > > +
> > > +#define HGS_GS05_BASE_NAME	"Hexagon Geosystems GS05"
> > > +
> > > +#define HGS_GS05_MACHINE(_revid, _compatible, _model_suffix) \
> > > +	HGS_MACHINE(_revid, _compatible, HGS_GS05_BASE_NAME " " _model_suffix)
> > > +
> > > +struct hgs_machine hgs_gs05_variants[] = {
> > 
> > static
> 
> Sure.
> 
> > > +	HGS_GS05_MACHINE(HGS_BOARD_REV_C, "hgs,gs05-rev-c", "Rev-C"),
> > > +	HGS_GS05_MACHINE(HGS_BOARD_REV_D, "hgs,gs05-rev-d", "Rev-D"),
> > > +	{ /* Sentinel */ }
> > > +};
> > > +
> > > +#define HGS_GS05_LEGACY_MACHINE(_revchar, _revid, _compatible, _model_suffix) \
> > > +{									\
> > > +	.revision = _revchar,						\
> > > +	.machine = HGS_GS05_MACHINE(_revid, _compatible, _model_suffix) \
> > > +}
> > > +
> > > +struct hgs_gs05_legacy_machine {
> > 
> > static
> 
> Sure.
> 
> > > +	u8 revision;
> > > +	struct hgs_machine machine;
> > > +} hgs_gs05_legacy_variants[] = {
> > > +	HGS_GS05_LEGACY_MACHINE('C', HGS_BOARD_REV_C, "hgs,gs05-rev-c", "Rev-C"),
> > > +	HGS_GS05_LEGACY_MACHINE('D', HGS_BOARD_REV_D, "hgs,gs05-rev-d", "Rev-D"),
> > > +	{ /* Sentinel */ }
> > > +};
> > > +
> > > +static int ar8031_phy_fixup(struct phy_device *phydev)
> > > +{
> > > +	/* enable rgmii rxc skew and phy mode select to RGMII copper */
> > > +	phy_write(phydev, 0x1d, 0x1f);
> > > +	phy_write(phydev, 0x1e, 0x8);
> > > +	phy_write(phydev, 0x1d, 0x00);
> > > +	phy_write(phydev, 0x1e, 0x82ee);
> > > +	phy_write(phydev, 0x1d, 0x05);
> > > +	phy_write(phydev, 0x1e, 0x100);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct hgs_machine *
> > > +hgs_gs05_get_board_from_legacy(const unsigned char *serial)
> > > +{
> > > +	struct hgs_gs05_legacy_machine *machine = hgs_gs05_legacy_variants;
> > > +
> > > +	for (; machine->revision; machine++)
> > > +		if (serial[6] == machine->revision)
> > > +			return &machine->machine;
> > 
> > Drop sentinel and use ARRAY_SIZE?
> 
> Is this rather a personal preference or barebox coding style?
> 
> I would like to keep the sentinel-approach since we use the same
> approach for other boards too which allowed us simpler code
> constructions.

As you've seen in this patch such a sentinel is easily forgotten. When
you use the arrays non locally and have to pass the number of array
entries around as an additional argument, then Ok, use the sentinel
approach. But since you don't do that, what's the good argument for not
using ARRAY_SIZE and save a few bytes of binary space?

> > [...]
> > 
> > > +	err = imx_hab_lockdown_device(flags);
> > > +	if (err) {
> > > +		dev_err(dev, "Failed to lockdown the device\n");
> > > +		return err;
> > > +	}
> > > +	dev_info(dev, "Lockdown of the device succeeded\n");
> > > +	return 0;
> > > +}
> > > +postenvironment_initcall(hgs_run_first_boot_setup);
> > 
> > Needs to be protected from running on other boards.
> 
> Guarded via the hgs_first_boot variable which is only set by
> hgs_run_first_boot() which in turn gets only called via
> hgs_common_boot().

Hm, you're right. Nevertheless please do not add multiple initcalls at
the same level in the same file.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |