2012-05-13 01:00:12

by Richard Zhao

[permalink] [raw]
Subject: [PATCH 1/2] mfd: anatop: make register accessor more flexible and rename meaningfully

- rename to anatop_read_reg and anatop_write_reg
- anatop_read_reg directly return reg value
- anatop_write_reg write reg with mask

Signed-off-by: Richard Zhao <[email protected]>
---
drivers/mfd/anatop-mfd.c | 35 +++++++++++-----------------------
drivers/regulator/anatop-regulator.c | 18 ++++++++---------
include/linux/mfd/anatop.h | 4 ++--
3 files changed, 21 insertions(+), 36 deletions(-)

diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
index 2af4248..3bb7c49 100644
--- a/drivers/mfd/anatop-mfd.c
+++ b/drivers/mfd/anatop-mfd.c
@@ -41,39 +41,26 @@
#include <linux/of_address.h>
#include <linux/mfd/anatop.h>

-u32 anatop_get_bits(struct anatop *adata, u32 addr, int bit_shift,
- int bit_width)
+u32 anatop_read_reg(struct anatop *adata, u32 addr)
{
- u32 val, mask;
-
- if (bit_width == 32)
- mask = ~0;
- else
- mask = (1 << bit_width) - 1;
-
- val = readl(adata->ioreg + addr);
- val = (val >> bit_shift) & mask;
-
- return val;
+ return readl(adata->ioreg + addr);
}
-EXPORT_SYMBOL_GPL(anatop_get_bits);
+EXPORT_SYMBOL_GPL(anatop_read_reg);

-void anatop_set_bits(struct anatop *adata, u32 addr, int bit_shift,
- int bit_width, u32 data)
+void anatop_write_reg(struct anatop *adata, u32 addr, u32 data, u32 mask)
{
- u32 val, mask;
+ u32 val;

- if (bit_width == 32)
- mask = ~0;
- else
- mask = (1 << bit_width) - 1;
+ data &= mask;

spin_lock(&adata->reglock);
- val = readl(adata->ioreg + addr) & ~(mask << bit_shift);
- writel((data << bit_shift) | val, adata->ioreg + addr);
+ val = readl(adata->ioreg + addr);
+ val |= data;
+ val &= ~data;
+ writel(val, adata->ioreg + addr);
spin_unlock(&adata->reglock);
}
-EXPORT_SYMBOL_GPL(anatop_set_bits);
+EXPORT_SYMBOL_GPL(anatop_write_reg);

static const struct of_device_id of_anatop_match[] = {
{ .compatible = "fsl,imx6q-anatop", },
diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 81fd606..0a34085 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -47,7 +47,7 @@ static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
int max_uV, unsigned *selector)
{
struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
- u32 val, sel;
+ u32 val, sel, mask;
int uv;

uv = min_uV;
@@ -71,11 +71,10 @@ static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
val = anatop_reg->min_bit_val + sel;
*selector = sel;
dev_dbg(&reg->dev, "%s: calculated val %d\n", __func__, val);
- anatop_set_bits(anatop_reg->mfd,
- anatop_reg->control_reg,
- anatop_reg->vol_bit_shift,
- anatop_reg->vol_bit_width,
- val);
+ mask = ((1 << anatop_reg->vol_bit_width) - 1) <<
+ anatop_reg->vol_bit_shift;
+ val <<= anatop_reg->vol_bit_shift;
+ anatop_write_reg(anatop_reg->mfd, anatop_reg->control_reg, val, mask);

return 0;
}
@@ -88,10 +87,9 @@ static int anatop_get_voltage_sel(struct regulator_dev *reg)
if (!anatop_reg->control_reg)
return -ENOTSUPP;

- val = anatop_get_bits(anatop_reg->mfd,
- anatop_reg->control_reg,
- anatop_reg->vol_bit_shift,
- anatop_reg->vol_bit_width);
+ val = anatop_read_reg(anatop_reg->mfd, anatop_reg->control_reg);
+ val = (val & ((1 << anatop_reg->vol_bit_width) - 1)) >>
+ anatop_reg->vol_bit_shift;

return val - anatop_reg->min_bit_val;
}
diff --git a/include/linux/mfd/anatop.h b/include/linux/mfd/anatop.h
index 22c1007..7f92acf 100644
--- a/include/linux/mfd/anatop.h
+++ b/include/linux/mfd/anatop.h
@@ -34,7 +34,7 @@ struct anatop {
spinlock_t reglock;
};

-extern u32 anatop_get_bits(struct anatop *, u32, int, int);
-extern void anatop_set_bits(struct anatop *, u32, int, int, u32);
+extern u32 anatop_read_reg(struct anatop *, u32);
+extern void anatop_write_reg(struct anatop *, u32, u32, u32);

#endif /* __LINUX_MFD_ANATOP_H */
--
1.7.9.5


2012-05-13 01:00:39

by Richard Zhao

[permalink] [raw]
Subject: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

It makes anatop register access easier. Anatop has many misc registers,
which may not be a specific driver.

There's only one anatop device for a running system, so we use a global
variable to store struct anatop.

Signed-off-by: Richard Zhao <[email protected]>
---
drivers/mfd/anatop-mfd.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
index 3bb7c49..b0313210 100644
--- a/drivers/mfd/anatop-mfd.c
+++ b/drivers/mfd/anatop-mfd.c
@@ -41,8 +41,15 @@
#include <linux/of_address.h>
#include <linux/mfd/anatop.h>

