[coreboot-gerrit] Change in coreboot[master]: soc/intel/common/block:[WIP] Add GPIO common code

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/18917 )

Change subject: soc/intel/common/block:[WIP] Add GPIO common code
......................................................................


Patch Set 23:

(29 comments)

https://review.coreboot.org/#/c/18917/23/src/soc/intel/common/block/gpio/gpio.c
File src/soc/intel/common/block/gpio/gpio.c:

PS23, Line 35: gpio_get_community
%s and use __func__


PS23, Line 39: int
Shouldn't this be uint16_t? pcr_read32 expects offset as a uint16_t. Same with pcr_write32 below.


Line 44: 
extra blank line


PS23, Line 46: 		
too many tabs?


PS23, Line 50: return
You don't need this.


PS23, Line 58: -
space around -


PS23, Line 58: /
space around /


PS23, Line 64: reg_base +
             : 			(index * reg_size)
You can save this in a variable reg_offset = reg_base + index * reg_size;

That way you don't need to calculate it again.


PS23, Line 66: return
Why do you need an explicit return?


PS23, Line 72: int		index;
There is a mix of two/three different ways for declaring variables in this file:

<datatype><tab><variable_name>
<datatype><tab><tab><variable_name>
<datatype><space><variable_name>

Can you please use a single consistent way?


PS23, Line 74: -
space around -


PS23, Line 74: /
space around /


PS23, Line 77:  
extra space


PS23, Line 79:  
extra space


PS23, Line 79:  
extra space


Line 81: }
same comments as gpio_set_bit above.


PS23, Line 87:  = 0
There is no need to set this to 0. It is set before using in the for loop.


PS23, Line 93: community_count
nit: All other functions in this file seem to refer to this as num_communities. It might help to use the same name in this function as well.


Line 121: 	for (i = 0; i < num_communities; i++) {
shouldn't index be set to:
index = comm->gpi_offset?


PS23, Line 135: print_gpi_status
Would it help if the print_gpi_status was moved in the loop above where sts->grp[index] &= smi_en; is done? Then you can avoid 2 loops in print_gpi_sts and just loop over bit set.


PS23, Line 162: soc_gpio_configure_itss
Might be better to name this something like: gpio_soc_configure_itss, so that it still starts with the same prefix "gpio" and has "soc" in the name to indicate the SoC has to implement this.


PS23, Line 171: soc_gpio_get_pad_config_offset
Same comment as above.


PS23, Line 189: printk(BIOS_DEBUG, "gpio_padcfg [0x%02x, %d] DW0 [0x%08x : 0x%08x] "
              : 	"DW1 [0x%08x : 0x%08x]\n",
              : 		comm->port,
              : 		(cfg->pad - comm->first_pad),
              : 		pad_conf0,/* old value */
              : 		cfg->pad_config0,			/* new value */
              : 		pad_conf1,/* old value */
              : 		cfg->pad_config1);			/* new value */
Should this be hidden behind a config so that it doesn't fill up the bios logs?


PS23, Line 212: if
space after if


PS23, Line 215: if
space after if


PS23, Line 216: {
this should go to the previous line.


PS23, Line 232: /* Calculate Address of DW0 register for given GPIO
              : 	 * pad - GPIO number
              : 	 * returns - address of GPIO
              : 	 */
this should be put before the function


PS23, Line 245: /* Get the port id of given pad
              : 	 * pad - GPIO number
              : 	 * returns - given pad port id
              : 	 */
same comment as above.


PS23, Line 256: DN_20K
Shouldn't this be accepted as an input?


-- 
To view, visit https://review.coreboot.org/18917
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a1d56df46668bfb08206ca4a99202db5cd1da7c
Gerrit-PatchSet: 23
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Hannah Williams <hannah.williams at intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra at intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov at gmail.com>
Gerrit-Reviewer: Barnali Sarkar <barnali.sarkar at intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik at intel.com>
Gerrit-Reviewer: Brenton Dong <brenton.m.dong at intel.com>
Gerrit-Reviewer: Dhaval Sharma <dhaval.v.sharma at intel.com>
Gerrit-Reviewer: Divya Chellappa <divya.chellappa at intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Han Lim Ng <nhlhanlim93 at gmail.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams at intel.com>
Gerrit-Reviewer: Lijian Zhao <lijian.zhao at intel.com>
Gerrit-Reviewer: Marc Herbert <marc.herbert at intel.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi at intel.com>
Gerrit-Reviewer: Shaunak Saha <shaunak.saha at intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar at intel.com>
Gerrit-Reviewer: V Sowmya <v.sowmya at intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Reviewer: sowmya v <sowmyav235 at gmail.com>
Gerrit-HasComments: Yes