[v1,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>
---
v1
- Return error code from i2c_transfer
- Change GPL license
- Change author

 drivers/net/dsa/microchip/Kconfig       |   6 +
 drivers/net/dsa/microchip/Makefile      |   1 +
 drivers/net/dsa/microchip/ksz9477_i2c.c | 196 ++++++++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_i2c.h     |  69 +++++++++++
 4 files changed, 272 insertions(+)
 create mode 100644 drivers/net/dsa/microchip/ksz9477_i2c.c
 create mode 100644 drivers/net/dsa/microchip/ksz_i2c.h

Hi Tristram,

> diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c
> b/drivers/net/dsa/microchip/ksz9477_i2c.c
> new file mode 100644
> index 0000000..29511e9
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
..
> +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);
> +}

Is there any specific reason that ksz_i2c_read24 & ksz_i2c_write24 in ksz9477_i2c.c
but, other functions are in ksz_i2c.h?

> +
> +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,
> +};

Thanks.
Woojung

On 12/19/2018 04:41 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>

Can this use regmap instead ?

Wed, Dec 19, 2018 at 04:41:37AM CET, 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>
>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);
>+}


This header file makes no sense. Please move the functions into .c



>+
>+#endif
>-- 
>1.9.1
>

Hi!

> >+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);
> >+}
> 
> 
> This header file makes no sense. Please move the functions into .c

No, that would make code bigger & slower.

It makes sense to me. But I'd add "inline" keyword to make the goal
explicit.

								Pavel

Wed, Dec 19, 2018 at 05:08:57PM CET, pavel@denx.de wrote:
>Hi!
>
>> >+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);
>> >+}
>> 
>> 
>> This header file makes no sense. Please move the functions into .c
>
>No, that would make code bigger & slower.
>
>It makes sense to me. But I'd add "inline" keyword to make the goal
>explicit.

1) It makes no sense to have header files for things like this. The
   functions are only used within the single .c file.

2) You cannot inline them, as they are used as ops.

>
>								Pavel
>-- 
>DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

From: Pavel Machek <pavel@denx.de>
Date: Wed, 19 Dec 2018 17:08:57 +0100

> Hi!
> 
>> >+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);
>> >+}
>> 
>> 
>> This header file makes no sense. Please move the functions into .c
> 
> No, that would make code bigger & slower.
> 
> It makes sense to me. But I'd add "inline" keyword to make the goal
> explicit.

Let the compiler decide in *.c files please.  That is our rule.

On Wed 2018-12-19 17:15:32, Jiri Pirko wrote:
> Wed, Dec 19, 2018 at 05:08:57PM CET, pavel@denx.de wrote:
> >Hi!
> >
> >> >+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);
> >> >+}
> >> 
> >> 
> >> This header file makes no sense. Please move the functions into .c
> >
> >No, that would make code bigger & slower.
> >
> >It makes sense to me. But I'd add "inline" keyword to make the goal
> >explicit.
> 
> 1) It makes no sense to have header files for things like this. The
>    functions are only used within the single .c file.
> 
> 2) You cannot inline them, as they are used as ops.

Ok, sorry for the noise.
								Pavel

On 12/19/2018 06:22 PM, Pavel Machek wrote:
> On Wed 2018-12-19 17:15:32, Jiri Pirko wrote:
>> Wed, Dec 19, 2018 at 05:08:57PM CET, pavel@denx.de wrote:
>>> Hi!
>>>
>>>>> +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);
>>>>> +}
>>>>
>>>>
>>>> This header file makes no sense. Please move the functions into .c
>>>
>>> No, that would make code bigger & slower.
>>>
>>> It makes sense to me. But I'd add "inline" keyword to make the goal
>>> explicit.
>>
>> 1) It makes no sense to have header files for things like this. The
>>    functions are only used within the single .c file.
>>
>> 2) You cannot inline them, as they are used as ops.
> 
> Ok, sorry for the noise.

If you were to use regmap, this whole boilerplate would go away ...

> Can this use regmap instead ?
> 

To tell the truth I do not know how.

Some customers, like Sergio, seem to be using KSZ9897 with I2C in their projects
and so cannot wait further.

