On 19-03-21, 13:31, Jie Deng wrote: > > On 2021/3/19 11:54, Viresh Kumar wrote: > > On 18-03-21, 15:52, Arnd Bergmann wrote: > > > Allowing multiple virtio-i2c controllers in one system, and multiple i2c > > > devices attached to each controller is clearly something that has to work. > > Good. > > > > > I don't actually see a limitation though. Viresh, what is the problem > > > you see for having multiple controllers? > > I thought this would be a problem in that case as we are using the global > > virtio_adapter here. > > > > + vi->adap = &virtio_adapter; > > + i2c_set_adapdata(vi->adap, vi); > > > > Multiple calls to probe() will end up updating the same pointer inside adap. > > > > + vi->adap->dev.parent = &vdev->dev; > > > > Same here, overwrite. > > > > + /* Setup ACPI node for controlled devices which will be probed through ACPI */ > > + ACPI_COMPANION_SET(&vi->adap->dev, ACPI_COMPANION(pdev)); > > + vi->adap->timeout = HZ / 10; > > > > These may be fine, but still not ideal I believe. > > > > + ret = i2c_add_adapter(vi->adap); > > i > > This should be a problem as well, we must be adding this to some sort of list, > > doing some RPM stuff, etc ? > > > > Jie, the solution is to allocate memory for adap at runtime in probe and remove > > the virtio_adapter structure completely. > > > If you want to support that. Then I think we don't need to change the > following at all. > > > + .algo = &virtio_algorithm, > > + > > + return ret; > > + > > + vi->adap = virtio_adapter; > This is strange, why are you allocating memory for adapter twice ? > Once for virtio_adapter and once for vi->adap ? Either fill the fields > directly for v->adap here and remove virtio_adapter or make vi->adap a > pointer. Yes, your previous version was partly okay but you don't need the virtio_algorithm structure to be allocated. There are only 4 fields you are updating here, just fill them directly in vi->adap. (FWIW, I also suggested the same when I said "Either fill the fields directly for v->adap here and remove virtio_adapter". ) See how drivers/i2c/busses/i2c-versatile.c and most of the other drivers have done it. -- viresh
