[PATCH v2 0/4] can: mcp251x: Make use of device properties

[PATCH v2 0/4] can: mcp251x: Make use of device properties

[Andy Shevchenko]

The purpose of this series is to simplify driver by switching to use device
properties. In particular it allows to drop legacy platform data.

Patch 1 switches driver to use devm_clk_get_optional() API.

Patch 2 unifies getting the driver data independently of the table which
provides it.

Patch 3 drops extra check for regulator presence by switch to use an already
present wrapper.

And patch 4 gets rid of legacy platform data.

Changelog v2:
- add patch 4 to get rid of legacy platform data

Andy Shevchenko (4):
  can: mcp251x: Use devm_clk_get_optional() to get the input clock
  can: mcp251x: Make use of device property API
  can: mcp251x: Call wrapper instead of regulator_disable()
  can: mcp251x: Get rid of legacy platform data

 arch/arm/mach-pxa/icontrol.c         |  9 ++--
 arch/arm/mach-pxa/zeus.c             |  9 ++--
 drivers/net/can/spi/mcp251x.c        | 65 +++++++++++-----------------
 include/linux/can/platform/mcp251x.h | 22 ----------
 4 files changed, 36 insertions(+), 69 deletions(-)
 delete mode 100644 include/linux/can/platform/mcp251x.h

-- 
2.23.0.rc1

[PATCH v2 1/4] can: mcp251x: Use devm_clk_get_optional() to get the input clock

[Andy Shevchenko]

Simplify the code which fetches the input clock by using
devm_clk_get_optional(). This comes with a small functional change: previously
all errors were ignored when platform data is present. Now all errors are
treated as errors. If no input clock is present devm_clk_get_optional() will
return NULL instead of an error which matches the behavior of the old code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/net/can/spi/mcp251x.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index 58992fd61cb9..e04b578f2b1f 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -1014,15 +1014,13 @@ static int mcp251x_can_probe(struct spi_device *spi)
 	struct clk *clk;
 	int freq, ret;
 
