[PATCH v2 2/3] can: sun4i_can: add support for R40 CAN controller

[Evgeny Boger]

11.01.2022 19:24, Andre Przywara пишет:
> On Tue, 11 Jan 2022 18:57:08 +0300
> Evgeny Boger <boger at wirenboard.com> wrote:
>
> Hi,
>
>> Allwinner R40 (also known as A40i, T3, V40) has a CAN controller. The
>> controller is the same as in earlier A10 and A20 SoCs, but needs reset
>> line to be deasserted before use.
>>
>> This patch adds a new compatible for R40 CAN controller. Depending
>> on the compatible, reset line can be requested from DT.
>>
>> Signed-off-by: Evgeny Boger <boger at wirenboard.com>
>> ---
>>   drivers/net/can/sun4i_can.c | 61 ++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/sun4i_can.c b/drivers/net/can/sun4i_can.c
>> index 54aa7c25c4de..24a61326915e 100644
>> --- a/drivers/net/can/sun4i_can.c
>> +++ b/drivers/net/can/sun4i_can.c
>> @@ -61,6 +61,7 @@
>>   #include <linux/of.h>
>>   #include <linux/of_device.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/reset.h>
>>   
>>   #define DRV_NAME "sun4i_can"
>>   
>> @@ -200,10 +201,20 @@
>>   #define SUN4I_CAN_MAX_IRQ	20
>>   #define SUN4I_MODE_MAX_RETRIES	100
>>   
>> +/**
>> + * struct sun4ican_quirks - Differences between SoC variants.
>> + *
>> + * @has_reset: SoC needs reset deasserted.
>> + */
>> +struct sun4ican_quirks {
>> +	bool		has_reset;
>> +};
>> +
>>   struct sun4ican_priv {
>>   	struct can_priv can;
>>   	void __iomem *base;
>>   	struct clk *clk;
>> +	struct reset_control *reset;
>>   	spinlock_t cmdreg_lock;	/* lock for concurrent cmd register writes */
>>   };
>>   
>> @@ -702,6 +713,13 @@ static int sun4ican_open(struct net_device *dev)
>>   		goto exit_irq;
>>   	}
>>   
>> +	/* software reset deassert */
>> +	err = reset_control_deassert(priv->reset);
>> +	if (err) {
>> +		netdev_err(dev, "could not deassert CAN reset\n");
>> +		goto exit_soft_reset;
>> +	}
>> +
>>   	/* turn on clocking for CAN peripheral block */
>>   	err = clk_prepare_enable(priv->clk);
>>   	if (err) {
>> @@ -723,6 +741,8 @@ static int sun4ican_open(struct net_device *dev)
>>   exit_can_start:
>>   	clk_disable_unprepare(priv->clk);
>>   exit_clock:
>> +	reset_control_assert(priv->reset);
>> +exit_soft_reset:
>>   	free_irq(dev->irq, dev);
>>   exit_irq:
>>   	close_candev(dev);
>> @@ -736,6 +756,7 @@ static int sun4ican_close(struct net_device *dev)
>>   	netif_stop_queue(dev);
>>   	sun4i_can_stop(dev);
>>   	clk_disable_unprepare(priv->clk);
>> +	reset_control_assert(priv->reset);
>>   
>>   	free_irq(dev->irq, dev);
>>   	close_candev(dev);
>> @@ -750,8 +771,27 @@ static const struct net_device_ops sun4ican_netdev_ops = {
>>   	.ndo_start_xmit = sun4ican_start_xmit,
>>   };
>>   
>> +static const struct sun4ican_quirks sun4ican_quirks_a10 = {
>> +	.has_reset = false,
>> +};
>> +
>> +static const struct sun4ican_quirks sun4ican_quirks_r40 = {
>> +	.has_reset = true,
>> +};
>> +
>>   static const struct of_device_id sun4ican_of_match[] = {
>> -	{.compatible = "allwinner,sun4i-a10-can"},
>> +	{
>> +		.compatible = "allwinner,sun4i-a10-can",
>> +		.data = &sun4ican_quirks_a10
>> +	},
>> +	{
>> +		.compatible = "allwinner,sun7i-a20-can",
>> +		.data = &sun4ican_quirks_a10
>> +	},
> This one looks unnecessary, the binding says that it must be paired with
> the A10 fallback.
> We should only add an A20 compatible match if there is really a reason,
> otherwise it encourages people to produce DTs with *only* the A20
> compatible (violating the binding).

Right, thank you! Since this series has been around for a couple of 
months already,
and changes in v2 are trivial, would it make sense to submit v3 right away?


> Cheers,
> Andre
>
>> +	{
>> +		.compatible = "allwinner,sun8i-r40-can",
>> +		.data = &sun4ican_quirks_r40
>> +	},
>>   	{},
>>   };
>>   
>> @@ -771,10 +811,28 @@ static int sun4ican_probe(struct platform_device *pdev)
>>   {
>>   	struct device_node *np = pdev->dev.of_node;
>>   	struct clk *clk;
>> +	struct reset_control *reset = NULL;
>>   	void __iomem *addr;
>>   	int err, irq;
>>   	struct net_device *dev;
>>   	struct sun4ican_priv *priv;
>> +	const struct sun4ican_quirks *quirks;
>> +
>> +	quirks = of_device_get_match_data(&pdev->dev);
>> +	if (!quirks) {
>> +		dev_err(&pdev->dev, "failed to determine the quirks to use\n");
>> +		err = -ENODEV;
>> +		goto exit;
>> +	}
>> +
>> +	if (quirks->has_reset) {
>> +		reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>> +		if (IS_ERR(reset)) {
>> +			dev_err(&pdev->dev, "unable to request reset\n");
>> +			err = PTR_ERR(reset);
>> +			goto exit;
>> +		}
>> +	}
>>   
>>   	clk = of_clk_get(np, 0);
>>   	if (IS_ERR(clk)) {
>> @@ -818,6 +876,7 @@ static int sun4ican_probe(struct platform_device *pdev)
>>   				       CAN_CTRLMODE_3_SAMPLES;
>>   	priv->base = addr;
>>   	priv->clk = clk;
>> +	priv->reset = reset;
>>   	spin_lock_init(&priv->cmdreg_lock);
>>   
>>   	platform_set_drvdata(pdev, dev);