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

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

[Sumitra Sharma]

Hi julia,

I forgot to Cc the maintainers of the code of the above patch. I have Cc
in this mail. Do let me know if there is a need of recreating the patch.

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

[Julia Lawall]

On Sat, 11 Mar 2023, Sumitra Sharma wrote:

> Hi julia,
>
> I forgot to Cc the maintainers of the code of the above patch. I have Cc
> in this mail. Do let me know if there is a need of recreating the patch.

You need to send the patch to all of the relevant people/mailing lists.

julia

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

[Sumitra Sharma]

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 <sumitraartsy@gmail.com>
---
 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

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

[Greg KH]

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 <sumitraartsy@gmail.com>
> ---
>  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

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

[Dan Carpenter]

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 <sumitraartsy@gmail.com>
> ---

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

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

regards,
dan carpenter

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

[Sumitra Sharma]

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." 

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

[Dan Carpenter]

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