2024-04-16 17:17:55

by Naresh Solanki

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

Add regmap support.

Signed-off-by: Naresh Solanki <[email protected]>
---
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)
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[] = {

base-commit: db85dba9fee5fed54e2c37eed465f8fd243a92e8
--
2.42.0

2024-04-16 17:17:58

by Naresh Solanki

Subject: [PATCH 2/4] hwmon: (max6639) : Utilise pwm subsystem

Utilise pwm subsystem for fan pwm handling

Signed-off-by: Naresh Solanki <[email protected]>
---
drivers/hwmon/Kconfig | 1 +
drivers/hwmon/max6639.c | 200 ++++++++++++++++++++++++++++++++++++++--
2 files changed, 191 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 257ec5360e35..c9cc74f8c807 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1224,6 +1224,7 @@ config SENSORS_MAX6639
tristate "Maxim MAX6639 sensor chip"
depends on I2C
select REGMAP_I2C
+ depends on PWM
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 1af93fc53cb5..f37fdd161154 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/pwm.h>
#include <linux/regmap.h>

/* Addresses to scan */
@@ -55,6 +56,9 @@ static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
#define MAX6639_GCONFIG_PWM_FREQ_HI 0x08

#define MAX6639_FAN_CONFIG1_PWM 0x80
+#define MAX6639_REG_FAN_CONFIG2a_PWM_POL 0x02
+#define MAX6639_FAN_CONFIG3_FREQ_MASK 0x03
+#define MAX6639_REG_TARGTDUTY_SLOT 120

#define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED 0x40

@@ -62,6 +66,10 @@ static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };

static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };

+/* Supported PWM frequency */
+static const unsigned int freq_table[] = { 20, 33, 50, 100, 5000, 8333, 12500,
+ 25000 };
+
#define FAN_FROM_REG(val, rpm_range) ((val) == 0 || (val) == 255 ? \
0 : (rpm_ranges[rpm_range] * 30) / (val))
#define TEMP_LIMIT_TO_REG(val) clamp_val((val) / 1000, 0, 255)
@@ -93,6 +101,9 @@ struct max6639_data {

/* Optional regulator for FAN supply */
struct regulator *reg;
+ /* max6639 pwm chip */
+ struct pwm_chip chip;
+ struct pwm_device *pwmd[MAX6639_NDEV]; /* max6639 has two pwm device */
};

static struct max6639_data *max6639_update_device(struct device *dev)
@@ -271,8 +282,11 @@ static ssize_t pwm_show(struct device *dev, struct device_attribute *dev_attr,
{
struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
struct max6639_data *data = dev_get_drvdata(dev);
+ struct pwm_state state;
+
+ pwm_get_state(data->pwmd[attr->index], &state);

- return sprintf(buf, "%d\n", data->pwm[attr->index] * 255 / 120);
+ return sprintf(buf, "%d\n", pwm_get_relative_duty_cycle(&state, 255));
}

static ssize_t pwm_store(struct device *dev,
@@ -281,6 +295,7 @@ 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 pwm_state state;
unsigned long val;
int res;

@@ -290,10 +305,10 @@ static ssize_t pwm_store(struct device *dev,

val = clamp_val(val, 0, 255);

- mutex_lock(&data->update_lock);
- data->pwm[attr->index] = (u8)(val * 120 / 255);
- regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(attr->index), data->pwm[attr->index]);
- mutex_unlock(&data->update_lock);
+ pwm_get_state(data->pwmd[attr->index], &state);
+ pwm_set_relative_duty_cycle(&state, val, 255);
+ pwm_apply_state(data->pwmd[attr->index], &state);
+
return count;
}

@@ -373,6 +388,158 @@ static struct attribute *max6639_attrs[] = {
};
ATTRIBUTE_GROUPS(max6639);

+static struct max6639_data *to_max6639_pwm(struct pwm_chip *chip)
+{
+ return container_of(chip, struct max6639_data, chip);
+}
+
+static int max6639_pwm_get_state(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct max6639_data *data = to_max6639_pwm(chip);
+ int value, i = pwm->hwpwm, x, err;
+ unsigned int freq;
+
+ mutex_lock(&data->update_lock);
+
+ err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG1(i), &value);
+ if (err < 0)
+ goto abort;
+
+ if (value & MAX6639_FAN_CONFIG1_PWM) {
+ state->enabled = true;
+
+ /* Determine frequency from respective registers */
+ err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG3(i), &value);
+ if (err < 0)
+ goto abort;
+ x = value & MAX6639_FAN_CONFIG3_FREQ_MASK;
+
+ err = regmap_read(data->regmap, MAX6639_REG_GCONFIG, &value);
+ if (err < 0)
+ goto abort;
+ if (value & MAX6639_GCONFIG_PWM_FREQ_HI)
+ x |= 0x4;
+ x &= 0x7;
+ freq = freq_table[x];
+
+ state->period = DIV_ROUND_UP(NSEC_PER_SEC, freq);
+
+ err = regmap_read(data->regmap, MAX6639_REG_TARGTDUTY(i), &value);
+ if (err < 0)
+ goto abort;
+ /* max6639 supports 120 slots only */
+ state->duty_cycle = mul_u64_u32_div(state->period, value, 120);
+
+ err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), &value);
+ if (err < 0)
+ goto abort;
+ value &= MAX6639_REG_FAN_CONFIG2a_PWM_POL;
+ state->polarity = (value != 0);
+ } else {
+ state->enabled = false;
+ }
+
+abort:
+ mutex_unlock(&data->update_lock);
+ return value;
+}
+
+static int max6639_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct max6639_data *data = to_max6639_pwm(chip);
+ int value, i = pwm->hwpwm, x, err;
+ unsigned int freq;
+ struct pwm_state cstate;
+
+ cstate = pwm->state;
+
+ mutex_lock(&data->update_lock);
+
+ if (state->period != cstate.period) {
+ /* Configure frequency */
+ freq = DIV_ROUND_UP_ULL(NSEC_PER_SEC, state->period);
+
+ /* Chip supports limited number of frequency */
+ for (x = 0; x < sizeof(freq_table); x++)
+ if (freq <= freq_table[x])
+ break;
+
+ err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG3(i), &value);
+ if (err < 0)
+ goto abort;
+
+ value &= ~MAX6639_FAN_CONFIG3_FREQ_MASK;
+ value |= (x & MAX6639_FAN_CONFIG3_FREQ_MASK);
+ err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG3(i), value);
+ if (err < 0)
+ goto abort;
+
+ err = regmap_read(data->regmap, MAX6639_REG_GCONFIG, &value);
+ if (err < 0)
+ goto abort;
+
+ if (x >> 2)
+ value &= ~MAX6639_GCONFIG_PWM_FREQ_HI;
+ else
+ value |= MAX6639_GCONFIG_PWM_FREQ_HI;
+ err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, value);
+ if (err < 0)
+ goto abort;
+ }
+
+ /* Configure dutycycle */
+ if (state->duty_cycle != cstate.duty_cycle ||
+ state->period != cstate.period) {
+ value = DIV_ROUND_DOWN_ULL(state->duty_cycle * MAX6639_REG_TARGTDUTY_SLOT,
+ state->period);
+ err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), value);
+ if (err < 0)
+ goto abort;
+ }
+
+ /* Configure polarity */
+ if (state->polarity != cstate.polarity) {
+ err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), &value);
+ if (err < 0)
+ goto abort;
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ value |= MAX6639_REG_FAN_CONFIG2a_PWM_POL;
+ else
+ value &= ~MAX6639_REG_FAN_CONFIG2a_PWM_POL;
+ err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), value);
+ if (err < 0)
+ goto abort;
+ }
+
+ if (state->enabled != cstate.enabled) {
+ err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG1(i), &value);
+ if (err < 0)
+ goto abort;
+ if (state->enabled)
+ value |= MAX6639_FAN_CONFIG1_PWM;
+ else
+ value &= ~MAX6639_FAN_CONFIG1_PWM;
+
+ err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG1(i), value);
+ if (err < 0)
+ goto abort;
+ }
+ value = 0;
+
+abort:
+ mutex_unlock(&data->update_lock);
+
+ return err;
+}
+
+static const struct pwm_ops max6639_pwm_ops = {
+ .apply = max6639_pwm_apply,
+ .get_state = max6639_pwm_get_state,
+};
+
/*
* returns respective index in rpm_ranges table
* 1 by default on invalid range
@@ -396,6 +563,7 @@ static int max6639_init_client(struct i2c_client *client,
dev_get_platdata(&client->dev);
int i;
int rpm_range = 1; /* default: 4000 RPM */
+ struct pwm_state state;
int err;