-	clk = devm_clk_get(&spi->dev, NULL);
-	if (IS_ERR(clk)) {
-		if (pdata)
-			freq = pdata->oscillator_frequency;
-		else
-			return PTR_ERR(clk);
-	} else {
-		freq = clk_get_rate(clk);
-	}
+	clk = devm_clk_get_optional(&spi->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	freq = clk_get_rate(clk);
+	if (freq == 0 && pdata)
+		freq = pdata->oscillator_frequency;
 
 	/* Sanity check */
 	if (freq < 1000000 || freq > 25000000)
@@ -1033,11 +1031,9 @@ static int mcp251x_can_probe(struct spi_device *spi)
 	if (!net)
 		return -ENOMEM;
 
-	if (!IS_ERR(clk)) {
-		ret = clk_prepare_enable(clk);
-		if (ret)
-			goto out_free;
-	}
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		goto out_free;
 
 	net->netdev_ops = &mcp251x_netdev_ops;
 	net->flags |= IFF_ECHO;
@@ -1122,8 +1118,7 @@ static int mcp251x_can_probe(struct spi_device *spi)
 	mcp251x_power_enable(priv->power, 0);
 
 out_clk:
-	if (!IS_ERR(clk))
-		clk_disable_unprepare(clk);
+	clk_disable_unprepare(clk);
 
 out_free:
 	free_candev(net);
@@ -1141,8 +1136,7 @@ static int mcp251x_can_remove(struct spi_device *spi)
 
 	mcp251x_power_enable(priv->power, 0);
 
-	if (!IS_ERR(priv->clk))
-		clk_disable_unprepare(priv->clk);
+	clk_disable_unprepare(priv->clk);
 
 	free_candev(net);
 
-- 
2.23.0.rc1

[PATCH v2 2/4] can: mcp251x: Make use of device property API

[Andy Shevchenko]

Make use of device property API in this driver so that both OF based
system and ACPI based system can use this driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/net/can/spi/mcp251x.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index e04b578f2b1f..0b7e743ca0a0 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -53,8 +53,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
-#include <linux/of.h>
-#include <linux/of_device.h>
+#include <linux/property.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
@@ -914,7 +913,7 @@ static int mcp251x_open(struct net_device *net)
 	priv->tx_skb = NULL;
 	priv->tx_len = 0;
 
-	if (!spi->dev.of_node)
+	if (!dev_fwnode(&spi->dev))
 		flags = IRQF_TRIGGER_FALLING;
 
 	ret = request_threaded_irq(spi->irq, NULL, mcp251x_can_ist,
@@ -1006,8 +1005,7 @@ MODULE_DEVICE_TABLE(spi, mcp251x_id_table);
 
 static int mcp251x_can_probe(struct spi_device *spi)
 {
-	const struct of_device_id *of_id = of_match_device(mcp251x_of_match,
-							   &spi->dev);
+	const void *match = device_get_match_data(&spi->dev);
 	struct mcp251x_platform_data *pdata = dev_get_platdata(&spi->dev);
 	struct net_device *net;
 	struct mcp251x_priv *priv;
@@ -1044,8 +1042,8 @@ static int mcp251x_can_probe(struct spi_device *spi)
 	priv->can.clock.freq = freq / 2;
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
 		CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY;
-	if (of_id)
-		priv->model = (enum mcp251x_model)of_id->data;
+	if (match)
+		priv->model = (enum mcp251x_model)match;
 	else
 		priv->model = spi_get_device_id(spi)->driver_data;
 	priv->net = net;
-- 
2.23.0.rc1

[PATCH v2 3/4] can: mcp251x: Call wrapper instead of regulator_disable()

[Andy Shevchenko]

There is no need to check for regulator presence in the ->suspend()
since a wrapper does it for us. Due to this we may unconditionally set
AFTER_SUSPEND_POWER flag.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/net/can/spi/mcp251x.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index 0b7e743ca0a0..6ee0ea51399a 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -1162,10 +1162,8 @@ static int __maybe_unused mcp251x_can_suspend(struct device *dev)
 		priv->after_suspend = AFTER_SUSPEND_DOWN;
 	}
 
-	if (!IS_ERR_OR_NULL(priv->power)) {
-		regulator_disable(priv->power);
-		priv->after_suspend |= AFTER_SUSPEND_POWER;
-	}
+	mcp251x_power_enable(priv->power, 0);
+	priv->after_suspend |= AFTER_SUSPEND_POWER;
 
 	return 0;
 }
-- 
2.23.0.rc1

[PATCH v2 4/4] can: mcp251x: Get rid of legacy platform data

[Andy Shevchenko]

Instead of using legacy platform data, switch to use device properties.
For clock frequency we are using well established clock-frequency property.

Users, two for now, are also converted here.

Cc: Daniel Mack <daniel@zonque.org>
Cc: Haojian Zhuang <haojian.zhuang@gmail.com>
Cc: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/arm/mach-pxa/icontrol.c         |  9 +++++----
 arch/arm/mach-pxa/zeus.c             |  9 +++++----
 drivers/net/can/spi/mcp251x.c        | 19 ++++++++-----------
 include/linux/can/platform/mcp251x.h | 22 ----------------------
 4 files changed, 18 insertions(+), 41 deletions(-)
 delete mode 100644 include/linux/can/platform/mcp251x.h

diff --git a/arch/arm/mach-pxa/icontrol.c b/arch/arm/mach-pxa/icontrol.c
index 865b10344ea2..aa4ccb9bb1c1 100644
--- a/arch/arm/mach-pxa/icontrol.c
+++ b/arch/arm/mach-pxa/icontrol.c
@@ -12,6 +12,7 @@
 
 #include <linux/irq.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/gpio.h>
 
 #include <asm/mach-types.h>
@@ -22,7 +23,6 @@
 
 #include <linux/spi/spi.h>
 #include <linux/spi/pxa2xx_spi.h>
-#include <linux/can/platform/mcp251x.h>
 #include <linux/regulator/machine.h>
 
 #include "generic.h"
@@ -69,8 +69,9 @@ static struct pxa2xx_spi_chip mcp251x_chip_info4 = {
 	.gpio_cs        = ICONTROL_MCP251x_nCS4
 };
 
-static struct mcp251x_platform_data mcp251x_info = {
-	.oscillator_frequency = 16E6,
+static const struct property_entry mcp251x_properties = {
+	PROPERTY_ENTRY_U32("clock-frequency", 16000000),
+	{}
 };
 
 static struct spi_board_info mcp251x_board_info[] = {
@@ -79,7 +80,7 @@ static struct spi_board_info mcp251x_board_info[] = {
 		.max_speed_hz    = 6500000,
 		.bus_num         = 3,
 		.chip_select     = 0,
-		.platform_data   = &mcp251x_info,
+		.properties      = &mcp251x_properties,
 		.controller_data = &mcp251x_chip_info1,
 		.irq             = PXA_GPIO_TO_IRQ(ICONTROL_MCP251x_nIRQ1)
 	},
diff --git a/arch/arm/mach-pxa/zeus.c b/arch/arm/mach-pxa/zeus.c
index da113c8eefbf..645500ef427a 100644
--- a/arch/arm/mach-pxa/zeus.c
+++ b/arch/arm/mach-pxa/zeus.c
@@ -13,6 +13,7 @@
 #include <linux/leds.h>
 #include <linux/irq.h>
 #include <linux/pm.h>
+#include <linux/property.h>
 #include <linux/gpio.h>
 #include <linux/gpio/machine.h>
 #include <linux/serial_8250.h>
@@ -27,7 +28,6 @@
 #include <linux/platform_data/i2c-pxa.h>
 #include <linux/platform_data/pca953x.h>
 #include <linux/apm-emulation.h>
-#include <linux/can/platform/mcp251x.h>
 #include <linux/regulator/fixed.h>
 #include <linux/regulator/machine.h>
 
@@ -428,14 +428,15 @@ static struct gpiod_lookup_table can_regulator_gpiod_table = {
 	},
 };
 
-static struct mcp251x_platform_data zeus_mcp2515_pdata = {
-	.oscillator_frequency	= 16*1000*1000,
+static const struct property_entry mcp251x_properties = {
+	PROPERTY_ENTRY_U32("clock-frequency", 16000000),
+	{}
 };
 
 static struct spi_board_info zeus_spi_board_info[] = {
 	[0] = {
 		.modalias	= "mcp2515",
-		.platform_data	= &zeus_mcp2515_pdata,
+		.properties	= &mcp251x_properties,
 		.irq		= PXA_GPIO_TO_IRQ(ZEUS_CAN_GPIO),
 		.max_speed_hz	= 1*1000*1000,
 		.bus_num	= 3,
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index 6ee0ea51399a..3a4d7089dc7c 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -20,29 +20,26 @@
  *
  * Your platform definition file should specify something like:
  *
- * static struct mcp251x_platform_data mcp251x_info = {
- *         .oscillator_frequency = 8000000,
+ * static const struct property_entry mpc251x_properties[] = {
+ *         PROPERTY_ENTRY_U32("clock-frequency", 8000000),
+ *         {}
  * };
  *
  * static struct spi_board_info spi_board_info[] = {
  *         {
  *                 .modalias = "mcp2510",
  *			// "mcp2515" or "mcp25625" depending on your controller
- *                 .platform_data = &mcp251x_info,
+ *                 .properties = &mcp251x_properties,
  *                 .irq = IRQ_EINT13,
  *                 .max_speed_hz = 2*1000*1000,
  *                 .chip_select = 2,
  *         },
  * };
- *
- * Please see mcp251x.h for a description of the fields in
- * struct mcp251x_platform_data.
  */
 
 #include <linux/can/core.h>
 #include <linux/can/dev.h>
 #include <linux/can/led.h>
-#include <linux/can/platform/mcp251x.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
@@ -1006,19 +1003,19 @@ MODULE_DEVICE_TABLE(spi, mcp251x_id_table);
 static int mcp251x_can_probe(struct spi_device *spi)
 {
 	const void *match = device_get_match_data(&spi->dev);
-	struct mcp251x_platform_data *pdata = dev_get_platdata(&spi->dev);
 	struct net_device *net;
 	struct mcp251x_priv *priv;
 	struct clk *clk;
-	int freq, ret;
+	u32 freq;
+	int ret;
 
 	clk = devm_clk_get_optional(&spi->dev, NULL);
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
 	freq = clk_get_rate(clk);
-	if (freq == 0 && pdata)
-		freq = pdata->oscillator_frequency;
+	if (freq == 0)
+		device_property_read_u32(&spi->dev, "clock-frequency", &freq);
 
 	/* Sanity check */
 	if (freq < 1000000 || freq > 25000000)
diff --git a/include/linux/can/platform/mcp251x.h b/include/linux/can/platform/mcp251x.h
deleted file mode 100644
index 9e5ac27fb6c1..000000000000
--- a/include/linux/can/platform/mcp251x.h
+++ /dev/null
@@ -1,22 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _CAN_PLATFORM_MCP251X_H
-#define _CAN_PLATFORM_MCP251X_H
-
-/*
- *
- * CAN bus driver for Microchip 251x CAN Controller with SPI Interface
- *
- */
-
-#include <linux/spi/spi.h>
-
-/*
- * struct mcp251x_platform_data - MCP251X SPI CAN controller platform data
- * @oscillator_frequency:       - oscillator frequency in Hz
- */
-
-struct mcp251x_platform_data {
-	unsigned long oscillator_frequency;
-};
-
-#endif /* !_CAN_PLATFORM_MCP251X_H */
-- 
2.23.0.rc1

Re: [PATCH v2 4/4] can: mcp251x: Get rid of legacy platform data

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1950 bytes --]

On 9/3/19 2:42 PM, Andy Shevchenko wrote:
> Instead of using legacy platform data, switch to use device properties.
> For clock frequency we are using well established clock-frequency property.
> 
> Users, two for now, are also converted here.

[...]

> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
> index 6ee0ea51399a..3a4d7089dc7c 100644
> --- a/drivers/net/can/spi/mcp251x.c
> +++ b/drivers/net/can/spi/mcp251x.c
> @@ -20,29 +20,26 @@
>   *
>   * Your platform definition file should specify something like:
>   *
> - * static struct mcp251x_platform_data mcp251x_info = {
> - *         .oscillator_frequency = 8000000,
> + * static const struct property_entry mpc251x_properties[] = {
> + *         PROPERTY_ENTRY_U32("clock-frequency", 8000000),
> + *         {}
>   * };
>   *
>   * static struct spi_board_info spi_board_info[] = {
>   *         {
>   *                 .modalias = "mcp2510",
>   *			// "mcp2515" or "mcp25625" depending on your controller
> - *                 .platform_data = &mcp251x_info,
> + *                 .properties = &mcp251x_properties,
>   *                 .irq = IRQ_EINT13,
>   *                 .max_speed_hz = 2*1000*1000,
>   *                 .chip_select = 2,
>   *         },
>   * };
> - *
> - * Please see mcp251x.h for a description of the fields in
> - * struct mcp251x_platform_data.
>   */

I've removed this section already in this patch:

https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?h=linux-can-next-for-5.4-20190903&id=f6cae800bfdb6711f0d45af98643a944998be6f2

...I've dropped that hunk.

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/4] can: mcp251x: Get rid of legacy platform data

[Andy Shevchenko]

On Tue, Sep 03, 2019 at 03:17:11PM +0200, Marc Kleine-Budde wrote:
> On 9/3/19 2:42 PM, Andy Shevchenko wrote:
> > Instead of using legacy platform data, switch to use device properties.
> > For clock frequency we are using well established clock-frequency property.
> > 
> > Users, two for now, are also converted here.
> 
> I've removed this section already in this patch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?h=linux-can-next-for-5.4-20190903&id=f6cae800bfdb6711f0d45af98643a944998be6f2
> 
> ...I've dropped that hunk.

Awesome, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/4] can: mcp251x: Make use of device properties

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1152 bytes --]

On 9/3/19 2:42 PM, Andy Shevchenko wrote:
> The purpose of this series is to simplify driver by switching to use device
> properties. In particular it allows to drop legacy platform data.
> 
> Patch 1 switches driver to use devm_clk_get_optional() API.
> 
> Patch 2 unifies getting the driver data independently of the table which
> provides it.
> 
> Patch 3 drops extra check for regulator presence by switch to use an already
> present wrapper.
> 
> And patch 4 gets rid of legacy platform data.
> 
> Changelog v2:
> - add patch 4 to get rid of legacy platform data

Sorry for not telling, your v1 series has already been applied and it's
included in the latest pull request. Can you please rebase your patches
on top of:

https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/tag/?h=linux-can-next-for-5.4-20190903

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/4] can: mcp251x: Make use of device properties

[Andy Shevchenko]

On Tue, Sep 03, 2019 at 02:50:05PM +0200, Marc Kleine-Budde wrote:
> On 9/3/19 2:42 PM, Andy Shevchenko wrote:
> > The purpose of this series is to simplify driver by switching to use device
> > properties. In particular it allows to drop legacy platform data.
> > 
> > Patch 1 switches driver to use devm_clk_get_optional() API.
> > 
> > Patch 2 unifies getting the driver data independently of the table which
> > provides it.
> > 
> > Patch 3 drops extra check for regulator presence by switch to use an already
> > present wrapper.
> > 
> > And patch 4 gets rid of legacy platform data.
> > 
> > Changelog v2:
> > - add patch 4 to get rid of legacy platform data
> 
> Sorry for not telling, your v1 series has already been applied and it's
> included in the latest pull request. Can you please rebase your patches
> on top of:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/tag/?h=linux-can-next-for-5.4-20190903


Thank you!

Basically first 3 patches didn't change. Consider patch 4 only. Should I resend
it separately as v3?

-- 
With Best Regards,
Andy Shevchenko