[PATCH net v3 5/5] drivers: net: cpsw: use of_phy_connect() in fixed-link case

[David Rivshin (Allworx)]

From: David Rivshin 

If a fixed-link DT subnode is used, the phy_device was looked up so
that a PHY ID string could be constructed and passed to phy_connect().
This is not necessary, as the device_node can be passed directly to
of_phy_connect() instead. This reuses the same codepath as if the
phy-handle DT property was used.

Signed-off-by: David Rivshin 
Tested-by: Nicolas Chauvet 
Tested-by: Andrew Goodbody 
Reviewed-by: Mugunthan V N 
Reviewed-by: Grygorii Strashko 
---

Changes since v2 [1]:
- Added Tested-by from Andrew Goodbody [3]
- Added Reviewed-by from Mugunthan V N [4]
- Added Reviewed-by from Grygorii Strashko [5]

Changes since v1 [2]:
- Rebased (trivial conflict, e5a03bfd modified the deleted snprintf)
- Added Tested-by from Nicolas Chauvet

[1] http://patchwork.ozlabs.org/patch/613276/
[2] http://patchwork.ozlabs.org/patch/560327/
[3] https://lkml.org/lkml/2016/4/22/537
[4] https://lkml.org/lkml/2016/4/22/63
[5] https://lkml.org/lkml/2016/4/22/529


 drivers/net/ethernet/ti/cpsw.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 712bc6d..e2fcdf1 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2044,30 +2044,21 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
"phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
if (slave_data->phy_node) {
dev_dbg(>dev,
"slave[%d] using phy-handle=\"%s\"\n",
i, slave_data->phy_node->full_name);
} else if (of_phy_is_fixed_link(slave_node)) {
-   struct device_node *phy_node;
-   struct phy_device *phy_dev;
-
/* In the case of a fixed PHY, the DT node associated
 * to the PHY is the Ethernet MAC DT node.
 */
ret = of_phy_register_fixed_link(slave_node);
if (ret)
return ret;
-   phy_node = of_node_get(slave_node);
-   phy_dev = of_phy_find_device(phy_node);
-   if (!phy_dev)
-   return -ENODEV;
-   snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
-PHY_ID_FMT, phy_dev->mdio.bus->id,
-phy_dev->mdio.addr);
+   slave_data->phy_node = of_node_get(slave_node);
} else if (parp) {
u32 phyid;
struct device_node *mdio_node;
struct platform_device *mdio;
 
if (lenp != (sizeof(__be32) * 2)) {
dev_err(>dev, "Invalid slave[%d] phy_id 
property\n", i);
-- 
2.5.5

[PATCH net v3 5/5] drivers: net: cpsw: use of_phy_connect() in fixed-link case

[David Rivshin (Allworx)]

From: David Rivshin 

If a fixed-link DT subnode is used, the phy_device was looked up so
that a PHY ID string could be constructed and passed to phy_connect().
This is not necessary, as the device_node can be passed directly to
of_phy_connect() instead. This reuses the same codepath as if the
phy-handle DT property was used.

Signed-off-by: David Rivshin 
Tested-by: Nicolas Chauvet 
Tested-by: Andrew Goodbody 
Reviewed-by: Mugunthan V N 
Reviewed-by: Grygorii Strashko 
---

Changes since v2 [1]:
- Added Tested-by from Andrew Goodbody [3]
- Added Reviewed-by from Mugunthan V N [4]
- Added Reviewed-by from Grygorii Strashko [5]

Changes since v1 [2]:
- Rebased (trivial conflict, e5a03bfd modified the deleted snprintf)
- Added Tested-by from Nicolas Chauvet

[1] http://patchwork.ozlabs.org/patch/613276/
[2] http://patchwork.ozlabs.org/patch/560327/
[3] https://lkml.org/lkml/2016/4/22/537
[4] https://lkml.org/lkml/2016/4/22/63
[5] https://lkml.org/lkml/2016/4/22/529


 drivers/net/ethernet/ti/cpsw.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 712bc6d..e2fcdf1 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2044,30 +2044,21 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
"phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
if (slave_data->phy_node) {
dev_dbg(>dev,
"slave[%d] using phy-handle=\"%s\"\n",
i, slave_data->phy_node->full_name);
} else if (of_phy_is_fixed_link(slave_node)) {
-   struct device_node *phy_node;
-   struct phy_device *phy_dev;
-
/* In the case of a fixed PHY, the DT node associated
 * to the PHY is the Ethernet MAC DT node.
 */
ret = of_phy_register_fixed_link(slave_node);
if (ret)
return ret;
-   phy_node = of_node_get(slave_node);
-   phy_dev = of_phy_find_device(phy_node);
-   if (!phy_dev)
-   return -ENODEV;
-   snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
-PHY_ID_FMT, phy_dev->mdio.bus->id,
-phy_dev->mdio.addr);
+   slave_data->phy_node = of_node_get(slave_node);
} else if (parp) {
u32 phyid;
struct device_node *mdio_node;
struct platform_device *mdio;
 
if (lenp != (sizeof(__be32) * 2)) {
dev_err(>dev, "Invalid slave[%d] phy_id 
property\n", i);
-- 
2.5.5

[PATCH net v3 4/5] dt: cpsw: phy-handle, phy_id, and fixed-link are mutually exclusive

[David Rivshin (Allworx)]

From: David Rivshin 

The phy-handle, phy_id, and fixed-link properties are mutually exclusive,
and only one need be specified. Make this clear in the binding doc.

Also mark the phy_id property as deprecated, as phy-handle should be
used instead.

Signed-off-by: David Rivshin 
---

Changes since v2 [1]:
- split from previous patch 2
- marked the phy_id property as deprecated [3]
- removed Rob Herring's Acked-by due to above change

Changes since v1 [2]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet
- Added Acked-by from Rob Herring for the binding change

[1] http://patchwork.ozlabs.org/patch/613260/
[2] http://patchwork.ozlabs.org/patch/560324/
[3] https://lkml.org/lkml/2016/4/22/494


 Documentation/devicetree/bindings/net/cpsw.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
b/Documentation/devicetree/bindings/net/cpsw.txt
index 28a4781..0ae0649 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -41,21 +41,21 @@ Optional properties:
 Slave Properties:
 Required properties:
 - phy-mode : See ethernet.txt file in the same directory
 
 Optional properties:
 - dual_emac_res_vlan   : Specifies VID to be used to segregate the ports
 - mac-address  : See ethernet.txt file in the same directory
-- phy_id   : Specifies slave phy id
+- phy_id   : Specifies slave phy id (deprecated, use phy-handle)
 - phy-handle   : See ethernet.txt file in the same directory
 
 Slave sub-nodes:
 - fixed-link   : See fixed-link.txt file in the same directory
- Either the property phy_id, or the sub-node
- fixed-link can be specified
+
+Note: Exactly one of phy_id, phy-handle, or fixed-link must be specified.
 
 Note: "ti,hwmods" field is used to fetch the base address and irq
 resources from TI, omap hwmod data base during device registration.
 Future plan is to migrate hwmod data base contents into device tree
 blob so that, all the required data will be used from device tree dts
 file.
 
-- 
2.5.5

[PATCH net v3 4/5] dt: cpsw: phy-handle, phy_id, and fixed-link are mutually exclusive

[David Rivshin (Allworx)]

From: David Rivshin 

The phy-handle, phy_id, and fixed-link properties are mutually exclusive,
and only one need be specified. Make this clear in the binding doc.

Also mark the phy_id property as deprecated, as phy-handle should be
used instead.

Signed-off-by: David Rivshin 
---

Changes since v2 [1]:
- split from previous patch 2
- marked the phy_id property as deprecated [3]
- removed Rob Herring's Acked-by due to above change

Changes since v1 [2]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet
- Added Acked-by from Rob Herring for the binding change

[1] http://patchwork.ozlabs.org/patch/613260/
[2] http://patchwork.ozlabs.org/patch/560324/
[3] https://lkml.org/lkml/2016/4/22/494


 Documentation/devicetree/bindings/net/cpsw.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
b/Documentation/devicetree/bindings/net/cpsw.txt
index 28a4781..0ae0649 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -41,21 +41,21 @@ Optional properties:
 Slave Properties:
 Required properties:
 - phy-mode : See ethernet.txt file in the same directory
 
 Optional properties:
 - dual_emac_res_vlan   : Specifies VID to be used to segregate the ports
 - mac-address  : See ethernet.txt file in the same directory
-- phy_id   : Specifies slave phy id
+- phy_id   : Specifies slave phy id (deprecated, use phy-handle)
 - phy-handle   : See ethernet.txt file in the same directory
 
 Slave sub-nodes:
 - fixed-link   : See fixed-link.txt file in the same directory
- Either the property phy_id, or the sub-node
- fixed-link can be specified
+
+Note: Exactly one of phy_id, phy-handle, or fixed-link must be specified.
 
 Note: "ti,hwmods" field is used to fetch the base address and irq
 resources from TI, omap hwmod data base during device registration.
 Future plan is to migrate hwmod data base contents into device tree
 blob so that, all the required data will be used from device tree dts
 file.
 
-- 
2.5.5

[PATCH net v3 3/5] drivers: net: cpsw: don't ignore phy-mode if phy-handle is used

[David Rivshin (Allworx)]

From: David Rivshin 

The phy-mode emac property was only being processed in the phy_id
or fixed-link cases. However if phy-handle was specified instead,
an error message would complain about the lack of phy_id or
fixed-link, and then jump past the of_get_phy_mode(). This would
result in the PHY mode defaulting to MII, regardless of what the
devicetree specified.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin 
Tested-by: Nicolas Chauvet 
Tested-by: Andrew Goodbody 
Reviewed-by: Mugunthan V N 
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.4.

Changes since v2 [1]:
- split from previous patch 2
- Added Tested-by from Andrew Goodbody [3]
- Added Reviewed-by from Mugunthan V N [4]
- rewrote commit log to focus on the functional bug fixed, rather
  than the bogus error message

Changes since v1 [2]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet
- Added Acked-by from Rob Herring for the binding change

[1] http://patchwork.ozlabs.org/patch/613260/
[2] http://patchwork.ozlabs.org/patch/560324/
[3] https://lkml.org/lkml/2016/4/22/537
[4] https://lkml.org/lkml/2016/4/22/63


 drivers/net/ethernet/ti/cpsw.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 5903448..712bc6d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2039,15 +2039,19 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
/* This is no slave child node, continue */
if (strcmp(slave_node->name, "slave"))
continue;
 
slave_data->phy_node = of_parse_phandle(slave_node,
"phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
-   if (of_phy_is_fixed_link(slave_node)) {
+   if (slave_data->phy_node) {
+   dev_dbg(>dev,
+   "slave[%d] using phy-handle=\"%s\"\n",
+   i, slave_data->phy_node->full_name);
+   } else if (of_phy_is_fixed_link(slave_node)) {
struct device_node *phy_node;
struct phy_device *phy_dev;
 
/* In the case of a fixed PHY, the DT node associated
 * to the PHY is the Ethernet MAC DT node.
 */
ret = of_phy_register_fixed_link(slave_node);
@@ -2076,15 +2080,17 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
if (!mdio) {
dev_err(>dev, "Missing mdio platform 
device\n");
return -EINVAL;
}
snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
 PHY_ID_FMT, mdio->name, phyid);
} else {
-   dev_err(>dev, "No slave[%d] phy_id or fixed-link 
property\n", i);
+   dev_err(>dev,
+   "No slave[%d] phy_id, phy-handle, or fixed-link 
property\n",
+   i);
goto no_phy_slave;
}
slave_data->phy_if = of_get_phy_mode(slave_node);
if (slave_data->phy_if < 0) {
dev_err(>dev, "Missing or malformed slave[%d] 
phy-mode property\n",
i);
return slave_data->phy_if;
-- 
2.5.5

[PATCH net v3 3/5] drivers: net: cpsw: don't ignore phy-mode if phy-handle is used

[David Rivshin (Allworx)]

From: David Rivshin 

The phy-mode emac property was only being processed in the phy_id
or fixed-link cases. However if phy-handle was specified instead,
an error message would complain about the lack of phy_id or
fixed-link, and then jump past the of_get_phy_mode(). This would
result in the PHY mode defaulting to MII, regardless of what the
devicetree specified.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin 
Tested-by: Nicolas Chauvet 
Tested-by: Andrew Goodbody 
Reviewed-by: Mugunthan V N 
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.4.

Changes since v2 [1]:
- split from previous patch 2
- Added Tested-by from Andrew Goodbody [3]
- Added Reviewed-by from Mugunthan V N [4]
- rewrote commit log to focus on the functional bug fixed, rather
  than the bogus error message

Changes since v1 [2]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet
- Added Acked-by from Rob Herring for the binding change

[1] http://patchwork.ozlabs.org/patch/613260/
[2] http://patchwork.ozlabs.org/patch/560324/
[3] https://lkml.org/lkml/2016/4/22/537
[4] https://lkml.org/lkml/2016/4/22/63


 drivers/net/ethernet/ti/cpsw.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 5903448..712bc6d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2039,15 +2039,19 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
/* This is no slave child node, continue */
if (strcmp(slave_node->name, "slave"))
continue;
 
slave_data->phy_node = of_parse_phandle(slave_node,
"phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
-   if (of_phy_is_fixed_link(slave_node)) {
+   if (slave_data->phy_node) {
+   dev_dbg(>dev,
+   "slave[%d] using phy-handle=\"%s\"\n",
+   i, slave_data->phy_node->full_name);
+   } else if (of_phy_is_fixed_link(slave_node)) {
struct device_node *phy_node;
struct phy_device *phy_dev;
 
/* In the case of a fixed PHY, the DT node associated
 * to the PHY is the Ethernet MAC DT node.
 */
ret = of_phy_register_fixed_link(slave_node);
@@ -2076,15 +2080,17 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
if (!mdio) {
dev_err(>dev, "Missing mdio platform 
device\n");
return -EINVAL;
}
snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
 PHY_ID_FMT, mdio->name, phyid);
} else {
-   dev_err(>dev, "No slave[%d] phy_id or fixed-link 
property\n", i);
+   dev_err(>dev,
+   "No slave[%d] phy_id, phy-handle, or fixed-link 
property\n",
+   i);
goto no_phy_slave;
}
slave_data->phy_if = of_get_phy_mode(slave_node);
if (slave_data->phy_if < 0) {
dev_err(>dev, "Missing or malformed slave[%d] 
phy-mode property\n",
i);
return slave_data->phy_if;
-- 
2.5.5

[PATCH net v3 2/5] drivers: net: cpsw: fix segfault in case of bad phy-handle

[David Rivshin (Allworx)]

From: David Rivshin 

If an emac node has a phy-handle property that points to something
which is not a phy, then a segmentation fault will occur when the
interface is brought up. This is because while phy_connect() will
return ERR_PTR() on failure, of_phy_connect() will return NULL.
The common error check uses IS_ERR(), and so missed when
of_phy_connect() fails. The NULL pointer is then dereferenced.

Also, the common error message referenced slave->data->phy_id,
which would be empty in the case of phy-handle. Instead, use the
name of the device_node as a useful identifier. And in the phy_id
case add the error code for completeness.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin 
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.5, although there is a trivial conflict in 4.4. I can produce a
separate patch against linux-4.4.y if preferred.

Changes since v2:
- new patch, although fixing part of previous patch 2 [1]

Changes since v1 [2]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet
- Added Acked-by from Rob Herring for the binding change

[1] http://patchwork.ozlabs.org/patch/613260/
[2] http://patchwork.ozlabs.org/patch/560324/


 drivers/net/ethernet/ti/cpsw.c | 37 +++--
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index ce0b0ca..5903448 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1143,33 +1143,42 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
struct cpsw_priv *priv)
 
if (priv->data.dual_emac)
cpsw_add_dual_emac_def_ale_entries(priv, slave, slave_port);
else
cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
   1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
 
-   if (slave->data->phy_node)
+   if (slave->data->phy_node) {
slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 _adjust_link, 0, slave->data->phy_if);
-   else
+   if (!slave->phy) {
+   dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
+   slave->data->phy_node->full_name,
+   slave->slave_num);
+   return;
+   }
+   } else {
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _adjust_link, slave->data->phy_if);
-   if (IS_ERR(slave->phy)) {
-   dev_err(priv->dev, "phy %s not found on slave %d\n",
-   slave->data->phy_id, slave->slave_num);
-   slave->phy = NULL;
-   } else {
-   phy_attached_info(slave->phy);
-
-   phy_start(slave->phy);
-
-   /* Configure GMII_SEL register */
-   cpsw_phy_sel(>pdev->dev, slave->phy->interface,
-slave->slave_num);
+   if (IS_ERR(slave->phy)) {
+   dev_err(priv->dev,
+   "phy \"%s\" not found on slave %d, err %ld\n",
+   slave->data->phy_id, slave->slave_num,
+   PTR_ERR(slave->phy));
+   slave->phy = NULL;
+   return;
+   }
}
+
+   phy_attached_info(slave->phy);
+
+   phy_start(slave->phy);
+
+   /* Configure GMII_SEL register */
+   cpsw_phy_sel(>pdev->dev, slave->phy->interface, slave->slave_num);
 }
 
 static inline void cpsw_add_default_vlan(struct cpsw_priv *priv)
 {
const int vlan = priv->data.default_vlan;
const int port = priv->host_port;
u32 reg;
-- 
2.5.5

[PATCH net v3 2/5] drivers: net: cpsw: fix segfault in case of bad phy-handle

[David Rivshin (Allworx)]

From: David Rivshin 

If an emac node has a phy-handle property that points to something
which is not a phy, then a segmentation fault will occur when the
interface is brought up. This is because while phy_connect() will
return ERR_PTR() on failure, of_phy_connect() will return NULL.
The common error check uses IS_ERR(), and so missed when
of_phy_connect() fails. The NULL pointer is then dereferenced.

Also, the common error message referenced slave->data->phy_id,
which would be empty in the case of phy-handle. Instead, use the
name of the device_node as a useful identifier. And in the phy_id
case add the error code for completeness.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin 
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.5, although there is a trivial conflict in 4.4. I can produce a
separate patch against linux-4.4.y if preferred.

Changes since v2:
- new patch, although fixing part of previous patch 2 [1]

Changes since v1 [2]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet
- Added Acked-by from Rob Herring for the binding change

[1] http://patchwork.ozlabs.org/patch/613260/
[2] http://patchwork.ozlabs.org/patch/560324/


 drivers/net/ethernet/ti/cpsw.c | 37 +++--
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index ce0b0ca..5903448 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1143,33 +1143,42 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
struct cpsw_priv *priv)
 
if (priv->data.dual_emac)
cpsw_add_dual_emac_def_ale_entries(priv, slave, slave_port);
else
cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
   1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
 
-   if (slave->data->phy_node)
+   if (slave->data->phy_node) {
slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 _adjust_link, 0, slave->data->phy_if);
-   else
+   if (!slave->phy) {
+   dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
+   slave->data->phy_node->full_name,
+   slave->slave_num);
+   return;
+   }
+   } else {
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _adjust_link, slave->data->phy_if);
-   if (IS_ERR(slave->phy)) {
-   dev_err(priv->dev, "phy %s not found on slave %d\n",
-   slave->data->phy_id, slave->slave_num);
-   slave->phy = NULL;
-   } else {
-   phy_attached_info(slave->phy);
-
-   phy_start(slave->phy);
-
-   /* Configure GMII_SEL register */
-   cpsw_phy_sel(>pdev->dev, slave->phy->interface,
-slave->slave_num);
+   if (IS_ERR(slave->phy)) {
+   dev_err(priv->dev,
+   "phy \"%s\" not found on slave %d, err %ld\n",
+   slave->data->phy_id, slave->slave_num,
+   PTR_ERR(slave->phy));
+   slave->phy = NULL;
+   return;
+   }
}
+
+   phy_attached_info(slave->phy);
+
+   phy_start(slave->phy);
+
+   /* Configure GMII_SEL register */
+   cpsw_phy_sel(>pdev->dev, slave->phy->interface, slave->slave_num);
 }
 
 static inline void cpsw_add_default_vlan(struct cpsw_priv *priv)
 {
const int vlan = priv->data.default_vlan;
const int port = priv->host_port;
u32 reg;
-- 
2.5.5

[PATCH net v3 1/5] drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac config

[David Rivshin (Allworx)]

From: David Rivshin 

Commit 9e42f715264ff158478fa30eaed847f6e131366b ("drivers: net: cpsw: add
phy-handle parsing") saved the "phy-handle" phandle into a new cpsw_priv
field. However, phy connections are per-slave, so the phy_node field should
be in cpsw_slave_data rather than cpsw_priv.

This would go unnoticed in a single emac configuration. But in dual_emac
mode, the last "phy-handle" property parsed for either slave would be used
by both of them, causing them both to refer to the same phy_device.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin 
Tested-by: Nicolas Chauvet 
Tested-by: Andrew Goodbody 
Reviewed-by: Mugunthan V N 
Reviewed-by: Grygorii Strashko 
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.4.

Changes since v2 [1]:
- Added Tested-by from Andrew Goodbody [3]
- Added Reviewed-by from Mugunthan V N [4]
- Added Reviewed-by from Grygorii Strashko [5]

Changes since v1 [2]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet

[1] http://patchwork.ozlabs.org/patch/613237/
[2] http://patchwork.ozlabs.org/patch/560326/
[3] https://lkml.org/lkml/2016/4/22/537
[4] https://lkml.org/lkml/2016/4/22/63
[5] https://lkml.org/lkml/2016/4/22/496


 drivers/net/ethernet/ti/cpsw.c | 13 ++---
 drivers/net/ethernet/ti/cpsw.h |  1 +
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index bbb77cd..ce0b0ca 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -363,15 +363,14 @@ static inline void slave_write(struct cpsw_slave *slave, 
u32 val, u32 offset)
__raw_writel(val, slave->regs + offset);
 }
 
 struct cpsw_priv {
spinlock_t  lock;
struct platform_device  *pdev;
struct net_device   *ndev;
-   struct device_node  *phy_node;
struct napi_struct  napi_rx;
struct napi_struct  napi_tx;
struct device   *dev;
struct cpsw_platform_data   data;
struct cpsw_ss_regs __iomem *regs;
struct cpsw_wr_regs __iomem *wr_regs;
u8 __iomem  *hw_stats;
@@ -1144,16 +1143,16 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
struct cpsw_priv *priv)
 
if (priv->data.dual_emac)
cpsw_add_dual_emac_def_ale_entries(priv, slave, slave_port);
else
cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
   1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
 
-   if (priv->phy_node)
-   slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
+   if (slave->data->phy_node)
+   slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 _adjust_link, 0, slave->data->phy_if);
else
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _adjust_link, slave->data->phy_if);
if (IS_ERR(slave->phy)) {
dev_err(priv->dev, "phy %s not found on slave %d\n",
slave->data->phy_id, slave->slave_num);
@@ -1936,20 +1935,19 @@ static void cpsw_slave_init(struct cpsw_slave *slave, 
struct cpsw_priv *priv,
 
slave->data = data;
slave->regs = regs + slave_reg_ofs;
slave->sliver   = regs + sliver_reg_ofs;
slave->port_vlan = data->dual_emac_res_vlan;
 }
 
-static int cpsw_probe_dt(struct cpsw_priv *priv,
+static int cpsw_probe_dt(struct cpsw_platform_data *data,
 struct platform_device *pdev)
 {
struct device_node *node = pdev->dev.of_node;
struct device_node *slave_node;
-   struct cpsw_platform_data *data = >data;
int i = 0, ret;
u32 prop;
 
if (!node)
return -EINVAL;
 
if (of_property_read_u32(node, "slaves", )) {
@@ -2029,15 +2027,16 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
int lenp;
const __be32 *parp;
 
/* This is no slave child node, continue */
if (strcmp(slave_node->name, "slave"))
continue;
 
-   priv->phy_node = of_parse_phandle(slave_node, "phy-handle", 0);
+   slave_data->phy_node = of_parse_phandle(slave_node,
+   "phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
if (of_phy_is_fixed_link(slave_node)) {
struct device_node *phy_node;
struct phy_device *phy_dev;
 
/* In the case of a fixed PHY, the DT node associated
 

[PATCH net v3 1/5] drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac config

[David Rivshin (Allworx)]

From: David Rivshin 

Commit 9e42f715264ff158478fa30eaed847f6e131366b ("drivers: net: cpsw: add
phy-handle parsing") saved the "phy-handle" phandle into a new cpsw_priv
field. However, phy connections are per-slave, so the phy_node field should
be in cpsw_slave_data rather than cpsw_priv.

This would go unnoticed in a single emac configuration. But in dual_emac
mode, the last "phy-handle" property parsed for either slave would be used
by both of them, causing them both to refer to the same phy_device.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin 
Tested-by: Nicolas Chauvet 
Tested-by: Andrew Goodbody 
Reviewed-by: Mugunthan V N 
Reviewed-by: Grygorii Strashko 
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.4.

Changes since v2 [1]:
- Added Tested-by from Andrew Goodbody [3]
- Added Reviewed-by from Mugunthan V N [4]
- Added Reviewed-by from Grygorii Strashko [5]

Changes since v1 [2]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet

[1] http://patchwork.ozlabs.org/patch/613237/
[2] http://patchwork.ozlabs.org/patch/560326/
[3] https://lkml.org/lkml/2016/4/22/537
[4] https://lkml.org/lkml/2016/4/22/63
[5] https://lkml.org/lkml/2016/4/22/496


 drivers/net/ethernet/ti/cpsw.c | 13 ++---
 drivers/net/ethernet/ti/cpsw.h |  1 +
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index bbb77cd..ce0b0ca 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -363,15 +363,14 @@ static inline void slave_write(struct cpsw_slave *slave, 
u32 val, u32 offset)
__raw_writel(val, slave->regs + offset);
 }
 
 struct cpsw_priv {
spinlock_t  lock;
struct platform_device  *pdev;
struct net_device   *ndev;
-   struct device_node  *phy_node;
struct napi_struct  napi_rx;
struct napi_struct  napi_tx;
struct device   *dev;
struct cpsw_platform_data   data;
struct cpsw_ss_regs __iomem *regs;
struct cpsw_wr_regs __iomem *wr_regs;
u8 __iomem  *hw_stats;
@@ -1144,16 +1143,16 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
struct cpsw_priv *priv)
 
if (priv->data.dual_emac)
cpsw_add_dual_emac_def_ale_entries(priv, slave, slave_port);
else
cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
   1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
 
-   if (priv->phy_node)
-   slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
+   if (slave->data->phy_node)
+   slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 _adjust_link, 0, slave->data->phy_if);
else
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _adjust_link, slave->data->phy_if);
if (IS_ERR(slave->phy)) {
dev_err(priv->dev, "phy %s not found on slave %d\n",
slave->data->phy_id, slave->slave_num);
@@ -1936,20 +1935,19 @@ static void cpsw_slave_init(struct cpsw_slave *slave, 
struct cpsw_priv *priv,
 
slave->data = data;
slave->regs = regs + slave_reg_ofs;
slave->sliver   = regs + sliver_reg_ofs;
slave->port_vlan = data->dual_emac_res_vlan;
 }
 
-static int cpsw_probe_dt(struct cpsw_priv *priv,
+static int cpsw_probe_dt(struct cpsw_platform_data *data,
 struct platform_device *pdev)
 {
struct device_node *node = pdev->dev.of_node;
struct device_node *slave_node;
-   struct cpsw_platform_data *data = >data;
int i = 0, ret;
u32 prop;
 
if (!node)
return -EINVAL;
 
if (of_property_read_u32(node, "slaves", )) {
@@ -2029,15 +2027,16 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
int lenp;
const __be32 *parp;
 
/* This is no slave child node, continue */
if (strcmp(slave_node->name, "slave"))
continue;
 
-   priv->phy_node = of_parse_phandle(slave_node, "phy-handle", 0);
+   slave_data->phy_node = of_parse_phandle(slave_node,
+   "phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
if (of_phy_is_fixed_link(slave_node)) {
struct device_node *phy_node;
struct phy_device *phy_dev;
 
/* In the case of a fixed PHY, the DT node associated
 * to the PHY is the Ethernet MAC DT node.
@@ -2271,15 +2270,15 @@ static int cpsw_probe(struct platform_device *pdev)
 * This 

[PATCH net v3 0/5] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

From: David Rivshin 

This series fixes a number of related issues around using phy-handle
properties in cpsw emac nodes.

Patch 1 fixes a bug if more than one slave is used, and either
slave uses the phy-handle property in the devicetree.

Patch 2 fixes a NULL pointer dereference which can occur if a
phy-handle property is used and of_phy_connect() return NULL,
such as with a bad devicetree.

Patch 3 fixes an issue where the phy-mode property would be ignored
if a phy-handle property was used. This also fixes a bogus error
message that would be emitted.

Patch 4 fixes makes the binding documentation more explicit that
exactly one PHY property should be used, and also marks phy_id as
deprecated.

Patch 5 cleans up the fixed-link case to work like the now-fixed
phy-handle case.

I have tested on the following hardware configurations:
 - (EVMSK) dual emac, phy_id property in both slaves
 - (EVMSK) dual emac, phy-handle property in both slaves
 - (EVMSK) a bad phy-handle property pointing to 
 - (EVMSK) phy_id property with incorrect PHY address
 - (BeagleBoneBlack) single emac, phy_id property
 - (custom) single emac, fixed-link subnode

Andrew Goodbody reported testing v2 on a board that doesn't use
dual_emac mode, but with 2 PHYs using phy-handle properties [1].

Nicolas Chauvet reported testing v2 on an HP t410 (dm8148).

Markus Brunner reported testing v1 on the following [2]:
 - emac0 with phy_id and emac1 with fixed phy
 - emac0 with phy-handle and emac1 with fixed phy
 - emac0 with fixed phy and emac1 with fixed phy

[1] https://lkml.org/lkml/2016/4/22/537
[2] http://www.spinics.net/lists/netdev/msg357890.html

David Rivshin (5):
  drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac
config
  drivers: net: cpsw: fix segfault in case of bad phy-handle
  drivers: net: cpsw: don't ignore phy-mode if phy-handle is used
  dt: cpsw: phy-handle, phy_id, and fixed-link are mutually exclusive
  drivers: net: cpsw: use of_phy_connect() in fixed-link case

 Documentation/devicetree/bindings/net/cpsw.txt |  6 +--
 drivers/net/ethernet/ti/cpsw.c | 69 ++
 drivers/net/ethernet/ti/cpsw.h |  1 +
 3 files changed, 41 insertions(+), 35 deletions(-)

-- 
2.5.5

[PATCH net v3 0/5] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

From: David Rivshin 

This series fixes a number of related issues around using phy-handle
properties in cpsw emac nodes.

Patch 1 fixes a bug if more than one slave is used, and either
slave uses the phy-handle property in the devicetree.

Patch 2 fixes a NULL pointer dereference which can occur if a
phy-handle property is used and of_phy_connect() return NULL,
such as with a bad devicetree.

Patch 3 fixes an issue where the phy-mode property would be ignored
if a phy-handle property was used. This also fixes a bogus error
message that would be emitted.

Patch 4 fixes makes the binding documentation more explicit that
exactly one PHY property should be used, and also marks phy_id as
deprecated.

Patch 5 cleans up the fixed-link case to work like the now-fixed
phy-handle case.

I have tested on the following hardware configurations:
 - (EVMSK) dual emac, phy_id property in both slaves
 - (EVMSK) dual emac, phy-handle property in both slaves
 - (EVMSK) a bad phy-handle property pointing to 
 - (EVMSK) phy_id property with incorrect PHY address
 - (BeagleBoneBlack) single emac, phy_id property
 - (custom) single emac, fixed-link subnode

Andrew Goodbody reported testing v2 on a board that doesn't use
dual_emac mode, but with 2 PHYs using phy-handle properties [1].

Nicolas Chauvet reported testing v2 on an HP t410 (dm8148).

Markus Brunner reported testing v1 on the following [2]:
 - emac0 with phy_id and emac1 with fixed phy
 - emac0 with phy-handle and emac1 with fixed phy
 - emac0 with fixed phy and emac1 with fixed phy

[1] https://lkml.org/lkml/2016/4/22/537
[2] http://www.spinics.net/lists/netdev/msg357890.html

David Rivshin (5):
  drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac
config
  drivers: net: cpsw: fix segfault in case of bad phy-handle
  drivers: net: cpsw: don't ignore phy-mode if phy-handle is used
  dt: cpsw: phy-handle, phy_id, and fixed-link are mutually exclusive
  drivers: net: cpsw: use of_phy_connect() in fixed-link case

 Documentation/devicetree/bindings/net/cpsw.txt |  6 +--
 drivers/net/ethernet/ti/cpsw.c | 69 ++
 drivers/net/ethernet/ti/cpsw.h |  1 +
 3 files changed, 41 insertions(+), 35 deletions(-)

-- 
2.5.5

Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property

[David Rivshin (Allworx)]

On Mon, 25 Apr 2016 22:12:20 +0300
Grygorii Strashko <grygorii.stras...@ti.com> wrote:

> On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> > On Fri, 22 Apr 2016 16:03:34 +0300
> > Grygorii Strashko <grygorii.stras...@ti.com> wrote:
> >   
> >> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:  
> >>> From: David Rivshin <drivs...@allworx.com>
> >>>
> >>> The phy-handle, phy_id, and fixed-link properties are mutually exclusive,
> >>> and only one need be specified. However if phy-handle was specified, an
> >>> error message would complain about the lack of phy_id or fixed-link.  
> 
> I think, commit message need to be updated.
> You not only fix log messages - you also fix the issue with 
> of_get_phy_mode(slave_node); which will not be called if phy-handle is used.

You are correct, and that is probably the more important fix compared
to the error messages.

Because the content is becoming less coherent, what I may do is split 
this patch into 3 small patches:
A) devicetree binding documentation changes
B) cpsw_probe_dt changes, with the fixes for of_get_phy_mode() and
   related error message
C) cpsw_slave_open changes, with the fixes for crash if of_phy_connect
   returns NULL, and related error message 

Does that sound reasonable?

>  
> 
> slave_data->phy_if = of_get_phy_mode(slave_node); 
> ^ see below
> >>>
> >>> Also, if phy-handle was specified and the subsequent of_phy_connect()
> >>> failed, the error message still referenced slaved->data->phy_id, which
> >>> would be empty. Instead, use the name of the device_node as a useful
> >>> identifier.
> >>>
> >>> Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
> >>> Signed-off-by: David Rivshin <drivs...@allworx.com>
> >>> Acked-by: Rob Herring <r...@kernel.org>
> >>> Tested-by: Nicolas Chauvet <kwiz...@gmail.com>
> >>> ---
> >>> If would like this for -stable it should apply cleanly as far back
> >>> as 4.5. It failes on 4.4 due to some context differences, but can be
> >>> applied with 'git am -C2'. Or, I can produce a separate patch against
> >>> linux-4.4.y if preferred.
> >>>
> >>> Changes since v1 [1]:
> >>> - Rebased (no conflicts)
> >>> - Added Tested-by from Nicolas Chauvet
> >>> - Added Acked-by from Rob Herring for the binding change
> >>>
> >>> [1] https://patchwork.ozlabs.org/patch/560324/
> >>>
> >>>Documentation/devicetree/bindings/net/cpsw.txt |  4 ++--
> >>>drivers/net/ethernet/ti/cpsw.c | 17 +
> >>>2 files changed, 15 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
> >>> b/Documentation/devicetree/bindings/net/cpsw.txt
> >>> index 28a4781..3033c0f 100644
> >>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> >>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> >>> @@ -46,16 +46,16 @@ Optional properties:
> >>>- dual_emac_res_vlan   : Specifies VID to be used to segregate the 
> >>> ports
> >>>- mac-address  : See ethernet.txt file in the same directory
> >>>- phy_id   : Specifies slave phy id  
> >>
> >> May be the "phy_id" can be marked as deprecated? (while here)
> >> The recommended property now is "phy-handle".  
> > 
> > I can certainly do that. Perhaps something like this?
> >   - phy_id  : Specifies slave phy id (deprecated, use phy-handle)
> > 
> > Rob, would you have any issues with bundling that?
> >   
> >>  
> >>>- phy-handle   : See ethernet.txt file in the same directory
> >>>
> >>>Slave sub-nodes:
> >>>- fixed-link   : See fixed-link.txt file in the same directory
> >>> -   Either the property phy_id, or the sub-node
> >>> -   fixed-link can be specified
> >>> +
> >>> +Note: Exactly one of phy_id, phy-handle, or fixed-link must be specified.
> >>>
> >>>Note: "ti,hwmods" field is used to fetch the base address and irq
> >>>resources from TI, omap hwmod data base during device registration.
> >>>Future plan is to migrate hwmod data base contents into device tree
> >>>blob so that, all the required dat

Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property

[David Rivshin (Allworx)]

On Mon, 25 Apr 2016 22:12:20 +0300
Grygorii Strashko  wrote:

> On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> > On Fri, 22 Apr 2016 16:03:34 +0300
> > Grygorii Strashko  wrote:
> >   
> >> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:  
> >>> From: David Rivshin 
> >>>
> >>> The phy-handle, phy_id, and fixed-link properties are mutually exclusive,
> >>> and only one need be specified. However if phy-handle was specified, an
> >>> error message would complain about the lack of phy_id or fixed-link.  
> 
> I think, commit message need to be updated.
> You not only fix log messages - you also fix the issue with 
> of_get_phy_mode(slave_node); which will not be called if phy-handle is used.

You are correct, and that is probably the more important fix compared
to the error messages.

Because the content is becoming less coherent, what I may do is split 
this patch into 3 small patches:
A) devicetree binding documentation changes
B) cpsw_probe_dt changes, with the fixes for of_get_phy_mode() and
   related error message
C) cpsw_slave_open changes, with the fixes for crash if of_phy_connect
   returns NULL, and related error message 

Does that sound reasonable?

>  
> 
> slave_data->phy_if = of_get_phy_mode(slave_node); 
> ^ see below
> >>>
> >>> Also, if phy-handle was specified and the subsequent of_phy_connect()
> >>> failed, the error message still referenced slaved->data->phy_id, which
> >>> would be empty. Instead, use the name of the device_node as a useful
> >>> identifier.
> >>>
> >>> Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
> >>> Signed-off-by: David Rivshin 
> >>> Acked-by: Rob Herring 
> >>> Tested-by: Nicolas Chauvet 
> >>> ---
> >>> If would like this for -stable it should apply cleanly as far back
> >>> as 4.5. It failes on 4.4 due to some context differences, but can be
> >>> applied with 'git am -C2'. Or, I can produce a separate patch against
> >>> linux-4.4.y if preferred.
> >>>
> >>> Changes since v1 [1]:
> >>> - Rebased (no conflicts)
> >>> - Added Tested-by from Nicolas Chauvet
> >>> - Added Acked-by from Rob Herring for the binding change
> >>>
> >>> [1] https://patchwork.ozlabs.org/patch/560324/
> >>>
> >>>Documentation/devicetree/bindings/net/cpsw.txt |  4 ++--
> >>>drivers/net/ethernet/ti/cpsw.c | 17 +
> >>>2 files changed, 15 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
> >>> b/Documentation/devicetree/bindings/net/cpsw.txt
> >>> index 28a4781..3033c0f 100644
> >>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> >>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> >>> @@ -46,16 +46,16 @@ Optional properties:
> >>>- dual_emac_res_vlan   : Specifies VID to be used to segregate the 
> >>> ports
> >>>- mac-address  : See ethernet.txt file in the same directory
> >>>- phy_id   : Specifies slave phy id  
> >>
> >> May be the "phy_id" can be marked as deprecated? (while here)
> >> The recommended property now is "phy-handle".  
> > 
> > I can certainly do that. Perhaps something like this?
> >   - phy_id  : Specifies slave phy id (deprecated, use phy-handle)
> > 
> > Rob, would you have any issues with bundling that?
> >   
> >>  
> >>>- phy-handle   : See ethernet.txt file in the same directory
> >>>
> >>>Slave sub-nodes:
> >>>- fixed-link   : See fixed-link.txt file in the same directory
> >>> -   Either the property phy_id, or the sub-node
> >>> -   fixed-link can be specified
> >>> +
> >>> +Note: Exactly one of phy_id, phy-handle, or fixed-link must be specified.
> >>>
> >>>Note: "ti,hwmods" field is used to fetch the base address and irq
> >>>resources from TI, omap hwmod data base during device registration.
> >>>Future plan is to migrate hwmod data base contents into device tree
> >>>blob so that, all the required data will be used from device tree dts
> >>>file.
> >>>
> >>> diff --git a/drivers/net/ethernet/ti/cpsw.c 
> >>> b/d

Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property

[David Rivshin (Allworx)]

On Fri, 22 Apr 2016 16:03:34 +0300
Grygorii Strashko <grygorii.stras...@ti.com> wrote:

> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> > From: David Rivshin <drivs...@allworx.com>
> > 
> > The phy-handle, phy_id, and fixed-link properties are mutually exclusive,
> > and only one need be specified. However if phy-handle was specified, an
> > error message would complain about the lack of phy_id or fixed-link.
> > 
> > Also, if phy-handle was specified and the subsequent of_phy_connect()
> > failed, the error message still referenced slaved->data->phy_id, which
> > would be empty. Instead, use the name of the device_node as a useful
> > identifier.
> > 
> > Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
> > Signed-off-by: David Rivshin <drivs...@allworx.com>
> > Acked-by: Rob Herring <r...@kernel.org>
> > Tested-by: Nicolas Chauvet <kwiz...@gmail.com>
> > ---
> > If would like this for -stable it should apply cleanly as far back
> > as 4.5. It failes on 4.4 due to some context differences, but can be
> > applied with 'git am -C2'. Or, I can produce a separate patch against
> > linux-4.4.y if preferred.
> > 
> > Changes since v1 [1]:
> > - Rebased (no conflicts)
> > - Added Tested-by from Nicolas Chauvet
> > - Added Acked-by from Rob Herring for the binding change
> > 
> > [1] https://patchwork.ozlabs.org/patch/560324/
> > 
> >   Documentation/devicetree/bindings/net/cpsw.txt |  4 ++--
> >   drivers/net/ethernet/ti/cpsw.c | 17 +
> >   2 files changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
> > b/Documentation/devicetree/bindings/net/cpsw.txt
> > index 28a4781..3033c0f 100644
> > --- a/Documentation/devicetree/bindings/net/cpsw.txt
> > +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> > @@ -46,16 +46,16 @@ Optional properties:
> >   - dual_emac_res_vlan  : Specifies VID to be used to segregate the 
> > ports
> >   - mac-address : See ethernet.txt file in the same directory
> >   - phy_id  : Specifies slave phy id  
> 
> May be the "phy_id" can be marked as deprecated? (while here)
> The recommended property now is "phy-handle".

I can certainly do that. Perhaps something like this?
 - phy_id   : Specifies slave phy id (deprecated, use phy-handle)

Rob, would you have any issues with bundling that?

> 
> >   - phy-handle  : See ethernet.txt file in the same directory
> >   
> >   Slave sub-nodes:
> >   - fixed-link  : See fixed-link.txt file in the same directory
> > - Either the property phy_id, or the sub-node
> > - fixed-link can be specified
> > +
> > +Note: Exactly one of phy_id, phy-handle, or fixed-link must be specified.
> >   
> >   Note: "ti,hwmods" field is used to fetch the base address and irq
> >   resources from TI, omap hwmod data base during device registration.
> >   Future plan is to migrate hwmod data base contents into device tree
> >   blob so that, all the required data will be used from device tree dts
> >   file.
> >   
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index d69cb3f..3c81413 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -1150,16 +1150,19 @@ static void cpsw_slave_open(struct cpsw_slave 
> > *slave, struct cpsw_priv *priv)
> > if (slave->data->phy_node)
> > slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
> >  _adjust_link, 0, slave->data->phy_if);
> > else
> > slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> >  _adjust_link, slave->data->phy_if);
> > if (IS_ERR(slave->phy)) {
> > -   dev_err(priv->dev, "phy %s not found on slave %d\n",
> > -   slave->data->phy_id, slave->slave_num);
> > +   dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
> > +   slave->data->phy_node ?
> > +   slave->data->phy_node->full_name :
> > +   slave->data->phy_id,
> > +   slave->slave_num);  
> 
> Unfortunately,  there are some inconsistency between legac

Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property

[David Rivshin (Allworx)]

On Fri, 22 Apr 2016 16:03:34 +0300
Grygorii Strashko  wrote:

> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> > From: David Rivshin 
> > 
> > The phy-handle, phy_id, and fixed-link properties are mutually exclusive,
> > and only one need be specified. However if phy-handle was specified, an
> > error message would complain about the lack of phy_id or fixed-link.
> > 
> > Also, if phy-handle was specified and the subsequent of_phy_connect()
> > failed, the error message still referenced slaved->data->phy_id, which
> > would be empty. Instead, use the name of the device_node as a useful
> > identifier.
> > 
> > Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
> > Signed-off-by: David Rivshin 
> > Acked-by: Rob Herring 
> > Tested-by: Nicolas Chauvet 
> > ---
> > If would like this for -stable it should apply cleanly as far back
> > as 4.5. It failes on 4.4 due to some context differences, but can be
> > applied with 'git am -C2'. Or, I can produce a separate patch against
> > linux-4.4.y if preferred.
> > 
> > Changes since v1 [1]:
> > - Rebased (no conflicts)
> > - Added Tested-by from Nicolas Chauvet
> > - Added Acked-by from Rob Herring for the binding change
> > 
> > [1] https://patchwork.ozlabs.org/patch/560324/
> > 
> >   Documentation/devicetree/bindings/net/cpsw.txt |  4 ++--
> >   drivers/net/ethernet/ti/cpsw.c | 17 +
> >   2 files changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
> > b/Documentation/devicetree/bindings/net/cpsw.txt
> > index 28a4781..3033c0f 100644
> > --- a/Documentation/devicetree/bindings/net/cpsw.txt
> > +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> > @@ -46,16 +46,16 @@ Optional properties:
> >   - dual_emac_res_vlan  : Specifies VID to be used to segregate the 
> > ports
> >   - mac-address : See ethernet.txt file in the same directory
> >   - phy_id  : Specifies slave phy id  
> 
> May be the "phy_id" can be marked as deprecated? (while here)
> The recommended property now is "phy-handle".

I can certainly do that. Perhaps something like this?
 - phy_id   : Specifies slave phy id (deprecated, use phy-handle)

Rob, would you have any issues with bundling that?

> 
> >   - phy-handle  : See ethernet.txt file in the same directory
> >   
> >   Slave sub-nodes:
> >   - fixed-link  : See fixed-link.txt file in the same directory
> > - Either the property phy_id, or the sub-node
> > - fixed-link can be specified
> > +
> > +Note: Exactly one of phy_id, phy-handle, or fixed-link must be specified.
> >   
> >   Note: "ti,hwmods" field is used to fetch the base address and irq
> >   resources from TI, omap hwmod data base during device registration.
> >   Future plan is to migrate hwmod data base contents into device tree
> >   blob so that, all the required data will be used from device tree dts
> >   file.
> >   
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index d69cb3f..3c81413 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -1150,16 +1150,19 @@ static void cpsw_slave_open(struct cpsw_slave 
> > *slave, struct cpsw_priv *priv)
> > if (slave->data->phy_node)
> > slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
> >  _adjust_link, 0, slave->data->phy_if);
> > else
> > slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> >  _adjust_link, slave->data->phy_if);
> > if (IS_ERR(slave->phy)) {
> > -   dev_err(priv->dev, "phy %s not found on slave %d\n",
> > -   slave->data->phy_id, slave->slave_num);
> > +   dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
> > +   slave->data->phy_node ?
> > +   slave->data->phy_node->full_name :
> > +   slave->data->phy_id,
> > +   slave->slave_num);  
> 
> Unfortunately,  there are some inconsistency between legacy and FDT API :(
> of_phy_connect() will return valid phy_device or NULL, but phy_connect()
> can return valid phy_device or ERR_PTR().

Good catch, I hadn't notice

[PATCH net v2 3/3] drivers: net: cpsw: use of_phy_connect() in fixed-link case

[David Rivshin (Allworx)]

From: David Rivshin 

If a fixed-link DT subnode is used, the phy_device was looked up so
that a PHY ID string could be constructed and passed to phy_connect().
This is not necessary, as the device_node can be passed directly to
of_phy_connect() instead. This reuses the same codepath as if the
phy-handle DT property was used.

Signed-off-by: David Rivshin 
Tested-by: Nicolas Chauvet 
---

Changes since v1 [1]:
- Rebased (trivial conflict, e5a03bfd modified the deleted snprintf)
- Added Tested-by from Nicolas Chauvet

[1] https://patchwork.ozlabs.org/patch/560327/


 drivers/net/ethernet/ti/cpsw.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 3c81413..245f919 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2038,30 +2038,21 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
"phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
if (slave_data->phy_node) {
dev_dbg(>dev,
"slave[%d] using phy-handle=\"%s\"\n",
i, slave_data->phy_node->full_name);
} else if (of_phy_is_fixed_link(slave_node)) {
-   struct device_node *phy_node;
-   struct phy_device *phy_dev;
-
/* In the case of a fixed PHY, the DT node associated
 * to the PHY is the Ethernet MAC DT node.
 */
ret = of_phy_register_fixed_link(slave_node);
if (ret)
return ret;
-   phy_node = of_node_get(slave_node);
-   phy_dev = of_phy_find_device(phy_node);
-   if (!phy_dev)
-   return -ENODEV;
-   snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
-PHY_ID_FMT, phy_dev->mdio.bus->id,
-phy_dev->mdio.addr);
+   slave_data->phy_node = of_node_get(slave_node);
} else if (parp) {
u32 phyid;
struct device_node *mdio_node;
struct platform_device *mdio;
 
if (lenp != (sizeof(__be32) * 2)) {
dev_err(>dev, "Invalid slave[%d] phy_id 
property\n", i);
-- 
2.5.5

[PATCH net v2 3/3] drivers: net: cpsw: use of_phy_connect() in fixed-link case

[David Rivshin (Allworx)]

From: David Rivshin 

If a fixed-link DT subnode is used, the phy_device was looked up so
that a PHY ID string could be constructed and passed to phy_connect().
This is not necessary, as the device_node can be passed directly to
of_phy_connect() instead. This reuses the same codepath as if the
phy-handle DT property was used.

Signed-off-by: David Rivshin 
Tested-by: Nicolas Chauvet 
---

Changes since v1 [1]:
- Rebased (trivial conflict, e5a03bfd modified the deleted snprintf)
- Added Tested-by from Nicolas Chauvet

[1] https://patchwork.ozlabs.org/patch/560327/


 drivers/net/ethernet/ti/cpsw.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 3c81413..245f919 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2038,30 +2038,21 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
"phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
if (slave_data->phy_node) {
dev_dbg(>dev,
"slave[%d] using phy-handle=\"%s\"\n",
i, slave_data->phy_node->full_name);
} else if (of_phy_is_fixed_link(slave_node)) {
-   struct device_node *phy_node;
-   struct phy_device *phy_dev;
-
/* In the case of a fixed PHY, the DT node associated
 * to the PHY is the Ethernet MAC DT node.
 */
ret = of_phy_register_fixed_link(slave_node);
if (ret)
return ret;
-   phy_node = of_node_get(slave_node);
-   phy_dev = of_phy_find_device(phy_node);
-   if (!phy_dev)
-   return -ENODEV;
-   snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
-PHY_ID_FMT, phy_dev->mdio.bus->id,
-phy_dev->mdio.addr);
+   slave_data->phy_node = of_node_get(slave_node);
} else if (parp) {
u32 phyid;
struct device_node *mdio_node;
struct platform_device *mdio;
 
if (lenp != (sizeof(__be32) * 2)) {
dev_err(>dev, "Invalid slave[%d] phy_id 
property\n", i);
-- 
2.5.5

[PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property

[David Rivshin (Allworx)]

From: David Rivshin 

The phy-handle, phy_id, and fixed-link properties are mutually exclusive,
and only one need be specified. However if phy-handle was specified, an
error message would complain about the lack of phy_id or fixed-link.

Also, if phy-handle was specified and the subsequent of_phy_connect()
failed, the error message still referenced slaved->data->phy_id, which
would be empty. Instead, use the name of the device_node as a useful
identifier.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin 
Acked-by: Rob Herring 
Tested-by: Nicolas Chauvet 
---
If would like this for -stable it should apply cleanly as far back
as 4.5. It failes on 4.4 due to some context differences, but can be
applied with 'git am -C2'. Or, I can produce a separate patch against
linux-4.4.y if preferred.

Changes since v1 [1]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet
- Added Acked-by from Rob Herring for the binding change

[1] https://patchwork.ozlabs.org/patch/560324/

 Documentation/devicetree/bindings/net/cpsw.txt |  4 ++--
 drivers/net/ethernet/ti/cpsw.c | 17 +
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
b/Documentation/devicetree/bindings/net/cpsw.txt
index 28a4781..3033c0f 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -46,16 +46,16 @@ Optional properties:
 - dual_emac_res_vlan   : Specifies VID to be used to segregate the ports
 - mac-address  : See ethernet.txt file in the same directory
 - phy_id   : Specifies slave phy id
 - phy-handle   : See ethernet.txt file in the same directory
 
 Slave sub-nodes:
 - fixed-link   : See fixed-link.txt file in the same directory
- Either the property phy_id, or the sub-node
- fixed-link can be specified
+
+Note: Exactly one of phy_id, phy-handle, or fixed-link must be specified.
 
 Note: "ti,hwmods" field is used to fetch the base address and irq
 resources from TI, omap hwmod data base during device registration.
 Future plan is to migrate hwmod data base contents into device tree
 blob so that, all the required data will be used from device tree dts
 file.
 
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index d69cb3f..3c81413 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1150,16 +1150,19 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
struct cpsw_priv *priv)
if (slave->data->phy_node)
slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 _adjust_link, 0, slave->data->phy_if);
else
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _adjust_link, slave->data->phy_if);
if (IS_ERR(slave->phy)) {
-   dev_err(priv->dev, "phy %s not found on slave %d\n",
-   slave->data->phy_id, slave->slave_num);
+   dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
+   slave->data->phy_node ?
+   slave->data->phy_node->full_name :
+   slave->data->phy_id,
+   slave->slave_num);
slave->phy = NULL;
} else {
phy_attached_info(slave->phy);
 
phy_start(slave->phy);
 
/* Configure GMII_SEL register */
@@ -2030,15 +2033,19 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
/* This is no slave child node, continue */
if (strcmp(slave_node->name, "slave"))
continue;
 
slave_data->phy_node = of_parse_phandle(slave_node,
"phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
-   if (of_phy_is_fixed_link(slave_node)) {
+   if (slave_data->phy_node) {
+   dev_dbg(>dev,
+   "slave[%d] using phy-handle=\"%s\"\n",
+   i, slave_data->phy_node->full_name);
+   } else if (of_phy_is_fixed_link(slave_node)) {
struct device_node *phy_node;
struct phy_device *phy_dev;
 
/* In the case of a fixed PHY, the DT node associated
 * to the PHY is the Ethernet MAC DT node.
 */
ret = of_phy_register_fixed_link(slave_node);
@@ -2067,15 +2074,17 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
if (!mdio) {
dev_err(>dev, 

[PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property

[David Rivshin (Allworx)]

From: David Rivshin 

The phy-handle, phy_id, and fixed-link properties are mutually exclusive,
and only one need be specified. However if phy-handle was specified, an
error message would complain about the lack of phy_id or fixed-link.

Also, if phy-handle was specified and the subsequent of_phy_connect()
failed, the error message still referenced slaved->data->phy_id, which
would be empty. Instead, use the name of the device_node as a useful
identifier.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin 
Acked-by: Rob Herring 
Tested-by: Nicolas Chauvet 
---
If would like this for -stable it should apply cleanly as far back
as 4.5. It failes on 4.4 due to some context differences, but can be
applied with 'git am -C2'. Or, I can produce a separate patch against
linux-4.4.y if preferred.

Changes since v1 [1]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet
- Added Acked-by from Rob Herring for the binding change

[1] https://patchwork.ozlabs.org/patch/560324/

 Documentation/devicetree/bindings/net/cpsw.txt |  4 ++--
 drivers/net/ethernet/ti/cpsw.c | 17 +
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
b/Documentation/devicetree/bindings/net/cpsw.txt
index 28a4781..3033c0f 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -46,16 +46,16 @@ Optional properties:
 - dual_emac_res_vlan   : Specifies VID to be used to segregate the ports
 - mac-address  : See ethernet.txt file in the same directory
 - phy_id   : Specifies slave phy id
 - phy-handle   : See ethernet.txt file in the same directory
 
 Slave sub-nodes:
 - fixed-link   : See fixed-link.txt file in the same directory
- Either the property phy_id, or the sub-node
- fixed-link can be specified
+
+Note: Exactly one of phy_id, phy-handle, or fixed-link must be specified.
 
 Note: "ti,hwmods" field is used to fetch the base address and irq
 resources from TI, omap hwmod data base during device registration.
 Future plan is to migrate hwmod data base contents into device tree
 blob so that, all the required data will be used from device tree dts
 file.
 
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index d69cb3f..3c81413 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1150,16 +1150,19 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
struct cpsw_priv *priv)
if (slave->data->phy_node)
slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 _adjust_link, 0, slave->data->phy_if);
else
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _adjust_link, slave->data->phy_if);
if (IS_ERR(slave->phy)) {
-   dev_err(priv->dev, "phy %s not found on slave %d\n",
-   slave->data->phy_id, slave->slave_num);
+   dev_err(priv->dev, "phy \"%s\" not found on slave %d\n",
+   slave->data->phy_node ?
+   slave->data->phy_node->full_name :
+   slave->data->phy_id,
+   slave->slave_num);
slave->phy = NULL;
} else {
phy_attached_info(slave->phy);
 
phy_start(slave->phy);
 
/* Configure GMII_SEL register */
@@ -2030,15 +2033,19 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
/* This is no slave child node, continue */
if (strcmp(slave_node->name, "slave"))
continue;
 
slave_data->phy_node = of_parse_phandle(slave_node,
"phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
-   if (of_phy_is_fixed_link(slave_node)) {
+   if (slave_data->phy_node) {
+   dev_dbg(>dev,
+   "slave[%d] using phy-handle=\"%s\"\n",
+   i, slave_data->phy_node->full_name);
+   } else if (of_phy_is_fixed_link(slave_node)) {
struct device_node *phy_node;
struct phy_device *phy_dev;
 
/* In the case of a fixed PHY, the DT node associated
 * to the PHY is the Ethernet MAC DT node.
 */
ret = of_phy_register_fixed_link(slave_node);
@@ -2067,15 +2074,17 @@ static int cpsw_probe_dt(struct cpsw_platform_data 
*data,
if (!mdio) {
dev_err(>dev, "Missing mdio platform 
device\n");
return 

[PATCH net v2 1/3] drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac config

[David Rivshin (Allworx)]

From: David Rivshin 

Commit 9e42f715264ff158478fa30eaed847f6e131366b ("drivers: net: cpsw: add
phy-handle parsing") saved the "phy-handle" phandle into a new cpsw_priv
field. However, phy connections are per-slave, so the phy_node field should
be in cpsw_slave_data rather than cpsw_priv.

This would go unnoticed in a single emac configuration. But in dual_emac
mode, the last "phy-handle" property parsed for either slave would be used
by both of them, causing them both to refer to the same phy_device.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin 
Tested-by: Nicolas Chauvet 
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.4.

Changes since v1 [1]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet

[1] https://patchwork.ozlabs.org/patch/560326/

 drivers/net/ethernet/ti/cpsw.c | 13 ++---
 drivers/net/ethernet/ti/cpsw.h |  1 +
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 42fdfd4..d69cb3f 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -363,15 +363,14 @@ static inline void slave_write(struct cpsw_slave *slave, 
u32 val, u32 offset)
__raw_writel(val, slave->regs + offset);
 }
 
 struct cpsw_priv {
spinlock_t  lock;
struct platform_device  *pdev;
struct net_device   *ndev;
-   struct device_node  *phy_node;
struct napi_struct  napi_rx;
struct napi_struct  napi_tx;
struct device   *dev;
struct cpsw_platform_data   data;
struct cpsw_ss_regs __iomem *regs;
struct cpsw_wr_regs __iomem *wr_regs;
u8 __iomem  *hw_stats;
@@ -1144,16 +1143,16 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
struct cpsw_priv *priv)
 
if (priv->data.dual_emac)
cpsw_add_dual_emac_def_ale_entries(priv, slave, slave_port);
else
cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
   1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
 
-   if (priv->phy_node)
-   slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
+   if (slave->data->phy_node)
+   slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 _adjust_link, 0, slave->data->phy_if);
else
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _adjust_link, slave->data->phy_if);
if (IS_ERR(slave->phy)) {
dev_err(priv->dev, "phy %s not found on slave %d\n",
slave->data->phy_id, slave->slave_num);
@@ -1936,20 +1935,19 @@ static void cpsw_slave_init(struct cpsw_slave *slave, 
struct cpsw_priv *priv,
 
slave->data = data;
slave->regs = regs + slave_reg_ofs;
slave->sliver   = regs + sliver_reg_ofs;
slave->port_vlan = data->dual_emac_res_vlan;
 }
 
-static int cpsw_probe_dt(struct cpsw_priv *priv,
+static int cpsw_probe_dt(struct cpsw_platform_data *data,
 struct platform_device *pdev)
 {
struct device_node *node = pdev->dev.of_node;
struct device_node *slave_node;
-   struct cpsw_platform_data *data = >data;
int i = 0, ret;
u32 prop;
 
if (!node)
return -EINVAL;
 
if (of_property_read_u32(node, "slaves", )) {
@@ -2029,15 +2027,16 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
int lenp;
const __be32 *parp;
 
/* This is no slave child node, continue */
if (strcmp(slave_node->name, "slave"))
continue;
 
-   priv->phy_node = of_parse_phandle(slave_node, "phy-handle", 0);
+   slave_data->phy_node = of_parse_phandle(slave_node,
+   "phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
if (of_phy_is_fixed_link(slave_node)) {
struct device_node *phy_node;
struct phy_device *phy_dev;
 
/* In the case of a fixed PHY, the DT node associated
 * to the PHY is the Ethernet MAC DT node.
@@ -2271,15 +2270,15 @@ static int cpsw_probe(struct platform_device *pdev)
 * This may be required here for child devices.
 */
pm_runtime_enable(>dev);
 
/* Select default pin state */
pinctrl_pm_select_default_state(>dev);
 
-   if (cpsw_probe_dt(priv, pdev)) {
+   if (cpsw_probe_dt(>data, pdev)) {
dev_err(>dev, "cpsw: platform data missing\n");
ret = 

[PATCH net v2 1/3] drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac config

[David Rivshin (Allworx)]

From: David Rivshin 

Commit 9e42f715264ff158478fa30eaed847f6e131366b ("drivers: net: cpsw: add
phy-handle parsing") saved the "phy-handle" phandle into a new cpsw_priv
field. However, phy connections are per-slave, so the phy_node field should
be in cpsw_slave_data rather than cpsw_priv.

This would go unnoticed in a single emac configuration. But in dual_emac
mode, the last "phy-handle" property parsed for either slave would be used
by both of them, causing them both to refer to the same phy_device.

Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Signed-off-by: David Rivshin 
Tested-by: Nicolas Chauvet 
---
I would suggest this for -stable. It should apply cleanly as far back
as 4.4.

Changes since v1 [1]:
- Rebased (no conflicts)
- Added Tested-by from Nicolas Chauvet

[1] https://patchwork.ozlabs.org/patch/560326/

 drivers/net/ethernet/ti/cpsw.c | 13 ++---
 drivers/net/ethernet/ti/cpsw.h |  1 +
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 42fdfd4..d69cb3f 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -363,15 +363,14 @@ static inline void slave_write(struct cpsw_slave *slave, 
u32 val, u32 offset)
__raw_writel(val, slave->regs + offset);
 }
 
 struct cpsw_priv {
spinlock_t  lock;
struct platform_device  *pdev;
struct net_device   *ndev;
-   struct device_node  *phy_node;
struct napi_struct  napi_rx;
struct napi_struct  napi_tx;
struct device   *dev;
struct cpsw_platform_data   data;
struct cpsw_ss_regs __iomem *regs;
struct cpsw_wr_regs __iomem *wr_regs;
u8 __iomem  *hw_stats;
@@ -1144,16 +1143,16 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
struct cpsw_priv *priv)
 
if (priv->data.dual_emac)
cpsw_add_dual_emac_def_ale_entries(priv, slave, slave_port);
else
cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
   1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
 
-   if (priv->phy_node)
-   slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
+   if (slave->data->phy_node)
+   slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node,
 _adjust_link, 0, slave->data->phy_if);
else
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
 _adjust_link, slave->data->phy_if);
if (IS_ERR(slave->phy)) {
dev_err(priv->dev, "phy %s not found on slave %d\n",
slave->data->phy_id, slave->slave_num);
@@ -1936,20 +1935,19 @@ static void cpsw_slave_init(struct cpsw_slave *slave, 
struct cpsw_priv *priv,
 
slave->data = data;
slave->regs = regs + slave_reg_ofs;
slave->sliver   = regs + sliver_reg_ofs;
slave->port_vlan = data->dual_emac_res_vlan;
 }
 
-static int cpsw_probe_dt(struct cpsw_priv *priv,
+static int cpsw_probe_dt(struct cpsw_platform_data *data,
 struct platform_device *pdev)
 {
struct device_node *node = pdev->dev.of_node;
struct device_node *slave_node;
-   struct cpsw_platform_data *data = >data;
int i = 0, ret;
u32 prop;
 
if (!node)
return -EINVAL;
 
if (of_property_read_u32(node, "slaves", )) {
@@ -2029,15 +2027,16 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
int lenp;
const __be32 *parp;
 
/* This is no slave child node, continue */
if (strcmp(slave_node->name, "slave"))
continue;
 
-   priv->phy_node = of_parse_phandle(slave_node, "phy-handle", 0);
+   slave_data->phy_node = of_parse_phandle(slave_node,
+   "phy-handle", 0);
parp = of_get_property(slave_node, "phy_id", );
if (of_phy_is_fixed_link(slave_node)) {
struct device_node *phy_node;
struct phy_device *phy_dev;
 
/* In the case of a fixed PHY, the DT node associated
 * to the PHY is the Ethernet MAC DT node.
@@ -2271,15 +2270,15 @@ static int cpsw_probe(struct platform_device *pdev)
 * This may be required here for child devices.
 */
pm_runtime_enable(>dev);
 
/* Select default pin state */
pinctrl_pm_select_default_state(>dev);
 
-   if (cpsw_probe_dt(priv, pdev)) {
+   if (cpsw_probe_dt(>data, pdev)) {
dev_err(>dev, "cpsw: platform data missing\n");
ret = -ENODEV;
goto clean_runtime_disable_ret;

[PATCH net v2 0/3] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

From: David Rivshin 

The first patch fixes a bug that makes dual_emac mode break if
either slave uses the phy-handle property in the devicetree.

The second patch fixes some cosmetic problems with error messages,
and also makes the binding documentation more explicit.

The third patch cleans up the fixed-link case to work like
the now-fixed phy-handle case.

I have tested on the following hardware configurations:
 - (EVMSK) dual emac, phy_id property in both slaves
 - (EVMSK) dual emac, phy-handle property in both slaves
 - (BeagleBoneBlack) single emac, phy_id property
 - (custom) single emac, fixed-link subnode

Nicolas Chauvet reported testing on an HP t410 (dm8148).

Markus Brunner reported testing v1 on the following [1]:
 - emac0 with phy_id and emac1 with fixed phy
 - emac0 with phy-handle and emac1 with fixed phy
 - emac0 with fixed phy and emac1 with fixed phy


Changes since v1 [2]:
- Rebased
- Added Tested-by from Nicolas Chauvet on all patches
- Added Acked-by from Rob Herring for the binding change in patch 2 [3]

[1] http://www.spinics.net/lists/netdev/msg357890.html
[2] http://www.spinics.net/lists/netdev/msg357772.html
[3] http://www.spinics.net/lists/netdev/msg358254.html

David Rivshin (3):
  drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac
config
  drivers: net: cpsw: fix error messages when using phy-handle DT
property
  drivers: net: cpsw: use of_phy_connect() in fixed-link case

 Documentation/devicetree/bindings/net/cpsw.txt |  4 +--
 drivers/net/ethernet/ti/cpsw.c | 41 +-
 drivers/net/ethernet/ti/cpsw.h |  1 +
 3 files changed, 23 insertions(+), 23 deletions(-)

-- 
2.5.5

[PATCH net v2 0/3] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

From: David Rivshin 

The first patch fixes a bug that makes dual_emac mode break if
either slave uses the phy-handle property in the devicetree.

The second patch fixes some cosmetic problems with error messages,
and also makes the binding documentation more explicit.

The third patch cleans up the fixed-link case to work like
the now-fixed phy-handle case.

I have tested on the following hardware configurations:
 - (EVMSK) dual emac, phy_id property in both slaves
 - (EVMSK) dual emac, phy-handle property in both slaves
 - (BeagleBoneBlack) single emac, phy_id property
 - (custom) single emac, fixed-link subnode

Nicolas Chauvet reported testing on an HP t410 (dm8148).

Markus Brunner reported testing v1 on the following [1]:
 - emac0 with phy_id and emac1 with fixed phy
 - emac0 with phy-handle and emac1 with fixed phy
 - emac0 with fixed phy and emac1 with fixed phy


Changes since v1 [2]:
- Rebased
- Added Tested-by from Nicolas Chauvet on all patches
- Added Acked-by from Rob Herring for the binding change in patch 2 [3]

[1] http://www.spinics.net/lists/netdev/msg357890.html
[2] http://www.spinics.net/lists/netdev/msg357772.html
[3] http://www.spinics.net/lists/netdev/msg358254.html

David Rivshin (3):
  drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac
config
  drivers: net: cpsw: fix error messages when using phy-handle DT
property
  drivers: net: cpsw: use of_phy_connect() in fixed-link case

 Documentation/devicetree/bindings/net/cpsw.txt |  4 +--
 drivers/net/ethernet/ti/cpsw.c | 41 +-
 drivers/net/ethernet/ti/cpsw.h |  1 +
 3 files changed, 23 insertions(+), 23 deletions(-)

-- 
2.5.5

Re: [PATCH net v2 0/3] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

Sorry all for the noise. Gmail seems to be deciding that this outgoing 
mail is spammy, and starts blocking it part-way through. I've tried 
cutting down the CC list, but still no luck. If anyone knows how to get 
around this (while still having a reasonable patch submission), please 
let me know. 

For tonight, I guess I have no choice but to give up. I'll try again
tomorrow in hopes gmail becomes sane again.


On Wed, 20 Apr 2016 23:24:39 -0400
"David Rivshin (Allworx)" <drivshin.allw...@gmail.com> wrote:

> From: David Rivshin <drivs...@allworx.com>
> 
> The first patch fixes a bug that makes dual_emac mode break if
> either slave uses the phy-handle property in the devicetree.
> 
> The second patch fixes some cosmetic problems with error messages,
> and also makes the binding documentation more explicit.
> 
> The third patch cleans up the fixed-link case to work like
> the now-fixed phy-handle case.
> 
> I have tested on the following hardware configurations:
>  - (EVMSK) dual emac, phy_id property in both slaves
>  - (EVMSK) dual emac, phy-handle property in both slaves
>  - (BeagleBoneBlack) single emac, phy_id property
>  - (custom) single emac, fixed-link subnode
> 
> Nicolas Chauvet reported testing on an HP t410 (dm8148).
> 
> Markus Brunner reported testing v1 on the following [1]:
>  - emac0 with phy_id and emac1 with fixed phy
>  - emac0 with phy-handle and emac1 with fixed phy
>  - emac0 with fixed phy and emac1 with fixed phy
> 
> 
> Changes since v1 [2]:
> - Rebased
> - Added Tested-by from Nicolas Chauvet on all patches
> - Added Acked-by from Rob Herring for the binding change in patch 2 [3]
> 
> [1] http://www.spinics.net/lists/netdev/msg357890.html
> [2] http://www.spinics.net/lists/netdev/msg357772.html
> [3] http://www.spinics.net/lists/netdev/msg358254.html
> 
> David Rivshin (3):
>   drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac
> config
>   drivers: net: cpsw: fix error messages when using phy-handle DT
> property
>   drivers: net: cpsw: use of_phy_connect() in fixed-link case
> 
>  Documentation/devicetree/bindings/net/cpsw.txt |  4 +--
>  drivers/net/ethernet/ti/cpsw.c | 41 
> +-
>  drivers/net/ethernet/ti/cpsw.h |  1 +
>  3 files changed, 23 insertions(+), 23 deletions(-)
> 

Re: [PATCH net v2 0/3] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

Sorry all for the noise. Gmail seems to be deciding that this outgoing 
mail is spammy, and starts blocking it part-way through. I've tried 
cutting down the CC list, but still no luck. If anyone knows how to get 
around this (while still having a reasonable patch submission), please 
let me know. 

For tonight, I guess I have no choice but to give up. I'll try again
tomorrow in hopes gmail becomes sane again.


On Wed, 20 Apr 2016 23:24:39 -0400
"David Rivshin (Allworx)"  wrote:

> From: David Rivshin 
> 
> The first patch fixes a bug that makes dual_emac mode break if
> either slave uses the phy-handle property in the devicetree.
> 
> The second patch fixes some cosmetic problems with error messages,
> and also makes the binding documentation more explicit.
> 
> The third patch cleans up the fixed-link case to work like
> the now-fixed phy-handle case.
> 
> I have tested on the following hardware configurations:
>  - (EVMSK) dual emac, phy_id property in both slaves
>  - (EVMSK) dual emac, phy-handle property in both slaves
>  - (BeagleBoneBlack) single emac, phy_id property
>  - (custom) single emac, fixed-link subnode
> 
> Nicolas Chauvet reported testing on an HP t410 (dm8148).
> 
> Markus Brunner reported testing v1 on the following [1]:
>  - emac0 with phy_id and emac1 with fixed phy
>  - emac0 with phy-handle and emac1 with fixed phy
>  - emac0 with fixed phy and emac1 with fixed phy
> 
> 
> Changes since v1 [2]:
> - Rebased
> - Added Tested-by from Nicolas Chauvet on all patches
> - Added Acked-by from Rob Herring for the binding change in patch 2 [3]
> 
> [1] http://www.spinics.net/lists/netdev/msg357890.html
> [2] http://www.spinics.net/lists/netdev/msg357772.html
> [3] http://www.spinics.net/lists/netdev/msg358254.html
> 
> David Rivshin (3):
>   drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac
> config
>   drivers: net: cpsw: fix error messages when using phy-handle DT
> property
>   drivers: net: cpsw: use of_phy_connect() in fixed-link case
> 
>  Documentation/devicetree/bindings/net/cpsw.txt |  4 +--
>  drivers/net/ethernet/ti/cpsw.c | 41 
> +-
>  drivers/net/ethernet/ti/cpsw.h |  1 +
>  3 files changed, 23 insertions(+), 23 deletions(-)
> 

[PATCH net v2 0/3] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

From: David Rivshin 

The first patch fixes a bug that makes dual_emac mode break if
either slave uses the phy-handle property in the devicetree.

The second patch fixes some cosmetic problems with error messages,
and also makes the binding documentation more explicit.

The third patch cleans up the fixed-link case to work like
the now-fixed phy-handle case.

I have tested on the following hardware configurations:
 - (EVMSK) dual emac, phy_id property in both slaves
 - (EVMSK) dual emac, phy-handle property in both slaves
 - (BeagleBoneBlack) single emac, phy_id property
 - (custom) single emac, fixed-link subnode

Nicolas Chauvet reported testing on an HP t410 (dm8148).

Markus Brunner reported testing v1 on the following [1]:
 - emac0 with phy_id and emac1 with fixed phy
 - emac0 with phy-handle and emac1 with fixed phy
 - emac0 with fixed phy and emac1 with fixed phy


Changes since v1 [2]:
- Rebased
- Added Tested-by from Nicolas Chauvet on all patches
- Added Acked-by from Rob Herring for the binding change in patch 2 [3]

[1] http://www.spinics.net/lists/netdev/msg357890.html
[2] http://www.spinics.net/lists/netdev/msg357772.html
[3] http://www.spinics.net/lists/netdev/msg358254.html

David Rivshin (3):
  drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac
config
  drivers: net: cpsw: fix error messages when using phy-handle DT
property
  drivers: net: cpsw: use of_phy_connect() in fixed-link case

 Documentation/devicetree/bindings/net/cpsw.txt |  4 +--
 drivers/net/ethernet/ti/cpsw.c | 41 +-
 drivers/net/ethernet/ti/cpsw.h |  1 +
 3 files changed, 23 insertions(+), 23 deletions(-)

-- 
2.5.5

[PATCH net v2 0/3] drivers: net: cpsw: phy-handle fixes

[David Rivshin (Allworx)]

From: David Rivshin 

The first patch fixes a bug that makes dual_emac mode break if
either slave uses the phy-handle property in the devicetree.

The second patch fixes some cosmetic problems with error messages,
and also makes the binding documentation more explicit.

The third patch cleans up the fixed-link case to work like
the now-fixed phy-handle case.

I have tested on the following hardware configurations:
 - (EVMSK) dual emac, phy_id property in both slaves
 - (EVMSK) dual emac, phy-handle property in both slaves
 - (BeagleBoneBlack) single emac, phy_id property
 - (custom) single emac, fixed-link subnode

Nicolas Chauvet reported testing on an HP t410 (dm8148).

Markus Brunner reported testing v1 on the following [1]:
 - emac0 with phy_id and emac1 with fixed phy
 - emac0 with phy-handle and emac1 with fixed phy
 - emac0 with fixed phy and emac1 with fixed phy


Changes since v1 [2]:
- Rebased
- Added Tested-by from Nicolas Chauvet on all patches
- Added Acked-by from Rob Herring for the binding change in patch 2 [3]

[1] http://www.spinics.net/lists/netdev/msg357890.html
[2] http://www.spinics.net/lists/netdev/msg357772.html
[3] http://www.spinics.net/lists/netdev/msg358254.html

David Rivshin (3):
  drivers: net: cpsw: fix parsing of phy-handle DT property in dual_emac
config
  drivers: net: cpsw: fix error messages when using phy-handle DT
property
  drivers: net: cpsw: use of_phy_connect() in fixed-link case

 Documentation/devicetree/bindings/net/cpsw.txt |  4 +--
 drivers/net/ethernet/ti/cpsw.c | 41 +-
 drivers/net/ethernet/ti/cpsw.h |  1 +
 3 files changed, 23 insertions(+), 23 deletions(-)

-- 
2.5.5

Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs

[David Rivshin (Allworx)]

On Tue, 19 Apr 2016 18:43:39 -0400 (EDT)
David Miller  wrote:

> From: Grygorii Strashko 
> Date: Tue, 19 Apr 2016 21:44:09 +0300
> 
> > May be you can send revert + your patch 1 (only fix for this issue).
> > 
> > Dave, Does that sound good to you?  
> 
> Sure.

OK, I will hopefully have that ready tomorrow evening.

Anything in particular I should mention in the revert commit message?
I didn't find a discussion on it, other than the earlier statement that 
"it breaks boot on many TI boards".

Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs

[David Rivshin (Allworx)]

On Tue, 19 Apr 2016 18:43:39 -0400 (EDT)
David Miller  wrote:

> From: Grygorii Strashko 
> Date: Tue, 19 Apr 2016 21:44:09 +0300
> 
> > May be you can send revert + your patch 1 (only fix for this issue).
> > 
> > Dave, Does that sound good to you?  
> 
> Sure.

OK, I will hopefully have that ready tomorrow evening.

Anything in particular I should mention in the revert commit message?
I didn't find a discussion on it, other than the earlier statement that 
"it breaks boot on many TI boards".

Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs

[David Rivshin (Allworx)]

On Tue, 19 Apr 2016 18:44:41 +0300
Grygorii Strashko <grygorii.stras...@ti.com> wrote:

> On 04/19/2016 06:01 PM, David Rivshin (Allworx) wrote:
> > On Tue, 19 Apr 2016 17:41:07 +0300
> > Grygorii Strashko <grygorii.stras...@ti.com> wrote:
> >   
> >> Hi,
> >>
> >> On 04/19/2016 04:56 PM, Andrew Goodbody wrote:  
> >>> Adding a 2nd PHY to cpsw results in a NULL pointer dereference
> >>> as below. Fix by maintaining a reference to each PHY node in slave
> >>> struct instead of a single reference in the priv struct which was
> >>> overwritten by the 2nd PHY.  
> >>
> >> David, Is it possible to drop prev version of this patch from linux-next
> >> - it breaks boot on many TI boards with -next.
> >>
> >>  
> >>>
> >>> [   17.870933] Unable to handle kernel NULL pointer dereference at 
> >>> virtual address 0180
> >>> [   17.879557] pgd = dc8bc000
> >>> [   17.882514] [0180] *pgd=9c882831, *pte=, *ppte=
> >>> [   17.889213] Internal error: Oops: 17 [#1] ARM
> >>> [   17.893838] Modules linked in:
> >>> [   17.897102] CPU: 0 PID: 1657 Comm: connmand Not tainted 
> >>> 4.5.0-ge463dfb-dirty #11
> >>> [   17.904947] Hardware name: Cambrionix whippet
> >>> [   17.909576] task: dc859240 ti: dc968000 task.ti: dc968000
> >>> [   17.915339] PC is at phy_attached_print+0x18/0x8c
> >>> [   17.920339] LR is at phy_attached_info+0x14/0x18
> >>> [   17.925247] pc : []lr : []psr: 600f0113
> >>> [   17.925247] sp : dc969cf8  ip : dc969d28  fp : dc969d18
> >>> [   17.937425] r10: dda7a400  r9 :   r8 : 
> >>> [   17.942971] r7 : 0001  r6 : ddb00480  r5 : ddb8cb34  r4 : 
> >>> [   17.949898] r3 : c0954cc0  r2 : c09562b0  r1 :   r0 : 
> >>> [   17.956829] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  
> >>> Segment none
> >>> [   17.964401] Control: 10c5387d  Table: 9c8bc019  DAC: 0051
> >>> [   17.970500] Process connmand (pid: 1657, stack limit = 0xdc968210)
> >>> [   17.977059] Stack: (0xdc969cf8 to 0xdc96a000)  
> >>
> >> [...]
> >>  
> >>> [   18.323956] [] (inet_ioctl) from [] 
> >>> (sock_ioctl+0x15c/0x2d8)
> >>> [   18.331829] [] (sock_ioctl) from [] 
> >>> (do_vfs_ioctl+0x98/0x8d0)
> >>> [   18.339765]  r7:8914 r6:dc8ab4c0 r5:dd257ae0 r4:beaeda20
> >>> [   18.345822] [] (do_vfs_ioctl) from [] 
> >>> (SyS_ioctl+0x74/0x84)
> >>> [   18.353573]  r10: r9:0011 r8:beaeda20 r7:8914 
> >>> r6:dc8ab4c0 r5:dc8ab4c0
> >>> [   18.361924]  r4:
> >>> [   18.364653] [] (SyS_ioctl) from [] 
> >>> (ret_fast_syscall+0x0/0x3c)
> >>> [   18.372682]  r9:dc968000 r8:c00165e8 r7:0036 r6:0002 
> >>> r5:0011 r4:
> >>> [   18.380960] Code: e92dd810 e24cb010 e24dd010 e59b4004 (e5902180)
> >>> [   18.387580] ---[ end trace c80529466223f3f3 ]---  
> >>
> >> ^ Could you make it shorter and drop timestamps, pls?
> >>  
> >>>
> >>> Signed-off-by: Andrew Goodbody <andrew.goodb...@cambrionix.com>
> >>> ---
> >>>
> >>> v2 - Move allocation of memory for priv->slaves to inside cpsw_probe_dt 
> >>> so it
> >>>has data->slaves initialised first which is needed to calculate 
> >>> size
> >>>
> >>>drivers/net/ethernet/ti/cpsw.c | 30 +++---
> >>>1 file changed, 15 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/ti/cpsw.c 
> >>> b/drivers/net/ethernet/ti/cpsw.c
> >>> index 42fdfd4..e62909c 100644
> >>> --- a/drivers/net/ethernet/ti/cpsw.c
> >>> +++ b/drivers/net/ethernet/ti/cpsw.c
> >>> @@ -349,6 +349,7 @@ struct cpsw_slave {
> >>>   struct cpsw_slave_data  *data;
> >>>   struct phy_device   *phy;
> >>>   struct net_device   *ndev;
> >>> + struct device_node  *phy_node;
> >>>   u32 port_vlan;
> >>>   u32 open_stat;
> >>>};
> >>> @@ -367,7 +368,6 @@ struct cpsw_priv {
> >>>   spinlock_t  lock;
> &

Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs

[David Rivshin (Allworx)]

On Tue, 19 Apr 2016 18:44:41 +0300
Grygorii Strashko  wrote:

> On 04/19/2016 06:01 PM, David Rivshin (Allworx) wrote:
> > On Tue, 19 Apr 2016 17:41:07 +0300
> > Grygorii Strashko  wrote:
> >   
> >> Hi,
> >>
> >> On 04/19/2016 04:56 PM, Andrew Goodbody wrote:  
> >>> Adding a 2nd PHY to cpsw results in a NULL pointer dereference
> >>> as below. Fix by maintaining a reference to each PHY node in slave
> >>> struct instead of a single reference in the priv struct which was
> >>> overwritten by the 2nd PHY.  
> >>
> >> David, Is it possible to drop prev version of this patch from linux-next
> >> - it breaks boot on many TI boards with -next.
> >>
> >>  
> >>>
> >>> [   17.870933] Unable to handle kernel NULL pointer dereference at 
> >>> virtual address 0180
> >>> [   17.879557] pgd = dc8bc000
> >>> [   17.882514] [0180] *pgd=9c882831, *pte=, *ppte=
> >>> [   17.889213] Internal error: Oops: 17 [#1] ARM
> >>> [   17.893838] Modules linked in:
> >>> [   17.897102] CPU: 0 PID: 1657 Comm: connmand Not tainted 
> >>> 4.5.0-ge463dfb-dirty #11
> >>> [   17.904947] Hardware name: Cambrionix whippet
> >>> [   17.909576] task: dc859240 ti: dc968000 task.ti: dc968000
> >>> [   17.915339] PC is at phy_attached_print+0x18/0x8c
> >>> [   17.920339] LR is at phy_attached_info+0x14/0x18
> >>> [   17.925247] pc : []lr : []psr: 600f0113
> >>> [   17.925247] sp : dc969cf8  ip : dc969d28  fp : dc969d18
> >>> [   17.937425] r10: dda7a400  r9 :   r8 : 
> >>> [   17.942971] r7 : 0001  r6 : ddb00480  r5 : ddb8cb34  r4 : 
> >>> [   17.949898] r3 : c0954cc0  r2 : c09562b0  r1 :   r0 : 
> >>> [   17.956829] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  
> >>> Segment none
> >>> [   17.964401] Control: 10c5387d  Table: 9c8bc019  DAC: 0051
> >>> [   17.970500] Process connmand (pid: 1657, stack limit = 0xdc968210)
> >>> [   17.977059] Stack: (0xdc969cf8 to 0xdc96a000)  
> >>
> >> [...]
> >>  
> >>> [   18.323956] [] (inet_ioctl) from [] 
> >>> (sock_ioctl+0x15c/0x2d8)
> >>> [   18.331829] [] (sock_ioctl) from [] 
> >>> (do_vfs_ioctl+0x98/0x8d0)
> >>> [   18.339765]  r7:8914 r6:dc8ab4c0 r5:dd257ae0 r4:beaeda20
> >>> [   18.345822] [] (do_vfs_ioctl) from [] 
> >>> (SyS_ioctl+0x74/0x84)
> >>> [   18.353573]  r10: r9:0011 r8:beaeda20 r7:8914 
> >>> r6:dc8ab4c0 r5:dc8ab4c0
> >>> [   18.361924]  r4:
> >>> [   18.364653] [] (SyS_ioctl) from [] 
> >>> (ret_fast_syscall+0x0/0x3c)
> >>> [   18.372682]  r9:dc968000 r8:c00165e8 r7:0036 r6:0002 
> >>> r5:0011 r4:
> >>> [   18.380960] Code: e92dd810 e24cb010 e24dd010 e59b4004 (e5902180)
> >>> [   18.387580] ---[ end trace c80529466223f3f3 ]---  
> >>
> >> ^ Could you make it shorter and drop timestamps, pls?
> >>  
> >>>
> >>> Signed-off-by: Andrew Goodbody 
> >>> ---
> >>>
> >>> v2 - Move allocation of memory for priv->slaves to inside cpsw_probe_dt 
> >>> so it
> >>>has data->slaves initialised first which is needed to calculate 
> >>> size
> >>>
> >>>drivers/net/ethernet/ti/cpsw.c | 30 +++---
> >>>1 file changed, 15 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/ti/cpsw.c 
> >>> b/drivers/net/ethernet/ti/cpsw.c
> >>> index 42fdfd4..e62909c 100644
> >>> --- a/drivers/net/ethernet/ti/cpsw.c
> >>> +++ b/drivers/net/ethernet/ti/cpsw.c
> >>> @@ -349,6 +349,7 @@ struct cpsw_slave {
> >>>   struct cpsw_slave_data  *data;
> >>>   struct phy_device   *phy;
> >>>   struct net_device   *ndev;
> >>> + struct device_node  *phy_node;
> >>>   u32 port_vlan;
> >>>   u32 open_stat;
> >>>};
> >>> @@ -367,7 +368,6 @@ struct cpsw_priv {
> >>>   spinlock_t  lock;
> >>>   struct platform_device  *pdev;
> >>>   struct

Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs

[David Rivshin (Allworx)]

On Tue, 19 Apr 2016 17:41:07 +0300
Grygorii Strashko  wrote:

> Hi,
> 
> On 04/19/2016 04:56 PM, Andrew Goodbody wrote:
> > Adding a 2nd PHY to cpsw results in a NULL pointer dereference
> > as below. Fix by maintaining a reference to each PHY node in slave
> > struct instead of a single reference in the priv struct which was
> > overwritten by the 2nd PHY.  
> 
> David, Is it possible to drop prev version of this patch from linux-next
> - it breaks boot on many TI boards with -next.
> 
> 
> > 
> > [   17.870933] Unable to handle kernel NULL pointer dereference at virtual 
> > address 0180
> > [   17.879557] pgd = dc8bc000
> > [   17.882514] [0180] *pgd=9c882831, *pte=, *ppte=
> > [   17.889213] Internal error: Oops: 17 [#1] ARM
> > [   17.893838] Modules linked in:
> > [   17.897102] CPU: 0 PID: 1657 Comm: connmand Not tainted 
> > 4.5.0-ge463dfb-dirty #11
> > [   17.904947] Hardware name: Cambrionix whippet
> > [   17.909576] task: dc859240 ti: dc968000 task.ti: dc968000
> > [   17.915339] PC is at phy_attached_print+0x18/0x8c
> > [   17.920339] LR is at phy_attached_info+0x14/0x18
> > [   17.925247] pc : []lr : []psr: 600f0113
> > [   17.925247] sp : dc969cf8  ip : dc969d28  fp : dc969d18
> > [   17.937425] r10: dda7a400  r9 :   r8 : 
> > [   17.942971] r7 : 0001  r6 : ddb00480  r5 : ddb8cb34  r4 : 
> > [   17.949898] r3 : c0954cc0  r2 : c09562b0  r1 :   r0 : 
> > [   17.956829] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment 
> > none
> > [   17.964401] Control: 10c5387d  Table: 9c8bc019  DAC: 0051
> > [   17.970500] Process connmand (pid: 1657, stack limit = 0xdc968210)
> > [   17.977059] Stack: (0xdc969cf8 to 0xdc96a000)  
> 
> [...]
> 
> > [   18.323956] [] (inet_ioctl) from [] 
> > (sock_ioctl+0x15c/0x2d8)
> > [   18.331829] [] (sock_ioctl) from [] 
> > (do_vfs_ioctl+0x98/0x8d0)
> > [   18.339765]  r7:8914 r6:dc8ab4c0 r5:dd257ae0 r4:beaeda20
> > [   18.345822] [] (do_vfs_ioctl) from [] 
> > (SyS_ioctl+0x74/0x84)
> > [   18.353573]  r10: r9:0011 r8:beaeda20 r7:8914 
> > r6:dc8ab4c0 r5:dc8ab4c0
> > [   18.361924]  r4:
> > [   18.364653] [] (SyS_ioctl) from [] 
> > (ret_fast_syscall+0x0/0x3c)
> > [   18.372682]  r9:dc968000 r8:c00165e8 r7:0036 r6:0002 r5:0011 
> > r4:
> > [   18.380960] Code: e92dd810 e24cb010 e24dd010 e59b4004 (e5902180)
> > [   18.387580] ---[ end trace c80529466223f3f3 ]---  
> 
> ^ Could you make it shorter and drop timestamps, pls?
> 
> > 
> > Signed-off-by: Andrew Goodbody 
> > ---
> > 
> > v2 - Move allocation of memory for priv->slaves to inside cpsw_probe_dt so 
> > it
> >   has data->slaves initialised first which is needed to calculate size
> > 
> >   drivers/net/ethernet/ti/cpsw.c | 30 +++---
> >   1 file changed, 15 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index 42fdfd4..e62909c 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -349,6 +349,7 @@ struct cpsw_slave {
> > struct cpsw_slave_data  *data;
> > struct phy_device   *phy;
> > struct net_device   *ndev;
> > +   struct device_node  *phy_node;
> > u32 port_vlan;
> > u32 open_stat;
> >   };
> > @@ -367,7 +368,6 @@ struct cpsw_priv {
> > spinlock_t  lock;
> > struct platform_device  *pdev;
> > struct net_device   *ndev;
> > -   struct device_node  *phy_node;
> > struct napi_struct  napi_rx;
> > struct napi_struct  napi_tx;
> > struct device   *dev;
> > @@ -1148,8 +1148,8 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
> > struct cpsw_priv *priv)
> > cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
> >1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
> >   
> > -   if (priv->phy_node)
> > -   slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
> > +   if (slave->phy_node)
> > +   slave->phy = of_phy_connect(priv->ndev, slave->phy_node,
> >  _adjust_link, 0, slave->data->phy_if);
> > else
> > slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> > @@ -1946,7 +1946,7 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> > struct device_node *node = pdev->dev.of_node;
> > struct device_node *slave_node;
> > struct cpsw_platform_data *data = >data;
> > -   int i = 0, ret;
> > +   int i, ret;
> > u32 prop;
> >   
> > if (!node)
> > @@ -1958,6 +1958,14 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> > }
> > data->slaves = prop;
> >   
> > +   priv->slaves = devm_kzalloc(>dev,
> > +   

Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs

[David Rivshin (Allworx)]

On Tue, 19 Apr 2016 17:41:07 +0300
Grygorii Strashko  wrote:

> Hi,
> 
> On 04/19/2016 04:56 PM, Andrew Goodbody wrote:
> > Adding a 2nd PHY to cpsw results in a NULL pointer dereference
> > as below. Fix by maintaining a reference to each PHY node in slave
> > struct instead of a single reference in the priv struct which was
> > overwritten by the 2nd PHY.  
> 
> David, Is it possible to drop prev version of this patch from linux-next
> - it breaks boot on many TI boards with -next.
> 
> 
> > 
> > [   17.870933] Unable to handle kernel NULL pointer dereference at virtual 
> > address 0180
> > [   17.879557] pgd = dc8bc000
> > [   17.882514] [0180] *pgd=9c882831, *pte=, *ppte=
> > [   17.889213] Internal error: Oops: 17 [#1] ARM
> > [   17.893838] Modules linked in:
> > [   17.897102] CPU: 0 PID: 1657 Comm: connmand Not tainted 
> > 4.5.0-ge463dfb-dirty #11
> > [   17.904947] Hardware name: Cambrionix whippet
> > [   17.909576] task: dc859240 ti: dc968000 task.ti: dc968000
> > [   17.915339] PC is at phy_attached_print+0x18/0x8c
> > [   17.920339] LR is at phy_attached_info+0x14/0x18
> > [   17.925247] pc : []lr : []psr: 600f0113
> > [   17.925247] sp : dc969cf8  ip : dc969d28  fp : dc969d18
> > [   17.937425] r10: dda7a400  r9 :   r8 : 
> > [   17.942971] r7 : 0001  r6 : ddb00480  r5 : ddb8cb34  r4 : 
> > [   17.949898] r3 : c0954cc0  r2 : c09562b0  r1 :   r0 : 
> > [   17.956829] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment 
> > none
> > [   17.964401] Control: 10c5387d  Table: 9c8bc019  DAC: 0051
> > [   17.970500] Process connmand (pid: 1657, stack limit = 0xdc968210)
> > [   17.977059] Stack: (0xdc969cf8 to 0xdc96a000)  
> 
> [...]
> 
> > [   18.323956] [] (inet_ioctl) from [] 
> > (sock_ioctl+0x15c/0x2d8)
> > [   18.331829] [] (sock_ioctl) from [] 
> > (do_vfs_ioctl+0x98/0x8d0)
> > [   18.339765]  r7:8914 r6:dc8ab4c0 r5:dd257ae0 r4:beaeda20
> > [   18.345822] [] (do_vfs_ioctl) from [] 
> > (SyS_ioctl+0x74/0x84)
> > [   18.353573]  r10: r9:0011 r8:beaeda20 r7:8914 
> > r6:dc8ab4c0 r5:dc8ab4c0
> > [   18.361924]  r4:
> > [   18.364653] [] (SyS_ioctl) from [] 
> > (ret_fast_syscall+0x0/0x3c)
> > [   18.372682]  r9:dc968000 r8:c00165e8 r7:0036 r6:0002 r5:0011 
> > r4:
> > [   18.380960] Code: e92dd810 e24cb010 e24dd010 e59b4004 (e5902180)
> > [   18.387580] ---[ end trace c80529466223f3f3 ]---  
> 
> ^ Could you make it shorter and drop timestamps, pls?
> 
> > 
> > Signed-off-by: Andrew Goodbody 
> > ---
> > 
> > v2 - Move allocation of memory for priv->slaves to inside cpsw_probe_dt so 
> > it
> >   has data->slaves initialised first which is needed to calculate size
> > 
> >   drivers/net/ethernet/ti/cpsw.c | 30 +++---
> >   1 file changed, 15 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index 42fdfd4..e62909c 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -349,6 +349,7 @@ struct cpsw_slave {
> > struct cpsw_slave_data  *data;
> > struct phy_device   *phy;
> > struct net_device   *ndev;
> > +   struct device_node  *phy_node;
> > u32 port_vlan;
> > u32 open_stat;
> >   };
> > @@ -367,7 +368,6 @@ struct cpsw_priv {
> > spinlock_t  lock;
> > struct platform_device  *pdev;
> > struct net_device   *ndev;
> > -   struct device_node  *phy_node;
> > struct napi_struct  napi_rx;
> > struct napi_struct  napi_tx;
> > struct device   *dev;
> > @@ -1148,8 +1148,8 @@ static void cpsw_slave_open(struct cpsw_slave *slave, 
> > struct cpsw_priv *priv)
> > cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
> >1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
> >   
> > -   if (priv->phy_node)
> > -   slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
> > +   if (slave->phy_node)
> > +   slave->phy = of_phy_connect(priv->ndev, slave->phy_node,
> >  _adjust_link, 0, slave->data->phy_if);
> > else
> > slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> > @@ -1946,7 +1946,7 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> > struct device_node *node = pdev->dev.of_node;
> > struct device_node *slave_node;
> > struct cpsw_platform_data *data = >data;
> > -   int i = 0, ret;
> > +   int i, ret;
> > u32 prop;
> >   
> > if (!node)
> > @@ -1958,6 +1958,14 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> > }
> > data->slaves = prop;
> >   
> > +   priv->slaves = devm_kzalloc(>dev,
> > +   sizeof(struct cpsw_slave) * data->slaves,
> > 

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[David Rivshin (Allworx)]

Hi Nikolaus,

I recently submitted a driver for the IS31FL32xx family of devices, so
this driver caught my eye. I have a few comments below.

On Mon, 18 Apr 2016 20:43:16 +0200
"H. Nikolaus Schaller"  wrote:

> This is a driver for the Integrated Silicon Solution Inc. LED driver
> chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
> LEDs.
> 
> Each LED is individually controllable in brightness (through pwm)
> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
> 
> The maximum current of the LEDs can be programmed and limited to
> 5 .. 40mA through a device tree property.
> 
> The chip is connected through I2C and can have one of 4 addresses (0x64 .. 
> 0x67)
> depending on how the AD pin is connected. The address is defined by the
> reg property as usual.
> 
> The chip also has a shutdown input which could be connected to a GPIO,
> but this driver uses software shutdown if all LEDs are inactivated.
> 
> The chip also has breathing and audio features which are not supported
> by this driver.
> 
> This driver was developed and tested on OMAP5 based Pyra handheld prototype.
> 
> Signed-off-by: H. Nikolaus Schaller 
> ---
>  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
>  drivers/leds/Kconfig   |   8 +
>  drivers/leds/Makefile  |   1 +
>  drivers/leds/leds-is31fl319x.c | 406 
> +
>  include/linux/leds-is31fl319x.h|  24 ++
>  5 files changed, 480 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
>  create mode 100644 drivers/leds/leds-is31fl319x.c
>  create mode 100644 include/linux/leds-is31fl319x.h
> 
> diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
> b/Documentation/devicetree/bindings/leds/is31fl319x.txt
> new file mode 100644
> index 000..d13c7ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
> @@ -0,0 +1,41 @@
> +LEDs connected to is31fl319x RGB LED controller chip
> +
> +Required properties:
> +- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".

>From a quick look at the datasheets, I believe the Si-En SN3199 [1] 
is the same as the ISSI IS31FL3199. ISSI purchased Si-En in 2011,
and looks to have rebranded Si-En's existing LED controllers. 
Assuming the SN3199 is indeed the same as the IS31FL3199, I would
suggest adding a compatible string of "si-en,sn3199" both here
and in the driver itself (and update the Kconfig text as well). 

[1] http://www.si-en.com/product.asp?parentid=890

> +- #address-cells: must be 1
> +- #size-cells: must be 0
> +- reg: 0x64, 0x65, 0x66, 0x67.
> +
> +Optional properties:
> +- max-current-ma:maximum led current (5..40 mA, default 20 mA)
> +- audio-gain-db: audio gain selection (0..21 dB. default 0 dB)
> +- breathing & audio: not implemented

I'm not certain, but that last line feels wrong to me. I would expect
to see just defined properties in this section, while that is more
commentary. I would just leave it out, myself.

> +
> +Each led is represented as a sub-node of the issi,is31fl319x device.
> +
> +LED sub-node properties:
> +- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
> +- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)

Since the datasheets for these devices name the outputs "OUT1" through 
"OUT6"/"OUT9", I believe the correct range for the 'reg' property should 
be 1-6 and 1-9. At least that was the result of a discussion with Rob
Herring [2], and therefore how the IS31FL32xx binding/driver was done.
It also makes devicetrees easier to write relative to schematics which 
likely use the OUT1-N naming.

[2] http://www.spinics.net/lists/devicetree/msg116019.html


> +- linux,default-trigger : (optional)
> +   see Documentation/devicetree/bindings/leds/common.txt
> +
> +Examples:
> +
> +fancy_leds: is31fl3196@65 {

I believe the current preference would be to name the node by
its function, rather than the device. For example:
fancy_leds: leds@65

> + compatible = "issi,is31fl319x";

I believe this should be "issi,is31fl3196" here.

> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x65>;
> +
> + led0: red-aux@0 {
> + label = "red:aux";
> + reg = <0>;
> + };
> +
> + led1: green-power@4 {
> + label = "green:power";
> + reg = <4>;
> + linux,default-trigger = "default-on";
> + };
> +};
> +
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 2251478..f099bcb 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -491,6 +491,14 @@ config LEDS_ASIC3
> cannot be used. This driver supports hardware blinking with an on+off
> period from 62ms to 125s. Say Y to enable LEDs on the HP iPAQ hx4700.
>  
> +config LEDS_IS31FL319X
> + tristate "LED Support for 

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[David Rivshin (Allworx)]

Hi Nikolaus,

I recently submitted a driver for the IS31FL32xx family of devices, so
this driver caught my eye. I have a few comments below.

On Mon, 18 Apr 2016 20:43:16 +0200
"H. Nikolaus Schaller"  wrote:

> This is a driver for the Integrated Silicon Solution Inc. LED driver
> chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
> LEDs.
> 
> Each LED is individually controllable in brightness (through pwm)
> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
> 
> The maximum current of the LEDs can be programmed and limited to
> 5 .. 40mA through a device tree property.
> 
> The chip is connected through I2C and can have one of 4 addresses (0x64 .. 
> 0x67)
> depending on how the AD pin is connected. The address is defined by the
> reg property as usual.
> 
> The chip also has a shutdown input which could be connected to a GPIO,
> but this driver uses software shutdown if all LEDs are inactivated.
> 
> The chip also has breathing and audio features which are not supported
> by this driver.
> 
> This driver was developed and tested on OMAP5 based Pyra handheld prototype.
> 
> Signed-off-by: H. Nikolaus Schaller 
> ---
>  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
>  drivers/leds/Kconfig   |   8 +
>  drivers/leds/Makefile  |   1 +
>  drivers/leds/leds-is31fl319x.c | 406 
> +
>  include/linux/leds-is31fl319x.h|  24 ++
>  5 files changed, 480 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
>  create mode 100644 drivers/leds/leds-is31fl319x.c
>  create mode 100644 include/linux/leds-is31fl319x.h
> 
> diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
> b/Documentation/devicetree/bindings/leds/is31fl319x.txt
> new file mode 100644
> index 000..d13c7ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
> @@ -0,0 +1,41 @@
> +LEDs connected to is31fl319x RGB LED controller chip
> +
> +Required properties:
> +- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".

>From a quick look at the datasheets, I believe the Si-En SN3199 [1] 
is the same as the ISSI IS31FL3199. ISSI purchased Si-En in 2011,
and looks to have rebranded Si-En's existing LED controllers. 
Assuming the SN3199 is indeed the same as the IS31FL3199, I would
suggest adding a compatible string of "si-en,sn3199" both here
and in the driver itself (and update the Kconfig text as well). 

[1] http://www.si-en.com/product.asp?parentid=890

> +- #address-cells: must be 1
> +- #size-cells: must be 0
> +- reg: 0x64, 0x65, 0x66, 0x67.
> +
> +Optional properties:
> +- max-current-ma:maximum led current (5..40 mA, default 20 mA)
> +- audio-gain-db: audio gain selection (0..21 dB. default 0 dB)
> +- breathing & audio: not implemented

I'm not certain, but that last line feels wrong to me. I would expect
to see just defined properties in this section, while that is more
commentary. I would just leave it out, myself.

> +
> +Each led is represented as a sub-node of the issi,is31fl319x device.
> +
> +LED sub-node properties:
> +- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
> +- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)

Since the datasheets for these devices name the outputs "OUT1" through 
"OUT6"/"OUT9", I believe the correct range for the 'reg' property should 
be 1-6 and 1-9. At least that was the result of a discussion with Rob
Herring [2], and therefore how the IS31FL32xx binding/driver was done.
It also makes devicetrees easier to write relative to schematics which 
likely use the OUT1-N naming.

[2] http://www.spinics.net/lists/devicetree/msg116019.html


> +- linux,default-trigger : (optional)
> +   see Documentation/devicetree/bindings/leds/common.txt
> +
> +Examples:
> +
> +fancy_leds: is31fl3196@65 {

I believe the current preference would be to name the node by
its function, rather than the device. For example:
fancy_leds: leds@65

> + compatible = "issi,is31fl319x";

I believe this should be "issi,is31fl3196" here.

> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x65>;
> +
> + led0: red-aux@0 {
> + label = "red:aux";
> + reg = <0>;
> + };
> +
> + led1: green-power@4 {
> + label = "green:power";
> + reg = <4>;
> + linux,default-trigger = "default-on";
> + };
> +};
> +
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 2251478..f099bcb 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -491,6 +491,14 @@ config LEDS_ASIC3
> cannot be used. This driver supports hardware blinking with an on+off
> period from 62ms to 125s. Say Y to enable LEDs on the HP iPAQ hx4700.
>  
> +config LEDS_IS31FL319X
> + tristate "LED Support for IS31FL319x I2C chip"

Pedantic: "chips" 

Re: [PATCH v2 0/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

On Tue, 08 Mar 2016 10:43:35 +0100
Jacek Anaszewski <j.anaszew...@samsung.com> wrote:

> Hi David,
> 
> Thanks for posting the final version. Very nice driver.
> Applied whole series, after dropping leds-sn3218 with its
> DT bindings. Stefan, thanks for testing the driver on the
> Si-En hardware.

Thanks Jacek. And thanks also for your patience with all my 
questions along the way.

> 
> Thanks,
> Jacek Anaszewski
> 
> On 03/08/2016 01:57 AM, David Rivshin (Allworx) wrote:
> > From: David Rivshin <drivs...@allworx.com>
> >
> > This series adds support for the ISSI IS31FL32xx family of I2C LED
> > controllers. Since the IS31FL3218/3216 are actually the same devices as
> > the SN3218/3216, adds their compatible strings as aliases.
> >
> > As requested, this series is based on the current linux-leds/for-next, minus
> > patch 2 and 3 of the standalone sn3218 driver. As such, it will not directly
> > apply to the current linux-next without minor merges in the leds Kconfig
> > and Makefile. This will apply cleanly to v4.5-rc7, albeit with a compiler
> > warning in patch 3 (which is fixed elsewhere in linux-next).
> >
> > Changes from v1 [1]:
> >   - swapped node name and label in binding example
> >   - removed line stating filename from file header comment of is31fl32xx.c
> >   - dropped #includes for err.h and of_platform.h
> >   - added #includes for device.h, of.h, and of_device.h
> >   - patch 4 no longer removes leds-sn3218, as that will be done separately
> >   - patch 4 commit log no longer references leds-sn3218 driver
> >   - added Rob's acks for patches 1, 2, and 4
> >   - added Tested-By from Stefan (for SN3218)
> >
> > Changes from RFC [2]:
> >   - Removed max-brightness DT property.
> >   - Added #address-cells and #size-cells properties to the example DT.
> >   - Refer to these devices as "LED controllers" in Kconfig.
> >   - Removed redundant last sentence from Kconfig entry
> >   - Removed unnecessary debug code.
> >   - Do not set led_classdev.brightness to 0 explicitly, as it is
> > already initialized to 0 by devm_kzalloc().
> >   - Used of_property_read_string() instead of of_get_property().
> >   - Fail immediately on DT parsing error in a child node, rather than
> > continuing on with the non-faulty ones.
> >   - Added additional comments for some things that might be non-obvious.
> >   - Added constants for the location of the SSD bit in the SHUTDOWN
> > register, and the 3216's CONFIG register.
> >   - Added special sw_shutdown_func for the 3216 device, as that bit
> > is in a different register, at a different position, and has reverse
> > polarity compared to all the other devices.
> >   - Refactored is31fl32xx_init_regs() to separate out some logic into
> > is31fl32xx_reset_regs() and is31fl32xx_software_shutdown().
> >   - Added 4th patch to replace the now-redundant leds-sn3218.
> >
> > [1] https://lkml.org/lkml/2016/3/2/1004
> > [2] http://www.spinics.net/lists/linux-leds/msg05564.html
> >
> > David Rivshin (4):
> >DT: Add vendor prefix for Integrated Silicon Solutions Inc.
> >DT: leds: Add binding for the ISSI IS31FL32xx family of LED
> >  controllers
> >leds: Add driver for the ISSI IS31FL32xx family of LED controllers
> >leds: Add SN3218 and SN3216 support to the IS31FL32XX driver
> >
> >   .../devicetree/bindings/leds/leds-is31fl32xx.txt   |  52 +++
> >   .../devicetree/bindings/vendor-prefixes.txt|   1 +
> >   drivers/leds/Kconfig   |   8 +
> >   drivers/leds/Makefile  |   1 +
> >   drivers/leds/leds-is31fl32xx.c | 508 
> > +
> >   5 files changed, 570 insertions(+)
> >   create mode 100644 
> > Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> >   create mode 100644 drivers/leds/leds-is31fl32xx.c
> >  

Re: [PATCH v2 0/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

On Tue, 08 Mar 2016 10:43:35 +0100
Jacek Anaszewski  wrote:

> Hi David,
> 
> Thanks for posting the final version. Very nice driver.
> Applied whole series, after dropping leds-sn3218 with its
> DT bindings. Stefan, thanks for testing the driver on the
> Si-En hardware.

Thanks Jacek. And thanks also for your patience with all my 
questions along the way.

> 
> Thanks,
> Jacek Anaszewski
> 
> On 03/08/2016 01:57 AM, David Rivshin (Allworx) wrote:
> > From: David Rivshin 
> >
> > This series adds support for the ISSI IS31FL32xx family of I2C LED
> > controllers. Since the IS31FL3218/3216 are actually the same devices as
> > the SN3218/3216, adds their compatible strings as aliases.
> >
> > As requested, this series is based on the current linux-leds/for-next, minus
> > patch 2 and 3 of the standalone sn3218 driver. As such, it will not directly
> > apply to the current linux-next without minor merges in the leds Kconfig
> > and Makefile. This will apply cleanly to v4.5-rc7, albeit with a compiler
> > warning in patch 3 (which is fixed elsewhere in linux-next).
> >
> > Changes from v1 [1]:
> >   - swapped node name and label in binding example
> >   - removed line stating filename from file header comment of is31fl32xx.c
> >   - dropped #includes for err.h and of_platform.h
> >   - added #includes for device.h, of.h, and of_device.h
> >   - patch 4 no longer removes leds-sn3218, as that will be done separately
> >   - patch 4 commit log no longer references leds-sn3218 driver
> >   - added Rob's acks for patches 1, 2, and 4
> >   - added Tested-By from Stefan (for SN3218)
> >
> > Changes from RFC [2]:
> >   - Removed max-brightness DT property.
> >   - Added #address-cells and #size-cells properties to the example DT.
> >   - Refer to these devices as "LED controllers" in Kconfig.
> >   - Removed redundant last sentence from Kconfig entry
> >   - Removed unnecessary debug code.
> >   - Do not set led_classdev.brightness to 0 explicitly, as it is
> > already initialized to 0 by devm_kzalloc().
> >   - Used of_property_read_string() instead of of_get_property().
> >   - Fail immediately on DT parsing error in a child node, rather than
> > continuing on with the non-faulty ones.
> >   - Added additional comments for some things that might be non-obvious.
> >   - Added constants for the location of the SSD bit in the SHUTDOWN
> > register, and the 3216's CONFIG register.
> >   - Added special sw_shutdown_func for the 3216 device, as that bit
> > is in a different register, at a different position, and has reverse
> > polarity compared to all the other devices.
> >   - Refactored is31fl32xx_init_regs() to separate out some logic into
> > is31fl32xx_reset_regs() and is31fl32xx_software_shutdown().
> >   - Added 4th patch to replace the now-redundant leds-sn3218.
> >
> > [1] https://lkml.org/lkml/2016/3/2/1004
> > [2] http://www.spinics.net/lists/linux-leds/msg05564.html
> >
> > David Rivshin (4):
> >DT: Add vendor prefix for Integrated Silicon Solutions Inc.
> >DT: leds: Add binding for the ISSI IS31FL32xx family of LED
> >  controllers
> >leds: Add driver for the ISSI IS31FL32xx family of LED controllers
> >leds: Add SN3218 and SN3216 support to the IS31FL32XX driver
> >
> >   .../devicetree/bindings/leds/leds-is31fl32xx.txt   |  52 +++
> >   .../devicetree/bindings/vendor-prefixes.txt|   1 +
> >   drivers/leds/Kconfig   |   8 +
> >   drivers/leds/Makefile  |   1 +
> >   drivers/leds/leds-is31fl32xx.c | 508 
> > +
> >   5 files changed, 570 insertions(+)
> >   create mode 100644 
> > Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> >   create mode 100644 drivers/leds/leds-is31fl32xx.c
> >  

[PATCH v2 2/4] DT: leds: Add binding for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

From: David Rivshin 

This adds a binding description for the is31fl3236/35/18/16 I2C LED
controllers.

Signed-off-by: David Rivshin 
Acked-by: Rob Herring 
---

Changes from v1:
 - swapped node name and label in example
 - added Rob's ack (given above fix) [1]

Changes from RFC:
 - Removed max-brightness property.
 - Added #address-cells and #size-cells properties to the example.

[1] https://lkml.org/lkml/2016/3/4/959

 .../devicetree/bindings/leds/leds-is31fl32xx.txt   | 49 ++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt 
b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
new file mode 100644
index 000..e72ed66
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
@@ -0,0 +1,49 @@
+Binding for ISSI IS31FL32xx LED Drivers
+
+The IS31FL32xx family of LED drivers are I2C devices with multiple
+constant-current channels, each with independent 256-level PWM control.
+Each LED is represented as a sub-node of the device.
+
+Required properties:
+- compatible: one of
+   issi,is31fl3236
+   issi,is31fl3235
+   issi,is31fl3218
+   issi,is31fl3216
+- reg: I2C slave address
+- address-cells : must be 1
+- size-cells : must be 0
+
+LED sub-node properties:
+- reg : LED channel number (1..N)
+- label :  (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger :  (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+
+
+Example:
+
+is31fl3236: led-controller@3c {
+   compatible = "issi,is31fl3236";
+   reg = <0x3c>;
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   led@1 {
+   reg = <1>;
+   label = "EB:blue:usr0";
+   };
+   led@2 {
+   reg = <2>;
+   label = "EB:blue:usr1";
+   };
+   ...
+   led@36 {
+   reg = <36>;
+   label = "EB:blue:usr35";
+   };
+};
+
+For more product information please see the link below:
+http://www.issi.com/US/product-analog-fxled-driver.shtml
-- 
2.5.0

[PATCH v2 4/4] leds: Add SN3218 and SN3216 support to the IS31FL32XX driver

[David Rivshin (Allworx)]

From: David Rivshin 

Si-En Technology was acquired by ISSI in 2011, and it appears that
the IS31FL3218/IS31FL3216 are just rebranded SN3218/SN3216 devices.

Add the "si-en,sn3218" and "si-en,sn3216" compatible strings into the
IS31FL32XX driver as aliases for the issi equivalents, and update
binding documentation.

Datasheets:
IS31FL3218: http://www.issi.com/WW/pdf/31FL3218.pdf
SN3218: http://www.si-en.com/uploadpdf/s2011517171720.pdf

IS31FL3216: http://www.issi.com/WW/pdf/31FL3216.pdf
SN3216: http://www.si-en.com/uploadpdf/SN3216201152410148.pdf

Signed-off-by: David Rivshin 
Acked-by: Rob Herring 
Tested-by: Stefan Wahren 
---

Changes from v1:
 - no longer removes leds-sn3218, as that will be done separately
 - commit log modified to remove references to leds-sn3218 driver
 - added Rob's ack (for binding change) [1]
 - added Tested-By from Stefan (for SN3218) [2]

Changes from RFC:
 new patch

[1] https://lkml.org/lkml/2016/3/4/960
[2] https://lkml.org/lkml/2016/3/4/773

 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt | 9 ++---
 drivers/leds/Kconfig   | 6 +++---
 drivers/leds/leds-is31fl32xx.c | 6 +-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt 
b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
index e72ed66..926c211 100644
--- a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
+++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
@@ -1,6 +1,6 @@
-Binding for ISSI IS31FL32xx LED Drivers
+Binding for ISSI IS31FL32xx and Si-En SN32xx LED Drivers
 
-The IS31FL32xx family of LED drivers are I2C devices with multiple
+The IS31FL32xx/SN32xx family of LED drivers are I2C devices with multiple
 constant-current channels, each with independent 256-level PWM control.
 Each LED is represented as a sub-node of the device.
 
@@ -10,6 +10,8 @@ Required properties:
issi,is31fl3235
issi,is31fl3218
issi,is31fl3216
+   si-en,sn3218
+   si-en,sn3216
 - reg: I2C slave address
 - address-cells : must be 1
 - size-cells : must be 0
@@ -45,5 +47,6 @@ is31fl3236: led-controller@3c {
};
 };
 
-For more product information please see the link below:
+For more product information please see the links below:
 http://www.issi.com/US/product-analog-fxled-driver.shtml
+http://www.si-en.com/product.asp?parentid=890
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 08a5743..1f64151 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -572,9 +572,9 @@ config LEDS_IS31FL32XX
tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
depends on LEDS_CLASS && I2C && OF
help
- Say Y here to include support for ISSI IS31FL32XX LED controllers.
- They are I2C devices with multiple constant-current channels, each
- with independent 256-level PWM control.
+ Say Y here to include support for ISSI IS31FL32XX and Si-En SN32xx
+ LED controllers. They are I2C devices with multiple constant-current
+ channels, each with independent 256-level PWM control.
 
 comment "LED driver for blink(1) USB RGB LED is under Special HID drivers 
(HID_THINGM)"
 
diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
index 9a67856..c901d13 100644
--- a/drivers/leds/leds-is31fl32xx.c
+++ b/drivers/leds/leds-is31fl32xx.c
@@ -8,7 +8,9 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  *
- * Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
+ * Datasheets:
+ *   http://www.issi.com/US/product-analog-fxled-driver.shtml
+ *   http://www.si-en.com/product.asp?parentid=890
  */
 
 #include 
@@ -424,7 +426,9 @@ static const struct of_device_id of_is31fl31xx_match[] = {
{ .compatible = "issi,is31fl3236", .data = _cdef, },
{ .compatible = "issi,is31fl3235", .data = _cdef, },
{ .compatible = "issi,is31fl3218", .data = _cdef, },
+   { .compatible = "si-en,sn3218",.data = _cdef, },
{ .compatible = "issi,is31fl3216", .data = _cdef, },
+   { .compatible = "si-en,sn3216",.data = _cdef, },
{},
 };
 
-- 
2.5.0

[PATCH v2 2/4] DT: leds: Add binding for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

From: David Rivshin 

This adds a binding description for the is31fl3236/35/18/16 I2C LED
controllers.

Signed-off-by: David Rivshin 
Acked-by: Rob Herring 
---

Changes from v1:
 - swapped node name and label in example
 - added Rob's ack (given above fix) [1]

Changes from RFC:
 - Removed max-brightness property.
 - Added #address-cells and #size-cells properties to the example.

[1] https://lkml.org/lkml/2016/3/4/959

 .../devicetree/bindings/leds/leds-is31fl32xx.txt   | 49 ++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt 
b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
new file mode 100644
index 000..e72ed66
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
@@ -0,0 +1,49 @@
+Binding for ISSI IS31FL32xx LED Drivers
+
+The IS31FL32xx family of LED drivers are I2C devices with multiple
+constant-current channels, each with independent 256-level PWM control.
+Each LED is represented as a sub-node of the device.
+
+Required properties:
+- compatible: one of
+   issi,is31fl3236
+   issi,is31fl3235
+   issi,is31fl3218
+   issi,is31fl3216
+- reg: I2C slave address
+- address-cells : must be 1
+- size-cells : must be 0
+
+LED sub-node properties:
+- reg : LED channel number (1..N)
+- label :  (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger :  (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+
+
+Example:
+
+is31fl3236: led-controller@3c {
+   compatible = "issi,is31fl3236";
+   reg = <0x3c>;
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   led@1 {
+   reg = <1>;
+   label = "EB:blue:usr0";
+   };
+   led@2 {
+   reg = <2>;
+   label = "EB:blue:usr1";
+   };
+   ...
+   led@36 {
+   reg = <36>;
+   label = "EB:blue:usr35";
+   };
+};
+
+For more product information please see the link below:
+http://www.issi.com/US/product-analog-fxled-driver.shtml
-- 
2.5.0

[PATCH v2 4/4] leds: Add SN3218 and SN3216 support to the IS31FL32XX driver

[David Rivshin (Allworx)]

From: David Rivshin 

Si-En Technology was acquired by ISSI in 2011, and it appears that
the IS31FL3218/IS31FL3216 are just rebranded SN3218/SN3216 devices.

Add the "si-en,sn3218" and "si-en,sn3216" compatible strings into the
IS31FL32XX driver as aliases for the issi equivalents, and update
binding documentation.

Datasheets:
IS31FL3218: http://www.issi.com/WW/pdf/31FL3218.pdf
SN3218: http://www.si-en.com/uploadpdf/s2011517171720.pdf

IS31FL3216: http://www.issi.com/WW/pdf/31FL3216.pdf
SN3216: http://www.si-en.com/uploadpdf/SN3216201152410148.pdf

Signed-off-by: David Rivshin 
Acked-by: Rob Herring 
Tested-by: Stefan Wahren 
---

Changes from v1:
 - no longer removes leds-sn3218, as that will be done separately
 - commit log modified to remove references to leds-sn3218 driver
 - added Rob's ack (for binding change) [1]
 - added Tested-By from Stefan (for SN3218) [2]

Changes from RFC:
 new patch

[1] https://lkml.org/lkml/2016/3/4/960
[2] https://lkml.org/lkml/2016/3/4/773

 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt | 9 ++---
 drivers/leds/Kconfig   | 6 +++---
 drivers/leds/leds-is31fl32xx.c | 6 +-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt 
b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
index e72ed66..926c211 100644
--- a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
+++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
@@ -1,6 +1,6 @@
-Binding for ISSI IS31FL32xx LED Drivers
+Binding for ISSI IS31FL32xx and Si-En SN32xx LED Drivers
 
-The IS31FL32xx family of LED drivers are I2C devices with multiple
+The IS31FL32xx/SN32xx family of LED drivers are I2C devices with multiple
 constant-current channels, each with independent 256-level PWM control.
 Each LED is represented as a sub-node of the device.
 
@@ -10,6 +10,8 @@ Required properties:
issi,is31fl3235
issi,is31fl3218
issi,is31fl3216
+   si-en,sn3218
+   si-en,sn3216
 - reg: I2C slave address
 - address-cells : must be 1
 - size-cells : must be 0
@@ -45,5 +47,6 @@ is31fl3236: led-controller@3c {
};
 };
 
-For more product information please see the link below:
+For more product information please see the links below:
 http://www.issi.com/US/product-analog-fxled-driver.shtml
+http://www.si-en.com/product.asp?parentid=890
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 08a5743..1f64151 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -572,9 +572,9 @@ config LEDS_IS31FL32XX
tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
depends on LEDS_CLASS && I2C && OF
help
- Say Y here to include support for ISSI IS31FL32XX LED controllers.
- They are I2C devices with multiple constant-current channels, each
- with independent 256-level PWM control.
+ Say Y here to include support for ISSI IS31FL32XX and Si-En SN32xx
+ LED controllers. They are I2C devices with multiple constant-current
+ channels, each with independent 256-level PWM control.
 
 comment "LED driver for blink(1) USB RGB LED is under Special HID drivers 
(HID_THINGM)"
 
diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
index 9a67856..c901d13 100644
--- a/drivers/leds/leds-is31fl32xx.c
+++ b/drivers/leds/leds-is31fl32xx.c
@@ -8,7 +8,9 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  *
- * Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
+ * Datasheets:
+ *   http://www.issi.com/US/product-analog-fxled-driver.shtml
+ *   http://www.si-en.com/product.asp?parentid=890
  */
 
 #include 
@@ -424,7 +426,9 @@ static const struct of_device_id of_is31fl31xx_match[] = {
{ .compatible = "issi,is31fl3236", .data = _cdef, },
{ .compatible = "issi,is31fl3235", .data = _cdef, },
{ .compatible = "issi,is31fl3218", .data = _cdef, },
+   { .compatible = "si-en,sn3218",.data = _cdef, },
{ .compatible = "issi,is31fl3216", .data = _cdef, },
+   { .compatible = "si-en,sn3216",.data = _cdef, },
{},
 };
 
-- 
2.5.0

[PATCH v2 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

From: David Rivshin 

The IS31FL32xx family of LED controllers are I2C devices with multiple
constant-current channels, each with independent 256-level PWM control.

Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml

This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
(TI am335x) platform.

The programming paradigm of these devices is similar in the following
ways:
 - All registers are 8 bit
 - All LED control registers are write-only
 - Each LED channel has a PWM register (0-255)
 - PWM register writes are shadowed until an Update register is poked
 - All have a concept of Software Shutdown, which disables output

However, there are some differences in devices:
 - 3236/3235 have a separate Control register for each LED,
   (3218/3216 pack the enable bits into fewer registers)
 - 3236/3235 have a per-channel current divisor setting
 - 3236/3235 have a Global Control register that can turn off all LEDs
 - 3216 is unique in a number of ways
- OUT9-OUT16 can be configured as GPIOs instead of LED controls
- LEDs can be programmed with an 8-frame animation, with
  programmable delay between frames
- LEDs can be modulated by an input audio signal
- Max output current can be adjusted from 1/4 to 2x globally
- Has a Configuration register instead of a Shutdown register

This driver currently only supports the base PWM control function
of these devices. The following features of these devices are not
implemented, although it should be possible to add them in the future:
 - All devices are capable of going into a lower-power "software
   shutdown" mode.
 - The is31fl3236 and is31fl3235 can reduce the max output current
   per-channel with a divisor of 1, 2, 3, or 4.
 - The is31fl3216 can use some LED channels as GPIOs instead.
 - The is31fl3216 can animate LEDs in hardware.
 - The is31fl3216 can modulate LEDs according to an audio input.
 - The is31fl3216 can reduce/increase max output current globally.

Signed-off-by: David Rivshin 
---

You may see two instances of this warning:
  "passing argument 1 of 'of_property_read_string' discards 'const'
   qualifier from pointer target type"
That is a result of of_property_read_string() taking a non-const
struct device_node pointer parameter. A fix for this [1] is already
in linux-next.

Changes from v1:
 - removed line stating filename from file header comment of is31fl32xx.c
 - dropped #includes for err.h and of_platform.h
 - added #includes for device.h, of.h, and of_device.h

Changes from RFC:
 - Removed max-brightness DT property.
 - Refer to these devices as "LED controllers" in Kconfig.
 - Removed redundant last sentence from Kconfig entry
 - Removed unnecessary debug code.
 - Do not set led_classdev.brightness to 0 explicitly, as it is
   already initialized to 0 by devm_kzalloc().
 - Used of_property_read_string() instead of of_get_property().
 - Fail immediately on DT parsing error in a child node, rather than
   continuing on with the non-faulty ones.
 - Added additional comments for some things that might be non-obvious.
 - Added constants for the location of the SSD bit in the SHUTDOWN
   register, and the 3216's CONFIG register.
 - Added special sw_shutdown_func for the 3216 device, as that bit
   is in a different register, at a different position, and has reverse
   polarity compared to all the other devices.
 - Refactored is31fl32xx_init_regs() to separate out some logic into
   is31fl32xx_reset_regs() and is31fl32xx_software_shutdown().

[1] https://lkml.org/lkml/2016/3/2/746

 drivers/leds/Kconfig   |   8 +
 drivers/leds/Makefile  |   1 +
 drivers/leds/leds-is31fl32xx.c | 504 +
 3 files changed, 513 insertions(+)
 create mode 100644 drivers/leds/leds-is31fl32xx.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 7f940c2..08a5743 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -568,6 +568,14 @@ config LEDS_SEAD3
  This driver can also be built as a module. If so the module
  will be called leds-sead3.
 
+config LEDS_IS31FL32XX
+   tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
+   depends on LEDS_CLASS && I2C && OF
+   help
+ Say Y here to include support for ISSI IS31FL32XX LED controllers.
+ They are I2C devices with multiple constant-current channels, each
+ with independent 256-level PWM control.
+
 comment "LED driver for blink(1) USB RGB LED is under Special HID drivers 
(HID_THINGM)"
 
 config LEDS_BLINKM
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index e9d53092..cb2013d 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC)  += leds-menf21bmc.o
 obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o
 obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
 obj-$(CONFIG_LEDS_SEAD3)   += leds-sead3.o

[PATCH v2 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

From: David Rivshin 

The IS31FL32xx family of LED controllers are I2C devices with multiple
constant-current channels, each with independent 256-level PWM control.

Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml

This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
(TI am335x) platform.

The programming paradigm of these devices is similar in the following
ways:
 - All registers are 8 bit
 - All LED control registers are write-only
 - Each LED channel has a PWM register (0-255)
 - PWM register writes are shadowed until an Update register is poked
 - All have a concept of Software Shutdown, which disables output

However, there are some differences in devices:
 - 3236/3235 have a separate Control register for each LED,
   (3218/3216 pack the enable bits into fewer registers)
 - 3236/3235 have a per-channel current divisor setting
 - 3236/3235 have a Global Control register that can turn off all LEDs
 - 3216 is unique in a number of ways
- OUT9-OUT16 can be configured as GPIOs instead of LED controls
- LEDs can be programmed with an 8-frame animation, with
  programmable delay between frames
- LEDs can be modulated by an input audio signal
- Max output current can be adjusted from 1/4 to 2x globally
- Has a Configuration register instead of a Shutdown register

This driver currently only supports the base PWM control function
of these devices. The following features of these devices are not
implemented, although it should be possible to add them in the future:
 - All devices are capable of going into a lower-power "software
   shutdown" mode.
 - The is31fl3236 and is31fl3235 can reduce the max output current
   per-channel with a divisor of 1, 2, 3, or 4.
 - The is31fl3216 can use some LED channels as GPIOs instead.
 - The is31fl3216 can animate LEDs in hardware.
 - The is31fl3216 can modulate LEDs according to an audio input.
 - The is31fl3216 can reduce/increase max output current globally.

Signed-off-by: David Rivshin 
---

You may see two instances of this warning:
  "passing argument 1 of 'of_property_read_string' discards 'const'
   qualifier from pointer target type"
That is a result of of_property_read_string() taking a non-const
struct device_node pointer parameter. A fix for this [1] is already
in linux-next.

Changes from v1:
 - removed line stating filename from file header comment of is31fl32xx.c
 - dropped #includes for err.h and of_platform.h
 - added #includes for device.h, of.h, and of_device.h

Changes from RFC:
 - Removed max-brightness DT property.
 - Refer to these devices as "LED controllers" in Kconfig.
 - Removed redundant last sentence from Kconfig entry
 - Removed unnecessary debug code.
 - Do not set led_classdev.brightness to 0 explicitly, as it is
   already initialized to 0 by devm_kzalloc().
 - Used of_property_read_string() instead of of_get_property().
 - Fail immediately on DT parsing error in a child node, rather than
   continuing on with the non-faulty ones.
 - Added additional comments for some things that might be non-obvious.
 - Added constants for the location of the SSD bit in the SHUTDOWN
   register, and the 3216's CONFIG register.
 - Added special sw_shutdown_func for the 3216 device, as that bit
   is in a different register, at a different position, and has reverse
   polarity compared to all the other devices.
 - Refactored is31fl32xx_init_regs() to separate out some logic into
   is31fl32xx_reset_regs() and is31fl32xx_software_shutdown().

[1] https://lkml.org/lkml/2016/3/2/746

 drivers/leds/Kconfig   |   8 +
 drivers/leds/Makefile  |   1 +
 drivers/leds/leds-is31fl32xx.c | 504 +
 3 files changed, 513 insertions(+)
 create mode 100644 drivers/leds/leds-is31fl32xx.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 7f940c2..08a5743 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -568,6 +568,14 @@ config LEDS_SEAD3
  This driver can also be built as a module. If so the module
  will be called leds-sead3.
 
+config LEDS_IS31FL32XX
+   tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
+   depends on LEDS_CLASS && I2C && OF
+   help
+ Say Y here to include support for ISSI IS31FL32XX LED controllers.
+ They are I2C devices with multiple constant-current channels, each
+ with independent 256-level PWM control.
+
 comment "LED driver for blink(1) USB RGB LED is under Special HID drivers 
(HID_THINGM)"
 
 config LEDS_BLINKM
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index e9d53092..cb2013d 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC)  += leds-menf21bmc.o
 obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o
 obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
 obj-$(CONFIG_LEDS_SEAD3)   += leds-sead3.o
+obj-$(CONFIG_LEDS_IS31FL32XX)  += 

[PATCH v2 1/4] DT: Add vendor prefix for Integrated Silicon Solutions Inc.

[David Rivshin (Allworx)]

From: David Rivshin 

ISSI is the stock ticker Integrated Silicon Solutions Inc.
Company website: http://www.issi.com

Signed-off-by: David Rivshin 
Acked-by: Rob Herring 
---

Changes from v1:
 - added Rob's ack [1]

Changes from RFC:
 none

[1] https://lkml.org/lkml/2016/3/4/961

 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 52f22f9..dd72e05 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -120,6 +120,7 @@ intercontrolInter Control Group
 invensense InvenSense Inc.
 isee   ISEE 2007 S.L.
 isil   Intersil
+issi   Integrated Silicon Solutions Inc.
 jedec  JEDEC Solid State Technology Association
 karo   Ka-Ro electronics GmbH
 keymileKeymile GmbH
-- 
2.5.0

[PATCH v2 0/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

From: David Rivshin 

This series adds support for the ISSI IS31FL32xx family of I2C LED
controllers. Since the IS31FL3218/3216 are actually the same devices as
the SN3218/3216, adds their compatible strings as aliases.

As requested, this series is based on the current linux-leds/for-next, minus
patch 2 and 3 of the standalone sn3218 driver. As such, it will not directly
apply to the current linux-next without minor merges in the leds Kconfig
and Makefile. This will apply cleanly to v4.5-rc7, albeit with a compiler
warning in patch 3 (which is fixed elsewhere in linux-next).

Changes from v1 [1]:
 - swapped node name and label in binding example
 - removed line stating filename from file header comment of is31fl32xx.c
 - dropped #includes for err.h and of_platform.h
 - added #includes for device.h, of.h, and of_device.h
 - patch 4 no longer removes leds-sn3218, as that will be done separately
 - patch 4 commit log no longer references leds-sn3218 driver
 - added Rob's acks for patches 1, 2, and 4
 - added Tested-By from Stefan (for SN3218)

Changes from RFC [2]:
 - Removed max-brightness DT property.
 - Added #address-cells and #size-cells properties to the example DT.
 - Refer to these devices as "LED controllers" in Kconfig.
 - Removed redundant last sentence from Kconfig entry
 - Removed unnecessary debug code.
 - Do not set led_classdev.brightness to 0 explicitly, as it is
   already initialized to 0 by devm_kzalloc().
 - Used of_property_read_string() instead of of_get_property().
 - Fail immediately on DT parsing error in a child node, rather than
   continuing on with the non-faulty ones.
 - Added additional comments for some things that might be non-obvious.
 - Added constants for the location of the SSD bit in the SHUTDOWN
   register, and the 3216's CONFIG register.
 - Added special sw_shutdown_func for the 3216 device, as that bit
   is in a different register, at a different position, and has reverse
   polarity compared to all the other devices.
 - Refactored is31fl32xx_init_regs() to separate out some logic into
   is31fl32xx_reset_regs() and is31fl32xx_software_shutdown().
 - Added 4th patch to replace the now-redundant leds-sn3218.

[1] https://lkml.org/lkml/2016/3/2/1004
[2] http://www.spinics.net/lists/linux-leds/msg05564.html

David Rivshin (4):
  DT: Add vendor prefix for Integrated Silicon Solutions Inc.
  DT: leds: Add binding for the ISSI IS31FL32xx family of LED
controllers
  leds: Add driver for the ISSI IS31FL32xx family of LED controllers
  leds: Add SN3218 and SN3216 support to the IS31FL32XX driver

 .../devicetree/bindings/leds/leds-is31fl32xx.txt   |  52 +++
 .../devicetree/bindings/vendor-prefixes.txt|   1 +
 drivers/leds/Kconfig   |   8 +
 drivers/leds/Makefile  |   1 +
 drivers/leds/leds-is31fl32xx.c | 508 +
 5 files changed, 570 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
 create mode 100644 drivers/leds/leds-is31fl32xx.c

-- 
2.5.0

[PATCH v2 1/4] DT: Add vendor prefix for Integrated Silicon Solutions Inc.

[David Rivshin (Allworx)]

From: David Rivshin 

ISSI is the stock ticker Integrated Silicon Solutions Inc.
Company website: http://www.issi.com

Signed-off-by: David Rivshin 
Acked-by: Rob Herring 
---

Changes from v1:
 - added Rob's ack [1]

Changes from RFC:
 none

[1] https://lkml.org/lkml/2016/3/4/961

 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 52f22f9..dd72e05 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -120,6 +120,7 @@ intercontrolInter Control Group
 invensense InvenSense Inc.
 isee   ISEE 2007 S.L.
 isil   Intersil
+issi   Integrated Silicon Solutions Inc.
 jedec  JEDEC Solid State Technology Association
 karo   Ka-Ro electronics GmbH
 keymileKeymile GmbH
-- 
2.5.0

[PATCH v2 0/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

From: David Rivshin 

This series adds support for the ISSI IS31FL32xx family of I2C LED
controllers. Since the IS31FL3218/3216 are actually the same devices as
the SN3218/3216, adds their compatible strings as aliases.

As requested, this series is based on the current linux-leds/for-next, minus
patch 2 and 3 of the standalone sn3218 driver. As such, it will not directly
apply to the current linux-next without minor merges in the leds Kconfig
and Makefile. This will apply cleanly to v4.5-rc7, albeit with a compiler
warning in patch 3 (which is fixed elsewhere in linux-next).

Changes from v1 [1]:
 - swapped node name and label in binding example
 - removed line stating filename from file header comment of is31fl32xx.c
 - dropped #includes for err.h and of_platform.h
 - added #includes for device.h, of.h, and of_device.h
 - patch 4 no longer removes leds-sn3218, as that will be done separately
 - patch 4 commit log no longer references leds-sn3218 driver
 - added Rob's acks for patches 1, 2, and 4
 - added Tested-By from Stefan (for SN3218)

Changes from RFC [2]:
 - Removed max-brightness DT property.
 - Added #address-cells and #size-cells properties to the example DT.
 - Refer to these devices as "LED controllers" in Kconfig.
 - Removed redundant last sentence from Kconfig entry
 - Removed unnecessary debug code.
 - Do not set led_classdev.brightness to 0 explicitly, as it is
   already initialized to 0 by devm_kzalloc().
 - Used of_property_read_string() instead of of_get_property().
 - Fail immediately on DT parsing error in a child node, rather than
   continuing on with the non-faulty ones.
 - Added additional comments for some things that might be non-obvious.
 - Added constants for the location of the SSD bit in the SHUTDOWN
   register, and the 3216's CONFIG register.
 - Added special sw_shutdown_func for the 3216 device, as that bit
   is in a different register, at a different position, and has reverse
   polarity compared to all the other devices.
 - Refactored is31fl32xx_init_regs() to separate out some logic into
   is31fl32xx_reset_regs() and is31fl32xx_software_shutdown().
 - Added 4th patch to replace the now-redundant leds-sn3218.

[1] https://lkml.org/lkml/2016/3/2/1004
[2] http://www.spinics.net/lists/linux-leds/msg05564.html

David Rivshin (4):
  DT: Add vendor prefix for Integrated Silicon Solutions Inc.
  DT: leds: Add binding for the ISSI IS31FL32xx family of LED
controllers
  leds: Add driver for the ISSI IS31FL32xx family of LED controllers
  leds: Add SN3218 and SN3216 support to the IS31FL32XX driver

 .../devicetree/bindings/leds/leds-is31fl32xx.txt   |  52 +++
 .../devicetree/bindings/vendor-prefixes.txt|   1 +
 drivers/leds/Kconfig   |   8 +
 drivers/leds/Makefile  |   1 +
 drivers/leds/leds-is31fl32xx.c | 508 +
 5 files changed, 570 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
 create mode 100644 drivers/leds/leds-is31fl32xx.c

-- 
2.5.0

Re: [PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

On Fri, 04 Mar 2016 16:38:01 +0100
Jacek Anaszewski <j.anaszew...@samsung.com> wrote:

> On 03/04/2016 03:27 PM, David Rivshin (Allworx) wrote:
> > (Stefan, sorry for the duplicate, I just realized that I originally
> > replied only to you by accident).
> >
> > On Thu, 3 Mar 2016 19:13:03 +0100 (CET)
> > Stefan Wahren <stefan.wah...@i2se.com> wrote:
> >  
> >> Hi David,
> >>
> >> i will test the driver on weekend. Some comments below  
> >
> > Great, thanks very much.
> >  
> >>> "David Rivshin (Allworx)" <drivshin.allw...@gmail.com> hat am 3. März 
> >>> 2016 um
> >>> 04:01 geschrieben:
> >>>
> >>>
> >>> From: David Rivshin <drivs...@allworx.com>
> >>>
> >>> The IS31FL32xx family of LED controllers are I2C devices with multiple
> >>> constant-current channels, each with independent 256-level PWM control.
> >>>
> >>> Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
> >>>
> >>> This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
> >>> (TI am335x) platform.
> >>>
> >>> The programming paradigm of these devices is similar in the following
> >>> ways:
> >>> - All registers are 8 bit
> >>> - All LED control registers are write-only
> >>> - Each LED channel has a PWM register (0-255)
> >>> - PWM register writes are shadowed until an Update register is poked
> >>> - All have a concept of Software Shutdown, which disables output
> >>>
> >>> However, there are some differences in devices:
> >>> - 3236/3235 have a separate Control register for each LED,
> >>> (3218/3216 pack the enable bits into fewer registers)
> >>> - 3236/3235 have a per-channel current divisor setting
> >>> - 3236/3235 have a Global Control register that can turn off all LEDs
> >>> - 3216 is unique in a number of ways
> >>> - OUT9-OUT16 can be configured as GPIOs instead of LED controls
> >>> - LEDs can be programmed with an 8-frame animation, with
> >>> programmable delay between frames
> >>> - LEDs can be modulated by an input audio signal
> >>> - Max output current can be adjusted from 1/4 to 2x globally
> >>> - Has a Configuration register instead of a Shutdown register
> >>>
> >>> This driver currently only supports the base PWM control function
> >>> of these devices. The following features of these devices are not
> >>> implemented, although it should be possible to add them in the future:
> >>> - All devices are capable of going into a lower-power "software
> >>> shutdown" mode.
> >>> - The is31fl3236 and is31fl3235 can reduce the max output current
> >>> per-channel with a divisor of 1, 2, 3, or 4.
> >>> - The is31fl3216 can use some LED channels as GPIOs instead.
> >>> - The is31fl3216 can animate LEDs in hardware.
> >>> - The is31fl3216 can modulate LEDs according to an audio input.
> >>> - The is31fl3216 can reduce/increase max output current globally.
> >>>
> >>> Signed-off-by: David Rivshin <drivs...@allworx.com>
> >>> ---
> >>>
> >>> You may see two instances of this warning:
> >>> "passing argument 1 of 'of_property_read_string' discards 'const'
> >>> qualifier from pointer target type"
> >>> That is a result of of_property_read_string() taking a non-const
> >>> struct device_node pointer parameter. I have separately submitted a
> >>> patch to fix that [1], and a few related functions which had the same
> >>> issue. I'm hoping that will get into linux-next before this does, so
> >>> that the warnings never show up there.
> >>>
> >>> Changes from RFC:
> >>> - Removed max-brightness DT property.
> >>> - Refer to these devices as "LED controllers" in Kconfig.
> >>> - Removed redundant last sentence from Kconfig entry
> >>> - Removed unnecessary debug code.
> >>> - Do not set led_classdev.brightness to 0 explicitly, as it is
> >>> already initialized to 0 by devm_kzalloc().
> >>> - Used of_property_read_string() instead of of_get_property().
> >>> - Fail immediately on DT parsing error in a child node, rather than
> >>> continuing on with the non-faulty ones.
> >>> - Added additional comments for some th

Re: [PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

On Fri, 04 Mar 2016 16:38:01 +0100
Jacek Anaszewski  wrote:

> On 03/04/2016 03:27 PM, David Rivshin (Allworx) wrote:
> > (Stefan, sorry for the duplicate, I just realized that I originally
> > replied only to you by accident).
> >
> > On Thu, 3 Mar 2016 19:13:03 +0100 (CET)
> > Stefan Wahren  wrote:
> >  
> >> Hi David,
> >>
> >> i will test the driver on weekend. Some comments below  
> >
> > Great, thanks very much.
> >  
> >>> "David Rivshin (Allworx)"  hat am 3. März 
> >>> 2016 um
> >>> 04:01 geschrieben:
> >>>
> >>>
> >>> From: David Rivshin 
> >>>
> >>> The IS31FL32xx family of LED controllers are I2C devices with multiple
> >>> constant-current channels, each with independent 256-level PWM control.
> >>>
> >>> Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
> >>>
> >>> This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
> >>> (TI am335x) platform.
> >>>
> >>> The programming paradigm of these devices is similar in the following
> >>> ways:
> >>> - All registers are 8 bit
> >>> - All LED control registers are write-only
> >>> - Each LED channel has a PWM register (0-255)
> >>> - PWM register writes are shadowed until an Update register is poked
> >>> - All have a concept of Software Shutdown, which disables output
> >>>
> >>> However, there are some differences in devices:
> >>> - 3236/3235 have a separate Control register for each LED,
> >>> (3218/3216 pack the enable bits into fewer registers)
> >>> - 3236/3235 have a per-channel current divisor setting
> >>> - 3236/3235 have a Global Control register that can turn off all LEDs
> >>> - 3216 is unique in a number of ways
> >>> - OUT9-OUT16 can be configured as GPIOs instead of LED controls
> >>> - LEDs can be programmed with an 8-frame animation, with
> >>> programmable delay between frames
> >>> - LEDs can be modulated by an input audio signal
> >>> - Max output current can be adjusted from 1/4 to 2x globally
> >>> - Has a Configuration register instead of a Shutdown register
> >>>
> >>> This driver currently only supports the base PWM control function
> >>> of these devices. The following features of these devices are not
> >>> implemented, although it should be possible to add them in the future:
> >>> - All devices are capable of going into a lower-power "software
> >>> shutdown" mode.
> >>> - The is31fl3236 and is31fl3235 can reduce the max output current
> >>> per-channel with a divisor of 1, 2, 3, or 4.
> >>> - The is31fl3216 can use some LED channels as GPIOs instead.
> >>> - The is31fl3216 can animate LEDs in hardware.
> >>> - The is31fl3216 can modulate LEDs according to an audio input.
> >>> - The is31fl3216 can reduce/increase max output current globally.
> >>>
> >>> Signed-off-by: David Rivshin 
> >>> ---
> >>>
> >>> You may see two instances of this warning:
> >>> "passing argument 1 of 'of_property_read_string' discards 'const'
> >>> qualifier from pointer target type"
> >>> That is a result of of_property_read_string() taking a non-const
> >>> struct device_node pointer parameter. I have separately submitted a
> >>> patch to fix that [1], and a few related functions which had the same
> >>> issue. I'm hoping that will get into linux-next before this does, so
> >>> that the warnings never show up there.
> >>>
> >>> Changes from RFC:
> >>> - Removed max-brightness DT property.
> >>> - Refer to these devices as "LED controllers" in Kconfig.
> >>> - Removed redundant last sentence from Kconfig entry
> >>> - Removed unnecessary debug code.
> >>> - Do not set led_classdev.brightness to 0 explicitly, as it is
> >>> already initialized to 0 by devm_kzalloc().
> >>> - Used of_property_read_string() instead of of_get_property().
> >>> - Fail immediately on DT parsing error in a child node, rather than
> >>> continuing on with the non-faulty ones.
> >>> - Added additional comments for some things that might be non-obvious.
> >>> - Added constants for the location of the SSD bit in the SHUTDOWN
> >>> register, and the 3216's CON

Re: [PATCH 4/4] leds: Replace dedicated SN3218 driver with IS31FL32XX driver

[David Rivshin (Allworx)]

On Thu, 03 Mar 2016 15:51:36 +0100
Jacek Anaszewski <j.anaszew...@samsung.com> wrote:

> Hi David,
> 
> I'll wait for Tested-by from Stefan before applying this patch.
> If Stefan will have managed to test your driver with his hardware
> by the end of this cycle, it will suffice for this patch to contain
> only leds-is31fl32xx extension part.
> 
> leds-sn3218 hasn't been merged to mainline yet, so I'd rather
> remove it when I get Tested-by from Stefan.
> 
> Otherwise, I will send leds-sn3218 to Linus, and this patch
> will be applied after being tested, during 4.7 cycle.

Since Stefan has given his Tested-by, do I understand correctly
that you would prefer to:
 - revert sn3218 patches 2 and 3 yourself
 - have me provide v2 patches that would apply ontop of that

If so, should I do as Rob suggests and fold the sn3216/3218 bits into 
the earlier patches, or leave it as a 4th patch? I'd probably prefer 
the former as being an easier workflow (less "rebase -i"ing).

And should I base off v4.5-rc6 at that point, or your for-next minus 
those patches? The only difference should be a 1-line offset in the 
vendor-prefixes.txt hunk. Either way is equally easy for me.

> 
> Thanks,
> Jacek Anaszewski
> 
> On 03/03/2016 04:01 AM, David Rivshin (Allworx) wrote:
> > From: David Rivshin <drivs...@allworx.com>
> >
> > Si-En Technology was acquired by ISSI in 2011, and it appears that
> > the IS31FL3218/IS31FL3216 are just rebranded SN3218/SN3216 devices.
> > As the IS31FL32XX driver already handles the *3218 devices, there
> > is no longer a need for the dedicated SN3218 driver.
> >
> > Add the "sn,sn3218" and "sn,sn3216" compatible strings into the
> > IS31FL32XX driver and binding documentation, and remove the
> > leds-sn3218 driver.
> >
> > Datasheets:
> >  IS31FL3218: http://www.issi.com/WW/pdf/31FL3218.pdf
> >  SN3218: http://www.si-en.com/uploadpdf/s2011517171720.pdf
> >
> >  IS31FL3216: http://www.issi.com/WW/pdf/31FL3216.pdf
> >  SN3216; http://www.si-en.com/uploadpdf/SN3216201152410148.pdf
> >
> > Signed-off-by: David Rivshin <drivs...@allworx.com>
> > ---
> >
> > Note that the leds-sn3218 binding use a 0-based 'reg' property, while
> > the leds-is31fl32xx binding uses a 1-based 'reg' property. This seemed
> > to be the preferred binding based on [1]. Since leds-sn3216 has not been
> > in a released kernel, there is are no backwards-compatibility concerns.
> >
> > Changes from RFC:
> >   new
> >
> > [1] http://www.spinics.net/lists/linux-leds/msg05589.html
> >
> >   .../devicetree/bindings/leds/leds-is31fl32xx.txt   |   9 +-
> >   .../devicetree/bindings/leds/leds-sn3218.txt   |  41 ---
> >   drivers/leds/Kconfig   |  18 +-
> >   drivers/leds/Makefile  |   1 -
> >   drivers/leds/leds-is31fl32xx.c |   6 +-
> >   drivers/leds/leds-sn3218.c | 306 
> > -
> >   6 files changed, 14 insertions(+), 367 deletions(-)
> >   delete mode 100644 Documentation/devicetree/bindings/leds/leds-sn3218.txt
> >   delete mode 100644 drivers/leds/leds-sn3218.c
> >
> > diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt 
> > b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> > index 539df2e..c59eb1a 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> > +++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> > @@ -1,6 +1,6 @@
> > -Binding for ISSI IS31FL32xx LED Drivers
> > +Binding for ISSI IS31FL32xx and Si-En SN32xx LED Drivers
> >
> > -The IS31FL32xx family of LED drivers are I2C devices with multiple
> > +The IS31FL32xx/SN32xx family of LED drivers are I2C devices with multiple
> >   constant-current channels, each with independent 256-level PWM control.
> >   Each LED is represented as a sub-node of the device.
> >
> > @@ -10,6 +10,8 @@ Required properties:
> > issi,is31fl3235
> > issi,is31fl3218
> > issi,is31fl3216
> > +   si-en,sn3218
> > +   si-en,sn3216
> >   - reg: I2C slave address
> >   - address-cells : must be 1
> >   - size-cells : must be 0
> > @@ -45,5 +47,6 @@ leds: is31fl3236@3c {
> > };
> >   };
> >
> > -For more product information please see the link below:
> > +For more product information please see the links below:
> >   http://www.issi.com/US/product-analog-fxled-driver.shtml
> > +http://www.si-en.com/product.asp?parentid=890
> > diff --g

Re: [PATCH 4/4] leds: Replace dedicated SN3218 driver with IS31FL32XX driver

[David Rivshin (Allworx)]

On Thu, 03 Mar 2016 15:51:36 +0100
Jacek Anaszewski  wrote:

> Hi David,
> 
> I'll wait for Tested-by from Stefan before applying this patch.
> If Stefan will have managed to test your driver with his hardware
> by the end of this cycle, it will suffice for this patch to contain
> only leds-is31fl32xx extension part.
> 
> leds-sn3218 hasn't been merged to mainline yet, so I'd rather
> remove it when I get Tested-by from Stefan.
> 
> Otherwise, I will send leds-sn3218 to Linus, and this patch
> will be applied after being tested, during 4.7 cycle.

Since Stefan has given his Tested-by, do I understand correctly
that you would prefer to:
 - revert sn3218 patches 2 and 3 yourself
 - have me provide v2 patches that would apply ontop of that

If so, should I do as Rob suggests and fold the sn3216/3218 bits into 
the earlier patches, or leave it as a 4th patch? I'd probably prefer 
the former as being an easier workflow (less "rebase -i"ing).

And should I base off v4.5-rc6 at that point, or your for-next minus 
those patches? The only difference should be a 1-line offset in the 
vendor-prefixes.txt hunk. Either way is equally easy for me.

> 
> Thanks,
> Jacek Anaszewski
> 
> On 03/03/2016 04:01 AM, David Rivshin (Allworx) wrote:
> > From: David Rivshin 
> >
> > Si-En Technology was acquired by ISSI in 2011, and it appears that
> > the IS31FL3218/IS31FL3216 are just rebranded SN3218/SN3216 devices.
> > As the IS31FL32XX driver already handles the *3218 devices, there
> > is no longer a need for the dedicated SN3218 driver.
> >
> > Add the "sn,sn3218" and "sn,sn3216" compatible strings into the
> > IS31FL32XX driver and binding documentation, and remove the
> > leds-sn3218 driver.
> >
> > Datasheets:
> >  IS31FL3218: http://www.issi.com/WW/pdf/31FL3218.pdf
> >  SN3218: http://www.si-en.com/uploadpdf/s2011517171720.pdf
> >
> >  IS31FL3216: http://www.issi.com/WW/pdf/31FL3216.pdf
> >  SN3216; http://www.si-en.com/uploadpdf/SN3216201152410148.pdf
> >
> > Signed-off-by: David Rivshin 
> > ---
> >
> > Note that the leds-sn3218 binding use a 0-based 'reg' property, while
> > the leds-is31fl32xx binding uses a 1-based 'reg' property. This seemed
> > to be the preferred binding based on [1]. Since leds-sn3216 has not been
> > in a released kernel, there is are no backwards-compatibility concerns.
> >
> > Changes from RFC:
> >   new
> >
> > [1] http://www.spinics.net/lists/linux-leds/msg05589.html
> >
> >   .../devicetree/bindings/leds/leds-is31fl32xx.txt   |   9 +-
> >   .../devicetree/bindings/leds/leds-sn3218.txt   |  41 ---
> >   drivers/leds/Kconfig   |  18 +-
> >   drivers/leds/Makefile  |   1 -
> >   drivers/leds/leds-is31fl32xx.c |   6 +-
> >   drivers/leds/leds-sn3218.c | 306 
> > -
> >   6 files changed, 14 insertions(+), 367 deletions(-)
> >   delete mode 100644 Documentation/devicetree/bindings/leds/leds-sn3218.txt
> >   delete mode 100644 drivers/leds/leds-sn3218.c
> >
> > diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt 
> > b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> > index 539df2e..c59eb1a 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> > +++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> > @@ -1,6 +1,6 @@
> > -Binding for ISSI IS31FL32xx LED Drivers
> > +Binding for ISSI IS31FL32xx and Si-En SN32xx LED Drivers
> >
> > -The IS31FL32xx family of LED drivers are I2C devices with multiple
> > +The IS31FL32xx/SN32xx family of LED drivers are I2C devices with multiple
> >   constant-current channels, each with independent 256-level PWM control.
> >   Each LED is represented as a sub-node of the device.
> >
> > @@ -10,6 +10,8 @@ Required properties:
> > issi,is31fl3235
> > issi,is31fl3218
> > issi,is31fl3216
> > +   si-en,sn3218
> > +   si-en,sn3216
> >   - reg: I2C slave address
> >   - address-cells : must be 1
> >   - size-cells : must be 0
> > @@ -45,5 +47,6 @@ leds: is31fl3236@3c {
> > };
> >   };
> >
> > -For more product information please see the link below:
> > +For more product information please see the links below:
> >   http://www.issi.com/US/product-analog-fxled-driver.shtml
> > +http://www.si-en.com/product.asp?parentid=890
> > diff --git a/Documentation/devicetree/bindings/leds/leds-sn3218.txt 
> > b/Documentation/devicetree

Re: [PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

On Fri, 4 Mar 2016 22:39:06 +0100
Jacek Anaszewski <jacek.anaszew...@gmail.com> wrote:

> On 03/04/2016 08:02 PM, David Rivshin (Allworx) wrote:
> > On Fri, 04 Mar 2016 17:01:52 +0100
> > Jacek Anaszewski <j.anaszew...@samsung.com> wrote:
> >  
> >> On 03/04/2016 04:05 PM, David Rivshin (Allworx) wrote:  
> >>> On Fri, 04 Mar 2016 08:54:02 +0100
> >>> Jacek Anaszewski <j.anaszew...@samsung.com> wrote:
> >>>  
> >>>> On 03/04/2016 01:45 AM, David Rivshin (Allworx) wrote:  
> >>>>> On Thu, 03 Mar 2016 15:51:32 +0100
> >>>>> Jacek Anaszewski <j.anaszew...@samsung.com> wrote:
> >>>>>  
> >>>>>> Hi David,
> >>>>>>
> >>>>>> Thanks for the update. Two remarks in the code.
> >>>>>>
> >>>>>> On 03/03/2016 04:01 AM, David Rivshin (Allworx) wrote:  
> >>>>>>> From: David Rivshin <drivs...@allworx.com>
> >>>>>>>
> >>>>>>> The IS31FL32xx family of LED controllers are I2C devices with multiple
> >>>>>>> constant-current channels, each with independent 256-level PWM 
> >>>>>>> control.
> >>>>>>>
> >>>>>>> Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
> >>>>>>>
> >>>>>>> This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
> >>>>>>> (TI am335x) platform.
> >>>>>>>
> >>>>>>> The programming paradigm of these devices is similar in the following
> >>>>>>> ways:
> >>>>>>>  - All registers are 8 bit
> >>>>>>>  - All LED control registers are write-only
> >>>>>>>  - Each LED channel has a PWM register (0-255)
> >>>>>>>  - PWM register writes are shadowed until an Update register is 
> >>>>>>> poked
> >>>>>>>  - All have a concept of Software Shutdown, which disables output
> >>>>>>>
> >>>>>>> However, there are some differences in devices:
> >>>>>>>  - 3236/3235 have a separate Control register for each LED,
> >>>>>>>(3218/3216 pack the enable bits into fewer registers)
> >>>>>>>  - 3236/3235 have a per-channel current divisor setting
> >>>>>>>  - 3236/3235 have a Global Control register that can turn off all 
> >>>>>>> LEDs
> >>>>>>>  - 3216 is unique in a number of ways
> >>>>>>> - OUT9-OUT16 can be configured as GPIOs instead of LED 
> >>>>>>> controls
> >>>>>>> - LEDs can be programmed with an 8-frame animation, with
> >>>>>>>   programmable delay between frames
> >>>>>>> - LEDs can be modulated by an input audio signal
> >>>>>>> - Max output current can be adjusted from 1/4 to 2x globally
> >>>>>>> - Has a Configuration register instead of a Shutdown register
> >>>>>>>
> >>>>>>> This driver currently only supports the base PWM control function
> >>>>>>> of these devices. The following features of these devices are not
> >>>>>>> implemented, although it should be possible to add them in the future:
> >>>>>>>  - All devices are capable of going into a lower-power "software
> >>>>>>>shutdown" mode.
> >>>>>>>  - The is31fl3236 and is31fl3235 can reduce the max output current
> >>>>>>>per-channel with a divisor of 1, 2, 3, or 4.
> >>>>>>>  - The is31fl3216 can use some LED channels as GPIOs instead.
> >>>>>>>  - The is31fl3216 can animate LEDs in hardware.
> >>>>>>>  - The is31fl3216 can modulate LEDs according to an audio input.
> >>>>>>>  - The is31fl3216 can reduce/increase max output current globally.
> >>>>>>>
> >>>>>>> Signed-off-by: David Rivshin <drivs...@allworx.com>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> You may see two instances of this warning:
> >>&

Re: [PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

On Fri, 4 Mar 2016 22:39:06 +0100
Jacek Anaszewski  wrote:

> On 03/04/2016 08:02 PM, David Rivshin (Allworx) wrote:
> > On Fri, 04 Mar 2016 17:01:52 +0100
> > Jacek Anaszewski  wrote:
> >  
> >> On 03/04/2016 04:05 PM, David Rivshin (Allworx) wrote:  
> >>> On Fri, 04 Mar 2016 08:54:02 +0100
> >>> Jacek Anaszewski  wrote:
> >>>  
> >>>> On 03/04/2016 01:45 AM, David Rivshin (Allworx) wrote:  
> >>>>> On Thu, 03 Mar 2016 15:51:32 +0100
> >>>>> Jacek Anaszewski  wrote:
> >>>>>  
> >>>>>> Hi David,
> >>>>>>
> >>>>>> Thanks for the update. Two remarks in the code.
> >>>>>>
> >>>>>> On 03/03/2016 04:01 AM, David Rivshin (Allworx) wrote:  
> >>>>>>> From: David Rivshin 
> >>>>>>>
> >>>>>>> The IS31FL32xx family of LED controllers are I2C devices with multiple
> >>>>>>> constant-current channels, each with independent 256-level PWM 
> >>>>>>> control.
> >>>>>>>
> >>>>>>> Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
> >>>>>>>
> >>>>>>> This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
> >>>>>>> (TI am335x) platform.
> >>>>>>>
> >>>>>>> The programming paradigm of these devices is similar in the following
> >>>>>>> ways:
> >>>>>>>  - All registers are 8 bit
> >>>>>>>  - All LED control registers are write-only
> >>>>>>>  - Each LED channel has a PWM register (0-255)
> >>>>>>>  - PWM register writes are shadowed until an Update register is 
> >>>>>>> poked
> >>>>>>>  - All have a concept of Software Shutdown, which disables output
> >>>>>>>
> >>>>>>> However, there are some differences in devices:
> >>>>>>>  - 3236/3235 have a separate Control register for each LED,
> >>>>>>>(3218/3216 pack the enable bits into fewer registers)
> >>>>>>>  - 3236/3235 have a per-channel current divisor setting
> >>>>>>>  - 3236/3235 have a Global Control register that can turn off all 
> >>>>>>> LEDs
> >>>>>>>  - 3216 is unique in a number of ways
> >>>>>>> - OUT9-OUT16 can be configured as GPIOs instead of LED 
> >>>>>>> controls
> >>>>>>> - LEDs can be programmed with an 8-frame animation, with
> >>>>>>>   programmable delay between frames
> >>>>>>> - LEDs can be modulated by an input audio signal
> >>>>>>> - Max output current can be adjusted from 1/4 to 2x globally
> >>>>>>> - Has a Configuration register instead of a Shutdown register
> >>>>>>>
> >>>>>>> This driver currently only supports the base PWM control function
> >>>>>>> of these devices. The following features of these devices are not
> >>>>>>> implemented, although it should be possible to add them in the future:
> >>>>>>>  - All devices are capable of going into a lower-power "software
> >>>>>>>shutdown" mode.
> >>>>>>>  - The is31fl3236 and is31fl3235 can reduce the max output current
> >>>>>>>per-channel with a divisor of 1, 2, 3, or 4.
> >>>>>>>  - The is31fl3216 can use some LED channels as GPIOs instead.
> >>>>>>>  - The is31fl3216 can animate LEDs in hardware.
> >>>>>>>  - The is31fl3216 can modulate LEDs according to an audio input.
> >>>>>>>  - The is31fl3216 can reduce/increase max output current globally.
> >>>>>>>
> >>>>>>> Signed-off-by: David Rivshin 
> >>>>>>> ---
> >>>>>>>
> >>>>>>> You may see two instances of this warning:
> >>>>>>>   "passing argument 1 of 'of_property_read_string' discards 
> >>>>>>> 'const'
> >>>>>>>qualifier

Re: [PATCH 4/4] leds: Replace dedicated SN3218 driver with IS31FL32XX driver

[David Rivshin (Allworx)]

On Fri, 4 Mar 2016 22:14:27 +0100 (CET)
Stefan Wahren <stefan.wah...@i2se.com> wrote:

> Hi David,
> 
> > "David Rivshin (Allworx)" <drivshin.allw...@gmail.com> hat am 3. März 2016 
> > um
> > 04:01 geschrieben:
> >
> >
> > From: David Rivshin <drivs...@allworx.com>
> >
> > Si-En Technology was acquired by ISSI in 2011, and it appears that
> > the IS31FL3218/IS31FL3216 are just rebranded SN3218/SN3216 devices.
> > As the IS31FL32XX driver already handles the *3218 devices, there
> > is no longer a need for the dedicated SN3218 driver.
> >
> > Add the "sn,sn3218" and "sn,sn3216" compatible strings into the
> > IS31FL32XX driver and binding documentation, and remove the
> > leds-sn3218 driver.
> >
> > Datasheets:
> > IS31FL3218: http://www.issi.com/WW/pdf/31FL3218.pdf
> > SN3218: http://www.si-en.com/uploadpdf/s2011517171720.pdf
> >
> > IS31FL3216: http://www.issi.com/WW/pdf/31FL3216.pdf
> > SN3216; http://www.si-en.com/uploadpdf/SN3216201152410148.pdf
> >
> > Signed-off-by: David Rivshin <drivs...@allworx.com>  
> 
> i tested this patch successfully with a Raspberry Pi and a PiGlow (SN3218).
> 
> Tested-by: Stefan Wahren <stefan.wah...@i2se.com>

Thanks very much for testing!

The PiGlow looks interesting, if I had known about it I might have just
ordered one for testing myself. Looks rather easier to hook up than the 
ISSI 3216 eval board.

Re: [PATCH 4/4] leds: Replace dedicated SN3218 driver with IS31FL32XX driver

[David Rivshin (Allworx)]

On Fri, 4 Mar 2016 22:14:27 +0100 (CET)
Stefan Wahren  wrote:

> Hi David,
> 
> > "David Rivshin (Allworx)"  hat am 3. März 2016 
> > um
> > 04:01 geschrieben:
> >
> >
> > From: David Rivshin 
> >
> > Si-En Technology was acquired by ISSI in 2011, and it appears that
> > the IS31FL3218/IS31FL3216 are just rebranded SN3218/SN3216 devices.
> > As the IS31FL32XX driver already handles the *3218 devices, there
> > is no longer a need for the dedicated SN3218 driver.
> >
> > Add the "sn,sn3218" and "sn,sn3216" compatible strings into the
> > IS31FL32XX driver and binding documentation, and remove the
> > leds-sn3218 driver.
> >
> > Datasheets:
> > IS31FL3218: http://www.issi.com/WW/pdf/31FL3218.pdf
> > SN3218: http://www.si-en.com/uploadpdf/s2011517171720.pdf
> >
> > IS31FL3216: http://www.issi.com/WW/pdf/31FL3216.pdf
> > SN3216; http://www.si-en.com/uploadpdf/SN3216201152410148.pdf
> >
> > Signed-off-by: David Rivshin   
> 
> i tested this patch successfully with a Raspberry Pi and a PiGlow (SN3218).
> 
> Tested-by: Stefan Wahren 

Thanks very much for testing!

The PiGlow looks interesting, if I had known about it I might have just
ordered one for testing myself. Looks rather easier to hook up than the 
ISSI 3216 eval board.

Re: [PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

On Fri, 04 Mar 2016 17:01:52 +0100
Jacek Anaszewski <j.anaszew...@samsung.com> wrote:

> On 03/04/2016 04:05 PM, David Rivshin (Allworx) wrote:
> > On Fri, 04 Mar 2016 08:54:02 +0100
> > Jacek Anaszewski <j.anaszew...@samsung.com> wrote:
> >  
> >> On 03/04/2016 01:45 AM, David Rivshin (Allworx) wrote:  
> >>> On Thu, 03 Mar 2016 15:51:32 +0100
> >>> Jacek Anaszewski <j.anaszew...@samsung.com> wrote:
> >>>  
> >>>> Hi David,
> >>>>
> >>>> Thanks for the update. Two remarks in the code.
> >>>>
> >>>> On 03/03/2016 04:01 AM, David Rivshin (Allworx) wrote:  
> >>>>> From: David Rivshin <drivs...@allworx.com>
> >>>>>
> >>>>> The IS31FL32xx family of LED controllers are I2C devices with multiple
> >>>>> constant-current channels, each with independent 256-level PWM control.
> >>>>>
> >>>>> Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
> >>>>>
> >>>>> This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
> >>>>> (TI am335x) platform.
> >>>>>
> >>>>> The programming paradigm of these devices is similar in the following
> >>>>> ways:
> >>>>> - All registers are 8 bit
> >>>>> - All LED control registers are write-only
> >>>>> - Each LED channel has a PWM register (0-255)
> >>>>> - PWM register writes are shadowed until an Update register is poked
> >>>>> - All have a concept of Software Shutdown, which disables output
> >>>>>
> >>>>> However, there are some differences in devices:
> >>>>> - 3236/3235 have a separate Control register for each LED,
> >>>>>   (3218/3216 pack the enable bits into fewer registers)
> >>>>> - 3236/3235 have a per-channel current divisor setting
> >>>>> - 3236/3235 have a Global Control register that can turn off all 
> >>>>> LEDs
> >>>>> - 3216 is unique in a number of ways
> >>>>>- OUT9-OUT16 can be configured as GPIOs instead of LED controls
> >>>>>- LEDs can be programmed with an 8-frame animation, with
> >>>>>  programmable delay between frames
> >>>>>- LEDs can be modulated by an input audio signal
> >>>>>- Max output current can be adjusted from 1/4 to 2x globally
> >>>>>- Has a Configuration register instead of a Shutdown register
> >>>>>
> >>>>> This driver currently only supports the base PWM control function
> >>>>> of these devices. The following features of these devices are not
> >>>>> implemented, although it should be possible to add them in the future:
> >>>>> - All devices are capable of going into a lower-power "software
> >>>>>   shutdown" mode.
> >>>>> - The is31fl3236 and is31fl3235 can reduce the max output current
> >>>>>   per-channel with a divisor of 1, 2, 3, or 4.
> >>>>> - The is31fl3216 can use some LED channels as GPIOs instead.
> >>>>> - The is31fl3216 can animate LEDs in hardware.
> >>>>> - The is31fl3216 can modulate LEDs according to an audio input.
> >>>>> - The is31fl3216 can reduce/increase max output current globally.
> >>>>>
> >>>>> Signed-off-by: David Rivshin <drivs...@allworx.com>
> >>>>> ---
> >>>>>
> >>>>> You may see two instances of this warning:
> >>>>>  "passing argument 1 of 'of_property_read_string' discards 'const'
> >>>>>   qualifier from pointer target type"
> >>>>> That is a result of of_property_read_string() taking a non-const
> >>>>> struct device_node pointer parameter. I have separately submitted a
> >>>>> patch to fix that [1], and a few related functions which had the same
> >>>>> issue. I'm hoping that will get into linux-next before this does, so
> >>>>> that the warnings never show up there.  
> >>>>
> >>>> Please adjust the patch so that it compiles without warnings on
> >>>> current linux-next. Your patch for DT API hasn't been reviewed yet
> >>>

Re: [PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

On Fri, 04 Mar 2016 17:01:52 +0100
Jacek Anaszewski  wrote:

> On 03/04/2016 04:05 PM, David Rivshin (Allworx) wrote:
> > On Fri, 04 Mar 2016 08:54:02 +0100
> > Jacek Anaszewski  wrote:
> >  
> >> On 03/04/2016 01:45 AM, David Rivshin (Allworx) wrote:  
> >>> On Thu, 03 Mar 2016 15:51:32 +0100
> >>> Jacek Anaszewski  wrote:
> >>>  
> >>>> Hi David,
> >>>>
> >>>> Thanks for the update. Two remarks in the code.
> >>>>
> >>>> On 03/03/2016 04:01 AM, David Rivshin (Allworx) wrote:  
> >>>>> From: David Rivshin 
> >>>>>
> >>>>> The IS31FL32xx family of LED controllers are I2C devices with multiple
> >>>>> constant-current channels, each with independent 256-level PWM control.
> >>>>>
> >>>>> Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
> >>>>>
> >>>>> This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
> >>>>> (TI am335x) platform.
> >>>>>
> >>>>> The programming paradigm of these devices is similar in the following
> >>>>> ways:
> >>>>> - All registers are 8 bit
> >>>>> - All LED control registers are write-only
> >>>>> - Each LED channel has a PWM register (0-255)
> >>>>> - PWM register writes are shadowed until an Update register is poked
> >>>>> - All have a concept of Software Shutdown, which disables output
> >>>>>
> >>>>> However, there are some differences in devices:
> >>>>> - 3236/3235 have a separate Control register for each LED,
> >>>>>   (3218/3216 pack the enable bits into fewer registers)
> >>>>> - 3236/3235 have a per-channel current divisor setting
> >>>>> - 3236/3235 have a Global Control register that can turn off all 
> >>>>> LEDs
> >>>>> - 3216 is unique in a number of ways
> >>>>>- OUT9-OUT16 can be configured as GPIOs instead of LED controls
> >>>>>- LEDs can be programmed with an 8-frame animation, with
> >>>>>  programmable delay between frames
> >>>>>- LEDs can be modulated by an input audio signal
> >>>>>- Max output current can be adjusted from 1/4 to 2x globally
> >>>>>- Has a Configuration register instead of a Shutdown register
> >>>>>
> >>>>> This driver currently only supports the base PWM control function
> >>>>> of these devices. The following features of these devices are not
> >>>>> implemented, although it should be possible to add them in the future:
> >>>>> - All devices are capable of going into a lower-power "software
> >>>>>   shutdown" mode.
> >>>>> - The is31fl3236 and is31fl3235 can reduce the max output current
> >>>>>   per-channel with a divisor of 1, 2, 3, or 4.
> >>>>> - The is31fl3216 can use some LED channels as GPIOs instead.
> >>>>> - The is31fl3216 can animate LEDs in hardware.
> >>>>> - The is31fl3216 can modulate LEDs according to an audio input.
> >>>>> - The is31fl3216 can reduce/increase max output current globally.
> >>>>>
> >>>>> Signed-off-by: David Rivshin 
> >>>>> ---
> >>>>>
> >>>>> You may see two instances of this warning:
> >>>>>  "passing argument 1 of 'of_property_read_string' discards 'const'
> >>>>>   qualifier from pointer target type"
> >>>>> That is a result of of_property_read_string() taking a non-const
> >>>>> struct device_node pointer parameter. I have separately submitted a
> >>>>> patch to fix that [1], and a few related functions which had the same
> >>>>> issue. I'm hoping that will get into linux-next before this does, so
> >>>>> that the warnings never show up there.  
> >>>>
> >>>> Please adjust the patch so that it compiles without warnings on
> >>>> current linux-next. Your patch for DT API hasn't been reviewed yet
> >>>> AFICS, and I can imagine that there will be some resistance against.  
> >>>
> >>> Since the DT API patch was just accep

Re: [PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

On Fri, 04 Mar 2016 08:54:02 +0100
Jacek Anaszewski <j.anaszew...@samsung.com> wrote:

> On 03/04/2016 01:45 AM, David Rivshin (Allworx) wrote:
> > On Thu, 03 Mar 2016 15:51:32 +0100
> > Jacek Anaszewski <j.anaszew...@samsung.com> wrote:
> >  
> >> Hi David,
> >>
> >> Thanks for the update. Two remarks in the code.
> >>
> >> On 03/03/2016 04:01 AM, David Rivshin (Allworx) wrote:  
> >>> From: David Rivshin <drivs...@allworx.com>
> >>>
> >>> The IS31FL32xx family of LED controllers are I2C devices with multiple
> >>> constant-current channels, each with independent 256-level PWM control.
> >>>
> >>> Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
> >>>
> >>> This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
> >>> (TI am335x) platform.
> >>>
> >>> The programming paradigm of these devices is similar in the following
> >>> ways:
> >>>- All registers are 8 bit
> >>>- All LED control registers are write-only
> >>>- Each LED channel has a PWM register (0-255)
> >>>- PWM register writes are shadowed until an Update register is poked
> >>>- All have a concept of Software Shutdown, which disables output
> >>>
> >>> However, there are some differences in devices:
> >>>- 3236/3235 have a separate Control register for each LED,
> >>>  (3218/3216 pack the enable bits into fewer registers)
> >>>- 3236/3235 have a per-channel current divisor setting
> >>>- 3236/3235 have a Global Control register that can turn off all LEDs
> >>>- 3216 is unique in a number of ways
> >>>   - OUT9-OUT16 can be configured as GPIOs instead of LED controls
> >>>   - LEDs can be programmed with an 8-frame animation, with
> >>> programmable delay between frames
> >>>   - LEDs can be modulated by an input audio signal
> >>>   - Max output current can be adjusted from 1/4 to 2x globally
> >>>   - Has a Configuration register instead of a Shutdown register
> >>>
> >>> This driver currently only supports the base PWM control function
> >>> of these devices. The following features of these devices are not
> >>> implemented, although it should be possible to add them in the future:
> >>>- All devices are capable of going into a lower-power "software
> >>>  shutdown" mode.
> >>>- The is31fl3236 and is31fl3235 can reduce the max output current
> >>>  per-channel with a divisor of 1, 2, 3, or 4.
> >>>- The is31fl3216 can use some LED channels as GPIOs instead.
> >>>- The is31fl3216 can animate LEDs in hardware.
> >>>- The is31fl3216 can modulate LEDs according to an audio input.
> >>>- The is31fl3216 can reduce/increase max output current globally.
> >>>
> >>> Signed-off-by: David Rivshin <drivs...@allworx.com>
> >>> ---
> >>>
> >>> You may see two instances of this warning:
> >>> "passing argument 1 of 'of_property_read_string' discards 'const'
> >>>  qualifier from pointer target type"
> >>> That is a result of of_property_read_string() taking a non-const
> >>> struct device_node pointer parameter. I have separately submitted a
> >>> patch to fix that [1], and a few related functions which had the same
> >>> issue. I'm hoping that will get into linux-next before this does, so
> >>> that the warnings never show up there.  
> >>
> >> Please adjust the patch so that it compiles without warnings on
> >> current linux-next. Your patch for DT API hasn't been reviewed yet
> >> AFICS, and I can imagine that there will be some resistance against.  
> >
> > Since the DT API patch was just accepted by Rob [1], would it be OK
> > to wait for the results of Stefan's testing (and any other reviews)
> > before making a decision on this? From Stefan's note, it won't be
> > until this weekend that he will have a chance to test, and I'm
> > guessing the DT API patch will make its way through Rob's tree to
> > linux-next by then.  
> 
> OK.
> 
> > FYI, the warning workaround would be to make the second parameter to
> > is31fl32xx_parse_child_dt() non-const.
> >
> > [1] https://lkml.org/lkml/2016/3/3/924
> >  
> >>> Changes from RFC:
> >>>

Re: [PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

On Fri, 04 Mar 2016 08:54:02 +0100
Jacek Anaszewski  wrote:

> On 03/04/2016 01:45 AM, David Rivshin (Allworx) wrote:
> > On Thu, 03 Mar 2016 15:51:32 +0100
> > Jacek Anaszewski  wrote:
> >  
> >> Hi David,
> >>
> >> Thanks for the update. Two remarks in the code.
> >>
> >> On 03/03/2016 04:01 AM, David Rivshin (Allworx) wrote:  
> >>> From: David Rivshin 
> >>>
> >>> The IS31FL32xx family of LED controllers are I2C devices with multiple
> >>> constant-current channels, each with independent 256-level PWM control.
> >>>
> >>> Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
> >>>
> >>> This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
> >>> (TI am335x) platform.
> >>>
> >>> The programming paradigm of these devices is similar in the following
> >>> ways:
> >>>- All registers are 8 bit
> >>>- All LED control registers are write-only
> >>>- Each LED channel has a PWM register (0-255)
> >>>- PWM register writes are shadowed until an Update register is poked
> >>>- All have a concept of Software Shutdown, which disables output
> >>>
> >>> However, there are some differences in devices:
> >>>- 3236/3235 have a separate Control register for each LED,
> >>>  (3218/3216 pack the enable bits into fewer registers)
> >>>- 3236/3235 have a per-channel current divisor setting
> >>>- 3236/3235 have a Global Control register that can turn off all LEDs
> >>>- 3216 is unique in a number of ways
> >>>   - OUT9-OUT16 can be configured as GPIOs instead of LED controls
> >>>   - LEDs can be programmed with an 8-frame animation, with
> >>> programmable delay between frames
> >>>   - LEDs can be modulated by an input audio signal
> >>>   - Max output current can be adjusted from 1/4 to 2x globally
> >>>   - Has a Configuration register instead of a Shutdown register
> >>>
> >>> This driver currently only supports the base PWM control function
> >>> of these devices. The following features of these devices are not
> >>> implemented, although it should be possible to add them in the future:
> >>>- All devices are capable of going into a lower-power "software
> >>>  shutdown" mode.
> >>>- The is31fl3236 and is31fl3235 can reduce the max output current
> >>>  per-channel with a divisor of 1, 2, 3, or 4.
> >>>- The is31fl3216 can use some LED channels as GPIOs instead.
> >>>- The is31fl3216 can animate LEDs in hardware.
> >>>- The is31fl3216 can modulate LEDs according to an audio input.
> >>>- The is31fl3216 can reduce/increase max output current globally.
> >>>
> >>> Signed-off-by: David Rivshin 
> >>> ---
> >>>
> >>> You may see two instances of this warning:
> >>> "passing argument 1 of 'of_property_read_string' discards 'const'
> >>>  qualifier from pointer target type"
> >>> That is a result of of_property_read_string() taking a non-const
> >>> struct device_node pointer parameter. I have separately submitted a
> >>> patch to fix that [1], and a few related functions which had the same
> >>> issue. I'm hoping that will get into linux-next before this does, so
> >>> that the warnings never show up there.  
> >>
> >> Please adjust the patch so that it compiles without warnings on
> >> current linux-next. Your patch for DT API hasn't been reviewed yet
> >> AFICS, and I can imagine that there will be some resistance against.  
> >
> > Since the DT API patch was just accepted by Rob [1], would it be OK
> > to wait for the results of Stefan's testing (and any other reviews)
> > before making a decision on this? From Stefan's note, it won't be
> > until this weekend that he will have a chance to test, and I'm
> > guessing the DT API patch will make its way through Rob's tree to
> > linux-next by then.  
> 
> OK.
> 
> > FYI, the warning workaround would be to make the second parameter to
> > is31fl32xx_parse_child_dt() non-const.
> >
> > [1] https://lkml.org/lkml/2016/3/3/924
> >  
> >>> Changes from RFC:
> >>>- Removed max-brightness DT property.
> >>>- Refer to these devices as "LED controllers" in Kcon

Re: [PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

(Stefan, sorry for the duplicate, I just realized that I originally 
replied only to you by accident).

On Thu, 3 Mar 2016 19:13:03 +0100 (CET)
Stefan Wahren <stefan.wah...@i2se.com> wrote:

> Hi David,
> 
> i will test the driver on weekend. Some comments below  

Great, thanks very much.

> > "David Rivshin (Allworx)" <drivshin.allw...@gmail.com> hat am 3. März 2016 
> > um
> > 04:01 geschrieben:
> >
> >
> > From: David Rivshin <drivs...@allworx.com>
> >
> > The IS31FL32xx family of LED controllers are I2C devices with multiple
> > constant-current channels, each with independent 256-level PWM control.
> >
> > Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
> >
> > This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
> > (TI am335x) platform.
> >
> > The programming paradigm of these devices is similar in the following
> > ways:
> > - All registers are 8 bit
> > - All LED control registers are write-only
> > - Each LED channel has a PWM register (0-255)
> > - PWM register writes are shadowed until an Update register is poked
> > - All have a concept of Software Shutdown, which disables output
> >
> > However, there are some differences in devices:
> > - 3236/3235 have a separate Control register for each LED,
> > (3218/3216 pack the enable bits into fewer registers)
> > - 3236/3235 have a per-channel current divisor setting
> > - 3236/3235 have a Global Control register that can turn off all LEDs
> > - 3216 is unique in a number of ways
> > - OUT9-OUT16 can be configured as GPIOs instead of LED controls
> > - LEDs can be programmed with an 8-frame animation, with
> > programmable delay between frames
> > - LEDs can be modulated by an input audio signal
> > - Max output current can be adjusted from 1/4 to 2x globally
> > - Has a Configuration register instead of a Shutdown register
> >
> > This driver currently only supports the base PWM control function
> > of these devices. The following features of these devices are not
> > implemented, although it should be possible to add them in the future:
> > - All devices are capable of going into a lower-power "software
> > shutdown" mode.
> > - The is31fl3236 and is31fl3235 can reduce the max output current
> > per-channel with a divisor of 1, 2, 3, or 4.
> > - The is31fl3216 can use some LED channels as GPIOs instead.
> > - The is31fl3216 can animate LEDs in hardware.
> > - The is31fl3216 can modulate LEDs according to an audio input.
> > - The is31fl3216 can reduce/increase max output current globally.
> >
> > Signed-off-by: David Rivshin <drivs...@allworx.com>
> > ---
> >
> > You may see two instances of this warning:
> > "passing argument 1 of 'of_property_read_string' discards 'const'
> > qualifier from pointer target type"
> > That is a result of of_property_read_string() taking a non-const
> > struct device_node pointer parameter. I have separately submitted a
> > patch to fix that [1], and a few related functions which had the same
> > issue. I'm hoping that will get into linux-next before this does, so
> > that the warnings never show up there.
> >
> > Changes from RFC:
> > - Removed max-brightness DT property.
> > - Refer to these devices as "LED controllers" in Kconfig.
> > - Removed redundant last sentence from Kconfig entry
> > - Removed unnecessary debug code.
> > - Do not set led_classdev.brightness to 0 explicitly, as it is
> > already initialized to 0 by devm_kzalloc().
> > - Used of_property_read_string() instead of of_get_property().
> > - Fail immediately on DT parsing error in a child node, rather than
> > continuing on with the non-faulty ones.
> > - Added additional comments for some things that might be non-obvious.
> > - Added constants for the location of the SSD bit in the SHUTDOWN
> > register, and the 3216's CONFIG register.
> > - Added special sw_shutdown_func for the 3216 device, as that bit
> > is in a different register, at a different position, and has reverse
> > polarity compared to all the other devices.
> > - Refactored is31fl32xx_init_regs() to separate out some logic into
> > is31fl32xx_reset_regs() and is31fl32xx_software_shutdown().
> >
> > [1] https://lkml.org/lkml/2016/3/2/746
> >
> > drivers/leds/Kconfig | 8 +
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-is31fl32xx.c | 505 
> > +
> > 3 files changed, 514 insertions(+)
> > create mode 100644 drivers/l

Re: [PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

(Stefan, sorry for the duplicate, I just realized that I originally 
replied only to you by accident).

On Thu, 3 Mar 2016 19:13:03 +0100 (CET)
Stefan Wahren  wrote:

> Hi David,
> 
> i will test the driver on weekend. Some comments below  

Great, thanks very much.

> > "David Rivshin (Allworx)"  hat am 3. März 2016 
> > um
> > 04:01 geschrieben:
> >
> >
> > From: David Rivshin 
> >
> > The IS31FL32xx family of LED controllers are I2C devices with multiple
> > constant-current channels, each with independent 256-level PWM control.
> >
> > Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
> >
> > This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
> > (TI am335x) platform.
> >
> > The programming paradigm of these devices is similar in the following
> > ways:
> > - All registers are 8 bit
> > - All LED control registers are write-only
> > - Each LED channel has a PWM register (0-255)
> > - PWM register writes are shadowed until an Update register is poked
> > - All have a concept of Software Shutdown, which disables output
> >
> > However, there are some differences in devices:
> > - 3236/3235 have a separate Control register for each LED,
> > (3218/3216 pack the enable bits into fewer registers)
> > - 3236/3235 have a per-channel current divisor setting
> > - 3236/3235 have a Global Control register that can turn off all LEDs
> > - 3216 is unique in a number of ways
> > - OUT9-OUT16 can be configured as GPIOs instead of LED controls
> > - LEDs can be programmed with an 8-frame animation, with
> > programmable delay between frames
> > - LEDs can be modulated by an input audio signal
> > - Max output current can be adjusted from 1/4 to 2x globally
> > - Has a Configuration register instead of a Shutdown register
> >
> > This driver currently only supports the base PWM control function
> > of these devices. The following features of these devices are not
> > implemented, although it should be possible to add them in the future:
> > - All devices are capable of going into a lower-power "software
> > shutdown" mode.
> > - The is31fl3236 and is31fl3235 can reduce the max output current
> > per-channel with a divisor of 1, 2, 3, or 4.
> > - The is31fl3216 can use some LED channels as GPIOs instead.
> > - The is31fl3216 can animate LEDs in hardware.
> > - The is31fl3216 can modulate LEDs according to an audio input.
> > - The is31fl3216 can reduce/increase max output current globally.
> >
> > Signed-off-by: David Rivshin 
> > ---
> >
> > You may see two instances of this warning:
> > "passing argument 1 of 'of_property_read_string' discards 'const'
> > qualifier from pointer target type"
> > That is a result of of_property_read_string() taking a non-const
> > struct device_node pointer parameter. I have separately submitted a
> > patch to fix that [1], and a few related functions which had the same
> > issue. I'm hoping that will get into linux-next before this does, so
> > that the warnings never show up there.
> >
> > Changes from RFC:
> > - Removed max-brightness DT property.
> > - Refer to these devices as "LED controllers" in Kconfig.
> > - Removed redundant last sentence from Kconfig entry
> > - Removed unnecessary debug code.
> > - Do not set led_classdev.brightness to 0 explicitly, as it is
> > already initialized to 0 by devm_kzalloc().
> > - Used of_property_read_string() instead of of_get_property().
> > - Fail immediately on DT parsing error in a child node, rather than
> > continuing on with the non-faulty ones.
> > - Added additional comments for some things that might be non-obvious.
> > - Added constants for the location of the SSD bit in the SHUTDOWN
> > register, and the 3216's CONFIG register.
> > - Added special sw_shutdown_func for the 3216 device, as that bit
> > is in a different register, at a different position, and has reverse
> > polarity compared to all the other devices.
> > - Refactored is31fl32xx_init_regs() to separate out some logic into
> > is31fl32xx_reset_regs() and is31fl32xx_software_shutdown().
> >
> > [1] https://lkml.org/lkml/2016/3/2/746
> >
> > drivers/leds/Kconfig | 8 +
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-is31fl32xx.c | 505 
> > +
> > 3 files changed, 514 insertions(+)
> > create mode 100644 drivers/leds/leds-is31fl32xx.c
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
&g

Re: [PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

On Thu, 03 Mar 2016 15:51:32 +0100
Jacek Anaszewski <j.anaszew...@samsung.com> wrote:

> Hi David,
> 
> Thanks for the update. Two remarks in the code.
> 
> On 03/03/2016 04:01 AM, David Rivshin (Allworx) wrote:
> > From: David Rivshin <drivs...@allworx.com>
> >
> > The IS31FL32xx family of LED controllers are I2C devices with multiple
> > constant-current channels, each with independent 256-level PWM control.
> >
> > Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
> >
> > This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
> > (TI am335x) platform.
> >
> > The programming paradigm of these devices is similar in the following
> > ways:
> >   - All registers are 8 bit
> >   - All LED control registers are write-only
> >   - Each LED channel has a PWM register (0-255)
> >   - PWM register writes are shadowed until an Update register is poked
> >   - All have a concept of Software Shutdown, which disables output
> >
> > However, there are some differences in devices:
> >   - 3236/3235 have a separate Control register for each LED,
> > (3218/3216 pack the enable bits into fewer registers)
> >   - 3236/3235 have a per-channel current divisor setting
> >   - 3236/3235 have a Global Control register that can turn off all LEDs
> >   - 3216 is unique in a number of ways
> >  - OUT9-OUT16 can be configured as GPIOs instead of LED controls
> >  - LEDs can be programmed with an 8-frame animation, with
> >programmable delay between frames
> >  - LEDs can be modulated by an input audio signal
> >  - Max output current can be adjusted from 1/4 to 2x globally
> >  - Has a Configuration register instead of a Shutdown register
> >
> > This driver currently only supports the base PWM control function
> > of these devices. The following features of these devices are not
> > implemented, although it should be possible to add them in the future:
> >   - All devices are capable of going into a lower-power "software
> > shutdown" mode.
> >   - The is31fl3236 and is31fl3235 can reduce the max output current
> > per-channel with a divisor of 1, 2, 3, or 4.
> >   - The is31fl3216 can use some LED channels as GPIOs instead.
> >   - The is31fl3216 can animate LEDs in hardware.
> >   - The is31fl3216 can modulate LEDs according to an audio input.
> >   - The is31fl3216 can reduce/increase max output current globally.
> >
> > Signed-off-by: David Rivshin <drivs...@allworx.com>
> > ---
> >
> > You may see two instances of this warning:
> >"passing argument 1 of 'of_property_read_string' discards 'const'
> > qualifier from pointer target type"
> > That is a result of of_property_read_string() taking a non-const
> > struct device_node pointer parameter. I have separately submitted a
> > patch to fix that [1], and a few related functions which had the same
> > issue. I'm hoping that will get into linux-next before this does, so
> > that the warnings never show up there.  
> 
> Please adjust the patch so that it compiles without warnings on
> current linux-next. Your patch for DT API hasn't been reviewed yet
> AFICS, and I can imagine that there will be some resistance against.

Since the DT API patch was just accepted by Rob [1], would it be OK 
to wait for the results of Stefan's testing (and any other reviews)
before making a decision on this? From Stefan's note, it won't be
until this weekend that he will have a chance to test, and I'm 
guessing the DT API patch will make its way through Rob's tree to
linux-next by then.

FYI, the warning workaround would be to make the second parameter to
is31fl32xx_parse_child_dt() non-const. 

[1] https://lkml.org/lkml/2016/3/3/924

> > Changes from RFC:
> >   - Removed max-brightness DT property.
> >   - Refer to these devices as "LED controllers" in Kconfig.
> >   - Removed redundant last sentence from Kconfig entry
> >   - Removed unnecessary debug code.
> >   - Do not set led_classdev.brightness to 0 explicitly, as it is
> > already initialized to 0 by devm_kzalloc().
> >   - Used of_property_read_string() instead of of_get_property().
> >   - Fail immediately on DT parsing error in a child node, rather than
> > continuing on with the non-faulty ones.
> >   - Added additional comments for some things that might be non-obvious.
> >   - Added constants for the location of the SSD bit in the SHUTDOWN
> > register, and the 3216's CONFIG register.
> >   - Added special sw_shutdown_func for 

Re: [PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

On Thu, 03 Mar 2016 15:51:32 +0100
Jacek Anaszewski  wrote:

> Hi David,
> 
> Thanks for the update. Two remarks in the code.
> 
> On 03/03/2016 04:01 AM, David Rivshin (Allworx) wrote:
> > From: David Rivshin 
> >
> > The IS31FL32xx family of LED controllers are I2C devices with multiple
> > constant-current channels, each with independent 256-level PWM control.
> >
> > Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml
> >
> > This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
> > (TI am335x) platform.
> >
> > The programming paradigm of these devices is similar in the following
> > ways:
> >   - All registers are 8 bit
> >   - All LED control registers are write-only
> >   - Each LED channel has a PWM register (0-255)
> >   - PWM register writes are shadowed until an Update register is poked
> >   - All have a concept of Software Shutdown, which disables output
> >
> > However, there are some differences in devices:
> >   - 3236/3235 have a separate Control register for each LED,
> > (3218/3216 pack the enable bits into fewer registers)
> >   - 3236/3235 have a per-channel current divisor setting
> >   - 3236/3235 have a Global Control register that can turn off all LEDs
> >   - 3216 is unique in a number of ways
> >  - OUT9-OUT16 can be configured as GPIOs instead of LED controls
> >  - LEDs can be programmed with an 8-frame animation, with
> >programmable delay between frames
> >  - LEDs can be modulated by an input audio signal
> >  - Max output current can be adjusted from 1/4 to 2x globally
> >  - Has a Configuration register instead of a Shutdown register
> >
> > This driver currently only supports the base PWM control function
> > of these devices. The following features of these devices are not
> > implemented, although it should be possible to add them in the future:
> >   - All devices are capable of going into a lower-power "software
> > shutdown" mode.
> >   - The is31fl3236 and is31fl3235 can reduce the max output current
> > per-channel with a divisor of 1, 2, 3, or 4.
> >   - The is31fl3216 can use some LED channels as GPIOs instead.
> >   - The is31fl3216 can animate LEDs in hardware.
> >   - The is31fl3216 can modulate LEDs according to an audio input.
> >   - The is31fl3216 can reduce/increase max output current globally.
> >
> > Signed-off-by: David Rivshin 
> > ---
> >
> > You may see two instances of this warning:
> >"passing argument 1 of 'of_property_read_string' discards 'const'
> > qualifier from pointer target type"
> > That is a result of of_property_read_string() taking a non-const
> > struct device_node pointer parameter. I have separately submitted a
> > patch to fix that [1], and a few related functions which had the same
> > issue. I'm hoping that will get into linux-next before this does, so
> > that the warnings never show up there.  
> 
> Please adjust the patch so that it compiles without warnings on
> current linux-next. Your patch for DT API hasn't been reviewed yet
> AFICS, and I can imagine that there will be some resistance against.

Since the DT API patch was just accepted by Rob [1], would it be OK 
to wait for the results of Stefan's testing (and any other reviews)
before making a decision on this? From Stefan's note, it won't be
until this weekend that he will have a chance to test, and I'm 
guessing the DT API patch will make its way through Rob's tree to
linux-next by then.

FYI, the warning workaround would be to make the second parameter to
is31fl32xx_parse_child_dt() non-const. 

[1] https://lkml.org/lkml/2016/3/3/924

> > Changes from RFC:
> >   - Removed max-brightness DT property.
> >   - Refer to these devices as "LED controllers" in Kconfig.
> >   - Removed redundant last sentence from Kconfig entry
> >   - Removed unnecessary debug code.
> >   - Do not set led_classdev.brightness to 0 explicitly, as it is
> > already initialized to 0 by devm_kzalloc().
> >   - Used of_property_read_string() instead of of_get_property().
> >   - Fail immediately on DT parsing error in a child node, rather than
> > continuing on with the non-faulty ones.
> >   - Added additional comments for some things that might be non-obvious.
> >   - Added constants for the location of the SSD bit in the SHUTDOWN
> > register, and the 3216's CONFIG register.
> >   - Added special sw_shutdown_func for the 3216 device, as that bit
> > is in a different register, at a different position, and has reverse
> > polar

Re: [PATCH] of: add 'const' for of_property_*_string*() parameter '*np'

[David Rivshin (Allworx)]

On Thu, 3 Mar 2016 07:52:51 -0600
Rob Herring <robh...@kernel.org> wrote:

> On Wed, Mar 2, 2016 at 3:35 PM, David Rivshin (Allworx)
> <drivshin.allw...@gmail.com> wrote:
> > From: David Rivshin <drivs...@allworx.com>
> >
> > The of_property_{read,count,match}_string* family of functions never
> > modify the struct device_node pointer that is passed in, so there is no
> > reason for it to be non-const. Equivalent functions for all other types
> > already take a 'const struct device_node *np'.
> >
> > Signed-off-by: David Rivshin <drivs...@allworx.com>
> > ---
> >
> > MAINTAINTERS says that the appropriate tree is
> > git://git.kernel.org/pub/scm/linux/kernel/git/glikely/linux.git  
> 
> Yes, we probably need to update that.
> 
> > but it looks like that hasn't been updated in a while. So this patch
> > is based off the "for-next" branch of
> > git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
> > instead. Let me know if you need me to respin from another tree/branch.  
> 
> You should base off of Linus' tree unless you have some dependency. I

I was under the impression that the general rule would be to base off
whichever tree it is likely to go through, to make it easier for the
maintainer if nothing else. Is that an incorrect impression, or do you 
mean that just for OF/DT changes?

> doubt it matters in this case, so no need to resend.

Right, these files have not changed recently, so I expect the patch to 
apply against just about any tree. FYI, I can confirm that this applies 
and compiles (arm_multi_v7_defconfig and x86_64_defconfig) on Linus' tree 
as of a few minutes ago. And the same was true against your for-next, and 
linux-next, as of when I sent the patch.

> >
> >  drivers/of/base.c  | 15 ---
> >  include/linux/of.h | 18 +-
> >  2 files changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 017dd94..b299de2 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1341,10 +1341,10 @@ EXPORT_SYMBOL_GPL(of_property_read_u64_array);
> >   *
> >   * The out_string pointer is modified only if a valid string can be 
> > decoded.
> >   */
> > -int of_property_read_string(struct device_node *np, const char *propname,
> > +int of_property_read_string(const struct device_node *np, const char 
> > *propname,
> > const char **out_string)
> >  {
> > -   struct property *prop = of_find_property(np, propname, NULL);
> > +   const struct property *prop = of_find_property(np, propname, NULL);
> > if (!prop)
> > return -EINVAL;
> > if (!prop->value)
> > @@ -1365,10 +1365,10 @@ EXPORT_SYMBOL_GPL(of_property_read_string);
> >   * This function searches a string list property and returns the index
> >   * of a specific string value.
> >   */
> > -int of_property_match_string(struct device_node *np, const char *propname,
> > +int of_property_match_string(const struct device_node *np, const char 
> > *propname,
> >  const char *string)
> >  {
> > -   struct property *prop = of_find_property(np, propname, NULL);
> > +   const struct property *prop = of_find_property(np, propname, NULL);
> > size_t l;
> > int i;
> > const char *p, *end;
> > @@ -1404,10 +1404,11 @@ EXPORT_SYMBOL_GPL(of_property_match_string);
> >   * Don't call this function directly. It is a utility helper for the
> >   * of_property_read_string*() family of functions.
> >   */
> > -int of_property_read_string_helper(struct device_node *np, const char 
> > *propname,
> > -  const char **out_strs, size_t sz, int 
> > skip)
> > +int of_property_read_string_helper(const struct device_node *np,
> > +  const char *propname, const char 
> > **out_strs,
> > +  size_t sz, int skip)
> >  {
> > -   struct property *prop = of_find_property(np, propname, NULL);
> > +   const struct property *prop = of_find_property(np, propname, NULL);
> > int l = 0, i = 0;
> > const char *p, *end;
> >
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index dc6e396..7fcb681 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -296,13 +296,13 @@ extern int of_property_read_u64_array(const struct 
> > device_node *np,
> >   

Re: [PATCH] of: add 'const' for of_property_*_string*() parameter '*np'

[David Rivshin (Allworx)]

On Thu, 3 Mar 2016 07:52:51 -0600
Rob Herring  wrote:

> On Wed, Mar 2, 2016 at 3:35 PM, David Rivshin (Allworx)
>  wrote:
> > From: David Rivshin 
> >
> > The of_property_{read,count,match}_string* family of functions never
> > modify the struct device_node pointer that is passed in, so there is no
> > reason for it to be non-const. Equivalent functions for all other types
> > already take a 'const struct device_node *np'.
> >
> > Signed-off-by: David Rivshin 
> > ---
> >
> > MAINTAINTERS says that the appropriate tree is
> > git://git.kernel.org/pub/scm/linux/kernel/git/glikely/linux.git  
> 
> Yes, we probably need to update that.
> 
> > but it looks like that hasn't been updated in a while. So this patch
> > is based off the "for-next" branch of
> > git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
> > instead. Let me know if you need me to respin from another tree/branch.  
> 
> You should base off of Linus' tree unless you have some dependency. I

I was under the impression that the general rule would be to base off
whichever tree it is likely to go through, to make it easier for the
maintainer if nothing else. Is that an incorrect impression, or do you 
mean that just for OF/DT changes?

> doubt it matters in this case, so no need to resend.

Right, these files have not changed recently, so I expect the patch to 
apply against just about any tree. FYI, I can confirm that this applies 
and compiles (arm_multi_v7_defconfig and x86_64_defconfig) on Linus' tree 
as of a few minutes ago. And the same was true against your for-next, and 
linux-next, as of when I sent the patch.

> >
> >  drivers/of/base.c  | 15 ---
> >  include/linux/of.h | 18 +-
> >  2 files changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 017dd94..b299de2 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1341,10 +1341,10 @@ EXPORT_SYMBOL_GPL(of_property_read_u64_array);
> >   *
> >   * The out_string pointer is modified only if a valid string can be 
> > decoded.
> >   */
> > -int of_property_read_string(struct device_node *np, const char *propname,
> > +int of_property_read_string(const struct device_node *np, const char 
> > *propname,
> > const char **out_string)
> >  {
> > -   struct property *prop = of_find_property(np, propname, NULL);
> > +   const struct property *prop = of_find_property(np, propname, NULL);
> > if (!prop)
> > return -EINVAL;
> > if (!prop->value)
> > @@ -1365,10 +1365,10 @@ EXPORT_SYMBOL_GPL(of_property_read_string);
> >   * This function searches a string list property and returns the index
> >   * of a specific string value.
> >   */
> > -int of_property_match_string(struct device_node *np, const char *propname,
> > +int of_property_match_string(const struct device_node *np, const char 
> > *propname,
> >  const char *string)
> >  {
> > -   struct property *prop = of_find_property(np, propname, NULL);
> > +   const struct property *prop = of_find_property(np, propname, NULL);
> > size_t l;
> > int i;
> > const char *p, *end;
> > @@ -1404,10 +1404,11 @@ EXPORT_SYMBOL_GPL(of_property_match_string);
> >   * Don't call this function directly. It is a utility helper for the
> >   * of_property_read_string*() family of functions.
> >   */
> > -int of_property_read_string_helper(struct device_node *np, const char 
> > *propname,
> > -  const char **out_strs, size_t sz, int 
> > skip)
> > +int of_property_read_string_helper(const struct device_node *np,
> > +  const char *propname, const char 
> > **out_strs,
> > +  size_t sz, int skip)
> >  {
> > -   struct property *prop = of_find_property(np, propname, NULL);
> > +   const struct property *prop = of_find_property(np, propname, NULL);
> > int l = 0, i = 0;
> > const char *p, *end;
> >
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index dc6e396..7fcb681 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -296,13 +296,13 @@ extern int of_property_read_u64_array(const struct 
> > device_node *np,
> >   u64 *out_values,
> >   size_t sz);
> >
> > -

[PATCH 2/4] DT: leds: Add binding for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

From: David Rivshin 

This adds a binding description for the is31fl3236/35/18/16 I2C LED
controllers.

Signed-off-by: David Rivshin 
---

Rob,
 I went with the 1-based 'reg' property here. I inferred that that
would be your preference based on the previous thread [1]. Let me
know if that's not the case.

Changes from RFC:
 - Removed max-brightness property.
 - Added #address-cells and #size-cells properties to the example.

[1] http://www.spinics.net/lists/linux-leds/msg05589.html

 .../devicetree/bindings/leds/leds-is31fl32xx.txt   | 49 ++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt 
b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
new file mode 100644
index 000..539df2e
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
@@ -0,0 +1,49 @@
+Binding for ISSI IS31FL32xx LED Drivers
+
+The IS31FL32xx family of LED drivers are I2C devices with multiple
+constant-current channels, each with independent 256-level PWM control.
+Each LED is represented as a sub-node of the device.
+
+Required properties:
+- compatible: one of
+   issi,is31fl3236
+   issi,is31fl3235
+   issi,is31fl3218
+   issi,is31fl3216
+- reg: I2C slave address
+- address-cells : must be 1
+- size-cells : must be 0
+
+LED sub-node properties:
+- reg : LED channel number (1..N)
+- label :  (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger :  (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+
+
+Example:
+
+leds: is31fl3236@3c {
+   compatible = "issi,is31fl3236";
+   reg = <0x3c>;
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   led@1 {
+   reg = <1>;
+   label = "EB:blue:usr0";
+   };
+   led@2 {
+   reg = <2>;
+   label = "EB:blue:usr1";
+   };
+   ...
+   led@36 {
+   reg = <36>;
+   label = "EB:blue:usr35";
+   };
+};
+
+For more product information please see the link below:
+http://www.issi.com/US/product-analog-fxled-driver.shtml
-- 
2.5.0

[PATCH 2/4] DT: leds: Add binding for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

From: David Rivshin 

This adds a binding description for the is31fl3236/35/18/16 I2C LED
controllers.

Signed-off-by: David Rivshin 
---

Rob,
 I went with the 1-based 'reg' property here. I inferred that that
would be your preference based on the previous thread [1]. Let me
know if that's not the case.

Changes from RFC:
 - Removed max-brightness property.
 - Added #address-cells and #size-cells properties to the example.

[1] http://www.spinics.net/lists/linux-leds/msg05589.html

 .../devicetree/bindings/leds/leds-is31fl32xx.txt   | 49 ++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt 
b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
new file mode 100644
index 000..539df2e
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
@@ -0,0 +1,49 @@
+Binding for ISSI IS31FL32xx LED Drivers
+
+The IS31FL32xx family of LED drivers are I2C devices with multiple
+constant-current channels, each with independent 256-level PWM control.
+Each LED is represented as a sub-node of the device.
+
+Required properties:
+- compatible: one of
+   issi,is31fl3236
+   issi,is31fl3235
+   issi,is31fl3218
+   issi,is31fl3216
+- reg: I2C slave address
+- address-cells : must be 1
+- size-cells : must be 0
+
+LED sub-node properties:
+- reg : LED channel number (1..N)
+- label :  (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger :  (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+
+
+Example:
+
+leds: is31fl3236@3c {
+   compatible = "issi,is31fl3236";
+   reg = <0x3c>;
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   led@1 {
+   reg = <1>;
+   label = "EB:blue:usr0";
+   };
+   led@2 {
+   reg = <2>;
+   label = "EB:blue:usr1";
+   };
+   ...
+   led@36 {
+   reg = <36>;
+   label = "EB:blue:usr35";
+   };
+};
+
+For more product information please see the link below:
+http://www.issi.com/US/product-analog-fxled-driver.shtml
-- 
2.5.0

[PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

From: David Rivshin 

The IS31FL32xx family of LED controllers are I2C devices with multiple
constant-current channels, each with independent 256-level PWM control.

Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml

This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
(TI am335x) platform.

The programming paradigm of these devices is similar in the following
ways:
 - All registers are 8 bit
 - All LED control registers are write-only
 - Each LED channel has a PWM register (0-255)
 - PWM register writes are shadowed until an Update register is poked
 - All have a concept of Software Shutdown, which disables output

However, there are some differences in devices:
 - 3236/3235 have a separate Control register for each LED,
   (3218/3216 pack the enable bits into fewer registers)
 - 3236/3235 have a per-channel current divisor setting
 - 3236/3235 have a Global Control register that can turn off all LEDs
 - 3216 is unique in a number of ways
- OUT9-OUT16 can be configured as GPIOs instead of LED controls
- LEDs can be programmed with an 8-frame animation, with
  programmable delay between frames
- LEDs can be modulated by an input audio signal
- Max output current can be adjusted from 1/4 to 2x globally
- Has a Configuration register instead of a Shutdown register

This driver currently only supports the base PWM control function
of these devices. The following features of these devices are not
implemented, although it should be possible to add them in the future:
 - All devices are capable of going into a lower-power "software
   shutdown" mode.
 - The is31fl3236 and is31fl3235 can reduce the max output current
   per-channel with a divisor of 1, 2, 3, or 4.
 - The is31fl3216 can use some LED channels as GPIOs instead.
 - The is31fl3216 can animate LEDs in hardware.
 - The is31fl3216 can modulate LEDs according to an audio input.
 - The is31fl3216 can reduce/increase max output current globally.

Signed-off-by: David Rivshin 
---

You may see two instances of this warning:
  "passing argument 1 of 'of_property_read_string' discards 'const'
   qualifier from pointer target type"
That is a result of of_property_read_string() taking a non-const
struct device_node pointer parameter. I have separately submitted a
patch to fix that [1], and a few related functions which had the same
issue. I'm hoping that will get into linux-next before this does, so
that the warnings never show up there.

Changes from RFC:
 - Removed max-brightness DT property.
 - Refer to these devices as "LED controllers" in Kconfig.
 - Removed redundant last sentence from Kconfig entry
 - Removed unnecessary debug code.
 - Do not set led_classdev.brightness to 0 explicitly, as it is
   already initialized to 0 by devm_kzalloc().
 - Used of_property_read_string() instead of of_get_property().
 - Fail immediately on DT parsing error in a child node, rather than
   continuing on with the non-faulty ones.
 - Added additional comments for some things that might be non-obvious.
 - Added constants for the location of the SSD bit in the SHUTDOWN
   register, and the 3216's CONFIG register.
 - Added special sw_shutdown_func for the 3216 device, as that bit
   is in a different register, at a different position, and has reverse
   polarity compared to all the other devices.
 - Refactored is31fl32xx_init_regs() to separate out some logic into
   is31fl32xx_reset_regs() and is31fl32xx_software_shutdown().

[1] https://lkml.org/lkml/2016/3/2/746

 drivers/leds/Kconfig   |   8 +
 drivers/leds/Makefile  |   1 +
 drivers/leds/leds-is31fl32xx.c | 505 +
 3 files changed, 514 insertions(+)
 create mode 100644 drivers/leds/leds-is31fl32xx.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1034696..9c63ba4 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -580,6 +580,14 @@ config LEDS_SN3218
  This driver can also be built as a module. If so the module
  will be called leds-sn3218.
 
+config LEDS_IS31FL32XX
+   tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
+   depends on LEDS_CLASS && I2C && OF
+   help
+ Say Y here to include support for ISSI IS31FL32XX LED controllers.
+ They are I2C devices with multiple constant-current channels, each
+ with independent 256-level PWM control.
+
 comment "LED driver for blink(1) USB RGB LED is under Special HID drivers 
(HID_THINGM)"
 
 config LEDS_BLINKM
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 89c9b6f..3fdf313 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_KTD2692)+= leds-ktd2692.o
 obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
 obj-$(CONFIG_LEDS_SEAD3)   += leds-sead3.o
 obj-$(CONFIG_LEDS_SN3218)  += leds-sn3218.o
+obj-$(CONFIG_LEDS_IS31FL32XX) 

[PATCH 1/4] DT: Add vendor prefix for Integrated Silicon Solutions Inc.

[David Rivshin (Allworx)]

From: David Rivshin 

ISSI is the stock ticker Integrated Silicon Solutions Inc.
Company website: http://www.issi.com

Signed-off-by: David Rivshin 
---

Changes from RFC:
 none

 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 52f22f9..dd72e05 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -120,6 +120,7 @@ intercontrolInter Control Group
 invensense InvenSense Inc.
 isee   ISEE 2007 S.L.
 isil   Intersil
+issi   Integrated Silicon Solutions Inc.
 jedec  JEDEC Solid State Technology Association
 karo   Ka-Ro electronics GmbH
 keymileKeymile GmbH
-- 
2.5.0

[PATCH 4/4] leds: Replace dedicated SN3218 driver with IS31FL32XX driver

[David Rivshin (Allworx)]

From: David Rivshin 

Si-En Technology was acquired by ISSI in 2011, and it appears that
the IS31FL3218/IS31FL3216 are just rebranded SN3218/SN3216 devices.
As the IS31FL32XX driver already handles the *3218 devices, there
is no longer a need for the dedicated SN3218 driver.

Add the "sn,sn3218" and "sn,sn3216" compatible strings into the
IS31FL32XX driver and binding documentation, and remove the
leds-sn3218 driver.

Datasheets:
IS31FL3218: http://www.issi.com/WW/pdf/31FL3218.pdf
SN3218: http://www.si-en.com/uploadpdf/s2011517171720.pdf

IS31FL3216: http://www.issi.com/WW/pdf/31FL3216.pdf
SN3216; http://www.si-en.com/uploadpdf/SN3216201152410148.pdf

Signed-off-by: David Rivshin 
---

Note that the leds-sn3218 binding use a 0-based 'reg' property, while
the leds-is31fl32xx binding uses a 1-based 'reg' property. This seemed
to be the preferred binding based on [1]. Since leds-sn3216 has not been
in a released kernel, there is are no backwards-compatibility concerns.

Changes from RFC:
 new

[1] http://www.spinics.net/lists/linux-leds/msg05589.html

 .../devicetree/bindings/leds/leds-is31fl32xx.txt   |   9 +-
 .../devicetree/bindings/leds/leds-sn3218.txt   |  41 ---
 drivers/leds/Kconfig   |  18 +-
 drivers/leds/Makefile  |   1 -
 drivers/leds/leds-is31fl32xx.c |   6 +-
 drivers/leds/leds-sn3218.c | 306 -
 6 files changed, 14 insertions(+), 367 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/leds/leds-sn3218.txt
 delete mode 100644 drivers/leds/leds-sn3218.c

diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt 
b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
index 539df2e..c59eb1a 100644
--- a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
+++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
@@ -1,6 +1,6 @@
-Binding for ISSI IS31FL32xx LED Drivers
+Binding for ISSI IS31FL32xx and Si-En SN32xx LED Drivers
 
-The IS31FL32xx family of LED drivers are I2C devices with multiple
+The IS31FL32xx/SN32xx family of LED drivers are I2C devices with multiple
 constant-current channels, each with independent 256-level PWM control.
 Each LED is represented as a sub-node of the device.
 
@@ -10,6 +10,8 @@ Required properties:
issi,is31fl3235
issi,is31fl3218
issi,is31fl3216
+   si-en,sn3218
+   si-en,sn3216
 - reg: I2C slave address
 - address-cells : must be 1
 - size-cells : must be 0
@@ -45,5 +47,6 @@ leds: is31fl3236@3c {
};
 };
 
-For more product information please see the link below:
+For more product information please see the links below:
 http://www.issi.com/US/product-analog-fxled-driver.shtml
+http://www.si-en.com/product.asp?parentid=890
diff --git a/Documentation/devicetree/bindings/leds/leds-sn3218.txt 
b/Documentation/devicetree/bindings/leds/leds-sn3218.txt
deleted file mode 100644
index 19cbf57..000
--- a/Documentation/devicetree/bindings/leds/leds-sn3218.txt
+++ /dev/null
@@ -1,41 +0,0 @@
-* Si-En Technology - SN3218 18-Channel LED Driver
-
-Required properties:
-- compatible :
-   "si-en,sn3218"
-- address-cells : must be 1
-- size-cells : must be 0
-- reg : I2C slave address, typically 0x54
-
-There must be at least 1 LED which is represented as a sub-node
-of the sn3218 device.
-
-LED sub-node properties:
-- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
-- reg : number of LED line (could be from 0 to 17)
-- linux,default-trigger : (optional)
-   see Documentation/devicetree/bindings/leds/common.txt
-
-Example:
-
-sn3218: led-controller@54 {
-   compatible = "si-en,sn3218";
-   #address-cells = <1>;
-   #size-cells = <0>;
-   reg = <0x54>;
-
-   led@0 {
-   label = "led1";
-   reg = <0x0>;
-   };
-
-   led@1 {
-   label = "led2";
-   reg = <0x1>;
-   };
-
-   led@2 {
-   label = "led3";
-   reg = <0x2>;
-   };
-};
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9c63ba4..1f64151 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -568,25 +568,13 @@ config LEDS_SEAD3
  This driver can also be built as a module. If so the module
  will be called leds-sead3.
 
-config LEDS_SN3218
-   tristate "LED support for Si-En SN3218 I2C chip"
-   depends on LEDS_CLASS && I2C
-   depends on OF
-   select REGMAP_I2C
-   help
- This option enables support for the Si-EN SN3218 LED driver
- connected through I2C. Say Y to enable support for the SN3218 LED.
-
- This driver can also be built as a module. If so the module
- will be called leds-sn3218.
-
 config LEDS_IS31FL32XX
tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
depends on LEDS_CLASS && 

[PATCH 3/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

From: David Rivshin 

The IS31FL32xx family of LED controllers are I2C devices with multiple
constant-current channels, each with independent 256-level PWM control.

Datasheets: http://www.issi.com/US/product-analog-fxled-driver.shtml

This has been tested on the IS31FL3236 and IS31FL3216, on an ARM
(TI am335x) platform.

The programming paradigm of these devices is similar in the following
ways:
 - All registers are 8 bit
 - All LED control registers are write-only
 - Each LED channel has a PWM register (0-255)
 - PWM register writes are shadowed until an Update register is poked
 - All have a concept of Software Shutdown, which disables output

However, there are some differences in devices:
 - 3236/3235 have a separate Control register for each LED,
   (3218/3216 pack the enable bits into fewer registers)
 - 3236/3235 have a per-channel current divisor setting
 - 3236/3235 have a Global Control register that can turn off all LEDs
 - 3216 is unique in a number of ways
- OUT9-OUT16 can be configured as GPIOs instead of LED controls
- LEDs can be programmed with an 8-frame animation, with
  programmable delay between frames
- LEDs can be modulated by an input audio signal
- Max output current can be adjusted from 1/4 to 2x globally
- Has a Configuration register instead of a Shutdown register

This driver currently only supports the base PWM control function
of these devices. The following features of these devices are not
implemented, although it should be possible to add them in the future:
 - All devices are capable of going into a lower-power "software
   shutdown" mode.
 - The is31fl3236 and is31fl3235 can reduce the max output current
   per-channel with a divisor of 1, 2, 3, or 4.
 - The is31fl3216 can use some LED channels as GPIOs instead.
 - The is31fl3216 can animate LEDs in hardware.
 - The is31fl3216 can modulate LEDs according to an audio input.
 - The is31fl3216 can reduce/increase max output current globally.

Signed-off-by: David Rivshin 
---

You may see two instances of this warning:
  "passing argument 1 of 'of_property_read_string' discards 'const'
   qualifier from pointer target type"
That is a result of of_property_read_string() taking a non-const
struct device_node pointer parameter. I have separately submitted a
patch to fix that [1], and a few related functions which had the same
issue. I'm hoping that will get into linux-next before this does, so
that the warnings never show up there.

Changes from RFC:
 - Removed max-brightness DT property.
 - Refer to these devices as "LED controllers" in Kconfig.
 - Removed redundant last sentence from Kconfig entry
 - Removed unnecessary debug code.
 - Do not set led_classdev.brightness to 0 explicitly, as it is
   already initialized to 0 by devm_kzalloc().
 - Used of_property_read_string() instead of of_get_property().
 - Fail immediately on DT parsing error in a child node, rather than
   continuing on with the non-faulty ones.
 - Added additional comments for some things that might be non-obvious.
 - Added constants for the location of the SSD bit in the SHUTDOWN
   register, and the 3216's CONFIG register.
 - Added special sw_shutdown_func for the 3216 device, as that bit
   is in a different register, at a different position, and has reverse
   polarity compared to all the other devices.
 - Refactored is31fl32xx_init_regs() to separate out some logic into
   is31fl32xx_reset_regs() and is31fl32xx_software_shutdown().

[1] https://lkml.org/lkml/2016/3/2/746

 drivers/leds/Kconfig   |   8 +
 drivers/leds/Makefile  |   1 +
 drivers/leds/leds-is31fl32xx.c | 505 +
 3 files changed, 514 insertions(+)
 create mode 100644 drivers/leds/leds-is31fl32xx.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1034696..9c63ba4 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -580,6 +580,14 @@ config LEDS_SN3218
  This driver can also be built as a module. If so the module
  will be called leds-sn3218.
 
+config LEDS_IS31FL32XX
+   tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
+   depends on LEDS_CLASS && I2C && OF
+   help
+ Say Y here to include support for ISSI IS31FL32XX LED controllers.
+ They are I2C devices with multiple constant-current channels, each
+ with independent 256-level PWM control.
+
 comment "LED driver for blink(1) USB RGB LED is under Special HID drivers 
(HID_THINGM)"
 
 config LEDS_BLINKM
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 89c9b6f..3fdf313 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_KTD2692)+= leds-ktd2692.o
 obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
 obj-$(CONFIG_LEDS_SEAD3)   += leds-sead3.o
 obj-$(CONFIG_LEDS_SN3218)  += leds-sn3218.o
+obj-$(CONFIG_LEDS_IS31FL32XX)  += leds-is31fl32xx.o
 
 # LED SPI 

[PATCH 1/4] DT: Add vendor prefix for Integrated Silicon Solutions Inc.

[David Rivshin (Allworx)]

From: David Rivshin 

ISSI is the stock ticker Integrated Silicon Solutions Inc.
Company website: http://www.issi.com

Signed-off-by: David Rivshin 
---

Changes from RFC:
 none

 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 52f22f9..dd72e05 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -120,6 +120,7 @@ intercontrolInter Control Group
 invensense InvenSense Inc.
 isee   ISEE 2007 S.L.
 isil   Intersil
+issi   Integrated Silicon Solutions Inc.
 jedec  JEDEC Solid State Technology Association
 karo   Ka-Ro electronics GmbH
 keymileKeymile GmbH
-- 
2.5.0

[PATCH 4/4] leds: Replace dedicated SN3218 driver with IS31FL32XX driver

[David Rivshin (Allworx)]

From: David Rivshin 

Si-En Technology was acquired by ISSI in 2011, and it appears that
the IS31FL3218/IS31FL3216 are just rebranded SN3218/SN3216 devices.
As the IS31FL32XX driver already handles the *3218 devices, there
is no longer a need for the dedicated SN3218 driver.

Add the "sn,sn3218" and "sn,sn3216" compatible strings into the
IS31FL32XX driver and binding documentation, and remove the
leds-sn3218 driver.

Datasheets:
IS31FL3218: http://www.issi.com/WW/pdf/31FL3218.pdf
SN3218: http://www.si-en.com/uploadpdf/s2011517171720.pdf

IS31FL3216: http://www.issi.com/WW/pdf/31FL3216.pdf
SN3216; http://www.si-en.com/uploadpdf/SN3216201152410148.pdf

Signed-off-by: David Rivshin 
---

Note that the leds-sn3218 binding use a 0-based 'reg' property, while
the leds-is31fl32xx binding uses a 1-based 'reg' property. This seemed
to be the preferred binding based on [1]. Since leds-sn3216 has not been
in a released kernel, there is are no backwards-compatibility concerns.

Changes from RFC:
 new

[1] http://www.spinics.net/lists/linux-leds/msg05589.html

 .../devicetree/bindings/leds/leds-is31fl32xx.txt   |   9 +-
 .../devicetree/bindings/leds/leds-sn3218.txt   |  41 ---
 drivers/leds/Kconfig   |  18 +-
 drivers/leds/Makefile  |   1 -
 drivers/leds/leds-is31fl32xx.c |   6 +-
 drivers/leds/leds-sn3218.c | 306 -
 6 files changed, 14 insertions(+), 367 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/leds/leds-sn3218.txt
 delete mode 100644 drivers/leds/leds-sn3218.c

diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt 
b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
index 539df2e..c59eb1a 100644
--- a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
+++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
@@ -1,6 +1,6 @@
-Binding for ISSI IS31FL32xx LED Drivers
+Binding for ISSI IS31FL32xx and Si-En SN32xx LED Drivers
 
-The IS31FL32xx family of LED drivers are I2C devices with multiple
+The IS31FL32xx/SN32xx family of LED drivers are I2C devices with multiple
 constant-current channels, each with independent 256-level PWM control.
 Each LED is represented as a sub-node of the device.
 
@@ -10,6 +10,8 @@ Required properties:
issi,is31fl3235
issi,is31fl3218
issi,is31fl3216
+   si-en,sn3218
+   si-en,sn3216
 - reg: I2C slave address
 - address-cells : must be 1
 - size-cells : must be 0
@@ -45,5 +47,6 @@ leds: is31fl3236@3c {
};
 };
 
-For more product information please see the link below:
+For more product information please see the links below:
 http://www.issi.com/US/product-analog-fxled-driver.shtml
+http://www.si-en.com/product.asp?parentid=890
diff --git a/Documentation/devicetree/bindings/leds/leds-sn3218.txt 
b/Documentation/devicetree/bindings/leds/leds-sn3218.txt
deleted file mode 100644
index 19cbf57..000
--- a/Documentation/devicetree/bindings/leds/leds-sn3218.txt
+++ /dev/null
@@ -1,41 +0,0 @@
-* Si-En Technology - SN3218 18-Channel LED Driver
-
-Required properties:
-- compatible :
-   "si-en,sn3218"
-- address-cells : must be 1
-- size-cells : must be 0
-- reg : I2C slave address, typically 0x54
-
-There must be at least 1 LED which is represented as a sub-node
-of the sn3218 device.
-
-LED sub-node properties:
-- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
-- reg : number of LED line (could be from 0 to 17)
-- linux,default-trigger : (optional)
-   see Documentation/devicetree/bindings/leds/common.txt
-
-Example:
-
-sn3218: led-controller@54 {
-   compatible = "si-en,sn3218";
-   #address-cells = <1>;
-   #size-cells = <0>;
-   reg = <0x54>;
-
-   led@0 {
-   label = "led1";
-   reg = <0x0>;
-   };
-
-   led@1 {
-   label = "led2";
-   reg = <0x1>;
-   };
-
-   led@2 {
-   label = "led3";
-   reg = <0x2>;
-   };
-};
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9c63ba4..1f64151 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -568,25 +568,13 @@ config LEDS_SEAD3
  This driver can also be built as a module. If so the module
  will be called leds-sead3.
 
-config LEDS_SN3218
-   tristate "LED support for Si-En SN3218 I2C chip"
-   depends on LEDS_CLASS && I2C
-   depends on OF
-   select REGMAP_I2C
-   help
- This option enables support for the Si-EN SN3218 LED driver
- connected through I2C. Say Y to enable support for the SN3218 LED.
-
- This driver can also be built as a module. If so the module
- will be called leds-sn3218.
-
 config LEDS_IS31FL32XX
tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
depends on LEDS_CLASS && I2C && OF
help
- Say Y here 

[PATCH 0/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

From: David Rivshin 

This series adds support for the ISSI IS31FL32xx family of I2C LED
controllers. Since the IS31FL3218 is actually the same device as the
SN3218, the dedicated leds-sn3218 driver is removed and the compatible
string is folded into this driver.

Changes from RFC [1]:
 - Removed max-brightness DT property.
 - Added #address-cells and #size-cells properties to the example DT.
 - Refer to these devices as "LED controllers" in Kconfig.
 - Removed redundant last sentence from Kconfig entry
 - Removed unnecessary debug code.
 - Do not set led_classdev.brightness to 0 explicitly, as it is
   already initialized to 0 by devm_kzalloc().
 - Used of_property_read_string() instead of of_get_property().
 - Fail immediately on DT parsing error in a child node, rather than
   continuing on with the non-faulty ones.
 - Added additional comments for some things that might be non-obvious.
 - Added constants for the location of the SSD bit in the SHUTDOWN
   register, and the 3216's CONFIG register.
 - Added special sw_shutdown_func for the 3216 device, as that bit
   is in a different register, at a different position, and has reverse
   polarity compared to all the other devices.
 - Refactored is31fl32xx_init_regs() to separate out some logic into
   is31fl32xx_reset_regs() and is31fl32xx_software_shutdown().
 - Added 4th patch to replace the now-redundant leds-sn3218.

[1] http://www.spinics.net/lists/linux-leds/msg05564.html
http://thread.gmane.org/gmane.linux.leds/4530

David Rivshin (4):
  DT: Add vendor prefix for Integrated Silicon Solutions Inc.
  DT: leds: Add binding for the ISSI IS31FL32xx family of LED
controllers
  leds: Add driver for the ISSI IS31FL32xx family of LED controllers
  leds: Replace dedicated SN3218 driver with IS31FL32XX driver

 .../devicetree/bindings/leds/leds-is31fl32xx.txt   |  52 +++
 .../devicetree/bindings/leds/leds-sn3218.txt   |  41 --
 .../devicetree/bindings/vendor-prefixes.txt|   1 +
 drivers/leds/Kconfig   |  16 +-
 drivers/leds/Makefile  |   2 +-
 drivers/leds/leds-is31fl32xx.c | 509 +
 drivers/leds/leds-sn3218.c | 306 -
 7 files changed, 569 insertions(+), 358 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
 delete mode 100644 Documentation/devicetree/bindings/leds/leds-sn3218.txt
 create mode 100644 drivers/leds/leds-is31fl32xx.c
 delete mode 100644 drivers/leds/leds-sn3218.c

-- 
2.5.0

[PATCH 0/4] leds: Add driver for the ISSI IS31FL32xx family of LED controllers

[David Rivshin (Allworx)]

From: David Rivshin 

This series adds support for the ISSI IS31FL32xx family of I2C LED
controllers. Since the IS31FL3218 is actually the same device as the
SN3218, the dedicated leds-sn3218 driver is removed and the compatible
string is folded into this driver.

Changes from RFC [1]:
 - Removed max-brightness DT property.
 - Added #address-cells and #size-cells properties to the example DT.
 - Refer to these devices as "LED controllers" in Kconfig.
 - Removed redundant last sentence from Kconfig entry
 - Removed unnecessary debug code.
 - Do not set led_classdev.brightness to 0 explicitly, as it is
   already initialized to 0 by devm_kzalloc().
 - Used of_property_read_string() instead of of_get_property().
 - Fail immediately on DT parsing error in a child node, rather than
   continuing on with the non-faulty ones.
 - Added additional comments for some things that might be non-obvious.
 - Added constants for the location of the SSD bit in the SHUTDOWN
   register, and the 3216's CONFIG register.
 - Added special sw_shutdown_func for the 3216 device, as that bit
   is in a different register, at a different position, and has reverse
   polarity compared to all the other devices.
 - Refactored is31fl32xx_init_regs() to separate out some logic into
   is31fl32xx_reset_regs() and is31fl32xx_software_shutdown().
 - Added 4th patch to replace the now-redundant leds-sn3218.

[1] http://www.spinics.net/lists/linux-leds/msg05564.html
http://thread.gmane.org/gmane.linux.leds/4530

David Rivshin (4):
  DT: Add vendor prefix for Integrated Silicon Solutions Inc.
  DT: leds: Add binding for the ISSI IS31FL32xx family of LED
controllers
  leds: Add driver for the ISSI IS31FL32xx family of LED controllers
  leds: Replace dedicated SN3218 driver with IS31FL32XX driver

 .../devicetree/bindings/leds/leds-is31fl32xx.txt   |  52 +++
 .../devicetree/bindings/leds/leds-sn3218.txt   |  41 --
 .../devicetree/bindings/vendor-prefixes.txt|   1 +
 drivers/leds/Kconfig   |  16 +-
 drivers/leds/Makefile  |   2 +-
 drivers/leds/leds-is31fl32xx.c | 509 +
 drivers/leds/leds-sn3218.c | 306 -
 7 files changed, 569 insertions(+), 358 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
 delete mode 100644 Documentation/devicetree/bindings/leds/leds-sn3218.txt
 create mode 100644 drivers/leds/leds-is31fl32xx.c
 delete mode 100644 drivers/leds/leds-sn3218.c

-- 
2.5.0

[PATCH] of: add 'const' for of_property_*_string*() parameter '*np'

[David Rivshin (Allworx)]

From: David Rivshin 

The of_property_{read,count,match}_string* family of functions never
modify the struct device_node pointer that is passed in, so there is no
reason for it to be non-const. Equivalent functions for all other types
already take a 'const struct device_node *np'.

Signed-off-by: David Rivshin 
---

MAINTAINTERS says that the appropriate tree is
git://git.kernel.org/pub/scm/linux/kernel/git/glikely/linux.git
but it looks like that hasn't been updated in a while. So this patch
is based off the "for-next" branch of
git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
instead. Let me know if you need me to respin from another tree/branch.

 drivers/of/base.c  | 15 ---
 include/linux/of.h | 18 +-
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 017dd94..b299de2 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1341,10 +1341,10 @@ EXPORT_SYMBOL_GPL(of_property_read_u64_array);
  *
  * The out_string pointer is modified only if a valid string can be decoded.
  */
-int of_property_read_string(struct device_node *np, const char *propname,
+int of_property_read_string(const struct device_node *np, const char *propname,
const char **out_string)
 {
-   struct property *prop = of_find_property(np, propname, NULL);
+   const struct property *prop = of_find_property(np, propname, NULL);
if (!prop)
return -EINVAL;
if (!prop->value)
@@ -1365,10 +1365,10 @@ EXPORT_SYMBOL_GPL(of_property_read_string);
  * This function searches a string list property and returns the index
  * of a specific string value.
  */
-int of_property_match_string(struct device_node *np, const char *propname,
+int of_property_match_string(const struct device_node *np, const char 
*propname,
 const char *string)
 {
-   struct property *prop = of_find_property(np, propname, NULL);
+   const struct property *prop = of_find_property(np, propname, NULL);
size_t l;
int i;
const char *p, *end;
@@ -1404,10 +1404,11 @@ EXPORT_SYMBOL_GPL(of_property_match_string);
  * Don't call this function directly. It is a utility helper for the
  * of_property_read_string*() family of functions.
  */
-int of_property_read_string_helper(struct device_node *np, const char 
*propname,
-  const char **out_strs, size_t sz, int skip)
+int of_property_read_string_helper(const struct device_node *np,
+  const char *propname, const char **out_strs,
+  size_t sz, int skip)
 {
-   struct property *prop = of_find_property(np, propname, NULL);
+   const struct property *prop = of_find_property(np, propname, NULL);
int l = 0, i = 0;
const char *p, *end;
 
diff --git a/include/linux/of.h b/include/linux/of.h
index dc6e396..7fcb681 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -296,13 +296,13 @@ extern int of_property_read_u64_array(const struct 
device_node *np,
  u64 *out_values,
  size_t sz);
 
-extern int of_property_read_string(struct device_node *np,
+extern int of_property_read_string(const struct device_node *np,
   const char *propname,
   const char **out_string);
-extern int of_property_match_string(struct device_node *np,
+extern int of_property_match_string(const struct device_node *np,
const char *propname,
const char *string);
-extern int of_property_read_string_helper(struct device_node *np,
+extern int of_property_read_string_helper(const struct device_node *np,
  const char *propname,
  const char **out_strs, size_t sz, 
int index);
 extern int of_device_is_compatible(const struct device_node *device,
@@ -538,14 +538,14 @@ static inline int of_property_read_u64_array(const struct 
device_node *np,
return -ENOSYS;
 }
 
-static inline int of_property_read_string(struct device_node *np,
+static inline int of_property_read_string(const struct device_node *np,
  const char *propname,
  const char **out_string)
 {
return -ENOSYS;
 }
 
-static inline int of_property_read_string_helper(struct device_node *np,
+static inline int of_property_read_string_helper(const struct device_node *np,
 const char *propname,
 const char **out_strs, size_t 
sz, int index)
 {
@@ -571,7 +571,7 @@ static inline int of_property_read_u64(const struct 
device_node *np,
return 

[PATCH] of: add 'const' for of_property_*_string*() parameter '*np'

[David Rivshin (Allworx)]

From: David Rivshin 

The of_property_{read,count,match}_string* family of functions never
modify the struct device_node pointer that is passed in, so there is no
reason for it to be non-const. Equivalent functions for all other types
already take a 'const struct device_node *np'.

Signed-off-by: David Rivshin 
---

MAINTAINTERS says that the appropriate tree is
git://git.kernel.org/pub/scm/linux/kernel/git/glikely/linux.git
but it looks like that hasn't been updated in a while. So this patch
is based off the "for-next" branch of
git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
instead. Let me know if you need me to respin from another tree/branch.

 drivers/of/base.c  | 15 ---
 include/linux/of.h | 18 +-
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 017dd94..b299de2 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1341,10 +1341,10 @@ EXPORT_SYMBOL_GPL(of_property_read_u64_array);
  *
  * The out_string pointer is modified only if a valid string can be decoded.
  */
-int of_property_read_string(struct device_node *np, const char *propname,
+int of_property_read_string(const struct device_node *np, const char *propname,
const char **out_string)
 {
-   struct property *prop = of_find_property(np, propname, NULL);
+   const struct property *prop = of_find_property(np, propname, NULL);
if (!prop)
return -EINVAL;
if (!prop->value)
@@ -1365,10 +1365,10 @@ EXPORT_SYMBOL_GPL(of_property_read_string);
  * This function searches a string list property and returns the index
  * of a specific string value.
  */
-int of_property_match_string(struct device_node *np, const char *propname,
+int of_property_match_string(const struct device_node *np, const char 
*propname,
 const char *string)
 {
-   struct property *prop = of_find_property(np, propname, NULL);
+   const struct property *prop = of_find_property(np, propname, NULL);
size_t l;
int i;
const char *p, *end;
@@ -1404,10 +1404,11 @@ EXPORT_SYMBOL_GPL(of_property_match_string);
  * Don't call this function directly. It is a utility helper for the
  * of_property_read_string*() family of functions.
  */
-int of_property_read_string_helper(struct device_node *np, const char 
*propname,
-  const char **out_strs, size_t sz, int skip)
+int of_property_read_string_helper(const struct device_node *np,
+  const char *propname, const char **out_strs,
+  size_t sz, int skip)
 {
-   struct property *prop = of_find_property(np, propname, NULL);
+   const struct property *prop = of_find_property(np, propname, NULL);
int l = 0, i = 0;
const char *p, *end;
 
diff --git a/include/linux/of.h b/include/linux/of.h
index dc6e396..7fcb681 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -296,13 +296,13 @@ extern int of_property_read_u64_array(const struct 
device_node *np,
  u64 *out_values,
  size_t sz);
 
-extern int of_property_read_string(struct device_node *np,
+extern int of_property_read_string(const struct device_node *np,
   const char *propname,
   const char **out_string);
-extern int of_property_match_string(struct device_node *np,
+extern int of_property_match_string(const struct device_node *np,
const char *propname,
const char *string);
-extern int of_property_read_string_helper(struct device_node *np,
+extern int of_property_read_string_helper(const struct device_node *np,
  const char *propname,
  const char **out_strs, size_t sz, 
int index);
 extern int of_device_is_compatible(const struct device_node *device,
@@ -538,14 +538,14 @@ static inline int of_property_read_u64_array(const struct 
device_node *np,
return -ENOSYS;
 }
 
-static inline int of_property_read_string(struct device_node *np,
+static inline int of_property_read_string(const struct device_node *np,
  const char *propname,
  const char **out_string)
 {
return -ENOSYS;
 }
 
-static inline int of_property_read_string_helper(struct device_node *np,
+static inline int of_property_read_string_helper(const struct device_node *np,
 const char *propname,
 const char **out_strs, size_t 
sz, int index)
 {
@@ -571,7 +571,7 @@ static inline int of_property_read_u64(const struct 
device_node *np,
return -ENOSYS;
 }
 
-static inline int 

Re: [alsa-devel] [PATCH] ALSA: oss: consolidate kmalloc/memset 0 call to kzalloc

[David Rivshin (Allworx)]

On Fri, 18 Dec 2015 21:01:57 +0100
Nicholas Mc Guire  wrote:

> This is an API consolidation only. The use of kmalloc + memset to 0
> is equivalent to kzalloc.
> 
> Signed-off-by: Nicholas Mc Guire 
> ---
> 
> Found by coccinelle script (relaxed version of
> scripts/coccinelle/api/alloc/kzalloc-simple.cocci)
> 
> Patch was compile tested with: x86_64_defconfig
> CONFIG_SND_PCM_OSS=y
> 
> Patch is against linux-next (localversion-next is -next-20150518)
> 
> diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
> index e557dbe..8cdd06f 100644
> --- a/sound/core/oss/pcm_oss.c
> +++ b/sound/core/oss/pcm_oss.c
> @@ -850,7 +850,7 @@ static int snd_pcm_oss_change_params(struct
> snd_pcm_substream *substream) 
>   if (mutex_lock_interruptible(>oss.params_lock))
>   return -EINTR;
> - sw_params = kmalloc(sizeof(*sw_params), GFP_KERNEL);
> + sw_params = kmalloc(sizeof(*sw_params), GFP_KERNEL);
 ^
Something odd (manual edit?) seems to have happened to this hunk, 
making it a no-op. I presume that 'm' was supposed to be a 'z'.

>   params = kmalloc(sizeof(*params), GFP_KERNEL);
>   sparams = kmalloc(sizeof(*sparams), GFP_KERNEL);
>   if (!sw_params || !params || !sparams) {
> @@ -988,7 +988,6 @@ static int snd_pcm_oss_change_params(struct
> snd_pcm_substream *substream) goto failure;
>   }
>  
> - memset(sw_params, 0, sizeof(*sw_params));
>   if (runtime->oss.trigger) {
>   sw_params->start_threshold = 1;
>   } else {

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [alsa-devel] [PATCH] ALSA: oss: consolidate kmalloc/memset 0 call to kzalloc

[David Rivshin (Allworx)]

On Fri, 18 Dec 2015 21:01:57 +0100
Nicholas Mc Guire  wrote:

> This is an API consolidation only. The use of kmalloc + memset to 0
> is equivalent to kzalloc.
> 
> Signed-off-by: Nicholas Mc Guire 
> ---
> 
> Found by coccinelle script (relaxed version of
> scripts/coccinelle/api/alloc/kzalloc-simple.cocci)
> 
> Patch was compile tested with: x86_64_defconfig
> CONFIG_SND_PCM_OSS=y
> 
> Patch is against linux-next (localversion-next is -next-20150518)
> 
> diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
> index e557dbe..8cdd06f 100644
> --- a/sound/core/oss/pcm_oss.c
> +++ b/sound/core/oss/pcm_oss.c
> @@ -850,7 +850,7 @@ static int snd_pcm_oss_change_params(struct
> snd_pcm_substream *substream) 
>   if (mutex_lock_interruptible(>oss.params_lock))
>   return -EINTR;
> - sw_params = kmalloc(sizeof(*sw_params), GFP_KERNEL);
> + sw_params = kmalloc(sizeof(*sw_params), GFP_KERNEL);
 ^
Something odd (manual edit?) seems to have happened to this hunk, 
making it a no-op. I presume that 'm' was supposed to be a 'z'.

>   params = kmalloc(sizeof(*params), GFP_KERNEL);
>   sparams = kmalloc(sizeof(*sparams), GFP_KERNEL);
>   if (!sw_params || !params || !sparams) {
> @@ -988,7 +988,6 @@ static int snd_pcm_oss_change_params(struct
> snd_pcm_substream *substream) goto failure;
>   }
>  
> - memset(sw_params, 0, sizeof(*sw_params));
>   if (runtime->oss.trigger) {
>   sw_params->start_threshold = 1;
>   } else {

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/