diff mbox series

[1/4] hwmon (max6639): Use regmap

Message ID 20240416171720.2875916-1-naresh.solanki@9elements.com (mailing list archive)
State Changes Requested
Headers show
Series [1/4] hwmon (max6639): Use regmap | expand

Commit Message

Naresh Solanki April 16, 2024, 5:17 p.m. UTC 
Add regmap support.

Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
---
 drivers/hwmon/Kconfig   |   1 +
 drivers/hwmon/max6639.c | 157 ++++++++++++++++++++--------------------
 2 files changed, 80 insertions(+), 78 deletions(-)


base-commit: db85dba9fee5fed54e2c37eed465f8fd243a92e8

Comments

Guenter Roeck April 16, 2024, 9:25 p.m. UTC | #1 
On Tue, Apr 16, 2024 at 10:47:14PM +0530, Naresh Solanki wrote:
> Add regmap support.
> 

Missing (and not really utilizing) the benefits of using regmap.

> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> ---
>  drivers/hwmon/Kconfig   |   1 +
>  drivers/hwmon/max6639.c | 157 ++++++++++++++++++++--------------------
>  2 files changed, 80 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index c89776d91795..257ec5360e35 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1223,6 +1223,7 @@ config SENSORS_MAX6621
>  config SENSORS_MAX6639
>  	tristate "Maxim MAX6639 sensor chip"
>  	depends on I2C
> +	select REGMAP_I2C
>  	help
>  	  If you say yes here you get support for the MAX6639
>  	  sensor chips.
> diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
> index aa7f21ab2395..1af93fc53cb5 100644
> --- a/drivers/hwmon/max6639.c
> +++ b/drivers/hwmon/max6639.c
> @@ -20,6 +20,7 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_data/max6639.h>
> +#include <linux/regmap.h>
>  
>  /* Addresses to scan */
>  static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
> @@ -57,6 +58,8 @@ static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
>  
>  #define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED	0x40
>  
> +#define MAX6639_NDEV				2
> +
>  static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
>  
>  #define FAN_FROM_REG(val, rpm_range)	((val) == 0 || (val) == 255 ? \
> @@ -67,7 +70,7 @@ static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
>   * Client data (each client gets its own)
>   */
>  struct max6639_data {
> -	struct i2c_client *client;
> +	struct regmap *regmap;
>  	struct mutex update_lock;
>  	bool valid;		/* true if following fields are valid */
>  	unsigned long last_updated;	/* In jiffies */
> @@ -95,9 +98,8 @@ struct max6639_data {
>  static struct max6639_data *max6639_update_device(struct device *dev)
>  {
>  	struct max6639_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>  	struct max6639_data *ret = data;
> -	int i;
> +	int i, err;
>  	int status_reg;
>  
>  	mutex_lock(&data->update_lock);
> @@ -105,39 +107,35 @@ static struct max6639_data *max6639_update_device(struct device *dev)

Conversions to regmap should drop all local caching and use regmap
for caching (where appropriate) instead.

Guenter
Naresh Solanki April 22, 2024, 10:36 a.m. UTC | #2 
Hi Guenter,

On Wed, 17 Apr 2024 at 02:55, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Apr 16, 2024 at 10:47:14PM +0530, Naresh Solanki wrote:
> > Add regmap support.
> >
>
> Missing (and not really utilizing) the benefits of using regmap.
This is just replacing with regmap support
>
> > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> > ---
> >  drivers/hwmon/Kconfig   |   1 +
> >  drivers/hwmon/max6639.c | 157 ++++++++++++++++++++--------------------
> >  2 files changed, 80 insertions(+), 78 deletions(-)
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index c89776d91795..257ec5360e35 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1223,6 +1223,7 @@ config SENSORS_MAX6621
> >  config SENSORS_MAX6639
> >       tristate "Maxim MAX6639 sensor chip"
> >       depends on I2C
> > +     select REGMAP_I2C
> >       help
> >         If you say yes here you get support for the MAX6639
> >         sensor chips.
> > diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
> > index aa7f21ab2395..1af93fc53cb5 100644
> > --- a/drivers/hwmon/max6639.c
> > +++ b/drivers/hwmon/max6639.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/err.h>
> >  #include <linux/mutex.h>
> >  #include <linux/platform_data/max6639.h>
> > +#include <linux/regmap.h>
> >
> >  /* Addresses to scan */
> >  static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
> > @@ -57,6 +58,8 @@ static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
> >
> >  #define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED 0x40
> >
> > +#define MAX6639_NDEV                         2
> > +
> >  static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
> >
> >  #define FAN_FROM_REG(val, rpm_range) ((val) == 0 || (val) == 255 ? \
> > @@ -67,7 +70,7 @@ static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
> >   * Client data (each client gets its own)
> >   */
> >  struct max6639_data {
> > -     struct i2c_client *client;
> > +     struct regmap *regmap;
> >       struct mutex update_lock;
> >       bool valid;             /* true if following fields are valid */
> >       unsigned long last_updated;     /* In jiffies */
> > @@ -95,9 +98,8 @@ struct max6639_data {
> >  static struct max6639_data *max6639_update_device(struct device *dev)
> >  {
> >       struct max6639_data *data = dev_get_drvdata(dev);
> > -     struct i2c_client *client = data->client;
> >       struct max6639_data *ret = data;
> > -     int i;
> > +     int i, err;
> >       int status_reg;
> >
> >       mutex_lock(&data->update_lock);
> > @@ -105,39 +107,35 @@ static struct max6639_data *max6639_update_device(struct device *dev)
>
> Conversions to regmap should drop all local caching and use regmap
> for caching (where appropriate) instead.
max6639_update_device deals with volatile registers & caching isn't
available for these.

Please let me know if there is specific optimization that you were looking for.

Regards,
Naresh
>
> Guenter
Guenter Roeck April 22, 2024, 4:02 p.m. UTC | #3 
On Mon, Apr 22, 2024 at 04:06:16PM +0530, Naresh Solanki wrote:
> Hi Guenter,
> 
> On Wed, 17 Apr 2024 at 02:55, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Tue, Apr 16, 2024 at 10:47:14PM +0530, Naresh Solanki wrote:
> > > Add regmap support.
> > >
> >
> > Missing (and not really utilizing) the benefits of using regmap.
> This is just replacing with regmap support
> >
> > > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> > > ---
> > >  drivers/hwmon/Kconfig   |   1 +
> > >  drivers/hwmon/max6639.c | 157 ++++++++++++++++++++--------------------
> > >  2 files changed, 80 insertions(+), 78 deletions(-)
> > >
> > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > index c89776d91795..257ec5360e35 100644
> > > --- a/drivers/hwmon/Kconfig
> > > +++ b/drivers/hwmon/Kconfig
> > > @@ -1223,6 +1223,7 @@ config SENSORS_MAX6621
> > >  config SENSORS_MAX6639
> > >       tristate "Maxim MAX6639 sensor chip"
> > >       depends on I2C
> > > +     select REGMAP_I2C
> > >       help
> > >         If you say yes here you get support for the MAX6639
> > >         sensor chips.
> > > diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
> > > index aa7f21ab2395..1af93fc53cb5 100644
> > > --- a/drivers/hwmon/max6639.c
> > > +++ b/drivers/hwmon/max6639.c
> > > @@ -20,6 +20,7 @@
> > >  #include <linux/err.h>
> > >  #include <linux/mutex.h>
> > >  #include <linux/platform_data/max6639.h>
> > > +#include <linux/regmap.h>
> > >
> > >  /* Addresses to scan */
> > >  static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
> > > @@ -57,6 +58,8 @@ static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
> > >
> > >  #define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED 0x40
> > >
> > > +#define MAX6639_NDEV                         2
> > > +
> > >  static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
> > >
> > >  #define FAN_FROM_REG(val, rpm_range) ((val) == 0 || (val) == 255 ? \
> > > @@ -67,7 +70,7 @@ static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
> > >   * Client data (each client gets its own)
> > >   */
> > >  struct max6639_data {
> > > -     struct i2c_client *client;
> > > +     struct regmap *regmap;
> > >       struct mutex update_lock;
> > >       bool valid;             /* true if following fields are valid */
> > >       unsigned long last_updated;     /* In jiffies */
> > > @@ -95,9 +98,8 @@ struct max6639_data {
> > >  static struct max6639_data *max6639_update_device(struct device *dev)
> > >  {
> > >       struct max6639_data *data = dev_get_drvdata(dev);
> > > -     struct i2c_client *client = data->client;
> > >       struct max6639_data *ret = data;
> > > -     int i;
> > > +     int i, err;
> > >       int status_reg;
> > >
> > >       mutex_lock(&data->update_lock);
> > > @@ -105,39 +107,35 @@ static struct max6639_data *max6639_update_device(struct device *dev)
> >
> > Conversions to regmap should drop all local caching and use regmap
> > for caching (where appropriate) instead.
> max6639_update_device deals with volatile registers & caching isn't
> available for these.
> 

So ? Unless you replace caching (and accept that volatile registers
are not cached) I do not see the value of this patch. Replacing direct
chip accesses with regmap should have a reason better than "because".
Using regmap for caching would be a valid reason. At the same time,
the value of caching volatile registers is very much questionable.

Guenter
Naresh Solanki April 25, 2024, 9:50 a.m. UTC | #4 
Hi Guenter,


On Mon, 22 Apr 2024 at 21:32, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Mon, Apr 22, 2024 at 04:06:16PM +0530, Naresh Solanki wrote:
> > Hi Guenter,
> >
> > On Wed, 17 Apr 2024 at 02:55, Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On Tue, Apr 16, 2024 at 10:47:14PM +0530, Naresh Solanki wrote:
> > > > Add regmap support.
> > > >
> > >
> > > Missing (and not really utilizing) the benefits of using regmap.
> > This is just replacing with regmap support
> > >
> > > > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> > > > ---
> > > >  drivers/hwmon/Kconfig   |   1 +
> > > >  drivers/hwmon/max6639.c | 157 ++++++++++++++++++++--------------------
> > > >  2 files changed, 80 insertions(+), 78 deletions(-)
> > > >
> > > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > > index c89776d91795..257ec5360e35 100644
> > > > --- a/drivers/hwmon/Kconfig
> > > > +++ b/drivers/hwmon/Kconfig
> > > > @@ -1223,6 +1223,7 @@ config SENSORS_MAX6621
> > > >  config SENSORS_MAX6639
> > > >       tristate "Maxim MAX6639 sensor chip"
> > > >       depends on I2C
> > > > +     select REGMAP_I2C
> > > >       help
> > > >         If you say yes here you get support for the MAX6639
> > > >         sensor chips.
> > > > diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
> > > > index aa7f21ab2395..1af93fc53cb5 100644
> > > > --- a/drivers/hwmon/max6639.c
> > > > +++ b/drivers/hwmon/max6639.c
> > > > @@ -20,6 +20,7 @@
> > > >  #include <linux/err.h>
> > > >  #include <linux/mutex.h>
> > > >  #include <linux/platform_data/max6639.h>
> > > > +#include <linux/regmap.h>
> > > >
> > > >  /* Addresses to scan */
> > > >  static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
> > > > @@ -57,6 +58,8 @@ static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
> > > >
> > > >  #define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED 0x40
> > > >
> > > > +#define MAX6639_NDEV                         2
> > > > +
> > > >  static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
> > > >
> > > >  #define FAN_FROM_REG(val, rpm_range) ((val) == 0 || (val) == 255 ? \
> > > > @@ -67,7 +70,7 @@ static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
> > > >   * Client data (each client gets its own)
> > > >   */
> > > >  struct max6639_data {
> > > > -     struct i2c_client *client;
> > > > +     struct regmap *regmap;
> > > >       struct mutex update_lock;
> > > >       bool valid;             /* true if following fields are valid */
> > > >       unsigned long last_updated;     /* In jiffies */
> > > > @@ -95,9 +98,8 @@ struct max6639_data {
> > > >  static struct max6639_data *max6639_update_device(struct device *dev)
> > > >  {
> > > >       struct max6639_data *data = dev_get_drvdata(dev);
> > > > -     struct i2c_client *client = data->client;
> > > >       struct max6639_data *ret = data;
> > > > -     int i;
> > > > +     int i, err;
> > > >       int status_reg;
> > > >
> > > >       mutex_lock(&data->update_lock);
> > > > @@ -105,39 +107,35 @@ static struct max6639_data *max6639_update_device(struct device *dev)
> > >
> > > Conversions to regmap should drop all local caching and use regmap
> > > for caching (where appropriate) instead.
> > max6639_update_device deals with volatile registers & caching isn't
> > available for these.
> >
>
> So ? Unless you replace caching (and accept that volatile registers
> are not cached) I do not see the value of this patch. Replacing direct
> chip accesses with regmap should have a reason better than "because".
> Using regmap for caching would be a valid reason. At the same time,
> the value of caching volatile registers is very much questionable.
This driver has 27 regmap accesses. Except volatile registers, others are
cached by regmap.
Some function which only access volatile registers will not be able to take
advantage of caching. This is also the case in various other drivers for similar
devices.
Also regmap offers bit handling which makes the code much cleaner.

Regards,
Naresh

>
> Guenter
Guenter Roeck April 28, 2024, 5:18 p.m. UTC | #5 
On 4/25/24 02:50, Naresh Solanki wrote:
...
> This driver has 27 regmap accesses. Except volatile registers, others are
> cached by regmap.
> Some function which only access volatile registers will not be able to take
> advantage of caching. This is also the case in various other drivers for similar
> devices.
> Also regmap offers bit handling which makes the code much cleaner.
> 

Maybe I need to make it explicit in documentation. I will not accept regmap
conversions unless local caching is dropped. Yes, that means that volatile
registers will not be cached. I consider that a positive.

Guenter
Naresh Solanki April 29, 2024, 8:19 a.m. UTC | #6 
Hi Guenter,


On Sun, 28 Apr 2024 at 22:48, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 4/25/24 02:50, Naresh Solanki wrote:
> ...
> > This driver has 27 regmap accesses. Except volatile registers, others are
> > cached by regmap.
> > Some function which only access volatile registers will not be able to take
> > advantage of caching. This is also the case in various other drivers for similar
> > devices.
> > Also regmap offers bit handling which makes the code much cleaner.
> >
>
> Maybe I need to make it explicit in documentation. I will not accept regmap
> conversions unless local caching is dropped. Yes, that means that volatile
> registers will not be cached. I consider that a positive.
I agree with you. Regmap conversion wouldn't make sense if local caching
is present.
Correct me if I'm wrong, but in this context, local caching points to the
various variables in max6639_data ?
i.e.,
    bool valid;     /* true if following fields are valid */
    unsigned long last_updated; /* In jiffies */

    /* Register values sampled regularly */
    u16 temp[2];        /* Temperature, in 1/8 C, 0..255 C */
    bool temp_fault[2]; /* Detected temperature diode failure */
    u8 fan[2];      /* Register value: TACH count for fans >=30 */
    u8 status;      /* Detected channel alarms and fan failures */

    /* Register values only written to */
    u8 pwm[2];      /* Register value: Duty cycle 0..120 */
    u8 temp_therm[2];   /* THERM Temperature, 0..255 C (->_max) */
    u8 temp_alert[2];   /* ALERT Temperature, 0..255 C (->_crit) */
    u8 temp_ot[2];      /* OT Temperature, 0..255 C (->_emergency) */

    /* Register values initialized only once */
    u8 ppr;         /* Pulses per rotation 0..3 for 1..4 ppr */
    u8 rpm_range;       /* Index in above rpm_ranges table */

Are you asking for removal of all these variables & each read sysfs
attribute read should access regmap cache directly ?

Regards,
Naresh
>
> Guenter
>
Guenter Roeck April 29, 2024, 1:49 p.m. UTC | #7 
On 4/29/24 01:19, Naresh Solanki wrote:
> Hi Guenter,
> 
> 
> On Sun, 28 Apr 2024 at 22:48, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 4/25/24 02:50, Naresh Solanki wrote:
>> ...
>>> This driver has 27 regmap accesses. Except volatile registers, others are
>>> cached by regmap.
>>> Some function which only access volatile registers will not be able to take
>>> advantage of caching. This is also the case in various other drivers for similar
>>> devices.
>>> Also regmap offers bit handling which makes the code much cleaner.
>>>
>>
>> Maybe I need to make it explicit in documentation. I will not accept regmap
>> conversions unless local caching is dropped. Yes, that means that volatile
>> registers will not be cached. I consider that a positive.
> I agree with you. Regmap conversion wouldn't make sense if local caching
> is present.
> Correct me if I'm wrong, but in this context, local caching points to the
> various variables in max6639_data ?
> i.e.,
>      bool valid;     /* true if following fields are valid */
>      unsigned long last_updated; /* In jiffies */
> 
>      /* Register values sampled regularly */
>      u16 temp[2];        /* Temperature, in 1/8 C, 0..255 C */
>      bool temp_fault[2]; /* Detected temperature diode failure */
>      u8 fan[2];      /* Register value: TACH count for fans >=30 */
>      u8 status;      /* Detected channel alarms and fan failures */
> 
>      /* Register values only written to */
>      u8 pwm[2];      /* Register value: Duty cycle 0..120 */
>      u8 temp_therm[2];   /* THERM Temperature, 0..255 C (->_max) */
>      u8 temp_alert[2];   /* ALERT Temperature, 0..255 C (->_crit) */
>      u8 temp_ot[2];      /* OT Temperature, 0..255 C (->_emergency) */
> 
>      /* Register values initialized only once */
>      u8 ppr;         /* Pulses per rotation 0..3 for 1..4 ppr */
>      u8 rpm_range;       /* Index in above rpm_ranges table */
> 
> Are you asking for removal of all these variables & each read sysfs
> attribute read should access regmap cache directly ?
> 

Mostly yes. Note that "ppr" is only used in max6639_init_client(),
and it is unnecessary to keep it in max6639_data to start with.
rpm_range is ok to keep because it is a calculated initialization value.
The fixed initialization of temp_therm, temp_alert, and temp_ot
is questionable to start with because it overrides bios/rommon
initialization values.

Guenter
diff mbox series

Patch

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index c89776d91795..257ec5360e35 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1223,6 +1223,7 @@  config SENSORS_MAX6621
 config SENSORS_MAX6639
 	tristate "Maxim MAX6639 sensor chip"
 	depends on I2C
+	select REGMAP_I2C
 	help
 	  If you say yes here you get support for the MAX6639
 	  sensor chips.
diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
index aa7f21ab2395..1af93fc53cb5 100644
--- a/drivers/hwmon/max6639.c
+++ b/drivers/hwmon/max6639.c
@@ -20,6 +20,7 @@ 
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/platform_data/max6639.h>
+#include <linux/regmap.h>
 
 /* Addresses to scan */
 static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
@@ -57,6 +58,8 @@  static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
 
 #define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED	0x40
 
+#define MAX6639_NDEV				2
+
 static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
 
 #define FAN_FROM_REG(val, rpm_range)	((val) == 0 || (val) == 255 ? \
@@ -67,7 +70,7 @@  static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
  * Client data (each client gets its own)
  */
 struct max6639_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 	struct mutex update_lock;
 	bool valid;		/* true if following fields are valid */
 	unsigned long last_updated;	/* In jiffies */
@@ -95,9 +98,8 @@  struct max6639_data {
 static struct max6639_data *max6639_update_device(struct device *dev)
 {
 	struct max6639_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	struct max6639_data *ret = data;
-	int i;
+	int i, err;
 	int status_reg;
 
 	mutex_lock(&data->update_lock);
@@ -105,39 +107,35 @@  static struct max6639_data *max6639_update_device(struct device *dev)
 	if (time_after(jiffies, data->last_updated + 2 * HZ) || !data->valid) {
 		int res;
 
-		dev_dbg(&client->dev, "Starting max6639 update\n");
+		dev_dbg(dev, "Starting max6639 update\n");
 
-		status_reg = i2c_smbus_read_byte_data(client,
-						      MAX6639_REG_STATUS);
-		if (status_reg < 0) {
-			ret = ERR_PTR(status_reg);
+		err = regmap_read(data->regmap, MAX6639_REG_STATUS, &status_reg);
+		if (err < 0) {
+			ret = ERR_PTR(err);
 			goto abort;
 		}
 
 		data->status = status_reg;
 
 		for (i = 0; i < 2; i++) {
-			res = i2c_smbus_read_byte_data(client,
-					MAX6639_REG_FAN_CNT(i));
-			if (res < 0) {
-				ret = ERR_PTR(res);
+			err = regmap_read(data->regmap, MAX6639_REG_FAN_CNT(i), &res);
+			if (err < 0) {
+				ret = ERR_PTR(err);
 				goto abort;
 			}
 			data->fan[i] = res;
 
-			res = i2c_smbus_read_byte_data(client,
-					MAX6639_REG_TEMP_EXT(i));
-			if (res < 0) {
-				ret = ERR_PTR(res);
+			err = regmap_read(data->regmap, MAX6639_REG_TEMP_EXT(i), &res);
+			if (err < 0) {
+				ret = ERR_PTR(err);
 				goto abort;
 			}
 			data->temp[i] = res >> 5;
 			data->temp_fault[i] = res & 0x01;
 
-			res = i2c_smbus_read_byte_data(client,
-					MAX6639_REG_TEMP(i));
-			if (res < 0) {
-				ret = ERR_PTR(res);
+			err = regmap_read(data->regmap, MAX6639_REG_TEMP(i), &res);
+			if (err < 0) {
+				ret = ERR_PTR(err);
 				goto abort;
 			}
 			data->temp[i] |= res << 3;
@@ -193,7 +191,6 @@  static ssize_t temp_max_store(struct device *dev,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
 	struct max6639_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	unsigned long val;
 	int res;
 
@@ -203,9 +200,8 @@  static ssize_t temp_max_store(struct device *dev,
 
 	mutex_lock(&data->update_lock);
 	data->temp_therm[attr->index] = TEMP_LIMIT_TO_REG(val);
-	i2c_smbus_write_byte_data(client,
-				  MAX6639_REG_THERM_LIMIT(attr->index),
-				  data->temp_therm[attr->index]);
+	regmap_write(data->regmap, MAX6639_REG_THERM_LIMIT(attr->index),
+		     data->temp_therm[attr->index]);
 	mutex_unlock(&data->update_lock);
 	return count;
 }
@@ -225,7 +221,6 @@  static ssize_t temp_crit_store(struct device *dev,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
 	struct max6639_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	unsigned long val;
 	int res;
 
@@ -235,9 +230,8 @@  static ssize_t temp_crit_store(struct device *dev,
 
 	mutex_lock(&data->update_lock);
 	data->temp_alert[attr->index] = TEMP_LIMIT_TO_REG(val);
-	i2c_smbus_write_byte_data(client,
-				  MAX6639_REG_ALERT_LIMIT(attr->index),
-				  data->temp_alert[attr->index]);
+	regmap_write(data->regmap, MAX6639_REG_ALERT_LIMIT(attr->index),
+		     data->temp_alert[attr->index]);
 	mutex_unlock(&data->update_lock);
 	return count;
 }
@@ -258,7 +252,6 @@  static ssize_t temp_emergency_store(struct device *dev,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
 	struct max6639_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	unsigned long val;
 	int res;
 
@@ -268,9 +261,7 @@  static ssize_t temp_emergency_store(struct device *dev,
 
 	mutex_lock(&data->update_lock);
 	data->temp_ot[attr->index] = TEMP_LIMIT_TO_REG(val);
-	i2c_smbus_write_byte_data(client,
-				  MAX6639_REG_OT_LIMIT(attr->index),
-				  data->temp_ot[attr->index]);
+	regmap_write(data->regmap, MAX6639_REG_OT_LIMIT(attr->index), data->temp_ot[attr->index]);
 	mutex_unlock(&data->update_lock);
 	return count;
 }
@@ -290,7 +281,6 @@  static ssize_t pwm_store(struct device *dev,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
 	struct max6639_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	unsigned long val;
 	int res;
 
@@ -302,9 +292,7 @@  static ssize_t pwm_store(struct device *dev,
 
 	mutex_lock(&data->update_lock);
 	data->pwm[attr->index] = (u8)(val * 120 / 255);
-	i2c_smbus_write_byte_data(client,
-				  MAX6639_REG_TARGTDUTY(attr->index),
-				  data->pwm[attr->index]);
+	regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(attr->index), data->pwm[attr->index]);
 	mutex_unlock(&data->update_lock);
 	return count;
 }
@@ -411,8 +399,7 @@  static int max6639_init_client(struct i2c_client *client,
 	int err;
 
 	/* Reset chip to default values, see below for GCONFIG setup */
-	err = i2c_smbus_write_byte_data(client, MAX6639_REG_GCONFIG,
-				  MAX6639_GCONFIG_POR);
+	err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, MAX6639_GCONFIG_POR);
 	if (err)
 		goto exit;
 
@@ -431,26 +418,21 @@  static int max6639_init_client(struct i2c_client *client,
 	for (i = 0; i < 2; i++) {
 
 		/* Set Fan pulse per revolution */
-		err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_FAN_PPR(i),
-				data->ppr << 6);
+		err = regmap_write(data->regmap, MAX6639_REG_FAN_PPR(i), data->ppr << 6);
 		if (err)
 			goto exit;
 
 		/* Fans config PWM, RPM */
-		err = i2c_smbus_write_byte_data(client,
-			MAX6639_REG_FAN_CONFIG1(i),
-			MAX6639_FAN_CONFIG1_PWM | rpm_range);
+		err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG1(i),
+				   MAX6639_FAN_CONFIG1_PWM | rpm_range);
 		if (err)
 			goto exit;
 
 		/* Fans PWM polarity high by default */
 		if (max6639_info && max6639_info->pwm_polarity == 0)
-			err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_FAN_CONFIG2a(i), 0x00);
+			err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), 0x00);
 		else
-			err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_FAN_CONFIG2a(i), 0x02);
+			err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), 0x02);
 		if (err)
 			goto exit;
 
@@ -458,9 +440,8 @@  static int max6639_init_client(struct i2c_client *client,
 		 * /THERM full speed enable,
 		 * PWM frequency 25kHz, see also GCONFIG below
 		 */
-		err = i2c_smbus_write_byte_data(client,
-			MAX6639_REG_FAN_CONFIG3(i),
-			MAX6639_FAN_CONFIG3_THERM_FULL_SPEED | 0x03);
+		err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG3(i),
+				   MAX6639_FAN_CONFIG3_THERM_FULL_SPEED | 0x03);
 		if (err)
 			goto exit;
 
@@ -468,32 +449,26 @@  static int max6639_init_client(struct i2c_client *client,
 		data->temp_therm[i] = 80;
 		data->temp_alert[i] = 90;
 		data->temp_ot[i] = 100;
-		err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_THERM_LIMIT(i),
-				data->temp_therm[i]);
+		err = regmap_write(data->regmap, MAX6639_REG_THERM_LIMIT(i), data->temp_therm[i]);
 		if (err)
 			goto exit;
-		err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_ALERT_LIMIT(i),
-				data->temp_alert[i]);
+		err = regmap_write(data->regmap, MAX6639_REG_ALERT_LIMIT(i), data->temp_alert[i]);
 		if (err)
 			goto exit;
-		err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_OT_LIMIT(i), data->temp_ot[i]);
+		err = regmap_write(data->regmap, MAX6639_REG_OT_LIMIT(i), data->temp_ot[i]);
 		if (err)
 			goto exit;
 
 		/* PWM 120/120 (i.e. 100%) */
 		data->pwm[i] = 120;
-		err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_TARGTDUTY(i), data->pwm[i]);
+		err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), data->pwm[i]);
 		if (err)
 			goto exit;
 	}
 	/* Start monitoring */
-	err = i2c_smbus_write_byte_data(client, MAX6639_REG_GCONFIG,
-		MAX6639_GCONFIG_DISABLE_TIMEOUT | MAX6639_GCONFIG_CH2_LOCAL |
-		MAX6639_GCONFIG_PWM_FREQ_HI);
+	err = regmap_write(data->regmap, MAX6639_REG_GCONFIG,
+			   MAX6639_GCONFIG_DISABLE_TIMEOUT | MAX6639_GCONFIG_CH2_LOCAL |
+			   MAX6639_GCONFIG_PWM_FREQ_HI);
 exit:
 	return err;
 }
@@ -524,6 +499,30 @@  static void max6639_regulator_disable(void *data)
 	regulator_disable(data);
 }
 
+static bool max6639_regmap_is_volatile(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MAX6639_REG_TEMP(0):
+	case MAX6639_REG_TEMP_EXT(0):
+	case MAX6639_REG_TEMP(1):
+	case MAX6639_REG_TEMP_EXT(1):
+	case MAX6639_REG_STATUS:
+	case MAX6639_REG_FAN_CNT(0):
+	case MAX6639_REG_FAN_CNT(1):
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config max6639_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = MAX6639_REG_DEVREV,
+	.cache_type = REGCACHE_MAPLE,
+	.volatile_reg = max6639_regmap_is_volatile,
+};
+
 static int max6639_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -535,7 +534,11 @@  static int max6639_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
-	data->client = client;
+	data->regmap = devm_regmap_init_i2c(client, &max6639_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev,
+				     PTR_ERR(data->regmap),
+				     "regmap initialization failed\n");
 
 	data->reg = devm_regulator_get_optional(dev, "fan");
 	if (IS_ERR(data->reg)) {
@@ -573,25 +576,24 @@  static int max6639_probe(struct i2c_client *client)
 
 static int max6639_suspend(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct max6639_data *data = dev_get_drvdata(dev);
-	int ret = i2c_smbus_read_byte_data(client, MAX6639_REG_GCONFIG);
+	int ret, err;
+
+	err = regmap_read(data->regmap, MAX6639_REG_GCONFIG, &ret);
 
-	if (ret < 0)
-		return ret;
+	if (err < 0)
+		return err;
 
 	if (data->reg)
 		regulator_disable(data->reg);
 
-	return i2c_smbus_write_byte_data(client,
-			MAX6639_REG_GCONFIG, ret | MAX6639_GCONFIG_STANDBY);
+	return regmap_write(data->regmap, MAX6639_REG_GCONFIG, ret | MAX6639_GCONFIG_STANDBY);
 }
 
 static int max6639_resume(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct max6639_data *data = dev_get_drvdata(dev);
-	int ret;
+	int ret, err;
 
 	if (data->reg) {
 		ret = regulator_enable(data->reg);
@@ -601,12 +603,11 @@  static int max6639_resume(struct device *dev)
 		}
 	}
 
-	ret = i2c_smbus_read_byte_data(client, MAX6639_REG_GCONFIG);
-	if (ret < 0)
-		return ret;
+	err = regmap_read(data->regmap, MAX6639_REG_GCONFIG, &ret);
+	if (err < 0)
+		return err;
 
-	return i2c_smbus_write_byte_data(client,
-			MAX6639_REG_GCONFIG, ret & ~MAX6639_GCONFIG_STANDBY);
+	return regmap_write(data->regmap, MAX6639_REG_GCONFIG, ret & ~MAX6639_GCONFIG_STANDBY);
 }
 
 static const struct i2c_device_id max6639_id[] = {