| Message ID | 20220906202829.1921114-3-eajames@linux.ibm.com |
|---|---|
| State | Needs Review / ACK |
| Delegated to: | Peter Rosin |
| Headers | show |
| Series | i2c: core: Add mux root adapter operations | expand |
Hi! First off, I'm very sorry for being too busy and too unresponsive. 2022-09-06 at 22:28, Eddie James wrote: > Use the new mux root operations to lock the root adapter while waiting for > the reset to complete. I2C commands issued after the SI7020 is starting up > or after reset can potentially upset the startup sequence. Therefore, the > host needs to wait for the startup sequence to finish before issuing > further I2C commands. > > Signed-off-by: Eddie James <eajames@linux.ibm.com> > --- > drivers/iio/humidity/si7020.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c > index ab6537f136ba..76ca7863f35b 100644 > --- a/drivers/iio/humidity/si7020.c > +++ b/drivers/iio/humidity/si7020.c > @@ -106,6 +106,7 @@ static const struct iio_info si7020_info = { > static int si7020_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > + struct i2c_adapter *root; > struct iio_dev *indio_dev; > struct i2c_client **data; > int ret; > @@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client, > I2C_FUNC_SMBUS_READ_WORD_DATA)) > return -EOPNOTSUPP; > > + root = i2c_lock_select_bus(client->adapter); > + if (IS_ERR(root)) > + return PTR_ERR(root); > + > /* Reset device, loads default settings. */ > - ret = i2c_smbus_write_byte(client, SI7020CMD_RESET); > - if (ret < 0) > + ret = __i2c_smbus_xfer(root, client->addr, client->flags, > + I2C_SMBUS_WRITE, SI7020CMD_RESET, > + I2C_SMBUS_BYTE, NULL); I'd say that this is too ugly. We should not add stuff that basically hides the actual xfer from the mux like this. That is too much of a break in the abstraction. Looking back, expanding on the previous series [1] so that it installs the hook on the root adapter, handles smbus xfers and clears out the callback afterwards is much more sensible. No? Maybe the callback in that series should also include a reference to the xfer that has just been done, so that the hook can potentially discriminate and only do the delay for the key xfer. But maybe that's overkill? Cheers, Peter [1] https://lore.kernel.org/lkml/20220518204119.38943-1-eajames@linux.ibm.com/ > + if (ret < 0) { > + i2c_unlock_deselect_bus(client->adapter); > return ret; > + } > + > /* Wait the maximum power-up time after software reset. */ > msleep(15); > > + i2c_unlock_deselect_bus(client->adapter); > + > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > if (!indio_dev) > return -ENOMEM;
On 9/7/22 02:10, Peter Rosin wrote: > Hi! > > First off, I'm very sorry for being too busy and too unresponsive. > > 2022-09-06 at 22:28, Eddie James wrote: >> Use the new mux root operations to lock the root adapter while waiting for >> the reset to complete. I2C commands issued after the SI7020 is starting up >> or after reset can potentially upset the startup sequence. Therefore, the >> host needs to wait for the startup sequence to finish before issuing >> further I2C commands. >> >> Signed-off-by: Eddie James <eajames@linux.ibm.com> >> --- >> drivers/iio/humidity/si7020.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c >> index ab6537f136ba..76ca7863f35b 100644 >> --- a/drivers/iio/humidity/si7020.c >> +++ b/drivers/iio/humidity/si7020.c >> @@ -106,6 +106,7 @@ static const struct iio_info si7020_info = { >> static int si7020_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> + struct i2c_adapter *root; >> struct iio_dev *indio_dev; >> struct i2c_client **data; >> int ret; >> @@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client, >> I2C_FUNC_SMBUS_READ_WORD_DATA)) >> return -EOPNOTSUPP; >> >> + root = i2c_lock_select_bus(client->adapter); >> + if (IS_ERR(root)) >> + return PTR_ERR(root); >> + >> /* Reset device, loads default settings. */ >> - ret = i2c_smbus_write_byte(client, SI7020CMD_RESET); >> - if (ret < 0) >> + ret = __i2c_smbus_xfer(root, client->addr, client->flags, >> + I2C_SMBUS_WRITE, SI7020CMD_RESET, >> + I2C_SMBUS_BYTE, NULL); > I'd say that this is too ugly. We should not add stuff that basically > hides the actual xfer from the mux like this. That is too much of a > break in the abstraction. Hm, I guess I'm not sure I follow - I see several drivers that use the raw __i2c_smbus_xfer or __i2c_transfer, some without a lock in sight. If it's not acceptable to use the unlocked versions in some cases, why are they exported in the header file? > > Looking back, expanding on the previous series [1] so that it installs > the hook on the root adapter, handles smbus xfers and clears out the > callback afterwards is much more sensible. No? Maybe so, though adding the callback is a more intrusive change, in my opinion, since every transfer has to check if the pointer is null. Thanks for your feedback! Eddie > > Maybe the callback in that series should also include a reference to > the xfer that has just been done, so that the hook can potentially > discriminate and only do the delay for the key xfer. But maybe that's > overkill? > > Cheers, > Peter > > [1] https://lore.kernel.org/lkml/20220518204119.38943-1-eajames@linux.ibm.com/ > >> + if (ret < 0) { >> + i2c_unlock_deselect_bus(client->adapter); >> return ret; >> + } >> + >> /* Wait the maximum power-up time after software reset. */ >> msleep(15); >> >> + i2c_unlock_deselect_bus(client->adapter); >> + >> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); >> if (!indio_dev) >> return -ENOMEM;
Hi! 2022-09-07 at 15:53, Eddie James wrote: > > On 9/7/22 02:10, Peter Rosin wrote: >> Hi! >> >> First off, I'm very sorry for being too busy and too unresponsive. >> >> 2022-09-06 at 22:28, Eddie James wrote: >>> Use the new mux root operations to lock the root adapter while waiting for >>> the reset to complete. I2C commands issued after the SI7020 is starting up >>> or after reset can potentially upset the startup sequence. Therefore, the >>> host needs to wait for the startup sequence to finish before issuing >>> further I2C commands. >>> >>> Signed-off-by: Eddie James <eajames@linux.ibm.com> >>> --- >>> drivers/iio/humidity/si7020.c | 16 ++++++++++++++-- >>> 1 file changed, 14 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c >>> index ab6537f136ba..76ca7863f35b 100644 >>> --- a/drivers/iio/humidity/si7020.c >>> +++ b/drivers/iio/humidity/si7020.c >>> @@ -106,6 +106,7 @@ static const struct iio_info si7020_info = { >>> static int si7020_probe(struct i2c_client *client, >>> const struct i2c_device_id *id) >>> { >>> + struct i2c_adapter *root; >>> struct iio_dev *indio_dev; >>> struct i2c_client **data; >>> int ret; >>> @@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client, >>> I2C_FUNC_SMBUS_READ_WORD_DATA)) >>> return -EOPNOTSUPP; >>> + root = i2c_lock_select_bus(client->adapter); >>> + if (IS_ERR(root)) >>> + return PTR_ERR(root); >>> + >>> /* Reset device, loads default settings. */ >>> - ret = i2c_smbus_write_byte(client, SI7020CMD_RESET); >>> - if (ret < 0) >>> + ret = __i2c_smbus_xfer(root, client->addr, client->flags, >>> + I2C_SMBUS_WRITE, SI7020CMD_RESET, >>> + I2C_SMBUS_BYTE, NULL); >> I'd say that this is too ugly. We should not add stuff that basically >> hides the actual xfer from the mux like this. That is too much of a >> break in the abstraction. > > > Hm, I guess I'm not sure I follow - I see several drivers that use the raw __i2c_smbus_xfer or __i2c_transfer, some without a lock in sight. If it's not acceptable to use the unlocked versions in some cases, why are they exported in the header file? Doing unlocked xfers w/o manually doing the locking is a bug. If you are aware of code doing this, please point them out! Issuing an unlocked xfer on the same adapter that has been manually locked has been a thing since forever. But issuing that xfer on the root adapter "behind the back" of e.g. an address translator is simply not going to work at all if it never sees the xfer and thus never gets a chance to modify it. A mux might also in theory add quirks or adjust xfers for whatever reason, and that possibility will be made impossible by hiding the xfer from the mux. > > >> >> Looking back, expanding on the previous series [1] so that it installs >> the hook on the root adapter, handles smbus xfers and clears out the >> callback afterwards is much more sensible. No? > > > Maybe so, though adding the callback is a more intrusive change, in my opinion, since every transfer has to check if the pointer is null. The runtime cost will be negligible. The bigger cost is IMO the maintenance overhead. Cheers, Peter > > > Thanks for your feedback! > > Eddie > > > >> >> Maybe the callback in that series should also include a reference to >> the xfer that has just been done, so that the hook can potentially >> discriminate and only do the delay for the key xfer. But maybe that's >> overkill? >> >> Cheers, >> Peter >> >> [1] https://lore.kernel.org/lkml/20220518204119.38943-1-eajames@linux.ibm.com/ >> >>> + if (ret < 0) { >>> + i2c_unlock_deselect_bus(client->adapter); >>> return ret; >>> + } >>> + >>> /* Wait the maximum power-up time after software reset. */ >>> msleep(15); >>> + i2c_unlock_deselect_bus(client->adapter); >>> + >>> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); >>> if (!indio_dev) >>> return -ENOMEM;
On 9/7/22 09:17, Peter Rosin wrote: > Hi! > > 2022-09-07 at 15:53, Eddie James wrote: >> On 9/7/22 02:10, Peter Rosin wrote: >>> Hi! >>> >>> First off, I'm very sorry for being too busy and too unresponsive. >>> >>> 2022-09-06 at 22:28, Eddie James wrote: >>>> Use the new mux root operations to lock the root adapter while waiting for >>>> the reset to complete. I2C commands issued after the SI7020 is starting up >>>> or after reset can potentially upset the startup sequence. Therefore, the >>>> host needs to wait for the startup sequence to finish before issuing >>>> further I2C commands. >>>> >>>> Signed-off-by: Eddie James <eajames@linux.ibm.com> >>>> --- >>>> drivers/iio/humidity/si7020.c | 16 ++++++++++++++-- >>>> 1 file changed, 14 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c >>>> index ab6537f136ba..76ca7863f35b 100644 >>>> --- a/drivers/iio/humidity/si7020.c >>>> +++ b/drivers/iio/humidity/si7020.c >>>> @@ -106,6 +106,7 @@ static const struct iio_info si7020_info = { >>>> static int si7020_probe(struct i2c_client *client, >>>> const struct i2c_device_id *id) >>>> { >>>> + struct i2c_adapter *root; >>>> struct iio_dev *indio_dev; >>>> struct i2c_client **data; >>>> int ret; >>>> @@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client, >>>> I2C_FUNC_SMBUS_READ_WORD_DATA)) >>>> return -EOPNOTSUPP; >>>> + root = i2c_lock_select_bus(client->adapter); >>>> + if (IS_ERR(root)) >>>> + return PTR_ERR(root); >>>> + >>>> /* Reset device, loads default settings. */ >>>> - ret = i2c_smbus_write_byte(client, SI7020CMD_RESET); >>>> - if (ret < 0) >>>> + ret = __i2c_smbus_xfer(root, client->addr, client->flags, >>>> + I2C_SMBUS_WRITE, SI7020CMD_RESET, >>>> + I2C_SMBUS_BYTE, NULL); >>> I'd say that this is too ugly. We should not add stuff that basically >>> hides the actual xfer from the mux like this. That is too much of a >>> break in the abstraction. >> >> Hm, I guess I'm not sure I follow - I see several drivers that use the raw __i2c_smbus_xfer or __i2c_transfer, some without a lock in sight. If it's not acceptable to use the unlocked versions in some cases, why are they exported in the header file? > Doing unlocked xfers w/o manually doing the locking is a bug. If you are > aware of code doing this, please point them out! Ah, I just meant the lock wasn't right next to the transfer call, since I didn't fully understand you. I'm sure everything is being locked correctly. > > Issuing an unlocked xfer on the same adapter that has been manually locked > has been a thing since forever. But issuing that xfer on the root adapter > "behind the back" of e.g. an address translator is simply not going to work > at all if it never sees the xfer and thus never gets a chance to modify it. > A mux might also in theory add quirks or adjust xfers for whatever reason, > and that possibility will be made impossible by hiding the xfer from the > mux. OK, I see what you're saying. But that's what lock_select does, it locks the bus and goes through the mux channel selection function. With the way muxes work right now, they can't modify the xfer at all, they just deselect/select channels. So this code should work fine, unless I'm still misunderstanding something. I do see your point about a possible enhancement of the mux interface... I guess I'd argue that whoever wants to do that should have to worry about it! But if you're firm on this, I'll try the other way. I'll also mention we (IBM/OpenBMC) has been running this series on lab systems for a couple of months now, so it has some testing, though admittedly with just one configuration of device/mux/adapter. Thanks! Eddie >> >>> Looking back, expanding on the previous series [1] so that it installs >>> the hook on the root adapter, handles smbus xfers and clears out the >>> callback afterwards is much more sensible. No? >> >> Maybe so, though adding the callback is a more intrusive change, in my opinion, since every transfer has to check if the pointer is null. > The runtime cost will be negligible. The bigger cost is IMO the maintenance > overhead. > > Cheers, > Peter > >> >> Thanks for your feedback! >> >> Eddie >> >> >> >>> Maybe the callback in that series should also include a reference to >>> the xfer that has just been done, so that the hook can potentially >>> discriminate and only do the delay for the key xfer. But maybe that's >>> overkill? >>> >>> Cheers, >>> Peter >>> >>> [1] https://lore.kernel.org/lkml/20220518204119.38943-1-eajames@linux.ibm.com/ >>> >>>> + if (ret < 0) { >>>> + i2c_unlock_deselect_bus(client->adapter); >>>> return ret; >>>> + } >>>> + >>>> /* Wait the maximum power-up time after software reset. */ >>>> msleep(15); >>>> + i2c_unlock_deselect_bus(client->adapter); >>>> + >>>> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); >>>> if (!indio_dev) >>>> return -ENOMEM;
Hi! 2022-09-07 at 18:02, Eddie James wrote: > > On 9/7/22 09:17, Peter Rosin wrote: >> Hi! >> >> 2022-09-07 at 15:53, Eddie James wrote: >>> On 9/7/22 02:10, Peter Rosin wrote: >>>> Hi! >>>> >>>> First off, I'm very sorry for being too busy and too unresponsive. >>>> >>>> 2022-09-06 at 22:28, Eddie James wrote: >>>>> Use the new mux root operations to lock the root adapter while waiting for >>>>> the reset to complete. I2C commands issued after the SI7020 is starting up >>>>> or after reset can potentially upset the startup sequence. Therefore, the >>>>> host needs to wait for the startup sequence to finish before issuing >>>>> further I2C commands. >>>>> >>>>> Signed-off-by: Eddie James <eajames@linux.ibm.com> >>>>> --- >>>>> drivers/iio/humidity/si7020.c | 16 ++++++++++++++-- >>>>> 1 file changed, 14 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c >>>>> index ab6537f136ba..76ca7863f35b 100644 >>>>> --- a/drivers/iio/humidity/si7020.c >>>>> +++ b/drivers/iio/humidity/si7020.c >>>>> @@ -106,6 +106,7 @@ static const struct iio_info si7020_info = { >>>>> static int si7020_probe(struct i2c_client *client, >>>>> const struct i2c_device_id *id) >>>>> { >>>>> + struct i2c_adapter *root; >>>>> struct iio_dev *indio_dev; >>>>> struct i2c_client **data; >>>>> int ret; >>>>> @@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client, >>>>> I2C_FUNC_SMBUS_READ_WORD_DATA)) >>>>> return -EOPNOTSUPP; >>>>> + root = i2c_lock_select_bus(client->adapter); >>>>> + if (IS_ERR(root)) >>>>> + return PTR_ERR(root); >>>>> + >>>>> /* Reset device, loads default settings. */ >>>>> - ret = i2c_smbus_write_byte(client, SI7020CMD_RESET); >>>>> - if (ret < 0) >>>>> + ret = __i2c_smbus_xfer(root, client->addr, client->flags, >>>>> + I2C_SMBUS_WRITE, SI7020CMD_RESET, >>>>> + I2C_SMBUS_BYTE, NULL); >>>> I'd say that this is too ugly. We should not add stuff that basically >>>> hides the actual xfer from the mux like this. That is too much of a >>>> break in the abstraction. >>> >>> Hm, I guess I'm not sure I follow - I see several drivers that use the raw __i2c_smbus_xfer or __i2c_transfer, some without a lock in sight. If it's not acceptable to use the unlocked versions in some cases, why are they exported in the header file? >> Doing unlocked xfers w/o manually doing the locking is a bug. If you are >> aware of code doing this, please point them out! > > > Ah, I just meant the lock wasn't right next to the transfer call, since I didn't fully understand you. I'm sure everything is being locked correctly. > > >> >> Issuing an unlocked xfer on the same adapter that has been manually locked >> has been a thing since forever. But issuing that xfer on the root adapter >> "behind the back" of e.g. an address translator is simply not going to work >> at all if it never sees the xfer and thus never gets a chance to modify it. >> A mux might also in theory add quirks or adjust xfers for whatever reason, >> and that possibility will be made impossible by hiding the xfer from the >> mux. > > > OK, I see what you're saying. But that's what lock_select does, it locks the bus and goes through the mux channel selection function. With the way muxes work right now, they can't modify the xfer at all, they just deselect/select channels. So this code should work fine, unless I'm still misunderstanding something. > > I do see your point about a possible enhancement of the mux interface... I guess I'd argue that whoever wants to do that should have to worry about it! But if you're firm on this, I'll try the other way. I'll also mention we (IBM/OpenBMC) has been running this series on lab systems for a couple of months now, so it has some testing, though admittedly with just one configuration of device/mux/adapter. > Good, thanks, because I do not like how the approach in this series paints us into a corner, making e.g. the address translation series much more difficult: https://lore.kernel.org/linux-media/20190723203723.11730-3-luca@lucaceresoli.net/ Cheers, Peter > > Thanks! > > Eddie > > >>> >>>> Looking back, expanding on the previous series [1] so that it installs >>>> the hook on the root adapter, handles smbus xfers and clears out the >>>> callback afterwards is much more sensible. No? >>> >>> Maybe so, though adding the callback is a more intrusive change, in my opinion, since every transfer has to check if the pointer is null. >> The runtime cost will be negligible. The bigger cost is IMO the maintenance >> overhead. >> >> Cheers, >> Peter >> >>> >>> Thanks for your feedback! >>> >>> Eddie >>> >>> >>> >>>> Maybe the callback in that series should also include a reference to >>>> the xfer that has just been done, so that the hook can potentially >>>> discriminate and only do the delay for the key xfer. But maybe that's >>>> overkill? >>>> >>>> Cheers, >>>> Peter >>>> >>>> [1] https://lore.kernel.org/lkml/20220518204119.38943-1-eajames@linux.ibm.com/ >>>> >>>>> + if (ret < 0) { >>>>> + i2c_unlock_deselect_bus(client->adapter); >>>>> return ret; >>>>> + } >>>>> + >>>>> /* Wait the maximum power-up time after software reset. */ >>>>> msleep(15); >>>>> + i2c_unlock_deselect_bus(client->adapter); >>>>> + >>>>> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); >>>>> if (!indio_dev) >>>>> return -ENOMEM;
diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c index ab6537f136ba..76ca7863f35b 100644 --- a/drivers/iio/humidity/si7020.c +++ b/drivers/iio/humidity/si7020.c @@ -106,6 +106,7 @@ static const struct iio_info si7020_info = { static int si7020_probe(struct i2c_client *client, const struct i2c_device_id *id) { + struct i2c_adapter *root; struct iio_dev *indio_dev; struct i2c_client **data; int ret; @@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client, I2C_FUNC_SMBUS_READ_WORD_DATA)) return -EOPNOTSUPP; + root = i2c_lock_select_bus(client->adapter); + if (IS_ERR(root)) + return PTR_ERR(root); + /* Reset device, loads default settings. */ - ret = i2c_smbus_write_byte(client, SI7020CMD_RESET); - if (ret < 0) + ret = __i2c_smbus_xfer(root, client->addr, client->flags, + I2C_SMBUS_WRITE, SI7020CMD_RESET, + I2C_SMBUS_BYTE, NULL); + if (ret < 0) { + i2c_unlock_deselect_bus(client->adapter); return ret; + } + /* Wait the maximum power-up time after software reset. */ msleep(15); + i2c_unlock_deselect_bus(client->adapter); + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); if (!indio_dev) return -ENOMEM;
Use the new mux root operations to lock the root adapter while waiting for the reset to complete. I2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further I2C commands. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- drivers/iio/humidity/si7020.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)