On 19.03.2012 20:55, Mark Brown wrote:
> On Thu, Mar 15, 2012 at 05:54:33PM +0100, Karol Lewandowski wrote:
>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