+/* For any running system, there's only one anatop device. */
+static struct anatop *anatop_data;
+
u32 anatop_read_reg(struct anatop *adata, u32 addr)
{
+ BUG_ON(!anatop_data);
+ if (!adata)
+ adata = anatop_data;
+
return readl(adata->ioreg + addr);
}
EXPORT_SYMBOL_GPL(anatop_read_reg);
@@ -51,6 +58,10 @@ void anatop_write_reg(struct anatop *adata, u32 addr, u32 data, u32 mask)
{
u32 val;

+ BUG_ON(!anatop_data);
+ if (!adata)
+ adata = anatop_data;
+
data &= mask;

spin_lock(&adata->reglock);
@@ -83,6 +94,7 @@ static int __devinit of_anatop_probe(struct platform_device *pdev)
drvdata->ioreg = ioreg;
spin_lock_init(&drvdata->reglock);
platform_set_drvdata(pdev, drvdata);
+ anatop_data = drvdata;
of_platform_populate(np, of_anatop_match, NULL, dev);

return 0;
--
1.7.9.5

2012-05-13 01:18:33

by Richard Zhao

[permalink] [raw]
Subject: [PATCH V2] mfd: anatop: make register accessor more flexible and rename meaningfully

- rename to anatop_read_reg and anatop_write_reg
- anatop_read_reg directly return reg value
- anatop_write_reg write reg with mask

Signed-off-by: Richard Zhao <[email protected]>
---
Changes since V1:
correct bit operation in anatop_write_reg

drivers/mfd/anatop-mfd.c | 35 +++++++++++-----------------------
drivers/regulator/anatop-regulator.c | 18 ++++++++---------
include/linux/mfd/anatop.h | 4 ++--
3 files changed, 21 insertions(+), 36 deletions(-)

diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
index 2af4248..6da0634 100644
--- a/drivers/mfd/anatop-mfd.c
+++ b/drivers/mfd/anatop-mfd.c
@@ -41,39 +41,26 @@
#include <linux/of_address.h>
#include <linux/mfd/anatop.h>

-u32 anatop_get_bits(struct anatop *adata, u32 addr, int bit_shift,
- int bit_width)
+u32 anatop_read_reg(struct anatop *adata, u32 addr)
{
- u32 val, mask;
-
- if (bit_width == 32)
- mask = ~0;
- else
- mask = (1 << bit_width) - 1;
-
- val = readl(adata->ioreg + addr);
- val = (val >> bit_shift) & mask;
-
- return val;
+ return readl(adata->ioreg + addr);
}
-EXPORT_SYMBOL_GPL(anatop_get_bits);
+EXPORT_SYMBOL_GPL(anatop_read_reg);

-void anatop_set_bits(struct anatop *adata, u32 addr, int bit_shift,
- int bit_width, u32 data)
+void anatop_write_reg(struct anatop *adata, u32 addr, u32 data, u32 mask)
{
- u32 val, mask;
+ u32 val;

- if (bit_width == 32)
- mask = ~0;
- else
- mask = (1 << bit_width) - 1;
+ data &= mask;

spin_lock(&adata->reglock);
- val = readl(adata->ioreg + addr) & ~(mask << bit_shift);
- writel((data << bit_shift) | val, adata->ioreg + addr);
+ val = readl(adata->ioreg + addr);
+ val &= ~mask;
+ val |= data;
+ writel(val, adata->ioreg + addr);
spin_unlock(&adata->reglock);
}
-EXPORT_SYMBOL_GPL(anatop_set_bits);
+EXPORT_SYMBOL_GPL(anatop_write_reg);

static const struct of_device_id of_anatop_match[] = {
{ .compatible = "fsl,imx6q-anatop", },
diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 81fd606..0a34085 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -47,7 +47,7 @@ static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
int max_uV, unsigned *selector)
{
struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
- u32 val, sel;
+ u32 val, sel, mask;
int uv;

uv = min_uV;
@@ -71,11 +71,10 @@ static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
val = anatop_reg->min_bit_val + sel;
*selector = sel;
dev_dbg(&reg->dev, "%s: calculated val %d\n", __func__, val);
- anatop_set_bits(anatop_reg->mfd,
- anatop_reg->control_reg,
- anatop_reg->vol_bit_shift,
- anatop_reg->vol_bit_width,
- val);
+ mask = ((1 << anatop_reg->vol_bit_width) - 1) <<
+ anatop_reg->vol_bit_shift;
+ val <<= anatop_reg->vol_bit_shift;
+ anatop_write_reg(anatop_reg->mfd, anatop_reg->control_reg, val, mask);

return 0;
}
@@ -88,10 +87,9 @@ static int anatop_get_voltage_sel(struct regulator_dev *reg)
if (!anatop_reg->control_reg)
return -ENOTSUPP;

- val = anatop_get_bits(anatop_reg->mfd,
- anatop_reg->control_reg,
- anatop_reg->vol_bit_shift,
- anatop_reg->vol_bit_width);
+ val = anatop_read_reg(anatop_reg->mfd, anatop_reg->control_reg);
+ val = (val & ((1 << anatop_reg->vol_bit_width) - 1)) >>
+ anatop_reg->vol_bit_shift;

return val - anatop_reg->min_bit_val;
}
diff --git a/include/linux/mfd/anatop.h b/include/linux/mfd/anatop.h
index 22c1007..7f92acf 100644
--- a/include/linux/mfd/anatop.h
+++ b/include/linux/mfd/anatop.h
@@ -34,7 +34,7 @@ struct anatop {
spinlock_t reglock;
};

-extern u32 anatop_get_bits(struct anatop *, u32, int, int);
-extern void anatop_set_bits(struct anatop *, u32, int, int, u32);
+extern u32 anatop_read_reg(struct anatop *, u32);
+extern void anatop_write_reg(struct anatop *, u32, u32, u32);

#endif /* __LINUX_MFD_ANATOP_H */
--
1.7.9.5

2012-05-13 17:48:35

by Ying-Chun Liu (PaulLiu)

[permalink] [raw]
Subject: Re: [PATCH V2] mfd: anatop: make register accessor more flexible and rename meaningfully

