[Upstream] [PATCH v2 1/2] board: phytec: common: Add product information to FTD
Yannic Moog
Y.Moog at phytec.de
Tue Jan 28 11:40:29 CET 2025
Hi Daniel,
On Thu, 2025-01-23 at 14:41 +0000, Daniel Schultz wrote:
> Hi Yannic,
>
> I sent these patches on our internal mailing list as RFC. Please
> understand that I won't make bigger now, since we're colleges and you
> haven't reviewed in the (for you) three iterations before. So, there
> might be feedback I won't comment.
I apologise for not reviewing earlier. I think improvements are worth adding and they have no
expiration date.
>
> Documentation should be added as follow-up patch because it's missing
> for all function right now. Same for 'const'. I'm not a fan of adding
> that now and making this module inconsistent about how we use it.
I am not asking you to change existing code/module. I am requesting you to change the signature of
your newly introduced functions.
The function takes pointer to data where neither the pointer nor the data must be modified.
Therefore they should be const.
Same for the documentation. I am fine with you moving the existing code and not adding documentation
. I would like brief documentation for the newly created functions.
Yannic
>
> On 16.01.25 09:06, Yannic Moog wrote:
> > Hello Daniel,
> >
> > On Wed, 2025-01-15 at 02:35 -0800, Daniel Schultz wrote:
> > > ft_board_setup inside the board code allows to alter
> > > device-tree during the boot process.
> > >
> > > Introduce a new function for the PHYTEC SOM detection
> > > to read the product name and part number from the EEPROM
> > > content and include both into the device-tree as
> > > * phytec,som-part-number
> > > * phytec,som-product-name
> > >
> > > This function can be called from the board code when those
> > > values should be exposed to Linux.
> > >
> > > Signed-off-by: Daniel Schultz <d.schultz at phytec.de>
> > > ---
> > >
> > > Changes in v2:
> > > * Removed 'struct bd_info' as argument in function phytec_ft_board_fixup
> > >
> > > board/phytec/common/phytec_som_detection.c | 202 ++++++++++++++++-----
> > > board/phytec/common/phytec_som_detection.h | 7 +
> > > 2 files changed, 166 insertions(+), 43 deletions(-)
> > >
> > > diff --git a/board/phytec/common/phytec_som_detection.c
> > > b/board/phytec/common/phytec_som_detection.c
> > > index 166c3eae565..a4a1d20e0ec 100644
> > > --- a/board/phytec/common/phytec_som_detection.c
> > > +++ b/board/phytec/common/phytec_som_detection.c
> > > @@ -271,11 +271,126 @@ err:
> > > return ret;
> > > }
> > >
> > > +static int phytec_get_product_name(struct phytec_eeprom_data *data,
> > > + char *product)
> > phytec_eeprom_data should be const
> >
> > Please add documentation to this function
> >
> > > +{
> > > + struct phytec_api2_data *api2;
> > > + unsigned int ksp_no, som_type;
> > > + int len;
> > > +
> > > + if (!data)
> > > + data = &eeprom_data;
> > Why do you have this in here?
> Removed that.
> >
> >
> > > +
> > > + if (!data->valid || data->payload.api_rev < PHYTEC_API_REV2)
> > > + return -EINVAL;
> > > +
> > > + api2 = &data->payload.data.data_api2;
> > > +
> > > + if (api2->som_type > 1 && api2->som_type <= 3) {
> > > + ksp_no = (api2->ksp_no << 8) | api2->som_no;
> > > + len = snprintf(product, PHYTEC_PRODUCT_NAME_LEN, "%s-%04u",
> > > + phytec_som_type_str[api2->som_type], ksp_no);
> > > + if (len != 8)
> > Please use a variable here. You used snprintf, so you probably the value you expect here should
> > be
> > connected to the value you passed to the function.
> Added defines for them.
> >
> > > + return -1;
> > Use error code?
> ack.
> >
> > > + return 0;
> > > + }
> > > +
> > > + switch (api2->som_type) {
> > I think some explanation/documentation on how som_type's value is determined would be useful.
> >
> > > + case 0:
> > > + som_type = api2->som_type;
> > > + break;
> > > + case 4:
> > > + som_type = 0;
> > > + break;
> > > + case 5:
> > > + som_type = 0;
> > > + break;
> > > + case 6:
> > > + som_type = 1;
> > > + break;
> > > + case 7:
> > > + som_type = 1;
> > > + break;
> > > + default:
> > > + pr_err("%s: Invalid SOM type: %i", __func__, api2->som_type);
> > > + return -EINVAL;
> > > + };
> > > +
> > > + len = snprintf(product, PHYTEC_PRODUCT_NAME_LEN, "%s-%03u",
> > > + phytec_som_type_str[som_type], api2->som_no);
> > > + if (len != 7)
> > > + return -1;
> > Same as above when you called snprintf.
> >
> >
> > > + return 0;
> > > +}
> > > +
> > > +static int phytec_get_part_number(struct phytec_eeprom_data *data,
> > > + char *part)
> > phytec_eeprom_data should be const
> >
> > Please add documentation to this function
> >
> > > +{
> > > + char product_name[PHYTEC_PRODUCT_NAME_LEN] = {'\0'};
> > nitpick: C99 should support {} as initializer.
> >
> > > + struct phytec_api2_data *api2;
> > > + unsigned int ksp_type;
> > > + int res, len;
> > > +
> > > + if (!data)
> > > + data = &eeprom_data;
> > Why is this here?
> >
> >
> > > +
> > > + if (!data->valid || data->payload.api_rev < PHYTEC_API_REV2)
> > > + return -EINVAL;
> > > +
> > > + api2 = &data->payload.data.data_api2;
> > > +
> > > + res = phytec_get_product_name(data, product_name);
> > > + if (res)
> > > + return res;
> > > +
> > > + if (api2->som_type <= 1) {
> > > + len = snprintf(part, PHYTEC_PART_NUMBER_LEN, "%s-%s.%s",
> > > + product_name, api2->opt, api2->bom_rev);
> > > + if (len < 11)
> > > + return -1;
> > Please use a variable instead of hardcoded number, see above.
> >
> > > + return 0;
> > > + }
> > > + if (api2->som_type <= 3) {
> > > + snprintf(part, PHYTEC_PART_NUMBER_LEN, "%s.%s", product_name,
> > > + api2->bom_rev);
> > > + if (len != 11)
> > > + return -1;
> > > + return 0;
> > > + }
> > > +
> > > + switch (api2->som_type) {
> > > + case 4:
> > > + ksp_type = 3;
> > > + break;
> > > + case 5:
> > > + ksp_type = 2;
> > > + break;
> > > + case 6:
> > > + ksp_type = 3;
> > > + break;
> > > + case 7:
> > > + ksp_type = 2;
> > > + break;
> > Please add some explanation here as well.
> >
> > > + default:
> > > + pr_err("%s: Invalid SOM type: %i", __func__, api2->som_type);
> > > + return -EINVAL;
> > > + };
> > > +
> > > + len = snprintf(part, PHYTEC_PART_NUMBER_LEN, "%s-%s%02u.%s",
> > > + product_name, phytec_som_type_str[ksp_type],
> > > + api2->ksp_no, api2->bom_rev);
> > > + if (len < 16)
> > > + return -1;
> > Same as for the other cases
> >
> > > +
> > > + return 0;
> > > +}
> > > +
> > > void __maybe_unused phytec_print_som_info(struct phytec_eeprom_data *data)
> > > {
> > > + char part_number[PHYTEC_PART_NUMBER_LEN] = {'\0'};
> > > struct phytec_api2_data *api2;
> > > char pcb_sub_rev;
> > > - unsigned int ksp_no, sub_som_type1, sub_som_type2;
> > > + int res;
> > >
> > > if (!data)
> > > data = &eeprom_data;
> > > @@ -289,50 +404,14 @@ void __maybe_unused phytec_print_som_info(struct phytec_eeprom_data
> > > *data)
> > > pcb_sub_rev = api2->pcb_sub_opt_rev & 0x0f;
> > > pcb_sub_rev = pcb_sub_rev ? ((pcb_sub_rev - 1) + 'a') : ' ';
> > >
> > > - /* print standard product string */
> > > - if (api2->som_type <= 1) {
> > > - printf("SoM: %s-%03u-%s.%s PCB rev: %u%c\n",
> > > - phytec_som_type_str[api2->som_type], api2->som_no,
> > > - api2->opt, api2->bom_rev, api2->pcb_rev, pcb_sub_rev);
> > > + res = phytec_get_part_number(data, part_number);
> > > + if (res)
> > > return;
> > > - }
> > > - /* print KSP/KSM string */
> > > - if (api2->som_type <= 3) {
> > > - ksp_no = (api2->ksp_no << 8) | api2->som_no;
> > > - printf("SoM: %s-%u ",
> > > - phytec_som_type_str[api2->som_type], ksp_no);
> > > - /* print standard product based KSP/KSM strings */
> > > - } else {
> > > - switch (api2->som_type) {
> > > - case 4:
> > > - sub_som_type1 = 0;
> > > - sub_som_type2 = 3;
> > > - break;
> > > - case 5:
> > > - sub_som_type1 = 0;
> > > - sub_som_type2 = 2;
> > > - break;
> > > - case 6:
> > > - sub_som_type1 = 1;
> > > - sub_som_type2 = 3;
> > > - break;
> > > - case 7:
> > > - sub_som_type1 = 1;
> > > - sub_som_type2 = 2;
> > > - break;
> > > - default:
> > > - pr_err("%s: Invalid SoM type: %i", __func__, api2->som_type);
> > > - return;
> > > - };
> > > -
> > > - printf("SoM: %s-%03u-%s-%03u ",
> > > - phytec_som_type_str[sub_som_type1],
> > > - api2->som_no, phytec_som_type_str[sub_som_type2],
> > > - api2->ksp_no);
> > > - }
> > >
> > > - printf("Option: %s BOM rev: %s PCB rev: %u%c\n", api2->opt,
> > > - api2->bom_rev, api2->pcb_rev, pcb_sub_rev);
> > > + printf("SOM: %s\n", part_number);
> > > + printf("PCB Rev.: %u%c\n", api2->pcb_rev, pcb_sub_rev);
> > > + if (api2->som_type > 1)
> > > + printf("Options: %s\n", api2->opt);
> > You are changing the way infos are printed. Please put this in the commit description! These are
> > hidden changes.
> > Otherwise, make a separate patch where you change this.
> >
> > > }
> > >
> > > char * __maybe_unused phytec_get_opt(struct phytec_eeprom_data *data)
> > > @@ -379,6 +458,37 @@ u8 __maybe_unused phytec_get_som_type(struct phytec_eeprom_data *data)
> > > return data->payload.data.data_api2.som_type;
> > > }
> > >
> > > +#if IS_ENABLED(CONFIG_OF_LIBFDT)
> > > +int phytec_ft_board_fixup(struct phytec_eeprom_data *data, void *blob)
> > > +{
> > > + char product_name[PHYTEC_PRODUCT_NAME_LEN] = {'\0'};
> > > + char part_number[PHYTEC_PART_NUMBER_LEN] = {'\0'};
> > > + int res;
> > > +
> > > + if (!data)
> > > + data = &eeprom_data;
> > > +
> > > + if (!data->valid || data->payload.api_rev < PHYTEC_API_REV2)
> > > + return -EINVAL;
> > > +
> > > + res = phytec_get_product_name(data, product_name);
> > > + if (res)
> > > + return res;
> > > +
> > > + fdt_setprop(blob, 0, "phytec,som-product-name", product_name,
> > > + strlen(product_name) + 1);
> > > +
> > > + res = phytec_get_part_number(data, part_number);
> > > + if (res)
> > > + return res;
> > > +
> > > + fdt_setprop(blob, 0, "phytec,som-part-number", part_number,
> > > + strlen(part_number) + 1);
> > > +
> > > + return 0;
> > > +}
> > > +#endif /* IS_ENABLED(CONFIG_OF_LIBFDT) */
> > > +
> > > #if IS_ENABLED(CONFIG_CMD_EXTENSION)
> > > struct extension *phytec_add_extension(const char *name, const char *overlay,
> > > const char *other)
> > > @@ -458,6 +568,12 @@ inline struct phytec_api3_element * __maybe_unused
> > > return NULL;
> > > }
> > >
> > > +#if IS_ENABLED(CONFIG_OF_LIBFDT)
> > > +inline int phytec_ft_board_fixup(struct phytec_eeprom_data *data, void *blob)
> > > +{
> > > + return 0;
> > > +}
> > > +#endif /* IS_ENABLED(CONFIG_OF_LIBFDT) */
> > > #if IS_ENABLED(CONFIG_CMD_EXTENSION)
> > > inline struct extension *phytec_add_extension(const char *name,
> > > const char *overlay,
> > > diff --git a/board/phytec/common/phytec_som_detection.h
> > > b/board/phytec/common/phytec_som_detection.h
> > > index 5e35a13cb21..5b0ff9b0c89 100644
> > > --- a/board/phytec/common/phytec_som_detection.h
> > > +++ b/board/phytec/common/phytec_som_detection.h
> > > @@ -8,6 +8,7 @@
> > > #define _PHYTEC_SOM_DETECTION_H
> > >
> > > #include "phytec_som_detection_blocks.h"
> > > +#include <fdtdec.h>
> > >
> > > #define PHYTEC_MAX_OPTIONS 17
> > > #define PHYTEC_EEPROM_INVAL 0xff
> > > @@ -17,6 +18,9 @@
> > > #define PHYTEC_GET_OPTION(option) \
> > > (((option) > '9') ? (option) - 'A' + 10 : (option) - '0')
> > >
> > > +#define PHYTEC_PRODUCT_NAME_LEN 8 + 1
> > > +#define PHYTEC_PART_NUMBER_LEN PHYTEC_PRODUCT_NAME_LEN + 14 + 1
> > > +
> > Do you add 1 here to account for the null byte in the string?
> > If so, please do not do that here, instead use PHYTEC_PRODUCT_NAME_LEN +1 in the snprintf
> > function
> > calls. Also, defining the PHYTEC_PART_NUMBER_LEN based on PHYTEC_PRODUCT_NAME_LEN now becomes
> > "inconsistent" since you would need to omit the +1 for the null byte you added before.
>
> Moved that to the snprintf calls.
>
> Thanks,
> Daniel
>
> >
> > Yannic
> >
> > > enum {
> > > PHYTEC_API_REV0 = 0,
> > > PHYTEC_API_REV1,
> > > @@ -86,6 +90,9 @@ void __maybe_unused phytec_print_som_info(struct phytec_eeprom_data *data);
> > > char * __maybe_unused phytec_get_opt(struct phytec_eeprom_data *data);
> > > u8 __maybe_unused phytec_get_rev(struct phytec_eeprom_data *data);
> > > u8 __maybe_unused phytec_get_som_type(struct phytec_eeprom_data *data);
> > > +#if IS_ENABLED(CONFIG_OF_LIBFDT)
> > > +int phytec_ft_board_fixup(struct phytec_eeprom_data *data, void *blob);
> > > +#endif /* IS_ENABLED(CONFIG_OF_LIBFDT) */
> > >
> > > #if IS_ENABLED(CONFIG_CMD_EXTENSION)
> > > struct extension *phytec_add_extension(const char *name, const char *overlay,
More information about the U-Boot
mailing list