Re: [PATCH v4 3/5] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

[Lukasz Stelmach]

It was <2020-10-29 czw 01:31>, when Andrew Lunn wrote:
>> +static void
>> +ax88796c_get_regs(struct net_device *ndev, struct ethtool_regs *regs, void *_p)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	u16 *p = _p;
>> +	int offset, i;
>
> You missed a reverse christmass tree fix here.
>

Done.

>> +static int comp;
>> +static int msg_enable = NETIF_MSG_PROBE |
>> +			NETIF_MSG_LINK |
>> +			/* NETIF_MSG_TIMER | */
>> +			/* NETIF_MSG_IFDOWN | */
>> +			/* NETIF_MSG_IFUP | */
>> +			NETIF_MSG_RX_ERR |
>> +			NETIF_MSG_TX_ERR |
>> +			/* NETIF_MSG_TX_QUEUED | */
>> +			/* NETIF_MSG_INTR | */
>> +			/* NETIF_MSG_TX_DONE | */
>> +			/* NETIF_MSG_RX_STATUS | */
>> +			/* NETIF_MSG_PKTDATA | */
>> +			/* NETIF_MSG_HW | */
>> +			/* NETIF_MSG_WOL | */
>> +			0;
>
> You should probably delete anything which is commented out.
>

Done.

>> +
>> +static char *no_regs_list = "80018001,e1918001,8001a001,fc0d0000";
>> +unsigned long ax88796c_no_regs_mask[AX88796C_REGDUMP_LEN / (sizeof(unsigned long) * 8)];
>> +
>> +module_param(comp, int, 0444);
>> +MODULE_PARM_DESC(comp, "0=Non-Compression Mode, 1=Compression Mode");
>
> I think you need to find a different way to configure this. How much
> does compression bring you anyway?
>

Anything between almost 0 for large transfers, to 50 for tiniest. ~5%
for ~500 byte transfers. Considering the chip is rather for small
devices, that won't transfer large amounts of data, I'd rather keep some
way to control it.

>> +module_param(msg_enable, int, 0444);
>> +MODULE_PARM_DESC(msg_enable, "Message mask (see linux/netdevice.h for bitmap)");
>
> I know a lot of drivers have msg_enable, but DaveM is generally
> against module parameters. So i would remove this.
>

These two parameters have something in common: no(?) other way to pass
the information at the right time. Compression might be tuned in
runtime, if there is an interface (via ethtool?) for setting custom
knobs? Ther is such interface for msg_level level but it can be used
before a device is probed and userland is running. Hence, there is no
way to control msg_level during boot. I can remove those parameters, but
I really would like to be able to control these parameter, especially
msg_level during boot. If there is any other way, do let me know.

>> +static void ax88796c_set_hw_multicast(struct net_device *ndev)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	u16 rx_ctl = RXCR_AB;
>> +	int mc_count = netdev_mc_count(ndev);
>
> reverse christmass tree.
>

Done.

>> +static struct sk_buff *
>> +ax88796c_tx_fixup(struct net_device *ndev, struct sk_buff_head *q)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	struct sk_buff *skb, *tx_skb;
>> +	struct tx_pkt_info *info;
>> +	struct skb_data *entry;
>> +	int headroom;
>> +	int tailroom;
>> +	u8 need_pages;
>> +	u16 tol_len, pkt_len;
>> +	u8 padlen, seq_num;
>> +	u8 spi_len = ax_local->ax_spi.comp ? 1 : 4;
>
> reverse christmass tree.
>

Done.

>> +static int ax88796c_receive(struct net_device *ndev)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	struct sk_buff *skb;
>> +	struct skb_data *entry;
>> +	u16 w_count, pkt_len;
>> +	u8 pkt_cnt;
>
> Reverse christmass tree
>

Done.

>> +
>> +static int ax88796c_process_isr(struct ax88796c_device *ax_local)
>> +{
>> +	u16 isr;
>> +	u8 done = 0;
>> +	struct net_device *ndev = ax_local->ndev;
>
> ...
>

Done.

>> +static irqreturn_t ax88796c_interrupt(int irq, void *dev_instance)
>> +{
>> +	struct net_device *ndev = dev_instance;
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>
> ...
>

Done.

>> +static int
>> +ax88796c_open(struct net_device *ndev)
>> +{
>> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +	int ret;
>> +	unsigned long irq_flag = IRQF_SHARED;
>> +	int fc = AX_FC_NONE;
>
> ...
>

Done.

>> +static int ax88796c_probe(struct spi_device *spi)
>> +{
>> +	struct net_device *ndev;
>> +	struct ax88796c_device *ax_local;
>> +	char phy_id[MII_BUS_ID_SIZE + 3];
>> +	int ret;
>> +	u16 temp;
>
> ...
>

Done.

> The mdio/phy/ethtool code looks O.K. now. I've not really looked at
> any of the frame transfer code, so i cannot comment on that.
>
>     Andrew
>
>

Thanks.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Attachment: signature.asc
Description: PGP signature