Messages in this thread | | | Date | Mon, 22 May 2023 14:00:04 -0500 | Subject | Re: [PATCH v6 2/2] can: m_can: Add hrtimer to generate software interrupt | From | Judith Mendez <> |
| |
Hello,
On 5/22/23 1:37 PM, Marc Kleine-Budde wrote: > On 22.05.2023 10:17:38, Judith Mendez wrote: >>>> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c >>>> index 94dc82644113..3e60cebd9d12 100644 >>>> --- a/drivers/net/can/m_can/m_can_platform.c >>>> +++ b/drivers/net/can/m_can/m_can_platform.c >>>> @@ -5,6 +5,7 @@ >>>> // >>>> // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/ >>>> +#include <linux/hrtimer.h> >>>> #include <linux/phy/phy.h> >>>> #include <linux/platform_device.h> >>>> @@ -96,12 +97,40 @@ static int m_can_plat_probe(struct platform_device *pdev) >>>> goto probe_fail; >>>> addr = devm_platform_ioremap_resource_byname(pdev, "m_can"); >>>> - irq = platform_get_irq_byname(pdev, "int0"); >>>> - if (IS_ERR(addr) || irq < 0) { >>>> - ret = -EINVAL; >>>> + if (IS_ERR(addr)) { >>>> + ret = PTR_ERR(addr); >>>> goto probe_fail; >>>> } >>> >>> As we don't use an explicit "poll-interval" anymore, this needs some >>> cleanup. The flow should be (pseudo code, error handling omitted): >>> >>> if (device_property_present("interrupts") { >>> platform_get_irq_byname(); >>> polling = false; >>> } else { >>> hrtimer_init(); >>> polling = true; >>> } >> >> Ok. >> >>> >>>> + irq = platform_get_irq_byname_optional(pdev, "int0"); >>> >>> Remove the "_optional" and.... >> >> On V2, you asked to add the _optional?..... >> >>> irq = platform_get_irq_byname(pdev, "int0"); >> >> use platform_get_irq_byname_optional(), it doesn't print an error >> message. > > ACK - I said that back in v2, when there was "poll-interval". But now we > don't use "poll-interval" anymore, but test if interrupt properties are > present. > > See again pseudo-code I posted in my last mail: > > | if (device_property_present("interrupts") { > | platform_get_irq_byname(); > > If this throws an error, it's fatal, bail out. > > | polling = false; > | } else { > | hrtimer_init(); > | polling = true; > | } >
Ok, will add this then..
>> >>> >>>> + if (irq == -EPROBE_DEFER) { >>>> + ret = -EPROBE_DEFER; >>>> + goto probe_fail; >>>> + } >>>> + >>>> + if (device_property_present(mcan_class->dev, "interrupts") || >>>> + device_property_present(mcan_class->dev, "interrupt-names")) >>>> + mcan_class->polling = false; >>> >>> ...move the platform_get_irq_byname() here >> >> ok, >> >>> >>>> + else >>>> + mcan_class->polling = true; >>>> + >>>> + if (!mcan_class->polling && irq < 0) { >>>> + ret = -ENXIO; >>>> + dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found, polling not activated\n"); >>>> + goto probe_fail; >>>> + } >>> >>> Remove this check. >> >> Should we not go to 'probe fail' if polling is not activated and irq is not >> found? > > If an interrupt property is present in the DT, we use it - if request > IRQ fails, something is broken and we've already bailed out. See above. > If there is no interrupt property we use polling.
Got it, thanks.
>> >>> >>>> + >>>> + if (mcan_class->polling) { >>>> + if (irq > 0) { >>>> + mcan_class->polling = false; >>>> + dev_info(mcan_class->dev, "Polling enabled, using hardware IRQ\n"); >>> >>> Remove this. >> >> Remove the dev_info? > > ACK, this is not possible anymore - we cannot have polling enabled and > HW IRQs configured.
Sounds good, will submit a v7 with these cleanup changes.
regards, Judith
| |