[RFC,net-next] net: dsa: microchip: add KSZ9477 I2C driver

From: Tristram Ha <Tristram.Ha@microchip.com>

Add KSZ9477 I2C driver support.  The code ksz9477.c and ksz_common.c are
used together to generate the I2C driver.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/Kconfig       |   6 +
 drivers/net/dsa/microchip/Makefile      |   1 +
 drivers/net/dsa/microchip/ksz9477_i2c.c | 194 ++++++++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_i2c.h     |  69 ++++++++++++
 4 files changed, 270 insertions(+)
 create mode 100644 drivers/net/dsa/microchip/ksz9477_i2c.c
 create mode 100644 drivers/net/dsa/microchip/ksz_i2c.h

Tristram,

> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
..
> +MODULE_AUTHOR("Woojung Huh <Woojung.Huh@microchip.com>");
Believe this should be your name.

Thanks.
Woojung

Sorry about the patch.  I know you were using the code from the new SPI and old I2C drivers to come up with your patch.  You can incorporate the changes and test the driver and re-submit the patch if you want.

Your i2c_probe function displays an error message when ksz9477_switch_register is not successful.  It is likely the error code is from the dsa_register_switch function when the core DSA driver is not loaded yet.

All the register access functions in ksz_io_ops structure will be called by the driver code.  The length should always be non-zero.  The set and get functions can be invoked by the standard kernel register access API, which is called in user space.  The functions that handle this API make sure the length is non-zero before continuing.  For switches with simple register set the get function can dump all registers in one call.

I am not sure this register access API is allowed anymore as this may create a security hole in the kernel, but it helps greatly during development and testing as the driver is quite opaque to display the hardware state when something is wrong.

A little out-of-topic is the modalias now returns a different string rather than "i2c:ksz9477."  This is done with the "cat /sys/bus/i2c/devices/0-005f/modalias" command.  The string looks legit but it is difficult for regular users to get anything from it.

Hi Tristram,

Two minor comments.

On Tue, Dec 18, 2018 at 2:40 AM <Tristram.Ha@microchip.com> wrote:
>
> From: Tristram Ha <Tristram.Ha@microchip.com>
>
> Add KSZ9477 I2C driver support.  The code ksz9477.c and ksz_common.c are
> used together to generate the I2C driver.
>
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---
>  drivers/net/dsa/microchip/Kconfig       |   6 +
>  drivers/net/dsa/microchip/Makefile      |   1 +
>  drivers/net/dsa/microchip/ksz9477_i2c.c | 194 ++++++++++++++++++++++++++++++++
>  drivers/net/dsa/microchip/ksz_i2c.h     |  69 ++++++++++++
>  4 files changed, 270 insertions(+)
>  create mode 100644 drivers/net/dsa/microchip/ksz9477_i2c.c
>  create mode 100644 drivers/net/dsa/microchip/ksz_i2c.h
>
> diff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig
> index bea29fd..fd94441 100644
> --- a/drivers/net/dsa/microchip/Kconfig
> +++ b/drivers/net/dsa/microchip/Kconfig
> @@ -9,6 +9,12 @@ menuconfig NET_DSA_MICROCHIP_KSZ9477
>         help
>           This driver adds support for Microchip KSZ9477 switch chips.
>
> +config NET_DSA_MICROCHIP_KSZ9477_I2C
> +       tristate "KSZ9477 series I2C connected switch driver"
> +       depends on NET_DSA_MICROCHIP_KSZ9477 && I2C
> +       help
> +         Select to enable support for registering switches configured through I2C.
> +
>  config NET_DSA_MICROCHIP_KSZ9477_SPI
>         tristate "KSZ9477 series SPI connected switch driver"
>         depends on NET_DSA_MICROCHIP_KSZ9477 && SPI
> diff --git a/drivers/net/dsa/microchip/Makefile b/drivers/net/dsa/microchip/Makefile
> index 3142c18..dbcc5db 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_I2C)    += ksz9477_i2c.o
>  obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_SPI)    += ksz9477_spi.o
> diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
> new file mode 100644
> index 0000000..63d2f57
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Microchip KSZ9477 series register access through I2C
> + *
> + * Copyright (C) 2018 Microchip Technology Inc.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +
> +#include "ksz_priv.h"
> +#include "ksz_i2c.h"
> +
> +/* Enough to read all switch port registers. */
> +#define I2C_TX_BUF_LEN                 0x100
> +
> +static int ksz9477_i2c_read_reg(struct i2c_client *i2c, u32 reg, u8 *val,
> +                               unsigned int len)
> +{
> +       struct i2c_msg msg[2];
> +       int ret = 0;
> +
> +       val[0] = (u8)(reg >> 8);
> +       val[1] = (u8)reg;
> +
> +       msg[0].addr = i2c->addr;
> +       msg[0].flags = 0;
> +       msg[0].len = 2;
> +       msg[0].buf = val;
> +
> +       msg[1].addr = i2c->addr;
> +       msg[1].flags = I2C_M_RD;
> +       msg[1].len = len;
> +       msg[1].buf = &val[2];
> +
> +       if (i2c_transfer(i2c->adapter, msg, 2) != 2)
> +               ret = -ENODEV;

Should this be -EREMOTEIO instead?

> +       return ret;
> +}
> +
> +static int ksz9477_i2c_write_reg(struct i2c_client *i2c, u32 reg, u8 *val,
> +                                unsigned int len)
> +{
> +       struct i2c_msg msg;
> +       int ret = 0;
> +
> +       val[0] = (u8)(reg >> 8);
> +       val[1] = (u8)reg;
> +
> +       msg.addr = i2c->addr;
> +       msg.flags = 0;
> +       msg.len = 2 + len;
> +       msg.buf = val;
> +
> +       if (i2c_transfer(i2c->adapter, &msg, 1) != 1)
> +               ret = -ENODEV;


Should this be -EREMOTEIO instead?

> +       return ret;
> +}
> +
> +static int ksz_i2c_read(struct ksz_device *dev, u32 reg, u8 *data,
> +                       unsigned int len)
> +{
> +       struct i2c_client *i2c = dev->priv;
> +       int ret;
> +
> +       ret = ksz9477_i2c_read_reg(i2c, reg, dev->txbuf, len);
> +       if (!ret)
> +               memcpy(data, &dev->txbuf[2], len);
> +       return ret;
> +}
> +
> +static int ksz_i2c_write(struct ksz_device *dev, u32 reg, void *data,
> +                        unsigned int len)
> +{
> +       struct i2c_client *i2c = dev->priv;
> +
> +       if (len > I2C_TX_BUF_LEN)
> +               len = I2C_TX_BUF_LEN;
> +       memcpy(&dev->txbuf[2], data, len);
> +       return ksz9477_i2c_write_reg(i2c, reg, dev->txbuf, len);
> +}
> +
> +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 24bit */
> +               *val >>= 8;
> +       }
> +
> +       return ret;
> +}
> +
> +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, &value, 3);
> +}
> +
> +static const struct ksz_io_ops ksz9477_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 ksz9477_i2c_probe(struct i2c_client *i2c,
> +                            const struct i2c_device_id *i2c_id)
> +{
> +       struct ksz_device *dev;
> +       int ret;
> +
> +       dev = ksz_switch_alloc(&i2c->dev, &ksz9477_i2c_ops, i2c);
> +       if (!dev)
> +               return -ENOMEM;
> +
> +       if (i2c->dev.platform_data)
> +               dev->pdata = i2c->dev.platform_data;
> +
> +       dev->txbuf = devm_kzalloc(dev->dev, 2 + I2C_TX_BUF_LEN, GFP_KERNEL);
> +
> +       ret = ksz9477_switch_register(dev);
> +
> +       /* Main DSA driver may not be started yet. */
> +       if (ret)
> +               return ret;
> +
> +       i2c_set_clientdata(i2c, dev);
> +
> +       return 0;
> +}
> +
> +static int ksz9477_i2c_remove(struct i2c_client *i2c)
> +{
> +       struct ksz_device *dev = i2c_get_clientdata(i2c);
> +
> +       if (dev)
> +               ksz_switch_remove(dev);
> +
> +       return 0;
> +}
> +
> +static void ksz9477_i2c_shutdown(struct i2c_client *i2c)
> +{
> +       struct ksz_device *dev = i2c_get_clientdata(i2c);
> +
> +       if (dev && dev->dev_ops->shutdown)
> +               dev->dev_ops->shutdown(dev);
> +}
> +
> +static const struct i2c_device_id ksz9477_i2c_id[] = {
> +       { "ksz9477-switch", 0 },
> +       {},
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, ksz9477_i2c_id);
> +
> +static const struct of_device_id ksz9477_dt_ids[] = {
> +       { .compatible = "microchip,ksz9477" },
> +       { .compatible = "microchip,ksz9897" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, ksz9477_dt_ids);
> +
> +static struct i2c_driver ksz9477_i2c_driver = {
> +       .driver = {
> +               .name   = "ksz9477-switch",
> +               .owner  = THIS_MODULE,
> +               .of_match_table = of_match_ptr(ksz9477_dt_ids),
> +       },
> +       .probe  = ksz9477_i2c_probe,
> +       .remove = ksz9477_i2c_remove,
> +       .shutdown = ksz9477_i2c_shutdown,
> +       .id_table = ksz9477_i2c_id,
> +};
> +
> +module_i2c_driver(ksz9477_i2c_driver);
> +
> +MODULE_AUTHOR("Woojung Huh <Woojung.Huh@microchip.com>");
> +MODULE_DESCRIPTION("Microchip KSZ9477 Series Switch I2C access Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/dsa/microchip/ksz_i2c.h b/drivers/net/dsa/microchip/ksz_i2c.h
> new file mode 100644
> index 0000000..b9af0a8
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz_i2c.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + * Microchip KSZ series I2C access common header
> + *
> + * Copyright (C) 2018 Microchip Technology Inc.
> + *     Tristram Ha <Tristram.Ha@microchip.com>
> + */
> +
> +#ifndef __KSZ_I2C_H
> +#define __KSZ_I2C_H
> +
> +/* Chip dependent I2C access */
> +static int ksz_i2c_read(struct ksz_device *dev, u32 reg, u8 *data,
> +                       unsigned int len);
> +static int ksz_i2c_write(struct ksz_device *dev, u32 reg, void *data,
> +                        unsigned int 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_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;
> +}
> +
> +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, &value, 2);
> +}
> +
> +static int ksz_i2c_write32(struct ksz_device *dev, u32 reg, u32 value)
> +{
> +       value = cpu_to_be32(value);
> +       return ksz_i2c_write(dev, reg, &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);
> +}
> +
> +#endif
> --
> 1.9.1
>

Best regards,
    Sergio Paracuellos

Hi Tristam,

On Tue, Dec 18, 2018 at 5:16 AM <Tristram.Ha@microchip.com> wrote:
>
> Sorry about the patch.  I know you were using the code from the new SPI and old I2C drivers to come up with your patch.  You can incorporate the changes and test the driver and re-submit the patch if you want.

For me is ok to just use the driver you have just submitted, there is
no need to me to resend anything, I think. I only wanted an i2c driver
for this switch and because there wasn't one I just sent my patches
:-).

>
> Your i2c_probe function displays an error message when ksz9477_switch_register is not successful.  It is likely the error code is from the dsa_register_switch function when the core DSA driver is not loaded yet.
>
> All the register access functions in ksz_io_ops structure will be called by the driver code.  The length should always be non-zero.  The set and get functions can be invoked by the standard kernel register access API, which is called in user space.  The functions that handle this API make sure the length is non-zero before continuing.  For switches with simple register set the get function can dump all registers in one call.
>
> I am not sure this register access API is allowed anymore as this may create a security hole in the kernel, but it helps greatly during development and testing as the driver is quite opaque to display the hardware state when something is wrong.
>
> A little out-of-topic is the modalias now returns a different string rather than "i2c:ksz9477."  This is done with the "cat /sys/bus/i2c/devices/0-005f/modalias" command.  The string looks legit but it is difficult for regular users to get anything from it.
>

Thanks for clarification.

Best regards,
    Sergio Paracuellos

On Tue, Dec 18, 2018 at 07:24:52AM +0100, Sergio Paracuellos wrote:
> > +static int ksz9477_i2c_read_reg(struct i2c_client *i2c, u32 reg, u8 *val,
> > +                               unsigned int len)
> > +{
> > +       struct i2c_msg msg[2];
> > +       int ret = 0;
> > +
> > +       val[0] = (u8)(reg >> 8);
> > +       val[1] = (u8)reg;
> > +
> > +       msg[0].addr = i2c->addr;
> > +       msg[0].flags = 0;
> > +       msg[0].len = 2;
> > +       msg[0].buf = val;
> > +
> > +       msg[1].addr = i2c->addr;
> > +       msg[1].flags = I2C_M_RD;
> > +       msg[1].len = len;
> > +       msg[1].buf = &val[2];
> > +
> > +       if (i2c_transfer(i2c->adapter, msg, 2) != 2)
> > +               ret = -ENODEV;
> 
> Should this be -EREMOTEIO instead?

It should actually be whatever error code i2c_transfer returns.

> > +static int ksz9477_i2c_write_reg(struct i2c_client *i2c, u32 reg, u8 *val,
> > +                                unsigned int len)
> > +{
> > +       struct i2c_msg msg;
> > +       int ret = 0;
> > +
> > +       val[0] = (u8)(reg >> 8);
> > +       val[1] = (u8)reg;
> > +
> > +       msg.addr = i2c->addr;
> > +       msg.flags = 0;
> > +       msg.len = 2 + len;
> > +       msg.buf = val;
> > +
> > +       if (i2c_transfer(i2c->adapter, &msg, 1) != 1)
> > +               ret = -ENODEV;
> 
> 
> Should this be -EREMOTEIO instead?

And the same here.

> > +MODULE_AUTHOR("Woojung Huh <Woojung.Huh@microchip.com>");
> > +MODULE_DESCRIPTION("Microchip KSZ9477 Series Switch I2C access Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/net/dsa/microchip/ksz_i2c.h b/drivers/net/dsa/microchip/ksz_i2c.h
> > new file mode 100644
> > index 0000000..b9af0a8
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/ksz_i2c.h
> > @@ -0,0 +1,69 @@
> > +/* SPDX-License-Identifier: GPL-2.0

This is i think inconsistent. MODULE_LICENSE("GPL") means GPL 2 and at
your choice, future versions. Your SPDX is for v2 only.

     Andrew

On 12/18/2018 11:01 AM, Andrew Lunn wrote:
> On Tue, Dec 18, 2018 at 07:24:52AM +0100, Sergio Paracuellos wrote:
>>> +static int ksz9477_i2c_read_reg(struct i2c_client *i2c, u32 reg, u8 *val,
>>> +                               unsigned int len)
>>> +{
>>> +       struct i2c_msg msg[2];
>>> +       int ret = 0;
>>> +
>>> +       val[0] = (u8)(reg >> 8);
>>> +       val[1] = (u8)reg;
>>> +
>>> +       msg[0].addr = i2c->addr;
>>> +       msg[0].flags = 0;
>>> +       msg[0].len = 2;
>>> +       msg[0].buf = val;
>>> +
>>> +       msg[1].addr = i2c->addr;
>>> +       msg[1].flags = I2C_M_RD;
>>> +       msg[1].len = len;
>>> +       msg[1].buf = &val[2];
>>> +
>>> +       if (i2c_transfer(i2c->adapter, msg, 2) != 2)
>>> +               ret = -ENODEV;
>>
>> Should this be -EREMOTEIO instead?
> 
> It should actually be whatever error code i2c_transfer returns.

Before we plunge into coding yet another I2C-and-SPI layer, can we use
regmap instead ?