Am 2020-06-05 15:15, schrieb Andy Shevchenko:

On Fri, Jun 05, 2020 at 02:42:53PM +0200, Michael Walle wrote:

quoted

Am 2020-06-05 14:00, schrieb Andy Shevchenko:

quoted

On Fri, Jun 5, 2020 at 12:14 AM Michael Walle [off-list ref] wrote:

quoted

quoted

quoted

+       return devm_regmap_add_irq_chip_np(dev, dev_of_node(dev),
regmap,

It seems regmap needs to be converted to use fwnode.

Mhh, this _np functions was actually part of this series in the
beginning.

Then, please, make them fwnode aware rather than OF centric.

ok

quoted

quoted

quoted

IRQF_ONESHOT, 0,
+                                          irq_chip, &gpio->irq_data);

...

quoted

quoted

quoted

+       dev_id = platform_get_device_id(pdev);
+       if (dev_id)
+               type = dev_id->driver_data;

Oh, no. In new code we don't need this. We have facilities to provide
platform data in a form of fwnode.

Ok I'll look into that.

But I already have a question, so there are of_property_read_xx(), 
which
seems to be the old functions, then there is device_property_read_xx() 
and
fwnode_property_read_xx(). What is the difference between the latter 
two?

It's easy. device_*() requires struct device to be established for 
this, so,
operates only against devices, while the fwnode_*() operates on pure 
data which
might or might not be related to any devices. If you understand OF 
examples
better, consider device node vs. child of such node.

Ahh thanks, got it.

...

quoted

quoted

quoted

+       if (irq_support &&

Why do you need this flag? Can't simple IRQ number be sufficient?

I want to make sure, the is no misconfiguration. Eg. only GPIO
flavors which has irq_support set, have the additional interrupt
registers.

In gpio-dwapb, for example, we simple check two things: a) hardware 
limitation
(if IRQ is assigned to a proper port) and b) if there is any IRQ comes 
from DT,
ACPI, etc.

I can't follow you here. irq_support is like your (a); or the
"pp->idx == 0" in your example.

quoted

quoted

quoted

+           device_property_read_bool(&pdev->dev,
"interrupt-controller")) {
+               irq = platform_get_irq(pdev, 0);
+               if (irq < 0)
+                       return irq;
+
+               ret = sl28cpld_gpio_irq_init(&pdev->dev, gpio, regmap,
+                                            base, irq);
+               if (ret)
+                       return ret;
+
+               config.irq_domain =
regmap_irq_get_domain(gpio->irq_data);
+       }

...

quoted

quoted

quoted

+       { .compatible = "kontron,sl28cpld-gpio",
+         .data = (void *)SL28CPLD_GPIO },
+       { .compatible = "kontron,sl28cpld-gpi",
+         .data = (void *)SL28CPLD_GPI },
+       { .compatible = "kontron,sl28cpld-gpo",
+         .data = (void *)SL28CPLD_GPO },

All above can be twice less LOCs.

They are longer than 80 chars. Or do I miss something?

We have 100 :-)

oh come on, since 6 days *g*

quoted

quoted

quoted

+               .name = KBUILD_MODNAME,

This actually not good idea in long term. File name can change and break
an ABI.

Ahh an explanation, why this is bad. Ok makes sense, although to be 
fair,
.id_table should be used for the driver name matching. I'm not sure if
this is used somewhere else, though.

I saw in my practice chain of renames for a driver. Now, if somebody
somewhere would like to instantiate a platform driver by its name...
Oops, ABI breakage.

And of course using platform data for such device makes less sense.

i just removed the id_table from all drivers anyways.

-michael