linux-kernel - Re: [PATCH 1/4] hwmon (max6639): Use regmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABqG17jHYymH02hAH0z-uqk2HjemL_-aLr9EyOUNR=uQ7U_wtA@mail.gmail.com>
Date: Thu, 25 Apr 2024 15:20:35 +0530
From: Naresh Solanki <naresh.solanki@...ements.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: krzysztof.kozlowski+dt@...aro.org, u.kleine-koenig@...gutronix.de, 
	Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] hwmon (max6639): Use regmap

Hi Guenter,


On Mon, 22 Apr 2024 at 21:32, Guenter Roeck <linux@...ck-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@...ck-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@...ements.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