(2012年05月13日 09:18), Richard Zhao wrote:
> - rename to anatop_read_reg and anatop_write_reg
> - anatop_read_reg directly return reg value
> - anatop_write_reg write reg with mask
>
> Signed-off-by: Richard Zhao <[email protected]>
> ---
> Changes since V1:
> correct bit operation in anatop_write_reg
>
> drivers/mfd/anatop-mfd.c | 35 +++++++++++-----------------------
> drivers/regulator/anatop-regulator.c | 18 ++++++++---------
> include/linux/mfd/anatop.h | 4 ++--
> 3 files changed, 21 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
> index 2af4248..6da0634 100644
> --- a/drivers/mfd/anatop-mfd.c
> +++ b/drivers/mfd/anatop-mfd.c
> @@ -41,39 +41,26 @@
> #include <linux/of_address.h>
> #include <linux/mfd/anatop.h>
>
> -u32 anatop_get_bits(struct anatop *adata, u32 addr, int bit_shift,
> - int bit_width)
> +u32 anatop_read_reg(struct anatop *adata, u32 addr)
> {
> - u32 val, mask;
> -
> - if (bit_width == 32)
> - mask = ~0;
> - else
> - mask = (1 << bit_width) - 1;
> -
> - val = readl(adata->ioreg + addr);
> - val = (val >> bit_shift) & mask;
> -
> - return val;
> + return readl(adata->ioreg + addr);
> }
> -EXPORT_SYMBOL_GPL(anatop_get_bits);
> +EXPORT_SYMBOL_GPL(anatop_read_reg);
>
> -void anatop_set_bits(struct anatop *adata, u32 addr, int bit_shift,
> - int bit_width, u32 data)
> +void anatop_write_reg(struct anatop *adata, u32 addr, u32 data, u32 mask)
> {
> - u32 val, mask;
> + u32 val;
>
> - if (bit_width == 32)
> - mask = ~0;
> - else
> - mask = (1 << bit_width) - 1;
> + data &= mask;
>
> spin_lock(&adata->reglock);
> - val = readl(adata->ioreg + addr) & ~(mask << bit_shift);
> - writel((data << bit_shift) | val, adata->ioreg + addr);
> + val = readl(adata->ioreg + addr);
> + val &= ~mask;
> + val |= data;
> + writel(val, adata->ioreg + addr);
> spin_unlock(&adata->reglock);
> }
> -EXPORT_SYMBOL_GPL(anatop_set_bits);
> +EXPORT_SYMBOL_GPL(anatop_write_reg);
>
> static const struct of_device_id of_anatop_match[] = {
> { .compatible = "fsl,imx6q-anatop", },
> diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
> index 81fd606..0a34085 100644
> --- a/drivers/regulator/anatop-regulator.c
> +++ b/drivers/regulator/anatop-regulator.c
> @@ -47,7 +47,7 @@ static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
> int max_uV, unsigned *selector)
> {
> struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
> - u32 val, sel;
> + u32 val, sel, mask;
> int uv;
>
> uv = min_uV;
> @@ -71,11 +71,10 @@ static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
> val = anatop_reg->min_bit_val + sel;
> *selector = sel;
> dev_dbg(&reg->dev, "%s: calculated val %d\n", __func__, val);
> - anatop_set_bits(anatop_reg->mfd,
> - anatop_reg->control_reg,
> - anatop_reg->vol_bit_shift,
> - anatop_reg->vol_bit_width,
> - val);
> + mask = ((1 << anatop_reg->vol_bit_width) - 1) <<
> + anatop_reg->vol_bit_shift;
> + val <<= anatop_reg->vol_bit_shift;
> + anatop_write_reg(anatop_reg->mfd, anatop_reg->control_reg, val, mask);
>
> return 0;
> }
> @@ -88,10 +87,9 @@ static int anatop_get_voltage_sel(struct regulator_dev *reg)
> if (!anatop_reg->control_reg)
> return -ENOTSUPP;
>
> - val = anatop_get_bits(anatop_reg->mfd,
> - anatop_reg->control_reg,
> - anatop_reg->vol_bit_shift,
> - anatop_reg->vol_bit_width);
> + val = anatop_read_reg(anatop_reg->mfd, anatop_reg->control_reg);
> + val = (val & ((1 << anatop_reg->vol_bit_width) - 1)) >>
> + anatop_reg->vol_bit_shift;
>
> return val - anatop_reg->min_bit_val;
> }
> diff --git a/include/linux/mfd/anatop.h b/include/linux/mfd/anatop.h
> index 22c1007..7f92acf 100644
> --- a/include/linux/mfd/anatop.h
> +++ b/include/linux/mfd/anatop.h
> @@ -34,7 +34,7 @@ struct anatop {
> spinlock_t reglock;
> };
>
> -extern u32 anatop_get_bits(struct anatop *, u32, int, int);
> -extern void anatop_set_bits(struct anatop *, u32, int, int, u32);
> +extern u32 anatop_read_reg(struct anatop *, u32);
> +extern void anatop_write_reg(struct anatop *, u32, u32, u32);
>
> #endif /* __LINUX_MFD_ANATOP_H */

