lkml.org 
[lkml]   [2012]   [Apr]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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 <k.lewandowsk@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >
> > 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/ |
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-04-18 22:37    [W:0.040 / U:0.528 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site