Hello,
On Wed, Aug 19, 2015 at 05:52:11PM +0800, Gao Pan wrote:
> @@ -963,6 +973,59 @@ out:
> return (result < 0) ? result : num;
> }
>
> +static int i2c_imx_get_scl(struct i2c_adapter *adap)
> +{
> + struct imx_i2c_struct *i2c_imx;
> +
> + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> +
> + return gpiod_get_value(i2c_imx->pins.scl);
> +}
> +
> +static int i2c_imx_get_sda(struct i2c_adapter *adap)
> +{
> + struct imx_i2c_struct *i2c_imx;
> +
> + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> +
> + return gpiod_get_value(i2c_imx->pins.sda);
> +}
> +
> +static void i2c_imx_set_scl(struct i2c_adapter *adap, int val)
> +{
> + struct imx_i2c_struct *i2c_imx;
> +
> + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> + gpiod_set_value(i2c_imx->pins.scl, val);
> +}
<nitpick>
These three functions are very similar (i.e. 1 line respectively for
variable declaration, driver data assignment and gpio operation), but
the last has an empty line less.
</nitpick>
> +static void i2c_imx_prepare_recovery(struct i2c_adapter *adap)
> +{
> + struct imx_i2c_struct *i2c_imx;
> +
> + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> + pinctrl_pm_select_sleep_state(&adap->dev);
I still think this is not nice, the v2 thread is still active with Linus
W. being involved.
> + gpiod_direction_input(i2c_imx->pins.sda);
> + gpiod_direction_output(i2c_imx->pins.scl, 1);
> +}
> [...]
> @@ -1011,12 +1074,13 @@ static int i2c_imx_probe(struct platform_device *pdev)
>
> /* Setup i2c_imx driver structure */
> strlcpy(i2c_imx->adapter.name, pdev->name, sizeof(i2c_imx->adapter.name));
> - i2c_imx->adapter.owner = THIS_MODULE;
> - i2c_imx->adapter.algo = &i2c_imx_algo;
> - i2c_imx->adapter.dev.parent = &pdev->dev;
> - i2c_imx->adapter.nr = pdev->id;
> - i2c_imx->adapter.dev.of_node = pdev->dev.of_node;
> - i2c_imx->base = base;
> + i2c_imx->adapter.owner = THIS_MODULE;
> + i2c_imx->adapter.algo = &i2c_imx_algo;
> + i2c_imx->adapter.dev.parent = &pdev->dev;
> + i2c_imx->adapter.nr = pdev->id;
> + i2c_imx->adapter.dev.of_node = pdev->dev.of_node;
> + i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info;
In a former review round I suggested to only do this assignment after
the two gpios were aquired successfully. Then the above checks could be
if (i2c_imx->adapter.bus_recovery_info)
instead of
if (i2c_imx->pins.sda && i2c_imx->pins.scl)
which IMHO looks nicer and more robust.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
