[RFC PATCH 5/5] ipq40xx: add target for Google WiFi (Gale)

[mail at adrianschmutzler.de]

> I'll spin up a new revision sooner or later, but I'm hoping I'll get some opinion
> on the RFC portion (cover letter / first ~3 patches) before doing that.

Yes, just wait for some comments on the actual content. :-)

> > I'd like a bit more detailed flashing instructions here. What you have right
> now only works if the reader knows what to do anyway.
> 
> Well, it's not exactly trivial, but I guess not that difficult, relative to the
> average device OpenWRT supports. As luck would have it, somebody has
> already documented it:
> 
> https://github.com/marcosscriven/galeforce#how-to-apply-an-image
> 
> Would it be best to summarize, link, or both?

Both. I'm a fan of putting information directly into the commit message, as that is the safest way of preserving it when links change, content changes etc.

The linked resources seem to be too much content for just pasting, though, so a helpful summary seems to be appropriate here.

Despite, also consider adding the full information to our Wiki's device page.

> 
> > > +++ b/target/linux/ipq40xx/files/arch/arm/boot/dts/qcom-ipq4019-gale-
> v2.
> > > +++ dts
> > > @@ -0,0 +1,402 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2016, 2018 The Linux Foundation. All rights reserved.
> > > + * Copyright (c) 2016 Google, Inc
> > > + */
> > > +
> > > +#include "qcom-ipq4019.dtsi"
> > > +#include <dt-bindings/input/input.h> #include
> > > +<dt-bindings/gpio/gpio.h>
> > > +
> > > +&tlmm {
> >
> > I'd have expected the root node first.
> 
> I've learned muscle memory to put things like pinctrl first, because they're
> often referenced by other nodes (possibly including in the root node) via
> phandle, and at least in the past, DTC would not support out-of-order
> references for phandles.
> 
> But that doesn't apply here (no phandles from the root section), so that
> doesn't apply. Will change.
> 
> > > +/ {
> > > +     model = "Google IPQ4019/Gale";
> >
> > This should be generally consistent with what you choose for
> DEVICE_MODEL. See my others comments there below.
> >
> > > +     compatible = "google,gale-v2", "qcom,ipq4019";
> ...
> > > --- a/target/linux/ipq40xx/image/Makefile
> > > +++ b/target/linux/ipq40xx/image/Makefile
> > > @@ -218,6 +218,20 @@ define Device/avm_fritzbox-4040  endef
> > > TARGET_DEVICES += avm_fritzbox-4040
> > >
> > > +define Device/google_gale-v2
> >
> > Alphabetic sorting.
> >
> > > +     DEVICE_VENDOR := Google
> > > +     DEVICE_MODEL := WiFi (Gale)
> >
> > This uses yet another name compared to device definition/compatible and
> model in DTS.
> > Please be more consistent. Despite, if the device node (and thus the
> > image) has a v2 in it, please also add
> >
> > DEVICE_VARIANT := v2
> >
> > here.
> 
> Thanks for the scrutiny! I do have some questions here: is the
> VENDOR/MODEL supposed to match closer to a marketing-friendly/user-
> friendly name, or a developer/low-level name?

I typically prefer something that the user can read on the device, as everything else will add to the confusion.

> Or just some balance of both? Because there's several constraints
> here:
> * The bootloader recognizes compatible="google,gale-v2" -- I don't believe I
> can reliably drop the "v2" there, but I suppose that doesn't require the file
> names, etc., to include it
> * There really is no v1 publicly-available; as noted in the commit message, I
> believe that was pre-release hardware, and the revisions just stuck around
> through development
> * the "v2" here does *not* mean second generation, as in
> https://en.wikipedia.org/wiki/Google_Nest_Wifi#Second_generation

I don't think we have to care too much about the v1 here; however, if there is a different "second generation" with the same name, then we should include a "1st-gen" in the name somewhere, maybe even instead of the v2 if there is no v3 to be expected for the "1st-gen".

The Wiki article is not entirely precise here, so is the gale-v2 the "first generation" in the nest article or is there actually a 1st and 2nd generation of that Nest devices and gale something even different?

> * "WiFi" doesn't really make for a good MODEL on its own, although it's OK
> when paired with the VENDOR. But I still preferred sticking the codename
> (Gale) around, since that's the unambiguous way hackers can recognize the
> model.
> 
> What do you think? Should I try to keep the keywords "google", "wifi", and
> "gale" in all of the config, image, and DTS name? And I'll avoid the "v2"
> labeling (and DEVICE_VARIANT) outside of "compatible", because I think that
> would be misleading.

If you call this gale, the question is how you proceed with the nest series then. Will you switch to marketing-based "nest" then, or will you use the internal name though the user will read something different on the device?

Unfortunately, this stuff is never easy to decide. Personally, I always lean towards what's printed on the thing.

Best

Adrian

> 
> Anyway, I'll try to figure out a better balance of the above on the next
> version.
> 
> Thanks,
> Brian
> 
> > > +     SOC := qcom-ipq4019
> > > +     DEVICE_DTS := qcom-ipq4019-gale-v2
> >
> > This line can be dropped, it will be calculated from SOC and device node
> name automatically.
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openpgp-digital-signature.asc
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.openwrt.org/pipermail/openwrt-devel/attachments/20200722/0028c884/attachment-0001.sig>