Re: [PATCH v7] i2c: virtio: add a virtio i2c frontend driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2021/3/12 14:10, Viresh Kumar wrote:
I saw your email about wrong version being sent, I already wrote some
reviews. Sending them anyway for FWIW :)

On 12-03-21, 21:33, Jie Deng wrote:
+struct virtio_i2c {
+	struct virtio_device *vdev;
+	struct completion completion;
+	struct i2c_adapter adap;
+	struct mutex lock;
As I said in the previous version (Yes, we were both waiting for
Wolfram to answer that), this lock shouldn't be required at all.

And since none of us have a use-case at hand where we will have a
problem without this lock, we should really skip it. We can always
come back and add it if we find an issue somewhere. Until then, better
to keep it simple.
The problem is you can't guarantee that adap->algo->master_xfer
is only called from i2c_transfer. Any function who holds the adapter can call adap->algo->master_xfer directly. I prefer to avoid potential issues rather than
find a issue then fix.

+
+static struct i2c_adapter virtio_adapter = {
+	.owner = THIS_MODULE,
+	.name = "Virtio I2C Adapter",
+	.class = I2C_CLASS_DEPRECATED,
What happened to this discussion ?

https://lore.kernel.org/lkml/20210305072903.wtw645rukmqr5hx5@vireshk-i7/

My understanding is that new driver sets this to warn users that the adapter doesn't support classes anymore.

I'm not sure if Wolfram can explain it more clear for you.


+	.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.

Will make vi->adap a pointer. Thanks.





[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux