Re: [PATCH v7 2/6] drivers: mfd: Add a driver for IEI WT61P803 PUZZLE MCU

[Andy Shevchenko]

On Tue, Nov 3, 2020 at 1:15 AM Luka Kovacic <luka.kovacic@xxxxxxxxxx> wrote:
> On Mon, Nov 2, 2020 at 12:18 PM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> > On Sun, Nov 1, 2020 at 3:22 PM Luka Kovacic <luka.kovacic@xxxxxxxxxx> wrote:
> > > On Mon, Oct 26, 2020 at 11:54 PM Andy Shevchenko
> > > <andy.shevchenko@xxxxxxxxx> wrote:
> > > > On Sun, Oct 25, 2020 at 3:59 AM Luka Kovacic <luka.kovacic@xxxxxxxxxx> wrote:

...

> > > > > +       /* Response format:
> > > > > +        * (IDX RESPONSE)
> > > > > +        * 0    @
> > > > > +        * 1    O
> > > > > +        * 2    S
> > > > > +        * 3    S
> > > > > +        * ...
> > > > > +        * 5    AC Recovery Status Flag
> > > > > +        * ...
> > > > > +        * 10   Power Loss Recovery
> > > > > +        * ...
> > > > > +        * 19   Power Status (system power on method)
> > > > > +        * 20   XOR checksum
> > > > > +        */
> > > >
> > > > Shouldn't be rather defined data structure for response?
> > >
> > > Every response, apart from the standard headers and a checksum
> > > at the end is completely different and I don't see a good way to
> > > standardize that in some other way.
> >
> > And that's my point. Provide data structures for all responses you are
> > taking care of.
> > It will be way better documentation and understanding of this IPC.
>
> Okay, I'll improve handling of these in the next patchset.
> Should I make a generic header structure for the common parts and
> define the common responses somewhere centrally?

Yes, something like TCP/IP headers have.
This will immediately show how good/bad was design of this IPC
protocol (as a side effect, but gives a good hint on how layers of
messages are looking )

> Then I can check those just as you suggested.
>
> For the variable ones I can reuse the generic header structure and just
> use the specific values as I would do normally.

...

> > > > > +               if (!(resp_buf[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_START &&
> > > > > +                     resp_buf[1] == IEI_WT61P803_PUZZLE_CMD_RESPONSE_OK &&
> > > > > +                     resp_buf[2] == IEI_WT61P803_PUZZLE_CHECKSUM_RESPONSE_OK)) {
> > > > > +                       ret = -EPROTO;
> > > > > +                       goto err;
> > > > > +               }
> > > >
> > > > I think it would be better to define data structure for replies and
> > > > then check would be as simple as memcmp().
> > >
> > > I'd keep this as is, because the replies are different a lot of the times.
> > > Especially when the reply isn't just an ACK.
> >
> > How do you know the type of the reply? Can't you provide a data
> > structure which will have necessary fields to recognize this?
> >
>
> It can be recognized by the specific header of the reply.
> I will separate the header and the checksum into some kind of a generic
> structure, but as the content itself is just an arbitrary array of characters
> I cannot generalize that sensibly for every type of a reply there is.
>
> Anyway, I agree it would be good to define the common responses...

Yep, something to look like a structure with a payload.

...

> Can I output the versions, the firmware build info and only print the baud
> rate when an error occurs?

What you think is crucial. I'm not against it, I'm just pointing to
the way of shrinking as much as possible. Otherwise, move messages to
debug level (but that shouldn't be many in the production driver).

-- 
With Best Regards,
Andy Shevchenko