Hi!

On 2020-07-02 12:13, Lars Povlsen wrote:

Surplus unused variable?

+};
+
+/*
+ * Set the owner of the SPI interfaces
+ */
+static void mux_sparx5_set_owner(struct regmap *syscon,
+				 u8 owner, u8 owner2)
+{
+	u32 val, msk;
+
+	val = FIELD_PREP(SPARX5_IF_SI_OWNER, owner) |
+		FIELD_PREP(SPARX5_IF_SI2_OWNER, owner2);
+	msk = SPARX5_IF_SI_OWNER | SPARX5_IF_SI2_OWNER;
+	regmap_update_bits(syscon,
+			   SPARX5_CPU_SYSTEM_CTRL_GENERAL_CTRL,
+			   msk, val);
+}
+
+static void mux_sparx5_set_cs_owner(struct mux_sparx5 *mux_sparx5,
+				    u8 cs, u8 owner)
+{
+	u8 other = (owner == MSCC_IF_SI_OWNER_SIBM ?
+		    MSCC_IF_SI_OWNER_SIMC : MSCC_IF_SI_OWNER_SIBM);

Empty line missing here.

+	if (mux_sparx5->bus[cs])
+		/* SPI2 */
+		mux_sparx5_set_owner(mux_sparx5->syscon, other, owner);
+	else
+		/* SPI1 */
+		mux_sparx5_set_owner(mux_sparx5->syscon, owner, other);
+}

This smells like there are only two states for this mux control, and that
the whole point of this driver is to make the exact numbering selectable.
I don't see the point of that. To me, it looks like the pre-existing
mmio-mux should be able to work. Something like this? Untested of course,
and I might easily have misunderstood something...

mux: mux-controller {
	compatible = "mmio-mux"
	#mux-control-cells = <1>;

	/* SI_OWNER and SI2_OWNER in GENERAL_CTRL */
	mux-reg-masks = <0x88 0xf0>;
};


spi0: spi@600104000 {
	compatible = "microchip,sparx5-spi";
	spi@0 {
		compatible = "spi-mux";
		mux-controls = <&mux 0>;
		reg = <0>;
		/* SI_OWNER = SIMC, SI2_OWNER = SIBM  --->  mux value 9 */
		spi-flash@9 {
			compatible = "jedec,spi-nor";
			reg = <9>;
		};
	};
	spi@e {
		compatible = "spi-mux";
		mux-controls = <&mux 0>;
		reg = <14>;
		/* SI_OWNER = SIBM, SI2_OWNER = SIMC  --->  mux value 6 */
		spi-flash@6 {
			compatible = "spi-nand";
			reg = <6>;
		};
	};
};

+
+static int mux_sparx5_set(struct mux_control *mux, int state)
+{
+	struct mux_sparx5 *mux_sparx5 = mux_chip_priv(mux->chip);
+
+	mux_sparx5_set_cs_owner(mux_sparx5, state, MSCC_IF_SI_OWNER_SIMC);
+
+	return 0;
+}
+
+static const struct mux_control_ops mux_sparx5_ops = {
+	.set = mux_sparx5_set,
+};
+
+static const struct of_device_id mux_sparx5_dt_ids[] = {
+	{ .compatible = "microchip,sparx5-spi-mux", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mux_sparx5_dt_ids);
+
+static int mux_sparx5_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mux_chip *mux_chip;
+	struct mux_sparx5 *mux_sparx5;
+	struct device_node *nc;
+	const char *syscon_name = "microchip,sparx5-cpu-syscon";
+	int ret;
+
+	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_sparx5));
+	if (IS_ERR(mux_chip))
+		return PTR_ERR(mux_chip);
+
+	mux_sparx5 = mux_chip_priv(mux_chip);
+	mux_chip->ops = &mux_sparx5_ops;
+
+	mux_sparx5->syscon =
+		syscon_regmap_lookup_by_compatible(syscon_name);
+	if (IS_ERR(mux_sparx5->syscon)) {
+		dev_err(dev, "No syscon map %s\n", syscon_name);
+		return PTR_ERR(mux_sparx5->syscon);
+	}
+
+	/* Get bus interface mapping */
+	for_each_available_child_of_node(dev->of_node, nc) {
+		u32 cs, bus;
+
+		if (of_property_read_u32(nc, "reg", &cs) == 0 &&
+		    cs < SPARX5_MAX_CS &&
+		    of_property_read_u32(nc, "microchip,bus-interface",
+					 &bus) == 0)
+			mux_sparx5->bus[cs] = bus;

The above if is a mess. The kernel model is to handle the exceptional cases
first and break/goto/continue/return/whatever so that the interesting code
can happen at lower indentation level.

		if (of_property_read_u32(nc, "reg", &cs))
			continue;
		if (cs >= SPARX5_MAX_CS)
			continue;
		if (of_property_read_u32(nc, "microchip,bus-interface", &bus))
			continue;

		mux_sparc5->bus[cs] = bus;

Cheers,
Peter

+	}
+
+	mux_chip->mux->states = SPARX5_MAX_CS;
+
+	ret = devm_mux_chip_register(dev, mux_chip);
+	if (ret < 0)
+		return ret;
+
+	dev_info(dev, "%u-way mux-controller registered\n",
+		 mux_chip->mux->states);
+
+	return 0;
+}
+
+static struct platform_driver mux_sparx5_driver = {
+	.driver = {
+		.name = "sparx5-mux",
+		.of_match_table	= of_match_ptr(mux_sparx5_dt_ids),
+	},
+	.probe = mux_sparx5_probe,
+};
+module_platform_driver(mux_sparx5_driver);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel