Hi,
On 01-10-18 23:59, Wolfram Sang wrote:
IMHO the is_suspended flag should be set by the adapter driver from its
own suspend callback (which may be a normal, late or noirq suspend callback),
as this is the point past which the bus should no longer be used because
e.g. the suspend callbacks has disabled clocks and/or put the controller
in reset, etc.
That's what I meant with "not seeing the woods for the trees". Thanks
for clearing my view!
the i2c-controller device where necessary. But you're hooking into the
child-device of the actual "hardware" device which represents the i2c_adapter
bits.
That was intentional because this is the device the I2C core has access
to. Because the callbacks deal only with 'is_suspended' and not with HW,
I thought the logic device could be suitable. I didn't think of
additional relations only affecting its parent.
on the adapter-device. I believe that instead we should add
i2c_adapter_mark_suspended and i2c_adapter_mark_resumed helpers and let
the adapter-driver suspend/resume callbacks call these. This will ensure
that they get called at the right phase and at the exact moment that
the adapter is actually suspending.
Agreed. And these helpers are basically this:
+ i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+ adap->is_suspended = true;
+ i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
and this:
+ i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+ adap->is_suspended = false;
+ i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
Yes that is correct.
together with this still in the I2C core:
@@ -1867,6 +1904,9 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
if (WARN_ON(!msgs || num < 1))
return -EINVAL;
+ if (WARN_ON(adap->is_suspended))
+ return -ESHUTDOWN;
+
if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
return -EOPNOTSUPP;
Ack.
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 65b4eaed1d96..ee46295a67d4 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -692,6 +692,8 @@ struct i2c_adapter {
const struct i2c_adapter_quirks *quirks;
struct irq_domain *host_notify_domain;
+
+ int is_suspended:1;
Are we aligned?
I believe we are.
Regards,
Hans
