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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120418133945.GA19220@pengutronix.de>
Date:	Wed, 18 Apr 2012 15:39:45 +0200
From:	Wolfram Sang <w.sang@...gutronix.de>
To:	Karol Lewandowski <k.lewandowsk@...sung.com>
Cc:	ben-linux@...ff.org, m.szyprowski@...sung.com,
	thomas.abraham@...aro.org, linux-kernel@...r.kernel.org,
	linux-i2c@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org,
	linux-samsung-soc@...r.kernel.org, t.stanislaws@...sung.com,
	kyungmin.park@...sung.com, broonie@...nsource.wolfsonmicro.com,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <robherring2@...il.com>
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@...sung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@...sung.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/  |

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)