RE: [PATCH] Input: Introduce the use of managed version of kzalloc
> -Original Message-
> From: Himangi Saraogi [mailto:himangi...@gmail.com]
> Sent: 08 May 2014 05:00
> To: Support Opensource; dmitry.torok...@gmail.com; linux-
> in...@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: julia.law...@lip6.fr
> Subject: [PATCH] Input: Introduce the use of managed version of kzalloc
> This patch moves data allocated using kzalloc to managed data allocated
> using devm_kzalloc and cleans now unnecessary kfrees in probe and remove
> functions. This data is the third argument to da9052_request_irq in the two
> cases below.
The remainder of the description .
> The following Coccinelle semantic patch was used for making the change:
> @platform@
> identifier p, probefn, removefn;
> @@
> struct platform_driver p = {
> .probe = probefn,
> .remove = removefn,
> };
> @prb@
> identifier platform.probefn, pdev;
> expression e, e1, e2;
> @@
> probefn(struct platform_device *pdev, ...) {
> <+...
> - e = kzalloc(e1, e2)
> + e = devm_kzalloc(>dev, e1, e2)
> ...
> ?-kfree(e);
> ...+>
> }
> @rem depends on prb@
> identifier platform.removefn;
> expression e;
> @@
> removefn(...) {
> <...
> - kfree(e);
> ...>
> }
does not seems appropriate for the commit message, it should IMHO go in
the following section
> Signed-off-by: Himangi Saraogi
> Acked-by: Julia Lawall
> ---
> As a follow up patch I would like to know if it would be desirable to modify
> request_threaded_irq to devm_request_threaded_irq in the helper function
> da9052_request_irq :
> int da9052_request_irq(struct da9052 *da9052, int irq, char *name,
> irq_handler_t handler, void *data) {
> irq = da9052_map_irq(da9052, irq);
> if (irq < 0)
> return irq;
> return request_threaded_irq(irq, NULL, handler,
>IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>name, data);
> }
The problem here is that there is no way back to the 'dev' associated with the
irq.
To solve this requires a change to the function, or even better, just placing
the function's
code inline, in which case a two stage approach is required:- first 'inline'
the function
(from the common header file) to each of the PMIC's component drivers and second
raise a patch for each component driver do the change as you suggest which will
work because the correct 'dev' is available. Not that the second stage will
have to
wait until the first statge is actually in main-line.
> drivers/input/misc/da9052_onkey.c | 4 +---
> drivers/input/touchscreen/da9052_tsi.c | 4 +---
> 2 files changed, 2 insertions(+), 6 deletions(-)
> diff --git a/drivers/input/misc/da9052_onkey.c
> b/drivers/input/misc/da9052_onkey.c
> index 184c8f2..6fc8243 100644
> --- a/drivers/input/misc/da9052_onkey.c
> +++ b/drivers/input/misc/da9052_onkey.c
> @@ -84,7 +84,7 @@ static int da9052_onkey_probe(struct platform_device
> *pdev)
> return -EINVAL;
> }
>
> - onkey = kzalloc(sizeof(*onkey), GFP_KERNEL);
> + onkey = devm_kzalloc(>dev, sizeof(*onkey), GFP_KERNEL);
> input_dev = input_allocate_device();
> if (!onkey || !input_dev) {
> dev_err(>dev, "Failed to allocate memory\n"); @@ -
> 126,7 +126,6 @@ err_free_irq:
> cancel_delayed_work_sync(>work);
> err_free_mem:
> input_free_device(input_dev);
> - kfree(onkey);
>
> return error;
> }
> @@ -139,7 +138,6 @@ static int da9052_onkey_remove(struct
> platform_device *pdev)
> cancel_delayed_work_sync(>work);
>
> input_unregister_device(onkey->input);
> - kfree(onkey);
>
> return 0;
> }
> diff --git a/drivers/input/touchscreen/da9052_tsi.c
> b/drivers/input/touchscreen/da9052_tsi.c
> index ab64d58..dff6a2e 100644
> --- a/drivers/input/touchscreen/da9052_tsi.c
> +++ b/drivers/input/touchscreen/da9052_tsi.c
> @@ -238,7 +238,7 @@ static int da9052_ts_probe(struct platform_device
> *pdev)
> if (!da9052)
> return -EINVAL;
>
> - tsi = kzalloc(sizeof(struct da9052_tsi), GFP_KERNEL);
> + tsi = devm_kzalloc(>dev, sizeof(struct da9052_tsi),
> GFP_KERNEL);
> input_dev = input_allocate_device();
> if (!tsi || !input_dev) {
> error = -ENOMEM;
> @@ -311,7 +311,6 @@ err_free_datardy_irq:
> err_free_pendwn_irq:
> da9052_free_irq(tsi->da9052, DA9052_IRQ_PENDOWN, tsi);
> err_free_mem:
> - kfree(tsi);
> input_free_device(input_dev);
>
> return error;
> @@ -327,7 +326,6 @@ static int da9052_ts_remove(struct platform_device
> *pdev)
> da9052_free_irq(t
RE: [PATCH] Input: Introduce the use of managed version of kzalloc
-Original Message-
From: Himangi Saraogi [mailto:himangi...@gmail.com]
Sent: 08 May 2014 05:00
To: Support Opensource; dmitry.torok...@gmail.com; linux-
in...@vger.kernel.org; linux-kernel@vger.kernel.org
Cc: julia.law...@lip6.fr
Subject: [PATCH] Input: Introduce the use of managed version of kzalloc
This patch moves data allocated using kzalloc to managed data allocated
using devm_kzalloc and cleans now unnecessary kfrees in probe and remove
functions. This data is the third argument to da9052_request_irq in the two
cases below.
The remainder of the description .
The following Coccinelle semantic patch was used for making the change:
@platform@
identifier p, probefn, removefn;
@@
struct platform_driver p = {
.probe = probefn,
.remove = removefn,
};
@prb@
identifier platform.probefn, pdev;
expression e, e1, e2;
@@
probefn(struct platform_device *pdev, ...) {
+...
- e = kzalloc(e1, e2)
+ e = devm_kzalloc(pdev-dev, e1, e2)
...
?-kfree(e);
...+
}
@rem depends on prb@
identifier platform.removefn;
expression e;
@@
removefn(...) {
...
- kfree(e);
...
}
does not seems appropriate for the commit message, it should IMHO go in
the following section
Signed-off-by: Himangi Saraogi himangi...@gmail.com
Acked-by: Julia Lawall julia.law...@lip6.fr
---
As a follow up patch I would like to know if it would be desirable to modify
request_threaded_irq to devm_request_threaded_irq in the helper function
da9052_request_irq :
int da9052_request_irq(struct da9052 *da9052, int irq, char *name,
irq_handler_t handler, void *data) {
irq = da9052_map_irq(da9052, irq);
if (irq 0)
return irq;
return request_threaded_irq(irq, NULL, handler,
IRQF_TRIGGER_LOW | IRQF_ONESHOT,
name, data);
}
The problem here is that there is no way back to the 'dev' associated with the
irq.
To solve this requires a change to the function, or even better, just placing
the function's
code inline, in which case a two stage approach is required:- first 'inline'
the function
(from the common header file) to each of the PMIC's component drivers and second
raise a patch for each component driver do the change as you suggest which will
work because the correct 'dev' is available. Not that the second stage will
have to
wait until the first statge is actually in main-line.
drivers/input/misc/da9052_onkey.c | 4 +---
drivers/input/touchscreen/da9052_tsi.c | 4 +---
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/input/misc/da9052_onkey.c
b/drivers/input/misc/da9052_onkey.c
index 184c8f2..6fc8243 100644
--- a/drivers/input/misc/da9052_onkey.c
+++ b/drivers/input/misc/da9052_onkey.c
@@ -84,7 +84,7 @@ static int da9052_onkey_probe(struct platform_device
*pdev)
return -EINVAL;
}
- onkey = kzalloc(sizeof(*onkey), GFP_KERNEL);
+ onkey = devm_kzalloc(pdev-dev, sizeof(*onkey), GFP_KERNEL);
input_dev = input_allocate_device();
if (!onkey || !input_dev) {
dev_err(pdev-dev, Failed to allocate memory\n); @@ -
126,7 +126,6 @@ err_free_irq:
cancel_delayed_work_sync(onkey-work);
err_free_mem:
input_free_device(input_dev);
- kfree(onkey);
return error;
}
@@ -139,7 +138,6 @@ static int da9052_onkey_remove(struct
platform_device *pdev)
cancel_delayed_work_sync(onkey-work);
input_unregister_device(onkey-input);
- kfree(onkey);
return 0;
}
diff --git a/drivers/input/touchscreen/da9052_tsi.c
b/drivers/input/touchscreen/da9052_tsi.c
index ab64d58..dff6a2e 100644
--- a/drivers/input/touchscreen/da9052_tsi.c
+++ b/drivers/input/touchscreen/da9052_tsi.c
@@ -238,7 +238,7 @@ static int da9052_ts_probe(struct platform_device
*pdev)
if (!da9052)
return -EINVAL;
- tsi = kzalloc(sizeof(struct da9052_tsi), GFP_KERNEL);
+ tsi = devm_kzalloc(pdev-dev, sizeof(struct da9052_tsi),
GFP_KERNEL);
input_dev = input_allocate_device();
if (!tsi || !input_dev) {
error = -ENOMEM;
@@ -311,7 +311,6 @@ err_free_datardy_irq:
err_free_pendwn_irq:
da9052_free_irq(tsi-da9052, DA9052_IRQ_PENDOWN, tsi);
err_free_mem:
- kfree(tsi);
input_free_device(input_dev);
return error;
@@ -327,7 +326,6 @@ static int da9052_ts_remove(struct platform_device
*pdev)
da9052_free_irq(tsi-da9052, DA9052_IRQ_PENDOWN, tsi);
input_unregister_device(tsi-dev);
- kfree(tsi);
return 0;
}
--
1.9.1
for the content of this patch:
Acked-by: Opensource [Anthony Olech] anthony.olech.opensou...@diasemi.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http
RE: [PATCH V1] drivers/rtc: da9052: ALARM causes interrupt storm
Hi Alessandro,
you did not reply to my patch submission.
Why not?
Tony Olech
Dialog Semiconductor
> -Original Message-
> From: Opensource [Anthony Olech]
> [mailto:anthony.olech.opensou...@diasemi.com]
> Sent: 02 April 2014 12:46
> To: Alessandro Zummo; Support Opensource
> Cc: linux-kernel@vger.kernel.org; rtc-li...@googlegroups.com; David Dajun
> Chen
> Subject: [PATCH V1] drivers/rtc: da9052: ALARM causes interrupt storm
>
> Setting the alarm to a time not on a minute boundary results in repeated
> interrupts being generated by the DA9052/3 PMIC device until the kernel RTC
> core sees that the alarm has rung. Sometimes the number and frequency of
> interrupts can cause the kernel to disable the IRQ line used by the
> DA9052/3 PMIC with disasterous consequences. This patch fixes the
> problem.
>
> Even though the DA9052/3 PMIC is capable generating periodic interrupts, ie
> TICKS, the method used to distinguish RTC_AF from RTC_PF events was
> flawed and can not work in conjunction with the regmap_irq kernel core.
> Thus that flawed detection has also been removed by the DA9052/3 PMIC
> RTC driver's irq handler, so that it no longer reports the wrong type of event
> to the kernel RTC core.
>
> The internal static functions within the DA9052/3 PMIC RTC driver have been
> changed to pass the 'da9052_rtc' structure instead of the 'da9052'
> because there is no backwards pointer from the 'da9052' structure.
>
> Signed-off-by: Anthony Olech
> ---
>
> This patch is relative to linux-next repository tag next-20140402
>
> This patch fixes the three issues described above. The first is serious
> because
> usiing the RTC alarm set to a non minute boundary will eventually cause all
> component drivers that depend on the interrupt line to fail. The solution
> adopted is to round up to alarm time to the next highest minute.
>
> The second bug, reporting a RTC_PF event instead of an RTC_AF event turns
> out to not matter with the current implementation of the kernel RTC core as
> it seems to ignore the event type. However, should that change in the future
> it is better to fix the issue now and not have 'problems waiting to happen'
>
> The third set of changes are to make the da9052_rtc structure available to all
> the local internal functions in the driver. This was done during testing so
> that
> diagnostic data could be stored there. Should the solution to the first issue
> be found not acceptable, then the alternative of using the TICKS interrupt at
> the fixed one second interval in order to step to the exact second of the
> requested alarm requires an extra (alarm time) piece of data to be stored. In
> devices that use the alarm function to wake up from sleep, accuracy to the
> second will result in the device being awake for up to nearly a minute longer
> than expected.
>
> drivers/rtc/rtc-da9052.c | 122 +-
>
> 1 file changed, 66 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/rtc/rtc-da9052.c b/drivers/rtc/rtc-da9052.c index
> a1cbf64..e5c9486 100644
> --- a/drivers/rtc/rtc-da9052.c
> +++ b/drivers/rtc/rtc-da9052.c
> @@ -20,28 +20,28 @@
> #include
> #include
>
> -#define rtc_err(da9052, fmt, ...) \
> - dev_err(da9052->dev, "%s: " fmt, __func__,
> ##__VA_ARGS__)
> +#define rtc_err(rtc, fmt, ...) \
> + dev_err(rtc->da9052->dev, "%s: " fmt, __func__,
> ##__VA_ARGS__)
>
> struct da9052_rtc {
> struct rtc_device *rtc;
> struct da9052 *da9052;
> };
>
> -static int da9052_rtc_enable_alarm(struct da9052 *da9052, bool enable)
> +static int da9052_rtc_enable_alarm(struct da9052_rtc *rtc, bool enable)
> {
> int ret;
> if (enable) {
> - ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG,
> - DA9052_ALARM_Y_ALARM_ON,
> - DA9052_ALARM_Y_ALARM_ON);
> + ret = da9052_reg_update(rtc->da9052,
> DA9052_ALARM_Y_REG,
> +
> DA9052_ALARM_Y_ALARM_ON|DA9052_ALARM_Y_TICK_ON,
> + DA9052_ALARM_Y_ALARM_ON);
> if (ret != 0)
> - rtc_err(da9052, "Failed to enable ALM: %d\n", ret);
> + rtc_err(rtc, "Failed to enable ALM: %d\n", ret);
> } else {
> - ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG,
> - DA9052_ALARM_Y_ALARM_ON, 0);
> + ret = da9052_reg_update(rtc->da9052,
> DA9052_ALARM_Y_REG,
> +
> DA9052_ALARM_Y_ALARM_ON|DA9052_ALARM_Y_TICK_ON, 0);
> if (ret != 0)
> -
RE: [PATCH V1] drivers/rtc: da9052: ALARM causes interrupt storm
Hi Alessandro,
you did not reply to my patch submission.
Why not?
Tony Olech
Dialog Semiconductor
-Original Message-
From: Opensource [Anthony Olech]
[mailto:anthony.olech.opensou...@diasemi.com]
Sent: 02 April 2014 12:46
To: Alessandro Zummo; Support Opensource
Cc: linux-kernel@vger.kernel.org; rtc-li...@googlegroups.com; David Dajun
Chen
Subject: [PATCH V1] drivers/rtc: da9052: ALARM causes interrupt storm
Setting the alarm to a time not on a minute boundary results in repeated
interrupts being generated by the DA9052/3 PMIC device until the kernel RTC
core sees that the alarm has rung. Sometimes the number and frequency of
interrupts can cause the kernel to disable the IRQ line used by the
DA9052/3 PMIC with disasterous consequences. This patch fixes the
problem.
Even though the DA9052/3 PMIC is capable generating periodic interrupts, ie
TICKS, the method used to distinguish RTC_AF from RTC_PF events was
flawed and can not work in conjunction with the regmap_irq kernel core.
Thus that flawed detection has also been removed by the DA9052/3 PMIC
RTC driver's irq handler, so that it no longer reports the wrong type of event
to the kernel RTC core.
The internal static functions within the DA9052/3 PMIC RTC driver have been
changed to pass the 'da9052_rtc' structure instead of the 'da9052'
because there is no backwards pointer from the 'da9052' structure.
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
---
This patch is relative to linux-next repository tag next-20140402
This patch fixes the three issues described above. The first is serious
because
usiing the RTC alarm set to a non minute boundary will eventually cause all
component drivers that depend on the interrupt line to fail. The solution
adopted is to round up to alarm time to the next highest minute.
The second bug, reporting a RTC_PF event instead of an RTC_AF event turns
out to not matter with the current implementation of the kernel RTC core as
it seems to ignore the event type. However, should that change in the future
it is better to fix the issue now and not have 'problems waiting to happen'
The third set of changes are to make the da9052_rtc structure available to all
the local internal functions in the driver. This was done during testing so
that
diagnostic data could be stored there. Should the solution to the first issue
be found not acceptable, then the alternative of using the TICKS interrupt at
the fixed one second interval in order to step to the exact second of the
requested alarm requires an extra (alarm time) piece of data to be stored. In
devices that use the alarm function to wake up from sleep, accuracy to the
second will result in the device being awake for up to nearly a minute longer
than expected.
drivers/rtc/rtc-da9052.c | 122 +-
1 file changed, 66 insertions(+), 56 deletions(-)
diff --git a/drivers/rtc/rtc-da9052.c b/drivers/rtc/rtc-da9052.c index
a1cbf64..e5c9486 100644
--- a/drivers/rtc/rtc-da9052.c
+++ b/drivers/rtc/rtc-da9052.c
@@ -20,28 +20,28 @@
#include linux/mfd/da9052/da9052.h
#include linux/mfd/da9052/reg.h
-#define rtc_err(da9052, fmt, ...) \
- dev_err(da9052-dev, %s: fmt, __func__,
##__VA_ARGS__)
+#define rtc_err(rtc, fmt, ...) \
+ dev_err(rtc-da9052-dev, %s: fmt, __func__,
##__VA_ARGS__)
struct da9052_rtc {
struct rtc_device *rtc;
struct da9052 *da9052;
};
-static int da9052_rtc_enable_alarm(struct da9052 *da9052, bool enable)
+static int da9052_rtc_enable_alarm(struct da9052_rtc *rtc, bool enable)
{
int ret;
if (enable) {
- ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG,
- DA9052_ALARM_Y_ALARM_ON,
- DA9052_ALARM_Y_ALARM_ON);
+ ret = da9052_reg_update(rtc-da9052,
DA9052_ALARM_Y_REG,
+
DA9052_ALARM_Y_ALARM_ON|DA9052_ALARM_Y_TICK_ON,
+ DA9052_ALARM_Y_ALARM_ON);
if (ret != 0)
- rtc_err(da9052, Failed to enable ALM: %d\n, ret);
+ rtc_err(rtc, Failed to enable ALM: %d\n, ret);
} else {
- ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG,
- DA9052_ALARM_Y_ALARM_ON, 0);
+ ret = da9052_reg_update(rtc-da9052,
DA9052_ALARM_Y_REG,
+
DA9052_ALARM_Y_ALARM_ON|DA9052_ALARM_Y_TICK_ON, 0);
if (ret != 0)
- rtc_err(da9052, Write error: %d\n, ret);
+ rtc_err(rtc, Write error: %d\n, ret);
}
return ret;
}
@@ -49,31 +49,20 @@ static int da9052_rtc_enable_alarm(struct da9052
*da9052, bool enable) static irqreturn_t da9052_rtc_irq(int irq, void *data)
{
struct da9052_rtc *rtc = data;
- int ret;
- ret = da9052_reg_read(rtc-da9052
[PATCH V1] drivers/mfd: da9052: use multiwrite mode
Use the new regmap core API regmap_multi_reg_write(), to prevent a rare
problem with the Dialog DA9052/3 PMIC devices that causes the device to
fail.
Signed-off-by: Anthony Olech
---
This patch is relative to linux-next repository tag next-20140403
Even though the probability of the problem occurring is exceedingly rare,
the consequences are a bricked device and so this workround is essential.
The patch has been tested using the RTC ALARM function in conjuctions with
an I2C logic analyser.
drivers/mfd/da9052-i2c.c | 34 +-
include/linux/mfd/da9052/da9052.h | 24
2 files changed, 25 insertions(+), 33 deletions(-)
diff --git a/drivers/mfd/da9052-i2c.c b/drivers/mfd/da9052-i2c.c
index 6da8ec8..ca6b4f6 100644
--- a/drivers/mfd/da9052-i2c.c
+++ b/drivers/mfd/da9052-i2c.c
@@ -87,27 +87,11 @@ static int da9052_i2c_fix(struct da9052 *da9052, unsigned
char reg)
return 0;
}
-/*
- * According to errata item 24, multiwrite mode should be avoided
- * in order to prevent register data corruption after power-down.
- */
-static int da9052_i2c_disable_multiwrite(struct da9052 *da9052)
+static int da9052_i2c_config_multiwrite(struct da9052 *da9052, bool enable)
{
- int reg_val, ret;
-
- ret = regmap_read(da9052->regmap, DA9052_CONTROL_B_REG, _val);
- if (ret < 0)
- return ret;
-
- if (!(reg_val & DA9052_CONTROL_B_WRITEMODE)) {
- reg_val |= DA9052_CONTROL_B_WRITEMODE;
- ret = regmap_write(da9052->regmap, DA9052_CONTROL_B_REG,
- reg_val);
- if (ret < 0)
- return ret;
- }
-
- return 0;
+ return regmap_update_bits(da9052->regmap, DA9052_CONTROL_B_REG,
+ DA9052_CONTROL_B_WRITEMODE,
+ enable ? 0xFF : 0);
}
static const struct i2c_device_id da9052_i2c_id[] = {
@@ -153,6 +137,8 @@ static int da9052_i2c_probe(struct i2c_client *client,
i2c_set_clientdata(client, da9052);
+ da9052_regmap_config.can_multi_write = true;
+
da9052->regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(da9052->regmap)) {
ret = PTR_ERR(da9052->regmap);
@@ -161,7 +147,8 @@ static int da9052_i2c_probe(struct i2c_client *client,
return ret;
}
- ret = da9052_i2c_disable_multiwrite(da9052);
+ ret = da9052_i2c_config_multiwrite(da9052,
+ da9052_regmap_config.can_multi_write);
if (ret < 0)
return ret;
@@ -182,10 +169,7 @@ static int da9052_i2c_probe(struct i2c_client *client,
}
ret = da9052_device_init(da9052, id->driver_data);
- if (ret != 0)
- return ret;
-
- return 0;
+ return ret;
}
static int da9052_i2c_remove(struct i2c_client *client)
diff --git a/include/linux/mfd/da9052/da9052.h
b/include/linux/mfd/da9052/da9052.h
index bba65f5..967c802 100644
--- a/include/linux/mfd/da9052/da9052.h
+++ b/include/linux/mfd/da9052/da9052.h
@@ -172,20 +172,28 @@ static inline int da9052_group_write(struct da9052
*da9052, unsigned char reg,
unsigned reg_cnt, unsigned char *val)
{
int ret;
+ unsigned char r = reg;
+ struct reg_default *regs;
int i;
+ regs = kmalloc(sizeof(struct reg_default)*reg_cnt, GFP_KERNEL);
+ if (!regs)
+ return -ENOMEM;
+
for (i = 0; i < reg_cnt; i++) {
- ret = regmap_write(da9052->regmap, reg + i, val[i]);
- if (ret < 0)
- return ret;
+ regs[i].reg = r++;
+ regs[i].def = val[i];
}
- if (da9052->fix_io) {
- ret = da9052->fix_io(da9052, reg);
- if (ret < 0)
- return ret;
- }
+ ret = regmap_multi_reg_write(da9052->regmap, regs, reg_cnt);
+
+ kfree(regs);
+
+ if (ret < 0)
+ return ret;
+ if (da9052->fix_io)
+ ret = da9052->fix_io(da9052, reg+reg_cnt-1);
return ret;
}
--
end-of-patch 1/1 for drivers/mfd: da9052: use multiwrite mode V1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V1] drivers/mfd: da9052: use multiwrite mode
Use the new regmap core API regmap_multi_reg_write(), to prevent a rare
problem with the Dialog DA9052/3 PMIC devices that causes the device to
fail.
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
---
This patch is relative to linux-next repository tag next-20140403
Even though the probability of the problem occurring is exceedingly rare,
the consequences are a bricked device and so this workround is essential.
The patch has been tested using the RTC ALARM function in conjuctions with
an I2C logic analyser.
drivers/mfd/da9052-i2c.c | 34 +-
include/linux/mfd/da9052/da9052.h | 24
2 files changed, 25 insertions(+), 33 deletions(-)
diff --git a/drivers/mfd/da9052-i2c.c b/drivers/mfd/da9052-i2c.c
index 6da8ec8..ca6b4f6 100644
--- a/drivers/mfd/da9052-i2c.c
+++ b/drivers/mfd/da9052-i2c.c
@@ -87,27 +87,11 @@ static int da9052_i2c_fix(struct da9052 *da9052, unsigned
char reg)
return 0;
}
-/*
- * According to errata item 24, multiwrite mode should be avoided
- * in order to prevent register data corruption after power-down.
- */
-static int da9052_i2c_disable_multiwrite(struct da9052 *da9052)
+static int da9052_i2c_config_multiwrite(struct da9052 *da9052, bool enable)
{
- int reg_val, ret;
-
- ret = regmap_read(da9052-regmap, DA9052_CONTROL_B_REG, reg_val);
- if (ret 0)
- return ret;
-
- if (!(reg_val DA9052_CONTROL_B_WRITEMODE)) {
- reg_val |= DA9052_CONTROL_B_WRITEMODE;
- ret = regmap_write(da9052-regmap, DA9052_CONTROL_B_REG,
- reg_val);
- if (ret 0)
- return ret;
- }
-
- return 0;
+ return regmap_update_bits(da9052-regmap, DA9052_CONTROL_B_REG,
+ DA9052_CONTROL_B_WRITEMODE,
+ enable ? 0xFF : 0);
}
static const struct i2c_device_id da9052_i2c_id[] = {
@@ -153,6 +137,8 @@ static int da9052_i2c_probe(struct i2c_client *client,
i2c_set_clientdata(client, da9052);
+ da9052_regmap_config.can_multi_write = true;
+
da9052-regmap = devm_regmap_init_i2c(client, da9052_regmap_config);
if (IS_ERR(da9052-regmap)) {
ret = PTR_ERR(da9052-regmap);
@@ -161,7 +147,8 @@ static int da9052_i2c_probe(struct i2c_client *client,
return ret;
}
- ret = da9052_i2c_disable_multiwrite(da9052);
+ ret = da9052_i2c_config_multiwrite(da9052,
+ da9052_regmap_config.can_multi_write);
if (ret 0)
return ret;
@@ -182,10 +169,7 @@ static int da9052_i2c_probe(struct i2c_client *client,
}
ret = da9052_device_init(da9052, id-driver_data);
- if (ret != 0)
- return ret;
-
- return 0;
+ return ret;
}
static int da9052_i2c_remove(struct i2c_client *client)
diff --git a/include/linux/mfd/da9052/da9052.h
b/include/linux/mfd/da9052/da9052.h
index bba65f5..967c802 100644
--- a/include/linux/mfd/da9052/da9052.h
+++ b/include/linux/mfd/da9052/da9052.h
@@ -172,20 +172,28 @@ static inline int da9052_group_write(struct da9052
*da9052, unsigned char reg,
unsigned reg_cnt, unsigned char *val)
{
int ret;
+ unsigned char r = reg;
+ struct reg_default *regs;
int i;
+ regs = kmalloc(sizeof(struct reg_default)*reg_cnt, GFP_KERNEL);
+ if (!regs)
+ return -ENOMEM;
+
for (i = 0; i reg_cnt; i++) {
- ret = regmap_write(da9052-regmap, reg + i, val[i]);
- if (ret 0)
- return ret;
+ regs[i].reg = r++;
+ regs[i].def = val[i];
}
- if (da9052-fix_io) {
- ret = da9052-fix_io(da9052, reg);
- if (ret 0)
- return ret;
- }
+ ret = regmap_multi_reg_write(da9052-regmap, regs, reg_cnt);
+
+ kfree(regs);
+
+ if (ret 0)
+ return ret;
+ if (da9052-fix_io)
+ ret = da9052-fix_io(da9052, reg+reg_cnt-1);
return ret;
}
--
end-of-patch 1/1 for drivers/mfd: da9052: use multiwrite mode V1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V1] drivers/rtc: da9052: ALARM causes interrupt storm
Setting the alarm to a time not on a minute boundary results in repeated
interrupts being generated by the DA9052/3 PMIC device until the kernel
RTC core sees that the alarm has rung. Sometimes the number and frequency
of interrupts can cause the kernel to disable the IRQ line used by the
DA9052/3 PMIC with disasterous consequences. This patch fixes the problem.
Even though the DA9052/3 PMIC is capable generating periodic interrupts,
ie TICKS, the method used to distinguish RTC_AF from RTC_PF events was
flawed and can not work in conjunction with the regmap_irq kernel core.
Thus that flawed detection has also been removed by the DA9052/3 PMIC RTC
driver's irq handler, so that it no longer reports the wrong type of event
to the kernel RTC core.
The internal static functions within the DA9052/3 PMIC RTC driver have
been changed to pass the 'da9052_rtc' structure instead of the 'da9052'
because there is no backwards pointer from the 'da9052' structure.
Signed-off-by: Anthony Olech
---
This patch is relative to linux-next repository tag next-20140402
This patch fixes the three issues described above. The first is serious
because usiing the RTC alarm set to a non minute boundary will eventually
cause all component drivers that depend on the interrupt line to fail. The
solution adopted is to round up to alarm time to the next highest minute.
The second bug, reporting a RTC_PF event instead of an RTC_AF event turns
out to not matter with the current implementation of the kernel RTC core
as it seems to ignore the event type. However, should that change in the
future it is better to fix the issue now and not have 'problems waiting to
happen'
The third set of changes are to make the da9052_rtc structure available
to all the local internal functions in the driver. This was done during
testing so that diagnostic data could be stored there. Should the solution
to the first issue be found not acceptable, then the alternative of using
the TICKS interrupt at the fixed one second interval in order to step to
the exact second of the requested alarm requires an extra (alarm time)
piece of data to be stored. In devices that use the alarm function to wake
up from sleep, accuracy to the second will result in the device being
awake for up to nearly a minute longer than expected.
drivers/rtc/rtc-da9052.c | 122 +-
1 file changed, 66 insertions(+), 56 deletions(-)
diff --git a/drivers/rtc/rtc-da9052.c b/drivers/rtc/rtc-da9052.c
index a1cbf64..e5c9486 100644
--- a/drivers/rtc/rtc-da9052.c
+++ b/drivers/rtc/rtc-da9052.c
@@ -20,28 +20,28 @@
#include
#include
-#define rtc_err(da9052, fmt, ...) \
- dev_err(da9052->dev, "%s: " fmt, __func__, ##__VA_ARGS__)
+#define rtc_err(rtc, fmt, ...) \
+ dev_err(rtc->da9052->dev, "%s: " fmt, __func__, ##__VA_ARGS__)
struct da9052_rtc {
struct rtc_device *rtc;
struct da9052 *da9052;
};
-static int da9052_rtc_enable_alarm(struct da9052 *da9052, bool enable)
+static int da9052_rtc_enable_alarm(struct da9052_rtc *rtc, bool enable)
{
int ret;
if (enable) {
- ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG,
- DA9052_ALARM_Y_ALARM_ON,
- DA9052_ALARM_Y_ALARM_ON);
+ ret = da9052_reg_update(rtc->da9052, DA9052_ALARM_Y_REG,
+ DA9052_ALARM_Y_ALARM_ON|DA9052_ALARM_Y_TICK_ON,
+ DA9052_ALARM_Y_ALARM_ON);
if (ret != 0)
- rtc_err(da9052, "Failed to enable ALM: %d\n", ret);
+ rtc_err(rtc, "Failed to enable ALM: %d\n", ret);
} else {
- ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG,
- DA9052_ALARM_Y_ALARM_ON, 0);
+ ret = da9052_reg_update(rtc->da9052, DA9052_ALARM_Y_REG,
+ DA9052_ALARM_Y_ALARM_ON|DA9052_ALARM_Y_TICK_ON, 0);
if (ret != 0)
- rtc_err(da9052, "Write error: %d\n", ret);
+ rtc_err(rtc, "Write error: %d\n", ret);
}
return ret;
}
@@ -49,31 +49,20 @@ static int da9052_rtc_enable_alarm(struct da9052 *da9052,
bool enable)
static irqreturn_t da9052_rtc_irq(int irq, void *data)
{
struct da9052_rtc *rtc = data;
- int ret;
- ret = da9052_reg_read(rtc->da9052, DA9052_ALARM_MI_REG);
- if (ret < 0) {
- rtc_err(rtc->da9052, "Read error: %d\n", ret);
- return IRQ_NONE;
- }
-
- if (ret & DA9052_ALARMMI_ALARMTYPE) {
- da9052_rtc_enable_alarm(rtc->da9052, 0);
- rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF);
- } else
- rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_PF);
+ rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF);
return IRQ_HANDLED;
}
-static int
[PATCH V1] drivers/rtc: da9052: ALARM causes interrupt storm
Setting the alarm to a time not on a minute boundary results in repeated
interrupts being generated by the DA9052/3 PMIC device until the kernel
RTC core sees that the alarm has rung. Sometimes the number and frequency
of interrupts can cause the kernel to disable the IRQ line used by the
DA9052/3 PMIC with disasterous consequences. This patch fixes the problem.
Even though the DA9052/3 PMIC is capable generating periodic interrupts,
ie TICKS, the method used to distinguish RTC_AF from RTC_PF events was
flawed and can not work in conjunction with the regmap_irq kernel core.
Thus that flawed detection has also been removed by the DA9052/3 PMIC RTC
driver's irq handler, so that it no longer reports the wrong type of event
to the kernel RTC core.
The internal static functions within the DA9052/3 PMIC RTC driver have
been changed to pass the 'da9052_rtc' structure instead of the 'da9052'
because there is no backwards pointer from the 'da9052' structure.
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
---
This patch is relative to linux-next repository tag next-20140402
This patch fixes the three issues described above. The first is serious
because usiing the RTC alarm set to a non minute boundary will eventually
cause all component drivers that depend on the interrupt line to fail. The
solution adopted is to round up to alarm time to the next highest minute.
The second bug, reporting a RTC_PF event instead of an RTC_AF event turns
out to not matter with the current implementation of the kernel RTC core
as it seems to ignore the event type. However, should that change in the
future it is better to fix the issue now and not have 'problems waiting to
happen'
The third set of changes are to make the da9052_rtc structure available
to all the local internal functions in the driver. This was done during
testing so that diagnostic data could be stored there. Should the solution
to the first issue be found not acceptable, then the alternative of using
the TICKS interrupt at the fixed one second interval in order to step to
the exact second of the requested alarm requires an extra (alarm time)
piece of data to be stored. In devices that use the alarm function to wake
up from sleep, accuracy to the second will result in the device being
awake for up to nearly a minute longer than expected.
drivers/rtc/rtc-da9052.c | 122 +-
1 file changed, 66 insertions(+), 56 deletions(-)
diff --git a/drivers/rtc/rtc-da9052.c b/drivers/rtc/rtc-da9052.c
index a1cbf64..e5c9486 100644
--- a/drivers/rtc/rtc-da9052.c
+++ b/drivers/rtc/rtc-da9052.c
@@ -20,28 +20,28 @@
#include linux/mfd/da9052/da9052.h
#include linux/mfd/da9052/reg.h
-#define rtc_err(da9052, fmt, ...) \
- dev_err(da9052-dev, %s: fmt, __func__, ##__VA_ARGS__)
+#define rtc_err(rtc, fmt, ...) \
+ dev_err(rtc-da9052-dev, %s: fmt, __func__, ##__VA_ARGS__)
struct da9052_rtc {
struct rtc_device *rtc;
struct da9052 *da9052;
};
-static int da9052_rtc_enable_alarm(struct da9052 *da9052, bool enable)
+static int da9052_rtc_enable_alarm(struct da9052_rtc *rtc, bool enable)
{
int ret;
if (enable) {
- ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG,
- DA9052_ALARM_Y_ALARM_ON,
- DA9052_ALARM_Y_ALARM_ON);
+ ret = da9052_reg_update(rtc-da9052, DA9052_ALARM_Y_REG,
+ DA9052_ALARM_Y_ALARM_ON|DA9052_ALARM_Y_TICK_ON,
+ DA9052_ALARM_Y_ALARM_ON);
if (ret != 0)
- rtc_err(da9052, Failed to enable ALM: %d\n, ret);
+ rtc_err(rtc, Failed to enable ALM: %d\n, ret);
} else {
- ret = da9052_reg_update(da9052, DA9052_ALARM_Y_REG,
- DA9052_ALARM_Y_ALARM_ON, 0);
+ ret = da9052_reg_update(rtc-da9052, DA9052_ALARM_Y_REG,
+ DA9052_ALARM_Y_ALARM_ON|DA9052_ALARM_Y_TICK_ON, 0);
if (ret != 0)
- rtc_err(da9052, Write error: %d\n, ret);
+ rtc_err(rtc, Write error: %d\n, ret);
}
return ret;
}
@@ -49,31 +49,20 @@ static int da9052_rtc_enable_alarm(struct da9052 *da9052,
bool enable)
static irqreturn_t da9052_rtc_irq(int irq, void *data)
{
struct da9052_rtc *rtc = data;
- int ret;
- ret = da9052_reg_read(rtc-da9052, DA9052_ALARM_MI_REG);
- if (ret 0) {
- rtc_err(rtc-da9052, Read error: %d\n, ret);
- return IRQ_NONE;
- }
-
- if (ret DA9052_ALARMMI_ALARMTYPE) {
- da9052_rtc_enable_alarm(rtc-da9052, 0);
- rtc_update_irq(rtc-rtc, 1, RTC_IRQF | RTC_AF);
- } else
- rtc_update_irq(rtc-rtc, 1, RTC_IRQF | RTC_PF);
+ rtc_update_irq(rtc-rtc, 1, RTC_IRQF | RTC_AF);
[RFC V3] drivers/base/regmap: Implementation for regmap_multi_reg_write
This is the implementation of regmap_multi_reg_write()
There is a new capability 'can_multi_write' that device drivers
must set in order to use this multi reg write mode.
This replaces the first definition, which just defined the API.
Signed-off-by: Anthony Olech
---
This patch is relative to linux-next repository tag next-20140304
This 3rd RFC attempt adds a 'can_multi_write' config capability,
that is initialized by a device driver through regmap_init().
If a driver making a call to regmap_multi_reg_write() has not
previously set the 'can_multi_write' config capability then the
implementation will just change the transfer to a sequence of
single register writes.
If there is a different or better way to advertise a device
capability please respond to this RFC.
The API definition of regmap_multi_reg_write() has been in the
kernel mainline since v3.13. This patch suggests an implementation
that works for DA9052 family of PMIC chips from Dialog Semiconductor.
This implementation will work in the presense of register ranges
because the algorithm will chop the set of (reg,val) changes each
time the page changes. This part of the algorithm will be need
for other Dialog PMIC that both need to use the Multi Register
Write mode and use paged registers.
The big difference between this attempt and V1 is that the
set of (reg,val) changes are now treated as 'const ...' and
the simplest option of doing a pre pass to catch possible
register changes to page relative is done so that in-situ
changes can be made to an kalloc'ed copy.
A minor change is moving knowledge of the range structure
from _regmap_range_multi_paged_reg_write() to a new static
function _regmap_register_page(). Specifically the function
_regmap_select_page() will send an I2C command if the register
is in a new page, whereas the algorothm needs to know in advance
when the page is going to change so that the fragment of multi
register writes can be sent out to the I2C device.
Please feel free to comment on this implementation. This patch
has only had limited testing with a DA9053 chip so the real
attempt to submit into the MainLine will come later giving any
reviewer ample time to make constructive comments.
drivers/base/regmap/internal.h |2 +
drivers/base/regmap/regmap.c | 188
include/linux/regmap.h |4 +
3 files changed, 178 insertions(+), 16 deletions(-)
diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 33414b1..7d13269 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -134,6 +134,8 @@ struct regmap {
/* if set, converts bulk rw to single rw */
bool use_single_rw;
+ /* if set, the device supports multi write mode */
+ bool can_multi_write;
struct rb_root range_tree;
void *selector_work_buf;/* Scratch buffer used for selector */
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 0e5c833..43c7bca 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -461,6 +461,7 @@ struct regmap *regmap_init(struct device *dev,
else
map->reg_stride = 1;
map->use_single_rw = config->use_single_rw;
+ map->can_multi_write = config->can_multi_write;
map->dev = dev;
map->bus = bus;
map->bus_context = bus_context;
@@ -1591,41 +1592,196 @@ out:
}
EXPORT_SYMBOL_GPL(regmap_bulk_write);
+/*
+ * _regmap_raw_multi_reg_write()
+ *
+ * the (register,newvalue) pairs in regs have not been formatted, but
+ * they are all in the same page and have been changed to being page
+ * relative. The page register has been written if that was neccessary.
+ */
+static int _regmap_raw_multi_reg_write(struct regmap *map,
+ const struct reg_default *regs,
+ size_t num_regs)
+{
+ int ret;
+ void *buf;
+ int i;
+ u8 *u8;
+ size_t val_bytes = map->format.val_bytes;
+ size_t reg_bytes = map->format.reg_bytes;
+ size_t pad_bytes = map->format.pad_bytes;
+ size_t pair_size = reg_bytes + pad_bytes + val_bytes;
+ size_t len = pair_size * num_regs;
+
+ buf = kzalloc(len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ /* We have to linearise by hand. */
+
+ u8 = buf;
+
+ for (i = 0; i < num_regs; i++) {
+ int reg = regs[i].reg;
+ int val = regs[i].def;
+ trace_regmap_hw_write_start(map->dev, reg, 1);
+ map->format.format_reg(u8, reg, map->reg_shift);
+ u8 += reg_bytes + pad_bytes;
+ map->format.format_val(u8, val, 0);
+ u8 += val_bytes;
+ }
+ u8 = buf;
+ *u8 |= map->write_flag_mask;
+
+ ret = map->bus->write(map->bus_context, buf, len);
+
+ kfree(buf);
+
+ for (i = 0; i < num_regs; i++) {
+
[RFC V3] drivers/base/regmap: Implementation for regmap_multi_reg_write
This is the implementation of regmap_multi_reg_write()
There is a new capability 'can_multi_write' that device drivers
must set in order to use this multi reg write mode.
This replaces the first definition, which just defined the API.
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
---
This patch is relative to linux-next repository tag next-20140304
This 3rd RFC attempt adds a 'can_multi_write' config capability,
that is initialized by a device driver through regmap_init().
If a driver making a call to regmap_multi_reg_write() has not
previously set the 'can_multi_write' config capability then the
implementation will just change the transfer to a sequence of
single register writes.
If there is a different or better way to advertise a device
capability please respond to this RFC.
The API definition of regmap_multi_reg_write() has been in the
kernel mainline since v3.13. This patch suggests an implementation
that works for DA9052 family of PMIC chips from Dialog Semiconductor.
This implementation will work in the presense of register ranges
because the algorithm will chop the set of (reg,val) changes each
time the page changes. This part of the algorithm will be need
for other Dialog PMIC that both need to use the Multi Register
Write mode and use paged registers.
The big difference between this attempt and V1 is that the
set of (reg,val) changes are now treated as 'const ...' and
the simplest option of doing a pre pass to catch possible
register changes to page relative is done so that in-situ
changes can be made to an kalloc'ed copy.
A minor change is moving knowledge of the range structure
from _regmap_range_multi_paged_reg_write() to a new static
function _regmap_register_page(). Specifically the function
_regmap_select_page() will send an I2C command if the register
is in a new page, whereas the algorothm needs to know in advance
when the page is going to change so that the fragment of multi
register writes can be sent out to the I2C device.
Please feel free to comment on this implementation. This patch
has only had limited testing with a DA9053 chip so the real
attempt to submit into the MainLine will come later giving any
reviewer ample time to make constructive comments.
drivers/base/regmap/internal.h |2 +
drivers/base/regmap/regmap.c | 188
include/linux/regmap.h |4 +
3 files changed, 178 insertions(+), 16 deletions(-)
diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 33414b1..7d13269 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -134,6 +134,8 @@ struct regmap {
/* if set, converts bulk rw to single rw */
bool use_single_rw;
+ /* if set, the device supports multi write mode */
+ bool can_multi_write;
struct rb_root range_tree;
void *selector_work_buf;/* Scratch buffer used for selector */
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 0e5c833..43c7bca 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -461,6 +461,7 @@ struct regmap *regmap_init(struct device *dev,
else
map-reg_stride = 1;
map-use_single_rw = config-use_single_rw;
+ map-can_multi_write = config-can_multi_write;
map-dev = dev;
map-bus = bus;
map-bus_context = bus_context;
@@ -1591,41 +1592,196 @@ out:
}
EXPORT_SYMBOL_GPL(regmap_bulk_write);
+/*
+ * _regmap_raw_multi_reg_write()
+ *
+ * the (register,newvalue) pairs in regs have not been formatted, but
+ * they are all in the same page and have been changed to being page
+ * relative. The page register has been written if that was neccessary.
+ */
+static int _regmap_raw_multi_reg_write(struct regmap *map,
+ const struct reg_default *regs,
+ size_t num_regs)
+{
+ int ret;
+ void *buf;
+ int i;
+ u8 *u8;
+ size_t val_bytes = map-format.val_bytes;
+ size_t reg_bytes = map-format.reg_bytes;
+ size_t pad_bytes = map-format.pad_bytes;
+ size_t pair_size = reg_bytes + pad_bytes + val_bytes;
+ size_t len = pair_size * num_regs;
+
+ buf = kzalloc(len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ /* We have to linearise by hand. */
+
+ u8 = buf;
+
+ for (i = 0; i num_regs; i++) {
+ int reg = regs[i].reg;
+ int val = regs[i].def;
+ trace_regmap_hw_write_start(map-dev, reg, 1);
+ map-format.format_reg(u8, reg, map-reg_shift);
+ u8 += reg_bytes + pad_bytes;
+ map-format.format_val(u8, val, 0);
+ u8 += val_bytes;
+ }
+ u8 = buf;
+ *u8 |= map-write_flag_mask;
+
+ ret = map-bus-write(map-bus_context, buf, len);
+
+ kfree(buf);
+
+ for (i = 0; i
[RFC V2] drivers/base/regmap: Implementation for regmap_multi_reg_write
This is the implementation of regmap_multi_reg_write()
It replaces the first definition, which just defined the API.
Signed-off-by: Anthony Olech
---
This patch is relative to linux-next repository tag next-20140228
The API definition of regmap_multi_reg_write()
has been in the kernel mainline since v3.13
This patch suggests an implementation that works for DA9052
family of PMIC chips from Dialog Semiconductor.
This implementation will work in the presense of register ranges
because the algorithm will chop the set of (reg,val) changes each
time the page changes. This part of the algorithm will be need
for other Dialog PMIC that both need to use the Multi Register
Write mode and use paged registers.
The big difference between this attempt and V1 is that the
set of (reg,val) changes are now treated as 'const ...' and
the simplest option of doing a pre pass to catch possible
register changes to page relative is done so that in-situ
changes can be made to an kalloc'ed copy.
A minor change is moving knowledge of the range structure
from _regmap_range_multi_paged_reg_write() to a new static
function _regmap_register_page(). Specifically the function
_regmap_select_page() will send an I2C command if the register
is in a new page, whereas the algorothm needs to know in advance
when the page is going to change so that the fragment of multi
register writes can be sent out to the I2C device.
Please feel free to comment on this implementation. This patch
has only had limited testing with a DA9053 chip so the real
attempt to submit into the MainLine will come later giving any
reviewer ample time to make constructive comments.
drivers/base/regmap/regmap.c | 175 ++
1 file changed, 159 insertions(+), 16 deletions(-)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 0e5c833..c17a2e1 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1591,41 +1591,184 @@ out:
}
EXPORT_SYMBOL_GPL(regmap_bulk_write);
+/*
+ * _regmap_raw_multi_reg_write()
+ *
+ * the (register,newvalue) pairs in regs have not been formatted, but
+ * they are all in the same page and have been changed to being page
+ * relative. The page register has been written if that was neccessary.
+ */
+static int _regmap_raw_multi_reg_write(struct regmap *map,
+ const struct reg_default *regs,
+ size_t num_regs)
+{
+ int ret;
+ void *buf;
+ int i;
+ u8 *u8;
+ size_t val_bytes = map->format.val_bytes;
+ size_t reg_bytes = map->format.reg_bytes;
+ size_t pad_bytes = map->format.pad_bytes;
+ size_t pair_size = reg_bytes + pad_bytes + val_bytes;
+ size_t len = pair_size * num_regs;
+
+ buf = kzalloc(len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ /* We have to linearise by hand. */
+
+ u8 = buf;
+
+ for (i = 0; i < num_regs; i++) {
+ int reg = regs[i].reg;
+ int val = regs[i].def;
+ trace_regmap_hw_write_start(map->dev, reg, 1);
+ map->format.format_reg(u8, reg, map->reg_shift);
+ u8 += reg_bytes + pad_bytes;
+ map->format.format_val(u8, val, 0);
+ u8 += val_bytes;
+ }
+ u8 = buf;
+ *u8 |= map->write_flag_mask;
+
+ ret = map->bus->write(map->bus_context, buf, len);
+
+ kfree(buf);
+
+ for (i = 0; i < num_regs; i++) {
+ int reg = regs[i].reg;
+ trace_regmap_hw_write_done(map->dev, reg, 1);
+ }
+ return ret;
+}
+
+static unsigned int _regmap_register_page(struct regmap *map,
+ unsigned int reg,
+ struct regmap_range_node *range)
+{
+ unsigned int win_page = (reg - range->range_min) / range->window_len;
+
+ return win_page;
+}
+
+static int _regmap_range_multi_paged_reg_write(struct regmap *map,
+ struct reg_default *regs,
+ size_t num_regs)
+{
+ int ret;
+ int i, n;
+ struct reg_default *base;
+ unsigned int this_page;
+ /*
+* the set of registers are not neccessarily in order, but
+* since the order of write must be preserved this algorithm
+* chops the set each time the page changes
+*/
+ base = regs;
+ for (i = 0, n = 0; i < num_regs; i++, n++) {
+ unsigned int reg = regs[i].reg;
+ struct regmap_range_node *range;
+
+ range = _regmap_range_lookup(map, reg);
+ if (range) {
+ unsigned int win_page = _regmap_register_page(map,
+ reg,
+ range);
+
+
[RFC V2] drivers/base/regmap: Implementation for regmap_multi_reg_write
This is the implementation of regmap_multi_reg_write()
It replaces the first definition, which just defined the API.
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
---
This patch is relative to linux-next repository tag next-20140228
The API definition of regmap_multi_reg_write()
has been in the kernel mainline since v3.13
This patch suggests an implementation that works for DA9052
family of PMIC chips from Dialog Semiconductor.
This implementation will work in the presense of register ranges
because the algorithm will chop the set of (reg,val) changes each
time the page changes. This part of the algorithm will be need
for other Dialog PMIC that both need to use the Multi Register
Write mode and use paged registers.
The big difference between this attempt and V1 is that the
set of (reg,val) changes are now treated as 'const ...' and
the simplest option of doing a pre pass to catch possible
register changes to page relative is done so that in-situ
changes can be made to an kalloc'ed copy.
A minor change is moving knowledge of the range structure
from _regmap_range_multi_paged_reg_write() to a new static
function _regmap_register_page(). Specifically the function
_regmap_select_page() will send an I2C command if the register
is in a new page, whereas the algorothm needs to know in advance
when the page is going to change so that the fragment of multi
register writes can be sent out to the I2C device.
Please feel free to comment on this implementation. This patch
has only had limited testing with a DA9053 chip so the real
attempt to submit into the MainLine will come later giving any
reviewer ample time to make constructive comments.
drivers/base/regmap/regmap.c | 175 ++
1 file changed, 159 insertions(+), 16 deletions(-)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 0e5c833..c17a2e1 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1591,41 +1591,184 @@ out:
}
EXPORT_SYMBOL_GPL(regmap_bulk_write);
+/*
+ * _regmap_raw_multi_reg_write()
+ *
+ * the (register,newvalue) pairs in regs have not been formatted, but
+ * they are all in the same page and have been changed to being page
+ * relative. The page register has been written if that was neccessary.
+ */
+static int _regmap_raw_multi_reg_write(struct regmap *map,
+ const struct reg_default *regs,
+ size_t num_regs)
+{
+ int ret;
+ void *buf;
+ int i;
+ u8 *u8;
+ size_t val_bytes = map-format.val_bytes;
+ size_t reg_bytes = map-format.reg_bytes;
+ size_t pad_bytes = map-format.pad_bytes;
+ size_t pair_size = reg_bytes + pad_bytes + val_bytes;
+ size_t len = pair_size * num_regs;
+
+ buf = kzalloc(len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ /* We have to linearise by hand. */
+
+ u8 = buf;
+
+ for (i = 0; i num_regs; i++) {
+ int reg = regs[i].reg;
+ int val = regs[i].def;
+ trace_regmap_hw_write_start(map-dev, reg, 1);
+ map-format.format_reg(u8, reg, map-reg_shift);
+ u8 += reg_bytes + pad_bytes;
+ map-format.format_val(u8, val, 0);
+ u8 += val_bytes;
+ }
+ u8 = buf;
+ *u8 |= map-write_flag_mask;
+
+ ret = map-bus-write(map-bus_context, buf, len);
+
+ kfree(buf);
+
+ for (i = 0; i num_regs; i++) {
+ int reg = regs[i].reg;
+ trace_regmap_hw_write_done(map-dev, reg, 1);
+ }
+ return ret;
+}
+
+static unsigned int _regmap_register_page(struct regmap *map,
+ unsigned int reg,
+ struct regmap_range_node *range)
+{
+ unsigned int win_page = (reg - range-range_min) / range-window_len;
+
+ return win_page;
+}
+
+static int _regmap_range_multi_paged_reg_write(struct regmap *map,
+ struct reg_default *regs,
+ size_t num_regs)
+{
+ int ret;
+ int i, n;
+ struct reg_default *base;
+ unsigned int this_page;
+ /*
+* the set of registers are not neccessarily in order, but
+* since the order of write must be preserved this algorithm
+* chops the set each time the page changes
+*/
+ base = regs;
+ for (i = 0, n = 0; i num_regs; i++, n++) {
+ unsigned int reg = regs[i].reg;
+ struct regmap_range_node *range;
+
+ range = _regmap_range_lookup(map, reg);
+ if (range) {
+ unsigned int win_page = _regmap_register_page(map,
+ reg,
+
RE: [RFC V1] drivers/base/regmap: Implementation for regmap_multi_reg_write
> -Original Message-
> From: Mark Brown [mailto:broo...@kernel.org]
> Sent: 28 February 2014 03:37
> To: Opensource [Anthony Olech]
> Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org; David Dajun Chen
> Subject: Re: [RFC V1] drivers/base/regmap: Implementation for
> regmap_multi_reg_write
> On Thu, Feb 27, 2014 at 11:28:56AM +0000, Opensource [Anthony Olech]
> wrote:
> > This is the implementation of regmap_multi_reg_write()
> > It replaces the first definition, which just defined the API.
> Aside from any review comments this won't apply with the recent patches
> that Charles did to provide a bypassed version of the API, it needs to be
> rebased.
I see that next-20140228 has Charles' patch applied, my next attempt
will be rebased against the latest linux-next.
> > a) should an async operation be allowed? easy in the case where
> >all the changes are in the same page - but if the operation
> >is broken due to changes over several pages not so easy.
> It's fine to support only the simple cases, async operation is just an
> optimisation so we can always just serialise in cases where it gets
> complicated and someone can optimise later if they care. It'd be fine to just
> decay to a series of regmap_reg_write()s if there's paging involved.
The algorithm for splitting up into smaller _multi_reg_writes is easy enough,
so if the calling device driver created a set of (reg,val) pairs for a multi reg
write operation then surely the intention is for the individual pieces to be
handled as multi reg writes.
> > b) the user supplied set (array of struct reg_default) of changes
> >has the register address modified when the target page alters.
> >Would it be better not to do an in-situ change, but rather to
> >alloc a new array of struct reg_default?
> Yes, the user should be able to pass in a const pointer (indeed Charles
> changed the API to do that).
my next attempt will match the API.
> > +++ b/drivers/base/regmap/regmap.c
> > @@ -1442,6 +1442,7 @@ int regmap_field_write(struct regmap_field
> > *field, unsigned int val) } EXPORT_SYMBOL_GPL(regmap_field_write);
> > +
> > /**
> > * regmap_field_update_bits(): Perform a read/modify/write cycle
> Random whitespace change here.
sorry about that - it slipped past my quality control!
> > +static int _switch_register_page(struct regmap *map, unsigned int
> win_page,
> > + struct regmap_range_node *range) {
> > + int ret;
> > + bool page_chg;
> > + void *orig_work_buf = map->work_buf;
> > + unsigned int swp;
> > +
> > + map->work_buf = map->selector_work_buf;
> > +
> > + swp = win_page << range->selector_shift;
> > + ret = _regmap_update_bits(map,
> > + range->selector_reg,
> > + range->selector_mask,
> > + swp, _chg);
> > +
> > + map->work_buf = orig_work_buf;
> > +
> I'd expect this to be using _regmap_select_page()? In general there seems
> like quite a bit of duplication to handle paging.
I will try to revamp that part!
> > + return ret;
> > +}
> > +/*
> You need a blank here.
I missed that one as well - I will do better in my next attempt!
> > + buf = kzalloc(len , GFP_KERNEL);
> > +
> > + if (!buf)
> > + return -ENOMEM;
> Coding style - extra blank between the kzalloc and the check and an extra
> space before the comma.
it will be fixed.
> > + /*
> > +* the set of registers are not neccessarily in order, but
> > +* since the order of write must be preserved this algorithm
> > +* chops the set each time the page changes
> > +*/
> > + for (i = 0, n = 0, switched = false, base = regs; i < num_regs;
> > + i++, n++) {
> Don't put all this stuff in the for (), just put the iteration in the for ().
all those variables are a fundamental part of the loop, but I will change it.
> > + /*
> > +* Some devices do not support multi write, for
> > +* them we have a series of single write operations.
> > +*/
> > + if (map->use_single_rw) {
> > + for (i = 0; i < num_regs; i++) {
> > + ret = _regmap_write(map, regs[i].reg, regs[i].def);
> > + if (ret != 0)
> > + goto out;
> > + }
> > + } else {
> > + ret = _regmap_multi_reg_write(map, regs, num_regs);
>
> I'd expect to see something that devices do to specifically advertise this
> ca
RE: [RFC V1] drivers/base/regmap: Implementation for regmap_multi_reg_write
-Original Message-
From: Mark Brown [mailto:broo...@kernel.org]
Sent: 28 February 2014 03:37
To: Opensource [Anthony Olech]
Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org; David Dajun Chen
Subject: Re: [RFC V1] drivers/base/regmap: Implementation for
regmap_multi_reg_write
On Thu, Feb 27, 2014 at 11:28:56AM +, Opensource [Anthony Olech]
wrote:
This is the implementation of regmap_multi_reg_write()
It replaces the first definition, which just defined the API.
Aside from any review comments this won't apply with the recent patches
that Charles did to provide a bypassed version of the API, it needs to be
rebased.
I see that next-20140228 has Charles' patch applied, my next attempt
will be rebased against the latest linux-next.
a) should an async operation be allowed? easy in the case where
all the changes are in the same page - but if the operation
is broken due to changes over several pages not so easy.
It's fine to support only the simple cases, async operation is just an
optimisation so we can always just serialise in cases where it gets
complicated and someone can optimise later if they care. It'd be fine to just
decay to a series of regmap_reg_write()s if there's paging involved.
The algorithm for splitting up into smaller _multi_reg_writes is easy enough,
so if the calling device driver created a set of (reg,val) pairs for a multi reg
write operation then surely the intention is for the individual pieces to be
handled as multi reg writes.
b) the user supplied set (array of struct reg_default) of changes
has the register address modified when the target page alters.
Would it be better not to do an in-situ change, but rather to
alloc a new array of struct reg_default?
Yes, the user should be able to pass in a const pointer (indeed Charles
changed the API to do that).
my next attempt will match the API.
+++ b/drivers/base/regmap/regmap.c
@@ -1442,6 +1442,7 @@ int regmap_field_write(struct regmap_field
*field, unsigned int val) } EXPORT_SYMBOL_GPL(regmap_field_write);
+
/**
* regmap_field_update_bits(): Perform a read/modify/write cycle
Random whitespace change here.
sorry about that - it slipped past my quality control!
+static int _switch_register_page(struct regmap *map, unsigned int
win_page,
+ struct regmap_range_node *range) {
+ int ret;
+ bool page_chg;
+ void *orig_work_buf = map-work_buf;
+ unsigned int swp;
+
+ map-work_buf = map-selector_work_buf;
+
+ swp = win_page range-selector_shift;
+ ret = _regmap_update_bits(map,
+ range-selector_reg,
+ range-selector_mask,
+ swp, page_chg);
+
+ map-work_buf = orig_work_buf;
+
I'd expect this to be using _regmap_select_page()? In general there seems
like quite a bit of duplication to handle paging.
I will try to revamp that part!
+ return ret;
+}
+/*
You need a blank here.
I missed that one as well - I will do better in my next attempt!
+ buf = kzalloc(len , GFP_KERNEL);
+
+ if (!buf)
+ return -ENOMEM;
Coding style - extra blank between the kzalloc and the check and an extra
space before the comma.
it will be fixed.
+ /*
+* the set of registers are not neccessarily in order, but
+* since the order of write must be preserved this algorithm
+* chops the set each time the page changes
+*/
+ for (i = 0, n = 0, switched = false, base = regs; i num_regs;
+ i++, n++) {
Don't put all this stuff in the for (), just put the iteration in the for ().
all those variables are a fundamental part of the loop, but I will change it.
+ /*
+* Some devices do not support multi write, for
+* them we have a series of single write operations.
+*/
+ if (map-use_single_rw) {
+ for (i = 0; i num_regs; i++) {
+ ret = _regmap_write(map, regs[i].reg, regs[i].def);
+ if (ret != 0)
+ goto out;
+ }
+ } else {
+ ret = _regmap_multi_reg_write(map, regs, num_regs);
I'd expect to see something that devices do to specifically advertise this
capability, it doesn't follow that a device that a device that only supports
single writes will support the multi write operation and frameworks may try
to use the multi write API to help optimise things.
Yes, you are correct - I think a driver needs to pass an extra bit of
information in struct regmap_config to indicate that it is capable
of using the multi_req_write mode.
I will invent a new flag
many thanks Mark for your very help review and comments.
Tony Olech
Dialog Semiconductor
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More
[RFC V1] drivers/base/regmap: Implementation for regmap_multi_reg_write
This is the implementation of regmap_multi_reg_write()
It replaces the first definition, which just defined the API.
Signed-off-by: Anthony Olech
---
This patch is relative to linux-next repository tag next-20140224
The API definition of regmap_multi_reg_write()
has been in the kernel mainline since v3.13
This patch suggests an implementation that works for DA9052
family of PMIC chips from Dialog Semiconductor.
I believe this implementation will work in the presense of
register ranges because the algorithm will chop the set of
(reg,val) changes each time the page changes.
Please feel free to comment on this implementation, in particular:
a) should an async operation be allowed? easy in the case where
all the changes are in the same page - but if the operation
is broken due to changes over several pages not so easy.
b) the user supplied set (array of struct reg_default) of changes
has the register address modified when the target page alters.
Would it be better not to do an in-situ change, but rather to
alloc a new array of struct reg_default?
drivers/base/regmap/regmap.c | 212 +++---
1 file changed, 201 insertions(+), 11 deletions(-)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 2b4e72f..90f3d57 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1442,6 +1442,7 @@ int regmap_field_write(struct regmap_field *field,
unsigned int val)
}
EXPORT_SYMBOL_GPL(regmap_field_write);
+
/**
* regmap_field_update_bits(): Perform a read/modify/write cycle
* on the register field
@@ -1591,26 +1592,206 @@ out:
}
EXPORT_SYMBOL_GPL(regmap_bulk_write);
+static int _switch_register_page(struct regmap *map, unsigned int win_page,
+ struct regmap_range_node *range)
+{
+ int ret;
+ bool page_chg;
+ void *orig_work_buf = map->work_buf;
+ unsigned int swp;
+
+ map->work_buf = map->selector_work_buf;
+
+ swp = win_page << range->selector_shift;
+ ret = _regmap_update_bits(map,
+ range->selector_reg,
+ range->selector_mask,
+ swp, _chg);
+
+ map->work_buf = orig_work_buf;
+
+ return ret;
+}
+/*
+ * _regmap_raw_multi_reg_write()
+ *
+ * the (register,newvalue) pairs in regs have not been formatted, but
+ * they are all in the same page and have been changed to being page
+ * relative. The page register has been written if that was neccessary.
+ */
+static int _regmap_raw_multi_reg_write(struct regmap *map,
+ struct reg_default *regs,
+ size_t num_regs)
+{
+ int ret;
+ void *buf;
+ int i;
+ u8 *u8;
+ size_t val_bytes = map->format.val_bytes;
+ size_t reg_bytes = map->format.reg_bytes;
+ size_t pad_bytes = map->format.pad_bytes;
+ size_t pair_size = reg_bytes + pad_bytes + val_bytes;
+ size_t len = pair_size * num_regs;
+
+ buf = kzalloc(len , GFP_KERNEL);
+
+ if (!buf)
+ return -ENOMEM;
+
+ /* We have to linearise by hand. */
+
+ u8 = buf;
+
+ for (i = 0; i < num_regs; i++) {
+ int reg = regs[i].reg;
+ int val = regs[i].def;
+ trace_regmap_hw_write_start(map->dev, reg, 1);
+ map->format.format_reg(u8, reg, map->reg_shift);
+ u8 += reg_bytes + pad_bytes;
+ map->format.format_val(u8, val, 0);
+ u8 += val_bytes;
+ }
+ u8 = buf;
+ *u8 |= map->write_flag_mask;
+
+ ret = map->bus->write(map->bus_context, buf, len);
+
+ kfree(buf);
+
+ for (i = 0; i < num_regs; i++) {
+ int reg = regs[i].reg;
+ trace_regmap_hw_write_done(map->dev, reg, 1);
+ }
+ return ret;
+}
+
+static int _regmap_multi_reg_write(struct regmap *map,
+ struct reg_default *regs,
+ size_t num_regs)
+{
+ int i, n;
+ int ret;
+ struct reg_default *base;
+ bool switched;
+ unsigned int this_page;
+
+ if (!map->format.parse_inplace)
+ return -EINVAL;
+
+ if (map->writeable_reg)
+ for (i = 0; i < num_regs; i++)
+ if (!map->writeable_reg(map->dev, regs[i].reg))
+ return -EINVAL;
+
+ if (!map->cache_bypass) {
+ for (i = 0; i < num_regs; i++) {
+ unsigned int val = regs[i].def;
+ unsigned int reg = regs[i].reg;
+ ret = regcache_write(map, reg, val);
+ if (ret) {
+ dev_err(map->dev,
+ "Error in caching of register: %x ret: %d\n",
+
[RFC V1] drivers/base/regmap: Implementation for regmap_multi_reg_write
This is the implementation of regmap_multi_reg_write()
It replaces the first definition, which just defined the API.
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
---
This patch is relative to linux-next repository tag next-20140224
The API definition of regmap_multi_reg_write()
has been in the kernel mainline since v3.13
This patch suggests an implementation that works for DA9052
family of PMIC chips from Dialog Semiconductor.
I believe this implementation will work in the presense of
register ranges because the algorithm will chop the set of
(reg,val) changes each time the page changes.
Please feel free to comment on this implementation, in particular:
a) should an async operation be allowed? easy in the case where
all the changes are in the same page - but if the operation
is broken due to changes over several pages not so easy.
b) the user supplied set (array of struct reg_default) of changes
has the register address modified when the target page alters.
Would it be better not to do an in-situ change, but rather to
alloc a new array of struct reg_default?
drivers/base/regmap/regmap.c | 212 +++---
1 file changed, 201 insertions(+), 11 deletions(-)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 2b4e72f..90f3d57 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1442,6 +1442,7 @@ int regmap_field_write(struct regmap_field *field,
unsigned int val)
}
EXPORT_SYMBOL_GPL(regmap_field_write);
+
/**
* regmap_field_update_bits(): Perform a read/modify/write cycle
* on the register field
@@ -1591,26 +1592,206 @@ out:
}
EXPORT_SYMBOL_GPL(regmap_bulk_write);
+static int _switch_register_page(struct regmap *map, unsigned int win_page,
+ struct regmap_range_node *range)
+{
+ int ret;
+ bool page_chg;
+ void *orig_work_buf = map-work_buf;
+ unsigned int swp;
+
+ map-work_buf = map-selector_work_buf;
+
+ swp = win_page range-selector_shift;
+ ret = _regmap_update_bits(map,
+ range-selector_reg,
+ range-selector_mask,
+ swp, page_chg);
+
+ map-work_buf = orig_work_buf;
+
+ return ret;
+}
+/*
+ * _regmap_raw_multi_reg_write()
+ *
+ * the (register,newvalue) pairs in regs have not been formatted, but
+ * they are all in the same page and have been changed to being page
+ * relative. The page register has been written if that was neccessary.
+ */
+static int _regmap_raw_multi_reg_write(struct regmap *map,
+ struct reg_default *regs,
+ size_t num_regs)
+{
+ int ret;
+ void *buf;
+ int i;
+ u8 *u8;
+ size_t val_bytes = map-format.val_bytes;
+ size_t reg_bytes = map-format.reg_bytes;
+ size_t pad_bytes = map-format.pad_bytes;
+ size_t pair_size = reg_bytes + pad_bytes + val_bytes;
+ size_t len = pair_size * num_regs;
+
+ buf = kzalloc(len , GFP_KERNEL);
+
+ if (!buf)
+ return -ENOMEM;
+
+ /* We have to linearise by hand. */
+
+ u8 = buf;
+
+ for (i = 0; i num_regs; i++) {
+ int reg = regs[i].reg;
+ int val = regs[i].def;
+ trace_regmap_hw_write_start(map-dev, reg, 1);
+ map-format.format_reg(u8, reg, map-reg_shift);
+ u8 += reg_bytes + pad_bytes;
+ map-format.format_val(u8, val, 0);
+ u8 += val_bytes;
+ }
+ u8 = buf;
+ *u8 |= map-write_flag_mask;
+
+ ret = map-bus-write(map-bus_context, buf, len);
+
+ kfree(buf);
+
+ for (i = 0; i num_regs; i++) {
+ int reg = regs[i].reg;
+ trace_regmap_hw_write_done(map-dev, reg, 1);
+ }
+ return ret;
+}
+
+static int _regmap_multi_reg_write(struct regmap *map,
+ struct reg_default *regs,
+ size_t num_regs)
+{
+ int i, n;
+ int ret;
+ struct reg_default *base;
+ bool switched;
+ unsigned int this_page;
+
+ if (!map-format.parse_inplace)
+ return -EINVAL;
+
+ if (map-writeable_reg)
+ for (i = 0; i num_regs; i++)
+ if (!map-writeable_reg(map-dev, regs[i].reg))
+ return -EINVAL;
+
+ if (!map-cache_bypass) {
+ for (i = 0; i num_regs; i++) {
+ unsigned int val = regs[i].def;
+ unsigned int reg = regs[i].reg;
+ ret = regcache_write(map, reg, val);
+ if (ret) {
+ dev_err(map-dev,
+ Error in caching of register: %x ret: %d\n,
+
RE: [PATCH V1 2/3] MFD: da9052: Add new DA9053 BC chip variant
> -Original Message- > From: Lee Jones [mailto:lee.jo...@linaro.org] > Sent: 25 February 2014 08:57 > To: Opensource [Anthony Olech] > Cc: Samuel Ortiz; Liam Girdwood; linux-kernel@vger.kernel.org; Mark > Brown; David Dajun Chen > Subject: Re: [PATCH V1 2/3] MFD: da9052: Add new DA9053 BC chip variant > > Add support for a new BC variant of the DA9053 PMIC. > > There is one difference between it and the AA, BA and BB. > > This patch also corrects a typing mistake in one of the BA name > > strings that was incorrectly typed as "ab". > > Signed-off-by: Anthony Olech > > --- > > > > This patch is relative to linux-next repository tag next-20140219 > > That's not generally a good idea. > Please use a Mainline version (inc. -rcs). Hi Lee, thanks for applying the patch. I can rebase against any tagged version in a public git repository, but in the past I was advised to use linux-next instead of the mainline. At this moment in time the latest tag in git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git is 'v3.14-rc4' Are you recommending that tag as the one to rebase (and test of course) against? many thanks Tony Olech > > This patch depends on patch number 1 of this patch series being > > applied first or it will not compile. > > > > drivers/mfd/da9052-i2c.c |5 - > > drivers/mfd/da9052-spi.c |1 + > > 2 files changed, 5 insertions(+), 1 deletion(-) > > Applied, thanks. > > -- > Lee Jones > Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source > software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
RE: [PATCH V1 2/3] MFD: da9052: Add new DA9053 BC chip variant
-Original Message- From: Lee Jones [mailto:lee.jo...@linaro.org] Sent: 25 February 2014 08:57 To: Opensource [Anthony Olech] Cc: Samuel Ortiz; Liam Girdwood; linux-kernel@vger.kernel.org; Mark Brown; David Dajun Chen Subject: Re: [PATCH V1 2/3] MFD: da9052: Add new DA9053 BC chip variant Add support for a new BC variant of the DA9053 PMIC. There is one difference between it and the AA, BA and BB. This patch also corrects a typing mistake in one of the BA name strings that was incorrectly typed as ab. Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com --- This patch is relative to linux-next repository tag next-20140219 That's not generally a good idea. Please use a Mainline version (inc. -rcs). Hi Lee, thanks for applying the patch. I can rebase against any tagged version in a public git repository, but in the past I was advised to use linux-next instead of the mainline. At this moment in time the latest tag in git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git is 'v3.14-rc4' Are you recommending that tag as the one to rebase (and test of course) against? many thanks Tony Olech This patch depends on patch number 1 of this patch series being applied first or it will not compile. drivers/mfd/da9052-i2c.c |5 - drivers/mfd/da9052-spi.c |1 + 2 files changed, 5 insertions(+), 1 deletion(-) Applied, thanks. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
RE: [PATCH V1 3/3] REGULATOR: da9052: Add new DA9053 BC chip variant
> -Original Message-
> From: Mark Brown [mailto:broo...@kernel.org]
> Sent: 19 February 2014 16:48
> To: Opensource [Anthony Olech]
> Cc: Liam Girdwood; Lee Jones; linux-kernel@vger.kernel.org; Samuel Ortiz;
> David Dajun Chen
> Subject: Re: [PATCH V1 3/3] REGULATOR: da9052: Add new DA9053 BC chip
> variant
> On Wed, Feb 19, 2014 at 04:32:47PM +, Opensource [Anthony Olech]
> wrote:
> > Add support for a new BC variant of the DA9053 PMIC.
> Acked-by: Mark Brown
> > There is one difference between it and the AA, BA and BB.
> Which is...? :)
Hi Mark,
it is easier to see in the diff -C15 output, that the BC chip does not need
a 'safe read':
--- drivers/mfd/da9052-i2c.cThu Feb 20 09:20:43 2014
***
*** 63,92
--- 63,93
static int da9052_i2c_fix(struct da9052 *da9052, unsigned char reg)
{
int val;
switch (da9052->chip_id) {
case DA9052:
case DA9053_AA:
case DA9053_BA:
case DA9053_BB:
/* A dummy read to a safe register address. */
if (!i2c_safe_reg(reg))
return regmap_read(da9052->regmap,
DA9052_PARK_REGISTER,
);
break;
+ case DA9053_BC:
default:
/*
* For other chips parking of I2C register
* to a safe place is not required.
*/
break;
}
return 0;
}
Tony Olech
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH V1 3/3] REGULATOR: da9052: Add new DA9053 BC chip variant
-Original Message-
From: Mark Brown [mailto:broo...@kernel.org]
Sent: 19 February 2014 16:48
To: Opensource [Anthony Olech]
Cc: Liam Girdwood; Lee Jones; linux-kernel@vger.kernel.org; Samuel Ortiz;
David Dajun Chen
Subject: Re: [PATCH V1 3/3] REGULATOR: da9052: Add new DA9053 BC chip
variant
On Wed, Feb 19, 2014 at 04:32:47PM +, Opensource [Anthony Olech]
wrote:
Add support for a new BC variant of the DA9053 PMIC.
Acked-by: Mark Brown broo...@linaro.org
There is one difference between it and the AA, BA and BB.
Which is...? :)
Hi Mark,
it is easier to see in the diff -C15 output, that the BC chip does not need
a 'safe read':
--- drivers/mfd/da9052-i2c.cThu Feb 20 09:20:43 2014
***
*** 63,92
--- 63,93
static int da9052_i2c_fix(struct da9052 *da9052, unsigned char reg)
{
int val;
switch (da9052-chip_id) {
case DA9052:
case DA9053_AA:
case DA9053_BA:
case DA9053_BB:
/* A dummy read to a safe register address. */
if (!i2c_safe_reg(reg))
return regmap_read(da9052-regmap,
DA9052_PARK_REGISTER,
val);
break;
+ case DA9053_BC:
default:
/*
* For other chips parking of I2C register
* to a safe place is not required.
*/
break;
}
return 0;
}
Tony Olech
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V1 0/3] da9052: Add new supported chip
This is submission attempt number 1 to have this patch sequence applied to the linux kernel source tree. The DA9052 and DA9053 are low power Power Management Integrated Circuits with extra functionality beyond the power regulators. There is now a new variant of the DA9053 PMIC called BC, and this patch sequence provides for it's support. There is one difference between it and the AA, BA and BB variants. There is also the correctiion of a typing mistake in one of the BA name strings that was erroneously typed as "ab". This patch sequence depends on the first being applied first because it provides the hash definition of DA9053_BC Many thanks, Anthony Olech, Dialog Semiconductor Opensource [Anthony Olech] (3): include/linux/mfd/da9052/da9052: Add new BC chip MFD: da9052: Add new DA9053 BC chip variant REGULATOR: da9052: Add new DA9053 BC chip variant drivers/mfd/da9052-i2c.c |5 - drivers/mfd/da9052-spi.c |1 + drivers/regulator/da9052-regulator.c |1 + include/linux/mfd/da9052/da9052.h|1 + 4 files changed, 7 insertions(+), 1 deletion(-) -- end-of-patch 0/3 for PATCH V1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH V1 1/3] include/linux/mfd/da9052/da9052: Add new BC chip
Add the hash define for the new variant of the DA9053 PMIC called BC.
Signed-off-by: Anthony Olech
---
This patch is relative to linux-next repository tag next-20140219
This patch must be applied before the others in this patch series
or the drivers that the other patches modify will not compile.
include/linux/mfd/da9052/da9052.h |1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/mfd/da9052/da9052.h
b/include/linux/mfd/da9052/da9052.h
index 21e21b8..bba65f5 100644
--- a/include/linux/mfd/da9052/da9052.h
+++ b/include/linux/mfd/da9052/da9052.h
@@ -83,6 +83,7 @@ enum da9052_chip_id {
DA9053_AA,
DA9053_BA,
DA9053_BB,
+ DA9053_BC,
};
struct da9052_pdata;
--
end-of-patch 1/3 for PATCH V1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V1 2/3] MFD: da9052: Add new DA9053 BC chip variant
Add support for a new BC variant of the DA9053 PMIC.
There is one difference between it and the AA, BA and BB.
This patch also corrects a typing mistake in one of the BA
name strings that was incorrectly typed as "ab".
Signed-off-by: Anthony Olech
---
This patch is relative to linux-next repository tag next-20140219
This patch depends on patch number 1 of this patch
series being applied first or it will not compile.
drivers/mfd/da9052-i2c.c |5 -
drivers/mfd/da9052-spi.c |1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/da9052-i2c.c b/drivers/mfd/da9052-i2c.c
index c319c4e..6da8ec8 100644
--- a/drivers/mfd/da9052-i2c.c
+++ b/drivers/mfd/da9052-i2c.c
@@ -75,6 +75,7 @@ static int da9052_i2c_fix(struct da9052 *da9052, unsigned
char reg)
DA9052_PARK_REGISTER,
);
break;
+ case DA9053_BC:
default:
/*
* For other chips parking of I2C register
@@ -114,6 +115,7 @@ static const struct i2c_device_id da9052_i2c_id[] = {
{"da9053-aa", DA9053_AA},
{"da9053-ba", DA9053_BA},
{"da9053-bb", DA9053_BB},
+ {"da9053-bc", DA9053_BC},
{}
};
@@ -121,8 +123,9 @@ static const struct i2c_device_id da9052_i2c_id[] = {
static const struct of_device_id dialog_dt_ids[] = {
{ .compatible = "dlg,da9052", .data = _i2c_id[0] },
{ .compatible = "dlg,da9053-aa", .data = _i2c_id[1] },
- { .compatible = "dlg,da9053-ab", .data = _i2c_id[2] },
+ { .compatible = "dlg,da9053-ba", .data = _i2c_id[2] },
{ .compatible = "dlg,da9053-bb", .data = _i2c_id[3] },
+ { .compatible = "dlg,da9053-bc", .data = _i2c_id[4] },
{ /* sentinel */ }
};
#endif
diff --git a/drivers/mfd/da9052-spi.c b/drivers/mfd/da9052-spi.c
index 0680bcb..17666b4 100644
--- a/drivers/mfd/da9052-spi.c
+++ b/drivers/mfd/da9052-spi.c
@@ -71,6 +71,7 @@ static struct spi_device_id da9052_spi_id[] = {
{"da9053-aa", DA9053_AA},
{"da9053-ba", DA9053_BA},
{"da9053-bb", DA9053_BB},
+ {"da9053-bc", DA9053_BC},
{}
};
--
end-of-patch 2/3 for PATCH V1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V1 3/3] REGULATOR: da9052: Add new DA9053 BC chip variant
Add support for a new BC variant of the DA9053 PMIC.
There is one difference between it and the AA, BA and BB.
Signed-off-by: Anthony Olech
---
This patch is relative to linux-next repository tag next-20140219
This patch depends on patch number 1 of this patch
series being applied first or it will not compile.
drivers/regulator/da9052-regulator.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/regulator/da9052-regulator.c
b/drivers/regulator/da9052-regulator.c
index 889c7c9..599f2ad 100644
--- a/drivers/regulator/da9052-regulator.c
+++ b/drivers/regulator/da9052-regulator.c
@@ -354,6 +354,7 @@ static inline struct da9052_regulator_info
*find_regulator_info(u8 chip_id,
case DA9053_AA:
case DA9053_BA:
case DA9053_BB:
+ case DA9053_BC:
for (i = 0; i < ARRAY_SIZE(da9053_regulator_info); i++) {
info = _regulator_info[i];
if (info->reg_desc.id == id)
--
end-of-patch 3/3 for PATCH V1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V1 2/3] MFD: da9052: Add new DA9053 BC chip variant
Add support for a new BC variant of the DA9053 PMIC.
There is one difference between it and the AA, BA and BB.
This patch also corrects a typing mistake in one of the BA
name strings that was incorrectly typed as ab.
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
---
This patch is relative to linux-next repository tag next-20140219
This patch depends on patch number 1 of this patch
series being applied first or it will not compile.
drivers/mfd/da9052-i2c.c |5 -
drivers/mfd/da9052-spi.c |1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/da9052-i2c.c b/drivers/mfd/da9052-i2c.c
index c319c4e..6da8ec8 100644
--- a/drivers/mfd/da9052-i2c.c
+++ b/drivers/mfd/da9052-i2c.c
@@ -75,6 +75,7 @@ static int da9052_i2c_fix(struct da9052 *da9052, unsigned
char reg)
DA9052_PARK_REGISTER,
val);
break;
+ case DA9053_BC:
default:
/*
* For other chips parking of I2C register
@@ -114,6 +115,7 @@ static const struct i2c_device_id da9052_i2c_id[] = {
{da9053-aa, DA9053_AA},
{da9053-ba, DA9053_BA},
{da9053-bb, DA9053_BB},
+ {da9053-bc, DA9053_BC},
{}
};
@@ -121,8 +123,9 @@ static const struct i2c_device_id da9052_i2c_id[] = {
static const struct of_device_id dialog_dt_ids[] = {
{ .compatible = dlg,da9052, .data = da9052_i2c_id[0] },
{ .compatible = dlg,da9053-aa, .data = da9052_i2c_id[1] },
- { .compatible = dlg,da9053-ab, .data = da9052_i2c_id[2] },
+ { .compatible = dlg,da9053-ba, .data = da9052_i2c_id[2] },
{ .compatible = dlg,da9053-bb, .data = da9052_i2c_id[3] },
+ { .compatible = dlg,da9053-bc, .data = da9052_i2c_id[4] },
{ /* sentinel */ }
};
#endif
diff --git a/drivers/mfd/da9052-spi.c b/drivers/mfd/da9052-spi.c
index 0680bcb..17666b4 100644
--- a/drivers/mfd/da9052-spi.c
+++ b/drivers/mfd/da9052-spi.c
@@ -71,6 +71,7 @@ static struct spi_device_id da9052_spi_id[] = {
{da9053-aa, DA9053_AA},
{da9053-ba, DA9053_BA},
{da9053-bb, DA9053_BB},
+ {da9053-bc, DA9053_BC},
{}
};
--
end-of-patch 2/3 for PATCH V1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V1 3/3] REGULATOR: da9052: Add new DA9053 BC chip variant
Add support for a new BC variant of the DA9053 PMIC.
There is one difference between it and the AA, BA and BB.
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
---
This patch is relative to linux-next repository tag next-20140219
This patch depends on patch number 1 of this patch
series being applied first or it will not compile.
drivers/regulator/da9052-regulator.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/regulator/da9052-regulator.c
b/drivers/regulator/da9052-regulator.c
index 889c7c9..599f2ad 100644
--- a/drivers/regulator/da9052-regulator.c
+++ b/drivers/regulator/da9052-regulator.c
@@ -354,6 +354,7 @@ static inline struct da9052_regulator_info
*find_regulator_info(u8 chip_id,
case DA9053_AA:
case DA9053_BA:
case DA9053_BB:
+ case DA9053_BC:
for (i = 0; i ARRAY_SIZE(da9053_regulator_info); i++) {
info = da9053_regulator_info[i];
if (info-reg_desc.id == id)
--
end-of-patch 3/3 for PATCH V1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V1 1/3] include/linux/mfd/da9052/da9052: Add new BC chip
Add the hash define for the new variant of the DA9053 PMIC called BC.
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
---
This patch is relative to linux-next repository tag next-20140219
This patch must be applied before the others in this patch series
or the drivers that the other patches modify will not compile.
include/linux/mfd/da9052/da9052.h |1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/mfd/da9052/da9052.h
b/include/linux/mfd/da9052/da9052.h
index 21e21b8..bba65f5 100644
--- a/include/linux/mfd/da9052/da9052.h
+++ b/include/linux/mfd/da9052/da9052.h
@@ -83,6 +83,7 @@ enum da9052_chip_id {
DA9053_AA,
DA9053_BA,
DA9053_BB,
+ DA9053_BC,
};
struct da9052_pdata;
--
end-of-patch 1/3 for PATCH V1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V1 0/3] da9052: Add new supported chip
This is submission attempt number 1 to have this patch sequence applied to the linux kernel source tree. The DA9052 and DA9053 are low power Power Management Integrated Circuits with extra functionality beyond the power regulators. There is now a new variant of the DA9053 PMIC called BC, and this patch sequence provides for it's support. There is one difference between it and the AA, BA and BB variants. There is also the correctiion of a typing mistake in one of the BA name strings that was erroneously typed as ab. This patch sequence depends on the first being applied first because it provides the hash definition of DA9053_BC Many thanks, Anthony Olech, Dialog Semiconductor Opensource [Anthony Olech] (3): include/linux/mfd/da9052/da9052: Add new BC chip MFD: da9052: Add new DA9053 BC chip variant REGULATOR: da9052: Add new DA9053 BC chip variant drivers/mfd/da9052-i2c.c |5 - drivers/mfd/da9052-spi.c |1 + drivers/regulator/da9052-regulator.c |1 + include/linux/mfd/da9052/da9052.h|1 + 4 files changed, 7 insertions(+), 1 deletion(-) -- end-of-patch 0/3 for PATCH V1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH V1] da9052: ONKEY: use correct register bit for key status
> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: 11 February 2014 16:29
..[snip]..
> > static void da9052_onkey_query(struct da9052_onkey *onkey) {
> > - int key_stat;
> > + int ret, key_stat;
> >
> > - key_stat = da9052_reg_read(onkey->da9052,
> DA9052_EVENT_B_REG);
> > - if (key_stat < 0) {
> > + ret = da9052_reg_read(onkey->da9052, DA9052_STATUS_A_REG);
> > + if (ret < 0) {
> > dev_err(onkey->da9052->dev,
> > - "Failed to read onkey event %d\n", key_stat);
> > + "Failed to read onkey event err=%d\n", ret);
> > + key_stat = false;
>
> Why do you need this assignment? Also, key_stat is integer, why are we
> using boolean values for it?
>
> > } else {
> > /*
> > * Since interrupt for deassertion of ONKEY pin is not
> > * generated, onkey event state determines the onkey
> > * button state.
> > */
> > - key_stat &= DA9052_EVENTB_ENONKEY;
> > + if (ret & DA9052_STATUSA_NONKEY)
> > + key_stat = false;
> > + else
> > + key_stat = true;
>
> It seems to me that the relevant changes are replacement of
> DA9052_EVENT_B_REG -> DA9052_STATUS_A_REG in da9052_reg_read()
> and doing:
>
> key_stat &= DA9052_STATUSA_NONKEY;
> input_report_key(onkey->input, KEY_POWER, !key_stat);
>
> Right?
Right as far as the register and the bit within it.
However, should the register read fail due to the PMIC not responding on the
i2c bus then the previous code would just loop round the work queue.
By explicitly using key_stat as a boolean it makes the code very clear that in
the case of failure a 'false' value is set. As it was the negative value of the
error code would be treated as true, since -EFAULT being non-zero will be
treated as true.
Tony Olech
> Thanks.
> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH V1] da9052: ONKEY: use correct register bit for key status
-Original Message-
From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
Sent: 11 February 2014 16:29
..[snip]..
static void da9052_onkey_query(struct da9052_onkey *onkey) {
- int key_stat;
+ int ret, key_stat;
- key_stat = da9052_reg_read(onkey-da9052,
DA9052_EVENT_B_REG);
- if (key_stat 0) {
+ ret = da9052_reg_read(onkey-da9052, DA9052_STATUS_A_REG);
+ if (ret 0) {
dev_err(onkey-da9052-dev,
- Failed to read onkey event %d\n, key_stat);
+ Failed to read onkey event err=%d\n, ret);
+ key_stat = false;
Why do you need this assignment? Also, key_stat is integer, why are we
using boolean values for it?
} else {
/*
* Since interrupt for deassertion of ONKEY pin is not
* generated, onkey event state determines the onkey
* button state.
*/
- key_stat = DA9052_EVENTB_ENONKEY;
+ if (ret DA9052_STATUSA_NONKEY)
+ key_stat = false;
+ else
+ key_stat = true;
It seems to me that the relevant changes are replacement of
DA9052_EVENT_B_REG - DA9052_STATUS_A_REG in da9052_reg_read()
and doing:
key_stat = DA9052_STATUSA_NONKEY;
input_report_key(onkey-input, KEY_POWER, !key_stat);
Right?
Right as far as the register and the bit within it.
However, should the register read fail due to the PMIC not responding on the
i2c bus then the previous code would just loop round the work queue.
By explicitly using key_stat as a boolean it makes the code very clear that in
the case of failure a 'false' value is set. As it was the negative value of the
error code would be treated as true, since -EFAULT being non-zero will be
treated as true.
Tony Olech
Thanks.
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH V1] fix da9052 volatile register definition ommissions
> -Original Message-
> From: Lee Jones [mailto:lee.jo...@linaro.org]
> Sent: 03 February 2014 10:29
> To: Opensource [Anthony Olech]
> Cc: Mark Brown; Samuel Ortiz; linux-kernel@vger.kernel.org; David Dajun
> Chen
> Subject: Re: [PATCH V1] fix da9052 volatile register definition ommissions
> > Three of the PMIC registers have some bits that are changed
> > autonomously by the PMIC itself (some time) after being set by some
> > component driver of the DA9052 PMIC and hence they need to be marked
> > as volatile so that the regmap API will not cache their values.
> > Signed-off-by: Anthony Olech
> > Signed-off-by: David Dajun Chen
> These are not correct.
> Who authored the patch?
Hi Lee,
I found the problem when running regression tests for another different problem.
And according to my testing on a SMDK6410+DA9053EVB the patch is correct!!
Tony Olech
> > ---
> > This patch is relative to linux-next repository tag next-20140128
> > The bug that this patch fixes affects two components of DA9052 namely:
> > WATCHDOG - the first kick will work but sebsequent ones will not
> >thus the will timeout at 2 x interval.
> > REGULATORS - the first change to any DA9052 BUCK voltage will be
> > actioned, but sebsequent ones will not.
> Which patch caused the bug?
I will find out when I start rebasing backwards to submit patches to
linux-stable!
> > drivers/mfd/da9052-core.c |3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/mfd/da9052-core.c b/drivers/mfd/da9052-core.c
> > index 25838f1..e8af816 100644
> > --- a/drivers/mfd/da9052-core.c
> > +++ b/drivers/mfd/da9052-core.c
> > @@ -279,6 +279,9 @@ static bool da9052_reg_volatile(struct device *dev,
> unsigned int reg)
> > case DA9052_EVENT_B_REG:
> > case DA9052_EVENT_C_REG:
> > case DA9052_EVENT_D_REG:
> > + case DA9052_CONTROL_B_REG:
> > + case DA9052_CONTROL_D_REG:
> > + case DA9052_SUPPLY_REG:
> > case DA9052_FAULTLOG_REG:
> > case DA9052_CHG_TIME_REG:
> > case DA9052_ADC_RES_L_REG:
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
N�r��yb�X��ǧv�^�)Þº{.n�+{zX����ܨ}ï¿½ï¿½ï¿½Æ z�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jÇ«y�m��@A�a���
0��h���i
RE: [PATCH V1] fix da9052 volatile register definition ommissions
-Original Message-
From: Lee Jones [mailto:lee.jo...@linaro.org]
Sent: 03 February 2014 10:29
To: Opensource [Anthony Olech]
Cc: Mark Brown; Samuel Ortiz; linux-kernel@vger.kernel.org; David Dajun
Chen
Subject: Re: [PATCH V1] fix da9052 volatile register definition ommissions
Three of the PMIC registers have some bits that are changed
autonomously by the PMIC itself (some time) after being set by some
component driver of the DA9052 PMIC and hence they need to be marked
as volatile so that the regmap API will not cache their values.
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
Signed-off-by: David Dajun Chen david.c...@diasemi.com
These are not correct.
Who authored the patch?
Hi Lee,
I found the problem when running regression tests for another different problem.
And according to my testing on a SMDK6410+DA9053EVB the patch is correct!!
Tony Olech
---
This patch is relative to linux-next repository tag next-20140128
The bug that this patch fixes affects two components of DA9052 namely:
WATCHDOG - the first kick will work but sebsequent ones will not
thus the will timeout at 2 x interval.
REGULATORS - the first change to any DA9052 BUCK voltage will be
actioned, but sebsequent ones will not.
Which patch caused the bug?
I will find out when I start rebasing backwards to submit patches to
linux-stable!
drivers/mfd/da9052-core.c |3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/da9052-core.c b/drivers/mfd/da9052-core.c
index 25838f1..e8af816 100644
--- a/drivers/mfd/da9052-core.c
+++ b/drivers/mfd/da9052-core.c
@@ -279,6 +279,9 @@ static bool da9052_reg_volatile(struct device *dev,
unsigned int reg)
case DA9052_EVENT_B_REG:
case DA9052_EVENT_C_REG:
case DA9052_EVENT_D_REG:
+ case DA9052_CONTROL_B_REG:
+ case DA9052_CONTROL_D_REG:
+ case DA9052_SUPPLY_REG:
case DA9052_FAULTLOG_REG:
case DA9052_CHG_TIME_REG:
case DA9052_ADC_RES_L_REG:
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
N�r��yb�X��ǧv�^�)Þº{.n�+{zX����ܨ}ï¿½ï¿½ï¿½Æ z�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jÇ«y�m��@A�a���
0��h���i
RE: [PATCH V1] input: use device managed memory in da9052 touchscreen driver
> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: 09 January 2014 19:13
> To: Opensource [Anthony Olech]
> Cc: linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org; Huqiu Liu;
> David Dajun Chen
> Subject: Re: [PATCH V1] input: use device managed memory in da9052
> touchscreen driver
>
> Hi Anthony,
>
> On Thu, Jan 09, 2014 at 12:51:37PM +, Anthony Olech wrote:
> > The touchscreen component driver for the da9052/3 Dialog PMICs is
> > changed to use device managed memory allocation.
> >
> > This results in simpler error paths as failures in the probe()
> > function do not require explicit calls to free the devm_...
> > allocated memory.
> > The allocation functions used in this driver are:
> > devm_kzalloc()
> > devm_input_allocate_device()
> > devm_request_threaded_irq()
> >
> > Suggested-by: Huqiu Liu
> > Signed-off-by: Anthony Olech
> > ---
> > This patch is relative to linux-next repository tag next-20140109
> >
> > Many thanks to Huqiu Liu who instigated this patch.
> >
> > drivers/input/touchscreen/da9052_tsi.c | 62 ---
> -
> > 1 file changed, 30 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/da9052_tsi.c
> > b/drivers/input/touchscreen/da9052_tsi.c
> > index ab64d58..dcc4cf1 100644
> > --- a/drivers/input/touchscreen/da9052_tsi.c
> > +++ b/drivers/input/touchscreen/da9052_tsi.c
> > @@ -233,18 +233,30 @@ static int da9052_ts_probe(struct
> platform_device *pdev)
> > struct da9052_tsi *tsi;
> > struct input_dev *input_dev;
> > int error;
> > + int pdown_irq;
> > + int ready_irq;
> >
> > da9052 = dev_get_drvdata(pdev->dev.parent);
> > if (!da9052)
> > return -EINVAL;
> >
> > - tsi = kzalloc(sizeof(struct da9052_tsi), GFP_KERNEL);
> > - input_dev = input_allocate_device();
> > - if (!tsi || !input_dev) {
> > - error = -ENOMEM;
> > - goto err_free_mem;
> > - }
> > + pdown_irq = regmap_irq_get_virq(da9052->irq_data,
> DA9052_IRQ_PENDOWN);
> > + if (pdown_irq < 0)
> > + return pdown_irq;
> ...
> >
> > - error = da9052_request_irq(tsi->da9052, DA9052_IRQ_PENDOWN,
> > - "pendown-irq", da9052_ts_pendwn_irq, tsi);
> > + error = devm_request_threaded_irq(>dev, pdown_irq,
> > + NULL, da9052_ts_pendwn_irq,
> > + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > + "pendown-irq", tsi);
>
> I am uncomfortable with the touchscreen portion of this driver ignoring the
> framework of it's MFD core and mixing native IRQ management with the
> ones done through the core.
>
> What would happen if somebody changes da9052_request_irq() to do some
> thing more than it is doing now so that your open-coded duplicate of the
> same in da9052_ts_probe() is no longer equivalent? Or
> da9052_disable_irq() no longer works correctly with IRQs allocated by this
> sub-module?
> Thanks.
> --
> Dmitry
Hi Dmitry,
unfortunately the PMIC is a multifunction device and the component drivers come
under different subsystem maintainers. Thus it is not possible to do one patch
in one
go to change them all.
Tony Olech
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH V1] input: use device managed memory in da9052 touchscreen driver
-Original Message-
From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
Sent: 09 January 2014 19:13
To: Opensource [Anthony Olech]
Cc: linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org; Huqiu Liu;
David Dajun Chen
Subject: Re: [PATCH V1] input: use device managed memory in da9052
touchscreen driver
Hi Anthony,
On Thu, Jan 09, 2014 at 12:51:37PM +, Anthony Olech wrote:
The touchscreen component driver for the da9052/3 Dialog PMICs is
changed to use device managed memory allocation.
This results in simpler error paths as failures in the probe()
function do not require explicit calls to free the devm_...
allocated memory.
The allocation functions used in this driver are:
devm_kzalloc()
devm_input_allocate_device()
devm_request_threaded_irq()
Suggested-by: Huqiu Liu liuh...@mails.tsinghua.edu.cn
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
---
This patch is relative to linux-next repository tag next-20140109
Many thanks to Huqiu Liu who instigated this patch.
drivers/input/touchscreen/da9052_tsi.c | 62 ---
-
1 file changed, 30 insertions(+), 32 deletions(-)
diff --git a/drivers/input/touchscreen/da9052_tsi.c
b/drivers/input/touchscreen/da9052_tsi.c
index ab64d58..dcc4cf1 100644
--- a/drivers/input/touchscreen/da9052_tsi.c
+++ b/drivers/input/touchscreen/da9052_tsi.c
@@ -233,18 +233,30 @@ static int da9052_ts_probe(struct
platform_device *pdev)
struct da9052_tsi *tsi;
struct input_dev *input_dev;
int error;
+ int pdown_irq;
+ int ready_irq;
da9052 = dev_get_drvdata(pdev-dev.parent);
if (!da9052)
return -EINVAL;
- tsi = kzalloc(sizeof(struct da9052_tsi), GFP_KERNEL);
- input_dev = input_allocate_device();
- if (!tsi || !input_dev) {
- error = -ENOMEM;
- goto err_free_mem;
- }
+ pdown_irq = regmap_irq_get_virq(da9052-irq_data,
DA9052_IRQ_PENDOWN);
+ if (pdown_irq 0)
+ return pdown_irq;
...
- error = da9052_request_irq(tsi-da9052, DA9052_IRQ_PENDOWN,
- pendown-irq, da9052_ts_pendwn_irq, tsi);
+ error = devm_request_threaded_irq(pdev-dev, pdown_irq,
+ NULL, da9052_ts_pendwn_irq,
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+ pendown-irq, tsi);
I am uncomfortable with the touchscreen portion of this driver ignoring the
framework of it's MFD core and mixing native IRQ management with the
ones done through the core.
What would happen if somebody changes da9052_request_irq() to do some
thing more than it is doing now so that your open-coded duplicate of the
same in da9052_ts_probe() is no longer equivalent? Or
da9052_disable_irq() no longer works correctly with IRQs allocated by this
sub-module?
Thanks.
--
Dmitry
Hi Dmitry,
unfortunately the PMIC is a multifunction device and the component drivers come
under different subsystem maintainers. Thus it is not possible to do one patch
in one
go to change them all.
Tony Olech
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH V1] input: fix memory leak in da9052 touchscreen driver
> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: 08 January 2014 17:26
> To: Opensource [Anthony Olech]
> Cc: linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org; Huqiu Liu;
> David Dajun Chen
> Subject: Re: [PATCH V1] input: fix memory leak in da9052 touchscreen driver
> Anthony Olech wrote:
> >The touchscreen component driver for the da9052/3 Dialog PMICs leaks
> >memory by not freeing the input device in the remove call.
> >This patch matches the existing call to input_alloc_device() in
> >da9052_ts_probe() to a new call to input_free_device() in
> >da9052_ts_remove()
> >Suggested-by: Huqiu Liu
> >Signed-off-by: Anthony Olech
> >---
> >This patch is relative to linux-next repository tag next-20140108
> >Many thanks to Huqiu Liu who found the bug.
> No, this is not a bug. Please refer to input API spec in input.h
> Thanks.
Hi Dmitry,
the spec *seems* to say that if the allocation is done via
devm_input_allocate_device(dev)
then no explicit freeing must be done.
However that is not the case here, so the bug exists.
It does seem that there is an alternative way of resolving the issue, namely to:
1) change from allocate_device() to devm_input_allocate_device(dev) and
2) remove the exiosting input_free_device() from the error path in the probe()
function
I will update the patch submission to the alternative tomorrow
Tony Olech (Dialog Semiconductor)
> > drivers/input/touchscreen/da9052_tsi.c |2 ++
> > 1 file changed, 2 insertions(+)
> >diff --git a/drivers/input/touchscreen/da9052_tsi.c
> >b/drivers/input/touchscreen/da9052_tsi.c
> >index ab64d58..43a69d1 100644
> >--- a/drivers/input/touchscreen/da9052_tsi.c
> >+++ b/drivers/input/touchscreen/da9052_tsi.c
> >@@ -320,6 +320,7 @@ err_free_mem:
> > static int da9052_ts_remove(struct platform_device *pdev) {
> > struct da9052_tsi *tsi = platform_get_drvdata(pdev);
> >+struct input_dev *input_dev = tsi->dev;
> > da9052_reg_write(tsi->da9052, DA9052_LDO9_REG, 0x19);
> >@@ -328,6 +329,7 @@ static int da9052_ts_remove(struct
> platform_device
> >*pdev)
> > input_unregister_device(tsi->dev);
> > kfree(tsi);
> >+input_free_device(input_dev);
> > return 0;
> > }
> --
> Dmitry
RE: [PATCH V1] input: fix memory leak in da9052 touchscreen driver
-Original Message-
From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
Sent: 08 January 2014 17:26
To: Opensource [Anthony Olech]
Cc: linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org; Huqiu Liu;
David Dajun Chen
Subject: Re: [PATCH V1] input: fix memory leak in da9052 touchscreen driver
Anthony Olech anthony.olech.opensou...@diasemi.com wrote:
The touchscreen component driver for the da9052/3 Dialog PMICs leaks
memory by not freeing the input device in the remove call.
This patch matches the existing call to input_alloc_device() in
da9052_ts_probe() to a new call to input_free_device() in
da9052_ts_remove()
Suggested-by: Huqiu Liu liuh...@mails.tsinghua.edu.cn
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
---
This patch is relative to linux-next repository tag next-20140108
Many thanks to Huqiu Liu who found the bug.
No, this is not a bug. Please refer to input API spec in input.h
Thanks.
Hi Dmitry,
the spec *seems* to say that if the allocation is done via
devm_input_allocate_device(dev)
then no explicit freeing must be done.
However that is not the case here, so the bug exists.
It does seem that there is an alternative way of resolving the issue, namely to:
1) change from allocate_device() to devm_input_allocate_device(dev) and
2) remove the exiosting input_free_device() from the error path in the probe()
function
I will update the patch submission to the alternative tomorrow
Tony Olech (Dialog Semiconductor)
drivers/input/touchscreen/da9052_tsi.c |2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/touchscreen/da9052_tsi.c
b/drivers/input/touchscreen/da9052_tsi.c
index ab64d58..43a69d1 100644
--- a/drivers/input/touchscreen/da9052_tsi.c
+++ b/drivers/input/touchscreen/da9052_tsi.c
@@ -320,6 +320,7 @@ err_free_mem:
static int da9052_ts_remove(struct platform_device *pdev) {
struct da9052_tsi *tsi = platform_get_drvdata(pdev);
+struct input_dev *input_dev = tsi-dev;
da9052_reg_write(tsi-da9052, DA9052_LDO9_REG, 0x19);
@@ -328,6 +329,7 @@ static int da9052_ts_remove(struct
platform_device
*pdev)
input_unregister_device(tsi-dev);
kfree(tsi);
+input_free_device(input_dev);
return 0;
}
--
Dmitry
RE: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver
> -Original Message- > From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter > Roeck > Sent: 19 December 2013 18:11 > To: Jean Delvare > Cc: Opensource [Anthony Olech]; lm-sens...@lm-sensors.org; Anton > Vorontsov; David Woodhouse; linux-kernel@vger.kernel.org; David Dajun > Chen > Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 > power driver > On Thu, Dec 19, 2013 at 03:54:54PM +0100, Jean Delvare wrote: > > Hi Anthony, > > > [ ... ] > > BTW, you (and we) probably shouldn't waste too much energy on this. > > ADCs are only accurate to some degree anyway. If you take the > > components connected to the input into consideration, the accuracy > > gets even worse. For example, if the input voltage must be scaled down > > to fit in the ADC's range, you need at least one resistor, which in > > best cases will have a 1% accurate value (known as tolerance.) That's > > ten times the LSB of your 10-bit ADC, at which point / 1023 or / 1024 > > really makes no practical difference. Sub-percent tolerant resistors > > are expensive and rare in consumer electronics in my experience. > True, but on the other side (and after looking into the datasheet) the driver > already calculated the voltage on adc4..6 correctly, so unless you disagree I > would like to apply the hwmon patch to -next for consistency. > Thanks, > Guenter OK, thanks, Tony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver
> -Original Message-
> From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: 18 December 2013 17:24
> To: Opensource [Anthony Olech]
> Cc: Jean Delvare; open list:HARDWARE MONITORING; Anton Vorontsov;
> David Woodhouse; open list; David Dajun Chen
> Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052
> power driver
> On Wed, Dec 18, 2013 at 03:45:32PM +0000, Opensource [Anthony Olech]
> wrote:
> > > -Original Message-
> > > From: Jean Delvare [mailto:kh...@linux-fr.org]
> > > Sent: 18 December 2013 15:33
> > > To: Opensource [Anthony Olech]
> > > Cc: Anton Vorontsov; David Woodhouse; David Dajun Chen; open list;
> > > open list:HARDWARE MONITORING
> > > Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation
> > > in da9052 power driver On Wed, 18 Dec 2013 15:21:13 +, Anthony
> > > Olech wrote:
> > > > The ADC resolution of the PMIC is 10-bits, this means that the
> > > > maximum possible value is 1023 and not the 1024 as in the code.
> > > The conversion from register value to mV depends on the ADC's LSB,
> > > not its range. So the maximum value which can be represented is
> irrelevant.
> > thanks for the speedy response, but the converted value returned by
> > the function does depend on the scaling.
> > For example take the two cases where value in 0x1F0, then the
> > erroneous previous calculation yields 3469 and the new corrected
> > calculation yields 3470
> > So please apply the patch as it truly fixes an error.
> AFAICS the previous calculation should return 3468. Replacing the division by
> 512 with DIV_ROUND_CLOSEST would return 3469, and the new calculation
> would return 3470.
> Still, an ADC would typically have an LSB. Question here is what that LSB is
> (in
> mV), or in other words what the maximim voltage represented by 0x3ff is.
> Current assumption is that it is 1.9531 mV or 2000/1024, which admittedly is
> odd. It might be 2000/1023 = 1.9550 mV, or it might be 2 mV.
the maximum voltage represented by 0x3ff is 2000 mV, so the existing code is
incorrect and the patch is to correct the error.
many thanks for your comment,
Tony Olech
> Thanks,
> Guenter
> > many thanks
> > Tony Olech
> > > > Signed-off-by: Anthony Olech
> > > >
> > > > This patch is relative to linux-next repository tag next-20131218
> > > > drivers/power/da9052-battery.c |2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-) diff --git
> > > > a/drivers/power/da9052-battery.c b/drivers/power/da9052-battery.c
> > > > index f8f4c0f..8f0f259 100644
> > > > --- a/drivers/power/da9052-battery.c
> > > > +++ b/drivers/power/da9052-battery.c
> > > > @@ -178,7 +178,7 @@ struct da9052_battery { static inline int
> > > > volt_reg_to_mV(int value) {
> > > > - return ((value * 1000) / 512) + 2500;
> > > > + return DIV_ROUND_CLOSEST(value * 2000, 1023) + 2500;
> > > > }
> > > > static inline int ichg_reg_to_mA(int value)
> > > Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver
-Original Message-
From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter
Roeck
Sent: 18 December 2013 17:24
To: Opensource [Anthony Olech]
Cc: Jean Delvare; open list:HARDWARE MONITORING; Anton Vorontsov;
David Woodhouse; open list; David Dajun Chen
Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052
power driver
On Wed, Dec 18, 2013 at 03:45:32PM +, Opensource [Anthony Olech]
wrote:
-Original Message-
From: Jean Delvare [mailto:kh...@linux-fr.org]
Sent: 18 December 2013 15:33
To: Opensource [Anthony Olech]
Cc: Anton Vorontsov; David Woodhouse; David Dajun Chen; open list;
open list:HARDWARE MONITORING
Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation
in da9052 power driver On Wed, 18 Dec 2013 15:21:13 +, Anthony
Olech wrote:
The ADC resolution of the PMIC is 10-bits, this means that the
maximum possible value is 1023 and not the 1024 as in the code.
The conversion from register value to mV depends on the ADC's LSB,
not its range. So the maximum value which can be represented is
irrelevant.
thanks for the speedy response, but the converted value returned by
the function does depend on the scaling.
For example take the two cases where value in 0x1F0, then the
erroneous previous calculation yields 3469 and the new corrected
calculation yields 3470
So please apply the patch as it truly fixes an error.
AFAICS the previous calculation should return 3468. Replacing the division by
512 with DIV_ROUND_CLOSEST would return 3469, and the new calculation
would return 3470.
Still, an ADC would typically have an LSB. Question here is what that LSB is
(in
mV), or in other words what the maximim voltage represented by 0x3ff is.
Current assumption is that it is 1.9531 mV or 2000/1024, which admittedly is
odd. It might be 2000/1023 = 1.9550 mV, or it might be 2 mV.
the maximum voltage represented by 0x3ff is 2000 mV, so the existing code is
incorrect and the patch is to correct the error.
many thanks for your comment,
Tony Olech
Thanks,
Guenter
many thanks
Tony Olech
Signed-off-by: Anthony Olech
anthony.olech.opensou...@diasemi.com
This patch is relative to linux-next repository tag next-20131218
drivers/power/da9052-battery.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-) diff --git
a/drivers/power/da9052-battery.c b/drivers/power/da9052-battery.c
index f8f4c0f..8f0f259 100644
--- a/drivers/power/da9052-battery.c
+++ b/drivers/power/da9052-battery.c
@@ -178,7 +178,7 @@ struct da9052_battery { static inline int
volt_reg_to_mV(int value) {
- return ((value * 1000) / 512) + 2500;
+ return DIV_ROUND_CLOSEST(value * 2000, 1023) + 2500;
}
static inline int ichg_reg_to_mA(int value)
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver
-Original Message- From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter Roeck Sent: 19 December 2013 18:11 To: Jean Delvare Cc: Opensource [Anthony Olech]; lm-sens...@lm-sensors.org; Anton Vorontsov; David Woodhouse; linux-kernel@vger.kernel.org; David Dajun Chen Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver On Thu, Dec 19, 2013 at 03:54:54PM +0100, Jean Delvare wrote: Hi Anthony, [ ... ] BTW, you (and we) probably shouldn't waste too much energy on this. ADCs are only accurate to some degree anyway. If you take the components connected to the input into consideration, the accuracy gets even worse. For example, if the input voltage must be scaled down to fit in the ADC's range, you need at least one resistor, which in best cases will have a 1% accurate value (known as tolerance.) That's ten times the LSB of your 10-bit ADC, at which point / 1023 or / 1024 really makes no practical difference. Sub-percent tolerant resistors are expensive and rare in consumer electronics in my experience. True, but on the other side (and after looking into the datasheet) the driver already calculated the voltage on adc4..6 correctly, so unless you disagree I would like to apply the hwmon patch to -next for consistency. Thanks, Guenter OK, thanks, Tony Olech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver
> -Original Message-
> From: Jean Delvare [mailto:kh...@linux-fr.org]
> Sent: 18 December 2013 15:33
> To: Opensource [Anthony Olech]
> Cc: Anton Vorontsov; David Woodhouse; David Dajun Chen; open list; open
> list:HARDWARE MONITORING
> Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052
> power driver
> On Wed, 18 Dec 2013 15:21:13 +, Anthony Olech wrote:
> > The ADC resolution of the PMIC is 10-bits, this means that the maximum
> > possible value is 1023 and not the 1024 as in the code.
> The conversion from register value to mV depends on the ADC's LSB, not its
> range. So the maximum value which can be represented is irrelevant.
thanks for the speedy response, but the converted value returned by the function
does depend on the scaling.
For example take the two cases where value in 0x1F0, then the erroneous previous
calculation yields 3469 and the new corrected calculation yields 3470
So please apply the patch as it truly fixes an error.
many thanks
Tony Olech
> > Signed-off-by: Anthony Olech
> > ---
> > This patch is relative to linux-next repository tag next-20131218
> > drivers/power/da9052-battery.c |2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > diff --git a/drivers/power/da9052-battery.c
> > b/drivers/power/da9052-battery.c index f8f4c0f..8f0f259 100644
> > --- a/drivers/power/da9052-battery.c
> > +++ b/drivers/power/da9052-battery.c
> > @@ -178,7 +178,7 @@ struct da9052_battery {
> > static inline int volt_reg_to_mV(int value) {
> > - return ((value * 1000) / 512) + 2500;
> > + return DIV_ROUND_CLOSEST(value * 2000, 1023) + 2500;
> > }
> > static inline int ichg_reg_to_mA(int value)
> --
> Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver
-Original Message-
From: Jean Delvare [mailto:kh...@linux-fr.org]
Sent: 18 December 2013 15:33
To: Opensource [Anthony Olech]
Cc: Anton Vorontsov; David Woodhouse; David Dajun Chen; open list; open
list:HARDWARE MONITORING
Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052
power driver
On Wed, 18 Dec 2013 15:21:13 +, Anthony Olech wrote:
The ADC resolution of the PMIC is 10-bits, this means that the maximum
possible value is 1023 and not the 1024 as in the code.
The conversion from register value to mV depends on the ADC's LSB, not its
range. So the maximum value which can be represented is irrelevant.
thanks for the speedy response, but the converted value returned by the function
does depend on the scaling.
For example take the two cases where value in 0x1F0, then the erroneous previous
calculation yields 3469 and the new corrected calculation yields 3470
So please apply the patch as it truly fixes an error.
many thanks
Tony Olech
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
---
This patch is relative to linux-next repository tag next-20131218
drivers/power/da9052-battery.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/da9052-battery.c
b/drivers/power/da9052-battery.c index f8f4c0f..8f0f259 100644
--- a/drivers/power/da9052-battery.c
+++ b/drivers/power/da9052-battery.c
@@ -178,7 +178,7 @@ struct da9052_battery {
static inline int volt_reg_to_mV(int value) {
- return ((value * 1000) / 512) + 2500;
+ return DIV_ROUND_CLOSEST(value * 2000, 1023) + 2500;
}
static inline int ichg_reg_to_mA(int value)
--
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH V1] new API regmap_multi_reg_write() definition
> -Original Message- > From: Mark Brown [mailto:broo...@kernel.org] > Sent: 14 October 2013 15:15 > To: Opensource [Anthony Olech] > Cc: Greg Kroah-Hartman; LKML; David Dajun Chen > Subject: Re: [PATCH V1] new API regmap_multi_reg_write() definition > > On Fri, Oct 11, 2013 at 03:31:11PM +0100, Anthony Olech wrote: > > New API regmap_multi_reg_write() is defined that allows a set of > > reg,val pairs to be written to a I2C client device as one block > > transfer from the point of view of a single I2C master system. > > Applied, thanks. I'll also push out a tag "multi-write" that other subsystems > can pull to start using the new API prior to it hitting Linus' tree. > > As well as implementing the feature for your devices it would be good to > refactor the code so it's implemented inside an internal version of the > function so that things like patch application can be implemented in terms of > this - that way they'll benefit from any work done here. I will work on that basis, but I expect a couple of iterations as there is room for me to have misunderstood you. Thanks Tony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH V1] new API regmap_multi_reg_write() definition
-Original Message- From: Mark Brown [mailto:broo...@kernel.org] Sent: 14 October 2013 15:15 To: Opensource [Anthony Olech] Cc: Greg Kroah-Hartman; LKML; David Dajun Chen Subject: Re: [PATCH V1] new API regmap_multi_reg_write() definition On Fri, Oct 11, 2013 at 03:31:11PM +0100, Anthony Olech wrote: New API regmap_multi_reg_write() is defined that allows a set of reg,val pairs to be written to a I2C client device as one block transfer from the point of view of a single I2C master system. Applied, thanks. I'll also push out a tag multi-write that other subsystems can pull to start using the new API prior to it hitting Linus' tree. As well as implementing the feature for your devices it would be good to refactor the code so it's implemented inside an internal version of the function so that things like patch application can be implemented in terms of this - that way they'll benefit from any work done here. I will work on that basis, but I expect a couple of iterations as there is room for me to have misunderstood you. Thanks Tony Olech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH V1] new API regmap_multi_write()
> -Original Message-
> From: Mark Brown [mailto:broo...@kernel.org]
> Sent: 10 October 2013 13:21
> To: Opensource [Anthony Olech]
> Cc: Greg Kroah-Hartman; LKML; David Dajun Chen
> Subject: Re: [PATCH V1] new API regmap_multi_write()
>
> On Thu, Oct 10, 2013 at 11:50:23AM +0100, Anthony Olech wrote:
>
> > +/*
> > + * regmap_multi_write(): Write multiple non sequential registers to
> > +the device
> > + *
> > + * @map: Register map to write to
> > + * @reg: Array of registers to be written, all on the same page
>
> Why all on the same page? That doesn't seem helpful for things trying to
> build on top of this. We currently manage to split even block writes up over
> page boundaries.
As far as I could see, the splitting of a block write that spans a page boundary
requires a read-modify-write of a "page" register. That therefore breaks the
primary raison d'etre for the new API, namely that it is atomic on the I2C bus
with respect to multiple I2C bus masters.
Cutting the transfer at a page boundary and inserting a write to a "page"
register would work very well for most of our PM MFDs because there is
no requirement to do a read modify write. Indeed I had thought that it should
be the responsibility of the device driver to insert any necessary "page"
register
writes into a transfer that spans page boundaries. The driver knows where the
boundaries are so it should be easy.
>
> > + * @val: Block of data to be written, in native register size for
> > + device
> > + * @reg_count: Number of registers to write
>
> I'm not seeing anything here which says how the registers and values are
> related to each other. I assume that the idea is that the same number of
> registers are provided as values...
Yes - 2 arrays with the same dimension.
> This really doesn't feel like an idiomatic abstraction - it's a bit
> cumbersome to
> have the pair of arrays and try to line them up especially in native register
> format, normally we do this with an array reg_default. This would also mean
> that generic code like patches and cache syncing could pick up on the same
> functionality.
You seem to be suggesting that the API could be used by drivers of devices that
do not support in hardware the MULTIWRITE capability. If that is the case then
driver need an config "multi_write_supported" field for initialization.
> > + /*
> > +* Some devices do not support multi write, for
> > +* them we have a series of single write operations.
> > +*/
> > + if (map->use_single_rw) {
>
> single_rw is somewhat relevant but this needs a separate flag since...
>
> > + } else {
> > + ret = _regmap_raw_multi_write(map,
> > + reg_count,
> > + reg,
> > + wval);
> > + }
>
> ...this will try to use the new functionality even if the device doesn't
> support
> it. Indeed what this looks like is support for devices that can only do
> single
> register writes but in the I2C case allow it to be done without releasing the
> bus (it looks a lot like someone optimised things to look like a bunch of SPI
> register writes, with SPI bouncing /CS is much quicker than starting a new I2C
> transfer is).
>
> I can see it being nice to have something like this but it needs more thought
> on the API and implementation. I'd suggest splitting the API addition from
> the underlying implementation to make things easier to review (the API
> should work for any user even if it just ends up as a series of separate
> register writes). Something like DAPM in ASoC could use it for example, as
> well as the patch and cache sync code.
Thanks Mark for your thoughtful comments.
What should the next step in progressing this be?
Tony Olech - Dialog Semiconductor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH V1] new API regmap_multi_write()
-Original Message-
From: Mark Brown [mailto:broo...@kernel.org]
Sent: 10 October 2013 13:21
To: Opensource [Anthony Olech]
Cc: Greg Kroah-Hartman; LKML; David Dajun Chen
Subject: Re: [PATCH V1] new API regmap_multi_write()
On Thu, Oct 10, 2013 at 11:50:23AM +0100, Anthony Olech wrote:
+/*
+ * regmap_multi_write(): Write multiple non sequential registers to
+the device
+ *
+ * @map: Register map to write to
+ * @reg: Array of registers to be written, all on the same page
Why all on the same page? That doesn't seem helpful for things trying to
build on top of this. We currently manage to split even block writes up over
page boundaries.
As far as I could see, the splitting of a block write that spans a page boundary
requires a read-modify-write of a page register. That therefore breaks the
primary raison d'etre for the new API, namely that it is atomic on the I2C bus
with respect to multiple I2C bus masters.
Cutting the transfer at a page boundary and inserting a write to a page
register would work very well for most of our PM MFDs because there is
no requirement to do a read modify write. Indeed I had thought that it should
be the responsibility of the device driver to insert any necessary page
register
writes into a transfer that spans page boundaries. The driver knows where the
boundaries are so it should be easy.
+ * @val: Block of data to be written, in native register size for
+ device
+ * @reg_count: Number of registers to write
I'm not seeing anything here which says how the registers and values are
related to each other. I assume that the idea is that the same number of
registers are provided as values...
Yes - 2 arrays with the same dimension.
This really doesn't feel like an idiomatic abstraction - it's a bit
cumbersome to
have the pair of arrays and try to line them up especially in native register
format, normally we do this with an array reg_default. This would also mean
that generic code like patches and cache syncing could pick up on the same
functionality.
You seem to be suggesting that the API could be used by drivers of devices that
do not support in hardware the MULTIWRITE capability. If that is the case then
driver need an config multi_write_supported field for initialization.
+ /*
+* Some devices do not support multi write, for
+* them we have a series of single write operations.
+*/
+ if (map-use_single_rw) {
single_rw is somewhat relevant but this needs a separate flag since...
+ } else {
+ ret = _regmap_raw_multi_write(map,
+ reg_count,
+ reg,
+ wval);
+ }
...this will try to use the new functionality even if the device doesn't
support
it. Indeed what this looks like is support for devices that can only do
single
register writes but in the I2C case allow it to be done without releasing the
bus (it looks a lot like someone optimised things to look like a bunch of SPI
register writes, with SPI bouncing /CS is much quicker than starting a new I2C
transfer is).
I can see it being nice to have something like this but it needs more thought
on the API and implementation. I'd suggest splitting the API addition from
the underlying implementation to make things easier to review (the API
should work for any user even if it just ends up as a series of separate
register writes). Something like DAPM in ASoC could use it for example, as
well as the patch and cache sync code.
Thanks Mark for your thoughtful comments.
What should the next step in progressing this be?
Tony Olech - Dialog Semiconductor
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver
> -Original Message-
> From: Linus Walleij [mailto:linus.wall...@linaro.org]
> Sent: 03 May 2013 12:59
> To: Opensource [Anthony Olech]
> Cc: Grant Likely; Linus Walleij; Mark Brown; LKML; Lee Jones
> Subject: Re: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver
>
> On Tue, Apr 30, 2013 at 6:17 PM, Opensource [Anthony Olech]
> wrote:
[...]
> >> Maybe it's more logical to have if (!offset) and handle pin
> >> 0 first, then pin 1 in the else clause?
> >
> > why is it logical for one state to be preferred over the other?
>
> Because 0 comes before 1 in the system of numbers?
OK, good enough reason - I will change for next submission.
> >> Please use #define to define these magic bits.
> >> Something like this:
> >>
> >> #include
> >>
> >> #define DA9058_GPIO0_VAL BIT(3)
> >> #define DA9058_GPIO0_INOUT BIT(1)
> >> #define DA9058_GPIO1_VAL BIT(7)
> >> #define DA9058_GPIO1_INOUT BIT(5)
> >>
> >> (etc, so we see what all bits are for)
> >>
> >> then use these defines in the code...
> >
> > the meaning of the bits other than the INP/OUT bits are not
> > independent, but depend on the direction. Thus each nibble contains 3
> dependent bits.
>
> (...)
> >> > + if (offset) {
> >> > + u8 debounce_bits = debounce ? 0x80 : 0x00;
> >>
> >> Is this really correct??
> >>
> >> In earlier code you use bit 7 to set the value!
> >
> > You are confusing input and output operations, the meanings of each
> > nibble's other 3 bits is depends on the direction. That is why the INP
> > and OUT configuration needs to be saved in a control structure.
> >
> >> This is partly why I ask you to use #defines for the bits:
> >> less risk to do things wrong by e.g. copy/paste bugs.
> >
> > The hardware definition is in terms of bit patterns in each nibble, so
> > introducing a dual name for 3 of the 4 bits means double the number of
> > points an error can be made.
>
> (...)
> >> > + gpio->inp_config = 0x99;
> >> > + gpio->out_config = 0x77;
> >>
> >> Again here you should #define the bits and | or them together instead
> >> of using comments to clarify it.
> >
> > I think that in this particular case it will be more confusing trying
> > to name the bits due to the fact that 3 out of 4 bit in each nibble depend
> > on
> the 4th bit.
>
> (...)
> > I failed previously to imagine any naming scheme that would make the
> > meaning of the magic bits in each nibble clearer. The best I could
> > come up with was the comment you referred to.
>
> So add #defines for all combinations, IN and OUT directions.
>
> Also write a comment explaining that the meaning of some bits change
> depending on how other bits are set.
>
> #include
>
>
> /* This bit in each nybble detemines if the pin is input or output */ #define
> DA9058_GPIO0_IN 0 #define DA9058_GPIO1_IN 0 #define
> DA9058_GPIO0_OUT BIT(1) #define DA9058_GPIO1_OUT BIT(5)
> /* This is the meaning of bits 0, 2, 3 in the input mode */ #define
> DA9058_GPIO0_IN_PU BIT(0) #define DA9058_GPIO0_IN_PU BIT(4) #define
> DA9058_GPIO0_IN_DEB BIT(3) #define DA9058_GPIO1_IN_DEB BIT(7) (what
> is bit 2 in input mode?)
> (...)
> /* This is the meaning of bits 0, 2, 3 in the output mode */ #define
> DA9058_GPIO0_OUT_VAL BIT(3) #define DA9058_GPIO0_OUT_PP_EXT BIT(2)
> | BIT(0) #define DA9058_GPIO0_OUT_PP_INT BIT(0) #define
> DA9058_GPIO0_OUT_OD 0 #define DA9058_GPIO1_OUT_VAL BIT(7) #define
> DA9058_GPIO1_OUT_PP_EXT BIT(6) | BIT(4) #define
> DA9058_GPIO1_OUT_PP_INT BIT(4) #define DA9058_GPIO1_OUT_OD 0
> (...)
>
> Then the init in probe() becomes readable:
>
> gpio->inp_config = DA9058_GPIO0_IN |
> DA9058_GPIO0_IN_PU |
> DA9058_GPIO0_IN_DEB |
> DA9058_GPIO1_IN |
> DA9058_GPIO0_IN_PU |
> DA9058_GPIO0_IN_DEB;
> gpio->out_config = DA9058_GPIO0_OUT |
> DA9058_GPIO0_OUT_PP_EXT |
> DA9058_GPIO1_OUT |
> DA9058_GPIO1_OUT_PP_EXT;
>
> This is way more readable than:
>
> gpio->inp_config = 0x99;
> gpio->out_config = 0x77;
>
> After this though, I start to wonder if it's not smarter to just
> have:
>
> struct da9058_gpio {
> (...)
> u8 gpio0_in_config:4;
> u8 gpio0_out_config:4;
> u8 gpio1_in_config:4;
> u8 gpio1_out_config:4;
> };
>
> Then only define one set of bits for a single nybble and use some <<4 to |
> together the apropriate config at runtime.
Thanks for this comment - bit fields look like the best way of managing the
bits. I will change the source for the next submission attempt.
Many thanks for taking the time to review this driver.
Tony Olech
Dialog Semiconductor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver
-Original Message-
From: Linus Walleij [mailto:linus.wall...@linaro.org]
Sent: 03 May 2013 12:59
To: Opensource [Anthony Olech]
Cc: Grant Likely; Linus Walleij; Mark Brown; LKML; Lee Jones
Subject: Re: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver
On Tue, Apr 30, 2013 at 6:17 PM, Opensource [Anthony Olech]
anthony.olech.opensou...@diasemi.com wrote:
[...]
Maybe it's more logical to have if (!offset) and handle pin
0 first, then pin 1 in the else clause?
why is it logical for one state to be preferred over the other?
Because 0 comes before 1 in the system of numbers?
OK, good enough reason - I will change for next submission.
Please use #define to define these magic bits.
Something like this:
#include linux/bitops.h
#define DA9058_GPIO0_VAL BIT(3)
#define DA9058_GPIO0_INOUT BIT(1)
#define DA9058_GPIO1_VAL BIT(7)
#define DA9058_GPIO1_INOUT BIT(5)
(etc, so we see what all bits are for)
then use these defines in the code...
the meaning of the bits other than the INP/OUT bits are not
independent, but depend on the direction. Thus each nibble contains 3
dependent bits.
(...)
+ if (offset) {
+ u8 debounce_bits = debounce ? 0x80 : 0x00;
Is this really correct??
In earlier code you use bit 7 to set the value!
You are confusing input and output operations, the meanings of each
nibble's other 3 bits is depends on the direction. That is why the INP
and OUT configuration needs to be saved in a control structure.
This is partly why I ask you to use #defines for the bits:
less risk to do things wrong by e.g. copy/paste bugs.
The hardware definition is in terms of bit patterns in each nibble, so
introducing a dual name for 3 of the 4 bits means double the number of
points an error can be made.
(...)
+ gpio-inp_config = 0x99;
+ gpio-out_config = 0x77;
Again here you should #define the bits and | or them together instead
of using comments to clarify it.
I think that in this particular case it will be more confusing trying
to name the bits due to the fact that 3 out of 4 bit in each nibble depend
on
the 4th bit.
(...)
I failed previously to imagine any naming scheme that would make the
meaning of the magic bits in each nibble clearer. The best I could
come up with was the comment you referred to.
So add #defines for all combinations, IN and OUT directions.
Also write a comment explaining that the meaning of some bits change
depending on how other bits are set.
#include linux/bitops.h
/* This bit in each nybble detemines if the pin is input or output */ #define
DA9058_GPIO0_IN 0 #define DA9058_GPIO1_IN 0 #define
DA9058_GPIO0_OUT BIT(1) #define DA9058_GPIO1_OUT BIT(5)
/* This is the meaning of bits 0, 2, 3 in the input mode */ #define
DA9058_GPIO0_IN_PU BIT(0) #define DA9058_GPIO0_IN_PU BIT(4) #define
DA9058_GPIO0_IN_DEB BIT(3) #define DA9058_GPIO1_IN_DEB BIT(7) (what
is bit 2 in input mode?)
(...)
/* This is the meaning of bits 0, 2, 3 in the output mode */ #define
DA9058_GPIO0_OUT_VAL BIT(3) #define DA9058_GPIO0_OUT_PP_EXT BIT(2)
| BIT(0) #define DA9058_GPIO0_OUT_PP_INT BIT(0) #define
DA9058_GPIO0_OUT_OD 0 #define DA9058_GPIO1_OUT_VAL BIT(7) #define
DA9058_GPIO1_OUT_PP_EXT BIT(6) | BIT(4) #define
DA9058_GPIO1_OUT_PP_INT BIT(4) #define DA9058_GPIO1_OUT_OD 0
(...)
Then the init in probe() becomes readable:
gpio-inp_config = DA9058_GPIO0_IN |
DA9058_GPIO0_IN_PU |
DA9058_GPIO0_IN_DEB |
DA9058_GPIO1_IN |
DA9058_GPIO0_IN_PU |
DA9058_GPIO0_IN_DEB;
gpio-out_config = DA9058_GPIO0_OUT |
DA9058_GPIO0_OUT_PP_EXT |
DA9058_GPIO1_OUT |
DA9058_GPIO1_OUT_PP_EXT;
This is way more readable than:
gpio-inp_config = 0x99;
gpio-out_config = 0x77;
After this though, I start to wonder if it's not smarter to just
have:
struct da9058_gpio {
(...)
u8 gpio0_in_config:4;
u8 gpio0_out_config:4;
u8 gpio1_in_config:4;
u8 gpio1_out_config:4;
};
Then only define one set of bits for a single nybble and use some 4 to |
together the apropriate config at runtime.
Thanks for this comment - bit fields look like the best way of managing the
bits. I will change the source for the next submission attempt.
Many thanks for taking the time to review this driver.
Tony Olech
Dialog Semiconductor
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver
> -Original Message-
> From: Linus Walleij [mailto:linus.wall...@linaro.org]
> Sent: 24 April 2013 14:15
> To: Opensource [Anthony Olech]
> Cc: Grant Likely; Linus Walleij; Mark Brown; LKML; David Dajun Chen; Lee Jones
> Subject: Re: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver
> On Fri, Apr 19, 2013 at 6:56 PM, Anthony Olech
> wrote:
> > This patch is relative to next-20130419 of linux-next
> >
> > This is the GPIO component driver of the Dialog DA9058 PMIC.
> > This driver is just one component of the whole DA9058 PMIC driver.
> > It depends on the CORE component driver of the DA9058 MFD.
> > The meaning of the PMIC register 21 bits 1 and 5 has been documented
> > in the driver source.
> >
> > Changes relative to V5 of this patch:
> > - rebased to next-20130419 in
> > git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > - removed redundant #include
> > - corrected dates on copyright statements
> >
> > Signed-off-by: Anthony Olech
> > Signed-off-by: David Dajun Chen
>
> Sorry for slow review ...
>
> > +struct da9058_gpio {
> > + struct da9058 *da9058;
> > + struct platform_device *pdev;
> > + struct gpio_chip gp;
> > + struct mutex lock;
> > + int irq_0;
> > + int irq_1;
>
> Why are you keeping thes two numbers here? Use the irqdomain to contain irq
> mappings,
this module does not use the irqdomain directly so it gets the numbers from
the platform data, which I thought did not persist past the probe() function.
Is the platform data permanently available? - if that is the case, I can of
course
re-get the values on demand from the platform data - please confirm or suggest
an alternative method of extracting the irq number.
> this is just confusion things. (See review commens further below.)
>
> > + u8 inp_config;
> > + u8 out_config;
>
> > +static int da9058_gpio_get(struct gpio_chip *gc, unsigned offset) {
> > + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> > + struct da9058 *da9058 = gpio->da9058;
> > + unsigned int gpio_level;
> > + int ret;
> > +
> > + if (offset > 1)
> > + return -EINVAL;
>
> So we have only two pins, OK that's said in Kconfig too...
>
> (...)
> > +static void da9058_gpio_set(struct gpio_chip *gc, unsigned offset,
> > +int value) {
> > + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> > + struct da9058 *da9058 = gpio->da9058;
> > + unsigned int gpio_cntrl;
> > + int ret;
> > +
> > + if (offset > 1) {
> > + dev_err(da9058->dev,
> > + "Failed to set GPIO%d output=%d because illegal
> > GPIO\n",
> > + offset, value);
> > + return;
> > + }
>
> Here you have an error print, pls insert that on the get function too then.
the error print was used here because there is no other way to report an error.
On the get function a negative number can be returned, so therefore there is
no need to report it via the error log. I can put more diagnostics back in, but
is it really necessary to duplicate the error indication in some cases?
> (...)
> > + if (offset) {
> > +
>
> I would put a comment here like
> /* pin 1 clause */
yes I will add such comments
> Maybe it's more logical to have if (!offset) and handle pin
> 0 first, then pin 1 in the else clause?
why is it logical for one state to be preferred over the other?
> > + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG,
> _cntrl);
> > + if (ret)
> > + goto exit;
> > + gpio->out_config &= ~0x80;
> > + gpio->out_config |= value ? 0x80 : 0x00;
> > +
> > + if (!(gpio_cntrl & 0x20)) /* an INP pin */
> > + goto exit;
>
> Please use #define to define these magic bits.
> Something like this:
>
> #include
>
> #define DA9058_GPIO0_VAL BIT(3)
> #define DA9058_GPIO0_INOUT BIT(1)
> #define DA9058_GPIO1_VAL BIT(7)
> #define DA9058_GPIO1_INOUT BIT(5)
>
> (etc, so we see what all bits are for)
>
> then use these defines in the code...
the meaning of the bits other than the INP/OUT bits are not independent,
but depend on the direction. Thus each nibble contains 3 dependent bits.
>
> > +
> > + gpio_cntrl &= ~0xF0;
> > + gpio_cntrl |= 0xF0 & gpio->out_config;
> > +
> > +
RE: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver
-Original Message-
From: Linus Walleij [mailto:linus.wall...@linaro.org]
Sent: 24 April 2013 14:15
To: Opensource [Anthony Olech]
Cc: Grant Likely; Linus Walleij; Mark Brown; LKML; David Dajun Chen; Lee Jones
Subject: Re: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver
On Fri, Apr 19, 2013 at 6:56 PM, Anthony Olech
anthony.olech.opensou...@diasemi.com wrote:
This patch is relative to next-20130419 of linux-next
This is the GPIO component driver of the Dialog DA9058 PMIC.
This driver is just one component of the whole DA9058 PMIC driver.
It depends on the CORE component driver of the DA9058 MFD.
The meaning of the PMIC register 21 bits 1 and 5 has been documented
in the driver source.
Changes relative to V5 of this patch:
- rebased to next-20130419 in
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
- removed redundant #include linux/mfd/da9058/version.h
- corrected dates on copyright statements
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
Signed-off-by: David Dajun Chen david.c...@diasemi.com
Sorry for slow review ...
+struct da9058_gpio {
+ struct da9058 *da9058;
+ struct platform_device *pdev;
+ struct gpio_chip gp;
+ struct mutex lock;
+ int irq_0;
+ int irq_1;
Why are you keeping thes two numbers here? Use the irqdomain to contain irq
mappings,
this module does not use the irqdomain directly so it gets the numbers from
the platform data, which I thought did not persist past the probe() function.
Is the platform data permanently available? - if that is the case, I can of
course
re-get the values on demand from the platform data - please confirm or suggest
an alternative method of extracting the irq number.
this is just confusion things. (See review commens further below.)
+ u8 inp_config;
+ u8 out_config;
+static int da9058_gpio_get(struct gpio_chip *gc, unsigned offset) {
+ struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
+ struct da9058 *da9058 = gpio-da9058;
+ unsigned int gpio_level;
+ int ret;
+
+ if (offset 1)
+ return -EINVAL;
So we have only two pins, OK that's said in Kconfig too...
(...)
+static void da9058_gpio_set(struct gpio_chip *gc, unsigned offset,
+int value) {
+ struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
+ struct da9058 *da9058 = gpio-da9058;
+ unsigned int gpio_cntrl;
+ int ret;
+
+ if (offset 1) {
+ dev_err(da9058-dev,
+ Failed to set GPIO%d output=%d because illegal
GPIO\n,
+ offset, value);
+ return;
+ }
Here you have an error print, pls insert that on the get function too then.
the error print was used here because there is no other way to report an error.
On the get function a negative number can be returned, so therefore there is
no need to report it via the error log. I can put more diagnostics back in, but
is it really necessary to duplicate the error indication in some cases?
(...)
+ if (offset) {
+
I would put a comment here like
/* pin 1 clause */
yes I will add such comments
Maybe it's more logical to have if (!offset) and handle pin
0 first, then pin 1 in the else clause?
why is it logical for one state to be preferred over the other?
+ ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG,
gpio_cntrl);
+ if (ret)
+ goto exit;
+ gpio-out_config = ~0x80;
+ gpio-out_config |= value ? 0x80 : 0x00;
+
+ if (!(gpio_cntrl 0x20)) /* an INP pin */
+ goto exit;
Please use #define to define these magic bits.
Something like this:
#include linux/bitops.h
#define DA9058_GPIO0_VAL BIT(3)
#define DA9058_GPIO0_INOUT BIT(1)
#define DA9058_GPIO1_VAL BIT(7)
#define DA9058_GPIO1_INOUT BIT(5)
(etc, so we see what all bits are for)
then use these defines in the code...
the meaning of the bits other than the INP/OUT bits are not independent,
but depend on the direction. Thus each nibble contains 3 dependent bits.
+
+ gpio_cntrl = ~0xF0;
+ gpio_cntrl |= 0xF0 gpio-out_config;
+
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG,
gpio_cntrl);
+ } else {
+
I would put a comment here like
/* pin 0 clause */
yes I will add such comments
+ ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG,
gpio_cntrl);
+ if (ret)
+ goto exit;
+ gpio-out_config = ~0x08;
+ gpio-out_config |= value ? 0x08 : 0x00;
+
+ if (!(gpio_cntrl 0x02)) /* an INP pin */
+ goto exit;
Magic bits, see above.
see end of e-mail
RE: [NEW DRIVER V6 1/7] drivers/mfd: DA9058 MFD core driver
> -Original Message-
> From: Mark Brown [mailto:broo...@kernel.org]
> Sent: 22 April 2013 14:34
> To: Opensource [Anthony Olech]
> Cc: Samuel Ortiz; Arnd Bergmann; Mauro Carvalho Chehab; Steven Toth;
> Michael Krufky; LKML; David Dajun Chen
> Subject: Re: [NEW DRIVER V6 1/7] drivers/mfd: DA9058 MFD core driver
>
> On Fri, Apr 19, 2013 at 05:56:24PM +0100, Anthony Olech wrote:
>
> > +static struct da9058_regulator_pdata buck1_pdata = {
> > + .init.constraints.name = "DA9058_BUCK1",
> > + .regulator_id = DA9058_BUCK_1,
> > + .init.constraints.min_uV = DA9058_BUCK12_VOLT_LOWER * 1000,
> > + .init.constraints.max_uV = DA9058_BUCK12_VOLT_UPPER * 1000,
> > + .control_voltage_step = DA9058_BUCK_VOLT_STEP * 1000,
> > + .control_register = DA9058_BUCK1_REG,
> > + .control_step_mask = DA9058_MAX_VSEL,
> > + .control_enable_mask = DA9058_BUCK_LDO_EN,
> > + .ramp_register = DA9058_SUPPLY_REG,
> > + .ramp_enable_mask = DA9058_SUPPLY_VBUCK1GO,
> > + .init.constraints.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE |
> > + REGULATOR_CHANGE_MODE |
> REGULATOR_CHANGE_STATUS,
> > + .init.constraints.valid_modes_mask = REGULATOR_MODE_NORMAL,
> > + .init.constraints.boot_on = 1,
>
> No, you're totally missing the point here. Your regulator driver needs
> to be generic and work on any machine using the device. This means you
> can't specify any constraints, the constraints need to come from the
> system integration.
>
[...]
thanks Mark for your helpful comments.
As far as I can see there are two types of constraints:-
1) the absolute max/min that the PMIC itself is designed for.
2) the max/min that the board is designed for, ie system integration.
Thus constraints type (1) must come from the driver and constraints type (2)
must come from the machine driver, which for my testing is the mach-smdk6410.
Also, I believe that two further principles apply:
A) the machine driver should only specify a constraint if it restricts a value
from (1) above.
B) sensible defaults should be specified in the driver so that a system should
have some
chance of operating in the case that the machine driver provides no
constraints/overrides.
Thus, in the case of this PMIC, the big BUCK is designed to provide power to
the processor,
so the only possible sensible default can be "boot_on == 1". It then makes
sense that if a
PMIC is to be used in the situation it is designed for, then the number of
system integration
constraints should be almost zero.
>From your comment it seems that I have missed a point, but what?
Tony Olech
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V6 1/7] drivers/mfd: DA9058 MFD core driver
-Original Message-
From: Mark Brown [mailto:broo...@kernel.org]
Sent: 22 April 2013 14:34
To: Opensource [Anthony Olech]
Cc: Samuel Ortiz; Arnd Bergmann; Mauro Carvalho Chehab; Steven Toth;
Michael Krufky; LKML; David Dajun Chen
Subject: Re: [NEW DRIVER V6 1/7] drivers/mfd: DA9058 MFD core driver
On Fri, Apr 19, 2013 at 05:56:24PM +0100, Anthony Olech wrote:
+static struct da9058_regulator_pdata buck1_pdata = {
+ .init.constraints.name = DA9058_BUCK1,
+ .regulator_id = DA9058_BUCK_1,
+ .init.constraints.min_uV = DA9058_BUCK12_VOLT_LOWER * 1000,
+ .init.constraints.max_uV = DA9058_BUCK12_VOLT_UPPER * 1000,
+ .control_voltage_step = DA9058_BUCK_VOLT_STEP * 1000,
+ .control_register = DA9058_BUCK1_REG,
+ .control_step_mask = DA9058_MAX_VSEL,
+ .control_enable_mask = DA9058_BUCK_LDO_EN,
+ .ramp_register = DA9058_SUPPLY_REG,
+ .ramp_enable_mask = DA9058_SUPPLY_VBUCK1GO,
+ .init.constraints.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE |
+ REGULATOR_CHANGE_MODE |
REGULATOR_CHANGE_STATUS,
+ .init.constraints.valid_modes_mask = REGULATOR_MODE_NORMAL,
+ .init.constraints.boot_on = 1,
No, you're totally missing the point here. Your regulator driver needs
to be generic and work on any machine using the device. This means you
can't specify any constraints, the constraints need to come from the
system integration.
[...]
thanks Mark for your helpful comments.
As far as I can see there are two types of constraints:-
1) the absolute max/min that the PMIC itself is designed for.
2) the max/min that the board is designed for, ie system integration.
Thus constraints type (1) must come from the driver and constraints type (2)
must come from the machine driver, which for my testing is the mach-smdk6410.
Also, I believe that two further principles apply:
A) the machine driver should only specify a constraint if it restricts a value
from (1) above.
B) sensible defaults should be specified in the driver so that a system should
have some
chance of operating in the case that the machine driver provides no
constraints/overrides.
Thus, in the case of this PMIC, the big BUCK is designed to provide power to
the processor,
so the only possible sensible default can be boot_on == 1. It then makes
sense that if a
PMIC is to be used in the situation it is designed for, then the number of
system integration
constraints should be almost zero.
From your comment it seems that I have missed a point, but what?
Tony Olech
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V6 6/7] drivers/hwmon: DA9058 HWMON driver
> -Original Message- > From: Guenter Roeck [mailto:li...@roeck-us.net] > Sent: 20 April 2013 18:35 > To: Lars-Peter Clausen > Cc: Opensource [Anthony Olech]; Jean Delvare; Mark Brown; Randy Dunlap; > lm-sens...@lm-sensors.org; LKML; David Dajun Chen > Subject: Re: [NEW DRIVER V6 6/7] drivers/hwmon: DA9058 HWMON driver > On Fri, Apr 19, 2013 at 08:25:02PM +0200, Lars-Peter Clausen wrote: > > Same comment as before, I'd like to see this using the generic IIO to > > HWMON bridge instead of recreating it. > ... and I agree. Seems we are getting more and more of those, and at some > point it makes really sense to find a generic solution. > Anthony, can you please look into that ? > Thanks, > Guenter OK -- generic IIO to HWMON bridge I will investigate - but if you know any prototypes or any useful starting point could you let me know as I don't want to re-invent the wheel. Thanks, Anthony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V6 6/7] drivers/hwmon: DA9058 HWMON driver
-Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 20 April 2013 18:35 To: Lars-Peter Clausen Cc: Opensource [Anthony Olech]; Jean Delvare; Mark Brown; Randy Dunlap; lm-sens...@lm-sensors.org; LKML; David Dajun Chen Subject: Re: [NEW DRIVER V6 6/7] drivers/hwmon: DA9058 HWMON driver On Fri, Apr 19, 2013 at 08:25:02PM +0200, Lars-Peter Clausen wrote: Same comment as before, I'd like to see this using the generic IIO to HWMON bridge instead of recreating it. ... and I agree. Seems we are getting more and more of those, and at some point it makes really sense to find a generic solution. Anthony, can you please look into that ? Thanks, Guenter OK -- generic IIO to HWMON bridge I will investigate - but if you know any prototypes or any useful starting point could you let me know as I don't want to re-invent the wheel. Thanks, Anthony Olech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: CodingStyle
> -Original Message-
> From: Guenter Roeck [mailto:li...@roeck-us.net]
> Sent: 18 April 2013 14:42
> To: Opensource [Anthony Olech]
> Cc: LKML
> Subject: Re: CodingStyle
>
> On Thu, Apr 18, 2013 at 10:48:06AM +0000, Opensource [Anthony Olech]
> wrote:
> > > -Original Message-
> > > From: Guenter Roeck [mailto:li...@roeck-us.net]
> > > Sent: 18 April 2013 05:14
> > > To: Opensource [Anthony Olech]
> > > Cc: Jean Delvare; Mark Brown; Randy Dunlap;
> > > lm-sens...@lm-sensors.org; LKML; David Dajun Chen
> > > Subject: Re: [NEW DRIVER V5 6/7] drivers/hwmon: DA9058 HWMON driver
> > >
> > [...]
> > > > + if (cell == NULL) {
> > > > + ret = -ENODEV;
> > > > + goto exit;
> > >
> > > Just return -ENODEV is good enough here. See CodingStyle, chapter 7.
> > >
> > [...]
> > > > +exit:
> > > > + return ret;
> > > > +}
> > [...]
> >
> > Hi Guenter,
> >
> > I have read CodingStyle, chapter 7 and it seems to encourage a single exit
> point from functions.
> >
> Yes, unless you can return directly. There is even an example.
> It actually says "... and some common work such as cleanup has to be done".
OK, thank you for clarifying that. I will make the necessary changes.
> > During development there was some logging done at the (single) exit point
> but that has been stripped out for submission.
> > Whilst I can duplicate the logging and do an immediate 'return' at all those
> points, I am hesitant to do so when chapter 7 of the CodingStyle appears to
> say
> to use a single exit point. In addition I had thought that a single exit
> point from
> functions was a good thing.
> >
> Not if you add a goto to a return statement. That defeats the purpose of easy
> readability.
> Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
CodingStyle
> -Original Message-
> From: Guenter Roeck [mailto:li...@roeck-us.net]
> Sent: 18 April 2013 05:14
> To: Opensource [Anthony Olech]
> Cc: Jean Delvare; Mark Brown; Randy Dunlap; lm-sens...@lm-sensors.org;
> LKML; David Dajun Chen
> Subject: Re: [NEW DRIVER V5 6/7] drivers/hwmon: DA9058 HWMON driver
>
[...]
> > + if (cell == NULL) {
> > + ret = -ENODEV;
> > + goto exit;
>
> Just return -ENODEV is good enough here. See CodingStyle, chapter 7.
>
[...]
> > +exit:
> > + return ret;
> > +}
[...]
Hi Guenter,
I have read CodingStyle, chapter 7 and it seems to encourage a single exit
point from functions.
During development there was some logging done at the (single) exit point but
that has been stripped out for submission.
Whilst I can duplicate the logging and do an immediate 'return' at all those
points, I am hesitant to do so when chapter 7 of the CodingStyle appears to say
to use a single exit point. In addition I had thought that a single exit point
from functions was a good thing.
Tony Olech
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
CodingStyle
-Original Message-
From: Guenter Roeck [mailto:li...@roeck-us.net]
Sent: 18 April 2013 05:14
To: Opensource [Anthony Olech]
Cc: Jean Delvare; Mark Brown; Randy Dunlap; lm-sens...@lm-sensors.org;
LKML; David Dajun Chen
Subject: Re: [NEW DRIVER V5 6/7] drivers/hwmon: DA9058 HWMON driver
[...]
+ if (cell == NULL) {
+ ret = -ENODEV;
+ goto exit;
Just return -ENODEV is good enough here. See CodingStyle, chapter 7.
[...]
+exit:
+ return ret;
+}
[...]
Hi Guenter,
I have read CodingStyle, chapter 7 and it seems to encourage a single exit
point from functions.
During development there was some logging done at the (single) exit point but
that has been stripped out for submission.
Whilst I can duplicate the logging and do an immediate 'return' at all those
points, I am hesitant to do so when chapter 7 of the CodingStyle appears to say
to use a single exit point. In addition I had thought that a single exit point
from functions was a good thing.
Tony Olech
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: CodingStyle
-Original Message-
From: Guenter Roeck [mailto:li...@roeck-us.net]
Sent: 18 April 2013 14:42
To: Opensource [Anthony Olech]
Cc: LKML
Subject: Re: CodingStyle
On Thu, Apr 18, 2013 at 10:48:06AM +, Opensource [Anthony Olech]
wrote:
-Original Message-
From: Guenter Roeck [mailto:li...@roeck-us.net]
Sent: 18 April 2013 05:14
To: Opensource [Anthony Olech]
Cc: Jean Delvare; Mark Brown; Randy Dunlap;
lm-sens...@lm-sensors.org; LKML; David Dajun Chen
Subject: Re: [NEW DRIVER V5 6/7] drivers/hwmon: DA9058 HWMON driver
[...]
+ if (cell == NULL) {
+ ret = -ENODEV;
+ goto exit;
Just return -ENODEV is good enough here. See CodingStyle, chapter 7.
[...]
+exit:
+ return ret;
+}
[...]
Hi Guenter,
I have read CodingStyle, chapter 7 and it seems to encourage a single exit
point from functions.
Yes, unless you can return directly. There is even an example.
It actually says ... and some common work such as cleanup has to be done.
OK, thank you for clarifying that. I will make the necessary changes.
During development there was some logging done at the (single) exit point
but that has been stripped out for submission.
Whilst I can duplicate the logging and do an immediate 'return' at all those
points, I am hesitant to do so when chapter 7 of the CodingStyle appears to
say
to use a single exit point. In addition I had thought that a single exit
point from
functions was a good thing.
Not if you add a goto to a return statement. That defeats the purpose of easy
readability.
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 2/7] DA9058 ADC driver
> [...] > please always test your drivers against the latest upstream version before > submitting them. > [...] Hi Lars, The driver was tested against linux mainline tag v3.9-rc6, because that was the most recent tagged commit in git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git on the day that I sent in the patches. What exactly do you mean by "latest upstream version" ? Did I use the correct repository ? Tony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver
> -Original Message- > From: Guenter Roeck [mailto:li...@roeck-us.net] > Sent: 16 April 2013 14:36 > To: Opensource [Anthony Olech] > Cc: LKML > Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver > > On Tue, Apr 16, 2013 at 09:17:27AM +0000, Opensource [Anthony Olech] > wrote: > > > -Original Message- > > > From: Guenter Roeck [mailto:li...@roeck-us.net] > > > Sent: 15 April 2013 18:46 > > > To: Opensource [Anthony Olech] > > > Cc: LKML > > > Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver > > > > > > On Mon, Apr 15, 2013 at 05:29:13PM +, Opensource [Anthony Olech] > > > wrote: > > > > > -Original Message----- > > > > > From: Guenter Roeck [mailto:li...@roeck-us.net] > > > > > Sent: 15 April 2013 17:36 > > > > > To: Opensource [Anthony Olech] > > > > > Cc: LKML > > > > > Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver > > > > > > > > > > On Mon, Apr 15, 2013 at 03:00:58PM +, Opensource [Anthony > > > > > Olech] > > > > > wrote: > > > > > > > > > > > > > > > > > > > -Original Message- > > > > > > > From: Guenter Roeck [mailto:li...@roeck-us.net] > > > > > > > Sent: 12 April 2013 14:32 > > > > > > > To: Opensource [Anthony Olech] > > > > > > > Cc: Mark Brown; Liam Girdwood; Jean Delvare; Randy Dunlap; > > > > > > > LKML; David Dajun Chen > > > > > > > Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver > > > > > > > > > > > > > > On Fri, Apr 12, 2013 at 02:05:28PM +0100, Anthony Olech wrote: > > > > > > > > This is the REGULATOR component driver of the Dialog DA9058 > PMIC. > > > > > > > > This driver is just one component of the whole DA9058 PMIC > driver. > > > > > > > > It depends on the CORE component driver of the DA9058 MFD. > > > > > > > > > > > > > > > > There are 6 warnings from scripts/checkpatch.pl, but since > > > > > > > > it seems to be complaining about variable names such as > > > > > > > > min_uV are in CamelCase, when it is obvious that they are > > > > > > > > not in CamelCase I have > > > > > ignored them. > > > > > > > > > > > > > > > ??? min_uV _is_ CamelCase ??? > > > > > > > > > > > > > > Ok, maybe it is camelcasE, but you are splitting hairs here. > > > > > > > > > > > > it is not me splitting hairs, it is scripts/checkpatch.pl > > > > > > > > > > > Maybe you did not understand what I meant. Per your logic, > > > > > > > > > > MicroVolt is CamelCase > > > > > uVolt is ??? > > > > > uV is not CamelCase > > > > > > > > > > By abbreviating CamelCase to camelCase to cC you make it, in > > > > > your opinion, acceptable. > > > > > > > > > > If you want to declare CamelCase variables, just do it, but > > > > > don't claim that they are not really CamelCase. > > > > > > > > > > Guenter > > > > > > > > I always thought that camel case meant "changing from lower case > > > > to upper case the first letter of each word and then joining the > > > > capitalized words together", so by that definition uV or mW are > > > > not camel > > > case because "v" and "w" are not words! > > > > The definition of CamelCase From Wikipedia, the free encyclopedia is: > > > > "CamelCase (camel case) is a term which refers to the practice of > > writing compound words where the first letter of an identifier is > > lowercase and the first letter of each subsequent concatenated word is > capitalized." > > > Maybe the rule should read "don't mix lowercase and uppercase letters in > variable names and defines" to prevent variable names such as cAmelcAse or > cameLcasE, which would be permitted with your logic :). It is really good to have a definition that anyone can work with! > > > > Either way it seems that the algorithm in scripts/checkpatch.pl is > > > > wrong! and > > > that was my point. > > > > > > Guess we'll have to agree to disagree here, as I happen to think > > > that checkpatch is perfectly right. > > > Guenter > > > > Hi Guenter, > > > > I am quite happy to accept the algorithm in scripts/checkpatch.pl as > > the arbiter for correctly formed linux kernel variable names. > > > > On that basis "old_mV", "new_uA" etc are incorrectly formed variable names. > > Could you possibly suggest legal alternatives to "mA", "uV", "kW" ?? > > > I just changed it to lowercase in the ntc_thermistor driver. What you use is > really your call as long as it does not mix uppercase and lowercase letters. > > Guenter many thanks, I will do the same. Tony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 3/7] DA9058 ONKEY driver
> -Original Message-
> From: Randy Dunlap [mailto:rdun...@infradead.org]
> Sent: 12 April 2013 21:02
> To: Opensource [Anthony Olech]
> Cc: Dmitry Torokhov; Mark Brown; Samuel Ortiz; Ashish Jangam; Eric
> Andersson; Andrew Jones; linux-in...@vger.kernel.org; LKML; David Dajun Chen
> Subject: Re: [NEW DRIVER V4 3/7] DA9058 ONKEY driver
>
> On 04/12/13 06:05, Anthony Olech wrote:
> > This is the ONKEY component driver of the Dialog DA9058 PMIC.
> > This driver is just one component of the whole DA9058 PMIC driver.
> > It depends on the CORE component driver of the DA9058 MFD.
> >
> > Signed-off-by: Anthony Olech
> > Signed-off-by: David Dajun Chen
> > ---
> > drivers/input/misc/Kconfig| 10 +++
> > drivers/input/misc/Makefile |1 +
> > drivers/input/misc/da9058_onkey.c | 177
> > +
> > 3 files changed, 188 insertions(+)
> > create mode 100644 drivers/input/misc/da9058_onkey.c
> >
> > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> > index 259ef31..bd07b38 100644
> > --- a/drivers/input/misc/Kconfig
> > +++ b/drivers/input/misc/Kconfig
> > @@ -93,6 +93,16 @@ config INPUT_BMA150
> > To compile this driver as a module, choose M here: the
> > module will be called bma150.
> >
> > +config INPUT_DA9058_ONKEY
> > + tristate "DA9058 ONKEY support"
> > + depends on MFD_DA9058
> > + help
> > + Support the ONKEY of DA9058 PMICs as an input device
> > + reporting power button status.
>
> What possible values can a power button status have?
> Must be more than my KISS guess:
> this software is running => ON
> software not running => OFF
> eh?
Pressing the button briefly and pressing and holding the button will have
different effects in a mobile device. The press and hold on phones normally
switches them into a sleep state. So the "power button status" is the fact
that the ONKEY is still being held down.
Does that answer your question? or have I missed your point??
[...]
> > + onkey->irq = platform_get_irq(pdev, 0);
> > + if (onkey->irq < 0) {
> > + dev_err(>dev, "can not get ONKEY IRQ error=%d\n",
>
>cannot
The Washington State University language site says:
"These two spellings [cannot/can not] are largely interchangeable, but by far
the most common is 'cannot' and you should probably use it except when you
want to be emphatic: 'No, you can not wash the dog in the Maytag.'"
Since I was not trying to be particularly emphatic, I will change to using
'cannot'
as per your suggestion.
Tony Olech
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver
> -Original Message-
> From: Mark Brown [mailto:broo...@kernel.org]
> Sent: 12 April 2013 14:27
> To: Opensource [Anthony Olech]
> Cc: Liam Girdwood; Guenter Roeck; Jean Delvare; Randy Dunlap; LKML; David
> Dajun Chen
> Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver
> On Fri, Apr 12, 2013 at 02:05:28PM +0100, Anthony Olech wrote:
> This looks good, I assume there's some dependencies on the MFD or other
> earlier patches so I won't apply it, let me know if there aren't any and I
> will
I will resubmit later after re-testing, fixing the 3 points you raised in this
driver.
> Acked-by: Mark Brown
> Please use subject lines reflecting the subsystem.
I assume you mean "regulator" for drivers in "drivers/regulator",
"misc" for drivers in "drivers/input/misc" etc etc ??
> > +static int da9058_buck_ramp_voltage(struct regulator_dev *rdev,
> > + unsigned int old_selector,
> > + unsigned int new_selector)
> > +{
> > + struct da9058_regulator *regulator = rdev_get_drvdata(rdev);
> > + struct da9058 *da9058 = regulator->da9058;
> > + int ret;
> > +
> > + if (regulator->ramp_register == 0)
> > + return -EINVAL;
> > +
> > + if (regulator->ramp_enable_mask == 0)
> > + return -EINVAL;
> > +
> > + ret = da9058_set_bits(da9058, regulator->ramp_register,
> > + regulator->ramp_enable_mask);
> > +
> > + if (ret)
> > + return ret;
> > +
> > + return 2200; /* micro Seconds needed to ramp to new voltage*/ }
>
> Hrm, this really should be implementable with a generic regmap operation...
Yes, I did not notice that the generic regmap operation
"reulator_set_voltage_sel_regmap()"
supports a trigger/go. I will change to using it.
> > + rdev = regulator_register(>desc, );
> > +
> > + if (IS_ERR(rdev)) {
> > + dev_err(>dev, "failed to register %s\n",
> > + rpdata->regulator_name);
> > + ret = PTR_ERR(rdev);
> > + goto failed_to_register;
> > + }
>
> In general it's a bit better style to print out the return value to help with
> diagnosis but it's no big deal.
Yes, I will include the value of "ret" in the dev_err message
Tony Olech
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 0/7] DA9058 PMIC - please comment on this new driver
> -Original Message- > From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com] > Sent: 12 April 2013 19:07 > To: Opensource [Anthony Olech] > Cc: LKML; Alessandro Zummo > Subject: Re: [NEW DRIVER V4 0/7] DA9058 PMIC - please comment on this new > driver > On Friday, April 12, 2013 02:05:29 PM Anthony Olech wrote: > > This is submission attempt number 4 to have this driver included in > > the linux kernel source tree. This is the driver for the Dialog DA9058. > > The DA9058 is a low power Power Management Integrated Circuit with > > extra functionality. It is a Multi Function Device controlled only > > from an I2C bus whose components can raise an interrupt request on a single > > IRQ line. > > The driver for the DA9058 consists of a core (i2c) device driver that > > instantiates the individual component device drivers for: > > adc - 5 ADC channels > > gpio - 2 available pins > > onkey - 1 device > > > This is almost exact copy of da9052_onkey, can they be merged together? > > Dmitry Hi Dmitry, it does look like the ONKEY component driver of the Dialog DA9058 PMIC is functionally similar to other onkey drivers. Specifically those drivers that have to poll for ONKEY de-assertion. The reason why they are separate is that they are probed with different data structures and they use a different wrapper to access the "regmap" API. However, that is not a blocking issue, but it would mean re-writing the core MFD component of all the affected drivers. Maybe the way forward is: Is it possible to provide some "core" ONKEY API or functionality to handle the commonality in a similar fashion to the regulator core as done by Mark Brown? Tony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver
> -Original Message- > From: Guenter Roeck [mailto:li...@roeck-us.net] > Sent: 15 April 2013 18:46 > To: Opensource [Anthony Olech] > Cc: LKML > Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver > > On Mon, Apr 15, 2013 at 05:29:13PM +0000, Opensource [Anthony Olech] > wrote: > > > -Original Message- > > > From: Guenter Roeck [mailto:li...@roeck-us.net] > > > Sent: 15 April 2013 17:36 > > > To: Opensource [Anthony Olech] > > > Cc: LKML > > > Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver > > > > > > On Mon, Apr 15, 2013 at 03:00:58PM +, Opensource [Anthony Olech] > > > wrote: > > > > > > > > > > > > > -Original Message- > > > > > From: Guenter Roeck [mailto:li...@roeck-us.net] > > > > > Sent: 12 April 2013 14:32 > > > > > To: Opensource [Anthony Olech] > > > > > Cc: Mark Brown; Liam Girdwood; Jean Delvare; Randy Dunlap; LKML; > > > > > David Dajun Chen > > > > > Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver > > > > > > > > > > On Fri, Apr 12, 2013 at 02:05:28PM +0100, Anthony Olech wrote: > > > > > > This is the REGULATOR component driver of the Dialog DA9058 PMIC. > > > > > > This driver is just one component of the whole DA9058 PMIC driver. > > > > > > It depends on the CORE component driver of the DA9058 MFD. > > > > > > > > > > > > There are 6 warnings from scripts/checkpatch.pl, but since it > > > > > > seems to be complaining about variable names such as min_uV > > > > > > are in CamelCase, when it is obvious that they are not in > > > > > > CamelCase I have > > > ignored them. > > > > > > > > > > > ??? min_uV _is_ CamelCase ??? > > > > > > > > > > Ok, maybe it is camelcasE, but you are splitting hairs here. > > > > > > > > it is not me splitting hairs, it is scripts/checkpatch.pl > > > > > > > Maybe you did not understand what I meant. Per your logic, > > > > > > MicroVolt is CamelCase > > > uVolt is ??? > > > uV is not CamelCase > > > > > > By abbreviating CamelCase to camelCase to cC you make it, in your > > > opinion, acceptable. > > > > > > If you want to declare CamelCase variables, just do it, but don't > > > claim that they are not really CamelCase. > > > > > > Guenter > > > > I always thought that camel case meant "changing from lower case to > > upper case the first letter of each word and then joining the > > capitalized words together", so by that definition uV or mW are not camel > case because "v" and "w" are not words! The definition of CamelCase From Wikipedia, the free encyclopedia is: "CamelCase (camel case) is a term which refers to the practice of writing compound words where the first letter of an identifier is lowercase and the first letter of each subsequent concatenated word is capitalized." > > Either way it seems that the algorithm in scripts/checkpatch.pl is wrong! > > and > that was my point. > > Guess we'll have to agree to disagree here, as I happen to think that > checkpatch is perfectly right. > Guenter Hi Guenter, I am quite happy to accept the algorithm in scripts/checkpatch.pl as the arbiter for correctly formed linux kernel variable names. On that basis "old_mV", "new_uA" etc are incorrectly formed variable names. Could you possibly suggest legal alternatives to "mA", "uV", "kW" ?? Tony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver
-Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 15 April 2013 18:46 To: Opensource [Anthony Olech] Cc: LKML Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver On Mon, Apr 15, 2013 at 05:29:13PM +, Opensource [Anthony Olech] wrote: -Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 15 April 2013 17:36 To: Opensource [Anthony Olech] Cc: LKML Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver On Mon, Apr 15, 2013 at 03:00:58PM +, Opensource [Anthony Olech] wrote: -Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 12 April 2013 14:32 To: Opensource [Anthony Olech] Cc: Mark Brown; Liam Girdwood; Jean Delvare; Randy Dunlap; LKML; David Dajun Chen Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver On Fri, Apr 12, 2013 at 02:05:28PM +0100, Anthony Olech wrote: This is the REGULATOR component driver of the Dialog DA9058 PMIC. This driver is just one component of the whole DA9058 PMIC driver. It depends on the CORE component driver of the DA9058 MFD. There are 6 warnings from scripts/checkpatch.pl, but since it seems to be complaining about variable names such as min_uV are in CamelCase, when it is obvious that they are not in CamelCase I have ignored them. ??? min_uV _is_ CamelCase ??? Ok, maybe it is camelcasE, but you are splitting hairs here. it is not me splitting hairs, it is scripts/checkpatch.pl Maybe you did not understand what I meant. Per your logic, MicroVolt is CamelCase uVolt is ??? uV is not CamelCase By abbreviating CamelCase to camelCase to cC you make it, in your opinion, acceptable. If you want to declare CamelCase variables, just do it, but don't claim that they are not really CamelCase. Guenter I always thought that camel case meant changing from lower case to upper case the first letter of each word and then joining the capitalized words together, so by that definition uV or mW are not camel case because v and w are not words! The definition of CamelCase From Wikipedia, the free encyclopedia is: CamelCase (camel case) is a term which refers to the practice of writing compound words where the first letter of an identifier is lowercase and the first letter of each subsequent concatenated word is capitalized. Either way it seems that the algorithm in scripts/checkpatch.pl is wrong! and that was my point. Guess we'll have to agree to disagree here, as I happen to think that checkpatch is perfectly right. Guenter Hi Guenter, I am quite happy to accept the algorithm in scripts/checkpatch.pl as the arbiter for correctly formed linux kernel variable names. On that basis old_mV, new_uA etc are incorrectly formed variable names. Could you possibly suggest legal alternatives to mA, uV, kW ?? Tony Olech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 0/7] DA9058 PMIC - please comment on this new driver
-Original Message- From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com] Sent: 12 April 2013 19:07 To: Opensource [Anthony Olech] Cc: LKML; Alessandro Zummo Subject: Re: [NEW DRIVER V4 0/7] DA9058 PMIC - please comment on this new driver On Friday, April 12, 2013 02:05:29 PM Anthony Olech wrote: This is submission attempt number 4 to have this driver included in the linux kernel source tree. This is the driver for the Dialog DA9058. The DA9058 is a low power Power Management Integrated Circuit with extra functionality. It is a Multi Function Device controlled only from an I2C bus whose components can raise an interrupt request on a single IRQ line. The driver for the DA9058 consists of a core (i2c) device driver that instantiates the individual component device drivers for: adc - 5 ADC channels gpio - 2 available pins onkey - 1 device This is almost exact copy of da9052_onkey, can they be merged together? Dmitry Hi Dmitry, it does look like the ONKEY component driver of the Dialog DA9058 PMIC is functionally similar to other onkey drivers. Specifically those drivers that have to poll for ONKEY de-assertion. The reason why they are separate is that they are probed with different data structures and they use a different wrapper to access the regmap API. However, that is not a blocking issue, but it would mean re-writing the core MFD component of all the affected drivers. Maybe the way forward is: Is it possible to provide some core ONKEY API or functionality to handle the commonality in a similar fashion to the regulator core as done by Mark Brown? Tony Olech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver
-Original Message-
From: Mark Brown [mailto:broo...@kernel.org]
Sent: 12 April 2013 14:27
To: Opensource [Anthony Olech]
Cc: Liam Girdwood; Guenter Roeck; Jean Delvare; Randy Dunlap; LKML; David
Dajun Chen
Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver
On Fri, Apr 12, 2013 at 02:05:28PM +0100, Anthony Olech wrote:
This looks good, I assume there's some dependencies on the MFD or other
earlier patches so I won't apply it, let me know if there aren't any and I
will
I will resubmit later after re-testing, fixing the 3 points you raised in this
driver.
Acked-by: Mark Brown broo...@opensource.wolfsonmicro.com
Please use subject lines reflecting the subsystem.
I assume you mean regulator for drivers in drivers/regulator,
misc for drivers in drivers/input/misc etc etc ??
+static int da9058_buck_ramp_voltage(struct regulator_dev *rdev,
+ unsigned int old_selector,
+ unsigned int new_selector)
+{
+ struct da9058_regulator *regulator = rdev_get_drvdata(rdev);
+ struct da9058 *da9058 = regulator-da9058;
+ int ret;
+
+ if (regulator-ramp_register == 0)
+ return -EINVAL;
+
+ if (regulator-ramp_enable_mask == 0)
+ return -EINVAL;
+
+ ret = da9058_set_bits(da9058, regulator-ramp_register,
+ regulator-ramp_enable_mask);
+
+ if (ret)
+ return ret;
+
+ return 2200; /* micro Seconds needed to ramp to new voltage*/ }
Hrm, this really should be implementable with a generic regmap operation...
Yes, I did not notice that the generic regmap operation
reulator_set_voltage_sel_regmap()
supports a trigger/go. I will change to using it.
+ rdev = regulator_register(reg-desc, config);
+
+ if (IS_ERR(rdev)) {
+ dev_err(pdev-dev, failed to register %s\n,
+ rpdata-regulator_name);
+ ret = PTR_ERR(rdev);
+ goto failed_to_register;
+ }
In general it's a bit better style to print out the return value to help with
diagnosis but it's no big deal.
Yes, I will include the value of ret in the dev_err message
Tony Olech
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 3/7] DA9058 ONKEY driver
-Original Message-
From: Randy Dunlap [mailto:rdun...@infradead.org]
Sent: 12 April 2013 21:02
To: Opensource [Anthony Olech]
Cc: Dmitry Torokhov; Mark Brown; Samuel Ortiz; Ashish Jangam; Eric
Andersson; Andrew Jones; linux-in...@vger.kernel.org; LKML; David Dajun Chen
Subject: Re: [NEW DRIVER V4 3/7] DA9058 ONKEY driver
On 04/12/13 06:05, Anthony Olech wrote:
This is the ONKEY component driver of the Dialog DA9058 PMIC.
This driver is just one component of the whole DA9058 PMIC driver.
It depends on the CORE component driver of the DA9058 MFD.
Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com
Signed-off-by: David Dajun Chen david.c...@diasemi.com
---
drivers/input/misc/Kconfig| 10 +++
drivers/input/misc/Makefile |1 +
drivers/input/misc/da9058_onkey.c | 177
+
3 files changed, 188 insertions(+)
create mode 100644 drivers/input/misc/da9058_onkey.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 259ef31..bd07b38 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -93,6 +93,16 @@ config INPUT_BMA150
To compile this driver as a module, choose M here: the
module will be called bma150.
+config INPUT_DA9058_ONKEY
+ tristate DA9058 ONKEY support
+ depends on MFD_DA9058
+ help
+ Support the ONKEY of DA9058 PMICs as an input device
+ reporting power button status.
What possible values can a power button status have?
Must be more than my KISS guess:
this software is running = ON
software not running = OFF
eh?
Pressing the button briefly and pressing and holding the button will have
different effects in a mobile device. The press and hold on phones normally
switches them into a sleep state. So the power button status is the fact
that the ONKEY is still being held down.
Does that answer your question? or have I missed your point??
[...]
+ onkey-irq = platform_get_irq(pdev, 0);
+ if (onkey-irq 0) {
+ dev_err(pdev-dev, can not get ONKEY IRQ error=%d\n,
cannot
The Washington State University language site says:
These two spellings [cannot/can not] are largely interchangeable, but by far
the most common is 'cannot' and you should probably use it except when you
want to be emphatic: 'No, you can not wash the dog in the Maytag.'
Since I was not trying to be particularly emphatic, I will change to using
'cannot'
as per your suggestion.
Tony Olech
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver
-Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 16 April 2013 14:36 To: Opensource [Anthony Olech] Cc: LKML Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver On Tue, Apr 16, 2013 at 09:17:27AM +, Opensource [Anthony Olech] wrote: -Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 15 April 2013 18:46 To: Opensource [Anthony Olech] Cc: LKML Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver On Mon, Apr 15, 2013 at 05:29:13PM +, Opensource [Anthony Olech] wrote: -Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 15 April 2013 17:36 To: Opensource [Anthony Olech] Cc: LKML Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver On Mon, Apr 15, 2013 at 03:00:58PM +, Opensource [Anthony Olech] wrote: -Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 12 April 2013 14:32 To: Opensource [Anthony Olech] Cc: Mark Brown; Liam Girdwood; Jean Delvare; Randy Dunlap; LKML; David Dajun Chen Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver On Fri, Apr 12, 2013 at 02:05:28PM +0100, Anthony Olech wrote: This is the REGULATOR component driver of the Dialog DA9058 PMIC. This driver is just one component of the whole DA9058 PMIC driver. It depends on the CORE component driver of the DA9058 MFD. There are 6 warnings from scripts/checkpatch.pl, but since it seems to be complaining about variable names such as min_uV are in CamelCase, when it is obvious that they are not in CamelCase I have ignored them. ??? min_uV _is_ CamelCase ??? Ok, maybe it is camelcasE, but you are splitting hairs here. it is not me splitting hairs, it is scripts/checkpatch.pl Maybe you did not understand what I meant. Per your logic, MicroVolt is CamelCase uVolt is ??? uV is not CamelCase By abbreviating CamelCase to camelCase to cC you make it, in your opinion, acceptable. If you want to declare CamelCase variables, just do it, but don't claim that they are not really CamelCase. Guenter I always thought that camel case meant changing from lower case to upper case the first letter of each word and then joining the capitalized words together, so by that definition uV or mW are not camel case because v and w are not words! The definition of CamelCase From Wikipedia, the free encyclopedia is: CamelCase (camel case) is a term which refers to the practice of writing compound words where the first letter of an identifier is lowercase and the first letter of each subsequent concatenated word is capitalized. Maybe the rule should read don't mix lowercase and uppercase letters in variable names and defines to prevent variable names such as cAmelcAse or cameLcasE, which would be permitted with your logic :). It is really good to have a definition that anyone can work with! Either way it seems that the algorithm in scripts/checkpatch.pl is wrong! and that was my point. Guess we'll have to agree to disagree here, as I happen to think that checkpatch is perfectly right. Guenter Hi Guenter, I am quite happy to accept the algorithm in scripts/checkpatch.pl as the arbiter for correctly formed linux kernel variable names. On that basis old_mV, new_uA etc are incorrectly formed variable names. Could you possibly suggest legal alternatives to mA, uV, kW ?? I just changed it to lowercase in the ntc_thermistor driver. What you use is really your call as long as it does not mix uppercase and lowercase letters. Guenter many thanks, I will do the same. Tony Olech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 2/7] DA9058 ADC driver
[...] please always test your drivers against the latest upstream version before submitting them. [...] Hi Lars, The driver was tested against linux mainline tag v3.9-rc6, because that was the most recent tagged commit in git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git on the day that I sent in the patches. What exactly do you mean by latest upstream version ? Did I use the correct repository ? Tony Olech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver
> -Original Message- > From: Guenter Roeck [mailto:li...@roeck-us.net] > Sent: 15 April 2013 17:36 > To: Opensource [Anthony Olech] > Cc: LKML > Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver > > On Mon, Apr 15, 2013 at 03:00:58PM +0000, Opensource [Anthony Olech] > wrote: > > > > > > > -Original Message- > > > From: Guenter Roeck [mailto:li...@roeck-us.net] > > > Sent: 12 April 2013 14:32 > > > To: Opensource [Anthony Olech] > > > Cc: Mark Brown; Liam Girdwood; Jean Delvare; Randy Dunlap; LKML; > > > David Dajun Chen > > > Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver > > > > > > On Fri, Apr 12, 2013 at 02:05:28PM +0100, Anthony Olech wrote: > > > > This is the REGULATOR component driver of the Dialog DA9058 PMIC. > > > > This driver is just one component of the whole DA9058 PMIC driver. > > > > It depends on the CORE component driver of the DA9058 MFD. > > > > > > > > There are 6 warnings from scripts/checkpatch.pl, but since it > > > > seems to be complaining about variable names such as min_uV are in > > > > CamelCase, when it is obvious that they are not in CamelCase I have > ignored them. > > > > > > > ??? min_uV _is_ CamelCase ??? > > > > > > Ok, maybe it is camelcasE, but you are splitting hairs here. > > > > it is not me splitting hairs, it is scripts/checkpatch.pl > > > Maybe you did not understand what I meant. Per your logic, > > MicroVolt is CamelCase > uVolt is ??? > uV is not CamelCase > > By abbreviating CamelCase to camelCase to cC you make it, in your opinion, > acceptable. > > If you want to declare CamelCase variables, just do it, but don't claim that > they > are not really CamelCase. > > Guenter I always thought that camel case meant "changing from lower case to upper case the first letter of each word and then joining the capitalized words together", so by that definition uV or mW are not camel case because "v" and "w" are not words! Either way it seems that the algorithm in scripts/checkpatch.pl is wrong! and that was my point. Tony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver
> -Original Message- > From: Guenter Roeck [mailto:li...@roeck-us.net] > Sent: 12 April 2013 14:32 > To: Opensource [Anthony Olech] > Cc: Mark Brown; Liam Girdwood; Jean Delvare; Randy Dunlap; LKML; David > Dajun Chen > Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver > > On Fri, Apr 12, 2013 at 02:05:28PM +0100, Anthony Olech wrote: > > This is the REGULATOR component driver of the Dialog DA9058 PMIC. > > This driver is just one component of the whole DA9058 PMIC driver. > > It depends on the CORE component driver of the DA9058 MFD. > > > > There are 6 warnings from scripts/checkpatch.pl, but since it seems to > > be complaining about variable names such as min_uV are in CamelCase, > > when it is obvious that they are not in CamelCase I have ignored them. > > > ??? min_uV _is_ CamelCase ??? > > Ok, maybe it is camelcasE, but you are splitting hairs here. it is not me splitting hairs, it is scripts/checkpatch.pl Tony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver
-Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 12 April 2013 14:32 To: Opensource [Anthony Olech] Cc: Mark Brown; Liam Girdwood; Jean Delvare; Randy Dunlap; LKML; David Dajun Chen Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver On Fri, Apr 12, 2013 at 02:05:28PM +0100, Anthony Olech wrote: This is the REGULATOR component driver of the Dialog DA9058 PMIC. This driver is just one component of the whole DA9058 PMIC driver. It depends on the CORE component driver of the DA9058 MFD. There are 6 warnings from scripts/checkpatch.pl, but since it seems to be complaining about variable names such as min_uV are in CamelCase, when it is obvious that they are not in CamelCase I have ignored them. ??? min_uV _is_ CamelCase ??? Ok, maybe it is camelcasE, but you are splitting hairs here. it is not me splitting hairs, it is scripts/checkpatch.pl Tony Olech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver
-Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 15 April 2013 17:36 To: Opensource [Anthony Olech] Cc: LKML Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver On Mon, Apr 15, 2013 at 03:00:58PM +, Opensource [Anthony Olech] wrote: -Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 12 April 2013 14:32 To: Opensource [Anthony Olech] Cc: Mark Brown; Liam Girdwood; Jean Delvare; Randy Dunlap; LKML; David Dajun Chen Subject: Re: [NEW DRIVER V4 7/7] DA9058 REGULATOR driver On Fri, Apr 12, 2013 at 02:05:28PM +0100, Anthony Olech wrote: This is the REGULATOR component driver of the Dialog DA9058 PMIC. This driver is just one component of the whole DA9058 PMIC driver. It depends on the CORE component driver of the DA9058 MFD. There are 6 warnings from scripts/checkpatch.pl, but since it seems to be complaining about variable names such as min_uV are in CamelCase, when it is obvious that they are not in CamelCase I have ignored them. ??? min_uV _is_ CamelCase ??? Ok, maybe it is camelcasE, but you are splitting hairs here. it is not me splitting hairs, it is scripts/checkpatch.pl Maybe you did not understand what I meant. Per your logic, MicroVolt is CamelCase uVolt is ??? uV is not CamelCase By abbreviating CamelCase to camelCase to cC you make it, in your opinion, acceptable. If you want to declare CamelCase variables, just do it, but don't claim that they are not really CamelCase. Guenter I always thought that camel case meant changing from lower case to upper case the first letter of each word and then joining the capitalized words together, so by that definition uV or mW are not camel case because v and w are not words! Either way it seems that the algorithm in scripts/checkpatch.pl is wrong! and that was my point. Tony Olech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver
> -Original Message- > From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] > Sent: 17 September 2012 12:33 > To: Opensource [Anthony Olech] > Cc: Liam Girdwood; Guenter Roeck; Jean Delvare; Randy Dunlap; LKML; David > Dajun Chen > Subject: Re: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver > On Mon, Sep 17, 2012 at 11:23:54AM +0000, Opensource [Anthony Olech] > wrote: > > Can you suggest a future proofed way of using the new regulator API > > that would solve my problem? > As I said you should set the voltage as part of the set voltage operation. So I will have to write my own set_voltage_sel() callback instead of using the default regulator_set_voltage_sel_regmap() ?? Well, I did try hard to use your defaults, but I suppose there is nothing to stop me calling regulator_set_voltage_sel_regmap() explicitly as the first part of my set_voltage_sel() callback before setting the ramp_enable bit. Then my implemenation of the set_voltage_time_sel() callback needs to just simply return the ramp time. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver
> -Original Message- > From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] > Sent: 17 September 2012 12:16 > To: Opensource [Anthony Olech] > Cc: Liam Girdwood; Guenter Roeck; Jean Delvare; Randy Dunlap; LKML; David > Dajun Chen > Subject: Re: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver > On Mon, Sep 17, 2012 at 10:49:22AM +0000, Opensource [Anthony Olech] > wrote: > > I thought that the set_voltage_sel = regulator_set_voltage_sel_regmap > > callback first set the target voltage, and that the > > set_voltage_time_sel = da9058_buck_ramp_voltage callback was called > afterwards? > > was I wrong? > That will currently happen but we might also decide to query the ramp time > first, or we might decide to query the ramp time totally separately to > actually > changing the voltage (to decide when we will want to change the voltage in > DVFS situations for example). Can you suggest a future proofed way of using the new regulator API that would solve my problem? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver
> -Original Message- > From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] > Sent: 17 September 2012 11:40 > To: Opensource [Anthony Olech] > Cc: Liam Girdwood; Guenter Roeck; Jean Delvare; Randy Dunlap; LKML; David > Dajun Chen > Subject: Re: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver > On Mon, Sep 17, 2012 at 10:29:43AM +0000, Opensource [Anthony Olech] > wrote: > > > Why is this function writing to the hardware, especially writing the > > > same value every time? > > the ramp_register is DA9058_SUPPLY_REG and it is marked as volitile. > > Writing to the ramp enable bit starts the voltage change. When the > > PMIC has finished making the change it resets the bit. Thus to make > > another voltage change the bit needs to be set again. > This function is retrieving the amount of time it would take to set the > voltage. > Why would it be starting a voltage ramp? The fact that it's not setting the > new > voltage in the hardware ought to be a warning sign here... Thanks for the quick response Mark, I thought that the set_voltage_sel = regulator_set_voltage_sel_regmap callback first set the target voltage, and that the set_voltage_time_sel = da9058_buck_ramp_voltage callback was called afterwards? was I wrong? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver
> -Original Message-
> From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com]
> Sent: 27 August 2012 17:51
> To: Opensource [Anthony Olech]
> Cc: Liam Girdwood; Guenter Roeck; Jean Delvare; Randy Dunlap; LKML; David
> Dajun Chen
> Subject: Re: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver
> On Wed, Aug 15, 2012 at 04:05:25PM +0100, Anthony Olech wrote:
> > +static int da9058_buck_ramp_voltage(struct regulator_dev *rdev,
> > + unsigned int old_selector,
> > + unsigned int new_selector)
> > +{
> > + ret = da9058_set_bits(da9058, regulator->ramp_register,
> > + regulator->ramp_enable_mask);
> > + if (ret)
> > + return ret;
> > + return 2200; /* micro Seconds needed to ramp to new voltage*/
> Why is this function writing to the hardware, especially writing the same
> value
> every time?
the ramp_register is DA9058_SUPPLY_REG and it is marked as volitile.
Writing to the ramp enable bit starts the voltage change. When the PMIC has
finished making the change it resets the bit. Thus to make another voltage
change the bit needs to be set again.
> > +static int da9058_get_fixed_regulator_voltage(struct regulator_dev
> > +*rdev) {
> > + struct da9058_regulator *regulator = rdev_get_drvdata(rdev);
> > +
> > + if (regulator_is_enabled_regmap(rdev))
> > + return regulator->fixed_voltage;
> > + else
> > + return 0;
> > +}
> list_voltage_linear()
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver
-Original Message-
From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com]
Sent: 27 August 2012 17:51
To: Opensource [Anthony Olech]
Cc: Liam Girdwood; Guenter Roeck; Jean Delvare; Randy Dunlap; LKML; David
Dajun Chen
Subject: Re: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver
On Wed, Aug 15, 2012 at 04:05:25PM +0100, Anthony Olech wrote:
+static int da9058_buck_ramp_voltage(struct regulator_dev *rdev,
+ unsigned int old_selector,
+ unsigned int new_selector)
+{
+ ret = da9058_set_bits(da9058, regulator-ramp_register,
+ regulator-ramp_enable_mask);
+ if (ret)
+ return ret;
+ return 2200; /* micro Seconds needed to ramp to new voltage*/
Why is this function writing to the hardware, especially writing the same
value
every time?
the ramp_register is DA9058_SUPPLY_REG and it is marked as volitile.
Writing to the ramp enable bit starts the voltage change. When the PMIC has
finished making the change it resets the bit. Thus to make another voltage
change the bit needs to be set again.
+static int da9058_get_fixed_regulator_voltage(struct regulator_dev
+*rdev) {
+ struct da9058_regulator *regulator = rdev_get_drvdata(rdev);
+
+ if (regulator_is_enabled_regmap(rdev))
+ return regulator-fixed_voltage;
+ else
+ return 0;
+}
list_voltage_linear()
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver
-Original Message- From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] Sent: 17 September 2012 11:40 To: Opensource [Anthony Olech] Cc: Liam Girdwood; Guenter Roeck; Jean Delvare; Randy Dunlap; LKML; David Dajun Chen Subject: Re: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver On Mon, Sep 17, 2012 at 10:29:43AM +, Opensource [Anthony Olech] wrote: Why is this function writing to the hardware, especially writing the same value every time? the ramp_register is DA9058_SUPPLY_REG and it is marked as volitile. Writing to the ramp enable bit starts the voltage change. When the PMIC has finished making the change it resets the bit. Thus to make another voltage change the bit needs to be set again. This function is retrieving the amount of time it would take to set the voltage. Why would it be starting a voltage ramp? The fact that it's not setting the new voltage in the hardware ought to be a warning sign here... Thanks for the quick response Mark, I thought that the set_voltage_sel = regulator_set_voltage_sel_regmap callback first set the target voltage, and that the set_voltage_time_sel = da9058_buck_ramp_voltage callback was called afterwards? was I wrong? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver
-Original Message- From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] Sent: 17 September 2012 12:16 To: Opensource [Anthony Olech] Cc: Liam Girdwood; Guenter Roeck; Jean Delvare; Randy Dunlap; LKML; David Dajun Chen Subject: Re: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver On Mon, Sep 17, 2012 at 10:49:22AM +, Opensource [Anthony Olech] wrote: I thought that the set_voltage_sel = regulator_set_voltage_sel_regmap callback first set the target voltage, and that the set_voltage_time_sel = da9058_buck_ramp_voltage callback was called afterwards? was I wrong? That will currently happen but we might also decide to query the ramp time first, or we might decide to query the ramp time totally separately to actually changing the voltage (to decide when we will want to change the voltage in DVFS situations for example). Can you suggest a future proofed way of using the new regulator API that would solve my problem? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver
-Original Message- From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] Sent: 17 September 2012 12:33 To: Opensource [Anthony Olech] Cc: Liam Girdwood; Guenter Roeck; Jean Delvare; Randy Dunlap; LKML; David Dajun Chen Subject: Re: [NEW DRIVER V3 8/8] DA9058 REGULATOR driver On Mon, Sep 17, 2012 at 11:23:54AM +, Opensource [Anthony Olech] wrote: Can you suggest a future proofed way of using the new regulator API that would solve my problem? As I said you should set the voltage as part of the set voltage operation. So I will have to write my own set_voltage_sel() callback instead of using the default regulator_set_voltage_sel_regmap() ?? Well, I did try hard to use your defaults, but I suppose there is nothing to stop me calling regulator_set_voltage_sel_regmap() explicitly as the first part of my set_voltage_sel() callback before setting the ramp_enable bit. Then my implemenation of the set_voltage_time_sel() callback needs to just simply return the ramp time. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V3 1/8] DA9058 MFD core driver
> -Original Message-
> From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com]
> Sent: 15 August 2012 19:53
> To: Opensource [Anthony Olech]
> Cc: Samuel Ortiz; Arnd Bergmann; Mauro Carvalho Chehab; Steven Toth;
> Michael Krufky; LKML; David Dajun Chen
> Subject: Re: [NEW DRIVER V3 1/8] DA9058 MFD core driver
> On Wed, Aug 15, 2012 at 04:05:21PM +0100, Anthony Olech wrote:
> >
> > if HAS_IOMEM
> > +
> > menu "Multifunction device drivers"
> This random change is still present from the first version
> > + /*
> > +* the init_board_irq() call-back function should be defined in
> > +* the machine driver initialization code and is used to set up
> > +* the actual (probably GPIO) line as an interrupt line.
> > +*/
> > + if (pdata->init_board_irq) {
> > + ret = pdata->init_board_irq();
> > + if (ret)
> > + goto failed_to_setup_the_actual_i2c_hw_irq;
> > + }
> You appear to have ignored my previous review comments about this... it still
> shouldn't be needed with modern kernels.
The comment you made was "Why is this conditional? With irqdomains there's no
real reason
to not allocate the IRQs even if you can't usefully use them and it helps make
the code simpler."
I obviously did not understand, because you also wrote that the driver must not
rely of platform
data. That therefore means that the call-back function pdata->init_board_irq
might be NULL,
which in turn means that a check must be done.
The only other interpretation of your comment is that the h/w IRQ line setup
code must be done
in the machine driver, in my case for testing is
arch/arm/mach-s3c64xx/mach-smdk6410.c
But that view is contricted by the init call-backs from the wm8350 and wm1192
drivers.
I am confused, but it does make sense to do the GPIO --> IRQ initialization in
the machine driver,
so I will try to do that. But who would be responsible for changing the wm8350
and wm1192 drivers
to also follow that philosophy?
> > +static bool da9058_register_volatile(struct device *dev, unsigned int
> > +reg) {
> > + switch (reg) {
> > + case DA9058_ADCMAN_REG:
> > + case DA9058_ADCRESH_REG:
> > + case DA9058_ADCRESL_REG:
> > + case DA9058_ALARMD_REG:
> > + case DA9058_ALARMH_REG:
> > + case DA9058_ALARMMI_REG:
> > + case DA9058_ALARMMO_REG:
> > + case DA9058_ALARMS_REG:
> > + case DA9058_ALARMY_REG:
> Are all the alarm registers really volatile?
register DA9058_ADCMAN_REG has one bit that is set by the PMIC
register DA9058_ADCRESH_REG has all bits set by the PMIC
register DA9058_ADCRESL_REG has some bits set by the PMIC
so those are volatile
the registers DA9058_ALARMD... are, on detailed inspection of the chip
documentation, not volatile. I misread the docs previous. I will make the
change. Well done for spotting my mistake - thanks!
> > + case DA9058_LDO9_REG:
> > + case DA9058_TOFFSET_REG:
> > + default:
> > + return false;
> Just use the default.
The compiler will (I hope) produce the same code even if some of the
default values are enumerated. I produced those call-backs by starting
with all the registers and moving the cases around. I seemed a good
idea at the time. Your objection, I presume, is based on source file size
or aesthetics, but I will eliminate the functionally redundant cases.
> > +static struct regulator_consumer_supply platform_vddarm_consumers[] = {
> > + {.supply = "vcc",}
> > +};
> No, this and all your other regulator configuration is board specific.
I will remove that.
Thank you Mark for the review.
Tony Olech
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V3 1/8] DA9058 MFD core driver
-Original Message-
From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com]
Sent: 15 August 2012 19:53
To: Opensource [Anthony Olech]
Cc: Samuel Ortiz; Arnd Bergmann; Mauro Carvalho Chehab; Steven Toth;
Michael Krufky; LKML; David Dajun Chen
Subject: Re: [NEW DRIVER V3 1/8] DA9058 MFD core driver
On Wed, Aug 15, 2012 at 04:05:21PM +0100, Anthony Olech wrote:
if HAS_IOMEM
+
menu Multifunction device drivers
This random change is still present from the first version
+ /*
+* the init_board_irq() call-back function should be defined in
+* the machine driver initialization code and is used to set up
+* the actual (probably GPIO) line as an interrupt line.
+*/
+ if (pdata-init_board_irq) {
+ ret = pdata-init_board_irq();
+ if (ret)
+ goto failed_to_setup_the_actual_i2c_hw_irq;
+ }
You appear to have ignored my previous review comments about this... it still
shouldn't be needed with modern kernels.
The comment you made was Why is this conditional? With irqdomains there's no
real reason
to not allocate the IRQs even if you can't usefully use them and it helps make
the code simpler.
I obviously did not understand, because you also wrote that the driver must not
rely of platform
data. That therefore means that the call-back function pdata-init_board_irq
might be NULL,
which in turn means that a check must be done.
The only other interpretation of your comment is that the h/w IRQ line setup
code must be done
in the machine driver, in my case for testing is
arch/arm/mach-s3c64xx/mach-smdk6410.c
But that view is contricted by the init call-backs from the wm8350 and wm1192
drivers.
I am confused, but it does make sense to do the GPIO -- IRQ initialization in
the machine driver,
so I will try to do that. But who would be responsible for changing the wm8350
and wm1192 drivers
to also follow that philosophy?
+static bool da9058_register_volatile(struct device *dev, unsigned int
+reg) {
+ switch (reg) {
+ case DA9058_ADCMAN_REG:
+ case DA9058_ADCRESH_REG:
+ case DA9058_ADCRESL_REG:
+ case DA9058_ALARMD_REG:
+ case DA9058_ALARMH_REG:
+ case DA9058_ALARMMI_REG:
+ case DA9058_ALARMMO_REG:
+ case DA9058_ALARMS_REG:
+ case DA9058_ALARMY_REG:
Are all the alarm registers really volatile?
register DA9058_ADCMAN_REG has one bit that is set by the PMIC
register DA9058_ADCRESH_REG has all bits set by the PMIC
register DA9058_ADCRESL_REG has some bits set by the PMIC
so those are volatile
the registers DA9058_ALARMD... are, on detailed inspection of the chip
documentation, not volatile. I misread the docs previous. I will make the
change. Well done for spotting my mistake - thanks!
+ case DA9058_LDO9_REG:
+ case DA9058_TOFFSET_REG:
+ default:
+ return false;
Just use the default.
The compiler will (I hope) produce the same code even if some of the
default values are enumerated. I produced those call-backs by starting
with all the registers and moving the cases around. I seemed a good
idea at the time. Your objection, I presume, is based on source file size
or aesthetics, but I will eliminate the functionally redundant cases.
+static struct regulator_consumer_supply platform_vddarm_consumers[] = {
+ {.supply = vcc,}
+};
No, this and all your other regulator configuration is board specific.
I will remove that.
Thank you Mark for the review.
Tony Olech
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V2 5/7] DA9058 GPIO driver
> -Original Message-
> From: Linus Walleij [mailto:linus.wall...@linaro.org]
> Sent: 13 August 2012 14:10
> To: Opensource [Anthony Olech]
> Cc: Grant Likely; Linus Walleij; Mark Brown; LKML; David Dajun Chen; Samuel
> Ortiz; Lee Jones
> Subject: Re: [NEW DRIVER V2 5/7] DA9058 GPIO driver
> Hi Anthony, sorry for delayed reply...
> On Sun, Aug 5, 2012 at 10:43 PM, Anthony Olech
> wrote:
> > This is the GPIO component driver of the Dialog DA9058 PMIC.
> > This driver is just one component of the whole DA9058 PMIC driver.
> > It depends on the core DA9058 MFD driver.
> OK
> > +config GPIO_DA9058
> > + tristate "Dialog DA9058 GPIO"
> > + depends on MFD_DA9058
> select IRQ_DOMAIN, you're going to want to use it...
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index
> > 0f55662..209224a 100644
> (...)
> > +#include
> > +#include
> Really?
> > +#include
> Really?
> > +#include
> > +#include
> > +#include
> Really?
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> If you're using regmap you better select it in Kconfig too, but it appears you
> don't. You should be using regmap in the main MFD driver in this case (I
> haven't looked at it though.)
> This header set just looks like it was copied from some other file and never
> really proofread, so please go over it in detail.
for some reason against 2.6.2x some of those were required, I just totally
forgot to prune out the unwanted ones when rebasing forwards to 3.5.
Good that you spotted it! Sorry I will try to prune the includes in the future.
> > +#include #include
> > + #include
> > +#include #include
> > +#include
> Samuel will have to comment on this organization of headers, it seems a little
> much. DO you really need all of them?
One of them should have been stripped out by my submit script, but as for the
others you must bear in mind that the DA9058 PMIC is a multifunction device,
and thus some header files are common and some are specific to various
component drivers. The very reason that you picked up on non-relevant include
files surely has implications on the structure of the header files for the
DA9058,
in particular struct's and define's that only apply to one component driver
should
be in separate header files.
> > +struct da9058_gpio {
> > + struct da9058 *da9058;
> > + struct platform_device *pdev;
> > + struct gpio_chip gp;
> > + struct mutex lock;
> > + u8 inp_config;
> > + u8 out_config;
> > +};
> > +
> > +static struct da9058_gpio *gpio_chip_to_da9058_gpio(struct gpio_chip
> > +*chip) {
> > + return container_of(chip, struct da9058_gpio, gp); }
> static inline, or a #define, but the compile will probably optimize-inline it
> anyway.
The compiler should optimize it to in-line, but I will change it anyway.
> > +static int da9058_gpio_get(struct gpio_chip *gc, unsigned offset) {
> > + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> > + struct da9058 *da9058 = gpio->da9058;
> > + unsigned int gpio_level;
> > + int ret;
> > +
> > + if (offset > 1)
> > + return -EINVAL;
> So there are two GPIO pins, 0 and 1? That seems odd, but OK.
That is a feature of the hardware. I believe that calling them "0" and
"1" is the correct thing to do. Correct me if they should have been
called "1" and "2", or something else.
> > + if (offset) {
> So this is for GPIO 1
Yes, it seemed the obvious thing to do.
> > + u8 value_bits = value ? 0x80 : 0x00;
>
> These "value_bits" are just confusing. Just delete this and use the direct
> value
> below.
Will do. It was done for diagnostics that have since been stripped out.
> > +
> > + gpio->out_config &= ~0x80;
> A better way of writing &= ~0x80; is &= 0x7F
> > + gpio->out_config |= value_bits;
> gpio->out_config = value ? 0x80 : 0x00;
> So, less confusing.
see HANDLING NIBBLES below
> > + if (!(gpio_cntrl & 0x20))
> > + goto exit;
> Please insert a comment explaining what this bit is doing and why you're just
> exiting if it's not set. I don't understand one thing.
I have explained why in the driver source in the next submission attempt
> Maybe this would be better if you didn't use so many magic values, what about:
> #include
> #define FOO_FLAG BIT(3) /* This is a flag for foo */
> > +
> > + g
RE: [NEW DRIVER V2 5/7] DA9058 GPIO driver
-Original Message-
From: Linus Walleij [mailto:linus.wall...@linaro.org]
Sent: 13 August 2012 14:10
To: Opensource [Anthony Olech]
Cc: Grant Likely; Linus Walleij; Mark Brown; LKML; David Dajun Chen; Samuel
Ortiz; Lee Jones
Subject: Re: [NEW DRIVER V2 5/7] DA9058 GPIO driver
Hi Anthony, sorry for delayed reply...
On Sun, Aug 5, 2012 at 10:43 PM, Anthony Olech
anthony.olech.opensou...@diasemi.com wrote:
This is the GPIO component driver of the Dialog DA9058 PMIC.
This driver is just one component of the whole DA9058 PMIC driver.
It depends on the core DA9058 MFD driver.
OK
+config GPIO_DA9058
+ tristate Dialog DA9058 GPIO
+ depends on MFD_DA9058
select IRQ_DOMAIN, you're going to want to use it...
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index
0f55662..209224a 100644
(...)
+#include linux/module.h
+#include linux/fs.h
Really?
+#include linux/uaccess.h
Really?
+#include linux/platform_device.h
+#include linux/gpio.h
+#include linux/syscalls.h
Really?
+#include linux/seq_file.h
+#include linux/slab.h
+#include linux/interrupt.h
+#include linux/mfd/core.h
+#include linux/regmap.h
If you're using regmap you better select it in Kconfig too, but it appears you
don't. You should be using regmap in the main MFD driver in this case (I
haven't looked at it though.)
This header set just looks like it was copied from some other file and never
really proofread, so please go over it in detail.
for some reason against 2.6.2x some of those were required, I just totally
forgot to prune out the unwanted ones when rebasing forwards to 3.5.
Good that you spotted it! Sorry I will try to prune the includes in the future.
+#include linux/mfd/da9058/version.h #include
+linux/mfd/da9058/registers.h #include linux/mfd/da9058/core.h
+#include linux/mfd/da9058/gpio.h #include linux/mfd/da9058/irq.h
+#include linux/mfd/da9058/pdata.h
Samuel will have to comment on this organization of headers, it seems a little
much. DO you really need all of them?
One of them should have been stripped out by my submit script, but as for the
others you must bear in mind that the DA9058 PMIC is a multifunction device,
and thus some header files are common and some are specific to various
component drivers. The very reason that you picked up on non-relevant include
files surely has implications on the structure of the header files for the
DA9058,
in particular struct's and define's that only apply to one component driver
should
be in separate header files.
+struct da9058_gpio {
+ struct da9058 *da9058;
+ struct platform_device *pdev;
+ struct gpio_chip gp;
+ struct mutex lock;
+ u8 inp_config;
+ u8 out_config;
+};
+
+static struct da9058_gpio *gpio_chip_to_da9058_gpio(struct gpio_chip
+*chip) {
+ return container_of(chip, struct da9058_gpio, gp); }
static inline, or a #define, but the compile will probably optimize-inline it
anyway.
The compiler should optimize it to in-line, but I will change it anyway.
+static int da9058_gpio_get(struct gpio_chip *gc, unsigned offset) {
+ struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
+ struct da9058 *da9058 = gpio-da9058;
+ unsigned int gpio_level;
+ int ret;
+
+ if (offset 1)
+ return -EINVAL;
So there are two GPIO pins, 0 and 1? That seems odd, but OK.
That is a feature of the hardware. I believe that calling them 0 and
1 is the correct thing to do. Correct me if they should have been
called 1 and 2, or something else.
+ if (offset) {
So this is for GPIO 1
Yes, it seemed the obvious thing to do.
+ u8 value_bits = value ? 0x80 : 0x00;
These value_bits are just confusing. Just delete this and use the direct
value
below.
Will do. It was done for diagnostics that have since been stripped out.
+
+ gpio-out_config = ~0x80;
A better way of writing = ~0x80; is = 0x7F
+ gpio-out_config |= value_bits;
gpio-out_config = value ? 0x80 : 0x00;
So, less confusing.
see HANDLING NIBBLES below
+ if (!(gpio_cntrl 0x20))
+ goto exit;
Please insert a comment explaining what this bit is doing and why you're just
exiting if it's not set. I don't understand one thing.
I have explained why in the driver source in the next submission attempt
Maybe this would be better if you didn't use so many magic values, what about:
#include linux/bitops.h
#define FOO_FLAG BIT(3) /* This is a flag for foo */
+
+ gpio_cntrl = ~0xF0;
A better way to write = ~F0 is to write = 0x0F;
If you don't #define the constants this way of negating numbers just get
confusing.
So this is OK:
foo = ~FOO_FLAG;
foo |= set ? FOO_FLAG : 0;
This is just hard to read:
foo = ~0x55;
foo |= set ? 0x55 : 0;
And is better off
foo = 0xAA;
foo
RE: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped
> -Original Message- > From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] > Sent: 07 August 2012 18:02 > To: Opensource [Anthony Olech] > Cc: LKML; David Dajun Chen > Subject: Re: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped > On Tue, Aug 07, 2012 at 02:37:37PM +, Opensource [Anthony Olech] > wrote: > > > The bottom line here is that if your driver requires a dynamically > > > allocated legacy domain it is broken. > > I am trying to use the latest REGMAP API, and I do not understand why > > you say the DSA9058 driver "requires" a dynamically allocated legacy > domain. > If you care if you got a linear or a legacy domain then that shows you're > reyling > on having a legacy domain (indeed, my statement above should've been > stronger - if anything in the driver itself cares if it's got a linear or a > legacy > domain the driver is buggy). > > Surely the virtual IRQs that the PMIC component drivers use must be > > dynamically allocated. It is only the single GPIO line designated as > > an interrupt line in the machne drivert that is fixed by the hardware. > > That surely means the "irq_base" parameter to regmap_add_irq_chip() > > must be set to "-1". What else could it be set to?? > If the driver doesn't have any inputs which could be used as an interrupt by > another device then it should be set to -1, yes, and nothing in the code > should > ever care how the specific virq values are related to each other. > If the driver does support another device using it as an interrupt controller > then > unfortunately for non-DT systems platform data would need to configure an > irq_base so that the interrupt can be supplied to whatever the other device is > but in all other circumstances it should be set to -1. > > I am beginning to suspect that I have misunderstood something. The > > regmap-irq API seemed taylor-made for our PMIC with one real h/w > > interrupt line with several PMIC chip irq sources controlled by a set > > of registers that seemed to slot into the "regmap_add_irq_chip" struct > > perfectly. Why should that set of virtual irqs be given a specific base?? > It shouldn't, this is what I'm saying. The IRQs shouldn't have a base at all > and > should instead be using a linear domain (which doesn't have a base but instead > maps each IRQ on demand). What your patch does is to stop that happening > and instead always allocate a legacy domain even when linear is OK. > It sounds like your code to allocate the IRQs is fine but the code using the > IRQs > is buggy as it's relying on the linear domain. Thanks Mark, now we are getting somewhere - it must be my use of the IRQ domain IRQs is wrong. There are exactly 3 mfd drivers that specify a base of '-1', namely: palmas.c, 88pm805.c, arizona-irq.c so I will examine how they use the IRQs thanks for your help Tony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V1 5/7] DA9058 GPIO driver
> -Original Message- > From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] > Sent: 07 August 2012 18:15 > To: Opensource [Anthony Olech] > Cc: LKML > Subject: Re: [NEW DRIVER V1 5/7] DA9058 GPIO driver > On Mon, Aug 06, 2012 at 03:15:17PM +, Opensource [Anthony Olech] > wrote: > > I do realize that REGMAP does locking on individual register accesses, > > however, the each GPIO line is controlled by 4-bits in a register, > > with the meaning of the most significant bit depending on the GPIO > > direction, so it is essential that the register be read first before > > do an update, thus two sequential register accesses must be protected > > by a mutex to prevent another process changing the register (and hence > > the meaning of the most-significant bit) in the middle of the two accesses. > > I hope this explains to your satisfaction why a driver mutex is > > required in addition to the regmap's register access mutex > This seems a bit excessive and complicated - I'd be inclined to either just > say > that the caller is responsible for avoiding confusion here (obviously if > you're > changing the direction there's a race anyway) or store the data in a variable > locally rather than having to do I/O on the device under lock every time it's > interacted with. By using a semaphore (mutex/critical region) as I do in the DA9058 GPIO component driver, there is absolutely no set-direction-race-condition in the driver. The driver will always be consistent, either INP or OUT for each GPIO line. There may, indeed, be confusion between multiple users of the GPIO lines, but that is, quite properly, their problem. Even though in practice there will usually be only one GPIO consumer/user, it still seems to me to be better to code for the worst case. If indeed there is only one GPIO consumer/ user then the mutex access will be fast and non-blocking. The GPIO control register (as opposed to the status register) is marked as non-volitile so there should be no i2c access overhead when reading it. Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V1 5/7] DA9058 GPIO driver
-Original Message- From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] Sent: 07 August 2012 18:15 To: Opensource [Anthony Olech] Cc: LKML Subject: Re: [NEW DRIVER V1 5/7] DA9058 GPIO driver On Mon, Aug 06, 2012 at 03:15:17PM +, Opensource [Anthony Olech] wrote: I do realize that REGMAP does locking on individual register accesses, however, the each GPIO line is controlled by 4-bits in a register, with the meaning of the most significant bit depending on the GPIO direction, so it is essential that the register be read first before do an update, thus two sequential register accesses must be protected by a mutex to prevent another process changing the register (and hence the meaning of the most-significant bit) in the middle of the two accesses. I hope this explains to your satisfaction why a driver mutex is required in addition to the regmap's register access mutex This seems a bit excessive and complicated - I'd be inclined to either just say that the caller is responsible for avoiding confusion here (obviously if you're changing the direction there's a race anyway) or store the data in a variable locally rather than having to do I/O on the device under lock every time it's interacted with. By using a semaphore (mutex/critical region) as I do in the DA9058 GPIO component driver, there is absolutely no set-direction-race-condition in the driver. The driver will always be consistent, either INP or OUT for each GPIO line. There may, indeed, be confusion between multiple users of the GPIO lines, but that is, quite properly, their problem. Even though in practice there will usually be only one GPIO consumer/user, it still seems to me to be better to code for the worst case. If indeed there is only one GPIO consumer/ user then the mutex access will be fast and non-blocking. The GPIO control register (as opposed to the status register) is marked as non-volitile so there should be no i2c access overhead when reading it. Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped
-Original Message- From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] Sent: 07 August 2012 18:02 To: Opensource [Anthony Olech] Cc: LKML; David Dajun Chen Subject: Re: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped On Tue, Aug 07, 2012 at 02:37:37PM +, Opensource [Anthony Olech] wrote: The bottom line here is that if your driver requires a dynamically allocated legacy domain it is broken. I am trying to use the latest REGMAP API, and I do not understand why you say the DSA9058 driver requires a dynamically allocated legacy domain. If you care if you got a linear or a legacy domain then that shows you're reyling on having a legacy domain (indeed, my statement above should've been stronger - if anything in the driver itself cares if it's got a linear or a legacy domain the driver is buggy). Surely the virtual IRQs that the PMIC component drivers use must be dynamically allocated. It is only the single GPIO line designated as an interrupt line in the machne drivert that is fixed by the hardware. That surely means the irq_base parameter to regmap_add_irq_chip() must be set to -1. What else could it be set to?? If the driver doesn't have any inputs which could be used as an interrupt by another device then it should be set to -1, yes, and nothing in the code should ever care how the specific virq values are related to each other. If the driver does support another device using it as an interrupt controller then unfortunately for non-DT systems platform data would need to configure an irq_base so that the interrupt can be supplied to whatever the other device is but in all other circumstances it should be set to -1. I am beginning to suspect that I have misunderstood something. The regmap-irq API seemed taylor-made for our PMIC with one real h/w interrupt line with several PMIC chip irq sources controlled by a set of registers that seemed to slot into the regmap_add_irq_chip struct perfectly. Why should that set of virtual irqs be given a specific base?? It shouldn't, this is what I'm saying. The IRQs shouldn't have a base at all and should instead be using a linear domain (which doesn't have a base but instead maps each IRQ on demand). What your patch does is to stop that happening and instead always allocate a legacy domain even when linear is OK. It sounds like your code to allocate the IRQs is fine but the code using the IRQs is buggy as it's relying on the linear domain. Thanks Mark, now we are getting somewhere - it must be my use of the IRQ domain IRQs is wrong. There are exactly 3 mfd drivers that specify a base of '-1', namely: palmas.c, 88pm805.c, arizona-irq.c so I will examine how they use the IRQs thanks for your help Tony Olech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped
> -Original Message- > From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] > Sent: 07 August 2012 15:03 > On Tue, Aug 07, 2012 at 11:18:20AM +, Opensource [Anthony Olech] > wrote: > > if you don't TOP POST how can you tell who wrote what? > Well, it's not clear who wrote what in your current e-mail since there's no > indication of what's quoted and what's new text... Take a look at all the > other > mails on the list - your mail should be in a similar style to them. There's > some > advice for common e-mail clients in email-clients.txt under Documentation. I found the option to quote/indent the email original, sorry about that but we are forced to use Microsoft Outlook and the default were set up strangely. > The bottom line here is that if your driver requires a dynamically allocated > legacy domain it is broken. I am trying to use the latest REGMAP API, and I do not understand why you say the DSA9058 driver "requires" a dynamically allocated legacy domain. Surely the virtual IRQs that the PMIC component drivers use must be dynamically allocated. It is only the single GPIO line designated as an interrupt line in the machne drivert that is fixed by the hardware. That surely means the "irq_base" parameter to regmap_add_irq_chip() must be set to "-1". What else could it be set to?? I am beginning to suspect that I have misunderstood something. The regmap-irq API seemed taylor-made for our PMIC with one real h/w interrupt line with several PMIC chip irq sources controlled by a set of registers that seemed to slot into the "regmap_add_irq_chip" struct perfectly. Why should that set of virtual irqs be given a specific base?? I really hope that you can help me clear this issue up, as there are not many examples of drivers that use the regmap-irq API in linux-release GIT repository. Tony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [lm-sensors] [NEW DRIVER V2 6/7] DA9058 HWMON driver
> -Original Message- > From: Guenter Roeck [mailto:li...@roeck-us.net] > Sent: 06 August 2012 18:40 > To: Opensource [Anthony Olech] > Cc: Guenter Roeck; Jean Delvare; Randy Dunlop; Mark Brown; David Dajun > Chen; LKML; lm-sens...@lm-sensors.org > Subject: Re: [lm-sensors] [NEW DRIVER V2 6/7] DA9058 HWMON driver > On Sun, Aug 05, 2012 at 09:43:44PM +0100, Anthony Olech wrote: > > This is the HWMON component driver of the Dialog DA9058 PMIC. > > This driver is just one component of the whole DA9058 PMIC driver. > > It depends on the core DA9058 MFD driver. > > Signed-off-by: Anthony Olech > > Signed-off-by: David Dajun Chen > [ ... ] > > +static SENSOR_DEVICE_ATTR(vbat_mV, S_IRUGO, da9058_read_vbat, NULL, > > +0); static SENSOR_DEVICE_ATTR(adc_mV, S_IRUGO, > da9058_read_misc_channel, NULL, > > + DA9058_ADCMAN_MUXSEL_ADCIN); > > +static SENSOR_DEVICE_ATTR(vfpin_mV, S_IRUGO, da9058_read_vfpin, > NULL, > > +0); static SENSOR_DEVICE_ATTR(vfpin_mode, S_IRUGO, > da9058_vfpin_mode, > > +NULL, 0); static SENSOR_DEVICE_ATTR(tbat_mV, S_IRUGO, > > +da9058_read_tbat, NULL, 0); static SENSOR_DEVICE_ATTR(tjunc_in, > > +S_IRUGO, da9058_read_tjunc, NULL, 0); static > SENSOR_DEVICE_ATTR(adc_mode, S_IWUSR | S_IRUGO, > da9058_get_adc_mode, > > + da9058_set_adc_mode, 0); > Please use standard sysfs attribute names for temperature and voltage > attributes. I could not find a naming convention, so I will try to abstract one from all the HWMON driver that have your name in them. I noted when searching that I missed out a file in also that Documentation/hwmon. I will correct both issues in my next submission attempt. > For configuration (XXX_mode), please use devicetreee and/or platform data, > not sysfs attributes. As far as I can see both devicetreee and platform data allow configuration data to be passed into the driver at "probe" time, they don't allow an operating mode to be changed dynamically. That is what I thought sysfs allowed. Thus your comments seem to imply that you do not want to allow the mode to be changed dynamically. If that is the case then I can remove the dynamic mode setting, leaving it fixed by platform data. Thanks, Tony Olech -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped
if you don't TOP POST how can you tell who wrote what? see my comments embedded below -Original Message- From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] Sent: 04 August 2012 11:54 To: Opensource [Anthony Olech] Cc: LKML; Anthony Olech; David Dajun Chen Subject: Re: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped On Wed, Aug 01, 2012 at 07:05:15PM +0100, Anthony Olech wrote: > if the irq_base is set to -1 when calling regmap_add_irq_chip() then > allow the IRQ to be mapped even if the allocated irq_base is actually > zero. > This restores the behaviour seen in v3.4, and I assume that the > tidy-ups just made in v3.5 INADVERTENTLY introduce this change in > behaviour. Please pay MORE attention to the changelog - obviously there's no problem mapping automatically allocated IRQs, there's only any effect if they happen to GET allocated at zero. That is the problem - they are allocated at zero, and hence my patch The only real issue I see with the current code is that if the user explicitly wants to statically allocate an IRQ range at zero they can't. I don't want to explicitly allocate at zero. The current intended behaviour is that we use a linear domain unless a positive IRQ base is specified, though we're not quite doing that right now as a transitional measure until drivers are updated. The fact remains that my patch enables the DA9058 driver to work in v3.5 The current da9052 driver usage seems to have quite a few problems, I do recall having to fix some problems that make me doubt if it ever worked well. Looking at the code now I see it's using hard coded references to absolute IRQ numbers which is an issue... It should be being converted to use regmap_irq_get_virq(). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V2 1/7] DA9058 MFD core and ADC driver
Thanks Venu for the two comments, (1) the RTC values are not encoded in BCD, I am adding a comment in the header file to that effect (2) the "*_REG" naming convention is used for MFD component drivers, see directory include/linux/mfd for numerous examples Tony Olech -Original Message- From: Venu Byravarasu [mailto:vbyravar...@nvidia.com] Sent: 06 August 2012 09:38 To: Opensource [Anthony Olech]; Samuel Ortiz Cc: Mark Brown; Arnd Bergmann; Mauro Carvalho Chehab; Steven Toth; Michael Krufky; LKML; David Dajun Chen Subject: RE: [NEW DRIVER V2 1/7] DA9058 MFD core and ADC driver > -Original Message- > From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel- > ow...@vger.kernel.org] On Behalf Of Anthony Olech > Sent: Monday, August 06, 2012 2:14 AM > To: Samuel Ortiz > Cc: Mark Brown; Arnd Bergmann; Mauro Carvalho Chehab; Steven Toth; > Michael Krufky; LKML; David Dajun Chen > Subject: [NEW DRIVER V2 1/7] DA9058 MFD core and ADC driver > > This is the MFD core driver for the Dialog DA9058 PMIC. > This driver, via MFD CELLs, causes all the component drivers to be > loaded, if it is a module, and initialized via their probe methods. > It also provides access to the ADC functions on the PMIC. > All the other component drivers depend on this one. > > +#endif /* __DA9058_PDATA_H */ > diff --git a/include/linux/mfd/da9058/registers.h > b/include/linux/mfd/da9058/registers.h > new file mode 100644 > index 000..0fd6aef > --- /dev/null > +++ b/include/linux/mfd/da9058/registers.h > @@ -0,0 +1,480 @@ > +/* > + * Copyright (C) 2012 Dialog Semiconductor Ltd. > + * > + * This program is free software; you can redistribute it and/or > +modify > + * it under the terms of the GNU General Public License as published > +by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + */ > + > +#ifndef __DA9058_REGISTERS_H > +#define __DA9058_REGISTERS_H > + > +#define DA9058_PAGECON0_REG 0 You can remove _REG after each register name. > +#define DA9058_STATUSA_REG 1 > +#define DA9058_STATUSB_REG 2 > +#define DA9058_STATUSC_REG 3 > +#define DA9058_STATUSD_REG 4 > +/* RTC fields */ > +#define DA9058_RTC_SECS_MASK 0x3F Usually most of the PMIC RTCs provide RTC time registers in BCD Format. Plz make sure that is not the case with your device, otherwise these masks may not be valid. > +#define DA9058_RTC_MINS_MASK 0x3F > +#define DA9058_RTC_HRS_MASK 0x1F > +#define DA9058_RTC_DAY_MASK 0x1F > +#define DA9058_RTC_MTH_MASK 0x0F > +#define DA9058_RTC_YRS_MASK 0x3F > +#define DA9058_RTC_ALMSECS_MASK 0x3F > +#define DA9058_RTC_ALMMINS_MASK 0x3F > +#define DA9058_RTC_ALMHRS_MASK 0x1F > +#define DA9058_RTC_ALMDAY_MASK 0x1F > +#define DA9058_RTC_ALMMTH_MASK 0x0F > +#define DA9058_RTC_ALMYRS_MASK 0x3F > +/* RTC TIMER SECONDS REGISTER */ > +#define DA9058_COUNTS_COUNTSEC (63<<0) > +/* RTC TIMER MINUTES REGISTER */ > +#define DA9058_COUNTMI_COUNTMIN (63<<0) > +/* RTC TIMER HOUR REGISTER */ > +#define DA9058_COUNTH_COUNTHOUR (31<<0) > +/* RTC TIMER DAYS REGISTER */ > +#define DA9058_COUNTD_COUNTDAY (31<<0) > +/* RTC TIMER MONTHS REGISTER */ > +#define DA9058_COUNTMO_COUNTMONTH(15<<0) > +/* RTC TIMER YEARS REGISTER */ > +#define DA9058_COUNTY_MONITOR(1<<6) > +#define DA9058_COUNTY_COUNTYEAR (63<<0) > +/* RTC ALARM SECONDS REGISTER */ > +#define DA9058_ALARMMI_COUNTSEC (63<<0) > +/* RTC ALARM MINUTES REGISTER */ > +#define DA9058_ALARMMI_TICKTYPE (1<<7) > +#define DA9058_ALARMMI_ALARMMIN (63<<0) > +/* RTC ALARM HOURS REGISTER */ > +#define DA9058_ALARMH_ALARMHOUR (31<<0) > +/* RTC ALARM DAYS REGISTER */ > +#define DA9058_ALARMD_ALARMDAY (31<<0) > +/* RTC ALARM MONTHS REGISTER */ > +#define DA9058_ALARMMO_ALARMMONTH(15<<0) > +/* RTC ALARM YEARS REGISTER */ > +#define DA9058_ALARMY_TICKON (1<<7) > +#define DA9058_ALARMY_ALARMON(1<<6) > +#define DA9058_ALARMY_ALARMYEAR (63<<0) > +/* CHIP IDENTIFICATION REGISTER */ > +#define DA9058_CHIPID_MRC
RE: [NEW DRIVER V2 1/7] DA9058 MFD core and ADC driver
Thanks Venu for the two comments, (1) the RTC values are not encoded in BCD, I am adding a comment in the header file to that effect (2) the *_REG naming convention is used for MFD component drivers, see directory include/linux/mfd for numerous examples Tony Olech -Original Message- From: Venu Byravarasu [mailto:vbyravar...@nvidia.com] Sent: 06 August 2012 09:38 To: Opensource [Anthony Olech]; Samuel Ortiz Cc: Mark Brown; Arnd Bergmann; Mauro Carvalho Chehab; Steven Toth; Michael Krufky; LKML; David Dajun Chen Subject: RE: [NEW DRIVER V2 1/7] DA9058 MFD core and ADC driver -Original Message- From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel- ow...@vger.kernel.org] On Behalf Of Anthony Olech Sent: Monday, August 06, 2012 2:14 AM To: Samuel Ortiz Cc: Mark Brown; Arnd Bergmann; Mauro Carvalho Chehab; Steven Toth; Michael Krufky; LKML; David Dajun Chen Subject: [NEW DRIVER V2 1/7] DA9058 MFD core and ADC driver This is the MFD core driver for the Dialog DA9058 PMIC. This driver, via MFD CELLs, causes all the component drivers to be loaded, if it is a module, and initialized via their probe methods. It also provides access to the ADC functions on the PMIC. All the other component drivers depend on this one. +#endif /* __DA9058_PDATA_H */ diff --git a/include/linux/mfd/da9058/registers.h b/include/linux/mfd/da9058/registers.h new file mode 100644 index 000..0fd6aef --- /dev/null +++ b/include/linux/mfd/da9058/registers.h @@ -0,0 +1,480 @@ +/* + * Copyright (C) 2012 Dialog Semiconductor Ltd. + * + * This program is free software; you can redistribute it and/or +modify + * it under the terms of the GNU General Public License as published +by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + */ + +#ifndef __DA9058_REGISTERS_H +#define __DA9058_REGISTERS_H + +#define DA9058_PAGECON0_REG 0 You can remove _REG after each register name. +#define DA9058_STATUSA_REG 1 +#define DA9058_STATUSB_REG 2 +#define DA9058_STATUSC_REG 3 +#define DA9058_STATUSD_REG 4 +/* RTC fields */ +#define DA9058_RTC_SECS_MASK 0x3F Usually most of the PMIC RTCs provide RTC time registers in BCD Format. Plz make sure that is not the case with your device, otherwise these masks may not be valid. +#define DA9058_RTC_MINS_MASK 0x3F +#define DA9058_RTC_HRS_MASK 0x1F +#define DA9058_RTC_DAY_MASK 0x1F +#define DA9058_RTC_MTH_MASK 0x0F +#define DA9058_RTC_YRS_MASK 0x3F +#define DA9058_RTC_ALMSECS_MASK 0x3F +#define DA9058_RTC_ALMMINS_MASK 0x3F +#define DA9058_RTC_ALMHRS_MASK 0x1F +#define DA9058_RTC_ALMDAY_MASK 0x1F +#define DA9058_RTC_ALMMTH_MASK 0x0F +#define DA9058_RTC_ALMYRS_MASK 0x3F +/* RTC TIMER SECONDS REGISTER */ +#define DA9058_COUNTS_COUNTSEC (630) +/* RTC TIMER MINUTES REGISTER */ +#define DA9058_COUNTMI_COUNTMIN (630) +/* RTC TIMER HOUR REGISTER */ +#define DA9058_COUNTH_COUNTHOUR (310) +/* RTC TIMER DAYS REGISTER */ +#define DA9058_COUNTD_COUNTDAY (310) +/* RTC TIMER MONTHS REGISTER */ +#define DA9058_COUNTMO_COUNTMONTH(150) +/* RTC TIMER YEARS REGISTER */ +#define DA9058_COUNTY_MONITOR(16) +#define DA9058_COUNTY_COUNTYEAR (630) +/* RTC ALARM SECONDS REGISTER */ +#define DA9058_ALARMMI_COUNTSEC (630) +/* RTC ALARM MINUTES REGISTER */ +#define DA9058_ALARMMI_TICKTYPE (17) +#define DA9058_ALARMMI_ALARMMIN (630) +/* RTC ALARM HOURS REGISTER */ +#define DA9058_ALARMH_ALARMHOUR (310) +/* RTC ALARM DAYS REGISTER */ +#define DA9058_ALARMD_ALARMDAY (310) +/* RTC ALARM MONTHS REGISTER */ +#define DA9058_ALARMMO_ALARMMONTH(150) +/* RTC ALARM YEARS REGISTER */ +#define DA9058_ALARMY_TICKON (17) +#define DA9058_ALARMY_ALARMON(16) +#define DA9058_ALARMY_ALARMYEAR (630) +/* CHIP IDENTIFICATION REGISTER */ +#define DA9058_CHIPID_MRC(154) +#define DA9058_CHIPID_TRC(150) +/* CONFIGURATION IDENTIFICATION REGISTER */ +#define DA9058_CONFIGID_CONFID (70) +/* OTP CONTROL REGISTER */ +#define DA9058_OTPCONT_GPWRITEDIS(17) +#define DA9058_OTPCONT_OTPCONFLOCK (16) +#define DA9058_OTPCONT_OTPGPLOCK (15) +#define DA9058_OTPCONT_OTPCONFG (13) +#define DA9058_OTPCONT_OTPGP (12
RE: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped
if you don't TOP POST how can you tell who wrote what? see my comments embedded below -Original Message- From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] Sent: 04 August 2012 11:54 To: Opensource [Anthony Olech] Cc: LKML; Anthony Olech; David Dajun Chen Subject: Re: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped On Wed, Aug 01, 2012 at 07:05:15PM +0100, Anthony Olech wrote: if the irq_base is set to -1 when calling regmap_add_irq_chip() then allow the IRQ to be mapped even if the allocated irq_base is actually zero. This restores the behaviour seen in v3.4, and I assume that the tidy-ups just made in v3.5 INADVERTENTLY introduce this change in behaviour. Please pay MORE attention to the changelog - obviously there's no problem mapping automatically allocated IRQs, there's only any effect if they happen to GET allocated at zero. That is the problem - they are allocated at zero, and hence my patch The only real issue I see with the current code is that if the user explicitly wants to statically allocate an IRQ range at zero they can't. I don't want to explicitly allocate at zero. The current intended behaviour is that we use a linear domain unless a positive IRQ base is specified, though we're not quite doing that right now as a transitional measure until drivers are updated. The fact remains that my patch enables the DA9058 driver to work in v3.5 The current da9052 driver usage seems to have quite a few problems, I do recall having to fix some problems that make me doubt if it ever worked well. Looking at the code now I see it's using hard coded references to absolute IRQ numbers which is an issue... It should be being converted to use regmap_irq_get_virq(). -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [lm-sensors] [NEW DRIVER V2 6/7] DA9058 HWMON driver
-Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 06 August 2012 18:40 To: Opensource [Anthony Olech] Cc: Guenter Roeck; Jean Delvare; Randy Dunlop; Mark Brown; David Dajun Chen; LKML; lm-sens...@lm-sensors.org Subject: Re: [lm-sensors] [NEW DRIVER V2 6/7] DA9058 HWMON driver On Sun, Aug 05, 2012 at 09:43:44PM +0100, Anthony Olech wrote: This is the HWMON component driver of the Dialog DA9058 PMIC. This driver is just one component of the whole DA9058 PMIC driver. It depends on the core DA9058 MFD driver. Signed-off-by: Anthony Olech anthony.olech.opensou...@diasemi.com Signed-off-by: David Dajun Chen david.c...@diasemi.com [ ... ] +static SENSOR_DEVICE_ATTR(vbat_mV, S_IRUGO, da9058_read_vbat, NULL, +0); static SENSOR_DEVICE_ATTR(adc_mV, S_IRUGO, da9058_read_misc_channel, NULL, + DA9058_ADCMAN_MUXSEL_ADCIN); +static SENSOR_DEVICE_ATTR(vfpin_mV, S_IRUGO, da9058_read_vfpin, NULL, +0); static SENSOR_DEVICE_ATTR(vfpin_mode, S_IRUGO, da9058_vfpin_mode, +NULL, 0); static SENSOR_DEVICE_ATTR(tbat_mV, S_IRUGO, +da9058_read_tbat, NULL, 0); static SENSOR_DEVICE_ATTR(tjunc_in, +S_IRUGO, da9058_read_tjunc, NULL, 0); static SENSOR_DEVICE_ATTR(adc_mode, S_IWUSR | S_IRUGO, da9058_get_adc_mode, + da9058_set_adc_mode, 0); Please use standard sysfs attribute names for temperature and voltage attributes. I could not find a naming convention, so I will try to abstract one from all the HWMON driver that have your name in them. I noted when searching that I missed out a file in also that Documentation/hwmon. I will correct both issues in my next submission attempt. For configuration (XXX_mode), please use devicetreee and/or platform data, not sysfs attributes. As far as I can see both devicetreee and platform data allow configuration data to be passed into the driver at probe time, they don't allow an operating mode to be changed dynamically. That is what I thought sysfs allowed. Thus your comments seem to imply that you do not want to allow the mode to be changed dynamically. If that is the case then I can remove the dynamic mode setting, leaving it fixed by platform data. Thanks, Tony Olech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped
-Original Message- From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] Sent: 07 August 2012 15:03 On Tue, Aug 07, 2012 at 11:18:20AM +, Opensource [Anthony Olech] wrote: if you don't TOP POST how can you tell who wrote what? Well, it's not clear who wrote what in your current e-mail since there's no indication of what's quoted and what's new text... Take a look at all the other mails on the list - your mail should be in a similar style to them. There's some advice for common e-mail clients in email-clients.txt under Documentation. I found the option to quote/indent the email original, sorry about that but we are forced to use Microsoft Outlook and the default were set up strangely. The bottom line here is that if your driver requires a dynamically allocated legacy domain it is broken. I am trying to use the latest REGMAP API, and I do not understand why you say the DSA9058 driver requires a dynamically allocated legacy domain. Surely the virtual IRQs that the PMIC component drivers use must be dynamically allocated. It is only the single GPIO line designated as an interrupt line in the machne drivert that is fixed by the hardware. That surely means the irq_base parameter to regmap_add_irq_chip() must be set to -1. What else could it be set to?? I am beginning to suspect that I have misunderstood something. The regmap-irq API seemed taylor-made for our PMIC with one real h/w interrupt line with several PMIC chip irq sources controlled by a set of registers that seemed to slot into the regmap_add_irq_chip struct perfectly. Why should that set of virtual irqs be given a specific base?? I really hope that you can help me clear this issue up, as there are not many examples of drivers that use the regmap-irq API in linux-release GIT repository. Tony Olech -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V1 5/7] DA9058 GPIO driver
Thanks Mark for looking at the DA9058 GPIO component driver.
I do realize that REGMAP does locking on individual register accesses,
however, the each GPIO line is controlled by 4-bits in a register, with
the meaning of the most significant bit depending on the GPIO direction,
so it is essential that the register be read first before do an update, thus
two sequential register accesses must be protected by a mutex to
prevent another process changing the register (and hence the meaning
of the most-significant bit) in the middle of the two accesses.
I hope this explains to your satisfaction why a driver mutex is required
in addition to the regmap's register access mutex
Tony Olech
-Original Message-
From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com]
Sent: 02 August 2012 11:20
To: Opensource [Anthony Olech]
Cc: LKML
Subject: Re: [NEW DRIVER V1 5/7] DA9058 GPIO driver
On Thu, Aug 02, 2012 at 09:48:57AM +0100, Anthony Olech wrote:
> + mutex_lock(>lock);
> + ret = da9058_reg_read(da9058, DA9058_STATUSC_REG, _level);
> + mutex_unlock(>lock);
regmap already does locking for you.
> + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, _cntrl);
> + if (ret)
> + goto exit;
> +
> + if (offset) {
> + gpio_cntrl &= ~0xF0;
> + gpio_cntrl |= 0xF0 & gpio->out_config;
> +
> + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
Just use regmap_update_bits().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V1 5/7] DA9058 GPIO driver
Thanks Mark for looking at the DA9058 GPIO component driver.
I do realize that REGMAP does locking on individual register accesses,
however, the each GPIO line is controlled by 4-bits in a register, with
the meaning of the most significant bit depending on the GPIO direction,
so it is essential that the register be read first before do an update, thus
two sequential register accesses must be protected by a mutex to
prevent another process changing the register (and hence the meaning
of the most-significant bit) in the middle of the two accesses.
I hope this explains to your satisfaction why a driver mutex is required
in addition to the regmap's register access mutex
Tony Olech
-Original Message-
From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com]
Sent: 02 August 2012 11:20
To: Opensource [Anthony Olech]
Cc: LKML
Subject: Re: [NEW DRIVER V1 5/7] DA9058 GPIO driver
On Thu, Aug 02, 2012 at 09:48:57AM +0100, Anthony Olech wrote:
+ mutex_lock(gpio-lock);
+ ret = da9058_reg_read(da9058, DA9058_STATUSC_REG, gpio_level);
+ mutex_unlock(gpio-lock);
regmap already does locking for you.
+ ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
+ if (ret)
+ goto exit;
+
+ if (offset) {
+ gpio_cntrl = ~0xF0;
+ gpio_cntrl |= 0xF0 gpio-out_config;
+
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
Just use regmap_update_bits().
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
