| Message ID | 20241008-mbly-i2c-v1-2-a06c1317a2f7@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: nomadik: support >=1MHz & Mobileye EyeQ6H platform | expand |
On Tue, Oct 08, 2024 at 12:29:41PM +0200, Théo Lebrun wrote: > Add EyeQ6H support to the nmk-i2c AMBA driver. It shares the same quirk > as EyeQ5: the memory bus only supports 32-bit accesses. Avoid writeb() > and readb() by reusing the same `priv->has_32b_bus` flag. > > It does NOT need to write speed-mode specific value into a register; > therefore it does not depend on the mobileye,olb DT property. > > Refactoring is done using is_eyeq5 and is_eyeq6h boolean local > variables. Sort variables in reverse christmas tree to try and > introduce some logic into the ordering. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/i2c/busses/i2c-nomadik.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c > index ad0f02acdb1215a1c04729f97bb14a4d93f88456..ea511d3a58073eaedb63850026e05b59427a69c6 100644 > --- a/drivers/i2c/busses/i2c-nomadik.c > +++ b/drivers/i2c/busses/i2c-nomadik.c > @@ -6,10 +6,10 @@ > * I2C master mode controller driver, used in Nomadik 8815 > * and Ux500 platforms. > * > - * The Mobileye EyeQ5 platform is also supported; it uses > + * The Mobileye EyeQ5 and EyeQ6H platforms are also supported; they use > * the same Ux500/DB8500 IP block with two quirks: > * - The memory bus only supports 32-bit accesses. > - * - A register must be configured for the I2C speed mode; > + * - (only EyeQ5) A register must be configured for the I2C speed mode; > * it is located in a shared register region called OLB. > * > * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com> > @@ -1046,8 +1046,6 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv) > struct regmap *olb; > unsigned int id; > > - priv->has_32b_bus = true; > - > olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id); > if (IS_ERR(olb)) > return PTR_ERR(olb); > @@ -1070,13 +1068,15 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv) > > static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) > { > - int ret = 0; > - struct nmk_i2c_dev *priv; > - struct device_node *np = adev->dev.of_node; > - struct device *dev = &adev->dev; > - struct i2c_adapter *adap; > struct i2c_vendor_data *vendor = id->data; > + struct device_node *np = adev->dev.of_node; > + bool is_eyeq6h = of_device_is_compatible(np, "mobileye,eyeq6h-i2c"); > + bool is_eyeq5 = of_device_is_compatible(np, "mobileye,eyeq5-i2c"); You should use match data, not add compatibles in the middle of code. That's preferred, scallable pattern. What you added here last time does not scale and above change is a proof for that. Best regards, Krzysztof
Hello Krzysztof, On Tue Oct 8, 2024 at 3:39 PM CEST, Krzysztof Kozlowski wrote: > On Tue, Oct 08, 2024 at 12:29:41PM +0200, Théo Lebrun wrote: > > Add EyeQ6H support to the nmk-i2c AMBA driver. It shares the same quirk > > as EyeQ5: the memory bus only supports 32-bit accesses. Avoid writeb() > > and readb() by reusing the same `priv->has_32b_bus` flag. > > > > It does NOT need to write speed-mode specific value into a register; > > therefore it does not depend on the mobileye,olb DT property. > > > > Refactoring is done using is_eyeq5 and is_eyeq6h boolean local > > variables. Sort variables in reverse christmas tree to try and > > introduce some logic into the ordering. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > drivers/i2c/busses/i2c-nomadik.c | 22 +++++++++++----------- > > 1 file changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c > > index ad0f02acdb1215a1c04729f97bb14a4d93f88456..ea511d3a58073eaedb63850026e05b59427a69c6 100644 > > --- a/drivers/i2c/busses/i2c-nomadik.c > > +++ b/drivers/i2c/busses/i2c-nomadik.c > > @@ -6,10 +6,10 @@ > > * I2C master mode controller driver, used in Nomadik 8815 > > * and Ux500 platforms. > > * > > - * The Mobileye EyeQ5 platform is also supported; it uses > > + * The Mobileye EyeQ5 and EyeQ6H platforms are also supported; they use > > * the same Ux500/DB8500 IP block with two quirks: > > * - The memory bus only supports 32-bit accesses. > > - * - A register must be configured for the I2C speed mode; > > + * - (only EyeQ5) A register must be configured for the I2C speed mode; > > * it is located in a shared register region called OLB. > > * > > * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com> > > @@ -1046,8 +1046,6 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv) > > struct regmap *olb; > > unsigned int id; > > > > - priv->has_32b_bus = true; > > - > > olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id); > > if (IS_ERR(olb)) > > return PTR_ERR(olb); > > @@ -1070,13 +1068,15 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv) > > > > static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) > > { > > - int ret = 0; > > - struct nmk_i2c_dev *priv; > > - struct device_node *np = adev->dev.of_node; > > - struct device *dev = &adev->dev; > > - struct i2c_adapter *adap; > > struct i2c_vendor_data *vendor = id->data; > > + struct device_node *np = adev->dev.of_node; > > + bool is_eyeq6h = of_device_is_compatible(np, "mobileye,eyeq6h-i2c"); > > + bool is_eyeq5 = of_device_is_compatible(np, "mobileye,eyeq5-i2c"); > > You should use match data, not add compatibles in the middle of code. > That's preferred, scallable pattern. What you added here last time does > not scale and above change is a proof for that. I would have used match data if the driver struct had a .of_match_table field. `struct amba_driver` does not. Are you recommending the approach below? I don't see how it brings much to the driver but I do get the scaling issue as the number of support compatibles increases. This is a fear based on what *could* happen in the future though. ------------------------------------------------------------------------ diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c index 82571983bbca..98fc40dfcbfc 100644 --- a/drivers/i2c/busses/i2c-nomadik.c +++ b/drivers/i2c/busses/i2c-nomadik.c @@ -26,6 +26,7 @@ #include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/pinctrl/consumer.h> #include <linux/pm_runtime.h> #include <linux/regmap.h> @@ -1061,28 +1062,46 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv) return 0; } +#define NMK_I2C_EYEQ_FLAG_32B_BUS BIT(0) +#define NMK_I2C_EYEQ_FLAG_IS_EYEQ5 BIT(1) + +static const struct of_device_id nmk_i2c_eyeq_match_table[] = { + { + .compatible = "mobileye,eyeq5-i2c", + .data = (void*)(NMK_I2C_EYEQ_FLAG_32B_BUS | NMK_I2C_EYEQ_FLAG_IS_EYEQ5), + }, + { + .compatible = "mobileye,eyeq6h-i2c", + .data = (void*)(NMK_I2C_EYEQ_FLAG_32B_BUS), + }, +}; + static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) { struct i2c_vendor_data *vendor = id->data; struct device_node *np = adev->dev.of_node; - bool is_eyeq6h = of_device_is_compatible(np, "mobileye,eyeq6h-i2c"); - bool is_eyeq5 = of_device_is_compatible(np, "mobileye,eyeq5-i2c"); u32 max_fifo_threshold = (vendor->fifodepth / 2) - 1; + const struct of_device_id *match; struct device *dev = &adev->dev; + unsigned long match_flags = 0; struct nmk_i2c_dev *priv; struct i2c_adapter *adap; int ret = 0; + match = of_match_device(nmk_i2c_eyeq_match_table, dev); + if (match) + match_flags = (unsigned long)match->data; + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; priv->vendor = vendor; priv->adev = adev; - priv->has_32b_bus = is_eyeq5 || is_eyeq6h; + priv->has_32b_bus = match_flags & NMK_I2C_EYEQ_FLAG_32B_BUS; nmk_i2c_of_probe(np, priv); - if (is_eyeq5) { + if (match_flags & NMK_I2C_EYEQ_FLAG_IS_EYEQ5) { ret = nmk_i2c_eyeq5_probe(priv); if (ret) return dev_err_probe(dev, ret, "failed OLB lookup\n"); ------------------------------------------------------------------------ Thanks Krzysztof, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 08/10/2024 16:43, Théo Lebrun wrote: > Hello Krzysztof, > > On Tue Oct 8, 2024 at 3:39 PM CEST, Krzysztof Kozlowski wrote: >> On Tue, Oct 08, 2024 at 12:29:41PM +0200, Théo Lebrun wrote: >>> Add EyeQ6H support to the nmk-i2c AMBA driver. It shares the same quirk >>> as EyeQ5: the memory bus only supports 32-bit accesses. Avoid writeb() >>> and readb() by reusing the same `priv->has_32b_bus` flag. >>> >>> It does NOT need to write speed-mode specific value into a register; >>> therefore it does not depend on the mobileye,olb DT property. >>> >>> Refactoring is done using is_eyeq5 and is_eyeq6h boolean local >>> variables. Sort variables in reverse christmas tree to try and >>> introduce some logic into the ordering. >>> >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >>> --- >>> drivers/i2c/busses/i2c-nomadik.c | 22 +++++++++++----------- >>> 1 file changed, 11 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c >>> index ad0f02acdb1215a1c04729f97bb14a4d93f88456..ea511d3a58073eaedb63850026e05b59427a69c6 100644 >>> --- a/drivers/i2c/busses/i2c-nomadik.c >>> +++ b/drivers/i2c/busses/i2c-nomadik.c >>> @@ -6,10 +6,10 @@ >>> * I2C master mode controller driver, used in Nomadik 8815 >>> * and Ux500 platforms. >>> * >>> - * The Mobileye EyeQ5 platform is also supported; it uses >>> + * The Mobileye EyeQ5 and EyeQ6H platforms are also supported; they use >>> * the same Ux500/DB8500 IP block with two quirks: >>> * - The memory bus only supports 32-bit accesses. >>> - * - A register must be configured for the I2C speed mode; >>> + * - (only EyeQ5) A register must be configured for the I2C speed mode; >>> * it is located in a shared register region called OLB. >>> * >>> * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com> >>> @@ -1046,8 +1046,6 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv) >>> struct regmap *olb; >>> unsigned int id; >>> >>> - priv->has_32b_bus = true; >>> - >>> olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id); >>> if (IS_ERR(olb)) >>> return PTR_ERR(olb); >>> @@ -1070,13 +1068,15 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv) >>> >>> static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) >>> { >>> - int ret = 0; >>> - struct nmk_i2c_dev *priv; >>> - struct device_node *np = adev->dev.of_node; >>> - struct device *dev = &adev->dev; >>> - struct i2c_adapter *adap; >>> struct i2c_vendor_data *vendor = id->data; >>> + struct device_node *np = adev->dev.of_node; >>> + bool is_eyeq6h = of_device_is_compatible(np, "mobileye,eyeq6h-i2c"); >>> + bool is_eyeq5 = of_device_is_compatible(np, "mobileye,eyeq5-i2c"); >> >> You should use match data, not add compatibles in the middle of code. >> That's preferred, scallable pattern. What you added here last time does >> not scale and above change is a proof for that. > > I would have used match data if the driver struct had a .of_match_table > field. `struct amba_driver` does not. Are you recommending the approach > below? > > I don't see how it brings much to the driver but I do get the scaling > issue as the number of support compatibles increases. This is a fear > based on what *could* happen in the future though. You still have adev->dev.of_node, which you can use for matching in probe. See for example of_match_device() (and add a note so people will not convert it to device_get_match_data() blindly). Best regards, Krzysztof
On Tue Oct 8, 2024 at 6:03 PM CEST, Krzysztof Kozlowski wrote: > On 08/10/2024 16:43, Théo Lebrun wrote: > > On Tue Oct 8, 2024 at 3:39 PM CEST, Krzysztof Kozlowski wrote: > >> On Tue, Oct 08, 2024 at 12:29:41PM +0200, Théo Lebrun wrote: > >>> + bool is_eyeq6h = of_device_is_compatible(np, "mobileye,eyeq6h-i2c"); > >>> + bool is_eyeq5 = of_device_is_compatible(np, "mobileye,eyeq5-i2c"); > >> > >> You should use match data, not add compatibles in the middle of code. > >> That's preferred, scallable pattern. What you added here last time does > >> not scale and above change is a proof for that. > > > > I would have used match data if the driver struct had a .of_match_table > > field. `struct amba_driver` does not. Are you recommending the approach > > below? > > > > I don't see how it brings much to the driver but I do get the scaling > > issue as the number of support compatibles increases. This is a fear > > based on what *could* happen in the future though. > > You still have adev->dev.of_node, which you can use for matching in > probe. See for example of_match_device() (and add a note so people will > not convert it to device_get_match_data() blindly). Just sent the new revision [0]. It uses of_match_device() in the same way as was shown in my previous answer to this thread [1]. Followed your recommendation and added a comment to avoid conversions to device_get_match_data(). Thanks! [0]: https://lore.kernel.org/lkml/20241009-mbly-i2c-v2-0-ac9230a8dac5@bootlin.com/ [1]: https://lore.kernel.org/lkml/D4QI63B6YQU5.3UPKA7G75J445@bootlin.com/ -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c index ad0f02acdb1215a1c04729f97bb14a4d93f88456..ea511d3a58073eaedb63850026e05b59427a69c6 100644 --- a/drivers/i2c/busses/i2c-nomadik.c +++ b/drivers/i2c/busses/i2c-nomadik.c @@ -6,10 +6,10 @@ * I2C master mode controller driver, used in Nomadik 8815 * and Ux500 platforms. * - * The Mobileye EyeQ5 platform is also supported; it uses + * The Mobileye EyeQ5 and EyeQ6H platforms are also supported; they use * the same Ux500/DB8500 IP block with two quirks: * - The memory bus only supports 32-bit accesses. - * - A register must be configured for the I2C speed mode; + * - (only EyeQ5) A register must be configured for the I2C speed mode; * it is located in a shared register region called OLB. * * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com> @@ -1046,8 +1046,6 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv) struct regmap *olb; unsigned int id; - priv->has_32b_bus = true; - olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id); if (IS_ERR(olb)) return PTR_ERR(olb); @@ -1070,13 +1068,15 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv) static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) { - int ret = 0; - struct nmk_i2c_dev *priv; - struct device_node *np = adev->dev.of_node; - struct device *dev = &adev->dev; - struct i2c_adapter *adap; struct i2c_vendor_data *vendor = id->data; + struct device_node *np = adev->dev.of_node; + bool is_eyeq6h = of_device_is_compatible(np, "mobileye,eyeq6h-i2c"); + bool is_eyeq5 = of_device_is_compatible(np, "mobileye,eyeq5-i2c"); u32 max_fifo_threshold = (vendor->fifodepth / 2) - 1; + struct device *dev = &adev->dev; + struct nmk_i2c_dev *priv; + struct i2c_adapter *adap; + int ret = 0; priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) @@ -1084,10 +1084,10 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) priv->vendor = vendor; priv->adev = adev; - priv->has_32b_bus = false; + priv->has_32b_bus = is_eyeq5 || is_eyeq6h; nmk_i2c_of_probe(np, priv); - if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) { + if (is_eyeq5) { ret = nmk_i2c_eyeq5_probe(priv); if (ret) return dev_err_probe(dev, ret, "failed OLB lookup\n");
Add EyeQ6H support to the nmk-i2c AMBA driver. It shares the same quirk as EyeQ5: the memory bus only supports 32-bit accesses. Avoid writeb() and readb() by reusing the same `priv->has_32b_bus` flag. It does NOT need to write speed-mode specific value into a register; therefore it does not depend on the mobileye,olb DT property. Refactoring is done using is_eyeq5 and is_eyeq6h boolean local variables. Sort variables in reverse christmas tree to try and introduce some logic into the ordering. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- drivers/i2c/busses/i2c-nomadik.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)