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

[Hugo Villeneuve <hugo@xxxxxxxxxxx>]

On Sun, 4 Jun 2023 19:16:13 -0400
Hugo Villeneuve <hugo@xxxxxxxxxxx> wrote:

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

Hi Greg,
altough I just send a new V8, I am still curious to hear your point of
view about those #ifdef...

Hugo.


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