Re: [PATCH 02/14] gpio: pca953x: Fix AI overflow on PCAL6524

[Bartosz Golaszewski]

niedz., 2 gru 2018 o 20:36 Marek Vasut  napisał(a):
>
> The PCAL_PINCTRL_MASK is too large. The extended register block on
> PCAL6524, which is the largest chip with this block, has the block
> limited to address range 0x40..0x7f. This is because the bit 7 in
> the command register is used for the Address Increment functionality.
>
> Trim the mask to 0x60 to match the datasheet and to prevent accidental
> overwrite of the AI bit.
>
> Signed-off-by: Marek Vasut 
> Cc: Linus Walleij 
> Cc: Bartosz Golaszewski 
> ---
>  drivers/gpio/gpio-pca953x.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 31e3b1b52330..4e9c79ca69c5 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -58,7 +58,7 @@
>  #define PCA_GPIO_MASK  0x00FF
>
>  #define PCAL_GPIO_MASK 0x1f
> -#define PCAL_PINCTRL_MASK  0xe0
> +#define PCAL_PINCTRL_MASK  0x60
>
>  #define PCA_INT    0x0100
>  #define PCA_PCAL   0x0200
> --
> 2.18.0
>

Reviewed-by: Bartosz Golaszewski 

Re: [PATCH 01/14] gpio: pca953x: Deduplicate the bank_size

[Bartosz Golaszewski]

niedz., 2 gru 2018 o 20:36 Marek Vasut  napisał(a):
>
> The bank_size = fls(...) code was duplicated in the driver 5 times,
> pull it into separate function.
>

Shouldn't it be bank_shift in the commit message?

Bart

