Comments below Regards, Roman
On 28 September 2010 19:44, Luca Niccoli <[email protected]> wrote: > This second version of the patch is rebased on the work done by Roman > Yeryomin and applied in r23126. > It corrects a minor miscalculation in the port rate code and cleans the > code a bit, plus it adds support for jumbo frames, port priority and storm > control. > > Signed-off-by: Luca Niccoli <[email protected]> > > > Index: target/linux/generic/files/drivers/net/phy/rtl8366rb.c > =================================================================== > --- target/linux/generic/files/drivers/net/phy/rtl8366rb.c (revisione > 23144) > +++ target/linux/generic/files/drivers/net/phy/rtl8366rb.c (copia locale) > @@ -777,6 +842,61 @@ > return 0; > } > > +static int rtl8366rb_sw_set_port_pri(struct switch_dev *dev, > + const struct switch_attr *attr, > + struct switch_val *val) > +{ > + struct rtl8366_smi *smi = sw_to_rtl8366_smi(dev); > + u32 mask; > + u32 reg; > + u32 data; > + > + if (val->port_vlan >= RTL8366RB_NUM_PORTS || val->value.i > attr->max) > + return -EINVAL; > + > + if (val->port_vlan < 5) { RTL8366RB_PORT_NUM_CPU? > + mask = RTL8366RB_PORT_PRIORITY_MASK << > + (RTL8366RB_PORT_PRIORITY_BITS * > val->port_vlan); > + reg = RTL8366RB_PORT_PRIORITY_BASE; > + data = val->value.i << > + (RTL8366RB_PORT_PRIORITY_BITS * > val->port_vlan); > + } else { > + mask = RTL8366RB_PORT_PRIORITY_MASK; > + reg = RTL8366RB_PORT_PRIORITY_BASE + 1; > + data = val->value.i; > + } > + > + return rtl8366_smi_rmwr(smi, reg, mask, data); > +} > + > +static int rtl8366rb_sw_get_port_pri(struct switch_dev *dev, > + const struct switch_attr *attr, > + struct switch_val *val) > +{ > + struct rtl8366_smi *smi = sw_to_rtl8366_smi(dev); > + u32 reg; > + u32 shift; > + u32 data = 0; > + > + if (val->port_vlan >= RTL8366RB_NUM_PORTS) > + return -EINVAL; > + > + if (val->port_vlan < 5) { RTL8366RB_PORT_NUM_CPU? > + reg = RTL8366RB_PORT_PRIORITY_BASE; > + shift = RTL8366RB_PORT_PRIORITY_BITS * val->port_vlan; > + > + } else { > + reg = RTL8366RB_PORT_PRIORITY_BASE + 1; > + shift = 0; > + } > + > + rtl8366_smi_read_reg(smi, reg, &data); > + > + val->value.i = ((data >> shift) & RTL8366RB_PORT_PRIORITY_MASK); > + > + return 0; > +} > + > static int rtl8366rb_sw_set_port_disable(struct switch_dev *dev, > const struct switch_attr *attr, > struct switch_val *val) > @@ -821,12 +941,11 @@ > { > struct rtl8366_smi *smi = sw_to_rtl8366_smi(dev); > > - if (val->port_vlan >= RTL8366RB_NUM_PORTS) > + if (val->port_vlan >= RTL8366RB_NUM_PORTS || val->value.i > attr->max) if to follow your logic you should append `|| val->value.i < 0` also > return -EINVAL; > > - if (val->value.i > 0 && val->value.i < RTL8366RB_BDTH_SW_MAX) > - val->value.i = (val->value.i - 1) / RTL8366RB_BDTH_BASE; > - else > + val->value.i = (val->value.i - 1) / RTL8366RB_BDTH_UNIT; > + if (val->value.i < 0) > val->value.i = RTL8366RB_BDTH_REG_DEFAULT; so you assume that 0 is valid value? I would think that 0 means unlimited This is probably more a matter of taste but why not to use if/else instead of double assignment? > > return rtl8366_smi_rmwr(smi, RTL8366RB_IB_REG(val->port_vlan), > @@ -862,16 +980,15 @@ > { > struct rtl8366_smi *smi = sw_to_rtl8366_smi(dev); > > - if (val->port_vlan >= RTL8366RB_NUM_PORTS) > + if (val->port_vlan >= RTL8366RB_NUM_PORTS || val->value.i > attr->max) same > return -EINVAL; > > rtl8366_smi_rmwr(smi, RTL8366RB_EB_PREIFG_REG, > RTL8366RB_EB_PREIFG_MASK, > (RTL8366RB_QOS_DEFAULT_PREIFG << RTL8366RB_EB_PREIFG_OFFSET)); > > - if (val->value.i > 0 && val->value.i < RTL8366RB_BDTH_SW_MAX) > - val->value.i = (val->value.i - 1) / RTL8366RB_BDTH_BASE; > - else > + val->value.i = (val->value.i - 1) / RTL8366RB_BDTH_UNIT; > + if (val->value.i < 0) > val->value.i = RTL8366RB_BDTH_REG_DEFAULT; 0 is valid? > > return rtl8366_smi_rmwr(smi, RTL8366RB_EB_REG(val->port_vlan), _______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
