Re: [PATCH v2 2/2] ARM: at91: microchip-sama5d3-eds: implement MDIO and SPI extension boards detection

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Oleksij,

Some small nitpicks inside.

On Sat, Jun 22, 2024 at 07:48:15PM +0200, Oleksij Rempel wrote:
> +++ b/arch/arm/boards/microchip-sama5d3-eds/board.c
> @@ -0,0 +1,414 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// SPDX-FileCopyrightText: 2024 Oleksij Rempel <o.rempel@pengutronix.de>
> +
> +#include <common.h>
> +#include <deep-probe.h>
> +#include <envfs.h>
> +#include <environment.h>
> +#include <gpio.h>
> +#include <init.h>
> +#include <linux/phy.h>
> +#include <spi/spi.h>
> +
> +#define BASE_BOARD_NAME_SIZE 100
> +static char base_board_name[BASE_BOARD_NAME_SIZE] = "MCP_S5D3_EDS";
> +
> +extern char __dtbo_sama5d3_eds_rgmii_phy_start[];
> +extern char __dtbo_sama5d3_eds_lan9370_start[];
> +
> +#define MCP_S5D3_EDS_MDIO_EXT_BOARD_FLAG_IGNGORE	BIT(0)

s/IGNGORE/IGNORE/

> +static int mcp_s5d3_eds_spi_find_ksz_switch(struct device *dev, int bus_idx,
> +					    int chip_select, u8 *response,
> +					    size_t response_size)
> +{
> +	struct device_node *spi_node;
> +	struct spi_device spi = {
> +		.max_speed_hz = 1 * 1000 * 1000,  /* 1 MHz */
> +		.bits_per_word = 8,
> +		.chip_select = chip_select,
> +	};
> +	/* read command for ksz switch */
> +	u8 tx_buf[4] = {0x60, 0x00, 0x00, 0x00};
> +	u8 rx_buf[4] = {0};
> +	int ret, i;
> +	bool all_zeros = true, all_ones = true;
> +	char spi_alias[] = "spiX";
> +
> +	snprintf(spi_alias, sizeof(spi_alias), "spi%d", bus_idx);
> +
> +	spi_node = of_find_node_by_alias(NULL, spi_alias);
> +	if (!spi_node)
> +		return -ENODEV;
> +
> +	of_device_ensure_probed(spi_node);
> +
> +	spi.controller = spi_get_controller(bus_idx);
> +	spi.master = spi.controller;
> +
> +	if (!spi.controller) {
> +		dev_err(dev, "SPI bus %d not found\n", bus_idx);
> +		return -ENODEV;
> +	}
> +
> +	ret = spi.controller->setup(&spi);
> +	if (ret) {
> +		dev_err(dev, "Cannot setup SPI controller %d (%d)\n", bus_idx,
> +			ret);

You can use ": %pe\n", ERR_PTR(ret) here.

> +		return ret;
> +	}
> +
> +	ret = spi_write_then_read(&spi, tx_buf, sizeof(tx_buf), rx_buf,
> +				  sizeof(rx_buf));
> +	if (ret) {
> +		dev_err(dev, "Failed to communicate with SPI device on bus %d chip select %d\n",
> +			bus_idx, chip_select);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < sizeof(rx_buf); i++) {
> +		if (rx_buf[i] != 0x00) {
> +			all_zeros = false;
> +		}
> +		if (rx_buf[i] != 0xFF) {
> +			all_ones = false;
> +		}
> +	}
> +
> +	if (all_zeros || all_ones)
> +		return 0;
> +
> +	if (response_size < sizeof(rx_buf)) {
> +		dev_err(dev, "Response storage is too small\n");

No need to print this message, it's more a sanity check if your code is
correct. Also, you can move this up and catch this before you are
sending SPI messages.

> +							scan->spi_bus_idx,
> +							scan->spi_chip_select,
> +							response);
> +		if (!found)
> +			dev_warn(dev, "No match for SPI device on bus %d chip select %d, response: %02x %02x %02x %02x\n",
> +				 scan->spi_bus_idx, scan->spi_chip_select,
> +				 response[0], response[1], response[2],
> +				 response[3]);

printf can print (small) hexdumps now using "%*ph" whic comes in handy
here.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |