Re: [PATCH 2/3] hwmon: bt1-pvt: Cache current update timeout
On Sun, Sep 20, 2020 at 02:09:22PM +0300, Serge Semin wrote:
> Instead of converting the update timeout data to the milliseconds each
> time on the read procedure let's preserve the currently set timeout in the
> dedicated driver private data cache. The cached value will be then used in
> the timeout read method and in the alarm-less data conversion to prevent
> the caller task hanging up in case if the PVT sensor is suddenly powered
> down.
>
> Fixes: 87976ce2825d ("hwmon: Add Baikal-T1 PVT sensor driver")
> Signed-off-by: Serge Semin
Applied.
Thanks,
Guenter
> ---
> drivers/hwmon/bt1-pvt.c | 85 ++---
> drivers/hwmon/bt1-pvt.h | 3 ++
> 2 files changed, 49 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/hwmon/bt1-pvt.c b/drivers/hwmon/bt1-pvt.c
> index f4b7353c078a..2600426a3b21 100644
> --- a/drivers/hwmon/bt1-pvt.c
> +++ b/drivers/hwmon/bt1-pvt.c
> @@ -655,44 +655,16 @@ static int pvt_write_trim(struct pvt_hwmon *pvt, long
> val)
>
> static int pvt_read_timeout(struct pvt_hwmon *pvt, long *val)
> {
> - unsigned long rate;
> - ktime_t kt;
> - u32 data;
> -
> - rate = clk_get_rate(pvt->clks[PVT_CLOCK_REF].clk);
> - if (!rate)
> - return -ENODEV;
> -
> - /*
> - * Don't bother with mutex here, since we just read data from MMIO.
> - * We also have to scale the ticks timeout up to compensate the
> - * ms-ns-data translations.
> - */
> - data = readl(pvt->regs + PVT_TTIMEOUT) + 1;
> + int ret;
>
> - /*
> - * Calculate ref-clock based delay (Ttotal) between two consecutive
> - * data samples of the same sensor. So we first must calculate the
> - * delay introduced by the internal ref-clock timer (Tref * Fclk).
> - * Then add the constant timeout cuased by each conversion latency
> - * (Tmin). The basic formulae for each conversion is following:
> - * Ttotal = Tref * Fclk + Tmin
> - * Note if alarms are enabled the sensors are polled one after
> - * another, so in order to have the delay being applicable for each
> - * sensor the requested value must be equally redistirbuted.
> - */
> -#if defined(CONFIG_SENSORS_BT1_PVT_ALARMS)
> - kt = ktime_set(PVT_SENSORS_NUM * (u64)data, 0);
> - kt = ktime_divns(kt, rate);
> - kt = ktime_add_ns(kt, PVT_SENSORS_NUM * PVT_TOUT_MIN);
> -#else
> - kt = ktime_set(data, 0);
> - kt = ktime_divns(kt, rate);
> - kt = ktime_add_ns(kt, PVT_TOUT_MIN);
> -#endif
> + ret = mutex_lock_interruptible(>iface_mtx);
> + if (ret)
> + return ret;
>
> /* Return the result in msec as hwmon sysfs interface requires. */
> - *val = ktime_to_ms(kt);
> + *val = ktime_to_ms(pvt->timeout);
> +
> + mutex_unlock(>iface_mtx);
>
> return 0;
> }
> @@ -700,7 +672,7 @@ static int pvt_read_timeout(struct pvt_hwmon *pvt, long
> *val)
> static int pvt_write_timeout(struct pvt_hwmon *pvt, long val)
> {
> unsigned long rate;
> - ktime_t kt;
> + ktime_t kt, cache;
> u32 data;
> int ret;
>
> @@ -713,7 +685,7 @@ static int pvt_write_timeout(struct pvt_hwmon *pvt, long
> val)
>* between all available sensors to have the requested delay
>* applicable to each individual sensor.
>*/
> - kt = ms_to_ktime(val);
> + cache = kt = ms_to_ktime(val);
> #if defined(CONFIG_SENSORS_BT1_PVT_ALARMS)
> kt = ktime_divns(kt, PVT_SENSORS_NUM);
> #endif
> @@ -742,6 +714,7 @@ static int pvt_write_timeout(struct pvt_hwmon *pvt, long
> val)
> return ret;
>
> pvt_set_tout(pvt, data);
> + pvt->timeout = cache;
>
> mutex_unlock(>iface_mtx);
>
> @@ -1018,10 +991,17 @@ static int pvt_check_pwr(struct pvt_hwmon *pvt)
> return ret;
> }
>
> -static void pvt_init_iface(struct pvt_hwmon *pvt)
> +static int pvt_init_iface(struct pvt_hwmon *pvt)
> {
> + unsigned long rate;
> u32 trim, temp;
>
> + rate = clk_get_rate(pvt->clks[PVT_CLOCK_REF].clk);
> + if (!rate) {
> + dev_err(pvt->dev, "Invalid reference clock rate\n");
> + return -ENODEV;
> + }
> +
> /*
>* Make sure all interrupts and controller are disabled so not to
>* accidentally have ISR executed before the driver data is fully
> @@ -1036,12 +1016,37 @@ static void pvt_init_iface(struct pvt_hwmon *pvt)
> pvt_set_mode(pvt, pvt_info[pvt->sensor].mode);
> pvt_set_tout(pvt, PVT_TOUT_DEF);
>
> + /*
> + * Preserve the current ref-clock based delay (Ttotal) between the
> + * sensors data samples in the driver data so not to recalculate it
> + * each time on the data requests and timeout reads. It consists of the
> + * delay introduced by the internal ref-clock timer (N / Fclk) and the
> + * constant timeout caused by each conversion latency (Tmin):
> + * Ttotal = N / Fclk + Tmin
> + * If alarms are enabled the sensors
[PATCH 2/3] hwmon: bt1-pvt: Cache current update timeout
Instead of converting the update timeout data to the milliseconds each
time on the read procedure let's preserve the currently set timeout in the
dedicated driver private data cache. The cached value will be then used in
the timeout read method and in the alarm-less data conversion to prevent
the caller task hanging up in case if the PVT sensor is suddenly powered
down.
Fixes: 87976ce2825d ("hwmon: Add Baikal-T1 PVT sensor driver")
Signed-off-by: Serge Semin
---
drivers/hwmon/bt1-pvt.c | 85 ++---
drivers/hwmon/bt1-pvt.h | 3 ++
2 files changed, 49 insertions(+), 39 deletions(-)
diff --git a/drivers/hwmon/bt1-pvt.c b/drivers/hwmon/bt1-pvt.c
index f4b7353c078a..2600426a3b21 100644
--- a/drivers/hwmon/bt1-pvt.c
+++ b/drivers/hwmon/bt1-pvt.c
@@ -655,44 +655,16 @@ static int pvt_write_trim(struct pvt_hwmon *pvt, long val)
static int pvt_read_timeout(struct pvt_hwmon *pvt, long *val)
{
- unsigned long rate;
- ktime_t kt;
- u32 data;
-
- rate = clk_get_rate(pvt->clks[PVT_CLOCK_REF].clk);
- if (!rate)
- return -ENODEV;
-
- /*
-* Don't bother with mutex here, since we just read data from MMIO.
-* We also have to scale the ticks timeout up to compensate the
-* ms-ns-data translations.
-*/
- data = readl(pvt->regs + PVT_TTIMEOUT) + 1;
+ int ret;
- /*
-* Calculate ref-clock based delay (Ttotal) between two consecutive
-* data samples of the same sensor. So we first must calculate the
-* delay introduced by the internal ref-clock timer (Tref * Fclk).
-* Then add the constant timeout cuased by each conversion latency
-* (Tmin). The basic formulae for each conversion is following:
-* Ttotal = Tref * Fclk + Tmin
-* Note if alarms are enabled the sensors are polled one after
-* another, so in order to have the delay being applicable for each
-* sensor the requested value must be equally redistirbuted.
-*/
-#if defined(CONFIG_SENSORS_BT1_PVT_ALARMS)
- kt = ktime_set(PVT_SENSORS_NUM * (u64)data, 0);
- kt = ktime_divns(kt, rate);
- kt = ktime_add_ns(kt, PVT_SENSORS_NUM * PVT_TOUT_MIN);
-#else
- kt = ktime_set(data, 0);
- kt = ktime_divns(kt, rate);
- kt = ktime_add_ns(kt, PVT_TOUT_MIN);
-#endif
+ ret = mutex_lock_interruptible(>iface_mtx);
+ if (ret)
+ return ret;
/* Return the result in msec as hwmon sysfs interface requires. */
- *val = ktime_to_ms(kt);
+ *val = ktime_to_ms(pvt->timeout);
+
+ mutex_unlock(>iface_mtx);
return 0;
}
@@ -700,7 +672,7 @@ static int pvt_read_timeout(struct pvt_hwmon *pvt, long
*val)
static int pvt_write_timeout(struct pvt_hwmon *pvt, long val)
{
unsigned long rate;
- ktime_t kt;
+ ktime_t kt, cache;
u32 data;
int ret;
@@ -713,7 +685,7 @@ static int pvt_write_timeout(struct pvt_hwmon *pvt, long
val)
* between all available sensors to have the requested delay
* applicable to each individual sensor.
*/
- kt = ms_to_ktime(val);
+ cache = kt = ms_to_ktime(val);
#if defined(CONFIG_SENSORS_BT1_PVT_ALARMS)
kt = ktime_divns(kt, PVT_SENSORS_NUM);
#endif
@@ -742,6 +714,7 @@ static int pvt_write_timeout(struct pvt_hwmon *pvt, long
val)
return ret;
pvt_set_tout(pvt, data);
+ pvt->timeout = cache;
mutex_unlock(>iface_mtx);
@@ -1018,10 +991,17 @@ static int pvt_check_pwr(struct pvt_hwmon *pvt)
return ret;
}
-static void pvt_init_iface(struct pvt_hwmon *pvt)
+static int pvt_init_iface(struct pvt_hwmon *pvt)
{
+ unsigned long rate;
u32 trim, temp;
+ rate = clk_get_rate(pvt->clks[PVT_CLOCK_REF].clk);
+ if (!rate) {
+ dev_err(pvt->dev, "Invalid reference clock rate\n");
+ return -ENODEV;
+ }
+
/*
* Make sure all interrupts and controller are disabled so not to
* accidentally have ISR executed before the driver data is fully
@@ -1036,12 +1016,37 @@ static void pvt_init_iface(struct pvt_hwmon *pvt)
pvt_set_mode(pvt, pvt_info[pvt->sensor].mode);
pvt_set_tout(pvt, PVT_TOUT_DEF);
+ /*
+* Preserve the current ref-clock based delay (Ttotal) between the
+* sensors data samples in the driver data so not to recalculate it
+* each time on the data requests and timeout reads. It consists of the
+* delay introduced by the internal ref-clock timer (N / Fclk) and the
+* constant timeout caused by each conversion latency (Tmin):
+* Ttotal = N / Fclk + Tmin
+* If alarms are enabled the sensors are polled one after another and
+* in order to get the next measurement of a particular sensor the
+* caller will have to wait for at most until all the others are
+* polled. In
