RE: [PATCH v1 0/2] [PATCH] hwmon: (pmbus/max31785) Add minimum delay between bus accesses

[Lakshmi Yadlapati]

Joel's suggestion to explore the previously proposed generic solution makes sense, especially considering the recent changes for PMBus devices that have added delays. Thank you for your review and input. 
I will modify the code to incorporate the necessary delay directly in the max31785 driver.

Thanks,
Lakshmi

On 10/10/23, 10:54 PM, "Andrew Jeffery" <andrew@xxxxxxxx <mailto:andrew@xxxxxxxx>> wrote:


On Wed, 11 Oct 2023, at 09:29, Guenter Roeck wrote:
> On Tue, Oct 10, 2023 at 08:58:06PM +0200, Wolfram Sang wrote:
>> Hi Guenter,
>> 
>> > > > Reference to Andrew's previous proposal:
>> > > > https://lore.kernel.org/all/20200914122811.3295678-1-andrew@xxxxxxxx/ <https://lore.kernel.org/all/20200914122811.3295678-1-andrew@xxxxxxxx/> 
>> > > 
>> > > I do totally agree with Guenter's comment[1], though. This just affects
>> > > a few drivers and this patch is way too intrusive for the I2C core. The
>> > > later suggested prepare_device() callback[2] sounds better to me. I
>> > > still haven't fully understood why this all cannot be handled in the
>> > > driver's probe. Could someone give me a small summary about that?
>> > > 
>> > 
>> > Lots of PMBus devices have the same problem, we have always handled
>> > it in PMBus drivers by implementing local wait code, and your references
>> > point that out.
>> 
>> I am confused now. Reading your reply:
>> 
>> "I am not sure if an implementation in the i2c core is desirable. It
>> looks quite invasive to me, and it won't solve the problem for all
>> devices since it isn't always a simple "wait <n> microseconds between
>> accesses". For example, some devices may require a wait after a write
>> but not after a read, or a wait only after certain commands (such as
>> commands writing to an EEPROM)."
>> 
>> I get the impression you don't prefer to have a generic mechanism in the
>> I2C core. This I share. Your response now sounds like you do support
>> that idea now?
>> 
>
> I didn't (want to) say that. I am perfectly happy with driver specific
> code, and I would personally still very much prefer it. I only wanted to
> suggest that _if_ a generic solution is implemented, it should cover all
> existing use cases and not just this one. But, really, I'd rather leave
> that alone and not risk introducing regressions to existing drivers.


We had an out-of-tree patch for the max31785[1] that I wrote a little 
after the initial discussion on this generic throttling and possibly 
somewhat before the other drivers had their delays added. Recently Joel 
pointed out the addition of the delays in the other drivers and I 
raised the idea that we could get rid of that out-of-tree patch by 
doing the same. Guenter's point about the work-arounds being very 
particular to the device is good justification for not trying to 
fix drivers that we can't immediately test - not that the series did 
that, but arguably if we're shooting for the generic solution then it 
should.


So I agree with Guenter that we probably want to do down the path of 
adding the delays directly into the max31785 driver and not trying to 
over-generalise.


Lakshmi: Apologies for misleading you in some way there - unfortunately 
I can't go back to understand exactly what I suggested as I've changed 
jobs in the mean time.


Andrew


[1]: https://github.com/openbmc/linux/commit/44e1397368a70ffe9cdad1f9212ffdef8c16b9be <https://github.com/openbmc/linux/commit/44e1397368a70ffe9cdad1f9212ffdef8c16b9be>