> >>>> This header file makes no sense. Please move the functions into .c
> >>>
> >>> No, that would make code bigger & slower.
> >>>
> >>> It makes sense to me. But I'd add "inline" keyword to make the goal
> >>> explicit.
> >>
> >> 1) It makes no sense to have header files for things like this. The
> >>    functions are only used within the single .c file.
> >>
> >> 2) You cannot inline them, as they are used as ops.
> >
> > Ok, sorry for the noise.
> 
> If you were to use regmap, this whole boilerplate would go away ...

Sometimes I am confused about this review process.

The new code is same as previous code submitted.  The old code was accepted,
but then there are objections to the new code.

So if I change the new code, should I go back to correct the old code?

The ksz9477_i2c.c should be the same as ksz9477_i2c.c while ksz_i2c.h matches
ksz_spi.h.  The idea is ksz_i2c.h can be used by another switch driver like ksz####_i2c.c.

> Is there any specific reason that ksz_i2c_read24 & ksz_i2c_write24 in
> ksz9477_i2c.c
> but, other functions are in ksz_i2c.h?
> 

There are specific registers in KSZ9477 such that using 24-bit is more efficient than
reading 32-bit first and writing 32-bit.  The other older switches mostly use 8-bit, while
one uses 16-bit.

KSZ9477 also has IBA mode that can use 24-bit.

On 12/19/2018 11:30 PM, Tristram.Ha@microchip.com wrote:
>> Can this use regmap instead ?
>>
> 
> To tell the truth I do not know how.
> 
> Some customers, like Sergio, seem to be using KSZ9897 with I2C in their projects
> and so cannot wait further.

Well, I just did 'git grep regmap_i2c drivers' and 'git grep regmap_spi
drivers/' and found eg.
drivers/iio/pressure/zpa2326_i2c.c
drivers/iio/pressure/zpa2326_spi.c

There's plenty of drivers using regmap in drivers/ to demonstrate how to
use it. And there's always include/linux/regmap.h .

In this specific (DSA) case, register either regmap_spi or regmap_i2c in
the ksz_switch_register(), pop the regmap pointer into ksz_device and
then use it in ksz_read*()/ksz_write*(). You can then, in a subsequent
patch, massage the regmap accesses directly into the code instead and
drop the ksz_read*()/ksz_write*() functions altogether, since those
would be just regmap function wrappers.

And if you're still lost, I can start on the conversion myself.

I'm sure Sergio can wait.

On 12/19/2018 11:50 PM, Tristram.Ha@microchip.com wrote:
>>>>>> This header file makes no sense. Please move the functions into .c
>>>>>
>>>>> No, that would make code bigger & slower.
>>>>>
>>>>> It makes sense to me. But I'd add "inline" keyword to make the goal
>>>>> explicit.
>>>>
>>>> 1) It makes no sense to have header files for things like this. The
>>>>    functions are only used within the single .c file.
>>>>
>>>> 2) You cannot inline them, as they are used as ops.
>>>
>>> Ok, sorry for the noise.
>>
>> If you were to use regmap, this whole boilerplate would go away ...
> 
> Sometimes I am confused about this review process.
> 
> The new code is same as previous code submitted.  The old code was accepted,
> but then there are objections to the new code.

Yes, continuous quality improvement is important.

> So if I change the new code, should I go back to correct the old code?

Maintaining old code and testing old code is indeed most welcome.

> The ksz9477_i2c.c should be the same as ksz9477_i2c.c while ksz_i2c.h matches
> ksz_spi.h.  The idea is ksz_i2c.h can be used by another switch driver like ksz####_i2c.c.

The regmap hides all the complexity and avoids re-implementations of
these kinds of accessors over and over again.

> > Is there any specific reason that ksz_i2c_read24 & ksz_i2c_write24 in
> > ksz9477_i2c.c
> > but, other functions are in ksz_i2c.h?
> >
> 
> There are specific registers in KSZ9477 such that using 24-bit is more efficient than
> reading 32-bit first and writing 32-bit.  The other older switches mostly use 8-bit, while
> one uses 16-bit.
> 
Meant location of functions than usage of function.

Because comments about using single C file, this can be ignored if do so.

Thanks.
Woojung

On 12/20/2018 12:11 AM, Marek Vasut wrote:
> On 12/19/2018 11:30 PM, Tristram.Ha@microchip.com wrote:
>>> Can this use regmap instead ?
>>>
>>
>> To tell the truth I do not know how.
>>
>> Some customers, like Sergio, seem to be using KSZ9897 with I2C in their projects
>> and so cannot wait further.
> 
> Well, I just did 'git grep regmap_i2c drivers' and 'git grep regmap_spi
> drivers/' and found eg.
> drivers/iio/pressure/zpa2326_i2c.c
> drivers/iio/pressure/zpa2326_spi.c
> 
> There's plenty of drivers using regmap in drivers/ to demonstrate how to
> use it. And there's always include/linux/regmap.h .
> 
> In this specific (DSA) case, register either regmap_spi or regmap_i2c in
> the ksz_switch_register(), pop the regmap pointer into ksz_device and
> then use it in ksz_read*()/ksz_write*(). You can then, in a subsequent
> patch, massage the regmap accesses directly into the code instead and
> drop the ksz_read*()/ksz_write*() functions altogether, since those
> would be just regmap function wrappers.
> 
> And if you're still lost, I can start on the conversion myself.

Actually, wait a few hours, I'll try to prepare a regmap patchset.

On 12/20/2018 12:31 AM, Marek Vasut wrote:
> On 12/20/2018 12:11 AM, Marek Vasut wrote:
>> On 12/19/2018 11:30 PM, Tristram.Ha@microchip.com wrote:
>>>> Can this use regmap instead ?
>>>>
>>>
>>> To tell the truth I do not know how.
>>>
>>> Some customers, like Sergio, seem to be using KSZ9897 with I2C in their projects
>>> and so cannot wait further.
>>
>> Well, I just did 'git grep regmap_i2c drivers' and 'git grep regmap_spi
>> drivers/' and found eg.
>> drivers/iio/pressure/zpa2326_i2c.c
>> drivers/iio/pressure/zpa2326_spi.c
>>
>> There's plenty of drivers using regmap in drivers/ to demonstrate how to
>> use it. And there's always include/linux/regmap.h .
>>
>> In this specific (DSA) case, register either regmap_spi or regmap_i2c in
>> the ksz_switch_register(), pop the regmap pointer into ksz_device and
>> then use it in ksz_read*()/ksz_write*(). You can then, in a subsequent
>> patch, massage the regmap accesses directly into the code instead and
>> drop the ksz_read*()/ksz_write*() functions altogether, since those
>> would be just regmap function wrappers.
>>
>> And if you're still lost, I can start on the conversion myself.
> 
> Actually, wait a few hours, I'll try to prepare a regmap patchset.

Try this [1], it should be a start. The last two patches might need some
adjustments, so please test and let me know how that works (or better
yet, provide fixes). Adding i2c regmap should then be trivial :)

[1] https://patchwork.ozlabs.org/cover/1016432/

> >> Well, I just did 'git grep regmap_i2c drivers' and 'git grep regmap_spi
> >> drivers/' and found eg.
> >> drivers/iio/pressure/zpa2326_i2c.c
> >> drivers/iio/pressure/zpa2326_spi.c
> >>
> >> There's plenty of drivers using regmap in drivers/ to demonstrate how to
> >> use it. And there's always include/linux/regmap.h .
> >>
> >> In this specific (DSA) case, register either regmap_spi or regmap_i2c in
> >> the ksz_switch_register(), pop the regmap pointer into ksz_device and
> >> then use it in ksz_read*()/ksz_write*(). You can then, in a subsequent
> >> patch, massage the regmap accesses directly into the code instead and
> >> drop the ksz_read*()/ksz_write*() functions altogether, since those
> >> would be just regmap function wrappers.
> >>
> >> And if you're still lost, I can start on the conversion myself.
> >
> > Actually, wait a few hours, I'll try to prepare a regmap patchset.
> 
> Try this [1], it should be a start. The last two patches might need some
> adjustments, so please test and let me know how that works (or better
> yet, provide fixes). Adding i2c regmap should then be trivial :)
> 
> [1] https://patchwork.ozlabs.org/cover/1016432/

I did look at the code in regmap_i2c.c and regmap_spi.c.

