Re: [PATCH net-next 2/2] net: dsa: microchip: configure strap pins during reset

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

Hi Andrew,

On 9/10/25 6:46 PM, Andrew Lunn wrote:
>> Support the KSZ8463's strap configuration that enforces SPI as
>> communication bus, since it is the only bus supported by the driver.
> 
> So this is the key sentence for this patchset, which should of been in
> patch 0/X. You have a chicken/egg problem. You cannot talk to the
> switch to put it into SPI mode because you cannot talk to the switch
> using SPI.
> 
> The current patch descriptions, and the patches themselves don't make
> this clear. They just vaguely mention configuration via strapping.
> 

Indeed, that wasn't clear, sorry. My intention was to keep it somewhat 
'generic', since other KSZ switches also use strap configurations. I 
thought the DT property could be re-used by others to enforce different 
kinds of strap configurations.

But I agree with you, it's probably better to have something KSZ8463 
specific.

>> +static int ksz_configure_strap(struct ksz_device *dev)
> 
> Please make it clear this function straps the switch for SPI. If
> somebody does add support for I2C, they need to understand that...
> 
>> +{
>> +	struct pinctrl_state *state = NULL;
>> +	struct pinctrl *pinctrl;
>> +	int ret;
>> +
>> +	if (of_device_is_compatible(dev->dev->of_node, "microchip,ksz8463")) {
> 
> I would not hide this here. Please move this if into
> ksz_switch_register(). I also think this function should have the
> ksz8463 prefix, since how you strap other devices might differ. So
> ksz8463_configure_straps_spi() ?
> 

Ack.

>> +		struct gpio_desc *rxd0;
>> +		struct gpio_desc *rxd1;
>> +
>> +		rxd0 = devm_gpiod_get_index_optional(dev->dev, "strap", 0, GPIOD_OUT_LOW);
>> +		if (IS_ERR(rxd0))
>> +			return PTR_ERR(rxd0);
>> +
>> +		rxd1 = devm_gpiod_get_index_optional(dev->dev, "strap", 1, GPIOD_OUT_HIGH);
>> +		if (IS_ERR(rxd1))
>> +			return PTR_ERR(rxd1);
>> +
>> +		/* If at least one strap definition is missing we don't do anything */
>> +		if (!rxd0 || !rxd1)
>> +			return 0;
> 
> I would say, if you have one, not two, the DT blob is broken, and you
> should return -EINVAL.
> 

Ack.

>> +
>> +		pinctrl = devm_pinctrl_get(dev->dev);
>> +		if (IS_ERR(pinctrl))
>> +			return PTR_ERR(pinctrl);
>> +
>> +		state = pinctrl_lookup_state(pinctrl, "reset");
>> +		if (IS_ERR(state))
>> +			return PTR_ERR(state);
>> +
>> +		ret = pinctrl_select_state(pinctrl, state);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   int ksz_switch_register(struct ksz_device *dev)
>>   {
>>   	const struct ksz_chip_data *info;
>> @@ -5353,10 +5392,18 @@ int ksz_switch_register(struct ksz_device *dev)
>>   		return PTR_ERR(dev->reset_gpio);
>>   
>>   	if (dev->reset_gpio) {
>> +		ret = ksz_configure_strap(dev);
>> +		if (ret)
>> +			return ret;
>> +
>>   		gpiod_set_value_cansleep(dev->reset_gpio, 1);
>>   		usleep_range(10000, 12000);
>>   		gpiod_set_value_cansleep(dev->reset_gpio, 0);
>>   		msleep(100);
>> +
>> +		ret = pinctrl_select_default_state(dev->dev);
>> +		if (ret)
>> +			return ret;
> 
> This does not look symmetrical. Maybe put the
> pinctrl_select_default_state() inside a function called
> ksz8463_release_straps_spi()?
> 
>

Ack.

Best regards,
-- 
Bastien Curutchet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com