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

[Andy Shevchenko]

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.

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

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

...

> > > +               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?

...

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

...

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

-- 
With Best Regards,
Andy Shevchenko