Hi Greg, Thank you again for the feedback. I will work on this and submit a new patch again. Thanks, Neeraj > > On Thu, Feb 23, 2023 at 01:57:58PM +0000, Neeraj sanjay kale wrote: > > Hi Greg, > > > > Thank you for your feedback. > > > > > > > > > + > > > > +static int init_baudrate = 115200; > > > > > > and neither will this, as you need to support multiple devices in > > > the system, your driver should never be only able to work with one > device. > > > Please make these device-specific things, not the same for the whole > driver. > > > > > > > I am using this init_baudrate as a module parameter static int > > init_baudrate = 115200; module_param(init_baudrate, int, 0444); > > MODULE_PARM_DESC(init_baudrate, "host baudrate after FW download: > > default=115200"); > > Ah, totally missed that. > > That is not ok, sorry, this is not the 1990's, we do not use module parameters > for drivers as that obviously does not work at all for when you have multiple > devices controlled by that driver. Please make this all dynamic and "just > work" properly for all devices. > > > We need this parameter configurable since different chip module vendors > using the same NXP chipset, configure these chips differently. > > Then you are pushing the configuration to userspace for someone else to put > on their boot command line? that's crazy, please never do that. > > > For example, module vendor A distributes his modules based on NXP > 88w8987 chips with a different configuration compared to module vendor B > (based on NXP 88w8987), and the init_baudrate is one of the important > distinctions between them. > > Then put that logic in DT where it belongs. > > > If we are able to keep this init_baudrate configurable, while compiling > btnxpuart.ko as module, we will be able to support such baudrate variations. > > Again, no, that's not how tty or serial devices work. > > thanks, > > greg k-h