So the assumption is there is a regmap_read function that automatically
maps to regmap_i2c_read or regmap_spi_read depending on the device.
But the SPI access does not use the register directly.  It has to convert it
into a special format recognized by the chip.  The size used by SPI is 4, while
I2C access uses the standard way and the size is 2.

On 12/21/2018 01:48 AM, Tristram.Ha@microchip.com wrote:
>>>> Well, I just did 'git grep regmap_i2c drivers' and 'git grep regmap_spi
>>>> drivers/' and found eg.
>>>> drivers/iio/pressure/zpa2326_i2c.c
>>>> drivers/iio/pressure/zpa2326_spi.c
>>>>
>>>> There's plenty of drivers using regmap in drivers/ to demonstrate how to
>>>> use it. And there's always include/linux/regmap.h .
>>>>
>>>> In this specific (DSA) case, register either regmap_spi or regmap_i2c in
>>>> the ksz_switch_register(), pop the regmap pointer into ksz_device and
>>>> then use it in ksz_read*()/ksz_write*(). You can then, in a subsequent
>>>> patch, massage the regmap accesses directly into the code instead and
>>>> drop the ksz_read*()/ksz_write*() functions altogether, since those
>>>> would be just regmap function wrappers.
>>>>
>>>> And if you're still lost, I can start on the conversion myself.
>>>
>>> Actually, wait a few hours, I'll try to prepare a regmap patchset.
>>
>> Try this [1], it should be a start. The last two patches might need some
>> adjustments, so please test and let me know how that works (or better
>> yet, provide fixes). Adding i2c regmap should then be trivial :)
>>
>> [1] https://patchwork.ozlabs.org/cover/1016432/
> 
> I did look at the code in regmap_i2c.c and regmap_spi.c.
> 
> So the assumption is there is a regmap_read function that automatically
> maps to regmap_i2c_read or regmap_spi_read depending on the device.

Yes, I just sent a V2 patchset tested on KSZ8795 . It removes over 200
LoC from the KSZ driver. Please test it on KSZ9477 and let me know if it
broke anything -- and ideally, send patch :)

> But the SPI access does not use the register directly.  It has to convert it
> into a special format recognized by the chip.  The size used by SPI is 4, while
> I2C access uses the standard way and the size is 2.

Regmap can handle that too, see the V2 :-)

On Wed, Dec 19, 2018 at 10:50:16PM +0000, Tristram.Ha@microchip.com wrote:
>
> So if I change the new code, should I go back to correct the old code?

Other operating systems would worry about breaking code which is working
fine, but re-writing ancient code is The Linux Way.

regards,
dan carpenter

On 12/21/2018 10:29 AM, Dan Carpenter wrote:
> On Wed, Dec 19, 2018 at 10:50:16PM +0000, Tristram.Ha@microchip.com wrote:
>>
>> So if I change the new code, should I go back to correct the old code?
> 
> Other operating systems would worry about breaking code which is working
> fine, but re-writing ancient code is The Linux Way.

So ... when will we finally rewrite Linux in rust ? :-)

On Fri, Dec 21, 2018 at 01:14:51PM +0100, Marek Vasut wrote:
> On 12/21/2018 10:29 AM, Dan Carpenter wrote:
> > On Wed, Dec 19, 2018 at 10:50:16PM +0000, Tristram.Ha@microchip.com wrote:
> >>
> >> So if I change the new code, should I go back to correct the old code?
> > 
> > Other operating systems would worry about breaking code which is working
> > fine, but re-writing ancient code is The Linux Way.
> 
> So ... when will we finally rewrite Linux in rust ? :-)

Probably we'll move more modules to userspace in time.

Wasn't there a joke proof of concept a while back that let people write
kernel modules in perl?

regards,
dan carpenter

On 12/21/2018 01:23 PM, Dan Carpenter wrote:
> On Fri, Dec 21, 2018 at 01:14:51PM +0100, Marek Vasut wrote:
>> On 12/21/2018 10:29 AM, Dan Carpenter wrote:
>>> On Wed, Dec 19, 2018 at 10:50:16PM +0000, Tristram.Ha@microchip.com wrote:
>>>>
>>>> So if I change the new code, should I go back to correct the old code?
>>>
>>> Other operating systems would worry about breaking code which is working
>>> fine, but re-writing ancient code is The Linux Way.
>>
>> So ... when will we finally rewrite Linux in rust ? :-)
> 
> Probably we'll move more modules to userspace in time.
> 
> Wasn't there a joke proof of concept a while back that let people write
> kernel modules in perl?

