On Sat, Apr 28, 2018 at 04:56:07PM +0300, Alexander Monakov wrote:
> Low-level controller enable function __i2c_dw_enable is overloaded to
> also handle disabling. What's worse, even though the documentation
> requires polling the IC_ENABLE_STATUS register when disabling, this
> is not done: polling needs to be requested specifically by calling
> __i2c_dw_enable_and_wait, which can also poll on enabling, but that
> doesn't work if the IC_ENABLE_STATUS register is not implemented.
> This is quite confusing if not in fact backwards.
>
> Especially since the documentation says that disabling should be
> followed by polling, the driver should be using a separate function
> where it does one-shot disables to make the optimization stand out.
>
> This refactors the two functions so that requested status is given
> in the name rather than in a boolean argument. Specifically:
>
> - __i2c_dw_enable: enable without polling (in accordance with docs)
> - __i2c_dw_disable: disable and do poll (also as suggested by docs)
> - __i2c_dw_disable_nowait: disable without polling (Linux-specific)
>
> No functional change.
>
> Cc: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
> Cc: Wolfram Sang <wsa@xxxxxxxxxxxxx>
> Signed-off-by: Alexander Monakov <amonakov@xxxxxxxxx>
I like the explicit enable/disable, so applied to for-next, thanks!
I improved the blank lines in this section, though (checkpatch --strict
warned about it):
> +static inline void __i2c_dw_enable(struct dw_i2c_dev *dev)
> +{
> + dw_writel(dev, 1, DW_IC_ENABLE);
> +}
> +static inline void __i2c_dw_disable_nowait(struct dw_i2c_dev *dev)
> +{
> + dw_writel(dev, 0, DW_IC_ENABLE);
> +}
> +void __i2c_dw_disable(struct dw_i2c_dev *dev);
> +
> +
Attachment:
signature.asc
Description: PGP signature
