Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2237 bytes --]

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/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]