Messages in this thread | | | Subject | Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink | From | Daniel Lezcano <> | Date | Wed, 1 Jul 2020 14:13:52 +0200 |
| |
On 01/07/2020 14:10, Amit Kucheria wrote: > On Wed, Jul 1, 2020 at 3:15 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> >> On 01/07/2020 11:33, Amit Kucheria wrote: >>> On Wed, Jul 1, 2020 at 2:56 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >>>> >>>> On 30/06/2020 13:47, Amit Kucheria wrote: >>>>> On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano >>>>> <daniel.lezcano@linaro.org> wrote: >>>>>> > >>>>>> /* Adding event notification support elements */ >>>>>> #define THERMAL_GENL_FAMILY_NAME "thermal_event" >>>>>> -#define THERMAL_GENL_VERSION 0x01 >>>>>> +#define THERMAL_GENL_VERSION 0x02 >>>>> >>>>> This hunk should be removed since you set version back to 1 in the >>>>> next patch and we don't actually intend to bump the version yet. >>>> >>>> Well, I've been very strict here for git-bisecting. >>>> >>>> I move to V2 because of the removal, but when adding the new genetlink >>>> code, the family name changed, so we returned back to the V1 as it is a >>>> new genetlink thermal brand. >>> >>> I don't understand the move to v2 for an empty skeleton UAPI. For the >>> purposes of bisection, couldn't you just remove all the v1 UAPI (w/o >>> bumping to v2) and then add a new UAPI in the next patch? >>> >>>> The name is change because it is no longer event based but also sampling >>>> and commands. >>> >>> In this case, just to avoid any confusion, the new UAPI could be v2 >>> making the transition clear in case of bisection. >>> >>> I'm afraid the v1->v2->v1 is a bit more confusing. >> >> Let me elaborate a bit: >> >> Why there is this patch ? >> - By removing this code first, the next patch will just contain >> additions, I thought it would be clearer >> >> Why increase the version here ? >> - Code must continue to compile and as the 'thermal_event' family is now >> different from V1, the version is changed >> >> Why the version goes to V1 in the next patch ? >> - The family name is changed as it is not doing event only, so it is a >> new netlink thermal protocol and we begin at V1 >> >> So the main reason of this patch is to be very strict in the iteration >> changes. May be it is too much, in this case I can merge this patch with >> 4/5, the old netlink protocol removal will be lost in the addition of >> the new protocol. I'm fine with that if you think it is simpler. > > Considering that there are no users of v1 currently, it feels a bit > over engineered, IMHO. > > Also, the new UAPI doesn't need to begin at v1. Just having it start > at v2 will avoid this confusion, no?
Ok, I will merge both patches but I will keep the V1 because the netlink protocol is a new one.
-- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
| |