Linux I2C development
 help / color / mirror / Atom feed
* [v15, 6/7] base: soc: introduce soc_device_match() interface
From: Yangbo Lu @ 2016-10-28  6:50 UTC (permalink / raw)
  To: linux-mmc-u79uwXL29TY76Z2rM5mHXA,
	ulf.hansson-QSEj5FYQhm4dnm+yROfE0A, Scott Wood, Arnd Bergmann
  Cc: Mark Rutland, Greg Kroah-Hartman, Xiaobo Xie, Minghuan Lian,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-clk-u79uwXL29TY76Z2rM5mHXA, Qiang Zhao, Russell King,
	Bhupesh Sharma, Jochen Friedrich, Claudiu Manoil,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Yangbo Lu, Rob Herring,
	Santosh Shilimkar,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Leo Li,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Kumar Gala,
	linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ
In-Reply-To: <1477637418-38938-1-git-send-email-yangbo.lu-3arQi8VN3Tc@public.gmane.org>

From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

We keep running into cases where device drivers want to know the exact
version of the a SoC they are currently running on. In the past, this has
usually been done through a vendor specific API that can be called by a
driver, or by directly accessing some kind of version register that is
not part of the device itself but that belongs to a global register area
of the chip.

Common reasons for doing this include:

- A machine is not using devicetree or similar for passing data about
  on-chip devices, but just announces their presence using boot-time
  platform devices, and the machine code itself does not care about the
  revision.

- There is existing firmware or boot loaders with existing DT binaries
  with generic compatible strings that do not identify the particular
  revision of each device, but the driver knows which SoC revisions
  include which part.

- A prerelease version of a chip has some quirks and we are using the same
  version of the bootloader and the DT blob on both the prerelease and the
  final version. An update of the DT binding seems inappropriate because
  that would involve maintaining multiple copies of the dts and/or
  bootloader.

This patch introduces the soc_device_match() interface that is meant to
work like of_match_node() but instead of identifying the version of a
device, it identifies the SoC itself using a vendor-agnostic interface.

Unlike of_match_node(), we do not do an exact string compare but instead
use glob_match() to allow wildcards in strings.

Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Signed-off-by: Yangbo Lu <yangbo.lu-3arQi8VN3Tc@public.gmane.org>
Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
---
Changes for v11:
	- Added this patch for soc match
Changes for v12:
	- Corrected the author
	- Rewrited soc_device_match with while loop
Changes for v13:
	- Added ack from Greg
Changes for v14:
	- None
Changes for v15:
	- None
---
 drivers/base/Kconfig    |  1 +
 drivers/base/soc.c      | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |  3 +++
 3 files changed, 70 insertions(+)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index fdf44ca..991b21e 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -235,6 +235,7 @@ config GENERIC_CPU_AUTOPROBE
 
 config SOC_BUS
 	bool
+	select GLOB
 
 source "drivers/base/regmap/Kconfig"
 
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index b63f23e..0c5cf87 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -13,6 +13,7 @@
 #include <linux/spinlock.h>
 #include <linux/sys_soc.h>
 #include <linux/err.h>
+#include <linux/glob.h>
 
 static DEFINE_IDA(soc_ida);
 
@@ -159,3 +160,68 @@ static int __init soc_bus_register(void)
 	return bus_register(&soc_bus_type);
 }
 core_initcall(soc_bus_register);
+
+static int soc_device_match_one(struct device *dev, void *arg)
+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+	const struct soc_device_attribute *match = arg;
+
+	if (match->machine &&
+	    !glob_match(match->machine, soc_dev->attr->machine))
+		return 0;
+
+	if (match->family &&
+	    !glob_match(match->family, soc_dev->attr->family))
+		return 0;
+
+	if (match->revision &&
+	    !glob_match(match->revision, soc_dev->attr->revision))
+		return 0;
+
+	if (match->soc_id &&
+	    !glob_match(match->soc_id, soc_dev->attr->soc_id))
+		return 0;
+
+	return 1;
+}
+
+/*
+ * soc_device_match - identify the SoC in the machine
+ * @matches: zero-terminated array of possible matches
+ *
+ * returns the first matching entry of the argument array, or NULL
+ * if none of them match.
+ *
+ * This function is meant as a helper in place of of_match_node()
+ * in cases where either no device tree is available or the information
+ * in a device node is insufficient to identify a particular variant
+ * by its compatible strings or other properties. For new devices,
+ * the DT binding should always provide unique compatible strings
+ * that allow the use of of_match_node() instead.
+ *
+ * The calling function can use the .data entry of the
+ * soc_device_attribute to pass a structure or function pointer for
+ * each entry.
+ */
+const struct soc_device_attribute *soc_device_match(
+	const struct soc_device_attribute *matches)
+{
+	int ret = 0;
+
+	if (!matches)
+		return NULL;
+
+	while (!ret) {
+		if (!(matches->machine || matches->family ||
+		      matches->revision || matches->soc_id))
+			break;
+		ret = bus_for_each_dev(&soc_bus_type, NULL, (void *)matches,
+				       soc_device_match_one);
+		if (!ret)
+			matches++;
+		else
+			return matches;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(soc_device_match);
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
index 2739ccb..9f5eb06 100644
--- a/include/linux/sys_soc.h
+++ b/include/linux/sys_soc.h
@@ -13,6 +13,7 @@ struct soc_device_attribute {
 	const char *family;
 	const char *revision;
 	const char *soc_id;
+	const void *data;
 };
 
 /**
@@ -34,4 +35,6 @@ void soc_device_unregister(struct soc_device *soc_dev);
  */
 struct device *soc_device_to_device(struct soc_device *soc);
 
+const struct soc_device_attribute *soc_device_match(
+	const struct soc_device_attribute *matches);
 #endif /* __SOC_BUS_H */
-- 
2.1.0.27.g96db324

^ permalink raw reply related

* [v15, 7/7] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
From: Yangbo Lu @ 2016-10-28  6:50 UTC (permalink / raw)
  To: linux-mmc-u79uwXL29TY76Z2rM5mHXA,
	ulf.hansson-QSEj5FYQhm4dnm+yROfE0A, Scott Wood, Arnd Bergmann
  Cc: Mark Rutland, Greg Kroah-Hartman, Xiaobo Xie, Minghuan Lian,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-clk-u79uwXL29TY76Z2rM5mHXA, Qiang Zhao, Russell King,
	Bhupesh Sharma, Jochen Friedrich, Claudiu Manoil,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Yangbo Lu, Rob Herring,
	Santosh Shilimkar,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Leo Li,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Kumar Gala,
	linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ
In-Reply-To: <1477637418-38938-1-git-send-email-yangbo.lu-3arQi8VN3Tc@public.gmane.org>

The eSDHC of T4240-R1.0-R2.0 has incorrect vender version and spec version.
Acturally the right version numbers should be VVN=0x13 and SVN = 0x1.
This patch adds the GUTS driver support for eSDHC driver to match SoC.
And fix host version to avoid that incorrect version numbers break down
the ADMA data transfer.

Signed-off-by: Yangbo Lu <yangbo.lu-3arQi8VN3Tc@public.gmane.org>
Acked-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Acked-by: Scott Wood <oss-fOR+EgIDQEHk1uMJSBkQmQ@public.gmane.org>
---
Changes for v2:
	- Got SVR through iomap instead of dts
Changes for v3:
	- Managed GUTS through syscon instead of iomap in eSDHC driver
Changes for v4:
	- Got SVR by GUTS driver instead of SYSCON
Changes for v5:
	- Changed to get SVR through API fsl_guts_get_svr()
	- Combined patch 4, patch 5 and patch 6 into one
Changes for v6:
	- Added 'Acked-by: Ulf Hansson'
Changes for v7:
	- None
Changes for v8:
	- Added 'Acked-by: Scott Wood'
Changes for v9:
	- None
Changes for v10:
	- None
Changes for v11:
	- Changed to use soc_device_match
Changes for v12:
	- Matched soc through .family field instead of .soc_id
Changes for v13:
	- None
Changes for v14:
	- None
Changes for v15:
	- None
---
 drivers/mmc/host/Kconfig          |  1 +
 drivers/mmc/host/sdhci-of-esdhc.c | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 5274f50..a1135a9 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -144,6 +144,7 @@ config MMC_SDHCI_OF_ESDHC
 	depends on MMC_SDHCI_PLTFM
 	depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
 	select MMC_SDHCI_IO_ACCESSORS
+	select FSL_GUTS
 	help
 	  This selects the Freescale eSDHC controller support.
 
diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index fb71c86..57bdb9e 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -18,6 +18,7 @@
 #include <linux/of.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/sys_soc.h>
 #include <linux/mmc/host.h>
 #include "sdhci-pltfm.h"
 #include "sdhci-esdhc.h"
@@ -28,6 +29,7 @@
 struct sdhci_esdhc {
 	u8 vendor_ver;
 	u8 spec_ver;
+	bool quirk_incorrect_hostver;
 };
 
 /**
@@ -73,6 +75,8 @@ static u32 esdhc_readl_fixup(struct sdhci_host *host,
 static u16 esdhc_readw_fixup(struct sdhci_host *host,
 				     int spec_reg, u32 value)
 {
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_esdhc *esdhc = sdhci_pltfm_priv(pltfm_host);
 	u16 ret;
 	int shift = (spec_reg & 0x2) * 8;
 
@@ -80,6 +84,12 @@ static u16 esdhc_readw_fixup(struct sdhci_host *host,
 		ret = value & 0xffff;
 	else
 		ret = (value >> shift) & 0xffff;
+	/* Workaround for T4240-R1.0-R2.0 eSDHC which has incorrect
+	 * vendor version and spec version information.
+	 */
+	if ((spec_reg == SDHCI_HOST_VERSION) &&
+	    (esdhc->quirk_incorrect_hostver))
+		ret = (VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) | SDHCI_SPEC_200;
 	return ret;
 }
 
@@ -558,6 +568,12 @@ static const struct sdhci_pltfm_data sdhci_esdhc_le_pdata = {
 	.ops = &sdhci_esdhc_le_ops,
 };
 
+static struct soc_device_attribute soc_incorrect_hostver[] = {
+	{ .family = "QorIQ T4240", .revision = "1.0", },
+	{ .family = "QorIQ T4240", .revision = "2.0", },
+	{ },
+};
+
 static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host;
@@ -571,6 +587,10 @@ static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host)
 	esdhc->vendor_ver = (host_ver & SDHCI_VENDOR_VER_MASK) >>
 			     SDHCI_VENDOR_VER_SHIFT;
 	esdhc->spec_ver = host_ver & SDHCI_SPEC_VER_MASK;
+	if (soc_device_match(soc_incorrect_hostver))
+		esdhc->quirk_incorrect_hostver = true;
+	else
+		esdhc->quirk_incorrect_hostver = false;
 }
 
 static int sdhci_esdhc_probe(struct platform_device *pdev)
-- 
2.1.0.27.g96db324

^ permalink raw reply related

* RE: [v13, 5/8] soc: fsl: add GUTS driver for QorIQ platforms
From: Y.B. Lu @ 2016-10-28  7:08 UTC (permalink / raw)
  To: Scott Wood, linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
	Arnd Bergmann
  Cc: Mark Rutland, Greg Kroah-Hartman, X.B. Xie, M.H. Lian,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Qiang Zhao,
	Russell King, Bhupesh Sharma, Jochen Friedrich, Claudiu Manoil,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
	Santosh Shilimkar,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Leo Li,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
In-Reply-To: <1477629956.6812.15.camel@buserror.net>

> -----Original Message-----
> From: Y.B. Lu
> Sent: Friday, October 28, 2016 2:06 PM
> To: Y.B. Lu; 'Scott Wood'; 'linux-mmc@vger.kernel.org';
> 'ulf.hansson@linaro.org'; 'Arnd Bergmann'
> Cc: 'linuxppc-dev@lists.ozlabs.org'; 'devicetree@vger.kernel.org';
> 'linux-arm-kernel@lists.infradead.org'; 'linux-kernel@vger.kernel.org';
> 'linux-clk@vger.kernel.org'; 'linux-i2c@vger.kernel.org';
> 'iommu@lists.linux-foundation.org'; 'netdev@vger.kernel.org'; 'Greg
> Kroah-Hartman'; 'Mark Rutland'; 'Rob Herring'; 'Russell King'; 'Jochen
> Friedrich'; 'Joerg Roedel'; 'Claudiu Manoil'; 'Bhupesh Sharma'; Qiang
> Zhao; 'Kumar Gala'; 'Santosh Shilimkar'; Leo Li; X.B. Xie; M.H. Lian
> Subject: RE: [v13, 5/8] soc: fsl: add GUTS driver for QorIQ platforms
> 
> > -----Original Message-----
> > From: Y.B. Lu
> > Sent: Friday, October 28, 2016 2:00 PM
> > To: 'Scott Wood'; linux-mmc@vger.kernel.org; ulf.hansson@linaro.org;
> > Arnd Bergmann
> > Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org;
> > linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > linux- clk@vger.kernel.org; linux-i2c@vger.kernel.org;
> > iommu@lists.linux- foundation.org; netdev@vger.kernel.org; Greg
> > Kroah-Hartman; Mark Rutland; Rob Herring; Russell King; Jochen
> > Friedrich; Joerg Roedel; Claudiu Manoil; Bhupesh Sharma; Qiang Zhao;
> Kumar Gala; Santosh Shilimkar; Leo Li; X.B.
> > Xie; M.H. Lian
> > Subject: RE: [v13, 5/8] soc: fsl: add GUTS driver for QorIQ platforms
> >
> >
> >
> > > -----Original Message-----
> > > From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> > > owner@vger.kernel.org] On Behalf Of Scott Wood
> > > Sent: Friday, October 28, 2016 12:46 PM
> > > To: Y.B. Lu; linux-mmc@vger.kernel.org; ulf.hansson@linaro.org; Arnd
> > > Bergmann
> > > Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org;
> > > linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > linux- clk@vger.kernel.org; linux-i2c@vger.kernel.org;
> > > iommu@lists.linux- foundation.org; netdev@vger.kernel.org; Greg
> > > Kroah-Hartman; Mark Rutland; Rob Herring; Russell King; Jochen
> > > Friedrich; Joerg Roedel; Claudiu Manoil; Bhupesh Sharma; Qiang Zhao;
> > Kumar Gala; Santosh Shilimkar; Leo Li; X.B.
> > > Xie; M.H. Lian
> > > Subject: Re: [v13, 5/8] soc: fsl: add GUTS driver for QorIQ
> > > platforms
> > >
> > > On Fri, 2016-10-28 at 11:32 +0800, Yangbo Lu wrote:
> > > > +	guts->regs = of_iomap(np, 0);
> > > > +	if (!guts->regs)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	/* Register soc device */
> > > > +	machine = of_flat_dt_get_machine_name();
> > > > +	if (machine)
> > > > +		soc_dev_attr.machine = devm_kstrdup(dev, machine,
> > > > GFP_KERNEL);
> > > > +
> > > > +	svr = fsl_guts_get_svr();
> > > > +	soc_die = fsl_soc_die_match(svr, fsl_soc_die);
> > > > +	if (soc_die) {
> > > > +		soc_dev_attr.family = devm_kasprintf(dev, GFP_KERNEL,
> > > > +						     "QorIQ %s", soc_die-
> > > > >die);
> > > > +	} else {
> > > > +		soc_dev_attr.family = devm_kasprintf(dev, GFP_KERNEL,
> > > > "QorIQ");
> > > > +	}
> > > > +	soc_dev_attr.soc_id = devm_kasprintf(dev, GFP_KERNEL,
> > > > +					     "svr:0x%08x", svr);
> > > > +	soc_dev_attr.revision = devm_kasprintf(dev, GFP_KERNEL,
> "%d.%d",
> > > > +					       SVR_MAJ(svr), SVR_MIN(svr));
> > > > +
> > > > +	soc_dev = soc_device_register(&soc_dev_attr);
> > > > +	if (IS_ERR(soc_dev))
> > > > +		return PTR_ERR(soc_dev);
> > >
> > > ioremap leaks on this error path.  Use devm_ioremap_resource().
> > >
> >
> > [Lu Yangbo-B47093] Ok. I have fixed it in v14. Thanks :)
> 
> [Lu Yangbo-B47093] Sorry, used the wrong error code... Will resent it

[Lu Yangbo-B47093] The v15 had been sent. And dropped patch 'dt: bindings: update Freescale DCFG compatible',
since that work has been done by below patch on ShawnGuo's linux tree.
'dt-bindings: fsl: add LS1043A/LS1046A/LS2080A compatible for SCFG and DCFG'
https://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/commit/?h=imx/dt64&id=981034a2bfcaff5c95dafde24d7abfe7f9025c19

Thanks.

> 
> >
> > > -Scott
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-mmc"
> > > in the body of a message to majordomo@vger.kernel.org More majordomo
> > > info at http://vger.kernel.org/majordomo-info.html
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply

* Re: [Linux-parport] True Parallel Port Interface for Bit-banging?
From: Sebastian Frias @ 2016-10-28 10:25 UTC (permalink / raw)
  To: Maciej S. Szmigiero
  Cc: Wolfram Sang, Sudip Mukherjee, linux-parport, philb, tim,
	Jean Delvare, linux-i2c
In-Reply-To: <7bd6ea08-361e-316f-be7f-d400b59aec0d@maciej.szmigiero.name>

On 10/26/2016 02:04 AM, Maciej S. Szmigiero wrote:
> 
> I don't have experience with bit-banging parallel port but I do have
> some with bit-banging PC serial ports (which should be very similar)
> and can confirm that old DOS software can be *very* fragile with regard
> to timing.
> I have tried dosemu, dosbox and as far I remember also VirtualBox
> with an old, DOS car service app.
> Wasn't able to make it communicate until I booted true DOS.
> That machine had old-style ISA serial ports so it was possible.
> 

Thanks for sharing your experience.
I think it should be related to the latencies Wolfram discussed.

> You also have asked previously whether laptop port would work.
> On my current HP Elitebook ports of both types are wired to Super IO,
> and using PC standard IO port and interrupt numbers.

Would you mind sharing the laptop's model? There are a bunch of
Elitebooks and maybe not all have the ports wired.
Does your laptop has the Parallel Port on the docking or on the laptop
itself?

> 
> I also remember that at least on ThinkPads and HP Compaqs that I had
> contact with (these were laptops from 10+ years ago) both serial and
> parallel ports were wired to Super IO chip (including ones available
> only via port replicator).
> So they were also at PC standard resources.
> 

Yes, it appears that Thinkad T42 could work:

https://forum.linuxcnc.org/forum/49-basic-configuration/15999-thinkpad-t42-parallel-port-not-working

