Re: [PATCH v3 2/3] mfd: lubbock_io: add lubbock_io board

Lee Jones [off-list ref] writes:

On Fri, 16 Jan 2015, Robert Jarzmik wrote:

quoted

As a fix, move the gpio0 chained handler setup to a place where we have
the guarantee that pxa_gpio_probe() was called before, so that lubbock
handler becomes the true IRQ chained handler of GPIO0, demuxing the
lubbock IO board interrupts.

How is this guaranteed?

In the following chunk :
	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
			       irqflags, dev_name(&pdev->dev), cot);
	if (ret == -ENOSYS)
		return -EPROBE_DEFER;

See __setup_irq(), and see what happens if the irq chip is not set (which
happens on pxa platform when the gpio driver is not registered).

quoted

+ * Lubbock motherboard driver, supporting lubbock (aka. pxa25x) soc board.

Please use uppercase characters i.e. Lubbock, PXA25X, SoC, etc.

OK, your tree, your rules.

Superfluous '\n'.

Yep.

Can this be built as a module?

Not in its current form.

If so, why isn't it a tristate?

See above.

Now the question is : should it be buildable as a module ? I was thinking it
shouldn't because without this driver lubbock becomes a bit useless (most of its
peripherals are on the motherboard).

quoted

+struct lubbock {
+	void __iomem	*base;

Random spacing.

Right.

quoted

+	pending = readl(cot->base + COT_IRQ_SET_CLR) & cot->irq_mask;
+	for_each_set_bit(bit, &pending, LUBBOCK_NB_IRQ)
+		generic_handle_irq(irq_find_mapping(cot->irqdomain, bit));

I'd like to see a '\n' here.

OK.

Again, I'd prefer some separation between code and the return.

(same in all cases below).

OK.

quoted

+	cot = devm_kzalloc(&pdev->dev, sizeof(*cot), GFP_KERNEL);
+	if (!cot)
+		return -ENOMEM;

'\n' here.

OK.

quoted

+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);

platform_get_irq()?

No. I need the flags.

quoted

+	if (res) {
+		cot->irq = (unsigned int)res->start;
+		irqflags = res->flags;
+	}
+	if (!cot->irq)
+		return -ENODEV;
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 1);

platform_get_irq()?

Yes, certainly.

quoted

+	writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN);
+	writel(0, cot->base + COT_IRQ_SET_CLR);

'\n'

OK.

quoted

+	ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
+			       irqflags, dev_name(&pdev->dev), cot);
+	if (ret == -ENOSYS)
+		return -EPROBE_DEFER;

I haven't seen anyone do this after devm_request_irq() before.
Why is it required here?

Explained above.

quoted

+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
+			ret);

I'm not keen on this type of formatting.  Besides the system will
print out the returned error on failure.

Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the
driver with -ENODEV, how will I know it will come from this request_irq() or
another part of the code ... Well I can remove it if you want, but I think it's
an error.

quoted

+	cot->irqdomain =
+		irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ,
+				      &lubbock_irq_domain_ops, cot);

As a personal preference, I would prefer to see:

	cot->irqdomain = irq_domain_add_linear(pdev->dev.of_node,
					       LUBBOCK_NB_IRQ,
					       &lubbock_irq_domain_ops, cot);

Your tree, your rules. OK.

quoted

+	if (!cot->irqdomain)
+		return -ENODEV;
+
+	ret = 0;

'ret' will be zero here, or we would have returned already.

Good catch. OK.

quoted

+	if (base_irq)
+		ret = irq_create_strict_mappings(cot->irqdomain, base_irq, 0,
+						 LUBBOCK_NB_IRQ);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't create the irq mapping %d..%d\n",
+			base_irq, base_irq + LUBBOCK_NB_IRQ);
+		return ret;
+	}

Is this solely the check from irq_create_strict_mappings(), therefore
it should be inside the previous if () { ... }.

As you wish.

quoted

+	dev_info(&pdev->dev, "base=%p, irq=%d, base_irq=%d\n",
+		 cot->base, cot->irq, base_irq);

Please remove this line.

OK.

quoted

+MODULE_DESCRIPTION("Lubbock driver");

"Lubbock MFD Driver"?

Yes.

quoted

+MODULE_AUTHOR("Robert Jarzmik");

Email.

Sure

Thanks for the review.

-- 
Robert

On Mon, 19 Jan 2015, Robert Jarzmik wrote:

Lee Jones [off-list ref] writes:

quoted

On Fri, 16 Jan 2015, Robert Jarzmik wrote:

[...]

quoted

quoted

+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);

platform_get_irq()?

No. I need the flags.

Where are they used?

[...]

quoted

quoted

+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
+			ret);

I'm not keen on this type of formatting.  Besides the system will
print out the returned error on failure.

Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the
driver with -ENODEV, how will I know it will come from this request_irq() or
another part of the code ... Well I can remove it if you want, but I think it's
an error.

I'm not asking you to remove the entire message, just the junk at the
end.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Tue, Jan 20, 2015 at 10:29:19AM +0000, Lee Jones wrote:

On Mon, 19 Jan 2015, Robert Jarzmik wrote:

quoted

quoted

quoted

+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n",
+			ret);

I'm not keen on this type of formatting.  Besides the system will
print out the returned error on failure.

Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the
driver with -ENODEV, how will I know it will come from this request_irq() or
another part of the code ... Well I can remove it if you want, but I think it's
an error.

I'm not asking you to remove the entire message, just the junk at the
end.

No.  Leave it.  If request_irq() returns -ENODEV or -ENXIO, you'll
just get the "Couldn't request main irq" message but without the
error code printed.

