Hi James, Andrew, Thierry and all,

On 24 Nov 2014, at 18:19, Andrew Bresticker [off-list ref] wrote:

quoted

quoted

quoted

quoted

quoted

+     val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
+     val |= BIT(pwm->hwpwm);
+     img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
+
+     regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL,
+                        CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
+                        CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0);

quoted

This smells like pinmux and should probably be a separate driver.

Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio)
and call the pin muxing driver APIs from here.

Please correct me if my understanding is wrong?

I don't think you need to call the pinmux API from here. Rather I'll
assume that the muxing is fixed on a given board, so this configuration
would be part of the static board-level pinmux so it will automatically
be applied at boot time.

The issue here is that this register does not belong to the
pinmux/pinctrl block on this SoC and instead is part of the
"peripheral control" block which primarily provides system interface
gate clocks and resets (for which I was planning to write a clock
provider driver), but also has a bunch of control registers for
various IP blocks thrown together (like this one) which we're using
syscon to access.  I'm not sure it makes sense to create a pinmux
driver for these 4 bits, but I'll defer to others on that...

Another option would be to have the main pinctrl driver for Pistachio
control these bits, but that doesn't seem quite right since these bits
are completely separate from the pinctrl block.

I would think the pinctrl driver should deal with this - yes it's a separate set of registers,
but the config is expected to be static and can be represented in the DT.

James.

I have discussed with the hardware team one more time today and discovered that
the value we configure to CR_PERIP_PWM_PDM_CONTROL not only configures PWM/PDM
output mux, but also enables control signal for PDM blocks.

As an example, if the PDM user wants to enable PDM block 2, he needs to set bit 8 to 1 in the
register CR_PERIP_PWM_PDM_CONTROL. Please also note that setting bit 8  to 1 not only
configures the output PWM/PDM mux, but also enables control signal for PDM block 2.
He needs access to this register from the PDM driver at least to configure (enable/disable)
PDM control signal.

And also because of the reasons explained by Andrew above, I think it is better to use syscon
(instead of handling it in Pinctrl driver) to access this register both in PWM and PDM drivers.

Please let us know comments and suggestions.

Thanks and regards,
Naidu.