| Message ID | 20230103064612.404401-2-frank@crawford.emu.id.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (it87) Set second Super-IO chip in configuration mode | expand |
On Tue, Jan 03, 2023 at 05:46:11PM +1100, Frank Crawford wrote: > Allow for superio_enter to be called outside mux, in particular for "outside mux" is really a bad wording. I had to look into the code to understand what it means. "without requesting the muxed memory region", maybe. Guenter > initialisation of the second chipset, which must be entered, but never > exited. The second chipset is not "entered", it must enter configuration mode (or be put into configuration mode). The name of the function does not reflect the associated functionality. Please rephrase. Thanks, Guenter > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > --- > drivers/hwmon/it87.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > index 9997f76b1f4a..4ebce2c661d7 100644 > --- a/drivers/hwmon/it87.c > +++ b/drivers/hwmon/it87.c > @@ -87,6 +87,14 @@ static struct platform_device *it87_pdev[2]; > #define DEVID 0x20 /* Register: Device ID */ > #define DEVREV 0x22 /* Register: Device Revision */ > > +static inline void __superio_enter(int ioreg) > +{ > + outb(0x87, ioreg); > + outb(0x01, ioreg); > + outb(0x55, ioreg); > + outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg); > +} > + > static inline int superio_inb(int ioreg, int reg) > { > outb(reg, ioreg); > @@ -124,10 +132,7 @@ static inline int superio_enter(int ioreg) > if (!request_muxed_region(ioreg, 2, DRVNAME)) > return -EBUSY; > > - outb(0x87, ioreg); > - outb(0x01, ioreg); > - outb(0x55, ioreg); > - outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg); > + __superio_enter(ioreg); > return 0; > } > > -- > 2.38.1 >
On Tue, 2023-01-03 at 10:37 -0800, Guenter Roeck wrote: > On Tue, Jan 03, 2023 at 05:46:11PM +1100, Frank Crawford wrote: > > Allow for superio_enter to be called outside mux, in particular for > > "outside mux" is really a bad wording. I had to look into the code > to understand what it means. "without requesting the muxed memory > region", maybe. > > Guenter Will update with better wording, such as you suggest. > > > initialisation of the second chipset, which must be entered, but > > never > > exited. > > The second chipset is not "entered", it must enter configuration > mode (or be put into configuration mode). The name of the function > does not reflect the associated functionality. > > Please rephrase. Will do. Do you believe the function should be renamed as well? > > Thanks, > Guenter Regards Frank > > > > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > > --- > > drivers/hwmon/it87.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > > index 9997f76b1f4a..4ebce2c661d7 100644 > > --- a/drivers/hwmon/it87.c > > +++ b/drivers/hwmon/it87.c > > @@ -87,6 +87,14 @@ static struct platform_device *it87_pdev[2]; > > #define DEVID 0x20 /* Register: Device ID */ > > #define DEVREV 0x22 /* Register: Device Revision */ > > > > +static inline void __superio_enter(int ioreg) > > +{ > > + outb(0x87, ioreg); > > + outb(0x01, ioreg); > > + outb(0x55, ioreg); > > + outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg); > > +} > > + > > static inline int superio_inb(int ioreg, int reg) > > { > > outb(reg, ioreg); > > @@ -124,10 +132,7 @@ static inline int superio_enter(int ioreg) > > if (!request_muxed_region(ioreg, 2, DRVNAME)) > > return -EBUSY; > > > > - outb(0x87, ioreg); > > - outb(0x01, ioreg); > > - outb(0x55, ioreg); > > - outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg); > > + __superio_enter(ioreg); > > return 0; > > } > > > > -- > > 2.38.1 > >
On Wed, Jan 04, 2023 at 11:37:28AM +1100, Frank Crawford wrote: > On Tue, 2023-01-03 at 10:37 -0800, Guenter Roeck wrote: > > On Tue, Jan 03, 2023 at 05:46:11PM +1100, Frank Crawford wrote: > > > Allow for superio_enter to be called outside mux, in particular for > > > > "outside mux" is really a bad wording. I had to look into the code > > to understand what it means. "without requesting the muxed memory > > region", maybe. > > > > Guenter > > Will update with better wording, such as you suggest. > > > > > initialisation of the second chipset, which must be entered, but > > > never > > > exited. > > > > The second chipset is not "entered", it must enter configuration > > mode (or be put into configuration mode). The name of the function > > does not reflect the associated functionality. > > > > Please rephrase. > > Will do. > > Do you believe the function should be renamed as well? No. "enter" in the function name is short for "enter configuration mode". That is good enough for a function name. Description and documentation need to be more elaborate, though. Guenter > > > > Thanks, > > Guenter > > Regards > Frank > > > > > > > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > > > --- > > > drivers/hwmon/it87.c | 13 +++++++++---- > > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > > > index 9997f76b1f4a..4ebce2c661d7 100644 > > > --- a/drivers/hwmon/it87.c > > > +++ b/drivers/hwmon/it87.c > > > @@ -87,6 +87,14 @@ static struct platform_device *it87_pdev[2]; > > > #define DEVID 0x20 /* Register: Device ID */ > > > #define DEVREV 0x22 /* Register: Device Revision */ > > > > > > +static inline void __superio_enter(int ioreg) > > > +{ > > > + outb(0x87, ioreg); > > > + outb(0x01, ioreg); > > > + outb(0x55, ioreg); > > > + outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg); > > > +} > > > + > > > static inline int superio_inb(int ioreg, int reg) > > > { > > > outb(reg, ioreg); > > > @@ -124,10 +132,7 @@ static inline int superio_enter(int ioreg) > > > if (!request_muxed_region(ioreg, 2, DRVNAME)) > > > return -EBUSY; > > > > > > - outb(0x87, ioreg); > > > - outb(0x01, ioreg); > > > - outb(0x55, ioreg); > > > - outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg); > > > + __superio_enter(ioreg); > > > return 0; > > > } > > > > > > -- > > > 2.38.1 > > >
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c index 9997f76b1f4a..4ebce2c661d7 100644 --- a/drivers/hwmon/it87.c +++ b/drivers/hwmon/it87.c @@ -87,6 +87,14 @@ static struct platform_device *it87_pdev[2]; #define DEVID 0x20 /* Register: Device ID */ #define DEVREV 0x22 /* Register: Device Revision */ +static inline void __superio_enter(int ioreg) +{ + outb(0x87, ioreg); + outb(0x01, ioreg); + outb(0x55, ioreg); + outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg); +} + static inline int superio_inb(int ioreg, int reg) { outb(reg, ioreg); @@ -124,10 +132,7 @@ static inline int superio_enter(int ioreg) if (!request_muxed_region(ioreg, 2, DRVNAME)) return -EBUSY; - outb(0x87, ioreg); - outb(0x01, ioreg); - outb(0x55, ioreg); - outb(ioreg == REG_4E ? 0xaa : 0x55, ioreg); + __superio_enter(ioreg); return 0; }
Allow for superio_enter to be called outside mux, in particular for initialisation of the second chipset, which must be entered, but never exited. Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> --- drivers/hwmon/it87.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)