On 2023-11-10 14:30, Daniel Stodden wrote: > > Hi. > > [Sorry, sending this twice, again, hoping this time it won't bounce, because html] > >> Nice catch, thanks! >> >> The solution looks good to me, though I might quibble slightly about >> style: I'm not sure why we need two helper functions (disable_dma_mrpc() >> and switchtec_exit_pci()) that are only called in one place. I'd >> probably just open code both of them. >> >> Other than that: >> >> Reviewed-by: Logan Gunthorpe <logang@xxxxxxxxxxxx> > > I'm fine with it either way. I added those mainly because of the enable_dma_mrpc/switctec_init_pci > counterparts were already there. And likewise single-use, as would be usual. > > So the subroutines would maintain symmetry between probe() and remove(). > > So.. just don't add the new remove()-relevant ones? (One alternative would be to send another change right after, > which goes after single-use pairs altogether.) Ah, I see. Just because it's single use doesn't necessarily mean it's wrong. I would say switchtec_init_pci() should not be inlined because it's rather long and keeps switchtec_pci_probe() a little more readable. enable_dma_mrpc() more questionable. I'm not sure why it was done that way. Perhaps something changed. In any case, I'm not all that concerned. If you want to leave it the way it is, I wouldn't mind. Thanks, Logan
