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

[Evgeny Boger <boger@xxxxxxxxxxxxxx>]

11.01.2022 19:24, Andre Przywara пишет:
> On Tue, 11 Jan 2022 18:57:08 +0300
> Evgeny Boger <boger@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
> >   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);