Messages in this thread | | | Date | Wed, 18 Apr 2012 15:39:45 +0200 | From | Wolfram Sang <> | Subject | 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 <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] | |