Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Lukasz Stelmach <l.stelmach@samsung.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     jim.cromie@gmail.com, Heiner Kallweit <hkallweit1@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Kukjin Kim <kgene@kernel.org>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Russell King <linux@armlinux.org.uk>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org,
        =?utf-8?Q?Bart=C5=82omiej_=C5=BBolni?= =?utf-8?Q?erkiewicz?= 
        <b.zolnierkie@samsung.com>,
        Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: [PATCH v4 3/5] net: ax88796c: ASIX AX88796C SPI Ethernet
 Adapter Driver
Date:   Thu, 29 Oct 2020 14:09:57 +0100
In-Reply-To: <20201029003131.GF933237@lunn.ch> (Andrew Lunn's message of
        "Thu, 29 Oct 2020 01:31:31 +0100")
Message-ID: <dleftjwnz9uqje.fsf%l.stelmach@samsung.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-="; micalg="pgp-sha256";
        protocol="application/pgp-signature"
CMS-TYPE: 201P
References: <20201029003131.GF933237@lunn.ch>
        <CGME20201029131011eucas1p1194c5614ca8f5d3835f888c8d1c09fa1@eucas1p1.samsung.com>
Precedence: bulk

--=-=-=
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

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, v=
oid *_p)
>> +{
>> +	struct ax88796c_device *ax_local =3D to_ax88796c_device(ndev);
>> +	u16 *p =3D _p;
>> +	int offset, i;
>
> You missed a reverse christmass tree fix here.
>

Done.

>> +static int comp;
>> +static int msg_enable =3D 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 =3D "80018001,e1918001,8001a001,fc0d0000";
>> +unsigned long ax88796c_no_regs_mask[AX88796C_REGDUMP_LEN / (sizeof(unsi=
gned long) * 8)];
>> +
>> +module_param(comp, int, 0444);
>> +MODULE_PARM_DESC(comp, "0=3DNon-Compression Mode, 1=3DCompression 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 b=
itmap)");
>
> 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 =3D to_ax88796c_device(ndev);
>> +	u16 rx_ctl =3D RXCR_AB;
>> +	int mc_count =3D 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 =3D 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 =3D ax_local->ax_spi.comp ? 1 : 4;
>
> reverse christmass tree.
>

Done.

>> +static int ax88796c_receive(struct net_device *ndev)
>> +{
>> +	struct ax88796c_device *ax_local =3D 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 =3D 0;
>> +	struct net_device *ndev =3D ax_local->ndev;
>
> ...
>

Done.

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

Done.

>> +static int
>> +ax88796c_open(struct net_device *ndev)
>> +{
>> +	struct ax88796c_device *ax_local =3D to_ax88796c_device(ndev);
>> +	int ret;
>> +	unsigned long irq_flag =3D IRQF_SHARED;
>> +	int fc =3D 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.

=2D-=20
=C5=81ukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEXpuyqjq9kGEVr9UQsK4enJilgBAFAl+avyYACgkQsK4enJil
gBCBMwf9FnD8E89ec3soQSzXByQAO6P8tm2DCbrdWpJDtt+llFnrA6h087U7EBmk
Qy/Wd/hPuhBjeNUYeX0BxJL3SHEFixlk2M18HFCJzTNbpBof7sYra9iB/q6NF/b9
Cs+ga5m/hfLpdSqVmQdsFiWwxCpjldB5htjBOzc7cnMnCkVPKJ+Cw4/KwYFlnGOR
F9TAPzugNnG2AH9JlOwBTZp/J6jTRlAp/naXssYWcWwJVSutidjyFJ472eElBkU/
2q13sSwY98lR6bYlvsGEV5XFIWFiSaLNJq3rUYelbojDR3E7ZO3opbzdogn0ROxY
MvqWStI2o5TBCDiHwp1RePhF6LdunA==
=n2/L
-----END PGP SIGNATURE-----
--=-=-=--