Re: [PATCH 11/17] add support for device trees

[Christoffer Dall <christoffer.dall@linaro.org>]

On Mon, Feb 03, 2014 at 04:31:24PM +0100, Andrew Jones wrote:
> On Sat, Feb 01, 2014 at 06:27:15PM -0800, Christoffer Dall wrote:
> > On Tue, Jan 21, 2014 at 05:21:57PM +0100, Andrew Jones wrote:

[...]

> > > +
> > > +int dt_get_reg(int nodeoffset, int regidx, u32 *address_cells,
> > > +	       u32 *size_cells, struct dt_reg *reg)
> > > +{
> > > +	const struct fdt_property *prop;
> > > +	u32 *data, regsz, i;
> > > +	int err;
> > > +
> > > +	if (!address_cells || !size_cells || !reg)
> > > +		return -EINVAL;
> > 
> > defensive programming?
> > 
> > > +
> > > +	memset(reg, 0, sizeof(struct dt_reg));
> > 
> > sizeof(*reg)
> > 
> > > +
> > > +	/*
> > > +	 * We assume #size-cells == 0 means translation is impossible,
> > 
> > hmm, this won't work for the cpus' reg properties for example.  I would
> > just assume (and document) that this function always takes valid
> > address_cells and size_cells (as u32 instead of u32 *).  I get that
> > you're trying to do some caching (from reading later patches), but that
> > should still be possible to do externally.
> 
> cpus will need their own 'dt_get_cpuids', built on a 'dt_for_each_cpu',
> or some such, and wouldn't use this function. Other than cpus, I think
> this is a safe convention. Although, that said, I'll look into cleaning
> up the interface a bit, while maintaining ease of use and the ability to
> cache.
> 

well, your function is called dt_get_reg and is generally exported from
lib/devicetree.c, so there's nothing here suggesting this can only be
used for a reg property for a certain type of devices.  cpus is an
example of a node that has #size-cells == 0 and has a perfectly valid
reg property.

The fact that you don't plan on using this function to iterate CPUs
doesn't mean that somebody else will not, or we will add another binding
that will use same convention, and now this funciton has to be
refactored.

If this function was virtio_get_reg, it would be a different story...

> > 
> > If you really really want to, then you should use address_cells, but I'm
> > not sure if it's actually valid for any bindings to have address_cells
> > == 0 and size_cells >0.  If you have conventions like this, it should be
> > clearly documented on the function itself!
> > 

[...]

Thanks,
-Christoffer