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