[PATCH v3 0/4] phy: phy-can-transceiver: Ad-hoc cleanups and refactoring

Andy Shevchenko posted 4 patches 1 month, 1 week ago
There is a newer version of this series
drivers/phy/phy-can-transceiver.c | 83 ++++++++++++++++---------------
1 file changed, 44 insertions(+), 39 deletions(-)
[PATCH v3 0/4] phy: phy-can-transceiver: Ad-hoc cleanups and refactoring
Posted by Andy Shevchenko 1 month, 1 week ago
The driver does two things that need to be addressed:
- includes subject to remove gpio.h
- checks for error code from device property APIs when it can be done in
  a robust way

This series addresses the above and adds a couple of additional refactoring.

Changelog v3:
- fixed commit message in patch 1 (Josua)
- dropped stray change (Vinod)
- collected tags (Josua)

v2: 20260317203001.2108568-1-andriy.shevchenko@linux.intel.com

Changelog v2:
- rebased on top of the latest changes in the driver
- Cc'ed to Ulf and Josua due to above
- elaborated dropping of_node parameter (Vladimir)

v1: 20260219202910.2304440-1-andriy.shevchenko@linux.intel.com

Andy Shevchenko (4):
  phy: phy-can-transceiver: use device_get_match_data()
  phy: phy-can-transceiver: Move OF ID table closer to their user
  phy: phy-can-transceiver: Don't check for specific errors when parsing
    properties
  phy: phy-can-transceiver: Drop unused include

 drivers/phy/phy-can-transceiver.c | 83 ++++++++++++++++---------------
 1 file changed, 44 insertions(+), 39 deletions(-)

-- 
2.50.1
Re: [PATCH v3 0/4] phy: phy-can-transceiver: Ad-hoc cleanups and refactoring
Posted by Vinod Koul 1 month ago
On 04-05-26, 08:58, Andy Shevchenko wrote:
> The driver does two things that need to be addressed:
> - includes subject to remove gpio.h
> - checks for error code from device property APIs when it can be done in
>   a robust way
> 
> This series addresses the above and adds a couple of additional refactoring.

Sashiko flagged some issues, some of them not introduced by this, can
you please check this:

https://sashiko.dev/#/patchset/20260504070054.29508-1-andriy.shevchenko%40linux.intel.com

-- 
~Vinod
Re: [PATCH v3 0/4] phy: phy-can-transceiver: Ad-hoc cleanups and refactoring
Posted by Andy Shevchenko 1 month ago
On Sun, May 10, 2026 at 04:21:38PM +0530, Vinod Koul wrote:
> On 04-05-26, 08:58, Andy Shevchenko wrote:
> > The driver does two things that need to be addressed:
> > - includes subject to remove gpio.h
> > - checks for error code from device property APIs when it can be done in
> >   a robust way
> > 
> > This series addresses the above and adds a couple of additional refactoring.
> 
> Sashiko flagged some issues, some of them not introduced by this, can
> you please check this:
> 
> https://sashiko.dev/#/patchset/20260504070054.29508-1-andriy.shevchenko%40linux.intel.com

"Could this result in a null pointer dereference if device_get_match_data()
returns null?"
Yes, it sounds legit but not introduced here.

"In the original code, the warning was suppressed when the property was missing
because err evaluated to -EINVAL. Now, if the property is absent, max_bitrate
is explicitly set to 0 in the else block, which then unconditionally triggers
this warning."
True, but I don't know which is better here, I consider that it's
good to inform user about default being used as a fallback. I can change
this back to the original logic. What do you prefer?

The third one is the repetition of the first one (see above).

TL;DR: The only one legitimated question is about a (new old) warning.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 0/4] phy: phy-can-transceiver: Ad-hoc cleanups and refactoring
Posted by Vinod Koul 1 month ago
On 10-05-26, 15:51, Andy Shevchenko wrote:
> On Sun, May 10, 2026 at 04:21:38PM +0530, Vinod Koul wrote:
> > On 04-05-26, 08:58, Andy Shevchenko wrote:
> > > The driver does two things that need to be addressed:
> > > - includes subject to remove gpio.h
> > > - checks for error code from device property APIs when it can be done in
> > >   a robust way
> > > 
> > > This series addresses the above and adds a couple of additional refactoring.
> > 
> > Sashiko flagged some issues, some of them not introduced by this, can
> > you please check this:
> > 
> > https://sashiko.dev/#/patchset/20260504070054.29508-1-andriy.shevchenko%40linux.intel.com
> 
> "Could this result in a null pointer dereference if device_get_match_data()
> returns null?"
> Yes, it sounds legit but not introduced here.
> 
> "In the original code, the warning was suppressed when the property was missing
> because err evaluated to -EINVAL. Now, if the property is absent, max_bitrate
> is explicitly set to 0 in the else block, which then unconditionally triggers
> this warning."
> True, but I don't know which is better here, I consider that it's
> good to inform user about default being used as a fallback. I can change
> this back to the original logic. What do you prefer?
> 
> The third one is the repetition of the first one (see above).
> 
> TL;DR: The only one legitimated question is about a (new old) warning.

Yeah would be great if we could fix these as well please

-- 
~Vinod
Re: [PATCH v3 0/4] phy: phy-can-transceiver: Ad-hoc cleanups and refactoring
Posted by Andy Shevchenko 1 month ago
On Mon, May 11, 2026 at 09:47:42PM +0530, Vinod Koul wrote:
> On 10-05-26, 15:51, Andy Shevchenko wrote:
> > On Sun, May 10, 2026 at 04:21:38PM +0530, Vinod Koul wrote:
> > > On 04-05-26, 08:58, Andy Shevchenko wrote:
> > > > The driver does two things that need to be addressed:
> > > > - includes subject to remove gpio.h
> > > > - checks for error code from device property APIs when it can be done in
> > > >   a robust way
> > > > 
> > > > This series addresses the above and adds a couple of additional refactoring.
> > > 
> > > Sashiko flagged some issues, some of them not introduced by this, can
> > > you please check this:
> > > 
> > > https://sashiko.dev/#/patchset/20260504070054.29508-1-andriy.shevchenko%40linux.intel.com
> > 
> > "Could this result in a null pointer dereference if device_get_match_data()
> > returns null?"
> > Yes, it sounds legit but not introduced here.
> > 
> > "In the original code, the warning was suppressed when the property was missing
> > because err evaluated to -EINVAL. Now, if the property is absent, max_bitrate
> > is explicitly set to 0 in the else block, which then unconditionally triggers
> > this warning."
> > True, but I don't know which is better here, I consider that it's
> > good to inform user about default being used as a fallback. I can change
> > this back to the original logic. What do you prefer?
> > 
> > The third one is the repetition of the first one (see above).
> > 
> > TL;DR: The only one legitimated question is about a (new old) warning.
> 
> Yeah would be great if we could fix these as well please

I just sent a v4:
20260512130552.272476-1-andriy.shevchenko@linux.intel.com

-- 
With Best Regards,
Andy Shevchenko