diff mbox series

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

Message ID 20230725142343.1724130-5-hugo@hugovil.com
State New
Headers
Series serial: sc16is7xx: fix GPIO regression and rs485 improvements |

Commit Message

Hugo Villeneuve July 25, 2023, 2:23 p.m. UTC 
  From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

In preparation for upcoming patch "fix regression with GPIO
configuration". To facilitate review and make code more modular.

Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 40 ++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 16 deletions(-)

Comments

Greg KH July 31, 2023, 3:55 p.m. UTC | #1 
On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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?

> 
> Cc: <stable@vger.kernel.org> # 6.1.x

What commit id does this fix?

> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> ---
>  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.


thanks,

greg k-h
Hugo Villeneuve Aug. 3, 2023, 4:14 p.m. UTC | #2 
On Mon, 31 Jul 2023 17:55:42 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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@vger.kernel.org> # 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@dimonoff.com>
> > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > ---
> >  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.
Greg KH Aug. 4, 2023, 1:14 p.m. UTC | #3 
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@linuxfoundation.org> wrote:
> 
> > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > 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.

> > > Cc: <stable@vger.kernel.org> # 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@dimonoff.com>
> > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > ---
> > >  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 :)

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

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

thanks,

greg k-h
Hugo Villeneuve Aug. 4, 2023, 2:15 p.m. UTC | #4 
On Fri, 4 Aug 2023 15:14:18 +0200
Greg KH <gregkh@linuxfoundation.org> 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@linuxfoundation.org> wrote:
> > 
> > > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > 
> > > > 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@vger.kernel.org> # 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@dimonoff.com>
> > > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > ---
> > > >  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...

Thank you,
Hugo.
Greg KH Aug. 4, 2023, 3:09 p.m. UTC | #5 
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@linuxfoundation.org> 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@linuxfoundation.org> wrote:
> > > 
> > > > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > > 
> > > > > 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@vger.kernel.org> # 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@dimonoff.com>
> > > > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > > ---
> > > > >  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.

thanks,

greg k-h
Hugo Villeneuve Aug. 7, 2023, 2:57 p.m. UTC | #6 
On Fri, 4 Aug 2023 17:09:13 +0200
Greg KH <gregkh@linuxfoundation.org> 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@linuxfoundation.org> 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@linuxfoundation.org> wrote:
> > > > 
> > > > > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > > > 
> > > > > > 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@vger.kernel.org> # 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@dimonoff.com>
> > > > > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > > > ---
> > > > > >  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.
diff mbox series

Patch

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;
 	struct regmap			*regmap;
 	struct clk			*clk;
 #ifdef CONFIG_GPIOLIB
@@ -1349,6 +1350,25 @@  static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 
 	return 0;
 }
+
+static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
+{
+	if (!s->devtype->nr_gpio)
+		return 0;
+
+	s->gpio.owner		 = THIS_MODULE;
+	s->gpio.parent		 = s->dev;
+	s->gpio.label		 = dev_name(s->dev);
+	s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
+	s->gpio.get		 = sc16is7xx_gpio_get;
+	s->gpio.direction_output = sc16is7xx_gpio_direction_output;
+	s->gpio.set		 = sc16is7xx_gpio_set;
+	s->gpio.base		 = -1;
+	s->gpio.ngpio		 = s->devtype->nr_gpio;
+	s->gpio.can_sleep	 = 1;
+
+	return gpiochip_add_data(&s->gpio, s);
+}
 #endif
 
 static const struct serial_rs485 sc16is7xx_rs485_supported = {
@@ -1412,6 +1432,7 @@  static int sc16is7xx_probe(struct device *dev,
 
 	s->regmap = regmap;
 	s->devtype = devtype;
+	s->dev = dev;
 	dev_set_drvdata(dev, s);
 	mutex_init(&s->efr_lock);
 
@@ -1502,22 +1523,9 @@  static int sc16is7xx_probe(struct device *dev,
 	}
 
 #ifdef CONFIG_GPIOLIB
-	if (devtype->nr_gpio) {
-		/* Setup GPIO cotroller */
-		s->gpio.owner		 = THIS_MODULE;
-		s->gpio.parent		 = dev;
-		s->gpio.label		 = dev_name(dev);
-		s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
-		s->gpio.get		 = sc16is7xx_gpio_get;
-		s->gpio.direction_output = sc16is7xx_gpio_direction_output;
-		s->gpio.set		 = sc16is7xx_gpio_set;
-		s->gpio.base		 = -1;
-		s->gpio.ngpio		 = devtype->nr_gpio;
-		s->gpio.can_sleep	 = 1;
-		ret = gpiochip_add_data(&s->gpio, s);
-		if (ret)
-			goto out_ports;
-	}
+	ret = sc16is7xx_setup_gpio_chip(s);
+	if (ret)
+		goto out_ports;
 #endif
 
 	/*