Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling
Hi, > >> Reorganize driver a bit to better handle device tree-based systems: > >> > >> - move machine type to driver's private structure instead of > >>quering platform device variants in runtime > >> > >> - replace s3c24xx_i2c_type enum with unsigned int that holds > >>bitmask with revision-specific quirks > >> > >> Signed-off-by: Karol Lewandowski > >> Signed-off-by: Kyungmin Park > > > > Okay, so this driver needs to the 'data' field from either > > platform_device_id or of_device_id and implements a function for that, > > namely s3c24xx_get_device_quirks(). Grant, Rob: I'd think there might be > > more drivers in need of that, so maybe it makes sense to have a generic > > of-helper function? Please wait one or two days before resending. Maybe Grant or Rob find some time to answer this question. (Yeah, we can fix it later if such a generic function is introduced somewhen). > >> -/* i2c controller state */ > >> +#ifdef CONFIG_OF > >> +static const struct of_device_id s3c24xx_i2c_match[]; > >> +#endif > > > > Uh, forward declaration with #ifdef. I'd think we should get this simply > > to the front. > > > Ok, as I think it's better to have dt and non-dt definitions together > I'll move both of_device_id and platform_device_id to the top. Agreed. > > >> +/* Treat S3C2410 as baseline hardware, anything else is supported via > >> quirks */ > >> +#define QUIRK_S3C2440 (1 << 0) > > > > Minor: Is it really a quirk being s3c2440? Maybe FLAG_*, dunno. > > > In first version[1] of this patch I've used TYPEs for device types > and FLAGS for quirks. However, I've squashed these into "quirks" after > discussion with Mark[2]. > > [1] http://permalink.gmane.org/gmane.linux.kernel/1266759 > [2] http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/10255 It's minor, keep the QUIRKs. > [ I'll also drop above CONFIG_OF ifdef, as of_match_node() seem > to be always defined since v3.2-rc1. ] Great. Thanks, Wolfram -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling
On 17.04.2012 19:31, Wolfram Sang wrote:
> Hi,
Hi Wolfram!
>
> On Wed, Mar 21, 2012 at 08:11:52PM +0100, Karol Lewandowski wrote:
>> Reorganize driver a bit to better handle device tree-based systems:
>>
>> - move machine type to driver's private structure instead of
>>quering platform device variants in runtime
>>
>> - replace s3c24xx_i2c_type enum with unsigned int that holds
>>bitmask with revision-specific quirks
>>
>> Signed-off-by: Karol Lewandowski
>> Signed-off-by: Kyungmin Park
>
> Okay, so this driver needs to the 'data' field from either
> platform_device_id or of_device_id and implements a function for that,
> namely s3c24xx_get_device_quirks(). Grant, Rob: I'd think there might be
> more drivers in need of that, so maybe it makes sense to have a generic
> of-helper function?
>
>> ---
>> drivers/i2c/busses/i2c-s3c2410.c | 47
>> ++---
>> 1 files changed, 23 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-s3c2410.c
>> b/drivers/i2c/busses/i2c-s3c2410.c
>> index 85e3664..f7b6a14 100644
>> --- a/drivers/i2c/busses/i2c-s3c2410.c
>> +++ b/drivers/i2c/busses/i2c-s3c2410.c
>> @@ -44,8 +44,14 @@
>> #include
>> #include
>>
>> -/* i2c controller state */
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id s3c24xx_i2c_match[];
>> +#endif
>
> Uh, forward declaration with #ifdef. I'd think we should get this simply
> to the front.
Ok, as I think it's better to have dt and non-dt definitions together
I'll move both of_device_id and platform_device_id to the top.
>> +/* Treat S3C2410 as baseline hardware, anything else is supported via
>> quirks */
>> +#define QUIRK_S3C2440 (1 << 0)
>
> Minor: Is it really a quirk being s3c2440? Maybe FLAG_*, dunno.
In first version[1] of this patch I've used TYPEs for device types
and FLAGS for quirks. However, I've squashed these into "quirks" after
discussion with Mark[2].
[1] http://permalink.gmane.org/gmane.linux.kernel/1266759
[2] http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/10255
>> -static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
>> +static inline unsigned int s3c24xx_get_device_quirks(struct platform_device
>> *pdev)
>> {
>> -struct platform_device *pdev = to_platform_device(i2c->dev);
>> -enum s3c24xx_i2c_type type;
>> -
>> #ifdef CONFIG_OF
>> -if (i2c->dev->of_node)
>> -return of_device_is_compatible(i2c->dev->of_node,
>> -"samsung,s3c2440-i2c");
>> +if (pdev->dev.of_node) {
>> +const struct of_device_id *match;
>> +match = of_match_node(&s3c24xx_i2c_match[0], pdev->dev.of_node);
>
> Minor: I think it is more readable to drop the [0]
I prefer explicit version, but I'll drop [] as both you and Thomas
found implicit version more readable.
[ I'll also drop above CONFIG_OF ifdef, as of_match_node() seem
to be always defined since v3.2-rc1. ]
Thanks for review! I'll resubmit updated version shortly.
Regards,
--
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling
Hi,
On Wed, Mar 21, 2012 at 08:11:52PM +0100, Karol Lewandowski wrote:
> Reorganize driver a bit to better handle device tree-based systems:
>
> - move machine type to driver's private structure instead of
>quering platform device variants in runtime
>
> - replace s3c24xx_i2c_type enum with unsigned int that holds
>bitmask with revision-specific quirks
>
> Signed-off-by: Karol Lewandowski
> Signed-off-by: Kyungmin Park
Okay, so this driver needs to the 'data' field from either
platform_device_id or of_device_id and implements a function for that,
namely s3c24xx_get_device_quirks(). Grant, Rob: I'd think there might be
more drivers in need of that, so maybe it makes sense to have a generic
of-helper function?
> ---
> drivers/i2c/busses/i2c-s3c2410.c | 47 ++---
> 1 files changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c
> b/drivers/i2c/busses/i2c-s3c2410.c
> index 85e3664..f7b6a14 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -44,8 +44,14 @@
> #include
> #include
>
> -/* i2c controller state */
> +#ifdef CONFIG_OF
> +static const struct of_device_id s3c24xx_i2c_match[];
> +#endif
Uh, forward declaration with #ifdef. I'd think we should get this simply
to the front.
> dd
> +/* Treat S3C2410 as baseline hardware, anything else is supported via quirks
> */
> +#define QUIRK_S3C2440(1 << 0)
Minor: Is it really a quirk being s3c2440? Maybe FLAG_*, dunno.
> +
> +/* i2c controller state */
> enum s3c24xx_i2c_state {
> STATE_IDLE,
> STATE_START,
> @@ -54,14 +60,10 @@ enum s3c24xx_i2c_state {
> STATE_STOP
> };
>
> -enum s3c24xx_i2c_type {
> - TYPE_S3C2410,
> - TYPE_S3C2440,
> -};
> -
> struct s3c24xx_i2c {
> spinlock_t lock;
> wait_queue_head_t wait;
> + unsigned intquirks;
> unsigned intsuspended:1;
>
> struct i2c_msg *msg;
> @@ -88,26 +90,22 @@ struct s3c24xx_i2c {
> #endif
> };
>
> -/* default platform data removed, dev should always carry data. */
> -
> -/* s3c24xx_i2c_is2440()
> +/* s3c24xx_get_device_quirks
> *
> - * return true is this is an s3c2440
> + * Get controller type either from device tree or platform device variant.
> */
>
> -static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
> +static inline unsigned int s3c24xx_get_device_quirks(struct platform_device
> *pdev)
> {
> - struct platform_device *pdev = to_platform_device(i2c->dev);
> - enum s3c24xx_i2c_type type;
> -
> #ifdef CONFIG_OF
> - if (i2c->dev->of_node)
> - return of_device_is_compatible(i2c->dev->of_node,
> - "samsung,s3c2440-i2c");
> +if (pdev->dev.of_node) {
> + const struct of_device_id *match;
> + match = of_match_node(&s3c24xx_i2c_match[0], pdev->dev.of_node);
Minor: I think it is more readable to drop the [0]
> + return (unsigned int)match->data;
> +}
> #endif
>
> - type = platform_get_device_id(pdev)->driver_data;
> - return type == TYPE_S3C2440;
> + return platform_get_device_id(pdev)->driver_data;
> }
>
> /* s3c24xx_i2c_master_complete
> @@ -676,7 +674,7 @@ static int s3c24xx_i2c_clockrate(struct s3c24xx_i2c *i2c,
> unsigned int *got)
>
> writel(iiccon, i2c->regs + S3C2410_IICCON);
>
> - if (s3c24xx_i2c_is2440(i2c)) {
> + if (i2c->quirks & QUIRK_S3C2440) {
> unsigned long sda_delay;
>
> if (pdata->sda_delay) {
> @@ -906,6 +904,7 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
> goto err_noclk;
> }
>
> + i2c->quirks = s3c24xx_get_device_quirks(pdev);
> if (pdata)
> memcpy(i2c->pdata, pdata, sizeof(*pdata));
> else
> @@ -1113,18 +1112,18 @@ 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,
> }, { },
> };
> MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);
>
> #ifdef CONFIG_OF
> static const struct of_device_id s3c24xx_i2c_match[] = {
> - { .compatible = "samsung,s3c2410-i2c" },
> - { .compatible = "samsung,s3c2440-i2c" },
> + { .compatible = "samsung,s3c2410-i2c", .data = (void *)0 },
> + { .compatible = "samsung,s3c2440-i2c", .data = (void *)QUIRK_S3C2440 },
Hmm, the need to sepcify the quirks twice may lead to only one instance
being updated in future patches, but I don't see a way around that,
currently :(
> {},
> };
> MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
> --
> 1.7.9
>
Thanks,
Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling
On Wed, Mar 21, 2012 at 08:11:52PM +0100, Karol Lewandowski wrote: > Reorganize driver a bit to better handle device tree-based systems: > > - move machine type to driver's private structure instead of >quering platform device variants in runtime > > - replace s3c24xx_i2c_type enum with unsigned int that holds >bitmask with revision-specific quirks Reviewed-by: Mark Brown signature.asc Description: Digital signature
[PATCH 2/3] i2c-s3c2410: Rework device type handling
Reorganize driver a bit to better handle device tree-based systems:
- move machine type to driver's private structure instead of
quering platform device variants in runtime
- replace s3c24xx_i2c_type enum with unsigned int that holds
bitmask with revision-specific quirks
Signed-off-by: Karol Lewandowski
Signed-off-by: Kyungmin Park
---
drivers/i2c/busses/i2c-s3c2410.c | 47 ++---
1 files changed, 23 insertions(+), 24 deletions(-)
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 85e3664..f7b6a14 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -44,8 +44,14 @@
#include
#include
-/* i2c controller state */
+#ifdef CONFIG_OF
+static const struct of_device_id s3c24xx_i2c_match[];
+#endif
+/* Treat S3C2410 as baseline hardware, anything else is supported via quirks */
+#define QUIRK_S3C2440 (1 << 0)
+
+/* i2c controller state */
enum s3c24xx_i2c_state {
STATE_IDLE,
STATE_START,
@@ -54,14 +60,10 @@ enum s3c24xx_i2c_state {
STATE_STOP
};
-enum s3c24xx_i2c_type {
- TYPE_S3C2410,
- TYPE_S3C2440,
-};
-
struct s3c24xx_i2c {
spinlock_t lock;
wait_queue_head_t wait;
+ unsigned intquirks;
unsigned intsuspended:1;
struct i2c_msg *msg;
@@ -88,26 +90,22 @@ struct s3c24xx_i2c {
#endif
};
-/* default platform data removed, dev should always carry data. */
-
-/* s3c24xx_i2c_is2440()
+/* s3c24xx_get_device_quirks
*
- * return true is this is an s3c2440
+ * Get controller type either from device tree or platform device variant.
*/
-static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
+static inline unsigned int s3c24xx_get_device_quirks(struct platform_device
*pdev)
{
- struct platform_device *pdev = to_platform_device(i2c->dev);
- enum s3c24xx_i2c_type type;
-
#ifdef CONFIG_OF
- if (i2c->dev->of_node)
- return of_device_is_compatible(i2c->dev->of_node,
- "samsung,s3c2440-i2c");
+if (pdev->dev.of_node) {
+ const struct of_device_id *match;
+ match = of_match_node(&s3c24xx_i2c_match[0], pdev->dev.of_node);
+ return (unsigned int)match->data;
+}
#endif
- type = platform_get_device_id(pdev)->driver_data;
- return type == TYPE_S3C2440;
+ return platform_get_device_id(pdev)->driver_data;
}
/* s3c24xx_i2c_master_complete
@@ -676,7 +674,7 @@ static int s3c24xx_i2c_clockrate(struct s3c24xx_i2c *i2c,
unsigned int *got)
writel(iiccon, i2c->regs + S3C2410_IICCON);
- if (s3c24xx_i2c_is2440(i2c)) {
+ if (i2c->quirks & QUIRK_S3C2440) {
unsigned long sda_delay;
if (pdata->sda_delay) {
@@ -906,6 +904,7 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
goto err_noclk;
}
+ i2c->quirks = s3c24xx_get_device_quirks(pdev);
if (pdata)
memcpy(i2c->pdata, pdata, sizeof(*pdata));
else
@@ -1113,18 +1112,18 @@ 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,
}, { },
};
MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);
#ifdef CONFIG_OF
static const struct of_device_id s3c24xx_i2c_match[] = {
- { .compatible = "samsung,s3c2410-i2c" },
- { .compatible = "samsung,s3c2440-i2c" },
+ { .compatible = "samsung,s3c2410-i2c", .data = (void *)0 },
+ { .compatible = "samsung,s3c2440-i2c", .data = (void *)QUIRK_S3C2440 },
{},
};
MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
--
1.7.9
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling
On 21.03.2012 12:50, Mark Brown wrote: > On Wed, Mar 21, 2012 at 11:33:58AM +0100, Karol Lewandowski wrote: > >> What do you think about following changes, then? > > That looks reasonable. Thanks. I'll incorporate this change and post whole patchset again. Regards, -- Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling
On Wed, Mar 21, 2012 at 11:33:58AM +0100, Karol Lewandowski wrote: > What do you think about following changes, then? That looks reasonable. signature.asc Description: Digital signature
Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling
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 0x00ff
-#define TYPE_S3C2410 0x0001
-#define TYPE_S3C2440 0x0002
-#define FLAG_HDMIPHY 0x0100
-#define FLAG_NO_GPIO 0x0200
+#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 inttype;
+ unsigned intquirks;
unsigned intsuspended: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
Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling
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. signature.asc Description: Digital signature
Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling
On 15.03.2012 13:56, Mark Brown wrote: > On Thu, Mar 15, 2012 at 11:04:56AM +0100, Karol Lewandowski wrote: > >> Introducing separate type (TYPE_S3C2440_HDMIPHY) has been our original >> attempt to solve this issue. However, this required adding explicit >> checks to driver code all over the place (if (type == S3C2400 || >> type == S3c2440_HDMIPHY). > > Another option is to change the type to be a pointer to a struct with > quirk flags in it. Sure, that's possible. However, I don't find it any simpler than using bitmasks. Quite to contrary - I consider it redundant to bring new structure into existence when simple uint does the trick just fine (IMHO, of course). 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. Thanks! -- Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling
On Thu, Mar 15, 2012 at 11:04:56AM +0100, Karol Lewandowski wrote: > Introducing separate type (TYPE_S3C2440_HDMIPHY) has been our original > attempt to solve this issue. However, this required adding explicit > checks to driver code all over the place (if (type == S3C2400 || > type == S3c2440_HDMIPHY). Another option is to change the type to be a pointer to a struct with quirk flags in it. signature.asc Description: Digital signature
Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling
On 14.03.2012 18:29, Mark Brown wrote:
> On Tue, Mar 13, 2012 at 05:54:38PM +0100, Karol Lewandowski wrote:
>
>> - replace s3c24xx_i2c_type enum with plain unsigned int that can
>>hold not only device type but also hw revision-specific quirks
>
> Would it not be clearer to just have explicit flags for the quirks (eg,
> as a set of bitfield flags)?
That would work on runtime but we also need to initialize this
somehow. The way it was done today on non-dt platforms is via platform
device variants, i.e. (taken from 3rd patch):
@@ -1128,6 +1161,9 @@ static struct platform_device_id s3c24xx_driver_ids[] = {
}, {
.name = "s3c2440-i2c",
.driver_data= TYPE_S3C2440,
+ }, {
+ .name = "s3c2440-hdmiphy-i2c",
+ .driver_data= TYPE_S3C2440 | FLAG_HDMIPHY | FLAG_NO_GPIO,
Ability to address above scenario was sole motivation for this change.
Without it one would need either need separate type (e.g.
TYPE_S3C2440_HDMIPHY) or setting flags based just on device name.
Introducing separate type (TYPE_S3C2440_HDMIPHY) has been our original
attempt to solve this issue. However, this required adding explicit
checks to driver code all over the place (if (type == S3C2400 ||
type == S3c2440_HDMIPHY).
Thus, I felt that sqeezing quirks into type is a bit cleaner approach.
Regards,
--
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling
On Tue, Mar 13, 2012 at 05:54:38PM +0100, Karol Lewandowski wrote: > - replace s3c24xx_i2c_type enum with plain unsigned int that can >hold not only device type but also hw revision-specific quirks Would it not be clearer to just have explicit flags for the quirks (eg, as a set of bitfield flags)? -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] i2c-s3c2410: Rework device type handling
Reorganize driver a bit to better handle device tree-based systems:
- move machine type to driver's private structure instead of
quering platform device variants in runtime
- replace s3c24xx_i2c_type enum with plain unsigned int that can
hold not only device type but also hw revision-specific quirks
Signed-off-by: Karol Lewandowski
Signed-off-by: Kyungmin Park
---
drivers/i2c/busses/i2c-s3c2410.c | 53 +++---
1 files changed, 32 insertions(+), 21 deletions(-)
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 85e3664..f84d26f 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -44,8 +44,16 @@
#include
#include
-/* i2c controller state */
+#ifdef CONFIG_OF
+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 0x00ff
+#define TYPE_S3C2410 0x0001
+#define TYPE_S3C2440 0x0002
+/* i2c controller state */
enum s3c24xx_i2c_state {
STATE_IDLE,
STATE_START,
@@ -54,14 +62,10 @@ enum s3c24xx_i2c_state {
STATE_STOP
};
-enum s3c24xx_i2c_type {
- TYPE_S3C2410,
- TYPE_S3C2440,
-};
-
struct s3c24xx_i2c {
spinlock_t lock;
wait_queue_head_t wait;
+ unsigned inttype;
unsigned intsuspended:1;
struct i2c_msg *msg;
@@ -88,26 +92,32 @@ struct s3c24xx_i2c {
#endif
};
-/* default platform data removed, dev should always carry data. */
-
-/* s3c24xx_i2c_is2440()
+/* s3c24xx_i2c_is_type()
*
- * return true is this is an s3c2440
+ * return true if this controller is of specified type
*/
-static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
+static inline int s3c24xx_i2c_is_type(struct s3c24xx_i2c *i2c, unsigned int
type)
{
- struct platform_device *pdev = to_platform_device(i2c->dev);
- enum s3c24xx_i2c_type type;
+ return (i2c->type & TYPE_BITS) == type;
+}
+
+/* s3c24xx_get_device_type
+ *
+ * Get controller type either from device tree or platform device variant.
+*/
+static inline unsigned int s3c24xx_get_device_type(struct platform_device
*pdev)
+{
#ifdef CONFIG_OF
- if (i2c->dev->of_node)
- return of_device_is_compatible(i2c->dev->of_node,
- "samsung,s3c2440-i2c");
+ if (pdev->dev.of_node) {
+ const struct of_device_id *match;
+ match = of_match_node(&s3c24xx_i2c_match[0], pdev->dev.of_node);
+ return (unsigned int)match->data;
+ }
#endif
- type = platform_get_device_id(pdev)->driver_data;
- return type == TYPE_S3C2440;
+ return platform_get_device_id(pdev)->driver_data;
}
/* s3c24xx_i2c_master_complete
@@ -676,7 +686,7 @@ static int s3c24xx_i2c_clockrate(struct s3c24xx_i2c *i2c,
unsigned int *got)
writel(iiccon, i2c->regs + S3C2410_IICCON);
- if (s3c24xx_i2c_is2440(i2c)) {
+ if (s3c24xx_i2c_is_type(i2c, TYPE_S3C2440)) {
unsigned long sda_delay;
if (pdata->sda_delay) {
@@ -906,6 +916,7 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
goto err_noclk;
}
+ i2c->type = s3c24xx_get_device_type(pdev);
if (pdata)
memcpy(i2c->pdata, pdata, sizeof(*pdata));
else
@@ -1123,8 +1134,8 @@ MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);
#ifdef CONFIG_OF
static const struct of_device_id s3c24xx_i2c_match[] = {
- { .compatible = "samsung,s3c2410-i2c" },
- { .compatible = "samsung,s3c2440-i2c" },
+ { .compatible = "samsung,s3c2410-i2c", .data = (void *)TYPE_S3C2410 },
+ { .compatible = "samsung,s3c2440-i2c", .data = (void *)TYPE_S3C2440 },
{},
};
MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
--
1.7.9
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
