Re: [PATCH/RFC v2] mmc: host: renesas_sdhi: Refactor of_device_id.data

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 1537 bytes --]

Hi Shimoda-san,

thank you for taking care of this!

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reported-by: kernel test robot <lkp@intel.com>

I think this Rep-by can go. Test bot mentioned one build error of v1,
but it didn't report that we should refactor this code.

>  -- should I make step-by-step patches to ease review?

I'd think this is good enough.

>  -- should I rename the current renesas_sdhi_of_data (e.g. renesas_sdhi_param)?
>     (renesas_sdhi_of_data and renesas_sdhi_of_data_with_quirks seem strange
>      a little?)

Yes, this may be a little better. I'd think the current naming is good
enough, though.

>  - I tested this patch on r8a77951 (ES3.0), r8a77960 (ES1.0) and r8a77965.

I also tested this on r8a77965.

>  - Also I tested this patch on r8a7791 [2].

I'll check r8a7790 later.

>  static const struct renesas_sdhi_quirks sdhi_quirks_r8a7796_es13 = {
>  	.hs400_4taps = true,
>  	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
>  	.hs400_calib_table = r8a7796_es13_calib_table,
>  };

You leave the quirk handling of different ES versions still in
renesas_sdhi_core. I'd think this should also be moved to
renesas_sdhi_internal_dmac? Then we have all the handling in one place.

> +static const struct renesas_sdhi_of_data_with_quirks of_r8a7795_compatible = {
> +	.of_data	= &of_data_rcar_gen3,
> +	.quirks		= &sdhi_quirks_bad_taps2367,
> +};

I'd suggest only a single space (and not a tabulator) before the "=".

Rest looks good so far!

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]