Re: [PATCH v7 05/24] wfx: add main.c/main.h

From: Kalle Valo <kvalo@codeaurora.org>
To: "Jérôme Pouiller" <jerome.pouiller@silabs.com>
Cc: linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
	devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"David S . Miller" <davem@davemloft.net>,
	devicetree@vger.kernel.org, "Rob Herring" <robh+dt@kernel.org>,
	linux-mmc@vger.kernel.org, "Pali Rohár" <pali@kernel.org>,
	"Ulf Hansson" <ulf.hansson@linaro.org>
Subject: Re: [PATCH v7 05/24] wfx: add main.c/main.h
Date: Tue, 05 Oct 2021 08:56:53 +0300	[thread overview]
Message-ID: <8735pgggka.fsf@codeaurora.org> (raw)
In-Reply-To: <3660480.PpQ1LQVJpD@pc-42> (=?utf-8?B?IkrDqXLDtG1l?= Pouiller"'s message of "Fri, 01 Oct 2021 17:29:57 +0200")

Jérôme Pouiller <jerome.pouiller@silabs.com> writes:

> On Friday 1 October 2021 11:22:08 CEST Kalle Valo wrote:
>> CAUTION: This email originated from outside of the organization. Do
>> not click links or open attachments unless you recognize the sender
>> and know the content is safe.
>> 
>> 
>> Jerome Pouiller <Jerome.Pouiller@silabs.com> writes:
>> 
>> > From: Jérôme Pouiller <jerome.pouiller@silabs.com>
>> >
>> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
>> 
>> [...]
>> 
>> > +/* The device needs data about the antenna configuration. This information in
>> > + * provided by PDS (Platform Data Set, this is the wording used in WF200
>> > + * documentation) files. For hardware integrators, the full process to create
>> > + * PDS files is described here:
>> > + *   https:github.com/SiliconLabs/wfx-firmware/blob/master/PDS/README.md
>> > + *
>> > + * So this function aims to send PDS to the device. However, the PDS file is
>> > + * often bigger than Rx buffers of the chip, so it has to be sent in multiple
>> > + * parts.
>> > + *
>> > + * In add, the PDS data cannot be split anywhere. The PDS files contains tree
>> > + * structures. Braces are used to enter/leave a level of the tree (in a JSON
>> > + * fashion). PDS files can only been split between root nodes.
>> > + */
>> > +int wfx_send_pds(struct wfx_dev *wdev, u8 *buf, size_t len)
>> > +{
>> > +     int ret;
>> > +     int start, brace_level, i;
>> > +
>> > +     start = 0;
>> > +     brace_level = 0;
>> > +     if (buf[0] != '{') {
>> > + dev_err(wdev->dev, "valid PDS start with '{'. Did you forget to
>> > compress it?\n");
>> > +             return -EINVAL;
>> > +     }
>> > +     for (i = 1; i < len - 1; i++) {
>> > +             if (buf[i] == '{')
>> > +                     brace_level++;
>> > +             if (buf[i] == '}')
>> > +                     brace_level--;
>> > +             if (buf[i] == '}' && !brace_level) {
>> > +                     i++;
>> > +                     if (i - start + 1 > WFX_PDS_MAX_SIZE)
>> > +                             return -EFBIG;
>> > +                     buf[start] = '{';
>> > +                     buf[i] = 0;
>> > +                     dev_dbg(wdev->dev, "send PDS '%s}'\n", buf + start);
>> > +                     buf[i] = '}';
>> > +                     ret = hif_configuration(wdev, buf + start,
>> > +                                             i - start + 1);
>> > +                     if (ret > 0) {
>> > + dev_err(wdev->dev, "PDS bytes %d to %d: invalid data (unsupported
>> > options?)\n",
>> > +                                     start, i);
>> > +                             return -EINVAL;
>> > +                     }
>> > +                     if (ret == -ETIMEDOUT) {
>> > + dev_err(wdev->dev, "PDS bytes %d to %d: chip didn't reply (corrupted
>> > file?)\n",
>> > +                                     start, i);
>> > +                             return ret;
>> > +                     }
>> > +                     if (ret) {
>> > + dev_err(wdev->dev, "PDS bytes %d to %d: chip returned an unknown
>> > error\n",
>> > +                                     start, i);
>> > +                             return -EIO;
>> > +                     }
>> > +                     buf[i] = ',';
>> > +                     start = i;
>> > +             }
>> > +     }
>> > +     return 0;
>> > +}
>> 
>> I'm not really fond of having this kind of ASCII based parser in the
>> kernel. Do you have an example compressed file somewhere?
>> 
>> Does the device still work without these PDS files? I ask because my
>> suggestion is to remove this part altogether and revisit after the
>> initial driver is moved to drivers/net/wireless. A lot simpler to review
>> complex features separately.
>
> Do you want I remove this function from this patch and place it a new
> patch at the end of this series?

I don't understand, how that would help? The problem here is the file
format and that's what we should try to fix.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches