Re: [PATCH net-next v4 2/2] net: pse-pd: add Realtek/Broadcom PSE MCU driver

[Jonas Jelonek]

Hi Oleksij,

thanks for your review.

On 02.07.26 09:13, Oleksij Rempel wrote:
> Hi Jonas,
>
> On Tue, Jun 30, 2026 at 10:56:50AM +0000, Jonas Jelonek wrote:
>> [...]
>>
>> +struct rtpse_mcu_port_config {
>> +	bool enable;
> in this struct we use only enable, do you plan to wire it somewhere
> later? I assume you wont to keep it as documentation. May be add some
> debug traces? And add some comments what do you already know about this
> fields.
>
>> +	u8 function_mode;
>> +	u8 detection_type;
>> +	u8 cls_type;
>> +	u8 disconnect_type;
>> +	u8 pair_type;
>> +};
>> +
>> +struct rtpse_mcu_port_ext_config {
> Same here.
>
>> +	u8 inrush_mode;
>> +	u8 limit_type;
>> +	u8 max_power;
>> +	u8 priority;
>> +	u8 chip_addr;
>> +	u8 channel;
>> +};

This might be good for follow-up patches. Right now, I don't get
the full complexity of the pse-pd framework so I'll rather keep it
simple. I see that at least some of this information is reportable,
though not configurable (yet).

I suppose it's preferred to not add unused stuff so I should just
drop the unused fields and add them later as needed. There isn't
an urgent need to document it here either, the guide I based my
driver on is quite available here and there in the web. I haven't
actually dealt with all those fields close enough, so adding
half-baked comments about them isn't good either.

>> +			break;
>> +		msleep(RTPSE_MCU_BOOT_RETRY_MS);
>> +	} while (time_before(jiffies, deadline));
>> +	if (ret)
>> +		return dev_err_probe(pse->dev, ret, "failed to read MCU info\n");
>> +
>> +	switch (info->device_id) {
>> +	case RTPSE_MCU_DEVICE_ID_RTL8238B:
>> +		pse->chip = &rtl8238b_info;
>> +		break;
>> +	case RTPSE_MCU_DEVICE_ID_RTL8239:
>> +		pse->chip = &rtl8239_info;
>> +		break;
>> +	case RTPSE_MCU_DEVICE_ID_RTL8239C:
>> +		pse->chip = &rtl8239c_info;
>> +		break;
>> +	case RTPSE_MCU_DEVICE_ID_BCM59111:
>> +		pse->chip = &bcm59111_info;
>> +		break;
>> +	case RTPSE_MCU_DEVICE_ID_BCM59121:
>> +		pse->chip = &bcm59121_info;
>> +		break;
>> +	default:
>> +		return dev_err_probe(pse->dev, -EINVAL, "unknown PSE id 0x%x\n",
>> +				     info->device_id);
>> +	}
>> +
>> +	if (!info->max_ports || info->max_ports > RTPSE_MCU_MAX_PORTS)
>> +		return dev_err_probe(pse->dev, -EINVAL,
>> +				     "MCU reports invalid port count %u\n", info->max_ports);
>> +
>> +	ret = rtpse_mcu_get_ext_config(pse, &ext_config);
>> +	if (ret)
>> +		return dev_err_probe(pse->dev, ret, "failed to read MCU ext config\n");
>> +
>> +	dev_info(pse->dev, "%s MCU, %s (id 0x%04x), %u ports across %u PSE chip(s)\n",
>> +		 pse->dialect->mcu_type_str(info->mcu_type), pse->chip->name,
>> +		 info->device_id, info->max_ports, ext_config.num_of_pses);
> Reduce it to debug level print.

Is there any guideline about what drivers are allowed to print during
probe? Usually, I'm a fan of at least seeing an immediate sign of life
from a driver, even in success case. And I also see that from drivers in
other subsystems, e.g. fan controllers or GPIO expanders.

>> +	return 0;
>> +}
>> +
> Best Regards,
> Oleksij

I'll address the other points I've not reacted to explicitly in the next
version then. Same for the low-severity sashiko-nipa issues.

Best regards,
Jonas