On 19.03.2012 20:55, Mark Brown wrote:

On Thu, Mar 15, 2012 at 05:54:33PM +0100, Karol Lewandowski wrote:

quoted

If you consider code to be inherently less readable because of this
approach I'll rework it.  If it's not a such big deal for you I would
prefer to keep it as is.

The thing that was causing me to think the code was funny was mainly the
fact that we're now combining both quirk based selection and chip type
based selection into the same bitmask.  If the chip types were quirks
it'd probably not have looked odd, and that might just be a case of
renaming them - I can't remember what they do.

What do you think about following changes, then?

diff --git a/drivers/i2c/busses/i2c-s3c2410.c

b/drivers/i2c/busses/i2c-s3c2410.c
index fa5f8c4..18c9760 100644

--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c

@@ -48,12 +48,9 @@
 static const struct of_device_id s3c24xx_i2c_match[];
 #endif

-/* reserve lower 8 bits for device type, use remaining space for hw

quirks */
-#define TYPE_BITS		0x000000ff
-#define TYPE_S3C2410		0x00000001
-#define TYPE_S3C2440		0x00000002
-#define FLAG_HDMIPHY		0x00000100
-#define FLAG_NO_GPIO		0x00000200
+#define QUIRK_S3C2440		(1 << 0)
+#define QUIRK_HDMIPHY		(1 << 1)
+#define QUIRK_NO_GPIO		(1 << 2)

 /* i2c controller state */
 enum s3c24xx_i2c_state {

@@ -67,7 +64,7 @@ enum s3c24xx_i2c_state {
 struct s3c24xx_i2c {
 	spinlock_t		lock;
 	wait_queue_head_t	wait;
-	unsigned int            type;
+	unsigned int            quirks;
 	unsigned int		suspended:1;

 	struct i2c_msg		*msg;

@@ -94,22 +91,12 @@ struct s3c24xx_i2c {
 #endif
 };

-/* s3c24xx_i2c_is_type()
- *
- * return true if this controller is of specified type
-*/
-
-static inline int s3c24xx_i2c_is_type(struct s3c24xx_i2c *i2c, unsigned

int type)
-{
-	return (i2c->type & TYPE_BITS) == type;
-}
-
-/* s3c24xx_get_device_type
+/* s3c24xx_get_device_quirks
  *
  * Get controller type either from device tree or platform device variant.
 */

-static inline unsigned int s3c24xx_get_device_type(struct
platform_device *pdev)
+static inline unsigned int s3c24xx_get_device_quirks(struct
platform_device *pdev)
 {
 #ifdef CONFIG_OF
         if (pdev->dev.of_node) {

@@ -487,7 +474,7 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c

*i2c)
 	 * the hangup is expected to happen, so waiting 400 ms
 	 * causes only unnecessary system hangup
 	 */
-	if (i2c->type & FLAG_HDMIPHY)
+	if (i2c->quirks & QUIRK_HDMIPHY)
 		timeout = 10;

 	while (timeout-- > 0) {

@@ -500,7 +487,7 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c

*i2c)
 	}

 	/* hang-up of bus dedicated for HDMIPHY occurred, resetting */
-	if (i2c->type & FLAG_HDMIPHY) {
+	if (i2c->quirks & QUIRK_HDMIPHY) {
 		writel(0, i2c->regs + S3C2410_IICCON);
 		writel(0, i2c->regs + S3C2410_IICSTAT);
 		writel(0, i2c->regs + S3C2410_IICDS);

@@ -704,7 +691,7 @@ static int s3c24xx_i2c_clockrate(struct s3c24xx_i2c

*i2c, unsigned int *got)

 	writel(iiccon, i2c->regs + S3C2410_IICCON);

-	if (s3c24xx_i2c_is_type(i2c, TYPE_S3C2440)) {
+	if (i2c->quirks & QUIRK_S3C2440) {
 		unsigned long sda_delay;

 		if (pdata->sda_delay) {

@@ -789,7 +776,7 @@ static int s3c24xx_i2c_parse_dt_gpio(struct

s3c24xx_i2c *i2c)
 {
 	int idx, gpio, ret;

-	if (i2c->type & FLAG_NO_GPIO)
+	if (i2c->quirks & QUIRK_NO_GPIO)
 		return 0;

 	for (idx = 0; idx < 2; idx++) {

@@ -817,7 +804,7 @@ static void s3c24xx_i2c_dt_gpio_free(struct

s3c24xx_i2c *i2c)
 {
 	unsigned int idx;

-	if (i2c->type & FLAG_NO_GPIO)
+	if (i2c->quirks & QUIRK_NO_GPIO)
 		return;

 	for (idx = 0; idx < 2; idx++)

@@ -898,10 +885,10 @@ s3c24xx_i2c_parse_dt(struct device_node *np,

struct s3c24xx_i2c *i2c)
 	pdata->bus_num = -1; /* i2c bus number is dynamically assigned */

 	if (of_get_property(np, "samsung,i2c-quirk-hdmiphy", NULL))
-		i2c->type |= FLAG_HDMIPHY;
+		i2c->quirks |= QUIRK_HDMIPHY;

 	if (of_get_property(np, "samsung,i2c-no-gpio", NULL))
-		i2c->type |= FLAG_NO_GPIO;
+		i2c->quirks |= QUIRK_NO_GPIO;

 	of_property_read_u32(np, "samsung,i2c-sda-delay", &pdata->sda_delay);
 	of_property_read_u32(np, "samsung,i2c-slave-addr", &pdata->slave_addr);

@@ -948,7 +935,7 @@ static int s3c24xx_i2c_probe(struct platform_device

*pdev)
 		goto err_noclk;
 	}

-	i2c->type = s3c24xx_get_device_type(pdev);
+	i2c->quirks = s3c24xx_get_device_quirks(pdev);
 	if (pdata)
 		memcpy(i2c->pdata, pdata, sizeof(*pdata));
 	else

@@ -1156,21 +1143,21 @@ static const struct dev_pm_ops

s3c24xx_i2c_dev_pm_ops = {
 static struct platform_device_id s3c24xx_driver_ids[] = {
 	{
 		.name		= "s3c2410-i2c",
-		.driver_data	= TYPE_S3C2410,
+		.driver_data	= 0,
 	}, {
 		.name		= "s3c2440-i2c",
-		.driver_data	= TYPE_S3C2440,
+		.driver_data	= QUIRK_S3C2440,
 	}, {
 		.name		= "s3c2440-hdmiphy-i2c",
-		.driver_data	= TYPE_S3C2440 | FLAG_HDMIPHY | FLAG_NO_GPIO,
+		.driver_data	= QUIRK_S3C2440 | QUIRK_HDMIPHY | QUIRK_NO_GPIO,
 	}, { },
 };
 MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);

 #ifdef CONFIG_OF
 static const struct of_device_id s3c24xx_i2c_match[] = {
-	{ .compatible = "samsung,s3c2410-i2c", .data = (void *)TYPE_S3C2410 },
-	{ .compatible = "samsung,s3c2440-i2c", .data = (void *)TYPE_S3C2440 },
+	{ .compatible = "samsung,s3c2410-i2c", .data = (void *)0 },
+	{ .compatible = "samsung,s3c2440-i2c", .data = (void *)QUIRK_S3C2440 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);


Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform