2023-03-11 12:23:58

by Sumitra Sharma

[permalink] [raw]
Subject: [PATCH] Staging: pi433: Change bit_rate type from u16 to u32

Change bit_rate type from u16 to u32 inside struct pi433_tx_cfg to
support bit rates up to 300kbps as per the spec.

Signed-off-by: Sumitra Sharma <[email protected]>
---
drivers/staging/pi433/pi433_if.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 25ee0b77a32c..1f8ffaf02d99 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -51,7 +51,7 @@ enum option_on_off {
#define PI433_TX_CFG_IOCTL_NR 0
struct pi433_tx_cfg {
__u32 frequency;
- __u16 bit_rate;
+ __u32 bit_rate;
__u32 dev_frequency;
enum modulation modulation;
enum mod_shaping mod_shaping;
--
2.25.1



2023-03-11 12:31:55

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] Staging: pi433: Change bit_rate type from u16 to u32

On Sat, Mar 11, 2023 at 04:23:46AM -0800, Sumitra Sharma wrote:
> Change bit_rate type from u16 to u32 inside struct pi433_tx_cfg to
> support bit rates up to 300kbps as per the spec.
>
> Signed-off-by: Sumitra Sharma <[email protected]>
> ---
> drivers/staging/pi433/pi433_if.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
> index 25ee0b77a32c..1f8ffaf02d99 100644
> --- a/drivers/staging/pi433/pi433_if.h
> +++ b/drivers/staging/pi433/pi433_if.h
> @@ -51,7 +51,7 @@ enum option_on_off {
> #define PI433_TX_CFG_IOCTL_NR 0
> struct pi433_tx_cfg {
> __u32 frequency;
> - __u16 bit_rate;
> + __u32 bit_rate;
> __u32 dev_frequency;
> enum modulation modulation;
> enum mod_shaping mod_shaping;
> --
> 2.25.1
>
>

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You sent a patch that has been sent multiple times in the past few
days, and is identical to ones that has been recently rejected.
Please always look at the mailing list traffic to determine if you are
duplicating other people's work.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

2023-03-11 12:39:39

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH] Staging: pi433: Change bit_rate type from u16 to u32

On Sat, Mar 11, 2023 at 04:23:46AM -0800, Sumitra Sharma wrote:
> Change bit_rate type from u16 to u32 inside struct pi433_tx_cfg to
> support bit rates up to 300kbps as per the spec.
>
> Signed-off-by: Sumitra Sharma <[email protected]>
> ---

That's not right. Read this for more information:

https://lore.kernel.org/all/?q=pi433_tx_cfg%20bit_rate

regards,
dan carpenter


2023-03-11 13:00:34

by Sumitra Sharma

[permalink] [raw]
Subject: Re: [PATCH] Staging: pi433: Change bit_rate type from u16 to u32

Hi Dan,

The description in TODO is difficult to understand. I would appreciae if
you could help me understand it more in a step-by-step simplified means.

"This configuration needs to be moved to sysfs instead of being done through
+ IOCTL. Goind forward, we need to port userspace tools to use sysfs instead
+ of IOCTL and then we would delete IOCTL."



2023-03-11 13:18:13

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH] Staging: pi433: Change bit_rate type from u16 to u32

On Sat, Mar 11, 2023 at 05:00:23AM -0800, Sumitra Sharma wrote:
> Hi Dan,
>
> The description in TODO is difficult to understand. I would appreciae if
> you could help me understand it more in a step-by-step simplified means.
>
> "This configuration needs to be moved to sysfs instead of being done through
> + IOCTL. Goind forward, we need to port userspace tools to use sysfs instead
> + of IOCTL and then we would delete IOCTL."
>

This driver comes with a user space program to control it. Your patch
breaks that program. Someone could find the program and change it to
use a u32 instead of a u16 but the old version would still be broken so
users would be confused.

Generally, in the kernel we try to avoid doing configuration through
ioctls these days but instead prefer to use sysfs.

Now the driver uses a pi433_tx_cfg struct which is the same for both
user space and kernel space. What we do is create a different one which
is private to the kernel. Then in pi433_ioctl() we copy the user space
struct and convert it to a kernel space struct (new format with a u32).
Change the kernel to use the new struct every where except the
pi433_ioctl.

Then add some new sysfs files so that the user can control the bitrate
without changing any of the rest of the struct. Update the user space
tool to use sysfs instead of the ioctl. Then after everyone upgrades to
the new version delete the ioctl.

regards,
dan carpenter