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

[Hugo Villeneuve <hugo@xxxxxxxxxxx>]

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"?


> > 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.

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?

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;

?

If not, can you show me how you would like to do it to avoid me trying
to guess?

Thank you,
Hugo.