/* Reset chip to default values, see below for GCONFIG setup */
@@ -459,11 +627,15 @@ static int max6639_init_client(struct i2c_client *client,
if (err)
goto exit;

- /* PWM 120/120 (i.e. 100%) */
- data->pwm[i] = 120;
- err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), data->pwm[i]);
- if (err)
- goto exit;
+ dev_dbg(&client->dev, "Using chip default PWM");
+ data->pwmd[i] = pwm_request_from_chip(&data->chip, i, NULL);
+ if (IS_ERR(data->pwmd[i]))
+ return PTR_ERR(data->pwmd[i]);
+ pwm_get_state(data->pwmd[i], &state);
+ state.period = DIV_ROUND_UP(NSEC_PER_SEC, 25000);
+ state.polarity = PWM_POLARITY_NORMAL;
+ pwm_set_relative_duty_cycle(&state, 0, 255);
+ pwm_apply_state(data->pwmd[i], &state);
}
/* Start monitoring */
err = regmap_write(data->regmap, MAX6639_REG_GCONFIG,
@@ -540,6 +712,14 @@ static int max6639_probe(struct i2c_client *client)
PTR_ERR(data->regmap),
"regmap initialization failed\n");

+ /* Add PWM controller of max6639 */
+ data->chip.dev = dev;
+ data->chip.ops = &max6639_pwm_ops;
+ data->chip.npwm = MAX6639_NDEV;
+ err = devm_pwmchip_add(dev, &data->chip);
+ if (err < 0)
+ return dev_err_probe(dev, err, "failed to add PWM chip\n");
+
data->reg = devm_regulator_get_optional(dev, "fan");
if (IS_ERR(data->reg)) {
if (PTR_ERR(data->reg) != -ENODEV)
--
2.42.0

2024-04-16 17:18:13

by Naresh Solanki

Subject: [PATCH 3/4] hwmon: (max6639) : Add hwmon init using info

Add hwmon init with info
Also added additional attribute for fan i.e.,
fanY_pulse
pwmY_freq

Signed-off-by: Naresh Solanki <[email protected]>
---
drivers/hwmon/max6639.c | 408 +++++++++++++++++++++++++++++++++++++---
1 file changed, 379 insertions(+), 29 deletions(-)

diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
index f37fdd161154..d9b23202d7a2 100644
--- a/drivers/hwmon/max6639.c
+++ b/drivers/hwmon/max6639.c
@@ -62,6 +62,8 @@ static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };

#define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED 0x40

+#define MAX6639_FAN_PPR_MASK(ppr) (((ppr) - 1) << 6)
+
#define MAX6639_NDEV 2

