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