Reviewed-by: Ying-Chun Liu (PaulLiu) <[email protected]>

Thanks...

2012-05-14 03:32:22

by Shawn Guo

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

On Sun, May 13, 2012 at 08:59:54AM +0800, Richard Zhao wrote:
> It makes anatop register access easier. Anatop has many misc registers,
> which may not be a specific driver.
>
> There's only one anatop device for a running system, so we use a global
> variable to store struct anatop.
>
>From what I see, it's reasonable. Then the immediate question I have
is, should we remove "struct anatop *adata" from anatop_read_reg and
anatop_write_reg completely?

Regards,
Shawn

> Signed-off-by: Richard Zhao <[email protected]>
> ---
> drivers/mfd/anatop-mfd.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
> index 3bb7c49..b0313210 100644
> --- a/drivers/mfd/anatop-mfd.c
> +++ b/drivers/mfd/anatop-mfd.c
> @@ -41,8 +41,15 @@
> #include <linux/of_address.h>
> #include <linux/mfd/anatop.h>
>
> +/* For any running system, there's only one anatop device. */
> +static struct anatop *anatop_data;
> +
> u32 anatop_read_reg(struct anatop *adata, u32 addr)
> {
> + BUG_ON(!anatop_data);
> + if (!adata)
> + adata = anatop_data;
> +
> return readl(adata->ioreg + addr);
> }
> EXPORT_SYMBOL_GPL(anatop_read_reg);
> @@ -51,6 +58,10 @@ void anatop_write_reg(struct anatop *adata, u32 addr, u32 data, u32 mask)
> {
> u32 val;
>
> + BUG_ON(!anatop_data);
> + if (!adata)
> + adata = anatop_data;
> +
> data &= mask;
>
> spin_lock(&adata->reglock);
> @@ -83,6 +94,7 @@ static int __devinit of_anatop_probe(struct platform_device *pdev)
> drvdata->ioreg = ioreg;
> spin_lock_init(&drvdata->reglock);
> platform_set_drvdata(pdev, drvdata);
> + anatop_data = drvdata;
> of_platform_populate(np, of_anatop_match, NULL, dev);
>
> return 0;
> --
> 1.7.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

2012-05-14 08:08:44

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

On Mon, May 14, 2012 at 11:51:38AM +0800, Shawn Guo wrote:

> From what I see, it's reasonable. Then the immediate question I have
> is, should we remove "struct anatop *adata" from anatop_read_reg and
> anatop_write_reg completely?

Given the way these things tend to go it's probably guaranteeing that
your next round of SoCs will have two register compatible anatop blocks :)


Attachments:
(No filename) (380.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2012-05-14 08:29:15

by Shawn Guo

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

On Mon, May 14, 2012 at 09:08:36AM +0100, Mark Brown wrote:
> On Mon, May 14, 2012 at 11:51:38AM +0800, Shawn Guo wrote:
>
> > From what I see, it's reasonable. Then the immediate question I have
> > is, should we remove "struct anatop *adata" from anatop_read_reg and
> > anatop_write_reg completely?
>
> Given the way these things tend to go it's probably guaranteeing that
> your next round of SoCs will have two register compatible anatop blocks :)

Considering anatop block tends to be a container of misc hardware
control bits, I haven't really seen any possibility that the future
SoCs will have multiple anatop blocks.

--
Regards,
Shawn

2012-05-14 09:01:15

by Ying-Chun Liu (PaulLiu)

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