static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
@@ -79,25 +81,27 @@ static const unsigned int freq_table[] = { 20, 33, 50, 100, 5000, 8333, 12500,
*/
struct max6639_data {
struct regmap *regmap;
+ struct device *hwmon_dev;
struct mutex update_lock;
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 */
+ u16 temp[MAX6639_NDEV]; /* Temperature, in 1/8 C, 0..255 C */
+ bool temp_fault[MAX6639_NDEV]; /* Detected temperature diode failure */
+ bool fan_enable[MAX6639_NDEV];
+ u8 fan[MAX6639_NDEV]; /* 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) */
+ u8 pwm[MAX6639_NDEV]; /* Register value: Duty cycle 0..120 */
+ u8 temp_therm[MAX6639_NDEV]; /* THERM Temperature, 0..255 C (->_max) */
+ u8 temp_alert[MAX6639_NDEV]; /* ALERT Temperature, 0..255 C (->_crit) */
+ u8 temp_ot[MAX6639_NDEV]; /* 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 */
+ u8 ppr[MAX6639_NDEV]; /* Pulses per rotation 0..3 for 1..4 ppr */
+ u8 rpm_range[MAX6639_NDEV]; /* Index in above rpm_ranges table */

/* Optional regulator for FAN supply */
struct regulator *reg;
@@ -128,7 +132,7 @@ static struct max6639_data *max6639_update_device(struct device *dev)

data->status = status_reg;

- for (i = 0; i < 2; i++) {
+ for (i = 0; i < MAX6639_NDEV; i++) {
err = regmap_read(data->regmap, MAX6639_REG_FAN_CNT(i), &res);
if (err < 0) {
ret = ERR_PTR(err);
@@ -322,7 +326,7 @@ static ssize_t fan_input_show(struct device *dev,
return PTR_ERR(data);

return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
- data->rpm_range));
+ data->rpm_range[attr->index]));
}

static ssize_t alarm_show(struct device *dev,
@@ -388,6 +392,342 @@ static struct attribute *max6639_attrs[] = {
};
ATTRIBUTE_GROUPS(max6639);

+static int max6639_temp_set_max(struct max6639_data *data, int channel, unsigned long val)
+{
+ int res;
+
+ mutex_lock(&data->update_lock);
+ data->temp_therm[channel] = TEMP_LIMIT_TO_REG(val);
+ res = regmap_write(data->regmap, MAX6639_REG_THERM_LIMIT(channel),
+ data->temp_therm[channel]);
+ mutex_unlock(&data->update_lock);
+ return res;
+}
+
+static int max6639_temp_set_crit(struct max6639_data *data, int channel, unsigned long val)
+{
+ int res;
+
+ mutex_lock(&data->update_lock);
+ data->temp_alert[channel] = TEMP_LIMIT_TO_REG(val);
+ res = regmap_write(data->regmap, MAX6639_REG_ALERT_LIMIT(channel),
+ data->temp_alert[channel]);
+ mutex_unlock(&data->update_lock);
+ return res;
+}
+
+static int max6639_temp_set_emergency(struct max6639_data *data, int channel, unsigned long val)
+{
+ int res;
+
+ mutex_lock(&data->update_lock);
+ data->temp_ot[channel] = TEMP_LIMIT_TO_REG(val);
+ res = regmap_write(data->regmap, MAX6639_REG_OT_LIMIT(channel),
+ data->temp_ot[channel]);
+ mutex_unlock(&data->update_lock);
+ return res;
+}
+
+static int set_ppr(struct max6639_data *data, u8 channel, u8 ppr)
+{
+ return regmap_write(data->regmap, MAX6639_REG_FAN_PPR(channel), MAX6639_FAN_PPR_MASK(ppr));
+}
+
+static int max6639_read_fan(struct device *dev, u32 attr, int channel,
+ long *val)
+{
+ struct max6639_data *data = max6639_update_device(dev);
+
+ if (IS_ERR(data))
+ return PTR_ERR(data);
+
+ switch (attr) {
+ case hwmon_fan_input:
+ *val = FAN_FROM_REG(data->fan[channel], data->rpm_range[channel]);
+ return 0;
+ case hwmon_fan_fault:
+ *val = !!(data->status & (2 >> channel));
+ return 0;
+ case hwmon_fan_pulses:
+ *val = data->ppr[channel];
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int max6639_write_fan(struct device *dev, u32 attr, int channel,
+ long val)
+{
+ struct max6639_data *data = max6639_update_device(dev);
+ int err;
+
+ if (IS_ERR(data))
+ return PTR_ERR(data);
+
+ switch (attr) {
+ case hwmon_fan_pulses:
+ if (val <= 0 || val > 5) {
+ dev_err(dev, "invalid pulses-per-revolution %ld. Valid range id 1 - 4.",
+ val);
+ return -EINVAL;
+ }
+ /* Set Fan pulse per revolution */
+ err = set_ppr(data, channel, val);
+ if (err)
+ dev_err(dev, "Failed to set pulses-per-revolution");
+ else
+ data->ppr[channel] = val;
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static umode_t max6639_fan_is_visible(const void *_data, u32 attr, int channel)
+{
+ struct max6639_data *data = (struct max6639_data *)_data;
+
+ if (IS_ERR(data))
+ return PTR_ERR(data);
+
+ switch (attr) {
+ case hwmon_fan_input:
+ case hwmon_fan_fault:
+ if (data->fan_enable[channel])
+ return 0444;
+ else
+ return 0;
+ case hwmon_fan_pulses:
+ if (data->fan_enable[channel])
+ return 0644;
+ else
+ return 0;
+ default:
+ return 0;
+ }
+}
+
+static int max6639_read_pwm(struct device *dev, u32 attr, int channel,
+ long *val)
+{
+ struct max6639_data *data = max6639_update_device(dev);
+ struct pwm_state state;
+
+ if (IS_ERR(data))
+ return PTR_ERR(data);
+
+ switch (attr) {
+ case hwmon_pwm_input:
+ pwm_get_state(data->pwmd[channel], &state);
+ *val = pwm_get_relative_duty_cycle(&state, 255);
+ return 0;
+ case hwmon_pwm_freq:
+ pwm_get_state(data->pwmd[channel], &state);
+ *val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, state.period);
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int max6639_write_pwm(struct device *dev, u32 attr, int channel,
+ long val)
+{
+ struct max6639_data *data = max6639_update_device(dev);
+ struct pwm_state state;
+ int err, duty_cycle;
+
+ if (IS_ERR(data))
+ return PTR_ERR(data);
+
+ switch (attr) {
+ case hwmon_pwm_input:
+ val = clamp_val(val, 0, 255);
+ pwm_get_state(data->pwmd[channel], &state);
+ pwm_set_relative_duty_cycle(&state, val, 255);
+ err = pwm_apply_state(data->pwmd[channel], &state);
+ return err;
+
+ case hwmon_pwm_freq:
+ val = clamp_val(val, 0, 25000);
+ pwm_get_state(data->pwmd[channel], &state);
+ duty_cycle = pwm_get_relative_duty_cycle(&state, 255);
+ state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, val);
+ pwm_set_relative_duty_cycle(&state, duty_cycle, 255);
+ err = pwm_apply_state(data->pwmd[channel], &state);
+ return err;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static umode_t max6639_pwm_is_visible(const void *_data, u32 attr, int channel)
+{
+ struct max6639_data *data = (struct max6639_data *)_data;
+
+ if (IS_ERR(data))
+ return PTR_ERR(data);
+
+ if (IS_ERR(data->pwmd[channel]))
+ return 0;
+
+ switch (attr) {
+ case hwmon_pwm_input:
+ case hwmon_pwm_freq:
+ if (data->fan_enable[channel])
+ return 0644;
+ else
+ return 0;
+ default:
+ return 0;
+ }
+}
+
+static int max6639_read_temp(struct device *dev, u32 attr, int channel,
+ long *val)
+{
+ struct max6639_data *data = max6639_update_device(dev);
+
+ switch (attr) {
+ case hwmon_temp_input:
+ *val = data->temp[channel] * 125;
+ return 0;
+ case hwmon_temp_fault:
+ *val = data->temp_fault[channel] * 125;
+ return 0;
+ case hwmon_temp_max:
+ *val = data->temp_therm[channel] * 1000;
+ return 0;
+ case hwmon_temp_crit:
+ *val = data->temp_alert[channel] * 1000;
+ return 0;
+ case hwmon_temp_emergency:
+ *val = data->temp_ot[channel] * 1000;
+ return 0;
+ case hwmon_temp_max_alarm:
+ *val = !!(data->status & (0x08 >> channel));
+ return 0;
+ case hwmon_temp_crit_alarm:
+ *val = !!(data->status & (0x80 >> channel));
+ return 0;
+ case hwmon_temp_emergency_alarm:
+ *val = !!(data->status & (0x20 >> channel));
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int max6639_write_temp(struct device *dev, u32 attr, int channel,
+ long val)
+{
+ struct max6639_data *data = max6639_update_device(dev);
+
+ switch (attr) {
+ case hwmon_temp_max:
+ return max6639_temp_set_max(data, channel, val);
+ case hwmon_temp_crit:
+ return max6639_temp_set_crit(data, channel, val);
+ case hwmon_temp_emergency:
+ return max6639_temp_set_emergency(data, channel, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static umode_t MAX6639_NDEV_is_visible(const void *_data, u32 attr, int channel)
+{
+ switch (attr) {
+ case hwmon_temp_input:
+ case hwmon_temp_fault:
+ case hwmon_temp_max_alarm:
+ case hwmon_temp_crit_alarm:
+ case hwmon_temp_emergency_alarm:
+ return 0444;
+ case hwmon_temp_max:
+ case hwmon_temp_crit:
+ case hwmon_temp_emergency:
+ return 0644;
+ default:
+ return 0;
+ }
+}
+
+static int max6639_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ switch (type) {
+ case hwmon_fan:
+ return max6639_read_fan(dev, attr, channel, val);
+ case hwmon_pwm:
+ return max6639_read_pwm(dev, attr, channel, val);
+ case hwmon_temp:
+ return max6639_read_temp(dev, attr, channel, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int max6639_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ switch (type) {
+ case hwmon_fan:
+ return max6639_write_fan(dev, attr, channel, val);
+ case hwmon_pwm:
+ return max6639_write_pwm(dev, attr, channel, val);
+ case hwmon_temp:
+ return max6639_write_temp(dev, attr, channel, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static umode_t max6639_is_visible(const void *data,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ switch (type) {
+ case hwmon_fan:
+ return max6639_fan_is_visible(data, attr, channel);
+ case hwmon_pwm:
+ return max6639_pwm_is_visible(data, attr, channel);
+ case hwmon_temp:
+ return MAX6639_NDEV_is_visible(data, attr, channel);
+ default:
+ return 0;
+ }
+}
+
+static const struct hwmon_channel_info * const max6639_info[] = {
+ HWMON_CHANNEL_INFO(fan,
+ HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_PULSES,
+ HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_PULSES),
+ HWMON_CHANNEL_INFO(pwm,
+ HWMON_PWM_INPUT | HWMON_PWM_FREQ,
+ HWMON_PWM_INPUT | HWMON_PWM_FREQ),
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_MAX | HWMON_T_MAX_ALARM |
+ HWMON_T_CRIT | HWMON_T_CRIT_ALARM | HWMON_T_EMERGENCY |
+ HWMON_T_EMERGENCY_ALARM,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_MAX | HWMON_T_MAX_ALARM |
+ HWMON_T_CRIT | HWMON_T_CRIT_ALARM | HWMON_T_EMERGENCY |
+ HWMON_T_EMERGENCY_ALARM),
+ NULL
+};
+
+static const struct hwmon_ops max6639_hwmon_ops = {
+ .is_visible = max6639_is_visible,
+ .read = max6639_read,
+ .write = max6639_write,
+};
+
+static const struct hwmon_chip_info max6639_chip_info = {
+ .ops = &max6639_hwmon_ops,
+ .info = max6639_info,
+};
+
static struct max6639_data *to_max6639_pwm(struct pwm_chip *chip)
{
return container_of(chip, struct max6639_data, chip);
@@ -542,18 +882,18 @@ static const struct pwm_ops max6639_pwm_ops = {

/*
* returns respective index in rpm_ranges table
- * 1 by default on invalid range
+ * 3 by default on invalid range
*/
static int rpm_range_to_reg(int range)
{
int i;

for (i = 0; i < ARRAY_SIZE(rpm_ranges); i++) {
- if (rpm_ranges[i] == range)
+ if (range <= rpm_ranges[i])
return i;
}

- return 1; /* default: 4000 RPM */
+ return 3; /* default: 16000 RPM */
}

static int max6639_init_client(struct i2c_client *client,
@@ -564,7 +904,7 @@ static int max6639_init_client(struct i2c_client *client,
int i;
int rpm_range = 1; /* default: 4000 RPM */
struct pwm_state state;
- int err;
+ int err, ppr = 2;

/* Reset chip to default values, see below for GCONFIG setup */
err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, MAX6639_GCONFIG_POR);
@@ -574,25 +914,27 @@ static int max6639_init_client(struct i2c_client *client,
/* Fans pulse per revolution is 2 by default */
if (max6639_info && max6639_info->ppr > 0 &&
max6639_info->ppr < 5)
- data->ppr = max6639_info->ppr;
- else
- data->ppr = 2;
- data->ppr -= 1;
+ ppr = max6639_info->ppr;
+ ppr -= 1;
+ data->ppr[0] = ppr;
+ data->ppr[1] = ppr;

if (max6639_info)
rpm_range = rpm_range_to_reg(max6639_info->rpm_range);
- data->rpm_range = rpm_range;
+ data->rpm_range[0] = rpm_range;
+ data->rpm_range[1] = rpm_range;

- for (i = 0; i < 2; i++) {
+ for (i = 0; i < MAX6639_NDEV; i++) {

/* Set Fan pulse per revolution */
- err = regmap_write(data->regmap, MAX6639_REG_FAN_PPR(i), data->ppr << 6);
+ err = regmap_write(data->regmap, MAX6639_REG_FAN_PPR(i),
+ MAX6639_FAN_PPR_MASK(data->ppr[i]));
if (err)
goto exit;

/* Fans config PWM, RPM */
err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG1(i),
- MAX6639_FAN_CONFIG1_PWM | rpm_range);
+ MAX6639_FAN_CONFIG1_PWM | data->rpm_range[i]);
if (err)
goto exit;

@@ -617,6 +959,8 @@ static int max6639_init_client(struct i2c_client *client,
data->temp_therm[i] = 80;
data->temp_alert[i] = 90;
data->temp_ot[i] = 100;
+ data->fan_enable[i] = true;
+
err = regmap_write(data->regmap, MAX6639_REG_THERM_LIMIT(i), data->temp_therm[i]);
if (err)
goto exit;
@@ -699,7 +1043,6 @@ static int max6639_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
struct max6639_data *data;
- struct device *hwmon_dev;
int err;

data = devm_kzalloc(dev, sizeof(struct max6639_data), GFP_KERNEL);
@@ -748,10 +1091,17 @@ static int max6639_probe(struct i2c_client *client)
if (err < 0)
return err;

- hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
- data,
- max6639_groups);
- return PTR_ERR_OR_ZERO(hwmon_dev);
+ if (0)
+ data->hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
+ data,
+ max6639_groups);
+
+ data->hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
+ &max6639_chip_info, NULL);
+ if (IS_ERR(data->hwmon_dev))
+ return dev_err_probe(dev, PTR_ERR(data->hwmon_dev),
+ "unable to register hwmon device\n");
+ return 0;
}

static int max6639_suspend(struct device *dev)
--
2.42.0

2024-04-16 17:18:19

by Naresh Solanki

Subject: [PATCH 4/4] hwmon (max6639): Remove hwmon init with group

Signed-off-by: Naresh Solanki <[email protected]>
---
drivers/hwmon/max6639.c | 232 ----------------------------------------
1 file changed, 232 deletions(-)

diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
index d9b23202d7a2..c39f32316fe2 100644
--- a/drivers/hwmon/max6639.c
+++ b/drivers/hwmon/max6639.c
@@ -165,233 +165,6 @@ static struct max6639_data *max6639_update_device(struct device *dev)
return ret;
}

-static ssize_t temp_input_show(struct device *dev,
- struct device_attribute *dev_attr, char *buf)
-{
- long temp;
- struct max6639_data *data = max6639_update_device(dev);
- struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
-
- if (IS_ERR(data))
- return PTR_ERR(data);
-
- temp = data->temp[attr->index] * 125;
- return sprintf(buf, "%ld\n", temp);
-}
-
-static ssize_t temp_fault_show(struct device *dev,
- struct device_attribute *dev_attr, char *buf)
-{
- struct max6639_data *data = max6639_update_device(dev);
- struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
-
- if (IS_ERR(data))
- return PTR_ERR(data);
-
- return sprintf(buf, "%d\n", data->temp_fault[attr->index]);
-}
-
-static ssize_t temp_max_show(struct device *dev,
- struct device_attribute *dev_attr, char *buf)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
- struct max6639_data *data = dev_get_drvdata(dev);
-
- return sprintf(buf, "%d\n", (data->temp_therm[attr->index] * 1000));
-}
-
-static ssize_t temp_max_store(struct device *dev,
- struct device_attribute *dev_attr,
- const char *buf, size_t count)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
- struct max6639_data *data = dev_get_drvdata(dev);
- unsigned long val;
- int res;
-
- res = kstrtoul(buf, 10, &val);
- if (res)
- return res;
-
- mutex_lock(&data->update_lock);
- data->temp_therm[attr->index] = TEMP_LIMIT_TO_REG(val);
- regmap_write(data->regmap, MAX6639_REG_THERM_LIMIT(attr->index),
- data->temp_therm[attr->index]);
- mutex_unlock(&data->update_lock);
- return count;
-}
-
-static ssize_t temp_crit_show(struct device *dev,
- struct device_attribute *dev_attr, char *buf)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
- struct max6639_data *data = dev_get_drvdata(dev);
-
- return sprintf(buf, "%d\n", (data->temp_alert[attr->index] * 1000));
-}
-
-static ssize_t temp_crit_store(struct device *dev,
- struct device_attribute *dev_attr,
- const char *buf, size_t count)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
- struct max6639_data *data = dev_get_drvdata(dev);
- unsigned long val;
- int res;
-
- res = kstrtoul(buf, 10, &val);
- if (res)
- return res;
-
- mutex_lock(&data->update_lock);
- data->temp_alert[attr->index] = TEMP_LIMIT_TO_REG(val);
- regmap_write(data->regmap, MAX6639_REG_ALERT_LIMIT(attr->index),
- data->temp_alert[attr->index]);
- mutex_unlock(&data->update_lock);
- return count;
-}
-
-static ssize_t temp_emergency_show(struct device *dev,
- struct device_attribute *dev_attr,
- char *buf)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
- struct max6639_data *data = dev_get_drvdata(dev);
-
- return sprintf(buf, "%d\n", (data->temp_ot[attr->index] * 1000));
-}
-
-static ssize_t temp_emergency_store(struct device *dev,
- struct device_attribute *dev_attr,
- const char *buf, size_t count)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
- struct max6639_data *data = dev_get_drvdata(dev);
- unsigned long val;
- int res;
-
- res = kstrtoul(buf, 10, &val);
- if (res)
- return res;
-
- mutex_lock(&data->update_lock);
- data->temp_ot[attr->index] = TEMP_LIMIT_TO_REG(val);
- regmap_write(data->regmap, MAX6639_REG_OT_LIMIT(attr->index), data->temp_ot[attr->index]);
- mutex_unlock(&data->update_lock);
- return count;
-}
-
-static ssize_t pwm_show(struct device *dev, struct device_attribute *dev_attr,
- char *buf)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
- struct max6639_data *data = dev_get_drvdata(dev);
- struct pwm_state state;
-
- pwm_get_state(data->pwmd[attr->index], &state);
-
- return sprintf(buf, "%d\n", pwm_get_relative_duty_cycle(&state, 255));
-}
-
-static ssize_t pwm_store(struct device *dev,
- struct device_attribute *dev_attr, const char *buf,
- size_t count)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
- struct max6639_data *data = dev_get_drvdata(dev);
- struct pwm_state state;
- unsigned long val;
- int res;
-
- res = kstrtoul(buf, 10, &val);
- if (res)
- return res;
-
- val = clamp_val(val, 0, 255);
-
- pwm_get_state(data->pwmd[attr->index], &state);
- pwm_set_relative_duty_cycle(&state, val, 255);
- pwm_apply_state(data->pwmd[attr->index], &state);
-
- return count;
-}
-
-static ssize_t fan_input_show(struct device *dev,
- struct device_attribute *dev_attr, char *buf)
-{
- struct max6639_data *data = max6639_update_device(dev);
- struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
-
- if (IS_ERR(data))
- return PTR_ERR(data);
-
- return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
- data->rpm_range[attr->index]));
-}
-
-static ssize_t alarm_show(struct device *dev,
- struct device_attribute *dev_attr, char *buf)
-{
- struct max6639_data *data = max6639_update_device(dev);
- struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
-
- if (IS_ERR(data))
- return PTR_ERR(data);
-
- return sprintf(buf, "%d\n", !!(data->status & (1 << attr->index)));
-}
-
-static SENSOR_DEVICE_ATTR_RO(temp1_input, temp_input, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_input, temp_input, 1);
-static SENSOR_DEVICE_ATTR_RO(temp1_fault, temp_fault, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_fault, temp_fault, 1);
-static SENSOR_DEVICE_ATTR_RW(temp1_max, temp_max, 0);
-static SENSOR_DEVICE_ATTR_RW(temp2_max, temp_max, 1);
-static SENSOR_DEVICE_ATTR_RW(temp1_crit, temp_crit, 0);
-static SENSOR_DEVICE_ATTR_RW(temp2_crit, temp_crit, 1);
-static SENSOR_DEVICE_ATTR_RW(temp1_emergency, temp_emergency, 0);
-static SENSOR_DEVICE_ATTR_RW(temp2_emergency, temp_emergency, 1);
-static SENSOR_DEVICE_ATTR_RW(pwm1, pwm, 0);
-static SENSOR_DEVICE_ATTR_RW(pwm2, pwm, 1);
-static SENSOR_DEVICE_ATTR_RO(fan1_input, fan_input, 0);
-static SENSOR_DEVICE_ATTR_RO(fan2_input, fan_input, 1);
-static SENSOR_DEVICE_ATTR_RO(fan1_fault, alarm, 1);
-static SENSOR_DEVICE_ATTR_RO(fan2_fault, alarm, 0);
-static SENSOR_DEVICE_ATTR_RO(temp1_max_alarm, alarm, 3);
-static SENSOR_DEVICE_ATTR_RO(temp2_max_alarm, alarm, 2);
-static SENSOR_DEVICE_ATTR_RO(temp1_crit_alarm, alarm, 7);
-static SENSOR_DEVICE_ATTR_RO(temp2_crit_alarm, alarm, 6);
-static SENSOR_DEVICE_ATTR_RO(temp1_emergency_alarm, alarm, 5);
-static SENSOR_DEVICE_ATTR_RO(temp2_emergency_alarm, alarm, 4);
-
-
-static struct attribute *max6639_attrs[] = {
- &sensor_dev_attr_temp1_input.dev_attr.attr,
- &sensor_dev_attr_temp2_input.dev_attr.attr,
- &sensor_dev_attr_temp1_fault.dev_attr.attr,
- &sensor_dev_attr_temp2_fault.dev_attr.attr,
- &sensor_dev_attr_temp1_max.dev_attr.attr,
- &sensor_dev_attr_temp2_max.dev_attr.attr,
- &sensor_dev_attr_temp1_crit.dev_attr.attr,
- &sensor_dev_attr_temp2_crit.dev_attr.attr,
- &sensor_dev_attr_temp1_emergency.dev_attr.attr,
- &sensor_dev_attr_temp2_emergency.dev_attr.attr,
- &sensor_dev_attr_pwm1.dev_attr.attr,
- &sensor_dev_attr_pwm2.dev_attr.attr,
- &sensor_dev_attr_fan1_input.dev_attr.attr,
- &sensor_dev_attr_fan2_input.dev_attr.attr,
- &sensor_dev_attr_fan1_fault.dev_attr.attr,
- &sensor_dev_attr_fan2_fault.dev_attr.attr,
- &sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
- &sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
- &sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
- &sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
- &sensor_dev_attr_temp1_emergency_alarm.dev_attr.attr,
- &sensor_dev_attr_temp2_emergency_alarm.dev_attr.attr,
- NULL
-};
-ATTRIBUTE_GROUPS(max6639);
-
static int max6639_temp_set_max(struct max6639_data *data, int channel, unsigned long val)
{
int res;
@@ -1091,11 +864,6 @@ static int max6639_probe(struct i2c_client *client)
if (err < 0)
return err;

- if (0)
- data->hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
- data,
- max6639_groups);
-
data->hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
&max6639_chip_info, NULL);
if (IS_ERR(data->hwmon_dev))
--
2.42.0

2024-04-16 21:37:03

by Guenter Roeck

Subject: Re: [PATCH 4/4] hwmon (max6639): Remove hwmon init with group

On Tue, Apr 16, 2024 at 10:47:17PM +0530, Naresh Solanki wrote:
> Signed-off-by: Naresh Solanki <[email protected]>

No description, and I don't really understand why this is
a separate patch.

Guenter

2024-04-16 21:37:21

by Guenter Roeck

Subject: Re: [PATCH 2/4] hwmon: (max6639) : Utilise pwm subsystem

On Tue, Apr 16, 2024 at 10:47:15PM +0530, Naresh Solanki wrote:
> Utilise pwm subsystem for fan pwm handling
>
> Signed-off-by: Naresh Solanki <[email protected]>

That adds a lot of complexity to the driver. I am missing the benefits.
You are supposed to explain why you are making changes, not just that
you are making them.

Why are you making those changes ?

Guenter

2024-04-16 21:38:24

by Guenter Roeck

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

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 <[email protected]>
> ---
> 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

2024-04-17 07:05:23

by Uwe Kleine-König

Subject: Re: [PATCH 2/4] hwmon: (max6639) : Utilise pwm subsystem

Hello,

On Tue, Apr 16, 2024 at 10:47:15PM +0530, Naresh Solanki wrote:
> Utilise pwm subsystem for fan pwm handling
>
> Signed-off-by: Naresh Solanki <[email protected]>
> ---
> drivers/hwmon/Kconfig | 1 +
> drivers/hwmon/max6639.c | 200 ++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 191 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 257ec5360e35..c9cc74f8c807 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1224,6 +1224,7 @@ config SENSORS_MAX6639
> tristate "Maxim MAX6639 sensor chip"
> depends on I2C
> select REGMAP_I2C
> + depends on PWM
> 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 1af93fc53cb5..f37fdd161154 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/pwm.h>
> #include <linux/regmap.h>
>
> /* Addresses to scan */
> @@ -55,6 +56,9 @@ static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
> #define MAX6639_GCONFIG_PWM_FREQ_HI 0x08
>
> #define MAX6639_FAN_CONFIG1_PWM 0x80
> +#define MAX6639_REG_FAN_CONFIG2a_PWM_POL 0x02
> +#define MAX6639_FAN_CONFIG3_FREQ_MASK 0x03
> +#define MAX6639_REG_TARGTDUTY_SLOT 120
>
> #define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED 0x40
>
> @@ -62,6 +66,10 @@ static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
>
> static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
>
> +/* Supported PWM frequency */
> +static const unsigned int freq_table[] = { 20, 33, 50, 100, 5000, 8333, 12500,
> + 25000 };
> +
> #define FAN_FROM_REG(val, rpm_range) ((val) == 0 || (val) == 255 ? \
> 0 : (rpm_ranges[rpm_range] * 30) / (val))
> #define TEMP_LIMIT_TO_REG(val) clamp_val((val) / 1000, 0, 255)
> @@ -93,6 +101,9 @@ struct max6639_data {
>
> /* Optional regulator for FAN supply */
> struct regulator *reg;
> + /* max6639 pwm chip */
> + struct pwm_chip chip;

That won't work with the recent changes to the pwm framework. Please
test your patch on top of next.

> + struct pwm_device *pwmd[MAX6639_NDEV]; /* max6639 has two pwm device */

s/device/devices/

> };
>
> static struct max6639_data *max6639_update_device(struct device *dev)
> @@ -271,8 +282,11 @@ static ssize_t pwm_show(struct device *dev, struct device_attribute *dev_attr,
> {
> struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
> struct max6639_data *data = dev_get_drvdata(dev);
> + struct pwm_state state;
> +
> + pwm_get_state(data->pwmd[attr->index], &state);
>
> - return sprintf(buf, "%d\n", data->pwm[attr->index] * 255 / 120);
> + return sprintf(buf, "%d\n", pwm_get_relative_duty_cycle(&state, 255));
> }
>
> static ssize_t pwm_store(struct device *dev,
> @@ -281,6 +295,7 @@ 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 pwm_state state;
> unsigned long val;
> int res;
>
> @@ -290,10 +305,10 @@ static ssize_t pwm_store(struct device *dev,
>
> val = clamp_val(val, 0, 255);
>
> - mutex_lock(&data->update_lock);
> - data->pwm[attr->index] = (u8)(val * 120 / 255);
> - regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(attr->index), data->pwm[attr->index]);
> - mutex_unlock(&data->update_lock);
> + pwm_get_state(data->pwmd[attr->index], &state);
> + pwm_set_relative_duty_cycle(&state, val, 255);
> + pwm_apply_state(data->pwmd[attr->index], &state);

I'm not a big fan of that pwm_get_state() + modify duty_cycle +
pwm_apply_state(). IMHO it's better to keep a struct pwm_state around in
the consumer and so have complete control in each step.

> +
> return count;
> }
>
> @@ -373,6 +388,158 @@ static struct attribute *max6639_attrs[] = {
> };
> ATTRIBUTE_GROUPS(max6639);
>
> +static struct max6639_data *to_max6639_pwm(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct max6639_data, chip);
> +}
> +
> +static int max6639_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct max6639_data *data = to_max6639_pwm(chip);
> + int value, i = pwm->hwpwm, x, err;
> + unsigned int freq;
> +
> + mutex_lock(&data->update_lock);
> +
> + err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG1(i), &value);
> + if (err < 0)
> + goto abort;

I don't know if the hwmon maintainers like that, but taking a mutex for
the whole function's runtime can also be expressed by:

guard(mutex)(&data->update_lock);

then all the goto abort below can be replaced by return err.

> +
> + if (value & MAX6639_FAN_CONFIG1_PWM) {
> + state->enabled = true;
> +
> + /* Determine frequency from respective registers */
> + err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG3(i), &value);
> + if (err < 0)
> + goto abort;
> + x = value & MAX6639_FAN_CONFIG3_FREQ_MASK;
> +
> + err = regmap_read(data->regmap, MAX6639_REG_GCONFIG, &value);
> + if (err < 0)
> + goto abort;
> + if (value & MAX6639_GCONFIG_PWM_FREQ_HI)
> + x |= 0x4;
> + x &= 0x7;
> + freq = freq_table[x];
> +
> + state->period = DIV_ROUND_UP(NSEC_PER_SEC, freq);
> +
> + err = regmap_read(data->regmap, MAX6639_REG_TARGTDUTY(i), &value);
> + if (err < 0)
> + goto abort;
> + /* max6639 supports 120 slots only */
> + state->duty_cycle = mul_u64_u32_div(state->period, value, 120);

MAX6639_REG_TARGTDUTY_SLOT instead of 120 here.

> + err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), &value);
> + if (err < 0)
> + goto abort;
> + value &= MAX6639_REG_FAN_CONFIG2a_PWM_POL;
> + state->polarity = (value != 0);

Please don't hardcode PWM_POLARITY_* values here. Please use:

state->polarity = (value != 0) ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;

(or an if statement).

> + } else {
> + state->enabled = false;
> + }
> +
> +abort:
> + mutex_unlock(&data->update_lock);
> + return value;
> +}
> +
> +static int max6639_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct max6639_data *data = to_max6639_pwm(chip);
> + int value, i = pwm->hwpwm, x, err;
> + unsigned int freq;
> + struct pwm_state cstate;
> +
> + cstate = pwm->state;
> +
> + mutex_lock(&data->update_lock);
> +
> + if (state->period != cstate.period) {
> + /* Configure frequency */
> + freq = DIV_ROUND_UP_ULL(NSEC_PER_SEC, state->period);
> +
> + /* Chip supports limited number of frequency */
> + for (x = 0; x < sizeof(freq_table); x++)
> + if (freq <= freq_table[x])
> + break;

If you store the periods instead of the frequencies in the global table
you can save several divisions and so simplify the code.

> + err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG3(i), &value);
> + if (err < 0)
> + goto abort;
> +
> + value &= ~MAX6639_FAN_CONFIG3_FREQ_MASK;
> + value |= (x & MAX6639_FAN_CONFIG3_FREQ_MASK);