> Signed-off-by: Marek Vasut 
> Cc: Linus Walleij 
> Cc: Bartosz Golaszewski 
> ---
>  drivers/gpio/gpio-pca953x.c | 18 +++---
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 023a32cfac42..31e3b1b52330 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -159,11 +159,16 @@ struct pca953x_chip {
> int (*read_regs)(struct pca953x_chip *, int, u8 *);
>  };
>
> +static int pca953x_bank_shift(struct pca953x_chip *chip)
> +{
> +   return fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +}
> +
>  static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
> int off)
>  {
> int ret;
> -   int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +   int bank_shift = pca953x_bank_shift(chip);
> int offset = off / BANK_SZ;
>
> ret = i2c_smbus_read_byte_data(chip->client,
> @@ -182,7 +187,7 @@ static int pca953x_write_single(struct pca953x_chip 
> *chip, int reg, u32 val,
> int off)
>  {
> int ret;
> -   int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +   int bank_shift = pca953x_bank_shift(chip);
> int offset = off / BANK_SZ;
>
> ret = i2c_smbus_write_byte_data(chip->client,
> @@ -221,7 +226,7 @@ static int pca957x_write_regs_16(struct pca953x_chip 
> *chip, int reg, u8 *val)
>
>  static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>  {
> -   int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +   int bank_shift = pca953x_bank_shift(chip);
> int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
> int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>
> @@ -265,7 +270,7 @@ static int pca953x_read_regs_16(struct pca953x_chip 
> *chip, int reg, u8 *val)
>
>  static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>  {
> -   int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +   int bank_shift = pca953x_bank_shift(chip);
> int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
> int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>
> @@ -402,13 +407,12 @@ static void pca953x_gpio_set_multiple(struct gpio_chip 
> *gc,
>   unsigned long *mask, unsigned long 
> *bits)
>  {
> struct pca953x_chip *chip = gpiochip_get_data(gc);
> +   int bank_shift = pca953x_bank_shift(chip);
> unsigned int bank_mask, bank_val;
> -   int bank_shift, bank;
> +   int bank;
> u8 reg_val[MAX_BANK];
> int ret;
>
> -   bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> -
> mutex_lock(>i2c_lock);
> memcpy(reg_val, chip->reg_output, NBANK(chip));
> for (bank = 0; bank < NBANK(chip); bank++) {
> --
> 2.18.0
>

Re: [PATCH 1/2] clocksource/drivers/ostm: Delay driver registration

[Bartosz Golaszewski]

2018-08-30 11:16 GMT+02:00 Daniel Lezcano :
>
> [Added Arnd Bergmann, Bartosz Golaszewski and Mark Brown]
>
> On 30/08/2018 10:48, Geert Uytterhoeven wrote:
>> Hi Daniel,
>
> [ ... ]
>
>>> Yeah, I got this point. But it is the meaning of your sentence: "...
>>> which causes issues with complex dependencies.".
>>>
>>> It is ambiguous *what* causes the issues.
>>>
>>> Did you meant an attempt was done to support EPROBE_DEFER with
>>> *_OF_DECLARE but caused too much issues with the complex dependencies?
>>>
>>> Or the current situation is causing too much issues with the complex
>>> dependencies?
>>>
>>> (I know the latter is true, it is about the meaning of the sentence).
>>
>> I meant the latter.
>>
>> AFAIK no attempt was done to support EPROBE_DEFER with *_OF_DECLARE.
>> IMHO it would be pointless, as it would be much easier to just switch to real
>> platform drivers.
>
> May be, may be not.
>
> From your point of view, the change is simple because it touches only a
> single driver.
>
> From my point of view, the change implies a split in the approach while
> I'm trying to unify the drivers little by little and there are hundred
> of them.
>
> It is not the first time we face this situation and Bartosz Golaszewski
> has a similar problem [1].
>

Hi,

thanks for Cc'in me on that.

This was my latest proposal for early platform drivers:

https://lkml.org/lkml/2018/5/11/488

I still intend on continuing this work, I just don't have the time right now.

Best regards,
Bartosz Golaszewski

> We have all the frameworks we need to solve this properly but I would
> like something we can propagate to all drivers (OF and !OF) so we end up
> with unified code.
>
> It is time we clearly state the dependency issues and we find a proper
> way to solve it.
>
>
>
>
> [1] https://lkml.org/lkml/2018/4/26/657
>
> --
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>

Re: [PATCH v2 5/5] dt-bindings: at24: add bindings for Rohm BR24T01

[Bartosz Golaszewski]

2018-01-29 16:45 GMT+01:00 Ulrich Hecht :
> Both manufacturer and name variant.
>
> Signed-off-by: Ulrich Hecht 
> ---
>  Documentation/devicetree/bindings/eeprom/at24.txt | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/eeprom/at24.txt 
> b/Documentation/devicetree/bindings/eeprom/at24.txt
> index 1812c84..dd29937 100644
> --- a/Documentation/devicetree/bindings/eeprom/at24.txt
> +++ b/Documentation/devicetree/bindings/eeprom/at24.txt
> @@ -40,6 +40,7 @@ Required properties:
>  "microchip",
>  "ramtron",
>  "renesas",
> +"rohm",
>  "nxp",
>  "st",
>
> @@ -47,6 +48,7 @@ Required properties:
>  variants of the above. Known such exceptions are listed 
> below:
>
>  "renesas,r1ex24002" - the fallback is "atmel,24c02"
> +"rohm,br24t01" - the fallback is "atmel,24c01"
>
>- reg: The I2C address of the EEPROM.
>
> --
> 2.7.4
>

Applied to for-next, thanks.

Bart

Re: [PATCH v2 5/5] dt-bindings: at24: add bindings for Rohm BR24T01

[Bartosz Golaszewski]

2018-02-05 7:07 GMT+01:00 Rob Herring :
> On Mon, Jan 29, 2018 at 04:45:48PM +0100, Ulrich Hecht wrote:
>> Both manufacturer and name variant.
>>
>> Signed-off-by: Ulrich Hecht 
>> ---
>>  Documentation/devicetree/bindings/eeprom/at24.txt | 2 ++
>>  1 file changed, 2 insertions(+)
>
> Reviewed-by: Rob Herring 
>
>>
>> diff --git a/Documentation/devicetree/bindings/eeprom/at24.txt 
>> b/Documentation/devicetree/bindings/eeprom/at24.txt
>> index 1812c84..dd29937 100644
>> --- a/Documentation/devicetree/bindings/eeprom/at24.txt
>> +++ b/Documentation/devicetree/bindings/eeprom/at24.txt
>> @@ -40,6 +40,7 @@ Required properties:
>>  "microchip",
>>  "ramtron",
>>  "renesas",
>> +"rohm",
>
> This is going to comflict with a patch sorting these IIRC. So they
> should go thru the same tree.
>
>>  "nxp",
>>  "st",
>>
>> @@ -47,6 +48,7 @@ Required properties:
>>  variants of the above. Known such exceptions are listed 
>> below:
>>
>>  "renesas,r1ex24002" - the fallback is "atmel,24c02"
>> +"rohm,br24t01" - the fallback is "atmel,24c01"
>>
>>- reg: The I2C address of the EEPROM.
>>
>> --
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

AT24 patches go through Wolfram's tree, but since he already sent his
PR for 4.16, I can pick them up for 4.17 (except for Peter's sorting
patch which should go in after 4.16-rc1).

Thanks,
Bartosz

Re: [PATCH v2 5/5] dt-bindings: at24: add bindings for Rohm BR24T01

[Bartosz Golaszewski]

2018-01-30 11:44 GMT+01:00 Wolfram Sang <w...@the-dreams.de>:
> On Mon, Jan 29, 2018 at 04:45:48PM +0100, Ulrich Hecht wrote:
>> Both manufacturer and name variant.
>>
>> Signed-off-by: Ulrich Hecht <ulrich.hecht+rene...@gmail.com>
>
> Reviewed-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
>

Acked-by: Bartosz Golaszewski <b...@bgdev.pl>

Re: [PATCH] dt-bindings: i2c: eeprom: Add Renesas R1EX24128

[Bartosz Golaszewski]

2017-11-27 19:05 GMT+01:00 Wolfram Sang <w...@the-dreams.de>:
> Bartosz,
>
>>  Documentation/devicetree/bindings/eeprom/eeprom.txt | 2 +-
>
> I just noticed this file is missing in the AT24 entry in MAINTAINERS. I
> wanted to prepare a patch for that but then I was wondering if the name
> "eeprom.txt" is really suitable? I have doubts. Would "at24.txt" be a
> better name maybe? Rob?
>
> Kind regards,
>
>Wolfram
>

Yes, it definitely should be called at24.txt - especially that we have
the legacy eeprom module in drivers/misc/eeprom/eeprom.c which has
nothing to do with the bindings in eeprom.txt.

Acked-in-advance-by: Bartosz Golaszewski <b...@bgdev.pl>

Thanks,
Bartosz