Re: [PATCH] brcmfmac: add debugfs entry for reading firmware capabilities

[Arend van Spriel <arend.vanspriel@broadcom.com>]

On 5/11/2018 12:15 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> This allows reading all capabilities as reported by a firmware. They are
> printed using native (raw) names, just like developers like it the most.
> It's how firmware reports support for various features, e.g. supported
> modes, supported standards, power saving details, max BSS-es.
>
> Access to all that info is useful for trying new firmwares, comparing
> them and debugging features AKA bugs.

What are you implying :-p ? Some comments below.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 36 ++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> index 876731c57bf5..782121cb9399 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> @@ -165,6 +165,41 @@ static void brcmf_feat_firmware_capabilities(struct brcmf_if *ifp)
>   	}
>   }
>
> +/**
> + * brcmf_cap_read() - expose firmware capabilities to debugfs.

Please stick to naming convention brcmf_<module>_foo(), ie. 
brcmf_feat_cap_read().

> + *
> + * @seq: sequence for debugfs entry.
> + * @data: raw data pointer.
> + */
> +static int brcmf_cap_read(struct seq_file *seq, void *data)
> +{
> +	struct brcmf_bus *bus_if = dev_get_drvdata(seq->private);
> +	struct brcmf_if *ifp = brcmf_get_ifp(bus_if->drvr, 0);
> +	char caps[MAX_CAPS_BUFFER_SIZE + 1] = { };
> +	char *tmp;
> +	int err;
> +
> +	err = brcmf_fil_iovar_data_get(ifp, "cap", caps, sizeof(caps));
> +	if (err) {
> +		brcmf_err("could not get firmware cap (%d)\n", err);
> +		return err;
> +	}
> +
> +	/* Put every capability in a new line */
> +	for (tmp = caps; *tmp; tmp++) {
> +		if (*tmp == ' ')
> +			*tmp = '\n';
> +	}
> +
> +	/* Usually there is a space at the end of capabilities string */
> +	seq_printf(seq, "%s", caps);
> +	/* So make sure we don't print two line breaks */
> +	if (tmp > caps && *(tmp - 1) != '\n')
> +		seq_printf(seq, "\n");
> +
> +	return 0;
> +}
> +
>   void brcmf_feat_attach(struct brcmf_pub *drvr)
>   {
>   	struct brcmf_if *ifp = brcmf_get_ifp(drvr, 0);
> @@ -233,6 +268,7 @@ void brcmf_feat_attach(struct brcmf_pub *drvr)
>   void brcmf_feat_debugfs_create(struct brcmf_pub *drvr)
>   {
>   	brcmf_debugfs_add_entry(drvr, "features", brcmf_feat_debugfs_read);
> +	brcmf_debugfs_add_entry(drvr, "cap", brcmf_cap_read);

Prefer to name the debugfs entry "fwcap".