From: Naresh Solanki <naresh.solanki@9elements.com>
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
Subject: Re: [PATCH 1/4] hwmon (max6639): Use regmap
Date: Thu, 25 Apr 2024 15:20:35 +0530 [thread overview]
Message-ID: <CABqG17jHYymH02hAH0z-uqk2HjemL_-aLr9EyOUNR=uQ7U_wtA@mail.gmail.com> (raw)
In-Reply-To: <116aeea1-c648-4abe-9ab2-693bf64000fc@roeck-us.net>
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
next prev parent reply other threads:[~2024-04-25 9:50 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-16 17:17 [PATCH 1/4] hwmon (max6639): Use regmap Naresh Solanki
2024-04-16 17:17 ` [PATCH 2/4] hwmon: (max6639) : Utilise pwm subsystem Naresh Solanki
2024-04-16 21:22 ` Guenter Roeck
2024-04-22 10:39 ` Naresh Solanki
2024-04-22 12:37 ` Guenter Roeck
2024-05-06 10:05 ` Naresh Solanki
2024-05-06 13:49 ` Guenter Roeck
2024-04-17 7:04 ` Uwe Kleine-König
2024-04-16 17:17 ` [PATCH 3/4] hwmon: (max6639) : Add hwmon init using info Naresh Solanki
2024-04-16 17:17 ` [PATCH 4/4] hwmon (max6639): Remove hwmon init with group Naresh Solanki
2024-04-16 21:23 ` Guenter Roeck
2024-04-16 21:25 ` [PATCH 1/4] hwmon (max6639): Use regmap Guenter Roeck
2024-04-22 10:36 ` Naresh Solanki
2024-04-22 16:02 ` Guenter Roeck
2024-04-25 9:50 ` Naresh Solanki [this message]
2024-04-28 17:18 ` Guenter Roeck
2024-04-29 8:19 ` Naresh Solanki
2024-04-29 13:49 ` Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CABqG17jHYymH02hAH0z-uqk2HjemL_-aLr9EyOUNR=uQ7U_wtA@mail.gmail.com' \
--to=naresh.solanki@9elements.com \
--cc=jdelvare@suse.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=u.kleine-koenig@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.