Received-SPF: pass (google.com: domain of linux-kernel+bounces-158332-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240416171720.2875916-1-naresh.solanki@9elements.com>
 <65607114-89f8-4f48-83fc-b89d87fee247@roeck-us.net> <CABqG17jveAdxN+tCcPq4hNqWDy1YCJ1z0+kLpM+ect0XEyqBcA@mail.gmail.com>
 <116aeea1-c648-4abe-9ab2-693bf64000fc@roeck-us.net>
In-Reply-To: <116aeea1-c648-4abe-9ab2-693bf64000fc@roeck-us.net>
From: Naresh Solanki <naresh.solanki@9elements.com>
Date: Thu, 25 Apr 2024 15:20:35 +0530
Message-ID: <CABqG17jHYymH02hAH0z-uqk2HjemL_-aLr9EyOUNR=uQ7U_wtA@mail.gmail.com>
Subject: Re: [PATCH 1/4] hwmon (max6639): Use regmap
To: Guenter Roeck <linux@roeck-us.net>
Cc: krzysztof.kozlowski+dt@linaro.org, u.kleine-koenig@pengutronix.de, 
	Jean Delvare <jdelvare@suse.com>, linux-hwmon@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

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