Re: [PATCH] i3c: master: dw: split dw-i3c-master.c into master and bus specific parts

[Boris Brezillon]

On Tue, 4 Dec 2018 00:34:20 +0000
vitor <vitor.soares@xxxxxxxxxxxx> wrote:
  
> >>> What I call a slave controller would be something that lets you reply to
> >>> SDR/DDR transactions or fill a generic regmap that would be exposed to
> >>> other masters on the bus. This way we could implement generic slave
> >>> drivers in Linux (the same way we have gadget drivers). Anything else
> >>> is likely to be to specific to be exposed as a generic framework.  
> >> I would bet to do something like in i2c, we don't need the same level of
> >> complexity found in USB.  
> > Can you detail a bit more what you have in mind? I don't think we can
> > do like I2C, simply because we need to expose a valid DCR +
> > manuf-ID/PID so that other masters can bind the device to the
> > appropriate driver on their side. Plus, if we're about to expose
> > generic profiles, we likely don't want each I3C slave controller driver
> > to do that on its own.  
> 
> 
> I think this should be discuss in another thread. Do you agree?

If you want. Actually that's the most interesting part for me:
discussing how we want to support I3C slave controllers or mixed
master/slave controllers. All the driver split we're talking about
here is just bikeshedding.

> 
> 
> >>>> Taking the USB as exemple do you prefer a dwc folder on i3c root?  
> >>> Hm, not sure I like this idea either. So I see 2 options:
> >>>
> >>> 1/ put all controller drivers (both master and slave ones) in a common
> >>>      directory (drivers/i3c/controllers) as you suggest, and prefix them
> >>>      correctly (i3c-master-<ip>.c, i3c-slave-<ip>.c and i3c-dual-<ip>.c)  
> >> I agree with the controller folder but not with prefix. Please check
> >> what is already in the kernel.  
> > If we mix everything in the same subdir, I'd like to have an easy way
> > to quickly identify those that are slave controllers and those that are
> > master controllers. For the dual-role thing, maybe we can consider them
> > as master (ones with advances slave features).
> >
> > Would you be okay with drivers/i3c/controllers/{designware,dw}/..., so
> > that you can have all designware drivers (for both slave and master
> > blocks) in the same dir?  
> 
> 
> Yes, that was what I trying to tell you. For me this might be the best 
> option.

Ok.

> 
> I would like to avoid having dual role i3c driver in a master folder.

I don't see why. If the driver is simple enough to fit in one file,
there's no reason to create a new subdir. You think your DW IP is so
complex and configurable that it requires several source files, fine,
but please don't force others to do the same.

> 
> 
> > For those that are placed directly under drivers/i3c/controllers/...
> > (because they only have one .c file), I'd like to keep a standard
> > prefix.  
> 
> 
> I don't disagree, and for those that have more than one file they should 
> be in a folder, right?

Yes.

> 
> What prefix do you have in mind for those files inside a folder?

You mean, inside a sub-folder (drivers/i3c/controllers/{vendor}/)? It
depends what you do with those source files. If they are to be exposed
directly as modules, then they should be prefixed
(i3c-<role>-<vendor>.c). On the other hand, if you create a single
module out of several source files, source files don't need to be
prefixed, as long as the resulting module as a proper prefix.

> 
> 
> >> To be clear, the subsystem is nice and I working with daily. As I said
> >> this is something that I dealing now and I'm telling what I think that
> >> is not correct.
> >>  
> > Come on! All I've seen so far are complaints on tiny details, it
> > definitely doesn't prevent you from adding new features.
> >
> > Regards,
> >
> > Boris  
> 
> 
> No, it's not. But as you can see to slipt the driver in parts this 
> subject has some relevance.

I'm not saying the discussion is useless, just that it's happening way
too early compared to the other things we should work on. If you were
adding support for slaves, and were doing this split as part of this
patch series explaining that part of the code between slave and master
can be shared, then we wouldn't have this debate. But right now, you're
telling me that we need to split the DW driver to prepare for features
that have not even been discussed/proposed. That's what I'm complaining
about.