You're using implicitly that there is no shift involved in
MAX6639_FAN_CONFIG3_FREQ_MASK. Better use FIELD_PREP().

Also MAX6639_FAN_CONFIG3_FREQ_MASK is 3, but x ranges in [0 ... 7].
That's a bug, isn't it?

> + err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG3(i), value);
> + if (err < 0)
> + goto abort;

How does the hardware behave here? Does it emit the new period already
with the old duty cycle and polarity setting? Please document that,
ideally in a paragraph captured "Limitations:" and a format matching
what several other pwm drivers do, to make that easily greppable.

> + err = regmap_read(data->regmap, MAX6639_REG_GCONFIG, &value);
> + if (err < 0)
> + goto abort;
> +
> + if (x >> 2)
> + value &= ~MAX6639_GCONFIG_PWM_FREQ_HI;
> + else
> + value |= MAX6639_GCONFIG_PWM_FREQ_HI;
> + err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, value);
> + if (err < 0)
> + goto abort;
> + }
> +
> + /* Configure dutycycle */
> + if (state->duty_cycle != cstate.duty_cycle ||
> + state->period != cstate.period) {
> + value = DIV_ROUND_DOWN_ULL(state->duty_cycle * MAX6639_REG_TARGTDUTY_SLOT,
> + state->period);

The multiplication might overflow, better use mul_u64_u64_div_u64()
here. Also you're loosing precision here because the real period might
be lower than state->period. Consider:

state->period = 9999999 [ns]
state->duty_cycle = 123456 [ns]

With the possible frequencies you have to pick freq = 5000 [Hz] giving
you a period of 200000 ns. You're then configuring 123456 * 120 / 9999999
= 1 as duty_cycle count giving you 1666 ns as duty cycle. However you're
supposed to configure 74 giving 123333 ns.

> + err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), value);
> + if (err < 0)
> + goto abort;
> + }
> +
> + /* Configure polarity */
> + if (state->polarity != cstate.polarity) {
> + err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), &value);
> + if (err < 0)
> + goto abort;
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + value |= MAX6639_REG_FAN_CONFIG2a_PWM_POL;
> + else
> + value &= ~MAX6639_REG_FAN_CONFIG2a_PWM_POL;
> + err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), value);
> + if (err < 0)
> + goto abort;
> + }
> +
> + if (state->enabled != cstate.enabled) {
> + err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG1(i), &value);
> + if (err < 0)
> + goto abort;
> + if (state->enabled)
> + value |= MAX6639_FAN_CONFIG1_PWM;
> + else
> + value &= ~MAX6639_FAN_CONFIG1_PWM;
> +
> + err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG1(i), value);
> + if (err < 0)
> + goto abort;
> + }
> + value = 0;
> +
> +abort:
> + mutex_unlock(&data->update_lock);
> +
> + return err;
> +}
> +
> +static const struct pwm_ops max6639_pwm_ops = {
> + .apply = max6639_pwm_apply,
> + .get_state = max6639_pwm_get_state,
> +};
> +
> /*
> * returns respective index in rpm_ranges table
> * 1 by default on invalid range
> @@ -396,6 +563,7 @@ static int max6639_init_client(struct i2c_client *client,
> dev_get_platdata(&client->dev);
> int i;
> int rpm_range = 1; /* default: 4000 RPM */
> + struct pwm_state state;

