Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Tue, 10 Oct 2023 15:59:27 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Wolfram Sang <wsa@kernel.org>,
        Lakshmi Yadlapati <lakshmiy@us.ibm.com>,
        sumit.semwal@linaro.org, christian.koenig@amd.com,
        jdelvare@suse.com, joel@jms.id.au, andrew@aj.id.au,
        eajames@linux.ibm.com, ninad@linux.ibm.com,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-media@vger.kernel.org, linux-hwmon@vger.kernel.org,
        dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v1 0/2] [PATCH] hwmon: (pmbus/max31785) Add minimum delay
 between bus accesses
Message-ID: <125cac30-b83d-4530-885b-5008fc3045af@roeck-us.net>
References: <20231009211420.3454026-1-lakshmiy@us.ibm.com>
 <ZSUaDIfWmEn5edrE@shikoro>
 <1284830f-025e-4e25-8ed0-50a6cc00d223@roeck-us.net>
 <ZSWevlHzu6kVcGWA@shikoro>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZSWevlHzu6kVcGWA@shikoro>
Precedence: bulk

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@aj.id.au/
> > > 
> > > 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.

> > What other summary are you looking for ?
> 
> What the actual problem is with these devices. The cover letter only
> mentions "issues with small command turn-around times". More details
> would be nice. Is it between transfers? Or even between messages within
> one transfer? Has it been tried to lower the bus frequency? Stuff like
> this.
> 

I don't know about this device, but in general the problem is that the
devices need some delay between some or all transfers or otherwise react
badly in one way or another. The problem is not always the same.
Lower bus frequencies don't help, at least not for the devices where
I have seen to problem myself. The issue is not bus speed, but time between
transfers. Typically the underlying problem is that there is some
microcontroller on the affected chips, and the microcode is less than
perfect. For example, the microcode may not poll its I2C interface
while it is busy writing into the chip's internal EEPROM or while it is
updating some internal parameters as result of a previous I2C transfer.

> > On a side note, if anyone plans to implement the prepare_device() callback,
> > please make sure that it covers all requirements. It would be unfortunate
> > if such a callback was implemented if that would still require per-driver
> > code (besides the callback).
> 
> Is there a list of that somewhere? Or does it mean going through all the
> drivers and see what they currently do?
> 

The latter. I never bothered trying to write up a list. Typically the behavior
is not documented and needs to be tweaked a couple of times, and it may be
different across chips supported by the same driver, or even across chip
revisions. Any list trying to keep track of the various details would
be difficult to maintain and notoriously be outdated.

Guenter