* [PATCH v1] xsysace: use platform_get_resource() and platform_get_irq_optional()
@ 2020-10-27 17:11 Andy Shevchenko
2020-10-29 7:18 ` Michal Simek
0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-10-27 17:11 UTC (permalink / raw)
To: Michal Simek, linux-arm-kernel, Jens Axboe, linux-block; +Cc: Andy Shevchenko
Use platform_get_resource() to fetch the memory resource and
platform_get_irq_optional() to get optional IRQ instead of
open-coded variants.
IRQ is not supposed to be changed at runtime, so there is
no functional change in ace_fsm_yieldirq().
On the other hand we now take first resources instead of last ones
to proceed. I can't imagine how broken should be firmware to have
a garbage in the first resource slots. But if it the case, it needs
to be documented.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/block/xsysace.c | 49 ++++++++++++++++++++++-------------------
1 file changed, 26 insertions(+), 23 deletions(-)
diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
index 8d581c7536fb..eb8ef65778c3 100644
--- a/drivers/block/xsysace.c
+++ b/drivers/block/xsysace.c
@@ -443,22 +443,27 @@ static void ace_fix_driveid(u16 *id)
#define ACE_FSM_NUM_STATES 11
/* Set flag to exit FSM loop and reschedule tasklet */
-static inline void ace_fsm_yield(struct ace_device *ace)
+static inline void ace_fsm_yieldpoll(struct ace_device *ace)
{
- dev_dbg(ace->dev, "ace_fsm_yield()\n");
tasklet_schedule(&ace->fsm_tasklet);
ace->fsm_continue_flag = 0;
}
+static inline void ace_fsm_yield(struct ace_device *ace)
+{
+ dev_dbg(ace->dev, "%s()\n", __func__);
+ ace_fsm_yieldpoll(ace);
+}
+
/* Set flag to exit FSM loop and wait for IRQ to reschedule tasklet */
static inline void ace_fsm_yieldirq(struct ace_device *ace)
{
dev_dbg(ace->dev, "ace_fsm_yieldirq()\n");
- if (!ace->irq)
- /* No IRQ assigned, so need to poll */
- tasklet_schedule(&ace->fsm_tasklet);
- ace->fsm_continue_flag = 0;
+ if (ace->irq > 0)
+ ace->fsm_continue_flag = 0;
+ else
+ ace_fsm_yieldpoll(ace);
}
static bool ace_has_next_request(struct request_queue *q)
@@ -1053,12 +1058,12 @@ static int ace_setup(struct ace_device *ace)
ACE_CTRL_DATABUFRDYIRQ | ACE_CTRL_ERRORIRQ);
/* Now we can hook up the irq handler */
- if (ace->irq) {
+ if (ace->irq > 0) {
rc = request_irq(ace->irq, ace_interrupt, 0, "systemace", ace);
if (rc) {
/* Failure - fall back to polled mode */
dev_err(ace->dev, "request_irq failed\n");
- ace->irq = 0;
+ ace->irq = rc;
}
}
@@ -1110,7 +1115,7 @@ static void ace_teardown(struct ace_device *ace)
tasklet_kill(&ace->fsm_tasklet);
- if (ace->irq)
+ if (ace->irq > 0)
free_irq(ace->irq, ace);
iounmap(ace->baseaddr);
@@ -1123,11 +1128,6 @@ static int ace_alloc(struct device *dev, int id, resource_size_t physaddr,
int rc;
dev_dbg(dev, "ace_alloc(%p)\n", dev);
- if (!physaddr) {
- rc = -ENODEV;
- goto err_noreg;
- }
-
/* Allocate and initialize the ace device structure */
ace = kzalloc(sizeof(struct ace_device), GFP_KERNEL);
if (!ace) {
@@ -1153,7 +1153,6 @@ static int ace_alloc(struct device *dev, int id, resource_size_t physaddr,
dev_set_drvdata(dev, NULL);
kfree(ace);
err_alloc:
-err_noreg:
dev_err(dev, "could not initialize device, err=%i\n", rc);
return rc;
}
@@ -1176,10 +1175,11 @@ static void ace_free(struct device *dev)
static int ace_probe(struct platform_device *dev)
{
- resource_size_t physaddr = 0;
int bus_width = ACE_BUS_WIDTH_16; /* FIXME: should not be hard coded */
+ resource_size_t physaddr;
+ struct resource *res;
u32 id = dev->id;
- int irq = 0;
+ int irq;
int i;
dev_dbg(&dev->dev, "ace_probe(%p)\n", dev);
@@ -1190,12 +1190,15 @@ static int ace_probe(struct platform_device *dev)
if (of_find_property(dev->dev.of_node, "8-bit", NULL))
bus_width = ACE_BUS_WIDTH_8;
- for (i = 0; i < dev->num_resources; i++) {
- if (dev->resource[i].flags & IORESOURCE_MEM)
- physaddr = dev->resource[i].start;
- if (dev->resource[i].flags & IORESOURCE_IRQ)
- irq = dev->resource[i].start;
- }
+ res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
+
+ physaddr = res->start;
+ if (!physaddr)
+ return -ENODEV;
+
+ irq = platform_get_irq_optional(dev, 0);
/* Call the bus-independent setup code */
return ace_alloc(&dev->dev, id, physaddr, irq, bus_width);
--
2.28.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1] xsysace: use platform_get_resource() and platform_get_irq_optional()
2020-10-27 17:11 [PATCH v1] xsysace: use platform_get_resource() and platform_get_irq_optional() Andy Shevchenko
@ 2020-10-29 7:18 ` Michal Simek
2020-10-29 14:22 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Michal Simek @ 2020-10-29 7:18 UTC (permalink / raw)
To: Andy Shevchenko, Michal Simek, linux-arm-kernel, Jens Axboe, linux-block
Hi Andy,
On 27. 10. 20 18:11, Andy Shevchenko wrote:
> Use platform_get_resource() to fetch the memory resource and
> platform_get_irq_optional() to get optional IRQ instead of
> open-coded variants.
>
> IRQ is not supposed to be changed at runtime, so there is
> no functional change in ace_fsm_yieldirq().
>
> On the other hand we now take first resources instead of last ones
> to proceed. I can't imagine how broken should be firmware to have
> a garbage in the first resource slots. But if it the case, it needs
> to be documented.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/block/xsysace.c | 49 ++++++++++++++++++++++-------------------
> 1 file changed, 26 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
> index 8d581c7536fb..eb8ef65778c3 100644
> --- a/drivers/block/xsysace.c
> +++ b/drivers/block/xsysace.c
> @@ -443,22 +443,27 @@ static void ace_fix_driveid(u16 *id)
> #define ACE_FSM_NUM_STATES 11
>
> /* Set flag to exit FSM loop and reschedule tasklet */
> -static inline void ace_fsm_yield(struct ace_device *ace)
> +static inline void ace_fsm_yieldpoll(struct ace_device *ace)
> {
> - dev_dbg(ace->dev, "ace_fsm_yield()\n");
> tasklet_schedule(&ace->fsm_tasklet);
> ace->fsm_continue_flag = 0;
> }
>
> +static inline void ace_fsm_yield(struct ace_device *ace)
> +{
> + dev_dbg(ace->dev, "%s()\n", __func__);
> + ace_fsm_yieldpoll(ace);
> +}
> +
> /* Set flag to exit FSM loop and wait for IRQ to reschedule tasklet */
> static inline void ace_fsm_yieldirq(struct ace_device *ace)
> {
> dev_dbg(ace->dev, "ace_fsm_yieldirq()\n");
>
> - if (!ace->irq)
> - /* No IRQ assigned, so need to poll */
> - tasklet_schedule(&ace->fsm_tasklet);
> - ace->fsm_continue_flag = 0;
> + if (ace->irq > 0)
> + ace->fsm_continue_flag = 0;
> + else
> + ace_fsm_yieldpoll(ace);
> }
>
> static bool ace_has_next_request(struct request_queue *q)
> @@ -1053,12 +1058,12 @@ static int ace_setup(struct ace_device *ace)
> ACE_CTRL_DATABUFRDYIRQ | ACE_CTRL_ERRORIRQ);
>
> /* Now we can hook up the irq handler */
> - if (ace->irq) {
> + if (ace->irq > 0) {
> rc = request_irq(ace->irq, ace_interrupt, 0, "systemace", ace);
> if (rc) {
> /* Failure - fall back to polled mode */
> dev_err(ace->dev, "request_irq failed\n");
> - ace->irq = 0;
> + ace->irq = rc;
> }
> }
>
> @@ -1110,7 +1115,7 @@ static void ace_teardown(struct ace_device *ace)
>
> tasklet_kill(&ace->fsm_tasklet);
>
> - if (ace->irq)
> + if (ace->irq > 0)
> free_irq(ace->irq, ace);
>
> iounmap(ace->baseaddr);
> @@ -1123,11 +1128,6 @@ static int ace_alloc(struct device *dev, int id, resource_size_t physaddr,
> int rc;
> dev_dbg(dev, "ace_alloc(%p)\n", dev);
>
> - if (!physaddr) {
> - rc = -ENODEV;
> - goto err_noreg;
> - }
> -
> /* Allocate and initialize the ace device structure */
> ace = kzalloc(sizeof(struct ace_device), GFP_KERNEL);
> if (!ace) {
> @@ -1153,7 +1153,6 @@ static int ace_alloc(struct device *dev, int id, resource_size_t physaddr,
> dev_set_drvdata(dev, NULL);
> kfree(ace);
> err_alloc:
> -err_noreg:
> dev_err(dev, "could not initialize device, err=%i\n", rc);
> return rc;
> }
> @@ -1176,10 +1175,11 @@ static void ace_free(struct device *dev)
>
> static int ace_probe(struct platform_device *dev)
> {
> - resource_size_t physaddr = 0;
> int bus_width = ACE_BUS_WIDTH_16; /* FIXME: should not be hard coded */
> + resource_size_t physaddr;
> + struct resource *res;
> u32 id = dev->id;
> - int irq = 0;
> + int irq;
> int i;
>
> dev_dbg(&dev->dev, "ace_probe(%p)\n", dev);
> @@ -1190,12 +1190,15 @@ static int ace_probe(struct platform_device *dev)
> if (of_find_property(dev->dev.of_node, "8-bit", NULL))
> bus_width = ACE_BUS_WIDTH_8;
>
> - for (i = 0; i < dev->num_resources; i++) {
> - if (dev->resource[i].flags & IORESOURCE_MEM)
> - physaddr = dev->resource[i].start;
> - if (dev->resource[i].flags & IORESOURCE_IRQ)
> - irq = dev->resource[i].start;
> - }
> + res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> +
> + physaddr = res->start;
> + if (!physaddr)
> + return -ENODEV;
> +
> + irq = platform_get_irq_optional(dev, 0);
>
> /* Call the bus-independent setup code */
> return ace_alloc(&dev->dev, id, physaddr, irq, bus_width);
>
This driver is quite old and obsolete. I am fine with whatever patch and
I am also fine with marking driver as BROKEN or remove it because I am
not aware about any user.
Acked-by: Michal Simek <michal.simek@xilinx.com>
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] xsysace: use platform_get_resource() and platform_get_irq_optional()
2020-10-29 7:18 ` Michal Simek
@ 2020-10-29 14:22 ` Jens Axboe
2020-10-30 7:08 ` Michal Simek
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2020-10-29 14:22 UTC (permalink / raw)
To: Michal Simek, Andy Shevchenko, linux-arm-kernel, linux-block
On 10/29/20 1:18 AM, Michal Simek wrote:
> Hi Andy,
>
> On 27. 10. 20 18:11, Andy Shevchenko wrote:
>> Use platform_get_resource() to fetch the memory resource and
>> platform_get_irq_optional() to get optional IRQ instead of
>> open-coded variants.
>>
>> IRQ is not supposed to be changed at runtime, so there is
>> no functional change in ace_fsm_yieldirq().
>>
>> On the other hand we now take first resources instead of last ones
>> to proceed. I can't imagine how broken should be firmware to have
>> a garbage in the first resource slots. But if it the case, it needs
>> to be documented.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>> drivers/block/xsysace.c | 49 ++++++++++++++++++++++-------------------
>> 1 file changed, 26 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
>> index 8d581c7536fb..eb8ef65778c3 100644
>> --- a/drivers/block/xsysace.c
>> +++ b/drivers/block/xsysace.c
>> @@ -443,22 +443,27 @@ static void ace_fix_driveid(u16 *id)
>> #define ACE_FSM_NUM_STATES 11
>>
>> /* Set flag to exit FSM loop and reschedule tasklet */
>> -static inline void ace_fsm_yield(struct ace_device *ace)
>> +static inline void ace_fsm_yieldpoll(struct ace_device *ace)
>> {
>> - dev_dbg(ace->dev, "ace_fsm_yield()\n");
>> tasklet_schedule(&ace->fsm_tasklet);
>> ace->fsm_continue_flag = 0;
>> }
>>
>> +static inline void ace_fsm_yield(struct ace_device *ace)
>> +{
>> + dev_dbg(ace->dev, "%s()\n", __func__);
>> + ace_fsm_yieldpoll(ace);
>> +}
>> +
>> /* Set flag to exit FSM loop and wait for IRQ to reschedule tasklet */
>> static inline void ace_fsm_yieldirq(struct ace_device *ace)
>> {
>> dev_dbg(ace->dev, "ace_fsm_yieldirq()\n");
>>
>> - if (!ace->irq)
>> - /* No IRQ assigned, so need to poll */
>> - tasklet_schedule(&ace->fsm_tasklet);
>> - ace->fsm_continue_flag = 0;
>> + if (ace->irq > 0)
>> + ace->fsm_continue_flag = 0;
>> + else
>> + ace_fsm_yieldpoll(ace);
>> }
>>
>> static bool ace_has_next_request(struct request_queue *q)
>> @@ -1053,12 +1058,12 @@ static int ace_setup(struct ace_device *ace)
>> ACE_CTRL_DATABUFRDYIRQ | ACE_CTRL_ERRORIRQ);
>>
>> /* Now we can hook up the irq handler */
>> - if (ace->irq) {
>> + if (ace->irq > 0) {
>> rc = request_irq(ace->irq, ace_interrupt, 0, "systemace", ace);
>> if (rc) {
>> /* Failure - fall back to polled mode */
>> dev_err(ace->dev, "request_irq failed\n");
>> - ace->irq = 0;
>> + ace->irq = rc;
>> }
>> }
>>
>> @@ -1110,7 +1115,7 @@ static void ace_teardown(struct ace_device *ace)
>>
>> tasklet_kill(&ace->fsm_tasklet);
>>
>> - if (ace->irq)
>> + if (ace->irq > 0)
>> free_irq(ace->irq, ace);
>>
>> iounmap(ace->baseaddr);
>> @@ -1123,11 +1128,6 @@ static int ace_alloc(struct device *dev, int id, resource_size_t physaddr,
>> int rc;
>> dev_dbg(dev, "ace_alloc(%p)\n", dev);
>>
>> - if (!physaddr) {
>> - rc = -ENODEV;
>> - goto err_noreg;
>> - }
>> -
>> /* Allocate and initialize the ace device structure */
>> ace = kzalloc(sizeof(struct ace_device), GFP_KERNEL);
>> if (!ace) {
>> @@ -1153,7 +1153,6 @@ static int ace_alloc(struct device *dev, int id, resource_size_t physaddr,
>> dev_set_drvdata(dev, NULL);
>> kfree(ace);
>> err_alloc:
>> -err_noreg:
>> dev_err(dev, "could not initialize device, err=%i\n", rc);
>> return rc;
>> }
>> @@ -1176,10 +1175,11 @@ static void ace_free(struct device *dev)
>>
>> static int ace_probe(struct platform_device *dev)
>> {
>> - resource_size_t physaddr = 0;
>> int bus_width = ACE_BUS_WIDTH_16; /* FIXME: should not be hard coded */
>> + resource_size_t physaddr;
>> + struct resource *res;
>> u32 id = dev->id;
>> - int irq = 0;
>> + int irq;
>> int i;
>>
>> dev_dbg(&dev->dev, "ace_probe(%p)\n", dev);
>> @@ -1190,12 +1190,15 @@ static int ace_probe(struct platform_device *dev)
>> if (of_find_property(dev->dev.of_node, "8-bit", NULL))
>> bus_width = ACE_BUS_WIDTH_8;
>>
>> - for (i = 0; i < dev->num_resources; i++) {
>> - if (dev->resource[i].flags & IORESOURCE_MEM)
>> - physaddr = dev->resource[i].start;
>> - if (dev->resource[i].flags & IORESOURCE_IRQ)
>> - irq = dev->resource[i].start;
>> - }
>> + res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return -EINVAL;
>> +
>> + physaddr = res->start;
>> + if (!physaddr)
>> + return -ENODEV;
>> +
>> + irq = platform_get_irq_optional(dev, 0);
>>
>> /* Call the bus-independent setup code */
>> return ace_alloc(&dev->dev, id, physaddr, irq, bus_width);
>>
>
> This driver is quite old and obsolete. I am fine with whatever patch and
> I am also fine with marking driver as BROKEN or remove it because I am
> not aware about any user.
>
> Acked-by: Michal Simek <michal.simek@xilinx.com>
How about I queue this one up for 5.10, and then we can queue a removal
patch for 5.11? Or at least schedule it for removal.
--
Jens Axboe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] xsysace: use platform_get_resource() and platform_get_irq_optional()
2020-10-29 14:22 ` Jens Axboe
@ 2020-10-30 7:08 ` Michal Simek
2020-11-09 11:00 ` Michal Simek
0 siblings, 1 reply; 6+ messages in thread
From: Michal Simek @ 2020-10-30 7:08 UTC (permalink / raw)
To: Jens Axboe, Michal Simek, Andy Shevchenko, linux-arm-kernel, linux-block
On 29. 10. 20 15:22, Jens Axboe wrote:
> On 10/29/20 1:18 AM, Michal Simek wrote:
>> Hi Andy,
>>
>> On 27. 10. 20 18:11, Andy Shevchenko wrote:
>>> Use platform_get_resource() to fetch the memory resource and
>>> platform_get_irq_optional() to get optional IRQ instead of
>>> open-coded variants.
>>>
>>> IRQ is not supposed to be changed at runtime, so there is
>>> no functional change in ace_fsm_yieldirq().
>>>
>>> On the other hand we now take first resources instead of last ones
>>> to proceed. I can't imagine how broken should be firmware to have
>>> a garbage in the first resource slots. But if it the case, it needs
>>> to be documented.
>>>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> ---
>>> drivers/block/xsysace.c | 49 ++++++++++++++++++++++-------------------
>>> 1 file changed, 26 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
>>> index 8d581c7536fb..eb8ef65778c3 100644
>>> --- a/drivers/block/xsysace.c
>>> +++ b/drivers/block/xsysace.c
>>> @@ -443,22 +443,27 @@ static void ace_fix_driveid(u16 *id)
>>> #define ACE_FSM_NUM_STATES 11
>>>
>>> /* Set flag to exit FSM loop and reschedule tasklet */
>>> -static inline void ace_fsm_yield(struct ace_device *ace)
>>> +static inline void ace_fsm_yieldpoll(struct ace_device *ace)
>>> {
>>> - dev_dbg(ace->dev, "ace_fsm_yield()\n");
>>> tasklet_schedule(&ace->fsm_tasklet);
>>> ace->fsm_continue_flag = 0;
>>> }
>>>
>>> +static inline void ace_fsm_yield(struct ace_device *ace)
>>> +{
>>> + dev_dbg(ace->dev, "%s()\n", __func__);
>>> + ace_fsm_yieldpoll(ace);
>>> +}
>>> +
>>> /* Set flag to exit FSM loop and wait for IRQ to reschedule tasklet */
>>> static inline void ace_fsm_yieldirq(struct ace_device *ace)
>>> {
>>> dev_dbg(ace->dev, "ace_fsm_yieldirq()\n");
>>>
>>> - if (!ace->irq)
>>> - /* No IRQ assigned, so need to poll */
>>> - tasklet_schedule(&ace->fsm_tasklet);
>>> - ace->fsm_continue_flag = 0;
>>> + if (ace->irq > 0)
>>> + ace->fsm_continue_flag = 0;
>>> + else
>>> + ace_fsm_yieldpoll(ace);
>>> }
>>>
>>> static bool ace_has_next_request(struct request_queue *q)
>>> @@ -1053,12 +1058,12 @@ static int ace_setup(struct ace_device *ace)
>>> ACE_CTRL_DATABUFRDYIRQ | ACE_CTRL_ERRORIRQ);
>>>
>>> /* Now we can hook up the irq handler */
>>> - if (ace->irq) {
>>> + if (ace->irq > 0) {
>>> rc = request_irq(ace->irq, ace_interrupt, 0, "systemace", ace);
>>> if (rc) {
>>> /* Failure - fall back to polled mode */
>>> dev_err(ace->dev, "request_irq failed\n");
>>> - ace->irq = 0;
>>> + ace->irq = rc;
>>> }
>>> }
>>>
>>> @@ -1110,7 +1115,7 @@ static void ace_teardown(struct ace_device *ace)
>>>
>>> tasklet_kill(&ace->fsm_tasklet);
>>>
>>> - if (ace->irq)
>>> + if (ace->irq > 0)
>>> free_irq(ace->irq, ace);
>>>
>>> iounmap(ace->baseaddr);
>>> @@ -1123,11 +1128,6 @@ static int ace_alloc(struct device *dev, int id, resource_size_t physaddr,
>>> int rc;
>>> dev_dbg(dev, "ace_alloc(%p)\n", dev);
>>>
>>> - if (!physaddr) {
>>> - rc = -ENODEV;
>>> - goto err_noreg;
>>> - }
>>> -
>>> /* Allocate and initialize the ace device structure */
>>> ace = kzalloc(sizeof(struct ace_device), GFP_KERNEL);
>>> if (!ace) {
>>> @@ -1153,7 +1153,6 @@ static int ace_alloc(struct device *dev, int id, resource_size_t physaddr,
>>> dev_set_drvdata(dev, NULL);
>>> kfree(ace);
>>> err_alloc:
>>> -err_noreg:
>>> dev_err(dev, "could not initialize device, err=%i\n", rc);
>>> return rc;
>>> }
>>> @@ -1176,10 +1175,11 @@ static void ace_free(struct device *dev)
>>>
>>> static int ace_probe(struct platform_device *dev)
>>> {
>>> - resource_size_t physaddr = 0;
>>> int bus_width = ACE_BUS_WIDTH_16; /* FIXME: should not be hard coded */
>>> + resource_size_t physaddr;
>>> + struct resource *res;
>>> u32 id = dev->id;
>>> - int irq = 0;
>>> + int irq;
>>> int i;
>>>
>>> dev_dbg(&dev->dev, "ace_probe(%p)\n", dev);
>>> @@ -1190,12 +1190,15 @@ static int ace_probe(struct platform_device *dev)
>>> if (of_find_property(dev->dev.of_node, "8-bit", NULL))
>>> bus_width = ACE_BUS_WIDTH_8;
>>>
>>> - for (i = 0; i < dev->num_resources; i++) {
>>> - if (dev->resource[i].flags & IORESOURCE_MEM)
>>> - physaddr = dev->resource[i].start;
>>> - if (dev->resource[i].flags & IORESOURCE_IRQ)
>>> - irq = dev->resource[i].start;
>>> - }
>>> + res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>>> + if (!res)
>>> + return -EINVAL;
>>> +
>>> + physaddr = res->start;
>>> + if (!physaddr)
>>> + return -ENODEV;
>>> +
>>> + irq = platform_get_irq_optional(dev, 0);
>>>
>>> /* Call the bus-independent setup code */
>>> return ace_alloc(&dev->dev, id, physaddr, irq, bus_width);
>>>
>>
>> This driver is quite old and obsolete. I am fine with whatever patch and
>> I am also fine with marking driver as BROKEN or remove it because I am
>> not aware about any user.
>>
>> Acked-by: Michal Simek <michal.simek@xilinx.com>
>
> How about I queue this one up for 5.10, and then we can queue a removal
> patch for 5.11? Or at least schedule it for removal.
>
Works for me. I will send removal patch for 5.11.
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] xsysace: use platform_get_resource() and platform_get_irq_optional()
2020-10-30 7:08 ` Michal Simek
@ 2020-11-09 11:00 ` Michal Simek
0 siblings, 0 replies; 6+ messages in thread
From: Michal Simek @ 2020-11-09 11:00 UTC (permalink / raw)
To: Michal Simek, Jens Axboe, Andy Shevchenko, linux-arm-kernel, linux-block
On 30. 10. 20 8:08, Michal Simek wrote:
>
>
> On 29. 10. 20 15:22, Jens Axboe wrote:
>> On 10/29/20 1:18 AM, Michal Simek wrote:
>>> Hi Andy,
>>>
>>> On 27. 10. 20 18:11, Andy Shevchenko wrote:
>>>> Use platform_get_resource() to fetch the memory resource and
>>>> platform_get_irq_optional() to get optional IRQ instead of
>>>> open-coded variants.
>>>>
>>>> IRQ is not supposed to be changed at runtime, so there is
>>>> no functional change in ace_fsm_yieldirq().
>>>>
>>>> On the other hand we now take first resources instead of last ones
>>>> to proceed. I can't imagine how broken should be firmware to have
>>>> a garbage in the first resource slots. But if it the case, it needs
>>>> to be documented.
>>>>
>>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>> ---
>>>> drivers/block/xsysace.c | 49 ++++++++++++++++++++++-------------------
>>>> 1 file changed, 26 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
>>>> index 8d581c7536fb..eb8ef65778c3 100644
>>>> --- a/drivers/block/xsysace.c
>>>> +++ b/drivers/block/xsysace.c
>>>> @@ -443,22 +443,27 @@ static void ace_fix_driveid(u16 *id)
>>>> #define ACE_FSM_NUM_STATES 11
>>>>
>>>> /* Set flag to exit FSM loop and reschedule tasklet */
>>>> -static inline void ace_fsm_yield(struct ace_device *ace)
>>>> +static inline void ace_fsm_yieldpoll(struct ace_device *ace)
>>>> {
>>>> - dev_dbg(ace->dev, "ace_fsm_yield()\n");
>>>> tasklet_schedule(&ace->fsm_tasklet);
>>>> ace->fsm_continue_flag = 0;
>>>> }
>>>>
>>>> +static inline void ace_fsm_yield(struct ace_device *ace)
>>>> +{
>>>> + dev_dbg(ace->dev, "%s()\n", __func__);
>>>> + ace_fsm_yieldpoll(ace);
>>>> +}
>>>> +
>>>> /* Set flag to exit FSM loop and wait for IRQ to reschedule tasklet */
>>>> static inline void ace_fsm_yieldirq(struct ace_device *ace)
>>>> {
>>>> dev_dbg(ace->dev, "ace_fsm_yieldirq()\n");
>>>>
>>>> - if (!ace->irq)
>>>> - /* No IRQ assigned, so need to poll */
>>>> - tasklet_schedule(&ace->fsm_tasklet);
>>>> - ace->fsm_continue_flag = 0;
>>>> + if (ace->irq > 0)
>>>> + ace->fsm_continue_flag = 0;
>>>> + else
>>>> + ace_fsm_yieldpoll(ace);
>>>> }
>>>>
>>>> static bool ace_has_next_request(struct request_queue *q)
>>>> @@ -1053,12 +1058,12 @@ static int ace_setup(struct ace_device *ace)
>>>> ACE_CTRL_DATABUFRDYIRQ | ACE_CTRL_ERRORIRQ);
>>>>
>>>> /* Now we can hook up the irq handler */
>>>> - if (ace->irq) {
>>>> + if (ace->irq > 0) {
>>>> rc = request_irq(ace->irq, ace_interrupt, 0, "systemace", ace);
>>>> if (rc) {
>>>> /* Failure - fall back to polled mode */
>>>> dev_err(ace->dev, "request_irq failed\n");
>>>> - ace->irq = 0;
>>>> + ace->irq = rc;
>>>> }
>>>> }
>>>>
>>>> @@ -1110,7 +1115,7 @@ static void ace_teardown(struct ace_device *ace)
>>>>
>>>> tasklet_kill(&ace->fsm_tasklet);
>>>>
>>>> - if (ace->irq)
>>>> + if (ace->irq > 0)
>>>> free_irq(ace->irq, ace);
>>>>
>>>> iounmap(ace->baseaddr);
>>>> @@ -1123,11 +1128,6 @@ static int ace_alloc(struct device *dev, int id, resource_size_t physaddr,
>>>> int rc;
>>>> dev_dbg(dev, "ace_alloc(%p)\n", dev);
>>>>
>>>> - if (!physaddr) {
>>>> - rc = -ENODEV;
>>>> - goto err_noreg;
>>>> - }
>>>> -
>>>> /* Allocate and initialize the ace device structure */
>>>> ace = kzalloc(sizeof(struct ace_device), GFP_KERNEL);
>>>> if (!ace) {
>>>> @@ -1153,7 +1153,6 @@ static int ace_alloc(struct device *dev, int id, resource_size_t physaddr,
>>>> dev_set_drvdata(dev, NULL);
>>>> kfree(ace);
>>>> err_alloc:
>>>> -err_noreg:
>>>> dev_err(dev, "could not initialize device, err=%i\n", rc);
>>>> return rc;
>>>> }
>>>> @@ -1176,10 +1175,11 @@ static void ace_free(struct device *dev)
>>>>
>>>> static int ace_probe(struct platform_device *dev)
>>>> {
>>>> - resource_size_t physaddr = 0;
>>>> int bus_width = ACE_BUS_WIDTH_16; /* FIXME: should not be hard coded */
>>>> + resource_size_t physaddr;
>>>> + struct resource *res;
>>>> u32 id = dev->id;
>>>> - int irq = 0;
>>>> + int irq;
>>>> int i;
>>>>
>>>> dev_dbg(&dev->dev, "ace_probe(%p)\n", dev);
>>>> @@ -1190,12 +1190,15 @@ static int ace_probe(struct platform_device *dev)
>>>> if (of_find_property(dev->dev.of_node, "8-bit", NULL))
>>>> bus_width = ACE_BUS_WIDTH_8;
>>>>
>>>> - for (i = 0; i < dev->num_resources; i++) {
>>>> - if (dev->resource[i].flags & IORESOURCE_MEM)
>>>> - physaddr = dev->resource[i].start;
>>>> - if (dev->resource[i].flags & IORESOURCE_IRQ)
>>>> - irq = dev->resource[i].start;
>>>> - }
>>>> + res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>>>> + if (!res)
>>>> + return -EINVAL;
>>>> +
>>>> + physaddr = res->start;
>>>> + if (!physaddr)
>>>> + return -ENODEV;
>>>> +
>>>> + irq = platform_get_irq_optional(dev, 0);
>>>>
>>>> /* Call the bus-independent setup code */
>>>> return ace_alloc(&dev->dev, id, physaddr, irq, bus_width);
>>>>
>>>
>>> This driver is quite old and obsolete. I am fine with whatever patch and
>>> I am also fine with marking driver as BROKEN or remove it because I am
>>> not aware about any user.
>>>
>>> Acked-by: Michal Simek <michal.simek@xilinx.com>
>>
>> How about I queue this one up for 5.10, and then we can queue a removal
>> patch for 5.11? Or at least schedule it for removal.
>>
>
> Works for me. I will send removal patch for 5.11.
patch sent.
M
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v1] xsysace: use platform_get_resource() and platform_get_irq_optional()
@ 2020-08-04 14:45 Andy Shevchenko
0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2020-08-04 14:45 UTC (permalink / raw)
To: Michal Simek, linux-arm-kernel, Jens Axboe, linux-block; +Cc: Andy Shevchenko
Use platform_get_resource() to fetch the memory resource and
platform_get_irq_optional() to get optional IRQ instead of
open-coded variants.
IRQ is not supposed to be changed at runtime, so there is
no functional change in ace_fsm_yieldirq().
On the other hand we now take first resources instead of last ones
to proceed. I can't imagine how broken should be firmware to have
a garbage in the first resource slots. But if it the case, it needs
to be documented.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/block/xsysace.c | 49 ++++++++++++++++++++++-------------------
1 file changed, 26 insertions(+), 23 deletions(-)
diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
index 5d8e0ab3f054..53bb4455ecc2 100644
--- a/drivers/block/xsysace.c
+++ b/drivers/block/xsysace.c
@@ -443,22 +443,27 @@ static void ace_fix_driveid(u16 *id)
#define ACE_FSM_NUM_STATES 11
/* Set flag to exit FSM loop and reschedule tasklet */
-static inline void ace_fsm_yield(struct ace_device *ace)
+static inline void ace_fsm_yieldpoll(struct ace_device *ace)
{
- dev_dbg(ace->dev, "ace_fsm_yield()\n");
tasklet_schedule(&ace->fsm_tasklet);
ace->fsm_continue_flag = 0;
}
+static inline void ace_fsm_yield(struct ace_device *ace)
+{
+ dev_dbg(ace->dev, "%s()\n", __func__);
+ ace_fsm_yieldpoll(ace);
+}
+
/* Set flag to exit FSM loop and wait for IRQ to reschedule tasklet */
static inline void ace_fsm_yieldirq(struct ace_device *ace)
{
dev_dbg(ace->dev, "ace_fsm_yieldirq()\n");
- if (!ace->irq)
- /* No IRQ assigned, so need to poll */
- tasklet_schedule(&ace->fsm_tasklet);
- ace->fsm_continue_flag = 0;
+ if (ace->irq > 0)
+ ace->fsm_continue_flag = 0;
+ else
+ ace_fsm_yieldpoll(ace);
}
static bool ace_has_next_request(struct request_queue *q)
@@ -1059,12 +1064,12 @@ static int ace_setup(struct ace_device *ace)
ACE_CTRL_DATABUFRDYIRQ | ACE_CTRL_ERRORIRQ);
/* Now we can hook up the irq handler */
- if (ace->irq) {
+ if (ace->irq > 0) {
rc = request_irq(ace->irq, ace_interrupt, 0, "systemace", ace);
if (rc) {
/* Failure - fall back to polled mode */
dev_err(ace->dev, "request_irq failed\n");
- ace->irq = 0;
+ ace->irq = rc;
}
}
@@ -1116,7 +1121,7 @@ static void ace_teardown(struct ace_device *ace)
tasklet_kill(&ace->fsm_tasklet);
- if (ace->irq)
+ if (ace->irq > 0)
free_irq(ace->irq, ace);
iounmap(ace->baseaddr);
@@ -1129,11 +1134,6 @@ static int ace_alloc(struct device *dev, int id, resource_size_t physaddr,
int rc;
dev_dbg(dev, "ace_alloc(%p)\n", dev);
- if (!physaddr) {
- rc = -ENODEV;
- goto err_noreg;
- }
-
/* Allocate and initialize the ace device structure */
ace = kzalloc(sizeof(struct ace_device), GFP_KERNEL);
if (!ace) {
@@ -1159,7 +1159,6 @@ static int ace_alloc(struct device *dev, int id, resource_size_t physaddr,
dev_set_drvdata(dev, NULL);
kfree(ace);
err_alloc:
-err_noreg:
dev_err(dev, "could not initialize device, err=%i\n", rc);
return rc;
}
@@ -1182,10 +1181,11 @@ static void ace_free(struct device *dev)
static int ace_probe(struct platform_device *dev)
{
- resource_size_t physaddr = 0;
int bus_width = ACE_BUS_WIDTH_16; /* FIXME: should not be hard coded */
+ resource_size_t physaddr;
+ struct resource *res;
u32 id = dev->id;
- int irq = 0;
+ int irq;
int i;
dev_dbg(&dev->dev, "ace_probe(%p)\n", dev);
@@ -1196,12 +1196,15 @@ static int ace_probe(struct platform_device *dev)
if (of_find_property(dev->dev.of_node, "8-bit", NULL))
bus_width = ACE_BUS_WIDTH_8;
- for (i = 0; i < dev->num_resources; i++) {
- if (dev->resource[i].flags & IORESOURCE_MEM)
- physaddr = dev->resource[i].start;
- if (dev->resource[i].flags & IORESOURCE_IRQ)
- irq = dev->resource[i].start;
- }
+ res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
+
+ physaddr = res->start;
+ if (!physaddr)
+ return -ENODEV;
+
+ irq = platform_get_irq_optional(dev, 0);
/* Call the bus-independent setup code */
return ace_alloc(&dev->dev, id, physaddr, irq, bus_width);
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-09 11:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-27 17:11 [PATCH v1] xsysace: use platform_get_resource() and platform_get_irq_optional() Andy Shevchenko
2020-10-29 7:18 ` Michal Simek
2020-10-29 14:22 ` Jens Axboe
2020-10-30 7:08 ` Michal Simek
2020-11-09 11:00 ` Michal Simek
-- strict thread matches above, loose matches on Subject: below --
2020-08-04 14:45 Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).