[PATCH v1 0/5] Help with lm75.c changes

[PATCH v1 0/5] Help with lm75.c changes

[Iker Perez]

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,

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(-)

-- 
2.11.0

[PATCH v1 1/5] hwmon: (lm75) Add kind field to struct lm75_data

[Iker Perez]

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

[PATCH v1 2/5] hwmon: (lm75) Include hwmon_chip in the permitted types to be writen

[Iker Perez]

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

[PATCH v1 3/5] hwmon: (lm75) Give write permission to hwmon_chip_update_interval

[Iker Perez]

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

[PATCH v1 4/5] hwmon: (lm75) Create function from code to write into registers

[Iker Perez]

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

[PATCH v1 5/5] First approach to sample time writing method

[Iker Perez]

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

Re: [PATCH v1 1/5] hwmon: (lm75) Add kind field to struct lm75_data

[Guenter Roeck]

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;
> 

Re: [PATCH v1 2/5] hwmon: (lm75) Include hwmon_chip in the permitted types to be writen

[Guenter Roeck]

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,
> 

Re: [PATCH v1 5/5] First approach to sample time writing method

[Guenter Roeck]

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;
>   	}
> 

Re: [PATCH v1 4/5] hwmon: (lm75) Create function from code to write into registers

[Guenter Roeck]

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,
> 

Re: [PATCH v1 0/5] Help with lm75.c changes

[Guenter Roeck]

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(-)
> 

Re: [PATCH v1 0/5] Help with lm75.c changes

[Iker Perez del Palomar]

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