Hi Dmitry,
Many thanks for your kind review. Agreed on all counts and a minor v9 will
follow shortly. Responses below.
On Sun, Apr 28, 2019 at 10:31:36AM -0700, Dmitry Torokhov wrote:
> Hi Jeff,
>
> On Sun, Apr 07, 2019 at 12:01:12AM -0500, Jeff LaBundy wrote:
> > +static void iqs5xx_reset(struct i2c_client *client)
> > +{
> > + struct iqs5xx_private *iqs5xx = i2c_get_clientdata(client);
> > +
> > + gpiod_set_value_cansleep(iqs5xx->reset_gpio, 0);
> > + usleep_range(200, 300);
> > +
> > + gpiod_set_value_cansleep(iqs5xx->reset_gpio, 1);
>
> I believe we need to switch these statements around:
>
> gpiod_set_value_cansleep(iqs5xx->reset_gpio, 1);
> usleep_range(200, 300);
> gpiod_set_value_cansleep(iqs5xx->reset_gpio, 0);
>
> so that you activate reset line, wait, and then release it. GPIOD deals
> with logical signals, with 1 being active and 0 beig inactive. If reset
> line is active low (it typically us) then it shoudl be described as such
> in DTS and then gpiod API will take care of converting "1" logical
> active to "0" actual value being output.
D'oh! Great catch; looks like I was getting away with this because my dts
was swapped too. I'll fix this here as well as the bindings doc too.
> > +
> > +static irqreturn_t iqs5xx_irq(int irq, void *data)
> > +{
> > + struct iqs5xx_private *iqs5xx = (struct iqs5xx_private *)data;
> > + struct iqs5xx_touch_data *touch_data;
> > + struct i2c_client *client = iqs5xx->client;
> > + struct input_dev *input = iqs5xx->input;
> > + int error, i;
> > + u8 buf[sizeof(*touch_data) * IQS5XX_NUM_CONTACTS];
>
> Given that iqs5xx_touch_data is packed, can't we do
>
> struct iqs5xx_touch_data touch_data[IQS5XX_NUM_CONTACTS];
>
> instead?
Absolutely; I've fixed this and updated the previous references to 'buf'
accordingly (much simpler now).
> No need to resubmit if you agree, I can make changes on my side before
> applying.
Thanks for the offer, but since I need to fix the reset polarity in the
bindings doc as well, I'll resubmit so that you don't have to go to any
trouble on account of my silly errors.
> I also noticed that you are overusing ARRAY_SIZE(): in several cases you
> used it instead of sizeof() for supplying size of a buffer to transfer
> functions. While the result will not change, from logical POW you are
> not interested in number of elements in an array there, you want the
> total size of data structure that just happens to be an array.
Thanks for spotting that; there was an instance of that in iqs5xx_irq which
has been taken care of as part of the above fix.
I still use ARRAY_SIZE in calls to i2c_transfer; that's because we need to
pass the number of i2c_msg elements and not the size of a buffer. However,
there were two instances where the msg.len member and associated memcpy or
memcmp should have used sizeof.
I'll make another pass through it then update the change list accordingly.
> Thanks.
>
> --
> Dmitry
Thanks,
Jeff L.
