Re: [PATCH RFC 2/2] mfd: add Renesas RPC-IF driver

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Sergei, Lee,

On Thu, Jun 20, 2019 at 8:46 PM Sergei Shtylyov
<sergei.shtylyov@xxxxxxxxxxxxxxxxxx> wrote:
> On 06/17/2019 12:30 PM, Lee Jones wrote:
> >>>> Add the MFD driver for Renesas RPC-IF which registers either the SPI or
> >>>> HyperFLash  device depending on the contents of the device tree subnode.
> >>>> It also provides the absract "back end" device APIs that can be used by
> >>>> the "front end" SPI/MTD drivers to talk to the real hardware.
> >>>>
> >>>> Based on the original patch by Mason Yang <masonccyang@xxxxxxxxxxx>.
> >>>>
> >>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
> >>
> >> [...]
> >>>> Index: mfd/drivers/mfd/rpc-if.c
> >>>> ===================================================================
> >>>> --- /dev/null
> >>>> +++ mfd/drivers/mfd/rpc-if.c
> >>>> @@ -0,0 +1,535 @@
> [...]
> >>>> +#define RPCIF_DIRMAP_SIZE 0x4000000
> >>>
> >>> Can you shift this lot out to a header file please.
> >>
> >>    You mean all register #defne's? Why? I'm not intending to use them outside
> >> this file.
> >
> > Because its 10's of lines of cruft.
>
>    Thank you! :-)
>
> > People won't want to wade through that to get to real functional C
> > code every time they open up this file.
>
>    This is how the most drivers are written.
>
> > You already have a header file, please use it.
>
>    Headers are for public things. I've encapsulated the h/w assess into
> the MFD driver, the client code doesn't have to see all the gory hardware
> details... IOW, I don't agree to this request.

+1

> >>>> +static int wait_msg_xfer_end(struct rpcif *rpc)
> >>>> +{
> >>>> +  u32 sts;
> >>>> +
> >>>> +  return regmap_read_poll_timeout(rpc->regmap, RPCIF_CMNSR, sts,
> >>>> +                                  sts & RPCIF_CMNSR_TEND, 0,
> >>>
> >>> Aren't you using sts undefined here?
> >>
> >>    No, the macro reads 'sts' from the register first.
> >
> > That's confusing and ugly.
> >
> > Please re-write it to the code is clear and easy to read/maintain.

How to rewrite?
This is exactly how the various *_poll_timeout*() helpers are intended
to be used.

 * @val: Unsigned integer variable to read the value into

See also include/linux/iopoll.h.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds