On Mon, Jan 17, 2022 at 07:02:20PM +1300, Paulo Miguel Almeida wrote: > Max bit rate is dependent on which modulation is used. Previous > validation routine only took into consideration min bit rate which can > lead a misconfiguration of the rf69 chip causing the packets not to be > sent/read. > > This patch enhances that input check in set_bit_rate to account for > modulation values and their respective max bit rate > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@xxxxxxxxx> > --- > Meta-comments: > > In the patchset v1 I kept bit_rate argument's original type as I thought that > changing it to accomodate values as high as 300kbps couldn't be part of this > patchset and therefore it should be a separate patchset. > > Given that kernel test bot compilation/test process 'complained' about the > argument's type, I decided to send the v2 patch that addresses the data type > problem while I work on the patch that will change bit_rate type across > tx_cfg and rx_cfg as this will require a bit more work. > > Please let me know if anyone dislikes the approach and wants me to deal with it > in a different way. I always think that correct static checker warnings should be preserved instead of papered over and silenced. Someone went to a lot of work to create that warning and then we're disabling it for being correct? It doesn't make sense. At the same time, this is a GCC warning so it will break the build. Instead of applying this patch, I wish you would just make a note of it in the drivers/staging/pi433/TODO file. "Change (struct pi433_tx_cfg)->bit_rate to be a u32 so that we can support bit rates up to 300kbps per the spec." But you're right that it's complicated to fix this because it's part of the UAPI. I think that the UAPI for pi433 is kind of garbage. No one like custom ioctls. It would be better to use sysfs. Each pi433_tx_cfg struct member would become it's own file obviously. We could pick sane default values where ever possible instead of leaving it all up to the user. So my idea is that instead of modifying the custom ioctl then we can just add a new sysfs file to set the bit_rate and all the other stuff. Eventually we will delete the ioctl after all the users have updated to the new userspace. (I say this with the complete confidence of someone who has never used pi433 and is not really sure what it is). regards, dan carpenter