linux-kernel - RE: [PATCH 4/4] mux: lan966: Add support for flexcom mux controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <PH0PR11MB4872AB5822FFA5C231C2EF5892C69@PH0PR11MB4872.namprd11.prod.outlook.com>
Date:   Mon, 9 May 2022 08:25:56 +0000
From:   <Kavyasree.Kotagiri@...rochip.com>
To:     <krzysztof.kozlowski@...aro.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <Nicolas.Ferre@...rochip.com>,
        <alexandre.belloni@...tlin.com>, <Claudiu.Beznea@...rochip.com>,
        <peda@...ntia.se>
CC:     <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <lee.jones@...aro.org>,
        <linux@...linux.org.uk>, <Manohar.Puri@...rochip.com>,
        <UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH 4/4] mux: lan966: Add support for flexcom mux controller

> > +#include <linux/err.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +#include <linux/mux/driver.h>
> > +#include <linux/io.h>
> > +
> > +#define FLEX_SHRD_MASK               0x1FFFFF
> > +#define LAN966_MAX_CS                21
> > +
> > +static void __iomem *flx_shared_base;
> 
> Why do you have file-scope shared variable? Cannot it be passed via
> private data?
> 
I want flx_shared_base to be global variable and use struct mux_lan966x to represent only 
"mux-offset-pin" parameters.

> > +struct mux_lan966x {
> > +     u32 offset;
> > +     u32 ss_pin;
> > +};
> > +
> > +static int mux_lan966x_set(struct mux_control *mux, int state)
> > +{
> > +     struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
> > +     u32 val;
> > +
> > +     val = ~(1 << mux_lan966x[state].ss_pin) & FLEX_SHRD_MASK;
> > +     writel(val, flx_shared_base + mux_lan966x[state].offset);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct mux_control_ops mux_lan966x_ops = {
> > +     .set = mux_lan966x_set,
> > +};
> > +
> > +static const struct of_device_id mux_lan966x_dt_ids[] = {
> > +     { .compatible = "microchip,lan966-flx-mux", },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mux_lan966x_dt_ids);
> > +
> > +static int mux_lan966x_probe(struct platform_device *pdev)
> > +{
> > +     struct device_node *np = pdev->dev.of_node;
> > +     struct device *dev = &pdev->dev;
> > +     struct mux_lan966x *mux_lan966x;
> > +     struct mux_chip *mux_chip;
> > +     int ret, num_fields, i;
> > +
> > +     ret = of_property_count_u32_elems(np, "mux-offset-pin");
> > +     if (ret == 0 || ret % 2)
> > +             ret = -EINVAL;
> > +     if (ret < 0)
> > +             return dev_err_probe(dev, ret,
> > +                                  "mux-offset-pin property missing or invalid");
> > +     num_fields = ret / 2;
> > +
> > +     mux_chip = devm_mux_chip_alloc(dev, num_fields,
> sizeof(*mux_lan966x));
> > +     if (IS_ERR(mux_chip))
> > +             return dev_err_probe(dev, PTR_ERR(mux_chip),
> > +                                  "failed to allocate mux_chips\n");
> > +
> > +     mux_lan966x = mux_chip_priv(mux_chip);
> > +
> > +     flx_shared_base = devm_platform_get_and_ioremap_resource(pdev,
> 0, NULL);
> > +     if (IS_ERR(flx_shared_base))
> > +             return dev_err_probe(dev, PTR_ERR(flx_shared_base),
> > +                                  "failed to get flexcom shared base address\n");
> > +
> > +     for (i = 0; i < num_fields; i++) {
> > +             struct mux_control *mux = &mux_chip->mux[i];
> > +             u32 offset, shared_pin;
> > +
> > +             ret = of_property_read_u32_index(np, "mux-offset-pin",
> > +                                              2 * i, &offset);
> > +             if (ret == 0)
> > +                     ret = of_property_read_u32_index(np, "mux-offset-pin",
> > +                                                      2 * i + 1,
> > +                                                      &shared_pin);
> > +             if (ret < 0)
> > +                     return dev_err_probe(dev, ret,
> > +                                          "failed to read mux-offset-pin property: %d", i);
> > +
> > +             if (shared_pin >= LAN966_MAX_CS)
> > +                     return -EINVAL;
> > +
> > +             mux_lan966x[i].offset = offset;
> > +             mux_lan966x[i].ss_pin = shared_pin;
> > +
> > +             mux->states = LAN966_MAX_CS;
> > +     }
> > +
> > +     mux_chip->ops = &mux_lan966x_ops;
> > +
> > +     ret = devm_mux_chip_register(dev, mux_chip);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> > +static struct platform_driver mux_lan966x_driver = {
> > +     .driver = {
> > +             .name = "lan966-mux",
> > +             .of_match_table = of_match_ptr(mux_lan966x_dt_ids),
> 
> of_match_ptr comes with maybe_unused on data structure. Are you sure it
> does not have W=1 warnings during compile tests? Just drop the
> of_match_ptr.
> 
No,  I haven't noticed any warning. Other mux drivers also follow the same.

> > +     },
> > +     .probe = mux_lan966x_probe,
> > +};
> > +
> > +module_platform_driver(mux_lan966x_driver);
> 
> Missing MODULE() stuff.
Ok. I will add it in next version of patch series.

> 
> 
> Best regards,
> Krzysztof