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

[Jérôme Pouiller]

On Tuesday 5 October 2021 07:56:53 CEST Kalle Valo wrote:
> Jérôme Pouiller <jerome.pouiller@xxxxxxxxxx> 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@xxxxxxxxxx> writes:
> >>
> >> > From: Jérôme Pouiller <jerome.pouiller@xxxxxxxxxx>
> >> >
> >> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@xxxxxxxxxx>
> >>
> >> [...]
> >>
> >> > +/* 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.

It was just to be able to review this function separately. Nevermind.


-- 
Jérôme Pouiller