(2012年05月14日 16:48), Shawn Guo wrote:
> On Mon, May 14, 2012 at 09:08:36AM +0100, Mark Brown wrote:
>> On Mon, May 14, 2012 at 11:51:38AM +0800, Shawn Guo wrote:
>>
>>> From what I see, it's reasonable. Then the immediate question I have
>>> is, should we remove "struct anatop *adata" from anatop_read_reg and
>>> anatop_write_reg completely?
>>
>> Given the way these things tend to go it's probably guaranteeing that
>> your next round of SoCs will have two register compatible anatop blocks :)
>
> Considering anatop block tends to be a container of misc hardware
> control bits, I haven't really seen any possibility that the future
> SoCs will have multiple anatop blocks.
>

Hi Shawn,

I think what the concern is we probably don't want several
non-continuous memory blocks of misc hardwares.
If we look into the current registers in anatop, it is really sparse.
Several regulators are using non-continuous address and the thermals are
also using different addresses. If the addresses are continuous then we
don't need the mfd driver.

We've already told Lily Zhang from Freescale and she promises to report
this problem to some inner team.

Yours Sincerely,
Paul

2012-05-14 09:24:42

by Shawn Guo

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

On Mon, May 14, 2012 at 05:01:08PM +0800, Ying-Chun Liu (PaulLiu) wrote:
> I think what the concern is we probably don't want several
> non-continuous memory blocks of misc hardwares.
> If we look into the current registers in anatop, it is really sparse.
> Several regulators are using non-continuous address and the thermals are
> also using different addresses. If the addresses are continuous then we
> don't need the mfd driver.
>
I do not quite follow that. The reason we need mfd driver isn't because
we do not want to both regulator and thermal drivers to map and access
the same address on their own which may have synchronization issue?

--
Regards,
Shawn

2012-05-14 13:27:04

by Ying-Chun Liu (PaulLiu)

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

(2012年05月14日 17:43), Shawn Guo wrote:
> On Mon, May 14, 2012 at 05:01:08PM +0800, Ying-Chun Liu (PaulLiu) wrote:
>> I think what the concern is we probably don't want several
>> non-continuous memory blocks of misc hardwares.
>> If we look into the current registers in anatop, it is really sparse.
>> Several regulators are using non-continuous address and the thermals are
>> also using different addresses. If the addresses are continuous then we
>> don't need the mfd driver.
>>
> I do not quite follow that. The reason we need mfd driver isn't because
> we do not want to both regulator and thermal drivers to map and access
> the same address on their own which may have synchronization issue?
>

Not sure about the synchronization issue. But currently thermal driver
in Linaro kernel do map and access the same address on its own now. It
is not a device driver yet and just access the address directly and
work. It seems to me that each different type of misc devices in Anatop
just work alone.

So let's go back to the patch. Why do we need this modification? Anatop
thermal driver can be written as a device driver and don't need this
patch. And we might get benefits when thermal driver written in this
way. Especially some boards do not have a correct fuse data. Any real
use cases of this patch?

Yours Sincerely,
Paul

2012-05-14 13:50:51

by Richard Zhao

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

On Mon, May 14, 2012 at 09:26:57PM +0800, Ying-Chun Liu (PaulLiu) wrote:
> (2012年05月14日 17:43), Shawn Guo wrote:
> > On Mon, May 14, 2012 at 05:01:08PM +0800, Ying-Chun Liu (PaulLiu) wrote:
> >> I think what the concern is we probably don't want several
> >> non-continuous memory blocks of misc hardwares.
> >> If we look into the current registers in anatop, it is really sparse.
> >> Several regulators are using non-continuous address and the thermals are
> >> also using different addresses. If the addresses are continuous then we
> >> don't need the mfd driver.
> >>
> > I do not quite follow that. The reason we need mfd driver isn't because
> > we do not want to both regulator and thermal drivers to map and access
> > the same address on their own which may have synchronization issue?
> >
>
> Not sure about the synchronization issue. But currently thermal driver
> in Linaro kernel do map and access the same address on its own now. It
> is not a device driver yet and just access the address directly and
> work. It seems to me that each different type of misc devices in Anatop
> just work alone.
>
> So let's go back to the patch. Why do we need this modification? Anatop
> thermal driver can be written as a device driver and don't need this
> patch. And we might get benefits when thermal driver written in this
> way. Especially some boards do not have a correct fuse data. Any real
> use cases of this patch?
Some bits in anatop is not owned by any driver.
It's for below code:
/* Some phy and power's special controls for host1
* 1. The external charger detector needs to be disabled
* or the signal at DP will be poor
* 2. The PLL's power and output to usb for host 1
* is totally controlled by IC, so the Software only needs
* to enable them at initializtion.
*/

