lkml.org 
[lkml]   [2015]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 3/5] misc: eeprom_93xx46: Implement eeprom_93xx46 DT bindings.
    From
    Date
    On 11/18/2015 11:50 PM, Vladimir Zapolskiy wrote:
    > Hi Cory,
    >
    > On 19.11.2015 05:29, Cory Tusar wrote:
    >> This commit implements bindings in the eeprom_93xx46 driver allowing
    >> device word size and read-only attributes to be specified via
    >> devicetree.
    >>
    >> Signed-off-by: Cory Tusar <cory.tusar@pid1solutions.com>
    >> ---
    >> drivers/misc/eeprom/eeprom_93xx46.c | 62 +++++++++++++++++++++++++++++++++++++
    >> 1 file changed, 62 insertions(+)
    >>
    >> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
    >> index e1bf0a5..1f29d9a 100644
    >> --- a/drivers/misc/eeprom/eeprom_93xx46.c
    >> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
    >> @@ -13,6 +13,8 @@
    >> #include <linux/kernel.h>
    >> #include <linux/module.h>
    >> #include <linux/mutex.h>
    >> +#include <linux/of.h>
    >> +#include <linux/of_device.h>
    >> #include <linux/slab.h>
    >> #include <linux/spi/spi.h>
    >> #include <linux/sysfs.h>
    >> @@ -294,12 +296,71 @@ static ssize_t eeprom_93xx46_store_erase(struct device *dev,
    >> }
    >> static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
    >>
    >> +#ifdef CONFIG_OF
    >> +static const struct of_device_id eeprom_93xx46_of_table[] = {
    >> + { .compatible = "eeprom-93xx46", },
    >> + {}
    >> +};
    >> +MODULE_DEVICE_TABLE(of, eeprom_93xx46_of_table);
    >> +
    >
    > Please move this declaration closer to struct spi_driver
    > eeprom_93xx46_driver below.
    >

    It is referenced in the function below.

    > Also you can avoid #ifdef here, if you write
    >
    > .of_match_table = of_match_ptr(eeprom_93xx46_of_table)
    >

    I think this is backwards, when eeprom_93xx46_of_table is #ifdef'd
    off you need to use of_match_ptr so when CONFIG_OF is not defined
    you wont get an undefined reference to eeprom_93xx46_of_table.

    Without the #ifdef, eeprom_93xx46_of_table will always be defined
    and you can just .of_match_table = eeprom_93xx46_of_table; safely.

    > Whenever possible please avoid #ifdef's in .c files.
    >
    >> +static int eeprom_93xx46_probe_dt(struct spi_device *spi)
    >> +{
    >> + struct device_node *np = spi->dev.of_node;
    >> + struct eeprom_93xx46_platform_data *pd;
    >> + u32 tmp;
    >> + int ret;
    >> +
    >> + if (!of_match_device(eeprom_93xx46_of_table, &spi->dev))
    >> + return 0;
    >> +
    >> + pd = devm_kzalloc(&spi->dev, sizeof(*pd), GFP_KERNEL);
    >> + if (!pd)
    >> + return -ENOMEM;
    >> +
    >> + ret = of_property_read_u32(np, "data-size", &tmp);
    >> + if (ret < 0) {
    >> + dev_err(&spi->dev, "data-size property not found\n");
    >> + goto error_free;
    >
    > Because you use devm_* resource allocation in .probe, just return error.
    >
    > Plus I would suggest to change "data-size" property to an optional one,
    > here I mean that if it is omitted, then by default consider pd->flags |=
    > EE_ADDR8.
    >
    >> + }
    >> +
    >> + if (tmp == 8) {
    >> + pd->flags |= EE_ADDR8;
    >> + } else if (tmp == 16) {
    >> + pd->flags |= EE_ADDR16;
    >> + } else {
    >> + dev_err(&spi->dev, "invalid data-size (%d)\n", tmp);
    >> + goto error_free;
    >
    > Same here.
    >
    >> + }
    >> +
    >> + if (of_property_read_bool(np, "read-only"))
    >> + pd->flags |= EE_READONLY;
    >> +
    >> + spi->dev.platform_data = pd;
    >> +
    >> + return 1;
    >
    > On success please return 0.
    >
    >> +error_free:
    >> + devm_kfree(&spi->dev, pd);
    >> + return ret;
    >> +}
    >> +
    >> +#else
    >> +static inline int eeprom_93xx46_probe_dt(struct spi_device *spi)
    >> +{
    >> + return 0;
    >> +}
    >> +#endif
    >> +
    >
    > I actually don't see a point to have #ifdef CONFIG_OF here.
    >

    Usually to avoid a lot of dead code and data when OF is not enabled.

    > Instead please add a check for !spi->dev.of_node at the beginning of
    > eeprom_93xx46_probe_dt() or in .probe()
    >
    >> static int eeprom_93xx46_probe(struct spi_device *spi)
    >> {
    >> struct eeprom_93xx46_platform_data *pd;
    >> struct eeprom_93xx46_dev *edev;
    >> int err;
    >>
    >> + err = eeprom_93xx46_probe_dt(spi);
    >> + if (err < 0)
    >> + return err;
    >> +
    >> pd = spi->dev.platform_data;
    >> if (!pd) {
    >> dev_err(&spi->dev, "missing platform data\n");
    >> @@ -370,6 +431,7 @@ static int eeprom_93xx46_remove(struct spi_device *spi)
    >> static struct spi_driver eeprom_93xx46_driver = {
    >> .driver = {
    >> .name = "93xx46",
    >> + .of_match_table = eeprom_93xx46_of_table,
    >> },
    >> .probe = eeprom_93xx46_probe,
    >> .remove = eeprom_93xx46_remove,
    >>


    \
     
     \ /
      Last update: 2015-11-19 15:21    [W:3.059 / U:0.172 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site