Re: [PATCH v9 04/10] serial: sc16is7xx: refactor GPIO controller registration

[Hugo Villeneuve <hugo@xxxxxxxxxxx>]

On Fri, 4 Aug 2023 17:09:13 +0200
Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:

> On Fri, Aug 04, 2023 at 10:15:54AM -0400, Hugo Villeneuve wrote:
> > On Fri, 4 Aug 2023 15:14:18 +0200
> > Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > 
> > > On Thu, Aug 03, 2023 at 12:14:49PM -0400, Hugo Villeneuve wrote:
> > > > On Mon, 31 Jul 2023 17:55:42 +0200
> > > > Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > 
> > > > > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > > > > From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> > > > > > 
> > > > > > In preparation for upcoming patch "fix regression with GPIO
> > > > > > configuration". To facilitate review and make code more modular.
> > > > > 
> > > > > I would much rather the issue be fixed _before_ the code is refactored,
> > > > > unless it is impossible to fix it without the refactor?
> > > > 
> > > > Hi Greg,
> > > > normally I would agree, but the refactor in this case helps a lot to
> > > > address some issues raised by you and Andy in V7 of this series.
> > > > 
> > > > Maybe I could merge it with the actual patch "fix regression with GPIO
> > > > configuration"?
> > > 
> > > Sure.
> > 
> > Hi Greg,
> > will do.
> > 
> >  
> > > > > > Cc: <stable@xxxxxxxxxxxxxxx> # 6.1.x
> > > > > 
> > > > > What commit id does this fix?
> > > > 
> > > > It doesn't fix anything, but I tought that I needed this tag since
> > > > this patch is a prerequisite for the next patch in the series, which
> > > > would be applied to stable kernels. I will remove this tag (assuming
> > > > the patch stays as it is, depending on your answer to the above
> > > > question).
> > > > 
> > > >  
> > > > > > Signed-off-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> > > > > > Reviewed-by: Lech Perczak <lech.perczak@xxxxxxxxxxxxxxx>
> > > > > > Tested-by: Lech Perczak <lech.perczak@xxxxxxxxxxxxxxx>
> > > > > > ---
> > > > > >  drivers/tty/serial/sc16is7xx.c | 40 ++++++++++++++++++++--------------
> > > > > >  1 file changed, 24 insertions(+), 16 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > > > > index 32d43d00a583..5b0aeef9d534 100644
> > > > > > --- a/drivers/tty/serial/sc16is7xx.c
> > > > > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > > > > @@ -332,6 +332,7 @@ struct sc16is7xx_one {
> > > > > >  
> > > > > >  struct sc16is7xx_port {
> > > > > >  	const struct sc16is7xx_devtype	*devtype;
> > > > > > +	struct device			*dev;
> > > > > 
> > > > > Why is this pointer needed?
> > > > > 
> > > > > Why is it grabbed and yet the reference count is never incremented?  Who
> > > > > owns the reference count and when will it go away?
> > > > > 
> > > > > And what device is this?  The parent?  Current device?  What type of
> > > > > device is it?  And why is it needed?
> > > > > 
> > > > > Using "raw" devices is almost never something a driver should do, they
> > > > > are only passed into functions by the driver core, but then the driver
> > > > > should instantly turn them into the "real" structure.
> > > > 
> > > > We already discussed that a lot in previous versions (v7)... I am
> > > > trying my best to modify the code to address your concerns, but I am
> > > > not fully understanding what you mean about raw devices, and you didn't
> > > > answer some of my previous questions/interrogations in v7 about that.
> > > 
> > > I don't have time to answer all questions, sorry.
> > > 
> > > Please help review submitted patches to reduce my load and allow me to
> > > answer other stuff :)
> > 
> > Ok.
> > 
> > 
> > > > So, in the new function that I
> > > > need to implement, sc16is7xx_setup_gpio_chip(), I absolutely need to use
> > > > a raw device to read a device tree property and to set
> > > > s->gpio.parent:
> > > > 
> > > >     count = device_property_count_u32(dev, ...
> > > >     ...
> > > >     s->gpio.parent = dev;
> > > > 
> > > > Do we agree on that?
> > > 
> > > Yes, but what type of parent is that?
> > 
> > I am confused by your question. I do not understand why the type of
> > parent matters... And what do you call the parent: s, s->gpio or
> > s->gpio.parent?
> > 
> > For me, the way I understand it, the only question that matters is how I
> > can extract the raw device structure pointer from maybe "struct
> > sc16is7xx_port" or some other structure, and then use it in my
> > new function...
> > 
> > I should not have put "s->gpio.parent = dev" in the example, I think it
> > just complexifies things. Lets start over with a more simple example and
> > only:
> > 
> >     count = device_property_count_u32(dev, ...
> > 
> > 
> > > > Then, how do I pass this raw device to the 
> > > > device_property_count_u32() function and to the s->gpio.parent
> > > > assignment?
> > > > 
> > > > Should I modify sc16is7xx_setup_gpio_chip() like so:
> > > > 
> > > >     static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
> > > >     {
> > > > 	struct device *dev = &s->p[0].port.dev;
> > > > 
> > > >         count = device_property_count_u32(dev, ...
> > > >         ...
> > > >         s->gpio.parent = dev;
> > > 
> > > Again, what is the real type of that parent?  It's a port, right, so
> > > pass in the port to this function and then do the "take the struct
> > > device of the port" at that point in time.
> > 
> > With the simplified example, is the following ok:
> > 
> > static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
> > {
> >     struct device *dev = &s->p[0].port.dev;
> > 
> >     count = device_property_count_u32(dev, ...
> >     ...
> > }
> > 
> > If not, please indicate how you would do it with an actual example...
> 
> At this point, after reviewing 500+ patches today, I really have no
> idea, my brain is fried.  Do what you think is right here and submit a
> new series and I'll be glad to review it.

Ok :)

Will do.

Hugo.