Re: [PATCH] i2c: allow building emev2 without slave mode again
On Thursday 17 December 2015 20:40:17 Wolfram Sang wrote: > > > My conclusion for now is: > > > > > > There needs something to be done surely, but currently I don't have the > > > bandwidth to do it or even play around with it. I am not fully happy > > > with your patches as well because __maybe_unused has some kind of "last > > > resort" feeling to me. > > > > I generally like __maybe_unused, but it's a matter of personal preference. > > We could avoid the __maybe_unused if the reg_slave/unreg_slave callback > > pointers are always available in struct i2c_algorithm. > > Yes, I was thinking in this direction, looking at how PM does it. Needs > some playing around, though. I think PM gets it slightly wrong, the way you have to use #ifdef leads to subtle bugs all the time, and I actually have a patch that converts a few dozen drivers to use __maybe_unused to shut up build warnings and errors. What you can do though is to use a reference like #define __i2c_slave_ptr(x) (IS_ENABLED(CONFIG_I2C_SLAVE) ? (x) : NULL) ... .reg_slave = __i2c_slave_ptr(em_i2c_reg_slave), .unreg_slave = __i2c_slave_ptr(em_i2c_unreg_slave), ... This has the same effect as the __maybe_unused annotation. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: allow building emev2 without slave mode again
On Thursday 17 December 2015 13:01:57 Wolfram Sang wrote:
> On Mon, Dec 14, 2015 at 11:27:22PM +0100, Arnd Bergmann wrote:
> > On Monday 14 December 2015 14:52:06 Wolfram Sang wrote:
> > > > > What about not ifdeffing the inline function and keep the build error
> > > > > whenever someone uses it without I2C_SLAVE being selected?
> > > >
> > > > The inline function is only added there for the case that I2C_SLAVE is
> > > > disabled, so that would be pointless.
> > > >
> > > > However, what we could do is move the extern declaration outside of
> > > > the #ifdef to make it always visible. The
> > > > if(IS_ENABLED(CONFIG_I2C_SLAVE))
> > > > check should then ensure that it never actually gets called, and we
> > > > get a link error if some driver gets it wrong.
> > >
> > > Yes, that's what I meant: move the whole function (as it was before your
> > > patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
> > > even, because for !I2C_SLAVE, the client struct will not have the
> > > slave_cb member.
> > >
> >
> > But we don't want a compile-error for randconfig builds, and we don't
> > want unnecessary #ifdef in the driver.
>
> My conclusion for now is:
>
> There needs something to be done surely, but currently I don't have the
> bandwidth to do it or even play around with it. I am not fully happy
> with your patches as well because __maybe_unused has some kind of "last
> resort" feeling to me.
I generally like __maybe_unused, but it's a matter of personal preference.
We could avoid the __maybe_unused if the reg_slave/unreg_slave callback
pointers are always available in struct i2c_algorithm.
> So, to get the build failures away immediately I'd simply submit a patch
> for the emev driver to select I2C_SLAVE and postpone the proper fix to
> later.
>
> That being said, thanks a lot for your input. I will surely come back to it.
Just for reference, see below for my combined patch, in case you decide
to use that at a later point.
Arnd
8<---
Subject: [PATCH] i2c: emev2 depends on i2c slave mode
The emev2 driver stopped compiling in today's linux-next kernel:
drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't
known
drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function
'i2c_slave_event' [-Werror=implicit-function-declaration]
drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared
(first use in this function)
It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
to add a dependency on that symbol:
* The symbol is user-selectable, but only one or two (including this
one) bus drivers actually implement it, and it makes no sense
if you don't have one of them.
* The other driver (R-Car) uses 'select I2C_SLAVE', which seems
reasonable in principle, but we should not do that on user
visible symbols.
* I2C slave mode could be implemented in a lot of other drivers
as an optional feature, but we shouldn't require enabling it
if we don't use it.
This changes the two drivers that provide I2C slave mode so they
can again build if the slave mode being disabled. To do this, I
move the definition of i2c_slave_event() and enum i2c_slave_event
out of the #ifdef and instead make the assignment of the reg_slave
and unreg_slave pointers optional in the bus drivers. The functions
implementing the feature are unused in that case, so they get
marked as __maybe_unused in order to still give compile-time
coverage.
Signed-off-by: Arnd Bergmann
Fixes: c31d0a00021d ("i2c: emev2: add slave support")
---
drivers/i2c/busses/Kconfig | 1 -
drivers/i2c/busses/i2c-emev2.c | 8 +---
drivers/i2c/busses/i2c-rcar.c | 8 +---
include/linux/i2c.h| 4 +++-
4 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index eaa7b4a0e484..f205b9fa7a74 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -985,7 +985,6 @@ config I2C_XLP9XX
config I2C_RCAR
tristate "Renesas R-Car I2C Controller"
depends on ARCH_SHMOBILE || COMPILE_TEST
- select I2C_SLAVE
help
If you say yes to this option, support will be included for the
R-Car I2C controller.
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 96bb4e749012..b01c9f30c545 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -303,7 +303,7 @@ static irqreturn_t em_i2c_irq_handler(int this_irq, vo
Re: [PATCH] i2c: allow building emev2 without slave mode again
On Monday 14 December 2015 14:52:06 Wolfram Sang wrote:
> > > What about not ifdeffing the inline function and keep the build error
> > > whenever someone uses it without I2C_SLAVE being selected?
> >
> > The inline function is only added there for the case that I2C_SLAVE is
> > disabled, so that would be pointless.
> >
> > However, what we could do is move the extern declaration outside of
> > the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE))
> > check should then ensure that it never actually gets called, and we
> > get a link error if some driver gets it wrong.
>
> Yes, that's what I meant: move the whole function (as it was before your
> patch) out of the CONFIG_I2C_SLAVE block. We should get a compiler error
> even, because for !I2C_SLAVE, the client struct will not have the
> slave_cb member.
>
But we don't want a compile-error for randconfig builds, and we don't
want unnecessary #ifdef in the driver.
This change on top of my earlier patch should do what I meant:
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 0236e5f2b5be..536641bad92d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -265,15 +265,15 @@ enum i2c_slave_event {
extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t
slave_cb);
extern int i2c_slave_unregister(struct i2c_client *client);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)
{
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
return client->slave_cb(client, event, val);
+}
#else
- return 0;
+extern int i2c_slave_event(struct i2c_client *client, enum i2c_slave_event
event, u8 *val);
#endif
-}
/**
* struct i2c_board_info - template for device creation
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: allow building emev2 without slave mode again
On Sunday 13 December 2015 10:09:59 Wolfram Sang wrote: > > What about not ifdeffing the inline function and keep the build error > whenever someone uses it without I2C_SLAVE being selected? The inline function is only added there for the case that I2C_SLAVE is disabled, so that would be pointless. However, what we could do is move the extern declaration outside of the #ifdef to make it always visible. The if(IS_ENABLED(CONFIG_I2C_SLAVE)) check should then ensure that it never actually gets called, and we get a link error if some driver gets it wrong. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: allow building emev2 without slave mode again
On Saturday 12 December 2015 17:20:57 Wolfram Sang wrote: > Hi Arnd, > > thanks for looking into this, but I don't get your point yet. > > > The slave_cb callback function is supposed to set the 'value' > > here, > > Only if a master wants to READ from us. Right, and can this never fail? > > but it might return an error not assign the pointer, > > An error is only returned if a WRITE from a master was not accepted by > the slave backend. > > > It might be best to change the callback to return 'void' and not > > allow it to fail. > > We need that because in case of an errno, the slave should send NACK to > the master instead of ACK. > > > At least the eeprom slave cannot fail anyway, and it is the only > > implementation we have at the moment. > > True. But giving a slave the possibility to NACK a write should be > present IMO. Ok, fair enough. > > Alternatively, the inline could return an error, and both bus > > drivers check for the error before using 'value'. > > Hum, it does return an error? > > return client->slave_cb(client, event, val); > > You probably mean something else? I mean specifically this code in em_i2c_slave_irq(): /* Send data */ event = status & I2C_BIT_STD0 ? I2C_SLAVE_READ_REQUESTED : I2C_SLAVE_READ_PROCESSED; i2c_slave_event(priv->slave, event, &value); writeb(value, priv->base + I2C_OFS_IIC0); With my current code to turn i2c_slave_event() into an empty inline function in case of !CONFIG_I2C_SLAVE, the compiler knows that "value" is uninitialized at the point where we write it to the register, and warns about it. The code will of course never run if slave mode is not allowed, but we should still shut up the warning, either by making the inline i2c_slave_event set 'value' to zero or 0xff, or by adding an error check: ret = i2c_slave_event(priv->slave, event, &value); if (!ret) writeb(value, priv->base + I2C_OFS_IIC0); and making the empty i2c_slave_event() function return -ENOSYS or -ENOTSUPP. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: allow building emev2 without slave mode again
On Thursday 10 December 2015 22:54:25 kbuild test robot wrote:
>
>In file included from arch/x86/include/asm/realmode.h:5:0,
> from arch/x86/include/asm/acpi.h:33,
> from arch/x86/include/asm/fixmap.h:19,
> from arch/x86/include/asm/apic.h:12,
> from arch/x86/include/asm/smp.h:12,
> from arch/x86/include/asm/mmzone_64.h:10,
> from arch/x86/include/asm/mmzone.h:4,
> from include/linux/mmzone.h:856,
> from include/linux/gfp.h:5,
> from include/linux/device.h:29,
> from drivers/i2c/busses/i2c-emev2.c:15:
>drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_irq_handler':
> >> arch/x86/include/asm/io.h:53:3: warning: 'value' may be used uninitialized
> >> in this function [-Wmaybe-uninitialized]
> { asm volatile("mov" size " %0,%1": :reg (val), \
> ^
>drivers/i2c/busses/i2c-emev2.c:232:13: note: 'value' was declared here
> u8 status, value;
> ^
The warning was indeed introduced by my change, but I think there
was a preexisting issue:
The slave_cb callback function is supposed to set the 'value'
here, but it might return an error not assign the pointer, which
is something that both the rcar and the emev2 drivers do not handle
correctly.
It might be best to change the callback to return 'void' and not
allow it to fail. At least the eeprom slave cannot fail anyway,
and it is the only implementation we have at the moment.
The inline function would then have to be changed to initialize
the 'value'.
Alternatively, the inline could return an error, and both bus
drivers check for the error before using 'value'.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: allow building emev2 without slave mode again
On Thursday 10 December 2015 14:34:46 Wolfram Sang wrote:
> On Thu, Dec 10, 2015 at 02:14:49PM +0100, Arnd Bergmann wrote:
> > The emev2 driver stopped compiling in today's linux-next kernel:
> >
> > drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
> > drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't
> > known
> > drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of
> > function 'i2c_slave_event' [-Werror=implicit-function-declaration]
> > drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared
> > (first use in this function)
> >
> > It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
> > to add a dependency on that symbol:
> >
> > * The symbol is user-selectable, but only one or two (including this
> > one) bus drivers actually implement it, and it makes no sense
> > if you don't have one of them.
> >
> > * The other driver (R-Car) uses 'select I2C_SLAVE', which seems
> > reasonable in principle, but we should not do that on user
> > visible symbols.
> >
> > * I2C slave mode could be implemented in a lot of other drivers
> > as an optional feature, but we shouldn't require enabling it
> > if we don't use it.
> >
> > This changes the two drivers that provide I2C slave mode so they
> > can again build if the slave mode being disabled. To do this, I
> > move the definition of i2c_slave_event() and enum i2c_slave_event
> > out of the #ifdef and instead make the assignment of the reg_slave
> > and unreg_slave pointers optional in the bus drivers. The functions
> > implementing the feature are unused in that case, so they get
> > marked as __maybe_unused in order to still give compile-time
> > coverage.
>
> Thanks a lot! Making this clear and consistent was on my todo-list,
> unfortunately below some other items.
>
> Both drivers have quite orthogonal slave_irq routines. What do you think
> about grouping this and the reg/unreg-calls together and compile them
> conditionally on I2C_SLAVE? I think the code savings are worth it.
Ah, I had missed that part, good point. The change below should take
care of leaving out the extra code here. Do you want to just fold that
in?
Arnd
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 75d6095c5fe1..b01c9f30c545 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -303,7 +303,7 @@ static irqreturn_t em_i2c_irq_handler(int this_irq, void
*dev_id)
{
struct em_i2c_device *priv = dev_id;
- if (em_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && em_i2c_slave_irq(priv))
return IRQ_HANDLED;
complete(&priv->msg_done);
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index e67824adeba0..a1ea66d6267e 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -428,7 +428,7 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
/* Only handle interrupts that are currently enabled */
msr &= rcar_i2c_read(priv, ICMIER);
if (!msr) {
- if (rcar_i2c_slave_irq(priv))
+ if (IS_ENABLED(CONFIG_I2C_SLAVE) && rcar_i2c_slave_irq(priv))
return IRQ_HANDLED;
return IRQ_NONE;
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: allow building emev2 without slave mode again
The emev2 driver stopped compiling in today's linux-next kernel:
drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't
known
drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function
'i2c_slave_event' [-Werror=implicit-function-declaration]
drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared
(first use in this function)
It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
to add a dependency on that symbol:
* The symbol is user-selectable, but only one or two (including this
one) bus drivers actually implement it, and it makes no sense
if you don't have one of them.
* The other driver (R-Car) uses 'select I2C_SLAVE', which seems
reasonable in principle, but we should not do that on user
visible symbols.
* I2C slave mode could be implemented in a lot of other drivers
as an optional feature, but we shouldn't require enabling it
if we don't use it.
This changes the two drivers that provide I2C slave mode so they
can again build if the slave mode being disabled. To do this, I
move the definition of i2c_slave_event() and enum i2c_slave_event
out of the #ifdef and instead make the assignment of the reg_slave
and unreg_slave pointers optional in the bus drivers. The functions
implementing the feature are unused in that case, so they get
marked as __maybe_unused in order to still give compile-time
coverage.
Signed-off-by: Arnd Bergmann
Fixes: c31d0a00021d ("i2c: emev2: add slave support")
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 69c46fe13777..1c8d53f34dd3 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -985,7 +985,6 @@ config I2C_XLP9XX
config I2C_RCAR
tristate "Renesas R-Car I2C Controller"
depends on ARCH_SHMOBILE || COMPILE_TEST
- select I2C_SLAVE
help
If you say yes to this option, support will be included for the
R-Car I2C controller.
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 96bb4e749012..75d6095c5fe1 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -316,7 +316,7 @@ static u32 em_i2c_func(struct i2c_adapter *adap)
return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SLAVE;
}
-static int em_i2c_reg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_reg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -334,7 +334,7 @@ static int em_i2c_reg_slave(struct i2c_client *slave)
return 0;
}
-static int em_i2c_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused em_i2c_unreg_slave(struct i2c_client *slave)
{
struct em_i2c_device *priv = i2c_get_adapdata(slave->adapter);
@@ -350,8 +350,10 @@ static int em_i2c_unreg_slave(struct i2c_client *slave)
static struct i2c_algorithm em_i2c_algo = {
.master_xfer = em_i2c_xfer,
.functionality = em_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = em_i2c_reg_slave,
.unreg_slave= em_i2c_unreg_slave,
+#endif
};
static int em_i2c_probe(struct platform_device *pdev)
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 3ed1f0aa5eeb..e67824adeba0 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -521,7 +521,7 @@ out:
return ret;
}
-static int rcar_reg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_reg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -542,7 +542,7 @@ static int rcar_reg_slave(struct i2c_client *slave)
return 0;
}
-static int rcar_unreg_slave(struct i2c_client *slave)
+static int __maybe_unused rcar_unreg_slave(struct i2c_client *slave)
{
struct rcar_i2c_priv *priv = i2c_get_adapdata(slave->adapter);
@@ -568,8 +568,10 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
static const struct i2c_algorithm rcar_i2c_algo = {
.master_xfer= rcar_i2c_master_xfer,
.functionality = rcar_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
.reg_slave = rcar_reg_slave,
.unreg_slave= rcar_unreg_slave,
+#endif
};
static const struct of_device_id rcar_i2c_dt_ids[] = {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 51028f351d13..69871e5ee44a 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -254,7 +254,6 @@ static inline void i2c_set_clientdata(struct i2c_client
*dev, void *data)
/* I2C slave support */
-#if IS_ENABLED(CONFIG_I2C_SLAVE)
enum i2c_slave_event {
I2C_SLAVE_READ_REQUESTED,
I2C_SLAVE_WRITE_REQUESTED,
@@ -269,9 +268,12 @@ extern int i2c_slave_unregister(struct i2c_client *client);
static inline int i2c_s
Re: [PATCH v1 00/13] intel-lpss: support non-ACPI platforms
On Tuesday 24 November 2015 12:22:46 Andy Shevchenko wrote: > This series includes few logical sets that bring a support of non-ACPI > platforms for Intel Skylake. > > First part is a refactoring of built-in device properties support: > - keep single value inside the structure > - provide helper macros to define built-in properties > - fall back to secondary fwnode if primary has no asked property > > Second one is modifications to MFD code and intel-lpss.c driver in particular > to define and pass built-in properties to the individual drivers. > > Last part is a fix for I2C bug found on Lenovo Yoga hardware and a first > converted user. > > Built-in device properties is an alternative to platform data. It provides a > unified API that drivers can use to cover all cases at once: DT, ACPI, and > built-in properties. > > With this series applied platform data can be considered obsolete. Moreover, > built-in device properties allows to adjust existing configuration, for > example, in cases when ACPI values are wrong on some platforms. > > The series has been tested on available hardware and doesn't break current > behaviour. But we ask you, Kevin, to apply the series on your side and check > with Lenovo hardware. I agree with Rafael, this looks really nice. I found one small thing that could be improved, see the comment on patch 11. Aside from that, I think we should have a nicer way to pass a property list through platform_device_info when calling platform_device_register_full(). You don't do that here because the drivers you change are based on MFD cells rather than direct platform devices, but it would fit in the series and should be easy enough to do. I don't know why Rafael didn't do that for the initial series already, maybe he had a good reason. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 11/13] mfd: intel-lpss: Pass HSUART configuration via properties
On Tuesday 24 November 2015 12:22:57 Andy Shevchenko wrote:
> +static struct property_entry uart_properties[] = {
> + PROPERTY_ENTRY_U32("reg-io-width", 4),
> + PROPERTY_ENTRY_U32("reg-shift", 2),
> + PROPERTY_ENTRY_U8("snps,uart-16550-compatible", 1),
> + { },
>
If I read the binding correctly, the "snps,uart-16550-compatible" property
is meant to be boolean, meaning true if present and zero-length or false
if absent. Using a u8 propert instead feels wrong.
Maybe we can have a PROPERTY_ENTRY_BOOL() for that?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] power: bq27xxx_battery: Add I2C module check dependency in Kconfig
On Monday 09 November 2015 15:23:03 Andrew F. Davis wrote:
> Check if I2C core has been built as a module when BATTERY_BQ27XXX
> is built-in. If so disable I2C functionality.
>
> Fixes: 6bd03ce3c12a ("power: bq27xxx_battery: Remove unneeded dependency in
> Kconfig")
> Reported-by: Arnd Bergmann
> Signed-off-by: Andrew F. Davis
Thanks for the fix!
Acked-by: Arnd Bergmann
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] power: bq27xxx_battery: Revert "Remove unneeded dependency in Kconfig"
On Monday 09 November 2015 10:56:13 Andrew F. Davis wrote: > On 11/09/2015 07:50 AM, Arnd Bergmann wrote: > Nothing enabled by BATTERY_BQ27XXX depends on I2C, this workaround is not > correct as it prevents BATTERY_BQ27XXX from being built-in when I2C is a > module, there is no reason for this limitation. > > The undefined references are caused by BATTERY_BQ27XXX being built-in AND > its I2C functionality being enabled (BATTERY_BQ27XXX_I2C) while I2C is a > module. Reorganizing this driver is being discussed anyway, but in the > meantime a more correct fix would be along the lines of: > > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig > index 6de6ec2..d1d32f9 100644 > --- a/drivers/power/Kconfig > +++ b/drivers/power/Kconfig > @@ -167,6 +167,7 @@ config BATTERY_BQ27XXX_I2C > bool "BQ27xxx I2C support" > depends on BATTERY_BQ27XXX > depends on I2C > + depends on !(I2C=m && BATTERY_BQ27XXX=y) > default y > help >Say Y here to enable support for batteries with BQ27xxx (I2C) > chips. That works too, there is just very little difference in the end here, and it's easier to revert an patch that only introduces a regression than to do a different hack, especially if it's going to be reworked soon anyway. Do you want to submit the above as a fixup to your other patch or should we just do the revert? It would be good to get one of the two into -rc1. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] power: bq27xxx_battery: Revert "Remove unneeded dependency in Kconfig"
The dependency was clearly needed, without it it is possible to
build the core i2c driver as a loadable module and the bq27xxx
driver built-in, which results in link errors:
drivers/built-in.o: In function `bq27xxx_battery_i2c_read':
binder.c:(.text+0x360bf0): undefined reference to `i2c_transfer'
binder.c:(.text+0x360c10): undefined reference to `i2c_transfer'
drivers/built-in.o: In function `bq27xxx_battery_init':
binder.c:(.init.text+0xe668): undefined reference to `i2c_register_driver'
drivers/built-in.o: In function `bq27xxx_battery_exit':
binder.c:(.exit.text+0x1a0c): undefined reference to `i2c_del_driver'
Signed-off-by: Arnd Bergmann
Fixes: 6bd03ce3c12a ("power: bq27xxx_battery: Remove unneeded dependency in
Kconfig")
---
The bug was originally found and fixed by Xiong Zhou, but Andrew Davis
broke it again by reverting the fix.
I found it today on my ARM randconfig builds.
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 237d7aa73e8c..9f53fb74ae6f 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -159,6 +159,7 @@ config BATTERY_SBS
config BATTERY_BQ27XXX
tristate "BQ27xxx battery driver"
+ depends on I2C || I2C=n
help
Say Y here to enable support for batteries with BQ27xxx (I2C/HDQ)
chips.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: designware: fix platform driver probe rename
A function rename was incomplete and missed the !CONFIG_PM
case:
i2c-designware-platdrv.c:340:13: error: 'dw_i2c_plat_prepare' undeclared here
(not in a function)
i2c-designware-platdrv.c:341:14: error: 'dw_i2c_plat_complete' undeclared here
(not in a function)
This renames the macros accordingly.
Signed-off-by: Arnd Bergmann
Fixes: 6ad6fde3970c ("i2c: designware: Rename platform driver probe and PM
functions")
---
Found on ARM randconfig builds
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index f6b252a8ffd1..eb55760ccd9f 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -307,8 +307,8 @@ static void dw_i2c_plat_complete(struct device *dev)
pm_request_resume(dev);
}
#else
-#define dw_i2c_prepare NULL
-#define dw_i2c_completeNULL
+#define dw_i2c_plat_prepareNULL
+#define dw_i2c_plat_complete NULL
#endif
#ifdef CONFIG_PM
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 2/4] i2c: busses: xgene: add acpi support for i2c xgene SLIMpro driver
On Thursday 23 April 2015 11:07:08 Feng Kan wrote:
>
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id xgene_slimpro_i2c_acpi_ids[] = {
> + {"PRP0001", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, slimpro_acpi_ids);
> +#endif
>
Sorry, but this is wrong: The PRP0001 name is meant for embedded
devices that just use the generic properties API for loading, and
does not require specifying an ACPI device ID.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] i2c: Support for Netlogic XLP9XX/5XX I2C controller.
On Friday 13 March 2015 11:59:58 Jayachandran C wrote: > +- compatible : should be "netlogic,xlp9xx-i2c" > Use a real model number here, not a wildcard. Also, please send the binding as a separate patch from the driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] i2c: sunxi: Add Reduced Serial Bus (RSB) DT bindings documentation
On Tuesday 24 February 2015 22:32:57 Chen-Yu Tsai wrote:
> On Tue, Feb 24, 2015 at 10:17 PM, Arnd Bergmann wrote:
> > On Tuesday 24 February 2015 22:01:26 Chen-Yu Tsai wrote:
> >> On Tue, Feb 24, 2015 at 6:37 PM, Arnd Bergmann wrote:
> >> > On Tuesday 24 February 2015 18:29:02 Chen-Yu Tsai wrote:
> >> >>
> >> >> + rsb@01f03400 {
> >> >> + compatible = "allwinner,sun8i-a23-rsb";
> >> >> + reg = <0x01f03400 0x400>;
> >> >> + interrupts = <0 39 4>;
> >> >> + clocks = <&apb0_gates 3>;
> >> >> + clock-frequency = <300>;
> >> >> + resets = <&apb0_rst 3>;
> >> >> +
> >> >> + axp223: pmic@2d {
> >> >> + compatible = "x-powers,axp223",
> >> >> "x-powers,axp221";
> >> >> + reg = <0x2d>;
> >> >> + allwinner,rsb-hw-addr = <0x3e3>;
> >> >> +
> >> >> + /* ... */
> >> >> + };
> >> >> + };
> >>
> >> > I don't really understand the need for having two addresses (runtime
> >> > and hardware). Could the runtime address be configured at runtime?
> >>
> >> You can, though the driver doesn't support this. I don't think the
> >> I2C subsystem allows arbitrary device insertion during normal
> >> operation, but maybe i2c-dev? I've tried using different addresses
> >> for devices so they do get changed during the probe phase, just
> >> to be sure that the code works, and it's not just sitting at
> >> the address the bootloader used.
> >>
> >> In any case, the distinction is more like burnt-in or hardwired
> >> addresses vs software configurable addresses.
> >
> > The simplest binding would the probably be to only put the
> > hardware address into the 'reg' property and always assign the
> > logical addresses dynamically.
> >
> > Would that add a lot of complexity or does it have any other
> > downsides?
>
> The hardware address is 12 bits wide. Any address higher than
> 0x3ff will be rejected by the I2C core. The AC100 is at 0xe89.
>
> Assigning addresses dynamically means the driver has to keep
> a lookup table to map the hardware address to the logical
> address to issue the command to.
>
> There's also the issue of dynamically assigned address colliding
> with unlisted devices, though I think this would only happen in
> the development / bring up phase of the device.
>
> I think the first issue pretty much rules out putting the
> hardware address into 'reg'.
>
> Putting the logical address in the 'reg' property allows the
> user to poke unlisted devices using i2c-tools, though this
> is not so useful to the average user.
Ok, fair enough. Your approach seems reasonable then, but it
might be helpful to add your explanation to the changelog of the
patch that introduces the binding.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] i2c: sunxi: Add Reduced Serial Bus (RSB) DT bindings documentation
On Tuesday 24 February 2015 22:01:26 Chen-Yu Tsai wrote:
> On Tue, Feb 24, 2015 at 6:37 PM, Arnd Bergmann wrote:
> > On Tuesday 24 February 2015 18:29:02 Chen-Yu Tsai wrote:
> >>
> >> + rsb@01f03400 {
> >> + compatible = "allwinner,sun8i-a23-rsb";
> >> + reg = <0x01f03400 0x400>;
> >> + interrupts = <0 39 4>;
> >> + clocks = <&apb0_gates 3>;
> >> + clock-frequency = <300>;
> >> + resets = <&apb0_rst 3>;
> >> +
> >> + axp223: pmic@2d {
> >> + compatible = "x-powers,axp223", "x-powers,axp221";
> >> + reg = <0x2d>;
> >> + allwinner,rsb-hw-addr = <0x3e3>;
> >> +
> >> + /* ... */
> >> + };
> >> + };
>
> > I don't really understand the need for having two addresses (runtime
> > and hardware). Could the runtime address be configured at runtime?
>
> You can, though the driver doesn't support this. I don't think the
> I2C subsystem allows arbitrary device insertion during normal
> operation, but maybe i2c-dev? I've tried using different addresses
> for devices so they do get changed during the probe phase, just
> to be sure that the code works, and it's not just sitting at
> the address the bootloader used.
>
> In any case, the distinction is more like burnt-in or hardwired
> addresses vs software configurable addresses.
The simplest binding would the probably be to only put the
hardware address into the 'reg' property and always assign the
logical addresses dynamically.
Would that add a lot of complexity or does it have any other
downsides?
arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] i2c: sunxi: Add Reduced Serial Bus (RSB) DT bindings documentation
On Tuesday 24 February 2015 18:29:02 Chen-Yu Tsai wrote:
>
> + rsb@01f03400 {
> + compatible = "allwinner,sun8i-a23-rsb";
> + reg = <0x01f03400 0x400>;
> + interrupts = <0 39 4>;
> + clocks = <&apb0_gates 3>;
> + clock-frequency = <300>;
> + resets = <&apb0_rst 3>;
> +
> + axp223: pmic@2d {
> + compatible = "x-powers,axp223", "x-powers,axp221";
> + reg = <0x2d>;
> + allwinner,rsb-hw-addr = <0x3e3>;
> +
> + /* ... */
> + };
> + };
The child node cannot have a 'reg' property if the parent does not
have #address-cells/#size-cells. You should add these as mandatory
properties in the list.
I don't really understand the need for having two addresses (runtime
and hardware). Could the runtime address be configured at runtime?
Alternatively, could you use #address-cells=<2> and put both into
'reg'?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] i2c: busses: add SLIMpro I2C device driver on APM X-Gene platform
On Friday 09 January 2015 10:56:51 Feng Kan wrote: > On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann wrote: > > On Tuesday 07 October 2014 17:06:47 Feng Kan wrote: > >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > >> index 2e45ae3..a03042c 100644 > >> --- a/drivers/i2c/busses/Kconfig > >> +++ b/drivers/i2c/busses/Kconfig > >> @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL > >> connected there. This will work whatever the interface used to > >> talk to the EC (SPI, I2C or LPC). > >> > >> +config I2C_XGENE_SLIMPRO > >> + tristate "APM X-Gene SoC I2C SLIMpro devices support" > >> + depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX > > > > Why this dependency on XGENE_SLIMPRO_MBOX? > > > > Better replace it with a dependency on MAILBOX. > Arnd, just a question. Is this because this possibly help with future > compatibility by choosing a more broad dependency? The dependency should ideally describe build-time dependencies, to make it possible to build on other architectures for static code analysis purposes. If the driver makes no sense on other platforms you can also use depends on ARCH_XGENE || COMPILE_TEST depends on MAILBOX to cover both the build-time and run-time dependencies. But the dependency on XGENE_SLIMPRO_MBOX just shouldn't be there, the driver will work with any mailbox implementation if someone puts the same hardware into a different SoC. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] i2c: at91: enable probe deferring on dma channel request
On Friday 21 November 2014 14:44:32 Ludovic Desroches wrote: > If dma controller is not probed, defer i2c probe. > > Signed-off-by: Ludovic Desroches > --- Reviewed-by: Arnd Bergmann > > Arnd, > > It's a combination of the first patch I sent and yours. As you said that my > patch "looks wrong but actually it's ok" I didn't dare to add your > signed-off-by. I think a good way to deal with this is to explain in the patch description who wrote what part. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: at91: introduce probe deferring
On Wednesday 19 November 2014 10:16:47 Wolfram Sang wrote:
> On Fri, Nov 14, 2014 at 02:47:59PM +0100, Ludovic Desroches wrote:
> > Return probe defer if requesting a dma channel without a dma controller
> > probed.
> >
> > Signed-off-by: Ludovic Desroches
> > ---
> > drivers/i2c/busses/i2c-at91.c | 22 --
> > 1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> > index 77fb647..df3f4c4 100644
> > --- a/drivers/i2c/busses/i2c-at91.c
> > +++ b/drivers/i2c/busses/i2c-at91.c
> > @@ -679,14 +679,21 @@ static int at91_twi_configure_dma(struct at91_twi_dev
> > *dev, u32 phy_addr)
> > dma_cap_zero(mask);
> > dma_cap_set(DMA_SLAVE, mask);
> >
> > - dma->chan_tx = dma_request_slave_channel_compat(mask, filter, pdata,
> > - dev->dev, "tx");
> > - if (!dma->chan_tx) {
> > + dma->chan_tx = dma_request_slave_channel_reason(dev->dev, "tx");
>
> Will it cause regressions if you drop the compat-version of requesting
> a channel?
I got curious about this, since the patch looks obviously wrong, but
actually it's ok. However the entire DMA support for non-DT platforms
can be dropped in this driver, see patch below
> > + if (IS_ERR(dma->chan_tx)) {
> > + ret = PTR_ERR(dma->chan_tx);
> > + if (ret == -EPROBE_DEFER) {
> > + dev_warn(dev->dev, "no DMA channel available at the
> > moment\n");
>
> I'd say drop this warning. The core usually prints when deferred probing
> takes place.
Definitely yes.
Arnd
8<---
[PATCH] i2c: at91: remove legacy DMA supoprt
Since at91sam9g45 is now DT-only, all DMA capable users of this driver are
using the DT case, and the legacy support can be removed. While at it, fix
the deferred probe case.
Signed-off-by: Arnd Bergmann
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 917d54588d95..534f4c07bfb6 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -72,8 +72,6 @@ struct at91_twi_pdata {
unsigned clk_max_div;
unsigned clk_offset;
bool has_unre_flag;
- bool has_dma_support;
- struct at_dma_slave dma_slave;
};
struct at91_twi_dma {
@@ -541,35 +539,30 @@ static struct at91_twi_pdata at91rm9200_config = {
.clk_max_div = 5,
.clk_offset = 3,
.has_unre_flag = true,
- .has_dma_support = false,
};
static struct at91_twi_pdata at91sam9261_config = {
.clk_max_div = 5,
.clk_offset = 4,
.has_unre_flag = false,
- .has_dma_support = false,
};
static struct at91_twi_pdata at91sam9260_config = {
.clk_max_div = 7,
.clk_offset = 4,
.has_unre_flag = false,
- .has_dma_support = false,
};
static struct at91_twi_pdata at91sam9g20_config = {
.clk_max_div = 7,
.clk_offset = 4,
.has_unre_flag = false,
- .has_dma_support = false,
};
static struct at91_twi_pdata at91sam9g10_config = {
.clk_max_div = 7,
.clk_offset = 4,
.has_unre_flag = false,
- .has_dma_support = false,
};
static const struct platform_device_id at91_twi_devtypes[] = {
@@ -598,7 +591,6 @@ static struct at91_twi_pdata at91sam9x5_config = {
.clk_max_div = 7,
.clk_offset = 4,
.has_unre_flag = false,
- .has_dma_support = true,
};
static const struct of_device_id atmel_twi_dt_ids[] = {
@@ -627,30 +619,11 @@ static const struct of_device_id atmel_twi_dt_ids[] = {
MODULE_DEVICE_TABLE(of, atmel_twi_dt_ids);
#endif
-static bool filter(struct dma_chan *chan, void *pdata)
-{
- struct at91_twi_pdata *sl_pdata = pdata;
- struct at_dma_slave *sl;
-
- if (!sl_pdata)
- return false;
-
- sl = &sl_pdata->dma_slave;
- if (sl && (sl->dma_dev == chan->device->dev)) {
- chan->private = sl;
- return true;
- } else {
- return false;
- }
-}
-
static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
{
int ret = 0;
- struct at91_twi_pdata *pdata = dev->pdata;
struct dma_slave_config slave_config;
struct at91_twi_dma *dma = &dev->dma;
- dma_cap_mask_t mask;
memset(&slave_config, 0, sizeof(slave_config));
slave_config.src_addr = (dma_addr_t)phy_addr + AT91_TWI_RHR;
@@ -661,22 +634,17 @@ static int at91_twi_configure_dma(struct at91_twi_dev
*dev, u32 phy_addr)
slave_config.dst_maxburst = 1;
slave_config.device_fc = false;
- dma_cap_zero(mask);
- dma_cap_set(DMA_SLAVE, ma
Re: [PATCH 4/6] i2c: busses: add SLIMpro I2C device driver on APM X-Gene platform
On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2e45ae3..a03042c 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL
> connected there. This will work whatever the interface used to
> talk to the EC (SPI, I2C or LPC).
>
> +config I2C_XGENE_SLIMPRO
> + tristate "APM X-Gene SoC I2C SLIMpro devices support"
> + depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX
Why this dependency on XGENE_SLIMPRO_MBOX?
Better replace it with a dependency on MAILBOX.
> + } else {
> + spin_lock_irqsave(&ctx->lock, flags);
> + ctx->i2c_rx_poll = 1;
> + for (count = SLIMPRO_I2C_WAIT_COUNT; count > 0; count--) {
> + if (ctx->i2c_rx_poll == 0)
> + break;
> + udelay(100);
> + }
No, you can't block the CPU for an extended amount of time with
interrupts disabled. Please kill this code.
> + ctx->resp_msg = data;
> + if (ctx->mbox_client.tx_block)
> + init_completion(&ctx->rd_complete);
reinit_completion()?
> +static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr,
> + u32 addrlen, u32 protocol, u32 readlen,
> + u32 with_data_len, void *data)
> +{
> + dma_addr_t paddr;
> + u32 msg[3];
> + int rc;
> +
> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen,
> +DMA_FROM_DEVICE);
ctx->dev is probably the wrong device here. The i2c controller is not
DMA capable itself, you need to have a pointer to the device that actually
performs the DMA here.
> + /* Request mailbox channel */
> + cl->dev = &pdev->dev;
> + cl->rx_callback = slimpro_i2c_rx_cb;
> + cl->tx_done = slimpro_i2c_tx_done;
> + cl->tx_block = true;
> + cl->tx_tout = SLIMPRO_OP_TO_MS;
> + cl->knows_txdone = false;
> + cl->chan_name = "i2c-slimpro";
> + ctx->mbox_chan = mbox_request_channel(cl);
This is not the correct interface.
> + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> + if (rc)
> + dev_warn(&pdev->dev, "Unable to set dma mask\n");
Are you sure that this is the correct device to perform the DMA?
Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd
function passes in only the lower 32 bit of the address, which would
be DMA_BIT_MASK(32).
> +#ifdef CONFIG_OF
> +static struct of_device_id xgene_slimpro_i2c_id[] = {
> + {.compatible = "apm,xgene-slimpro-i2c" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids);
> +#endif
> +
> +static struct platform_driver xgene_slimpro_i2c_driver = {
> + .probe = xgene_slimpro_i2c_probe,
> + .remove = xgene_slimpro_i2c_remove,
> + .driver = {
> + .name = XGENE_SLIMPRO_I2C,
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(xgene_slimpro_i2c_id)
> + },
> +};
The driver only supports DT, so just drop the #ifdef and the of_match_ptr().
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] Documentation: i2c: Add APM X-Gene platform SLIMpro I2C driver documentation
On Tuesday 07 October 2014 17:06:48 Feng Kan wrote: > Add APM X-Gene platform SLIMpro I2C driver documentation. > Don't just repeat the subject line, explain what this is for. > Signed-off-by: Feng Kan > Signed-off-by: Hieu Le > --- > .../devicetree/bindings/i2c/i2c-xgene-slimpro.txt| 20 > > 1 file changed, 20 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt > b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt > new file mode 100644 > index 000..1a79d53 > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt > @@ -0,0 +1,20 @@ > +APM X-Gene SLIMpro Mailbox I2C Driver > + > +This is a I2C driver that access the I2C bus through the mailbox mechanism. > +There is documentation of the mailbox driver in the > +Documentation/devicetree/binding/mailbox/xgene-slimpro-mbox.txt > + > +Required properties : > + > + - compatible : should be "apm,xgene-slimpro-i2c" > + - mbox : ptr to the mailbox dts node, use the name of the mailbox as the > + first parameter. The second parameter is the channel number. > + The APM X-Gene SLIMpro mailbox has 8 channels. > + - mbox-names : the name of the mailbox channel. The current form of the mailbox interface no longer uses mbox-names, so just drop that. In the old form, the binding would have been incomplete because you don't list the required name. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] i2c: hix5hd2: add devicetree documentation
On Tuesday 30 September 2014 17:25:25 zhangfei wrote: > On 09/30/2014 05:01 PM, Arnd Bergmann wrote: > > On Sunday 28 September 2014 12:22:07 Zhangfei Gao wrote: > >> + > >> +Required properties: > >> +- compatible: Must be "hisilicon,hix5hd2-i2c" > >> + Specifically, the following versions of the chipset are supported: > >> + Hi3716CV200 (support six I2C module) > >> + Hi3719CV100 (support six I2C module) > >> + Hi3718CV100 (support six I2C module) > >> + Hi3719MV100 (support two I2C module) > >> + Hi3718MV100 (support two I2C module) > >> > > > > How do you detect the specific model? Is there a hardware register that > > lets you know the type? > > If you have a device specific "compatible" string, you should list all > > the known strings. > > > In fact, no need to distinguish these hardware, the only difference is > i2c module number. > The same compatible is used. Ah, so you have multiple nodes in those cases, not just one node with a variable number of I2C hosts. I was a bit confused by the description > These info can be removed to remove the confusion. Yes, I think that would be better, both because it avoids the confusion, and because it means you can use the driver for future machines without having to update the binding each time. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] ARM: dts: hix5hd2: add i2c node
On Sunday 28 September 2014 12:22:09 Zhangfei Gao wrote:
> +
> + i2c0: i2c@b1 {
> + compatible = "hisilicon,hix5hd2-i2c";
> + reg = <0xb1 0x1000>;
> + interrupts = <0 38 4>;
> + clocks = <&clock HIX5HD2_I2C0_RST>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
>
HIX5HD2_I2C0_RST is not defined anywhere, so this will result in the
same build error that has required reverting a lot of patches for the
3.18 merge window.
How do you plan to deal with the dependency in the future?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] i2c: hix5hd2: add devicetree documentation
On Sunday 28 September 2014 12:22:07 Zhangfei Gao wrote: > + > +Required properties: > +- compatible: Must be "hisilicon,hix5hd2-i2c" > + Specifically, the following versions of the chipset are supported: > + Hi3716CV200 (support six I2C module) > + Hi3719CV100 (support six I2C module) > + Hi3718CV100 (support six I2C module) > + Hi3719MV100 (support two I2C module) > + Hi3718MV100 (support two I2C module) > How do you detect the specific model? Is there a hardware register that lets you know the type? If you have a device specific "compatible" string, you should list all the known strings. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/4] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Friday 19 September 2014, Octavian Purdila wrote:
> +struct dln2_gpio_pin {
> + __le16 pin;
> +} __packed;
This does not need to be marked packed, since it is never embedded in another
structure.
> +struct dln2_gpio_pin_val {
> + __le16 pin;
> + u8 value;
> +} __packed;
It's enough here to mark just the 'pin' member as packed.
> +static int dln2_gpio_get_pin_count(struct platform_device *pdev)
> +{
> + int ret;
> + __le16 count;
> + int len = sizeof(count);
> +
> + ret = dln2_transfer(pdev, DLN2_GPIO_GET_PIN_COUNT, NULL, 0, &count,
> + &len);
You must not do a USB transaction on stack memory.
> +static int dln2_gpio_pin_cmd(struct dln2_gpio *dln2, int cmd, unsigned pin)
> +{
> + struct dln2_gpio_pin req = {
> + .pin = cpu_to_le16(pin),
> + };
> +
> + return dln2_transfer(dln2->pdev, cmd, &req, sizeof(req), NULL, NULL);
> +}
Same here
> +static int dln2_gpio_pin_val(struct dln2_gpio *dln2, int cmd, unsigned int
> pin)
> +{
> + int ret;
> + struct dln2_gpio_pin req = {
> + .pin = cpu_to_le16(pin),
> + };
> + struct dln2_gpio_pin_val rsp;
And here.
> +static int dln2_gpio_set_debounce(struct gpio_chip *chip, unsigned offset,
> + unsigned debounce)
> +{
> + struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> + struct {
> + __le32 duration;
> + } __packed req = {
> + .duration = cpu_to_le32(debounce),
> + };
> +
> + return dln2_transfer(dln2->pdev, DLN2_GPIO_SET_DEBOUNCE,
> + &req, sizeof(req), NULL, NULL);
> +}
Here you also have a strange __packed attribute that makes no sense
for a local variable, in addition to the stack problem.
I think the only correct way to handle these is to add a dynamic
allocation of an entire page for the DMA, which can probably be
part of the dln2_transfer function so you don't have to do it
in each caller.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] usb: add support for Diolan DLN-2 devices
On Wednesday 20 August 2014, Daniel Baluta wrote: > From: Octavian Purdila > > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO > Master Adapter DLN-2. Details about the device can be found here: > > https://www.diolan.com/i2c/i2c_interface.html. > > Information about the USB protocol can be found in the Programmer's > Reference Manual [1], see section 1.7. > > Because the hardware has a single transmit endpoint and a single > receive endpoint the communication between the various DLN2 drivers > and the hardware will be muxed/demuxed by this driver. > > The functional DLN2 drivers (i2c, GPIO, etc.) will have to register > themselves as DLN2 modules in order to send or receive data. > > Each DLN2 module will be identified by the handle field within the DLN2 > message header. If a DLN2 module issues multiple commands in parallel > they will be identified by the echo counter field in the message header. > > The DLN2 modules can use the dln2_transfer() function to issue a > command and wait for its response. They can also use an asynchronous > mode of operation, in which case a receive callback function is going > to be notified when messages for a specific handle are received. > > Because the hardware reserves handle 0 for GPIO events, the driver > also reserves handle 0. It will be allocated to a DLN2 module only if > it is explicitly requested. > > [1] https://www.diolan.com/downloads/dln-api-manual.pdf > > Signed-off-by: Octavian Purdila After a very brief review of the driver, I think this would be better handled as an MFD driver in drivers/mfd that creates child devices and has the high-level drivers get registered as platform_driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support
On Wednesday 11 June 2014 10:39:40 Boris BREZILLON wrote: > The P2WI controller looks like an SMBus controller which only supports byte > data transfers. But, it differs from standard SMBus protocol on several > aspects: > - it supports only one slave device, and thus drop the address field > - it adds a parity bit every 8bits of data > - only one read access is required to read a byte (instead of a write > followed by a read access in standard SMBus protocol) > - there's no Ack bit after each byte transfer > > This means this bus cannot be used to interface with standard SMBus > devices (the only known device to support this interface is the AXP221 > PMIC). > However the P2WI protocol is close enough to SMBus to be integrated in > the I2C subsystem (see this thread [1] for detailed reasons that led to > integrating this driver in the I2C subsystem). > > [1] http://www.spinics.net/lists/linux-i2c/msg15066.html > > Signed-off-by: Boris BREZILLON > Acked-by: Maxime Ripard Acked-by: Arnd Bergmann > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index c94db1c..a6023fe 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -771,6 +771,18 @@ config I2C_STU300 > This driver can also be built as a module. If so, the module > will be called i2c-stu300. > > +config I2C_SUN6I_P2WI > + tristate "Allwinner sun6i internal P2WI controller" > + depends on MACH_SUN6I && RESET_CONTROLLER One very minor comment: I wonder if we should make this depends on RESET_CONTROLLER depends on MACH_SUN6I || COMPILE_TEST instead so we have better build coverage. I haven't tested if building on other architectures works, or if there are additional dependencies. No need to change this if there are no other comments. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support
On Wednesday 11 June 2014 09:52:52 Boris BREZILLON wrote: > You mean in my commit message ? > I thought it was already explaining the subtle differences between P2WI > and the SMBus protocols. > > What would you like me to add to this explanation ? > Something about the I2C to P2WI initialization part ? > What I'd like to see is a mention of previous discussion that concluded that it should be an i2c driver rather than a new type of driver, and why. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support
On Tuesday 10 June 2014 16:36:04 Maxime Ripard wrote:
> On Tue, Jun 10, 2014 at 03:54:56PM +0200, Arnd Bergmann wrote:
> > On Tuesday 10 June 2014 15:47:16 Boris BREZILLON wrote:
> > >
> > > +config I2C_SUN6I_P2WI
> > > + tristate "Allwinner sun6i internal P2WI controller"
> > > + depends on ARCH_SUNXI
> > > + help
> > > + If you say yes to this option, support will be included for the
> > > + P2WI (Push/Pull 2 Wire Interface) controller embedded in some
> > > sunxi
> > > + SOCs.
> > > + The P2WI looks like an SMBus controller (which supports only
> > > byte
> > > + accesses), except that it only supports one slave device.
> > > + This interface is used to connect to specific PMIC devices
> > > (like the
> > > + AXP221).
> > > +
> >
> > Sorry for the stupid question, but why is this an i2c driver if the
> > hardware protocol is completely different?
>
> It's not completely different. It deviates, but still looks very
> similar to i2c, and to be precise, SMBus.
>
> You'll have the full discussion that led to do this in i2c here:
> http://www.spinics.net/lists/linux-i2c/msg15066.html
>
> Also, one significant thing to take into account is that the
> communication with a device starts as I2C, only to switch to this
> protocol after some initialization sequence.
Ok, sounds good.
> > I understand that a lot of devices can be driven using either spi or
> > i2c, and we have two sets of {directories,maintainers,bus_types,...}
> > for them. Your description sounds like this is a separate option
> > that isn't any closer to i2c than it is to spi.
>
> That's not true. It's *much* closer from I2C than it is from SPI.
Ok.
> > Would it perhaps be better to expose it only as a regmap rather than
> > an i2c host?
>
> That could be a solution, but is it a common practice to define a bus
> adapter driver in a regmap driver?
No, not yet.
Maybe Boris can just put an explanation into the changeset description
of the driver so other people are able to find it more easily.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support
On Tuesday 10 June 2014 15:47:16 Boris BREZILLON wrote:
>
> +config I2C_SUN6I_P2WI
> + tristate "Allwinner sun6i internal P2WI controller"
> + depends on ARCH_SUNXI
> + help
> + If you say yes to this option, support will be included for the
> + P2WI (Push/Pull 2 Wire Interface) controller embedded in some sunxi
> + SOCs.
> + The P2WI looks like an SMBus controller (which supports only byte
> + accesses), except that it only supports one slave device.
> + This interface is used to connect to specific PMIC devices (like the
> + AXP221).
> +
Sorry for the stupid question, but why is this an i2c driver if the
hardware protocol is completely different?
I understand that a lot of devices can be driven using either spi or
i2c, and we have two sets of {directories,maintainers,bus_types,...}
for them. Your description sounds like this is a separate option
that isn't any closer to i2c than it is to spi.
Would it perhaps be better to expose it only as a regmap rather than
an i2c host?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: mux: pca954x needs gpiolib
On Thursday 05 June 2014 12:56:08 Laurent Pinchart wrote:
>
> On Thursday 05 June 2014 12:44:47 Arnd Bergmann wrote:
> > commit 4807e8459bce ("i2c: mux: pca954x: Use the descriptor-based GPIO
> > API") moved this driver over to the gpio descriptor API, which means
> > we now have a dependency on GPIOLIB and get this build error when
> > it is disabled:
> >
> > i2c/muxes/i2c-mux-pca954x.c: In function 'pca954x_probe':
> > i2c/muxes/i2c-mux-pca954x.c:204:2: error: implicit declaration of function
> > 'devm_gpiod_get' [-Werror=implicit-function-declaration] gpio =
> > devm_gpiod_get(&client->dev, "reset");
> > ^
> > i2c/muxes/i2c-mux-pca954x.c:204:7: warning: assignment makes pointer from
> > integer without a cast [enabled by default] gpio =
> > devm_gpiod_get(&client->dev, "reset");
> >^
> > i2c/muxes/i2c-mux-pca954x.c:206:3: error: implicit declaration of function
> > 'gpiod_direction_output' [-Werror=implicit-function-declaration]
> > gpiod_direction_output(gpio, 0);
> >^
> >
> > This adds the dependency in Kconfig as we do for other similar drivers.
>
> I've sent "i2c: pca954x: Fix compilation without CONFIG_GPIOLIB" yesterday,
> which fixes the compilation issue by including . When
> CONFIG_GPIOLIB isn't set the header defines stub functions, keeping the
> driver
> usable without GPIOLIB support.
Ok, makes sense. Should we remove the 'depends on GPIOLIB' from other
drivers doing the same, too?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: mux: pca954x needs gpiolib
commit 4807e8459bce ("i2c: mux: pca954x: Use the descriptor-based GPIO
API") moved this driver over to the gpio descriptor API, which means
we now have a dependency on GPIOLIB and get this build error when
it is disabled:
i2c/muxes/i2c-mux-pca954x.c: In function 'pca954x_probe':
i2c/muxes/i2c-mux-pca954x.c:204:2: error: implicit declaration of function
'devm_gpiod_get' [-Werror=implicit-function-declaration]
gpio = devm_gpiod_get(&client->dev, "reset");
^
i2c/muxes/i2c-mux-pca954x.c:204:7: warning: assignment makes pointer from
integer without a cast [enabled by default]
gpio = devm_gpiod_get(&client->dev, "reset");
^
i2c/muxes/i2c-mux-pca954x.c:206:3: error: implicit declaration of function
'gpiod_direction_output' [-Werror=implicit-function-declaration]
gpiod_direction_output(gpio, 0);
^
This adds the dependency in Kconfig as we do for other similar drivers.
Signed-off-by: Arnd Bergmann
Cc: Laurent Pinchart
Cc: Linus Walleij
Cc: Wolfram Sang
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index f7f9865b..f6d313e 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -40,6 +40,7 @@ config I2C_MUX_PCA9541
config I2C_MUX_PCA954x
tristate "Philips PCA954x I2C Mux/switches"
+ depends on GPIOLIB
help
If you say yes here you get support for the Philips PCA954x
I2C mux/switch devices.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c: nuc900: remove driver
On Sunday 01 June 2014 22:35:25 Wolfram Sang wrote: > Arnd said in another patch: > > "As far as I can tell, this driver must have produced this > error for as long as it has been merged into the mainline kernel, but > it was never part of the normal build tests: > > drivers/i2c/busses/i2c-nuc900.c: In function 'nuc900_i2c_probe': > drivers/i2c/busses/i2c-nuc900.c:601:17: error: request for member > 'apbfreq' in something not a structure or union > ret = (i2c->clk.apbfreq)/(pdata->bus_freq * 5) - 1; > ^ > This is an attempt to get the driver to build and possibly > work correctly, although I do wonder whether we should just > remove it, as it has clearly never worked." > > I agree with removing it since nobody showed interest in Arnd's fixup > patch. > > Reported-by: Arnd Bergmann > Signed-off-by: Wolfram Sang > Cc: Wan ZongShun > Acked-by: Arnd Bergmann -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c/nuc900: fix ancient build error
On Wednesday 14 May 2014 18:27:25 Wolfram Sang wrote: > On Thu, May 08, 2014 at 04:56:22PM +0200, Arnd Bergmann wrote: > > As far as I can tell, this driver must have produced this error > > for as long as it has been merged into the mainline kernel, but > > it was never part of the normal build tests: > > > > drivers/i2c/busses/i2c-nuc900.c: In function 'nuc900_i2c_probe': > > drivers/i2c/busses/i2c-nuc900.c:601:17: error: request for member 'apbfreq' > > in something not a structure or union > > ret = (i2c->clk.apbfreq)/(pdata->bus_freq * 5) - 1; > > ^ > > > > This is an attempt to get the driver to build and possibly > > work correctly, although I do wonder whether we should just > > remove it, as it has clearly never worked. > > I'd go for removing. For this platform, the last patch which was not a > generic cleanup seems to be from late 2011? Ah, you mean removing the entire platform? I guess we could do that as well, but I was really thinking of just removing the i2c driver. For the moment, I'd leave this up to Wan ZongShun. He has in the past at least replied to emails about the platform, even though there hasn't been any new development. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/22] Random ARM randconfig fixes in drivers
On Thursday 08 May 2014, Guenter Roeck wrote: > On Thu, May 08, 2014 at 04:46:51PM +0200, Arnd Bergmann wrote: > > These are a bunch of fixes I had to do to get all randconfig > > configurations on ARM working. Most of these are really old > > bugs, but there are also some new ones. I don't think any of > > them require a backport to linux-stable. > > > > I have checked that they are all still required on yesterday's > > linux-next kernel. Please apply on the appropriate trees unless > > there are objections. > > > Is this series of patches also going to fix arm:allmodconfig ? Possibly, I haven't checked in a while. I'm unfortunately sitting on about 200 other patches in the same branch, which together fix all build errors in any configuration I encountered. I should really do some allmodconfig/allnoconfig/allyesconfig builds without my series again, and prioritize sending out the ones required for that. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c/nuc900: fix ancient build error
As far as I can tell, this driver must have produced this error for as long as it has been merged into the mainline kernel, but it was never part of the normal build tests: drivers/i2c/busses/i2c-nuc900.c: In function 'nuc900_i2c_probe': drivers/i2c/busses/i2c-nuc900.c:601:17: error: request for member 'apbfreq' in something not a structure or union ret = (i2c->clk.apbfreq)/(pdata->bus_freq * 5) - 1; ^ This is an attempt to get the driver to build and possibly work correctly, although I do wonder whether we should just remove it, as it has clearly never worked. Signed-off-by: Arnd Bergmann Cc: Wolfram Sang Cc: linux-i2c@vger.kernel.org Cc: Wan ZongShun Cc: Marek Vasut Cc: Baruch Siach --- drivers/i2c/busses/i2c-nuc900.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-nuc900.c b/drivers/i2c/busses/i2c-nuc900.c index 36394d7..e3e9f95 100644 --- a/drivers/i2c/busses/i2c-nuc900.c +++ b/drivers/i2c/busses/i2c-nuc900.c @@ -598,7 +598,7 @@ static int nuc900_i2c_probe(struct platform_device *pdev) clk_get_rate(i2c->clk); - ret = (i2c->clk.apbfreq)/(pdata->bus_freq * 5) - 1; + ret = clk_get_rate(i2c->clk)/(pdata->bus_freq * 5) - 1; writel(ret & 0x, i2c->regs + DIVIDER); /* find the IRQ for this unit (note, this relies on the init call to -- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 00/22] Random ARM randconfig fixes in drivers
These are a bunch of fixes I had to do to get all randconfig configurations on ARM working. Most of these are really old bugs, but there are also some new ones. I don't think any of them require a backport to linux-stable. I have checked that they are all still required on yesterday's linux-next kernel. Please apply on the appropriate trees unless there are objections. Patch numbers are per subsystem, which I thought is less confusing than numbering them 1-22 when they are all totall independent. Arnd Bergmann (22): mdio_bus: fix devm_mdiobus_alloc_size export phy: kona2: use 'select GENERIC_PHY' in Kconfig phy: exynos: fix SATA phy license typo dmaengine: omap: hide filter_fn for built-in drivers dmaengine: sa11x0: remove broken #ifdef mtd/onenand: fix build warning for dma type mtd: orion-nand: fix build error with ARMv4 clk/versatile: export symbols for impd1 bus/omap_l3: avoid sync initcall for modules bus/arm-cci: add dependency on OF && CPU_V7 watchdog: iop_wdt only builds for mach-iop13xx mpilib: use 'static inline' for mpi-inline.h ata: pata_at91 only works on sam9 i2c/nuc900: fix ancient build error iio: always select ANON_INODES iio:adc: at91 requires the input subsystem pci: rcar host needs OF input: fix ps2/serio module dependency input: atmel-wm97xx: only build for AVR32 misc: atmel_pwm: only build for supported platforms remoteproc: da8xx: don't select CMA on no-MMU regulator: arizona-ldo1: add missing #include drivers/ata/Kconfig | 2 +- drivers/bus/Kconfig | 2 +- drivers/bus/omap_l3_noc.c | 4 drivers/bus/omap_l3_smx.c | 4 drivers/clk/versatile/clk-icst.c | 1 + drivers/clk/versatile/clk-impd1.c | 2 ++ drivers/dma/sa11x0-dma.c | 4 drivers/i2c/busses/i2c-nuc900.c | 2 +- drivers/iio/Kconfig | 1 + drivers/iio/adc/Kconfig | 1 + drivers/input/keyboard/Kconfig| 2 +- drivers/input/mouse/Kconfig | 2 +- drivers/input/touchscreen/Kconfig | 2 +- drivers/misc/Kconfig | 3 ++- drivers/mtd/nand/orion_nand.c | 5 + drivers/mtd/onenand/samsung.c | 8 drivers/net/phy/mdio_bus.c| 2 +- drivers/pci/host/Kconfig | 2 +- drivers/phy/Kconfig | 2 +- drivers/phy/phy-exynos5250-sata.c | 2 +- drivers/regulator/arizona-ldo1.c | 1 + drivers/remoteproc/Kconfig| 2 +- drivers/watchdog/Kconfig | 2 +- include/linux/omap-dma.h | 2 +- lib/mpi/mpi-inline.h | 2 +- lib/mpi/mpi-internal.h| 8 26 files changed, 39 insertions(+), 31 deletions(-) -- 1.8.3.2 Cc: bhelg...@google.com Cc: dw...@infradead.org Cc: dmitry.torok...@gmail.com Cc: ba...@ti.com Cc: gre...@linuxfoundation.org Cc: plagn...@jcrosoft.com Cc: ji...@kernel.org Cc: josh...@atmel.com Cc: kis...@ti.com Cc: linus.wall...@linaro.org Cc: broo...@kernel.org Cc: mturque...@linaro.org Cc: nicolas.fe...@atmel.com Cc: o...@wizery.com Cc: li...@arm.linux.org.uk Cc: t...@atomide.com Cc: vinod.k...@intel.com Cc: w...@iguana.be Cc: w...@the-dreams.de Cc: dmaeng...@vger.kernel.org Cc: linux-i2c@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-in...@vger.kernel.org Cc: linux-...@lists.infradead.org Cc: linux-...@vger.kernel.org Cc: linux-samsung-...@vger.kernel.org Cc: linux...@vger.kernel.org Cc: linux-watch...@vger.kernel.org Cc: net...@vger.kernel.org -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: mv64xxx: Fix compilation breakage
On Friday 21 March 2014 20:17:39 Maxime Ripard wrote: > On Fri, Mar 21, 2014 at 11:49:59AM -0400, Paul Gortmaker wrote: > > On Mon, Mar 10, 2014 at 7:29 AM, Russell King - ARM Linux > > wrote: > > > On Mon, Mar 10, 2014 at 11:58:08AM +0100, Maxime Ripard wrote: > > >> On Fri, Mar 07, 2014 at 04:08:36PM +, Russell King - ARM Linux wrote: > > >> > On Fri, Mar 07, 2014 at 03:59:30PM +0100, Maxime Ripard wrote: > > >> > > @@ -900,7 +902,8 @@ mv64xxx_i2c_probe(struct platform_device *pd) > > >> > > exit_free_irq: > > >> > > free_irq(drv_data->irq, drv_data); > > >> > > exit_reset: > > >> > > - if (pd->dev.of_node && !IS_ERR(drv_data->rstc)) > > >> > > + if (pd->dev.of_node && IS_ENABLED(CONFIG_RESET_CONTROLLER) && > > >> > > + !IS_ERR(drv_data->rstc)) > > >> > > reset_control_assert(drv_data->rstc); > > >> > > > >> > Another question is... why do we need to check pd->dev.of_node here? > > >> > If CONFIG_RESET_CONTROLLER is set, we always try to get the reset > > >> > controller node, so drv_data->rstc is either going to be a valid > > >> > pointer, or it's going to be an error pointer - neither > > >> > reset_control_get() nor devm_reset_control_get return NULL. > > >> > > >> Following back on this as I was doing the patch, actually, > > >> drv_data->rstc will be NULL if we're not probed by DT, and hence never > > >> call reset_control_get, that would set an error pointer. > > >> > > >> But then, we can use IS_ERR_OR_NULL on drv_data->rstc. > > > > > > I think you can also move the devm_reset_control_get() into the main > > > probe function: you're only checking for -EPROBE_DEFER from it to fail, > > > allowing other errors to continue with the driver init. This means > > > that on non-OF, devm_reset_control_get() will fail with -ENOENT. > > > > Looping linux-next into the CC since this is the cause of the failure > > in orion5x_defconfig there, and no point in anyone else re-doing the > > same bisect. > > I sent a fix for this that hasn't been picked up yet: > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-March/239069.html > > IIRC, Wolfram's away until Monday, so I guess it will be merged some > time next week. I think there is something wrong with an interface that makes you use IS_ERR_OR_NULL(). If you are calling reset_control_get_optional(), that' should not return an error when there is no reset controller listed in the device tree. We should still have a way to propagate -EPROBE_DEFER, or bail out if there is a reset controller but there is something wrong with it, but otherwise I'd suggest just leaving NULL as a valid pointer in drv_data->rstc and making sure that the reset controller functions can just deal with a NULL argument, so you never have to check it again. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: use {readl|writel}_relaxed instead of readl/writel in i2c-designware-core ?
On Friday 14 February 2014 15:54:38 Jisheng Zhang wrote: > Hi all, > > The writel/readl is too expensive especially on Cortex A9 w/ outer L2 cache. > This > introduce i2c read/write error on Marvell Berlin SoCs when there are L2 cache > maintenance operations at the same time. > > In our internal berlin bsp, we just replaced readl/writel with the relaxed > version. But AFAIK, the "relaxed" version doesn't exist on all architectures. > How > to handle this issue? In case of i2c-designware, this is safe because that driver does not perform DMA. In other drivers, you may have to be more careful, to ensure that all MMIO is serialized with DMA operations performed by the driver. > Any suggestions are appreciated. I would definitely welcome a patch that adds a default _relaxed implementation to include/linux/io.h, like this: #ifndef readb_relaxed #define readb_relaxed(p) readb(p) #endif and then adds "#define readb_relaxed(p) readb_relaxed(p)" etc. to all architectures that have a non-macro definition for readb. Alternatively, we could have a CONFIG_ARCH_MMIO_RELAXED configuration symbol that gets selected by any architecture that provides the _relaxed accessors, and get linux/io.h to define all of them for the other architectures. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/4] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
On Wednesday 08 January 2014, Gregory CLEMENT wrote:
> On 08/01/2014 16:21, Wolfram Sang wrote:
> >> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c
> >> b/drivers/i2c/busses/i2c-mv64xxx.c
> >> index 8be7e42aa4de..f424c0f89946 100644
> >> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> >> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> >> @@ -692,6 +692,10 @@ static const struct of_device_id
> >> mv64xxx_i2c_of_match_table[] = {
> >> { .compatible = "allwinner,sun4i-i2c", .data =
> >> &mv64xxx_i2c_regs_sun4i},
> >> { .compatible = "marvell,mv64xxx-i2c", .data =
> >> &mv64xxx_i2c_regs_mv64xxx},
> >> { .compatible = "marvell,mv78230-i2c", .data =
> >> &mv64xxx_i2c_regs_mv64xxx},
> >> +{
> >> +.compatible = "marvell,mv78230-a0-i2c",
> >> +.data = &mv64xxx_i2c_regs_mv64xxx
> >> +},
> >
> > I think a oneliner entry like the entries above is easier to read, but
> > that is very minor...
>
> By using one line we would break the 80 character rule,
> hat why I did in this way.
>
It's more a guideline than a strict rule. I agree that one line would
be better here, but it's not important.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs
On Wednesday 08 January 2014 16:06:25 Gregory CLEMENT wrote: > Hi, > > Here come the 5th version of the series fixing the i2c bus hang on A0 > version of the Armada XP SoCs. It occurred on the early release of the > OpenBlocks AX3-4 boards. Indeed the first variants of Armada XP SoCs > (A0 stepping) have issues related to the i2c controller which prevent > to use the offload mechanism and lead to a kernel hang during boot. > > The main change are the use of marvell,mv78230-a0-i2c and that the > function mvebu_get_soc_id() is now local to mach-mvebu. > > The first patch add a mean to detect the SoCs version at run-time and > the second one use this feature in the driver. > > The 3 first patches should be applied on 3.13-rc and on stable kernel > 3.12 as it fixes a regression introduce by the commit 930ab3d403ae > "i2c: mv64xxx: Add I2C Transaction Generator support". > > The first patch could be latter be extend to also be used with dove, > kirkwood, orion5x and mv78x00 when there will be merged in mvebu and > even expose the SoC ID and revision to userspace. > > Jason, do you still agree to take the series once Wolfram have given > his acked-by? Whole series Acked-by: Arnd Bergmann -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On Wednesday 08 January 2014 14:39:59 Wolfram Sang wrote: > > However, when I first read this I thought it should be a -a0 specific > > compatible string, not a 'offload-broken' property - any idea what the > > DT consensus is here? I've seen both approach in use .. > > >>> > > >>> I prefer the replacement of the compatible string. If it should really > > >>> be a seperate property, then it should be a vendor specific property. It > > >>> is not generic, at all. > > >> > > >> Something like "marvell,offload-broken" would be acceptable? > > > > > > A tad more, yes. Still, since this is a feature/quirk of the IP core > > > revision, it should be deduced from the compatible property IMO. It > > > cannot be configured anywhere, so it doesn't change on board level. > > > > So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and > > updating it with the follwing piece of code? > > This is the approach I favour, yes. Can't say much about the > implementation. Looks OK, but dunno if this is minimal... > I would prefer the separate property in this case, but only because it's easier to add in the quirk than to change the compatible string, but it's not a strong preference and I don't mind getting overruled if you all favor the alternative. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On Wednesday 08 January 2014 14:42:45 Gregory CLEMENT wrote:
> You means something like the following code ?
>
> static void __init armada_370_xp_dt_init(void)
> {
> + if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
> + i2c_quirk();
> of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> }
Yes, that looks like a good way to do it.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] ARM: mvebu: Add support to get the ID and the revision of a SoC
On Tuesday 07 January 2014, Gregory CLEMENT wrote:
> diff --git a/include/linux/mvebu-soc-id.h b/include/linux/mvebu-soc-id.h
> new file mode 100644
> index ..31654252fe35
> --- /dev/null
> +++ b/include/linux/mvebu-soc-id.h
> +#ifdef CONFIG_ARCH_MVEBU
> +int mvebu_get_soc_id(u32 *dev, u32 *rev);
> +#else
> +static inline int mvebu_get_soc_id(u32 *dev, u32 *rev)
> +{
> + return -1;
> +}
> +#endif
> +
> +#endif /* __LINUX_MVEBU_SOC_ID_H */
With the quirk handling in patch 3, I think we should remove the public
interface
and EXPORT_SYMBOL(), as well as move this header file into mach-mvebu.
That said, we may want to add support for the soc_bus later on (not as part of
the
stable bug fix) to make it possible to query the soc version from user space
through
the standard sysfs files.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] ARM: mvebu: Add quirk for i2c
On Tuesday 07 January 2014, Gregory CLEMENT wrote:
> static void __init armada_370_xp_dt_init(void)
> {
> + i2c_quirk();
> of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> }
I'd prefer to enable the quirk only on machines that we know may be affected,
i.e.
OpenBlocks AX3-4. That would make it easier in the future for everyone to figure
out whether they need to include the quirk in their kernels or not, depending
on whether they want to support these machines. Just a precaution in case we
end up having lots of quirks in the long run.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
On Tuesday 07 January 2014, Gregory CLEMENT wrote: > On 07/01/2014 10:03, Arnd Bergmann wrote: > > On Monday 06 January 2014, Gregory CLEMENT wrote: > > My main concern is that this patch is adding platform code that we'd > > otherwise have to carry in the kernel indefinitely. I agree that > > it's best if we can probe stuff automatically, but that doesn't normally > > mean looking at an unrelated piece of information. If the i2c controller > > registers themselves tell us whether this device is broken or not, > > we should use that information. Looking at a global SoC version register > > however is more like checking a board ID in the pre-DT days where the > > board number is the only information we have and everything is > > derived from that. > > Well the way the hardware is designed is exactly like this: between > two revision of a SoC you can have slightly differences between various > IP and most of the time this IP don't have a specific register for it. > Moreover from my experience a change done in a IP of a given revision > of a SoC will be for this revision and not necessary reported in future > generation of a SoC. Most of the time the IP are not really under a VCS. > That means that the SoC ID is the only reliable information to know > the version of most of the IP inside this SoC. Right. This is not exactly what I'd call "discoverable" though: ideally we would have a way to ask the hardware for the availability of specific features, but that clearly doesn't work here, which leaves the default way to handle it by using DT properties to describe it in software. In case of the AX3-4 board, that would unfortunately imply that we would either need two separate dtb files or fall back to the workaround for all of them. Neither approach seems particularly user-friendly, so some form of autodetection seems appropriate. > > Another idea: Could we have a quirk in the mvebu platform code for > > the AX3-4 to check the SoC version and then change the property for > > the i2c controller based on whether we expect it to work or not? > > This way, we wouldn't even need an interface between the platform > > code and the driver code. > > I can try this last approach: using the device tree to pass platform > parameter from the arch part to the driver. Ok, thanks! Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
On Monday 06 January 2014, Andrew Lunn wrote: > > That would be rather unfortunate and we should probably > > try to merge the bindings eventually and make sure that either OS can > > boot with any conforming DTB > > It probably requires one of the DT maintainers to talk to FreeBSD > equivalents to get some coordination going. We have a lot of generic > stuff, like gpio keys, gpio leds, cpu nodes, mtd partitions, etc, > which could be done at a high level, and then SoC specific nodes > sorted out between individual developers. Right. They seem to have quite a selection of dts files by now, which are roughly comparable to the ones we have, but slightly different in some aspects. > > On the example of missing clocks, it should work as long as all relevant > > clocks are enabled by the boot loader and the clock properties are > > optional the binding. > > However, not all clocks are optional. We need the clock in order to > know how fast it ticks. So at least the serial ports and i2c will not > work, and maybe other devices, i would have to check. Right. We used to have "clock-frequency" properties defined in a number of bindings that predate the generic clock binding, but I guess we wouldn't do that anymore. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
On Monday 06 January 2014, Gregory CLEMENT wrote: > > Relying on the soc id patch for a "stable" bug fix seems a little > > far-reaching to me. I would be happier to first try to do a local > > detection based on the i2c bus device node itself. Do you know how > > It was my first proposal in case adding the soc id detection was > a too big things. But it turned out that the amount of code is very > low so I really think it worth adding it along the fix. Device tree > is supposed to be stable so as soon as we add something in it we are > supposed support it forever. Moreover using device tree for something > we can probe is counter productive. I would still be happier if we did both and only need to check the SoC version if the device is in the "possibly broken" category but default to the existing behavior. My main concern is that this patch is adding platform code that we'd otherwise have to carry in the kernel indefinitely. I agree that it's best if we can probe stuff automatically, but that doesn't normally mean looking at an unrelated piece of information. If the i2c controller registers themselves tell us whether this device is broken or not, we should use that information. Looking at a global SoC version register however is more like checking a board ID in the pre-DT days where the board number is the only information we have and everything is derived from that. > > common the A0 revision is? You mention "early release of the > > OpenBlocks AX3-4 boards". Any others that you suspect? If not, > > No, from the info I gathered I expected that only OpenBlocks AX3-4 > would be the only product shipped with an A0 version and as I said > it should be only a limit amount of them. Ok, good. So we really only need to worry about this one board for now and can make all the others default to normal operation without checking the SoC version. Another idea: Could we have a quirk in the mvebu platform code for the AX3-4 to check the SoC version and then change the property for the i2c controller based on whether we expect it to work or not? This way, we wouldn't even need an interface between the platform code and the driver code. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
On Monday 06 January 2014, Andrew Lunn wrote: > This raises the question, should mainline try to support any random > dtb blob, or only those that have ever shipped with mainline? It should support any dtb that conforms to the binding. > There are some older mainline DT blobs which won't have PCIe in them, > since PCIe support was not there day 1. So returning -ENODEV, and the > i2c controller assuming an A0 would make sense. That seems reasonable, yes. > But what should we do if somebody was to boot linux with a FreeBSD DT > blob? It is a valid blob, it describes the hardware, but the FreeBSD > nodes have different compatibility strings, don't have clocks, etc. > Now that is at the extreme of the range, but where do we put the > marker for compatibility in this range from current mainline blobs to > FreeBSD blobs? Are you saying that FreeBSD has a different set of bindings for the same hardware? That would be rather unfortunate and we should probably try to merge the bindings eventually and make sure that either OS can boot with any conforming DTB, which means we would accept both compatible strings, or that we make an incompatible change to the binding for at least one of them before we call the binding stable and move the repository to devicetree.org (or whereever it goes after moving out of Linux). On the example of missing clocks, it should work as long as all relevant clocks are enabled by the boot loader and the clock properties are optional the binding. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
On Sunday 05 January 2014, Andrew Lunn wrote: > That would be rather odd. These nodes are in the top level SoC dtsi > file. When they are not used, they have status = "disabled" and are in > the dtb blob with this state. > > The only reason i can think of them not being present at all is if > somebody adds an optimizer to dtc which removed disabled nodes. What > does the device tree spec say about that? Are we relying on undefined > dtc behavior? There is no requirement to use the include files. If someone decides to ship a default dtb file in their boot loader, it wouldn't be a bug to leave the nodes out entirely. Arn -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
On Friday 03 January 2014, Gregory CLEMENT wrote: > The first variants of Armada XP SoCs (A0 stepping) have issues related > to the i2c controller which prevent to use the offload mechanism and > lead to a kernel hang during boot. > > The driver now check the revision of the SoC. If the revision is not > more recent than the A0 or if the driver can't get the SoC revision > then it disables the offload mechanism. > > Cc: sta...@vger.kernel.org > Signed-off-by: Gregory CLEMENT > Acked-by: Wolfram Sang Relying on the soc id patch for a "stable" bug fix seems a little far-reaching to me. I would be happier to first try to do a local detection based on the i2c bus device node itself. Do you know how common the A0 revision is? You mention "early release of the OpenBlocks AX3-4 boards". Any others that you suspect? If not, how about adding either a boolean property in the node or a new "compatible" value to distinguish the working version from the broken one? If A0 is very common, you might do the same thing in the opposite way and default to "broken" unless it is explicitly known to be the good version. In general, I'm much in favor of keeping "quirks" local to device drivers if possible and not rely on global system state. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
On Friday 03 January 2014, Gregory CLEMENT wrote:
> All the mvebu SoCs have information related to their variant and
> revision that can be read from the PCI control register.
>
> This patch adds support for Armada XP and Armada 370. This reading of
> the revision and the ID are done before the PCI initialization to
> avoid any conflicts. Once these data are retrieved, the resources are
> freed to let the PCI subsystem use it.
I like the idea of identifying the soc version for any number of
reasons, but I would hope there was some way of doing this that didn't
involve probing the PCI device. I know this is the traditional way
on orion/kirkwood/dove/... but it always felt wrong to me.
> +static u32 soc_dev_id;
> +static u32 soc_rev;
> +static bool is_id_valid;
Would it be reasonable to reuse the global "system_rev" variable for this
rather than a platform specific exported function?
> +static const struct of_device_id mvebu_pcie_of_match_table[] = {
> + { .compatible = "marvell,armada-xp-pcie", },
> + { .compatible = "marvell,armada-370-pcie", },
> + {},
> +};
> +
> +int mvebu_get_soc_id(u32 *dev, u32 *rev)
> +{
> + if (is_id_valid) {
> + *dev = soc_dev_id;
> + *rev = soc_rev;
> + return 0;
> + } else
> + return -1;
> +}
> +
> +EXPORT_SYMBOL(mvebu_get_soc_id);
Maybe EXPORT_SYMBOL_GPL, out of principle?
> +static int __init mvebu_soc_id_init(void)
> +{
> + struct device_node *np;
> + int ret = 0;
> +
> + np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
> + if (np) {
> + void __iomem *pci_base;
> + struct clk *clk;
> + /*
> + * ID and revision are available from any port, so we
> + * just pick the first one
> + */
> + struct device_node *child = of_get_next_child(np, NULL);
I guess all this will fail if for some reason the PCIe node is not
present on machines that don't use PCIe.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MTD EEPROM support and driver integration
On Saturday 06 July 2013 14:01:12 Maxime Ripard wrote: > I'm not exactly sure on what happened to the previous mail that has been > sent empty, but anyway: > > On Sat, Jul 06, 2013 at 11:18:00AM +0200, Arnd Bergmann wrote: > > On Saturday 06 July 2013 10:28:04 Maxime Ripard wrote: > > > > a) like interrupts, regs, dmas, clocks, pinctrl, reset, pwm: fixed > > > > property names > > > > > > > > regmap = <&at25 0xstart 0xlen>; > > > > regmap-names = "mac-address"; > > > > > > > > b) like gpio, regulator: variable property names > > > > > > > > mac-storage = <&at25 0xstart 0xlen>; > > > > > > > > It's unfortunate that we already have examples of both. They are largely > > > > equivalent, but the tendency is towards the first. > > > > > > I don't have a strong feeling for one against another, so whatever works > > > best. Both solutions will be a huge improvement anyway > > > > > > Just out of curiosity, is there any advantages besides having a fixed > > > property name to the first solution? > > > > I think it's mostly for consistency: trying to get most subsystems to > > do it the same way to make it easier for people to write dts files. > > > > A lesser point is that it simplifies the driver code if you don't > > have to pass a name. > > So that leave us with mainly one path to achieve this goal: > - Add a regmap-mtd backend > - Add DT parsing code for regmap > - Move the EEPROM drivers from misc to mtd Yes, I think that would be good. For the last step, we definitely need buy-in from Wolfgand and Jean, as they are maintaining the current eeprom drivers. > What other option would we have? > > I also thought about writing an EEPROM framework of its own, but the > line is really thin between a large EEPROM and say a small SPI > dataflash, which would make it pretty hard to choose between such a > framework and MTD. Isn't flash by definition block based, while EEPROM can be written in byte or word units? I think that is a significant difference, although it doesn't necessarily mean that we can't use MTD for both. We also have a bunch of OTP drivers spread around the kernel, it probably makes sense to consolidate them at the same time, at least on the DT binding side if not the device drivers. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 03 July 2013, Stehle Vincent-B46079 wrote:
> > From: Christian Ruppert [mailto:christian.rupp...@abilis.com]
> (..)
> > Although this doesn't explicitly state what the function returns to me
> > this sounds more like the quotient is returned rather than the remainder?
>
> Hi,
>
> Yes div_u64() returns the quotient.
>
> It is defined in linux/math64.h as:
>
> static inline u64 div_u64(u64 dividend, u32 divisor)
> {
> u32 remainder;
> return div_u64_rem(dividend, divisor, &remainder);
> }
>
> The remainder is probably fully optimized out.
Ah, you are right, sorry for the confusion on my part.
I thought you had used do_div() rather than div_u64().
Everything's fine then, feel free to add my
Acked-by: Arnd Bergmann
to your patch.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 03 July 2013, Christian Ruppert wrote: > On Wed, Jul 03, 2013 at 04:20:03PM +0200, Arnd Bergmann wrote: > > On Wednesday 03 July 2013, Christian Ruppert wrote: > > > On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote: > > > > On Wednesday 26 June 2013, Wolfram Sang wrote: > > > > > On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote: > > > > > > This patch makes the SDA hold time configurable through device tree. > > > > > > > > > > > > Signed-off-by: Christian Ruppert > > > > > > Signed-off-by: Pierrick Hascoet > > > > > > > > > > Applied to for-next, thanks for keeping at it and providing lots of > > > > > useful information. Much appreciated! > > > > > > > > Sorry, but I got a regression that I didn't find reported elsewhere > > > > so far, even though it breaks a lot of the ARM defconfig builds: > > > > > > > > drivers/built-in.o: In function `dw_i2c_probe': > > > > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined > > > > reference to `__udivdi3' > > > > > > > > I suspect you want something like the change below. > > > > > > This looks similar to a patch Vincent Stehle submitted yesterday, see > > > https://lkml.org/lkml/2013/7/2/145 > > > > Thanks for the link. Actually his patch looks wrong to me, because > > > > dev->sda_hold_time = div_u64((u64)ic_clk * ht + 50, 100); > > > > assigns the division remainder to sda_hold_time, not the quotient. > > Hrmmm... At least when I tested it this morning on an ARC architecture > it worked as intended and returned the quotient. Does that mean we have > an issue with this function on ARC? Can anyone who knows these functions > better than I comment? ARC just uses the generic version of div_u64, which is defined in lib/div64.c. I suspect that the division remainder just happens to work well enough for you to not cause any run-time error. You could try adding a printk in that function to show the values you get on ARC. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 03 July 2013, Christian Ruppert wrote: > On Wed, Jul 03, 2013 at 04:20:03PM +0200, Arnd Bergmann wrote: > > On Wednesday 03 July 2013, Christian Ruppert wrote: > > > On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote: > > > > On Wednesday 26 June 2013, Wolfram Sang wrote: > > > > > On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote: > > > > > > This patch makes the SDA hold time configurable through device tree. > > > > > > > > > > > > Signed-off-by: Christian Ruppert > > > > > > Signed-off-by: Pierrick Hascoet > > > > > > > > > > Applied to for-next, thanks for keeping at it and providing lots of > > > > > useful information. Much appreciated! > > > > > > > > Sorry, but I got a regression that I didn't find reported elsewhere > > > > so far, even though it breaks a lot of the ARM defconfig builds: > > > > > > > > drivers/built-in.o: In function `dw_i2c_probe': > > > > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined > > > > reference to `__udivdi3' > > > > > > > > I suspect you want something like the change below. > > > > > > This looks similar to a patch Vincent Stehle submitted yesterday, see > > > https://lkml.org/lkml/2013/7/2/145 > > > > Thanks for the link. Actually his patch looks wrong to me, because > > > > dev->sda_hold_time = div_u64((u64)ic_clk * ht + 50, 100); > > > > assigns the division remainder to sda_hold_time, not the quotient. > > Hrmmm... At least when I tested it this morning on an ARC architecture > it worked as intended and returned the quotient. Does that mean we have > an issue with this function on ARC? Can anyone who knows these functions > better than I comment? ARC just uses the generic version of div_u64, which is defined in lib/div64.c. I suspect that the division remainder just happens to work well enough for you to not cause any run-time error. You could try adding a printk in that function to show the values you get on ARC. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 03 July 2013, Christian Ruppert wrote:
> On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote:
> > On Wednesday 26 June 2013, Wolfram Sang wrote:
> > > On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
> > > > This patch makes the SDA hold time configurable through device tree.
> > > >
> > > > Signed-off-by: Christian Ruppert
> > > > Signed-off-by: Pierrick Hascoet
> > >
> > > Applied to for-next, thanks for keeping at it and providing lots of
> > > useful information. Much appreciated!
> >
> > Sorry, but I got a regression that I didn't find reported elsewhere
> > so far, even though it breaks a lot of the ARM defconfig builds:
> >
> > drivers/built-in.o: In function `dw_i2c_probe':
> > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined
> > reference to `__udivdi3'
> >
> > I suspect you want something like the change below.
>
> This looks similar to a patch Vincent Stehle submitted yesterday, see
> https://lkml.org/lkml/2013/7/2/145
Thanks for the link. Actually his patch looks wrong to me, because
dev->sda_hold_time = div_u64((u64)ic_clk * ht + 50, 100);
assigns the division remainder to sda_hold_time, not the quotient.
Arnd
> > Signed-off-by: Arnd Bergmann
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index def79b5..57e3a07 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -119,10 +119,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
> > if (pdev->dev.of_node) {
> > u32 ht = 0;
> > u32 ic_clk = dev->get_clk_rate_khz(dev);
> > + u64 hold_time;
> >
> > of_property_read_u32(pdev->dev.of_node,
> > "i2c-sda-hold-time-ns", &ht);
> > - dev->sda_hold_time = ((u64)ic_clk * ht + 50) / 100;
> > + hold_time = (u64)ic_clk * ht + 50;
> > + do_div(hold_time, 100);
> > + dev->sda_hold_time = hold_time;
> > }
> >
> > dev->functionality =
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 26 June 2013, Wolfram Sang wrote:
> On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
> > This patch makes the SDA hold time configurable through device tree.
> >
> > Signed-off-by: Christian Ruppert
> > Signed-off-by: Pierrick Hascoet
>
> Applied to for-next, thanks for keeping at it and providing lots of
> useful information. Much appreciated!
Sorry, but I got a regression that I didn't find reported elsewhere
so far, even though it breaks a lot of the ARM defconfig builds:
drivers/built-in.o: In function `dw_i2c_probe':
/git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined
reference to `__udivdi3'
I suspect you want something like the change below.
Signed-off-by: Arnd Bergmann
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index def79b5..57e3a07 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -119,10 +119,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
if (pdev->dev.of_node) {
u32 ht = 0;
u32 ic_clk = dev->get_clk_rate_khz(dev);
+ u64 hold_time;
of_property_read_u32(pdev->dev.of_node,
"i2c-sda-hold-time-ns", &ht);
- dev->sda_hold_time = ((u64)ic_clk * ht + 50) / 100;
+ hold_time = (u64)ic_clk * ht + 50;
+ do_div(hold_time, 100);
+ dev->sda_hold_time = hold_time;
}
dev->functionality =
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 0/9] Add I2C support for Allwinner SoCs
On Wednesday 12 June 2013, Maxime Ripard wrote: > The Marvell and Allwinner controllers share the exact same logic (which > is definitely not trivial), based on a finite state machine that > triggers interrupts at each change of state, each state being a state in > the I2C protocol (like address sent, data received with an ACK, etc.). > > The weird thing is that the only difference between the two controllers > is the register offsets, and that's it. The state numbers, bit index, > etc, are exactly the same. Ok, cool. Great someone noticed! > So yes, I think they both licensed the same IP. I wonder if it's the Mentor Graphics Inventra mi2c block, which would make sense given that Allwinner also uses musb. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 0/9] Add I2C support for Allwinner SoCs
On Wednesday 12 June 2013 10:07:09 Maxime Ripard wrote: > > This patchset adds support for the I2C controller found on most of the > Allwinner SoCs, especially the already supported A10 and A13, and the > yet to come A31. > > This driver leverages the Marvel mv64xxx i2c controller driver, that has > an almost identical logic, with a slightly different register layout. > > It has been tested on a A13-Olinuxino and an A10s-Olinuxino. It doesn't really matter, but can someone clarify why this driver can be reused? Did marvell and allwinner license the same hardware block or is just a really simple driver that does things in an obvious way? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] at_hdmac: move to generic DMA binding
On Thursday 18 April 2013, Nicolas Ferre wrote: > Arnd, > Do you feel like giving your "Acked-by" to this patch? I think that you > were pretty in line with Ludovic's RFC, so it may be interesting to have > your support on this... Yes, it's fine. Acked-by: Arnd Bergmann -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 05/10] i2c: s3c2410: make header file local
On Thursday 11 April 2013, Heiko Stübner wrote: > Am Donnerstag, 11. April 2013, 23:37:20 schrieb Arnd Bergmann: > > No other file in the kernel besides i2c-s3c2410.c uses the current > > plat/regs-iic.h, so we can simply move the header file to live in the > > same directory as the driver, as a preparation to multiplatform builds. > > There is already a patch doing similar changes [0] in the i2c tree for 3.10 > Excellent, that patch is actually nicer than my version, thanks for taking care of this! I'll drop my patch from the series then. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 05/10] i2c: s3c2410: make header file local
No other file in the kernel besides i2c-s3c2410.c uses the current plat/regs-iic.h, so we can simply move the header file to live in the same directory as the driver, as a preparation to multiplatform builds. Signed-off-by: Arnd Bergmann Cc: linux-i2c@vger.kernel.org Cc: Wolfram Sang Cc: Ben Dooks --- arch/arm/mach-s3c24xx/mach-rx1950.c| 1 - arch/arm/plat-samsung/devs.c | 1 - drivers/i2c/busses/i2c-s3c2410.c | 3 ++- .../include/plat/regs-iic.h => drivers/i2c/busses/i2c-s3c2410.h| 0 4 files changed, 2 insertions(+), 3 deletions(-) rename arch/arm/plat-samsung/include/plat/regs-iic.h => drivers/i2c/busses/i2c-s3c2410.h (100%) diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c index 1f9ba2a..43f3ac5 100644 --- a/arch/arm/mach-s3c24xx/mach-rx1950.c +++ b/arch/arm/mach-s3c24xx/mach-rx1950.c @@ -56,7 +56,6 @@ #include #include #include -#include #include #include "common.h" diff --git a/arch/arm/plat-samsung/devs.c b/arch/arm/plat-samsung/devs.c index de9ad27..5e37051 100644 --- a/arch/arm/plat-samsung/devs.c +++ b/arch/arm/plat-samsung/devs.c @@ -62,7 +62,6 @@ #include #include #include -#include #include #include #include diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c index f6b880b..d042ad0 100644 --- a/drivers/i2c/busses/i2c-s3c2410.c +++ b/drivers/i2c/busses/i2c-s3c2410.c @@ -42,9 +42,10 @@ #include -#include #include +#include "i2c-s3c2410.h" + /* Treat S3C2410 as baseline hardware, anything else is supported via quirks */ #define QUIRK_S3C2440 (1 << 0) #define QUIRK_HDMIPHY (1 << 1) diff --git a/arch/arm/plat-samsung/include/plat/regs-iic.h b/drivers/i2c/busses/i2c-s3c2410.h similarity index 100% rename from arch/arm/plat-samsung/include/plat/regs-iic.h rename to drivers/i2c/busses/i2c-s3c2410.h -- 1.8.1.2 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/30] i2c: s3c2410: make header file local
No other file in the kernel besides i2c-s3c2410.c uses the current plat/regs-iic.h, so we can simply move the header file to live in the same directory as the driver, as a preparation to multiplatform builds. Signed-off-by: Arnd Bergmann Cc: linux-i2c@vger.kernel.org Cc: Wolfram Sang Cc: Ben Dooks --- arch/arm/mach-s3c24xx/mach-rx1950.c| 1 - arch/arm/plat-samsung/devs.c | 1 - drivers/i2c/busses/i2c-s3c2410.c | 3 ++- .../include/plat/regs-iic.h => drivers/i2c/busses/i2c-s3c2410.h| 0 4 files changed, 2 insertions(+), 3 deletions(-) rename arch/arm/plat-samsung/include/plat/regs-iic.h => drivers/i2c/busses/i2c-s3c2410.h (100%) diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c index e4d67a3..44ca018 100644 --- a/arch/arm/mach-s3c24xx/mach-rx1950.c +++ b/arch/arm/mach-s3c24xx/mach-rx1950.c @@ -56,7 +56,6 @@ #include #include #include -#include #include #include diff --git a/arch/arm/plat-samsung/devs.c b/arch/arm/plat-samsung/devs.c index 4cf660e..78be9c0 100644 --- a/arch/arm/plat-samsung/devs.c +++ b/arch/arm/plat-samsung/devs.c @@ -62,7 +62,6 @@ #include #include #include -#include #include #include #include diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c index f6b880b..d042ad0 100644 --- a/drivers/i2c/busses/i2c-s3c2410.c +++ b/drivers/i2c/busses/i2c-s3c2410.c @@ -42,9 +42,10 @@ #include -#include #include +#include "i2c-s3c2410.h" + /* Treat S3C2410 as baseline hardware, anything else is supported via quirks */ #define QUIRK_S3C2440 (1 << 0) #define QUIRK_HDMIPHY (1 << 1) diff --git a/arch/arm/plat-samsung/include/plat/regs-iic.h b/drivers/i2c/busses/i2c-s3c2410.h similarity index 100% rename from arch/arm/plat-samsung/include/plat/regs-iic.h rename to drivers/i2c/busses/i2c-s3c2410.h -- 1.8.1.2 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 00/30] ARM: exynos multiplatform support
Hi everyone, I have updated my series for multiplatform support of the ARM exynos platform, based on what is currently queued up in arm-soc. It would be really nice to still get this merged for 3.10. A lot of the patches are really trivial, but there are some complex ones as well. To all subsystem maintainers: feel free to directly apply the patches for your subsystem, there should be no dependencies between any of them, aside from the last patch requiring all of the earlier ones to be applied first. Getting an Ack is also fine so we can put the patches into arm-soc. Arnd Arnd Bergmann (30): ARM: exynos: introduce EXYNOS_ATAGS symbol ARM: exynos: prepare for sparse IRQ ARM: exynos: move debug-macro.S to include/debug/ ARM: samsung: move mfc device definition to s5p-dev-mfc.c tty: serial/samsung: prepare for common clock API tty: serial/samsung: make register definitions global tty: serial/samsung: fix modular build i2c: s3c2410: make header file local mmc: sdhci-s3c: remove platform dependencies usb: exynos: do not include plat/usb-phy.h [media] exynos: remove unnecessary header inclusions video/exynos: remove unnecessary header inclusions video/s3c: move platform_data out of arch/arm thermal/exynos: remove unnecessary header inclusions mtd: onenand/samsung: make regs-onenand.h file local rtc: s3c: make header file local pwm: samsung: repair the worst MMIO abuses ASoC: samsung: move plat/ headers to local directory ASoC: samsung: use irq resource for idma ASoC: samsung: convert to dmaengine API ASoC: samsung/i2s: fix module_device_table ASoC: samsung/idma: export idma_reg_addr_init clk: exynos: prepare for multiplatform clocksource: exynos_mct: remove platform header dependency irqchip: exynos: pass max combiner number to combiner_init irqchip: exynos: allocate combiner_data dynamically irqchip: exynos: localize irq lookup for ATAGS irqchip: exynos: pass irq_base from platform spi: s3c64xx: move to generic dmaengine API ARM: exynos: enable multiplatform support arch/arm/Kconfig | 10 +- arch/arm/Kconfig.debug | 8 + arch/arm/configs/exynos4_defconfig | 2 +- .../mach/debug-macro.S => include/debug/exynos.S} | 12 +- .../plat/debug-macro.S => include/debug/samsung.S} | 2 +- arch/arm/mach-exynos/Kconfig | 40 ++- arch/arm/mach-exynos/Makefile | 5 +- arch/arm/mach-exynos/common.c | 26 +- arch/arm/mach-exynos/common.h | 7 +- arch/arm/mach-exynos/dev-uart.c| 1 + arch/arm/mach-exynos/include/mach/irqs.h | 5 +- arch/arm/mach-exynos/mach-armlex4210.c | 2 + arch/arm/mach-exynos/mach-exynos4-dt.c | 3 + arch/arm/mach-exynos/mach-exynos5-dt.c | 2 + arch/arm/mach-exynos/mach-nuri.c | 2 + arch/arm/mach-exynos/mach-origen.c | 2 + arch/arm/mach-exynos/mach-smdk4x12.c | 2 + arch/arm/mach-exynos/mach-smdkv310.c | 3 + arch/arm/mach-exynos/setup-sdhci-gpio.c| 2 +- arch/arm/mach-exynos/setup-usb-phy.c | 8 +- arch/arm/mach-s3c24xx/clock-s3c2440.c | 5 + arch/arm/mach-s3c24xx/common.c | 5 + arch/arm/mach-s3c24xx/dma-s3c2410.c| 2 - arch/arm/mach-s3c24xx/dma-s3c2412.c| 2 - arch/arm/mach-s3c24xx/dma-s3c2440.c| 2 - arch/arm/mach-s3c24xx/dma-s3c2443.c| 2 - arch/arm/mach-s3c24xx/include/mach/debug-macro.S | 2 +- arch/arm/mach-s3c24xx/mach-rx1950.c| 1 - arch/arm/mach-s3c64xx/include/mach/debug-macro.S | 2 +- arch/arm/mach-s3c64xx/setup-usb-phy.c | 4 +- arch/arm/mach-s5p64x0/include/mach/debug-macro.S | 2 +- arch/arm/mach-s5pc100/include/mach/debug-macro.S | 2 +- arch/arm/mach-s5pc100/setup-sdhci-gpio.c | 1 - arch/arm/mach-s5pv210/include/mach/debug-macro.S | 2 +- arch/arm/mach-s5pv210/setup-sdhci-gpio.c | 1 - arch/arm/mach-s5pv210/setup-usb-phy.c | 4 +- arch/arm/plat-samsung/Kconfig | 7 +- arch/arm/plat-samsung/Makefile | 8 +- arch/arm/plat-samsung/devs.c | 62 ++--- arch/arm/plat-samsung/include/plat/fb.h| 50 +--- arch/arm/plat-samsung/include/plat/pm.h| 5 + arch/arm/plat-samsung/include/plat/regs-serial.h | 282 + arch/arm/plat-samsung/include/plat/sdhci.h | 56 +--- arch/arm/plat-samsung/include/plat/usb-phy.h | 5 +- arch/arm/plat-samsung/irq-vic-timer.c | 1 + arch/arm/plat-samsung/pm.c | 1 + arch/arm/plat-samsung/s5p-dev-mfc.c| 42 ++- arch/arm/plat-samsung/s5p
[PATCH] i2c: davinci: rename "i2c_recover_bus" function
As of commit 5f9296ba "i2c: Add bus recovery infrastructure", there
is now a global function with the same name, which clashes with
the davinci specific one. The obvious solution is to rename the
function with a davinci prefix.
Signed-off-by: Arnd Bergmann
Cc: Viresh Kumar
Cc: linux-i2c@vger.kernel.org
Cc: davinci-linux-open-sou...@linux.davincidsp.com
Cc: Wolfram Sang
Cc: Ben Dooks
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 7d1e590..3acc65a 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -155,7 +155,7 @@ static void generic_i2c_clock_pulse(unsigned int scl_pin)
/* This routine does i2c bus recovery as specified in the
* i2c protocol Rev. 03 section 3.16 titled "Bus clear"
*/
-static void i2c_recover_bus(struct davinci_i2c_dev *dev)
+static void i2c_davinci_recover_bus(struct davinci_i2c_dev *dev)
{
u32 flag = 0;
struct davinci_i2c_platform_data *pdata = dev->pdata;
@@ -289,7 +289,7 @@ static int i2c_davinci_wait_bus_not_busy(struct
davinci_i2c_dev *dev,
return -ETIMEDOUT;
} else {
to_cnt = 0;
- i2c_recover_bus(dev);
+ i2c_davinci_recover_bus(dev);
i2c_davinci_init(dev);
}
}
@@ -379,7 +379,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
dev->adapter.timeout);
if (r == 0) {
dev_err(dev->dev, "controller timed out\n");
- i2c_recover_bus(dev);
+ i2c_davinci_recover_bus(dev);
i2c_davinci_init(dev);
dev->buf_len = 0;
return -ETIMEDOUT;
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/12] ARM: mxs: move to generic DMA device tree binding
On Tuesday 05 March 2013, Shawn Guo wrote: > The series converts mxs-dma and its clients to generic DMA device tree > binding/helper. > > I published a branch below to ease people who is willing to help > testing the series. Looks all good, aside from the bug I found in the first patch. Reviewed-by: Arnd Bergmann -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/11] ARM: s3c: i2c: add platform_device forward declaration
A recent cleanup to the mach-osiris.c file is causing build errors because the i2c-s3c2410.h header file is included before we see the definition for platform_device. The fix is to make the header file more robust against inclusion from other places. While this should normally go through the i2c tree, the bug only exists in arm-soc at the moment, so it's easier to fix it there before it goes upstream. Without this patch, building s3c2410_defconfig results in: arch/arm/mach-s3c24xx/mach-osiris.c:34:0: include/linux/platform_data/i2c-s3c2410.h:37:26: warning: 'struct platform_device' declared inside parameter list [enabled by default] Signed-off-by: Arnd Bergmann Cc: linux-i2c@vger.kernel.org Cc: Wolfram Sang Cc: Ben Dooks Cc: Kukjin Kim --- include/linux/platform_data/i2c-s3c2410.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/linux/platform_data/i2c-s3c2410.h b/include/linux/platform_data/i2c-s3c2410.h index 51d52e7..2a50048 100644 --- a/include/linux/platform_data/i2c-s3c2410.h +++ b/include/linux/platform_data/i2c-s3c2410.h @@ -15,6 +15,8 @@ #define S3C_IICFLG_FILTER (1<<0) /* enable s3c2440 filter */ +struct platform_device; + /** * struct s3c2410_platform_i2c - Platform data for s3c I2C. * @bus_num: The bus number to use (if possible). -- 1.8.1.2 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/34] i.MX multi-platform support
On Thursday 20 September 2012, Shawn Guo wrote: > > On Thu, Sep 20, 2012 at 07:39:34AM +, Arnd Bergmann wrote: > > The first five branches are scheduled to go through the arm-soc tree, so > > I'm fine with that. For the sound/for-3.7 branch, I'd like to know when > > to expect that hitting mainline. If it always gets in very early during the > > merge window, it's probably ok to put the imx/multi-platform patches into > > the same branch as the other ones in arm-soc and wait for the sound stuff > > to hit mainline first, otherwise I'd suggest we start a second > > next/multiplatform-2 branch for imx and send the first part early on > > but then wait with the second batch before sound gets in. > > > It seems that we will have to go with next/multiplatform-2. I just > tried to merge the series with linux-next together, and got some > non-trivial conflicts with media and mtd tree. I might have to rebase > my series on top of these trees to sort out those conflicts. That said, > I will have several dependencies outside arm-soc tree, and have to pend > my series until all those trees get merged into mainline. Ok, fair enough. I think we can put it in arm-soc/for-next as a staging branch anyway to give it some exposure to linux-next, and then we can decide whether a rebase is necessary before sending it to Linus. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/34] i.MX multi-platform support
On Thursday 20 September 2012, Shawn Guo wrote: > > Here is the second post, which should have addressed the comments that > reviewers put on v1. > > It's available on branch below. > > git://git.linaro.org/people/shawnguo/linux-2.6.git imx/multi-platform-v2 > > And it's based on the following branches. > > calxeda/multi-plat > arm-soc/multiplatform/platform-data > arm-soc/multiplatform/smp_ops > arm-soc/imx/cleanup > arm-soc/imx/dt > sound/for-3.7 > > Subsystem maintainers, > > I plan to send the whole series for 3.7 via arm-soc tree. Please let > me know if you have problem with that. Thanks. The first five branches are scheduled to go through the arm-soc tree, so I'm fine with that. For the sound/for-3.7 branch, I'd like to know when to expect that hitting mainline. If it always gets in very early during the merge window, it's probably ok to put the imx/multi-platform patches into the same branch as the other ones in arm-soc and wait for the sound stuff to hit mainline first, otherwise I'd suggest we start a second next/multiplatform-2 branch for imx and send the first part early on but then wait with the second batch before sound gets in. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/34] i.MX multi-platform support
On Monday 17 September 2012, Sascha Hauer wrote: > On Mon, Sep 17, 2012 at 01:34:29PM +0800, Shawn Guo wrote: > > The series enables multi-platform support for imx. Since the required > > frameworks (clk, pwm) and spare_irq have already been adopted on imx, > > the series is all about cleaning up mach/* headers. Along with the > > changes, arch/arm/plat-mxc gets merged into arch/arm/mach-imx. > > > > It's based on a bunch of branches (works from others), Rob's initial > > multi-platform series, Arnd's platform-data and smp_ops (Marc's) and > > imx 3.7 material (Sascha and myself). > > > > It's available on branch below. > > > > git://git.linaro.org/people/shawnguo/linux-2.6.git imx/multi-platform > > > > It's been tested on imx5 and imx6, and only compile-tested on imx2 and > > imx3, so testing on imx2/3 are appreciated. > > Great work! This really pushes the i.MX architecture one step closer to > a clean code base. I agree, this series is wonderful, I thought it would take much longer to get this far. Two small comments on the last two patches from me, but overall I really love it. Acked-by: Arnd Bergmann -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
On Monday 03 September 2012, Lee Jones wrote: > > But if you are handling both, then I agree that platform_data should > > override DT. > > I do agree with this, but I haven't stumbled over such a use-case yet. > I have only provided; clock names, DMA settings and call-back information > via AUX_DATA() thus far, and those are being removed too when a) the > correct bindings are mainlined and b) I have the time. I'd prefer if you just disallow the case where pdata and DT have conflicting information. We don't seem to have a clear rule that is enforced over the kernel, so I don't think we can rely on either one taking precedence over the other in general. In this particular case, we don't have a single board file providing a struct nmk_i2c_controller definition for platform data, so the best way to handle this IMHO is to remove the header file with the platform data definition, and just encode the defaults in the driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 1/2] i2c: pnx: Fix bit definitions
On Monday 20 August 2012, Roland Stigge wrote: > On 08/20/2012 06:26 PM, Kevin Wells wrote: > > I've never had my hands on a PNX4008 chip at NXP, but I do believe they > > are the same IP. That specific I2C IP was used in a number of NXP/Phillips > > chips besides the PNX4008/LPC32xx. I don't think there are any PNX4008's in > > the wild, and even working in NXP, I can't find any non-marketing reference > > material for that part (including the user manual). > > Considering this, it might be a good idea to remove support for PNX4008 > (arch/arm/mach-pnx4008/) altogether. It's hard to maintain support for > hardware which isn't available, even at NXP. It would also simplify > maintenance of mach-lpc32xx because the overlap currently makes me > always wonder if the respective changes still work with mach-pnx4008. > > Any opposition? > > > PS: I just wonder how mach-pnx4008 came into the kernel at all... According to the git logs, Vitaly Wool originally added support for the platform in 2006 when working at MontaVista, and that year was also the last time he or anyone else from that company contributed anything in that directory. Russell was the only other person to make substantial contributions to it, but they all seem to be cross-platform changes. In the platform code, there is only a single board number reserved, with the name of the SoC: MACHINE_START(PNX4008, "Philips PNX4008"). This indicates that whoever was actually using the code did not have their board code upstream and relied on out-of-tree patches for the platform. >From all I can tell, the PNX4008 family probably went to ST-Ericsson, not to NXP in the various acquisitions and mergers that happened around NXP. This explains why Kevin has no documentation or hardware for it. On the ST-Ericsson web site, I could find some information about the PNX4908, presumably a follow-on chip, but not about PNX4008, so I guess the company also considers the product line dead. Finally, the chips seems to be targetted at mobile phones and was introduced seven years ago, which is multiple generations of products in that market, so probably people stopped caring about them long ago, unlike embedded chips from the same era for other markets. I'd say let's wait for Vitaly to reply on this matter, if he doesn't care about the code, we can kill it off in 3.7 or 3.8. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4 V5] add 88pm80x mfd driver
On Monday 09 July 2012, Samuel Ortiz wrote: > On Mon, Jul 09, 2012 at 02:37:31PM +0800, Qiao Zhou wrote: > > change log [v5->v4]: > > 1, change the file name, from 88pm800-core.c, 88pm805-core.c, 88pm80x-i2c.c > > to 88pm800.c, 88pm805.c, 88pm80x.c, and modified Makefile accordingly. > > 2, replace the spinlock used to protect wakeup flag, with using set_bit/ > > clear_bit, which is suitable adn enough in SMP env. > > 3, add the version number in each patch. > I applied the first 2 patches, I'd like to get an ACK from the respective > maintainers for the rtc and the input ones. They can even take it as both > drivers are protected by the MFD symbol. > Please add my Reviewed-by: Arnd Bergmann comment to these two patches as well. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/12] ARM: Orion: DT support for IRQ and GPIO Controllers
On Thursday 05 July 2012, Andrew Lunn wrote:
> > I think the latter one needs to be
> >
> > +static int __initdata gpio1_irqs[4] = {
> > + IRQ_DOVE_HIGH_GPIO,
> > + IRQ_DOVE_HIGH_GPIO,
> > + IRQ_DOVE_HIGH_GPIO,
> > + IRQ_DOVE_HIGH_GPIO,
> > +};
> >
> > so we register all four parts to the same primary IRQ. The
> > same is true for the devicetree representation.
>
> Nope, does not work like that.
>
> It does not matter which IRQ of a GPIO chip fires. It looks at the IRQ
> cause bits for all lines and fires off the secondary ISR as needed for
> the whole chip. So in effect, there is a mapping IRQ->GPIO chip, not
> IRQ->1/4 of GPIO chip. With what you suggest above, you would end up
> with four chained interrupt handlers, all being handled by the same
> interrupt handler for one chio, and the last three in the chain would
> never do anything since the first one does all the work.
Does it really?
The handler function I'm looking at is
static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
{
int irqoff;
BUG_ON(irq < IRQ_DOVE_GPIO_0_7 || irq > IRQ_DOVE_HIGH_GPIO);
irqoff = irq <= IRQ_DOVE_GPIO_16_23 ? irq - IRQ_DOVE_GPIO_0_7 :
3 + irq - IRQ_DOVE_GPIO_24_31;
orion_gpio_irq_handler(irqoff << 3);
if (irq == IRQ_DOVE_HIGH_GPIO) {
orion_gpio_irq_handler(40);
orion_gpio_irq_handler(48);
orion_gpio_irq_handler(56);
}
}
My reading of this is a manual hardwired implementation of a
primary handler that triggers the secondary handler four times
when it's called with a specific argument.
If you want to keep that behavior, this handler cannot be
generic across all mvebu socs, whereas registering four
chained handlers for the same primary interrupt would have
the same effect at a very small runtime overhead without the
need for any special case.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/12] ARM: Orion: DT support for IRQ and GPIO Controllers
On Thursday 05 July 2012, Sebastian Hesselbarth wrote:
> Andrew,
>
> is it possible to group all gpio banks into one DT description?
> For mach-dove it could be something like:
>
> gpio: gpio-controller {
> compatible = "marvell, orion-gpio";
> ...
>
> bank0@d0400 {
> reg = <0xd0400 0x40>;
> ngpio = <8>;
> mask-offset = <0>;
> interrupts = <12>;
> };
>
> bank1@d0400 {
> reg = <0xd0400 0x40>;
> ngpio = <8>;
> mask-offset = <8>;
> interrupts = <13>;
> };
This way you have multiple nodes with the same register
and different names, which is not how it normally works.
>
> This would have the advantage that DT describes gpio-to-irq dependencies.
> Moreover, nodes that reference gpios can do gpios = <&gpio 71 0>; instead of
> gpios = <&gpio3 7 0>;
Is that desired?
The device tree representation should match what is in the data sheet
normally. If they are in a single continuous number range, then we should
probably have a single device node with multiple register ranges
rather than one device node for each 32-bit register. Looking at
arch/arm/plat-orion/gpio.c I think that is not actually the case though
and having separate banks is more logical.
Something completely different I just noticed in the original patch:
> @@ -90,6 +74,27 @@ static void pmu_irq_handler(unsigned int irq, struct
> irq_desc *desc)
> }
> }
>
> +static int __initdata gpio0_irqs[4] = {
> + IRQ_DOVE_GPIO_0_7,
> + IRQ_DOVE_GPIO_8_15,
> + IRQ_DOVE_GPIO_16_23,
> + IRQ_DOVE_GPIO_24_31,
> +};
> +
> +static int __initdata gpio1_irqs[4] = {
> + IRQ_DOVE_HIGH_GPIO,
> + 0,
> + 0,
> + 0,
> +};
I think the latter one needs to be
+static int __initdata gpio1_irqs[4] = {
+ IRQ_DOVE_HIGH_GPIO,
+ IRQ_DOVE_HIGH_GPIO,
+ IRQ_DOVE_HIGH_GPIO,
+ IRQ_DOVE_HIGH_GPIO,
+};
so we register all four parts to the same primary IRQ. The
same is true for the devicetree representation.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/12] ARM: Orion: DT support for IRQ and GPIO Controllers
On Thursday 05 July 2012, Andrew Lunn wrote: > > > > I'm wondering about this one. The other platforms usually put the secondary > > interrupt controllers into the same match table, while you call > > orion_gpio_of_init > > from orion_add_irq_domain. Can you explain why you do this? Does it have > > any disadvantages? > > The issue is knowing what IRQ number to use for the secondary > interrupts. > > Orion use generic chip interrupts, both for the main interrupts and > the GPIO interrupts. This does not yet support irq domain, so i have > to layer a legacy domain on top. The legacy domain needs to know the > first IRQ and the number of IRQs. For the primary IRQs that is > easy. However, GPIO IRQ is not so easy, it depends on how many primary > IRQs there are. This is not fixed. Orion5x has 32, Dove 64, kirkwood, > 64, and mv78xx0 has 96. I need to know this number when adding the > GPIO secondary IRQ legacy domain. By calling orion_gpio_of_init() in > the orion_add_irq_domain() i have this number to hand. If i used to > entries in the match table, i would have to put this number into some > global variable, or somehow ask the IRQ subsystem what the next free > IRQ number is. But couldn't you store the number of interrupts for the primary controller in a local variable? I think the of_irq code already guarantees that the parent is probed first. > As for disadvantages, humm. Dove has yet more interrupts, from the > PMU. They are currently unsupported in DT. When we add support for the > PMU interrupt controller, we are going to have the same problem, what > IRQ base should it use. Either we extend the chaining, calling > dove_pmu_of_init from orion_gpio_of_init(), where we know the next > free IRQ. Or we find out how to ask the IRQ subsystem for the next > available. Better still, the work to make generic chip interrupts irq > domain aware would get completed, and we can swap all this code to irq > domain linear and this whole probable probably goes away. Yes, that makes sense. Using the linear domain should solve all these nicely. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/12] ARM: Orion: DT support for IRQ and GPIO Controllers
On Tuesday 03 July 2012, Andrew Lunn wrote:
> Both IRQ and GPIO controllers can now be represented in DT. The IRQ
> controllers are setup first, and then the GPIO controllers. Interrupts
> for GPIO lines are placed directly after the main interrupts in the
> interrupt space.
Overall looks very good.
> diff --git a/Documentation/devicetree/bindings/arm/mrvl/intc.txt
> b/Documentation/devicetree/bindings/arm/mrvl/intc.txt
> index 80b9a94..8927e10 100644
> --- a/Documentation/devicetree/bindings/arm/mrvl/intc.txt
> +++ b/Documentation/devicetree/bindings/arm/mrvl/intc.txt
> @@ -38,3 +38,22 @@ Example:
> reg-names = "mux status", "mux mask";
> mrvl,intc-nr-irqs = <2>;
> };
> +
> +* Marvell Orion Interrupt controller
> +
> +Required properties
> +- compatible : Should be "marvell,orion-intc".
> +- #interrupt-cells: Specifies the number of cells needed to encode an
> + interrupt source. Supported value is <1>.
> +- interrupt-controller : Declare this node to be an interrupt controller.
> +- reg : Interrupt mask address.
I think you should clarify that the "reg" property can be multiple
4-byte ranges, because that is fairly unusual.
> +#ifdef CONFIG_OF
> +static int __init orion_add_irq_domain(struct device_node *np,
> +struct device_node *interrupt_parent)
> +{
> + int i = 0, irq_gpio;
> + void __iomem *base;
> +
> + do {
> + base = of_iomap(np, i);
> + if (base) {
> + orion_irq_init(i * 32, base);
> + i++;
> + }
> + } while (base);
> +
> + irq_domain_add_legacy(np, i * 32, 0, 0,
> + &irq_domain_simple_ops, NULL);
> +
> + irq_gpio = i * 32;
> + orion_gpio_of_init(irq_gpio);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id orion_irq_match[] = {
> + { .compatible = "marvell,orion-intc",
> + .data = orion_add_irq_domain, },
> + {},
> +};
> +
> +void __init orion_dt_init_irq(void)
> +{
> + of_irq_init(orion_irq_match);
> +}
> +#endif
I'm wondering about this one. The other platforms usually put the secondary
interrupt controllers into the same match table, while you call
orion_gpio_of_init
from orion_add_irq_domain. Can you explain why you do this? Does it have
any disadvantages?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] mfd: support 88pm80x in 80x driver
On Thursday 05 July 2012, Qiao Zhou wrote:
> +
> +static const struct i2c_device_id pm80x_id_table[] = {
> + {"88PM800", CHIP_PM800},
> + {"88PM805", CHIP_PM805},
> +};
> +MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
The point of moving the table to the individual drivers was that the
right one can get loaded automatically. This requires of course that
you put only one in there. It really needs to be
static const struct i2c_device_id pm800_id_table[] = {
{"88PM800", CHIP_PM800},
};
MODULE_DEVICE_TABLE(i2c, pm800_id_table);
and
static const struct i2c_device_id pm805_id_table[] = {
{"88PM805", CHIP_PM805},
};
MODULE_DEVICE_TABLE(i2c, pm805_id_table);
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 75f6ed6..22dba98 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -3,7 +3,12 @@
> #
>
> 88pm860x-objs:= 88pm860x-core.o 88pm860x-i2c.o
> +88pm80x-objs := 88pm80x-i2c.o
> +88pm800-objs := 88pm800-core.o
> +88pm805-objs := 88pm805-core.o
> obj-$(CONFIG_MFD_88PM860X) += 88pm860x.o
> +obj-$(CONFIG_MFD_88PM800)+= 88pm800.o 88pm80x.o
> +obj-$(CONFIG_MFD_88PM805)+= 88pm805.o 88pm80x.o
This can be written much shorter if you leave out the "88pm80?-objs:=" lines
and just build each module from one file as in
obj-$(CONFIG_MFD_88PM800) += 88pm800-core.o 88pm80x-i2c.o
It might make sense to drop the "core" and "i2c" postfixes on the names,
your choice.
> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> new file mode 100644
> index 000..687f296
> --- /dev/null
> +++ b/include/linux/mfd/88pm80x.h
I don't really like repeating myself, but this still contains a lot
of crap that needs to be moved out of this file, into the places
where it's used:
> +enum {
> + /* Procida */
> + PM800_CHIP_A0 = 0x60,
> + PM800_CHIP_A1 = 0x61,
> + PM800_CHIP_B0 = 0x62,
> + PM800_CHIP_C0 = 0x63,
> + PM800_CHIP_END = PM800_CHIP_C0,
> +
> + /* Make sure to update this to the last stepping */
> + PM8XXX_CHIP_END = PM800_CHIP_END
> +};
88pm800-core.c
> +enum {
> + PM800_ID_INVALID,
> + PM800_ID_VIBRATOR,
> + PM800_ID_SOUND,
> + PM800_ID_MAX,
> +};
unused?
> +enum {
> + PM800_ID_BUCK1 = 0,
> + PM800_ID_BUCK2,
> + PM800_ID_BUCK3,
> + PM800_ID_BUCK4,
> + PM800_ID_BUCK5,
> +
> + PM800_ID_LDO1,
> + PM800_ID_LDO2,
> + PM800_ID_LDO3,
> + PM800_ID_LDO4,
> + PM800_ID_LDO5,
> + PM800_ID_LDO6,
> + PM800_ID_LDO7,
> + PM800_ID_LDO8,
> + PM800_ID_LDO9,
> + PM800_ID_LDO10,
> + PM800_ID_LDO11,
> + PM800_ID_LDO12,
> + PM800_ID_LDO13,
> + PM800_ID_LDO14,
> + PM800_ID_LDO15,
> + PM800_ID_LDO16,
> + PM800_ID_LDO17,
> + PM800_ID_LDO18,
> + PM800_ID_LDO19,
> +
> + PM800_ID_RG_MAX,
> +};
> +#define PM800_MAX_REGULATOR PM800_ID_RG_MAX /* 5 Bucks, 19 LDOs */
> +#define PM800_NUM_BUCK (5) /*5 Bucks */
> +#define PM800_NUM_LDO (19) /*19 Bucks */
unused? should probably go into the regulator driver
> +/* 88PM805 Registers */
> +#define PM805_CHIP_ID(0x00)
88pm805-core.c
> +/* Audio */
> +
> +/* 88PM800 registers */
> +enum {
> + PM80X_INVALID_PAGE = 0,
> + PM80X_BASE_PAGE,
> + PM80X_POWER_PAGE,
> + PM80X_GPADC_PAGE,
> + PM80X_TEST_PAGE,
> +};
unused?
> +/* page 0 basic: slave adder 0x60 */
> +
> +/* Interrupt Registers */
> +#define PM800_CHIP_ID(0x00)
> +
> +#define PM800_STATUS_1 (0x01)
> +#define PM800_ONKEY_STS1 (1 << 0)
> +#define PM800_EXTON_STS1 (1 << 1)
> +#define PM800_CHG_STS1 (1 << 2)
> +#define PM800_BAT_STS1 (1 << 3)
> +#define PM800_VBUS_STS1 (1 << 4)
> +#define PM800_LDO_PGOOD_STS1 (1 << 5)
> +#define PM800_BUCK_PGOOD_STS1(1 << 6)
> +
> +#define PM800_STATUS_2 (0x02)
> +#define PM800_RTC_ALARM_STS2 (1 << 0)
These can probably stay here.
> +#define PM800_INT_STATUS1(0x05)
> +#define PM800_ONKEY_INT_STS1 (1 << 0)
> +#define PM800_EXTON_INT_STS1 (1 << 1)
> +#define PM800_CHG_INT_STS1 (1 << 2)
> +#define PM800_BAT_INT_STS1 (1 << 3)
> +#define PM800_RTC_INT_STS1 (1 << 4)
> +#define PM800_CLASSD_OC_INT_STS1 (1 << 5)
> ...
> +/* number of INT_ENA & INT_STATUS regs */
> +#define PM800_INT_REG_NUM(4)
all the interrupt handling can go into 88pm800-core.c
> +/* RTC Registers */
> +#define PM800_RTC_CONTROL(0xD0)
> +#define PM800_RTC_COUNTER1 (0xD1)
> +#define PM800_RTC_COUNTER2 (0xD2)
> +#define PM800_RTC_COUNTER3 (0xD3)
> +#define PM800_RTC_COUNTER4 (0xD4)
> +#define PM800_RTC_EXPIRE1_1 (0xD5)
> +#define PM800_RTC_EXPIRE1_2 (0xD6)
> +#define
Re: [PATCH 1/4] mfd: support 88pm80x in 80x driver
On Wednesday 04 July 2012, Qiao Zhou wrote:
> >> + ret = mfd_add_devices(chip->dev, 0, &onkey_devs[0],
> >> +ARRAY_SIZE(onkey_devs), &onkey_resources[0],
> >> +chip->irq_base);
> >
> >According to what I discussed with Mark in the previous version, I think you
> >need to pass 0 instead of chip->irq_base here, and transform the interrupt
> >numbers using the domain in the client drivers.
> >
> >> +
> >> +const struct i2c_device_id pm80x_id_table[] = {
> >> + {"88PM800", CHIP_PM800},
> >> + {"88PM805", CHIP_PM805},
> >> +};
> >> +MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
> >
> >Since these are separate modules now, you have to move the device table
> >into the split files as well.
> Is it ok to export it in 88pm80x.h?
You mean putting the MODULE_DEVICE_TABLE() into 88pm80x.h? No, that would
not work.
The correct way is to have two MODULE_DEVICE_TABLE statements, one per file.
Actually the table only makes sense for loadable modules, so if you decide
to keep the driver only built-in, it's best to just drop this table.
> >> +
> >> +/**
> >> + * pm80x_reg_read: Read a single 88PM80x register.
> >> + *
> >> + * @map: regmap to read from.
> >> + * @reg: Register to read.
> >> + */
> >> +int pm80x_reg_read(struct regmap *map, u32 reg)
> >> +{
> >> + unsigned int val;
> >> + int ret;
> >> +
> >> + ret = regmap_read(map, reg, &val);
> >> +
> >> + if (ret < 0)
> >> + return ret;
> >> + else
> >> + return val;
> >> +}
> >> +EXPORT_SYMBOL_GPL(pm80x_reg_read);
> >
> >In your introductory email you write
> >
> >"Exported r/w API which requires regmap handle. as currently the pm800
> >chip has 3 i2c device, only passing a pm80x_chip info can't ensure r/w the
> >register in correct i2c device."
> >
> >Your first driver version had this, then you removed the functions
> >after I asked you to, and now they are back, so I assume there is something
> >I don't see yet. It looks like the function is just an unnecessary wrapper
> >that is better open-coded in the caller. Can you explain again what the
> >difference is?
>
> After you suggest to change the r/w API so that caller doesn't care about it's
> via i2c or spi, it makes sense. However due to pm800 has 3 i2c devices, and
> it's hard to export such interface for pm800. Currently to add such interface
> via regmap handle, caller still doesn't care about the actual hw implement,
> also it's clear that all pm80x sub-driver or plat call the unified r/w API.
But there is nothing unified about the function above, it just calls
regmap_read(), so the caller already has access to the regmap pointer.
> >> +/**
> >> + * pm80x_bulk_read: Read multiple 88PM80x registers
> >> + *
> >> + * @map: regmap to read from
> >> + * @reg: First register
> >> + * @buf: Buffer to fill. The data will be returned big endian.
> >> + * @count: Number of registers
> >> + */
> >> +int pm80x_bulk_read(struct regmap *map, u32 reg, u8 *buf, int count)
> >> +{
> >> + return regmap_raw_read(map, reg, buf, count);
> >> +}
> >
> >Unused function? Either export this if you want to provide it as
> >the general API, or drop the function.
> It's used by rtc driver.
Then it needs to be exported, so the rtc driver can be a module.
> >
> >I would still argue that the majority of the constants in this file
> >should get moved into the driver .c file that uses them. Putting them
> >into the header is better done only for interfaces between the
> >driver parts, and for constants that are used by multiple drivers.
>
> These registers still in this header file are either used by mfd core
> driver, regulator/rtc/onkey/codec, or used in platform initial setting.
Exactly. All the values that are only used by the core driver should be
part of the core driver (or a local header in the mfd directory if they
need to be shared between multiple files). The platform code should not
really touch the registers, but only things like the platform_data
structure, which indeed belongs into the global header.
> >> +struct pm80x_subchip {
> >> + struct i2c_client *power_page; /* chip client for power page */
> >> + struct i2c_client *gpadc_page; /* chip client for gpadc page */
> >> + struct regmap *regmap_power;
> >> + struct regmap *regmap_gpadc;
> >> + unsigned short power_page_addr; /* power page I2C address */
> >> + unsigned short gpadc_page_addr; /* gpadc page I2C address */
> >> +};
> >> +
> >> +struct pm80x_chip {
> >> + struct pm80x_subchip *subchip;
> >> + struct device *dev;
> >> + struct i2c_client *client;
> >> + struct regmap *regmap;
> >> + struct regmap_irq_chip *regmap_irq_chip;
> >> + struct regmap_irq_chip_data *irq_data;
> >> + unsigned char version;
> >> + int id;
> >> + int irq;
> >> + int irq_mode;
> >> + int irq_base;
> >> + unsigned int wu_flag;
> >> +};
> >
> >One thing I forgot to ask in the previous review although I had already
> >noticed it then: What is the separation of pm80x_chip and pm80x_subchip
> >used
Re: [PATCH 1/4] mfd: support 88pm80x in 80x driver
On Wednesday 04 July 2012, Qiao Zhou wrote:
> 88PM800 and 88PM805 are two discrete chips used for power management.
> Hardware designer can use them together or only one of them according
> to requirement.
>
> 88pm80x_i2c.c provides common i2c driver handling for both 800 and
> 805, such as i2c_driver init, regmap init, read/write api etc.
>
> 88pm800_core.c handles specifically for 800, such as chip init, irq
> init/handle, mfd device register, including rtc, onkey, regulator(
> to be add later) etc. besides that, 800 has three i2c device, one
> regular i2c client, two other i2c dummy for gpadc and power purpose.
>
> 88pm805_core.c handles specifically for 805, such as chip init, irq
> init/handle, mfd device register, including codec, headset/mic detect
> etc.
>
> the i2c operation of both 800 and 805 are via regmap.
>
> Signed-off-by: Qiao Zhou
The split between the two files looks very good now, I think this way
it makes much more sense for the reader.
> + ret = mfd_add_devices(chip->dev, 0, &onkey_devs[0],
> + ARRAY_SIZE(onkey_devs), &onkey_resources[0],
> + chip->irq_base);
According to what I discussed with Mark in the previous version, I think you
need to pass 0 instead of chip->irq_base here, and transform the interrupt
numbers using the domain in the client drivers.
> +
> +const struct i2c_device_id pm80x_id_table[] = {
> + {"88PM800", CHIP_PM800},
> + {"88PM805", CHIP_PM805},
> +};
> +MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
Since these are separate modules now, you have to move the device table
into the split files as well.
> +
> +/**
> + * pm80x_reg_read: Read a single 88PM80x register.
> + *
> + * @map: regmap to read from.
> + * @reg: Register to read.
> + */
> +int pm80x_reg_read(struct regmap *map, u32 reg)
> +{
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(map, reg, &val);
> +
> + if (ret < 0)
> + return ret;
> + else
> + return val;
> +}
> +EXPORT_SYMBOL_GPL(pm80x_reg_read);
In your introductory email you write
"Exported r/w API which requires regmap handle. as currently the pm800
chip has 3 i2c device, only passing a pm80x_chip info can't ensure r/w the
register in correct i2c device."
Your first driver version had this, then you removed the functions
after I asked you to, and now they are back, so I assume there is something
I don't see yet. It looks like the function is just an unnecessary wrapper
that is better open-coded in the caller. Can you explain again what the
difference is?
> +/**
> + * pm80x_bulk_read: Read multiple 88PM80x registers
> + *
> + * @map: regmap to read from
> + * @reg: First register
> + * @buf: Buffer to fill. The data will be returned big endian.
> + * @count: Number of registers
> + */
> +int pm80x_bulk_read(struct regmap *map, u32 reg, u8 *buf, int count)
> +{
> + return regmap_raw_read(map, reg, buf, count);
> +}
Unused function? Either export this if you want to provide it as
the general API, or drop the function.
> +int __devinit pm80x_device_init(struct pm80x_chip *chip,
> + struct pm80x_platform_data *pdata)
> +void __devexit pm80x_device_exit(struct pm80x_chip *chip)
> +SIMPLE_DEV_PM_OPS(pm80x_pm_ops, pm80x_suspend, pm80x_resume);
I would think that these need to be exported as well, at least if
you want the driver to be modular.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index e129c82..96dd2d7 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -20,6 +20,18 @@ config MFD_88PM860X
> select individual components like voltage regulators, RTC and
> battery-charger under the corresponding menus.
>
> +config MFD_88PM80X
> + bool "Support Marvell 88PM800/88PM805"
> + depends on I2C=y && GENERIC_HARDIRQS
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + select MFD_CORE
> + help
> + This supports for Marvell 88PM800/88PM805 Power Management IC.
> + This includes the I2C driver and the core APIs _only_, you have to
> + select individual components like voltage regulators, RTC and
> + battery-charger under the corresponding menus.
> +
> config MFD_SM501
> tristate "Support for Silicon Motion SM501"
>---help---
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 75f6ed6..dc2584e 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -3,7 +3,9 @@
> #
>
> 88pm860x-objs:= 88pm860x-core.o 88pm860x-i2c.o
> +88pm80x-objs := 88pm800-core.o 88pm805-core.o 88pm80x-i2c.o
> obj-$(CONFIG_MFD_88PM860X) += 88pm860x.o
> +obj-$(CONFIG_MFD_88PM80X)+= 88pm80x.o
> obj-$(CONFIG_MFD_SM501) += sm501.o
> obj-$(CONFIG_MFD_ASIC3) += asic3.o tmio_core.o
I just noticed that it can currently only be builtin. Is there a strict
requirement for that? If not, better make it a tristate option.
If you
Re: [PATCH 1/3] mfd: support 88pm80x in 80x driver
On Monday 02 July 2012, Qiao Zhou wrote: > On 07/02/2012 06:12 PM, Mark Brown wrote: > > On Mon, Jul 02, 2012 at 06:09:57PM +0800, Qiao Zhou wrote: > >> On 07/02/2012 06:03 PM, Mark Brown wrote: > > > >>> What do you mean by pages? regmap has paging support which just maps > >>> everything into a single flat register map from the point of view of > >>> callers. > > > >> Mark, let me explain: the 88pm800 chip has three i2c address > >> internally, which we called different page instead. it confuses you > >> with the register page_read/write operation. there are registers in > >> each i2c address domain, and we need to use different i2c client to > >> access reg in different domain. such as some common regs are in the > >> page of i2c_addr = 0x30, and power related regs are in the page of > >> i2c_addr = 0x31, and gpadc related regs are in the page of 0x32. > > > > These aren't what people normally call pages, those are just separate > > I2C devices from a Linux point of view. > > > Mark, surely I'll pay attention to the terms used. thanks! > due to there separate I2C devices, does it make sense to export separate > r/w interface for them? do you have suggestion in such case? (adding the i2c mailing list to get more insight) I think in case of device tree based probing, it would be straightforward to represent 88pm800 as a single device with three addresses in the "reg" property, while the natural linux representation would be one regular i2c_client device with two dummies. Do we or should we have any infrastructure to deal with this? If this is a common scenario, we could probably let regmap handle it entirely internally and represent the i2c client with its dummies as a single regmap. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ARM: Kirkwood: Add interrupt controller support for DT boards
On Thursday 14 June 2012, Andrew Lunn wrote: > > I think if you compute the number of interrupts in the domain from the > > number of > > registers that are described in the device tree, call orion_irq_init() > > based on those registers, and use irq domains for the gpio stuff as Rob > > suggested, > > this init_irq function can become completely generic to all orion platforms. > > I'm reworking the GPIO stuff at the moment, moving it to use IRQ > domains for its interrupts. That code will be generic to all Orion > platforms. I'm modeling the code on the AT91 GPIO handler, since that > has a similar structure, one hardware interrupt for a number of GPIO > lines, which is in software demultiplexed and triggers secondary level > interrupts. The major difference is that AT91 has one hardware > interrupt for a GPIO chip with 32 lines, where as Orion has 4 hardware > interrupt lines, so maybe four interrupt domains, for one GPIO chip > with 32 lines. My guess is that you're still better off with a single interrupt domain for 32 lines, or even for all the gpio lines, but I'm sure you can find a solution that works best for you. > Non-GPIO interrupts are more of a problem. Underneath it uses the > generic-chip interrupt code. The patchset to add _domain versions of > these calls is stalled. So i think for the moment, we need to stay > with the domain bolted on top, until generic-chip gets domain > support. I would also expect that generic-chip also gets a common DT > binding which any SoC can use. I suggested that before, and the comments I got back then were that we should treat the generic irq chip more like a library and keep the device tree binding at a higher level. If I understood things right, that means we would just have one plat-orion (or mach-mvebu later) irq chip abstraction that handles the DT binding and the irq domains itself but uses the generic irq chip code to implement the actual irq handling. > My aim at the moment is to get something which works well enough that > we can add DT support to other drivers. Without interrupts, we are not > going to get very far. We can later rip it out what i create now and > replace it once generic-chip becomes domain and DT aware, and there > should be no effect on other drivers. Right. I just want to ensure that we don't need to change the bindings in incompatible ways. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/9] ARM: kirkwood: use devicetree for orion-spi
On Thursday 14 June 2012, Andrew Lunn wrote: > We either have auxdata and clean clock code, or no auxdata and messy > clock code with lots of aliases. > > The auxdata is also not limited to the name of the clocks. It allows > me to diff the kernel log for a DT boot and a none DT boot of the same > box and see they are identical. I've found a few typo's that way, and > not been hindered by noise of the devices changing name is useful. > > Once we have the clock tree described in DT, then i think it makes > sense to remove these auxdata entries. > Ok, fair enough. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ARM: Kirkwood: Add interrupt controller support for DT boards
On Sunday 10 June 2012, Andrew Lunn wrote:
> +static int __init kirkwood_add_irq_domain(struct device_node *np,
> + struct device_node
> *interrupt_parent)
> +{
> + kirkwood_init_irq();
> + irq_domain_add_legacy(np, 64, 0, 0, &irq_domain_simple_ops, NULL);
> + return 0;
> +}
> +
> +static const struct of_device_id kirkwood_irq_match[] = {
> + { .compatible = "marvell,orion-intc",
> + .data = kirkwood_add_irq_domain, },
> + {},
> +};
> +
> +static void __init kirkwood_dt_init_irq(void)
> +{
> + of_irq_init(kirkwood_irq_match);
> +}
> +
I think if you compute the number of interrupts in the domain from the number of
registers that are described in the device tree, call orion_irq_init()
based on those registers, and use irq domains for the gpio stuff as Rob
suggested,
this init_irq function can become completely generic to all orion platforms.
That is what I tried to do with an earlier patch, which was flawed for other
reasons.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 2/3] i2c-nomadik: turn the platform driver to an amba driver
On Monday 11 June 2012, Alessandro Rubini wrote: > The i2c-nomadik gateware is really a PrimeCell APB device. By hosting > the driver under the amba bus we can access it more easily, for > example using the generic pci-amba driver. The patch also fixes the > mach-ux500 users, so they register an amba device instead than a > platform device. > > Signed-off-by: Alessandro Rubini > Acked-by: Giancarlo Asnaghi > Tested-by: Linus Walleij > --- > arch/arm/mach-ux500/devices-common.h | 22 + > drivers/i2c/busses/i2c-nomadik.c | 142 + > 2 files changed, 78 insertions(+), 86 deletions(-) You change only one half of ux500 here: the part where the device gets defined statically, but not not the definition in the device-tree. I think all you need to do is mark the device compatible with primecell and add an ID if necessary, but we should definitely make sure that that path still works, because the one you patch is going away. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/9] ARM: kirkwood: use devicetree for orion-spi
On Sunday 10 June 2012, Andrew Lunn wrote:
> @@ -26,6 +26,11 @@ static struct of_device_id kirkwood_dt_match_table[]
> __initdata = {
> { }
> };
>
> +struct of_dev_auxdata kirkwood_auxdata_lookup[] __initdata = {
> + OF_DEV_AUXDATA("marvell,orion-spi", 0xf1010600, "orion_spi.0", NULL),
> + {},
> +};
> +
> static void __init kirkwood_dt_init(void)
> {
How about instead adding the clkdev for "f1010600.spi" so you don't need the
auxdata?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree
On Monday 14 May 2012, Lee Jones wrote: > > You should be using of_regulator_match() for this (I think it's supposed > > to do an equivalent job...) rather than open coding. > > I've ripped this out completely and the code appears to continue be > fully functional. Happy days! :) Ok, very good! Of course it would still be helpful to understand the reasons of the person putting that code in originally, but I'm at least comfortable with leaving it out for now, and fixing any problems we see in a proper way after your code is merged. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 03/11] ASoC: imx-audmux: add pinctrl support
On Friday 11 May 2012, Linus Walleij wrote: > On Fri, May 11, 2012 at 7:32 AM, Shawn Guo wrote: > > > pinctrl-mergebase-20120418 is not enough for me. I need mxs and dummy > > states support. So if your devel branch will never be rebased, we can > > simply ask Arnd and Olof to get that as the dependent branch? > > Hm, yes the devel branch is getting real stable now, so I will only add things > on top. > > Basically the for-next branch is supposed to be even more stable, > but right now it's a copy of devel. > > ARM SoC guys: how do you want the pinctrl deps? I can make > a tag today if that is preferred. No need for a signed tag, since we're not going to submit you changes upstream. Knowing that we can pull in the for-next branch is good enough, so we'll do that for anything that needs it. > Also: short-cut to another subject: how have you other guys managed > this? By e.g.: > > git fetch > > git checkout -b my-pinwork v3.4-rc4 > git merge pinctrl-tag > (develop develop) > > Or: > > git checkout -b my-pinwork pinctrl-tag > > ? > > I was a bit uncertain on how to do this for the pending Ux500 > stuff so better ask. I merged it for now but maybe it's better if > I just base the whole pullrequest on top of a stable pinctrl > branch? No strong preference on my side. I've seen both ways getting done. In arm-soc we try to do a 'git pull --log --no-ff' for all changes that are being pulled into one of the main next/* branches, just like torvalds does when he pulls in branches from maintainers. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] drivers/mfd: Enable Device Tree for ab8500-core driver
On Wednesday 09 May 2012, Linus Walleij wrote:
> > + else if (np)
> > + ret = of_property_read_u32(np, "stericsson,irq-base",
> > &ab8500->irq_base);
> > +
> > + if (ab8500->irq_base == 0) {
>
> Shouldn't this be (av8500->irq_base == NO_IRQ) now that we're tranisitioning
> to
> use 0 as NO_IRQ?
Since we're into bike-shedding already, why not make it :? :-)
if (!ab8500->irq_base)
I usually prefer this syntax for testing if something has been assigned,
while I use a comparison with 0 only when it is a meant as a numeric value
rather than meant as validity test.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree
On Tuesday 08 May 2012, Mark Brown wrote: > On Tue, May 08, 2012 at 01:48:14PM +0000, Arnd Bergmann wrote: > > On Tuesday 08 May 2012, Lee Jones wrote: > > > > I did run this past Arnd before writing the code and he agreed that this > > > would be suitable; however, if you know of a better way in which I can > > > do this, I'd be pleased to hear of it. > > > It was the best approach that I could think of for this, but I'm also > > a total newbie on regulators. > > It's not really a regulator thing, what the code is doing is blasting in > a bunch of magic register writes to set things up, some of which are > configuring things that the subsystem already knows how to configure. Right, which is what the driver has done since 79568b9412 "regulator: initialization for ab8500 regulators" with your ack, so we decided not to change that and simply move the init data from platform code to the device tree. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree
On Tuesday 08 May 2012, Lee Jones wrote: > This piece of code plucks pre-defined initialisation values and from the > Device Tree and uses them to set-up regulator related registers on the > u8500. See 'struct ab8500_regulator_reg_init ab8500_regulator_reg_init' > in arch/arm/mach-ux500/board-mop500-regulators.c for reference. > > I did run this past Arnd before writing the code and he agreed that this > would be suitable; however, if you know of a better way in which I can > do this, I'd be pleased to hear of it. It was the best approach that I could think of for this, but I'm also a total newbie on regulators. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/15] DT enablement for Snowball
On Friday 04 May 2012, Lee Jones wrote: > > Here's your next back of DT related doings for the ux500, > along with some bugs encountered fixed along the way. > > arch/arm/boot/dts/db8500.dtsi | 103 +- > arch/arm/boot/dts/snowball.dts |3 + > arch/arm/configs/u8500_defconfig |1 + > arch/arm/mach-ux500/board-mop500.c | 55 ++-- > drivers/i2c/busses/i2c-nomadik.c | 53 ++- > drivers/mfd/Makefile |5 +- > drivers/mfd/ab8500-core.c | 165 +++--- > drivers/mfd/ab8500-i2c.c | 128 - > drivers/mfd/db8500-prcmu.c | 30 ++-- > drivers/power/ab8500_btemp.c | 12 +- > drivers/power/ab8500_charger.c | 12 +- > drivers/power/ab8500_fg.c | 12 +- > drivers/regulator/ab8500.c | 273 > +++- Looks all good, but I commented on two patches that I think can be done in a simpler way. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/15] mfd/ab8500: Remove confusing ab8500-i2c file and merge into ab8500-core
On Friday 04 May 2012, Lee Jones wrote:
>
> ab8500-i2c is used as core code to register the ab8500 device.
> After allocating ab8500 memory, it immediately calls into
> ab8500-core where the real initialisation takes place. This
> patch moves all core registration and memory allocation into
> the true ab8500-core file and removes ab8500-i2c completely.
>
> Signed-off-by: Lee Jones
These changes all look good, but I think I would go further here.
I believe we discussed this and I agreed that we could leave that
for later, but upon reading this code, I think now that it's getting
rather silly.
> @@ -125,6 +126,41 @@ static const char ab8500_version_str[][7] = {
> [AB8500_VERSION_AB8540] = "AB8540",
> };
>
> +static int ab8500_i2c_write(struct ab8500 *ab8500, u16 addr, u8 data)
> +{
> + int ret;
> +
> + ret = prcmu_abb_write((u8)(addr >> 8), (u8)(addr & 0xFF), &data, 1);
> + if (ret < 0)
> + dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
> + return ret;
> +}
> +
> +static int ab8500_i2c_write_masked(struct ab8500 *ab8500, u16 addr, u8 mask,
> + u8 data)
> +{
> + int ret;
> +
> + ret = prcmu_abb_write_masked((u8)(addr >> 8), (u8)(addr & 0xFF),
> &data,
> + &mask, 1);
> + if (ret < 0)
> + dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
> + return ret;
> +}
> +
> +static int ab8500_i2c_read(struct ab8500 *ab8500, u16 addr)
> +{
> + int ret;
> + u8 data;
> +
> + ret = prcmu_abb_read((u8)(addr >> 8), (u8)(addr & 0xFF), &data, 1);
> + if (ret < 0) {
> + dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
> + return ret;
> + }
> + return (int)data;
> +}
> +
> static int ab8500_get_chip_id(struct device *dev)
> {
> struct ab8500 *ab8500;
Each of these functions is called only from a single location, and through an
indirect
function pointer.
> + ab8500->read = ab8500_i2c_read;
> + ab8500->write = ab8500_i2c_write;
> + ab8500->write_masked = ab8500_i2c_write_masked;
Which you unconditionally assign here.
If you apply this patch below, then there is no reason to add any of those.
There is room for additional simplification even, but this is the most
important one. Note that the ab8500 mutex was only needed to support the
case where write_masked is not present, and that the debug output
on error is pointless because the prcmu driver already writes the same
output. The next step would be to remove all the {get,set}_register functions
from ab8500 and just call the prcmu directly.
Signed-off-by: Arnd Bergmann
---
drivers/mfd/ab8500-core.c | 72
+++-
include/linux/mfd/abx500/ab8500.h |1 -
2 files changed, 3 insertions(+), 70 deletions(-)
diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 1f08704..b10bd2f 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -138,24 +138,8 @@ static int ab8500_get_chip_id(struct device *dev)
static int set_register_interruptible(struct ab8500 *ab8500, u8 bank,
u8 reg, u8 data)
{
- int ret;
- /*
-* Put the u8 bank and u8 register together into a an u16.
-* The bank on higher 8 bits and register in lower 8 bits.
-* */
- u16 addr = ((u16)bank) << 8 | reg;
-
dev_vdbg(ab8500->dev, "wr: addr %#x <= %#x\n", addr, data);
-
- mutex_lock(&ab8500->lock);
-
- ret = ab8500->write(ab8500, addr, data);
- if (ret < 0)
- dev_err(ab8500->dev, "failed to write reg %#x: %d\n",
- addr, ret);
- mutex_unlock(&ab8500->lock);
-
- return ret;
+ return prcmu_abb_write(bank, reg, &data, 1);
}
static int ab8500_set_register(struct device *dev, u8 bank,
@@ -169,21 +153,7 @@ static int ab8500_set_register(struct device *dev, u8 bank,
static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
u8 reg, u8 *value)
{
- int ret;
- /* put the u8 bank and u8 reg together into a an u16.
-* bank on higher 8 bits and reg in lower */
- u16 addr = ((u16)bank) << 8 | reg;
-
- mutex_lock(&ab8500->lock);
-
- ret = ab8500->read(ab8500, addr);
- if (ret < 0)
- dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
- addr, ret);
- else
- *value = ret;
-
- mutex_unlock(&ab8500->lock);
+ ret = prcmu_abb_read(bank, addr, value, 1);
dev_vdbg(ab8500->dev, &quo
Re: [PATCH 01/15] i2c/busses: Add Device Tree support to the Nomadik I2C driver
On Friday 04 May 2012, Lee Jones wrote:
> +static const struct nmk_i2c_controller *
> +nmk_i2c_find_pdata_from_compatible(struct device_node *np)
> +{
> + /*
> +* The u8500 is currently our only user. As more SoCs are added,
> +* search for the correct value set using of_machine_is_compatible
> +* and return a 'struct nmk_i2c_controller *' which contains the
> +* correct information for the given SoC, whilst leaving u8500_i2c
> +* as the default/fall-back value set.
> +*/
> + return &u8500_i2c;
> +}
Why not just put this pointer ...
> +static const struct of_device_id nmk_gpio_match[] = {
> + { .compatible = "st,nomadik-i2c", },
> + {}
> +};
into the .data field after the .compatible match, and make it more specific
to the soc, i.e.
static const struct of_device_id nmk_gpio_match[] = {
{ .compatible = "st-ericsson,u8500-i2c", .data = &u8500_i2c },
{ .compatible = "st,nomadik-i2c", .data = &default_i2c_controller },
};
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/8]: arm: lpc32xx: Device tree support
On Monday 02 April 2012, Roland Stigge wrote: > This is the first series of patches to introduce device tree support for the > LPC32xx SoC. This series includes patches for the various subsystems to > support > device tree to be used later by the machine's initialization. > > The patches apply to various subsystems: > > * staging/iio/adc (1) > * rtc (1) > * net (1) > * wdt (1) > * i2c (4) > * arm-soc (i2c related in the former, to be merged via i2c, as suggested by > Arnd Bergmann) Looks good, please add Reviewed-by: Arnd Bergmann for the entire series. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/8] net: Add device tree support to LPC32xx
On Monday 02 April 2012, Roland Stigge wrote: > > + if (pdev->dev.of_node && of_get_property(pdev->dev.of_node, > +"use-iram", NULL)) > + pldat->use_iram = true; > + else > + pldat->use_iram = false; > + One more thing I just noticed: this is equivalent to the much shorter pldat->use_iram = of_property_read_bool(pdev->dev.of_node, "use-iram"); Everything else looks good here. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
