Re: [PATCH v5 2/5] mfd: Add support for the Lantiq PEF2256 framer

[Herve Codina]

Hi Krzysztof, Lee

On Fri, 31 Mar 2023 11:13:30 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:

> On 31/03/2023 09:42, Herve Codina wrote:
> > Hi Lee,
> > 
> > On Thu, 30 Mar 2023 17:05:10 +0100
> > Lee Jones <lee@xxxxxxxxxx> wrote:
> >   
> >> On Tue, 28 Mar 2023, Herve Codina wrote:
> >>  
> >>> The Lantiq PEF2256 is a framer and line interface component designed to
> >>> fulfill all required interfacing between an analog E1/T1/J1 line and the
> >>> digital PCM system highway/H.100 bus.    
> >>
> >> My goodness!
> >>
> >> It's been a long time since I've seen anything quite like this.  
> > 
> > Yes, old things but working on recent kernel.
> >   
> >>
> >> My suggestion to you:
> >>
> >> * Split this up into components that fit functional subsystems  
> > 
> > It is done. The audio part is present in ASoC subsystem (path 5 in this
> > series). pinctrl function is implemented in this driver and, as I don't
> > want to share registers, I would prefer to keep this function inside this
> > driver.  
> 
> The amount of defines and huge functions like pef2256_setup_e1()
> contradict it.
> 
> Even the pef2256_setup_e1() really does not follow Linux coding style -
> you know the size requirement, right?

I know that pef2256_setup_e1() is quite big and I will look at a way
to split it in a consistent way.

> 
> pef2256_get_groups_count, struct pinmux_ops and others - this is
> pinctrl, not MFD! They cannot be in MFD driver.

Maybe the issue is that MFD was not a good choice.
The "function" provided are not independent of each other.
The "main" driver (pef2256.c) needs to do the setup and handle the interrupt.
The "function" provided are some glues in order to be used in some sub-systems
such as audio. Indeed, ASoC needs a codec DAI to be connected to a CPU DAI.
These "functions" need to be started (ie probe()) after the pef2256 setup was
done. So a start (probe()) order relationship is needed.

If a MFD driver needs independent children to handle independent functions,
the pef2256 does not fit well as a MFD driver.

I switched from misc to MFD just to handle child DT nodes instead of having
phandles. Using child DT nodes instead of phandles is really a good thing and
need to be kept.
The switch to MFD was probably not the best thing to do.

What do you think if I switched back the pef2256 "main" driver (pef2256.c) to
misc ?


> 
> > 
> > Also, I sent a RFC related to HDLC and PHY. In this RFC, the pef2256 is
> > considered as a PHY and handled in the PHY subsystem.
> >   https://lore.kernel.org/linux-kernel/20230323103154.264546-1-herve.codina@xxxxxxxxxxx/
> >   
> >> * Run checkpatch.pl  
> > 
> > I did.  
> 
> There are tons of weird indentation,e.g.:
> +#define     PEF2256_2X_PC_XPC_XLT	(0x8 << 0)
>         ^^^^ there is only one space after #define

I ran checkpatch.pl, not checkpatch.pl --strict.

The spaces related the #define can be seen on many other drivers.

#define FOO_REG_BAR	0x10
#define   FOO_REG_BAR_BIT0	BIT(0)
#define   FOO_REG_BAR_BIT4	BIT(4)

The first line is the register offset and the other lines (indented) are
the bits description related to this register.

> 
> ... and other style issues:
> 
> CHECK: Please don't use multiple blank lines
> CHECK: spaces preferred around that '+' (ctx:VxV)
> CHECK: Alignment should match open parenthesis
> CHECK: Macro argument reuse '_groups' - possible side-effects?
> CHECK: usleep_range is preferred over udelay; see
> Documentation/timers/timers-howto.rst
> CHECK: spaces preferred around that '/' (ctx:VxV)

I will have a look and fix them in the next iteration.

> 
> 
> >   
> >> * Remove all of the debug prints  
> > 
> > I can do that in the next iteration if really needed.
> >   
> >> * Move all of the defines out to a header file  
> > 
> > These defines are related to registers. As I don't want to share these
> > registers, is it really necessary to use a header file for them ?
> >   
> >> * Be more verbose in your documentation / comments  
> > 
> > I can improve the API documentation present in include/mfd/pef2256.h.
> > Do you thing that is necessary ? Only a few devices will use this API.
> >   
> Krzysztof
> 

Best regards,
Hervé