Hi Boris,
Thanks for the review.
> > + u64 addr = 0;
> > + u64 data = 0;
> > + u32 val_h = 0;
> > + u32 val_l, id, synd;
>
> u32 val_h = 0, val_l, id, synd;
> u64 addr = 0, data = 0;
>
> Also, for all your functions:
>
> The EDAC tree preferred ordering of variable declarations at the
> beginning of a function is reverse fir tree order::
>
> struct long_struct_name *descriptive_name;
> unsigned long foo, bar;
> unsigned int tmp;
> int ret;
>
> The above is faster to parse than the reverse ordering::
>
> int ret;
> unsigned int tmp;
> unsigned long foo, bar;
> struct long_struct_name *descriptive_name;
>
> And even more so than random ordering::
>
> unsigned long foo, bar;
> int ret;
> struct long_struct_name *descriptive_name;
> unsigned int tmp;
I'll check all functions and follow this order.
> > + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1, addr >> PAGE_SHIFT,
> > + addr & ~PAGE_MASK, synd, 0, 0, -1, priv->message,
>
> No need for that linebreak with the trailing piece.
>
> > + "");
> > + u64 addr = 0;
> > + u64 data = 0;
> > + u32 val_h = 0;
> > + u32 val_l, id, synd;
>
> As above.
Check.
> > + regmap_read(npcm_regmap, pdata->ctl_int_status, &status);
> > + if (status & pdata->int_status_ce_mask) {
> > + handle_ce(mci);
> > +
> > + /* acknowledge the CE interrupt */
> > + regmap_write(npcm_regmap, pdata->ctl_int_ack,
> > + pdata->int_ack_ce_mask);
> > + return IRQ_HANDLED;
> > + } else if (status & pdata->int_status_ue_mask) {
> > + handle_ue(mci);
> > +
> > + /* acknowledge the UE interrupt */
> > + regmap_write(npcm_regmap, pdata->ctl_int_ack,
> > + pdata->int_ack_ue_mask);
> > + return IRQ_HANDLED;
> > + }
>
> WARN_ON_ONCE(1);
>
> to catch weird cases.
OK.
> > + /* write syndrome to XOR_CHECK_BITS */
> > + if (priv->error_type == 0) {
>
> if (priv->error_type == ERROR_TYPE_CORRECTABLE
>
> Use defines. Below too.
>
> > + if (priv->location == 0 && priv->bit > 63) {
Will add defines.
> > + edac_printk(KERN_INFO, EDAC_MOD_NAME,
> > + "data bit should not exceed 63\n");
>
> "data bit should not exceed 63 (%d)\n", priv->bit...)
>
> Below too.
>
> > + return count;
> > + }
> > +
> > + if (priv->location == 1 && priv->bit > 7) {
> > + edac_printk(KERN_INFO, EDAC_MOD_NAME,
> > + "checkcode bit should not exceed 7\n");
OK.
> > + syndrome = priv->location ? 1 << priv->bit :
> > + data_synd[priv->bit];
>
> syndrome = priv->location ? 1 << priv->bit
> : data_synd[priv->bit];
Just to confirm the indentation, is it right as follows?
syndrome = priv->location ? 1 << priv->bit
: data_synd[priv->bit];
And I was wondering if I should just remove the line break and let it stick out?
> I'd find it helpful if there were a short recipe in a comment here
> explaining how the injection should be done...
>
> > +static void setup_debugfs(struct mem_ctl_info *mci)
> > +{
OK, will add some injection examples here.
> > + regmap_update_bits(npcm_regmap, pdata->ctl_ecc_en, pdata->ecc_en_mask,
> > + 0);
>
> You have a bunch of those things: the 80 cols rule is not a rigid and a
> static one - you should rather apply common sense. As in:
>
> Does it make sense to have this ugly linebreak with only the 0 argument there?
>
> regmap_update_bits(npcm_regmap, pdata->ctl_ecc_en, pdata->ecc_en_mask,
> 0);
>
> or should I simply let it stick out:
>
> regmap_update_bits(npcm_regmap, pdata->ctl_ecc_en, pdata->ecc_en_mask, 0);
>
> and have much more readable code?
>
> Please apply common sense to all your linebreaks above.
Thanks for the guide.
> > + edac_mc_del_mc(&pdev->dev);
> > + edac_mc_free(mci);
>
> <--- What happens if someone tries to inject errors right at this
> moment, when you've freed the mci?
>
> Hint: you should destroy resources in the opposite order you've
> allocated them.
Understand. I'll correct the destroy order.
Regards,
Marvin
