Re: [PATCH v8 1/3] serdev: Add method to assert break signal over tty UART port

[Neeraj sanjay kale]

Hi Simon,

> > > > > > diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> > > > > > index
> > > > > > 66f624fc618c..c065ef1c82f1 100644
> > > > > > --- a/include/linux/serdev.h
> > > > > > +++ b/include/linux/serdev.h
> > > > >
> > > > > ...
> > > > >
> > > > > > @@ -255,6 +257,10 @@ static inline int
> > > > > > serdev_device_set_tiocm(struct serdev_device *serdev, int set,  {
> > > > > >       return -ENOTSUPP;
> > > > > >  }
> > > > > > +static inline int serdev_device_break_ctl(struct
> > > > > > +serdev_device *serdev, int break_state) {
> > > > > > +     return -EOPNOTSUPP;
> > > > >
> > > > > Is the use of -EOPNOTSUPP intentional here?
> > > > > I see -ENOTSUPP is used elsewhere in this file.
> > > > I was suggested to use - EOPNOTSUPP instead of - ENOTSUPP by the
> > > > check
> > > patch scripts and by Leon Romanovsky.
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > >
> patc%2F&data=05%7C01%7Cneeraj.sanjaykale%40nxp.com%7Cd6011072c88
> 94
> > > >
> b141a3008db23bb5bc0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C6
> > > >
> 38143059832335354%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
> DAiLCJQ
> > > >
> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata
> =k
> > > >
> gk6w%2Fn2d3rNx%2FXe1rLfN0U%2BeTBJTxztSRUEUW2Noqk%3D&reserved=
> 0
> > > >
> > >
> hwork.kernel.org%2Fproject%2Fbluetooth%2Fpatch%2F20230130180504.202
> > > 944
> > > > 0-2-
> > >
> neeraj.sanjaykale%40nxp.com%2F&data=05%7C01%7Cneeraj.sanjaykale%40
> > > >
> > >
> nxp.com%7Cf2ae2c9ad3c243df2c1a08db232dc0f1%7C686ea1d3bc2b4c6fa92
> > > cd99c5
> > > >
> > >
> c301635%7C0%7C0%7C638142451647332825%7CUnknown%7CTWFpbGZsb3
> > > d8eyJWIjoiM
> > > >
> > >
> C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
> > > %7C%7C
> > > > %7C&sdata=6cF0gipe4kkwYI6txo0vs8vnmF8azCO6gxQ%2F6Tdyd%2Fw%
> 3D
> > > &reserved=
> > > > 0
> > > >
> > > > ENOTSUPP is not a standard error code and should be avoided in new
> > > patches.
> > > > See:
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > >
> lore%2F&data=05%7C01%7Cneeraj.sanjaykale%40nxp.com%7Cd6011072c88
> 94
> > > >
> b141a3008db23bb5bc0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C6
> > > >
> 38143059832335354%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
> DAiLCJQ
> > > >
> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata
> =9
> > > >
> wojVR7aEgzoeajvy%2FMAkSqAYGBkxrJZtyxlvIpeZ%2Bw%3D&reserved=0
> > > > .kernel.org%2Fnetdev%2F20200510182252.GA411829%40lunn.ch%2F&
> data
> > > =05%7C
> > > >
> > >
> 01%7Cneeraj.sanjaykale%40nxp.com%7Cf2ae2c9ad3c243df2c1a08db232dc0f
> > > 1%7C
> > > >
> > >
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638142451647332825%
> > > 7CUnknow
> > > >
> > >
> n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> > > WwiLC
> > > >
> > >
> JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=wFgYY6VnZ8BBn6Wme8%2BYj
> > > aJRy98qyPnUy
> > > > XC8iCFCv5k%3D&reserved=0
> > >
> > > Thanks.
> > >
> > > I agree that EOPNOTSUPP is preferable.
> > > But my question is if we chose to use it in this case, even if it is
> > > inconsistent with similar code in the same file/API.
> > > If so, then I have no objections.
> >
> > No, it was just to satisfy the check patch error and Leon's comment. The
> driver is happy to check if the serdev returned success or not, and simply
> print the error code during driver debug.
> > Do you think this should be reverted to ENOTSUPP to maintain consistency?
> 
> My _opinion_, is that first prize would be converting existing instances of
> ENOTSUPP in this file to EOPNOTSUPP. And then use EOPNOTSUPP going
> forward. And that second prize would be for your patch to use ENOTSUPP.
> Because I think there is a value consistency.
> 
> But I do see why you have done things the way you have.
> And I don't necessarily think it is wrong.

When you put it that way, how can I say no to a first prize! :D
I have replaced all instances of ENOTSUPP with EOPNOTSUPP in these 2 files after making sure none of the functions returning it has a check for ENOTSUPP.
It seems most of the instances do not check for a return value anyways.
Please check v9 patch for the changes.

Thanks
Neeraj