On Sun, 4 Jun 2023 19:16:13 -0400 Hugo Villeneuve hugo@hugovil.com wrote:
On Sun, 4 Jun 2023 20:29:58 +0200 Greg KH gregkh@linuxfoundation.org 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.