On Tue, 9 Jul 2019 14:01:03 +0300 "Hawa, Hanna" <hhhawa@xxxxxxxxxx> wrote: > On 7/9/2019 12:32 PM, Jonathan Cameron wrote: > >> Signed-off-by: Hanna Hawa<hhhawa@xxxxxxxxxx> > > A quick drive by review as I was feeling curious. > > > > Just a couple of trivial queries and observation on the fact it > > might be useful to add a few devm managed functions to cut down > > on edac driver boilerplate. > > > > Thanks, > > > > Jonathan > > > >> +#define ARM_CA57_CPUMERRSR_VALID GENMASK(31, 31) > > For a single bit it's common to use BIT(31) rather than GENMASK to make > > it explicit. > > Will fix. > > > > > > >> + edac_dev->mod_name = dev_name(dev); > > I'd admit I'm not that familiar with edac, but seems odd that a > > module name field would have the dev_name. > > Will fix when I got more inputs. > > > > >> + edac_device_free_ctl_info(edac_dev); > > More a passing observation than a suggestion for this driver, but if there was > > ever a place where it looked like a couple of devm_ allocation functions would > > be useful, this is it;) > > > > edac_dev = devm_device_alloc_ctrl_info(dev, ...) > > ... > > devm_edac_device_add_device(dev, ...) > > I agree. > I can implement the devm_* functions in separate patches as this is not > related to my patches (and not to delay this patches). > Great. Jonathan > > > >