anatop_write_reg(NULL, HW_ANADIG_USB2_CHRG_DETECT,
BM_ANADIG_USB2_CHRG_DETECT_EN_B |
BM_ANADIG_USB2_CHRG_DETECT_CHK_CHRG_B,
~0);
anatop_write_reg(NULL, HW_ANADIG_USB2_PLL_480_CTRL, 0,
BM_ANADIG_USB2_PLL_480_CTRL_BYPASS);

val = BM_ANADIG_USB2_PLL_480_CTRL_ENABLE |
BM_ANADIG_USB2_PLL_480_CTRL_POWER |
BM_ANADIG_USB2_PLL_480_CTRL_EN_USB_CLKS;
anatop_write_reg(NULL, HW_ANADIG_USB2_PLL_480_CTRL, val, val);

Thanks
Richard
>
> Yours Sincerely,
> Paul
>

2012-05-18 09:00:02

by Samuel Ortiz

[permalink] [raw]
Subject: Re: [PATCH V2] mfd: anatop: make register accessor more flexible and rename meaningfully

Hi Richard,

On Sun, May 13, 2012 at 09:18:02AM +0800, Richard Zhao wrote:
> - rename to anatop_read_reg and anatop_write_reg
> - anatop_read_reg directly return reg value
> - anatop_write_reg write reg with mask
Patch applied, thanks.

Cheers,
Samuel.

--
Intel Open Source Technology Centre
http://oss.intel.com/

2012-05-18 09:59:44

by Richard Zhao

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

On Mon, May 14, 2012 at 09:50:36PM +0800, Richard Zhao wrote:
> On Mon, May 14, 2012 at 09:26:57PM +0800, Ying-Chun Liu (PaulLiu) wrote:
> > (2012年05月14日 17:43), Shawn Guo wrote:
> > > On Mon, May 14, 2012 at 05:01:08PM +0800, Ying-Chun Liu (PaulLiu) wrote:
> > >> I think what the concern is we probably don't want several
> > >> non-continuous memory blocks of misc hardwares.
> > >> If we look into the current registers in anatop, it is really sparse.
> > >> Several regulators are using non-continuous address and the thermals are
> > >> also using different addresses. If the addresses are continuous then we
> > >> don't need the mfd driver.
> > >>
> > > I do not quite follow that. The reason we need mfd driver isn't because
> > > we do not want to both regulator and thermal drivers to map and access
> > > the same address on their own which may have synchronization issue?
> > >
> >
> > Not sure about the synchronization issue. But currently thermal driver
> > in Linaro kernel do map and access the same address on its own now. It
> > is not a device driver yet and just access the address directly and
> > work. It seems to me that each different type of misc devices in Anatop
> > just work alone.
Guys, we need to draw conclusion.
I think current patch keep some how compatible with multi-anatop.
It's ok.

Thanks
Richard
> >
> > So let's go back to the patch. Why do we need this modification? Anatop
> > thermal driver can be written as a device driver and don't need this
> > patch. And we might get benefits when thermal driver written in this
> > way. Especially some boards do not have a correct fuse data. Any real
> > use cases of this patch?
> Some bits in anatop is not owned by any driver.
> It's for below code:
> /* Some phy and power's special controls for host1
> * 1. The external charger detector needs to be disabled
> * or the signal at DP will be poor
> * 2. The PLL's power and output to usb for host 1
> * is totally controlled by IC, so the Software only needs
> * to enable them at initializtion.
> */
>
> anatop_write_reg(NULL, HW_ANADIG_USB2_CHRG_DETECT,
> BM_ANADIG_USB2_CHRG_DETECT_EN_B |
> BM_ANADIG_USB2_CHRG_DETECT_CHK_CHRG_B,
> ~0);
> anatop_write_reg(NULL, HW_ANADIG_USB2_PLL_480_CTRL, 0,
> BM_ANADIG_USB2_PLL_480_CTRL_BYPASS);
>
> val = BM_ANADIG_USB2_PLL_480_CTRL_ENABLE |
> BM_ANADIG_USB2_PLL_480_CTRL_POWER |
> BM_ANADIG_USB2_PLL_480_CTRL_EN_USB_CLKS;
> anatop_write_reg(NULL, HW_ANADIG_USB2_PLL_480_CTRL, val, val);
>
> Thanks
> Richard
> >
> > Yours Sincerely,
> > Paul
> >
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2012-05-21 03:54:23

