Re: [PATCH v1 4/5] mtd: m25p80: Check if the spi flash device has pm support

[Kamal Dasu <kdasu.kdev@xxxxxxxxx>]

On Mon, Feb 6, 2017 at 6:01 AM, Cyrille Pitchen
<cyrille.pitchen@xxxxxxxxx> wrote:
> Hi Kamal,
>
> Le 04/02/2017 à 00:31, Kamal Dasu a écrit :
>> Call the spi_nor_rescan() only if the controller driver needs this
>> support. This way SoCs that need this feature can use it.
>>
>> Signed-off-by: Kamal Dasu <kdasu.kdev@xxxxxxxxx>
>> ---
>>  drivers/mtd/devices/m25p80.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 4528e33..ffdec60 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -328,8 +328,13 @@ static int m25p_suspend(struct device *dev)
>>  static int m25p_resume(struct device *dev)
>>  {
>>       struct m25p *flash = dev_get_drvdata(dev);
>> +     struct spi_device *spi = flash->spi;
>> +     int ret = 0;
>> +
>> +     if (spi_flash_pm_supported(spi))
>> +             ret = spi_nor_pm_rescan(&flash->spi_nor, NULL);
>>
>
> Why don't you squash patch 2 into this one? Patch 2 suggests that
> spi_nor_pm_rescan() could safely be called in any case but now this patch
> suggests that calling that function is not so safe.
>
> I see two cases:
>
> 1/ either calling spi_nor_pm_rescan() is safe in any case, then patches 3,
> 4 and 5 are needless.
>

I agree it should be safe to call resume after implementing your
suggestions for patch 1 itself.

> 2/ or calling spi_nor_pm_rescan() has unwanted side effects in some cases,
> then patch 2 should be squashed into this patch: patch 2 will never be
> taken alone in the spi-nor tree if it may introduce bugs or regressions.
>
> If something has to be done in the SPI sub-system, I guess it should be
> done first: for sure applying patches 3 and 5 would not create any issue.
> Then patches 2 and 4 squashed into a single patch may be safely applied after.
>
> However, patches 3 and 5 still need to be discussed first.
>

I agree that these are not needed.

> Best regards,
>
> Cyrille
>
>> -     return spi_nor_pm_rescan(&flash->spi-nor, NULL);
>> +     return ret;
>>  }
>>  #endif
>>  static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
>>
>

Thanks
Kamal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html