Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve <hugo@xxxxxxxxxxx>]

On Sun, 4 Jun 2023 20:29:58 +0200
Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:

> On Sun, Jun 04, 2023 at 01:43:44PM -0400, Hugo Villeneuve wrote:
> > Here is what I suggest to silence the warning:
> > 
> > 	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
> > 
> > #ifdef CONFIG_GPIOLIB
> > 	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
> > 	if (ret)
> > 		goto out_thread;
> > #else
> > 	(void) mctrl_mask;
> > #endif
> 
> Eeek,  no, please no...
> 
> First off, please don't put #ifdef in .c files if at all possible.

Hi Greg,
Andy also made a similar comment, but couldn't suggest a valid
alternative when I asked him what to do about that.

Just as a sidenote, I didn't add those #ifdef, they were already
present in the driver in multiple places.

What would be your suggestion to get rid of those #ifdef, simply delete
them all?

If you suggest me what to do, I will be happy to submit a
future patch after this series is finalized to clean that aspect.


> Secondly, that (void) craziness is just that.  Rework this to not be an
> issue some other way please.
> 
> > I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port...
> 
> Sure, that sounds best.

Ok, I will do that.


> > > And you have a real port here, no need to pass in a "raw" struct device,
> > > right?
> > 
> > The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the 
> > struct sc16is7xx_port from it:
> > 
> >     struct sc16is7xx_port *s = dev_get_drvdata(dev);
> > 
> > Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this:
> > 
> > static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
> > {
> > 	struct device *dev = &s->p[0].port.dev;
> > 
> > But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device...
> 
> You should never need a "raw" struct device for stuff (if so, something
> is really odd).  Except for error messages, but that's not really a big
> deal, right?

> Don't pass around struct device in a driver, use the real types as you
> know you have it and it saves odd casting around and it just doesn't
> look safe at all to do so.

If you look at the patch, you will see that I need "struct device *dev"
at two places in the sc16is7xx_setup_mctrl_ports() function to read the
device properties:

...
+static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
...
+	count = device_property_count_u32(dev,...
...
+	ret = device_property_read_u32_array(dev,
...

I do not understand why this is odd?


> And if you have that crazy s->p.... stuff in multiple places, the
> perhaps you might want to rethink the structure somehow?  Or at the very
> least, write an inline function to get it when needed.

I am not sure what you mean by that, since again that "crazy" stuff is
already used everywhere in this driver?


> Also, meta comment, you might want to use some \n characters in your
> emails, your lines are really long :)

Strange, I use sylpheed as a mail client, and the option "Wrap lines at
72 characters" is enabled by default, but somehow you must also check
the box "Wrap on input" for it to work, not very intuitive :) Thanks for
pointing that to me.

Hugo.