Re: [RFC PATCH 1/3] siox: new driver/bus framework for Eckelmann SIOX

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sat, Mar 12, 2016 at 02:43:21PM +0100, Uwe Kleine-König wrote:
> Hello Greg,
> 
> On Fri, Mar 11, 2016 at 02:20:39PM -0800, Greg Kroah-Hartman wrote:
> > On Fri, Mar 11, 2016 at 10:52:12PM +0100, Uwe Kleine-König wrote:
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/Kconfig          |   2 +
> > >  drivers/Makefile         |   1 +
> > >  drivers/siox/Kconfig     |   2 +
> > >  drivers/siox/Makefile    |   1 +
> > >  drivers/siox/siox-core.c | 572 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/siox/siox.h      |  48 ++++
> > >  include/linux/siox.h     |  53 +++++
> > >  7 files changed, 679 insertions(+)
> > >  create mode 100644 drivers/siox/Kconfig
> > >  create mode 100644 drivers/siox/Makefile
> > >  create mode 100644 drivers/siox/siox-core.c
> > >  create mode 100644 drivers/siox/siox.h
> > >  create mode 100644 include/linux/siox.h
> > 
> > What's the relationship between your "master" and "regular" devices
> > here?  I think that's getting things confused, you are unregistering
> > your 'master' yet the children it controls don't go away because you
> > never unregister them.  You need to fix this heirachy up a bit, and
> > maybe just drop the 'master' logic?
> 
> A master is the bus controller (i.e. it drives DOUT, DCLK and DLD). A
> device is a consumer and provides DIN to the master.
> 
> The topology looks as follows:
> 
>       ,------->--DCLK-->---------------+----------------------.
>       ^                                v                      v
>  ,--------.                ,----------------------.       ,------
>  |        |                |   ,--------------.   |       |
>  |        |--->--DOUT-->---|->-|shift register|->-|--->---|
>  |        |                |   `--------------'   |       |
>  | master |                |        device        |       |  device
>  |        |                |   ,--------------.   |       |
>  |        |---<--DIN---<---|-<-|shift register|-<-|---<---|
>  |        |                |   `--------------'   |       |
>  `--------'                `----------------------'       `------
>       v                                ^                      ^
>       `----------DLD-------------------+----------------------'
> 
> So there are two chains of shift registers, one for letting the master
> write to the devices, and the other to let the master read from the
> devices. DCLK is the clock to shift both chains by a single bit. An edge
> on DLD is used to make the devices load the inputs and sample the
> outputs.
> 
> I think I need the following:
> 
> 	void siox_unregister_master(struct siox_master *smaster)
> 	{
> 		for (each siox_device d of smaster)
> 			siox_device_remove(d);
> 
> 		device_unregister(&smaster->dev);
> 	}

Yes, you have to do this, as that's how USB and other busses do it when
their "controllers" go away.

> But the locking scheme that is necessary isn't obvious for me. For
> example I must prevent that after the for loop is done a new device is
> registered. Do I need to prevent this explicitly (e.g. by doing
> something like:
> 
> 	lock(smaster);
> 	smaster->is_going_away = true;
> 	for (each siox_device d of smaster)
> 		siox_device_remove(d);
> 	unlock(smaster);
> 
> 	device_unregister(&smaster->dev);
> 
> and let registering new devices fail if smaster->is_going_away is true?)
> Or is there something that the kobject stuff/driver core can help me
> with?

Look at what happens when you remove a USB host controller with the
locking there, it shouldn't be that hard.

thanks,

greg k-h