Hello,
On Tue Feb 27, 2024 at 7:14 PM CET, Andy Shevchenko wrote:
> On Tue, Feb 27, 2024 at 03:55:26PM +0100, Théo Lebrun wrote:
> > Add the Mobileye EyeQ5 pin controller driver. It might grow to add later
> > support of other platforms from Mobileye. It belongs to a syscon region
> > called OLB.
> >
> > Existing pins and their function live statically in the driver code
> > rather than in the devicetree, see compatible match data.
>
> ...
>
> > +config PINCTRL_EYEQ5
> > + bool "Mobileye EyeQ5 pinctrl driver"
>
> Can't be a module?
It theory it could, I however do not see why that would be done. Pinctrl
is essential to the platform capabilities. The platform is an embedded
one and performance-oriented; boot-time is important and no user will
ever want to load pinctrl as a module.
>
> > + depends on OF
>
> It's even not needed for this software as far as I can tell from the code.
Indeed looks like it. Will try that out and remove the dependency if it
works as expected.
[...]
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pinctrl/pinconf-generic.h>
> > +#include <linux/pinctrl/pinconf.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <linux/pinctrl/pinmux.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/seq_file.h>
>
> Semi-random list of the inclusions. Please, fix it.
> While doing that, group out pinctrl/* ones as it's done in other drivers.
Here is my new list:
#include <linux/array_size.h>
#include <linux/bits.h>
#include <linux/bug.h>
#include <linux/device.h>
#include <linux/err.h>
#include <linux/errno.h>
#include <linux/io.h>
#include <linux/mod_devicetable.h>
#include <linux/platform_device.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
#include <linux/types.h>
#include <linux/pinctrl/pinconf-generic.h>
#include <linux/pinctrl/pinconf.h>
#include <linux/pinctrl/pinctrl.h>
#include <linux/pinctrl/pinmux.h>
#include "core.h"
#include "pinctrl-utils.h"
[...]
> > +static bool eq5p_test_bit(const struct eq5p_pinctrl *pctrl,
> > + enum eq5p_bank bank, enum eq5p_regs reg, int offset)
> > +{
> > + u32 val = readl(pctrl->base + eq5p_regs[bank][reg]);
>
> > + if (WARN_ON(offset > 31))
> > + return false;
>
> When this condition can be true?
If there is a bug in the code. Defensive programming.
There is this subtle conversion of pin numbers => offset inside of a
bank. If one function forgets doing this then eq5p_test_bit() gets
called with a pin number.
In this GPIO series I fixed such a bug in a 10 year old driver:
https://lore.kernel.org/lkml/20240228-mbly-gpio-v2-5-3ba757474006@xxxxxxxxxxx/
The whole "if it can happen it will happen" mantra. We'll get a warning
in the logs using pinctrl-eyeq5.
>
> > + return (val & BIT(offset)) != 0;
> > +}
>
> ...
>
> > +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> > + unsigned long *config);
>
> Can't you avoid forward declarations?
Yes, will do so.
>
> ...
>
> > + if (!eq5p_test_bit(pctrl, bank, EQ5P_IOCR, offset)) {
>
> What's wrong with positive conditional?
Nothing. In my mind GPIO was first, other was second. Will change.
>
>
> > + } else {
>
> > + }
>
> ...
>
> > +static const struct pinctrl_ops eq5p_pinctrl_ops = {
> > + .get_groups_count = eq5p_pinctrl_get_groups_count,
> > + .get_group_name = eq5p_pinctrl_get_group_name,
> > + .get_group_pins = eq5p_pinctrl_get_group_pins,
> > + .pin_dbg_show = eq5p_pinctrl_pin_dbg_show,
>
> > + .dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
> > + .dt_free_map = pinctrl_utils_free_map,
>
> ifdef is missing for these... But the question is, isn't these a default when
> OF is in use?
Doesn't look like it is. In drivers/pinctrl/devicetree.c:
static int dt_to_map_one_config(struct pinctrl *p,
struct pinctrl_dev *hog_pctldev,
const char *statename,
struct device_node *np_config)
{
// ...
/*
* Call pinctrl driver to parse device tree node, and
* generate mapping table entries
*/
ops = pctldev->desc->pctlops;
if (!ops->dt_node_to_map) {
dev_err(p->dev, "pctldev %s doesn't support DT\n",
dev_name(pctldev->dev));
return -ENODEV;
}
// ...
}
And I see nowhere that puts a value if ->dt_node_to_map is empty.
For dt_free_map, it is an optional value. If the field is NULL nothing
is done. See dt_free_map() in the same file.
[...]
> > + mask = BIT(offset);
> > + val = is_gpio ? 0 : U32_MAX;
>
> I think you meant something else (semantically) than U32_MAX.
> Perhaps GENMASK(31, 0)?
To me the semantic of U32_MAX is the same. I see where you are coming
from. A better alternative however would be:
mask = BIT(offset);
val = is_gpio ? 0 : mask;
That way the desire is clear and the code is simpler.
>
> ...
>
> > +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> > + unsigned long *config)
> > +{
> > + enum pin_config_param param = pinconf_to_config_param(*config);
> > + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + unsigned int offset = eq5p_pin_to_offset(pin);
> > + enum eq5p_bank bank = eq5p_pin_to_bank(pin);
> > + u32 val_ds, arg = 0;
>
> What's arg assignment for?
No reason indeed. Will remove the assignment.
>
> > + bool pd, pu;
> > +
> > + pd = eq5p_test_bit(pctrl, bank, EQ5P_PD, offset);
> > + pu = eq5p_test_bit(pctrl, bank, EQ5P_PU, offset);
> > +
> > + switch (param) {
> > + case PIN_CONFIG_BIAS_DISABLE:
> > + arg = !(pd || pu);
> > + break;
> > + case PIN_CONFIG_BIAS_PULL_DOWN:
> > + arg = pd;
> > + break;
> > + case PIN_CONFIG_BIAS_PULL_UP:
> > + arg = pu;
> > + break;
> > + case PIN_CONFIG_DRIVE_STRENGTH:
> > + offset *= 2; /* two bits per pin */
> > + if (offset >= 32) {
> > + val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_HIGH]);
> > + offset -= 32;
> > + } else {
> > + val_ds = readl(pctrl->base + eq5p_regs[bank][EQ5P_DS_LOW]);
> > + }
>
> I'm wondering why you can't use your helpers before multiplication?
I'm unsure what helpers you are talking about?
If the question is about why multiply before if-condition: I feel like
multiplying first allows having the if condition be "offset >= 32".
That explicits why we readl HIGH vs LOW regs.
[...]
>
> > +static int eq5p_pinconf_set_drive_strength(struct pinctrl_dev *pctldev,
> > + unsigned int pin, u32 arg)
> > +{
> > + struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + unsigned int offset = eq5p_pin_to_offset(pin);
> > + enum eq5p_bank bank = eq5p_pin_to_bank(pin);
> > + unsigned int reg;
> > + u32 mask, val;
> > +
> > + if (arg > 3) {
>
> Magic number.
Would 0b11 explicit why? The value is two bits wide, so 0 thru 3.
>
> > + dev_err(pctldev->dev, "Unsupported drive strength: %u\n", arg);
> > + return -EINVAL;
> > + }
> > +
> > + offset *= 2; /* two bits per pin */
> > +
> > + if (offset >= 32) {
> > + reg = EQ5P_DS_HIGH;
> > + offset -= 32;
> > + } else {
> > + reg = EQ5P_DS_LOW;
> > + }
>
> > + mask = 0b11 << offset;
> > + val = arg << offset;
> > + eq5p_update_bits(pctrl, bank, reg, mask, val);
>
> Similar comments as per previous function.
So GENMASK(1, 0) rather than 0b11. Or GENMASK(offset+1, offset).
Something else?
>
> > + return 0;
> > +}
>
> ...
>
> > +static const struct of_device_id eq5p_match[] = {
> > + { .compatible = "mobileye,eyeq5-pinctrl" },
> > + {},
>
> No comma in the terminator entry.
>
> > +};
>
> No MODULE_DEVICE_TABLE()?
It is an oversight. Will be added.
Thanks for the review Andy.
Have a nice day,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
