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

[Luka Kovacic]

Hello Andy,

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:
>
> ...
>
> > > > +#include <linux/of_device.h>
> > >
> > > Don't see a user of this, but of_platform.h seems to be missed.
> >
> > Okay, I'll add it.
> > I'm still using devm_of_platform_populate() in iei_wt61p803_puzzle_probe().
>
> Yes, and that's why I have mentioned of_platform.h above.
>
> ...
>
> > > > +       struct kobject *kobj;
> > >
> > > It's quite strange you need this,
> >
> > This is used in iei_wt61p803_puzzle_sysfs_create() and
> > iei_wt61p803_puzzle_sysfs_remove() to clean up afterwards.
>
> I didn't get why you need this in the first place.
>
> ...
>
> > > > +       /* Return the number of processed bytes if function returns error */
> > > > +       if (ret < 0)
> > >
> > > > +               return (int)size;
> > >
> > > Will be interesting result, maybe you wanted other way around?
> >
> > That is intentional.
> > A single frame is concatenated in the iei_wt61p803_puzzle_process_resp()
> > function. In case we find ourselves in an unknown state, an error is
> > returned there.
> >
> > We want to discard the remaining incoming data, since the frame this
> > data belongs
> > to is broken anyway.
>
> Elaborate in the comment.

Okay.

>
> > > > +       return ret;
>
> ...
>
> > > > +err:
> > >
> > > err_unlock: ?
> >
> > I use goto only in case there is also a mutex to unlock, so I don't see why
> > to change this.
>
> The comment was about clarification of what is done at this label.

Ok, I understand. I will change the labels where mutexes are unlocked to
indicate this as you suggested.

>
> > > > +       mutex_unlock(&mcu->lock);
> > > > +       return ret;
>
> ...
>
> > > > +       /* 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?
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...

> ...
>
> > > > +       power_loss_recovery_cmd[3] = cmd_buf[0];
> > >
> > > One decimal (most significant) digit?! Isn't it a bit ambiguous?
> >
> > The power_loss_recovery_action can only have a value of 0 - 4.
> > My understanding is that if I give snprintf a buffer of size 1, it will
> > truncate the one character to make space for NUL.
>
> Why to bother with snprintf()? hex_asc[] would be sufficient. But my
> point that the code is fragile. If it ever gets 15, you will get 1.

Ok, I'll simplify this.

>
> ...
>
> > I can reduce this, but I'd just like to log the baud rate and the
> > firmware build info.
>
> > These two could be useful in a kernel log, if something doesn't work.
>
> FW build info is definitely good to have, but don't you have other
> means to retrieve baud rate?

The normal boot log is completely clean, there are no serdev bus specific
messages.

The baud rate is defined in the device tree, so if something goes wrong it
should be sufficient to only print the baud rate then.
Can I output the versions, the firmware build info and only print the baud
rate when an error occurs?

>
> --
> With Best Regards,
> Andy Shevchenko