Re: [PATCH v2 3/5] clk: lochnagar: Add support for the Cirrus Logic Lochnagar

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]

On Thu, Oct 11, 2018 at 12:00:46AM -0700, Stephen Boyd wrote:
> Quoting Charles Keepax (2018-10-08 06:25:40)

> > +struct lochnagar_regmap_clk {
> > +       unsigned int cfg_reg;
> > +       unsigned int ena_mask;
> > +       unsigned int dir_mask;
> > +
> > +       unsigned int src_reg;
> > +       unsigned int src_mask;

> Are these 32 bits or 16 bits or 8 bits? Please use a u32/u16/u8 so we
> know the register width.

Note that regmap always uses unsigned int to represent registers
regardless of the underlying physical register width, this can be an
issue on reads since we pass the destination for the read in by address.

> > +struct lochnagar_clk_priv {
> > +       struct device *dev;
> > +       struct lochnagar *lochnagar;

> Is this used for anything besides getting the regmap? Can you get the
> pointer to the parent in probe and use that to get the regmap pointer
> from dev_get_remap() and also use the of_node of the parent to register

dev_get_regmap() is pretty expensive, I'd not advise using it in
anything that might approximate a fast path.  It's kind of debatable
when I2C gets involved but still feels wrong.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]