From: Boris Brezillon boris.brezillon@collabora.com Date: Mon, Apr 22, 2019 at 17:07:15
On Mon, 22 Apr 2019 15:54:33 +0000 Vitor Soares Vitor.Soares@synopsys.com wrote:
{ i3cbus->mode = mode;
- if (!i3cbus->scl_rate.i3c)
i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;- if (!i3cbus->scl_rate.i2c) {
if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW)i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;elsei3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
- switch (i3cbus->mode) {
- case I3C_BUS_MODE_PURE:
if (!i3cbus->scl_rate.i3c)i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;break;- case I3C_BUS_MODE_MIXED_FAST:
if (!i3cbus->scl_rate.i3c)i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;if (!i3cbus->scl_rate.i2c)i3cbus->scl_rate.i2c = i2c_scl_rate;break;- case I3C_BUS_MODE_MIXED_SLOW:
if (!i3cbus->scl_rate.i2c)i3cbus->scl_rate.i2c = i2c_scl_rate;i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;Maybe we should do
if (!i3cbus->scl_rate.i3c || i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;Just in case the I3C rate forced by the user is lower than the max I2C rate.
That was something that I considered but TBH it isn't a real use case.
Add a WARN_ON() to at least catch such inconsistencies. And maybe we should add a dev_warn() when the user-defined rates do not match the mode/LVR constraints. It's easy to do a mistake when writing a dts.
I think the WARN_ON() is too evasive on the screen and won't provide the information we want. The dev_warn() should work perfectly here.
if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) dev_warn(&i3cbus->cur_master->dev->dev, "%s: i3c-scl-hz lower then i2c-scl-hz\n", __func__);Using dev_warn() sounds good, though I don't think you need the __func__ here. Also, please print the i2c/i3c rates in the message, and align the second line on the open parens.
if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE || i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE) dev_warn(&i3cbus->cur_master->dev->dev, "%s: i2c-scl-hz not defined according MIPI I3C spec\n", __func__);Is that really a problem? Having an i2c rate that is less than FM speed sounds like a valid case to me.
I'm addressing the spec constrains.
In the practice it can be SM or even HS, it depends on the interface.
Maybe it make more sense to do this check on of_populate_i3c_bus(), what do you think?
No, we really want to have this check here, because we might support other HW description formats at some point (board-files, ACPI, ...).
Yes, you are right. I forgot that point.