Date:   Sat, 11 Mar 2023 16:17:52 +0300
From:   Dan Carpenter <error27@gmail.com>
To:     Sumitra Sharma <sumitraartsy@gmail.com>
Cc:     gregkh@linuxfoundation.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org, outreachy@lists.linux.dev
Subject: Re: [PATCH] Staging: pi433: Change bit_rate type from u16 to u32
Message-ID: <de7df101-6cbf-4e69-afcb-b80204bc1d5f@kili.mountain>
References: <b9d62635-b968-4dfd-aa10-e34e38a114a5@kili.mountain>
 <20230311130023.GB14247@ubuntu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230311130023.GB14247@ubuntu>
Precedence: bulk

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