[PATCH linux dev-4.10 v4 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults
Andrew Jeffery
andrew at aj.id.au
Fri Aug 25 21:40:34 AEST 2017
On Thu, 2017-08-24 at 13:45 -0500, Christopher Bostic wrote:
>
> On 8/21/17 9:00 PM, Andrew Jeffery wrote:
> > Hi Chris,
> >
> > On Mon, 2017-08-21 at 17:09 -0500, Christopher Bostic wrote:
> > > Add a struct gpio_chip and define some methods so that this device's
> > > I/O can be accessed via /sys/class/gpio.
> > >
> > > Present requirements only call for retrieving current state of pin
> > > values and their direction. No requirement at this time to switch
> > > modes between output and input within the device driver.
> > >
> > > Add capability to clear logged faults via sysfs.
> > >
> > > > Signed-off-by: Christopher Bostic <cbostic at linux.vnet.ibm.com>
> > >
> > > ---
> > > v4 - Change status check from branch to a !! non branching method
> > > - Remove usage comments on libgpiod for the struct gpio_chip
> > > methods.
> > > - Clean up some text formatting.
> > > v3 - Correct bug in gpio_chip get method. Wasn't retrieving
> > > gpio config information correctly.
> > > - Remove old debugfs flag from previous pmbus core changes.
> > > - Remove all sysfs files for mfr_status command.
> > > - Add comments on direct i2c_smbus calls to clarify that no page
> > > set is required.
> > > v2 - Remove clear_faults file - redundant since all other sysfs
> > > core accesses result in an automatic clear fault.
> > > - Removed local status_word and status_vout register dumps and
> > > use the new pmbus core status facilities instead.
> > > - Rename gpi<x>_fault to gpi<x>_alarm to better match core naming
> > > conventions.
> > > - Add full register dump for mfr_status.
> > > - Add gpio chip to device structure and use gpio interfaces.
> > > ---
> > > drivers/hwmon/pmbus/ucd9000.c | 128 +++++++++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 126 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> > > index 3e3aa95..5cea5f6 100644
> > > --- a/drivers/hwmon/pmbus/ucd9000.c
> > > +++ b/drivers/hwmon/pmbus/ucd9000.c
> > > @@ -26,6 +26,9 @@
> > > #include <linux/slab.h>
> > > #include <linux/i2c.h>
> > > #include <linux/i2c/pmbus.h>
> > > +#include <linux/sysfs.h>
> > > +#include <linux/hwmon-sysfs.h>
> > > +#include <linux/gpio.h>
> > > #include "pmbus.h"
> > >
> > > enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
> > > @@ -34,8 +37,16 @@
> > > > > > > > #define UCD9000_NUM_PAGES 0xd6
> > > > > > > > #define UCD9000_FAN_CONFIG_INDEX 0xe7
> > > > > > > > #define UCD9000_FAN_CONFIG 0xe8
> > > > > > > > +#define UCD9000_LOGGED_FAULTS 0xea
> > > > > > > > +#define UCD9000_GPIO_SELECT 0xfa
> > > > > > > > +#define UCD9000_GPIO_CONFIG 0xfb
> > > > #define UCD9000_DEVICE_ID 0xfd
> > >
> > >
> > > +/* GPIO CONFIG bits */
> > > > > > > > +#define UCD9000_GPIO_CONFIG_ENABLE BIT(0)
> > > > > > > > +#define UCD9000_GPIO_CONFIG_OUT_ENABLE BIT(1)
> > > > +#define UCD9000_GPIO_CONFIG_STATUS BIT(3)
> > >
> > > +
> > > > > > > > #define UCD9000_MON_TYPE(x) (((x) >> 5) & 0x07)
> > > > #define UCD9000_MON_PAGE(x) ((x) & 0x0f)
> > >
> > >
> > > @@ -46,9 +57,12 @@
> > >
> > > > #define UCD9000_NUM_FAN 4
> > >
> > >
> > > > +#define UCD90160_NUM_GPIOS 26
> > >
> > > +
> > > struct ucd9000_data {
> > > > > > > > u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
> > > > > > > > struct pmbus_driver_info info;
> > > > + struct gpio_chip gpio;
> > >
> > > };
> > > #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
> > >
> > > @@ -119,6 +133,87 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
> > > };
> > > MODULE_DEVICE_TABLE(i2c, ucd9000_id);
> > >
> > > +static int ucd9000_gpio_read_config(struct i2c_client *client,
> > > > + unsigned int offset)
> > >
> > > +{
> > > > + int ret;
> > >
> > > +
> > > > > > > > + /* No page set required */
> > > > > > > > + ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_SELECT, offset);
> > > > > > > > + if (ret < 0) {
> > > > > > > > + dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
> > > > > > > > + ret);
> > > > > > > > + return ret;
> > > > + }
> > >
> > > +
> > > > + return i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);
> > >
> > > +}
> > > +
> > > +static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
> > > +{
> > > > > > > > + struct i2c_client *client = gpiochip_get_data(gc);
> > > > + int ret;
> > >
> > > +
> > > > > > > > + ret = ucd9000_gpio_read_config(client, offset);
> > > > > > > > + if (ret < 0) {
> > > > > > > > + dev_err(&client->dev, "failed to read GPIO config\n");
> > > > > > > > + return ret;
> > > > + }
> > >
> > > +
> > > > + return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
> >
> > I see you've fixed this case, but didn't audit the rest of the code to fix the
> > remainder. For example the return statement in ucd9000_gpio_get_direction()
> > still branches. Similarly there are error messages that don't report the error
> > return value*, and unnecessary line breaks in in dev_err() calls. In the future,
> > can you please audit your code for classes of problems that are identified in
> > review? I'd hate to have to comment on literally every instance to get them all
> > fixed, as that feels like a waste of my time and a bit patronising towards you.
> >
> > * Where and when we do this needs some judgement with respect to taste, but it
> > certainly frustrates me when I'm reading logs and I see vague messages like
> > "Failed". What failed? Why?
>
> Hi Andrew,
>
> OK, I'll be sure to be more thorough with respect to auditing as you
> suggest and will address these items for next version.
Thanks.
>
> > > +}
> > > +
> > > +static int ucd9000_gpio_get_direction(struct gpio_chip *gc,
> > > > + unsigned int offset)
> > >
> > > +{
> > > > > > > > + struct i2c_client *client = gpiochip_get_data(gc);
> > > > + int ret;
> > >
> > > +
> > > > > > > > + ret = ucd9000_gpio_read_config(client, offset);
> > > > > > > > + if (ret < 0) {
> > > > > > > > + dev_err(&client->dev,
> > > > > > > > + "failed to read GPIO config\n");
> > > > > > > > + return ret;
> > > > + }
> > >
> > > +
> > > > + return ret & UCD9000_GPIO_CONFIG_OUT_ENABLE ? 0 : 1;
> > >
> > > +}
> > > +
> > > +static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
> > > > + unsigned int offset)
> > >
> > > +{
> > > > + return 0;
> >
> > In your reply to my comments on v3 you said you'd implement the direction
> > switching, but this is still unimplemented. What happened?
>
> Based on discussions in our meeting that followed my replies we had
> decided to only implement what was required for our implementation (non
> upstream OpenBMC). Direction checks weren't added as a result. For
> next pass I will add them.
Right; that wasn't my understanding, or at least not what I intended to convey.
We've spent more time discussing whether to implement it than it should take to
do. Implementing it removes a pretty large assumption in the code so I think it
is worthwhile.
>
> > Also I spent some
> > time discussing documentation of assumptions in my reply to Matt, but that
> > seems to not be addressed either. Why is that?
>
> Some basic notes on assumptions were added to the patch set cover
> letter. I hadn't understood this was to be commented within the code.
> I'll do that for next version.
Thanks. Explicitly documenting the assumptions in the code will help when we
get around to improving it to the point we can send it upstream.
> > Did you look at a bare-bones pinctrl implementation so we can describe the GPIO
> > ranges appropriately? If you did and it was too much effort then it would be
> > nice to know that, and why.
>
> I haven't looked into pinctrl implementation in very much depth yet.
> I'd like to implement what the user requires for now and phase pinctrl
> in later. It might be trivial but regardless of that fact there will, I
> assume, be some refining of any implementation requiring further
> revisions/time.
Yeah, this was always a stretch request. However, like the direction issues
above, it would eliminate some pretty hefty assumptions from the code.
>
> > > +}
> > > +
> > > +static ssize_t ucd9000_clear_logged_faults(struct device *dev,
> > > > > > > > + struct device_attribute *attr, const char *buf,
> > > > + size_t count)
> > >
> > > +{
> > > > > > > > + struct i2c_client *client = to_i2c_client(dev);
> > > > + int ret;
> > >
> > > +
> > > > > > > > + /* No page set required */
> > > > > > > > + ret = i2c_smbus_write_byte_data(client, UCD9000_LOGGED_FAULTS, 0);
> > > > > > > > + if (ret) {
> > > > > > > > + dev_err(&client->dev, "Failed to clear logged faults\n");
> > > > > > > > + return ret;
> > > > + }
> > >
> > > +
> > > > + return count;
> > >
> > > +}
> > > +
> > > +static DEVICE_ATTR(clear_logged_faults, 0200, NULL,
> > > > + ucd9000_clear_logged_faults);
> > >
> > > +
> > > +static struct attribute *ucd9000_attributes[] = {
> > > > > > > > + &dev_attr_clear_logged_faults.attr,
> > > > + NULL
> > >
> > > +};
> > > +
> > > +static const struct attribute_group ucd9000_attr_group = {
> > > > + .attrs = ucd9000_attributes,
> > >
> > > +};
> > > +
> >
> > Joel requested the patch be sent upstream to get a decision from the
> > maintainers on this new ABI, and I suggested to you yesterday that it should
> > probably be split out to address ABI changes separate to the GPIO problem, but
> > neither appear to have been done. Why was that?
>
> I have done the separate ABI changes but was interrupted by other
> unrelated high priority work before I could send out to maintainers. It
> is on its way, I have not forgotten.
Great. Thanks.
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170825/7273dc27/attachment.sig>
More information about the openbmc
mailing list