[bug report] i2c: mv64xxx: Add runtime PM support

[bug report] i2c: mv64xxx: Add runtime PM support

[Dan Carpenter]

Hello Samuel Holland,

The patch e5c02cf54154: "i2c: mv64xxx: Add runtime PM support" from
Jan 3, 2021, leads to the following static checker warning:

	drivers/i2c/busses/i2c-mv64xxx.c:816 mv64xxx_of_config()
	warn: 'drv_data->clk' isn't an ERR_PTR

drivers/i2c/busses/i2c-mv64xxx.c
   803  static int
   804  mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
   805                    struct device *dev)
   806  {
   807          const struct of_device_id *device;
   808          struct device_node *np = dev->of_node;
   809          u32 bus_freq, tclk;
   810          int rc = 0;
   811  
   812          /* CLK is mandatory when using DT to describe the i2c bus. We
   813           * need to know tclk in order to calculate bus clock
   814           * factors.
   815           */
   816          if (IS_ERR(drv_data->clk)) {

This check used to be correct, but now if it's an error pointer in probe
then we set it to NULL.

   817                  rc = -ENODEV;
   818                  goto out;
   819          }
   820          tclk = clk_get_rate(drv_data->clk);

The result is that "tclk" is zero.  So probably the correct fix is to
change the IS_ERR() check to a NULL check?  zero rate seems useless.

   821  
   822          if (of_property_read_u32(np, "clock-frequency", &bus_freq))
   823                  bus_freq = I2C_MAX_STANDARD_MODE_FREQ; /* 100kHz by default */
   824  
   825          if (of_device_is_compatible(np, "allwinner,sun4i-a10-i2c") ||
   826              of_device_is_compatible(np, "allwinner,sun6i-a31-i2c"))
   827                  drv_data->clk_n_base_0 = true;
   828  
   829          if (!mv64xxx_find_baud_factors(drv_data, bus_freq, tclk)) {
   830                  rc = -EINVAL;
   831                  goto out;
   832          }
   833  

regards,
dan carpenter

Re: [bug report] i2c: mv64xxx: Add runtime PM support

[Samuel Holland]

On 2/3/21 8:41 AM, Dan Carpenter wrote:
> Hello Samuel Holland,
> 
> The patch e5c02cf54154: "i2c: mv64xxx: Add runtime PM support" from
> Jan 3, 2021, leads to the following static checker warning:
> 
> 	drivers/i2c/busses/i2c-mv64xxx.c:816 mv64xxx_of_config()
> 	warn: 'drv_data->clk' isn't an ERR_PTR
> 
> drivers/i2c/busses/i2c-mv64xxx.c
>    803  static int
>    804  mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>    805                    struct device *dev)
>    806  {
>    807          const struct of_device_id *device;
>    808          struct device_node *np = dev->of_node;
>    809          u32 bus_freq, tclk;
>    810          int rc = 0;
>    811  
>    812          /* CLK is mandatory when using DT to describe the i2c bus. We
>    813           * need to know tclk in order to calculate bus clock
>    814           * factors.
>    815           */
>    816          if (IS_ERR(drv_data->clk)) {
> 
> This check used to be correct, but now if it's an error pointer in probe
> then we set it to NULL.
> 
>    817                  rc = -ENODEV;
>    818                  goto out;
>    819          }
>    820          tclk = clk_get_rate(drv_data->clk);
> 
> The result is that "tclk" is zero.  So probably the correct fix is to
> change the IS_ERR() check to a NULL check?  zero rate seems useless.

Yes, that looks like the correct fix. I sent a patch.

Thank you!
Samuel

>    821  
>    822          if (of_property_read_u32(np, "clock-frequency", &bus_freq))
>    823                  bus_freq = I2C_MAX_STANDARD_MODE_FREQ; /* 100kHz by default */
>    824  
>    825          if (of_device_is_compatible(np, "allwinner,sun4i-a10-i2c") ||
>    826              of_device_is_compatible(np, "allwinner,sun6i-a31-i2c"))
>    827                  drv_data->clk_n_base_0 = true;
>    828  
>    829          if (!mv64xxx_find_baud_factors(drv_data, bus_freq, tclk)) {
>    830                  rc = -EINVAL;
>    831                  goto out;
>    832          }
>    833  
> 
> regards,
> dan carpenter
>