[PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[Sergio Paracuellos]

In this mode the switch device and the internal phys will be managed via
I2C interface.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/net/dsa/microchip/Kconfig       |   6 +
 drivers/net/dsa/microchip/Makefile      |   1 +
 drivers/net/dsa/microchip/ksz9477_i2c.c | 258 ++++++++++++++++++++++++
 3 files changed, 265 insertions(+)
 create mode 100644 drivers/net/dsa/microchip/ksz9477_i2c.c

diff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig
index a8caf9249d50..162fec43d204 100644
--- a/drivers/net/dsa/microchip/Kconfig
+++ b/drivers/net/dsa/microchip/Kconfig
@@ -14,3 +14,9 @@ config NET_DSA_MICROCHIP_KSZ9477_SPI
 	depends on NET_DSA_MICROCHIP_KSZ9477 && SPI
 	help
 	  Select to enable support for registering switches configured through SPI.
+
+config NET_DSA_MICROCHIP_KSZ9477_I2C
+	tristate "KSZ series I2C connected switch driver"
+	depends on NET_DSA_MICROCHIP_KSZ9477 && I2C
+	help
+	  Select to enable support for registering switches configured through I2C.
diff --git a/drivers/net/dsa/microchip/Makefile b/drivers/net/dsa/microchip/Makefile
index 3142c18b8f57..eb9b768face2 100644
--- a/drivers/net/dsa/microchip/Makefile
+++ b/drivers/net/dsa/microchip/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON)	+= ksz_common.o
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477)		+= ksz9477.o
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_SPI)	+= ksz9477_spi.o
+obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_I2C)	+= ksz9477_i2c.o
diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
new file mode 100644
index 000000000000..2b88d66e8c20
--- /dev/null
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -0,0 +1,258 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Microchip KSZ series register access through I2C
+ *
+ * Author: Sergio Paracuellos <sergio.paracuellos@gmail.com>
+ */
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include "ksz_priv.h"
+
+/**
+ * ksz_i2c_read_reg - issue read register command
+ * @client: i2c client structure
+ * @reg: The register address.
+ * @val: The buffer to return the result into.
+ * @len: The length of data expected.
+ *
+ * This is the low level read call that issues the necessary i2c message(s)
+ * to read data from the register specified in @reg.
+ */
+static int ksz_i2c_read_reg(struct i2c_client *client, u32 reg, u8 *val,
+			    unsigned int len)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	struct i2c_msg msg[2];
+	u8 txd[2];
+	int ret;
+
+	txd[0] = (u8)(reg >> 8);
+	txd[1] = (u8)reg;
+
+	msg[0].addr = client->addr;
+	msg[0].flags = 0;
+	msg[0].len = 2;
+	msg[0].buf = txd;
+
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = len;
+	msg[1].buf = val;
+
+	ret = i2c_transfer(adapter, msg, 2);
+	return (ret != 2) ? -EREMOTEIO : 0;
+}
+
+static int ksz_i2c_read(struct ksz_device *dev, u32 reg, u8 *data,
+			unsigned int len)
+{
+	struct i2c_client *client = dev->priv;
+
+	return ksz_i2c_read_reg(client, reg, data, len);
+}
+
+static int ksz_i2c_read8(struct ksz_device *dev, u32 reg, u8 *val)
+{
+	return ksz_i2c_read(dev, reg, val, 1);
+}
+
+static int ksz_i2c_read16(struct ksz_device *dev, u32 reg, u16 *val)
+{
+	int ret = ksz_i2c_read(dev, reg, (u8 *)val, 2);
+
+	if (!ret)
+		*val = be16_to_cpu(*val);
+
+	return ret;
+}
+
+static int ksz_i2c_read24(struct ksz_device *dev, u32 reg, u32 *val)
+{
+	int ret;
+
+	*val = 0;
+	ret = ksz_i2c_read(dev, reg, (u8 *)val, 3);
+
+	if (!ret) {
+		*val = be32_to_cpu(*val);
+		/* convert to 24 bit */
+		*val >>= 8;
+	}
+
+	return ret;
+}
+
+static int ksz_i2c_read32(struct ksz_device *dev, u32 reg, u32 *val)
+{
+	int ret = ksz_i2c_read(dev, reg, (u8 *)val, 4);
+
+	if (!ret)
+		*val = be32_to_cpu(*val);
+
+	return ret;
+}
+
+/**
+ * ksz_i2c_write_reg - issue write register command
+ * @client: i2c client structure
+ * @reg: The register address.
+ * @val: value to write
+ * @len: The length of data
+ *
+ * This is the low level write call that issues the necessary i2c message(s)
+ * to write data to the register specified in @reg.
+ */
+static int ksz_i2c_write_reg(struct i2c_client *client, u32 reg, u8 *val,
+			     unsigned int len)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	struct i2c_msg msg;
+	u8 txd[32];
+	int ret;
+
+	txd[0] = (u8)(reg >> 8);
+	txd[1] = (u8)reg;
+	memcpy(&txd[2], val, len);
+
+	msg.addr = client->addr;
+	msg.flags = 0;
+	msg.len = 2 + len;
+	msg.buf = txd;
+
+	ret = i2c_transfer(adapter, &msg, 1);
+	return (ret != 1) ? -EREMOTEIO : 0;
+}
+
+static int ksz_i2c_write(struct ksz_device *dev, u32 reg, u8 *val,
+			 unsigned int len)
+{
+	struct i2c_client *client = dev->priv;
+	unsigned int cnt = len;
+	int i = 0;
+	u8 txb[4];
+
+	do {
+		txb[i++] = (u8)(*val >> (8 * (cnt - 1)));
+		cnt--;
+	} while (cnt);
+
+	return ksz_i2c_write_reg(client, reg, txb, len);
+}
+
+static int ksz_i2c_write8(struct ksz_device *dev, u32 reg, u8 value)
+{
+	return ksz_i2c_write(dev, reg, &value, 1);
+}
+
+static int ksz_i2c_write16(struct ksz_device *dev, u32 reg, u16 value)
+{
+	value = cpu_to_be16(value);
+	return ksz_i2c_write(dev, reg, (u8 *)&value, 2);
+}
+
+static int ksz_i2c_write24(struct ksz_device *dev, u32 reg, u32 value)
+{
+	/* make it to big endian 24bit from MSB */
+	value <<= 8;
+	value = cpu_to_be32(value);
+
+	return ksz_i2c_write(dev, reg, (u8 *)&value, 3);
+}
+
+static int ksz_i2c_write32(struct ksz_device *dev, u32 reg, u32 value)
+{
+	value = cpu_to_be32(value);
+	return ksz_i2c_write(dev, reg, (u8 *)&value, 4);
+}
+
+static int ksz_i2c_get(struct ksz_device *dev, u32 reg, void *data, size_t len)
+{
+	return ksz_i2c_read(dev, reg, data, len);
+}
+
+static int ksz_i2c_set(struct ksz_device *dev, u32 reg, void *data, size_t len)
+{
+	return ksz_i2c_write(dev, reg, data, len);
+}
+
+static const struct ksz_io_ops ksz_i2c_ops = {
+	.read8 = ksz_i2c_read8,
+	.read16 = ksz_i2c_read16,
+	.read24 = ksz_i2c_read24,
+	.read32 = ksz_i2c_read32,
+	.write8 = ksz_i2c_write8,
+	.write16 = ksz_i2c_write16,
+	.write24 = ksz_i2c_write24,
+	.write32 = ksz_i2c_write32,
+	.get = ksz_i2c_get,
+	.set = ksz_i2c_set,
+};
+
+static int ksz_i2c_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct ksz_device *dev;
+	int ret;
+
+	dev = ksz_switch_alloc(&client->dev, &ksz_i2c_ops, client);
+	if (!dev)
+		return -ENOMEM;
+
+	if (client->dev.platform_data)
+		dev->pdata = client->dev.platform_data;
+
+	i2c_set_clientdata(client, dev);
+
+	ret = ksz9477_switch_register(dev);
+	if (ret) {
+		dev_err(&client->dev, "registering switch (ret: %d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ksz_i2c_remove(struct i2c_client *client)
+{
+	struct ksz_device *dev = i2c_get_clientdata(client);
+
+	if (dev)
+		ksz_switch_remove(dev);
+
+	return 0;
+}
+
+static const struct i2c_device_id ksz_i2c_id[] = {
+	{ "ksz9477-switch", 0 },
+	{},
+};
+
+MODULE_DEVICE_TABLE(i2c, ksz_i2c_id);
+
+static const struct of_device_id ksz_dt_match[] = {
+	{ .compatible = "microchip,ksz9477" },
+	{ .compatible = "microchip,ksz9897" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, ksz_dt_match);
+
+static struct i2c_driver ksz_i2c_driver = {
+	.driver = {
+		.name   = "ksz9477-switch",
+		.owner  = THIS_MODULE,
+		.of_match_table = ksz_dt_match,
+	},
+	.probe  = ksz_i2c_probe,
+	.remove = ksz_i2c_remove,
+	.id_table = ksz_i2c_id,
+};
+
+module_i2c_driver(ksz_i2c_driver);
+
+MODULE_AUTHOR("Sergio Paracuellos <sergio.paracuellos@gmail.com>");
+MODULE_DESCRIPTION("Microchip KSZ Series Switch I2C access Driver");
+MODULE_LICENSE("GPL");
-- 
2.19.1

[PATCH 2/2] dt-bindings: net: dsa: ksz9477: add sample of switch bindings managed in i2c mode

[Sergio Paracuellos]

Add device-tree binding example of the ksz9477 switch managed in i2c mode.

Cc: devicetree@vger.kernel.org
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 .../devicetree/bindings/net/dsa/ksz.txt       | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt b/Documentation/devicetree/bindings/net/dsa/ksz.txt
index 0f407fb371ce..9e715358cebb 100644
--- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
+++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
@@ -74,3 +74,53 @@ Ethernet switch connected via SPI to the host, CPU port wired to eth0:
                                                              };
                                              };
                              };
