* [PATCH v1 1/5] hwmon: (lm75) Add kind field to struct lm75_data
2019-07-09 9:50 [PATCH v1 0/5] Help with lm75.c changes Iker Perez
@ 2019-07-09 9:50 ` Iker Perez
2019-07-09 13:20 ` Guenter Roeck
2019-07-09 9:50 ` [PATCH v1 2/5] hwmon: (lm75) Include hwmon_chip in the permitted types to be writen Iker Perez
` (4 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Iker Perez @ 2019-07-09 9:50 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, linux-hwmon; +Cc: Iker Perez del Palomar Sustatxa
From: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
The purpose of this change is to store the kind of device the
kernel is working with.
Signed-off-by: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
---
drivers/hwmon/lm75.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 3fb9c0a2d6d0..0209e0719784 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -68,6 +68,7 @@ struct lm75_data {
u8 resolution; /* In bits, between 9 and 16 */
u8 resolution_limits;
unsigned int sample_time; /* In ms */
+ enum lm75_type kind;
};
/*-----------------------------------------------------------------------*/
@@ -237,15 +238,16 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id)
struct device *dev = &client->dev;
struct device *hwmon_dev;
struct lm75_data *data;
+ struct lm75_data device_data;
int status, err;
u8 set_mask, clr_mask;
int new;
- enum lm75_type kind;
+ data = &device_data;
if (client->dev.of_node)
- kind = (enum lm75_type)of_device_get_match_data(&client->dev);
+ data->kind = (enum lm75_type)of_device_get_match_data(&client->dev);
else
- kind = id->driver_data;
+ data->kind = id->driver_data;
if (!i2c_check_functionality(client->adapter,
I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
@@ -267,7 +269,7 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id)
set_mask = 0;
clr_mask = LM75_SHUTDOWN; /* continuous conversions */
- switch (kind) {
+ switch (data->kind) {
case adt75:
clr_mask |= 1 << 5; /* not one-shot mode */
data->resolution = 12;
--
2.11.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/5] hwmon: (lm75) Add kind field to struct lm75_data
2019-07-09 9:50 ` [PATCH v1 1/5] hwmon: (lm75) Add kind field to struct lm75_data Iker Perez
@ 2019-07-09 13:20 ` Guenter Roeck
0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2019-07-09 13:20 UTC (permalink / raw)
To: Iker Perez, Jean Delvare, linux-hwmon
On 7/9/19 2:50 AM, Iker Perez wrote:
> From: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
>
> The purpose of this change is to store the kind of device the
> kernel is working with.
>
> Signed-off-by: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
> ---
> drivers/hwmon/lm75.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index 3fb9c0a2d6d0..0209e0719784 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -68,6 +68,7 @@ struct lm75_data {
> u8 resolution; /* In bits, between 9 and 16 */
> u8 resolution_limits;
> unsigned int sample_time; /* In ms */
> + enum lm75_type kind;
> };
>
> /*-----------------------------------------------------------------------*/
> @@ -237,15 +238,16 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id)
> struct device *dev = &client->dev;
> struct device *hwmon_dev;
> struct lm75_data *data;
> + struct lm75_data device_data;
> int status, err;
> u8 set_mask, clr_mask;
> int new;
> - enum lm75_type kind;
>
> + data = &device_data;
This makes no sense.
> if (client->dev.of_node)
> - kind = (enum lm75_type)of_device_get_match_data(&client->dev);
> + data->kind = (enum lm75_type)of_device_get_match_data(&client->dev);
> else
> - kind = id->driver_data;
> + data->kind = id->driver_data;
>
... because data is overwritten a couple of lines below when it is allocated.
You would do something like
data->kind = kind;
after data was allocated. Looking at the above code, though, I really wonder if
and how you tested it.
Guenter
> if (!i2c_check_functionality(client->adapter,
> I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
> @@ -267,7 +269,7 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id)
> set_mask = 0;
> clr_mask = LM75_SHUTDOWN; /* continuous conversions */
>
> - switch (kind) {
> + switch (data->kind) {
> case adt75:
> clr_mask |= 1 << 5; /* not one-shot mode */
> data->resolution = 12;
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1 2/5] hwmon: (lm75) Include hwmon_chip in the permitted types to be writen
2019-07-09 9:50 [PATCH v1 0/5] Help with lm75.c changes Iker Perez
2019-07-09 9:50 ` [PATCH v1 1/5] hwmon: (lm75) Add kind field to struct lm75_data Iker Perez
@ 2019-07-09 9:50 ` Iker Perez
2019-07-09 13:26 ` Guenter Roeck
2019-07-09 9:50 ` [PATCH v1 3/5] hwmon: (lm75) Give write permission to hwmon_chip_update_interval Iker Perez
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Iker Perez @ 2019-07-09 9:50 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, linux-hwmon; +Cc: Iker Perez del Palomar Sustatxa
From: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
hwmon_chip needs to be allowed to be written because tmp75b's sample time
can be configured. Allowing hwmon_chip to be written will allow to
configure the update_interval from sysfs.
Signed-off-by: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
---
drivers/hwmon/lm75.c | 62 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 38 insertions(+), 24 deletions(-)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 0209e0719784..80a11c33db77 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -128,34 +128,48 @@ static int lm75_write(struct device *dev, enum hwmon_sensor_types type,
u8 resolution;
int reg;
- if (type != hwmon_temp)
- return -EINVAL;
-
- switch (attr) {
- case hwmon_temp_max:
- reg = LM75_REG_MAX;
- break;
- case hwmon_temp_max_hyst:
- reg = LM75_REG_HYST;
- break;
- default:
- return -EINVAL;
+ switch (type) {
+ case hwmon_chip:
+ switch (attr) {
+ case hwmon_chip_update_interval:
+ if (data->kind == tmp75b)
+ pr_info("Iker inside write\n");
+ else
+ return -EINVAL;
+ break;
+ default:
+ return -EINVAL;
+ }
+ case hwmon_temp:
+ switch (attr) {
+ case hwmon_temp_max:
+ reg = LM75_REG_MAX;
+ break;
+ case hwmon_temp_max_hyst:
+ reg = LM75_REG_HYST;
+ break;
+ default:
+ return -EINVAL;
}
- /*
- * Resolution of limit registers is assumed to be the same as the
- * temperature input register resolution unless given explicitly.
- */
- if (data->resolution_limits)
- resolution = data->resolution_limits;
- else
- resolution = data->resolution;
+ /*
+ * Resolution of limit registers is assumed to be the same as the
+ * temperature input register resolution unless given explicitly.
+ */
+ if (data->resolution_limits)
+ resolution = data->resolution_limits;
+ else
+ resolution = data->resolution;
- temp = clamp_val(temp, LM75_TEMP_MIN, LM75_TEMP_MAX);
- temp = DIV_ROUND_CLOSEST(temp << (resolution - 8),
- 1000) << (16 - resolution);
+ temp = clamp_val(temp, LM75_TEMP_MIN, LM75_TEMP_MAX);
+ temp = DIV_ROUND_CLOSEST(temp << (resolution - 8),
+ 1000) << (16 - resolution);
- return regmap_write(data->regmap, reg, temp);
+ return regmap_write(data->regmap, reg, temp);
+ default:
+ return -EINVAL;
+ }
+ return 0;
}
static umode_t lm75_is_visible(const void *data, enum hwmon_sensor_types type,
--
2.11.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1 2/5] hwmon: (lm75) Include hwmon_chip in the permitted types to be writen
2019-07-09 9:50 ` [PATCH v1 2/5] hwmon: (lm75) Include hwmon_chip in the permitted types to be writen Iker Perez
@ 2019-07-09 13:26 ` Guenter Roeck
0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2019-07-09 13:26 UTC (permalink / raw)
To: Iker Perez, Jean Delvare, linux-hwmon
On 7/9/19 2:50 AM, Iker Perez wrote:
> From: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
>
> hwmon_chip needs to be allowed to be written because tmp75b's sample time
> can be configured. Allowing hwmon_chip to be written will allow to
> configure the update_interval from sysfs.
>
You'll want to have separate functions for the different sensor types,
and not fold it all into one. lm75_write() should be something like
static int lm75_write(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long val)
{
switch(type) {
case hwmon_temp:
return lm75_write_temp(dev, attr, channel, val);
case hwmon_chip:
return lm75_write_chip(dev, attr, val);
default:
return -EINVAL;
}
}
> Signed-off-by: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
> ---
> drivers/hwmon/lm75.c | 62 ++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 38 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index 0209e0719784..80a11c33db77 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -128,34 +128,48 @@ static int lm75_write(struct device *dev, enum hwmon_sensor_types type,
> u8 resolution;
> int reg;
>
> - if (type != hwmon_temp)
> - return -EINVAL;
> -
> - switch (attr) {
> - case hwmon_temp_max:
> - reg = LM75_REG_MAX;
> - break;
> - case hwmon_temp_max_hyst:
> - reg = LM75_REG_HYST;
> - break;
> - default:
> - return -EINVAL;
> + switch (type) {
> + case hwmon_chip:
> + switch (attr) {
> + case hwmon_chip_update_interval:
> + if (data->kind == tmp75b)
> + pr_info("Iker inside write\n");
> + else
> + return -EINVAL;
> + break;
> + default:
> + return -EINVAL;
> + }
> + case hwmon_temp:
> + switch (attr) {
> + case hwmon_temp_max:
> + reg = LM75_REG_MAX;
> + break;
> + case hwmon_temp_max_hyst:
> + reg = LM75_REG_HYST;
> + break;
> + default:
> + return -EINVAL;
> }
Please watch out for indentation. Running checkpatch over your patches
would be highly recommended.
Guenter
>
> - /*
> - * Resolution of limit registers is assumed to be the same as the
> - * temperature input register resolution unless given explicitly.
> - */
> - if (data->resolution_limits)
> - resolution = data->resolution_limits;
> - else
> - resolution = data->resolution;
> + /*
> + * Resolution of limit registers is assumed to be the same as the
> + * temperature input register resolution unless given explicitly.
> + */
> + if (data->resolution_limits)
> + resolution = data->resolution_limits;
> + else
> + resolution = data->resolution;
>
> - temp = clamp_val(temp, LM75_TEMP_MIN, LM75_TEMP_MAX);
> - temp = DIV_ROUND_CLOSEST(temp << (resolution - 8),
> - 1000) << (16 - resolution);
> + temp = clamp_val(temp, LM75_TEMP_MIN, LM75_TEMP_MAX);
> + temp = DIV_ROUND_CLOSEST(temp << (resolution - 8),
> + 1000) << (16 - resolution);
>
> - return regmap_write(data->regmap, reg, temp);
> + return regmap_write(data->regmap, reg, temp);
> + default:
> + return -EINVAL;
> + }
> + return 0;
> }
>
> static umode_t lm75_is_visible(const void *data, enum hwmon_sensor_types type,
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1 3/5] hwmon: (lm75) Give write permission to hwmon_chip_update_interval
2019-07-09 9:50 [PATCH v1 0/5] Help with lm75.c changes Iker Perez
2019-07-09 9:50 ` [PATCH v1 1/5] hwmon: (lm75) Add kind field to struct lm75_data Iker Perez
2019-07-09 9:50 ` [PATCH v1 2/5] hwmon: (lm75) Include hwmon_chip in the permitted types to be writen Iker Perez
@ 2019-07-09 9:50 ` Iker Perez
2019-07-09 9:50 ` [PATCH v1 4/5] hwmon: (lm75) Create function from code to write into registers Iker Perez
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Iker Perez @ 2019-07-09 9:50 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, linux-hwmon; +Cc: Iker Perez del Palomar Sustatxa
From: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
This is needed in order to later be able to write into update_time.
Signed-off-by: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
---
drivers/hwmon/lm75.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 80a11c33db77..1d4d060bd695 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -179,7 +179,7 @@ static umode_t lm75_is_visible(const void *data, enum hwmon_sensor_types type,
case hwmon_chip:
switch (attr) {
case hwmon_chip_update_interval:
- return 0444;
+ return 0644;
}
break;
case hwmon_temp:
--
2.11.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 4/5] hwmon: (lm75) Create function from code to write into registers
2019-07-09 9:50 [PATCH v1 0/5] Help with lm75.c changes Iker Perez
` (2 preceding siblings ...)
2019-07-09 9:50 ` [PATCH v1 3/5] hwmon: (lm75) Give write permission to hwmon_chip_update_interval Iker Perez
@ 2019-07-09 9:50 ` Iker Perez
2019-07-09 13:39 ` Guenter Roeck
2019-07-09 9:50 ` [PATCH v1 5/5] First approach to sample time writing method Iker Perez
2019-07-09 13:43 ` [PATCH v1 0/5] Help with lm75.c changes Guenter Roeck
5 siblings, 1 reply; 12+ messages in thread
From: Iker Perez @ 2019-07-09 9:50 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, linux-hwmon; +Cc: Iker Perez del Palomar Sustatxa
From: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
This function will be needed later to configure update_interval
Signed-off-by: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
---
drivers/hwmon/lm75.c | 63 ++++++++++++++++++++++++++++++----------------------
1 file changed, 36 insertions(+), 27 deletions(-)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 1d4d060bd695..5ba7277dac69 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -78,6 +78,40 @@ static inline long lm75_reg_to_mc(s16 temp, u8 resolution)
return ((temp >> (16 - resolution)) * 1000) >> (resolution - 8);
}
+static void lm75_remove(void *data)
+{
+ struct lm75_data *lm75 = data;
+ struct i2c_client *client = lm75->client;
+
+ i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf);
+}
+static int configure_reg(u8 set_mask, u8 clr_mask, struct lm75_data *data,
+ struct i2c_client *client)
+{
+ int status, err, new;
+ struct device *dev = &client->dev;
+
+ /* configure as specified */
+ status = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
+ if (status < 0) {
+ dev_dbg(dev, "Can't read config? %d\n", status);
+ return status;
+ }
+ data->orig_conf = status;
+ new = status & ~clr_mask;
+ new |= set_mask;
+ if (status != new)
+ i2c_smbus_write_byte_data(client, LM75_REG_CONF, new);
+
+ err = devm_add_action_or_reset(dev, lm75_remove, data);
+ if (err)
+ return err;
+
+ dev_dbg(dev, "Config %02x\n", new);
+
+ return 0;
+}
+
static int lm75_read(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long *val)
{
@@ -238,14 +272,6 @@ static const struct regmap_config lm75_regmap_config = {
.use_single_write = true,
};
-static void lm75_remove(void *data)
-{
- struct lm75_data *lm75 = data;
- struct i2c_client *client = lm75->client;
-
- i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf);
-}
-
static int
lm75_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
@@ -253,9 +279,8 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id)
struct device *hwmon_dev;
struct lm75_data *data;
struct lm75_data device_data;
- int status, err;
+ int status;
u8 set_mask, clr_mask;
- int new;
data = &device_data;
if (client->dev.of_node)
@@ -370,23 +395,7 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id)
break;
}
- /* configure as specified */
- status = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
- if (status < 0) {
- dev_dbg(dev, "Can't read config? %d\n", status);
- return status;
- }
- data->orig_conf = status;
- new = status & ~clr_mask;
- new |= set_mask;
- if (status != new)
- i2c_smbus_write_byte_data(client, LM75_REG_CONF, new);
-
- err = devm_add_action_or_reset(dev, lm75_remove, data);
- if (err)
- return err;
-
- dev_dbg(dev, "Config %02x\n", new);
+ status = configure_reg(set_mask, clr_mask, data, client);
hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
data, &lm75_chip_info,
--
2.11.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1 4/5] hwmon: (lm75) Create function from code to write into registers
2019-07-09 9:50 ` [PATCH v1 4/5] hwmon: (lm75) Create function from code to write into registers Iker Perez
@ 2019-07-09 13:39 ` Guenter Roeck
0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2019-07-09 13:39 UTC (permalink / raw)
To: Iker Perez, Jean Delvare, linux-hwmon
On 7/9/19 2:50 AM, Iker Perez wrote:
> From: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
>
> This function will be needed later to configure update_interval
>
> Signed-off-by: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
> ---
> drivers/hwmon/lm75.c | 63 ++++++++++++++++++++++++++++++----------------------
> 1 file changed, 36 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index 1d4d060bd695..5ba7277dac69 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -78,6 +78,40 @@ static inline long lm75_reg_to_mc(s16 temp, u8 resolution)
> return ((temp >> (16 - resolution)) * 1000) >> (resolution - 8);
> }
>
> +static void lm75_remove(void *data)
> +{
> + struct lm75_data *lm75 = data;
> + struct i2c_client *client = lm75->client;
> +
> + i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf);
> +}
> +static int configure_reg(u8 set_mask, u8 clr_mask, struct lm75_data *data,
> + struct i2c_client *client)
> +{
> + int status, err, new;
> + struct device *dev = &client->dev;
> +
> + /* configure as specified */
> + status = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
> + if (status < 0) {
> + dev_dbg(dev, "Can't read config? %d\n", status);
> + return status;
> + }
> + data->orig_conf = status;
Overwriting the "original" configuration each time the update time is changed
is really a bad idea.
You'll want to cache the current configuration register value, and not
re-read it each time the configuration is updated.
> + new = status & ~clr_mask;
> + new |= set_mask;
> + if (status != new)
> + i2c_smbus_write_byte_data(client, LM75_REG_CONF, new);
> +
> + err = devm_add_action_or_reset(dev, lm75_remove, data);
> + if (err)
> + return err;
> +
The function is a good idea, but you can't use devm_add_action_or_reset() here.
That will have to remain in the probe function. Otherwise an action will be added
each time the resolution/update time is changed.
> + dev_dbg(dev, "Config %02x\n", new);
> +
> + return 0;
> +}
> +
> static int lm75_read(struct device *dev, enum hwmon_sensor_types type,
> u32 attr, int channel, long *val)
> {
> @@ -238,14 +272,6 @@ static const struct regmap_config lm75_regmap_config = {
> .use_single_write = true,
> };
>
> -static void lm75_remove(void *data)
> -{
> - struct lm75_data *lm75 = data;
> - struct i2c_client *client = lm75->client;
> -
> - i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf);
> -}
> -
> static int
> lm75_probe(struct i2c_client *client, const struct i2c_device_id *id)
> {
> @@ -253,9 +279,8 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id)
> struct device *hwmon_dev;
> struct lm75_data *data;
> struct lm75_data device_data;
> - int status, err;
> + int status;
> u8 set_mask, clr_mask;
> - int new;
>
> data = &device_data;
> if (client->dev.of_node)
> @@ -370,23 +395,7 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id)
> break;
> }
>
> - /* configure as specified */
> - status = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
> - if (status < 0) {
> - dev_dbg(dev, "Can't read config? %d\n", status);
> - return status;
> - }
> - data->orig_conf = status;
> - new = status & ~clr_mask;
> - new |= set_mask;
> - if (status != new)
> - i2c_smbus_write_byte_data(client, LM75_REG_CONF, new);
> -
> - err = devm_add_action_or_reset(dev, lm75_remove, data);
> - if (err)
> - return err;
> -
> - dev_dbg(dev, "Config %02x\n", new);
> + status = configure_reg(set_mask, clr_mask, data, client);
>
> hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> data, &lm75_chip_info,
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1 5/5] First approach to sample time writing method
2019-07-09 9:50 [PATCH v1 0/5] Help with lm75.c changes Iker Perez
` (3 preceding siblings ...)
2019-07-09 9:50 ` [PATCH v1 4/5] hwmon: (lm75) Create function from code to write into registers Iker Perez
@ 2019-07-09 9:50 ` Iker Perez
2019-07-09 13:33 ` Guenter Roeck
2019-07-09 13:43 ` [PATCH v1 0/5] Help with lm75.c changes Guenter Roeck
5 siblings, 1 reply; 12+ messages in thread
From: Iker Perez @ 2019-07-09 9:50 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, linux-hwmon; +Cc: Iker Perez del Palomar Sustatxa
From: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
Switch between the possible update_time values and write into the
configuration register the selected value.
Signed-off-by: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
---
drivers/hwmon/lm75.c | 43 ++++++++++++++++++++++++++++++++++---------
1 file changed, 34 insertions(+), 9 deletions(-)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 5ba7277dac69..9d48a85fd3e5 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -156,21 +156,46 @@ static int lm75_read(struct device *dev, enum hwmon_sensor_types type,
}
static int lm75_write(struct device *dev, enum hwmon_sensor_types type,
- u32 attr, int channel, long temp)
+ u32 attr, int channel, long val)
{
struct lm75_data *data = dev_get_drvdata(dev);
- u8 resolution;
- int reg;
+ u8 resolution, set_mask, clr_mask;
+ int reg, status;
+
+ // This are provisional changes, to be improved in case this approach works
+ set_mask = 0;
+ clr_mask = LM75_SHUTDOWN;
switch (type) {
case hwmon_chip:
switch (attr) {
case hwmon_chip_update_interval:
- if (data->kind == tmp75b)
- pr_info("Iker inside write\n");
+ if (data->kind == tmp75b) {
+ clr_mask |= 1 << 15 | 0x3 << 13;
+ switch (val) {
+ case (27):
+ set_mask |= 0x3 << 13;
+ data->sample_time = MSEC_PER_SEC / 37;
+ break;
+ case (55):
+ set_mask |= 0x2 << 13;
+ data->sample_time = MSEC_PER_SEC / 18;
+ break;
+ case (111):
+ set_mask |= 0x1 << 13;
+ data->sample_time = MSEC_PER_SEC / 9;
+ break;
+ case (250):
+ set_mask |= 0x0 << 13;
+ data->sample_time = MSEC_PER_SEC / 4;
+ break;
+ default:
+ return -EINVAL;
+ status = configure_reg(set_mask, clr_mask, data, client);
+ }
+ }
else
return -EINVAL;
- break;
default:
return -EINVAL;
}
@@ -195,11 +220,11 @@ static int lm75_write(struct device *dev, enum hwmon_sensor_types type,
else
resolution = data->resolution;
- temp = clamp_val(temp, LM75_TEMP_MIN, LM75_TEMP_MAX);
- temp = DIV_ROUND_CLOSEST(temp << (resolution - 8),
+ val = clamp_val(val, LM75_TEMP_MIN, LM75_TEMP_MAX);
+ val = DIV_ROUND_CLOSEST(val << (resolution - 8),
1000) << (16 - resolution);
- return regmap_write(data->regmap, reg, temp);
+ return regmap_write(data->regmap, reg, val);
default:
return -EINVAL;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1 5/5] First approach to sample time writing method
2019-07-09 9:50 ` [PATCH v1 5/5] First approach to sample time writing method Iker Perez
@ 2019-07-09 13:33 ` Guenter Roeck
0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2019-07-09 13:33 UTC (permalink / raw)
To: Iker Perez, Jean Delvare, linux-hwmon
On 7/9/19 2:50 AM, Iker Perez wrote:
> From: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
>
> Switch between the possible update_time values and write into the
> configuration register the selected value.
>
> Signed-off-by: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
> ---
> drivers/hwmon/lm75.c | 43 ++++++++++++++++++++++++++++++++++---------
> 1 file changed, 34 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index 5ba7277dac69..9d48a85fd3e5 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -156,21 +156,46 @@ static int lm75_read(struct device *dev, enum hwmon_sensor_types type,
> }
>
> static int lm75_write(struct device *dev, enum hwmon_sensor_types type,
> - u32 attr, int channel, long temp)
> + u32 attr, int channel, long val)
> {
> struct lm75_data *data = dev_get_drvdata(dev);
> - u8 resolution;
> - int reg;
> + u8 resolution, set_mask, clr_mask;
> + int reg, status;
> +
> + // This are provisional changes, to be improved in case this approach works
> + set_mask = 0;
> + clr_mask = LM75_SHUTDOWN;
>
> switch (type) {
> case hwmon_chip:
> switch (attr) {
> case hwmon_chip_update_interval:
> - if (data->kind == tmp75b)
> - pr_info("Iker inside write\n");
> + if (data->kind == tmp75b) {
> + clr_mask |= 1 << 15 | 0x3 << 13;
> + switch (val) {
> + case (27):
> + set_mask |= 0x3 << 13;
> + data->sample_time = MSEC_PER_SEC / 37;
> + break;
> + case (55):
> + set_mask |= 0x2 << 13;
> + data->sample_time = MSEC_PER_SEC / 18;
> + break;
> + case (111):
> + set_mask |= 0x1 << 13;
> + data->sample_time = MSEC_PER_SEC / 9;
> + break;
> + case (250):
> + set_mask |= 0x0 << 13;
> + data->sample_time = MSEC_PER_SEC / 4;
> + break;
> + default:
> + return -EINVAL;
> + status = configure_reg(set_mask, clr_mask, data, client);
You'll probably want to provide all the necessary variants (supported update times
as well as associated resolutions and set/clear masks) in the data structure,
to make it easy to support multiple chips. Either case, it is unacceptable to
only accept specific values: You can not expect the user to figure out individual
supported values on a per-chip basis. Please use something like find_closest()
to find the best match.
> + }
> + }
> else
> return -EINVAL;
> - break;
> default:
> return -EINVAL;
> }
> @@ -195,11 +220,11 @@ static int lm75_write(struct device *dev, enum hwmon_sensor_types type,
> else
> resolution = data->resolution;
>
> - temp = clamp_val(temp, LM75_TEMP_MIN, LM75_TEMP_MAX);
> - temp = DIV_ROUND_CLOSEST(temp << (resolution - 8),
> + val = clamp_val(val, LM75_TEMP_MIN, LM75_TEMP_MAX);
> + val = DIV_ROUND_CLOSEST(val << (resolution - 8),
> 1000) << (16 - resolution);
>
> - return regmap_write(data->regmap, reg, temp);
> + return regmap_write(data->regmap, reg, val);
> default:
> return -EINVAL;
> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 0/5] Help with lm75.c changes
2019-07-09 9:50 [PATCH v1 0/5] Help with lm75.c changes Iker Perez
` (4 preceding siblings ...)
2019-07-09 9:50 ` [PATCH v1 5/5] First approach to sample time writing method Iker Perez
@ 2019-07-09 13:43 ` Guenter Roeck
2019-07-09 15:11 ` Iker Perez del Palomar
5 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2019-07-09 13:43 UTC (permalink / raw)
To: Iker Perez, Jean Delvare, linux-hwmon
Hi,
On 7/9/19 2:50 AM, Iker Perez wrote:
> From: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
>
> Hello,
>
> I have been working in the lm75.c driver, trying to add a variable update_time
> to the tmp75b device.
>
> I am not very confident about, if what I am doing and how I am doing it is the
> best way it could be done. For that reason, I decided to send my current
> changes, so maybe I could be helped and my code revised.
>
> I decided to separate my all my changes in probably more than needed commits
> because I thought that it would b easier to understand at first place. After
> the feedback and my changes are ready to submit I will squash the ones that are
> related between them and the patch series will be much shorter.
>
> Thanks in advance for your help,
>
Looking through your patch series, I can't help thinking that you don't have
much experience writing kernel drivers. I am open to coaching you through this,
but I have to ask: Do you have an actual use case ? This is not something
we'll want to do as a coding exercise, since it will add a non-trivial
amount of code to the kernel.
Thanks,
Guenter
> Regards,
>
> Iker
>
> Iker Perez del Palomar Sustatxa (5):
> hwmon: (lm75) Add kind field to struct lm75_data
> hwmon: (lm75) Include hwmon_chip in the permitted types to be writen
> hwmon: (lm75) Give write permission to hwmon_chip_update_interval
> hwmon: (lm75) Create function from code to write into registers
> First approach to sample time writing method
>
> drivers/hwmon/lm75.c | 166 +++++++++++++++++++++++++++++++++------------------
> 1 file changed, 108 insertions(+), 58 deletions(-)
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 0/5] Help with lm75.c changes
2019-07-09 13:43 ` [PATCH v1 0/5] Help with lm75.c changes Guenter Roeck
@ 2019-07-09 15:11 ` Iker Perez del Palomar
0 siblings, 0 replies; 12+ messages in thread
From: Iker Perez del Palomar @ 2019-07-09 15:11 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare, linux-hwmon
Hi,
Thank you for offering to coach me! You are right, I don't have
experience of writing kernel drivers at all. I would like to be
coached by you. I have a system with an tmp75b device to test with.
At the moment I haven't been given any project to work in. so I decided
to work in this driver. We were using this driver and I wanted
to give a boost to it, while I learn more about the kernel drivers.
However I am open to any suggestions you might have, although If I am
given a new project I won't be able to work on this as much as now.
Thanks for your feedback and for offering to coach me,
Iker
On 09/07/2019 14:43, Guenter Roeck wrote:
> Hi,
>
> On 7/9/19 2:50 AM, Iker Perez wrote:
>> From: Iker Perez del Palomar Sustatxa <iker.perez@codethink.co.uk>
>>
>> Hello,
>>
>> I have been working in the lm75.c driver, trying to add a variable
>> update_time to the tmp75b device.
>>
>> I am not very confident about, if what I am doing and how I am
>> doing it is the best way it could be done. For that reason, I
>> decided to send my current changes, so maybe I could be helped and
>> my code revised.
>>
>> I decided to separate my all my changes in probably more than
>> needed commits because I thought that it would b easier to
>> understand at first place. After the feedback and my changes are
>> ready to submit I will squash the ones that are related between
>> them and the patch series will be much shorter.
>>
>> Thanks in advance for your help,
>>
>
> Looking through your patch series, I can't help thinking that you
> don't have much experience writing kernel drivers. I am open to
> coaching you through this, but I have to ask: Do you have an actual
> use case ? This is not something we'll want to do as a coding
> exercise, since it will add a non-trivial amount of code to the
> kernel.
>
> Thanks, Guenter
>
>> Regards,
>>
>> Iker
>>
>> Iker Perez del Palomar Sustatxa (5): hwmon: (lm75) Add kind field
>> to struct lm75_data hwmon: (lm75) Include hwmon_chip in the
>> permitted types to be writen hwmon: (lm75) Give write permission
>> to hwmon_chip_update_interval hwmon: (lm75) Create function from
>> code to write into registers First approach to sample time writing
>> method
>>
>> drivers/hwmon/lm75.c | 166
>> +++++++++++++++++++++++++++++++++------------------ 1 file
>> changed, 108 insertions(+), 58 deletions(-)
>>
>
>
--
Iker Perez del Palomar, Software Engineer
Codethink Ltd 35 Dale St, Manchester M1 2HF
http://www.codethink.co.uk/ Manchester, M1 2JW, United Kingdom
We respect your privacy. See https://www.codethink.co.uk/privacy.html
^ permalink raw reply [flat|nested] 12+ messages in thread