What I'd suggest (and always have done) is:

	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
		irq, ret);

but I guess printing the IRQ number no longer makes sense with todays
dynamic mapping of logical IRQ numbers, as it is no longer meaningful.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Russell King - ARM Linux [off-list ref] writes:

What I'd suggest (and always have done) is:

	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
		irq, ret);

I like it, it's even more compact, I'll use it for next patch version.

but I guess printing the IRQ number no longer makes sense with todays
dynamic mapping of logical IRQ numbers, as it is no longer meaningful.

Yes ... we're not yet there with pxa gpio interrupts, maybe it will come
eventually one day.

For Lee:

quoted

quoted

platform_get_irq()?

No. I need the flags.

Where are they used?

A couple of lines below, using local "irqflags" variable :
       ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
                              irqflags, dev_name(&pdev->dev), cot);

Cheers.

-- 
Robert

On Wed, 21 Jan 2015, Robert Jarzmik wrote:

quoted

quoted

quoted

platform_get_irq()?

No. I need the flags.

Where are they used?

A couple of lines below, using local "irqflags" variable :
       ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
                              irqflags, dev_name(&pdev->dev), cot);

No, I mean where are they _used_?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Wed, Jan 21, 2015 at 08:16:45AM +0000, Lee Jones wrote:

On Wed, 21 Jan 2015, Robert Jarzmik wrote:

quoted

quoted

quoted

quoted

platform_get_irq()?

No. I need the flags.

Where are they used?

A couple of lines below, using local "irqflags" variable :
       ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler,
                              irqflags, dev_name(&pdev->dev), cot);

No, I mean where are they _used_?

They're used in request_irq, and request_irq uses it to set the interrupt
type (edge or level) as specified in the IRQ resource.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

On Wed, Jan 21, 2015 at 08:46:29AM +0100, Robert Jarzmik wrote:

Russell King - ARM Linux [off-list ref] writes:

quoted

What I'd suggest (and always have done) is:

	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
		irq, ret);

I like it, it's even more compact, I'll use it for next patch version.

BTW, this is an example why I have the policy of always ensuring that
the kernel messages print sufficient diagnostics.  Right now, I have
a problem - since I rebooted my firewall a few nights ago, I now get
on one of my machines:

  rt6_redirect: source isn't a valid nexthop for redirect target

and it spews that for a few minutes every 26 hours or so.  No further
information, and it leaves you wondering "well, what was the invalid
next hop?  What was the source?"

Pretty much the only way to try and find out is to leave a tcpdump or
wireshark running for 24 hours to try and get a dump - which is not
that easy if you don't have lots of disk space.  So, right now, I have
no way to diagnose the above.

If it printed that information, then I'd be able to see what the
addresses were, and I'd probably be able to come up with a tcpdump
filter which didn't involve logging all IPv6 traffic.

Kernel messages need to be smart.  If not, they might as well just be
"The kernel encountered a problem. Abort, Retry or Fail?"

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

(adding netdev)

On Wed, 2015-01-21 at 09:44 +0000, Russell King - ARM Linux wrote:

On Wed, Jan 21, 2015 at 08:46:29AM +0100, Robert Jarzmik wrote:

quoted

Russell King - ARM Linux [off-list ref] writes:

quoted

What I'd suggest (and always have done) is:

	dev_err(&pdev->dev, "couldn't request main irq%d: %d\n",
		irq, ret);

I like it, it's even more compact, I'll use it for next patch version.

BTW, this is an example why I have the policy of always ensuring that
the kernel messages print sufficient diagnostics.  Right now, I have
a problem - since I rebooted my firewall a few nights ago, I now get
on one of my machines:

  rt6_redirect: source isn't a valid nexthop for redirect target

and it spews that for a few minutes every 26 hours or so.  No further
information, and it leaves you wondering "well, what was the invalid
next hop?  What was the source?"

Pretty much the only way to try and find out is to leave a tcpdump or
wireshark running for 24 hours to try and get a dump - which is not
that easy if you don't have lots of disk space.  So, right now, I have
no way to diagnose the above.

If it printed that information, then I'd be able to see what the
addresses were, and I'd probably be able to come up with a tcpdump
filter which didn't involve logging all IPv6 traffic.

Kernel messages need to be smart.  If not, they might as well just be
"The kernel encountered a problem. Abort, Retry or Fail?"

On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote:

(adding netdev)

I wasn't actually reporting that as an issue; I was using it as an
example.  It's from a very old kernel (2.6.27.21) which I run on one
of my old x86 machines.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

On Wed, 2015-01-21 at 16:11 +0000, Russell King - ARM Linux wrote:

On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote:

quoted

(adding netdev)

I wasn't actually reporting that as an issue; I was using it as an
example.  It's from a very old kernel (2.6.27.21) which I run on one
of my old x86 machines.

It's still the same code.
If the message can be improved, why not do it?

On Wed, Jan 21, 2015 at 08:40:44AM -0800, Joe Perches wrote:

On Wed, 2015-01-21 at 16:11 +0000, Russell King - ARM Linux wrote:

quoted

On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote:

quoted

(adding netdev)

I wasn't actually reporting that as an issue; I was using it as an
example.  It's from a very old kernel (2.6.27.21) which I run on one
of my old x86 machines.

It's still the same code.
If the message can be improved, why not do it?

I assume you're taking the responsibility to test anything that comes
out of this then?

I'm not; I tried updating the kernel on the machine to 2.6.32 many
years ago and that was a no-go because of userspace (udev)
incompatibilities.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.