Re: [PATCH v3 08/10] iio: dac: ad3552r: extract common code (no changes in behavior intended)

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

> > > +int ad3552r_get_custom_gain(struct device *dev, struct fwnode_handle *child,
> > > +			    u8 *gs_p, u8 *gs_n, u16 *rfb, s16 *goffs)
> > > +{
> > > +	int err;
> > > +	u32 val;
> > > +	struct fwnode_handle *gain_child __free(fwnode_handle) =
> > > +				fwnode_get_named_child_node(child,  
> > One tab more than the line above is fine for cases like this and makes for
> > more readable code.
> >  
> Aligning with c then line comes to long. 
> I can offer, as in other drivers:
> 
> int ad3552r_get_custom_gain(struct device *dev, struct fwnode_handle *child,
> 			    u8 *gs_p, u8 *gs_n, u16 *rfb, s16 *goffs)
> {
> 	int err;
> 	u32 val;
> 	struct fwnode_handle *gain_child __free(fwnode_handle) =
> 		fwnode_get_named_child_node(child,
> 					    "custom-output-range-config");

That looks good to me

> 
> Also, do you prefer 80 or 100 as eol limit ?

Prefer 80, but not at the cost of readability

> 
>  
> > > +				"custom-output-range-config");  
> > 
> > Align this final parameter with c of child.
> >   
>

> > > +static int ad3552r_find_range(u16 id, s32 *vals)
> > > +{
> > > +	int i, len;
> > > +	const s32 (*ranges)[2];
> > > +
> > > +	if (id == AD3542R_ID) {  
> > 
> > This is already in your model_data. Use that not another lookup via
> > an ID enum.  The ID enum approach doesn't scale as we add more parts
> > as it scatters device specific code through the driver.
> >  
> 
> This function is only used internally to this common part.
>  
> >   
> > > +		len = ARRAY_SIZE(ad3542r_ch_ranges);
> > > +		ranges = ad3542r_ch_ranges;
> > > +	} else {
> > > +		len = ARRAY_SIZE(ad3552r_ch_ranges);
> > > +		ranges = ad3552r_ch_ranges;
> > > +	}
> > > +
> > > +	for (i = 0; i < len; i++)
> > > +		if (vals[0] == ranges[i][0] * 1000 &&
> > > +		    vals[1] == ranges[i][1] * 1000)
> > > +			return i;
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +int ad3552r_get_output_range(struct device *dev, enum ad3552r_id chip_id,
> > > +			     struct fwnode_handle *child, u32 *val)  
> > As above, don't pass the enum. Either pass the model_data or pass the
> > actual stuff you need which is the ranges array and size of that array.
> >  
> 
> Cannot pass model data, structures of the 2 drviers are different.
> If i pass arrays, the logic of deciding what array (checking the id)
> must be done in the drivers, but in this way, there will be less
> common code.

I'd prefer that to having an ID register look up in here.

Roughly speaking looking up by ID is almost always the wrong
way to go for long term scalability of a driver as more parts
are added.

Jonathan