[PATCH 1/2] hwmon: (emc2305) fix unable to probe emc2301/2/3/4

[PATCH 1/2] hwmon: (emc2305) fix unable to probe emc2301/2/3/4

[Xingjiang Qiao]

The definitions of 'EMC2305_REG_PRODUCT_ID' and 'EMC2305_REG_DEVICE' are
both '0xfd', they actually return the same value, but the values returned
by emc2301/2/3/4/5 are different, so probe emc2301/2/3/4 will fail, This
patch fixes that.

The second parameter of 'emc2305_probe' is actually useless, it is more
appropriate to use 'probe_new' instead of 'probe' here.

Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>
---
 drivers/hwmon/emc2305.c | 58 ++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 39 deletions(-)

diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
index aa1f25add0b6..4df84065cbfb 100644
--- a/drivers/hwmon/emc2305.c
+++ b/drivers/hwmon/emc2305.c
@@ -21,9 +21,7 @@ emc2305_normal_i2c[] = { 0x27, 0x2c, 0x2d, 0x2e, 0x2f, 0x4c, 0x4d, I2C_CLIENT_EN
 #define EMC2305_FAN_MAX			0xff
 #define EMC2305_FAN_MIN			0x00
 #define EMC2305_FAN_MAX_STATE		10
-#define EMC2305_DEVICE			0x34
 #define EMC2305_VENDOR			0x5d
-#define EMC2305_REG_PRODUCT_ID		0xfd
 #define EMC2305_TACH_REGS_UNUSE_BITS	3
 #define EMC2305_TACH_CNT_MULTIPLIER	0x02
 #define EMC2305_TACH_RANGE_MIN		480
@@ -488,43 +486,14 @@ static const struct hwmon_chip_info emc2305_chip_info = {
 	.info = emc2305_info,
 };
 
-static int emc2305_identify(struct device *dev)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-	struct emc2305_data *data = i2c_get_clientdata(client);
-	int ret;
-
-	ret = i2c_smbus_read_byte_data(client, EMC2305_REG_PRODUCT_ID);
-	if (ret < 0)
-		return ret;
-
-	switch (ret) {
-	case EMC2305:
-		data->pwm_num = 5;
-		break;
-	case EMC2303:
-		data->pwm_num = 3;
-		break;
-	case EMC2302:
-		data->pwm_num = 2;
-		break;
-	case EMC2301:
-		data->pwm_num = 1;
-		break;
-	default:
-		return -ENODEV;
-	}
-
-	return 0;
-}
-
-static int emc2305_probe(struct i2c_client *client, const struct i2c_device_id *id)
+static int emc2305_probe(struct i2c_client *client)
 {
 	struct i2c_adapter *adapter = client->adapter;
 	struct device *dev = &client->dev;
 	struct emc2305_data *data;
 	struct emc2305_platform_data *pdata;
 	int vendor, device;
+	int pwm_num;
 	int ret;
 	int i;
 
@@ -536,20 +505,31 @@ static int emc2305_probe(struct i2c_client *client, const struct i2c_device_id *
 		return -ENODEV;
 
 	device = i2c_smbus_read_byte_data(client, EMC2305_REG_DEVICE);
-	if (device != EMC2305_DEVICE)
+	switch (device) {
+	case EMC2305:
+		pwm_num = 5;
+		break;
+	case EMC2303:
+		pwm_num = 3;
+		break;
+	case EMC2302:
+		pwm_num = 2;
+		break;
+	case EMC2301:
+		pwm_num = 1;
+		break;
+	default:
 		return -ENODEV;
+	}
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
 	i2c_set_clientdata(client, data);
+	data->pwm_num = pwm_num;
 	data->client = client;
 
-	ret = emc2305_identify(dev);
-	if (ret)
-		return ret;
-
 	pdata = dev_get_platdata(&client->dev);
 	if (pdata) {
 		if (!pdata->max_state || pdata->max_state > EMC2305_FAN_MAX_STATE)
@@ -604,10 +584,10 @@ static void emc2305_remove(struct i2c_client *client)
 
 static struct i2c_driver emc2305_driver = {
 	.class  = I2C_CLASS_HWMON,
+	.probe_new = emc2305_probe,
 	.driver = {
 		.name = "emc2305",
 	},
-	.probe    = emc2305_probe,
 	.remove	  = emc2305_remove,
 	.id_table = emc2305_ids,
 	.address_list = emc2305_normal_i2c,
-- 
2.38.1

[PATCH 2/2] hwmon: (emc2305) fix pwm never being able to set lower

[Xingjiang Qiao]

There are fields 'last_hwmon_state' and 'last_thermal_state' in the
structure 'emc2305_cdev_data', which respectively store the cooling state
set by the 'hwmon' and 'thermal' subsystem, and the driver author hopes
that if the state set by 'hwmon' is lower than the value set by 'thermal',
the driver will just save it without actually setting the pwm. Currently,
the 'last_thermal_state' also be updated by 'hwmon', which will cause the
cooling state to never be set to a lower value. This patch fixes that.

Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>
---
 drivers/hwmon/emc2305.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
index 4df84065cbfb..f51760f8aa85 100644
--- a/drivers/hwmon/emc2305.c
+++ b/drivers/hwmon/emc2305.c
@@ -398,11 +398,9 @@ emc2305_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int ch
 				 * Update PWM only in case requested state is not less than the
 				 * last thermal state.
 				 */
-				if (data->cdev_data[cdev_idx].last_hwmon_state >=
+				if (data->cdev_data[cdev_idx].last_hwmon_state <
 				    data->cdev_data[cdev_idx].last_thermal_state)
-					return emc2305_set_cur_state(data->cdev_data[cdev_idx].cdev,
-							data->cdev_data[cdev_idx].last_hwmon_state);
-				return 0;
+					return 0;
 			}
 			return emc2305_set_pwm(dev, val, channel);
 		default:
-- 
2.38.1

Re: [PATCH 1/2] hwmon: (emc2305) fix unable to probe emc2301/2/3/4

[Guenter Roeck]

On 12/5/22 09:36, Xingjiang Qiao wrote:
> The definitions of 'EMC2305_REG_PRODUCT_ID' and 'EMC2305_REG_DEVICE' are
> both '0xfd', they actually return the same value, but the values returned
> by emc2301/2/3/4/5 are different, so probe emc2301/2/3/4 will fail, This
> patch fixes that.
> 

This solves the wrong problem. The check for REG_DEVICE should simply
be removed instead (EMC2305_REG_PRODUCT_ID and EMC2305_REG_DEVICE are
both 0xfd). On top of that, moving the functionality of emc2305_identify()
does not improve code quality (quite the contrary) and is thus not acceptable.

> The second parameter of 'emc2305_probe' is actually useless, it is more
> appropriate to use 'probe_new' instead of 'probe' here.
> 

This would be a second patch. Besides, this change is already queued in
hwmon-next for v6.2.

Guenter

> Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>
> ---
>   drivers/hwmon/emc2305.c | 58 ++++++++++++++---------------------------
>   1 file changed, 19 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
> index aa1f25add0b6..4df84065cbfb 100644
> --- a/drivers/hwmon/emc2305.c
> +++ b/drivers/hwmon/emc2305.c
> @@ -21,9 +21,7 @@ emc2305_normal_i2c[] = { 0x27, 0x2c, 0x2d, 0x2e, 0x2f, 0x4c, 0x4d, I2C_CLIENT_EN
>   #define EMC2305_FAN_MAX			0xff
>   #define EMC2305_FAN_MIN			0x00
>   #define EMC2305_FAN_MAX_STATE		10
> -#define EMC2305_DEVICE			0x34
>   #define EMC2305_VENDOR			0x5d
> -#define EMC2305_REG_PRODUCT_ID		0xfd
>   #define EMC2305_TACH_REGS_UNUSE_BITS	3
>   #define EMC2305_TACH_CNT_MULTIPLIER	0x02
>   #define EMC2305_TACH_RANGE_MIN		480
> @@ -488,43 +486,14 @@ static const struct hwmon_chip_info emc2305_chip_info = {
>   	.info = emc2305_info,
>   };
>   
> -static int emc2305_identify(struct device *dev)
> -{
> -	struct i2c_client *client = to_i2c_client(dev);
> -	struct emc2305_data *data = i2c_get_clientdata(client);
> -	int ret;
> -
> -	ret = i2c_smbus_read_byte_data(client, EMC2305_REG_PRODUCT_ID);
> -	if (ret < 0)
> -		return ret;
> -
> -	switch (ret) {
> -	case EMC2305:
> -		data->pwm_num = 5;
> -		break;
> -	case EMC2303:
> -		data->pwm_num = 3;
> -		break;
> -	case EMC2302:
> -		data->pwm_num = 2;
> -		break;
> -	case EMC2301:
> -		data->pwm_num = 1;
> -		break;
> -	default:
> -		return -ENODEV;
> -	}
> -
> -	return 0;
> -}
> -
> -static int emc2305_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +static int emc2305_probe(struct i2c_client *client)
>   {
>   	struct i2c_adapter *adapter = client->adapter;
>   	struct device *dev = &client->dev;
>   	struct emc2305_data *data;
>   	struct emc2305_platform_data *pdata;
>   	int vendor, device;
> +	int pwm_num;
>   	int ret;
>   	int i;
>   
> @@ -536,20 +505,31 @@ static int emc2305_probe(struct i2c_client *client, const struct i2c_device_id *
>   		return -ENODEV;
>   
>   	device = i2c_smbus_read_byte_data(client, EMC2305_REG_DEVICE);
> -	if (device != EMC2305_DEVICE)
> +	switch (device) {
> +	case EMC2305:
> +		pwm_num = 5;
> +		break;
> +	case EMC2303:
> +		pwm_num = 3;
> +		break;
> +	case EMC2302:
> +		pwm_num = 2;
> +		break;
> +	case EMC2301:
> +		pwm_num = 1;
> +		break;
> +	default:
>   		return -ENODEV;
> +	}
>   
>   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>   	if (!data)
>   		return -ENOMEM;
>   
>   	i2c_set_clientdata(client, data);
> +	data->pwm_num = pwm_num;
>   	data->client = client;
>   
> -	ret = emc2305_identify(dev);
> -	if (ret)
> -		return ret;
> -
>   	pdata = dev_get_platdata(&client->dev);
>   	if (pdata) {
>   		if (!pdata->max_state || pdata->max_state > EMC2305_FAN_MAX_STATE)
> @@ -604,10 +584,10 @@ static void emc2305_remove(struct i2c_client *client)
>   
>   static struct i2c_driver emc2305_driver = {
>   	.class  = I2C_CLASS_HWMON,
> +	.probe_new = emc2305_probe,
>   	.driver = {
>   		.name = "emc2305",
>   	},
> -	.probe    = emc2305_probe,
>   	.remove	  = emc2305_remove,
>   	.id_table = emc2305_ids,
>   	.address_list = emc2305_normal_i2c,

RE: [PATCH 2/2] hwmon: (emc2305) fix pwm never being able to set lower

[Michael Shych]

Hi,

This is intention - to set low limit through 'hwmon'. 
Thus, if it is set through 'hwom', cooling state is limited and not allowed to be set to a lower value.
If user doesn't want this feature, he is not supposed to use 'hwmon' for fan speed setting.

Regards,
   Michael.

> -----Original Message-----
> From: Xingjiang Qiao <nanpuyue@gmail.com>
> Sent: Monday, December 5, 2022 7:36 PM
> To: Guenter Roeck <linux@roeck-us.net>; Jean Delvare
> <jdelvare@suse.com>
> Cc: Michael Shych <michaelsh@nvidia.com>; Xingjiang Qiao
> <nanpuyue@gmail.com>; linux-hwmon@vger.kernel.org
> Subject: [PATCH 2/2] hwmon: (emc2305) fix pwm never being able to set
> lower
> 
> There are fields 'last_hwmon_state' and 'last_thermal_state' in the structure
> 'emc2305_cdev_data', which respectively store the cooling state set by the
> 'hwmon' and 'thermal' subsystem, and the driver author hopes that if the
> state set by 'hwmon' is lower than the value set by 'thermal', the driver will
> just save it without actually setting the pwm. Currently, the
> 'last_thermal_state' also be updated by 'hwmon', which will cause the
> cooling state to never be set to a lower value. This patch fixes that.
> 
> Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>
> ---
>  drivers/hwmon/emc2305.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c index
> 4df84065cbfb..f51760f8aa85 100644
> --- a/drivers/hwmon/emc2305.c
> +++ b/drivers/hwmon/emc2305.c
> @@ -398,11 +398,9 @@ emc2305_write(struct device *dev, enum
> hwmon_sensor_types type, u32 attr, int ch
>  				 * Update PWM only in case requested state
> is not less than the
>  				 * last thermal state.
>  				 */
> -				if (data-
> >cdev_data[cdev_idx].last_hwmon_state >=
> +				if (data-
> >cdev_data[cdev_idx].last_hwmon_state <
>  				    data-
> >cdev_data[cdev_idx].last_thermal_state)
> -					return emc2305_set_cur_state(data-
> >cdev_data[cdev_idx].cdev,
> -							data-
> >cdev_data[cdev_idx].last_hwmon_state);
> -				return 0;
> +					return 0;
>  			}
>  			return emc2305_set_pwm(dev, val, channel);
>  		default:
> --
> 2.38.1

Re: [PATCH 2/2] hwmon: (emc2305) fix pwm never being able to set lower

[Guenter Roeck]

On 12/5/22 09:36, Xingjiang Qiao wrote:
> There are fields 'last_hwmon_state' and 'last_thermal_state' in the
> structure 'emc2305_cdev_data', which respectively store the cooling state
> set by the 'hwmon' and 'thermal' subsystem, and the driver author hopes
> that if the state set by 'hwmon' is lower than the value set by 'thermal',
> the driver will just save it without actually setting the pwm. Currently,
> the 'last_thermal_state' also be updated by 'hwmon', which will cause the
> cooling state to never be set to a lower value. This patch fixes that.
> 
> Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>
> ---
>   drivers/hwmon/emc2305.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
> index 4df84065cbfb..f51760f8aa85 100644
> --- a/drivers/hwmon/emc2305.c
> +++ b/drivers/hwmon/emc2305.c
> @@ -398,11 +398,9 @@ emc2305_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int ch
>   				 * Update PWM only in case requested state is not less than the
>   				 * last thermal state.
>   				 */
> -				if (data->cdev_data[cdev_idx].last_hwmon_state >=
> +				if (data->cdev_data[cdev_idx].last_hwmon_state <
>   				    data->cdev_data[cdev_idx].last_thermal_state)
> -					return emc2305_set_cur_state(data->cdev_data[cdev_idx].cdev,
> -							data->cdev_data[cdev_idx].last_hwmon_state);
> -				return 0;
> +					return 0;
>   			}
>   			return emc2305_set_pwm(dev, val, channel);

This would bypass the thermal code, leaving the thermal subsystem unaware
of the current state. I am not entirely sure what the best solution is.
Skipping the entire thermal code in that situation doesn't seem the be
correct. Maybe there needs to be a shim function for the thermal code to
only update last_thermal_state if the request was indeed made by the thermal
subsystem.

Guenter

RE: [PATCH 1/2] hwmon: (emc2305) fix unable to probe emc2301/2/3/4

[Michael Shych]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, December 5, 2022 8:15 PM
> To: Xingjiang Qiao <nanpuyue@gmail.com>; Jean Delvare
> <jdelvare@suse.com>
> Cc: Michael Shych <michaelsh@nvidia.com>; linux-hwmon@vger.kernel.org
> Subject: Re: [PATCH 1/2] hwmon: (emc2305) fix unable to probe
> emc2301/2/3/4
> 
> On 12/5/22 09:36, Xingjiang Qiao wrote:
> > The definitions of 'EMC2305_REG_PRODUCT_ID' and
> 'EMC2305_REG_DEVICE'
> > are both '0xfd', they actually return the same value, but the values
> > returned by emc2301/2/3/4/5 are different, so probe emc2301/2/3/4 will
> > fail, This patch fixes that.
> >
> 
> This solves the wrong problem. The check for REG_DEVICE should simply be
> removed instead (EMC2305_REG_PRODUCT_ID and EMC2305_REG_DEVICE
> are both 0xfd). On top of that, moving the functionality of
> emc2305_identify() does not improve code quality (quite the contrary) and is
> thus not acceptable.

Thanks for catching the problem. 
Generalization for EMC2301/2/3 was added later to the code.
I agree with Guenter that check of EMC2305_REG_DEVICE should be just removed.

> 
> > The second parameter of 'emc2305_probe' is actually useless, it is
> > more appropriate to use 'probe_new' instead of 'probe' here.
> >
> 
> This would be a second patch. Besides, this change is already queued in
> hwmon-next for v6.2.
> 
> Guenter
> 
> > Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>
> > ---
> >   drivers/hwmon/emc2305.c | 58 ++++++++++++++---------------------------
> >   1 file changed, 19 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c index
> > aa1f25add0b6..4df84065cbfb 100644
> > --- a/drivers/hwmon/emc2305.c
> > +++ b/drivers/hwmon/emc2305.c
> > @@ -21,9 +21,7 @@ emc2305_normal_i2c[] = { 0x27, 0x2c, 0x2d, 0x2e, 0x2f,
> 0x4c, 0x4d, I2C_CLIENT_EN
> >   #define EMC2305_FAN_MAX			0xff
> >   #define EMC2305_FAN_MIN			0x00
> >   #define EMC2305_FAN_MAX_STATE		10
> > -#define EMC2305_DEVICE			0x34
> >   #define EMC2305_VENDOR			0x5d
> > -#define EMC2305_REG_PRODUCT_ID		0xfd
> >   #define EMC2305_TACH_REGS_UNUSE_BITS	3
> >   #define EMC2305_TACH_CNT_MULTIPLIER	0x02
> >   #define EMC2305_TACH_RANGE_MIN		480
> > @@ -488,43 +486,14 @@ static const struct hwmon_chip_info
> emc2305_chip_info = {
> >   	.info = emc2305_info,
> >   };
> >
> > -static int emc2305_identify(struct device *dev) -{
> > -	struct i2c_client *client = to_i2c_client(dev);
> > -	struct emc2305_data *data = i2c_get_clientdata(client);
> > -	int ret;
> > -
> > -	ret = i2c_smbus_read_byte_data(client,
> EMC2305_REG_PRODUCT_ID);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	switch (ret) {
> > -	case EMC2305:
> > -		data->pwm_num = 5;
> > -		break;
> > -	case EMC2303:
> > -		data->pwm_num = 3;
> > -		break;
> > -	case EMC2302:
> > -		data->pwm_num = 2;
> > -		break;
> > -	case EMC2301:
> > -		data->pwm_num = 1;
> > -		break;
> > -	default:
> > -		return -ENODEV;
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> > -static int emc2305_probe(struct i2c_client *client, const struct
> > i2c_device_id *id)
> > +static int emc2305_probe(struct i2c_client *client)
> >   {
> >   	struct i2c_adapter *adapter = client->adapter;
> >   	struct device *dev = &client->dev;
> >   	struct emc2305_data *data;
> >   	struct emc2305_platform_data *pdata;
> >   	int vendor, device;
> > +	int pwm_num;
> >   	int ret;
> >   	int i;
> >
> > @@ -536,20 +505,31 @@ static int emc2305_probe(struct i2c_client *client,
> const struct i2c_device_id *
> >   		return -ENODEV;
> >
> >   	device = i2c_smbus_read_byte_data(client, EMC2305_REG_DEVICE);
> > -	if (device != EMC2305_DEVICE)
> > +	switch (device) {
> > +	case EMC2305:
> > +		pwm_num = 5;
> > +		break;
> > +	case EMC2303:
> > +		pwm_num = 3;
> > +		break;
> > +	case EMC2302:
> > +		pwm_num = 2;
> > +		break;
> > +	case EMC2301:
> > +		pwm_num = 1;
> > +		break;
> > +	default:
> >   		return -ENODEV;
> > +	}
> >
> >   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >   	if (!data)
> >   		return -ENOMEM;
> >
> >   	i2c_set_clientdata(client, data);
> > +	data->pwm_num = pwm_num;
> >   	data->client = client;
> >
> > -	ret = emc2305_identify(dev);
> > -	if (ret)
> > -		return ret;
> > -
> >   	pdata = dev_get_platdata(&client->dev);
> >   	if (pdata) {
> >   		if (!pdata->max_state || pdata->max_state >
> EMC2305_FAN_MAX_STATE)
> > @@ -604,10 +584,10 @@ static void emc2305_remove(struct i2c_client
> > *client)
> >
> >   static struct i2c_driver emc2305_driver = {
> >   	.class  = I2C_CLASS_HWMON,
> > +	.probe_new = emc2305_probe,
> >   	.driver = {
> >   		.name = "emc2305",
> >   	},
> > -	.probe    = emc2305_probe,
> >   	.remove	  = emc2305_remove,
> >   	.id_table = emc2305_ids,
> >   	.address_list = emc2305_normal_i2c,

[PATCH v2 1/2] hwmon: (emc2305) fix unable to probe emc2301/2/3/4

[Xingjiang Qiao]

The definitions of 'EMC2305_REG_PRODUCT_ID' and 'EMC2305_REG_DEVICE' are
both '0xfd', they actually return the same value, but the values returned
by emc2301/2/3/4/5 are different, so probe emc2301/2/3/4 will fail, This
patch fixes that.

Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>
---
 drivers/hwmon/emc2305.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
index aa1f25add0b6..9a78ca22541e 100644
--- a/drivers/hwmon/emc2305.c
+++ b/drivers/hwmon/emc2305.c
@@ -16,7 +16,6 @@ static const unsigned short
 emc2305_normal_i2c[] = { 0x27, 0x2c, 0x2d, 0x2e, 0x2f, 0x4c, 0x4d, I2C_CLIENT_END };
 
 #define EMC2305_REG_DRIVE_FAIL_STATUS	0x27
-#define EMC2305_REG_DEVICE		0xfd
 #define EMC2305_REG_VENDOR		0xfe
 #define EMC2305_FAN_MAX			0xff
 #define EMC2305_FAN_MIN			0x00
@@ -524,7 +523,7 @@ static int emc2305_probe(struct i2c_client *client, const struct i2c_device_id *
 	struct device *dev = &client->dev;
 	struct emc2305_data *data;
 	struct emc2305_platform_data *pdata;
-	int vendor, device;
+	int vendor;
 	int ret;
 	int i;
 
@@ -535,10 +534,6 @@ static int emc2305_probe(struct i2c_client *client, const struct i2c_device_id *
 	if (vendor != EMC2305_VENDOR)
 		return -ENODEV;
 
-	device = i2c_smbus_read_byte_data(client, EMC2305_REG_DEVICE);
-	if (device != EMC2305_DEVICE)
-		return -ENODEV;
-
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
-- 
2.38.1

RE: [PATCH 2/2] hwmon: (emc2305) fix pwm never being able to set lower

[Xingjiang Qiao]

Even when the cooling state is not set by the thermal subsystem, no lower state can be set by hwmon.



> On Tue, Dec 6, 2022 at 2:27 AM Michael Shych <michaelsh@nvidia.com> wrote:
> Hi,
> 
> This is intention - to set low limit through 'hwmon'.
> Thus, if it is set through 'hwom', cooling state is limited and not allowed to be set to a lower value.
> If user doesn't want this feature, he is not supposed to use 'hwmon' for fan speed setting.
> 
> Regards,
>    Michael.
> 
> > -----Original Message-----
> > From: Xingjiang Qiao <nanpuyue@gmail.com>
> > Sent: Monday, December 5, 2022 7:36 PM
> > To: Guenter Roeck <linux@roeck-us.net>; Jean Delvare
> > <jdelvare@suse.com>
> > Cc: Michael Shych <michaelsh@nvidia.com>; Xingjiang Qiao
> > <nanpuyue@gmail.com>; linux-hwmon@vger.kernel.org
> > Subject: [PATCH 2/2] hwmon: (emc2305) fix pwm never being able to set
> > lower
> >
> > There are fields 'last_hwmon_state' and 'last_thermal_state' in the structure
> > 'emc2305_cdev_data', which respectively store the cooling state set by the
> > 'hwmon' and 'thermal' subsystem, and the driver author hopes that if the
> > state set by 'hwmon' is lower than the value set by 'thermal', the driver will
> > just save it without actually setting the pwm. Currently, the
> > 'last_thermal_state' also be updated by 'hwmon', which will cause the
> > cooling state to never be set to a lower value. This patch fixes that.
> >
> > Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>
> > ---
> >  drivers/hwmon/emc2305.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c index
> > 4df84065cbfb..f51760f8aa85 100644
> > --- a/drivers/hwmon/emc2305.c
> > +++ b/drivers/hwmon/emc2305.c
> > @@ -398,11 +398,9 @@ emc2305_write(struct device *dev, enum
> > hwmon_sensor_types type, u32 attr, int ch
> >                                * Update PWM only in case requested state
> > is not less than the
> >                                * last thermal state.
> >                                */
> > -                             if (data-
> > >cdev_data[cdev_idx].last_hwmon_state >=
> > +                             if (data-
> > >cdev_data[cdev_idx].last_hwmon_state <
> >                                   data-
> > >cdev_data[cdev_idx].last_thermal_state)
> > -                                     return emc2305_set_cur_state(data-
> > >cdev_data[cdev_idx].cdev,
> > -                                                     data-
> > >cdev_data[cdev_idx].last_hwmon_state);
> > -                             return 0;
> > +                                     return 0;
> >                       }
> >                       return emc2305_set_pwm(dev, val, channel);
> >               default:
> > --
> > 2.38.1

RE: [PATCH 2/2] hwmon: (emc2305) fix pwm never being able to set lower

[Xingjiang Qiao]

The value range of the chip's 'FAN DRIVE SETTING' register is 0-255.
But currently, the driver can only set up to 10 values at most (even via
the 'hwmon' subsystem) when the 'thermal' subsystem is reachable, and
there is no way to increase this limit via "emc2305_platform_data".

Should the "pdata->max_state > EMC2305_FAN_MAX_STATE" check be removed?
Or 'EMC2305_FAN_MAX_STATE' should be defined as '0xff'?

This should be the third patch?


> 	pdata = dev_get_platdata(&client->dev);
> 	if (pdata) {
> 		if (!pdata->max_state || pdata->max_state > EMC2305_FAN_MAX_STATE)
> 			return -EINVAL;
> 		data->max_state = pdata->max_state;

[PATCH v3 1/2] hwmon: (emc2305) fix unable to probe emc2301/2/3

[Xingjiang Qiao]

The definitions of 'EMC2305_REG_PRODUCT_ID' and 'EMC2305_REG_DEVICE' are
both '0xfd', they actually return the same value, but the values returned
by emc2301/2/3/5 are different, so probe emc2301/2/3 will fail, This patch
fixes that.

Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>
---
 drivers/hwmon/emc2305.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
index aa1f25add0b6..9a78ca22541e 100644
--- a/drivers/hwmon/emc2305.c
+++ b/drivers/hwmon/emc2305.c
@@ -16,7 +16,6 @@ static const unsigned short
 emc2305_normal_i2c[] = { 0x27, 0x2c, 0x2d, 0x2e, 0x2f, 0x4c, 0x4d, I2C_CLIENT_END };
 
 #define EMC2305_REG_DRIVE_FAIL_STATUS	0x27
-#define EMC2305_REG_DEVICE		0xfd
 #define EMC2305_REG_VENDOR		0xfe
 #define EMC2305_FAN_MAX			0xff
 #define EMC2305_FAN_MIN			0x00
@@ -524,7 +523,7 @@ static int emc2305_probe(struct i2c_client *client, const struct i2c_device_id *
 	struct device *dev = &client->dev;
 	struct emc2305_data *data;
 	struct emc2305_platform_data *pdata;
-	int vendor, device;
+	int vendor;
 	int ret;
 	int i;
 
@@ -535,10 +534,6 @@ static int emc2305_probe(struct i2c_client *client, const struct i2c_device_id *
 	if (vendor != EMC2305_VENDOR)
 		return -ENODEV;
 
-	device = i2c_smbus_read_byte_data(client, EMC2305_REG_DEVICE);
-	if (device != EMC2305_DEVICE)
-		return -ENODEV;
-
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
-- 
2.38.1

[PATCH v3 2/2] hwmon: (emc2305) fix pwm never being able to set lower

[Xingjiang Qiao]

There are fields 'last_hwmon_state' and 'last_thermal_state' in the
structure 'emc2305_cdev_data', which respectively store the cooling state
set by the 'hwmon' and 'thermal' subsystem, and the driver author hopes
that if the state set by 'hwmon' is lower than the value set by 'thermal',
the driver will just save it without actually setting the pwm. Currently,
the 'last_thermal_state' also be updated by 'hwmon', which will cause the
cooling state to never be set to a lower value. This patch fixes that.

Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>
---
 drivers/hwmon/emc2305.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
index 9a78ca22541e..e0f6eb8d72fc 100644
--- a/drivers/hwmon/emc2305.c
+++ b/drivers/hwmon/emc2305.c
@@ -171,22 +171,12 @@ static int emc2305_get_max_state(struct thermal_cooling_device *cdev, unsigned l
 	return 0;
 }
 
-static int emc2305_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
+static int emc2305_set_cur_state_shim(struct emc2305_data *data, int cdev_idx, unsigned long state)
 {
-	int cdev_idx, ret;
-	struct emc2305_data *data = cdev->devdata;
+	int ret;
 	struct i2c_client *client = data->client;
 	u8 val, i;
 
-	if (state > data->max_state)
-		return -EINVAL;
-
-	cdev_idx =  emc2305_get_cdev_idx(cdev);
-	if (cdev_idx < 0)
-		return cdev_idx;
-
-	/* Save thermal state. */
-	data->cdev_data[cdev_idx].last_thermal_state = state;
 	state = max_t(unsigned long, state, data->cdev_data[cdev_idx].last_hwmon_state);
 
 	val = EMC2305_PWM_STATE2DUTY(state, data->max_state, EMC2305_FAN_MAX);
@@ -211,6 +201,27 @@ static int emc2305_set_cur_state(struct thermal_cooling_device *cdev, unsigned l
 	return 0;
 }
 
+static int emc2305_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
+{
+	int cdev_idx, ret;
+	struct emc2305_data *data = cdev->devdata;
+
+	if (state > data->max_state)
+		return -EINVAL;
+
+	cdev_idx =  emc2305_get_cdev_idx(cdev);
+	if (cdev_idx < 0)
+		return cdev_idx;
+
+	/* Save thermal state. */
+	data->cdev_data[cdev_idx].last_thermal_state = state;
+	ret = emc2305_set_cur_state_shim(data, cdev_idx, state);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static const struct thermal_cooling_device_ops emc2305_cooling_ops = {
 	.get_max_state = emc2305_get_max_state,
 	.get_cur_state = emc2305_get_cur_state,
@@ -401,7 +412,7 @@ emc2305_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int ch
 				 */
 				if (data->cdev_data[cdev_idx].last_hwmon_state >=
 				    data->cdev_data[cdev_idx].last_thermal_state)
-					return emc2305_set_cur_state(data->cdev_data[cdev_idx].cdev,
+					return emc2305_set_cur_state_shim(data, cdev_idx,
 							data->cdev_data[cdev_idx].last_hwmon_state);
 				return 0;
 			}
-- 
2.38.1

Re: [PATCH v3 1/2] hwmon: (emc2305) fix unable to probe emc2301/2/3

[Guenter Roeck]

On 12/5/22 21:53, Xingjiang Qiao wrote:
> The definitions of 'EMC2305_REG_PRODUCT_ID' and 'EMC2305_REG_DEVICE' are
> both '0xfd', they actually return the same value, but the values returned
> by emc2301/2/3/5 are different, so probe emc2301/2/3 will fail, This patch
> fixes that.
> 
> Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>

Please stop sending new versions of your patches as reply to previous versions,
and please provide change logs. I have no idea what is different between
versions 2 and 3 of this patch, and v2 as well as v3 almost got lost.

Guenter

> ---
>   drivers/hwmon/emc2305.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
> index aa1f25add0b6..9a78ca22541e 100644
> --- a/drivers/hwmon/emc2305.c
> +++ b/drivers/hwmon/emc2305.c
> @@ -16,7 +16,6 @@ static const unsigned short
>   emc2305_normal_i2c[] = { 0x27, 0x2c, 0x2d, 0x2e, 0x2f, 0x4c, 0x4d, I2C_CLIENT_END };
>   
>   #define EMC2305_REG_DRIVE_FAIL_STATUS	0x27
> -#define EMC2305_REG_DEVICE		0xfd
>   #define EMC2305_REG_VENDOR		0xfe
>   #define EMC2305_FAN_MAX			0xff
>   #define EMC2305_FAN_MIN			0x00
> @@ -524,7 +523,7 @@ static int emc2305_probe(struct i2c_client *client, const struct i2c_device_id *
>   	struct device *dev = &client->dev;
>   	struct emc2305_data *data;
>   	struct emc2305_platform_data *pdata;
> -	int vendor, device;
> +	int vendor;
>   	int ret;
>   	int i;
>   
> @@ -535,10 +534,6 @@ static int emc2305_probe(struct i2c_client *client, const struct i2c_device_id *
>   	if (vendor != EMC2305_VENDOR)
>   		return -ENODEV;
>   
> -	device = i2c_smbus_read_byte_data(client, EMC2305_REG_DEVICE);
> -	if (device != EMC2305_DEVICE)
> -		return -ENODEV;
> -
>   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>   	if (!data)
>   		return -ENOMEM;

Re: [PATCH v3 1/2] hwmon: (emc2305) fix unable to probe emc2301/2/3

[Xingjiang Qiao]

> Please stop sending new versions of your patches as reply to previous versions,
> and please provide change logs. I have no idea what is different between
> versions 2 and 3 of this patch, and v2 as well as v3 almost got lost.
> 
> Guenter

Sorry about that.

And the changlogs of the patch 1/2 is:

- V1 -> V2: Just remove the check for 'EMC2305_REG_DEVICE' instead of
  moving the functionality of 'emc2305_identify'
- V2 -> V3: Reword the commit as there is no emc2304

changlogs of the patch 2/2 is:

- V1 -> V2: No substantive changes, was going to send later, but v3 has
  being sent before that (sorry about that again)
- V1 -> V3: Add 'emc2305_set_cur_state_shim' to avoid updating
  'last_thermal_state' when cooling state is set by 'hwmon' subsystem

The v3 patch 2/2 can be found at:
https://lore.kernel.org/all/20221206055331.170459-2-nanpuyue@gmail.com/

Should I resend them?

One more question about 'EMC2305_FAN_MAX_STATE':
https://lore.kernel.org/all/20221206053029.169506-1-nanpuyue@gmail.com/

> The value range of the chip's 'FAN DRIVE SETTING' register is 0-255.
> But currently, the driver can only set up to 10 values at most (even via
> the 'hwmon' subsystem) when the 'thermal' subsystem is reachable, and
> there is no way to increase this limit via "emc2305_platform_data".
> 
> Should the "pdata->max_state > EMC2305_FAN_MAX_STATE" check be removed?
> Or 'EMC2305_FAN_MAX_STATE' should be defined as '0xff'?
> 
> This should be the third patch?
> 
> 
> > 	pdata = dev_get_platdata(&client->dev);
> > 	if (pdata) {
> > 		if (!pdata->max_state || pdata->max_state > EMC2305_FAN_MAX_STATE)
> > 			return -EINVAL;
> > 		data->max_state = pdata->max_state;

Re: [PATCH v3 1/2] hwmon: (emc2305) fix unable to probe emc2301/2/3

[Guenter Roeck]

On Tue, Dec 06, 2022 at 01:53:30PM +0800, Xingjiang Qiao wrote:
> The definitions of 'EMC2305_REG_PRODUCT_ID' and 'EMC2305_REG_DEVICE' are
> both '0xfd', they actually return the same value, but the values returned
> by emc2301/2/3/5 are different, so probe emc2301/2/3 will fail, This patch
> fixes that.
> 
> Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/emc2305.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
> index aa1f25add0b6..9a78ca22541e 100644
> --- a/drivers/hwmon/emc2305.c
> +++ b/drivers/hwmon/emc2305.c
> @@ -16,7 +16,6 @@ static const unsigned short
>  emc2305_normal_i2c[] = { 0x27, 0x2c, 0x2d, 0x2e, 0x2f, 0x4c, 0x4d, I2C_CLIENT_END };
>  
>  #define EMC2305_REG_DRIVE_FAIL_STATUS	0x27
> -#define EMC2305_REG_DEVICE		0xfd
>  #define EMC2305_REG_VENDOR		0xfe
>  #define EMC2305_FAN_MAX			0xff
>  #define EMC2305_FAN_MIN			0x00
> @@ -524,7 +523,7 @@ static int emc2305_probe(struct i2c_client *client, const struct i2c_device_id *
>  	struct device *dev = &client->dev;
>  	struct emc2305_data *data;
>  	struct emc2305_platform_data *pdata;
> -	int vendor, device;
> +	int vendor;
>  	int ret;
>  	int i;
>  
> @@ -535,10 +534,6 @@ static int emc2305_probe(struct i2c_client *client, const struct i2c_device_id *
>  	if (vendor != EMC2305_VENDOR)
>  		return -ENODEV;
>  
> -	device = i2c_smbus_read_byte_data(client, EMC2305_REG_DEVICE);
> -	if (device != EMC2305_DEVICE)
> -		return -ENODEV;
> -
>  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;

Re: [PATCH v3 2/2] hwmon: (emc2305) fix pwm never being able to set lower

[Guenter Roeck]

On Tue, Dec 06, 2022 at 01:53:31PM +0800, Xingjiang Qiao wrote:
> There are fields 'last_hwmon_state' and 'last_thermal_state' in the
> structure 'emc2305_cdev_data', which respectively store the cooling state
> set by the 'hwmon' and 'thermal' subsystem, and the driver author hopes
> that if the state set by 'hwmon' is lower than the value set by 'thermal',
> the driver will just save it without actually setting the pwm. Currently,
> the 'last_thermal_state' also be updated by 'hwmon', which will cause the
> cooling state to never be set to a lower value. This patch fixes that.
> 
> Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>

Applied, after renaming emc2305_set_cur_state_shim to __emc2305_set_cur_state.

Thanks,
Guenter

> ---
>  drivers/hwmon/emc2305.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
> index 9a78ca22541e..e0f6eb8d72fc 100644
> --- a/drivers/hwmon/emc2305.c
> +++ b/drivers/hwmon/emc2305.c
> @@ -171,22 +171,12 @@ static int emc2305_get_max_state(struct thermal_cooling_device *cdev, unsigned l
>  	return 0;
>  }
>  
> -static int emc2305_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> +static int emc2305_set_cur_state_shim(struct emc2305_data *data, int cdev_idx, unsigned long state)
>  {
> -	int cdev_idx, ret;
> -	struct emc2305_data *data = cdev->devdata;
> +	int ret;
>  	struct i2c_client *client = data->client;
>  	u8 val, i;
>  
> -	if (state > data->max_state)
> -		return -EINVAL;
> -
> -	cdev_idx =  emc2305_get_cdev_idx(cdev);
> -	if (cdev_idx < 0)
> -		return cdev_idx;
> -
> -	/* Save thermal state. */
> -	data->cdev_data[cdev_idx].last_thermal_state = state;
>  	state = max_t(unsigned long, state, data->cdev_data[cdev_idx].last_hwmon_state);
>  
>  	val = EMC2305_PWM_STATE2DUTY(state, data->max_state, EMC2305_FAN_MAX);
> @@ -211,6 +201,27 @@ static int emc2305_set_cur_state(struct thermal_cooling_device *cdev, unsigned l
>  	return 0;
>  }
>  
> +static int emc2305_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> +{
> +	int cdev_idx, ret;
> +	struct emc2305_data *data = cdev->devdata;
> +
> +	if (state > data->max_state)
> +		return -EINVAL;
> +
> +	cdev_idx =  emc2305_get_cdev_idx(cdev);
> +	if (cdev_idx < 0)
> +		return cdev_idx;
> +
> +	/* Save thermal state. */
> +	data->cdev_data[cdev_idx].last_thermal_state = state;
> +	ret = emc2305_set_cur_state_shim(data, cdev_idx, state);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static const struct thermal_cooling_device_ops emc2305_cooling_ops = {
>  	.get_max_state = emc2305_get_max_state,
>  	.get_cur_state = emc2305_get_cur_state,
> @@ -401,7 +412,7 @@ emc2305_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int ch
>  				 */
>  				if (data->cdev_data[cdev_idx].last_hwmon_state >=
>  				    data->cdev_data[cdev_idx].last_thermal_state)
> -					return emc2305_set_cur_state(data->cdev_data[cdev_idx].cdev,
> +					return emc2305_set_cur_state_shim(data, cdev_idx,
>  							data->cdev_data[cdev_idx].last_hwmon_state);
>  				return 0;
>  			}