Re: [PATCH v2 2/2] iio: dac: dac8163: Add driver for DAC8163

[Siratul Islam]

On July 27, 2026 12:18:33 AM GMT+06:00, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>On Sun, 26 Jul 2026 17:01:51 +0000
>"Siratul Islam" <siratul.islam@xxxxxxxxx> wrote:
>
>> July 26, 2026 at 8:10 PM, "Lukas" <lukas.metz@xxxxxxx> wrote:
>> 
>> 
>> > 
>> > On Wed, Jul 08, 2026 at 09:58:13PM +0600, Siratul Islam wrote:
>> >   
>> > > 
>> > > +
>> > >  + const struct reg_default reg_defaults[] = {
>> > >  + {
>> > >  + .reg = CMD_SET(CMD_WRITE_UPDATE, 0),
>> > >  + .def = info->default_output_reg,
>> > >  + },
>> > >  + {
>> > >  + .reg = CMD_SET(CMD_WRITE_UPDATE, 1),
>> > >  + .def = info->default_output_reg,
>> > >  + },
>> > >  + };
>> > >  +
>> > >  + const struct regmap_config regmap_config = {
>> > >  + .reg_bits = 8,
>> > >  + .val_bits = 16,
>> > >  +
>> > >  + .max_register = CMD_REF << 3,
>> > >  + .cache_type = REGCACHE_MAPLE,
>> > >  +
>> > >  + .volatile_reg = dac8163_reg_false,
>> > >  +
>> > >  + .reg_defaults = reg_defaults,
>> > >  + .num_reg_defaults = ARRAY_SIZE(reg_defaults),
>> > >  + };
>> > >  Why are these defined inside probe? Should be defined outside the function.
>> > >   
>> > The default values after reset are different for the compatible devices
>> > (0 for the dacxxx2 devices and mid-scale for the dacxxx3 devices).
>> > So these are runtime dependent. The alternative would be to have two
>> > regmap_configs for the two device types and add these to the chip_info.
>> > Or do you have another suggestion?  
>> You don't need two regmap_configs. The way I would do it is, move the "regmap_config" out of probe and name it something 
>> like "dac8563_regmap_config". And in chip info struct, replace "default_output_reg" with "reg_defaults[]". 
>> Then for each "dacxxxx_chip_info" define the appropriate "reg_defaults" configuration.
>> 
>> This way you can do
>> 
>> struct regmap_config regmap_config = dac8563_regmap_config;
>> ...
>> regmap_config.reg_defaults = info->reg_defaults;
>> regmap_config.num_reg_defaults = ARRAY_SIZE(info->reg_defaults);
>> 
>> A bit of work, but it would ensure most of the work is done at compile time,
>> and we avoid reconstructing these sructs everytime probe runs.
>
>As this has come up, I'd favour just having two static const regmap_config structures
>and picking between them.  Obviously I haven't checked but I'd guess that any
>overhead of extra text data for those is not that different to the extra code of the current
>approach.  Generally if you can make this sort of thing data rather than code it
>is a net gain.
>
>Jonathan
>
Fair enough. I was going for minimizing duplication. But if you are open to duplicate structs, it makes it simpler and it may even read easier too.
>> 
...
>
--
Best regards,
Sirat