Re: [PATCH V2 1/2] mtd: move code reading DT specified part probes to the core

["Rafał Miłecki" <zajec5@gmail.com>]

On 03/31/2017 12:31 PM, Boris Brezillon wrote:
> On Fri, 31 Mar 2017 12:12:56 +0200
> Rafał Miłecki <zajec5@gmail.com> wrote:
>
>> On 03/31/2017 11:56 AM, Boris Brezillon wrote:
>>> On Fri, 31 Mar 2017 11:30:12 +0200
>>> Rafał Miłecki <zajec5@gmail.com> wrote:
>>>
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>
>>>> Handling (creating) partitions for flash devices requires using a proper
>>>> driver that will read partition table (out of somewhere). We can't
>>>> simply try all existing drivers one by one, so MTD subsystem allows
>>>> drivers to specify a list of applicable part probes.
>>>>
>>>> So far physmap_of was the only driver with support for linux,part-probe
>>>> DT property. Other ones had list or probes hardcoded which wasn't making
>>>> them really flexible. It prevented using common flash drivers on
>>>> platforms that required some specific partition table access.
>>>>
>>>> This commit moves support for mentioned DT property to the MTD core
>>>> file. Thanks to calling it on device parse registration (as suggested by
>>>> Boris) all drivers gain support for it for free.
>>>>
>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>> ---
>>>>  drivers/mtd/maps/physmap_of.c | 36 +-----------------------------------
>>>>  drivers/mtd/mtdcore.c         | 34 ++++++++++++++++++++++++++++++++++
>>>
>>> Maybe you should split the patch:
>>> 1/ add core infrastructure
>>> 2/ remove open-coded version in drivers/mtd/maps/physmap_of.c
>>
>> What's the gain of that? Is this patch too complex to review properly? Will that
>> be useful for anyone to have it split? For me it only adds an intermediate code
>> duplication.
>
> The gain is that we get rid of the dependency we have on patch "mtd:
> physmap_of: use OF helpers for reading strings" which is not even
> mentioned in your mails.

This is definitely my mistake, initially I pushed a proper comment below tear
line but have overwritten it later. Sorry!


>>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
>>> for reading strings" patch is really useful, since you move to the
>>> common infrastructure here.
>>> By following my suggestion you get rid of the dependency you have
>>> between this series and patch "mtd: physmap_of: use OF helpers for
>>> reading strings".
>>
>> I learned (the very hard way) MTD people can be really nitpicking so I'm
>> sending as simple patches as I can. I see it as the only way for someone from
>> OpenWrt/LEDE project to get patch through your review.
>
> And I learned the hard way that OpenWRT/LEDE developers tend to not
> listen to our advices and keep arguing on things that have been proven
> to be existing because of bad decision they made at some point in the
> project life. So I think we're even :-P.

I wish you could sometimes forget what you've learned and review/discuss things
without all that negative approach I keep seeing.


>> It's like with this patch: even a simple code move can be questioned. Please
>> drop this patchset, I'll resend it after/if I manage to get
>> [PATCH] mtd: physmap_of: use OF helpers for reading strings
>> accepted.
>
> But really, what's the point of this patch? It's just a cleanup. You're
> not fixing a bug or changing the behavior, and your real objective is
> to get support for the linux,part-probe in the core, which will then
> allow us to drop the open-coded version you have in physmap_of.c.
>
> I don't think it deserves an intermediate patch, unless your real
> objective is patchcount.

OK, I'm going to trust that and see how easily I get can patch your way. I'll
resend combined version soon.