[PATCH 1/3] mfd: ucb1x00: add irq field to the platform data

[PATCH 1/3] mfd: ucb1x00: add irq field to the platform data

[Dmitry Eremin-Solenikov]

To allow boards to specify the irq that is used by UCB1x00 chip, add irq
field to the platform data structure.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 include/linux/mfd/ucb1x00.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/mfd/ucb1x00.h b/include/linux/mfd/ucb1x00.h
index e1345ff..9a2dacb 100644
--- a/include/linux/mfd/ucb1x00.h
+++ b/include/linux/mfd/ucb1x00.h
@@ -118,6 +118,7 @@ struct ucb1x00_plat_data {
 	unsigned		irq_base;
 	int			gpio_base;
 	unsigned		can_wakeup;
+	int			irq;
 };
 
 struct ucb1x00 {
-- 
2.1.4

[PATCH 2/3] ARM: sa1100: populate codec platform data with irq

[Dmitry Eremin-Solenikov]

For the sa11x0 boards that provide ucb1x00 codec platform data, provide
the irq data.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 arch/arm/mach-sa1100/assabet.c | 1 +
 arch/arm/mach-sa1100/collie.c  | 1 +
 arch/arm/mach-sa1100/simpad.c  | 1 +
 3 files changed, 3 insertions(+)

diff --git a/arch/arm/mach-sa1100/assabet.c b/arch/arm/mach-sa1100/assabet.c
index d28ecb9..28bb7ca 100644
--- a/arch/arm/mach-sa1100/assabet.c
+++ b/arch/arm/mach-sa1100/assabet.c
@@ -309,6 +309,7 @@ static struct ucb1x00_plat_data assabet_ucb1x00_data = {
 	.reset		= assabet_ucb1x00_reset,
 	.gpio_base	= -1,
 	.can_wakeup	= 1,
+	.irq		= IRQ_GPIO23,
 };
 
 static struct mcp_plat_data assabet_mcp_data = {
diff --git a/arch/arm/mach-sa1100/collie.c b/arch/arm/mach-sa1100/collie.c
index 98710ea..cb1e7d7 100644
--- a/arch/arm/mach-sa1100/collie.c
+++ b/arch/arm/mach-sa1100/collie.c
@@ -97,6 +97,7 @@ static struct scoop_pcmcia_config collie_pcmcia_config = {
 
 static struct ucb1x00_plat_data collie_ucb1x00_data = {
 	.gpio_base	= COLLIE_TC35143_GPIO_BASE,
+	.irq		= COLLIE_IRQ_GPIO_UCB1x00_IRQ,
 };
 
 static struct mcp_plat_data collie_mcp_data = {
diff --git a/arch/arm/mach-sa1100/simpad.c b/arch/arm/mach-sa1100/simpad.c
index 41e476e..117281c 100644
--- a/arch/arm/mach-sa1100/simpad.c
+++ b/arch/arm/mach-sa1100/simpad.c
@@ -184,6 +184,7 @@ static struct resource simpad_flash_resources [] = {
 
 static struct ucb1x00_plat_data simpad_ucb1x00_data = {
 	.gpio_base	= SIMPAD_UCB1X00_GPIO_BASE,
+	.irq		= IRQ_GPIO_UCB1300_IRQ,
 };
 
 static struct mcp_plat_data simpad_mcp_data = {
-- 
2.1.4

[PATCH 3/3] mfd: ucb1x00: make use of provided irq

[Dmitry Eremin-Solenikov]

If the platform data provides IRQ information, use that instead always
probing the IRQ.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 drivers/mfd/ucb1x00-core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/ucb1x00-core.c b/drivers/mfd/ucb1x00-core.c
index cd4d39e..9b3c86b 100644
--- a/drivers/mfd/ucb1x00-core.c
+++ b/drivers/mfd/ucb1x00-core.c
@@ -465,9 +465,12 @@ static int ucb1x00_probe(struct mcp *mcp)
 	if (ret)
 		goto err_dev_add;
 
-	ucb1x00_enable(ucb);
-	ucb->irq = ucb1x00_detect_irq(ucb);
-	ucb1x00_disable(ucb);
+	ucb->irq = pdata ? pdata->irq : NO_IRQ;
+	if (ucb->irq == NO_IRQ) {
+		ucb1x00_enable(ucb);
+		ucb->irq = ucb1x00_detect_irq(ucb);
+		ucb1x00_disable(ucb);
+	}
 	if (ucb->irq == NO_IRQ) {
 		dev_err(&ucb->dev, "IRQ probe failed\n");
 		ret = -ENODEV;
-- 
2.1.4

[PATCH 2/3] ARM: sa1100: populate codec platform data with irq

[Russell King - ARM Linux]

On Thu, Apr 02, 2015 at 06:41:46PM +0300, Dmitry Eremin-Solenikov wrote:
> For the sa11x0 boards that provide ucb1x00 codec platform data, provide
> the irq data.

This isn't necessary.  The ucb1x00 can find its IRQ on its own.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/3] ARM: sa1100: populate codec platform data with irq

[Dmitry Eremin-Solenikov]

2015-04-02 19:00 GMT+03:00 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Thu, Apr 02, 2015 at 06:41:46PM +0300, Dmitry Eremin-Solenikov wrote:
>> For the sa11x0 boards that provide ucb1x00 codec platform data, provide
>> the irq data.
>
> This isn't necessary.  The ucb1x00 can find its IRQ on its own.

Yes, it can. But if it's a static configuration, why can't it be
configured statically?



-- 
With best wishes
Dmitry

[PATCH 1/3] mfd: ucb1x00: add irq field to the platform data

[Russell King - ARM Linux]

Just like has already been pointed out on the list once today, we need
much better commit descriptions than this.

The code has been perfectly happy to auto-detect the IRQ for over ten
years.  Why do we suddenly need to change it now?  What's the
justification for this change.

Or is this just a case of "because we can" and you happen to have a
different opinion on how stuff should be done from how it's been
successfully done for the last ten years?

On Thu, Apr 02, 2015 at 06:41:45PM +0300, Dmitry Eremin-Solenikov wrote:
> To allow boards to specify the irq that is used by UCB1x00 chip, add irq
> field to the platform data structure.
> 
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
>  include/linux/mfd/ucb1x00.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/mfd/ucb1x00.h b/include/linux/mfd/ucb1x00.h
> index e1345ff..9a2dacb 100644
> --- a/include/linux/mfd/ucb1x00.h
> +++ b/include/linux/mfd/ucb1x00.h
> @@ -118,6 +118,7 @@ struct ucb1x00_plat_data {
>  	unsigned		irq_base;
>  	int			gpio_base;
>  	unsigned		can_wakeup;
> +	int			irq;
>  };
>  
>  struct ucb1x00 {
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/3] ARM: sa1100: populate codec platform data with irq

[Russell King - ARM Linux]

On Thu, Apr 02, 2015 at 07:01:52PM +0300, Dmitry Eremin-Solenikov wrote:
> 2015-04-02 19:00 GMT+03:00 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > On Thu, Apr 02, 2015 at 06:41:46PM +0300, Dmitry Eremin-Solenikov wrote:
> >> For the sa11x0 boards that provide ucb1x00 codec platform data, provide
> >> the irq data.
> >
> > This isn't necessary.  The ucb1x00 can find its IRQ on its own.
> 
> Yes, it can. But if it's a static configuration, why can't it be
> configured statically?

Why change something that's worked for ages?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/3] mfd: ucb1x00: add irq field to the platform data

[Dmitry Eremin-Solenikov]

Hello,

2015-04-02 19:02 GMT+03:00 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> Just like has already been pointed out on the list once today, we need
> much better commit descriptions than this.
>
> The code has been perfectly happy to auto-detect the IRQ for over ten
> years.  Why do we suddenly need to change it now?  What's the
> justification for this change.

Because using static configuration would allow me to drop a nice piece
of code. Because it would allow later to use dts to describe ucb1x00
configuration. Because I will no longer have to think whether ucb1x00I h
driver barfing on IRQ is a problem of a chip, of an IRQ or just me having
touched wrong GPIO line at wrong time with a scope probe. Yes, I didn't
put all these causes to the commit description. Do you really want it?

>
> Or is this just a case of "because we can" and you happen to have a
> different opinion on how stuff should be done from how it's been
> successfully done for the last ten years?

Not only "because we can", but "because we should". In my humble
opinion.

It looks like the whole story of me touching sa1100 is like fighting with
'it was done so for ages' windmills. Yes, I have different opinions sometimes.
I would like for sa1100 to converge to other platforms. PXA, especially.
Why? Because I like several small devices sitting on my table. PXA
is now slowly moving towards dts, cleaner drivers and cleaner interfaces.
We should have done this ages ago. Nobody had time and interest.
SA-11x0 had even older drivers. Older implementations. Older ideas.
Yes, I have a different opinion at this place. I'd like for sa1100 to live
in a contemporary world. I'd like to have my devices work with the rest
of Linux subsystems. Should I just fork sa1100 subarch, make it work
with devices I have at hand and submit it as a 'new' subarch?
Or we can have an evolution-like process that will make sa1100 live
in style with the rest of the Linux kernel.

>
> On Thu, Apr 02, 2015 at 06:41:45PM +0300, Dmitry Eremin-Solenikov wrote:
>> To allow boards to specify the irq that is used by UCB1x00 chip, add irq
>> field to the platform data structure.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>> ---
>>  include/linux/mfd/ucb1x00.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/mfd/ucb1x00.h b/include/linux/mfd/ucb1x00.h
>> index e1345ff..9a2dacb 100644
>> --- a/include/linux/mfd/ucb1x00.h
>> +++ b/include/linux/mfd/ucb1x00.h
>> @@ -118,6 +118,7 @@ struct ucb1x00_plat_data {
>>       unsigned                irq_base;
>>       int                     gpio_base;
>>       unsigned                can_wakeup;
>> +     int                     irq;
>>  };
>>
>>  struct ucb1x00 {
>> --
>> 2.1.4
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> --
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.



-- 
With best wishes
Dmitry

[PATCH 1/3] mfd: ucb1x00: add irq field to the platform data

[Russell King - ARM Linux]

On Thu, Apr 02, 2015 at 07:27:24PM +0300, Dmitry Eremin-Solenikov wrote:
> Because using static configuration would allow me to drop a nice piece
> of code. Because it would allow later to use dts to describe ucb1x00
> configuration. Because I will no longer have to think whether ucb1x00I h
> driver barfing on IRQ is a problem of a chip, of an IRQ or just me having
> touched wrong GPIO line at wrong time with a scope probe. Yes, I didn't
> put all these causes to the commit description. Do you really want it?

What I want is a decent commit description, one which isn't airy fairy
and looks like it's just mindless churn.  That's all.

It's what every other kernel developer wants to know.  This is precisely
what the ARM ecosystem has been soo bad at for decades.

It matters.  It's one of the things what pisses Linus off.  What Linus
sees from ARM people is lots and lots of apparently pointless churn
which he doesn't understand - and part of that problem is that ARM
people do _not_ justify in their commit messages why a change is
necessary.

So please, improve the commit messages and justify the commits.

It really doesn't help that all that was posted was three patches and
no covering message which could either explain what the three patches
were trying to achieve.  Like most people do.  (And some people take
that to an extreme, posting a cover message for just one patch.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/3] mfd: ucb1x00: add irq field to the platform data

[Russell King - ARM Linux]

On Thu, Apr 02, 2015 at 07:27:24PM +0300, Dmitry Eremin-Solenikov wrote:
> Not only "because we can", but "because we should". In my humble
> opinion.
> 
> It looks like the whole story of me touching sa1100 is like fighting with
> 'it was done so for ages' windmills. Yes, I have different opinions sometimes.

Right, and what you're forgetting is that others may have different
opinions.  Like me - and as I'm the sa1100 and ucb1x00 maintainer,
if you have a different opinion, you need to convince me that your
solution is the right thing to do.

Had I thought that passing an IRQ through platform data would be a
good thing, then I would've done it from the outset.

On principle, I don't like the idea of passing driver resources
through platform data, I find it abhorrent.  However, there isn't
really any good solution here other than platform data or
auto-detecting it, and auto-detecting it is, in my opinion, the
lesser of the evils.

> I would like for sa1100 to converge to other platforms. PXA, especially.
> Why? Because I like several small devices sitting on my table. PXA
> is now slowly moving towards dts, cleaner drivers and cleaner interfaces.

Right, so when the UCB1x00 gets a node in DT, the driver can parse
the interrupt from DT, without needing it in platform data - and
if it finds that it's in DT, then there's no need to auto-probe it.

I don't see the point of adding it to platform data only to later
have to change it /again/ when SA11x0 moves to DT.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.