arnRe: [PATCH v2 1/5] ARM: rcm-k1879xb1: Add support for K1879XB1 SoC

On Tuesday 30 June 2015 18:15:03 Andrew Andrianov wrote:

Could this be a bit shorter, e.g. 'mach-rcm' or 'mach-k1879xb1'?

The directories contain very few files for new platforms these days,
so we just tend to lump everything together by manufacturer.

Generally speaking, please avoid comments that only contain the file
names. Also, 'board-dt' is not a particularly useful name if that is
used to handle all machines. Maybe call it the same as the platform.

+
+static void __iomem *k1879_sctl_base(void)
+{
+	return (void __iomem *)RCM_K1879_SCTL_VIRT_BASE;
+}

Please don't hardcode virtual addresses like this. Instead read it from
DT at boot time using of_iomap or similar.

+static void k1879_level_irq_i2c0_fixup(unsigned int irq, struct irq_desc *desc)
+{
+	writel(1, k1879_mif_base() + RCM_K1879_MIF_I2C_INT_STAT);
+	handle_level_irq(irq, desc);
+}
+
+static void k1879_level_irq_i2c1_fixup(unsigned int irq, struct irq_desc *desc)
+{
+	writel(1 << 0, k1879_sctl_base() + RCM_K1879_SCTL_INT_P_OUT);
+	handle_level_irq(irq, desc);
+}
+
+static void k1879_level_irq_i2c2_fixup(unsigned int irq, struct irq_desc *desc)
+{
+	writel(1 << 1, k1879_sctl_base() + RCM_K1879_SCTL_INT_P_OUT);
+	handle_level_irq(irq, desc);
+}
+
+static void k1879_level_irq_i2c3_fixup(unsigned int irq, struct irq_desc *desc)
+{
+	writel(1 << 2, k1879_sctl_base() + RCM_K1879_SCTL_INT_P_OUT);
+	handle_level_irq(irq, desc);
+}
+
+static void (*k1879_i2c_fixups[])(unsigned int irq, struct irq_desc *desc) = {
+	k1879_level_irq_i2c0_fixup,
+	k1879_level_irq_i2c1_fixup,
+	k1879_level_irq_i2c2_fixup,
+	k1879_level_irq_i2c3_fixup
+};

It looks to me like this could be implemented as a nested irq domain with
its own irqchip driver rather than a hack in platform code.

+static void __init k1879_dt_mach_init(void)
+{
+	struct device_node *np;
+
+	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
+	k1879_mif = ioremap(RCM_K1879_MIF_PHYS_BASE,
+			    RCM_K1879_MIF_SIZE);
+	BUG_ON(!k1879_mif);
+
+	np = of_find_compatible_node(NULL, NULL, "arm,sp805");
+	WARN_ON(!np);
+	if (np)
+		k1879_wdt = of_iomap(np, 0);

Please move this into the watchdog driver as a reboot handler.

+	/* Setup i2c interrupt fixups */
+	setup_i2c_fixups();
+
+	/* I2C0 (Internal, HDMI) needs some extra love */
+	do {
+		void __iomem *mif;
+
+		mif = k1879_mif_base();
+		writel(1, mif + RCM_K1879_MIF_I2C_INT_TYPE_ENA);
+		writel(1, mif + RCM_K1879_MIF_I2C_INT_TYPE);
+		writel(1, mif + RCM_K1879_MIF_I2C_INT_ENA);
+	} while (0);
+}

It would be nice if this could be moved to some driver as well, ideally
the one that already maps k1879_mif_base. Does this need to be
done really early at boot time?

;-)


	Arnd