> However, other issue is that sometimes such ports are inactive by
> default (don't respond) until enabled by PnP.
> 
> This is also a problem with PCMCIA cards - they are disabled
> by default.

Do you know the command to enable them thru PnP? Shouldn't loading
the kernel module be enough? Or is it the other way around and 
enabling thru PnP triggers the kernel module load process?

> I have Argosy 2xRS232 PCMCIA card and remember it came with
> DOS driver which could enable them on I/O port close to standard
> one - this was one of their major selling points.
> So your ExpressCard would probably need similar software.
> 

Thanks for the pointer.

^ permalink raw reply

* Re: [v15, 6/7] base: soc: introduce soc_device_match() interface
From: Arnd Bergmann @ 2016-10-28 10:47 UTC (permalink / raw)
  To: linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ
  Cc: Mark Rutland, ulf.hansson-QSEj5FYQhm4dnm+yROfE0A,
	Geert Uytterhoeven, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	Minghuan Lian, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-clk-u79uwXL29TY76Z2rM5mHXA, Qiang Zhao, Russell King,
	Bhupesh Sharma, Claudiu Manoil, devicetree-u79uwXL29TY76Z2rM5mHXA,
	Kumar Gala, Scott Wood, Rob Herring, Santosh Shilimkar,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Greg Kroah-Hartman, linux-mmc-u79uwXL29TY76Z2rM5mHXA, Xiaobo Xie,
	Leo Li, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	Yangbo Lu
In-Reply-To: <1477637418-38938-7-git-send-email-yangbo.lu-3arQi8VN3Tc@public.gmane.org>

On Friday, October 28, 2016 2:50:17 PM CEST Yangbo Lu wrote:
> +
> +static int soc_device_match_one(struct device *dev, void *arg)
> +{
> +       struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> +       const struct soc_device_attribute *match = arg;
> +
> +       if (match->machine &&
> +           !glob_match(match->machine, soc_dev->attr->machine))
> +               return 0;
> +
> +       if (match->family &&
> +           !glob_match(match->family, soc_dev->attr->family))
> +               return 0;
> +
> 

Geert found a bug in my code, and submitted a fix at
https://patchwork.kernel.org/patch/9361395/

I think you should include that one in your series.

	Arnd

^ permalink raw reply

* Re: [v15, 3/7] powerpc/fsl: move mpc85xx.h to include/linux/fsl
From: Arnd Bergmann @ 2016-10-28 10:52 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Yangbo Lu, linux-mmc, ulf.hansson, Scott Wood, Mark Rutland,
	Greg Kroah-Hartman, Xiaobo Xie, Minghuan Lian, linux-i2c,
	linux-clk, Qiang Zhao, Russell King, Bhupesh Sharma, Joerg Roedel,
	Claudiu Manoil, devicetree, Rob Herring, Santosh Shilimkar,
	linux-arm-kernel, netdev, linux-kernel, Leo Li, iommu
In-Reply-To: <1477637418-38938-4-git-send-email-yangbo.lu@nxp.com>

On Friday, October 28, 2016 2:50:14 PM CEST Yangbo Lu wrote:
> Move mpc85xx.h to include/linux/fsl and rename it to svr.h as a common
> header file.  This SVR numberspace is used on some ARM chips as well as
> PPC, and even to check for a PPC SVR multi-arch drivers would otherwise
> need to ifdef the header inclusion and all references to the SVR symbols.
> 
> 

I don't see any of the contents of this header referenced by the soc driver
any more. I think you can just drop this patch.

	Arnd


^ permalink raw reply

* Re: [v15, 0/7] Fix eSDHC host version register bug
From: Arnd Bergmann @ 2016-10-28 10:53 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Yangbo Lu, linux-mmc, ulf.hansson, Scott Wood, Mark Rutland,
	Greg Kroah-Hartman, Xiaobo Xie, Minghuan Lian, linux-i2c,
	linux-clk, Qiang Zhao, Russell King, Bhupesh Sharma, Joerg Roedel,
	Jochen Friedrich, Claudiu Manoil, devicetree, Rob Herring,
	Santosh Shilimkar, netdev, linux-kernel, Leo Li, iommu
In-Reply-To: <1477637418-38938-1-git-send-email-yangbo.lu@nxp.com>

On Friday, October 28, 2016 2:50:11 PM CEST Yangbo Lu wrote:
> This patchset is used to fix a host version register bug in the T4240-R1.0-R2.0
> eSDHC controller. To match the SoC version and revision, 10 previous version
> patchsets had tried many methods but all of them were rejected by reviewers.
> Such as
>         - dts compatible method
>         - syscon method
>         - ifdef PPC method
>         - GUTS driver getting SVR method
> Anrd suggested a soc_device_match method in v10, and this is the only available
> method left now. This v11 patchset introduces the soc_device_match interface in
> soc driver.
> 
> The first five patches of Yangbo are to add the GUTS driver. This is used to
> register a soc device which contain soc version and revision information.
> The other two patches introduce the soc_device_match method in soc driver
> and apply it on esdhc driver to fix this bug.
> 

Looks good overall. With patch 3 dropped (or an explanation why it's still
needed), everything

Acked-by: Arnd Bergmann <arnd@arndb.de>

	Arnd


^ permalink raw reply

* Re: [Linux-parport] True Parallel Port Interface for Bit-banging?
From: Maciej S. Szmigiero @ 2016-10-28 23:12 UTC (permalink / raw)
  To: Sebastian Frias
  Cc: Wolfram Sang, Sudip Mukherjee, linux-parport, philb, tim,
	Jean Delvare, linux-i2c
In-Reply-To: <581327AC.8060404@laposte.net>

On 28.10.2016 12:25, Sebastian Frias wrote:
> On 10/26/2016 02:04 AM, Maciej S. Szmigiero wrote:
>>
>> I don't have experience with bit-banging parallel port but I do have
>> some with bit-banging PC serial ports (which should be very similar)
>> and can confirm that old DOS software can be *very* fragile with regard
>> to timing.
>> I have tried dosemu, dosbox and as far I remember also VirtualBox
>> with an old, DOS car service app.
>> Wasn't able to make it communicate until I booted true DOS.
>> That machine had old-style ISA serial ports so it was possible.
>>
> 
> Thanks for sharing your experience.
> I think it should be related to the latencies Wolfram discussed.

And probably also baudrate UART tweaks than Jan wrote about.

>> You also have asked previously whether laptop port would work.
>> On my current HP Elitebook ports of both types are wired to Super IO,
>> and using PC standard IO port and interrupt numbers.
> 
> Would you mind sharing the laptop's model?

This is Elitebook model 8570p.

> There are a bunch of
> Elitebooks and maybe not all have the ports wired.
> Does your laptop has the Parallel Port on the docking or on the laptop
> itself?

Only a serial port socket is built-it, parallel port socket is
available only via a port replicator.

According to description of this replicator it is compatible with
a lot of different Elitebooks and Probooks:
http://h30094.www3.hp.com/product.aspx?sku=10432928&mfg_part=A7E34AA&pagemode=ca

I wasn't able find any note in this replicator description that port
availability depends on connected laptop model.
You can try to search for particular laptop model schematics, even
if it is available only for sale sometimes there is a preview
page with block diagram which clearly show where the parallel port
is connected (this is true for my laptop model for example).

However, whether the port is on standard I/O address and interrupt
line requires some more digging.

>> I also remember that at least on ThinkPads and HP Compaqs that I had
>> contact with (these were laptops from 10+ years ago) both serial and
>> parallel ports were wired to Super IO chip (including ones available
>> only via port replicator).
>> So they were also at PC standard resources.
>>
> 
> Yes, it appears that Thinkad T42 could work:
> 
> https://forum.linuxcnc.org/forum/49-basic-configuration/15999-thinkpad-t42-parallel-port-not-working
> 
>> However, other issue is that sometimes such ports are inactive by
>> default (don't respond) until enabled by PnP.
>>
>> This is also a problem with PCMCIA cards - they are disabled
>> by default.
> 
> Do you know the command to enable them thru PnP? Shouldn't loading
> the kernel module be enough? Or is it the other way around and 
> enabling thru PnP triggers the kernel module load process?

If you want to use them under Linux then everything should be taken
care of by kernel's PnP support.
It can be problem if you want to use them under pure DOS.

>> I have Argosy 2xRS232 PCMCIA card and remember it came with
>> DOS driver which could enable them on I/O port close to standard
>> one - this was one of their major selling points.
>> So your ExpressCard would probably need similar software.
>>
> 
> Thanks for the pointer.

Maciej

^ permalink raw reply

* Re: [RFC PATCH 3/5] gpio-dmec: gpio support for dmec
From: Linus Walleij @ 2016-10-29  9:05 UTC (permalink / raw)
  To: Zahari Doychev
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Greg KH,
	Lee Jones, Wolfram Sang, Wim Van Sebroeck, Guenter Roeck,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Alexandre Courbot, linux-watchdog-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <fe8ac70204de48edd8a3da8a783b10810f3d4ca1.1477564996.git-series.zahari.doychev-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>

On Thu, Oct 27, 2016 at 12:47 PM, Zahari Doychev
<zahari.doychev-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org> wrote:

> This is support for the gpio functionality found on the Data Modul embedded
> controllers
>
> Signed-off-by: Zahari Doychev <zahari.doychev-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/staging/dmec/Kconfig     |  10 +-
>  drivers/staging/dmec/Makefile    |   1 +-
>  drivers/staging/dmec/dmec.h      |   5 +-
>  drivers/staging/dmec/gpio-dmec.c | 390 ++++++++++++++++++++++++++++++++-

I guess Greg has already asked, but why is this in staging?
The driver doesn't seem very complex or anything, it would not
be overly troublesome to get it into the proper kernel subsystems.

> +config GPIO_DMEC
> +       tristate "Data Modul GPIO"
> +       depends on MFD_DMEC && GPIOLIB

So the depends on GPIOLIB can go away if you just put it into
drivers/gpio with the rest.

> +struct dmec_gpio_platform_data {
> +       int gpio_base;

NAK, always use -1. No hardcoding the GPIO base other than on
legacy systems.

> +       int chip_num;

I suspect you may not need this either. struct platform_device
already contains a ->id field, just use that when instantiating
your driver if you need an instance number.

So I think you need zero platform data for this.

> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/errno.h>
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/gpio.h>

You should only need <linux/gpio/driver.h>

> +#ifdef CONFIG_PM
> +struct dmec_reg_ctx {
> +       u32 dat;
> +       u32 dir;
> +       u32 imask;
> +       u32 icfg[2];
> +       u32 emask[2];
> +};
> +#endif
> +
> +struct dmec_gpio_priv {
> +       struct regmap *regmap;
> +       struct gpio_chip gpio_chip;
> +       struct irq_chip irq_chip;
> +       unsigned int chip_num;
> +       unsigned int irq;
> +       u8 ver;
> +#ifdef CONFIG_PM
> +       struct dmec_reg_ctx regs;
> +#endif
> +};

The #ifdef for saving the state is a bit kludgy. Can't you just have it there
all the time? Or is this a footprint-sensitive system?

> +static int dmec_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct dmec_gpio_priv *priv = container_of(gc, struct dmec_gpio_priv,
> +                                                  gpio_chip);

Use the new pattern with

struct dmec_gpio_priv *priv = gpiochip_get_data(gc);

This needs you to use devm_gpiochip_add_data() below.

> +static int dmec_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct dmec_gpio_priv *priv = container_of(gc, struct dmec_gpio_priv,
> +                                                  gpio_chip);
> +       struct regmap *regmap = priv->regmap;
> +       unsigned int offset, mask, val;
> +
> +       offset = DMEC_GPIO_IRQTYPE_OFFSET(priv) + (d->hwirq >> 2);
> +       mask = ((d->hwirq & 3) << 1);
> +
> +       regmap_read(regmap, offset, &val);
> +
> +       val &= ~(3 << mask);
> +       switch (type & IRQ_TYPE_SENSE_MASK) {
> +       case IRQ_TYPE_LEVEL_LOW:
> +               break;
> +       case IRQ_TYPE_EDGE_RISING:
> +               val |= (1 << mask);
> +               break;
> +       case IRQ_TYPE_EDGE_FALLING:
> +               val |= (2 << mask);
> +               break;
> +       case IRQ_TYPE_EDGE_BOTH:
> +               val |= (3 << mask);
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       regmap_write(regmap, offset, val);
> +
> +       return 0;
> +}

This chip uses handle_simple_irq() which is fine if the chip really has no
edge detector ACK register.

What some chips have is a special register to clearing (ACK:ing) the edge
IRQ which makes it possible for a new IRQ to be handled as soon as
that has happened, and those need to use handle_edge_irq() for edge IRQs
and handle_level_irq() for level IRQs, with the side effect that the edge
IRQ path will additionally call the .irq_ack() callback on the irqchip
when handle_edge_irq() is used. In this case we set handle_bad_irq()
as default handler and set up the right handler i .set_type().

Look at drivers/gpio/gpio-pl061.c for an example.

If you DON'T have a special edge ACK register, keep it like this.

> +static irqreturn_t dmec_gpio_irq_handler(int irq, void *dev_id)
> +{
> +       struct dmec_gpio_priv *p = dev_id;
> +       struct irq_domain *d = p->gpio_chip.irqdomain;
> +       unsigned int irqs_handled = 0;
> +       unsigned int val = 0, stat = 0;
> +
> +       regmap_read(p->regmap, DMEC_GPIO_IRQSTA_OFFSET(p), &val);
> +       stat = val;
> +       while (stat) {
> +               int line = __ffs(stat);
> +               int child_irq = irq_find_mapping(d, line);
> +
> +               handle_nested_irq(child_irq);
> +               stat &= ~(BIT(line));
> +               irqs_handled++;
> +       }

I think you should re-read the status register in the loop. An IRQ may
appear while you are reading.

> +static int dmec_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct dmec_gpio_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +       struct dmec_gpio_priv *priv;
> +       struct gpio_chip *gpio_chip;
> +       struct irq_chip *irq_chip;
> +       int ret = 0;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->regmap = dmec_get_regmap(pdev->dev.parent);

Do you really need a special accessor function to get the regmap like this?
If you just use syscon you get these kind of helpers for free. I don't know
if you can use syscon though, just a suggestion.

> +       priv->chip_num = pdata->chip_num;

As mentioned, why not just use pdev->id

> +       gpio_chip = &priv->gpio_chip;
> +       gpio_chip->label = "gpio-dmec";

You set the label

> +       gpio_chip->owner = THIS_MODULE;
> +       gpio_chip->parent = dev;
> +       gpio_chip->label = dev_name(dev);

You set the label again?

> +       gpio_chip->can_sleep = true;
> +
> +       gpio_chip->base = pdata->gpio_base;

NAK. Use -1

> +#ifdef CONFIG_PM
> +static int dmec_gpio_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +       struct dmec_gpio_priv *p = platform_get_drvdata(pdev);
> +       struct regmap *regmap = p->regmap;
> +       struct dmec_reg_ctx *ctx = &p->regs;
> +
> +       regmap_read(regmap, DMEC_GPIO_BASE(p), &ctx->dat);
> +       regmap_read(regmap, DMEC_GPIO_DIR_OFFSET(p), &ctx->dir);
> +       regmap_read(regmap, DMEC_GPIO_IRQCFG_OFFSET(p), &ctx->imask);
> +       regmap_read(regmap, DMEC_GPIO_IRQTYPE_OFFSET(p), &ctx->emask[0]);
> +       regmap_read(regmap, DMEC_GPIO_IRQTYPE_OFFSET(p) + 1, &ctx->emask[1]);
> +       regmap_read(regmap, DMEC_GPIO_IRQCFG_OFFSET(p), &ctx->icfg[0]);
> +       regmap_read(regmap, DMEC_GPIO_IRQCFG_OFFSET(p) + 1, &ctx->icfg[1]);
> +
> +       devm_free_irq(&pdev->dev, p->irq, p);

That seems a bit violent.

> +static int dmec_gpio_resume(struct platform_device *pdev)
> +{
> +       struct dmec_gpio_priv *p = platform_get_drvdata(pdev);
> +       struct regmap *regmap = p->regmap;
> +       struct dmec_reg_ctx *ctx = &p->regs;
> +       int ret;
> +
> +       regmap_write(regmap, DMEC_GPIO_BASE(p), ctx->dat);
> +       regmap_write(regmap, DMEC_GPIO_DIR_OFFSET(p), ctx->dir);
> +       regmap_write(regmap, DMEC_GPIO_IRQCFG_OFFSET(p), ctx->icfg[0]);
> +       regmap_write(regmap, DMEC_GPIO_IRQCFG_OFFSET(p) + 1, ctx->icfg[1]);
> +       regmap_write(regmap, DMEC_GPIO_IRQTYPE_OFFSET(p), ctx->emask[0]);
> +       regmap_write(regmap, DMEC_GPIO_IRQTYPE_OFFSET(p) + 1, ctx->emask[1]);
> +       regmap_write(regmap, DMEC_GPIO_IRQCFG_OFFSET(p), ctx->imask);
> +       regmap_write(regmap, DMEC_GPIO_EVTSTA_OFFSET(p), 0xff);
> +
> +       ret = devm_request_threaded_irq(&pdev->dev, p->irq,
> +                                       NULL, dmec_gpio_irq_handler,
> +                                       IRQF_ONESHOT | IRQF_SHARED,
> +                                       dev_name(&pdev->dev), p);
> +       if (ret)
> +               dev_err(&pdev->dev, "unable to get irq: %d\n", ret);

Re-requesting the IRQ for every suspend/resume cycle?

That's not right.

Look into the wakeup API. You need to tell the core whether this IRQ
should be masked during suspend or not, the default is to mask it I think.

This is too big hammer to solve that.

> +static struct platform_driver dmec_gpio_driver = {
> +       .driver = {
> +               .name = "dmec-gpio",
> +               .owner = THIS_MODULE,
> +       },
> +       .probe = dmec_gpio_probe,
> +       .remove = dmec_gpio_remove,
> +       .suspend = dmec_gpio_suspend,
> +       .resume = dmec_gpio_resume,
> +};

Don't use the legacy suspend/resume callbacks.

Use the DEV_PM_OPS directly in .pm in .driver above. It should
work the same.

(You probably need to fix this on the other patches too.)

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH -next] i2c: digicolor: use clk_disable_unprepare instead of clk_unprepare
From: Wei Yongjun @ 2016-10-29 16:31 UTC (permalink / raw)
  To: Wolfram Sang, Baruch Siach; +Cc: Wei Yongjun, linux-i2c, linux-arm-kernel

From: Wei Yongjun <weiyongjun1@huawei.com>

since clk_prepare_enable() is used to get i2c->clk, we should
use clk_disable_unprepare() to release it for the error path.

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
 drivers/i2c/busses/i2c-digicolor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-digicolor.c b/drivers/i2c/busses/i2c-digicolor.c
index 49f2084..50813a2 100644
--- a/drivers/i2c/busses/i2c-digicolor.c
+++ b/drivers/i2c/busses/i2c-digicolor.c
@@ -347,7 +347,7 @@ static int dc_i2c_probe(struct platform_device *pdev)
 
 	ret = i2c_add_adapter(&i2c->adap);
 	if (ret < 0) {
-		clk_unprepare(i2c->clk);
+		clk_disable_unprepare(i2c->clk);
 		return ret;
 	}

^ permalink raw reply related

* Re: [PATCH -next] i2c: digicolor: use clk_disable_unprepare instead of clk_unprepare
From: Baruch Siach @ 2016-10-30  4:45 UTC (permalink / raw)
  To: Wei Yongjun; +Cc: Wolfram Sang, Wei Yongjun, linux-i2c, linux-arm-kernel
In-Reply-To: <1477758677-7831-1-git-send-email-weiyj.lk@gmail.com>

Hi Wei Yongjun,

On Sat, Oct 29, 2016 at 04:31:17PM +0000, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
> 
> since clk_prepare_enable() is used to get i2c->clk, we should
> use clk_disable_unprepare() to release it for the error path.
> 
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

Acked-by: Baruch Siach <baruch@tkos.co.il>

Thanks,
baruch

>  drivers/i2c/busses/i2c-digicolor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-digicolor.c b/drivers/i2c/busses/i2c-digicolor.c
> index 49f2084..50813a2 100644
> --- a/drivers/i2c/busses/i2c-digicolor.c
> +++ b/drivers/i2c/busses/i2c-digicolor.c
> @@ -347,7 +347,7 @@ static int dc_i2c_probe(struct platform_device *pdev)
>  
>  	ret = i2c_add_adapter(&i2c->adap);
>  	if (ret < 0) {
> -		clk_unprepare(i2c->clk);
> +		clk_disable_unprepare(i2c->clk);
>  		return ret;
>  	}

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

^ permalink raw reply

* Re: [PATCH] Add IDT 89HPESx EEPROM/CSR driver
From: Greg KH @ 2016-10-30 13:53 UTC (permalink / raw)
  To: Serge Semin; +Cc: arnd, wsa, jdelvare, linux-i2c, linux-kernel, Sergey.Semin
In-Reply-To: <1475450025-29507-1-git-send-email-fancer.lancer@gmail.com>

On Mon, Oct 03, 2016 at 02:13:45AM +0300, Serge Semin wrote:
> Hello linux folks,
> 
>     This driver primarily is developed to give an access to EEPROM of IDT
> PCIe-switches. Such switches provide a simple SMBus interface to perform
> IO-operations from/to EEPROM, which is located at private (also called master)
> SMBus of the switches. Using that interface this driver creates a simple
> binary sysfs-file in the device directory:
> /sys/bus/i2c/devices/<bus>-<devaddr>/eeprom
> In case if read-only flag is specified in the device dts-node, user-space
> applications won't be able to write to the EEPROM sysfs-node.
> 
>     Additionally IDT 89HPESx SMBus interface provides an ability to write/read
> data of device CSRs. This driver exposes corresponding sysfs-file to perform
> simple IO operations using that facility for just basic debug purpose.

If it's for debugging, please put it in debugfs, not in sysfs.  sysfs
files for one-off drivers is usually discouraged, but at the least, you
have to document it in a Documentation/ABI/ file.  For debugfs files, we
don't care, you can do whatever you want in them :)

> Particularly next file is created in the device specific sysfs-directory:
> /sys/bus/i2c/devices/<bus>-<devaddr>/csr
> Format of the sysfs-node is:
> $ cat /sys/bus/i2c/devices/<bus>-<devaddr>/csr;
> <CSR address>:<CSR value>
> So reading the content of the sysfs-file gives current CSR address and
> it value. If user-space application wishes to change current CSR address,
> it can just write a proper value to the sysfs-file:
> $ echo "<CSR address>" > /sys/bus/i2c/devices/<bus>-<devaddr>/csr
> If it wants to change the CSR value as well, the format of the write
> operation is:
> $ echo "<CSR address>:<CSR value>" > \
>         /sys/bus/i2c/devices/<bus>-<devaddr>/csr;
> CSR address and value can be any of hexadecimal, decimal or octal format.

Yeah, that all should go into debugfs.

>     The driver supports the most of the commonly available SMBus operations:
> SMBus i2c block, SMBus block, smbus word and byte. The code has been tested
> to be built for x32/x64 MIPS architecture and tested on the x32 MIPS machine.
> The patch was applied on top of commit c6935931c1894ff857616ff8549b61236a19148f
> of master branch of repository
> git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
> 
> 
> Thanks,
> 
> =============================
> Serge V. Semin
> Leading Programmer
> Embedded SW development group
> T-platforms
> =============================
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

meta-comment, the "hello", and "thanks" and signature doesn't need to be
in a changelog text, next time you can drop that.  See the many kernel
patches on the mailing lists for examples of how to do this.


> 
> ---
>  .../devicetree/bindings/misc/idt_89hpesx.txt       |   41 +


Can you split this out into a separate file and be sure to cc: the patch
to the devicetree maintainers?

>  drivers/misc/eeprom/Kconfig                        |   10 +
>  drivers/misc/eeprom/Makefile                       |    1 +
>  drivers/misc/eeprom/idt_89hpesx.c                  | 1483 ++++++++++++++++++++
>  4 files changed, 1535 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/idt_89hpesx.txt
>  create mode 100644 drivers/misc/eeprom/idt_89hpesx.c
> 
> diff --git a/Documentation/devicetree/bindings/misc/idt_89hpesx.txt b/Documentation/devicetree/bindings/misc/idt_89hpesx.txt
> new file mode 100644
> index 0000000..469cc93
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/idt_89hpesx.txt
> @@ -0,0 +1,41 @@
> +EEPROM / CSR SMBus-slave interface of IDT 89HPESx devices
> +
> +Required properties:
> +  - compatible : should be "<manufacturer>,<type>"
> +		 Basically there is only one manufacturer: idt, but some
> +		 compatible devices may be produced in future. Following devices
> +		 are supported: 89hpes8nt2, 89hpes12nt3, 89hpes24nt6ag2,
> +		 89hpes32nt8ag2, 89hpes32nt8bg2, 89hpes12nt12g2, 89hpes16nt16g2,
> +		 89hpes24nt24g2, 89hpes32nt24ag2, 89hpes32nt24bg2;
> +		 89hpes12n3, 89hpes12n3a, 89hpes24n3, 89hpes24n3a;
> +		 89hpes32h8, 89hpes32h8g2, 89hpes48h12, 89hpes48h12g2,
> +		 89hpes48h12ag2, 89hpes16h16, 89hpes22h16, 89hpes22h16g2,
> +		 89hpes34h16, 89hpes34h16g2, 89hpes64h16, 89hpes64h16g2,
> +		 89hpes64h16ag2;
> +		 89hpes12t3g2, 89hpes24t3g2, 89hpes16t4, 89hpes4t4g2,
> +		 89hpes10t4g2, 89hpes16t4g2, 89hpes16t4ag2, 89hpes5t5,
> +		 89hpes6t5, 89hpes8t5, 89hpes8t5a, 89hpes24t6, 89hpes6t6g2,
> +		 89hpes24t6g2, 89hpes16t7, 89hpes32t8, 89hpes32t8g2,
> +		 89hpes48t12, 89hpes48t12g2.
> +		 Current implementation of the driver doesn't have any device-
> +		 specific functionalities. But since each of them differs
> +		 by registers mapping, CSRs read/write restrictions can be
> +		 added in future.
> +  - reg :	 I2C address of the IDT 89HPES device.
> +
> +Optional properties:
> +  - read-only :	 Parameterless property disables writes to the EEPROM
> +  - idt,eesize : Size of EEPROM device connected to IDT 89HPES i2c-master bus
> +		 (default value is 4096 bytes if option isn't specified)
> +  - idt,eeaddr : Custom address of EEPROM device
> +		 (If not specified IDT 89HPESx device will try to communicate
> +		  with EEPROM sited by default address - 0x50)
> +
> +Example:
> +	idt_pcie_sw@60 {
> +		compatible = "idt,89hpes12nt3";
> +		reg = <0x60>;
> +		read-only;
> +		idt,eesize = <65536>;
> +		idt,eeaddr = <0x50>;
> +	};
> diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
> index c4e41c2..de58762 100644
> --- a/drivers/misc/eeprom/Kconfig
> +++ b/drivers/misc/eeprom/Kconfig
> @@ -100,4 +100,14 @@ config EEPROM_DIGSY_MTC_CFG
>  
>  	  If unsure, say N.
>  
> +config EEPROM_IDT_89HPESX
> +	tristate "IDT 89HPESx PCIe-swtiches EEPROM / CSR support"
> +	depends on I2C && SYSFS
> +	help
> +	  Enable this driver to get read/write access to EEPROM / CSRs
> +	  over IDT PCIe-swtich i2c-slave interface.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called idt_89hpesx.
> +
>  endmenu
> diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
> index fc1e81d..90a5262 100644
> --- a/drivers/misc/eeprom/Makefile
> +++ b/drivers/misc/eeprom/Makefile
> @@ -5,3 +5,4 @@ obj-$(CONFIG_EEPROM_MAX6875)	+= max6875.o
>  obj-$(CONFIG_EEPROM_93CX6)	+= eeprom_93cx6.o
>  obj-$(CONFIG_EEPROM_93XX46)	+= eeprom_93xx46.o
>  obj-$(CONFIG_EEPROM_DIGSY_MTC_CFG) += digsy_mtc_eeprom.o
> +obj-$(CONFIG_EEPROM_IDT_89HPESX) += idt_89hpesx.o
> diff --git a/drivers/misc/eeprom/idt_89hpesx.c b/drivers/misc/eeprom/idt_89hpesx.c
> new file mode 100644
> index 0000000..f95f4f0
> --- /dev/null
> +++ b/drivers/misc/eeprom/idt_89hpesx.c
> @@ -0,0 +1,1483 @@
> +/*
> + *   This file is provided under a GPLv2 license.  When using or
> + *   redistributing this file, you may do so under that license.
> + *
> + *   GPL LICENSE SUMMARY
> + *
> + *   Copyright (C) 2016 T-Platforms All Rights Reserved.
> + *
> + *   This program is free software; you can redistribute it and/or modify it
> + *   under the terms and conditions of the GNU General Public License,
> + *   version 2, as published by the Free Software Foundation.
> + *
> + *   This program is distributed in the hope that it will be useful, but WITHOUT
> + *   ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + *   FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + *   more details.
> + *
> + *   You should have received a copy of the GNU General Public License along
> + *   with this program; if not, it can be found <http://www.gnu.org/licenses/>.
> + *
> + *   The full GNU General Public License is included in this distribution in
> + *   the file called "COPYING".
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + *
> + * IDT PCIe-switch NTB Linux driver
> + *
> + * Contact Information:
> + * Serge Semin <fancer.lancer@gmail.com>, <Sergey.Semin@t-platforms.ru>
> + */
> +/*
> + *           NOTE of the IDT 89HPESx SMBus-slave interface driver
> + *    This driver primarily is developed to have an access to EEPROM device of
> + * IDT PCIe-switches. IDT provides a simple SMBus interface to perform IO-
> + * operations from/to EEPROM, which is located at private (so called Master)
> + * SMBus of switches. Using that interface this the driver creates a simple
> + * binary sysfs-file in the device directory:
> + * /sys/bus/i2c/devices/<bus>-<devaddr>/eeprom
> + * In case if read-only flag is specified in the dts-node of device desription,
> + * User-space applications won't be able to write to the EEPROM sysfs-node.
> + *    Additionally IDT 89HPESx SMBus interface has an ability to write/read
> + * data of device CSRs. This driver exposes another sysfs-file to perform
> + * simple IO operations using that ability for just basic debug purpose.
> + * Particularly next file is created in the device specific sysfs-directory:
> + * /sys/bus/i2c/devices/<bus>-<devaddr>/csr
> + * Format of the sysfs-node is:
> + * $ cat /sys/bus/i2c/devices/<bus>-<devaddr>/csr;
> + * <CSR address>:<CSR value>
> + * So reading the content of the sysfs-file gives current CSR address and
> + * it value. If User-space application wishes to change current CSR address,
> + * it can just write a proper value to the sysfs-file:
> + * $ echo "<CSR address>" > /sys/bus/i2c/devices/<bus>-<devaddr>/csr
> + * If it wants to change the CSR value as well, the format of the write
> + * operation is:
> + * $ echo "<CSR address>:<CSR value>" > \
> + *        /sys/bus/i2c/devices/<bus>-<devaddr>/csr;
> + * CSR address and value can be any of hexadecimal, decimal or octal format.
> + */
> +
> +/*
> + * Note: You can load this module with either option 'dyndbg=+p' or define the
> + * next preprocessor constant

What does this option do?  I don't understand it.

> + */
> +/* #define DEBUG */

Ah, just drop all of this, dynamic debugging is known how to be used
through the whole kernel, no need to document it again :)

> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/sysfs.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/of.h>
> +#include <linux/i2c.h>
> +#include <linux/pci_ids.h>

why pci_ids.h?

> +
> +#define IDT_NAME		"89hpesx"
> +#define IDT_89HPESX_DESC	"IDT 89HPESx SMBus-slave interface driver"
> +#define IDT_89HPESX_VER		"1.0"
> +
> +MODULE_DESCRIPTION(IDT_89HPESX_DESC);
> +MODULE_VERSION(IDT_89HPESX_VER);
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("T-platforms");
> +
> +/*
> + * Some common constant used in the driver for better readability:
> + * @SUCCESS:	Success of a function execution
> + */
> +#define SUCCESS (0)

No need to define this, we all know that 0 is "success", this is the
kernel.

> +
> +/*
> + * struct idt_89hpesx_dev - IDT 89HPESx device data structure
> + * @eesize:	Size of EEPROM in bytes (calculated from "idt,eecompatible")
> + * @eero:	EEPROM Read-only flag
> + * @eeaddr:	EEPROM custom address
> + *
> + * @inieecmd:	Initial cmd value for EEPROM read/write operations
> + * @inicsrcmd:	Initial cmd value for CSR read/write operations
> + * @iniccode:	Initialial command code value for IO-operations
> + *
> + * @csr:	CSR address to perform read operation
> + *
> + * @smb_write:	SMBus write method
> + * @smb_read:	SMBus read method
> + * @smb_mtx:	SMBus mutex
> + *
> + * @client:	i2c client used to perform IO operations
> + *
> + * @eenode:	EEPROM sysfs-node to read/write data to/from EEPROM
> + * @regnode:	Register sysfs-node to read/write CSRs
> + */
> +struct idt_smb_seq;
> +struct idt_89hpesx_dev {
> +	u32 eesize;
> +	bool eero;
> +	u8 eeaddr;
> +
> +	u8 inieecmd;
> +	u8 inicsrcmd;
> +	u8 iniccode;
> +
> +	atomic_t csr;
> +
> +	int (*smb_write)(struct idt_89hpesx_dev *, const struct idt_smb_seq *);
> +	int (*smb_read)(struct idt_89hpesx_dev *, struct idt_smb_seq *);
> +	struct mutex smb_mtx;
> +
> +	struct i2c_client *client;
> +
> +	struct bin_attribute eenode;
> +	struct bin_attribute regnode;
> +};
> +#define to_pdev_kobj(__kobj) \
> +	dev_get_drvdata(container_of(__kobj, struct device, kobj))
> +
> +/*
> + * struct idt_smb_seq - sequence of data to be read/written from/to IDT 89HPESx
> + * @ccode:	SMBus command code
> + * @bytecnt:	Byte count of operation
> + * @data:	Data to by written
> + */
> +struct idt_smb_seq {
> +	u8 ccode;
> +	u8 bytecnt;
> +	u8 *data;
> +};
> +
> +/*
> + * struct idt_eeprom_seq - sequence of data to be read/written from/to EEPROM
> + * @cmd:	Transaction CMD
> + * @eeaddr:	EEPROM custom address
> + * @memaddr:	Internal memory address of EEPROM
> + * @data:	Data to be written at the memory address
> + */
> +struct idt_eeprom_seq {
> +	u8 cmd;
> +	u8 eeaddr;
> +	u16 memaddr;

endian?  Is this big or little?

> +	u8 data;
> +} __packed;
> +
> +/*
> + * struct idt_csr_seq - sequence of data to be read/written from/to CSR
> + * @cmd:	Transaction CMD
> + * @csraddr:	Internal IDT device CSR address
> + * @data:	Data to be read/written from/to the CSR address
> + */
> +struct idt_csr_seq {
> +	u8 cmd;
> +	u16 csraddr;
> +	u32 data;

Same for these, what endian are they?

> +} __packed;
> +
> +/*
> + * SMBus command code macros
> + * @CCODE_END:		Indicates the end of transaction
> + * @CCODE_START:	Indicates the start of transaction
> + * @CCODE_CSR:		CSR read/write transaction
> + * @CCODE_EEPROM:	EEPROM read/write transaction
> + * @CCODE_BYTE:		Supplied data has BYTE length
> + * @CCODE_WORD:		Supplied data has WORD length
> + * @CCODE_BLOCK:	Supplied data has variable length passed in bytecnt
> + *			byte right following CCODE byte
> + */
> +#define CCODE_END	((u8)0x01)
> +#define CCODE_START	((u8)0x02)
> +#define CCODE_CSR	((u8)0x00)
> +#define CCODE_EEPROM	((u8)0x04)
> +#define CCODE_BYTE	((u8)0x00)
> +#define CCODE_WORD	((u8)0x20)
> +#define CCODE_BLOCK	((u8)0x40)
> +#define CCODE_PEC	((u8)0x80)
> +
> +/*
> + * EEPROM command macros
> + * @EEPROM_OP_WRITE:	EEPROM write operation
> + * @EEPROM_OP_READ:	EEPROM read operation
> + * @EEPROM_USA:		Use specified address of EEPROM
> + * @EEPROM_NAERR:	EEPROM device is not ready to respond
> + * @EEPROM_LAERR:	EEPROM arbitration loss error
> + * @EEPROM_MSS:		EEPROM misplace start & stop bits error
> + * @EEPROM_WR_CNT:	Bytes count to perform write operation
> + * @EEPROM_WRRD_CNT:	Bytes count to write before reading
> + * @EEPROM_RD_CNT:	Bytes count to perform read operation
> + * @EEPROM_DEF_SIZE:	Fall back size of EEPROM
> + * @EEPROM_DEF_ADDR:	Defatul EEPROM address
> + */
> +#define EEPROM_OP_WRITE	((u8)0x00)
> +#define EEPROM_OP_READ	((u8)0x01)
> +#define EEPROM_USA	((u8)0x02)
> +#define EEPROM_NAERR	((u8)0x08)
> +#define EEPROM_LAERR    ((u8)0x10)
> +#define EEPROM_MSS	((u8)0x20)
> +#define EEPROM_WR_CNT	((u8)5)
> +#define EEPROM_WRRD_CNT	((u8)4)
> +#define EEPROM_RD_CNT	((u8)5)
> +#define EEPROM_DEF_SIZE	((u16)4096)
> +#define EEPROM_DEF_ADDR	((u8)0x50)
> +
> +/*
> + * CSR command macros
> + * @CSR_DWE:		Enable all four bytes of the operation
> + * @CSR_OP_WRITE:	CSR write operation
> + * @CSR_OP_READ:	CSR read operation
> + * @CSR_RERR:		Read operation error
> + * @CSR_WERR:		Write operation error
> + * @CSR_WR_CNT:		Bytes count to perform write operation
> + * @CSR_WRRD_CNT:	Bytes count to write before reading
> + * @CSR_RD_CNT:		Bytes count to perform read operation
> + * @CSR_MAX:		Maximum CSR address
> + * @CSR_DEF:		Default CSR address
> + * @CSR_REAL_ADDR:	CSR real unshifted address
> + */
> +#define CSR_DWE			((u8)0x0F)
> +#define CSR_OP_WRITE		((u8)0x00)
> +#define CSR_OP_READ		((u8)0x10)
> +#define CSR_RERR		((u8)0x40)
> +#define CSR_WERR		((u8)0x80)
> +#define CSR_WR_CNT		((u8)7)
> +#define CSR_WRRD_CNT		((u8)3)
> +#define CSR_RD_CNT		((u8)7)
> +#define CSR_MAX			((u32)0x3FFFF)
> +#define CSR_DEF			((u16)0x0000)
> +#define CSR_REAL_ADDR(val)	((unsigned int)val << 2)
> +
> +/*
> + * IDT 89HPESx basic register
> + * @IDT_VIDDID_CSR:	PCIe VID and DID of IDT 89HPESx
> + * @IDT_VID_MASK:	Mask of VID
> + */
> +#define IDT_VIDDID_CSR	((u32)0x0000)
> +#define IDT_VID_MASK	((u32)0xFFFF)
> +
> +/*
> + * IDT 89HPESx can send NACK when new command is sent before previous one
> + * fininshed execution. In this case driver retries operation
> + * certain times.
> + * @RETRY_CNT:		Number of retries before giving up and fail
> + * @idt_smb_safe:	Generate a retry loop on corresponding SMBus method
> + */
> +#define RETRY_CNT (128)
> +#define idt_smb_safe(ops, args...) ({ \
> +	int __retry = RETRY_CNT; \
> +	s32 __sts; \
> +	do { \
> +		__sts = i2c_smbus_ ## ops ## _data(args); \
> +	} while (__retry-- && __sts < SUCCESS); \
> +	__sts; \
> +})
> +
> +/*
> + * Wrapper dev_err/dev_warn/dev_info/dev_dbg macros
> + */
> +#define dev_err_idt(pdev, args...) \
> +	dev_err(&pdev->client->dev, ## args)
> +#define dev_warn_idt(pdev, args...) \
> +	dev_warn(&pdev->client->dev, ## args)
> +#define dev_info_idt(pdev, args...) \
> +	dev_info(&pdev->client->dev, ## args)
> +#define dev_dbg_idt(pdev, args...) \
> +	dev_dbg(&pdev->client->dev, ## args)

ick, don't wrap them, just use them as-is, it's not that hard to write
them out please.

> +
> +/*===========================================================================
> + *                         i2c bus level IO-operations
> + *===========================================================================
> + */
> +
> +/*
> + * idt_smb_write_byte() - SMBus write method when I2C_SMBUS_BYTE_DATA operation
> + *                        is only available
> + * @pdev:	Pointer to the driver data
> + * @seq:	Sequence of data to be written
> + */
> +static int idt_smb_write_byte(struct idt_89hpesx_dev *pdev,
> +			      const struct idt_smb_seq *seq)
> +{
> +	s32 sts;
> +	u8 ccode;
> +	int idx;
> +
> +	/* Loop over the supplied data sending byte one-by-one */
> +	for (idx = 0; idx < seq->bytecnt; idx++) {
> +		/* Collect the command code byte */
> +		ccode = seq->ccode | CCODE_BYTE;
> +		if (idx == 0)
> +			ccode |= CCODE_START;
> +		if (idx == seq->bytecnt - 1)
> +			ccode |= CCODE_END;
> +
> +		/* Send data to the device */
> +		sts = idt_smb_safe(write_byte, pdev->client, ccode,
> +			seq->data[idx]);
> +		if (sts != SUCCESS)
> +			return (int)sts;
> +	}
> +
> +	return SUCCESS;
> +}
> +
> +/*
> + * idt_smb_read_byte() - SMBus read method when I2C_SMBUS_BYTE_DATA operation
> + *                        is only available
> + * @pdev:	Pointer to the driver data
> + * @seq:	Buffer to read data to
> + */
> +static int idt_smb_read_byte(struct idt_89hpesx_dev *pdev,
> +			     struct idt_smb_seq *seq)
> +{
> +	s32 sts;
> +	u8 ccode;
> +	int idx;
> +
> +	/* Loop over the supplied buffer receiving byte one-by-one */
> +	for (idx = 0; idx < seq->bytecnt; idx++) {
> +		/* Collect the command code byte */
> +		ccode = seq->ccode | CCODE_BYTE;
> +		if (idx == 0)
> +			ccode |= CCODE_START;
> +		if (idx == seq->bytecnt - 1)
> +			ccode |= CCODE_END;
> +
> +		/* Read data from the device */
> +		sts = idt_smb_safe(read_byte, pdev->client, ccode);
> +		if (sts < SUCCESS)
> +			return (int)sts;
> +
> +		seq->data[idx] = (u8)sts;
> +	}
> +
> +	return SUCCESS;
> +}
> +
> +/*
> + * idt_smb_write_word() - SMBus write method when I2C_SMBUS_BYTE_DATA and
> + *                        I2C_FUNC_SMBUS_WORD_DATA operations are available
> + * @pdev:	Pointer to the driver data
> + * @seq:	Sequence of data to be written
> + */
> +static int idt_smb_write_word(struct idt_89hpesx_dev *pdev,
> +			      const struct idt_smb_seq *seq)
> +{
> +	s32 sts;
> +	u8 ccode;
> +	int idx, evencnt;
> +
> +	/* Calculate the even count of data to send */
> +	evencnt = seq->bytecnt - (seq->bytecnt % 2);
> +
> +	/* Loop over the supplied data sending two bytes at a time */
> +	for (idx = 0; idx < evencnt; idx += 2) {
> +		/* Collect the command code byte */
> +		ccode = seq->ccode | CCODE_WORD;
> +		if (idx == 0)
> +			ccode |= CCODE_START;
> +		if (idx == evencnt - 2)
> +			ccode |= CCODE_END;
> +
> +		/* Send word data to the device */
> +		sts = idt_smb_safe(write_word, pdev->client, ccode,
> +			*(u16 *)&seq->data[idx]);
> +		if (sts != SUCCESS)
> +			return (int)sts;
> +	}
> +
> +	/* If there is odd number of bytes then send just one last byte */
> +	if (seq->bytecnt != evencnt) {
> +		/* Collect the command code byte */
> +		ccode = seq->ccode | CCODE_BYTE | CCODE_END;
> +		if (idx == 0)
> +			ccode |= CCODE_START;
> +
> +		/* Send byte data to the device */
> +		sts = idt_smb_safe(write_byte, pdev->client, ccode,
> +			seq->data[idx]);
> +		if (sts != SUCCESS)
> +			return (int)sts;
> +	}
> +
> +	return SUCCESS;
> +}
> +
> +/*
> + * idt_smb_read_word() - SMBus read method when I2C_SMBUS_BYTE_DATA and
> + *                       I2C_FUNC_SMBUS_WORD_DATA operations are available
> + * @pdev:	Pointer to the driver data
> + * @seq:	Buffer to read data to
> + */
> +static int idt_smb_read_word(struct idt_89hpesx_dev *pdev,
> +			     struct idt_smb_seq *seq)
> +{
> +	s32 sts;
> +	u8 ccode;
> +	int idx, evencnt;
> +
> +	/* Calculate the even count of data to send */
> +	evencnt = seq->bytecnt - (seq->bytecnt % 2);
> +
> +	/* Loop over the supplied data reading two bytes at a time */
> +	for (idx = 0; idx < evencnt; idx += 2) {
> +		/* Collect the command code byte */
> +		ccode = seq->ccode | CCODE_WORD;
> +		if (idx == 0)
> +			ccode |= CCODE_START;
> +		if (idx == evencnt - 2)
> +			ccode |= CCODE_END;
> +
> +		/* Read word data from the device */
> +		sts = idt_smb_safe(read_word, pdev->client, ccode);
> +		if (sts < SUCCESS)
> +			return (int)sts;
> +
> +		*(u16 *)&seq->data[idx] = (u16)sts;
> +	}
> +
> +	/* If there is odd number of bytes then receive just one last byte */
> +	if (seq->bytecnt != evencnt) {
> +		/* Collect the command code byte */
> +		ccode = seq->ccode | CCODE_BYTE | CCODE_END;
> +		if (idx == 0)
> +			ccode |= CCODE_START;
> +
> +		/* Read last data byte from the device */
> +		sts = idt_smb_safe(read_byte, pdev->client, ccode);
> +		if (sts < SUCCESS)
> +			return (int)sts;
> +
> +		seq->data[idx] = (u8)sts;
> +	}
> +
> +	return SUCCESS;
> +}
> +
> +/*
> + * idt_smb_write_block() - SMBus write method when I2C_SMBUS_BLOCK_DATA
> + *                         operation is available
> + * @pdev:	Pointer to the driver data
> + * @seq:	Sequence of data to be written
> + */
> +static int idt_smb_write_block(struct idt_89hpesx_dev *pdev,
> +			       const struct idt_smb_seq *seq)
> +{
> +	u8 ccode;
> +
> +	/* Return error if too much data passed to send */
> +	if (seq->bytecnt > I2C_SMBUS_BLOCK_MAX)
> +		return -EINVAL;
> +
> +	/* Collect the command code byte */
> +	ccode = seq->ccode | CCODE_BLOCK | CCODE_START | CCODE_END;
> +
> +	/* Send block of data to the device */
> +	return idt_smb_safe(write_block, pdev->client, ccode, seq->bytecnt,
> +		seq->data);
> +}
> +
> +/*
> + * idt_smb_read_block() - SMBus read method when I2C_SMBUS_BLOCK_DATA
> + *                        operation is available
> + * @pdev:	Pointer to the driver data
> + * @seq:	Buffer to read data to
> + */
> +static int idt_smb_read_block(struct idt_89hpesx_dev *pdev,
> +			      struct idt_smb_seq *seq)
> +{
> +	s32 sts;
> +	u8 ccode;
> +
> +	/* Return error if too much data passed to send */
> +	if (seq->bytecnt > I2C_SMBUS_BLOCK_MAX)
> +		return -EINVAL;
> +
> +	/* Collect the command code byte */
> +	ccode = seq->ccode | CCODE_BLOCK | CCODE_START | CCODE_END;
> +
> +	/* Read block of data from the device */
> +	sts = idt_smb_safe(read_block, pdev->client, ccode, seq->data);
> +	if (sts != seq->bytecnt)
> +		return (sts < SUCCESS ? sts : -ENODATA);
> +
> +	return SUCCESS;
> +}
> +
> +/*
> + * idt_smb_write_i2c_block() - SMBus write method when I2C_SMBUS_I2C_BLOCK_DATA
> + *                             operation is available
> + * @pdev:	Pointer to the driver data
> + * @seq:	Sequence of data to be written
> + *
> + * NOTE It's usual SMBus write block operation, except the actual data length is
> + * sent as first byte of data
> + */
> +static int idt_smb_write_i2c_block(struct idt_89hpesx_dev *pdev,
> +				   const struct idt_smb_seq *seq)
> +{
> +	u8 ccode, buf[I2C_SMBUS_BLOCK_MAX + 1];
> +
> +	/* Return error if too much data passed to send */
> +	if (seq->bytecnt > I2C_SMBUS_BLOCK_MAX)
> +		return -EINVAL;
> +
> +	/* Collect the data to send. Length byte must be added prior the data */
> +	buf[0] = seq->bytecnt;
> +	memcpy(&buf[1], seq->data, seq->bytecnt);
> +
> +	/* Collect the command code byte */
> +	ccode = seq->ccode | CCODE_BLOCK | CCODE_START | CCODE_END;
> +
> +	/* Send length and block of data to the device */
> +	return idt_smb_safe(write_i2c_block, pdev->client, ccode,
> +		seq->bytecnt + 1, buf);
> +}
> +
> +/*
> + * idt_smb_read_i2c_block() - SMBus read method when I2C_SMBUS_I2C_BLOCK_DATA
> + *                            operation is available
> + * @pdev:	Pointer to the driver data
> + * @seq:	Buffer to read data to
> + *
> + * NOTE It's usual SMBus read block operation, except the actual data length is
> + * retrieved as first byte of data
> + */
> +static int idt_smb_read_i2c_block(struct idt_89hpesx_dev *pdev,
> +				  struct idt_smb_seq *seq)
> +{
> +	u8 ccode, buf[I2C_SMBUS_BLOCK_MAX + 1];
> +	s32 sts;
> +
> +	/* Return error if too much data passed to send */
> +	if (seq->bytecnt > I2C_SMBUS_BLOCK_MAX)
> +		return -EINVAL;
> +
> +	/* Collect the command code byte */
> +	ccode = seq->ccode | CCODE_BLOCK | CCODE_START | CCODE_END;
> +
> +	/* Read length and block of data from the device */
> +	sts = idt_smb_safe(read_i2c_block, pdev->client, ccode,
> +		seq->bytecnt + 1, buf);
> +	if (sts != seq->bytecnt + 1)
> +		return (sts < SUCCESS ? sts : -ENODATA);
> +	if (buf[0] != seq->bytecnt)
> +		return -ENODATA;
> +
> +	/* Copy retrieved data to the output data buffer */
> +	memcpy(seq->data, &buf[1], seq->bytecnt);
> +
> +	return SUCCESS;
> +}
> +
> +/*===========================================================================
> + *                          EEPROM IO-operations
> + *===========================================================================
> + */
> +
> +/*
> + * idt_eeprom_write() - EEPROM write operation
> + * @pdev:	Pointer to the driver data
> + * @memaddr:	Start EEPROM memory address
> + * @len:	Length of data to be written
> + * @data:	Data to be written to EEPROM
> + */
> +static int idt_eeprom_write(struct idt_89hpesx_dev *pdev, u16 memaddr, u16 len,
> +			    const u8 *data)
> +{
> +	struct idt_eeprom_seq eeseq;
> +	struct idt_smb_seq smbseq;
> +	int ret, retry;
> +	u16 idx;
> +
> +	/* Initialize SMBus sequence fields */
> +	smbseq.ccode = pdev->iniccode | CCODE_EEPROM;
> +	smbseq.data = (u8 *)&eeseq;
> +
> +	/* Send data byte-by-byte, checking if it is successfully written */
> +	for (idx = 0; idx < len; idx++, memaddr++) {
> +		/* Lock IDT SMBus device */
> +		mutex_lock(&pdev->smb_mtx);
> +
> +		/* Perform write operation */
> +		smbseq.bytecnt = EEPROM_WR_CNT;
> +		eeseq.cmd = pdev->inieecmd | EEPROM_OP_WRITE;
> +		eeseq.eeaddr = pdev->eeaddr;
> +		eeseq.memaddr = cpu_to_le16(memaddr);
> +		eeseq.data = data[idx];
> +		ret = pdev->smb_write(pdev, &smbseq);
> +		if (ret != SUCCESS) {
> +			dev_err_idt(pdev,
> +				"Failed to write 0x%04hx:0x%02hhx to eeprom",
> +				memaddr, data[idx]);
> +			goto err_mutex_unlock;
> +		}
> +
> +		/*
> +		 * Check whether the data is successfully written by reading
> +		 * from the same EEPROM memory address. Sometimes EEPROM may
> +		 * respond with NACK if it's busy with writing previous data,
> +		 * so we need to perform a few attempts of read cycle
> +		 */
> +		retry = RETRY_CNT;
> +		do {
> +			/* Send EEPROM memory address to read data from */
> +			smbseq.bytecnt = EEPROM_WRRD_CNT;
> +			eeseq.cmd = pdev->inieecmd | EEPROM_OP_READ;
> +			ret = pdev->smb_write(pdev, &smbseq);
> +			if (ret != SUCCESS) {
> +				dev_err_idt(pdev,
> +					"Failed to init mem address 0x%02hhx",
> +					memaddr);
> +				goto err_mutex_unlock;
> +			}
> +
> +			/* Perform read operation */
> +			smbseq.bytecnt = EEPROM_RD_CNT;
> +			eeseq.data = ~data[idx];
> +			ret = pdev->smb_read(pdev, &smbseq);
> +			if (ret != SUCCESS) {
> +				dev_err_idt(pdev,
> +					"Failed to read mem address 0x%02hhx",
> +					memaddr);
> +				goto err_mutex_unlock;
> +			}
> +		} while (retry-- && (eeseq.cmd & EEPROM_NAERR));
> +
> +		/* Check whether IDT successfully sent data to EEPROM */
> +		if (eeseq.cmd & (EEPROM_NAERR | EEPROM_LAERR | EEPROM_MSS)) {
> +			dev_err_idt(pdev, "Communication with EEPROM failed");
> +			ret = -EREMOTEIO;
> +			goto err_mutex_unlock;
> +		}
> +		if (eeseq.data != data[idx]) {
> +			dev_err_idt(pdev,
> +				"Values don't match 0x%02hhx != 0x%02hhx",
> +				eeseq.data, data[idx]);
> +			ret = -EREMOTEIO;
> +			goto err_mutex_unlock;
> +		}
> +
> +		/* Unlock IDT SMBus device */
> +err_mutex_unlock:
> +		mutex_unlock(&pdev->smb_mtx);
> +		if (ret != SUCCESS)
> +			return ret;
> +	}
> +
> +	return SUCCESS;
> +}
> +
> +/*
> + * idt_eeprom_read() - EEPROM read operation
> + * @pdev:	Pointer to the driver data
> + * @memaddr:	Start EEPROM memory address
> + * @len:	Length of data to read
> + * @buf:	Buffer to read data to
> + */
> +static int idt_eeprom_read(struct idt_89hpesx_dev *pdev, u16 memaddr, u16 len,
> +			   u8 *buf)
> +{
> +	struct idt_eeprom_seq eeseq;
> +	struct idt_smb_seq smbseq;
> +	u16 idx;
> +	int ret;
> +
> +	/* Initialize SMBus sequence fields */
> +	smbseq.ccode = pdev->iniccode | CCODE_EEPROM;
> +	smbseq.data = (u8 *)&eeseq;
> +
> +	/* Send data one-by-one, checking if it is successfully written */
> +	for (idx = 0; idx < len; idx++, memaddr++) {
> +		/* Lock IDT SMBus device */
> +		mutex_lock(&pdev->smb_mtx);
> +
> +		/* Send EEPROM memory address to read data from */
> +		smbseq.bytecnt = EEPROM_WRRD_CNT;
> +		eeseq.cmd = pdev->inieecmd | EEPROM_OP_READ;
> +		eeseq.eeaddr = pdev->eeaddr;
> +		eeseq.memaddr = cpu_to_le16(memaddr);
> +		ret = pdev->smb_write(pdev, &smbseq);
> +		if (ret != SUCCESS) {
> +			dev_err_idt(pdev, "Failed to init mem address 0x%02hhx",
> +				memaddr);
> +			goto err_mutex_unlock;
> +		}
> +
> +		/* Perform read operation (rest of fields stay the same) */
> +		smbseq.bytecnt = EEPROM_RD_CNT;
> +		ret = pdev->smb_read(pdev, &smbseq);
> +		if (ret != SUCCESS) {
> +			dev_err_idt(pdev,
> +				"Failed to read eeprom address 0x%02hhx",
> +				memaddr);
> +			ret = -EREMOTEIO;
> +			goto err_mutex_unlock;
> +		}
> +
> +		/* Check whether IDT successfully read data from EEPROM */
> +		if (eeseq.cmd & (EEPROM_NAERR | EEPROM_LAERR | EEPROM_MSS)) {
> +			dev_err_idt(pdev, "Communication with eeprom failed");
> +			ret = -EREMOTEIO;
> +			goto err_mutex_unlock;
> +		}
> +
> +		/* Save retrieved data */
> +		buf[idx] = eeseq.data;
> +
> +		/* Unlock IDT SMBus device */
> +err_mutex_unlock:
> +		mutex_unlock(&pdev->smb_mtx);
> +		if (ret != SUCCESS)
> +			return ret;
> +	}
> +
> +	return SUCCESS;
> +}
> +
> +/*===========================================================================
> + *                          CSR IO-operations
> + *===========================================================================
> + */
> +
> +/*
> + * idt_csr_write() - CSR write operation
> + * @pdev:	Pointer to the driver data
> + * @csraddr:	CSR address (with no two LS bits)
> + * @data:	Data to be written to CSR
> + */
> +static int idt_csr_write(struct idt_89hpesx_dev *pdev, u16 csraddr,
> +			 const u32 data)
> +{
> +	struct idt_csr_seq csrseq;
> +	struct idt_smb_seq smbseq;
> +	int ret;
> +
> +	/* Initialize SMBus sequence fields */
> +	smbseq.ccode = pdev->iniccode | CCODE_CSR;
> +	smbseq.data = (u8 *)&csrseq;
> +
> +	/* Lock IDT SMBus device */
> +	mutex_lock(&pdev->smb_mtx);
> +
> +	/* Perform write operation */
> +	smbseq.bytecnt = CSR_WR_CNT;
> +	csrseq.cmd = pdev->inicsrcmd | CSR_OP_WRITE;
> +	csrseq.csraddr = cpu_to_le16(csraddr);
> +	csrseq.data = cpu_to_le32(data);
> +	ret = pdev->smb_write(pdev, &smbseq);
> +	if (ret != SUCCESS) {
> +		dev_err_idt(pdev, "Failed to write 0x%04x: 0x%04x to csr",
> +			CSR_REAL_ADDR(csraddr), data);
> +		goto err_mutex_unlock;
> +	}
> +
> +	/* Send CSR address to read data from */
> +	smbseq.bytecnt = CSR_WRRD_CNT;
> +	csrseq.cmd = pdev->inicsrcmd | CSR_OP_READ;
> +	ret = pdev->smb_write(pdev, &smbseq);
> +	if (ret != SUCCESS) {
> +		dev_err_idt(pdev, "Failed to init csr address 0x%04x",
> +			CSR_REAL_ADDR(csraddr));
> +		goto err_mutex_unlock;
> +	}
> +
> +	/* Perform read operation */
> +	smbseq.bytecnt = CSR_RD_CNT;
> +	ret = pdev->smb_read(pdev, &smbseq);
> +	if (ret != SUCCESS) {
> +		dev_err_idt(pdev, "Failed to read csr 0x%04x",
> +			CSR_REAL_ADDR(csraddr));
> +		goto err_mutex_unlock;
> +	}
> +
> +	/* Check whether IDT successfully retrieved CSR data */
> +	if (csrseq.cmd & (CSR_RERR | CSR_WERR)) {
> +		dev_err_idt(pdev, "IDT failed to perform CSR r/w");
> +		ret = -EREMOTEIO;
> +		goto err_mutex_unlock;
> +	}
> +
> +	/* Unlock IDT SMBus device */
> +err_mutex_unlock:
> +	mutex_unlock(&pdev->smb_mtx);
> +
> +	return ret;
> +}
> +
> +/*
> + * idt_csr_read() - CSR read operation
> + * @pdev:	Pointer to the driver data
> + * @csraddr:	CSR address (with no two LS bits)
> + * @data:	Data to be written to CSR
> + */
> +static int idt_csr_read(struct idt_89hpesx_dev *pdev, u16 csraddr, u32 *data)
> +{
> +	struct idt_csr_seq csrseq;
> +	struct idt_smb_seq smbseq;
> +	int ret;
> +
> +	/* Initialize SMBus sequence fields */
> +	smbseq.ccode = pdev->iniccode | CCODE_CSR;
> +	smbseq.data = (u8 *)&csrseq;
> +
> +	/* Lock IDT SMBus device */
> +	mutex_lock(&pdev->smb_mtx);
> +
> +	/* Send CSR register address before reading it */
> +	smbseq.bytecnt = CSR_WRRD_CNT;
> +	csrseq.cmd = pdev->inicsrcmd | CSR_OP_READ;
> +	csrseq.csraddr = cpu_to_le16(csraddr);
> +	ret = pdev->smb_write(pdev, &smbseq);
> +	if (ret != SUCCESS) {
> +		dev_err_idt(pdev, "Failed to init csr address 0x%04x",
> +			CSR_REAL_ADDR(csraddr));
> +		goto err_mutex_unlock;
> +	}
> +
> +	/* Perform read operation */
> +	smbseq.bytecnt = CSR_RD_CNT;
> +	ret = pdev->smb_read(pdev, &smbseq);
> +	if (ret != SUCCESS) {
> +		dev_err_idt(pdev, "Failed to read csr 0x%04hx",
> +			CSR_REAL_ADDR(csraddr));
> +		goto err_mutex_unlock;
> +	}
> +
> +	/* Check whether IDT successfully retrieved CSR data */
> +	if (csrseq.cmd & (CSR_RERR | CSR_WERR)) {
> +		dev_err_idt(pdev, "IDT failed to perform CSR r/w");
> +		ret = -EREMOTEIO;
> +		goto err_mutex_unlock;
> +	}
> +
> +	/* Save data retrieved from IDT */
> +	*data = le32_to_cpu(csrseq.data);
> +
> +	/* Unlock IDT SMBus device */
> +err_mutex_unlock:
> +	mutex_unlock(&pdev->smb_mtx);
> +
> +	return ret;
> +}
> +
> +/*===========================================================================
> + *                          Sysfs-nodes IO-operations
> + *===========================================================================
> + */
> +
> +/*
> + * idt_sysfs_eeprom_write() - EEPROM sysfs-node write callback
> + * @filep:	Pointer to the file system node
> + * @kobj:	Pointer to the kernel object related to the sysfs-node
> + * @attr:	Attributes of the file
> + * @buf:	Buffer to write data to
> + * @off:	Offset at which data should be written to
> + * @count:	Number of bytes to write
> + */
> +static ssize_t idt_sysfs_eeprom_write(struct file *filp, struct kobject *kobj,
> +				      struct bin_attribute *attr,
> +				      char *buf, loff_t off, size_t count)
> +{
> +	struct idt_89hpesx_dev *pdev;
> +	int ret;
> +
> +	/* Retrieve driver data */
> +	pdev = to_pdev_kobj(kobj);
> +
> +	/* Perform EEPROM write operation */
> +	ret = idt_eeprom_write(pdev, (u16)off, (u16)count, (u8 *)buf);
> +	return (ret != SUCCESS ? ret : count);
> +}
> +
> +/*
> + * idt_sysfs_eeprom_read() - EEPROM sysfs-node read callback
> + * @filep:	Pointer to the file system node
> + * @kobj:	Pointer to the kernel object related to the sysfs-node
> + * @attr:	Attributes of the file
> + * @buf:	Buffer to write data to
> + * @off:	Offset at which data should be written to
> + * @count:	Number of bytes to write
> + */
> +static ssize_t idt_sysfs_eeprom_read(struct file *filp, struct kobject *kobj,
> +				     struct bin_attribute *attr,
> +				     char *buf, loff_t off, size_t count)
> +{
> +	struct idt_89hpesx_dev *pdev;
> +	int ret;
> +
> +	/* Retrieve driver data */
> +	pdev = to_pdev_kobj(kobj);
> +
> +	/* Perform EEPROM read operation */
> +	ret = idt_eeprom_read(pdev, (u16)off, (u16)count, (u8 *)buf);
> +	return (ret != SUCCESS ? ret : count);
> +}
> +
> +/*
> + * idt_sysfs_csr_store() - CSR sysfs-node write callback
> + * @kobj:	Pointer to the kernel object related to the sysfs-node
> + * @attr:	Attributes of the file
> + * @buf:	Buffer to write data to
> + * @count:	Size of the buffer
> + *
> + * It accepts either "0x<reg addr>:0x<value>" for saving register address
> + * and writing value to specified DWORD register or "0x<reg addr>" for
> + * just saving register address in order to perform next read operation.
> + *
> + * WARNING No spaces are allowed. Incoming string must be strictly formated as:
> + * "<reg addr>:<value>". Register address must be aligned within 4 bytes
> + * (one DWORD).
> + */
> +static ssize_t idt_sysfs_csr_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct idt_89hpesx_dev *pdev;
> +	char *colon_ch, *csraddr_str, *csrval_str;
> +	int ret, csraddr_len, csrval_len;
> +	u32 csraddr, csrval;
> +
> +	/* Retrieve driver data */
> +	pdev = dev_get_drvdata(dev);
> +
> +	/* Find position of colon in the buffer */
> +	colon_ch = strnchr(buf, count, ':');
> +
> +	/*
> +	 * If there is colon passed then new CSR value should be parsed as
> +	 * well, so allocate buffer for CSR address substring.
> +	 * If no colon is found, then string must have just one number with
> +	 * no new CSR value
> +	 */
> +	if (colon_ch != NULL) {
> +		csraddr_len = colon_ch - buf;
> +		csraddr_str =
> +			kmalloc(sizeof(char)*(csraddr_len + 1), GFP_KERNEL);
> +		if (csraddr_str == NULL)
> +			return -ENOMEM;
> +		/* Copy the register address to the substring buffer */
> +		strncpy(csraddr_str, buf, csraddr_len);
> +		csraddr_str[csraddr_len] = '\0';
> +		/* Register value must follow the colon */
> +		csrval_str = colon_ch + 1;
> +		csrval_len = strnlen(csrval_str, count - csraddr_len);
> +	} else /* if (str_colon == NULL) */ {
> +		csraddr_str = (char *)buf; /* Just to shut warning up */
> +		csraddr_len = strnlen(csraddr_str, count);
> +		csrval_str = NULL;
> +		csrval_len = 0;
> +	}
> +
> +	/* Convert CSR address to u32 value */
> +	ret = kstrtou32(csraddr_str, 0, &csraddr);
> +	if (ret != SUCCESS)
> +		goto free_csraddr_str;
> +
> +	/* Check whether passed register address is valid */
> +	if (csraddr > CSR_MAX || !IS_ALIGNED(csraddr, SZ_4)) {
> +		ret = -EINVAL;
> +		goto free_csraddr_str;
> +	}
> +
> +	/* Shift register address to the right so to have u16 address */
> +	csraddr >>= 2;
> +
> +	/* Parse new CSR value and send it to IDT, if colon has been found */
> +	if (colon_ch != NULL) {
> +		ret = kstrtou32(csrval_str, 0, &csrval);
> +		if (ret != SUCCESS)
> +			goto free_csraddr_str;
> +
> +		ret = idt_csr_write(pdev, (u16)csraddr, csrval);
> +		if (ret != SUCCESS)
> +			goto free_csraddr_str;
> +	}
> +
> +	/* Save CSR address in the data structure for future read operations */
> +	atomic_set(&pdev->csr, (int)csraddr);
> +
> +	/* Free memory only if colon has been found */
> +free_csraddr_str:
> +	if (colon_ch != NULL)
> +		kfree(csraddr_str);
> +
> +	return (ret != SUCCESS ? ret : count);
> +}
> +
> +/*
> + * idt_sysfs_csr_show() - CSR sysfs-node read callback
> + * @kobj:	Pointer to the kernel object related to the sysfs-node
> + * @attr:	Attributes of the file
> + * @buf:	Buffer to write data to
> + *
> + * It just prints the pair "0x<reg addr>:0x<value>" to passed buffer.
> + */
> +static ssize_t idt_sysfs_csr_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct idt_89hpesx_dev *pdev;
> +	u32 csraddr, csrval;
> +	int ret;
> +
> +	/* Retrieve driver data */
> +	pdev = dev_get_drvdata(dev);
> +
> +	/* Read current CSR address */
> +	csraddr = atomic_read(&pdev->csr);
> +
> +	/* Perform CSR read operation */
> +	ret = idt_csr_read(pdev, (u16)csraddr, &csrval);
> +	if (ret != SUCCESS)
> +		return ret;
> +
> +	/* Shift register address to the left so to have real address */
> +	csraddr <<= 2;
> +
> +	/* Print the "0x<reg addr>:0x<value>" to buffer */
> +	return snprintf(buf, PAGE_SIZE, "0x%05x:0x%08x\n",
> +		(unsigned int)csraddr, (unsigned int)csrval);
> +}
> +
> +/*
> + * eeprom_attribute - EEPROM sysfs-node attributes
> + *
> + * NOTE Size will be changed in compliance with OF node. EEPROM attribute will
> + * be read-only as well if the corresponding flag is specified in OF node.
> + */
> +static struct bin_attribute eeprom_attribute = {
> +	.attr = {
> +		.name = "eeprom",
> +		.mode = S_IRUGO | S_IWUSR
> +	},
> +	.size = EEPROM_DEF_SIZE,
> +	.write = idt_sysfs_eeprom_write,
> +	.read = idt_sysfs_eeprom_read
> +};
> +
> +/*
> + * csr_attribute - CSR sysfs-node attributes
> + */
> +static struct device_attribute csr_attribute = {
> +	.attr = {
> +		.name = "csr",
> +		.mode = S_IRUGO | S_IWUSR
> +	},
> +	.store = idt_sysfs_csr_store,
> +	.show = idt_sysfs_csr_show,
> +};

Note, for the future, just use DEVICE_ATTR_RW() for stuff like this.

But convert it to debugfs please.

thanks,

greg k-h

^ permalink raw reply

* Re: [PATCH] i2c: imx: add slave support. v2 status
From: Frkuska, Joshua @ 2016-10-31  2:14 UTC (permalink / raw)
  To: Maxim Syrchin, linux-i2c
  Cc: wsa, peda, Jiada_Wang, linux-kernel, Zapolskiy, Vladimir,
	Baxter, Jim
In-Reply-To: <2404b260-4bd6-8ff6-3898-c9c63aaf6855@dev.rtsoft.ru>

Hello Maxim,

Thank you very much for the intermediate patch. I am in the process of 
reviewing it. Please let me clarify a few questions I have.

1. What alternative to "bus busy/bus free/IBB" polling do you have in 
mind? This seems like a reasonable approach to me.
2. What are the major points you consider in need of refactoring?
3. You mention race conditions being fixed in this version relating to 
bus-locking by the slave and breaking slave transactions by the master. 
Does this mean mixed slave/master mode works to your satisfaction? If 
not, what work still needs to be done here?
4. You mention the need for a slave locking test and a work-around 
(checking IMX_I2C_I2DR and IBB) being in-place. Why is this work-around 
not sufficient?

Thanks again,

Joshua


On 10/28/2016 04:38 AM, Maxim Syrchin wrote:
> Hi,
> Sorry for huge delay in answering. Unfortunately we don't have enough 
> resources now to prepare clean enough patch to be accepted by community.
> Please find the latest version attached.  Driver has passed stress 
> tests, but looks like it need seriuos refactoring (it is unnecessarily 
> complicated).
> We still have polling in slave code. Since imx doesn't generate 
> interrupt on "bus busy"/"bus free" events we have to test IBB bit in 
> polling loop.
> Comparing to v2 version several race conditions were fixed (bus 
> locking by slave, breaking slave transaction by starting master xfer). 
> v2 is working pretty good in slave-only or master-only mode. It is 
> reasonable to add  slave locking test  - sometimes imx6 hw doesn't 
> release data line. As workaround we use dummy reading of IMX_I2C_I2DR 
> if driver is in  slave mode and IBB bit is set for a long time.
>
> Thanks,
> Maxim Syrchin
>
>
> 27.10.2016 10:31, Frkuska, Joshua пишет:
>> Hi Maxim, Dmitriy, Wolfram,
>>
>> If there is no immediate plan for a third release of the below patch 
>> set, would it be possible to continue with merging v2 after 
>> addressing the remaining concerns?
>>
>>
>> Thank you and regards,
>>
>> Joshua
>>> Hi Maxim,
>>>
>>> On 2016-03-04 11:06:10 in the thread "Re: [PATCH] i2c: imx: add 
>>> slave support. v2"
>>> referenced here:   https://patchwork.ozlabs.org/patch/573353/ you said:
>>>> Hi Wolfram,
>>>> I'm now working on creating new driver version. I think I'll be 
>>>> able to
>>>> sent it soon.
>>> Do you still plan to release a driver update for an i2c imx driver 
>>> slave support?
>>>
>>> Best regards,
>>> Jim Baxter
>>>
>

-- 
_______________________________________________
Joshua Frkuska | Embedded Software Engineer
Mentor Graphics Japan Co., ltd. | +81-3-6866-7611

PRIVACY AND CONFIDENTIALITY NOTICE
This email and any attachments may contain confidential or privileged information for the sole use of the intended recipient.  Any review, reliance or distribution by others or forwarding without express permission is strictly prohibited.  If you are not the intended recipient, please contact the sender and delete all copies.

^ permalink raw reply

* Re: [PATCH v3 1/2] Documentation: tpm: add the IBM Virtual TPM device tree binding documentation
From: Rob Herring @ 2016-10-31  3:47 UTC (permalink / raw)
  To: Nayna Jain
  Cc: mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA,
	pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
	wsa-z923LK4zBo2bacvFa/9K2g,
	honclo-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, galak-sgV2jX0FEOL9JmXXK+q4OQ,
	cclaudio-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8
In-Reply-To: <1477473705-12746-1-git-send-email-nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

On Wed, Oct 26, 2016 at 05:21:44AM -0400, Nayna Jain wrote:
> Virtual TPM, which is being used on IBM POWER7+ and POWER8 systems running
> POWERVM, is currently supported by tpm device driver but lacks the
> documentation. This patch adds the missing documentation for the existing
> support.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
> Changelog v3:
> 
> - No changes done.
> 
> Changelog v2:
> 
> - New Patch.
> 
>  .../devicetree/bindings/security/tpm/ibmvtpm.txt   | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik

^ permalink raw reply

* Re: [PATCH v3 2/2] Documentation: tpm: add the Physical TPM device tree binding documentation
From: Rob Herring @ 2016-10-31  4:02 UTC (permalink / raw)
  To: Nayna Jain
  Cc: devicetree, tpmdd-devel, wsa, pawel.moll, mark.rutland,
	ijc+devicetree, galak, linux-i2c, peterhuewe, tpmdd,
	jarkko.sakkinen, jgunthorpe, hellerda, ltcgcw, cclaudio, honclo
In-Reply-To: <1477473705-12746-2-git-send-email-nayna@linux.vnet.ibm.com>

On Wed, Oct 26, 2016 at 05:21:45AM -0400, Nayna Jain wrote:
> Newly added support of TPM 2.0 eventlog securityfs pseudo files in tpm
> device driver consumes device tree bindings representing I2C based
> Physical TPM. This patch adds the documentation for corresponding device
> tree bindings of I2C based Physical TPM. These bindings are similar to
> vtpm device tree bindings being used on IBM Power7+ and Power8 Systems
> running PowerVM.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> ---
> Changelog v3:
>  - Added documentation for "label" property.
> 
> Changelog v2:
> 
> - Include review feedbacks.
>   - Move the doc within bindings/security/tpm.
>   - Add example for compatible property in description.
>   - Delete implicit properties like status, label from description.
>   - Redefine linux,sml-base description.
> 
>  .../devicetree/bindings/security/tpm/tpm-i2c.txt    | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* [PATCH v3] i2c: designware: Implement support for SMBus block read and write
From: tnhuynh @ 2016-10-31  6:51 UTC (permalink / raw)
  To: Jarkko Nikula, Andy Shevchenko, Mika Westerberg, Wolfram Sang,
	linux-i2c, linux-kernel
  Cc: Loc Ho, Thang Nguyen, Phong Vo, patches, Tin Huynh

From: Tin Huynh <tnhuynh@apm.com>

Free and Open IPMI use SMBUS BLOCK Read/Write to support SSIF protocol.
However, I2C Designware Core Driver doesn't handle the case at the moment.
The below patch supports this feature.

Signed-off-by: Tin Huynh <tnhuynh@apm.com>
---
Change from V2:
- Change subject of email
- Add a helper function to handle
  length byte receiving
Change from V1:
- Remove empty lines
- Add flags variable to make clean code
- Change DW_DEFAULT_FUNCTIONALITY
  in i2c-designware-pcidrv.c
---
 drivers/i2c/busses/i2c-designware-core.c    |   45 +++++++++++++++++++++++++--
 drivers/i2c/busses/i2c-designware-pcidrv.c  |    1 +
 drivers/i2c/busses/i2c-designware-platdrv.c |    1 +
 3 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 1fe93c4..69c7ab8 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -543,6 +543,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	intr_mask = DW_IC_INTR_DEFAULT_MASK;
 
 	for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
+		u32 flags = msgs[dev->msg_write_idx].flags;
 		/*
 		 * if target address has changed, we need to
 		 * reprogram the target address in the i2c
@@ -588,8 +589,15 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 			 * detected from the registers so we set it always
 			 * when writing/reading the last byte.
 			 */
+
+			/*
+			 * i2c-core.c always set the buffer length of
+			 * I2C_FUNC_SMBUS_BLOCK_DATA to 1. The length will
+			 * be adjusted when receiving the first byte.
+			 * Thus we can't stop the transaction here.
+			 */
 			if (dev->msg_write_idx == dev->msgs_num - 1 &&
-			    buf_len == 1)
+			    buf_len == 1 && !(flags & I2C_M_RECV_LEN))
 				cmd |= BIT(9);
 
 			if (need_restart) {
@@ -614,7 +622,12 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 		dev->tx_buf = buf;
 		dev->tx_buf_len = buf_len;
 
-		if (buf_len > 0) {
+		/*
+		 * Because we don't know the buffer length in the
+		 * I2C_FUNC_SMBUS_BLOCK_DATA case, we can't stop
+		 * the transaction here.
+		 */
+		if (buf_len > 0 || flags & I2C_M_RECV_LEN) {
 			/* more bytes to be written */
 			dev->status |= STATUS_WRITE_IN_PROGRESS;
 			break;
@@ -635,6 +648,25 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	dw_writel(dev, intr_mask,  DW_IC_INTR_MASK);
 }
 
+static u8
+i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
+{
+	struct i2c_msg *msgs = dev->msgs;
+	u32 flags = msgs[dev->msg_read_idx].flags;
+
+	/*
+	 * Adjust the buffer length and mask the flag
+	 * after receiving the first byte
+	 */
+	len = (flags & I2C_CLIENT_PEC) ? len + 2 : len + 1;
+	dev->tx_buf_len = len > dev->rx_outstanding ?
+		len - dev->rx_outstanding : 0;
+	msgs[dev->msg_read_idx].len = len;
+	msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
+
+	return len;
+}
+
 static void
 i2c_dw_read(struct dw_i2c_dev *dev)
 {
@@ -659,7 +691,14 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 		rx_valid = dw_readl(dev, DW_IC_RXFLR);
 
 		for (; len > 0 && rx_valid > 0; len--, rx_valid--) {
-			*buf++ = dw_readl(dev, DW_IC_DATA_CMD);
+			u32 flags = msgs[dev->msg_read_idx].flags;
+			*buf = dw_readl(dev, DW_IC_DATA_CMD);
+			/* Ensure length byte is a valid value */
+			if (flags & I2C_M_RECV_LEN &&
+				*buf <= I2C_SMBUS_BLOCK_MAX && *buf > 0) {
+				len = i2c_dw_recv_len(dev, *buf);
+			}
+			buf++;
 			dev->rx_outstanding--;
 		}
 
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 96f8230..8ffe2da 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -75,6 +75,7 @@ struct dw_pci_controller {
 					I2C_FUNC_SMBUS_BYTE |		\
 					I2C_FUNC_SMBUS_BYTE_DATA |	\
 					I2C_FUNC_SMBUS_WORD_DATA |	\
+					I2C_FUNC_SMBUS_BLOCK_DATA |	\
 					I2C_FUNC_SMBUS_I2C_BLOCK)
 
 /* Merrifield HCNT/LCNT/SDA hold time */
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0b42a12..886fb62 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -220,6 +220,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		I2C_FUNC_SMBUS_BYTE |
 		I2C_FUNC_SMBUS_BYTE_DATA |
 		I2C_FUNC_SMBUS_WORD_DATA |
+		I2C_FUNC_SMBUS_BLOCK_DATA |
 		I2C_FUNC_SMBUS_I2C_BLOCK;
 
 	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
-- 
1.7.1

^ permalink raw reply related

* Re: [RFC PATCH 3/5] gpio-dmec: gpio support for dmec
From: Zahari Doychev @ 2016-10-31  8:50 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-kernel@vger.kernel.org, Greg KH, Lee Jones, Wolfram Sang,
	Wim Van Sebroeck, Guenter Roeck, linux-i2c@vger.kernel.org,
	linux-gpio@vger.kernel.org, Alexandre Courbot, linux-watchdog
In-Reply-To: <CACRpkdZ+v0HGUCQsTAdeK0+ymi+wQsz8M51kRxCGN7Nwjwny1w@mail.gmail.com>

On Sat, Oct 29, 2016 at 11:05:29AM +0200, Linus Walleij wrote:
> On Thu, Oct 27, 2016 at 12:47 PM, Zahari Doychev
> <zahari.doychev@linux.com> wrote:
> 
> > This is support for the gpio functionality found on the Data Modul embedded
> > controllers
> >
> > Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
> > ---
> >  drivers/staging/dmec/Kconfig     |  10 +-
> >  drivers/staging/dmec/Makefile    |   1 +-
> >  drivers/staging/dmec/dmec.h      |   5 +-
> >  drivers/staging/dmec/gpio-dmec.c | 390 ++++++++++++++++++++++++++++++++-
> 

Thanks for the review.

> I guess Greg has already asked, but why is this in staging?
> The driver doesn't seem very complex or anything, it would not
> be overly troublesome to get it into the proper kernel subsystems.

I was unsure what was the right way to go with this. Next time I will move it
out of staging.

> 
> > +config GPIO_DMEC
> > +       tristate "Data Modul GPIO"
> > +       depends on MFD_DMEC && GPIOLIB
> 
> So the depends on GPIOLIB can go away if you just put it into
> drivers/gpio with the rest.
> 
> > +struct dmec_gpio_platform_data {
> > +       int gpio_base;
> 
> NAK, always use -1. No hardcoding the GPIO base other than on
> legacy systems.
> 
> > +       int chip_num;
> 
> I suspect you may not need this either. struct platform_device
> already contains a ->id field, just use that when instantiating
> your driver if you need an instance number.
> 
> So I think you need zero platform data for this.
> 
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/io.h>
> > +#include <linux/slab.h>
> > +#include <linux/errno.h>
> > +#include <linux/acpi.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/gpio.h>
> 
> You should only need <linux/gpio/driver.h>
> 
> > +#ifdef CONFIG_PM
> > +struct dmec_reg_ctx {
> > +       u32 dat;
> > +       u32 dir;
> > +       u32 imask;
> > +       u32 icfg[2];
> > +       u32 emask[2];
> > +};
> > +#endif
> > +
> > +struct dmec_gpio_priv {
> > +       struct regmap *regmap;
> > +       struct gpio_chip gpio_chip;
> > +       struct irq_chip irq_chip;
> > +       unsigned int chip_num;
> > +       unsigned int irq;
> > +       u8 ver;
> > +#ifdef CONFIG_PM
> > +       struct dmec_reg_ctx regs;
> > +#endif
> > +};
> 
> The #ifdef for saving the state is a bit kludgy. Can't you just have it there
> all the time? Or is this a footprint-sensitive system?

It will be no problem to have it always there.

> 
> > +static int dmec_gpio_get(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +       struct dmec_gpio_priv *priv = container_of(gc, struct dmec_gpio_priv,
> > +                                                  gpio_chip);
> 
> Use the new pattern with
> 
> struct dmec_gpio_priv *priv = gpiochip_get_data(gc);
> 
> This needs you to use devm_gpiochip_add_data() below.
> 
> > +static int dmec_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +       struct dmec_gpio_priv *priv = container_of(gc, struct dmec_gpio_priv,
> > +                                                  gpio_chip);
> > +       struct regmap *regmap = priv->regmap;
> > +       unsigned int offset, mask, val;
> > +
> > +       offset = DMEC_GPIO_IRQTYPE_OFFSET(priv) + (d->hwirq >> 2);
> > +       mask = ((d->hwirq & 3) << 1);
> > +
> > +       regmap_read(regmap, offset, &val);
> > +
> > +       val &= ~(3 << mask);
> > +       switch (type & IRQ_TYPE_SENSE_MASK) {
> > +       case IRQ_TYPE_LEVEL_LOW:
> > +               break;
> > +       case IRQ_TYPE_EDGE_RISING:
> > +               val |= (1 << mask);
> > +               break;
> > +       case IRQ_TYPE_EDGE_FALLING:
> > +               val |= (2 << mask);
> > +               break;
> > +       case IRQ_TYPE_EDGE_BOTH:
> > +               val |= (3 << mask);
> > +               break;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       regmap_write(regmap, offset, val);
> > +
> > +       return 0;
> > +}
> 
> This chip uses handle_simple_irq() which is fine if the chip really has no
> edge detector ACK register.
> 
> What some chips have is a special register to clearing (ACK:ing) the edge
> IRQ which makes it possible for a new IRQ to be handled as soon as
> that has happened, and those need to use handle_edge_irq() for edge IRQs
> and handle_level_irq() for level IRQs, with the side effect that the edge
> IRQ path will additionally call the .irq_ack() callback on the irqchip
> when handle_edge_irq() is used. In this case we set handle_bad_irq()
> as default handler and set up the right handler i .set_type().
> 
> Look at drivers/gpio/gpio-pl061.c for an example.
> 
> If you DON'T have a special edge ACK register, keep it like this.

What is the difference between this special edge ACK register and the normal
interrupt ACK register? I think I do not have such dedicated register
but I will have to check this again.

> 
> > +static irqreturn_t dmec_gpio_irq_handler(int irq, void *dev_id)
> > +{
> > +       struct dmec_gpio_priv *p = dev_id;
> > +       struct irq_domain *d = p->gpio_chip.irqdomain;
> > +       unsigned int irqs_handled = 0;
> > +       unsigned int val = 0, stat = 0;
> > +
> > +       regmap_read(p->regmap, DMEC_GPIO_IRQSTA_OFFSET(p), &val);
> > +       stat = val;
> > +       while (stat) {
> > +               int line = __ffs(stat);
> > +               int child_irq = irq_find_mapping(d, line);
> > +
> > +               handle_nested_irq(child_irq);
> > +               stat &= ~(BIT(line));
> > +               irqs_handled++;
> > +       }
> 
> I think you should re-read the status register in the loop. An IRQ may
> appear while you are reading.

The irqs are signalled over the SERIRQ of the LPC bus. Wnen irq comes in, it
should be acknowledged then the embedded controller can trigger the next one.
Nevertheless I will look at it and fix it if necessary.

> 
> > +static int dmec_gpio_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct dmec_gpio_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +       struct dmec_gpio_priv *priv;
> > +       struct gpio_chip *gpio_chip;
> > +       struct irq_chip *irq_chip;
> > +       int ret = 0;
> > +
> > +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       priv->regmap = dmec_get_regmap(pdev->dev.parent);
> 
> Do you really need a special accessor function to get the regmap like this?
> If you just use syscon you get these kind of helpers for free. I don't know
> if you can use syscon though, just a suggestion.

The I/O memory is mapped in the mfd driver. The addresing mode is also set
there which should be the same for all child devices. So in this way I have
dependcy between the mfd and the rest of the drivers. I am not sure that I
can use syscon as the driver is getting its resources from acpi.

> 
> > +       priv->chip_num = pdata->chip_num;
> 
> As mentioned, why not just use pdev->id
> 
> > +       gpio_chip = &priv->gpio_chip;
> > +       gpio_chip->label = "gpio-dmec";
> 
> You set the label
> 
> > +       gpio_chip->owner = THIS_MODULE;
> > +       gpio_chip->parent = dev;
> > +       gpio_chip->label = dev_name(dev);
> 
> You set the label again?
> 
> > +       gpio_chip->can_sleep = true;
> > +
> > +       gpio_chip->base = pdata->gpio_base;
> 
> NAK. Use -1
> 
> > +#ifdef CONFIG_PM
> > +static int dmec_gpio_suspend(struct platform_device *pdev, pm_message_t state)
> > +{
> > +       struct dmec_gpio_priv *p = platform_get_drvdata(pdev);
> > +       struct regmap *regmap = p->regmap;
> > +       struct dmec_reg_ctx *ctx = &p->regs;
> > +
> > +       regmap_read(regmap, DMEC_GPIO_BASE(p), &ctx->dat);
> > +       regmap_read(regmap, DMEC_GPIO_DIR_OFFSET(p), &ctx->dir);
> > +       regmap_read(regmap, DMEC_GPIO_IRQCFG_OFFSET(p), &ctx->imask);
> > +       regmap_read(regmap, DMEC_GPIO_IRQTYPE_OFFSET(p), &ctx->emask[0]);
> > +       regmap_read(regmap, DMEC_GPIO_IRQTYPE_OFFSET(p) + 1, &ctx->emask[1]);
> > +       regmap_read(regmap, DMEC_GPIO_IRQCFG_OFFSET(p), &ctx->icfg[0]);
> > +       regmap_read(regmap, DMEC_GPIO_IRQCFG_OFFSET(p) + 1, &ctx->icfg[1]);
> > +
> > +       devm_free_irq(&pdev->dev, p->irq, p);
> 
> That seems a bit violent.
> 
> > +static int dmec_gpio_resume(struct platform_device *pdev)
> > +{
> > +       struct dmec_gpio_priv *p = platform_get_drvdata(pdev);
> > +       struct regmap *regmap = p->regmap;
> > +       struct dmec_reg_ctx *ctx = &p->regs;
> > +       int ret;
> > +
> > +       regmap_write(regmap, DMEC_GPIO_BASE(p), ctx->dat);
> > +       regmap_write(regmap, DMEC_GPIO_DIR_OFFSET(p), ctx->dir);
> > +       regmap_write(regmap, DMEC_GPIO_IRQCFG_OFFSET(p), ctx->icfg[0]);
> > +       regmap_write(regmap, DMEC_GPIO_IRQCFG_OFFSET(p) + 1, ctx->icfg[1]);
> > +       regmap_write(regmap, DMEC_GPIO_IRQTYPE_OFFSET(p), ctx->emask[0]);
> > +       regmap_write(regmap, DMEC_GPIO_IRQTYPE_OFFSET(p) + 1, ctx->emask[1]);
> > +       regmap_write(regmap, DMEC_GPIO_IRQCFG_OFFSET(p), ctx->imask);
> > +       regmap_write(regmap, DMEC_GPIO_EVTSTA_OFFSET(p), 0xff);
> > +
> > +       ret = devm_request_threaded_irq(&pdev->dev, p->irq,
> > +                                       NULL, dmec_gpio_irq_handler,
> > +                                       IRQF_ONESHOT | IRQF_SHARED,
> > +                                       dev_name(&pdev->dev), p);
> > +       if (ret)
> > +               dev_err(&pdev->dev, "unable to get irq: %d\n", ret);
> 
> Re-requesting the IRQ for every suspend/resume cycle?
> 
> That's not right.
> 
> Look into the wakeup API. You need to tell the core whether this IRQ
> should be masked during suspend or not, the default is to mask it I think.
> 
> This is too big hammer to solve that.
> 
> > +static struct platform_driver dmec_gpio_driver = {
> > +       .driver = {
> > +               .name = "dmec-gpio",
> > +               .owner = THIS_MODULE,
> > +       },
> > +       .probe = dmec_gpio_probe,
> > +       .remove = dmec_gpio_remove,
> > +       .suspend = dmec_gpio_suspend,
> > +       .resume = dmec_gpio_resume,
> > +};
> 
> Don't use the legacy suspend/resume callbacks.
> 
> Use the DEV_PM_OPS directly in .pm in .driver above. It should
> work the same.
> 
> (You probably need to fix this on the other patches too.)
> 

Thanks I will change this.

Regards Zahari

> Yours,
> Linus Walleij

^ permalink raw reply

* RE: [v15, 6/7] base: soc: introduce soc_device_match() interface
From: Y.B. Lu @ 2016-10-31  9:28 UTC (permalink / raw)
  To: Arnd Bergmann,
	linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
  Cc: Mark Rutland, ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
	Geert Uytterhoeven,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, M.H. Lian,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Qiang Zhao,
	Russell King, Bhupesh Sharma, Claudiu Manoil,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Kumar Gala,
	Scott Wood, Rob Herring, Santosh Shilimkar,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Greg Kroah-Hartman,
	"linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-mmc>
In-Reply-To: <2572890.e6aV4hmMEL@wuerfel>

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd-r2nGTMty4D4@public.gmane.org]
> Sent: Friday, October 28, 2016 6:48 PM
> To: linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> Cc: Y.B. Lu; linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Scott
> Wood; Mark Rutland; Greg Kroah-Hartman; X.B. Xie; M.H. Lian; linux-
> i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Qiang Zhao; Russell King;
> Bhupesh Sharma; Joerg Roedel; Claudiu Manoil; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> Rob Herring; Santosh Shilimkar; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org;
> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Leo Li;
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Kumar Gala; Geert Uytterhoeven
> Subject: Re: [v15, 6/7] base: soc: introduce soc_device_match() interface
> 
> On Friday, October 28, 2016 2:50:17 PM CEST Yangbo Lu wrote:
> > +
> > +static int soc_device_match_one(struct device *dev, void *arg) {
> > +       struct soc_device *soc_dev = container_of(dev, struct
> soc_device, dev);
> > +       const struct soc_device_attribute *match = arg;
> > +
> > +       if (match->machine &&
> > +           !glob_match(match->machine, soc_dev->attr->machine))
> > +               return 0;
> > +
> > +       if (match->family &&
> > +           !glob_match(match->family, soc_dev->attr->family))
> > +               return 0;
> > +
> >
> 
> Geert found a bug in my code, and submitted a fix at
> https://patchwork.kernel.org/patch/9361395/
> 
> I think you should include that one in your series.
> 

[Lu Yangbo-B47093] Ok, no problem. Thanks :)

> 	Arnd

^ permalink raw reply

* RE: [v15, 3/7] powerpc/fsl: move mpc85xx.h to include/linux/fsl
From: Y.B. Lu @ 2016-10-31  9:35 UTC (permalink / raw)
  To: Arnd Bergmann,
	linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
  Cc: Mark Rutland, ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, M.H. Lian,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Qiang Zhao,
	Russell King, Bhupesh Sharma, Claudiu Manoil,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Kumar Gala,
	Scott Wood, Rob Herring, Santosh Shilimkar,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Greg Kroah-Hartman,
	linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, X.B. Xie
In-Reply-To: <2723366.1bJeJ7SKI6@wuerfel>

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd-r2nGTMty4D4@public.gmane.org]
> Sent: Friday, October 28, 2016 6:53 PM
> To: linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> Cc: Y.B. Lu; linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Scott
> Wood; Mark Rutland; Greg Kroah-Hartman; X.B. Xie; M.H. Lian; linux-
> i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Qiang Zhao; Russell King;
> Bhupesh Sharma; Joerg Roedel; Claudiu Manoil; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> Rob Herring; Santosh Shilimkar; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org;
> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Leo Li;
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Kumar Gala
> Subject: Re: [v15, 3/7] powerpc/fsl: move mpc85xx.h to include/linux/fsl
> 
> On Friday, October 28, 2016 2:50:14 PM CEST Yangbo Lu wrote:
> > Move mpc85xx.h to include/linux/fsl and rename it to svr.h as a common
> > header file.  This SVR numberspace is used on some ARM chips as well
> > as PPC, and even to check for a PPC SVR multi-arch drivers would
> > otherwise need to ifdef the header inclusion and all references to the
> SVR symbols.
> >
> >
> 
> I don't see any of the contents of this header referenced by the soc
> driver any more. I think you can just drop this patch.
> 

[Lu Yangbo-B47093] This header file was included by guts.c.
The guts driver used macro SVR_MAJ/SVR_MIN for calculation.

This header file was for powerpc arch before. And this patch is to made it as common header file for both ARM and PPC.
Sooner or later this is needed.

> 	Arnd

^ permalink raw reply

* RE: [v15, 0/7] Fix eSDHC host version register bug
From: Y.B. Lu @ 2016-10-31  9:36 UTC (permalink / raw)
  To: Arnd Bergmann,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
  Cc: Mark Rutland, ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, M.H. Lian,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Qiang Zhao,
	Russell King, Bhupesh Sharma, Jochen Friedrich, Claudiu Manoil,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Kumar Gala,
	Scott Wood, Rob Herring, Santosh Shilimkar, Greg Kroah-Hartman,
	linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, X.B. Xie,
	Leo Li, iommu
In-Reply-To: <3386858.dvuMhvkN3m@wuerfel>

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd-r2nGTMty4D4@public.gmane.org]
> Sent: Friday, October 28, 2016 6:54 PM
> To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Cc: Y.B. Lu; linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Scott
> Wood; Mark Rutland; Greg Kroah-Hartman; X.B. Xie; M.H. Lian; linux-
> i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Qiang Zhao; Russell King;
> Bhupesh Sharma; Joerg Roedel; Jochen Friedrich; Claudiu Manoil;
> devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Rob Herring; Santosh Shilimkar;
> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Leo Li;
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Kumar Gala; linuxppc-
> dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> Subject: Re: [v15, 0/7] Fix eSDHC host version register bug
> 
> On Friday, October 28, 2016 2:50:11 PM CEST Yangbo Lu wrote:
> > This patchset is used to fix a host version register bug in the
> > T4240-R1.0-R2.0 eSDHC controller. To match the SoC version and
> > revision, 10 previous version patchsets had tried many methods but all
> of them were rejected by reviewers.
> > Such as
> >         - dts compatible method
> >         - syscon method
> >         - ifdef PPC method
> >         - GUTS driver getting SVR method Anrd suggested a
> > soc_device_match method in v10, and this is the only available method
> > left now. This v11 patchset introduces the soc_device_match interface
> > in soc driver.
> >
> > The first five patches of Yangbo are to add the GUTS driver. This is
> > used to register a soc device which contain soc version and revision
> information.
> > The other two patches introduce the soc_device_match method in soc
> > driver and apply it on esdhc driver to fix this bug.
> >
> 
> Looks good overall. With patch 3 dropped (or an explanation why it's
> still needed), everything
> 
> Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>
 
[Lu Yangbo-B47093] Thank you very much:) See my explaination in patch 3 email.

> 	Arnd

^ permalink raw reply

* Re: [PATCH v3] i2c: designware: Implement support for SMBus block read and write
From: Mika Westerberg @ 2016-10-31 12:05 UTC (permalink / raw)
  To: tnhuynh
  Cc: Jarkko Nikula, Andy Shevchenko, Wolfram Sang, linux-i2c,
	linux-kernel, Loc Ho, Thang Nguyen, Phong Vo, patches
In-Reply-To: <1477896677-13085-1-git-send-email-tnhuynh@apm.com>

On Mon, Oct 31, 2016 at 01:51:17PM +0700, tnhuynh@apm.com wrote:
> From: Tin Huynh <tnhuynh@apm.com>
> 
> Free and Open IPMI use SMBUS BLOCK Read/Write to support SSIF protocol.
> However, I2C Designware Core Driver doesn't handle the case at the moment.
> The below patch supports this feature.
> 
> Signed-off-by: Tin Huynh <tnhuynh@apm.com>

Looks good to me,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

^ permalink raw reply

* Re: [RFC PATCH 3/5] gpio-dmec: gpio support for dmec
From: Linus Walleij @ 2016-10-31 12:26 UTC (permalink / raw)
  To: Zahari Doychev
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Greg KH,
	Lee Jones, Wolfram Sang, Wim Van Sebroeck, Guenter Roeck,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Alexandre Courbot, linux-watchdog-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161031085044.GC9050-S5msFFqROX52XvNHrDenQA@public.gmane.org>

On Mon, Oct 31, 2016 at 9:50 AM, Zahari Doychev
<zahari.doychev-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org> wrote:
> On Sat, Oct 29, 2016 at 11:05:29AM +0200, Linus Walleij wrote:

>> > +static int dmec_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>> > +{
>> > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> > +       struct dmec_gpio_priv *priv = container_of(gc, struct dmec_gpio_priv,
>> > +                                                  gpio_chip);
>> > +       struct regmap *regmap = priv->regmap;
>> > +       unsigned int offset, mask, val;
>> > +
>> > +       offset = DMEC_GPIO_IRQTYPE_OFFSET(priv) + (d->hwirq >> 2);
>> > +       mask = ((d->hwirq & 3) << 1);
>> > +
>> > +       regmap_read(regmap, offset, &val);
>> > +
>> > +       val &= ~(3 << mask);
>> > +       switch (type & IRQ_TYPE_SENSE_MASK) {
>> > +       case IRQ_TYPE_LEVEL_LOW:
>> > +               break;
>> > +       case IRQ_TYPE_EDGE_RISING:
>> > +               val |= (1 << mask);
>> > +               break;
>> > +       case IRQ_TYPE_EDGE_FALLING:
>> > +               val |= (2 << mask);
>> > +               break;
>> > +       case IRQ_TYPE_EDGE_BOTH:
>> > +               val |= (3 << mask);
>> > +               break;
>> > +       default:
>> > +               return -EINVAL;
>> > +       }
>> > +
>> > +       regmap_write(regmap, offset, val);
>> > +
>> > +       return 0;
>> > +}
>>
>> This chip uses handle_simple_irq() which is fine if the chip really has no
>> edge detector ACK register.
>>
>> What some chips have is a special register to clearing (ACK:ing) the edge
>> IRQ which makes it possible for a new IRQ to be handled as soon as
>> that has happened, and those need to use handle_edge_irq() for edge IRQs
>> and handle_level_irq() for level IRQs, with the side effect that the edge
>> IRQ path will additionally call the .irq_ack() callback on the irqchip
>> when handle_edge_irq() is used. In this case we set handle_bad_irq()
>> as default handler and set up the right handler i .set_type().
>>
>> Look at drivers/gpio/gpio-pl061.c for an example.
>>
>> If you DON'T have a special edge ACK register, keep it like this.
>
> What is the difference between this special edge ACK register and the normal
> interrupt ACK register?

With level interrupts there is seldom use of any ACK register.
They will by definition just hold the line low until the clients stop
asserting their IRQs.

With edge triggered interrupts, you can have a transient event so
that you need an ACK register to tell the hardware that you have
seen and acknowledged this IRQ, so it can go ahead and fire a
second IRQ on the same line while you are still processing the
first one.

> I think I do not have such dedicated register
> but I will have to check this again.

Read the documentation for the register(s) and see what the
use case is.

>> > +static int dmec_gpio_probe(struct platform_device *pdev)
>> > +{
>> > +       struct device *dev = &pdev->dev;
>> > +       struct dmec_gpio_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> > +       struct dmec_gpio_priv *priv;
>> > +       struct gpio_chip *gpio_chip;
>> > +       struct irq_chip *irq_chip;
>> > +       int ret = 0;
>> > +
>> > +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> > +       if (!priv)
>> > +               return -ENOMEM;
>> > +
>> > +       priv->regmap = dmec_get_regmap(pdev->dev.parent);
>>
>> Do you really need a special accessor function to get the regmap like this?
>> If you just use syscon you get these kind of helpers for free. I don't know
>> if you can use syscon though, just a suggestion.
>
> The I/O memory is mapped in the mfd driver. The addresing mode is also set
> there which should be the same for all child devices. So in this way I have
> dependcy between the mfd and the rest of the drivers. I am not sure that I
> can use syscon as the driver is getting its resources from acpi.

OK.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v3] i2c: designware: Implement support for SMBus block read and write
From: Jarkko Nikula @ 2016-10-31 13:16 UTC (permalink / raw)
  To: Mika Westerberg, tnhuynh
  Cc: Andy Shevchenko, Wolfram Sang, linux-i2c, linux-kernel, Loc Ho,
	Thang Nguyen, Phong Vo, patches
In-Reply-To: <20161031120509.GO23812@lahna.fi.intel.com>

On 10/31/2016 02:05 PM, Mika Westerberg wrote:
> On Mon, Oct 31, 2016 at 01:51:17PM +0700, tnhuynh@apm.com wrote:
>> From: Tin Huynh <tnhuynh@apm.com>
>>
>> Free and Open IPMI use SMBUS BLOCK Read/Write to support SSIF protocol.
>> However, I2C Designware Core Driver doesn't handle the case at the moment.
>> The below patch supports this feature.
>>
>> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
>
> Looks good to me,
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

^ permalink raw reply

* Re: [PATCHv6 08/11] i2c: match vendorless strings on the internal string length
From: Peter Rosin @ 2016-10-31 13:55 UTC (permalink / raw)
  To: Lee Jones, Kieran Bingham
  Cc: Wolfram Sang, linux-i2c, linux-kernel, Javier Martinez Canillas,
	sameo
In-Reply-To: <20161026085316.GK8574@dell>

On 2016-10-26 10:53, Lee Jones wrote:
> On Tue, 25 Oct 2016, Kieran Bingham wrote:
> 
>> If a user provides a shortened string to match a device to the sysfs i2c
>> interface it will match on the first string that contains that string
>> prefix.
>>
>> for example:
>>   echo a 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
>>
>> will match as3711, as3722, and ak8975 incorrectly.
>>
>> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
> 
> Acked-by: Lee Jones <lee.jones@linaro.org>
>   
>> ---
>>  drivers/i2c/i2c-core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 01bce56f733a..50c9cfdb87b7 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -1708,7 +1708,7 @@ i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
>>  		else
>>  			name++;
>>  
>> -		if (!strncasecmp(client->name, name, strlen(client->name)))
>> +		if (!strncasecmp(client->name, name, strlen(name)))
>>  			return matches;
>>  	}
>>  
> 

Is that really so much better?

With this patch
	echo as3711CRAP 0x68 > /sys/...
will match as3711.

What if there is some as37112 driver that is the real target?

Cheers,
Peter

^ permalink raw reply

* Re: [PATCHv6 08/11] i2c: match vendorless strings on the internal string length
From: Kieran Bingham @ 2016-10-31 15:01 UTC (permalink / raw)
  To: Peter Rosin, Lee Jones
  Cc: Wolfram Sang, linux-i2c, linux-kernel, Javier Martinez Canillas,
	sameo
In-Reply-To: <bc87437c-c53e-b6f6-e9b1-2d488639317b@axentia.se>

Hi Peter,

Thanks for your review

On 31/10/16 13:55, Peter Rosin wrote:
> On 2016-10-26 10:53, Lee Jones wrote:
>> On Tue, 25 Oct 2016, Kieran Bingham wrote:
>>
>>> If a user provides a shortened string to match a device to the sysfs i2c
>>> interface it will match on the first string that contains that string
>>> prefix.
>>>
>>> for example:
>>>   echo a 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
>>>
>>> will match as3711, as3722, and ak8975 incorrectly.
>>>
>>> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
>>
>> Acked-by: Lee Jones <lee.jones@linaro.org>
>>   
>>> ---
>>>  drivers/i2c/i2c-core.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>>> index 01bce56f733a..50c9cfdb87b7 100644
>>> --- a/drivers/i2c/i2c-core.c
>>> +++ b/drivers/i2c/i2c-core.c
>>> @@ -1708,7 +1708,7 @@ i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
>>>  		else
>>>  			name++;
>>>  
>>> -		if (!strncasecmp(client->name, name, strlen(client->name)))
>>> +		if (!strncasecmp(client->name, name, strlen(name)))
>>>  			return matches;
>>>  	}
>>>  
>>
> 
> Is that really so much better?

My original thought was that it verifies 'more' of the userspace input.
but...

> With this patch
> 	echo as3711CRAP 0x68 > /sys/...
> will match as3711.
>
> What if there is some as37112 driver that is the real target?

You're right - It looks like the only way to do this correctly is to
match the strncasecmp and the strlen of both strings.

So really we should be using sysfs_streq(). The only limitation there is
that this original code was performing a case-insensitive compare.

Lee - Where did the requirement for case insensitive matching come from
in your original code. Is it expected to be case-insensitive from the
I2C sysfs interface? or are dt-nodes expected to be case-sensitive?

Does anyone see reason that this shouldn't be using sysfs_streq()? or do
we need a sysfs_strcaseeq()...

> Cheers,
> Peter
> 

-- 
Regards

Kieran Bingham

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox