On 08/25/2014 09:52 AM, Arnd Bergmann wrote:

On Sunday 24 August 2014 23:24:41 Hauke Mehrtens wrote:

quoted

This driver needs an nvram driver and fetches the sprom values from the
nvram and provides it to any other driver. The calibration data for the
wifi chip the mac address and some more board description data is
stores in the sprom.

This is based on a copy of arch/mips/bcm47xx/sprom.c and my plan is to
make the bcm47xx MIPS SoCs also use this driver some time later.

quoted

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 .../devicetree/bindings/misc/bcm47xx-sprom.txt     |  16 +

I'd prefer not to list the binding in a 'misc' category. Maybe we can
have a new category and move the misc/sram.txt into the same?

quoted

 drivers/misc/Kconfig                               |  14 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/bcm47xx-sprom.c                       | 690 +++++++++++++++++++++

On a similar note, putting the driver into drivers/misc seems
suboptimal: misc drivers should by definition be something that
is for some odd hardware with no external dependencies on it,
whereas your driver seems to be used by multiple other drivers.

Would it make sense to put it into drivers/bcma when that is the
only bus it is used on?

As Jonas already said this code should be used for the bcm53xx ARM code
and the bcm47xx MIPS code and it is needed for drivers/bcma/ and
drivers/ssb/ (ssb only for old mips devices). Do you have any better
idea than putting this to drivers/misc/ ? For the mips SoC we need the
code very early and will not use the driver interface but probably
directly call the function name.

quoted

 4 files changed, 721 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/bcm47xx-sprom.txt
 create mode 100644 drivers/misc/bcm47xx-sprom.c

diff --git a/Documentation/devicetree/bindings/misc/bcm47xx-sprom.txt b/Documentation/devicetree/bindings/misc/bcm47xx-sprom.txt
new file mode 100644
index 0000000..eed2a4a
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/bcm47xx-sprom.txt

@@ -0,0 +1,16 @@
+Broadcom bcm47xx/bcm53xx sprom converter
+
+This driver provbides an sprom based on a given nvram.
+
+Required properties:
+
+- compatible : brcm,bcm47xx-sprom

Please use a specific SoC here as the compatible string, not something
with 'x' in it as a wildcard.

I will change that.

quoted

+#define NVRAM_READ_VAL(type)						\
+static void nvram_read_ ## type (const struct bcm47xx_sprom_fill *fill,	\
+				 const char *postfix, const char *name, \
+				 type *val, type allset)		\
+{									\
+	char buf[100];							\
+	int err;							\
+	type var;							\
+									\
+	err = get_nvram_var(fill, postfix, name, buf, sizeof(buf));	\
+	if (err < 0)							\
+		return;							\
+	err = kstrto ## type(strim(buf), 0, &var);			\
+	if (err) {							\
+		pr_warn("can not parse nvram name %s%s%s with value %s got %i\n",	\
+			fill->prefix, name, postfix, buf, err);		\
+		return;							\
+	}								\
+	if (allset && var == allset)					\
+		return;							\
+	*val = var;							\
+}
+
+NVRAM_READ_VAL(u8)
+NVRAM_READ_VAL(s8)
+NVRAM_READ_VAL(u16)
+NVRAM_READ_VAL(u32)
+
+#undef NVRAM_READ_VAL

Hmm, multiline macros are ugly. How about using one function to read
into an s64 internally using kstrtoll, and simple inline wrappers around
that for the smaller types, doing the appropriate range check?

Yes that should work I will try it.

quoted

+static void bcm47xx_sprom_fill_r1234589(struct ssb_sprom *sprom,
+					const struct bcm47xx_sprom_fill *fill)
+{
+	nvram_read_u16(fill, NULL, "devid", &sprom->dev_id, 0);
+	nvram_read_u8(fill, NULL, "ledbh0", &sprom->gpio0, 0xff);
+	nvram_read_u8(fill, NULL, "ledbh1", &sprom->gpio1, 0xff);
+	nvram_read_u8(fill, NULL, "ledbh2", &sprom->gpio2, 0xff);
+	nvram_read_u8(fill, NULL, "ledbh3", &sprom->gpio3, 0xff);
+	nvram_read_u8(fill, NULL, "aa2g", &sprom->ant_available_bg, 0);
+	nvram_read_u8(fill, NULL, "aa5g", &sprom->ant_available_a, 0);
+	nvram_read_s8(fill, NULL, "ag0", &sprom->antenna_gain.a0, 0);
+	nvram_read_s8(fill, NULL, "ag1", &sprom->antenna_gain.a1, 0);
+	nvram_read_alpha2(fill, "ccode", sprom->alpha2);
+}

Rather than calling the same set of functions multiple times, can you
do this using a table driven approach for smaller code size and
improved readability?

I will try to change that.

quoted

+static int bcm47xx_sprom_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct ssb_sprom *sprom;
+	const __be32 *handle;
+	struct device_node *nvram_node;
+	struct platform_device *nvram_dev;
+	struct bcm47xx_sprom_fill fill;
+
+	/* Alloc */
+	sprom = devm_kzalloc(dev, sizeof(*sprom), GFP_KERNEL);
+	if (!sprom)
+		return -ENOMEM;
+
+	handle = of_get_property(np, "nvram", NULL);
+	if (!handle)
+		return -ENOMEM;
+
+	nvram_node = of_find_node_by_phandle(be32_to_cpup(handle));

You can avoid the explicit be32_to_cpup here if you use
of_property_read_u32 above.

Thanks for the hint.

Hauke