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

On St, 2008-02-06 at 11:47 +0200, Paul Sokolovsky wrote:
> 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).

  Uhm, well, not. I just tested the driver and it works, I forgot to
think about that in bigger image. :(

>
> > 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 ;-).

  Yes, it is, because it is just pure guessing from my side. But as I
said, documentation isn't more verbose either, so this is the only thing
I'm able to do from my point of view.

>
> > 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.

  Again I agree, but what could I do in this case? I could either put
there mdelay(47) and hope it will be the right constant, or leave it
this way.

<dont_take_too_seriously>
  With some reverse logic, it can even be good -- if my approach is bad,
this will likely lead to problems pretty soon and they will be more
visible. With mdelay, the number of cases where problems would be
visible is pretty small, and thus harder to debug
</dont_take_too_seriously>

>
> >
> > >
> > > >
> > > > /* 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.

  I believe this should not be problem (and I should also mention this
in comments inside original patch), documentation for RESET bit say:

"Software chip reset. Setting this bit resets the chip except for the
bus-interface unit."

  Although I have not found exact definifion of bus-interface unit, I
think endianess belongs here as well.

>
> Committed, thanks!
>

  Given objections above, I will submit one more patch, which will try
to address them -- some are quite serious and I have not realized what
impact could they have.

>
> > >
> > >
> >
> > Milan
> >
> >
>
>
>
Received on Wed Feb 06 2008 - 05:37:40 EST