Re: [PATCH v9 05/24] wfx: add main.c/main.h
Jérôme Pouiller writes: >> > There is also the patch 01/24 about the SDIO IDs. >> > >> > I think the v10 could contain only 3 patches: >> > >> > 1. mmc: sdio: add SDIO IDs for Silabs WF200 chip >> > 2. dt-bindings: introduce silabs,wfx.yaml >> > 3. [all the patches 3 to 24 squashed] >> > >> > Would it be right for you? >> >> TBH I don't see the point of patch 3 at this moment, we have had so many >> iterations with the full driver already. If people want to look at the >> driver, they can check it from the staging tree. So in the next round I >> recommend submitting only patches 1 and 2 and focus on getting all the >> pending patches to staging tree. > > Ok. > >> And the chances are that a big patch like that would be filtered by the >> mailing lists anyway. > > I believe that with -M, the patch would be very small. Ah, you mean patch 3 would be about moving wfx from drivers/staging to drivers/net/wireless? Yeah, with -M that would be a good idea. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 05/24] wfx: add main.c/main.h
On Thursday 10 February 2022 17:25:05 CET Kalle Valo wrote:
> Jérôme Pouiller writes:
>
> > On Thursday 10 February 2022 15:51:03 CET Kalle Valo wrote:
> >> Jérôme Pouiller writes:
> >> > On Thursday 10 February 2022 15:20:56 CET Kalle Valo wrote:
> >> >> Jérôme Pouiller writes:
> >> >>
> >> >> > Kalle, is this function what you expected? If it is right for you, I
> >> >> > am
> >> >> > going to send it to the staging tree.
> >> >>
> >> >> Looks better, but I don't get why '{' and '}' are still needed. Ah, does
> >> >> the firmware require to have them?
> >> >
> >> > Indeed. If '{' and '}' are not present, I guarantee the firmware will
> >> > return
> >> > an error (or assert). However, I am more confident in the driver than in
> >> > the
> >> > firmware to report errors to the user.
> >>
> >> Agreed.
> >>
> >> > If there is no other comment, I am going to:
> >> > - submit this change to the staging tree
> >>
> >> Good, it's important that you get all your changes to the staging tree
> >> before the next merge window.
> >>
> >> > - publish the tool that generate this new format
> >> > - submit the PDS files referenced in bus_{sdio,spi}.c to linux-firmware
> >> > - send the v10 of this PR
> >>
> >> I'm not sure if there's a need to send a full patchset anymore? We are
> >> so close now anyway and the full driver is available from the staging
> >> tree, at least that's what I will use from now on when reviewing wfx.
> >>
> >> What about the Device Tree bindings? That needs to be acked by the DT
> >> maintainers, so that's good to submit as a separate patch for review.
> >
> > There is also the patch 01/24 about the SDIO IDs.
> >
> > I think the v10 could contain only 3 patches:
> >
> > 1. mmc: sdio: add SDIO IDs for Silabs WF200 chip
> > 2. dt-bindings: introduce silabs,wfx.yaml
> > 3. [all the patches 3 to 24 squashed]
> >
> > Would it be right for you?
>
> TBH I don't see the point of patch 3 at this moment, we have had so many
> iterations with the full driver already. If people want to look at the
> driver, they can check it from the staging tree. So in the next round I
> recommend submitting only patches 1 and 2 and focus on getting all the
> pending patches to staging tree.
Ok.
> And the chances are that a big patch like that would be filtered by the
> mailing lists anyway.
I believe that with -M, the patch would be very small.
--
Jérôme Pouiller
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 05/24] wfx: add main.c/main.h
Jérôme Pouiller writes:
> On Thursday 10 February 2022 15:51:03 CET Kalle Valo wrote:
>> Jérôme Pouiller writes:
>> > On Thursday 10 February 2022 15:20:56 CET Kalle Valo wrote:
>> >> Jérôme Pouiller writes:
>> >>
>> >> > Kalle, is this function what you expected? If it is right for you, I am
>> >> > going to send it to the staging tree.
>> >>
>> >> Looks better, but I don't get why '{' and '}' are still needed. Ah, does
>> >> the firmware require to have them?
>> >
>> > Indeed. If '{' and '}' are not present, I guarantee the firmware will
>> > return
>> > an error (or assert). However, I am more confident in the driver than in
>> > the
>> > firmware to report errors to the user.
>>
>> Agreed.
>>
>> > If there is no other comment, I am going to:
>> > - submit this change to the staging tree
>>
>> Good, it's important that you get all your changes to the staging tree
>> before the next merge window.
>>
>> > - publish the tool that generate this new format
>> > - submit the PDS files referenced in bus_{sdio,spi}.c to linux-firmware
>> > - send the v10 of this PR
>>
>> I'm not sure if there's a need to send a full patchset anymore? We are
>> so close now anyway and the full driver is available from the staging
>> tree, at least that's what I will use from now on when reviewing wfx.
>>
>> What about the Device Tree bindings? That needs to be acked by the DT
>> maintainers, so that's good to submit as a separate patch for review.
>
> There is also the patch 01/24 about the SDIO IDs.
>
> I think the v10 could contain only 3 patches:
>
> 1. mmc: sdio: add SDIO IDs for Silabs WF200 chip
> 2. dt-bindings: introduce silabs,wfx.yaml
> 3. [all the patches 3 to 24 squashed]
>
> Would it be right for you?
TBH I don't see the point of patch 3 at this moment, we have had so many
iterations with the full driver already. If people want to look at the
driver, they can check it from the staging tree. So in the next round I
recommend submitting only patches 1 and 2 and focus on getting all the
pending patches to staging tree.
And the chances are that a big patch like that would be filtered by the
mailing lists anyway.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 05/24] wfx: add main.c/main.h
On Thursday 10 February 2022 15:51:03 CET Kalle Valo wrote:
> Jérôme Pouiller writes:
> > On Thursday 10 February 2022 15:20:56 CET Kalle Valo wrote:
> >> Jérôme Pouiller writes:
> >>
> >> > Kalle, is this function what you expected? If it is right for you, I am
> >> > going to send it to the staging tree.
> >>
> >> Looks better, but I don't get why '{' and '}' are still needed. Ah, does
> >> the firmware require to have them?
> >
> > Indeed. If '{' and '}' are not present, I guarantee the firmware will return
> > an error (or assert). However, I am more confident in the driver than in the
> > firmware to report errors to the user.
>
> Agreed.
>
> > If there is no other comment, I am going to:
> > - submit this change to the staging tree
>
> Good, it's important that you get all your changes to the staging tree
> before the next merge window.
>
> > - publish the tool that generate this new format
> > - submit the PDS files referenced in bus_{sdio,spi}.c to linux-firmware
> > - send the v10 of this PR
>
> I'm not sure if there's a need to send a full patchset anymore? We are
> so close now anyway and the full driver is available from the staging
> tree, at least that's what I will use from now on when reviewing wfx.
>
> What about the Device Tree bindings? That needs to be acked by the DT
> maintainers, so that's good to submit as a separate patch for review.
There is also the patch 01/24 about the SDIO IDs.
I think the v10 could contain only 3 patches:
1. mmc: sdio: add SDIO IDs for Silabs WF200 chip
2. dt-bindings: introduce silabs,wfx.yaml
3. [all the patches 3 to 24 squashed]
Would it be right for you?
--
Jérôme Pouiller
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 05/24] wfx: add main.c/main.h
Jérôme Pouiller writes:
> On Thursday 10 February 2022 15:20:56 CET Kalle Valo wrote:
>>
>> Jérôme Pouiller writes:
>>
>> > Kalle, is this function what you expected? If it is right for you, I am
>> > going to send it to the staging tree.
>>
>> Looks better, but I don't get why '{' and '}' are still needed. Ah, does
>> the firmware require to have them?
>
> Indeed. If '{' and '}' are not present, I guarantee the firmware will return
> an error (or assert). However, I am more confident in the driver than in the
> firmware to report errors to the user.
Agreed.
> If there is no other comment, I am going to:
> - submit this change to the staging tree
Good, it's important that you get all your changes to the staging tree
before the next merge window.
> - publish the tool that generate this new format
> - submit the PDS files referenced in bus_{sdio,spi}.c to linux-firmware
> - send the v10 of this PR
I'm not sure if there's a need to send a full patchset anymore? We are
so close now anyway and the full driver is available from the staging
tree, at least that's what I will use from now on when reviewing wfx.
What about the Device Tree bindings? That needs to be acked by the DT
maintainers, so that's good to submit as a separate patch for review.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 05/24] wfx: add main.c/main.h
On Thursday 10 February 2022 15:20:56 CET Kalle Valo wrote:
>
> Jérôme Pouiller writes:
>
> > Hi Kalle,
> >
> > On Tuesday 11 January 2022 18:14:05 CET Jerome Pouiller wrote:
> >> From: Jérôme Pouiller
> >>
> >> Signed-off-by: Jérôme Pouiller
> >> ---
> >> drivers/net/wireless/silabs/wfx/main.c | 485 +
> >> drivers/net/wireless/silabs/wfx/main.h | 42 +++
> >> 2 files changed, 527 insertions(+)
> >> create mode 100644 drivers/net/wireless/silabs/wfx/main.c
> >> create mode 100644 drivers/net/wireless/silabs/wfx/main.h
> >>
> > [...]
> >> +/* 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
> >> + *
> >> + * The PDS file is an array of Time-Length-Value structs.
> >> + */
> >> + int wfx_send_pds(struct wfx_dev *wdev, u8 *buf, size_t len)
> >> +{
> >> +int ret, chunk_type, chunk_len, chunk_num = 0;
> >> +
> >> +if (*buf == '{') {
> >> +dev_err(wdev->dev, "PDS: malformed file (legacy format?)\n");
> >> +return -EINVAL;
> >> +}
> >> +while (len > 0) {
> >> +chunk_type = get_unaligned_le16(buf + 0);
> >> +chunk_len = get_unaligned_le16(buf + 2);
> >> +if (chunk_len > len) {
> >> +dev_err(wdev->dev, "PDS:%d: corrupted file\n",
> >> chunk_num);
> >> +return -EINVAL;
> >> +}
> >> +if (chunk_type != WFX_PDS_TLV_TYPE) {
> >> +dev_info(wdev->dev, "PDS:%d: skip unknown data\n",
> >> chunk_num);
> >> +goto next;
> >> +}
> >> +if (chunk_len > WFX_PDS_MAX_CHUNK_SIZE)
> >> + dev_warn(wdev->dev, "PDS:%d: unexpectly large chunk\n",
> >> chunk_num);
> >> +if (buf[4] != '{' || buf[chunk_len - 1] != '}')
> >> + dev_warn(wdev->dev, "PDS:%d: unexpected content\n", chunk_num);
> >> +
> >> +ret = wfx_hif_configuration(wdev, buf + 4, chunk_len - 4);
> >> +if (ret > 0) {
> >> + dev_err(wdev->dev, "PDS:%d: invalid data (unsupported
> >> options?)\n",
> >> +chunk_num);
> >> +return -EINVAL;
> >> +}
> >> +if (ret == -ETIMEDOUT) {
> >> + dev_err(wdev->dev, "PDS:%d: chip didn't reply (corrupted
> >> file?)\n",
> >> +chunk_num);
> >> +return ret;
> >> +}
> >> +if (ret) {
> >> + dev_err(wdev->dev, "PDS:%d: chip returned an unknown error\n",
> >> chunk_num);
> >> +return -EIO;
> >> +}
> >> +next:
> >> +chunk_num++;
> >> +len -= chunk_len;
> >> +buf += chunk_len;
> >> +}
> >> +return 0;
> >> +}
> >
> > Kalle, is this function what you expected? If it is right for you, I am
> > going to send it to the staging tree.
>
> Looks better, but I don't get why '{' and '}' are still needed. Ah, does
> the firmware require to have them?
Indeed. If '{' and '}' are not present, I guarantee the firmware will return
an error (or assert). However, I am more confident in the driver than in the
firmware to report errors to the user.
If there is no other comment, I am going to:
- submit this change to the staging tree
- publish the tool that generate this new format
- submit the PDS files referenced in bus_{sdio,spi}.c to linux-firmware
- send the v10 of this PR
--
Jérôme Pouiller
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 05/24] wfx: add main.c/main.h
On Tuesday 11 January 2022 18:14:05 Jerome Pouiller wrote: > +/* 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 > + * Just a small cosmetic issue but URL cannot be automatically opened as it is missing slashes after https protocol. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 05/24] wfx: add main.c/main.h
Jérôme Pouiller writes:
> Hi Kalle,
>
> On Tuesday 11 January 2022 18:14:05 CET Jerome Pouiller wrote:
>> From: Jérôme Pouiller
>>
>> Signed-off-by: Jérôme Pouiller
>> ---
>> drivers/net/wireless/silabs/wfx/main.c | 485 +
>> drivers/net/wireless/silabs/wfx/main.h | 42 +++
>> 2 files changed, 527 insertions(+)
>> create mode 100644 drivers/net/wireless/silabs/wfx/main.c
>> create mode 100644 drivers/net/wireless/silabs/wfx/main.h
>>
> [...]
>> +/* 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
>> + *
>> + * The PDS file is an array of Time-Length-Value structs.
>> + */
>> + int wfx_send_pds(struct wfx_dev *wdev, u8 *buf, size_t len)
>> +{
>> +int ret, chunk_type, chunk_len, chunk_num = 0;
>> +
>> +if (*buf == '{') {
>> +dev_err(wdev->dev, "PDS: malformed file (legacy format?)\n");
>> +return -EINVAL;
>> +}
>> +while (len > 0) {
>> +chunk_type = get_unaligned_le16(buf + 0);
>> +chunk_len = get_unaligned_le16(buf + 2);
>> +if (chunk_len > len) {
>> +dev_err(wdev->dev, "PDS:%d: corrupted file\n",
>> chunk_num);
>> +return -EINVAL;
>> +}
>> +if (chunk_type != WFX_PDS_TLV_TYPE) {
>> +dev_info(wdev->dev, "PDS:%d: skip unknown data\n",
>> chunk_num);
>> +goto next;
>> +}
>> +if (chunk_len > WFX_PDS_MAX_CHUNK_SIZE)
>> + dev_warn(wdev->dev, "PDS:%d: unexpectly large chunk\n",
>> chunk_num);
>> +if (buf[4] != '{' || buf[chunk_len - 1] != '}')
>> + dev_warn(wdev->dev, "PDS:%d: unexpected content\n", chunk_num);
>> +
>> +ret = wfx_hif_configuration(wdev, buf + 4, chunk_len - 4);
>> +if (ret > 0) {
>> + dev_err(wdev->dev, "PDS:%d: invalid data (unsupported
>> options?)\n",
>> +chunk_num);
>> +return -EINVAL;
>> +}
>> +if (ret == -ETIMEDOUT) {
>> + dev_err(wdev->dev, "PDS:%d: chip didn't reply (corrupted
>> file?)\n",
>> +chunk_num);
>> +return ret;
>> +}
>> +if (ret) {
>> + dev_err(wdev->dev, "PDS:%d: chip returned an unknown error\n",
>> chunk_num);
>> +return -EIO;
>> +}
>> +next:
>> +chunk_num++;
>> +len -= chunk_len;
>> +buf += chunk_len;
>> +}
>> +return 0;
>> +}
>
> Kalle, is this function what you expected? If it is right for you, I am
> going to send it to the staging tree.
Looks better, but I don't get why '{' and '}' are still needed. Ah, does
the firmware require to have them?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 05/24] wfx: add main.c/main.h
Hi Kalle,
On Tuesday 11 January 2022 18:14:05 CET Jerome Pouiller wrote:
> From: Jérôme Pouiller
>
> Signed-off-by: Jérôme Pouiller
> ---
> drivers/net/wireless/silabs/wfx/main.c | 485 +
> drivers/net/wireless/silabs/wfx/main.h | 42 +++
> 2 files changed, 527 insertions(+)
> create mode 100644 drivers/net/wireless/silabs/wfx/main.c
> create mode 100644 drivers/net/wireless/silabs/wfx/main.h
>
[...]
> +/* 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
> + *
> + * The PDS file is an array of Time-Length-Value structs.
> + */
> + int wfx_send_pds(struct wfx_dev *wdev, u8 *buf, size_t len)
> +{
> + int ret, chunk_type, chunk_len, chunk_num = 0;
> +
> + if (*buf == '{') {
> + dev_err(wdev->dev, "PDS: malformed file (legacy format?)\n");
> + return -EINVAL;
> + }
> + while (len > 0) {
> + chunk_type = get_unaligned_le16(buf + 0);
> + chunk_len = get_unaligned_le16(buf + 2);
> + if (chunk_len > len) {
> + dev_err(wdev->dev, "PDS:%d: corrupted file\n",
> chunk_num);
> + return -EINVAL;
> + }
> + if (chunk_type != WFX_PDS_TLV_TYPE) {
> + dev_info(wdev->dev, "PDS:%d: skip unknown data\n",
> chunk_num);
> + goto next;
> + }
> + if (chunk_len > WFX_PDS_MAX_CHUNK_SIZE)
> + dev_warn(wdev->dev, "PDS:%d: unexpectly large chunk\n",
> chunk_num);
> + if (buf[4] != '{' || buf[chunk_len - 1] != '}')
> + dev_warn(wdev->dev, "PDS:%d: unexpected content\n",
> chunk_num);
> +
> + ret = wfx_hif_configuration(wdev, buf + 4, chunk_len - 4);
> + if (ret > 0) {
> + dev_err(wdev->dev, "PDS:%d: invalid data (unsupported
> options?)\n",
> + chunk_num);
> + return -EINVAL;
> + }
> + if (ret == -ETIMEDOUT) {
> + dev_err(wdev->dev, "PDS:%d: chip didn't reply
> (corrupted file?)\n",
> + chunk_num);
> + return ret;
> + }
> + if (ret) {
> + dev_err(wdev->dev, "PDS:%d: chip returned an unknown
> error\n", chunk_num);
> + return -EIO;
> + }
> +next:
> + chunk_num++;
> + len -= chunk_len;
> + buf += chunk_len;
> + }
> + return 0;
> +}
Kalle, is this function what you expected? If it is right for you, I am
going to send it to the staging tree.
--
Jérôme Pouiller
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
