Re: [PATCH 01/28] mfd: Add Microchip ZL3073x support

[Andy Shevchenko <andy@kernel.org>]

On Wed, Apr 09, 2025 at 08:40:22AM +0200, Ivan Vecera wrote:
> On 07. 04. 25 8:05 odp., Andy Shevchenko wrote:
> > On Mon, Apr 07, 2025 at 07:28:28PM +0200, Ivan Vecera wrote:

...

> > > +/*
> > > + * Regmap ranges
> > > + */
> > > +#define ZL3073x_PAGE_SIZE	128
> > > +#define ZL3073x_NUM_PAGES	16
> > > +#define ZL3073x_PAGE_SEL	0x7F
> > > +
> > > +static const struct regmap_range_cfg zl3073x_regmap_ranges[] = {
> > > +	{
> > > +		.range_min	= 0,
> > 
> > Are you sure you get the idea of virtual address pages here?
> 
> What is wrong here?
> 
> I have a device that uses 7-bit addresses and have 16 register pages.
> Each pages is from 0x00-0x7f and register 0x7f is used as page selector
> where bits 0-3 select the page.

The problem is that you overlap virtual page over the real one (the main one).

The drivers you mentioned in v2 discussions most likely are also buggy.
As I implied in the above question the developers hardly get the regmap ranges
right. It took me quite a while to see the issue, so it's not particularly your
fault.

> > > +		.range_max	= ZL3073x_NUM_PAGES * ZL3073x_PAGE_SIZE,
> > > +		.selector_reg	= ZL3073x_PAGE_SEL,
> > > +		.selector_mask	= GENMASK(3, 0),
> > > +		.selector_shift	= 0,
> > > +		.window_start	= 0,
> > > +		.window_len	= ZL3073x_PAGE_SIZE,
> > > +	},
> > > +};

...

> > > +	zldev->regmap = devm_regmap_init_i2c(client,
> > > +					     zl3073x_get_regmap_config());
> > 
> > It's perfectly a single line.
> 
> I tried to follow strictly 80 chars/line. Will fix.

Yes, but in some cases when it gains in readability it is allowed to use longer
lines even if one sticks with stricter 80 characters limit.

-- 
With Best Regards,
Andy Shevchenko