Re: [PATCH v2] mtd: nand: print full chip ID

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 30-07-15 13:52, Michal Suchanek wrote:
> Full chip ID is printed so user has data to paste from syslog in case
> of chip misidentification.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>
> ---
> v2:
>
>   - remove superfluous hunk
>   - join printed lines
> ---
>   drivers/mtd/nand/nand_base.c | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index ceb68ca..7c10e06 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3631,6 +3631,22 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
>   }
>
>   /*
> + * Print full detail of chip ID read from chip.
> + */
> +static void nand_print_chip_info(int maf_id, int dev_id, u8 id_data[8])
> +{
> +	u8 delim[8] = { [0 ... 7] = ',' };
> +
> +	/* Kernel inserts newline after every other printk so format in one go. */
> +	delim[7] = ' ';
> +	delim[nand_id_len(id_data, 8) - 1] = ';';
> +	pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x, ID data: 0x%02x%c 0x%02x%c 0x%02x%c 0x%02x%c 0x%02x%c 0x%02x%c 0x%02x%c 0x%02x%c\n",
> +		maf_id, dev_id,
> +		id_data[0], delim[0], id_data[1], delim[1], id_data[2], delim[2], id_data[3], delim[3],
> +		id_data[4], delim[4], id_data[5], delim[5], id_data[6], delim[6], id_data[7], delim[7]);

These lines are longer then 80 chars, that is allowed only for lines ending with "...",

Which is why the version of this patch I submitted upstream had:

+		id_data[0], delim[0], id_data[1], delim[1],
+		id_data[2], delim[2], id_data[3], delim[3],
+		id_data[4], delim[4], id_data[5], delim[5],
+		id_data[6], delim[6], id_data[7], delim[7]);

Other then that this version looks good to me, and I agree that having this
info as non pr_debug is quite useful so that if (variants of) boards show up
with a nand with an unknown nand id, but one which the auto-detect code
does handle, we will still know and we can add an entry to the id table
for the nand to enable extra features (higher speed, special read retry
logic). This is quite likely to happen with e.g. the popular mk802 hdmi
tv sticks which have a single dts file for all of them, but come with
different nand chips based on the exact production run.

Regards,

Hans


> +}
> +
> +/*
>    * Get the flash and manufacturer id and lookup if the type is supported.
>    */
>   static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
> @@ -3743,8 +3759,7 @@ ident_done:
>   		 * Check, if buswidth is correct. Hardware drivers should set
>   		 * chip correct!
>   		 */
> -		pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
> -			*maf_id, *dev_id);
> +		nand_print_chip_info(*maf_id, *dev_id, id_data);
>   		pr_info("%s %s\n", nand_manuf_ids[maf_idx].name, mtd->name);
>   		pr_warn("bus width %d instead %d bit\n",
>   			   (chip->options & NAND_BUSWIDTH_16) ? 16 : 8,
> @@ -3775,8 +3790,7 @@ ident_done:
>   	if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
>   		chip->cmdfunc = nand_command_lp;
>
> -	pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
> -		*maf_id, *dev_id);
> +	nand_print_chip_info(*maf_id, *dev_id, id_data);
>
>   	if (chip->onfi_version)
>   		pr_info("%s %s\n", nand_manuf_ids[maf_idx].name,
>