Re: [PATCH v3 1/7] hwmon: (it87) Allow disabling exiting of configuration mode

[Guenter Roeck <linux@roeck-us.net>]

On 1/30/23 14:19, Frank Crawford wrote:
> On Mon, 2023-01-30 at 10:42 -0800, Guenter Roeck wrote:
>> On 1/29/23 12:56, Guenter Roeck wrote:
>>> On Sat, Jan 28, 2023 at 05:03:02PM +1100, Frank Crawford wrote:
>>>> Disabling configuration mode on some chips can result in system
>>>> hang-ups and access failures to the Super-IO chip at the
>>>> second SIO address. Never exit configuration mode on these
>>>> chips to avoid the problem.
>>>>
>>>> This patch should be applied in conjunction with a previous one
>>>> to
>>>> initialise the second chip for certain mother boards.
>>>>
>>>> Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
>>>> ---
>>>>
>>>> v3:
>>>>    * Correct possible uninitialised pointer issue.
>>>>
>>>> v2:
>>>>    * Convert to use feature flag and related macros rather than a
>>>> separate
>>>>      field, as suggested in review.
>>>>    * Reverse sense of flag in superio_exit to simplify feature
>>>> macro.
>>>>
>>>> ---
>>>>    drivers/hwmon/it87.c | 26 ++++++++++++++++++--------
>>>>    1 file changed, 18 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
>>>> index a8a6a0ffee82..923a9563be92 100644
>>>> --- a/drivers/hwmon/it87.c
>>>> +++ b/drivers/hwmon/it87.c
>>>> @@ -128,10 +128,12 @@ static inline int superio_enter(int ioreg)
>>>>          return 0;
>>>>    }
>>>>    
>>>> -static inline void superio_exit(int ioreg)
>>>> +static inline void superio_exit(int ioreg, bool noexit)
>>>>    {
>>>> -       outb(0x02, ioreg);
>>>> -       outb(0x02, ioreg + 1);
>>>> +       if (!noexit) {
>>>> +               outb(0x02, ioreg);
>>>> +               outb(0x02, ioreg + 1);
>>>> +       }
>>>>          release_region(ioreg, 2);
>>>>    }
>>>>    
>>>> @@ -300,6 +302,13 @@ struct it87_devices {
>>>>    #define FEAT_PWM_FREQ2                BIT(16) /* Separate pwm
>>>> freq 2 */
>>>>    #define FEAT_SIX_TEMP         BIT(17) /* Up to 6 temp sensors
>>>> */
>>>>    #define FEAT_VIN3_5V          BIT(18) /* VIN3 connected to +5V
>>>> */
>>>> +/*
>>>> + * Disabling configuration mode on some chips can result in
>>>> system
>>>> + * hang-ups and access failures to the Super-IO chip at the
>>>> + * second SIO address. Never exit configuration mode on these
>>>> + * chips to avoid the problem.
>>>> + */
>>>> +#define FEAT_CONF_NOEXIT       BIT(28) /* Chip should not exit
>>>> conf mode */
>>>
>>> Feature bits are supposed to be numbered sequentially, not randomly
>>> assigned. Please use bit 19.
>>>
>>
>> Given that the commit window is coming up, and since I don't have
>> other issues with your series, I am inclined to apply it with the
>> bit changed. Please let me know in the next day or two if this is
>> acceptable for you. If not, the series will have to wait until v6.4.
>>
> 
> Yes, I'm okay with that.
> 
> I was planning to submit an update with the only changes being both of
> your changes in your previous comment, but the second one can wait for
> a later group if you are happy with that.
> 

The other change request was a minor optimization. Just leave it alone.

Series applied to hwmon-next, with bit changed from 28 to 19.

Guenter