Re: [PATCH v2 4/5] spi: Add OF binding support for SPI busses

On Fri, Jul 04, 2008 at 11:54:57AM +0800, Chen Gong wrote:

Grant Likely wrote:

quoted

+		/* Mode (clock phase/polarity/etc.) */
+		if (of_find_property(nc, "spi,cpha", NULL))
+			spi->mode |= SPI_CPHA;
+		if (of_find_property(nc, "spi,cpol", NULL))
+			spi->mode |= SPI_CPOL;

so becuase in function spi_alloc_deive, spi is allocated by kzalloc,
how about writing as follows:
		/* Mode (clock phase/polarity/etc.) */
		prop = of_get_property(nc, "spi,cpha", NULL))
                        if (prop)
			spi->mode |= *prop;
		prop = of_get_property(nc, "spi,cpol", NULL))
                        if (prop)
			spi->mode |= *prop;

spi,cpha and spi,cpol are defined as empty properties.  The presence of
the property in the node means I need to set the flag in the spi_device.

quoted

+		/* Select device driver */
+		sprop = of_get_property(nc, "linux,modalias", &len);
+		if (sprop && len > 0)
+			strncpy(spi->modalias, sprop, KOBJ_NAME_LEN);
+		else
+			strncpy(spi->modalias, "spidev", KOBJ_NAME_LEN);
+

 how about writing as follows:

		if (sprop && len > 0)
			strncpy(spi->modalias, sprop, KOBJ_NAME_LEN -
1);
		else
			strncpy(spi->modalias, "spidev", KOBJ_NAME_LEN -
1);

Actually, neither are very good.  What it should really be is:

	if (sprop && len > 0)
		strlcpy(spi->modalias, sprop, sizeof (spi->modalias));
	else
		strlcpy(spi->modalias, "spidev", sizeof (spi->modalias));

This ensures that the string is always null terminated and always the
right size.

... But the whole argument is a bit moot.  The fact that I even defined
a "linux,modalias" property is a great big hairy hack that I would
never want my mother to see.  Instead, I need a method to bind to an SPI
driver based on the compatible list.  Jon Smirl has done some work
in this regard for i2c, but I haven't looked at it very deeply, and so
do not at all understand it (yet).

Thanks for the comments.
g.