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

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

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

quoted

quoted

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

...

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.

...

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.

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

+       { .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 :-)

...

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.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel