Re: [PATCH] i2c: mv64xxx: Fix compilation breakage

Re: [PATCH] i2c: mv64xxx: Fix compilation breakage

[Paul Gortmaker]

On Mon, Mar 10, 2014 at 7:29 AM, Russell King - ARM Linux
<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
> On Mon, Mar 10, 2014 at 11:58:08AM +0100, Maxime Ripard wrote:
>> On Fri, Mar 07, 2014 at 04:08:36PM +0000, 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.

Paul.
--

Bisecting: 0 revisions left to test after this (roughly 0 steps)
[80c69915e5fbe6493119d87eee2a2a6a7115c74c] i2c: mv64xxx: fix circular
Kconfig dependency
running ./x
#
# configuration written to .config
#
drivers/built-in.o: In function `mv64xxx_i2c_remove':
/home/paul/git/linux-head/drivers/i2c/busses/i2c-mv64xxx.c:924:
undefined reference to `reset_control_assert'
drivers/built-in.o: In function `mv64xxx_i2c_probe':
/home/paul/git/linux-head/drivers/i2c/busses/i2c-mv64xxx.c:904:
undefined reference to `reset_control_assert'
drivers/built-in.o: In function `mv64xxx_of_config':
/home/paul/git/linux-head/drivers/i2c/busses/i2c-mv64xxx.c:771:
undefined reference to `devm_reset_control_get'
/home/paul/git/linux-head/drivers/i2c/busses/i2c-mv64xxx.c:778:
undefined reference to `reset_control_deassert'
make: *** [vmlinux] Error 1
80c69915e5fbe6493119d87eee2a2a6a7115c74c is the first bad commit
commit 80c69915e5fbe6493119d87eee2a2a6a7115c74c
Author: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Date:   Thu Mar 6 10:08:50 2014 +0100

    i2c: mv64xxx: fix circular Kconfig dependency

    Commit 370136bc67c3 ("i2c: mv64xxx: Add reset deassert call")
    introduced:

    drivers/video/Kconfig:42:error: recursive dependency detected!

    ARCH_SUNXI selects RESET_CONTROLLER anyhow.

    Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>

:040000 040000 533a0ca6b40f2dd1d0b3bb434e6ed13ff4796953
cfd47b9ad19651148a2d0d4fa3a4df0b8cbbe1df M    drivers
bisect run success




>
> --
> FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
> improving, and getting towards what was expected from it.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] i2c: mv64xxx: Fix compilation breakage

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

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
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Mar 10, 2014 at 11:58:08AM +0100, Maxime Ripard wrote:
> >> On Fri, Mar 07, 2014 at 04:08:36PM +0000, 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.

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] i2c: mv64xxx: Fix compilation breakage

[Arnd Bergmann]

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
> > <linux@arm.linux.org.uk> wrote:
> > > On Mon, Mar 10, 2014 at 11:58:08AM +0100, Maxime Ripard wrote:
> > >> On Fri, Mar 07, 2014 at 04:08:36PM +0000, 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

Re: [PATCH] i2c: mv64xxx: Fix compilation breakage

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3548 bytes --]

On Sat, Mar 22, 2014 at 12:11:24PM +0100, Arnd Bergmann wrote:
> 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
> > > <linux@arm.linux.org.uk> wrote:
> > > > On Mon, Mar 10, 2014 at 11:58:08AM +0100, Maxime Ripard wrote:
> > > >> On Fri, Mar 07, 2014 at 04:08:36PM +0000, 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.

Actually, it's not the reset framework but the driver itself that
needs this. The framework will always return an error pointer here,
but we won't ever call reset_control_get_optional if we are not probed
with DT, and in that case, we will have NULL is data->rstc, hence why
we need to use IS_ERR_OR_NULL.

We should probably fix the reset functions, but maybe that can come
later so that we have marvell's defconfig fixed?

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] i2c: mv64xxx: Fix compilation breakage

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]


> > 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.
> 
> Actually, it's not the reset framework but the driver itself that
> needs this. The framework will always return an error pointer here,
> but we won't ever call reset_control_get_optional if we are not probed
> with DT, and in that case, we will have NULL is data->rstc, hence why
> we need to use IS_ERR_OR_NULL.
> 
> We should probably fix the reset functions, but maybe that can come
> later so that we have marvell's defconfig fixed?

Yes, let's fix that incrementally.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] i2c: mv64xxx: Fix compilation breakage

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]


> > 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'd love to apply the fix but the reset framework update is not in
linux-next yet.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] i2c: mv64xxx: Fix compilation breakage

[Gregory CLEMENT]

Arnd, Olof, Kevin,

On 24/03/2014 14:33, Wolfram Sang wrote:
> 
>>> 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'd love to apply the fix but the reset framework update is not in
> linux-next yet.
> 

I saw the there was a pull request resent last week. Is there any
reason to not have pulled it, yet?
http://www.spinics.net/lists/arm-kernel/msg316518.html

I would really like that it was sorted out, because all the build using
the mvebu related defconfig fails since two weeks because of it now.

I am ready to help if you need it.

Thanks!


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com