diff mbox series

[v2,06/16] hwmon: (mr75203) fix multi-channel voltage reading

Message ID 20220817054321.6519-7-farbere@amazon.com (mailing list archive)
State Changes Requested
Headers show
Series Variety of fixes and new features for mr75203 driver | expand

Commit Message

Farber, Eliav Aug. 17, 2022, 5:43 a.m. UTC 
- Fix voltage reading to support number of channels in VM IP (CH_NUM).
- Configure the ip-polling register to enable polling for all channels.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/hwmon/mr75203.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

Comments

Guenter Roeck Aug. 18, 2022, 8:03 p.m. UTC | #1 
On Wed, Aug 17, 2022 at 05:43:11AM +0000, Eliav Farber wrote:
> - Fix voltage reading to support number of channels in VM IP (CH_NUM).
> - Configure the ip-polling register to enable polling for all channels.
> 

That fails to explain what is actually wrong in the current code.
Also, one fix per patch, please.

> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  drivers/hwmon/mr75203.c | 40 +++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
> index bec63b611eb4..4419e481d47c 100644
> --- a/drivers/hwmon/mr75203.c
> +++ b/drivers/hwmon/mr75203.c
> @@ -69,8 +69,9 @@
>  
>  /* VM Individual Macro Register */
>  #define VM_COM_REG_SIZE	0x200
> -#define VM_SDIF_DONE(n)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (n))
> -#define VM_SDIF_DATA(n)	(VM_COM_REG_SIZE + 0x40 + 0x200 * (n))
> +#define VM_SDIF_DONE(vm)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (vm))
> +#define VM_SDIF_DATA(vm, ch)	\
> +	(VM_COM_REG_SIZE + 0x40 + 0x200 * (vm) + 0x4 * (ch))
>  
>  /* SDA Slave Register */
>  #define IP_CTRL			0x00
> @@ -116,6 +117,7 @@ struct pvt_device {
>  	u32			t_num;
>  	u32			p_num;
>  	u32			v_num;
> +	u32			c_num;
>  	u32			ip_freq;
>  	u8			*vm_idx;
>  };
> @@ -181,12 +183,14 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
>  	struct regmap *v_map = pvt->v_map;
>  	u32 n, stat;
>  	u8 vm_idx;
> +	u8 ch_idx;
>  	int ret;
>  
> -	if (channel >= pvt->v_num)
> +	if (channel >= pvt->v_num * pvt->c_num)
>  		return -EINVAL;
>  
> -	vm_idx = pvt->vm_idx[channel];
> +	vm_idx = pvt->vm_idx[channel / pvt->c_num];
> +	ch_idx = channel % pvt->c_num;
>  
>  	switch (attr) {
>  	case hwmon_in_input:
> @@ -197,7 +201,7 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
>  		if (ret)
>  			return ret;
>  
> -		ret = regmap_read(v_map, VM_SDIF_DATA(vm_idx), &n);
> +		ret = regmap_read(v_map, VM_SDIF_DATA(vm_idx, ch_idx), &n);
>  		if(ret < 0)
>  			return ret;
>  
> @@ -386,6 +390,20 @@ static int pvt_init(struct pvt_device *pvt)
>  		if (ret)
>  			return ret;
>  
> +		val = GENMASK(pvt->c_num - 1, 0) | VM_CH_INIT |
> +		      IP_POLL << SDIF_ADDR_SFT |
> +		      SDIF_WRN_W | SDIF_PROG;
> +		ret = regmap_write(v_map, SDIF_W, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
> +					       val, !(val & SDIF_BUSY),
> +					       PVT_POLL_DELAY_US,
> +					       PVT_POLL_TIMEOUT_US);
> +		if (ret)
> +			return ret;
> +
>  		val = CFG1_VOL_MEAS_MODE | CFG1_PARALLEL_OUT |
>  		      CFG1_14_BIT | IP_CFG << SDIF_ADDR_SFT |
>  		      SDIF_WRN_W | SDIF_PROG;
> @@ -501,7 +519,7 @@ static int pvt_reset_control_deassert(struct device *dev, struct pvt_device *pvt
>  static int mr75203_probe(struct platform_device *pdev)
>  {
>  	const struct hwmon_channel_info **pvt_info;
> -	u32 ts_num, vm_num, pd_num, val, index, i;
> +	u32 ts_num, vm_num, pd_num, ch_num, val, index, i;
>  	struct device *dev = &pdev->dev;
>  	u32 *temp_config, *in_config;
>  	struct device *hwmon_dev;
> @@ -547,9 +565,11 @@ static int mr75203_probe(struct platform_device *pdev)
>  	ts_num = (val & TS_NUM_MSK) >> TS_NUM_SFT;
>  	pd_num = (val & PD_NUM_MSK) >> PD_NUM_SFT;
>  	vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
> +	ch_num = (val & CH_NUM_MSK) >> CH_NUM_SFT;
>  	pvt->t_num = ts_num;
>  	pvt->p_num = pd_num;
>  	pvt->v_num = vm_num;
> +	pvt->c_num = ch_num;
>  	val = 0;
>  	if (ts_num)
>  		val++;
> @@ -586,6 +606,8 @@ static int mr75203_probe(struct platform_device *pdev)
>  	}
>  
>  	if (vm_num) {
> +		u32 total_ch = ch_num * vm_num;
> +
>  		ret = pvt_get_regmap(pdev, "vm", pvt);
>  		if (ret)
>  			return ret;
> @@ -614,13 +636,13 @@ static int mr75203_probe(struct platform_device *pdev)
>  			pvt->v_num = i;
>  		}
>  
> -		in_config = devm_kcalloc(dev, vm_num + 1,
> +		in_config = devm_kcalloc(dev, total_ch + 1,
>  					 sizeof(*in_config), GFP_KERNEL);
>  		if (!in_config)
>  			return -ENOMEM;
>  
> -		memset32(in_config, HWMON_I_INPUT, vm_num);
> -		in_config[vm_num] = 0;
> +		memset32(in_config, HWMON_I_INPUT, total_ch);
> +		in_config[total_ch] = 0;
>  		pvt_in.config = in_config;
>  
>  		pvt_info[index++] = &pvt_in;
Farber, Eliav Aug. 22, 2022, 12:37 p.m. UTC | #2 
On 8/18/2022 11:03 PM, Guenter Roeck wrote:
> On Wed, Aug 17, 2022 at 05:43:11AM +0000, Eliav Farber wrote:
>> - Fix voltage reading to support number of channels in VM IP (CH_NUM).
>> - Configure the ip-polling register to enable polling for all channels.
>>
>
> That fails to explain what is actually wrong in the current code.
> Also, one fix per patch, please.
I moved the configuration of the ip-polling register to a separate patch.

The problem in the current code is that it allocates in_config according
to the total number of voltage monitors and not according to the total
number of channels in all voltage monitors.
Therefore it didn’t create enough sysfs to read all inputs.
Also pvr_read_in() only tries to access the first channel in each voltage
monitor.
I will add this explanation to next version.

--
Thanks, Eliav
diff mbox series

Patch

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index bec63b611eb4..4419e481d47c 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -69,8 +69,9 @@ 
 
 /* VM Individual Macro Register */
 #define VM_COM_REG_SIZE	0x200
-#define VM_SDIF_DONE(n)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (n))
-#define VM_SDIF_DATA(n)	(VM_COM_REG_SIZE + 0x40 + 0x200 * (n))
+#define VM_SDIF_DONE(vm)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (vm))
+#define VM_SDIF_DATA(vm, ch)	\
+	(VM_COM_REG_SIZE + 0x40 + 0x200 * (vm) + 0x4 * (ch))
 
 /* SDA Slave Register */
 #define IP_CTRL			0x00
@@ -116,6 +117,7 @@  struct pvt_device {
 	u32			t_num;
 	u32			p_num;
 	u32			v_num;
+	u32			c_num;
 	u32			ip_freq;
 	u8			*vm_idx;
 };
@@ -181,12 +183,14 @@  static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 	struct regmap *v_map = pvt->v_map;
 	u32 n, stat;
 	u8 vm_idx;
+	u8 ch_idx;
 	int ret;
 
-	if (channel >= pvt->v_num)
+	if (channel >= pvt->v_num * pvt->c_num)
 		return -EINVAL;
 
-	vm_idx = pvt->vm_idx[channel];
+	vm_idx = pvt->vm_idx[channel / pvt->c_num];
+	ch_idx = channel % pvt->c_num;
 
 	switch (attr) {
 	case hwmon_in_input:
@@ -197,7 +201,7 @@  static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 		if (ret)
 			return ret;
 
-		ret = regmap_read(v_map, VM_SDIF_DATA(vm_idx), &n);
+		ret = regmap_read(v_map, VM_SDIF_DATA(vm_idx, ch_idx), &n);
 		if(ret < 0)
 			return ret;
 
@@ -386,6 +390,20 @@  static int pvt_init(struct pvt_device *pvt)
 		if (ret)
 			return ret;
 
+		val = GENMASK(pvt->c_num - 1, 0) | VM_CH_INIT |
+		      IP_POLL << SDIF_ADDR_SFT |
+		      SDIF_WRN_W | SDIF_PROG;
+		ret = regmap_write(v_map, SDIF_W, val);
+		if (ret < 0)
+			return ret;
+
+		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
+					       val, !(val & SDIF_BUSY),
+					       PVT_POLL_DELAY_US,
+					       PVT_POLL_TIMEOUT_US);
+		if (ret)
+			return ret;
+
 		val = CFG1_VOL_MEAS_MODE | CFG1_PARALLEL_OUT |
 		      CFG1_14_BIT | IP_CFG << SDIF_ADDR_SFT |
 		      SDIF_WRN_W | SDIF_PROG;
@@ -501,7 +519,7 @@  static int pvt_reset_control_deassert(struct device *dev, struct pvt_device *pvt
 static int mr75203_probe(struct platform_device *pdev)
 {
 	const struct hwmon_channel_info **pvt_info;
-	u32 ts_num, vm_num, pd_num, val, index, i;
+	u32 ts_num, vm_num, pd_num, ch_num, val, index, i;
 	struct device *dev = &pdev->dev;
 	u32 *temp_config, *in_config;
 	struct device *hwmon_dev;
@@ -547,9 +565,11 @@  static int mr75203_probe(struct platform_device *pdev)
 	ts_num = (val & TS_NUM_MSK) >> TS_NUM_SFT;
 	pd_num = (val & PD_NUM_MSK) >> PD_NUM_SFT;
 	vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
+	ch_num = (val & CH_NUM_MSK) >> CH_NUM_SFT;
 	pvt->t_num = ts_num;
 	pvt->p_num = pd_num;
 	pvt->v_num = vm_num;
+	pvt->c_num = ch_num;
 	val = 0;
 	if (ts_num)
 		val++;
@@ -586,6 +606,8 @@  static int mr75203_probe(struct platform_device *pdev)
 	}
 
 	if (vm_num) {
+		u32 total_ch = ch_num * vm_num;
+
 		ret = pvt_get_regmap(pdev, "vm", pvt);
 		if (ret)
 			return ret;
@@ -614,13 +636,13 @@  static int mr75203_probe(struct platform_device *pdev)
 			pvt->v_num = i;
 		}
 
-		in_config = devm_kcalloc(dev, vm_num + 1,
+		in_config = devm_kcalloc(dev, total_ch + 1,
 					 sizeof(*in_config), GFP_KERNEL);
 		if (!in_config)
 			return -ENOMEM;
 
-		memset32(in_config, HWMON_I_INPUT, vm_num);
-		in_config[vm_num] = 0;
+		memset32(in_config, HWMON_I_INPUT, total_ch);
+		in_config[total_ch] = 0;
 		pvt_in.config = in_config;
 
 		pvt_info[index++] = &pvt_in;