I recall seeing a couple different, more or less obscure, languages :)

On Wed 2018-12-19 22:50:16, Tristram.Ha@microchip.com wrote:
> > >>>> This header file makes no sense. Please move the functions into .c
> > >>>
> > >>> No, that would make code bigger & slower.
> > >>>
> > >>> It makes sense to me. But I'd add "inline" keyword to make the goal
> > >>> explicit.
> > >>
> > >> 1) It makes no sense to have header files for things like this. The
> > >>    functions are only used within the single .c file.
> > >>
> > >> 2) You cannot inline them, as they are used as ops.
> > >
> > > Ok, sorry for the noise.
> > 
> > If you were to use regmap, this whole boilerplate would go away ...
> 
> Sometimes I am confused about this review process.
> 
> The new code is same as previous code submitted.  The old code was accepted,
> but then there are objections to the new code.

Sometimes you just need to reject review comment...

It took extremely long to merge this driver; that does not help,
either.

Good luck,
								Pavel

On 1/9/19 5:05 PM, Pavel Machek wrote:
> On Wed 2018-12-19 22:50:16, Tristram.Ha@microchip.com wrote:
>>>>>>> This header file makes no sense. Please move the functions into .c
>>>>>>
>>>>>> No, that would make code bigger & slower.
>>>>>>
>>>>>> It makes sense to me. But I'd add "inline" keyword to make the goal
>>>>>> explicit.
>>>>>
>>>>> 1) It makes no sense to have header files for things like this. The
>>>>>    functions are only used within the single .c file.
>>>>>
>>>>> 2) You cannot inline them, as they are used as ops.
>>>>
>>>> Ok, sorry for the noise.
>>>
>>> If you were to use regmap, this whole boilerplate would go away ...
>>
>> Sometimes I am confused about this review process.
>>
>> The new code is same as previous code submitted.  The old code was accepted,
>> but then there are objections to the new code.
> 
> Sometimes you just need to reject review comment...
> 
> It took extremely long to merge this driver; that does not help,
> either.

FYI I submitted the regmap conversion series a few weeks ago, however I
couldn't test it on this specific switch. It'd be real nice to get
feedback on it and then build the subsequent patches on top of it, since
regmap hides the accessor differences and even makes the code cleaner.

On Wed, Dec 19, 2018 at 4:41 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>
> ---
> v1
> - Return error code from i2c_transfer
> - Change GPL license
> - Change author
>
>  drivers/net/dsa/microchip/Kconfig       |   6 +
>  drivers/net/dsa/microchip/Makefile      |   1 +
>  drivers/net/dsa/microchip/ksz9477_i2c.c | 196 ++++++++++++++++++++++++++++++++
>  drivers/net/dsa/microchip/ksz_i2c.h     |  69 +++++++++++
>  4 files changed, 272 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..29511e9
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
> @@ -0,0 +1,196 @@
> +// 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;
> +
> +       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];
> +
> +       ret = i2c_transfer(i2c->adapter, msg, 2);
> +       if (ret == 2)
> +               ret = 0;
> +       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;
> +
> +       val[0] = (u8)(reg >> 8);
> +       val[1] = (u8)reg;
> +
> +       msg.addr = i2c->addr;
> +       msg.flags = 0;
> +       msg.len = 2 + len;
> +       msg.buf = val;
> +
> +       ret = i2c_transfer(i2c->adapter, &msg, 1);
> +       if (ret == 1)
> +               ret = 0;
> +       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("Tristram Ha <Tristram.Ha@microchip.com>");
> +MODULE_DESCRIPTION("Microchip KSZ9477 Series Switch I2C access Driver");
> +MODULE_LICENSE("GPL v2");
> 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
>

I don't know if at the end this is going to be changed together with
spi etc to use regmap API but I tested this code as it is and it
works. So you can add my Tested-by if you want:

Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Best regards,
     Sergio Paracuellos