+
+Ethernet switch connected via I2C to the host, CPU port wired to eth0:
+
+                             eth0: ethernet@10001000 {
+                                             fixed-link {
+                                                             speed = <1000>;
+                                                             full-duplex;
+                                             };
+                             };
+
+                             i2c0: i2c@f8008000 {
+                                             ksz9897: ksz9897@0 {
+                                                             compatible = "microchip,ksz9897";
+                                                             reg = <5f>;
+
+                                                             ports {
+                                                                             #address-cells = <1>;
+                                                                             #size-cells = <0>;
+                                                                             port@0 {
+                                                                                             reg = <0>;
+                                                                                             label = "lan1";
+                                                                             };
+                                                                             port@1 {
+                                                                                             reg = <1>;
+                                                                                             label = "lan2";
+                                                                             };
+                                                                             port@2 {
+                                                                                             reg = <2>;
+                                                                                             label = "lan3";
+                                                                             };
+                                                                             port@3 {
+                                                                                             reg = <3>;
+                                                                                             label = "lan4";
+                                                                             };
+                                                                             port@4 {
+                                                                                             reg = <4>;
+                                                                                             label = "lan5";
+                                                                             };
+                                                                             port@6 {
+                                                                                             reg = <6>;
+                                                                                             label = "cpu";
+                                                                                             ethernet = <&eth0>;
+                                                                                             fixed-link {
+                                                                                                             speed = <1000>;
+                                                                                                             full-duplex;
+                                                                                             };
+                                                                             };
+                                                             };
+                                             };
+                             };
-- 
2.19.1

Re: [PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[Andrew Lunn]

On Sun, Dec 16, 2018 at 08:57:40AM +0100, Sergio Paracuellos wrote:
> +static int ksz_i2c_read_reg(struct i2c_client *client, u32 reg, u8 *val,
> +			    unsigned int len)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +	struct i2c_msg msg[2];
> +	u8 txd[2];

Hi Sergio

I'm not sure that having the TX buffer on the stack is safe. If the
i2c bus master is using DMA, you then DMA from the stack, which some
architectures memory models do no allow. You have to use memory which
comes from an alloc function.

> +	int ret;
> +
> +	txd[0] = (u8)(reg >> 8);
> +	txd[1] = (u8)reg;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].flags = 0;
> +	msg[0].len = 2;
> +	msg[0].buf = txd;
> +
> +	msg[1].addr = client->addr;
> +	msg[1].flags = I2C_M_RD;
> +	msg[1].len = len;
> +	msg[1].buf = val;

You potentially have the same issue with val.

    Andrew

Re: [PATCH 2/2] dt-bindings: net: dsa: ksz9477: add sample of switch bindings managed in i2c mode

[Andrew Lunn]

On Sun, Dec 16, 2018 at 08:57:41AM +0100, Sergio Paracuellos wrote:
> Add device-tree binding example of the ksz9477 switch managed in i2c mode.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  .../devicetree/bindings/net/dsa/ksz.txt       | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> index 0f407fb371ce..9e715358cebb 100644
> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> @@ -74,3 +74,53 @@ Ethernet switch connected via SPI to the host, CPU port wired to eth0:
>                                                               };
>                                               };
>                               };
> +
> +Ethernet switch connected via I2C to the host, CPU port wired to eth0:
> +
> +                             eth0: ethernet@10001000 {
> +                                             fixed-link {
> +                                                             speed = <1000>;
> +                                                             full-duplex;
> +                                             };
> +                             };
> +
> +                             i2c0: i2c@f8008000 {
> +                                             ksz9897: ksz9897@0 {

Hi Sergio

You should use a generic name here. Plus the @X needs to be the same as the reg value.
So switch: ksz9897@5f.

   Andrew

Re: [PATCH 2/2] dt-bindings: net: dsa: ksz9477: add sample of switch bindings managed in i2c mode

[Sergio Paracuellos]

On Sun, Dec 16, 2018 at 9:18 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, Dec 16, 2018 at 08:57:41AM +0100, Sergio Paracuellos wrote:
> > Add device-tree binding example of the ksz9477 switch managed in i2c mode.
> >
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  .../devicetree/bindings/net/dsa/ksz.txt       | 50 +++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> > index 0f407fb371ce..9e715358cebb 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> > +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> > @@ -74,3 +74,53 @@ Ethernet switch connected via SPI to the host, CPU port wired to eth0:
> >                                                               };
> >                                               };
> >                               };
> > +
> > +Ethernet switch connected via I2C to the host, CPU port wired to eth0:
> > +
> > +                             eth0: ethernet@10001000 {
> > +                                             fixed-link {
> > +                                                             speed = <1000>;
> > +                                                             full-duplex;
> > +                                             };
> > +                             };
> > +
> > +                             i2c0: i2c@f8008000 {
> > +                                             ksz9897: ksz9897@0 {
>
> Hi Sergio
>

Hi Andrew,

Thanks for such a fast review :-)

> You should use a generic name here. Plus the @X needs to be the same as the reg value.
> So switch: ksz9897@5f.

Ok, I'll change this. Copy paste is bad friend sometimes.

>
>    Andrew

Best regards,
    Sergio Paracuellos

Re: [PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[Sergio Paracuellos]

On Sun, Dec 16, 2018 at 9:15 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, Dec 16, 2018 at 08:57:40AM +0100, Sergio Paracuellos wrote:
> > +static int ksz_i2c_read_reg(struct i2c_client *client, u32 reg, u8 *val,
> > +                         unsigned int len)
> > +{
> > +     struct i2c_adapter *adapter = client->adapter;
> > +     struct i2c_msg msg[2];
> > +     u8 txd[2];
>

Hi Andrew,

> Hi Sergio
>
> I'm not sure that having the TX buffer on the stack is safe. If the
> i2c bus master is using DMA, you then DMA from the stack, which some
> architectures memory models do no allow. You have to use memory which
> comes from an alloc function.
>
> > +     int ret;
> > +
> > +     txd[0] = (u8)(reg >> 8);
> > +     txd[1] = (u8)reg;
> > +
> > +     msg[0].addr = client->addr;
> > +     msg[0].flags = 0;
> > +     msg[0].len = 2;
> > +     msg[0].buf = txd;
> > +
> > +     msg[1].addr = client->addr;
> > +     msg[1].flags = I2C_M_RD;
> > +     msg[1].len = len;
> > +     msg[1].buf = val;
>
> You potentially have the same issue with val.
>

I'll change these two to to get memory from kernel allocators instead
of using the stack. Thanks for let me know this.

>     Andrew

Best regards,
    Sergio Paracuellos

Re: [PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[Andrew Lunn]

> I'll change these two to to get memory from kernel allocators instead
> of using the stack. Thanks for let me know this.

There appear to be other cases as well. Please review the whole
driver.

	Thanks
		Andrew

Re: [PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[Dan Carpenter]

On Sun, Dec 16, 2018 at 08:57:40AM +0100, Sergio Paracuellos wrote:
> +static int ksz_i2c_write(struct ksz_device *dev, u32 reg, u8 *val,
> +			 unsigned int len)
> +{
> +	struct i2c_client *client = dev->priv;
> +	unsigned int cnt = len;
> +	int i = 0;
> +	u8 txb[4];
> +
> +	do {
> +		txb[i++] = (u8)(*val >> (8 * (cnt - 1)));
                                              ^^^^^^^

Can "cnt" be zero from ksz_i2c_set()?  If so this loop will corrupt
memory.

> +		cnt--;
> +	} while (cnt);
> +
> +	return ksz_i2c_write_reg(client, reg, txb, len);
> +}
> +
> +static int ksz_i2c_write8(struct ksz_device *dev, u32 reg, u8 value)
> +{
> +	return ksz_i2c_write(dev, reg, &value, 1);
> +}
> +
> +static int ksz_i2c_write16(struct ksz_device *dev, u32 reg, u16 value)
> +{
> +	value = cpu_to_be16(value);
> +	return ksz_i2c_write(dev, reg, (u8 *)&value, 2);
> +}
> +
> +static int ksz_i2c_write24(struct ksz_device *dev, u32 reg, u32 value)
> +{
> +	/* make it to big endian 24bit from MSB */
> +	value <<= 8;
> +	value = cpu_to_be32(value);
> +
> +	return ksz_i2c_write(dev, reg, (u8 *)&value, 3);
> +}
> +
> +static int ksz_i2c_write32(struct ksz_device *dev, u32 reg, u32 value)
> +{
> +	value = cpu_to_be32(value);
> +	return ksz_i2c_write(dev, reg, (u8 *)&value, 4);
> +}
> +
> +static int ksz_i2c_get(struct ksz_device *dev, u32 reg, void *data, size_t len)
> +{
> +	return ksz_i2c_read(dev, reg, data, len);
> +}
> +
> +static int ksz_i2c_set(struct ksz_device *dev, u32 reg, void *data, size_t len)
> +{
> +	return ksz_i2c_write(dev, reg, data, len);
> +}
> +
> +static const struct ksz_io_ops ksz_i2c_ops = {
> +	.read8 = ksz_i2c_read8,
> +	.read16 = ksz_i2c_read16,
> +	.read24 = ksz_i2c_read24,
> +	.read32 = ksz_i2c_read32,
> +	.write8 = ksz_i2c_write8,
> +	.write16 = ksz_i2c_write16,
> +	.write24 = ksz_i2c_write24,
> +	.write32 = ksz_i2c_write32,
> +	.get = ksz_i2c_get,
> +	.set = ksz_i2c_set,
> +};
> +
> +static int ksz_i2c_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct ksz_device *dev;
> +	int ret;
> +
> +	dev = ksz_switch_alloc(&client->dev, &ksz_i2c_ops, client);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	if (client->dev.platform_data)
> +		dev->pdata = client->dev.platform_data;
> +
> +	i2c_set_clientdata(client, dev);
> +
> +	ret = ksz9477_switch_register(dev);
> +	if (ret) {
> +		dev_err(&client->dev, "registering switch (ret: %d)\n", ret);


free dev on this error path?

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +

regards,
dan carpenter

Re: [PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[Sergio Paracuellos]

Hi Dan,

Thanks for the feedback.

On Mon, Dec 17, 2018 at 7:55 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Sun, Dec 16, 2018 at 08:57:40AM +0100, Sergio Paracuellos wrote:
> > +static int ksz_i2c_write(struct ksz_device *dev, u32 reg, u8 *val,
> > +                      unsigned int len)
> > +{
> > +     struct i2c_client *client = dev->priv;
> > +     unsigned int cnt = len;
> > +     int i = 0;
> > +     u8 txb[4];
> > +
> > +     do {
> > +             txb[i++] = (u8)(*val >> (8 * (cnt - 1)));
>                                               ^^^^^^^
>
> Can "cnt" be zero from ksz_i2c_set()?  If so this loop will corrupt
> memory.

This get and set interface seems to be introduced recently but it is
not being used yet, so
in this moment the answer is not. For me, there is no sense in call
'set' with no len. Should
I add a check for zero len and return EINVAL just in case?

>
> > +             cnt--;
> > +     } while (cnt);
> > +
> > +     return ksz_i2c_write_reg(client, reg, txb, len);
> > +}
> > +
> > +static int ksz_i2c_write8(struct ksz_device *dev, u32 reg, u8 value)
> > +{
> > +     return ksz_i2c_write(dev, reg, &value, 1);
> > +}
> > +
> > +static int ksz_i2c_write16(struct ksz_device *dev, u32 reg, u16 value)
> > +{
> > +     value = cpu_to_be16(value);
> > +     return ksz_i2c_write(dev, reg, (u8 *)&value, 2);
> > +}
> > +
> > +static int ksz_i2c_write24(struct ksz_device *dev, u32 reg, u32 value)
> > +{
> > +     /* make it to big endian 24bit from MSB */
> > +     value <<= 8;
> > +     value = cpu_to_be32(value);
> > +
> > +     return ksz_i2c_write(dev, reg, (u8 *)&value, 3);
> > +}
> > +
> > +static int ksz_i2c_write32(struct ksz_device *dev, u32 reg, u32 value)
> > +{
> > +     value = cpu_to_be32(value);
> > +     return ksz_i2c_write(dev, reg, (u8 *)&value, 4);
> > +}
> > +
> > +static int ksz_i2c_get(struct ksz_device *dev, u32 reg, void *data, size_t len)
> > +{
> > +     return ksz_i2c_read(dev, reg, data, len);
> > +}
> > +
> > +static int ksz_i2c_set(struct ksz_device *dev, u32 reg, void *data, size_t len)
> > +{
> > +     return ksz_i2c_write(dev, reg, data, len);
> > +}
> > +
> > +static const struct ksz_io_ops ksz_i2c_ops = {
> > +     .read8 = ksz_i2c_read8,
> > +     .read16 = ksz_i2c_read16,
> > +     .read24 = ksz_i2c_read24,
> > +     .read32 = ksz_i2c_read32,
> > +     .write8 = ksz_i2c_write8,
> > +     .write16 = ksz_i2c_write16,
> > +     .write24 = ksz_i2c_write24,
> > +     .write32 = ksz_i2c_write32,
> > +     .get = ksz_i2c_get,
> > +     .set = ksz_i2c_set,
> > +};
> > +
> > +static int ksz_i2c_probe(struct i2c_client *client,
> > +                      const struct i2c_device_id *id)
> > +{
> > +     struct ksz_device *dev;
> > +     int ret;
> > +
> > +     dev = ksz_switch_alloc(&client->dev, &ksz_i2c_ops, client);
> > +     if (!dev)
> > +             return -ENOMEM;
> > +
> > +     if (client->dev.platform_data)
> > +             dev->pdata = client->dev.platform_data;
> > +
> > +     i2c_set_clientdata(client, dev);
> > +
> > +     ret = ksz9477_switch_register(dev);
> > +     if (ret) {
> > +             dev_err(&client->dev, "registering switch (ret: %d)\n", ret);
>
>
> free dev on this error path?

Internally all of this are using devm_* funcions, so I think is there
is no need to free anything. Also, the
spi managed driver for this does nothing also about this.

>
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
>
> regards,
> dan carpenter

Best regards,
    Sergio Paracuellos

Re: [PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[Dan Carpenter]

On Mon, Dec 17, 2018 at 07:22:51PM +0100, Sergio Paracuellos wrote:
> > > +static int ksz_i2c_write(struct ksz_device *dev, u32 reg, u8 *val,
> > > +                      unsigned int len)
> > > +{
> > > +     struct i2c_client *client = dev->priv;
> > > +     unsigned int cnt = len;
> > > +     int i = 0;
> > > +     u8 txb[4];
> > > +
> > > +     do {
> > > +             txb[i++] = (u8)(*val >> (8 * (cnt - 1)));
> >                                               ^^^^^^^
> >
> > Can "cnt" be zero from ksz_i2c_set()?  If so this loop will corrupt
> > memory.
> 
> This get and set interface seems to be introduced recently but it is
> not being used yet, so
> in this moment the answer is not. For me, there is no sense in call
> 'set' with no len. Should
> I add a check for zero len and return EINVAL just in case?
> 

Yes, please.

> > > +static int ksz_i2c_probe(struct i2c_client *client,
> > > +                      const struct i2c_device_id *id)
> > > +{
> > > +     struct ksz_device *dev;
> > > +     int ret;
> > > +
> > > +     dev = ksz_switch_alloc(&client->dev, &ksz_i2c_ops, client);
> > > +     if (!dev)
> > > +             return -ENOMEM;
> > > +
> > > +     if (client->dev.platform_data)
> > > +             dev->pdata = client->dev.platform_data;
> > > +
> > > +     i2c_set_clientdata(client, dev);
> > > +
> > > +     ret = ksz9477_switch_register(dev);
> > > +     if (ret) {
> > > +             dev_err(&client->dev, "registering switch (ret: %d)\n", ret);
> >
> >
> > free dev on this error path?
> 
> Internally all of this are using devm_* funcions, so I think is there
> is no need to free anything. Also, the
> spi managed driver for this does nothing also about this.

You're right.  My bad.  I should have looked at this.

regards,
dan carpenter

Re: [PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[Sergio Paracuellos]

On Mon, Dec 17, 2018 at 8:11 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Dec 17, 2018 at 07:22:51PM +0100, Sergio Paracuellos wrote:
> > > > +static int ksz_i2c_write(struct ksz_device *dev, u32 reg, u8 *val,
> > > > +                      unsigned int len)
> > > > +{
> > > > +     struct i2c_client *client = dev->priv;
> > > > +     unsigned int cnt = len;
> > > > +     int i = 0;
> > > > +     u8 txb[4];
> > > > +
> > > > +     do {
> > > > +             txb[i++] = (u8)(*val >> (8 * (cnt - 1)));
> > >                                               ^^^^^^^
> > >
> > > Can "cnt" be zero from ksz_i2c_set()?  If so this loop will corrupt
> > > memory.
> >
> > This get and set interface seems to be introduced recently but it is
> > not being used yet, so
> > in this moment the answer is not. For me, there is no sense in call
> > 'set' with no len. Should
> > I add a check for zero len and return EINVAL just in case?
> >
>
> Yes, please.

Done. v3 with this change already sent.

>
> > > > +static int ksz_i2c_probe(struct i2c_client *client,
> > > > +                      const struct i2c_device_id *id)
> > > > +{
> > > > +     struct ksz_device *dev;
> > > > +     int ret;
> > > > +
> > > > +     dev = ksz_switch_alloc(&client->dev, &ksz_i2c_ops, client);
> > > > +     if (!dev)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     if (client->dev.platform_data)
> > > > +             dev->pdata = client->dev.platform_data;
> > > > +
> > > > +     i2c_set_clientdata(client, dev);
> > > > +
> > > > +     ret = ksz9477_switch_register(dev);
> > > > +     if (ret) {
> > > > +             dev_err(&client->dev, "registering switch (ret: %d)\n", ret);
> > >
> > >
> > > free dev on this error path?
> >
> > Internally all of this are using devm_* funcions, so I think is there
> > is no need to free anything. Also, the
> > spi managed driver for this does nothing also about this.
>
> You're right.  My bad.  I should have looked at this.
>
> regards,
> dan carpenter
>

Best regards,
    Sergio Paracuellos