Il 20/05/22 11:35, Miles Chen ha scritto:
>
>>>
>>> Thanks for submitting this patch.
>>>
>>> I compare this with drivers/clk/mediatek/clk-mt7986-apmixed.c,
>>> and other clk files are using macros to make the mtk_pll_data array
>>> more readable.
>>
>> I'd actually argue that macros make it less readable. While reading
>> other drivers I had a lot of trouble figuring out which argument
>> is which field of the struct, and had to constantly go back to the
>> macro definitions and count arguments to find it. Having it this
>> way, each value is labeled clearly with the field it's in. I think
>> the tradeoff between line count and readability here is worth it.
>
> It is easier for multiple developers to work together if we have a common style.
>
> How do you think?
>
In my opinion, Yassine is definitely right about this one: unrolling these macros
will make the code more readable, even though this has the side effect of making
it bigger in the source code form (obviously, when compiled, it's going to be the
exact same size).
I wouldn't mind getting this clock driver in without the usage of macros, as much
as I wouldn't mind converting all of the existing drivers to open-code everything
instead of using macros that you have to find in various headers... this practice
was done in multiple drivers (clock or elsewhere), so I don't think that it would
actually be a bad idea to do it here on MediaTek too, even though I'm not aware of
any *rule* that may want us to do that: if you check across drivers/clk/*, there's
a big split in how drivers are made, where some are using macros (davinci, renesas,
samsung, sprd, etc), and some are not (bcm, sunxi-ng, qcom, tegra, versatile, etc),
so it's really "do it as you wish"...
... *but:*
Apart from that, I also don't think that it is a good idea to convert the other
MTK clock drivers right now, as this would make the upstreaming of MediaTek clock
drivers harder for some of the community in this moment... especially when we look
at how many MTK SoCs are out there in the wild, and how many we have upstream:
something like 10% of them, or less.
I see the huge benefit of having a bigger community around MediaTek platforms as
that's beneficial to get a way better support and solidity for all SoCs as they
are sharing the same drivers and same framework, and expanding the support to more
of them will only make it better with highly valuable community contributions.
That said, Yassine, you should've understood that you have my full support on
unrolling these macros - but it's not time to do that yet: you definitely know
that MediaTek clock drivers are going through a big cleanup phase which is, at
this point, unavoidable... if we are able to get the aid of scripts (cocci and
others), that will make our life easier in this cleanup, and will also make us
able to perform the entire cleanup with less effort and in less overall time.
With that, I'm sad but I have to support Miles' decision on this one, and I also
have to ask you to use macros in this driver.
I am sure - and it is my wish - to see MediaTek clock drivers open-coding stuff
instead of using macros, but that's something for the future - which will happen
after the more important cleanups.
After all, it will be just about running "gcc -E xxxx.c" and copy-pasting the
unrolled macros to the clock drivers, which will be pretty fast and straightforward.
Sorry for the wall of text, by the way.
Cheers,
Angelo
On Fri, May 20 2022 at 12:26:25 +0200, AngeloGioacchino Del Regno
<[email protected]> wrote:
> Il 20/05/22 11:35, Miles Chen ha scritto:
>>
>>>>
>>>> Thanks for submitting this patch.
>>>>
>>>> I compare this with drivers/clk/mediatek/clk-mt7986-apmixed.c,
>>>> and other clk files are using macros to make the mtk_pll_data array
>>>> more readable.
>>>
>>> I'd actually argue that macros make it less readable. While reading
>>> other drivers I had a lot of trouble figuring out which argument
>>> is which field of the struct, and had to constantly go back to the
>>> macro definitions and count arguments to find it. Having it this
>>> way, each value is labeled clearly with the field it's in. I think
>>> the tradeoff between line count and readability here is worth it.
>>
>> It is easier for multiple developers to work together if we have a
>> common style.
>>
>> How do you think?
>>
>
> In my opinion, Yassine is definitely right about this one: unrolling
> these macros
> will make the code more readable, even though this has the side
> effect of making
> it bigger in the source code form (obviously, when compiled, it's
> going to be the
> exact same size).
>
> I wouldn't mind getting this clock driver in without the usage of
> macros, as much
> as I wouldn't mind converting all of the existing drivers to
> open-code everything
> instead of using macros that you have to find in various headers...
> this practice
> was done in multiple drivers (clock or elsewhere), so I don't think
> that it would
> actually be a bad idea to do it here on MediaTek too, even though I'm
> not aware of
> any *rule* that may want us to do that: if you check across
> drivers/clk/*, there's
> a big split in how drivers are made, where some are using macros
> (davinci, renesas,
> samsung, sprd, etc), and some are not (bcm, sunxi-ng, qcom, tegra,
> versatile, etc),
> so it's really "do it as you wish"...
>
> ... *but:*
>
> Apart from that, I also don't think that it is a good idea to convert
> the other
> MTK clock drivers right now, as this would make the upstreaming of
> MediaTek clock
> drivers harder for some of the community in this moment... especially
> when we look
> at how many MTK SoCs are out there in the wild, and how many we have
> upstream:
> something like 10% of them, or less.
>
> I see the huge benefit of having a bigger community around MediaTek
> platforms as
> that's beneficial to get a way better support and solidity for all
> SoCs as they
> are sharing the same drivers and same framework, and expanding the
> support to more
> of them will only make it better with highly valuable community
> contributions.
>
>
> That said, Yassine, you should've understood that you have my full
> support on
> unrolling these macros - but it's not time to do that yet: you
> definitely know
> that MediaTek clock drivers are going through a big cleanup phase
> which is, at
> this point, unavoidable... if we are able to get the aid of scripts
> (cocci and
> others), that will make our life easier in this cleanup, and will
> also make us
> able to perform the entire cleanup with less effort and in less
> overall time.
>
> With that, I'm sad but I have to support Miles' decision on this one,
> and I also
> have to ask you to use macros in this driver.
>
>
> I am sure - and it is my wish - to see MediaTek clock drivers
> open-coding stuff
> instead of using macros, but that's something for the future - which
> will happen
> after the more important cleanups.
>
> After all, it will be just about running "gcc -E xxxx.c" and
> copy-pasting the
> unrolled macros to the clock drivers, which will be pretty fast and
> straightforward.
>
> Sorry for the wall of text, by the way.
>
> Cheers,
> Angelo
Fair enough. I'll switch to macros in the next version.
Thanks,
Yassine
On Friday, May 20th, 2022 at 11:26 AM, AngeloGioacchino Del Regno <[email protected]> wrote:
> Il 20/05/22 11:35, Miles Chen ha scritto:
>
> > > > Thanks for submitting this patch.
> > > >
> > > > I compare this with drivers/clk/mediatek/clk-mt7986-apmixed.c,
> > > > and other clk files are using macros to make the mtk_pll_data array
> > > > more readable.
> > >
> > > I'd actually argue that macros make it less readable. While reading
> > > other drivers I had a lot of trouble figuring out which argument
> > > is which field of the struct, and had to constantly go back to the
> > > macro definitions and count arguments to find it. Having it this
> > > way, each value is labeled clearly with the field it's in. I think
> > > the tradeoff between line count and readability here is worth it.
> >
> > It is easier for multiple developers to work together if we have a common style.
> >
> > How do you think?
>
>
> In my opinion, Yassine is definitely right about this one: unrolling these macros
> will make the code more readable, even though this has the side effect of making
> it bigger in the source code form (obviously, when compiled, it's going to be the
> exact same size).
>
> I wouldn't mind getting this clock driver in without the usage of macros, as much
> as I wouldn't mind converting all of the existing drivers to open-code everything
> instead of using macros that you have to find in various headers... this practice
> was done in multiple drivers (clock or elsewhere), so I don't think that it would
> actually be a bad idea to do it here on MediaTek too, even though I'm not aware of
> any rule that may want us to do that: if you check across drivers/clk/*, there's
> a big split in how drivers are made, where some are using macros (davinci, renesas,
> samsung, sprd, etc), and some are not (bcm, sunxi-ng, qcom, tegra, versatile, etc),
> so it's really "do it as you wish"...
>
> ... but:
>
> Apart from that, I also don't think that it is a good idea to convert the other
> MTK clock drivers right now, as this would make the upstreaming of MediaTek clock
> drivers harder for some of the community in this moment... especially when we look
> at how many MTK SoCs are out there in the wild, and how many we have upstream:
> something like 10% of them, or less.
>
> I see the huge benefit of having a bigger community around MediaTek platforms as
> that's beneficial to get a way better support and solidity for all SoCs as they
> are sharing the same drivers and same framework, and expanding the support to more
> of them will only make it better with highly valuable community contributions.
>
>
> That said, Yassine, you should've understood that you have my full support on
> unrolling these macros - but it's not time to do that yet: you definitely know
> that MediaTek clock drivers are going through a big cleanup phase which is, at
> this point, unavoidable... if we are able to get the aid of scripts (cocci and
> others), that will make our life easier in this cleanup, and will also make us
> able to perform the entire cleanup with less effort and in less overall time.
>
> With that, I'm sad but I have to support Miles' decision on this one, and I also
> have to ask you to use macros in this driver.
I'm picking up this series again now after taking a long break to allow for
ongoing cleanup and refactoring work to settle down. I was going to make this
change but then I couldn't find the PLL macro defined in any common header.
It seems that it is defined in every driver that uses it, with slight variations
in some of them. Should I just do the same, or would it be better to define it
in clk-pll.h? Also, would now be a good time to unroll the macros in all drivers,
or is it still too soon?
Another thing: Since I've been out of touch with the cleanup work for a while,
it would be great if someone makes me aware of any pending cleanup patches that
I should know of so that I base my patches on them and avoid duplicating work.
> ...
Il 13/08/22 12:44, Yassine Oudjana ha scritto:
> On Friday, May 20th, 2022 at 11:26 AM, AngeloGioacchino Del Regno <[email protected]> wrote:
>
>> Il 20/05/22 11:35, Miles Chen ha scritto:
>>
>>>>> Thanks for submitting this patch.
>>>>>
>>>>> I compare this with drivers/clk/mediatek/clk-mt7986-apmixed.c,
>>>>> and other clk files are using macros to make the mtk_pll_data array
>>>>> more readable.
>>>>
>>>> I'd actually argue that macros make it less readable. While reading
>>>> other drivers I had a lot of trouble figuring out which argument
>>>> is which field of the struct, and had to constantly go back to the
>>>> macro definitions and count arguments to find it. Having it this
>>>> way, each value is labeled clearly with the field it's in. I think
>>>> the tradeoff between line count and readability here is worth it.
>>>
>>> It is easier for multiple developers to work together if we have a common style.
>>>
>>> How do you think?
>>
>>
>> In my opinion, Yassine is definitely right about this one: unrolling these macros
>> will make the code more readable, even though this has the side effect of making
>> it bigger in the source code form (obviously, when compiled, it's going to be the
>> exact same size).
>>
>> I wouldn't mind getting this clock driver in without the usage of macros, as much
>> as I wouldn't mind converting all of the existing drivers to open-code everything
>> instead of using macros that you have to find in various headers... this practice
>> was done in multiple drivers (clock or elsewhere), so I don't think that it would
>> actually be a bad idea to do it here on MediaTek too, even though I'm not aware of
>> any rule that may want us to do that: if you check across drivers/clk/*, there's
>> a big split in how drivers are made, where some are using macros (davinci, renesas,
>> samsung, sprd, etc), and some are not (bcm, sunxi-ng, qcom, tegra, versatile, etc),
>> so it's really "do it as you wish"...
>>
>> ... but:
>>
>> Apart from that, I also don't think that it is a good idea to convert the other
>> MTK clock drivers right now, as this would make the upstreaming of MediaTek clock
>> drivers harder for some of the community in this moment... especially when we look
>> at how many MTK SoCs are out there in the wild, and how many we have upstream:
>> something like 10% of them, or less.
>>
>> I see the huge benefit of having a bigger community around MediaTek platforms as
>> that's beneficial to get a way better support and solidity for all SoCs as they
>> are sharing the same drivers and same framework, and expanding the support to more
>> of them will only make it better with highly valuable community contributions.
>>
>>
>> That said, Yassine, you should've understood that you have my full support on
>> unrolling these macros - but it's not time to do that yet: you definitely know
>> that MediaTek clock drivers are going through a big cleanup phase which is, at
>> this point, unavoidable... if we are able to get the aid of scripts (cocci and
>> others), that will make our life easier in this cleanup, and will also make us
>> able to perform the entire cleanup with less effort and in less overall time.
>>
>> With that, I'm sad but I have to support Miles' decision on this one, and I also
>> have to ask you to use macros in this driver.
>
> I'm picking up this series again now after taking a long break to allow for
> ongoing cleanup and refactoring work to settle down. I was going to make this
> change but then I couldn't find the PLL macro defined in any common header.
> It seems that it is defined in every driver that uses it, with slight variations
> in some of them. Should I just do the same, or would it be better to define it
> in clk-pll.h? Also, would now be a good time to unroll the macros in all drivers,
> or is it still too soon?
Hello Yassine,
I'm sorry for the very late reply to this topic, but I just got back from vacation.
Please follow the current way of defining the PLL macro into the SoC-specific
driver: MediaTek folks are in the process of implementing Frequency Hopping (FHCTL)
on supported PLLs so "some things may change"... maybe in your driver too... I'm
not sure, though, whether MT6735 does support FHCTL and anyway, even if it does,
I would recommend to just go without it as a first step, as adding FHCTL capability
will be done on all(?) of the supported SoC clock drivers as soon as it lands.
>
> Another thing: Since I've been out of touch with the cleanup work for a while,
> it would be great if someone makes me aware of any pending cleanup patches that
> I should know of so that I base my patches on them and avoid duplicating work.
>
Simply check linux-mediatek, but I don't think that there's anything in the
pipeline that would be blocking your MT6735 drivers.
Cheers,
Angelo