Re: [RFC net-next v2 2/2] net: dsa: microchip: Add drive strength configuration

[Russell King (Oracle)]

On Wed, Sep 06, 2023 at 12:59:04PM +0200, Oleksij Rempel wrote:
> +static void ksz9477_drive_strength_error(struct ksz_device *dev, int milliamp)
> +{
> +	size_t array_size = ARRAY_SIZE(ksz9477_drive_strengths);
> +	char supported_values[100];
> +	int i;
> +
> +	for (i = 0; i < array_size; i++) {
> +		if (i == 0)
> +			snprintf(supported_values, sizeof(supported_values),
> +				 "%d", ksz9477_drive_strengths[i].milliamp);
> +		else
> +			snprintf(supported_values, sizeof(supported_values),
> +				 "%s, %d", supported_values,
> +				 ksz9477_drive_strengths[i].milliamp);

That's an interesting way to append... I note that snprintf(3) has a
note about this, suggesting that (a) the standards make this undefined
and (b) that depending on the gcc version used, this may not produce
the expected results. Taking both together seems sufficient
justification to stay away from attempting this method of appending
a string.

> +static int ksz9477_drive_strength_write(struct ksz_device *dev,
> +					struct ksz_driver_strength_prop *props,
> +					int num_props)
> +{
> +	int i, ret, reg;
> +	u8 val;
	u8 val, mask;

> +
> +	if (props[KSZ_DRIVER_STRENGTH_IO].value != -1)
> +		dev_warn(dev->dev, "%s is not supported by this chip variant\n",
> +			 props[KSZ_DRIVER_STRENGTH_IO].name);
> +
> +	if (dev->chip_id == KSZ8795_CHIP_ID ||
> +	    dev->chip_id == KSZ8794_CHIP_ID ||
> +	    dev->chip_id == KSZ8765_CHIP_ID)
> +		reg = KSZ8795_REG_SW_CTRL_20;
> +	else
> +		reg = KSZ9477_REG_SW_IO_STRENGTH;
> +

> +	ret = ksz_read8(dev, reg, &val);
> +	if (ret)
> +		return ret;
> +
Remote this.

	val = mask = 0;

> +	for (i = 0; i < num_props; i++) {
> +		if (props[i].value == -1)
> +			continue;
> +
> +		ret = ksz9477_drive_strength_to_reg(props[i].value);
> +		if (ret < 0) {
> +			ksz9477_drive_strength_error(dev, props[i].value);
> +			return ret;
> +		}
> +
> +		val &= ~(SW_DRIVE_STRENGTH_M << props[i].offset);

		mask |= SW_DRIVE_STRENGTH_M << props[i].offset;

> +		val |= ret << props[i].offset;

		val |= ret << props[i].offset;

> +	}
> +
> +	return ksz_write8(dev, reg, val);

	return ksz_rmw8(dev, reg, mask, val);

maybe safer?


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!