state could have a more local scope.

> int err;
>
> /* Reset chip to default values, see below for GCONFIG setup */
> @@ -459,11 +627,15 @@ static int max6639_init_client(struct i2c_client *client,
> if (err)
> goto exit;
>
> - /* PWM 120/120 (i.e. 100%) */
> - data->pwm[i] = 120;
> - err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), data->pwm[i]);
> - if (err)
> - goto exit;
> + dev_dbg(&client->dev, "Using chip default PWM");
> + data->pwmd[i] = pwm_request_from_chip(&data->chip, i, NULL);
> + if (IS_ERR(data->pwmd[i]))
> + return PTR_ERR(data->pwmd[i]);
> + pwm_get_state(data->pwmd[i], &state);

What I said above about the pwm_get_state() + modify + pwm_apply_state()
approach applies still more to the initial configuration. Here you're
keeping some HW state set previously by an earlier incarnation of the
driver, or the boot loader or the reset default value, which might
involve state.enabled = false.

> + state.period = DIV_ROUND_UP(NSEC_PER_SEC, 25000);
> + state.polarity = PWM_POLARITY_NORMAL;
> + pwm_set_relative_duty_cycle(&state, 0, 255);

This involves a division. Using

state.duty_cycle = 0;

would be more efficient here.

> + pwm_apply_state(data->pwmd[i], &state);
> }
> /* Start monitoring */
> err = regmap_write(data->regmap, MAX6639_REG_GCONFIG,
> @@ -540,6 +712,14 @@ static int max6639_probe(struct i2c_client *client)
> PTR_ERR(data->regmap),
> "regmap initialization failed\n");
>
> + /* Add PWM controller of max6639 */
> + data->chip.dev = dev;
> + data->chip.ops = &max6639_pwm_ops;
> + data->chip.npwm = MAX6639_NDEV;
> + err = devm_pwmchip_add(dev, &data->chip);
> + if (err < 0)
> + return dev_err_probe(dev, err, "failed to add PWM chip\n");
> +
> data->reg = devm_regulator_get_optional(dev, "fan");
> if (IS_ERR(data->reg)) {
> if (PTR_ERR(data->reg) != -ENODEV)

Do I understand right that the driver itself is expected to be the only
consumer of this PWM? I'm not sure that using the PWM stuff is a useful
improvement then. You're just gaining some debug possibilies for the
overhead. Probably it's easier to just inspect the device's registers
directly to debug and stick to the old abstraction without a struct
pwm_chip?!

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

2024-04-22 10:36:40

by Naresh Solanki

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

Hi Guenter,

On Wed, 17 Apr 2024 at 02:55, Guenter Roeck <[email protected]> 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 <[email protected]>
> > ---
> > 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

2024-04-22 10:44:25

by Naresh Solanki

Subject: Re: [PATCH 2/4] hwmon: (max6639) : Utilise pwm subsystem

Hi Guenter,

On Wed, 17 Apr 2024 at 02:52, Guenter Roeck <[email protected]> wrote:
>
> On Tue, Apr 16, 2024 at 10:47:15PM +0530, Naresh Solanki wrote:
> > Utilise pwm subsystem for fan pwm handling
> >
> > Signed-off-by: Naresh Solanki <[email protected]>
>
> That adds a lot of complexity to the driver. I am missing the benefits.
> You are supposed to explain why you are making changes, not just that
> you are making them.
>
> Why are you making those changes ?
Sure.
This is to align with fan-common.yml wherein chip pwm is exposed.
I'll update commit message

Regards,
Naresh
>
> Guenter

2024-04-22 12:38:55

by Guenter Roeck

Subject: Re: [PATCH 2/4] hwmon: (max6639) : Utilise pwm subsystem

On 4/22/24 03:39, Naresh Solanki wrote:
> Hi Guenter,
>
> On Wed, 17 Apr 2024 at 02:52, Guenter Roeck <[email protected]> wrote:
>>
>> On Tue, Apr 16, 2024 at 10:47:15PM +0530, Naresh Solanki wrote:
>>> Utilise pwm subsystem for fan pwm handling
>>>
>>> Signed-off-by: Naresh Solanki <[email protected]>
>>
>> That adds a lot of complexity to the driver. I am missing the benefits.
>> You are supposed to explain why you are making changes, not just that
>> you are making them.
>>
>> Why are you making those changes ?
> Sure.
> This is to align with fan-common.yml wherein chip pwm is exposed.
> I'll update commit message
>

Adding lots of complexity to a driver just to have it match a yaml file ?
I'll want to see a use case. Explain why you need the pwm exposed.
"because the yaml file demands it" is not a use case.

Guenter

2024-04-22 16:02:52

by Guenter Roeck

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

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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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

2024-04-25 10:17:26

by Naresh Solanki

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

Hi Guenter,


On Mon, 22 Apr 2024 at 21:32, Guenter Roeck <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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

2024-04-28 17:19:01

by Guenter Roeck

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

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

2024-04-29 08:19:50

by Naresh Solanki

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

Hi Guenter,


On Sun, 28 Apr 2024 at 22:48, Guenter Roeck <[email protected]> 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
>

2024-04-29 13:59:06

by Guenter Roeck

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

On 4/29/24 01:19, Naresh Solanki wrote:
> Hi Guenter,
>
>
> On Sun, 28 Apr 2024 at 22:48, Guenter Roeck <[email protected]> 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

2024-05-06 10:06:01

by Naresh Solanki

Subject: Re: [PATCH 2/4] hwmon: (max6639) : Utilise pwm subsystem

+Rob Herring

Hi Guenter,


On Mon, 22 Apr 2024 at 18:07, Guenter Roeck <[email protected]> wrote:
>
> On 4/22/24 03:39, Naresh Solanki wrote:
> > Hi Guenter,
> >
> > On Wed, 17 Apr 2024 at 02:52, Guenter Roeck <[email protected]> wrote:
> >>
> >> On Tue, Apr 16, 2024 at 10:47:15PM +0530, Naresh Solanki wrote:
> >>> Utilise pwm subsystem for fan pwm handling
> >>>
> >>> Signed-off-by: Naresh Solanki <[email protected]>
> >>
> >> That adds a lot of complexity to the driver. I am missing the benefits.
> >> You are supposed to explain why you are making changes, not just that
> >> you are making them.
> >>
> >> Why are you making those changes ?
> > Sure.
> > This is to align with fan-common.yml wherein chip pwm is exposed.
> > I'll update commit message
> >
>
> Adding lots of complexity to a driver just to have it match a yaml file ?
> I'll want to see a use case. Explain why you need the pwm exposed.
> "because the yaml file demands it" is not a use case.
The idea behind this was that this approach provides flexibility with
hardware routing i.e., PWM0 might be connected to Fan1 & vise
versa instead of assuming 1:1 mapping.

Regards,
Naresh
>
> Guenter
>

2024-05-06 13:49:49

by Guenter Roeck

Subject: Re: [PATCH 2/4] hwmon: (max6639) : Utilise pwm subsystem

On Mon, May 06, 2024 at 03:35:40PM +0530, Naresh Solanki wrote:
> +Rob Herring
>
> Hi Guenter,
>
>
> On Mon, 22 Apr 2024 at 18:07, Guenter Roeck <[email protected]> wrote:
> >
> > On 4/22/24 03:39, Naresh Solanki wrote:
> > > Hi Guenter,
> > >
> > > On Wed, 17 Apr 2024 at 02:52, Guenter Roeck <[email protected]> wrote:
> > >>
> > >> On Tue, Apr 16, 2024 at 10:47:15PM +0530, Naresh Solanki wrote:
> > >>> Utilise pwm subsystem for fan pwm handling
> > >>>
> > >>> Signed-off-by: Naresh Solanki <[email protected]>
> > >>
> > >> That adds a lot of complexity to the driver. I am missing the benefits.
> > >> You are supposed to explain why you are making changes, not just that
> > >> you are making them.
> > >>
> > >> Why are you making those changes ?
> > > Sure.
> > > This is to align with fan-common.yml wherein chip pwm is exposed.
> > > I'll update commit message
> > >
> >
> > Adding lots of complexity to a driver just to have it match a yaml file ?
> > I'll want to see a use case. Explain why you need the pwm exposed.
> > "because the yaml file demands it" is not a use case.
> The idea behind this was that this approach provides flexibility with
> hardware routing i.e., PWM0 might be connected to Fan1 & vise
> versa instead of assuming 1:1 mapping.
>

The chip does not support such a configuration. Any hardware implementing
this would make automatic fan control using the chip's capabilities
impossible. Also, userspace fan control does not rely on the pwm subsystem.

This would make sense if someone was to use the chip as generic pwm
controller, which would be very unlikely. A "might be" is not sufficient
to warrant such an invasive and (in terms of code size) expensive change.

Thanks,
Guenter