Re: [PATCH v2 2/6] iio: frequency: adf41513: driver implementation

[Jonathan Cameron]

...

> > > +	cfg->ref_div2 = FIELD_GET(ADF41513_REG5_RDIV2_MSK,
> > > +				  st->regs_hw[ADF41513_REG5]);
> > > +	cfg->prescaler = FIELD_GET(ADF41513_REG5_PRESCALER_MSK,
> > > +				   st->regs_hw[ADF41513_REG5]);  
> > For cases like this I think keeping to 80 chars is hurting readability
> > and so it is fine to go a little higher. 
> > 	cfg->int_value = FIELD_GET(ADF41513_REG0_INT_MSK, st->regs_hw[ADF41513_REG0]);
> > 	cfg->frac1 = FIELD_GET(ADF41513_REG1_FRAC1_MSK, st->regs_hw[ADF41513_REG1]);
> > 	cfg->frac2 = FIELD_GET(ADF41513_REG3_FRAC2_MSK, st->regs_hw[ADF41513_REG3]);
> > 	cfg->mod2 = FIELD_GET(ADF41513_REG4_MOD2_MSK, st->regs_hw[ADF41513_REG4]);
> > 	cfg->r_counter = FIELD_GET(ADF41513_REG5_R_CNT_MSK, st->regs_hw[ADF41513_REG5]);
> > 	cfg->ref_doubler = FIELD_GET(ADF41513_REG5_REF_DOUBLER_MSK, st->regs_hw[ADF41513_REG5]);
> > 	cfg->ref_div2 = FIELD_GET(ADF41513_REG5_RDIV2_MSK, st->regs_hw[ADF41513_REG5]);
> > 	cfg->prescaler = FIELD_GET(ADF41513_REG5_PRESCALER_MSK,st->regs_hw[ADF41513_REG5]);
> > Is fine here. I'd also be fine with wrapping the ref_doubler line as it's rather
> > longer than the others.  
> 
> ack

Small kernel development process thing.  For efficiency general rule is
don't bother replying at all to suggestions you accept. It just adds noise.
Much better to just crop that chunk of the reply out so we can
rapidly see where more discussion is needed.

>  
> > > +
> > > +	/* calculate pfd frequency */

...

> > > +static int adf41513_parse_fw(struct adf41513_state *st)
> > > +{
> > > +	struct device *dev = &st->spi->dev;
> > > +	int ret;
> > > +	u32 tmp;
> > > +	u32 cp_resistance;
> > > +	u32 cp_current;  
> > Where you have set of variables of same type and grouping doesn't hurt
> > readability, declare them all on one line.
> > 
> > 	u32 tmp, cp_resistance, cp_current;  
> 
> ack
> 
> > I'll not repeat comments I made on the dt-binding in here but I'd expect
> > this code to change somewhat in response to those.  
> 
> understood. for now, will use -mhz for the power-up frequency, but I will
> see how the discussion follows.

That one is indeed non obvious and so (assuming I didn't miss anything) this
is the one block where there was a non trivial comment in your reply so
ideally would have been only bit there.

I get grumpy about this every now and then and this time you get to be
the target!

Jonathan