Hi Vadim, On 2021-01-28 20:29, Vadim Pasternak wrote: > Hi Peter, > *snip* > I can merge 5 and 6 to one patch. > Would it be OK? Please no, that's backwards. > I would like to not drop one-byte register support. > We have InfiniBand modular system with leaves and spines devices on which > I need one-byte case. Code for those device was not upstream-ed yet. If you could have dropped the one-byte code, you'd have had a lot more freedom. If that support has to remain, then I have to assume that it should be preserved. So, you need to make it as clear as possible that the one-byte code still works as before. The current series makes this hard to see. I have to read patches out of order, which should not be needed. Look, when you add a new driver the most important thing to look out for is that the new code isn't adding bad interfaces or doing stupid things like double free etc. If the hardware works or not is not really important. If it doesn't work, nothing is lost because it didn't work before either. But when you extend a driver to support new hardware, you need to make it clear that the old hardware keeps working. In my experience, the best way to do that is to, if needed, first make the existing code more flexible but without adding any support for new hardware. I.e. first prepare the driver. Then, when the code is in such a shape that adding the support for the new hardware is easy, that final patch will be as easy to take for the maintainer as a completely new driver would be. The difficulty will always be in the preparatory patches that make the driver more flexible while preserving the old functionality. In this case, you have a couple of issus that you need to address before you can add support for your new hardware. One issue is that the one-byte code is never writing regval 0 to the register. It is instead using that value as a hint to not short out the *next* write of the regval. You seem to need your new two-byte code to be able to program regval 0 to the mux register. I would solve this by changing the 0 key to some other number, probably -1, so that the same mechanism could be used in both cases. For that to happen last_chan needs to be able to hold -1, so it needs to change type from u8. This change should be done in one patch, before adding the two-byte support. Then this issue becomes a non-issue when adding the two-byte support, because the driver already fits the need. One issue is that the current mechanism for selecting the actual regval to program is inflexible; you want to be able to not create all the child adapters, only the ones you actually need. I.e. the change from adap_id to chan_id. That change should be done in a preparatory patch, before adding two-byte support. One issue is that the driver hardcodes 8 maximum child adapters, and you need to make that more flexible. Do that change before you add two-byte support. That could possibly be in the same patch as the above adap_id to chan_id change, but it would not be wrong to do it as separate steps. As a reviewer I would probably prefer it in a separate patch. Etc, take small steps and do one logical change in each patch. That makes reviewing much more pleasant than reviewing everything mashed together. Cheers, Peter
