On Sun, 4 Jun 2023 19:16:13 -0400
Hugo Villeneuve [off-list ref] wrote:

On Sun, 4 Jun 2023 20:29:58 +0200
Greg KH [off-list ref] wrote:

quoted

On Sun, Jun 04, 2023 at 01:43:44PM -0400, Hugo Villeneuve wrote:

quoted

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.

quoted

Secondly, that (void) craziness is just that.  Rework this to not be an
issue some other way please.

quoted

I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port...

Sure, that sounds best.

Ok, I will do that.

quoted

quoted

quoted

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?

quoted

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?

Hi Greg,
I finally added a "struct device" member inside "struct sc16is7xx_port"
and now I simply pass "struct sc16is7xx_port *s as the sole argument to
sc16is7xx_setup_mctrl_ports().

That should take care of your concern I hope, and I will submit a V8
soon with all these changes.

Hugo.

quoted

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?

quoted

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.