| 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
| ||
|
Date: Tue, 06 May 2014 20:36:58 +0200
From: Tomasz Figa <tomasz.figa@...il.com>
To: Pankaj Dubey <pankaj.dubey@...sung.com>,
linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
CC: devicetree@...r.kernel.org, kgene.kim@...sung.com,
Russell King <linux@....linux.org.uk>, arnd@...db.de,
Wolfram Sang <wsa@...-dreams.de>, t.figa@...sung.com,
Randy Dunlap <rdunlap@...radead.org>,
linux-doc@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
linux-i2c@...r.kernel.org
Subject: Re: [PATCH v2 1/6] i2c: s3c2410: Moving I2C interrupt re-configuration
code into i2c driver
Hi Pankaj,
On 06.05.2014 10:51, Pankaj Dubey wrote:
> Let's move I2C interrupt re-configuration code from machine
> file exynos.c to I2C driver. Since only Exynos5250, and
> Exynos5420 need to do this, added syscon based phandle to
> i2c device nodes of respective SoC DT files.
>
> CC: Rob Herring <robh+dt@...nel.org>
> CC: Randy Dunlap <rdunlap@...radead.org>
> CC: Wolfram Sang <wsa@...-dreams.de>
> CC: Russell King <linux@....linux.org.uk>
> CC: devicetree@...r.kernel.org
> CC: linux-doc@...r.kernel.org
> CC: linux-i2c@...r.kernel.org
> Signed-off-by: Pankaj Dubey <pankaj.dubey@...sung.com>
> ---
> .../devicetree/bindings/arm/samsung/sysreg.txt | 1 +
> arch/arm/boot/dts/exynos5.dtsi | 5 ++++
> arch/arm/boot/dts/exynos5250.dtsi | 4 +++
> arch/arm/boot/dts/exynos5420.dtsi | 4 +++
> arch/arm/mach-exynos/exynos.c | 26 -------------------
> drivers/i2c/busses/i2c-s3c2410.c | 27 ++++++++++++++++++++
> 6 files changed, 41 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/samsung/sysreg.txt b/Documentation/devicetree/bindings/arm/samsung/sysreg.txt
> index 0ab3251..fd71581 100644
> --- a/Documentation/devicetree/bindings/arm/samsung/sysreg.txt
> +++ b/Documentation/devicetree/bindings/arm/samsung/sysreg.txt
> @@ -3,6 +3,7 @@ SAMSUNG S5P/Exynos SoC series System Registers (SYSREG)
> Properties:
> - compatible : should contain "samsung,<chip name>-sysreg", "syscon";
> For Exynos4 SoC series it should be "samsung,exynos4-sysreg", "syscon";
> + For Exynos5 SoC series it should be "samsung,exynos5-sysreg", "syscon";
> - reg : offset and length of the register set.
>
> Example:
> diff --git a/arch/arm/boot/dts/exynos5.dtsi b/arch/arm/boot/dts/exynos5.dtsi
> index 79d0608..3027e37 100644
> --- a/arch/arm/boot/dts/exynos5.dtsi
> +++ b/arch/arm/boot/dts/exynos5.dtsi
> @@ -99,4 +99,9 @@
> #size-cells = <0>;
> status = "disabled";
> };
> +
> + sys_reg: syscon@...50000 {
> + compatible = "samsung,exynos5-sysreg", "syscon";
> + reg = <0x10050000 0x400>;
> + };
> };
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index 8d724d5..46f0233 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -285,6 +285,7 @@
> clock-names = "i2c";
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_bus>;
> + samsung,syscon-phandle = <&sys_reg>;
> status = "disabled";
> };
>
> @@ -298,6 +299,7 @@
> clock-names = "i2c";
> pinctrl-names = "default";
> pinctrl-0 = <&i2c1_bus>;
> + samsung,syscon-phandle = <&sys_reg>;
> status = "disabled";
> };
>
> @@ -311,6 +313,7 @@
> clock-names = "i2c";
> pinctrl-names = "default";
> pinctrl-0 = <&i2c2_bus>;
> + samsung,syscon-phandle = <&sys_reg>;
> status = "disabled";
> };
>
> @@ -324,6 +327,7 @@
> clock-names = "i2c";
> pinctrl-names = "default";
> pinctrl-0 = <&i2c3_bus>;
> + samsung,syscon-phandle = <&sys_reg>;
> status = "disabled";
> };
>
> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
> index ff496ad..762128c 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -517,6 +517,7 @@
> clock-names = "i2c";
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_bus>;
> + samsung,syscon-phandle = <&sys_reg>;
> status = "disabled";
> };
>
> @@ -530,6 +531,7 @@
> clock-names = "i2c";
> pinctrl-names = "default";
> pinctrl-0 = <&i2c1_bus>;
> + samsung,syscon-phandle = <&sys_reg>;
> status = "disabled";
> };
>
> @@ -543,6 +545,7 @@
> clock-names = "i2c";
> pinctrl-names = "default";
> pinctrl-0 = <&i2c2_bus>;
> + samsung,syscon-phandle = <&sys_reg>;
> status = "disabled";
> };
>
> @@ -556,6 +559,7 @@
> clock-names = "i2c";
> pinctrl-names = "default";
> pinctrl-0 = <&i2c3_bus>;
> + samsung,syscon-phandle = <&sys_reg>;
> status = "disabled";
> };
>
> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
> index 59eb1f1..54ae2e1 100644
> --- a/arch/arm/mach-exynos/exynos.c
> +++ b/arch/arm/mach-exynos/exynos.c
> @@ -293,32 +293,6 @@ void __init exynos_map_pmu(void)
>
> static void __init exynos_dt_machine_init(void)
> {
> - struct device_node *i2c_np;
> - const char *i2c_compat = "samsung,s3c2440-i2c";
> - unsigned int tmp;
> - int id;
> -
> - /*
> - * Exynos5's legacy i2c controller and new high speed i2c
> - * controller have muxed interrupt sources. By default the
> - * interrupts for 4-channel HS-I2C controller are enabled.
> - * If node for first four channels of legacy i2c controller
> - * are available then re-configure the interrupts via the
> - * system register.
> - */
> - if (soc_is_exynos5()) {
> - for_each_compatible_node(i2c_np, NULL, i2c_compat) {
> - if (of_device_is_available(i2c_np)) {
> - id = of_alias_get_id(i2c_np, "i2c");
> - if (id < 4) {
> - tmp = readl(EXYNOS5_SYS_I2C_CFG);
> - writel(tmp & ~(0x1 << id),
> - EXYNOS5_SYS_I2C_CFG);
> - }
> - }
> - }
> - }
> -
> exynos_map_pmu();
>
> if (!soc_is_exynos5440())
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index ae44910..0420150 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -39,6 +39,8 @@
> #include <linux/of.h>
> #include <linux/of_gpio.h>
> #include <linux/pinctrl/consumer.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
>
> #include <asm/irq.h>
>
> @@ -91,6 +93,9 @@
> /* Max time to wait for bus to become idle after a xfer (in us) */
> #define S3C2410_IDLE_TIMEOUT 5000
>
> +/* Exynos5 Sysreg offset */
> +#define EXYNOS5_SYS_I2C_CFG 0x0234
> +
> /* i2c controller state */
> enum s3c24xx_i2c_state {
> STATE_IDLE,
> @@ -127,6 +132,7 @@ struct s3c24xx_i2c {
> #if defined(CONFIG_ARM_S3C24XX_CPUFREQ)
> struct notifier_block freq_transition;
> #endif
> + struct regmap *sysreg;
> };
>
> static struct platform_device_id s3c24xx_driver_ids[] = {
> @@ -1075,6 +1081,8 @@ static void
> s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
> {
> struct s3c2410_platform_i2c *pdata = i2c->pdata;
> + unsigned int tmp;
> + int id;
>
> if (!np)
> return;
> @@ -1084,6 +1092,25 @@ s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
> of_property_read_u32(np, "samsung,i2c-slave-addr", &pdata->slave_addr);
> of_property_read_u32(np, "samsung,i2c-max-bus-freq",
> (u32 *)&pdata->frequency);
> + /*
> + * Exynos5's legacy i2c controller and new high speed i2c
> + * controller have muxed interrupt sources. By default the
> + * interrupts for 4-channel HS-I2C controller are enabled.
> + * If node for first four channels of legacy i2c controller
> + * are available then re-configure the interrupts via the
> + * system register.
> + */
> + id = of_alias_get_id(np, "i2c");
> + i2c->sysreg = syscon_regmap_lookup_by_phandle(np,
> + "samsung,syscon-phandle");
> + if (IS_ERR(i2c->sysreg)) {
> + i2c->sysreg = NULL;
NULL shouldn't be considered for pointers using the ERR_PTR()
convention, so you should just preserve the value returned by
syscon_regmap_lookup_by_phandle() here.
Also, the driver seems to do this only once in probe, so maybe you
should simply use a local variable here, without storing the regmap in
i2c struct.
> + /* As this is not compulsary do not return error */
Is it? Will the driver work normally without interrupts in this case?
Also, typo: s/compulsary/compulsory/
> + pr_info("i2c-%d skipping re-configuration of interrutps\n", id);
> + return;
> + }
> + regmap_read(i2c->sysreg, EXYNOS5_SYS_I2C_CFG, &tmp);
> + regmap_write(i2c->sysreg, EXYNOS5_SYS_I2C_CFG, tmp & ~(0x1 << id));
You could use regmap_update_bits() [1] here instead, with mask set to
BIT(id) and val set to 0.
[1] http://lxr.free-electrons.com/source/drivers/base/regmap/regmap.c#L1977
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists