Re: [PATCH 01/16] lib: fix match_string() helper on -1 array size
On Wed, 2019-05-08 at 14:28 +0300, Alexandru Ardelean wrote:
> The documentation the `_match_string()` helper mentions that `n`
> should be:
> * @n: number of strings in the array or -1 for NULL terminated arrays
>
> The behavior of the function is different, in the sense that it exits on
> the first NULL element in the array, regardless of whether `n` is -1 or a
> positive number.
>
> This patch changes the behavior, to exit the loop when a NULL element is
> found and n == -1. Essentially, this aligns the behavior with the
> doc-string.
>
> There are currently many users of `match_string()`, and so, in order to
> go
> through them, the next patches in the series will focus on doing some
> cosmetic changes, which are aimed at grouping the users of
> `match_string()`.
>
This is the duplicate & should be dropped.
Sorry for this.
> Signed-off-by: Alexandru Ardelean
> ---
> lib/string.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/string.c b/lib/string.c
> index 3ab861c1a857..76edb7bf76cb 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -648,8 +648,11 @@ int match_string(const char * const *array, size_t
> n, const char *string)
>
> for (index = 0; index < n; index++) {
> item = array[index];
> - if (!item)
> + if (!item) {
> + if (n != (size_t)-1)
> + continue;
> break;
> + }
> if (!strcmp(item, string))
> return index;
> }
Re: [PATCH 09/16] mmc: sdhci-xenon: use new match_string() helper/macro
On Wed, 2019-05-08 at 15:20 +0300, Dan Carpenter wrote:
>
>
> On Wed, May 08, 2019 at 02:28:35PM +0300, Alexandru Ardelean wrote:
> > -static const char * const phy_types[] = {
> > - "emmc 5.0 phy",
> > - "emmc 5.1 phy"
> > -};
> > -
> > enum xenon_phy_type_enum {
> > EMMC_5_0_PHY,
> > EMMC_5_1_PHY,
> > NR_PHY_TYPES
>
> There is no need for NR_PHY_TYPES now so you could remove that as well.
>
I thought the same.
The only reason to keep NR_PHY_TYPES, is for potential future patches,
where it would be just 1 addition
enum xenon_phy_type_enum {
EMMC_5_0_PHY,
EMMC_5_1_PHY,
+ EMMC_5_2_PHY,
NR_PHY_TYPES
}
Depending on style/preference of how to do enums (allow comma on last enum
or not allow comma on last enum value), adding new enum values woudl be 2
additions + 1 deletion lines.
enum xenon_phy_type_enum {
EMMC_5_0_PHY,
- EMMC_5_1_PHY
+ EMM
C_5_1_PHY,
+ EMMC_5_2_PHY
}
Either way (leave NR_PHY_TYPES or remove NR_PHY_TYPES) is fine from my
side.
Thanks
Alex
> regards,
> dan carpenter
>
Re: [PATCH 03/16] lib,treewide: add new match_string() helper/macro
On Wed, 2019-05-08 at 15:18 +0200, Greg KH wrote: > > > On Wed, May 08, 2019 at 04:11:28PM +0300, Andy Shevchenko wrote: > > On Wed, May 08, 2019 at 02:28:29PM +0300, Alexandru Ardelean wrote: > > > This change re-introduces `match_string()` as a macro that uses > > > ARRAY_SIZE() to compute the size of the array. > > > The macro is added in all the places that do > > > `match_string(_a, ARRAY_SIZE(_a), s)`, since the change is pretty > > > straightforward. > > > > Can you split include/linux/ change from the rest? > > That would break the build, why do you want it split out? This makes > sense all as a single patch to me. > Not really. It would be just be the new match_string() helper/macro in a new commit. And the conversions of the simple users of match_string() (the ones using ARRAY_SIZE()) in another commit. Thanks Alex > thanks, > > greg k-h
Re: [PATCH 09/16] mmc: sdhci-xenon: use new match_string() helper/macro
On Wed, 2019-05-08 at 16:26 +0300, Alexandru Ardelean wrote:
> On Wed, 2019-05-08 at 15:20 +0300, Dan Carpenter wrote:
> >
> >
> > On Wed, May 08, 2019 at 02:28:35PM +0300, Alexandru Ardelean wrote:
> > > -static const char * const phy_types[] = {
> > > - "emmc 5.0 phy",
> > > - "emmc 5.1 phy"
> > > -};
> > > -
> > > enum xenon_phy_type_enum {
> > > EMMC_5_0_PHY,
> > > EMMC_5_1_PHY,
> > > NR_PHY_TYPES
> >
> > There is no need for NR_PHY_TYPES now so you could remove that as well.
> >
>
> I thought the same.
> The only reason to keep NR_PHY_TYPES, is for potential future patches,
> where it would be just 1 addition
>
> enum xenon_phy_type_enum {
> EMMC_5_0_PHY,
> EMMC_5_1_PHY,
> + EMMC_5_2_PHY,
> NR_PHY_TYPES
> }
>
> Depending on style/preference of how to do enums (allow comma on last
> enum
> or not allow comma on last enum value), adding new enum values woudl be 2
> additions + 1 deletion lines.
>
> enum xenon_phy_type_enum {
> EMMC_5_0_PHY,
> - EMMC_5_1_PHY
> + EMM
> C_5_1_PHY,
> + EMMC_5_2_PHY
> }
>
> Either way (leave NR_PHY_TYPES or remove NR_PHY_TYPES) is fine from my
> side.
>
Preference on this ?
If no objection [nobody insists] I would keep.
I don't feel strongly about it [dropping NR_PHY_TYPES or not].
Thanks
Alex
> Thanks
> Alex
>
> > regards,
> > dan carpenter
> >
Re: [PATCH 03/16] lib,treewide: add new match_string() helper/macro
On Wed, 2019-05-08 at 16:22 +0300, Alexandru Ardelean wrote: > On Wed, 2019-05-08 at 15:18 +0200, Greg KH wrote: > > > > > > On Wed, May 08, 2019 at 04:11:28PM +0300, Andy Shevchenko wrote: > > > On Wed, May 08, 2019 at 02:28:29PM +0300, Alexandru Ardelean wrote: > > > > This change re-introduces `match_string()` as a macro that uses > > > > ARRAY_SIZE() to compute the size of the array. > > > > The macro is added in all the places that do > > > > `match_string(_a, ARRAY_SIZE(_a), s)`, since the change is pretty > > > > straightforward. > > > > > > Can you split include/linux/ change from the rest? > > > > That would break the build, why do you want it split out? This makes > > sense all as a single patch to me. > > > > Not really. > It would be just be the new match_string() helper/macro in a new commit. > And the conversions of the simple users of match_string() (the ones using > ARRAY_SIZE()) in another commit. > I should have asked in my previous reply. Leave this as-is or re-formulate in 2 patches ? No strong preference from my side. Thanks Alex > Thanks > Alex > > > thanks, > > > > greg k-h
Re: [PATCH 09/16] mmc: sdhci-xenon: use new match_string() helper/macro
On Fri, 2019-05-10 at 14:01 +0300, Dan Carpenter wrote:
> [External]
>
>
> On Fri, May 10, 2019 at 09:13:26AM +0000, Ardelean, Alexandru wrote:
> > On Wed, 2019-05-08 at 16:26 +0300, Alexandru Ardelean wrote:
> > > On Wed, 2019-05-08 at 15:20 +0300, Dan Carpenter wrote:
> > > >
> > > >
> > > > On Wed, May 08, 2019 at 02:28:35PM +0300, Alexandru Ardelean wrote:
> > > > > -static const char * const phy_types[] = {
> > > > > - "emmc 5.0 phy",
> > > > > - "emmc 5.1 phy"
> > > > > -};
> > > > > -
> > > > > enum xenon_phy_type_enum {
> > > > > EMMC_5_0_PHY,
> > > > > EMMC_5_1_PHY,
> > > > > NR_PHY_TYPES
> > > >
> > > > There is no need for NR_PHY_TYPES now so you could remove that as
> > > > well.
> > > >
> > >
> > > I thought the same.
> > > The only reason to keep NR_PHY_TYPES, is for potential future
> > > patches,
> > > where it would be just 1 addition
> > >
> > > enum xenon_phy_type_enum {
> > > EMMC_5_0_PHY,
> > > EMMC_5_1_PHY,
> > > + EMMC_5_2_PHY,
> > > NR_PHY_TYPES
> > > }
> > >
> > > Depending on style/preference of how to do enums (allow comma on last
> > > enum
> > > or not allow comma on last enum value), adding new enum values woudl
> > > be 2
> > > additions + 1 deletion lines.
> > >
> > > enum xenon_phy_type_enum {
> > > EMMC_5_0_PHY,
> > > - EMMC_5_1_PHY
> > > + EMM
> > > C_5_1_PHY,
> > > + EMMC_5_2_PHY
> > > }
> > >
> > > Either way (leave NR_PHY_TYPES or remove NR_PHY_TYPES) is fine from
> > > my
> > > side.
> > >
> >
> > Preference on this ?
> > If no objection [nobody insists] I would keep.
> >
> > I don't feel strongly about it [dropping NR_PHY_TYPES or not].
>
> If you end up resending the series could you remove it, but if not then
> it's not worth it.
ack
thanks
Alex
>
> regards,
> dan carpenter
>
Re: [PATCH 03/16] lib,treewide: add new match_string() helper/macro
On Fri, 2019-05-10 at 17:34 +0300, andriy.shevche...@linux.intel.com wrote: > [External] > > > On Fri, May 10, 2019 at 09:15:27AM +0000, Ardelean, Alexandru wrote: > > On Wed, 2019-05-08 at 16:22 +0300, Alexandru Ardelean wrote: > > > On Wed, 2019-05-08 at 15:18 +0200, Greg KH wrote: > > > > On Wed, May 08, 2019 at 04:11:28PM +0300, Andy Shevchenko wrote: > > > > > On Wed, May 08, 2019 at 02:28:29PM +0300, Alexandru Ardelean > > > > > wrote: > > > > > Can you split include/linux/ change from the rest? > > > > > > > > That would break the build, why do you want it split out? This > > > > makes > > > > sense all as a single patch to me. > > > > > > > > > > Not really. > > > It would be just be the new match_string() helper/macro in a new > > > commit. > > > And the conversions of the simple users of match_string() (the ones > > > using > > > ARRAY_SIZE()) in another commit. > > > > > > > I should have asked in my previous reply. > > Leave this as-is or re-formulate in 2 patches ? > > Depends on on what you would like to spend your time: collecting Acks for > all > pieces in treewide patch or send new API first followed up by per driver > / > module update in next cycle. I actually would have preferred new API first, with the current `match_string()` -> `__match_string()` rename from the start, but I wasn't sure. I am still navigating through how feedbacks are working in this realm. I'll send a V2 with the API change-first/only; should be a smaller list. Then see about follow-ups/changes per subsystems. > > I also have no strong preference. > And I think it's good to add Heikki Krogerus to Cc list for both patch > series, > since he is the author of sysfs variant and may have something to comment > on > the rest. Thanks for the reference. > > -- > With Best Regards, > Andy Shevchenko > >
