Alexander,

Sorry for being a bother.  See below for two comments.

Cheers,
Phil.

> On 26 Feb 2018, at 18:42, Alexander Kochetkov <al.koc...@gmail.com> wrote:
> 
> rk3066 and rk3188 has two I2C controller implementations.
> Current I2C driver wan't work with legacy implementation.
> Switching between controllers is performed using a bit inside
> GFR_SOC_CON1 register. The bit setting is performed by pinctrl
> driver. The patch ask pinctrl to do settings.
> 
> Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
> ---
> drivers/i2c/rk_i2c.c |   90 +++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 85 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c
> index 332280c..ce6b441 100644
> --- a/drivers/i2c/rk_i2c.c
> +++ b/drivers/i2c/rk_i2c.c
> @@ -34,6 +34,18 @@ struct rk_i2c {
>       unsigned int speed;
> };
> 
> +enum {
> +     RK_I2C_LEGACY,
> +     RK_I2C_NEW,
> +};
> +
> +/**
> + * @controller_type: i2c controller type
> + */
> +struct rk_i2c_soc_data {
> +     int controller_type;
> +};
> +
> static inline void rk_i2c_get_div(int div, int *divh, int *divl)
> {
>       *divl = div / 2;
> @@ -381,9 +393,38 @@ static int rockchip_i2c_ofdata_to_platdata(struct 
> udevice *bus)
> static int rockchip_i2c_probe(struct udevice *bus)
> {
>       struct rk_i2c *priv = dev_get_priv(bus);
> +     struct rk_i2c_soc_data *soc_data;
> +     struct udevice *pinctrl;
> +     int bus_nr;
> +     int ret;
> 
>       priv->regs = dev_read_addr_ptr(bus);
> 
> +     soc_data = (struct rk_i2c_soc_data*)dev_get_driver_data(bus);
> +
> +     if (soc_data->controller_type == RK_I2C_LEGACY) {

This should be safe both from legacy and new variants.

> +             ret = dev_read_alias_seq(bus, &bus_nr);
> +             if (ret < 0) {
> +                     debug("%s: Could not get alias for %s: %d\n",
> +                      __func__, bus->name, ret);
> +                     return ret;
> +             }
> +
> +             ret = uclass_get_device(UCLASS_PINCTRL, 0, &pinctrl);
> +             if (ret) {
> +                     debug("%s: Cannot find pinctrl device\n", __func__);
> +                     return ret;
> +             }
> +
> +             /* pinctrl will switch I2C to new type */
> +             ret = pinctrl_request_noflags(pinctrl, PERIPH_ID_I2C0 + bus_nr);
> +             if (ret) {
> +                     debug("%s: Failed to switch I2C to new type %s: %d\n",
> +                             __func__, bus->name, ret);
> +                     return ret;
> +             }

I wonder if this is really necessary (or if there’s something going wrong in the
device framework)…  The way I always understood our device framework was
that if there’s a pinctrl associated with the DTS node, then it should get 
processed
automatically during probing.

See the following in drivers/core/device.c:

        /*
         * Process pinctrl for everything except the root device, and
         * continue regardless of the result of pinctrl. Don't process pinctrl
         * settings for pinctrl devices since the device may not yet be
         * probed.
         */
        if (dev->parent && device_get_uclass_id(dev) != UCLASS_PINCTRL)
                pinctrl_select_state(dev, "default");


> +     }
> +
>       return 0;
> }
> 
> @@ -392,12 +433,51 @@ static const struct dm_i2c_ops rockchip_i2c_ops = {
>       .set_bus_speed  = rockchip_i2c_set_bus_speed,
> };
> 
> +static const struct rk_i2c_soc_data rk3066_soc_data = {
> +     .controller_type = RK_I2C_LEGACY,
> +};
> +
> +static const struct rk_i2c_soc_data rk3188_soc_data = {
> +     .controller_type = RK_I2C_LEGACY,
> +};
> +
> +static const struct rk_i2c_soc_data rk3228_soc_data = {
> +     .controller_type = RK_I2C_NEW,
> +};
> +
> +static const struct rk_i2c_soc_data rk3288_soc_data = {
> +     .controller_type = RK_I2C_NEW,
> +};
> +
> +static const struct rk_i2c_soc_data rk3328_soc_data = {
> +     .controller_type = RK_I2C_NEW,
> +};
> +
> +static const struct rk_i2c_soc_data rk3399_soc_data = {
> +     .controller_type = RK_I2C_NEW,
> +};
> +
> static const struct udevice_id rockchip_i2c_ids[] = {
> -     { .compatible = "rockchip,rk3066-i2c" },
> -     { .compatible = "rockchip,rk3188-i2c" },
> -     { .compatible = "rockchip,rk3288-i2c" },
> -     { .compatible = "rockchip,rk3328-i2c" },
> -     { .compatible = "rockchip,rk3399-i2c" },
> +     {
> +             .compatible = "rockchip,rk3066-i2c",
> +             .data = (ulong)&rk3066_soc_data,
> +     },
> +     {
> +             .compatible = "rockchip,rk3188-i2c",
> +             .data = (ulong)&rk3188_soc_data,
> +     },
> +     {
> +             .compatible = "rockchip,rk3288-i2c",
> +             .data = (ulong)&rk3288_soc_data,
> +     },
> +     {
> +             .compatible = "rockchip,rk3328-i2c",
> +             .data = (ulong)&rk3328_soc_data,
> +     },
> +     {
> +             .compatible = "rockchip,rk3399-i2c",
> +             .data = (ulong)&rk3399_soc_data,
> +     },
>       { }
> };
> 
> -- 
> 1.7.9.5
> 

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to