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

[Lee Jones <lee.jones@xxxxxxxxxx>]

On Fri, 21 Jun 2019, Geert Uytterhoeven wrote:

> 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

Header files aren't only for sharing.  Plenty of source files have
their own headers for storing defines which are of little use to the
reader.

Keeping 125 lines of defines at the top of a source file is pretty
ugly and only border-line unsociable.  If you had many more, I'd be
more insistent.

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

Yuck!  What a horrible way to write a function.

Well if this is how the API is meant to called then I guess it is not
you I am taking exception to.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog