Re: [PATCH v3] drivers/net/phy: add driver for Microchip LAN867x 10BASE-T1S PHY

["Ramón Nordin Rodriguez" <ramon.nordin.rodriguez@ferroamp.se>]

On Thu, Apr 20, 2023 at 02:43:45PM +0200, Andrew Lunn wrote:
> On Wed, Apr 19, 2023 at 06:16:05PM +0200, Ramón Nordin Rodriguez wrote:
> > Changes:
> >     v2:
> > - Removed mentioning of not supporting auto-negotiation from commit
> >   message
> > - Renamed file drivers/net/phy/lan867x.c ->
> >   drivers/net/phy/microchip_t1s.c
> > - Renamed Kconfig option to reflect implementation filename (from
> >   LAN867X_PHY to MICROCHIP_T1S_PHY)
> > - Moved entry in drivers/net/phy/KConfig to correct sort order
> > - Moved entry in drivers/net/phy/Makefile to correct sort order
> > - Moved variable declarations to conform to reverse christmas tree order
> >   (in func lan867x_config_init)
> > - Moved register write to disable chip interrupts to func lan867x_config_init, when omitting the irq disable all togheter I got null pointer dereference, see the call trace below:
> > 
> >     Call Trace:
> >      <TASK>
> >      phy_interrupt+0xa8/0xf0 [libphy]
> >      irq_thread_fn+0x1c/0x60
> >      irq_thread+0xf7/0x1c0
> >      ? __pfx_irq_thread_dtor+0x10/0x10
> >      ? __pfx_irq_thread+0x10/0x10
> >      kthread+0xe6/0x110
> >      ? __pfx_kthread+0x10/0x10
> >      ret_from_fork+0x29/0x50
> >      </TASK>
> > 
> > - Removed func lan867x_config_interrupt and removed the member
> >   .config_intr from the phy_driver struct
> > 
> >     v3:
> > - Indentation level in drivers/net/phy/Kconfig
> > - Moved const arrays into global scope and made them static in order to have
> >   them placed in the .rodata section
> > - Renamed array variables, since they are no longer as closely scoped as
> >   earlier
> > - Added comment about why phy_write_mmd is used over phy_modify_mmd
> >   (this should have been addressed in the V2 change since it was brought
> >   up in the V1 review)
> > - Return result of last call instead of saving it in a var and then
> >   returning the var (in lan867x_config_init)
> > 
> > Testing:
> > This has been tested with ethtool --set/get-plca-cfg and verified on an
> > oscilloscope where it was observed that:
> > - The PLCA beacon was enabled/disabled when setting the node-id to 0/not
> >   0
> > - The PLCA beacon is transmitted with the expected frequency when
> >   changing max nodes
> > - Two devices using the evaluation board EVB-LAN8670-USB could ping each
> >   other
> > 
> > 
> > This patch adds support for the Microchip LAN867x 10BASE-T1S family
> > (LAN8670/1/2). The driver supports P2MP with PLCA.
> > 
> > Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
> 
> All the above ends up in the commit message, as you see in git log. So
> this last paragraph should really be first. As you have it, the
> history of the patch will also be included in the commit. Most Linux
> subsystems don't want that, although DaveM has argued it maybe should
> be in the commit message. But i personally think we have good archives
> of emails, and search engines for those archives, so i don't see the
> need.
> 

The intention was not to include the history in the commit message, as
you allude to this was a misstake on my part.

> Anything you put after the --- will get discarded when the Maintainers
> perform the merge. So i suggest you move most of what you have in the
> commit message below the ---. You can also have two ---, which is how
> i tend to do it, so i can keep the history in my git repo, but it then
> gets removed when merged upstream.

I'll move the history to after the ---

> 
> Additionally, you should add any Reviewed-by:, Acked-by: to your
> patches. Only discard them if you make major changed which invalidates
> any reviews.
> 
>     Andrew

I will post a V4 soon as the 24h window closes on V4, with
- history after the patch
- reviewers:
    - Andrew Lunn <andrew@lunn.ch>
    - Vladimir Oltean <olteanv@gmail.com>

Hoping this is the last of the newbie misstakes I've managed to pull
off.
Ramón