Re: [PATCH 2/3] hwmon: (pmbus/ltc2978): Fix PMBus polling of MFR_COMMON definitions.

[Guenter Roeck]

On Tue, Jan 28, 2020 at 09:16:39PM +0000, Jones, Michael-A1 wrote:
> Guenter,
> 
> The decision to not poll PEND was based on some other non-driver code based on /dev/i2c that looked like this:
> 
> // Set to 0 for LTC3883 which does not support PEND
> #define USE_PEND        1
> 
> #define NOT_BUSY        1 << 6
> #define NOT_TRANS       1 << 4
> #if (USE_PEND)
> #define NOT_PENDING     1 << 5
> #else
> #define NOT_PENDING     0
> #endif
> 
> My recollection is that came from the datasheet, many years ago.
> 
> I talked to the designer, and if the above is correct, it has not been correct since 2012. The designer was not interested in researching artifacts that far back in history. So we know it has been in the part for at least 8 years.
> 
> There seems to be two choices:
> 
> A) Leave it as is
> B) Poll the PEND bit and possibly break compatibility on ancient hardware
> 
> Generally, unused status bits in this kind are high when reserved or not used. That is good for polling.
> 
> The shipping volume of LTC3888 has always been very low compared to other parts, so exposure is very small, certainly Cisco/Juniper type companies would not be effected.
> 
I assume you mean LTC3883, not LTC3888.

> I would feel ok with polling PEND on this part. Let me know your opinion.
> 
> On a related matter, bit 4 is asserted low when the output is changing value. Hwmon cannot cause this because it only performs telemetry.
> 
> A user application could change VOUT and cause this to happen. Telemetry would reflect the last measured value from a 100ms internal polling loop, which may be a before, after, or during value. I have always judged that checking this bit has no value, and it can be problematic. If the part is set to have a very long transition rate, like 5 seconds, this would hang the call for a long time. That seemed bad to me, and is why I did not poll this bit.
> 

Your call, really. My major concern was that bit 5 is no longer polled
on LTC3883. From the above, it looks like it actually _is_ the pending
bit (5) that isn't supported on LTC3883. With that in mind, I'll apply
the series as-is and add the Fixes: tag myself; no need to resend.

Thanks,
Guenter

> Mike
> 
> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Tuesday, January 28, 2020 12:13 PM
> To: Jones, Michael-A1 <Michael-A1.Jones@xxxxxxxxxx>
> Cc: linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; jdelvare@xxxxxxxx; robh+dt@xxxxxxxxxx; corbet@xxxxxxx
> Subject: Re: [PATCH 2/3] hwmon: (pmbus/ltc2978): Fix PMBus polling of MFR_COMMON definitions.
> 
> [External]
> 
> On Tue, Jan 28, 2020 at 10:59:59AM -0700, Mike Jones wrote:
> > Change 21537dc driver PMBus polling of MFR_COMMON from bits 5/4 to 
> > bits 6/5. This fixs a LTC297X family bug where polling always returns 
> > not busy even when the part is busy. This fixes a LTC388X and LTM467X 
> > bug where polling used PEND and NOT_IN_TRANS, and BUSY was not polled, 
> > which can lead to NACKing of commands. LTC388X and LTM467X modules now 
> > poll BUSY and PEND, increasing reliability by eliminating NACKing of 
> > commands.
> > 
> > Signed-off-by: Mike Jones <michael-a1.jones@xxxxxxxxxx>
> 
> Fixes: e04d1ce9bbb49 ("hwmon: (ltc2978) Add polling for chips requiring it")
> 
> > ---
> >  drivers/hwmon/pmbus/ltc2978.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pmbus/ltc2978.c 
> > b/drivers/hwmon/pmbus/ltc2978.c index f01f488..a91ed01 100644
> > --- a/drivers/hwmon/pmbus/ltc2978.c
> > +++ b/drivers/hwmon/pmbus/ltc2978.c
> > @@ -82,8 +82,8 @@ enum chips { ltc2974, ltc2975, ltc2977, ltc2978, 
> > ltc2980, ltc3880, ltc3882,
> >  
> >  #define LTC_POLL_TIMEOUT		100	/* in milli-seconds */
> >  
> > -#define LTC_NOT_BUSY			BIT(5)
> > -#define LTC_NOT_PENDING			BIT(4)
> > +#define LTC_NOT_BUSY			BIT(6)
> > +#define LTC_NOT_PENDING			BIT(5)
> >  
> 
> In ltc_wait_ready(), we have:
> 
> 	/*
>          * LTC3883 does not support LTC_NOT_PENDING, even though
>          * the datasheet claims that it does.
>          */
>         mask = LTC_NOT_BUSY;
>         if (data->id != ltc3883)
>                 mask |= LTC_NOT_PENDING;
> 
> The semantics of this code is now different: It means that on
> LTC3883 only bit 6 is checked; previously, it was bit 5. I agree that the above change makes sense, but it doesn't seem correct to drop the check for bit 5 on LTC3883. Maybe remove the if() above and always check for bit 5 and 6 ? Or should bit 4 be checked on parts other than LTC3883 ?
> 
> #define LTC_NOT_TRANSITIONING		BIT(4)
> ...
>         mask = LTC_NOT_BUSY | LTC_NOT_PENDING;
>         if (data->id != ltc3883)
>                 mask |= LTC_NOT_TRANSITIONING;
> 
> Thanks,
> Guenter