Date:   Tue, 31 Jan 2023 11:52:41 +0100
From:   Guru Mehar Rachaputi <gurumeharrachaputi@gmail.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: pi433: modify bit_rate from u16 to u32
Message-ID: <Y9jy+f5JpD1NrbZn@combine-ThinkPad-S1-Yoga>
References: <Y9h42l/8EcPqn63x@combine-ThinkPad-S1-Yoga>
 <Y9ilj812USHrSXe+@kroah.com>
 <Y9jrOKMGl7ITKxhP@combine-ThinkPad-S1-Yoga>
 <Y9jwcdYcFQ9f8+mR@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y9jwcdYcFQ9f8+mR@kroah.com>
Precedence: bulk

On Tue, Jan 31, 2023 at 11:41:53AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 31, 2023 at 11:19:36AM +0100, Guru Mehar Rachaputi wrote:
> > On Tue, Jan 31, 2023 at 06:22:23AM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Jan 31, 2023 at 03:11:38AM +0100, Guru Mehar Rachaputi wrote:
> > > > (struct pi433_tx_cfg)->bit_rate is modified from u16 to u32 to
> > > > support bit rates up to 300kbps per the spec
> > > 
> > > What spec?
> > > 
> > > And how can changing the size of a variable that crosses the user/kernel
> > > boundry like this change the bit rate max?
> > >
> > Honestly, I followed the TODO file suggestion.
> 
> Do you have this hardware to test with?
>
Unfortunately, No.

> > > > Signed-off-by: Guru Mehar Rachaputi <gurumeharrachaputi@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;
> > > 
> > > And didn't you just break existing userspace code?  If not, how?  If so,
> > > how did you test this?
> > >
> > My apologies, I did not study code. While testing, the probe function of
> > pi433 driver didn't appear in the lsmod operation. I suspected my
> > testing was wrong.
> 
> You have to test the existing applications that talk to the device to
> ensure that this works properly.  This change just breaks the
> user/kernel api and doesn't actually change anything to work different
> than that :(
>
I understand. Since I do not have hardware I couldn't test it. I think I
will have to choose my patches wisely.

I would like to know, if it is mentioned in the TODO, why is it
so?

Thanks for the explaination and appreciate taking time for
helping. This was my first patch and I am already learning.

> thanks,
> 
> greg k-h

-- 
Thanks & Regards,
Guru