Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

Hi Greg,

On Tue, 19 Dec 2017 10:36:43 +0100
Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:

> On Tue, Dec 19, 2017 at 10:28:58AM +0100, Boris Brezillon wrote:
> > On Tue, 19 Dec 2017 10:21:19 +0100
> > Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >   
> > > On Tue, Dec 19, 2017 at 10:13:36AM +0100, Boris Brezillon wrote:  
> > > > On Tue, 19 Dec 2017 10:09:00 +0100
> > > > Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote:
> > > >     
> > > > > On Tue, 19 Dec 2017 09:52:50 +0100
> > > > > Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > >     
> > > > > > On Thu, Dec 14, 2017 at 04:16:05PM +0100, Boris Brezillon wrote:      
> > > > > > > +/**
> > > > > > > + * i3c_device_match_id() - Find the I3C device ID entry matching an I3C dev
> > > > > > > + * @i3cdev: the I3C device we're searching a match for
> > > > > > > + * @id_table: the I3C device ID table
> > > > > > > + *
> > > > > > > + * Return: a pointer to the first entry matching @i3cdev, or NULL if there's
> > > > > > > + *	   no match.
> > > > > > > + */
> > > > > > > +const struct i3c_device_id *
> > > > > > > +i3c_device_match_id(struct i3c_device *i3cdev,
> > > > > > > +		    const struct i3c_device_id *id_table)
> > > > > > > +{
> > > > > > > +	const struct i3c_device_id *id;
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * The lower 32bits of the provisional ID is just filled with a random
> > > > > > > +	 * value, try to match using DCR info.
> > > > > > > +	 */
> > > > > > > +	if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) {
> > > > > > > +		u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> > > > > > > +		u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> > > > > > > +		u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> > > > > > > +
> > > > > > > +		/* First try to match by manufacturer/part ID. */
> > > > > > > +		for (id = id_table; id->match_flags != 0; id++) {
> > > > > > > +			if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) !=
> > > > > > > +			    I3C_MATCH_MANUF_AND_PART)
> > > > > > > +				continue;
> > > > > > > +
> > > > > > > +			if (manuf != id->manuf_id || part != id->part_id)
> > > > > > > +				continue;
> > > > > > > +
> > > > > > > +			if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
> > > > > > > +			    ext_info != id->extra_info)
> > > > > > > +				continue;
> > > > > > > +
> > > > > > > +			return id;
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	/* Fallback to DCR match. */
> > > > > > > +	for (id = id_table; id->match_flags != 0; id++) {
> > > > > > > +		if ((id->match_flags & I3C_MATCH_DCR) &&
> > > > > > > +		    id->dcr == i3cdev->info.dcr)
> > > > > > > +			return id;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return NULL;
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(i3c_device_match_id);        
> > > > > > 
> > > > > > I just picked one random export here, but it feels like you are
> > > > > > exporting a bunch of symbols you don't need to.  Why would something
> > > > > > outside of the i3c "core" need to call this function?      
> > > > > 
> > > > > Because I'm not passing the i3c_device_id to the ->probe() method, and
> > > > > if the driver is supporting different variants of the device, it may
> > > > > want to know which one is being probed.
> > > > > 
> > > > > I considered retrieving this information in the core just before probing
> > > > > the driver and passing it to the ->probe() function, but it means
> > > > > having an extra i3c_device_match_id() call for everyone even those who
> > > > > don't care about the device_id information, so I thought exporting this
> > > > > function was a good alternative (device drivers can use it when they
> > > > > actually need to retrieve the device_id).
> > > > > 
> > > > > Anyway, that's something I can change if you think passing the
> > > > > i3c_device_id to the ->probe() method is preferable.
> > > > >     
> > > > > > Have you looked
> > > > > > to see if you really have callers for everything you are exporting?      
> > > > > 
> > > > > Yes, I tried to only export functions that I think will be needed by
> > > > > I3C device drivers and I3C master drivers. Note that I didn't post the
> > > > > dummy device driver I developed to test the framework (partly because
> > > > > this is     
> > > > 
> > > > Sorry, I hit the send button before finishing my sentence :-).
> > > > 
> > > > "
> > > > Note that I didn't post the dummy device driver [1] I developed to test
> > > > the framework (partly because the quality of the code does not meet
> > > > mainline standards and I was ashamed of posting it publicly :-)), but
> > > > this driver is using some of the exported functions.
> > > > "    
> > > 
> > > We don't export functions that has no in-kernel users :)  
> > 
> > But then, I can't export device driver related functions, because
> > there's no official device driver yet :-). So what should I do?  
> 
> Export them when you have a driver.  Or better yet, submit a driver as
> part of the patch series.  Why would we want infrastructure that no one
> uses?

I understand your point of view, it may sound odd to add a framework
for a bus that we have no slave devices for. But, as far as I can tell,
many vendors (both IP vendors and sensor manufacturers) are working
actively on creating master and slave I3C devices. Actually, I've even
been contacted privately by an IP vendor after posting this patchset.
So, I think one argument for pushing the current framework with no
users yet is that it may help others develop drivers for their device
early on, or even help them test those devices more easily than if they
had to develop baremetal code.

The kernel community has been asking hardware vendors for a long time to
upstream their code as early as possible. And this is exactly what is
happening with I3C: even before actual devices are shipping, we have
the opportunity to start merging support for I3C in the mainline
kernel. It would be good to merge it before vendors spend time working
on competing implementations, which will take even more time to
reconcile when they will be submitted for upstream inclusion.

Also, we're not talking about some random bus/protocol, I3C spec is
developed and pushed by MIPI, and for once, they decided to open the
spec, so anyone can actually make sure the framework is matching the
protocol description if they want to.

Now, if you still think an I3C device driver is needed to consider
merging these patches, I can provide one. As I said earlier, I
developed a driver for a dummy device to test the various features I
add support for in this series, it's just that this device will never
ever be available in real life and it's not even fitting in any of the
subsystem we have in the kernel, hence my initial decision to not
upstream it.

Regards,

Boris

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com