RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li <min.li.xe@renesas.com>]

> 
> Ah, so if this is for a PTP related driver, it should probably be integrated into
> the PTP subsystem rather than being a separate class.
> 

I was trying to add these functions to PHC subsystem but was not accepted because the functions
are specific to Renesas device and there is no place for those functions in PHC driver.

> > > A pure list of register values seems neither particular portable nor
> intuitive.
> > > How is a user expected to interpret these, and are you sure that any
> > > driver for this class would have the same interpretation at the same
> register index?
> > >
> >
> > Yes we need a way to dump register values when remote debugging with
> customers.
> > And all the Renesas SMU has similar register layout
> 
> A sysfs interface is a poor choice for this though -- how can you guarantee
> that even future Renesas devices follow the exact same register layout? By
> encoding the current hardware generation into the user interface, you would
> end up having to emulate this on other chips you want to support later.
> 
> If it's only for debugging, best leave it out of the public interface, and only
> have it in your own copy of the driver until the bugs are gone, or add a
> debugfs interface.
> 

I will drop the sysfs change in the new patch

> 
> Unit tests are good, but it's better to have them in the kernel.
> Can you add the unit test into the patch then?
> We now have the kunit framework for running unit tests.
> 

Our unit test is based on ceedling. But I will definitely look into the kunit and try to
Transfer it to kunit for the next release.

> > > This tells me that you got the abstraction the wrong way: the common
> > > files should not need to know anything about the specific
> implementations.
> > >
> > > Instead, these should be in separate modules that call exported
> > > functions from the common code.
> > >
> > >
> >
> > I got what you mean. But so far it only supports small set of
> > functions, which is why I don't feet it is worth the effort to over abstract
> things.
> 
> Then maybe pick one of the two hardware variants and drop the abstraction
> you have. You can then add more features before you add a proper
> abstraction layer and then the second driver.
> 

If I come up with a new file and move all the abstraction code there, does that work?