Re: [PATCH v10 10/15] spi: dw: Add support for AMD Pensando Elba SoC

[Serge Semin <fancer.lancer@xxxxxxxxx>]

On Mon, Mar 06, 2023 at 06:20:02PM -0800, Brad Larson wrote:
> On Mon, Mar 06, 2023 at 16:00, Serge Semin wrote:
> > On Sun, Mar 05, 2023 at 08:07:34PM -0800, Brad Larson wrote:
> >> The AMD Pensando Elba SoC includes a DW apb_ssi v4 controller
> >> with device specific chip-select control.  The Elba SoC
> >> provides four chip-selects where the native DW IP supports
> >> two chip-selects.  The Elba DW_SPI instance has two native
> >> CS signals that are always overridden.
> >> 
> >> Signed-off-by: Brad Larson <blarson@xxxxxxx>
> >> ---
> >> 
> >> v10 changes:
> >> - Delete struct dw_spi_elba, use regmap directly in priv
> >> 
> >> v9 changes:
> >> - Add use of macros GENMASK() and BIT()
> >> - Change ELBA_SPICS_SHIFT() to ELBA_SPICS_OFFSET()
> >> 
> >> ---
> >>  drivers/spi/spi-dw-mmio.c | 65 +++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 65 insertions(+)
> >> 
> >> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> >> index 26c40ea6dd12..2076cb83a11b 100644
> >> --- a/drivers/spi/spi-dw-mmio.c
> >> +++ b/drivers/spi/spi-dw-mmio.c
> >> @@ -53,6 +53,20 @@ struct dw_spi_mscc {
> >>  	void __iomem        *spi_mst; /* Not sparx5 */
> >>  };
> >>  
> >> +/*
> >> + * Elba SoC does not use ssi, pin override is used for cs 0,1 and
> >> + * gpios for cs 2,3 as defined in the device tree.
> >> + *
> >> + * cs:  |       1               0
> >> + * bit: |---3-------2-------1-------0
> >> + *      |  cs1   cs1_ovr   cs0   cs0_ovr
> >> + */
> >> +#define ELBA_SPICS_REG			0x2468
> >> +#define ELBA_SPICS_OFFSET(cs)		((cs) << 1)
> >> +#define ELBA_SPICS_MASK(cs)		(GENMASK(1, 0) << ELBA_SPICS_OFFSET(cs))
> >> +#define ELBA_SPICS_SET(cs, val)		\
> >> +		((((val) << 1) | BIT(0)) << ELBA_SPICS_OFFSET(cs))
> >> +
> >>  /*
> >>   * The Designware SPI controller (referred to as master in the documentation)
> >>   * automatically deasserts chip select when the tx fifo is empty. The chip
> >> @@ -237,6 +251,56 @@ static int dw_spi_canaan_k210_init(struct platform_device *pdev,
> >>  	return 0;
> >>  }
> >>  
> >> +static void dw_spi_elba_override_cs(struct regmap *syscon, int cs, int enable)
> >> +{
> >> +	regmap_update_bits(syscon, ELBA_SPICS_REG, ELBA_SPICS_MASK(cs),
> >> +			   ELBA_SPICS_SET(cs, enable));
> >> +}
> >> +
> >> +static void dw_spi_elba_set_cs(struct spi_device *spi, bool enable)
> >> +{
> >> +	struct dw_spi *dws = spi_master_get_devdata(spi->master);
> >> +	struct dw_spi_mmio *dwsmmio = container_of(dws, struct dw_spi_mmio, dws);
> >> +	struct regmap *syscon = dwsmmio->priv;
> >> +	u8 cs;
> >> +
> >> +	cs = spi->chip_select;
> >> +	if (cs < 2)
> >> +		dw_spi_elba_override_cs(syscon, spi->chip_select, enable);
> >> +
> >> +	/*
> >> +	 * The DW SPI controller needs a native CS bit selected to start
> >> +	 * the serial engine.
> >> +	 */
> >> +	spi->chip_select = 0;
> >> +	dw_spi_set_cs(spi, enable);
> >> +	spi->chip_select = cs;
> >> +}
> >> +
> >> +static int dw_spi_elba_init(struct platform_device *pdev,
> >> +			    struct dw_spi_mmio *dwsmmio)
> >> +{
> >> +	const char *syscon_name = "amd,pensando-elba-syscon";
> >
> >> +	struct device_node *np = pdev->dev.of_node;
> >
> > Drop this since it's used only once below. 
> >
> 
> Removed
> 
> >> +	struct device_node *node;                                         
> 
> Renamed *node to *np
> 
> >> +	struct regmap *syscon;                                            
> >> +                                                                       
> >> -	node = of_parse_phandle(np, syscon_name, 0);                      
> >
> >	node = of_parse_phandle(dev_of_node(pdev->dev), syscon_name, 0);
> >
> > +	if (!node)
> >
> >> +		return dev_err_probe(&pdev->dev, -ENODEV, "failed to find %s\n",
> >> +				     syscon_name);
> >
> > Hm, using dev_err_probe() with known error value seems overkill.
> 
> Changed to: return -ENODEV
> 
> >> +
> >
> >> +	syscon = syscon_node_to_regmap(node);
> >> +	if (IS_ERR(syscon))
> >> +		return dev_err_probe(&pdev->dev, PTR_ERR(syscon),
> >> +				     "syscon regmap lookup failed\n");
> >
> > of_node_put() is missing in the error and success paths.
> 
> Result of the above changes are:
> 
> +       const char *syscon_name = "amd,pensando-elba-syscon";
> +       struct device_node *np;
> +       struct regmap *syscon;
> +

> +       np = of_parse_phandle(pdev->dev.of_node, syscon_name, 0);
> +       if (!np)
> +               return -ENODEV;
> +
> +       syscon = syscon_node_to_regmap(np);
> +       of_node_put(np);
> +       if (IS_ERR(syscon))
> +               return dev_err_probe(&pdev->dev, PTR_ERR(syscon),
> +                                    "syscon regmap lookup failed\n");

As Andy correctly noted this can be fully converted to just a single call:
syscon_regmap_lookup_by_phandle().

and replace pdev->dev.of_node with dev_of_node(pdev->dev).

-Serge(y)

> 
> Regards,
> Brad