by Shawn Guo

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

On Fri, May 18, 2012 at 05:59:32PM +0800, Richard Zhao wrote:
> Guys, we need to draw conclusion.
> I think current patch keep some how compatible with multi-anatop.
> It's ok.
>
I'm fine with that.

--
Regards,
Shawn

2012-05-21 09:28:04

by Ying-Chun Liu (PaulLiu)

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

(2012年05月21日 12:13), Shawn Guo wrote:
> On Fri, May 18, 2012 at 05:59:32PM +0800, Richard Zhao wrote:
>> Guys, we need to draw conclusion.
>> I think current patch keep some how compatible with multi-anatop.
>> It's ok.
>>
> I'm fine with that.
>
Yes. It is good for me.

2012-05-21 09:39:55

by Richard Zhao

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

On Mon, May 21, 2012 at 05:27:54PM +0800, Ying-Chun Liu (PaulLiu) wrote:
> (2012年05月21日 12:13), Shawn Guo wrote:
> > On Fri, May 18, 2012 at 05:59:32PM +0800, Richard Zhao wrote:
> >> Guys, we need to draw conclusion.
> >> I think current patch keep some how compatible with multi-anatop.
> >> It's ok.
> >>
> > I'm fine with that.
> >
> Yes. It is good for me.
>
So, Sameo, could you pick the patch.

Thanks
Richard

2012-06-18 20:24:29

by Rob Lee

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

Hi,

What is the status of this patch?

Thanks,
Rob

On Mon, May 21, 2012 at 4:39 AM, Richard Zhao
<[email protected]> wrote:
> On Mon, May 21, 2012 at 05:27:54PM +0800, Ying-Chun Liu (PaulLiu) wrote:
>> (2012年05月21日 12:13), Shawn Guo wrote:
>> > On Fri, May 18, 2012 at 05:59:32PM +0800, Richard Zhao wrote:
>> >> Guys, we need to draw conclusion.
>> >> I think current patch keep some how compatible with multi-anatop.
>> >> It's ok.
>> >>
>> > I'm fine with that.
>> >
>> Yes. It is good for me.
>>
> So, Sameo, could you pick the patch.
>
> Thanks
> Richard
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

2012-06-18 21:48:00

by Samuel Ortiz

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

Hi Rob,

On Mon, Jun 18, 2012 at 03:23:45PM -0500, Rob Lee wrote:
> Hi,
>
> What is the status of this patch?
I'm swamped with other things to look at right now, but I expect to clean my
MFD patches backlog at the beginning of next week.

Cheers,
Samuel.

--
Intel Open Source Technology Centre
http://oss.intel.com/

2012-06-18 23:00:14

by Rob Lee

[permalink] [raw]
Subject: Re: [PATCH 2/2] mfd: anatop: permit adata be NULL when access register

Hey Samuel, thanks for your response.

On Mon, Jun 18, 2012 at 4:58 PM, Samuel Ortiz <[email protected]> wrote:
> Hi Rob,
>
> On Mon, Jun 18, 2012 at 03:23:45PM -0500, Rob Lee wrote:
>> Hi,
>>
>> What is the status of this patch?
> I'm swamped with other things to look at right now, but I expect to clean my
> MFD patches backlog at the beginning of next week.

Fyi, I also saw this patch was resent (and acked) here:
https://lkml.org/lkml/2012/6/3/159

Thanks,
Rob

>
> Cheers,
> Samuel.
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/