Re: [H5400-port] [PATCH] Reset MQ11xx chip before any initialization

Hello,

On Thu, 24 Jan 2008 21:55:24 +0100
Milan Plzik <milan.plzik_at_gmail.com> wrote:

>
> On Št, 2008-01-24 at 21:45 +0200, Paul Sokolovsky wrote:
> > Hello Milan,
> >
> > Thursday, January 24, 2008, 10:58:11 AM, you wrote:
> >
> > > mq11xx_base driver (at least mq1100fb using mq11xx_base portion)
> >
> > > From: Milan Plzik <milan.plzik_at_gmail.com>
> >
> > > tends to malfunction on h5000 series after booting linux from
> > > haret, or reseting device with active framebuffer. This patch
> > > adds mq11xx reset to the start of initialization routine, so chip
> > > is always in well-defined state.
> > > ---
> >
> > > drivers/mfd/mq11xx_base.c | 4 ++++
> > > 1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > > diff --git a/drivers/mfd/mq11xx_base.c b/drivers/mfd/mq11xx_base.c
> > > index 91a1d98..0506d56 100644
> > > --- a/drivers/mfd/mq11xx_base.c
> > > +++ b/drivers/mfd/mq11xx_base.c
> > > @@ -435,6 +435,10 @@ mq11xx_init (struct mq_data *mqdata)
> > > endian = endian | (endian << 4);
> > > endian = endian | (endian << 8);
> > > *((u16 *)&mqdata->base.regs->DC.config_0) = endian;
> > > +
> > > + /* Reset the chip, so we don't need to worry about
> > > previous state */
> > > + mqdata->base.regs->DC.config_1 |=
> > > MQ_CONFIG_SOFTWARE_CHIP_RESET; +
> >
> > Here apparently should be some delay, no?
>
> It works correctly without delay

How do you know that? Tested it in a loop for million times? At least
thousand (not enough to be sure, but at least would give some
heuristical picture).

> as well and documentation
> unfortunately doesn't say anything (there is nothing written about
> delays at all).

So, then there's engineering common sense, which tells that reset of a
complex device may be a long operation.

> 100ms should be sufficient for almost anything, but I
> don't like using ad-hoc constants, so I did not include any mdelay.

Well, this logic appears to be faulty to me ;-). And we still speak
about *delay*, not about *duration* of it, so "some" delay should
suffice ;-).

> And therefore there is also request to test it (afaik h2220 also uses
> mq11xx_base.c:)

Well, making changes, we should make code formally correct (or not
incorrect), and not just rely on subjective processes like testing -
some thing are extremely hard to test. In our situation, testing
make take years.

>
> >
> > >
> > > /* First of all, enable the oscillator clock */
> > > mqdata->base.regs->DC.config_1 = mqdata->mq_init->DC [1];
> > > /* Wait for oscillator to run - 30ms doesnt suffice */

Ok, looking at the patch further, your patch adds reset after setting
up endianness on chip. Doesn't seem too good either, but based on the
code logic, I decided not to try and change it.

Committed, thanks!

> >
> >
>
> Milan
>
>

-- 
Best regards,
 Paul                          mailto:pmiscml_at_gmail.com