Messages in this thread | | | Subject | Re: [PATCH V5 1/6] drivers/mfd: sensehat: Raspberry Pi Sense HAT core driver | From | Stefan Wahren <> | Date | Sun, 12 Dec 2021 12:43:30 +0100 |
| |
Am 10.12.21 um 23:10 schrieb Charles Mirabile: > This patch adds the core driver file, containing the regmap configuration > needed to communicate with the board over I2C. We also add the header > file shared by all three drivers, containing common data and definitions. > In addition, we add a config option to toggle compilation of the driver. > > Signed-off-by: Charles Mirabile <cmirabil@redhat.com> > Co-developed-by: Mwesigwa Guma <mguma@redhat.com> > Signed-off-by: Mwesigwa Guma <mguma@redhat.com> > Co-developed-by: Joel Savitz <jsavitz@redhat.com> > Signed-off-by: Joel Savitz <jsavitz@redhat.com> > --- > drivers/mfd/Kconfig | 8 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/sensehat-core.c | 157 +++++++++++++++++++++++++++++++++++ > include/linux/mfd/sensehat.h | 51 ++++++++++++ > 4 files changed, 217 insertions(+) > create mode 100644 drivers/mfd/sensehat-core.c > create mode 100644 include/linux/mfd/sensehat.h > ... > diff --git a/drivers/mfd/sensehat-core.c b/drivers/mfd/sensehat-core.c > new file mode 100644 > index 000000000000..c5b6f4648d88 > --- /dev/null > +++ b/drivers/mfd/sensehat-core.c > @@ -0,0 +1,157 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Raspberry Pi Sense HAT core driver > + * http://raspberrypi.org > + * > + * Copyright (C) 2015 Raspberry Pi > + * Copyright (C) 2021 Charles Mirabile, Mwesigwa Guma, Joel Savitz > + * > + * Original Author: Serge Schneider > + * Revised for upstream Linux by: Charles Mirabile, Mwesigwa Guma, Joel Savitz > + * > + * This driver is based on wm8350 implementation and was refactored to use the > + * misc device subsystem rather than the deprecated framebuffer subsystem. > + */ > + > +#include <linux/module.h> > +#include <linux/moduleparam.h> Is this header really necessary? > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/i2c.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/regmap.h> > +#include <linux/mfd/sensehat.h> > + > +#define SENSEHAT_WAI 0xF0 > +#define SENSEHAT_VER 0xF1 > + > +#define SENSEHAT_ID 's' > + > +static struct platform_device * > +sensehat_client_dev_register(struct sensehat *sensehat, const char *name); Please avoid this forward declaration > + > +static struct regmap_config sensehat_config; Is there a specific reason, why the driver only supports one sense hat? > + > +static int sensehat_probe(struct i2c_client *i2c, > + const struct i2c_device_id *id) > +{ > + int ret; > + unsigned int reg; > + > + struct sensehat *sensehat = > + devm_kzalloc(&i2c->dev, sizeof(*sensehat), GFP_KERNEL); > + > + if (!sensehat) > + return -ENOMEM; > + > + i2c_set_clientdata(i2c, sensehat); > + sensehat->dev = &i2c->dev; > + sensehat->i2c_client = i2c; > + > + sensehat->regmap = > + devm_regmap_init_i2c(sensehat->i2c_client, &sensehat_config); > + > + if (IS_ERR(sensehat->regmap)) { > + dev_err(sensehat->dev, "Failed to initialize sensehat regmap"); > + return PTR_ERR(sensehat->regmap); > + } > + > + ret = regmap_read(sensehat->regmap, SENSEHAT_WAI, ®); > + if (ret < 0) { > + dev_err(sensehat->dev, "failed to read from device"); I think the return code of regmap_read could be helpful in the log entry. > + return ret; > + } > + > + if (reg != SENSEHAT_ID) { > + dev_err(sensehat->dev, "expected device ID %i, got %i", > + SENSEHAT_ID, ret); > + return -EINVAL; > + } > + > + ret = regmap_read(sensehat->regmap, SENSEHAT_VER, ®); > + if (ret < 0) { > + dev_err(sensehat->dev, > + "Unable to get sensehat firmware version"); > + return ret; > + } > + > + dev_info(sensehat->dev, "Raspberry Pi Sense HAT firmware version %i\n", > + reg); > + > +#ifdef CONFIG_JOYSTICK_SENSEHAT > + sensehat->joystick.pdev = > + sensehat_client_dev_register(sensehat, "sensehat-joystick"); > + > + if (IS_ERR(sensehat->joystick.pdev)) { > + dev_err(sensehat->dev, "failed to register sensehat-joystick"); > + return PTR_ERR(sensehat->joystick.pdev); > + } > +#endif Please replace the #ifdev blocks with IS_ENABLED() to increase compile coverage
| |